Skip to content

Commit f693f34

Browse files
kesselbkesselb
authored
Merge pull request #59253 from mvanhorn/fix/public-preview-input-validation
fix(files_sharing): validate input in PublicPreviewController#getPreview
2 parents 59c05fc + b035809 commit f693f34

4 files changed

Lines changed: 163 additions & 26 deletions

File tree

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

Lines changed: 38 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,10 @@
1717
use OCP\AppFramework\Http\RedirectResponse;
1818
use OCP\AppFramework\PublicShareController;
1919
use OCP\Constants;
20+
use OCP\Files\File;
2021
use OCP\Files\Folder;
2122
use OCP\Files\NotFoundException;
23+
use OCP\Files\NotPermittedException;
2224
use OCP\IPreview;
2325
use OCP\IRequest;
2426
use OCP\ISession;
@@ -29,8 +31,7 @@
2931

3032
class PublicPreviewController extends PublicShareController {
3133

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

3536
public function __construct(
3637
string $appName,
@@ -64,10 +65,13 @@ protected function isPasswordProtected(): bool {
6465
}
6566

6667
/**
67-
* 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.
6872
*
6973
* @param string $token Token of the share
70-
* @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
7175
* @param int $x Width of the preview
7276
* @param int $y Height of the preview
7377
* @param bool $a Whether to not crop the preview
@@ -122,27 +126,43 @@ public function getPreview(
122126
return new DataResponse([], Http::STATUS_FORBIDDEN);
123127
}
124128

129+
$previewFile = null;
130+
125131
try {
126-
$node = $share->getNode();
127-
if ($node instanceof Folder) {
128-
$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+
}
129142
} else {
130-
$file = $node;
143+
$previewFile = $shareNode;
131144
}
132145

133-
$f = $this->previewManager->getPreview($file, $x, $y, !$a);
134-
$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+
135153
$response->cacheFor($cacheForSeconds);
136154
return $response;
137-
} catch (NotFoundException $e) {
138-
// If we have no preview enabled, we can redirect to the mime icon if any
139-
if ($mimeFallback) {
140-
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())) {
141159
return new RedirectResponse($url);
142160
}
143161
}
144162
return new DataResponse([], Http::STATUS_NOT_FOUND);
145-
} catch (\InvalidArgumentException $e) {
163+
} catch (NotPermittedException) {
164+
return new DataResponse([], Http::STATUS_FORBIDDEN);
165+
} catch (\InvalidArgumentException) {
146166
return new DataResponse([], Http::STATUS_BAD_REQUEST);
147167
}
148168
}
@@ -200,8 +220,10 @@ public function directLink(string $token) {
200220
$response = new FileDisplayResponse($f, Http::STATUS_OK, ['Content-Type' => $f->getMimeType()]);
201221
$response->cacheFor(3600 * 24);
202222
return $response;
203-
} catch (NotFoundException $e) {
223+
} catch (NotFoundException) {
204224
return new DataResponse([], Http::STATUS_NOT_FOUND);
225+
} catch (NotPermittedException) {
226+
return new DataResponse([], Http::STATUS_FORBIDDEN);
205227
} catch (\InvalidArgumentException $e) {
206228
return new DataResponse([], Http::STATUS_BAD_REQUEST);
207229
}

‎apps/files_sharing/openapi.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1474,7 +1474,7 @@
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",
14781478
"tags": [
14791479
"public_preview"
14801480
],
@@ -1500,7 +1500,7 @@
15001500
{
15011501
"name": "file",
15021502
"in": "query",
1503-
"description": "File in the share",
1503+
"description": "Relative path to a file inside a shared folder; ignored for single-file shares",
15041504
"schema": {
15051505
"type": "string",
15061506
"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: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25605,7 +25605,7 @@
2560525605
"/index.php/apps/files_sharing/publicpreview/{token}": {
2560625606
"get": {
2560725607
"operationId": "files_sharing-public_preview-get-preview",
25608-
"summary": "Get a preview for a shared file",
25608+
"summary": "Get a preview for a public share",
2560925609
"tags": [
2561025610
"files_sharing/public_preview"
2561125611
],
@@ -25631,7 +25631,7 @@
2563125631
{
2563225632
"name": "file",
2563325633
"in": "query",
25634-
"description": "File in the share",
25634+
"description": "Relative path to a file inside a shared folder; ignored for single-file shares",
2563525635
"schema": {
2563625636
"type": "string",
2563725637
"default": ""

0 commit comments

Comments
 (0)