Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
151 changes: 149 additions & 2 deletions src/Relay/RelayHttpRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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<string>
*/
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<string>
*/
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.
Expand Down Expand Up @@ -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);
}
}
131 changes: 131 additions & 0 deletions tests/Relay/RelayHttpRequestTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, array{string}>
*/
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<string, array{string}>
*/
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);
}
}
Loading