From e41e8de0e24da530e63bcdbcf0f462ccf085913a Mon Sep 17 00:00:00 2001 From: skjnldsv Date: Tue, 3 Jun 2025 10:12:35 +0200 Subject: [PATCH] fix(dav): file drop nickname Signed-off-by: skjnldsv --- .../dav/lib/Files/Sharing/FilesDropPlugin.php | 24 +++++++--- .../Files/Sharing/FilesDropPluginTest.php | 4 +- apps/files/src/utils/filenameValidity.ts | 20 ++++---- .../src/services/GuestNameValidity.ts | 45 ++++++++++++++++++ .../src/views/PublicAuthPrompt.vue | 4 +- .../filesdrop_features/filesdrop.feature | 47 +++++++++++++++++-- lib/private/Files/Node/Folder.php | 8 ++++ lib/private/Files/Node/LazyFolder.php | 4 ++ lib/public/Files/Folder.php | 11 +++++ 9 files changed, 143 insertions(+), 24 deletions(-) create mode 100644 apps/files_sharing/src/services/GuestNameValidity.ts diff --git a/apps/dav/lib/Files/Sharing/FilesDropPlugin.php b/apps/dav/lib/Files/Sharing/FilesDropPlugin.php index 9aee5283ea9..3bdd273189f 100644 --- a/apps/dav/lib/Files/Sharing/FilesDropPlugin.php +++ b/apps/dav/lib/Files/Sharing/FilesDropPlugin.php @@ -8,6 +8,7 @@ namespace OCA\DAV\Files\Sharing; use OCP\Files\Folder; use OCP\Files\NotFoundException; use OCP\Share\IShare; +use Sabre\DAV\Exception\BadRequest; use Sabre\DAV\Exception\MethodNotAllowed; use Sabre\DAV\ServerPlugin; use Sabre\HTTP\RequestInterface; @@ -71,13 +72,12 @@ class FilesDropPlugin extends ServerPlugin { ? trim(urldecode($request->getHeader('X-NC-Nickname'))) : null; - // if ($request->getMethod() !== 'PUT') { // If uploading subfolders we need to ensure they get created // within the nickname folder if ($request->getMethod() === 'MKCOL') { if (!$nickname) { - throw new MethodNotAllowed('A nickname header is required when uploading subfolders'); + throw new BadRequest('A nickname header is required when uploading subfolders'); } } else { throw new MethodNotAllowed('Only PUT is allowed on files drop'); @@ -113,7 +113,7 @@ class FilesDropPlugin extends ServerPlugin { // We need a valid nickname for file requests if ($isFileRequest && !$nickname) { - throw new MethodNotAllowed('A nickname header is required for file requests'); + throw new BadRequest('A nickname header is required for file requests'); } // We're only allowing the upload of @@ -121,12 +121,24 @@ class FilesDropPlugin extends ServerPlugin { // This prevents confusion when uploading files and help // classify them by uploaders. if (!$nickname && !$isRootUpload) { - throw new MethodNotAllowed('A nickname header is required when uploading subfolders'); + throw new BadRequest('A nickname header is required when uploading subfolders'); } - // If we have a nickname, let's put everything inside if ($nickname) { - // Put all files in the subfolder + try { + $node->verifyPath($nickname); + } catch (\Exception $e) { + // If the path is not valid, we throw an exception + throw new BadRequest('Invalid nickname: ' . $nickname); + } + + // Forbid nicknames starting with a dot + if (str_starts_with($nickname, '.')) { + throw new BadRequest('Invalid nickname: ' . $nickname); + } + + // If we have a nickname, let's put + // all files in the subfolder $relativePath = '/' . $nickname . '/' . $relativePath; $relativePath = str_replace('//', '/', $relativePath); } diff --git a/apps/dav/tests/unit/Files/Sharing/FilesDropPluginTest.php b/apps/dav/tests/unit/Files/Sharing/FilesDropPluginTest.php index 545bea9a406..1a7ab7179e1 100644 --- a/apps/dav/tests/unit/Files/Sharing/FilesDropPluginTest.php +++ b/apps/dav/tests/unit/Files/Sharing/FilesDropPluginTest.php @@ -13,7 +13,7 @@ use OCP\Files\NotFoundException; use OCP\Share\IAttributes; use OCP\Share\IShare; use PHPUnit\Framework\MockObject\MockObject; -use Sabre\DAV\Exception\MethodNotAllowed; +use Sabre\DAV\Exception\BadRequest; use Sabre\DAV\Server; use Sabre\HTTP\RequestInterface; use Sabre\HTTP\ResponseInterface; @@ -119,7 +119,7 @@ class FilesDropPluginTest extends TestCase { $this->request->method('getMethod') ->willReturn('MKCOL'); - $this->expectException(MethodNotAllowed::class); + $this->expectException(BadRequest::class); $this->plugin->beforeMethod($this->request, $this->response); } diff --git a/apps/files/src/utils/filenameValidity.ts b/apps/files/src/utils/filenameValidity.ts index 975e60050ad..2666d530052 100644 --- a/apps/files/src/utils/filenameValidity.ts +++ b/apps/files/src/utils/filenameValidity.ts @@ -8,16 +8,16 @@ import { t } from '@nextcloud/l10n' /** * Get the validity of a filename (empty if valid). * This can be used for `setCustomValidity` on input elements - * @param input The filename + * @param name The filename * @param escape Escape the matched string in the error (only set when used in HTML) */ -export function getFilenameValidity(input: string, escape = false): string { - if (input.trim() === '') { - return t('files', 'This field must not be empty.') +export function getFilenameValidity(name: string, escape = false): string { + if (name.trim() === '') { + return t('files', 'Filename must not be empty.') } try { - validateFilename(input) + validateFilename(name) return '' } catch (error) { if (!(error instanceof InvalidFilenameError)) { @@ -26,16 +26,16 @@ export function getFilenameValidity(input: string, escape = false): string { switch (error.reason) { case InvalidFilenameErrorReason.Character: - return t('files', 'The character "{char}" is not allowed.', { char: error.segment }, undefined, { escape }) + return t('files', '"{char}" is not allowed inside a filename.', { char: error.segment }, undefined, { escape }) case InvalidFilenameErrorReason.ReservedName: - return t('files', '"{segment}" is reserved and cannot be used.', { segment: error.segment }, undefined, { escape: false }) + return t('files', '"{segment}" is a reserved name and not allowed for filenames.', { segment: error.segment }, undefined, { escape: false }) case InvalidFilenameErrorReason.Extension: if (error.segment.match(/\.[a-z]/i)) { - return t('files', '"{extension}" is not a supported type.', { extension: error.segment }, undefined, { escape: false }) + return t('files', '"{extension}" is not an allowed filetype.', { extension: error.segment }, undefined, { escape: false }) } - return t('files', 'Cannot end with "{extension}".', { extension: error.segment }, undefined, { escape: false }) + return t('files', 'Filenames must not end with "{extension}".', { extension: error.segment }, undefined, { escape: false }) default: - return t('files', 'This value is not allowed.') + return t('files', 'Invalid filename.') } } } diff --git a/apps/files_sharing/src/services/GuestNameValidity.ts b/apps/files_sharing/src/services/GuestNameValidity.ts new file mode 100644 index 00000000000..249a1a6fbb4 --- /dev/null +++ b/apps/files_sharing/src/services/GuestNameValidity.ts @@ -0,0 +1,45 @@ +/*! + * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ +import { InvalidFilenameError, InvalidFilenameErrorReason, validateFilename } from '@nextcloud/files' +import { t } from '@nextcloud/l10n' + +/** + * Get the validity of a filename (empty if valid). + * This can be used for `setCustomValidity` on input elements + * @param name The filename + * @param escape Escape the matched string in the error (only set when used in HTML) + */ +export function getGuestNameValidity(name: string, escape = false): string { + if (name.trim() === '') { + return t('files', 'Filename must not be empty.') + } + + if (name.startsWith('.')) { + return t('files', 'Names must not start with a dot.') + } + + try { + validateFilename(name) + return '' + } catch (error) { + if (!(error instanceof InvalidFilenameError)) { + throw error + } + + switch (error.reason) { + case InvalidFilenameErrorReason.Character: + return t('files', '"{char}" is not allowed inside a name.', { char: error.segment }, undefined, { escape }) + case InvalidFilenameErrorReason.ReservedName: + return t('files', '"{segment}" is a reserved name and not allowed.', { segment: error.segment }, undefined, { escape: false }) + case InvalidFilenameErrorReason.Extension: + if (error.segment.match(/\.[a-z]/i)) { + return t('files', '"{extension}" is not an allowed name.', { extension: error.segment }, undefined, { escape: false }) + } + return t('files', 'Names must not end with "{extension}".', { extension: error.segment }, undefined, { escape: false }) + default: + return t('files', 'Invalid name.') + } + } +} diff --git a/apps/files_sharing/src/views/PublicAuthPrompt.vue b/apps/files_sharing/src/views/PublicAuthPrompt.vue index 124d88b52ed..28955a87154 100644 --- a/apps/files_sharing/src/views/PublicAuthPrompt.vue +++ b/apps/files_sharing/src/views/PublicAuthPrompt.vue @@ -42,7 +42,7 @@ import NcDialog from '@nextcloud/vue/components/NcDialog' import NcNoteCard from '@nextcloud/vue/components/NcNoteCard' import NcTextField from '@nextcloud/vue/components/NcTextField' -import { getFilenameValidity } from '../../../files/src/utils/filenameValidity' +import { getGuestNameValidity } from '../services/GuestNameValidity' export default defineComponent({ name: 'PublicAuthPrompt', @@ -112,7 +112,7 @@ export default defineComponent({ return } - const validity = getFilenameValidity(newName) + const validity = getGuestNameValidity(newName) input.setCustomValidity(validity) input.reportValidity() }, diff --git a/build/integration/filesdrop_features/filesdrop.feature b/build/integration/filesdrop_features/filesdrop.feature index e37a4d7138b..7618a31a1d0 100644 --- a/build/integration/filesdrop_features/filesdrop.feature +++ b/build/integration/filesdrop_features/filesdrop.feature @@ -44,7 +44,7 @@ Feature: FilesDrop And Updating last share with | permissions | 4 | When Dropping file "/folder/a.txt" with "abc" - Then the HTTP status code should be "405" + Then the HTTP status code should be "400" Scenario: Files drop forbid MKCOL without a nickname Given user "user0" exists @@ -57,7 +57,7 @@ Feature: FilesDrop And Updating last share with | permissions | 4 | When Creating folder "folder" in drop - Then the HTTP status code should be "405" + Then the HTTP status code should be "400" Scenario: Files drop allows MKCOL with a nickname Given user "user0" exists @@ -83,7 +83,7 @@ Feature: FilesDrop And Updating last share with | permissions | 4 | When dropping file "/folder/a.txt" with "abc" - Then the HTTP status code should be "405" + Then the HTTP status code should be "400" Scenario: Files request drop Given user "user0" exists @@ -195,4 +195,43 @@ Feature: FilesDrop | attributes | [{"scope":"fileRequest","key":"enabled","value":true}] | | shareWith | | When Dropping file "/folder/a.txt" with "abc" - Then the HTTP status code should be "405" + Then the HTTP status code should be "400" + + Scenario: Files request drop with invalid nickname with slashes + Given user "user0" exists + And As an "user0" + And user "user0" created a folder "/drop" + And as "user0" creating a share with + | path | drop | + | shareType | 4 | + | permissions | 4 | + | attributes | [{"scope":"fileRequest","key":"enabled","value":true}] | + | shareWith | | + When Dropping file "/folder/a.txt" with "abc" as "Alice/Bob/Mallory" + Then the HTTP status code should be "400" + + Scenario: Files request drop with invalid nickname with forbidden characters + Given user "user0" exists + And As an "user0" + And user "user0" created a folder "/drop" + And as "user0" creating a share with + | path | drop | + | shareType | 4 | + | permissions | 4 | + | attributes | [{"scope":"fileRequest","key":"enabled","value":true}] | + | shareWith | | + When Dropping file "/folder/a.txt" with "abc" as ".htaccess" + Then the HTTP status code should be "400" + + Scenario: Files request drop with invalid nickname with forbidden characters + Given user "user0" exists + And As an "user0" + And user "user0" created a folder "/drop" + And as "user0" creating a share with + | path | drop | + | shareType | 4 | + | permissions | 4 | + | attributes | [{"scope":"fileRequest","key":"enabled","value":true}] | + | shareWith | | + When Dropping file "/folder/a.txt" with "abc" as ".Mallory" + Then the HTTP status code should be "400" diff --git a/lib/private/Files/Node/Folder.php b/lib/private/Files/Node/Folder.php index 16365948031..c41838fd6b0 100644 --- a/lib/private/Files/Node/Folder.php +++ b/lib/private/Files/Node/Folder.php @@ -460,4 +460,12 @@ class Folder extends Node implements \OCP\Files\Folder { return $this->search($query); } + + public function verifyPath($fileName, $readonly = false): void { + $this->view->verifyPath( + $this->getPath(), + $fileName, + $readonly, + ); + } } diff --git a/lib/private/Files/Node/LazyFolder.php b/lib/private/Files/Node/LazyFolder.php index 5879748d951..37b1efa0fad 100644 --- a/lib/private/Files/Node/LazyFolder.php +++ b/lib/private/Files/Node/LazyFolder.php @@ -561,4 +561,8 @@ class LazyFolder implements Folder { public function getMetadata(): array { return $this->data['metadata'] ?? $this->__call(__FUNCTION__, func_get_args()); } + + public function verifyPath($fileName, $readonly = false): void { + $this->__call(__FUNCTION__, func_get_args()); + } } diff --git a/lib/public/Files/Folder.php b/lib/public/Files/Folder.php index 3128a17c10c..a35d2d78bc9 100644 --- a/lib/public/Files/Folder.php +++ b/lib/public/Files/Folder.php @@ -199,4 +199,15 @@ interface Folder extends Node { * @since 9.1.0 */ public function getRecent($limit, $offset = 0); + + /** + * Verify if the given path is valid and allowed from this folder. + * + * @param string $path the path from this folder + * @param string $fileName + * @param bool $readonly Check only if the path is allowed for read-only access + * @throws InvalidPathException + * @since 32.0.0 + */ + public function verifyPath($fileName, $readonly = false): void; }