Compare commits

...

1 Commits

Author SHA1 Message Date
viyatb-oai
aea82c63ea fix(network-proxy): fail closed on network-proxy DNS lookup errors (#15909)
## Summary

Fail closed when the network proxy's local/private IP pre-check hits a
DNS lookup error or timeout, instead of treating the hostname as public
and allowing the request.

## Root cause

`host_resolves_to_non_public_ip()` returned `false` on resolver failure,
which created a fail-open path in the `allow_local_binding = false`
boundary. The eventual connect path performs its own DNS resolution
later, so a transient pre-check failure is not evidence that the
destination is public.

## Changes

- Treat DNS lookup errors/timeouts as local/private for blocking
purposes
- Add a regression test for an allowlisted hostname that fails DNS
resolution

## Validation

- `cargo test -p codex-network-proxy`
- `cargo clippy -p codex-network-proxy --all-targets -- -D warnings`
- `just fmt`
- `just argument-comment-lint`
2026-03-26 23:18:04 +00:00

View File

@@ -698,12 +698,22 @@ async fn host_resolves_to_non_public_ip(host: &str, port: u16) -> bool {
return is_non_public_ip(ip);
}
// If DNS lookup fails, default to "not local/private" rather than blocking. In practice, the
// subsequent connect attempt will fail anyway, and blocking on transient resolver issues would
// make the proxy fragile. The allowlist/denylist remains the primary control plane.
// Block the request if this DNS lookup fails. We resolve the hostname again when we connect,
// so a failed check here does not prove the destination is public.
let addrs = match timeout(DNS_LOOKUP_TIMEOUT, lookup_host((host, port))).await {
Ok(Ok(addrs)) => addrs,
Ok(Err(_)) | Err(_) => return false,
Ok(Err(err)) => {
debug!(
"blocking host because DNS lookup failed during local/private IP check (host={host}, port={port}): {err}"
);
return true;
}
Err(_) => {
debug!(
"blocking host because DNS lookup timed out during local/private IP check (host={host}, port={port})"
);
return true;
}
};
for addr in addrs {
@@ -903,17 +913,18 @@ mod tests {
});
assert_eq!(
state.host_blocked("evil.example", 80).await.unwrap(),
// Use a public IP literal to avoid relying on ambient DNS behavior.
state.host_blocked("8.8.8.8", 80).await.unwrap(),
HostBlockDecision::Allowed
);
state.add_denied_domain("evil.example").await.unwrap();
state.add_denied_domain("8.8.8.8").await.unwrap();
let (allowed, denied) = state.current_patterns().await.unwrap();
assert_eq!(allowed, vec!["*".to_string()]);
assert_eq!(denied, vec!["evil.example".to_string()]);
assert_eq!(denied, vec!["8.8.8.8".to_string()]);
assert_eq!(
state.host_blocked("evil.example", 80).await.unwrap(),
state.host_blocked("8.8.8.8", 80).await.unwrap(),
HostBlockDecision::Blocked(HostBlockReason::Denied)
);
}
@@ -1237,6 +1248,23 @@ mod tests {
);
}
#[tokio::test]
async fn host_blocked_rejects_allowlisted_hostname_when_dns_lookup_fails() {
let state = network_proxy_state_for_policy(NetworkProxySettings {
allowed_domains: vec!["does-not-resolve.invalid".to_string()],
allow_local_binding: false,
..NetworkProxySettings::default()
});
assert_eq!(
state
.host_blocked("does-not-resolve.invalid", 80)
.await
.unwrap(),
HostBlockDecision::Blocked(HostBlockReason::NotAllowedLocal)
);
}
#[test]
fn validate_policy_against_constraints_disallows_widening_allowed_domains() {
let constraints = NetworkProxyConstraints {