Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add getForbiddenFilenames to \OCP\Util and unify filename sanitizing #45151

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions apps/dav/lib/Connector/Sabre/Node.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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());
}
Expand Down
2 changes: 1 addition & 1 deletion apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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], []],
];
}

Expand Down
7 changes: 3 additions & 4 deletions apps/dav/tests/unit/Connector/Sabre/FileTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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 */
Expand Down
7 changes: 4 additions & 3 deletions apps/dav/tests/unit/Connector/Sabre/ObjectTreeTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -205,19 +206,19 @@ 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')
->willReturnCallback(function ($path) use ($storage) {
return [$storage, ltrim($path, '/')];
});

/** @var Directory&MockObject */
$rootNode = $this->getMockBuilder(Directory::class)
->disableOriginalConstructor()
->getMock();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -59,6 +60,7 @@ public function __construct(
private IConfig $config,
private Manager $externalShareManager,
private LoggerInterface $logger,
private IFilenameValidator $filenameValidator,
) {
}

Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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(),
Expand Down
19 changes: 11 additions & 8 deletions apps/files/lib/Capabilities.php
Original file line number Diff line number Diff line change
Expand Up @@ -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<mixed>, forbidden_filename_characters: array<string>}}
* @return array{files: array{$comment: string, bigfilechunking: bool, blacklisted_files: array<mixed>, forbidden_filenames: list<string>, forbidden_filename_characters: list<string>, forbidden_filename_extensions: list<string>}}
*/
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(),
],
];
}
Expand Down
4 changes: 0 additions & 4 deletions apps/files/lib/Controller/ViewController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
19 changes: 2 additions & 17 deletions apps/files/src/components/FileEntry/FileEntryName.vue
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -54,8 +53,6 @@ import { useNavigation } from '../../composables/useNavigation'
import { useRenamingStore } from '../../store/renaming.ts'
import logger from '../../logger.js'
const forbiddenCharacters = loadState<string[]>('files', 'forbiddenCharacters', [])
export default defineComponent({
name: 'FileEntryName',
Expand Down Expand Up @@ -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
},
Expand Down
2 changes: 1 addition & 1 deletion apps/files/src/views/FilesList.vue
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ export default defineComponent({
const { currentView } = useNavigation()
const enableGridView = (loadState('core', 'config', [])['enable_non-accessible_features'] ?? true)
const forbiddenCharacters = loadState<string[]>('files', 'forbiddenCharacters', [])
const forbiddenCharacters = (window as unknown as { '_oc_config': { 'forbidden_filename_characters': string }})._oc_config.forbidden_filename_characters
return {
currentView,
Expand Down
4 changes: 3 additions & 1 deletion build/integration/features/bootstrap/CapabilitiesContext.php
Original file line number Diff line number Diff line change
Expand Up @@ -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']);
Expand Down
24 changes: 18 additions & 6 deletions config/config.sample.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 2 additions & 4 deletions core/Controller/AvatarController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
5 changes: 4 additions & 1 deletion core/Controller/OCJSController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -44,6 +45,7 @@ public function __construct(
CapabilitiesManager $capabilitiesManager,
IInitialStateService $initialStateService,
IProvider $tokenProvider,
FilenameValidator $filenameValidator,
) {
parent::__construct($appName, $request);

Expand All @@ -59,7 +61,8 @@ public function __construct(
$urlGenerator,
$capabilitiesManager,
$initialStateService,
$tokenProvider
$tokenProvider,
$filenameValidator,
);
}

Expand Down
2 changes: 2 additions & 0 deletions lib/composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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',
Expand Down
2 changes: 2 additions & 0 deletions lib/composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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',
Expand Down
4 changes: 3 additions & 1 deletion lib/private/AppFramework/OCS/BaseResponse.php
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Loading
Loading