fix: improve checks for moving shares/storages into other mounts

Signed-off-by: Robin Appelman <robin@icewind.nl>

2 files changed, 93 insertions, 24 deletions

diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php
index 738e1442b67..37842ce02aa 100644
--- a/lib/private/Files/View.php
+++ b/lib/private/Files/View.php
@@ -24,6 +24,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;
@@ -700,6 +701,9 @@ 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;
@@ -757,24 +761,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($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 {
@@ -782,6 +787,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($targetUser, $absolutePath2));
+							}
 							$result = $storage2->moveFromStorage($storage1, $internalPath1, $internalPath2);
 						}
 
@@ -831,6 +839,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 2dfbf61641a..f20c9f05a22 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;
@@ -1639,10 +1640,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([
@@ -1658,8 +1676,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');
 	}
 
 	/**
@@ -1701,9 +1719,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) {
+
+		}
 		$this->assertTrue($view->rename('mount2', 'shareddir notshared/sub'), 'Can move mount point into a similarly named but non-shared folder');
 
 		$shareManager->deleteShare($share);