diff --git a/codex-rs/http-client/src/outbound_proxy.rs b/codex-rs/http-client/src/outbound_proxy.rs index 692da254fc..f91cebd3cf 100644 --- a/codex-rs/http-client/src/outbound_proxy.rs +++ b/codex-rs/http-client/src/outbound_proxy.rs @@ -12,6 +12,8 @@ use std::sync::Mutex; use std::sync::OnceLock; use std::time::Duration; use std::time::Instant; +#[cfg(any(target_os = "windows", target_os = "macos"))] +use tokio::sync::Semaphore; use crate::custom_ca::BuildCustomCaTransportError; use crate::custom_ca::build_reqwest_client_with_custom_ca; @@ -25,6 +27,8 @@ use thiserror::Error; const SYSTEM_PROXY_SUCCESS_CACHE_TTL: Duration = Duration::from_secs(60); const SYSTEM_PROXY_UNAVAILABLE_CACHE_TTL: Duration = Duration::from_secs(5); const SYSTEM_PROXY_CACHE_MAX_ENTRIES: usize = 256; +#[cfg(any(target_os = "windows", target_os = "macos"))] +static ASYNC_SYSTEM_PROXY_RESOLUTION_PERMIT: Semaphore = Semaphore::const_new(1); #[cfg(target_os = "macos")] mod macos; @@ -100,16 +104,22 @@ pub enum OutboundProxyPolicy { /// Resolved proxy route for a concrete outbound destination. /// -/// `TransportDefault` delegates environment-proxy handling to the underlying transport. Proxy -/// URLs are intentionally redacted from `Debug` output because they may contain credentials. -#[derive(Clone, PartialEq, Eq)] +/// `TransportDefault` preserves the underlying transport behavior only when system-proxy support +/// is disabled. When system resolution is enabled, environment and direct fallbacks are resolved +/// explicitly so the transport cannot repeat system discovery. Proxy URLs and no-proxy settings +/// are intentionally redacted from `Debug` output because they may contain credentials or private +/// hostnames. +#[derive(Clone, Hash, PartialEq, Eq)] pub enum OutboundProxyRoute { /// Preserve the underlying transport's existing proxy behavior. TransportDefault, /// Connect directly and bypass transport-level proxy discovery. Direct, /// Connect through the selected proxy URL. - Proxy { url: String }, + Proxy { + url: String, + no_proxy: Option, + }, } impl fmt::Debug for OutboundProxyRoute { @@ -117,7 +127,11 @@ impl fmt::Debug for OutboundProxyRoute { match self { Self::TransportDefault => f.write_str("TransportDefault"), Self::Direct => f.write_str("Direct"), - Self::Proxy { .. } => f.debug_struct("Proxy").field("url", &"").finish(), + Self::Proxy { .. } => f + .debug_struct("Proxy") + .field("url", &"") + .field("no_proxy", &"") + .finish(), } } } @@ -149,16 +163,63 @@ impl HttpClientFactory { /// /// WebSocket schemes are resolved through their HTTP equivalents so platform PAC and system /// proxy APIs apply the same policy to `ws`/`wss` and `http`/`https` destinations. When system - /// resolution is unavailable, the transport retains responsibility for environment-proxy - /// fallback. + /// resolution is unavailable, explicit environment settings are resolved before falling back + /// to a direct route. pub fn resolve_proxy_route(&self, request_url: &str) -> OutboundProxyRoute { resolve_proxy_route( + &ProcessEnv, request_url, self.outbound_proxy_policy, resolve_system_proxy, ) } + /// Resolves the proxy route for a concrete destination without blocking a Tokio worker. + pub async fn resolve_proxy_route_async( + &self, + request_url: String, + ) -> io::Result { + if matches!( + self.outbound_proxy_policy, + OutboundProxyPolicy::ReqwestDefault + ) { + return Ok(OutboundProxyRoute::TransportDefault); + } + + if let Some(route) = self.cached_proxy_route(&request_url) { + return Ok(route); + } + + #[cfg(not(any(target_os = "windows", target_os = "macos")))] + return Ok(self.resolve_proxy_route(&request_url)); + + #[cfg(any(target_os = "windows", target_os = "macos"))] + { + let permit = ASYNC_SYSTEM_PROXY_RESOLUTION_PERMIT + .acquire() + .await + .map_err(io::Error::other)?; + let factory = self.clone(); + tokio::task::spawn_blocking(move || { + // Keep the permit with the blocking task: cancelling the caller must not allow a + // second PAC/WinHTTP lookup to start while this one is still running. + let _permit = permit; + factory.resolve_proxy_route(&request_url) + }) + .await + .map_err(io::Error::other) + } + } + + fn cached_proxy_route(&self, request_url: &str) -> Option { + let request_url = proxy_resolution_url(request_url); + let Some(origin) = RequestOrigin::parse(&request_url) else { + return Some(OutboundProxyRoute::Direct); + }; + cached_system_proxy_decision(&request_url) + .map(|decision| route_from_system_decision(&ProcessEnv, &origin, decision)) + } + /// Builds an HTTP client for a concrete outbound route. pub fn build_client( &self, @@ -193,9 +254,11 @@ impl HttpClientFactory { self.outbound_proxy_policy, ) } + } fn resolve_proxy_route( + env: &dyn EnvSource, request_url: &str, outbound_proxy_policy: OutboundProxyPolicy, resolve_system_proxy: impl FnOnce(&str, &RequestOrigin) -> SystemProxyDecision, @@ -206,13 +269,41 @@ fn resolve_proxy_route( let request_url = proxy_resolution_url(request_url); let Some(origin) = RequestOrigin::parse(&request_url) else { - return OutboundProxyRoute::TransportDefault; + return OutboundProxyRoute::Direct; }; - match resolve_system_proxy(&request_url, &origin) { + route_from_system_decision(env, &origin, resolve_system_proxy(&request_url, &origin)) +} + +fn route_from_system_decision( + env: &dyn EnvSource, + origin: &RequestOrigin, + decision: SystemProxyDecision, +) -> OutboundProxyRoute { + match decision { SystemProxyDecision::Direct => OutboundProxyRoute::Direct, - SystemProxyDecision::Proxy { url } => OutboundProxyRoute::Proxy { url }, - SystemProxyDecision::Unavailable { .. } => OutboundProxyRoute::TransportDefault, + SystemProxyDecision::Proxy { url } => OutboundProxyRoute::Proxy { + url, + no_proxy: None, + }, + SystemProxyDecision::Unavailable { .. } => resolve_env_proxy_route(env, origin), + } +} + +fn resolve_env_proxy_route(env: &dyn EnvSource, origin: &RequestOrigin) -> OutboundProxyRoute { + let proxy_url = match origin.scheme.as_str() { + "https" => { + proxy_env_value(env, "HTTPS_PROXY").or_else(|| proxy_env_value(env, "ALL_PROXY")) + } + "http" => proxy_env_value(env, "HTTP_PROXY").or_else(|| proxy_env_value(env, "ALL_PROXY")), + _ => proxy_env_value(env, "ALL_PROXY"), + }; + match proxy_url { + Some(url) => OutboundProxyRoute::Proxy { + url, + no_proxy: proxy_env_value(env, "NO_PROXY"), + }, + None => OutboundProxyRoute::Direct, } } @@ -276,22 +367,26 @@ fn configure_proxy_for_route( outbound_proxy_policy: OutboundProxyPolicy, resolve_system_proxy: impl FnOnce(&str, &RequestOrigin) -> SystemProxyDecision, ) -> Result { - if matches!(outbound_proxy_policy, OutboundProxyPolicy::ReqwestDefault) { - return Ok(builder); - } - let origin = RequestOrigin::parse(request_url); + let route = resolve_proxy_route( + env, + request_url, + outbound_proxy_policy, + resolve_system_proxy, + ); + configure_builder_for_resolved_route(builder, route_class, &route) +} - let Some(origin) = origin.as_ref() else { - return configure_env_proxy_handling(env, builder, /*origin*/ None, route_class); - }; - - match resolve_system_proxy(request_url, origin) { - SystemProxyDecision::Direct => Ok(builder.no_proxy()), - SystemProxyDecision::Proxy { url } => { - configure_concrete_proxy(builder, route_class, &url, /*no_proxy*/ None) - } - SystemProxyDecision::Unavailable { .. } => { - configure_env_proxy_handling(env, builder, Some(origin), route_class) +fn configure_builder_for_resolved_route( + builder: reqwest::ClientBuilder, + route_class: ClientRouteClass, + route: &OutboundProxyRoute, +) -> Result { + match route { + OutboundProxyRoute::TransportDefault => Ok(builder), + OutboundProxyRoute::Direct => Ok(builder.no_proxy()), + OutboundProxyRoute::Proxy { url, no_proxy } => { + let no_proxy = no_proxy.as_deref().and_then(reqwest::NoProxy::from_string); + configure_concrete_proxy(builder, route_class, url, no_proxy) } } } @@ -311,31 +406,6 @@ fn configure_concrete_proxy( Ok(builder.proxy(proxy.no_proxy(no_proxy))) } -fn configure_env_proxy_handling( - env: &dyn EnvSource, - builder: reqwest::ClientBuilder, - origin: Option<&RequestOrigin>, - route_class: ClientRouteClass, -) -> Result { - if let Some(origin) = origin { - let proxy_url = match origin.scheme.as_str() { - "https" => { - proxy_env_value(env, "HTTPS_PROXY").or_else(|| proxy_env_value(env, "ALL_PROXY")) - } - "http" => { - proxy_env_value(env, "HTTP_PROXY").or_else(|| proxy_env_value(env, "ALL_PROXY")) - } - _ => proxy_env_value(env, "ALL_PROXY"), - }; - if let Some(proxy_url) = proxy_url { - let no_proxy = proxy_env_value(env, "NO_PROXY") - .and_then(|value| reqwest::NoProxy::from_string(&value)); - return configure_concrete_proxy(builder, route_class, &proxy_url, no_proxy); - } - } - Ok(builder.no_proxy()) -} - #[derive(Debug, Clone, PartialEq, Eq)] #[allow(dead_code)] struct RequestOrigin { @@ -431,7 +501,6 @@ struct CachedSystemProxyDecision { static SYSTEM_PROXY_CACHE: OnceLock>> = OnceLock::new(); -#[cfg(test)] fn cached_system_proxy_decision(request_url: &str) -> Option { let cache = SYSTEM_PROXY_CACHE.get_or_init(|| Mutex::new(HashMap::new())); let mut cache = cache.lock().ok()?; diff --git a/codex-rs/http-client/src/outbound_proxy_tests.rs b/codex-rs/http-client/src/outbound_proxy_tests.rs index 387973eec4..bec3999416 100644 --- a/codex-rs/http-client/src/outbound_proxy_tests.rs +++ b/codex-rs/http-client/src/outbound_proxy_tests.rs @@ -12,7 +12,11 @@ struct MapEnv { #[test] fn websocket_route_uses_http_equivalent_for_system_resolution() { + let env = MapEnv { + values: HashMap::new(), + }; let route = resolve_proxy_route( + &env, "wss://api.openai.com/v1/responses", OutboundProxyPolicy::RespectSystemProxy, |request_url, origin| { @@ -30,13 +34,18 @@ fn websocket_route_uses_http_equivalent_for_system_resolution() { route, OutboundProxyRoute::Proxy { url: "http://proxy.example:8080".to_string(), + no_proxy: None, } ); } #[test] fn reqwest_default_route_preserves_transport_proxy_behavior() { + let env = MapEnv { + values: HashMap::new(), + }; let route = resolve_proxy_route( + &env, "wss://api.openai.com/v1/responses", OutboundProxyPolicy::ReqwestDefault, |_, _| panic!("default policy should not resolve system proxy settings"), @@ -80,11 +89,11 @@ fn environment_fallback_reads_injected_proxy_environment() { values: HashMap::from([("HTTPS_PROXY".to_string(), "://invalid".to_string())]), }; let origin = RequestOrigin::parse("https://auth.openai.com/oauth/token").expect("valid URL"); - let result = configure_env_proxy_handling( - &env, + let route = resolve_env_proxy_route(&env, &origin); + let result = configure_builder_for_resolved_route( reqwest::Client::builder(), - Some(&origin), ClientRouteClass::Auth, + &route, ); assert!(matches!( @@ -95,6 +104,69 @@ fn environment_fallback_reads_injected_proxy_environment() { )); } +#[test] +fn unavailable_system_route_resolves_environment_or_direct_explicitly() { + let origin = RequestOrigin::parse("https://backend.example/tasks").expect("valid URL"); + let env = MapEnv { + values: HashMap::from([ + ( + "HTTPS_PROXY".to_string(), + "http://proxy.example:8080".to_string(), + ), + ("NO_PROXY".to_string(), "localhost,.internal".to_string()), + ]), + }; + + assert_eq!( + route_from_system_decision( + &env, + &origin, + SystemProxyDecision::Unavailable { + failure: RouteFailureClass::ProxyResolutionUnavailable, + }, + ), + OutboundProxyRoute::Proxy { + url: "http://proxy.example:8080".to_string(), + no_proxy: Some("localhost,.internal".to_string()), + } + ); + assert_eq!( + route_from_system_decision( + &MapEnv { + values: HashMap::new(), + }, + &origin, + SystemProxyDecision::Unavailable { + failure: RouteFailureClass::ProxyResolutionUnavailable, + }, + ), + OutboundProxyRoute::Direct + ); +} + +#[cfg(any(target_os = "windows", target_os = "macos"))] +#[tokio::test] +async fn async_resolution_uses_cached_route_before_global_permit() { + let request_url = "https://cached-fast-path.test/request"; + cache_system_proxy_decision(request_url, SystemProxyDecision::Direct); + let factory = HttpClientFactory::new(OutboundProxyPolicy::RespectSystemProxy); + let permit = ASYNC_SYSTEM_PROXY_RESOLUTION_PERMIT + .acquire() + .await + .expect("global proxy permit should stay open"); + + let route = tokio::time::timeout( + Duration::from_millis(100), + factory.resolve_proxy_route_async(request_url.to_string()), + ) + .await + .expect("cached resolution should not wait for the global permit") + .expect("cached route should resolve"); + drop(permit); + + assert_eq!(route, OutboundProxyRoute::Direct); +} + #[tokio::test] async fn enabled_environment_proxy_routes_request_through_proxy() { let listener = diff --git a/codex-rs/websocket-client/src/dialer.rs b/codex-rs/websocket-client/src/dialer.rs index 29d4f42884..b71c253800 100644 --- a/codex-rs/websocket-client/src/dialer.rs +++ b/codex-rs/websocket-client/src/dialer.rs @@ -38,9 +38,14 @@ pub(crate) async fn connect( proxy_route: OutboundProxyRoute, ) -> Result<(ConnectionInner, Response), WebSocketError> { let stream: Box = match proxy_route { - OutboundProxyRoute::TransportDefault => { + OutboundProxyRoute::TransportDefault + | OutboundProxyRoute::Proxy { + no_proxy: Some(_), .. + } => { // The workspace enables tokio-tungstenite's `proxy` feature, so its default dialer // resolves HTTP_PROXY, HTTPS_PROXY, ALL_PROXY, and NO_PROXY before opening the socket. + // Environment routes retain this path when NO_PROXY is present so Tungstenite applies + // its complete bypass semantics rather than duplicating them in this crate. let (stream, response) = connect_async_tls_with_config( request, Some(config), @@ -59,7 +64,10 @@ pub(crate) async fn connect( .map_err(WebSocketError::Io)?, ) } - OutboundProxyRoute::Proxy { url } => { + OutboundProxyRoute::Proxy { + url, + no_proxy: None, + } => { let proxy = ProxyEndpoint::parse(&url)?; let host = websocket_host(&request)?; let port = websocket_port(&request)?; diff --git a/codex-rs/websocket-client/src/dialer_tests.rs b/codex-rs/websocket-client/src/dialer_tests.rs index 4d7ae7731d..5dae9d16f7 100644 --- a/codex-rs/websocket-client/src/dialer_tests.rs +++ b/codex-rs/websocket-client/src/dialer_tests.rs @@ -232,6 +232,7 @@ async fn assert_proxy_tunnels_secure_websocket(proxy_tls: bool) { tls_config, OutboundProxyRoute::Proxy { url: format!("{proxy_scheme}://localhost:{}", proxy_addr.port()), + no_proxy: None, }, ) .await