From 1945e4c01148b751495dea0d803398cd2388793f 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] fix(files_sharing): validate input in PublicPreviewController#getPreview 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 Assisted-by: ClaudeCode:claude-opus-4-6 Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Signed-off-by: Josh Signed-off-by: Daniel Kesselberg # Conflicts: # apps/files_sharing/lib/Controller/PublicPreviewController.php # Conflicts: # apps/files_sharing/lib/Controller/PublicPreviewController.php [skip ci] --- .../Controller/PublicPreviewController.php | 45 ++++++++++++++----- apps/files_sharing/openapi.json | 5 ++- .../PublicPreviewControllerTest.php | 15 ++++--- openapi.json | 5 ++- 4 files changed, 48 insertions(+), 22 deletions(-) diff --git a/apps/files_sharing/lib/Controller/PublicPreviewController.php b/apps/files_sharing/lib/Controller/PublicPreviewController.php index 70c39315e3e2e..427ccd3f35a97 100644 --- a/apps/files_sharing/lib/Controller/PublicPreviewController.php +++ b/apps/files_sharing/lib/Controller/PublicPreviewController.php @@ -17,6 +17,7 @@ use OCP\Constants; use OCP\Files\Folder; use OCP\Files\NotFoundException; +use OCP\Files\NotPermittedException; use OCP\IPreview; use OCP\IRequest; use OCP\ISession; @@ -27,8 +28,7 @@ class PublicPreviewController extends PublicShareController { - /** @var IShare */ - private $share; + private IShare $share; public function __construct( string $appName, @@ -60,10 +60,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. + * 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 @@ -118,16 +121,30 @@ 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) { @@ -138,7 +155,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); } } @@ -197,8 +216,10 @@ public function directLink(string $token) { $response = new FileDisplayResponse($f, Http::STATUS_OK, ['Content-Type' => $f->getMimeType()]); $response->cacheFor(3600 * 24); return $response; - } catch (NotFoundException $e) { + } catch (NotFoundException) { return new DataResponse([], Http::STATUS_NOT_FOUND); + } catch (NotPermittedException) { + return new DataResponse([], Http::STATUS_FORBIDDEN); } catch (\InvalidArgumentException $e) { return new DataResponse([], Http::STATUS_BAD_REQUEST); } diff --git a/apps/files_sharing/openapi.json b/apps/files_sharing/openapi.json index 9eac468689794..375667ba9dd0b 100644 --- a/apps/files_sharing/openapi.json +++ b/apps/files_sharing/openapi.json @@ -1455,7 +1455,8 @@ "/index.php/apps/files_sharing/publicpreview/{token}": { "get": { "operationId": "public_preview-get-preview", - "summary": "Get a preview for a shared file", + "summary": "Get a preview for a public share", + "description": "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.", "tags": [ "public_preview" ], @@ -1481,7 +1482,7 @@ { "name": "file", "in": "query", - "description": "File in the share", + "description": "Relative path to a file inside a shared folder; ignored for single-file shares", "schema": { "type": "string", "default": "" diff --git a/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php b/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php index 94ecd0bc23e43..93769f64bc5ee 100644 --- a/apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php +++ b/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,7 +250,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); } @@ -284,7 +287,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); diff --git a/openapi.json b/openapi.json index 4eb3b8e0bdc99..9a0ac1c53bcb2 100644 --- a/openapi.json +++ b/openapi.json @@ -24438,7 +24438,8 @@ "/index.php/apps/files_sharing/publicpreview/{token}": { "get": { "operationId": "files_sharing-public_preview-get-preview", - "summary": "Get a preview for a shared file", + "summary": "Get a preview for a public share", + "description": "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.", "tags": [ "files_sharing/public_preview" ], @@ -24464,7 +24465,7 @@ { "name": "file", "in": "query", - "description": "File in the share", + "description": "Relative path to a file inside a shared folder; ignored for single-file shares", "schema": { "type": "string", "default": ""