diff --git a/codex-cli/src/approvals.ts b/codex-cli/src/approvals.ts index e626da7f..35b8c0ae 100644 --- a/codex-cli/src/approvals.ts +++ b/codex-cli/src/approvals.ts @@ -370,11 +370,26 @@ export function isSafeCommand( reason: "View file with line numbers", group: "Reading files", }; - case "rg": + case "rg": { + // Certain ripgrep options execute external commands or invoke other + // processes, so we must reject them. + const isUnsafe = command.some( + (arg: string) => + UNSAFE_OPTIONS_FOR_RIPGREP_WITHOUT_ARGS.has(arg) || + [...UNSAFE_OPTIONS_FOR_RIPGREP_WITH_ARGS].some( + (opt) => arg === opt || arg.startsWith(`${opt}=`), + ), + ); + + if (isUnsafe) { + break; + } + return { reason: "Ripgrep search", group: "Searching", }; + } case "find": { // Certain options to `find` allow executing arbitrary processes, so we // cannot auto-approve them. @@ -495,6 +510,22 @@ const UNSAFE_OPTIONS_FOR_FIND_COMMAND: ReadonlySet = new Set([ "-fprintf", ]); +// Ripgrep options that are considered unsafe because they may execute +// arbitrary commands or spawn auxiliary processes. +const UNSAFE_OPTIONS_FOR_RIPGREP_WITH_ARGS: ReadonlySet = new Set([ + // Executes an arbitrary command for each matching file. + "--pre", + // Allows custom hostname command which could leak environment details. + "--hostname-bin", +]); + +const UNSAFE_OPTIONS_FOR_RIPGREP_WITHOUT_ARGS: ReadonlySet = new Set([ + // Enables searching inside archives which triggers external decompression + // utilities – reject out of an abundance of caution. + "--search-zip", + "-z", +]); + // ---------------- Helper utilities for complex shell expressions ----------------- // A conservative allow-list of bash operators that do not, on their own, cause diff --git a/codex-cli/tests/approvals.test.ts b/codex-cli/tests/approvals.test.ts index c592c395..645ab44c 100644 --- a/codex-cli/tests/approvals.test.ts +++ b/codex-cli/tests/approvals.test.ts @@ -44,6 +44,14 @@ describe("canAutoApprove()", () => { group: "Navigating", runInSandbox: false, }); + + // Ripgrep safe invocation. + expect(check(["rg", "TODO"])).toEqual({ + type: "auto-approve", + reason: "Ripgrep search", + group: "Searching", + runInSandbox: false, + }); }); test("simple safe commands within a `bash -lc` call", () => { @@ -67,6 +75,24 @@ describe("canAutoApprove()", () => { }); }); + test("ripgrep unsafe flags", () => { + // Flags that do not take arguments + expect(check(["rg", "--search-zip", "TODO"])).toEqual({ type: "ask-user" }); + expect(check(["rg", "-z", "TODO"])).toEqual({ type: "ask-user" }); + + // Flags that take arguments (provided separately) + expect(check(["rg", "--pre", "cat", "TODO"])).toEqual({ type: "ask-user" }); + expect(check(["rg", "--hostname-bin", "hostname", "TODO"])).toEqual({ + type: "ask-user", + }); + + // Flags that take arguments in = form + expect(check(["rg", "--pre=cat", "TODO"])).toEqual({ type: "ask-user" }); + expect(check(["rg", "--hostname-bin=hostname", "TODO"])).toEqual({ + type: "ask-user", + }); + }); + test("bash -lc commands with unsafe redirects", () => { expect(check(["bash", "-lc", "echo hello > file.txt"])).toEqual({ type: "ask-user", diff --git a/codex-rs/core/src/is_safe_command.rs b/codex-rs/core/src/is_safe_command.rs index 98c41dbd..237123c5 100644 --- a/codex-rs/core/src/is_safe_command.rs +++ b/codex-rs/core/src/is_safe_command.rs @@ -23,9 +23,9 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool { let cmd0 = command.first().map(String::as_str); match cmd0 { - Some( - "cat" | "cd" | "echo" | "grep" | "head" | "ls" | "pwd" | "rg" | "tail" | "wc" | "which", - ) => true, + Some("cat" | "cd" | "echo" | "grep" | "head" | "ls" | "pwd" | "tail" | "wc" | "which") => { + true + } Some("find") => { // Certain options to `find` can delete files, write to files, or @@ -46,6 +46,29 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool { .any(|arg| UNSAFE_FIND_OPTIONS.contains(&arg.as_str())) } + // 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}="))) + }) + } + // Git Some("git") => matches!( command.get(1).map(String::as_str), @@ -245,6 +268,40 @@ 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 bash_lc_safe_examples() { assert!(is_known_safe_command(&vec_str(&["bash", "-lc", "ls"])));