Merge remote-tracking branch 'origin/main' into fix/dependabot-openssl-astro

.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

Cargo.lock

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

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"

bin/dev/check-env-mutation.sh

@@ -1,43 +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-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."

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

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

lib/crates/fabro-cli/src/commands/run/runner.rs

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

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

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

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::<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;

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

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

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