From bb044ab9fc9c91db1bce17350e4523a7195ae3fa Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 12 Dec 2025 16:52:24 -0800 Subject: [PATCH] fix: consolidate logic related to checking list of safe commands I noted that the logic for determining whether an `rg` or `git` invocation was safe diverged between `is_safe_command.rs` and `windows_safe_commands.rs`, so this PR reconciles the two in a shared `shared_rules.rs` file. --- .../src/command_safety/is_safe_command.rs | 63 +------- codex-rs/core/src/command_safety/mod.rs | 1 + .../core/src/command_safety/shared_rules.rs | 138 ++++++++++++++++++ .../command_safety/windows_safe_commands.rs | 63 +------- 4 files changed, 147 insertions(+), 118 deletions(-) create mode 100644 codex-rs/core/src/command_safety/shared_rules.rs diff --git a/codex-rs/core/src/command_safety/is_safe_command.rs b/codex-rs/core/src/command_safety/is_safe_command.rs index 01a52026e2..2e42244875 100644 --- a/codex-rs/core/src/command_safety/is_safe_command.rs +++ b/codex-rs/core/src/command_safety/is_safe_command.rs @@ -1,4 +1,6 @@ use crate::bash::parse_shell_lc_plain_commands; +use crate::command_safety::shared_rules::is_safe_git_command; +use crate::command_safety::shared_rules::is_safe_ripgrep_command; use crate::command_safety::windows_safe_commands::is_safe_command_windows; pub fn is_known_safe_command(command: &[String]) -> bool { @@ -108,33 +110,10 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool { } // Ripgrep - Some("rg") => { - const UNSAFE_RIPGREP_OPTIONS_WITH_ARGS: &[&str] = &[ - // Takes an arbitrary command that is executed for each match. - "--pre", - // Takes a command that can be used to obtain the local hostname. - "--hostname-bin", - ]; - const UNSAFE_RIPGREP_OPTIONS_WITHOUT_ARGS: &[&str] = &[ - // Calls out to other decompression tools, so do not auto-approve - // out of an abundance of caution. - "--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("rg") => is_safe_ripgrep_command(command), // Git - Some("git") => matches!( - command.get(1).map(String::as_str), - Some("branch" | "status" | "log" | "diff" | "show") - ), + Some("git") => is_safe_git_command(command), // Rust Some("cargo") if command.get(1).map(String::as_str) == Some("check") => true, @@ -280,40 +259,6 @@ mod tests { } } - #[test] - fn ripgrep_rules() { - // Safe ripgrep invocations – none of the unsafe flags are present. - assert!(is_safe_to_call_with_exec(&vec_str(&[ - "rg", - "Cargo.toml", - "-n" - ]))); - - // Unsafe flags that do not take an argument (present verbatim). - for args in [ - vec_str(&["rg", "--search-zip", "files"]), - vec_str(&["rg", "-z", "files"]), - ] { - assert!( - !is_safe_to_call_with_exec(&args), - "expected {args:?} to be considered unsafe due to zip-search flag", - ); - } - - // Unsafe flags that expect a value, provided in both split and = forms. - 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_to_call_with_exec(&args), - "expected {args:?} to be considered unsafe due to external-command flag", - ); - } - } - #[test] fn windows_powershell_full_path_is_safe() { if !cfg!(windows) { diff --git a/codex-rs/core/src/command_safety/mod.rs b/codex-rs/core/src/command_safety/mod.rs index caf5c9f6ea..bd226d1356 100644 --- a/codex-rs/core/src/command_safety/mod.rs +++ b/codex-rs/core/src/command_safety/mod.rs @@ -1,3 +1,4 @@ pub mod is_dangerous_command; pub mod is_safe_command; +mod shared_rules; pub mod windows_safe_commands; diff --git a/codex-rs/core/src/command_safety/shared_rules.rs b/codex-rs/core/src/command_safety/shared_rules.rs new file mode 100644 index 0000000000..cafaa85ebb --- /dev/null +++ b/codex-rs/core/src/command_safety/shared_rules.rs @@ -0,0 +1,138 @@ +//! Shared safelist logic for commands that are allowed on both Unix and Windows. +//! Keep these helpers read-only and conservative. + +/// Ensures the ripgrep invocation does not contain flags that could run +/// arbitrary commands. +pub(crate) fn is_safe_ripgrep_command>(words: &[S]) -> bool { + const UNSAFE_RIPGREP_OPTIONS_WITH_ARGS: &[&str] = &[ + // Takes an arbitrary command that is executed for each match. + "--pre", + // Takes a command that can be used to obtain the local hostname. + "--hostname-bin", + ]; + const UNSAFE_RIPGREP_OPTIONS_WITHOUT_ARGS: &[&str] = &[ + // Calls out to other decompression tools, so do not auto-approve out of + // an abundance of caution. + "--search-zip", + "-z", + ]; + + !words.iter().skip(1).any(|arg| { + let arg_lc = arg.as_ref().to_ascii_lowercase(); + UNSAFE_RIPGREP_OPTIONS_WITHOUT_ARGS.contains(&arg_lc.as_str()) + || UNSAFE_RIPGREP_OPTIONS_WITH_ARGS + .iter() + .any(|opt| arg_lc == *opt || arg_lc.starts_with(&format!("{opt}="))) + }) +} + +/// Ensures a Git command sticks to whitelisted read-only subcommands and flags. +pub(crate) fn is_safe_git_command>(words: &[S]) -> bool { + const SAFE_SUBCOMMANDS: &[&str] = &["status", "log", "show", "diff", "cat-file"]; + + let mut iter = words.iter().skip(1); + while let Some(arg) = iter.next() { + let arg = arg.as_ref(); + let arg_lc = arg.to_ascii_lowercase(); + + if arg.starts_with('-') { + if arg_lc.starts_with("-c=") || arg_lc.starts_with("--config=") { + return false; + } + + if arg_lc.starts_with("--git-dir=") || arg_lc.starts_with("--work-tree=") { + continue; + } + + if arg.eq_ignore_ascii_case("-c") || arg.eq_ignore_ascii_case("--config") { + return false; + } + + if arg.eq_ignore_ascii_case("--git-dir") || arg.eq_ignore_ascii_case("--work-tree") { + if iter.next().is_none() { + return false; + } + continue; + } + + continue; + } + + return SAFE_SUBCOMMANDS.contains(&arg_lc.as_str()); + } + + false +} + +#[cfg(test)] +mod tests { + use super::*; + + const RIPGREP_UNSAFE_NO_ARG: &[&[&str]] = + &[&["rg", "--search-zip", "files"], &["rg", "-z", "files"]]; + const RIPGREP_UNSAFE_WITH_ARG: &[&[&str]] = &[ + &["rg", "--pre", "pwned", "files"], + &["rg", "--pre=pwned", "files"], + &["rg", "--hostname-bin", "pwned", "files"], + &["rg", "--hostname-bin=pwned", "files"], + ]; + + #[test] + fn ripgrep_rules() { + // Safe ripgrep invocations – none of the unsafe flags are present. + assert!(is_safe_ripgrep_command(&["rg", "Cargo.toml", "-n"])); + + // Unsafe flags that do not take an argument (present verbatim). + for args in RIPGREP_UNSAFE_NO_ARG { + assert!( + !is_safe_ripgrep_command(args), + "expected {args:?} to be considered unsafe due to zip-search flag", + ); + } + + // Unsafe flags that expect a value, provided in both split and = forms. + for args in RIPGREP_UNSAFE_WITH_ARG { + assert!( + !is_safe_ripgrep_command(args), + "expected {args:?} to be considered unsafe due to external-command flag", + ); + } + } + + #[test] + fn git_rules() { + assert!(is_safe_git_command(&["git", "status"])); + assert!(is_safe_git_command(&[ + "git", + "--work-tree=.", + "--git-dir=.git", + "diff" + ])); + + assert!(!is_safe_git_command(&["git"])); + assert!(!is_safe_git_command(&["git", "branch"])); + assert!(!is_safe_git_command(&["git", "fetch"])); + assert!(!is_safe_git_command(&["git", "-c"])); + assert!(!is_safe_git_command(&[ + "git", + "-c", + "core.pager=cat", + "show" + ])); + assert!(!is_safe_git_command(&[ + "git", + "--config", + "core.pager=cat", + "show" + ])); + assert!(!is_safe_git_command(&["git", "-c=core.pager=cat", "show"])); + assert!(!is_safe_git_command(&[ + "git", + "--config=core.pager=cat", + "show" + ])); + assert!(!is_safe_git_command(&["git", "--config"])); + assert!(!is_safe_git_command(&["git", "--git-dir"])); + assert!(!is_safe_git_command(&["git", "--work-tree"])); + } +} 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 ac479a4d2c..daeae7cd92 100644 --- a/codex-rs/core/src/command_safety/windows_safe_commands.rs +++ b/codex-rs/core/src/command_safety/windows_safe_commands.rs @@ -5,6 +5,9 @@ use std::path::Path; use std::process::Command; use std::sync::LazyLock; +use crate::command_safety::shared_rules::is_safe_git_command; +use crate::command_safety::shared_rules::is_safe_ripgrep_command; + const POWERSHELL_PARSER_SCRIPT: &str = include_str!("powershell_parser.ps1"); /// On Windows, we conservatively allow only clearly read-only PowerShell invocations @@ -270,7 +273,7 @@ fn is_safe_powershell_command(words: &[String]) -> bool { "git" => is_safe_git_command(words), - "rg" => is_safe_ripgrep(words), + "rg" => is_safe_ripgrep_command(words), // Extra safety: explicitly prohibit common side-effecting cmdlets regardless of args. "set-content" | "add-content" | "out-file" | "new-item" | "remove-item" | "move-item" @@ -286,64 +289,6 @@ fn is_safe_powershell_command(words: &[String]) -> bool { } } -/// Checks that an `rg` invocation avoids options that can spawn arbitrary executables. -fn is_safe_ripgrep(words: &[String]) -> bool { - const UNSAFE_RIPGREP_OPTIONS_WITH_ARGS: &[&str] = &["--pre", "--hostname-bin"]; - const UNSAFE_RIPGREP_OPTIONS_WITHOUT_ARGS: &[&str] = &["--search-zip", "-z"]; - - !words.iter().skip(1).any(|arg| { - let arg_lc = arg.to_ascii_lowercase(); - // Examples rejected here: "pwsh -Command 'rg --pre cat pattern'" and "pwsh -Command 'rg --search-zip pattern'". - UNSAFE_RIPGREP_OPTIONS_WITHOUT_ARGS.contains(&arg_lc.as_str()) - || UNSAFE_RIPGREP_OPTIONS_WITH_ARGS - .iter() - .any(|opt| arg_lc == *opt || arg_lc.starts_with(&format!("{opt}="))) - }) -} - -/// Ensures a Git command sticks to whitelisted read-only subcommands and flags. -fn is_safe_git_command(words: &[String]) -> bool { - const SAFE_SUBCOMMANDS: &[&str] = &["status", "log", "show", "diff", "cat-file"]; - - let mut iter = words.iter().skip(1); - while let Some(arg) = iter.next() { - let arg_lc = arg.to_ascii_lowercase(); - - if arg.starts_with('-') { - if arg.eq_ignore_ascii_case("-c") || arg.eq_ignore_ascii_case("--config") { - if iter.next().is_none() { - // Examples rejected here: "pwsh -Command 'git -c'" and "pwsh -Command 'git --config'". - return false; - } - continue; - } - - if arg_lc.starts_with("-c=") - || arg_lc.starts_with("--config=") - || arg_lc.starts_with("--git-dir=") - || arg_lc.starts_with("--work-tree=") - { - continue; - } - - if arg.eq_ignore_ascii_case("--git-dir") || arg.eq_ignore_ascii_case("--work-tree") { - if iter.next().is_none() { - // Examples rejected here: "pwsh -Command 'git --git-dir'" and "pwsh -Command 'git --work-tree'". - return false; - } - continue; - } - - continue; - } - - return SAFE_SUBCOMMANDS.contains(&arg_lc.as_str()); - } - - // Examples rejected here: "pwsh -Command 'git'" and "pwsh -Command 'git status --short | Remove-Item foo'". - false -} - #[cfg(all(test, windows))] mod tests { use super::*;