Merge pull request #43775 from nextcloud/enforce/forbidden_chars

7 files changed, 92 insertions, 13 deletions

diff --git a/apps/files/lib/Capabilities.php b/apps/files/lib/Capabilities.php
index dc2aae6acfc..0fc95d67226 100644
--- a/apps/files/lib/Capabilities.php
+++ b/apps/files/lib/Capabilities.php
@@ -39,13 +39,14 @@ class Capabilities implements ICapability {
 	/**
 	 * Return this classes capabilities
 	 *
-	 * @return array{files: array{bigfilechunking: bool, blacklisted_files: array<mixed>}}
+	 * @return array{files: array{bigfilechunking: bool, blacklisted_files: array<mixed>, forbidden_filename_characters: array<string>}}
 	 */
 	public function getCapabilities() {
 		return [
 			'files' => [
 				'bigfilechunking' => true,
-				'blacklisted_files' => (array)$this->config->getSystemValue('blacklisted_files', ['.htaccess'])
+				'blacklisted_files' => (array)$this->config->getSystemValue('blacklisted_files', ['.htaccess']),
+				'forbidden_filename_characters' => \OCP\Util::getForbiddenFileNameChars(),
 			],
 		];
 	}
diff --git a/apps/files/openapi.json b/apps/files/openapi.json
index f9432f6c57c..f9cdb67783d 100644
--- a/apps/files/openapi.json
+++ b/apps/files/openapi.json
@@ -31,6 +31,7 @@
                         "required": [
                             "bigfilechunking",
                             "blacklisted_files",
+                            "forbidden_filename_characters",
                             "directEditing"
                         ],
                         "properties": {
@@ -43,6 +44,12 @@
                                     "type": "object"
                                 }
                             },
