From e7a754d8e3e8ea24f0a38b0da580d19ac08ce449 Mon Sep 17 00:00:00 2001 From: luvs01 Date: Sat, 8 Aug 2026 19:29:29 +0900 Subject: [PATCH] fix(oauth): remove legacy backup on account deletion --- src/oauth/store.ts | 23 +++++++++++++++++------ tests/oauth-store-multi.test.ts | 29 ++++++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/src/oauth/store.ts b/src/oauth/store.ts index a018de097f..be0adacfcf 100644 --- a/src/oauth/store.ts +++ b/src/oauth/store.ts @@ -4,8 +4,10 @@ * Multiauth shape (260706): each provider value is a ProviderAccountSet * `{ activeAccountId, accounts: [{ id, credential, needsReauth?, addedAt? }] }`. * Legacy single-credential values (`{ access, refresh, expires, ... }`) normalize on load, - * and the first new-shape persist writes a one-time `auth.json.pre-multiauth` backup so a - * downgraded loader (which silently drops unknown shapes) cannot destroy refresh tokens. + * and the first non-destructive new-shape persist writes a one-time + * `auth.json.pre-multiauth` backup so a downgraded loader (which silently drops unknown + * shapes) cannot destroy refresh tokens. Destructive mutations remove that backup so + * logout and account deletion do not retain the deleted credentials. * * Exceptions: * - `chatgpt` stays single-slot (always replaced): codex-auth-api uses it as a scratch slot @@ -235,6 +237,14 @@ function backupLegacyOnce(): void { } catch { /* best-effort */ } } +function removeLegacyBackup(): void { + try { + unlinkSync(`${getAuthStorePath()}.pre-multiauth`); + } catch (error) { + if (errorCode(error) !== "ENOENT") throw error; + } +} + function isCredentialSource(value: unknown): value is OAuthCredentialSource { return value === "oauth" || value === "local-cli" || value === "credential-file" || value === "environment" || value === "manual"; } @@ -450,11 +460,12 @@ function serializeMutation(work: () => Promise, retainedValues: readonly u drainOAuthMutations(); return result; } -export function mutateStore(fn:(store:AuthStore)=>T|Promise, retainedValues: readonly unknown[] = [], options?: { waitMs?: number }):Promise{return serializeMutation(async()=>{const guard=await createOAuthFileLock({path:getAuthStoreLockPath(),staleAfterMs:30000}).acquire();try{ +export function mutateStore(fn:(store:AuthStore)=>T|Promise, retainedValues: readonly unknown[] = [], options?: { waitMs?: number; removeLegacyBackup?: boolean }):Promise{return serializeMutation(async()=>{const guard=await createOAuthFileLock({path:getAuthStoreLockPath(),staleAfterMs:30000}).acquire();try{ const { store, hadLegacy } = loadAuthStoreInternal(); - if (hadLegacy) backupLegacyOnce(); + if (hadLegacy && !options?.removeLegacyBackup) backupLegacyOnce(); const result = await fn(store); persist(store); + if (options?.removeLegacyBackup) removeLegacyBackup(); return result; }finally{guard.release();}}, retainedValues, options?.waitMs); } @@ -534,7 +545,7 @@ export async function removeCredential(provider: string): Promise { return; } set.activeAccountId = set.accounts[0]!.id; - }, [provider]); + }, [provider], { removeLegacyBackup: true }); } // --------------------------------------------------------------------------- @@ -609,7 +620,7 @@ export async function removeAccount(provider: string, accountId: string): Promis } if (set.activeAccountId === accountId) set.activeAccountId = set.accounts[0]!.id; return true; - }, [provider, accountId]); + }, [provider, accountId], { removeLegacyBackup: true }); return removed; } diff --git a/tests/oauth-store-multi.test.ts b/tests/oauth-store-multi.test.ts index fcf09036c6..33a0f95929 100644 --- a/tests/oauth-store-multi.test.ts +++ b/tests/oauth-store-multi.test.ts @@ -67,7 +67,7 @@ describe("multi-account auth store", () => { xai: { access: "legacy-access", refresh: "legacy-refresh", expires: Date.now() + 1000, email: "old@example.com" }, })); expect(getCredential("xai")?.access).toBe("legacy-access"); - // Any mutation persists the new shape + writes the downgrade backup. + // A non-destructive mutation persists the new shape + writes the downgrade backup. await saveCredential("xai", cred({ email: "old@example.com", access: "new-access" })); expect(getCredential("xai")?.access).toBe("new-access"); const raw = JSON.parse(readFileSync(authPath, "utf-8")); @@ -75,6 +75,33 @@ describe("multi-account auth store", () => { expect(existsSync(`${authPath}.pre-multiauth`)).toBe(true); }); + test("logout migrates a legacy store without retaining its credential backup", async () => { + const authPath = join(TEST_DIR, "auth.json"); + writeFileSync(authPath, JSON.stringify({ + xai: { access: "legacy-access", refresh: "legacy-refresh", expires: Date.now() + 1000 }, + })); + + await removeCredential("xai"); + + expect(JSON.parse(readFileSync(authPath, "utf-8"))).toEqual({}); + expect(existsSync(`${authPath}.pre-multiauth`)).toBe(false); + }); + + test("account deletion removes an existing legacy credential backup", async () => { + const authPath = join(TEST_DIR, "auth.json"); + const legacy = { + xai: { access: "legacy-access", refresh: "legacy-refresh", expires: Date.now() + 1000 }, + }; + writeFileSync(authPath, JSON.stringify(legacy)); + writeFileSync(`${authPath}.pre-multiauth`, JSON.stringify(legacy)); + const accountId = getAccountSet("xai")!.activeAccountId; + + expect(await removeAccount("xai", accountId)).toBe(true); + + expect(JSON.parse(readFileSync(authPath, "utf-8"))).toEqual({}); + expect(existsSync(`${authPath}.pre-multiauth`)).toBe(false); + }); + test("legacy credential WITHOUT identity gets a deterministic account id across loads", async () => { // Legacy stores are re-normalized on EVERY load without being persisted, so the // derived id must be stable: a time-salted id would make getAccountSet and