Skip to content

S1: harden RelayHttpRequest with path/method gate + forbidden-header helpers#14

Merged
detain merged 1 commit into
masterfrom
fix/shared-s1
Jun 28, 2026
Merged

S1: harden RelayHttpRequest with path/method gate + forbidden-header helpers#14
detain merged 1 commit into
masterfrom
fix/shared-s1

Conversation

@detain

@detain detain commented Jun 28, 2026

Copy link
Copy Markdown
Owner

Step S1 — RelayHttpRequest::assertSafe() path/method/header gate

Findings: S1 (HIGH), F1 — RelayHttpRequest deserializes an untrusted, hub-tunnelled HTTP envelope with no path/method/header hardening. The shared-lib half provides the gate; the actual auth-bypass fix lands in a paired phlix-server PR.

Changes (scoped to S1's files only)

  • src/Relay/RelayHttpRequest.php
    • public function assertSafe(): void — throws InvalidArgumentException on unsafe method/path. Invoked at the end of fromJson() so every consumer that deserializes inherits the gate.
      • Path: rejects empty / no-leading-/, protocol-relative //…, .. (raw and percent-encoded %2e%2e), NUL (raw and %00), backslash, ://, embedded ?/#, control chars < 0x20. Percent-decoded once and re-checked.
      • Method: allow-list ALLOWED_METHODS = [GET,HEAD,POST,PUT,PATCH,DELETE,OPTIONS], compared upper-cased.
    • const ALLOWED_METHODS, const STRIPPED_HEADERS (x-phlix-relay-user, x-forwarded-for, authorization, cookie).
    • static isForbiddenHeader(string): bool (case-insensitive) and withoutForbiddenHeaders(): self — expose the forbidden-header set without silently stripping x-phlix-relay-user; the consumer owns identity injection.
  • tests/Relay/RelayHttpRequestTest.php — covers every rejection branch (empty path, no leading slash, /a/../b, /%2e%2e/b, //evil.com, NUL raw + %00, gopher://x, backslash, ?/#, control char, methods CONNECT/TRACE/garbage/whitespace), valid GET passes, lowercase method passes, fromJson runs the gate, valid round-trip succeeds, isForbiddenHeader case-insensitivity, withoutForbiddenHeaders strips and leaves original immutable.
  • CHANGELOG.md [Unreleased] entry. (No src/Version.php bump / tag — batch-final release step.)

Additive / BC-safe

New methods + constants only; valid requests round-trip unchanged. No public signature changes.

How verified

  • composer test342 tests, 2827 assertions, OK (was 319 baseline)
  • composer stan — PHPStan level 9, no baseline: No errors
  • composer cs — phpcs PSR-12 clean (exit 0)
  • composer psalm — No errors found

Cross-repo coupling (follow-up, NOT in this PR)

phlix-server RelayConsumer::buildRequest() (src/Hub/RelayConsumer.php:685-727) must (a) keep calling RelayHttpRequest::fromJson() (now self-validating) and (b) stop trusting x-phlix-relay-user from envelope headers for \$request->userId — derive owner identity from the hub-validated relay session and strip forbidden headers via isForbiddenHeader() / withoutForbiddenHeaders(). That paired PR is the actual auth-bypass fix.

🤖 Generated with Claude Code

…helpers

Findings S1 (HIGH) / F1. RelayHttpRequest deserializes an untrusted,
hub-tunnelled HTTP envelope; add a self-validating gate so every consumer
inherits it.

- assertSafe(): void — invoked at the end of fromJson(); rejects unsafe
  method (allow-list ALLOWED_METHODS, case-insensitive) and unsafe path
  (no leading /, protocol-relative //, .. raw + %2e%2e, NUL raw + %00,
  backslash, ://, embedded ?/#, control chars < 0x20; percent-decoded once
  and re-checked).
- STRIPPED_HEADERS constant + static isForbiddenHeader() + withoutForbiddenHeaders():
  expose the trust-bearing inbound header set without silently mutating the
  envelope; the consumer owns identity injection.

Additive / BC-safe; valid requests round-trip unchanged. The paired
phlix-server RelayConsumer change (stop trusting x-phlix-relay-user) is the
actual auth-bypass fix and ships separately.

Gate: 342 tests OK (was 319); PHPStan L9 no errors; phpcs PSR-12 clean;
psalm clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@codecov

codecov Bot commented Jun 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.43590% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 58.57%. Comparing base (956c3fe) to head (661d596).

Files with missing lines Patch % Lines
src/Relay/RelayHttpRequest.php 97.43% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master      #14      +/-   ##
============================================
+ Coverage     57.36%   58.57%   +1.20%     
- Complexity      403      422      +19     
============================================
  Files            38       38              
  Lines          1222     1260      +38     
============================================
+ Hits            701      738      +37     
- Misses          521      522       +1     
Flag Coverage Δ
phpunit 58.57% <97.43%> (+1.20%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@codacy-production

Copy link
Copy Markdown

Not up to standards ⛔

🔴 Issues 11 medium · 11 minor

Alerts:
⚠ 22 issues (≤ 0 issues of at least minor severity)

Results:
22 new issues

Category Results
BestPractice 8 medium
CodeStyle 11 minor
Complexity 3 medium

View in Codacy

🟢 Metrics 31 complexity · 0 duplication

Metric Results
Complexity 31
Duplication 0

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@detain detain merged commit b9073f2 into master Jun 28, 2026
8 of 9 checks passed
@detain detain deleted the fix/shared-s1 branch June 28, 2026 16:16
detain added a commit that referenced this pull request Jun 28, 2026
Bump Version::VERSION to 0.11.0 and promote the [Unreleased] CHANGELOG
section to [0.11.0] - 2026-06-28, rolling up the four changes merged
since v0.10.1:

- #14 S1: harden RelayHttpRequest with path/method gate + forbidden-header
  helpers (isForbiddenHeader/withoutForbiddenHeaders/assertSafe)
- #15 JwtClaims: strict-aud variant (fromPayloadStrict) + verification/
  round-trip docs (S4+B4)
- #16 refactor(arr): extract AbstractArrClient to dedup the four *arr
  clients (F2a)
- #17 feat(arr): inject async HTTP transport via ArrTransportInterface
  (F2b/B1/P1)

composer.json carries no hardcoded version field (tags drive version),
so it is left unchanged. The git tag is applied by the coordinator
after merge.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant