Skip to content

fix(files_sharing): validate input in PublicPreviewController#getPreview#61960

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kesselb wants to merge 1 commit into
base:master
Choose a base branch
from
+164 −26
Open

fix(files_sharing): validate input in PublicPreviewController#getPreview #61960

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 37 additions & 16 deletions apps/files_sharing/lib/Controller/PublicPreviewController.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
Expand All@@ -30,8 +31,7 @@

class PublicPreviewController extends PublicShareController {

/** @var IShare */
private $share;
private IShare $share;

public function __construct(
string $appName,
Expand DownExpand Up@@ -65,10 +65,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
Expand DownExpand Up@@ -123,27 +126,43 @@ 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) {
Comment on lines +129 to +133
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);
Comment on lines +143 to +146
$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 ($file instanceof File && $mimeFallback) {
if ($url = $this->mimeIconProvider->getMimeIconUrl($file->getMimeType())) {
} 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())) {
return new RedirectResponse($url);
}
}
Comment on lines +155 to 161
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);
}
}
Expand DownExpand Up@@ -201,8 +220,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);
}
Expand Down
5 changes: 3 additions & 2 deletions apps/files_sharing/openapi.json
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1474,7 +1474,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"
],
Expand All@@ -1500,7 +1501,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": ""
Expand Down
127 changes: 121 additions & 6 deletions apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
Expand All@@ -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;

Expand All@@ -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);
Expand All@@ -55,7 +58,7 @@ protected function setUp(): void {
$this->shareManager,
$this->createMock(ISession::class),
$this->previewManager,
$this->createMock(IMimeIconProvider::class),
$this->mimeIconProvider,
);
}

Expand DownExpand Up@@ -154,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);
Expand DownExpand Up@@ -190,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);
Expand DownExpand Up@@ -222,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);
Expand All@@ -248,11 +251,123 @@ 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 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')
->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, true)
->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);
$this->shareManager->method('getShareByToken')
Expand DownExpand Up@@ -284,7 +399,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);
Expand Down
5 changes: 3 additions & 2 deletions openapi.json
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -26400,7 +26400,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"
],
Expand All@@ -26426,7 +26427,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": ""
Expand Down
Loading