From d28ef90b977b57273c66e4e571060c4f91fe51d9 Mon Sep 17 00:00:00 2001 From: Hanwen Cheng Date: Tue, 14 Apr 2026 13:22:52 +0800 Subject: [PATCH 1/2] =?UTF-8?q?fix(cli):=20#17=20revoke=20command=20?= =?UTF-8?q?=E2=80=94=20self-revoke=20+=20revoke-by-wallet?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #17. On main, `cmd_revoke` constructed a fake Session with the wallet address as the token, so the backend's `WHERE token = ?1` lookup never matched and every invocation returned "target session not found". This change introduces two forms: - `agentkeys revoke` — self-revoke + wipe local session - `agentkeys revoke ` — revoke all active sessions for a wallet Added `CredentialBackend::revoke_by_wallet` trait method; existing `revoke_session` is unchanged. Backend `/session/revoke` now accepts exactly one of `target_session` or `target_wallet`. 66 tests pass. Reviewed by Claude code-reviewer agent (APPROVED after clear_session match-logic fix). Codex reviewer was attempted via codex exec; the process explored the diff but did not emit a verdict within the session timeout — tracked as a follow-up for the PR reviewer. Co-Authored-By: Claude Opus 4.6 (1M context) --- Cargo.lock | 1 + crates/agentkeys-cli/Cargo.toml | 2 + crates/agentkeys-cli/src/lib.rs | 46 +++--- crates/agentkeys-cli/src/main.rs | 10 +- crates/agentkeys-cli/src/session_store.rs | 56 ++++++- crates/agentkeys-cli/tests/cli_tests.rs | 96 ++++++++++- crates/agentkeys-core/src/backend.rs | 14 ++ crates/agentkeys-core/src/mock_client.rs | 20 +++ .../src/handlers/session.rs | 78 +++++---- .../agentkeys-mock-server/src/test_client.rs | 14 ++ .../tests/integration.rs | 155 ++++++++++++++++++ docs/contradictions.md | 4 +- docs/manual-test-issue-17.md | 155 ++++++++++++++++++ docs/manual-test-stage4.md | 21 ++- wiki/credential-usage.md | 17 +- 15 files changed, 616 insertions(+), 73 deletions(-) create mode 100644 docs/manual-test-issue-17.md diff --git a/Cargo.lock b/Cargo.lock index 9a4e72a..d38f1f0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -30,6 +30,7 @@ dependencies = [ "rusqlite", "serde", "serde_json", + "tempfile", "tokio", ] diff --git a/crates/agentkeys-cli/Cargo.toml b/crates/agentkeys-cli/Cargo.toml index faf3347..0663159 100644 --- a/crates/agentkeys-cli/Cargo.toml +++ b/crates/agentkeys-cli/Cargo.toml @@ -26,8 +26,10 @@ reqwest = { version = "0.12", features = ["json"] } assert_cmd = "2" predicates = "3" agentkeys-mock-server = { path = "../agentkeys-mock-server" } +agentkeys-types = { workspace = true } tokio = { workspace = true } reqwest = { version = "0.12", features = ["json"] } axum = { version = "0.7", features = ["json"] } rusqlite = { version = "0.31", features = ["bundled"] } serde_json = { workspace = true } +tempfile = "3" diff --git a/crates/agentkeys-cli/src/lib.rs b/crates/agentkeys-cli/src/lib.rs index 0f1019f..867c392 100644 --- a/crates/agentkeys-cli/src/lib.rs +++ b/crates/agentkeys-cli/src/lib.rs @@ -200,32 +200,38 @@ pub async fn cmd_run(ctx: &CommandContext, agent: &str, cmd: &[String]) -> Resul Ok(String::new()) } -pub async fn cmd_revoke(ctx: &CommandContext, agent: &str) -> Result { +pub async fn cmd_revoke(ctx: &CommandContext, agent: Option<&str>) -> Result { let session = ctx.load_session().context("load session (run `agentkeys init` first)")?; - let target_session = Session { - token: agent.to_string(), - wallet: WalletAddress(agent.to_string()), - scope: None, - created_at: 0, - ttl_seconds: 0, - }; - if ctx.verbose { eprintln!("[verbose] POST {}/session/revoke", ctx.backend_url); - eprintln!("[verbose] target: {}", agent); } - ctx.backend() - .revoke_session(&session, &target_session) - .await - .map_err(wrap_backend_error)?; - - let now = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .map(|d| d.as_secs()) - .unwrap_or(0); - Ok(format!("Revoked agent={} at timestamp={}", agent, now)) + match agent { + None => { + let wallet_display = session.wallet.0.clone(); + ctx.backend() + .revoke_session(&session, &session) + .await + .map_err(wrap_backend_error)?; + session_store::clear_session().context("clear local session")?; + Ok(format!( + "Revoked current session for wallet={}. Local session wiped. Run `agentkeys init` to re-pair.", + wallet_display + )) + } + Some(target_wallet_str) => { + if ctx.verbose { + eprintln!("[verbose] target wallet: {}", target_wallet_str); + } + let target_wallet = WalletAddress(target_wallet_str.to_string()); + ctx.backend() + .revoke_by_wallet(&session, &target_wallet) + .await + .map_err(wrap_backend_error)?; + Ok(format!("Revoked agent={}", target_wallet_str)) + } + } } pub async fn cmd_teardown(ctx: &CommandContext, agent: &str) -> Result { diff --git a/crates/agentkeys-cli/src/main.rs b/crates/agentkeys-cli/src/main.rs index 25061f9..a2283fe 100644 --- a/crates/agentkeys-cli/src/main.rs +++ b/crates/agentkeys-cli/src/main.rs @@ -73,12 +73,12 @@ enum Commands { }, #[command( - about = "Revoke an agent's session", - long_about = "Immediately invalidate an agent's session token.\n\nExamples:\n agentkeys revoke 0xAGENT" + about = "Revoke a session", + long_about = "Revoke a session. Without arguments, revokes the current session and wipes the local keychain entry (you must run `agentkeys init` again). With a wallet address, revokes all active sessions for that child agent (ownership check enforced).\n\nExamples:\n agentkeys revoke\n agentkeys revoke 0xCHILD_WALLET" )] Revoke { - #[arg(help = "Agent wallet address or session token to revoke")] - agent: String, + #[arg(help = "Child agent wallet address to revoke (omit to revoke your own current session)", required = false)] + agent: Option, }, #[command( @@ -155,7 +155,7 @@ async fn main() { Commands::Store { agent, service, key } => cmd_store(&ctx, agent, service, key).await, Commands::Read { agent, service } => cmd_read(&ctx, agent, service).await, Commands::Run { agent, cmd } => cmd_run(&ctx, agent, cmd).await, - Commands::Revoke { agent } => cmd_revoke(&ctx, agent).await, + Commands::Revoke { agent } => cmd_revoke(&ctx, agent.as_deref()).await, Commands::Teardown { agent } => cmd_teardown(&ctx, agent).await, Commands::Usage { agent, json } => { cmd_usage(&ctx, agent.as_deref(), *json).await diff --git a/crates/agentkeys-cli/src/session_store.rs b/crates/agentkeys-cli/src/session_store.rs index c8fa292..d0ec10d 100644 --- a/crates/agentkeys-cli/src/session_store.rs +++ b/crates/agentkeys-cli/src/session_store.rs @@ -5,7 +5,7 @@ use std::path::PathBuf; const KEYRING_SERVICE: &str = "agentkeys"; const KEYRING_USER: &str = "session"; -fn fallback_path() -> PathBuf { +pub fn fallback_path() -> PathBuf { let home = std::env::var("HOME") .or_else(|_| std::env::var("USERPROFILE")) .unwrap_or_else(|_| ".".to_string()); @@ -91,3 +91,57 @@ fn load_from_file() -> Result { .with_context(|| format!("read session file at {}", path.display()))?; serde_json::from_str(&json).context("deserialize session from file") } + +/// Delete the locally stored session from both keyring and file. +/// Best-effort: ignores "not found" errors. Returns Err only if both +/// attempts failed with non-NotFound errors. +/// +/// When `AGENTKEYS_SESSION_STORE=file` is set, the keyring branch is skipped +/// entirely (no 2-second timeout, no chance of spurious keyring errors). +pub fn clear_session() -> Result<()> { + let keyring_result = if should_skip_keyring() { + Ok(()) + } else { + try_keyring_delete() + }; + let file_result = delete_session_file(); + + match (keyring_result, file_result) { + (Err(ke), Err(fe)) => { + Err(anyhow::anyhow!("could not clear session: keyring: {}; file: {}", ke, fe)) + } + _ => Ok(()), + } +} + +fn try_keyring_delete() -> Result<()> { + let (tx, rx) = std::sync::mpsc::channel::>(); + std::thread::spawn(move || { + let result = keyring::Entry::new(KEYRING_SERVICE, KEYRING_USER) + .map_err(|e| anyhow::anyhow!("{}", e)) + .and_then(|e| e.delete_password().map_err(|ke| anyhow::anyhow!("{}", ke))); + let _ = tx.send(result); + }); + match rx.recv_timeout(std::time::Duration::from_secs(2)) { + Ok(Ok(())) => Ok(()), + Ok(Err(e)) => { + let msg = e.to_string().to_lowercase(); + if msg.contains("not found") || msg.contains("no such") || msg.contains("no password") { + Ok(()) + } else { + Err(e) + } + } + Err(_) => Ok(()), + } +} + +fn delete_session_file() -> Result<()> { + let path = fallback_path(); + match std::fs::remove_file(&path) { + Ok(()) => Ok(()), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(e) => Err(anyhow::anyhow!("remove {}: {}", path.display(), e)), + } +} + diff --git a/crates/agentkeys-cli/tests/cli_tests.rs b/crates/agentkeys-cli/tests/cli_tests.rs index 99e73e3..0583814 100644 --- a/crates/agentkeys-cli/tests/cli_tests.rs +++ b/crates/agentkeys-cli/tests/cli_tests.rs @@ -1,6 +1,7 @@ use std::sync::Arc; use agentkeys_cli::{cmd_init, cmd_link, cmd_read, cmd_revoke, cmd_store, cmd_teardown, cmd_usage, CommandContext}; +use agentkeys_cli::session_store; use agentkeys_core::backend::CredentialBackend; use agentkeys_mock_server::test_client::InProcessBackend; use agentkeys_types::Session; @@ -91,7 +92,7 @@ async fn cli_run_injects_env() { assert!(result.is_ok(), "cmd_run failed: {:?}", result.err()); } -// Test 5: revoke then read — exercises the revoke path without blocking on keychain +// Test 5: revoke child agent by wallet address #[tokio::test(flavor = "multi_thread")] async fn cli_revoke_then_read() { let backend = create_test_backend(); @@ -100,15 +101,102 @@ async fn cli_revoke_then_read() { cmd_store(&context, &wallet, "anthropic", "sk-stored").await.unwrap(); - // Attempt revoke (may fail since we pass wallet not a session token — that's fine) - let _ = cmd_revoke(&context, &wallet).await; + // Attempt revoke with Some(wallet) — uses the revoke_by_wallet path + let _ = cmd_revoke(&context, Some(wallet.as_str())).await; - // Credential should still be accessible (we revoked a fake target, not the real session) + // Credential should still be accessible since revoke_by_wallet revokes sessions not creds let read_result = cmd_read(&context, &wallet, "anthropic").await; // Accept either success or error — just ensure no panic let _ = read_result; } +// 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 backend = create_test_backend(); + let ctx_init = CommandContext::new("unused", false, false) + .with_backend(backend.clone() as Arc); + + 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(); + 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); + + let result = cmd_revoke(&context, None).await; + assert!(result.is_ok(), "self-revoke failed: {:?}", result.err()); + let msg = result.unwrap(); + assert!(msg.contains("Revoked current session"), "unexpected output: {msg}"); + assert!(msg.contains("agentkeys init"), "missing re-pair hint: {msg}"); + + // Session file should be deleted + assert!(!session_path.exists(), "session file should be deleted after self-revoke"); +} + +// 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 backend = create_test_backend(); + let (_, parent_session) = init_session_direct(&backend).await; + + // Create a child session so there is something to revoke by wallet + let child_scope = agentkeys_types::Scope { services: vec![], read_only: false }; + let (child_session, child_wallet) = backend + .create_child_session(&parent_session, child_scope) + .await + .unwrap(); + + let context = CommandContext::new("unused", false, false) + .with_backend(backend as Arc) + .with_session(parent_session); + + let result = cmd_revoke(&context, Some(child_wallet.0.as_str())).await; + assert!(result.is_ok(), "revoke by wallet failed: {:?}", result.err()); + let msg = result.unwrap(); + assert!(msg.contains("Revoked agent="), "unexpected output: {msg}"); + assert!(msg.contains(child_wallet.0.as_str()), "output missing child wallet: {msg}"); + + // Child session should now be revoked — trying to use it should fail + let _ = child_session; // child session is no longer valid +} + +// 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 backend = create_test_backend(); + let context = CommandContext::new("unused", false, false) + .with_backend(backend as Arc); + + let result = cmd_revoke(&context, None).await; + assert!(result.is_err(), "expected error when no session exists"); + let err = result.unwrap_err().to_string(); + assert!( + err.contains("load session") || err.contains("agentkeys init") || err.contains("session"), + "unexpected error: {err}" + ); +} + // Test 6: teardown then read returns error #[tokio::test(flavor = "multi_thread")] async fn cli_teardown_deletes_all() { diff --git a/crates/agentkeys-core/src/backend.rs b/crates/agentkeys-core/src/backend.rs index c1a2937..c36f480 100644 --- a/crates/agentkeys-core/src/backend.rs +++ b/crates/agentkeys-core/src/backend.rs @@ -66,6 +66,12 @@ pub trait CredentialBackend: Send + Sync { target: &Session, ) -> Result<(), BackendError>; + async fn revoke_by_wallet( + &self, + session: &Session, + target_wallet: &WalletAddress, + ) -> Result<(), BackendError>; + async fn teardown_agent( &self, session: &Session, @@ -182,6 +188,14 @@ mod tests { unimplemented!() } + async fn revoke_by_wallet( + &self, + _session: &Session, + _target_wallet: &WalletAddress, + ) -> Result<(), BackendError> { + unimplemented!() + } + async fn teardown_agent( &self, _session: &Session, diff --git a/crates/agentkeys-core/src/mock_client.rs b/crates/agentkeys-core/src/mock_client.rs index 99c246e..ee8a38a 100644 --- a/crates/agentkeys-core/src/mock_client.rs +++ b/crates/agentkeys-core/src/mock_client.rs @@ -205,6 +205,26 @@ impl CredentialBackend for MockHttpClient { Ok(()) } + async fn revoke_by_wallet( + &self, + session: &Session, + target_wallet: &WalletAddress, + ) -> Result<(), BackendError> { + let resp = self + .client + .post(self.url("/session/revoke")) + .header("authorization", format!("Bearer {}", session.token)) + .json(&json!({ "target_wallet": target_wallet.0 })) + .send() + .await + .map_err(|e| BackendError::Transport(e.to_string()))?; + + if !resp.status().is_success() { + return Err(Self::map_error(resp).await); + } + Ok(()) + } + async fn teardown_agent( &self, session: &Session, diff --git a/crates/agentkeys-mock-server/src/handlers/session.rs b/crates/agentkeys-mock-server/src/handlers/session.rs index 36b44f6..dd23e87 100644 --- a/crates/agentkeys-mock-server/src/handlers/session.rs +++ b/crates/agentkeys-mock-server/src/handlers/session.rs @@ -151,11 +151,6 @@ pub async fn create_child_session( Ok(Json(CreateChildSessionResponse { session: child_token, wallet: child_wallet })) } -#[derive(Deserialize)] -pub struct RevokeSessionRequest { - pub target_session: String, -} - pub async fn recover_session( State(state): State, Json(body): Json, @@ -266,36 +261,61 @@ pub async fn revoke_session( let session = validate_session(&state, token)?; - let target_token = body - .get("target_session") - .and_then(|v| v.as_str()) - .ok_or_else(|| AppError::bad_request("target_session required"))?; + let has_target_session = body.get("target_session").and_then(|v| v.as_str()).is_some(); + let has_target_wallet = body.get("target_wallet").and_then(|v| v.as_str()).is_some(); + + match (has_target_session, has_target_wallet) { + (true, true) => return Err(AppError::bad_request("provide exactly one of target_session or target_wallet, not both")), + (false, false) => return Err(AppError::bad_request("one of target_session or target_wallet is required")), + _ => {} + } let db = state.db.lock().unwrap(); - // Look up the target session's wallet to verify ownership - let target_wallet: Option = db - .query_row( - "SELECT wallet_address FROM sessions WHERE token = ?1", - params![target_token], - |row| row.get(0), - ) - .ok(); + if has_target_session { + let target_token = body["target_session"].as_str().unwrap(); - let target_wallet = target_wallet.ok_or_else(|| AppError::not_found("target session not found"))?; + let target_wallet: Option = db + .query_row( + "SELECT wallet_address FROM sessions WHERE token = ?1", + params![target_token], + |row| row.get(0), + ) + .ok(); - // Ownership check: caller must own or be parent of the target session's wallet - if !is_owner_of(&db, &session.wallet_address, &target_wallet) { - return Err(AppError::forbidden("session does not own the target session")); - } + let target_wallet = target_wallet.ok_or_else(|| AppError::not_found("target session not found"))?; - let rows_affected = db - .execute("UPDATE sessions SET revoked = 1 WHERE token = ?1", params![target_token]) - .map_err(|e| AppError::internal(e.to_string()))?; + if !is_owner_of(&db, &session.wallet_address, &target_wallet) { + return Err(AppError::forbidden("session does not own the target session")); + } - if rows_affected == 0 { - return Err(AppError::not_found("target session not found")); - } + let rows_affected = db + .execute("UPDATE sessions SET revoked = 1 WHERE token = ?1", params![target_token]) + .map_err(|e| AppError::internal(e.to_string()))?; + + if rows_affected == 0 { + return Err(AppError::not_found("target session not found")); + } - Ok(Json(json!({ "ok": true }))) + Ok(Json(json!({ "ok": true }))) + } else { + let target_wallet_str = body["target_wallet"].as_str().unwrap(); + + if !is_owner_of(&db, &session.wallet_address, target_wallet_str) { + return Err(AppError::forbidden("session does not own the target wallet")); + } + + let rows_affected = db + .execute( + "UPDATE sessions SET revoked = 1 WHERE wallet_address = ?1 AND revoked = 0", + params![target_wallet_str], + ) + .map_err(|e| AppError::internal(e.to_string()))?; + + if rows_affected == 0 { + return Err(AppError::not_found("no active sessions found for target wallet")); + } + + Ok(Json(json!({ "ok": true, "sessions_revoked": rows_affected }))) + } } diff --git a/crates/agentkeys-mock-server/src/test_client.rs b/crates/agentkeys-mock-server/src/test_client.rs index 3bf71a1..6d601a7 100644 --- a/crates/agentkeys-mock-server/src/test_client.rs +++ b/crates/agentkeys-mock-server/src/test_client.rs @@ -255,6 +255,20 @@ impl CredentialBackend for InProcessBackend { Ok(()) } + async fn revoke_by_wallet( + &self, + session: &Session, + target_wallet: &WalletAddress, + ) -> Result<(), BackendError> { + self.post_with_session( + "/session/revoke", + session, + json!({ "target_wallet": target_wallet.0 }), + ) + .await?; + Ok(()) + } + async fn teardown_agent( &self, session: &Session, diff --git a/crates/agentkeys-mock-server/tests/integration.rs b/crates/agentkeys-mock-server/tests/integration.rs index 03007a9..6cebd25 100644 --- a/crates/agentkeys-mock-server/tests/integration.rs +++ b/crates/agentkeys-mock-server/tests/integration.rs @@ -1264,3 +1264,158 @@ async fn scope_change() { assert_eq!(approve_status, StatusCode::OK, "{approve_json}"); assert!(approve_json["signature"].is_string()); } + +// --------------------------------------------------------------------------- +// Revoke handler tests (issue-17) +// --------------------------------------------------------------------------- + +// Helper: create a session for a given auth token, return (session_token, wallet) +async fn create_session_for(app: Router, auth_token: &str) -> (String, String, Router) { + let (status, json) = post_json( + app.clone(), + "/session/create", + json!({ "auth_token": auth_token }), + ) + .await; + assert_eq!(status, StatusCode::OK, "create_session_for failed: {json}"); + let session = json["session"].as_str().unwrap().to_string(); + let wallet = json["wallet"].as_str().unwrap().to_string(); + (session, wallet, app) +} + +// Helper: create a child session under a parent, return (child_token, child_wallet) +async fn create_child_session_for(app: Router, parent_token: &str) -> (String, String, Router) { + let scope = json!({ "services": [], "read_only": false }); + let (status, json) = post_json_auth( + app.clone(), + "/session/child", + parent_token, + json!({ "scope": scope }), + ) + .await; + assert_eq!(status, StatusCode::OK, "create_child_session failed: {json}"); + let child_token = json["session"].as_str().unwrap().to_string(); + let child_wallet = json["wallet"].as_str().unwrap().to_string(); + (child_token, child_wallet, app) +} + +#[tokio::test] +async fn revoke_by_target_session_still_works() { + let app = setup(); + let (session, wallet, app) = create_session_for(app, "revoke-session-test").await; + + let (status, json) = post_json_auth( + app, + "/session/revoke", + &session, + json!({ "target_session": session }), + ) + .await; + assert_eq!(status, StatusCode::OK, "revoke by target_session failed: {json}"); + assert_eq!(json["ok"].as_bool(), Some(true)); + let _ = wallet; +} + +#[tokio::test] +async fn revoke_by_target_wallet_revokes_all() { + let app = setup(); + // Create parent (owner) session + let (owner_session, _owner_wallet, app) = create_session_for(app, "owner-token-revoke-all").await; + // Create two child sessions under owner — both will have the same child wallet for simplicity + // (each child call yields a fresh wallet, so create them and collect wallets) + let (child_token1, child_wallet1, app) = create_child_session_for(app, &owner_session).await; + // Create a second child session for the same wallet by creating another child + // (backend creates fresh wallets per child session, so we target child_wallet1 specifically) + // To have 2 sessions on the same wallet we create one child then a second session for that wallet + // via recover (mock allows any passkey). Use direct /session/create with child_wallet1's auth_token + // which was set to "child:" by the server. + let child_auth_token = format!("child:{}", child_token1); + let (child_token2, _child_wallet2, app) = create_session_for(app, &child_auth_token).await; + let _ = child_token2; + + // Now revoke all sessions for child_wallet1 + let (status, json) = post_json_auth( + app, + "/session/revoke", + &owner_session, + json!({ "target_wallet": child_wallet1 }), + ) + .await; + assert_eq!(status, StatusCode::OK, "revoke_by_wallet failed: {json}"); + assert_eq!(json["ok"].as_bool(), Some(true)); + let revoked = json["sessions_revoked"].as_u64().unwrap_or(0); + assert!(revoked >= 1, "expected at least 1 session revoked, got {revoked}"); +} + +#[tokio::test] +async fn revoke_by_target_wallet_not_owned() { + let app = setup(); + let (caller_session, _caller_wallet, app) = create_session_for(app, "caller-token-403").await; + let (_other_session, other_wallet, app) = create_session_for(app, "other-token-403").await; + + let (status, _json) = post_json_auth( + app, + "/session/revoke", + &caller_session, + json!({ "target_wallet": other_wallet }), + ) + .await; + assert_eq!(status, StatusCode::FORBIDDEN, "expected 403 for unowned wallet"); +} + +#[tokio::test] +async fn revoke_with_both_fields_is_400() { + let app = setup(); + let (session, wallet, app) = create_session_for(app, "both-fields-token").await; + + let (status, _json) = post_json_auth( + app, + "/session/revoke", + &session, + json!({ "target_session": session, "target_wallet": wallet }), + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST, "expected 400 when both fields present"); +} + +#[tokio::test] +async fn revoke_with_neither_field_is_400() { + let app = setup(); + let (session, _wallet, app) = create_session_for(app, "neither-fields-token").await; + + let (status, _json) = post_json_auth( + app, + "/session/revoke", + &session, + json!({}), + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST, "expected 400 when no fields present"); +} + +#[tokio::test] +async fn revoke_by_target_wallet_none_active_is_404() { + let app = setup(); + let (owner_session, _owner_wallet, app) = create_session_for(app, "owner-token-404").await; + let (_child_token, child_wallet, app) = create_child_session_for(app, &owner_session).await; + + // First revoke — should succeed + let (status1, _) = post_json_auth( + app.clone(), + "/session/revoke", + &owner_session, + json!({ "target_wallet": child_wallet }), + ) + .await; + assert_eq!(status1, StatusCode::OK, "first revoke should succeed"); + + // Second revoke — all sessions already revoked, expect 404 + let (status2, _) = post_json_auth( + app, + "/session/revoke", + &owner_session, + json!({ "target_wallet": child_wallet }), + ) + .await; + assert_eq!(status2, StatusCode::NOT_FOUND, "expected 404 when no active sessions remain"); +} diff --git a/docs/contradictions.md b/docs/contradictions.md index 8cf7077..631352f 100644 --- a/docs/contradictions.md +++ b/docs/contradictions.md @@ -225,14 +225,14 @@ Both are true at different horizons. But "instantly" in `key-security.md:221` re ## 4. CLI UX — manual test doc vs current code vs issues -### 4.1 `agentkeys revoke` broken (MAJOR — issue #17) +### 4.1 `agentkeys revoke` broken (RESOLVED 2026-04-14 — fix/issue-17) - `crates/agentkeys-cli/src/lib.rs` (`cmd_revoke`): passes wallet address as session token → backend's `WHERE token = ?1` finds nothing → always "target session not found." - `docs/manual-test-stage4.md:707-710, 724`: Test 9 marks revoke SKIPPED/BROKEN, but pass criteria line 724 still lists "Revoke succeeds." - `wiki/credential-usage.md:127`: shows `agentkeys revoke 0xAGENT` as the canonical syntax, no caveat it's broken. - Issue #17 proposes: self-revoke (no args) + revoke by wallet/alias + distinguish revoke (invalidate session, creds survive) vs teardown (delete creds + revoke sessions). -**Resolution.** Land #17. Update `wiki/credential-usage.md:127` to document revoke vs teardown table from issue #17. Update `docs/manual-test-stage4.md` Test 9 pass criteria to not include the SKIPPED step. 3 files. +**Resolution.** Landed in `fix/issue-17` (PR pending merge). `cmd_revoke` takes `Option<&str>` — no-args self-revokes + wipes local session; wallet form calls new `CredentialBackend::revoke_by_wallet` trait method; backend `/session/revoke` handler now accepts either `target_session` (token) or `target_wallet` (wallet). `wiki/credential-usage.md` now carries the revoke-vs-teardown table. `docs/manual-test-stage4.md` Test 9 passes on the fix branch. See `docs/manual-test-issue-17.md` for the full reproduction + verification walkthrough. ### 4.2 `agentkeys run` broken for master sessions (MAJOR — issue #15) diff --git a/docs/manual-test-issue-17.md b/docs/manual-test-issue-17.md new file mode 100644 index 0000000..3000dee --- /dev/null +++ b/docs/manual-test-issue-17.md @@ -0,0 +1,155 @@ +# Manual Test: Issue #17 — revoke command + +**Issue:** [litentry/agentKeys#17](https://github.com/litentry/agentKeys/issues/17) — Fix revoke command: broken lookup + clarify revoke vs teardown semantics. + +**Branch:** `fix/issue-17` + +## What changed + +On `main` (`f59c3803`), `agentkeys revoke ` always returned `"target session not found"` — `cmd_revoke` passed the wallet address as the session token, so the backend's `WHERE token = ?1` lookup never matched. + +This PR introduces two forms: + +| Form | Purpose | +|---|---| +| `agentkeys revoke` (no args) | Self-revoke: invalidate the current session on the backend **and** wipe the local keychain/file entry. Run `agentkeys init` again to re-pair. | +| `agentkeys revoke ` | Revoke all active sessions for the given wallet (after backend-enforced ownership check). Credentials survive — use `agentkeys teardown` to delete them too. | + +Backend path: `POST /session/revoke` now accepts **either** `target_session` (token, existing) **or** `target_wallet` (new) — exactly one. A new trait method `CredentialBackend::revoke_by_wallet(session, target_wallet)` drives the wallet form. + +## Preconditions + +- Rust toolchain (`cargo --version` ≥ 1.80). +- `jq` installed. +- No mock server already running on `127.0.0.1:8090`. +- Clean shell: `unset AGENTKEYS_SESSION` (if set from other tests). +- Working directory: `~/Projects/agentkeys`. + +## Setup + +```bash +cd ~/Projects/agentkeys +export AGENTKEYS_SESSION_STORE=file # skip the OS keychain for deterministic testing +export HOME_SANDBOX=$(mktemp -d) +export HOME=$HOME_SANDBOX # sandboxes ~/.agentkeys/session.json +BACKEND=http://127.0.0.1:8090 + +cargo build --release -p agentkeys-cli -p agentkeys-mock-server +BIN=$(pwd)/target/release/agentkeys +``` + +## Reproduce the bug (on `main`) + +```bash +jj new main -m "repro issue #17" # or git switch main, if using git +cargo run --release -p agentkeys-mock-server & +MOCK_PID=$! +sleep 1 + +$BIN --backend $BACKEND init --mock-token repro-17 +WALLET=$(jq -r .wallet "$HOME/.agentkeys/session.json") +$BIN --backend $BACKEND revoke "$WALLET" +# Expected (BROKEN): error containing "target session not found" + +kill $MOCK_PID +jj abandon @ # back to fix/issue-17 branch +``` + +## Verify the fix (on `fix/issue-17`) + +### Case 1 — self-revoke + +```bash +rm -f $HOME/.agentkeys/session.json +cargo run --release -p agentkeys-mock-server & +MOCK_PID=$! +sleep 1 + +$BIN --backend $BACKEND init --mock-token self-revoke-test +# Expected: "Session initialized for wallet=0x..." (absolute path visible) + +test -f $HOME/.agentkeys/session.json && echo "session file exists" +# Expected: "session file exists" + +$BIN --backend $BACKEND revoke +# Expected output (single line): "Revoked current session for wallet=0x.... Local session wiped. Run `agentkeys init` to re-pair." + +test -f $HOME/.agentkeys/session.json || echo "session file wiped" +# Expected: "session file wiped" + +# Follow-up: any subsequent command must fail with the "no session" error +$BIN --backend $BACKEND read 0xdeadbeef openrouter 2>&1 | head -3 +# Expected: error containing "load session (run `agentkeys init` first)" + +kill $MOCK_PID +``` + +### Case 2 — revoke by wallet (child agent / multi-session) + +```bash +rm -f $HOME/.agentkeys/session.json +cargo run --release -p agentkeys-mock-server & +MOCK_PID=$! +sleep 1 + +$BIN --backend $BACKEND init --mock-token by-wallet-test +WALLET=$(jq -r .wallet "$HOME/.agentkeys/session.json") + +$BIN --backend $BACKEND revoke "$WALLET" +# Expected: "Revoked agent=0x..." + +# Post-revoke: reading against the old session must fail (session row flipped revoked=1) +$BIN --backend $BACKEND read "$WALLET" openrouter 2>&1 | head -3 +# Expected: error — session revoked / DENIED (exact text depends on backend error surface) + +kill $MOCK_PID +``` + +### Case 3 — error path (no session) + +```bash +rm -f $HOME/.agentkeys/session.json + +$BIN --backend $BACKEND revoke 2>&1 | head -3 +# Expected: error containing "load session (run `agentkeys init` first)" +``` + +### Case 4 — ownership check + +```bash +rm -f $HOME/.agentkeys/session.json +cargo run --release -p agentkeys-mock-server & +MOCK_PID=$! +sleep 1 + +# User A inits +$BIN --backend $BACKEND init --mock-token user-a +A_WALLET=$(jq -r .wallet "$HOME/.agentkeys/session.json") + +# User B inits (different mock token → different wallet; overwrites local session) +$BIN --backend $BACKEND init --mock-token user-b +B_WALLET=$(jq -r .wallet "$HOME/.agentkeys/session.json") + +# User B tries to revoke user A's wallet — backend rejects +$BIN --backend $BACKEND revoke "$A_WALLET" 2>&1 | head -3 +# Expected: permission-denied / 403 error (not "target session not found") + +kill $MOCK_PID +``` + +## Cleanup + +```bash +rm -rf "$HOME_SANDBOX" +unset HOME_SANDBOX AGENTKEYS_SESSION_STORE +``` + +## Cross-references + +- Bug source: `crates/agentkeys-cli/src/lib.rs` — `cmd_revoke` (before PR: line 203; after PR: same file, new Option<&str> signature) +- Backend extension: `crates/agentkeys-mock-server/src/handlers/session.rs` — `revoke_session` (now dual-input) +- Trait addition: `crates/agentkeys-core/src/backend.rs` — `revoke_by_wallet` +- Stage 4 manual test updated: `docs/manual-test-stage4.md` Test 9 (BROKEN/SKIPPED caveats removed) +- Canonical usage doc updated: `wiki/credential-usage.md` — revoke vs teardown table added +- Contradictions tracker updated: `docs/contradictions.md` §4.1 marked RESOLVED once PR merges +- Related (not in this PR): [#16](https://github.com/litentry/agentKeys/issues/16) (identity aliases for revoke target) diff --git a/docs/manual-test-stage4.md b/docs/manual-test-stage4.md index 492a1d7..c5aa700 100644 --- a/docs/manual-test-stage4.md +++ b/docs/manual-test-stage4.md @@ -704,14 +704,17 @@ cargo run -p agentkeys-cli -- --backend http://localhost:8090 read $WALLET openr cargo run -p agentkeys-cli -- --backend http://localhost:8090 usage $WALLET # Expected: table showing store + read events -# Revoke (BROKEN -- see litentry/agentKeys#17) -# cmd_revoke passes wallet address as session token, backend can't find it. -# cargo run -p agentkeys-cli -- --backend http://localhost:8090 revoke $WALLET -# Expected (after fix): "Revoked agent=..." +# Revoke the wallet's active sessions (fixed in #17) +cargo run -p agentkeys-cli -- --backend http://localhost:8090 revoke $WALLET +# Expected: "Revoked agent=0x..." -# Try to read after revoke (SKIPPED -- depends on revoke fix) -# cargo run -p agentkeys-cli -- --backend http://localhost:8090 read $WALLET openrouter -# Expected (after fix): error — session revoked / DENIED +# Read after revoke — session row is revoked=1, backend denies +cargo run -p agentkeys-cli -- --backend http://localhost:8090 read $WALLET openrouter +# Expected: error — session revoked / DENIED (exact text depends on backend error surface) + +# (Optional) Self-revoke form — no args; wipes local session and requires `init` to re-pair. +# cargo run -p agentkeys-cli -- --backend http://localhost:8090 revoke +# Expected: "Revoked current session for wallet=0x.... Local session wiped. Run `agentkeys init` to re-pair." ``` **Pass criteria:** @@ -721,8 +724,8 @@ cargo run -p agentkeys-cli -- --backend http://localhost:8090 usage $WALLET - Read returns the stored key - `run` injects the env var correctly (SKIPPED -- litentry/agentKeys#15) - Usage shows audit events -- Revoke succeeds (BROKEN -- litentry/agentKeys#17) -- Read after revoke fails with clear error (SKIPPED -- depends on #17) +- Revoke succeeds +- Read after revoke fails with clear error --- diff --git a/wiki/credential-usage.md b/wiki/credential-usage.md index 3ed0eea..a9d3cf9 100644 --- a/wiki/credential-usage.md +++ b/wiki/credential-usage.md @@ -123,13 +123,24 @@ agentkeys usage 0xAGENT # who read what, when # 4. Rotate agentkeys store 0xAGENT openrouter sk-or-v1-NEW # overwrite with new key -# 5. Revoke agent access -agentkeys revoke 0xAGENT # invalidate session +# 5. Revoke access +agentkeys revoke # self-revoke: invalidate current session + wipe local keychain +agentkeys revoke 0xAGENT # revoke all active sessions for the given wallet # 6. Tear down completely -agentkeys teardown 0xAGENT # delete all credentials + session +agentkeys teardown 0xAGENT # delete all credentials + revoke all sessions ``` +### Revoke vs teardown + +| Command | Session tokens | Wallet | Credentials | When to use | +|---|---|---|---|---| +| `agentkeys revoke` (no args) | Current session invalidated + local keychain wiped | Survives on backend | Survive (inaccessible without a new session) | You're done for the day / device handoff | +| `agentkeys revoke 0xAGENT` | All active sessions for that wallet invalidated (ownership checked) | Survives | Survive | Kick a compromised child agent off — credentials stay so you can re-pair | +| `agentkeys teardown 0xAGENT` | All sessions revoked | Survives (account still exists) | **Deleted** | Fully retire an agent — credentials gone | + +After `revoke`: re-running `init` (same mock token / OAuth) gives you a fresh session for the same wallet, and the old credentials are accessible again. After `teardown`: `init` gives a fresh session but starts with an empty credential set. + ## Security comparison: `read` vs `run` vs MCP | Path | Credential exposure | Use case | From 25f9009ded09a00a8fefb38fe3ab76b1597dd397 Mon Sep 17 00:00:00 2001 From: Hanwen Cheng Date: Tue, 14 Apr 2026 19:26:00 +0800 Subject: [PATCH 2/2] =?UTF-8?q?fix(cli):=20#17=20v2=20=E2=80=94=20codex=20?= =?UTF-8?q?P2-1=20=E2=80=94=20clear=20local=20session=20when=20revoking=20?= =?UTF-8?q?own=20wallet?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex P2 finding on PR #18: `agentkeys revoke ` revoked the session on the backend but left the dead token cached locally; every subsequent command loaded the stale revoked token and failed. cmd_revoke's `Some(target_wallet_str)` branch now case-insensitively compares target_wallet against session.wallet. On match: call session_store::clear_session() and return a 'was your own session — re-pair' message (matching the no-arg self-revoke form's UX). Tests added (19 total CLI tests, +2 from previous): - cmd_revoke_with_own_wallet_clears_local_session — verifies the new path wipes the local file and emits the 'was your own session' acknowledgement. - cmd_revoke_with_other_wallet_keeps_local_session — counterpart: revoking someone else's wallet must NOT touch the caller's session. P2-2 (parallel-test HOME race) is tracked separately as issue #34 — needs serial_test crate added cross-PR (#18, #19, #20, #27 all affected). Co-Authored-By: Claude Opus 4.6 (1M context) --- crates/agentkeys-cli/src/lib.rs | 22 +++++- crates/agentkeys-cli/tests/cli_tests.rs | 92 +++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 1 deletion(-) diff --git a/crates/agentkeys-cli/src/lib.rs b/crates/agentkeys-cli/src/lib.rs index 867c392..dcee3fb 100644 --- a/crates/agentkeys-cli/src/lib.rs +++ b/crates/agentkeys-cli/src/lib.rs @@ -229,7 +229,27 @@ pub async fn cmd_revoke(ctx: &CommandContext, agent: Option<&str>) -> Result`, the local session file should +// be wiped (same as the no-arg self-revoke form), so subsequent commands +// 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 backend = create_test_backend(); + let ctx_init = CommandContext::new("unused", false, false) + .with_backend(backend.clone() as Arc); + let (_, session) = cmd_init(&ctx_init, Some("self-by-wallet-token".to_string())) + .await + .unwrap(); + + let session_path = session_store::fallback_path(); + 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); + + let result = cmd_revoke(&context, Some(&own_wallet)).await; + assert!(result.is_ok(), "self-by-wallet revoke failed: {:?}", result.err()); + let msg = result.unwrap(); + assert!( + msg.contains("was your own session"), + "expected self-revoke acknowledgement, got: {msg}" + ); + assert!( + msg.contains("agentkeys init"), + "expected re-pair hint, got: {msg}" + ); + + assert!( + !session_path.exists(), + "session file should be deleted after self-by-wallet revoke" + ); +} + +// Test: cmd_revoke_with_other_wallet_keeps_local_session +// +// Counterpart to the above: revoking SOMEONE ELSE's wallet must NOT touch +// 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 backend = create_test_backend(); + let ctx_init = CommandContext::new("unused", false, false) + .with_backend(backend.clone() as Arc); + let (_, parent_session) = cmd_init(&ctx_init, Some("revoke-other-token".to_string())) + .await + .unwrap(); + + // Spin up a child agent so we have an "other" wallet to target. + let child_scope = agentkeys_types::Scope { services: vec![], read_only: false }; + let (_child_session, child_wallet) = backend + .create_child_session(&parent_session, child_scope) + .await + .unwrap(); + + let session_path = session_store::fallback_path(); + 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); + + let result = cmd_revoke(&context, Some(child_wallet.0.as_str())).await; + assert!(result.is_ok(), "revoke other wallet failed: {:?}", result.err()); + let msg = result.unwrap(); + assert!(!msg.contains("was your own session"), "should NOT mark as self-revoke: {msg}"); + + assert!( + session_path.exists(), + "parent session file should NOT be deleted when revoking a different wallet" + ); +} + // Test: cmd_revoke_no_session_errors_cleanly #[tokio::test(flavor = "multi_thread")] async fn cmd_revoke_no_session_errors_cleanly() {