Stop replaying request bodies on 301/302/303 proxy redirects

[dhruv8sh]

Summary

• The shared proxy redirect loop resent the original request body on every followed hop, so a 301/302/303 could deliver a POST payload to a redirect target the caller never addressed (open-mode callers permit any host).
• Followed 301/302/303 redirects for GET, HEAD and POST are now a bodyless GET (a HEAD stays HEAD) and drop body-describing headers (Content-Type, Content-Length, Content-Encoding, Content-Language, Content-Location). 307/308 keep the original method and body. The policy is documented on proxy_request.
• Redirect limits, scheme checks, and per-hop HTTPS/allowlist checks are unchanged.

Changes

File

Change

crates/trusted-server-core/src/proxy.rs

Track the outbound body alongside the method across hops in proxy_with_redirects; drop it and strip REQUEST_BODY_HEADERS after a 301/302/303; document the follow-up policy. Return 301/302 unfollowed for methods other than GET/HEAD/POST; pass HEAD responses through without body rewriting; warn when a redirect drops a body. Add tests for POST over 301/302/303/307/308, HEAD over 301/302/303 (including header-only compressed responses), PUT/PATCH/DELETE over 301/302/303, and a cross-host 307 blocked by the allowlist.

Behaviour notes

• A HEAD request redirected by 303 used to become GET; it now stays HEAD, which matches the Fetch spec.
• A 301/302 answering any other method (e.g. PUT, PATCH, DELETE) is no longer followed. The redirect response goes back to the caller with a warn log, so an update can't silently become a GET that reports success. On a 303, those methods still become a bodyless GET. No 301/302/303 follow-up ever replays a body.
• HEAD responses now skip HTML/CSS rewriting in finalize_proxied_response, so a header-only compressed HTML/CSS response no longer fails decompression with a 502.
• A 301/302/303 that drops a non-empty request body now logs a warning.
• GTM /collect POST beacons follow GA4 regional redirects. Browsers likewise turn a POST into a bodyless GET on 302, so those redirects are expected to be 307.

Closes

Closes #431

Test plan

• cargo test-fastly && cargo test-axum (plus cargo test-cloudflare && cargo test-spin)
• cargo clippy-fastly && cargo clippy-axum (plus clippy-cloudflare, clippy-cloudflare-wasm, clippy-spin-native, clippy-spin-wasm)
• cargo fmt --all -- --check
• JS tests: cd crates/trusted-server-js/lib && npx vitest run (no JS changes; run locally on Node 26 with NODE_OPTIONS=--no-experimental-webstorage)
• JS format: cd crates/trusted-server-js/lib && npm run format
• Docs format: cd docs && npm run format
• WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
• Manual testing via fastly compute serve
• Other: parity suite cargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test parity. The new POST test failed before the fix (["POST", "POST"] on 301), and temporarily removing the HEAD guard made the HEAD test fail.

Checklist

• Changes follow AGENTS.md conventions
• No unwrap() in production code — use expect("should ...")
• Uses tracing macros (not println!)
• New code has tests
• No secrets or credentials committed

[ChristianPavilonis]

Reviewed 7bc0e8781c880552d98ce939b569ada45dcfb755 against 80483011e30a446ac741b4423e36f8d142c5d2ac. The body-replay fix works, including mixed redirect chains. One limited HEAD compatibility regression is noted inline.

Validation: cargo test-fastly proxy::tests::proxy_request_ -- --nocapture passed 13 tests; cargo test-fastly proxy::tests:: -- --nocapture passed 137 matched tests; cargo test-axum passed 43 tests with one doctest ignored. cargo clippy-fastly, cargo fmt --all -- --check, and the PR diff whitespace check passed. A stdin Rust harness linked against the reviewed WASM library and run with viceroy run -C fastly.toml /tmp/ts-pr1239-redirect-review.wasm passed 15 mixed redirect chains. All reported CI checks passed.

The executed tests establish that dropped bodies and body headers do not reappear on later 307/308 redirects. Live platform transports were not exercised locally. No repository files were changed, and the review was not delegated.

[aleksUIX]

This also turns PUT, PATCH and DELETE into GET after a 301 or 302. A redirected update can then return 200 without making the update. Can we keep those methods, or return the redirect if replaying the body is blocked? Please add a PUT redirect test.

[prk-Jr]

Summary

