From 2dbde94aa9e645715d14fff0d8d00143e236019b Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Tue, 28 Apr 2026 15:50:00 -0700 Subject: [PATCH] fix(network-proxy): normalize network proxy host matching (#19995) ## Why The proxy matches allow and deny rules against normalized host strings. Scoped IPv6 literals can arrive in equivalent forms, such as `fd00::1%eth0`, `[fd00::1%eth0]`, or `[fd00::1%25eth0]`. Policy should canonicalize those spellings without erasing scope granularity: an unscoped rule like `fd00::1` should still cover scoped requests for that address, while a scoped rule like `fd00::1%eth0` should remain exact to that scope. ## What changed - preserve IPv6 scope IDs during host normalization and canonicalize `%25scope` to `%scope` - match policy against the exact normalized host plus the unscoped IP base for scoped literals - keep local-address explicit allow checks aligned with the same scoped/unscoped semantics - add focused coverage for scoped IPv6 normalization, scoped allow rules, and scoped deny rules in `network-proxy` ## Security impact A request cannot bypass a broad deny rule by adding an IPv6 scope suffix. At the same time, scoped policy remains precise: `deny=fd00::1%eth0` affects that scoped spelling without collapsing `fd00::1%eth1` onto the same key, and `allow=fe80::1%eth0` does not implicitly allow other scopes. ## Verification - `just fmt` - `cargo test -p codex-network-proxy` - `just fix -p codex-network-proxy` - `git diff --check` --------- Co-authored-by: Codex Co-authored-by: evawong-oai --- codex-rs/network-proxy/src/http_proxy.rs | 5 +- codex-rs/network-proxy/src/policy.rs | 52 +++++++++++++-- codex-rs/network-proxy/src/proxy.rs | 21 +++--- codex-rs/network-proxy/src/runtime.rs | 83 +++++++++++++++++++++--- 4 files changed, 136 insertions(+), 25 deletions(-) diff --git a/codex-rs/network-proxy/src/http_proxy.rs b/codex-rs/network-proxy/src/http_proxy.rs index fa3616844..658ee9f61 100644 --- a/codex-rs/network-proxy/src/http_proxy.rs +++ b/codex-rs/network-proxy/src/http_proxy.rs @@ -1034,7 +1034,10 @@ mod tests { #[tokio::test] async fn http_connect_accept_allows_allowlisted_host_in_full_mode() { let policy = { - let mut policy = NetworkProxySettings::default(); + let mut policy = NetworkProxySettings { + allow_local_binding: true, + ..NetworkProxySettings::default() + }; policy.set_allowed_domains(vec!["example.com".to_string()]); policy }; diff --git a/codex-rs/network-proxy/src/policy.rs b/codex-rs/network-proxy/src/policy.rs index 40e71caf3..f83863ded 100644 --- a/codex-rs/network-proxy/src/policy.rs +++ b/codex-rs/network-proxy/src/policy.rs @@ -32,7 +32,7 @@ impl Host { /// Returns true if the host is a loopback hostname or IP literal. pub fn is_loopback_host(host: &Host) -> bool { let host = host.as_str(); - let host = host.split_once('%').map(|(ip, _)| ip).unwrap_or(host); + let host = unscoped_ip_literal(host).unwrap_or(host); if host == "localhost" { return true; } @@ -103,24 +103,48 @@ pub fn normalize_host(host: &str) -> String { if host.starts_with('[') && let Some(end) = host.find(']') { - return normalize_dns_host(&host[1..end]); + return normalize_dns_host_or_ip_literal(&host[1..end]); } // The proxy stack should typically hand us a host without a port, but be // defensive and strip `:port` when there is exactly one `:`. if host.bytes().filter(|b| *b == b':').count() == 1 { let host = host.split(':').next().unwrap_or_default(); - return normalize_dns_host(host); + return normalize_dns_host_or_ip_literal(host); } // Avoid mangling unbracketed IPv6 literals, but strip trailing dots so fully qualified domain // names are treated the same as their dotless variants. - normalize_dns_host(host) + normalize_dns_host_or_ip_literal(host) } -fn normalize_dns_host(host: &str) -> String { +fn normalize_dns_host_or_ip_literal(host: &str) -> String { let host = host.to_ascii_lowercase(); - host.trim_end_matches('.').to_string() + let host = host.trim_end_matches('.'); + if let Some(ip) = normalize_ip_literal(host) { + return ip; + } + host.to_string() +} + +pub(crate) fn unscoped_ip_literal(host: &str) -> Option<&str> { + let (ip, _) = host.split_once('%')?; + ip.parse::().ok()?; + Some(ip) +} + +fn normalize_ip_literal(host: &str) -> Option { + if host.parse::().is_ok() { + return Some(host.to_string()); + } + for delimiter in ["%25", "%"] { + if let Some((ip, scope)) = host.split_once(delimiter) + && ip.parse::().is_ok() + { + return Some(format!("{ip}%{scope}")); + } + } + None } fn normalize_pattern(pattern: &str) -> String { @@ -392,6 +416,15 @@ mod tests { assert_eq!(true, set.is_match("::1")); } + #[test] + fn compile_globset_preserves_scoped_ipv6_literals() { + let set = compile_denylist_globset(&["[fe80::1%25lo0]".to_string()]).unwrap(); + + assert_eq!(true, set.is_match("fe80::1%lo0")); + assert_eq!(false, set.is_match("fe80::1%lo1")); + assert_eq!(false, set.is_match("fe80::1")); + } + #[test] fn is_loopback_host_handles_localhost_variants() { assert!(is_loopback_host(&Host::parse("localhost").unwrap())); @@ -462,4 +495,11 @@ mod tests { assert_eq!(normalize_host("[::1]"), "::1"); assert_eq!(normalize_host("[::1]:443"), "::1"); } + + #[test] + fn normalize_host_preserves_ipv6_scope_ids() { + assert_eq!(normalize_host("fe80::1%lo0"), "fe80::1%lo0"); + assert_eq!(normalize_host("[fe80::1%lo0]"), "fe80::1%lo0"); + assert_eq!(normalize_host("[fe80::1%25lo0]"), "fe80::1%lo0"); + } } diff --git a/codex-rs/network-proxy/src/proxy.rs b/codex-rs/network-proxy/src/proxy.rs index 2d70ae3e1..2b76b27b8 100644 --- a/codex-rs/network-proxy/src/proxy.rs +++ b/codex-rs/network-proxy/src/proxy.rs @@ -790,9 +790,16 @@ mod tests { #[tokio::test] async fn managed_proxy_builder_uses_loopback_ports() { + let http_listener = StdTcpListener::bind(SocketAddr::from(([127, 0, 0, 1], 0))).unwrap(); + let http_addr = http_listener.local_addr().unwrap(); + let socks_listener = StdTcpListener::bind(SocketAddr::from(([127, 0, 0, 1], 0))).unwrap(); + let socks_addr = socks_listener.local_addr().unwrap(); + drop(http_listener); + drop(socks_listener); + let state = Arc::new(network_proxy_state_for_policy(NetworkProxySettings { - proxy_url: "http://127.0.0.1:43128".to_string(), - socks_url: "http://127.0.0.1:48081".to_string(), + proxy_url: format!("http://{http_addr}"), + socks_url: format!("http://{socks_addr}"), ..NetworkProxySettings::default() })); let proxy = match NetworkProxy::builder().state(state).build().await { @@ -812,14 +819,8 @@ mod tests { assert!(proxy.socks_addr.ip().is_loopback()); #[cfg(target_os = "windows")] { - assert_eq!( - proxy.http_addr, - "127.0.0.1:43128".parse::().unwrap() - ); - assert_eq!( - proxy.socks_addr, - "127.0.0.1:48081".parse::().unwrap() - ); + assert_eq!(proxy.http_addr, http_addr); + assert_eq!(proxy.socks_addr, socks_addr); } #[cfg(not(target_os = "windows"))] { diff --git a/codex-rs/network-proxy/src/runtime.rs b/codex-rs/network-proxy/src/runtime.rs index bddf81441..984236d89 100644 --- a/codex-rs/network-proxy/src/runtime.rs +++ b/codex-rs/network-proxy/src/runtime.rs @@ -7,6 +7,7 @@ use crate::policy::Host; use crate::policy::is_loopback_host; use crate::policy::is_non_public_ip; use crate::policy::normalize_host; +use crate::policy::unscoped_ip_literal; use crate::reasons::REASON_DENIED; use crate::reasons::REASON_NOT_ALLOWED; use crate::reasons::REASON_NOT_ALLOWED_LOCAL; @@ -371,11 +372,11 @@ impl NetworkProxyState { // 1) explicit deny always wins // 2) local/private networking is opt-in (defense-in-depth) // 3) allowlist is enforced when configured - if deny_set.is_match(host_str) { + if globset_matches_host_or_unscoped(&deny_set, host_str) { return Ok(HostBlockDecision::Blocked(HostBlockReason::Denied)); } - let is_allowlisted = allow_set.is_match(host_str); + let is_allowlisted = globset_matches_host_or_unscoped(&allow_set, host_str); if !allow_local_binding { // If the intent is "prevent access to local/internal networks", we must not rely solely // on string checks like `localhost` / `127.0.0.1`. Attackers can use DNS rebinding or @@ -386,10 +387,7 @@ impl NetworkProxyState { // allowlisted; hostnames that resolve to local/private IPs are blocked even if // allowlisted. let local_literal = { - let host_no_scope = host_str - .split_once('%') - .map(|(ip, _)| ip) - .unwrap_or(host_str); + let host_no_scope = unscoped_ip_literal(host_str).unwrap_or(host_str); if is_loopback_host(&host) { true } else if let Ok(ip) = host_no_scope.parse::() { @@ -797,8 +795,13 @@ fn log_domain_list_changes(list_name: &str, previous: &[String], next: &[String] } } +fn globset_matches_host_or_unscoped(set: &GlobSet, host: &str) -> bool { + set.is_match(host) || unscoped_ip_literal(host).is_some_and(|ip| set.is_match(ip)) +} + fn is_explicit_local_allowlisted(allowed_domains: &[String], host: &Host) -> bool { let normalized_host = host.as_str(); + let unscoped_host = unscoped_ip_literal(normalized_host); allowed_domains.iter().any(|pattern| { let pattern = pattern.trim(); if pattern == "*" || pattern.starts_with("*.") || pattern.starts_with("**.") { @@ -807,7 +810,9 @@ fn is_explicit_local_allowlisted(allowed_domains: &[String], host: &Host) -> boo if pattern.contains('*') || pattern.contains('?') { return false; } - normalize_host(pattern) == normalized_host + let normalized_pattern = normalize_host(pattern); + normalized_pattern == normalized_host + || unscoped_host.is_some_and(|ip| normalized_pattern == ip) }) } @@ -1247,7 +1252,7 @@ mod tests { #[tokio::test] async fn host_blocked_allows_scoped_ipv6_literal_when_explicitly_allowlisted() { - let state = network_proxy_state_for_policy(network_settings(&["fe80::1%lo0"], &[])); + let state = network_proxy_state_for_policy(network_settings(&["fe80::1"], &[])); assert_eq!( state @@ -1258,6 +1263,68 @@ mod tests { ); } + #[tokio::test] + async fn host_blocked_requires_exact_scoped_ipv6_allowlist_match() { + let state = network_proxy_state_for_policy(NetworkProxySettings { + allow_local_binding: true, + ..network_settings(&["fe80::1%eth0"], &[]) + }); + + assert_eq!( + state + .host_blocked("fe80::1%eth0", /*port*/ 80) + .await + .unwrap(), + HostBlockDecision::Allowed + ); + assert_eq!( + state + .host_blocked("fe80::1%eth1", /*port*/ 80) + .await + .unwrap(), + HostBlockDecision::Blocked(HostBlockReason::NotAllowed) + ); + } + + #[tokio::test] + async fn host_blocked_denies_scoped_ipv6_literal_before_local_binding() { + let state = network_proxy_state_for_policy(NetworkProxySettings { + allow_local_binding: true, + ..network_settings(&["*"], &["fd00::1"]) + }); + + for host in ["fd00::1%eth0", "[fd00::1%eth0]", "[fd00::1%25eth0]"] { + assert_eq!( + state.host_blocked(host, /*port*/ 80).await.unwrap(), + HostBlockDecision::Blocked(HostBlockReason::Denied), + "host should be denied after normalization: {host}" + ); + } + } + + #[tokio::test] + async fn host_blocked_requires_exact_scoped_ipv6_denylist_match() { + let state = network_proxy_state_for_policy(NetworkProxySettings { + allow_local_binding: true, + ..network_settings(&["*"], &["fd00::1%eth0"]) + }); + + assert_eq!( + state + .host_blocked("fd00::1%eth0", /*port*/ 80) + .await + .unwrap(), + HostBlockDecision::Blocked(HostBlockReason::Denied) + ); + assert_eq!( + state + .host_blocked("fd00::1%eth1", /*port*/ 80) + .await + .unwrap(), + HostBlockDecision::Allowed + ); + } + #[tokio::test] async fn host_blocked_rejects_private_ip_literals_when_local_binding_disabled() { let state = network_proxy_state_for_policy(network_settings(&["example.com"], &[]));