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.

Sign up for GitHub

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

Jump to bottom

feat: Add forbidden_filename_basenames config option #46545

Merged
merged 1 commit into from
Jul 16, 2024
Merged

feat: Add forbidden_filename_basenames config option #46545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion apps/files/lib/Capabilities.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,15 @@ public function __construct(
/**
* Return this classes capabilities
*
* @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>}}
* @return array{files: array{'$comment': ?string, bigfilechunking: bool, blacklisted_files: array<mixed>, forbidden_filenames: list<string>, forbidden_filename_basenames: list<string>, forbidden_filename_characters: list<string>, forbidden_filename_extensions: list<string>}}
*/
public function getCapabilities(): array {
return [
'files' => [
'$comment' => '"blacklisted_files" is deprecated as of Nextcloud 30, use "forbidden_filenames" instead',
'blacklisted_files' => $this->filenameValidator->getForbiddenFilenames(),
'forbidden_filenames' => $this->filenameValidator->getForbiddenFilenames(),
'forbidden_filename_basenames' => $this->filenameValidator->getForbiddenBasenames(),
'forbidden_filename_characters' => $this->filenameValidator->getForbiddenCharacters(),
'forbidden_filename_extensions' => $this->filenameValidator->getForbiddenExtensions(),

Expand Down
7 changes: 7 additions & 0 deletions apps/files/openapi.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
"bigfilechunking",
"blacklisted_files",
"forbidden_filenames",
"forbidden_filename_basenames",
"forbidden_filename_characters",
"forbidden_filename_extensions",
"directEditing"
Expand All @@ -57,6 +58,12 @@
"type": "string"
}
},
"forbidden_filename_basenames": {
"type": "array",
"items": {
"type": "string"
}
},
"forbidden_filename_characters": {
"type": "array",
"items": {
Expand Down
12 changes: 12 additions & 0 deletions config/config.sample.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1987,6 +1987,18 @@
*/
'forbidden_filenames' => ['.htaccess'],

/**
* Disallow the upload of files with specific basenames.
*
* The basename is the name of the file without the extension,
* e.g. for "archive.tar.gz" the basename would be "archive".
*
* Note that this list is case-insensitive.
*
* Defaults to ``array()``
*/
'forbidden_filename_basenames' => [],

/**
* 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.
Expand Down
71 changes: 48 additions & 23 deletions lib/private/Files/FilenameValidator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ class FilenameValidator implements IFilenameValidator {
*/
private array $forbiddenNames = [];

/**
* @var list<string>
*/
private array $forbiddenBasenames = [];
/**
* @var list<string>
*/
Expand All @@ -56,17 +60,11 @@ public function __construct(
*/
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'];
}
$forbiddenExtensions = $this->getConfigValue('forbidden_filename_extensions', ['.filepart']);

// Always forbid .part files as they are used internally
$forbiddenExtensions = array_merge($forbiddenExtensions, ['.part']);
$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;
Expand All @@ -80,31 +78,37 @@ public function getForbiddenExtensions(): array {
*/
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'];
}
$forbiddenNames = $this->getConfigValue('forbidden_filenames', ['.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 = [];
}
$legacyForbiddenNames = $this->getConfigValue('blacklisted_files', []);
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);
// Ensure we are having a proper string list
$this->forbiddenNames = array_values($forbiddenNames);
}
return $this->forbiddenNames;
}

/**
* Get a list of forbidden file basenames that must not be used
* This list should be checked case-insensitive, all names are returned lowercase.
* @return list<string>
* @since 30.0.0
*/
public function getForbiddenBasenames(): array {
if (empty($this->forbiddenBasenames)) {
$forbiddenBasenames = $this->getConfigValue('forbidden_filename_basenames', []);
// Ensure we are having a proper string list
$this->forbiddenBasenames = array_values($forbiddenBasenames);
}
return $this->forbiddenBasenames;
}

/**
* Get a list of characters forbidden in filenames
*
Expand Down Expand Up @@ -194,17 +198,24 @@ public function validateFilename(string $filename): void {
* @return bool True if invalid name, False otherwise
*/
public function isForbidden(string $path): bool {
// We support paths here as this function is also used in some storage internals
$filename = basename($path);
$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($filename, $forbiddenNames)) {
return true;
}

// Check for forbidden basenames - basenames are the part of the file until the first dot
// (except if the dot is the first character as this is then part of the basename "hidden files")
$basename = substr($filename, 0, strpos($filename, '.', 1) ?: null);
$forbiddenNames = $this->getForbiddenBasenames();
if (in_array($basename, $forbiddenNames)) {
return true;
}
Expand All @@ -226,7 +237,7 @@ protected function checkForbiddenCharacters(string $filename): void {

foreach ($this->getForbiddenCharacters() as $char) {
if (str_contains($filename, $char)) {
throw new InvalidCharacterInPathException($char);
throw new InvalidCharacterInPathException($this->l10n->t('Invalid character "%1$s" in filename', [$char]));
}
}
}
Expand All @@ -246,4 +257,18 @@ protected function checkForbiddenExtension(string $filename): void {
}
}
}

