Allowlist a reverse proxy's Host + log bridge rejections

[evansenter]

Follow-up to #135, which merged with these items still open. Two gaps that only surface in the off-host-terminator topology config_from_args explicitly blesses — its three "correct only if something forwards between them" warnings.

1. A forwarding proxy's Host had no allowlist entry

The DNS-rebinding guard admits loopback literals, the hook URL's hostname, and a pinned non-wildcard --bind. None of those can match a reverse proxy:

• A non-loopback hook URL derives bind 0.0.0.0, and is_unspecified deliberately keeps the wildcard out of the allowlist.
• nginx's proxy_pass rewrites Host to the upstream address unless the operator adds proxy_set_header Host $host.

So the middleware sees e.g. Host: 10.0.0.5:8082 and 421s every dispatch, while /health reports registered: true forever. --bind was the only lever and can't fix it: pinning it drops the wildcard (a behaviour change with no obvious connection to a Host rejection), and a proxy rewriting to a name has no address to pin at all.

Adds --allowed-host / AGENT_EVENT_BUS_BRIDGE_ALLOWED_HOSTS, comma-separated, canonicalized through _host_from_header so a 10.0.0.5:8082 or bracketed-IPv6 entry collapses onto the same key as the incoming Host — the same normalization the derived entries already get.

2. No reject arm in the bridge logged anything

The 421 above, plus the 415, the two 413s, the 400s, and the 401, all returned silently. deliver's docstring commits to the opposite posture — "the bus discards the response body — so operator-facing visibility is this module's log" — and that held for delivered events only. For a rejected one there was nothing to see on this side, and the bus's own returned 421 warning sits on a different host in every non-loopback topology.

_log_rejection gives every reject the first-sighting-warn / repeats-at-debug shape already used by _warn_panes_once, the skew line, and the wake-failure guard. Rate-limited per reason, since Host and signature values are peer-controlled and would otherwise spam. The 421 line carries the rejected Host, the current allowlist, and the flag that admits it — that value is what makes the failure diagnosable at all.

3. Drive-by: the unserializable-payload 400 got its own string

It reused the parse-failure arm's "invalid JSON", collapsing two structurally distinct pre-durable failures. The body parsed fine — it just can't re-serialize to a standard-JSON spool line (deep nesting, or an inf/nan that allow_nan=False rejects). Different producer, different fix, and the response body is the only surface where the distinction can appear.

Testing

make check green: 565 passed (562 + 3).

• test_allowed_host_admits_a_forwarding_proxy — a listed proxy Host delivers under the derived wildcard bind; an unlisted one still 421s, so the escape hatch isn't a bypass.
• test_rejected_host_names_itself_and_the_fix — the warning carries the rejected Host and --allowed-host, and the second rejection inside the interval does not re-warn.
• test_allowed_hosts_from_flag_and_env — comma-separated on both surfaces, blanks dropped so a trailing comma or empty env var yields () rather than an "" entry.

Plus an autouse fixture clearing _reject_warn_state between tests (the rate-limit state is process-wide, so a leaked reading would make a later assertion depend on wall clock and run order), and AGENT_EVENT_BUS_BRIDGE_ALLOWED_HOSTS added to the env scrub list.

Docs: the guide's /hook section gains a "Behind a reverse proxy" paragraph naming the nginx default and the failure shape; CLAUDE.md's env var list gains _BRIDGE_ALLOWED_HOSTS.

Not included

bridge.py's _spool builds its symlink-refusal message with an os.readlink call inside the raise expression. If the planted link is removed between the is_symlink() check and that readlink, the deliberate named refusal becomes the unnamed 500 it exists to avoid. Real but very narrow, and unrelated to this change — worth its own small PR.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AFp3uCezpvbeyAMYnNrQ4R

---

Generated by Claude Code

[claude]

[Important] The blank-entry filter lives on the CLI path only, but the guard is built here.

config_from_args (line 1774) drops blanks and strips whitespace via if h.strip(), and test_allowed_hosts_from_flag_and_env states exactly why: an empty-string entry would match a Host-less request. But this loop is what actually seeds the allowlist, and it applies no such filter, so the protection exists only for callers that came through argparse.

