From 60ca57e09d6b19c175403f34729fd5707034d008 Mon Sep 17 00:00:00 2001 From: celia-oai Date: Fri, 20 Feb 2026 15:20:55 -0800 Subject: [PATCH] changes --- codex-rs/core/src/exec_policy.rs | 263 ++++++++++++++++++----- codex-rs/core/src/zsh_exec_bridge/mod.rs | 5 + 2 files changed, 210 insertions(+), 58 deletions(-) diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 01ae0e6800..b950c42036 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -166,6 +166,16 @@ impl ExecPolicyManager { Self::create_exec_approval_requirement_for_command_with_policy(exec_policy.as_ref(), req) } + /// Evaluates approval requirements with an in-memory prompt-only overlay. + /// + /// The base execpolicy is always evaluated first. If any base policy rule + /// matches (allow/prompt/forbidden), that result is returned as-is and the + /// overlay is ignored for this command. + /// + /// Overlay prefixes are only applied when the base evaluation had no policy + /// matches (i.e. only heuristics would decide). In that case, each overlay + /// prefix is added as an in-memory `prompt` rule and the command is + /// re-evaluated. The overlay is not persisted to the manager's policy. pub(crate) async fn create_exec_approval_requirement_for_command_with_overlay( &self, req: ExecApprovalRequest<'_>, @@ -175,7 +185,33 @@ impl ExecPolicyManager { return self.create_exec_approval_requirement_for_command(req).await; } - let mut exec_policy = self.current().as_ref().clone(); + let ExecApprovalRequest { + command, + approval_policy, + sandbox_policy, + sandbox_permissions, + prefix_rule, + } = req; + + let base_policy = self.current(); + let (base_evaluation, base_auto_amendment_allowed) = evaluate_exec_policy_for_command( + base_policy.as_ref(), + command, + approval_policy, + sandbox_policy, + sandbox_permissions, + ); + if base_evaluation.matched_rules.iter().any(is_policy_match) { + return create_exec_approval_requirement_from_evaluation( + command, + approval_policy, + prefix_rule.as_ref(), + &base_evaluation, + base_auto_amendment_allowed, + ); + } + + let mut exec_policy = base_policy.as_ref().clone(); for prefix in overlay_prompt_prefixes { if let Err(err) = exec_policy.add_prefix_rule(prefix, Decision::Prompt) { tracing::warn!( @@ -184,7 +220,20 @@ impl ExecPolicyManager { } } - Self::create_exec_approval_requirement_for_command_with_policy(&exec_policy, req) + let (overlay_evaluation, overlay_auto_amendment_allowed) = evaluate_exec_policy_for_command( + &exec_policy, + command, + approval_policy, + sandbox_policy, + sandbox_permissions, + ); + create_exec_approval_requirement_from_evaluation( + command, + approval_policy, + prefix_rule.as_ref(), + &overlay_evaluation, + overlay_auto_amendment_allowed, + ) } fn create_exec_approval_requirement_for_command_with_policy( @@ -198,63 +247,20 @@ impl ExecPolicyManager { sandbox_permissions, prefix_rule, } = req; - let (commands, used_complex_parsing) = commands_for_exec_policy(command); - // Keep heredoc prefix parsing for rule evaluation so existing - // allow/prompt/forbidden rules still apply, but avoid auto-derived - // amendments when only the heredoc fallback parser matched. - let auto_amendment_allowed = !used_complex_parsing; - let exec_policy_fallback = |cmd: &[String]| { - render_decision_for_unmatched_command( - approval_policy, - sandbox_policy, - cmd, - sandbox_permissions, - used_complex_parsing, - ) - }; - let evaluation = exec_policy.check_multiple(commands.iter(), &exec_policy_fallback); - - let requested_amendment = derive_requested_execpolicy_amendment_from_prefix_rule( - prefix_rule.as_ref(), - &evaluation.matched_rules, + let (evaluation, auto_amendment_allowed) = evaluate_exec_policy_for_command( + exec_policy, + command, + approval_policy, + sandbox_policy, + sandbox_permissions, ); - - match evaluation.decision { - Decision::Forbidden => ExecApprovalRequirement::Forbidden { - reason: derive_forbidden_reason(command, &evaluation), - }, - Decision::Prompt => { - if matches!(approval_policy, AskForApproval::Never) { - ExecApprovalRequirement::Forbidden { - reason: PROMPT_CONFLICT_REASON.to_string(), - } - } else { - ExecApprovalRequirement::NeedsApproval { - reason: derive_prompt_reason(command, &evaluation), - proposed_execpolicy_amendment: requested_amendment.or_else(|| { - if auto_amendment_allowed { - try_derive_execpolicy_amendment_for_prompt_rules( - &evaluation.matched_rules, - ) - } else { - None - } - }), - } - } - } - Decision::Allow => ExecApprovalRequirement::Skip { - // Bypass sandbox if execpolicy allows the command - bypass_sandbox: evaluation.matched_rules.iter().any(|rule_match| { - is_policy_match(rule_match) && rule_match.decision() == Decision::Allow - }), - proposed_execpolicy_amendment: if auto_amendment_allowed { - try_derive_execpolicy_amendment_for_allow_rules(&evaluation.matched_rules) - } else { - None - }, - }, - } + create_exec_approval_requirement_from_evaluation( + command, + approval_policy, + prefix_rule.as_ref(), + &evaluation, + auto_amendment_allowed, + ) } pub(crate) async fn append_amendment_and_update( @@ -283,6 +289,81 @@ impl ExecPolicyManager { } } +fn evaluate_exec_policy_for_command( + exec_policy: &Policy, + command: &[String], + approval_policy: AskForApproval, + sandbox_policy: &SandboxPolicy, + sandbox_permissions: SandboxPermissions, +) -> (Evaluation, bool) { + let (commands, used_complex_parsing) = commands_for_exec_policy(command); + // Keep heredoc prefix parsing for rule evaluation so existing + // allow/prompt/forbidden rules still apply, but avoid auto-derived + // amendments when only the heredoc fallback parser matched. + let auto_amendment_allowed = !used_complex_parsing; + let exec_policy_fallback = |cmd: &[String]| { + render_decision_for_unmatched_command( + approval_policy, + sandbox_policy, + cmd, + sandbox_permissions, + used_complex_parsing, + ) + }; + let evaluation = exec_policy.check_multiple(commands.iter(), &exec_policy_fallback); + (evaluation, auto_amendment_allowed) +} + +fn create_exec_approval_requirement_from_evaluation( + command: &[String], + approval_policy: AskForApproval, + prefix_rule: Option<&Vec>, + evaluation: &Evaluation, + auto_amendment_allowed: bool, +) -> ExecApprovalRequirement { + let requested_amendment = derive_requested_execpolicy_amendment_from_prefix_rule( + prefix_rule, + &evaluation.matched_rules, + ); + + match evaluation.decision { + Decision::Forbidden => ExecApprovalRequirement::Forbidden { + reason: derive_forbidden_reason(command, evaluation), + }, + Decision::Prompt => { + if matches!(approval_policy, AskForApproval::Never) { + ExecApprovalRequirement::Forbidden { + reason: PROMPT_CONFLICT_REASON.to_string(), + } + } else { + ExecApprovalRequirement::NeedsApproval { + reason: derive_prompt_reason(command, evaluation), + proposed_execpolicy_amendment: requested_amendment.or_else(|| { + if auto_amendment_allowed { + try_derive_execpolicy_amendment_for_prompt_rules( + &evaluation.matched_rules, + ) + } else { + None + } + }), + } + } + } + Decision::Allow => ExecApprovalRequirement::Skip { + // Bypass sandbox if execpolicy allows the command + bypass_sandbox: evaluation.matched_rules.iter().any(|rule_match| { + is_policy_match(rule_match) && rule_match.decision() == Decision::Allow + }), + proposed_execpolicy_amendment: if auto_amendment_allowed { + try_derive_execpolicy_amendment_for_allow_rules(&evaluation.matched_rules) + } else { + None + }, + }, + } +} + impl Default for ExecPolicyManager { fn default() -> Self { Self::new(Arc::new(Policy::empty())) @@ -1307,6 +1388,72 @@ prefix_rule( ); } + #[tokio::test] + async fn exec_approval_overlay_does_not_override_base_allow_rule_for_same_prefix() { + let policy_src = r#"prefix_rule(pattern=["/tmp/skill-script.sh"], decision="allow")"#; + let mut parser = PolicyParser::new(); + parser + .parse("test.rules", policy_src) + .expect("parse policy"); + let manager = ExecPolicyManager::new(Arc::new(parser.build())); + let command = vec!["/tmp/skill-script.sh".to_string()]; + + let requirement = manager + .create_exec_approval_requirement_for_command_with_overlay( + ExecApprovalRequest { + command: &command, + approval_policy: AskForApproval::UnlessTrusted, + sandbox_policy: &SandboxPolicy::new_read_only_policy(), + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: None, + }, + std::slice::from_ref(&command), + ) + .await; + + assert_eq!( + requirement, + ExecApprovalRequirement::Skip { + bypass_sandbox: true, + proposed_execpolicy_amendment: None, + } + ); + } + + #[tokio::test] + async fn exec_approval_overlay_does_not_override_base_prompt_reason_for_same_prefix() { + let policy_src = r#"prefix_rule(pattern=["/tmp/skill-script.sh"], decision="prompt", justification="base policy prompt")"#; + let mut parser = PolicyParser::new(); + parser + .parse("test.rules", policy_src) + .expect("parse policy"); + let manager = ExecPolicyManager::new(Arc::new(parser.build())); + let command = vec!["/tmp/skill-script.sh".to_string()]; + + let requirement = manager + .create_exec_approval_requirement_for_command_with_overlay( + ExecApprovalRequest { + command: &command, + approval_policy: AskForApproval::UnlessTrusted, + sandbox_policy: &SandboxPolicy::new_read_only_policy(), + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: None, + }, + std::slice::from_ref(&command), + ) + .await; + + assert_eq!( + requirement, + ExecApprovalRequirement::NeedsApproval { + reason: Some( + "`/tmp/skill-script.sh` requires approval: base policy prompt".to_string() + ), + proposed_execpolicy_amendment: None, + } + ); + } + #[tokio::test] async fn empty_bash_lc_script_falls_back_to_original_command() { let command = vec!["bash".to_string(), "-lc".to_string(), "".to_string()]; diff --git a/codex-rs/core/src/zsh_exec_bridge/mod.rs b/codex-rs/core/src/zsh_exec_bridge/mod.rs index 6ceab7b6a5..8eb4fc5d0f 100644 --- a/codex-rs/core/src/zsh_exec_bridge/mod.rs +++ b/codex-rs/core/src/zsh_exec_bridge/mod.rs @@ -353,11 +353,13 @@ impl ZshExecBridge { argv.clone() }; + // Check exec policy against the resolved executable path, not the original argv[0]. let mut command_for_execpolicy = command_for_approval.clone(); if let Some(program) = command_for_execpolicy.first_mut() { *program = file.clone(); } + // Collect skill-provided approval overlays that may add promptable prefix rules. let overlay_prompt_prefixes = { let registry = turn .skill_prefix_permissions @@ -365,6 +367,8 @@ impl ZshExecBridge { .unwrap_or_else(std::sync::PoisonError::into_inner); registry.keys().cloned().collect::>() }; + + // Ask exec policy whether this command can run, is forbidden, or needs approval. let exec_approval_requirement = session .services .exec_policy @@ -380,6 +384,7 @@ impl ZshExecBridge { ) .await; + // Send immediate allow/deny responses, or carry forward approval details for prompting. let (reason, proposed_execpolicy_amendment) = match exec_approval_requirement { ExecApprovalRequirement::Skip { .. } => { write_json_line(