From b765f6bfded495bebcc996f7ce367fa8dac2d685 Mon Sep 17 00:00:00 2001 From: Cameron Taggart Date: Thu, 20 Aug 2026 12:14:44 -0600 Subject: [PATCH] fix(core): quote arguments passed to external commands on Windows Windows has no argv array: a child process receives a single command-line string and parses it itself. The standard library only quotes an argument when it is empty or contains whitespace or a quote, which leaves every other argument exposed to a second round of interpretation in the child. Programs built on the MSYS2/Cygwin runtime -- including the tools shipped with Git for Windows -- re-expand the arguments they receive from a native parent, applying glob, brace, and tilde expansion along with quote removal. Words the shell had already finished expanding were silently corrupted: grep -oE '[0-9]{3}' -> child saw [0-9]3 grep -oE 'MAC\(x' -> child saw MAC(x find . -name '*.txt' -> child saw every file in the directory printf '%s' '~' -> child saw the home directory Explicitly quote every argument using the rules understood by CommandLineToArgvW so the child recovers exactly what the shell expanded. Native programs are unaffected, since they strip the quotes while parsing. Arguments bound for cmd.exe are left to the standard library to encode: cmd.exe rejects quotes around its own options, and the standard library applies extra escaping when spawning a batch file that a raw command line would bypass. Neither is an MSYS2/Cygwin program, so no protection is lost. Assisted-by: GitHub Copilot CLI:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- brush-core/src/commands.rs | 10 +- brush-core/src/sys/stubs/commands.rs | 10 + brush-core/src/sys/unix/commands.rs | 12 + brush-core/src/sys/windows.rs | 2 +- brush-core/src/sys/windows/commands.rs | 300 +++++++++++++++++++++++++ 5 files changed, 328 insertions(+), 6 deletions(-) create mode 100644 brush-core/src/sys/windows/commands.rs diff --git a/brush-core/src/commands.rs b/brush-core/src/commands.rs index 64e0505a8..0d4548a9d 100644 --- a/brush-core/src/commands.rs +++ b/brush-core/src/commands.rs @@ -184,7 +184,7 @@ pub fn compose_std_command, SE: extensions::ShellExtensions>( cmd.arg0(argv0); // Pass through args. - cmd.args(args); + sys::commands::set_args(&mut cmd, args); // Use the shell's current working dir. cmd.current_dir(context.shell.working_dir()); @@ -623,14 +623,14 @@ pub(crate) fn execute_external_command( } } - // When tracing is enabled, report. + // When tracing is enabled, report. Note that we report the args as the shell + // expanded them; on some platforms they are encoded before being handed to the + // child process. tracing::debug!( target: trace_categories::COMMANDS, "Spawning: cmd='{} {}'", cmd.get_program().to_string_lossy().to_string(), - cmd.get_args() - .map(|a| a.to_string_lossy().to_string()) - .join(" ") + cmd_args.iter().map(|a| a.as_str()).join(" ") ); match sys::process::spawn(cmd, context.shell.options().kill_external_commands_on_drop) { diff --git a/brush-core/src/sys/stubs/commands.rs b/brush-core/src/sys/stubs/commands.rs index 03e9a3a7e..f42e5a891 100644 --- a/brush-core/src/sys/stubs/commands.rs +++ b/brush-core/src/sys/stubs/commands.rs @@ -38,6 +38,16 @@ impl CommandExt for std::process::Command { } } +/// Sets the arguments to be passed to the given command. +/// +/// # Arguments +/// +/// * `cmd` - The command to set arguments on. +/// * `args` - The arguments to pass to the command. +pub fn set_args>(cmd: &mut std::process::Command, args: &[S]) { + cmd.args(args); +} + /// Extension trait for Unix-like exit status extensions. pub trait ExitStatusExt { /// Returns the signal that terminated the process, if any. diff --git a/brush-core/src/sys/unix/commands.rs b/brush-core/src/sys/unix/commands.rs index b5554399a..095a9c6b6 100644 --- a/brush-core/src/sys/unix/commands.rs +++ b/brush-core/src/sys/unix/commands.rs @@ -3,12 +3,24 @@ pub use std::os::unix::process::CommandExt; pub use std::os::unix::process::ExitStatusExt; +use std::ffi::OsStr; + use command_fds::{CommandFdExt, FdMapping}; use crate::ShellFd; use crate::error; use crate::openfiles; +/// Sets the arguments to be passed to the given command. +/// +/// # Arguments +/// +/// * `cmd` - The command to set arguments on. +/// * `args` - The arguments to pass to the command. +pub fn set_args>(cmd: &mut std::process::Command, args: &[S]) { + cmd.args(args); +} + /// Extension trait for injecting file descriptors into commands. pub trait CommandFdInjectionExt { /// Injects the given open files as file descriptors into the command. diff --git a/brush-core/src/sys/windows.rs b/brush-core/src/sys/windows.rs index 9b9aa3d3f..b78e3b919 100644 --- a/brush-core/src/sys/windows.rs +++ b/brush-core/src/sys/windows.rs @@ -1,5 +1,5 @@ pub use crate::sys::stubs::async_pipe; -pub use crate::sys::stubs::commands; +pub mod commands; pub(crate) mod env; pub use crate::sys::stubs::fd; pub(crate) mod fs; diff --git a/brush-core/src/sys/windows/commands.rs b/brush-core/src/sys/windows/commands.rs new file mode 100644 index 000000000..e90b67267 --- /dev/null +++ b/brush-core/src/sys/windows/commands.rs @@ -0,0 +1,300 @@ +//! Command execution utilities. + +use std::{ + ffi::{OsStr, OsString}, + os::windows::{ + ffi::{OsStrExt, OsStringExt}, + process::CommandExt as _, + }, + path::Path, +}; + +pub use crate::sys::stubs::commands::{ + CommandExt, CommandFdInjectionExt, CommandFgControlExt, ExitStatusExt, +}; + +/// Sets the arguments to be passed to the given command. +/// +/// Windows has no `argv` array; a child process receives a single command-line +/// string and is responsible for parsing it itself. The standard library only +/// wraps an argument in double quotes when it is empty or contains whitespace +/// or a quote, which leaves every other argument exposed to a second round of +/// interpretation in the child. +/// +/// That second round is not hypothetical: programs built on the MSYS2/Cygwin +/// runtime (for example, the tools shipped with Git for Windows) re-expand the +/// arguments they receive from a native parent, applying glob, brace, and tilde +/// expansion along with quote removal. Words that the shell has already +/// finished expanding are silently corrupted as a result. +/// +/// To keep the child's view of its arguments identical to the shell's, each +/// argument is explicitly quoted using the rules understood by +/// `CommandLineToArgvW` and the Microsoft C runtime. Native programs are +/// unaffected by the added quotes, since they strip them while parsing. +/// +/// # Arguments +/// +/// * `cmd` - The command to set arguments on. +/// * `args` - The arguments to pass to the command. +pub fn set_args>(cmd: &mut std::process::Command, args: &[S]) { + // Arguments destined for `cmd.exe` are deliberately left alone; see + // `uses_cmd_exe_parsing` for why. + if uses_cmd_exe_parsing(cmd.get_program()) { + cmd.args(args); + return; + } + + for arg in args { + cmd.raw_arg(quote_arg(arg.as_ref())); + } +} + +/// Returns true if the given program's arguments will be parsed by `cmd.exe`. +/// +/// Such arguments must be left to the standard library to encode, for two +/// reasons. First, `cmd.exe` does not accept quotes around its own options: a +/// command line of `cmd.exe "/c" "ver"` fails outright. Second, the standard +/// library applies extra escaping when spawning a batch file (which it runs via +/// `cmd.exe`) to keep the child from interpreting metacharacters such as `%`; +/// bypassing that with a raw command line would reintroduce a command injection +/// hazard. +/// +/// Neither `cmd.exe` nor a batch file is an MSYS2/Cygwin program, so no +/// protection is lost by deferring to the standard library here. +/// +/// # Arguments +/// +/// * `program` - The program the command will execute. +fn uses_cmd_exe_parsing(program: &OsStr) -> bool { + let path = Path::new(program); + let extension = path.extension().and_then(OsStr::to_str); + + // Batch files are launched through `cmd.exe`. + if extension + .is_some_and(|ext| ext.eq_ignore_ascii_case("bat") || ext.eq_ignore_ascii_case("cmd")) + { + return true; + } + + // `cmd.exe` itself, named with or without its extension. + if extension.is_none_or(|ext| ext.eq_ignore_ascii_case("exe")) { + return path + .file_stem() + .and_then(OsStr::to_str) + .is_some_and(|stem| stem.eq_ignore_ascii_case("cmd")); + } + + false +} + +/// Quotes the given argument so that a child process parsing its command line +/// with `CommandLineToArgvW` semantics recovers the argument unmodified. +/// +/// The argument is always quoted, even when quoting would not strictly be +/// required to preserve word boundaries. The quotes are what stop an +/// MSYS2/Cygwin child from re-expanding the argument. +/// +/// # Arguments +/// +/// * `arg` - The argument to quote. +fn quote_arg(arg: &OsStr) -> OsString { + let backslash = u16::from(b'\\'); + let quote = u16::from(b'"'); + + let mut quoted: Vec = Vec::with_capacity(arg.len() + 2); + quoted.push(quote); + + let mut pending_backslashes = 0usize; + for unit in arg.encode_wide() { + if unit == backslash { + pending_backslashes += 1; + } else { + if unit == quote { + // Any backslashes immediately preceding a quote are treated as escape + // characters, so they need doubling; the quote itself then needs escaping. + quoted.extend(std::iter::repeat_n(backslash, pending_backslashes + 1)); + } + pending_backslashes = 0; + } + + quoted.push(unit); + } + + // Backslashes at the end of the argument would otherwise escape the closing quote. + quoted.extend(std::iter::repeat_n(backslash, pending_backslashes)); + quoted.push(quote); + + OsString::from_wide(quoted.as_slice()) +} + +#[cfg(test)] +#[expect(clippy::panic_in_result_fn)] +mod tests { + use anyhow::Result; + + use super::*; + + fn quoted(arg: &str) -> String { + quote_arg(OsStr::new(arg)).to_string_lossy().into_owned() + } + + fn args_of(cmd: &std::process::Command) -> Vec { + cmd.get_args() + .map(|arg| arg.to_string_lossy().into_owned()) + .collect() + } + + #[test] + fn test_quote_arg_quotes_ordinary_words() { + assert_eq!(quoted("abc"), r#""abc""#); + assert_eq!(quoted(""), r#""""#); + assert_eq!(quoted("a b"), r#""a b""#); + assert_eq!(quoted("-oE"), r#""-oE""#); + } + + #[test] + fn test_quote_arg_protects_msys_metacharacters() { + // These are the characters an MSYS2/Cygwin child would otherwise re-expand. + assert_eq!(quoted("[0-9]{3}"), r#""[0-9]{3}""#); + assert_eq!(quoted("x{a,b}y"), r#""x{a,b}y""#); + assert_eq!(quoted("*"), r#""*""#); + assert_eq!(quoted("*.txt"), r#""*.txt""#); + assert_eq!(quoted("~"), r#""~""#); + assert_eq!(quoted("a'b"), r#""a'b""#); + assert_eq!(quoted(r"MAC\([A-Fa-f0-9]{12}"), r#""MAC\([A-Fa-f0-9]{12}""#); + } + + #[test] + fn test_quote_arg_escapes_embedded_quotes() { + assert_eq!(quoted(r#"a"b"#), r#""a\"b""#); + assert_eq!(quoted(r#"a\"b"#), r#""a\\\"b""#); + assert_eq!(quoted(r#"he said "hi""#), r#""he said \"hi\"""#); + } + + #[test] + fn test_quote_arg_doubles_only_trailing_backslashes() { + assert_eq!(quoted(r"C:\path"), r#""C:\path""#); + assert_eq!(quoted(r"C:\path\"), r#""C:\path\\""#); + assert_eq!(quoted(r"C:\path\\"), r#""C:\path\\\\""#); + } + + #[test] + fn test_uses_cmd_exe_parsing() { + assert!(uses_cmd_exe_parsing(OsStr::new("cmd"))); + assert!(uses_cmd_exe_parsing(OsStr::new("cmd.exe"))); + assert!(uses_cmd_exe_parsing(OsStr::new("CMD.EXE"))); + assert!(uses_cmd_exe_parsing(OsStr::new( + r"C:\Windows\System32\cmd.exe" + ))); + assert!(uses_cmd_exe_parsing(OsStr::new(r"C:\scripts\build.bat"))); + assert!(uses_cmd_exe_parsing(OsStr::new(r"C:\scripts\build.CMD"))); + + assert!(!uses_cmd_exe_parsing(OsStr::new("grep.exe"))); + assert!(!uses_cmd_exe_parsing(OsStr::new("mycmd.exe"))); + assert!(!uses_cmd_exe_parsing(OsStr::new("cmd.com"))); + assert!(!uses_cmd_exe_parsing(OsStr::new( + r"C:\Program Files\Git\usr\bin\grep.exe" + ))); + } + + #[test] + fn test_set_args_quotes_args_for_ordinary_programs() { + let mut cmd = std::process::Command::new("grep.exe"); + set_args(&mut cmd, &["-oE", "[0-9]{3}"]); + + assert_eq!(args_of(&cmd), vec![r#""-oE""#, r#""[0-9]{3}""#]); + } + + #[test] + fn test_set_args_leaves_cmd_exe_args_alone() { + let mut cmd = std::process::Command::new("cmd.exe"); + set_args(&mut cmd, &["/c", "ver"]); + + assert_eq!(args_of(&cmd), vec!["/c", "ver"]); + + let mut cmd = std::process::Command::new(r"C:\scripts\build.bat"); + set_args(&mut cmd, &["%CD%"]); + + assert_eq!(args_of(&cmd), vec!["%CD%"]); + } + + /// Verifies end-to-end that `cmd.exe` still works; blanket quoting would + /// break it with `'"ver' is not recognized as an internal or external command`. + #[test] + fn test_cmd_exe_still_runs() -> Result<()> { + let mut cmd = std::process::Command::new("cmd.exe"); + set_args(&mut cmd, &["/c", "echo hello world"]); + + let output = cmd.output()?; + + assert!(output.status.success()); + assert_eq!( + String::from_utf8_lossy(&output.stdout).trim_end(), + "hello world" + ); + + Ok(()) + } + + /// Verifies end-to-end that a batch file still receives `%`-bearing arguments + /// literally; encoding them into a raw command line would let `cmd.exe` expand + /// them. + #[test] + fn test_batch_file_args_are_not_expanded() -> Result<()> { + let dir = tempfile::tempdir()?; + let script_path = dir.path().join("echo-arg.bat"); + std::fs::write(&script_path, "@echo off\r\necho GOT:[%~1]\r\n")?; + + let mut cmd = std::process::Command::new(&script_path); + set_args(&mut cmd, &["%CD%"]); + + let output = cmd.output()?; + + assert_eq!( + String::from_utf8_lossy(&output.stdout).trim_end(), + "GOT:[%CD%]" + ); + + Ok(()) + } + + /// Verifies end-to-end that an MSYS2/Cygwin child receives arguments exactly as + /// the shell expanded them. Skipped when no such program can be found. + #[test] + fn test_msys_child_receives_args_unmodified() -> Result<()> { + let Some(printf_path) = find_msys_printf() else { + return Ok(()); + }; + + let cases = [ + "[0-9]{3}", + r"MAC\([A-Fa-f0-9]{12}", + "x{a,b}y", + "*", + "~", + "a'b", + r#"a"b"#, + r"C:\path\", + "a b", + ]; + + for case in cases { + let mut cmd = std::process::Command::new(&printf_path); + set_args(&mut cmd, &["<%s>\n", case]); + + let output = cmd.output()?; + let stdout = String::from_utf8_lossy(&output.stdout); + + assert_eq!(stdout.trim_end(), std::format!("<{case}>")); + } + + Ok(()) + } + + /// Locates an MSYS2/Cygwin build of `printf`, if one is installed. + fn find_msys_printf() -> Option { + // Git for Windows ships its MSYS2 tools here. + let candidate = std::path::PathBuf::from(r"C:\Program Files\Git\usr\bin\printf.exe"); + candidate.is_file().then_some(candidate) + } +}