diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 281fee0a16..59381459dc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -688,6 +688,26 @@ jobs: run: cargo clippy --workspace --all-targets --target $env:TARGET -- -D warnings - name: Check (workspace) run: cargo check --workspace --all-targets --target $env:TARGET + - name: Test (buzz-dev-mcp) + # The Windows-only bash resolver lives in buzz-dev-mcp; its unit tests + # only gate if this crate is tested ON Windows. + run: cargo test -p buzz-dev-mcp --target $env:TARGET + # The bundled-bash staging (PortableGit download, SFX extract, mingw64 drop) + # runs only in release.yml on tag — so without this step the agent's only + # Windows transport would ship UNEXERCISED until a tagged release hits users. + # Stage the tree and spawn the staged bash on a real coreutils pipeline: this + # gates the SFX `-o` POSIX-path extraction, that bash.exe still spawns after + # mingw64/ is dropped, and that the lazily-loaded MSYS DLL closure (msys-2.0.dll, + # coreutils) survives — the exact behaviors unconfirmable off a non-Windows host. + - name: Smoke-test bundled bash staging + shell: bash + run: | + set -euo pipefail + stage_dir="$RUNNER_TEMP/git-bash" + scripts/stage-windows-bash.sh "$stage_dir" + out=$("$stage_dir/usr/bin/bash.exe" -c 'echo hello | tr a-z A-Z') + [[ "$out" == "HELLO" ]] || { echo "staged bash pipeline failed: got '$out'" >&2; exit 1; } + echo "staged bash spawned and ran a coreutils pipeline" - name: Check (Tauri crate) run: cargo check --manifest-path desktop/src-tauri/Cargo.toml --target $env:TARGET env: diff --git a/Cargo.lock b/Cargo.lock index 075998be85..05ff37be38 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -894,6 +894,7 @@ dependencies = [ "tokio-util", "tracing", "tracing-subscriber", + "windows-sys 0.61.2", "zeroize", ] diff --git a/crates/buzz-dev-mcp/Cargo.toml b/crates/buzz-dev-mcp/Cargo.toml index dd0872b35f..131a97df78 100644 --- a/crates/buzz-dev-mcp/Cargo.toml +++ b/crates/buzz-dev-mcp/Cargo.toml @@ -38,3 +38,12 @@ image = { version = "0.25", default-features = false, features = ["jpeg", "png", [target.'cfg(unix)'.dependencies] nix = { version = "0.31", default-features = false, features = ["signal", "process"] } + +# Windows Job Object APIs for the shell tool's timeout kill path: terminating a +# job kills the bash child AND every MSYS grandchild it forked, the Windows +# analogue of the Unix killpg above. windows-sys 0.61 is already workspace- +# resident (pulled transitively), so this adds no new crate. Win32_Security is +# required because CreateJobObjectW takes a SECURITY_ATTRIBUTES parameter; +# Win32_System_Threading supplies IO_COUNTERS inside the extended-limit struct. +[target.'cfg(windows)'.dependencies] +windows-sys = { version = "0.61", features = ["Win32_Foundation", "Win32_Security", "Win32_System_JobObjects", "Win32_System_Threading"] } diff --git a/crates/buzz-dev-mcp/src/lib.rs b/crates/buzz-dev-mcp/src/lib.rs index ab4f6ad4c9..102a1c6551 100644 --- a/crates/buzz-dev-mcp/src/lib.rs +++ b/crates/buzz-dev-mcp/src/lib.rs @@ -1,4 +1,5 @@ -#![forbid(unsafe_code)] +#![cfg_attr(not(windows), forbid(unsafe_code))] +#![cfg_attr(windows, deny(unsafe_code))] use rmcp::{ handler::server::{router::tool::ToolRouter, wrapper::Parameters}, model::{CallToolResult, ServerCapabilities, ServerInfo}, diff --git a/crates/buzz-dev-mcp/src/read_file.rs b/crates/buzz-dev-mcp/src/read_file.rs index b75867b61e..c9233390bc 100644 --- a/crates/buzz-dev-mcp/src/read_file.rs +++ b/crates/buzz-dev-mcp/src/read_file.rs @@ -143,9 +143,16 @@ mod tests { #[test] fn read_allows_absolute_path() { let dir = tempdir().expect("tempdir"); + // A real file in a SECOND tempdir, genuinely outside the workspace + // root — proves absolute paths beyond workdir resolve, without a + // Unix-only system path like /etc/hosts (which is C:\etc\hosts on + // Windows and does not exist). + let outside = tempdir().expect("tempdir"); + let target = outside.path().join("outside.txt"); + fs::write(&target, b"localhost").expect("write"); let state = make_state(dir.path()); let p = ReadFileParams { - path: "/etc/hosts".into(), + path: target.display().to_string(), offset: None, limit: None, workdir: Some(dir.path().display().to_string()), @@ -153,7 +160,7 @@ mod tests { let out = run(&state, p).expect("ok"); assert!( out.contains("localhost"), - "expected /etc/hosts content, got: {out}" + "expected out-of-workspace file content, got: {out}" ); } diff --git a/crates/buzz-dev-mcp/src/shell.rs b/crates/buzz-dev-mcp/src/shell.rs index 2a48d2cfd4..0b27b160d1 100644 --- a/crates/buzz-dev-mcp/src/shell.rs +++ b/crates/buzz-dev-mcp/src/shell.rs @@ -143,7 +143,11 @@ pub async fn run( )); } - let mut cmd = Command::new("bash"); + let bash = match resolve_bash(&state.shim.path_env) { + Ok(path) => path, + Err(msg) => return Ok(CallToolResult::error(vec![Content::text(msg)])), + }; + let mut cmd = Command::new(&bash); cmd.arg("-c").arg(&p.command); cmd.current_dir(&workdir); cmd.env("PATH", &state.shim.path_env); @@ -170,15 +174,11 @@ pub async fn run( let pid = child.id(); - struct PgidGuard(Option); - impl Drop for PgidGuard { - fn drop(&mut self) { - if let Some(pid) = self.0 { - kill_process_group_immediate(pid as i32); - } - } - } - let mut pgid_guard = PgidGuard(pid); + // KillGroup ties the spawned bash and all its descendants to a single kill + // primitive (Unix process group / Windows Job Object). Built from the live + // child so the Windows job can take the process handle, which only exists + // after spawn. Held for the whole run; its Drop is the last-resort reaper. + let mut kill_group = KillGroup::new(&child, pid); let stdout_pipe = child.stdout.take(); let stderr_pipe = child.stderr.take(); @@ -202,19 +202,17 @@ pub async fn run( biased; _ = ct.cancelled() => { // Kill process group, reap child, abort reader tasks. - if let Some(pid) = pid { - kill_process_group_immediate(pid as i32); - } + kill_group.kill_immediate(); // Bounded reap so we don't leak zombies. If reap times out, - // PgidGuard drop will SIGKILL again as a last resort. + // KillGroup drop will kill again as a last resort. match tokio::time::timeout(Duration::from_secs(1), child.wait()).await { - Ok(Ok(_)) => { pgid_guard.0 = None; } // reaped; disarm guard + Ok(Ok(_)) => { kill_group.disarm(); } // reaped; disarm guard Ok(Err(e)) => { tracing::debug!("cancel: child wait error: {e}"); - // Leave pgid_guard armed for drop-kill. + // Leave kill_group armed for drop-kill. } Err(_) => { - tracing::debug!("cancel: child reap timed out; guard will SIGKILL on drop"); + tracing::debug!("cancel: child reap timed out; guard will kill on drop"); } } stdout_handle.abort(); @@ -229,9 +227,7 @@ pub async fn run( } Err(_) => { // Kill process group — this closes the pipes, causing reads to EOF. - if let Some(pid) = pid { - kill_process_group_graceful(pid as i32).await; - } + kill_group.kill_graceful().await; // Reap the child so it doesn't become a zombie. let deadline = Instant::now() + Duration::from_secs(2); loop { @@ -261,9 +257,7 @@ pub async fn run( }; if !timed_out { - if let Some(pid) = pid { - kill_process_group_graceful(pid as i32).await; - } + kill_group.kill_graceful().await; } let stdout_cap = match tokio::time::timeout(Duration::from_secs(5), &mut stdout_handle).await { @@ -308,10 +302,151 @@ pub async fn run( "notes": notes, }); let text = serde_json::to_string_pretty(&body).unwrap_or_else(|_| "{}".into()); - pgid_guard.0 = None; + kill_group.disarm(); Ok(CallToolResult::success(vec![Content::text(text)])) } +/// The bundled bash subtree's directory name under the install root, and the +/// relative path to its `bash.exe`. This is the THREE-FILE PATH CONTRACT — it must +/// stay byte-identical with: +/// 1. `scripts/bundle-sidecars.sh` — stages the bash tree to +/// `desktop/src-tauri/binaries/git-bash/` (the bundle-source dir). +/// 2. `desktop/scripts/build-release-config.mjs` — emits the Windows-only +/// `bundle.resources` Map `{ "binaries/git-bash": "git-bash" }`, whose TARGET +/// (`git-bash`) is what Tauri's NSIS/MSI installer stages next to the exe. +/// 3. this resolver — joins `current_exe().parent()` + `git-bash\usr\bin\bash.exe`. +/// +/// Drift between (2)'s target and this string ships a working bundle but a broken +/// runtime path. Keep all three in lockstep. +/// +/// The bundled constant points at `usr\bin\bash.exe` (the real MSYS2 bash, which +/// boots from its co-located `msys-2.0.dll` and needs only `usr/`, no `mingw64/`) +/// — NOT the `bin\bash.exe` launcher shim, which refuses to start without a sibling +/// `mingw64\bin` marker the bundle deliberately drops. The installed-Git branch +/// below stays on `bin\bash.exe` on purpose: a real Git-for-Windows install has +/// `mingw64/`, so its launcher is the correct entry and sets up MSYSTEM/PATH. +/// Different tree shape -> different correct entry point. +#[cfg(windows)] +const BUNDLED_BASH_REL: &str = r"git-bash\usr\bin\bash.exe"; + +/// Resolve a genuine, non-WSL bash to an absolute path so we spawn it directly +/// instead of letting `Command::new("bash")` re-enter PATH search — on Windows +/// that search finds `System32\bash.exe` (the WSL launcher), which fails at spawn +/// with `0x8007072c` and can never run the agent's POSIX commands. +/// +/// On Unix, bare `bash` resolved via PATH is correct and was never broken, so the +/// resolver is a no-op there. The probe logic is Windows-only. +#[cfg(not(windows))] +fn resolve_bash(_path_env: &str) -> Result { + Ok(PathBuf::from("bash")) +} + +/// Windows bash resolution. Probe order (first hit wins): +/// 1. `GIT_BASH` env override (escape hatch / explicit operator choice). +/// 2. Installed Git for Windows (fast path when the user has Git). +/// 3. The bundled bash staged next to our exe (guaranteed target — this +/// is what makes a bare, Git-less host work since the app is self-contained). +/// 4. PATH scan, EXCLUDING System32 (so we never resolve WSL's `bash.exe`). +/// +/// No bash found -> actionable error returned BEFORE spawn. +#[cfg(windows)] +fn resolve_bash(path_env: &str) -> Result { + if let Some(p) = std::env::var_os("GIT_BASH").map(PathBuf::from) { + if p.is_file() { + return Ok(p); + } + } + + for root in ["ProgramFiles", "LocalAppData"] { + if let Some(base) = std::env::var_os(root) { + let candidate = match root { + "LocalAppData" => PathBuf::from(&base).join("Programs").join("Git"), + _ => PathBuf::from(&base).join("Git"), + } + .join("bin") + .join("bash.exe"); + if candidate.is_file() { + return Ok(candidate); + } + } + } + + // Bundled bash, located relative to OUR OWN executable. On Windows, Tauri + // stages `bundle.resources` flat in the directory that contains the exe + // (tauri 2.11.2 `resource_dir()` == exe parent on Windows), and every sidecar + // — including this one — lives in that same dir. This relative-to-self resolution + // is Windows-ONLY: macOS stages resources to `../Resources` and Linux to + // `usr/lib/`, so a cross-platform "resource relative to exe" helper would + // be wrong on those platforms. + if let Ok(exe) = std::env::current_exe() { + if let Some(dir) = exe.parent() { + if let Some(p) = bundled_bash(dir) { + return Ok(p); + } + } + } + + if let Some(p) = scan_path_for_bash(path_env, std::env::var_os("SystemRoot").map(PathBuf::from)) + { + return Ok(p); + } + + Err("no bash found: install Git for Windows, or set GIT_BASH to a bash.exe path".into()) +} + +/// Compute the bundled bash path relative to the install dir (the exe's parent), +/// `is_file`-gated so dev/CI builds without a staged resource return None and let +/// the caller fall through cleanly — never returning a non-existent path that +/// would fail later at spawn with a worse message. +#[cfg(windows)] +fn bundled_bash(install_dir: &Path) -> Option { + let bundled = install_dir.join(BUNDLED_BASH_REL); + bundled.is_file().then_some(bundled) +} + +/// True if `dir` is `root` or lives under it, comparing path components +/// case-INsensitively. Windows paths are case-insensitive, but `Path::starts_with` +/// compares components case-sensitively on every platform — so a PATH entry spelled +/// `C:\WINDOWS\System32` would slip past a `%SystemRoot%`=`C:\Windows` prefix test +/// and let WSL's `System32\bash.exe` be resolved, reintroducing the `0x8007072c` +/// spawn failure. Component-wise comparison (not a lowercased substring match) avoids +/// a false hit on a sibling like `C:\Windows2`. +#[cfg(windows)] +fn is_under_dir(dir: &Path, root: &Path) -> bool { + let mut dir_components = dir.components(); + for root_component in root.components() { + match dir_components.next() { + Some(d) + if d.as_os_str() + .eq_ignore_ascii_case(root_component.as_os_str()) => {} + _ => return false, + } + } + true +} + +/// Scan the child's PATH for `bash.exe`, skipping the Windows system directory +/// (`system_root`, normally `%SystemRoot%`) so we never resolve WSL's +/// `System32\bash.exe`. PATH is parsed with `std::env::split_paths` (never a +/// hand-split on ';') so it matches exactly what the spawned child would see. +#[cfg(windows)] +fn scan_path_for_bash(path_env: &str, system_root: Option) -> Option { + for dir in std::env::split_paths(path_env) { + if let Some(ref root) = system_root { + // Skip System32 (and any other dir under %SystemRoot%) — that's where + // WSL's bash.exe lives. + if is_under_dir(&dir, root) { + continue; + } + } + let candidate = dir.join("bash.exe"); + if candidate.is_file() { + return Some(candidate); + } + } + None +} + #[cfg(unix)] fn set_process_group(cmd: &mut Command) { cmd.process_group(0); @@ -320,31 +455,182 @@ fn set_process_group(cmd: &mut Command) { #[cfg(not(unix))] fn set_process_group(_cmd: &mut Command) {} -/// Immediate SIGKILL of the process group. Sync; safe to call from Drop. -/// No grace period — used when the parent task is being torn down. +/// Kill primitive covering the spawned bash AND every descendant it forks, +/// mirroring the same guarantee across platforms. +/// +/// - Unix: the child's process group (set via [`set_process_group`]); kills go +/// to the whole group via `killpg`. +/// - Windows: a Job Object the child is assigned to at construction. A bare +/// `TerminateProcess` on bash leaves MSYS-forked grandchildren (e.g. `sleep`) +/// running — they hold the stdout/stderr pipes open, so the reap blocks until +/// they self-exit. Terminating the job kills the entire tree atomically. +/// +/// Held for the whole `run`; `Drop` is the last-resort reaper if an explicit +/// kill was skipped or failed. #[cfg(unix)] -fn kill_process_group_immediate(pid: i32) { - use nix::sys::signal::{killpg, Signal}; - use nix::unistd::Pid; - let _ = killpg(Pid::from_raw(pid), Signal::SIGKILL); -} +struct KillGroup(Option); -#[cfg(not(unix))] -fn kill_process_group_immediate(_pid: i32) {} +#[cfg(unix)] +impl KillGroup { + fn new(_child: &tokio::process::Child, pid: Option) -> Self { + Self(pid.map(|p| p as i32)) + } + + /// Immediate SIGKILL of the process group. Sync; safe to call from Drop. + /// No grace period — used when the parent task is being torn down. + fn kill_immediate(&self) { + use nix::sys::signal::{killpg, Signal}; + use nix::unistd::Pid; + if let Some(pid) = self.0 { + let _ = killpg(Pid::from_raw(pid), Signal::SIGKILL); + } + } + + /// Graceful SIGTERM → 200ms async sleep → SIGKILL. Async; never blocks the runtime. + async fn kill_graceful(&self) { + use nix::sys::signal::{killpg, Signal}; + use nix::unistd::Pid; + if let Some(pid) = self.0 { + let pgid = Pid::from_raw(pid); + let _ = killpg(pgid, Signal::SIGTERM); + tokio::time::sleep(Duration::from_millis(200)).await; + let _ = killpg(pgid, Signal::SIGKILL); + } + } + + /// Disarm the Drop-time kill once the child has been reaped explicitly. + fn disarm(&mut self) { + self.0 = None; + } +} -/// Graceful SIGTERM → 200ms async sleep → SIGKILL. Async; never blocks the runtime. #[cfg(unix)] -async fn kill_process_group_graceful(pid: i32) { - use nix::sys::signal::{killpg, Signal}; - use nix::unistd::Pid; - let pgid = Pid::from_raw(pid); - let _ = killpg(pgid, Signal::SIGTERM); - tokio::time::sleep(Duration::from_millis(200)).await; - let _ = killpg(pgid, Signal::SIGKILL); +impl Drop for KillGroup { + fn drop(&mut self) { + self.kill_immediate(); + } } -#[cfg(not(unix))] -async fn kill_process_group_graceful(_pid: i32) {} +#[cfg(windows)] +struct KillGroup { + job: windows_sys::Win32::Foundation::HANDLE, +} + +// SAFETY: `job` is a raw Win32 HANDLE (`*mut c_void`), which is neither `Send` +// nor `Sync` by default. The shell tool's async future holds a `KillGroup` +// across an `.await`, so it must be `Send` to be spawned. A job-object handle +// is a kernel object reference, not thread-affine: `TerminateJobObject` and +// `CloseHandle` are thread-safe, and Rust's `&self`/`&mut self` borrows still +// serialize access to the field. Moving or sharing it across threads is sound. +#[cfg(windows)] +#[allow(unsafe_code)] +unsafe impl Send for KillGroup {} +#[cfg(windows)] +#[allow(unsafe_code)] +unsafe impl Sync for KillGroup {} + +#[cfg(windows)] +#[allow(unsafe_code)] +impl KillGroup { + fn new(child: &tokio::process::Child, _pid: Option) -> Self { + use std::mem::{size_of, zeroed}; + use windows_sys::Win32::Foundation::HANDLE; + use windows_sys::Win32::System::JobObjects::{ + AssignProcessToJobObject, CreateJobObjectW, JobObjectExtendedLimitInformation, + SetInformationJobObject, JOBOBJECT_EXTENDED_LIMIT_INFORMATION, + JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, + }; + + // SAFETY: each call is a documented Win32 FFI call with arguments that + // satisfy its contract — a null SECURITY_ATTRIBUTES/name for an + // anonymous job, a zeroed #[repr(C)] info struct sized by size_of, and + // the live process handle from `child` (valid while it is running). + // A null job HANDLE on failure makes every later call a harmless no-op. + let job = unsafe { + let job: HANDLE = CreateJobObjectW(std::ptr::null(), std::ptr::null()); + if !job.is_null() { + let mut info: JOBOBJECT_EXTENDED_LIMIT_INFORMATION = zeroed(); + // KILL_ON_JOB_CLOSE: when the LAST handle to the job closes, + // Windows kills every process still in it. This is both the + // explicit-kill mechanism and the Drop-time safety net — and the + // reason the job HANDLE must outlive the child (see Drop). + info.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; + SetInformationJobObject( + job, + JobObjectExtendedLimitInformation, + std::ptr::addr_of!(info).cast(), + size_of::() as u32, + ); + if let Some(handle) = child.raw_handle() { + AssignProcessToJobObject(job, handle as HANDLE); + } + } + job + }; + Self { job } + } + + fn kill_immediate(&self) { + self.terminate(); + } + + async fn kill_graceful(&self) { + // A Job Object has no SIGTERM analogue; termination is atomic, so the + // graceful path is the same single terminate as the immediate path. + self.terminate(); + } + + fn terminate(&self) { + use windows_sys::Win32::System::JobObjects::TerminateJobObject; + if !self.job.is_null() { + // SAFETY: `self.job` is a valid job HANDLE for this struct's + // lifetime; exit code 137 mirrors the SIGKILL (128+9) we report on + // Unix. + unsafe { + TerminateJobObject(self.job, 137); + } + } + } + + /// No-op on Windows: the job is terminated explicitly, and closing the + /// handle on Drop with no live processes left is harmless. Kept for a + /// uniform call shape with the Unix guard. + fn disarm(&mut self) {} +} + +#[cfg(windows)] +#[allow(unsafe_code)] +impl Drop for KillGroup { + fn drop(&mut self) { + use windows_sys::Win32::Foundation::CloseHandle; + if !self.job.is_null() { + // Closing the last job handle triggers KILL_ON_JOB_CLOSE, killing any + // process still in the job — the last-resort reaper. The handle is + // held until here precisely so this fires no earlier than run end. + // SAFETY: `self.job` is a valid HANDLE created in `new` and closed + // exactly once here. + unsafe { + CloseHandle(self.job); + } + } + } +} + +// Fallback for targets that are neither unix nor windows: no process-tree kill +// primitive is wired up, so timeouts rely on the cross-platform start_kill in +// `run`. Keeps the crate compiling everywhere. +#[cfg(not(any(unix, windows)))] +struct KillGroup; + +#[cfg(not(any(unix, windows)))] +impl KillGroup { + fn new(_child: &tokio::process::Child, _pid: Option) -> Self { + Self + } + fn kill_immediate(&self) {} + async fn kill_graceful(&self) {} + fn disarm(&mut self) {} +} #[derive(Default)] struct CapturedStream { @@ -520,7 +806,12 @@ mod tests { let r = run( &state, ShellParams { - command: "sleep 999".into(), + // Short sleep, not 999: the kill path must actually terminate + // the process tree on timeout. If a regression leaves the child + // (or an MSYS grandchild) orphaned, the test stalls until this + // brief sleep self-exits — ~5s, not ~16min — so the failure + // stays visible instead of hiding behind a 999s sleep. + command: "sleep 5".into(), workdir: None, timeout_ms: Some(150), }, @@ -563,3 +854,96 @@ mod tests { ); } } + +#[cfg(all(test, windows))] +mod windows_resolver_tests { + use super::*; + use std::env; + use tempfile::tempdir; + + fn touch(path: &Path) { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).expect("mkdir"); + } + std::fs::write(path, b"").expect("touch"); + } + + #[test] + fn bundled_branch_returns_none_when_path_absent() { + // Dev/CI: no staged resource next to the exe -> the bundled branch must + // yield None so the resolver falls through instead of returning a + // non-existent path that would fail at spawn. + let dir = tempdir().expect("tempdir"); + assert!(bundled_bash(dir.path()).is_none()); + } + + #[test] + fn bundled_branch_returns_absolute_path_when_staged() { + // A staged PortableGit bash runtime next to the exe resolves to the absolute bash path. + let dir = tempdir().expect("tempdir"); + let bash = dir.path().join(BUNDLED_BASH_REL); + touch(&bash); + let resolved = bundled_bash(dir.path()).expect("bundled bash"); + assert!(resolved.is_absolute()); + assert_eq!(resolved, bash); + } + + #[test] + fn path_scan_skips_system32_and_returns_absolute() { + // A bash.exe under %SystemRoot% (where WSL's launcher lives) must be + // skipped; a bash.exe elsewhere on PATH is returned as an absolute path. + let sys_root = tempdir().expect("sysroot"); + let real = tempdir().expect("real"); + touch(&sys_root.path().join("System32").join("bash.exe")); + let real_bash = real.path().join("bash.exe"); + touch(&real_bash); + + let path_env = + env::join_paths([sys_root.path().join("System32"), real.path().to_path_buf()]) + .expect("join"); + + let found = scan_path_for_bash( + path_env.to_str().expect("utf8"), + Some(sys_root.path().to_path_buf()), + ) + .expect("bash found outside System32"); + assert!(found.is_absolute()); + assert!(!found.starts_with(sys_root.path())); + assert_eq!(found, real_bash); + } + + #[test] + fn path_scan_returns_none_when_only_system32_has_bash() { + // If the ONLY bash.exe on PATH is under System32, the scan finds nothing. + let sys_root = tempdir().expect("sysroot"); + touch(&sys_root.path().join("System32").join("bash.exe")); + let path_env = env::join_paths([sys_root.path().join("System32")]).expect("join"); + + let found = scan_path_for_bash( + path_env.to_str().expect("utf8"), + Some(sys_root.path().to_path_buf()), + ); + assert!(found.is_none()); + } + + #[test] + fn path_scan_skips_system32_when_path_case_differs_from_root() { + // Windows paths are case-insensitive; a PATH entry spelled differently from + // %SystemRoot% (e.g. `...\WINDOWS\System32` vs root `...\Windows`) must STILL + // be excluded, or WSL's bash.exe leaks through. Build the System32 dir under a + // genuinely upper-cased sibling component so the exclusion can only pass via a + // case-insensitive compare, not a literal `starts_with`. + let base = tempdir().expect("base"); + let root = base.path().join("Windows"); + let upper = base.path().join("WINDOWS"); + let sys32 = upper.join("System32"); + touch(&sys32.join("bash.exe")); + + let path_env = env::join_paths([sys32]).expect("join"); + let found = scan_path_for_bash(path_env.to_str().expect("utf8"), Some(root)); + assert!( + found.is_none(), + "case-divergent System32 must still be excluded" + ); + } +} diff --git a/crates/buzz-dev-mcp/src/str_replace.rs b/crates/buzz-dev-mcp/src/str_replace.rs index 7feb0aa10d..cffd65f909 100644 --- a/crates/buzz-dev-mcp/src/str_replace.rs +++ b/crates/buzz-dev-mcp/src/str_replace.rs @@ -257,12 +257,17 @@ mod tests { #[test] fn run_allows_path_outside_workspace() { let dir = tempdir().expect("tempdir"); + // A real file in a SECOND tempdir, genuinely outside the workspace + // root, that does NOT contain our old_str — we expect a "not found" + // error (proving the path resolved), not a path-escape error. Avoids + // the Unix-only /etc/hosts assumption (C:\etc\hosts does not exist). + let outside = tempdir().expect("tempdir"); + let target = outside.path().join("outside.txt"); + fs::write(&target, b"some content").expect("write"); let state = make_state(dir.path()); - // /etc/hosts is readable but won't contain our old_str — we expect - // a "not found" error, not a path-escape error. let p = StrReplaceParams { - path: "/etc/hosts".into(), - old_str: "UNIQUE_STRING_NOT_IN_HOSTS_FILE_abc123".into(), + path: target.display().to_string(), + old_str: "UNIQUE_STRING_NOT_IN_FILE_abc123".into(), new_str: "y".into(), replace_all: false, workdir: Some(dir.path().display().to_string()), diff --git a/crates/buzz-dev-mcp/src/view_image.rs b/crates/buzz-dev-mcp/src/view_image.rs index 5a623b81e0..e7c1679d5a 100644 --- a/crates/buzz-dev-mcp/src/view_image.rs +++ b/crates/buzz-dev-mcp/src/view_image.rs @@ -687,13 +687,18 @@ mod tests { #[tokio::test] async fn allows_path_outside_workspace() { let dir = tempdir().unwrap(); + // A real non-image file in a SECOND tempdir, genuinely outside the + // workspace root — we expect a format error, not a path-escape error, + // proving the traversal limit is gone. Avoids the Unix-only /etc/hosts + // assumption (C:\etc\hosts does not exist on Windows). + let outside = tempdir().unwrap(); + let target = outside.path().join("outside.txt"); + fs::write(&target, b"not an image").unwrap(); let state = make_state(dir.path()); - // /etc/hosts exists but is not an image — we expect a format error, - // not a path-escape error, proving the traversal limit is gone. let res = run( &state, ViewImageParams { - source: "/etc/hosts".into(), + source: target.display().to_string(), max_dim: None, workdir: Some(dir.path().display().to_string()), }, diff --git a/desktop/scripts/build-release-config.mjs b/desktop/scripts/build-release-config.mjs index 389d18aec5..1eea02ad05 100644 --- a/desktop/scripts/build-release-config.mjs +++ b/desktop/scripts/build-release-config.mjs @@ -52,6 +52,27 @@ const releaseConfig = { }, }; +// Windows-only: bundle the PortableGit bash runtime as a resource so the MCP shell +// tool always has a genuine, non-WSL bash to spawn on a bare host (the app must +// be self-contained — we cannot assume Git for Windows is installed). +// +// This is emitted ONLY on the Windows runner because the static tauri.conf.json +// uses `targets: "all"` with a shared bundle block — a bare `resources` entry +// there would ship the ~184MB tree into the macOS .dmg and Linux packages too. +// The release build runs THIS generator on each platform's own runner and merges +// the output via --config, so guarding on process.platform keeps the tree off +// mac/Linux. +// +// PATH CONTRACT (keep byte-identical across three files): +// - source `binaries/git-bash` (relative to src-tauri/) is staged by +// scripts/bundle-sidecars.sh. +// - target `git-bash` is the install-root subdir; Tauri's Windows installer +// stages it next to the exe, and crates/buzz-dev-mcp/src/shell.rs resolves +// `git-bash\bin\bash.exe` relative to its own executable at runtime. +if (process.platform === "win32") { + releaseConfig.bundle.resources = { "binaries/git-bash": "git-bash" }; +} + console.log(`Updater enabled -> ${updaterEndpoint}`); writeFileSync(outputConfigPath, `${JSON.stringify(releaseConfig, null, 2)}\n`); diff --git a/scripts/bundle-sidecars.sh b/scripts/bundle-sidecars.sh index be37cbce0d..3db194a13a 100755 --- a/scripts/bundle-sidecars.sh +++ b/scripts/bundle-sidecars.sh @@ -38,3 +38,12 @@ for bin in "${SIDECARS[@]}"; do cp "$SRC_DIR/${bin}${EXE}" "$BINARIES_DIR/${bin}-${TARGET}${EXE}" done echo "Sidecars bundled for $TARGET" + +# Windows-only: stage a genuine, non-WSL bash next to the sidecars so the MCP +# shell tool works on a bare host. The download/extract/drop logic lives in a +# self-contained script (no release-binary precondition) so CI can call it +# directly to exercise this path on a real Windows runner — see +# scripts/stage-windows-bash.sh for the full rationale and the PATH CONTRACT. +if [[ "$TARGET" == *windows* ]]; then + "$(dirname "$0")/stage-windows-bash.sh" "$BINARIES_DIR/git-bash" +fi diff --git a/scripts/stage-windows-bash.sh b/scripts/stage-windows-bash.sh new file mode 100755 index 0000000000..ece5d08bfb --- /dev/null +++ b/scripts/stage-windows-bash.sh @@ -0,0 +1,73 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Stage a genuine, non-WSL bash for the Windows MCP shell tool. The app is +# self-contained — we cannot assume Git for Windows is installed — so we bundle +# bash rather than probing for an install. +# +# There is no standalone "bash for Windows" upstream: working Windows bash ships +# only inside git-for-windows. We download PortableGit and keep ONLY the MSYS2 +# bash runtime (`usr/` + `bin/`), dropping the `mingw64/` git-program subtree as +# one separable unit (~200MB of git.exe etc. the MCP shell never invokes). We do +# NOT trim INSIDE `usr/`: bash loads `msys-2.0.dll` and other libraries lazily, +# and load-bearing pieces (terminfo, gawk libs) live alongside the docs there, so +# a hand-trimmed copy can pass an existence check yet fail mid-command with a +# cryptic error — exactly the bug class this fixes. The retained runtime is the +# untouched, complete closure git-for-windows maintains. +# +# Self-contained (no release-binary precondition) so CI can call it directly to +# exercise the download/extract/drop path on a real Windows runner — the only +# automated gate on this logic before it ships to users. +# +# Single arg: the destination dir for the staged tree (the real MSYS2 bash lands +# at /usr/bin/bash.exe). Idempotent: a `.stage-complete` marker, written last, +# proves a whole prior stage and skips the re-download; a partial stage lacks it +# and re-extracts cleanly. +# +# PATH CONTRACT (keep byte-identical across three files): +# - dest `git-bash` (== desktop/src-tauri/binaries/git-bash) is the +# `bundle.resources` SOURCE in desktop/scripts/build-release-config.mjs. +# - that resource's TARGET `git-bash` is staged next to the exe by Tauri's +# Windows installer, and crates/buzz-dev-mcp/src/shell.rs resolves +# `git-bash\usr\bin\bash.exe` relative to its own executable at runtime. + +GIT_BASH_DIR=${1:?usage: stage-windows-bash.sh } +PORTABLEGIT_VERSION="2.54.0" +PORTABLEGIT_TAG="v${PORTABLEGIT_VERSION}.windows.1" +PORTABLEGIT_EXE="PortableGit-${PORTABLEGIT_VERSION}-64-bit.7z.exe" +PORTABLEGIT_URL="https://github.com/git-for-windows/git/releases/download/${PORTABLEGIT_TAG}/${PORTABLEGIT_EXE}" + +STAGE_MARKER="$GIT_BASH_DIR/.stage-complete" +if [[ -f "$STAGE_MARKER" ]]; then + echo "PortableGit bash already staged at $GIT_BASH_DIR" + exit 0 +fi + +echo "Downloading PortableGit ${PORTABLEGIT_VERSION}..." +tmp_dir=$(mktemp -d -t portablegit.XXXXXX) +trap 'rm -rf "$tmp_dir"' EXIT +tmp_sfx="$tmp_dir/portablegit.7z.exe" +extract_dir="$tmp_dir/extract" +curl -fsSL "$PORTABLEGIT_URL" -o "$tmp_sfx" +# PortableGit is a 7-Zip self-extracting archive; -o/-y are its SFX flags, +# so we don't need a separate 7z on PATH. +chmod +x "$tmp_sfx" +"$tmp_sfx" -y "-o$extract_dir" + +# Keep the bash runtime whole, drop the separable git-program subtree. +rm -rf "$extract_dir/mingw64" +rm -rf "$GIT_BASH_DIR" +mkdir -p "$GIT_BASH_DIR" +cp -a "$extract_dir/." "$GIT_BASH_DIR/" + +rm -rf "$tmp_dir" +trap - EXIT +[[ -f "$GIT_BASH_DIR/usr/bin/bash.exe" ]] || { + echo "Error: PortableGit extracted but $GIT_BASH_DIR/usr/bin/bash.exe is missing" >&2 + exit 1 +} +# Written last, only after cp -a and the integrity check both succeed, so it is +# positive proof the whole tree landed. An interrupted stage never writes it, so +# the idempotency skip falls through to a clean re-extract. +touch "$STAGE_MARKER" +echo "PortableGit bash staged at $GIT_BASH_DIR (mingw64/ dropped)"