Prevent moving mount point into already shared folder (outgoing)

It is already not allowed to share a folder containing mount points / incoming shares. This fixes an issue that made it possible to bypass the check by moving the incoming share mount point into an existing outgoing share folder.
10 years ago · 64ca00925b
2 changed files with 141 additions and 27 deletions
--- a/lib/private/files/view.php
+++ b/lib/private/files/view.php
@ -1602,25 +1602,46 @@ class View {

 	/**
 	 * check if it is allowed to move a mount point to a given target.
-	 * It is not allowed to move a mount point into a different mount point
+	 * It is not allowed to move a mount point into a different mount point or
+	 * into an already shared folder
 	 *
 	 * @param string $target path
 	 * @return boolean
 	 */
 	private function isTargetAllowed($target) {

-		$result = false;
-
-		list($targetStorage,) = \OC\Files\Filesystem::resolvePath($target);
-		if ($targetStorage->instanceOfStorage('\OCP\Files\IHomeStorage')) {
-			$result = true;
-		} else {
+		list($targetStorage, $targetInternalPath) = \OC\Files\Filesystem::resolvePath($target);
+		if (!$targetStorage->instanceOfStorage('\OCP\Files\IHomeStorage')) {
 			\OCP\Util::writeLog('files',
 				'It is not allowed to move one mount point into another one',
 				\OCP\Util::DEBUG);
+			return false;
 		}

-		return $result;
+		// note: cannot use the view because the target is already locked
+		$fileId = (int)$targetStorage->getCache()->getId($targetInternalPath);
+		if ($fileId === -1) {
+			// target might not exist, need to check parent instead
+			$fileId = (int)$targetStorage->getCache()->getId(dirname($targetInternalPath));
+		}
+
+		// check if any of the parents were shared by the current owner (include collections)
+		$shares = \OCP\Share::getItemShared(
+			'folder',
+			$fileId,
+			\OCP\Share::FORMAT_NONE,
+			null,
+			true
+		);
+
+		if (count($shares) > 0) {
+			\OCP\Util::writeLog('files',
+				'It is not allowed to move one mount point into a shared folder',
+				\OCP\Util::DEBUG);
+			return false;
+		}
+
+		return true;
 	}

 	/**
--- a/tests/lib/files/view.php
+++ b/tests/lib/files/view.php
@ -104,6 +104,9 @@ class View extends \Test\TestCase {
 		$this->userObject->delete();
 		$this->groupObject->delete();

+		$mountProviderCollection = \OC::$server->getMountProviderCollection();
+		\Test\TestCase::invokePrivate($mountProviderCollection, 'providers', [[]]);
+
 		parent::tearDown();
 	}

@ -1486,6 +1489,112 @@ class View extends \Test\TestCase {
 		$this->assertEquals($shouldEmit, $result);
 	}

+	/**
+	 * Create test movable mount points
+	 *
+	 * @param array $mountPoints array of mount point locations
+	 * @return array array of MountPoint objects
+	 */
+	private function createTestMovableMountPoints($mountPoints) {
+		$mounts = [];
+		foreach ($mountPoints as $mountPoint) {
+			$storage = $this->getMockBuilder('\OC\Files\Storage\Temporary')
+				->setMethods([])
+				->getMock();
+
+			$mounts[] = $this->getMock(
+				'\Test\TestMoveableMountPoint',
+				['moveMount'],
+				[$storage, $mountPoint]
+			);
+		}
+
+		$mountProvider = $this->getMock('\OCP\Files\Config\IMountProvider');
+		$mountProvider->expects($this->any())
+			->method('getMountsForUser')
+			->will($this->returnValue($mounts));
+
+		$mountProviderCollection = \OC::$server->getMountProviderCollection();
+		$mountProviderCollection->registerProvider($mountProvider);
+
+		return $mounts;
+	}
+
+	/**
+	 * Test mount point move
+	 */
+	public function testMountPointMove() {
+		$this->loginAsUser($this->user);
+
+		list($mount1, $mount2) = $this->createTestMovableMountPoints([
+			$this->user . '/files/mount1',
+			$this->user . '/files/mount2',
+		]);
+		$mount1->expects($this->once())
+			->method('moveMount')
+			->will($this->returnValue(true));
+
+		$mount2->expects($this->once())
+			->method('moveMount')
+			->will($this->returnValue(true));
+
+		$view = new \OC\Files\View('/' . $this->user . '/files/');
+		$view->mkdir('sub');
+
+		$this->assertTrue($view->rename('mount1', 'renamed_mount'), 'Can rename mount point');
+		$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() {
+		$this->loginAsUser($this->user);
+
+		list($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 \OC\Files\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');
+	}
+	/**
+	 * Test that moving a mount point into a shared folder is forbidden
+	 */
+	public function testMoveMountPointIntoSharedFolder() {
+		$this->loginAsUser($this->user);
+
+		list($mount1) = $this->createTestMovableMountPoints([
+			$this->user . '/files/mount1',
+		]);
+
+		$mount1->expects($this->never())
+			->method('moveMount');
+
+		$view = new \OC\Files\View('/' . $this->user . '/files/');
+		$view->mkdir('shareddir');
+		$view->mkdir('shareddir/sub');
+		$view->mkdir('shareddir/sub2');
+
+		$fileId = $view->getFileInfo('shareddir')->getId();
+		$userObject = \OC::$server->getUserManager()->createUser('test2', 'IHateNonMockableStaticClasses');
+		$this->assertTrue(\OCP\Share::shareItem('folder', $fileId, \OCP\Share::SHARE_TYPE_USER, 'test2', \OCP\Constants::PERMISSION_READ));
+
+		$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');
+
+		$this->assertTrue(\OCP\Share::unshare('folder', $fileId, \OCP\Share::SHARE_TYPE_USER, 'test2'));
+		$userObject->delete();
+	}

 	public function basicOperationProviderForLocks() {
 		return [
@ -2042,23 +2151,9 @@ class View extends \Test\TestCase {
 	public function testLockMoveMountPoint() {
 		$this->loginAsUser('test');

-		$subStorage = $this->getMockBuilder('\OC\Files\Storage\Temporary')
-			->setMethods([])
-			->getMock();
-
-		$mount = $this->getMock(
-			'\Test\TestMoveableMountPoint',
-			['moveMount'],
-			[$subStorage, $this->user . '/files/substorage']
-		);
-
-		$mountProvider = $this->getMock('\OCP\Files\Config\IMountProvider');
-		$mountProvider->expects($this->once())
-			->method('getMountsForUser')
-			->will($this->returnValue([$mount]));
-
-		$mountProviderCollection = \OC::$server->getMountProviderCollection();
-		$mountProviderCollection->registerProvider($mountProvider);
+		list($mount) = $this->createTestMovableMountPoints([
+			$this->user . '/files/substorage',
+		]);

 		$view = new \OC\Files\View('/' . $this->user . '/files/');
 		$view->mkdir('subdir');
@ -2109,8 +2204,6 @@ class View extends \Test\TestCase {
 		$this->assertNull($this->getFileLockType($view, $sourcePath, false), 'Shared storage root not locked after operation');
 		$this->assertNull($this->getFileLockType($view, $sourcePath, true), 'Source path not locked after operation');
 		$this->assertNull($this->getFileLockType($view, $targetPath, true), 'Target path not locked after operation');
-
-		\Test\TestCase::invokePrivate($mountProviderCollection, 'providers', [[]]);
 	}

 	/**