+                            "forbidden_filename_characters": {
+                                "type": "array",
+                                "items": {
+                                    "type": "string"
+                                }
+                            },
                             "directEditing": {
                                 "type": "object",
                                 "required": [
diff --git a/config/config.sample.php b/config/config.sample.php
index d999b1e39b0..79f813a4dae 100644
--- a/config/config.sample.php
+++ b/config/config.sample.php
@@ -1962,6 +1962,8 @@ $CONFIG = [
 /**
  * Blacklist characters from being used in filenames. This is useful if you
  * have a filesystem or OS which does not support certain characters like windows.
+ * 
+ * The '/' and '\' characters are always forbidden.
  *
  * Example for windows systems: ``array('?', '<', '>', ':', '*', '|', '"', chr(0), "\n", "\r")``
  * see https://en.wikipedia.org/wiki/Comparison_of_file_systems#Limits
diff --git a/lib/private/Files/Storage/Common.php b/lib/private/Files/Storage/Common.php
index 830f0aaded7..0d4e8d29295 100644
--- a/lib/private/Files/Storage/Common.php
+++ b/lib/private/Files/Storage/Common.php
@@ -567,7 +567,9 @@ abstract class Common implements Storage, ILockingStorage, IWriteStreamStorage {
 	 * @throws InvalidPathException
 	 */
 	protected function verifyPosixPath($fileName) {
-		$this->scanForInvalidCharacters($fileName, "\\/");
+		$invalidChars = \OCP\Util::getForbiddenFileNameChars();
+		$this->scanForInvalidCharacters($fileName, $invalidChars);
+
 		$fileName = trim($fileName);
 		$reservedNames = ['*'];
 		if (in_array($fileName, $reservedNames)) {
@@ -577,11 +579,11 @@ abstract class Common implements Storage, ILockingStorage, IWriteStreamStorage {
 
 	/**
 	 * @param string $fileName
-	 * @param string $invalidChars
+	 * @param string[] $invalidChars
 	 * @throws InvalidPathException
 	 */
-	private function scanForInvalidCharacters($fileName, $invalidChars) {
-		foreach (str_split($invalidChars) as $char) {
+	private function scanForInvalidCharacters(string $fileName, array $invalidChars) {
+		foreach ($invalidChars as $char) {
 			if (str_contains($fileName, $char)) {
 				throw new InvalidCharacterInPathException();
 			}
@@ -668,7 +670,7 @@ abstract class Common implements Storage, ILockingStorage, IWriteStreamStorage {
 	private function isSameStorage(IStorage $storage): bool {
 		while ($storage->instanceOfStorage(Wrapper::class)) {
 			/**
-			 * @var Wrapper $sourceStorage
+			 * @var Wrapper $storage
 			 */
 			$storage = $storage->getWrapperStorage();
 		}
diff --git a/lib/private/legacy/OC_Util.php b/lib/private/legacy/OC_Util.php
index 3b6d9b8baec..42a0d9450b5 100644
--- a/lib/private/legacy/OC_Util.php
+++ b/lib/private/legacy/OC_Util.php
@@ -1112,8 +1112,8 @@ class OC_Util {
 			return false;
 		}
 
-		foreach (str_split($trimmed) as $char) {
-			if (str_contains(\OCP\Constants::FILENAME_INVALID_CHARS, $char)) {
+		foreach (\OCP\Util::getForbiddenFileNameChars() as $char) {
+			if (str_contains($trimmed, $char)) {
 				return false;
 			}
 		}
diff --git a/lib/public/Util.php b/lib/public/Util.php
index 4009237db5d..e3f394c1ca9 100644
--- a/lib/public/Util.php
+++ b/lib/public/Util.php
@@ -51,6 +51,7 @@ use OC\AppScriptDependency;
 use OC\AppScriptSort;
 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
@@ -521,10 +522,31 @@ class Util {
 	}
 
 	/**
+	 * 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 \OC\Files\View::verifyPath()
+	 * @deprecated 8.1.0 use OCP\Files\Storage\IStorage::verifyPath()
 	 * @since 7.0.0
 	 * @suppress PhanDeprecatedFunction
 	 */
diff --git a/tests/lib/Files/Storage/CommonTest.php b/tests/lib/Files/Storage/CommonTest.php
index 0900765c510..3740289df95 100644
--- a/tests/lib/Files/Storage/CommonTest.php
+++ b/tests/lib/Files/Storage/CommonTest.php
@@ -24,6 +24,7 @@ namespace Test\Files\Storage;
 
 use OC\Files\Storage\Wrapper\Jail;
 use OC\Files\Storage\Wrapper\Wrapper;
+use OCP\Files\InvalidPathException;
 use PHPUnit\Framework\MockObject\MockObject;
 
 /**
@@ -39,22 +40,66 @@ class CommonTest extends Storage {
 	 */
 	private $tmpDir;
 
+	private array $invalidCharsBackup;
+
 	protected function setUp(): void {
 		parent::setUp();
 
 		$this->tmpDir = \OC::$server->getTempManager()->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);
 		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'));
+
+		\OC::$server->getConfig()->setSystemValue('forbidden_chars', $additionalChars);
+
+		if ($throws) {
+			$this->expectException(InvalidPathException::class);
+		} else {
+			$this->expectNotToPerformAssertions();
+		}
+		$instance->verifyPath('/', $filename);
+	}
+
+	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 testMoveFromStorageWrapped() {
 		/** @var \OC\Files\Storage\CommonTest|MockObject $instance */
 		$instance = $this->getMockBuilder(\OC\Files\Storage\CommonTest::class)
-			->setMethods(['copyFromStorage', 'rmdir', 'unlink'])
+			->onlyMethods(['copyFromStorage', 'rmdir', 'unlink'])
 			->setConstructorArgs([['datadir' => $this->tmpDir]])
 			->getMock();
 		$instance->method('copyFromStorage')
@@ -72,7 +117,7 @@ class CommonTest extends Storage {
 	public function testMoveFromStorageJailed() {
 		/** @var \OC\Files\Storage\CommonTest|MockObject $instance */
 		$instance = $this->getMockBuilder(\OC\Files\Storage\CommonTest::class)
-			->setMethods(['copyFromStorage', 'rmdir', 'unlink'])
+			->onlyMethods(['copyFromStorage', 'rmdir', 'unlink'])
 			->setConstructorArgs([['datadir' => $this->tmpDir]])
 			->getMock();
 		$instance->method('copyFromStorage')
@@ -95,7 +140,7 @@ class CommonTest extends Storage {
 	public function testMoveFromStorageNestedJail() {
 		/** @var \OC\Files\Storage\CommonTest|MockObject $instance */
 		$instance = $this->getMockBuilder(\OC\Files\Storage\CommonTest::class)
-			->setMethods(['copyFromStorage', 'rmdir', 'unlink'])
+			->onlyMethods(['copyFromStorage', 'rmdir', 'unlink'])
 			->setConstructorArgs([['datadir' => $this->tmpDir]])
 			->getMock();
 		$instance->method('copyFromStorage')