The docstring on validate_config says the security posture must travel with the config, not with the entry point, and it normalizes port / cooldown_seconds / wake_dir in place for precisely the embedder paths (uvicorn --factory, an ASGI mount) that build a BridgeConfig by hand. allowed_hosts is the one new field that skips that treatment.

Concretely, an embedder writing the obvious thing:

allowed_hosts=tuple(os.environ.get("AGENT_EVENT_BUS_BRIDGE_ALLOWED_HOSTS", "").split(","))

gets a one-tuple holding the empty string. _host_from_header returns it unchanged, and it lands in allowed_hook_hosts. The middleware defaults raw_host to the empty string when no host header is present (an empty Host value yields the same), so the not in self.allowed test is False for any request that simply omits or blanks the header -- h11 permits that on HTTP/1.0, and an empty value on 1.1. The DNS-rebinding guard is then off for /hook and /health alike, silently, while /health keeps reporting registered: true.

A trailing-whitespace entry such as " proxy.example" has the mirror-image problem on the same path: never stripped, never matches, escape hatch silently inert.

Cheapest fix is to make this loop do the sanitation rather than trust the caller:

for extra in config.allowed_hosts:
    canonical = _host_from_header(extra.strip())
    if canonical:
        allowed_hook_hosts.add(canonical)

or normalize allowed_hosts in place in validate_config alongside the other coercions, which additionally covers the bare-str case (allowed_hosts="proxy.example") that currently iterates per-character.

[claude]

[Suggestion] A bare (unbracketed) IPv6 entry silently mangles into a non-matching hostname.

--bind accepts ::1 and fd7a::1 unbracketed and refuses anything unparseable loudly (test_invalid_bind_is_refused), so that is the muscle memory an operator arrives with. But --allowed-host fd7a::1 goes through _host_from_header, whose non-bracketed branch is raw_host.rsplit(":", 1)[0], yielding fd7a: -- not an IP, so it is kept verbatim as a hostname and can never match anything. No validation error, no warning; the flag is simply inert.

The recovery path mostly saves this: the new 421 line prints the rejected Host bracketed, so copying that value verbatim gives a working entry. But the help text and the guide paragraph both say nothing about brackets, and the embedder path has no log line to copy from. Worth either a clause in the help (bracket IPv6 literals) or accepting a bare literal by trying ipaddress.ip_address(extra) before falling back to _host_from_header.

[claude]

[Suggestion] Only one of the six new _log_rejection call sites is covered by a test.

test_rejected_host_names_itself_and_the_fix pins the 421 arm (message content plus the repeat-drops-to-debug behaviour), which is the one that most needed it. The 415, both 413s, the disconnect 400, and this post-process arm covering 401/400 are untested -- and this one is the odd shape of the set: reason is the human-readable error string (bad signature, payload not serializable to a spool line) while detail is only the status code, the inverse of the slug-reason / descriptive-detail convention at the other five sites.

That inversion is what couples the per-reason key here to the response strings of process. Bounded at four today, but a future arm interpolating anything peer-derived into its error string would turn _reject_warn_state into an unbounded dict and defeat the rate limit at the same time. One test asserting that a 401 produces exactly one WARNING, and naming the reason key it lands under, would nail both the arm and that coupling.

Separately, test_allowed_hosts_from_flag_and_env covers flag and env independently but not the precedence when both are set. The argparse-default idiom means the flag wins, which is the intended behaviour and is worth one line to lock in.

[claude]

[Suggestion] The 421 detail string is fully built for every rejection, including the ones that end up at DEBUG.

raw_host is peer-controlled and bounded only by the uvicorn header limit (~8KB), and sorted(self.allowed) is rebuilt per call. Under a scanner or a rebinding flood, every request past the first in the window pays the full f-string plus the sort to produce a line that is then discarded at the suppressed DEBUG level. The !r conversion does neutralize log injection (newlines get escaped), so this is cost rather than correctness.

Truncating the interpolated host (raw_host[:256]) would bound the worst case; hoisting sorted(self.allowed) to a create_bridge_app-time constant would drop the per-request sort, since the allowlist is immutable after construction.

