From f344c69b822a9fefea4fa6817f46fd266eb23010 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 28 Nov 2024 19:08:27 +0100 Subject: [PATCH] fix: improve checks for moving shares/storages into other mounts Signed-off-by: Robin Appelman --- lib/private/Files/View.php | 68 ++++++++++++++++++++++++++++-------- tests/lib/Files/ViewTest.php | 51 ++++++++++++++++++++++----- 2 files changed, 95 insertions(+), 24 deletions(-) diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 50e860c83fc..2a396bf3507 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -63,6 +63,7 @@ use OCP\Files\ForbiddenException; use OCP\Files\InvalidCharacterInPathException; use OCP\Files\InvalidDirectoryException; use OCP\Files\InvalidPathException; +use OCP\Files\Mount\IMountManager; use OCP\Files\Mount\IMountPoint; use OCP\Files\NotFoundException; use OCP\Files\ReservedWordException; @@ -737,6 +738,11 @@ class View { throw new ForbiddenException("Moving a folder into a child folder is forbidden", false); } + /** @var IMountManager $mountManager */ + $mountManager = \OC::$server->get(IMountManager::class); + + $targetParts = explode('/', $absolutePath2); + $targetUser = $targetParts[1] ?? null; $result = false; if ( Filesystem::isValidPath($target) @@ -788,24 +794,25 @@ class View { try { $this->changeLock($target, ILockingProvider::LOCK_EXCLUSIVE, true); + $movedMounts = $mountManager->findIn($this->getAbsolutePath($source)); + if ($internalPath1 === '') { - if ($mount1 instanceof MoveableMount) { - $sourceParentMount = $this->getMount(dirname($source)); - if ($sourceParentMount === $mount2 && $this->targetIsNotShared($storage2, $internalPath2)) { - /** - * @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; - } + $sourceParentMount = $this->getMount(dirname($source)); + $movedMounts[] = $mount1; + $this->validateMountMove($movedMounts, $sourceParentMount, $mount2, !$this->targetIsNotShared($storage2, $internalPath2)); + + /** + * @var \OC\Files\Mount\MountPoint | \OC\Files\Mount\MoveableMount $mount1 + */ + $sourceMountPoint = $mount1->getMountPoint(); + $result = $mount1->moveMount($absolutePath2); + $manager->moveMount($sourceMountPoint, $mount1->getMountPoint()); + // moving a file/folder within the same mount point } elseif ($storage1 === $storage2) { + if (count($movedMounts) > 0) { + $this->validateMountMove($movedMounts, $mount1, $mount2, !$this->targetIsNotShared($storage2, $absolutePath2)); + } if ($storage1) { $result = $storage1->rename($internalPath1, $internalPath2); } else { @@ -813,6 +820,9 @@ class View { } // moving a file/folder between storages (from $storage1 to $storage2) } else { + if (count($movedMounts) > 0) { + $this->validateMountMove($movedMounts, $mount1, $mount2, !$this->targetIsNotShared($storage2, $absolutePath2)); + } $result = $storage2->moveFromStorage($storage1, $internalPath1, $internalPath2); } @@ -862,6 +872,34 @@ class View { return $result; } + private function validateMountMove(array $mounts, IMountPoint $sourceMount, IMountPoint $targetMount, bool $targetIsShared): void { + $targetType = 'storage'; + if ($targetMount instanceof SharedMount) { + $targetType = 'share'; + } + $targetPath = rtrim($targetMount->getMountPoint(), '/'); + + foreach ($mounts as $mount) { + $sourcePath = rtrim($mount->getMountPoint(), '/'); + $sourceType = 'storage'; + if ($mount instanceof SharedMount) { + $sourceType = 'share'; + } + + if (!$mount instanceof MoveableMount) { + throw new ForbiddenException("Storage {$sourcePath} cannot be moved", false); + } + + if ($targetIsShared) { + throw new ForbiddenException("Moving a $sourceType ($sourcePath) into shared folder is not allowed", false); + } + + if ($sourceMount !== $targetMount) { + throw new ForbiddenException("Moving a $sourceType ($sourcePath) into another $targetType ($targetPath) is not allowed", false); + } + } + } + /** * Copy a file/folder from the source path to target path * diff --git a/tests/lib/Files/ViewTest.php b/tests/lib/Files/ViewTest.php index ad22a7267e6..b4a557b0a38 100644 --- a/tests/lib/Files/ViewTest.php +++ b/tests/lib/Files/ViewTest.php @@ -20,6 +20,7 @@ use OCP\Cache\CappedMemoryCache; use OCP\Constants; use OCP\Files\Config\IMountProvider; use OCP\Files\FileInfo; +use OCP\Files\ForbiddenException; use OCP\Files\GenericFileException; use OCP\Files\Mount\IMountManager; use OCP\Files\Storage\IStorage; @@ -1638,10 +1639,27 @@ class ViewTest extends \Test\TestCase { $this->assertTrue($view->rename('mount2', 'sub/moved_mount'), 'Can move a mount point into a subdirectory'); } - /** - * Test that moving a mount point into another is forbidden - */ - public function testMoveMountPointIntoAnother() { + public function testMoveMountPointOverwrite(): void { + self::loginAsUser($this->user); + + [$mount1, $mount2] = $this->createTestMovableMountPoints([ + $this->user . '/files/mount1', + $this->user . '/files/mount2', + ]); + + $mount1->expects($this->never()) + ->method('moveMount'); + + $mount2->expects($this->never()) + ->method('moveMount'); + + $view = new View('/' . $this->user . '/files/'); + + $this->expectException(ForbiddenException::class); + $view->rename('mount1', 'mount2'); + } + + public function testMoveMountPointIntoMount(): void { self::loginAsUser($this->user); [$mount1, $mount2] = $this->createTestMovableMountPoints([ @@ -1657,8 +1675,8 @@ class ViewTest extends \Test\TestCase { $view = new View('/' . $this->user . '/files/'); - $this->assertFalse($view->rename('mount1', 'mount2'), 'Cannot overwrite another mount point'); - $this->assertFalse($view->rename('mount1', 'mount2/sub'), 'Cannot move a mount point into another'); + $this->expectException(ForbiddenException::class); + $view->rename('mount1', 'mount2/sub'); } /** @@ -1693,9 +1711,24 @@ class ViewTest extends \Test\TestCase { ->setNode($shareDir); $shareManager->createShare($share); - $this->assertFalse($view->rename('mount1', 'shareddir'), 'Cannot overwrite shared folder'); - $this->assertFalse($view->rename('mount1', 'shareddir/sub'), 'Cannot move mount point into shared folder'); - $this->assertFalse($view->rename('mount1', 'shareddir/sub/sub2'), 'Cannot move mount point into shared subfolder'); + try { + $view->rename('mount1', 'shareddir'); + $this->fail('Cannot overwrite shared folder'); + } catch (ForbiddenException $e) { + + } + try { + $view->rename('mount1', 'shareddir/sub'); + $this->fail('Cannot move mount point into shared folder'); + } catch (ForbiddenException $e) { + + } + try { + $view->rename('mount1', 'shareddir/sub/sub2'); + $this->fail('Cannot move mount point into shared subfolder'); + } catch (ForbiddenException $e) { + + } $shareManager->deleteShare($share); $userObject->delete();