From 700a29e15780c16cd54dc5c895ef4cafb65232c9 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 27 Jan 2026 11:15:38 -0800 Subject: [PATCH] chore: introduce *Args types for new() methods (#10009) Constructors with long param lists can be hard to reason about when a number of the args are `None`, in practice. Introducing a struct to use as the args type helps make things more self-documenting. --- codex-rs/network-proxy/src/http_proxy.rs | 110 ++++++++-------- codex-rs/network-proxy/src/lib.rs | 1 + codex-rs/network-proxy/src/network_policy.rs | 83 +++++++----- codex-rs/network-proxy/src/runtime.rs | 26 ++-- codex-rs/network-proxy/src/socks5.rs | 130 ++++++++++--------- codex-rs/network-proxy/src/state.rs | 1 + 6 files changed, 189 insertions(+), 162 deletions(-) diff --git a/codex-rs/network-proxy/src/http_proxy.rs b/codex-rs/network-proxy/src/http_proxy.rs index abe6b3014..b2856ac60 100644 --- a/codex-rs/network-proxy/src/http_proxy.rs +++ b/codex-rs/network-proxy/src/http_proxy.rs @@ -2,6 +2,7 @@ use crate::config::NetworkMode; use crate::network_policy::NetworkDecision; use crate::network_policy::NetworkPolicyDecider; use crate::network_policy::NetworkPolicyRequest; +use crate::network_policy::NetworkPolicyRequestArgs; use crate::network_policy::NetworkProtocol; use crate::network_policy::evaluate_host_policy; use crate::policy::normalize_host; @@ -12,6 +13,7 @@ use crate::responses::blocked_header_value; use crate::responses::json_response; use crate::runtime::unix_socket_permissions_supported; use crate::state::BlockedRequest; +use crate::state::BlockedRequestArgs; use crate::state::NetworkProxyState; use crate::upstream::UpstreamClient; use crate::upstream::proxy_for_connect; @@ -146,27 +148,27 @@ async fn http_connect_accept( .await); } - let request = NetworkPolicyRequest::new( - NetworkProtocol::HttpsConnect, - host.clone(), - authority.port, - client.clone(), - Some("CONNECT".to_string()), - None, - None, - ); + let request = NetworkPolicyRequest::new(NetworkPolicyRequestArgs { + protocol: NetworkProtocol::HttpsConnect, + host: host.clone(), + port: authority.port, + client_addr: client.clone(), + method: Some("CONNECT".to_string()), + command: None, + exec_policy_hint: None, + }); match evaluate_host_policy(&app_state, policy_decider.as_ref(), &request).await { Ok(NetworkDecision::Deny { reason }) => { let _ = app_state - .record_blocked(BlockedRequest::new( - host.clone(), - reason.clone(), - client.clone(), - Some("CONNECT".to_string()), - None, - "http-connect".to_string(), - )) + .record_blocked(BlockedRequest::new(BlockedRequestArgs { + host: host.clone(), + reason: reason.clone(), + client: client.clone(), + method: Some("CONNECT".to_string()), + mode: None, + protocol: "http-connect".to_string(), + })) .await; let client = client.as_deref().unwrap_or_default(); warn!("CONNECT blocked (client={client}, host={host}, reason={reason})"); @@ -189,14 +191,14 @@ async fn http_connect_accept( if mode == NetworkMode::Limited { let _ = app_state - .record_blocked(BlockedRequest::new( - host.clone(), - REASON_METHOD_NOT_ALLOWED.to_string(), - client.clone(), - Some("CONNECT".to_string()), - Some(NetworkMode::Limited), - "http-connect".to_string(), - )) + .record_blocked(BlockedRequest::new(BlockedRequestArgs { + host: host.clone(), + reason: REASON_METHOD_NOT_ALLOWED.to_string(), + client: client.clone(), + method: Some("CONNECT".to_string()), + mode: Some(NetworkMode::Limited), + protocol: "http-connect".to_string(), + })) .await; let client = client.as_deref().unwrap_or_default(); warn!("CONNECT blocked by method policy (client={client}, host={host}, mode=limited)"); @@ -425,27 +427,27 @@ async fn http_plain_proxy( .await); } - let request = NetworkPolicyRequest::new( - NetworkProtocol::Http, - host.clone(), + let request = NetworkPolicyRequest::new(NetworkPolicyRequestArgs { + protocol: NetworkProtocol::Http, + host: host.clone(), port, - client.clone(), - Some(req.method().as_str().to_string()), - None, - None, - ); + client_addr: client.clone(), + method: Some(req.method().as_str().to_string()), + command: None, + exec_policy_hint: None, + }); match evaluate_host_policy(&app_state, policy_decider.as_ref(), &request).await { Ok(NetworkDecision::Deny { reason }) => { let _ = app_state - .record_blocked(BlockedRequest::new( - host.clone(), - reason.clone(), - client.clone(), - Some(req.method().as_str().to_string()), - None, - "http".to_string(), - )) + .record_blocked(BlockedRequest::new(BlockedRequestArgs { + host: host.clone(), + reason: reason.clone(), + client: client.clone(), + method: Some(req.method().as_str().to_string()), + mode: None, + protocol: "http".to_string(), + })) .await; let client = client.as_deref().unwrap_or_default(); warn!("request blocked (client={client}, host={host}, reason={reason})"); @@ -460,14 +462,14 @@ async fn http_plain_proxy( if !method_allowed { let _ = app_state - .record_blocked(BlockedRequest::new( - host.clone(), - REASON_METHOD_NOT_ALLOWED.to_string(), - client.clone(), - Some(req.method().as_str().to_string()), - Some(NetworkMode::Limited), - "http".to_string(), - )) + .record_blocked(BlockedRequest::new(BlockedRequestArgs { + host: host.clone(), + reason: REASON_METHOD_NOT_ALLOWED.to_string(), + client: client.clone(), + method: Some(req.method().as_str().to_string()), + mode: Some(NetworkMode::Limited), + protocol: "http".to_string(), + })) .await; let client = client.as_deref().unwrap_or_default(); let method = req.method(); @@ -565,14 +567,14 @@ async fn proxy_disabled_response( protocol: &str, ) -> Response { let _ = app_state - .record_blocked(BlockedRequest::new( + .record_blocked(BlockedRequest::new(BlockedRequestArgs { host, - REASON_PROXY_DISABLED.to_string(), + reason: REASON_PROXY_DISABLED.to_string(), client, method, - None, - protocol.to_string(), - )) + mode: None, + protocol: protocol.to_string(), + })) .await; text_response(StatusCode::SERVICE_UNAVAILABLE, "proxy disabled") } diff --git a/codex-rs/network-proxy/src/lib.rs b/codex-rs/network-proxy/src/lib.rs index f4bcb8c74..e63627312 100644 --- a/codex-rs/network-proxy/src/lib.rs +++ b/codex-rs/network-proxy/src/lib.rs @@ -17,6 +17,7 @@ use anyhow::Result; pub use network_policy::NetworkDecision; pub use network_policy::NetworkPolicyDecider; pub use network_policy::NetworkPolicyRequest; +pub use network_policy::NetworkPolicyRequestArgs; pub use network_policy::NetworkProtocol; pub use proxy::Args; pub use proxy::NetworkProxy; diff --git a/codex-rs/network-proxy/src/network_policy.rs b/codex-rs/network-proxy/src/network_policy.rs index 6f410b3e4..f14202510 100644 --- a/codex-rs/network-proxy/src/network_policy.rs +++ b/codex-rs/network-proxy/src/network_policy.rs @@ -26,16 +26,27 @@ pub struct NetworkPolicyRequest { pub exec_policy_hint: Option, } +pub struct NetworkPolicyRequestArgs { + pub protocol: NetworkProtocol, + pub host: String, + pub port: u16, + pub client_addr: Option, + pub method: Option, + pub command: Option, + pub exec_policy_hint: Option, +} + impl NetworkPolicyRequest { - pub fn new( - protocol: NetworkProtocol, - host: String, - port: u16, - client_addr: Option, - method: Option, - command: Option, - exec_policy_hint: Option, - ) -> Self { + pub fn new(args: NetworkPolicyRequestArgs) -> Self { + let NetworkPolicyRequestArgs { + protocol, + host, + port, + client_addr, + method, + command, + exec_policy_hint, + } = args; Self { protocol, host, @@ -139,15 +150,15 @@ mod tests { } }); - let request = NetworkPolicyRequest::new( - NetworkProtocol::Http, - "example.com".to_string(), - 80, - None, - Some("GET".to_string()), - None, - None, - ); + let request = NetworkPolicyRequest::new(NetworkPolicyRequestArgs { + protocol: NetworkProtocol::Http, + host: "example.com".to_string(), + port: 80, + client_addr: None, + method: Some("GET".to_string()), + command: None, + exec_policy_hint: None, + }); let decision = evaluate_host_policy(&state, Some(&decider), &request) .await @@ -172,15 +183,15 @@ mod tests { } }); - let request = NetworkPolicyRequest::new( - NetworkProtocol::Http, - "blocked.com".to_string(), - 80, - None, - Some("GET".to_string()), - None, - None, - ); + let request = NetworkPolicyRequest::new(NetworkPolicyRequestArgs { + protocol: NetworkProtocol::Http, + host: "blocked.com".to_string(), + port: 80, + client_addr: None, + method: Some("GET".to_string()), + command: None, + exec_policy_hint: None, + }); let decision = evaluate_host_policy(&state, Some(&decider), &request) .await @@ -210,15 +221,15 @@ mod tests { } }); - let request = NetworkPolicyRequest::new( - NetworkProtocol::Http, - "127.0.0.1".to_string(), - 80, - None, - Some("GET".to_string()), - None, - None, - ); + let request = NetworkPolicyRequest::new(NetworkPolicyRequestArgs { + protocol: NetworkProtocol::Http, + host: "127.0.0.1".to_string(), + port: 80, + client_addr: None, + method: Some("GET".to_string()), + command: None, + exec_policy_hint: None, + }); let decision = evaluate_host_policy(&state, Some(&decider), &request) .await diff --git a/codex-rs/network-proxy/src/runtime.rs b/codex-rs/network-proxy/src/runtime.rs index 49e34d5c0..1c29672cf 100644 --- a/codex-rs/network-proxy/src/runtime.rs +++ b/codex-rs/network-proxy/src/runtime.rs @@ -73,15 +73,25 @@ pub struct BlockedRequest { pub timestamp: i64, } +pub struct BlockedRequestArgs { + pub host: String, + pub reason: String, + pub client: Option, + pub method: Option, + pub mode: Option, + pub protocol: String, +} + impl BlockedRequest { - pub fn new( - host: String, - reason: String, - client: Option, - method: Option, - mode: Option, - protocol: String, - ) -> Self { + pub fn new(args: BlockedRequestArgs) -> Self { + let BlockedRequestArgs { + host, + reason, + client, + method, + mode, + protocol, + } = args; Self { host, reason, diff --git a/codex-rs/network-proxy/src/socks5.rs b/codex-rs/network-proxy/src/socks5.rs index 942650254..44be9060b 100644 --- a/codex-rs/network-proxy/src/socks5.rs +++ b/codex-rs/network-proxy/src/socks5.rs @@ -2,12 +2,14 @@ use crate::config::NetworkMode; use crate::network_policy::NetworkDecision; use crate::network_policy::NetworkPolicyDecider; use crate::network_policy::NetworkPolicyRequest; +use crate::network_policy::NetworkPolicyRequestArgs; use crate::network_policy::NetworkProtocol; use crate::network_policy::evaluate_host_policy; use crate::policy::normalize_host; use crate::reasons::REASON_METHOD_NOT_ALLOWED; use crate::reasons::REASON_PROXY_DISABLED; use crate::state::BlockedRequest; +use crate::state::BlockedRequestArgs; use crate::state::NetworkProxyState; use anyhow::Context as _; use anyhow::Result; @@ -122,14 +124,14 @@ async fn handle_socks5_tcp( Ok(true) => {} Ok(false) => { let _ = app_state - .record_blocked(BlockedRequest::new( - host.clone(), - REASON_PROXY_DISABLED.to_string(), - client.clone(), - None, - None, - "socks5".to_string(), - )) + .record_blocked(BlockedRequest::new(BlockedRequestArgs { + host: host.clone(), + reason: REASON_PROXY_DISABLED.to_string(), + client: client.clone(), + method: None, + mode: None, + protocol: "socks5".to_string(), + })) .await; let client = client.as_deref().unwrap_or_default(); warn!("SOCKS blocked; proxy disabled (client={client}, host={host})"); @@ -144,14 +146,14 @@ async fn handle_socks5_tcp( match app_state.network_mode().await { Ok(NetworkMode::Limited) => { let _ = app_state - .record_blocked(BlockedRequest::new( - host.clone(), - REASON_METHOD_NOT_ALLOWED.to_string(), - client.clone(), - None, - Some(NetworkMode::Limited), - "socks5".to_string(), - )) + .record_blocked(BlockedRequest::new(BlockedRequestArgs { + host: host.clone(), + reason: REASON_METHOD_NOT_ALLOWED.to_string(), + client: client.clone(), + method: None, + mode: Some(NetworkMode::Limited), + protocol: "socks5".to_string(), + })) .await; let client = client.as_deref().unwrap_or_default(); warn!( @@ -166,27 +168,27 @@ async fn handle_socks5_tcp( } } - let request = NetworkPolicyRequest::new( - NetworkProtocol::Socks5Tcp, - host.clone(), + let request = NetworkPolicyRequest::new(NetworkPolicyRequestArgs { + protocol: NetworkProtocol::Socks5Tcp, + host: host.clone(), port, - client.clone(), - None, - None, - None, - ); + client_addr: client.clone(), + method: None, + command: None, + exec_policy_hint: None, + }); match evaluate_host_policy(&app_state, policy_decider.as_ref(), &request).await { Ok(NetworkDecision::Deny { reason }) => { let _ = app_state - .record_blocked(BlockedRequest::new( - host.clone(), - reason.clone(), - client.clone(), - None, - None, - "socks5".to_string(), - )) + .record_blocked(BlockedRequest::new(BlockedRequestArgs { + host: host.clone(), + reason: reason.clone(), + client: client.clone(), + method: None, + mode: None, + protocol: "socks5".to_string(), + })) .await; let client = client.as_deref().unwrap_or_default(); warn!("SOCKS blocked (client={client}, host={host}, reason={reason})"); @@ -231,14 +233,14 @@ async fn inspect_socks5_udp( Ok(true) => {} Ok(false) => { let _ = state - .record_blocked(BlockedRequest::new( - host.clone(), - REASON_PROXY_DISABLED.to_string(), - client.clone(), - None, - None, - "socks5-udp".to_string(), - )) + .record_blocked(BlockedRequest::new(BlockedRequestArgs { + host: host.clone(), + reason: REASON_PROXY_DISABLED.to_string(), + client: client.clone(), + method: None, + mode: None, + protocol: "socks5-udp".to_string(), + })) .await; let client = client.as_deref().unwrap_or_default(); warn!("SOCKS UDP blocked; proxy disabled (client={client}, host={host})"); @@ -256,14 +258,14 @@ async fn inspect_socks5_udp( match state.network_mode().await { Ok(NetworkMode::Limited) => { let _ = state - .record_blocked(BlockedRequest::new( - host.clone(), - REASON_METHOD_NOT_ALLOWED.to_string(), - client.clone(), - None, - Some(NetworkMode::Limited), - "socks5-udp".to_string(), - )) + .record_blocked(BlockedRequest::new(BlockedRequestArgs { + host: host.clone(), + reason: REASON_METHOD_NOT_ALLOWED.to_string(), + client: client.clone(), + method: None, + mode: Some(NetworkMode::Limited), + protocol: "socks5-udp".to_string(), + })) .await; return Ok(RelayResponse { maybe_payload: None, @@ -277,27 +279,27 @@ async fn inspect_socks5_udp( } } - let request = NetworkPolicyRequest::new( - NetworkProtocol::Socks5Udp, - host.clone(), + let request = NetworkPolicyRequest::new(NetworkPolicyRequestArgs { + protocol: NetworkProtocol::Socks5Udp, + host: host.clone(), port, - client.clone(), - None, - None, - None, - ); + client_addr: client.clone(), + method: None, + command: None, + exec_policy_hint: None, + }); match evaluate_host_policy(&state, policy_decider.as_ref(), &request).await { Ok(NetworkDecision::Deny { reason }) => { let _ = state - .record_blocked(BlockedRequest::new( - host.clone(), - reason.clone(), - client.clone(), - None, - None, - "socks5-udp".to_string(), - )) + .record_blocked(BlockedRequest::new(BlockedRequestArgs { + host: host.clone(), + reason: reason.clone(), + client: client.clone(), + method: None, + mode: None, + protocol: "socks5-udp".to_string(), + })) .await; let client = client.as_deref().unwrap_or_default(); warn!("SOCKS UDP blocked (client={client}, host={host}, reason={reason})"); diff --git a/codex-rs/network-proxy/src/state.rs b/codex-rs/network-proxy/src/state.rs index e1e8a1e4e..552455f51 100644 --- a/codex-rs/network-proxy/src/state.rs +++ b/codex-rs/network-proxy/src/state.rs @@ -20,6 +20,7 @@ use serde::Deserialize; use std::collections::HashSet; pub use crate::runtime::BlockedRequest; +pub use crate::runtime::BlockedRequestArgs; pub use crate::runtime::NetworkProxyState; #[cfg(test)] pub(crate) use crate::runtime::network_proxy_state_for_policy;