feat(FilenameValidator): allow to sanitize filenames

Share the filename sanitizing with the OCP filename validator.

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

apps/files/lib/Command/SanitizeFilenames.php

@@ -27,7 +27,7 @@ use Symfony\Component\Console\Output\OutputInterface;
 class SanitizeFilenames extends Base {
 
 	private OutputInterface $output;
-	private string $charReplacement;
+	private ?string $charReplacement;
 	private bool $dryRun;
 
 	public function __construct(
@@ -43,10 +43,6 @@ class SanitizeFilenames extends Base {
 	protected function configure(): void {
 		parent::configure();
 
-		$forbiddenCharacter = $this->filenameValidator->getForbiddenCharacters();
-		$charReplacement = array_diff([' ', '_', '-'], $forbiddenCharacter);
-		$charReplacement = reset($charReplacement) ?: '';
-
 		$this
 			->setName('files:sanitize-filenames')
 			->setDescription('Renames files to match naming constraints')
@@ -65,16 +61,25 @@ class SanitizeFilenames extends Base {
 				'c',
 				mode: InputOption::VALUE_REQUIRED,
 				description: 'Replacement for invalid character (by default space, underscore or dash is used)',
-				default: $charReplacement,
 			);
 			
 	}
 
 	protected function execute(InputInterface $input, OutputInterface $output): int {
 		$this->charReplacement = $input->getOption('char-replacement');
-		if ($this->charReplacement === '' || mb_strlen($this->charReplacement) > 1) {
-			$output->writeln('<error>No character replacement given</error>');
-			return 1;
+		// check if replacement is needed
+		$c = $this->filenameValidator->getForbiddenCharacters();
+		if (count($c) > 0) {
+			try {
+				$this->filenameValidator->sanitizeFilename($c[0], $this->charReplacement);
+			} catch (\InvalidArgumentException) {
+				if ($this->charReplacement === null) {
+					$output->writeln('<error>Character replacement required</error>');
+				} else {
+					$output->writeln('<error>Invalid character replacement given</error>');
+				}
+				return 1;
+			}
 		}
 
 		$this->dryRun = $input->getOption('dry-run');
@@ -115,8 +120,8 @@ class SanitizeFilenames extends Base {
 
 			try {
 				$oldName = $node->getName();
-				if (!$this->filenameValidator->isFilenameValid($oldName)) {
-					$newName = $this->sanitizeName($oldName);
+				$newName = $this->filenameValidator->sanitizeFilename($oldName, $this->charReplacement);
+				if ($oldName !== $newName) {
 					$newName = $folder->getNonExistingName($newName);
 					$path = rtrim(dirname($node->getPath()), '/');
 
@@ -142,27 +147,4 @@ class SanitizeFilenames extends Base {
 		}
 	}
 
-	private function sanitizeName(string $name): string {
-		$l10n = $this->l10nFactory->get('files');
-
-		foreach ($this->filenameValidator->getForbiddenExtensions() as $extension) {
-			if (str_ends_with($name, $extension)) {
-				$name = substr($name, 0, strlen($name) - strlen($extension));
-			}
-		}
-
-		$basename = substr($name, 0, strpos($name, '.', 1) ?: null);
-		if (in_array($basename, $this->filenameValidator->getForbiddenBasenames())) {
-			$name = str_replace($basename, $l10n->t('%1$s (renamed)', [$basename]), $name);
-		}
-
-		if ($name === '') {
-			$name = $l10n->t('renamed file');
-		}
-
-		$forbiddenCharacter = $this->filenameValidator->getForbiddenCharacters();
-		$name = str_replace($forbiddenCharacter, $this->charReplacement, $name);
-
-		return $name;
-	}
 }

lib/private/Files/FilenameValidator.php

@@ -228,6 +228,43 @@ class FilenameValidator implements IFilenameValidator {
 		return false;
 	}
 
+	public function sanitizeFilename(string $name, ?string $charReplacement = null): string {
+		$forbiddenCharacters = $this->getForbiddenCharacters();
+
+		if ($charReplacement === null) {
+			$charReplacement = array_diff([' ', '_', '-'], $forbiddenCharacters);
+			$charReplacement = reset($charReplacement) ?: '';
+		}
+		if (mb_strlen($charReplacement) !== 1) {
+			throw new \InvalidArgumentException('No or invalid character replacement given');
+		}
+
+		$nameLowercase = mb_strtolower($name);
+		foreach ($this->getForbiddenExtensions() as $extension) {
+			if (str_ends_with($nameLowercase, $extension)) {
+				$name = substr($name, 0, strlen($name) - strlen($extension));
+			}
+		}
+
+		$basename = strlen($name) > 1
+			? substr($name, 0, strpos($name, '.', 1) ?: null)
+			: $name;
+		if (in_array(mb_strtolower($basename), $this->getForbiddenBasenames())) {
+			$name = str_replace($basename, $this->l10n->t('%1$s (renamed)', [$basename]), $name);
+		}
+
+		if ($name === '') {
+			$name = $this->l10n->t('renamed file');
+		}
+
+		if (in_array(mb_strtolower($name), $this->getForbiddenFilenames())) {
+			$name = $this->l10n->t('%1$s (renamed)', [$name]);
+		}
+
+		$name = str_replace($forbiddenCharacters, $charReplacement, $name);
+		return $name;
+	}
+
 	protected function checkForbiddenName(string $filename): void {
 		$filename = mb_strtolower($filename);
 		if ($this->isForbidden($filename)) {

lib/public/Files/IFilenameValidator.php

@@ -36,4 +36,17 @@ interface IFilenameValidator {
 	 * @since 30.0.0
 	 */
 	public function validateFilename(string $filename): void;
+
+	/**
+	 * Sanitize a give filename to comply with admin setup naming constrains.
+	 *
+	 * If no sanitizing is needed the same name is returned.
+	 *
+	 * @param string $name The filename to sanitize
+	 * @param null|string $charReplacement Character to use for replacing forbidden ones - by default space, dash or underscore is used if allowed.
+	 * @throws \InvalidArgumentException if no character replacement was given (and the default could not be applied) or the replacement is not valid.
+	 * @since 32.0.0
+	 */
+	public function sanitizeFilename(string $name, ?string $charReplacement = null): string;
+
 }

tests/lib/Files/FilenameValidatorTest.php

@@ -374,4 +374,138 @@ class FilenameValidatorTest extends TestCase {
 			[['AuX', 'COM1'], ['aux', 'com1']],
 		];
 	}
+
+	/**
+	 * @dataProvider dataSanitizeFilename
+	 */
+	public function testSanitizeFilename(
+		string $filename,
+		array $forbiddenNames,
+		array $forbiddenBasenames,
+		array $forbiddenExtensions,
+		array $forbiddenCharacters,
+		string $expected,
+	): void {
+		/** @var FilenameValidator&MockObject */
+		$validator = $this->getMockBuilder(FilenameValidator::class)
+			->onlyMethods([
+				'getForbiddenBasenames',
+				'getForbiddenExtensions',
+				'getForbiddenFilenames',
+				'getForbiddenCharacters',
+			])
+			->setConstructorArgs([$this->l10n, $this->database, $this->config, $this->logger])
+			->getMock();
+
+		$validator->method('getForbiddenBasenames')
+			->willReturn($forbiddenBasenames);
+		$validator->method('getForbiddenCharacters')
+			->willReturn($forbiddenCharacters);
+		$validator->method('getForbiddenExtensions')
+			->willReturn($forbiddenExtensions);
+		$validator->method('getForbiddenFilenames')
+			->willReturn($forbiddenNames);
+
+		$this->assertEquals($expected, $validator->sanitizeFilename($filename));
+	}
+
+	public function dataSanitizeFilename(): array {
+		return [
+			'valid name' => [
+				'a * b.txt', ['.htaccess'], [], [], [], 'a * b.txt'
+			],
+			'forbidden name in the middle is ok' => [
+				'a.htaccess.txt', ['.htaccess'], [], [], [], 'a.htaccess.txt'
+			],
+			'forbidden name on the beginning' => [
+				'.htaccess.sample', ['.htaccess'], [], [], [], '.htaccess.sample'
+			],
+			'forbidden name' => [
+				'.htaccess', ['.htaccess'], [], [], [], '.htaccess (renamed)'
+			],
+			'forbidden name - name is case insensitive' => [
+				'COM1', ['.htaccess', 'com1'], [], [], [], 'COM1 (renamed)'
+			],
+			'forbidden basename' => [
+				'com1.suffix', ['.htaccess'], ['com1'], [], [], 'com1 (renamed).suffix'
+			],
+			'forbidden basename case insensitive' => [
+				// needed for Windows namespaces
+				'COM1.suffix', ['.htaccess'], ['com1'], [], [], 'COM1 (renamed).suffix'
+			],
+			'forbidden basename for hidden files' => [
+				// needed for Windows namespaces
+				'.thumbs.db', ['.htaccess'], ['.thumbs'], [], [], '.thumbs (renamed).db'
+			],
+			'invalid character' => [
+				'a: b.txt', ['.htaccess'], [], [], [':'], 'a  b.txt',
+			],
+			'invalid extension' => [
+				'a: b.txt', ['.htaccess'], [], ['.txt'], [], 'a: b'
+			],
+			'invalid extension case insensitive' => [
+				'a: b.TXT', ['.htaccess'], [], ['.txt'], [], 'a: b'
+			],
+			'empty filename' => [
+				'', [], [], [], [], 'renamed file'
+			],
+		];
+	}
+
+	/**
+	 * @dataProvider dataSanitizeFilenameCharacterReplacement
+	 */
+	public function testSanitizeFilenameCharacterReplacement(
+		string $filename,
+		array $forbiddenCharacters,
+		?string $characterReplacement,
+		?string $expected,
+	): void {
+		/** @var FilenameValidator&MockObject */
+		$validator = $this->getMockBuilder(FilenameValidator::class)
+			->onlyMethods([
+				'getForbiddenBasenames',
+				'getForbiddenExtensions',
+				'getForbiddenFilenames',
+				'getForbiddenCharacters',
+			])
+			->setConstructorArgs([$this->l10n, $this->database, $this->config, $this->logger])
+			->getMock();
+
+		$validator->method('getForbiddenBasenames')
+			->willReturn([]);
+		$validator->method('getForbiddenCharacters')
+			->willReturn($forbiddenCharacters);
+		$validator->method('getForbiddenExtensions')
+			->willReturn([]);
+		$validator->method('getForbiddenFilenames')
+			->willReturn([]);
+
+		if ($expected === null) {
+			$this->expectException(\InvalidArgumentException::class);
+			$validator->sanitizeFilename($filename, $characterReplacement);
+		} else {
+			$this->assertEquals($expected, $validator->sanitizeFilename($filename, $characterReplacement));
+		}
+	}
+
+	public static function dataSanitizeFilenameCharacterReplacement(): array {
+		return [
+			'default' => [
+				'foo*bar', ['*'], null, 'foo bar'
+			],
+			'default - space not allowed' => [
+				'foo*bar', ['*', ' '], null, 'foo_bar'
+			],
+			'default - space and underscore not allowed' => [
+				'foo*bar', ['*', ' ', '_'], null, 'foo-bar'
+			],
+			'default - no replacement' => [
+				'foo*bar', ['*', ' ', '_', '-'], null, null
+			],
+			'custom replacement' => [
+				'foo*bar', ['*'], 'x', 'fooxbar'
+			],
+		];
+	}
 }