fix: Prevent download of view-only files

Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>

Author: Koc

apps/dav/lib/Connector/Sabre/ZipFolderPlugin.php

@@ -137,7 +137,14 @@ public function handleDownload(Request $request, Response $response): ?bool {
 		}
 
 		$folder = $node->getNode();
-		$event = new BeforeZipCreatedEvent($folder, $files);
+		$nodes = empty($files) ? $folder->getDirectoryListing() : [];
+		foreach ($files as $path) {
+			$child = $node->getChild($path);
+			assert($child instanceof Node);
+			$nodes[] = $child->getNode();
+		}
+
+		$event = new BeforeZipCreatedEvent($folder, $files, $nodes);
 		$this->eventDispatcher->dispatchTyped($event);
 		if ((!$event->isSuccessful()) || $event->getErrorMessage() !== null) {
 			$errorMessage = $event->getErrorMessage();
@@ -150,12 +157,7 @@ public function handleDownload(Request $request, Response $response): ?bool {
 			throw new Forbidden($errorMessage);
 		}
 
-		$content = empty($files) ? $folder->getDirectoryListing() : [];
-		foreach ($files as $path) {
-			$child = $node->getChild($path);
-			assert($child instanceof Node);
-			$content[] = $child->getNode();
-		}
+		$nodes = $event->getNodes();
 
 		$archiveName = $folder->getName();
 		if (count(explode('/', trim($folder->getPath(), '/'), 3)) === 2) {
@@ -169,13 +171,13 @@ public function handleDownload(Request $request, Response $response): ?bool {
 			$rootPath = dirname($folder->getPath());
 		}
 
-		$streamer = new Streamer($tarRequest, -1, count($content), $this->timezoneFactory);
+		$streamer = new Streamer($tarRequest, -1, count($nodes), $this->timezoneFactory);
 		$streamer->sendHeaders($archiveName);
 		// For full folder downloads we also add the folder itself to the archive
 		if (empty($files)) {
 			$streamer->addEmptyDir($archiveName);
 		}
-		foreach ($content as $node) {
+		foreach ($nodes as $node) {
 			$this->streamNode($streamer, $node, $rootPath);
 		}
 		$streamer->finalize();

apps/files_sharing/lib/Listener/BeforeZipCreatedListener.php

@@ -32,28 +32,38 @@ public function handle(Event $event): void {
 			return;
 		}
 
-		$dir = $event->getDirectory();
-		$files = $event->getFiles();
+		$user = $this->userSession->getUser();
+		if (!$user) {
+			// fixme: do we need check anything for anonymous users?
+			$event->setSuccessful(true);
+		}
 
-		$pathsToCheck = [];
-		foreach ($files as $file) {
+		$userFolder = $this->rootFolder->getUserFolder($user->getUID());
+		$viewOnlyHandler = new ViewOnly($userFolder);
+		$this->checkSelectedFilesCanBeDownloaded($event, $viewOnlyHandler);
+		$this->checkNodesCanBeDownloaded($event, $viewOnlyHandler);
+	}
+
+	private function checkSelectedFilesCanBeDownloaded(BeforeZipCreatedEvent $event, ViewOnly $viewOnlyHandler): void {
+		// Check only for user/group shares. Don't restrict e.g. share links
+		$dir = $event->getDirectory();
+		$pathsToCheck = [$dir];
+		foreach ($event->getFiles() as $file) {
 			$pathsToCheck[] = $dir . '/' . $file;
 		}
 
-		// Check only for user/group shares. Don't restrict e.g. share links
-		$user = $this->userSession->getUser();
-		if ($user) {
-			$viewOnlyHandler = new ViewOnly(
-				$this->rootFolder->getUserFolder($user->getUID())
-			);
-			if (!$viewOnlyHandler->check($pathsToCheck)) {
-				$event->setErrorMessage('Access to this resource or one of its sub-items has been denied.');
-				$event->setSuccessful(false);
-			} else {
-				$event->setSuccessful(true);
-			}
+		if (!$viewOnlyHandler->check($pathsToCheck)) {
+			$event->setErrorMessage('Access to this resource or one of its sub-items has been denied.');
+			$event->setSuccessful(false);
 		} else {
 			$event->setSuccessful(true);
 		}
 	}
+
+	private function checkNodesCanBeDownloaded(BeforeZipCreatedEvent $event, ViewOnly $viewOnlyHandler): void {
+		$nodes = array_values(array_filter($event->getNodes(),
+			static fn ($node) => $viewOnlyHandler->checkNode($node)));
+
+		$event->setNodes($nodes);
+	}
 }

apps/files_sharing/lib/ViewOnly.php

@@ -23,7 +23,17 @@ public function __construct(
 	) {
 	}
 
+	public function checkNode(Node $node): bool {
+		return match (true) {
+			// access to filecache is expensive in the loop
+			$node instanceof File => $this->checkFileInfo($node),
+			// get directory content is a rather cheap query
+			$node instanceof Folder => $this->dirRecursiveCheck($node),
+		};
+	}
+
 	/**
+	 * // fixme: inline this method to listener
 	 * @param string[] $pathsToCheck
 	 * @return bool
 	 */
@@ -32,17 +42,7 @@ public function check(array $pathsToCheck): bool {
 		foreach ($pathsToCheck as $file) {
 			try {
 				$info = $this->userFolder->get($file);
-				if ($info instanceof File) {
-					// access to filecache is expensive in the loop
-					if (!$this->checkFileInfo($info)) {
-						return false;
-					}
-				} elseif ($info instanceof Folder) {
-					// get directory content is rather cheap query
-					if (!$this->dirRecursiveCheck($info)) {
-						return false;
-					}
-				}
+				return $this->checkNode($info);
 			} catch (NotFoundException $e) {
 				continue;
 			}

lib/public/Files/Events/BeforeZipCreatedEvent.php

@@ -9,6 +9,7 @@
 
 namespace OCP\Files\Events;
 
+use OCP\Files\Node;
 use OCP\EventDispatcher\Event;
 use OCP\Files\Folder;
 
@@ -28,12 +29,14 @@ class BeforeZipCreatedEvent extends Event {
 
 	/**
 	 * @param list<string> $files
+	 * @param list<Node> $nodes
 	 * @since 25.0.0
 	 * @since 31.0.0 support `OCP\Files\Folder` as `$directory` parameter - passing a string is deprecated now
 	 */
 	public function __construct(
 		string|Folder $directory,
 		private array $files,
+		private array $nodes = [],
 	) {
 		parent::__construct();
 		if ($directory instanceof Folder) {
@@ -65,6 +68,20 @@ public function getFiles(): array {
 		return $this->files;
 	}
 
+	/**
+	 * @return Node[]
+	 */
+	public function getNodes(): array {
+		return $this->nodes;
+	}
+
+	/**
+	 * @param Node[] $nodes
+	 */
+	public function setNodes(array $nodes): void {
+		$this->nodes = $nodes;
+	}
+
 	/**
 	 * @since 25.0.0
 	 */