From 6d6c9718793ed0df7e8d0a04edf6b858e206bdb2 Mon Sep 17 00:00:00 2001 From: cpjet64 Date: Thu, 25 Sep 2025 01:30:42 -0400 Subject: [PATCH 1/4] Fix Windows session approval caching --- codex-rs/core/src/codex.rs | 48 ++++++++++++++++++++++++++++++++++---- 1 file changed, 43 insertions(+), 5 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 48aa3dbd7c..9740cb4915 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -671,9 +671,23 @@ impl Session { } } - pub async fn add_approved_command(&self, cmd: Vec) { - let mut state = self.state.lock().await; - state.add_approved_command(cmd); + pub async fn remember_approved_command( + &self, + command_for_display: &[String], + executed_command: &[String], + ) { + let same = command_for_display == executed_command; + let display_vec = command_for_display.to_vec(); + let executed_vec = if same { + None + } else { + Some(executed_command.to_vec()) + }; + + self.add_approved_command(display_vec).await; + if let Some(exec_vec) = executed_vec { + self.add_approved_command(exec_vec).await; + } } /// Records input items: always append to conversation history and @@ -2814,7 +2828,8 @@ async fn handle_container_exec_with_params( match decision { ReviewDecision::Approved => (), ReviewDecision::ApprovedForSession => { - sess.add_approved_command(params.command.clone()).await; + sess.remember_approved_command(&command_for_display, ¶ms.command) + .await; } ReviewDecision::Denied | ReviewDecision::Abort => { return Err(FunctionCallError::RespondToModel( @@ -2960,7 +2975,11 @@ async fn handle_sandbox_error( // remainder of the session so future // executions skip the sandbox directly. // TODO(ragona): Isn't this a bug? It always saves the command in an | fork? - sess.add_approved_command(params.command.clone()).await; + sess.remember_approved_command( + &exec_command_context.command_for_display, + ¶ms.command, + ) + .await; // Inform UI we are retrying without sandbox. sess.notify_background_event(&sub_id, "retrying command without sandbox") .await; @@ -3334,6 +3353,25 @@ mod tests { assert_eq!(expected, actual); } + #[test] + fn remember_approved_command_records_both_variants() { + let (session, _) = make_session_and_context(); + let original = vec!["bash".to_string(), "-lc".to_string(), "ls".to_string()]; + let translated = vec![ + "powershell.exe".to_string(), + "-NoProfile".to_string(), + "-Command".to_string(), + "bash -lc ls".to_string(), + ]; + + tokio_test::block_on(session.remember_approved_command(&original, &translated)); + + let approved = + tokio_test::block_on(async { session.state.lock().await.approved_commands.clone() }); + + assert!(approved.contains(&original)); + assert!(approved.contains(&translated)); + } #[test] fn prefers_structured_content_when_present() { let ctr = CallToolResult { From 9e612d48b649a0b1d18cfe6d3e4433df1d982874 Mon Sep 17 00:00:00 2001 From: cpjet64 Date: Thu, 25 Sep 2025 02:06:49 -0400 Subject: [PATCH 2/4] Remember translated command when caching session approvals --- codex-rs/core/src/codex.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 9740cb4915..976de49389 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -2813,6 +2813,7 @@ async fn handle_container_exec_with_params( } }; + let mut remember_for_session = false; let sandbox_type = match safety { SafetyCheck::AutoApprove { sandbox_type } => sandbox_type, SafetyCheck::AskUser => { @@ -2828,8 +2829,7 @@ async fn handle_container_exec_with_params( match decision { ReviewDecision::Approved => (), ReviewDecision::ApprovedForSession => { - sess.remember_approved_command(&command_for_display, ¶ms.command) - .await; + remember_for_session = true; } ReviewDecision::Denied | ReviewDecision::Abort => { return Err(FunctionCallError::RespondToModel( @@ -2867,6 +2867,12 @@ async fn handle_container_exec_with_params( }; let params = maybe_translate_shell_command(params, sess, turn_context); + + if remember_for_session { + sess.remember_approved_command(&command_for_display, ¶ms.command) + .await; + } + let output_result = sess .run_exec_with_events( turn_diff_tracker, From 5b1ef60f6d8bcff1d97f8650c0bf7a05018d94a9 Mon Sep 17 00:00:00 2001 From: cpjet64 Date: Thu, 25 Sep 2025 19:43:59 -0400 Subject: [PATCH 3/4] resolve merge issue --- codex-rs/core/src/codex.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 976de49389..981d1de608 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -671,6 +671,11 @@ impl Session { } } + pub async fn add_approved_command(&self, cmd: Vec) { + let mut state = self.state.lock().await; + state.add_approved_command(cmd); + } + pub async fn remember_approved_command( &self, command_for_display: &[String], @@ -3372,8 +3377,9 @@ mod tests { tokio_test::block_on(session.remember_approved_command(&original, &translated)); - let approved = - tokio_test::block_on(async { session.state.lock().await.approved_commands.clone() }); + let approved = tokio_test::block_on(async { + session.state.lock().await.approved_commands_ref().clone() + }); assert!(approved.contains(&original)); assert!(approved.contains(&translated)); From 3fdc81ff650cb5af6a1b5968834758ca0feb950d Mon Sep 17 00:00:00 2001 From: cpjet64 Date: Sat, 27 Sep 2025 23:37:49 -0400 Subject: [PATCH 4/4] Align Windows command allowlist with Unix --- .../command_safety/windows_safe_commands.rs | 172 +++++++++++++++++- 1 file changed, 162 insertions(+), 10 deletions(-) diff --git a/codex-rs/core/src/command_safety/windows_safe_commands.rs b/codex-rs/core/src/command_safety/windows_safe_commands.rs index c6e781f8ae..75ddb02813 100644 --- a/codex-rs/core/src/command_safety/windows_safe_commands.rs +++ b/codex-rs/core/src/command_safety/windows_safe_commands.rs @@ -1,25 +1,177 @@ -// This is a WIP. This will eventually contain a real list of common safe Windows commands. -pub fn is_safe_command_windows(_command: &[String]) -> bool { - false +pub fn is_safe_command_windows(command: &[String]) -> bool { + let cmd0 = command.first().map(String::as_str); + + match cmd0 { + #[rustfmt::skip] + Some( + "cat" | + "cd" | + "echo" | + "false" | + "grep" | + "head" | + "ls" | + "nl" | + "pwd" | + "tail" | + "true" | + "wc" | + "which") => true, + + Some("find") => { + #[rustfmt::skip] + const UNSAFE_FIND_OPTIONS: &[&str] = &[ + "-exec", "-execdir", "-ok", "-okdir", + "-delete", + "-fls", "-fprint", "-fprint0", "-fprintf", + ]; + + !command + .iter() + .any(|arg| UNSAFE_FIND_OPTIONS.contains(&arg.as_str())) + } + + Some("rg") => { + const UNSAFE_RIPGREP_OPTIONS_WITH_ARGS: &[&str] = &["--pre", "--hostname-bin"]; + const UNSAFE_RIPGREP_OPTIONS_WITHOUT_ARGS: &[&str] = &["--search-zip", "-z"]; + + !command.iter().any(|arg| { + UNSAFE_RIPGREP_OPTIONS_WITHOUT_ARGS.contains(&arg.as_str()) + || UNSAFE_RIPGREP_OPTIONS_WITH_ARGS + .iter() + .any(|&opt| arg == opt || arg.starts_with(&format!("{opt}="))) + }) + } + + Some("git") => matches!( + command.get(1).map(String::as_str), + Some("branch" | "status" | "log" | "diff" | "show"), + ), + + Some("cargo") if command.get(1).map(String::as_str) == Some("check") => true, + + Some("sed") + if { + command.len() == 4 + && command.get(1).map(String::as_str) == Some("-n") + && is_valid_sed_n_arg(command.get(2).map(String::as_str)) + && command.get(3).map(String::is_empty) == Some(false) + } => + { + true + } + + _ => false, + } +} + +fn is_valid_sed_n_arg(arg: Option<&str>) -> bool { + let s = match arg { + Some(s) => s, + None => return false, + }; + + let core = match s.strip_suffix('p') { + Some(rest) => rest, + None => return false, + }; + + let parts: Vec<&str> = core.split(',').collect(); + match parts.as_slice() { + [num] => !num.is_empty() && num.chars().all(|c| c.is_ascii_digit()), + [a, b] => { + !a.is_empty() + && !b.is_empty() + && a.chars().all(|c| c.is_ascii_digit()) + && b.chars().all(|c| c.is_ascii_digit()) + } + _ => false, + } } #[cfg(test)] mod tests { use super::is_safe_command_windows; + use std::string::ToString; fn vec_str(args: &[&str]) -> Vec { args.iter().map(ToString::to_string).collect() } #[test] - fn everything_is_unsafe() { - for cmd in [ - vec_str(&["powershell.exe", "-NoLogo", "-Command", "echo hello"]), - vec_str(&["copy", "foo", "bar"]), - vec_str(&["del", "file.txt"]), - vec_str(&["powershell.exe", "Get-ChildItem"]), + fn known_safe_examples() { + assert!(is_safe_command_windows(&vec_str(&["ls"]))); + assert!(is_safe_command_windows(&vec_str(&["git", "status"]))); + assert!(is_safe_command_windows(&vec_str(&[ + "sed", "-n", "1,5p", "file.txt" + ]))); + assert!(is_safe_command_windows(&vec_str(&[ + "nl", + "-nrz", + "Cargo.toml" + ]))); + + assert!(is_safe_command_windows(&vec_str(&[ + "find", ".", "-name", "file.txt" + ]))); + } + + #[test] + fn unknown_or_partial() { + assert!(!is_safe_command_windows(&vec_str(&["foo"]))); + assert!(!is_safe_command_windows(&vec_str(&["git", "fetch"]))); + assert!(!is_safe_command_windows(&vec_str(&[ + "sed", "-n", "xp", "file.txt" + ]))); + + for args in [ + vec_str(&["find", ".", "-name", "file.txt", "-exec", "rm", "{}", ";"]), + vec_str(&[ + "find", ".", "-name", "*.py", "-execdir", "python3", "{}", ";", + ]), + vec_str(&["find", ".", "-name", "file.txt", "-ok", "rm", "{}", ";"]), + vec_str(&["find", ".", "-name", "*.py", "-okdir", "python3", "{}", ";"]), + vec_str(&["find", ".", "-delete", "-name", "file.txt"]), + vec_str(&["find", ".", "-fls", "/etc/passwd"]), + vec_str(&["find", ".", "-fprint", "/etc/passwd"]), + vec_str(&["find", ".", "-fprint0", "/etc/passwd"]), + vec_str(&["find", ".", "-fprintf", "/root/suid.txt", "%#m %u %p\n"]), + ] { + assert!( + !is_safe_command_windows(&args), + "expected {args:?} to be unsafe" + ); + } + } + + #[test] + fn ripgrep_rules() { + assert!(is_safe_command_windows(&vec_str(&[ + "rg", + "Cargo.toml", + "-n" + ]))); + + for args in [ + vec_str(&["rg", "--search-zip", "files"]), + vec_str(&["rg", "-z", "files"]), + ] { + assert!( + !is_safe_command_windows(&args), + "expected {args:?} to be considered unsafe due to zip-search flag", + ); + } + + for args in [ + vec_str(&["rg", "--pre", "pwned", "files"]), + vec_str(&["rg", "--pre=pwned", "files"]), + vec_str(&["rg", "--hostname-bin", "pwned", "files"]), + vec_str(&["rg", "--hostname-bin=pwned", "files"]), ] { - assert!(!is_safe_command_windows(&cmd)); + assert!( + !is_safe_command_windows(&args), + "expected {args:?} to be considered unsafe due to external-command flag", + ); } } }