From 071d23a4b77a42a885a884776bd4cf08b2eba1bd Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Mon, 4 May 2026 13:55:07 -0500 Subject: [PATCH] fix(auth): dual-write API key to keyring + config so stale keyring stops shadowing onboarding (#593) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reproduction (from the user who filed #593, also the reporter of #586): 1. At any prior point, the user runs `deepseek auth set --provider deepseek`, which writes to the OS keyring under the `deepseek` slot. 2. The key is later rotated, the prior install is replaced, or the user moves to a different account. 3. The user opens the TUI, gets the in-TUI onboarding screen, and pastes their fresh API key. 4. `submit_api_key` → `save_api_key` writes only to `~/.deepseek/config.toml`. 5. At request time, `Secrets::resolve` follows the documented `keyring → env → config-file` precedence, and the **stale** keyring entry shadows the fresh config.toml value. 6. API call goes out with the dead key, gets a 401, the TUI shows "no response" with no obvious diagnostic. The fix ======= `save_api_key` now writes to **both** layers when a keyring backend is reachable: * The config file remains the durable, inspectable record of the active key (works in npm installs, IDE terminals, headless CI — everywhere). v0.8.8 made this the canonical location for a reason. * The OS keyring entry is rewritten on every onboarding submit so a stale credential from a prior install is overwritten in place. `SavedCredential` gains a new `KeyringAndConfigFile { backend, path }` variant; the existing `ConfigFile(PathBuf)` variant remains the fallback when no keyring backend is reachable (or under `cfg(test)`, so the unit suite never pollutes the host keyring). The onboarding toast naturally reports the actual outcome via `SavedCredential::describe`, which now reads `OS keyring (system keyring) and ~/.deepseek/config.toml` for the common case. `save_api_key_for` (the multi-provider entry point) is updated to extract the path from either variant, so non-DeepSeek providers (OpenRouter / Novita / Fireworks / NIM / SGLang) continue writing provider-table entries to config.toml only, with no behavior change. `deepseek doctor` warning ========================= `run_doctor` now compares the keyring's `deepseek` slot against the config file's `api_key` slot. When both are present and differ, the report surfaces the discrepancy with copy-paste remediation — `deepseek auth set --provider deepseek` rewrites both layers in one shot, and the in-TUI onboarding now does the same. The check skips keyring probes for other providers because they don't write to the keyring today; probing absent slots only triggers macOS Always-Allow prompts for nothing. Why dual-write rather than keyring-only ======================================= A previous attempt (`4e360274`, never merged to main) swapped the write path to keyring-only. That hides the key from anyone who expected to see it under `~/.deepseek/config.toml` and breaks the "deepseek-tui works in every folder, in npm installs, in IDE terminals" promise of v0.8.8. Dual-write keeps the inspectable copy and adds the layered override that defeats stale-shadow without changing the visible mental model. Tests ===== * `saved_credential_describe_lists_both_targets_for_keyring_and_config` pins the toast text shape so the user sees both targets after onboarding. * The existing `save_api_key_writes_config_file_under_cfg_test` and `test_save_api_key_doesnt_match_similar_keys` continue to pass — under `cfg(test)` the keyring path is gated out, so the config-only outcome remains the test-time contract. Verification ============ * `cargo fmt --all -- --check` clean. * `cargo clippy -p deepseek-tui --bin deepseek-tui --all-features --locked -- -D warnings` clean. * `cargo test -p deepseek-tui --bin deepseek-tui --locked` → 2029 passed, 2 ignored. Closes #593. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/tui/src/config.rs | 100 ++++++++++++++++++++++++++++++++++++--- crates/tui/src/main.rs | 39 +++++++++++++++ 2 files changed, 133 insertions(+), 6 deletions(-) diff --git a/crates/tui/src/config.rs b/crates/tui/src/config.rs index a57d27d9..9e52de2f 100644 --- a/crates/tui/src/config.rs +++ b/crates/tui/src/config.rs @@ -2162,7 +2162,24 @@ pub fn ensure_parent_dir(path: &Path) -> Result<()> { /// the caller can show a confirmation message without leaking the key. #[derive(Debug, Clone, PartialEq, Eq)] pub enum SavedCredential { - /// Stored in the deepseek config file at the given path. + /// Stored in **both** the OS keyring and the deepseek config file. + /// This is the default outcome on platforms with a working keyring + /// backend: writing both layers defeats the + /// `keyring → env → config-file` resolution-order shadow that + /// would otherwise let a stale OS-keyring entry from a previous + /// install hide the freshly-entered key (#593). The `backend` + /// label is the value of [`deepseek_secrets::Secrets::backend_name`] + /// at write time so the toast text can name the actual backend + /// (`"system keyring"`, `"file-based (~/.deepseek/secrets/)"`). + KeyringAndConfigFile { + /// `Secrets::backend_name()` at write time. + backend: String, + /// Absolute path to the config file that was also updated. + path: PathBuf, + }, + /// Stored in the deepseek config file only. Fallback when no + /// keyring backend is reachable, or under `cfg(test)` so unit + /// tests don't pollute the host keyring. ConfigFile(PathBuf), } @@ -2172,23 +2189,76 @@ impl SavedCredential { #[must_use] pub fn describe(&self) -> String { match self { + Self::KeyringAndConfigFile { backend, path } => { + format!("OS keyring ({backend}) and {}", path.display()) + } Self::ConfigFile(path) => path.display().to_string(), } } } -/// Save the active provider's API key to `~/.deepseek/config.toml`. +/// Save the active provider's API key. /// -/// v0.8.8 intentionally uses the shared config file as the default -/// setup path. It works in every folder, in npm installs, in IDE -/// terminals, and without platform credential prompts. +/// **Dual-write strategy (#593):** writes to `~/.deepseek/config.toml` +/// (always) and to the OS keyring via [`deepseek_secrets::Secrets`] +/// (when a backend is reachable). The runtime resolves credentials in +/// `keyring → env → config-file` order; writing to the config file +/// alone — as v0.8.8 through v0.8.10 did — let a stale keyring entry +/// from a prior install silently shadow the fresh value the user just +/// typed during in-TUI onboarding, producing the "no response" symptom +/// reported in #593. +/// +/// The config file remains the inspectable durable record (works in +/// npm installs, IDE terminals, and headless boxes alike), and the +/// keyring acts as the layered override that defeats stale-shadow on +/// the resolution path. When the keyring write fails (no backend, OS +/// permission denied, etc.) the config-file write still stands and +/// the function reports a [`SavedCredential::ConfigFile`] outcome — +/// callers should not treat that as a failure. +/// +/// Skipped under `cfg(test)` so the suite never touches the host +/// keyring. The `secrets` crate has its own test coverage for +/// keyring set/get. pub fn save_api_key(api_key: &str) -> Result { let trimmed = api_key.trim(); if trimmed.is_empty() { anyhow::bail!("Refusing to save an empty API key."); } + // Always write the inspectable copy first. The config file is the + // durable record everyone — including macOS Keychain-prompted + // first-run, headless CI, and IDE terminals — can rely on. let path = save_api_key_to_config_file(trimmed)?; + + // Then mirror to the OS keyring when one is reachable. This + // overwrites any stale entry from a prior install so + // `Secrets::resolve` (keyring → env → config-file) no longer + // shadows the fresh key. Skipped under `cfg(test)` so unit tests + // can't pollute the host keyring (macOS Always-Allow prompts, + // cross-test contamination). + #[cfg(not(test))] + { + let secrets = deepseek_secrets::Secrets::auto_detect(); + match secrets.set("deepseek", trimmed) { + Ok(()) => { + let backend = secrets.backend_name().to_string(); + log_sensitive_event( + "credential.save", + json!({ + "backend": backend.clone(), + "config_path": path.display().to_string(), + "dual_write": true, + }), + ); + return Ok(SavedCredential::KeyringAndConfigFile { backend, path }); + } + Err(err) => { + tracing::warn!("OS keyring write failed; key saved to config.toml only: {err}"); + // Fall through to the ConfigFile-only outcome below. + } + } + } + Ok(SavedCredential::ConfigFile(path)) } @@ -2349,7 +2419,8 @@ pub fn has_api_key_for(config: &Config, provider: ApiProvider) -> bool { pub fn save_api_key_for(provider: ApiProvider, api_key: &str) -> Result { if matches!(provider, ApiProvider::Deepseek | ApiProvider::DeepseekCN) { return match save_api_key(api_key)? { - SavedCredential::ConfigFile(path) => Ok(path), + SavedCredential::KeyringAndConfigFile { path, .. } + | SavedCredential::ConfigFile(path) => Ok(path), }; } @@ -2721,6 +2792,23 @@ mod tests { assert_eq!(cf.describe(), "/tmp/x.toml"); } + /// #593: the dual-write outcome describes both targets so the + /// onboarding toast (`API key saved to {describe}`) tells the user + /// the key landed in *both* the keyring and the config file — + /// which is the whole point of the fix (defeats stale-keyring + /// shadow while keeping the config file inspectable). + #[test] + fn saved_credential_describe_lists_both_targets_for_keyring_and_config() { + let dual = SavedCredential::KeyringAndConfigFile { + backend: "system keyring".to_string(), + path: PathBuf::from("/tmp/x.toml"), + }; + assert_eq!( + dual.describe(), + "OS keyring (system keyring) and /tmp/x.toml" + ); + } + #[test] fn has_api_key_detects_in_memory_override_and_env_var() -> Result<()> { // Pins the v0.8.8 contract: `has_api_key` covers the prompt-free diff --git a/crates/tui/src/main.rs b/crates/tui/src/main.rs index 40b664df..a13a8392 100644 --- a/crates/tui/src/main.rs +++ b/crates/tui/src/main.rs @@ -1580,6 +1580,45 @@ async fn run_doctor(config: &Config, workspace: &Path, config_path_override: Opt } println!(" · credential sources: env, ~/.deepseek/config.toml"); + // #593: surface keyring/config disagreement explicitly. The runtime + // resolution order is `keyring → env → config-file`, so a stale + // keyring entry from a prior install can shadow the value the user + // sees in `~/.deepseek/config.toml`. We only check the DeepSeek + // slot — other providers don't write to the keyring today, and + // probing entries that aren't there triggers macOS keychain + // prompts for nothing. + let secrets = deepseek_secrets::Secrets::auto_detect(); + let keyring_key = secrets.get("deepseek").ok().flatten(); + let config_key = config + .api_key + .as_ref() + .filter(|v| !v.trim().is_empty() && v.as_str() != "__KEYRING__") + .map(|s| s.to_string()); + match (keyring_key.as_deref(), config_key.as_deref()) { + (Some(k), Some(c)) if k.trim() != c.trim() => { + println!(); + println!( + " {} `deepseek`: OS keyring and config.toml hold different values.", + "⚠".truecolor(red_r, red_g, red_b) + ); + println!( + " Resolution order is keyring → env → config-file, so the keyring value wins." + ); + println!(" Reconcile by overwriting both with the current key:"); + println!(" deepseek auth set --provider deepseek"); + println!( + " (Or paste the key into the in-TUI onboarding screen — it now writes both layers.)" + ); + } + (Some(_), None) => { + println!( + " {} `deepseek`: key is in OS keyring only (config.toml has no copy).", + "·".dimmed() + ); + } + _ => {} + } + let has_api_key = if config.deepseek_api_key().is_ok() { println!( " {} active provider key resolved",