Repository navigation
docs(spec): scope broker allowlist to host and port - #133
Conversation
Require a matching port for every broker request and document 403 refusal for an allowed host on an unlisted port. Link the updated reference broker and its coverage. Refs: #131
tbhb-agent-reviewer
left a comment
There was a problem hiding this comment.
Claude Code, claude-opus-5-5, security focus. Changes requested.
Reviewed at 1292240a against the reference broker on internal broker-port-allowlist at cbb1474a. Full note: internal wiki/reviews/2026-10-09T0635Z-broker-port-allowlist-review.md on branch review-broker-port-allowlist (c2c293ff).
Matches the broker: exact host and port pairs, lowercase host, trailing dot refused, 403 for an allowed host on an unlisted port with nothing forwarded, default ports (CONNECT 443, https 443, http 80), and the residual risk paragraph without the "any port" clause.
Does not match:
- "The broker also checks port agreement between the request target and the
Hostheader. If the two ports differ, the request gets400." The broker checks this only for absolute form. Observed:CONNECT stub.rfc37-r8.test:443withHost: stub.rfc37-r8.test:22gets200and a tunnel to 443. Say the check covers absolute form and CONNECT is judged from its target alone (safe, since the tunnel goes to the judged target), or have the broker refuse it. - "A missing request port defaults to 443 ..." The broker also defaults a present but malformed port:
:abc,:-1,:+22, an empty port, andhttp://host:22?qare judged as 443 or 80 and forwarded. I've asked for the broker to answer400for any port that isn't 1 to 65535 in ASCII digits; once it does, the spec should state that400, and say whether leading zeros (:0443, now treated as 443) are canonicalized or refused. - The reference link now points at branch commit
cbb1474aand drops the section anchor. Point it at the merged commit and the findings section.
Verified: broker tests 20 of 20, repro_72.py, mise run check and ao check in the internal worktree. The #72 bypass set still holds.
Align the socket contract with the reference broker, including ASCII port parsing, numeric leading zeros, and CONNECT Host port checks. Link the reviewed findings section at the corrected broker head. Refs: #75
Link the final reviewed broker commit, including the leading-zero content length guard, from the contract findings section. Refs: #75
tbhb-agent-reviewer
left a comment
There was a problem hiding this comment.
Claude Code, claude-opus-5-5, security focus. Approved at 6137b17.
Re-reviewed against the reference broker at internal f005ebff. Note appended in internal wiki/reviews/2026-10-09T0635Z-broker-port-allowlist-review.md on branch review-broker-port-allowlist (40503452).
All three earlier points are resolved. The spec now says a present malformed port gets 400, that leading zeros resolve to the same port, and that CONNECT and absolute-form Host must agree with the target. The broker does all three: I reran my raw-socket probes (:abc, :-1, :+22, empty, :0, :65536, :², http://host:22?q, and CONNECT :443 with Host: :22). Each got 400 with nothing reaching the stub and no handler exception, and the #72 bypass set still forwards nothing. The reference link has its anchor back.
Non-blocking:
- The broker also answers
400for CONNECT with more than oneHostline, for an unbracketed IPv6 literal, and for an unclosed bracket. The spec doesn't list these. That leaves the spec less strict than the broker, not more permissive, but listing them would make the contract exact. - An origin-form request with two
Hostlines is still forwarded with both lines and judged on the first. This predates #75 and the spec is silent on it. It needs an issue, or an answer here, before merge. checkwas still pending when I reviewed.
What
Scope the broker socket contract's allowlist to exact host and port pairs. A listed host on an unlisted port gets
403before forwarding. A present malformed port gets400with no forwarding, including non-ASCII digits and out-of-range values. Leading zeros resolve to the same numeric port. CONNECT and absolute-form requests with aHostheader that disagrees with the target get400.Why
Internal #75 found that host-only entries allowed arbitrary ports. The independent security review also found malformed ports falling back to defaults and a CONNECT
Hostmismatch in the stand-in broker. The reference broker now refuses both.Evidence
f005ebffpasses 24 broker tests, including malformed-port socket cases, CONNECT port mismatch, long numeric headers, and 372 arbitrary authority and Host combinations checked for handler exceptions and allowlist confinement.Docs
specs/runtime-driver-contract.mdmatches the reference broker at internal commitf005ebff.Checklist
Refs: #131