From 04d43785e6a01005a2424d14e26efd85ba79b94f Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Apr 2026 08:22:35 -0400 Subject: [PATCH] refactor(server): drop EnvSource trait for direct HashMap passing After StubEnv was removed, the EnvSource trait had only two impls (ProcessEnv unit struct and a blanket HashMap impl) and tests already passed HashMaps. Replace the trait with a free `process_env_snapshot()` function and take `HashMap` by value in `ServerSecrets::load` and the startup validators. Also flatten `StartupResolution` to a `(AuthMode, ServerSecrets)` tuple and drop the `StartupValidationError` wrapper in favor of `anyhow::Result`, and inline the `*_with_lookup` test-only wrappers in `spawn_env.rs` so tests call `apply_allowlist` directly. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../fabro-cli/src/commands/server/start.rs | 10 +-- lib/crates/fabro-server/src/install.rs | 9 +- lib/crates/fabro-server/src/lib.rs | 3 +- lib/crates/fabro-server/src/serve.rs | 12 +-- lib/crates/fabro-server/src/server.rs | 10 +-- lib/crates/fabro-server/src/server_secrets.rs | 39 ++------- lib/crates/fabro-server/src/spawn_env.rs | 86 ++++++++----------- lib/crates/fabro-server/src/startup.rs | 72 +++++----------- 8 files changed, 87 insertions(+), 154 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/server/start.rs b/lib/crates/fabro-cli/src/commands/server/start.rs index 937baa296..6380cf1a9 100644 --- a/lib/crates/fabro-cli/src/commands/server/start.rs +++ b/lib/crates/fabro-cli/src/commands/server/start.rs @@ -13,7 +13,7 @@ use fabro_config::daemon::ServerDaemon; use fabro_config::user::{FABRO_CONFIG_ENV, default_settings_path, load_settings_config}; use fabro_server::jwt_auth::auth_method_name; use fabro_server::serve::{DEFAULT_TCP_PORT, ServeArgs, resolve_runtime_server_settings_for_start}; -use fabro_server::{ProcessEnv, validate_startup}; +use fabro_server::{process_env_snapshot, validate_startup}; use fabro_types::settings::ServerAuthMethod; use fabro_util::printer::Printer; use fabro_util::terminal::Styles; @@ -234,9 +234,6 @@ 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 } @@ -270,10 +267,9 @@ async fn execute_daemon( let resolved_settings = resolve_runtime_server_settings_for_start(serve_args, storage_dir)?; validate_startup( runtime_directory.env_path().as_path(), - &ProcessEnv, + process_env_snapshot(), &resolved_settings, - ) - .map_err(anyhow::Error::from)?; + )?; let log_path = runtime_directory.log_path(); if let Some(parent) = log_path.parent() { diff --git a/lib/crates/fabro-server/src/install.rs b/lib/crates/fabro-server/src/install.rs index 32e3ee5fd..f0e81d4aa 100644 --- a/lib/crates/fabro-server/src/install.rs +++ b/lib/crates/fabro-server/src/install.rs @@ -42,7 +42,7 @@ use zeroize::Zeroizing; use crate::error::ApiError; use crate::serve::{self, DEFAULT_TCP_PORT}; -use crate::server_secrets::{ProcessEnv, ServerSecrets}; +use crate::server_secrets::{ServerSecrets, process_env_snapshot}; use crate::{security_headers, static_files}; #[derive(Clone)] @@ -961,8 +961,8 @@ async fn validate_install_object_store_selection( let server_env_path = Storage::new(state.storage_dir.as_ref()) .runtime_directory() .env_path(); - let server_secrets = - ServerSecrets::load(server_env_path, &ProcessEnv).map_err(|err| err.to_string())?; + let server_secrets = ServerSecrets::load(server_env_path, process_env_snapshot()) + .map_err(|err| err.to_string())?; let build_options = serve::ObjectStoreBuildOptions { client_options, retry_config: RetryConfig { @@ -2090,8 +2090,7 @@ AWS_SESSION_TOKEN=ambient-session\n\ AWS_WEB_IDENTITY_TOKEN_FILE=/tmp/fabro-web-identity-token\n", ) .unwrap(); - let server_secrets = - ServerSecrets::load(env_path.clone(), &HashMap::::new()).unwrap(); + let server_secrets = ServerSecrets::load(env_path.clone(), HashMap::new()).unwrap(); let manual_credentials = InstallAwsCredentialPair::new("submitted-access", "submitted-secret"); diff --git a/lib/crates/fabro-server/src/lib.rs b/lib/crates/fabro-server/src/lib.rs index 582fcaca7..229c71af4 100644 --- a/lib/crates/fabro-server/src/lib.rs +++ b/lib/crates/fabro-server/src/lib.rs @@ -38,4 +38,5 @@ pub mod static_files; pub mod web_auth; pub use error::{ApiError, Error, Result}; -pub use startup::{EnvSource, ProcessEnv, StartupValidationError, validate_startup}; +pub use server_secrets::process_env_snapshot; +pub use startup::validate_startup; diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index ecd0686af..eaf64c5d3 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -35,7 +35,7 @@ use crate::server::{ AppState, AppStateConfig, RouterOptions, build_app_state, build_router_with_options, reconcile_incomplete_runs_on_startup, shutdown_active_workers, spawn_scheduler, }; -use crate::server_secrets::{ProcessEnv, ServerSecrets}; +use crate::server_secrets::{ServerSecrets, process_env_snapshot}; use crate::startup::resolve_startup; const TEST_IN_MEMORY_STORE_ENV: &str = "FABRO_TEST_IN_MEMORY_STORE"; @@ -518,7 +518,7 @@ fn load_server_secrets_for_settings( ) -> anyhow::Result { let storage_root = resolve_interp_path(&settings.storage.root)?; let server_env_path = Storage::new(&storage_root).runtime_directory().env_path(); - ServerSecrets::load(server_env_path, &ProcessEnv).map_err(anyhow::Error::from) + ServerSecrets::load(server_env_path, process_env_snapshot()).map_err(anyhow::Error::from) } pub(crate) fn build_artifact_object_store_with_server_secrets( @@ -603,9 +603,11 @@ where // Shared config for live reloading let effective_settings = apply_runtime_settings(&disk_settings, &args, &data_dir); let resolved_server_settings = resolve_server_settings(&effective_settings)?; - let startup = resolve_startup(&server_env_path, &ProcessEnv, &resolved_server_settings)?; - let auth_mode = startup.auth_mode; - let server_secrets = startup.server_secrets; + let (auth_mode, server_secrets) = resolve_startup( + &server_env_path, + process_env_snapshot(), + &resolved_server_settings, + )?; let webhook_secret_present = server_secrets.get(WEBHOOK_SECRET_ENV).is_some(); let bind_request = resolve_bind_request_from_settings(&effective_settings, args.bind.as_deref())?; diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 1d96a7bfd..9cc11700e 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -2516,7 +2516,7 @@ pub fn create_app_state_with_env_lookup_and_server_secret_env( config.store = store; config.artifact_store = artifact_store; let server_env_path = config.vault_path.with_file_name("server.env"); - config.server_secrets = load_test_server_secrets(server_env_path, server_secret_env); + config.server_secrets = load_test_server_secrets(server_env_path, server_secret_env.clone()); build_app_state(config).expect("test app state should build") } @@ -2553,7 +2553,7 @@ pub(crate) fn create_test_app_state_with_session_key( store, artifact_store, vault_path, - server_secrets: load_test_server_secrets(server_env_path, &HashMap::new()), + server_secrets: load_test_server_secrets(server_env_path, HashMap::new()), local_daemon_mode, env_lookup, http_client: Some(fabro_http::test_http_client().expect("test HTTP client should build")), @@ -2589,7 +2589,7 @@ fn default_test_app_state_config( store, artifact_store, vault_path, - server_secrets: load_test_server_secrets(server_env_path, &HashMap::new()), + server_secrets: load_test_server_secrets(server_env_path, HashMap::new()), local_daemon_mode: false, env_lookup, http_client: Some(fabro_http::test_http_client().expect("test HTTP client should build")), @@ -2646,7 +2646,7 @@ fn default_env_lookup() -> EnvLookup { Arc::new(|name| std::env::var(name).ok()) } -fn load_test_server_secrets(path: PathBuf, env: &HashMap) -> ServerSecrets { +fn load_test_server_secrets(path: PathBuf, env: HashMap) -> ServerSecrets { ServerSecrets::load(path, env).expect("test server secrets should load") } @@ -7892,7 +7892,7 @@ type = "http" let secrets = ServerSecrets::load( dir.path().join("server.env"), - &HashMap::from([("SESSION_SECRET".to_string(), "env-value".to_string())]), + HashMap::from([("SESSION_SECRET".to_string(), "env-value".to_string())]), ) .unwrap(); diff --git a/lib/crates/fabro-server/src/server_secrets.rs b/lib/crates/fabro-server/src/server_secrets.rs index 125025084..b09ed710b 100644 --- a/lib/crates/fabro-server/src/server_secrets.rs +++ b/lib/crates/fabro-server/src/server_secrets.rs @@ -11,22 +11,8 @@ use tokio::sync::RwLock as AsyncRwLock; type EnvLookup = Arc Option + Send + Sync>; -pub trait EnvSource { - fn snapshot(&self) -> HashMap; -} - -pub struct ProcessEnv; - -impl EnvSource for ProcessEnv { - fn snapshot(&self) -> HashMap { - std::env::vars().collect() - } -} - -impl EnvSource for HashMap { - fn snapshot(&self) -> HashMap { - self.clone() - } +pub fn process_env_snapshot() -> HashMap { + std::env::vars().collect() } #[derive(Debug, thiserror::Error)] @@ -41,9 +27,12 @@ pub(crate) struct ServerSecrets { } impl ServerSecrets { - pub(crate) fn load(path: impl AsRef, env: &dyn EnvSource) -> Result { + pub(crate) fn load( + path: impl AsRef, + env_entries: HashMap, + ) -> Result { Ok(Self { - env_entries: env.snapshot(), + env_entries, file_entries: envfile::read_env_file(path.as_ref())?, }) } @@ -231,7 +220,7 @@ mod tests { let secrets = ServerSecrets::load( env_path, - &HashMap::from([("SESSION_SECRET".to_string(), "env-value".to_string())]), + HashMap::from([("SESSION_SECRET".to_string(), "env-value".to_string())]), ) .unwrap(); @@ -241,16 +230,4 @@ mod tests { Some("file-client") ); } - - #[test] - fn server_secrets_snapshot_is_owned_after_load() { - let dir = tempfile::tempdir().unwrap(); - let env_path = dir.path().join("server.env"); - let mut env = HashMap::from([("SESSION_SECRET".to_string(), "before".to_string())]); - - let secrets = ServerSecrets::load(env_path, &env.clone()).unwrap(); - env.insert("SESSION_SECRET".to_string(), "after".to_string()); - - assert_eq!(secrets.get("SESSION_SECRET").as_deref(), Some("before")); - } } diff --git a/lib/crates/fabro-server/src/spawn_env.rs b/lib/crates/fabro-server/src/spawn_env.rs index d9a6029cc..cd55bd18a 100644 --- a/lib/crates/fabro-server/src/spawn_env.rs +++ b/lib/crates/fabro-server/src/spawn_env.rs @@ -3,39 +3,26 @@ use std::ffi::OsString; use tokio::process::Command; const WORKER_ENV_ALLOWLIST: &[&str] = &[ - "PATH", // process essentials - "HOME", // process essentials - "TMPDIR", // temp file staging - "USER", // process identity - "RUST_LOG", // diagnostics - "RUST_BACKTRACE", // diagnostics - "FABRO_HOME", // worker state lookup - "FABRO_STORAGE_ROOT", // worker state lookup + "PATH", + "HOME", + "TMPDIR", + "USER", + "RUST_LOG", + "RUST_BACKTRACE", + "FABRO_HOME", + "FABRO_STORAGE_ROOT", ]; -const RENDER_GRAPH_ENV_ALLOWLIST: &[&str] = &[ - "PATH", // executable lookup - "HOME", // graphviz/font resolution - "TMPDIR", // temp file staging -]; +const RENDER_GRAPH_ENV_ALLOWLIST: &[&str] = &["PATH", "HOME", "TMPDIR"]; pub(crate) fn apply_worker_env(cmd: &mut Command) { - apply_worker_env_with_lookup(cmd, &|name| std::env::var_os(name)); + apply_allowlist(cmd, WORKER_ENV_ALLOWLIST, &|name| std::env::var_os(name)); } pub(crate) fn apply_render_graph_env(cmd: &mut Command) { - apply_render_graph_env_with_lookup(cmd, &|name| std::env::var_os(name)); -} - -fn apply_worker_env_with_lookup(cmd: &mut Command, lookup: &dyn Fn(&str) -> Option) { - apply_allowlist(cmd, WORKER_ENV_ALLOWLIST, lookup); -} - -fn apply_render_graph_env_with_lookup( - cmd: &mut Command, - lookup: &dyn Fn(&str) -> Option, -) { - apply_allowlist(cmd, RENDER_GRAPH_ENV_ALLOWLIST, lookup); + apply_allowlist(cmd, RENDER_GRAPH_ENV_ALLOWLIST, &|name| { + std::env::var_os(name) + }); } fn apply_allowlist(cmd: &mut Command, keys: &[&str], lookup: &dyn Fn(&str) -> Option) { @@ -53,31 +40,28 @@ mod tests { use std::ffi::OsString; use std::path::Path; - use super::{apply_render_graph_env_with_lookup, apply_worker_env_with_lookup}; + use super::{RENDER_GRAPH_ENV_ALLOWLIST, WORKER_ENV_ALLOWLIST, apply_allowlist}; fn env_command() -> tokio::process::Command { assert!(Path::new("/usr/bin/env").exists()); tokio::process::Command::new("/usr/bin/env") } - fn env_output(mut cmd: tokio::process::Command) -> HashMap { - let runtime = tokio::runtime::Runtime::new().expect("creating test Tokio runtime"); - runtime.block_on(async move { - let output = cmd.output().await.expect("running env subprocess"); - assert!(output.status.success()); - String::from_utf8(output.stdout) - .expect("parsing env subprocess output as UTF-8") - .lines() - .filter_map(|line| { - let (key, value) = line.split_once('=')?; - Some((key.to_string(), value.to_string())) - }) - .collect() - }) + async fn env_output(mut cmd: tokio::process::Command) -> HashMap { + let output = cmd.output().await.expect("running env subprocess"); + assert!(output.status.success()); + String::from_utf8(output.stdout) + .expect("parsing env subprocess output as UTF-8") + .lines() + .filter_map(|line| { + let (key, value) = line.split_once('=')?; + Some((key.to_string(), value.to_string())) + }) + .collect() } - #[test] - fn worker_allowlist_is_fail_closed() { + #[tokio::test] + async fn worker_allowlist_is_fail_closed() { let env = HashMap::from([ ("PATH".to_string(), "/bin".to_string()), ("HOME".to_string(), "/tmp/home".to_string()), @@ -94,13 +78,15 @@ mod tests { ("MY_API_KEY".to_string(), "blocked".to_string()), ]); let mut cmd = env_command(); - apply_worker_env_with_lookup(&mut cmd, &|name| env.get(name).map(OsString::from)); + apply_allowlist(&mut cmd, WORKER_ENV_ALLOWLIST, &|name| { + env.get(name).map(OsString::from) + }); cmd.env( "FABRO_DEV_TOKEN", "fabro_dev_abababababababababababababababababababababababababababababababab", ); - let actual = env_output(cmd); + let actual = env_output(cmd).await; assert_eq!(actual.get("PATH").map(String::as_str), Some("/bin")); assert_eq!(actual.get("HOME").map(String::as_str), Some("/tmp/home")); @@ -112,8 +98,8 @@ mod tests { assert!(!actual.contains_key("MY_API_KEY")); } - #[test] - fn render_graph_allowlist_is_fail_closed() { + #[tokio::test] + async fn render_graph_allowlist_is_fail_closed() { let env = HashMap::from([ ("PATH".to_string(), "/bin".to_string()), ("HOME".to_string(), "/tmp/home".to_string()), @@ -122,10 +108,12 @@ mod tests { ("SESSION_SECRET".to_string(), "leak".to_string()), ]); let mut cmd = env_command(); - apply_render_graph_env_with_lookup(&mut cmd, &|name| env.get(name).map(OsString::from)); + apply_allowlist(&mut cmd, RENDER_GRAPH_ENV_ALLOWLIST, &|name| { + env.get(name).map(OsString::from) + }); cmd.env("FABRO_TELEMETRY", "off"); - let actual = env_output(cmd); + let actual = env_output(cmd).await; assert_eq!(actual.get("PATH").map(String::as_str), Some("/bin")); assert_eq!( diff --git a/lib/crates/fabro-server/src/startup.rs b/lib/crates/fabro-server/src/startup.rs index a991d86e3..3817c5e6a 100644 --- a/lib/crates/fabro-server/src/startup.rs +++ b/lib/crates/fabro-server/src/startup.rs @@ -1,54 +1,27 @@ +use std::collections::HashMap; use std::path::Path; use fabro_types::settings::ServerSettings as ResolvedServerSettings; use crate::jwt_auth::{AuthMode, resolve_auth_mode_with_lookup}; -pub use crate::server_secrets::{EnvSource, ProcessEnv}; -use crate::server_secrets::{Error as ServerSecretsError, ServerSecrets}; - -#[derive(Debug)] -pub(crate) struct StartupResolution { - pub(crate) auth_mode: AuthMode, - pub(crate) server_secrets: ServerSecrets, -} - -#[derive(Debug, thiserror::Error)] -pub enum StartupValidationError { - #[error("{0}")] - Message(String), -} - -impl From for StartupValidationError { - fn from(err: ServerSecretsError) -> Self { - Self::Message(err.to_string()) - } -} - -impl From for StartupValidationError { - fn from(err: anyhow::Error) -> Self { - Self::Message(err.to_string()) - } -} +use crate::server_secrets::ServerSecrets; pub(crate) fn resolve_startup( env_path: &Path, - env: &dyn EnvSource, + env_entries: HashMap, settings: &ResolvedServerSettings, -) -> std::result::Result { - let server_secrets = ServerSecrets::load(env_path, env)?; +) -> anyhow::Result<(AuthMode, ServerSecrets)> { + let server_secrets = ServerSecrets::load(env_path, env_entries)?; let auth_mode = resolve_auth_mode_with_lookup(settings, |name| server_secrets.get(name))?; - Ok(StartupResolution { - auth_mode, - server_secrets, - }) + Ok((auth_mode, server_secrets)) } pub fn validate_startup( env_path: &Path, - env: &dyn EnvSource, + env_entries: HashMap, settings: &ResolvedServerSettings, -) -> std::result::Result<(), StartupValidationError> { - resolve_startup(env_path, env, settings).map(|_| ()) +) -> anyhow::Result<()> { + resolve_startup(env_path, env_entries, settings).map(|_| ()) } #[cfg(test)] @@ -58,7 +31,7 @@ mod tests { use fabro_config::parse_settings_layer; use fabro_types::settings::ServerSettings as ResolvedServerSettings; - use super::{resolve_startup, validate_startup}; + use super::validate_startup; fn resolved_settings(auth_methods: &[&str]) -> ResolvedServerSettings { let settings = parse_settings_layer(&format!( @@ -79,7 +52,7 @@ methods = [{}] } #[test] - fn validate_startup_matches_resolve_startup() { + fn validate_startup_accepts_configured_secrets() { let dir = tempfile::tempdir().unwrap(); let env = HashMap::from([ ( @@ -94,24 +67,21 @@ methods = [{}] ]); let settings = resolved_settings(&["dev-token"]); - assert!(validate_startup(dir.path().join("server.env").as_path(), &env, &settings).is_ok()); - assert!(resolve_startup(dir.path().join("server.env").as_path(), &env, &settings).is_ok()); + assert!(validate_startup(dir.path().join("server.env").as_path(), env, &settings).is_ok()); } #[test] - fn validate_startup_and_resolve_startup_share_errors() { + fn validate_startup_rejects_missing_secrets() { let dir = tempfile::tempdir().unwrap(); - let env: HashMap = HashMap::new(); let settings = resolved_settings(&["dev-token"]); - let validate_err = - validate_startup(dir.path().join("server.env").as_path(), &env, &settings) - .unwrap_err() - .to_string(); - let resolve_err = resolve_startup(dir.path().join("server.env").as_path(), &env, &settings) - .unwrap_err() - .to_string(); - - assert_eq!(validate_err, resolve_err); + assert!( + validate_startup( + dir.path().join("server.env").as_path(), + HashMap::new(), + &settings, + ) + .is_err() + ); } }