[claude]

[Suggestion] Rate-limited PER REASON by wall clock -- it is the monotonic clock, not wall clock. _log_rejection reads _now(), whose own docstring calls it the Monotonic-clock seam, and that is the property that makes this window immune to an NTP step. The reset_reject_warn_state docstring in the test file has the same slip (pass or fail on 60s of wall clock). Trivial, but the comments in this file are load-bearing enough that the distinction is worth keeping straight.

[claude]

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review

Summary

Both gaps are real and the framing is right: a wildcard-derived bind genuinely leaves no lever for a proxy-rewritten Host, and a reject arm that logs nothing on the daemon side is undiagnosable when the 421 the bus records sits on a different machine. Routing allowlist entries through _host_from_header is the correct reuse, per-reason is the right rate-limit granularity for peer-controlled values, and splitting the unserializable-payload 400 off from the parse 400 is a clean drive-by.

One Important finding: the blank-entry sanitation that makes --allowed-host safe lives in config_from_args, i.e. on the CLI path only, while the guard it protects is constructed in create_bridge_app -- the same entry-point/config split the validate_config docstring exists to prevent. Four suggestions besides. All five are posted as inline comments on the diff.

Findings

• [Important] bridge.py:936 -- blank and unstripped allowed_hosts entries are filtered only on the argparse path; an empty-string entry reaching create_bridge_app admits Host-less requests.
• [Suggestion] bridge.py:1543 -- a bare (unbracketed) IPv6 --allowed-host silently parses to a non-matching hostname with no validation error, unlike --bind.
• [Suggestion] bridge.py:1168 -- five of the six new _log_rejection call sites are untested, and this one inverts the reason/detail convention in a way that couples the rate-limit key to response strings.
• [Suggestion] bridge.py:1252 -- the 421 detail (peer-controlled Host plus a re-sorted allowlist) is built eagerly even when the result is a suppressed DEBUG line.
• [Suggestion] bridge.py:279 -- the comment says wall clock where _now() is monotonic.

Previously Addressed (Filtered)

None -- no prior reviews or Feedback Addressed comments on this PR.

Verdict

REQUEST_CHANGES - Blank-entry filtering for allowed_hosts is CLI-path-only, so a hand-built config can seed an empty-string allowlist entry that turns the DNS-rebinding guard off.

Note: the sandbox blocked the gh api --input heredoc form (a shell guard rejects JSON braces), so the findings were posted as individual inline review comments rather than bundled into this review.

---

Automated review by Claude Code

[claude]

[Suggestion] --allowed-host is singular while the value is a comma-separated list, the env var is AGENT_EVENT_BUS_BRIDGE_ALLOWED_HOSTS, and the config field is allowed_hosts. The existing list-valued CLI flag in this project is plural (--exclude-types), so --allowed-hosts would match both the convention and the two sibling spellings of this same setting. Cheap to rename now, a breaking change for a supervisor unit file later.

[claude]

[Important] The blank drop above runs BEFORE canonicalization, but _host_from_header can itself PRODUCE an empty string from an entry that is not blank:

• :8082 -> no leading bracket, so the rsplit on the last colon yields the empty string -> ip_address raises -> the empty string is returned
• [] -> bracketed branch, split on the close bracket then lstrip the open bracket yields the empty string

That empty value is appended to canonical_hosts, reaches allowed_hook_hosts.update(config.allowed_hosts) in create_bridge_app, and matches the middleware raw_host default for an absent or blank Host header - i.e. the first of the three shapes the comment above documents this block as preventing, and exactly what test_blank_allowed_host_does_not_disable_the_guard asserts against for the ("",) spelling. --allowed-host :8082, read as "any host arriving on this port", is a plausible operator typo, and it silently turns the DNS-rebinding guard off for /hook and /health alike, while /health keeps reporting registered.

Testing the CANONICAL value rather than the pre-canonical one closes it: compute the canonical host into a local (either branch), and if it is falsy raise a named BridgeConfigError. A raise reads better than a continue here - unlike a trailing comma, a port-only entry is a typo the operator wants named rather than silently dropped. Worth a (":8082",) case alongside the existing ones in test_blank_allowed_host_does_not_disable_the_guard.