/**
* Helper to get lower case list from config with validation
* @return string[]
*/
private function getConfigValue(string $key, array $fallback): array {
$values = $this->config->getSystemValue($key, ['.filepart']);
if (!is_array($values)) {
$this->logger->error('Invalid system config value for "' . $key . '" is ignored.');
$values = $fallback;
}

return array_map('mb_strtolower', $values);
}
};
85 changes: 58 additions & 27 deletions tests/lib/Files/FilenameValidatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,16 +49,24 @@ protected function setUp(): void {
public function testValidateFilename(
string $filename,
array $forbiddenNames,
array $forbiddenBasenames,
array $forbiddenExtensions,
array $forbiddenCharacters,
?string $exception,
): void {
/** @var FilenameValidator&MockObject */
$validator = $this->getMockBuilder(FilenameValidator::class)
->onlyMethods(['getForbiddenExtensions', 'getForbiddenFilenames', 'getForbiddenCharacters'])
->onlyMethods([
'getForbiddenBasenames',
'getForbiddenCharacters',
'getForbiddenExtensions',
'getForbiddenFilenames',
])
->setConstructorArgs([$this->l10n, $this->config, $this->logger])
->getMock();

$validator->method('getForbiddenBasenames')
->willReturn($forbiddenBasenames);
$validator->method('getForbiddenCharacters')
->willReturn($forbiddenCharacters);
$validator->method('getForbiddenExtensions')
Expand All @@ -80,16 +88,24 @@ public function testValidateFilename(
public function testIsFilenameValid(
string $filename,
array $forbiddenNames,
array $forbiddenBasenames,
array $forbiddenExtensions,
array $forbiddenCharacters,
?string $exception,
): void {
/** @var FilenameValidator&MockObject */
$validator = $this->getMockBuilder(FilenameValidator::class)
->onlyMethods(['getForbiddenExtensions', 'getForbiddenFilenames', 'getForbiddenCharacters'])
->onlyMethods([
'getForbiddenBasenames',
'getForbiddenExtensions',
'getForbiddenFilenames',
'getForbiddenCharacters',
])
->setConstructorArgs([$this->l10n, $this->config, $this->logger])
->getMock();

$validator->method('getForbiddenBasenames')
->willReturn($forbiddenBasenames);
$validator->method('getForbiddenCharacters')
->willReturn($forbiddenCharacters);
$validator->method('getForbiddenExtensions')
Expand All @@ -104,84 +120,99 @@ public function testIsFilenameValid(
public function dataValidateFilename(): array {
return [
'valid name' => [
'a: b.txt', ['.htaccess'], [], [], null
'a: b.txt', ['.htaccess'], [], [], [], null
],
'valid name with some more parameters' => [
'a: b.txt', ['.htaccess'], ['exe'], ['~'], null
'a: b.txt', ['.htaccess'], [], ['exe'], ['~'], null
],
'valid name checks only the full name' => [
'.htaccess.sample', ['.htaccess'], [], [], [], null
],
'forbidden name' => [
'.htaccess', ['.htaccess'], [], [], ReservedWordException::class
'.htaccess', ['.htaccess'], [], [], [], ReservedWordException::class
],
'forbidden name - name is case insensitive' => [
'COM1', ['.htaccess', 'com1'], [], [], ReservedWordException::class
'COM1', ['.htaccess', 'com1'], [], [], [], ReservedWordException::class
],
'forbidden basename' => [
// needed for Windows namespaces
'com1.suffix', ['.htaccess'], ['com1'], [], [], ReservedWordException::class
],
'forbidden name - name checks the filename' => [
'forbidden basename for hidden files' => [
// needed for Windows namespaces
'com1.suffix', ['.htaccess', 'com1'], [], [], ReservedWordException::class
'.thumbs.db', ['.htaccess'], ['.thumbs'], [], [], ReservedWordException::class
],
'invalid character' => [
'a: b.txt', ['.htaccess'], [], [':'], InvalidCharacterInPathException::class
'a: b.txt', ['.htaccess'], [], [], [':'], InvalidCharacterInPathException::class
],
'invalid path' => [
'../../foo.bar', ['.htaccess'], [], ['/', '\\'], InvalidCharacterInPathException::class,
'../../foo.bar', ['.htaccess'], [], [], ['/', '\\'], InvalidCharacterInPathException::class,
],
'invalid extension' => [
'a: b.txt', ['.htaccess'], ['.txt'], [], InvalidPathException::class
'a: b.txt', ['.htaccess'], [], ['.txt'], [], InvalidPathException::class
],
'empty filename' => [
'', [], [], [], EmptyFileNameException::class
'', [], [], [], [], EmptyFileNameException::class
],
'reserved unix name "."' => [
'.', [], [], [], InvalidPathException::class
'.', [], [], [], [], InvalidPathException::class
],
'reserved unix name ".."' => [
'..', [], [], [], ReservedWordException::class
'..', [], [], [], [], ReservedWordException::class
],
'too long filename "."' => [
str_repeat('a', 251), [], [], [], FileNameTooLongException::class
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],
['🌫️.txt', ['.htaccess'], [], [], ['😶‍🌫️'], null],
// This is the reverse: clouds are forbidden -> so is also the face in the clouds emoji
['😶‍🌫️.txt', ['.htaccess'], [], ['🌫️'], InvalidCharacterInPathException::class],
['😶‍🌫️.txt', ['.htaccess'], [], [], ['🌫️'], InvalidCharacterInPathException::class],
];
}

/**
* @dataProvider dataIsForbidden
*/
public function testIsForbidden(string $filename, array $forbiddenNames, bool $expected): void {
public function testIsForbidden(string $filename, array $forbiddenNames, array $forbiddenBasenames, bool $expected): void {
/** @var FilenameValidator&MockObject */
$validator = $this->getMockBuilder(FilenameValidator::class)
->onlyMethods(['getForbiddenFilenames'])
->onlyMethods(['getForbiddenFilenames', 'getForbiddenBasenames'])
->setConstructorArgs([$this->l10n, $this->config, $this->logger])
->getMock();

$validator->method('getForbiddenBasenames')
->willReturn($forbiddenBasenames);
$validator->method('getForbiddenFilenames')
->willReturn($forbiddenNames);


$this->assertEquals($expected, $validator->isFilenameValid($filename));
$this->assertEquals($expected, $validator->isForbidden($filename));
}

public function dataIsForbidden(): array {
return [
'valid name' => [
'a: b.txt', ['.htaccess'], true
'a: b.txt', ['.htaccess'], [], false
],
'valid name with some more parameters' => [
'a: b.txt', ['.htaccess'], true
'a: b.txt', ['.htaccess'], [], false
],
'valid name as only full forbidden should be matched' => [
'.htaccess.sample', ['.htaccess'], [], false,
],
'forbidden name' => [
'.htaccess', ['.htaccess'], false
'.htaccess', ['.htaccess'], [], true
],
'forbidden name - name is case insensitive' => [
'COM1', ['.htaccess', 'com1'], false
'COM1', ['.htaccess', 'com1'], [], true,
],
'forbidden name - basename is checked' => [
// needed for Windows namespaces
'com1.suffix', ['.htaccess'], ['com1'], true
],
'forbidden name - name checks the filename' => [
'forbidden name - basename is checked also with multiple extensions' => [
// needed for Windows namespaces
'com1.suffix', ['.htaccess', 'com1'], false
'com1.tar.gz', ['.htaccess'], ['com1'], true
],
];
}
Expand Down
Loading