fix(http): remove dead auto-redirect SSRF footgun; prove the rebind window is closed at connect (LC-152-TOCTOU) #290

Merged

longjacksonle merged 1 commit from fix/lc-152-pin-validated-ip into main

2026-05-31 15:55:34 +02:00

longjacksonle commented

2026-05-31 05:30:14 +02:00

Owner

Closes #286 (LC-152-TOCTOU). Filed investigate-first; the investigation changed the conclusion, so the deliverable differs from the literal title - details below.

What the investigation found

The ticket asked to "pin outbound connections to the SSRF-validated IP / close the DNS-rebind re-resolve window." Reading http_client.rs end to end, the live paths already do this:

For a hostname, PublicOnlyResolver is the authoritative resolution reqwest connects on, and it validates public-only on exactly the addresses it returns. lc152_resolver_property_pair::reqwest_connects_to_resolver_supplied_ip proves reqwest uses the resolver's IPs (no silent re-resolve). So a DNS rebind to a private address between submit-time validation and connect-time resolution is refused at connect, not left open.
Literal-IP URLs are rejected at submit (validate_url), before reqwest's literal-IP fast path (which skips the resolver) can fire.
The only redirect-following caller (link unfurl) re-validates every hop with its own outbound_get loop.

So there is no exploitable rebind residual on any path in use. The prior "residual TOCTOU / known gap" prose overstated it.

The one real latent hole (now removed)

outbound_get_following_redirects + its Policy::limited client relied on reqwest's auto-redirect, whose connector takes a literal-IP fast path that skips dns::Resolve. A response that 302s to a literal private IP (http://169.254.169.254/...) would have been followed with neither guard layer firing - an SSRF bypass. It had zero callers, so it was dead code that was also unsafe-if-used. Removed it, the redirect client, MAX_REDIRECTS, and the redirect plumbing; the shared client is now unconditionally Policy::none. The module now documents that any future redirect-follower must use the per-hop-validated pattern (mirroring the desktop net_guard manual loop, LC-210).

Hardening + proof

Factored the resolver core into resolve_public_only(host) and added in-module tests that the connect-time layer (not just validate_url) refuses loopback / RFC1918 / metadata literals and a loopback hostname, and accepts a public literal - pinning the "the resolver is the real guard" guarantee that was only covered indirectly.
Corrected the overstated residual-TOCTOU prose in ssrf.rs, bridge_avatar.rs, and docs/protocol-bridges.md. The only genuinely deferred item is a single connect-layer IP-pin (defense-in-depth against a hypothetical is_globally_routable bug), which still awaits a clean reqwest connector hook (Option A in the module doc).

Validation

just test + just test-saas + just check all green. New: 4 in-module resolver tests; existing 8 resolver-property + 4 error-propagation + grep-ban tests still pass.

No [operator-action]: the removed helper was never called, so no shipped path was exploitable - this is footgun removal + documentation accuracy, no behavior change for any in-use path.

🤖 Generated with Claude Code

Closes #286 (LC-152-TOCTOU). Filed investigate-first; the investigation changed the conclusion, so the deliverable differs from the literal title - details below. ## What the investigation found The ticket asked to "pin outbound connections to the SSRF-validated IP / close the DNS-rebind re-resolve window." Reading `http_client.rs` end to end, the **live** paths already do this: - For a **hostname**, `PublicOnlyResolver` is the authoritative resolution reqwest connects on, and it validates public-only on exactly the addresses it returns. `lc152_resolver_property_pair::reqwest_connects_to_resolver_supplied_ip` proves reqwest uses the resolver's IPs (no silent re-resolve). So a DNS rebind to a private address between submit-time validation and connect-time resolution is **refused at connect**, not left open. - **Literal-IP** URLs are rejected at submit (`validate_url`), before reqwest's literal-IP fast path (which skips the resolver) can fire. - The only **redirect-following** caller (link unfurl) re-validates every hop with its own `outbound_get` loop. So there is no exploitable rebind residual on any path in use. The prior "residual TOCTOU / known gap" prose overstated it. ## The one real latent hole (now removed) `outbound_get_following_redirects` + its `Policy::limited` client relied on reqwest's auto-redirect, whose connector takes a literal-IP fast path that skips `dns::Resolve`. A response that 302s to a literal private IP (`http://169.254.169.254/...`) would have been followed with **neither** guard layer firing - an SSRF bypass. It had **zero callers**, so it was dead code that was also unsafe-if-used. Removed it, the redirect client, `MAX_REDIRECTS`, and the redirect plumbing; the shared client is now unconditionally `Policy::none`. The module now documents that any future redirect-follower must use the per-hop-validated pattern (mirroring the desktop net_guard manual loop, LC-210). ## Hardening + proof - Factored the resolver core into `resolve_public_only(host)` and added in-module tests that the **connect-time** layer (not just `validate_url`) refuses loopback / RFC1918 / metadata literals and a loopback hostname, and accepts a public literal - pinning the "the resolver is the real guard" guarantee that was only covered indirectly. - Corrected the overstated residual-TOCTOU prose in `ssrf.rs`, `bridge_avatar.rs`, and `docs/protocol-bridges.md`. The only genuinely deferred item is a single connect-layer IP-pin (defense-in-depth against a hypothetical `is_globally_routable` bug), which still awaits a clean reqwest connector hook (Option A in the module doc). ## Validation `just test` + `just test-saas` + `just check` all green. New: 4 in-module resolver tests; existing 8 resolver-property + 4 error-propagation + grep-ban tests still pass. No `[operator-action]`: the removed helper was never called, so no shipped path was exploitable - this is footgun removal + documentation accuracy, no behavior change for any in-use path. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

