fix(http): pin SSRF-validated DNS targets - #181
Conversation
Greptile SummaryThe PR changes outbound SSRF validation to retain validated socket addresses, pins those addresses in reqwest, and bypasses proxies for protected requests while retaining fail-closed redirects.
Confidence Score: 2/5This PR is not safe to merge until localhost secret transport remains pinned to loopback and IPv6 literal targets are resolved correctly. The changed client construction can send plaintext secrets accepted as localhost traffic to a resolver-selected non-loopback address, and the new socket-address formatting rejects valid IPv6-literal URLs. Files Needing Attention: src/stdlib/http.rs
|
| Filename | Overview |
|---|---|
| src/stdlib/http.rs | Adds SSRF-safe DNS pinning, but removes loopback enforcement for secret-bearing localhost traffic and constructs invalid socket strings for IPv6 literal targets. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
URL[Outbound URL] --> Validate[Resolve and validate every address]
Validate -->|SSRF disabled| DefaultDNS[Use normal client resolution]
Validate -->|SSRF enabled| Pin[Disable proxies and pin validated addresses]
DefaultDNS --> Connect[Connect]
Pin --> Connect
Secret[Development plaintext Secret request] --> LoopbackCheck[Lexically check localhost or loopback IP]
LoopbackCheck --> DefaultDNS
Reviews (1): Last reviewed commit: "fix(http): pin SSRF-validated DNS target..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if direct_loopback_http { | ||
| // Plaintext development traffic must remain on loopback even when the process | ||
| // has system proxy settings or a nonstandard localhost resolver. | ||
| // has system proxy settings. | ||
| builder = builder.no_proxy(); | ||
|
|
||
| let parsed = reqwest::Url::parse(url).expect("secret transport URL was validated"); | ||
| if parsed | ||
| .host_str() | ||
| .is_some_and(|host| host.eq_ignore_ascii_case("localhost")) | ||
| { | ||
| // reqwest uses the URL's port; zero is only a DNS-override placeholder. | ||
| let loopback = [ | ||
| SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 0), | ||
| SocketAddr::new(IpAddr::V6(Ipv6Addr::LOCALHOST), 0), | ||
| ]; | ||
| builder = builder.resolve_to_addrs("localhost", &loopback); | ||
| } | ||
| } |
There was a problem hiding this comment.
If system resolution maps localhost to a non-loopback address, a secret-bearing development HTTP request accepted as loopback traffic now connects to that address because this branch only disables proxies. This sends plaintext secrets outside the loopback boundary.
How this was verified: The secret-transport check accepts the lexical localhost hostname, while the changed direct-loopback branch no longer pins it to 127.0.0.1 and ::1.
Knowledge Base Used: HTTP Server: Async Bridge to the Sync Interpreter
| let socket_addrs = format!("{}:{}", host, port); | ||
|
|
||
| // Try to resolve DNS | ||
| let addrs: Vec<IpAddr> = match socket_addrs.to_socket_addrs() { | ||
| Ok(iter) => iter.map(|s| s.ip()).collect(), | ||
| Err(_) => { | ||
| // DNS resolution failed - might be a blocked internal hostname | ||
| // In production, block; in dev with allow_localhost, permit | ||
| if config.allow_localhost { | ||
| return Ok(()); | ||
| } | ||
| return Err(format!("Could not resolve hostname: {}", host)); | ||
| } | ||
| // Resolve once, validate every address, and return this exact set to the | ||
| // client builder so connection-time DNS cannot rebind the target. | ||
| let addrs: Vec<SocketAddr> = match socket_addrs.to_socket_addrs() { |
There was a problem hiding this comment.
When an SSRF-protected URL contains an IPv6 literal such as http://[::1]:8080, host_str() supplies the unbracketed host and this concatenation produces an ambiguous value such as ::1:8080. to_socket_addrs therefore rejects a valid target and the request fails with Could not resolve hostname.
Knowledge Base Used: HTTP Server: Async Bridge to the Sync Interpreter
1 participant
Summary
Verification