diff --git a/src/oauth/store.ts b/src/oauth/store.ts index a7ea6eaa2df..fb60041424e 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,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 }):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) backupLegacyOnce(); const selections = new Map(Object.entries(store).map(([provider, set]) => [provider, { set, accountId: set.activeAccountId, @@ -722,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)])) { @@ -743,6 +771,7 @@ export function mutateStore(fn:(store:AuthStore)=>T|Promise, retainedValue } } persist(store); + if (dropLegacyBackup) removeLegacyBackup(); for (const provider of changedProviders) publishAccountSelection(provider, "oauth"); return result; }finally{guard.release();}}, retainedValues, options?.waitMs); @@ -886,7 +915,7 @@ export async function removeCredential(provider: string): Promise<"removed" | "n } set.activeAccountId = set.accounts[0]!.id; return "removed" as const; - }, [provider]); + }, [provider], { removeLegacyBackup: result => result === "removed" }); } // --------------------------------------------------------------------------- @@ -1029,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]); + }, [provider, accountId], { removeLegacyBackup: removed => removed }); return removed; } 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. | diff --git a/tests/oauth/oauth-store-multi.test.ts b/tests/oauth/oauth-store-multi.test.ts index 32fcf293f84..a084d2320bc 100644 --- a/tests/oauth/oauth-store-multi.test.ts +++ b/tests/oauth/oauth-store-multi.test.ts @@ -213,6 +213,71 @@ 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("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