diff --git a/CHANGELOG.md b/CHANGELOG.md index 949fbf82..02f49d0f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Registry mappings of a deploy daemon (`occ app_api:daemon:registry:add`) are applied by the Kubernetes backend too; before, ExApp images were always taken from the registry in `info.xml`. A `local` mapping sets `imagePullPolicy: Never` with HaRP newer than 0.4.5, so the image must be present on every node. #1046 - Updating a deploy daemon that does not exist answers `success: false` instead of a server error. #1082 +- The ExApp proxy also refuses `.` path segments and `..` segments followed by `;`, `%3F` or `%23`. #1163 ## [34.0.0] diff --git a/lib/Controller/ExAppProxyController.php b/lib/Controller/ExAppProxyController.php index 87989b3c..7f45e293 100644 --- a/lib/Controller/ExAppProxyController.php +++ b/lib/Controller/ExAppProxyController.php @@ -239,9 +239,10 @@ private function prepareProxy( string $appId, string $other, array &$route, array &$bruteforceProtection, int &$delay, ): ?ExApp { $delay = 0; - if (preg_match('#(?:^|/|%2[fF])(?:\.|%2[eE]){2}(?:/|%2[fF]|$)#', $other) === 1) { + // The forwarded path ends at "?" or "#", curl resolves dot segments, Tomcat drops ";" parameters first. + if (preg_match('#(?:^|/|%2[fF])(?:\.|%2[eE]){1,2}(?:[/;?\#]|%2[fF]|$)#', $other) === 1) { $this->logger->debug( - sprintf('Returning status 404 for "%s": path contains a parent-directory segment.', $other) + sprintf('Returning status 404 for "%s": path contains a dot segment.', $other) ); return null; } diff --git a/tests/php/Controller/ExAppProxyControllerTest.php b/tests/php/Controller/ExAppProxyControllerTest.php index 5ab88518..eefef966 100644 --- a/tests/php/Controller/ExAppProxyControllerTest.php +++ b/tests/php/Controller/ExAppProxyControllerTest.php @@ -18,6 +18,7 @@ use OCP\IGroupManager; use OCP\IRequest; use OCP\Security\Bruteforce\IThrottler; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; use ReflectionMethod; @@ -29,18 +30,17 @@ class ExAppProxyControllerTest extends TestCase { ['url' => '^/settings$', 'verb' => 'POST', 'access_level' => 2], ]; - /** - * The route the proxy picks for this request, or [] when it refuses it. - */ - private function routeFor(string $method, string $path, array $routes, ?string $userId = null, bool $isAdmin = false): array { + private function controller(string $method, ?string $userId = null, bool $isAdmin = false, ?ExApp $exApp = null): ExAppProxyController { $request = $this->createMock(IRequest::class); $request->method('getMethod')->willReturn($method); + $exAppService = $this->createMock(ExAppService::class); + $exAppService->method('getExApp')->willReturn($exApp); $groupManager = $this->createMock(IGroupManager::class); $groupManager->method('isAdmin')->willReturn($isAdmin); - $controller = new ExAppProxyController( + return new ExAppProxyController( $request, $this->createMock(AppAPIService::class), - $this->createMock(ExAppService::class), + $exAppService, $this->createMock(IMimeTypeDetector::class), $this->createMock(ContentSecurityPolicyNonceManager::class), $userId, @@ -48,10 +48,30 @@ private function routeFor(string $method, string $path, array $routes, ?string $ $this->createMock(LoggerInterface::class), $this->createMock(IThrottler::class), ); + } + + /** + * The route the proxy picks for this request, or [] when it refuses it. + */ + private function routeFor(string $method, string $path, array $routes, ?string $userId = null, bool $isAdmin = false): array { + $controller = $this->controller($method, $userId, $isAdmin); $exApp = new ExApp(['appid' => 'test-proxy', 'routes' => $routes]); return (new ReflectionMethod($controller, 'passesExAppProxyRoutesChecks'))->invoke($controller, $exApp, $path); } + /** + * Whether the proxy forwards a public GET for this path to an ExApp with a catch-all route. + */ + private function isForwarded(string $path): bool { + $exApp = new ExApp(['appid' => 'test-proxy', 'enabled' => 1, 'routes' => [['url' => '.*', 'verb' => 'GET', 'access_level' => 0]]]); + $controller = $this->controller('GET', exApp: $exApp); + $route = []; + $bruteforceProtection = []; + $delay = 0; + $args = ['test-proxy', $path, &$route, &$bruteforceProtection, &$delay]; + return (new ReflectionMethod($controller, 'prepareProxy'))->invokeArgs($controller, $args) !== null; + } + public function testFirstRouteMatchingPathAndVerbDecides(): void { self::assertSame('GET', $this->routeFor('GET', 'settings', self::SAME_URL)['verb']); self::assertSame([], $this->routeFor('POST', 'settings', self::SAME_URL)); @@ -83,4 +103,42 @@ public function testVerbAsAppApiStoresIt(): void { self::assertSame('^/z$', $this->routeFor('POST', 'z', $routes)['url']); self::assertSame([], $this->routeFor('PUT', 'z', $routes)); } + + public static function dotSegmentProvider(): array { + return [ + 'in the middle' => ['a/../b'], + 'at the end' => ['a/..'], + 'percent-encoded in the middle' => ['a/%2e%2e/b'], + 'percent-encoded at the end' => ['a/%2E%2e'], + 'with an empty parameter in the middle' => ['a/..;/b'], + 'with an empty parameter at the end' => ['a/..;'], + 'with a parameter in the middle' => ['a/..;x=1/b'], + 'with a parameter at the end' => ['a/..;x=1'], + 'followed by a decoded "?"' => ['a/..?/b'], + 'followed by a decoded "#"' => ['a/..#x'], + 'current directory in the middle' => ['a/./b'], + 'current directory at the start' => ['./a'], + 'current directory with a parameter' => ['a/.;/b'], + 'percent-encoded current directory' => ['a/%2e/b'], + ]; + } + + #[DataProvider('dotSegmentProvider')] + public function testDotSegmentIsRefused(string $path): void { + self::assertFalse($this->isForwarded($path)); + } + + public static function forwardedPathProvider(): array { + return [ + 'dots inside a segment' => ['a..b'], + 'dots starting a segment' => ['..foo'], + 'path parameter' => ['file;v=1'], + 'dot starting a directory' => ['.well-known/x'], + ]; + } + + #[DataProvider('forwardedPathProvider')] + public function testPathIsForwarded(string $path): void { + self::assertTrue($this->isForwarded($path)); + } }