fix: is_known_safe_command() should test exclusively with the PowerShell parser for PowerShell commands

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<String> = 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."#
+        );
+    }
 }

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<Vec<Vec<Str
 }
 
 /// Returns true when the executable name is one of the supported PowerShell binaries.
-fn is_powershell_executable(exe: &str) -> 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(),