scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-09-11 22:53:00 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

test(harness): scrub ambient creds from spawned fabro CLI

Browse source 
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.
This commit is contained in:
Bryan Helmkamp 2026-04-18 01:55:01 -04:00
parent eb3c6d5ce3
commit cf80fe567a
No known key found for this signature in database
5 changed files with 84 additions and 49 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
lib/crates/fabro-cli/tests/it/cmd/attach.rs
View file

@ -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());

5
lib/crates/fabro-cli/tests/it/cmd/render_graph.rs
View file

@ -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
}

7
lib/crates/fabro-cli/tests/it/cmd/runner.rs
View file

@ -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);
}

10
lib/crates/fabro-cli/tests/it/cmd/server_start.rs
View file

@ -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")

103
lib/crates/fabro-test/src/lib.rs
View file

@ -100,6 +100,31 @@ pub fn require_env(name: &str) -> Option<String> {
}
}
/// 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")