Feat/pwd change failure recovery - #10148
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@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". |
|
@metamaskbot publish-preview |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
|
@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 2 potential issues.
❌ 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 d53243c. Configure here.
| await this.#persistKeyringEncryptionKey(keyringEncryptionKey); | ||
| } | ||
|
|
||
| this.#resetPasswordOutdatedCache(); |
There was a problem hiding this comment.
Key wrap breaks recovery retry
High Severity
#syncLatestGlobalPasswordInner rewrites the Seedless vault (and persisted authPubKey) before re-wrapping encryptedKeyringEncryptionKey. After that first persist, the wrapping key in the vault no longer matches the stored ciphertext. A crash or failure before the later persist leaves loadKeyringEncryptionKey unable to decrypt, and a later #runPasswordSyncFlow retry hits the same decrypt against the already-new vault, so the documented idempotent SEEDLESS_COMMITTED / LOCAL_KEYRING_PENDING recovery cannot restore the Keyring encryption key.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit d53243c. Configure here.
| this.#writePasswordChangePhase( | ||
| SeedlessPasswordChangePhase.LocalKeyringPending, | ||
| ); | ||
| return PasswordSyncStatus.ReconcileKeyring; |
There was a problem hiding this comment.
Reconcile drops another-device signal
High Severity
reconcilePassword with no phase rewrites the local Seedless vault and authPubKey before writing LOCAL_KEYRING_PENDING. If that phase write is lost, resolvePasswordSyncState sees matching local and remote authPubKey values, reports in-sync, and (for SEEDLESS_CHANGE_PENDING) even clears the phase. The client then skips Keyring recovery even though the local Keyring is still on the old password.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d53243c. Configure here.
1 participant
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
Changes authentication/password sync, vault rewrite, and breaking public APIs; incorrect client migration could block wallet unlock or leave partial password-change state unresolved.
Overview
Adds a server-first password-change recovery model to
@metamask/seedless-onboarding-controllerso interrupted or cross-device Seedless password changes can be resumed safely instead of leaving the wallet ambiguous.New persisted recovery signal: optional
passwordChangePhase(SeedlessPasswordChangePhase) plusPasswordSyncStatusreturned from unlock-time routing.changePasswordnow advances phases at irreversible steps, keeps the last phase on failure, and throwsPasswordChangeInProgresson concurrent changes.New public API:
resolvePasswordSyncState({ skipCache })(password-less unlock routing),reconcilePassword({ globalPassword })(internal chain unlock + local vault rewrite + key re-wrap), and lifecycle helpersmarkPasswordChangeKeySyncPending/clearPasswordChangePhase. Messenger action types are updated accordingly.Breaking removals:
checkIsPasswordOutdated,submitGlobalPassword, andsyncLatestGlobalPassword(and their actions)—clients should use the new resolve/reconcile flow. Keyring-side recovery stays in clients (Option A); ADR 0001, integration guide 0002, and future Option B plan 0003 document the contract.Reviewed by Cursor Bugbot for commit d53243c. Bugbot is set up for automated code reviews on this repo. Configure here.