Repository navigation
T105 — Strip Alt-Svc on the live response relay and point the HTTP checks at the live path - #2791
Conversation
relay_responses now parses each response head on the flow, removes every Alt-Svc header and writes the canonical head. It then relays the body by the response's framing, and flushes each part as it arrives. It does this for every response on a kept-alive flow. Flow hands the response half each forwarded request, so a reply to HEAD is framed right. The HTTP head tests now drive forward_requests and relay_responses, the path bep serve runs. relay_response_head, which no live path called, is gone.
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
|
Approved: every review thread resolved at @norrietaylor resolved the last thread, which releases the approval this review had withheld. Nothing to do. |
There was a problem hiding this comment.
arch-review: CONDITIONS · 1 condition · the full ruling
The live relay now strips every Alt-Svc field and streams each body part as it arrives. The placement in bep conforms. One path fails open. ResponseRelay::run opens a raw tunnel on any 101, also when the request did not ask to switch. Later responses on the flow then reach the box unparsed, with Alt-Svc. Section 5.7 line 219 says the BEP must strip Alt-Svc from upstream responses. Make the change in C1.
Ruled at a6578aa against gominimal/arch@8640f85e193d by claude-opus-5-5.
There was a problem hiding this comment.
Requesting changes at a6578aa.
An unsolicited 101 turns the relay into a raw tunnel, and Alt-Svc gets through. crates/bep/src/forward.rs:1390.
ResponseRelay::runcallsself.tunnel()on any101. It never checks that the matching request asked to switch protocols.Sent.switchesis set at:770, and its doc comment at:762says it marks the last request on the flow, but nothing reads it.- Failure: the box sends
GET /awith noUpgrade. The upstream repliesHTTP/1.1 101 Switching\r\n\r\nand thenHTTP/1.1 200 OK\r\nAlt-Svc: h3=":443"\r\nContent-Length: 0\r\n\r\n. Everything after the 101 is copied unparsed, so theAlt-Svcreaches the box (BEP-076). - The request half keeps forwarding and parsing new requests, because it stops only after a request that asks to switch. So a hostile upstream can make the two halves lose step.
- Reproduced with a throwaway live-path test: the box received both responses verbatim,
Alt-Svcincluded. - Fix: enter the tunnel on
101only whenrequest.switchesis true. Otherwise refuse the response and close the flow (for exampleself.broken("unsolicited_switch")). Add a live-path test that pins the refusal and that no later byte reaches the box.
Test gaps, worth closing in the same round:
- No live-path test covers the relay's own refusals: a bad chunk size, a missing CRLF after chunk data, an oversized trailer, or EOF mid-chunk. The 204, 304 and close-delimited branches are also untested.
- The last assertion in
h2_prior_knowledge_preface_is_closed,relayed(responses, b"").is_empty(), passes whatever the relay does.
Checked and correct:
Alt-Svcis removed case-insensitively, from repeated headers, on interim 1xx and on every response of a kept-alive flow.- Obs-fold is refused, and so is whitespace before the colon. Trailers are dropped.
- CL with TE, duplicate CL and invalid CL are refused before any byte reaches the box.
- The head is capped at 64 KiB. The chunk-size line is bounded.
- Pipelined pairing stays in order.
- Each read is flushed before the next upstream read (BEP-N01).
- I found no panic reachable from upstream input.
- All six named tests pass, and so does the
bepcrate (233 run).
Arch (local build:arch-review at arch 8640f85e): PASS. BEP-076 conforms, with no conditions.
…svc-on-the-live-response-relay-and-poi
The response relay opened a raw tunnel on any 101. A hostile upstream could then send later responses unparsed, with their Alt-Svc headers. The relay now tunnels on a 101 only when the request asked to switch, and otherwise closes the flow before a byte reaches the box. Adds live-path tests for the refusal, a solicited switch, the chunked body refusals, and the 204, 304 and close-delimited branches. The h2 preface test now checks that no request reached the response half.
|
Round 1 of 10 on a review requesting changes, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
A response relayed to a box through the running proxy carries no Alt-Svc header, and the box still gets each chunk as the upstream sends it. An upstream can no longer get past the strip with a 101 that the request did not ask for: the flow closes, and no byte of that 101 or of a later response reaches the box. Notes from the agent: Revise round 1. ResponseRelay::run now refuses a 101 when Sent.switches is false, with the rule unsolicited_switch, before it writes any byte of the head. A 101 to a request that sends Upgrade, or to CONNECT, still opens the tunnel. New live-path tests: unsolicited_switch_is_refused (it fails with the check disabled, and passes with it), solicited_switch_opens_the_tunnel, broken_chunked_body_stops_the_relay (bad chunk size, no CRLF after chunk data, oversized trailer, EOF in a chunk), and bodiless_and_close_delimited_responses_are_relayed (204, 304, close-delimited). The last assertion of h2_prior_knowledge_preface_is_closed now checks that the request half sent no request record to the response half. Before, it relayed empty upstream bytes, and that check passed whatever the relay did. When a chunked body breaks the rules, the box keeps the head and the chunk bytes that arrived before the break. This is unchanged, and the tests pin it. cargo clippy -p bep --all-targets is clean. I did not run the whole crate suite or just clippy-strict. |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The live response relay strips every Alt-Svc header, as §5.7 line 219 requires. The fix for the earlier ruling is correct: a 101 now opens a tunnel only when the request asked to switch. Any other 101 closes the flow before a byte reaches the box. The tests drive forward_requests and relay_responses, which is the path that bep serve runs. No condition is open.
Ruled at 4b84400 against gominimal/arch@8640f85e193d by claude-opus-5-5.
There was a problem hiding this comment.
Approving at 4b84400. Commit 4b84400 fixes the finding from my last review: ResponseRelay::run now refuses a 101 unless the request asked to switch (unsolicited_switch). It also adds the live-path tests I asked for: the unsolicited and solicited switch, the four chunked-body refusals, the 204, 304 and close-delimited branches, and a meaningful h2 preface assertion. The merge of main brings no change of its own. The arch PASS (BEP-076) carries forward, because the fix only narrows when the relay stops parsing. The proxy.rs call-site change was accepted by the owner earlier, so I marked the PR ready again.
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 2 unresolved threads — minimal-foundry[bot] crates/bep/src/forward.rs:772, minimal-arch[bot] crates/bep/src/forward.rs:1445. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
There was a problem hiding this comment.
minimal-review — approved
Verdict APPROVE — every review thread is resolved; @norrietaylor closed the last one. The review of 4b84400dd54c withheld this approval on the threads open then. Posted as a comment: the App cannot approve its own pull request; the check and the label carry it.
T105 — Strip Alt-Svc on the live response relay and point the HTTP checks at the live path
slice: S15
kind: task
covers: BEP-076
size: L
est: 260 code, 360 tests
executor: agent
difficulty: needs care
model: strongest
diagnostics: a box that requests through the proxy sees no
Alt-Svcheader in any relayed response, so a client never learns an HTTP/3 endpoint to try past the proxy; the box sees the first chunk of a response before the upstream sends the rest, as beforeobservability: one debug line per relayed response that carried an
Alt-Svcheader, naming the box and the authority, beside the flow's existing spanlayers:
- relay:
- crates/bep/src/forward.rs:
relay_responsesreads each response head on the flow, removes everyAlt-Svcheader (BEP-076), writes the head, then streams the body chunks as they arrive by the response's framing (BEP-N01), for every response on a kept-alive flow; the testsalt_svc_is_stripped_from_responses,h2_prior_knowledge_preface_is_closed,prop_forwarded_head_is_canonicalandprop_ambiguous_framing_is_refuseddriveforward::forward_requestsandrelay_responses, the pathbep serverunstest: cargo nextest run -p bep alt_svc_is_stripped_from_responses
test: cargo nextest run -p bep h2_prior_knowledge_preface_is_closed
test: cargo nextest run -p bep prop_forwarded_head_is_canonical
test: cargo nextest run -p bep prop_ambiguous_framing_is_refused
test: cargo nextest run -p bep streamed_response_is_not_buffered
- http:
- crates/bep/src/http.rs: the four tests of the same names, which drive
relay_response_headandforward_request_headthat no live path calls, go; the response-head parse andstrip_alt_svcserve the live relaytest: cargo nextest run -p bep ambiguous_response_framing_is_refused
produces: none
consumes: none
proof: A response relayed to a box through the running proxy carries no Alt-Svc header, and the box still gets each chunk as the upstream sends it.
depends on:
- blocked by #2261
- blocked by #2272
Proof
A box that requests through bep serve gets each relayed response with no Alt-Svc header, on every response of a kept-alive flow (interim 1xx, HEAD replies, Content-Length and chunked bodies). The box still gets the first chunk of a streamed response before the upstream sends the rest (streamed_response_is_not_buffered runs through the live proxy).
Notes from the agent: Out of layers: crates/bep/src/proxy.rs, 3 lines in relay(). It takes flow.responses() before the request half moves the Flow, and passes it to relay_responses. This is necessary: a reply to HEAD (and a 2xx to CONNECT) can only be framed if the response half knows the request, and relay_responses had no way to learn it. Flow now carries an unbounded channel of Sent {head, connect, switches}. forward_requests sends one entry before it writes each head upstream. relay_responses gained a Responses argument (box id, authority, that channel). Framing follows RFC 9112 6.3: HEAD/1xx/204/304 have no body. 101 or 2xx to CONNECT, and a body with no framing header, are copied raw until close. A Content-Length body is copied exactly. A chunked body is re-serialized canonically, and its chunk extensions and trailers are dropped, the same as the request side. A response head or chunk that breaks the strict rules closes the flow with a debug line that names the rule. The observability line is tracing::debug with box_id, authority and status, in the box_connection span. In http.rs, the four old tests and relay_response_head are gone, and the shared proptest helpers moved to forward.rs tests. Response now carries its parsed
bodyframing, and parse_chunk_size is pub(crate). forward_request_head and http::forward_requests stay, because other http.rs tests still use them. The proxy.rs module doc line about relay_responses is still true and was not changed. One fullcargo nextest run -p bepafter the commit: 234 passed.Tests
cargo nextest run -p bep alt_svc_is_stripped_from_responsescargo nextest run -p bep h2_prior_knowledge_preface_is_closedcargo nextest run -p bep prop_forwarded_head_is_canonicalcargo nextest run -p bep prop_ambiguous_framing_is_refusedcargo nextest run -p bep streamed_response_is_not_bufferedcargo nextest run -p bep ambiguous_response_framing_is_refusedVerify
min task run agent-verifypassed ([tasks.verify]).Out of scope
These files are outside the task's
layers:, so this PR is a draft:crates/bep/src/proxy.rsCloses #2782