longjacksonle added 1 commit

2026-05-31 05:30:14 +02:00

fix(http): remove dead auto-redirect SSRF footgun; prove + document the rebind window is closed at connect (LC-152-TOCTOU, #286 )

check-secrets / TruffleHog (pull_request) Successful in 4s

Details

check-secrets / TruffleHog (push) Successful in 6s

Details

check-secrets / Nosey parker (push) Successful in 6s

Details

check-secrets / Kingfisher (pull_request) Successful in 6s

Details

check-secrets / Nosey parker (pull_request) Successful in 5s

Details

check-secrets / Kingfisher (push) Successful in 8s

Details

Check / clippy + fmt + tests (pull_request) Successful in 1m59s

Details

Create release / Create release from merged PR (pull_request) Has been skipped

Details

9230df3950

Investigate-first outcome for #286. The ticket asked to "pin outbound connections to the SSRF-validated IP / close the DNS-rebind re-resolve window." Reading the guard end to end shows the LIVE paths already do this: for a hostname, `PublicOnlyResolver` is the authoritative resolution reqwest connects on and it validates public-only on exactly the addresses it returns (proven by `lc152_resolver_property_pair::reqwest_connects_to_resolver_supplied_ip`), so a DNS rebind to a private address between submit-time validation and connect-time resolution is REFUSED at connect, not left open. Literal-IP URLs are caught at submit, and the one redirect-following caller (link unfurl) re-validates every hop manually. So there is no exploitable rebind residual on any path in use.

The one real latent hole was the `outbound_get_following_redirects` helper plus its `Policy::limited` client: it relied on reqwest's auto-redirect, whose connector takes a literal-IP fast path that skips `dns::Resolve`, so a response that 302s to a literal private IP (e.g. `http://169.254.169.254/...`) would be followed with NEITHER guard layer firing - an SSRF bypass. It had ZERO callers (unfurl does its own per-hop-validated loop), so it was dead code that was also unsafe-if-used. Removed it, the redirect client, the `MAX_REDIRECTS` const, and the redirect plumbing; the shared client is now unconditionally `Policy::none`. Any future redirect-follower must use the per-hop-validated pattern (unfurl is the reference, mirroring the desktop net_guard manual loop from LC-210), now documented in the module.

Hardening + proof: factored the resolver core into `resolve_public_only(host)` and added in-module tests that the CONNECT-TIME layer (not just `validate_url`) refuses loopback/RFC1918/metadata literals and a loopback hostname, and accepts a public literal - pinning the "the resolver is the real guard" guarantee that was only covered indirectly. Corrected the overstated "residual TOCTOU / known gap" prose in `ssrf.rs`, `bridge_avatar.rs`, and `docs/protocol-bridges.md` to state accurately that the rebind-to-private window is closed at connect; the only deferred item is a single connect-layer IP-pin (defense-in-depth vs a hypothetical `is_globally_routable` bug), which still awaits a clean reqwest connector hook.

No operator action: the removed helper was never called, so no shipped path was exploitable; this is footgun removal + documentation accuracy. just test + just test-saas + just check all green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>