diff --git a/CHANGELOG.md b/CHANGELOG.md index 7de55fe6..a7d00e69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - 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 - The ExApp proxy refuses requests to the `/heartbeat`, `/init` and `/enabled` endpoints of ExApps, as HaRP does. #1168 +- The ExApp proxy also applies a stricter route that matches the path as an ExApp may read it, as HaRP 0.4.8 does. #1182 ## [34.0.0] diff --git a/lib/Controller/ExAppProxyController.php b/lib/Controller/ExAppProxyController.php index 2f33d992..5798dc3c 100644 --- a/lib/Controller/ExAppProxyController.php +++ b/lib/Controller/ExAppProxyController.php @@ -239,16 +239,16 @@ private function prepareProxy( string $appId, string $other, array &$route, array &$bruteforceProtection, int &$delay, ): ?ExApp { $delay = 0; - // 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) { + // A dot segment curl resolves, also before ";" (Tomcat drops parameters), "?" or "#" (they end the path), raw or as an escape a proxy or wrapper ExApp decodes. + if (preg_match('#(?:^|/|%2[fF])(?:\.|%2[eE]){1,2}(?:[/;?\#]|%2[fF]|%3[bBfF]|%23|$)#', $other) === 1) { $this->logger->debug( sprintf('Returning status 404 for "%s": path contains a dot segment.', $other) ); return null; } - // As the ExApp may read it: cut at "?" or "#", decoded once, any case, surrounding slashes and newlines. - $target = rawurldecode(substr($other, 0, strcspn($other, '?#'))); - if (in_array(strtolower(trim($target, "/\n")), ['heartbeat', 'init', 'enabled'], true)) { + // In any reading of the path, any case, without surrounding slashes and newlines. + $targets = array_map(static fn (string $path): string => strtolower(trim($path, "/\n")), self::exAppPathReadings($other)); + if (array_intersect($targets, ['heartbeat', 'init', 'enabled']) !== []) { $this->logger->debug( sprintf('Returning status 404 for "%s": only AppAPI calls this ExApp endpoint.', $other) ); @@ -327,13 +327,31 @@ private function buildMultipartFormData(array $bodyParams, array $files): array } private function passesExAppProxyRoutesChecks(ExApp $exApp, string $exAppRoute): array { + $readings = self::exAppPathReadings($exAppRoute); + $route = $this->firstExAppRoute($exApp, $readings[0], 'i'); + if ($route === [] || !$this->passesExAppProxyRouteAccessLevelCheck($route['access_level'])) { + return []; + } + // As in HaRP, the route of another reading of the path or another letter case can only raise the access level. + foreach ($readings as $path) { + foreach (['', 'i'] as $flags) { + $candidate = $this->firstExAppRoute($exApp, $path, $flags); + if ($candidate !== [] && $candidate['access_level'] > $route['access_level']) { + $route = $candidate; + } + } + } + // Apply the access level of the decided route without falling through to broader routes. + return $this->passesExAppProxyRouteAccessLevelCheck($route['access_level']) ? $route : []; + } + + private function firstExAppRoute(ExApp $exApp, string $canonicalSubject, string $flags): array { // Route URL is a regex matched against the request path including its leading slash, mirroring HaRP's target_path semantics. - $canonicalSubject = '/' . $exAppRoute; $method = strtoupper($this->request->getMethod()); foreach ($exApp->getRoutes() as $route) { - $pattern = '~^(?:' . str_replace('~', '\\~', $route['url']) . ')~i'; + $pattern = '~^(?:' . str_replace('~', '\\~', $route['url']) . ')~' . $flags; $matched = preg_match($pattern, $canonicalSubject) === 1; - if (!$matched && preg_match($pattern, $exAppRoute) === 1) { + if (!$matched && preg_match($pattern, substr($canonicalSubject, 1)) === 1) { // TODO(deprecation): remove this bare-path fallback once known ExApps have migrated their info.xml route URLs to the canonical `^/path$` form. Tracked by the AppAPI / context_chat_backend coordination effort. $this->logger->debug(sprintf( 'ExApp "%s" matched route "%s" via legacy bare-path fallback. Update the info.xml route URL to start with "/" or "^/" so it matches against "%s".', @@ -342,13 +360,43 @@ private function passesExAppProxyRoutesChecks(ExApp $exApp, string $exAppRoute): $matched = true; } if ($matched && self::routeAcceptsMethod($route['verb'], $method)) { - // First match by path+verb wins. Apply its access level without falling through to broader routes. - return $this->passesExAppProxyRouteAccessLevelCheck($route['access_level']) ? $route : []; + return $route; } } return []; } + /** + * The path as checked, then as ExApp servers read the forwarded one, like HaRP's exapp_paths. + */ + private static function exAppPathReadings(string $other): array { + $path = '/' . $other; + $sent = substr($path, 0, strcspn($path, '?#')); // the forwarded path ends at "?" or "#" + $decoded = rawurldecode($sent); + $resent = substr($decoded, 0, strcspn($decoded, '?#')); // a wrapper ExApp re-sending the decoded path cuts it there + $readings = [ + $path, + $sent, + $decoded, // most servers decode the path before routing + preg_replace('#//+#', '/', $decoded), + rawurldecode(str_ireplace('%2F', '%252F', $sent)), // Express, aiohttp and Go keep "%2F" in its segment + preg_replace('#//+#', '/', rawurldecode(str_ireplace('%2F', '%252F', $sent))), // the same with "//" merged, as HaRP reads it + preg_replace('#//+#', '/', rawurldecode(preg_replace('#;[^/]*#', '', $sent))), // Tomcat drops ";" parameters first + preg_replace('#//+#', '/', preg_replace('#;[^/]*#', '', $decoded)), // Tomcat behind a proxy that decodes the path, such as nginx with a URI in proxy_pass + $resent, + preg_replace('#//+#', '/', $resent), // nginx and Go behind such a wrapper merge "//" + preg_replace('#//+#', '/', preg_replace('#;[^/]*#', '', $resent)), // Tomcat behind such a wrapper + rawurldecode(preg_replace('#^//+[^/]*(?:/|$)#', '/', $sent)), // WHATWG and Python URL parsers read a leading "//x" as a host + rawurldecode(preg_replace('#^//+[^/]*(?:/|$)#', '/', str_ireplace('%2F', '%252F', $sent))), // and keep "%2F" in its segment + ]; + foreach ($readings as $reading) { + if (strlen($reading) > 1 && str_ends_with($reading, '/')) { + $readings[] = substr($reading, 0, -1); // Express and Spring 5 ignore a trailing slash + } + } + return array_unique($readings); + } + /** * As in HaRP: the verb lists methods separated by commas or whitespace, and HEAD is accepted wherever GET is. */ diff --git a/tests/php/Controller/ExAppProxyControllerTest.php b/tests/php/Controller/ExAppProxyControllerTest.php index 84e9aa3a..db14eebe 100644 --- a/tests/php/Controller/ExAppProxyControllerTest.php +++ b/tests/php/Controller/ExAppProxyControllerTest.php @@ -102,6 +102,8 @@ public static function internalEndpointProvider(): array { 'followed by a decoded "?"' => ['enabled?enabled=0'], 'followed by a decoded "#"' => ['init#x'], 'followed by a newline' => ["heartbeat\n"], + 'with a ";" parameter, dropped by Tomcat' => ['heartbeat;x'], + 'followed by an escaped "?", where a wrapper ExApp cuts the decoded path' => ['enabled%3Fenabled=0'], ]; } @@ -115,6 +117,53 @@ public function testPathsNearInternalEndpointsAreForwarded(): void { self::assertTrue($this->isForwarded('api/enabled')); } + public static function stricterReadingProvider(): array { + // The path, and the stricter route that only the ExApp's reading of it matches. + return [ + '";" parameter, dropped by Tomcat' => ['admin;x/a.txt', '^/admin/.*'], + '";" parameter, dropped by Tomcat before it decodes' => ['admin;x%3F/a.txt', '^/admin/.*'], + 'escaped ";" and "?", decoded by nginx with a URI in proxy_pass and dropped by Tomcat' => ['%3Bx%3Fy/admin', '^/admin$'], + 'decoded "?", where the forwarded path ends' => ['admin?x', '^/admin$'], + 'decoded "#", where the forwarded path ends' => ['admin#x', '^/admin$'], + 'escape, decoded by the ExApp' => ['%61dmin', '^/admin$'], + 'escaped "?", kept in the name by Starlette' => ['%66iles/a%3F.txt', '^/files/.+\.txt$'], + '"//", merged by Tomcat, nginx and Go' => ['admin//x', '^/admin/x$'], + '"//" with a ";" and an escaped "?" in the name, merged by nginx' => ['files//a;b%3F.txt', '^/files/[^/]+\.txt$'], + 'leading "//x" with an escaped "/", read as a host by Python\'s urlsplit before it decodes' => ['/x%2Fy/admin%2Fx', '^/admin/x$'], + 'leading "//x", with "%2F" kept in its segment by URL parsers' => ['/x/files/a%2Fb', '^/files/[^/]+$'], + 'escaped "/", kept in its segment by aiohttp and Go' => ['%61dmin/a%2Fb', '^/admin/[^/]+$'], + 'escaped "/" kept in its segment, with "//" merged' => ['//files/a.txt%2Fa.txt', '^/files/[^/]+\.txt$'], + 'trailing slash, ignored by Express' => ['admin/', '^/admin$'], + 'trailing slash and another letter case, both ignored by Express' => ['Admin/', '^/admin$'], + 'escaped "?", where a wrapper ExApp cuts the decoded path it re-sends' => ['admin%3Fx', '^/admin$'], + 'escaped "?" after a ";", where a wrapper ExApp cuts the decoded path it re-sends' => ['files/a;b.txt%3Fz', '^/files/.+\.txt$'], + 'escaped "/" and "?" after a ";", where a wrapper ExApp re-sends the decoded path to nginx' => ['files/%2Fa;b.txt%3Fz', '^/files/[^/]+\.txt$'], + 'escaped ";", which a wrapper ExApp re-sends to Tomcat' => ['admin%3Bx/a.txt', '^/admin/.*'], + 'decoded "?", with a route URL without a leading slash' => ['admin?x', '^admin$'], + ]; + } + + #[DataProvider('stricterReadingProvider')] + public function testStricterRouteOfAnotherReadingApplies(string $path, string $adminUrl): void { + $routes = [ + ['url' => $adminUrl, 'verb' => 'GET', 'access_level' => 2], + ['url' => '.*', 'verb' => 'GET', 'access_level' => 0], + ]; + self::assertSame([], $this->routeFor('GET', $path, $routes)); + self::assertSame($adminUrl, $this->routeFor('GET', $path, $routes, 'admin', true)['url']); + } + + public function testRouteOfAnotherLetterCaseCanOnlyRaiseTheAccessLevel(): void { + // A case-sensitive ExApp serves "/PUB/x" with its user route. + $routes = [ + ['url' => '^/pub/.*', 'verb' => 'GET', 'access_level' => 0], + ['url' => '.*', 'verb' => 'GET', 'access_level' => 1], + ]; + self::assertSame([], $this->routeFor('GET', 'PUB/x', $routes)); + self::assertSame('.*', $this->routeFor('GET', 'PUB/x', $routes, 'alice')['url']); + self::assertSame('^/pub/.*', $this->routeFor('GET', 'PUB/x', [$routes[0]])['url']); + } + public function testHeadIsAcceptedWhereGetIs(): void { // Nextcloud routes HEAD to the proxy's GET action, and HaRP accepts HEAD on GET routes too. self::assertSame('GET', $this->routeFor('HEAD', 'settings', self::SAME_URL)['verb']); @@ -139,8 +188,11 @@ public static function dotSegmentProvider(): array { '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'], + 'with an escaped parameter, which a proxy decodes' => ['a/..%3Bx/b'], 'followed by a decoded "?"' => ['a/..?/b'], 'followed by a decoded "#"' => ['a/..#x'], + 'followed by an escaped "?", where a wrapper ExApp cuts the decoded path' => ['a/..%3Fx'], + 'followed by an escaped "#", where a wrapper ExApp cuts the decoded path' => ['a/..%23x'], 'current directory in the middle' => ['a/./b'], 'current directory at the start' => ['./a'], 'current directory with a parameter' => ['a/.;/b'],