refactor: Migrate filename validation logic from Storage to FilenameValidator

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
2026-05-28 04:32:30 -04:00 · 2024-07-15 16:28:43 +02:00 · 2024-07-15 16:28:43 +02:00 · 69341e4306
commit 69341e4306
parent 465cee2da3
7 changed files with 146 additions and 170 deletions
--- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php
+++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php
@ -570,7 +570,7 @@ class FileTest extends TestCase {
 			->method('getRelativePath')
 			->willReturnArgument(0);

-		$info = new \OC\Files\FileInfo('/*', $this->getMockStorage(), null, [
+		$info = new \OC\Files\FileInfo("/i\nvalid", $this->getMockStorage(), null, [
 			'permissions' => \OCP\Constants::PERMISSION_ALL,
 			'type' => FileInfo::TYPE_FOLDER,
 		], null);
@ -611,12 +611,13 @@ class FileTest extends TestCase {
 			->method('getRelativePath')
 			->willReturnArgument(0);

-		$info = new \OC\Files\FileInfo('/*', $this->getMockStorage(), null, [
+		$info = new \OC\Files\FileInfo('/valid', $this->getMockStorage(), null, [
 			'permissions' => \OCP\Constants::PERMISSION_ALL,
 			'type' => FileInfo::TYPE_FOLDER,
 		], null);
 		$file = new \OCA\DAV\Connector\Sabre\File($view, $info);
-		$file->setName('/super*star.txt');
+
+		$file->setName("/i\nvalid");
 	}


--- a/lib/private/Files/FilenameValidator.php
+++ b/lib/private/Files/FilenameValidator.php
@ -11,9 +11,11 @@ use OCP\Files\EmptyFileNameException;
 use OCP\Files\FileNameTooLongException;
 use OCP\Files\IFilenameValidator;
 use OCP\Files\InvalidCharacterInPathException;
+use OCP\Files\InvalidDirectoryException;
 use OCP\Files\InvalidPathException;
 use OCP\Files\ReservedWordException;
 use OCP\IConfig;
+use OCP\IDBConnection;
 use OCP\IL10N;
 use OCP\L10N\IFactory;
 use Psr\Log\LoggerInterface;
@ -46,6 +48,7 @@ class FilenameValidator implements IFilenameValidator {

 	public function __construct(
 		IFactory $l10nFactory,
+		private IDBConnection $database,
 		private IConfig $config,
 		private LoggerInterface $logger,
 	) {
@ -173,8 +176,9 @@ class FilenameValidator implements IFilenameValidator {
 		}

 		// the special directories . and .. would cause never ending recursion
+		// we check the trimmed name here to ensure unexpected trimming will not cause severe issues
 		if ($trimmed === '.' || $trimmed === '..') {
-			throw new ReservedWordException();
+			throw new InvalidDirectoryException($this->l10n->t('Dot files are not allowed'));
 		}

 		// 255 characters is the limit on common file systems (ext/xfs)
@ -183,6 +187,17 @@ class FilenameValidator implements IFilenameValidator {
 			throw new FileNameTooLongException();
 		}

+		if (!$this->database->supports4ByteText()) {
+			// verify database - e.g. mysql only 3-byte chars
+			if (preg_match('%(?:
+      \xF0[\x90-\xBF][\x80-\xBF]{2}      # planes 1-3
+    | [\xF1-\xF3][\x80-\xBF]{3}          # planes 4-15
+    | \xF4[\x80-\x8F][\x80-\xBF]{2}      # plane 16
+)%xs', $filename)) {
+				throw new InvalidCharacterInPathException();
+			}
+		}
+
 		if ($this->isForbidden($filename)) {
 			throw new ReservedWordException();
 		}
@ -263,7 +278,7 @@ class FilenameValidator implements IFilenameValidator {
 	 * @return string[]
 	 */
 	private function getConfigValue(string $key, array $fallback): array {
-		$values = $this->config->getSystemValue($key, ['.filepart']);
+		$values = $this->config->getSystemValue($key, $fallback);
 		if (!is_array($values)) {
 			$this->logger->error('Invalid system config value for "' . $key . '" is ignored.');
 			$values = $fallback;
--- a/lib/private/Files/Storage/Common.php
+++ b/lib/private/Files/Storage/Common.php
@ -16,14 +16,10 @@ use OC\Files\Cache\Watcher;
 use OC\Files\Filesystem;
 use OC\Files\Storage\Wrapper\Jail;
 use OC\Files\Storage\Wrapper\Wrapper;
-use OCP\Files\EmptyFileNameException;
-use OCP\Files\FileNameTooLongException;
 use OCP\Files\ForbiddenException;
 use OCP\Files\GenericFileException;
-use OCP\Files\InvalidCharacterInPathException;
-use OCP\Files\InvalidDirectoryException;
+use OCP\Files\IFilenameValidator;
 use OCP\Files\InvalidPathException;
-use OCP\Files\ReservedWordException;
 use OCP\Files\Storage\ILockingStorage;
 use OCP\Files\Storage\IStorage;
 use OCP\Files\Storage\IWriteStreamStorage;
@ -57,10 +53,9 @@ abstract class Common implements Storage, ILockingStorage, IWriteStreamStorage {
 	protected $mountOptions = [];
 	protected $owner = null;

-	/** @var ?bool */
-	private $shouldLogLocks = null;
-	/** @var ?LoggerInterface */
-	private $logger;
+	private ?bool $shouldLogLocks = null;
+	private ?LoggerInterface $logger = null;
+	private ?IFilenameValidator $filenameValidator = null;

 	public function __construct($parameters) {
 	}
@ -496,68 +491,21 @@ abstract class Common implements Storage, ILockingStorage, IWriteStreamStorage {
 	 * @throws InvalidPathException
 	 */
 	public function verifyPath($path, $fileName) {
-		// verify empty and dot files
-		$trimmed = trim($fileName);
-		if ($trimmed === '') {
-			throw new EmptyFileNameException();
-		}
-
-		if (\OC\Files\Filesystem::isIgnoredDir($trimmed)) {
-			throw new InvalidDirectoryException();
-		}
-
-		if (!\OC::$server->getDatabaseConnection()->supports4ByteText()) {
-			// verify database - e.g. mysql only 3-byte chars
-			if (preg_match('%(?:
-      \xF0[\x90-\xBF][\x80-\xBF]{2}      # planes 1-3
-    | [\xF1-\xF3][\x80-\xBF]{3}          # planes 4-15
-    | \xF4[\x80-\x8F][\x80-\xBF]{2}      # plane 16
-)%xs', $fileName)) {
-				throw new InvalidCharacterInPathException();
-			}
-		}
-
-		// 255 characters is the limit on common file systems (ext/xfs)
-		// oc_filecache has a 250 char length limit for the filename
-		if (isset($fileName[250])) {
-			throw new FileNameTooLongException();
-		}
+		$this->getFilenameValidator()
+			->validateFilename($fileName);

 		// NOTE: $path will remain unverified for now
-		$this->verifyPosixPath($fileName);
 	}

 	/**
-	 * @param string $fileName
-	 * @throws InvalidPathException
+	 * Get the filename validator
+	 * (cached for performance)
 	 */
-	protected function verifyPosixPath($fileName) {
-		$invalidChars = \OCP\Util::getForbiddenFileNameChars();
-		$this->scanForInvalidCharacters($fileName, $invalidChars);
-
-		$fileName = trim($fileName);
-		$reservedNames = ['*'];
-		if (in_array($fileName, $reservedNames)) {
-			throw new ReservedWordException();
-		}
-	}
-
-	/**
-	 * @param string $fileName
-	 * @param string[] $invalidChars
-	 * @throws InvalidPathException
-	 */
-	private function scanForInvalidCharacters(string $fileName, array $invalidChars) {
-		foreach ($invalidChars as $char) {
-			if (str_contains($fileName, $char)) {
-				throw new InvalidCharacterInPathException();
-			}
-		}
-
-		$sanitizedFileName = filter_var($fileName, FILTER_UNSAFE_RAW, FILTER_FLAG_STRIP_LOW);
-		if ($sanitizedFileName !== $fileName) {
-			throw new InvalidCharacterInPathException();
+	protected function getFilenameValidator(): IFilenameValidator {
+		if ($this->filenameValidator === null) {
+			$this->filenameValidator = \OCP\Server::get(IFilenameValidator::class);
 		}
+		return $this->filenameValidator;
 	}

 	/**
--- a/lib/private/Files/View.php
+++ b/lib/private/Files/View.php
@ -716,6 +716,12 @@ class View {
 				return false;
 			}

+			try {
+				$this->verifyPath(dirname($target), basename($target));
+			} catch (InvalidPathException) {
+				return false;
+			}
+
 			$this->lockFile($source, ILockingProvider::LOCK_SHARED, true);
 			try {
 				$this->lockFile($target, ILockingProvider::LOCK_SHARED, true);
@ -739,8 +745,6 @@ class View {
 					}
 				}
 				if ($run) {
-					$this->verifyPath(dirname($target), basename($target));
-
 					$manager = Filesystem::getMountManager();
 					$mount1 = $this->getMount($source);
 					$mount2 = $this->getMount($target);
--- a/tests/lib/Files/FilenameValidatorTest.php
+++ b/tests/lib/Files/FilenameValidatorTest.php
@ -13,9 +13,11 @@ use OC\Files\FilenameValidator;
 use OCP\Files\EmptyFileNameException;
 use OCP\Files\FileNameTooLongException;
 use OCP\Files\InvalidCharacterInPathException;
+use OCP\Files\InvalidDirectoryException;
 use OCP\Files\InvalidPathException;
 use OCP\Files\ReservedWordException;
 use OCP\IConfig;
+use OCP\IDBConnection;
 use OCP\IL10N;
 use OCP\L10N\IFactory;
 use PHPUnit\Framework\MockObject\MockObject;
@ -26,6 +28,7 @@ class FilenameValidatorTest extends TestCase {

 	protected IFactory&MockObject $l10n;
 	protected IConfig&MockObject $config;
+	protected IDBConnection&MockObject $database;
 	protected LoggerInterface&MockObject $logger;

 	protected function setUp(): void {
@ -41,6 +44,8 @@ class FilenameValidatorTest extends TestCase {

 		$this->config = $this->createMock(IConfig::class);
 		$this->logger = $this->createMock(LoggerInterface::class);
+		$this->database = $this->createMock(IDBConnection::class);
+		$this->database->method('supports4ByteText')->willReturn(true);
 	}

 	/**
@ -62,7 +67,7 @@ class FilenameValidatorTest extends TestCase {
 				'getForbiddenExtensions',
 				'getForbiddenFilenames',
 			])
-			->setConstructorArgs([$this->l10n, $this->config, $this->logger])
+			->setConstructorArgs([$this->l10n, $this->database, $this->config, $this->logger])
 			->getMock();

 		$validator->method('getForbiddenBasenames')
@ -101,7 +106,7 @@ class FilenameValidatorTest extends TestCase {
 				'getForbiddenFilenames',
 				'getForbiddenCharacters',
 			])
-			->setConstructorArgs([$this->l10n, $this->config, $this->logger])
+			->setConstructorArgs([$this->l10n, $this->database, $this->config, $this->logger])
 			->getMock();

 		$validator->method('getForbiddenBasenames')
@ -122,6 +127,9 @@ class FilenameValidatorTest extends TestCase {
 			'valid name' => [
 				'a: b.txt', ['.htaccess'], [], [], [], null
 			],
+			'forbidden name in the middle is ok' => [
+				'a.htaccess.txt', ['.htaccess'], [], [], null
+			],
 			'valid name with some more parameters' => [
 				'a: b.txt', ['.htaccess'], [], ['exe'], ['~'], null
 			],
@ -155,10 +163,13 @@ class FilenameValidatorTest extends TestCase {
 				'', [], [], [], [], EmptyFileNameException::class
 			],
 			'reserved unix name "."' => [
-				'.', [], [], [], [], InvalidPathException::class
+				'.', [], [], [], [], InvalidDirectoryException::class
 			],
 			'reserved unix name ".."' => [
-				'..', [], [], [], [], ReservedWordException::class
+				'..', [], [], [], [], InvalidDirectoryException::class
+			],
+			'weird but valid tripple dot name' => [
+				'...', [], [], [], null // is valid
 			],
 			'too long filename "."' => [
 				str_repeat('a', 251), [], [], [], [], FileNameTooLongException::class
@ -171,6 +182,73 @@ class FilenameValidatorTest extends TestCase {
 		];
 	}

+	/**
+	 * @dataProvider data4ByteUnicode
+	 */
+	public function testDatabaseDoesNotSupport4ByteText($filename): void {
+		$database = $this->createMock(IDBConnection::class);
+		$database->expects($this->once())
+			->method('supports4ByteText')
+			->willReturn(false);
+		$this->expectException(InvalidCharacterInPathException::class);
+		$validator = new FilenameValidator($this->l10n, $database, $this->config, $this->logger);
+		$validator->validateFilename($filename);
+	}
+
+	public function data4ByteUnicode(): array {
+		return [
+			['plane 1 𐪅'],
+			['emoji 😶‍🌫️'],
+
+		];
+	}
+
+	/**
+	 * @dataProvider dataInvalidAsciiCharacters
+	 */
+	public function testInvalidAsciiCharactersAreAlwaysForbidden(string $filename): void {
+		$this->expectException(InvalidPathException::class);
+		$validator = new FilenameValidator($this->l10n, $this->database, $this->config, $this->logger);
+		$validator->validateFilename($filename);
+	}
+
+	public function dataInvalidAsciiCharacters(): array {
+		return [
+			[\chr(0)],
+			[\chr(1)],
+			[\chr(2)],
+			[\chr(3)],
+			[\chr(4)],
+			[\chr(5)],
+			[\chr(6)],
+			[\chr(7)],
+			[\chr(8)],
+			[\chr(9)],
+			[\chr(10)],
+			[\chr(11)],
+			[\chr(12)],
+			[\chr(13)],
+			[\chr(14)],
+			[\chr(15)],
+			[\chr(16)],
+			[\chr(17)],
+			[\chr(18)],
+			[\chr(19)],
+			[\chr(20)],
+			[\chr(21)],
+			[\chr(22)],
+			[\chr(23)],
+			[\chr(24)],
+			[\chr(25)],
+			[\chr(26)],
+			[\chr(27)],
+			[\chr(28)],
+			[\chr(29)],
+			[\chr(30)],
+			[\chr(31)],
+		];
+	}
+
 	/**
 	 * @dataProvider dataIsForbidden
 	 */
@ -178,7 +256,7 @@ class FilenameValidatorTest extends TestCase {
 		/** @var FilenameValidator&MockObject */
 		$validator = $this->getMockBuilder(FilenameValidator::class)
 			->onlyMethods(['getForbiddenFilenames', 'getForbiddenBasenames'])
-			->setConstructorArgs([$this->l10n, $this->config, $this->logger])
+			->setConstructorArgs([$this->l10n, $this->database, $this->config, $this->logger])
 			->getMock();

 		$validator->method('getForbiddenBasenames')
--- a/tests/lib/Files/PathVerificationTest.php
+++ b/tests/lib/Files/PathVerificationTest.php
@ -106,57 +106,6 @@ class PathVerificationTest extends \Test\TestCase {
 		];
 	}

-	/**
-	 * @dataProvider providesInvalidCharsPosix
-	 */
-	public function testPathVerificationInvalidCharsPosix($fileName) {
-		$this->expectException(\OCP\Files\InvalidCharacterInPathException::class);
-
-		$storage = new Local(['datadir' => '']);
-
-		$fileName = " 123{$fileName}456 ";
-		self::invokePrivate($storage, 'verifyPosixPath', [$fileName]);
-	}
-
-	public function providesInvalidCharsPosix() {
-		return [
-			[\chr(0)],
-			[\chr(1)],
-			[\chr(2)],
-			[\chr(3)],
-			[\chr(4)],
-			[\chr(5)],
-			[\chr(6)],
-			[\chr(7)],
-			[\chr(8)],
-			[\chr(9)],
-			[\chr(10)],
-			[\chr(11)],
-			[\chr(12)],
-			[\chr(13)],
-			[\chr(14)],
-			[\chr(15)],
-			[\chr(16)],
-			[\chr(17)],
-			[\chr(18)],
-			[\chr(19)],
-			[\chr(20)],
-			[\chr(21)],
-			[\chr(22)],
-			[\chr(23)],
-			[\chr(24)],
-			[\chr(25)],
-			[\chr(26)],
-			[\chr(27)],
-			[\chr(28)],
-			[\chr(29)],
-			[\chr(30)],
-			[\chr(31)],
-			['/'],
-			['\\'],
-		];
-	}
-
 	/**
 	 * @dataProvider providesValidPosixPaths
 	 */
--- a/tests/lib/Files/Storage/CommonTest.php
+++ b/tests/lib/Files/Storage/CommonTest.php
@ -9,7 +9,10 @@ namespace Test\Files\Storage;

 use OC\Files\Storage\Wrapper\Jail;
 use OC\Files\Storage\Wrapper\Wrapper;
+use OCP\Files\IFilenameValidator;
+use OCP\Files\InvalidCharacterInPathException;
 use OCP\Files\InvalidPathException;
+use OCP\ITempManager;
 use PHPUnit\Framework\MockObject\MockObject;

 /**
@ -20,65 +23,43 @@ use PHPUnit\Framework\MockObject\MockObject;
 * @package Test\Files\Storage
 */
 class CommonTest extends Storage {
-	/**
-	 * @var string tmpDir
-	 */
-	private $tmpDir;

-	private array $invalidCharsBackup;
+	private string $tmpDir;
+	private IFilenameValidator&MockObject $filenameValidator;

 	protected function setUp(): void {
 		parent::setUp();

-		$this->tmpDir = \OC::$server->getTempManager()->getTemporaryFolder();
+		$this->filenameValidator = $this->createMock(IFilenameValidator::class);
+		$this->overwriteService(IFilenameValidator::class, $this->filenameValidator);
+		$this->tmpDir = \OCP\Server::get(ITempManager::class)->getTemporaryFolder();
 		$this->instance = new \OC\Files\Storage\CommonTest(['datadir' => $this->tmpDir]);
-		$this->invalidCharsBackup = \OC::$server->getConfig()->getSystemValue('forbidden_chars', []);
 	}

 	protected function tearDown(): void {
 		\OC_Helper::rmdirr($this->tmpDir);
-		\OC::$server->getConfig()->setSystemValue('forbidden_chars', $this->invalidCharsBackup);
+		$this->restoreService(IFilenameValidator::class);
 		parent::tearDown();
 	}

-	/**
-	 * @dataProvider dataVerifyPath
-	 */
-	public function testVerifyPath(string $filename, array $additionalChars, bool $throws) {
-		/** @var \OC\Files\Storage\CommonTest|MockObject $instance */
-		$instance = $this->getMockBuilder(\OC\Files\Storage\CommonTest::class)
-			->onlyMethods(['copyFromStorage', 'rmdir', 'unlink'])
-			->setConstructorArgs([['datadir' => $this->tmpDir]])
-			->getMock();
-		$instance->method('copyFromStorage')
-			->willThrowException(new \Exception('copy'));
+	public function testVerifyPath() {
+		$this->filenameValidator
+			->expects($this->once())
+			->method('validateFilename')
+			->with('invalid:char.txt')
+			->willThrowException(new InvalidCharacterInPathException());
+		$this->expectException(InvalidPathException::class);

-		\OC::$server->getConfig()->setSystemValue('forbidden_chars', $additionalChars);
-
-		if ($throws) {
-			$this->expectException(InvalidPathException::class);
-		} else {
-			$this->expectNotToPerformAssertions();
-		}
-		$instance->verifyPath('/', $filename);
+		$this->instance->verifyPath('/', 'invalid:char.txt');
 	}

-	public function dataVerifyPath(): array {
-		return [
-			// slash is always forbidden
-			'invalid slash' => ['a/b.txt', [], true],
-			// backslash is also forbidden
-			'invalid backslash' => ['a\\b.txt', [], true],
-			// by default colon is not forbidden
-			'valid name' => ['a: b.txt', [], false],
-			// colon can be added to the list of forbidden character
-			'invalid custom character' => ['a: b.txt', [':'], true],
-			// 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
-			'valid unicode sequence' => ['🌫️.txt', ['😶‍🌫️'], false],
-			// This is the reverse: clouds are forbidden -> so is also the face in the clouds emoji
-			'valid unicode sequence' => ['😶‍🌫️.txt', ['🌫️'], true],
-		];
+	public function testVerifyPathSucceed() {
+		$this->filenameValidator
+			->expects($this->once())
+			->method('validateFilename')
+			->with('valid-char.txt');
+
+		$this->instance->verifyPath('/', 'valid-char.txt');
 	}

 	public function testMoveFromStorageWrapped() {