From e2dbe7dfc37e1c368ac521429a82a7562ebaaca2 Mon Sep 17 00:00:00 2001 From: Won Park Date: Wed, 15 Apr 2026 17:02:42 -0700 Subject: [PATCH] removing network proxy for yolo (#17742) **Summary** - prevent managed requirements.toml network settings from leaking into DangerFullAccess / yolo turns by gating managed proxy attachment on sandbox mode - keep guardian/sandboxed modes on the managed proxy path, while making true yolo bypass the proxy entirely, including /shell full-access commands --- codex-rs/core/src/codex.rs | 28 +++- codex-rs/core/src/codex_tests.rs | 198 +++++++++++++++++++++++++ codex-rs/core/src/config/mod.rs | 6 +- codex-rs/core/src/tasks/user_shell.rs | 4 +- codex-rs/core/tests/suite/approvals.rs | 163 ++++++++++++++++++++ 5 files changed, 392 insertions(+), 7 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index c39b5bc71..66a0dc527 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1338,6 +1338,10 @@ impl Session { } } + fn managed_network_proxy_active_for_sandbox_policy(sandbox_policy: &SandboxPolicy) -> bool { + !matches!(sandbox_policy, SandboxPolicy::DangerFullAccess) + } + /// Builds the `x-codex-beta-features` header value for this session. /// /// `ModelClient` is session-scoped and intentionally does not depend on the full `Config`, so @@ -2004,10 +2008,15 @@ impl Session { .await; session_configuration.thread_name = thread_name.clone(); let state = SessionState::new(session_configuration.clone()); + let managed_network_requirements_configured = config + .config_layer_stack + .requirements_toml() + .network + .is_some(); let managed_network_requirements_enabled = config.managed_network_requirements_enabled(); let network_approval = Arc::new(NetworkApprovalService::default()); // The managed proxy can call back into core for allowlist-miss decisions. - let network_policy_decider_session = if managed_network_requirements_enabled { + let network_policy_decider_session = if managed_network_requirements_configured { config .permissions .network @@ -2016,7 +2025,7 @@ impl Session { } else { None }; - let blocked_request_observer = if managed_network_requirements_enabled { + let blocked_request_observer = if managed_network_requirements_configured { config .permissions .network @@ -2043,7 +2052,7 @@ impl Session { config.permissions.sandbox_policy.get(), network_policy_decider.as_ref().map(Arc::clone), blocked_request_observer.as_ref().map(Arc::clone), - managed_network_requirements_enabled, + managed_network_requirements_configured, network_proxy_audit_metadata, ) .instrument(info_span!( @@ -2200,7 +2209,11 @@ impl Session { history_log_id, history_entry_count, initial_messages, - network_proxy: session_network_proxy, + network_proxy: session_network_proxy.filter(|_| { + Self::managed_network_proxy_active_for_sandbox_policy( + session_configuration.sandbox_policy.get(), + ) + }), rollout_path, }), }) @@ -2734,7 +2747,12 @@ impl Session { self.services .network_proxy .as_ref() - .map(StartedNetworkProxy::proxy), + .and_then(|started_proxy| { + Self::managed_network_proxy_active_for_sandbox_policy( + session_configuration.sandbox_policy.get(), + ) + .then(|| started_proxy.proxy()) + }), self.services.environment.clone(), sub_id, Arc::clone(&self.js_repl), diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index 19c0917f6..054e98be6 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -45,6 +45,8 @@ use crate::rollout::recorder::RolloutRecorder; use crate::state::TaskKind; use crate::tasks::SessionTask; use crate::tasks::SessionTaskContext; +use crate::tasks::UserShellCommandMode; +use crate::tasks::execute_user_shell_command; use crate::tools::ToolRouter; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; @@ -740,6 +742,99 @@ async fn new_turn_refreshes_managed_network_proxy_for_sandbox_change() -> anyhow Ok(()) } +#[tokio::test] +async fn danger_full_access_turns_do_not_expose_managed_network_proxy() -> anyhow::Result<()> { + let network_spec = crate::config::NetworkProxySpec::from_config_and_constraints( + NetworkProxyConfig::default(), + Some(NetworkConstraints { + enabled: Some(true), + ..Default::default() + }), + &SandboxPolicy::DangerFullAccess, + )?; + + let session = make_session_with_config(move |config| { + config.permissions.sandbox_policy = + codex_config::Constrained::allow_any(SandboxPolicy::DangerFullAccess); + config.permissions.network = Some(network_spec); + }) + .await?; + + let turn_context = session.new_default_turn().await; + assert!(turn_context.network.is_none()); + Ok(()) +} + +#[tokio::test] +async fn workspace_write_turns_continue_to_expose_managed_network_proxy() -> anyhow::Result<()> { + let sandbox_policy = SandboxPolicy::new_workspace_write_policy(); + let network_spec = crate::config::NetworkProxySpec::from_config_and_constraints( + NetworkProxyConfig::default(), + Some(NetworkConstraints { + enabled: Some(true), + ..Default::default() + }), + &sandbox_policy, + )?; + + let session = make_session_with_config(move |config| { + config.permissions.sandbox_policy = codex_config::Constrained::allow_any(sandbox_policy); + config.permissions.network = Some(network_spec); + }) + .await?; + + let turn_context = session.new_default_turn().await; + assert!(turn_context.network.is_some()); + Ok(()) +} + +#[tokio::test] +async fn user_shell_commands_do_not_inherit_managed_network_proxy() -> anyhow::Result<()> { + let sandbox_policy = SandboxPolicy::new_workspace_write_policy(); + let network_spec = crate::config::NetworkProxySpec::from_config_and_constraints( + NetworkProxyConfig::default(), + Some(NetworkConstraints { + enabled: Some(true), + ..Default::default() + }), + &sandbox_policy, + )?; + + let (session, rx) = make_session_with_config_and_rx(move |config| { + config.permissions.sandbox_policy = codex_config::Constrained::allow_any(sandbox_policy); + config.permissions.network = Some(network_spec); + }) + .await?; + + let turn_context = session.new_default_turn().await; + assert!(turn_context.network.is_some()); + + #[cfg(windows)] + let command = r#"$val = $env:HTTP_PROXY; if ([string]::IsNullOrEmpty($val)) { $val = 'not-set' } ; [System.Console]::Write($val)"#.to_string(); + #[cfg(not(windows))] + let command = r#"sh -c "printf '%s' \"${HTTP_PROXY:-not-set}\"""#.to_string(); + + execute_user_shell_command( + Arc::clone(&session), + turn_context, + command, + CancellationToken::new(), + UserShellCommandMode::StandaloneTurn, + ) + .await; + + loop { + let event = rx.recv().await.expect("channel open"); + if let EventMsg::ExecCommandEnd(event) = event.msg { + assert_eq!(event.exit_code, 0); + assert_eq!(event.stdout.trim(), "not-set"); + break; + } + } + + Ok(()) +} + #[tokio::test] async fn get_base_instructions_no_user_content() { let prompt_with_apply_patch_instructions = @@ -3001,6 +3096,109 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { (session, turn_context) } +async fn make_session_with_config( + mutator: impl FnOnce(&mut Config), +) -> anyhow::Result> { + let (session, _rx_event) = make_session_with_config_and_rx(mutator).await?; + Ok(session) +} + +async fn make_session_with_config_and_rx( + mutator: impl FnOnce(&mut Config), +) -> anyhow::Result<(Arc, async_channel::Receiver)> { + let codex_home = tempfile::tempdir().expect("create temp dir"); + let mut config = build_test_config(codex_home.path()).await; + mutator(&mut config); + let config = Arc::new(config); + let auth_manager = AuthManager::from_auth_for_testing(CodexAuth::from_api_key("Test API Key")); + let models_manager = Arc::new(ModelsManager::new( + config.codex_home.to_path_buf(), + auth_manager.clone(), + /*model_catalog*/ None, + CollaborationModesConfig::default(), + )); + let model = ModelsManager::get_model_offline_for_tests(config.model.as_deref()); + let model_info = ModelsManager::construct_model_info_offline_for_tests( + model.as_str(), + &config.to_models_manager_config(), + ); + let collaboration_mode = CollaborationMode { + mode: ModeKind::Default, + settings: Settings { + model, + reasoning_effort: config.model_reasoning_effort, + developer_instructions: None, + }, + }; + let session_configuration = SessionConfiguration { + provider: config.model_provider.clone(), + collaboration_mode, + model_reasoning_summary: config.model_reasoning_summary, + developer_instructions: config.developer_instructions.clone(), + user_instructions: config.user_instructions.clone(), + service_tier: None, + personality: config.personality, + base_instructions: config + .base_instructions + .clone() + .unwrap_or_else(|| model_info.get_model_instructions(config.personality)), + compact_prompt: config.compact_prompt.clone(), + approval_policy: config.permissions.approval_policy.clone(), + approvals_reviewer: config.approvals_reviewer, + sandbox_policy: config.permissions.sandbox_policy.clone(), + file_system_sandbox_policy: config.permissions.file_system_sandbox_policy.clone(), + network_sandbox_policy: config.permissions.network_sandbox_policy, + windows_sandbox_level: WindowsSandboxLevel::from_config(&config), + cwd: config.cwd.clone(), + codex_home: config.codex_home.clone(), + thread_name: None, + original_config_do_not_use: Arc::clone(&config), + metrics_service_name: None, + app_server_client_name: None, + app_server_client_version: None, + session_source: SessionSource::Exec, + dynamic_tools: Vec::new(), + persist_extended_history: false, + inherited_shell_snapshot: None, + user_shell_override: None, + }; + + let (tx_event, rx_event) = async_channel::unbounded(); + let (agent_status_tx, _agent_status_rx) = watch::channel(AgentStatus::PendingInit); + let plugins_manager = Arc::new(PluginsManager::new(config.codex_home.to_path_buf())); + let mcp_manager = Arc::new(McpManager::new(Arc::clone(&plugins_manager))); + let skills_manager = Arc::new(SkillsManager::new( + config.codex_home.clone(), + /*bundled_skills_enabled*/ true, + )); + + let session = Session::new( + session_configuration, + Arc::clone(&config), + auth_manager, + models_manager, + Arc::new(ExecPolicyManager::default()), + tx_event, + agent_status_tx, + InitialHistory::New, + SessionSource::Exec, + skills_manager, + plugins_manager, + mcp_manager, + Arc::new(SkillsWatcher::noop()), + AgentControl::default(), + Some(Arc::new( + codex_exec_server::Environment::create(/*exec_server_url*/ None) + .await + .expect("create environment"), + )), + /*analytics_events_client*/ None, + ) + .await?; + + Ok((session, rx_event)) +} + #[tokio::test] async fn notify_request_permissions_response_ignores_unmatched_call_id() { let (session, _turn_context) = make_session_and_context().await; diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index b58acbf65..5faf4be48 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -2282,7 +2282,11 @@ impl Config { } pub fn managed_network_requirements_enabled(&self) -> bool { - self.config_layer_stack + !matches!( + self.permissions.sandbox_policy.get(), + SandboxPolicy::DangerFullAccess + ) && self + .config_layer_stack .requirements_toml() .network .is_some() diff --git a/codex-rs/core/src/tasks/user_shell.rs b/codex-rs/core/src/tasks/user_shell.rs index 76e531744..8b108eee0 100644 --- a/codex-rs/core/src/tasks/user_shell.rs +++ b/codex-rs/core/src/tasks/user_shell.rs @@ -163,7 +163,9 @@ pub(crate) async fn execute_user_shell_command( cwd: cwd.clone(), env: exec_env_map, exec_server_env_config: None, - network: turn_context.network.clone(), + // `/shell` is the explicit full-access escape hatch, so it must not + // inherit a managed proxy from the surrounding session or turn. + network: None, // TODO(zhao-oai): Now that we have ExecExpiration::Cancellation, we // should use that instead of an "arbitrarily large" timeout here. expiration: USER_SHELL_TIMEOUT_MS.into(), diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index 84066ff6e..c5f776019 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -2803,6 +2803,169 @@ allow_local_binding = true Ok(()) } +#[tokio::test(flavor = "current_thread")] +async fn network_approval_flow_survives_danger_full_access_session_start() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let home = Arc::new(TempDir::new()?); + fs::write( + home.path().join("config.toml"), + r#"default_permissions = "workspace" + +[permissions.workspace.filesystem] +":minimal" = "read" + +[permissions.workspace.network] +enabled = true +mode = "limited" +allow_local_binding = true +"#, + )?; + let approval_policy = AskForApproval::OnFailure; + let turn_sandbox_policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + read_only_access: Default::default(), + network_access: true, + exclude_tmpdir_env_var: false, + exclude_slash_tmp: false, + }; + let mut builder = test_codex().with_home(home).with_config(move |config| { + config.permissions.approval_policy = Constrained::allow_any(approval_policy); + config.permissions.sandbox_policy = Constrained::allow_any(SandboxPolicy::DangerFullAccess); + let layers = config + .config_layer_stack + .get_layers( + ConfigLayerStackOrdering::LowestPrecedenceFirst, + /*include_disabled*/ true, + ) + .into_iter() + .cloned() + .collect(); + let mut requirements = config.config_layer_stack.requirements().clone(); + requirements.network = Some(Sourced::new( + NetworkConstraints { + enabled: Some(true), + allow_local_binding: Some(true), + ..Default::default() + }, + RequirementSource::CloudRequirements, + )); + let mut requirements_toml = config.config_layer_stack.requirements_toml().clone(); + requirements_toml.network = Some(NetworkRequirementsToml { + enabled: Some(true), + allow_local_binding: Some(true), + ..Default::default() + }); + config.config_layer_stack = ConfigLayerStack::new(layers, requirements, requirements_toml) + .expect("rebuild config layer stack with network requirements"); + }); + let test = builder.build(&server).await?; + assert!( + !test.config.managed_network_requirements_enabled(), + "expected managed network requirements to stay inactive in danger-full-access" + ); + assert!( + test.config.permissions.network.is_some(), + "expected managed network proxy config to be present" + ); + assert!( + test.session_configured.network_proxy.is_none(), + "expected session configured event to hide managed network proxy in danger-full-access" + ); + + let call_id = "allow-network-after-yolo"; + let fetch_command = r#"python3 -c "import urllib.request; opener = urllib.request.build_opener(urllib.request.ProxyHandler()); print('OK:' + opener.open('http://codex-network-test.invalid', timeout=30).read().decode(errors='replace'))""# + .to_string(); + let event = shell_event( + call_id, + &fetch_command, + /*timeout_ms*/ 30_000, + SandboxPermissions::UseDefault, + )?; + + let _ = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-network-after-yolo-1"), + event, + ev_completed("resp-network-after-yolo-1"), + ]), + ) + .await; + let _ = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-network-after-yolo-1", "done"), + ev_completed("resp-network-after-yolo-2"), + ]), + ) + .await; + + submit_turn( + &test, + "allow-network-after-yolo", + approval_policy, + turn_sandbox_policy, + ) + .await?; + + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(30); + let approval = loop { + let remaining = deadline + .checked_duration_since(std::time::Instant::now()) + .expect("timed out waiting for network approval request"); + let event = wait_for_event_with_timeout( + &test.codex, + |event| { + matches!( + event, + EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_) + ) + }, + remaining, + ) + .await; + match event { + EventMsg::ExecApprovalRequest(approval) => { + if approval.command.first().map(std::string::String::as_str) + == Some("network-access") + { + break approval; + } + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::Approved, + }) + .await?; + } + EventMsg::TurnComplete(_) => { + panic!("expected network approval request before completion"); + } + other => panic!("unexpected event: {other:?}"), + } + }; + + let network_context = approval + .network_approval_context + .clone() + .expect("expected network approval context"); + assert_eq!(network_context.protocol, NetworkApprovalProtocol::Http); + + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::Denied, + }) + .await?; + wait_for_completion(&test).await; + + Ok(()) +} + // todo(dylan) add ScenarioSpec support for rules #[tokio::test(flavor = "current_thread")] #[cfg(unix)]