From 73abec9a995d157e3acb55edfd7d8f082cc11aae Mon Sep 17 00:00:00 2001
From: luvs01 <27862058+luvs01@users.noreply.github.com>
Date: Mon, 7 Sep 2026 11:02:32 +0900
Subject: [PATCH] fix(providers): refuse keychain restore across provider
 ownership

---
 src/providers/key-store.ts                 | 22 +++++++++++++++
 tests/providers/provider-key-store.test.ts | 32 +++++++++++++++++++++-
 2 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/src/providers/key-store.ts b/src/providers/key-store.ts
index 12e4ce6cb7..614fd3372f 100644
--- a/src/providers/key-store.ts
+++ b/src/providers/key-store.ts
@@ -64,6 +64,16 @@ function keychainAccount(reference: string): string {
   return reference.slice(KEYCHAIN_REFERENCE_PREFIX.length);
 }
 
+/**
+ * A reference belongs to `name` only when its account is that provider's own active account
+ * or one of its pool accounts. `storeProviderKeyInKeychain` writes exactly those two shapes,
+ * so anything else in a provider's config names another provider's secret.
+ */
+function keychainReferenceBelongsToProvider(reference: string, name: string): boolean {
+  const account = keychainAccount(reference);
+  return account === name || account.startsWith(`${name}/`);
+}
+
 function readKeychain(account: string): string | undefined {
   const cached = resolvedCache.get(account);
   if (cached !== undefined) return cached;
@@ -185,6 +195,18 @@ export function restoreProviderKeyFromKeychain(config: OcxConfig, name: string):
   const pool = provider.apiKeyPool ?? [];
   const resolved = new Map<string, string>();
   const refs = [provider.apiKey, ...pool.map(e => e.key)].filter(isKeychainReference);
+  // Restore reads a secret out of the keychain, writes it back to config as plaintext, and then
+  // DELETES the keychain item. Following a reference to another provider's account would both
+  // disclose that secret through this provider's config and destroy the real owner's credential,
+  // so refuse before anything is read or removed.
+  const foreign = refs.filter(ref => !keychainReferenceBelongsToProvider(ref, name));
+  if (foreign.length > 0) {
+    return {
+      ok: false,
+      error: `provider "${name}" references a keychain account it does not own (${foreign.length} reference(s)); config left unchanged`,
+      status: 400,
+    };
+  }
   for (const ref of refs) {
     const account = keychainAccount(ref);
     if (resolved.has(account)) continue;
diff --git a/tests/providers/provider-key-store.test.ts b/tests/providers/provider-key-store.test.ts
index 645920e32c..1197a1fea9 100644
--- a/tests/providers/provider-key-store.test.ts
+++ b/tests/providers/provider-key-store.test.ts
@@ -156,6 +156,37 @@ describe("store / restore", () => {
     expect(probeProviderKeychain().available).toBe(false);
   });
 
+  test("restore refuses a reference to another provider's keychain account", () => {
+    const { store, factory } = fakeKeychain();
+    setProviderKeychainEntryFactoryForTests(factory);
+    const config = loadConfig();
+    config.providers.other = { adapter: "openai-chat", baseUrl: "https://other.example/v1", apiKey: POOL_SECRET };
+    expect(storeProviderKeyInKeychain(config, "other")).toEqual({ ok: true, moved: 1 });
+    expect(config.providers.other!.apiKey).toBe("keychain:other");
+
+    // Point "relay" at the account "other" owns. Restore would otherwise read that secret,
+    // write it into relay's config as plaintext, and delete the owner's keychain item.
+    config.providers.relay!.apiKey = "keychain:other";
+    const result = restoreProviderKeyFromKeychain(config, "relay");
+    expect(result.ok).toBe(false);
+    if (!result.ok) expect(result.status).toBe(400);
+
+    expect(config.providers.relay!.apiKey).toBe("keychain:other");
+    expect(readFileSync(join(testDir, "config.json"), "utf8")).not.toContain(POOL_SECRET);
+    // The real owner's secret is still in the keychain and still resolves for that provider.
+    expect(store.size).toBe(1);
+    expect(resolveProviderApiKey(config.providers.other!.apiKey)).toBe(POOL_SECRET);
+  });
+
+  test("restore still accepts a provider's own active and pool accounts", () => {
+    const { factory } = fakeKeychain();
+    setProviderKeychainEntryFactoryForTests(factory);
+    const config = loadConfig();
+    config.providers.relay!.apiKeyPool = [{ id: "a1", key: SECRET }, { id: "b2", key: POOL_SECRET }];
+    expect(storeProviderKeyInKeychain(config, "relay")).toEqual({ ok: true, moved: 2 });
+    expect(restoreProviderKeyFromKeychain(config, "relay")).toEqual({ ok: true, restored: 2 });
+  });
+
   test("management route: GET reports store kind, POST store/restore round-trips", async () => {
     const { factory } = fakeKeychain();
     setProviderKeychainEntryFactoryForTests(factory);
@@ -192,4 +223,3 @@ describe("store / restore", () => {
     }
   });
 });
-