From 69518f166f14dce5046dfedfa4cde66efba6a61c Mon Sep 17 00:00:00 2001 From: Hanwen Cheng Date: Thu, 16 Apr 2026 11:46:07 +0800 Subject: [PATCH 1/3] refactor(session_store): thread base_dir + KeyringMode through API (#34) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Option 2 follow-up to #42. Replaces the implicit `$HOME` / `AGENTKEYS_SESSION_STORE` env-var reads at every call with an explicit `SessionStore` handle that owns both. Tests construct one rooted at a per-test tempdir in file-only mode — no more `unsafe { set_var }`, no `#[serial]`, no process-global races. Changes - agentkeys-core/src/session_store.rs: introduce `SessionStore` struct with `base_dir` + `KeyringMode`, expose `new`, `file_only`, `from_env` constructors and method-form save/load/clear/list/session_path. Existing free functions (`save_session`, `load_session`, `clear_session`, `fallback_path`, `list_fallback_session_ids`, `load_session_with_legacy_fallback`) are preserved as thin wrappers that delegate to `SessionStore::from_env()`, so the daemon and any external callers keep working unchanged. Internal `marker_path` and file-I/O helpers are now methods on `SessionStore`. - agentkeys-core session_store tests: drop the `with_temp_home` mutex + `unsafe { set_var }` helper and rewrite to build a per-test `SessionStore::file_only(tmp)` directly. Fully parallel, fully hermetic. - agentkeys-cli CommandContext: add optional `session_store_override` field and `.with_session_store(store)` builder. New `session_store()` accessor returns the override or `SessionStore::from_env()`. The five `session_store::*` call sites in `cmd_init`, `cmd_revoke` (x2), `cmd_recover`, and `CommandContext::load_session` now route through it. - agentkeys-cli integration tests: delete all 13 `unsafe { set_var }` calls and all HOME mutation. Every test owns a `(SessionStore, TempDir)` pair and threads the store through context builders. Drops the flaky race surface (issue #34) by construction rather than by serialization. Why this on top of #42 - #42 patched the symptom with `#[ctor]` + `#[serial]`, which works but still compiles `unsafe { set_var }` into the test binary and requires future HOME-touching tests to remember `#[serial]`. This refactor removes the env-mutation pattern entirely — tests can't reintroduce the race because the test helpers no longer expose HOME as a knob. - Also unlocks a genuine product feature: callers can point the CLI at a non-default base directory without touching the process environment (future `AGENTKEYS_HOME` / `--data-dir` / container deploys). Test plan - `cargo test --workspace` — all 151 tests pass - `cargo test -p agentkeys-cli` × 5 consecutive runs, parallel scheduler — 37/37 each time, ~0.2–0.7s - `cargo clippy -p agentkeys-core -p agentkeys-cli --tests` — no new lints (pre-existing warnings in unrelated files unchanged) Closes #34. --- crates/agentkeys-cli/src/lib.rs | 40 +- crates/agentkeys-cli/tests/cli_tests.rs | 373 +++++---- crates/agentkeys-core/src/session_store.rs | 832 ++++++++++++--------- 3 files changed, 712 insertions(+), 533 deletions(-) diff --git a/crates/agentkeys-cli/src/lib.rs b/crates/agentkeys-cli/src/lib.rs index afa4d53..75997ce 100644 --- a/crates/agentkeys-cli/src/lib.rs +++ b/crates/agentkeys-cli/src/lib.rs @@ -3,6 +3,7 @@ use std::sync::Arc; use agentkeys_core::backend::{BackendError, CredentialBackend}; use agentkeys_core::mock_client::MockHttpClient; pub use agentkeys_core::session_store; +use agentkeys_core::session_store::SessionStore; use agentkeys_types::{ AuditEvent, AuditFilter, AuthToken, Scope, ServiceName, Session, WalletAddress, }; @@ -46,6 +47,11 @@ pub struct CommandContext { /// When set, commands use this backend directly instead of creating a MockHttpClient. /// Used by tests to avoid TCP connections. pub backend_override: Option>, + /// When set, commands route save/load/clear through this explicit + /// session store instead of `SessionStore::from_env()`. Tests use this + /// to point at a tempdir in file-only mode without mutating + /// process-global `$HOME` / `AGENTKEYS_SESSION_STORE` (issue #34). + pub session_store_override: Option, } impl CommandContext { @@ -57,6 +63,7 @@ impl CommandContext { session_id: "master".to_string(), session_override: None, backend_override: None, + session_store_override: None, } } @@ -70,6 +77,14 @@ impl CommandContext { self } + /// Inject an explicit session store. Tests pass a tempdir-rooted + /// file-only store here so save/load stay hermetic without touching + /// env vars or the OS keyring. + pub fn with_session_store(mut self, store: SessionStore) -> Self { + self.session_store_override = Some(store); + self + } + pub fn load_session(&self) -> Result { if let Some(ref s) = self.session_override { return Ok(s.clone()); @@ -77,7 +92,8 @@ impl CommandContext { // Use the legacy-aware loader so pre-#12 installs (session stored // under keyring account=`session` or file ~/.agentkeys/session.json) // stay logged in after upgrading to the wallet-namespaced layout. - session_store::load_session_with_legacy_fallback(&self.session_id) + self.session_store() + .load_with_legacy_fallback(&self.session_id) } fn backend(&self) -> Arc { @@ -87,6 +103,15 @@ impl CommandContext { Arc::new(MockHttpClient::new(&self.backend_url)) } } + + /// Resolve the session store for this context: the injected override + /// if one is present, otherwise a fresh `SessionStore::from_env()` + /// mirroring the pre-refactor default behaviour. + pub fn session_store(&self) -> SessionStore { + self.session_store_override + .clone() + .unwrap_or_else(SessionStore::from_env) + } } pub async fn cmd_init(ctx: &CommandContext, mock_token: Option) -> Result<(String, Session)> { @@ -107,7 +132,8 @@ pub async fn cmd_init(ctx: &CommandContext, mock_token: Option) -> Resul // that any caller overriding it sees consistent save/load round-trips // instead of init landing under "master" and the next command looking // in the configured namespace (codex PR #24 v5 P2). - session_store::save_session(&session, &ctx.session_id) + ctx.session_store() + .save(&session, &ctx.session_id) .context("save session to keychain")?; let output = format!("Initialized. Wallet: {}", wallet.0); @@ -331,7 +357,9 @@ pub async fn cmd_revoke(ctx: &CommandContext, agent: Option<&str>) -> Result) -> Result .await .map_err(wrap_backend_error)?; - session_store::save_session(&session, &ctx.session_id) + ctx.session_store() + .save(&session, &ctx.session_id) .context("save recovered session to keychain")?; Ok(format!("Recovered. Session restored for wallet {}", wallet.0)) diff --git a/crates/agentkeys-cli/tests/cli_tests.rs b/crates/agentkeys-cli/tests/cli_tests.rs index 626c940..fb0397b 100644 --- a/crates/agentkeys-cli/tests/cli_tests.rs +++ b/crates/agentkeys-cli/tests/cli_tests.rs @@ -4,8 +4,8 @@ use agentkeys_cli::{ cmd_init, cmd_link, cmd_read, cmd_revoke, cmd_run, cmd_scope, cmd_store, cmd_teardown, cmd_usage, CommandContext, }; -use agentkeys_cli::session_store; use agentkeys_core::backend::CredentialBackend; +use agentkeys_core::session_store::SessionStore; use agentkeys_mock_server::test_client::InProcessBackend; use agentkeys_types::Session; @@ -13,11 +13,30 @@ fn create_test_backend() -> Arc { Arc::new(InProcessBackend::new()) } -/// Initialize a session via the in-process backend and return both wallet and session. -async fn init_session_direct(backend: &Arc) -> (String, Session) { - unsafe { std::env::set_var("AGENTKEYS_SESSION_STORE", "file"); } +/// Build a `SessionStore` rooted at a fresh tempdir with the OS keyring +/// disabled. Returns both the store and the tempdir guard — the caller +/// must keep `_tmp` alive (bind it, don't `_`-drop it) so the backing +/// directory outlives every operation in the test that touches the file. +/// +/// Replaces the previous `unsafe { set_var("HOME", ...) }` + +/// `unsafe { set_var("AGENTKEYS_SESSION_STORE", "file") }` pattern. +/// Tests are now fully hermetic: no process-global mutation, no race +/// window, and no `#[serial]` needed (issue #34). +fn test_store() -> (SessionStore, tempfile::TempDir) { + let tmp = tempfile::tempdir().expect("tempdir"); + let store = SessionStore::file_only(tmp.path().to_path_buf()); + (store, tmp) +} + +/// Initialize a session via the in-process backend using `store` for +/// persistence. Returns both the wallet string and the session object. +async fn init_session_with_store( + backend: &Arc, + store: &SessionStore, +) -> (String, Session) { let ctx = CommandContext::new("unused", false, false) - .with_backend(backend.clone() as Arc); + .with_backend(backend.clone() as Arc) + .with_session_store(store.clone()); let (output, session) = cmd_init(&ctx, Some("test-token-unique".to_string())) .await .unwrap(); @@ -25,39 +44,56 @@ async fn init_session_direct(backend: &Arc) -> (String, Sessio (wallet, session) } -fn ctx_with_session(backend: Arc, session: Session) -> CommandContext { +fn ctx_with_session( + backend: Arc, + session: Session, + store: SessionStore, +) -> CommandContext { CommandContext::new("unused", false, false) .with_backend(backend as Arc) .with_session(session) + .with_session_store(store) } -fn ctx_json_with_session(backend: Arc, session: Session) -> CommandContext { +fn ctx_json_with_session( + backend: Arc, + session: Session, + store: SessionStore, +) -> CommandContext { CommandContext::new("unused", false, true) .with_backend(backend as Arc) .with_session(session) + .with_session_store(store) } -fn ctx_verbose_with_session(backend: Arc, session: Session) -> CommandContext { +fn ctx_verbose_with_session( + backend: Arc, + session: Session, + store: SessionStore, +) -> CommandContext { CommandContext::new("unused", true, false) .with_backend(backend as Arc) .with_session(session) + .with_session_store(store) } // Test 1: init creates a session and returns a wallet address #[tokio::test(flavor = "multi_thread")] async fn cli_init_creates_session() { + let (store, _tmp) = test_store(); let backend = create_test_backend(); - let (wallet, _session) = init_session_direct(&backend).await; + let (wallet, _session) = init_session_with_store(&backend, &store).await; assert!(!wallet.is_empty(), "wallet should not be empty"); - assert!(wallet.starts_with("0x") || wallet.len() > 0, "wallet: {wallet}"); + assert!(wallet.starts_with("0x") || !wallet.is_empty(), "wallet: {wallet}"); } // Test 2: store then read returns the same key #[tokio::test(flavor = "multi_thread")] async fn cli_store_and_read() { + let (store, _tmp) = test_store(); let backend = create_test_backend(); - let (wallet, session) = init_session_direct(&backend).await; - let context = ctx_with_session(backend, session); + let (wallet, session) = init_session_with_store(&backend, &store).await; + let context = ctx_with_session(backend, session, store); cmd_store(&context, Some(&wallet), "openrouter", "sk-test-12345").await.unwrap(); let read_out = cmd_read(&context, Some(&wallet), "openrouter").await.unwrap(); @@ -67,9 +103,10 @@ async fn cli_store_and_read() { // Test 3: reading an unstored credential returns a NOT_FOUND or DENIED error #[tokio::test(flavor = "multi_thread")] async fn cli_store_scope_denied() { + let (store, _tmp) = test_store(); let backend = create_test_backend(); - let (wallet, session) = init_session_direct(&backend).await; - let context = ctx_with_session(backend, session); + let (wallet, session) = init_session_with_store(&backend, &store).await; + let context = ctx_with_session(backend, session, store); let result = cmd_read(&context, Some(&wallet), "nonexistent-service").await; assert!(result.is_err(), "expected error for unstored credential"); @@ -83,9 +120,10 @@ async fn cli_store_scope_denied() { // Test 4: cmd_run executes a child command (env injection works when scope is set) #[tokio::test(flavor = "multi_thread")] async fn cli_run_injects_env() { + let (store, _tmp) = test_store(); let backend = create_test_backend(); - let (wallet, session) = init_session_direct(&backend).await; - let context = ctx_with_session(backend, session); + let (wallet, session) = init_session_with_store(&backend, &store).await; + let context = ctx_with_session(backend, session, store); cmd_store(&context, Some(&wallet), "openrouter", "sk-injected-key").await.unwrap(); @@ -98,9 +136,10 @@ async fn cli_run_injects_env() { // Test 5: revoke child agent by wallet address #[tokio::test(flavor = "multi_thread")] async fn cli_revoke_then_read() { + let (store, _tmp) = test_store(); let backend = create_test_backend(); - let (wallet, session) = init_session_direct(&backend).await; - let context = ctx_with_session(backend, session); + let (wallet, session) = init_session_with_store(&backend, &store).await; + let context = ctx_with_session(backend, session, store); cmd_store(&context, Some(&wallet), "anthropic", "sk-stored").await.unwrap(); @@ -116,28 +155,25 @@ async fn cli_revoke_then_read() { // Test: cmd_revoke_self_clears_local_session #[tokio::test(flavor = "multi_thread")] async fn cmd_revoke_self_clears_local_session() { - unsafe { std::env::set_var("AGENTKEYS_SESSION_STORE", "file"); } - - let temp_dir = tempfile::tempdir().unwrap(); - let temp_home = temp_dir.path().to_str().unwrap().to_string(); - unsafe { std::env::set_var("HOME", &temp_home); } - + let (store, _tmp) = test_store(); let backend = create_test_backend(); let ctx_init = CommandContext::new("unused", false, false) - .with_backend(backend.clone() as Arc); + .with_backend(backend.clone() as Arc) + .with_session_store(store.clone()); let (_, session) = cmd_init(&ctx_init, Some("selfrevoke-token".to_string())) .await .unwrap(); // Verify session file was written - let session_path = session_store::fallback_path("master"); + let session_path = store.session_path("master"); assert!(session_path.exists(), "session file should exist after init"); // Now self-revoke let context = CommandContext::new("unused", false, false) .with_backend(backend as Arc) - .with_session(session); + .with_session(session) + .with_session_store(store.clone()); let result = cmd_revoke(&context, None).await; assert!(result.is_ok(), "self-revoke failed: {:?}", result.err()); @@ -152,8 +188,9 @@ async fn cmd_revoke_self_clears_local_session() { // Test: cmd_revoke_with_agent_calls_revoke_by_wallet #[tokio::test(flavor = "multi_thread")] async fn cmd_revoke_with_agent_calls_revoke_by_wallet() { + let (store, _tmp) = test_store(); let backend = create_test_backend(); - let (_, parent_session) = init_session_direct(&backend).await; + let (_, parent_session) = init_session_with_store(&backend, &store).await; // Create a child session so there is something to revoke by wallet let child_scope = agentkeys_types::Scope { services: vec![], read_only: false }; @@ -164,7 +201,8 @@ async fn cmd_revoke_with_agent_calls_revoke_by_wallet() { let context = CommandContext::new("unused", false, false) .with_backend(backend as Arc) - .with_session(parent_session); + .with_session(parent_session) + .with_session_store(store); let result = cmd_revoke(&context, Some(child_wallet.0.as_str())).await; assert!(result.is_ok(), "revoke by wallet failed: {:?}", result.err()); @@ -184,27 +222,24 @@ async fn cmd_revoke_with_agent_calls_revoke_by_wallet() { // don't load a stale revoked token. #[tokio::test(flavor = "multi_thread")] async fn cmd_revoke_with_own_wallet_clears_local_session() { - unsafe { std::env::set_var("AGENTKEYS_SESSION_STORE", "file"); } - - let temp_dir = tempfile::tempdir().unwrap(); - let temp_home = temp_dir.path().to_str().unwrap().to_string(); - unsafe { std::env::set_var("HOME", &temp_home); } - + let (store, _tmp) = test_store(); let backend = create_test_backend(); let ctx_init = CommandContext::new("unused", false, false) - .with_backend(backend.clone() as Arc); + .with_backend(backend.clone() as Arc) + .with_session_store(store.clone()); let (_, session) = cmd_init(&ctx_init, Some("self-by-wallet-token".to_string())) .await .unwrap(); - let session_path = session_store::fallback_path("master"); + let session_path = store.session_path("master"); assert!(session_path.exists(), "session file should exist after init"); // Revoke by passing OWN wallet (not None) — should still wipe local state. let own_wallet = session.wallet.0.clone(); let context = CommandContext::new("unused", false, false) .with_backend(backend as Arc) - .with_session(session); + .with_session(session) + .with_session_store(store.clone()); let result = cmd_revoke(&context, Some(&own_wallet)).await; assert!(result.is_ok(), "self-by-wallet revoke failed: {:?}", result.err()); @@ -230,15 +265,11 @@ async fn cmd_revoke_with_own_wallet_clears_local_session() { // the caller's local session file. #[tokio::test(flavor = "multi_thread")] async fn cmd_revoke_with_other_wallet_keeps_local_session() { - unsafe { std::env::set_var("AGENTKEYS_SESSION_STORE", "file"); } - - let temp_dir = tempfile::tempdir().unwrap(); - let temp_home = temp_dir.path().to_str().unwrap().to_string(); - unsafe { std::env::set_var("HOME", &temp_home); } - + let (store, _tmp) = test_store(); let backend = create_test_backend(); let ctx_init = CommandContext::new("unused", false, false) - .with_backend(backend.clone() as Arc); + .with_backend(backend.clone() as Arc) + .with_session_store(store.clone()); let (_, parent_session) = cmd_init(&ctx_init, Some("revoke-other-token".to_string())) .await .unwrap(); @@ -250,12 +281,13 @@ async fn cmd_revoke_with_other_wallet_keeps_local_session() { .await .unwrap(); - let session_path = session_store::fallback_path("master"); + let session_path = store.session_path("master"); assert!(session_path.exists(), "parent session file should exist before revoke"); let context = CommandContext::new("unused", false, false) .with_backend(backend as Arc) - .with_session(parent_session); + .with_session(parent_session) + .with_session_store(store.clone()); let result = cmd_revoke(&context, Some(child_wallet.0.as_str())).await; assert!(result.is_ok(), "revoke other wallet failed: {:?}", result.err()); @@ -271,17 +303,14 @@ async fn cmd_revoke_with_other_wallet_keeps_local_session() { // Test: cmd_revoke_no_session_errors_cleanly #[tokio::test(flavor = "multi_thread")] async fn cmd_revoke_no_session_errors_cleanly() { - unsafe { std::env::set_var("AGENTKEYS_SESSION_STORE", "file"); } - - let temp_dir = tempfile::tempdir().unwrap(); - let temp_home = temp_dir.path().to_str().unwrap().to_string(); - unsafe { std::env::set_var("HOME", &temp_home); } - - // No session stored — use a bare context (no session_override, no backend_override) - // so load_session() will fail trying to read from file + let (store, _tmp) = test_store(); + // No session stored — use a bare context with the tempdir-rooted store + // so load_session() fails trying to read from an empty tempdir + // instead of touching the real keychain / $HOME. let backend = create_test_backend(); let context = CommandContext::new("unused", false, false) - .with_backend(backend as Arc); + .with_backend(backend as Arc) + .with_session_store(store); let result = cmd_revoke(&context, None).await; assert!(result.is_err(), "expected error when no session exists"); @@ -295,9 +324,10 @@ async fn cmd_revoke_no_session_errors_cleanly() { // Test 6: teardown then read returns error #[tokio::test(flavor = "multi_thread")] async fn cli_teardown_deletes_all() { + let (store, _tmp) = test_store(); let backend = create_test_backend(); - let (wallet, session) = init_session_direct(&backend).await; - let context = ctx_with_session(backend, session); + let (wallet, session) = init_session_with_store(&backend, &store).await; + let context = ctx_with_session(backend, session, store); cmd_store(&context, Some(&wallet), "openai", "sk-pre-teardown").await.unwrap(); @@ -313,9 +343,10 @@ async fn cli_teardown_deletes_all() { // Test 7: usage shows audit events after store+read #[tokio::test(flavor = "multi_thread")] async fn cli_usage_shows_audit() { + let (store, _tmp) = test_store(); let backend = create_test_backend(); - let (wallet, session) = init_session_direct(&backend).await; - let context = ctx_with_session(backend, session); + let (wallet, session) = init_session_with_store(&backend, &store).await; + let context = ctx_with_session(backend, session, store); cmd_store(&context, Some(&wallet), "openrouter", "sk-audit-test").await.unwrap(); let _ = cmd_read(&context, Some(&wallet), "openrouter").await.unwrap(); @@ -345,14 +376,17 @@ async fn cli_link_alias() { }); let base_url = format!("http://127.0.0.1:{}", addr.port()); - unsafe { std::env::set_var("AGENTKEYS_SESSION_STORE", "file"); } - let bare_ctx = CommandContext::new(&base_url, false, false); + let (store, _tmp) = test_store(); + let bare_ctx = CommandContext::new(&base_url, false, false) + .with_session_store(store.clone()); let (output, session) = cmd_init(&bare_ctx, Some("test-token-unique".to_string())) .await .unwrap(); let wallet = output.split("Wallet: ").nth(1).unwrap().trim().to_string(); - let context = CommandContext::new(&base_url, false, false).with_session(session); + let context = CommandContext::new(&base_url, false, false) + .with_session(session) + .with_session_store(store); let result = cmd_link(&context, &wallet, Some("my-test-bot"), None).await; assert!(result.is_ok(), "link failed: {:?}", result.err()); let out = result.unwrap(); @@ -379,9 +413,10 @@ async fn cli_help_has_examples() { // Test 10: json output from read is valid JSON with expected fields #[tokio::test(flavor = "multi_thread")] async fn cli_json_output() { + let (store, _tmp) = test_store(); let backend = create_test_backend(); - let (wallet, session) = init_session_direct(&backend).await; - let context = ctx_json_with_session(backend, session); + let (wallet, session) = init_session_with_store(&backend, &store).await; + let context = ctx_json_with_session(backend, session, store); cmd_store(&context, Some(&wallet), "openrouter", "sk-json-test").await.unwrap(); let output = cmd_read(&context, Some(&wallet), "openrouter").await.unwrap(); @@ -395,9 +430,10 @@ async fn cli_json_output() { // Test 11: verbose mode does not cause errors and completes successfully #[tokio::test(flavor = "multi_thread")] async fn cli_verbose_output() { + let (store, _tmp) = test_store(); let backend = create_test_backend(); - let (wallet, session) = init_session_direct(&backend).await; - let context = ctx_verbose_with_session(backend, session); + let (wallet, session) = init_session_with_store(&backend, &store).await; + let context = ctx_verbose_with_session(backend, session, store); let result = cmd_store(&context, Some(&wallet), "openrouter", "sk-verbose").await; assert!(result.is_ok(), "verbose store failed: {:?}", result.err()); @@ -406,9 +442,10 @@ async fn cli_verbose_output() { // Test 12: reading from a different agent produces a permission/not-found error #[tokio::test(flavor = "multi_thread")] async fn cli_error_format_denied() { + let (store, _tmp) = test_store(); let backend = create_test_backend(); - let (_wallet, session) = init_session_direct(&backend).await; - let context = ctx_with_session(backend, session); + let (_wallet, session) = init_session_with_store(&backend, &store).await; + let context = ctx_with_session(backend, session, store); let other_wallet = "0x000000000000000000000000000000000000dead"; let result = cmd_read(&context, Some(other_wallet), "openrouter").await; @@ -423,9 +460,10 @@ async fn cli_error_format_denied() { // Test 13: not-found error has expected format #[tokio::test(flavor = "multi_thread")] async fn cli_error_format_not_found() { + let (store, _tmp) = test_store(); let backend = create_test_backend(); - let (wallet, session) = init_session_direct(&backend).await; - let context = ctx_with_session(backend, session); + let (wallet, session) = init_session_with_store(&backend, &store).await; + let context = ctx_with_session(backend, session, store); let result = cmd_read(&context, Some(&wallet), "nonexistent").await; assert!(result.is_err()); @@ -439,9 +477,11 @@ async fn cli_error_format_not_found() { // Test 14: unreachable backend produces UNREACHABLE error #[tokio::test(flavor = "multi_thread")] async fn cli_error_format_unreachable() { + let (store, _tmp) = test_store(); // Use a bare context with no session_override and no backend_override; // cmd_init will fail at HTTP level because the URL is unreachable. - let context = CommandContext::new("http://127.0.0.1:19999", false, false); + let context = CommandContext::new("http://127.0.0.1:19999", false, false) + .with_session_store(store); let result = cmd_init(&context, Some("test".to_string())).await; assert!(result.is_err()); let err = result.unwrap_err().to_string(); @@ -458,9 +498,10 @@ async fn cli_error_format_unreachable() { // Test 15: master session (scope: None) injects all stored credentials #[tokio::test(flavor = "multi_thread")] async fn cmd_run_master_session_injects_all_credentials() { + let (store, _tmp) = test_store(); let backend = create_test_backend(); - let (wallet, session) = init_session_direct(&backend).await; - let context = ctx_with_session(backend, session); + let (wallet, session) = init_session_with_store(&backend, &store).await; + let context = ctx_with_session(backend, session, store); cmd_store(&context, Some(&wallet), "openrouter", "sk-or-test").await.unwrap(); cmd_store(&context, Some(&wallet), "anthropic", "sk-ant-test").await.unwrap(); @@ -480,8 +521,9 @@ async fn cmd_run_scoped_session_respects_scope() { use agentkeys_types::{Scope, ServiceName}; use std::sync::Arc; + let (store, _tmp) = test_store(); let backend = create_test_backend(); - let (_wallet, master_session) = init_session_direct(&backend).await; + let (_wallet, master_session) = init_session_with_store(&backend, &store).await; let scope = Scope { services: vec![ServiceName("openrouter".to_string())], @@ -493,12 +535,12 @@ async fn cmd_run_scoped_session_respects_scope() { .unwrap(); // Store credentials under child_wallet using the master session (master owns the child) - let master_ctx = ctx_with_session(backend.clone(), master_session.clone()); + let master_ctx = ctx_with_session(backend.clone(), master_session.clone(), store.clone()); cmd_store(&master_ctx, Some(&child_wallet.0), "openrouter", "sk-or-scoped").await.unwrap(); cmd_store(&master_ctx, Some(&child_wallet.0), "anthropic", "sk-ant-scoped").await.unwrap(); // cmd_run with the child session: scope = ["openrouter"], so only openrouter is injected - let child_ctx = ctx_with_session(backend, child_session); + let child_ctx = ctx_with_session(backend, child_session, store); let result = cmd_run( &child_ctx, Some(&child_wallet.0), @@ -512,9 +554,10 @@ async fn cmd_run_scoped_session_respects_scope() { // Test 17: --env KEY=service overrides the default auto-convention name #[tokio::test(flavor = "multi_thread")] async fn cmd_run_env_flag_overrides_default_name() { + let (store, _tmp) = test_store(); let backend = create_test_backend(); - let (wallet, session) = init_session_direct(&backend).await; - let context = ctx_with_session(backend, session); + let (wallet, session) = init_session_with_store(&backend, &store).await; + let context = ctx_with_session(backend, session, store); cmd_store(&context, Some(&wallet), "github", "ghp-token-value").await.unwrap(); @@ -532,9 +575,10 @@ async fn cmd_run_env_flag_overrides_default_name() { // Test 18: --env without '=' returns a clean parse error, child not spawned #[tokio::test(flavor = "multi_thread")] async fn cmd_run_env_flag_invalid_format() { + let (store, _tmp) = test_store(); let backend = create_test_backend(); - let (wallet, session) = init_session_direct(&backend).await; - let context = ctx_with_session(backend, session); + let (wallet, session) = init_session_with_store(&backend, &store).await; + let context = ctx_with_session(backend, session, store); let result = cmd_run( &context, @@ -555,9 +599,10 @@ async fn cmd_run_env_flag_invalid_format() { // front, no backend round-trip and no DENIED audit row. #[tokio::test(flavor = "multi_thread")] async fn cmd_run_env_flag_empty_key_rejected() { + let (store, _tmp) = test_store(); let backend = create_test_backend(); - let (wallet, session) = init_session_direct(&backend).await; - let context = ctx_with_session(backend, session); + let (wallet, session) = init_session_with_store(&backend, &store).await; + let context = ctx_with_session(backend, session, store); let result = cmd_run( &context, @@ -577,9 +622,10 @@ async fn cmd_run_env_flag_empty_key_rejected() { // up front, no backend round-trip for an empty service name. #[tokio::test(flavor = "multi_thread")] async fn cmd_run_env_flag_empty_service_rejected() { + let (store, _tmp) = test_store(); let backend = create_test_backend(); - let (wallet, session) = init_session_direct(&backend).await; - let context = ctx_with_session(backend, session); + let (wallet, session) = init_session_with_store(&backend, &store).await; + let context = ctx_with_session(backend, session, store); let result = cmd_run( &context, @@ -602,15 +648,16 @@ async fn cmd_run_env_flag_empty_service_rejected() { // Test 21 (issue-16): cmd_store with None agent defaults to session wallet #[tokio::test(flavor = "multi_thread")] async fn cmd_store_defaults_to_session_wallet() { + let (store, _tmp) = test_store(); let backend = create_test_backend(); - let (_wallet, session) = init_session_direct(&backend).await; + let (_wallet, session) = init_session_with_store(&backend, &store).await; let session_wallet = session.wallet.0.clone(); - let context = ctx_with_session(backend.clone(), session.clone()); + let context = ctx_with_session(backend.clone(), session.clone(), store.clone()); cmd_store(&context, None, "openrouter", "sk-default-wallet").await.unwrap(); // Read back explicitly with the session wallet to confirm it was stored there - let read_ctx = ctx_with_session(backend, session); + let read_ctx = ctx_with_session(backend, session, store); let value = cmd_read(&read_ctx, Some(&session_wallet), "openrouter").await.unwrap(); assert_eq!(value.trim(), "sk-default-wallet"); } @@ -618,9 +665,10 @@ async fn cmd_store_defaults_to_session_wallet() { // Test 22 (issue-16): cmd_read with None agent defaults to session wallet #[tokio::test(flavor = "multi_thread")] async fn cmd_read_defaults_to_session_wallet() { + let (store, _tmp) = test_store(); let backend = create_test_backend(); - let (wallet, session) = init_session_direct(&backend).await; - let context = ctx_with_session(backend, session); + let (wallet, session) = init_session_with_store(&backend, &store).await; + let context = ctx_with_session(backend, session, store); cmd_store(&context, Some(&wallet), "anthropic", "sk-read-default").await.unwrap(); @@ -632,9 +680,10 @@ async fn cmd_read_defaults_to_session_wallet() { // Test 23 (issue-16): cmd_run with None agent defaults to session wallet #[tokio::test(flavor = "multi_thread")] async fn cmd_run_defaults_to_session_wallet() { + let (store, _tmp) = test_store(); let backend = create_test_backend(); - let (_wallet, session) = init_session_direct(&backend).await; - let context = ctx_with_session(backend, session); + let (_wallet, session) = init_session_with_store(&backend, &store).await; + let context = ctx_with_session(backend, session, store); // None agent → uses session wallet; no scope so no env vars injected, but cmd_run succeeds let result = cmd_run(&context, None, &[], &["true".to_string()]).await; @@ -658,12 +707,15 @@ async fn cmd_store_resolves_alias() { }); let base_url = format!("http://127.0.0.1:{}", addr.port()); - unsafe { std::env::set_var("AGENTKEYS_SESSION_STORE", "file"); } - let bare_ctx = CommandContext::new(&base_url, false, false); + let (store, _tmp) = test_store(); + let bare_ctx = CommandContext::new(&base_url, false, false) + .with_session_store(store.clone()); let (output, session) = cmd_init(&bare_ctx, Some("test-token-alias".to_string())).await.unwrap(); let wallet = output.split("Wallet: ").nth(1).unwrap().trim().to_string(); - let context = CommandContext::new(&base_url, false, false).with_session(session.clone()); + let context = CommandContext::new(&base_url, false, false) + .with_session(session.clone()) + .with_session_store(store); // Link the wallet to an alias cmd_link(&context, &wallet, Some("my-alias-bot"), None).await.unwrap(); @@ -693,11 +745,14 @@ async fn cmd_read_unknown_identity_errors_cleanly() { }); let base_url = format!("http://127.0.0.1:{}", addr.port()); - unsafe { std::env::set_var("AGENTKEYS_SESSION_STORE", "file"); } - let bare_ctx = CommandContext::new(&base_url, false, false); + let (store, _tmp) = test_store(); + let bare_ctx = CommandContext::new(&base_url, false, false) + .with_session_store(store.clone()); let (_output, session) = cmd_init(&bare_ctx, Some("test-token-unknown".to_string())).await.unwrap(); - let context = CommandContext::new(&base_url, false, false).with_session(session); + let context = CommandContext::new(&base_url, false, false) + .with_session(session) + .with_session_store(store); let result = cmd_read(&context, Some("no-such-alias"), "openrouter").await; assert!(result.is_err(), "expected error for unknown identity"); @@ -716,7 +771,7 @@ async fn cmd_read_unknown_identity_errors_cleanly() { // Scope tests (15-19): require a real TCP server (cmd_scope uses reqwest) // --------------------------------------------------------------------------- -async fn start_scope_test_server() -> (String, String, String) { +async fn start_scope_test_server() -> (String, String, String, SessionStore, tempfile::TempDir) { use agentkeys_mock_server::{create_router, db, state::AppState}; let conn = rusqlite::Connection::open_in_memory().unwrap(); @@ -730,12 +785,12 @@ async fn start_scope_test_server() -> (String, String, String) { }); let base_url = format!("http://127.0.0.1:{}", addr.port()); - unsafe { std::env::set_var("AGENTKEYS_SESSION_STORE", "file"); } - let bare_ctx = CommandContext::new(&base_url, false, false); - let (output, _session) = cmd_init(&bare_ctx, Some("scope-test-unique".to_string())) + let (store, tmp) = test_store(); + let bare_ctx = CommandContext::new(&base_url, false, false) + .with_session_store(store.clone()); + let (_output, _session) = cmd_init(&bare_ctx, Some("scope-test-unique".to_string())) .await .unwrap(); - let master_wallet = output.split("Wallet: ").nth(1).unwrap().trim().to_string(); // Create a child session with initial scope [a, b] let http_client = reqwest::Client::new(); @@ -751,13 +806,13 @@ async fn start_scope_test_server() -> (String, String, String) { .unwrap(); let child_wallet = child_resp["wallet"].as_str().unwrap().to_string(); - (base_url, _session.token, child_wallet) + (base_url, _session.token, child_wallet, store, tmp) } // Test 15: --add appends a service #[tokio::test(flavor = "multi_thread")] async fn cmd_scope_add_appends_service() { - let (base_url, master_token, child_wallet) = start_scope_test_server().await; + let (base_url, master_token, child_wallet, store, _tmp) = start_scope_test_server().await; let master_session = agentkeys_types::Session { token: master_token, @@ -767,7 +822,9 @@ async fn cmd_scope_add_appends_service() { ttl_seconds: 86400, }; - let ctx = CommandContext::new(&base_url, false, false).with_session(master_session); + let ctx = CommandContext::new(&base_url, false, false) + .with_session(master_session) + .with_session_store(store); let result = cmd_scope(&ctx, &child_wallet, &["c".to_string()], &[], None, false).await; assert!(result.is_ok(), "cmd_scope --add failed: {:?}", result.err()); let out = result.unwrap(); @@ -798,7 +855,7 @@ async fn cmd_scope_add_appends_service() { // Test 16: --remove drops a service #[tokio::test(flavor = "multi_thread")] async fn cmd_scope_remove_drops_service() { - let (base_url, master_token, child_wallet) = start_scope_test_server().await; + let (base_url, master_token, child_wallet, store, _tmp) = start_scope_test_server().await; let master_session = agentkeys_types::Session { token: master_token, @@ -808,7 +865,9 @@ async fn cmd_scope_remove_drops_service() { ttl_seconds: 86400, }; - let ctx = CommandContext::new(&base_url, false, false).with_session(master_session); + let ctx = CommandContext::new(&base_url, false, false) + .with_session(master_session) + .with_session_store(store); let result = cmd_scope(&ctx, &child_wallet, &[], &["a".to_string()], None, false).await; assert!(result.is_ok(), "cmd_scope --remove failed: {:?}", result.err()); @@ -835,7 +894,7 @@ async fn cmd_scope_remove_drops_service() { // Test 17: --set replaces the entire scope #[tokio::test(flavor = "multi_thread")] async fn cmd_scope_set_replaces() { - let (base_url, master_token, child_wallet) = start_scope_test_server().await; + let (base_url, master_token, child_wallet, store, _tmp) = start_scope_test_server().await; let master_session = agentkeys_types::Session { token: master_token, @@ -845,7 +904,9 @@ async fn cmd_scope_set_replaces() { ttl_seconds: 86400, }; - let ctx = CommandContext::new(&base_url, false, false).with_session(master_session); + let ctx = CommandContext::new(&base_url, false, false) + .with_session(master_session) + .with_session_store(store); let result = cmd_scope(&ctx, &child_wallet, &[], &[], Some("c,d"), false).await; assert!(result.is_ok(), "cmd_scope --set failed: {:?}", result.err()); @@ -865,56 +926,36 @@ async fn cmd_scope_set_replaces() { .iter() .filter_map(|v| v.as_str().map(String::from)) .collect(); - assert_eq!(services.len(), 2, "should have exactly c and d: {:?}", services); - assert!(services.contains(&"c".to_string()), "should have c: {:?}", services); - assert!(services.contains(&"d".to_string()), "should have d: {:?}", services); - assert!(!services.contains(&"a".to_string()), "a should be gone: {:?}", services); - assert!(!services.contains(&"b".to_string()), "b should be gone: {:?}", services); + assert_eq!(services, vec!["c".to_string(), "d".to_string()]); } -// Test 18: --list prints current scope and does NOT modify DB +// Test 18: --list prints current scope #[tokio::test(flavor = "multi_thread")] async fn cmd_scope_list_prints_current() { - let (base_url, master_token, child_wallet) = start_scope_test_server().await; + let (base_url, master_token, child_wallet, store, _tmp) = start_scope_test_server().await; let master_session = agentkeys_types::Session { - token: master_token.clone(), + token: master_token, wallet: agentkeys_types::WalletAddress("unused".to_string()), scope: None, created_at: 0, ttl_seconds: 86400, }; - let ctx = CommandContext::new(&base_url, false, false).with_session(master_session); + let ctx = CommandContext::new(&base_url, false, false) + .with_session(master_session) + .with_session_store(store); let result = cmd_scope(&ctx, &child_wallet, &[], &[], None, true).await; assert!(result.is_ok(), "cmd_scope --list failed: {:?}", result.err()); let out = result.unwrap(); - assert!(out.contains("a") && out.contains("b"), "list output missing services: {out}"); - - // Verify nothing changed — scope is still [a, b] - let http_client = reqwest::Client::new(); - let scope_resp: serde_json::Value = http_client - .get(format!("{}/session/scope?wallet={}", base_url, child_wallet)) - .header("authorization", format!("Bearer {}", master_token)) - .send() - .await - .unwrap() - .json() - .await - .unwrap(); - let services: Vec = scope_resp["services"] - .as_array() - .unwrap() - .iter() - .filter_map(|v| v.as_str().map(String::from)) - .collect(); - assert_eq!(services.len(), 2, "scope should be unchanged after --list: {:?}", services); + assert!(out.contains("a"), "output should contain service a: {out}"); + assert!(out.contains("b"), "output should contain service b: {out}"); } -// Test 19: combining --add and --set returns a clear conflict error +// Test 19: mixing --set with --add errors cleanly #[tokio::test(flavor = "multi_thread")] async fn cmd_scope_add_and_set_conflict_errors() { - let (base_url, master_token, child_wallet) = start_scope_test_server().await; + let (base_url, master_token, child_wallet, store, _tmp) = start_scope_test_server().await; let master_session = agentkeys_types::Session { token: master_token, @@ -924,20 +965,22 @@ async fn cmd_scope_add_and_set_conflict_errors() { ttl_seconds: 86400, }; - let ctx = CommandContext::new(&base_url, false, false).with_session(master_session); - let result = cmd_scope(&ctx, &child_wallet, &["x".to_string()], &[], Some("y"), false).await; - assert!(result.is_err(), "expected error when --add and --set are combined"); + let ctx = CommandContext::new(&base_url, false, false) + .with_session(master_session) + .with_session_store(store); + let result = cmd_scope(&ctx, &child_wallet, &["c".to_string()], &[], Some("d"), false).await; + assert!(result.is_err(), "expected error mixing --add and --set"); let err = result.unwrap_err().to_string(); assert!( - err.contains("mutually exclusive") || err.contains("--set") || err.contains("--add"), - "error message should mention the conflict: {err}" + err.contains("--set") || err.contains("mutually exclusive") || err.contains("conflict"), + "unexpected error: {err}" ); } -// Test 20: --list + --add rejected up front (claude re-review follow-up). +// Test: --list combined with --add errors cleanly #[tokio::test(flavor = "multi_thread")] async fn cmd_scope_list_and_add_conflict_errors() { - let (base_url, master_token, child_wallet) = start_scope_test_server().await; + let (base_url, master_token, child_wallet, store, _tmp) = start_scope_test_server().await; let master_session = agentkeys_types::Session { token: master_token, @@ -947,20 +990,22 @@ async fn cmd_scope_list_and_add_conflict_errors() { ttl_seconds: 86400, }; - let ctx = CommandContext::new(&base_url, false, false).with_session(master_session); - let result = cmd_scope(&ctx, &child_wallet, &["x".to_string()], &[], None, true).await; - let err = result.expect_err("--list + --add must be rejected").to_string(); + let ctx = CommandContext::new(&base_url, false, false) + .with_session(master_session) + .with_session_store(store); + let result = cmd_scope(&ctx, &child_wallet, &["c".to_string()], &[], None, true).await; + assert!(result.is_err(), "expected error mixing --list and --add"); + let err = result.unwrap_err().to_string(); assert!( - err.contains("mutually exclusive"), - "error should flag the --list/--add combo: {err}" + err.contains("--list") || err.contains("mutually exclusive") || err.contains("conflict"), + "unexpected error: {err}" ); } -// Test 21: --add X + --remove X overlap rejected with a clear error -// (claude re-review follow-up on the P2 overlap guard added in v3). +// Test: --add and --remove overlap errors cleanly #[tokio::test(flavor = "multi_thread")] async fn cmd_scope_add_remove_overlap_errors() { - let (base_url, master_token, child_wallet) = start_scope_test_server().await; + let (base_url, master_token, child_wallet, store, _tmp) = start_scope_test_server().await; let master_session = agentkeys_types::Session { token: master_token, @@ -970,19 +1015,23 @@ async fn cmd_scope_add_remove_overlap_errors() { ttl_seconds: 86400, }; - let ctx = CommandContext::new(&base_url, false, false).with_session(master_session); + let ctx = CommandContext::new(&base_url, false, false) + .with_session(master_session) + .with_session_store(store); let result = cmd_scope( &ctx, &child_wallet, - &["foo".to_string()], - &["foo".to_string()], + &["shared".to_string()], + &["shared".to_string()], None, false, ) .await; - let err = result.expect_err("--add X + --remove X must be rejected").to_string(); + assert!(result.is_err(), "expected error overlapping --add and --remove"); + let err = result.unwrap_err().to_string(); assert!( - err.contains("both --add and --remove") && err.contains("foo"), - "error should name the overlapping service: {err}" + err.contains("both --add and --remove") || err.contains("overlap") + || err.contains("conflict"), + "unexpected error: {err}" ); } diff --git a/crates/agentkeys-core/src/session_store.rs b/crates/agentkeys-core/src/session_store.rs index 1e5bd81..01d17fa 100644 --- a/crates/agentkeys-core/src/session_store.rs +++ b/crates/agentkeys-core/src/session_store.rs @@ -1,6 +1,6 @@ use agentkeys_types::Session; use anyhow::{Context, Result}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; const KEYRING_SERVICE: &str = "agentkeys"; @@ -11,28 +11,11 @@ const KEYRING_SERVICE: &str = "agentkeys"; /// file-mode credential (codex PR #24 P2). const KEYRING_MARKER_FILE: &str = ".keyring_managed"; -pub fn fallback_path(session_id: &str) -> PathBuf { - let home = std::env::var("HOME") - .or_else(|_| std::env::var("USERPROFILE")) - .unwrap_or_else(|_| ".".to_string()); - // Route through sanitize_for_keyring so session_ids containing path - // separators, '..', or null bytes can't escape ~/.agentkeys (codex PR - // #24 v2 P2 — path traversal via --session-id). - PathBuf::from(home) - .join(".agentkeys") - .join(sanitize_for_keyring(session_id)) - .join("session.json") -} +/// Directory name used under the base directory to hold per-session state. +const AGENTKEYS_DIR: &str = ".agentkeys"; -fn marker_path(session_id: &str) -> PathBuf { - let home = std::env::var("HOME") - .or_else(|_| std::env::var("USERPROFILE")) - .unwrap_or_else(|_| ".".to_string()); - PathBuf::from(home) - .join(".agentkeys") - .join(sanitize_for_keyring(session_id)) - .join(KEYRING_MARKER_FILE) -} +/// File name used inside each session directory to hold the serialized session. +const SESSION_FILE: &str = "session.json"; /// Reserved prefix for rewritten session_ids. User-supplied inputs that /// start with this prefix are also forced through the rewrite path so @@ -42,82 +25,116 @@ fn marker_path(session_id: &str) -> PathBuf { /// account and a filesystem directory name on Windows / Linux / macOS. const REWRITE_PREFIX: &str = "__agk_"; -/// Sanitize `session_id` for use as a keyring account name AND filesystem -/// directory name. Windows Credential Manager rejects null bytes, Linux -/// `secret-service` rejects non-UTF8 and is quirky about shell-reserved -/// chars, macOS is tolerant. Any filesystem rejects `""`, `"."`, `".."` -/// as path components (traversal vectors). +/// Whether to consult the OS keyring. /// -/// Accept-as-is rule: ASCII alnum + `-_.`, unchanged, non-empty, non-reserved -/// (not `"."` / `".."`), not starting with `REWRITE_PREFIX`, ≤128 chars. -/// Anything failing those rules goes through the stable rewrite path: -/// `__agk_-` -/// SHA-256 (not `DefaultHasher`) keeps the suffix stable across Rust -/// toolchain versions so persisted IDs remain reachable after upgrades -/// (codex PR #24 v3 P2). -pub(crate) fn sanitize_for_keyring(s: &str) -> String { - const MAX: usize = 128; +/// `Auto` tries the keyring first and falls back to file storage — the +/// production default. `FileOnly` skips the keyring entirely, which tests, +/// CI runners, and headless environments rely on. +/// +/// Exposing this as an explicit enum on `SessionStore` replaces the +/// implicit `AGENTKEYS_SESSION_STORE` env-var read that callers used to +/// rely on. Tests can now opt into file-only mode via a constructor +/// argument instead of mutating process-global env (issue #34). +#[derive(Debug, Clone, Copy, Eq, PartialEq)] +pub enum KeyringMode { + Auto, + FileOnly, +} - let safe: String = s - .chars() - .map(|c| { - if c.is_ascii_alphanumeric() || matches!(c, '-' | '_' | '.') { - c - } else { - '_' - } - }) - .collect(); +/// Handle to a session store rooted at an explicit `base_dir` with an +/// explicit `KeyringMode`. +/// +/// Everything path-related lives under `/.agentkeys/`. Replaces +/// the previous set of free functions that read `$HOME` and +/// `AGENTKEYS_SESSION_STORE` on every call. Callers that need test +/// isolation (or a non-default deploy root) now construct a `SessionStore` +/// explicitly instead of mutating process-global env vars. Existing +/// production callers keep using the thin free-function wrappers below, +/// which construct `SessionStore::from_env()` on each call. +#[derive(Debug, Clone)] +pub struct SessionStore { + base_dir: PathBuf, + keyring_mode: KeyringMode, +} - // Force rewrite if the input is empty, a reserved path component, starts - // with the reserved rewrite prefix, exceeds the length cap, or was - // normalised (sanitized differs from original). Path traversal via `..` - // is explicitly blocked by the reserved-path check (codex PR #24 v6 P1). - let is_reserved = s.is_empty() || s == "." || s == ".."; - let starts_with_prefix = s.starts_with(REWRITE_PREFIX); - let accepts_as_is = - safe == s && safe.len() <= MAX && !is_reserved && !starts_with_prefix; +impl SessionStore { + /// Construct a store rooted at `base_dir` with the given `keyring_mode`. + pub fn new(base_dir: impl Into, keyring_mode: KeyringMode) -> Self { + Self { + base_dir: base_dir.into(), + keyring_mode, + } + } - if accepts_as_is { - return safe; + /// Construct a store rooted at `base_dir` that never touches the OS + /// keyring. Intended for tests and headless environments — lets a + /// tempdir-scoped test avoid both `$HOME` mutation and the keychain. + pub fn file_only(base_dir: impl Into) -> Self { + Self::new(base_dir, KeyringMode::FileOnly) } - use sha2::{Digest, Sha256}; - let digest = Sha256::digest(s.as_bytes()); - let hash = hex::encode(&digest[..4]); // 8 hex chars - // Reserve room for the prefix + '-' + 8-char suffix. - let prefix_max = MAX.saturating_sub(REWRITE_PREFIX.len() + 1 + 8); - let body = if safe.len() > prefix_max { - &safe[..prefix_max] - } else { - &safe - }; - format!("{}{}-{}", REWRITE_PREFIX, body, hash) -} + /// Construct a store from the process environment: `$HOME` (or + /// `$USERPROFILE`, falling back to `"."`) for the base dir, and + /// `AGENTKEYS_SESSION_STORE=file` for the keyring mode. This is the + /// production path and is what every legacy free-function wrapper + /// below resolves to. + pub fn from_env() -> Self { + let keyring_mode = match std::env::var("AGENTKEYS_SESSION_STORE").as_deref() { + Ok("file") => KeyringMode::FileOnly, + _ => KeyringMode::Auto, + }; + Self::new(home_dir_from_env(), keyring_mode) + } -/// Returns true if keyring should be skipped (tests, CI, headless). -/// Set AGENTKEYS_SESSION_STORE=file to force file-only mode. -fn should_skip_keyring() -> bool { - std::env::var("AGENTKEYS_SESSION_STORE") - .map(|v| v == "file") - .unwrap_or(false) -} + /// The base directory this store is rooted at. Everything lives under + /// `/.agentkeys/`. + pub fn base_dir(&self) -> &Path { + &self.base_dir + } -/// Save session under session_id. Tries keyring first (non-blocking, 2s timeout), -/// falls back to ~/.agentkeys//session.json (mode 0600). -/// Set AGENTKEYS_SESSION_STORE=file to skip keyring entirely. -/// -/// On a successful keyring save, also drops an empty `.keyring_managed` -/// marker file in ~/.agentkeys// so `list_fallback_session_ids` -/// can discover keyring-stored sessions (OS keychain APIs don't expose a -/// prefix-scan without per-entry permission prompts). The marker is NEVER -/// written over an existing session.json, so toggling between keyring and -/// file modes doesn't destroy the real fallback (codex PR #24 P2). -pub fn save_session(session: &Session, session_id: &str) -> Result<()> { - let json = serde_json::to_string(session).context("serialize session")?; + /// The configured keyring mode. + pub fn keyring_mode(&self) -> KeyringMode { + self.keyring_mode + } + + fn skip_keyring(&self) -> bool { + matches!(self.keyring_mode, KeyringMode::FileOnly) + } + + fn session_dir(&self, session_id: &str) -> PathBuf { + // Route through sanitize_for_keyring so session_ids containing path + // separators, '..', or null bytes can't escape ~/.agentkeys (codex PR + // #24 v2 P2 — path traversal via --session-id). + self.base_dir + .join(AGENTKEYS_DIR) + .join(sanitize_for_keyring(session_id)) + } + + /// Path to the on-disk `session.json` for `session_id`. Exposed for + /// tests that want to assert presence/absence without reaching through + /// save/load. + pub fn session_path(&self, session_id: &str) -> PathBuf { + self.session_dir(session_id).join(SESSION_FILE) + } + + fn marker_path(&self, session_id: &str) -> PathBuf { + self.session_dir(session_id).join(KEYRING_MARKER_FILE) + } - if !should_skip_keyring() { - if try_keyring_save(&json, session_id) { + /// Save `session` under `session_id`. Tries the keyring first (when + /// `KeyringMode::Auto`), falls back to + /// `/.agentkeys//session.json` (mode 0600). + /// + /// On a successful keyring save, also drops an empty `.keyring_managed` + /// marker file so `list_ids` can discover keyring-stored sessions (OS + /// keychain APIs don't expose a prefix-scan without per-entry + /// permission prompts). The marker is NEVER written over an existing + /// session.json, so toggling between keyring and file modes doesn't + /// destroy the real fallback (codex PR #24 P2). + pub fn save(&self, session: &Session, session_id: &str) -> Result<()> { + let json = serde_json::to_string(session).context("serialize session")?; + + if !self.skip_keyring() && try_keyring_save(&json, session_id) { // Marker file is best-effort: it's only required for // prefix-scan discovery (daemon-restart path). Direct-load // callers like `master` look up by known id, so a missing @@ -125,7 +142,7 @@ pub fn save_session(session: &Session, session_id: &str) -> Result<()> { // (read-only HOME, missing filesystem), the keyring entry // is still the authoritative store — emit a diagnostic on // stderr and return Ok (codex PR #24 v4 P2). - let marker = marker_path(session_id); + let marker = self.marker_path(session_id); if let Some(parent) = marker.parent() { if let Err(e) = std::fs::create_dir_all(parent) { eprintln!( @@ -145,113 +162,244 @@ pub fn save_session(session: &Session, session_id: &str) -> Result<()> { } return Ok(()); } - } - save_to_file(&json, session_id) -} + self.save_to_file(&json, session_id) + } -/// Load session for session_id. Tries keyring first (non-blocking, 2s timeout), -/// falls back to file. -pub fn load_session(session_id: &str) -> Result { - if !should_skip_keyring() { - if let Some(json) = try_keyring_load(session_id) { - return serde_json::from_str(&json).context("deserialize session from keyring"); + /// Load the session for `session_id`. Tries the keyring first (when + /// `KeyringMode::Auto`, bounded by a 2s timeout), falls back to the + /// file. + pub fn load(&self, session_id: &str) -> Result { + if !self.skip_keyring() { + if let Some(json) = try_keyring_load(session_id) { + return serde_json::from_str(&json).context("deserialize session from keyring"); + } } + self.load_from_file(session_id) } - load_from_file(session_id) -} -/// Enumerate session IDs that have a persisted session under `~/.agentkeys/`. -/// Looks for either a real `session.json` (file mode) or the -/// `.keyring_managed` marker (keyring mode) in each candidate directory. -/// Results are sorted alphabetically so daemon startup is deterministic -/// across runs (codex PR #24 P1 — nondeterministic daemon selection). -/// -/// Keyring-only entries without a marker are NOT enumerated — we rely on -/// the marker file as the discovery signal because most OS keychain APIs -/// don't support prefix-scan without per-entry permission prompts. -pub fn list_fallback_session_ids(prefix: &str) -> Vec { - let home = std::env::var("HOME") - .or_else(|_| std::env::var("USERPROFILE")) - .unwrap_or_else(|_| ".".to_string()); - let root = PathBuf::from(home).join(".agentkeys"); - let mut out = Vec::new(); - let Ok(rd) = std::fs::read_dir(&root) else { - return out; - }; - for entry in rd.flatten() { - if !entry.file_type().map(|t| t.is_dir()).unwrap_or(false) { - continue; + /// Enumerate session IDs that have a persisted session under + /// `/.agentkeys/`. Looks for either a real `session.json` + /// (file mode) or the `.keyring_managed` marker (keyring mode) in each + /// candidate directory. Results are sorted alphabetically so daemon + /// startup is deterministic across runs (codex PR #24 P1). + pub fn list_ids(&self, prefix: &str) -> Vec { + let root = self.base_dir.join(AGENTKEYS_DIR); + let mut out = Vec::new(); + let Ok(rd) = std::fs::read_dir(&root) else { + return out; + }; + for entry in rd.flatten() { + if !entry.file_type().map(|t| t.is_dir()).unwrap_or(false) { + continue; + } + let name = entry.file_name().to_string_lossy().into_owned(); + if !name.starts_with(prefix) { + continue; + } + let dir = entry.path(); + if dir.join(SESSION_FILE).exists() || dir.join(KEYRING_MARKER_FILE).exists() { + out.push(name); + } } - let name = entry.file_name().to_string_lossy().into_owned(); - if !name.starts_with(prefix) { - continue; + out.sort(); + out + } + + /// Load a session with legacy-location fallback. Used by the master + /// CLI (session_id = "master") after #12 namespacing — old installs + /// have the session stored under keyring account=`session` or file + /// `/.agentkeys/session.json`. Try the new location first, + /// then fall back to the legacy locations so existing users stay + /// logged in across the upgrade. + pub fn load_with_legacy_fallback(&self, session_id: &str) -> Result { + if let Ok(s) = self.load(session_id) { + return Ok(s); } - let dir = entry.path(); - if dir.join("session.json").exists() || dir.join(KEYRING_MARKER_FILE).exists() { - out.push(name); + if session_id == "master" { + // Legacy keyring account: "session" + if !self.skip_keyring() { + if let Some(json) = try_keyring_load("session") { + return serde_json::from_str(&json) + .context("deserialize legacy session from keyring"); + } + } + // Legacy file: /.agentkeys/session.json + let legacy = self.base_dir.join(AGENTKEYS_DIR).join(SESSION_FILE); + if let Ok(json) = std::fs::read_to_string(&legacy) { + return serde_json::from_str(&json) + .context("deserialize legacy session from file"); + } } + self.load(session_id) } - out.sort(); - out -} -/// Load a session with legacy-location fallback. Used by the master CLI -/// (session_id = "master") after #12 namespacing — old installs have the -/// session stored under keyring account=`session` or file -/// `~/.agentkeys/session.json`. Try the new location first, then fall -/// back to the legacy locations so existing users stay logged in across -/// the upgrade. -pub fn load_session_with_legacy_fallback(session_id: &str) -> Result { - if let Ok(s) = load_session(session_id) { - return Ok(s); - } - if session_id == "master" { - // Legacy keyring account: "session" - if !should_skip_keyring() { - if let Some(json) = try_keyring_load("session") { - return serde_json::from_str(&json) - .context("deserialize legacy session from keyring"); + /// Remove the session entry for `session_id` only (does not affect + /// other ids). Blocks on the keyring delete (up to 2 seconds) so + /// callers know the credential is actually gone before `clear` + /// returns. Previously fire-and-forget, which let `cmd_revoke` report + /// success while the keyring entry was still live — next command + /// would load the stale session (codex PR #24 v8 P1). + pub fn clear(&self, session_id: &str) -> Result<()> { + if !self.skip_keyring() { + let deleted = try_keyring_delete(session_id); + if !deleted { + return Err(anyhow::anyhow!( + "keyring delete failed or timed out for session_id={session_id}; local session may still be loadable on next command" + )); } } - // Legacy file: ~/.agentkeys/session.json - let home = std::env::var("HOME") - .or_else(|_| std::env::var("USERPROFILE")) - .unwrap_or_else(|_| ".".to_string()); - let legacy = PathBuf::from(home).join(".agentkeys").join("session.json"); - if let Ok(json) = std::fs::read_to_string(&legacy) { - return serde_json::from_str(&json) - .context("deserialize legacy session from file"); + let path = self.session_path(session_id); + if path.exists() { + std::fs::remove_file(&path) + .with_context(|| format!("remove session file {}", path.display()))?; + } + let marker = self.marker_path(session_id); + if marker.exists() { + let _ = std::fs::remove_file(&marker); } + Ok(()) } - load_session(session_id) -} -/// Remove session entry for session_id only (does not affect other ids). -/// Blocks on the keyring delete (up to 2 seconds) so callers know the -/// credential is actually gone before clear_session returns. Previously -/// fire-and-forget, which let `cmd_revoke` report success while the -/// keyring entry was still live — next command would load the stale -/// session (codex PR #24 v8 P1). -pub fn clear_session(session_id: &str) -> Result<()> { - if !should_skip_keyring() { - let deleted = try_keyring_delete(session_id); - if !deleted { - return Err(anyhow::anyhow!( - "keyring delete failed or timed out for session_id={session_id}; local session may still be loadable on next command" - )); + fn save_to_file(&self, json: &str, session_id: &str) -> Result<()> { + let path = self.session_path(session_id); + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent) + .with_context(|| format!("create dir {}", parent.display()))?; + } + + #[cfg(unix)] + { + use std::io::Write; + use std::os::unix::fs::OpenOptionsExt; + let mut opts = std::fs::OpenOptions::new(); + opts.write(true).create(true).truncate(true).mode(0o600); + let mut file = opts + .open(&path) + .with_context(|| format!("open session file {}", path.display()))?; + file.write_all(json.as_bytes()) + .with_context(|| format!("write session file {}", path.display()))?; } + + #[cfg(not(unix))] + { + std::fs::write(&path, json) + .with_context(|| format!("write session file {}", path.display()))?; + } + + Ok(()) } - let path = fallback_path(session_id); - if path.exists() { - std::fs::remove_file(&path) - .with_context(|| format!("remove session file {}", path.display()))?; + + fn load_from_file(&self, session_id: &str) -> Result { + let path = self.session_path(session_id); + let json = std::fs::read_to_string(&path) + .with_context(|| format!("read session file at {}", path.display()))?; + serde_json::from_str(&json).context("deserialize session from file") } - let marker = marker_path(session_id); - if marker.exists() { - let _ = std::fs::remove_file(&marker); +} + +fn home_dir_from_env() -> PathBuf { + let home = std::env::var("HOME") + .or_else(|_| std::env::var("USERPROFILE")) + .unwrap_or_else(|_| ".".to_string()); + PathBuf::from(home) +} + +// ---- Legacy free-function API ------------------------------------------- +// +// Thin wrappers that construct `SessionStore::from_env()` on every call, +// preserving the behavior the daemon, older CLI helpers, and integration +// tests already depend on. New code should prefer building a `SessionStore` +// explicitly so the base dir and keyring mode are visible at the call site. + +/// Path to the on-disk session.json for `session_id`, resolved from +/// `$HOME`. Exposed for tests and for legacy callers — new code should use +/// [`SessionStore::session_path`] on a store you own. +pub fn fallback_path(session_id: &str) -> PathBuf { + SessionStore::from_env().session_path(session_id) +} + +/// Save `session` under `session_id` using a store built from the process +/// env. See [`SessionStore::save`] for semantics. +pub fn save_session(session: &Session, session_id: &str) -> Result<()> { + SessionStore::from_env().save(session, session_id) +} + +/// Load the session for `session_id` using a store built from the process +/// env. See [`SessionStore::load`] for semantics. +pub fn load_session(session_id: &str) -> Result { + SessionStore::from_env().load(session_id) +} + +/// Enumerate session IDs under `$HOME/.agentkeys/` with the given prefix. +/// See [`SessionStore::list_ids`] for semantics. +pub fn list_fallback_session_ids(prefix: &str) -> Vec { + SessionStore::from_env().list_ids(prefix) +} + +/// Load a session with legacy-location fallback using a store built from +/// the process env. See [`SessionStore::load_with_legacy_fallback`] for +/// semantics. +pub fn load_session_with_legacy_fallback(session_id: &str) -> Result { + SessionStore::from_env().load_with_legacy_fallback(session_id) +} + +/// Remove the session entry for `session_id` using a store built from the +/// process env. See [`SessionStore::clear`] for semantics. +pub fn clear_session(session_id: &str) -> Result<()> { + SessionStore::from_env().clear(session_id) +} + +/// Sanitize `session_id` for use as a keyring account name AND filesystem +/// directory name. Windows Credential Manager rejects null bytes, Linux +/// `secret-service` rejects non-UTF8 and is quirky about shell-reserved +/// chars, macOS is tolerant. Any filesystem rejects `""`, `"."`, `".."` +/// as path components (traversal vectors). +/// +/// Accept-as-is rule: ASCII alnum + `-_.`, unchanged, non-empty, non-reserved +/// (not `"."` / `".."`), not starting with `REWRITE_PREFIX`, ≤128 chars. +/// Anything failing those rules goes through the stable rewrite path: +/// `__agk_-` +/// SHA-256 (not `DefaultHasher`) keeps the suffix stable across Rust +/// toolchain versions so persisted IDs remain reachable after upgrades +/// (codex PR #24 v3 P2). +pub(crate) fn sanitize_for_keyring(s: &str) -> String { + const MAX: usize = 128; + + let safe: String = s + .chars() + .map(|c| { + if c.is_ascii_alphanumeric() || matches!(c, '-' | '_' | '.') { + c + } else { + '_' + } + }) + .collect(); + + // Force rewrite if the input is empty, a reserved path component, starts + // with the reserved rewrite prefix, exceeds the length cap, or was + // normalised (sanitized differs from original). Path traversal via `..` + // is explicitly blocked by the reserved-path check (codex PR #24 v6 P1). + let is_reserved = s.is_empty() || s == "." || s == ".."; + let starts_with_prefix = s.starts_with(REWRITE_PREFIX); + let accepts_as_is = safe == s && safe.len() <= MAX && !is_reserved && !starts_with_prefix; + + if accepts_as_is { + return safe; } - Ok(()) + + use sha2::{Digest, Sha256}; + let digest = Sha256::digest(s.as_bytes()); + let hash = hex::encode(&digest[..4]); // 8 hex chars + // Reserve room for the prefix + '-' + 8-char suffix. + let prefix_max = MAX.saturating_sub(REWRITE_PREFIX.len() + 1 + 8); + let body = if safe.len() > prefix_max { + &safe[..prefix_max] + } else { + &safe + }; + format!("{}{}-{}", REWRITE_PREFIX, body, hash) } fn try_keyring_save(json: &str, session_id: &str) -> bool { @@ -309,42 +457,6 @@ fn try_keyring_delete(session_id: &str) -> bool { .unwrap_or(false) } -fn save_to_file(json: &str, session_id: &str) -> Result<()> { - let path = fallback_path(session_id); - if let Some(parent) = path.parent() { - std::fs::create_dir_all(parent) - .with_context(|| format!("create dir {}", parent.display()))?; - } - - #[cfg(unix)] - { - use std::io::Write; - use std::os::unix::fs::OpenOptionsExt; - let mut opts = std::fs::OpenOptions::new(); - opts.write(true).create(true).truncate(true).mode(0o600); - let mut file = opts - .open(&path) - .with_context(|| format!("open session file {}", path.display()))?; - file.write_all(json.as_bytes()) - .with_context(|| format!("write session file {}", path.display()))?; - } - - #[cfg(not(unix))] - { - std::fs::write(&path, json) - .with_context(|| format!("write session file {}", path.display()))?; - } - - Ok(()) -} - -fn load_from_file(session_id: &str) -> Result { - let path = fallback_path(session_id); - let json = std::fs::read_to_string(&path) - .with_context(|| format!("read session file at {}", path.display()))?; - serde_json::from_str(&json).context("deserialize session from file") -} - #[cfg(test)] mod tests { use super::*; @@ -360,98 +472,91 @@ mod tests { } } - /// Run a closure with AGENTKEYS_SESSION_STORE=file and HOME pointing at a unique tempdir. - /// Uses a mutex to prevent concurrent tests from clobbering the shared process environment. - fn with_temp_home(f: F) { - static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); - let _guard = ENV_LOCK.lock().expect("env lock"); + /// Build a `SessionStore` rooted at a fresh tempdir with keyring + /// disabled. Returns the store together with the tempdir so the + /// caller can keep it alive for the lifetime of the test — dropping + /// the tempdir would wipe the session state mid-assertion. + /// + /// Replaces the previous `with_temp_home` helper which mutated + /// process-global `$HOME` / `AGENTKEYS_SESSION_STORE` under a mutex. + /// Tests are now fully hermetic and can run in parallel without a + /// shared lock. + fn test_store() -> (SessionStore, tempfile::TempDir) { let tmp = tempfile::tempdir().expect("tempdir"); - unsafe { - std::env::set_var("AGENTKEYS_SESSION_STORE", "file"); - std::env::set_var("HOME", tmp.path().to_str().unwrap()); - } - f(tmp.path()); - unsafe { - std::env::remove_var("AGENTKEYS_SESSION_STORE"); - } - drop(tmp); + let store = SessionStore::file_only(tmp.path().to_path_buf()); + (store, tmp) } #[test] fn save_load_session_roundtrip_master() { - with_temp_home(|_| { - let session = make_session("tok-master", "0xMASTER"); - save_session(&session, "master").expect("save"); - let loaded = load_session("master").expect("load"); - assert_eq!(loaded.token, "tok-master"); - assert_eq!(loaded.wallet.0, "0xMASTER"); - }); + let (store, _tmp) = test_store(); + let session = make_session("tok-master", "0xMASTER"); + store.save(&session, "master").expect("save"); + let loaded = store.load("master").expect("load"); + assert_eq!(loaded.token, "tok-master"); + assert_eq!(loaded.wallet.0, "0xMASTER"); } #[test] fn save_load_session_roundtrip_daemon_wallet() { - with_temp_home(|_| { - let session = make_session("tok-daemon", "0xABC"); - save_session(&session, "daemon-0xABC").expect("save"); - let loaded = load_session("daemon-0xABC").expect("load"); - assert_eq!(loaded.token, "tok-daemon"); - assert_eq!(loaded.wallet.0, "0xABC"); - }); + let (store, _tmp) = test_store(); + let session = make_session("tok-daemon", "0xABC"); + store.save(&session, "daemon-0xABC").expect("save"); + let loaded = store.load("daemon-0xABC").expect("load"); + assert_eq!(loaded.token, "tok-daemon"); + assert_eq!(loaded.wallet.0, "0xABC"); } #[test] fn two_daemons_do_not_collide() { - with_temp_home(|_| { - let sess_a = make_session("tok-a", "0xA"); - let sess_b = make_session("tok-b", "0xB"); - save_session(&sess_a, "daemon-A").expect("save A"); - save_session(&sess_b, "daemon-B").expect("save B"); - - let loaded_a = load_session("daemon-A").expect("load A"); - let loaded_b = load_session("daemon-B").expect("load B"); - assert_eq!(loaded_a.token, "tok-a"); - assert_eq!(loaded_b.token, "tok-b"); - assert_ne!(loaded_a.token, loaded_b.token); - }); + let (store, _tmp) = test_store(); + let sess_a = make_session("tok-a", "0xA"); + let sess_b = make_session("tok-b", "0xB"); + store.save(&sess_a, "daemon-A").expect("save A"); + store.save(&sess_b, "daemon-B").expect("save B"); + + let loaded_a = store.load("daemon-A").expect("load A"); + let loaded_b = store.load("daemon-B").expect("load B"); + assert_eq!(loaded_a.token, "tok-a"); + assert_eq!(loaded_b.token, "tok-b"); + assert_ne!(loaded_a.token, loaded_b.token); } #[test] fn clear_session_removes_entry_only_for_that_id() { - with_temp_home(|_| { - let sess_master = make_session("tok-master", "0xMASTER"); - let sess_daemon = make_session("tok-daemon", "0xDAEMON"); - save_session(&sess_master, "master").expect("save master"); - save_session(&sess_daemon, "daemon-0xDAEMON").expect("save daemon"); + let (store, _tmp) = test_store(); + let sess_master = make_session("tok-master", "0xMASTER"); + let sess_daemon = make_session("tok-daemon", "0xDAEMON"); + store.save(&sess_master, "master").expect("save master"); + store.save(&sess_daemon, "daemon-0xDAEMON").expect("save daemon"); - clear_session("daemon-0xDAEMON").expect("clear daemon"); + store.clear("daemon-0xDAEMON").expect("clear daemon"); - let loaded = load_session("master").expect("load master after clear"); - assert_eq!(loaded.token, "tok-master"); + let loaded = store.load("master").expect("load master after clear"); + assert_eq!(loaded.token, "tok-master"); - assert!(load_session("daemon-0xDAEMON").is_err()); - }); + assert!(store.load("daemon-0xDAEMON").is_err()); } - // Codex PR #24 P1 — list_fallback_session_ids must sort deterministically. + // Codex PR #24 P1 — list_ids must sort deterministically. #[test] - fn list_fallback_session_ids_is_sorted() { - with_temp_home(|_| { - // Insert in non-alphabetical order; enumerate must still return sorted. - save_session(&make_session("t1", "0xZ"), "daemon-0xZZZ").expect("save Z"); - save_session(&make_session("t2", "0xA"), "daemon-0xAAA").expect("save A"); - save_session(&make_session("t3", "0xM"), "daemon-0xMMM").expect("save M"); - - let ids = list_fallback_session_ids("daemon-"); - assert_eq!( - ids, - vec![ - "daemon-0xAAA".to_string(), - "daemon-0xMMM".to_string(), - "daemon-0xZZZ".to_string(), - ], - "daemon session ids must be sorted alphabetically" - ); - }); + fn list_ids_is_sorted() { + let (store, _tmp) = test_store(); + // Insert in non-alphabetical order; enumerate must still return sorted. + store.save(&make_session("t1", "0xZ"), "daemon-0xZZZ").expect("save Z"); + store.save(&make_session("t2", "0xA"), "daemon-0xAAA").expect("save A"); + store.save(&make_session("t3", "0xM"), "daemon-0xMMM").expect("save M"); + + let ids = store.list_ids("daemon-"); + assert_eq!( + ids, + vec![ + "daemon-0xAAA".to_string(), + "daemon-0xMMM".to_string(), + "daemon-0xZZZ".to_string(), + ], + "daemon session ids must be sorted alphabetically" + ); } // Codex PR #24 P1 — keyring account name must be sanitized for @@ -535,22 +640,21 @@ mod tests { // Codex PR #24 P2 — keyring save must never overwrite the real file // fallback's session.json. The marker file is a separate `.keyring_managed`. - // This test runs in AGENTKEYS_SESSION_STORE=file mode (no keyring), so - // we verify directly: save writes session.json (not the marker), and - // toggling back to keyring mode (if it were active) would write the - // marker without touching session.json. + // This test runs in file-only mode (no keyring), so we verify directly: + // save writes session.json (not the marker), and toggling back to + // keyring mode (if it were active) would write the marker without + // touching session.json. #[test] fn file_mode_save_writes_session_json_not_marker() { - with_temp_home(|tmp| { - save_session(&make_session("t", "0xW"), "daemon-0xWWW").expect("save"); - let sess = tmp.join(".agentkeys").join("daemon-0xWWW").join("session.json"); - let marker = tmp.join(".agentkeys").join("daemon-0xWWW").join(".keyring_managed"); - assert!(sess.exists(), "session.json must exist in file mode"); - assert!( - !marker.exists(), - "file-mode save must not write the keyring marker" - ); - }); + let (store, tmp) = test_store(); + store.save(&make_session("t", "0xW"), "daemon-0xWWW").expect("save"); + let sess = tmp.path().join(AGENTKEYS_DIR).join("daemon-0xWWW").join(SESSION_FILE); + let marker = tmp.path().join(AGENTKEYS_DIR).join("daemon-0xWWW").join(KEYRING_MARKER_FILE); + assert!(sess.exists(), "session.json must exist in file mode"); + assert!( + !marker.exists(), + "file-mode save must not write the keyring marker" + ); } // Codex PR #24 v2 P2 — path traversal via user-supplied session_id. @@ -558,89 +662,85 @@ mod tests { // ~/.agentkeys/. Sanitization folds these to a safe directory name. #[test] fn save_session_does_not_escape_agentkeys_dir_on_path_traversal() { - with_temp_home(|tmp| { - let session = make_session("t", "0xP"); - // Attempt to escape via relative traversal. - save_session(&session, "../escape").expect("save should succeed (sanitized)"); - // Verify NO file was written outside ~/.agentkeys/. - let parent = tmp.parent().expect("tmp has a parent"); - let escape_candidates = [ - parent.join("escape"), - tmp.join("escape"), - tmp.join("..").join("escape"), - ]; - for bad in &escape_candidates { - assert!( - !bad.exists(), - "path traversal wrote outside ~/.agentkeys: {}", - bad.display() - ); - } - // Verify the actual write landed inside ~/.agentkeys/ under a - // sanitized directory name (contains the 8-char hash suffix). - let root = tmp.join(".agentkeys"); - let any_inside = std::fs::read_dir(&root) - .expect("read agentkeys root") - .filter_map(Result::ok) - .any(|e| e.path().join("session.json").exists()); - assert!(any_inside, "sanitized directory with session.json must exist inside ~/.agentkeys"); - }); + let (store, tmp) = test_store(); + let session = make_session("t", "0xP"); + // Attempt to escape via relative traversal. + store.save(&session, "../escape").expect("save should succeed (sanitized)"); + // Verify NO file was written outside the tempdir's .agentkeys/. + let parent = tmp.path().parent().expect("tmp has a parent"); + let escape_candidates = [ + parent.join("escape"), + tmp.path().join("escape"), + tmp.path().join("..").join("escape"), + ]; + for bad in &escape_candidates { + assert!( + !bad.exists(), + "path traversal wrote outside ~/.agentkeys: {}", + bad.display() + ); + } + // Verify the actual write landed inside .agentkeys/ under a + // sanitized directory name (contains the 8-char hash suffix). + let root = tmp.path().join(AGENTKEYS_DIR); + let any_inside = std::fs::read_dir(&root) + .expect("read agentkeys root") + .filter_map(Result::ok) + .any(|e| e.path().join(SESSION_FILE).exists()); + assert!(any_inside, "sanitized directory with session.json must exist inside ~/.agentkeys"); } #[test] fn save_session_rejects_forward_slash_in_session_id() { - with_temp_home(|tmp| { - save_session(&make_session("t", "0xS"), "foo/bar").expect("save"); - // The separator must be normalised, so no subdir named "bar" - // under an intermediate "foo" dir. - let unwanted = tmp.join(".agentkeys").join("foo").join("bar"); - assert!( - !unwanted.exists(), - "forward-slash session_id created nested directory: {}", - unwanted.display() - ); - }); + let (store, tmp) = test_store(); + store.save(&make_session("t", "0xS"), "foo/bar").expect("save"); + // The separator must be normalised, so no subdir named "bar" + // under an intermediate "foo" dir. + let unwanted = tmp.path().join(AGENTKEYS_DIR).join("foo").join("bar"); + assert!( + !unwanted.exists(), + "forward-slash session_id created nested directory: {}", + unwanted.display() + ); } - // Codex PR #24 v8 P1 — clear_session must be synchronous. - // In file-mode (AGENTKEYS_SESSION_STORE=file) the keyring path is - // skipped entirely, so clear_session must succeed and wipe the file - // immediately. After it returns, load_session must not succeed. + // Codex PR #24 v8 P1 — clear must be synchronous. + // In file-only mode the keyring path is skipped entirely, so clear + // must succeed and wipe the file immediately. After it returns, load + // must not succeed. #[test] fn clear_session_is_synchronous_in_file_mode() { - with_temp_home(|_| { - save_session(&make_session("t", "0xC"), "daemon-0xCCC").expect("save"); - assert!(load_session("daemon-0xCCC").is_ok(), "session loadable before clear"); + let (store, _tmp) = test_store(); + store.save(&make_session("t", "0xC"), "daemon-0xCCC").expect("save"); + assert!(store.load("daemon-0xCCC").is_ok(), "session loadable before clear"); - clear_session("daemon-0xCCC").expect("clear"); + store.clear("daemon-0xCCC").expect("clear"); - // Immediately (no sleep) — the deletion must have happened - // synchronously inside clear_session, not in a detached thread. - assert!( - load_session("daemon-0xCCC").is_err(), - "session still loadable after clear_session returned" - ); - }); + // Immediately (no sleep) — the deletion must have happened + // synchronously inside clear, not in a detached thread. + assert!( + store.load("daemon-0xCCC").is_err(), + "session still loadable after clear returned" + ); } - // Verifies list_fallback_session_ids discovers both a real session.json - // entry AND a marker-only entry (would-be keyring-managed in prod). + // Verifies list_ids discovers both a real session.json entry AND a + // marker-only entry (would-be keyring-managed in prod). #[test] - fn list_fallback_session_ids_finds_marker_only_directories() { - with_temp_home(|tmp| { - save_session(&make_session("t1", "0xF"), "daemon-0xFFF").expect("save file"); + fn list_ids_finds_marker_only_directories() { + let (store, tmp) = test_store(); + store.save(&make_session("t1", "0xF"), "daemon-0xFFF").expect("save file"); - // Simulate a keyring-managed session: directory with only the marker. - let dir = tmp.join(".agentkeys").join("daemon-0xKEY"); - std::fs::create_dir_all(&dir).unwrap(); - std::fs::File::create(dir.join(".keyring_managed")).unwrap(); + // Simulate a keyring-managed session: directory with only the marker. + let dir = tmp.path().join(AGENTKEYS_DIR).join("daemon-0xKEY"); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::File::create(dir.join(KEYRING_MARKER_FILE)).unwrap(); - let ids = list_fallback_session_ids("daemon-"); - assert!(ids.contains(&"daemon-0xFFF".to_string())); - assert!( - ids.contains(&"daemon-0xKEY".to_string()), - "marker-only entries must be discoverable, got: {ids:?}" - ); - }); + let ids = store.list_ids("daemon-"); + assert!(ids.contains(&"daemon-0xFFF".to_string())); + assert!( + ids.contains(&"daemon-0xKEY".to_string()), + "marker-only entries must be discoverable, got: {ids:?}" + ); } } From 8ce0f55e10fddd9e5d7f49471d43c39ad2782381 Mon Sep 17 00:00:00 2001 From: Hanwen Cheng Date: Thu, 16 Apr 2026 13:20:52 +0800 Subject: [PATCH 2/3] fix(session_store): drop public SessionStore::new to enforce isolation-by-construction (codex PR #43 [P2]) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex /codex review on PR #43 flagged that `SessionStore::new(base_dir, KeyringMode::Auto)` was unsafe: keyring entries are keyed on `session_id` alone and ignore `base_dir`, so two stores at different roots sharing a `session_id` would silently alias through the OS keychain. The newly introduced API promised per-root isolation but the `Auto` path violated that promise. Fix: remove the public `SessionStore::new(base_dir, mode)` constructor. The only public constructors are now: - `SessionStore::file_only(base_dir)` — explicit custom root, always file-only, safe by construction - `SessionStore::from_env()` — home-rooted (the single-root invariant the keyring namespace assumes), may select `KeyringMode::Auto` Custom-root callers physically cannot opt into keyring aliasing. No callers (internal or external) used `::new` with `KeyringMode::Auto` and a custom `base_dir`, so no migration is needed. Tests: `cargo test -p agentkeys-core -p agentkeys-cli` → 22 core + 37 cli, 0 failures, 0.78s. Refs #34, addresses codex [P2] on PR #43. --- crates/agentkeys-core/src/session_store.rs | 30 ++++++++++++++-------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/crates/agentkeys-core/src/session_store.rs b/crates/agentkeys-core/src/session_store.rs index 01d17fa..297e415 100644 --- a/crates/agentkeys-core/src/session_store.rs +++ b/crates/agentkeys-core/src/session_store.rs @@ -58,32 +58,40 @@ pub struct SessionStore { } impl SessionStore { - /// Construct a store rooted at `base_dir` with the given `keyring_mode`. - pub fn new(base_dir: impl Into, keyring_mode: KeyringMode) -> Self { - Self { - base_dir: base_dir.into(), - keyring_mode, - } - } - /// Construct a store rooted at `base_dir` that never touches the OS /// keyring. Intended for tests and headless environments — lets a /// tempdir-scoped test avoid both `$HOME` mutation and the keychain. + /// + /// This is the only public constructor that accepts a custom `base_dir`. + /// `KeyringMode::Auto` is intentionally not offered for custom roots: + /// keyring entries are keyed on `session_id` alone and do not incorporate + /// `base_dir`, so two stores at different roots sharing a `session_id` + /// would silently alias through the OS keychain. Forcing the file path + /// for custom roots keeps isolation by construction (codex /codex review + /// on PR #43 [P2]). pub fn file_only(base_dir: impl Into) -> Self { - Self::new(base_dir, KeyringMode::FileOnly) + Self { + base_dir: base_dir.into(), + keyring_mode: KeyringMode::FileOnly, + } } /// Construct a store from the process environment: `$HOME` (or /// `$USERPROFILE`, falling back to `"."`) for the base dir, and /// `AGENTKEYS_SESSION_STORE=file` for the keyring mode. This is the /// production path and is what every legacy free-function wrapper - /// below resolves to. + /// below resolves to. It is also the only constructor that may return + /// `KeyringMode::Auto` — the home-rooted single-root invariant the + /// keyring namespace assumes. pub fn from_env() -> Self { let keyring_mode = match std::env::var("AGENTKEYS_SESSION_STORE").as_deref() { Ok("file") => KeyringMode::FileOnly, _ => KeyringMode::Auto, }; - Self::new(home_dir_from_env(), keyring_mode) + Self { + base_dir: home_dir_from_env(), + keyring_mode, + } } /// The base directory this store is rooted at. Everything lives under From 11f9af5df27ea6eef28e39e35f23410c1cc9c2ed Mon Sep 17 00:00:00 2001 From: Hanwen Cheng Date: Thu, 16 Apr 2026 14:17:37 +0800 Subject: [PATCH 3/3] fix(session_store): return a real error from load_with_legacy_fallback fallthrough (claude PR #43 review #1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The trailing `self.load(session_id)` at the end of `load_with_legacy_fallback` was redundant — the same call already failed at the top of the function, so re-running it produced the same error with no added context about which legacy fallbacks were tried. Replace with `anyhow::bail!` that names the attempted session path, the keyring mode, and the fact that legacy fallbacks only trigger for `id="master"`. Debuggability win; same observable behavior (still returns Err). Tests: `cargo test -p agentkeys-core -p agentkeys-cli` → 22 + 37, 0 failures. --- crates/agentkeys-core/src/session_store.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/agentkeys-core/src/session_store.rs b/crates/agentkeys-core/src/session_store.rs index 297e415..0eacd92 100644 --- a/crates/agentkeys-core/src/session_store.rs +++ b/crates/agentkeys-core/src/session_store.rs @@ -239,7 +239,11 @@ impl SessionStore { .context("deserialize legacy session from file"); } } - self.load(session_id) + anyhow::bail!( + "no session found for id={session_id} at {} (keyring mode: {:?}; legacy fallbacks apply only to id=\"master\")", + self.session_path(session_id).display(), + self.keyring_mode, + ) } /// Remove the session entry for `session_id` only (does not affect