Skip to content

Commit

Permalink
chore: Replace deprecated functions with new interface (IFilenameVali…
Browse files Browse the repository at this point in the history 
…dator)

Signed-off-by: Ferdinand Thiessen <[email protected]>
  • Loading branch information
@susnux
susnux committed Aug 27, 2024
1 parent 54915b5 commit 0c5f5c1
Show file tree
Hide file tree
Showing 7 changed files with 128 additions and 155 deletions.
18 changes: 0 additions & 18 deletions lib/private/Files/Filesystem.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion lib/private/Files/Storage/Common.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -630,7 +630,9 @@ public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t
* @inheritdoc
*/
public function getMetaData($path) {
if (Filesystem::isFileBlacklisted($path)) {
/** @var \OC\Files\FilenameValidator */
$validator = $this->getFilenameValidator();
if ($validator->isForbidden($path)) {
throw new ForbiddenException('Invalid path: ' . $path, false);
}

Expand Down
4 changes: 3 additions & 1 deletion lib/private/Files/Storage/DAV.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -561,7 +561,9 @@ public function copy($source, $target) {
}

public function getMetaData($path) {
if (Filesystem::isFileBlacklisted($path)) {
/** @var \OC\Files\FilenameValidator */
$validator = $this->getFilenameValidator();
if ($validator->isForbidden($path)) {
throw new ForbiddenException('Invalid path: ' . $path, false);
}
$response = $this->propfind($path);
Expand Down
8 changes: 6 additions & 2 deletions lib/private/Files/Storage/Local.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -318,9 +318,11 @@ public function unlink($path) {

private function checkTreeForForbiddenItems(string $path) {
$iterator = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($path));
/** @var \OC\Files\FilenameValidator */
$validator = $this->getFilenameValidator();
foreach ($iterator as $file) {
/** @var \SplFileInfo $file */
if (Filesystem::isFileBlacklisted($file->getBasename())) {
if ($validator->isForbidden($file->getBasename())) {
throw new ForbiddenException('Invalid path: ' . $file->getPathname(), false);
}
}
Expand Down Expand Up @@ -475,7 +477,9 @@ public function hasUpdated($path, $time) {
* @throws ForbiddenException
*/
public function getSourcePath($path) {
if (Filesystem::isFileBlacklisted($path)) {
/** @var \OC\Files\FilenameValidator */
$validator = $this->getFilenameValidator();
if ($validator->isForbidden($path)) {
throw new ForbiddenException('Invalid path: ' . $path, false);
}

Expand Down
220 changes: 110 additions & 110 deletions lib/private/Files/View.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -590,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->isForbidden($path)
) {
$path = $this->getRelativePath($absolutePath);
if ($path === null) {
Expand Down Expand Up @@ -702,133 +702,133 @@ public function rename($source, $target) {
throw new ForbiddenException('Moving a folder into a child folder is forbidden', false);
}

$targetParts = explode('/', $absolutePath2);
$targetUser = $targetParts[1] ?? null;
$result = false;
if (
Filesystem::isValidPath($target)
&& Filesystem::isValidPath($source)
&& !Filesystem::isFileBlacklisted($target)
if (!Filesystem::isValidPath($target)
|| !Filesystem::isValidPath($source)
) {
$source = $this->getRelativePath($absolutePath1);
$target = $this->getRelativePath($absolutePath2);
$exists = $this->file_exists($target);
return false;
}

if ($source == null || $target == null) {
return false;
}
$source = $this->getRelativePath($absolutePath1);
$target = $this->getRelativePath($absolutePath2);
$exists = $this->file_exists($target);

try {
$this->verifyPath(dirname($target), basename($target));
} catch (InvalidPathException) {
return false;
}
if ($source == null || $target == null) {
return false;
}

$this->lockFile($source, ILockingProvider::LOCK_SHARED, true);
try {
$this->lockFile($target, ILockingProvider::LOCK_SHARED, true);
try {
$this->verifyPath(dirname($target), basename($target));
} catch (InvalidPathException) {
return false;
}

$run = true;
if ($this->shouldEmitHooks($source) && (Cache\Scanner::isPartialFile($source) && !Cache\Scanner::isPartialFile($target))) {
// if it was a rename from a part file to a regular file it was a write and not a rename operation
$this->emit_file_hooks_pre($exists, $target, $run);
} elseif ($this->shouldEmitHooks($source)) {
$sourcePath = $this->getHookPath($source);
$targetPath = $this->getHookPath($target);
if ($sourcePath !== null && $targetPath !== null) {
\OC_Hook::emit(
Filesystem::CLASSNAME, Filesystem::signal_rename,
[
Filesystem::signal_param_oldpath => $sourcePath,
Filesystem::signal_param_newpath => $targetPath,
Filesystem::signal_param_run => &$run
]
);
}
}
if ($run) {
$manager = Filesystem::getMountManager();
$mount1 = $this->getMount($source);
$mount2 = $this->getMount($target);
$storage1 = $mount1->getStorage();
$storage2 = $mount2->getStorage();
$internalPath1 = $mount1->getInternalPath($absolutePath1);
$internalPath2 = $mount2->getInternalPath($absolutePath2);
$targetParts = explode('/', $absolutePath2);
$targetUser = $targetParts[1] ?? null;
$result = false;
$this->lockFile($source, ILockingProvider::LOCK_SHARED, true);
try {
$this->lockFile($target, ILockingProvider::LOCK_SHARED, true);

$this->changeLock($source, ILockingProvider::LOCK_EXCLUSIVE, true);
try {
$this->changeLock($target, ILockingProvider::LOCK_EXCLUSIVE, true);

if ($internalPath1 === '') {
if ($mount1 instanceof MoveableMount) {
$sourceParentMount = $this->getMount(dirname($source));
if ($sourceParentMount === $mount2 && $this->targetIsNotShared($targetUser, $absolutePath2)) {
/**
* @var \OC\Files\Mount\MountPoint | \OC\Files\Mount\MoveableMount $mount1
*/
$sourceMountPoint = $mount1->getMountPoint();
$result = $mount1->moveMount($absolutePath2);
$manager->moveMount($sourceMountPoint, $mount1->getMountPoint());
} else {
$result = false;
}
} else {
$result = false;
}
// moving a file/folder within the same mount point
} elseif ($storage1 === $storage2) {
if ($storage1) {
$result = $storage1->rename($internalPath1, $internalPath2);
$run = true;
if ($this->shouldEmitHooks($source) && (Cache\Scanner::isPartialFile($source) && !Cache\Scanner::isPartialFile($target))) {
// if it was a rename from a part file to a regular file it was a write and not a rename operation
$this->emit_file_hooks_pre($exists, $target, $run);
} elseif ($this->shouldEmitHooks($source)) {
$sourcePath = $this->getHookPath($source);
$targetPath = $this->getHookPath($target);
if ($sourcePath !== null && $targetPath !== null) {
\OC_Hook::emit(
Filesystem::CLASSNAME, Filesystem::signal_rename,
[
Filesystem::signal_param_oldpath => $sourcePath,
Filesystem::signal_param_newpath => $targetPath,
Filesystem::signal_param_run => &$run
]
);
}
}
if ($run) {
$manager = Filesystem::getMountManager();
$mount1 = $this->getMount($source);
$mount2 = $this->getMount($target);
$storage1 = $mount1->getStorage();
$storage2 = $mount2->getStorage();
$internalPath1 = $mount1->getInternalPath($absolutePath1);
$internalPath2 = $mount2->getInternalPath($absolutePath2);

$this->changeLock($source, ILockingProvider::LOCK_EXCLUSIVE, true);
try {
$this->changeLock($target, ILockingProvider::LOCK_EXCLUSIVE, true);

if ($internalPath1 === '') {
if ($mount1 instanceof MoveableMount) {
$sourceParentMount = $this->getMount(dirname($source));
if ($sourceParentMount === $mount2 && $this->targetIsNotShared($targetUser, $absolutePath2)) {
/**
* @var \OC\Files\Mount\MountPoint | \OC\Files\Mount\MoveableMount $mount1
*/
$sourceMountPoint = $mount1->getMountPoint();
$result = $mount1->moveMount($absolutePath2);
$manager->moveMount($sourceMountPoint, $mount1->getMountPoint());
} else {
$result = false;
}
// moving a file/folder between storages (from $storage1 to $storage2)
} else {
$result = $storage2->moveFromStorage($storage1, $internalPath1, $internalPath2);
$result = false;
}

if ((Cache\Scanner::isPartialFile($source) && !Cache\Scanner::isPartialFile($target)) && $result !== false) {
// if it was a rename from a part file to a regular file it was a write and not a rename operation
$this->writeUpdate($storage2, $internalPath2);
} elseif ($result) {
if ($internalPath1 !== '') { // don't do a cache update for moved mounts
$this->renameUpdate($storage1, $storage2, $internalPath1, $internalPath2);
}
// moving a file/folder within the same mount point
} elseif ($storage1 === $storage2) {
if ($storage1) {
$result = $storage1->rename($internalPath1, $internalPath2);
} else {
$result = false;
}
} catch (\Exception $e) {
throw $e;
} finally {
$this->changeLock($source, ILockingProvider::LOCK_SHARED, true);
$this->changeLock($target, ILockingProvider::LOCK_SHARED, true);
// moving a file/folder between storages (from $storage1 to $storage2)
} else {
$result = $storage2->moveFromStorage($storage1, $internalPath1, $internalPath2);
}

if ((Cache\Scanner::isPartialFile($source) && !Cache\Scanner::isPartialFile($target)) && $result !== false) {
if ($this->shouldEmitHooks()) {
$this->emit_file_hooks_post($exists, $target);
}
// if it was a rename from a part file to a regular file it was a write and not a rename operation
$this->writeUpdate($storage2, $internalPath2);
} elseif ($result) {
if ($this->shouldEmitHooks($source) && $this->shouldEmitHooks($target)) {
$sourcePath = $this->getHookPath($source);
$targetPath = $this->getHookPath($target);
if ($sourcePath !== null && $targetPath !== null) {
\OC_Hook::emit(
Filesystem::CLASSNAME,
Filesystem::signal_post_rename,
[
Filesystem::signal_param_oldpath => $sourcePath,
Filesystem::signal_param_newpath => $targetPath,
]
);
}
if ($internalPath1 !== '') { // don't do a cache update for moved mounts
$this->renameUpdate($storage1, $storage2, $internalPath1, $internalPath2);
}
}
} catch (\Exception $e) {
throw $e;
} finally {
$this->changeLock($source, ILockingProvider::LOCK_SHARED, true);
$this->changeLock($target, ILockingProvider::LOCK_SHARED, true);
}

if ((Cache\Scanner::isPartialFile($source) && !Cache\Scanner::isPartialFile($target)) && $result !== false) {
if ($this->shouldEmitHooks()) {
$this->emit_file_hooks_post($exists, $target);
}
} elseif ($result) {
if ($this->shouldEmitHooks($source) && $this->shouldEmitHooks($target)) {
$sourcePath = $this->getHookPath($source);
$targetPath = $this->getHookPath($target);
if ($sourcePath !== null && $targetPath !== null) {
\OC_Hook::emit(
Filesystem::CLASSNAME,
Filesystem::signal_post_rename,
[
Filesystem::signal_param_oldpath => $sourcePath,
Filesystem::signal_param_newpath => $targetPath,
]
);
}
}
}
} catch (\Exception $e) {
throw $e;
} finally {
$this->unlockFile($source, ILockingProvider::LOCK_SHARED, true);
$this->unlockFile($target, ILockingProvider::LOCK_SHARED, true);
}
} catch (\Exception $e) {
throw $e;
} finally {
$this->unlockFile($source, ILockingProvider::LOCK_SHARED, true);
$this->unlockFile($target, ILockingProvider::LOCK_SHARED, true);
}
return $result;
}
Expand All @@ -849,7 +849,7 @@ public function copy($source, $target, $preserveMtime = false) {
if (
Filesystem::isValidPath($target)
&& Filesystem::isValidPath($source)
&& !Filesystem::isFileBlacklisted($target)
&& !$this->filenameValidator->isForbidden($target)
) {
$source = $this->getRelativePath($absolutePath1);
$target = $this->getRelativePath($absolutePath2);
Expand Down Expand Up @@ -1106,7 +1106,7 @@ private function basicOperation(string $operation, string $path, array $hooks =
$postFix = (substr($path, -1) === '/') ? '/' : '';
$absolutePath = Filesystem::normalizePath($this->getAbsolutePath($path));
if (Filesystem::isValidPath($path)
&& !Filesystem::isFileBlacklisted($path)
&& !$this->filenameValidator->isForbidden($path)
) {
$path = $this->getRelativePath($absolutePath);
if ($path == null) {
Expand Down
22 changes: 0 additions & 22 deletions tests/lib/Files/FilesystemTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
7 changes: 6 additions & 1 deletion tests/lib/Files/Storage/CommonTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

namespace Test\Files\Storage;

use OC\Files\FilenameValidator;
use OC\Files\Storage\Wrapper\Jail;
use OC\Files\Storage\Wrapper\Wrapper;
use OCP\Files\IFilenameValidator;
Expand All @@ -30,7 +31,11 @@ class CommonTest extends Storage {
protected function setUp(): void {
parent::setUp();

$this->filenameValidator = $this->createMock(IFilenameValidator::class);
$this->filenameValidator = $this->createMock(FilenameValidator::class);
$this->filenameValidator
->expects(self::any())
->method('isForbidden')
->willReturn(false);
$this->overwriteService(IFilenameValidator::class, $this->filenameValidator);
$this->tmpDir = \OCP\Server::get(ITempManager::class)->getTemporaryFolder();
$this->instance = new \OC\Files\Storage\CommonTest(['datadir' => $this->tmpDir]);
Expand Down

0 comments on commit 0c5f5c1

Please sign in to comment.