From be893749c133d225241ae8b6833b964afa6aee6e 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 | 66 ++++++++++++++++++++++++++++-------- tests/lib/Files/ViewTest.php | 51 +++++++++++++++++++++++----- 2 files changed, 93 insertions(+), 24 deletions(-) diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 80679a9481fa5..fe3186b3fa594 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -24,6 +24,7 @@ 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; @@ -701,6 +702,9 @@ public function rename($source, $target) { 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; @@ -758,24 +762,25 @@ public function rename($source, $target) { 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($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; - } + $sourceParentMount = $this->getMount(dirname($source)); + $movedMounts[] = $mount1; + $this->validateMountMove($movedMounts, $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()); + // moving a file/folder within the same mount point } elseif ($storage1 === $storage2) { + if (count($movedMounts) > 0) { + $this->validateMountMove($movedMounts, $mount1, $mount2, !$this->targetIsNotShared($targetUser, $absolutePath2)); + } if ($storage1) { $result = $storage1->rename($internalPath1, $internalPath2); } else { @@ -783,6 +788,9 @@ public function rename($source, $target) { } // moving a file/folder between storages (from $storage1 to $storage2) } else { + if (count($movedMounts) > 0) { + $this->validateMountMove($movedMounts, $mount1, $mount2, !$this->targetIsNotShared($targetUser, $absolutePath2)); + } $result = $storage2->moveFromStorage($storage1, $internalPath1, $internalPath2); } @@ -832,6 +840,34 @@ public function rename($source, $target) { 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 2a74558c70d1c..619a476694633 100644 --- a/tests/lib/Files/ViewTest.php +++ b/tests/lib/Files/ViewTest.php @@ -21,6 +21,7 @@ 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; @@ -1641,10 +1642,27 @@ public function testMountPointMove(): void { $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(): void { + 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([ @@ -1660,8 +1678,8 @@ public function testMoveMountPointIntoAnother(): void { $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'); } /** @@ -1703,9 +1721,24 @@ public function testMoveMountPointIntoSharedFolder(): void { ->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) { + + } $this->assertTrue($view->rename('mount2', 'shareddir notshared/sub'), 'Can move mount point into a similarly named but non-shared folder'); $shareManager->deleteShare($share);