89c63891fa
Replace all instances of the legacy product names "Kon" and "Corbie" with "Magnotia" across user-facing copy, code identifiers, package names, bundle ids, file paths, and documentation. Preserves the unrelated "konsole" (KDE terminal) reference and the parent CORBEL company name. - Renames 10 Rust crates (kon-* → magnotia-*) and the tauri binary - Updates package.json, tauri.conf.json (productName + identifier) - Renames CSS classes (kon-rh-* → magnotia-rh-*) and animations - Renames brand and roadmap docs - Regenerates Cargo.lock and package-lock.json Verified: svelte-check passes; pure-rust crates compile under new names.
52 lines
2.3 KiB
Markdown
52 lines
2.3 KiB
Markdown
# RB-11 MAJOR: keystore::store_api_key is a thread-unsafe safe API
|
|
|
|
**Severity:** MAJOR
|
|
**Path:** `crates/cloud-providers/src/keystore.rs:6-18`
|
|
**Source:** [2026-04-22 code review](../code-review-2026-04-22.md)
|
|
**Labels:** release-blocker, major, unsafe-api, cloud
|
|
**Status:** RESOLVED (2026-04-22)
|
|
|
|
## Resolution
|
|
|
|
Chose acceptance option 2. The environment-mutation stub is gone;
|
|
`store_api_key` now writes into a process-global
|
|
`OnceLock<Mutex<HashMap<String, String>>>`, so the safe signature matches
|
|
the actual safety properties.
|
|
|
|
Additional details:
|
|
|
|
- Stored keys now live in-memory only for the life of the process.
|
|
- `retrieve_api_key` checks the in-memory keystore first, then falls
|
|
back to read-only `MAGNOTIA_API_KEY_<PROVIDER>` environment variables so
|
|
externally injected secrets still work.
|
|
- Module docs now describe the real tradeoff clearly: safe from any
|
|
thread, but non-persistent until a proper OS keychain backend lands.
|
|
|
|
Regression tests:
|
|
|
|
- `stored_key_is_retrievable_without_env_mutation`
|
|
- `providers_do_not_overlap`
|
|
|
|
## Problem
|
|
|
|
`store_api_key` is declared as a safe `pub fn`. Its implementation relies on `std::env::set_var`, which is documented as Undefined Behaviour outside single-threaded initialisation. The file's module comment acknowledges the precondition but the function signature does not enforce it — any caller can invoke it from any thread, and the compiler won't object.
|
|
|
|
## Acceptance
|
|
|
|
Choose one:
|
|
|
|
1. **Use an OS keychain backend** (e.g. `keyring` crate) so there is no `set_var` involvement. Preferred — actually secret-safe, cross-platform.
|
|
2. **Use a process-global `OnceLock` or `Mutex<HashMap>`** inside the module instead of `set_var`. Removes the UB, trades persistence.
|
|
3. **Mark `store_api_key` as `unsafe`** and document the "call once before threads spawn" contract at the signature level. Ugly but honest.
|
|
|
|
Whichever path, update the signature and doc comments to match the safety properties actually provided.
|
|
|
|
## Fix scope
|
|
|
|
Medium. Option 1 is the right long-term answer but adds a dep and platform-specific auth prompts (macOS Keychain asks the user on first access). Option 2 is fastest. Option 3 is cosmetic.
|
|
|
|
## Dependencies
|
|
|
|
- None — standalone fix.
|
|
- Coupled with future BYO LLM endpoint work (storing API keys safely is a prerequisite).
|