From 52168a18f16f3ba82037da8a13c15d1a2789cbf6 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 12 Jan 2026 09:46:24 -0800 Subject: [PATCH] fix: is_known_safe_command() should test exclusively with the PowerShell parser for PowerShell commands --- .../src/command_safety/is_safe_command.rs | 66 +++++++++++++---- .../command_safety/windows_safe_commands.rs | 74 ++++++++++--------- 2 files changed, 89 insertions(+), 51 deletions(-) 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..968ac3bf39 100644 --- a/codex-rs/core/src/command_safety/is_safe_command.rs +++ b/codex-rs/core/src/command_safety/is_safe_command.rs @@ -1,23 +1,28 @@ use crate::bash::parse_shell_lc_plain_commands; -use crate::command_safety::windows_safe_commands::is_safe_command_windows; +use crate::command_safety::windows_safe_commands::is_powershell_executable; +use crate::command_safety::windows_safe_commands::is_safe_powershell_invocation; +/// `command` must be the _exact_ proposed list of arguments that will be sent +/// to `execvp(3)`. That is: +/// - If the command is to be run via a shell, then the first argument MUST be +/// the shell executable (e.g. "bash", "zsh", "pwsh", "powershell"). +/// - The first argument will be resolved against the `PATH` environment +/// variable, so it can be either a bare command name (e.g. "ls") or a full +/// path. +/// - If the first argument is NOT a shell executable, then the command MUST be +/// discoverable in the system `PATH` and MUST NOT rely on shell features +/// (e.g. shell built-ins, operators, or syntax). pub fn is_known_safe_command(command: &[String]) -> bool { - let command: Vec = command - .iter() - .map(|s| { - if s == "zsh" { - "bash".to_string() - } else { - s.clone() - } - }) - .collect(); - - if is_safe_command_windows(&command) { - return true; + if command.is_empty() { + return false; } - if is_safe_to_call_with_exec(&command) { + let exe = &command[0]; + if is_powershell_executable(exe) { + return is_safe_powershell_invocation(command); + } + + if is_safe_to_call_with_exec(command) { return true; } @@ -27,7 +32,7 @@ pub fn is_known_safe_command(command: &[String]) -> bool { // introduce side effects ( "&&", "||", ";", and "|" ). If every // individual command in the script is itself a known‑safe command, then // the composite expression is considered safe. - if let Some(all_commands) = parse_shell_lc_plain_commands(&command) + if let Some(all_commands) = parse_shell_lc_plain_commands(command) && !all_commands.is_empty() && all_commands .iter() @@ -422,4 +427,33 @@ mod tests { "> redirection should be rejected" ); } + + /// This test only works on Windows because it requires access to PowerShell + /// to parse the argument to `pwsh -Command`. + #[test] + fn ensure_safe_unix_command_that_is_unsafe_powershell_command_is_rejected() { + // `brew install powershell` to run this test on a Mac! + if which::which("pwsh").is_err() { + return; + } + + assert!( + is_known_safe_command(&vec_str(&["echo", "hi", "@(calc)"])), + "Running echo with execvp is safe because there is no shell interpretation." + ); + assert!( + !is_known_safe_command(&vec_str(&["bash", "-lc", "echo hi @(calc)"])), + "This command should not get marked as safe because the Bash script has a parse error." + ); + assert!( + is_known_safe_command(&vec_str(&["pwsh", "-Command", "echo hi"])), + "Our logic should recognize that `echo hi` is safe to run under PowerShell." + ); + assert!( + !is_known_safe_command(&vec_str(&["pwsh", "-Command", "echo hi @(calc)"])), + r#"Even though `echo hi @(calc)` would not do anything malicious +when run under Bash (because it would be rejected with a parse error), it would run +calc.exec when run under PowerShell and therefore should be rejected."# + ); + } } 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..672a6d6686 100644 --- a/codex-rs/core/src/command_safety/windows_safe_commands.rs +++ b/codex-rs/core/src/command_safety/windows_safe_commands.rs @@ -9,7 +9,11 @@ const POWERSHELL_PARSER_SCRIPT: &str = include_str!("powershell_parser.ps1"); /// On Windows, we conservatively allow only clearly read-only PowerShell invocations /// that match a small safelist. Anything else (including direct CMD commands) is unsafe. -pub fn is_safe_command_windows(command: &[String]) -> bool { +/// +/// `command` must be the _exact_ proposed list of arguments that will be sent +/// to `execvp(3)`, so `command[0]` must satisfy [`is_powershell_executable`] +/// for this function to return true. +pub fn is_safe_powershell_invocation(command: &[String]) -> bool { if let Some(commands) = try_parse_powershell_command_sequence(command) { commands .iter() @@ -108,7 +112,7 @@ fn parse_powershell_script(executable: &str, script: &str) -> Option bool { +pub fn is_powershell_executable(exe: &str) -> bool { let executable_name = Path::new(exe) .file_name() .and_then(|osstr| osstr.to_str()) @@ -357,21 +361,21 @@ mod tests { #[test] fn recognizes_safe_powershell_wrappers() { - assert!(is_safe_command_windows(&vec_str(&[ + assert!(is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "-NoLogo", "-Command", "Get-ChildItem -Path .", ]))); - assert!(is_safe_command_windows(&vec_str(&[ + assert!(is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "-NoProfile", "-Command", "git status", ]))); - assert!(is_safe_command_windows(&vec_str(&[ + assert!(is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "Get-Content", "Cargo.toml", @@ -379,7 +383,7 @@ mod tests { // pwsh parity if let Some(pwsh) = try_find_pwsh_executable_blocking() { - assert!(is_safe_command_windows(&[ + assert!(is_safe_powershell_invocation(&[ pwsh.as_path().to_str().unwrap().into(), "-NoProfile".to_string(), "-Command".to_string(), @@ -396,7 +400,7 @@ mod tests { } if let Some(pwsh) = try_find_pwsh_executable_blocking() { - assert!(is_safe_command_windows(&[ + assert!(is_safe_powershell_invocation(&[ pwsh.as_path().to_str().unwrap().into(), "-NoProfile".to_string(), "-Command".to_string(), @@ -404,7 +408,7 @@ mod tests { ])); } - assert!(is_safe_command_windows(&vec_str(&[ + assert!(is_safe_powershell_invocation(&vec_str(&[ r"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe", "-Command", "Get-Content Cargo.toml", @@ -418,7 +422,7 @@ mod tests { }; let pwsh: String = pwsh.as_path().to_str().unwrap().into(); - assert!(is_safe_command_windows(&[ + assert!(is_safe_powershell_invocation(&[ pwsh.clone(), "-NoLogo".to_string(), "-NoProfile".to_string(), @@ -427,7 +431,7 @@ mod tests { .to_string() ])); - assert!(is_safe_command_windows(&[ + assert!(is_safe_powershell_invocation(&[ pwsh.clone(), "-NoLogo".to_string(), "-NoProfile".to_string(), @@ -435,7 +439,7 @@ mod tests { "Get-Content foo.rs | Select-Object -Skip 200".to_string() ])); - assert!(is_safe_command_windows(&[ + assert!(is_safe_powershell_invocation(&[ pwsh.clone(), "-NoLogo".to_string(), "-NoProfile".to_string(), @@ -443,19 +447,19 @@ mod tests { "git -c core.pager=cat show HEAD:foo.rs".to_string() ])); - assert!(is_safe_command_windows(&[ + assert!(is_safe_powershell_invocation(&[ pwsh.clone(), "-Command".to_string(), "-git cat-file -p HEAD:foo.rs".to_string() ])); - assert!(is_safe_command_windows(&[ + assert!(is_safe_powershell_invocation(&[ pwsh.clone(), "-Command".to_string(), "(Get-Content foo.rs -Raw)".to_string() ])); - assert!(is_safe_command_windows(&[ + assert!(is_safe_powershell_invocation(&[ pwsh, "-Command".to_string(), "Get-Item foo.rs | Select-Object Length".to_string() @@ -464,97 +468,97 @@ mod tests { #[test] fn rejects_powershell_commands_with_side_effects() { - assert!(!is_safe_command_windows(&vec_str(&[ + assert!(!is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "-NoLogo", "-Command", "Remove-Item foo.txt", ]))); - assert!(!is_safe_command_windows(&vec_str(&[ + assert!(!is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "-NoProfile", "-Command", "rg --pre cat", ]))); - assert!(!is_safe_command_windows(&vec_str(&[ + assert!(!is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "-Command", "Set-Content foo.txt 'hello'", ]))); // Redirections are blocked - assert!(!is_safe_command_windows(&vec_str(&[ + assert!(!is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "-Command", "echo hi > out.txt", ]))); - assert!(!is_safe_command_windows(&vec_str(&[ + assert!(!is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "-Command", "Get-Content x | Out-File y", ]))); - assert!(!is_safe_command_windows(&vec_str(&[ + assert!(!is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "-Command", "Write-Output foo 2> err.txt", ]))); // Call operator is blocked - assert!(!is_safe_command_windows(&vec_str(&[ + assert!(!is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "-Command", "& Remove-Item foo", ]))); // Chained safe + unsafe must fail - assert!(!is_safe_command_windows(&vec_str(&[ + assert!(!is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "-Command", "Get-ChildItem; Remove-Item foo", ]))); // Nested unsafe cmdlet inside safe command must fail - assert!(!is_safe_command_windows(&vec_str(&[ + assert!(!is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "-Command", "Write-Output (Set-Content foo6.txt 'abc')", ]))); // Additional nested unsafe cmdlet examples must fail - assert!(!is_safe_command_windows(&vec_str(&[ + assert!(!is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "-Command", "Write-Host (Remove-Item foo.txt)", ]))); - assert!(!is_safe_command_windows(&vec_str(&[ + assert!(!is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "-Command", "Get-Content (New-Item bar.txt)", ]))); // Unsafe @ expansion. - assert!(!is_safe_command_windows(&vec_str(&[ + assert!(!is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "-Command", "ls @(calc.exe)" ]))); // Unsupported constructs that the AST parser refuses (no fallback to manual splitting). - assert!(!is_safe_command_windows(&vec_str(&[ + assert!(!is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "-Command", "ls && pwd" ]))); // Sub-expressions are rejected even if they contain otherwise safe commands. - assert!(!is_safe_command_windows(&vec_str(&[ + assert!(!is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "-Command", "Write-Output $(Get-Content foo)" ]))); // Empty words from the parser (e.g. '') are rejected. - assert!(!is_safe_command_windows(&vec_str(&[ + assert!(!is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "-Command", "''" @@ -563,13 +567,13 @@ mod tests { #[test] fn accepts_constant_expression_arguments() { - assert!(is_safe_command_windows(&vec_str(&[ + assert!(is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "-Command", "Get-Content 'foo bar'" ]))); - assert!(is_safe_command_windows(&vec_str(&[ + assert!(is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "-Command", "Get-Content \"foo bar\"" @@ -578,13 +582,13 @@ mod tests { #[test] fn rejects_dynamic_arguments() { - assert!(!is_safe_command_windows(&vec_str(&[ + assert!(!is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "-Command", "Get-Content $foo" ]))); - assert!(!is_safe_command_windows(&vec_str(&[ + assert!(!is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "-Command", "Write-Output \"foo $bar\"" @@ -599,7 +603,7 @@ mod tests { let chain = "pwd && ls"; assert!( - !is_safe_command_windows(&vec_str(&[ + !is_safe_powershell_invocation(&vec_str(&[ "powershell.exe", "-NoProfile", "-Command", @@ -610,7 +614,7 @@ mod tests { if let Some(pwsh) = try_find_pwsh_executable_blocking() { assert!( - is_safe_command_windows(&[ + is_safe_powershell_invocation(&[ pwsh.as_path().to_str().unwrap().into(), "-NoProfile".to_string(), "-Command".to_string(),