fix: Throw exception when trying to access invalid Node

Instead of just ignoring it and possibly creating subtle bugs.

Signed-off-by: Carl Schwan <[email protected]>

Author: CarlSchwan

apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php

@@ -336,9 +336,12 @@ private function shareAccepted($id, array $notification) {
 	 * @param IShare $share
 	 * @throws ShareNotFound
 	 */
-	protected function executeAcceptShare(IShare $share) {
+	protected function executeAcceptShare(IShare $share): void {
 		try {
-			$fileId = $share->getNode()->getId() ?? -1;
+			$fileId = $share->getNode()->getId();
+			if ($fileId == null) {
+				throw new \LogicException('Invalid node for share');
+			}
 			[$file, $link] = $this->getFile($this->getCorrectUid($share), $fileId);
 		} catch (\Exception $e) {
 			throw new ShareNotFound();

apps/files/lib/Listener/SyncLivePhotosListener.php

@@ -134,14 +134,18 @@ private function handleMove(Node $sourceFile, Node $targetFile, Node $peerFile):
 			return;
 		}
 
-		$this->pendingRenames[] = $sourceFile->getId() ?? -1;
+		$sourceFileId = $sourceFile->getId();
+		if ($sourceFileId === null) {
+			throw new \LogicException('Invalid source file given with a null id');
+		}
+		$this->pendingRenames[] = $sourceFileId;
 		try {
 			$peerFile->move($targetParent->getPath() . '/' . $peerTargetName);
 		} catch (\Throwable $ex) {
 			throw new AbortedEventException($ex->getMessage());
 		}
 
-		$this->pendingRenames = array_diff($this->pendingRenames, [$sourceFile->getId() ?? -1]);
+		$this->pendingRenames = array_diff($this->pendingRenames, [$sourceFileId]);
 	}
 
 
@@ -163,15 +167,26 @@ private function handleCopy(File $sourceFile, File $targetFile, File $peerFile):
 			$targetPeerFile = $peerFile->copy($targetParent->getPath() . '/' . $peerTargetName);
 		}
 
+		$targetFileId = $targetFile->getId();
+		if ($targetFileId === null) {
+			throw new \LogicException('Invalid target file given with a null id');
+		}
+
+		$targetPeerFileId = $targetPeerFile->getId();
+		if ($targetPeerFileId === null) {
+			throw new \LogicException('Invalid target peer file given with a null id');
+		}
+
 		/** @var FilesMetadata $targetMetadata */
-		$targetMetadata = $this->filesMetadataManager->getMetadata($targetFile->getId(), true);
+		$targetMetadata = $this->filesMetadataManager->getMetadata($targetFileId, true);
 		$targetMetadata->setStorageId($targetFile->getStorage()->getCache()->getNumericStorageId());
-		$targetMetadata->setString('files-live-photo', (string)$targetPeerFile->getId());
+		$targetMetadata->setString('files-live-photo', (string)$targetPeerFileId);
 		$this->filesMetadataManager->saveMetadata($targetMetadata);
+
 		/** @var FilesMetadata $peerMetadata */
-		$peerMetadata = $this->filesMetadataManager->getMetadata($targetPeerFile->getId(), true);
+		$peerMetadata = $this->filesMetadataManager->getMetadata($targetPeerFileId, true);
 		$peerMetadata->setStorageId($targetPeerFile->getStorage()->getCache()->getNumericStorageId());
-		$peerMetadata->setString('files-live-photo', (string)$targetFile->getId());
+		$peerMetadata->setString('files-live-photo', (string)$targetFileId);
 		$this->filesMetadataManager->saveMetadata($peerMetadata);
 	}
 