301/302/303 follow-ups now go out as a bodyless GET (HEAD stays HEAD) with body-describing headers stripped, while 307/308 keep the method and body. This meets every "Done when" item in #431, and the redirect limits, scheme checks and per-hop allowlist/HTTPS checks are unchanged. Nothing blocking.

1 of the inline comments below carries a one-click GitHub suggestion. Use Commit suggestion to apply it.

Non-blocking

🤔 thinking

• GTM /collect beacons can lose their payload silently on a 302: see inline at crates/trusted-server-core/src/proxy.rs:1510

Cross-cutting / body-level findings

• 🌱 Open-mode 307/308 still resend the body to any host: callers that pass no allowlist still forward the body across a cross-host 307/308. testlight is one example (integrations/testlight.rs:199-206): it posts a JSON payload with the EC ID rewritten into it. This follows the Fetch spec and #431 keeps it in scope by design. A possible follow-up is to resend the body only on same-origin redirects when no allowlist is set.
• ⛏ The PUT → GET change is untested: the PR description says a PUT redirected by 301/302 also becomes a bodyless GET, but the tests cover only POST and HEAD. Running proxy_request_post_redirect_drops_body_for_301_302_303 for Method::PUT as well (for example, by looping over [Method::POST, Method::PUT]) would lock that behaviour in.
• 👍 Header list matches the Fetch spec: REQUEST_BODY_HEADERS is exactly the Fetch spec's request-body-header names plus Content-Length. It is also good that the new tests were confirmed to fail without the fix.

CI Status

• integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• browser integration tests: PASS
• CodeQL: PASS
• cargo test (ts CLI, native): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• cargo test: PASS
• vitest: PASS
• cargo fmt: PASS
• Analyze (javascript-typescript): PASS
• cargo test (axum native): PASS
• format-typescript: PASS
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• CLAUDE.md symlink guard: PASS
• format-docs: PASS
• Analyze (rust): PASS
• cargo test (cross-adapter parity): PASS
• prepare integration artifacts: PASS
• Analyze (actions): PASS

[dhruv8sh]

This also turns PUT, PATCH and DELETE into GET after a 301 or 302. A redirected update can then return 200 without making the update. Can we keep those methods, or return the redirect if replaying the body is blocked? Please add a PUT redirect test.

@aleksUIX thanks for pointing it out. Just pushing a fix regarding the same.

[ChristianPavilonis]

Review summary

Reviewed 70918731324d887419072cbc78b6d68d18be690f against 7a0ecb4cbfa39af3d83f7e1ed2733d7eabdc4cb1. No meaningful actionable issues introduced by this revision were found.

The change drops bodies on followed 301/302/303 redirects, returns update-method 301/302 responses unfollowed, and bypasses body rewriting for HEAD. All changed hunks were reviewed locally, including their callers, response consumers, and adapter transports.

Safety proof

• A stdin-compiled harness linked against the reviewed core and Axum libraries passed 175 real-HTTP mixed redirect chains. It checked exact methods and body bytes, all five body headers, preservation of unrelated headers, and unfollowed update redirects. Dropped payloads and body headers did not reappear on later redirects.
• The compressed HEAD regression passed under Viceroy for gzip HTML and Brotli CSS, preserving representation headers and removing the upstream CORS grant. The real-HTTP harness also exercised compressed HEAD finalization.

Validation and review context

• cargo test-fastly proxy::tests:: -- --nocapture: 140 matched tests passed.
• cargo test-axum: 43 tests passed; one doctest ignored.
• /tmp/pr1239-redirect-review: 175 real-HTTP cases passed. Its initial fixture failed configuration validation; correcting the fixture allowed execution.
• cargo clippy-fastly, cargo fmt --all -- --check, and git diff --check 7a0ecb4cbfa39af3d83f7e1ed2733d7eabdc4cb1...70918731324d887419072cbc78b6d68d18be690f: passed.
• All reported CI checks passed. Test and format workflow runs identify the reviewed head SHA.
• Inspected paginated reviews, inline comments, replies, issue comments, and thread resolution. Earlier HEAD, update-method, and warning feedback is addressed; both inline threads are resolved. No duplicate findings reported.
• Residual risk: Live Fastly, Cloudflare, and Spin transports were not exercised locally. GA4 regional redirect statuses remain unverified against live traffic. Open-mode cross-host 307/308 body replay remains intentional, pre-existing behavior.

No repository files were modified, and the review was not delegated.