diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 8f31f150b33a..7aae784d34c1 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -24,7 +24,8 @@ use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::FileSystemSandboxKind; use codex_protocol::protocol::AskForApproval; -use codex_shell_command::is_dangerous_command::command_might_be_dangerous; +use codex_shell_command::is_dangerous_command::DangerousCommandMatch; +use codex_shell_command::is_dangerous_command::dangerous_command_match; use codex_shell_command::is_safe_command::is_known_safe_command; use thiserror::Error; use tokio::fs; @@ -325,16 +326,34 @@ impl ExecPolicyManager { match evaluation.decision { Decision::Forbidden => ExecApprovalRequirement::Forbidden { - reason: derive_forbidden_reason(command, &evaluation), + reason: derive_forbidden_reason( + command, + &evaluation, + dangerous_command_match_for_heuristics( + &evaluation, + Decision::Forbidden, + command_origin, + ), + ), }, Decision::Prompt => { let prompt_is_rule = evaluation.matched_rules.iter().any(|rule_match| { is_policy_match(rule_match) && rule_match.decision() == Decision::Prompt }); match prompt_is_rejected_by_policy(approval_policy, prompt_is_rule) { - Some(reason) => ExecApprovalRequirement::Forbidden { + Some(reason) if prompt_is_rule => ExecApprovalRequirement::Forbidden { reason: reason.to_string(), }, + Some(reason) => ExecApprovalRequirement::Forbidden { + reason: derive_rejected_prompt_reason( + reason, + dangerous_command_match_for_heuristics( + &evaluation, + Decision::Prompt, + command_origin, + ), + ), + }, None => ExecApprovalRequirement::NeedsApproval { reason: derive_prompt_reason(command, &evaluation), proposed_execpolicy_amendment: requested_amendment.or_else(|| { @@ -630,11 +649,46 @@ pub async fn load_exec_policy(config_stack: &ConfigLayerStack) -> Result Option { + match command_origin { + ExecPolicyCommandOrigin::Generic => dangerous_command_match(command), + #[cfg(windows)] + ExecPolicyCommandOrigin::PowerShell => { + codex_shell_command::is_dangerous_command::dangerous_powershell_words_match(command) + } + } +} + +/// Extract DangerousCommandMatch from an Evaluation +fn dangerous_command_match_for_heuristics( + evaluation: &Evaluation, + decision: Decision, + command_origin: ExecPolicyCommandOrigin, +) -> Option { + evaluation + .matched_rules + .iter() + .find_map(|rule_match| match rule_match { + RuleMatch::HeuristicsRuleMatch { + command, + decision: matched_decision, + } if *matched_decision == decision => { + dangerous_command_match_for_origin(command, command_origin) + } + _ => None, + }) +} + /// If a command is not matched by any execpolicy rule, derive a [`Decision`]. pub(crate) fn render_decision_for_unmatched_command( command: &[String], context: UnmatchedCommandContext<'_>, ) -> Decision { + let dangerous_command_match = + dangerous_command_match_for_origin(command, context.command_origin); let UnmatchedCommandContext { approval_policy, permission_profile, @@ -674,27 +728,10 @@ pub(crate) fn render_decision_for_unmatched_command( // We prefer to prompt the user rather than outright forbid the command, // but if the user has explicitly disabled prompts, we must // forbid the command. - let command_is_dangerous = match command_origin { - ExecPolicyCommandOrigin::Generic => command_might_be_dangerous(command), - #[cfg(windows)] - ExecPolicyCommandOrigin::PowerShell => { - codex_shell_command::is_dangerous_command::is_dangerous_powershell_words(command) - } - }; - if command_is_dangerous || windows_managed_fs_restrictions_without_sandbox_backend { + if dangerous_command_match.is_some() || windows_managed_fs_restrictions_without_sandbox_backend + { return match approval_policy { - AskForApproval::Never => { - let sandbox_is_explicitly_disabled = matches!( - permission_profile, - PermissionProfile::Disabled | PermissionProfile::External { .. } - ); - if sandbox_is_explicitly_disabled { - // If the sandbox is explicitly disabled, we should allow the command to run - Decision::Allow - } else { - Decision::Forbidden - } - } + AskForApproval::Never => Decision::Forbidden, AskForApproval::OnRequest | AskForApproval::UnlessTrusted | AskForApproval::Granular(_) => Decision::Prompt, @@ -955,7 +992,11 @@ fn render_shlex_command(args: &[String]) -> String { /// Derive a string explaining why the command was forbidden. If `justification` /// is set by the user, this can contain instructions with recommended /// alternatives, for example. -fn derive_forbidden_reason(command_args: &[String], evaluation: &Evaluation) -> String { +fn derive_forbidden_reason( + command_args: &[String], + evaluation: &Evaluation, + dangerous_command_match: Option, +) -> String { let command = render_shlex_command(command_args); let most_specific_forbidden = evaluation @@ -980,7 +1021,37 @@ fn derive_forbidden_reason(command_args: &[String], evaluation: &Evaluation) -> let prefix = render_shlex_command(matched_prefix); format!("`{command}` rejected: policy forbids commands starting with `{prefix}`") } - None => format!("`{command}` rejected: blocked by policy"), + None => { + if let Some(dangerous_command_match) = dangerous_command_match { + let reason = dangerous_command_rejection_reason(dangerous_command_match); + format!("`{command}` rejected: {reason}") + } else { + format!("`{command}` rejected: blocked by policy") + } + } + } +} + +fn derive_rejected_prompt_reason( + fallback_reason: &str, + dangerous_command_match: Option, +) -> String { + match dangerous_command_match { + Some(dangerous_command_match @ DangerousCommandMatch::ForcedRm) => { + dangerous_command_rejection_reason(dangerous_command_match).to_string() + } + Some(DangerousCommandMatch::Other) | None => fallback_reason.to_string(), + } +} + +fn dangerous_command_rejection_reason( + dangerous_command_match: DangerousCommandMatch, +) -> &'static str { + match dangerous_command_match { + DangerousCommandMatch::ForcedRm => { + "rm -f style commands are not permitted. Use a safer approach" + } + DangerousCommandMatch::Other => "blocked by policy", } } diff --git a/codex-rs/core/src/exec_policy_tests.rs b/codex-rs/core/src/exec_policy_tests.rs index 95d29e0bacb1..4b47cdab3d49 100644 --- a/codex-rs/core/src/exec_policy_tests.rs +++ b/codex-rs/core/src/exec_policy_tests.rs @@ -224,7 +224,7 @@ async fn returns_empty_policy_when_no_policy_files_exist() { decision: Decision::Allow, matched_rules: vec![RuleMatch::HeuristicsRuleMatch { command: vec!["rm".to_string()], - decision: Decision::Allow + decision: Decision::Allow, }], }, policy.check_multiple(commands.iter(), &|_| Decision::Allow) @@ -481,7 +481,7 @@ async fn ignores_policies_outside_policy_dir() { decision: Decision::Allow, matched_rules: vec![RuleMatch::HeuristicsRuleMatch { command: vec!["ls".to_string()], - decision: Decision::Allow + decision: Decision::Allow, }], }, policy.check_multiple(command.iter(), &|_| Decision::Allow) @@ -1346,6 +1346,28 @@ async fn exec_approval_requirement_rejects_unmatched_sandbox_escalation_when_gra .await; } +#[test] +fn other_danger_preserves_rejected_prompt_reason() { + assert_eq!( + derive_rejected_prompt_reason( + REJECT_SANDBOX_APPROVAL_REASON, + Some(DangerousCommandMatch::Other), + ), + REJECT_SANDBOX_APPROVAL_REASON + ); +} + +#[test] +fn forced_rm_rejected_prompt_reason_does_not_repeat_command() { + assert_eq!( + derive_rejected_prompt_reason( + REJECT_SANDBOX_APPROVAL_REASON, + Some(DangerousCommandMatch::ForcedRm), + ), + "rm -f style commands are not permitted. Use a safer approach" + ); +} + #[tokio::test] async fn mixed_rule_and_sandbox_prompt_prioritizes_rule_for_rejection_decision() { let policy_src = r#"prefix_rule(pattern=["git"], decision="prompt")"#; @@ -1384,7 +1406,7 @@ async fn mixed_rule_and_sandbox_prompt_prioritizes_rule_for_rejection_decision() } #[tokio::test] -async fn mixed_rule_and_sandbox_prompt_rejects_when_granular_rules_are_disabled() { +async fn forced_rm_preserves_rule_rejection_when_granular_rules_are_disabled() { let policy_src = r#"prefix_rule(pattern=["git"], decision="prompt")"#; let mut parser = PolicyParser::new(); parser @@ -1394,7 +1416,7 @@ async fn mixed_rule_and_sandbox_prompt_rejects_when_granular_rules_are_disabled( let command = vec![ "bash".to_string(), "-lc".to_string(), - "git status && madeup-cmd".to_string(), + "git status && rm -rf /tmp/example".to_string(), ]; let requirement = manager @@ -2015,10 +2037,85 @@ async fn dangerous_rm_rf_requires_approval_in_danger_full_access() { .await; } +#[tokio::test] +async fn dangerous_rm_rf_in_shell_loop_requires_approval_in_danger_full_access() { + let command = vec_str(&[ + "bash", + "-lc", + "for target in /tmp/a /tmp/b; do rm -rf \"$target\"; done", + ]); + + assert_exec_approval_requirement_for_command( + ExecApprovalRequirementScenario { + policy_src: None, + command: command.clone(), + approval_policy: AskForApproval::OnRequest, + permission_profile: PermissionProfile::Disabled, + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: None, + }, + ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(command)), + }, + ) + .await; +} + fn vec_str(items: &[&str]) -> Vec { items.iter().map(std::string::ToString::to_string).collect() } +#[tokio::test] +async fn forced_rm_requires_approval_or_specific_rejection_on_all_platforms() { + let policy = ExecPolicyManager::new(Arc::new(Policy::empty())); + let permissions = SandboxPermissions::UseDefault; + let dangerous_command = vec_str(&["rm", "-rf", "/important/data"]); + assert_eq!( + ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec_str(&[ + "rm", + "-rf", + "/important/data", + ]))), + }, + policy + .create_exec_approval_requirement_for_command(ExecApprovalRequest { + command: &dangerous_command, + approval_policy: AskForApproval::OnRequest, + permission_profile: PermissionProfile::read_only(), + windows_sandbox_level: WindowsSandboxLevel::Disabled, + sandbox_permissions: permissions, + prefix_rule: None, + }) + .await, + r#"On all platforms, a forbidden command should require approval + (unless AskForApproval::Never is specified)."# + ); + + // A dangerous command should be forbidden if the user has specified + // AskForApproval::Never. + assert_eq!( + ExecApprovalRequirement::Forbidden { + reason: "`rm -rf /important/data` rejected: rm -f style commands are not permitted. Use a safer approach" + .to_string(), + }, + policy + .create_exec_approval_requirement_for_command(ExecApprovalRequest { + command: &dangerous_command, + approval_policy: AskForApproval::Never, + permission_profile: PermissionProfile::read_only(), + windows_sandbox_level: WindowsSandboxLevel::Disabled, + sandbox_permissions: permissions, + prefix_rule: None, + }) + .await, + r#"On all platforms, a forbidden command should require approval + (unless AskForApproval::Never is specified)."# + ); +} + /// Note this test behaves differently on Windows because it exercises an /// `if cfg!(windows)` code path in render_decision_for_unmatched_command(). #[tokio::test] @@ -2074,55 +2171,10 @@ async fn verify_approval_requirement_for_unsafe_powershell_command() { .await, "{pwsh_approval_reason}" ); - - // This is flagged as a dangerous command on all platforms. - let dangerous_command = vec_str(&["rm", "-rf", "/important/data"]); - assert_eq!( - ExecApprovalRequirement::NeedsApproval { - reason: None, - proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec_str(&[ - "rm", - "-rf", - "/important/data", - ]))), - }, - policy - .create_exec_approval_requirement_for_command(ExecApprovalRequest { - command: &dangerous_command, - approval_policy: AskForApproval::OnRequest, - permission_profile: PermissionProfile::read_only(), - windows_sandbox_level: WindowsSandboxLevel::Disabled, - sandbox_permissions: permissions, - prefix_rule: None, - }) - .await, - r#"On all platforms, a forbidden command should require approval - (unless AskForApproval::Never is specified)."# - ); - - // A dangerous command should be forbidden if the user has specified - // AskForApproval::Never. - assert_eq!( - ExecApprovalRequirement::Forbidden { - reason: "`rm -rf /important/data` rejected: blocked by policy".to_string(), - }, - policy - .create_exec_approval_requirement_for_command(ExecApprovalRequest { - command: &dangerous_command, - approval_policy: AskForApproval::Never, - permission_profile: PermissionProfile::read_only(), - windows_sandbox_level: WindowsSandboxLevel::Disabled, - sandbox_permissions: permissions, - prefix_rule: None, - }) - .await, - r#"On all platforms, a forbidden command should require approval - (unless AskForApproval::Never is specified)."# - ); } #[tokio::test] -async fn dangerous_command_allowed_when_sandbox_is_explicitly_disabled() { +async fn dangerous_command_forbidden_when_sandbox_is_explicitly_disabled() { let command = vec_str(&["rm", "-rf", "/tmp/nonexistent"]); assert_exec_approval_requirement_for_command( ExecApprovalRequirementScenario { @@ -2135,11 +2187,9 @@ async fn dangerous_command_allowed_when_sandbox_is_explicitly_disabled() { sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, - ExecApprovalRequirement::Skip { - bypass_sandbox: false, - proposed_execpolicy_amendment: Some(ExecPolicyAmendment { - command: vec_str(&["rm", "-rf", "/tmp/nonexistent"]), - }), + ExecApprovalRequirement::Forbidden { + reason: "`rm -rf /tmp/nonexistent` rejected: rm -f style commands are not permitted. Use a safer approach" + .to_string(), }, ) .await; diff --git a/codex-rs/core/tests/suite/exec_policy.rs b/codex-rs/core/tests/suite/exec_policy.rs index 0c73b74ab29e..b699f6db1b0a 100644 --- a/codex-rs/core/tests/suite/exec_policy.rs +++ b/codex-rs/core/tests/suite/exec_policy.rs @@ -9,7 +9,9 @@ use codex_protocol::config_types::Settings; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::GranularApprovalConfig; use codex_protocol::protocol::Op; +use codex_protocol::protocol::ReviewDecision; use codex_protocol::user_input::UserInput; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; @@ -27,6 +29,8 @@ use serde_json::Value; use serde_json::json; use std::fs; +const COMPLEX_FORCED_RM_COMMAND: &str = "for target in \"\"; do rm -rf \"$target\"; done"; + fn collaboration_mode_for_model(model: String) -> CollaborationMode { CollaborationMode { mode: ModeKind::Default, @@ -90,6 +94,146 @@ fn assert_no_matched_rules_invariant(output_item: &Value) { ); } +#[tokio::test] +async fn granular_complex_forced_rm_denial_explains_why_the_command_was_rejected() -> Result<()> { + skip_if_target_windows!(Ok(()), "uses a POSIX shell command fixture"); + + let server = start_mock_server().await; + let mut builder = test_codex(); + let test = builder.build_with_auto_env(&server).await?; + let call_id = "forced-rm-denied"; + let args = json!({ + "command": COMPLEX_FORCED_RM_COMMAND, + "timeout_ms": 1_000, + }); + + mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-forced-rm-1"), + ev_function_call(call_id, "shell_command", &serde_json::to_string(&args)?), + ev_completed("resp-forced-rm-1"), + ]), + ) + .await; + let results_mock = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-forced-rm-1", "done"), + ev_completed("resp-forced-rm-2"), + ]), + ) + .await; + + submit_user_turn( + &test, + "run the forced rm loop", + AskForApproval::Granular(GranularApprovalConfig { + sandbox_approval: false, + rules: true, + skill_approval: true, + request_permissions: true, + mcp_elicitations: true, + }), + PermissionProfile::read_only(), + /*collaboration_mode*/ None, + ) + .await?; + + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + let output_item = results_mock.single_request().function_call_output(call_id); + let output = output_item + .get("output") + .and_then(Value::as_str) + .expect("function call output should include a string output payload"); + assert!( + output.contains("rm -f style commands are not permitted. Use a safer approach"), + "unexpected output: {output}" + ); + + Ok(()) +} + +#[tokio::test] +async fn granular_complex_forced_rm_requests_approval_when_allowed() -> Result<()> { + skip_if_target_windows!(Ok(()), "uses a POSIX shell command fixture"); + + let server = start_mock_server().await; + let mut builder = test_codex(); + let test = builder.build_with_auto_env(&server).await?; + let call_id = "forced-rm-approval"; + let args = json!({ + "command": COMPLEX_FORCED_RM_COMMAND, + "timeout_ms": 1_000, + }); + + mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-forced-rm-approval-1"), + ev_function_call(call_id, "shell_command", &serde_json::to_string(&args)?), + ev_completed("resp-forced-rm-approval-1"), + ]), + ) + .await; + mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-forced-rm-approval-1", "done"), + ev_completed("resp-forced-rm-approval-2"), + ]), + ) + .await; + + submit_user_turn( + &test, + "run the forced rm loop", + AskForApproval::Granular(GranularApprovalConfig { + sandbox_approval: true, + rules: true, + skill_approval: true, + request_permissions: true, + mcp_elicitations: true, + }), + PermissionProfile::read_only(), + /*collaboration_mode*/ None, + ) + .await?; + + let approval_event = wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_) + ) + }) + .await; + let EventMsg::ExecApprovalRequest(approval) = approval_event else { + panic!("expected forced rm to request approval before turn completion"); + }; + assert_eq!( + approval.command.last().map(String::as_str), + Some(COMPLEX_FORCED_RM_COMMAND) + ); + + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::Denied, + }) + .await?; + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + Ok(()) +} + #[cfg(windows)] #[tokio::test] async fn unified_exec_disabled_windows_sandbox_rejects_managed_read_only_command() -> Result<()> { diff --git a/codex-rs/shell-command/src/bash.rs b/codex-rs/shell-command/src/bash.rs index 007fbf956c03..10da75fb65dd 100644 --- a/codex-rs/shell-command/src/bash.rs +++ b/codex-rs/shell-command/src/bash.rs @@ -123,6 +123,39 @@ pub fn parse_shell_lc_plain_commands(command: &[String]) -> Option Option>> { + let (_, script) = extract_bash_command(command)?; + let tree = try_parse_shell(script)?; + let root = tree.root_node(); + if root.has_error() { + return None; + } + + let mut commands = Vec::new(); + let mut stack = vec![root]; + while let Some(node) = stack.pop() { + if node.kind() == "command" + && let Some(command) = parse_literal_command_from_node(node, script) + { + commands.push(command); + } + + let mut cursor = node.walk(); + for child in node.named_children(&mut cursor) { + stack.push(child); + } + } + + Some(commands) +} + /// Returns the parsed argv for a single shell command in a here-doc style /// script (`<<`), as long as the script contains exactly one command node. pub fn parse_shell_lc_single_command_prefix(command: &[String]) -> Option> { @@ -201,6 +234,46 @@ fn parse_plain_command_from_node(cmd: tree_sitter::Node, src: &str) -> Option, src: &str) -> Option> { + if cmd.kind() != "command" { + return None; + } + + let mut words = Vec::new(); + let mut found_command_name = false; + let mut cursor = cmd.walk(); + for child in cmd.named_children(&mut cursor) { + if child.kind() == "command_name" { + let command_name = parse_literal_shell_word(child.named_child(0)?, src)?; + words.push(command_name); + found_command_name = true; + } else if found_command_name && let Some(word) = parse_literal_shell_word(child, src) { + words.push(word); + } + } + + found_command_name.then_some(words) +} + +fn parse_literal_shell_word(node: Node<'_>, src: &str) -> Option { + match node.kind() { + "word" | "number" if is_literal_word_or_number(node) => { + Some(node.utf8_text(src.as_bytes()).ok()?.to_owned()) + } + "string" => parse_double_quoted_string(node, src), + "raw_string" => parse_raw_string(node, src), + "concatenation" => { + let mut concatenated = String::new(); + let mut cursor = node.walk(); + for part in node.named_children(&mut cursor) { + concatenated.push_str(&parse_literal_shell_word(part, src)?); + } + (!concatenated.is_empty()).then_some(concatenated) + } + _ => None, + } +} + fn parse_heredoc_command_words(cmd: Node<'_>, src: &str) -> Option> { if cmd.kind() != "command" { return None; diff --git a/codex-rs/shell-command/src/command_safety/is_dangerous_command.rs b/codex-rs/shell-command/src/command_safety/is_dangerous_command.rs index fef98e836243..0574fcf7d76e 100644 --- a/codex-rs/shell-command/src/command_safety/is_dangerous_command.rs +++ b/codex-rs/shell-command/src/command_safety/is_dangerous_command.rs @@ -1,45 +1,69 @@ -use crate::bash::parse_shell_lc_plain_commands; +use crate::bash::parse_shell_lc_literal_commands; use std::path::Path; #[cfg(windows)] #[path = "windows_dangerous_commands.rs"] mod windows_dangerous_commands; -pub fn command_might_be_dangerous(command: &[String]) -> bool { - #[cfg(windows)] - { - if windows_dangerous_commands::is_dangerous_command_windows(command) { - return true; - } +/// Identifies the dangerous-command rule matched by a command invocation. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum DangerousCommandMatch { + /// An `rm` invocation includes the force option. + ForcedRm, + /// Another dangerous-command rule matched. + Other, +} + +const MAX_DANGEROUS_COMMAND_WRAPPER_DEPTH: usize = 8; + +/// Returns the dangerous-command rule matched by an already-tokenized command. +pub fn dangerous_command_match(command: &[String]) -> Option { + dangerous_command_match_with_depth(command, /*wrapper_depth*/ 0) +} + +fn dangerous_command_match_with_depth( + command: &[String], + wrapper_depth: usize, +) -> Option { + if wrapper_depth > MAX_DANGEROUS_COMMAND_WRAPPER_DEPTH { + return None; } - if is_dangerous_to_call_with_exec(command) { - return true; + if let Some(dangerous_match) = dangerous_command_match_for_exec(command, wrapper_depth) { + return Some(dangerous_match); } - // Support `bash -lc "