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 <opensource@fthiessen.de>
2 years ago · bdbeabafa7
parent 8c571dda47
commit bdbeabafa7
5 changed files with 127 additions and 51 deletions
--- a/apps/files/lib/Capabilities.php
+++ b/apps/files/lib/Capabilities.php
@ -20,7 +20,7 @@ class Capabilities implements ICapability {
 	/**
 	 * Return this classes capabilities
 	 *
-	 * @return array{files: array{'$comment': ?string, bigfilechunking: bool, blacklisted_files: array<mixed>, forbidden_filenames: list<string>, forbidden_filename_characters: list<string>, forbidden_filename_extensions: list<string>}}
+	 * @return array{files: array{'$comment': ?string, bigfilechunking: bool, blacklisted_files: array<mixed>, forbidden_filenames: list<string>, forbidden_filename_basenames: list<string>, forbidden_filename_characters: list<string>, forbidden_filename_extensions: list<string>}}
 	 */
 	public function getCapabilities(): array {
 		return [
@ -28,6 +28,7 @@ class Capabilities implements ICapability {
 				'$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(),

--- 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": {
--- a/config/config.sample.php
+++ b/config/config.sample.php
@ -1987,6 +1987,18 @@ $CONFIG = [
 */
 '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.
--- 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<string>
+	 */
+	private array $forbiddenBasenames = [];
 	/**
 	 * @var list<string>
 	 */
@ -56,17 +60,11 @@ class FilenameValidator implements IFilenameValidator {
 	 */
 	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 @@ class FilenameValidator implements IFilenameValidator {
 	 */
 	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<string>
+	 * @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 @@ class FilenameValidator implements IFilenameValidator {
 	 * @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 @@ class FilenameValidator implements IFilenameValidator {
 			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 @@ class FilenameValidator implements IFilenameValidator {

 		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 @@ class FilenameValidator implements IFilenameValidator {
 			}
 		}
 	}
+
+	/**
+	 * 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);
+	}
 };
--- a/tests/lib/Files/FilenameValidatorTest.php
+++ b/tests/lib/Files/FilenameValidatorTest.php
@ -49,16 +49,24 @@ class FilenameValidatorTest extends TestCase {
 	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 @@ class FilenameValidatorTest extends TestCase {
 	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 @@ class FilenameValidatorTest extends TestCase {
 	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
 			],
 		];
 	}