Merge pull request #50430 from nextcloud/fix/harden-thumbnail-endpoint

files: harden thumbnail endpoint
nextcloud · Jan 26, 2025 · 8981f32 · 8981f32
2 parents fba7ef9 + 3c357f8
commit 8981f32
Show file tree

Hide file tree

Showing 3 changed files with 104 additions and 6 deletions.
diff --git a/apps/files/lib/Controller/ApiController.php b/apps/files/lib/Controller/ApiController.php
@@ -29,8 +29,11 @@
 use OCP\AppFramework\Http\StreamResponse;
 use OCP\Files\File;
 use OCP\Files\Folder;
+use OCP\Files\InvalidPathException;
 use OCP\Files\IRootFolder;
 use OCP\Files\NotFoundException;
+use OCP\Files\NotPermittedException;
+use OCP\Files\Storage\ISharedStorage;
 use OCP\Files\StorageNotAvailableException;
 use OCP\IConfig;
 use OCP\IL10N;
@@ -72,6 +75,7 @@ public function __construct(
 	 * Gets a thumbnail of the specified file
 	 *
 	 * @since API version 1.0
+	 * @deprecated 32.0.0 Use the preview endpoint provided by core instead
 	 *
 	 * @param int $x Width of the thumbnail
 	 * @param int $y Height of the thumbnail
@@ -92,20 +96,28 @@ public function getThumbnail($x, $y, $file) {
 		}
 
 		try {
-			$file = $this->userFolder->get($file);
-			if ($file instanceof Folder) {
+			$file = $this->userFolder?->get($file);
+			if ($file === null
+				|| !($file instanceof File)
+				|| ($file->getId() <= 0)
+			) {
 				throw new NotFoundException();
 			}
 
-			if ($file->getId() <= 0) {
-				return new DataResponse(['message' => 'File not found.'], Http::STATUS_NOT_FOUND);
+			// Validate the user is allowed to download the file (preview is some kind of download)
+			$storage = $file->getStorage();
+			if ($storage->instanceOfStorage(ISharedStorage::class)) {
+				/** @var ISharedStorage $storage */
+				$attributes = $storage->getShare()->getAttributes();
+				if ($attributes !== null && $attributes->getAttribute('permissions', 'download') === false) {
+					throw new NotFoundException();
+				}
 			}
 
-			/** @var File $file */
 			$preview = $this->previewManager->getPreview($file, $x, $y, true);
 
 			return new FileDisplayResponse($preview, Http::STATUS_OK, ['Content-Type' => $preview->getMimeType()]);
-		} catch (NotFoundException $e) {
+		} catch (NotFoundException|NotPermittedException|InvalidPathException) {
 			return new DataResponse(['message' => 'File not found.'], Http::STATUS_NOT_FOUND);
 		} catch (\Exception $e) {
 			return new DataResponse([], Http::STATUS_BAD_REQUEST);

diff --git a/apps/files/openapi.json b/apps/files/openapi.json
@@ -429,6 +429,7 @@
             "get": {
                 "operationId": "api-get-thumbnail",
                 "summary": "Gets a thumbnail of the specified file",
+                "deprecated": true,
                 "tags": [
                     "api"
                 ],

diff --git a/apps/files/tests/Controller/ApiControllerTest.php b/apps/files/tests/Controller/ApiControllerTest.php
@@ -19,14 +19,18 @@
 use OCP\Files\IRootFolder;
 use OCP\Files\NotFoundException;
 use OCP\Files\SimpleFS\ISimpleFile;
+use OCP\Files\Storage\ISharedStorage;
+use OCP\Files\Storage\IStorage;
 use OCP\Files\StorageNotAvailableException;
 use OCP\IConfig;
 use OCP\IL10N;
 use OCP\IPreview;
 use OCP\IRequest;
 use OCP\IUser;
 use OCP\IUserSession;
+use OCP\Share\IAttributes;
 use OCP\Share\IManager;
+use OCP\Share\IShare;
 use Psr\Log\LoggerInterface;
 use Test\TestCase;
 
@@ -173,8 +177,12 @@ public function testGetThumbnailInvalidSize(): void {
 	}
 
 	public function testGetThumbnailInvalidImage(): void {
+		$storage = $this->createMock(IStorage::class);
+		$storage->method('instanceOfStorage')->with(ISharedStorage::class)->willReturn(false);
+
 		$file = $this->createMock(File::class);
 		$file->method('getId')->willReturn(123);
+		$file->method('getStorage')->willReturn($storage);
 		$this->userFolder->method('get')
 			->with($this->equalTo('unknown.jpg'))
 			->willReturn($file);
@@ -196,9 +204,86 @@ public function testGetThumbnailInvalidPartFile(): void {
 		$this->assertEquals($expected, $this->apiController->getThumbnail(10, 10, 'unknown.jpg'));
 	}
 
+	public function testGetThumbnailSharedNoDownload(): void {
+		$attributes = $this->createMock(IAttributes::class);
+		$attributes->expects(self::once())
+			->method('getAttribute')
+			->with('permissions', 'download')
+			->willReturn(false);
+
+		$share = $this->createMock(IShare::class);
+		$share->expects(self::once())
+			->method('getAttributes')
+			->willReturn($attributes);
+
+		$storage = $this->createMock(ISharedStorage::class);
+		$storage->expects(self::once())
+			->method('instanceOfStorage')
+			->with(ISharedStorage::class)
+			->willReturn(true);
+		$storage->expects(self::once())
+			->method('getShare')
+			->willReturn($share);
+
+		$file = $this->createMock(File::class);
+		$file->method('getId')->willReturn(123);
+		$file->method('getStorage')->willReturn($storage);
+
+		$this->userFolder->method('get')
+			->with('unknown.jpg')
+			->willReturn($file);
+
+		$this->preview->expects($this->never())
+			->method('getPreview');
+
+		$expected = new DataResponse(['message' => 'File not found.'], Http::STATUS_NOT_FOUND);
+		$this->assertEquals($expected, $this->apiController->getThumbnail(10, 10, 'unknown.jpg'));
+	}
+
+	public function testGetThumbnailShared(): void {
+		$share = $this->createMock(IShare::class);
+		$share->expects(self::once())
+			->method('getAttributes')
+			->willReturn(null);
+
+		$storage = $this->createMock(ISharedStorage::class);
+		$storage->expects(self::once())
+			->method('instanceOfStorage')
+			->with(ISharedStorage::class)
+			->willReturn(true);
+		$storage->expects(self::once())
+			->method('getShare')
+			->willReturn($share);
+
+		$file = $this->createMock(File::class);
+		$file->method('getId')->willReturn(123);
+		$file->method('getStorage')->willReturn($storage);
+
+		$this->userFolder->method('get')
+			->with($this->equalTo('known.jpg'))
+			->willReturn($file);
+		$preview = $this->createMock(ISimpleFile::class);
+		$preview->method('getName')->willReturn('my name');
+		$preview->method('getMTime')->willReturn(42);
+		$this->preview->expects($this->once())
+			->method('getPreview')
+			->with($this->equalTo($file), 10, 10, true)
+			->willReturn($preview);
+
+		$ret = $this->apiController->getThumbnail(10, 10, 'known.jpg');
+
+		$this->assertEquals(Http::STATUS_OK, $ret->getStatus());
+		$this->assertInstanceOf(FileDisplayResponse::class, $ret);
+	}
+
 	public function testGetThumbnail(): void {
+		$storage = $this->createMock(IStorage::class);
+		$storage->method('instanceOfStorage')->with(ISharedStorage::class)->willReturn(false);
+
 		$file = $this->createMock(File::class);
 		$file->method('getId')->willReturn(123);
+		$file->method('getStorage')->willReturn($storage);
+
 		$this->userFolder->method('get')
 			->with($this->equalTo('known.jpg'))
 			->willReturn($file);