fix(profile-sync-controller): scope storage-key & snap-signature caches by entropySourceId#8948
Merged
Merged
Conversation
mathieuartu
temporarily deployed
to
default-branch
GitHub Actions
Inactive
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. |
ccharly
requested changes
Jun 2, 2026
| message: `metamask:${string}`, | ||
| entropySourceId?: string, | ||
| ): string { | ||
| return `${entropySourceId ?? 'primary'}:${message}`; |
Contributor
There was a problem hiding this comment.
Just wondering, if we somehow start using the primary keyring with its explicit entropySourceId, then it would be both under primary:... and <primary-entropy-source-id>:... scoped keys?
That's probably ok?
Otherwise, we could just fallback to the real primary entropy source ID too?
Contributor
There was a problem hiding this comment.
Another question that came to mind is, during a "reset wallet" flow, are we clearing this cache?
If during that flow we start using a new SRP, this would become the new primary, but I think the Snap message we put in cache here, would change with a different SRP, so we need to invalidate this cache somehow.
(I haven't checked thoroughly this controller, just pointing this out so we can double-check we're not invalid cached value 😅)
Member
There was a problem hiding this comment.
since this was mentioned, can we double check if #_snapSignMessageCache needs to be cleaned/flushed at some point?
gantunesr
reviewed
Jun 2, 2026
ccharly
approved these changes
Jun 3, 2026
Contributor
ccharly
left a comment
ccharly
left a comment
There was a problem hiding this comment.
Logic LGTM now, thanks for addressing that!
The only remark I might have is that we have no way to flush the #snapSignMessageCache cache. Though, given that we now use truly random ID (entropy source ID) even for the primary, we should never ever have have conflicting keys (+ this will get flushed upon client restart anyway).
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
3 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
References
Related to: https://consensyssoftware.atlassian.net/browse/MUL-1861
Test drive extension PR: MetaMask/metamask-extension#43110
Checklist
Note
High Risk
Fixes incorrect cross-SRP cache sharing that could decrypt or write another SRP's backup/sync data when profile IDs align; touches encryption key derivation and snap signing paths.
Overview
Fixes a multi-SRP / shared-profile cache collision where
UserStorageControllerkeyed storage-key and snap-signature caches only bymetamask:${profileId}. When two SRPs shared the sameprofileId(e.g. after canonical profile pairing), one SRP could reuse the other’s cached derived key and read or write the wrong encrypted user storage (including symptoms like a second SRP showing the first’s account names).UserStorageControllernow scopes both caches with#scopedCacheKeyas`${entropySourceId}:${message}`(primary uses the first HD keyring metadata id, not a fixed"primary"string). Storage callbacks passentropySourceIdthrough;getStorageKeyreturns null while locked; snap signing uses the same scoped cache.user-storageSDKStorageOptionsandgetStorageKeypropagate optionalentropySourceIdon get/set cache. Tests cover vault restore (new entropy id → new key), signature reuse after storage-key flush, and distinct API paths per entropy source with identicalprofileId. Changelog documents the fix; test fixtures include a default HD keyring for primary scope.Reviewed by Cursor Bugbot for commit a1edb9a. Bugbot is set up for automated code reviews on this repo. Configure here.