Skip to content

Commit

Permalink
fix: improve checks for moving shares/storages into other mounts
Browse files Browse the repository at this point in the history
Signed-off-by: Robin Appelman <robin@icewind.nl>
  • Loading branch information
icewind1991 committed Nov 29, 2024
1 parent dd101dd commit be89374
Show file tree
Hide file tree
Showing 2 changed files with 93 additions and 24 deletions.
66 changes: 51 additions & 15 deletions lib/private/Files/View.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -758,31 +762,35 @@ 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 {
$result = false;
}
// 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);
}

Expand Down Expand Up @@ -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
*
Expand Down
51 changes: 42 additions & 9 deletions tests/lib/Files/ViewTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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([
Expand All @@ -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');
}

/**
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit be89374

Please sign in to comment.