fix(auth): dual-write API key to keyring + config to stop stale-keyring shadow (#593)#594
Merged
Conversation
…ops shadowing onboarding (#593) 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) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a dual-write strategy for API keys, saving them to both the OS keyring and the configuration file to prevent stale keyring entries from shadowing new keys. It also updates the doctor command to detect and report discrepancies between these storage locations. Feedback was provided regarding redundant logging, as the current implementation results in duplicate credential.save events during a successful dual-write operation.
Comment on lines
+2245
to
+2252
| log_sensitive_event( | ||
| "credential.save", | ||
| json!({ | ||
| "backend": backend.clone(), | ||
| "config_path": path.display().to_string(), | ||
| "dual_write": true, | ||
| }), | ||
| ); |
Contributor
There was a problem hiding this comment.
This log_sensitive_event call is redundant. The save_api_key_to_config_file function, called on line 2231, already logs a credential.save event. With this change, a successful dual-write will result in two separate log events for a single user action, which can complicate analytics.
To ensure a single, comprehensive log event per operation, I recommend centralizing the logging within this save_api_key function. You could:
- Remove the
log_sensitive_eventcall fromsave_api_key_to_config_file. - Add a
log_sensitive_eventcall for the config-file-only scenario at the end of this function, before the finalOk(SavedCredential::ConfigFile(path))return.
This will ensure that whether the operation is a dual-write or a config-file-only write, exactly one event is logged with the correct details.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses stale-credential shadowing by updating the TUI onboarding API-key save flow to write credentials to multiple persistence layers and by adding a deepseek doctor diagnostic for disagreements between those layers.
Changes:
- Extend
SavedCredentialto represent a dual-write outcome (keyring + config file) and update its user-facing description. - Update
save_api_keyto write~/.deepseek/config.tomland then attempt to mirror the key into the secrets backend. - Enhance
run_doctorto detect and report a mismatch between the secrets backend value andconfig.tomlfor the DeepSeek slot.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| crates/tui/src/config.rs | Adds a dual-write SavedCredential variant and updates save_api_key to mirror credentials into the secrets backend after writing config.toml. |
| crates/tui/src/main.rs | Adds a doctor-time check for secrets-backend vs config-file API key mismatches and prints remediation guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2239
to
+2258
| #[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. | ||
| } |
Comment on lines
+2192
to
2195
| Self::KeyringAndConfigFile { backend, path } => { | ||
| format!("OS keyring ({backend}) and {}", path.display()) | ||
| } | ||
| Self::ConfigFile(path) => path.display().to_string(), |
Comment on lines
+1589
to
+1606
| // 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." | ||
| ); |
Comment on lines
+2233
to
+2260
| // 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. | ||
| } | ||
| } | ||
| } |
6 tasks
MMMarcinho
pushed a commit
to MMMarcinho/DeepSeek-TUI
that referenced
this pull request
May 6, 2026
fix(auth): dual-write API key to keyring + config to stop stale-keyring shadow (Hmbown#593)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #593. The TUI in-app onboarding writes the user's API key to
~/.deepseek/config.toml, butSecrets::resolvereadskeyring → env → config-file— so a stale keyring entry from a prior install silently shadows the freshly-typed key, and the user sees "no response" / 401 with no obvious diagnostic.This PR makes
save_api_keywrite to both the keyring (overwriting any stale entry) and~/.deepseek/config.toml(durable, inspectable). The onboarding toast now reports both targets via the newSavedCredential::KeyringAndConfigFilevariant.deepseek doctorlearns to spot the keyring/config disagreement when it occurs in the wild and prints copy-paste remediation.This also intentionally rejects the prior keyring-only approach (
4e360274, never merged) — keyring-only hides the key from anyone who expects to see it in~/.deepseek/config.tomland breaks v0.8.8's "works in every folder / npm install / IDE terminal" promise. Dual-write keeps the inspectable copy and adds the layered override that defeats the shadow.What changed
crates/tui/src/config.rsSavedCredentialgainsKeyringAndConfigFile { backend, path }.ConfigFile(PathBuf)is now the no-keyring fallback.save_api_keywrites config.toml first, then mirrors to the OS keyring viadeepseek_secrets::Secrets::auto_detect(). Keyring write failures are logged and downgrade the outcome toConfigFile-only — never a hard error, so headless / no-keyring environments keep working.cfg(test)so unit tests never touch the host keyring.save_api_key_forupdated to extract the path from either variant.crates/tui/src/main.rs::run_doctorSecrets::auto_detect().get("deepseek")and compares againstconfig.api_key. Reports the mismatch with a one-line remediation (deepseek auth set --provider deepseekrewrites both).Test plan
cargo fmt --all -- --checkclean.cargo clippy -p deepseek-tui --bin deepseek-tui --all-features --locked -- -D warningsclean.cargo test -p deepseek-tui --bin deepseek-tui --locked→ 2029 passed, 2 ignored.saved_credential_describe_lists_both_targets_for_keyring_and_configpins the onboarding toast text shape.security find-generic-password -s deepseek -a deepseekand~/.deepseek/config.toml, and that subsequent turns succeed even with a previously-saved keyring entry.save_api_keyfalls through toConfigFile-only without erroring.deepseek doctorshows the warning section when the two layers disagree, and is silent when they agree (or when the keyring slot is absent and only config has the key).🤖 Generated with Claude Code