fix(files): Harden thumbnail endpoint

- Catch all thrown exceptions and handle in such a way you do not get
  information about forbidden files.
- Resepect download permissions of shares.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

2 files changed, 102 insertions, 6 deletions

diff --git a/apps/files/lib/Controller/ApiController.php b/apps/files/lib/Controller/ApiController.php
index dba732c9c52..9b9a0980389 100644
--- a/apps/files/lib/Controller/ApiController.php
+++ b/apps/files/lib/Controller/ApiController.php
@@ -29,8 +29,11 @@ use OCP\AppFramework\Http\Response;
 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;
@@ -92,20 +95,28 @@ class ApiController extends Controller {
 		}
 
 		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/tests/Controller/ApiControllerTest.php b/apps/files/tests/Controller/ApiControllerTest.php
index 2b1ba34f98e..429d3c06f66 100644
--- a/apps/files/tests/Controller/ApiControllerTest.php
+++ b/apps/files/tests/Controller/ApiControllerTest.php
@@ -19,6 +19,8 @@ use OCP\Files\Folder;
 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;
@@ -26,7 +28,9 @@ 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 @@ class ApiControllerTest extends TestCase {
 	}
 
 	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 @@ class ApiControllerTest extends TestCase {
 		$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);