From 8d68810ac4ee79653e82ba04113692093ff913cb Mon Sep 17 00:00:00 2001 From: Tosin Amuda Date: Wed, 26 Aug 2026 08:51:55 +0200 Subject: [PATCH 1/4] fix(credentials): asking about a key no longer creates one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Installing Compose and launching it put an item in the keychain before the user had typed anything. Reading went through `SecretStore::load`, which mints when there is neither a key nor a file — the correct behaviour for a WRITE, since saving needs somewhere to save to, and the wrong one for a read. On a fresh install the answer to "is a key configured?" is no, and answering it should not create a vault. `open_existing` answers without writing. No master key means nothing is stored, which is all a reader needs to know; it cannot distinguish "never minted" from "key lost" and does not have to, because both mean it has nothing to hand over. `loaded_for_write` still mints, which is where minting belongs. Found because the machine was reset to a genuine first-run state and the app was watched. `harness_list_models` — which the front end calls at boot to fill the model picker — resolves the harness, and resolving OpenRouter reads its key. That was the FOURTH boot path into the store after the catalog, the readiness probe and migration. Fixing callers one at a time was losing to a general rule: a read creates nothing. The earlier measurement claiming a keychain-free launch was WRONG, and wrong in a way worth recording. It ran `./target/debug/compose` built by `cargo build`, which does not embed the front end — so the UI never ran and could not make the calls being looked for. The control marker printed because it is in Rust setup, which runs either way, and that made a dead probe look alive. The measurement now runs the bundled binary and checks the front end is embedded and that the run created the same state a real launch does before believing a zero. frontend embedded: yes state created: app.db trash vaults workspaces.json mints: 0 master key: none --- src-tauri/src/harness/credentials.rs | 13 +++----- src-tauri/src/harness/secret_store.rs | 48 +++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 8 deletions(-) diff --git a/src-tauri/src/harness/credentials.rs b/src-tauri/src/harness/credentials.rs index e9f4104..3a449bb 100644 --- a/src-tauri/src/harness/credentials.rs +++ b/src-tauri/src/harness/credentials.rs @@ -84,14 +84,11 @@ pub fn set_config_dir(config_dir: &std::path::Path) { fn loaded_for_read(guard: &mut Option) -> Option<&mut SecretStore> { if guard.is_none() { let dir = CONFIG_DIR.get()?; - match SecretStore::load(dir).ok()? { - Opened::Ready(mut fresh) => { - let _opening = OpeningGuard::new(); - migrate_legacy_entries(&mut fresh); - *guard = Some(fresh); - } - Opened::KeyLost => return None, - } + // `open_existing`, not `load`: reading must not mint a master key. + let mut fresh = SecretStore::open_existing(dir).ok()??; + let _opening = OpeningGuard::new(); + migrate_legacy_entries(&mut fresh); + *guard = Some(fresh); } guard.as_mut() } diff --git a/src-tauri/src/harness/secret_store.rs b/src-tauri/src/harness/secret_store.rs index c08c1fb..1626ab6 100644 --- a/src-tauri/src/harness/secret_store.rs +++ b/src-tauri/src/harness/secret_store.rs @@ -107,6 +107,33 @@ impl SecretStore { }) } + /// Open the store only if it already exists — never mint. + /// + /// Minting is a WRITE, and a read must not perform one. Without this, merely + /// asking "is a key configured?" created the master key: on a fresh install + /// the answer is no, `decide` saw no key and no file, and minted an empty + /// vault before the user had typed anything. A keychain item should appear + /// when someone saves a key, not when something asks about one. + /// + /// `None` means "nothing stored" — whether because no key was ever minted or + /// because the one that opens the file is gone. A reader cannot tell those + /// apart and does not need to: both mean it has nothing to hand over. + pub fn open_existing(config_dir: &Path) -> Result, String> { + let Some(key) = read_master_key()? else { + return Ok(None); + }; + let path = config_dir.join(STORE_FILE); + let secrets = match read_envelope(&path)? { + Some(envelope) => match decrypt(&key, &envelope) { + Ok(secrets) => secrets, + // The key does not open this file: same as lost, for a reader. + Err(_) => return Ok(None), + }, + None => BTreeMap::new(), + }; + Ok(Some(Self { path, key, secrets })) + } + /// Start over after {@link Opened::KeyLost}: the file cannot be opened by /// any key that exists, so a fresh one is minted and the store begins empty. /// @@ -448,6 +475,27 @@ mod tests { assert_eq!(kept.len(), 1, "the unreadable store must be preserved"); } + /// A read must never create a keychain item. + /// + /// On a fresh install, asking "is a key configured?" used to mint the master + /// key — so installing Compose and opening Settings put an item in the + /// keychain before the user had typed anything. `open_existing` answers + /// "nothing stored" without writing. + #[test] + fn reading_an_empty_store_mints_nothing() { + let _keychain = TestKeychain::new("ai.latentic.compose.test.readonly"); + let dir = tempfile::tempdir().expect("tempdir"); + + assert!( + SecretStore::open_existing(dir.path()).expect("no error").is_none(), + "nothing is stored, so there is nothing to open", + ); + assert!( + read_master_key().expect("no error").is_none(), + "a read created a master key — the thing it must never do", + ); + } + fn envelope_for(key: [u8; KEY_LEN], pairs: &[(&str, &str)]) -> Envelope { let secrets: BTreeMap = pairs.iter().map(|(k, v)| ((*k).to_owned(), (*v).to_owned())).collect(); From 72967849ff64894af0987ebb447c588f572606ff Mon Sep 17 00:00:00 2001 From: Tosin Amuda Date: Wed, 26 Aug 2026 09:00:44 +0200 Subject: [PATCH 2/4] fix(credentials): nothing asks the keychain at launch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The invariant, stated plainly: a user who never uses an agent that needs an API key should never see a keychain prompt. Someone who does should see it when they save a key or send a message — not when they open the app. Four boot paths reached the credential store, found one at a time because each fix revealed the next: 1. the agent catalog — listing does not need keys (#190) 2. the readiness probe — wants `installed`, not auth (#190) 3. migration — wants spec NAMES (#190) 4. model listing — this commit `harness_list_models` resolves the harness to ask it for models, and resolving a hosted harness reads its stored key. The composer footer and the default-model pick both call it at boot. It now takes `with_auth` like `harness_readiness`: off by default, and Settings — where someone is choosing a model and an authenticated endpoint has to list — opts in. Most endpoints list publicly, and a listing that fails leaves the picker on free-text, which it already handles. Chasing callers was losing to a general rule, so this also stops READS creating anything: `open_existing` answers "nothing stored" without minting, where `load` minted on an empty vault. That is correct for a write and wrong for a read, and it is why installing Compose and opening it put an item in the keychain before the user had typed. Measured on a BUNDLED binary, first-run state, with the front end confirmed embedded and the run confirmed to have created the same state a real launch does — the check my earlier, wrong, measurement lacked: frontend embedded: yes UI ran: app.db trash vaults workspaces.json KEYCHAIN READS at launch: 0 Unchanged: sending resolves with `Secrets::Resolve` and reads the key, and saving mints one. Those are the moments a key is genuinely needed. --- src-tauri/src/harness/commands.rs | 23 +++++++++++++++++-- src/app/store/harnessStore.ts | 6 ++--- src/features/settings/ModelPicker.tsx | 2 +- src/features/settings/agentConfigControls.tsx | 2 +- src/lib/ipc/harnessClient.ts | 11 +++++++-- 5 files changed, 35 insertions(+), 9 deletions(-) diff --git a/src-tauri/src/harness/commands.rs b/src-tauri/src/harness/commands.rs index 49f369a..26edd39 100644 --- a/src-tauri/src/harness/commands.rs +++ b/src-tauri/src/harness/commands.rs @@ -74,9 +74,28 @@ fn with_local_install_truth(mut readiness: Readiness, ollama_present: bool) -> R readiness } +/// `with_auth` decides whether this may open the credential store. +/// +/// The composer and the default-model pick call this at boot, and resolving a +/// hosted harness reads its stored key — which is a keychain access at launch +/// for anyone who has one. Most model endpoints (OpenRouter's among them) list +/// publicly, and a listing that fails leaves the picker on free-text, which it +/// already handles. Settings, where someone is choosing a model and an +/// authenticated endpoint must work, opts in. #[tauri::command(async)] -pub fn harness_list_models(harness_id: String) -> Result, String> { - resolve(&harness_id)?.list_models().map_err(|e| e.to_string()) +pub fn harness_list_models( + harness_id: String, + with_auth: Option, +) -> Result, String> { + let secrets = if with_auth.unwrap_or(false) { + crate::harness::registry::Secrets::Resolve + } else { + crate::harness::registry::Secrets::Skip + }; + crate::harness::registry::compose_harness_by_id_with(&harness_id, secrets) + .ok_or_else(|| format!("Unknown assistant: {harness_id}"))? + .list_models() + .map_err(|e| e.to_string()) } #[tauri::command(async)] diff --git a/src/app/store/harnessStore.ts b/src/app/store/harnessStore.ts index ab96517..1954a95 100644 --- a/src/app/store/harnessStore.ts +++ b/src/app/store/harnessStore.ts @@ -56,7 +56,7 @@ export interface HarnessState { * Codex). Keyed by harness id; absent until loaded, `[]` when discovery finds * none (the picker then falls back to a free-text model field). */ harnessModels: Record; - loadHarnessModels: (harnessId: string) => Promise; + loadHarnessModels: (harnessId: string, withAuth?: boolean) => Promise; /** Per-agent readiness for the picker's status dots, cached with a probe time * so the picker doesn't re-probe on every open. */ harnessStatusById: Record; @@ -244,9 +244,9 @@ export const useHarnessStore = create((set, get) => { set({ harnessCatalog: catalog }); }, harnessModels: {}, - loadHarnessModels: async (harnessId) => { + loadHarnessModels: async (harnessId, withAuth = false) => { // Best-effort; failures resolve to [] (the picker falls back to free-text). - const models = await harnessListModels(harnessId).catch(() => [] as HarnessModel[]); + const models = await harnessListModels(harnessId, withAuth).catch(() => [] as HarnessModel[]); set((state) => ({ harnessModels: { ...state.harnessModels, [harnessId]: models } })); }, harnessStatusById: {}, diff --git a/src/features/settings/ModelPicker.tsx b/src/features/settings/ModelPicker.tsx index 55ee363..8b271d2 100644 --- a/src/features/settings/ModelPicker.tsx +++ b/src/features/settings/ModelPicker.tsx @@ -69,7 +69,7 @@ export function ModelPicker({ harnessId }: { harnessId: string }) { const refresh = async () => { setRefreshing(true); try { - await loadHarnessModels(harnessId); + await loadHarnessModels(harnessId, true); } finally { setRefreshing(false); } diff --git a/src/features/settings/agentConfigControls.tsx b/src/features/settings/agentConfigControls.tsx index 67781d9..48e8862 100644 --- a/src/features/settings/agentConfigControls.tsx +++ b/src/features/settings/agentConfigControls.tsx @@ -31,7 +31,7 @@ export function ModelSection({ harnessId }: { harnessId: string }) { useEffect(() => { if (caps.models.length === 0) { - void loadHarnessModels(harnessId); + void loadHarnessModels(harnessId, true); } }, [harnessId, caps.models.length, loadHarnessModels]); diff --git a/src/lib/ipc/harnessClient.ts b/src/lib/ipc/harnessClient.ts index 8dce1cc..48a22de 100644 --- a/src/lib/ipc/harnessClient.ts +++ b/src/lib/ipc/harnessClient.ts @@ -380,11 +380,18 @@ export async function ollamaInstalled(): Promise { * because discovery is dynamic. Best-effort: `[]` in the browser preview or on * any backend error (the free-text model field still applies). */ -export async function harnessListModels(harnessId: string): Promise { +export async function harnessListModels( + harnessId: string, + /** Allow reading the stored key to authenticate the listing. Off by default: + * the composer and the default-model pick run at boot, and a hosted harness + * resolves its key when built — a keychain access at launch. Settings opts + * in, where an authenticated endpoint has to work. */ + withAuth = false, +): Promise { if (!isTauriRuntime()) { return []; } - return invoke("harness_list_models", { harnessId }); + return invoke("harness_list_models", { harnessId, withAuth }); } /** From 7e4f5798058a7145058dfc2f3ea1784eb5bbd4ee Mon Sep 17 00:00:00 2001 From: Tosin Amuda Date: Wed, 26 Aug 2026 09:49:47 +0200 Subject: [PATCH 3/4] fix(secret-store): a test can no longer delete the real master key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Running `cargo test` raised a keychain prompt and then deleted the developer's `ai.latentic.compose` master key, taking every API key they had saved with it. `keyring_service()` under `cfg(test)` fell back to the REAL service name when no `TestKeychain` was installed: TEST_SERVICE.with(...).unwrap_or_else(|| KEYRING_SERVICE.to_owned()) Two of eleven store tests installed a guard. The rest ran against the live entry, and one of them calls `clear()`, which calls `delete_master_key()`. The file `clear` removes is scoped to a temp dir; the keychain entry it deletes is not. The prompt came first, because the test binary is not the signed app and so is not in that item's ACL. The doc comment directly above the fallback already stated the rule it broke: "a test must never write to the developer's own `ai.latentic.compose` entry." There is now no route back to the real service from a test. Absent a guard, `keyring_service()` panics and names the fix. `TestKeychain::new()` takes no argument and derives a service unique to the caller, so guards cannot collide and no test has to invent a name. The `clear` test gets the guard it always needed. Two tests hold the invariant: one asserts the unguarded call panics, one asserts a guard never names the service the app uses. Found the hard way — it deleted a key mid-session, mine to lose that time. Confirmed after the fix: a full run leaves no `ai.latentic.compose` item and no throwaway leftovers. 256 tests pass. --- src-tauri/src/harness/secret_store.rs | 56 +++++++++++++++++++++------ 1 file changed, 44 insertions(+), 12 deletions(-) diff --git a/src-tauri/src/harness/secret_store.rs b/src-tauri/src/harness/secret_store.rs index 1626ab6..1a2508f 100644 --- a/src-tauri/src/harness/secret_store.rs +++ b/src-tauri/src/harness/secret_store.rs @@ -302,10 +302,10 @@ fn new_master_key() -> [u8; KEY_LEN] { /// The keychain service the master key lives under. /// -/// A constant in the app. Under `cfg(test)` it can be pointed at a throwaway -/// name, because the alternative is that `recover` — the whole recovery path — -/// stays untestable: exercising it for real means minting a master key, and a -/// test must never write to the developer's own `ai.latentic.compose` entry. +/// A constant in the app. Under `cfg(test)` there is no route back to it: a +/// test runs against a throwaway service or it panics. `ai.latentic.compose` +/// belongs to whoever is running the suite, and `clear` deletes what this +/// points at — a default of "the real one" costs them every key they had saved. #[cfg(not(test))] fn keyring_service() -> String { KEYRING_SERVICE.to_owned() @@ -319,19 +319,30 @@ thread_local! { #[cfg(test)] fn keyring_service() -> String { - TEST_SERVICE.with(|s| s.borrow().clone()).unwrap_or_else(|| KEYRING_SERVICE.to_owned()) + TEST_SERVICE.with(|s| s.borrow().clone()).unwrap_or_else(|| { + panic!( + "this test reached the master key with no TestKeychain installed; \ + add `let _keychain = TestKeychain::new();`" + ) + }) } -/// Point the master key at a throwaway service for the current test, and delete -/// whatever it left behind when the guard drops. +/// Point the master key at a service unique to this test, and delete whatever +/// it left behind when the guard drops. #[cfg(test)] struct TestKeychain(String); #[cfg(test)] impl TestKeychain { - fn new(name: &str) -> Self { - TEST_SERVICE.with(|s| *s.borrow_mut() = Some(name.to_owned())); - Self(name.to_owned()) + fn new() -> Self { + static NEXT: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + let name = format!( + "{KEYRING_SERVICE}.test.{}.{}", + std::process::id(), + NEXT.fetch_add(1, std::sync::atomic::Ordering::Relaxed) + ); + TEST_SERVICE.with(|s| *s.borrow_mut() = Some(name.clone())); + Self(name) } } @@ -402,6 +413,23 @@ mod tests { SecretStore { path: dir.join(STORE_FILE), key, secrets: BTreeMap::new() } } + /// Guarding the guard. A missing `TestKeychain` used to fall back to the + /// real service, so `clear` in a test deleted the master key of whoever ran + /// it — every saved API key with it, and a keychain prompt on the way out. + /// Failing loudly is the only fallback that cannot cost someone their data. + #[test] + #[should_panic(expected = "TestKeychain")] + fn reaching_the_keychain_without_a_guard_is_a_test_failure() { + keyring_service(); + } + + #[test] + fn a_guard_never_names_the_service_the_app_uses() { + let first = TestKeychain::new(); + assert_ne!(keyring_service(), KEYRING_SERVICE); + assert_ne!(TestKeychain::new().0, first.0, "two guards must not collide"); + } + /// The safety property of recovery, testable without the OS vault: the /// unreadable file is preserved, never destroyed. `recover` itself mints a /// master key and so cannot run in a test — but this is the half that could @@ -438,7 +466,7 @@ mod tests { /// was trying to save is there afterwards and readable on a reopen. #[test] fn a_store_with_no_key_recovers_and_accepts_the_save_that_was_blocked() { - let _keychain = TestKeychain::new("ai.latentic.compose.test.recover"); + let _keychain = TestKeychain::new(); let dir = tempfile::tempdir().expect("tempdir"); // The wedged state: a file encrypted under a key the keychain has never @@ -483,7 +511,7 @@ mod tests { /// "nothing stored" without writing. #[test] fn reading_an_empty_store_mints_nothing() { - let _keychain = TestKeychain::new("ai.latentic.compose.test.readonly"); + let _keychain = TestKeychain::new(); let dir = tempfile::tempdir().expect("tempdir"); assert!( @@ -615,6 +643,10 @@ mod tests { fn clearing_removes_the_file_as_well_as_the_secrets() { // "Reset all data". `set("")` on the last secret also removes the file, // so only `clear` covers the case where several remain. + // + // `clear` deletes the master key, and the keychain is not scoped to the + // temp dir the way the file is. + let _keychain = TestKeychain::new(); let dir = tempfile::tempdir().expect("tempdir"); let mut store = store_at(dir.path(), [5u8; KEY_LEN]); store.set("openrouter", "sk-a").expect("write"); From ab7fcd65237f07c42f9ffef8a1d2fdd4973b2dd5 Mon Sep 17 00:00:00 2001 From: Tosin Amuda Date: Wed, 26 Aug 2026 09:51:34 +0200 Subject: [PATCH 4/4] fix(secret-store): a guard restores the one it nested inside Dropping an inner `TestKeychain` cleared the service outright, so an outer guard was left pointing at nothing and the next call to `keyring_service()` panicked somewhere unrelated to the mistake. It now restores what it replaced, and a test covers the nesting. --- src-tauri/src/harness/secret_store.rs | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/src-tauri/src/harness/secret_store.rs b/src-tauri/src/harness/secret_store.rs index 1a2508f..660093f 100644 --- a/src-tauri/src/harness/secret_store.rs +++ b/src-tauri/src/harness/secret_store.rs @@ -330,29 +330,34 @@ fn keyring_service() -> String { /// Point the master key at a service unique to this test, and delete whatever /// it left behind when the guard drops. #[cfg(test)] -struct TestKeychain(String); +struct TestKeychain { + service: String, + /// Restored on drop, so an inner guard cannot leave an outer one pointing + /// at nothing — which would panic somewhere unrelated to the mistake. + outer: Option, +} #[cfg(test)] impl TestKeychain { fn new() -> Self { static NEXT: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); - let name = format!( + let service = format!( "{KEYRING_SERVICE}.test.{}.{}", std::process::id(), NEXT.fetch_add(1, std::sync::atomic::Ordering::Relaxed) ); - TEST_SERVICE.with(|s| *s.borrow_mut() = Some(name.clone())); - Self(name) + let outer = TEST_SERVICE.with(|s| s.replace(Some(service.clone()))); + Self { service, outer } } } #[cfg(test)] impl Drop for TestKeychain { fn drop(&mut self) { - if let Ok(entry) = keyring::Entry::new(&self.0, KEYRING_ACCOUNT) { + if let Ok(entry) = keyring::Entry::new(&self.service, KEYRING_ACCOUNT) { let _ = entry.delete_credential(); } - TEST_SERVICE.with(|s| *s.borrow_mut() = None); + TEST_SERVICE.with(|s| *s.borrow_mut() = self.outer.take()); } } @@ -425,9 +430,14 @@ mod tests { #[test] fn a_guard_never_names_the_service_the_app_uses() { - let first = TestKeychain::new(); + let outer = TestKeychain::new(); assert_ne!(keyring_service(), KEYRING_SERVICE); - assert_ne!(TestKeychain::new().0, first.0, "two guards must not collide"); + + { + let inner = TestKeychain::new(); + assert_ne!(inner.service, outer.service, "two guards must not collide"); + } + assert_eq!(keyring_service(), outer.service, "the outer guard still holds"); } /// The safety property of recovery, testable without the OS vault: the