Feat/pwd change failure recovery - #10148
Open
lwin-kyaw wants to merge 13 commits into
Open
Conversation
![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.
| // keep the wallet locked. | ||
| return PasswordSyncStatus.Unknown; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Stale phase snapshot races recovery
High Severity
resolvePasswordSyncState reads passwordChangePhase before taking the controller lock. A concurrent changePassword can finish and advance local Seedless state first, so this method can still follow the SEEDLESS_CHANGE_PENDING branch, clear a later recovery phase, and return in-sync while the Keyring still needs reconciliation.
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 to password/unlock APIs and lifecycle-gated
changePasswordtouch vault crypto and multi-device sync; incorrect client migration could lock users out or leave ambiguous wallet state.Overview
Introduces a server-first password-change recovery model for Seedless: a persisted
passwordChangePhaselifecycle (non-secret recovery signal) andPasswordSyncStatusso clients can route unlock/recovery without guessing wallet state after partial failures or another-device password changes.Breaking API: removes public
checkIsPasswordOutdated,submitGlobalPassword, andsyncLatestGlobalPassword(and their messenger actions). Clients should callresolvePasswordSyncState({ skipCache })at unlock (replaces the outdated boolean;password-outdated≈ oldtrue) andreconcilePassword({ globalPassword })to run chain-unlock + local vault rewrite internally, re-wrapencryptedKeyringEncryptionKey, and advance toLOCAL_KEYRING_PENDING. AddsclearPasswordChangePhase,markPasswordChangeKeySyncPending, andPasswordChangeInProgresswhenchangePasswordruns while a lifecycle is unfinished.changePasswordnow writes phases at irreversible boundaries, preserves the last phase on error, and documents that Keyring reconciliation, locking, and finalclearPasswordChangePhaseremain client-owned (Option A). Adds ADR/integration docs (0001–0003) and changelog entries for consumers.Reviewed by Cursor Bugbot for commit 89df8df. Bugbot is set up for automated code reviews on this repo. Configure here.