From 69341e4306259430533cf66fd0a76688c7c8e6ab Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Mon, 15 Jul 2024 16:28:43 +0200 Subject: [PATCH] refactor: Migrate filename validation logic from `Storage` to `FilenameValidator` Signed-off-by: Ferdinand Thiessen --- .../tests/unit/Connector/Sabre/FileTest.php | 7 +- lib/private/Files/FilenameValidator.php | 19 +++- lib/private/Files/Storage/Common.php | 76 +++------------- lib/private/Files/View.php | 8 +- tests/lib/Files/FilenameValidatorTest.php | 88 +++++++++++++++++-- tests/lib/Files/PathVerificationTest.php | 51 ----------- tests/lib/Files/Storage/CommonTest.php | 67 +++++--------- 7 files changed, 146 insertions(+), 170 deletions(-) diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index 060f0294256..d099a431184 100644 --- 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"); } diff --git a/lib/private/Files/FilenameValidator.php b/lib/private/Files/FilenameValidator.php index 0026dfdf451..b1ce8e02b13 100644 --- 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; diff --git a/lib/private/Files/Storage/Common.php b/lib/private/Files/Storage/Common.php index e613e435f03..a8f8c05ab54 100644 --- 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; } /** diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 0150a3e117a..26d419842d6 100644 --- 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); diff --git a/tests/lib/Files/FilenameValidatorTest.php b/tests/lib/Files/FilenameValidatorTest.php index 42705a23f02..223e86ccd0a 100644 --- 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') diff --git a/tests/lib/Files/PathVerificationTest.php b/tests/lib/Files/PathVerificationTest.php index 4addd583354..0fd0c330ea5 100644 --- 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 */ diff --git a/tests/lib/Files/Storage/CommonTest.php b/tests/lib/Files/Storage/CommonTest.php index bd22d93fb69..321894cc23b 100644 --- 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() {