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-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..660093f 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. /// @@ -275,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() @@ -292,29 +319,45 @@ 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); +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(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 service = format!( + "{KEYRING_SERVICE}.test.{}.{}", + std::process::id(), + NEXT.fetch_add(1, std::sync::atomic::Ordering::Relaxed) + ); + 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()); } } @@ -375,6 +418,28 @@ 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 outer = TestKeychain::new(); + assert_ne!(keyring_service(), KEYRING_SERVICE); + + { + 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 /// 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 @@ -411,7 +476,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 @@ -448,6 +513,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(); + 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(); @@ -567,6 +653,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"); 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 }); } /**