diff --git a/CHANGELOG.md b/CHANGELOG.md index 663458dc..949fbf82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - `POST /ocs/v2.php/apps/app_api/api/v1/notification` now answers `404` when the `notifications` app is not enabled, instead of `200` for a notification that nobody delivers. - `occ app_api:daemon:register` and the admin settings refuse a HaRP or HTTPS daemon key shorter than 12 characters. #1082 - The daemon templates in the admin settings no longer prefill the HaRP shared key or HaProxy password. #1083 +- HaRP receives the verb of each ExApp route, so HaRP 0.4.8 and newer check the HTTP method of a request as the ExApp proxy does. #1138 +- The ExApp proxy accepts `HEAD` on routes that list `GET` and reads a route's verb as a list of methods, as HaRP does. #1144 ### Fixed diff --git a/lib/Controller/ExAppProxyController.php b/lib/Controller/ExAppProxyController.php index be13b333..87989b3c 100644 --- a/lib/Controller/ExAppProxyController.php +++ b/lib/Controller/ExAppProxyController.php @@ -320,6 +320,7 @@ private function buildMultipartFormData(array $bodyParams, array $files): array private function passesExAppProxyRoutesChecks(ExApp $exApp, string $exAppRoute): 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'; $matched = preg_match($pattern, $canonicalSubject) === 1; @@ -331,9 +332,7 @@ private function passesExAppProxyRoutesChecks(ExApp $exApp, string $exAppRoute): )); $matched = true; } - if ($matched - && str_contains(strtolower($route['verb']), strtolower($this->request->getMethod())) - ) { + 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 : []; } @@ -341,6 +340,14 @@ private function passesExAppProxyRoutesChecks(ExApp $exApp, string $exAppRoute): return []; } + /** + * As in HaRP: the verb lists methods separated by commas or whitespace, and HEAD is accepted wherever GET is. + */ + private static function routeAcceptsMethod(string $verb, string $method): bool { + $methods = array_map('strtoupper', preg_split('/[\s,]+/', $verb, -1, PREG_SPLIT_NO_EMPTY) ?: []); + return in_array($method, $methods, true) || ($method === 'HEAD' && in_array('GET', $methods, true)); + } + private function passesExAppProxyRouteAccessLevelCheck(int $accessLevel): bool { return match ($accessLevel) { ExAppRouteAccessLevel::PUBLIC->value => true, diff --git a/tests/php/Controller/ExAppProxyControllerTest.php b/tests/php/Controller/ExAppProxyControllerTest.php new file mode 100644 index 00000000..5ab88518 --- /dev/null +++ b/tests/php/Controller/ExAppProxyControllerTest.php @@ -0,0 +1,86 @@ + '^/settings$', 'verb' => 'GET', 'access_level' => 0], + ['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 { + $request = $this->createMock(IRequest::class); + $request->method('getMethod')->willReturn($method); + $groupManager = $this->createMock(IGroupManager::class); + $groupManager->method('isAdmin')->willReturn($isAdmin); + $controller = new ExAppProxyController( + $request, + $this->createMock(AppAPIService::class), + $this->createMock(ExAppService::class), + $this->createMock(IMimeTypeDetector::class), + $this->createMock(ContentSecurityPolicyNonceManager::class), + $userId, + $groupManager, + $this->createMock(LoggerInterface::class), + $this->createMock(IThrottler::class), + ); + $exApp = new ExApp(['appid' => 'test-proxy', 'routes' => $routes]); + return (new ReflectionMethod($controller, 'passesExAppProxyRoutesChecks'))->invoke($controller, $exApp, $path); + } + + public function testFirstRouteMatchingPathAndVerbDecides(): void { + self::assertSame('GET', $this->routeFor('GET', 'settings', self::SAME_URL)['verb']); + self::assertSame([], $this->routeFor('POST', 'settings', self::SAME_URL)); + self::assertSame([], $this->routeFor('POST', 'settings', self::SAME_URL, 'alice')); + self::assertSame('POST', $this->routeFor('POST', 'settings', self::SAME_URL, 'admin', true)['verb']); + self::assertSame([], $this->routeFor('PUT', 'settings', self::SAME_URL, 'admin', true)); + } + + public function testAccessDeniedDoesNotFallThroughToLaterRoutes(): void { + // An admin route listed before a broader user route, as some ExApps declare their setup pages. + $adminFirst = [ + ['url' => '^/setup$', 'verb' => 'GET,POST', 'access_level' => 2], + ['url' => '^/.*', 'verb' => 'GET,POST', 'access_level' => 1], + ]; + self::assertSame([], $this->routeFor('GET', 'setup', $adminFirst, 'alice')); + self::assertSame('^/.*', $this->routeFor('GET', 'other', $adminFirst, 'alice')['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']); + self::assertSame([], $this->routeFor('HEAD', 'form', [['url' => '^/form$', 'verb' => 'POST', 'access_level' => 0]])); + } + + public function testVerbAsAppApiStoresIt(): void { + // AppAPI stores the text of as it is in info.xml, line breaks and case included. + $routes = [['url' => '^/z$', 'verb' => "\n get, POST\n ", 'access_level' => 0]]; + self::assertSame('^/z$', $this->routeFor('GET', 'z', $routes)['url']); + self::assertSame('^/z$', $this->routeFor('POST', 'z', $routes)['url']); + self::assertSame([], $this->routeFor('PUT', 'z', $routes)); + } +}