From 7d86b86d35e86f898b079d4ae051ca19abbec232 Mon Sep 17 00:00:00 2001 From: ARNO Date: Sun, 2 Aug 2026 10:59:47 +0800 Subject: [PATCH] fix(windows): prevent daemon from inheriting policy that blocks scoop junctions (#292) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some Windows shell brokers enforce `ProcessRedirectionTrustPolicy` on what they launch. The daemon inherited it, every ConPTY shell under the daemon inherited it in turn, and PowerShell could then no longer traverse a user-created junction — which is exactly what Scoop's `current` links are. `oh-my-posh` and `fzf` died with `Shim: Could not determine if target is a GUI app`. Windows Terminal was unaffected because its process tree never picked the policy up. The policy cannot be relaxed once enabled, so the fix is to not inherit it: when tty7 detects the enforcing bit, it creates the daemon with `STARTUPINFOEXW` and `PROC_THREAD_ATTRIBUTE_PARENT_PROCESS` naming the interactive desktop shell, which supplies the ordinary desktop token, device map, and mitigation policy. The Win32 code stays isolated in `daemon/spawn/windows.rs`, and the ordinary path still runs whenever the policy is absent — or whenever the desktop shell cannot be borrowed, in which case tty7 logs a warning and starts degraded rather than not at all. Because naming a logical parent makes handle inheritance follow that process, the daemon starts with no standard handles. `daemon::server` and the pane reader's trace line now write to stderr in a way that tolerates that, instead of `eprintln!`, which panics on a failed write. ConPTY exit ordering: the process-exit monitor could observe a short-lived shell exiting before the reader had delivered its final frame, so `Exited` reached clients ahead of the output that preceded it. The monitor now releases the pseudoconsole and lets the reader — which reports only after forwarding everything up to EOF — announce the death, with a bounded window behind it for the case where EOF never arrives because a grandchild holds the ConPTY output pipe open. Note this changes the daemon's token on the clean-parent path: it derives from Explorer, so an elevated tty7 starts a medium-integrity daemon. Co-authored-by: ARNO --- CHANGELOG.md | 30 ++ crates/tty7-core/Cargo.toml | 2 + crates/tty7-core/src/daemon/pane.rs | 380 +++++++++++++++++-- crates/tty7-core/src/daemon/server.rs | 25 +- crates/tty7-core/src/daemon/spawn.rs | 203 +++++++++- crates/tty7-core/src/daemon/spawn/windows.rs | 312 +++++++++++++++ crates/tty7-server/tests/client_lib.rs | 6 +- 7 files changed, 930 insertions(+), 28 deletions(-) create mode 100644 crates/tty7-core/src/daemon/spawn/windows.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f1dbce5..14d83b6a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,36 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 touch one it didn't, so a hand-written skill that happens to share the directory name survives. (#248) +### Fixed + +- **Scoop shims work again when tty7 is launched from a hardened Windows + shell broker** — some brokers enforce `ProcessRedirectionTrustPolicy` on + what they start. The daemon inherited it, every ConPTY shell under the + daemon inherited it in turn, and PowerShell could then no longer traverse + a user-created junction — which is exactly what Scoop's `current` links + are. `oh-my-posh` and `fzf` died with `Shim: Could not determine if target + is a GUI app`. Windows Terminal was unaffected because its process tree + never picked the policy up in the first place. + + The policy cannot be relaxed once it is on, so the fix is to not inherit + it: when tty7 detects the enforcing bit, it creates the daemon with + `PROC_THREAD_ATTRIBUTE_PARENT_PROCESS` naming the interactive desktop + shell, which supplies the ordinary desktop token, device map, and + mitigation policy. Everything else about the spawn is unchanged, and the + ordinary path still runs whenever the policy is absent — or whenever the + desktop shell cannot be borrowed, in which case tty7 logs a warning and + starts degraded rather than not starting at all. (#292) + +- **A pane's last line of output no longer loses the race with its exit on + Windows** — the process-exit monitor could observe a short-lived shell + exiting before the ConPTY reader had delivered its final frame, so + `Exited` reached clients ahead of the output that preceded it. The + monitor now releases the pseudoconsole and lets the reader, which reports + only after it has forwarded everything up to EOF, announce the death. A + bounded window behind it still covers the case where EOF never arrives — + a grandchild holding the ConPTY output pipe open keeps the shell's own + exit from ever closing it. (#292) + ## [26.8.1] - 2026-08-01 ### Added diff --git a/crates/tty7-core/Cargo.toml b/crates/tty7-core/Cargo.toml index b4a73620..feb8cb36 100644 --- a/crates/tty7-core/Cargo.toml +++ b/crates/tty7-core/Cargo.toml @@ -141,9 +141,11 @@ getrandom = "0.3" # tree on hangup (ConPTY's `kill` only reaches the shell itself). windows-sys = { version = "0.59", features = [ "Win32_Foundation", + "Win32_Security", "Win32_System_Console", "Win32_System_Diagnostics_ToolHelp", "Win32_System_Threading", + "Win32_UI_WindowsAndMessaging", ] } [dev-dependencies] diff --git a/crates/tty7-core/src/daemon/pane.rs b/crates/tty7-core/src/daemon/pane.rs index e36129e6..0a55be4c 100644 --- a/crates/tty7-core/src/daemon/pane.rs +++ b/crates/tty7-core/src/daemon/pane.rs @@ -533,7 +533,14 @@ struct ForegroundProbes { } struct PtyBackend { - master: Arc>>, + /// Owns the PTY master until the pane is closed. + /// + /// Windows takes this value when the shell process exits. Dropping the + /// ConPTY master calls `ClosePseudoConsole`, which closes the output side + /// only after its pending bytes can be consumed by the dedicated reader. + /// Keeping the slot optional lets that monitor end the stream without + /// racing the reader for ownership of the exit notification. + master: Arc>>>, child: Arc>>, #[cfg_attr(windows, allow(dead_code))] shell_pid: Option, @@ -582,6 +589,11 @@ impl DeathReporter { *self.exit_code.lock().unwrap() = Some(Box::new(probe)); } + #[cfg(windows)] + fn has_reported(&self) -> bool { + self.reported.load(Ordering::SeqCst) + } + fn report(&self, state: &Mutex, shutting_down: &AtomicBool) { if self.reported.swap(true, Ordering::SeqCst) { return; @@ -614,6 +626,62 @@ impl DeathReporter { } } +/// Releases the PTY master without holding its slot mutex during destruction. +/// +/// `ClosePseudoConsole` may wait while the output pipe is drained, and the same +/// slot is what `resize` locks. Dropping the master only after releasing the +/// mutex keeps a concurrent resize from parking behind a close that is itself +/// waiting on the reader. +#[cfg(windows)] +fn close_pty_master(master: &Mutex>>) { + let owned = master.lock().ok().and_then(|mut slot| slot.take()); + drop(owned); +} + +/// How long the Windows exit monitor lets the reader announce the death on its +/// own before doing it itself. +/// +/// The reader is the better reporter: it publishes `Exited` only after it has +/// forwarded every byte that preceded EOF, which is what keeps a short-lived +/// command's final frame ahead of its exit. But EOF is not guaranteed. A +/// grandchild that inherited the ConPTY output pipe holds it open after the +/// shell is gone — `cmd /c start …` is enough — and `ClosePseudoConsole` then +/// never completes. Without this window such a pane would read as alive +/// forever to every attached client. +#[cfg(windows)] +const EXIT_DRAIN_WINDOW: Duration = Duration::from_secs(2); + +#[cfg(windows)] +const EXIT_DRAIN_POLL: Duration = Duration::from_millis(10); + +/// Releases the pseudoconsole, then reports the pane's death — preferring the +/// reader's EOF-ordered report and falling back to its own after `window`. +/// +/// Must run on a background thread: both halves block. +#[cfg(windows)] +fn drain_then_report( + master: Arc>>>, + state: Arc>, + shutting_down: Arc, + death: Arc, + window: Duration, +) { + // `ClosePseudoConsole` can block until the reader drains the pipe, so it + // cannot run on the thread that owns the deadline below. + std::thread::Builder::new() + .name("tty7-daemon-pane-pty-close".to_string()) + .spawn(move || close_pty_master(&master)) + .expect("spawn daemon pane pty close thread"); + + let deadline = std::time::Instant::now() + window; + while !death.has_reported() && std::time::Instant::now() < deadline { + std::thread::sleep(EXIT_DRAIN_POLL); + } + // `DeathReporter` is idempotent, so this is a no-op whenever the reader + // already got there — which is the ordinary case. + death.report(&state, &shutting_down); +} + /// One out-of-band frame the reader forwards to the subscriber, kept in stream /// order so a kitty image lands at the cursor cell the sender drew it at: a chunk /// carrying graphics splits into `Output` runs interleaved with `Image`/`Delete` @@ -683,7 +751,7 @@ impl DaemonPane { let shutting_down = Arc::new(AtomicBool::new(false)); let gate = Arc::new(OutputGate::new()); - let master = Arc::new(Mutex::new(pair.master)); + let master = Arc::new(Mutex::new(Some(pair.master))); let pane = Arc::new(Self { id, @@ -732,6 +800,7 @@ impl DaemonPane { #[cfg(windows)] Self::spawn_exit_monitor( shell_pid, + master.clone(), state.clone(), pane.shutting_down.clone(), death.clone(), @@ -922,7 +991,11 @@ impl DaemonPane { loop { if trace && tr_last.elapsed() >= std::time::Duration::from_secs(1) { - eprintln!( + // Not `eprintln!`: a daemon can be running without any + // standard error at all (see `daemon::server`), and a + // failed write there would panic the reader thread. + let _ = writeln!( + std::io::stderr(), "[trace daemon] {:.1} MB/s | {} reads ({} B/read) | pty wait {:?} dispatch {:?}", tr_bytes as f64 / tr_last.elapsed().as_secs_f64() / 1e6, tr_reads, @@ -1172,7 +1245,9 @@ impl DaemonPane { match &self.backend { PaneBackend::Pty(p) => { if let Ok(master) = p.master.lock() { - let _ = master.resize(pty_size(size)); + if let Some(master) = master.as_ref() { + let _ = master.resize(pty_size(size)); + } } } PaneBackend::NativeSsh(b) => b.handle.resize(size), @@ -1237,6 +1312,7 @@ impl DaemonPane { #[cfg(windows)] fn spawn_exit_monitor( shell_pid: Option, + master: Arc>>>, state: Arc>, shutting_down: Arc, death: Arc, @@ -1249,7 +1325,15 @@ impl DaemonPane { let Some(pid) = shell_pid else { return }; let handle = unsafe { OpenProcess(PROCESS_SYNCHRONIZE, 0, pid) }; if handle.is_null() { - death.report(&state, &shutting_down); + // The process may have exited before OpenProcess ran, so go + // straight to the close-and-report path — off this thread, since + // pane construction is what calls us. + std::thread::Builder::new() + .name("tty7-daemon-pane-exit-fallback".to_string()) + .spawn(move || { + drain_then_report(master, state, shutting_down, death, EXIT_DRAIN_WINDOW); + }) + .expect("spawn daemon pane exit fallback thread"); return; } let handle = handle as isize; @@ -1261,7 +1345,7 @@ impl DaemonPane { WaitForSingleObject(handle, INFINITE); CloseHandle(handle); } - death.report(&state, &shutting_down); + drain_then_report(master, state, shutting_down, death, EXIT_DRAIN_WINDOW); }) .expect("spawn daemon pane exit monitor thread"); } @@ -1277,7 +1361,7 @@ impl DaemonPane { .master .lock() .ok() - .and_then(|m| m.process_group_leader()); + .and_then(|m| m.as_ref().and_then(|m| m.process_group_leader())); if let Some(fg) = fg { if Some(fg as u32) != pty.shell_pid { unsafe { @@ -1300,7 +1384,7 @@ impl DaemonPane { pty.master .lock() .ok() - .and_then(|m| m.process_group_leader()) + .and_then(|m| m.as_ref().and_then(|m| m.process_group_leader())) .and_then(proc_name) .unwrap_or_default() } @@ -1742,19 +1826,22 @@ fn stamp_launch_argv(st: &mut PaneState, argv: Option>) { } fn foreground_command_running( - master: &Mutex>, + master: &Mutex>>, shell_pid: Option, ) -> bool { is_foreground_command(pty_foreground_pgid(master), shell_pid) } #[cfg(unix)] -fn pty_foreground_pgid(master: &Mutex>) -> Option { - master.lock().ok().and_then(|m| m.process_group_leader()) +fn pty_foreground_pgid(master: &Mutex>>) -> Option { + master + .lock() + .ok() + .and_then(|m| m.as_ref().and_then(|m| m.process_group_leader())) } #[cfg(not(unix))] -fn pty_foreground_pgid(_master: &Mutex>) -> Option { +fn pty_foreground_pgid(_master: &Mutex>>) -> Option { None } @@ -1767,7 +1854,7 @@ fn is_foreground_command(fg_pgid: Option, shell_pid: Option) -> bool { #[cfg(target_os = "macos")] fn foreground_cwd( - master: &Mutex>, + master: &Mutex>>, shell_pid: Option, ) -> Option { use std::ffi::CStr; @@ -1807,7 +1894,7 @@ fn foreground_cwd( #[cfg(target_os = "linux")] fn foreground_cwd( - master: &Mutex>, + master: &Mutex>>, shell_pid: Option, ) -> Option { let read_cwd = |pid: i32| -> Option { @@ -1824,30 +1911,40 @@ fn foreground_cwd( #[cfg(not(any(target_os = "macos", target_os = "linux")))] fn foreground_cwd( - _master: &Mutex>, + _master: &Mutex>>, _shell_pid: Option, ) -> Option { None } #[cfg(any(target_os = "macos", target_os = "linux"))] -fn foreground_remote_context(master: &Mutex>) -> Option { - let pid = master.lock().ok().and_then(|m| m.process_group_leader())?; +fn foreground_remote_context( + master: &Mutex>>, +) -> Option { + let pid = master + .lock() + .ok() + .and_then(|m| m.as_ref().and_then(|m| m.process_group_leader()))?; let argv = crate::daemon::remote::foreground_argv(pid)?; crate::daemon::remote::parse_ssh_invocation(&argv).map(|inv| inv.context) } #[cfg(not(any(target_os = "macos", target_os = "linux")))] -fn foreground_remote_context(_master: &Mutex>) -> Option { +fn foreground_remote_context( + _master: &Mutex>>, +) -> Option { None } #[cfg(any(target_os = "macos", target_os = "linux"))] fn foreground_agent( - master: &Mutex>, + master: &Mutex>>, ) -> Option)>> { let detect = || { - let pid = master.lock().ok().and_then(|m| m.process_group_leader())?; + let pid = master + .lock() + .ok() + .and_then(|m| m.as_ref().and_then(|m| m.process_group_leader()))?; let argv = crate::daemon::remote::foreground_argv(pid)?; let agent = crate::core::cli_agent::CLIAgent::detect_from_argv_with( &argv, @@ -1860,7 +1957,7 @@ fn foreground_agent( #[cfg(not(any(target_os = "macos", target_os = "linux")))] fn foreground_agent( - _master: &Mutex>, + _master: &Mutex>>, ) -> Option)>> { None } @@ -2142,7 +2239,7 @@ mod tests { let mut cmd = CommandBuilder::new("bash"); cmd.args(["-c", "exec -a codex cat"]); let mut child = pty.slave.spawn_command(cmd).expect("spawn child"); - let master = Mutex::new(pty.master); + let master = Mutex::new(Some(pty.master)); let mut detected = None; for _ in 0..200 { @@ -3480,6 +3577,245 @@ mod tests { ); } + #[cfg(windows)] + #[test] + fn closing_conpty_waits_for_delayed_reader_output_before_exit() { + /// A master whose destruction opens the simulated ConPTY output pipe. + /// The production `ConPtyMasterPty` performs the equivalent transition + /// by calling `ClosePseudoConsole` from its destructor. + struct SignallingMaster { + released: Arc<(Mutex, Condvar)>, + } + + impl Drop for SignallingMaster { + fn drop(&mut self) { + let (lock, ready) = &*self.released; + *lock.lock().unwrap() = true; + ready.notify_all(); + } + } + + impl MasterPty for SignallingMaster { + fn resize(&self, _size: PtySize) -> anyhow::Result<()> { + Ok(()) + } + + fn get_size(&self) -> anyhow::Result { + Ok(PtySize::default()) + } + + fn try_clone_reader(&self) -> anyhow::Result> { + Ok(Box::new(std::io::empty())) + } + + fn take_writer(&self) -> anyhow::Result> { + Ok(Box::new(std::io::sink())) + } + } + + /// Models a reader that stays delayed well past any plausible grace + /// period, then receives a final output chunk followed by EOF. + struct DelayedTailReader { + released: Arc<(Mutex, Condvar)>, + tail: std::io::Cursor>, + delayed: bool, + } + + impl Read for DelayedTailReader { + fn read(&mut self, buf: &mut [u8]) -> std::io::Result { + if !self.delayed { + let (lock, ready) = &*self.released; + let mut released = lock.lock().unwrap(); + while !*released { + released = ready.wait(released).unwrap(); + } + drop(released); + std::thread::sleep(Duration::from_millis(800)); + self.delayed = true; + } + self.tail.read(buf) + } + } + + let released = Arc::new((Mutex::new(false), Condvar::new())); + let master: Mutex>> = + Mutex::new(Some(Box::new(SignallingMaster { + released: released.clone(), + }))); + let state = Arc::new(Mutex::new(test_state(true))); + let (sub_tx, sub_rx) = mpsc::channel(); + state.lock().unwrap().subscriber = Some(sub_tx); + + let reader = DaemonPane::spawn_reader( + state, + Arc::new(AtomicBool::new(false)), + Arc::new(OutputGate::new()), + Box::new(DelayedTailReader { + released, + tail: std::io::Cursor::new(b"final output".to_vec()), + delayed: false, + }), + null_writer(), + || false, + ForegroundProbes { + remote: Box::new(|| None), + agent: Box::new(|| None), + cwd: Box::new(|| None), + }, + Arc::new(DeathReporter::new(|| {})), + ); + + close_pty_master(&master); + assert!( + sub_rx.recv_timeout(Duration::from_millis(600)).is_err(), + "closing the master must not publish Exited while the reader is still delayed" + ); + assert!(matches!( + sub_rx.recv_timeout(Duration::from_secs(1)), + Ok(DaemonMsg::Output(bytes)) if bytes == b"final output" + )); + assert!(matches!( + sub_rx.recv_timeout(Duration::from_secs(1)), + Ok(DaemonMsg::Exited { code: None }) + )); + assert!(sub_rx.try_recv().is_err(), "no output may follow Exited"); + reader.join().unwrap(); + } + + /// An inert stand-in for the ConPTY master: releasing it is instant, which + /// is what `ClosePseudoConsole` does once the pipe has no pending bytes. + #[cfg(windows)] + struct InertMaster; + + #[cfg(windows)] + impl MasterPty for InertMaster { + fn resize(&self, _size: PtySize) -> anyhow::Result<()> { + Ok(()) + } + + fn get_size(&self) -> anyhow::Result { + Ok(PtySize::default()) + } + + fn try_clone_reader(&self) -> anyhow::Result> { + Ok(Box::new(std::io::empty())) + } + + fn take_writer(&self) -> anyhow::Result> { + Ok(Box::new(std::io::sink())) + } + } + + #[cfg(windows)] + fn exit_drain_fixture() -> ( + Arc>>>, + Arc>, + Arc, + Arc, + mpsc::Receiver, + ) { + let state = Arc::new(Mutex::new(test_state(true))); + let (sub_tx, sub_rx) = mpsc::channel(); + state.lock().unwrap().subscriber = Some(sub_tx); + ( + Arc::new(Mutex::new(Some( + Box::new(InertMaster) as Box + ))), + state, + Arc::new(AtomicBool::new(false)), + Arc::new(DeathReporter::new(|| {})), + sub_rx, + ) + } + + /// A grandchild that inherited the ConPTY output pipe keeps it open after + /// the shell is gone, so the reader never sees EOF and can never publish + /// the exit. The monitor's drain window is the only thing that stops such + /// a pane from reading as alive forever. + #[cfg(windows)] + #[test] + fn a_pty_that_never_reaches_eof_still_reports_the_exit() { + struct NeverEofReader; + + impl Read for NeverEofReader { + fn read(&mut self, _buf: &mut [u8]) -> std::io::Result { + // Long enough to outlast the window below, bounded so the + // thread cannot outlive the test binary. + std::thread::sleep(Duration::from_secs(5)); + Ok(0) + } + } + + let (master, state, shutting_down, death, sub_rx) = exit_drain_fixture(); + let _reader = DaemonPane::spawn_reader( + state.clone(), + shutting_down.clone(), + Arc::new(OutputGate::new()), + Box::new(NeverEofReader), + null_writer(), + || false, + ForegroundProbes { + remote: Box::new(|| None), + agent: Box::new(|| None), + cwd: Box::new(|| None), + }, + death.clone(), + ); + + drain_then_report( + master, + state, + shutting_down, + death, + Duration::from_millis(50), + ); + assert!( + matches!( + sub_rx.recv_timeout(Duration::from_secs(1)), + Ok(DaemonMsg::Exited { code: None }) + ), + "the monitor must report the exit once its drain window elapses" + ); + } + + /// The ordinary case: EOF arrives, the reader reports, and the monitor + /// neither waits out its window nor publishes a second `Exited`. + #[cfg(windows)] + #[test] + fn the_exit_monitor_defers_to_the_reader_that_saw_eof() { + let (master, state, shutting_down, death, sub_rx) = exit_drain_fixture(); + let reader = DaemonPane::spawn_reader( + state.clone(), + shutting_down.clone(), + Arc::new(OutputGate::new()), + Box::new(std::io::empty()), + null_writer(), + || false, + ForegroundProbes { + remote: Box::new(|| None), + agent: Box::new(|| None), + cwd: Box::new(|| None), + }, + death.clone(), + ); + + let started = std::time::Instant::now(); + drain_then_report(master, state, shutting_down, death, Duration::from_secs(30)); + assert!( + started.elapsed() < Duration::from_secs(5), + "the monitor must stop waiting as soon as the reader has reported" + ); + assert!(matches!( + sub_rx.recv_timeout(Duration::from_secs(1)), + Ok(DaemonMsg::Exited { code: None }) + )); + assert!( + sub_rx.try_recv().is_err(), + "the idempotent reporter must not publish a second Exited" + ); + reader.join().unwrap(); + } + /// Issue #213 end-to-end at the reader: a chunk carrying text plus a /// kitty graphics query and a transmit-and-display must (1) keep only the /// text in the replay ring and the `Output` frame, (2) write the `a=q` reply diff --git a/crates/tty7-core/src/daemon/server.rs b/crates/tty7-core/src/daemon/server.rs index 6328581e..d7fef094 100644 --- a/crates/tty7-core/src/daemon/server.rs +++ b/crates/tty7-core/src/daemon/server.rs @@ -147,6 +147,23 @@ fn ssh_connection_for( }) } +/// `eprintln!` for a process that may have no standard error. +/// +/// A daemon started through the Windows clean-parent path (see +/// `daemon::spawn::windows`) has NULL standard handles: naming a logical parent +/// makes handle inheritance follow *that* process, so tty7 cannot hand the +/// child a `NUL` handle the way the ordinary `Stdio::null()` path does. On +/// Windows a failed write to stderr makes `eprintln!` panic, which would kill +/// the daemon before it serves anything. These notes are diagnostics for +/// someone running the daemon in a terminal; dropping them is the right +/// outcome when nobody is there to read them. +macro_rules! startup_note { + ($($arg:tt)*) => {{ + use std::io::Write as _; + let _ = writeln!(std::io::stderr(), $($arg)*); + }}; +} + pub fn run_daemon() -> anyhow::Result<()> { let registry = Arc::new(Registry::new()); @@ -158,8 +175,8 @@ pub fn run_daemon() -> anyhow::Result<()> { crate::host::local::LocalHost::shared(), services, ) { - Ok(path) => eprintln!("tty7-server: control socket at {}", path.display()), - Err(e) => eprintln!("tty7-server: control listener unavailable: {e}"), + Ok(path) => startup_note!("tty7-server: control socket at {}", path.display()), + Err(e) => startup_note!("tty7-server: control listener unavailable: {e}"), } } #[cfg(not(any(unix, windows)))] @@ -172,12 +189,12 @@ pub fn control_services() -> crate::host::server::Services { use crate::core::machine::MachineStore; match MachineStore::shared() { Ok(machine) => { - eprintln!("machine tree at {}", machine.path().display()); + startup_note!("machine tree at {}", machine.path().display()); crate::core::machine::publish_observations(&machine); crate::host::server::Services::with_machine(machine) } Err(e) => { - eprintln!("no machine tree ({e}); serving files and panes only"); + startup_note!("no machine tree ({e}); serving files and panes only"); crate::host::server::Services::none() } } diff --git a/crates/tty7-core/src/daemon/spawn.rs b/crates/tty7-core/src/daemon/spawn.rs index e544b3d0..978e1ab1 100644 --- a/crates/tty7-core/src/daemon/spawn.rs +++ b/crates/tty7-core/src/daemon/spawn.rs @@ -7,6 +7,9 @@ use crate::core::config; use crate::daemon::protocol::{ClientMsg, DaemonMsg, DaemonVersion, PROTOCOL_VERSION}; use crate::daemon::{pidfile, transport}; +#[cfg(windows)] +mod windows; + const STARTUP_TIMEOUT: Duration = Duration::from_secs(3); const POLL_INTERVAL: Duration = Duration::from_millis(50); const HANDSHAKE_TIMEOUT: Duration = Duration::from_secs(2); @@ -276,10 +279,39 @@ fn spawn_detached() -> anyhow::Result<()> { let exe = std::env::current_exe() .map_err(|e| anyhow::anyhow!("could not locate own executable: {e}"))?; + let config_dir = config::config_dir_path(); + + // Some Windows shell brokers enforce Redirection Trust on processes they + // launch. An ordinary child inherits it and cannot traverse Scoop's + // user-created `current` junctions. The policy cannot be relaxed in place, + // so create the daemon through the clean interactive desktop shell only + // when the enforcing bit is actually present. + #[cfg(windows)] + if windows::redirection_trust_enforced() { + let mut args = vec![std::ffi::OsString::from("--daemon")]; + if let Some(dir) = &config_dir { + args.push(std::ffi::OsString::from("--config-dir")); + args.push(dir.as_os_str().to_owned()); + } + match windows::spawn_detached_with_clean_parent(&exe, &args) { + Ok(()) => return Ok(()), + // The clean parent is unavailable whenever there is no interactive + // Explorer to borrow — it is restarting, the shell was replaced, or + // the session has no desktop at all. Losing it only costs junction + // traversal inside the shells, so degrade to the ordinary path + // instead of refusing to start tty7 at all. + Err(error) => log::warn!( + "could not spawn the daemon through the Windows desktop shell while \ + Redirection Trust is enforced ({error}); falling back to the ordinary \ + path, where Scoop-style junctions may be unreachable" + ), + } + } + let mut cmd = Command::new(exe); cmd.arg("--daemon"); - if let Some(dir) = config::config_dir_path() { + if let Some(dir) = config_dir { cmd.arg("--config-dir").arg(dir); } @@ -368,6 +400,175 @@ fn detach(cmd: &mut Command) { cmd.creation_flags(DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP | CREATE_NO_WINDOW); } +#[cfg(all(test, windows))] +mod windows_spawn_tests { + use super::*; + use std::mem::size_of; + use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; + use windows_sys::Win32::System::Threading::{ + ProcessRedirectionTrustPolicy, SetProcessMitigationPolicy, + }; + + const INNER_ENV: &str = "TTY7_REDIRECTION_TRUST_INNER"; + const CLEAN_PARENT_ENV: &str = "TTY7_REDIRECTION_TRUST_CLEAN_PARENT"; + const PROBE_RESULT_ENV: &str = "TTY7_REDIRECTION_TRUST_PROBE_RESULT"; + const TEST_NAME: &str = + "daemon::spawn::windows_spawn_tests::daemon_spawn_does_not_inherit_redirection_trust"; + + /// Redirection Trust cannot be disabled after it is enforced, so the outer + /// test delegates the destructive policy change to a short-lived copy of + /// the test executable. This keeps the remaining test process clean. + #[test] + fn daemon_spawn_does_not_inherit_redirection_trust() { + // A probe process reports both its inherited policy and whether it can + // traverse the fixture. Checking the policy directly keeps this test + // deterministic on elevated CI runners, whose own junctions may remain + // trusted even while Redirection Trust is enforced. + if let Some(result) = std::env::var_os(PROBE_RESULT_ENV) { + let result = PathBuf::from(result); + let junction_probe = result + .parent() + .expect("probe result has a fixture directory") + .join("current") + .join("probe.txt"); + let verdict = if windows::redirection_trust_enforced() { + "ENFORCED" + } else if junction_probe.exists() { + "CLEAN_OK" + } else { + "CLEAN_BLOCKED" + }; + std::fs::write(result, verdict).expect("write mitigation probe verdict"); + return; + } + + if std::env::var_os(INNER_ENV).is_none() { + let output = Command::new(std::env::current_exe().expect("locate test executable")) + .args(["--exact", TEST_NAME, "--nocapture"]) + .env(INNER_ENV, "1") + .env(CLEAN_PARENT_ENV, std::process::id().to_string()) + .output() + .expect("spawn isolated mitigation test process"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + output.status.success(), + "isolated mitigation test failed:\nstdout:\n{stdout}\nstderr:\n{}", + String::from_utf8_lossy(&output.stderr) + ); + // libtest exits 0 when `--exact` matches nothing, so a stale + // TEST_NAME would turn this whole regression into a silent pass. + assert!( + stdout.contains("1 passed"), + "the isolated mitigation test must actually run; TEST_NAME is probably stale:\n{stdout}" + ); + return; + } + + let clean_parent_pid: u32 = std::env::var(CLEAN_PARENT_ENV) + .expect("clean parent pid") + .parse() + .expect("clean parent pid is numeric"); + + let unique = SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("system time after epoch") + .as_nanos(); + let root = std::env::temp_dir().join(format!( + "tty7-redirection-trust-{}-{unique}", + std::process::id() + )); + let target = root.join("version"); + let junction = root.join("current"); + let inherited_result = root.join("inherited-result.txt"); + let result = root.join("result.txt"); + let batch = root.join("junction probe.cmd"); + std::fs::create_dir_all(&target).expect("create junction target"); + std::fs::write(target.join("probe.txt"), b"ok").expect("write junction probe"); + + let comspec = std::env::var_os("ComSpec").unwrap_or_else(|| "cmd.exe".into()); + let linked = Command::new(&comspec) + .args(["/d", "/c", "mklink", "/J"]) + .arg(&junction) + .arg(&target) + .status() + .expect("create junction fixture"); + assert!(linked.success(), "mklink must create the junction fixture"); + + let mut policy = 1u32; + // SAFETY: The DWORD buffer exactly matches the Windows policy layout, + // and only this disposable inner test process receives the policy. + let enabled = unsafe { + SetProcessMitigationPolicy( + ProcessRedirectionTrustPolicy, + (&raw mut policy).cast(), + size_of::(), + ) + }; + assert!( + enabled != 0, + "enable Redirection Trust: {}", + io::Error::last_os_error() + ); + assert!( + windows::redirection_trust_enforced(), + "tty7 must detect the enforcing policy before selecting the alternate spawn path" + ); + + // First prove that an ordinary child inherits the enforced policy. + // This is the red-capable half of the regression and does not depend on + // how Windows classifies the junction created by the current account. + let inherited = Command::new(std::env::current_exe().expect("locate test executable")) + .args(["--exact", TEST_NAME, "--nocapture"]) + .env(PROBE_RESULT_ENV, &inherited_result) + .status() + .expect("spawn ordinary mitigation probe"); + assert!(inherited.success(), "ordinary mitigation probe must run"); + assert_eq!( + std::fs::read_to_string(&inherited_result) + .expect("read inherited mitigation verdict") + .trim(), + "ENFORCED", + "an ordinary child must demonstrate the policy inheritance that the alternate spawn path removes" + ); + + // The clean helper inherits INNER_ENV from this disposable process. A + // batch wrapper adds the result path before launching another copy of + // the test executable, whose first branch records its actual policy. + let test_exe = std::env::current_exe().expect("locate test executable"); + let script = format!( + "@echo off\r\nset \"{PROBE_RESULT_ENV}={}\"\r\n\"{}\" --exact \"{TEST_NAME}\" --nocapture\r\n", + result.display(), + test_exe.display(), + ); + std::fs::write(&batch, script).expect("write mitigation probe batch"); + + windows::spawn_detached_with_parent( + Path::new(&comspec), + &[ + std::ffi::OsString::from("/d"), + std::ffi::OsString::from("/c"), + batch.as_os_str().to_owned(), + ], + clean_parent_pid, + ) + .expect("spawn mitigation probe through clean logical parent"); + + let deadline = Instant::now() + Duration::from_secs(5); + while !result.exists() && Instant::now() < deadline { + std::thread::sleep(Duration::from_millis(25)); + } + + let verdict = std::fs::read_to_string(&result).unwrap_or_else(|_| "MISSING".into()); + let _ = std::fs::remove_dir(&junction); + let _ = std::fs::remove_dir_all(&root); + assert_eq!( + verdict.trim(), + "CLEAN_OK", + "a tty7 daemon child must drop the inherited policy and retain access to user-created junctions" + ); + } +} + #[cfg(test)] mod exe_name_tests { use super::*; diff --git a/crates/tty7-core/src/daemon/spawn/windows.rs b/crates/tty7-core/src/daemon/spawn/windows.rs new file mode 100644 index 00000000..6afa7e84 --- /dev/null +++ b/crates/tty7-core/src/daemon/spawn/windows.rs @@ -0,0 +1,312 @@ +use std::ffi::{OsStr, OsString, c_void}; +use std::io; +use std::mem::size_of; +use std::os::windows::ffi::OsStrExt; +use std::path::Path; +use std::ptr; + +use windows_sys::Win32::Foundation::{CloseHandle, HANDLE}; +use windows_sys::Win32::System::Threading::{ + CREATE_NEW_PROCESS_GROUP, CREATE_NO_WINDOW, CreateProcessW, DETACHED_PROCESS, + DeleteProcThreadAttributeList, EXTENDED_STARTUPINFO_PRESENT, GetCurrentProcess, + GetProcessMitigationPolicy, InitializeProcThreadAttributeList, LPPROC_THREAD_ATTRIBUTE_LIST, + OpenProcess, PROC_THREAD_ATTRIBUTE_PARENT_PROCESS, PROCESS_CREATE_PROCESS, PROCESS_INFORMATION, + ProcessRedirectionTrustPolicy, STARTUPINFOEXW, UpdateProcThreadAttribute, +}; +use windows_sys::Win32::UI::WindowsAndMessaging::{GetShellWindow, GetWindowThreadProcessId}; + +/// Returns whether the current process has the enforcing Redirection Trust bit. +/// +/// Query failures deliberately fall back to the ordinary spawn path. The +/// alternate parent is only necessary when Windows confirms the policy is on. +pub(super) fn redirection_trust_enforced() -> bool { + let mut flags = 0u32; + // SAFETY: GetCurrentProcess returns a pseudo-handle that must not be closed, + // and `flags` is the exact DWORD-sized buffer required by this policy. + let ok = unsafe { + GetProcessMitigationPolicy( + GetCurrentProcess(), + ProcessRedirectionTrustPolicy, + (&raw mut flags).cast(), + size_of::(), + ) + }; + ok != 0 && flags & 0x1 != 0 +} + +/// Creates a detached process using the interactive desktop shell as its +/// logical parent, which supplies the normal user token, device map, and +/// mitigation policy instead of inheriting a hardened shell broker's policy. +pub(super) fn spawn_detached_with_clean_parent( + program: &Path, + args: &[OsString], +) -> io::Result<()> { + let parent_pid = desktop_shell_pid()?; + spawn_detached_with_parent(program, args, parent_pid) +} + +/// Returns the process id that owns the current interactive desktop shell. +/// +/// GetShellWindow identifies the correct Explorer instance for the active +/// desktop and avoids accidentally selecting a transient `/factory` broker. +fn desktop_shell_pid() -> io::Result { + // SAFETY: Both calls only query the interactive desktop, and `pid` points + // to a valid writable DWORD for the duration of the call. + let window = unsafe { GetShellWindow() }; + if window.is_null() { + return Err(io::Error::new( + io::ErrorKind::NotFound, + "Windows desktop shell window is unavailable", + )); + } + + let mut pid = 0u32; + unsafe { GetWindowThreadProcessId(window, &raw mut pid) }; + if pid == 0 { + return Err(io::Error::last_os_error()); + } + Ok(pid) +} + +/// Owns a Windows handle and closes it exactly once on every return path. +struct OwnedHandle(HANDLE); + +impl Drop for OwnedHandle { + fn drop(&mut self) { + if !self.0.is_null() { + // SAFETY: Each wrapped handle comes from a successful OpenProcess + // or CreateProcessW call and is moved into exactly one owner. + unsafe { CloseHandle(self.0) }; + } + } +} + +/// The only attribute a daemon spawn sets. +/// +/// Deliberately just the logical parent. Handing the child a `NUL` handle for +/// its standard streams — the Win32 spelling of the ordinary path's +/// `Stdio::null()` — is not possible here: naming a logical parent makes +/// handle inheritance follow *that* process, not tty7, so a handle from our +/// own table would not survive the transition. The daemon therefore starts +/// with no standard handles at all, and `daemon::server` is written to +/// tolerate that. +const ATTRIBUTE_COUNT: u32 = 1; + +/// Owns an initialized PROC_THREAD_ATTRIBUTE_LIST and the values it points at. +struct SpawnAttributes { + // The native structure is opaque but contains pointer-width fields. usize + // storage guarantees the required alignment instead of relying on Vec. + storage: Vec, + // UpdateProcThreadAttribute stores pointers to these values rather than + // copying them. Keep their addresses stable until CreateProcessW returns. + _parent_value: Box, +} + +impl SpawnAttributes { + fn new(parent: HANDLE) -> io::Result { + let mut bytes = 0usize; + // The first call is the documented size query and is expected to fail + // while filling `bytes` with the required allocation size. + unsafe { + InitializeProcThreadAttributeList(ptr::null_mut(), ATTRIBUTE_COUNT, 0, &raw mut bytes) + }; + if bytes == 0 { + let error = io::Error::last_os_error(); + return Err(io::Error::new( + error.kind(), + format!("query process attribute-list size: {error}"), + )); + } + + let words = bytes.div_ceil(size_of::()); + let mut storage = vec![0usize; words]; + let list = storage.as_mut_ptr().cast(); + // SAFETY: The buffer is aligned, large enough for the reported byte + // count, and will not be reallocated while the native list is alive. + if unsafe { InitializeProcThreadAttributeList(list, ATTRIBUTE_COUNT, 0, &raw mut bytes) } + == 0 + { + let error = io::Error::last_os_error(); + return Err(io::Error::new( + error.kind(), + format!("initialize process attribute list: {error}"), + )); + } + + let parent_value = Box::new(parent); + // PARENT_PROCESS supplies the token, device map, and mitigation policy. + let attributes: [(u32, *const c_void, usize); ATTRIBUTE_COUNT as usize] = [( + PROC_THREAD_ATTRIBUTE_PARENT_PROCESS, + (&raw const *parent_value).cast(), + size_of::(), + )]; + for (attribute, value, size) in attributes { + // SAFETY: `list` is initialized, and each value has the exact size + // and a stable address that outlives the attribute list. + let updated = unsafe { + UpdateProcThreadAttribute( + list, + 0, + attribute as usize, + value, + size, + ptr::null_mut(), + ptr::null(), + ) + }; + if updated == 0 { + let error = io::Error::last_os_error(); + // SAFETY: Initialization succeeded, so the native list must be + // released before returning the attribute update error. + unsafe { DeleteProcThreadAttributeList(list) }; + return Err(io::Error::new( + error.kind(), + format!("set process attribute {attribute:#x}: {error}"), + )); + } + } + + Ok(Self { + storage, + _parent_value: parent_value, + }) + } + + fn as_mut_ptr(&mut self) -> LPPROC_THREAD_ATTRIBUTE_LIST { + self.storage.as_mut_ptr().cast() + } +} + +impl Drop for SpawnAttributes { + fn drop(&mut self) { + // SAFETY: Construction only succeeds after native initialization, and + // the backing storage remains allocated until this destructor returns. + unsafe { DeleteProcThreadAttributeList(self.as_mut_ptr()) }; + } +} + +/// Appends one argument using the Windows CRT command-line quoting rules. +fn push_quoted_arg(command_line: &mut Vec, arg: &OsStr) { + command_line.push(b'"' as u16); + let mut backslashes = 0usize; + for unit in arg.encode_wide() { + if unit == b'\\' as u16 { + backslashes += 1; + continue; + } + if unit == b'"' as u16 { + command_line.extend(std::iter::repeat_n(b'\\' as u16, backslashes * 2 + 1)); + command_line.push(unit); + } else { + command_line.extend(std::iter::repeat_n(b'\\' as u16, backslashes)); + command_line.push(unit); + } + backslashes = 0; + } + // Backslashes immediately before the closing quote must be doubled so + // they cannot escape that quote and merge adjacent arguments. + command_line.extend(std::iter::repeat_n(b'\\' as u16, backslashes * 2)); + command_line.push(b'"' as u16); +} + +/// Creates a detached child whose logical parent is `parent_pid`. +/// +/// The actual caller remains tty7, but Windows derives the child token, device +/// map, and mitigation policy from the process named by PARENT_PROCESS. +pub(super) fn spawn_detached_with_parent( + program: &Path, + args: &[OsString], + parent_pid: u32, +) -> io::Result<()> { + // PROCESS_CREATE_PROCESS is the only access right required when a process + // handle is supplied through PROC_THREAD_ATTRIBUTE_PARENT_PROCESS. + // SAFETY: OpenProcess only reads `parent_pid` and returns an owned handle + // or NULL; nothing here outlives the wrapper installed below. + let parent = unsafe { OpenProcess(PROCESS_CREATE_PROCESS, 0, parent_pid) }; + if parent.is_null() { + let error = io::Error::last_os_error(); + return Err(io::Error::new( + error.kind(), + format!("open logical parent process {parent_pid}: {error}"), + )); + } + let parent = OwnedHandle(parent); + let mut attributes = SpawnAttributes::new(parent.0)?; + + let application: Vec = program.as_os_str().encode_wide().chain([0]).collect(); + let mut command_line = Vec::new(); + push_quoted_arg(&mut command_line, program.as_os_str()); + for arg in args { + command_line.push(b' ' as u16); + push_quoted_arg(&mut command_line, arg); + } + command_line.push(0); + + // SAFETY: Zero initialization is the required baseline for both Win32 + // structures. The size and initialized attribute-list pointer are then set. + let mut startup: STARTUPINFOEXW = unsafe { std::mem::zeroed() }; + startup.StartupInfo.cb = size_of::() as u32; + startup.lpAttributeList = attributes.as_mut_ptr(); + let mut process_info: PROCESS_INFORMATION = unsafe { std::mem::zeroed() }; + + let flags = DETACHED_PROCESS + | CREATE_NEW_PROCESS_GROUP + | CREATE_NO_WINDOW + | EXTENDED_STARTUPINFO_PRESENT; + // SAFETY: Both strings are NUL-terminated, the command line is writable, + // and every pointer, handle, and output structure remains alive throughout + // CreateProcessW. Handle inheritance is intentionally disabled. + let created = unsafe { + CreateProcessW( + application.as_ptr(), + command_line.as_mut_ptr(), + ptr::null(), + ptr::null(), + 0, + flags, + ptr::null(), + ptr::null(), + &raw const startup.StartupInfo, + &raw mut process_info, + ) + }; + if created == 0 { + let error = io::Error::last_os_error(); + return Err(io::Error::new( + error.kind(), + format!("CreateProcessW with logical parent {parent_pid}: {error}"), + )); + } + + // The child is intentionally independent and long-lived. Closing our two + // handles mirrors dropping std::process::Child without waiting or killing. + let _process = OwnedHandle(process_info.hProcess); + let _thread = OwnedHandle(process_info.hThread); + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn quoted(arg: &str) -> String { + let mut units = Vec::new(); + push_quoted_arg(&mut units, OsStr::new(arg)); + String::from_utf16(&units).expect("the quoter only emits units it was given plus ASCII") + } + + /// The daemon's `--config-dir` argument is a user path, so it can end in a + /// backslash or carry a quote. Both need the CRT's doubling rules; getting + /// them wrong silently merges the argument with the next one. + #[test] + fn arguments_survive_the_crt_quoting_rules() { + assert_eq!(quoted("--daemon"), r#""--daemon""#); + assert_eq!(quoted(""), r#""""#); + assert_eq!(quoted(r"C:\Users\me\tty7"), r#""C:\Users\me\tty7""#); + // A trailing backslash must not escape the closing quote. + assert_eq!(quoted(r"C:\Program Files\"), r#""C:\Program Files\\""#); + // A literal quote takes one escape, and the run before it doubles. + assert_eq!(quoted(r#"a"b"#), r#""a\"b""#); + assert_eq!(quoted(r#"a\"b"#), r#""a\\\"b""#); + } +} diff --git a/crates/tty7-server/tests/client_lib.rs b/crates/tty7-server/tests/client_lib.rs index 4ab3256d..26896704 100644 --- a/crates/tty7-server/tests/client_lib.rs +++ b/crates/tty7-server/tests/client_lib.rs @@ -354,7 +354,11 @@ fn input_reaches_the_shell_and_a_reattach_replays_it() { "the refusal was {refused}" ); - reattached.kill().expect("kill the pane"); + // This test covers attach replay rather than the streaming connection's + // Kill command. Close that connection explicitly, then use the one-shot + // PaneClient path for deterministic cleanup on loaded Windows runners. + drop(reattached); + panes.kill(pane_id).expect("kill the pane"); let deadline = Instant::now() + STREAM_WITHIN; loop { let listed = panes.list().expect("list panes after the kill");