Merge 60838bf5f4 into 72470e33be

apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php

@@ -100,6 +100,12 @@ class DirectoryTest extends \Test\TestCase {
 			->willReturn(Constants::PERMISSION_READ);
 	}
 
+	protected function tearDown(): void {
+		// Reset invalid chars as we touched this during the tests
+		self::invokePrivate(\OCP\Util::class, 'invalidChars', [[]]);
+		parent::tearDown();
+	}
+
 	private function getDir($path = '/') {
 		$this->view->expects($this->once())
 			->method('getRelativePath')
@@ -411,6 +417,9 @@ class DirectoryTest extends \Test\TestCase {
 	 * @dataProvider moveFailedInvalidCharsProvider
 	 */
 	public function testMoveFailedInvalidChars($source, $destination, $updatables, $deletables): void {
+		// Enforce * as an invalid character
+		self::invokePrivate(\OCP\Util::class, 'invalidChars', [['*', '/', '\\']]);
+
 		$this->expectException(\OCA\DAV\Connector\Sabre\Exception\InvalidPath::class);
 
 		$this->moveTest($source, $destination, $updatables, $deletables);

apps/dav/tests/unit/Connector/Sabre/FileTest.php

@@ -91,6 +91,8 @@ class FileTest extends TestCase {
 		$userManager = \OCP\Server::get(IUserManager::class);
 		$userManager->get($this->user)->delete();
 
+		// Reset invalid chars as we touched this during the tests
+		self::invokePrivate(\OCP\Util::class, 'invalidChars', [[]]);
 		parent::tearDown();
 	}
 
@@ -823,7 +825,8 @@ class FileTest extends TestCase {
 	 * Test put file with invalid chars
 	 */
 	public function testSimplePutInvalidChars(): void {
-		// setup
+		// Enforce * as an invalid character
+		self::invokePrivate(\OCP\Util::class, 'invalidChars', [['*', '/', '\\']]);
 		/** @var View|MockObject */
 		$view = $this->getMockBuilder(View::class)
 			->onlyMethods(['getRelativePath'])
@@ -858,12 +861,12 @@ class FileTest extends TestCase {
 
 	/**
 	 * Test setting name with setName() with invalid chars
-	 *
 	 */
 	public function testSetNameInvalidChars(): void {
 		$this->expectException(\OCA\DAV\Connector\Sabre\Exception\InvalidPath::class);
 
-		// setup
+		// Enforce * as an invalid character
+		self::invokePrivate(\OCP\Util::class, 'invalidChars', [['*', '/', '\\']]);
 		/** @var View|MockObject */
 		$view = $this->getMockBuilder(View::class)
 			->onlyMethods(['getRelativePath'])

config/config.sample.php

@@ -1961,21 +1961,24 @@ $CONFIG = [
 'updatedirectory' => '',
 
 /**
- * Blacklist a specific file or files and disallow the upload of files
+ * Deny a specific file or files and disallow the upload of files
  * with this name. ``.htaccess`` is blocked by default.
+ *
  * WARNING: USE THIS ONLY IF YOU KNOW WHAT YOU ARE DOING.
  *
+ * Note that this list is case-insensitive.
+ *
  * Defaults to ``array('.htaccess')``
  */
 'blacklisted_files' => ['.htaccess'],
 
 /**
- * Blacklist characters from being used in filenames. This is useful if you
+ * Deny 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.
+ * The '/' and '\' characters are always forbidden, as well as all characters in the ASCII range [0-31].
  *
- * Example for windows systems: ``array('?', '<', '>', ':', '*', '|', '"', chr(0), "\n", "\r")``
+ * Example for windows systems: ``array('?', '<', '>', ':', '*', '|', '"')``
  * see https://en.wikipedia.org/wiki/Comparison_of_file_systems#Limits
  *
  * Defaults to ``array()``

lib/private/Files/Filesystem.php

@@ -59,9 +59,6 @@ class Filesystem {
 
 	private static ?CappedMemoryCache $normalizedPathCache = null;
 
-	/** @var string[]|null */
-	private static ?array $blacklist = null;
-
 	/**
 	 * classname which used for hooks handling
 	 * used as signalclass in OC_Hooks::emit()
@@ -462,15 +459,35 @@ class Filesystem {
 	 * @param string $filename
 	 * @return bool
 	 */
-	public static function isFileBlacklisted($filename) {
+	public static function hasFilenameInvalidCharacters(string $filename): bool {
+		$invalidChars = \OCP\Util::getForbiddenFileNameChars();
+		foreach ($invalidChars as $char) {
+			if (str_contains($filename, $char)) {
+				return true;
+			}
+		}
+
+		$sanitizedFileName = filter_var($filename, FILTER_UNSAFE_RAW, FILTER_FLAG_STRIP_LOW);
+		if ($sanitizedFileName !== $filename) {
+			return true;
+		}
+		return false;
+	}
+
+	/**
+	 * @param string $filename
+	 * @return bool
+	 */
+	public static function isFileBlacklisted(string $filename): bool {
 		$filename = self::normalizePath($filename);
+		$filename = basename($filename);
 
-		if (self::$blacklist === null) {
-			self::$blacklist = \OC::$server->getConfig()->getSystemValue('blacklisted_files', ['.htaccess']);
+		if ($filename === '') {
+			return false;
 		}
 
-		$filename = strtolower(basename($filename));
-		return in_array($filename, self::$blacklist);
+		$forbiddenNames = \OCP\Util::getForbiddenFilenames();
+		return in_array(mb_strtolower($filename), $forbiddenNames);
 	}
 
 	/**

lib/private/Files/Storage/Common.php

@@ -558,41 +558,14 @@ abstract class Common implements Storage, ILockingStorage, IWriteStreamStorage {
 			throw new FileNameTooLongException();
 		}
 
-		// NOTE: $path will remain unverified for now
-		$this->verifyPosixPath($fileName);
-	}
-
-	/**
-	 * @param string $fileName
-	 * @throws InvalidPathException
-	 */
-	protected function verifyPosixPath($fileName) {
-		$invalidChars = \OCP\Util::getForbiddenFileNameChars();
-		$this->scanForInvalidCharacters($fileName, $invalidChars);
-
-		$fileName = trim($fileName);
-		$reservedNames = ['*'];
-		if (in_array($fileName, $reservedNames)) {
+		if (\OC\Files\Filesystem::isFileBlacklisted($fileName)) {
 			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) {
+		if (\OC\Files\Filesystem::hasFilenameInvalidCharacters($fileName)) {
 			throw new InvalidCharacterInPathException();
 		}
+		// NOTE: $path will remain unverified for now
 	}
 
 	/**

lib/private/Template/JSConfigHelper.php

@@ -168,7 +168,10 @@ class JSConfigHelper {
 
 		$config = [
 			'auto_logout' => $this->config->getSystemValue('auto_logout', false),
+			// deprecated
 			'blacklist_files_regex' => FileInfo::BLACKLIST_FILES_REGEX,
+			'forbidden_filenames' => \OCP\Util::getForbiddenFilenames(),
+			'forbidden_filename_characters' => \OCP\Util::getForbiddenFileNameChars(),
 			'loglevel' => $this->config->getSystemValue('loglevel_frontend',
 				$this->config->getSystemValue('loglevel', ILogger::WARN)
 			),

lib/private/legacy/OC_Util.php

@@ -1092,35 +1092,6 @@ class OC_Util {
 		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

lib/public/Files/FileInfo.php

@@ -70,6 +70,7 @@ interface FileInfo {
 	/**
 	 * @const \OCP\Files\FileInfo::BLACKLIST_FILES_REGEX Return regular expression to test filenames against (blacklisting)
 	 * @since 12.0.0
+	 * @deprecated 30.0.0 Use \OCP\Util::getForbiddenFilenames() instead
 	 */
 	public const BLACKLIST_FILES_REGEX = '\.(part|filepart)$';
 

lib/public/Util.php

@@ -61,6 +61,10 @@ use Psr\Log\LoggerInterface;
 class Util {
 	private static ?IManager $shareManager = null;
 
+	/** @var string[] */
+	private static array $invalidChars = [];
+	/** @var string[] */
+	private static array $invalidFilenames = [];
 	private static array $scriptsInit = [];
 	private static array $scripts = [];
 	private static array $scriptDeps = [];
@@ -521,25 +525,55 @@ class Util {
 		return \OC_Helper::uploadLimit();
 	}
 
+	/**
+	 * Get a list of reserved file names that must not be used
+	 * This list should be checked case-insensitive, all names are returned lowercase.
+	 * @return string[]
+	 * @since 30.0.0
+	 */
+	public static function getForbiddenFilenames(): array {
+		if (empty(self::$invalidFilenames)) {
+			$config = \OCP\Server::get(IConfig::class);
+
+			$invalidFilenames = $config->getSystemValue('blacklisted_files', ['.htaccess']);
+			if (!is_array($invalidFilenames)) {
+				\OCP\Server::get(LoggerInterface::class)->error('Invalid system config value for "blacklisted_files" is ignored.');
+				$invalidFilenames = ['.htaccess'];
+			}
+			self::$invalidFilenames = array_map('mb_strtolower', $invalidFilenames);
+		}
+		return self::$invalidFilenames;
+	}
+
 	/**
 	 * Get a list of characters forbidden in file names
+	 *
+	 * Note: Characters in the range [0-31] are always forbidden,
+	 * even if not inside this list (see OCP\Files\Storage\IStorage::verifyPath).
+	 *
 	 * @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 = [];
+		if (empty(self::$invalidChars)) {
+			$config = \OCP\Server::get(IConfig::class);
+
+			// Get always forbidden characters
+			$invalidChars = str_split(\OCP\Constants::FILENAME_INVALID_CHARS);
+			if ($invalidChars === false) {
+				$invalidChars = [];
+			}
+
+			// Get admin defined invalid characters
+			$additionalChars = $config->getSystemValue('forbidden_chars', []);
+			if (!is_array($additionalChars)) {
+				\OCP\Server::get(LoggerInterface::class)->error('Invalid system config value for "forbidden_chars" is ignored.');
+				$additionalChars = [];
+			}
+			self::$invalidChars = array_merge($invalidChars, $additionalChars);
 		}
 
-		// 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);
+		return self::$invalidChars;
 	}
 
 	/**
@@ -551,7 +585,16 @@ class Util {
 	 * @suppress PhanDeprecatedFunction
 	 */
 	public static function isValidFileName($file) {
-		return \OC_Util::isValidFileName($file);
+		$trimmed = trim($file);
+		if ($trimmed === '' || \OC\Files\Filesystem::isIgnoredDir($trimmed)) {
+			return false;
+		}
+
+		if (\OC\Files\Filesystem::isFileBlacklisted($file)) {
+			return false;
+		}
+
+		return !\OC\Files\Filesystem::hasFilenameInvalidCharacters($file);
 	}
 
 	/**

tests/lib/Files/FilesystemTest.php

@@ -295,6 +295,46 @@ class FilesystemTest extends \Test\TestCase {
 		$this->assertSame($expected, \OC\Files\Filesystem::isFileBlacklisted($path));
 	}
 
+	/**
+	 * @dataProvider hasFilenameInvalidCharactersData
+	 */
+	public function testHasFilenameInvalidCharacters($filename, $expected) {
+		$this->assertSame($expected, \OC\Files\Filesystem::hasFilenameInvalidCharacters($filename));
+	}
+
+	public function hasFilenameInvalidCharactersData(): array {
+		return array_merge(
+			[
+				// slash and backslash are always forbidden
+				['foobar/txt', true],
+				['foobar\\txt', true],
+				// some valid special characters
+				['foobar txt', false],
+				['foobar-txt', false],
+				['foobar_txt', false],
+				// Also unicode is allowed by default
+				['😶‍🌫️.txt', false],
+			],
+			// Always block ascii 0-31
+			array_map(fn (int $i) => ['foo' . chr($i) . 'txt', true], range(0, 31)),
+		);
+	}
+
+	public static function hasFilenameInvalidCharacters(string $filename): bool {
+		$invalidChars = \OCP\Util::getForbiddenFileNameChars();
+		foreach ($invalidChars as $char) {
+			if (str_contains($filename, $char)) {
+				return true;
+			}
+		}
+
+		$sanitizedFileName = filter_var($filename, FILTER_UNSAFE_RAW, FILTER_FLAG_STRIP_LOW);
+		if ($sanitizedFileName !== $filename) {
+			return true;
+		}
+		return false;
+	}
+
 	public function testNormalizePathUTF8() {
 		if (!class_exists('Patchwork\PHP\Shim\Normalizer')) {
 			$this->markTestSkipped('UTF8 normalizer Patchwork was not found');

tests/lib/Files/PathVerificationTest.php

@@ -27,8 +27,19 @@ class PathVerificationTest extends \Test\TestCase {
 	protected function setUp(): void {
 		parent::setUp();
 		$this->view = new View();
+		self::resetOCPUtil();
 	}
 
+	protected function tearDown(): void {
+		parent::tearDown();
+		self::resetOCPUtil();
+	}
+
+	protected static function resetOCPUtil(): void {
+		// Reset util cache
+		self::invokePrivate(\OCP\Util::class, 'invalidChars', [[]]);
+		self::invokePrivate(\OCP\Util::class, 'invalidFilenames', [[]]);
+	}
 
 	public function testPathVerificationFileNameTooLong() {
 		$this->expectException(\OCP\Files\InvalidPathException::class);
@@ -115,12 +126,16 @@ class PathVerificationTest extends \Test\TestCase {
 		$storage = new Local(['datadir' => '']);
 
 		$fileName = " 123{$fileName}456 ";
-		self::invokePrivate($storage, 'verifyPosixPath', [$fileName]);
+		$storage->verifyPath('', $fileName);
 	}
 
 	public function providesInvalidCharsPosix() {
 		return [
+			// posix forbidden
 			[\chr(0)],
+			['/'],
+			['\\'],
+			// We restrict also ascii 1-31
 			[\chr(1)],
 			[\chr(2)],
 			[\chr(3)],
@@ -152,8 +167,6 @@ class PathVerificationTest extends \Test\TestCase {
 			[\chr(29)],
 			[\chr(30)],
 			[\chr(31)],
-			['/'],
-			['\\'],
 		];
 	}
 

tests/lib/Files/Storage/CommonTest.php

@@ -25,6 +25,8 @@ namespace Test\Files\Storage;
 use OC\Files\Storage\Wrapper\Jail;
 use OC\Files\Storage\Wrapper\Wrapper;
 use OCP\Files\InvalidPathException;
+use OCP\IConfig;
+use OCP\ITempManager;
 use PHPUnit\Framework\MockObject\MockObject;
 
 /**
@@ -33,6 +35,7 @@ use PHPUnit\Framework\MockObject\MockObject;
  * @group DB
  *
  * @package Test\Files\Storage
+ * @backupStaticAttributes enabled
  */
 class CommonTest extends Storage {
 	/**
@@ -44,16 +47,25 @@ class CommonTest extends Storage {
 
 	protected function setUp(): void {
 		parent::setUp();
+		self::resetOCPUtil();
 
-		$this->tmpDir = \OC::$server->getTempManager()->getTemporaryFolder();
+		$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', []);
+		$this->invalidCharsBackup = \OCP\Server::get(IConfig::class)->getSystemValue('forbidden_chars', []);
 	}
 
 	protected function tearDown(): void {
 		\OC_Helper::rmdirr($this->tmpDir);
-		\OC::$server->getConfig()->setSystemValue('forbidden_chars', $this->invalidCharsBackup);
+		\OCP\Server::get(IConfig::class)->setSystemValue('forbidden_chars', $this->invalidCharsBackup);
+
 		parent::tearDown();
+		self::resetOCPUtil();
+	}
+
+	protected static function resetOCPUtil(): void {
+		// Reset util cache as we do not want to leak our test values into other tests
+		self::invokePrivate(\OCP\Util::class, 'invalidChars', [[]]);
+		self::invokePrivate(\OCP\Util::class, 'invalidFilenames', [[]]);
 	}
 
 	/**
@@ -68,7 +80,7 @@ class CommonTest extends Storage {
 		$instance->method('copyFromStorage')
 			->willThrowException(new \Exception('copy'));
 
-		\OC::$server->getConfig()->setSystemValue('forbidden_chars', $additionalChars);
+		\OCP\Server::get(IConfig::class)->setSystemValue('forbidden_chars', $additionalChars);
 
 		if ($throws) {
 			$this->expectException(InvalidPathException::class);

tests/lib/UtilTest.php

@@ -9,6 +9,7 @@
 namespace Test;
 
 use OC_Util;
+use OCP\IConfig;
 
 /**
  * Class UtilTest
@@ -17,6 +18,25 @@ use OC_Util;
  * @group DB
  */
 class UtilTest extends \Test\TestCase {
+	protected function setUp(): void {
+		parent::setUp();
+		self::resetOCPUtil();
+	}
+
+	protected function tearDown(): void {
+		parent::tearDown();
+		self::resetOCPUtil();
+	}
+
+	protected static function resetOCPUtil() {
+		\OC_Util::$scripts = [];
+		\OC_Util::$styles = [];
+		self::invokePrivate(\OCP\Util::class, 'scripts', [[]]);
+		self::invokePrivate(\OCP\Util::class, 'scriptDeps', [[]]);
+		self::invokePrivate(\OCP\Util::class, 'invalidChars', [[]]);
+		self::invokePrivate(\OCP\Util::class, 'invalidFilenames', [[]]);
+	}
+
 	public function testGetVersion() {
 		$version = \OCP\Util::getVersion();
 		$this->assertTrue(is_array($version));
@@ -122,12 +142,89 @@ class UtilTest extends \Test\TestCase {
 		$this->assertSame(1, $matchesRegex);
 	}
 
+	public function testGetForbiddenCharacters() {
+		$config = \OCP\Server::get(IConfig::class);
+		$backup = $config->getSystemValue('forbidden_chars', []);
+
+		try {
+			// Fake config
+			$config->setSystemValue('forbidden_chars', ['*', '-']);
+			$this->assertEqualsCanonicalizing(
+				[
+					// Added by the test
+					'*',
+					'-',
+					// Always added
+					'/',
+					'\\',
+				],
+				\OCP\Util::getForbiddenFileNameChars(),
+			);
+		} finally {
+			// Reset config
+			$config->setSystemValue('forbidden_chars', $backup);
+		}
+	}
+
+	public function testGetForbiddenCharactersInvalidConfig() {
+		$config = \OCP\Server::get(IConfig::class);
+		$backup = $config->getSystemValue('forbidden_chars', []);
+
+		try {
+			// Fake config
+			$config->setSystemValue('forbidden_chars', 'not an array');
+			$this->assertEqualsCanonicalizing(
+				[
+					// Always added
+					'/',
+					'\\',
+				],
+				\OCP\Util::getForbiddenFileNameChars(),
+			);
+		} finally {
+			// Reset config
+			$config->setSystemValue('forbidden_chars', $backup);
+		}
+	}
+
+	public function testGetForbiddenFilenames() {
+		$config = \OCP\Server::get(IConfig::class);
+		$backup = $config->getSystemValue('blacklisted_files', ['.htaccess']);
+
+		try {
+			// Fake config
+			$config->setSystemValue('blacklisted_files', ['.htaccess', 'foo-bar']);
+			$this->assertEqualsCanonicalizing(
+				['.htaccess', 'foo-bar'],
+				\OCP\Util::getForbiddenFilenames(),
+			);
+		} finally {
+			// Reset config
+			$config->setSystemValue('blacklisted_files', $backup);
+		}
+	}
+
+	public function testGetForbiddenFilenamesInvalidConfig() {
+		$config = \OCP\Server::get(IConfig::class);
+		$backup = $config->getSystemValue('blacklisted_files', ['.htaccess']);
+
+		try {
+			// Fake config
+			$config->setSystemValue('blacklisted_files', 'not an array');
+			$this->assertEqualsCanonicalizing(
+				['.htaccess'],
+				\OCP\Util::getForbiddenFilenames(),
+			);
+		} finally {
+			// Reset config
+			$config->setSystemValue('blacklisted_files', $backup);
+		}
+	}
+
 	/**
 	 * @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));
 	}
@@ -169,10 +266,8 @@ class UtilTest extends \Test\TestCase {
 			['. ', false],
 			[' .', false],
 
-			// part files not allowed
-			['.part', false],
-			['notallowed.part', false],
-			['neither.filepart', false],
+			// htaccess files not allowed
+			['.htaccess', false],
 
 			// part in the middle is ok
 			['super movie part one.mkv', true],
@@ -220,23 +315,6 @@ class UtilTest extends \Test\TestCase {
 		$this->assertNotEmpty($errors);
 	}
 
-	protected function setUp(): void {
-		parent::setUp();
-
-		\OC_Util::$scripts = [];
-		\OC_Util::$styles = [];
-		self::invokePrivate(\OCP\Util::class, 'scripts', [[]]);
-		self::invokePrivate(\OCP\Util::class, 'scriptDeps', [[]]);
-	}
-	protected function tearDown(): void {
-		parent::tearDown();
-
-		\OC_Util::$scripts = [];
-		\OC_Util::$styles = [];
-		self::invokePrivate(\OCP\Util::class, 'scripts', [[]]);
-		self::invokePrivate(\OCP\Util::class, 'scriptDeps', [[]]);
-	}
-
 	public function testAddScript() {
 		\OCP\Util::addScript('first', 'myFirstJSFile');
 		\OCP\Util::addScript('core', 'myFancyJSFile1');