From f456a4f2ee9683501d1c32308f6386c148e367dc Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Fri, 18 Sep 2026 06:50:14 +0900 Subject: [PATCH 1/3] fix(oauth): drop legacy credential backup on destructive mutations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit auth.json.pre-multiauth copies the whole legacy store for downgrade recovery, but removeCredential/removeAccount left it behind — and a legacy-shaped store re-created it — so logout and account deletion kept a file holding the very refresh tokens the user destroyed. mutateStore gains a removeLegacyBackup option: it skips the one-time create and unlinks the backup after persist. Removal is best-effort (ENOENT ignored, other failures warn) because it runs after the store is persisted — a failed unlink must not report a failed logout for an account that is already gone. A stale uninstall-manifest entry is harmless: removeOwnedConfigState skips missing paths. Covers the destructive-migration, pre-existing-backup, and stale-backup-on-migrated-store cases in oauth-store-multi tests. --- src/oauth/store.ts | 33 +++++++++++++++++---- tests/oauth/oauth-store-multi.test.ts | 41 +++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 6 deletions(-) diff --git a/src/oauth/store.ts b/src/oauth/store.ts index a7ea6eaa2df..c53b396fd9d 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 @@ -448,6 +450,24 @@ function backupLegacyOnce(): void { } catch { /* best-effort */ } } +/** + * Destructive mutations (logout, account deletion) also drop the downgrade backup: it + * holds a copy of the very credentials the user removed, so keeping it would retain + * tokens the user asked to destroy. Best-effort like the create path — the removal runs + * after persist, so a failed unlink must not report a failed logout for an account that + * is already gone. A stale uninstall-manifest entry is harmless: removeOwnedConfigState + * skips paths that no longer exist. + */ +function removeLegacyBackup(): void { + try { + unlinkSync(`${getAuthStorePath()}.pre-multiauth`); + } catch (error) { + if (errorCode(error) !== "ENOENT") { + console.warn(`[oauth] could not remove legacy credential backup: ${error instanceof Error ? error.message : String(error)}`); + } + } +} + function isCredentialSource(value: unknown): value is OAuthCredentialSource { return value === "oauth" || value === "local-cli" || value === "credential-file" || value === "environment" || value === "manual"; } @@ -712,9 +732,9 @@ function serializeMutation(work: () => Promise, retainedValues: readonly u drainOAuthMutations(); return result; } -export function mutateStore(fn:(store:AuthStore)=>T|Promise, retainedValues: readonly unknown[] = [], options?: { waitMs?: number; assertBeforePersist?: () => void }):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; assertBeforePersist?: () => void; 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 selections = new Map(Object.entries(store).map(([provider, set]) => [provider, { set, accountId: set.activeAccountId, @@ -743,6 +763,7 @@ export function mutateStore(fn:(store:AuthStore)=>T|Promise, retainedValue } } persist(store); + if (options?.removeLegacyBackup) removeLegacyBackup(); for (const provider of changedProviders) publishAccountSelection(provider, "oauth"); return result; }finally{guard.release();}}, retainedValues, options?.waitMs); @@ -886,7 +907,7 @@ export async function removeCredential(provider: string): Promise<"removed" | "n } set.activeAccountId = set.accounts[0]!.id; return "removed" as const; - }, [provider]); + }, [provider], { removeLegacyBackup: true }); } // --------------------------------------------------------------------------- @@ -1029,7 +1050,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/oauth-store-multi.test.ts b/tests/oauth/oauth-store-multi.test.ts index 32fcf293f84..6be31c1e323 100644 --- a/tests/oauth/oauth-store-multi.test.ts +++ b/tests/oauth/oauth-store-multi.test.ts @@ -213,6 +213,47 @@ describe("multi-account auth store", () => { } }); + test("logout migrates a legacy store without retaining its credential backup", async () => { + const authPath = join(TEST_DIR, "auth.json"); + mkdirSync(TEST_DIR, { recursive: true, mode: 0o700 }); + writeFileSync(authPath, JSON.stringify({ + xai: { access: "legacy-access", refresh: "legacy-refresh", expires: Date.now() + 1000 }, + })); + + expect(await removeCredential("xai")).toBe("removed"); + + 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"); + mkdirSync(TEST_DIR, { recursive: true, mode: 0o700 }); + 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("logout on a migrated store still removes a stale credential backup", async () => { + const authPath = join(TEST_DIR, "auth.json"); + mkdirSync(TEST_DIR, { recursive: true, mode: 0o700 }); + await saveCredential("xai", cred({ email: "a@example.test" })); + // A backup left over from an earlier migration holds copies of removed credentials. + writeFileSync(`${authPath}.pre-multiauth`, JSON.stringify({ xai: { access: "stale", refresh: "stale", expires: 1 } })); + + expect(await removeCredential("xai")).toBe("removed"); + + 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 From b00133c7bd3c357b10e67461b1ed9cd67fafccae Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Fri, 18 Sep 2026 06:52:04 +0900 Subject: [PATCH 2/3] docs(structure): note legacy backup removal on destructive auth mutations --- structure/overview.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/structure/overview.md b/structure/overview.md index fb5b287d5e7..32d6636bb2a 100644 --- a/structure/overview.md +++ b/structure/overview.md @@ -66,7 +66,7 @@ opencodex state root does not undo those writes. Putting native Codex back is th | Path | Owner | Notes | | --- | --- | --- | | `~/.opencodex/config.json` | opencodex | Init creates via private temp plus no-replace hard link; dashboard and explicit updates use atomic replacement. | -| `~/.opencodex/auth.json` | opencodex | OAuth tokens; not committed. Multiauth shape: `provider -> { activeAccountId, accounts[] }` (legacy single-credential values normalize on load; a one-time `auth.json.pre-multiauth` backup guards downgrades). ChatGPT scratch OAuth stays separate from the Codex account store. For multi-slot providers, credentials without `accountId`/email replace the active slot on a normal login; an explicit add-account login preserves the prior slot and appends a distinct one. Single-slot providers such as ChatGPT remain replacement-only. | +| `~/.opencodex/auth.json` | opencodex | OAuth tokens; not committed. Multiauth shape: `provider -> { activeAccountId, accounts[] }` (legacy single-credential values normalize on load; a one-time `auth.json.pre-multiauth` backup guards downgrades and is removed by destructive mutations such as logout or account deletion). ChatGPT scratch OAuth stays separate from the Codex account store. For multi-slot providers, credentials without `accountId`/email replace the active slot on a normal login; an explicit add-account login preserves the prior slot and appends a distinct one. Single-slot providers such as ChatGPT remain replacement-only. | | `~/.opencodex/codex-accounts.json` | opencodex | Hardened main-plus-added credential store used by `openai` in Pool mode. | | `~/.opencodex/catalog-backup.json` | opencodex | One-time pristine Codex catalog backup for restore; per-catalog copies are hashed variants (see [`catalog.md`](catalog.md)). | | `~/.opencodex/usage.jsonl` | opencodex | Append-only request usage log (0o600); request metadata + token counts only, never prompts or auth. | From 160d3ab0991ecd94ce09359af573b09664bb5b7a Mon Sep 17 00:00:00 2001 From: lidge-jun Date: Fri, 18 Sep 2026 09:47:50 +0900 Subject: [PATCH 3/3] fix(oauth): only drop the downgrade backup when a credential was actually removed removeCredential and removeAccount passed removeLegacyBackup unconditionally, so a stale or concurrent request that returned "not-found" or false still unlinked auth.json.pre-multiauth and still skipped creating it for a legacy store. That backup is a whole-store copy, so a removal that deleted nothing destroyed downgrade recovery for every provider in it. Decide from the mutation result instead. The decision moves to just after the mutation body, which only edits the in-memory store, so nothing has touched disk when it is taken, and the create and remove paths stay mutually exclusive as before. Cover both no-op results. --- src/oauth/store.ts | 18 +++++++++++++----- tests/oauth/oauth-store-multi.test.ts | 24 ++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/src/oauth/store.ts b/src/oauth/store.ts index c53b396fd9d..fb60041424e 100644 --- a/src/oauth/store.ts +++ b/src/oauth/store.ts @@ -732,9 +732,8 @@ function serializeMutation(work: () => Promise, retainedValues: readonly u drainOAuthMutations(); return result; } -export function mutateStore(fn:(store:AuthStore)=>T|Promise, retainedValues: readonly unknown[] = [], options?: { waitMs?: number; assertBeforePersist?: () => void; removeLegacyBackup?: boolean }):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; assertBeforePersist?: () => void; removeLegacyBackup?: boolean | ((result: T) => boolean) }):Promise{return serializeMutation(async()=>{const guard=await createOAuthFileLock({path:getAuthStoreLockPath(),staleAfterMs:30000}).acquire();try{ const { store, hadLegacy } = loadAuthStoreInternal(); - if (hadLegacy && !options?.removeLegacyBackup) backupLegacyOnce(); const selections = new Map(Object.entries(store).map(([provider, set]) => [provider, { set, accountId: set.activeAccountId, @@ -742,6 +741,15 @@ export function mutateStore(fn:(store:AuthStore)=>T|Promise, retainedValue accountIds: set.accounts.map(account => account.id), }])); const result = await fn(store); + // A destructive mutation that removed nothing must not drop the downgrade + // backup: the request was a no-op, so there is no deleted credential to + // stop retaining, and the backup is a whole-store copy for every provider. + // The decision needs the mutation's result, so it is taken here; `fn` only + // edits the in-memory store, and nothing has touched disk yet. + const dropLegacyBackup = typeof options?.removeLegacyBackup === "function" + ? options.removeLegacyBackup(result) + : options?.removeLegacyBackup === true; + if (hadLegacy && !dropLegacyBackup) backupLegacyOnce(); options?.assertBeforePersist?.(); const changedProviders: string[] = []; for (const provider of new Set([...selections.keys(), ...Object.keys(store)])) { @@ -763,7 +771,7 @@ export function mutateStore(fn:(store:AuthStore)=>T|Promise, retainedValue } } persist(store); - if (options?.removeLegacyBackup) removeLegacyBackup(); + if (dropLegacyBackup) removeLegacyBackup(); for (const provider of changedProviders) publishAccountSelection(provider, "oauth"); return result; }finally{guard.release();}}, retainedValues, options?.waitMs); @@ -907,7 +915,7 @@ export async function removeCredential(provider: string): Promise<"removed" | "n } set.activeAccountId = set.accounts[0]!.id; return "removed" as const; - }, [provider], { removeLegacyBackup: true }); + }, [provider], { removeLegacyBackup: result => result === "removed" }); } // --------------------------------------------------------------------------- @@ -1050,7 +1058,7 @@ export async function removeAccount(provider: string, accountId: string): Promis } if (set.activeAccountId === accountId) set.activeAccountId = set.accounts[0]!.id; return true; - }, [provider, accountId], { removeLegacyBackup: true }); + }, [provider, accountId], { removeLegacyBackup: removed => removed }); return removed; } diff --git a/tests/oauth/oauth-store-multi.test.ts b/tests/oauth/oauth-store-multi.test.ts index 6be31c1e323..a084d2320bc 100644 --- a/tests/oauth/oauth-store-multi.test.ts +++ b/tests/oauth/oauth-store-multi.test.ts @@ -254,6 +254,30 @@ describe("multi-account auth store", () => { expect(existsSync(`${authPath}.pre-multiauth`)).toBe(false); }); + test("a logout that removed nothing keeps the downgrade backup", async () => { + const authPath = join(TEST_DIR, "auth.json"); + mkdirSync(TEST_DIR, { recursive: true, mode: 0o700 }); + await saveCredential("xai", cred({ email: "a@example.test" })); + // The backup is a whole-store copy, so a no-op removal for one provider must + // not destroy downgrade recovery for every other provider in it. + writeFileSync(`${authPath}.pre-multiauth`, JSON.stringify({ xai: { access: "stale", refresh: "stale", expires: 1 } })); + + expect(await removeCredential("anthropic")).toBe("not-found"); + + expect(existsSync(`${authPath}.pre-multiauth`)).toBe(true); + }); + + test("an account deletion that matched nothing keeps the downgrade backup", async () => { + const authPath = join(TEST_DIR, "auth.json"); + mkdirSync(TEST_DIR, { recursive: true, mode: 0o700 }); + await saveCredential("xai", cred({ email: "a@example.test" })); + writeFileSync(`${authPath}.pre-multiauth`, JSON.stringify({ xai: { access: "stale", refresh: "stale", expires: 1 } })); + + expect(await removeAccount("xai", "no-such-account")).toBe(false); + + expect(existsSync(`${authPath}.pre-multiauth`)).toBe(true); + }); + 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