From 080f87c32edb6b6899fd12c4c65248b705a340fc Mon Sep 17 00:00:00 2001 From: "Andrei G." Date: Wed, 13 May 2026 18:39:09 +0200 Subject: [PATCH] fix(tools): preserve env overrides and cwd across macOS sandbox rewrite rewrite_command_with_sandbox_exec replaces the whole std::process::Command with `Command::new(sandbox_exec)` to prepend sandbox-exec to the program. The replacement carried over only program + args; env overrides set via .env / .env_remove and current_dir set via .current_dir were silently dropped. Concrete impact, post #3869: - Skill secret env injection through CompositeExecutor reached ShellExecutor and was applied via cmd.envs(extra_env) in build_bash_command. Then the sandbox rewrite wiped it. Subprocess spawned with parent env only, so GITHUB_TOKEN (and any other x-requires-secrets value) never reached gh. - Any caller-configured cwd was likewise discarded. Capture program, args, envs, and current_dir BEFORE the in-place replacement, then replay envs and cwd onto the new Command. Adds a regression test that sets env, env_remove, and current_dir, runs the rewrite, and asserts all three survive. Closes #3871. --- CHANGELOG.md | 7 +++ crates/zeph-tools/src/sandbox/macos.rs | 85 +++++++++++++++++++++++++- 2 files changed, 91 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ec76bb35..75bddf3dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). `bash` subprocess) and bypassing `TrustGateExecutor`'s quarantine enforcement for active skills. Added two regression tests using a nested composition and spy executors (closes #3869). +- `zeph-tools` (macOS): `rewrite_command_with_sandbox_exec` now preserves env overrides + (`.env`/`.env_remove`) and `current_dir` across the Command rewrite that prepends + `sandbox-exec`. Previously, the wholesale `*std_cmd = Command::new(sandbox_exec)` reset + dropped every caller-configured env entry on the floor — so even with #3869 fixed, skill + secret env injection (`GITHUB_TOKEN` for the `github` skill) never reached the spawned + subprocess when `[tools.sandbox] enabled = true`. Added a regression test that asserts + env and cwd survive the rewrite (closes #3871). ### Changed diff --git a/crates/zeph-tools/src/sandbox/macos.rs b/crates/zeph-tools/src/sandbox/macos.rs index 8796590d0..03b16e7a8 100644 --- a/crates/zeph-tools/src/sandbox/macos.rs +++ b/crates/zeph-tools/src/sandbox/macos.rs @@ -358,12 +358,20 @@ fn rewrite_command_with_sandbox_exec( // Since tokio 1.x does not expose set_program, we rebuild via the `as_std_mut` method. let std_cmd = cmd.as_std_mut(); - // Collect existing args before clearing. + // Collect existing program, args, env overrides, and cwd before clearing. + // The Command struct is fully replaced below to swap the program to sandbox-exec; without + // capturing these, caller-configured per-spawn state (skill secret env, cwd, etc.) is + // silently dropped on the floor — see #3871. let original_program = std_cmd.get_program().to_os_string(); let original_args: Vec = std_cmd .get_args() .map(std::ffi::OsStr::to_os_string) .collect(); + let original_envs: Vec<(std::ffi::OsString, Option)> = std_cmd + .get_envs() + .map(|(k, v)| (k.to_os_string(), v.map(std::ffi::OsStr::to_os_string))) + .collect(); + let original_cwd: Option = std_cmd.get_current_dir().map(Path::to_path_buf); // Replace program with sandbox-exec. *std_cmd = std::process::Command::new(sandbox_exec); @@ -374,6 +382,21 @@ fn rewrite_command_with_sandbox_exec( for arg in original_args { std_cmd.arg(arg); } + + // Restore env overrides and cwd captured above. `Some(v)` = explicit set, `None` = remove. + for (key, value) in original_envs { + match value { + Some(val) => { + std_cmd.env(key, val); + } + None => { + std_cmd.env_remove(key); + } + } + } + if let Some(cwd) = original_cwd { + std_cmd.current_dir(cwd); + } // stdout/stderr piping must be re-applied by the caller (execute_bash already does this // before calling wrap, so the Stdio handles are set on the freshly-built std_cmd above). // Actually: Stdio configuration is not preserved across Command replacement. The caller @@ -755,4 +778,64 @@ mod tests { "allow must appear after deny (last-rule-wins)" ); } + + /// Regression for #3871: env overrides and cwd set on the original Command must survive + /// the sandbox-exec rewrite. Before the fix, the inner `*std_cmd = Command::new(...)` + /// reset replaced the entire struct, dropping `.env(...)` / `.env_remove(...)` / `.current_dir(...)` + /// entries — breaking skill secret env injection (`x-requires-secrets`) on macOS. + #[test] + fn rewrite_preserves_env_overrides_and_cwd() { + let mut cmd = tokio::process::Command::new("bash"); + cmd.arg("-c").arg("echo hi"); + cmd.env("GITHUB_TOKEN", "tok-xyz"); + cmd.env("FOO", "bar"); + cmd.env_remove("SECRET_TO_DROP"); + cmd.current_dir("/tmp"); + + let sandbox_exec = PathBuf::from("/usr/bin/sandbox-exec"); + let profile_path = PathBuf::from("/tmp/fake-profile.sb"); + rewrite_command_with_sandbox_exec(&mut cmd, &sandbox_exec, &profile_path); + + let std_cmd = cmd.as_std(); + assert_eq!(std_cmd.get_program(), "/usr/bin/sandbox-exec"); + + let args: Vec<&std::ffi::OsStr> = std_cmd.get_args().collect(); + assert_eq!( + args, + vec![ + std::ffi::OsStr::new("-f"), + std::ffi::OsStr::new("/tmp/fake-profile.sb"), + std::ffi::OsStr::new("--"), + std::ffi::OsStr::new("bash"), + std::ffi::OsStr::new("-c"), + std::ffi::OsStr::new("echo hi"), + ] + ); + + let envs: std::collections::HashMap> = + std_cmd + .get_envs() + .map(|(k, v)| (k.to_os_string(), v.map(std::ffi::OsStr::to_os_string))) + .collect(); + assert_eq!( + envs.get(std::ffi::OsStr::new("GITHUB_TOKEN")), + Some(&Some(std::ffi::OsString::from("tok-xyz"))), + "GITHUB_TOKEN must survive sandbox rewrite (#3871)" + ); + assert_eq!( + envs.get(std::ffi::OsStr::new("FOO")), + Some(&Some(std::ffi::OsString::from("bar"))) + ); + assert_eq!( + envs.get(std::ffi::OsStr::new("SECRET_TO_DROP")), + Some(&None), + "env_remove entries must also be preserved as removals" + ); + + assert_eq!( + std_cmd.get_current_dir(), + Some(std::path::Path::new("/tmp")), + "current_dir must survive sandbox rewrite" + ); + } }