From 077469d0c6002cb1cf444e5f3c6e8b8de39bfe37 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Apr 2026 15:10:58 -0400 Subject: [PATCH 1/3] 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`]. From b5b67226f176a4e831e6eb2ae5a712b0ad82540d Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Apr 2026 15:20:58 -0400 Subject: [PATCH 2/3] ci: restore install.rs server-symbol allowlist and worker-token scrub exemption Both Boundary checks have been red on main for multiple commits: - check-boundary.sh: install.rs reintroduced direct use of fabro_config::ServerSettings::from_layer in 93b6577cd but was dropped from server_symbol_allowlist in bb0d05be2. Re-add it. - check-env-mutation.sh: the worker FABRO_WORKER_TOKEN scrub added in 077469d0c is documented as the approved pattern in docs-internal/server-secrets-strategy.md but was missing from the allowlist. Add the exact line. Co-Authored-By: Claude Opus 4.7 (1M context) --- bin/dev/check-boundary.sh | 1 + bin/dev/check-env-mutation.sh | 1 + 2 files changed, 2 insertions(+) diff --git a/bin/dev/check-boundary.sh b/bin/dev/check-boundary.sh index 974dc4442..d855e2384 100755 --- a/bin/dev/check-boundary.sh +++ b/bin/dev/check-boundary.sh @@ -5,6 +5,7 @@ cd "$(dirname "$0")/../.." server_symbol_allowlist=( "lib/crates/fabro-cli/src/local_server.rs" + "lib/crates/fabro-cli/src/commands/install.rs" "lib/crates/fabro-cli/src/commands/run/runner.rs" "lib/crates/fabro-cli/src/commands/pr/mod.rs" "lib/crates/fabro-cli/src/commands/pr/create.rs" diff --git a/bin/dev/check-env-mutation.sh b/bin/dev/check-env-mutation.sh index 6b95cb8f7..dff4ac811 100755 --- a/bin/dev/check-env-mutation.sh +++ b/bin/dev/check-env-mutation.sh @@ -21,6 +21,7 @@ while IFS= read -r match; do case "$path:$line" in "lib/crates/fabro-telemetry/src/spawn.rs:std::env::set_var(key, value);" | \ "lib/crates/fabro-telemetry/src/spawn.rs:std::env::remove_var(key);" | \ + 'lib/crates/fabro-cli/src/main.rs:std::env::remove_var("FABRO_WORKER_TOKEN");' | \ 'lib/crates/fabro-server/src/install.rs:std::env::set_var("FABRO_TEST_IN_MEMORY_STORE", "1");') continue ;; From dda6f44d1eadc97debefc2dd262ce02423bdc170 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Apr 2026 15:24:33 -0400 Subject: [PATCH 3/3] ci: drop check-env-mutation.sh, rely on clippy disallowed_methods clippy.toml already bans std::env::{set_var,remove_var} via disallowed_methods, and every existing call site carries a scoped #[expect(clippy::disallowed_methods, reason = "...")]. The shell grep is redundant and forced a second, less granular allowlist. Also update server-secrets-strategy.md to describe clippy as the enforcement mechanism. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/rust.yml | 1 - bin/dev/check-env-mutation.sh | 44 ------------------------ docs-internal/server-secrets-strategy.md | 2 +- 3 files changed, 1 insertion(+), 46 deletions(-) delete mode 100755 bin/dev/check-env-mutation.sh diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index d47092a01..ac96ed440 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -47,7 +47,6 @@ jobs: with: persist-credentials: false - run: bin/dev/check-boundary.sh - - run: bin/dev/check-env-mutation.sh fmt: name: Format diff --git a/bin/dev/check-env-mutation.sh b/bin/dev/check-env-mutation.sh deleted file mode 100755 index dff4ac811..000000000 --- a/bin/dev/check-env-mutation.sh +++ /dev/null @@ -1,44 +0,0 @@ -#!/usr/bin/env bash -set -euo pipefail - -cd "$(dirname "$0")/../.." - -if command -v rg >/dev/null 2>&1; then - matches=$(rg -n 'std::env::(set_var|remove_var)' --glob '*.rs' || true) -else - matches=$(grep -R -n -E 'std::env::(set_var|remove_var)' . --include='*.rs' --exclude-dir=target --exclude-dir=.git || true) -fi - -fail=0 -while IFS= read -r match; do - [[ -z "$match" ]] && continue - - path=${match%%:*} - rest=${match#*:} - line=${rest#*:} - line=${line#"${line%%[![:space:]]*}"} - - case "$path:$line" in - "lib/crates/fabro-telemetry/src/spawn.rs:std::env::set_var(key, value);" | \ - "lib/crates/fabro-telemetry/src/spawn.rs:std::env::remove_var(key);" | \ - 'lib/crates/fabro-cli/src/main.rs:std::env::remove_var("FABRO_WORKER_TOKEN");' | \ - 'lib/crates/fabro-server/src/install.rs:std::env::set_var("FABRO_TEST_IN_MEMORY_STORE", "1");') - continue - ;; - esac - - echo "process env mutation check failed: $match" >&2 - fail=1 -done <<< "$matches" - -if [[ $fail -ne 0 ]]; then - cat >&2 <<'EOF' - -Do not mutate process-wide env with std::env::set_var/remove_var. -Inject env at construction time or on child-process Command values instead. -See docs-internal/server-secrets-strategy.md. -EOF - exit 1 -fi - -echo "Process env mutation checks passed." diff --git a/docs-internal/server-secrets-strategy.md b/docs-internal/server-secrets-strategy.md index 17ee5e724..1c092c3e5 100644 --- a/docs-internal/server-secrets-strategy.md +++ b/docs-internal/server-secrets-strategy.md @@ -9,7 +9,7 @@ This document defines how Fabro handles server-level secrets. - Resolution is snapshot-based: env and file are read once at construction, then treated as immutable for the life of the process. - `process env` wins over `server.env` on conflicts. - `fabro server start` never generates secrets. Missing required secrets are a startup error. -- `std::env::set_var` and `std::env::remove_var` are banned workspace-wide. Tests are not exempt. CI enforces this with `bin/dev/check-env-mutation.sh` so broad clippy suppressions cannot bypass it. +- `std::env::set_var` and `std::env::remove_var` are banned workspace-wide. Tests are not exempt. Enforced by clippy via `disallowed_methods` in `clippy.toml`; intentional exceptions must be annotated with a scoped `#[expect(clippy::disallowed_methods, reason = "...")]` at the call site. ## Active Server Secrets