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

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

6 files changed, 44 insertions, 59 deletions

diff --git a/apps/dav/lib/Connector/Sabre/Node.php b/apps/dav/lib/Connector/Sabre/Node.php
index 63e453c86af..379574b30d6 100644
--- a/apps/dav/lib/Connector/Sabre/Node.php
+++ b/apps/dav/lib/Connector/Sabre/Node.php
@@ -122,11 +122,11 @@ abstract class Node implements \Sabre\DAV\INode {
 
 		[$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 @@ abstract class Node implements \Sabre\DAV\INode {
 		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 3eca9b7ac1c..d2be66c13f5 100644
--- a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php
+++ b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php
@@ -397,7 +397,7 @@ class DirectoryTest extends \Test\TestCase {
 
 	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 @@ class DirectoryTest extends \Test\TestCase {
 
 		$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 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/apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php b/apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php
index 2f41502d4f6..e1bbbbc310f 100644
--- a/apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php
+++ b/apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php
@@ -23,6 +23,7 @@ use OCP\Federation\ICloudFederationProvider;
 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 @@ class CloudFederationProviderFiles implements ICloudFederationProvider {
 		private IConfig $config,
 		private Manager $externalShareManager,
 		private LoggerInterface $logger,
+		private IFilenameValidator $filenameValidator,
 	) {
 	}
 
@@ -115,7 +117,7 @@ class CloudFederationProviderFiles implements ICloudFederationProvider {
 		}
 
 		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 0f24db6b077..3be7e61d010 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\IL10N;
 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 @@ class ViewController extends Controller {
 		$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 98147228cf1..aade58da5a3 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 @@ class ViewControllerTest extends TestCase {
 		$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 @@ class ViewControllerTest extends TestCase {
 				$this->activityHelper,
 				$this->initialState,
 				$this->templateManager,
-				$this->shareManager,
 				$this->userConfig,
 				$this->viewConfig,
+				$filenameValidator,
 			])
-		->setMethods([
+		->onlyMethods([
 			'getStorageInfo',
 		])
 		->getMock();