From c046a411715ec7cce6153d7a099e1fa7500cd7bc Mon Sep 17 00:00:00 2001 From: Svetlin Ralchev Date: Tue, 6 Oct 2026 11:31:07 +0400 Subject: [PATCH] fix: delete every cached item of a profile, including zsh-op leftovers `keysafe profile clear` deleted only the secrets named in the config or the loaded metadata, so items zsh-op cached under op-secrets- for secrets no longer configured stayed in the keychain forever. Clear now also searches the keychain for every item under keysafe. and op-secrets- (attributes only, no prompts) and deletes them. `keysafe doctor` warns when zsh-op items remain for a configured profile and says how to remove them. --- README.md | 1 + src/app/exec.rs | 39 ++++++++++++++++++- src/vault/cache.rs | 96 +++++++++++++++++++++++++++++++++++++++++----- 3 files changed, 125 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index ff9dd60..df45eeb 100644 --- a/README.md +++ b/README.md @@ -218,6 +218,7 @@ keysafe picks up where the zsh-op plugin left off: - With no config at the new location, `~/.config/op/config.yml` is used, with a hint to move it. - Profiles recorded in `~/.cache/op` count as loaded until you load them again. - Secrets cached under `op-secrets-` move to `keysafe.` the first time they are read; the old items are deleted. +- Items zsh-op cached that keysafe never reads, such as secrets no longer in your config, stay behind. `keysafe doctor` warns about them and `keysafe profile clear ` deletes them. ## Troubleshooting diff --git a/src/app/exec.rs b/src/app/exec.rs index a08d439..064b763 100644 --- a/src/app/exec.rs +++ b/src/app/exec.rs @@ -1012,7 +1012,25 @@ impl DoctorCommand { // The keychain: looking up a missing item needs the store, but never prompts match self.cache.store.get("keysafe.doctor", "check") { - Ok(_) => self.report(Check::Ok, "Keychain", "reachable")?, + Ok(_) => { + self.report(Check::Ok, "Keychain", "reachable")?; + + // Secrets zsh-op cached under its own service names + for profile in profiles { + let Ok(items) = self.cache.legacy_items(&profile.name) else { + continue; + }; + if !items.is_empty() { + let detail = format!( + "{} left from zsh-op under {} (remove them with `keysafe profile clear {}`)", + count(items.len(), "item"), + Cache::legacy_service(&profile.name), + profile.name + ); + self.report(Check::Warning, "Keychain", detail)?; + } + } + } Err(err) => { failures += 1; self.report(Check::Failure, "Keychain", format!("{err:#}"))?; @@ -2572,6 +2590,25 @@ mod tests { assert!(output.ends_with("✓ Shell integration: active (bash)\n")); } + #[test] + fn doctor_warns_about_items_left_from_zsh_op() -> Result<()> { + let fixture = Fixture::new(); + fixture.store.set("op-secrets-work", "OLD_TOKEN", "x")?; + let mut client = MockSecretClient::new(); + client.expect_check().returning(|_| Ok("fine".into())); + let (mut cmd, writer) = doctor(&fixture, client, agent(vec![])); + + cmd.execute(&DoctorCommandArgs { + parent: fixture.parent(), + shell: Some(Shell::Zsh), + })?; + + assert!(writer.contents().contains( + "! Keychain: 1 item left from zsh-op under op-secrets-work (remove them with `keysafe profile clear work`)\n" + )); + Ok(()) + } + #[test] fn doctor_skips_the_agent_without_ssh_keys() -> Result<()> { let fixture = Fixture::new(); diff --git a/src/vault/cache.rs b/src/vault/cache.rs index 3c2d672..0e9bebf 100644 --- a/src/vault/cache.rs +++ b/src/vault/cache.rs @@ -1,5 +1,6 @@ use anyhow::{anyhow, Context, Result}; use std::{ + collections::HashMap, io::ErrorKind, path::{Path, PathBuf}, sync::OnceLock, @@ -16,6 +17,8 @@ pub trait SecretStore { fn set(&self, service: &str, account: &str, value: &str) -> Result<()>; /// Deletes the value stored for `service` / `account`. Returns false if there was none. fn delete(&self, service: &str, account: &str) -> Result; + /// Returns the accounts that have a value stored for `service`, without reading values. + fn accounts(&self, service: &str) -> Result>; } /// Keychain stores secrets in the platform credential store: the login keychain on macOS @@ -29,14 +32,19 @@ impl Keychain { Self } - /// Returns the keyring entry for `service` / `account`. - fn entry(service: &str, account: &str) -> Result { + /// Opens the platform credential store, once. + fn open() -> Result<()> { static STORE: OnceLock> = OnceLock::new(); STORE .get_or_init(|| open_store().map_err(|e| e.to_string())) .as_ref() - .map_err(|e| anyhow!("failed to open the credential store: {e}"))?; + .map(|_| ()) + .map_err(|e| anyhow!("failed to open the credential store: {e}")) + } + /// Returns the keyring entry for `service` / `account`. + fn entry(service: &str, account: &str) -> Result { + Self::open()?; Ok(keyring_core::Entry::new(service, account)?) } } @@ -59,6 +67,28 @@ fn open_store() -> keyring_core::Result<()> { } impl SecretStore for Keychain { + fn accounts(&self, service: &str) -> Result> { + Self::open()?; + // A search returns the items' attributes only, so it never prompts for access + let spec = HashMap::from([("service", service)]); + let entries = keyring_core::Entry::search(&spec) + .with_context(|| format!("failed to search the keychain for {service}"))?; + let mut accounts: Vec = entries + .iter() + .filter_map(|entry| match entry.get_specifiers() { + Some((_, account)) => Some(account), + None => entry.get_attributes().ok().and_then(|attributes| { + ["username", "user", "acct"] + .iter() + .find_map(|key| attributes.get(*key).cloned()) + }), + }) + .collect(); + accounts.sort(); + accounts.dedup(); + Ok(accounts) + } + fn get(&self, service: &str, account: &str) -> Result> { match Self::entry(service, account)?.get_password() { Ok(value) => Ok(Some(value)), @@ -131,6 +161,11 @@ impl Cache { format!("op-secrets-{profile}") } + /// Returns the secrets of `profile` still stored under the service zsh-op used. + pub fn legacy_items(&self, profile: &str) -> Result> { + self.store.accounts(&Self::legacy_service(profile)) + } + /// Returns the cached value of secret `name` in `profile`. pub fn get(&self, profile: &str, name: &str) -> Result> { let service = Self::service(profile); @@ -216,17 +251,23 @@ impl Cache { /// its metadata or stored under the legacy service, and forgets that it was loaded. /// Returns the number of deleted secrets. pub fn clear(&self, account: &Profile) -> Result { - let mut names: Vec = account.secrets.iter().map(|s| s.name.clone()).collect(); - for name in self.loaded(&account.name)?.unwrap_or_default() { - if !names.contains(&name) { - names.push(name); - } - } - let services = [ Self::service(&account.name), Self::legacy_service(&account.name), ]; + + // Configured and recorded secrets, plus anything else stored under either service, + // like secrets since removed from the config + let mut names: Vec = account.secrets.iter().map(|s| s.name.clone()).collect(); + names.extend(self.loaded(&account.name)?.unwrap_or_default()); + for service in &services { + match self.store.accounts(service) { + Ok(found) => names.extend(found), + Err(err) => debug(format!("{err:#}")), + } + } + names.sort(); + names.dedup(); let mut count = 0; for name in &names { let mut deleted = false; @@ -361,6 +402,16 @@ impl SecretStore for MemoryStore { let key = (service.to_string(), account.to_string()); Ok(self.0.borrow_mut().remove(&key).is_some()) } + + fn accounts(&self, service: &str) -> Result> { + Ok(self + .0 + .borrow() + .keys() + .filter(|(s, _)| s == service) + .map(|(_, account)| account.clone()) + .collect()) + } } #[cfg(test)] @@ -462,6 +513,31 @@ mod tests { ); } + #[test] + fn clear_deletes_items_the_config_no_longer_names() { + let dir = tempfile::tempdir().unwrap(); + let store = MemoryStore::with(&[ + ("op-secrets-personal", "REMOVED_LONG_AGO", "a"), + ("keysafe.personal", "ALSO_GONE", "b"), + ("op-secrets-work", "OTHER_PROFILE", "c"), + ]); + let cache = Cache::new(Box::new(store.clone()), dir.path()); + assert_eq!( + cache.legacy_items("personal").unwrap(), + ["REMOVED_LONG_AGO"] + ); + + assert_eq!(cache.clear(&account()).unwrap(), 2); + + assert_eq!(store.value("op-secrets-personal", "REMOVED_LONG_AGO"), None); + assert_eq!(store.value("keysafe.personal", "ALSO_GONE"), None); + assert_eq!( + store.value("op-secrets-work", "OTHER_PROFILE").as_deref(), + Some("c") + ); + assert!(cache.legacy_items("personal").unwrap().is_empty()); + } + #[test] fn service_uses_profile_prefix() { assert_eq!(Cache::service("work"), "keysafe.work");