From 0659c6329827f96f6c8273ffb4ad3a73cc0d55a1 Mon Sep 17 00:00:00 2001 From: Oleksandr Piskun Date: Fri, 23 Oct 2026 12:06:48 +0000 Subject: [PATCH 1/2] fix(proxy): match the route verb like HaRP Signed-off-by: Oleksandr Piskun --- lib/Controller/ExAppProxyController.php | 13 +++- .../Controller/ExAppProxyControllerTest.php | 76 +++++++++++++++++++ 2 files changed, 86 insertions(+), 3 deletions(-) create mode 100644 tests/php/Controller/ExAppProxyControllerTest.php diff --git a/lib/Controller/ExAppProxyController.php b/lib/Controller/ExAppProxyController.php index be13b3330..87989b3ce 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 000000000..c6feb2ccb --- /dev/null +++ b/tests/php/Controller/ExAppProxyControllerTest.php @@ -0,0 +1,76 @@ + '^/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 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)); + } +} From 77b8c233438493828007ba3d384a09716e2ce294 Mon Sep 17 00:00:00 2001 From: Oleksandr Piskun Date: Fri, 23 Oct 2026 14:06:16 +0000 Subject: [PATCH 2/2] test(proxy): an access denial does not fall through to later routes Signed-off-by: Oleksandr Piskun --- tests/php/Controller/ExAppProxyControllerTest.php | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/php/Controller/ExAppProxyControllerTest.php b/tests/php/Controller/ExAppProxyControllerTest.php index c6feb2ccb..5ab885187 100644 --- a/tests/php/Controller/ExAppProxyControllerTest.php +++ b/tests/php/Controller/ExAppProxyControllerTest.php @@ -60,6 +60,16 @@ public function testFirstRouteMatchingPathAndVerbDecides(): void { 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']);