Skip to content

Commit

Permalink
Merge pull request #43775 from nextcloud/enforce/forbidden_chars
Browse files Browse the repository at this point in the history
  • Loading branch information
skjnldsv authored Feb 28, 2024
2 parents 281c8a4 + 1017f4f commit be98ea3
Show file tree
Hide file tree
Showing 7 changed files with 92 additions and 13 deletions.
5 changes: 3 additions & 2 deletions apps/files/lib/Capabilities.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,14 @@ public function __construct(IConfig $config) {
/**
* Return this classes capabilities
*
* @return array{files: array{bigfilechunking: bool, blacklisted_files: array<mixed>}}
* @return array{files: array{bigfilechunking: bool, blacklisted_files: array<mixed>, forbidden_filename_characters: array<string>}}
*/
public function getCapabilities() {
return [
'files' => [
'bigfilechunking' => true,
'blacklisted_files' => (array)$this->config->getSystemValue('blacklisted_files', ['.htaccess'])
'blacklisted_files' => (array)$this->config->getSystemValue('blacklisted_files', ['.htaccess']),
'forbidden_filename_characters' => \OCP\Util::getForbiddenFileNameChars(),
],
];
}
Expand Down
7 changes: 7 additions & 0 deletions apps/files/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
"required": [
"bigfilechunking",
"blacklisted_files",
"forbidden_filename_characters",
"directEditing"
],
"properties": {
Expand All @@ -43,6 +44,12 @@
"type": "object"
}
},
"forbidden_filename_characters": {
"type": "array",
"items": {
"type": "string"
}
},
"directEditing": {
"type": "object",
"required": [
Expand Down
2 changes: 2 additions & 0 deletions config/config.sample.php
Original file line number Diff line number Diff line change
Expand Up @@ -1962,6 +1962,8 @@
/**
* Blacklist characters from being used in filenames. This is useful if you
* have a filesystem or OS which does not support certain characters like windows.
*
* The '/' and '\' characters are always forbidden.
*
* Example for windows systems: ``array('?', '<', '>', ':', '*', '|', '"', chr(0), "\n", "\r")``
* see https://en.wikipedia.org/wiki/Comparison_of_file_systems#Limits
Expand Down
12 changes: 7 additions & 5 deletions lib/private/Files/Storage/Common.php
Original file line number Diff line number Diff line change
Expand Up @@ -567,7 +567,9 @@ public function verifyPath($path, $fileName) {
* @throws InvalidPathException
*/
protected function verifyPosixPath($fileName) {
$this->scanForInvalidCharacters($fileName, "\\/");
$invalidChars = \OCP\Util::getForbiddenFileNameChars();
$this->scanForInvalidCharacters($fileName, $invalidChars);

$fileName = trim($fileName);
$reservedNames = ['*'];
if (in_array($fileName, $reservedNames)) {
Expand All @@ -577,11 +579,11 @@ protected function verifyPosixPath($fileName) {

/**
* @param string $fileName
* @param string $invalidChars
* @param string[] $invalidChars
* @throws InvalidPathException
*/
private function scanForInvalidCharacters($fileName, $invalidChars) {
foreach (str_split($invalidChars) as $char) {
private function scanForInvalidCharacters(string $fileName, array $invalidChars) {
foreach ($invalidChars as $char) {
if (str_contains($fileName, $char)) {
throw new InvalidCharacterInPathException();
}
Expand Down Expand Up @@ -668,7 +670,7 @@ public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t
private function isSameStorage(IStorage $storage): bool {
while ($storage->instanceOfStorage(Wrapper::class)) {
/**
* @var Wrapper $sourceStorage
* @var Wrapper $storage
*/
$storage = $storage->getWrapperStorage();
}
Expand Down
4 changes: 2 additions & 2 deletions lib/private/legacy/OC_Util.php
Original file line number Diff line number Diff line change
Expand Up @@ -1112,8 +1112,8 @@ public static function isValidFileName($file) {
return false;
}

foreach (str_split($trimmed) as $char) {
if (str_contains(\OCP\Constants::FILENAME_INVALID_CHARS, $char)) {
foreach (\OCP\Util::getForbiddenFileNameChars() as $char) {
if (str_contains($trimmed, $char)) {
return false;
}
}
Expand Down
24 changes: 23 additions & 1 deletion lib/public/Util.php
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
use OC\AppScriptSort;
use OCP\Share\IManager;
use Psr\Container\ContainerExceptionInterface;
use Psr\Log\LoggerInterface;

/**
* This class provides different helper functions to make the life of a developer easier
Expand Down Expand Up @@ -520,11 +521,32 @@ public static function uploadLimit(): int|float {
return \OC_Helper::uploadLimit();
}

/**
* Get a list of characters forbidden in file names
* @return string[]
* @since 29.0.0
*/
public static function getForbiddenFileNameChars(): array {
// Get always forbidden characters
$invalidChars = str_split(\OCP\Constants::FILENAME_INVALID_CHARS);
if ($invalidChars === false) {
$invalidChars = [];
}

// Get admin defined invalid characters
$additionalChars = \OCP\Server::get(IConfig::class)->getSystemValue('forbidden_chars', []);
if (!is_array($additionalChars)) {
\OCP\Server::get(LoggerInterface::class)->error('Invalid system config value for "forbidden_chars" is ignored.');
$additionalChars = [];
}
return array_merge($invalidChars, $additionalChars);
}

/**
* Returns whether the given file name is valid
* @param string $file file name to check
* @return bool true if the file name is valid, false otherwise
* @deprecated 8.1.0 use \OC\Files\View::verifyPath()
* @deprecated 8.1.0 use OCP\Files\Storage\IStorage::verifyPath()
* @since 7.0.0
* @suppress PhanDeprecatedFunction
*/
Expand Down
51 changes: 48 additions & 3 deletions tests/lib/Files/Storage/CommonTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

use OC\Files\Storage\Wrapper\Jail;
use OC\Files\Storage\Wrapper\Wrapper;
use OCP\Files\InvalidPathException;
use PHPUnit\Framework\MockObject\MockObject;

/**
Expand All @@ -39,22 +40,66 @@ class CommonTest extends Storage {
*/
private $tmpDir;

private array $invalidCharsBackup;

protected function setUp(): void {
parent::setUp();

$this->tmpDir = \OC::$server->getTempManager()->getTemporaryFolder();
$this->instance = new \OC\Files\Storage\CommonTest(['datadir' => $this->tmpDir]);
$this->invalidCharsBackup = \OC::$server->getConfig()->getSystemValue('forbidden_chars', []);
}

protected function tearDown(): void {
\OC_Helper::rmdirr($this->tmpDir);
\OC::$server->getConfig()->setSystemValue('forbidden_chars', $this->invalidCharsBackup);
parent::tearDown();
}

/**
* @dataProvider dataVerifyPath
*/
public function testVerifyPath(string $filename, array $additionalChars, bool $throws) {
/** @var \OC\Files\Storage\CommonTest|MockObject $instance */
$instance = $this->getMockBuilder(\OC\Files\Storage\CommonTest::class)
->onlyMethods(['copyFromStorage', 'rmdir', 'unlink'])
->setConstructorArgs([['datadir' => $this->tmpDir]])
->getMock();
$instance->method('copyFromStorage')
->willThrowException(new \Exception('copy'));

\OC::$server->getConfig()->setSystemValue('forbidden_chars', $additionalChars);

if ($throws) {
$this->expectException(InvalidPathException::class);
} else {
$this->expectNotToPerformAssertions();
}
$instance->verifyPath('/', $filename);
}

public function dataVerifyPath(): array {
return [
// slash is always forbidden
'invalid slash' => ['a/b.txt', [], true],
// backslash is also forbidden
'invalid backslash' => ['a\\b.txt', [], true],
// by default colon is not forbidden
'valid name' => ['a: b.txt', [], false],
// colon can be added to the list of forbidden character
'invalid custom character' => ['a: b.txt', [':'], true],
// make sure to not split the list entries as they migh contain Unicode sequences
// in this example the "face in clouds" emoji contains the clouds emoji so only having clouds is ok
'valid unicode sequence' => ['🌫️.txt', ['😶‍🌫️'], false],
// This is the reverse: clouds are forbidden -> so is also the face in the clouds emoji
'valid unicode sequence' => ['😶‍🌫️.txt', ['🌫️'], true],
];
}

public function testMoveFromStorageWrapped() {
/** @var \OC\Files\Storage\CommonTest|MockObject $instance */
$instance = $this->getMockBuilder(\OC\Files\Storage\CommonTest::class)
->setMethods(['copyFromStorage', 'rmdir', 'unlink'])
->onlyMethods(['copyFromStorage', 'rmdir', 'unlink'])
->setConstructorArgs([['datadir' => $this->tmpDir]])
->getMock();
$instance->method('copyFromStorage')
Expand All @@ -72,7 +117,7 @@ public function testMoveFromStorageWrapped() {
public function testMoveFromStorageJailed() {
/** @var \OC\Files\Storage\CommonTest|MockObject $instance */
$instance = $this->getMockBuilder(\OC\Files\Storage\CommonTest::class)
->setMethods(['copyFromStorage', 'rmdir', 'unlink'])
->onlyMethods(['copyFromStorage', 'rmdir', 'unlink'])
->setConstructorArgs([['datadir' => $this->tmpDir]])
->getMock();
$instance->method('copyFromStorage')
Expand All @@ -95,7 +140,7 @@ public function testMoveFromStorageJailed() {
public function testMoveFromStorageNestedJail() {
/** @var \OC\Files\Storage\CommonTest|MockObject $instance */
$instance = $this->getMockBuilder(\OC\Files\Storage\CommonTest::class)
->setMethods(['copyFromStorage', 'rmdir', 'unlink'])
->onlyMethods(['copyFromStorage', 'rmdir', 'unlink'])
->setConstructorArgs([['datadir' => $this->tmpDir]])
->getMock();
$instance->method('copyFromStorage')
Expand Down

0 comments on commit be98ea3

Please sign in to comment.