From 591be689cfa638092d1c295ef0e06b5156b717ae Mon Sep 17 00:00:00 2001 From: shiva Date: Sat, 11 Jul 2026 17:19:20 +0530 Subject: [PATCH 1/2] refactor: consolidate gateway secrets into agentflare.db (closes #138) --- src/cli/gateway.rs | 5 ++--- src/db.rs | 49 +++++++++++++++++++++++++++++++++++++++++- src/gateway_secrets.rs | 29 ++++++++++++------------- src/mcp_server.rs | 12 ++--------- 4 files changed, 66 insertions(+), 29 deletions(-) diff --git a/src/cli/gateway.rs b/src/cli/gateway.rs index 742da114b..a4de1b869 100644 --- a/src/cli/gateway.rs +++ b/src/cli/gateway.rs @@ -34,11 +34,10 @@ impl GatewayArgs { } fn run_secret(action: GatewaySecretAction) { - let db_path = crate::paths::home().join(".agentflare").join("gateway.db"); - let conn = match crate::gateway_secrets::open_db(&db_path) { + let conn = match crate::db::open() { Ok(c) => c, Err(e) => { - eprintln!("failed to open gateway secrets db: {e}"); + eprintln!("failed to open agentflare.db: {e}"); std::process::exit(1); } }; diff --git a/src/db.rs b/src/db.rs index c82387632..9a9539de9 100644 --- a/src/db.rs +++ b/src/db.rs @@ -3,7 +3,7 @@ //! (see #138 — gateway secrets fold in later). The rebuildable caches under //! `~/.local/share/agentflare/` (skills index, gateway tool-index) stay //! separate: they belong in the data dir, not next to source-of-truth state. -use rusqlite::Connection; +use rusqlite::{params, Connection}; use std::path::PathBuf; use std::time::Duration; @@ -11,6 +11,10 @@ pub fn agentflare_db_path() -> PathBuf { crate::paths::home().join(".agentflare").join("agentflare.db") } +fn old_gateway_db_path() -> PathBuf { + crate::paths::home().join(".agentflare").join("gateway.db") +} + /// Opens (creating if absent) `agentflare.db` and applies every table's /// migration. Each subsystem owns its own `CREATE TABLE IF NOT EXISTS` so /// this stays a thin dispatcher. @@ -26,9 +30,52 @@ pub fn open() -> rusqlite::Result { restrict(&path, 0o600); tune(&conn)?; crate::claims::migrate(&conn)?; + crate::gateway_secrets::migrate(&conn)?; + // One-time migration: copy secrets from old gateway.db + // (pre-#138 separate file) into agentflare.db. + migrate_old_gateway_db(&conn)?; Ok(conn) } +/// If `~/.agentflare/gateway.db` exists with a `gateway_secrets` table, +/// copy rows into `agentflare.db` that don't already exist (so re-running +/// the migration is idempotent). Leaves the old file in place. +fn migrate_old_gateway_db(conn: &Connection) -> rusqlite::Result<()> { + let old_path = old_gateway_db_path(); + if !old_path.exists() { + return Ok(()); + } + // Probe: does the old file have a gateway_secrets table with rows? + let old = match Connection::open(&old_path) { + Ok(c) => c, + Err(_) => return Ok(()), + }; + let count: i64 = match old.query_row( + "SELECT COUNT(*) FROM gateway_secrets", + [], + |r| r.get(0), + ) { + Ok(n) => n, + Err(_) => return Ok(()), + }; + if count == 0 { + return Ok(()); + } + // Copy each row that doesn't already exist in the new db. + let mut stmt = old.prepare("SELECT name, ciphertext FROM gateway_secrets")?; + let rows = stmt.query_map([], |r| { + Ok((r.get::<_, String>(0)?, r.get::<_, Vec>(1)?)) + })?; + for row in rows { + let (name, ciphertext) = row?; + conn.execute( + "INSERT OR IGNORE INTO gateway_secrets (name, ciphertext) VALUES (?1, ?2)", + params![name, ciphertext], + )?; + } + Ok(()) +} + /// Best-effort owner-only permissions (no-op off Unix). #[cfg(unix)] fn restrict(path: &std::path::Path, mode: u32) { diff --git a/src/gateway_secrets.rs b/src/gateway_secrets.rs index accd28226..f4b5ff59d 100644 --- a/src/gateway_secrets.rs +++ b/src/gateway_secrets.rs @@ -8,18 +8,15 @@ use crate::auth_crypt; use rusqlite::{params, Connection, OptionalExtension}; use std::path::Path; -pub fn open_db(path: &Path) -> rusqlite::Result { - if let Some(parent) = path.parent() { - let _ = std::fs::create_dir_all(parent); - } - let conn = Connection::open(path)?; +/// Creates the `gateway_secrets` table in an existing connection (called +/// from `db::open()` so the table lives in `agentflare.db`). +pub fn migrate(conn: &Connection) -> rusqlite::Result<()> { conn.execute_batch( "CREATE TABLE IF NOT EXISTS gateway_secrets ( name TEXT PRIMARY KEY, ciphertext BLOB NOT NULL );", - )?; - Ok(conn) + ) } #[derive(Debug)] @@ -112,12 +109,17 @@ mod tests { unsafe { std::env::remove_var("AGENTFLARE_VAULT_PASSPHRASE") }; } + fn mem_migrated() -> Connection { + let conn = Connection::open_in_memory().unwrap(); + migrate(&conn).unwrap(); + conn + } + #[test] fn roundtrip_set_get() { let _guard = GLOBAL_STATE_LOCK.lock().unwrap_or_else(|e| e.into_inner()); set_passphrase("test-pass"); - let conn = Connection::open_in_memory().unwrap(); - conn.execute_batch("CREATE TABLE gateway_secrets (name TEXT PRIMARY KEY, ciphertext BLOB NOT NULL);").unwrap(); + let conn = mem_migrated(); set_secret(&conn, "github_pat", "ghp_abc123").unwrap(); assert_eq!(get_secret(&conn, "github_pat").unwrap(), Some("ghp_abc123".to_string())); clear_passphrase(); @@ -126,8 +128,7 @@ mod tests { #[test] fn get_missing_secret_returns_none() { let _guard = GLOBAL_STATE_LOCK.lock().unwrap_or_else(|e| e.into_inner()); - let conn = Connection::open_in_memory().unwrap(); - conn.execute_batch("CREATE TABLE gateway_secrets (name TEXT PRIMARY KEY, ciphertext BLOB NOT NULL);").unwrap(); + let conn = mem_migrated(); assert_eq!(get_secret(&conn, "missing").unwrap(), None); } @@ -135,8 +136,7 @@ mod tests { fn wrong_passphrase_is_a_clear_error_not_silent_empty_string() { let _guard = GLOBAL_STATE_LOCK.lock().unwrap_or_else(|e| e.into_inner()); set_passphrase("right-pass"); - let conn = Connection::open_in_memory().unwrap(); - conn.execute_batch("CREATE TABLE gateway_secrets (name TEXT PRIMARY KEY, ciphertext BLOB NOT NULL);").unwrap(); + let conn = mem_migrated(); set_secret(&conn, "s", "value").unwrap(); set_passphrase("wrong-pass"); let err = get_secret(&conn, "s").unwrap_err(); @@ -148,8 +148,7 @@ mod tests { fn list_and_remove() { let _guard = GLOBAL_STATE_LOCK.lock().unwrap_or_else(|e| e.into_inner()); set_passphrase("test-pass"); - let conn = Connection::open_in_memory().unwrap(); - conn.execute_batch("CREATE TABLE gateway_secrets (name TEXT PRIMARY KEY, ciphertext BLOB NOT NULL);").unwrap(); + let conn = mem_migrated(); set_secret(&conn, "a", "1").unwrap(); set_secret(&conn, "b", "2").unwrap(); assert_eq!(list_secrets(&conn).unwrap(), vec!["a".to_string(), "b".to_string()]); diff --git a/src/mcp_server.rs b/src/mcp_server.rs index 9a530964f..ef5d8b3e3 100644 --- a/src/mcp_server.rs +++ b/src/mcp_server.rs @@ -796,10 +796,6 @@ impl AgentflareMcp { dirs::data_local_dir().unwrap_or_else(std::env::temp_dir).join("agentflare").join("gateway.db") } - fn gateway_secrets_db_path() -> std::path::PathBuf { - crate::paths::home().join(".agentflare").join("gateway.db") - } - fn load_gateway_config() -> gateway_registry::GatewayConfig { let path = crate::paths::home().join(".agentflare").join("gateway.toml"); match std::fs::read_to_string(&path) { @@ -828,14 +824,10 @@ impl AgentflareMcp { } fn resolve_gateway_secrets() -> std::collections::HashMap { - let db_path = Self::gateway_secrets_db_path(); - let conn = match crate::gateway_secrets::open_db(&db_path) { + let conn = match crate::db::open() { Ok(conn) => conn, Err(e) => { - eprintln!( - "agentflare: failed to open gateway secrets db at {}: {e}", - db_path.display() - ); + eprintln!("agentflare: failed to open agentflare.db for gateway secrets: {e}"); return std::collections::HashMap::new(); } }; From e95e41cc9ba23dbbc1fdafcc15ccf846e428e053 Mon Sep 17 00:00:00 2001 From: shiva Date: Sat, 11 Jul 2026 17:43:23 +0530 Subject: [PATCH 2/2] fix(db): make legacy gateway.db migration one-shot and non-fatal - rename gateway.db -> gateway.db.migrated after a successful import so a deleted secret can't resurrect via INSERT OR IGNORE on the next open() - make legacy reads best-effort: an incompatible/malformed old schema is skipped instead of propagating and bricking every db::open() (which would also disable unrelated claims access) - regression tests: deleted-stays-deleted, incompatible-schema-non-fatal Addresses CodeRabbit review on #146. --- src/db.rs | 130 ++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 106 insertions(+), 24 deletions(-) diff --git a/src/db.rs b/src/db.rs index 9a9539de9..51f25455f 100644 --- a/src/db.rs +++ b/src/db.rs @@ -37,42 +37,49 @@ pub fn open() -> rusqlite::Result { Ok(conn) } -/// If `~/.agentflare/gateway.db` exists with a `gateway_secrets` table, -/// copy rows into `agentflare.db` that don't already exist (so re-running -/// the migration is idempotent). Leaves the old file in place. fn migrate_old_gateway_db(conn: &Connection) -> rusqlite::Result<()> { - let old_path = old_gateway_db_path(); + import_legacy_secrets(conn, &old_gateway_db_path()) +} + +/// Copy secrets from a legacy `gateway.db` (pre-#138 separate file) into the +/// shared db, then rename the legacy file so it's imported exactly once. +/// +/// Reads from the legacy db are best-effort: a missing file, an unopenable +/// db, or an incompatible/malformed `gateway_secrets` schema all skip the +/// import rather than failing `open()` — otherwise one bad legacy file would +/// brick unrelated claims access too. Only writes into our own (healthy) +/// shared db are fatal. +/// +/// Renaming to `gateway.db.migrated` on success is the migration-complete +/// marker: without it every `open()` re-imports via `INSERT OR IGNORE`, so a +/// secret the user deleted would resurrect on the next run. +fn import_legacy_secrets(conn: &Connection, old_path: &std::path::Path) -> rusqlite::Result<()> { if !old_path.exists() { return Ok(()); } - // Probe: does the old file have a gateway_secrets table with rows? - let old = match Connection::open(&old_path) { - Ok(c) => c, - Err(_) => return Ok(()), + let Ok(old) = Connection::open(old_path) else { + return Ok(()); }; - let count: i64 = match old.query_row( - "SELECT COUNT(*) FROM gateway_secrets", - [], - |r| r.get(0), - ) { - Ok(n) => n, - Err(_) => return Ok(()), + let Ok(mut stmt) = old.prepare("SELECT name, ciphertext FROM gateway_secrets") else { + return Ok(()); // incompatible legacy schema — skip, don't brick open() }; - if count == 0 { - return Ok(()); - } - // Copy each row that doesn't already exist in the new db. - let mut stmt = old.prepare("SELECT name, ciphertext FROM gateway_secrets")?; - let rows = stmt.query_map([], |r| { + let Ok(rows) = stmt.query_map([], |r| { Ok((r.get::<_, String>(0)?, r.get::<_, Vec>(1)?)) - })?; - for row in rows { - let (name, ciphertext) = row?; + }) else { + return Ok(()); + }; + // `flatten` drops rows that fail to decode (e.g. malformed ciphertext) + // rather than propagating — one bad row must not abort the migration. + for (name, ciphertext) in rows.flatten() { conn.execute( "INSERT OR IGNORE INTO gateway_secrets (name, ciphertext) VALUES (?1, ?2)", params![name, ciphertext], )?; } + // Best-effort: if the rename fails we just re-import idempotently next time. + drop(stmt); + drop(old); + let _ = std::fs::rename(old_path, old_path.with_extension("db.migrated")); Ok(()) } @@ -99,3 +106,78 @@ fn tune(conn: &Connection) -> rusqlite::Result<()> { let _: String = conn.query_row("PRAGMA journal_mode=WAL", [], |r| r.get(0))?; Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + fn new_db() -> Connection { + let conn = Connection::open_in_memory().unwrap(); + crate::gateway_secrets::migrate(&conn).unwrap(); + conn + } + + fn write_legacy(path: &std::path::Path, ddl: &str, rows: &[(&str, &[u8])]) { + let old = Connection::open(path).unwrap(); + old.execute_batch(ddl).unwrap(); + for (name, ct) in rows { + old.execute( + "INSERT INTO gateway_secrets (name, ciphertext) VALUES (?1, ?2)", + params![name, ct], + ) + .unwrap(); + } + } + + const GOOD_DDL: &str = + "CREATE TABLE gateway_secrets (name TEXT PRIMARY KEY, ciphertext BLOB NOT NULL);"; + + #[test] + fn deleted_secret_stays_deleted_after_reopen() { + let dir = tempfile::tempdir().unwrap(); + let old_path = dir.path().join("gateway.db"); + write_legacy(&old_path, GOOD_DDL, &[("github_pat", b"cipher")]); + + let conn = new_db(); + import_legacy_secrets(&conn, &old_path).unwrap(); + let present: i64 = conn + .query_row("SELECT COUNT(*) FROM gateway_secrets WHERE name='github_pat'", [], |r| r.get(0)) + .unwrap(); + assert_eq!(present, 1, "secret should import on first open"); + + // Legacy file is renamed once migrated, so a re-import can't resurrect. + assert!(!old_path.exists(), "legacy file should be renamed after migration"); + conn.execute("DELETE FROM gateway_secrets WHERE name='github_pat'", []).unwrap(); + import_legacy_secrets(&conn, &old_path).unwrap(); + let after: i64 = conn + .query_row("SELECT COUNT(*) FROM gateway_secrets", [], |r| r.get(0)) + .unwrap(); + assert_eq!(after, 0, "deleted secret must not resurrect on reopen"); + } + + #[test] + fn incompatible_legacy_schema_does_not_brick_open() { + let dir = tempfile::tempdir().unwrap(); + let old_path = dir.path().join("gateway.db"); + // gateway_secrets exists but lacks the `ciphertext` column. + let old = Connection::open(&old_path).unwrap(); + old.execute_batch("CREATE TABLE gateway_secrets (name TEXT, blob BLOB);").unwrap(); + old.execute("INSERT INTO gateway_secrets (name, blob) VALUES ('x', X'00')", []).unwrap(); + drop(old); + + let conn = new_db(); + // Must be Ok — a malformed legacy schema is skipped, not propagated. + import_legacy_secrets(&conn, &old_path).unwrap(); + let count: i64 = conn + .query_row("SELECT COUNT(*) FROM gateway_secrets", [], |r| r.get(0)) + .unwrap(); + assert_eq!(count, 0); + } + + #[test] + fn missing_legacy_file_is_noop() { + let dir = tempfile::tempdir().unwrap(); + let conn = new_db(); + import_legacy_secrets(&conn, &dir.path().join("nope.db")).unwrap(); + } +}