dgf1988/codewhale
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix(config): /logout + new key now uses the new key (#343)

Browse Source 
After running /logout and entering a new API key, subsequent requests
could still be sent with the old key because the resolution path checked
the OS keyring before the in-memory override. The keyring still held
the old credential, so it shadowed the freshly-typed one.

Three changes:

1. **`Config::deepseek_api_key()` — explicit override is now path 0.**
   When `self.api_key` is explicitly set (non-empty, non-sentinel), it
   wins over keyring/env/provider-config. This is what the user just
   typed, so it should be authoritative. Existing keyring-based flows
   are unaffected: users who store their key via `auth set` have
   `self.api_key = None`, so path 1 (keyring) still wins for them.

2. **`clear_api_key()` now wipes the keyring + provider-scoped keys.**
   Previously only the legacy root `api_key = ...` line was stripped
   from config.toml. Now every known provider slot in the OS keyring
   (deepseek, nvidia-nim, openrouter, novita, fireworks, sglang) is
   deleted, and every `api_key` line nested in a `[providers.<name>]`
   table is also stripped.

3. **`/logout` clears the in-memory `Config` too.** The dispatcher
   handler in ui.rs::execute_command_input wipes `config.api_key` and
   every `config.providers.*.api_key` so a future clone of the
   long-lived Config doesn't leak the stale value. The companion
   onboarding flow in ui.rs also stamps the new key onto `config`
   itself rather than only on a one-shot clone, so subsequent
   /provider switches see the new credential.

Test coverage:
- `clear_api_key_strips_root_and_provider_scoped_keys` — verifies all
  three credential locations get wiped from a fixture config.toml.
- `deepseek_api_key_prefers_explicit_in_memory_override` — guards the
  precedence flip.
- `deepseek_api_key_ignores_sentinel_placeholder` — confirms the
  legacy `KEYRING_SENTINEL` placeholder still falls through.

Closes #343.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Hunter Bown
2026-05-02 10:08:52 -05:00
parent 899c703d81
commit c5627ebb14
2 changed files with 210 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/tui/src/config.rs
+186 -21
View File
@@ -1130,10 +1130,16 @@ impl Config {
/// Read the API key.
///
/// Precedence: **OS keyring → environment → config file**. The
/// keyring + env layers are collapsed by [`deepseek_secrets::Secrets::resolve`];
/// the config-file fallback is preserved here for users who haven't
/// run `deepseek auth migrate` yet.
/// Precedence: **explicit in-memory override → OS keyring → environment
/// → provider-scoped config → legacy root config**. The keyring + env
/// layers are collapsed by [`deepseek_secrets::Secrets::resolve`].
///
/// The in-memory `self.api_key` override takes priority over the
/// keyring so a fresh key entered via `/logout` + onboarding actually
/// takes effect even when an old key is still cached in the OS keyring
/// (#343). The override is only honored when the user *explicitly* set
/// the field (not the legacy `API_KEYRING_SENTINEL` placeholder, not
/// empty whitespace).
pub fn deepseek_api_key(&self) -> Result<String> {
let provider = self.api_provider();
let slot = match provider {
@@ -1145,6 +1151,17 @@ impl Config {
ApiProvider::Sglang => "sglang",
};
// 0. Explicit in-memory override (set by onboarding / provider
// picker). Wins over keyring + env so a freshly-entered key
// takes effect immediately even if a stale credential lingers
// in the OS keyring (#343).
if let Some(configured) = self.api_key.as_ref()
&& !configured.trim().is_empty()
&& configured != API_KEYRING_SENTINEL
{
return Ok(configured.clone());
}
// 1. OS keyring + 2. environment variables (handled by Secrets).
let secrets = deepseek_secrets::Secrets::auto_detect();
if let Some(value) = secrets.resolve(slot)
@@ -1166,17 +1183,6 @@ impl Config {
return Ok(configured);
}
// 4. legacy root `api_key` (deepseek only).
if let Some(configured) = self.api_key.clone()
&& !configured.trim().is_empty()
&& configured != API_KEYRING_SENTINEL
{
tracing::warn!(
"api_key in config.toml is deprecated; run 'deepseek auth migrate' to move it to the OS keyring"
);
return Ok(configured);
}
match provider {
ApiProvider::Deepseek | ApiProvider::DeepseekCN => anyhow::bail!(
"DeepSeek API key not found. Set it using one of these methods:\n\
@@ -2253,11 +2259,49 @@ pub fn save_api_key_for(provider: ApiProvider, api_key: &str) -> Result<PathBuf>
Ok(config_path)
}
/// Clear the API key from the config file
/// Clear the API key from every storage layer the resolver consults.
///
/// `/logout` calls this to wipe credentials so the next request can't
/// silently use a stale key (#343). The function clears:
///
/// 1. **OS keyring** — every known provider slot (`deepseek`, `nvidia-nim`,
/// `openrouter`, `novita`, `fireworks`, `sglang`). A leftover keyring
/// entry would otherwise be returned by [`Config::deepseek_api_key`]
/// even after the user enters a new key.
/// 2. **Config file** — strips the legacy root `api_key = ...` line *and*
/// every `api_key` line nested in a `[providers.<name>]` table.
///
/// Environment variables (`DEEPSEEK_API_KEY`, etc.) are intentionally
/// **not** unset — they are managed by the user's shell and outside the
/// CLI's purview. `Config::deepseek_api_key`'s explicit-override path
/// (Path 0) ensures a freshly-entered key still wins over a stale env
/// var that lingers from a previous session.
pub fn clear_api_key() -> Result<()> {
// Don't clear keychain - we're not using it anymore
// Just clear from config file
// 1. Clear every known provider slot from the OS keyring (or
// file-backed fallback). Errors are warned-and-continued because
// a missing entry is a no-op and a transient keyring failure
// shouldn't block the rest of the wipe.
let secrets = deepseek_secrets::Secrets::auto_detect();
let backend = secrets.backend_name();
for slot in &[
"deepseek",
"nvidia-nim",
"openrouter",
"novita",
"fireworks",
"sglang",
] {
if let Err(err) = secrets.delete(slot) {
tracing::warn!("Failed to clear keyring slot '{slot}': {err}");
}
}
log_sensitive_event(
"credential.clear",
json!({ "backend": backend, "scope": "keyring_all_slots" }),
);
// 2. Strip api_key lines from config.toml, including provider-scoped
// nested entries.
let config_path = default_config_path()
.context("Failed to resolve config path: home directory not found.")?;
@@ -2269,10 +2313,17 @@ pub fn clear_api_key() -> Result<()> {
let mut result = String::new();
for line in existing.lines() {
if !line.trim_start().starts_with("api_key") {
result.push_str(line);
result.push('\n');
// Match `api_key`, `api_key =`, ` api_key=`, etc. — anywhere it
// appears as the leading non-whitespace token.
let trimmed = line.trim_start();
if trimmed.strip_prefix("api_key").is_some_and(|rest| {
let rest = rest.trim_start();
rest.is_empty() || rest.starts_with('=')
}) {
continue;
}
result.push_str(line);
result.push('\n');
}
fs::write(&config_path, result)
@@ -2282,6 +2333,7 @@ pub fn clear_api_key() -> Result<()> {
json!({
"backend": "config_file",
"config_path": config_path.display().to_string(),
"scope": "root_and_provider_keys",
}),
);
@@ -2477,6 +2529,119 @@ mod tests {
Ok(())
}
/// Regression for #343: clear_api_key strips both the root `api_key`
/// and any nested `[providers.<name>].api_key` lines from config.toml
/// so a stale credential can't shadow a fresh login.
#[test]
fn clear_api_key_strips_root_and_provider_scoped_keys() -> Result<()> {
let _lock = lock_test_env();
let nanos = SystemTime::now()
.duration_since(UNIX_EPOCH)
.unwrap()
.as_nanos();
let temp_root = env::temp_dir().join(format!(
"deepseek-tui-clear-{}-{}",
std::process::id(),
nanos
));
fs::create_dir_all(&temp_root)?;
let _guard = EnvGuard::new(&temp_root);
let config_dir = temp_root.join(".deepseek");
fs::create_dir_all(&config_dir)?;
let config_path = config_dir.join("config.toml");
fs::write(
&config_path,
r#"api_key = "old-root-key"
default_text_model = "deepseek-v4-flash"
[providers.deepseek]
api_key = "old-provider-key"
base_url = "https://api.deepseek.com"
[providers.openrouter]
api_key = "old-openrouter-key"
"#,
)?;
clear_api_key()?;
let after = fs::read_to_string(&config_path)?;
assert!(
!after.contains("old-root-key"),
"root api_key must be stripped: {after}"
);
assert!(
!after.contains("old-provider-key"),
"provider-scoped deepseek key must be stripped: {after}"
);
assert!(
!after.contains("old-openrouter-key"),
"provider-scoped openrouter key must be stripped: {after}"
);
// Non-credential lines must survive.
assert!(after.contains("default_text_model"));
assert!(after.contains("base_url"));
Ok(())
}
/// Regression for #343: explicit in-memory `api_key` (non-empty,
/// non-sentinel) wins over the keyring/env layer so a freshly-typed
/// onboarding key takes effect even if a stale credential lingers.
#[test]
fn deepseek_api_key_prefers_explicit_in_memory_override() -> Result<()> {
let _lock = lock_test_env();
let nanos = SystemTime::now()
.duration_since(UNIX_EPOCH)
.unwrap()
.as_nanos();
let temp_root = env::temp_dir().join(format!(
"deepseek-tui-override-{}-{}",
std::process::id(),
nanos
));
fs::create_dir_all(&temp_root)?;
let _guard = EnvGuard::new(&temp_root);
let config = Config {
api_key: Some("freshly-typed-key".to_string()),
..Config::default()
};
let resolved = config
.deepseek_api_key()
.expect("explicit override must resolve");
assert_eq!(resolved, "freshly-typed-key");
Ok(())
}
#[test]
fn deepseek_api_key_ignores_sentinel_placeholder() -> Result<()> {
let _lock = lock_test_env();
let nanos = SystemTime::now()
.duration_since(UNIX_EPOCH)
.unwrap()
.as_nanos();
let temp_root = env::temp_dir().join(format!(
"deepseek-tui-sentinel-{}-{}",
std::process::id(),
nanos
));
fs::create_dir_all(&temp_root)?;
let _guard = EnvGuard::new(&temp_root);
let config = Config {
api_key: Some(API_KEYRING_SENTINEL.to_string()),
..Config::default()
};
// Sentinel must not be treated as a real key — the resolver should
// fall through to keyring / env / config-provider and ultimately
// bail out with a "key not found" error.
let _err = config.deepseek_api_key().expect_err(
"sentinel placeholder must not satisfy the API key check",
);
Ok(())
}
#[test]
fn test_tilde_expansion_in_paths() -> Result<()> {
let _lock = lock_test_env();
crates/tui/src/tui/ui.rs
+24
View File
@@ -1323,6 +1323,14 @@ async fn run_event_loop(
// Recreate the engine so it picks up the newly saved key
// without requiring a full process restart.
let _ = engine_handle.send(Op::Shutdown).await;
// Stamp the new key on the long-lived
// `Config` reference so any future clone
// (e.g. a subsequent /provider switch)
// sees it; the explicit-override path
// in `deepseek_api_key` (#343) makes
// this win even if the OS keyring
// still holds a stale credential.
config.api_key = Some(key.clone());
let mut refreshed_config = config.clone();
refreshed_config.api_key = Some(key);
let engine_config = build_engine_config(app, &refreshed_config);
@@ -3367,6 +3375,22 @@ async fn execute_command_input(
input: &str,
) -> Result<bool> {
let result = commands::execute(input, app);
// After /logout: clear the in-memory api_key fields so the next
// onboarding round entering a new key doesn't see the stale value
// (#343). The on-disk + keyring side is handled by clear_api_key()
// inside commands::config::logout.
if input.trim().eq_ignore_ascii_case("/logout") {
config.api_key = None;
if let Some(providers) = config.providers.as_mut() {
providers.deepseek.api_key = None;
providers.deepseek_cn.api_key = None;
providers.nvidia_nim.api_key = None;
providers.openrouter.api_key = None;
providers.novita.api_key = None;
providers.fireworks.api_key = None;
providers.sglang.api_key = None;
}
}
apply_command_result(app, engine_handle, task_manager, config, result).await
}