refactor(auth): scrub FABRO_WORKER_TOKEN from worker env at startup

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) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-04-23 15:10:58 -04:00
parent fbb924b9c4
commit 077469d0c6
No known key found for this signature in database
10 changed files with 53 additions and 65 deletions

1
Cargo.lock generated
View file

@ -1983,7 +1983,6 @@ dependencies = [
"fabro-github",
"fabro-proc",
"fabro-types",
"fabro-util",
"futures",
"git2",
"glob",

View file

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

View file

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

View file

@ -59,19 +59,13 @@ pub(crate) async fn execute(
storage_dir: Option<PathBuf>,
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::<fabro_client::ServerTarget>()?;
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));

View file

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

View file

@ -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
}
"#,

View file

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

View file

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

View file

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

View file

@ -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`].