Merge pull request #46538 from nextcloud/fix/use-filename-validator

refactor: Migrate filename validation from `Storage` and `Util` to `FilenameValidator`

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());
 		}

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')

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");
 	}
 
 

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);
 			}
 

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();

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();

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;

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;
 	}
 
 	/**