diff --git a/crates/openhuman-cli/Cargo.toml b/crates/openhuman-cli/Cargo.toml index ba9c6054fc9..24c80d722b0 100644 --- a/crates/openhuman-cli/Cargo.toml +++ b/crates/openhuman-cli/Cargo.toml @@ -195,6 +195,10 @@ path = "../../tests/composio_list_tools_stack_overflow_regression.rs" name = "cwd_jail_e2e" path = "../../tests/cwd_jail_e2e.rs" +[[test]] +name = "windows_sandbox_env_e2e" +path = "../../tests/windows_sandbox_env_e2e.rs" + [[test]] name = "inference_provider_e2e" path = "../../tests/inference_provider_e2e.rs" diff --git a/crates/openhuman-core/src/agent/platform_shell.rs b/crates/openhuman-core/src/agent/platform_shell.rs index 82981648e80..36e804b87e5 100644 --- a/crates/openhuman-core/src/agent/platform_shell.rs +++ b/crates/openhuman-core/src/agent/platform_shell.rs @@ -29,6 +29,109 @@ use std::path::Path; +/// Environment variables a Windows child process needs before it can run at +/// all, beyond the functional allow-list each launcher already forwards. +/// +/// Every sanitized spawn path in the core calls `env_clear()` and re-forwards +/// only what it names, so this list is what makes a cleared Windows environment +/// bootable. These are forwarded from the parent environment, never synthesised +/// or hard-coded: a name the parent does not have is simply not set. +/// +/// Measured consequences of omitting them (Windows 11, `SystemRoot` absent from +/// an otherwise-valid child environment): +/// +/// - `node.exe` aborts during startup with +/// `Assertion failed: ncrypto::CSPRNG(nullptr, 0)` (exit 134), because the +/// OS random provider cannot initialise without a system directory. +/// - `powershell.exe` exits with `Internal Windows PowerShell error. Loading +/// managed Windows PowerShell failed with error 8009001d.` +/// - `cmd.exe` leaves `%SystemRoot%`/`%TEMP%`/`%USERPROFILE%` unexpanded. It +/// does *not* repair them for its own children, so a wrapper shell cannot +/// rescue a stripped environment. +/// +/// `COMSPEC` and `PATHEXT` are here because `cmd.exe` needs `PATHEXT` to resolve +/// the `.cmd`/`.bat` shims that npm, npx and the Git toolchain are installed +/// as; `TEMP`/`TMP`/`USERPROFILE`/`APPDATA`/`LOCALAPPDATA` because tooling +/// writes scratch and reads config from them; the `ProgramFiles*` trio because +/// installers and SDK locators probe them. +/// +/// Single source of truth: keep every launcher's allow-list a superset of this +/// (enforced by the launcher unit tests), and add a launcher to those tests +/// rather than inventing a sixth list. +pub const WINDOWS_PROCESS_ENV_VARS: &[&str] = &[ + "SystemRoot", + "WINDIR", + "COMSPEC", + "PATHEXT", + "TEMP", + "TMP", + "USERPROFILE", + "APPDATA", + "LOCALAPPDATA", + "ProgramFiles", + "ProgramFiles(x86)", + "ProgramW6432", +]; + +/// Assert that a launcher's environment allow-list covers every Windows +/// process-bootstrap variable. +/// +/// Each launcher guards its own list with this, so a new launcher cannot +/// silently ship a `env_clear()` that produces an unbootable Windows child. +/// The failure this prevents is not a clean error: the child aborts inside the +/// OS crypto provider (`node` → `ncrypto::CSPRNG` assertion, `powershell` → +/// `8009001d`), and the harness reports it as a mysterious exit code rather +/// than a missing environment variable. +#[track_caller] +pub fn assert_forwards_windows_bootstrap(allowlist: &[&str], launcher: &str) { + for var in WINDOWS_PROCESS_ENV_VARS { + assert!( + allowlist.contains(var), + "{launcher} clears the child environment but does not forward \ + `{var}`; a Windows child spawned without it aborts during crypto \ + init. Add it to the allow-list." + ); + } +} + +/// Add Windows bootstrap variables to a host child. Keep these out of the +/// sandbox policy because that policy is also forwarded into Linux containers. +#[cfg(windows)] +pub fn forward_windows_bootstrap_env(cmd: &mut tokio::process::Command) -> anyhow::Result<()> { + for var in WINDOWS_PROCESS_ENV_VARS { + if let Ok(val) = std::env::var(var) { + if val.is_empty() { + anyhow::bail!("Windows bootstrap environment variable {var} is empty"); + } + cmd.env(var, val); + } + } + Ok(()) +} + +#[cfg(not(windows))] +pub fn forward_windows_bootstrap_env(_cmd: &mut tokio::process::Command) -> anyhow::Result<()> { + Ok(()) +} + +#[cfg(windows)] +pub fn forward_windows_bootstrap_env_std(cmd: &mut std::process::Command) -> anyhow::Result<()> { + for var in WINDOWS_PROCESS_ENV_VARS { + if let Ok(val) = std::env::var(var) { + if val.is_empty() { + anyhow::bail!("Windows bootstrap environment variable {var} is empty"); + } + cmd.env(var, val); + } + } + Ok(()) +} + +#[cfg(not(windows))] +pub fn forward_windows_bootstrap_env_std(_cmd: &mut std::process::Command) -> anyhow::Result<()> { + Ok(()) +} + /// Whether the Unix arm prefixes `set -o pipefail`. #[derive(Clone, Copy, PartialEq, Eq)] enum PipeFail { diff --git a/crates/openhuman-core/src/sandbox/ops.rs b/crates/openhuman-core/src/sandbox/ops.rs index 84a0d60806a..2ca9b0de421 100644 --- a/crates/openhuman-core/src/sandbox/ops.rs +++ b/crates/openhuman-core/src/sandbox/ops.rs @@ -17,6 +17,20 @@ use std::path::{Path, PathBuf}; use std::time::Duration; /// Safe environment variables forwarded into sandboxed execution. +/// +/// This list is the *entire* environment a sandboxed child gets: both +/// [`execute_unsandboxed`] and [`execute_local_jail`] call `env_clear()` and +/// re-forward only what is named here. Windows process-bootstrap variables are +/// added only in the host spawn paths below; keeping them out of this policy +/// prevents Windows paths from being passed into Linux Docker containers. +/// +/// They were missing when this defect was found, and it survived the first +/// round of fixes because the four tool launchers (`shell`, `node_exec`, +/// `npm_exec`, `python_exec`) each carry their own copy of the allow-list and +/// had already been patched: the built-in `orchestrator` runs with +/// `sandbox_mode = "sandboxed"`, and all four tools divert to +/// [`crate::sandbox`] *before* reaching those lists, so the host spawn paths +/// below are the ones that must add the Windows-only bootstrap set. pub const SANDBOX_ENV_PASSTHROUGH: &[&str] = &[ "PATH", "HOME", "TERM", "LANG", "LC_ALL", "LC_CTYPE", "USER", "SHELL", "TMPDIR", ]; @@ -259,9 +273,13 @@ async fn execute_unsandboxed( cmd.env_clear(); for var in SANDBOX_ENV_PASSTHROUGH { if let Ok(val) = std::env::var(var) { + if val.is_empty() { + anyhow::bail!("sandbox passthrough environment variable {var} is empty"); + } cmd.env(var, val); } } + platform_shell::forward_windows_bootstrap_env(&mut cmd)?; for (k, v) in extra_env { cmd.env(k, v); } @@ -378,6 +396,9 @@ async fn execute_local_jail( jail = jail.add_read_write(&scratch.path); let stdout_file = capture.stdout(); let stderr_file = capture.stderr(); + let caller_sets_tmpdir = extra_env.contains_key(std::ffi::OsStr::new("TMPDIR")); + let caller_sets_temp = extra_env.contains_key(std::ffi::OsStr::new("TEMP")); + let caller_sets_tmp = extra_env.contains_key(std::ffi::OsStr::new("TMP")); // Platform-aware output-capture wrap: `{ … ; } > … 2> …` on sh/bash, // trailing `> … 2> …` on cmd.exe (no brace grouping). Shell binary is // picked by `platform_shell` so this path is Windows-safe (#4705). @@ -387,13 +408,28 @@ async fn execute_local_jail( cmd.env_clear(); for var in SANDBOX_ENV_PASSTHROUGH { if let Ok(val) = std::env::var(var) { + if val.is_empty() { + anyhow::bail!("sandbox passthrough environment variable {var} is empty"); + } cmd.env(var, val); } } - cmd.env("TMPDIR", &scratch.path); + platform_shell::forward_windows_bootstrap_env_std(&mut cmd)?; for (k, v) in extra_env { cmd.env(k, v); } + // Keep every Windows spelling of the temporary directory inside this + // per-call grant. `TEMP`/`TMP` are the variables used by Windows tools; + // `TMPDIR` covers Unix-oriented tools running on the same host. + if !caller_sets_tmpdir { + cmd.env("TMPDIR", &scratch.path); + } + if !caller_sets_temp { + cmd.env("TEMP", &scratch.path); + } + if !caller_sets_tmp { + cmd.env("TMP", &scratch.path); + } let os_backend = cwd_jail::default_backend(); let spawn_result = if os_backend.is_available() { diff --git a/crates/openhuman-core/src/sandbox/ops_tests.rs b/crates/openhuman-core/src/sandbox/ops_tests.rs index 374ced5cb81..082e8dd5464 100644 --- a/crates/openhuman-core/src/sandbox/ops_tests.rs +++ b/crates/openhuman-core/src/sandbox/ops_tests.rs @@ -322,6 +322,23 @@ fn local_status_is_ready_for_a_real_jail() { } } +// ── Windows child environment ──────────────────────────────────────────────── +// +// `execute_unsandboxed` and `execute_local_jail` both call `env_clear()` and +// re-forward only `SANDBOX_ENV_PASSTHROUGH`. On Windows that list has to carry +// the process-bootstrap variables or the child cannot initialise the OS crypto +// provider — `node` dies with `Assertion failed: ncrypto::CSPRNG(nullptr, 0)` +// (exit 134) and `powershell` with `8009001d`. Both read as opaque failures to +// the agent, which is how the original defect shipped: the four tool launchers' +// allow-lists were fixed, the sandbox allow-list — the one the `sandboxed` +// orchestrator actually routes through — was not. + +// What a real child actually receives is covered end-to-end in +// `tests/windows_sandbox_env_e2e.rs`, which spawns through `execute_in_sandbox` +// and asserts on the child's own environment. It lives there because the probe +// has to be a Rust `CreateProcess` spawn: `node`'s own `child_process.spawn` +// silently injects `SystemRoot`, so a JavaScript probe reports a stripped +// environment as healthy. // ── #6961: local-jail output capture stays out of the user's project ───────── #[cfg(unix)] diff --git a/crates/openhuman-core/src/tools/impl/system/node_exec_tests.rs b/crates/openhuman-core/src/tools/impl/system/node_exec_tests.rs index 62d4187f49b..9615701f339 100644 --- a/crates/openhuman-core/src/tools/impl/system/node_exec_tests.rs +++ b/crates/openhuman-core/src/tools/impl/system/node_exec_tests.rs @@ -150,3 +150,14 @@ fn resolve_script_path_targets_action_dir_not_workspace_dir() { resolved.display() ); } + +/// `node_exec` clears the child environment, so its allow-list must carry the +/// Windows process-bootstrap set — without `SystemRoot` a spawned `node.exe` +/// aborts with `Assertion failed: ncrypto::CSPRNG(nullptr, 0)`. +#[test] +fn safe_env_vars_cover_windows_bootstrap() { + crate::agent::platform_shell::assert_forwards_windows_bootstrap( + SAFE_ENV_VARS, + "node_exec::SAFE_ENV_VARS", + ); +} diff --git a/crates/openhuman-core/src/tools/impl/system/npm_exec_tests.rs b/crates/openhuman-core/src/tools/impl/system/npm_exec_tests.rs index 30c51a322e0..059e42a84eb 100644 --- a/crates/openhuman-core/src/tools/impl/system/npm_exec_tests.rs +++ b/crates/openhuman-core/src/tools/impl/system/npm_exec_tests.rs @@ -75,10 +75,8 @@ fn resolve_cwd_allows_relative_subdir() { #[test] fn safe_env_vars_include_windows_process_essentials() { - for var in ["SystemRoot", "COMSPEC", "PATHEXT", "TEMP", "USERPROFILE"] { - assert!( - SAFE_ENV_VARS.contains(&var), - "{var} must be forwarded for Windows child processes" - ); - } + crate::agent::platform_shell::assert_forwards_windows_bootstrap( + SAFE_ENV_VARS, + "npm_exec::SAFE_ENV_VARS", + ); } diff --git a/crates/openhuman-core/src/tools/impl/system/python_exec_tests.rs b/crates/openhuman-core/src/tools/impl/system/python_exec_tests.rs index b8b87da3f89..94bdbe59a24 100644 --- a/crates/openhuman-core/src/tools/impl/system/python_exec_tests.rs +++ b/crates/openhuman-core/src/tools/impl/system/python_exec_tests.rs @@ -37,3 +37,13 @@ fn resolve_script_path_rejects_escapes() { std::path::Path::new("/ws/scripts/run.py") ); } + +/// `python_exec` clears the child environment, so its allow-list must carry the +/// Windows process-bootstrap set — a child without them fails to start. +#[test] +fn safe_env_vars_cover_windows_bootstrap() { + crate::agent::platform_shell::assert_forwards_windows_bootstrap( + SAFE_ENV_VARS, + "python_exec::SAFE_ENV_VARS", + ); +} diff --git a/crates/openhuman-core/src/tools/impl/system/shell_tests_schema_and_env_tests.rs b/crates/openhuman-core/src/tools/impl/system/shell_tests_schema_and_env_tests.rs index c012a38fec2..cbbbbae6b54 100644 --- a/crates/openhuman-core/src/tools/impl/system/shell_tests_schema_and_env_tests.rs +++ b/crates/openhuman-core/src/tools/impl/system/shell_tests_schema_and_env_tests.rs @@ -618,10 +618,8 @@ fn shell_safe_env_vars_includes_essentials() { #[test] fn shell_safe_env_vars_include_windows_process_essentials() { - for var in ["SystemRoot", "COMSPEC", "PATHEXT", "TEMP", "USERPROFILE"] { - assert!( - SAFE_ENV_VARS.contains(&var), - "{var} must be forwarded for Windows child processes" - ); - } + crate::agent::platform_shell::assert_forwards_windows_bootstrap( + SAFE_ENV_VARS, + "shell::SAFE_ENV_VARS", + ); } diff --git a/tests/windows_sandbox_env_e2e.rs b/tests/windows_sandbox_env_e2e.rs new file mode 100644 index 00000000000..300254113fd --- /dev/null +++ b/tests/windows_sandbox_env_e2e.rs @@ -0,0 +1,298 @@ +//! Windows child-environment e2e for OpenHuman's own sandbox execution path. +//! +//! Both `sandbox::ops` exec functions call `Command::env_clear()` and re-forward +//! only `SANDBOX_ENV_PASSTHROUGH`, so that list is the entire environment a +//! sandboxed child gets. This test measures what a real child sees when spawned +//! through `execute_in_sandbox` — the same function the `shell`, `node_exec`, +//! `npm_exec` and `python_exec` tools route into whenever the active agent is +//! `SandboxMode::Sandboxed` (which the built-in `orchestrator` is). +//! +//! The defect this pins: the list carried no Windows process-bootstrap +//! variables. A child without `SystemRoot` cannot initialise the OS crypto +//! provider, and the failures are opaque to the agent rather than diagnostic: +//! +//! - `node.exe` aborts at startup — `Assertion failed: ncrypto::CSPRNG(nullptr, 0)`, exit 134. +//! - `powershell.exe` exits with `Internal Windows PowerShell error. Loading +//! managed Windows PowerShell failed with error 8009001d.` +//! +//! Note on method: these must be spawned from Rust. `node`'s own +//! `child_process.spawn` (libuv) injects `SystemRoot` into the child, and a +//! probe driven through `cmd.exe` from JavaScript reports it as present, so a +//! JS harness cannot observe the stripping at all. + +#[cfg(windows)] +use std::collections::HashMap; +#[cfg(windows)] +use std::time::Duration; + +#[cfg(windows)] +use openhuman_core::agent::harness::definition::SandboxMode; +#[cfg(windows)] +use openhuman_core::config::RuntimeConfig; +#[cfg(windows)] +use openhuman_core::sandbox::ops::{execute_in_sandbox, resolve_sandbox_policy}; + +/// Run `command` through OpenHuman's sandbox execution path under `mode`. +#[cfg(windows)] +async fn run_in_sandbox( + mode: SandboxMode, + command: &str, +) -> ( + openhuman_core::sandbox::types::SandboxExecResult, + std::path::PathBuf, +) { + run_in_sandbox_with_env(mode, command, HashMap::new()).await +} + +#[cfg(windows)] +async fn run_in_sandbox_with_env( + mode: SandboxMode, + command: &str, + extra_env: HashMap, +) -> ( + openhuman_core::sandbox::types::SandboxExecResult, + std::path::PathBuf, +) { + let tempdir = tempfile::tempdir().expect("tempdir"); + let root = tempdir.path().to_path_buf(); + let policy = resolve_sandbox_policy( + mode, + tempdir.path(), + tempdir.path(), + &RuntimeConfig::default(), + false, + ); + let result = execute_in_sandbox( + &policy, + command, + tempdir.path(), + extra_env + .into_iter() + .map(|(key, value)| (key.into(), value.into())) + .collect(), + Duration::from_secs(120), + ) + .await + .unwrap_or_else(|e| panic!("execute_in_sandbox({mode:?}) failed to run the command: {e}")); + (result, root) +} + +#[cfg(windows)] +fn env_probe_command() -> &'static str { + "echo SR=[%SystemRoot%] TEMP=[%TEMP%] UP=[%USERPROFILE%] PATH=[%PATH%]" +} + +/// `cmd.exe` prints `%SystemRoot%` *verbatim* when the variable is absent, which +/// makes non-expansion the assertion. A resolved value is also required, so a +/// child that echoes nothing cannot pass. +#[cfg(windows)] +fn assert_bootstrap_env_resolved(result: &openhuman_core::sandbox::types::SandboxExecResult) { + assert!( + result.success(), + "child exited {} with stderr {:?}", + result.exit_code, + result.stderr + ); + for name in ["SystemRoot", "TEMP", "USERPROFILE", "PATH"] { + assert!( + !result.stdout.contains(&format!("%{name}%")), + "`{name}` reached the child unexpanded, so it is not in the \ + forwarded environment; stdout {:?}", + result.stdout + ); + } + let system_root = bracketed(&result.stdout, "SR=").unwrap_or_default(); + assert!( + system_root.contains(':'), + "`SystemRoot` did not resolve to a directory; stdout {:?}", + result.stdout + ); + assert!( + !bracketed(&result.stdout, "UP=") + .unwrap_or_default() + .is_empty(), + "`USERPROFILE` resolved to nothing; stdout {:?}", + result.stdout + ); + for name in ["TEMP", "PATH"] { + assert!( + !bracketed(&result.stdout, &format!("{name}=")) + .unwrap_or_default() + .is_empty(), + "`{name}` resolved to nothing; stdout {:?}", + result.stdout + ); + } +} + +#[cfg(windows)] +fn assert_temp_is_in_scratch( + result: &openhuman_core::sandbox::types::SandboxExecResult, + root: &std::path::Path, +) { + let temp = bracketed(&result.stdout, "TEMP=").unwrap_or_default(); + let expected_prefix = root.join("artifacts").join("sandbox-scratch"); + let normalize = |path: &str| path.replace('/', "\\").to_ascii_lowercase(); + assert!( + normalize(&temp).starts_with(&normalize(&expected_prefix.to_string_lossy())), + "local-jail TEMP escaped its per-test scratch tree: TEMP={temp:?}, expected under {:?}; stdout {:?}", + expected_prefix, + result.stdout + ); +} + +#[cfg(windows)] +fn bracketed(stdout: &str, prefix: &str) -> Option { + let tail = stdout.split(prefix).nth(1)?; + Some(tail.split(']').next()?.trim().to_string()) +} + +/// The unsandboxed exec path (`SandboxMode::None`), which shares the +/// allow-list with the jail path and has no OS-jail dependency. +#[cfg(windows)] +#[tokio::test] +async fn unsandboxed_child_receives_windows_bootstrap_env() { + let (result, _) = run_in_sandbox(SandboxMode::None, env_probe_command()).await; + assert_bootstrap_env_resolved(&result); +} + +/// The route the built-in orchestrator actually takes: +/// `SandboxMode::Sandboxed` → `SandboxBackendKind::Local` → +/// `execute_local_jail`. +#[cfg(windows)] +#[tokio::test] +async fn sandboxed_child_receives_windows_bootstrap_env() { + let (result, root) = run_in_sandbox(SandboxMode::Sandboxed, env_probe_command()).await; + assert_bootstrap_env_resolved(&result); + assert_temp_is_in_scratch(&result, &root); +} + +/// Caller-provided temporary-directory values remain effective on both host +/// spawn paths; the local jail must not silently replace per-call overrides. +#[cfg(windows)] +#[tokio::test] +async fn caller_temp_overrides_survive_sandbox_paths() { + let expected = r"C:\openhuman-test-temp"; + for mode in [SandboxMode::None, SandboxMode::Sandboxed] { + let (result, _) = run_in_sandbox_with_env( + mode, + "echo TEMP=[%TEMP%] TMP=[%TMP%] TMPDIR=[%TMPDIR%]", + HashMap::from([ + ("TEMP".to_string(), expected.to_string()), + ("TMP".to_string(), expected.to_string()), + ("TMPDIR".to_string(), expected.to_string()), + ]), + ) + .await; + assert!(result.success(), "child failed: {:?}", result.stderr); + for name in ["TEMP", "TMP", "TMPDIR"] { + assert_eq!( + bracketed(&result.stdout, &format!("{name}=")).as_deref(), + Some(expected), + "caller override for {name} was replaced; stdout {:?}", + result.stdout + ); + } + } +} + +/// The reported Node failure, end to end through OpenHuman's spawn code: +/// `node -e` must reach the crypto provider and print random bytes. +#[cfg(windows)] +#[tokio::test] +async fn node_crypto_runs_through_sandbox_path() { + assert!( + tool_available("node"), + "node is required for node_crypto_runs_through_sandbox_path; missing tooling must not silently pass" + ); + + let (result, _) = run_in_sandbox( + SandboxMode::None, + r#"node -e "console.log('SR=' + process.env.SystemRoot); console.log(require('crypto').randomBytes(8).toString('hex'))""#, + ) + .await; + + assert!( + !result.stderr.contains("ncrypto"), + "node's OS random provider failed to initialise — the child \ + environment is missing Windows bootstrap variables; stderr {:?}", + result.stderr + ); + assert!( + result.success(), + "node aborted through the sandbox path (exit {}); stderr {:?}", + result.exit_code, + result.stderr + ); + assert!( + result + .stdout + .lines() + .any(|line| line.trim().len() == 16 + && line.trim().chars().all(|c| c.is_ascii_hexdigit())), + "no random bytes were printed; stdout {:?}", + result.stdout + ); +} + +/// The reported PowerShell failure: `8009001d` when the child has no system +/// directory to load its crypto provider from. +#[cfg(windows)] +#[tokio::test] +async fn powershell_runs_through_sandbox_path() { + assert!( + tool_available("powershell.exe"), + "powershell.exe is required for powershell_runs_through_sandbox_path; missing tooling must not silently pass" + ); + + let (result, _) = run_in_sandbox( + SandboxMode::None, + "powershell.exe -NoProfile -Command [guid]::NewGuid().ToString()", + ) + .await; + + assert!( + !result.stderr.contains("8009001d"), + "PowerShell could not load managed PowerShell — the child environment \ + is missing Windows bootstrap variables; stderr {:?}", + result.stderr + ); + assert!( + result.success(), + "powershell failed through the sandbox path (exit {}); stderr {:?}", + result.exit_code, + result.stderr + ); + let guid: String = result + .stdout + .trim() + .chars() + .filter(|c| c.is_ascii_hexdigit() || *c == '-') + .collect(); + assert!( + guid.len() >= 36, + "no GUID was produced by the child; stdout {:?}", + result.stdout + ); +} + +#[cfg(windows)] +fn tool_available(program: &str) -> bool { + let args = if program.eq_ignore_ascii_case("powershell.exe") { + vec![ + "-NoProfile", + "-NonInteractive", + "-Command", + "$PSVersionTable.PSVersion.ToString()", + ] + } else { + vec!["--version"] + }; + std::process::Command::new(program) + .args(args) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + .map(|s| s.success()) + .unwrap_or(false) +}