From 9b062ee5a4940ef895f763cc1f0726e8fbf845f9 Mon Sep 17 00:00:00 2001 From: "blu@blu.cx" <38222767+thebrandonlucas@users.noreply.github.com> Date: Mon, 26 Oct 2026 12:12:20 -0500 Subject: [PATCH] fix(config): read each saved secret once per command The macOS Keychain can ask for approval on every read, and one command read a saved login once for each credential lookup. Settings now keeps a secret in memory after the first read or write. Taking the credential lock forgets it, so a refresh still sees a token another process saved. Refs #116, DEV-2952 --- CHANGELOG.md | 1 + docs/reference/configuration.md | 2 + src/api.rs | 10 +--- src/config.rs | 96 ++++++++++++++++++++++++++++++--- tests/cli_integration.rs | 27 +++++----- 5 files changed, 108 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 39995ce..a64a216 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ User-visible changes to `voltage`, newest first. Each pull request that changes - A payment that settles on a terminal closes with `⚡ Payment ID completed` instead of a check mark, printed after the payment's record so it stays on screen. - `voltage payments list --watch` follows payments until Ctrl-C: a live table that marks new and changed payments on a terminal, or one `snapshot`, `new`, or `updated` event per payment when piped or with `--json`. `--interval` sets the refresh (default 5 seconds). - `voltage` with no command, on a terminal, greets you with the wordmark, who you are signed in as, your saved scope, and commands to try; off a terminal it is still a usage error. An empty wallet, payment, or webhook list, and a new profile, end with a `›` line naming the next command. +- A command reads a saved login's or API key's secret from the OS credential store once, instead of once for each lookup, so macOS asks for keychain access at most once per command. A login refresh still reads it again under the credential lock, to see a refresh that another command saved. ## 0.2.0 - 2026-10-22 diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index a2cdfea..c0655dc 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -16,6 +16,8 @@ A credential's secret is kept in one store, chosen when you log in or import a k - `keychain` (default): the macOS Keychain, the Linux Secret Service, or the Windows Credential Manager, under the service name `voltage-cli`. - `file`: an owner-only file under `credentials/`, for machines without an unlocked keychain, such as headless Linux. +A command reads a secret from its store once and keeps it in memory until it exits, because the macOS Keychain can ask for approval on every read. A login refresh reads it again under the credential lock. To stop the prompts, select **Always Allow** when macOS asks, or log in with `--credential-store file`. + ## Profiles A profile saves an organization, an environment, and a credential under one name. `--profile NAME` uses it as a complete configuration; see [log in and use profiles](../guides/login-and-profiles.md). diff --git a/src/api.rs b/src/api.rs index 8e13c73..0f553f1 100644 --- a/src/api.rs +++ b/src/api.rs @@ -1893,10 +1893,7 @@ mod tests { ) ); } - use crate::{ - cli::{Command, Invocation}, - config::Config, - }; + use crate::cli::{Command, Invocation}; use reqwest::header::{HeaderMap, HeaderValue}; use serde_json::json; @@ -2431,10 +2428,7 @@ mod tests { use std::os::unix::fs::PermissionsExt; std::fs::set_permissions(dir.path(), std::fs::Permissions::from_mode(0o700)).unwrap(); } - let settings = Settings { - dir: dir.path().to_path_buf(), - config: Config::default(), - }; + let settings = Settings::open(dir.path().to_path_buf()).unwrap(); let create_payment = operation(OperationId::CreatePayment); let id = Uuid::nil(); let body = json!({"id": id, "wallet_id": WALLET}); diff --git a/src/config.rs b/src/config.rs index 1887d2e..fee98cf 100644 --- a/src/config.rs +++ b/src/config.rs @@ -2,7 +2,8 @@ //! //! Every file the CLI writes here is owner-only and written atomically. Credentials default //! to the operating system store; file storage is an explicit choice with the same -//! ownership and permission checks on every read. +//! ownership and permission checks on every read. A command reads each secret from its store +//! once, because the macOS Keychain can ask for approval on every read. use crate::{ Error, Result, @@ -17,6 +18,7 @@ use std::{ fs::{self, File, OpenOptions, TryLockError}, io::{Read, Write}, path::{Path, PathBuf}, + sync::{Mutex, MutexGuard, PoisonError}, time::Duration, }; use uuid::Uuid; @@ -360,13 +362,27 @@ pub(crate) fn sourced_directory(explicit: Option) -> Result<(PathBuf, S pub struct Settings { pub dir: PathBuf, pub config: Config, + /// Secrets this process has read or written. Taking the lock forgets them, because another + /// process may have changed them since. + secrets: Mutex, } +/// Secrets by account name, with the store each is in. +type Secrets = BTreeMap)>; + impl Settings { /// Read `config.toml` from an absolute directory, or start empty when it does not exist. pub fn open(dir: PathBuf) -> Result { let config = read_config(&dir)?; - Ok(Self { dir, config }) + Ok(Self { + dir, + config, + secrets: Mutex::default(), + }) + } + + fn secrets(&self) -> MutexGuard<'_, Secrets> { + self.secrets.lock().unwrap_or_else(PoisonError::into_inner) } /// Where `config.toml` lives, whether or not it exists yet. @@ -549,8 +565,22 @@ impl Settings { Credential::try_from(stored) } - /// The secret saved for `name` in `store`, as it was written. + /// The secret saved for `name` in `store`, as it was written. A secret this process has + /// already read or written is not read again. fn read_secret(&self, name: &str, store: CredentialStore) -> Result> { + if let Some((held, secret)) = self.secrets().get(name) + && *held == store + { + return Ok(secret.clone()); + } + let secret = self.fetch_secret(name, store)?; + self.secrets() + .insert(name.to_owned(), (store, secret.clone())); + Ok(secret) + } + + /// The secret saved for `name` in `store`, read from the store itself. + fn fetch_secret(&self, name: &str, store: CredentialStore) -> Result> { match store { CredentialStore::File => read_private(&self.credential_path(name)), CredentialStore::Keychain => { @@ -570,6 +600,8 @@ impl Settings { credential: &Credential, ) -> Result<()> { let raw = serialized(credential)?; + // A failed write may still have replaced the secret, so the store is read again. + self.secrets().remove(name); match store { CredentialStore::File => { private_dir(&self.dir)?; @@ -584,7 +616,9 @@ impl Settings { "Cannot write OS credential store; explicitly select --credential-store file if needed", ) }), - } + }?; + self.secrets().insert(name.to_owned(), (store, raw)); + Ok(()) } pub fn delete_credential(&self, name: &str) -> Result<()> { @@ -594,6 +628,7 @@ impl Settings { /// Remove the secret saved for `name` in `store`, whatever store its account names. A /// secret that is already gone is not an error, so `logout --local` can remove its account. fn delete_secret(&self, name: &str, store: CredentialStore) -> Result<()> { + self.secrets().remove(name); match store { CredentialStore::File => { fs::remove_file(self.credential_path(name)).or_else(|error| match error.kind() { @@ -745,7 +780,7 @@ impl Settings { /// Whether `store` holds `credential` for `name`, as when a failed write never replaced it. fn holds_secret(&self, name: &str, store: CredentialStore, credential: &Credential) -> bool { - let held = self.read_secret(name, store).and_then(|raw| { + let held = self.fetch_secret(name, store).and_then(|raw| { let stored: StoredCredential = serde_json::from_str(&raw)?; serialized(&Credential::try_from(stored)?) }); @@ -762,7 +797,9 @@ impl Settings { read_config(&self.dir).is_ok_and(|disk| store(&disk) == store(&self.config)) } - /// Exclusive process lock for credential and configuration writes. + /// Exclusive process lock for credential and configuration writes. It forgets the secrets + /// read so far, so a secret read under the lock, such as a refresh token about to be + /// spent, is the one another process may have saved meanwhile. pub async fn lock(&self, terminal: Terminal) -> Result { private_dir(&self.dir)?; let path = self.dir.join(LOCK_FILE); @@ -783,7 +820,10 @@ impl Settings { let mut progress = None; loop { match file.try_lock() { - Ok(()) => return Ok(file), + Ok(()) => { + self.secrets().clear(); + return Ok(file); + } Err(TryLockError::WouldBlock) => { progress .get_or_insert_with(|| terminal.progress("Waiting for credential lock...")); @@ -1104,6 +1144,7 @@ mod tests { Settings { dir, config: Config::default(), + secrets: Mutex::default(), } } @@ -1353,6 +1394,47 @@ mod tests { dir } + /// A command reads a secret from its store once, because the macOS Keychain can ask for + /// approval on every read. Under the lock it reads the store again, so it sees a refresh + /// another process saved meanwhile. + #[tokio::test] + async fn a_secret_is_read_once_until_the_lock_is_taken() { + let dir = private_tempdir(); + let mut settings = settings(dir.path().to_path_buf()); + let terminal = Terminal::new(true, true); + settings + .add_account("me", user_account(), &login("old"), terminal) + .await + .unwrap(); + let other = Settings::open(dir.path().to_path_buf()).unwrap(); + other + .write_credential("me", CredentialStore::File, &login("new")) + .unwrap(); + assert_eq!(refresh_token(&settings, "me"), "old"); + let _lock = settings.lock(terminal).await.unwrap(); + assert_eq!(refresh_token(&settings, "me"), "new"); + } + + /// A deleted secret, or one whose write failed, is read from its store again. + #[tokio::test] + async fn a_deleted_or_failed_write_reads_the_store_again() { + use write_faults::Fault::AfterRename; + let dir = private_tempdir(); + let mut settings = settings(dir.path().to_path_buf()); + let terminal = Terminal::new(true, true); + settings + .add_account("me", user_account(), &login("old"), terminal) + .await + .unwrap(); + write_faults::set([Some(AfterRename)]); + let written = settings.write_credential("me", CredentialStore::File, &login("new")); + write_faults::set([]); + assert!(written.is_err()); + assert_eq!(refresh_token(&settings, "me"), "new"); + settings.delete_credential("me").unwrap(); + assert!(settings.read_credential("me").is_err()); + } + #[tokio::test] async fn a_saved_account_is_replaced_only_when_accepted() { let dir = private_tempdir(); diff --git a/tests/cli_integration.rs b/tests/cli_integration.rs index 53c22e4..413e087 100644 --- a/tests/cli_integration.rs +++ b/tests/cli_integration.rs @@ -21,9 +21,7 @@ use tempfile::TempDir; #[cfg(unix)] use tokio::sync::Notify; use voltage_cli::{ - config::{ - self, Account, AccountKind, Config, Credential, CredentialStore, Login, Profile, Settings, - }, + config::{self, Account, AccountKind, Credential, CredentialStore, Login, Profile, Settings}, registry::{AuthScheme, Method, OPERATIONS, OperationId, ResourceTarget}, secret::Secret, }; @@ -494,10 +492,7 @@ async fn result_output_treats_an_early_closing_pipe_as_success_without_resubmitt } fn settings_at(dir: &Path) -> Settings { - Settings { - dir: dir.into(), - config: Config::default(), - } + Settings::open(dir.into()).unwrap() } /// A saved browser login stored in a private file, expired or good for ten minutes. @@ -557,8 +552,14 @@ fn seeded_api_key(dir: &Path, name: &str, key: &str) -> Settings { } /// The saved login's access and refresh tokens. +/// The credential saved on disk now. A `Settings` reads each secret once, so a read through +/// one that read it before a command ran would not see what the command saved. +fn saved_credential(settings: &Settings, name: &str) -> Option { + settings_at(&settings.dir).read_credential(name).ok() +} + fn login_tokens(settings: &Settings, name: &str) -> (String, String) { - match settings.read_credential(name).unwrap() { + match saved_credential(settings, name).unwrap() { Credential::Login(login) => ( login.access_token.expose().to_owned(), login.refresh_token.expose().to_owned(), @@ -1317,7 +1318,7 @@ async fn imported_keys_bind_their_scope_and_report_status_without_secrets() { assert_eq!(account.kind, AccountKind::ApiKey); assert_eq!(account.store, CredentialStore::File); assert_eq!(account.organization_id, Some(uuid(ORG))); - let Credential::ApiKey(key) = settings.read_credential("staging-key").unwrap() else { + let Credential::ApiKey(key) = saved_credential(&settings, "staging-key").unwrap() else { panic!("expected an API key"); }; assert_eq!(key.expose(), "imported-secret"); @@ -1348,7 +1349,7 @@ async fn imported_keys_bind_their_scope_and_report_status_without_secrets() { .code(2); assert_eq!(json_error(&login)["error"]["message"], in_use); let settings = Settings::open(dir.path().to_path_buf()).unwrap(); - let Credential::ApiKey(key) = settings.read_credential("staging-key").unwrap() else { + let Credential::ApiKey(key) = saved_credential(&settings, "staging-key").unwrap() else { panic!("expected an API key"); }; assert_eq!(key.expose(), "imported-secret"); @@ -1720,12 +1721,12 @@ async fn logout_retains_credentials_on_revocation_failure_and_local_is_explicit( .args(["logout", "--account", LOGIN_NAME]) .assert() .code(3); - assert!(settings.read_credential(LOGIN_NAME).is_ok()); + assert!(saved_credential(&settings, LOGIN_NAME).is_some()); cli(dir.path(), &server) .args(["logout", "--account", LOGIN_NAME, "--local"]) .assert() .success(); - assert!(settings.read_credential(LOGIN_NAME).is_err()); + assert!(saved_credential(&settings, LOGIN_NAME).is_none()); } /// A device login that is approved at once and signs in as `user_id`. @@ -2367,7 +2368,7 @@ async fn interrupt_during_a_logout_revoke_points_to_a_local_logout() { .contains("--local") ); // The saved login is still there for the local logout the hint suggests. - assert!(settings.read_credential(LOGIN_NAME).is_ok()); + assert!(saved_credential(&settings, LOGIN_NAME).is_some()); server.verify().await; }