From 476d2d0b8f6aef9d7161c96ebc02a82f40f0fc67 Mon Sep 17 00:00:00 2001 From: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> Date: Thu, 27 Aug 2026 10:23:15 +0200 Subject: [PATCH 1/3] fix: fail uploads when uri does not match session This is a cosmetic change that can cause false security reports: the current implementation of the upload root collection returns UploadHome in every case, based on the current session, either the one of the logged in user, or for the share. The former allows uploading files in what looks like the upload folder of another user, but is in reality the one of the logged in user. Those requests will now fail with a 403 instead. Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> --- apps/dav/lib/Upload/RootCollection.php | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/apps/dav/lib/Upload/RootCollection.php b/apps/dav/lib/Upload/RootCollection.php index 9f722dc98a7e8..08c620e5ce222 100644 --- a/apps/dav/lib/Upload/RootCollection.php +++ b/apps/dav/lib/Upload/RootCollection.php @@ -12,6 +12,8 @@ use OCP\Files\IRootFolder; use OCP\IUserSession; use OCP\Share\IManager; +use Sabre\DAV\Exception\Forbidden; +use Sabre\DAV\INode; use Sabre\DAVACL\AbstractPrincipalCollection; use Sabre\DAVACL\PrincipalBackend; @@ -33,7 +35,14 @@ public function __construct( * @inheritdoc */ #[\Override] - public function getChildForPrincipal(array $principalInfo): UploadHome { + public function getChildForPrincipal(array $principalInfo): INode|UploadHome { + [$prefix, $name] = \Sabre\Uri\split($principalInfo['uri']); + $user = $this->userSession->getUser(); + if ($prefix !== 'principals/shares' && $user?->getUID() !== $name) { + // if the request is not using a share token and the URL does not match the user, error out + throw new Forbidden('Not allowed'); + } + return new UploadHome( $principalInfo, $this->cleanupService, From c2c243bc60b826542adfa3c58a78e3eed8632e04 Mon Sep 17 00:00:00 2001 From: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> Date: Tue, 1 Sep 2026 15:41:27 +0200 Subject: [PATCH 2/3] test: add unit test for upload RootCollection Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> --- .../tests/unit/Upload/RootCollectionTest.php | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 apps/dav/tests/unit/Upload/RootCollectionTest.php diff --git a/apps/dav/tests/unit/Upload/RootCollectionTest.php b/apps/dav/tests/unit/Upload/RootCollectionTest.php new file mode 100644 index 0000000000000..28d8d315ef797 --- /dev/null +++ b/apps/dav/tests/unit/Upload/RootCollectionTest.php @@ -0,0 +1,95 @@ +principalBackend = $this->createMock(BackendInterface::class); + $this->cleanupService = $this->createMock(CleanupService::class); + $this->rootFolder = $this->createMock(IRootFolder::class); + $this->userSession = $this->createMock(IUserSession::class); + $this->shareManager = $this->createMock(IShareManager::class); + + $this->collection = new RootCollection( + $this->principalBackend, + 'principals/users', + $this->cleanupService, + $this->rootFolder, + $this->userSession, + $this->shareManager, + ); + } + + private function mockUser(string $uid): IUser&MockObject { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn($uid); + return $user; + } + + public function testGetChildForPrincipalReturnsUploadHomeForOwnPrincipal(): void { + $this->userSession->method('getUser')->willReturn($this->mockUser('alice')); + + $node = $this->collection->getChildForPrincipal(['uri' => 'principals/users/alice']); + + $this->assertInstanceOf(UploadHome::class, $node); + } + + public function testGetChildForPrincipalReturnsUploadHomeForShareToken(): void { + $this->userSession->method('getUser')->willReturn($this->mockUser('alice')); + + $share = $this->createMock(IShare::class); + $share->method('getShareOwner')->willReturn('bob'); + $this->shareManager->method('getShareByToken') + ->with('sometoken') + ->willReturn($share); + + $node = $this->collection->getChildForPrincipal(['uri' => 'principals/shares/sometoken']); + + $this->assertInstanceOf(UploadHome::class, $node); + } + + public function testGetChildForPrincipalThrowsWhenPrincipalDoesNotMatchUser(): void { + $this->userSession->method('getUser')->willReturn($this->mockUser('alice')); + + $this->expectException(Forbidden::class); + + $this->collection->getChildForPrincipal(['uri' => 'principals/users/bob']); + } + + public function testGetChildForPrincipalThrowsWhenNotLoggedIn(): void { + $this->userSession->method('getUser')->willReturn(null); + + $this->expectException(Forbidden::class); + + $this->collection->getChildForPrincipal(['uri' => 'principals/users/alice']); + } +} From b7a11155b1a41c7a6d113e12b500c74364a8bee5 Mon Sep 17 00:00:00 2001 From: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> Date: Tue, 1 Sep 2026 15:41:45 +0200 Subject: [PATCH 3/3] test: add integration test for upload case Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> --- .../integration/dav_features/webdav-related.feature | 8 ++++++++ build/integration/features/bootstrap/WebDav.php | 12 ++++++++++++ 2 files changed, 20 insertions(+) diff --git a/build/integration/dav_features/webdav-related.feature b/build/integration/dav_features/webdav-related.feature index ef5a4fa7d4717..3403caa0a8184 100644 --- a/build/integration/dav_features/webdav-related.feature +++ b/build/integration/dav_features/webdav-related.feature @@ -424,6 +424,14 @@ Feature: webdav-related And Downloading file "/myChunkedFile.txt" Then Downloaded content should be "AAAAABBBBBCCCCC" + Scenario: Cannot create a chunked upload in another user's uploads folder + Given using new dav path + And user "user0" exists + And user "user1" exists + And As an "user1" + When user "user1" creates a new chunking upload with id "chunking-42" in the uploads folder for "user0" + Then the HTTP status code should be "403" + Scenario: A disabled user cannot use webdav Given user "userToBeDisabled" exists And As an "admin" diff --git a/build/integration/features/bootstrap/WebDav.php b/build/integration/features/bootstrap/WebDav.php index 83f9778cee3d8..dc64680d90c04 100644 --- a/build/integration/features/bootstrap/WebDav.php +++ b/build/integration/features/bootstrap/WebDav.php @@ -879,6 +879,18 @@ public function userCreatesANewChunkingUploadWithId($user, $id) { $this->makeDavRequest($user, 'MKCOL', $destination, [], null, 'uploads'); } + /** + * @When user :user creates a new chunking upload with id :id in the uploads folder for :uidOrToken + */ + public function userCreatesANewChunkingUploadWithIdInFolderOf($user, $id, $uidOrToken): void { + $destination = '/uploads/' . $uidOrToken . '/' . $id; + try { + $this->response = $this->makeDavRequest($user, 'MKCOL', $destination, [], null, 'uploads'); + } catch (\GuzzleHttp\Exception\ClientException $e) { + $this->response = $e->getResponse(); + } + } + /** * @Given user :user uploads new chunk file :num with :data to id :id */