diff --git a/CHANGELOG.md b/CHANGELOG.md index e048d71..bcc6944 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,30 @@ All notable changes to `detain/phlix-shared` are documented here. This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Added +- **`Relay\RelayHttpRequest` security gate** (findings S1/F1) — the untrusted, + hub-tunnelled HTTP envelope now self-validates its method and path: + - `assertSafe(): void` throws `InvalidArgumentException` on an unsafe method or + path; it is invoked automatically at the end of `fromJson()` so every consumer + that deserializes the wire envelope inherits the gate. Path rules reject + missing leading `/`, protocol-relative `//…`, `..` (raw and percent-encoded + `%2e%2e`), NUL (raw and `%00`), backslash, `://`, an embedded query (`?`) or + fragment (`#`), and control characters (`< 0x20`). Method must be in + `ALLOWED_METHODS` (case-insensitive). + - `ALLOWED_METHODS` constant: `GET HEAD POST PUT PATCH DELETE OPTIONS`. + - `STRIPPED_HEADERS` constant + `static isForbiddenHeader(string): bool` + (case-insensitive) + `withoutForbiddenHeaders(): self` — expose the + trust-bearing inbound header set (`x-phlix-relay-user`, `x-forwarded-for`, + `authorization`, `cookie`) so the consumer can drop them before forwarding and + inject the hub-validated owner identity itself. The DTO does NOT silently strip + `x-phlix-relay-user` — identity injection remains the consumer's responsibility. + - All changes are additive / backward-compatible; valid requests round-trip + unchanged. **Consumer follow-up (phlix-server `RelayConsumer::buildRequest()`):** + stop trusting `x-phlix-relay-user` from the envelope and strip forbidden headers + via `isForbiddenHeader()` — that paired PR is the actual auth-bypass fix. + ## [0.10.1] - 2026-06-23 ### Added diff --git a/src/Relay/RelayHttpRequest.php b/src/Relay/RelayHttpRequest.php index 85fa384..806966d 100644 --- a/src/Relay/RelayHttpRequest.php +++ b/src/Relay/RelayHttpRequest.php @@ -8,11 +8,17 @@ use function base64_decode; use function base64_encode; +use function in_array; use function is_array; -use function is_int; use function is_string; use function json_decode; use function json_encode; +use function ord; +use function rawurldecode; +use function str_contains; +use function strlen; +use function strtolower; +use function strtoupper; use const JSON_THROW_ON_ERROR; @@ -37,6 +43,28 @@ */ final readonly class RelayHttpRequest { + /** + * HTTP methods the relay will forward. Anything else is rejected by + * {@see self::assertSafe()}. Compared case-insensitively (upper-cased). + * + * @var list + */ + public const ALLOWED_METHODS = ['GET', 'HEAD', 'POST', 'PUT', 'PATCH', 'DELETE', 'OPTIONS']; + + /** + * Trust-bearing inbound headers the consumer keys auth/identity off and + * which an untrusted relay producer must never be allowed to set. The DTO + * does NOT silently strip these in {@see self::fromJson()} (the hub owns + * identity injection); instead it exposes the set via + * {@see self::isForbiddenHeader()} and {@see self::withoutForbiddenHeaders()} + * so the consumer can drop them before forwarding. + * + * Names are lower-cased for case-insensitive matching. + * + * @var list + */ + public const STRIPPED_HEADERS = ['x-phlix-relay-user', 'x-forwarded-for', 'authorization', 'cookie']; + /** * @param string $method Upper-case HTTP method (GET/POST/…). * @param string $path Request path (no query string), e.g. /api/v1/libraries. @@ -132,6 +160,125 @@ public static function fromJson(string $json): self throw new InvalidArgumentException('RelayHttpRequest: "body" is not valid base64.'); } - return new self($method, $path, $query, $headers, $body); + $request = new self($method, $path, $query, $headers, $body); + $request->assertSafe(); + + return $request; + } + + /** + * Validate that the method and path of this (untrusted, hub-tunnelled) + * request are safe to forward. Throws on any violation; returns void on + * success. Called automatically at the end of {@see self::fromJson()} so + * every consumer that deserializes the wire envelope inherits the gate. + * + * This validates METHOD and PATH only. It does NOT strip trust-bearing + * headers — that is the consumer's responsibility via + * {@see self::isForbiddenHeader()} / {@see self::withoutForbiddenHeaders()}, + * because the consumer owns identity injection (e.g. the relay session's + * authenticated owner) and the DTO must not silently mutate the envelope. + * + * Path rules: must start with a single '/'; must not be protocol-relative + * ('//…'); must not contain '..' (raw or percent-encoded), a NUL byte + * (raw or '%00'), a backslash, '://', a '?' (query is a separate field), + * a '#', or any control character (< 0x20). Percent-encoded sequences are + * decoded once and re-checked so '%2e%2e' / '%00' are caught. + * + * Method rules: must match {@see self::ALLOWED_METHODS} case-insensitively. + * + * @throws InvalidArgumentException When the method or path is unsafe. + * + * @since 0.11.0 + */ + public function assertSafe(): void + { + if (!in_array(strtoupper($this->method), self::ALLOWED_METHODS, true)) { + throw new InvalidArgumentException( + 'RelayHttpRequest: HTTP method "' . $this->method . '" is not allowed.', + ); + } + + $path = $this->path; + + if ($path === '' || $path[0] !== '/') { + throw new InvalidArgumentException('RelayHttpRequest: "path" must start with "/".'); + } + + // Protocol-relative URL (//host/...) — would target an external origin. + if (isset($path[1]) && $path[1] === '/') { + throw new InvalidArgumentException('RelayHttpRequest: "path" must not be protocol-relative.'); + } + + // Re-check the path with percent-encoding decoded once so encoded + // traversal / NUL bytes are caught alongside their literal forms. + $decoded = rawurldecode($path); + + foreach ([$path, $decoded] as $candidate) { + if (str_contains($candidate, '..')) { + throw new InvalidArgumentException('RelayHttpRequest: "path" must not contain "..".'); + } + if (str_contains($candidate, "\0")) { + throw new InvalidArgumentException('RelayHttpRequest: "path" must not contain a NUL byte.'); + } + if (str_contains($candidate, '\\')) { + throw new InvalidArgumentException('RelayHttpRequest: "path" must not contain a backslash.'); + } + if (str_contains($candidate, '://')) { + throw new InvalidArgumentException('RelayHttpRequest: "path" must not contain "://".'); + } + if (str_contains($candidate, '?')) { + throw new InvalidArgumentException('RelayHttpRequest: "path" must not contain a query string.'); + } + if (str_contains($candidate, '#')) { + throw new InvalidArgumentException('RelayHttpRequest: "path" must not contain a fragment.'); + } + + $length = strlen($candidate); + for ($i = 0; $i < $length; $i++) { + if (ord($candidate[$i]) < 0x20) { + throw new InvalidArgumentException( + 'RelayHttpRequest: "path" must not contain control characters.', + ); + } + } + } + } + + /** + * Whether a header name is a trust-bearing header the consumer must not + * accept from the untrusted relay producer (see {@see self::STRIPPED_HEADERS}). + * Case-insensitive. + * + * @param string $name Header name. + * + * @return bool True when the header must be stripped before forwarding. + * + * @since 0.11.0 + */ + public static function isForbiddenHeader(string $name): bool + { + return in_array(strtolower($name), self::STRIPPED_HEADERS, true); + } + + /** + * Return a copy of this request with all forbidden (trust-bearing) headers + * removed. The consumer should call this before forwarding so an untrusted + * relay producer cannot spoof identity/auth headers, then inject the + * hub-validated owner identity itself. + * + * @return self A new instance with {@see self::STRIPPED_HEADERS} dropped. + * + * @since 0.11.0 + */ + public function withoutForbiddenHeaders(): self + { + $headers = []; + foreach ($this->headers as $name => $value) { + if (!self::isForbiddenHeader($name)) { + $headers[$name] = $value; + } + } + + return new self($this->method, $this->path, $this->query, $headers, $this->body); } } diff --git a/tests/Relay/RelayHttpRequestTest.php b/tests/Relay/RelayHttpRequestTest.php index 86e509b..c7cb78d 100644 --- a/tests/Relay/RelayHttpRequestTest.php +++ b/tests/Relay/RelayHttpRequestTest.php @@ -70,4 +70,135 @@ public function test_from_json_rejects_invalid_base64_body(): void $this->expectException(InvalidArgumentException::class); RelayHttpRequest::fromJson('{"method":"GET","path":"/x","headers":{},"body":"!!!not base64!!!"}'); } + + /** + * @return array + */ + public static function unsafePathProvider(): array + { + return [ + 'no leading slash' => ['foo'], + 'literal traversal' => ['/a/../b'], + 'percent-encoded traversal' => ['/%2e%2e/b'], + 'protocol-relative' => ['//evil.com'], + 'embedded NUL' => ["/a\0b"], + 'percent-encoded NUL' => ['/a%00b'], + 'scheme prefix' => ['gopher://x'], + 'backslash' => ['/a\\b'], + 'query in path' => ['/a?b=1'], + 'fragment in path' => ['/a#b'], + 'control char' => ["/a\x01b"], + ]; + } + + /** + * @dataProvider unsafePathProvider + */ + public function test_assert_safe_rejects_unsafe_path(string $path): void + { + $req = new RelayHttpRequest('GET', $path, '', [], ''); + $this->expectException(InvalidArgumentException::class); + $req->assertSafe(); + } + + public function test_assert_safe_rejects_empty_path(): void + { + $req = new RelayHttpRequest('GET', '', '', [], ''); + $this->expectException(InvalidArgumentException::class); + $req->assertSafe(); + } + + /** + * @return array + */ + public static function disallowedMethodProvider(): array + { + return [ + 'CONNECT' => ['CONNECT'], + 'TRACE' => ['TRACE'], + 'lowercase garbage' => ['frobnicate'], + 'empty-ish whitespace' => [' '], + ]; + } + + /** + * @dataProvider disallowedMethodProvider + */ + public function test_assert_safe_rejects_disallowed_method(string $method): void + { + $req = new RelayHttpRequest($method, '/api/v1/libraries', '', [], ''); + $this->expectException(InvalidArgumentException::class); + $req->assertSafe(); + } + + public function test_assert_safe_accepts_valid_get(): void + { + $req = new RelayHttpRequest('GET', '/api/v1/libraries', '', [], ''); + $req->assertSafe(); + $this->addToAssertionCount(1); + } + + public function test_assert_safe_accepts_lower_case_method(): void + { + $req = new RelayHttpRequest('get', '/api/v1/libraries', '', [], ''); + $req->assertSafe(); + $this->addToAssertionCount(1); + } + + public function test_from_json_runs_assert_safe(): void + { + $this->expectException(InvalidArgumentException::class); + RelayHttpRequest::fromJson('{"method":"GET","path":"/a/../b","headers":{},"body":""}'); + } + + public function test_from_json_rejects_disallowed_method(): void + { + $this->expectException(InvalidArgumentException::class); + RelayHttpRequest::fromJson('{"method":"CONNECT","path":"/x","headers":{},"body":""}'); + } + + public function test_round_trip_of_valid_request_still_succeeds(): void + { + $req = new RelayHttpRequest('GET', '/api/v1/libraries', 'a=1', ['Accept' => 'application/json'], ''); + $decoded = RelayHttpRequest::fromJson($req->toJson()); + + $this->assertSame('GET', $decoded->method); + $this->assertSame('/api/v1/libraries', $decoded->path); + $this->assertSame('a=1', $decoded->query); + $this->assertSame(['Accept' => 'application/json'], $decoded->headers); + $this->assertSame('', $decoded->body); + } + + public function test_is_forbidden_header_is_case_insensitive(): void + { + $this->assertTrue(RelayHttpRequest::isForbiddenHeader('X-Phlix-Relay-User')); + $this->assertTrue(RelayHttpRequest::isForbiddenHeader('authorization')); + $this->assertTrue(RelayHttpRequest::isForbiddenHeader('Cookie')); + $this->assertTrue(RelayHttpRequest::isForbiddenHeader('X-Forwarded-For')); + $this->assertFalse(RelayHttpRequest::isForbiddenHeader('Accept')); + $this->assertFalse(RelayHttpRequest::isForbiddenHeader('X-Phlix-Relay')); + } + + public function test_without_forbidden_headers_strips_trust_bearing_headers(): void + { + $req = new RelayHttpRequest( + 'GET', + '/api/v1/libraries', + '', + [ + 'Accept' => 'application/json', + 'X-Phlix-Relay-User' => '42', + 'Authorization' => 'Bearer x', + 'Cookie' => 'session=abc', + 'X-Forwarded-For' => '1.2.3.4', + ], + '', + ); + + $clean = $req->withoutForbiddenHeaders(); + + $this->assertSame(['Accept' => 'application/json'], $clean->headers); + // Original is untouched (immutable copy semantics). + $this->assertArrayHasKey('X-Phlix-Relay-User', $req->headers); + } }