Skip to content

fix(oauth): drop legacy credential backup on destructive mutations - #4949

Draft
luvs01 wants to merge 4 commits into
lidge-jun:devfrom
luvs01:agent/oauth-legacy-backup-cleanup-20260918
Draft

luvs01 wants to merge 4 commits into
lidge-jun:devfrom
luvs01:agent/oauth-legacy-backup-cleanup-20260918

Conversation

@luvs01

@luvs01 luvs01 commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Summary

  • auth.json.pre-multiauth is 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/removeAccount left 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.
  • mutateStore gains a removeLegacyBackup option: it skips the one-time create and unlinks the backup after the store is persisted. removeCredential and removeAccount opt in.
  • Removal is best-effort (ENOENT ignored, 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: removeOwnedConfigState skips missing paths, so this composes with the owned-cleanup registration from fix(oauth): register legacy recovery backups for owned cleanup #4572.
  • Automatic rollback paths (replaceProviderAccountSet, provider rekey) do not opt in — they are not user-initiated credential destruction.

Verification

  • Head: 8640e2039c4d28a52e5320d92d9a543a5e4575db (tree cc1951344b636766b638741b00ca592885f0c7f0), based on dev a0f611d4aceb9476d44268e43722273b7b211846.
  • 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.md updated to describe the removal).
  • bun run privacy:scan — passed.
  • bun scripts/file-size-ratchet.ts — passed.

Remaining gates

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

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.

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.
@coderabbitai

coderabbitai Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added bug Something isn't working intake: hygiene-blocked Deterministic PR hygiene checks failed labels Sep 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Deterministic hygiene checks failed.

  • unsponsored_surface — This changes an authentication, workflow, release-automation, or dependency surface. MAINTAINERS.md requires security review for these; ask a maintainer to apply maintainer-sponsored once they have reviewed it. Paths: src/oauth/store.ts.

@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • hygiene: unsponsored_surface.

What to do

  • Fix unsponsored_surface — This changes an authentication, workflow, release-automation, or dependency surface. MAINTAINERS.md requires security review for these; ask a maintainer to apply maintainer-sponsored once they have reviewed it. Paths: src/oauth/store.ts.

Review readiness checklist

  • ✅ All CI tests are green on my local testing.
  • ✅ I pushed my PR to the latest dev commit.
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ✅ My PR is ready for review.

4/4 boxes ticked.

This pull request was already a draft. Its draft status will be preserved after every issue above is resolved.

lidge-jun and others added 2 commits September 18, 2026 09:47
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working intake: hygiene-blocked Deterministic PR hygiene checks failed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants