mirror of
https://github.com/openai/codex.git
synced 2026-05-05 05:42:33 +03:00
Enable SOCKS defaults for common local network proxy use cases (#11362)
## Summary - enable local-use defaults in network proxy settings: SOCKS5 on, SOCKS5 UDP on, upstream proxying on, and local binding on - add a regression test that asserts the full `NetworkProxySettings::default()` baseline - Fixed managed listener reservation behavior. Before: we always reserved a loopback SOCKS listener, even when enable_socks5 = false. Now: SOCKS listener is only reserved when SOCKS is enabled. - Fixed /debug-config env output for SOCKS-disabled sessions. ALL_PROXY now shows the HTTP proxy URL when SOCKS is disabled (instead of incorrectly showing socks5h://...). ## Validation - just fmt - cargo test -p codex-network-proxy - cargo clippy -p codex-network-proxy --all-targets
This commit is contained in:
@@ -15,6 +15,7 @@ pub struct NetworkProxyConfig {
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
|
||||
#[serde(default)]
|
||||
pub struct NetworkProxySettings {
|
||||
#[serde(default)]
|
||||
pub enabled: bool,
|
||||
@@ -22,13 +23,10 @@ pub struct NetworkProxySettings {
|
||||
pub proxy_url: String,
|
||||
#[serde(default = "default_admin_url")]
|
||||
pub admin_url: String,
|
||||
#[serde(default)]
|
||||
pub enable_socks5: bool,
|
||||
#[serde(default = "default_socks_url")]
|
||||
pub socks_url: String,
|
||||
#[serde(default)]
|
||||
pub enable_socks5_udp: bool,
|
||||
#[serde(default)]
|
||||
pub allow_upstream_proxy: bool,
|
||||
#[serde(default)]
|
||||
pub dangerously_allow_non_loopback_proxy: bool,
|
||||
@@ -42,7 +40,6 @@ pub struct NetworkProxySettings {
|
||||
pub denied_domains: Vec<String>,
|
||||
#[serde(default)]
|
||||
pub allow_unix_sockets: Vec<String>,
|
||||
#[serde(default)]
|
||||
pub allow_local_binding: bool,
|
||||
}
|
||||
|
||||
@@ -52,17 +49,17 @@ impl Default for NetworkProxySettings {
|
||||
enabled: false,
|
||||
proxy_url: default_proxy_url(),
|
||||
admin_url: default_admin_url(),
|
||||
enable_socks5: false,
|
||||
enable_socks5: true,
|
||||
socks_url: default_socks_url(),
|
||||
enable_socks5_udp: false,
|
||||
allow_upstream_proxy: false,
|
||||
enable_socks5_udp: true,
|
||||
allow_upstream_proxy: true,
|
||||
dangerously_allow_non_loopback_proxy: false,
|
||||
dangerously_allow_non_loopback_admin: false,
|
||||
mode: NetworkMode::default(),
|
||||
allowed_domains: Vec::new(),
|
||||
denied_domains: Vec::new(),
|
||||
allow_unix_sockets: Vec::new(),
|
||||
allow_local_binding: false,
|
||||
allow_local_binding: true,
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -329,6 +326,47 @@ mod tests {
|
||||
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[test]
|
||||
fn network_proxy_settings_default_matches_local_use_baseline() {
|
||||
assert_eq!(
|
||||
NetworkProxySettings::default(),
|
||||
NetworkProxySettings {
|
||||
enabled: false,
|
||||
proxy_url: "http://127.0.0.1:3128".to_string(),
|
||||
admin_url: "http://127.0.0.1:8080".to_string(),
|
||||
enable_socks5: true,
|
||||
socks_url: "http://127.0.0.1:8081".to_string(),
|
||||
enable_socks5_udp: true,
|
||||
allow_upstream_proxy: true,
|
||||
dangerously_allow_non_loopback_proxy: false,
|
||||
dangerously_allow_non_loopback_admin: false,
|
||||
mode: NetworkMode::Full,
|
||||
allowed_domains: Vec::new(),
|
||||
denied_domains: Vec::new(),
|
||||
allow_unix_sockets: Vec::new(),
|
||||
allow_local_binding: true,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn partial_network_config_uses_struct_defaults_for_missing_fields() {
|
||||
let config: NetworkProxyConfig = serde_json::from_str(
|
||||
r#"{
|
||||
"network": {
|
||||
"enabled": true
|
||||
}
|
||||
}"#,
|
||||
)
|
||||
.unwrap();
|
||||
let expected = NetworkProxySettings {
|
||||
enabled: true,
|
||||
..NetworkProxySettings::default()
|
||||
};
|
||||
|
||||
assert_eq!(config.network, expected);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parse_host_port_defaults_for_empty_string() {
|
||||
assert!(parse_host_port("", 1234).is_err());
|
||||
|
||||
@@ -29,10 +29,10 @@ struct ReservedListeners {
|
||||
}
|
||||
|
||||
impl ReservedListeners {
|
||||
fn new(http: StdTcpListener, socks: StdTcpListener, admin: StdTcpListener) -> Self {
|
||||
fn new(http: StdTcpListener, socks: Option<StdTcpListener>, admin: StdTcpListener) -> Self {
|
||||
Self {
|
||||
http: Mutex::new(Some(http)),
|
||||
socks: Mutex::new(Some(socks)),
|
||||
socks: Mutex::new(socks),
|
||||
admin: Mutex::new(Some(admin)),
|
||||
}
|
||||
}
|
||||
@@ -133,15 +133,20 @@ impl NetworkProxyBuilder {
|
||||
let current_cfg = state.current_cfg().await?;
|
||||
let (requested_http_addr, requested_socks_addr, requested_admin_addr, reserved_listeners) =
|
||||
if self.managed_by_codex {
|
||||
let runtime = config::resolve_runtime(¤t_cfg)?;
|
||||
let (http_listener, socks_listener, admin_listener) =
|
||||
reserve_loopback_ephemeral_listeners()
|
||||
reserve_loopback_ephemeral_listeners(current_cfg.network.enable_socks5)
|
||||
.context("reserve managed loopback proxy listeners")?;
|
||||
let http_addr = http_listener
|
||||
.local_addr()
|
||||
.context("failed to read reserved HTTP proxy address")?;
|
||||
let socks_addr = socks_listener
|
||||
.local_addr()
|
||||
.context("failed to read reserved SOCKS5 proxy address")?;
|
||||
let socks_addr = if let Some(socks_listener) = socks_listener.as_ref() {
|
||||
socks_listener
|
||||
.local_addr()
|
||||
.context("failed to read reserved SOCKS5 proxy address")?
|
||||
} else {
|
||||
runtime.socks_addr
|
||||
};
|
||||
let admin_addr = admin_listener
|
||||
.local_addr()
|
||||
.context("failed to read reserved admin API address")?;
|
||||
@@ -186,13 +191,19 @@ impl NetworkProxyBuilder {
|
||||
}
|
||||
}
|
||||
|
||||
fn reserve_loopback_ephemeral_listeners() -> Result<(StdTcpListener, StdTcpListener, StdTcpListener)>
|
||||
{
|
||||
Ok((
|
||||
reserve_loopback_ephemeral_listener().context("reserve HTTP proxy listener")?,
|
||||
reserve_loopback_ephemeral_listener().context("reserve SOCKS5 proxy listener")?,
|
||||
reserve_loopback_ephemeral_listener().context("reserve admin API listener")?,
|
||||
))
|
||||
fn reserve_loopback_ephemeral_listeners(
|
||||
reserve_socks_listener: bool,
|
||||
) -> Result<(StdTcpListener, Option<StdTcpListener>, StdTcpListener)> {
|
||||
let http_listener =
|
||||
reserve_loopback_ephemeral_listener().context("reserve HTTP proxy listener")?;
|
||||
let socks_listener = if reserve_socks_listener {
|
||||
Some(reserve_loopback_ephemeral_listener().context("reserve SOCKS5 proxy listener")?)
|
||||
} else {
|
||||
None
|
||||
};
|
||||
let admin_listener =
|
||||
reserve_loopback_ephemeral_listener().context("reserve admin API listener")?;
|
||||
Ok((http_listener, socks_listener, admin_listener))
|
||||
}
|
||||
|
||||
fn reserve_loopback_ephemeral_listener() -> Result<StdTcpListener> {
|
||||
@@ -612,6 +623,43 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn managed_proxy_builder_does_not_reserve_socks_listener_when_disabled() {
|
||||
let settings = NetworkProxySettings {
|
||||
enable_socks5: false,
|
||||
socks_url: "http://127.0.0.1:43129".to_string(),
|
||||
..NetworkProxySettings::default()
|
||||
};
|
||||
let state = Arc::new(network_proxy_state_for_policy(settings));
|
||||
let proxy = match NetworkProxy::builder().state(state).build().await {
|
||||
Ok(proxy) => proxy,
|
||||
Err(err) => {
|
||||
if err
|
||||
.chain()
|
||||
.any(|cause| cause.to_string().contains("Operation not permitted"))
|
||||
{
|
||||
return;
|
||||
}
|
||||
panic!("failed to build managed proxy: {err:#}");
|
||||
}
|
||||
};
|
||||
|
||||
assert!(proxy.http_addr.ip().is_loopback());
|
||||
assert!(proxy.admin_addr.ip().is_loopback());
|
||||
assert_eq!(
|
||||
proxy.socks_addr,
|
||||
"127.0.0.1:43129".parse::<SocketAddr>().unwrap()
|
||||
);
|
||||
assert!(
|
||||
proxy
|
||||
.reserved_listeners
|
||||
.as_ref()
|
||||
.expect("managed builder should reserve listeners")
|
||||
.take_socks()
|
||||
.is_none()
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn proxy_url_env_value_resolves_lowercase_aliases() {
|
||||
let mut env = HashMap::new();
|
||||
|
||||
Reference in New Issue
Block a user