[claude]

[Suggestion] A non-empty allowed_hosts is itself evidence that something forwards to this listener - that is the whole motivation in the comment above. But the exposure derivation in validate_config (exposed = not _is_host_loopback(bind) or not _is_loopback(hook) or config.assume_exposed) does not consider it, so a loopback bind plus a loopback hook URL plus --allowed-host bridge.example demands no secret and emits none of the four "correct only if something forwards between them" advisories - even though the operator has just declared that a forwarder exists and rewrites Host to a public name.

Not a regression (a same-host proxy that leaves Host as the loopback literal already reached that state before this PR), and a hard refusal would be wrong, since the forwarder may carry its own auth. But an advisory in the config_from_args topology block, next to the loopback-bind-under-a-reachable-hook-URL warning, would fit the existing warn-do-not-refuse posture and name the one combination where the new flag widens reach without widening the secret requirement.

[claude]

[Suggestion] This paragraph sits under the POST /hook bullet, but the flag help text says "/hook & /health" and test_allowed_host_admits_a_forwarding_proxy exercises both. The /health bullet just below still ends with "probe by the hook URL hostname or a loopback literal" under a wildcard bind, which is now incomplete - a listed Host works there too, and a monitoring probe behind the same proxy is precisely the case that hits it. A clause on that bullet (... or a value listed with --allowed-host) would keep the two descriptions of the same allowlist in sync.

[claude]

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review

Summary

Moving the allowed_hosts sanitation out of config_from_args and into validate_config is the right answer to the prior round: the allowlist is assembled in create_bridge_app, so that is where the invariant has to hold, and each of the three hand-built shapes now has a test. The per-reason rate limit is the right granularity for peer-controlled values, and splitting the unserializable-payload 400 off the parse 400 is a clean drive-by.

One Important finding remains: the blank-entry drop runs BEFORE canonicalization, but _host_from_header can itself produce an empty string from an entry that is not blank, so the empty-allowlist-entry bypass this block documents is still reachable through a plausible operator typo. Three suggestions besides. All four are posted as inline comments on the diff.

Findings

• [Important] bridge.py:1652 - an entry like :8082 or [] canonicalizes to the empty string, lands in the allowlist, and matches the middleware raw_host default for a Host-less request.
• [Suggestion] bridge.py:186 - a non-empty allowed_hosts declares that a forwarder exists, but the exposure derivation ignores it, so no secret is demanded and no topology advisory fires.
• [Suggestion] bridge.py:1545 - --allowed-host is singular for a comma-separated list, against the plural env var, config field, and the --exclude-types precedent.
• [Suggestion] guide.md:531 - the new paragraph documents /hook only; the /health bullet just below still lists the old set of probe-able Host values.

Previously Addressed (Filtered)

No Feedback Addressed comment exists on this PR, so the prior review round was read off the current code:

• Resolved, not re-raised - [Important] blank/unstripped entries filtered on the argparse path only. Canonicalization now lives in validate_config, which create_bridge_app calls, so embedders get it.
• Resolved, not re-raised - [Suggestion] a bare unbracketed IPv6 --allowed-host was silently mangled. Now handled by the ipaddress.ip_address branch, with a test.
• Still open, deliberately not re-raised - the reason/detail inversion at the hook_endpoint catch-all plus the untested _log_rejection call sites, the eagerly built 421 detail string, and the "by wall clock" comment over a monotonic _now(). Unchanged in this revision; repeating them would be noise.

Verdict

REQUEST_CHANGES - an --allowed-host entry that canonicalizes to the empty string disables the DNS-rebinding guard for Host-less requests on both /hook and /health.

Note: the sandbox again blocked the gh api --input heredoc form, so the four findings are posted as individual inline review comments rather than bundled into this review.

---

Automated review by Claude Code

[claude]

[Suggestion] The new refusal catches the entries that canonicalize to the empty string, but the neighbouring operator slip -- pasting a URL instead of a Host value -- still lands silently inert, which is the failure mode this flag exists to remove.

