Skip to content

FileKeyringStore::set / delete silently wipe all stored secrets when load_unlocked errors #281

@Hmbown

Description

@Hmbown

What

`crates/secrets/src/lib.rs:275-279` (`FileKeyringStore::set`) and `crates/secrets/src/lib.rs:281-285` (`FileKeyringStore::delete`) both start with:

```rust
let mut blob = self.load_unlocked().unwrap_or_default();
```

`load_unlocked` returns an error when the secrets file:

  1. Has insecure permissions (`0644` instead of `0600`) — emits `SecretsError::InsecurePermissions` (line 230-235).
  2. Has corrupted JSON — emits `SecretsError::Json` from `serde_json::from_str` (line 241).
  3. Hits any other I/O error mid-read (rare but possible).

In all of those cases `unwrap_or_default()` falls back to an empty `FileSecretsBlob`, then `store_unlocked` writes it back — erasing every existing secret in the file and resetting permissions to `0600` on the way out. If the user has multiple provider keys stored (deepseek, nvidia-nim, …), updating one wipes the rest.

The flow: bad perms detected on load, load errors, code falls back to empty blob, write succeeds, all other secrets gone.

Reproduction (Unix)

```bash
chmod 644 ~/.deepseek/secrets/secrets.json # simulate accidentally chmod'd file
deepseek auth set deepseek-api-key xyz # any auth-set path that reaches FileKeyringStore::set
cat ~/.deepseek/secrets/secrets.json # only "deepseek-api-key" survives; previous nvidia-api-key etc. are gone.
```

(Same pattern on `delete` — running `deepseek auth delete ` on a permission-broken file deletes ALL secrets, not just ``.)

Why this matters

The `FileKeyringStore` is the fallback path on headless Linux (no `gnome-keyring` / `dbus`), CI runners, devcontainers, and Docker. Users who ssh in and accidentally `chmod 644` the file (e.g. via a permissive-default `umask` after `tar -x`) lose every credential the next time they run any auth command. Silent data loss is the worst kind of bug; the fix is failing loudly.

Proposed fix

In both `set` and `delete`:

```rust
let mut blob = match self.load_unlocked() {
Ok(blob) => blob,
Err(err) => return Err(err), // fail loud, do not propagate by overwriting
};
```

Tests that exercise this:

  1. `set` on an existing file with mode `0644` returns `InsecurePermissions` and leaves the file unmodified.
  2. `set` on an existing file with corrupt JSON returns `Json` and leaves the file unmodified.
  3. (Idempotence preserved): `set` on a missing file (`Ok(default)` from `load_unlocked`) still creates the file with one entry.

The user-visible recovery path then becomes "chmod the file to 0600" or "back up and delete the corrupt file" — both auditable, neither destructive.

Severity

Medium-high. Silent credential loss for users on the file-fallback path. Doesn't apply to the OS keyring fallback (`DefaultKeyringStore`), which is the primary path on macOS / Windows / desktop Linux.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions