Skip to content

Resolve request host from URI authority when headers are absent - #1413

Open
prk-Jr wants to merge 5 commits into
mainfrom
fix/http2-authority-host
Open

prk-Jr wants to merge 5 commits into
mainfrom
fix/http2-authority-host

Conversation

@prk-Jr

@prk-Jr prk-Jr commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator

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, nonempty Host, then URI authority. The URI fallback removes userinfo while preserving ports and IPv6 brackets. Empty Host values now allow the fallback, consistently with empty forwarded host values.

Where the fallback applies

  • The stock Axum dev server supports HTTP/1 only. The fallback handles absolute-form HTTP/1.1 requests without a usable Host; it does not enable HTTP/2 support.
  • Viceroy can receive h2c requests with :authority and no usable Host. An Axum build with HTTP/2 explicitly enabled can encounter the same request shape.
  • Normal Fastly production and Cloudflare requests already supply Host; Spin reconstructs it from spin-full-url. These paths normally continue using Host.

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/sign currently 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

  • Add and harden URI authority host extraction; clarify host-source documentation.
  • Update the publisher call-site comment and log URI host/port without logging URI userinfo.
  • Test authority-only requests, explicit header precedence, empty Host, userinfo with/without ports, and IPv6 userinfo.
  • Exercise the publisher pipeline for absolute-form HTTP/1.1 and HTTP/2, with absent/empty Host and URI userinfo, checking rewritten links and script injection.
  • Add an operator-facing entry under CHANGELOG.md → [Unreleased] → Fixed.

Validation

Both added host-extraction regressions were observed failing before their fixes and passing afterward.

  • All five adapter test aliases pass: Fastly, Fastly reusable sandbox, Axum, Cloudflare, and Spin. Fastly runs use the pinned Viceroy 0.21.0 (2,978 core tests passed; reusable sandbox: 221 passed).
  • All eight target-matched Clippy aliases pass.
  • Native core suite: 2,984 passed by running the compiled test binary directly, avoiding Cargo's injected build-version environment variable that the legacy settings loader rejects.
  • Native build-digest tests: 10 passed; matching Clippy check passed.
  • Cross-adapter parity: 17 passed.
  • Node 24.20.0: JS build and 1,211 tests pass; JS, docs, and repository Markdown formatting pass.
  • Wasm clock lint, documentation-proposal ShellCheck, hunk tests, and proposal tests pass.
  • cargo fmt --all -- --check and git diff --check pass.
  • Core documentation builds successfully with 31 existing warnings outside the changed lines.
  • An independent review of the complete diff against main found 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.

@prk-Jr prk-Jr self-assigned this Oct 19, 2026

@ChristianPavilonis ChristianPavilonis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. The URI-authority fallback addresses hostless HTTP/2 publisher processing while preserving existing host-header precedence.

@dhruv8sh dhruv8sh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.rs still describe a Host-header-only source — the PR updates the http_util.rs docs to "Host or URI authority", but the comment above the RequestInfo::from_request call at crates/trusted-server-core/src/publisher.rs:4452 still reads // Extract request host and scheme (uses Host header and TLS detection after edge sanitization). The debug log just below it (lines 4456–4463) prints X-Forwarded-Host and Host but 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 a suggestion):

        // 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

Comment thread crates/trusted-server-core/src/http_util.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs Outdated

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Host are rewritten and get the script.
  • On an http2-enabled build, HTTP/2 requests without Host are 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 Host header blocks the new fallback: see inline at crates/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_scheme reads TLS info and forwarded headers, but never req.uri().scheme().

    • This fallback can now fire behind a TLS terminator when no X-Forwarded-Proto survives. Example: Viceroy behind an h2c proxy, since the Fastly adapter strips X-Forwarded-Proto and Viceroy has no TLS metadata.
    • Pages there used to pass through unmodified. They are now rewritten with http:// links on an https page.
    • On an http2-enabled Axum build, an HTTP/2 request with :scheme https also produced http:// links.
    • /first-party/sign prefers 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 Host already behave this way, so the fix does not belong in this PR.

  • 🔧 Say where the fallback actually fires. It never fires on Fastly production or Cloudflare, which always supply Host, or on Spin, which sets Host from spin-full-url. The stock Axum dev server is HTTP/1-only: hyper-util is built without http2, 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: "Host is the effective source in production; the URI authority fills in only when no Host arrives (absolute-form HTTP/1.1, or HTTP/2 without Host)."

  • 🔧 Add a CHANGELOG entry. HTML that used to pass through unmodified is now rewritten, which operators will notice. Please add a ### Fixed line 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 without Host), 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

Comment thread crates/trusted-server-core/src/http_util.rs Outdated
Comment thread crates/trusted-server-core/src/http_util.rs
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.
@prk-Jr
prk-Jr requested a review from aram356 October 24, 2026 05:24
@prk-Jr

prk-Jr commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed both reviews in 48b8b1bbb.

  • Strip URI userinfo while retaining ports and IPv6 brackets; skip empty Host before the URI fallback. Unit regressions were observed failing before the fixes and passing afterward.
  • Extend publisher pipeline coverage across absolute-form HTTP/1.1 and HTTP/2 with absent/empty Host and URI userinfo.
  • Clarify runtime scope in the docs and PR description, update the publisher comment and diagnostics, and add the Unreleased changelog entry.
  • File the requested scheme-policy follow-up as Align publisher rewriting and first-party signing request schemes #1447. Scheme behavior is unchanged in this PR.

All four inline threads have replies and are resolved. Independent review of the full diff against main found no actionable gaps. All adapter tests, all eight Clippy aliases, 2,984 native core tests, 17 parity tests, 1,211 JS tests, JS build, formatting, and applicable script checks pass. Fastly tests used Viceroy 0.21.0; Node checks used Node 24.20.0. Core documentation builds with existing warnings outside the changed lines.

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.

Comment on lines +609 to +612
(
"https://user:pass@[2001:db8::1]:8443/page",
"[2001:db8::1]:8443",
),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏ 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.

Suggested change
(
"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-spin and parity.
  • With split_once swapped in, the new row fails (left: "b@publisher.example.com").

Comment on lines +19797 to +19810
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}"
);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏ 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): ...

Suggested change
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-spin and parity.
  • With the empty-Host filter removed, the failure message names the case, as shown above.

This branch has not been deployed

No deployments
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.

Nothing is processed over HTTP/2, because the request host is read from a header HTTP/2 does not send

4 participants