From f97e77569352a2bf5be9955e623edad0a15d9b93 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 18 Aug 2026 02:18:56 +0000 Subject: [PATCH] Fail closed on deeply nested command wrappers (#39122) ## Why Dangerous-command inspection has a bounded wrapper depth. Returning no match after that limit could let a nested dangerous payload escape policy detection. ## What changed - Classify commands beyond the wrapper-depth limit as dangerous. - Cover the boundary and overflow cases in the command-safety unit tests. - Verify that exec policy rejects a deeply `env`-wrapped forced `rm` before it can delete a sentinel file when approvals are disabled. GitOrigin-RevId: a6cdbbb7bdc6274a62745f668189fabaaf46745f --- codex-rs/core/tests/suite/exec_policy.rs | 62 +++++++++++++++++++ .../command_safety/is_dangerous_command.rs | 23 ++++++- 2 files changed, 84 insertions(+), 1 deletion(-) diff --git a/codex-rs/core/tests/suite/exec_policy.rs b/codex-rs/core/tests/suite/exec_policy.rs index 042b96f48c..9bd3e0b3ee 100644 --- a/codex-rs/core/tests/suite/exec_policy.rs +++ b/codex-rs/core/tests/suite/exec_policy.rs @@ -290,6 +290,68 @@ async fn granular_complex_forced_rm_requests_approval_when_allowed() -> Result<( Ok(()) } +#[tokio::test] +async fn deeply_nested_forced_rm_is_rejected_before_execution_when_approvals_are_disabled() +-> 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 sentinel = test.config.cwd.join("forced-rm-sentinel"); + fs::write(&sentinel, "must not be deleted")?; + let call_id = "deeply-nested-forced-rm"; + let args = json!({ + "command": "env env env env env env env env env rm -rf forced-rm-sentinel", + "timeout_ms": 1_000, + }); + + mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-deeply-nested-forced-rm-1"), + ev_function_call(call_id, "shell_command", &serde_json::to_string(&args)?), + ev_completed("resp-deeply-nested-forced-rm-1"), + ]), + ) + .await; + let results_mock = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-deeply-nested-forced-rm", "done"), + ev_completed("resp-deeply-nested-forced-rm-2"), + ]), + ) + .await; + + submit_user_turn( + &test, + "run the deeply nested forced rm", + AskForApproval::Never, + PermissionProfile::Disabled, + /*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("rejected: blocked by policy"), + "unexpected output: {output}" + ); + assert!(sentinel.exists(), "the rejected command must not execute"); + + 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/command_safety/is_dangerous_command.rs b/codex-rs/shell-command/src/command_safety/is_dangerous_command.rs index 0574fcf7d7..59fce97566 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 @@ -25,7 +25,7 @@ fn dangerous_command_match_with_depth( wrapper_depth: usize, ) -> Option { if wrapper_depth > MAX_DANGEROUS_COMMAND_WRAPPER_DEPTH { - return None; + return Some(DangerousCommandMatch::Other); } if let Some(dangerous_match) = dangerous_command_match_for_exec(command, wrapper_depth) { @@ -288,6 +288,27 @@ mod tests { } } + #[test] + fn deeply_nested_command_wrappers_fail_closed() { + for (depth, expected) in [ + ( + MAX_DANGEROUS_COMMAND_WRAPPER_DEPTH, + DangerousCommandMatch::ForcedRm, + ), + ( + MAX_DANGEROUS_COMMAND_WRAPPER_DEPTH + 1, + DangerousCommandMatch::Other, + ), + ] { + let command = std::iter::repeat_n("env", depth) + .chain(["rm", "-rf", "/tmp/example"]) + .map(str::to_owned) + .collect::>(); + + assert_eq!(dangerous_command_match(&command), Some(expected)); + } + } + #[test] fn forced_rm_in_complex_shell_syntax_is_dangerous() { for script in [