From f40bcd1215330e95225e4fa33fb307d54458d759 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 29 Apr 2026 08:09:02 -0400 Subject: [PATCH] fix(server): forward GITHUB_APP_PRIVATE_KEY via worker_command The previous commit added GITHUB_APP_PRIVATE_KEY to the worker env allowlist, but the secret is in server.env / ServerSecrets, not in the server's process env, so the allowlist couldn't see it. Forward the value explicitly from ServerSecrets at spawn time, mirroring how FABRO_WORKER_TOKEN is already passed. Keeps the allowlist narrow as a fail-closed barrier against ambient env leakage and keeps ServerSecrets as the single read site for server.env secrets. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-server/src/server.rs | 58 +++++++++++++++++++++++- lib/crates/fabro-server/src/spawn_env.rs | 11 +---- 2 files changed, 59 insertions(+), 10 deletions(-) diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index d50266e5b..4c8eb55a3 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -3907,6 +3907,9 @@ fn worker_command( cmd.env(EnvVars::FABRO_LOG_DESTINATION, value); cmd.env_remove(EnvVars::FABRO_WORKER_TOKEN); cmd.env(EnvVars::FABRO_WORKER_TOKEN, worker_token); + if let Some(pem) = state.server_secret(EnvVars::GITHUB_APP_PRIVATE_KEY) { + cmd.env(EnvVars::GITHUB_APP_PRIVATE_KEY, pem); + } #[cfg(unix)] fabro_proc::pre_exec_setpgid(cmd.as_std_mut()); @@ -9172,6 +9175,52 @@ provider = "invalid-provider" assert_eq!(dev_claims.run_id, dev_token_run_id.to_string()); } + #[cfg(unix)] + #[test] + fn worker_command_forwards_github_app_private_key_from_server_secrets() { + let storage_dir = tempfile::tempdir().unwrap(); + let state = worker_command_test_state_with_extra_config_and_env_lookup( + storage_dir.path(), + &["dev-token"], + Some(TEST_DEV_TOKEN), + "", + &[(EnvVars::GITHUB_APP_PRIVATE_KEY, "test-private-key")], + |_| None, + ); + let cmd = worker_command( + state.as_ref(), + RunId::new(), + RunExecutionMode::Start, + storage_dir.path(), + ) + .unwrap(); + + assert_eq!( + command_env_value(&cmd, EnvVars::GITHUB_APP_PRIVATE_KEY), + EnvOverride::Set("test-private-key".to_string()) + ); + } + + #[cfg(unix)] + #[test] + fn worker_command_omits_github_app_private_key_when_unset() { + let storage_dir = tempfile::tempdir().unwrap(); + let state = + worker_command_test_state(storage_dir.path(), &["dev-token"], Some(TEST_DEV_TOKEN)); + let cmd = worker_command( + state.as_ref(), + RunId::new(), + RunExecutionMode::Start, + storage_dir.path(), + ) + .unwrap(); + + assert_eq!( + command_env_value(&cmd, EnvVars::GITHUB_APP_PRIVATE_KEY), + EnvOverride::Unchanged + ); + } + #[cfg(unix)] #[test] fn worker_command_sets_fabro_log_from_server_logging_config() { @@ -9242,6 +9291,7 @@ destination = "stdout" [server.logging] destination = "file" "#, + &[], |name| (name == EnvVars::FABRO_LOG_DESTINATION).then(|| "stdout".to_string()), ); let run_id = RunId::new(); @@ -9272,6 +9322,7 @@ destination = "file" [server.logging] destination = "file" "#, + &[], |name| (name == EnvVars::FABRO_LOG_DESTINATION).then(|| "stdot".to_string()), ); let run_id = RunId::new(); @@ -9347,6 +9398,7 @@ methods = ["dev-token"] methods, dev_token, extra_config, + &[], |_| None, ) } @@ -9356,6 +9408,7 @@ methods = ["dev-token"] methods: &[&str], dev_token: Option<&str>, extra_config: &str, + extra_server_secrets: &[(&str, &str)], env_lookup: impl Fn(&str) -> Option + Send + Sync + 'static, ) -> Arc { let dev_token = dev_token.map(str::to_owned); @@ -9390,9 +9443,12 @@ allowed_usernames = ["octocat"] .write(&runtime_directory) .unwrap(); - let server_secret_env = dev_token + let mut server_secret_env: HashMap = dev_token .map(|token| HashMap::from([("FABRO_DEV_TOKEN".to_string(), token)])) .unwrap_or_default(); + for (key, value) in extra_server_secrets { + server_secret_env.insert((*key).to_string(), (*value).to_string()); + } create_app_state_with_env_lookup_and_server_secret_env( server_settings_from_toml(&source), manifest_run_defaults_from_toml(&source), diff --git a/lib/crates/fabro-server/src/spawn_env.rs b/lib/crates/fabro-server/src/spawn_env.rs index a8b417d10..934729af8 100644 --- a/lib/crates/fabro-server/src/spawn_env.rs +++ b/lib/crates/fabro-server/src/spawn_env.rs @@ -13,7 +13,6 @@ const WORKER_ENV_ALLOWLIST: &[&str] = &[ EnvVars::FABRO_LOG, EnvVars::FABRO_HOME, EnvVars::FABRO_STORAGE_ROOT, - EnvVars::GITHUB_APP_PRIVATE_KEY, ]; const RENDER_GRAPH_ENV_ALLOWLIST: &[&str] = &[EnvVars::PATH, EnvVars::HOME, EnvVars::TMPDIR]; @@ -87,10 +86,7 @@ mod tests { ("SESSION_SECRET".to_string(), "leak".to_string()), ("FABRO_JWT_PRIVATE_KEY".to_string(), "leak".to_string()), ("FABRO_JWT_PUBLIC_KEY".to_string(), "leak".to_string()), - ( - "GITHUB_APP_PRIVATE_KEY".to_string(), - "private-key".to_string(), - ), + ("GITHUB_APP_PRIVATE_KEY".to_string(), "leak".to_string()), ("GITHUB_APP_CLIENT_SECRET".to_string(), "leak".to_string()), ("GITHUB_APP_WEBHOOK_SECRET".to_string(), "leak".to_string()), ("FABRO_DEV_TOKEN".to_string(), "garbage".to_string()), @@ -118,10 +114,7 @@ mod tests { assert!(!actual.contains_key("SESSION_SECRET")); assert!(!actual.contains_key("FABRO_JWT_PRIVATE_KEY")); assert!(!actual.contains_key("FABRO_JWT_PUBLIC_KEY")); - assert_eq!( - actual.get("GITHUB_APP_PRIVATE_KEY").map(String::as_str), - Some("private-key") - ); + assert!(!actual.contains_key("GITHUB_APP_PRIVATE_KEY")); assert!(!actual.contains_key("GITHUB_APP_CLIENT_SECRET")); assert!(!actual.contains_key("GITHUB_APP_WEBHOOK_SECRET")); assert!(!actual.contains_key("MY_API_KEY"));