scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-10-08 03:10:26 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 5

refactor(server): drop EnvSource trait for direct HashMap passing

Browse source 
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<String, String>` 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) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-04-23 08:22:35 -04:00
parent e7099adf3d
commit 04d43785e6
No known key found for this signature in database
8 changed files with 87 additions and 154 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
lib/crates/fabro-cli/src/commands/server/start.rs
View file

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

9
lib/crates/fabro-server/src/install.rs
View file

@ -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::<String, String>::new()).unwrap();
let server_secrets = ServerSecrets::load(env_path.clone(), HashMap::new()).unwrap();
let manual_credentials =
InstallAwsCredentialPair::new("submitted-access", "submitted-secret");

3
lib/crates/fabro-server/src/lib.rs
View file

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

12
lib/crates/fabro-server/src/serve.rs
View file

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

10
lib/crates/fabro-server/src/server.rs
View file

@ -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<String, String>) -> ServerSecrets {
fn load_test_server_secrets(path: PathBuf, env: HashMap<String, String>) -> 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();

39
lib/crates/fabro-server/src/server_secrets.rs
View file

@ -11,22 +11,8 @@ use tokio::sync::RwLock as AsyncRwLock;
type EnvLookup = Arc<dyn Fn(&str) -> Option<String> + Send + Sync>;
pub trait EnvSource {
fn snapshot(&self) -> HashMap<String, String>;
}
pub struct ProcessEnv;
impl EnvSource for ProcessEnv {
fn snapshot(&self) -> HashMap<String, String> {
std::env::vars().collect()
}
}
impl EnvSource for HashMap<String, String> {
fn snapshot(&self) -> HashMap<String, String> {
self.clone()
}
pub fn process_env_snapshot() -> HashMap<String, String> {
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<Path>, env: &dyn EnvSource) -> Result<Self, Error> {
pub(crate) fn load(
path: impl AsRef<Path>,
env_entries: HashMap<String, String>,
) -> Result<Self, Error> {
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"));
}
}

86
lib/crates/fabro-server/src/spawn_env.rs
View file

@ -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<OsString>) {
apply_allowlist(cmd, WORKER_ENV_ALLOWLIST, lookup);
}
fn apply_render_graph_env_with_lookup(
cmd: &mut Command,
lookup: &dyn Fn(&str) -> Option<OsString>,
) {
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<OsString>) {
@ -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<String, String> {
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<String, String> {
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!(

72
lib/crates/fabro-server/src/startup.rs
View file

@ -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<ServerSecretsError> for StartupValidationError {
fn from(err: ServerSecretsError) -> Self {
Self::Message(err.to_string())
}
}
impl From<anyhow::Error> 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<String, String>,
settings: &ResolvedServerSettings,
) -> std::result::Result<StartupResolution, StartupValidationError> {
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<String, String>,
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<String, String> = 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()
);
}
}