From 0c5f5c1440a32cbc3903b2dd8aba8294a91a52b1 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Tue, 27 Aug 2024 01:45:53 +0200 Subject: [PATCH] chore: Replace deprecated functions with new interface (IFilenameValidator) Signed-off-by: Ferdinand Thiessen --- lib/private/Files/Filesystem.php | 18 -- lib/private/Files/Storage/Common.php | 4 +- lib/private/Files/Storage/DAV.php | 4 +- lib/private/Files/Storage/Local.php | 8 +- lib/private/Files/View.php | 220 ++++++++++++------------- tests/lib/Files/FilesystemTest.php | 22 --- tests/lib/Files/Storage/CommonTest.php | 7 +- 7 files changed, 128 insertions(+), 155 deletions(-) diff --git a/lib/private/Files/Filesystem.php b/lib/private/Files/Filesystem.php index 48c069de0b95c..2cd37df67c796 100644 --- a/lib/private/Files/Filesystem.php +++ b/lib/private/Files/Filesystem.php @@ -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() @@ -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 diff --git a/lib/private/Files/Storage/Common.php b/lib/private/Files/Storage/Common.php index 77e5b95fc20be..207495a6712d6 100644 --- a/lib/private/Files/Storage/Common.php +++ b/lib/private/Files/Storage/Common.php @@ -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); } diff --git a/lib/private/Files/Storage/DAV.php b/lib/private/Files/Storage/DAV.php index 7abc0ccc18245..887dc50305eee 100644 --- a/lib/private/Files/Storage/DAV.php +++ b/lib/private/Files/Storage/DAV.php @@ -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); diff --git a/lib/private/Files/Storage/Local.php b/lib/private/Files/Storage/Local.php index a65d60bf278dd..2061866ad47e7 100644 --- a/lib/private/Files/Storage/Local.php +++ b/lib/private/Files/Storage/Local.php @@ -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); } } @@ -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); } diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index ca0320f55219d..f6dd33fb0d335 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -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) { @@ -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; } @@ -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); @@ -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) { diff --git a/tests/lib/Files/FilesystemTest.php b/tests/lib/Files/FilesystemTest.php index f7964feeae8ab..f2f5c35577308 100644 --- a/tests/lib/Files/FilesystemTest.php +++ b/tests/lib/Files/FilesystemTest.php @@ -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'); diff --git a/tests/lib/Files/Storage/CommonTest.php b/tests/lib/Files/Storage/CommonTest.php index 321894cc23b1e..b941e240abbd7 100644 --- a/tests/lib/Files/Storage/CommonTest.php +++ b/tests/lib/Files/Storage/CommonTest.php @@ -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; @@ -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]);