fix(core): remember Windows session approvals (#4212)

codex-rs/core/src/codex.rs

@@ -647,6 +647,25 @@ impl Session {
         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
     /// persist these response items to rollout.
     async fn record_conversation_items(&self, items: &[ResponseItem]) {
@@ -2648,6 +2667,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 => {
@@ -2663,7 +2683,7 @@ async fn handle_container_exec_with_params(
             match decision {
                 ReviewDecision::Approved => (),
                 ReviewDecision::ApprovedForSession => {
-                    sess.add_approved_command(params.command.clone()).await;
+                    remember_for_session = true;
                 }
                 ReviewDecision::Denied | ReviewDecision::Abort => {
                     return Err(FunctionCallError::RespondToModel(
@@ -2701,6 +2721,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, &params.command)
+            .await;
+    }
+
     let output_result = sess
         .run_exec_with_events(
             turn_diff_tracker,
@@ -2809,7 +2835,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,
+                &params.command,
+            )
+            .await;
             // Inform UI we are retrying without sandbox.
             sess.notify_background_event(&sub_id, "retrying command without sandbox")
                 .await;
@@ -3188,6 +3218,26 @@ 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_ref().clone()
+        });
+
+        assert!(approved.contains(&original));
+        assert!(approved.contains(&translated));
+    }
     #[test]
     fn prefers_structured_content_when_present() {
         let ctr = CallToolResult {

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<String> {
         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",
+            );
         }
     }
 }