diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index c20471047..62ebdde43 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -170,23 +170,26 @@ impl ExecPolicyManager { prefix_rule, } = req; let exec_policy = self.current(); - let (commands, used_heredoc_fallback) = commands_for_exec_policy(command); + 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_heredoc_fallback; + 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(prefix_rule.as_ref(), &evaluation.matched_rules); + let requested_amendment = derive_requested_execpolicy_amendment_from_prefix_rule( + prefix_rule.as_ref(), + &evaluation.matched_rules, + ); match evaluation.decision { Decision::Forbidden => ExecApprovalRequirement::Forbidden { @@ -406,8 +409,9 @@ pub fn render_decision_for_unmatched_command( sandbox_policy: &SandboxPolicy, command: &[String], sandbox_permissions: SandboxPermissions, + used_complex_parsing: bool, ) -> Decision { - if is_known_safe_command(command) { + if is_known_safe_command(command) && !used_complex_parsing { return Decision::Allow; } @@ -534,7 +538,7 @@ fn try_derive_execpolicy_amendment_for_allow_rules( }) } -fn derive_requested_execpolicy_amendment( +fn derive_requested_execpolicy_amendment_from_prefix_rule( prefix_rule: Option<&Vec>, matched_rules: &[RuleMatch], ) -> Option { @@ -552,10 +556,8 @@ fn derive_requested_execpolicy_amendment( return None; } - if matched_rules - .iter() - .any(|rule_match| is_policy_match(rule_match) && rule_match.decision() == Decision::Prompt) - { + // if any policy rule already matches, don't suggest an additional rule that might conflict or not apply + if matched_rules.iter().any(is_policy_match) { return None; } @@ -1560,14 +1562,17 @@ prefix_rule( #[test] fn derive_requested_execpolicy_amendment_returns_none_for_missing_prefix_rule() { - assert_eq!(None, derive_requested_execpolicy_amendment(None, &[])); + assert_eq!( + None, + derive_requested_execpolicy_amendment_from_prefix_rule(None, &[]) + ); } #[test] fn derive_requested_execpolicy_amendment_returns_none_for_empty_prefix_rule() { assert_eq!( None, - derive_requested_execpolicy_amendment(Some(&Vec::new()), &[]) + derive_requested_execpolicy_amendment_from_prefix_rule(Some(&Vec::new()), &[]) ); } @@ -1575,7 +1580,7 @@ prefix_rule( fn derive_requested_execpolicy_amendment_returns_none_for_exact_banned_prefix_rule() { assert_eq!( None, - derive_requested_execpolicy_amendment( + derive_requested_execpolicy_amendment_from_prefix_rule( Some(&vec!["python".to_string(), "-c".to_string()]), &[], ) @@ -1594,7 +1599,7 @@ prefix_rule( ] { assert_eq!( None, - derive_requested_execpolicy_amendment(Some(&prefix_rule), &[]) + derive_requested_execpolicy_amendment_from_prefix_rule(Some(&prefix_rule), &[]) ); } } @@ -1620,7 +1625,7 @@ prefix_rule( ] { assert_eq!( None, - derive_requested_execpolicy_amendment(Some(&prefix_rule), &[]) + derive_requested_execpolicy_amendment_from_prefix_rule(Some(&prefix_rule), &[]) ); } } @@ -1635,22 +1640,52 @@ prefix_rule( assert_eq!( Some(ExecPolicyAmendment::new(prefix_rule.clone())), - derive_requested_execpolicy_amendment(Some(&prefix_rule), &[]) + derive_requested_execpolicy_amendment_from_prefix_rule(Some(&prefix_rule), &[]) ); } #[test] - fn derive_requested_execpolicy_amendment_returns_none_when_policy_prompt_matches() { + fn derive_requested_execpolicy_amendment_returns_none_when_policy_matches() { let prefix_rule = vec!["cargo".to_string(), "build".to_string()]; - let matched_rules = vec![RuleMatch::PrefixRuleMatch { + + let matched_rules_prompt = vec![RuleMatch::PrefixRuleMatch { matched_prefix: vec!["cargo".to_string()], decision: Decision::Prompt, justification: None, }]; - assert_eq!( None, - derive_requested_execpolicy_amendment(Some(&prefix_rule), &matched_rules) + derive_requested_execpolicy_amendment_from_prefix_rule( + Some(&prefix_rule), + &matched_rules_prompt + ), + "should return none when prompt policy matches" + ); + let matched_rules_allow = vec![RuleMatch::PrefixRuleMatch { + matched_prefix: vec!["cargo".to_string()], + decision: Decision::Allow, + justification: None, + }]; + assert_eq!( + None, + derive_requested_execpolicy_amendment_from_prefix_rule( + Some(&prefix_rule), + &matched_rules_allow + ), + "should return none when prompt policy matches" + ); + let matched_rules_forbidden = vec![RuleMatch::PrefixRuleMatch { + matched_prefix: vec!["cargo".to_string()], + decision: Decision::Forbidden, + justification: None, + }]; + assert_eq!( + None, + derive_requested_execpolicy_amendment_from_prefix_rule( + Some(&prefix_rule), + &matched_rules_forbidden + ), + "should return none when prompt policy matches" ); } diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index 72de76bab..09d6755d3 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -1479,6 +1479,44 @@ fn scenarios() -> Vec { output_contains: "rejected by user", }, }, + ScenarioSpec { + name: "safe command with heredoc and redirect still requires approval", + approval_policy: AskForApproval::OnRequest, + sandbox_policy: workspace_write(false), + action: ActionKind::RunUnifiedExecCommand { + command: "cat <<'EOF' > /tmp/out.txt \nhello\nEOF", + justification: None, + }, + sandbox_permissions: SandboxPermissions::RequireEscalated, + features: vec![Feature::UnifiedExec], + model_override: None, + outcome: Outcome::ExecApproval { + decision: ReviewDecision::Denied, + expected_reason: None, + }, + expectation: Expectation::CommandFailure { + output_contains: "rejected by user", + }, + }, + ScenarioSpec { + name: "compound command with one safe command still requires approval", + approval_policy: AskForApproval::OnRequest, + sandbox_policy: workspace_write(false), + action: ActionKind::RunUnifiedExecCommand { + command: "cat ./one.txt && touch ./two.txt", + justification: None, + }, + sandbox_permissions: SandboxPermissions::RequireEscalated, + features: vec![Feature::UnifiedExec], + model_override: None, + outcome: Outcome::ExecApproval { + decision: ReviewDecision::Denied, + expected_reason: None, + }, + expectation: Expectation::CommandFailure { + output_contains: "rejected by user", + }, + }, ] } @@ -1890,14 +1928,15 @@ async fn approving_execpolicy_amendment_persists_policy_and_skips_future_prompts Ok(()) } +// todo(dylan) add ScenarioSpec support for rules #[tokio::test(flavor = "current_thread")] #[cfg(unix)] -async fn heredoc_with_chained_allowed_prefix_still_requires_approval() -> Result<()> { +async fn compound_command_with_one_safe_command_still_requires_approval() -> Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; let approval_policy = AskForApproval::UnlessTrusted; - let sandbox_policy = SandboxPolicy::new_read_only_policy(); + let sandbox_policy = SandboxPolicy::new_workspace_write_policy(); let sandbox_policy_for_config = sandbox_policy.clone(); let mut builder = test_codex().with_config(move |config| { config.permissions.approval_policy = Constrained::allow_any(approval_policy); @@ -1913,12 +1952,12 @@ async fn heredoc_with_chained_allowed_prefix_still_requires_approval() -> Result )?; let call_id = "heredoc-with-chained-prefix"; - let command = "cat <<'EOF' > /tmp/test.txt && touch allow-prefix.txt\nhello\nEOF"; + let command = "touch ./test.txt && rm ./test.txt"; let (event, expected_command) = ActionKind::RunCommand { command } .prepare(&test, &server, call_id, SandboxPermissions::UseDefault) .await?; let expected_command = - expected_command.expect("heredoc chained command scenario should produce a shell command"); + expected_command.expect("compound command should produce a shell command"); let _ = mount_sse_once( &server, @@ -1940,7 +1979,7 @@ async fn heredoc_with_chained_allowed_prefix_still_requires_approval() -> Result submit_turn( &test, - "heredoc chained prefix", + "compound command", approval_policy, sandbox_policy.clone(), ) diff --git a/codex-rs/shell-command/src/bash.rs b/codex-rs/shell-command/src/bash.rs index 47573621a..770ff4795 100644 --- a/codex-rs/shell-command/src/bash.rs +++ b/codex-rs/shell-command/src/bash.rs @@ -128,9 +128,6 @@ pub fn parse_shell_lc_single_command_prefix(command: &[String]) -> Option) -> bool { node.named_children(&mut cursor).next().is_none() } -fn has_named_descendant_kind(node: Node<'_>, kind: &str) -> bool { - let mut stack = vec![node]; - while let Some(current) = stack.pop() { - if current.kind() == kind { - return true; - } - let mut cursor = current.walk(); - for child in current.named_children(&mut cursor) { - stack.push(child); - } - } - false -} - fn is_allowed_heredoc_attachment_kind(kind: &str) -> bool { matches!( kind, @@ -532,7 +515,10 @@ mod tests { "-lc".to_string(), "echo hello > /tmp/out.txt".to_string(), ]; - assert_eq!(parse_shell_lc_single_command_prefix(&command), None); + assert_eq!( + parse_shell_lc_single_command_prefix(&command), + Some(vec!["echo".to_string(), "hello".to_string()]) + ); } #[test] @@ -548,6 +534,16 @@ mod tests { ); } + #[test] + fn parse_shell_lc_single_command_prefix_rejects_herestring_with_chaining() { + let command = vec![ + "bash".to_string(), + "-lc".to_string(), + r#"echo hello > /tmp/out.txt && cat /tmp/out.txt"#.to_string(), + ]; + assert_eq!(parse_shell_lc_single_command_prefix(&command), None); + } + #[test] fn parse_shell_lc_single_command_prefix_rejects_herestring_with_substitution() { let command = vec![