diff --git a/apps/dav/lib/Connector/Sabre/Node.php b/apps/dav/lib/Connector/Sabre/Node.php index 63e453c86afe7..379574b30d695 100644 --- a/apps/dav/lib/Connector/Sabre/Node.php +++ b/apps/dav/lib/Connector/Sabre/Node.php @@ -122,11 +122,11 @@ public function setName($name) { [$parentPath,] = \Sabre\Uri\split($this->path); [, $newName] = \Sabre\Uri\split($name); + $newPath = $parentPath . '/' . $newName; // verify path of the target - $this->verifyPath(); + $this->verifyPath($newPath); - $newPath = $parentPath . '/' . $newName; if (!$this->fileView->rename($this->path, $newPath)) { throw new \Sabre\DAV\Exception('Failed to rename '. $this->path . ' to ' . $newPath); @@ -355,10 +355,13 @@ public function getOwner() { return $this->info->getOwner(); } - protected function verifyPath() { + protected function verifyPath(?string $path = null): void { try { - $fileName = basename($this->info->getPath()); - $this->fileView->verifyPath($this->path, $fileName); + $path = $path ?? $this->info->getPath(); + $this->fileView->verifyPath( + dirname($path), + basename($path), + ); } catch (\OCP\Files\InvalidPathException $ex) { throw new InvalidPath($ex->getMessage()); } diff --git a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php index 3eca9b7ac1c1c..d2be66c13f52f 100644 --- a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php @@ -397,7 +397,7 @@ public function testMoveFailedInvalidChars($source, $destination, $updatables, $ public function moveFailedInvalidCharsProvider() { return [ - ['a/b', 'a/*', ['a' => true, 'a/b' => true, 'a/c*' => false], []], + ['a/valid', "a/i\nvalid", ['a' => true, 'a/valid' => true, 'a/c*' => false], []], ]; } @@ -463,7 +463,7 @@ public function testFailingMove(): void { $sourceNode = new Directory($view, $sourceInfo); $targetNode = $this->getMockBuilder(Directory::class) - ->setMethods(['childExists']) + ->onlyMethods(['childExists']) ->setConstructorArgs([$view, $targetInfo]) ->getMock(); $targetNode->expects($this->once())->method('childExists') diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index 060f0294256e4..d099a43118413 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -570,7 +570,7 @@ public function testSimplePutInvalidChars(): void { ->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 @@ public function testSetNameInvalidChars(): void { ->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/apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php b/apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php index 2f41502d4f607..e1bbbbc310f6c 100644 --- a/apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php +++ b/apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php @@ -23,6 +23,7 @@ use OCP\Federation\ICloudFederationProviderManager; use OCP\Federation\ICloudFederationShare; use OCP\Federation\ICloudIdManager; +use OCP\Files\IFilenameValidator; use OCP\Files\NotFoundException; use OCP\HintException; use OCP\IConfig; @@ -59,6 +60,7 @@ public function __construct( private IConfig $config, private Manager $externalShareManager, private LoggerInterface $logger, + private IFilenameValidator $filenameValidator, ) { } @@ -115,7 +117,7 @@ public function shareReceived(ICloudFederationShare $share) { } if ($remote && $token && $name && $owner && $remoteId && $shareWith) { - if (!Util::isValidFileName($name)) { + if (!$this->filenameValidator->isFilenameValid($name)) { throw new ProviderCouldNotAddShareException('The mountpoint name contains invalid characters.', '', Http::STATUS_BAD_REQUEST); } diff --git a/apps/files/lib/Controller/ViewController.php b/apps/files/lib/Controller/ViewController.php index 0f24db6b0776c..3be7e61d01081 100644 --- a/apps/files/lib/Controller/ViewController.php +++ b/apps/files/lib/Controller/ViewController.php @@ -7,6 +7,7 @@ */ namespace OCA\Files\Controller; +use OC\Files\FilenameValidator; use OCA\Files\Activity\Helper; use OCA\Files\AppInfo\Application; use OCA\Files\Event\LoadAdditionalScriptsEvent; @@ -34,57 +35,31 @@ use OCP\IRequest; use OCP\IURLGenerator; use OCP\IUserSession; -use OCP\Share\IManager; /** * @package OCA\Files\Controller */ #[OpenAPI(scope: OpenAPI::SCOPE_IGNORE)] class ViewController extends Controller { - private IURLGenerator $urlGenerator; - private IL10N $l10n; - private IConfig $config; - private IEventDispatcher $eventDispatcher; - private IUserSession $userSession; - private IAppManager $appManager; - private IRootFolder $rootFolder; - private Helper $activityHelper; - private IInitialState $initialState; - private ITemplateManager $templateManager; - private IManager $shareManager; - private UserConfig $userConfig; - private ViewConfig $viewConfig; - - public function __construct(string $appName, + + public function __construct( + string $appName, IRequest $request, - IURLGenerator $urlGenerator, - IL10N $l10n, - IConfig $config, - IEventDispatcher $eventDispatcher, - IUserSession $userSession, - IAppManager $appManager, - IRootFolder $rootFolder, - Helper $activityHelper, - IInitialState $initialState, - ITemplateManager $templateManager, - IManager $shareManager, - UserConfig $userConfig, - ViewConfig $viewConfig + private IURLGenerator $urlGenerator, + private IL10N $l10n, + private IConfig $config, + private IEventDispatcher $eventDispatcher, + private IUserSession $userSession, + private IAppManager $appManager, + private IRootFolder $rootFolder, + private Helper $activityHelper, + private IInitialState $initialState, + private ITemplateManager $templateManager, + private UserConfig $userConfig, + private ViewConfig $viewConfig, + private FilenameValidator $filenameValidator, ) { parent::__construct($appName, $request); - $this->urlGenerator = $urlGenerator; - $this->l10n = $l10n; - $this->config = $config; - $this->eventDispatcher = $eventDispatcher; - $this->userSession = $userSession; - $this->appManager = $appManager; - $this->rootFolder = $rootFolder; - $this->activityHelper = $activityHelper; - $this->initialState = $initialState; - $this->templateManager = $templateManager; - $this->shareManager = $shareManager; - $this->userConfig = $userConfig; - $this->viewConfig = $viewConfig; } /** @@ -220,8 +195,9 @@ public function index($dir = '', $view = '', $fileid = null, $fileNotFound = fal $filesSortingConfig = json_decode($this->config->getUserValue($userId, 'files', 'files_sorting_configs', '{}'), true); $this->initialState->provideInitialState('filesSortingConfig', $filesSortingConfig); - // Forbidden file characters - $forbiddenCharacters = \OCP\Util::getForbiddenFileNameChars(); + // Forbidden file characters (deprecated use capabilities) + // TODO: Remove with next release of `@nextcloud/files` + $forbiddenCharacters = $this->filenameValidator->getForbiddenCharacters(); $this->initialState->provideInitialState('forbiddenCharacters', $forbiddenCharacters); $event = new LoadAdditionalScriptsEvent(); diff --git a/apps/files/tests/Controller/ViewControllerTest.php b/apps/files/tests/Controller/ViewControllerTest.php index 98147228cf12e..aade58da5a395 100644 --- a/apps/files/tests/Controller/ViewControllerTest.php +++ b/apps/files/tests/Controller/ViewControllerTest.php @@ -7,6 +7,7 @@ */ namespace OCA\Files\Tests\Controller; +use OC\Files\FilenameValidator; use OCA\Files\Activity\Helper; use OCA\Files\Controller\ViewController; use OCA\Files\Service\UserConfig; @@ -87,10 +88,12 @@ protected function setUp(): void { $this->activityHelper = $this->createMock(Helper::class); $this->initialState = $this->createMock(IInitialState::class); $this->templateManager = $this->createMock(ITemplateManager::class); - $this->shareManager = $this->createMock(IManager::class); $this->userConfig = $this->createMock(UserConfig::class); $this->viewConfig = $this->createMock(ViewConfig::class); - $this->viewController = $this->getMockBuilder('\OCA\Files\Controller\ViewController') + + $filenameValidator = $this->createMock(FilenameValidator::class); + + $this->viewController = $this->getMockBuilder(ViewController::class) ->setConstructorArgs([ 'files', $this->request, @@ -104,11 +107,11 @@ protected function setUp(): void { $this->activityHelper, $this->initialState, $this->templateManager, - $this->shareManager, $this->userConfig, $this->viewConfig, + $filenameValidator, ]) - ->setMethods([ + ->onlyMethods([ 'getStorageInfo', ]) ->getMock(); diff --git a/lib/private/Files/FilenameValidator.php b/lib/private/Files/FilenameValidator.php index 0026dfdf451ca..b1ce8e02b13ee 100644 --- a/lib/private/Files/FilenameValidator.php +++ b/lib/private/Files/FilenameValidator.php @@ -11,9 +11,11 @@ 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 @@ public function validateFilename(string $filename): void { } // 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 @@ public function validateFilename(string $filename): void { 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 @@ protected function checkForbiddenExtension(string $filename): void { * @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 e613e435f03f6..a8f8c05ab5483 100644 --- a/lib/private/Files/Storage/Common.php +++ b/lib/private/Files/Storage/Common.php @@ -16,14 +16,10 @@ 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 @@ public function getDirectDownload($path) { * @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 0150a3e117ab5..26d419842d66b 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -716,6 +716,12 @@ public function rename($source, $target) { 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 @@ public function rename($source, $target) { } } if ($run) { - $this->verifyPath(dirname($target), basename($target)); - $manager = Filesystem::getMountManager(); $mount1 = $this->getMount($source); $mount2 = $this->getMount($target); diff --git a/lib/private/legacy/OC_Util.php b/lib/private/legacy/OC_Util.php index b7836e86d7b7b..d8045e8343d74 100644 --- a/lib/private/legacy/OC_Util.php +++ b/lib/private/legacy/OC_Util.php @@ -1036,35 +1036,6 @@ public static function getHumanVersion() { return $version; } - /** - * Returns whether the given file name is valid - * - * @param string $file file name to check - * @return bool true if the file name is valid, false otherwise - * @deprecated use \OC\Files\View::verifyPath() - */ - public static function isValidFileName($file) { - $trimmed = trim($file); - if ($trimmed === '') { - return false; - } - if (\OC\Files\Filesystem::isIgnoredDir($trimmed)) { - return false; - } - - // detect part files - if (preg_match('/' . \OCP\Files\FileInfo::BLACKLIST_FILES_REGEX . '/', $trimmed) !== 0) { - return false; - } - - foreach (\OCP\Util::getForbiddenFileNameChars() as $char) { - if (str_contains($trimmed, $char)) { - return false; - } - } - return true; - } - /** * Check whether the instance needs to perform an upgrade, * either when the core version is higher or any app requires diff --git a/lib/public/Util.php b/lib/public/Util.php index 4cee9addf100a..17794810c1a8f 100644 --- a/lib/public/Util.php +++ b/lib/public/Util.php @@ -18,7 +18,6 @@ use OCP\Mail\IMailer; use OCP\Share\IManager; use Psr\Container\ContainerExceptionInterface; -use Psr\Log\LoggerInterface; /** * This class provides different helper functions to make the life of a developer easier @@ -488,39 +487,6 @@ public static function uploadLimit(): int|float { return \OC_Helper::uploadLimit(); } - /** - * Get a list of characters forbidden in file names - * @return string[] - * @since 29.0.0 - */ - public static function getForbiddenFileNameChars(): array { - // Get always forbidden characters - $invalidChars = str_split(\OCP\Constants::FILENAME_INVALID_CHARS); - if ($invalidChars === false) { - $invalidChars = []; - } - - // Get admin defined invalid characters - $additionalChars = \OCP\Server::get(IConfig::class)->getSystemValue('forbidden_chars', []); - if (!is_array($additionalChars)) { - \OCP\Server::get(LoggerInterface::class)->error('Invalid system config value for "forbidden_chars" is ignored.'); - $additionalChars = []; - } - return array_merge($invalidChars, $additionalChars); - } - - /** - * Returns whether the given file name is valid - * @param string $file file name to check - * @return bool true if the file name is valid, false otherwise - * @deprecated 8.1.0 use OCP\Files\Storage\IStorage::verifyPath() - * @since 7.0.0 - * @suppress PhanDeprecatedFunction - */ - public static function isValidFileName($file) { - return \OC_Util::isValidFileName($file); - } - /** * Compare two strings to provide a natural sort * @param string $a first string to compare diff --git a/tests/lib/Files/FilenameValidatorTest.php b/tests/lib/Files/FilenameValidatorTest.php index 42705a23f0255..1fcc03064a2a2 100644 --- a/tests/lib/Files/FilenameValidatorTest.php +++ b/tests/lib/Files/FilenameValidatorTest.php @@ -13,9 +13,11 @@ 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 @@ protected function setUp(): void { $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 @@ public function testValidateFilename( '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 @@ public function testIsFilenameValid( '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 @@ public function dataValidateFilename(): array { '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 @@ public function dataValidateFilename(): array { '', [], [], [], [], 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 @@ public function dataValidateFilename(): array { ]; } + /** + * @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 @@ public function testIsForbidden(string $filename, array $forbiddenNames, array $ /** @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 4addd5833549e..0fd0c330ea5bb 100644 --- a/tests/lib/Files/PathVerificationTest.php +++ b/tests/lib/Files/PathVerificationTest.php @@ -106,57 +106,6 @@ public function providesAstralPlane() { ]; } - /** - * @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 bd22d93fb69d9..321894cc23b1e 100644 --- a/tests/lib/Files/Storage/CommonTest.php +++ b/tests/lib/Files/Storage/CommonTest.php @@ -9,7 +9,10 @@ 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 @@ * @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() { diff --git a/tests/lib/UtilTest.php b/tests/lib/UtilTest.php index 82897cbca298d..a83f27d5cf7e2 100644 --- a/tests/lib/UtilTest.php +++ b/tests/lib/UtilTest.php @@ -137,64 +137,6 @@ public function testGetInstanceIdGeneratesValidId() { $this->assertSame(1, $matchesRegex); } - /** - * @dataProvider filenameValidationProvider - */ - public function testFilenameValidation($file, $valid) { - // private API - $this->assertEquals($valid, \OC_Util::isValidFileName($file)); - // public API - $this->assertEquals($valid, \OCP\Util::isValidFileName($file)); - } - - public function filenameValidationProvider() { - return [ - // valid names - ['boringname', true], - ['something.with.extension', true], - ['now with spaces', true], - ['.a', true], - ['..a', true], - ['.dotfile', true], - ['single\'quote', true], - [' spaces before', true], - ['spaces after ', true], - ['allowed chars including the crazy ones $%&_-^@!,()[]{}=;#', true], - ['汉字也能用', true], - ['und Ümläüte sind auch willkommen', true], - // disallowed names - ['', false], - [' ', false], - ['.', false], - ['..', false], - ['back\\slash', false], - ['sl/ash', false], - ['ltgt', true], - ['col:on', true], - ['double"quote', true], - ['pi|pe', true], - ['dont?ask?questions?', true], - ['super*star', true], - ['new\nline', false], - - // better disallow these to avoid unexpected trimming to have side effects - [' ..', false], - ['.. ', false], - ['. ', false], - [' .', false], - - // part files not allowed - ['.part', false], - ['notallowed.part', false], - ['neither.filepart', false], - - // part in the middle is ok - ['super movie part one.mkv', true], - ['super.movie.part.mkv', true], - ]; - } - /** * Test needUpgrade() when the core version is increased */