Repository navigation
Conversation
Publisher pages had origin URLs rewritten in the body but not in headers, so browsers followed Location and Refresh to the origin, preloaded Link targets from it, and blocked rewritten resources under the forwarded CSP. Map origin URLs in Location, Content-Location, Refresh, and Link (including imagesrcset) to the serving host on every publisher response, and add serving-host sources beside origin sources in Content-Security-Policy and its report-only variant. Rewriting runs before the template-cache gate so cached templates replay the rewritten policy headers; bump the template schema to 6 so entries stored with origin-only policy metadata are not replayed. Leave a Location or Refresh target unchanged when it names the current request URL under a different scheme than the origin, so an origin scheme upgrade cannot loop the browser. Closes #1128 Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Solid, well-tested change. Rewrites origin URLs in Location, Content-Location, Refresh, Link, and both CSP headers; Set-Cookie/Vary untouched. Host matching is strict and case-insensitive, multi-value headers preserve order, and the rewrite runs before template-cache capture (with the schema bump to 6) in the shared handle_publisher_request path used by all adapters. No blocking findings.
Non-blocking
- 🤔 Loop-guard escape logs only at
debug(inline, suggestion) - 🤔 Same-URL cookie-setting redirect can loop when the cookie has
Domain=<origin host>(inline) - 🌱 CSP
:*/ explicit default-port sources not matched (inline) - ⛏ Docs row omits
report-to(inline, suggestion)
📌 Out of scope
Set-Cookie Domain, Reporting-Endpoints/Report-To, and Access-Control-Allow-Origin still pass through unchanged. Leaving report endpoints alone matches the report-uri choice; the Set-Cookie Domain gap (see inline comment) is worth a follow-up issue. Asset-proxy routes (handle_asset_proxy_request) also bypass this rewrite, consistent with the docs saying "publisher response".
📝 Note
The PR checklist says "Uses tracing macros", but the code correctly uses log — only the checklist wording is off.
Verification
cargo fmt --all -- --check✅cargo clippy-fastly✅ (no warnings)cargo test -p trusted-server-core(host) ✅ 2916 passed, 0 failed (includes 18 newresponse_header_rewritetests and 5 new publisher tests)- Both suggestions scratch-verified (fmt/clippy/tests; docs prettier check)
- CI: all 20 checks passing
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review summary
Reviewed 7871acdd56324d20c182e0c3093e5bd535dd96e9 against 80483011e30a446ac741b4423e36f8d142c5d2ac, branch fix/rewrite-origin-urls-in-response-headers into main. Approving with two P2 edge-case findings posted inline.
Inspected all five changed files and traced the rewrite through publisher response modes, cache capture/replay, all four adapters, and final header overrides. Cache-hit, repeated-policy-header, and previous-schema rejection tests confirm rewritten policy replay for the tested core cache implementation.
Validation and review context
cargo test -p trusted-server-core response_header_rewrite --locked: 18 passed. Initial compilation timed out; rerun completed.cargo test -p trusted-server-core publisher::tests --locked: 372 passed.cargo test -p trusted-server-core platform::template_cache --locked: 32 passed.cargo test -p trusted-server-core --locked --quiet: 2,916 unit tests passed; 4 doctests passed, 5 ignored.cargo fmt --all -- --check: passed.- An exact-module scratch executable and two
node --input-type=moduleChromium fixtures reproduced the inline findings. - CI: All 20 reported checks passed; none reported failed or skipped.
- Existing feedback: Inspected the review, inline comments, unresolved threads, replies, and issue comments. Neither finding duplicates existing feedback.
- Residual risk: Browser fixtures exercised the exact rewrite module, not a deployed adapter. Adapter suites, production Fastly cache, and lint checks were not rerun locally.
- Revision remained unchanged; working tree is clean. No repository files were edited and no review work was delegated.
A scheme-change redirect such as https://origin.example.com?x=1 or one with dot segments was compared to the request path as raw text, so it did not match /?x=1 and was rewritten back to the URL the browser had just requested, looping. Parse the target as a URL so the comparison sees the same path and query the browser will follow. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Deduplication compared whole source expressions case-insensitively, so a policy listing origin paths /A.js and /a.js gained a serving-host sibling only for /A.js and the rewritten /a.js stayed blocked. Compare scheme and host case-insensitively and the path exactly, as CSP path matching does. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
When the origin forces a different scheme than publisher.origin_url, every navigation leaves the serving host and bypasses Trusted Server. That almost always means origin_url has the wrong scheme, so surface it as a warning naming the setting to check. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Same-scheme redirects to the current URL are now kept on the serving host, but Set-Cookie Domain attributes are not rewritten. A cookie scoped to the origin host is rejected on the serving host, so a cookie-gated self-redirect repeats. State this in the navigation rewrite docs and the configuration guide. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
…ls-in-response-headers Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
aram356
left a comment
There was a problem hiding this comment.
Summary
Rewrites publisher-origin URLs in Location, Content-Location, Refresh, Link, and CSP headers so the browser stays on the serving host (closes #1128). The rewrite holds up in a real browser, but the branch now conflicts with main after #1210, and #1210 changes the right resolution for the schema bump.
2 of the inline comments below carry a one-click GitHub
suggestion. Use Commit suggestion (or Add suggestion to batch for both) to apply them as commits on the PR branch. The remaining comments describe the fix in prose because the change touches multiple files or lines outside the diff and can't be auto-applied.
Blocking
🔧 wrench
- Conflicts with main; drop the v6 schema bump: see inline at
crates/trusted-server-core/src/platform/template_cache.rs:41
Non-blocking
🤔 thinking / 📌 out of scope / ⛏ nitpick
- 🤔 A Host override turns a canonical-host redirect into a loop: see inline at
crates/trusted-server-core/src/response_header_rewrite.rs:92 - 🤔 A serving-host CSP source also allows
/first-party/proxy(suggestion): see inline atdocs/guide/configuration.md:828 - 📌
<meta http-equiv>refresh and CSP in the page body are not rewritten: see inline atdocs/guide/configuration.md:783 - ⛏ Allocate lazily in
rewrite_link(suggestion): see inline atcrates/trusted-server-core/src/response_header_rewrite.rs:296 - ⛏ Reuse
request_path_and_query: see inline atcrates/trusted-server-core/src/publisher.rs:4523
Cross-cutting / body-level findings
-
🏕 CHANGELOG entry: operators upgrading will see changed
Locationvalues and wider CSP headers on proxied pages. A suggested line under### Fixed:- Publisher responses now map origin URLs in
Location,Content-Location,Refresh, andLinkheaders to the serving host, and each CSP source naming the origin gains a serving-host source beside it. A redirect to the current URL under a different scheme is left unchanged to avoid a loop. See "Origin URLs in proxied response headers" in the configuration guide.
- Publisher responses now map origin URLs in
Verification
- Browser checks from the test plan's unchecked item, run with the Axum dev server and Chrome at
0d0689aca.Location,Refresh, and theLinkpreload stay on the serving host, and the origin log shows no direct hits. In the CSPimg-srccase, the rewritten image loads with no violation. The scheme-upgrade redirect is left unchanged, with one hop and no loop. On main, the same headers pass through unrewritten. - A scratch merge with
origin/main(da31a215e), resolved as described in the 🔧 comment, passes fmt, clippy-fastly, test-fastly (2,919 core tests), test-axum, test-cloudflare, test-spin, and parity. - Both suggestions are scratch-verified. The docs one passes the docs prettier check. The Rust one passes fmt, all six adapter clippy aliases, the four adapter test suites, and parity.
CI Status
All checks ran on head 0d0689aca, which predates #1210. Nothing has run on a merge with current main.
- Analyze (actions): PASS
- Analyze (javascript-typescript): 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
- format-docs: PASS (required)
- format-typescript: PASS (required)
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- vitest: PASS
Keep template schema version 5 from main. The core build digest added in #1210 already invalidates templates stored before the response header rewrite, so the version 6 bump is no longer needed. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
A GET or HEAD redirect to the current URL that sets no cookie loops through Trusted Server when the origin redirects on something sent unchanged, such as an origin Host override. Leave its Location unchanged and log a warning. Also allocate lazily when rewriting Link values, reuse the captured request path and query, document the meta tag gap and the proxy reach of serving-host CSP sources, and add a changelog entry. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Summary
Location/Refreshoff the appliance, preloadedLinktargets straight from the origin, and blocked rewritten resources under a CSP that only allowed the origin.Location,Content-Location,RefreshandLink(includingimagesrcset) are rewritten. For CSP, a serving-host source is added beside each origin source, and no origin source is removed. Each response's own header values are rewritten, so per-page policies survive; this is not one static replacement.http://origin forcing HTTPS) is left unchanged so it can't loop. So is a cookielessGET/HEADredirect to the current URL under the same scheme, which loops when the origin redirects on something sent unchanged (such as an originHostoverride); it is logged atwarn. Same-scheme redirects to the current URL after a form POST, or that set a cookie, are still rewritten.Changes
crates/trusted-server-core/src/response_header_rewrite.rsLocation,Content-Location,Refresh,Link(<target>and quotedimagesrcset) and CSP/CSP-Report-Only. CSP is additive with de-duplication, andreport-uri/report-toare skipped. Includes the scheme-change and cookieless same-URLGET/HEADloop guards and unit tests.crates/trusted-server-core/src/publisher.rsGETredirects to the current URL, and a template-cache hit replaying the rewritten CSP/Link. Reuses the captured request path and query for the readthrough reader URL and the template key.crates/trusted-server-core/src/lib.rspub(crate)).docs/guide/configuration.md<meta http-equiv>refresh/CSP tags in the body are not rewritten, that a serving-host CSP source also allows/first-party/proxy, that[response_headers]still overrides the rewrite, and that host rewriting cannot authorize inline inserts that lack a nonce.Closes
Closes #1128
Test plan
cargo test-fastly && cargo test-axum(alsocargo test-cloudflare,cargo test-spin, and theparityintegration test)cargo clippy-fastly && cargo clippy-axum(alsoclippy-cloudflare,clippy-cloudflare-wasm,clippy-spin-native,clippy-spin-wasm)cargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest run(Node 24.12.0, per.tool-versions)cd crates/trusted-server-js/lib && npm run formatcd docs && npm run format(plus the markdown check outsidedocs/)cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serveNot yet done: re-running the issue's browser checks (Chrome against the Axum dev server) for
Location,Refresh,Linkpreload and the CSPimg-srccase.Checklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)