diff --git a/codex-rs/core/src/tools/approvals.rs b/codex-rs/core/src/tools/approvals.rs index 05b687a6f7..4f5512482b 100644 --- a/codex-rs/core/src/tools/approvals.rs +++ b/codex-rs/core/src/tools/approvals.rs @@ -1,10 +1,13 @@ //! Central approval policy-stage execution and reviewer routing. use crate::command_canonicalization::canonicalize_command_for_approval; +use crate::guardian::GuardianNetworkAccessTrigger; use crate::guardian::GuardianReviewContext; +use crate::guardian::GuardianReviewOptions; use crate::guardian::guardian_timeout_message; use crate::guardian::new_guardian_review_id; use crate::guardian::review_approval_request; +use crate::guardian::review_approval_request_with_cancel; use crate::guardian::routes_approval_policy_to_guardian; use crate::hook_runtime::run_permission_request_hooks; use crate::mcp_tool_call::request_mcp_tool_user_approval; @@ -20,6 +23,7 @@ use crate::tools::sandboxing::ApprovalRequestReasons; use crate::tools::sandboxing::PermissionRequestPayload; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::with_cached_approval; +use codex_analytics::GuardianApprovalRequestSource; use codex_config::types::AppToolApproval; use codex_hooks::PermissionRequestDecision; use codex_otel::ToolDecisionSource; @@ -27,6 +31,7 @@ use codex_protocol::approvals::ExecPolicyAmendment; #[cfg(unix)] use codex_protocol::approvals::GuardianCommandSource; use codex_protocol::approvals::NetworkApprovalContext; +use codex_protocol::approvals::NetworkApprovalProtocol; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::error::CodexErr; use codex_protocol::models::AdditionalPermissionProfile; @@ -40,7 +45,9 @@ use codex_utils_path_uri::PathUri; use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; +use tokio_util::sync::CancellationToken; use tracing::error; +use tracing::warn; #[derive(Clone)] pub(crate) struct ApprovalContext { @@ -118,6 +125,20 @@ pub(crate) enum ApprovalAction { allow_session_remember: bool, allow_persistent_approval: bool, }, + NetworkAccess { + id: String, + turn_id: String, + environment_id: String, + target: String, + host: String, + protocol: NetworkApprovalProtocol, + port: u16, + trigger: Option, + hook_command: String, + hook_run_id: String, + command: Vec, + cwd: AbsolutePathBuf, + }, } #[derive(Clone, Debug, Eq, Hash, PartialEq, serde::Serialize)] @@ -160,6 +181,14 @@ impl ApprovalAction { .clone() .unwrap_or_else(|| serde_json::Value::Object(serde_json::Map::new())), }, + Self::NetworkAccess { + hook_command, + target, + .. + } => PermissionRequestPayload::bash( + hook_command.clone(), + Some(format!("network-access {target}")), + ), } } @@ -197,7 +226,7 @@ impl ApprovalAction { })], #[cfg(unix)] Self::Execve { .. } => Vec::new(), - Self::McpToolCall { .. } => Vec::new(), + Self::McpToolCall { .. } | Self::NetworkAccess { .. } => Vec::new(), Self::ApplyPatch { environment_id, files, @@ -312,6 +341,24 @@ impl ApprovalAction { tool_description, annotations, }, + Self::NetworkAccess { + id, + turn_id, + target, + host, + protocol, + port, + trigger, + .. + } => crate::guardian::GuardianApprovalRequest::NetworkAccess { + id, + turn_id, + target, + host, + protocol, + port, + trigger, + }, }) } } @@ -408,9 +455,11 @@ impl Session { ctx: ApprovalContext, ) -> Result { let is_mcp_tool_call = matches!(&action, ApprovalAction::McpToolCall { .. }); + let is_network_approval = matches!(&action, ApprovalAction::NetworkAccess { .. }); let permission_request_run_id = match &action { #[cfg(unix)] ApprovalAction::Execve { approval_id, .. } => approval_id.clone(), + ApprovalAction::NetworkAccess { hook_run_id, .. } => hook_run_id.clone(), _ if ctx.retry_reason.is_some() => format!("{}:retry", ctx.call_id), _ => ctx.call_id.clone(), }; @@ -436,10 +485,31 @@ impl Session { }, None => self.request_reviewer_approval(action, &ctx).await, }; - record_resolution(&ctx, &resolution); + // Network approvals record their final telemetry after validation and persistence. + if !is_network_approval { + record_resolution(&ctx, &resolution); + } if is_mcp_tool_call && resolution.decision == ReviewDecision::ApprovedMcpPolicyAmendment { return Ok(resolution.decision); } + if is_network_approval { + match (&resolution.decision, resolution.source) { + ( + ReviewDecision::NetworkPolicyAmendment { + network_policy_amendment, + }, + _, + ) if network_policy_amendment.action == NetworkPolicyRuleAction::Deny => { + return Ok(resolution.decision); + } + (ReviewDecision::Abort, ApprovalResolutionSource::Guardian) => { + return Err(ToolError::Rejected( + "automatic approval review was cancelled".to_string(), + )); + } + _ => {} + } + } resolution.into_tool_result() } @@ -477,6 +547,7 @@ impl Session { action: ApprovalAction, ctx: &ApprovalContext, ) -> ReviewDecision { + let is_network_approval = matches!(&action, ApprovalAction::NetworkAccess { .. }); let review_id = new_guardian_review_id(); let action = match action.into_guardian_request() { Ok(action) => action, @@ -488,17 +559,46 @@ impl Session { } }; - review_approval_request( - self, - ctx.review_context.clone(), - review_id, - action, - ApprovalRequestReasons { - approval: ctx.approval_reason.clone(), - retry: ctx.retry_reason.clone(), - }, - ) - .await + if is_network_approval { + let review_cancel = CancellationToken::new(); + let review_cancel_guard = review_cancel.clone().drop_guard(); + let review_session = Arc::clone(self); + let review_context = ctx.review_context.clone(); + let retry_reason = ctx.retry_reason.clone(); + let review = tokio::spawn(async move { + review_approval_request_with_cancel( + &review_session, + review_context, + review_id, + action, + retry_reason, + GuardianReviewOptions { + plugin_attribution_override: None, + approval_request_source: GuardianApprovalRequestSource::MainTurn, + external_cancel: Some(review_cancel), + }, + ) + .await + }); + let decision = review.await.unwrap_or_else(|err| { + warn!("network Guardian review task failed: {err}"); + ReviewDecision::denied("automatic approval review could not complete") + }); + drop(review_cancel_guard.disarm()); + decision + } else { + review_approval_request( + self, + ctx.review_context.clone(), + review_id, + action, + ApprovalRequestReasons { + approval: ctx.approval_reason.clone(), + retry: ctx.retry_reason.clone(), + }, + ) + .await + } } async fn request_user_approval( @@ -540,7 +640,9 @@ impl Session { #[cfg(unix)] ApprovalAction::Execve { .. } => unreachable!("matched command approval"), ApprovalAction::ApplyPatch { .. } => unreachable!("matched command approval"), - ApprovalAction::McpToolCall { .. } => unreachable!("matched command approval"), + ApprovalAction::McpToolCall { .. } | ApprovalAction::NetworkAccess { .. } => { + unreachable!("matched command approval") + } }; let reason = ctx .retry_reason @@ -640,6 +742,28 @@ impl Session { ) .await } + ApprovalAction::NetworkAccess { + environment_id, + command, + cwd, + .. + } => { + self.request_command_approval( + ctx.review_context.turn(), + ctx.call_id.clone(), + /*approval_id*/ None, + Some(environment_id.clone()), + command.clone(), + cwd.clone(), + ctx.approval_reason.clone(), + ctx.network_approval_context.clone(), + /*proposed_execpolicy_amendment*/ None, + /*additional_permissions*/ None, + /*available_decisions*/ None, + /*plugin_attribution_override*/ None, + ) + .await + } } } } @@ -655,7 +779,7 @@ fn record_resolution(ctx: &ApprovalContext, resolution: &ApprovalResolution) { tool_name.as_ref(), &ctx.call_id, &resolution.decision, - source, + Some(source), ); } diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index 3b809e57a9..ca6630916d 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -1,18 +1,12 @@ -use crate::guardian::GuardianApprovalRequest; use crate::guardian::GuardianNetworkAccessTrigger; -use crate::guardian::GuardianReviewOptions; -use crate::guardian::new_guardian_review_id; -use crate::guardian::review_approval_request_with_cancel; -use crate::guardian::routes_approval_to_guardian; -use crate::hook_runtime::run_permission_request_hooks; +use crate::guardian::GuardianReviewContext; use crate::network_policy_decision::denied_network_policy_message; use crate::session::session::Session; use crate::session::turn_context::TurnEnvironment; +use crate::tools::approvals::ApprovalAction; +use crate::tools::approvals::ApprovalContext; use crate::tools::events::truncate_rejection_message; -use crate::tools::sandboxing::PermissionRequestPayload; use crate::tools::sandboxing::ToolError; -use codex_analytics::GuardianApprovalRequestSource; -use codex_hooks::PermissionRequestDecision; use codex_network_proxy::BlockedRequest; use codex_network_proxy::BlockedRequestObserver; use codex_network_proxy::NetworkDecision; @@ -30,6 +24,7 @@ use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::WarningEvent; use codex_sandboxing::record_network_sandbox_violation; +use codex_tools::ToolName; use indexmap::IndexMap; use std::collections::HashMap; use std::collections::HashSet; @@ -569,16 +564,6 @@ impl NetworkApprovalService { owner_call.cancellation_token.cancel(); } - async fn active_turn_context( - session: &Session, - ) -> Option> { - let active_turn = session.active_turn.lock().await; - active_turn - .as_ref() - .and_then(|turn| turn.task.as_ref()) - .map(|task| Arc::clone(&task.turn_context)) - } - fn format_network_target(protocol: &str, host: &str, port: u16) -> String { format!("{protocol}://{host}:{port}") } @@ -614,11 +599,11 @@ impl NetworkApprovalService { else { return NetworkDecision::deny(REASON_NOT_ALLOWED); }; - let turn_context = Self::active_turn_context(session.as_ref()).await; + let active_turn = session.active_turn_context_and_strict_auto_review().await; let Some(environment_id) = active_environment_id.or_else(|| { - turn_context + active_turn .as_ref() - .and_then(|turn_context| turn_context.environments.primary()) + .and_then(|(turn_context, _)| turn_context.environments.primary()) .map(|environment| environment.environment_id.clone()) }) else { return NetworkDecision::deny(REASON_NOT_ALLOWED); @@ -645,7 +630,7 @@ impl NetworkApprovalService { format!("Network access to \"{target}\" was blocked by policy."); let prompt_reason = format!("{} is not in the allowed_domains", request.host); - let Some(turn_context) = turn_context else { + let Some((turn_context, strict_auto_review)) = active_turn else { if let Some(owner_call) = owner_call.as_ref() { self.record_call_outcome(&owner_call.registration_id, policy_denial_message) .await; @@ -714,99 +699,97 @@ impl NetworkApprovalService { let command = owner_call .as_ref() .map_or_else(|| prompt_command.join(" "), |call| call.command.clone()); - let hook_approval_decision = match run_permission_request_hooks( - &session, - &turn_context, - &hook_run_id_suffix, - PermissionRequestPayload::bash(command, Some(format!("network-access {target}"))), - ) - .await - { - Some(PermissionRequestDecision::Allow) => Some(ReviewDecision::Approved), - Some(PermissionRequestDecision::Deny { message }) => { + let cwd = if let Some(owner_call) = owner_call.as_ref() { + owner_call.trigger.cwd.clone() + } else { + turn_context + .environments + .turn_environments() + .find(|environment| environment.environment_id == environment_id) + .and_then(|environment| environment.cwd().to_abs_path().ok()) + .unwrap_or_else(|| { + #[allow(deprecated)] + turn_context.cwd.clone() + }) + }; + let approval_call_id = format!("{guardian_approval_id}#{}", Uuid::new_v4()); + let telemetry_call_id = owner_call.as_ref().map_or_else( + || Uuid::new_v4().to_string(), + |call| call.trigger.call_id.clone(), + ); + let telemetry_tool_name = owner_call.as_ref().map_or_else( + || "network_access".to_string(), + |call| call.trigger.tool_name.clone(), + ); + let action = ApprovalAction::NetworkAccess { + id: guardian_approval_id, + turn_id: turn_context.sub_id.clone(), + environment_id, + target, + host: request.host.clone(), + protocol, + port: key.port, + trigger: owner_call.as_ref().map(|call| call.trigger.clone()), + hook_command: command, + hook_run_id: hook_run_id_suffix, + command: prompt_command, + cwd, + }; + let approval_context = ApprovalContext { + review_context: GuardianReviewContext::from(&turn_context), + call_id: approval_call_id, + tool_name: ToolName::plain(telemetry_tool_name.clone()), + strict_auto_review, + approval_reason: Some(prompt_reason), + retry_reason: Some(policy_denial_message.clone()), + network_approval_context: Some(network_approval_context.clone()), + }; + let approval_decision = match session.request_approval(action, approval_context).await { + Ok(decision) => decision, + Err(ToolError::Rejected(rejection)) => { if let Some(owner_call) = owner_call.as_ref() { - self.record_call_outcome(&owner_call.registration_id, message) + self.record_call_outcome(&owner_call.registration_id, rejection) .await; } + turn_context.session_telemetry.tool_decision( + &telemetry_tool_name, + &telemetry_call_id, + &ReviewDecision::denied("network approval was rejected"), + /*source*/ None, + ); + pending_owner.complete(PendingApprovalDecision::Deny); + return NetworkDecision::deny(REASON_NOT_ALLOWED); + } + Err(ToolError::Codex(err)) => { + let telemetry_decision = if matches!( + err.details(), + codex_protocol::error::CodexErrorDetails::TurnAborted + ) { + ReviewDecision::Abort + } else { + ReviewDecision::denied("network approval failed") + }; + if let Some(owner_call) = owner_call.as_ref() { + let rejection = if matches!( + err.details(), + codex_protocol::error::CodexErrorDetails::TurnAborted + ) { + "rejected by user".to_string() + } else { + format!("Error while requesting approval: {err}") + }; + self.record_call_outcome(&owner_call.registration_id, rejection) + .await; + } + turn_context.session_telemetry.tool_decision( + &telemetry_tool_name, + &telemetry_call_id, + &telemetry_decision, + /*source*/ None, + ); pending_owner.complete(PendingApprovalDecision::Deny); return NetworkDecision::deny(REASON_NOT_ALLOWED); } - None => None, - }; - let use_guardian = routes_approval_to_guardian(&turn_context); - let guardian_review_id = use_guardian.then(new_guardian_review_id); - let approval_decision = if let Some(hook_approval_decision) = hook_approval_decision { - hook_approval_decision - } else if let Some(review_id) = guardian_review_id.clone() { - let review_cancel = CancellationToken::new(); - let review_cancel_guard = review_cancel.clone().drop_guard(); - let review_session = Arc::clone(&session); - let review_turn = Arc::clone(&turn_context); - let review_request = GuardianApprovalRequest::NetworkAccess { - id: guardian_approval_id.clone(), - turn_id: owner_call - .as_ref() - .map_or_else(|| turn_context.sub_id.clone(), |call| call.turn_id.clone()), - target: target.clone(), - host: request.host.clone(), - protocol, - port: key.port, - trigger: owner_call.as_ref().map(|call| call.trigger.clone()), - }; - let retry_reason = Some(policy_denial_message.clone()); - let review = tokio::spawn(async move { - review_approval_request_with_cancel( - &review_session, - &review_turn, - review_id, - review_request, - retry_reason, - GuardianReviewOptions { - plugin_attribution_override: None, - approval_request_source: GuardianApprovalRequestSource::MainTurn, - external_cancel: Some(review_cancel), - }, - ) - .await - }); - let decision = review.await.unwrap_or_else(|err| { - warn!("network Guardian review task failed: {err}"); - ReviewDecision::denied("automatic approval review could not complete") - }); - drop(review_cancel_guard.disarm()); - decision - } else { - let available_decisions = None; - let cwd = if let Some(owner_call) = owner_call.as_ref() { - owner_call.trigger.cwd.clone() - } else { - turn_context - .environments - .turn_environments() - .find(|environment| environment.environment_id == environment_id) - .and_then(|environment| environment.cwd().to_abs_path().ok()) - .unwrap_or_else(|| { - #[allow(deprecated)] - turn_context.cwd.clone() - }) - }; - let approval_call_id = format!("{guardian_approval_id}#{}", Uuid::new_v4()); - session - .request_command_approval( - turn_context.as_ref(), - approval_call_id, - /*approval_id*/ None, - Some(environment_id), - prompt_command, - cwd, - Some(prompt_reason), - Some(network_approval_context.clone()), - /*proposed_execpolicy_amendment*/ None, - /*additional_permissions*/ None, - available_decisions, - /*plugin_attribution_override*/ None, - ) - .await }; let _session_policy_commit_guard = if matches!( @@ -820,6 +803,8 @@ impl NetworkApprovalService { } else { None }; + let mut telemetry_decision = approval_decision.clone(); + let mut network_policy_amendment_applied = false; let resolved = match approval_decision { ReviewDecision::Approved | ReviewDecision::ApprovedExecpolicyAmendment { .. } => { if self.session_denied_hosts.lock().await.contains(&key) { @@ -866,6 +851,7 @@ impl NetworkApprovalService { .await { Ok(()) => { + network_policy_amendment_applied = true; session .record_network_policy_amendment_message( &turn_context.sub_id, @@ -915,6 +901,7 @@ impl NetworkApprovalService { .await { Ok(()) => { + network_policy_amendment_applied = true; session .record_network_policy_amendment_message( &turn_context.sub_id, @@ -949,8 +936,11 @@ impl NetworkApprovalService { PendingApprovalDecision::Deny } }, - ReviewDecision::ApprovedMcpPolicyAmendment => { - error!("Network approval received ApprovedMcpPolicyAmendment"); + ReviewDecision::ApprovedMcpPolicyAmendment + | ReviewDecision::Denied { .. } + | ReviewDecision::TimedOut + | ReviewDecision::Abort => { + error!("centralized network approval returned an invalid decision"); if let Some(owner_call) = owner_call.as_ref() { self.record_call_outcome( &owner_call.registration_id, @@ -960,44 +950,32 @@ impl NetworkApprovalService { } PendingApprovalDecision::Deny } - ReviewDecision::Denied { rejection } => { - if let Some(owner_call) = owner_call.as_ref() { - self.record_call_outcome(&owner_call.registration_id, rejection) - .await; - } - PendingApprovalDecision::Deny - } - ReviewDecision::TimedOut => { - if let Some(owner_call) = owner_call.as_ref() { - self.record_call_outcome( - &owner_call.registration_id, - crate::guardian::guardian_timeout_message(), - ) - .await; - } - PendingApprovalDecision::Deny - } - ReviewDecision::Abort => { - if use_guardian { - if let Some(owner_call) = owner_call.as_ref() { - self.record_call_outcome( - &owner_call.registration_id, - "automatic approval review was cancelled".to_string(), - ) - .await; - } - } else if let Some(owner_call) = owner_call.as_ref() { - self.record_call_outcome( - &owner_call.registration_id, - "rejected by user".to_string(), - ) - .await; - } - PendingApprovalDecision::Deny - } }; pending_owner.set_decision_on_drop(resolved); + let decision_was_network_policy_amendment = matches!( + &telemetry_decision, + ReviewDecision::NetworkPolicyAmendment { .. } + ); + if decision_was_network_policy_amendment && !network_policy_amendment_applied { + telemetry_decision = match resolved { + PendingApprovalDecision::AllowOnce => ReviewDecision::Approved, + PendingApprovalDecision::AllowForSession => ReviewDecision::ApprovedForSession, + PendingApprovalDecision::Deny => { + ReviewDecision::denied("network approval was not applied") + } + }; + } else if matches!(resolved, PendingApprovalDecision::Deny) + && !decision_was_network_policy_amendment + { + telemetry_decision = ReviewDecision::denied("network approval was not applied"); + } + turn_context.session_telemetry.tool_decision( + &telemetry_tool_name, + &telemetry_call_id, + &telemetry_decision, + /*source*/ None, + ); pending_owner.complete(resolved); resolved.to_network_decision() diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index 2f3ee18701..ad3ab60a9b 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -195,7 +195,7 @@ impl ToolOrchestrator { &otel_tn, otel_ci, &ReviewDecision::Approved, - ToolDecisionSource::Config, + Some(ToolDecisionSource::Config), ); } } diff --git a/codex-rs/core/tests/suite/network_approval.rs b/codex-rs/core/tests/suite/network_approval.rs index 17465ee0af..730ba37990 100644 --- a/codex-rs/core/tests/suite/network_approval.rs +++ b/codex-rs/core/tests/suite/network_approval.rs @@ -15,6 +15,7 @@ use codex_protocol::approvals::NetworkPolicyRuleAction; use codex_protocol::config_types::CollaborationMode; use codex_protocol::config_types::ModeKind; use codex_protocol::config_types::Settings; +use codex_protocol::models::NetworkPermissions; use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; @@ -26,6 +27,9 @@ use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::ThreadSettingsOverrides; use codex_protocol::protocol::TurnEnvironmentSelection; use codex_protocol::protocol::TurnEnvironmentSelections; +use codex_protocol::request_permissions::PermissionGrantScope; +use codex_protocol::request_permissions::RequestPermissionProfile; +use codex_protocol::request_permissions::RequestPermissionsResponse; use codex_protocol::user_input::UserInput; use codex_utils_path_uri::PathUri; use core_test_support::PathBufExt; @@ -204,6 +208,120 @@ async fn guardian_network_approval_preserves_action_and_outcome_routing() -> Res .find_map(|request| request.function_call_output_text(second_call_id)) .context("expected denied network tool output")?; assert!(denied_output.contains(denial)); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[cfg_attr( + not(target_os = "linux"), + ignore = "requires the trusted Linux proxy bridge" +)] +async fn strict_auto_review_routes_network_approval_to_guardian_when_user_reviewer_is_selected() +-> Result<()> { + skip_if_target_windows!(Ok(()), "uses the POSIX/Python network fixture"); + skip_if_host_windows!(Ok(())); + skip_if_no_network!(Ok(())); + skip_if_sandbox!(Ok(())); + + let server = start_mock_server().await; + let test = managed_network_unified_exec_test_with_features( + &server, + &[Feature::RequestPermissionsTool], + ) + .await?; + let permission_call_id = "strict-network-permissions"; + let network_call_id = "strict-network-access"; + let requested_permissions = RequestPermissionProfile { + network: Some(NetworkPermissions { + enabled: Some(true), + }), + ..Default::default() + }; + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-strict-network-permissions"), + ev_function_call( + permission_call_id, + "request_permissions", + &serde_json::to_string(&json!({ + "reason": "Require automatic review for the rest of this turn", + "permissions": requested_permissions, + }))?, + ), + ev_completed("resp-strict-network-permissions"), + ]), + sse(vec![ + ev_response_created("resp-strict-network-command"), + ev_function_call( + network_call_id, + "exec_command", + &serde_json::to_string(&network_fetch_args(LOCAL_ENVIRONMENT_ID))?, + ), + ev_completed("resp-strict-network-command"), + ]), + sse(vec![ + ev_response_created("resp-strict-command-guardian"), + ev_assistant_message( + "msg-strict-command-guardian", + r#"{"risk_level":"low","user_authorization":"high","outcome":"allow","rationale":"The strict-review command is safe."}"#, + ), + ev_completed("resp-strict-command-guardian"), + ]), + sse(vec![ + ev_response_created("resp-strict-network-guardian"), + ev_assistant_message( + "msg-strict-network-guardian", + r#"{"risk_level":"low","user_authorization":"high","outcome":"allow","rationale":"The strict-review network request is safe."}"#, + ), + ev_completed("resp-strict-network-guardian"), + ]), + sse(vec![ + ev_response_created("resp-strict-network-complete"), + ev_assistant_message("msg-strict-network-complete", "reviewed"), + ev_completed("resp-strict-network-complete"), + ]), + ], + ) + .await; + + submit_managed_network_turn( + &test, + "grant turn permissions, then automatically review network access", + vec![local(test.config.cwd.clone())], + ApprovalsReviewer::User, + AskForApproval::OnRequest, + ) + .await?; + let EventMsg::RequestPermissions(request) = wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::RequestPermissions(_)) + }) + .await + else { + unreachable!("matched request permissions event") + }; + assert_eq!(request.call_id, permission_call_id); + test.codex + .submit(Op::RequestPermissionsResponse { + id: permission_call_id.to_string(), + response: RequestPermissionsResponse { + permissions: request.permissions, + scope: PermissionGrantScope::Turn, + strict_auto_review: true, + }, + }) + .await?; + wait_for_completion_without_network_prompt(&test).await; + + let actions = guardian_network_actions(&responses)?; + assert_eq!(actions.len(), 1); + assert_eq!( + actions[0] + .pointer("/trigger/callId") + .and_then(Value::as_str), + Some(network_call_id) + ); Ok(()) } @@ -412,6 +530,194 @@ async fn timed_out_guardian_network_review_uses_timeout_outcome_without_user_fal Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[cfg_attr( + not(target_os = "linux"), + ignore = "requires the trusted Linux proxy bridge" +)] +async fn background_network_approval_uses_active_turn_after_original_turn_completes() -> Result<()> +{ + skip_if_target_windows!(Ok(()), "uses the POSIX/Python network fixture"); + skip_if_host_windows!(Ok(())); + skip_if_no_network!(Ok(())); + skip_if_sandbox!(Ok(())); + + let server = start_mock_server().await; + let test = managed_network_unified_exec_test_with_features( + &server, + &[Feature::RequestPermissionsTool], + ) + .await?; + let start_call_id = "cross-turn-network-start"; + let permission_call_id = "cross-turn-network-permissions"; + let stdin_call_id = "cross-turn-network-stdin"; + let requested_permissions = RequestPermissionProfile { + network: Some(NetworkPermissions { + enabled: Some(true), + }), + ..Default::default() + }; + let command = format!( + "read _; python3 -c \"import urllib.request; urllib.request.build_opener(urllib.request.ProxyHandler()).open('{NETWORK_TEST_TARGET}', timeout=2).read()\"; echo CROSS-TURN-NETWORK-COMPLETE; read _" + ); + let mut start_args = network_exec_args(&command); + start_args["environment_id"] = json!(LOCAL_ENVIRONMENT_ID); + start_args["tty"] = json!(true); + start_args["yield_time_ms"] = json!(250); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-cross-turn-network-start"), + ev_function_call( + start_call_id, + "exec_command", + &serde_json::to_string(&start_args)?, + ), + ev_completed("resp-cross-turn-network-start"), + ]), + sse(vec![ + ev_response_created("resp-cross-turn-network-first-complete"), + ev_assistant_message("msg-cross-turn-network-first-complete", "terminal started"), + ev_completed("resp-cross-turn-network-first-complete"), + ]), + sse(vec![ + ev_response_created("resp-cross-turn-network-permissions"), + ev_function_call( + permission_call_id, + "request_permissions", + &serde_json::to_string(&json!({ + "reason": "Automatically review the existing terminal's network access", + "permissions": requested_permissions, + }))?, + ), + ev_completed("resp-cross-turn-network-permissions"), + ]), + sse(vec![ + ev_response_created("resp-cross-turn-network-stdin"), + ev_function_call( + stdin_call_id, + "write_stdin", + &serde_json::to_string(&json!({ + "session_id": 1000, + "chars": "continue\n", + "yield_time_ms": 1_000, + }))?, + ), + ev_completed("resp-cross-turn-network-stdin"), + ]), + sse(vec![ + ev_response_created("resp-cross-turn-network-guardian"), + ev_assistant_message( + "msg-cross-turn-network-guardian", + r#"{"risk_level":"low","user_authorization":"high","outcome":"allow","rationale":"The existing terminal's network request is safe."}"#, + ), + ev_completed("resp-cross-turn-network-guardian"), + ]), + sse(vec![ + ev_response_created("resp-cross-turn-network-second-complete"), + ev_assistant_message( + "msg-cross-turn-network-second-complete", + "network request approved", + ), + ev_completed("resp-cross-turn-network-second-complete"), + ]), + ], + ) + .await; + + submit_managed_network_turn( + &test, + "start a background terminal that waits before requesting network access", + vec![local(test.config.cwd.clone())], + ApprovalsReviewer::User, + AskForApproval::OnRequest, + ) + .await?; + let EventMsg::TurnComplete(first_turn) = wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await + else { + unreachable!("matched first turn completion") + }; + assert_eq!(test.codex.list_background_terminals().await.len(), 1); + + submit_managed_network_turn( + &test, + "allow the existing background terminal to request network access", + vec![local(test.config.cwd.clone())], + ApprovalsReviewer::User, + AskForApproval::OnRequest, + ) + .await?; + let EventMsg::TurnStarted(active_turn) = wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnStarted(_)) + }) + .await + else { + unreachable!("matched second turn start") + }; + let EventMsg::RequestPermissions(request) = wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::RequestPermissions(_)) + }) + .await + else { + unreachable!("matched request permissions event") + }; + assert_eq!(request.call_id, permission_call_id); + test.codex + .submit(Op::RequestPermissionsResponse { + id: permission_call_id.to_string(), + response: RequestPermissionsResponse { + permissions: request.permissions, + scope: PermissionGrantScope::Turn, + strict_auto_review: true, + }, + }) + .await?; + let assessment = wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::GuardianAssessment(assessment) + if assessment.status == GuardianAssessmentStatus::Approved + ) || matches!( + event, + EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_) + ) + }) + .await; + let EventMsg::GuardianAssessment(assessment) = assessment else { + panic!("expected Guardian to approve the background terminal's network request"); + }; + assert_eq!(assessment.turn_id, active_turn.turn_id); + assert_ne!(assessment.turn_id, first_turn.turn_id); + wait_for_turn_complete(&test).await; + + let actions = guardian_network_actions(&responses)?; + assert_eq!(actions.len(), 1); + assert_eq!( + actions[0] + .pointer("/trigger/callId") + .and_then(Value::as_str), + Some(start_call_id) + ); + let stdin_output = responses + .requests() + .iter() + .find_map(|request| request.function_call_output_text(stdin_call_id)) + .context("expected background terminal network request output")?; + assert!(!stdin_output.contains("blocked by policy")); + assert_eq!( + test.codex.list_background_terminals().await.len(), + 1, + "approved network access must not terminate the background process" + ); + test.codex.submit(Op::CleanBackgroundTerminals).await?; + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[cfg_attr( not(target_os = "linux"), @@ -753,7 +1059,6 @@ async fn allowing_network_policy_amendment_persists_context_and_bypasses_prompt( "Allowed network rule saved in execpolicy (allowlist): codex-network-test.invalid", ) })); - mount_exec_network_turn( &server, "resp-network-amendment-2", @@ -774,6 +1079,62 @@ async fn allowing_network_policy_amendment_persists_context_and_bypasses_prompt( Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[cfg_attr( + not(target_os = "linux"), + ignore = "requires the trusted Linux proxy bridge" +)] +async fn denying_network_policy_amendment_persists_and_blocks_request() -> Result<()> { + skip_if_target_windows!(Ok(()), "uses the POSIX/Python network fixture"); + skip_if_host_windows!(Ok(())); + skip_if_no_network!(Ok(())); + skip_if_sandbox!(Ok(())); + + let server = start_mock_server().await; + let test = managed_network_unified_exec_test(&server).await?; + let responses = mount_exec_network_turn( + &server, + "resp-network-deny-amendment", + "network-deny-amendment", + network_fetch_args(LOCAL_ENVIRONMENT_ID), + ) + .await?; + submit_managed_network_turn( + &test, + "persist a deny rule for this host", + vec![local(test.config.cwd.clone())], + ApprovalsReviewer::User, + AskForApproval::OnRequest, + ) + .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::NetworkPolicyAmendment { + network_policy_amendment: NetworkPolicyAmendment { + host: NETWORK_TEST_HOST.to_string(), + action: NetworkPolicyRuleAction::Deny, + }, + }, + }) + .await?; + wait_for_turn_complete(&test).await; + + let policy = fs::read_to_string(test.home.path().join("rules/default.rules"))?; + assert!(policy.contains( + r#"network_rule(host="codex-network-test.invalid", protocol="http", decision="deny""# + )); + let output = responses + .requests() + .iter() + .find_map(|request| request.function_call_output_text("network-deny-amendment")) + .context("expected denied network tool output")?; + assert!(output.contains("rejected by user")); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[cfg_attr( not(target_os = "linux"), @@ -824,7 +1185,6 @@ async fn failed_network_policy_amendment_denies_request_and_does_not_approve_hos .find_map(|request| request.function_call_output_text("network-failed-amendment-1")) .context("expected the failed policy amendment to reject the network request")?; assert!(denied_output.contains("blocked by policy")); - mount_exec_network_turn( &server, "resp-network-failed-amendment-2", @@ -1550,6 +1910,13 @@ async fn approved_network_host_for_one_environment_still_prompts_in_another() -> } async fn managed_network_unified_exec_test(server: &wiremock::MockServer) -> Result { + managed_network_unified_exec_test_with_features(server, &[]).await +} + +async fn managed_network_unified_exec_test_with_features( + server: &wiremock::MockServer, + features: &[Feature], +) -> Result { let home = Arc::new(TempDir::new()?); fs::write( home.path().join("config.toml"), @@ -1572,6 +1939,7 @@ allow_local_binding = true /*exclude_slash_tmp*/ false, ); let permission_profile_for_config = permission_profile.clone(); + let features = features.to_vec(); let mut builder = test_codex() .with_home(home) .with_cloud_config_bundle(managed_network_requirements_loader()) @@ -1581,6 +1949,12 @@ allow_local_binding = true .features .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); + for feature in &features { + config + .features + .enable(*feature) + .expect("test config should allow feature update"); + } config.permissions.approval_policy = Constrained::allow_any(approval_policy); config .permissions @@ -1790,6 +2164,10 @@ fn guardian_network_actions(responses: &ResponseMock) -> Result> { .into_iter() .filter(|request| { request.body_json()["client_metadata"]["x-openai-subagent"].as_str() == Some("guardian") + && request + .message_input_texts("user") + .iter() + .any(|text| text.contains("\"tool\": \"network_access\"")) }) .map(|request| { let user_texts = request.message_input_texts("user"); diff --git a/codex-rs/core/tests/suite/otel.rs b/codex-rs/core/tests/suite/otel.rs index cd40166938..6fd4c4360e 100644 --- a/codex-rs/core/tests/suite/otel.rs +++ b/codex-rs/core/tests/suite/otel.rs @@ -4,6 +4,8 @@ use codex_features::Feature; use codex_otel::SessionTelemetry; use codex_otel::TelemetryAuthMode; use codex_protocol::ThreadId; +use codex_protocol::approvals::NetworkPolicyAmendment; +use codex_protocol::approvals::NetworkPolicyRuleAction; use codex_protocol::config_types::ServiceTier; use codex_protocol::models::PermissionProfile; use codex_protocol::openai_models::ReasoningEffort; @@ -1123,6 +1125,73 @@ fn sandbox_outcome_assertion<'a>( } } +#[test] +#[traced_test] +fn network_policy_decisions_omit_source_and_destination() { + let telemetry = SessionTelemetry::new( + ThreadId::new(), + "gpt-5.5", + "gpt-5.5", + /*account_id*/ None, + /*account_email*/ None, + Some(TelemetryAuthMode::ApiKey), + "Codex_Desktop".to_string(), + /*log_user_prompts*/ false, + "tty".to_string(), + SessionSource::Cli, + ); + + for (call_id, action, expected_decision) in [ + ( + "network-allow", + NetworkPolicyRuleAction::Allow, + "approved_with_network_policy_allow", + ), + ( + "network-deny", + NetworkPolicyRuleAction::Deny, + "denied_with_network_policy_deny", + ), + ] { + telemetry.tool_decision( + "exec_command", + call_id, + &ReviewDecision::NetworkPolicyAmendment { + network_policy_amendment: NetworkPolicyAmendment { + host: "private.example.com".to_string(), + action, + }, + }, + /*source*/ None, + ); + + logs_assert(|lines: &[&str]| { + let line = lines + .iter() + .find(|line| { + line.contains("codex.tool_decision") + && line.contains(&format!("call_id={call_id}")) + }) + .ok_or_else(|| format!("missing network tool decision for {call_id}"))?; + + if !line.contains("tool_name=exec_command") { + return Err("missing triggering network tool name".to_string()); + } + if !line.contains(&format!("decision={expected_decision}")) { + return Err(format!("unexpected network tool decision for {call_id}")); + } + if line.contains("source=") { + return Err("network tool decision unexpectedly included a source".to_string()); + } + if line.contains("private.example.com") { + return Err("network tool decision exposed the destination host".to_string()); + } + + Ok(()) + }); + } +} + #[test] #[traced_test] fn sandbox_outcome_event_records_outcome() { @@ -1327,7 +1396,7 @@ async fn handle_shell_command_user_approved_for_session_records_tool_decision() logs_assert(tool_decision_assertion( "user_approved_session_call", - "approvedforsession", + "approved_for_session", "user", )); } @@ -1519,7 +1588,7 @@ async fn handle_sandbox_error_user_approves_for_session_records_tool_decision() logs_assert(tool_decision_assertion( "sandbox_session_call", - "approvedforsession", + "approved_for_session", "user", )); } diff --git a/codex-rs/otel/src/events/session_telemetry.rs b/codex-rs/otel/src/events/session_telemetry.rs index 0e5c94bae1..11981c3aeb 100644 --- a/codex-rs/otel/src/events/session_telemetry.rs +++ b/codex-rs/otel/src/events/session_telemetry.rs @@ -992,16 +992,25 @@ impl SessionTelemetry { tool_name: &str, call_id: &str, decision: &ReviewDecision, - source: ToolDecisionSource, + source: Option, ) { - log_event!( - self, - event.name = "codex.tool_decision", - tool_name = %tool_name, - call_id = %call_id, - decision = %decision.clone().to_string().to_lowercase(), - source = %source.to_string(), - ); + match source { + Some(source) => log_event!( + self, + event.name = "codex.tool_decision", + tool_name = %tool_name, + call_id = %call_id, + decision = %decision.to_opaque_string(), + source = %source.to_string(), + ), + None => log_event!( + self, + event.name = "codex.tool_decision", + tool_name = %tool_name, + call_id = %call_id, + decision = %decision.to_opaque_string(), + ), + } } pub fn sandbox_outcome(