Merge pull request #64638 from nextcloud/fix/sharing/validate-source-with-owner

fix(Sharing): Enforce source to be accessible by the owner

Author: provokateurin

apps/files/lib/Sharing/Source/NodeShareSourceType.php

@@ -56,12 +56,17 @@ public function getDisplayName(IFactory $l10nFactory): string {
 	}
 
 	#[\Override]
-	public function validateSource(string $source): bool {
+	public function validateSource(IUser $owner, string $source): bool {
 		if ((string)(int)$source !== $source) {
 			return false;
 		}
 
-		return $this->rootFolder->getFirstNodeById((int)$source) instanceof Node;
+		$node = $this->rootFolder->getUserFolder($owner->getUID())->getFirstNodeById((int)$source);
+		if (!$node instanceof Node) {
+			return false;
+		}
+
+		return $node->isReadable() && $node->isShareable();
 	}
 
 	#[\Override]

apps/files/tests/Sharing/Source/NodeShareSourceTypeTest.php

@@ -37,6 +37,8 @@ final class NodeShareSourceTypeTest extends TestCase {
 
 	private IUser $user1;
 
+	private IUser $user2;
+
 	private Node $node;
 
 	private NodeShareSourceType $sourceType;
@@ -49,8 +51,8 @@ public function setUp(): void {
 
 		$this->manager = Server::get(ISharingManager::class);
 
-		$user1 = $this->createUser('user1', 'password');
-		$this->user1 = $user1;
+		$this->user1 = $this->createUser('user1', 'password');
+		$this->user2 = $this->createUser('user2', 'password');
 
 		$userFolder = Server::get(IRootFolder::class)->getUserFolder($this->user1->getUID());
 		$this->node = $userFolder->newFile('foo.txt', 'bar');
@@ -75,11 +77,12 @@ protected function tearDown(): void {
 	}
 
 	public function testValidateSource(): void {
-		$this->assertTrue($this->sourceType->validateSource((string)$this->node->getId()));
-		$this->assertFalse($this->sourceType->validateSource('-1'));
-		$this->assertFalse($this->sourceType->validateSource('000123'));
-		$this->assertFalse($this->sourceType->validateSource('123abcdef'));
-		$this->assertFalse($this->sourceType->validateSource('000123abcdef'));
+		$this->assertTrue($this->sourceType->validateSource($this->user1, (string)$this->node->getId()));
+		$this->assertFalse($this->sourceType->validateSource($this->user2, (string)$this->node->getId()));
+		$this->assertFalse($this->sourceType->validateSource($this->user1, '-1'));
+		$this->assertFalse($this->sourceType->validateSource($this->user1, '000123'));
+		$this->assertFalse($this->sourceType->validateSource($this->user1, '123abcdef'));
+		$this->assertFalse($this->sourceType->validateSource($this->user1, '000123abcdef'));
 	}
 
 	public function testGetSourceDisplayName(): void {

lib/private/Sharing/SharingManager.php

@@ -259,14 +259,23 @@ public function updateShareUserStatus(ShareAccessContext $accessContext, Share $
 
 	#[\Override]
 	public function addShareSource(ShareAccessContext $accessContext, Share $share, ShareSource $source): Share {
+		if ($share->owner->instance !== null) {
+			throw new ShareOperationForbiddenException();
+		}
+
 		// only the owner can add sources, otherwise a user could add sources others don't have access to, which would remove their access
 		$this->validateShareEditPermissions($accessContext, $share, true);
 
 		if (($sourceType = $this->registry->getSourceTypes()[$source->class] ?? null) === null) {
 			throw new RuntimeException('The source type is not registered: ' . $source->class);
 		}
 
-		if (!$sourceType->validateSource($source->value)) {
+		$ownerUser = $this->userManager->get($share->owner->userId);
+		if (!$ownerUser instanceof IUser) {
+			throw new RuntimeException('Owner does not exist.');
+		}
+
+		if (!$sourceType->validateSource($ownerUser, $source->value)) {
 			throw new ShareInvalidException('Invalid source: ' . $source->value . ' ' . $source->class, $this->l10n->t('The source does not exist.'));
 		}
 
@@ -870,11 +879,17 @@ private function validateInteraction(ShareAccessContext $accessContext, Share $s
 			null, array_values(array_map(static fn (SharePermission $permission): string => $permission->class, $share->getEffectiveEnabledPermissions($accessContext)))
 		);
 
-		$usersToCheck = [];
-		if ($share->owner->instance === null && ($ownerUser = $this->userManager->get($share->owner->userId)) instanceof IUser) {
-			$usersToCheck[] = $ownerUser;
+		if ($share->owner->instance !== null) {
+			throw new ShareOperationForbiddenException();
+		}
+
+		$ownerUser = $this->userManager->get($share->owner->userId);
+		if (!$ownerUser instanceof IUser) {
+			throw new RuntimeException('Owner does not exist.');
 		}
 
+		$usersToCheck = [$ownerUser];
+
 		if ($accessContext->currentUser instanceof IUser && !$share->owner->isCurrentUser($accessContext)) {
 			$usersToCheck[] = $accessContext->currentUser;
 		}
@@ -902,7 +917,7 @@ private function validateInteraction(ShareAccessContext $accessContext, Share $s
 					throw new RuntimeException('The source type is not registered: ' . $source->class);
 				}
 
-				if (!$sourceType->validateSource($source->value)) {
+				if (!$sourceType->validateSource($ownerUser, $source->value)) {
 					continue;
 				}
 

lib/unstable/Sharing/Source/IShareSourceType.php

@@ -28,14 +28,12 @@ interface IShareSourceType {
 	public function getDisplayName(IFactory $l10nFactory): string;
 
 	/**
-	 * Validate that a source exists.
-	 *
-	 * Any check if the source is allowed to be accessed and shared, must be implemented through {@see RestrictInteractionEvent}.
+	 * Validate that a source exists and is accessible by the owner.
 	 *
 	 * @param non-empty-string $source
 	 * @experimental 35.0.0
 	 */
-	public function validateSource(string $source): bool;
+	public function validateSource(IUser $owner, string $source): bool;
 
 	/**
 	 * @param non-empty-string $source

tests/lib/Sharing/TestShareSourceType1.php

@@ -34,7 +34,7 @@ public function getDisplayName(IFactory $l10nFactory): string {
 	}
 
 	#[\Override]
-	public function validateSource(string $source): bool {
+	public function validateSource(IUser $owner, string $source): bool {
 		return array_key_exists($source, $this->validSources);
 	}