@@ -185,14 +200,22 @@ private function handleCopy(File $sourceFile, File $targetFile, File $peerFile):
 	private function handleDeletion(BeforeNodeDeletedEvent $event, Node $peerFile): void {
 		$deletedFile = $event->getNode();
 		if ($deletedFile->getMimetype() === 'video/quicktime') {
-			if (isset($this->pendingDeletion[$peerFile->getId() ?? -1])) {
-				unset($this->pendingDeletion[$peerFile->getId() ?? -1]);
+			$peerFileId = $peerFile->getId();
+			if ($peerFileId === null) {
+				throw new \LogicException('Invalid peer file given with a null id');
+			}
+			if (isset($this->pendingDeletion[$peerFileId])) {
+				unset($this->pendingDeletion[$peerFileId]);
 				return;
 			} else {
 				throw new AbortedEventException('Cannot delete the video part of a live photo');
 			}
 		} else {
-			$this->pendingDeletion[$deletedFile->getId() ?? -1] = true;
+			$deletedFileId = $peerFile->getId();
+			if ($deletedFileId === null) {
+				throw new \LogicException('Invalid deleted file given with a null id');
+			}
+			$this->pendingDeletion[$deletedFileId] = true;
 			try {
 				$peerFile->delete();
 			} catch (\Throwable $ex) {
@@ -243,7 +266,7 @@ private function handleCopyRecursive(Event $event, Node $sourceNode, Node $targe
 			$this->pendingCopies[] = $peerFileId;
 			if ($event instanceof BeforeNodeCopiedEvent) {
 				$this->runMoveOrCopyChecks($sourceNode, $targetNode, $peerFile);
-			} elseif ($event instanceof NodeCopiedEvent) {
+			} elseif ($event instanceof NodeCopiedEvent && $peerFile instanceof File) {
 				$this->handleCopy($sourceNode, $targetNode, $peerFile);
 			}
 			$this->pendingCopies = array_diff($this->pendingCopies, [$peerFileId]);

apps/files_trashbin/lib/Listeners/SyncLivePhotosListener.php

@@ -72,8 +72,13 @@ private function handleRestore(BeforeNodeRestoredEvent $event, Node $peerFile):
 		$sourceFile = $event->getSource();
 
 		if ($sourceFile->getMimetype() === 'video/quicktime') {
-			if (isset($this->pendingRestores[$peerFile->getId() ?? -1])) {
-				unset($this->pendingRestores[$peerFile->getId() ?? -1]);
+			$peerFileId = $peerFile->getId();
+			if ($peerFileId === null) {
+				throw new \LogicException('Invalid peer file given with a null id');
+			}
+
+			if (isset($this->pendingRestores[$peerFileId])) {
+				unset($this->pendingRestores[$peerFileId]);
 				return;
 			} else {
 				$event->abortOperation(new NotPermittedException('Cannot restore the video part of a live photo'));
@@ -97,7 +102,12 @@ private function handleRestore(BeforeNodeRestoredEvent $event, Node $peerFile):
 				$event->abortOperation(new NotFoundException("Couldn't find peer file in trashbin"));
 			}
 
-			$this->pendingRestores[$sourceFile->getId() ?? -1] = true;
+			$sourceFileId = $sourceFile->getId();
+			if ($sourceFileId === null) {
+				throw new \LogicException('Invalid source file given with a null id');
+			}
+
+			$this->pendingRestores[$sourceFileId] = true;
 			try {
 				$this->trashManager->restoreItem($trashItem);
 			} catch (\Throwable $ex) {

apps/files_versions/lib/Listener/FileEventsListener.php

@@ -8,7 +8,6 @@
 namespace OCA\Files_Versions\Listener;
 
 use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
-use OC\DB\Exceptions\DbalException;
 use OC\Files\Filesystem;
 use OC\Files\Mount\MoveableMount;
 use OC\Files\Node\NonExistingFile;
@@ -122,7 +121,12 @@ public function pre_touch_hook(Node $node): void {
 			return;
 		}
 
-		$this->nodesTouched[$node->getId() ?? -1] = $node;
+		$nodeId = $node->getId();
+		if ($nodeId === null) {
+			return;
+		}
+
+		$this->nodesTouched[$nodeId] = $node;
 	}
 
 	public function touch_hook(Node $node): void {
@@ -142,13 +146,17 @@ public function touch_hook(Node $node): void {
 			return;
 		}
 
-		$previousNode = $this->nodesTouched[$node->getId()] ?? null;
+		$nodeId = $node->getId();
+		if ($nodeId === null) {
+			throw new \LogicException('Invalid node given');
+		}
+		$previousNode = $this->nodesTouched[$nodeId] ?? null;
 
 		if ($previousNode === null) {
 			return;
 		}
 
-		unset($this->nodesTouched[$node->getId()]);
+		unset($this->nodesTouched[$nodeId]);
 
 		try {
 			if ($node instanceof File && $this->versionManager instanceof INeedSyncVersionBackend) {
@@ -157,15 +165,15 @@ public function touch_hook(Node $node): void {
 				// We update the timestamp of the version entity associated with the previousNode.
 				$this->versionManager->updateVersionEntity($node, $revision, ['timestamp' => $node->getMTime()]);
 			}
-		} catch (DbalException $ex) {
+		} catch (Exception $ex) {
 			// Ignore UniqueConstraintViolationException, as we are probably in the middle of a rollback
-			// Where the previous node would temporary have the mtime of the old version, so the rollback touches it to fix it.
-			if (!($ex->getPrevious() instanceof UniqueConstraintViolationException)) {
+			// Where the previous node would temporarily have the mtime of the old version, so the rollback touches it to fix it.
+			if ($ex->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) {
 				throw $ex;
 			}
 		} catch (DoesNotExistException $ex) {
 			// Ignore DoesNotExistException, as we are probably in the middle of a rollback
-			// Where the previous node would temporary have a wrong mtime, so the rollback touches it to fix it.
+			// Where the previous node would temporarily have a wrong mtime, so the rollback touches it to fix it.
 		}
 	}
 
@@ -208,8 +216,13 @@ public function write_hook(Node $node): void {
 		$path = $this->getPathForNode($node);
 		$result = Storage::store($path);
 
+		$nodeId = $node->getId();
+		if ($nodeId === null) {
+			throw new \LogicException('Invalid node given');
+		}
+
 		// Store the result of the version creation so it can be used in post_write_hook.
-		$this->writeHookInfo[$node->getId() ?? -1] = [
+		$this->writeHookInfo[$nodeId] = [
 			'previousNode' => $node,
 			'versionCreated' => $result !== false
 		];
@@ -235,7 +248,12 @@ public function post_write_hook(Node $node): void {
 			return;
 		}
 
-		$writeHookInfo = $this->writeHookInfo[$node->getId()] ?? null;
+		$nodeId = $node->getId();
+		if ($nodeId === null) {
+			throw new \LogicException('Invalid node given');
+		}
+
+		$writeHookInfo = $this->writeHookInfo[$nodeId] ?? null;
 
 		if ($writeHookInfo === null) {
 			return;