From b9cc7bcec71854665008e1332f6a7d3f39249d50 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Mon, 12 Aug 2024 18:11:31 +0200 Subject: [PATCH 1/2] fix: `FilenameValidator::isForbidden` should only check forbidden files And not forbidden basenames as this is used for different purposes. Signed-off-by: Ferdinand Thiessen --- lib/private/Files/FilenameValidator.php | 27 +++++++++++++++-------- tests/lib/Files/FilenameValidatorTest.php | 24 ++++++-------------- 2 files changed, 25 insertions(+), 26 deletions(-) diff --git a/lib/private/Files/FilenameValidator.php b/lib/private/Files/FilenameValidator.php index b1ce8e02b13ee..2fe3c93d02664 100644 --- a/lib/private/Files/FilenameValidator.php +++ b/lib/private/Files/FilenameValidator.php @@ -198,9 +198,7 @@ public function validateFilename(string $filename): void { } } - if ($this->isForbidden($filename)) { - throw new ReservedWordException(); - } + $this->checkForbiddenName($filename); $this->checkForbiddenExtension($filename); @@ -227,18 +225,25 @@ public function isForbidden(string $path): bool { return true; } + // Filename is not forbidden + return false; + } + + protected function checkForbiddenName($filename): void { + if ($this->isForbidden($filename)) { + throw new ReservedWordException($this->l10n->t('"%1$s" is a forbidden file or folder name.', [$filename])); + } + // Check for forbidden basenames - basenames are the part of the file until the first dot // (except if the dot is the first character as this is then part of the basename "hidden files") $basename = substr($filename, 0, strpos($filename, '.', 1) ?: null); $forbiddenNames = $this->getForbiddenBasenames(); if (in_array($basename, $forbiddenNames)) { - return true; + throw new ReservedWordException($this->l10n->t('"%1$s" is a forbidden prefix for file or folder names.', [$filename])); } - - // Filename is not forbidden - return false; } + /** * Check if a filename contains any of the forbidden characters * @param string $filename @@ -252,7 +257,7 @@ protected function checkForbiddenCharacters(string $filename): void { foreach ($this->getForbiddenCharacters() as $char) { if (str_contains($filename, $char)) { - throw new InvalidCharacterInPathException($this->l10n->t('Invalid character "%1$s" in filename', [$char])); + throw new InvalidCharacterInPathException($this->l10n->t('"%1$s" is not allowed inside a file or folder name.', [$char])); } } } @@ -268,7 +273,11 @@ protected function checkForbiddenExtension(string $filename): void { $forbiddenExtensions = $this->getForbiddenExtensions(); foreach ($forbiddenExtensions as $extension) { if (str_ends_with($filename, $extension)) { - throw new InvalidPathException($this->l10n->t('Invalid filename extension "%1$s"', [$extension])); + if (str_starts_with($extension, '.')) { + throw new InvalidPathException($this->l10n->t('"%1$s" is a forbidden file type.', [$extension])); + } else { + throw new InvalidPathException($this->l10n->t('Filenames must not end with "%1$s".', [$extension])); + } } } } diff --git a/tests/lib/Files/FilenameValidatorTest.php b/tests/lib/Files/FilenameValidatorTest.php index 1fcc03064a2a2..ac9ac032b6435 100644 --- a/tests/lib/Files/FilenameValidatorTest.php +++ b/tests/lib/Files/FilenameValidatorTest.php @@ -252,15 +252,13 @@ public function dataInvalidAsciiCharacters(): array { /** * @dataProvider dataIsForbidden */ - public function testIsForbidden(string $filename, array $forbiddenNames, array $forbiddenBasenames, bool $expected): void { + public function testIsForbidden(string $filename, array $forbiddenNames, bool $expected): void { /** @var FilenameValidator&MockObject */ $validator = $this->getMockBuilder(FilenameValidator::class) - ->onlyMethods(['getForbiddenFilenames', 'getForbiddenBasenames']) + ->onlyMethods(['getForbiddenFilenames']) ->setConstructorArgs([$this->l10n, $this->database, $this->config, $this->logger]) ->getMock(); - $validator->method('getForbiddenBasenames') - ->willReturn($forbiddenBasenames); $validator->method('getForbiddenFilenames') ->willReturn($forbiddenNames); @@ -270,27 +268,19 @@ public function testIsForbidden(string $filename, array $forbiddenNames, array $ public function dataIsForbidden(): array { return [ 'valid name' => [ - 'a: b.txt', ['.htaccess'], [], false + 'a: b.txt', ['.htaccess'], false ], 'valid name with some more parameters' => [ - 'a: b.txt', ['.htaccess'], [], false + 'a: b.txt', ['.htaccess'], false ], 'valid name as only full forbidden should be matched' => [ - '.htaccess.sample', ['.htaccess'], [], false, + '.htaccess.sample', ['.htaccess'], false, ], 'forbidden name' => [ - '.htaccess', ['.htaccess'], [], true + '.htaccess', ['.htaccess'], true ], 'forbidden name - name is case insensitive' => [ - 'COM1', ['.htaccess', 'com1'], [], true, - ], - 'forbidden name - basename is checked' => [ - // needed for Windows namespaces - 'com1.suffix', ['.htaccess'], ['com1'], true - ], - 'forbidden name - basename is checked also with multiple extensions' => [ - // needed for Windows namespaces - 'com1.tar.gz', ['.htaccess'], ['com1'], true + 'COM1', ['.htaccess', 'com1'], true, ], ]; } From 91573781dfdabdac55307004aa554f8ade003289 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Tue, 13 Aug 2024 10:29:53 +0200 Subject: [PATCH 2/2] docs: Add information how forbidden filenames are handled on existing files. Signed-off-by: Ferdinand Thiessen --- config/config.sample.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/config/config.sample.php b/config/config.sample.php index 561416b7275d9..87aef75fe78e4 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -2008,8 +2008,9 @@ 'updatedirectory' => '', /** - * Block a specific file or files and disallow the upload of files - * with this name. ``.htaccess`` is blocked by default. + * Block a specific file or files and disallow the upload of files with this name. + * This blocks any access to those files (read and write). + * ``.htaccess`` is blocked by default. * * WARNING: USE THIS ONLY IF YOU KNOW WHAT YOU ARE DOING. * @@ -2021,6 +2022,7 @@ /** * Disallow the upload of files with specific basenames. + * Matching existing files can no longer be updated and in matching folders no files can be created anymore. * * The basename is the name of the file without the extension, * e.g. for "archive.tar.gz" the basename would be "archive". @@ -2034,6 +2036,7 @@ /** * Block characters from being used in filenames. This is useful if you * have a filesystem or OS which does not support certain characters like windows. + * Matching existing files can no longer be updated and in matching folders no files can be created anymore. * * The '/' and '\' characters are always forbidden, as well as all characters in the ASCII range [0-31]. * @@ -2046,6 +2049,7 @@ /** * Deny extensions from being used for filenames. + * Matching existing files can no longer be updated and in matching folders no files can be created anymore. * * The '.part' extension is always forbidden, as this is used internally by Nextcloud. *