fix(files_sharing): validate input in PublicPreviewController#getPreview

apps/files_sharing/lib/Controller/PublicPreviewController.php

@@ -16,6 +16,7 @@
 use OCP\AppFramework\Http\RedirectResponse;
 use OCP\AppFramework\PublicShareController;
 use OCP\Constants;
+use OCP\Files\File;
 use OCP\Files\Folder;
 use OCP\Files\NotFoundException;
 use OCP\IPreview;
@@ -28,8 +29,7 @@
 
 class PublicPreviewController extends PublicShareController {
 
-	/** @var IShare */
-	private $share;
+	private IShare $share;
 
 	public function __construct(
 		string $appName,
@@ -59,12 +59,14 @@ protected function isPasswordProtected(): bool {
 		return $this->share->getPassword() !== null;
 	}
 
-
 	/**
-	 * Get a preview for a shared file
+	 * Get a preview for a public share.
+	 *
+	 * For shares pointing to a single file, the $file parameter is ignored.
+	 * For folder shares, $file must be the relative path to a file inside the shared folder.
 	 *
 	 * @param string $token Token of the share
-	 * @param string $file File in the share
+	 * @param string $file Relative path to a file inside a shared folder; ignored for single-file shares
 	 * @param int $x Width of the preview
 	 * @param int $y Height of the preview
 	 * @param bool $a Whether to not crop the preview
@@ -119,22 +121,36 @@ public function getPreview(
 			return new DataResponse([], Http::STATUS_FORBIDDEN);
 		}
 
+		$previewFile = null;
+
 		try {
-			$node = $share->getNode();
-			if ($node instanceof Folder) {
-				$file = $node->get($file);
+			$shareNode = $share->getNode();
+			if ($shareNode instanceof Folder) {
+				if ($file === '') {
+					return new DataResponse([], Http::STATUS_BAD_REQUEST);
+				}
+
+				$previewFile = $shareNode->get($file);
+				if ($previewFile instanceof Folder) {
+					return new DataResponse([], Http::STATUS_BAD_REQUEST);
+				}
 			} else {
-				$file = $node;
+				$previewFile = $shareNode;
 			}
 
-			$f = $this->previewManager->getPreview($file, $x, $y, !$a);
-			$response = new FileDisplayResponse($f, Http::STATUS_OK, ['Content-Type' => $f->getMimeType()]);
+			$preview = $this->previewManager->getPreview($previewFile, $x, $y, !$a);
+			$response = new FileDisplayResponse(
+				$preview,
+				Http::STATUS_OK,
+				['Content-Type' => $preview->getMimeType()]
+			);
+
 			$response->cacheFor($cacheForSeconds);
 			return $response;
 		} catch (NotFoundException $e) {
-			// If we have no preview enabled, we can redirect to the mime icon if any
-			if ($mimeFallback) {
-				if ($url = $this->mimeIconProvider->getMimeIconUrl($file->getMimeType())) {
+			// If a preview could not be generated for a resolved file, we can redirect to the mime icon if any
+			if ($mimeFallback && $previewFile instanceof File) {
+				if ($url = $this->mimeIconProvider->getMimeIconUrl($previewFile->getMimeType())) {
 					return new RedirectResponse($url);
 				}
 			}

apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php

@@ -10,6 +10,7 @@
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\DataResponse;
 use OCP\AppFramework\Http\FileDisplayResponse;
+use OCP\AppFramework\Http\RedirectResponse;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Constants;
 use OCP\Files\File;
@@ -32,6 +33,7 @@ class PublicPreviewControllerTest extends TestCase {
 	private IManager&MockObject $shareManager;
 	private ITimeFactory&MockObject $timeFactory;
 	private IRequest&MockObject $request;
+	private IMimeIconProvider&MockObject $mimeIconProvider;
 
 	private PublicPreviewController $controller;
 
@@ -42,6 +44,7 @@ protected function setUp(): void {
 		$this->shareManager = $this->createMock(IManager::class);
 		$this->timeFactory = $this->createMock(ITimeFactory::class);
 		$this->request = $this->createMock(IRequest::class);
+		$this->mimeIconProvider = $this->createMock(IMimeIconProvider::class);
 
 		$this->timeFactory->method('getTime')
 			->willReturn(1337);
@@ -54,7 +57,7 @@ protected function setUp(): void {
 			$this->shareManager,
 			$this->createMock(ISession::class),
 			$this->previewManager,
-			$this->createMock(IMimeIconProvider::class),
+			$this->mimeIconProvider,
 		);
 	}
 
@@ -153,7 +156,7 @@ public function testShareNoDownloadButPreviewHeader() {
 		$preview->method('getMimeType')
 			->willReturn('myMime');
 
-		$res = $this->controller->getPreview('token', 'file', 10, 10, true);
+		$res = $this->controller->getPreview('token', 'file', 10, 10, true, false);
 		$expected = new FileDisplayResponse($preview, Http::STATUS_OK, ['Content-Type' => 'myMime']);
 		$expected->cacheFor(15 * 60);
 		$this->assertEquals($expected, $res);
@@ -189,7 +192,7 @@ public function testShareWithAttributes() {
 		$preview->method('getMimeType')
 			->willReturn('myMime');
 
-		$res = $this->controller->getPreview('token', 'file', 10, 10, true);
+		$res = $this->controller->getPreview('token', 'file', 10, 10, true, false);
 		$expected = new FileDisplayResponse($preview, Http::STATUS_OK, ['Content-Type' => 'myMime']);
 		$expected->cacheFor(3600 * 24);
 		$this->assertEquals($expected, $res);
@@ -221,7 +224,7 @@ public function testPreviewFile() {
 		$preview->method('getMimeType')
 			->willReturn('myMime');
 
-		$res = $this->controller->getPreview('token', 'file', 10, 10, true);
+		$res = $this->controller->getPreview('token', 'file', 10, 10, true, false);
 		$expected = new FileDisplayResponse($preview, Http::STATUS_OK, ['Content-Type' => 'myMime']);
 		$expected->cacheFor(3600 * 24);
 		$this->assertEquals($expected, $res);
@@ -247,11 +250,96 @@ public function testPreviewFolderInvalidFile(): void {
 			->with($this->equalTo('file'))
 			->willThrowException(new NotFoundException());
 
-		$res = $this->controller->getPreview('token', 'file', 10, 10, true);
+		$res = $this->controller->getPreview('token', 'file', 10, 10, true, false);
 		$expected = new DataResponse([], Http::STATUS_NOT_FOUND);
 		$this->assertEquals($expected, $res);
 	}
 
+	public function testPreviewFolderEmptyFileReturnsBadRequest(): void {
+		$share = $this->createMock(IShare::class);
+		$this->shareManager->method('getShareByToken')
+			->with($this->equalTo('token'))
+			->willReturn($share);
+
+		$share->method('getPermissions')
+			->willReturn(Constants::PERMISSION_READ);
+
+		$folder = $this->createMock(Folder::class);
+		$share->method('getNode')
+			->willReturn($folder);
+
+		$share->method('canSeeContent')
+			->willReturn(true);
+
+		$res = $this->controller->getPreview('token', '', 10, 10, false, false);
+		$expected = new DataResponse([], Http::STATUS_BAD_REQUEST);
+		$this->assertEquals($expected, $res);
+	}
+
+	public function testPreviewFolderInvalidFileWithMimeFallbackReturnsNotFound(): void {
+		$share = $this->createMock(IShare::class);
+		$this->shareManager->method('getShareByToken')
+			->with($this->equalTo('token'))
+			->willReturn($share);
+
+		$share->method('getPermissions')
+			->willReturn(Constants::PERMISSION_READ);
+
+		$folder = $this->createMock(Folder::class);
+		$share->method('getNode')
+			->willReturn($folder);
+
+		$share->method('canSeeContent')
+			->willReturn(true);
+
+		$folder->method('get')
+			->with($this->equalTo('file'))
+			->willThrowException(new NotFoundException());
+
+		$this->mimeIconProvider->expects($this->never())
+			->method('getMimeIconUrl');
+
+		$res = $this->controller->getPreview('token', 'file', 10, 10, false, true);
+		$expected = new DataResponse([], Http::STATUS_NOT_FOUND);
+		$this->assertEquals($expected, $res);
+	}
+
+	public function testPreviewFolderValidFileMimeFallbackRedirectsWhenPreviewMissing(): void {
+		$share = $this->createMock(IShare::class);
+		$this->shareManager->method('getShareByToken')
+			->with($this->equalTo('token'))
+			->willReturn($share);
+
+		$share->method('getPermissions')
+			->willReturn(Constants::PERMISSION_READ);
+
+		$folder = $this->createMock(Folder::class);
+		$share->method('getNode')
+			->willReturn($folder);
+
+		$share->method('canSeeContent')
+			->willReturn(true);
+
+		$file = $this->createMock(File::class);
+		$folder->method('get')
+			->with($this->equalTo('file'))
+			->willReturn($file);
+
+		$file->method('getMimeType')
+			->willReturn('text/plain');
+
+		$this->previewManager->method('getPreview')
+			->with($this->equalTo($file), 10, 10, false)
+			->willThrowException(new NotFoundException());
+
+		$this->mimeIconProvider->method('getMimeIconUrl')
+			->with('text/plain')
+			->willReturn('/icon-url');
+
+		$res = $this->controller->getPreview('token', 'file', 10, 10, false, true);
+		$expected = new RedirectResponse('/icon-url');
+		$this->assertEquals($expected, $res);
+	}
 
 	public function testPreviewFolderValidFile(): void {
 		$share = $this->createMock(IShare::class);
@@ -284,7 +372,7 @@ public function testPreviewFolderValidFile(): void {
 		$preview->method('getMimeType')
 			->willReturn('myMime');
 
-		$res = $this->controller->getPreview('token', 'file', 10, 10, true);
+		$res = $this->controller->getPreview('token', 'file', 10, 10, true, false);
 		$expected = new FileDisplayResponse($preview, Http::STATUS_OK, ['Content-Type' => 'myMime']);
 		$expected->cacheFor(3600 * 24);
 		$this->assertEquals($expected, $res);