From f146c5bb19c4a53f039729b58da5dd24725afd12 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Mon, 21 Sep 2026 18:20:46 +0200 Subject: [PATCH] fix(Sharing): Enforce source to be accessible by the owner Signed-off-by: provokateurin --- .../Sharing/Source/NodeShareSourceType.php | 9 +++++-- .../Source/NodeShareSourceTypeTest.php | 17 +++++++------ lib/private/Sharing/SharingManager.php | 25 +++++++++++++++---- .../Sharing/Source/IShareSourceType.php | 6 ++--- tests/lib/Sharing/TestShareSourceType1.php | 2 +- 5 files changed, 40 insertions(+), 19 deletions(-) diff --git a/apps/files/lib/Sharing/Source/NodeShareSourceType.php b/apps/files/lib/Sharing/Source/NodeShareSourceType.php index cabaaf2089f18..e2b10ca37a3aa 100644 --- a/apps/files/lib/Sharing/Source/NodeShareSourceType.php +++ b/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] diff --git a/apps/files/tests/Sharing/Source/NodeShareSourceTypeTest.php b/apps/files/tests/Sharing/Source/NodeShareSourceTypeTest.php index 6a1f93dc4952c..6d0b57cdb464e 100644 --- a/apps/files/tests/Sharing/Source/NodeShareSourceTypeTest.php +++ b/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 { diff --git a/lib/private/Sharing/SharingManager.php b/lib/private/Sharing/SharingManager.php index 6836ea596433f..328b919ec110f 100644 --- a/lib/private/Sharing/SharingManager.php +++ b/lib/private/Sharing/SharingManager.php @@ -259,6 +259,10 @@ 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); @@ -266,7 +270,12 @@ public function addShareSource(ShareAccessContext $accessContext, Share $share, 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; } diff --git a/lib/unstable/Sharing/Source/IShareSourceType.php b/lib/unstable/Sharing/Source/IShareSourceType.php index f4cf17ae7f697..fc6efd78cc304 100644 --- a/lib/unstable/Sharing/Source/IShareSourceType.php +++ b/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 diff --git a/tests/lib/Sharing/TestShareSourceType1.php b/tests/lib/Sharing/TestShareSourceType1.php index a189335f32913..219bfd4bf27a1 100644 --- a/tests/lib/Sharing/TestShareSourceType1.php +++ b/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); }