fix(secrets): never overwrite the secrets file when load_unlocked errors#282
Conversation
`FileKeyringStore::set` and `delete` did `self.load_unlocked().unwrap_or_default()`, which wiped every existing secret if the read failed for any reason other than \"file is missing\": - file mode != 0600 (`InsecurePermissions`) — easy on headless / CI environments where a permissive umask got applied - corrupt JSON - transient I/O error In all of those, the next `store_unlocked` overwrote the file with an empty-or-single-entry blob and reset perms to 0600, silently losing every other provider's key. Switch both call sites to `?`. `load_unlocked` already returns `Ok(default)` for a missing file, so the first-write-creates-the-file ergonomic is preserved (covered by the new `file_store_set_still_creates_file_when_missing` test). Adds four regression tests: - set: insecure perms surface InsecurePermissions and leave the file byte-identical. - delete: same. - set: corrupt JSON surfaces the parse error and leaves the file byte-identical. - set: missing file path still works (idempotence guard). Closes #281
There was a problem hiding this comment.
Pull request overview
Fixes a data-loss bug in the deepseek-secrets file-backed backend where read errors during set/delete could be silently swallowed and then persisted back as an empty/partial secrets blob, overwriting other stored provider keys.
Changes:
- Propagate
load_unlocked()errors inFileKeyringStore::setanddeleteinstead of falling back todefault. - Add regression tests covering insecure permissions, corrupt JSON, and the missing-file “first write creates file” path.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request addresses a bug where FileKeyringStore would silently overwrite secrets upon encountering read errors. It replaces unwrap_or_default() with error propagation in the set and delete methods and adds regression tests for permission issues and JSON corruption. Feedback suggests further optimizing these methods to avoid redundant disk I/O when the file content would remain unchanged.
| let mut blob = self.load_unlocked()?; | ||
| blob.entries.insert(key.to_string(), value.to_string()); | ||
| self.store_unlocked(&blob) |
There was a problem hiding this comment.
To avoid unnecessary disk I/O and reduce the window for potential file corruption (as fs::write is not atomic), consider skipping the write if the secret value is already identical to what's on disk.
| let mut blob = self.load_unlocked()?; | |
| blob.entries.insert(key.to_string(), value.to_string()); | |
| self.store_unlocked(&blob) | |
| let mut blob = self.load_unlocked()?; | |
| if blob.entries.get(key).is_some_and(|v| v.as_str() == value) { | |
| return Ok(()); | |
| } | |
| blob.entries.insert(key.to_string(), value.to_string()); | |
| self.store_unlocked(&blob) |
| let mut blob = self.load_unlocked()?; | ||
| blob.entries.remove(key); | ||
| self.store_unlocked(&blob) |
There was a problem hiding this comment.
This optimization avoids unnecessary writes when the key is already absent. More importantly, it prevents the side effect of creating an empty secrets file (and its parent directories) when calling delete on a non-existent store.
let mut blob = self.load_unlocked()?;
if blob.entries.remove(key).is_some() {
return self.store_unlocked(&blob);
}
Ok(())
Summary
Closes #281.
`FileKeyringStore::set` and `delete` did:
```rust
let mut blob = self.load_unlocked().unwrap_or_default();
```
`load_unlocked` returns `Ok(default)` only for a missing file. For every other error — `InsecurePermissions` (file mode != 0600), `Json` (corrupt content), or any I/O error mid-read — `unwrap_or_default()` swallowed the failure, then `store_unlocked` wrote an empty-or-single-entry blob over the file and reset permissions to 0600. Net effect: every other provider's key got silently wiped.
Switch both call sites to `?`. The first-write-creates-the-file ergonomic still works because the missing-file path returns `Ok(default)` and `?` passes that through.
Repro before this PR (Unix)
```bash
chmod 644 ~/.deepseek/secrets/secrets.json # accidentally permissive umask
deepseek auth set deepseek-api-key xyz
cat ~/.deepseek/secrets/secrets.json # only deepseek-api-key survives
```
Test plan
Severity
Medium-high. Silent credential loss on the file-fallback path (headless Linux / CI / devcontainers). Doesn't affect `DefaultKeyringStore` (OS keyring), which is the primary path on macOS / Windows / desktop Linux.
🤖 Generated with Claude Code