From ac2411ac20179920bd9a5e2bb0336474c3d751bd Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Fri, 27 Mar 2026 06:13:53 -0700 Subject: [PATCH 1/7] fix(files_sharing): validate input in PublicPreviewController#getPreview Return 400 Bad Request when the file parameter is empty and the shared node is a folder, instead of passing the folder itself to getPreview which triggers an internal server error. Also rename the local variable to $fileNode to prevent the catch block from calling getMimeType() on the original string parameter when get() throws NotFoundException. Fixes #59229 Signed-off-by: Matt Van Horn Co-Authored-By: Claude Opus 4.6 Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> --- .../lib/Controller/PublicPreviewController.php | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/apps/files_sharing/lib/Controller/PublicPreviewController.php b/apps/files_sharing/lib/Controller/PublicPreviewController.php index 24a537e110d1d..77e47a7b1d3cf 100644 --- a/apps/files_sharing/lib/Controller/PublicPreviewController.php +++ b/apps/files_sharing/lib/Controller/PublicPreviewController.php @@ -125,19 +125,22 @@ public function getPreview( try { $node = $share->getNode(); if ($node instanceof Folder) { - $file = $node->get($file); + if ($file === '') { + return new DataResponse([], Http::STATUS_BAD_REQUEST); + } + $fileNode = $node->get($file); } else { - $file = $node; + $fileNode = $node; } - $f = $this->previewManager->getPreview($file, $x, $y, !$a); + $f = $this->previewManager->getPreview($fileNode, $x, $y, !$a); $response = new FileDisplayResponse($f, Http::STATUS_OK, ['Content-Type' => $f->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 ($mimeFallback && isset($fileNode) && !($fileNode instanceof Folder)) { + if ($url = $this->mimeIconProvider->getMimeIconUrl($fileNode->getMimeType())) { return new RedirectResponse($url); } } From 99fd0e8fade8b6c7d16c5f58a7235e0abb47da25 Mon Sep 17 00:00:00 2001 From: Josh Date: Sat, 28 Mar 2026 09:17:38 -0400 Subject: [PATCH 2/7] chore(sharing): update docblock for PublicPreviewController::getPreview Signed-off-by: Josh Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> --- .../lib/Controller/PublicPreviewController.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/apps/files_sharing/lib/Controller/PublicPreviewController.php b/apps/files_sharing/lib/Controller/PublicPreviewController.php index 77e47a7b1d3cf..1c98e845db044 100644 --- a/apps/files_sharing/lib/Controller/PublicPreviewController.php +++ b/apps/files_sharing/lib/Controller/PublicPreviewController.php @@ -64,10 +64,13 @@ protected function isPasswordProtected(): bool { } /** - * 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 and may be empty. + * 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 From 097ca88b7831519b65b57992824d70a92f18d54d Mon Sep 17 00:00:00 2001 From: Josh Date: Sun, 29 Mar 2026 09:35:15 -0400 Subject: [PATCH 3/7] refactor(files_sharing): improve PublicPreviewController::getPreview variable naming And slightly streamline the logic for clarity. Signed-off-by: Josh Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> --- .../Controller/PublicPreviewController.php | 35 ++++++++++++------- 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/apps/files_sharing/lib/Controller/PublicPreviewController.php b/apps/files_sharing/lib/Controller/PublicPreviewController.php index 1c98e845db044..0d9afd9a729f6 100644 --- a/apps/files_sharing/lib/Controller/PublicPreviewController.php +++ b/apps/files_sharing/lib/Controller/PublicPreviewController.php @@ -17,6 +17,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; @@ -29,8 +30,7 @@ class PublicPreviewController extends PublicShareController { - /** @var IShare */ - private $share; + private IShare $share; public function __construct( string $appName, @@ -66,7 +66,7 @@ protected function isPasswordProtected(): bool { /** * Get a preview for a public share. * - * For shares pointing to a single file, the $file parameter is ignored and may be empty. + * 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 @@ -125,25 +125,36 @@ public function getPreview( return new DataResponse([], Http::STATUS_FORBIDDEN); } + $previewFile = null; + try { - $node = $share->getNode(); - if ($node instanceof Folder) { + $shareNode = $share->getNode(); + if ($shareNode instanceof Folder) { if ($file === '') { return new DataResponse([], Http::STATUS_BAD_REQUEST); } - $fileNode = $node->get($file); + + $previewFile = $shareNode->get($file); + if ($previewFile instanceof Folder) { + return new DataResponse([], Http::STATUS_BAD_REQUEST); + } } else { - $fileNode = $node; + $previewFile = $shareNode; } - $f = $this->previewManager->getPreview($fileNode, $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 && isset($fileNode) && !($fileNode instanceof Folder)) { - if ($url = $this->mimeIconProvider->getMimeIconUrl($fileNode->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); } } From e32fca91a8275923e743acd05427fae3414cb5d5 Mon Sep 17 00:00:00 2001 From: Josh Date: Sun, 29 Mar 2026 09:45:55 -0400 Subject: [PATCH 4/7] test(files_sharing): add regression coverage for PublicPreviewController#getPreview edge cases Signed-off-by: Josh Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> --- .../PublicPreviewControllerTest.php | 91 ++++++++++++++++++- 1 file changed, 90 insertions(+), 1 deletion(-) diff --git a/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php b/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php index d8cbaed227fd1..b171fa1a3e420 100644 --- a/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php +++ b/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php @@ -11,6 +11,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; @@ -33,6 +34,7 @@ class PublicPreviewControllerTest extends TestCase { private IManager&MockObject $shareManager; private ITimeFactory&MockObject $timeFactory; private IRequest&MockObject $request; + private IMimeIconProvider&MockObject $mimeIconProvider; private PublicPreviewController $controller; @@ -43,6 +45,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); @@ -55,7 +58,7 @@ protected function setUp(): void { $this->shareManager, $this->createMock(ISession::class), $this->previewManager, - $this->createMock(IMimeIconProvider::class), + $this->mimeIconProvider, ); } @@ -253,6 +256,92 @@ public function testPreviewFolderInvalidFile(): void { $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, true); + $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, 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, true); + $expected = new RedirectResponse('/icon-url'); + $this->assertEquals($expected, $res); + } + public function testPreviewFolderValidFile(): void { $share = $this->createMock(IShare::class); $this->shareManager->method('getShareByToken') From 59bf63a5c5846177c3590b025cb400159141cfc9 Mon Sep 17 00:00:00 2001 From: Josh Date: Sun, 29 Mar 2026 10:06:26 -0400 Subject: [PATCH 5/7] test(files_sharing): make PublicPreviewController flag usage explicit in tests Signed-off-by: Josh Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> --- .../Controller/PublicPreviewControllerTest.php | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php b/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php index b171fa1a3e420..6a09abe78b7c1 100644 --- a/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php +++ b/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php @@ -157,7 +157,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); @@ -193,7 +193,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); @@ -225,7 +225,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); @@ -251,7 +251,7 @@ 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); } @@ -272,7 +272,7 @@ public function testPreviewFolderEmptyFileReturnsBadRequest(): void { $share->method('canSeeContent') ->willReturn(true); - $res = $this->controller->getPreview('token', '', 10, 10, true); + $res = $this->controller->getPreview('token', '', 10, 10, false, false); $expected = new DataResponse([], Http::STATUS_BAD_REQUEST); $this->assertEquals($expected, $res); } @@ -300,7 +300,7 @@ public function testPreviewFolderInvalidFileWithMimeFallbackReturnsNotFound(): v $this->mimeIconProvider->expects($this->never()) ->method('getMimeIconUrl'); - $res = $this->controller->getPreview('token', 'file', 10, 10, true); + $res = $this->controller->getPreview('token', 'file', 10, 10, false, true); $expected = new DataResponse([], Http::STATUS_NOT_FOUND); $this->assertEquals($expected, $res); } @@ -337,7 +337,7 @@ public function testPreviewFolderValidFileMimeFallbackRedirectsWhenPreviewMissin ->with('text/plain') ->willReturn('/icon-url'); - $res = $this->controller->getPreview('token', 'file', 10, 10, true); + $res = $this->controller->getPreview('token', 'file', 10, 10, false, true); $expected = new RedirectResponse('/icon-url'); $this->assertEquals($expected, $res); } @@ -373,7 +373,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); From cdd2843bb193250f59d4c7657832760139f53b83 Mon Sep 17 00:00:00 2001 From: Josh Date: Sun, 29 Mar 2026 10:25:52 -0400 Subject: [PATCH 6/7] chore: fixup PublicPreviewControllerTest getPreview expected crop boolean Signed-off-by: Josh Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> --- .../tests/Controller/PublicPreviewControllerTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php b/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php index 6a09abe78b7c1..d9091aef50535 100644 --- a/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php +++ b/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php @@ -330,7 +330,7 @@ public function testPreviewFolderValidFileMimeFallbackRedirectsWhenPreviewMissin ->willReturn('text/plain'); $this->previewManager->method('getPreview') - ->with($this->equalTo($file), 10, 10, false) + ->with($this->equalTo($file), 10, 10, true) ->willThrowException(new NotFoundException()); $this->mimeIconProvider->method('getMimeIconUrl') From edbf7b5aaf8abfe615d842d865fe8525e6c31d77 Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Sat, 11 Apr 2026 14:25:07 -0400 Subject: [PATCH 7/7] fix(files_sharing): handle NotPermittedException and drop catch variables Addresses review feedback on PR #59253: * Catch `NotPermittedException` from `$shareNode->get()` and return 403 FORBIDDEN instead of propagating an internal server error (per @kesselb on line 133). * Drop the unused `$e` capture from the existing `NotFoundException` and `\InvalidArgumentException` catch blocks, using PHP 8.0 no-capture syntax (per @kesselb on line 150). * Add `testPreviewFolderSubfolderReturnsBadRequest` covering the branch where a folder share resolves `$file` to a subfolder and must return 400 (per @Copilot on line 136). Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> --- .../Controller/PublicPreviewController.php | 7 +++-- .../PublicPreviewControllerTest.php | 26 +++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/apps/files_sharing/lib/Controller/PublicPreviewController.php b/apps/files_sharing/lib/Controller/PublicPreviewController.php index 0d9afd9a729f6..9386cf5b05f01 100644 --- a/apps/files_sharing/lib/Controller/PublicPreviewController.php +++ b/apps/files_sharing/lib/Controller/PublicPreviewController.php @@ -20,6 +20,7 @@ use OCP\Files\File; use OCP\Files\Folder; use OCP\Files\NotFoundException; +use OCP\Files\NotPermittedException; use OCP\IPreview; use OCP\IRequest; use OCP\ISession; @@ -151,7 +152,7 @@ public function getPreview( $response->cacheFor($cacheForSeconds); return $response; - } catch (NotFoundException $e) { + } catch (NotFoundException) { // 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())) { @@ -159,7 +160,9 @@ public function getPreview( } } return new DataResponse([], Http::STATUS_NOT_FOUND); - } catch (\InvalidArgumentException $e) { + } catch (NotPermittedException) { + return new DataResponse([], Http::STATUS_FORBIDDEN); + } catch (\InvalidArgumentException) { return new DataResponse([], Http::STATUS_BAD_REQUEST); } } diff --git a/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php b/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php index d9091aef50535..17dd231f202e4 100644 --- a/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php +++ b/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php @@ -277,6 +277,32 @@ public function testPreviewFolderEmptyFileReturnsBadRequest(): void { $this->assertEquals($expected, $res); } + public function testPreviewFolderSubfolderReturnsBadRequest(): 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); + + $subfolder = $this->createMock(Folder::class); + $folder->method('get') + ->with($this->equalTo('nested')) + ->willReturn($subfolder); + + $res = $this->controller->getPreview('token', 'nested', 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')