openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-22 17:58:46 +00:00
Code Issues Packages Projects Releases Wiki Activity

chore: narrow ripgrep safety fix scope

Browse Source
This commit is contained in:
Eva Wong
2026-05-15 13:26:47 -07:00
committed by Adrian Bravo
parent 502ca48c78
commit f97d1c6db8
5 changed files with 34 additions and 184 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/shell-command/src/bash.rs
+2 -79
View File
@@ -1,4 +1,3 @@
use std::borrow::Cow;
use std::path::PathBuf;
use tree_sitter::Node;
@@ -116,55 +115,8 @@ pub fn extract_bash_command(command: &[String]) -> Option<(&str, &str)> {
pub fn parse_shell_lc_plain_commands(command: &[String]) -> Option<Vec<Vec<String>>> {
let (_, script) = extract_bash_command(command)?;
let script = strip_line_continuations(script);
let tree = try_parse_shell(&script)?;
try_parse_word_only_commands_sequence(&tree, &script)
}
fn strip_line_continuations(script: &str) -> Cow<'_, str> {
if !script.contains("\\\n") {
return Cow::Borrowed(script);
}
let mut stripped = String::with_capacity(script.len());
let mut chars = script.chars().peekable();
let mut escaped = false;
let mut in_double_quote = false;
let mut in_single_quote = false;
let mut stripped_any = false;
while let Some(ch) = chars.next() {
if escaped {
stripped.push(ch);
escaped = false;
} else if ch == '\\' && !in_single_quote {
if chars.peek() == Some(&'\n') {
chars.next();
stripped_any = true;
continue;
}
stripped.push(ch);
escaped = true;
} else {
match ch {
'\'' if !in_double_quote => {
in_single_quote = !in_single_quote;
}
'"' if !in_single_quote => {
in_double_quote = !in_double_quote;
}
_ => {}
}
stripped.push(ch);
}
}
if stripped_any {
Cow::Owned(stripped)
} else {
Cow::Borrowed(script)
}
let tree = try_parse_shell(script)?;
try_parse_word_only_commands_sequence(&tree, script)
}
/// Returns the parsed argv for a single shell command in a here-doc style
@@ -320,7 +272,6 @@ fn unescape_unquoted_word(raw: &str) -> Option<String> {
}
match chars.next() {
Some('\n') => {}
Some(escaped) => unescaped.push(escaped),
None => return None,
}
@@ -456,34 +407,6 @@ mod tests {
);
}
#[test]
fn shell_lc_parser_removes_line_continuations_outside_single_quotes() {
assert_eq!(
parse_shell_lc_plain_commands(&[
"bash".to_string(),
"-lc".to_string(),
"echo foo\\\nbar".to_string(),
]),
Some(vec![vec!["echo".to_string(), "foobar".to_string()]])
);
assert_eq!(
parse_shell_lc_plain_commands(&[
"bash".to_string(),
"-lc".to_string(),
"echo \"foo\\\nbar\"".to_string(),
]),
Some(vec![vec!["echo".to_string(), "foobar".to_string()]])
);
assert_eq!(
parse_shell_lc_plain_commands(&[
"bash".to_string(),
"-lc".to_string(),
"echo 'foo\\\nbar'".to_string(),
]),
Some(vec![vec!["echo".to_string(), "foo\\\nbar".to_string()]])
);
}
#[test]
fn accepts_double_quoted_strings_with_newlines() {
let cmds = parse_seq("git commit -m \"line1\nline2\"").unwrap();
codex-rs/shell-command/src/command_safety/is_dangerous_command.rs
+1 -4
View File
@@ -1,5 +1,4 @@
use crate::bash::parse_shell_lc_plain_commands;
use crate::command_safety::ripgrep::RipgrepArgCase;
use crate::command_safety::ripgrep::ripgrep_command_can_execute_arbitrary_command;
use std::path::Path;
#[cfg(windows)]
@@ -185,9 +184,7 @@ fn direct_command_can_execute_arbitrary_command(command: &[String]) -> bool {
match cmd0.as_deref() {
Some("sudo") => direct_command_can_execute_arbitrary_command(&command[1..]),
Some("rg") => {
ripgrep_command_can_execute_arbitrary_command(command, RipgrepArgCase::Sensitive)
}
Some("rg") => ripgrep_command_can_execute_arbitrary_command(command),
_ => false,
}
}
codex-rs/shell-command/src/command_safety/is_safe_command.rs
+1 -8
View File
@@ -4,7 +4,6 @@ use crate::command_safety::is_dangerous_command::executable_name_lookup_key;
// may appear before it (e.g., `-C`, `-c`, `--git-dir`).
// Implemented in `is_dangerous_command` and shared here.
use crate::command_safety::is_dangerous_command::find_git_subcommand;
use crate::command_safety::ripgrep::RipgrepArgCase;
use crate::command_safety::ripgrep::is_safe_ripgrep_command;
use crate::command_safety::windows_safe_commands::is_safe_command_windows;
#[cfg(windows)]
@@ -129,7 +128,7 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool {
}
// Ripgrep
Some("rg") => is_safe_ripgrep_command(command, RipgrepArgCase::Sensitive),
Some("rg") => is_safe_ripgrep_command(command),
// Git
Some("git") => is_safe_git_command(command),
@@ -570,10 +569,7 @@ mod tests {
// Unsafe flags that do not take an argument (present verbatim).
for args in [
vec_str(&["rg", "--search-zip", "files"]),
vec_str(&["rg", "--search-zip=true", "files"]),
vec_str(&["rg", "-z", "files"]),
vec_str(&["rg", "-zn", "files"]),
vec_str(&["rg", "-nz", "files"]),
] {
assert!(
!is_safe_to_call_with_exec(&args),
@@ -602,9 +598,6 @@ mod tests {
r"rg --\pre=./pre.sh files",
r"rg --hostname\-bin=hostname files",
r"rg -\z files",
r"rg -\zn needle .",
"rg --pr\\\ne=./pre.sh files",
"rg \"--pr\\\ne=./pre.sh\" files",
] {
assert!(
!is_known_safe_command(&vec_str(&["bash", "-lc", script])),
codex-rs/shell-command/src/command_safety/ripgrep.rs
+14 -90
View File
@@ -1,43 +1,30 @@
use std::borrow::Cow;
#[derive(Clone, Copy, Eq, PartialEq)]
pub(crate) enum RipgrepArgCase {
Sensitive,
AsciiInsensitive,
}
pub(crate) fn is_safe_ripgrep_command(command: &[String], arg_case: RipgrepArgCase) -> bool {
pub(crate) fn is_safe_ripgrep_command(command: &[String]) -> bool {
!command
.iter()
.skip(1)
.map(String::as_str)
.any(|arg| is_unsafe_ripgrep_arg(arg, arg_case))
.any(is_unsafe_ripgrep_arg)
}
pub(crate) fn ripgrep_command_can_execute_arbitrary_command(
command: &[String],
arg_case: RipgrepArgCase,
) -> bool {
command.iter().skip(1).map(String::as_str).any(|arg| {
let normalized = normalized_long_arg(arg, arg_case);
ripgrep_arg_can_execute_arbitrary_command(normalized.as_ref())
})
pub(crate) fn ripgrep_command_can_execute_arbitrary_command(command: &[String]) -> bool {
command
.iter()
.skip(1)
.map(String::as_str)
.any(ripgrep_arg_can_execute_arbitrary_command)
}
fn is_unsafe_ripgrep_arg(arg: &str, arg_case: RipgrepArgCase) -> bool {
let normalized = normalized_long_arg(arg, arg_case);
if ripgrep_arg_can_execute_arbitrary_command(normalized.as_ref()) {
fn is_unsafe_ripgrep_arg(arg: &str) -> bool {
if ripgrep_arg_can_execute_arbitrary_command(arg) {
return true;
}
match normalized.as_ref() {
match arg {
// Calls out to other decompression tools, so do not auto-approve
// out of an abundance of caution.
"--search-zip" => true,
_ => {
normalized.starts_with("--search-zip=")
|| ripgrep_short_options_contain_search_zip(arg, arg_case)
}
"-z" => true,
_ => false,
}
}
@@ -52,44 +39,6 @@ fn ripgrep_arg_can_execute_arbitrary_command(normalized_arg: &str) -> bool {
|| normalized_arg.starts_with("--hostname-bin=")
}
fn normalized_long_arg(arg: &str, arg_case: RipgrepArgCase) -> Cow<'_, str> {
if !arg.starts_with("--") {
return Cow::Borrowed(arg);
}
match arg_case {
RipgrepArgCase::Sensitive => Cow::Borrowed(arg),
RipgrepArgCase::AsciiInsensitive => Cow::Owned(arg.to_ascii_lowercase()),
}
}
fn ripgrep_short_options_contain_search_zip(arg: &str, arg_case: RipgrepArgCase) -> bool {
let Some(short_options) = arg.strip_prefix('-') else {
return false;
};
if short_options.is_empty() || short_options.starts_with('-') {
return false;
}
for option in short_options.chars() {
if option == 'z' || (arg_case == RipgrepArgCase::AsciiInsensitive && option == 'Z') {
return true;
}
if ripgrep_short_option_takes_value(option) {
return false;
}
}
false
}
fn ripgrep_short_option_takes_value(option: char) -> bool {
matches!(
option,
'A' | 'B' | 'C' | 'E' | 'M' | 'T' | 'd' | 'e' | 'f' | 'g' | 'j' | 'm' | 'r' | 't'
)
}
#[cfg(test)]
mod tests {
use super::*;
@@ -106,37 +55,12 @@ mod tests {
vec_str(&["rg", "--hostname-bin", "pwned", "files"]),
vec_str(&["rg", "--hostname-bin=pwned", "files"]),
vec_str(&["rg", "--search-zip", "files"]),
vec_str(&["rg", "--search-zip=true", "files"]),
vec_str(&["rg", "-z", "files"]),
vec_str(&["rg", "-zn", "files"]),
vec_str(&["rg", "-nz", "files"]),
] {
assert!(
!is_safe_ripgrep_command(&args, RipgrepArgCase::Sensitive),
!is_safe_ripgrep_command(&args),
"expected {args:?} to be unsafe",
);
}
}
#[test]
fn case_insensitive_matching_preserves_short_option_value_shape() {
for args in [
vec_str(&["rg", "--PRE", "pwned", "files"]),
vec_str(&["rg", "--HOSTNAME-BIN=pwned", "files"]),
vec_str(&["rg", "--SEARCH-ZIP", "files"]),
vec_str(&["rg", "-Z", "files"]),
vec_str(&["rg", "-Fz", "needle", "."]),
] {
assert!(
!is_safe_ripgrep_command(&args, RipgrepArgCase::AsciiInsensitive),
"expected {args:?} to be unsafe with case-insensitive matching",
);
}
let args = vec_str(&["rg", "-fz", "needle", "."]);
assert!(
is_safe_ripgrep_command(&args, RipgrepArgCase::AsciiInsensitive),
"expected lowercase -f to consume z as its pattern-file value",
);
}
}
codex-rs/shell-command/src/command_safety/windows_safe_commands.rs
+16 -3
View File
@@ -1,8 +1,6 @@
use crate::command_safety::is_safe_command::is_safe_git_command;
use crate::command_safety::powershell_parser::PowershellParseOutcome;
use crate::command_safety::powershell_parser::parse_with_powershell_ast;
use crate::command_safety::ripgrep::RipgrepArgCase;
use crate::command_safety::ripgrep::is_safe_ripgrep_command;
use std::path::Path;
/// On Windows, we conservatively allow only clearly read-only PowerShell invocations
@@ -192,7 +190,7 @@ pub(crate) fn is_safe_powershell_words(words: &[String]) -> bool {
"git" => is_safe_git_command(words),
"rg" => is_safe_ripgrep_command(words, RipgrepArgCase::AsciiInsensitive),
"rg" => is_safe_ripgrep(words),
// Extra safety: explicitly prohibit common side-effecting cmdlets regardless of args.
"set-content" | "add-content" | "out-file" | "new-item" | "remove-item" | "move-item"
@@ -208,6 +206,21 @@ pub(crate) fn is_safe_powershell_words(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}=")))
})
}
#[cfg(all(test, windows))]
mod tests {
use super::*;