Skip to content

Commit a0cef76

Browse files
mvanhornkesselb
mvanhorn
authored and
kesselb
committed
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 <josh.t.richards@gmail.com> Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de> # Conflicts: # apps/files_sharing/lib/Controller/PublicPreviewController.php # Conflicts: # apps/files_sharing/lib/Controller/PublicPreviewController.php
1 parent be98f77 commit a0cef76

4 files changed

Lines changed: 164 additions & 26 deletions

File tree

‎apps/files_sharing/lib/Controller/PublicPreviewController.php‎

Lines changed: 37 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
use OCP\Files\File;
2121
use OCP\Files\Folder;
2222
use OCP\Files\NotFoundException;
23+
use OCP\Files\NotPermittedException;
2324
use OCP\IPreview;
2425
use OCP\IRequest;
2526
use OCP\ISession;
@@ -30,8 +31,7 @@
3031

3132
class PublicPreviewController extends PublicShareController {
3233

33-
/** @var IShare */
34-
private $share;
34+
private IShare $share;
3535

3636
public function __construct(
3737
string $appName,
@@ -65,10 +65,13 @@ protected function isPasswordProtected(): bool {
6565
}
6666

6767
/**
68-
* Get a preview for a shared file
68+
* Get a preview for a public share
69+
*
70+
* For shares pointing to a single file, the file parameter is ignored.
71+
* For folder shares, file must be the relative path to a file inside the shared folder.
6972
*
7073
* @param string $token Token of the share
71-
* @param string $file File in the share
74+
* @param string $file Relative path to a file inside a shared folder; ignored for single-file shares
7275
* @param int $x Width of the preview
7376
* @param int $y Height of the preview
7477
* @param bool $a Whether to not crop the preview
@@ -123,27 +126,43 @@ public function getPreview(
123126
return new DataResponse([], Http::STATUS_FORBIDDEN);
124127
}
125128

129+
$previewFile = null;
130+
126131
try {
127-
$node = $share->getNode();
128-
if ($node instanceof Folder) {
129-
$file = $node->get($file);
132+
$shareNode = $share->getNode();
133+
if ($shareNode instanceof Folder) {
134+
if ($file === '') {
135+
return new DataResponse([], Http::STATUS_BAD_REQUEST);
136+
}
137+
138+
$previewFile = $shareNode->get($file);
139+
if ($previewFile instanceof Folder) {
140+
return new DataResponse([], Http::STATUS_BAD_REQUEST);
141+
}
130142
} else {
131-
$file = $node;
143+
$previewFile = $shareNode;
132144
}
133145

134-
$f = $this->previewManager->getPreview($file, $x, $y, !$a);
135-
$response = new FileDisplayResponse($f, Http::STATUS_OK, ['Content-Type' => $f->getMimeType()]);
146+
$preview = $this->previewManager->getPreview($previewFile, $x, $y, !$a);
147+
$response = new FileDisplayResponse(
148+
$preview,
149+
Http::STATUS_OK,
150+
['Content-Type' => $preview->getMimeType()]
151+
);
152+
136153
$response->cacheFor($cacheForSeconds);
137154
return $response;
138-
} catch (NotFoundException $e) {
139-
// If we have no preview enabled, we can redirect to the mime icon if any
140-
if ($file instanceof File && $mimeFallback) {
141-
if ($url = $this->mimeIconProvider->getMimeIconUrl($file->getMimeType())) {
155+
} catch (NotFoundException) {
156+
// If a preview could not be generated for a resolved file, we can redirect to the mime icon if any
157+
if ($mimeFallback && $previewFile instanceof File) {
158+
if ($url = $this->mimeIconProvider->getMimeIconUrl($previewFile->getMimeType())) {
142159
return new RedirectResponse($url);
143160
}
144161
}
145162
return new DataResponse([], Http::STATUS_NOT_FOUND);
146-
} catch (\InvalidArgumentException $e) {
163+
} catch (NotPermittedException) {
164+
return new DataResponse([], Http::STATUS_FORBIDDEN);
165+
} catch (\InvalidArgumentException) {
147166
return new DataResponse([], Http::STATUS_BAD_REQUEST);
148167
}
149168
}
@@ -201,8 +220,10 @@ public function directLink(string $token) {
201220
$response = new FileDisplayResponse($f, Http::STATUS_OK, ['Content-Type' => $f->getMimeType()]);
202221
$response->cacheFor(3600 * 24);
203222
return $response;
204-
} catch (NotFoundException $e) {
223+
} catch (NotFoundException) {
205224
return new DataResponse([], Http::STATUS_NOT_FOUND);
225+
} catch (NotPermittedException) {
226+
return new DataResponse([], Http::STATUS_FORBIDDEN);
206227
} catch (\InvalidArgumentException $e) {
207228
return new DataResponse([], Http::STATUS_BAD_REQUEST);
208229
}

‎apps/files_sharing/openapi.json‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1474,7 +1474,8 @@
14741474
"/index.php/apps/files_sharing/publicpreview/{token}": {
14751475
"get": {
14761476
"operationId": "public_preview-get-preview",
1477-
"summary": "Get a preview for a shared file",
1477+
"summary": "Get a preview for a public share",
1478+
"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.",
14781479
"tags": [
14791480
"public_preview"
14801481
],
@@ -1500,7 +1501,7 @@
15001501
{
15011502
"name": "file",
15021503
"in": "query",
1503-
"description": "File in the share",
1504+
"description": "Relative path to a file inside a shared folder; ignored for single-file shares",
15041505
"schema": {
15051506
"type": "string",
15061507
"default": ""

‎apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php‎

Lines changed: 121 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
use OCP\AppFramework\Http;
1212
use OCP\AppFramework\Http\DataResponse;
1313
use OCP\AppFramework\Http\FileDisplayResponse;
14+
use OCP\AppFramework\Http\RedirectResponse;
1415
use OCP\AppFramework\Utility\ITimeFactory;
1516
use OCP\Constants;
1617
use OCP\Files\File;
@@ -33,6 +34,7 @@ class PublicPreviewControllerTest extends TestCase {
3334
private IManager&MockObject $shareManager;
3435
private ITimeFactory&MockObject $timeFactory;
3536
private IRequest&MockObject $request;
37+
private IMimeIconProvider&MockObject $mimeIconProvider;
3638

3739
private PublicPreviewController $controller;
3840

@@ -43,6 +45,7 @@ protected function setUp(): void {
4345
$this->shareManager = $this->createMock(IManager::class);
4446
$this->timeFactory = $this->createMock(ITimeFactory::class);
4547
$this->request = $this->createMock(IRequest::class);
48+
$this->mimeIconProvider = $this->createMock(IMimeIconProvider::class);
4649

4750
$this->timeFactory->method('getTime')
4851
->willReturn(1337);
@@ -55,7 +58,7 @@ protected function setUp(): void {
5558
$this->shareManager,
5659
$this->createMock(ISession::class),
5760
$this->previewManager,
58-
$this->createMock(IMimeIconProvider::class),
61+
$this->mimeIconProvider,
5962
);
6063
}
6164

@@ -154,7 +157,7 @@ public function testShareNoDownloadButPreviewHeader() {
154157
$preview->method('getMimeType')
155158
->willReturn('myMime');
156159

157-
$res = $this->controller->getPreview('token', 'file', 10, 10, true);
160+
$res = $this->controller->getPreview('token', 'file', 10, 10, true, false);
158161
$expected = new FileDisplayResponse($preview, Http::STATUS_OK, ['Content-Type' => 'myMime']);
159162
$expected->cacheFor(15 * 60);
160163
$this->assertEquals($expected, $res);
@@ -190,7 +193,7 @@ public function testShareWithAttributes() {
190193
$preview->method('getMimeType')
191194
->willReturn('myMime');
192195

193-
$res = $this->controller->getPreview('token', 'file', 10, 10, true);
196+
$res = $this->controller->getPreview('token', 'file', 10, 10, true, false);
194197
$expected = new FileDisplayResponse($preview, Http::STATUS_OK, ['Content-Type' => 'myMime']);
195198
$expected->cacheFor(3600 * 24);
196199
$this->assertEquals($expected, $res);
@@ -222,7 +225,7 @@ public function testPreviewFile() {
222225
$preview->method('getMimeType')
223226
->willReturn('myMime');
224227

225-
$res = $this->controller->getPreview('token', 'file', 10, 10, true);
228+
$res = $this->controller->getPreview('token', 'file', 10, 10, true, false);
226229
$expected = new FileDisplayResponse($preview, Http::STATUS_OK, ['Content-Type' => 'myMime']);
227230
$expected->cacheFor(3600 * 24);
228231
$this->assertEquals($expected, $res);
@@ -248,11 +251,123 @@ public function testPreviewFolderInvalidFile(): void {
248251
->with($this->equalTo('file'))
249252
->willThrowException(new NotFoundException());
250253

251-
$res = $this->controller->getPreview('token', 'file', 10, 10, true);
254+
$res = $this->controller->getPreview('token', 'file', 10, 10, true, false);
252255
$expected = new DataResponse([], Http::STATUS_NOT_FOUND);
253256
$this->assertEquals($expected, $res);
254257
}
255258

259+
public function testPreviewFolderEmptyFileReturnsBadRequest(): void {
260+
$share = $this->createMock(IShare::class);
261+
$this->shareManager->method('getShareByToken')
262+
->with($this->equalTo('token'))
263+
->willReturn($share);
264+
265+
$share->method('getPermissions')
266+
->willReturn(Constants::PERMISSION_READ);
267+
268+
$folder = $this->createMock(Folder::class);
269+
$share->method('getNode')
270+
->willReturn($folder);
271+
272+
$share->method('canSeeContent')
273+
->willReturn(true);
274+
275+
$res = $this->controller->getPreview('token', '', 10, 10, false, false);
276+
$expected = new DataResponse([], Http::STATUS_BAD_REQUEST);
277+
$this->assertEquals($expected, $res);
278+
}
279+
280+
public function testPreviewFolderSubfolderReturnsBadRequest(): void {
281+
$share = $this->createMock(IShare::class);
282+
$this->shareManager->method('getShareByToken')
283+
->with($this->equalTo('token'))
284+
->willReturn($share);
285+
286+
$share->method('getPermissions')
287+
->willReturn(Constants::PERMISSION_READ);
288+
289+
$folder = $this->createMock(Folder::class);
290+
$share->method('getNode')
291+
->willReturn($folder);
292+
293+
$share->method('canSeeContent')
294+
->willReturn(true);
295+
296+
$subfolder = $this->createMock(Folder::class);
297+
$folder->method('get')
298+
->with($this->equalTo('nested'))
299+
->willReturn($subfolder);
300+
301+
$res = $this->controller->getPreview('token', 'nested', 10, 10, false, false);
302+
$expected = new DataResponse([], Http::STATUS_BAD_REQUEST);
303+
$this->assertEquals($expected, $res);
304+
}
305+
306+
public function testPreviewFolderInvalidFileWithMimeFallbackReturnsNotFound(): void {
307+
$share = $this->createMock(IShare::class);
308+
$this->shareManager->method('getShareByToken')
309+
->with($this->equalTo('token'))
310+
->willReturn($share);
311+
312+
$share->method('getPermissions')
313+
->willReturn(Constants::PERMISSION_READ);
314+
315+
$folder = $this->createMock(Folder::class);
316+
$share->method('getNode')
317+
->willReturn($folder);
318+
319+
$share->method('canSeeContent')
320+
->willReturn(true);
321+
322+
$folder->method('get')
323+
->with($this->equalTo('file'))
324+
->willThrowException(new NotFoundException());
325+
326+
$this->mimeIconProvider->expects($this->never())
327+
->method('getMimeIconUrl');
328+
329+
$res = $this->controller->getPreview('token', 'file', 10, 10, false, true);
330+
$expected = new DataResponse([], Http::STATUS_NOT_FOUND);
331+
$this->assertEquals($expected, $res);
332+
}
333+
334+
public function testPreviewFolderValidFileMimeFallbackRedirectsWhenPreviewMissing(): void {
335+
$share = $this->createMock(IShare::class);
336+
$this->shareManager->method('getShareByToken')
337+
->with($this->equalTo('token'))
338+
->willReturn($share);
339+
340+
$share->method('getPermissions')
341+
->willReturn(Constants::PERMISSION_READ);
342+
343+
$folder = $this->createMock(Folder::class);
344+
$share->method('getNode')
345+
->willReturn($folder);
346+
347+
$share->method('canSeeContent')
348+
->willReturn(true);
349+
350+
$file = $this->createMock(File::class);
351+
$folder->method('get')
352+
->with($this->equalTo('file'))
353+
->willReturn($file);
354+
355+
$file->method('getMimeType')
356+
->willReturn('text/plain');
357+
358+
$this->previewManager->method('getPreview')
359+
->with($this->equalTo($file), 10, 10, true)
360+
->willThrowException(new NotFoundException());
361+
362+
$this->mimeIconProvider->method('getMimeIconUrl')
363+
->with('text/plain')
364+
->willReturn('/icon-url');
365+
366+
$res = $this->controller->getPreview('token', 'file', 10, 10, false, true);
367+
$expected = new RedirectResponse('/icon-url');
368+
$this->assertEquals($expected, $res);
369+
}
370+
256371
public function testPreviewFolderValidFile(): void {
257372
$share = $this->createMock(IShare::class);
258373
$this->shareManager->method('getShareByToken')
@@ -284,7 +399,7 @@ public function testPreviewFolderValidFile(): void {
284399
$preview->method('getMimeType')
285400
->willReturn('myMime');
286401

287-
$res = $this->controller->getPreview('token', 'file', 10, 10, true);
402+
$res = $this->controller->getPreview('token', 'file', 10, 10, true, false);
288403
$expected = new FileDisplayResponse($preview, Http::STATUS_OK, ['Content-Type' => 'myMime']);
289404
$expected->cacheFor(3600 * 24);
290405
$this->assertEquals($expected, $res);

‎openapi.json‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26400,7 +26400,8 @@
2640026400
"/index.php/apps/files_sharing/publicpreview/{token}": {
2640126401
"get": {
2640226402
"operationId": "files_sharing-public_preview-get-preview",
26403-
"summary": "Get a preview for a shared file",
26403+
"summary": "Get a preview for a public share",
26404+
"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.",
2640426405
"tags": [
2640526406
"files_sharing/public_preview"
2640626407
],
@@ -26426,7 +26427,7 @@
2642626427
{
2642726428
"name": "file",
2642826429
"in": "query",
26429-
"description": "File in the share",
26430+
"description": "Relative path to a file inside a shared folder; ignored for single-file shares",
2643026431
"schema": {
2643126432
"type": "string",
2643226433
"default": ""

0 commit comments

Comments
 (0)