diff --git a/apps/dav/lib/Connector/Sabre/Node.php b/apps/dav/lib/Connector/Sabre/Node.php index 63e453c86afe7..03992b3e681fc 100644 --- a/apps/dav/lib/Connector/Sabre/Node.php +++ b/apps/dav/lib/Connector/Sabre/Node.php @@ -122,12 +122,11 @@ public function setName($name) { [$parentPath,] = \Sabre\Uri\split($this->path); [, $newName] = \Sabre\Uri\split($name); - - // verify path of the target - $this->verifyPath(); - $newPath = $parentPath . '/' . $newName; + // verify path of the target + $this->verifyPath($newPath); + if (!$this->fileView->rename($this->path, $newPath)) { throw new \Sabre\DAV\Exception('Failed to rename '. $this->path . ' to ' . $newPath); } @@ -355,10 +354,12 @@ public function getOwner() { return $this->info->getOwner(); } - protected function verifyPath() { + protected function verifyPath(?string $path = null): void { + $path = $path ?? $this->info->getPath(); try { - $fileName = basename($this->info->getPath()); - $this->fileView->verifyPath($this->path, $fileName); + $filename = basename($path); + $dirname = dirname($path); + $this->fileView->verifyPath($dirname, $filename); } 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 3eca9b7ac1c1c..40dbdf663a10a 100644 --- a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php +++ b/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/b', 'a/ ', ['a' => true, 'a/b' => true, 'a/c ' => false], []], ]; } diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index 060f0294256e4..2ef1e01e8ccaf 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/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("/\n", $this->getMockStorage(), null, [ 'permissions' => \OCP\Constants::PERMISSION_ALL, 'type' => FileInfo::TYPE_FOLDER, ], null); @@ -611,15 +611,14 @@ public function testSetNameInvalidChars(): void { ->method('getRelativePath') ->willReturnArgument(0); - $info = new \OC\Files\FileInfo('/*', $this->getMockStorage(), null, [ + $info = new \OC\Files\FileInfo('/super', $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("/super\nstar.txt"); } - public function testUploadAbort(): void { // setup /** @var View|MockObject */ diff --git a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php index f0996a17a3360..24816eab4c8c9 100644 --- a/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php @@ -15,6 +15,7 @@ use OCA\DAV\Connector\Sabre\Directory; use OCA\DAV\Connector\Sabre\ObjectTree; use OCP\Files\Mount\IMountManager; +use PHPUnit\Framework\MockObject\MockObject; /** * Class ObjectTreeTest @@ -205,12 +206,11 @@ public function testGetNodeForPathInvalidPath(): void { $this->expectException(\OCA\DAV\Connector\Sabre\Exception\InvalidPath::class); $path = '/foo\bar'; - - $storage = new Temporary([]); + /** @var View&MockObject */ $view = $this->getMockBuilder(View::class) - ->setMethods(['resolvePath']) + ->onlyMethods(['resolvePath']) ->getMock(); $view->expects($this->once()) ->method('resolvePath') @@ -218,6 +218,7 @@ public function testGetNodeForPathInvalidPath(): void { return [$storage, ltrim($path, '/')]; }); + /** @var Directory&MockObject */ $rootNode = $this->getMockBuilder(Directory::class) ->disableOriginalConstructor() ->getMock(); diff --git a/apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php b/apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php index 2f41502d4f607..f47a261d224b6 100644 --- a/apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php +++ b/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); } @@ -732,7 +734,7 @@ public function getUserDisplayName(string $userId): string { } try { - $slaveService = Server::get(\OCA\GlobalSiteSelector\Service\SlaveService::class); + $slaveService = Server::get('\OCA\GlobalSiteSelector\Service\SlaveService'); } catch (\Throwable $e) { Server::get(LoggerInterface::class)->error( $e->getMessage(), diff --git a/apps/files/lib/Capabilities.php b/apps/files/lib/Capabilities.php index 9147e7d9f3aaa..7be6843e4f60a 100644 --- a/apps/files/lib/Capabilities.php +++ b/apps/files/lib/Capabilities.php @@ -7,28 +7,31 @@ */ namespace OCA\Files; +use OC\Files\FilenameValidator; use OCP\Capabilities\ICapability; -use OCP\IConfig; class Capabilities implements ICapability { - protected IConfig $config; - - public function __construct(IConfig $config) { - $this->config = $config; + public function __construct( + protected FilenameValidator $filenameValidator, + ) { } /** * Return this classes capabilities * - * @return array{files: array{bigfilechunking: bool, blacklisted_files: array, forbidden_filename_characters: array}} + * @return array{files: array{$comment: string, bigfilechunking: bool, blacklisted_files: array, forbidden_filenames: list, forbidden_filename_characters: list, forbidden_filename_extensions: list}} */ public function getCapabilities() { return [ 'files' => [ + '$comment' => '"blacklisted_files" is deprecacted as of Nextcloud 30, use "forbidden_filenames" instead', + 'blacklisted_files' => $this->filenameValidator->getForbiddenFilenames(), + 'forbidden_filenames' => $this->filenameValidator->getForbiddenFilenames(), + 'forbidden_filename_characters' => $this->filenameValidator->getForbiddenCharacters(), + 'forbidden_filename_extensions' => $this->filenameValidator->getForbiddenExtensions(), + 'bigfilechunking' => true, - 'blacklisted_files' => (array)$this->config->getSystemValue('blacklisted_files', ['.htaccess']), - 'forbidden_filename_characters' => \OCP\Util::getForbiddenFileNameChars(), ], ]; } diff --git a/apps/files/lib/Controller/ViewController.php b/apps/files/lib/Controller/ViewController.php index 0f24db6b0776c..0d525e48290aa 100644 --- a/apps/files/lib/Controller/ViewController.php +++ b/apps/files/lib/Controller/ViewController.php @@ -220,10 +220,6 @@ 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(); - $this->initialState->provideInitialState('forbiddenCharacters', $forbiddenCharacters); - $event = new LoadAdditionalScriptsEvent(); $this->eventDispatcher->dispatchTyped($event); $this->eventDispatcher->dispatchTyped(new ResourcesLoadAdditionalScriptsEvent()); diff --git a/apps/files/src/components/FileEntry/FileEntryName.vue b/apps/files/src/components/FileEntry/FileEntryName.vue index 4e5a3571e74ba..ef730e3fa3168 100644 --- a/apps/files/src/components/FileEntry/FileEntryName.vue +++ b/apps/files/src/components/FileEntry/FileEntryName.vue @@ -42,8 +42,7 @@ import type { PropType } from 'vue' import { showError, showSuccess } from '@nextcloud/dialogs' import { emit } from '@nextcloud/event-bus' -import { FileType, NodeStatus, Permission } from '@nextcloud/files' -import { loadState } from '@nextcloud/initial-state' +import { FileType, NodeStatus, Permission, isFilenameValid } from '@nextcloud/files' import { translate as t } from '@nextcloud/l10n' import axios, { isAxiosError } from '@nextcloud/axios' import { defineComponent } from 'vue' @@ -54,8 +53,6 @@ import { useNavigation } from '../../composables/useNavigation' import { useRenamingStore } from '../../store/renaming.ts' import logger from '../../logger.js' -const forbiddenCharacters = loadState('files', 'forbiddenCharacters', []) - export default defineComponent({ name: 'FileEntryName', @@ -210,24 +207,12 @@ export default defineComponent({ }, isFileNameValid(name: string) { - const trimmedName = name.trim() - if (trimmedName === '.' || trimmedName === '..') { + if (!isFilenameValid(name)) { throw new Error(t('files', '"{name}" is an invalid file name.', { name })) - } else if (trimmedName.length === 0) { - throw new Error(t('files', 'File name cannot be empty.')) - } else if (trimmedName.indexOf('/') !== -1) { - throw new Error(t('files', '"/" is not allowed inside a file name.')) - } else if (trimmedName.match(window.OC.config.blacklist_files_regex)) { - throw new Error(t('files', '"{name}" is not an allowed filetype.', { name })) } else if (this.checkIfNodeExists(name)) { throw new Error(t('files', '{newName} already exists.', { newName: name })) } - const char = forbiddenCharacters.find((char) => trimmedName.includes(char)) - if (char) { - throw new Error(t('files', '"{char}" is not allowed inside a file name.', { char })) - } - return true }, diff --git a/apps/files/src/views/FilesList.vue b/apps/files/src/views/FilesList.vue index 98a817ac06754..45a9a58d1461e 100644 --- a/apps/files/src/views/FilesList.vue +++ b/apps/files/src/views/FilesList.vue @@ -194,7 +194,7 @@ export default defineComponent({ const { currentView } = useNavigation() const enableGridView = (loadState('core', 'config', [])['enable_non-accessible_features'] ?? true) - const forbiddenCharacters = loadState('files', 'forbiddenCharacters', []) + const forbiddenCharacters = (window as unknown as { '_oc_config': { 'forbidden_filename_characters': string }})._oc_config.forbidden_filename_characters return { currentView, diff --git a/build/integration/features/bootstrap/CapabilitiesContext.php b/build/integration/features/bootstrap/CapabilitiesContext.php index 79ede6ac8ba62..8e98d9fb72076 100644 --- a/build/integration/features/bootstrap/CapabilitiesContext.php +++ b/build/integration/features/bootstrap/CapabilitiesContext.php @@ -22,7 +22,9 @@ class CapabilitiesContext implements Context, SnippetAcceptingContext { * @param \Behat\Gherkin\Node\TableNode|null $formData */ public function checkCapabilitiesResponse(\Behat\Gherkin\Node\TableNode $formData) { - $capabilitiesXML = simplexml_load_string($this->response->getBody())->data->capabilities; + $capabilitiesXML = simplexml_load_string($this->response->getBody()); + Assert::assertNotFalse($capabilitiesXML, 'Failed to fetch capabilities'); + $capabilitiesXML = $capabilitiesXML->data->capabilities; foreach ($formData->getHash() as $row) { $path_to_element = explode('@@@', $row['path_to_element']); diff --git a/config/config.sample.php b/config/config.sample.php index 77fd2490a1498..76b6532a19c46 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -1976,26 +1976,38 @@ 'updatedirectory' => '', /** - * Blacklist a specific file or files and disallow the upload of files + * Block 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'], +'forbidden_filenames' => ['.htaccess'], /** - * Blacklist characters from being used in filenames. This is useful if you + * Block 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()`` */ -'forbidden_chars' => [], +'forbidden_filename_characters' => [], + +/** + * Deny extensions from being used for filenames. + * + * The '.part' extension is always forbidden, as this is used internally by Nextcloud. + * + * Defaults to ``array('.filepart', '.part')`` + */ +'forbidden_filename_extensions' => ['.part', '.filepart'], /** * If you are applying a theme to Nextcloud, enter the name of the theme here. diff --git a/core/Controller/AvatarController.php b/core/Controller/AvatarController.php index f29c9c5ab4f54..5935ce1ef2e63 100644 --- a/core/Controller/AvatarController.php +++ b/core/Controller/AvatarController.php @@ -187,10 +187,8 @@ public function postAvatar(?string $path = null): JSONResponse { ); } } elseif (!is_null($files)) { - if ( - $files['error'][0] === 0 && - is_uploaded_file($files['tmp_name'][0]) && - !\OC\Files\Filesystem::isFileBlacklisted($files['tmp_name'][0]) + if ($files['error'][0] === 0 + && is_uploaded_file($files['tmp_name'][0]) ) { if ($files['size'][0] > 20 * 1024 * 1024) { return new JSONResponse( diff --git a/core/Controller/OCJSController.php b/core/Controller/OCJSController.php index 11a6e5827d8ae..8a6193d2e5341 100644 --- a/core/Controller/OCJSController.php +++ b/core/Controller/OCJSController.php @@ -8,6 +8,7 @@ use bantu\IniGetWrapper\IniGetWrapper; use OC\Authentication\Token\IProvider; use OC\CapabilitiesManager; +use OC\Files\FilenameValidator; use OC\Template\JSConfigHelper; use OCP\App\IAppManager; use OCP\AppFramework\Controller; @@ -44,6 +45,7 @@ public function __construct( CapabilitiesManager $capabilitiesManager, IInitialStateService $initialStateService, IProvider $tokenProvider, + FilenameValidator $filenameValidator, ) { parent::__construct($appName, $request); @@ -59,7 +61,8 @@ public function __construct( $urlGenerator, $capabilitiesManager, $initialStateService, - $tokenProvider + $tokenProvider, + $filenameValidator, ); } diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 958223bf2fc0f..d5c40482e67dd 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -378,6 +378,7 @@ 'OCP\\Files\\ForbiddenException' => $baseDir . '/lib/public/Files/ForbiddenException.php', 'OCP\\Files\\GenericFileException' => $baseDir . '/lib/public/Files/GenericFileException.php', 'OCP\\Files\\IAppData' => $baseDir . '/lib/public/Files/IAppData.php', + 'OCP\\Files\\IFilenameValidator' => $baseDir . '/lib/public/Files/IFilenameValidator.php', 'OCP\\Files\\IHomeStorage' => $baseDir . '/lib/public/Files/IHomeStorage.php', 'OCP\\Files\\IMimeTypeDetector' => $baseDir . '/lib/public/Files/IMimeTypeDetector.php', 'OCP\\Files\\IMimeTypeLoader' => $baseDir . '/lib/public/Files/IMimeTypeLoader.php', @@ -1442,6 +1443,7 @@ 'OC\\Files\\Config\\UserMountCache' => $baseDir . '/lib/private/Files/Config/UserMountCache.php', 'OC\\Files\\Config\\UserMountCacheListener' => $baseDir . '/lib/private/Files/Config/UserMountCacheListener.php', 'OC\\Files\\FileInfo' => $baseDir . '/lib/private/Files/FileInfo.php', + 'OC\\Files\\FilenameValidator' => $baseDir . '/lib/private/Files/FilenameValidator.php', 'OC\\Files\\Filesystem' => $baseDir . '/lib/private/Files/Filesystem.php', 'OC\\Files\\Lock\\LockManager' => $baseDir . '/lib/private/Files/Lock/LockManager.php', 'OC\\Files\\Mount\\CacheMountProvider' => $baseDir . '/lib/private/Files/Mount/CacheMountProvider.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index df24fac37657e..059ba7eff2fc6 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -411,6 +411,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OCP\\Files\\ForbiddenException' => __DIR__ . '/../../..' . '/lib/public/Files/ForbiddenException.php', 'OCP\\Files\\GenericFileException' => __DIR__ . '/../../..' . '/lib/public/Files/GenericFileException.php', 'OCP\\Files\\IAppData' => __DIR__ . '/../../..' . '/lib/public/Files/IAppData.php', + 'OCP\\Files\\IFilenameValidator' => __DIR__ . '/../../..' . '/lib/public/Files/IFilenameValidator.php', 'OCP\\Files\\IHomeStorage' => __DIR__ . '/../../..' . '/lib/public/Files/IHomeStorage.php', 'OCP\\Files\\IMimeTypeDetector' => __DIR__ . '/../../..' . '/lib/public/Files/IMimeTypeDetector.php', 'OCP\\Files\\IMimeTypeLoader' => __DIR__ . '/../../..' . '/lib/public/Files/IMimeTypeLoader.php', @@ -1475,6 +1476,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Files\\Config\\UserMountCache' => __DIR__ . '/../../..' . '/lib/private/Files/Config/UserMountCache.php', 'OC\\Files\\Config\\UserMountCacheListener' => __DIR__ . '/../../..' . '/lib/private/Files/Config/UserMountCacheListener.php', 'OC\\Files\\FileInfo' => __DIR__ . '/../../..' . '/lib/private/Files/FileInfo.php', + 'OC\\Files\\FilenameValidator' => __DIR__ . '/../../..' . '/lib/private/Files/FilenameValidator.php', 'OC\\Files\\Filesystem' => __DIR__ . '/../../..' . '/lib/private/Files/Filesystem.php', 'OC\\Files\\Lock\\LockManager' => __DIR__ . '/../../..' . '/lib/private/Files/Lock/LockManager.php', 'OC\\Files\\Mount\\CacheMountProvider' => __DIR__ . '/../../..' . '/lib/private/Files/Mount/CacheMountProvider.php', diff --git a/lib/private/AppFramework/OCS/BaseResponse.php b/lib/private/AppFramework/OCS/BaseResponse.php index 2e685de856b20..d367574a179bb 100644 --- a/lib/private/AppFramework/OCS/BaseResponse.php +++ b/lib/private/AppFramework/OCS/BaseResponse.php @@ -133,7 +133,9 @@ protected function toXML(array $array, \XMLWriter $writer): void { $v = []; } - if (\is_array($v)) { + if ($k === '$comment') { + $writer->writeComment($v); + } elseif (\is_array($v)) { $writer->startElement($k); $this->toXML($v, $writer); $writer->endElement(); diff --git a/lib/private/Files/FilenameValidator.php b/lib/private/Files/FilenameValidator.php new file mode 100644 index 0000000000000..75ec68bcb401e --- /dev/null +++ b/lib/private/Files/FilenameValidator.php @@ -0,0 +1,249 @@ + + */ + private array $forbiddenNames = []; + + /** + * @var list + */ + private array $forbiddenCharacters = []; + + /** + * @var list + */ + private array $forbiddenExtensions = []; + + public function __construct( + IFactory $l10nFactory, + private IConfig $config, + private LoggerInterface $logger, + ) { + $this->l10n = $l10nFactory->get('core'); + } + + /** + * Get a list of reserved filenames that must not be used + * This list should be checked case-insensitive, all names are returned lowercase. + * @return list + * @since 30.0.0 + */ + public function getForbiddenExtensions(): array { + if (empty($this->forbiddenExtensions)) { + $forbiddenExtensions = $this->config->getSystemValue('forbidden_filename_extensions', ['.filepart']); + if (!is_array($forbiddenExtensions)) { + $this->logger->error('Invalid system config value for "forbidden_filename_extensions" is ignored.'); + $forbiddenExtensions = ['.filepart']; + } + + // Always forbid .part files as they are used internally + $forbiddenExtensions = array_merge($forbiddenExtensions, ['.part']); + + // The list is case insensitive so we provide it always lowercase + $forbiddenExtensions = array_map('mb_strtolower', $forbiddenExtensions); + $this->forbiddenExtensions = array_values($forbiddenExtensions); + } + return $this->forbiddenExtensions; + } + + /** + * Get a list of forbidden filename extensions that must not be used + * This list should be checked case-insensitive, all names are returned lowercase. + * @return list + * @since 30.0.0 + */ + public function getForbiddenFilenames(): array { + if (empty($this->forbiddenNames)) { + $forbiddenNames = $this->config->getSystemValue('forbidden_filenames', ['.htaccess']); + if (!is_array($forbiddenNames)) { + $this->logger->error('Invalid system config value for "forbidden_filenames" is ignored.'); + $forbiddenNames = ['.htaccess']; + } + + // Handle legacy config option + // TODO: Drop with Nextcloud 34 + $legacyForbiddenNames = $this->config->getSystemValue('blacklisted_files', []); + if (!is_array($legacyForbiddenNames)) { + $this->logger->error('Invalid system config value for "blacklisted_files" is ignored.'); + $legacyForbiddenNames = []; + } + if (!empty($legacyForbiddenNames)) { + $this->logger->warning('System config option "blacklisted_files" is deprecated and will be removed in Nextcloud 34, use "forbidden_filenames" instead.'); + } + $forbiddenNames = array_merge($legacyForbiddenNames, $forbiddenNames); + + // The list is case insensitive so we provide it always lowercase + $forbiddenNames = array_map('mb_strtolower', $forbiddenNames); + $this->forbiddenNames = array_values($forbiddenNames); + } + return $this->forbiddenNames; + } + + /** + * Get a list of characters forbidden in filenames + * + * Note: Characters in the range [0-31] are always forbidden, + * even if not inside this list (see OCP\Files\Storage\IStorage::verifyPath). + * + * @return list + * @since 30.0.0 + */ + public function getForbiddenCharacters(): array { + if (empty($this->forbiddenCharacters)) { + // Get always forbidden characters + $forbiddenCharacters = str_split(\OCP\Constants::FILENAME_INVALID_CHARS); + if ($forbiddenCharacters === false) { + $forbiddenCharacters = []; + } + + // Get admin defined invalid characters + $additionalChars = $this->config->getSystemValue('forbidden_chars', []); + if (!is_array($additionalChars)) { + $this->logger->error('Invalid system config value for "forbidden_chars" is ignored.'); + $additionalChars = []; + } + $forbiddenCharacters = array_merge($forbiddenCharacters, $additionalChars); + + // Handle legacy config option + // TODO: Drop with Nextcloud 34 + $legacyForbiddenCharacters = $this->config->getSystemValue('forbidden_chars', []); + if (!is_array($legacyForbiddenCharacters)) { + $this->logger->error('Invalid system config value for "forbidden_chars" is ignored.'); + $legacyForbiddenCharacters = []; + } + if (!empty($legacyForbiddenCharacters)) { + $this->logger->warning('System config option "forbidden_chars" is deprecated and will be removed in Nextcloud 34, use "forbidden_filename_characters" instead.'); + } + $forbiddenCharacters = array_merge($legacyForbiddenCharacters, $forbiddenCharacters); + + $this->forbiddenCharacters = array_values($forbiddenCharacters); + } + return $this->forbiddenCharacters; + } + + /** + * @inheritdoc + */ + public function isFilenameValid(string $filename): bool { + try { + $this->validateFilename($filename); + } catch (\OCP\Files\InvalidPathException) { + return false; + } + return true; + } + + /** + * @inheritdoc + */ + public function validateFilename(string $filename): void { + $trimmed = trim($filename); + if ($trimmed === '') { + throw new EmptyFileNameException(); + } + + // the special directories . and .. would cause never ending recursion + if ($trimmed === '.' || $trimmed === '..') { + throw new ReservedWordException(); + } + + // 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(); + } + + if ($this->isForbidden($filename)) { + throw new ReservedWordException(); + } + + $this->checkForbiddenExtension($filename); + + $this->checkForbiddenCharacters($filename); + } + + /** + * Check if the filename is forbidden + * @param string $filename + * @return bool True if invalid name, False otherwise + */ + public function isForbidden(string $filename): bool { + $filename = basename($filename); + $filename = mb_strtolower($filename); + + if ($filename === '') { + return false; + } + + // The name part without extension + $basename = substr($filename, 0, strpos($filename, '.', 1) ?: null); + // Check for forbidden filenames + $forbiddenNames = $this->getForbiddenFilenames(); + if (in_array($basename, $forbiddenNames)) { + return true; + } + + // Filename is not forbidden + return false; + } + + /** + * Check if a filename contains any of the forbidden characters + * @param string $filename + * @throws InvalidCharacterInPathException + */ + protected function checkForbiddenCharacters(string $filename): void { + $sanitizedFileName = filter_var($filename, FILTER_UNSAFE_RAW, FILTER_FLAG_STRIP_LOW); + if ($sanitizedFileName !== $filename) { + throw new InvalidCharacterInPathException(); + } + + foreach ($this->getForbiddenCharacters() as $char) { + if (str_contains($filename, $char)) { + throw new InvalidCharacterInPathException($char); + } + } + } + + /** + * Check if a filename has a forbidden filename extension + * @param string $filename The filename to validate + * @throws InvalidPathException + */ + protected function checkForbiddenExtension(string $filename): void { + $filename = mb_strtolower($filename); + // Check for forbidden filename extengetForbiddenExtensions(); + foreach ($forbiddenExtensions as $extension) { + if (str_ends_with($filename, $extension)) { + throw new InvalidPathException($this->l10n->t('Invalid filename extension "%1$s"', [$extension])); + } + } + } +}; diff --git a/lib/private/Files/Filesystem.php b/lib/private/Files/Filesystem.php index db7420c3c4cf9..f200066e7ccdc 100644 --- a/lib/private/Files/Filesystem.php +++ b/lib/private/Files/Filesystem.php @@ -29,9 +29,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() @@ -428,21 +425,6 @@ public static function isValidPath($path) { return true; } - /** - * @param string $filename - * @return bool - */ - public static function isFileBlacklisted($filename) { - $filename = self::normalizePath($filename); - - if (self::$blacklist === null) { - self::$blacklist = \OC::$server->getConfig()->getSystemValue('blacklisted_files', ['.htaccess']); - } - - $filename = strtolower(basename($filename)); - return in_array($filename, self::$blacklist); - } - /** * check if the directory should be ignored when scanning * NOTE: the special directories . and .. would cause never ending recursion diff --git a/lib/private/Files/Storage/Common.php b/lib/private/Files/Storage/Common.php index e613e435f03f6..d4bbe8e5b6f87 100644 --- a/lib/private/Files/Storage/Common.php +++ b/lib/private/Files/Storage/Common.php @@ -13,21 +13,21 @@ use OC\Files\Cache\Scanner; use OC\Files\Cache\Updater; use OC\Files\Cache\Watcher; +use OC\Files\FilenameValidator; 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\InvalidPathException; -use OCP\Files\ReservedWordException; use OCP\Files\Storage\ILockingStorage; use OCP\Files\Storage\IStorage; use OCP\Files\Storage\IWriteStreamStorage; use OCP\Files\StorageNotAvailableException; +use OCP\IDBConnection; use OCP\Lock\ILockingProvider; use OCP\Lock\LockedException; use OCP\Server; @@ -53,6 +53,7 @@ abstract class Common implements Storage, ILockingStorage, IWriteStreamStorage { protected $propagator; protected $storageCache; protected $updater; + protected ?FilenameValidator $filenameValidator; protected $mountOptions = []; protected $owner = null; @@ -491,6 +492,13 @@ public function getDirectDownload($path) { return []; } + protected function getFilenameValidator(): FilenameValidator { + if (!isset($this->filenameValidator)) { + $this->filenameValidator = \OCP\Server::get(FilenameValidator::class); + } + return $this->filenameValidator; + } + /** * @inheritdoc * @throws InvalidPathException @@ -506,7 +514,8 @@ public function verifyPath($path, $fileName) { throw new InvalidDirectoryException(); } - if (!\OC::$server->getDatabaseConnection()->supports4ByteText()) { + $connection = \OCP\Server::get(IDBConnection::class); + if (!$connection->supports4ByteText()) { // verify database - e.g. mysql only 3-byte chars if (preg_match('%(?: \xF0[\x90-\xBF][\x80-\xBF]{2} # planes 1-3 @@ -517,47 +526,10 @@ public function verifyPath($path, $fileName) { } } - // 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 - */ - 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(); - } } /** @@ -682,7 +654,9 @@ public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t * @inheritdoc */ public function getMetaData($path) { - if (Filesystem::isFileBlacklisted($path)) { + try { + $this->verifyPath($path, basename($path)); + } catch (InvalidPathException) { throw new ForbiddenException('Invalid path: ' . $path, false); } diff --git a/lib/private/Files/Storage/DAV.php b/lib/private/Files/Storage/DAV.php index 63ef1399a694b..605db888c5228 100644 --- a/lib/private/Files/Storage/DAV.php +++ b/lib/private/Files/Storage/DAV.php @@ -18,6 +18,7 @@ use OCP\Files\FileInfo; use OCP\Files\ForbiddenException; use OCP\Files\IMimeTypeDetector; +use OCP\Files\InvalidPathException; use OCP\Files\StorageInvalidException; use OCP\Files\StorageNotAvailableException; use OCP\Http\Client\IClientService; @@ -561,7 +562,9 @@ public function copy($source, $target) { } public function getMetaData($path) { - if (Filesystem::isFileBlacklisted($path)) { + try { + $this->verifyPath($path, basename($path)); + } catch (InvalidPathException) { throw new ForbiddenException('Invalid path: ' . $path, false); } $response = $this->propfind($path); diff --git a/lib/private/Files/Storage/Local.php b/lib/private/Files/Storage/Local.php index ac3696118f7ee..ce610f9d2a8e4 100644 --- a/lib/private/Files/Storage/Local.php +++ b/lib/private/Files/Storage/Local.php @@ -7,7 +7,6 @@ */ namespace OC\Files\Storage; -use OC\Files\Filesystem; use OC\Files\Storage\Wrapper\Encryption; use OC\Files\Storage\Wrapper\Jail; use OCP\Constants; @@ -44,6 +43,8 @@ public function __construct($arguments) { if (!isset($arguments['datadir']) || !is_string($arguments['datadir'])) { throw new \InvalidArgumentException('No data directory set for local storage'); } + parent::__construct($arguments); + $this->datadir = str_replace('//', '/', $arguments['datadir']); // some crazy code uses a local storage on root... if ($this->datadir === '/') { @@ -316,11 +317,13 @@ public function unlink($path) { } private function checkTreeForForbiddenItems(string $path) { - $iterator = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($path)); + $iterator = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($path, \RecursiveDirectoryIterator::SKIP_DOTS)); + /** @var \SplFileInfo $file */ foreach ($iterator as $file) { - /** @var \SplFileInfo $file */ - if (Filesystem::isFileBlacklisted($file->getBasename())) { - throw new ForbiddenException('Invalid path: ' . $file->getPathname(), false); + if (!$this->getFilenameValidator()->isFilenameValid($file->getBasename())) { + // Do not leak data dir + $filePath = substr($file->getPathname(), strlen($this->datadir)); + throw new ForbiddenException('Invalid path: ' . $filePath, false); } } } @@ -474,11 +477,17 @@ public function hasUpdated($path, $time) { * @throws ForbiddenException */ public function getSourcePath($path) { - if (Filesystem::isFileBlacklisted($path)) { + $fullPath = $this->datadir . $path; + // Special handling for getting the source path + if ($path === '') { + return $fullPath; + } + + // Validate the path for invalid access, we only check forbidden files + if ($this->getFilenameValidator()->isForbidden(basename($path))) { throw new ForbiddenException('Invalid path: ' . $path, false); } - $fullPath = $this->datadir . $path; $currentPath = $path; $allowSymlinks = $this->config->getSystemValueBool('localstorage.allowsymlinks', false); if ($allowSymlinks || $currentPath === '') { @@ -556,7 +565,7 @@ private function canDoCrossStorageMove(IStorage $sourceStorage) { // more permissions checks. && !$sourceStorage->instanceOfStorage('OCA\GroupFolders\ACL\ACLStorageWrapper') // Same for access control - && !$sourceStorage->instanceOfStorage(\OCA\FilesAccessControl\StorageWrapper::class) + && !$sourceStorage->instanceOfStorage('\OCA\FilesAccessControl\StorageWrapper') // when moving encrypted files we have to handle keys and the target might not be encrypted && !$sourceStorage->instanceOfStorage(Encryption::class); } diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 0150a3e117ab5..8c79a58eed275 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -58,6 +58,7 @@ class View { private bool $updaterEnabled = true; private UserManager $userManager; private LoggerInterface $logger; + private FilenameValidator $filenameValidator; /** * @throws \Exception If $root contains an invalid path @@ -72,6 +73,7 @@ public function __construct(string $root = '') { $this->lockingEnabled = !($this->lockingProvider instanceof \OC\Lock\NoopLockingProvider); $this->userManager = \OC::$server->getUserManager(); $this->logger = \OC::$server->get(LoggerInterface::class); + $this->filenameValidator = \OCP\Server::get(FilenameValidator::class); } /** @@ -588,7 +590,7 @@ public function file_put_contents($path, $data) { if (is_resource($data)) { //not having to deal with streams in file_put_contents makes life easier $absolutePath = Filesystem::normalizePath($this->getAbsolutePath($path)); if (Filesystem::isValidPath($path) - && !Filesystem::isFileBlacklisted($path) + && $this->filenameValidator->isFilenameValid(basename($absolutePath)) ) { $path = $this->getRelativePath($absolutePath); if ($path === null) { @@ -706,7 +708,7 @@ public function rename($source, $target) { if ( Filesystem::isValidPath($target) && Filesystem::isValidPath($source) - && !Filesystem::isFileBlacklisted($target) + && $this->filenameValidator->isFilenameValid(basename($target)) ) { $source = $this->getRelativePath($absolutePath1); $target = $this->getRelativePath($absolutePath2); @@ -843,7 +845,7 @@ public function copy($source, $target, $preserveMtime = false) { if ( Filesystem::isValidPath($target) && Filesystem::isValidPath($source) - && !Filesystem::isFileBlacklisted($target) + && $this->filenameValidator->isFilenameValid(basename($target)) ) { $source = $this->getRelativePath($absolutePath1); $target = $this->getRelativePath($absolutePath2); @@ -1099,9 +1101,8 @@ public function free_space($path = '/') { private function basicOperation(string $operation, string $path, array $hooks = [], $extraParam = null) { $postFix = (substr($path, -1) === '/') ? '/' : ''; $absolutePath = Filesystem::normalizePath($this->getAbsolutePath($path)); - if (Filesystem::isValidPath($path) - && !Filesystem::isFileBlacklisted($path) - ) { + $filename = basename($absolutePath); + if (Filesystem::isValidPath($path) && !$this->filenameValidator->isForbidden($filename)) { $path = $this->getRelativePath($absolutePath); if ($path == null) { return false; diff --git a/lib/private/Security/CertificateManager.php b/lib/private/Security/CertificateManager.php index f9fd2b160b8ad..c19579dd052ec 100644 --- a/lib/private/Security/CertificateManager.php +++ b/lib/private/Security/CertificateManager.php @@ -10,6 +10,7 @@ use OC\Files\Filesystem; use OC\Files\View; +use OCP\Files\IFilenameValidator; use OCP\ICertificate; use OCP\ICertificateManager; use OCP\IConfig; @@ -27,6 +28,7 @@ public function __construct( protected IConfig $config, protected LoggerInterface $logger, protected ISecureRandom $random, + protected IFilenameValidator $filenameValidator, ) { } @@ -150,7 +152,7 @@ public function createCertificateBundle(): void { * @throws \Exception If the certificate could not get added */ public function addCertificate(string $certificate, string $name): ICertificate { - if (!Filesystem::isValidPath($name) or Filesystem::isFileBlacklisted($name)) { + if (!$this->filenameValidator->isFilenameValid($name)) { throw new \Exception('Filename is not valid'); } $this->bundlePath = null; diff --git a/lib/private/Server.php b/lib/private/Server.php index bcdf482f02d0e..795d72e307629 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -1371,6 +1371,8 @@ public function __construct($webRoot, \OC\Config $config) { $this->registerAlias(\OCP\Files\AppData\IAppDataFactory::class, \OC\Files\AppData\Factory::class); + $this->registerAlias(\OCP\Files\IFilenameValidator::class, \OC\Files\FilenameValidator::class); + $this->registerAlias(IBinaryFinder::class, BinaryFinder::class); $this->registerAlias(\OCP\Share\IPublicShareTemplateFactory::class, \OC\Share20\PublicShareTemplateFactory::class); diff --git a/lib/private/Template/JSConfigHelper.php b/lib/private/Template/JSConfigHelper.php index a41e99ae8c484..024989aacb9fc 100644 --- a/lib/private/Template/JSConfigHelper.php +++ b/lib/private/Template/JSConfigHelper.php @@ -10,6 +10,7 @@ use bantu\IniGetWrapper\IniGetWrapper; use OC\Authentication\Token\IProvider; use OC\CapabilitiesManager; +use OC\Files\FilenameValidator; use OC\Share\Share; use OCP\App\AppPathNotFoundException; use OCP\App\IAppManager; @@ -51,6 +52,7 @@ public function __construct( protected CapabilitiesManager $capabilitiesManager, protected IInitialStateService $initialStateService, protected IProvider $tokenProvider, + protected FilenameValidator $filenameValidator, ) { } @@ -133,8 +135,13 @@ public function getConfig(): string { $config = [ 'auto_logout' => $this->config->getSystemValue('auto_logout', false), + /** @deprecated 30.0.0 */ 'blacklist_files_regex' => FileInfo::BLACKLIST_FILES_REGEX, - 'forbidden_filename_characters' => Util::getForbiddenFileNameChars(), + + 'forbidden_filenames' => $this->filenameValidator->getForbiddenFilenames(), + 'forbidden_filename_characters' => $this->filenameValidator->getForbiddenCharacters(), + 'forbidden_filename_extensions' => $this->filenameValidator->getForbiddenExtensions(), + 'loglevel' => $this->config->getSystemValue('loglevel_frontend', $this->config->getSystemValue('loglevel', ILogger::WARN) ), diff --git a/lib/private/TemplateLayout.php b/lib/private/TemplateLayout.php index 27e988711befc..c1c99c88f2bcb 100644 --- a/lib/private/TemplateLayout.php +++ b/lib/private/TemplateLayout.php @@ -9,6 +9,7 @@ use bantu\IniGetWrapper\IniGetWrapper; use OC\Authentication\Token\IProvider; +use OC\Files\FilenameValidator; use OC\Search\SearchQuery; use OC\Template\CSSResourceLocator; use OC\Template\JSConfigHelper; @@ -228,6 +229,7 @@ public function __construct($renderAs, $appId = '') { \OC::$server->get(CapabilitiesManager::class), \OCP\Server::get(IInitialStateService::class), \OCP\Server::get(IProvider::class), + \OCP\Server::get(FilenameValidator::class), ); $config = $jsConfigHelper->getConfig(); if (\OC::$server->getContentSecurityPolicyNonceManager()->browserSupportsCspV3()) { diff --git a/lib/private/legacy/OC_Helper.php b/lib/private/legacy/OC_Helper.php index 0c913709111a8..858ed2d28936a 100644 --- a/lib/private/legacy/OC_Helper.php +++ b/lib/private/legacy/OC_Helper.php @@ -6,6 +6,7 @@ * SPDX-License-Identifier: AGPL-3.0-only */ use bantu\IniGetWrapper\IniGetWrapper; +use OC\Files\FilenameValidator; use OC\Files\Filesystem; use OCP\Files\Mount\IMountPoint; use OCP\IBinaryFinder; @@ -126,8 +127,11 @@ public static function copyr($src, $dest) { self::copyr("$src/$file", "$dest/$file"); } } - } elseif (file_exists($src) && !\OC\Files\Filesystem::isFileBlacklisted($src)) { - copy($src, $dest); + } elseif (file_exists($src)) { + $validator = \OCP\Server::get(FilenameValidator::class); + if (!$validator->isForbidden(\basename($src))) { + copy($src, $dest); + } } } @@ -499,7 +503,7 @@ public static function getStorageInfo($path, $rootInfo = null, $includeMountPoin // TODO: need a better way to get total space from storage if ($sourceStorage->instanceOfStorage('\OC\Files\Storage\Wrapper\Quota')) { - /** @var \OC\Files\Storage\Wrapper\Quota $storage */ + /** @var \OC\Files\Storage\Wrapper\Quota $sourceStorage */ $quota = $sourceStorage->getQuota(); } try { diff --git a/lib/private/legacy/OC_Util.php b/lib/private/legacy/OC_Util.php index b7836e86d7b7b..d8045e8343d74 100644 --- a/lib/private/legacy/OC_Util.php +++ b/lib/private/legacy/OC_Util.php @@ -1036,35 +1036,6 @@ public static function getHumanVersion() { 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 diff --git a/lib/public/Files/FileInfo.php b/lib/public/Files/FileInfo.php index 468013ac271ac..53380acb8bc8c 100644 --- a/lib/public/Files/FileInfo.php +++ b/lib/public/Files/FileInfo.php @@ -48,6 +48,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\Files\Storage\IStorage::verifyPath() */ public const BLACKLIST_FILES_REGEX = '\.(part|filepart)$'; diff --git a/lib/public/Files/IFilenameValidator.php b/lib/public/Files/IFilenameValidator.php new file mode 100644 index 0000000000000..2bd3bb945dcab --- /dev/null +++ b/lib/public/Files/IFilenameValidator.php @@ -0,0 +1,39 @@ +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 OCP\Files\Storage\IStorage::verifyPath() - * @since 7.0.0 - * @suppress PhanDeprecatedFunction - */ - public static function isValidFileName($file) { - return \OC_Util::isValidFileName($file); - } - /** * Compare two strings to provide a natural sort * @param string $a first string to compare diff --git a/tests/lib/Files/FilenameValidatorTest.php b/tests/lib/Files/FilenameValidatorTest.php new file mode 100644 index 0000000000000..ec67e208b919b --- /dev/null +++ b/tests/lib/Files/FilenameValidatorTest.php @@ -0,0 +1,188 @@ +createMock(IL10N::class); + $l10n->method('t') + ->willReturnCallback(fn ($string, $params) => sprintf($string, ...$params)); + $this->l10n = $this->createMock(IFactory::class); + $this->l10n + ->method('get') + ->with('core') + ->willReturn($l10n); + + $this->config = $this->createMock(IConfig::class); + $this->logger = $this->createMock(LoggerInterface::class); + } + + /** + * @dataProvider dataValidateFilename + */ + public function testValidateFilename( + string $filename, + array $forbiddenNames, + array $forbiddenExtensions, + array $forbiddenCharacters, + ?string $exception, + ): void { + /** @var FilenameValidator&MockObject */ + $validator = $this->getMockBuilder(FilenameValidator::class) + ->onlyMethods(['getForbiddenExtensions', 'getForbiddenFilenames', 'getForbiddenCharacters']) + ->setConstructorArgs([$this->l10n, $this->config, $this->logger]) + ->getMock(); + + $validator->method('getForbiddenCharacters') + ->willReturn($forbiddenCharacters); + $validator->method('getForbiddenExtensions') + ->willReturn($forbiddenExtensions); + $validator->method('getForbiddenFilenames') + ->willReturn($forbiddenNames); + + if ($exception !== null) { + $this->expectException($exception); + } else { + $this->expectNotToPerformAssertions(); + } + $validator->validateFilename($filename); + } + + /** + * @dataProvider dataValidateFilename + */ + public function testIsFilenameValid( + string $filename, + array $forbiddenNames, + array $forbiddenExtensions, + array $forbiddenCharacters, + ?string $exception, + ): void { + /** @var FilenameValidator&MockObject */ + $validator = $this->getMockBuilder(FilenameValidator::class) + ->onlyMethods(['getForbiddenExtensions', 'getForbiddenFilenames', 'getForbiddenCharacters']) + ->setConstructorArgs([$this->l10n, $this->config, $this->logger]) + ->getMock(); + + $validator->method('getForbiddenCharacters') + ->willReturn($forbiddenCharacters); + $validator->method('getForbiddenExtensions') + ->willReturn($forbiddenExtensions); + $validator->method('getForbiddenFilenames') + ->willReturn($forbiddenNames); + + + $this->assertEquals($exception === null, $validator->isFilenameValid($filename)); + } + + public function dataValidateFilename(): array { + return [ + 'valid name' => [ + 'a: b.txt', ['.htaccess'], [], [], null + ], + 'valid name with some more parameters' => [ + 'a: b.txt', ['.htaccess'], ['exe'], ['~'], null + ], + 'forbidden name' => [ + '.htaccess', ['.htaccess'], [], [], ReservedWordException::class + ], + 'forbidden name - name is case insensitive' => [ + 'COM1', ['.htaccess', 'com1'], [], [], ReservedWordException::class + ], + 'forbidden name - name checks the filename' => [ + // needed for Windows namespaces + 'com1.suffix', ['.htaccess', 'com1'], [], [], ReservedWordException::class + ], + 'invalid character' => [ + 'a: b.txt', ['.htaccess'], [], [':'], InvalidCharacterInPathException::class + ], + 'invalid path' => [ + '../../foo.bar', ['.htaccess'], [], ['/', '\\'], InvalidCharacterInPathException::class, + ], + 'invalid extension' => [ + 'a: b.txt', ['.htaccess'], ['.txt'], [], InvalidPathException::class + ], + 'empty filename' => [ + '', [], [], [], EmptyFileNameException::class + ], + 'reserved unix name "."' => [ + '.', [], [], [], InvalidPathException::class + ], + 'reserved unix name ".."' => [ + '..', [], [], [], ReservedWordException::class + ], + 'too long filename "."' => [ + str_repeat('a', 251), [], [], [], FileNameTooLongException::class + ], + // 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 + ['🌫️.txt', ['.htaccess'], [], ['😶‍🌫️'], null], + // This is the reverse: clouds are forbidden -> so is also the face in the clouds emoji + ['😶‍🌫️.txt', ['.htaccess'], [], ['🌫️'], InvalidCharacterInPathException::class], + ]; + } + + /** + * @dataProvider dataIsForbidden + */ + public function testIsForbidden(string $filename, array $forbiddenNames, bool $expected): void { + /** @var FilenameValidator&MockObject */ + $validator = $this->getMockBuilder(FilenameValidator::class) + ->onlyMethods(['getForbiddenFilenames']) + ->setConstructorArgs([$this->l10n, $this->config, $this->logger]) + ->getMock(); + + $validator->method('getForbiddenFilenames') + ->willReturn($forbiddenNames); + + + $this->assertEquals($expected, $validator->isFilenameValid($filename)); + } + + public function dataIsForbidden(): array { + return [ + 'valid name' => [ + 'a: b.txt', ['.htaccess'], true + ], + 'valid name with some more parameters' => [ + 'a: b.txt', ['.htaccess'], true + ], + 'forbidden name' => [ + '.htaccess', ['.htaccess'], false + ], + 'forbidden name - name is case insensitive' => [ + 'COM1', ['.htaccess', 'com1'], false + ], + 'forbidden name - name checks the filename' => [ + // needed for Windows namespaces + 'com1.suffix', ['.htaccess', 'com1'], false + ], + ]; + } +} diff --git a/tests/lib/Files/FilesystemTest.php b/tests/lib/Files/FilesystemTest.php index 4ab0456a81033..603cd2ebddb18 100644 --- a/tests/lib/Files/FilesystemTest.php +++ b/tests/lib/Files/FilesystemTest.php @@ -258,28 +258,6 @@ public function testIsValidPath($path, $expected) { $this->assertSame($expected, \OC\Files\Filesystem::isValidPath($path)); } - public function isFileBlacklistedData() { - return [ - ['/etc/foo/bar/foo.txt', false], - ['\etc\foo/bar\foo.txt', false], - ['.htaccess', true], - ['.htaccess/', true], - ['.htaccess\\', true], - ['/etc/foo\bar/.htaccess\\', true], - ['/etc/foo\bar/.htaccess/', true], - ['/etc/foo\bar/.htaccess/foo', false], - ['//foo//bar/\.htaccess/', true], - ['\foo\bar\.HTAccess', true], - ]; - } - - /** - * @dataProvider isFileBlacklistedData - */ - public function testIsFileBlacklisted($path, $expected) { - $this->assertSame($expected, \OC\Files\Filesystem::isFileBlacklisted($path)); - } - public function testNormalizePathUTF8() { if (!class_exists('Patchwork\PHP\Shim\Normalizer')) { $this->markTestSkipped('UTF8 normalizer Patchwork was not found'); diff --git a/tests/lib/Files/PathVerificationTest.php b/tests/lib/Files/PathVerificationTest.php index 4addd5833549e..d56ee9164c7c2 100644 --- a/tests/lib/Files/PathVerificationTest.php +++ b/tests/lib/Files/PathVerificationTest.php @@ -115,12 +115,16 @@ public function testPathVerificationInvalidCharsPosix($fileName) { $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 +156,6 @@ public function providesInvalidCharsPosix() { [\chr(29)], [\chr(30)], [\chr(31)], - ['/'], - ['\\'], ]; } diff --git a/tests/lib/Files/Storage/CommonTest.php b/tests/lib/Files/Storage/CommonTest.php index bd22d93fb69d9..f0b5794b3e96e 100644 --- a/tests/lib/Files/Storage/CommonTest.php +++ b/tests/lib/Files/Storage/CommonTest.php @@ -9,7 +9,12 @@ use OC\Files\Storage\Wrapper\Jail; use OC\Files\Storage\Wrapper\Wrapper; +use OCP\Files\EmptyFileNameException; +use OCP\Files\FileNameTooLongException; +use OCP\Files\InvalidCharacterInPathException; use OCP\Files\InvalidPathException; +use OCP\Files\ReservedWordException; +use OCP\ITempManager; use PHPUnit\Framework\MockObject\MockObject; /** @@ -18,6 +23,7 @@ * @group DB * * @package Test\Files\Storage + * @backupStaticAttributes enabled */ class CommonTest extends Storage { /** @@ -25,26 +31,22 @@ class CommonTest extends Storage { */ private $tmpDir; - private array $invalidCharsBackup; - protected function setUp(): void { parent::setUp(); - $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', []); } 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) { + public function testVerifyPath(string $filename, ?string $exception, bool $throws) { /** @var \OC\Files\Storage\CommonTest|MockObject $instance */ $instance = $this->getMockBuilder(\OC\Files\Storage\CommonTest::class) ->onlyMethods(['copyFromStorage', 'rmdir', 'unlink']) @@ -53,7 +55,12 @@ public function testVerifyPath(string $filename, array $additionalChars, bool $t $instance->method('copyFromStorage') ->willThrowException(new \Exception('copy')); - \OC::$server->getConfig()->setSystemValue('forbidden_chars', $additionalChars); + if ($exception !== null) { + $this->filenameValidator->expects($this->any()) + ->method('validateFilename') + ->with($filename) + ->willThrowException(new $exception()); + } if ($throws) { $this->expectException(InvalidPathException::class); @@ -65,19 +72,14 @@ public function testVerifyPath(string $filename, array $additionalChars, bool $t 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], + ['a/b.txt', InvalidCharacterInPathException::class, true], + ['', EmptyFileNameException::class, true], + ['verylooooooong.txt', FileNameTooLongException::class, true], + ['COM1', ReservedWordException::class, true], + ['a/b.txt', InvalidCharacterInPathException::class, true], + + ['a: b.txt', null, false], + ['🌫️.txt', null, false], ]; } diff --git a/tests/lib/Files/Storage/Storage.php b/tests/lib/Files/Storage/Storage.php index 0a170a0e7d53b..38b4b29bede46 100644 --- a/tests/lib/Files/Storage/Storage.php +++ b/tests/lib/Files/Storage/Storage.php @@ -8,7 +8,9 @@ namespace Test\Files\Storage; use OC\Files\Cache\Watcher; +use OC\Files\FilenameValidator; use OCP\Files\Storage\IWriteStreamStorage; +use PHPUnit\Framework\MockObject\MockObject; abstract class Storage extends \Test\TestCase { /** @@ -17,6 +19,22 @@ abstract class Storage extends \Test\TestCase { protected $instance; protected $waitDelay = 0; + protected FilenameValidator&MockObject $filenameValidator; + + protected function setUp(): void { + $this->filenameValidator = $this->createMock(FilenameValidator::class); + $this->filenameValidator->method('isFilenameValid')->willReturn(true); + $this->filenameValidator->method('isForbidden')->willReturn(false); + $this->overwriteService(FilenameValidator::class, $this->filenameValidator); + + parent::setUp(); + } + + protected function tearDown(): void { + $this->restoreService(FilenameValidator::class); + parent::tearDown(); + } + /** * Sleep for the number of seconds specified in the * $waitDelay attribute diff --git a/tests/lib/Security/CertificateManagerTest.php b/tests/lib/Security/CertificateManagerTest.php index fc5ae1f03f239..fcbac7840bd4a 100644 --- a/tests/lib/Security/CertificateManagerTest.php +++ b/tests/lib/Security/CertificateManagerTest.php @@ -12,8 +12,10 @@ use OC\Files\View; use OC\Security\CertificateManager; +use OCP\Files\IFilenameValidator; use OCP\IConfig; use OCP\Security\ISecureRandom; +use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; /** @@ -25,12 +27,10 @@ class CertificateManagerTest extends \Test\TestCase { use \Test\Traits\UserTrait; use \Test\Traits\MountProviderTrait; - /** @var CertificateManager */ - private $certificateManager; - /** @var String */ - private $username; - /** @var ISecureRandom */ - private $random; + private string $username; + private CertificateManager $certificateManager; + private ISecureRandom&MockObject $random; + private IFilenameValidator&MockObject $filenameValidator; protected function setUp(): void { parent::setUp(); @@ -54,11 +54,19 @@ protected function setUp(): void { $this->random->method('generate') ->willReturn('random'); + $this->filenameValidator = $this->createMock(IFilenameValidator::class); + $this->filenameValidator->method('isFilenameValid') + ->willReturnCallback(fn ($name) => match($name) { + '.htaccess' => false, + default => true, + }); + $this->certificateManager = new CertificateManager( new \OC\Files\View(), $config, $this->createMock(LoggerInterface::class), - $this->random + $this->random, + $this->filenameValidator, ); } @@ -101,26 +109,14 @@ public function testAddInvalidCertificate() { $this->certificateManager->addCertificate('InvalidCertificate', 'invalidCertificate'); } - /** - * @return array - */ - public function dangerousFileProvider() { - return [ - ['.htaccess'], - ['../../foo.txt'], - ['..\..\foo.txt'], - ]; - } + public function testAddDangerousFile() { + $this->filenameValidator->expects($this->once()) + ->method('isFilenameValid'); - /** - * @dataProvider dangerousFileProvider - * @param string $filename - */ - public function testAddDangerousFile($filename) { $this->expectException(\Exception::class); $this->expectExceptionMessage('Filename is not valid'); - $this->certificateManager->addCertificate(file_get_contents(__DIR__ . '/../../data/certificates/expiredCertificate.crt'), $filename); + $this->certificateManager->addCertificate(file_get_contents(__DIR__ . '/../../data/certificates/expiredCertificate.crt'), '.htaccess'); } public function testRemoveDangerousFile() { @@ -155,7 +151,7 @@ public function testNeedRebundling($CaBundleMtime, /** @var CertificateManager | \PHPUnit\Framework\MockObject\MockObject $certificateManager */ $certificateManager = $this->getMockBuilder('OC\Security\CertificateManager') - ->setConstructorArgs([$view, $config, $this->createMock(LoggerInterface::class), $this->random]) + ->setConstructorArgs([$view, $config, $this->createMock(LoggerInterface::class), $this->random, $this->filenameValidator]) ->setMethods(['getFilemtimeOfCaBundle', 'getCertificateBundle']) ->getMock(); diff --git a/tests/lib/UtilTest.php b/tests/lib/UtilTest.php index 82897cbca298d..a83f27d5cf7e2 100644 --- a/tests/lib/UtilTest.php +++ b/tests/lib/UtilTest.php @@ -137,64 +137,6 @@ public function testGetInstanceIdGeneratesValidId() { $this->assertSame(1, $matchesRegex); } - /** - * @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)); - } - - public function filenameValidationProvider() { - return [ - // valid names - ['boringname', true], - ['something.with.extension', true], - ['now with spaces', true], - ['.a', true], - ['..a', true], - ['.dotfile', true], - ['single\'quote', true], - [' spaces before', true], - ['spaces after ', true], - ['allowed chars including the crazy ones $%&_-^@!,()[]{}=;#', true], - ['汉字也能用', true], - ['und Ümläüte sind auch willkommen', true], - // disallowed names - ['', false], - [' ', false], - ['.', false], - ['..', false], - ['back\\slash', false], - ['sl/ash', false], - ['ltgt', true], - ['col:on', true], - ['double"quote', true], - ['pi|pe', true], - ['dont?ask?questions?', true], - ['super*star', true], - ['new\nline', false], - - // better disallow these to avoid unexpected trimming to have side effects - [' ..', false], - ['.. ', false], - ['. ', false], - [' .', false], - - // part files not allowed - ['.part', false], - ['notallowed.part', false], - ['neither.filepart', false], - - // part in the middle is ok - ['super movie part one.mkv', true], - ['super.movie.part.mkv', true], - ]; - } - /** * Test needUpgrade() when the core version is increased */