fix(seedless-onboarding-controller): fix InvalidPrimarySecretDataType for legacy accounts with client clock skew#9247
Queued
huggingbot wants to merge 16 commits into
Queued
fix(seedless-onboarding-controller): fix InvalidPrimarySecretDataType for legacy accounts with client clock skew#9247huggingbot wants to merge 16 commits into
huggingbot wants to merge 16 commits into
Conversation
huggingbot
temporarily deployed
to
default-branch
GitHub Actions
Inactive
huggingbot
changed the title
…y SRP fix Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… scenarios Trace the InvalidPrimarySecretDataType crash to legacy data + clock skew rather than the dataType migration code: - legacy primary + legacy private key with skew -> private key in slot 0 (the crash) - old device adds a private key today -> server createdAt keeps primary in slot 0 - legacy private key still crashes even with a newer createdAt-tagged key present - untagged primary behind a private key with earlier server createdAt (no skew needed) - once migration tags PrimarySrp, it always sorts to slot 0 (the cure) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…enarios Cover collections that mix primary SRP, imported SRP, and imported private keys written across legacy / old-device / v2 eras: - legacy private key in slot 0 with the real primary still promotable -> fix recovers - tagged imported SRP in slot 0 ahead of an untagged primary -> old code crashes, fix recovers - two legacy mnemonics -> fix recovers but may promote the imported SRP (documents the inherent ambiguity, same heuristic the dataType migration uses) - tagged PrimarySrp wins slot 0 over legacy SRPs and an earliest-timestamp legacy PK - no mnemonic present -> fix fails closed (still throws) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tic root cause Drop scenarios that violate the "primary is created first" invariant (a non-primary item with an earlier createdAt than the primary, a tagged imported SRP ahead of an untagged primary, a tagged primary beside untagged legacy siblings) — these are unreachable given addNewSecretData rejects PrimarySrp and the migration tags the whole collection atomically. Model the pre-PR validation (results[0] only) to drill into the root cause: - baseline (no skew): all-legacy collection -> primary in slot 0, old code accepts - root cause (crash): all-legacy private key skewed into slot 0 -> old code throws - root cause (silent wrong primary): all-legacy imported SRP skewed into slot 0 -> old code accepts it as the primary (legacy mnemonics carry no dataType) - not the root cause: an item with a real createdAt cannot precede the legacy primary - not the root cause: a migration-tagged PrimarySrp wins slot 0 regardless of skew Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
@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. |
huggingbot
force-pushed
the
fix/seedless-promote-primary-srp
branch
from
d187862 to
e808444
Compare
lwin-kyaw
reviewed
Jun 25, 2026
| // indistinguishable — we take the oldest (best-effort). | ||
| const primaryIndex = results.findIndex((item) => { | ||
| const isDataTypePrimary = | ||
| item.dataType === undefined || |
Contributor
There was a problem hiding this comment.
Legacy data has type field, privateKey data could have the item.type="privatekey", so we should add one more condition here item.type !== 'privatekey'.
Contributor
There was a problem hiding this comment.
Sorry, my bad. Wasn't aware of this, SecretMetadata.matchesType(item, SecretType.Mnemonic).
lwin-kyaw
previously approved these changes
Jun 25, 2026
lwin-kyaw
reviewed
Jun 25, 2026
| oldAuthKeyPair: authKeyPair, | ||
| newKeyShareIndex: globalKeyIndex, | ||
| newPassword, | ||
| existingDataItems, |
Contributor
There was a problem hiding this comment.
There's a slim chance of race condition here.
After we fetched and sorted the existing data items and before we acquire the metadata lock, user could add new secret data and the existingDataItems isn't updated anymore.
This could cause the data loss or crash (if we don't handle decryption very well).
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
lwin-kyaw
approved these changes
Jun 25, 2026
Any commits made after this event will not be merged.
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
#fetchAllSecretDataFromMetadataStoresorted results then assumedresults[0]was the primary SRP. This assumption breaks for legacy accounts where:dataTypemigration have nocreatedAt(server-assigned TIMEUUID), so ordering falls back to the client-sidetimestamp.toUnixTimestamp(NOW()), millisecond resolution), so collisions were possible.timestampsorts intoresults[0], failing theisMnemoniccheck and throwingInvalidPrimarySecretDataType.Changes
Instead of hard-failing on
results[0], scan the full sorted list for the first item whosedataTypeisPrimarySrpor unset AND whoseSecretTypeisMnemonic. Promote that item to index 0 so callers continue to work correctly.Limitation:
SecretType.Mnemoniccovers both primary and imported SRPs — thedataTypeplaintext server field is the only discriminator, and legacy items lack it. If multiple legacy untagged mnemonics exist, the true primary is indistinguishable; we take the oldest by sort order (same heuristic thedataTypemigration uses). For users already tagged (dataType = PrimarySrp), the sort puts them first anyway so thefindIndexshort-circuits immediately.Only throws
InvalidPrimarySecretDataTypewhen no mnemonic candidate exists at all — a genuinely unrecoverable state.References
should promote the primary SRP to first when a legacy non-mnemonic sorts ahead of it (clock skew)Checklist
yarn jest SeedlessOnboardingController)