_host_from_header("http://proxy.example") rsplits on the last colon, so the entry becomes "http"; "https://proxy.example:8082" becomes "https://proxy.example". Neither is empty, so both survive to the allowlist and can never match an incoming Host. The operator then gets the same symptom they came here to fix -- every dispatch 421'd -- with --allowed-hosts already set. The 421 warning does print the allowlist, so 'http' sitting in it is a clue, but a clue at runtime rather than a refusal at config time.

Since this block already commits to the named-refusal posture for a wish the flag cannot grant, the same treatment fits: reject a canonical entry containing / (or a stripped input containing ://), pointing at "Host values, not URLs".

[claude]

[Suggestion] The entries get a named BridgeConfigError, but the container does not: a hand-built BridgeConfig(allowed_hosts=None) -- a plausible embedder spelling of "no extras", and the shape every other optional string field on this config explicitly tolerates (if value is not None and not isinstance(value, str) immediately above) -- falls into this loop and raises a bare TypeError: 'NoneType' object is not iterable out of create_bridge_app.

That is the exact class of failure the surrounding block's own comment says it exists to prevent (a bare AttributeError off .strip()), just one level up. An is None -> () normalization beside the isinstance(..., str) one, or a non-iterable check, would keep the whole field on the named-error path.

[claude]

[Suggestion] These two comments contradict each other. The first says blank entries are dropped here; the second -- added by the same revision that moved the work -- says this is split-only and validate_config does the stripping and blank-dropping. The second matches the code: tuple(args.allowed_hosts.split(",")) keeps ("",) for an empty flag and hands it on.

The first two lines are a leftover from the previous revision. Worth deleting, or folding the surviving guarantee into the second comment ("a trailing comma or an empty env var still yields (), after validate_config drops the blanks") -- the outcome is still true and worth stating, it just no longer happens on this line.

[claude]

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review

Summary

Reviewed at head c4524cd, which landed after the previous round. The round-2 Important finding -- the blank drop running BEFORE canonicalization, so :8082 / [] / []:8082 reached the allowlist as the empty string and matched the middleware's raw_host default -- is genuinely fixed: emptiness is now tested on the canonical value, refused with a named BridgeConfigError rather than silently dropped, and test_port_only_allowed_host_is_refused_not_silently_dropped pins all three spellings. The --allowed-host -> --allowed-hosts rename and the /health guide bullet are done too. The canonicalization is idempotent across the double validate_config call (config_from_args, then create_bridge_app), which the ipaddress.ip_address fast path is what makes true for a bare IPv6 entry.

No Critical or Important issues at this head. Three suggestions, all on code this revision introduced, posted as inline comments.

Findings

• [Suggestion] bridge.py:1665 - a URL-shaped entry (http://proxy.example) canonicalizes to http and lands in the allowlist silently inert, right beside the new named refusal for the port-only spelling.
• [Suggestion] bridge.py:1642 - the entries get a named BridgeConfigError but the container does not: allowed_hosts=None raises a bare TypeError out of create_bridge_app.
• [Suggestion] bridge.py:1830 - the two stacked comments contradict each other; the first is a leftover from the revision that dropped blanks on this line.

Previously Addressed (Filtered)

No "Feedback Addressed" comment exists, so both prior rounds were read off the current code:

• Fixed in c4524cd, not re-raised - [Important, round 2] the canonical-empty entry reopening the Host-less bypass; [Suggestion, round 2] singular --allowed-host; [Suggestion, round 2] the /health probe bullet listing a stale set of Host values.
• Fixed in 1c03676 / 9d8eee2, not re-raised - [Important, round 1] blank-entry sanitation living on the argparse path only; [Suggestion, round 1] a bare unbracketed IPv6 entry mangled; [Suggestion, round 1] "wall clock" over a monotonic _now().
• Still open, deliberately not re-posted (unchanged since rounds 1-2; repeating them verbatim would be noise): the exposure derivation ignoring a non-empty allowed_hosts; five of the six _log_rejection call sites untested, plus the reason/detail inversion at the hook_endpoint catch-all; the 421 detail string built eagerly even when the result is a suppressed DEBUG line.

Verdict

APPROVE - the security-relevant finding from the prior round is fixed and tested; only suggestions remain.

Note: the sandbox again blocked the gh api --input heredoc form (a shell guard rejects JSON braces), so the three findings are posted as individual inline review comments rather than bundled into this review.

---

Automated review by Claude Code

[claude]

[Suggestion] allowed_hosts=None still lands as a bare TypeError.

The isinstance(config.allowed_hosts, str) guard above covers the bare-str shape, and the isinstance(entry, str) check inside the loop covers a non-string element -- but a None container reaches for entry in None and raises TypeError: NoneType object is not iterable, naming neither the field, the flag, nor the env var.

That is a plausible embedder slip precisely because it matches the sibling fields: hook_url, bind, and secret all default to None, so allowed_hosts=None reads as "the unset spelling" to anyone writing a config by hand. And it is the failure mode the block twenty lines up exists to prevent -- that comment argues a hand-built config must not fall through to an unnamed AttributeError/TypeError, which is why bus_url/hook_url/bind/secret get the named type check and wake_dir gets the TypeError-to-BridgeConfigError translation.

One line ahead of the loop closes it in the same idiom as the rest -- coerce None to () -- or, if silently accepting None reads as too lenient for a security-guard field, a named BridgeConfigError matching the two already in this block.

[claude]

[Suggestion] A constant "host" key lets unrelated traffic claim the window that the operator rejection needs.

The rate-limit key here is the literal string "host", so every 421 -- whatever Host it carried -- shares one 60s window. test_rejected_host_names_itself_and_the_fix pins that deliberately: two different rejected Hosts produce exactly one WARNING, and the second Host never appears above DEBUG.

That is the right call for the rebinding case the guard was built for (attacker-chosen values, unbounded cardinality). It cuts against the case this PR adds, though. The detail string entire payload is which Host was rejected -- the PR body calls that value "what makes the failure diagnosable at all" -- and under a wildcard bind on a shared network, any other 421 inside the window (a port scan, a monitoring probe on the wrong name, a stale second proxy) suppresses the proxy Host to DEBUG, which is invisible without DEV_MODE=1. The condition is persistent, so a later window recovers, but the first-sighting line an operator restarts the bridge to read may name someone else Host.

This module already has an idiom for a peer-derived warn key that must not grow without bound: the _WARN_KEYS_CAP = 256 sets behind _warn_panes_once and the wake-failure guard. Keying on the canonical rejected host under that cap would warn once per distinct rejected authority -- canonical, so the key space an attacker can burn is the same space the allowlist compares against -- while leaving the other five sites on their constant slugs.

[claude]

[Suggestion] Still open from round 1, restated once because it is the one place the new state can grow unbounded.

This arm passes the error string as reason and the status code as detail -- the inverse of the other five sites, which pass a short stable slug (content-type, too-large, disconnect, host) as reason and the descriptive text as detail. The consequence is not cosmetic: it makes the _reject_warn_state key set equal to the set of error strings process can return.

Today that is four fixed literals, so the dict is bounded at four. But nothing marks those strings as rate-limit keys at their definition sites, so a future arm interpolating anything peer-derived (a field name, a media type, an offending value) into its error payload would silently make the dict unbounded and defeat the rate limit for that arm in the same edit -- and this PR just demonstrated that these strings do get rewritten, since the unserializable-payload 400 changed from invalid JSON to payload not serializable to a spool line in this same diff. A slug at this call site (with the status and error string moved into detail, or a small status-to-slug map) decouples the two.

Coverage matches: test_rejected_host_names_itself_and_the_fix pins the 421 arm, which is the one that most needed it, but the 415, both 413s, the disconnect 400, and this arm have none. One test asserting that a bad-signature POST produces exactly one WARNING -- and naming the reason key it lands under -- would nail both the arm and the coupling.

Separately, test_allowed_hosts_from_flag_and_env covers the flag and the env var independently but not the precedence when both are set. The argparse default= idiom means the flag wins, which is the intended behaviour and worth one line to lock in.

[claude]

[Suggestion] The new paragraph is good, but the canonical flag inventory in this same file does not know about the flag.

Line 569 ("Flags mirror env vars: --port/..., --backend/..., --cooldown/..., --wake-dir/..., --bus-url/..., --hook-url/..., --bind/AGENT_EVENT_BUS_BRIDGE_BIND.") enumerates every bridge flag with its env twin, and goes on to explain the two deliberate naming exceptions (AGENT_EVENT_BUS_WAKE_DIR lacking the _BRIDGE_ infix, AGENT_EVENT_BUS_BRIDGE_SECRET being env-only). It is the one place a reader goes for "what can I set", and --allowed-hosts/AGENT_EVENT_BUS_BRIDGE_ALLOWED_HOSTS is missing from it -- so the pairing is discoverable only by already reading the /hook bullet for a problem you may not have diagnosed yet.

CLAUDE.md "When modifying API: Update CLI help, MCP docstrings, and guide.md together" is the convention this leaves half-done; the CLI help and CLAUDE.md env list both got the new name. Appending it to that sentence is the whole fix.

[claude]

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review

Summary

Both Important findings from the prior two rounds are closed in this revision, and closed at the right layer. The blank/unstripped/bare-str sanitation now lives in validate_config, which create_bridge_app calls, so the guard is protected on the embedder path that actually assembles the allowlist -- not just behind argparse. The empty-string-by-canonicalization hole (:8082, [], []:8082) is now a named BridgeConfigError tested on the CANONICAL value rather than the raw input, which is the correct place for that test since _host_from_header is what produces the empty string. The bare-IPv6 entry, the singular-to-plural flag rename, and the /health guide bullet are all handled too, and the "wall clock" comment now correctly says monotonic.

I re-derived the sanitation rather than trusting the tests: ("",) and " " hit the blank continue; ":8082", "[]", and "[]:8082" all reach an empty canonical and raise; a bare str expands to a one-tuple before the loop; and the two-pass case (config_from_args validates, then create_bridge_app validates the already-canonical tuple) is idempotent on every shape, since a canonicalized IP re-enters through the ip_address branch and a canonicalized name is a fixed point of _host_from_header. An empty incoming Host can no longer match anything in the allowlist.

No Critical or Important issues. Four suggestions, posted as inline comments on the diff.

Findings

• [Suggestion] bridge.py:1642 -- allowed_hosts=None raises a bare TypeError, the exact shape the surrounding block names for every other field.
• [Suggestion] bridge.py:1257 -- the rate-limit key is the constant "host", so unrelated 421 traffic can claim the window and demote the operator real proxy rejection to DEBUG.
• [Suggestion] bridge.py:1172 -- still open from round 1: the reason/detail inversion couples the rate-limit key set to process response strings, and 5 of the 6 _log_rejection call sites are untested.
• [Suggestion] guide.md:532 -- the canonical "Flags mirror env vars" inventory at line 569 does not list --allowed-hosts.

Previously Addressed (Filtered)

No "Feedback Addressed" comment exists, so the two prior rounds were read off the current code.

Resolved, not re-raised: blank/unstripped entries sanitized on the CLI path only; bare unbracketed IPv6 silently mangled; :8082/[] canonicalizing to the empty string; the singular flag name; the /health guide bullet listing a stale set of probe-able Hosts; "by wall clock" over a monotonic _now().

Still open, deliberately not re-raised: the eagerly built 421 detail string (cost, not correctness -- !r already neutralizes log injection), and the exposure derivation ignoring a non-empty allowed_hosts. On the latter I agree with the round-2 severity: a same-host proxy leaving Host as the loopback literal (the nginx $proxy_host default against a 127.0.0.1 upstream) already reached that no-secret state before this PR, so the flag does not regress it.

Verdict

APPROVE - the security-relevant findings from both prior rounds are fixed at the layer that owns the invariant; only suggestions remain.

Note: the sandbox again blocked both the gh api --input heredoc form and writing a JSON file, so the four findings are posted as individual inline review comments rather than bundled into this review.

---

Automated review by Claude Code