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

fix: FilenameValidator::isForbidden should only check forbidden files #47372

Merged
merged 2 commits into from
Aug 21, 2024
Merged
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
8 changes: 6 additions & 2 deletions config/config.sample.php
Original file line number Diff line number Diff line change
Expand Up @@ -2008,8 +2008,9 @@
'updatedirectory' => '',

/**
* Block a specific file or files and disallow the upload of files
* with this name. ``.htaccess`` is blocked by default.
* Block a specific file or files and disallow the upload of files with this name.
* This blocks any access to those files (read and write).
* ``.htaccess`` is blocked by default.
*
* WARNING: USE THIS ONLY IF YOU KNOW WHAT YOU ARE DOING.
*
Expand All @@ -2021,6 +2022,7 @@

/**
* Disallow the upload of files with specific basenames.
* Matching existing files can no longer be updated and in matching folders no files can be created anymore.
*
* The basename is the name of the file without the extension,
* e.g. for "archive.tar.gz" the basename would be "archive".
Expand All @@ -2034,6 +2036,7 @@
/**
* 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.
* Matching existing files can no longer be updated and in matching folders no files can be created anymore.
*
* The '/' and '\' characters are always forbidden, as well as all characters in the ASCII range [0-31].
*
Expand All @@ -2046,6 +2049,7 @@

/**
* Deny extensions from being used for filenames.
* Matching existing files can no longer be updated and in matching folders no files can be created anymore.
*
* The '.part' extension is always forbidden, as this is used internally by Nextcloud.
*
Expand Down
27 changes: 18 additions & 9 deletions lib/private/Files/FilenameValidator.php
Original file line number Diff line number Diff line change
Expand Up @@ -198,9 +198,7 @@ public function validateFilename(string $filename): void {
}
}

if ($this->isForbidden($filename)) {
throw new ReservedWordException();
}
$this->checkForbiddenName($filename);

$this->checkForbiddenExtension($filename);

Expand All @@ -227,18 +225,25 @@ public function isForbidden(string $path): bool {
return true;
}

// Filename is not forbidden
return false;
}

protected function checkForbiddenName($filename): void {
if ($this->isForbidden($filename)) {
throw new ReservedWordException($this->l10n->t('"%1$s" is a forbidden file or folder name.', [$filename]));
}

// 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;
throw new ReservedWordException($this->l10n->t('"%1$s" is a forbidden prefix for file or folder names.', [$filename]));
}

// Filename is not forbidden
return false;
}


/**
* Check if a filename contains any of the forbidden characters
* @param string $filename
Expand All @@ -252,7 +257,7 @@ protected function checkForbiddenCharacters(string $filename): void {

foreach ($this->getForbiddenCharacters() as $char) {
if (str_contains($filename, $char)) {
throw new InvalidCharacterInPathException($this->l10n->t('Invalid character "%1$s" in filename', [$char]));
throw new InvalidCharacterInPathException($this->l10n->t('"%1$s" is not allowed inside a file or folder name.', [$char]));
}
}
}
Expand All @@ -268,7 +273,11 @@ protected function checkForbiddenExtension(string $filename): void {
$forbiddenExtensions = $this->getForbiddenExtensions();
foreach ($forbiddenExtensions as $extension) {
if (str_ends_with($filename, $extension)) {
throw new InvalidPathException($this->l10n->t('Invalid filename extension "%1$s"', [$extension]));
if (str_starts_with($extension, '.')) {
throw new InvalidPathException($this->l10n->t('"%1$s" is a forbidden file type.', [$extension]));
} else {
throw new InvalidPathException($this->l10n->t('Filenames must not end with "%1$s".', [$extension]));
}
}
}
}
Expand Down
24 changes: 7 additions & 17 deletions tests/lib/Files/FilenameValidatorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -252,15 +252,13 @@ public function dataInvalidAsciiCharacters(): array {
/**
* @dataProvider dataIsForbidden
*/
public function testIsForbidden(string $filename, array $forbiddenNames, array $forbiddenBasenames, bool $expected): void {
public function testIsForbidden(string $filename, array $forbiddenNames, bool $expected): void {
/** @var FilenameValidator&MockObject */
$validator = $this->getMockBuilder(FilenameValidator::class)
->onlyMethods(['getForbiddenFilenames', 'getForbiddenBasenames'])
->onlyMethods(['getForbiddenFilenames'])
->setConstructorArgs([$this->l10n, $this->database, $this->config, $this->logger])
->getMock();

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

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