From b6482910e517d00dfc3c4a2f2d3e417c9348f7f6 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sat, 5 Sep 2026 14:05:48 -0400 Subject: [PATCH] Remove expired startup secret migrations --- docs/internal/migrations-strategy.md | 4 +- docs/internal/server-secrets-strategy.md | 2 +- .../administration/server-configuration.mdx | 2 +- .../fabro-cli/src/commands/server/start.rs | 5 +- .../fabro-cli/tests/it/cmd/server_start.rs | 9 + .../2026051801_legacy_vault_entries.rs | 275 ------------------ ...01_optional_server_env_secrets_to_vault.rs | 172 ----------- lib/apps/fabro-server/src/lib.rs | 2 +- lib/apps/fabro-server/src/migrations.rs | 28 -- lib/apps/fabro-server/src/serve.rs | 27 +- lib/apps/fabro-server/src/server/tests.rs | 122 -------- lib/apps/fabro-server/src/startup.rs | 246 +--------------- lib/apps/fabro-server/src/test_support.rs | 3 +- 13 files changed, 19 insertions(+), 878 deletions(-) delete mode 100644 lib/apps/fabro-server/migrations/2026051801_legacy_vault_entries.rs delete mode 100644 lib/apps/fabro-server/migrations/2026052501_optional_server_env_secrets_to_vault.rs diff --git a/docs/internal/migrations-strategy.md b/docs/internal/migrations-strategy.md index 318463a58..026abe844 100644 --- a/docs/internal/migrations-strategy.md +++ b/docs/internal/migrations-strategy.md @@ -20,7 +20,7 @@ The crate-local `src/migrations.rs` module is the registry. It imports numbered Examples: - `fabro-config` owns settings-file migrations. -- `fabro-server` owns server startup migrations for `server.env` and vault files. +- `fabro-server` owns server startup activation migrations for SQLite blob storage and run history. Keep migration APIs `pub(crate)` unless another crate genuinely orchestrates the migration. @@ -125,7 +125,7 @@ If a migration removes entries from a file after writing another store, write th Choose the error policy deliberately. -Use warn-and-continue only when the normal path may still succeed and compatibility is best-effort. The legacy vault-entry migration does this because an unreadable legacy shape should not block loading an otherwise usable vault file. +Use warn-and-continue only when the normal path may still succeed and compatibility is best-effort. Return an error when the migration found data it must move or rewrite and cannot do so safely. This gives operators a precise migration failure instead of a later, misleading startup error. diff --git a/docs/internal/server-secrets-strategy.md b/docs/internal/server-secrets-strategy.md index df4160886..a6546f751 100644 --- a/docs/internal/server-secrets-strategy.md +++ b/docs/internal/server-secrets-strategy.md @@ -119,7 +119,7 @@ Bootstrap secrets come from one of two sources: Optional integration secrets are provisioned into the vault, usually with `fabro secret set` or `fabro install`. -There is no startup-time secret generation. A temporary startup migration moves recognized legacy optional secrets from process env or `server.env` into the vault, removes matching `server.env` entries after writing a backup, and logs conflicts by key name only. Runtime lookup remains vault-only after that migration step. See [migrations-strategy.md](migrations-strategy.md) for the migration pattern. +There is no startup-time secret generation or import of optional integration secrets from process env or `server.env`. The compatibility migrations for those sources and pre-token/OAuth vault entries have been removed. The separate one-time import of current-format `secrets.json` entries into SQLite remains supported. ## Subprocess Boundaries diff --git a/docs/public/administration/server-configuration.mdx b/docs/public/administration/server-configuration.mdx index d37b7223a..6fc9be4fb 100644 --- a/docs/public/administration/server-configuration.mdx +++ b/docs/public/administration/server-configuration.mdx @@ -371,7 +371,7 @@ Fabro splits server-runtime secrets into two scopes: `server.env` is not used for Slack, Daytona, Brave Search, Venice Search, LLM provider keys, `GITHUB_TOKEN`, or GitHub App private key/client secret/webhook secret. Configure those optional integrations with `fabro secret set`, `fabro provider login`, or `fabro install`. -During startup, Fabro temporarily migrates recognized legacy optional integration secrets from process env or `server.env` into the vault. When a matching `server.env` entry can be safely removed, Fabro writes a hidden backup beside `server.env` first. Process env values cannot be cleaned up automatically, so remove those from your deployment environment after the vault contains the secret. +Startup does not import optional integration secrets from process env or `server.env`, or rewrite old `credential` / `environment` vault entries. Provision these secrets with the commands above before upgrading an installation that still uses those retired sources or formats. Fabro no longer auto-loads `.env` files. Provider API keys are required for the models you want to use; everything else is optional. diff --git a/lib/apps/fabro-cli/src/commands/server/start.rs b/lib/apps/fabro-cli/src/commands/server/start.rs index c703555a9..233465885 100644 --- a/lib/apps/fabro-cli/src/commands/server/start.rs +++ b/lib/apps/fabro-cli/src/commands/server/start.rs @@ -13,9 +13,7 @@ use fabro_config::user::default_settings_path; use fabro_config::{RuntimeDirectory, Storage}; 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::{ - migrate_startup_vault, process_env_snapshot, validate_startup, validate_startup_configuration, -}; +use fabro_server::{process_env_snapshot, validate_startup, validate_startup_configuration}; use fabro_static::EnvVars; use fabro_types::settings::{LogDestination, ServerAuthMethod}; use fabro_util::printer::Printer; @@ -291,7 +289,6 @@ async fn execute_daemon( } validate_startup_configuration(&resolved_settings)?; let storage = Storage::new(storage_dir); - migrate_startup_vault(storage.secrets_path()); let startup_vault = SecretStore::open_snapshot(storage.sqlite_path(), storage.secrets_path()) .await .context("loading secrets for startup validation")?; diff --git a/lib/apps/fabro-cli/tests/it/cmd/server_start.rs b/lib/apps/fabro-cli/tests/it/cmd/server_start.rs index 899405856..68c1ceb13 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/server_start.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/server_start.rs @@ -292,6 +292,15 @@ methods = [] 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 configured in the vault.", }, + StartupFailureCase { + name: "github-client-secret-only-in-server-env", + settings: GITHUB_SETTINGS, + server_env: &[ + ("SESSION_SECRET", TEST_SESSION_SECRET), + ("GITHUB_APP_CLIENT_SECRET", "unprovisioned-client-secret"), + ], + expected_error: "Fabro server refuses to start: github auth is enabled but GITHUB_APP_CLIENT_SECRET is not configured in the vault.", + }, StartupFailureCase { name: "empty-auth-methods", settings: EMPTY_AUTH_METHODS_SETTINGS, diff --git a/lib/apps/fabro-server/migrations/2026051801_legacy_vault_entries.rs b/lib/apps/fabro-server/migrations/2026051801_legacy_vault_entries.rs deleted file mode 100644 index d37d5213d..000000000 --- a/lib/apps/fabro-server/migrations/2026051801_legacy_vault_entries.rs +++ /dev/null @@ -1,275 +0,0 @@ -//! Temporary compatibility shim for pre-token/oauth vault files. -//! -//! Delete this module after 2026-08-18, once supported installs have had a -//! release window to rewrite `credential` / `environment` entries to the -//! `oauth` / `token` schemas. - -#![expect( - clippy::disallowed_methods, - reason = "Temporary startup migration uses synchronous vault file I/O before serving requests." -)] - -use std::collections::HashSet; -use std::path::{Path, PathBuf}; - -use anyhow::{Context, bail}; -use fabro_auth::{OAuthConfig, OAuthCredential, OAuthTokens}; -use serde::Deserialize; -use serde_json::{Map, Value}; - -pub(crate) const REMOVAL_DEADLINE: &str = "2026-08-18"; - -#[derive(Debug, Default, PartialEq, Eq)] -pub(crate) struct LegacyVaultMigrationReport { - pub(crate) migrated_entries: usize, - pub(crate) skipped_entries: usize, - pub(crate) backup_path: Option, -} - -impl LegacyVaultMigrationReport { - pub(crate) fn changed(&self) -> bool { - self.migrated_entries > 0 || self.skipped_entries > 0 - } -} - -#[derive(Debug, Deserialize)] -struct LegacyAuthCredential { - provider: String, - #[serde(flatten)] - details: LegacyAuthDetails, -} - -#[derive(Debug, Deserialize)] -#[serde(tag = "type", rename_all = "snake_case")] -enum LegacyAuthDetails { - ApiKey { - key: String, - }, - CodexOauth { - tokens: OAuthTokens, - config: OAuthConfig, - #[serde(default)] - account_id: Option, - }, -} - -pub(crate) fn migrate_legacy_vault_file(path: &Path) -> anyhow::Result { - let contents = match std::fs::read_to_string(path) { - Ok(contents) => contents, - Err(err) if err.kind() == std::io::ErrorKind::NotFound => { - return Ok(LegacyVaultMigrationReport::default()); - } - Err(err) => return Err(err).with_context(|| format!("read vault {}", path.display())), - }; - let entries = parse_vault_entries(&contents)?; - let (next_entries, migrated_entries, skipped_entries) = rewrite_entries(entries); - let mut report = LegacyVaultMigrationReport { - migrated_entries, - skipped_entries, - backup_path: None, - }; - if !report.changed() { - return Ok(report); - } - - let backup_path = backup_vault_file(path)?; - write_vault_entries(path, &next_entries)?; - report.backup_path = Some(backup_path); - Ok(report) -} - -fn parse_vault_entries(contents: &str) -> anyhow::Result> { - let value: Value = serde_json::from_str(contents).context("parse vault JSON")?; - match value { - Value::Object(entries) => Ok(entries), - _ => bail!("vault JSON root must be an object"), - } -} - -fn rewrite_entries(entries: Map) -> (Map, usize, usize) { - let mut next_entries = Map::new(); - let mut occupied = HashSet::new(); - for (name, entry) in &entries { - if matches!( - entry.get("type").and_then(Value::as_str), - Some("token" | "oauth" | "file") - ) { - next_entries.insert(name.clone(), entry.clone()); - occupied.insert(name.clone()); - } - } - - let mut migrated_entries = 0; - let mut skipped_entries = 0; - for (name, entry) in entries { - match entry.get("type").and_then(Value::as_str) { - Some("token" | "oauth" | "file") => {} - Some("environment") => { - if insert_rewritten_entry( - &mut next_entries, - &mut occupied, - name, - rewrite_entry(entry, "token", None), - ) { - migrated_entries += 1; - } else { - skipped_entries += 1; - } - } - Some("credential") => match legacy_credential_entry(&name, &entry) { - Some((target_name, rewritten)) => { - if insert_rewritten_entry( - &mut next_entries, - &mut occupied, - target_name, - rewritten, - ) { - migrated_entries += 1; - } else { - skipped_entries += 1; - } - } - None => skipped_entries += 1, - }, - _ => skipped_entries += 1, - } - } - - (next_entries, migrated_entries, skipped_entries) -} - -fn insert_rewritten_entry( - entries: &mut Map, - occupied: &mut HashSet, - name: String, - entry: Value, -) -> bool { - if !occupied.insert(name.clone()) { - return false; - } - entries.insert(name, entry); - true -} - -fn legacy_credential_entry(name: &str, entry: &Value) -> Option<(String, Value)> { - let value = entry.get("value").and_then(Value::as_str)?; - let credential: LegacyAuthCredential = serde_json::from_str(value).ok()?; - match credential.details { - LegacyAuthDetails::ApiKey { key } => { - let target_name = api_key_secret_name(&credential.provider)?; - Some(( - target_name, - rewrite_entry(entry.clone(), "token", Some(key)), - )) - } - LegacyAuthDetails::CodexOauth { - tokens, - config, - account_id, - } if credential.provider == "openai" && name == "openai_codex" => { - let credential = OAuthCredential { - tokens, - config, - account_id, - }; - let value = serde_json::to_string(&credential).ok()?; - Some(( - "OPENAI_CODEX".to_string(), - rewrite_entry(entry.clone(), "oauth", Some(value)), - )) - } - LegacyAuthDetails::CodexOauth { .. } => None, - } -} - -fn rewrite_entry(mut entry: Value, secret_type: &str, value: Option) -> Value { - if let Value::Object(fields) = &mut entry { - fields.insert("type".to_string(), Value::String(secret_type.to_string())); - if let Some(value) = value { - fields.insert("value".to_string(), Value::String(value)); - } - } - entry -} - -fn api_key_secret_name(provider: &str) -> Option { - let mut name = String::new(); - for ch in provider.chars() { - if ch.is_ascii_alphanumeric() { - name.push(ch.to_ascii_uppercase()); - } else if !name.ends_with('_') { - name.push('_'); - } - } - while name.ends_with('_') { - name.pop(); - } - if name.is_empty() { - return None; - } - if !name.ends_with("_API_KEY") { - name.push_str("_API_KEY"); - } - Some(name) -} - -fn backup_vault_file(path: &Path) -> anyhow::Result { - let parent = path.parent().unwrap_or_else(|| Path::new(".")); - let file_name = path - .file_name() - .and_then(|name| name.to_str()) - .unwrap_or("secrets.json"); - let backup_path = parent.join(format!( - ".{file_name}.legacy-vault-migration-{}.bak", - ulid::Ulid::new() - )); - std::fs::copy(path, &backup_path).with_context(|| { - format!( - "copy vault {} to backup {}", - path.display(), - backup_path.display() - ) - })?; - set_private_permissions(&backup_path)?; - Ok(backup_path) -} - -fn write_vault_entries(path: &Path, entries: &Map) -> anyhow::Result<()> { - let parent = path.parent().unwrap_or_else(|| Path::new(".")); - std::fs::create_dir_all(parent) - .with_context(|| format!("create vault directory {}", parent.display()))?; - let file_name = path - .file_name() - .and_then(|name| name.to_str()) - .unwrap_or("secrets.json"); - let tmp_path = parent.join(format!( - ".{file_name}.legacy-vault-migration-tmp-{}", - ulid::Ulid::new() - )); - let json = serde_json::to_vec_pretty(entries).context("serialize migrated vault JSON")?; - std::fs::write(&tmp_path, json) - .with_context(|| format!("write migrated vault temp file {}", tmp_path.display()))?; - set_private_permissions(&tmp_path)?; - std::fs::rename(&tmp_path, path).with_context(|| { - format!( - "rename migrated vault temp file {} to {}", - tmp_path.display(), - path.display() - ) - })?; - Ok(()) -} - -#[cfg(unix)] -fn set_private_permissions(path: &Path) -> anyhow::Result<()> { - use std::os::unix::fs::PermissionsExt; - - std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)) - .with_context(|| format!("set private permissions on {}", path.display()))?; - Ok(()) -} - -#[cfg(not(unix))] -fn set_private_permissions(_path: &Path) -> anyhow::Result<()> { - Ok(()) -} diff --git a/lib/apps/fabro-server/migrations/2026052501_optional_server_env_secrets_to_vault.rs b/lib/apps/fabro-server/migrations/2026052501_optional_server_env_secrets_to_vault.rs deleted file mode 100644 index ab2f03650..000000000 --- a/lib/apps/fabro-server/migrations/2026052501_optional_server_env_secrets_to_vault.rs +++ /dev/null @@ -1,172 +0,0 @@ -//! Temporary compatibility shim for optional secrets that used to live in -//! `server.env`. -//! -//! Delete this migration after 2026-08-18, once supported installs have had a -//! release window to move optional integration secrets into the vault. - -#![expect( - clippy::disallowed_methods, - reason = "Temporary startup migration uses synchronous file I/O before serving requests." -)] - -use std::collections::HashMap; -use std::path::{Path, PathBuf}; - -use anyhow::Context as _; -use fabro_config::envfile::{self, EnvFileRemoval}; -use fabro_static::{EnvVars, optional_vault_secrets}; -use fabro_vault::{SecretStore, SecretStoreWrite, SecretType}; - -pub(crate) const REMOVAL_DEADLINE: &str = "2026-08-18"; - -#[derive(Debug, Default, PartialEq, Eq)] -pub(crate) struct OptionalServerEnvSecretsMigrationReport { - pub(crate) migrated_secrets: usize, - pub(crate) removed_env_entries: usize, - pub(crate) preserved_env_entries: usize, - pub(crate) backup_path: Option, - pub(crate) warnings: Vec, -} - -impl OptionalServerEnvSecretsMigrationReport { - pub(crate) fn changed(&self) -> bool { - self.migrated_secrets > 0 || self.removed_env_entries > 0 - } -} - -pub(crate) async fn migrate_to_store( - store: &SecretStore, - server_env_path: &Path, - env_entries: &HashMap, -) -> anyhow::Result { - let server_env_entries = envfile::read_env_file(server_env_path) - .with_context(|| format!("read server env file {}", server_env_path.display()))?; - let stored = store.snapshot().await?; - let mut writes = Vec::new(); - let mut env_removals = Vec::new(); - let mut warnings = Vec::new(); - let mut preserved_env_entries = 0; - - for &name in optional_vault_secrets() { - let process_value = env_entries.get(name); - let file_value = server_env_entries.get(name); - if let Some(entry) = stored.get_entry(name) { - if let Some(file_value) = file_value { - if file_value == &entry.value { - env_removals.push(env_removal(name)); - } else { - preserved_env_entries += 1; - warnings.push(format!( - "Preserved {name} in server.env because the secret store already contains a different value" - )); - } - } - continue; - } - - let value = match (process_value, file_value) { - (Some(value), Some(file_value)) => { - if value == file_value { - env_removals.push(env_removal(name)); - } else { - preserved_env_entries += 1; - warnings.push(format!( - "Preserved {name} in server.env because process env takes precedence and the file value differs" - )); - } - Some(value) - } - (Some(value), None) => Some(value), - (None, Some(value)) => { - env_removals.push(env_removal(name)); - Some(value) - } - (None, None) => None, - }; - if let Some(value) = value { - writes.push(SecretStoreWrite { - name: name.to_string(), - value: value.clone(), - secret_type: secret_type_for(name), - description: None, - }); - } - } - - let mut report = OptionalServerEnvSecretsMigrationReport { - migrated_secrets: writes.len(), - removed_env_entries: 0, - preserved_env_entries, - backup_path: None, - warnings, - }; - if writes.is_empty() && env_removals.is_empty() { - return Ok(report); - } - - store.apply(&[], &writes).await?; - if !env_removals.is_empty() { - let backup_path = backup_server_env_file(server_env_path)?; - let update_report = - envfile::update_env_file_with_report(server_env_path, env_removals, Vec::new()) - .with_context(|| { - format!( - "remove migrated optional secrets from {}", - server_env_path.display() - ) - })?; - report.removed_env_entries = update_report.removed_keys.len(); - report.backup_path = Some(backup_path); - } - Ok(report) -} - -fn secret_type_for(name: &str) -> SecretType { - if name == EnvVars::GITHUB_APP_PRIVATE_KEY { - SecretType::File - } else { - SecretType::Token - } -} - -fn env_removal(name: &str) -> EnvFileRemoval { - EnvFileRemoval { - key: name.to_string(), - comment: None, - } -} - -fn backup_server_env_file(path: &Path) -> anyhow::Result { - let parent = path.parent().unwrap_or_else(|| Path::new(".")); - let file_name = path - .file_name() - .and_then(|name| name.to_str()) - .unwrap_or("server.env"); - let backup_path = parent.join(format!( - ".{file_name}.optional-server-env-secrets-to-vault-migration-{}.bak", - ulid::Ulid::new() - )); - std::fs::copy(path, &backup_path).with_context(|| { - format!( - "copy server env {} to backup {}", - path.display(), - backup_path.display() - ) - })?; - set_private_permissions(&backup_path)?; - Ok(backup_path) -} - -#[cfg(unix)] -fn set_private_permissions(path: &Path) -> anyhow::Result<()> { - use std::os::unix::fs::PermissionsExt; - - std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)) - .with_context(|| format!("set permissions on {}", path.display()))?; - Ok(()) -} - -#[cfg(not(unix))] -fn set_private_permissions(_path: &Path) -> anyhow::Result<()> { - Ok(()) -} diff --git a/lib/apps/fabro-server/src/lib.rs b/lib/apps/fabro-server/src/lib.rs index d58c92f51..98f7a2c77 100644 --- a/lib/apps/fabro-server/src/lib.rs +++ b/lib/apps/fabro-server/src/lib.rs @@ -59,4 +59,4 @@ mod worker_token; pub use error::{ApiError, Error, Result}; pub use run_manifest::workflow_bundle_from_manifest; pub use server_secrets::process_env_snapshot; -pub use startup::{migrate_startup_vault, validate_startup, validate_startup_configuration}; +pub use startup::{validate_startup, validate_startup_configuration}; diff --git a/lib/apps/fabro-server/src/migrations.rs b/lib/apps/fabro-server/src/migrations.rs index 5c89015d0..68c4f9bd5 100644 --- a/lib/apps/fabro-server/src/migrations.rs +++ b/lib/apps/fabro-server/src/migrations.rs @@ -1,12 +1,3 @@ -use std::collections::HashMap; -use std::path::Path; - -use fabro_vault::SecretStore; - -#[path = "../migrations/2026051801_legacy_vault_entries.rs"] -mod legacy_vault_entries; -#[path = "../migrations/2026052501_optional_server_env_secrets_to_vault.rs"] -mod optional_server_env_secrets_to_vault; #[path = "../migrations/sqlite_activation_backup.rs"] mod sqlite_activation_backup; #[path = "../migrations/2026082301_sqlite_blob_activation.rs"] @@ -14,24 +5,5 @@ mod sqlite_blob_activation; #[path = "../migrations/2026082801_sqlite_run_history_activation.rs"] mod sqlite_run_history_activation; -pub(crate) use legacy_vault_entries::REMOVAL_DEADLINE as LEGACY_VAULT_REMOVAL_DEADLINE; -pub(crate) use optional_server_env_secrets_to_vault::REMOVAL_DEADLINE as OPTIONAL_SERVER_ENV_SECRETS_REMOVAL_DEADLINE; pub(crate) use sqlite_blob_activation::activate_blob_storage; pub(crate) use sqlite_run_history_activation::activate_run_history; - -pub(crate) type LegacyVaultMigrationReport = legacy_vault_entries::LegacyVaultMigrationReport; -pub(crate) type OptionalServerEnvSecretsMigrationReport = - optional_server_env_secrets_to_vault::OptionalServerEnvSecretsMigrationReport; - -pub(crate) fn migrate_legacy_vault_file(path: &Path) -> anyhow::Result { - legacy_vault_entries::migrate_legacy_vault_file(path) -} - -pub(crate) async fn migrate_optional_server_env_secrets_to_store( - store: &SecretStore, - server_env_path: &Path, - env_entries: &HashMap, -) -> anyhow::Result { - optional_server_env_secrets_to_vault::migrate_to_store(store, server_env_path, env_entries) - .await -} diff --git a/lib/apps/fabro-server/src/serve.rs b/lib/apps/fabro-server/src/serve.rs index 7cbe5ac7f..bcb4432b1 100644 --- a/lib/apps/fabro-server/src/serve.rs +++ b/lib/apps/fabro-server/src/serve.rs @@ -39,7 +39,7 @@ use crate::server::{ spawn_automation_scheduler, spawn_pull_request_creation_supervisor, spawn_scheduler, }; use crate::server_secrets::{ServerSecrets, process_env_snapshot}; -use crate::startup::{migrate_startup_vault, resolve_startup, validate_startup_configuration}; +use crate::startup::{resolve_startup, validate_startup_configuration}; use crate::{migrations, static_files}; pub const DEFAULT_TCP_PORT: u16 = 32276; @@ -666,7 +666,6 @@ where let resolved_server_settings = resolved_app_settings.server_settings.server.clone(); validate_startup_configuration(&resolved_server_settings)?; let env_entries = process_env_snapshot(); - migrate_startup_vault(&vault_path); let bind_request = resolve_bind_request_from_server_settings( &resolved_app_settings.server_settings, args.bind.as_deref(), @@ -680,30 +679,6 @@ where .await .with_context(|| format!("importing legacy secrets file {}", vault_path.display()))?; let secret_store = fabro_vault::SecretStore::new(database.clone_pool()); - let optional_report = migrations::migrate_optional_server_env_secrets_to_store( - &secret_store, - &server_env_path, - &env_entries, - ) - .await - .context("migrate optional server env secrets into SQLite")?; - for warning in &optional_report.warnings { - warn!( - warning = %warning, - removal_deadline = migrations::OPTIONAL_SERVER_ENV_SECRETS_REMOVAL_DEADLINE, - "Optional server env secrets migration warning" - ); - } - if optional_report.changed() { - warn!( - migrated_secrets = optional_report.migrated_secrets, - removed_env_entries = optional_report.removed_env_entries, - preserved_env_entries = optional_report.preserved_env_entries, - backup_path = ?optional_report.backup_path, - removal_deadline = migrations::OPTIONAL_SERVER_ENV_SECRETS_REMOVAL_DEADLINE, - "Migrated optional server env secrets into SQLite" - ); - } let startup_vault = secret_store .snapshot() .await diff --git a/lib/apps/fabro-server/src/server/tests.rs b/lib/apps/fabro-server/src/server/tests.rs index 2e969bdff..ff8604b88 100644 --- a/lib/apps/fabro-server/src/server/tests.rs +++ b/lib/apps/fabro-server/src/server/tests.rs @@ -2622,128 +2622,6 @@ methods = ["dev-token"] )); } -#[tokio::test] -async fn build_app_state_migrates_legacy_vault_file_on_boot() { - let vault_path = test_secret_store_path(); - let timestamp = "2026-05-18T12:00:00Z"; - let legacy_api_key = json!({ - "provider": "anthropic", - "type": "api_key", - "key": "sk-ant-legacy", - }); - let legacy_oauth = json!({ - "provider": "openai", - "type": "codex_oauth", - "tokens": { - "access_token": "codex-access", - "refresh_token": "codex-refresh", - "expires_at": "2026-05-18T13:00:00Z", - }, - "config": { - "auth_url": "https://auth.openai.com", - "token_url": "https://auth.openai.com/oauth/token", - "client_id": "client", - "scopes": ["openid", "offline_access"], - "redirect_uri": "https://auth.openai.com/deviceauth/callback", - "use_pkce": false, - }, - "account_id": "acct_legacy", - }); - let legacy_vault = json!({ - "anthropic": { - "value": legacy_api_key.to_string(), - "type": "credential", - "created_at": timestamp, - "updated_at": timestamp, - }, - "openai_codex": { - "value": legacy_oauth.to_string(), - "type": "credential", - "created_at": timestamp, - "updated_at": timestamp, - }, - "GITHUB_TOKEN": { - "value": "ghp_legacy", - "type": "environment", - "created_at": timestamp, - "updated_at": timestamp, - }, - "/tmp/github.pem": { - "value": "/tmp/github.pem", - "type": "file", - "created_at": timestamp, - "updated_at": timestamp, - }, - }); - std::fs::write( - &vault_path, - serde_json::to_vec_pretty(&legacy_vault).unwrap(), - ) - .expect("legacy vault should be writable"); - - let state = build_test_app_state_with_vault_path(&vault_path) - .expect("legacy vault should not prevent server boot"); - - let vault = state.stores.vault.snapshot().await.unwrap(); - let api_key_entry = vault - .get_entry("ANTHROPIC_API_KEY") - .expect("legacy provider credential should be migrated to token name"); - assert_eq!(api_key_entry.secret_type, SecretType::Token); - assert_eq!(api_key_entry.value, "sk-ant-legacy"); - assert!(vault.get_entry("anthropic").is_none()); - - let oauth_entry = vault - .get_entry("OPENAI_CODEX") - .expect("legacy Codex credential should be migrated to canonical OAuth name"); - assert_eq!(oauth_entry.secret_type, SecretType::Oauth); - let oauth: fabro_auth::OAuthCredential = - serde_json::from_str(&oauth_entry.value).expect("migrated OAuth JSON should parse"); - assert_eq!(oauth.tokens.access_token, "codex-access"); - assert_eq!(oauth.account_id.as_deref(), Some("acct_legacy")); - assert!(vault.get_entry("openai_codex").is_none()); - - assert_eq!( - vault.get_entry("GITHUB_TOKEN").unwrap().secret_type, - SecretType::Token - ); - assert_eq!( - vault.get_entry("/tmp/github.pem").unwrap().secret_type, - SecretType::File - ); -} - -fn build_test_app_state_with_vault_path(vault_path: &Path) -> anyhow::Result> { - let (store, artifact_store) = test_store_bundle(); - let db_pool = test_db_pool_for_vault_path(vault_path)?; - let preloaded_vault = crate::test_support::test_secret_snapshot(db_pool.clone())?; - build_app_state(AppStateConfig { - resolved_settings: resolved_runtime_settings_for_tests( - default_test_server_settings(), - RunLayer::default(), - LlmCatalogSettings::default(), - ), - registry_factory_override: None, - max_concurrent_runs: 5, - store, - artifact_store, - db_pool, - preloaded_vault, - server_secrets: load_test_server_secrets( - vault_path.with_file_name("server.env"), - HashMap::new(), - ), - env_lookup: default_env_lookup(), - github_api_base_url: None, - active_config_path: tempfile::tempdir().unwrap().path().join("settings.toml"), - http_client: Some(fabro_http::test_http_client().expect("test HTTP client should build")), - sandbox_provider_registry: None, - shutdown: tokio_util::sync::CancellationToken::new(), - worker_control_bus: None, - worker_runtime: None, - automation_materializer_override: None, - }) -} - fn test_worker_ref(pid: u32) -> WorkerRef { WorkerRef::Local { pid } } diff --git a/lib/apps/fabro-server/src/startup.rs b/lib/apps/fabro-server/src/startup.rs index 78fef39cb..bdf343c36 100644 --- a/lib/apps/fabro-server/src/startup.rs +++ b/lib/apps/fabro-server/src/startup.rs @@ -4,10 +4,8 @@ use std::path::Path; use fabro_static::EnvVars; use fabro_types::settings::ServerNamespace; use fabro_vault::Vault; -use tracing::warn; use crate::jwt_auth::{AuthMode, resolve_auth_mode_with_lookup, validate_auth_configuration}; -use crate::migrations; use crate::server_secrets::ServerSecrets; pub(crate) fn resolve_startup( @@ -25,33 +23,6 @@ pub(crate) fn resolve_startup( Ok((auth_mode, server_secrets)) } -pub fn migrate_startup_vault(vault_path: impl AsRef) { - let vault_path = vault_path.as_ref(); - match migrations::migrate_legacy_vault_file(vault_path) { - Ok(report) if report.changed() => { - let backup_path = report - .backup_path - .as_ref() - .map_or_else(|| "".to_string(), |path| path.display().to_string()); - warn!( - migrated_entries = report.migrated_entries, - skipped_entries = report.skipped_entries, - backup_path = %backup_path, - removal_deadline = migrations::LEGACY_VAULT_REMOVAL_DEADLINE, - "Migrated legacy vault file" - ); - } - Ok(_) => {} - Err(err) => { - warn!( - error = %err, - removal_deadline = migrations::LEGACY_VAULT_REMOVAL_DEADLINE, - "Legacy vault migration failed; continuing with normal vault load" - ); - } - } -} - pub fn validate_startup( env_path: &Path, env_entries: HashMap, @@ -68,15 +39,13 @@ pub fn validate_startup_configuration(settings: &ServerNamespace) -> anyhow::Res #[cfg(test)] mod tests { use std::collections::HashMap; - use std::path::{Path, PathBuf}; - use fabro_config::{ServerSettingsBuilder, envfile}; + use fabro_config::ServerSettingsBuilder; use fabro_static::EnvVars; use fabro_types::settings::ServerNamespace; - use fabro_vault::{SecretStore, SecretType, Vault}; + use fabro_vault::{SecretType, Vault}; use super::validate_startup; - use crate::migrations; fn resolved_settings(auth_methods: &[&str]) -> ServerNamespace { ServerSettingsBuilder::from_toml(&format!( @@ -106,36 +75,6 @@ client_id = "Iv1.test" Vault::load(dir.path().join("secrets.json")).unwrap() } - fn env_path(dir: &tempfile::TempDir) -> PathBuf { - dir.path().join("server.env") - } - - async fn test_secret_store(dir: &tempfile::TempDir) -> SecretStore { - let database = fabro_db::Database::connect(dir.path().join("fabro.db")) - .await - .unwrap(); - database.migrate().await.unwrap(); - SecretStore::new(database.clone_pool()) - } - - #[expect( - clippy::disallowed_methods, - reason = "test helper scans a temporary directory after startup migration completes" - )] - fn migration_backups(dir: &Path) -> Vec { - std::fs::read_dir(dir) - .unwrap() - .map(|entry| entry.unwrap().path()) - .filter(|path| { - path.file_name() - .and_then(|name| name.to_str()) - .is_some_and(|name| { - name.contains("optional-server-env-secrets-to-vault-migration") - }) - }) - .collect() - } - #[test] fn validate_startup_accepts_configured_secrets() { let dir = tempfile::tempdir().unwrap(); @@ -234,185 +173,4 @@ client_id = "Iv1.test" ) .expect("github client secret in vault should satisfy startup"); } - - #[tokio::test] - async fn migrate_optional_secrets_moves_server_env_secrets_to_store() { - let dir = tempfile::tempdir().unwrap(); - let server_env_path = env_path(&dir); - envfile::write_env_file( - &server_env_path, - &HashMap::from([ - ( - EnvVars::SESSION_SECRET.to_string(), - "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef".to_string(), - ), - ( - EnvVars::GITHUB_APP_CLIENT_SECRET.to_string(), - "legacy-client-secret".to_string(), - ), - ( - EnvVars::GITHUB_APP_PRIVATE_KEY.to_string(), - "legacy-private-key".to_string(), - ), - (EnvVars::OPENAI_API_KEY.to_string(), "sk-legacy".to_string()), - ]), - ) - .unwrap(); - let store = test_secret_store(&dir).await; - - migrations::migrate_optional_server_env_secrets_to_store( - &store, - &server_env_path, - &HashMap::new(), - ) - .await - .expect("legacy optional secrets should migrate"); - - let client_secret = store - .get(EnvVars::GITHUB_APP_CLIENT_SECRET) - .await - .unwrap() - .expect("client secret should be stored"); - assert_eq!(client_secret.value, "legacy-client-secret"); - assert_eq!(client_secret.secret_type, SecretType::Token); - let private_key = store - .get(EnvVars::GITHUB_APP_PRIVATE_KEY) - .await - .unwrap() - .expect("private key should be stored"); - assert_eq!(private_key.value, "legacy-private-key"); - assert_eq!(private_key.secret_type, SecretType::File); - let openai_key = store - .get(EnvVars::OPENAI_API_KEY) - .await - .unwrap() - .expect("openai key should be stored"); - assert_eq!(openai_key.value, "sk-legacy"); - - let server_env = envfile::read_env_file(&server_env_path).unwrap(); - assert!(server_env.contains_key(EnvVars::SESSION_SECRET)); - assert!(!server_env.contains_key(EnvVars::GITHUB_APP_CLIENT_SECRET)); - assert!(!server_env.contains_key(EnvVars::GITHUB_APP_PRIVATE_KEY)); - assert!(!server_env.contains_key(EnvVars::OPENAI_API_KEY)); - assert_eq!(migration_backups(dir.path()).len(), 1); - } - - #[tokio::test] - async fn migrate_optional_secrets_prefers_process_env_and_preserves_conflicting_server_env() { - let dir = tempfile::tempdir().unwrap(); - let server_env_path = env_path(&dir); - envfile::write_env_file( - &server_env_path, - &HashMap::from([( - EnvVars::GITHUB_APP_CLIENT_SECRET.to_string(), - "file-client-secret".to_string(), - )]), - ) - .unwrap(); - let env_entries = HashMap::from([( - EnvVars::GITHUB_APP_CLIENT_SECRET.to_string(), - "process-client-secret".to_string(), - )]); - let store = test_secret_store(&dir).await; - - migrations::migrate_optional_server_env_secrets_to_store( - &store, - &server_env_path, - &env_entries, - ) - .await - .expect("process env secret should migrate"); - - let client_secret = store - .get(EnvVars::GITHUB_APP_CLIENT_SECRET) - .await - .unwrap() - .expect("client secret should be stored"); - assert_eq!(client_secret.value, "process-client-secret"); - let server_env = envfile::read_env_file(&server_env_path).unwrap(); - assert_eq!( - server_env - .get(EnvVars::GITHUB_APP_CLIENT_SECRET) - .map(String::as_str), - Some("file-client-secret") - ); - assert!(migration_backups(dir.path()).is_empty()); - } - - #[tokio::test] - async fn migrate_optional_secrets_keeps_existing_stored_secret_and_removes_matching_server_env() - { - let dir = tempfile::tempdir().unwrap(); - let server_env_path = env_path(&dir); - envfile::write_env_file( - &server_env_path, - &HashMap::from([( - EnvVars::GITHUB_APP_CLIENT_SECRET.to_string(), - "vault-client-secret".to_string(), - )]), - ) - .unwrap(); - let store = test_secret_store(&dir).await; - store - .set( - EnvVars::GITHUB_APP_CLIENT_SECRET, - "vault-client-secret", - SecretType::Token, - None, - ) - .await - .unwrap(); - - migrations::migrate_optional_server_env_secrets_to_store( - &store, - &server_env_path, - &HashMap::new(), - ) - .await - .expect("redundant server env secret should be cleaned up"); - - let client_secret = store - .get(EnvVars::GITHUB_APP_CLIENT_SECRET) - .await - .unwrap() - .expect("client secret should be stored"); - assert_eq!(client_secret.value, "vault-client-secret"); - let server_env = envfile::read_env_file(&server_env_path).unwrap(); - assert!(!server_env.contains_key(EnvVars::GITHUB_APP_CLIENT_SECRET)); - assert_eq!(migration_backups(dir.path()).len(), 1); - } - - #[tokio::test] - async fn migrate_optional_secrets_migrated_github_client_secret_satisfies_startup() { - let dir = tempfile::tempdir().unwrap(); - let server_env_path = env_path(&dir); - envfile::write_env_file( - &server_env_path, - &HashMap::from([ - ( - EnvVars::SESSION_SECRET.to_string(), - "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef".to_string(), - ), - ( - EnvVars::GITHUB_APP_CLIENT_SECRET.to_string(), - "legacy-client-secret".to_string(), - ), - ]), - ) - .unwrap(); - let settings = resolved_settings(&["github"]); - let store = test_secret_store(&dir).await; - - migrations::migrate_optional_server_env_secrets_to_store( - &store, - &server_env_path, - &HashMap::new(), - ) - .await - .expect("legacy github client secret should migrate"); - - let vault = store.snapshot().await.unwrap().into_vault(); - validate_startup(&server_env_path, HashMap::new(), &settings, &vault) - .expect("migrated github client secret should satisfy startup"); - } } diff --git a/lib/apps/fabro-server/src/test_support.rs b/lib/apps/fabro-server/src/test_support.rs index 8b0dd2185..3593fad6a 100644 --- a/lib/apps/fabro-server/src/test_support.rs +++ b/lib/apps/fabro-server/src/test_support.rs @@ -33,6 +33,7 @@ use tokio::runtime::Builder as TokioRuntimeBuilder; use tokio_util::sync::CancellationToken; use ulid::Ulid; +use crate::auth; pub use crate::automation_materializer::TestAutomationRunMaterializer; use crate::interp::process_env_var; use crate::jwt_auth::{AuthMode, ConfiguredAuth}; @@ -45,7 +46,6 @@ use crate::server::{ use crate::server_secrets::ServerSecrets; #[cfg(test)] use crate::worker_runtime::WorkerRuntime; -use crate::{auth, migrations}; pub const TEST_DEV_TOKEN: &str = "fabro_dev_abababababababababababababababababababababababababababababababab"; @@ -600,7 +600,6 @@ fn test_db_pool( default_environment_provider: Option, ) -> anyhow::Result { std::thread::spawn(move || { - migrations::migrate_legacy_vault_file(&vault_path)?; let runtime = TokioRuntimeBuilder::new_current_thread() .enable_all() .build()?;