From bdbeabafa7ee6596934f7a84c4574f434e12fe1f Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Mon, 15 Jul 2024 19:10:52 +0200 Subject: [PATCH] feat: Add `forbidden_filename_basenames` config option This allows to configure forbidden filenames (the full filename like `.htaccess`) and also forbidden basenames like `com0` where `com0`, `com0.txt` and `com0.tar.gz` will match. We need this as only using basenames was too restrictive and will cause problems on some systems when updating. Signed-off-by: Ferdinand Thiessen --- apps/files/lib/Capabilities.php | 3 +- apps/files/openapi.json | 7 ++ config/config.sample.php | 12 ++++ lib/private/Files/FilenameValidator.php | 71 +++++++++++++------ tests/lib/Files/FilenameValidatorTest.php | 85 ++++++++++++++++------- 5 files changed, 127 insertions(+), 51 deletions(-) diff --git a/apps/files/lib/Capabilities.php b/apps/files/lib/Capabilities.php index b024307c25be7..fdbbdf63f22b1 100644 --- a/apps/files/lib/Capabilities.php +++ b/apps/files/lib/Capabilities.php @@ -20,7 +20,7 @@ public function __construct( /** * Return this classes capabilities * - * @return array{files: array{'$comment': ?string, bigfilechunking: bool, blacklisted_files: array, forbidden_filenames: list, forbidden_filename_characters: list, forbidden_filename_extensions: list}} + * @return array{files: array{'$comment': ?string, bigfilechunking: bool, blacklisted_files: array, forbidden_filenames: list, forbidden_filename_basenames: list, forbidden_filename_characters: list, forbidden_filename_extensions: list}} */ public function getCapabilities(): array { return [ @@ -28,6 +28,7 @@ public function getCapabilities(): array { '$comment' => '"blacklisted_files" is deprecated as of Nextcloud 30, use "forbidden_filenames" instead', 'blacklisted_files' => $this->filenameValidator->getForbiddenFilenames(), 'forbidden_filenames' => $this->filenameValidator->getForbiddenFilenames(), + 'forbidden_filename_basenames' => $this->filenameValidator->getForbiddenBasenames(), 'forbidden_filename_characters' => $this->filenameValidator->getForbiddenCharacters(), 'forbidden_filename_extensions' => $this->filenameValidator->getForbiddenExtensions(), diff --git a/apps/files/openapi.json b/apps/files/openapi.json index 7fc6bc3e0b0fc..6fff32e485452 100644 --- a/apps/files/openapi.json +++ b/apps/files/openapi.json @@ -33,6 +33,7 @@ "bigfilechunking", "blacklisted_files", "forbidden_filenames", + "forbidden_filename_basenames", "forbidden_filename_characters", "forbidden_filename_extensions", "directEditing" @@ -57,6 +58,12 @@ "type": "string" } }, + "forbidden_filename_basenames": { + "type": "array", + "items": { + "type": "string" + } + }, "forbidden_filename_characters": { "type": "array", "items": { diff --git a/config/config.sample.php b/config/config.sample.php index 76b6532a19c46..67110a1844aa6 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -1987,6 +1987,18 @@ */ 'forbidden_filenames' => ['.htaccess'], +/** + * Disallow the upload of files with specific basenames. + * + * The basename is the name of the file without the extension, + * e.g. for "archive.tar.gz" the basename would be "archive". + * + * Note that this list is case-insensitive. + * + * Defaults to ``array()`` + */ +'forbidden_filename_basenames' => [], + /** * 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. diff --git a/lib/private/Files/FilenameValidator.php b/lib/private/Files/FilenameValidator.php index d650089e5c478..0026dfdf451ca 100644 --- a/lib/private/Files/FilenameValidator.php +++ b/lib/private/Files/FilenameValidator.php @@ -30,6 +30,10 @@ class FilenameValidator implements IFilenameValidator { */ private array $forbiddenNames = []; + /** + * @var list + */ + private array $forbiddenBasenames = []; /** * @var list */ @@ -56,17 +60,11 @@ public function __construct( */ public function getForbiddenExtensions(): array { if (empty($this->forbiddenExtensions)) { - $forbiddenExtensions = $this->config->getSystemValue('forbidden_filename_extensions', ['.filepart']); - if (!is_array($forbiddenExtensions)) { - $this->logger->error('Invalid system config value for "forbidden_filename_extensions" is ignored.'); - $forbiddenExtensions = ['.filepart']; - } + $forbiddenExtensions = $this->getConfigValue('forbidden_filename_extensions', ['.filepart']); // Always forbid .part files as they are used internally - $forbiddenExtensions = array_merge($forbiddenExtensions, ['.part']); + $forbiddenExtensions[] = '.part'; - // The list is case insensitive so we provide it always lowercase - $forbiddenExtensions = array_map('mb_strtolower', $forbiddenExtensions); $this->forbiddenExtensions = array_values($forbiddenExtensions); } return $this->forbiddenExtensions; @@ -80,31 +78,37 @@ public function getForbiddenExtensions(): array { */ public function getForbiddenFilenames(): array { if (empty($this->forbiddenNames)) { - $forbiddenNames = $this->config->getSystemValue('forbidden_filenames', ['.htaccess']); - if (!is_array($forbiddenNames)) { - $this->logger->error('Invalid system config value for "forbidden_filenames" is ignored.'); - $forbiddenNames = ['.htaccess']; - } + $forbiddenNames = $this->getConfigValue('forbidden_filenames', ['.htaccess']); // Handle legacy config option // TODO: Drop with Nextcloud 34 - $legacyForbiddenNames = $this->config->getSystemValue('blacklisted_files', []); - if (!is_array($legacyForbiddenNames)) { - $this->logger->error('Invalid system config value for "blacklisted_files" is ignored.'); - $legacyForbiddenNames = []; - } + $legacyForbiddenNames = $this->getConfigValue('blacklisted_files', []); if (!empty($legacyForbiddenNames)) { $this->logger->warning('System config option "blacklisted_files" is deprecated and will be removed in Nextcloud 34, use "forbidden_filenames" instead.'); } $forbiddenNames = array_merge($legacyForbiddenNames, $forbiddenNames); - // The list is case insensitive so we provide it always lowercase - $forbiddenNames = array_map('mb_strtolower', $forbiddenNames); + // Ensure we are having a proper string list $this->forbiddenNames = array_values($forbiddenNames); } return $this->forbiddenNames; } + /** + * Get a list of forbidden file basenames that must not be used + * This list should be checked case-insensitive, all names are returned lowercase. + * @return list + * @since 30.0.0 + */ + public function getForbiddenBasenames(): array { + if (empty($this->forbiddenBasenames)) { + $forbiddenBasenames = $this->getConfigValue('forbidden_filename_basenames', []); + // Ensure we are having a proper string list + $this->forbiddenBasenames = array_values($forbiddenBasenames); + } + return $this->forbiddenBasenames; + } + /** * Get a list of characters forbidden in filenames * @@ -194,6 +198,7 @@ public function validateFilename(string $filename): void { * @return bool True if invalid name, False otherwise */ public function isForbidden(string $path): bool { + // We support paths here as this function is also used in some storage internals $filename = basename($path); $filename = mb_strtolower($filename); @@ -201,10 +206,16 @@ public function isForbidden(string $path): bool { return false; } - // The name part without extension - $basename = substr($filename, 0, strpos($filename, '.', 1) ?: null); // Check for forbidden filenames $forbiddenNames = $this->getForbiddenFilenames(); + if (in_array($filename, $forbiddenNames)) { + return true; + } + + // 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; } @@ -226,7 +237,7 @@ protected function checkForbiddenCharacters(string $filename): void { foreach ($this->getForbiddenCharacters() as $char) { if (str_contains($filename, $char)) { - throw new InvalidCharacterInPathException($char); + throw new InvalidCharacterInPathException($this->l10n->t('Invalid character "%1$s" in filename', [$char])); } } } @@ -246,4 +257,18 @@ protected function checkForbiddenExtension(string $filename): void { } } } + + /** + * Helper to get lower case list from config with validation + * @return string[] + */ + private function getConfigValue(string $key, array $fallback): array { + $values = $this->config->getSystemValue($key, ['.filepart']); + if (!is_array($values)) { + $this->logger->error('Invalid system config value for "' . $key . '" is ignored.'); + $values = $fallback; + } + + return array_map('mb_strtolower', $values); + } }; diff --git a/tests/lib/Files/FilenameValidatorTest.php b/tests/lib/Files/FilenameValidatorTest.php index ec67e208b919b..42705a23f0255 100644 --- a/tests/lib/Files/FilenameValidatorTest.php +++ b/tests/lib/Files/FilenameValidatorTest.php @@ -49,16 +49,24 @@ protected function setUp(): void { public function testValidateFilename( string $filename, array $forbiddenNames, + array $forbiddenBasenames, array $forbiddenExtensions, array $forbiddenCharacters, ?string $exception, ): void { /** @var FilenameValidator&MockObject */ $validator = $this->getMockBuilder(FilenameValidator::class) - ->onlyMethods(['getForbiddenExtensions', 'getForbiddenFilenames', 'getForbiddenCharacters']) + ->onlyMethods([ + 'getForbiddenBasenames', + 'getForbiddenCharacters', + 'getForbiddenExtensions', + 'getForbiddenFilenames', + ]) ->setConstructorArgs([$this->l10n, $this->config, $this->logger]) ->getMock(); + $validator->method('getForbiddenBasenames') + ->willReturn($forbiddenBasenames); $validator->method('getForbiddenCharacters') ->willReturn($forbiddenCharacters); $validator->method('getForbiddenExtensions') @@ -80,16 +88,24 @@ public function testValidateFilename( public function testIsFilenameValid( string $filename, array $forbiddenNames, + array $forbiddenBasenames, array $forbiddenExtensions, array $forbiddenCharacters, ?string $exception, ): void { /** @var FilenameValidator&MockObject */ $validator = $this->getMockBuilder(FilenameValidator::class) - ->onlyMethods(['getForbiddenExtensions', 'getForbiddenFilenames', 'getForbiddenCharacters']) + ->onlyMethods([ + 'getForbiddenBasenames', + 'getForbiddenExtensions', + 'getForbiddenFilenames', + 'getForbiddenCharacters', + ]) ->setConstructorArgs([$this->l10n, $this->config, $this->logger]) ->getMock(); + $validator->method('getForbiddenBasenames') + ->willReturn($forbiddenBasenames); $validator->method('getForbiddenCharacters') ->willReturn($forbiddenCharacters); $validator->method('getForbiddenExtensions') @@ -104,84 +120,99 @@ public function testIsFilenameValid( public function dataValidateFilename(): array { return [ 'valid name' => [ - 'a: b.txt', ['.htaccess'], [], [], null + 'a: b.txt', ['.htaccess'], [], [], [], null ], 'valid name with some more parameters' => [ - 'a: b.txt', ['.htaccess'], ['exe'], ['~'], null + 'a: b.txt', ['.htaccess'], [], ['exe'], ['~'], null + ], + 'valid name checks only the full name' => [ + '.htaccess.sample', ['.htaccess'], [], [], [], null ], 'forbidden name' => [ - '.htaccess', ['.htaccess'], [], [], ReservedWordException::class + '.htaccess', ['.htaccess'], [], [], [], ReservedWordException::class ], 'forbidden name - name is case insensitive' => [ - 'COM1', ['.htaccess', 'com1'], [], [], ReservedWordException::class + 'COM1', ['.htaccess', 'com1'], [], [], [], ReservedWordException::class + ], + 'forbidden basename' => [ + // needed for Windows namespaces + 'com1.suffix', ['.htaccess'], ['com1'], [], [], ReservedWordException::class ], - 'forbidden name - name checks the filename' => [ + 'forbidden basename for hidden files' => [ // needed for Windows namespaces - 'com1.suffix', ['.htaccess', 'com1'], [], [], ReservedWordException::class + '.thumbs.db', ['.htaccess'], ['.thumbs'], [], [], ReservedWordException::class ], 'invalid character' => [ - 'a: b.txt', ['.htaccess'], [], [':'], InvalidCharacterInPathException::class + 'a: b.txt', ['.htaccess'], [], [], [':'], InvalidCharacterInPathException::class ], 'invalid path' => [ - '../../foo.bar', ['.htaccess'], [], ['/', '\\'], InvalidCharacterInPathException::class, + '../../foo.bar', ['.htaccess'], [], [], ['/', '\\'], InvalidCharacterInPathException::class, ], 'invalid extension' => [ - 'a: b.txt', ['.htaccess'], ['.txt'], [], InvalidPathException::class + 'a: b.txt', ['.htaccess'], [], ['.txt'], [], InvalidPathException::class ], 'empty filename' => [ - '', [], [], [], EmptyFileNameException::class + '', [], [], [], [], EmptyFileNameException::class ], 'reserved unix name "."' => [ - '.', [], [], [], InvalidPathException::class + '.', [], [], [], [], InvalidPathException::class ], 'reserved unix name ".."' => [ - '..', [], [], [], ReservedWordException::class + '..', [], [], [], [], ReservedWordException::class ], 'too long filename "."' => [ - str_repeat('a', 251), [], [], [], FileNameTooLongException::class + str_repeat('a', 251), [], [], [], [], FileNameTooLongException::class ], // make sure to not split the list entries as they migh contain Unicode sequences // in this example the "face in clouds" emoji contains the clouds emoji so only having clouds is ok - ['🌫️.txt', ['.htaccess'], [], ['πŸ˜Άβ€πŸŒ«οΈ'], null], + ['🌫️.txt', ['.htaccess'], [], [], ['πŸ˜Άβ€πŸŒ«οΈ'], null], // This is the reverse: clouds are forbidden -> so is also the face in the clouds emoji - ['πŸ˜Άβ€πŸŒ«οΈ.txt', ['.htaccess'], [], ['🌫️'], InvalidCharacterInPathException::class], + ['πŸ˜Άβ€πŸŒ«οΈ.txt', ['.htaccess'], [], [], ['🌫️'], InvalidCharacterInPathException::class], ]; } /** * @dataProvider dataIsForbidden */ - public function testIsForbidden(string $filename, array $forbiddenNames, bool $expected): void { + public function testIsForbidden(string $filename, array $forbiddenNames, array $forbiddenBasenames, bool $expected): void { /** @var FilenameValidator&MockObject */ $validator = $this->getMockBuilder(FilenameValidator::class) - ->onlyMethods(['getForbiddenFilenames']) + ->onlyMethods(['getForbiddenFilenames', 'getForbiddenBasenames']) ->setConstructorArgs([$this->l10n, $this->config, $this->logger]) ->getMock(); + $validator->method('getForbiddenBasenames') + ->willReturn($forbiddenBasenames); $validator->method('getForbiddenFilenames') ->willReturn($forbiddenNames); - - $this->assertEquals($expected, $validator->isFilenameValid($filename)); + $this->assertEquals($expected, $validator->isForbidden($filename)); } public function dataIsForbidden(): array { return [ 'valid name' => [ - 'a: b.txt', ['.htaccess'], true + 'a: b.txt', ['.htaccess'], [], false ], 'valid name with some more parameters' => [ - 'a: b.txt', ['.htaccess'], true + 'a: b.txt', ['.htaccess'], [], false + ], + 'valid name as only full forbidden should be matched' => [ + '.htaccess.sample', ['.htaccess'], [], false, ], 'forbidden name' => [ - '.htaccess', ['.htaccess'], false + '.htaccess', ['.htaccess'], [], true ], 'forbidden name - name is case insensitive' => [ - 'COM1', ['.htaccess', 'com1'], false + 'COM1', ['.htaccess', 'com1'], [], true, + ], + 'forbidden name - basename is checked' => [ + // needed for Windows namespaces + 'com1.suffix', ['.htaccess'], ['com1'], true ], - 'forbidden name - name checks the filename' => [ + 'forbidden name - basename is checked also with multiple extensions' => [ // needed for Windows namespaces - 'com1.suffix', ['.htaccess', 'com1'], false + 'com1.tar.gz', ['.htaccess'], ['com1'], true ], ]; }