diff --git a/Cargo.lock b/Cargo.lock index d479c5cd..4bb35423 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2131,7 +2131,6 @@ dependencies = [ "tokio", "tokio-util", "uuid", - "which", "windows", ] @@ -3751,15 +3750,6 @@ dependencies = [ "wasm-bindgen", ] -[[package]] -name = "which" -version = "8.0.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8f3ef584124b911bcc3875c2f1472e80f24361ceb789bd1c62b3e9a3df9ff43c" -dependencies = [ - "libc", -] - [[package]] name = "winapi-util" version = "0.1.11" diff --git a/crates/openjd-cli/tests/cli_tests.rs b/crates/openjd-cli/tests/cli_tests.rs index 8a187f31..8edec751 100644 --- a/crates/openjd-cli/tests/cli_tests.rs +++ b/crates/openjd-cli/tests/cli_tests.rs @@ -54,7 +54,14 @@ fn python_shim_dir() -> Option { // If the interpreter is literally named `python` we don't need a shim — // its parent directory is already on `PATH` (that's how `which` found // it, transitively). Detect this by comparing the file-name component. - if target.file_name().and_then(|n| n.to_str()) == Some("python") { + let fname = target.file_name().and_then(|n| n.to_str()).unwrap_or(""); + // On Windows, `python.exe` is the canonical name. + let is_canonical = if cfg!(windows) { + fname.eq_ignore_ascii_case("python.exe") + } else { + fname == "python" + }; + if is_canonical { return None; } let shim_dir = @@ -65,7 +72,7 @@ fn python_shim_dir() -> Option { std::fs::create_dir_all(&shim_dir) .unwrap_or_else(|e| panic!("Failed to create shim_dir {shim_dir:?}: {e}")); let shim_path = shim_dir.join(if cfg!(windows) { - "python.cmd" + "python.exe" } else { "python" }); @@ -89,11 +96,16 @@ fn python_shim_dir() -> Option { } #[cfg(windows)] { - // On Windows a .cmd wrapper works for anything spawned via - // CreateProcess with a bare name lookup. - let script = format!("@echo off\r\n{} %*\r\n", target.display()); - std::fs::write(&shim_path, script) - .unwrap_or_else(|e| panic!("Failed to write python shim {shim_path:?}: {e}")); + // On Windows, hard-link the real interpreter as `python.exe` so + // it resolves correctly under PATHEXT semantics without the + // argument-mangling issues of a .cmd/.bat wrapper. Hard link + // avoids a ~5 MB copy and works as long as source and dest are on + // the same volume (both are under %TEMP% / %LOCALAPPDATA%). + std::fs::hard_link(&target, &shim_path).unwrap_or_else(|_| { + std::fs::copy(&target, &shim_path).unwrap_or_else(|e| { + panic!("Failed to copy python interpreter to {shim_path:?}: {e}") + }); + }); } Some(shim_dir) }); diff --git a/crates/openjd-sessions/Cargo.toml b/crates/openjd-sessions/Cargo.toml index 3adea5dc..61fd0617 100644 --- a/crates/openjd-sessions/Cargo.toml +++ b/crates/openjd-sessions/Cargo.toml @@ -45,7 +45,6 @@ futures-util = "0.3.32" nix = { version = "0.31", features = ["signal", "process", "user", "fs"] } [target.'cfg(windows)'.dependencies] -which = "8.0.2" windows = { version = "0.62", features = [ "Win32_Foundation", "Win32_Security", diff --git a/crates/openjd-sessions/src/cross_user_helper.rs b/crates/openjd-sessions/src/cross_user_helper.rs index 604281a3..c4b9b959 100644 --- a/crates/openjd-sessions/src/cross_user_helper.rs +++ b/crates/openjd-sessions/src/cross_user_helper.rs @@ -561,6 +561,15 @@ pub(crate) async fn run_via_helper( message_tx: tokio::sync::mpsc::UnboundedSender, cancel_writer: Option<&std::fs::File>, ) -> Result { + // Windows note: executable resolution happens inside the helper + // (helper/src/runner_win.rs::locate_executable), which runs as the + // target user — it can resolve executables in directories only that + // user can read, and its PATH fallback is the target user's own + // environment (matching what the workload actually runs under). + // Resolution failures come back over the protocol as an error response + // and are mapped to SessionError::SubprocessStart below. + let args = &config.args; + // Build the env map (only set values; unsets are excluded). let env: serde_json::Map = config .env_vars @@ -572,8 +581,8 @@ pub(crate) async fn run_via_helper( .collect(); let cmd = serde_json::json!({ - "command": config.args[0], - "args": &config.args[1..], + "command": args[0], + "args": &args[1..], "env": env, "cwd": config.working_dir, }); @@ -586,7 +595,7 @@ pub(crate) async fn run_via_helper( session_id, LogContent::FILE_PATH | LogContent::PROCESS_CONTROL, "Running command {}", - crate::subprocess::format_command_for_log(&config.args) + crate::subprocess::format_command_for_log(args) ); // Timeout as async future instead of OS thread @@ -682,7 +691,7 @@ pub(crate) async fn run_via_helper( if let Some(msg) = resp.get("error").and_then(|v| v.as_str()) { return Err(SessionError::SubprocessStart { - command: config.args[0].clone(), + command: args[0].clone(), source: std::io::Error::other(msg.to_string()), }); } diff --git a/crates/openjd-sessions/src/helper/src/main.rs b/crates/openjd-sessions/src/helper/src/main.rs index 821ac83c..862dde1b 100644 --- a/crates/openjd-sessions/src/helper/src/main.rs +++ b/crates/openjd-sessions/src/helper/src/main.rs @@ -9,6 +9,8 @@ mod protocol; mod runner; #[cfg(windows)] mod runner_win; +#[cfg(windows)] +mod win32_which; use protocol::{constant_time_eq, send, Command, Response, AUTH_TOKEN_LEN}; use std::io::{BufRead, Read}; diff --git a/crates/openjd-sessions/src/helper/src/runner_win.rs b/crates/openjd-sessions/src/helper/src/runner_win.rs index ffe210b8..5313f8bd 100644 --- a/crates/openjd-sessions/src/helper/src/runner_win.rs +++ b/crates/openjd-sessions/src/helper/src/runner_win.rs @@ -40,7 +40,9 @@ pub fn run_command( use windows::Win32::Foundation::HANDLE; const CREATE_NEW_PROCESS_GROUP: u32 = 0x00000200; - let mut child = Command::new(&cmd.command) + let command = locate_executable(&cmd.command, &cmd.env, &cmd.cwd)?; + + let mut child = Command::new(&command) .args(&cmd.args) .envs(&cmd.env) .current_dir(&cmd.cwd) @@ -184,6 +186,52 @@ fn handle_cancel(child_pid: u32, method: &CancelMethod) -> Option, + cwd: &str, +) -> Result { + if std::path::Path::new(command).is_absolute() { + return Ok(command.to_string()); + } + let env_var = |name: &str| -> Option { + env.iter() + .find(|(k, _)| k.eq_ignore_ascii_case(name)) + .map(|(_, v)| v.clone()) + }; + let path_var = env_var("PATH").unwrap_or_else(|| std::env::var("PATH").unwrap_or_default()); + let pathext = + env_var("PATHEXT").unwrap_or_else(|| std::env::var("PATHEXT").unwrap_or_default()); + let search_path = format!("{cwd};{path_var}"); + match crate::win32_which::locate_in(command, &search_path, &pathext, std::path::Path::new(cwd)) + { + Some(found) => Ok(found.to_string_lossy().into_owned()), + None => Err(format!("Could not find executable file: {command}")), + } +} + /// Send CTRL_BREAK_EVENT to a process. fn send_ctrl_break(pid: u32) -> bool { use windows::Win32::System::Console::{GenerateConsoleCtrlEvent, CTRL_BREAK_EVENT}; diff --git a/crates/openjd-sessions/src/helper/src/win32_which.rs b/crates/openjd-sessions/src/helper/src/win32_which.rs new file mode 100644 index 00000000..09d7b4ff --- /dev/null +++ b/crates/openjd-sessions/src/helper/src/win32_which.rs @@ -0,0 +1,224 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright by contributors to this project. +// SPDX-License-Identifier: (Apache-2.0 OR MIT) + +//! Windows command search mirroring Python's `shutil.which`. +//! +//! This file is shared by both Windows spawn paths so their search +//! semantics are identical by construction: the session crate includes it +//! via `#[path]` from `win32_locate.rs` (same-user actions), and the +//! embedded helper compiles it as a module (cross-user actions). +//! +//! It exists instead of the `which` crate because resolution here must +//! honor the *action's* PATHEXT, not the resolving process's: `which` +//! always reads PATHEXT from the process environment, and it accepts any +//! existing file when the command has an explicit extension — whereas +//! `shutil.which` (and cmd.exe) treat an extension outside PATHEXT as +//! not-runnable and report not-found. + +use std::path::{Path, PathBuf}; + +/// Default PATHEXT when none is available, matching Python +/// `shutil._WIN_DEFAULT_PATHEXT`. +pub const DEFAULT_PATHEXT: &str = ".COM;.EXE;.BAT;.CMD;.VBS;.JS;.WS;.MSC"; + +/// Search for `command` with `shutil.which` semantics. +/// +/// - `search_path` is a `;`-separated directory list, searched in order; +/// within each directory every candidate extension is tried before +/// moving to the next directory (earliest directory wins). +/// - `pathext` is a `;`-separated extension list. An empty or blank value +/// selects [`DEFAULT_PATHEXT`] (mirroring `shutil.which`, and cmd.exe's +/// own treatment of an unset PATHEXT). +/// - If `command` already ends with one of the PATHEXT extensions +/// (case-insensitive), it is looked up as-is; otherwise each extension +/// is appended in PATHEXT order. A command whose explicit extension is +/// *not* in PATHEXT is therefore never matched by its literal name — +/// `script.ps1` with a default PATHEXT returns `None`, exactly like +/// `shutil.which`. +/// - A command containing a path separator is resolved against `cwd` +/// only (no PATH search), with the same extension rules. +pub fn locate_in(command: &str, search_path: &str, pathext: &str, cwd: &Path) -> Option { + let pathext = if pathext.trim().is_empty() { + DEFAULT_PATHEXT + } else { + pathext + }; + let exts: Vec<&str> = pathext.split(';').filter(|e| !e.is_empty()).collect(); + let cmd_lower = command.to_lowercase(); + let has_listed_ext = exts.iter().any(|e| cmd_lower.ends_with(&e.to_lowercase())); + let candidates: Vec = if has_listed_ext { + vec![command.to_string()] + } else { + exts.iter().map(|e| format!("{command}{e}")).collect() + }; + + if command.contains('\\') || command.contains('/') { + for cand in &candidates { + let p = cwd.join(cand); + if p.is_file() { + return Some(p); + } + } + return None; + } + + for dir in search_path.split(';').filter(|d| !d.is_empty()) { + let dir = Path::new(dir); + for cand in &candidates { + let p = dir.join(cand); + if p.is_file() { + return Some(p); + } + } + } + None +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Compare two optional paths case-insensitively: the search returns + /// the candidate's PATHEXT casing (e.g. `tool.BAT` for an on-disk + /// `tool.bat`), exactly like Python's shutil.which; NTFS resolves it + /// case-insensitively. + fn assert_path_eq(actual: Option, expected: Option, msg: &str) { + let norm = |p: Option| p.map(|p| p.to_string_lossy().to_lowercase()); + assert_eq!(norm(actual), norm(expected), "{msg}"); + } + + fn touch(path: &Path) { + std::fs::write(path, "").unwrap(); + } + + fn tempdir() -> std::path::PathBuf { + // No tempfile dependency in the helper crate: use a unique dir under + // the OS temp dir keyed by test name via a counter + PID. + use std::sync::atomic::{AtomicU64, Ordering}; + static N: AtomicU64 = AtomicU64::new(0); + let d = std::env::temp_dir().join(format!( + "openjd-win32-which-test-{}-{}", + std::process::id(), + N.fetch_add(1, Ordering::Relaxed) + )); + std::fs::create_dir_all(&d).unwrap(); + d + } + + /// PATHEXT is honored per directory: earliest directory wins across + /// all extensions. + #[test] + fn earliest_directory_wins_across_extensions() { + let root = tempdir(); + let (a, b) = (root.join("a"), root.join("b")); + std::fs::create_dir_all(&a).unwrap(); + std::fs::create_dir_all(&b).unwrap(); + touch(&a.join("tool.bat")); + touch(&b.join("tool.exe")); + let sp = format!("{};{}", a.display(), b.display()); + let found = locate_in("tool", &sp, ".EXE;.BAT", &root); + assert_path_eq(found, Some(a.join("tool.bat")), "earliest dir wins"); + } + + /// The caller's PATHEXT restricts candidates: with PATHEXT=.EXE a + /// `.bat` earlier in PATH is NOT runnable and the later `.exe` wins. + /// (The action's PATHEXT must be honored, not the resolving + /// process's.) + #[test] + fn action_pathext_restricts_candidates() { + let root = tempdir(); + let (a, b) = (root.join("a"), root.join("b")); + std::fs::create_dir_all(&a).unwrap(); + std::fs::create_dir_all(&b).unwrap(); + touch(&a.join("tool.bat")); + touch(&b.join("tool.exe")); + let sp = format!("{};{}", a.display(), b.display()); + let found = locate_in("tool", &sp, ".EXE", &root); + assert_path_eq( + found, + Some(b.join("tool.exe")), + "PATHEXT=.EXE excludes .bat", + ); + assert_eq!( + locate_in("tool", &a.to_string_lossy(), ".EXE", &root), + None, + "a .bat-only match is not runnable under PATHEXT=.EXE" + ); + } + + /// An explicit extension outside PATHEXT is not runnable — not-found, + /// matching Python's shutil.which (and cmd.exe). The file existing is + /// not sufficient. + #[test] + fn explicit_extension_outside_pathext_is_not_found() { + let root = tempdir(); + touch(&root.join("script.ps1")); + let sp = root.to_string_lossy().into_owned(); + assert_eq!( + locate_in("script.ps1", &sp, DEFAULT_PATHEXT, &root), + None, + ".ps1 is not in the default PATHEXT" + ); + // ...but IS runnable when the action's PATHEXT includes .PS1. + assert_path_eq( + locate_in("script.ps1", &sp, ".EXE;.PS1", &root), + Some(root.join("script.ps1")), + "explicit .ps1 runnable when PATHEXT lists it", + ); + } + + /// A command with a listed explicit extension is looked up as-is; + /// extension matching is case-insensitive. + #[test] + fn explicit_listed_extension_matches_case_insensitively() { + let root = tempdir(); + touch(&root.join("tool.BAT")); + let sp = root.to_string_lossy().into_owned(); + assert_path_eq( + locate_in("tool.bat", &sp, ".COM;.EXE;.BAT", &root), + Some(root.join("tool.bat")), + "explicit .bat is in PATHEXT and matches on disk case-insensitively", + ); + } + + /// Empty/blank PATHEXT selects the shutil.which default list. + #[test] + fn empty_pathext_uses_default() { + let root = tempdir(); + touch(&root.join("tool.exe")); + let sp = root.to_string_lossy().into_owned(); + assert_path_eq( + locate_in("tool", &sp, "", &root), + Some(root.join("tool.exe")), + "default PATHEXT finds .exe", + ); + assert_eq!( + locate_in("script.ps1", &sp, "", &root), + None, + ".ps1 is outside the default PATHEXT" + ); + } + + /// A command containing a path separator resolves against cwd only — + /// PATH directories are not searched. + #[test] + fn relative_path_resolves_against_cwd_only() { + let root = tempdir(); + let (sub, elsewhere) = (root.join("sub"), root.join("elsewhere")); + std::fs::create_dir_all(&sub).unwrap(); + std::fs::create_dir_all(&elsewhere).unwrap(); + touch(&sub.join("tool.bat")); + touch(&elsewhere.join("sub-tool.bat")); + assert_path_eq( + locate_in(r"sub\tool", &elsewhere.to_string_lossy(), ".BAT", &root), + Some(root.join(r"sub\tool.bat")), + "pathy command resolves against cwd", + ); + assert_eq!( + locate_in(r"missing\tool", &elsewhere.to_string_lossy(), ".BAT", &root), + None, + "a pathy command is never searched on PATH" + ); + } +} diff --git a/crates/openjd-sessions/src/subprocess.rs b/crates/openjd-sessions/src/subprocess.rs index 260196b3..79be9729 100644 --- a/crates/openjd-sessions/src/subprocess.rs +++ b/crates/openjd-sessions/src/subprocess.rs @@ -479,6 +479,34 @@ pub async fn run_subprocess( )); } + // Windows: resolve the command to an absolute path with canonical + // search semantics (PATHEXT-aware, working_dir first, action PATH) so + // the spawn below cannot fall back to CreateProcessW's legacy search. + // See win32_locate.rs. Mirrors Python's locate_windows_executable call + // in _runner_base.py. + #[cfg(windows)] + let args = &{ + let wd = config + .working_dir + .clone() + .unwrap_or_else(|| std::path::PathBuf::from(".")); + crate::win32_locate::locate_windows_executable(args, Some(&config.env_vars), &wd).map_err( + |msg| { + session_log!( + info, + session_id, + LogContent::EXCEPTION_INFO | LogContent::PROCESS_CONTROL, + "{}", + msg + ); + SessionError::SubprocessStart { + command: args[0].clone(), + source: std::io::Error::new(std::io::ErrorKind::NotFound, msg), + } + }, + )? + }; + // Build merged environment let mut merged: HashMap = std::env::vars().collect(); for (k, v) in &config.env_vars { diff --git a/crates/openjd-sessions/src/win32_locate.rs b/crates/openjd-sessions/src/win32_locate.rs index 7a4d1afb..21b928ed 100644 --- a/crates/openjd-sessions/src/win32_locate.rs +++ b/crates/openjd-sessions/src/win32_locate.rs @@ -3,53 +3,283 @@ // SPDX-License-Identifier: (Apache-2.0 OR MIT) //! Windows executable resolution — mirrors Python `_win32/_locate_executable.py`. +//! +//! Rust's `std::process::Command` alone is not a faithful Windows command +//! search: it only appends `.exe` to bare names (never consulting PATHEXT, +//! so a `.bat` in an earlier PATH directory loses to an `.exe` in a later +//! one), and when the child environment's PATH has no match it falls back +//! to `CreateProcessW`'s legacy search (application directory, system +//! directories, the parent process's PATH). Resolving the command to an +//! absolute path here, before spawn, gives canonical Windows semantics and +//! makes "not on the action's PATH" a hard error instead of a silent +//! resolution through the worker's own environment. +use std::collections::HashMap; use std::path::Path; -use crate::session_user::SessionUser; +// The search itself lives in the helper's source tree and is compiled into +// both binaries (see the module docs in the file). `#[path]` inclusion — +// not a shared dependency crate — because the helper is a standalone +// nested Cargo project that cannot depend on this crate. +#[path = "helper/src/win32_which.rs"] +mod win32_which; /// Resolve the executable in `args[0]` for Windows, returning updated args. /// -/// - Absolute paths are returned as-is (OS resolves extensions). -/// - Relative names are resolved via `which` using the provided PATH + working_dir. -/// - Cross-user resolution falls back to same-user lookup (the executable must be -/// accessible to both users). -#[allow(dead_code)] -pub fn locate_windows_executable( +/// - Absolute paths are returned as-is (the OS resolves extensions). +/// - Other commands are resolved with `shutil.which` semantics (PATHEXT +/// tried per directory, earliest PATH directory wins; an explicit +/// extension outside PATHEXT is not runnable and reports not-found) +/// over a search path of `{working_dir};{PATH}`, so executables in the +/// session working directory take precedence. +/// - PATH and PATHEXT each come from exactly one source, chosen before +/// searching: a key in `os_env_vars` (case-insensitive) is used +/// exclusively — the process environment is never consulted then, even +/// if the search finds nothing. An explicit unset (`Some(None)`, which +/// the spawn merge turns into a removed variable) counts as present: +/// PATH resolves as empty (working directory only) and PATHEXT as the +/// default extension list. Only when `os_env_vars` defines no such key +/// at all is the process environment's value used, matching the +/// environment merge applied at spawn (an action-supplied value +/// overwrites the inherited one; an unset removes it). Note: Python's +/// `_get_path_var_for_shutil_which` conflates unset with absent and +/// falls back to the process PATH; we intentionally diverge so +/// resolution matches what the spawned child actually sees. +/// - Resolution failure is an `Err` with the Python-parity message +/// `Could not find executable file: `. +/// +/// This function serves the **same-user** spawn path (`run_subprocess`). +/// Cross-user actions are resolved inside the embedded helper instead +/// (`helper/src/runner_win.rs::locate_executable`, same search — both +/// compile the shared `win32_which.rs`) — the helper runs as the target +/// user, so it can probe directories only that user can read, and its +/// PATH/PATHEXT fallback is the target user's environment rather than +/// the service user's. +pub(crate) fn locate_windows_executable( args: &[String], - _user: Option<&dyn SessionUser>, - os_env_vars: Option<&std::collections::HashMap>>, - working_dir: &str, -) -> Vec { - let mut result = args.to_vec(); + os_env_vars: Option<&HashMap>>, + working_dir: &Path, +) -> Result, String> { let cmd = Path::new(&args[0]); - // Absolute paths: leave as-is + // Absolute paths: leave as-is (the OS resolves the extension). if cmd.is_absolute() { - return result; + return Ok(args.to_vec()); } - // Build PATH with working_dir prepended - let path_var = os_env_vars - .and_then(|env| { + // Three states per key, and they must not be conflated: absent (outer + // None → the child inherits the process value, so resolution uses it), + // set (Some(Some(v)) → used exclusively), and explicitly unset + // (Some(None) → the spawn merge removes the variable from the child, so + // resolution must treat it as empty — falling back to the process value + // here would resolve against a PATH the action deliberately dropped). + let env_var = |name: &str| -> Option> { + os_env_vars.and_then(|env| { env.iter() - .find(|(k, _)| k.eq_ignore_ascii_case("path")) - .and_then(|(_, v)| v.as_ref().map(|s| s.as_str())) + .find(|(k, _)| k.eq_ignore_ascii_case(name)) + .map(|(_, v)| v.clone()) }) - .or_else(|| std::env::var("PATH").ok().as_deref().map(|_| "")) - .unwrap_or(""); - - let search_path = format!("{};{}", working_dir, path_var); + }; + let path_var = match env_var("PATH") { + Some(set_or_unset) => set_or_unset.unwrap_or_default(), + None => std::env::var("PATH").unwrap_or_default(), + }; + // An explicitly unset PATHEXT maps to "" which selects the default + // extension list — matching how cmd.exe and shutil.which behave in a + // child that has no PATHEXT variable. + let pathext = match env_var("PATHEXT") { + Some(set_or_unset) => set_or_unset.unwrap_or_default(), + None => std::env::var("PATHEXT").unwrap_or_default(), + }; + let search_path = format!("{};{}", working_dir.display(), path_var); - // Use which crate with custom path - match which::which_in(&args[0], Some(&search_path), working_dir) { - Ok(found) => { - result[0] = found.to_string_lossy().to_string(); - } - Err(_) => { - // Leave as-is; let the OS fail naturally + match win32_which::locate_in(&args[0], &search_path, &pathext, working_dir) { + Some(found) => { + let mut result = args.to_vec(); + result[0] = found.to_string_lossy().into_owned(); + Ok(result) } + None => Err(format!("Could not find executable file: {}", args[0])), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn touch(path: &Path) { + std::fs::write(path, "").unwrap(); + } + + fn env_with_path(dirs: &[&Path]) -> HashMap> { + let joined = dirs + .iter() + .map(|d| d.to_string_lossy().into_owned()) + .collect::>() + .join(";"); + // Mixed-case key: the lookup must be case-insensitive. + HashMap::from([("Path".to_string(), Some(joined))]) + } + + fn args(cmd: &str) -> Vec { + vec![cmd.to_string(), "arg1".to_string()] + } + + /// A `.bat` in an earlier PATH directory wins over an `.exe` in a later + /// one — canonical Windows search order (PATHEXT tried per directory). + #[test] + fn bat_earlier_in_path_beats_exe_later() { + let tmp = tempfile::TempDir::new().unwrap(); + let dir_a = tmp.path().join("a"); + let dir_b = tmp.path().join("b"); + std::fs::create_dir_all(&dir_a).unwrap(); + std::fs::create_dir_all(&dir_b).unwrap(); + touch(&dir_a.join("tool.bat")); + touch(&dir_b.join("tool.exe")); + let wd = tmp.path(); + + let resolved = + locate_windows_executable(&args("tool"), Some(&env_with_path(&[&dir_a, &dir_b])), wd) + .unwrap(); + assert_eq!( + resolved[0].to_lowercase(), + dir_a.join("tool.bat").to_string_lossy().to_lowercase(), + "earliest PATH directory must win across extensions" + ); + assert_eq!(resolved[1], "arg1", "remaining args are preserved"); + } + + /// A bare name whose only match is a `.bat` resolves (std's `.exe`-only + /// probing would miss it). + #[test] + fn bare_name_resolves_bat_only_match() { + let tmp = tempfile::TempDir::new().unwrap(); + let dir_a = tmp.path().join("a"); + std::fs::create_dir_all(&dir_a).unwrap(); + touch(&dir_a.join("onlybat.bat")); + + let resolved = locate_windows_executable( + &args("onlybat"), + Some(&env_with_path(&[&dir_a])), + tmp.path(), + ) + .unwrap(); + assert_eq!( + resolved[0].to_lowercase(), + dir_a.join("onlybat.bat").to_string_lossy().to_lowercase() + ); + } + + /// The working directory is searched first, before any PATH entry. + #[test] + fn working_dir_wins_over_path() { + let tmp = tempfile::TempDir::new().unwrap(); + let dir_a = tmp.path().join("a"); + let wd = tmp.path().join("wd"); + std::fs::create_dir_all(&dir_a).unwrap(); + std::fs::create_dir_all(&wd).unwrap(); + touch(&dir_a.join("dup.bat")); + touch(&wd.join("dup.bat")); + + let resolved = + locate_windows_executable(&args("dup"), Some(&env_with_path(&[&dir_a])), &wd).unwrap(); + assert_eq!( + resolved[0].to_lowercase(), + wd.join("dup.bat").to_string_lossy().to_lowercase() + ); + } + + /// A command absent from the search path is a hard error with the + /// Python-parity message — no fallback to the process's own PATH for + /// bare names. `whoami` exists on the real PATH, so success here would + /// prove a fallback leak. + #[test] + fn absent_command_is_error_not_process_path_fallback() { + let tmp = tempfile::TempDir::new().unwrap(); + let empty = tmp.path().join("empty"); + std::fs::create_dir_all(&empty).unwrap(); + + let err = + locate_windows_executable(&args("whoami"), Some(&env_with_path(&[&empty])), &empty) + .unwrap_err(); + assert_eq!(err, "Could not find executable file: whoami"); + } + + /// Absolute paths pass through untouched. + #[test] + fn absolute_path_passthrough() { + let a = args(r"C:\Windows\System32\whoami.exe"); + let resolved = locate_windows_executable(&a, None, Path::new(".")).unwrap(); + assert_eq!(resolved, a); + } + + /// An explicitly unset PATH (`Some(None)` — the spawn merge removes + /// the variable from the child) must resolve as an empty PATH, not + /// fall back to the process environment: the process PATH points at + /// directories the action deliberately dropped. `whoami` is on the + /// real PATH, so success here would prove the leak. + #[test] + fn explicitly_unset_path_does_not_fall_back_to_process_path() { + let env: HashMap> = HashMap::from([("PATH".to_string(), None)]); + let err = + locate_windows_executable(&args("whoami"), Some(&env), Path::new(".")).unwrap_err(); + assert_eq!(err, "Could not find executable file: whoami"); + } + + /// With PATH explicitly unset, the working directory is still + /// searched — an unset PATH means "working dir only", not "nothing". + #[test] + fn explicitly_unset_path_still_searches_working_dir() { + let tmp = tempfile::TempDir::new().unwrap(); + touch(&tmp.path().join("wdonly.bat")); + let env: HashMap> = HashMap::from([("PATH".to_string(), None)]); + let resolved = locate_windows_executable(&args("wdonly"), Some(&env), tmp.path()).unwrap(); + assert_eq!( + resolved[0].to_lowercase(), + tmp.path() + .join("wdonly.bat") + .to_string_lossy() + .to_lowercase() + ); + } + + /// An explicitly unset PATHEXT selects the default extension list + /// (like a child with no PATHEXT variable), not the process's + /// PATHEXT. A `.ps1` must stay not-runnable even if the process + /// PATHEXT were to include .PS1. + #[test] + fn explicitly_unset_pathext_uses_default_list() { + let tmp = tempfile::TempDir::new().unwrap(); + touch(&tmp.path().join("tool.exe")); + std::fs::write(tmp.path().join("script.ps1"), "").unwrap(); + let env: HashMap> = HashMap::from([ + ( + "PATH".to_string(), + Some(tmp.path().to_string_lossy().into_owned()), + ), + ("PATHEXT".to_string(), None), + ]); + // Default list finds the .exe... + let resolved = locate_windows_executable(&args("tool"), Some(&env), tmp.path()).unwrap(); + assert!(resolved[0].to_lowercase().ends_with("tool.exe")); + // ...and excludes the .ps1. + let err = + locate_windows_executable(&args("script.ps1"), Some(&env), tmp.path()).unwrap_err(); + assert_eq!(err, "Could not find executable file: script.ps1"); } - result + /// Without action env vars (or without PATH in them), resolution falls + /// back to the process environment's PATH. + #[test] + fn falls_back_to_process_path() { + // `whoami` is on every Windows system PATH via System32. + let resolved = + locate_windows_executable(&args("whoami"), Some(&HashMap::new()), Path::new(".")) + .unwrap(); + assert!( + resolved[0].to_lowercase().ends_with("whoami.exe"), + "expected whoami.exe from process PATH; got {}", + resolved[0] + ); + } } diff --git a/crates/openjd-sessions/tests/integration.rs b/crates/openjd-sessions/tests/integration.rs index 42241ae5..c0849a00 100644 --- a/crates/openjd-sessions/tests/integration.rs +++ b/crates/openjd-sessions/tests/integration.rs @@ -34,6 +34,8 @@ mod test_session_env_step; mod test_session_scenarios; #[path = "integration/test_tempdir_os.rs"] mod test_tempdir_os; +#[path = "integration/test_win32_locate.rs"] +mod test_win32_locate; #[path = "integration/test_windows_permissions.rs"] mod test_windows_permissions; #[path = "integration/test_wrap_actions.rs"] diff --git a/crates/openjd-sessions/tests/integration/test_cross_user_windows.rs b/crates/openjd-sessions/tests/integration/test_cross_user_windows.rs index f194df0b..c5273f67 100644 --- a/crates/openjd-sessions/tests/integration/test_cross_user_windows.rs +++ b/crates/openjd-sessions/tests/integration/test_cross_user_windows.rs @@ -571,6 +571,104 @@ async fn test_cross_user_session_run_subprocess() { session.cleanup(); } +// === Session-level: executable resolution through the helper === + +/// A `.bat` materialized into the session working directory is runnable by +/// bare name through the cross-user helper — the helper searches the cwd +/// first, then the action's PATH (see runner_win.rs::locate_executable). +#[tokio::test(flavor = "multi_thread")] +#[ignore] +async fn test_cross_user_session_bat_in_working_dir_resolves() { + let user = require_windows_user(); + let mut session = make_session(user); + std::fs::write( + session.working_directory().join("wdtool.bat"), + "@echo off\r\necho CROSS-USER-WD-BAT\r\n", + ) + .unwrap(); + + let r = session + .run_subprocess("wdtool", None, None, None, true, None) + .await + .unwrap(); + assert_eq!(r.state, ActionState::Success); + assert!( + r.stdout.contains("CROSS-USER-WD-BAT"), + "expected the working-directory .bat to resolve via the helper; stdout: {}", + r.stdout + ); + session.cleanup(); +} + +/// A command that is not on the action's PATH fails with the Python-parity +/// error, surfaced from the helper over the protocol — no fallback to the +/// helper's own PATH or CreateProcessW's legacy search. +#[tokio::test(flavor = "multi_thread")] +#[ignore] +async fn test_cross_user_session_absent_command_fails() { + let user = require_windows_user(); + let mut session = make_session(user); + let empty = session.working_directory().join("empty"); + std::fs::create_dir_all(&empty).unwrap(); + + let env = HashMap::from([("PATH".to_string(), empty.to_string_lossy().into_owned())]); + let err = session + .run_subprocess("whoami", None, None, Some(&env), false, None) + .await + .expect_err("whoami is not on the action's PATH and must not resolve via fallback"); + assert!( + err.to_string() + .contains("Could not find executable file: whoami"), + "expected Python-parity not-found error; got: {err}" + ); + session.cleanup(); +} + +/// Resolution runs as the target user: an executable in a directory the +/// target user can read resolves even when that directory's DACL is +/// protected (no inherited ACEs). Host-side resolution as the service user +/// would be the wrong vantage point; helper-side resolution is what makes +/// this correct by construction. +#[tokio::test(flavor = "multi_thread")] +#[ignore] +async fn test_cross_user_session_resolves_in_user_readable_dir() { + let user = require_windows_user(); + let user_name = windows_user_name(); + let proc_user = process_user_bare(); + let mut session = make_session(user); + + // A tool directory outside the working dir, readable by the target + // user (and the process user, so the test can create/clean it). + let tool_dir = test_session_root().join("tools"); + std::fs::create_dir_all(&tool_dir).unwrap(); + openjd_sessions::win32_permissions::set_permissions( + tool_dir.to_str().unwrap(), + &[&proc_user], + &[], + &[&user_name], + ) + .unwrap(); + std::fs::write( + tool_dir.join("usertool.bat"), + "@echo off\r\necho USER-DIR-TOOL\r\n", + ) + .unwrap(); + + let env = HashMap::from([("PATH".to_string(), tool_dir.to_string_lossy().into_owned())]); + let r = session + .run_subprocess("usertool", None, None, Some(&env), false, None) + .await + .unwrap(); + assert_eq!(r.state, ActionState::Success); + assert!( + r.stdout.contains("USER-DIR-TOOL"), + "expected the tool in the user-readable dir to resolve; stdout: {}", + r.stdout + ); + session.cleanup(); + let _ = std::fs::remove_dir_all(&tool_dir); +} + // === Session-level: SessionCancelHandle cancels a helper-routed subprocess === /// The session working directory must carry an explicit DACL granting the diff --git a/crates/openjd-sessions/tests/integration/test_helper.rs b/crates/openjd-sessions/tests/integration/test_helper.rs index 5256eeae..90a8ee76 100644 --- a/crates/openjd-sessions/tests/integration/test_helper.rs +++ b/crates/openjd-sessions/tests/integration/test_helper.rs @@ -494,6 +494,228 @@ fn test_helper_crash_during_execution_windows() { } } +// ──────────────────────────────────────────────────────────────────── +// Windows executable resolution inside the helper (runner_win.rs:: +// locate_executable). Same-user spawn of the helper binary; mirrors the +// host-side test_win32_locate.rs suite for the cross-user spawn path. +// ──────────────────────────────────────────────────────────────────── + +/// Build a run-command JSON with an explicit PATH env var. +#[cfg(windows)] +fn run_with_path_json(command: &str, path_value: &str, cwd: &str) -> String { + format!( + r#"{{"token": "{TEST_TOKEN}", "command": "{}", "args": [], "env": {{"PATH": "{}"}}, "cwd": "{}"}}"#, + command, + path_value.replace('\\', "\\\\"), + cwd.replace('\\', "\\\\") + ) +} + +/// Write a `.bat` that prints `marker` (CRLF line endings). +#[cfg(windows)] +fn write_bat(path: &std::path::Path, marker: &str) { + std::fs::write(path, format!("@echo off\r\necho {marker}\r\n")).unwrap(); +} + +/// A `.bat` in an earlier PATH directory must beat an `.exe` in a later +/// one — the helper resolves PATHEXT-aware before spawning. +#[cfg(windows)] +#[test] +fn test_helper_windows_bat_earlier_in_path_beats_exe_later() { + let tmp = tempfile::TempDir::new().unwrap(); + let dir_a = tmp.path().join("dirA"); + let dir_b = tmp.path().join("dirB"); + std::fs::create_dir_all(&dir_a).unwrap(); + std::fs::create_dir_all(&dir_b).unwrap(); + write_bat(&dir_a.join("pick.bat"), "HELPER-BAT-A"); + let system32 = PathBuf::from(std::env::var("SYSTEMROOT").unwrap()).join("System32"); + std::fs::copy(system32.join("whoami.exe"), dir_b.join("pick.exe")).unwrap(); + + let path_value = format!("{};{}", dir_a.display(), dir_b.display()); + let mut h = Helper::spawn(); + h.send(&run_with_path_json( + "pick", + &path_value, + &tmp.path().to_string_lossy(), + )); + let resp = h.read_until_done(); + assert!( + resp.iter() + .any(|v| v.get("out").and_then(|o| o.as_str()) == Some("HELPER-BAT-A")), + "expected the .bat in the earlier PATH directory to win; responses: {resp:?}" + ); + h.shutdown(); +} + +/// The working directory (the session dir, in production) is searched +/// first, before PATH. +#[cfg(windows)] +#[test] +fn test_helper_windows_cwd_searched_first() { + let tmp = tempfile::TempDir::new().unwrap(); + let dir_a = tmp.path().join("dirA"); + let cwd = tmp.path().join("cwd"); + std::fs::create_dir_all(&dir_a).unwrap(); + std::fs::create_dir_all(&cwd).unwrap(); + write_bat(&dir_a.join("dup.bat"), "FROM-PATH"); + write_bat(&cwd.join("dup.bat"), "FROM-CWD"); + + let mut h = Helper::spawn(); + h.send(&run_with_path_json( + "dup", + &dir_a.to_string_lossy(), + &cwd.to_string_lossy(), + )); + let resp = h.read_until_done(); + assert!( + resp.iter() + .any(|v| v.get("out").and_then(|o| o.as_str()) == Some("FROM-CWD")), + "expected the cwd match to win over PATH; responses: {resp:?}" + ); + h.shutdown(); +} + +/// A command absent from the run's PATH is a protocol error with the +/// Python-parity message — the helper must not fall back to its own PATH +/// or CreateProcessW's legacy search. `whoami` exists in System32, so +/// success here would prove a fallback leak. +#[cfg(windows)] +#[test] +fn test_helper_windows_absent_command_is_error() { + let tmp = tempfile::TempDir::new().unwrap(); + let empty = tmp.path().join("empty"); + std::fs::create_dir_all(&empty).unwrap(); + + let mut h = Helper::spawn(); + h.send(&run_with_path_json( + "whoami", + &empty.to_string_lossy(), + &empty.to_string_lossy(), + )); + let resp = h.read_until_done(); + let err = resp + .iter() + .find_map(|v| v.get("error").and_then(|e| e.as_str())) + .unwrap_or_else(|| panic!("expected an error response; responses: {resp:?}")); + assert_eq!(err, "Could not find executable file: whoami"); + h.shutdown(); +} + +/// The action's PATHEXT restricts candidates in the helper too: with +/// PATHEXT=.EXE, a `.bat` earlier in PATH is not runnable and the `.exe` +/// later in PATH must win (the helper must honor the run command's +/// PATHEXT, not its own process environment's). +#[cfg(windows)] +#[test] +fn test_helper_windows_action_pathext_restricts_candidates() { + let tmp = tempfile::TempDir::new().unwrap(); + let dir_a = tmp.path().join("dirA"); + let dir_b = tmp.path().join("dirB"); + std::fs::create_dir_all(&dir_a).unwrap(); + std::fs::create_dir_all(&dir_b).unwrap(); + write_bat(&dir_a.join("pickext.bat"), "SHOULD-NOT-RUN"); + let system32 = PathBuf::from(std::env::var("SYSTEMROOT").unwrap()).join("System32"); + std::fs::copy(system32.join("whoami.exe"), dir_b.join("pickext.exe")).unwrap(); + + let path_value = format!("{};{}", dir_a.display(), dir_b.display()); + let mut h = Helper::spawn(); + h.send(&format!( + r#"{{"token": "{TEST_TOKEN}", "command": "pickext", "args": [], "env": {{"PATH": "{}", "PATHEXT": ".EXE"}}, "cwd": "{}"}}"#, + path_value.replace('\\', "\\\\"), + tmp.path().to_string_lossy().replace('\\', "\\\\") + )); + let resp = h.read_until_done(); + assert!( + resp.iter().any(|v| v.get("exited").is_some()), + "expected a clean exit; responses: {resp:?}" + ); + assert!( + !resp + .iter() + .any(|v| v.get("out").and_then(|o| o.as_str()) == Some("SHOULD-NOT-RUN")), + "PATHEXT=.EXE must exclude the earlier .bat; responses: {resp:?}" + ); + h.shutdown(); +} + +/// An explicit extension outside PATHEXT is not runnable through the +/// helper either: `.ps1` with a default PATHEXT is a protocol not-found +/// error, matching shutil.which — not a later spawn failure. +#[cfg(windows)] +#[test] +fn test_helper_windows_explicit_extension_outside_pathext_not_found() { + let tmp = tempfile::TempDir::new().unwrap(); + let dir_a = tmp.path().join("dirA"); + std::fs::create_dir_all(&dir_a).unwrap(); + std::fs::write(dir_a.join("script.ps1"), "Write-Output 'hi'\r\n").unwrap(); + + let mut h = Helper::spawn(); + // Pin PATHEXT explicitly: some hosts add .PS1 to their machine-wide + // PATHEXT, which would legitimately make the .ps1 runnable. + h.send(&format!( + r#"{{"token": "{TEST_TOKEN}", "command": "script.ps1", "args": [], "env": {{"PATH": "{}", "PATHEXT": ".COM;.EXE;.BAT;.CMD;.VBS;.JS;.WS;.MSC"}}, "cwd": "{}"}}"#, + dir_a.to_string_lossy().replace('\\', "\\\\"), + tmp.path().to_string_lossy().replace('\\', "\\\\") + )); + let resp = h.read_until_done(); + let err = resp + .iter() + .find_map(|v| v.get("error").and_then(|e| e.as_str())) + .unwrap_or_else(|| panic!("expected an error response; responses: {resp:?}")); + assert_eq!(err, "Could not find executable file: script.ps1"); + h.shutdown(); +} + +/// A `.bat` invoked through the helper with an argument containing a +/// newline must fail with a protocol error, not run with a truncated +/// argument: the helper's spawn uses std::process::Command, whose +/// BatBadBut mitigation (CVE-2024-24576) refuses arguments it cannot +/// safely escape for cmd.exe — an embedded newline has no escape and +/// cmd.exe would silently truncate the argument there. +#[cfg(windows)] +#[test] +fn test_helper_windows_bat_with_newline_arg_is_error() { + let tmp = tempfile::TempDir::new().unwrap(); + let dir_a = tmp.path().join("dirA"); + std::fs::create_dir_all(&dir_a).unwrap(); + std::fs::write(dir_a.join("nltool.bat"), "@echo off\r\necho %1\r\n").unwrap(); + + let mut h = Helper::spawn(); + // args contains a JSON-escaped newline (\n) inside the argument string. + h.send(&format!( + r#"{{"token": "{TEST_TOKEN}", "command": "nltool", "args": ["line one\nline two"], "env": {{"PATH": "{}"}}, "cwd": "{}"}}"#, + dir_a.to_string_lossy().replace('\\', "\\\\"), + tmp.path().to_string_lossy().replace('\\', "\\\\") + )); + let resp = h.read_until_done(); + let err = resp + .iter() + .find_map(|v| v.get("error").and_then(|e| e.as_str())) + .unwrap_or_else(|| panic!("expected an error response; responses: {resp:?}")); + assert!( + err.contains("batch file arguments are invalid"), + "expected std's batch-argument rejection over the protocol; got: {err}" + ); + h.shutdown(); +} + +/// When the run's env has no PATH, the helper falls back to its own +/// environment's PATH (the target user's PATH in production). +#[cfg(windows)] +#[test] +fn test_helper_windows_falls_back_to_helper_path() { + // echo_cmd sends env: {} — "cmd" resolves via the helper's own PATH. + let mut h = Helper::spawn(); + h.send(&echo_cmd("fallback-ok")); + let resp = h.read_until_done(); + assert!( + resp.iter() + .any(|v| v.get("out").and_then(|o| o.as_str()) == Some("fallback-ok")), + "expected cmd to resolve via the helper's own PATH; responses: {resp:?}" + ); + h.shutdown(); +} + // ──────────────────────────────────────────────────────────────────── // Windows helpers for the tests above. // ──────────────────────────────────────────────────────────────────── diff --git a/crates/openjd-sessions/tests/integration/test_win32_locate.rs b/crates/openjd-sessions/tests/integration/test_win32_locate.rs new file mode 100644 index 00000000..f0e468d8 --- /dev/null +++ b/crates/openjd-sessions/tests/integration/test_win32_locate.rs @@ -0,0 +1,344 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright by contributors to this project. +// SPDX-License-Identifier: (Apache-2.0 OR MIT) + +//! Windows executable resolution behavior through the public `Session` API. +//! +//! These tests pin the Windows PATH-search semantics that mirror the Python +//! implementation's `_win32/_locate_executable.py`, which resolves the +//! command with `shutil.which` over `working_dir;PATH` before every action: +//! +//! 1. PATHEXT-aware, earliest-directory-wins search: a `.bat` in an earlier +//! PATH directory must win over an `.exe` in a later one (Rust's +//! `std::process::Command` alone only appends `.exe` and would pick the +//! later `.exe`). +//! 2. No fallback to the worker process's own PATH or `CreateProcessW`'s +//! legacy search: a command absent from the action's PATH must fail with +//! "Could not find executable file", not silently resolve via the parent +//! environment, application directory, or system directories. +//! 3. The session working directory is searched first, so a bare name +//! materialized into the working directory resolves. + +#![cfg(windows)] + +use std::collections::HashMap; +use std::path::{Path, PathBuf}; + +use openjd_sessions::{ActionState, Session, SessionConfig, SessionState}; + +/// Create a session with stdout collection enabled and no session user. +fn make_session(root: PathBuf) -> Session { + let config = SessionConfig { + session_id: "win32-locate-test".into(), + job_parameter_values: HashMap::new(), + path_mapping_rules: None, + retain_working_dir: false, + callback: None, + os_env_vars: None, + session_root_directory: Some(root), + user: None, + profile: None, + cancel_token: None, + debug_collect_stdout: true, + echo_openjd_directives: true, + sticky_bit_policy: openjd_sessions::StickyBitPolicy::Disabled, + }; + Session::with_config(config).unwrap() +} + +/// Write a `.bat` file that prints `marker` (CRLF line endings). +fn write_bat(path: &Path, marker: &str) { + std::fs::write(path, format!("@echo off\r\necho {marker}\r\n")).unwrap(); +} + +/// Copy `whoami.exe` (a small, non-interactive, always-present system +/// executable) to `dest` so a real runnable `.exe` exists under that name. +fn copy_system_exe(dest: &Path) { + let system32 = PathBuf::from(std::env::var("SYSTEMROOT").unwrap()).join("System32"); + std::fs::copy(system32.join("whoami.exe"), dest).unwrap(); +} + +/// PATH value for the action's environment. +fn path_env(dirs: &[&Path]) -> HashMap { + let joined = dirs + .iter() + .map(|d| d.to_string_lossy().to_string()) + .collect::>() + .join(";"); + HashMap::from([("PATH".to_string(), joined)]) +} + +/// Finding 1: a `.bat` in an earlier PATH directory must beat an `.exe` of +/// the same basename in a later PATH directory (Windows canonical search +/// order, as used by cmd.exe, where.exe, and Python's shutil.which). +#[tokio::test(flavor = "multi_thread")] +async fn test_bat_earlier_in_path_beats_exe_later() { + let tmp = tempfile::TempDir::new().unwrap(); + let dir_a = tmp.path().join("dirA"); + let dir_b = tmp.path().join("dirB"); + std::fs::create_dir_all(&dir_a).unwrap(); + std::fs::create_dir_all(&dir_b).unwrap(); + write_bat(&dir_a.join("pick.bat"), "RESOLVED-BAT-A"); + copy_system_exe(&dir_b.join("pick.exe")); + + let root = tmp.path().join("session"); + std::fs::create_dir_all(&root).unwrap(); + let mut session = make_session(root); + let env = path_env(&[&dir_a, &dir_b]); + let r = session + .run_subprocess("pick", None, None, Some(&env), false, None) + .await + .unwrap(); + assert_eq!(r.state, ActionState::Success); + assert!( + r.stdout.contains("RESOLVED-BAT-A"), + "expected the .bat in the earlier PATH directory to win; stdout: {}", + r.stdout + ); + session.cleanup(); +} + +/// Finding 1 corollary: a bare name whose only match is a `.bat` on the +/// action's PATH must resolve (std::process::Command alone only tries +/// `.exe` and reports "program not found"). +#[tokio::test(flavor = "multi_thread")] +async fn test_bare_name_resolves_bat_only_match() { + let tmp = tempfile::TempDir::new().unwrap(); + let dir_a = tmp.path().join("dirA"); + std::fs::create_dir_all(&dir_a).unwrap(); + write_bat(&dir_a.join("onlybat.bat"), "ONLY-BAT"); + + let root = tmp.path().join("session"); + std::fs::create_dir_all(&root).unwrap(); + let mut session = make_session(root); + let env = path_env(&[&dir_a]); + let r = session + .run_subprocess("onlybat", None, None, Some(&env), false, None) + .await + .unwrap(); + assert_eq!(r.state, ActionState::Success); + assert!( + r.stdout.contains("ONLY-BAT"), + "expected the .bat-only match to resolve; stdout: {}", + r.stdout + ); + session.cleanup(); +} + +/// A `.bat`/`.cmd` invoked with an argument containing a newline must fail +/// with a spawn error, not run with a truncated argument. Rust's std +/// refuses to spawn a batch file with arguments it cannot safely escape +/// for cmd.exe (the BatBadBut mitigation, CVE-2024-24576); cmd.exe has no +/// escape for an embedded newline and would silently truncate the argument +/// there. This pins that the refusal (a) still happens now that resolution +/// finds `.bat` files, and (b) surfaces as a clear SubprocessStart error. +#[tokio::test(flavor = "multi_thread")] +async fn test_bat_with_newline_arg_fails_to_spawn() { + let tmp = tempfile::TempDir::new().unwrap(); + let dir_a = tmp.path().join("dirA"); + std::fs::create_dir_all(&dir_a).unwrap(); + // The .bat would echo its first argument if it ever ran. + std::fs::write(dir_a.join("nltool.bat"), "@echo off\r\necho %1\r\n").unwrap(); + + let root = tmp.path().join("session"); + std::fs::create_dir_all(&root).unwrap(); + let mut session = make_session(root); + let env = path_env(&[&dir_a]); + let err = session + .run_subprocess( + "nltool", + Some(&["line one\nline two".to_string()]), + None, + Some(&env), + false, + None, + ) + .await + .expect_err("a newline argument to a .bat must be rejected, not truncated"); + let msg = err.to_string(); + assert!( + msg.contains("batch file arguments are invalid"), + "expected std's batch-argument rejection; got: {msg}" + ); + session.cleanup(); +} + +/// Finding 2: a command that is not on the action's PATH must fail with +/// "Could not find executable file" (matching the Python implementation), +/// not silently resolve through the worker process's own PATH or +/// CreateProcessW's legacy search (application directory, system +/// directories). `whoami` exists in System32 and on the parent's PATH, so +/// success here would prove the fallback leak. +#[tokio::test(flavor = "multi_thread")] +async fn test_command_absent_from_action_path_fails() { + let tmp = tempfile::TempDir::new().unwrap(); + let empty_dir = tmp.path().join("empty"); + std::fs::create_dir_all(&empty_dir).unwrap(); + + let root = tmp.path().join("session"); + std::fs::create_dir_all(&root).unwrap(); + let mut session = make_session(root); + let env = path_env(&[&empty_dir]); + let err = session + .run_subprocess("whoami", None, None, Some(&env), false, None) + .await + .expect_err("whoami is not on the action's PATH and must not resolve via fallback search"); + assert!( + err.to_string() + .contains("Could not find executable file: whoami"), + "expected Python-parity not-found error; got: {err}" + ); + session.cleanup(); +} + +/// Finding 3: the session working directory is searched (first), matching +/// Python's `working_dir;PATH` search string — a script materialized into +/// the working directory is runnable by bare name. +#[tokio::test(flavor = "multi_thread")] +async fn test_working_directory_is_searched() { + let tmp = tempfile::TempDir::new().unwrap(); + let empty_dir = tmp.path().join("empty"); + std::fs::create_dir_all(&empty_dir).unwrap(); + + let root = tmp.path().join("session"); + std::fs::create_dir_all(&root).unwrap(); + let mut session = make_session(root); + write_bat( + &session.working_directory().join("wdtool.bat"), + "WORKING-DIR-BAT", + ); + let env = path_env(&[&empty_dir]); + let r = session + .run_subprocess("wdtool", None, None, Some(&env), false, None) + .await + .unwrap(); + assert_eq!(r.state, ActionState::Success); + assert!( + r.stdout.contains("WORKING-DIR-BAT"), + "expected the working-directory .bat to resolve; stdout: {}", + r.stdout + ); + session.cleanup(); +} + +/// The working directory wins over PATH when both contain a match, +/// because it is prepended to the search path (Python parity). +#[tokio::test(flavor = "multi_thread")] +async fn test_working_directory_wins_over_path() { + let tmp = tempfile::TempDir::new().unwrap(); + let dir_a = tmp.path().join("dirA"); + std::fs::create_dir_all(&dir_a).unwrap(); + write_bat(&dir_a.join("dup.bat"), "FROM-PATH-DIR"); + + let root = tmp.path().join("session"); + std::fs::create_dir_all(&root).unwrap(); + let mut session = make_session(root); + write_bat( + &session.working_directory().join("dup.bat"), + "FROM-WORKING-DIR", + ); + let env = path_env(&[&dir_a]); + let r = session + .run_subprocess("dup", None, None, Some(&env), false, None) + .await + .unwrap(); + assert_eq!(r.state, ActionState::Success); + assert!( + r.stdout.contains("FROM-WORKING-DIR"), + "expected the working-directory match to win over PATH; stdout: {}", + r.stdout + ); + session.cleanup(); +} + +/// The action's PATHEXT (not the worker process's) restricts candidates: +/// with PATHEXT=.EXE, a `.bat` earlier in PATH is not runnable, so the +/// `.exe` later in PATH must win. +#[tokio::test(flavor = "multi_thread")] +async fn test_action_pathext_restricts_candidates() { + let tmp = tempfile::TempDir::new().unwrap(); + let dir_a = tmp.path().join("dirA"); + let dir_b = tmp.path().join("dirB"); + std::fs::create_dir_all(&dir_a).unwrap(); + std::fs::create_dir_all(&dir_b).unwrap(); + write_bat(&dir_a.join("pickext.bat"), "SHOULD-NOT-RUN"); + copy_system_exe(&dir_b.join("pickext.exe")); + + let root = tmp.path().join("session"); + std::fs::create_dir_all(&root).unwrap(); + let mut session = make_session(root); + let mut env = path_env(&[&dir_a, &dir_b]); + env.insert("PATHEXT".to_string(), ".EXE".to_string()); + let r = session + .run_subprocess("pickext", None, None, Some(&env), false, None) + .await + .unwrap(); + assert_eq!(r.state, ActionState::Success); + assert!( + !r.stdout.contains("SHOULD-NOT-RUN"), + "PATHEXT=.EXE must exclude the earlier .bat; stdout: {}", + r.stdout + ); + session.cleanup(); +} + +/// An explicit extension outside PATHEXT is not runnable: `script.ps1` +/// must fail with the Python-parity not-found error (shutil.which +/// returns None), not resolve to the file and die later with +/// "%1 is not a valid Win32 application" (error 193). +#[tokio::test(flavor = "multi_thread")] +async fn test_explicit_extension_outside_pathext_not_found() { + let tmp = tempfile::TempDir::new().unwrap(); + let dir_a = tmp.path().join("dirA"); + std::fs::create_dir_all(&dir_a).unwrap(); + std::fs::write(dir_a.join("script.ps1"), "Write-Output 'hi'\r\n").unwrap(); + + let root = tmp.path().join("session"); + std::fs::create_dir_all(&root).unwrap(); + let mut session = make_session(root); + let mut env = path_env(&[&dir_a]); + // Pin PATHEXT explicitly: some hosts add .PS1 to their machine-wide + // PATHEXT, which would legitimately make the .ps1 runnable. + env.insert( + "PATHEXT".to_string(), + ".COM;.EXE;.BAT;.CMD;.VBS;.JS;.WS;.MSC".to_string(), + ); + let err = session + .run_subprocess("script.ps1", None, None, Some(&env), false, None) + .await + .expect_err(".ps1 is outside PATHEXT and must be not-found, not spawn-failed"); + assert!( + err.to_string() + .contains("Could not find executable file: script.ps1"), + "expected Python-parity not-found error; got: {err}" + ); + session.cleanup(); +} + +/// Absolute paths bypass resolution entirely and are passed to the OS +/// as-is (extension resolution included) — unchanged behavior. +#[tokio::test(flavor = "multi_thread")] +async fn test_absolute_path_passthrough() { + let tmp = tempfile::TempDir::new().unwrap(); + let root = tmp.path().join("session"); + std::fs::create_dir_all(&root).unwrap(); + let mut session = make_session(root); + let system32 = PathBuf::from(std::env::var("SYSTEMROOT").unwrap()).join("System32"); + let empty_dir = tmp.path().join("empty"); + std::fs::create_dir_all(&empty_dir).unwrap(); + let env = path_env(&[&empty_dir]); + let r = session + .run_subprocess( + &system32.join("whoami.exe").to_string_lossy(), + None, + None, + Some(&env), + false, + None, + ) + .await + .unwrap(); + assert_eq!(r.state, ActionState::Success); + assert_eq!(session.state(), SessionState::Ready); + session.cleanup(); +} diff --git a/specs/sessions/README.md b/specs/sessions/README.md index 9d7cbe5c..d27fc413 100644 --- a/specs/sessions/README.md +++ b/specs/sessions/README.md @@ -28,7 +28,7 @@ replaces all of that with async/await, channels, and cancellation tokens. | [tempdir.md](tempdir.md) | Secure temp directory creation, sticky bit validation, cleanup | | [logging.md](logging.md) | LogContent bitflags, structured kv metadata, session_log! macro, banners | | [error-handling.md](error-handling.md) | SessionError enum, error propagation patterns | -| [win32-locate.md](win32-locate.md) | Windows executable resolution (not yet integrated) | +| [win32-locate.md](win32-locate.md) | Windows executable resolution (PATHEXT-aware pre-spawn command search) | ## How the Worker Agent Uses Sessions diff --git a/specs/sessions/architecture.md b/specs/sessions/architecture.md index eb904feb..948dcf6e 100644 --- a/specs/sessions/architecture.md +++ b/specs/sessions/architecture.md @@ -168,21 +168,27 @@ action lifecycle through `&mut self` methods. The `drive_action` method holds `& while concurrently processing messages from the channel, which is safe because the subprocess runs in a separate future joined via `tokio::select!`. -### POSIX-first, Windows partially implemented +### Windows support The Python library supports both POSIX and Windows with extensive platform-specific code (ACLs, `CreateProcessWithLogonW`, `PopenWindowsAsUser`, etc.). The Rust crate implements -POSIX/Linux as the primary target since Linux workers are the primary deployment. - -Windows has partial support: -- Same-user subprocess execution: implemented (`subprocess.rs` Windows platform module) -- Cross-user subprocess execution: partially implemented (`WindowsSessionUser` with - `CreateProcessWithLogonW`/`CreateProcessAsUserW`, process tree kill via - `CreateToolhelp32Snapshot`) -- Win32 helpers: `win32.rs` (logon, user lookup), `win32_permissions.rs` (ACL management), - `win32_locate.rs` (executable resolution, not yet integrated) +both platforms: + +- Same-user subprocess execution: implemented (`subprocess.rs` Windows platform module), + with pre-spawn executable resolution (`win32_locate.rs` — PATHEXT-aware, + working-directory-first, no fallback to the worker's own PATH) +- Cross-user subprocess execution: implemented end-to-end — `WindowsSessionUser` + (password and logon-token modes), the embedded helper binary spawned via + `CreateProcessWithLogonW`/`CreateProcessAsUserW` (`win32.rs`), CTRL_BREAK notify and + Job-Object/`CreateToolhelp32Snapshot` process-tree termination inside the helper + (`helper/src/runner_win.rs`) +- Win32 helpers: `win32.rs` (logon, user lookup, environment blocks), + `win32_permissions.rs` (ACL management), `win32_locate.rs` (executable resolution, + called from both spawn paths) - Temp directory and embedded file permissions: Windows ACL paths implemented -- Integration testing on Windows: pending +- Integration testing on Windows: dedicated CI job (`cross-user-windows`) runs the + cross-user and permissions suites on `windows-latest` with a temporary test user, + and the standard test matrix includes Windows ## Python-vs-Rust Design Comparison diff --git a/specs/sessions/cross-user.md b/specs/sessions/cross-user.md index 4e44a097..01c3f7fd 100644 --- a/specs/sessions/cross-user.md +++ b/specs/sessions/cross-user.md @@ -12,8 +12,8 @@ actions as a different user (the job's designated user). This is required for pr deployments where the worker agent runs as root or a service account but actions must run with the job submitter's permissions. -Currently fully implemented for POSIX/Linux (including the embedded cross-user helper). -Windows has partial support — see the Windows section below. +Fully implemented for POSIX/Linux and Windows, both via the embedded cross-user +helper binary. See the Windows section below for the Windows specifics. ## SessionUser Trait @@ -131,7 +131,10 @@ The Python library supports Windows cross-user execution via: - `WindowsPermissionHelper` for ACL management - `PopenWindowsAsUser` subclass of `Popen` -The Rust crate has partial Windows support implemented: +The Rust crate implements full Windows cross-user support. Unlike Python +(which spawns each action directly as the target user), the Rust crate uses +the same architecture as POSIX: a persistent helper binary running as the +target user executes the actions. ### WindowsSessionUser (`session_user.rs`) @@ -141,19 +144,46 @@ The Rust crate has partial Windows support implemented: If the user matches the process owner, neither password nor token is needed. -### Process spawning (`win32.rs`) - -`spawn_as_user()` creates a cross-user process via `CreateProcessWithLogonW` (password -mode) or `CreateProcessAsUserW` (token mode). Environment variables are passed as a -Win32 environment block. Stdout and stderr are merged via a shared anonymous pipe -(mirroring the POSIX `dup2` approach). - -### Signal delivery (`subprocess.rs`) - -- **Notify**: `CTRL_BREAK_EVENT` via `GenerateConsoleCtrlEvent` with console - attach/detach dance (mirrors Python's `_signal_win_subprocess.py`) +### Helper spawning (`win32.rs`, `cross_user_helper.rs`) + +`CrossUserHelperWin::spawn()` launches the embedded helper binary as the target +user via `spawn_as_user_with_stdin()`, which uses `CreateProcessWithLogonW` +(password mode) or `CreateProcessAsUserW` (token mode). The target user's base +environment comes from `CreateEnvironmentBlock` on the user's token, merged +with OpenJD-managed variables under uppercase key normalization (Windows env +var names are case-insensitive). Stdout and stderr are merged via a shared +anonymous pipe (mirroring the POSIX `dup2` approach). The cancel channel is a +pipe write-end duplicated into the helper via `DuplicateHandle`. + +Individual actions are dispatched to the helper over the same stdin JSON +protocol as POSIX. Inside the helper — i.e. as the target user — the +action's command is resolved to an absolute path before spawn (see +[win32-locate.md](win32-locate.md)): the helper can probe directories only +the target user can read, and `CreateProcessW`'s legacy fallback search is +bypassed. Resolution searches an action-supplied PATH exclusively when one +is set; only when the action defines no PATH does it use the target user's +own environment PATH instead (matching the environment the workload +inherits in each case — never a union of the two). Resolution failures +return over the protocol as an error response. + +### Workload execution and signal delivery (`helper/src/runner_win.rs`) + +The helper places itself in a Job Object with +`JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE`, so if the helper dies, the kernel +reaps the whole workload tree. Workloads are spawned with +`CREATE_NEW_PROCESS_GROUP` and explicitly re-asserted into the job. + +- **Notify**: `CTRL_BREAK_EVENT` via `GenerateConsoleCtrlEvent` to the + workload's process group (CTRL_BREAK is used because CTRL_C is disabled by + `CREATE_NEW_PROCESS_GROUP`), followed by escalation to tree termination + after the notify period. - **Terminate**: `TerminateProcess` on the entire process tree via - `CreateToolhelp32Snapshot` traversal (mirrors Python's `_windows_process_killer.py`) + `CreateToolhelp32Snapshot` traversal (mirrors Python's + `_windows_process_killer.py`), with the Job Object as the backstop. + +Because signals are delivered by the helper — which runs as the target user — +no cross-user signal permissions are needed, the same property the POSIX +helper provides. ### Permissions (`win32_permissions.rs`) @@ -173,7 +203,19 @@ Two entry points: Modify ACE on the session working directory from granting write or delete access to the helper binary. -### Not yet implemented +### Testing + +Windows cross-user behavior is integration-tested in +`tests/integration/test_cross_user_windows.rs` (spawn, exit codes, env vars, +terminate, notify-then-terminate, process-tree kill, cancel handles, cleanup, +helper-binary DACL protection, bad credentials) and +`tests/integration/test_windows_permissions.rs` (ACLs). CI runs both suites +on `windows-latest` with a temporary test user created via `net user` +(the `cross-user-windows` job in `.github/workflows/ci.yml`); the tests run +with `--test-threads=1` because concurrent `CreateProcessWithLogonW` logons +for the same account can fail transiently. + +### Known limitations -- Cross-user helper binary (Windows equivalent of the POSIX embedded helper) -- Full integration testing (no Windows Docker test infrastructure yet) +- Password authentication is unsupported in Windows Session 0 (services); + a logon token must be used instead. This matches the Python library. diff --git a/specs/sessions/embedded-cross-user-helper.md b/specs/sessions/embedded-cross-user-helper.md index 1944cb56..8ca6e5a4 100644 --- a/specs/sessions/embedded-cross-user-helper.md +++ b/specs/sessions/embedded-cross-user-helper.md @@ -389,6 +389,37 @@ line warning to stderr and continues without the guard. The helper remains functional; it just loses the descendant-cleanup guarantee on crash, matching the pre-Job-Object behaviour. +### Windows runner: executable resolution + +Before spawning a workload, `runner_win::locate_executable` resolves +the command to an absolute path with `shutil.which` semantics via +`win32_which::locate_in` (a source file shared with the session +crate's same-user resolution — see [win32-locate.md](win32-locate.md) +"Shared search implementation"), searching `{cwd};{PATH}` with the +action's PATHEXT. PATH and PATHEXT are each selected from exactly one +source, chosen before searching: a key in the run command's env map +(case-insensitive) is used exclusively — the helper's own value is +never consulted, even when the search then finds nothing — and only +when the env map defines no such key at all does the helper use its +own environment, which is the target user's environment block, i.e. +the exact base environment the workload inherits in that case. This +matches the `.envs()` merge at spawn (an action-supplied value +overwrites the inherited one), so resolution always searches with what +the child actually sees, never a union of the two. Absolute paths pass +through unchanged. A not-found result — including a command whose +explicit extension is outside PATHEXT, e.g. `script.ps1` under the +default PATHEXT — is returned over the protocol as +`{"error": "Could not find executable file: "}` (the same +message Python's `_locate_executable` raises), which the session maps +to `SessionError::SubprocessStart`. + +Doing this inside the helper — as the target user — means executables +in directories only the target user can read resolve correctly, and +`CreateProcessW`'s legacy fallback search (application directory, +system directories, parent PATH) is bypassed. See +[win32-locate.md](win32-locate.md) for the full rationale and the +matching same-user resolution in the session process. + ## Session Integration ### CrossUserHelper struct diff --git a/specs/sessions/subprocess.md b/specs/sessions/subprocess.md index aef42acd..38d2b646 100644 --- a/specs/sessions/subprocess.md +++ b/specs/sessions/subprocess.md @@ -50,6 +50,20 @@ interior mutability. Instead, the subprocess sends `ActionMessage` values throug channel, and the `Session::drive_action()` method receives them with `&mut self`. This avoids shared mutable state entirely. +### Windows executable resolution + +On Windows, `run_subprocess` resolves `args[0]` to an absolute path before +spawning, via `win32_locate::locate_windows_executable` (see +[win32-locate.md](win32-locate.md)): PATHEXT-aware search over +`{working_dir};{action PATH}`, earliest directory wins, hard +`SubprocessStart` error ("Could not find executable file: …") when there is +no match. This gives canonical Windows search semantics (a `.bat` earlier in +PATH beats an `.exe` later) and prevents `CreateProcessW`'s legacy fallback +search from silently resolving commands through the worker process's own +PATH, application directory, or system directories. The cross-user path +performs the same resolution inside the helper — as the target user — see +[win32-locate.md](win32-locate.md). + ### Process Group Isolation On POSIX, the subprocess is placed in its own process group via `setsid` in a `pre_exec` diff --git a/specs/sessions/win32-locate.md b/specs/sessions/win32-locate.md index 1d45c4d2..a84ddb0c 100644 --- a/specs/sessions/win32-locate.md +++ b/specs/sessions/win32-locate.md @@ -8,63 +8,173 @@ launching. This module mirrors the Python implementation's `_win32/_locate_executable.py` to resolve executables using the working directory and PATH environment variable. -## Status - -This module is not yet integrated into the subprocess launch path. It is marked -`#[allow(dead_code)]` pending full Windows cross-user support. +## Why pre-spawn resolution is required + +Rust's `std::process::Command` alone is not a faithful Windows command search: + +1. **No PATHEXT awareness.** `Command::new("tool")` only probes `tool.exe` + per PATH directory. Canonical Windows search (cmd.exe, `where.exe`, + Python's `shutil.which`) tries every PATHEXT extension in each directory + before moving to the next — so a `tool.bat` in an earlier PATH directory + must beat a `tool.exe` in a later one. Without pre-resolution, the later + `.exe` wins, and a bare name whose only match is a `.bat`/`.cmd` fails + with "program not found". +2. **Legacy fallback search.** When the child environment's PATH has no + match, `CreateProcessW` falls back to the application directory, system + directories, and the *parent process's* PATH — even after `env_clear()`. + An action's command could silently resolve to a binary the action's + environment never referenced. +3. **Working directory.** The Python implementation searches the session + working directory first (`working_dir;PATH`), so scripts materialized as + embedded files are runnable by bare name. `std::process::Command` never + searches the cwd. + +Resolving to an absolute path before spawn eliminates all three: absolute +paths bypass the fallback machinery entirely. ## Function ```rust -pub fn locate_windows_executable( +pub(crate) fn locate_windows_executable( args: &[String], - user: Option<&dyn SessionUser>, os_env_vars: Option<&HashMap>>, - working_dir: &str, -) -> Vec + working_dir: &Path, +) -> Result, String> ``` -Returns a copy of `args` with `args[0]` resolved to an absolute path if possible. -If resolution fails, returns `args` unchanged so the OS can produce its own error. +Returns a copy of `args` with `args[0]` resolved to an absolute path, or +`Err("Could not find executable file: ")` — the same message the +Python implementation raises. The error surfaces as +`SessionError::SubprocessStart` with `std::io::ErrorKind::NotFound`. ## Resolution rules 1. **Absolute paths** — returned as-is. The OS handles extension resolution (e.g., `C:\Python\python` → `C:\Python\python.exe`). -2. **Relative names** — resolved via `which::which_in` using a search path - constructed as `{working_dir};{PATH}`. The working directory is prepended - so that executables in the session working directory take precedence. - -3. **PATH lookup** — case-insensitive key search in `os_env_vars` for the - `PATH` variable. Falls back to the process environment's `PATH` if not - found in the provided env vars. +2. **All other commands** — resolved with `shutil.which` semantics via the + shared `win32_which::locate_in` (see "Shared search implementation" + below) using a search path constructed as `{working_dir};{PATH}`. The + working directory is prepended so that executables in the session + working directory take precedence. Within each directory every + candidate PATHEXT extension is tried before moving to the next + directory (earliest directory wins). + +3. **PATHEXT semantics** — the candidate extensions come from the + action's PATHEXT, not the resolving process's: + - A bare name tries each PATHEXT extension in order per directory. + - A command that already ends with a *listed* extension + (case-insensitive) is looked up as-is. + - A command whose explicit extension is **not** in PATHEXT is not + runnable and reports not-found even if the file exists — matching + `shutil.which` and cmd.exe. (`script.ps1` under the default PATHEXT + is an error, not a spawn attempt that later fails with + `%1 is not a valid Win32 application`.) + - An empty or absent PATHEXT selects the `shutil.which` default list + (`.COM;.EXE;.BAT;.CMD;.VBS;.JS;.WS;.MSC`). + +4. **PATH/PATHEXT source selection** — each comes from exactly one + place, chosen once before searching (never a chain where a failed + search retries against the other source): + - If `os_env_vars` contains the key (case-insensitive), that value is + used **exclusively** — for PATH, even if it is empty, in which case + only the working directory is searched. The process environment is + never consulted. + - An **explicit unset** (`Some(None)` in the same-user path's + `HashMap>` — e.g. from an + `openjd_unset_env: PATH` directive; the spawn merge removes the + variable from the child) counts as *present*: PATH resolves as + empty (working directory only) and PATHEXT as the default + extension list. Falling back to the process value here would + resolve against directories the action deliberately dropped. + (The helper path is unaffected: its protocol env map is + `String → String` with unsets already filtered out before + dispatch.) + - Only when `os_env_vars` has no such key at all is the process + environment's value used instead. + + This mirrors the environment merge applied at spawn: an + action-supplied value *overwrites* the base value for the child, an + unset *removes* it, so resolution searches exactly what the child + will see. Note: Python's `_get_path_var_for_shutil_which` conflates + unset with absent and falls back to the process PATH; this is an + intentional divergence in favor of the merge semantics. + +5. **Not found** — a hard error (Python parity), never a fallthrough to + `CreateProcessW`'s legacy search. + +## Shared search implementation + +The search itself lives in `helper/src/win32_which.rs` and is compiled +into **both** binaries: the session crate includes it via +`#[path = "helper/src/win32_which.rs"]` from `win32_locate.rs`, and the +embedded helper compiles it as a regular module. One source file, two +binaries — the two spawn paths cannot drift apart. + +It is hand-written rather than using the `which` crate because `which` +cannot honor the action's environment: it always reads PATHEXT from the +resolving process (so an action setting `PATHEXT=.EXE` would still match +a `.bat`), and it accepts any existing file when the command has an +explicit extension (so `script.ps1` would resolve and then fail at spawn +with error 193 instead of the not-found error `shutil.which` gives). + +## Call sites + +Both Windows spawn paths resolve the command before launch, with +identical search semantics (both compile the shared `win32_which.rs`): + +- **Same-user** — `win32_locate::locate_windows_executable`, called from + `run_subprocess()` in `subprocess.rs` before building the merged + environment. Mirrors the Python `_runner_base.py` call site. PATH + fallback: the session process's environment. +- **Cross-user** — `locate_executable` in the embedded helper + (`helper/src/runner_win.rs`), called before `Command::new` for each + dispatched action. Resolution failures return over the stdin/stdout + protocol as `{"error": "Could not find executable file: …"}`, which + `run_via_helper` maps to `SessionError::SubprocessStart`. ## Cross-user behavior -The `user` parameter is accepted for API symmetry with the Python implementation -but is currently unused. Cross-user executable resolution would require querying -the target user's PATH, which is not yet implemented. The function falls back to -same-user resolution — the executable must be accessible to both users. - -## Known issue - -The process-environment PATH fallback has a bug where it always resolves to an -empty string instead of the actual PATH value. Since the module is not yet -integrated (`#[allow(dead_code)]`), this does not affect runtime behavior. It -should be fixed before the module is activated: - -```rust -// Current (broken): -.or_else(|| std::env::var("PATH").ok().as_deref().map(|_| "")) - -// Correct: -.unwrap_or_else(|| std::env::var("PATH").unwrap_or_default()); -``` - -## Integration plan - -When Windows cross-user support is implemented, this function should be called -from `run_subprocess` before command execution, matching the Python library's -`_locate_executable` call site. The resolved path ensures `CreateProcessAsUserW` -can find the executable even when the target user has a different PATH. +Cross-user resolution runs **inside the helper — as the target user** — +which makes it correct by construction on two axes: + +1. **Permissions.** Executables in directories only the target user can + read resolve correctly. This matches the intent of Python's + `_locate_for_other_user`, which spawns a `shutil.which` probe as the + target user; the Rust helper does the probe in-process since it already + runs as that user (no extra subprocess, no probe overhead). +2. **PATH/PATHEXT source.** The same either/or selection as the + same-user path: action-supplied values are used exclusively; only + when the action's env vars define no such key at all does the helper + use its own environment — which is the target user's environment + block (`CreateEnvironmentBlock` at helper spawn), i.e. the exact base + environment the workload inherits when the value isn't overridden. + In both cases resolution searches with what the child actually runs + under, never a union of the two. A host-side resolver would use the + *service user's* values in the no-override case, resolving against + an environment the child never sees. + +## Tests + +- Unit tests in `win32_which.rs` cover the search itself: PATHEXT + precedence, action-PATHEXT restriction (PATHEXT=.EXE excludes `.bat`), + explicit-extension-outside-PATHEXT not-found, case-insensitive + extension matching, empty-PATHEXT default, and pathy-command + cwd-only resolution. They run in both the session crate's test suite + and the helper's standalone suite. +- Unit tests in `win32_locate.rs` cover the wrapper: working-dir + precedence, case-insensitive PATH key lookup, process-PATH fallback, + absolute-path passthrough, and the not-found error. +- Integration tests in `tests/integration/test_win32_locate.rs` pin the + behaviors end-to-end through `Session::run_subprocess` (same-user), + including the "absent from action PATH must fail" guarantee that proves + the legacy fallback search is bypassed, action-PATHEXT restriction, + and `.ps1`-outside-PATHEXT not-found. +- `tests/integration/test_helper.rs` drives the helper binary directly + over its protocol and pins the helper-side resolution (PATHEXT + precedence, action-PATHEXT restriction, `.ps1` not-found, cwd-first, + not-found error, helper-PATH fallback). +- `tests/integration/test_cross_user_windows.rs` covers the full + cross-user path with a real second user (working-dir `.bat` by bare + name, not-found via the protocol, resolution in a user-readable + directory with a protected DACL).