From 56667c17a8c8c547f67cec24850e44d9e077bec1 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Apr 2026 07:51:28 -0400 Subject: [PATCH] fix(server): cover secret boundary enforcement --- .github/workflows/rust.yml | 1 + bin/dev/check-env-mutation.sh | 42 ++++ docs-internal/server-secrets-strategy.md | 2 +- docs/changelog/2026-03-03.mdx | 1 - .../fabro-cli/src/commands/server/start.rs | 3 + lib/crates/fabro-cli/tests/it/cmd/runner.rs | 136 ++++++++++- .../fabro-cli/tests/it/cmd/server_start.rs | 218 +++++++++++++++++- lib/crates/fabro-server/src/server_secrets.rs | 11 +- lib/crates/fabro-server/src/startup.rs | 2 +- 9 files changed, 401 insertions(+), 15 deletions(-) create mode 100755 bin/dev/check-env-mutation.sh diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index ac96ed440..d47092a01 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -47,6 +47,7 @@ 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 new file mode 100755 index 000000000..5387a0052 --- /dev/null +++ b/bin/dev/check-env-mutation.sh @@ -0,0 +1,42 @@ +#!/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);") + 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 dd0df73f5..804795484 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. +- `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. ## Active Server Secrets diff --git a/docs/changelog/2026-03-03.mdx b/docs/changelog/2026-03-03.mdx index 2b9137aea..1fc0f648c 100644 --- a/docs/changelog/2026-03-03.mdx +++ b/docs/changelog/2026-03-03.mdx @@ -47,5 +47,4 @@ If something is misconfigured, `fabro doctor` tells you exactly what's wrong and - New indicatif-based progress display for `fabro run start` shows real-time stage progress, tool calls, model names, and timing - Mercury provider updated to `mercury-2`; estimated output speed (tok/s) added to `fabro model list` - Run defaults in `server.toml` are inherited by all workflow runs, so you don't have to repeat sandbox, model, or concurrency settings -- `FABRO_JWT_PUBLIC_KEY` and `FABRO_JWT_PRIVATE_KEY` accept base64-encoded PEM strings for containerized deployments diff --git a/lib/crates/fabro-cli/src/commands/server/start.rs b/lib/crates/fabro-cli/src/commands/server/start.rs index f2b47ab2b..937baa296 100644 --- a/lib/crates/fabro-cli/src/commands/server/start.rs +++ b/lib/crates/fabro-cli/src/commands/server/start.rs @@ -234,6 +234,9 @@ async fn execute_foreground( styles: &'static Styles, _printer: Printer, ) -> Result<()> { + // Foreground mode validates inside serve_command after lock/log setup so + // operator-visible startup failures use the same path as normal foreground + // boot. super::foreground::serve_with_daemon_record(serve_args, bind, storage_dir, styles).await } diff --git a/lib/crates/fabro-cli/tests/it/cmd/runner.rs b/lib/crates/fabro-cli/tests/it/cmd/runner.rs index 6cf0ad28c..337c5b213 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/runner.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/runner.rs @@ -13,16 +13,18 @@ use std::time::{Duration, Instant}; use fabro_store::EventEnvelope; use fabro_test::{assert_reqwest_status, expect_reqwest_json, fabro_snapshot, test_context}; -use fabro_types::{EventBody, FailureReason, RunEvent}; +use fabro_types::{EventBody, FailureReason, RunEvent, StageId}; use httpmock::MockServer; use super::support::{ - local_dev_token, output_stderr, run_events, run_state, server_endpoint, server_target, - wait_for_event_names, wait_for_status, write_gated_workflow, + find_run_dir, local_dev_token, output_stderr, run_events, run_state, server_endpoint, + server_target, wait_for_event_names, wait_for_status, write_gated_workflow, }; use crate::support::{fabro_json_snapshot, unique_run_id}; const SHARED_DAEMON_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(30); +const LEAKED_WORKER_PARENT_TOKEN: &str = "leak-worker-parent-token"; +const LEAKED_NEW_RELIC_LICENSE: &str = "leak-new-relic-license"; fn auth_context() -> fabro_test::TestContext { let context = test_context!(); @@ -126,6 +128,20 @@ fn worker_command(context: &fabro_test::TestContext) -> assert_cmd::Command { cmd } +fn assert_no_worker_env_leak(scope: &str, content: &str) { + for needle in [ + "MY_API_TOKEN=", + "NEW_RELIC_LICENSE_KEY=", + LEAKED_WORKER_PARENT_TOKEN, + LEAKED_NEW_RELIC_LICENSE, + ] { + assert!( + !content.contains(needle), + "{scope} leaked {needle:?}:\n{content}" + ); + } +} + async fn wait_for_server_question( client: &fabro_http::HttpClient, base_url: &str, @@ -373,6 +389,120 @@ digraph DetachedStoreOnly { assert_worker_succeeded(&run_dir, &output); } +#[test] +fn server_dispatched_worker_does_not_inherit_parent_secret_env() { + let mut context = test_context!(); + let server_root = tempfile::tempdir_in("/tmp").unwrap(); + let storage_dir = server_root.path().join("storage"); + let socket_path = server_root.path().join("fabro.sock"); + let config_path = server_root.path().join("settings.toml"); + context.manage_storage_dir(&storage_dir); + std::fs::write( + &config_path, + format!( + r#"_version = 1 + +[server.storage] +root = "{}" + +[server.auth] +methods = ["dev-token"] +"#, + storage_dir.display() + ), + ) + .expect("writing leak-probe server settings"); + + let start_output = context + .command() + .env("MY_API_TOKEN", LEAKED_WORKER_PARENT_TOKEN) + .env("NEW_RELIC_LICENSE_KEY", LEAKED_NEW_RELIC_LICENSE) + .args(["server", "start"]) + .arg("--storage-dir") + .arg(&storage_dir) + .arg("--bind") + .arg(&socket_path) + .arg("--config") + .arg(&config_path) + .output() + .expect("server start should execute"); + assert!( + start_output.status.success(), + "server start failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&start_output.stdout), + String::from_utf8_lossy(&start_output.stderr) + ); + + let workflow_path = context.temp_dir.join("worker-leak-probe.fabro"); + std::fs::write( + &workflow_path, + r#"digraph WorkerLeakProbe { + 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"] + start -> probe -> exit +} +"#, + ) + .expect("writing leak-probe workflow"); + + let run_id = unique_run_id(); + let dev_token = local_dev_token(&storage_dir).expect("managed server should have a dev token"); + let run_output = context + .run_cmd() + .env("FABRO_DEV_TOKEN", dev_token) + .args([ + "--server", + socket_path.to_str().expect("socket path should be UTF-8"), + "--run-id", + run_id.as_str(), + "--detach", + "--auto-approve", + "--no-retro", + "--sandbox", + "local", + workflow_path + .to_str() + .expect("workflow path should be UTF-8"), + ]) + .output() + .expect("detached leak-probe run should execute"); + assert!( + run_output.status.success(), + "detached run failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&run_output.stdout), + String::from_utf8_lossy(&run_output.stderr) + ); + + let run_dir = find_run_dir(&storage_dir, &run_id).expect("leak-probe run dir should exist"); + wait_for_status(&run_dir, &["succeeded"]); + + let state = run_state(&run_dir); + let _probe = state + .node(&StageId::new("probe", 1)) + .expect("probe node state should exist"); + let stdout = state + .checkpoint + .as_ref() + .and_then(|checkpoint| checkpoint.context_values.get("command.output")) + .and_then(serde_json::Value::as_str) + .expect("probe command output should exist"); + assert!( + stdout.contains("probe-ran"), + "probe stage should have executed, got stdout:\n{stdout}" + ); + assert_no_worker_env_leak("probe stdout", stdout); + assert_no_worker_env_leak( + "run state", + &serde_json::to_string(&state).expect("run state should serialize"), + ); + + let server_log = + std::fs::read_to_string(storage_dir.join("logs/server.log")).unwrap_or_default(); + assert_no_worker_env_leak("server log", &server_log); +} + #[test] fn runner_resume_rejects_completed_run_without_mutating_it() { let context = auth_context(); diff --git a/lib/crates/fabro-cli/tests/it/cmd/server_start.rs b/lib/crates/fabro-cli/tests/it/cmd/server_start.rs index 7c8df950b..b5c54475b 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/server_start.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/server_start.rs @@ -14,8 +14,8 @@ use std::time::{Duration, Instant}; use fabro_config::{Storage, envfile}; use fabro_test::{ - apply_test_isolation, fabro_snapshot, isolated_storage_dir, server_log_files, test_context, - wait_for_log_line, wait_for_path, + TestContext, apply_test_isolation, fabro_snapshot, isolated_storage_dir, server_log_files, + test_context, wait_for_log_line, wait_for_path, }; use fabro_util::dev_token; @@ -43,6 +43,106 @@ fn provision_dev_token_auth(home_dir: &std::path::Path, storage_dir: &std::path: .expect("writing home dev-token"); } +#[derive(Clone, Copy, Debug)] +enum ServerStartMode { + Foreground, + Daemon, +} + +impl ServerStartMode { + const ALL: [Self; 2] = [Self::Foreground, Self::Daemon]; + + fn name(self) -> &'static str { + match self { + Self::Foreground => "foreground", + Self::Daemon => "daemon", + } + } + + fn add_args(self, cmd: &mut assert_cmd::Command) { + if matches!(self, Self::Foreground) { + cmd.arg("--foreground"); + } + } +} + +struct StartupFailureCase { + name: &'static str, + settings: &'static str, + server_env: &'static [(&'static str, &'static str)], + expected_error: &'static str, +} + +fn run_startup_failure(context: &TestContext, mode: ServerStartMode, case: &StartupFailureCase) { + let storage_root = isolated_storage_dir(); + let storage_dir = storage_root + .path() + .join(format!("{}-{}", case.name, mode.name())); + let socket_path = storage_root + .path() + .join(format!("{}-{}.sock", case.name, mode.name())); + let config_dir = tempfile::tempdir_in("/tmp").expect("creating startup failure config dir"); + let config_path = config_dir.path().join("settings.toml"); + std::fs::write(&config_path, case.settings).expect("writing startup failure settings"); + if !case.server_env.is_empty() { + envfile::merge_env_file( + &Storage::new(&storage_dir).runtime_directory().env_path(), + case.server_env.iter().copied(), + ) + .expect("writing startup failure server.env"); + } + + let mut cmd = context.command(); + cmd.args(["server", "start"]); + mode.add_args(&mut cmd); + cmd.arg("--storage-dir") + .arg(&storage_dir) + .arg("--bind") + .arg(&socket_path) + .arg("--config") + .arg(&config_path); + let output = cmd + .output() + .expect("server start failure command should run"); + + assert!( + !output.status.success(), + "server start should reject {} in {} mode\nstdout:\n{}\nstderr:\n{}", + case.name, + mode.name(), + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert!( + output.stdout.is_empty(), + "server start rejection should not write stdout for {} in {} mode:\n{}", + case.name, + mode.name(), + String::from_utf8_lossy(&output.stdout) + ); + assert_eq!( + String::from_utf8_lossy(&output.stderr), + format!("error: {}\n", case.expected_error), + "unexpected stderr for {} in {} mode", + case.name, + mode.name() + ); + + let log_path = storage_dir.join("logs/server.log"); + match mode { + ServerStartMode::Foreground => assert!( + log_path.exists(), + "foreground validation intentionally runs after log bootstrap for {}", + case.name + ), + ServerStartMode::Daemon => assert!( + !log_path.exists(), + "daemon validation should fail before creating server.log for {}", + case.name + ), + } +} + #[test] fn help() { let context = test_context!(); @@ -101,6 +201,120 @@ fn help() { "); } +#[test] +fn start_rejects_invalid_startup_configuration_in_foreground_and_daemon() { + const DEV_TOKEN_SETTINGS: &str = r#"_version = 1 + +[server.auth] +methods = ["dev-token"] +"#; + const GITHUB_SETTINGS: &str = r#"_version = 1 + +[server.web] +enabled = true + +[server.auth] +methods = ["github"] + +[server.auth.github] +allowed_usernames = ["octocat"] + +[server.integrations.github] +client_id = "Iv1.testclient" +"#; + const GITHUB_WITHOUT_CLIENT_ID_SETTINGS: &str = r#"_version = 1 + +[server.web] +enabled = true + +[server.auth] +methods = ["github"] + +[server.auth.github] +allowed_usernames = ["octocat"] +"#; + const GITHUB_WEB_DISABLED_SETTINGS: &str = r#"_version = 1 + +[server.web] +enabled = false + +[server.auth] +methods = ["github"] + +[server.auth.github] +allowed_usernames = ["octocat"] + +[server.integrations.github] +client_id = "Iv1.testclient" +"#; + const EMPTY_AUTH_METHODS_SETTINGS: &str = r"_version = 1 + +[server.auth] +methods = [] +"; + + let context = test_context!(); + let cases = [ + StartupFailureCase { + name: "missing-session-secret", + settings: DEV_TOKEN_SETTINGS, + server_env: &[("FABRO_DEV_TOKEN", TEST_DEV_TOKEN)], + expected_error: "Fabro server refuses to start: auth is configured but SESSION_SECRET is not set.", + }, + StartupFailureCase { + name: "missing-dev-token", + settings: DEV_TOKEN_SETTINGS, + server_env: &[("SESSION_SECRET", TEST_SESSION_SECRET)], + expected_error: "Fabro server refuses to start: dev-token auth is enabled but FABRO_DEV_TOKEN is not set.", + }, + StartupFailureCase { + name: "missing-github-client-secret", + settings: GITHUB_SETTINGS, + server_env: &[("SESSION_SECRET", TEST_SESSION_SECRET)], + expected_error: "Fabro server refuses to start: github auth is enabled but GITHUB_APP_CLIENT_SECRET is not set.", + }, + StartupFailureCase { + name: "empty-auth-methods", + settings: EMPTY_AUTH_METHODS_SETTINGS, + server_env: &[], + expected_error: "failed to resolve server settings:\n server.auth.methods: invalid value - must not be empty", + }, + StartupFailureCase { + name: "github-web-disabled", + settings: GITHUB_WEB_DISABLED_SETTINGS, + server_env: &[ + ("SESSION_SECRET", TEST_SESSION_SECRET), + ("GITHUB_APP_CLIENT_SECRET", "github-client-secret"), + ], + expected_error: "Fabro server refuses to start: github auth is enabled but server.web.enabled is false.", + }, + StartupFailureCase { + name: "github-missing-client-id", + settings: GITHUB_WITHOUT_CLIENT_ID_SETTINGS, + server_env: &[ + ("SESSION_SECRET", TEST_SESSION_SECRET), + ("GITHUB_APP_CLIENT_SECRET", "github-client-secret"), + ], + expected_error: "Fabro server refuses to start: github auth is enabled but server.integrations.github.client_id is not configured.", + }, + StartupFailureCase { + name: "invalid-dev-token", + settings: DEV_TOKEN_SETTINGS, + server_env: &[ + ("SESSION_SECRET", TEST_SESSION_SECRET), + ("FABRO_DEV_TOKEN", "not-a-valid-dev-token"), + ], + expected_error: "Fabro server refuses to start: FABRO_DEV_TOKEN has invalid format.", + }, + ]; + + for case in &cases { + for mode in ServerStartMode::ALL { + run_startup_failure(&context, mode, case); + } + } +} + #[test] fn start_already_running_exits_with_error() { let context = test_context!(); diff --git a/lib/crates/fabro-server/src/server_secrets.rs b/lib/crates/fabro-server/src/server_secrets.rs index a27a2acde..4e0006744 100644 --- a/lib/crates/fabro-server/src/server_secrets.rs +++ b/lib/crates/fabro-server/src/server_secrets.rs @@ -1,5 +1,5 @@ use std::collections::HashMap; -use std::path::PathBuf; +use std::path::Path; use std::sync::Arc; use fabro_auth::{CredentialResolver, CredentialUsage, ResolveError, ResolvedCredential}; @@ -39,17 +39,15 @@ pub(crate) enum Error { } pub(crate) struct ServerSecrets { - path: PathBuf, env_entries: HashMap, file_entries: HashMap, } impl ServerSecrets { - pub(crate) fn load(path: PathBuf, env: &dyn EnvSource) -> Result { + pub(crate) fn load(path: impl AsRef, env: &dyn EnvSource) -> Result { Ok(Self { - env_entries: env.snapshot(), - file_entries: envfile::read_env_file(&path)?, - path, + env_entries: env.snapshot(), + file_entries: envfile::read_env_file(path.as_ref())?, }) } @@ -64,7 +62,6 @@ impl ServerSecrets { impl std::fmt::Debug for ServerSecrets { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("ServerSecrets") - .field("path", &self.path) .field("env_entries", &self.env_entries.keys().collect::>()) .field( "file_entries", diff --git a/lib/crates/fabro-server/src/startup.rs b/lib/crates/fabro-server/src/startup.rs index 32e6ce606..5b346b0e1 100644 --- a/lib/crates/fabro-server/src/startup.rs +++ b/lib/crates/fabro-server/src/startup.rs @@ -35,7 +35,7 @@ pub(crate) fn resolve_startup( env: &dyn EnvSource, settings: &ResolvedServerSettings, ) -> std::result::Result { - let server_secrets = ServerSecrets::load(env_path.to_path_buf(), env)?; + let server_secrets = ServerSecrets::load(env_path, env)?; let auth_mode = resolve_auth_mode_with_lookup(settings, |name| server_secrets.get(name))?; Ok(StartupResolution { auth_mode,