From b86d25566049f5b84729f7a03a3ed5d1b97a708c Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Thu, 29 Oct 2026 18:41:55 +0100 Subject: [PATCH] fix(dav): align public request path handling Ensure consistent path handling for public DAV requests. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 Signed-off-by: Ghassen kefi --- apps/dav/appinfo/v2/publicremote.php | 7 +- apps/dav/lib/Connector/Sabre/PublicAuth.php | 5 +- .../unit/Connector/Sabre/PublicAuthTest.php | 68 ++++++++++++++----- 3 files changed, 59 insertions(+), 21 deletions(-) diff --git a/apps/dav/appinfo/v2/publicremote.php b/apps/dav/appinfo/v2/publicremote.php index 64d4a6ff74715..24b21520c4a9e 100644 --- a/apps/dav/appinfo/v2/publicremote.php +++ b/apps/dav/appinfo/v2/publicremote.php @@ -91,7 +91,7 @@ $filesDropPlugin = new FilesDropPlugin(); /** @var string $baseuri defined in public.php */ -$server = $serverFactory->createServer(true, $baseuri, $requestUri, $authPlugin, function (\Sabre\DAV\Server $server) use ($baseuri, $requestUri, $authBackend, $bearerAuthBackend, $linkCheckPlugin, $filesDropPlugin) { +$server = $serverFactory->createServer(true, $baseuri, $requestUri, $authPlugin, function (\Sabre\DAV\Server $server) use ($authBackend, $bearerAuthBackend, $linkCheckPlugin, $filesDropPlugin) { // GET must be allowed for e.g. showing images and allowing Zip downloads if ($server->httpRequest->getMethod() !== 'GET') { // If this is *not* a GET request we only allow access to public DAV from AJAX or when Server2Server is allowed @@ -110,18 +110,19 @@ } $isReadable = $share->getPermissions() & Constants::PERMISSION_READ; $fileId = $share->getNodeId(); + $isUpload = str_starts_with($server->httpRequest->getPath(), 'uploads/'); // FIXME: should not add storage wrappers outside of preSetup, need to find a better way /** @psalm-suppress InternalMethod */ $previousLog = Filesystem::logWarningWhenAddingStorageWrapper(false); /** @psalm-suppress MissingClosureParamType */ - Filesystem::addStorageWrapper('sharePermissions', function (string $mountPoint, IStorage $storage) use ($requestUri, $baseuri, $share) { + Filesystem::addStorageWrapper('sharePermissions', function (string $mountPoint, IStorage $storage) use ($isUpload, $share) { $mask = $share->getPermissions() | Constants::PERMISSION_SHARE; // For chunked uploads it is necessary to have read and delete permission, // so the temporary directory, chunks and destination file can be read and delete after the assembly. - if (str_starts_with(substr($requestUri, strlen($baseuri) - 1), '/uploads/')) { + if ($isUpload) { $mask |= Constants::PERMISSION_READ | Constants::PERMISSION_DELETE; } diff --git a/apps/dav/lib/Connector/Sabre/PublicAuth.php b/apps/dav/lib/Connector/Sabre/PublicAuth.php index 4649a9a00f40c..3e900c942ac86 100644 --- a/apps/dav/lib/Connector/Sabre/PublicAuth.php +++ b/apps/dav/lib/Connector/Sabre/PublicAuth.php @@ -107,7 +107,8 @@ public function check(RequestInterface $request, ResponseInterface $response): a * @throws NotFound */ private function getToken(): string { - $path = $this->request->getPathInfo() ?: ''; + // Normalized the same way as Sabre resolves the requested node + $path = \Sabre\Uri\normalize($this->request->getRawPathInfo()); // ['', 'dav', 'files', 'token'] $splittedPath = explode('/', $path); @@ -115,7 +116,7 @@ private function getToken(): string { throw new NotFound(); } - return $splittedPath[3]; + return rawurldecode($splittedPath[3]); } /** diff --git a/apps/dav/tests/unit/Connector/Sabre/PublicAuthTest.php b/apps/dav/tests/unit/Connector/Sabre/PublicAuthTest.php index e76fc347d9359..de64d29e992b3 100644 --- a/apps/dav/tests/unit/Connector/Sabre/PublicAuthTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/PublicAuthTest.php @@ -79,7 +79,7 @@ protected function tearDown(): void { } public function testGetToken(): void { - $this->request->method('getPathInfo') + $this->request->method('getRawPathInfo') ->willReturn('/dav/files/GX9HSGQrGE'); $result = self::invokePrivate($this->auth, 'getToken'); @@ -87,8 +87,44 @@ public function testGetToken(): void { $this->assertSame('GX9HSGQrGE', $result); } + public static function dataGetTokenNormalizesPath(): array { + return [ + 'parent segment' => ['/dav/uploads/tokenA/../tokenB/upload-id/1', 'tokenB'], + 'nested parent segments' => ['/dav/files/tokenA/../../uploads/tokenB/upload-id/1', 'tokenB'], + 'current segment' => ['/dav/uploads/./tokenB/upload-id/1', 'tokenB'], + 'encoded dots are not a parent segment' => ['/dav/uploads/%2e%2e/tokenB', '..'], + 'encoded token' => ['/dav/files/token%41', 'tokenA'], + ]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'dataGetTokenNormalizesPath')] + public function testGetTokenNormalizesPath(string $rawPath, string $expected): void { + $this->request->method('getRawPathInfo') + ->willReturn($rawPath); + + $this->assertSame($expected, self::invokePrivate($this->auth, 'getToken')); + } + + public function testCheckTokenUsesNormalizedToken(): void { + $this->request->method('getRawPathInfo') + ->willReturn('/dav/uploads/tokenA/../tokenB/upload-id/1'); + + $share = $this->createMock(IShare::class); + $share->method('isPasswordProtected')->willReturn(true); + + $this->shareManager->expects($this->once()) + ->method('getShareByToken') + ->with('tokenB') + ->willReturn($share); + + $this->session->method('exists')->with('public_link_authenticated')->willReturn(false); + + $this->expectException(\Sabre\DAV\Exception\NotAuthenticated::class); + self::invokePrivate($this->auth, 'checkToken'); + } + public function testGetTokenInvalid(): void { - $this->request->method('getPathInfo') + $this->request->method('getRawPathInfo') ->willReturn('/dav/files'); $this->expectException(\Sabre\DAV\Exception\NotFound::class); @@ -96,7 +132,7 @@ public function testGetTokenInvalid(): void { } public function testCheckTokenValidShare(): void { - $this->request->method('getPathInfo') + $this->request->method('getRawPathInfo') ->willReturn('/dav/files/GX9HSGQrGE'); $share = $this->createMock(IShare::class); @@ -113,7 +149,7 @@ public function testCheckTokenValidShare(): void { } public function testCheckTokenInvalidShare(): void { - $this->request->method('getPathInfo') + $this->request->method('getRawPathInfo') ->willReturn('/dav/files/GX9HSGQrGE'); $this->shareManager @@ -127,7 +163,7 @@ public function testCheckTokenInvalidShare(): void { } public function testCheckTokenAlreadyAuthenticated(): void { - $this->request->method('getPathInfo') + $this->request->method('getRawPathInfo') ->willReturn('/dav/files/GX9HSGQrGE'); $share = $this->createMock(IShare::class); @@ -146,7 +182,7 @@ public function testCheckTokenAlreadyAuthenticated(): void { } public function testCheckTokenPasswordNotAuthenticated(): void { - $this->request->method('getPathInfo') + $this->request->method('getRawPathInfo') ->willReturn('/dav/files/GX9HSGQrGE'); $share = $this->createMock(IShare::class); @@ -166,7 +202,7 @@ public function testCheckTokenPasswordNotAuthenticated(): void { } public function testCheckTokenPasswordAuthenticatedWrongShare(): void { - $this->request->method('getPathInfo') + $this->request->method('getRawPathInfo') ->willReturn('/dav/files/GX9HSGQrGE'); $share = $this->createMock(IShare::class); @@ -187,7 +223,7 @@ public function testCheckTokenPasswordAuthenticatedWrongShare(): void { } public function testNoShare(): void { - $this->request->method('getPathInfo') + $this->request->method('getRawPathInfo') ->willReturn('/dav/files/GX9HSGQrGE'); $this->shareManager->expects($this->once()) @@ -201,7 +237,7 @@ public function testNoShare(): void { } public function testShareNoPassword(): void { - $this->request->method('getPathInfo') + $this->request->method('getRawPathInfo') ->willReturn('/dav/files/GX9HSGQrGE'); $share = $this->createMock(IShare::class); @@ -219,7 +255,7 @@ public function testShareNoPassword(): void { } public function testSharePasswordFancyShareType(): void { - $this->request->method('getPathInfo') + $this->request->method('getRawPathInfo') ->willReturn('/dav/files/GX9HSGQrGE'); $share = $this->createMock(IShare::class); @@ -238,7 +274,7 @@ public function testSharePasswordFancyShareType(): void { } public function testSharePasswordRemote(): void { - $this->request->method('getPathInfo') + $this->request->method('getRawPathInfo') ->willReturn('/dav/files/GX9HSGQrGE'); $share = $this->createMock(IShare::class); @@ -257,7 +293,7 @@ public function testSharePasswordRemote(): void { } public function testSharePasswordLinkValidPassword(): void { - $this->request->method('getPathInfo') + $this->request->method('getRawPathInfo') ->willReturn('/dav/files/GX9HSGQrGE'); $share = $this->createMock(IShare::class); @@ -282,7 +318,7 @@ public function testSharePasswordLinkValidPassword(): void { } public function testSharePasswordMailValidPassword(): void { - $this->request->method('getPathInfo') + $this->request->method('getRawPathInfo') ->willReturn('/dav/files/GX9HSGQrGE'); $share = $this->createMock(IShare::class); @@ -307,7 +343,7 @@ public function testSharePasswordMailValidPassword(): void { } public function testInvalidSharePasswordLinkValidSession(): void { - $this->request->method('getPathInfo') + $this->request->method('getRawPathInfo') ->willReturn('/dav/files/GX9HSGQrGE'); $share = $this->createMock(IShare::class); @@ -337,7 +373,7 @@ public function testInvalidSharePasswordLinkValidSession(): void { } public function testSharePasswordLinkInvalidSession(): void { - $this->request->method('getPathInfo') + $this->request->method('getRawPathInfo') ->willReturn('/dav/files/GX9HSGQrGE'); $share = $this->createMock(IShare::class); @@ -367,7 +403,7 @@ public function testSharePasswordLinkInvalidSession(): void { } public function testSharePasswordMailInvalidSession(): void { - $this->request->method('getPathInfo') + $this->request->method('getRawPathInfo') ->willReturn('/dav/files/GX9HSGQrGE'); $share = $this->createMock(IShare::class);