Merge pull request #47372 from nextcloud/fix/filename-validator

fix: `FilenameValidator::isForbidden` should only check forbidden files

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.
  * 

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]));
+				}
 			}
 		}
 	}

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,
 			],
 		];
 	}