Repository navigation
Conversation
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Approved. The URI-authority fallback addresses hostless HTTP/2 publisher processing while preserving existing host-header precedence.
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
Falls back to the request URI authority in extract_request_host when the Forwarded host, X-Forwarded-Host, and Host headers are all absent, so HTTP/2 requests that carry only :authority are processed instead of being returned unmodified (#1118). The precedence of explicit headers is preserved, and the regression tests fail without the fallback and pass with it. No blocking findings.
1 of the inline comments below carries a one-click GitHub
suggestion— use Commit suggestion to apply it as a commit on the PR
branch. The remaining feedback is either praise or sits on lines outside the
diff, so it is described in prose.
Non-blocking
♻️ refactor
- Strip userinfo from the URI authority fallback — see inline at
crates/trusted-server-core/src/http_util.rs:183
👍 praise
- End-to-end regression through the publisher pipeline — see inline at
crates/trusted-server-core/src/publisher.rs:19736
Cross-cutting / body-level findings
-
🏕 Call-site comment and debug log in
publisher.rsstill describe a Host-header-only source — the PR updates thehttp_util.rsdocs to "Hostor URI authority", but the comment above theRequestInfo::from_requestcall atcrates/trusted-server-core/src/publisher.rs:4452still reads// Extract request host and scheme (uses Host header and TLS detection after edge sanitization). The debug log just below it (lines 4456–4463) printsX-Forwarded-HostandHostbut not the URI authority, which is the value that now matters when diagnosing this exact case. Suggested comment (apply manually — these lines are outside the diff, so they can't carry asuggestion):// Extract request host and scheme (uses Host header or URI authority and TLS detection after edge sanitization)Optionally, add
req.uri().authority()to that debug log.
CI Status
- integration tests: PASS
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- CodeQL: PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test (cross-adapter parity): PASS
- format-typescript: PASS (required)
- Analyze (rust): PASS
- format-docs: PASS (required)
- vitest: PASS
- cargo test (axum native): PASS
- Analyze (javascript-typescript): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo test: PASS (required)
- Analyze (actions): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- docs-proposal scripts: PASS
- CLAUDE.md symlink guard: PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- cargo fmt: PASS (required)
- prepare integration artifacts: PASS
- Analyze (python): PASS
- Analyze (javascript-typescript): PASS
aram356
left a comment
There was a problem hiding this comment.
Summary
When Forwarded, X-Forwarded-Host and Host are all absent, this falls back to the request URI authority, so publisher HTML is processed instead of returned unmodified (#1118). The fix works:
- On an Axum binary built from this head, absolute-form requests without
Hostare rewritten and get the script. - On an http2-enabled build, HTTP/2 requests without
Hostare rewritten too. - The base build reproduces the bug.
- The new tests catch every mutation of the fallback: removing it, reordering it, or dropping the port.
The branch is behind main. A scratch merge has no conflicts and passes fmt, clippy, test-fastly (2,978 core tests) and test-axum.
Two small gaps in the new fallback each have a one-click suggestion. There are also three body-level asks.
Both inline comments below carry a one-click GitHub
suggestion. Use Commit suggestion, or Add suggestion to batch to apply both at once. Their test cases are in prose because they sit in a different hunk.
Blocking
🔧 wrench
- Userinfo from the URI authority reaches rewritten URLs: see inline at
crates/trusted-server-core/src/http_util.rs:183 - An empty
Hostheader blocks the new fallback: see inline atcrates/trusted-server-core/src/http_util.rs:181 - The scheme still ignores the URI; file a follow-up: see below
- Say where the fallback actually fires: see below
- Add a CHANGELOG entry: see below
Cross-cutting / body-level findings
-
🔧 The scheme still ignores the URI; please file a follow-up issue and link it here.
detect_request_schemereads TLS info and forwarded headers, but neverreq.uri().scheme().- This fallback can now fire behind a TLS terminator when no
X-Forwarded-Protosurvives. Example: Viceroy behind an h2c proxy, since the Fastly adapter stripsX-Forwarded-Protoand Viceroy has no TLS metadata. - Pages there used to pass through unmodified. They are now rewritten with
http://links on anhttpspage. - On an http2-enabled Axum build, an HTTP/2 request with
:scheme httpsalso producedhttp://links. /first-party/signprefers the URI scheme (crates/trusted-server-core/src/proxy.rs:1713-1717), so signing and rewriting can disagree on the same request.
Requests that carry
Hostalready behave this way, so the fix does not belong in this PR. - This fallback can now fire behind a TLS terminator when no
-
🔧 Say where the fallback actually fires. It never fires on Fastly production or Cloudflare, which always supply
Host, or on Spin, which setsHostfromspin-full-url. The stock Axum dev server is HTTP/1-only: hyper-util is built withouthttp2, and prior-knowledge h2c connections are reset.- On Axum, the fallback fires for absolute-form HTTP/1.1 requests without
Host. - On Viceroy, it also fires for h2c without
Host.
The PR summary ("as can happen with HTTP/2") and the new doc lines in
http_util.rs(126-127 and 153-154, "effective sources in production") read as if production HTTP/2 traffic depends on this. Please name the cases above in the PR description and reword those doc lines. For example: "Hostis the effective source in production; the URI authority fills in only when noHostarrives (absolute-form HTTP/1.1, or HTTP/2 withoutHost)." - On Axum, the fallback fires for absolute-form HTTP/1.1 requests without
-
🔧 Add a CHANGELOG entry. HTML that used to pass through unmodified is now rewritten, which operators will notice. Please add a
### Fixedline under[Unreleased], for example: "Publisher HTML is now processed when the request carries its host only in the request URI (absolute-form HTTP/1.1, or HTTP/2 withoutHost), instead of being returned unmodified with an 'Empty request host' warning (#1118)."
CI Status
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
- Analyze (rust): PASS
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- CLAUDE.md symlink guard: PASS
- CodeQL: PASS
- docs-proposal scripts: PASS
- format-docs: PASS (required)
- format-typescript: PASS (required)
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- vitest: PASS
Skip empty Host values and strip URI userinfo while preserving ports and IPv6 brackets. Cover the publisher pipeline for absolute-form HTTP/1.1 and HTTP/2 requests, and clarify deployment scope and diagnostics. Track scheme consistency separately in #1447.
|
Addressed both reviews in 48b8b1bbb.
All four inline threads have replies and are resolved. Independent review of the full diff against The live source branch points to the tested commit, but GitHub currently still reports the previous head in the PR commit list and has no checks for the new commit yet. The commit link above points directly to the fixes. Re-review is requested from the reviewer who requested changes; fresh GitHub CI remains pending synchronization. |
| ( | ||
| "https://user:pass@[2001:db8::1]:8443/page", | ||
| "[2001:db8::1]:8443", | ||
| ), |
There was a problem hiding this comment.
⛏ nitpick: Pin rsplit_once with a two-@ authority.
The http crate accepts an authority like a@b@publisher.example.com, and Authority::host() returns publisher.example.com for it. If line 192 used split_once('@') instead of rsplit_once('@'), every current test would still pass, but the result would be b@publisher.example.com. This row catches that.
| ( | |
| "https://user:pass@[2001:db8::1]:8443/page", | |
| "[2001:db8::1]:8443", | |
| ), | |
| ( | |
| "https://user:pass@[2001:db8::1]:8443/page", | |
| "[2001:db8::1]:8443", | |
| ), | |
| ( | |
| "https://a@b@publisher.example.com/page", | |
| "publisher.example.com", | |
| ), |
Verified on this head:
- The suggestion passes rustfmt, all six clippy aliases,
test-fastly,test-axum,test-cloudflare,test-spinand parity. - With
split_onceswapped in, the new row fails (left: "b@publisher.example.com").
| let html = response_body_string(response); | ||
|
|
||
| assert!( | ||
| html.contains("href=\"https://publisher.example.com:8443/article\""), | ||
| "should rewrite origin links using URI authority: {html}" | ||
| ); | ||
| assert!( | ||
| !html.contains("user:pass@"), | ||
| "should omit URI userinfo from rewritten HTML: {html}" | ||
| ); | ||
| assert!( | ||
| html.contains("id=\"trustedserver-js\""), | ||
| "should inject the Trusted Server script: {html}" | ||
| ); |
There was a problem hiding this comment.
⛏ nitpick: Name the failing case in the loop's assertions.
The loop covers six cases: two HTTP versions, each with a plain authority, userinfo and an empty Host. A failure prints only the HTML, so you can't tell which case broke. With this change a failure reads, for example: should rewrite origin links using URI authority for HTTP/1.1 https://publisher.example.com:8443/page (empty Host: true): ...
| let html = response_body_string(response); | |
| assert!( | |
| html.contains("href=\"https://publisher.example.com:8443/article\""), | |
| "should rewrite origin links using URI authority: {html}" | |
| ); | |
| assert!( | |
| !html.contains("user:pass@"), | |
| "should omit URI userinfo from rewritten HTML: {html}" | |
| ); | |
| assert!( | |
| html.contains("id=\"trustedserver-js\""), | |
| "should inject the Trusted Server script: {html}" | |
| ); | |
| let html = response_body_string(response); | |
| let case = format!("{version:?} {uri} (empty Host: {empty_host})"); | |
| assert!( | |
| html.contains("href=\"https://publisher.example.com:8443/article\""), | |
| "should rewrite origin links using URI authority for {case}: {html}" | |
| ); | |
| assert!( | |
| !html.contains("user:pass@"), | |
| "should omit URI userinfo from rewritten HTML for {case}: {html}" | |
| ); | |
| assert!( | |
| html.contains("id=\"trustedserver-js\""), | |
| "should inject the Trusted Server script for {case}: {html}" | |
| ); |
Verified on this head:
- The suggestion passes rustfmt, all six clippy aliases,
test-fastly,test-axum,test-cloudflare,test-spinand parity. - With the empty-
Hostfilter removed, the failure message names the case, as shown above.
Summary
Publisher HTML is processed when the request carries its host only in the URI. Previously these requests passed through without URL rewriting or script injection and emitted an "Empty request host" warning. Closes #1118.
Host selection keeps the existing precedence:
Forwarded: host=...,X-Forwarded-Host, nonemptyHost, then URI authority. The URI fallback removes userinfo while preserving ports and IPv6 brackets. EmptyHostvalues now allow the fallback, consistently with empty forwarded host values.Where the fallback applies
Host; it does not enable HTTP/2 support.:authorityand no usableHost. An Axum build with HTTP/2 explicitly enabled can encounter the same request shape.Host; Spin reconstructs it fromspin-full-url. These paths normally continue usingHost.Scheme detection is unchanged. If TLS metadata and forwarded scheme headers are absent, rewriting still defaults to HTTP even when the URI contains an HTTPS scheme.
/first-party/signcurrently prefers the URI scheme. Follow-up bug #1447 tracks a shared scheme policy and coverage for TLS-terminated h2c deployments, as requested in review.Changes
Host, userinfo with/without ports, and IPv6 userinfo.Hostand URI userinfo, checking rewritten links and script injection.CHANGELOG.md→[Unreleased]→Fixed.Validation
Both added host-extraction regressions were observed failing before their fixes and passing afterward.
cargo fmt --all -- --checkandgit diff --checkpass.mainfound no actionable gaps. The PR description and scheme follow-up were also reviewed.The release-producer checks named in the current workspace instructions are not present on this PR branch. Production release packaging and manual browser testing were not run.
The HTTP/2 publisher test constructs a core request directly; it does not assert that the stock Axum network listener supports HTTP/2. It supplies an explicit forwarded HTTPS scheme to isolate host extraction; scheme-policy changes are tracked in #1447.