agent/codex
1
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix(exec_policy) heredoc parsing file_redirect (#20113)

Browse Source 
## 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 <noreply@openai.com>
This commit is contained in:
Dylan Hurd
2026-04-30 18:05:02 -07:00
committed by GitHub
Unverified
parent 4f96001fa7
commit af089fb21d
4 changed files with 314 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/exec_policy.rs
+12 -8
View File
@@ -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 {
codex-rs/core/src/exec_policy_tests.rs
+121
View File
@@ -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(
codex-rs/core/tests/suite/approvals.rs
+161 -1
View File
@@ -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<ScenarioSpec> {
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,
codex-rs/shell-command/src/bash.rs
+20 -9
View File
@@ -131,6 +131,9 @@ pub fn parse_shell_lc_single_command_prefix(command: &[String]) -> Option<Vec<St
if !has_named_descendant_kind(root, "heredoc_redirect") {
return None;
}
if has_named_descendant_kind(root, "file_redirect") {
return None;
}
let command_node = find_single_command_node(root)?;
parse_heredoc_command_words(command_node, script)
@@ -218,9 +221,11 @@ fn parse_heredoc_command_words(cmd: Node<'_>, src: &str) -> Option<Vec<String>>
}
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]