From 684a475aa70c38dd9385f83823596268b582a93b 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 9292d40927628..d8db03cb030d3 100644 --- a/apps/dav/lib/Upload/RootCollection.php +++ b/apps/dav/lib/Upload/RootCollection.php @@ -13,6 +13,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; @@ -34,7 +36,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 90174ba52a8901d6049425f553ae2e33f9f09311 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 ca617eb326d2a09ae5bc79ee0781e54ffe785308 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 7e910e1824639..2053b62315ff4 100644 --- a/build/integration/features/bootstrap/WebDav.php +++ b/build/integration/features/bootstrap/WebDav.php @@ -878,6 +878,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 */