From af089fb21df3022d3bd64b2e5dd95291c1b8cc1e Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Thu, 30 Apr 2026 18:05:02 -0700 Subject: [PATCH] fix(exec_policy) heredoc parsing file_redirect (#20113) ## Summary Fixes a regression introduced in #10941 so that heredocs do not permit file redirects to be approved by rules, and adds scenario tests to cover this behavior. Previously, heredoc command parsing would allow redirects and environment variables: ```bash # commands_for_exec_policy() would parse this via parse_shell_lc_single_command_prefix PATH=/tmp/bad:$PATH cat <<'EOF' > /tmp/bad/hello.txt hello EOF ``` This conflicts with the Codex Rules documentation; heredoc parsing logic should abide by the same strictness of parsing. ## Tests - [x] Updated unit tests accordingly - [x] Added scenario tests for these cases --------- Co-authored-by: Codex --- codex-rs/core/src/exec_policy.rs | 20 +-- codex-rs/core/src/exec_policy_tests.rs | 121 ++++++++++++++++++ codex-rs/core/tests/suite/approvals.rs | 162 ++++++++++++++++++++++++- codex-rs/shell-command/src/bash.rs | 29 +++-- 4 files changed, 314 insertions(+), 18 deletions(-) diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 57e85cece..3574ee77f 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -315,14 +315,18 @@ impl ExecPolicyManager { &match_options, ); - let requested_amendment = derive_requested_execpolicy_amendment_from_prefix_rule( - prefix_rule.as_ref(), - &evaluation.matched_rules, - exec_policy.as_ref(), - &commands, - &exec_policy_fallback, - &match_options, - ); + let requested_amendment = if auto_amendment_allowed { + derive_requested_execpolicy_amendment_from_prefix_rule( + prefix_rule.as_ref(), + &evaluation.matched_rules, + exec_policy.as_ref(), + &commands, + &exec_policy_fallback, + &match_options, + ) + } else { + None + }; match evaluation.decision { Decision::Forbidden => ExecApprovalRequirement::Forbidden { diff --git a/codex-rs/core/src/exec_policy_tests.rs b/codex-rs/core/src/exec_policy_tests.rs index 9463b85f8..96c32c449 100644 --- a/codex-rs/core/src/exec_policy_tests.rs +++ b/codex-rs/core/src/exec_policy_tests.rs @@ -809,6 +809,127 @@ async fn drops_requested_amendment_for_heredoc_fallback_prompts_when_it_wont_mat .await; } +#[tokio::test] +async fn drops_requested_amendment_for_heredoc_fallback_prompts_when_it_matches() { + assert_exec_approval_requirement_for_command( + ExecApprovalRequirementScenario { + policy_src: None, + command: vec![ + "bash".to_string(), + "-lc".to_string(), + "python3 <<'PY'\nprint('hello')\nPY".to_string(), + ], + 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: Some(vec!["python3".to_string()]), + }, + ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: None, + }, + ) + .await; +} + +#[tokio::test] +#[cfg(not(windows))] +async fn heredoc_with_variable_assignment_is_not_reduced_to_allowed_prefix() { + assert_exec_approval_requirement_for_command( + ExecApprovalRequirementScenario { + policy_src: Some(r#"prefix_rule(pattern=["cat"], decision="allow")"#.to_string()), + command: vec![ + "bash".to_string(), + "-lc".to_string(), + "PATH=/tmp/evil:$PATH cat <<'EOF'\nhello\nEOF".to_string(), + ], + approval_policy: AskForApproval::OnRequest, + sandbox_policy: SandboxPolicy::new_read_only_policy(), + file_system_sandbox_policy: read_only_file_system_sandbox_policy(), + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: None, + }, + ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![ + "bash".to_string(), + "-lc".to_string(), + "PATH=/tmp/evil:$PATH cat <<'EOF'\nhello\nEOF".to_string(), + ])), + }, + ) + .await; +} + +#[tokio::test] +async fn heredoc_redirect_without_escalation_runs_inside_sandbox() { + assert_exec_approval_requirement_for_command( + ExecApprovalRequirementScenario { + policy_src: None, + command: vec![ + "zsh".to_string(), + "-lc".to_string(), + r#"cat <<'EOF' > /some/important/folder/test.txt +hello world +EOF"# + .to_string(), + ], + approval_policy: AskForApproval::OnRequest, + sandbox_policy: SandboxPolicy::new_workspace_write_policy(), + file_system_sandbox_policy: workspace_write_file_system_sandbox_policy(), + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: None, + }, + ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![ + "zsh".to_string(), + "-lc".to_string(), + r#"cat <<'EOF' > /some/important/folder/test.txt +hello world +EOF"# + .to_string(), + ])), + }, + ) + .await; +} + +#[tokio::test] +async fn heredoc_redirect_with_escalation_requires_approval() { + assert_exec_approval_requirement_for_command( + ExecApprovalRequirementScenario { + policy_src: Some(r#"prefix_rule(pattern=["cat"], decision="allow")"#.to_string()), + command: vec![ + "zsh".to_string(), + "-lc".to_string(), + r#"cat <<'EOF' > /some/important/folder/test.txt +hello world +EOF"# + .to_string(), + ], + approval_policy: AskForApproval::OnRequest, + sandbox_policy: SandboxPolicy::new_workspace_write_policy(), + file_system_sandbox_policy: workspace_write_file_system_sandbox_policy(), + sandbox_permissions: SandboxPermissions::RequireEscalated, + prefix_rule: None, + }, + ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![ + "zsh".to_string(), + "-lc".to_string(), + r#"cat <<'EOF' > /some/important/folder/test.txt +hello world +EOF"# + .to_string(), + ])), + }, + ) + .await; +} + #[tokio::test] async fn justification_is_included_in_forbidden_exec_approval_requirement() { assert_exec_approval_requirement_for_command( diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index 79bf0f830..96cc1f3a9 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -96,6 +96,14 @@ enum ActionKind { RunCommand { command: &'static str, }, + RunCommandWithPolicy { + command: &'static str, + policy_src: &'static str, + }, + RunCommandWithPrefixRule { + command: &'static str, + prefix_rule: &'static [&'static str], + }, RunUnifiedExecCommand { command: &'static str, justification: Option<&'static str>, @@ -114,6 +122,20 @@ const DEFAULT_UNIFIED_EXEC_JUSTIFICATION: &str = "Requires escalated permissions to bypass the sandbox in tests."; impl ActionKind { + fn policy_src(&self) -> Option<&'static str> { + match self { + ActionKind::RunCommandWithPolicy { policy_src, .. } => Some(*policy_src), + ActionKind::WriteFile { .. } + | ActionKind::FetchUrlNoProxy { .. } + | ActionKind::FetchUrl { .. } + | ActionKind::RunCommand { .. } + | ActionKind::RunCommandWithPrefixRule { .. } + | ActionKind::RunUnifiedExecCommand { .. } + | ActionKind::ApplyPatchFunction { .. } + | ActionKind::ApplyPatchShell { .. } => None, + } + } + async fn prepare( &self, test: &TestCodex, @@ -204,6 +226,31 @@ impl ActionKind { )?; Ok((event, Some(command.to_string()))) } + ActionKind::RunCommandWithPolicy { command, .. } => { + // Bazel Linux runners can be heavily oversubscribed while this + // matrix runs, so avoid making scheduling latency look like an + // approval behavior failure. + let event = shell_event( + call_id, + command, + /*timeout_ms*/ 30_000, + sandbox_permissions, + )?; + Ok((event, Some(command.to_string()))) + } + ActionKind::RunCommandWithPrefixRule { + command, + prefix_rule, + } => { + let event = shell_event_with_prefix_rule( + call_id, + command, + /*timeout_ms*/ 30_000, + sandbox_permissions, + Some(prefix_rule.iter().map(|part| (*part).to_string()).collect()), + )?; + Ok((event, Some(command.to_string()))) + } ActionKind::RunUnifiedExecCommand { command, justification, @@ -549,6 +596,11 @@ enum Outcome { decision: ReviewDecision, expected_reason: Option<&'static str>, }, + ExecApprovalWithAmendment { + decision: ReviewDecision, + expected_reason: Option<&'static str>, + expected_execpolicy_amendment: Option<&'static [&'static str]>, + }, PatchApproval { decision: ReviewDecision, expected_reason: Option<&'static str>, @@ -913,6 +965,70 @@ fn scenarios() -> Vec { output_contains: "rejected by user", }, }, + ScenarioSpec { + name: "cat_heredoc_file_redirect_prefix_rule_requires_escalation_approval", + approval_policy: OnRequest, + sandbox_policy: workspace_write(false), + action: ActionKind::RunCommandWithPrefixRule { + command: r#"cat <<'EOF' > /tmp/out.txt + hello + EOF"#, + prefix_rule: &["cat"], + }, + sandbox_permissions: SandboxPermissions::RequireEscalated, + features: vec![], + model_override: Some("gpt-5.2"), + outcome: Outcome::ExecApproval { + decision: ReviewDecision::Denied, + expected_reason: None, + }, + expectation: Expectation::CommandFailure { + output_contains: "rejected by user", + }, + }, + ScenarioSpec { + name: "cat_heredoc_variable_assignment_policy_requires_escalation_approval", + approval_policy: OnRequest, + sandbox_policy: workspace_write(false), + action: ActionKind::RunCommandWithPolicy { + command: r#"PATH=/tmp/evil:$PATH cat <<'EOF' + hello + EOF"#, + policy_src: r#"prefix_rule(pattern=["cat"], decision="allow")"#, + }, + sandbox_permissions: SandboxPermissions::RequireEscalated, + features: vec![], + model_override: Some("gpt-5.2"), + outcome: Outcome::ExecApproval { + decision: ReviewDecision::Denied, + expected_reason: None, + }, + expectation: Expectation::CommandFailure { + output_contains: "rejected by user", + }, + }, + ScenarioSpec { + name: "python_heredoc_requested_prefix_rule_omits_amendment", + approval_policy: OnRequest, + sandbox_policy: workspace_write(false), + action: ActionKind::RunCommandWithPrefixRule { + command: r#"python3 <<'PY' + print('hello') + PY"#, + prefix_rule: &["python3"], + }, + sandbox_permissions: SandboxPermissions::RequireEscalated, + features: vec![], + model_override: Some("gpt-5.2"), + outcome: Outcome::ExecApprovalWithAmendment { + decision: ReviewDecision::Denied, + expected_reason: None, + expected_execpolicy_amendment: None, + }, + expectation: Expectation::CommandFailure { + output_contains: "rejected by user", + }, + }, ScenarioSpec { name: "danger_full_access_on_failure_allows_outside_write", approval_policy: OnFailure, @@ -1703,7 +1819,9 @@ fn scenario_group(scenario: &ScenarioSpec) -> ScenarioGroup { ActionKind::WriteFile { .. } | ActionKind::FetchUrlNoProxy { .. } | ActionKind::FetchUrl { .. } - | ActionKind::RunCommand { .. } => match &scenario.sandbox_policy { + | ActionKind::RunCommand { .. } + | ActionKind::RunCommandWithPolicy { .. } + | ActionKind::RunCommandWithPrefixRule { .. } => match &scenario.sandbox_policy { SandboxPolicy::DangerFullAccess => ScenarioGroup::DangerFullAccess, SandboxPolicy::ReadOnly { .. } => ScenarioGroup::ReadOnly, SandboxPolicy::WorkspaceWrite { .. } => ScenarioGroup::WorkspaceWrite, @@ -1720,6 +1838,7 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> { let features = scenario.features.clone(); let model_override = scenario.model_override; let model = model_override.unwrap_or("gpt-5.4"); + let policy_src = scenario.action.policy_src(); let mut builder = test_codex().with_model(model).with_config(move |config| { config.permissions.approval_policy = Constrained::allow_any(approval_policy); @@ -1733,6 +1852,13 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> { .expect("test config should allow feature update"); } }); + if let Some(policy_src) = policy_src { + builder = builder.with_pre_build_hook(move |home| { + let rules_dir = home.join("rules"); + fs::create_dir_all(&rules_dir).expect("create rules dir"); + fs::write(rules_dir.join("default.rules"), policy_src).expect("write policy"); + }); + } let test = builder.build(&server).await?; let call_id = scenario.name; @@ -1799,6 +1925,40 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> { .await?; wait_for_completion(&test).await; } + Outcome::ExecApprovalWithAmendment { + decision, + expected_reason, + expected_execpolicy_amendment, + } => { + let command = expected_command + .as_deref() + .expect("exec approval requires shell command"); + let approval = expect_exec_approval(&test, command).await; + if let Some(expected_reason) = expected_reason { + assert_eq!( + approval.reason.as_deref(), + Some(*expected_reason), + "unexpected approval reason for {}", + scenario.name + ); + } + let expected_execpolicy_amendment = expected_execpolicy_amendment.map(|command| { + ExecPolicyAmendment::new(command.iter().map(|part| (*part).to_string()).collect()) + }); + assert_eq!( + approval.proposed_execpolicy_amendment, expected_execpolicy_amendment, + "unexpected execpolicy amendment for {}", + scenario.name + ); + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: decision.clone(), + }) + .await?; + wait_for_completion(&test).await; + } Outcome::PatchApproval { decision, expected_reason, diff --git a/codex-rs/shell-command/src/bash.rs b/codex-rs/shell-command/src/bash.rs index 2fe299108..60ee5c420 100644 --- a/codex-rs/shell-command/src/bash.rs +++ b/codex-rs/shell-command/src/bash.rs @@ -131,6 +131,9 @@ pub fn parse_shell_lc_single_command_prefix(command: &[String]) -> Option, src: &str) -> Option> } words.push(child.utf8_text(src.as_bytes()).ok()?.to_owned()); } - // Allow shell constructs that attach IO to a single command without - // changing argv matching semantics for the executable prefix. - "variable_assignment" | "comment" => {} + // Allow heredoc constructs that attach stdin to a single command + // without changing argv matching semantics for the executable + // prefix. Other file redirects may write outside the sandbox and + // must not be collapsed to the executable prefix for execpolicy. + "comment" => {} kind if is_allowed_heredoc_attachment_kind(kind) => {} _ => return None, } @@ -244,7 +249,6 @@ fn is_allowed_heredoc_attachment_kind(kind: &str) -> bool { | "simple_heredoc_body" | "heredoc_redirect" | "herestring_redirect" - | "file_redirect" | "redirected_statement" ) } @@ -536,16 +540,23 @@ mod tests { } #[test] - fn parse_shell_lc_single_command_prefix_accepts_heredoc_with_extra_redirect() { + fn parse_shell_lc_single_command_prefix_rejects_heredoc_with_extra_file_redirect() { let command = vec![ "bash".to_string(), "-lc".to_string(), "python3 <<'PY' > /tmp/out.txt\nprint('hello')\nPY".to_string(), ]; - assert_eq!( - parse_shell_lc_single_command_prefix(&command), - Some(vec!["python3".to_string()]) - ); + assert_eq!(parse_shell_lc_single_command_prefix(&command), None); + } + + #[test] + fn parse_shell_lc_single_command_prefix_rejects_heredoc_with_variable_assignment() { + let command = vec![ + "bash".to_string(), + "-lc".to_string(), + "PATH=/tmp/evil:$PATH cat <<'EOF'\nhello\nEOF".to_string(), + ]; + assert_eq!(parse_shell_lc_single_command_prefix(&command), None); } #[test]