Conversation
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.
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
Contributor
⏳ DRAFT
What to do
Review readiness checklist
✅ 4/4 boxes ticked. This pull request was already a draft. Its draft status will be preserved after every issue above is resolved. |
…ally 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
auth.json.pre-multiauthis a one-time downgrade-recovery copy of the whole legacy credential store, written on the first new-shape persist. Destructive mutations went through the same path:removeCredential/removeAccountleft an existing backup behind, and a still-legacy store even created the backup during the logout itself — so after the user removed an account, a file holding the very refresh tokens they destroyed remained on disk.mutateStoregains aremoveLegacyBackupoption: it skips the one-time create and unlinks the backup after the store is persisted.removeCredentialandremoveAccountopt in.ENOENTignored, other failures warn) because it runs after persist — a failed unlink must not report a failed logout for an account that is already gone. A stale uninstall-manifest entry is harmless:removeOwnedConfigStateskips missing paths, so this composes with the owned-cleanup registration from fix(oauth): register legacy recovery backups for owned cleanup #4572.replaceProviderAccountSet, provider rekey) do not opt in — they are not user-initiated credential destruction.Verification
8640e2039c4d28a52e5320d92d9a543a5e4575db(treecc1951344b636766b638741b00ca592885f0c7f0), based on deva0f611d4aceb9476d44268e43722273b7b211846.bun test tests/oauth/oauth-store-multi.test.ts— 44 pass / 0 fail, including new regressions covering destructive-migration, pre-existing-backup, and stale-backup-on-migrated-store cases.bun x tsc --noEmit— clean.bun run structure:check— passed (structure/overview.mdupdated to describe the removal).bun run privacy:scan— passed.bun scripts/file-size-ratchet.ts— passed.Remaining gates
gh workflow run ci.yml: https://github.com/luvs01/opencodex/actions/runs/35279061859. The macOS control job is expected to cancel at the 30-minute mark; that is the known upstream limitation tracked in [Bug]: macOS control is cancelled near its 30-minute limit in full dispatch CI #4905, not a signal from this change. Occasional shard flakes are likewise known upstream noise.src/oauth/, which the intake gate may classify as a restricted surface requiring maintainer sponsorship.gui/files are touched, so the UI-screenshot gate does not apply.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing. Local gates passed on this head (tsc, structure:check, privacy:scan, focused tests on the merged head).
I pushed my PR to the latest dev commit. The branch carries dev
4c0124acb.I resolved all correct Codex and CodeRabbit findings. No unresolved review threads on this head.
My PR is ready for review.