fix(server): cover secret boundary enforcement

This commit is contained in:
Bryan Helmkamp 2026-04-23 07:51:28 -04:00
parent e6e091fe8e
commit 56667c17a8
No known key found for this signature in database
9 changed files with 401 additions and 15 deletions

View file

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

42
bin/dev/check-env-mutation.sh Executable file
View file

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

View file

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

View file

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

View file

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

View file

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

View file

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

View file

@ -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<String, String>,
file_entries: HashMap<String, String>,
}
impl ServerSecrets {
pub(crate) fn load(path: PathBuf, env: &dyn EnvSource) -> Result<Self, Error> {
pub(crate) fn load(path: impl AsRef<Path>, env: &dyn EnvSource) -> Result<Self, Error> {
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::<Vec<_>>())
.field(
"file_entries",

View file

@ -35,7 +35,7 @@ pub(crate) fn resolve_startup(
env: &dyn EnvSource,
settings: &ResolvedServerSettings,
) -> std::result::Result<StartupResolution, StartupValidationError> {
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,