From 472619f9fc6739d5c4fa499b3ef10007655b031d Mon Sep 17 00:00:00 2001 From: Winston Howes Date: Tue, 9 Jun 2026 13:08:01 -0700 Subject: [PATCH 1/3] Add SOCKS5 TCP MITM coverage (#22685) ## Summary - reuse the MITM HTTPS serving path for raw SOCKS5 TCP streams - route limited-mode and hooked SOCKS5 TCP requests through MITM before dialing upstream - keep SOCKS5 UDP limited-mode behavior unchanged ## Validation - `just fmt` - `just test -p codex-network-proxy` - `just fix -p codex-network-proxy` - `git diff --check` --- codex-rs/network-proxy/README.md | 9 +- codex-rs/network-proxy/src/config.rs | 2 +- codex-rs/network-proxy/src/mitm.rs | 26 +- codex-rs/network-proxy/src/socks5.rs | 510 +++++++++++++++++++++------ 4 files changed, 419 insertions(+), 128 deletions(-) diff --git a/codex-rs/network-proxy/README.md b/codex-rs/network-proxy/README.md index 300cd3fcb0..d5d0730227 100644 --- a/codex-rs/network-proxy/README.md +++ b/codex-rs/network-proxy/README.md @@ -107,9 +107,9 @@ When a request is blocked, the proxy responds with `403` and includes: - `blocked-by-method-policy` - `blocked-by-policy` -In "limited" mode, only `GET`, `HEAD`, and `OPTIONS` are allowed. HTTPS `CONNECT` requests require -MITM to enforce limited-mode method policy; otherwise they are blocked. SOCKS5 remains blocked in -limited mode. +In "limited" mode, only `GET`, `HEAD`, and `OPTIONS` are allowed. HTTPS `CONNECT` requests and +HTTPS SOCKS5 TCP targets on `:443` require MITM to enforce limited-mode method policy; otherwise +they are blocked. SOCKS5 UDP and non-HTTPS SOCKS5 TCP remain blocked in limited mode. Websocket clients typically tunnel `wss://` through HTTPS `CONNECT`; those CONNECT targets still go through the same host allowlist/denylist checks. @@ -215,7 +215,8 @@ what it can reasonably guarantee. allowlisted (best-effort DNS lookup). - Limited mode enforcement: - only `GET`, `HEAD`, and `OPTIONS` are allowed - - HTTPS `CONNECT` remains a tunnel; limited-mode method enforcement does not apply to HTTPS + - HTTPS `CONNECT` requests and HTTPS SOCKS5 TCP targets on `:443` require MITM so the proxy can + enforce limited-mode method policy; SOCKS5 UDP and non-HTTPS SOCKS5 TCP remain blocked - Listener safety defaults: - the HTTP proxy listener clamps non-loopback binds unless explicitly enabled via `dangerously_allow_non_loopback_proxy` diff --git a/codex-rs/network-proxy/src/config.rs b/codex-rs/network-proxy/src/config.rs index 0b0a8233f5..d9cef46b15 100644 --- a/codex-rs/network-proxy/src/config.rs +++ b/codex-rs/network-proxy/src/config.rs @@ -276,7 +276,7 @@ impl NetworkProxySettings { pub enum NetworkMode { /// Limited (read-only) access: only GET/HEAD/OPTIONS are allowed for HTTP. HTTPS CONNECT is /// blocked unless MITM is enabled so the proxy can enforce method policy on inner requests. - /// SOCKS5 remains blocked in limited mode. + /// SOCKS5 UDP and non-HTTPS SOCKS5 TCP remain blocked in limited mode. Limited, /// Full network access: all HTTP methods are allowed. HTTPS CONNECTs are tunneled directly. /// MITM hooks do not currently make full mode enter MITM. diff --git a/codex-rs/network-proxy/src/mitm.rs b/codex-rs/network-proxy/src/mitm.rs index 9ee5bfc253..345c5b5032 100644 --- a/codex-rs/network-proxy/src/mitm.rs +++ b/codex-rs/network-proxy/src/mitm.rs @@ -21,10 +21,12 @@ use rama_core::Layer; use rama_core::Service; use rama_core::bytes::Bytes; use rama_core::error::BoxError; +use rama_core::extensions::ExtensionsMut; use rama_core::extensions::ExtensionsRef; -use rama_core::futures::stream::Stream; +use rama_core::futures::stream::Stream as FuturesStream; use rama_core::rt::Executor; use rama_core::service::service_fn; +use rama_core::stream::Stream; use rama_http::Body; use rama_http::BodyDataStream; use rama_http::HeaderMap; @@ -135,17 +137,25 @@ impl MitmState { /// Terminate the upgraded CONNECT stream with a generated leaf cert and proxy inner HTTPS traffic. pub(crate) async fn mitm_tunnel(upgraded: Upgraded) -> Result<()> { - let mitm = upgraded + mitm_stream(upgraded).await +} + +/// Terminate a raw client stream with a generated leaf cert and proxy inner HTTPS traffic. +pub(crate) async fn mitm_stream(stream: S) -> Result<()> +where + S: Stream + Unpin + ExtensionsMut, +{ + let mitm = stream .extensions() .get::>() .cloned() .context("missing MITM state")?; - let app_state = upgraded + let app_state = stream .extensions() .get::>() .cloned() .context("missing app state")?; - let target = upgraded + let target = stream .extensions() .get::() .context("missing proxy target")? @@ -154,7 +164,7 @@ pub(crate) async fn mitm_tunnel(upgraded: Upgraded) -> Result<()> { let target_host = normalize_host(&target.host.to_string()); let target_port = target.port; let acceptor_data = mitm.tls_acceptor_data_for_host(&target_host)?; - let mode = upgraded + let mode = stream .extensions() .get::() .copied() @@ -169,7 +179,7 @@ pub(crate) async fn mitm_tunnel(upgraded: Upgraded) -> Result<()> { mitm, }); - let executor = upgraded + let executor = stream .extensions() .get::() .cloned() @@ -194,7 +204,7 @@ pub(crate) async fn mitm_tunnel(upgraded: Upgraded) -> Result<()> { .into_layer(http_service); https_service - .serve(upgraded) + .serve(stream) .await .map_err(|err| anyhow!("MITM serve error: {err}"))?; Ok(()) @@ -456,7 +466,7 @@ struct InspectStream { max_body_bytes: usize, } -impl Stream for InspectStream { +impl FuturesStream for InspectStream { type Item = Result; fn poll_next(self: Pin<&mut Self>, cx: &mut TaskContext<'_>) -> Poll> { diff --git a/codex-rs/network-proxy/src/socks5.rs b/codex-rs/network-proxy/src/socks5.rs index 8369c4da80..d9a8a559fd 100644 --- a/codex-rs/network-proxy/src/socks5.rs +++ b/codex-rs/network-proxy/src/socks5.rs @@ -1,5 +1,6 @@ use crate::config::NetworkMode; use crate::connect_policy::TargetCheckedTcpConnector; +use crate::mitm; use crate::network_policy::BlockDecisionAuditEventArgs; use crate::network_policy::NetworkDecision; use crate::network_policy::NetworkDecisionSource; @@ -24,10 +25,17 @@ use anyhow::Result; use rama_core::Layer; use rama_core::Service; use rama_core::error::BoxError; +use rama_core::extensions::Extensions; +use rama_core::extensions::ExtensionsMut; use rama_core::extensions::ExtensionsRef; use rama_core::layer::AddInputExtensionLayer; use rama_core::service::service_fn; +use rama_net::address::HostWithPort; use rama_net::client::EstablishedClientConnection; +use rama_net::proxy::ProxyRequest; +use rama_net::proxy::ProxyTarget; +use rama_net::proxy::StreamForwardService; +use rama_net::stream::Socket; use rama_net::stream::SocketInfo; use rama_socks5::Socks5Acceptor; use rama_socks5::server::DefaultConnector; @@ -40,8 +48,14 @@ use rama_tcp::server::TcpListener; use std::io; use std::net::SocketAddr; use std::net::TcpListener as StdTcpListener; +use std::pin::Pin; use std::sync::Arc; +use std::task::Context as TaskContext; +use std::task::Poll; use std::time::Instant; +use tokio::io::AsyncRead; +use tokio::io::AsyncWrite; +use tokio::io::ReadBuf; use tracing::error; use tracing::info; use tracing::warn; @@ -88,7 +102,9 @@ async fn run_socks5_with_listener( match state.network_mode().await { Ok(NetworkMode::Limited) => { - info!("SOCKS5 is blocked in limited mode; set mode=\"full\" to allow SOCKS5"); + info!( + "SOCKS5 UDP and non-HTTPS SOCKS5 TCP are blocked in limited mode; HTTPS SOCKS5 TCP requires MITM inspection" + ); } Ok(NetworkMode::Full) => {} Err(err) => { @@ -106,7 +122,10 @@ async fn run_socks5_with_listener( } }); - let socks_connector = DefaultConnector::default().with_connector(policy_tcp_connector); + let socks_proxy = service_fn(|request| async move { proxy_socks5_tcp(request).await }); + let socks_connector = DefaultConnector::default() + .with_connector(policy_tcp_connector) + .with_service(socks_proxy); let base = Socks5Acceptor::new().with_connector(socks_connector); if enable_socks5_udp { @@ -135,7 +154,7 @@ async fn handle_socks5_tcp( req: TcpRequest, tcp_connector: TargetCheckedTcpConnector, policy_decider: Option>, -) -> Result, BoxError> { +) -> Result, BoxError> { let app_state = req .extensions() .get::>() @@ -144,6 +163,7 @@ async fn handle_socks5_tcp( let host = normalize_host(&req.authority.host.to_string()); let port = req.authority.port; + let target = req.authority.clone(); if host.is_empty() { return Err(io::Error::new(io::ErrorKind::InvalidInput, "invalid host").into()); } @@ -196,94 +216,52 @@ async fn handle_socks5_tcp( } } - match app_state.network_mode().await { - Ok(NetworkMode::Limited) => { - emit_socks_block_decision_audit_event( - &app_state, - NetworkDecisionSource::ModeGuard, - REASON_METHOD_NOT_ALLOWED, - NetworkProtocol::Socks5Tcp, - host.as_str(), - port, - client.as_deref(), - ); - let details = PolicyDecisionDetails { - decision: NetworkPolicyDecision::Deny, - reason: REASON_METHOD_NOT_ALLOWED, - source: NetworkDecisionSource::ModeGuard, - protocol: NetworkProtocol::Socks5Tcp, - host: &host, - port, - }; - let _ = app_state - .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(), - decision: Some(details.decision.as_str().to_string()), - source: Some(details.source.as_str().to_string()), - port: Some(port), - })) - .await; - let client = client.as_deref().unwrap_or_default(); - warn!( - "SOCKS blocked by method policy (client={client}, host={host}, mode=limited, allowed_methods=GET, HEAD, OPTIONS)" - ); - return Err(policy_denied_error(REASON_METHOD_NOT_ALLOWED, &details).into()); - } - Ok(NetworkMode::Full) => {} + let mode = match app_state.network_mode().await { + Ok(mode) => mode, Err(err) => { error!("failed to evaluate method policy: {err}"); return Err(io::Error::other("proxy error").into()); } - } - - match app_state.host_has_mitm_hooks(&host).await { - Ok(true) => { - emit_socks_block_decision_audit_event( - &app_state, - NetworkDecisionSource::ModeGuard, - REASON_MITM_REQUIRED, - NetworkProtocol::Socks5Tcp, - host.as_str(), - port, - client.as_deref(), - ); - let details = PolicyDecisionDetails { - decision: NetworkPolicyDecision::Deny, - reason: REASON_MITM_REQUIRED, - source: NetworkDecisionSource::ModeGuard, - protocol: NetworkProtocol::Socks5Tcp, - host: &host, - port, - }; - let _ = app_state - .record_blocked(BlockedRequest::new(BlockedRequestArgs { - host: host.clone(), - reason: REASON_MITM_REQUIRED.to_string(), - client: client.clone(), - method: None, - mode: Some(NetworkMode::Full), - protocol: "socks5".to_string(), - decision: Some(details.decision.as_str().to_string()), - source: Some(details.source.as_str().to_string()), - port: Some(port), - })) - .await; - let client = client.as_deref().unwrap_or_default(); - warn!( - "SOCKS blocked; MITM required to enforce HTTPS policy (client={client}, host={host}, mode=full)" - ); - return Err(policy_denied_error(REASON_MITM_REQUIRED, &details).into()); - } - Ok(false) => {} - Err(err) => { - error!("failed to inspect MITM hooks for {host}: {err}"); - return Err(io::Error::other("proxy error").into()); - } + }; + // SOCKS5 only exposes host and port, so only the default HTTPS port is identifiable as a + // TLS stream that the HTTPS MITM path can safely terminate. + let socks5_tcp_target_is_https = port == 443; + if mode == NetworkMode::Limited && !socks5_tcp_target_is_https { + emit_socks_block_decision_audit_event( + &app_state, + NetworkDecisionSource::ModeGuard, + REASON_METHOD_NOT_ALLOWED, + NetworkProtocol::Socks5Tcp, + host.as_str(), + port, + client.as_deref(), + ); + let details = PolicyDecisionDetails { + decision: NetworkPolicyDecision::Deny, + reason: REASON_METHOD_NOT_ALLOWED, + source: NetworkDecisionSource::ModeGuard, + protocol: NetworkProtocol::Socks5Tcp, + host: &host, + port, + }; + let _ = app_state + .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(), + decision: Some(details.decision.as_str().to_string()), + source: Some(details.source.as_str().to_string()), + port: Some(port), + })) + .await; + let client = client.as_deref().unwrap_or_default(); + warn!( + "SOCKS blocked; limited mode only supports HTTPS MITM (client={client}, host={host}, port={port})" + ); + return Err(policy_denied_error(REASON_METHOD_NOT_ALLOWED, &details).into()); } let request = NetworkPolicyRequest::new(NetworkPolicyRequestArgs { @@ -337,9 +315,85 @@ async fn handle_socks5_tcp( } } + let host_has_mitm_hooks = match app_state.host_has_mitm_hooks(&host).await { + Ok(has_hooks) => has_hooks, + Err(err) => { + error!("failed to inspect MITM hooks for {host}: {err}"); + return Err(io::Error::other("proxy error").into()); + } + }; + let mitm_state = match app_state.mitm_state().await { + Ok(state) => state, + Err(err) => { + error!("failed to load MITM state: {err}"); + return Err(io::Error::other("proxy error").into()); + } + }; + let socks_needs_mitm = + socks5_tcp_target_is_https && (mode == NetworkMode::Limited || host_has_mitm_hooks); + if (host_has_mitm_hooks && !socks5_tcp_target_is_https) + || (socks_needs_mitm && mitm_state.is_none()) + { + emit_socks_block_decision_audit_event( + &app_state, + NetworkDecisionSource::ModeGuard, + REASON_MITM_REQUIRED, + NetworkProtocol::Socks5Tcp, + host.as_str(), + port, + client.as_deref(), + ); + let details = PolicyDecisionDetails { + decision: NetworkPolicyDecision::Deny, + reason: REASON_MITM_REQUIRED, + source: NetworkDecisionSource::ModeGuard, + protocol: NetworkProtocol::Socks5Tcp, + host: &host, + port, + }; + let _ = app_state + .record_blocked(BlockedRequest::new(BlockedRequestArgs { + host: host.clone(), + reason: REASON_MITM_REQUIRED.to_string(), + client: client.clone(), + method: None, + mode: Some(mode), + protocol: "socks5".to_string(), + decision: Some(details.decision.as_str().to_string()), + source: Some(details.source.as_str().to_string()), + port: Some(port), + })) + .await; + let client = client.as_deref().unwrap_or_default(); + warn!( + "SOCKS blocked; MITM required to enforce HTTPS policy (client={client}, host={host}, mode={mode:?}, hooked_host={host_has_mitm_hooks}, https_target={socks5_tcp_target_is_https})" + ); + return Err(policy_denied_error(REASON_MITM_REQUIRED, &details).into()); + } + + if socks_needs_mitm && let Some(mitm_state) = mitm_state { + let client = client.as_deref().unwrap_or_default(); + info!("SOCKS MITM enabled (client={client}, host={host}, port={port}, mode={mode:?})"); + return Ok(EstablishedClientConnection { + input: req, + conn: Socks5TcpConnection::Mitm { + target, + mode, + mitm: mitm_state, + extensions: Extensions::new(), + }, + }); + } + info!("SOCKS upstream dial started (host={host}, port={port})"); let connect_started_at = Instant::now(); - let result = tcp_connector.serve(req).await; + let result = tcp_connector.serve(req).await.map(|connection| { + let EstablishedClientConnection { input, conn } = connection; + EstablishedClientConnection { + input, + conn: Socks5TcpConnection::Direct(conn), + } + }); match &result { Ok(_) => info!( "SOCKS upstream dial established (host={host}, port={port}, elapsed_ms={})", @@ -353,6 +407,113 @@ async fn handle_socks5_tcp( result } +/// Internal connector output for SOCKS5 TCP. MITM requests do not dial upstream before the +/// inner HTTPS request is inspected, so they carry the target metadata instead of a socket. +#[derive(Debug)] +enum Socks5TcpConnection { + Direct(TcpStream), + Mitm { + target: HostWithPort, + mode: NetworkMode, + mitm: Arc, + extensions: Extensions, + }, +} + +impl AsyncRead for Socks5TcpConnection { + fn poll_read( + self: Pin<&mut Self>, + cx: &mut TaskContext<'_>, + buf: &mut ReadBuf<'_>, + ) -> Poll> { + match self.get_mut() { + Self::Direct(stream) => Pin::new(stream).poll_read(cx, buf), + Self::Mitm { .. } => Poll::Ready(Ok(())), + } + } +} + +impl AsyncWrite for Socks5TcpConnection { + fn poll_write( + self: Pin<&mut Self>, + cx: &mut TaskContext<'_>, + buf: &[u8], + ) -> Poll> { + match self.get_mut() { + Self::Direct(stream) => Pin::new(stream).poll_write(cx, buf), + Self::Mitm { .. } => Poll::Ready(Ok(buf.len())), + } + } + + fn poll_flush(self: Pin<&mut Self>, cx: &mut TaskContext<'_>) -> Poll> { + match self.get_mut() { + Self::Direct(stream) => Pin::new(stream).poll_flush(cx), + Self::Mitm { .. } => Poll::Ready(Ok(())), + } + } + + fn poll_shutdown(self: Pin<&mut Self>, cx: &mut TaskContext<'_>) -> Poll> { + match self.get_mut() { + Self::Direct(stream) => Pin::new(stream).poll_shutdown(cx), + Self::Mitm { .. } => Poll::Ready(Ok(())), + } + } +} + +impl Socket for Socks5TcpConnection { + fn local_addr(&self) -> io::Result { + match self { + Self::Direct(stream) => stream.local_addr(), + Self::Mitm { .. } => Ok(SocketAddr::from(([0, 0, 0, 0], 0))), + } + } + + fn peer_addr(&self) -> io::Result { + match self { + Self::Direct(stream) => stream.peer_addr(), + Self::Mitm { .. } => Ok(SocketAddr::from(([0, 0, 0, 0], 0))), + } + } +} + +impl ExtensionsRef for Socks5TcpConnection { + fn extensions(&self) -> &Extensions { + match self { + Self::Direct(stream) => stream.extensions(), + Self::Mitm { extensions, .. } => extensions, + } + } +} + +impl ExtensionsMut for Socks5TcpConnection { + fn extensions_mut(&mut self) -> &mut Extensions { + match self { + Self::Direct(stream) => stream.extensions_mut(), + Self::Mitm { extensions, .. } => extensions, + } + } +} + +async fn proxy_socks5_tcp( + request: ProxyRequest, +) -> Result<(), BoxError> { + let ProxyRequest { mut source, target } = request; + match target { + Socks5TcpConnection::Direct(target) => StreamForwardService::default() + .serve(ProxyRequest { source, target }) + .await + .map_err(Into::into), + Socks5TcpConnection::Mitm { + target, mode, mitm, .. + } => { + source.extensions_mut().insert(ProxyTarget(target)); + source.extensions_mut().insert(mode); + source.extensions_mut().insert(mitm); + mitm::mitm_stream(source).await.map_err(Into::into) + } + } +} + async fn inspect_socks5_udp( request: RelayRequest, state: Arc, @@ -554,7 +715,6 @@ mod tests { use crate::network_policy::test_support::find_event_by_name; use crate::runtime::ConfigReloader; use crate::runtime::ConfigState; - use crate::runtime::network_proxy_state_for_policy; use crate::state::NetworkProxyConstraints; use crate::state::build_config_state; use async_trait::async_trait; @@ -567,6 +727,11 @@ mod tests { use std::net::IpAddr; use std::net::Ipv4Addr; use std::sync::Arc; + use std::sync::Mutex; + + // Managed MITM CA files live under the shared test CODEX_HOME, so MITM-enabled config state + // must be materialized one test at a time. + static MITM_CONFIG_STATE_LOCK: Mutex<()> = Mutex::new(()); #[derive(Clone)] struct StaticReloader { @@ -590,6 +755,10 @@ mod tests { fn state_for_settings(network: NetworkProxySettings) -> Arc { let config = NetworkProxyConfig { network }; + let _mitm_config_state_guard = config + .network + .mitm + .then(|| MITM_CONFIG_STATE_LOCK.lock().unwrap()); let state = build_config_state(config, NetworkProxyConstraints::default()).unwrap(); let reloader = Arc::new(StaticReloader { state: state.clone(), @@ -639,43 +808,56 @@ mod tests { } #[tokio::test(flavor = "current_thread")] - async fn handle_socks5_tcp_blocks_hooked_host_in_full_mode() { - let state = Arc::new(network_proxy_state_for_policy(NetworkProxySettings { + async fn handle_socks5_tcp_uses_mitm_in_limited_mode() { + let mut settings = NetworkProxySettings { enabled: true, - mode: NetworkMode::Full, + mode: NetworkMode::Limited, mitm: true, - mitm_hooks: vec![MitmHookConfig { - host: "api.github.com".to_string(), - matcher: MitmHookMatchConfig { - methods: vec!["GET".to_string()], - path_prefixes: vec!["/".to_string()], - ..MitmHookMatchConfig::default() - }, - ..MitmHookConfig::default() - }], ..NetworkProxySettings::default() - })); + }; + settings.set_allowed_domains(vec!["example.com".to_string()]); + let state = state_for_settings(settings); let mut request = - TcpRequest::new(HostWithPort::try_from("api.github.com:443").expect("valid authority")); + TcpRequest::new(HostWithPort::try_from("example.com:443").expect("valid authority")); + request.extensions_mut().insert(state.clone()); + + let result = handle_socks5_tcp( + request, + TargetCheckedTcpConnector::new(state), + /*policy_decider*/ None, + ) + .await + .expect("limited-mode HTTPS should use MITM"); + + assert!(matches!(result.conn, Socks5TcpConnection::Mitm { .. })); + } + + #[tokio::test(flavor = "current_thread")] + async fn handle_socks5_tcp_blocks_non_https_in_limited_mode() { + let mut settings = NetworkProxySettings { + enabled: true, + mode: NetworkMode::Limited, + ..NetworkProxySettings::default() + }; + settings.set_allowed_domains(vec!["example.com".to_string()]); + let state = state_for_settings(settings); + let mut request = + TcpRequest::new(HostWithPort::try_from("example.com:80").expect("valid authority")); request.extensions_mut().insert(state.clone()); let (result, events) = capture_events(|| async { handle_socks5_tcp( request, - TargetCheckedTcpConnector::new(state.clone()), + TargetCheckedTcpConnector::new(state), /*policy_decider*/ None, ) .await }) .await; - assert!(result.is_err(), "hooked host should require MITM"); - - let blocked = state.drain_blocked().await.unwrap(); - assert_eq!(blocked.len(), 1); - assert_eq!(blocked[0].reason, REASON_MITM_REQUIRED); - assert_eq!(blocked[0].host, "api.github.com"); - assert_eq!(blocked[0].port, Some(443)); - assert_eq!(blocked[0].protocol, "socks5"); + assert!( + result.is_err(), + "limited-mode non-HTTPS SOCKS should be denied" + ); let event = find_event_by_name(&events, POLICY_DECISION_EVENT_NAME) .expect("expected policy decision event"); @@ -684,18 +866,116 @@ mod tests { assert_eq!(event.field("network.policy.source"), Some("mode_guard")); assert_eq!( event.field("network.policy.reason"), - Some(REASON_MITM_REQUIRED) + Some(REASON_METHOD_NOT_ALLOWED) ); assert_eq!( event.field("network.transport.protocol"), Some("socks5_tcp") ); - assert_eq!(event.field("server.address"), Some("api.github.com")); - assert_eq!(event.field("server.port"), Some("443")); + assert_eq!(event.field("server.address"), Some("example.com")); + assert_eq!(event.field("server.port"), Some("80")); assert_eq!(event.field("http.request.method"), Some("none")); assert_eq!(event.field("client.address"), Some("unknown")); } + #[tokio::test(flavor = "current_thread")] + async fn handle_socks5_tcp_blocks_limited_mode_without_mitm_state() { + let mut settings = NetworkProxySettings { + enabled: true, + mode: NetworkMode::Limited, + ..NetworkProxySettings::default() + }; + settings.set_allowed_domains(vec!["example.com".to_string()]); + let state = state_for_settings(settings); + let mut request = + TcpRequest::new(HostWithPort::try_from("example.com:443").expect("valid authority")); + request.extensions_mut().insert(state.clone()); + + let err = handle_socks5_tcp( + request, + TargetCheckedTcpConnector::new(state), + /*policy_decider*/ None, + ) + .await + .expect_err("limited-mode HTTPS requires MITM"); + + assert!( + format!("{err:?}").contains("MITM required"), + "unexpected error: {err:?}" + ); + } + + #[tokio::test(flavor = "current_thread")] + async fn handle_socks5_tcp_uses_mitm_for_hooked_host_in_full_mode() { + let mut settings = NetworkProxySettings { + enabled: true, + mode: NetworkMode::Full, + mitm: true, + mitm_hooks: vec![MitmHookConfig { + host: "api.github.com".to_string(), + matcher: MitmHookMatchConfig { + methods: vec!["POST".to_string()], + path_prefixes: vec!["/repos/openai/".to_string()], + ..MitmHookMatchConfig::default() + }, + ..MitmHookConfig::default() + }], + ..NetworkProxySettings::default() + }; + settings.set_allowed_domains(vec!["api.github.com".to_string()]); + let state = state_for_settings(settings); + let mut request = + TcpRequest::new(HostWithPort::try_from("api.github.com:443").expect("valid authority")); + request.extensions_mut().insert(state.clone()); + + let result = handle_socks5_tcp( + request, + TargetCheckedTcpConnector::new(state), + /*policy_decider*/ None, + ) + .await + .expect("hooked HTTPS should use MITM"); + + assert!(matches!(result.conn, Socks5TcpConnection::Mitm { .. })); + } + + #[tokio::test(flavor = "current_thread")] + async fn handle_socks5_tcp_blocks_hooked_non_https_host_in_full_mode() { + let mut settings = NetworkProxySettings { + enabled: true, + mode: NetworkMode::Full, + mitm: true, + mitm_hooks: vec![MitmHookConfig { + host: "api.github.com".to_string(), + matcher: MitmHookMatchConfig { + methods: vec!["POST".to_string()], + path_prefixes: vec!["/repos/openai/".to_string()], + ..MitmHookMatchConfig::default() + }, + ..MitmHookConfig::default() + }], + ..NetworkProxySettings::default() + }; + settings.set_allowed_domains(vec!["api.github.com".to_string()]); + let state = state_for_settings(settings); + let mut request = + TcpRequest::new(HostWithPort::try_from("api.github.com:80").expect("valid authority")); + request.extensions_mut().insert(state.clone()); + + let err = handle_socks5_tcp( + request, + TargetCheckedTcpConnector::new(state), + /*policy_decider*/ None, + ) + .await + .expect_err("hooked non-HTTPS SOCKS should require MITM"); + + assert!( + format!("{err:?}").contains("MITM required"), + "unexpected error: {err:?}" + ); + } + #[tokio::test(flavor = "current_thread")] async fn inspect_socks5_udp_emits_block_decision_for_mode_guard_deny() { let state = state_for_settings(NetworkProxySettings { From 36bf63a5cf02dafb1574b2396597ed1ef16bb5d7 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Tue, 9 Jun 2026 13:23:31 -0700 Subject: [PATCH 2/3] Allow creating a new goal after completion (#26681) ## Why Users have indicated that they want an agent to be able to create a new goal for itself after completing the previous goal. Currently, that's not possible because agents cannot overwrite an existing goal even if it's complete. This PR removes this limitation and allows `create_goal` to overwrite an existing goal if it is in the `complete` state. ## What changed `create_goal` now replaces the existing goal only when its status is `complete`. The replacement is performed atomically in the goal store, creates a fresh active goal with reset usage, and continues to reject creation while any unfinished goal exists. App server clients see a single `thread/goal/updated` event when the previous goal is replaced with the new one. The tool description and error message now reflect these semantics. ## What didn't change Agents are not allowed to create a new goal (overwrite their existing goal) if an existing goal is still active, blocked, paused, or in any other state other than "completed". --- codex-rs/ext/goal/src/spec.rs | 4 +-- codex-rs/ext/goal/src/tool.rs | 2 +- .../ext/goal/tests/goal_extension_backend.rs | 25 +++++++++++++++++-- codex-rs/state/src/runtime/goals.rs | 11 +++++++- 4 files changed, 36 insertions(+), 6 deletions(-) diff --git a/codex-rs/ext/goal/src/spec.rs b/codex-rs/ext/goal/src/spec.rs index 2c92c03848..6b97a24e9c 100644 --- a/codex-rs/ext/goal/src/spec.rs +++ b/codex-rs/ext/goal/src/spec.rs @@ -27,7 +27,7 @@ pub fn create_create_goal_tool() -> ToolSpec { ( "objective".to_string(), JsonSchema::string(Some( - "Required. The concrete objective to start pursuing. This starts a new active goal only when no goal is currently defined; if a goal already exists, this tool fails." + "Required. The concrete objective to start pursuing. This starts a new active goal when no goal exists or replaces the current goal when it is complete." .to_string(), )), ), @@ -44,7 +44,7 @@ pub fn create_create_goal_tool() -> ToolSpec { name: CREATE_GOAL_TOOL_NAME.to_string(), description: format!( r#"Create a goal only when explicitly requested by the user or system/developer instructions; do not infer goals from ordinary tasks. -Set token_budget only when an explicit token budget is requested. Fails if a goal exists; use {UPDATE_GOAL_TOOL_NAME} only for status."# +Set token_budget only when an explicit token budget is requested. Fails if an unfinished goal exists; use {UPDATE_GOAL_TOOL_NAME} only for status."# ), strict: false, defer_loading: None, diff --git a/codex-rs/ext/goal/src/tool.rs b/codex-rs/ext/goal/src/tool.rs index 17947aa343..8c0f513004 100644 --- a/codex-rs/ext/goal/src/tool.rs +++ b/codex-rs/ext/goal/src/tool.rs @@ -191,7 +191,7 @@ impl GoalToolExecutor { .map_err(|err| FunctionCallError::RespondToModel(format!("failed to create goal: {err}")))? .ok_or_else(|| { FunctionCallError::RespondToModel( - "cannot create a new goal because this thread already has a goal; use update_goal only when the existing goal is complete" + "cannot create a new goal because this thread has an unfinished goal; complete the existing goal first" .to_string(), ) })?; diff --git a/codex-rs/ext/goal/tests/goal_extension_backend.rs b/codex-rs/ext/goal/tests/goal_extension_backend.rs index f89a08ebbb..b922f5332d 100644 --- a/codex-rs/ext/goal/tests/goal_extension_backend.rs +++ b/codex-rs/ext/goal/tests/goal_extension_backend.rs @@ -124,7 +124,7 @@ async fn goal_tools_hidden_for_review_subagents() -> anyhow::Result<()> { } #[tokio::test] -async fn installed_goal_tools_reject_duplicate_goal_creation() -> anyhow::Result<()> { +async fn installed_goal_tools_only_replace_complete_goal() -> anyhow::Result<()> { let runtime = test_runtime().await?; let thread_id = test_thread_id()?; seed_thread_metadata(runtime.as_ref(), thread_id).await?; @@ -152,10 +152,31 @@ async fn installed_goal_tools_reject_duplicate_goal_creation() -> anyhow::Result assert_eq!( err, FunctionCallError::RespondToModel( - "cannot create a new goal because this thread already has a goal; use update_goal only when the existing goal is complete" + "cannot create a new goal because this thread has an unfinished goal; complete the existing goal first" .to_string() ) ); + + let update_tool = tool_by_name(&tools, "update_goal"); + update_tool + .handle(tool_call( + "update_goal", + "call-complete-goal", + json!({ "status": "complete" }), + )) + .await?; + + let invocation = tool_call( + "create_goal", + "call-create-goal-3", + json!({ "objective": "replacement goal" }), + ); + let output = create_tool.handle(invocation.clone()).await?; + let result = output.code_mode_result(&invocation.payload); + + assert_eq!(json!("replacement goal"), result["goal"]["objective"]); + assert_eq!(json!("active"), result["goal"]["status"]); + assert_eq!(json!(0), result["goal"]["tokensUsed"]); Ok(()) } diff --git a/codex-rs/state/src/runtime/goals.rs b/codex-rs/state/src/runtime/goals.rs index aa0e90462b..50e8cf068e 100644 --- a/codex-rs/state/src/runtime/goals.rs +++ b/codex-rs/state/src/runtime/goals.rs @@ -141,7 +141,16 @@ INSERT INTO thread_goals ( created_at_ms, updated_at_ms ) VALUES (?, ?, ?, ?, ?, 0, 0, ?, ?) -ON CONFLICT(thread_id) DO NOTHING +ON CONFLICT(thread_id) DO UPDATE SET + goal_id = excluded.goal_id, + objective = excluded.objective, + status = excluded.status, + token_budget = excluded.token_budget, + tokens_used = 0, + time_used_seconds = 0, + created_at_ms = excluded.created_at_ms, + updated_at_ms = excluded.updated_at_ms +WHERE thread_goals.status = 'complete' RETURNING thread_id, goal_id, From 8e69d29521488506182d2b5814851cdbb90c1354 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Tue, 9 Jun 2026 13:26:00 -0700 Subject: [PATCH 3/3] Reduce TUI legacy core dependencies (#26711) ## Why The TUI still reached through `app-server-client::legacy_core` for thread-name normalization and project-instruction filename details. In particular, checking the TUI's local filesystem for `/init` is incorrect for remote app-server sessions, where the server owns the working directory and instruction discovery. ## What changed - use the instruction source paths supplied by the app server to decide whether `/init` should avoid overwriting project instructions - keep the small thread-name normalization helper local to the TUI - remove the now-unused instruction filename constants, utility module, and other unused `legacy_core` re-exports - make status helper tests independent of concrete instruction filenames ## Verification - `just test -p codex-app-server-client` - `just test -p codex-tui slash_init_skips_when_project_instructions_are_loaded` - `just test -p codex-tui` ran 2,799 tests; 2,797 passed and two unrelated guardian feature-flag tests failed reproducibly in untouched code ### Manual test Started an app server over WebSocket with a remote workspace containing `AGENTS.md`, then connected the TUI using `--remote`. After confirming `thread/start` returned the file in `instructionSources`, deleted `AGENTS.md` and ran `/init` in the existing session. The TUI still reported that project instructions already existed and skipped `/init`. The trace contained no `turn/start` request, confirming the decision came from app-server session state rather than a new client-local filesystem check. --- codex-rs/app-server-client/src/lib.rs | 20 ------------ codex-rs/tui/prompt_for_init_command.md | 1 + codex-rs/tui/src/chatwidget.rs | 6 +++- codex-rs/tui/src/chatwidget/interaction.rs | 2 +- codex-rs/tui/src/chatwidget/slash_dispatch.rs | 10 +----- .../src/chatwidget/tests/slash_commands.rs | 31 +++---------------- codex-rs/tui/src/status/helpers.rs | 12 +++---- 7 files changed, 18 insertions(+), 64 deletions(-) diff --git a/codex-rs/app-server-client/src/lib.rs b/codex-rs/app-server-client/src/lib.rs index c75915629c..11684c6360 100644 --- a/codex-rs/app-server-client/src/lib.rs +++ b/codex-rs/app-server-client/src/lib.rs @@ -70,13 +70,9 @@ pub use crate::remote::RemoteAppServerEndpoint; /// module exists so clients can remove a direct `codex-core` dependency /// while legacy startup/config paths are migrated to RPCs. pub mod legacy_core { - pub use codex_core::DEFAULT_AGENTS_MD_FILENAME; - pub use codex_core::LOCAL_AGENTS_MD_FILENAME; - pub use codex_core::McpManager; pub use codex_core::check_execpolicy_for_warnings; pub use codex_core::format_exec_policy_error_with_source; pub use codex_core::grant_read_root_non_elevated; - pub use codex_core::web_search_detail; pub mod config { pub use codex_core::config::*; @@ -86,10 +82,6 @@ pub mod legacy_core { } } - pub mod connectors { - pub use codex_core::connectors::*; - } - pub mod otel_init { pub use codex_core::otel_init::*; } @@ -98,22 +90,10 @@ pub mod legacy_core { pub use codex_core::personality_migration::*; } - pub mod review_format { - pub use codex_core::review_format::*; - } - - pub mod review_prompts { - pub use codex_core::review_prompts::*; - } - pub mod test_support { pub use codex_core::test_support::*; } - pub mod util { - pub use codex_core::util::*; - } - pub mod windows_sandbox { pub use codex_core::windows_sandbox::*; } diff --git a/codex-rs/tui/prompt_for_init_command.md b/codex-rs/tui/prompt_for_init_command.md index b8fd3886b3..e96b002df7 100644 --- a/codex-rs/tui/prompt_for_init_command.md +++ b/codex-rs/tui/prompt_for_init_command.md @@ -1,4 +1,5 @@ Generate a file named AGENTS.md that serves as a contributor guide for this repository. +Before writing, check whether AGENTS.md already exists in the current working directory. If it does, do not overwrite or modify it. Your goal is to produce a clear, concise, and well-structured document with descriptive headings and actionable explanations for each section. Follow the outline below, but adapt as needed — add sections if relevant, and omit those that do not apply to this project. diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index c9827a4e71..6fd95e8dd5 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -58,7 +58,6 @@ use crate::bottom_pane::TerminalTitleItem; use crate::bottom_pane::TerminalTitleSetupView; use crate::diff_model::FileChange; use crate::git_action_directives::parse_assistant_markdown; -use crate::legacy_core::DEFAULT_AGENTS_MD_FILENAME; use crate::legacy_core::config::Config; use crate::legacy_core::config::Constrained; use crate::legacy_core::config::ConstraintResult; @@ -257,6 +256,11 @@ fn queued_message_edit_hint_binding( .or_else(|| bindings.first().copied()) } +fn normalize_thread_name(name: &str) -> Option { + let trimmed = name.trim(); + (!trimmed.is_empty()).then(|| trimmed.to_string()) +} + use crate::app_event::AppEvent; use crate::app_event::ExitMode; use crate::app_event::PermissionProfileSelection; diff --git a/codex-rs/tui/src/chatwidget/interaction.rs b/codex-rs/tui/src/chatwidget/interaction.rs index 7a3385034d..49fdd438f8 100644 --- a/codex-rs/tui/src/chatwidget/interaction.rs +++ b/codex-rs/tui/src/chatwidget/interaction.rs @@ -302,7 +302,7 @@ impl ChatWidget { /*initial_text*/ existing_name.unwrap_or_default().to_string(), /*context_label*/ None, Box::new(move |name: String| { - let Some(name) = crate::legacy_core::util::normalize_thread_name(&name) else { + let Some(name) = normalize_thread_name(&name) else { tx.send(AppEvent::InsertHistoryCell(Box::new( history_cell::new_error_event("Thread name cannot be empty.".to_string()), ))); diff --git a/codex-rs/tui/src/chatwidget/slash_dispatch.rs b/codex-rs/tui/src/chatwidget/slash_dispatch.rs index 48caf8f54d..e7776a3007 100644 --- a/codex-rs/tui/src/chatwidget/slash_dispatch.rs +++ b/codex-rs/tui/src/chatwidget/slash_dispatch.rs @@ -213,14 +213,6 @@ impl ChatWidget { .send(AppEvent::OpenDesktopThread { thread_id }); } SlashCommand::Init => { - let init_target = self.config.cwd.join(DEFAULT_AGENTS_MD_FILENAME); - if init_target.exists() { - let message = format!( - "{DEFAULT_AGENTS_MD_FILENAME} already exists here. Skipping /init to avoid overwriting it." - ); - self.add_info_message(message, /*hint*/ None); - return; - } const INIT_PROMPT: &str = include_str!("../../prompt_for_init_command.md"); self.submit_user_message(INIT_PROMPT.to_string().into()); } @@ -662,7 +654,7 @@ impl ChatWidget { } self.session_telemetry .counter("codex.thread.rename", /*inc*/ 1, &[]); - let Some(name) = crate::legacy_core::util::normalize_thread_name(&args) else { + let Some(name) = normalize_thread_name(&args) else { self.add_error_message("Thread name cannot be empty.".to_string()); return; }; diff --git a/codex-rs/tui/src/chatwidget/tests/slash_commands.rs b/codex-rs/tui/src/chatwidget/tests/slash_commands.rs index 9c2802fb60..02faef89f4 100644 --- a/codex-rs/tui/src/chatwidget/tests/slash_commands.rs +++ b/codex-rs/tui/src/chatwidget/tests/slash_commands.rs @@ -565,35 +565,14 @@ async fn ctrl_d_with_modal_open_does_not_quit() { } #[tokio::test] -async fn slash_init_skips_when_project_doc_exists() { - let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await; - let tempdir = tempdir().unwrap(); - let existing_path = tempdir.path().join(DEFAULT_AGENTS_MD_FILENAME); - std::fs::write(&existing_path, "existing instructions").unwrap(); - chat.config.cwd = tempdir.path().to_path_buf().abs(); +async fn slash_init_does_not_depend_on_loaded_instruction_sources() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + chat.instruction_source_paths = vec![chat.config.cwd.join("project-instructions.md")]; submit_composer_text(&mut chat, "/init"); - match op_rx.try_recv() { - Err(TryRecvError::Empty) => {} - other => panic!("expected no Codex op to be sent, got {other:?}"), - } - - let cells = drain_insert_history(&mut rx); - assert_eq!(cells.len(), 1, "expected one info message"); - let rendered = lines_to_single_string(&cells[0]); - assert!( - rendered.contains(DEFAULT_AGENTS_MD_FILENAME), - "info message should mention the existing file: {rendered:?}" - ); - assert!( - rendered.contains("Skipping /init"), - "info message should explain why /init was skipped: {rendered:?}" - ); - assert_eq!( - std::fs::read_to_string(existing_path).unwrap(), - "existing instructions" - ); + assert_eq!(chat.input_queue.queued_user_messages.len(), 1); + assert!(drain_insert_history(&mut rx).is_empty()); assert_eq!(recall_latest_after_clearing(&mut chat), "/init"); } diff --git a/codex-rs/tui/src/status/helpers.rs b/codex-rs/tui/src/status/helpers.rs index 27292e1364..f3ceeb435f 100644 --- a/codex-rs/tui/src/status/helpers.rs +++ b/codex-rs/tui/src/status/helpers.rs @@ -185,8 +185,6 @@ fn title_case(s: &str) -> String { #[cfg(test)] mod tests { use super::*; - use crate::legacy_core::DEFAULT_AGENTS_MD_FILENAME; - use crate::legacy_core::LOCAL_AGENTS_MD_FILENAME; use crate::legacy_core::config::ConfigBuilder; use codex_utils_absolute_path::test_support::PathBufExt; use pretty_assertions::assert_eq; @@ -227,7 +225,7 @@ mod tests { async fn compose_agents_summary_includes_global_agents_path() { let codex_home = TempDir::new().expect("temp codex home"); let cwd = TempDir::new().expect("temp cwd"); - let global_agents_path = codex_home.path().join(DEFAULT_AGENTS_MD_FILENAME); + let global_agents_path = codex_home.path().join("global.md"); let config = test_config(&codex_home, &cwd).await; assert_eq!( @@ -240,7 +238,7 @@ mod tests { async fn compose_agents_summary_names_global_agents_override() { let codex_home = TempDir::new().expect("temp codex home"); let cwd = TempDir::new().expect("temp cwd"); - let override_path = codex_home.path().join(LOCAL_AGENTS_MD_FILENAME); + let override_path = codex_home.path().join("override.md"); let config = test_config(&codex_home, &cwd).await; assert_eq!( @@ -253,8 +251,8 @@ mod tests { async fn compose_agents_summary_orders_global_before_project_agents() { let codex_home = TempDir::new().expect("temp codex home"); let cwd = TempDir::new().expect("temp cwd"); - let global_agents_path = codex_home.path().join(DEFAULT_AGENTS_MD_FILENAME); - let project_agents_path = cwd.path().join(DEFAULT_AGENTS_MD_FILENAME); + let global_agents_path = codex_home.path().join("global.md"); + let project_agents_path = cwd.path().join("project.md"); let config = test_config(&codex_home, &cwd).await; let summary = compose_agents_summary( @@ -270,7 +268,7 @@ mod tests { Some(format_directory_display(&global_agents_path, /*max_width*/ None).as_str()) ); let project_path = paths.next().expect("project agents path"); - assert!(project_path.ends_with(DEFAULT_AGENTS_MD_FILENAME)); + assert!(project_path.ends_with("project.md")); assert_eq!(paths.next(), None); } }