fix(seedless-onboarding-controller): fix InvalidPrimarySecretDataType for legacy accounts with client clock skew#9247
Open
huggingbot wants to merge 7 commits into
Open
fix(seedless-onboarding-controller): fix InvalidPrimarySecretDataType for legacy accounts with client clock skew#9247huggingbot wants to merge 7 commits into
huggingbot wants to merge 7 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. |
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
2 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)