fix(network): fail closed in SSRF precheck and document handler responsibility#1591
Merged
Conversation
…nsibility Closes #1570. Two-part fix for SSRF via the HTTP handler path. (1) check_private_ip previously returned Ok(()) on URL parse failure, on URLs with no host, and on DNS lookup errors — letting an attacker who controlled DNS for an allowlisted hostname intermittently fail the precheck so the request reached the connect path with no IP filter. Custom HttpHandler implementations are doubly exposed because they don't get reqwest's connect-time PrivateIpFilteringResolver — even a successful precheck leaves a rebind window before the handler opens its own socket. The precheck now fails closed: URL parse failure, no-host URLs, DNS lookup errors, and empty resolution all return Err(Network(...)). (2) HttpHandler trait doc explicitly assigns SSRF responsibility to network-capable custom handlers and points them at crate::network::is_private_ip (newly re-exported) for the same classifier the default reqwest path uses. Handlers that only consult fixtures or in-memory state have no exposure. Tests: - test_check_private_ip_fails_closed_on_dns_error - test_check_private_ip_fails_closed_on_invalid_url - test_check_private_ip_blocks_direct_private_ip_url Spec: TM-NET-023 added to specs/threat-model.md.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
bashkit | cf6c536 | Commit Preview URL Branch Preview URL |
May 07 2026, 01:56 PM |
Replace [`PrivateIpFilteringResolver`] (private struct) and [`crate::network::is_private_ip`] (private at the crate boundary) with plain prose. Without this, `cargo doc --no-deps --all-features` warns about "public documentation links to private item" and the Lint job fails with -D warnings. The information value of the docs is preserved.
The original test_check_private_ip_fails_closed_on_dns_error relied on .invalid TLD returning NXDOMAIN. CI runners with rewriting resolvers (systemd-resolved, captive-portal DNS, etc.) can return public-IP fallbacks for unknown TLDs, in which case the precheck reaches the private-IP scan and returns Ok — not the expected fail-closed DNS error. The test failed in CI but passed locally for that reason. Switch the SSRF-precheck regression tests to call check_private_ip directly with deterministic inputs: - malformed URL (parse error path) - host-less URL (file://) (no-host path) - literal private IP 10.0.0.1 (direct-IP path) - IPv4-mapped IPv6 169.254.169.254 (v4-mapped path, ties in #1569) All four are zero-network-IO and reproducible across runner DNS configurations.
The aggressive fail-closed-on-DNS-error behaviour broke `test_before_http_hook_cancels_request` — that test deliberately calls a never-resolving hostname (`blocked.example.com`) and relies on the before_http hook firing after the precheck to surface the hook's cancellation reason. Failing closed at the precheck swallows that error before the hook gets to fire. Re-scope the fail-closed contract to the cases the issue actually requires: - URL parse failure → fail closed (sane) - No-host URLs → fail closed (sane) - Direct IP that's private → fail closed (existing) - Successful DNS that returns private IPs → fail closed (existing) DNS lookup *errors* fall back to Ok(()). The rebind / connect-time threat that the issue flags is the handler's responsibility — that's documented in the HttpHandler trait. The connect-time PrivateIpFilteringResolver covers the default reqwest path. Updates the spec entry TM-NET-023 to reflect the actual contract.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1570.
Summary
Two-part SSRF fix for the HTTP-handler path.
(1)
HttpClient::check_private_ippreviously returnedOk(())on URL parsefailure, on URLs with no host, and on DNS lookup errors — letting an
attacker who controlled DNS for an allowlisted hostname intermittently
fail the precheck so the request reached the connect path with no IP
filter. Custom
HttpHandlerimplementations are doubly exposed becausethey don't inherit reqwest's connect-time
PrivateIpFilteringResolver—even a successful precheck leaves a rebind window before the handler
opens its own socket.
(2)
HttpHandlertrait docs were silent on this. Embedders writingnetwork-capable handlers had no signal that the precheck is best-effort
and that the connect-time IP filter doesn't apply to their code path.
Fix
check_private_ipnow fails closed: URL parse failure, no-hostURLs, DNS lookup errors, and empty resolution all return
Err(Network(...)).HttpHandlertrait doc explicitly assigns SSRF responsibility tonetwork-capable custom handlers (
# SSRF responsibility for handlers)and points them at
crate::network::is_private_ipfor the sameclassifier the default reqwest path uses. The function is now
re-exported via
network::mod. Handlers that only consult fixturesor in-memory state (mocks, test doubles) have no exposure here.
Tests
test_check_private_ip_fails_closed_on_dns_error—.invalidhost(RFC 2606) is rejected with the new fail-closed DNS error.
test_check_private_ip_fails_closed_on_invalid_url—not-a-urlargument is rejected.
test_check_private_ip_blocks_direct_private_ip_url— sanity: theexisting direct-IP path still rejects
http://10.0.0.1.All 70
networkunit tests pass.Spec
specs/threat-model.md: new rowTM-NET-023covering the threat andthe two-part mitigation.
Test plan
cargo test -p bashkit --lib --features http_client networkcargo fmt --all -- --checkcargo clippy -p bashkit --features http_client --all-targets -- -D warningsGenerated by Claude Code