fix(desktop): make the Windows terminal spawn, close, and inherit PATH (#4930)

desktop/src-tauri/crates/buzz-terminal/src/env_fence.rs

@@ -19,13 +19,13 @@
 
 use portable_pty::CommandBuilder;
 
-/// Keys the child is allowed to inherit from Buzz's own environment.
+/// Keys the child is allowed to inherit from Buzz's own environment, on Unix.
 ///
 /// Deliberately minimal: each entry is something a shell genuinely cannot
 /// function without, or that visibly degrades the session by its absence.
 /// Anything not listed here does not reach the child, including keys that do
 /// not exist yet — which is the property a denylist cannot offer.
-const INHERIT_ALLOWLIST: &[&str] = &[
+pub const UNIX_INHERIT_ALLOWLIST: &[&str] = &[
     "HOME",     // shell startup files, ~ expansion
     "USER",     // prompt expansion, `whoami`-adjacent tooling
     "LOGNAME",  // POSIX companion to USER
@@ -36,6 +36,62 @@ const INHERIT_ALLOWLIST: &[&str] = &[
     "TMPDIR",   // per-user temp dir; absence breaks many tools on macOS
 ];
 
+/// The Windows counterpart. Same rule — allowlist, never denylist — but the
+/// set a shell cannot function without is different, and two entries are
+/// load-bearing for the spawn itself, not just the session:
+///
+/// - `ComSpec`: `portable-pty` resolves a default program by reading `ComSpec`
+///   from the **builder's** env map (`cmdbuilder.rs:671-675`), not the process
+///   environment, and hands it to `CreateProcessW` as `lpApplicationName` —
+///   which performs no path search. Fencing it away turned every spawn into
+///   `CreateProcessW "cmd.exe" ... (os error 2)`.
+/// - `USERPROFILE`: same story for the working directory — `portable-pty`
+///   falls back to the builder-env `USERPROFILE` (`cmdbuilder.rs:609-611`);
+///   without it the child starts with `cwd None`.
+///
+/// None of these carry secrets: they are machine/user layout paths and shell
+/// plumbing that every process on the machine can read. The keys the fence
+/// exists to withhold (`BUZZ_PRIVATE_KEY`, `BUZZ_AUTH_TAG`, ...) stay
+/// unlisted, exactly as on Unix.
+/// The list is longer than the Unix one for a structural reason rather than a
+/// permissive one: a Unix shell reads rc files and a Windows shell does not,
+/// so anything omitted here is omitted for the whole session with no mechanism
+/// to restore it. Every entry is machine or user *layout* — paths and counts
+/// any process on the box can read — and the keys the fence exists to withhold
+/// stay unlisted, exactly as on Unix.
+pub const WINDOWS_INHERIT_ALLOWLIST: &[&str] = &[
+    "ComSpec",                // default-program resolution (see above)
+    "SystemRoot",             // required by much of Win32; DLL and WMI resolution
+    "SystemDrive",            // `%SystemDrive%` in scripts and installers
+    "windir",                 // legacy alias of SystemRoot; old scripts read it
+    "PATHEXT",                // which extensions the shell treats as executable
+    "USERPROFILE",            // ~ equivalent; portable-pty's cwd fallback (see above)
+    "HOMEDRIVE",              // POSIX-ish home components used by ports and MSYS tools
+    "HOMEPATH",               // companion to HOMEDRIVE
+    "APPDATA",                // roaming config; PowerShell profiles, npm, git
+    "LOCALAPPDATA",           // local config and caches
+    "ProgramData",            // machine-wide app data; chocolatey, docker, certs
+    "ALLUSERSPROFILE",        // legacy alias of ProgramData
+    "ProgramFiles",           // install root many tool scripts resolve through
+    "ProgramFiles(x86)",      // 32-bit install root on 64-bit Windows
+    "ProgramW6432",           // 64-bit install root as seen from a 32-bit process
+    "PUBLIC",                 // shared user profile; some installers write here
+    "PSModulePath",           // how `powershell` finds its modules at all
+    "TEMP",                   // temp dir; absence breaks cmd internals and most tools
+    "TMP",                    // companion to TEMP
+    "USERNAME",               // prompt expansion, `whoami`-adjacent tooling
+    "USERDOMAIN",             // companion to USERNAME on domain-joined machines
+    "COMPUTERNAME",           // prompt expansion; build scripts label output with it
+    "NUMBER_OF_PROCESSORS",   // parallelism default for cargo, make, ninja
+    "PROCESSOR_ARCHITECTURE", // which binaries a script picks to run
+    "OS",                     // `%OS%`, still branched on by older scripts
+];
+
+#[cfg(unix)]
+const INHERIT_ALLOWLIST: &[&str] = UNIX_INHERIT_ALLOWLIST;
+#[cfg(windows)]
+const INHERIT_ALLOWLIST: &[&str] = WINDOWS_INHERIT_ALLOWLIST;
+
 /// Values Buzz sets on the child unconditionally, overriding any inherited
 /// value. `TERM` in particular must describe *our* emulator, not whatever
 /// terminal happened to launch the desktop app.
@@ -81,5 +137,16 @@ pub fn fence_env(cmd: &mut CommandBuilder, path: &str, shell: &str) {
     // 5. The resolved shell, last. `CommandBuilder::as_command` writes its own
     //    `SHELL` before applying this map (`cmdbuilder.rs:528-536`), so our
     //    explicit entry is the one the child sees.
+    #[cfg(not(windows))]
     cmd.env("SHELL", shell);
+
+    // 5. (Windows) The contract key is `ComSpec`, not `SHELL`, and it is
+    //    doubly load-bearing: besides telling the child what its shell is, it
+    //    is what `portable-pty` spawns for a default program — read from this
+    //    builder's env map and passed to `CreateProcessW` as
+    //    `lpApplicationName`, which does no path search. It must be the
+    //    validated absolute path from `resolve_shell`, overriding whatever
+    //    value step 2 inherited.
+    #[cfg(windows)]
+    cmd.env("ComSpec", shell);
 }

desktop/src-tauri/crates/buzz-terminal/src/env_fence_tests.rs

@@ -4,9 +4,11 @@
 //! assertion against the `CommandBuilder` alone would be weaker: it would not
 //! prove that what the builder holds is what the kernel hands the child.
 
-use crate::env_fence::fence_env;
-use crate::path::user_shell_path;
-use crate::shell::{is_executable_file, login_argv0, resolve_shell, FALLBACK_SHELL};
+use crate::env_fence::{fence_env, WINDOWS_INHERIT_ALLOWLIST};
+use crate::path::{user_shell_path, windows_shell_path};
+use crate::shell::{
+    is_executable_file, login_argv0, pick_windows_shell, resolve_shell, FALLBACK_SHELL,
+};
 use portable_pty::{native_pty_system, CommandBuilder, PtySize};
 use std::io::Read;
 
@@ -326,6 +328,208 @@ fn child_shell_is_the_resolved_shell_not_the_inherited_one() {
     );
 }
 
+/// The Windows allowlist obeys the same law as the Unix one: no secrets, ever.
+/// Windows environment names are case-insensitive, so the comparison here is
+/// too — an allowlist entry of `comspec` and one of `BUZZ_PRIVATE_KEY` differ
+/// only in how obviously wrong they are.
+#[test]
+fn windows_allowlist_admits_no_secrets() {
+    for entry in WINDOWS_INHERIT_ALLOWLIST {
+        let upper = entry.to_ascii_uppercase();
+        assert!(
+            !SECRET_KEYS
+                .iter()
+                .any(|secret| secret.eq_ignore_ascii_case(entry)),
+            "{entry} is a reserved secret and must not be allowlisted"
+        );
+        assert!(
+            !upper.starts_with("BUZZ") && !upper.starts_with("NOSTR"),
+            "{entry}: Buzz/Nostr-namespaced keys are exactly what the fence \
+             exists to withhold"
+        );
+    }
+}
+
+/// The keys the spawn itself depends on must be present: `ComSpec` and
+/// `USERPROFILE` feed `portable-pty`'s default-program and cwd resolution
+/// (`cmdbuilder.rs:671-675`, `:609-611`), and their absence is the confirmed
+/// `CreateProcessW "cmd.exe" in cwd None ... (os error 2)` failure. This
+/// pins them so a future trim of the list cannot silently reintroduce it.
+#[test]
+fn windows_allowlist_covers_the_spawn_contract() {
+    for key in ["ComSpec", "SystemRoot", "PATHEXT", "USERPROFILE"] {
+        assert!(
+            WINDOWS_INHERIT_ALLOWLIST.contains(&key),
+            "{key} is load-bearing for the Windows spawn and must stay \
+             allowlisted"
+        );
+    }
+}
+
+/// A rooted `ComSpec` naming a real file is the user's choice; honour it
+/// verbatim, including the `C:/` forward-slash and `\\server` UNC spellings
+/// Windows itself accepts.
+#[test]
+fn windows_shell_honours_a_valid_comspec() {
+    for candidate in [
+        r"C:\Windows\System32\cmd.exe",
+        r"D:\shells\nu.exe",
+        "C:/Windows/System32/cmd.exe",
+        r"\\server\share\cmd.exe",
+    ] {
+        assert_eq!(
+            pick_windows_shell(Some(candidate), Some(r"C:\Windows"), |path| path
+                == candidate),
+            candidate,
+            "a rooted, existing ComSpec must be used verbatim"
+        );
+    }
+}
+
+/// A `ComSpec` naming a file that does not exist falls through to the
+/// `SystemRoot`-derived absolute fallback — the case that used to become
+/// `CreateProcessW`'s os error 2, because `lpApplicationName` is never
+/// path-searched.
+#[test]
+fn windows_shell_falls_back_when_comspec_is_missing() {
+    let picked = pick_windows_shell(
+        Some(r"C:\definitely\not\real\cmd.exe"),
+        Some(r"D:\CustomRoot"),
+        |_| false,
+    );
+    assert_eq!(picked, r"D:\CustomRoot\System32\cmd.exe");
+}
+
+/// The hijack guard: a relative `ComSpec=cmd.exe` is rejected even when a file
+/// by that name exists, because `lpApplicationName` would execute whatever
+/// `cmd.exe` sits in Buzz's current directory. `is_file` answering `true` is
+/// exactly the attack scenario, so this arm discriminates the rootedness
+/// check from the existence check.
+#[test]
+fn windows_shell_rejects_a_relative_comspec_even_if_the_file_exists() {
+    for candidate in ["cmd.exe", r"tools\cmd.exe", r"\cmd.exe", "C:cmd.exe"] {
+        assert_eq!(
+            pick_windows_shell(Some(candidate), Some(r"C:\Windows"), |_| true),
+            r"C:\Windows\System32\cmd.exe",
+            "{candidate}: a ComSpec not anchored to a drive or UNC share must \
+             not be spawned"
+        );
+    }
+}
+
+/// With no `ComSpec` and no `SystemRoot` at all — a hand-stripped environment
+/// — the resolver still produces an absolute path rather than a bare name.
+#[test]
+fn windows_shell_survives_an_empty_environment() {
+    assert_eq!(
+        pick_windows_shell(None, None, |_| false),
+        r"C:\Windows\System32\cmd.exe"
+    );
+}
+
+/// `SystemRoot` comes from the mutable process environment, so presence alone
+/// does not make the fallback absolute. A relative or empty value must not
+/// recreate the same current-directory hijack rejected for `ComSpec`.
+#[test]
+fn windows_shell_rejects_an_unrooted_system_root() {
+    for system_root in ["", "Windows", r"\Windows", "C:Windows"] {
+        assert_eq!(
+            pick_windows_shell(None, Some(system_root), |_| false),
+            r"C:\Windows\System32\cmd.exe",
+            "{system_root:?}: an unrooted SystemRoot must use the absolute fallback"
+        );
+    }
+}
+
+/// The user's own entries survive, in their own order, ahead of the system
+/// directories we guarantee. This is the regression that stripped `ssh`,
+/// `git`, and every other installed tool from the Windows terminal.
+#[test]
+fn windows_path_keeps_what_the_user_installed() {
+    let inherited = r"C:\Users\dev\.cargo\bin;C:\Program Files\Git\cmd;C:\Windows\System32\OpenSSH";
+    let path = windows_shell_path(Some(inherited), Some(r"C:\Windows"));
+    let entries: Vec<&str> = path.split(';').collect();
+
+    assert_eq!(
+        &entries[..3],
+        &[
+            r"C:\Users\dev\.cargo\bin",
+            r"C:\Program Files\Git\cmd",
+            r"C:\Windows\System32\OpenSSH",
+        ],
+        "inherited entries must keep their order and their precedence"
+    );
+    for required in [
+        r"C:\Windows\System32",
+        r"C:\Windows",
+        r"C:\Windows\System32\Wbem",
+        r"C:\Windows\System32\WindowsPowerShell\v1.0",
+    ] {
+        assert!(
+            entries.contains(&required),
+            "{required} must be reachable however sparse the inherited PATH"
+        );
+    }
+}
+
+/// A `PATH` that already names a system directory must not gain a second copy
+/// of it, whatever case or trailing separator it was written with.
+#[test]
+fn windows_path_does_not_duplicate_system_directories() {
+    let path = windows_shell_path(
+        Some(r"c:\windows\system32\;C:\tools;C:\WINDOWS\System32\Wbem"),
+        Some(r"C:\Windows"),
+    );
+    let entries: Vec<&str> = path.split(';').collect();
+
+    let system32 = entries
+        .iter()
+        .filter(|entry| {
+            entry
+                .trim_end_matches('\\')
+                .eq_ignore_ascii_case(r"C:\Windows\System32")
+        })
+        .count();
+    assert_eq!(
+        system32, 1,
+        "System32 appears once, in the form the user wrote: {path}"
+    );
+    assert!(
+        entries.contains(&r"C:\tools"),
+        "de-duplication must not drop unrelated entries: {path}"
+    );
+}
+
+/// An absent or empty inherited `PATH` still yields a usable shell rather than
+/// an empty string.
+#[test]
+fn windows_path_survives_an_empty_environment() {
+    for inherited in [None, Some(""), Some(";;")] {
+        let path = windows_shell_path(inherited, None);
+        assert_eq!(
+            path,
+            r"C:\Windows\System32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0",
+            "an empty inherited PATH falls back to the system directories alone"
+        );
+    }
+}
+
+/// The system directories appended to PATH obey the same rootedness rule as
+/// the shell fallback; otherwise an invalid `SystemRoot` would still add
+/// current-directory-relative command lookup locations.
+#[test]
+fn windows_path_rejects_an_unrooted_system_root() {
+    let expected = r"C:\tools;C:\Windows\System32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0";
+
+    for system_root in ["", "Windows", r"\Windows", "C:Windows"] {
+        assert_eq!(
+            windows_shell_path(Some(r"C:\tools"), Some(system_root)),
+            expected,
+            "{system_root:?}: PATH must append only rooted system directories"
+        );
+    }
+}
+
 /// Guards the duplication of `RESERVED_ENV_KEYS` above. If the desktop crate
 /// grows a new secret, this points at the file to update.
 #[test]

desktop/src-tauri/crates/buzz-terminal/src/lifecycle.rs

@@ -45,7 +45,9 @@
 //! is correct behaviour, not a gap.
 
 use std::io;
-use std::time::{Duration, Instant};
+use std::time::Duration;
+#[cfg(unix)]
+use std::time::Instant;
 
 use portable_pty::Child;
 
@@ -62,6 +64,7 @@ pub const TERM_GRACE: Duration = Duration::from_millis(250);
 /// Polling rather than blocking in `wait()`: a blocking wait cannot be given a
 /// deadline without a second thread, and the whole point of the grace period
 /// is that it expires.
+#[cfg(unix)]
 const POLL_INTERVAL: Duration = Duration::from_millis(5);
 
 /// How a session ended.
@@ -265,3 +268,53 @@ pub fn shutdown_draining(
     reader.join();
     outcome
 }
+
+/// Ends a session on a platform whose PTY is a ConPTY pseudo console, closing
+/// the console as part of the teardown rather than leaving it to `Drop`.
+///
+/// ConPTY inverts the EOF contract the Unix path is written against. There, the
+/// slave closing when the child dies is what EOFs the master, so the reader
+/// ends on its own and [`DrainingReader::stop`] is only a safety net. Here the
+/// output pipe does **not** EOF when the child dies — it EOFs when
+/// `ClosePseudoConsole` runs, which `portable-pty` performs in the master's
+/// `Drop` (`win/psuedocon.rs:73-75`). A reader parked in `read()` therefore
+/// never observes the stop flag, and joining it while the master is still alive
+/// blocks forever. For a synchronous Tauri command that block lands on the
+/// app's main thread: the whole window stops responding with no panic and no
+/// stack, which is the `AppHangB1` reported as #4930.
+///
+/// So the master is taken **by value** and dropped here, before the join. The
+/// ordering is also what ConPTY itself requires: `ClosePseudoConsole` blocks
+/// until pending output has been consumed, so the reader must still be draining
+/// when it runs, and only then does the reader see EOF and finish.
+///
+/// The child is killed rather than signalled politely because Windows has no
+/// process-group equivalent of the Unix escalation: `portable-pty`'s `kill` is
+/// `TerminateProcess` and its `wait` is `WaitForSingleObject`, both prompt and
+/// both pid-scoped. [`Shutdown::Terminated`] is therefore never returned from
+/// this path — a session either was already gone or was killed.
+#[cfg(not(unix))]
+pub fn shutdown_closing_console(
+    child: &mut Box<dyn Child + Send + Sync>,
+    reader: Box<dyn DrainingReader>,
+    master: Option<Box<dyn portable_pty::MasterPty + Send>>,
+) -> io::Result<Shutdown> {
+    reader.begin_closing();
+
+    let already_exited = child.try_wait()?.is_some();
+    if !already_exited {
+        let _ = child.kill();
+    }
+    child.wait()?;
+
+    // ClosePseudoConsole: the only thing that can EOF the reader.
+    drop(master);
+    reader.stop();
+    reader.join();
+
+    Ok(if already_exited {
+        Shutdown::AlreadyExited
+    } else {
+        Shutdown::Killed
+    })
+}