Repository navigation
feat(bep): parse request heads strictly and forward a canonical head and body - #2362
norrietaylor merged 15 commits into
Conversation
Add a strict HTTP/1.1 head parser that closes on the h2 prior-knowledge preface and on ambiguous framing, records a framing_ambiguous audit event, strips an h2c upgrade, and forwards the canonical re-serialization of the parsed head (BEP-074, BEP-077). Response heads lose every Alt-Svc header (BEP-076). Idle upstream connections are kept per box and authority, so no two boxes share one.
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
Reviewed: 1 finding, none blocking at Address the comments on the diff. Clobber: none: all three files are new or one-line additions. |
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The parser and the pool conform to §5.7. The h2 preface closes the connection, h2c is stripped, Alt-Svc is stripped, only canonical heads go upstream, and upstream connections are keyed per box. One condition is open. The PR body defers the production audit sink, the debug line for each closed connection, and the body relay, but it names no issue for them. Line 216 of §5.7 requires an audited close and no parser desync, so these three items need an owner. File or name the issues and list them in the PR body (C1).
Ruled at d24c6cd against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
|
/revise address ALL review concerns |
|
That Nothing to type again: the command gets a rocket when its round starts, or a refusal that names the reason. No round was counted for it yet. |
The request parser now closes on a head with more than one Host field. The upstream pool keeps at most MAX_IDLE_PER_KEY idle connections per box and authority, and removes a key when its idle list is empty.
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The compare conforms to §5.7. The duplicate Host refusal is a fast close (line 218), and the idle cap keeps the per-box key (line 216). One gap stays open. Line 216 requires an audited close on ambiguous framing, but no AuditSink exists outside the tests, and no caller logs CloseReason::rule(). The PR body names this work but links no issue. File the issue and link it, as C1 says.
Ruled at 08ed473 against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
|
Round 1 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
A request head with the h2 preface or ambiguous framing closes the connection, and a framing refusal records a framing_ambiguous audit event. Every forwarded head is the canonical re-serialization of the parsed request. A head with two Host fields now also closes the connection, and each box keeps its own capped set of idle upstream connections. Notes from the agent: Revise round 1. Two threads led to code changes: duplicate Host is refused as Malformed("duplicate_host"), and the upstream pool has a per-key idle cap (MAX_IDLE_PER_KEY = 8) and removes empty keys. Three threads ask for the PR body to name follow-up issues. I cannot edit the PR body, so I list the follow-ups here for the runner: (1) a production AuditSink that writes the framing_ambiguous event to the local audit log read by |
|
Round 2 of 10 on a /revise note: the coder changed nothing. Revise round 2: no code change. The two foundry findings (duplicate Host, idle cap per key) were fixed in 08ed473 and that fix is still at the branch head. The four minimal-arch C1 threads ask for issue links in the PR body. A coder cannot write the PR body and has no gh credential, so the runner or a person must add these follow-ups to the PR body: (1) the production AuditSink that appends the framing_ambiguous record to the min/v1 audit log, so that Review threads the coder judged:
1 outdated runtime thread(s) were not judged: |
There was a problem hiding this comment.
Babysitter review of 08ed473 (T8: strict HTTP/1.1 head parser, canonical re-serialization, per-box upstream pool). Read as a COMMENT review so it starts no coder round.
What I checked. parse_request refuses the h2 preface on its first line, obs-fold, Content-Length with Transfer-Encoding, more than one Content-Length element (equal or not), a non-decimal or empty length, any transfer coding other than exactly one chunked, a bare CR or LF, whitespace before a colon, a second Host, and a head over 64 KiB. Nothing reaches the upstream on a refusal, and only a framing refusal records framing_ambiguous. strip_h2c removes the h2c token, HTTP2-Settings and their Connection tokens while keeping other upgrades and options. relay_response_head drops every Alt-Svc spelling. UpstreamPool keys idle connections by (box, lowercased host, port) and checkin can only return a connection under the key it was checked out with, so no path shares a connection across boxes. The two proptests pin the canonical round trip and the framing oracle. No blocking finding from me.
Findings, none blocking
-
parse_request_lineforwards the target as received, and the pool is keyed by host. A box can send an absolute-form target whose authority differs fromHost(GET https://other.example/ HTTP/1.1withHost: api.github.com). RFC 9112 §3.2.2 says a recipient uses the absolute-form authority overHost, so the proxy would pin the connection and the grant to one authority while the upstream routes by another. The connection still goes to theHostserver, so the exposure is bounded to that server, but the check is cheap: refuse (Malformed("target_authority")) when an absolute-form target's authority is notHost, or rewrite it to origin-form beforeserialize. If the authority check belongs to the task that resolves the host set, say so in the module doc and leave it there. -
HTTP/1.0 request lines close as
Malformed("http_version"). Git and curl send HTTP/1.1, so this is fine if intended by BEP-077, but the module doc lists the close reasons and does not say that HTTP/1.0 is one. One sentence there. -
A request with no
Hostparses as complete and is forwarded. The caller has to have an authority to check out an upstream, so it must check; the doc onforward_request_headshould say the authority check is the caller's.
Residuals. I agree with minimal-arch's C1: the production AuditSink that appends to the min/v1 audit log, the body relay that re-frames from Forwarded.request.body, and the per-close debug line all belong in one filed issue linked from the PR body. The coder cannot file it or edit the body, so that is on the person running this epic, and the PR waits on it before merge.
Generated by Claude Code
|
/revise The epic owner decided (2026-10-19) to fix in this PR the two of minimal-arch's C1 residuals that fit, and to file the third. This round:
The production Generated by Claude Code |
drop_box now moves the box id's epoch on. A connection checked out before drop_box carries the old epoch, and checkin drops it, so a later box with the same id never gets a dead box's connection.
relay_request_body copies exactly Content-Length bytes, or parses the box's chunked framing and writes canonical chunks upstream. Raw chunked bytes never go upstream. forward_requests is the request half of a box connection. It writes one tracing::debug! line per closed connection, with the box id and CloseReason::rule(). The parser also refuses a request with no Host, an absolute-form target that names another authority, and a Connection option that names Content-Length, Transfer-Encoding or Host. Response heads now pass the same framing checks as request heads. The tracing dependency in crates/bep/Cargo.toml (and its Cargo.lock line) is outside this task's layers. The epic owner accepted it as a layer exception, the same line that #2361 adds.
|
Round 3 of 10 on a /revise note, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
A head with ambiguous framing or the h2 preface closes the connection, and nothing reaches the upstream. Every forwarded head is the canonical serialization. A request body now goes upstream re-framed: exactly Content-Length bytes, or canonical chunks parsed from the box's chunked body. forward_requests writes one debug line per closed connection with the box id and the rule that closed it. Notes from the agent: Body relay: relay_request_body and the connection loop forward_requests are new in http.rs. The tests content_length_body_is_relayed_exactly, chunked_body_is_reframed and client_closing_mid_body_closes cover them. A close between two requests is now CloseReason::ClientClosed, and a close inside a body is BodyEof. Chunk extensions and trailer fields are dropped, and are not forwarded. On a body error, part of the body can already be upstream, and the caller must drop that upstream connection. The doc comment says so. Outside the layers: crates/bep/Cargo.toml adds tracing.workspace, and Cargo.lock gets the matching line. The directive accepted this, and the commit body says so. Response heads now fail closed when Transfer-Encoding is anything other than one chunked. This is stricter than RFC 9112, which allows a close-delimited body there. The production AuditSink is #2435, so it is not here. I did not resolve any thread, because only the reviewer resolves threads. Threads PRRT_kwDOPdfcxM6uGRq6 and PRRT_kwDOPdfcxM6uGb8Z (audit sink) keep their verdict, and their pointer is #2435. I give a new verdict for the body-relay and debug-line C1 threads, because this round fixes them. just clippy-strict and just ci were not run. I ran clippy -p bep --all-targets, which was clean, and cargo fmt --check. |
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The compare conforms to §5.7. Request bodies are re-framed from the parsed head, so raw box bytes never go upstream, as the framing discipline at line 216 requires. A Connection option that names a framing header is now refused and audited as framing_ambiguous. The pool epoch keeps a connection from a dropped box away from a later box with the same id. C1 stays open as filed residual #2435: the production AuditSink that writes the marker to the min/v1 log. Update the PR body. Its notes about logging and body relay are stale at this head.
Ruled at e94f459 against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
drop_box now removes the box id's generation instead of moving an epoch on, so the pool keeps no entry for a box that ended. Generations come from one counter and are never reused, so a connection checked out before the drop is still refused when it comes back.
parse_request closes with invalid_host on an empty Host, or one with user info, a path, a comma, a space or a bad port, so the upstream and the policy read one authority.
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The two new commits conform to §5.7 line 216. A box id dropped from the upstream pool no longer leaves an entry behind. A connection from before the drop never returns to a reused box id. The Host check refuses any value that is not a host and optional port, and it closes fast as line 218 requires. C1 is the audit sink that is still open. It is filed as #2435, and this change can merge with it open.
Ruled at 9283b40 against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
|
Round 4 of 10 on the runtime's review findings, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
A request head with ambiguous framing or the h2 prior-knowledge preface closes the connection, and every head the proxy forwards is the canonical re-serialization of the parsed request. This round, the parser also closes on a Host value that is not a host with an optional port, and the upstream pool keeps no entry for a box id after drop_box. Notes from the agent: Round 4. Added the test host_must_be_a_host_and_port (exit 0) and changed two inputs in host_and_absolute_target_must_agree, because an empty or user-info Host now closes as invalid_host before the target check. The whole bep suite passed once at the end: 26 passed, 1 skipped. The production audit sink that writes framing_ambiguous to the min/v1 log, so that |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The compare adds one flush after the forwarded head, a doc comment on parse_response, and one test. The flush keeps the head out of a buffer while the box waits for 100 Continue. That agrees with the §5.7 determinism rule (line 218). The flush runs only after a successful parse, so the audited framing refusal of line 216 does not change. The PASS on 87972328 stands for the rest of the change.
Ruled at 774dfc9 against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
|
Round 10 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
Ambiguous framing or the h2 preface closes the connection, and every forwarded head is canonical. The canonical head now also reaches the upstream at once, before the proxy reads the body. Notes from the agent: Revise round 10. forward_head now flushes the upstream after it writes the head. The new test expect_continue_head_is_flushed_before_the_body proves this with a BufWriter. parse_response keeps the strict request framing rules on purpose, and its doc comment now states the refusal and its examples. cargo clippy -p bep --all-targets and cargo fmt --check are clean. just clippy-strict was not run. |
|
The runtime will not revise this pull request again on its own: round 11 would exceed the cap of 10; 0 base merges were not counted (the last trigger was the runtime's review findings). A person decides now. Remove |
There was a problem hiding this comment.
Babysitter review of 774dfc9 (T8, rounds 3 to 9 on top of the 08ed473 head I reviewed earlier). Read as a COMMENT review so it starts no coder round.
What changed since my first review, and what I checked. The two non-blocking findings I raised on 08ed473 are closed: one Host is now required, it must be a plain host with an optional port, an absolute-form target's authority must agree with it (default port equals none, user info never matches), and a CONNECT target must be authority-form with an explicit port and agree with Host too. A Connection option naming Host, Content-Length or Transfer-Encoding, and an empty list element in a framing header, are refused before the head is forwarded. relay_request_body copies exactly Content-Length bytes or re-frames the box's chunks canonically with extensions and trailers dropped, chunk-size lines bounded whether or not their CRLF is already buffered, and the trailer section bounded by the head limit. forward_requests is the connection loop: it flushes the head before reading the body (so Expect: 100-continue can be answered), stops at a protocol switch without parsing the bytes after it, and writes one debug line per close with the rule. parse_response applies the same framing rules to the upstream on purpose. The pool now drops a connection checked out before drop_box ran for its box, and caps idle connections per box over all authorities. The quarantined round-5 commit is on the branch and main is merged in. No blocking finding from me.
Findings, none blocking
-
read_headreportsClientClosedfor an empty stream on both sides, so an upstream that closes between responses is logged as the client closing (the runtime's open LOW). APeerClosedname or a side parameter fixes the log line; the behaviour is right. -
HTTP/1.0 request lines still close as
Malformed("http_version")without a sentence in the module doc saying so (carried from my first review). -
PoolState::generationskeeps an entry per box id from its first checkout untildrop_box; a host that never callsdrop_boxfor a box grows the map by one entry per box.tracked_boxesmakes it visible, and the box lifecycle owns the call, so this is a doc point: say oncheckoutthatdrop_boxis required at box end.
Residual. The production AuditSink stays #2435, as the epic owner decided.
Generated by Claude Code
|
The runtime will not revise this pull request again on its own: round 11 would exceed the cap of 10; 0 base merges were not counted (the last trigger was a failed check suite). A person decides now. Remove |
|
/revise resolve comments and conflicts |
…ests-strictly-and-forward-a-canonical # Conflicts: # crates/bep/src/lib.rs
There was a problem hiding this comment.
arch-review: PASS · the full ruling
This head merges main into the branch. The PR's own changes to http.rs, upstream.rs and lib.rs are identical to head 774dfc9f, which passed. The merge adds crates/bep/src/audit.rs from #2361, and it does not change the meaning of any PR line. BEP-074, BEP-076 and BEP-077 still conform to §5.7 lines 208, 216, 218 and 219. The connection-loop task will connect AuditSink to AuditLog. That work is not in the layers of this task.
Ruled at 0398445 against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
|
Round 1 of 10 on a /revise note, pushed as Resolved conflicts in:
Verify Ambiguous framing or the HTTP/2 prior-knowledge preface closes the connection, and every forwarded head is a canonical re-serialization. This stays true after the merge of origin/main. Notes from the agent: This round resolved the merge conflict in crates/bep/src/lib.rs. The fix keeps both module lines: |
There was a problem hiding this comment.
Own review carried to 0398445. This head is the runtime's round 1 on the person's /revise resolve comments and conflicts: a merge of main (767c59f, T13) into the branch, resolving crates/bep/src/lib.rs. compare 774dfc9f...0398445a lists exactly the 17 files that main changed between f8431d0 and 767c59f, and the branch's own diff against main is the same 3 files (http.rs, upstream.rs, lib.rs) as before. The round-10 review on 774dfc9 and its thread re-verifications stand. minimal-arch ruled PASS on this head. All 23 threads stay resolved.
Generated by Claude Code
|
/review |
|
/revise Base merge only, per the epic owner's order (first green first, base rounds after each landing). T3 merged as 788b6ec (
Generated by Claude Code |
…ests-strictly-and-forward-a-canonical # Conflicts: # crates/bep/src/lib.rs
There was a problem hiding this comment.
arch-review: PASS · the full ruling
This head is a clean merge of main. The merge does not change http.rs or upstream.rs. lib.rs keeps the module lines from both sides. The PASS on 0398445 stands for BEP-074, BEP-076 and BEP-077 under §5.7. The production AuditSink stays the filed residual #2435.
Ruled at a6c15c9 against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
|
Round 2 of 10 on a /revise note, pushed as Resolved conflicts in:
Verify On the merged branch, a request with ambiguous framing or the h2 preface closes the connection, and every forwarded head is canonical. The bep crate now also carries the ca module from main. Notes from the agent: Base merge only. Resolved the conflict in crates/bep/src/lib.rs to keep both sides: audit, ca, http, keys, module, publish, seal, upstream. Neither side declares a signin module, so I added none. Git merged crates/bep/Cargo.toml and Cargo.lock without conflict. Every dependency appears once, and cargo clippy -p bep --all-targets left Cargo.lock unchanged and reported no warnings. http.rs and upstream.rs are unchanged. I did not run the workspace-wide just fix or just test. In their place I ran cargo clippy -p bep --all-targets, cargo fmt -p bep --check and the full bep nextest suite, and all of them passed. Outside bep, the merge took the main side without conflict. |
|
/revise Base merge only, per the epic owner's order (first green first, base rounds after each landing). T4 merged as 554eff3 and T12 as 5919d28 after this branch's last base merge, and the branch is dirty against
Generated by Claude Code |
…ests-strictly-and-forward-a-canonical
|
Round 3 of 10 on a /revise note, pushed as Resolved conflicts in:
Verify On the merged branch, a request that opens with the HTTP/2 preface or carries ambiguous framing closes the connection. Every forwarded head is the canonical re-serialization. The bep tests for this pass after the merge of origin/main 5919d28. Notes from the agent: Base merge only. I merged origin/main (5919d28) in commit 8b49a95. The only conflict was crates/bep/src/lib.rs. It now lists every pub mod from both sides once, in alphabetical order (signin, then upstream). crates/bep/Cargo.toml and crates/minimal/Cargo.toml merged cleanly and are identical to main. Cargo.lock came from the merge. Cargo resolved it with no further change (cargo metadata left no diff, and cargo tree --locked passes). http.rs and upstream.rs are unchanged. Checks run: cargo clippy -p bep --all-targets (clean), cargo fmt -p bep --check (clean), the six test lines, and one full cargo nextest run -p bep (80 passed, 1 skipped). I did not run |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
This head merges main (#2359, #2364) into the branch. The merge does not change http.rs or upstream.rs. The PASS on a6c15c9d stands for BEP-074, BEP-076 and BEP-077 against §5.7 lines 208, 216, 218 and 219. The production AuditSink stays open as residual #2435.
Ruled at 8b49a95 against gominimal/arch@3dab1097cda1 by claude-opus-5-5.
There was a problem hiding this comment.
Own review carried to 8b49a95. This head is round 3 on the base-merge directive: a merge of main (5919d28, T4 and T12) into the branch, resolving crates/bep/src/lib.rs and the lockfile. The branch's own diff against main is the same 3 files (http.rs, upstream.rs, lib.rs) as at 774dfc9, where it was reviewed, and at a6c15c9. minimal-arch ruled PASS on this head. All 23 threads stay resolved.
Generated by Claude Code
There was a problem hiding this comment.
Babysit gate for T8 #2362 passed on 8b49a95: every lane green, arch PASS, review threads resolved, rows carried, no needs-human. Squash subject checked against commitlint. Approving and enqueueing under the epic owner's merge-on-green delegation.
Generated by Claude Code
T8 — Parse requests strictly and forward a canonical head over HTTP/1.1
slice: S0
kind: task
covers: BEP-074, BEP-076, BEP-077
size: L
est: 450 code, 360 tests
executor: agent
difficulty: needs care
model: strongest
diagnostics: a refused head is audited
framing_ambiguous, andmin box audit <box>shows the markerobservability: one debug log line per closed connection naming the framing rule that closed it
layers:
- http:
- crates/bep/src/http.rs (new): the strict HTTP/1.1 parser and canonical re-serializer of BEP-077, with BEP-074's close and h2c strip and BEP-076's Alt-Svc strip
- crates/bep/src/upstream.rs (new): upstream connections keyed per box, never shared across boxes
- crates/bep/src/lib.rs: module lines
test: cargo next 8000 est run -p bep h2_prior_knowledge_preface_is_closed
test: cargo nextest run -p bep h2c_upgrade_is_stripped
test: cargo nextest run -p bep alt_svc_is_stripped_from_responses
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 upstream_connections_never_shared_across_boxes
produces:
- crates/bep/src/http.rs
- crates/bep/src/upstream.rs
consumes: none
proof: Ambiguous framing or the h2 preface closes the connection, and every forwarded head is canonical.
depends on:
- blocked by #2258
Proof
In crates/bep, a client that opens a TCP connection with the HTTP/2 preface gets the connection closed and nothing reaches the upstream. A head with Content-Length plus Transfer-Encoding, duplicate Content-Length, an obs-fold or a transfer coding other than one final chunked is refused and records a framing_ambiguous audit event for the box. Every forwarded head is the canonical re-serialization of the parsed request (lowercase names, trimmed values, HTTP/1.1), and it parses back to the same request. h2c upgrades and Alt-Svc response headers are stripped. Idle upstream connections are held per box and authority and never go to another box.
Notes from the agent: Observability gap: the bep crate has no logging dependency, and Cargo.toml is not in this task's layers. So http.rs does not write the debug log line itself. CloseReason::rule() returns the name of the framing rule that closed the connection. The caller that owns the connection loop must log one debug line per closed connection with that name. Diagnostics gap: no audit log or
min box auditexists in the tree yet. forward_request_head records an AuditEvent { box_id, marker: "framing_ambiguous", rule } through an AuditSink trait in http.rs. The task that adds the local audit log must implement AuditSink so thatmin box audit <box>shows the marker. Design choices: only HTTP/1.1 request lines are accepted (an HTTP/1.0 request is refused as malformed). A Content-Length value that is not a single decimal number counts as framing_ambiguous. Upgrade: h2c removes only the h2c token, so another upgrade (for example websocket) and its Connection token stay. Body relay (chunked and length-framed) is not part of this change. forward_request_head returns the bytes read after the head for the caller to relay. I ran the whole bep suite once at the end: 19 passed. I deleted a proptest-regressions file that the first failing run created, and did not commit it.Tests
cargo nextest run -p bep h2_prior_knowledge_preface_is_closedcargo nextest run -p bep h2c_upgrade_is_strippedcargo nextest run -p bep alt_svc_is_stripped_from_responsescargo nextest run -p bep prop_forwarded_head_is_canonicalcargo nextest run -p bep prop_ambiguous_framing_is_refusedcargo nextest run -p bep upstream_connections_never_shared_across_boxesVerify
min task run agent-verifypassed ([tasks.verify]).Closes #2261
Residuals
AuditSinkthat appends theframing_ambiguousrecord to the min/v1 audit log (BEP-039); blocked on feat(bep): append one hash-chained audit record per proxy decision #2361 (T9).Generated by Claude Code