Feat/pwd change failure recovery - #10148
Open
lwin-kyaw wants to merge 16 commits into
Open
Conversation
lwin-kyaw
temporarily deployed
to
default-branch
September 9, 2026 12:38 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Contributor
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
Contributor
Author
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0adf182. Configure here.
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
1 participant
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.
Explanation
Seedless password changes span multiple independently persisted states (remote Seedless/TOPRF, local Seedless vault, local KeyringController vault, the stored Keyring encryption key, and a lifecycle marker). These cannot be committed atomically, so a crash, lost response, or partial local update could previously leave the wallet in an ambiguous state where neither the old nor the new password reliably unlocks, and the client had no way to tell that recovery was needed.
This PR adds a server-first password-change recovery model to
SeedlessOnboardingController:passwordChangePhasefield (SeedlessPasswordChangePhase:SEEDLESS_CHANGE_PENDING→SEEDLESS_COMMITTED→LOCAL_KEYRING_PENDING→KEY_SYNC_PENDING, plusUNKNOWN) acts as a recovery signal. An unset/undefinedphase means no change is in progress. The phase is not proof of remote or local state — recovery always re-verifies actual state before acting.changePasswordis now lifecycle-aware: it writes each phase at the irreversible boundaries, preserves the last known phase on error, and rejects a second concurrent change withPasswordChangeInProgress.resolvePasswordSyncState({ skipCache })— password-less, called at unlock (render + submit). Replaces the publiccheckIsPasswordOutdatedread and returns aPasswordChangeRecoveryStatusthat tells the client which recovery step to run next.reconcilePassword({ globalPassword })— password-consuming. Internally runs password-chain unlock and local vault rewrite, re-encryptsencryptedKeyringEncryptionKeyunder the new wrapping key soloadKeyringEncryptionKeykeeps working, and advances toLOCAL_KEYRING_PENDING. Used both for an interrupted local password change and for a password change made on another device.markPasswordChangeKeySyncPendingafter the Keyring encryption key is stored, andclearPasswordChangePhaseonce key synchronization and local persistence are verified.clearPasswordChangePhaseis the only way back to "no change in progress".changeEncKeyresponse is classified viafetchAuthPubKeycomparison into old / new / unknown; ambiguous results stayUNKNOWNand keep the wallet locked.Breaking changes vs
main:checkIsPasswordOutdatedandSeedlessOnboardingControllerCheckIsPasswordOutdatedAction. CallresolvePasswordSyncState({ skipCache })instead (truemaps toPasswordChangeRecoveryStatus.PasswordOutdated).submitGlobalPassword,syncLatestGlobalPassword, and their messenger actions. CallreconcilePassword({ globalPassword })instead.changePasswordnow writes lifecycle phases and rejects a concurrent change withPasswordChangeInProgress. Clients must not start a second password change while a lifecycle is unfinished, and must drive it to completion withclearPasswordChangePhase.There is no awaitable durability hook on the controller for lifecycle writes — the phase is persisted as ordinary debounced controller state, so recovery re-verifies actual state (a stale/missing marker is recoverable via the outdated check + cryptographic Keyring verification). The controller does not call
KeyringController(AllowedActions = never); clients own the Keyring-side steps and wallet locking.Full design, recovery flow, and a step-by-step client integration guide are in
docs/0002-password-change-recovery-flow.md. The ADR is indocs/0001-seedless-password-change-recovery.md.Test plan
yarn workspace @metamask/seedless-onboarding-controller run testchangePasswordwritesSEEDLESS_CHANGE_PENDING→SEEDLESS_COMMITTED→LOCAL_KEYRING_PENDINGand rejects a second concurrent change withPasswordChangeInProgressresolvePasswordSyncState→reconcilePassword→ Keyring old/new branch →storeKeyringEncryptionKey/markPasswordChangeKeySyncPending→clearPasswordChangePhaseresolvePasswordSyncStatereturnspassword-outdated,reconcilePasswordreturnsreconcile-keyring, andloadKeyringEncryptionKeystill decrypts after vault rewritechangeEncKeyresult staysUNKNOWNand does not infer success from a rejected PromisecheckIsPasswordOutdated,submitGlobalPassword, orsyncLatestGlobalPasswordReferences
docs/0001-seedless-password-change-recovery.mddocs/0002-password-change-recovery-flow.mddocs/0003-controller-owned-password-change-recovery-plan.mdChecklist
Note
High Risk
Breaking changes and new recovery paths touch Seedless/TOPRF passwords, vault rewrites, and unlock routing; incorrect client adoption or remaining concurrency gaps could leave wallets locked or skip Keyring reconciliation.
Overview
Introduces a server-first password-change recovery model for Seedless: a persisted
passwordChangePhasesignals interrupted changes, and clients route unlock/recovery viaPasswordSyncStatusinstead of ad-hoc outdated checks.Breaking API: removes
checkIsPasswordOutdated,submitGlobalPassword, andsyncLatestGlobalPassword(and their messenger actions). UseresolvePasswordSyncState({ skipCache })at unlock (password-less) andreconcilePassword({ globalPassword })to run chain unlock, local vault rewrite, and re-wrapencryptedKeyringEncryptionKeyinternally.changePasswordnow writes lifecycle phases, preserves phase on failure, and throwsPasswordChangeInProgresson overlap. AddsclearPasswordChangePhase,markPasswordChangeKeySyncPending, and related actions; Keyring reconciliation stays client-owned.Fixes a high-severity race where
resolvePasswordSyncStatecould clear a newer phase after concurrentchangePasswordby acquiring the controller lock before readingpasswordChangePhase(documented in audit 0004). Ships ADRs/guides for client integration and notes remaining lock/durability follow-ups.Reviewed by Cursor Bugbot for commit ae94d42. Bugbot is set up for automated code reviews on this repo. Configure here.