From cf80fe567ab037d422bdb5b577ceff3b5154ab78 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sat, 18 Apr 2026 01:55:01 -0400 Subject: [PATCH] test(harness): scrub ambient creds from spawned fabro CLI CLI integration tests spawned the real fabro binary while letting the parent process's env pass through. The pr_view "no credentials" snapshot failed in CI because the Nightly workflow's minted GITHUB_TOKEN was inherited by the child and turned the expected "credentials required" error into a real GitHub API call (404 / 401). On developer laptops the same leak occurs whenever gh auth login is active. Introduce apply_test_isolation(cmd, home) in fabro-test: env_clear() + re-populate PATH, HOME, NO_COLOR, and the FABRO_* test overrides. Route TestContext::command(), the internal server bootstrap, and the four ad-hoc spawners in tests/it/cmd/{attach,render_graph,runner,server_start} through the same helper so the isolation is systemic instead of per-callsite. Tests that deliberately need a credential (OPENAI_API_KEY, GITHUB_APP_PRIVATE_KEY, etc.) continue to set it explicitly on the returned Command; those survive the clear. Add a regression test that sets sentinel GITHUB_TOKEN and ANTHROPIC_API_KEY in the parent, spawns /usr/bin/env through the helper, and asserts the child sees neither credential while still seeing PATH and the harness's FABRO_NO_UPGRADE_CHECK override. Verified: the full workspace (4022 tests) passes with GITHUB_TOKEN and ANTHROPIC_API_KEY set in the parent, which previously broke the pr_view_reads_pull_request_from_store_without_pull_request_json snapshot. cargo fmt and nightly clippy are clean. --- lib/crates/fabro-cli/tests/it/cmd/attach.rs | 8 +- .../fabro-cli/tests/it/cmd/render_graph.rs | 5 +- lib/crates/fabro-cli/tests/it/cmd/runner.rs | 7 +- .../fabro-cli/tests/it/cmd/server_start.rs | 10 +- lib/crates/fabro-test/src/lib.rs | 103 +++++++++++++----- 5 files changed, 84 insertions(+), 49 deletions(-) diff --git a/lib/crates/fabro-cli/tests/it/cmd/attach.rs b/lib/crates/fabro-cli/tests/it/cmd/attach.rs index 7340be8b2..a27a59b0b 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/attach.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/attach.rs @@ -199,14 +199,8 @@ fn attach_before_completion_streams_to_finished_state() { "[DURATION]".to_string(), )); let mut attach_cmd = std::process::Command::new(env!("CARGO_BIN_EXE_fabro")); + fabro_test::apply_test_isolation(&mut attach_cmd, &context.home_dir); attach_cmd.current_dir(&context.temp_dir); - attach_cmd.env("NO_COLOR", "1"); - attach_cmd.env("HOME", &context.home_dir); - attach_cmd - .env("FABRO_NO_UPGRADE_CHECK", "true") - .env("FABRO_HTTP_PROXY_POLICY", "disabled"); - attach_cmd.env("FABRO_SERVER_MAX_CONCURRENT_RUNS", "64"); - attach_cmd.env("FABRO_TEST_IN_MEMORY_STORE", "1"); attach_cmd.args(["attach", &run_id]); attach_cmd.stdout(Stdio::piped()); attach_cmd.stderr(Stdio::piped()); diff --git a/lib/crates/fabro-cli/tests/it/cmd/render_graph.rs b/lib/crates/fabro-cli/tests/it/cmd/render_graph.rs index e58c0bef7..3313dee01 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/render_graph.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/render_graph.rs @@ -10,11 +10,8 @@ use fabro_test::{fabro_snapshot, test_context}; fn render_graph_command(context: &fabro_test::TestContext) -> Command { let mut cmd = Command::new(env!("CARGO_BIN_EXE_fabro")); + fabro_test::apply_test_isolation(&mut cmd, &context.home_dir); cmd.current_dir(&context.temp_dir); - cmd.env("NO_COLOR", "1"); - cmd.env("HOME", &context.home_dir); - cmd.env("FABRO_NO_UPGRADE_CHECK", "true") - .env("FABRO_HTTP_PROXY_POLICY", "disabled"); cmd } diff --git a/lib/crates/fabro-cli/tests/it/cmd/runner.rs b/lib/crates/fabro-cli/tests/it/cmd/runner.rs index 9f822804d..6f2d0291e 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/runner.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/runner.rs @@ -48,13 +48,8 @@ fn spawn_worker_process( mode: &str, ) -> Child { let mut cmd = std::process::Command::new(env!("CARGO_BIN_EXE_fabro")); + fabro_test::apply_test_isolation(&mut cmd, &context.home_dir); cmd.current_dir(&context.temp_dir); - cmd.env("NO_COLOR", "1"); - cmd.env("HOME", &context.home_dir); - cmd.env("FABRO_NO_UPGRADE_CHECK", "true") - .env("FABRO_HTTP_PROXY_POLICY", "disabled"); - cmd.env("FABRO_SERVER_MAX_CONCURRENT_RUNS", "64"); - cmd.env("FABRO_TEST_IN_MEMORY_STORE", "1"); if let Some(token) = local_dev_token(&context.storage_dir) { cmd.env("FABRO_DEV_TOKEN", token); } diff --git a/lib/crates/fabro-cli/tests/it/cmd/server_start.rs b/lib/crates/fabro-cli/tests/it/cmd/server_start.rs index 41a10d966..9ec16e98f 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/server_start.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/server_start.rs @@ -413,15 +413,11 @@ fn concurrent_autostart_converges_on_one_shared_daemon_and_cleans_up() { temp_dir: &std::path::Path, config_path: &std::path::Path, ) -> std::process::Output { - std::process::Command::new(env!("CARGO_BIN_EXE_fabro")) - .current_dir(temp_dir) - .env("FABRO_TEST_IN_MEMORY_STORE", "1") - .env("NO_COLOR", "1") - .env("HOME", home_dir) + let mut cmd = std::process::Command::new(env!("CARGO_BIN_EXE_fabro")); + fabro_test::apply_test_isolation(&mut cmd, home_dir); + cmd.current_dir(temp_dir) .env("FABRO_HOME", home_dir.join(".fabro")) .env("FABRO_CONFIG", config_path) - .env("FABRO_NO_UPGRADE_CHECK", "true") - .env("FABRO_HTTP_PROXY_POLICY", "disabled") .args(["ps", "-a", "--json"]) .output() .expect("ps command should execute") diff --git a/lib/crates/fabro-test/src/lib.rs b/lib/crates/fabro-test/src/lib.rs index 092600a29..906756fa8 100644 --- a/lib/crates/fabro-test/src/lib.rs +++ b/lib/crates/fabro-test/src/lib.rs @@ -100,6 +100,31 @@ pub fn require_env(name: &str) -> Option { } } +/// Apply baseline environment isolation to a `Command` that spawns the +/// `fabro` binary (or a helper that will act like it). +/// +/// Starts from a cleared environment and re-populates only the variables +/// the harness needs. Credentials (`GITHUB_TOKEN`, `GH_TOKEN`, provider API +/// keys, `SESSION_SECRET`, `GITHUB_APP_*`) and ambient `FABRO_*` overrides +/// from the developer shell or CI runner are dropped, so tests that assert +/// on "no credentials" error paths behave the same on a laptop with +/// `gh auth login` active and on a CI runner with a minted +/// `GITHUB_TOKEN`. Tests that deliberately need a credential set it with a +/// subsequent `.env(...)` call, which survives the clear. +pub fn apply_test_isolation(cmd: &mut std::process::Command, home_dir: &Path) { + cmd.env_clear(); + if let Some(path) = std::env::var_os("PATH") { + cmd.env("PATH", path); + } + cmd.env("NO_COLOR", "1"); + cmd.env("HOME", home_dir); + cmd.env("FABRO_NO_UPGRADE_CHECK", "true") + .env("FABRO_HTTP_PROXY_POLICY", "disabled") + .env("FABRO_TELEMETRY", "off"); + cmd.env("FABRO_SERVER_MAX_CONCURRENT_RUNS", "64"); + cmd.env(TEST_IN_MEMORY_STORE_ENV, "1"); +} + /// A test context for running fabro CLI commands. /// /// Each context gets isolated home/temp directories. The storage directory is @@ -630,13 +655,10 @@ fn ensure_server_running(fabro_bin: &Path, server: &ServerPaths, config_path: &P let _ = std::fs::remove_file(server_record_path(&server.storage_dir)); let _ = std::fs::remove_file(&server.socket_path); - let output = std::process::Command::new(fabro_bin) - .env("NO_COLOR", "1") - .env("FABRO_NO_UPGRADE_CHECK", "true") - .env("FABRO_HTTP_PROXY_POLICY", "disabled") - .env("FABRO_SERVER_MAX_CONCURRENT_RUNS", "64") + let mut bootstrap = std::process::Command::new(fabro_bin); + apply_test_isolation(&mut bootstrap, &server.root); + let output = bootstrap .env("SESSION_SECRET", TEST_SESSION_SECRET) - .env(TEST_IN_MEMORY_STORE_ENV, "1") .env("FABRO_HOME", &server.root) .args(["server", "start"]) .arg("--storage-dir") @@ -950,26 +972,15 @@ impl TestContext { /// directory) so tests never accidentally interact with the real repo. /// Tests that need a specific working directory can override this with /// a subsequent `.current_dir(path)` call. + #[expect( + clippy::disallowed_methods, + reason = "Tests spawn the real fabro CLI synchronously; assert_cmd wraps the std Command we build here." + )] pub fn command(&self) -> Command { - let mut cmd = Command::new(&self.fabro_bin); - cmd.current_dir(&self.temp_dir); - // Scrub all inherited FABRO_* env so a developer running with - // e.g. FABRO_CONFIG=/some/path doesn't pollute child processes. - for (key, _) in std::env::vars_os() { - if let Some(s) = key.to_str() { - if s.starts_with("FABRO_") { - cmd.env_remove(&key); - } - } - } - cmd.env("NO_COLOR", "1"); - cmd.env("HOME", &self.home_dir); - cmd.env("FABRO_NO_UPGRADE_CHECK", "true") - .env("FABRO_HTTP_PROXY_POLICY", "disabled") - .env("FABRO_TELEMETRY", "off"); - cmd.env("FABRO_SERVER_MAX_CONCURRENT_RUNS", "64"); - cmd.env(TEST_IN_MEMORY_STORE_ENV, "1"); - cmd + let mut inner = std::process::Command::new(&self.fabro_bin); + apply_test_isolation(&mut inner, &self.home_dir); + inner.current_dir(&self.temp_dir); + Command::from_std(inner) } /// Build a `validate` subcommand. @@ -1808,6 +1819,48 @@ mod tests { }),); } + #[cfg(unix)] + #[test] + #[expect( + clippy::disallowed_methods, + reason = "Regression test: spawn /usr/bin/env synchronously and inspect stdout to assert the harness's env isolation." + )] + fn apply_test_isolation_strips_ambient_credentials() { + let _lock = env_lock().lock().expect("env lock poisoned"); + let _token = EnvGuard::set("GITHUB_TOKEN", Some("sentinel-should-not-leak")); + let _anthropic = EnvGuard::set("ANTHROPIC_API_KEY", Some("sentinel-also-should-not-leak")); + + let home = tempfile::tempdir().expect("temp home should be created"); + let mut cmd = std::process::Command::new("/usr/bin/env"); + apply_test_isolation(&mut cmd, home.path()); + let output = cmd.output().expect("/usr/bin/env should execute"); + assert!(output.status.success(), "env exited non-zero"); + let env_output = String::from_utf8(output.stdout).expect("env stdout should be UTF-8"); + + assert!( + !env_output + .lines() + .any(|line| line.starts_with("GITHUB_TOKEN=")), + "GITHUB_TOKEN leaked into child env:\n{env_output}" + ); + assert!( + !env_output + .lines() + .any(|line| line.starts_with("ANTHROPIC_API_KEY=")), + "ANTHROPIC_API_KEY leaked into child env:\n{env_output}" + ); + assert!( + env_output.lines().any(|line| line.starts_with("PATH=")), + "PATH should be preserved so subprocess can find git and friends" + ); + assert!( + env_output + .lines() + .any(|line| line.starts_with("FABRO_NO_UPGRADE_CHECK=true")), + "harness-set env vars should still be present" + ); + } + #[test] fn twin_scenario_builder_matches_admin_contract() { let scenario = TwinScenario::responses("gpt-5.4-mini")