diff --git a/apps/files/lib/Capabilities.php b/apps/files/lib/Capabilities.php index dc2aae6acfcd9..0fc95d6722682 100644 --- a/apps/files/lib/Capabilities.php +++ b/apps/files/lib/Capabilities.php @@ -39,13 +39,14 @@ public function __construct(IConfig $config) { /** * Return this classes capabilities * - * @return array{files: array{bigfilechunking: bool, blacklisted_files: array}} + * @return array{files: array{bigfilechunking: bool, blacklisted_files: array, forbidden_filename_characters: array}} */ 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 f9432f6c57cd1..f9cdb67783d78 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 d999b1e39b0b0..79f813a4daec2 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -1962,6 +1962,8 @@ /** * 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 830f0aaded79e..0d4e8d2929557 100644 --- a/lib/private/Files/Storage/Common.php +++ b/lib/private/Files/Storage/Common.php @@ -567,7 +567,9 @@ public function verifyPath($path, $fileName) { * @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 @@ protected function verifyPosixPath($fileName) { /** * @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 @@ public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t 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 3b6d9b8baec98..42a0d9450b567 100644 --- a/lib/private/legacy/OC_Util.php +++ b/lib/private/legacy/OC_Util.php @@ -1112,8 +1112,8 @@ public static function isValidFileName($file) { 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 4009237db5dc7..e3f394c1ca9d5 100644 --- a/lib/public/Util.php +++ b/lib/public/Util.php @@ -51,6 +51,7 @@ 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 @@ -520,11 +521,32 @@ 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 \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 0900765c5100c..3740289df95be 100644 --- a/tests/lib/Files/Storage/CommonTest.php +++ b/tests/lib/Files/Storage/CommonTest.php @@ -24,6 +24,7 @@ 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 @@ public function testMoveFromStorageWrapped() { 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 @@ public function testMoveFromStorageJailed() { 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')