diff --git a/apps/dav/appinfo/v2/publicremote.php b/apps/dav/appinfo/v2/publicremote.php index ca2fb9250ddff..4a8604f441758 100644 --- a/apps/dav/appinfo/v2/publicremote.php +++ b/apps/dav/appinfo/v2/publicremote.php @@ -88,7 +88,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, $linkCheckPlugin, $filesDropPlugin) { +$server = $serverFactory->createServer(true, $baseuri, $requestUri, $authPlugin, function (\Sabre\DAV\Server $server) use ($authBackend, $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 @@ -103,18 +103,19 @@ $share = $authBackend->getShare(); $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 ($mountPoint, $storage) use ($requestUri, $baseuri, $share) { + Filesystem::addStorageWrapper('sharePermissions', function ($mountPoint, $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 912e5800aec1a..f5206181f795e 100644 --- a/apps/dav/lib/Connector/Sabre/PublicAuth.php +++ b/apps/dav/lib/Connector/Sabre/PublicAuth.php @@ -106,7 +106,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); @@ -114,7 +115,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 2f18b4305087e..24aef0efa5c1a 100644 --- a/apps/dav/tests/unit/Connector/Sabre/PublicAuthTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/PublicAuthTest.php @@ -74,7 +74,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'); @@ -82,8 +82,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('getPassword')->willReturn('password'); + + $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); @@ -91,7 +127,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); @@ -107,7 +143,7 @@ public function testCheckTokenValidShare(): void { } public function testCheckTokenInvalidShare(): void { - $this->request->method('getPathInfo') + $this->request->method('getRawPathInfo') ->willReturn('/dav/files/GX9HSGQrGE'); $this->shareManager @@ -121,7 +157,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); @@ -140,7 +176,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); @@ -159,7 +195,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); @@ -179,7 +215,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()) @@ -193,7 +229,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); @@ -210,7 +246,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); @@ -229,7 +265,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); @@ -247,7 +283,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); @@ -271,7 +307,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); @@ -295,7 +331,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); @@ -324,7 +360,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); @@ -354,7 +390,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);