fix(proxy): refuse dot segments resolved after the route check

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]
 

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;
 		}

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,29 +30,48 @@ 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,
 			$groupManager,
 			$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));
+	}
 }