From bb728b069396e22db870e68598db92a8ce86ab0f Mon Sep 17 00:00:00 2001 From: rreichel3-oai Date: Thu, 19 Mar 2026 14:44:01 -0400 Subject: [PATCH] Tighten generated execpolicy prefix suggestions In codex-rs/core/src/exec_policy.rs, narrow auto-generated execpolicy amendments by truncating a command at the first flag-like token, while falling back to the whole command segment when that would leave fewer than two tokens. For parseable multi-command shell scripts, derive the suggestion from the first parsed segment. In codex-rs/core/src/exec_policy_tests.rs, add and update coverage for single-command flag truncation, early-flag fallback, and multi-command first-segment suggestion behavior. --- codex-rs/core/src/exec_policy.rs | 50 +++++++++++++-- codex-rs/core/src/exec_policy_tests.rs | 84 ++++++++++++++++++++++---- 2 files changed, 115 insertions(+), 19 deletions(-) diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 0c95af4c02..733e9dd4ce 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -287,6 +287,7 @@ impl ExecPolicyManager { if auto_amendment_allowed { try_derive_execpolicy_amendment_for_prompt_rules( &evaluation.matched_rules, + &commands, ) } else { None @@ -301,7 +302,10 @@ impl ExecPolicyManager { 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) + try_derive_execpolicy_amendment_for_allow_rules( + &evaluation.matched_rules, + &commands, + ) } else { None }, @@ -640,12 +644,13 @@ fn commands_for_exec_policy(command: &[String]) -> (Vec>, bool) { /// - Examples: /// - execpolicy: empty. Command: `["python"]`. Heuristics prompt -> `Some(vec!["python"])`. /// - execpolicy: empty. Command: `["bash", "-c", "cd /some/folder && prog1 --option1 arg1 && prog2 --option2 arg2"]`. -/// Parsed commands include `cd /some/folder`, `prog1 --option1 arg1`, and `prog2 --option2 arg2`. If heuristics allow `cd` but prompt -/// on `prog1`, we return `Some(vec!["prog1", "--option1", "arg1"])`. +/// Parsed commands include `cd /some/folder`, `prog1 --option1 arg1`, and `prog2 --option2 arg2`. For multi-command scripts, +/// we derive the suggestion from the first parsed segment, so this returns `Some(vec!["cd", "/some/folder"])`. /// - execpolicy: contains a `prompt for prefix ["prog2"]` rule. For the same command as above, -/// we return `None` because an execpolicy prompt still applies even if we amend execpolicy to allow ["prog1", "--option1", "arg1"]. +/// we return `None` because an execpolicy prompt still applies even if we amend execpolicy to allow ["cd", "/some/folder"]. fn try_derive_execpolicy_amendment_for_prompt_rules( matched_rules: &[RuleMatch], + commands: &[Vec], ) -> Option { if matched_rules .iter() @@ -654,13 +659,17 @@ fn try_derive_execpolicy_amendment_for_prompt_rules( return None; } + if commands.len() > 1 { + return auto_derived_execpolicy_amendment(&commands[0]); + } + matched_rules .iter() .find_map(|rule_match| match rule_match { RuleMatch::HeuristicsRuleMatch { command, decision: Decision::Prompt, - } => Some(ExecPolicyAmendment::from(command.clone())), + } => auto_derived_execpolicy_amendment(command), _ => None, }) } @@ -670,22 +679,51 @@ fn try_derive_execpolicy_amendment_for_prompt_rules( /// - If any execpolicy rule matches, return None, because we would already be running command outside the sandbox fn try_derive_execpolicy_amendment_for_allow_rules( matched_rules: &[RuleMatch], + commands: &[Vec], ) -> Option { if matched_rules.iter().any(is_policy_match) { return None; } + if commands.len() > 1 { + return auto_derived_execpolicy_amendment(&commands[0]); + } + matched_rules .iter() .find_map(|rule_match| match rule_match { RuleMatch::HeuristicsRuleMatch { command, decision: Decision::Allow, - } => Some(ExecPolicyAmendment::from(command.clone())), + } => auto_derived_execpolicy_amendment(command), _ => None, }) } +/// Keep generated execpolicy suggestions broad enough to cover similar +/// invocations, but stop before the first flag so we do not bake incidental +/// option values into a persisted allow rule. +/// +/// If truncating before the first flag would leave fewer than two tokens, fall +/// back to the whole command segment. +fn auto_derived_execpolicy_amendment(command: &[String]) -> Option { + let prefix: Vec = command + .iter() + .take_while(|token| !token.starts_with('-')) + .cloned() + .collect(); + + if prefix.len() >= 2 { + return Some(ExecPolicyAmendment::from(prefix)); + } + + if !command.is_empty() { + return Some(ExecPolicyAmendment::from(command.to_vec())); + } + + None +} + fn derive_requested_execpolicy_amendment_from_prefix_rule( prefix_rule: Option<&Vec>, matched_rules: &[RuleMatch], diff --git a/codex-rs/core/src/exec_policy_tests.rs b/codex-rs/core/src/exec_policy_tests.rs index fd3fe05e11..e1967c7d53 100644 --- a/codex-rs/core/src/exec_policy_tests.rs +++ b/codex-rs/core/src/exec_policy_tests.rs @@ -1032,7 +1032,7 @@ async fn exec_approval_requirement_falls_back_to_heuristics() { } #[tokio::test] -async fn empty_bash_lc_script_falls_back_to_original_command() { +async fn empty_bash_lc_script_falls_back_to_whole_command_when_truncated_prefix_is_too_short() { let command = vec!["bash".to_string(), "-lc".to_string(), "".to_string()]; let manager = ExecPolicyManager::default(); @@ -1057,7 +1057,8 @@ async fn empty_bash_lc_script_falls_back_to_original_command() { } #[tokio::test] -async fn whitespace_bash_lc_script_falls_back_to_original_command() { +async fn whitespace_bash_lc_script_falls_back_to_whole_command_when_truncated_prefix_is_too_short() +{ let command = vec![ "bash".to_string(), "-lc".to_string(), @@ -1118,7 +1119,7 @@ async fn request_rule_uses_prefix_rule() { } #[tokio::test] -async fn request_rule_falls_back_when_prefix_rule_does_not_approve_all_commands() { +async fn request_rule_falls_back_to_first_segment_for_multi_command_scripts() { let command = vec![ "bash".to_string(), "-lc".to_string(), @@ -1142,9 +1143,9 @@ async fn request_rule_falls_back_when_prefix_rule_does_not_approve_all_commands( ExecApprovalRequirement::NeedsApproval { reason: None, proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![ - "rm".to_string(), - "-rf".to_string(), - "/tmp/codex".to_string(), + "cargo".to_string(), + "install".to_string(), + "cargo-insta".to_string(), ])), } ); @@ -1178,8 +1179,8 @@ async fn heuristics_apply_when_other_commands_match_policy() { ExecApprovalRequirement::NeedsApproval { reason: None, proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![ - "orange".to_string() - ])) + "apple".to_string() + ])), } ); } @@ -1260,6 +1261,64 @@ async fn proposed_execpolicy_amendment_is_present_for_single_command_without_pol ); } +#[tokio::test] +async fn proposed_execpolicy_amendment_stops_before_first_flag_for_generated_suggestions() { + let command = vec![ + "cargo".to_string(), + "test".to_string(), + "--package".to_string(), + "codex-core".to_string(), + ]; + + let manager = ExecPolicyManager::default(); + let requirement = manager + .create_exec_approval_requirement_for_command(ExecApprovalRequest { + command: &command, + approval_policy: AskForApproval::UnlessTrusted, + sandbox_policy: &SandboxPolicy::new_read_only_policy(), + file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: None, + }) + .await; + + assert_eq!( + requirement, + ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![ + "cargo".to_string(), + "test".to_string(), + ])) + } + ); +} + +#[tokio::test] +async fn proposed_execpolicy_amendment_falls_back_to_whole_command_if_flag_starts_too_early() { + let command = vec!["curl".to_string(), "-k".to_string(), "xyz.com".to_string()]; + + let manager = ExecPolicyManager::default(); + let requirement = manager + .create_exec_approval_requirement_for_command(ExecApprovalRequest { + command: &command, + approval_policy: AskForApproval::UnlessTrusted, + sandbox_policy: &SandboxPolicy::new_read_only_policy(), + file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: None, + }) + .await; + + assert_eq!( + requirement, + ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(command)) + } + ); +} + #[tokio::test] async fn proposed_execpolicy_amendment_is_omitted_when_policy_prompts() { let policy_src = r#"prefix_rule(pattern=["rm"], decision="prompt")"#; @@ -1292,7 +1351,7 @@ async fn proposed_execpolicy_amendment_is_omitted_when_policy_prompts() { } #[tokio::test] -async fn proposed_execpolicy_amendment_is_present_for_multi_command_scripts() { +async fn proposed_execpolicy_amendment_is_based_on_first_segment_for_multi_command_scripts() { let command = vec![ "bash".to_string(), "-lc".to_string(), @@ -1323,7 +1382,8 @@ async fn proposed_execpolicy_amendment_is_present_for_multi_command_scripts() { } #[tokio::test] -async fn proposed_execpolicy_amendment_uses_first_no_match_in_multi_command_scripts() { +async fn proposed_execpolicy_amendment_uses_whole_one_token_first_segment_for_multi_command_scripts() + { let policy_src = r#"prefix_rule(pattern=["cat"], decision="allow")"#; let mut parser = PolicyParser::new(); parser @@ -1350,9 +1410,7 @@ async fn proposed_execpolicy_amendment_uses_first_no_match_in_multi_command_scri .await, ExecApprovalRequirement::NeedsApproval { reason: None, - proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![ - "apple".to_string() - ])), + proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec!["cat".to_string()])), } ); }