feat: [MFA-581] mfa recovery controller - #10022
Draft
ieow wants to merge 4 commits into
Draft
Conversation
- Introduced the `@metamask/mfa-recovery-controller` package to manage MFA recovery flows across MetaMask clients. - Added core functionalities including methods for registering recovery secrets, updating identifiers, and handling recovery states. - Updated relevant documentation and code ownership files to include the new package. - Ensured integration with existing MetaMask infrastructure by updating configuration files and dependencies. This commit lays the groundwork for enhanced MFA recovery capabilities in MetaMask, improving user experience and security.
- Introduced a new `escrow-utils.ts` module for handling escrow-related operations, including receipt verification and selecting the highest consistent recovery secret version. - Updated `MfaRecoveryController` to require `RecoveryEscrowProvider.verifyReceipt` to accept an expected escrow ID, enhancing receipt verification. - Exposed state updates through the `MfaRecoveryController:stateChanged` event, allowing better tracking of state changes. - Improved documentation in `CHANGELOG.md` and `README.md` to reflect changes in state management and mutation handling. - Fixed issues related to clearing acknowledged mutations and validating persisted mutation states. This commit enhances the robustness of the MFA recovery process and improves the overall user experience in MetaMask.
…ization handling - Added tests to ensure recovery secret retrieval continues despite failures in escrow availability checks and authorization processes. - Updated the controller to handle cases where authorization for one escrow fails, preventing unauthorized writes and ensuring consistent recovery secret retrieval. - Refactored authorization handling to return structured results for better error management and state validation. These changes improve the robustness of the MFA recovery process, ensuring a smoother user experience in MetaMask.
Contributor
Author
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 2a3b74e. Configure here.
| identifier, | ||
| receipts: [], | ||
| }; | ||
| await this.#persistPending(pending); |
There was a problem hiding this comment.
Auth token persisted without mutation checks
Medium Severity
The controller persists a writing token from #authorizeMutation without checking profileId or requestHash against the mutation. A mismatched token later fails pending validation, so resume and abort cannot load the record they just wrote.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 2a3b74e. 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
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
Current state
MFA recovery needs to keep a recovery secret consistent across multiple escrow
replicas. A mutation can succeed at one escrow and fail at another, so the
controller needs durable progress tracking, request binding, and a safe way to
resume incomplete writes.
The original implementation also combined mutation orchestration, pending-state
validation, receipt handling, and read consistency logic in one large
controller file.
Solution
This PR introduces
@metamask/mfa-recovery-controller, which coordinatesprofile authentication, identifier authentication, recovery-secret reads, and
replication across build-configured escrow providers.
The mutation protocol now:
expected/new versions, payload hash, and escrow audiences.
authorizingstate before authorization.writingstate with the auth token and acknowledgedescrow receipts before starting writes.
receipt against its expected escrow.
resume()to retry only missing acknowledgements.before receipt persistence is safe to retry.
The controller now rejects malformed persisted operations before authorization
or escrow calls. Fully acknowledged pending mutations can be cleared without
checking availability of already-acknowledged escrows.
The large controller has been split into focused internal modules:
pending-operation-validation.tshandles runtime validation of decryptedpending state.
escrow-utils.tshandles receipt validation, settled-result narrowing, andhighest-consistent-version selection.
The public exports remain unchanged by this refactor. The
RecoveryEscrowProvider.verifyReceiptcontract intentionally requires theexpected escrow id, which is a breaking provider-interface change documented
in the changelog.
Security and trust model
Mutation authorization uses one request-bound
AuthControllerTokenfor theconfigured escrow audience set. Each escrow implementation must independently
verify the token signature, issuer, expiry, profile, exact mutation hash, and
that its own id is included in the mutation audiences.
Key-bound identifier authentication creates a separate challenge and proof for
each escrow. Escrow-challenge identifiers also complete a separate challenge
for each escrow.
The current design treats configured escrows as trusted equivalent replicas:
the recovery secret is replicated in full to each escrow. This is not an
M-of-N threshold-recovery design. A future independent-trust model would need
escrow-specific authorization scopes and escrow-specific or threshold-encrypted
secret material.
References
Related work is contained in this pull request.
Validation
yarn workspace @metamask/mfa-recovery-controller run teststub files
yarn workspace @metamask/mfa-recovery-controller run buildyarn workspace @metamask/mfa-recovery-controller run changelog:validategit diff --checkChecklist
verifyReceiptsignature changeNote
High Risk
Introduces new recovery-secret replication, auth, and escrow mutation logic with durable encrypted state—security-sensitive account recovery behavior and a breaking escrow provider interface.
Overview
Adds
@metamask/mfa-recovery-controller, a new BaseController that orchestrates MFA recovery by replicating a recovery secret across build-configured escrow replicas with injectableRecoveryAuthProviderandRecoveryEscrowProviderimplementations.Mutations run through persisted
authorizing→writingphases (encrypted pending state), exposingregister,updateRecoverySecret,updateIdentifiers,getRecoverySecret,resume, andabort.resumeretries only escrows missing receipts;abortis limited to pre-writeauthorizingstate. Receipt verification is bound to the expected escrow id via a breakingRecoveryEscrowProvider.verifyReceipt(receipt, mutation, expectedEscrowId)contract.Monorepo wiring registers the package under Web3Auth / team-onboarding (CODEOWNERS,
codeowners.ts,teams.json, root README/tsconfig,yarn.lock).Reviewed by Cursor Bugbot for commit 2a3b74e. Bugbot is set up for automated code reviews on this repo. Configure here.