From a9ee7fec09e51f4fdb127a206d4532e1d156e75e Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Tue, 7 Jul 2026 15:27:54 -0300 Subject: [PATCH] fix(terminal-browser): approve human navigation network access --- codex-rs/core/src/session/handlers.rs | 7 +- codex-rs/core/src/session/mod.rs | 37 +++++- codex-rs/core/src/tools/network_approval.rs | 36 +++++- .../core/src/tools/network_approval_tests.rs | 33 +++++ codex-rs/core/tests/suite/network_approval.rs | 116 +++++++++++++++++- 5 files changed, 219 insertions(+), 10 deletions(-) diff --git a/codex-rs/core/src/session/handlers.rs b/codex-rs/core/src/session/handlers.rs index a483ce5e0f..5ce5e89298 100644 --- a/codex-rs/core/src/session/handlers.rs +++ b/codex-rs/core/src/session/handlers.rs @@ -396,7 +396,12 @@ pub async fn exec_approval( } match decision { ReviewDecision::Abort => { - sess.interrupt_task().await; + if !sess + .notify_out_of_turn_network_approval(&approval_id, ReviewDecision::Abort) + .await + { + sess.interrupt_task().await; + } } other => sess.notify_approval(&approval_id, other).await, } diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 4247d95401..0cfeeb20f3 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -2187,17 +2187,30 @@ impl Session { let effective_approval_id = approval_id.clone().unwrap_or_else(|| call_id.clone()); // Add the tx_approve callback to the map before sending the request. let (tx_approve, rx_approve) = oneshot::channel(); + let mut tx_approve = Some(tx_approve); let prev_entry = { let mut active = self.active_turn.lock().await; match active.as_mut() { Some(at) => { let mut ts = at.turn_state.lock().await; - ts.insert_pending_approval(effective_approval_id.clone(), tx_approve) + tx_approve.take().and_then(|tx_approve| { + ts.insert_pending_approval(effective_approval_id.clone(), tx_approve) + }) } None => None, } }; - if prev_entry.is_some() { + let previous_out_of_turn = if network_approval_context.is_some() + && let Some(tx_approve) = tx_approve + { + self.services + .network_approval + .insert_out_of_turn_approval(effective_approval_id.clone(), tx_approve) + .await + } else { + None + }; + if prev_entry.is_some() || previous_out_of_turn.is_some() { warn!("Overwriting existing pending approval for call_id: {effective_approval_id}"); } @@ -2755,11 +2768,29 @@ impl Session { tx_approve.send(decision).ok(); } None => { - warn!("No pending approval found for call_id: {approval_id}"); + if !self + .services + .network_approval + .notify_out_of_turn_approval(approval_id, decision) + .await + { + warn!("No pending approval found for call_id: {approval_id}"); + } } } } + pub(crate) async fn notify_out_of_turn_network_approval( + &self, + approval_id: &str, + decision: ReviewDecision, + ) -> bool { + self.services + .network_approval + .notify_out_of_turn_approval(approval_id, decision) + .await + } + /// Records conversation items: append to history, persist to rollout, and /// notify clients observing raw response items. pub(crate) fn prepare_conversation_items_for_history<'a>( diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index 2b63164b4d..928f4c1ab5 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -35,6 +35,7 @@ use tokio::sync::Mutex; use tokio::sync::Notify; use tokio::sync::OnceCell; use tokio::sync::RwLock; +use tokio::sync::oneshot; use tokio_util::sync::CancellationToken; use tracing::warn; use uuid::Uuid; @@ -250,6 +251,7 @@ struct NetworkApprovalCallState { pub(crate) struct NetworkApprovalService { calls: Mutex, pending_host_approvals: Mutex>>, + pending_out_of_turn_approvals: Mutex>>, session_approved_hosts: Mutex>, session_denied_hosts: Mutex>, } @@ -259,6 +261,7 @@ impl Default for NetworkApprovalService { Self { calls: Mutex::new(NetworkApprovalCallState::default()), pending_host_approvals: Mutex::new(HashMap::new()), + pending_out_of_turn_approvals: Mutex::new(HashMap::new()), session_approved_hosts: Mutex::new(HashSet::new()), session_denied_hosts: Mutex::new(HashSet::new()), } @@ -266,6 +269,30 @@ impl Default for NetworkApprovalService { } impl NetworkApprovalService { + pub(crate) async fn insert_out_of_turn_approval( + &self, + approval_id: String, + sender: oneshot::Sender, + ) -> Option> { + self.pending_out_of_turn_approvals + .lock() + .await + .insert(approval_id, sender) + } + + pub(crate) async fn notify_out_of_turn_approval( + &self, + approval_id: &str, + decision: ReviewDecision, + ) -> bool { + let sender = self + .pending_out_of_turn_approvals + .lock() + .await + .remove(approval_id); + sender.is_some_and(|sender| sender.send(decision).is_ok()) + } + /// Replace the target session's approval cache with the source session's /// currently approved hosts. pub(crate) async fn sync_session_approved_hosts_to(&self, other: &Self) { @@ -453,7 +480,14 @@ impl NetworkApprovalService { } } }; - let turn_context = Self::active_turn_context(session.as_ref()).await; + let turn_context = match Self::active_turn_context(session.as_ref()).await { + Some(turn_context) => Some(turn_context), + // The untagged proxy listener is exposed to trusted host clients such as the + // terminal browser. Environment-tagged listeners belong to sandboxed commands and + // remain fail-closed after their originating turn ends. + None if request.environment_id.is_none() => Some(session.new_default_turn().await), + None => None, + }; let Some(environment_id) = active_environment_id.or_else(|| { turn_context .as_ref() diff --git a/codex-rs/core/src/tools/network_approval_tests.rs b/codex-rs/core/src/tools/network_approval_tests.rs index 7a9ba86cef..5add4d966b 100644 --- a/codex-rs/core/src/tools/network_approval_tests.rs +++ b/codex-rs/core/src/tools/network_approval_tests.rs @@ -234,6 +234,39 @@ async fn pending_waiters_receive_owner_decision() { assert_eq!(decision, PendingApprovalDecision::AllowOnce); } +#[tokio::test] +async fn out_of_turn_approval_receives_the_submitted_decision() { + let service = NetworkApprovalService::default(); + let (sender, receiver) = oneshot::channel(); + assert!( + service + .insert_out_of_turn_approval("approval-1".to_string(), sender) + .await + .is_none() + ); + + assert!( + service + .notify_out_of_turn_approval("approval-1", ReviewDecision::Approved) + .await + ); + assert_eq!( + receiver.await.expect("approval sender should remain open"), + ReviewDecision::Approved + ); +} + +#[tokio::test] +async fn out_of_turn_approval_rejects_unknown_ids() { + let service = NetworkApprovalService::default(); + + assert!( + !service + .notify_out_of_turn_approval("missing", ReviewDecision::Approved) + .await + ); +} + #[test] fn allow_once_and_allow_for_session_both_allow_network() { assert_eq!( diff --git a/codex-rs/core/tests/suite/network_approval.rs b/codex-rs/core/tests/suite/network_approval.rs index 6b96c0d635..e59f8cfacf 100644 --- a/codex-rs/core/tests/suite/network_approval.rs +++ b/codex-rs/core/tests/suite/network_approval.rs @@ -55,6 +55,100 @@ use tempfile::TempDir; const NETWORK_TEST_HOST: &str = "codex-network-test.invalid"; const NETWORK_TEST_TARGET: &str = "http://codex-network-test.invalid:80"; +const NETWORK_ABORT_HOST: &str = "codex-network-abort.invalid"; +const NETWORK_ABORT_TARGET: &str = "http://codex-network-abort.invalid:80"; + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn host_proxy_request_can_be_approved_without_an_active_turn() -> Result<()> { + skip_if_target_windows!(Ok(()), "uses a local managed-network proxy"); + skip_if_host_windows!(Ok(())); + skip_if_no_network!(Ok(())); + skip_if_sandbox!(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 permission_profile = PermissionProfile::workspace_write_with( + &[], + NetworkSandboxPolicy::Enabled, + /*exclude_tmpdir_env_var*/ false, + /*exclude_slash_tmp*/ false, + ); + let test = test_codex() + .with_home(home) + .with_cloud_config_bundle(managed_network_requirements_loader()) + .with_config(move |config| { + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); + config.approvals_reviewer = ApprovalsReviewer::User; + config + .permissions + .set_permission_profile(permission_profile) + .expect("set permission profile"); + }) + .build_with_auto_env(&server) + .await?; + let http_addr = test + .session_configured + .network_proxy + .as_ref() + .context("expected runtime managed network proxy addresses")? + .http_addr + .clone(); + let client = reqwest::Client::builder() + .proxy(reqwest::Proxy::http(format!("http://{http_addr}"))?) + .build()?; + let request_client = client.clone(); + let request = tokio::spawn(async move { request_client.get(NETWORK_TEST_TARGET).send().await }); + + let approval = expect_network_approval(&test, LOCAL_ENVIRONMENT_ID).await?; + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: Some(approval.turn_id), + decision: ReviewDecision::Approved, + }) + .await?; + + tokio::time::timeout(Duration::from_secs(/*secs*/ 5), request) + .await + .context("host proxy request did not resume after approval")?? + .context("host proxy request failed before receiving a proxy response")?; + + let request = tokio::spawn(async move { client.get(NETWORK_ABORT_TARGET).send().await }); + let approval = expect_network_approval_for( + &test, + LOCAL_ENVIRONMENT_ID, + NETWORK_ABORT_HOST, + NETWORK_ABORT_TARGET, + ) + .await?; + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: Some(approval.turn_id), + decision: ReviewDecision::Abort, + }) + .await?; + let response = tokio::time::timeout(Duration::from_secs(/*secs*/ 5), request) + .await + .context("host proxy request did not resume after abort")?? + .context("host proxy request failed before receiving its denial")?; + assert_eq!(response.status(), reqwest::StatusCode::FORBIDDEN); + + Ok(()) +} #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn approved_network_host_for_one_environment_still_prompts_in_another() -> Result<()> { @@ -284,6 +378,21 @@ async fn submit_managed_network_turn( async fn expect_network_approval( test: &TestCodex, expected_environment_id: &str, +) -> Result { + expect_network_approval_for( + test, + expected_environment_id, + NETWORK_TEST_HOST, + NETWORK_TEST_TARGET, + ) + .await +} + +async fn expect_network_approval_for( + test: &TestCodex, + expected_environment_id: &str, + expected_host: &str, + expected_target: &str, ) -> Result { let deadline = std::time::Instant::now() + Duration::from_secs(30); let remaining = deadline @@ -304,15 +413,12 @@ async fn expect_network_approval( EventMsg::ExecApprovalRequest(approval) => { assert_eq!( approval.command, - vec![ - "network-access".to_string(), - NETWORK_TEST_TARGET.to_string() - ] + vec!["network-access".to_string(), expected_target.to_string()] ); assert_eq!( approval.network_approval_context, Some(NetworkApprovalContext { - host: NETWORK_TEST_HOST.to_string(), + host: expected_host.to_string(), protocol: NetworkApprovalProtocol::Http, }) );