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

And not forbidden basenames as this is used for different purposes.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

2 files changed, 25 insertions, 26 deletions

diff --git a/lib/private/Files/FilenameValidator.php b/lib/private/Files/FilenameValidator.php
index b1ce8e02b13..2fe3c93d026 100644
--- a/lib/private/Files/FilenameValidator.php
+++ b/lib/private/Files/FilenameValidator.php
@@ -198,9 +198,7 @@ class FilenameValidator implements IFilenameValidator {
 			}
 		}
 
-		if ($this->isForbidden($filename)) {
-			throw new ReservedWordException();
-		}
+		$this->checkForbiddenName($filename);
 
 		$this->checkForbiddenExtension($filename);
 
@@ -227,18 +225,25 @@ class FilenameValidator implements IFilenameValidator {
 			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 @@ class FilenameValidator implements IFilenameValidator {
 
 		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 @@ class FilenameValidator implements IFilenameValidator {
 		$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 1fcc03064a2..ac9ac032b64 100644
--- a/tests/lib/Files/FilenameValidatorTest.php
+++ b/tests/lib/Files/FilenameValidatorTest.php
@@ -252,15 +252,13 @@ class FilenameValidatorTest extends TestCase {
 	/**
 	 * @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 @@ class FilenameValidatorTest extends TestCase {
 	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,
 			],
 		];
 	}