From 077469d0c6002cb1cf444e5f3c6e8b8de39bfe37 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Apr 2026 15:10:58 -0400 Subject: [PATCH] refactor(auth): scrub FABRO_WORKER_TOKEN from worker env at startup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The worker subprocess is spawned with env_clear+allowlist by the server, so the only sensitive value in its env is FABRO_WORKER_TOKEN itself. Read the token and remove_var it from the process env in main() before Tokio starts worker threads, then thread it explicitly through runner::execute(&str). Every descendant (hooks, local sandbox, devcontainer initializeCommand, MCP stdio, etc.) now inherits a worker env with no bearer in it, so an unscrubbed spawn site cannot leak the token. This makes the prior denylist scrub in fabro-hooks and fabro-sandbox redundant — delete it and the shared WORKER_SECRET_ENV_DENYLIST constant. The sandbox keeps its _api_key/_secret/ _token/_password/_credential suffix heuristic for user-supplied env_vars hygiene. Extend the server-dispatched-worker env-leak integration test to also assert a Bash stage running in the worker does not observe FABRO_WORKER_TOKEN. Co-Authored-By: Claude Opus 4.7 (1M context) --- Cargo.lock | 1 - docs-internal/server-secrets-strategy.md | 1 + lib/crates/fabro-cli/src/commands/run/mod.rs | 25 ++++++++++++++--- .../fabro-cli/src/commands/run/runner.rs | 12 +++------ lib/crates/fabro-cli/src/main.rs | 27 ++++++++++++++++--- lib/crates/fabro-cli/tests/it/cmd/runner.rs | 3 ++- lib/crates/fabro-hooks/src/executor.rs | 26 +----------------- lib/crates/fabro-sandbox/Cargo.toml | 1 - lib/crates/fabro-sandbox/src/local.rs | 7 ----- lib/crates/fabro-util/src/env.rs | 15 ----------- 10 files changed, 53 insertions(+), 65 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 12f4c5e52..f3ce18892 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1983,7 +1983,6 @@ dependencies = [ "fabro-github", "fabro-proc", "fabro-types", - "fabro-util", "futures", "git2", "glob", diff --git a/docs-internal/server-secrets-strategy.md b/docs-internal/server-secrets-strategy.md index 9a9de5612..17ee5e724 100644 --- a/docs-internal/server-secrets-strategy.md +++ b/docs-internal/server-secrets-strategy.md @@ -47,6 +47,7 @@ There is no compatibility layer for removed secrets and no startup-time secret g - Worker and render-graph subprocesses start from `env_clear()` and re-add only explicit allowlisted variables. - Authority-bearing values are re-injected intentionally. For worker subprocesses this is `FABRO_WORKER_TOKEN`, not user auth state such as `FABRO_DEV_TOKEN` or `auth.json`. +- The worker reads `FABRO_WORKER_TOKEN` from its env at startup (in `main()` before Tokio initializes) and immediately calls `std::env::remove_var` to scrub it. The token then flows through function arguments to `runner::execute`. Every descendant process (hooks, sandbox commands, devcontainer setup, MCP stdio, etc.) therefore inherits a worker env that no longer contains the bearer, so an unscrubbed spawn site cannot leak it. - The daemon child inherits the parent env unchanged except for output-format hygiene (`FABRO_JSON` removal). ## Tests diff --git a/lib/crates/fabro-cli/src/commands/run/mod.rs b/lib/crates/fabro-cli/src/commands/run/mod.rs index daea20516..b8451b004 100644 --- a/lib/crates/fabro-cli/src/commands/run/mod.rs +++ b/lib/crates/fabro-cli/src/commands/run/mod.rs @@ -1,4 +1,4 @@ -use anyhow::Result; +use anyhow::{Result, anyhow}; use fabro_util::terminal::Styles; use crate::args::{AttachArgs, RunCommands, RunWorkerArgs, StartArgs}; @@ -23,7 +23,11 @@ pub(crate) mod ssh; pub(crate) mod start; pub(crate) mod wait; -pub(crate) async fn dispatch(cmd: RunCommands, base_ctx: &CommandContext) -> Result<()> { +pub(crate) async fn dispatch( + cmd: RunCommands, + base_ctx: &CommandContext, + worker_token: Option, +) -> Result<()> { let printer = base_ctx.printer(); match cmd { @@ -76,7 +80,22 @@ pub(crate) async fn dispatch(cmd: RunCommands, base_ctx: &CommandContext) -> Res run_dir, run_id, mode, - }) => Box::pin(runner::execute(run_id, server, storage_dir, run_dir, mode)).await, + }) => { + let worker_token = worker_token + .filter(|token| !token.trim().is_empty()) + .ok_or_else(|| { + anyhow!("FABRO_WORKER_TOKEN is required for worker subprocess auth") + })?; + Box::pin(runner::execute( + run_id, + server, + storage_dir, + run_dir, + mode, + &worker_token, + )) + .await + } RunCommands::Diff(args) => diff::run(args, base_ctx).await, RunCommands::Logs(args) => { let styles = Styles::detect_stdout(); diff --git a/lib/crates/fabro-cli/src/commands/run/runner.rs b/lib/crates/fabro-cli/src/commands/run/runner.rs index dada255ec..9c3c75d9d 100644 --- a/lib/crates/fabro-cli/src/commands/run/runner.rs +++ b/lib/crates/fabro-cli/src/commands/run/runner.rs @@ -59,19 +59,13 @@ pub(crate) async fn execute( storage_dir: Option, run_dir: PathBuf, mode: RunWorkerMode, + worker_token: &str, ) -> Result<()> { let _ = fabro_proc::title_init(); set_worker_title(&run_id, initial_worker_title_phase(mode)); - let worker_token = std::env::var("FABRO_WORKER_TOKEN") - .map_err(|_| anyhow!("FABRO_WORKER_TOKEN is required for worker subprocess auth"))?; - if worker_token.trim().is_empty() { - return Err(anyhow!( - "FABRO_WORKER_TOKEN is required for worker subprocess auth" - )); - } let target = server.parse::()?; - let client = server_client::connect_server_target_with_bearer(&target, &worker_token).await?; + let client = server_client::connect_server_target_with_bearer(&target, worker_token).await?; let run_store = HttpRunStore::connect(run_id, client.clone_for_reuse()).await?; let run_state = run_store .state() @@ -84,7 +78,7 @@ pub(crate) async fn execute( let artifact_sink = Some(ArtifactSink::Uploader(build_artifact_uploader( run_id, client.clone_for_reuse(), - worker_token, + worker_token.to_owned(), ))); let interviewer = Arc::new(ControlInterviewer::new()); let cancel_token = Arc::new(AtomicBool::new(false)); diff --git a/lib/crates/fabro-cli/src/main.rs b/lib/crates/fabro-cli/src/main.rs index d22813e2e..d42f59a58 100644 --- a/lib/crates/fabro-cli/src/main.rs +++ b/lib/crates/fabro-cli/src/main.rs @@ -72,12 +72,33 @@ async fn main() { std::process::exit(commands::render_graph::execute()); } + // Capture the worker bearer token immediately and scrub it from the process + // env before any subprocess can be spawned. Every descendant of the worker + // (hooks, sandbox commands, devcontainer setup, MCP stdio, etc.) therefore + // inherits a process env that no longer contains FABRO_WORKER_TOKEN, so an + // unscrubbed spawn site cannot leak it. The token flows to `runner::execute` + // through an explicit function argument instead of the environment. + let worker_token = if subcommand == Some("__run-worker") { + let token = std::env::var("FABRO_WORKER_TOKEN").ok(); + #[expect( + clippy::disallowed_methods, + reason = "Scrub the worker bearer from this process's env before any \ + child process is spawned, so no descendant can inherit it." + )] + { + std::env::remove_var("FABRO_WORKER_TOKEN"); + } + token + } else { + None + }; + tel_panic::install_panic_hook(); fabro_telemetry::init_cli(); let start = std::time::Instant::now(); - let (command_name, result) = Box::pin(main_inner()).await; + let (command_name, result) = Box::pin(main_inner(worker_token)).await; let duration_ms = u64::try_from(start.elapsed().as_millis()).unwrap_or(u64::MAX); let exit_code = result.as_ref().err().map_or(0, exit::exit_code_for); @@ -145,7 +166,7 @@ async fn main() { } } -async fn main_inner() -> (String, Result<()>) { +async fn main_inner(worker_token: Option) -> (String, Result<()>) { let _ = default_provider().install_default(); let cli = Cli::parse(); @@ -206,7 +227,7 @@ async fn main_inner() -> (String, Result<()>) { commands::exec::execute(args, &base_ctx).await?; } Commands::RunCmd(cmd) => { - Box::pin(commands::run::dispatch(cmd, &base_ctx)).await?; + Box::pin(commands::run::dispatch(cmd, &base_ctx, worker_token)).await?; } Commands::Preflight(args) => { commands::preflight::execute(args, &base_ctx).await?; diff --git a/lib/crates/fabro-cli/tests/it/cmd/runner.rs b/lib/crates/fabro-cli/tests/it/cmd/runner.rs index 36e5e4e60..32799e105 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/runner.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/runner.rs @@ -184,6 +184,7 @@ fn assert_no_worker_env_leak(scope: &str, content: &str) { for needle in [ "MY_API_TOKEN=", "NEW_RELIC_LICENSE_KEY=", + "FABRO_WORKER_TOKEN=", LEAKED_WORKER_PARENT_TOKEN, LEAKED_NEW_RELIC_LICENSE, ] { @@ -522,7 +523,7 @@ methods = ["dev-token"] graph [goal="Verify worker subprocess env isolation", default_max_retries=0] start [shape=Mdiamond, label="Start"] exit [shape=Msquare, label="Exit"] - probe [shape=parallelogram, label="Probe", script="echo probe-ran; for key in $(printf 'MY%s NEW%s' '_API_TOKEN' '_RELIC_LICENSE_KEY'); do value=$(printenv \"$key\" || true); if [ -n \"$value\" ]; then echo \"$key=$value\"; fi; done"] + probe [shape=parallelogram, label="Probe", script="echo probe-ran; for key in $(printf 'MY%s NEW%s FABRO%s' '_API_TOKEN' '_RELIC_LICENSE_KEY' '_WORKER_TOKEN'); do value=$(printenv \"$key\" || true); if [ -n \"$value\" ]; then echo \"$key=$value\"; fi; done"] start -> probe -> exit } "#, diff --git a/lib/crates/fabro-hooks/src/executor.rs b/lib/crates/fabro-hooks/src/executor.rs index e9d51aa3e..e69ad09ae 100644 --- a/lib/crates/fabro-hooks/src/executor.rs +++ b/lib/crates/fabro-hooks/src/executor.rs @@ -13,7 +13,7 @@ use fabro_llm::generate::{GenerateParams, generate_object}; use fabro_llm::types::{Message, Request, ToolResult}; use fabro_template::{TemplateContext, render as render_template}; use fabro_types::settings::InterpString; -use fabro_util::env::{Env, SystemEnv, WORKER_SECRET_ENV_DENYLIST}; +use fabro_util::env::{Env, SystemEnv}; use tokio::process::Command as TokioCommand; use tokio::time::timeout as tokio_timeout; use tokio_util::sync::CancellationToken; @@ -77,12 +77,6 @@ where pub struct HookExecutorImpl; impl HookExecutorImpl { - fn scrub_worker_secret_env(cmd: &mut TokioCommand) { - for key in WORKER_SECRET_ENV_DENYLIST { - cmd.env_remove(key); - } - } - /// Parse a hook decision from JSON stdout and exit code. fn parse_decision(exit_code: i32, stdout: &str) -> HookDecision { if exit_code == 0 { @@ -195,7 +189,6 @@ impl HookExecutorImpl { if let Some(wd) = work_dir { cmd.current_dir(wd); } - Self::scrub_worker_secret_env(&mut cmd); for (k, v) in &env_vars { cmd.env(k, v); } @@ -847,23 +840,6 @@ mod tests { assert_eq!(result.decision, HookDecision::Proceed); } - #[test] - fn host_command_scrubs_worker_secret_env() { - let mut cmd = TokioCommand::new("sh"); - HookExecutorImpl::scrub_worker_secret_env(&mut cmd); - - let removed = cmd - .as_std() - .get_envs() - .filter(|(_, value)| value.is_none()) - .map(|(name, _)| name.to_string_lossy().into_owned()) - .collect::>(); - - for key in WORKER_SECRET_ENV_DENYLIST { - assert!(removed.iter().any(|name| name == key)); - } - } - #[tokio::test] async fn no_hook_type_blocks() { let executor = HookExecutorImpl; diff --git a/lib/crates/fabro-sandbox/Cargo.toml b/lib/crates/fabro-sandbox/Cargo.toml index 878b79ef8..5b03e9039 100644 --- a/lib/crates/fabro-sandbox/Cargo.toml +++ b/lib/crates/fabro-sandbox/Cargo.toml @@ -30,7 +30,6 @@ strum.workspace = true tracing.workspace = true base64.workspace = true fabro-proc = { path = "../fabro-proc" } -fabro-util = { path = "../fabro-util" } shlex = "1" # local diff --git a/lib/crates/fabro-sandbox/src/local.rs b/lib/crates/fabro-sandbox/src/local.rs index c7892ef3b..af6d54ddd 100644 --- a/lib/crates/fabro-sandbox/src/local.rs +++ b/lib/crates/fabro-sandbox/src/local.rs @@ -2,7 +2,6 @@ use std::path::{Path, PathBuf}; use std::time::Instant; use async_trait::async_trait; -use fabro_util::env::WORKER_SECRET_ENV_DENYLIST; use tokio::io::AsyncReadExt; use tokio::process::{Child, Command}; use tokio::task::spawn_blocking; @@ -58,9 +57,6 @@ impl LocalSandbox { if Self::ENV_SAFELIST.contains(&key) { return false; } - if WORKER_SECRET_ENV_DENYLIST.contains(&key) { - return true; - } let lower = key.to_lowercase(); lower.ends_with("_api_key") || lower.ends_with("_secret") @@ -740,9 +736,6 @@ mod tests { assert!(LocalSandbox::should_filter_env_var("MY_CREDENTIAL")); assert!(LocalSandbox::should_filter_env_var("FABRO_WORKER_TOKEN")); assert!(LocalSandbox::should_filter_env_var("SESSION_SECRET")); - assert!(LocalSandbox::should_filter_env_var( - "GITHUB_APP_PRIVATE_KEY" - )); // Case insensitive assert!(LocalSandbox::should_filter_env_var("my_api_key")); assert!(LocalSandbox::should_filter_env_var("Some_Secret")); diff --git a/lib/crates/fabro-util/src/env.rs b/lib/crates/fabro-util/src/env.rs index 0b9693727..e3109fed3 100644 --- a/lib/crates/fabro-util/src/env.rs +++ b/lib/crates/fabro-util/src/env.rs @@ -1,18 +1,3 @@ -/// Server-managed secret env vars that must never leak into subprocesses -/// (hook executors, local sandbox). A suffix filter (`_secret`, `_token`, -/// `_api_key`, `_password`, `_credential`) catches many secrets by convention, -/// but these names don't match those suffixes and are explicitly named to -/// eliminate ambiguity when the list is inspected. -pub const WORKER_SECRET_ENV_DENYLIST: &[&str] = &[ - "FABRO_WORKER_TOKEN", - "SESSION_SECRET", - "FABRO_JWT_PRIVATE_KEY", - "FABRO_JWT_PUBLIC_KEY", - "GITHUB_APP_PRIVATE_KEY", - "GITHUB_APP_CLIENT_SECRET", - "GITHUB_APP_WEBHOOK_SECRET", -]; - /// Abstraction over environment variable lookup. /// /// Production code uses [`SystemEnv`] which delegates to [`std::env::var`].