Repository navigation
feat: identify social login users with incomplete metadata backup - #10568
Merged
lwin-kyaw merged 30 commits intoOct 8, 2026
Merged
Conversation
Contributor
|
Looks good for Shapes A/B/C (missing primary, V1 earliest-imported, V2 mislabeled PrimarySrp). Two asks before we treat this as SSOT: (1) fix the mermaid Yes/No on “Primary SRP Different” - it’s inverted vs the prose; (2) specify controller vs client ownership; Keyring compare + profile_id telemetry aren’t in seedless today (AllowedActions = never). Also worth noting identify can’t see users who already rehydrated onto a wrong primary |
Contributor
Author
Thanks for the comment @grvgoel81 |
…portSeedPhraseAction to the SeedlessController allowedActions list
lwin-kyaw
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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. |
grvgoel81
reviewed
Sep 30, 2026
Comment on lines
+73
to
+79
| await trackEventSafely({ | ||
| name: SeedlessPrimarySrpMismatchEventName, | ||
| properties: {}, | ||
| sensitiveProperties: {}, | ||
| saveDataRecording: false, | ||
| hasProperties: false, | ||
| }); |
Contributor
There was a problem hiding this comment.
Both SeedlessPrimarySrpMismatchEventName and SeedlessPrimarySrpMissingEventName emit with properties: {}. Without a profile_id, metaMetricsId, or keyringId, you can only count affected users, cannot identify who they are for manual remediation. This defeats the purpose of the detection for incident response.
Contributor
Author
There was a problem hiding this comment.
Without a profile_id, metaMetricsId,
We're calling the AnalyticsController:trackEvent directly, both of these will be assigned by the controller.
I've also updated with the relevant properties here dc77a1e
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
grvgoel81
reviewed
Oct 6, 2026
Comment on lines
927
to
941
| globalPassword, | ||
| maxKeyChainLength = 5, | ||
| }: { | ||
| globalPassword: string; | ||
| maxKeyChainLength?: number; | ||
| }): Promise<void> { | ||
| return await this.#withControllerLock(async () => { | ||
| return await this.#executeWithTokenRefresh(async () => { | ||
| const currentDeviceAuthPubKey = this.#recoverAuthPubKey(); | ||
| await this.#submitGlobalPassword({ | ||
| targetAuthPubKey: currentDeviceAuthPubKey, | ||
| globalPassword, | ||
| maxKeyChainLength, | ||
| }); | ||
| }, 'submitGlobalPassword'); |
Contributor
There was a problem hiding this comment.
Users who unlock via the global password path (Mobile) are never scanned. Inside #submitGlobalPassword, decryptedVaultData with its toprfEncryptionKey and toprfAuthKeyPair is available - the same ingredients used in submitPassword. The detection needs to be wired in here too.
Contributor
Author
There was a problem hiding this comment.
I've exposed a new public method, identifyIncompleteMetadataBackup to track the analytics, for this case.
We can't do this in the controller side because currently, in Password sync cases, the wallet unlock, KeyringController:submitPassword is done in the client side.
So, you should call identifyIncompleteMetadataBackup method after wallet is unlocked with global password.
Normal unlock should be still tracked by the controller internally.
Eventually, after we move every orchestration inside the controller, we will mark this method, identifyIncompleteMetadataBackup as internal private method.
grvgoel81
reviewed
Oct 6, 2026
| ); | ||
|
|
||
| // Sort: PrimarySrp first, then by client timestamp (oldest first). | ||
| results.sort((a, b) => SecretMetadata.compare(a, b, 'asc')); |
Contributor
There was a problem hiding this comment.
SecretMetadata.compare returns 0 when both items have dataType === PrimarySrp (the V2 corruption shape from this incident). That makes the relative order between them dependent on the TOPRF node fetch order, which is not stable across unlocks. findIndex then picks whichever ends up first. For a user with two PrimarySrp-tagged items where the wrong one is first, areUint8ArraysEqual in identifyIncompleteMetadataBackup returns true and the user is silently classified as healthy.
The dual-PrimarySrp case should be detected explicitly here e.g., check if more than one PrimarySrp-tagged item exists after the sort and surface that as a distinct error or return value so the caller can emit a separate analytics event for it, rather than silently picking one.
Contributor
Author
There was a problem hiding this comment.
@grvgoel81 , I've replied to this same comment here, #10568 (comment) :)
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. |
grvgoel81
previously approved these changes
Oct 7, 2026
grvgoel81
left a comment
grvgoel81
left a comment
Contributor
There was a problem hiding this comment.
Approving. Remaining action item: the Mobile side needs to wire up identifyIncompleteMetadataBackup after submitGlobalPassword unlock; global-password users won't be scanned until that call site is added.
grvgoel81
previously approved these changes
Oct 8, 2026
lwin-kyaw
enabled auto-merge
October 8, 2026 06:06
Mrtenz
reviewed
Oct 8, 2026
|
|
||
| ### Changed | ||
|
|
||
| - **BREAKING:** Delegate `AnalyticsController:trackEvent` and `KeyringController:exportSeedPhrase` from the wallet root messenger to `SeedlessOnboardingController`. ([#10568](https://github.com/MetaMask/core/pull/10568)) |
Member
There was a problem hiding this comment.
This is more in line with other changelog entries.
Suggested change
| - **BREAKING:** Delegate `AnalyticsController:trackEvent` and `KeyringController:exportSeedPhrase` from the wallet root messenger to `SeedlessOnboardingController`. ([#10568](https://github.com/MetaMask/core/pull/10568)) | |
| - **BREAKING:** Grant `SeedlessOnboardingController` access to `AnalyticsController:trackEvent` and `KeyringController:exportSeedPhrase` ([#10568](https://github.com/MetaMask/core/pull/10568)) |
Mrtenz
approved these changes
Oct 8, 2026
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 1f496f5. Configure here.
chaitanyapotti
approved these changes
Oct 8, 2026
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
4 participants
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
Social-login users may have an incomplete or incorrect TOPRF metadata backup after a partial backup failure. This change adds read-only detection after a successful unlock so affected users can be measured before a future repair flow.
After
submitPasswordsucceeds,identifyIncompleteMetadataBackupruns asynchronously and:PrimarySrptype.Seedless Onboarding Primary SRP MissingorSeedless Onboarding Primary SRP Mismatchevents.The
SeedlessOnboardingControllerMessengernow requires:AnalyticsController:trackEventKeyringController:exportSeedPhraseThe wallet initializer delegates these actions from the root messenger. The seedless onboarding controller also adds the analytics-controller dependency, exposes its
./utilsentry point, and refactors secret metadata parsing and migration helpers.References
Test plan
Checklist
Note
High Risk
Runs on unlock and calls exportSeedPhrase to compare SRPs, with breaking messenger wiring for analytics and keyring export; behavior is read-only but touches wallet recovery secrets.
Overview
Adds read-only detection for social-login wallets whose remote TOPRF backup is missing or wrong for the primary SRP, so affected users can be measured before a future repair.
After a successful
submitPasswordunlock, the controller kicks offidentifyIncompleteMetadataBackupin the background (no await). It fetches remote secret metadata, picks the primary candidate (legacy v1 ordering vs v2PrimarySrp), compares it to the local keyring SRP viaKeyringController:exportSeedPhrase, and emitsSeedless Onboarding Primary SRP MissingorMismatchthroughAnalyticsController:trackEvent. Failures are logged only; unlock is not blocked and nothing is written locally or remotely. The same flow is exposed as a public messenger action for explicit calls when unlocked.Breaking: integrators must allow and delegate
AnalyticsController:trackEventandKeyringController:exportSeedPhraseon the seedless messenger (wallet initializer updated). The package adds@metamask/analytics-controller, a./utilsexport, and moves secret-metadata parsing/migration helpers plus identification logic intoutils/(includingtrackIncompleteMetadataBackupEvents). Docs describe the identification plan for production remediation.Reviewed by Cursor Bugbot for commit 1f496f5. Bugbot is set up for automated code reviews on this repo. Configure here.