From b3f28286183e2b5c2c6445c06be1ae0f00aff87d Mon Sep 17 00:00:00 2001 From: huggingbot <83656073+huggingbot@users.noreply.github.com> Date: Wed, 24 Jun 2026 12:57:12 +0800 Subject: [PATCH 01/16] feat: promote primary SRP in secret data fetch to handle legacy items --- .../src/SeedlessOnboardingController.test.ts | 52 +++++++++++++++++++ .../src/SeedlessOnboardingController.ts | 38 ++++++++++---- 2 files changed, 79 insertions(+), 11 deletions(-) diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts index bc1406e3e6..542cf5ae91 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts @@ -3472,6 +3472,58 @@ describe('SeedlessOnboardingController', () => { }, ); }); + + it('should promote the primary SRP to first when a legacy non-mnemonic sorts ahead of it (clock skew)', async () => { + await withController( + { + state: getMockInitialControllerState({ + withMockAuthenticatedUser: true, + }), + }, + async ({ controller, toprfClient }) => { + mockRecoverEncKey(toprfClient, MOCK_PASSWORD); + + // Legacy data (no dataType, no createdAt). An imported private key + // has the earliest client timestamp (e.g. due to clock skew on the + // device that imported it), so it sorts ahead of the primary SRP. + // The fetch must still recover by promoting the primary SRP, not + // throw InvalidPrimarySecretDataType. + const mockSecretDataGet = handleMockSecretDataGet({ + status: 200, + body: createMockSecretDataGetResponse( + [ + { + data: new Uint8Array(Buffer.from('importedPk', 'utf-8')), + timestamp: 100, + type: SecretType.PrivateKey, + itemId: 'imported-pk-id', + // No dataType or createdAt (legacy) + }, + { + data: new Uint8Array(Buffer.from('primarySrp', 'utf-8')), + timestamp: 200, + type: SecretType.Mnemonic, + itemId: 'primary-srp-id', + // No dataType or createdAt (legacy) + }, + ], + MOCK_PASSWORD, + ), + }); + + const secretData = await controller.fetchAllSecretData(MOCK_PASSWORD); + + expect(mockSecretDataGet.isDone()).toBe(true); + expect(secretData).toHaveLength(2); + // Primary SRP (mnemonic) promoted to first despite later timestamp. + expect(secretData[0].type).toBe(SecretType.Mnemonic); + expect(secretData[0].data).toStrictEqual(stringToBytes('primarySrp')); + // The private key is preserved, not dropped. + expect(secretData[1].type).toBe(SecretType.PrivateKey); + expect(secretData[1].data).toStrictEqual(stringToBytes('importedPk')); + }, + ); + }); }); describe('submitPassword', () => { diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts index 91020b33f2..6ec7f2e203 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts @@ -1646,22 +1646,38 @@ export class SeedlessOnboardingController< // Sort: PrimarySrp first, then by createdAt/timestamp (oldest first) results.sort((a, b) => SecretMetadata.compare(a, b, 'asc')); - // Validate the first item is the primary SRP - const firstItem = results[0]; - const isDataTypePrimary = - firstItem.dataType === undefined || - firstItem.dataType === null || - firstItem.dataType === EncAccountDataType.PrimarySrp; - const isMnemonic = SecretMetadata.matchesType( - firstItem, - SecretType.Mnemonic, - ); + // Find the primary SRP instead of assuming it is at index 0: legacy + // items have no `createdAt`, so ordering falls back to the client + // `timestamp`, which clock skew can sort a private key ahead of. + // A candidate is a mnemonic whose `dataType` is `PrimarySrp` or unset. + // `dataType` (a plaintext server field) is the only thing separating + // primary from imported SRP; legacy items lack it and the encrypted + // `SecretType` groups both, so among legacy mnemonics the primary is + // indistinguishable — we take the oldest (best-effort). + const primaryIndex = results.findIndex((item) => { + const isDataTypePrimary = + item.dataType === undefined || + item.dataType === null || + item.dataType === EncAccountDataType.PrimarySrp; + return ( + isDataTypePrimary && + SecretMetadata.matchesType(item, SecretType.Mnemonic) + ); + }); - if (!isDataTypePrimary || !isMnemonic) { + // No recoverable primary SRP exists in the metadata store. + if (primaryIndex === -1) { throw new Error( SeedlessOnboardingControllerErrorMessage.InvalidPrimarySecretDataType, ); } + + // Ensure the primary SRP is first; callers rely on `results[0]` being it. + if (primaryIndex > 0) { + const [primary] = results.splice(primaryIndex, 1); + results.unshift(primary); + } + return results; } From 311dc7975219dbd4df1a23c2e3447120075b3785 Mon Sep 17 00:00:00 2001 From: huggingbot <83656073+huggingbot@users.noreply.github.com> Date: Wed, 24 Jun 2026 13:17:24 +0800 Subject: [PATCH 02/16] chore(seedless-onboarding-controller): add changelog entry for primary SRP fix Co-Authored-By: Claude Sonnet 4.6 --- packages/seedless-onboarding-controller/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/seedless-onboarding-controller/CHANGELOG.md b/packages/seedless-onboarding-controller/CHANGELOG.md index a991b2ac4a..caa52e47fa 100644 --- a/packages/seedless-onboarding-controller/CHANGELOG.md +++ b/packages/seedless-onboarding-controller/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Fix `InvalidPrimarySecretDataType` thrown for legacy accounts where client clock skew sorts a non-mnemonic item ahead of the primary SRP ([#9247](https://github.com/MetaMask/core/pull/9247)) + ### Changed - Bump `@metamask/utils` from `^11.9.0` to `^11.11.0` ([#9074](https://github.com/MetaMask/core/pull/9074)) From 45701500a026b37fee9613f9be2cadc7afe1db8d Mon Sep 17 00:00:00 2001 From: huggingbot <83656073+huggingbot@users.noreply.github.com> Date: Wed, 24 Jun 2026 13:21:24 +0800 Subject: [PATCH 03/16] chore(seedless-onboarding-controller): fix changelog section order Co-Authored-By: Claude Sonnet 4.6 --- packages/seedless-onboarding-controller/CHANGELOG.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/seedless-onboarding-controller/CHANGELOG.md b/packages/seedless-onboarding-controller/CHANGELOG.md index caa52e47fa..9dcb4b2a63 100644 --- a/packages/seedless-onboarding-controller/CHANGELOG.md +++ b/packages/seedless-onboarding-controller/CHANGELOG.md @@ -7,15 +7,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -### Fixed - -- Fix `InvalidPrimarySecretDataType` thrown for legacy accounts where client clock skew sorts a non-mnemonic item ahead of the primary SRP ([#9247](https://github.com/MetaMask/core/pull/9247)) - ### Changed - Bump `@metamask/utils` from `^11.9.0` to `^11.11.0` ([#9074](https://github.com/MetaMask/core/pull/9074)) - Bump `@metamask/keyring-controller` from `^27.0.0` to `^27.1.0` ([#9129](https://github.com/MetaMask/core/pull/9129)) +### Fixed + +- Fix `InvalidPrimarySecretDataType` thrown for legacy accounts where client clock skew sorts a non-mnemonic item ahead of the primary SRP ([#9247](https://github.com/MetaMask/core/pull/9247)) + ## [10.0.2] ### Changed From 19df57a7a53617d39525e9ea2ce6e963fa9b36a1 Mon Sep 17 00:00:00 2001 From: huggingbot <83656073+huggingbot@users.noreply.github.com> Date: Wed, 24 Jun 2026 16:32:25 +0800 Subject: [PATCH 04/16] test(seedless-onboarding-controller): add compare slot-0 reproduction 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 --- .../src/SeedlessOnboardingController.test.ts | 132 ++++++++++++++++++ 1 file changed, 132 insertions(+) diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts index 542cf5ae91..dbcf587d58 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts @@ -4691,6 +4691,138 @@ describe('SeedlessOnboardingController', () => { }); }); + // These tests trace the `InvalidPrimarySecretDataType` crash back to its + // root cause by checking which item lands in slot 0 after the same sort the + // controller runs before validating. The crash happens whenever the primary + // SRP is NOT in slot 0, which only occurs when it lacks the PrimarySrp tag. + describe('compare - primary SRP slot-0 scenarios', () => { + // Three valid TIMEUUIDs in ascending server-assigned order (T1 < T2 < T3). + const T1 = '00000001-0000-1000-8000-000000000001'; + const T2 = '00000002-0000-1000-8000-000000000002'; + const T3 = '00000003-0000-1000-8000-000000000003'; + + // Mirror how the controller orders fetched items before validating slot 0. + const sortAsc = ( + items: SecretMetadata[], + ): SecretMetadata[] => + [...items].sort((a, b) => SecretMetadata.compare(a, b, 'asc')); + + it('reproduces the bug: a legacy primary and legacy private key with a skewed clock sorts the private key into slot 0', () => { + // Pure pre-migration data: neither item has a server `createdAt` or a + // `dataType` tag, so ordering falls back to the client `timestamp`. A + // skewed clock stamped the private key earlier than the primary SRP. + // No migration code runs here — this is purely legacy data + skew. + const primarySrp = new SecretMetadata(MOCK_SEED_PHRASE, { + type: SecretType.Mnemonic, + timestamp: 2000, + }); + const importedPk = new SecretMetadata(MOCK_SEED_PHRASE, { + type: SecretType.PrivateKey, + timestamp: 1000, // skewed earlier + }); + + const sorted = sortAsc([primarySrp, importedPk]); + + // Slot 0 is a private key, not the primary SRP -> the crash. + expect(sorted[0].type).toBe(SecretType.PrivateKey); + expect(sorted[1].type).toBe(SecretType.Mnemonic); + }); + + it('does NOT reproduce when an old device adds a private key today: the server `createdAt` sorts it after the legacy primary', () => { + // Device A (old version, no migration code) adds a private key now. It + // cannot send `dataType`, but the server still stamps `created_at = NOW()`. + // The legacy primary has a null `createdAt`, which the comparator treats + // as older, so the primary stays in slot 0 and Device B does not crash. + const legacyPrimarySrp = new SecretMetadata(MOCK_SEED_PHRASE, { + type: SecretType.Mnemonic, + timestamp: 2000, + // no createdAt (legacy) + }); + const newlyAddedPk = new SecretMetadata(MOCK_SEED_PHRASE, { + type: SecretType.PrivateKey, + createdAt: T1, // server-assigned on insert + timestamp: 1000, // skewed earlier, but irrelevant once createdAt exists + }); + + const sorted = sortAsc([legacyPrimarySrp, newlyAddedPk]); + + expect(sorted[0].type).toBe(SecretType.Mnemonic); + expect(sorted[1].type).toBe(SecretType.PrivateKey); + }); + + it('reproduces the bug from a legacy private key even when a newer private key (with `createdAt`) is also present', () => { + // A freshly added private key (real `createdAt`) does not cause the + // crash; the pre-existing legacy private key + skew does. The new item + // just sorts last and is irrelevant to slot 0. + const legacyPrimarySrp = new SecretMetadata(MOCK_SEED_PHRASE, { + type: SecretType.Mnemonic, + timestamp: 2000, + }); + const legacyPk = new SecretMetadata(MOCK_SEED_PHRASE, { + type: SecretType.PrivateKey, + timestamp: 1000, // skewed earlier + }); + const newPk = new SecretMetadata(MOCK_SEED_PHRASE, { + type: SecretType.PrivateKey, + createdAt: T1, + timestamp: 500, + }); + + const sorted = sortAsc([legacyPrimarySrp, legacyPk, newPk]); + + // Legacy items (null createdAt) come first, ordered by timestamp; the + // skewed legacy private key lands in slot 0. + expect(sorted[0].type).toBe(SecretType.PrivateKey); + expect(sorted[0].createdAt).toBeUndefined(); + expect(sorted[1].type).toBe(SecretType.Mnemonic); + expect(sorted[2].createdAt).toBe(T1); + }); + + it('reproduces the bug without clock skew: an untagged primary sorts behind a private key with an earlier server `createdAt`', () => { + // Both items have a real `createdAt` but neither is tagged PrimarySrp + // (no migration has run). If the private key was stamped before the + // primary on the server, it wins slot 0 regardless of client timestamp. + // Shows the crash is not exclusively a client-clock problem. + const untaggedPrimarySrp = new SecretMetadata(MOCK_SEED_PHRASE, { + type: SecretType.Mnemonic, + createdAt: T2, // stamped later + timestamp: 2000, + }); + const importedPk = new SecretMetadata(MOCK_SEED_PHRASE, { + type: SecretType.PrivateKey, + createdAt: T1, // stamped earlier + timestamp: 1000, + }); + + const sorted = sortAsc([untaggedPrimarySrp, importedPk]); + + expect(sorted[0].type).toBe(SecretType.PrivateKey); + expect(sorted[1].type).toBe(SecretType.Mnemonic); + }); + + it('is fixed once the dataType migration tags the primary: PrimarySrp always sorts to slot 0', () => { + // After migration the primary carries dataType=PrimarySrp, which the + // comparator ranks first unconditionally, so clock skew no longer + // matters. The migration is the cure, not the cause, of the crash. + const taggedPrimarySrp = new SecretMetadata(MOCK_SEED_PHRASE, { + dataType: EncAccountDataType.PrimarySrp, + createdAt: T3, + timestamp: 2000, + }); + const importedPk = new SecretMetadata(MOCK_SEED_PHRASE, { + dataType: EncAccountDataType.ImportedPrivateKey, + createdAt: T1, + timestamp: 1000, // skewed earlier + }); + + const sorted = sortAsc([importedPk, taggedPrimarySrp]); + + expect(sorted[0].type).toBe(SecretType.Mnemonic); + expect(sorted[0].dataType).toBe(EncAccountDataType.PrimarySrp); + expect(sorted[1].type).toBe(SecretType.PrivateKey); + }); + }); + it('should default type to Mnemonic when parsing metadata without type field', () => { // Create raw metadata JSON without type field const rawMetadataWithoutType = JSON.stringify({ From 713c2f5d80bd4f8bf51c91582c2f42ee098d0377 Mon Sep 17 00:00:00 2001 From: huggingbot <83656073+huggingbot@users.noreply.github.com> Date: Wed, 24 Jun 2026 16:51:16 +0800 Subject: [PATCH 05/16] test(seedless-onboarding-controller): add mixed-collection compare scenarios 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 --- .../src/SeedlessOnboardingController.test.ts | 156 +++++++++++++++++- 1 file changed, 155 insertions(+), 1 deletion(-) diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts index dbcf587d58..b38a7453ac 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts @@ -4696,10 +4696,11 @@ describe('SeedlessOnboardingController', () => { // controller runs before validating. The crash happens whenever the primary // SRP is NOT in slot 0, which only occurs when it lacks the PrimarySrp tag. describe('compare - primary SRP slot-0 scenarios', () => { - // Three valid TIMEUUIDs in ascending server-assigned order (T1 < T2 < T3). + // Valid TIMEUUIDs in ascending server-assigned order (T1 < T2 < T3 < T4). const T1 = '00000001-0000-1000-8000-000000000001'; const T2 = '00000002-0000-1000-8000-000000000002'; const T3 = '00000003-0000-1000-8000-000000000003'; + const T4 = '00000004-0000-1000-8000-000000000004'; // Mirror how the controller orders fetched items before validating slot 0. const sortAsc = ( @@ -4707,6 +4708,17 @@ describe('SeedlessOnboardingController', () => { ): SecretMetadata[] => [...items].sort((a, b) => SecretMetadata.compare(a, b, 'asc')); + // Mirror the controller's promote predicate: a mnemonic whose `dataType` + // is PrimarySrp or unset. The old code crashed unless slot 0 matched this; + // the fix scans for the first item that does and promotes it. + const isPrimaryCandidate = ( + item: SecretMetadata, + ): boolean => + (item.dataType === undefined || + item.dataType === null || + item.dataType === EncAccountDataType.PrimarySrp) && + SecretMetadata.matchesType(item, SecretType.Mnemonic); + it('reproduces the bug: a legacy primary and legacy private key with a skewed clock sorts the private key into slot 0', () => { // Pure pre-migration data: neither item has a server `createdAt` or a // `dataType` tag, so ordering falls back to the client `timestamp`. A @@ -4821,6 +4833,148 @@ describe('SeedlessOnboardingController', () => { expect(sorted[0].dataType).toBe(EncAccountDataType.PrimarySrp); expect(sorted[1].type).toBe(SecretType.PrivateKey); }); + + it('mixed eras: a legacy private key takes slot 0 while the real primary (old device, real createdAt) stays promotable', () => { + // Collection spans all three eras at once. The legacy private key has a + // null createdAt, so it sorts first and the old code crashes — but the + // first mnemonic candidate is the actual primary, so the fix recovers it. + const primaryData = stringToBytes('primary'); + const legacyImportedPk = new SecretMetadata(stringToBytes('pk'), { + type: SecretType.PrivateKey, + timestamp: 100, + // legacy: null createdAt + }); + const oldDevicePrimary = new SecretMetadata(primaryData, { + type: SecretType.Mnemonic, // old device cannot send dataType + createdAt: T2, // but the server stamped a real createdAt + timestamp: 500, + }); + const v2ImportedSrp = new SecretMetadata(stringToBytes('impSrp'), { + dataType: EncAccountDataType.ImportedSrp, // new device + createdAt: T3, + timestamp: 200, + }); + + const sorted = sortAsc([ + oldDevicePrimary, + legacyImportedPk, + v2ImportedSrp, + ]); + + expect(sorted[0].type).toBe(SecretType.PrivateKey); + expect(isPrimaryCandidate(sorted[0])).toBe(false); // old code crashes + expect(sorted.find(isPrimaryCandidate)?.data).toStrictEqual(primaryData); + }); + + it('mixed eras: a tagged imported SRP can take slot 0 ahead of an untagged primary, and the fix still finds the primary', () => { + // Slot 0 IS a mnemonic, but its dataType is ImportedSrp, so the old + // code crashes anyway. The fix skips it and promotes the real primary. + const primaryData = stringToBytes('primary'); + const v2ImportedSrp = new SecretMetadata(stringToBytes('impSrp'), { + dataType: EncAccountDataType.ImportedSrp, + createdAt: T1, + timestamp: 100, + }); + const oldDevicePrimary = new SecretMetadata(primaryData, { + type: SecretType.Mnemonic, // untagged + createdAt: T2, + timestamp: 500, + }); + + const sorted = sortAsc([oldDevicePrimary, v2ImportedSrp]); + + expect(sorted[0].type).toBe(SecretType.Mnemonic); + expect(sorted[0].dataType).toBe(EncAccountDataType.ImportedSrp); + expect(isPrimaryCandidate(sorted[0])).toBe(false); // old code crashes + expect(sorted.find(isPrimaryCandidate)?.data).toStrictEqual(primaryData); + }); + + it('mixed legacy mnemonics: without a dataType to separate primary from imported SRP, the fix recovers but may promote the wrong mnemonic', () => { + // Two legacy mnemonics: both are untagged Mnemonics, so neither the + // comparator nor the promote logic can tell the primary from the + // imported SRP. The fix avoids the crash but picks the first mnemonic + // by timestamp (the imported SRP here) — the same best-effort heuristic + // the dataType migration uses, and the same ambiguity the old code had. + const primaryData = stringToBytes('primary'); + const importedSrpData = stringToBytes('impSrp'); + const legacyPrimary = new SecretMetadata(primaryData, { + type: SecretType.Mnemonic, + timestamp: 300, // skewed latest + }); + const legacyImportedSrp = new SecretMetadata(importedSrpData, { + type: SecretType.Mnemonic, + timestamp: 200, + }); + const legacyImportedPk = new SecretMetadata(stringToBytes('pk'), { + type: SecretType.PrivateKey, + timestamp: 100, // skewed earliest + }); + + const sorted = sortAsc([ + legacyPrimary, + legacyImportedSrp, + legacyImportedPk, + ]); + + expect(sorted[0].type).toBe(SecretType.PrivateKey); // old code crashes + // The promoted mnemonic is the imported SRP, NOT the true primary. + expect(sorted.find(isPrimaryCandidate)?.data).toStrictEqual( + importedSrpData, + ); + expect(sorted.find(isPrimaryCandidate)?.data).not.toStrictEqual( + primaryData, + ); + }); + + it('mixed collection: a tagged PrimarySrp wins slot 0 over legacy imported SRPs and an earliest-timestamp legacy private key', () => { + const primaryData = stringToBytes('primary'); + const taggedPrimary = new SecretMetadata(primaryData, { + dataType: EncAccountDataType.PrimarySrp, + createdAt: T4, + timestamp: 999, + }); + const legacyImportedSrp1 = new SecretMetadata(stringToBytes('srp1'), { + type: SecretType.Mnemonic, + timestamp: 100, + }); + const legacyImportedSrp2 = new SecretMetadata(stringToBytes('srp2'), { + type: SecretType.Mnemonic, + timestamp: 200, + }); + const legacyImportedPk = new SecretMetadata(stringToBytes('pk'), { + type: SecretType.PrivateKey, + timestamp: 50, // earliest, but the PrimarySrp tag beats timestamp + }); + + const sorted = sortAsc([ + legacyImportedSrp1, + legacyImportedPk, + taggedPrimary, + legacyImportedSrp2, + ]); + + expect(sorted[0].data).toStrictEqual(primaryData); + expect(sorted[0].dataType).toBe(EncAccountDataType.PrimarySrp); + expect(isPrimaryCandidate(sorted[0])).toBe(true); + }); + + it('no mnemonic present: the fix fails closed instead of promoting a private key', () => { + // Only private keys exist (one legacy, one v2). There is no primary + // candidate, so the controller correctly throws InvalidPrimarySecretDataType. + const legacyPk = new SecretMetadata(stringToBytes('pk1'), { + type: SecretType.PrivateKey, + timestamp: 100, + }); + const v2Pk = new SecretMetadata(stringToBytes('pk2'), { + dataType: EncAccountDataType.ImportedPrivateKey, + createdAt: T1, + timestamp: 200, + }); + + const sorted = sortAsc([legacyPk, v2Pk]); + + expect(sorted.find(isPrimaryCandidate)).toBeUndefined(); + }); }); it('should default type to Mnemonic when parsing metadata without type field', () => { From 196a323a2ebd8f4bfb893be0957b93a77ae2d4eb Mon Sep 17 00:00:00 2001 From: huggingbot <83656073+huggingbot@users.noreply.github.com> Date: Wed, 24 Jun 2026 17:07:37 +0800 Subject: [PATCH 06/16] test(seedless-onboarding-controller): refocus compare tests on realistic root cause MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../src/SeedlessOnboardingController.test.ts | 331 ++++++------------ 1 file changed, 104 insertions(+), 227 deletions(-) diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts index b38a7453ac..05b5c9530b 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts @@ -4691,289 +4691,166 @@ describe('SeedlessOnboardingController', () => { }); }); - // These tests trace the `InvalidPrimarySecretDataType` crash back to its - // root cause by checking which item lands in slot 0 after the same sort the - // controller runs before validating. The crash happens whenever the primary - // SRP is NOT in slot 0, which only occurs when it lacks the PrimarySrp tag. - describe('compare - primary SRP slot-0 scenarios', () => { - // Valid TIMEUUIDs in ascending server-assigned order (T1 < T2 < T3 < T4). + // These tests drill into the ROOT CAUSE of the `InvalidPrimarySecretDataType` + // crash as the code behaved BEFORE this PR: the controller sorted the fetched + // items, then assumed `results[0]` was the primary SRP and threw unless slot 0 + // was a mnemonic tagged PrimarySrp or untagged. + // + // Invariant that bounds the realistic cases: the primary SRP is created at + // onboarding before any imported item (`addNewSecretData` rejects PrimarySrp), + // so it always has the earliest server `createdAt` — or null if it predates + // the `created_at` column. A non-primary item therefore can never have an + // earlier `createdAt` than the primary, and `#migrateDataTypes` tags the whole + // collection at once. So the primary only loses slot 0 when it is a legacy + // (null `createdAt`) item, another legacy item exists, and client clock skew + // reorders them by the fallback client `timestamp`. Any item with a real + // `createdAt`, or a tagged item, can never sort ahead of the primary. + describe('compare - primary SRP slot-0 root cause (pre-fix behavior)', () => { + // A server-assigned TIMEUUID. Only ever appears on items created after the + // primary, so an item carrying one always sorts after the legacy primary. const T1 = '00000001-0000-1000-8000-000000000001'; - const T2 = '00000002-0000-1000-8000-000000000002'; - const T3 = '00000003-0000-1000-8000-000000000003'; - const T4 = '00000004-0000-1000-8000-000000000004'; - // Mirror how the controller orders fetched items before validating slot 0. const sortAsc = ( items: SecretMetadata[], ): SecretMetadata[] => [...items].sort((a, b) => SecretMetadata.compare(a, b, 'asc')); - // Mirror the controller's promote predicate: a mnemonic whose `dataType` - // is PrimarySrp or unset. The old code crashed unless slot 0 matched this; - // the fix scans for the first item that does and promotes it. - const isPrimaryCandidate = ( - item: SecretMetadata, - ): boolean => - (item.dataType === undefined || - item.dataType === null || - item.dataType === EncAccountDataType.PrimarySrp) && - SecretMetadata.matchesType(item, SecretType.Mnemonic); - - it('reproduces the bug: a legacy primary and legacy private key with a skewed clock sorts the private key into slot 0', () => { - // Pure pre-migration data: neither item has a server `createdAt` or a - // `dataType` tag, so ordering falls back to the client `timestamp`. A - // skewed clock stamped the private key earlier than the primary SRP. - // No migration code runs here — this is purely legacy data + skew. - const primarySrp = new SecretMetadata(MOCK_SEED_PHRASE, { - type: SecretType.Mnemonic, - timestamp: 2000, - }); - const importedPk = new SecretMetadata(MOCK_SEED_PHRASE, { - type: SecretType.PrivateKey, - timestamp: 1000, // skewed earlier - }); - - const sorted = sortAsc([primarySrp, importedPk]); - - // Slot 0 is a private key, not the primary SRP -> the crash. - expect(sorted[0].type).toBe(SecretType.PrivateKey); - expect(sorted[1].type).toBe(SecretType.Mnemonic); - }); + // Model the pre-PR validation: the old code looked ONLY at results[0] and + // threw InvalidPrimarySecretDataType unless it was a mnemonic whose + // `dataType` was PrimarySrp or unset. Returns true if the old code accepted + // slot 0 (no crash), false if it threw. + const oldCodeAcceptsSlot0 = ( + sorted: SecretMetadata[], + ): boolean => { + const first = sorted[0]; + const dataTypeOk = + first.dataType === undefined || + first.dataType === null || + first.dataType === EncAccountDataType.PrimarySrp; + return ( + dataTypeOk && SecretMetadata.matchesType(first, SecretType.Mnemonic) + ); + }; - it('does NOT reproduce when an old device adds a private key today: the server `createdAt` sorts it after the legacy primary', () => { - // Device A (old version, no migration code) adds a private key now. It - // cannot send `dataType`, but the server still stamps `created_at = NOW()`. - // The legacy primary has a null `createdAt`, which the comparator treats - // as older, so the primary stays in slot 0 and Device B does not crash. - const legacyPrimarySrp = new SecretMetadata(MOCK_SEED_PHRASE, { + it('baseline (no skew): an all-legacy collection sorts the primary into slot 0 and the old code accepts it', () => { + // The primary is created first, so without skew it has the earliest + // client timestamp and sorts first across a legacy primary + imported + // SRP + imported private key. + const primaryData = stringToBytes('primary'); + const primarySrp = new SecretMetadata(primaryData, { type: SecretType.Mnemonic, - timestamp: 2000, - // no createdAt (legacy) - }); - const newlyAddedPk = new SecretMetadata(MOCK_SEED_PHRASE, { - type: SecretType.PrivateKey, - createdAt: T1, // server-assigned on insert - timestamp: 1000, // skewed earlier, but irrelevant once createdAt exists + timestamp: 100, }); - - const sorted = sortAsc([legacyPrimarySrp, newlyAddedPk]); - - expect(sorted[0].type).toBe(SecretType.Mnemonic); - expect(sorted[1].type).toBe(SecretType.PrivateKey); - }); - - it('reproduces the bug from a legacy private key even when a newer private key (with `createdAt`) is also present', () => { - // A freshly added private key (real `createdAt`) does not cause the - // crash; the pre-existing legacy private key + skew does. The new item - // just sorts last and is irrelevant to slot 0. - const legacyPrimarySrp = new SecretMetadata(MOCK_SEED_PHRASE, { + const importedSrp = new SecretMetadata(stringToBytes('impSrp'), { type: SecretType.Mnemonic, - timestamp: 2000, + timestamp: 200, }); - const legacyPk = new SecretMetadata(MOCK_SEED_PHRASE, { + const importedPk = new SecretMetadata(stringToBytes('pk'), { type: SecretType.PrivateKey, - timestamp: 1000, // skewed earlier - }); - const newPk = new SecretMetadata(MOCK_SEED_PHRASE, { - type: SecretType.PrivateKey, - createdAt: T1, - timestamp: 500, + timestamp: 300, }); - const sorted = sortAsc([legacyPrimarySrp, legacyPk, newPk]); + const sorted = sortAsc([primarySrp, importedSrp, importedPk]); - // Legacy items (null createdAt) come first, ordered by timestamp; the - // skewed legacy private key lands in slot 0. - expect(sorted[0].type).toBe(SecretType.PrivateKey); - expect(sorted[0].createdAt).toBeUndefined(); - expect(sorted[1].type).toBe(SecretType.Mnemonic); - expect(sorted[2].createdAt).toBe(T1); + expect(sorted[0].data).toStrictEqual(primaryData); + expect(oldCodeAcceptsSlot0(sorted)).toBe(true); }); - it('reproduces the bug without clock skew: an untagged primary sorts behind a private key with an earlier server `createdAt`', () => { - // Both items have a real `createdAt` but neither is tagged PrimarySrp - // (no migration has run). If the private key was stamped before the - // primary on the server, it wins slot 0 regardless of client timestamp. - // Shows the crash is not exclusively a client-clock problem. - const untaggedPrimarySrp = new SecretMetadata(MOCK_SEED_PHRASE, { + it('root cause (crash): an all-legacy private key with a skewed-earlier timestamp sorts into slot 0 and the old code throws', () => { + // Both items are legacy (null createdAt), so ordering falls back to the + // client timestamp. The private key was imported on a skewed clock and + // sorts ahead of the primary -> slot 0 is a private key -> the old code + // throws InvalidPrimarySecretDataType. + const primarySrp = new SecretMetadata(stringToBytes('primary'), { type: SecretType.Mnemonic, - createdAt: T2, // stamped later - timestamp: 2000, + timestamp: 300, }); - const importedPk = new SecretMetadata(MOCK_SEED_PHRASE, { + const importedPk = new SecretMetadata(stringToBytes('pk'), { type: SecretType.PrivateKey, - createdAt: T1, // stamped earlier - timestamp: 1000, + timestamp: 100, // skewed earlier }); - const sorted = sortAsc([untaggedPrimarySrp, importedPk]); + const sorted = sortAsc([primarySrp, importedPk]); expect(sorted[0].type).toBe(SecretType.PrivateKey); - expect(sorted[1].type).toBe(SecretType.Mnemonic); + // -> InvalidPrimarySecretDataType + expect(oldCodeAcceptsSlot0(sorted)).toBe(false); }); - it('is fixed once the dataType migration tags the primary: PrimarySrp always sorts to slot 0', () => { - // After migration the primary carries dataType=PrimarySrp, which the - // comparator ranks first unconditionally, so clock skew no longer - // matters. The migration is the cure, not the cause, of the crash. - const taggedPrimarySrp = new SecretMetadata(MOCK_SEED_PHRASE, { - dataType: EncAccountDataType.PrimarySrp, - createdAt: T3, - timestamp: 2000, - }); - const importedPk = new SecretMetadata(MOCK_SEED_PHRASE, { - dataType: EncAccountDataType.ImportedPrivateKey, - createdAt: T1, - timestamp: 1000, // skewed earlier - }); - - const sorted = sortAsc([importedPk, taggedPrimarySrp]); - - expect(sorted[0].type).toBe(SecretType.Mnemonic); - expect(sorted[0].dataType).toBe(EncAccountDataType.PrimarySrp); - expect(sorted[1].type).toBe(SecretType.PrivateKey); - }); - - it('mixed eras: a legacy private key takes slot 0 while the real primary (old device, real createdAt) stays promotable', () => { - // Collection spans all three eras at once. The legacy private key has a - // null createdAt, so it sorts first and the old code crashes — but the - // first mnemonic candidate is the actual primary, so the fix recovers it. + it('root cause (silent wrong primary): an all-legacy imported SRP skewed into slot 0 is accepted as if it were the primary', () => { + // When the item skewed into slot 0 is an imported SRP (still a mnemonic), + // the old code does NOT crash — it silently treats the imported SRP as + // the primary. Legacy mnemonics carry no `dataType`, so primary and + // imported SRP are indistinguishable. This is a second failure mode of + // the same root cause. const primaryData = stringToBytes('primary'); - const legacyImportedPk = new SecretMetadata(stringToBytes('pk'), { - type: SecretType.PrivateKey, - timestamp: 100, - // legacy: null createdAt + const importedSrpData = stringToBytes('impSrp'); + const primarySrp = new SecretMetadata(primaryData, { + type: SecretType.Mnemonic, + timestamp: 300, }); - const oldDevicePrimary = new SecretMetadata(primaryData, { - type: SecretType.Mnemonic, // old device cannot send dataType - createdAt: T2, // but the server stamped a real createdAt - timestamp: 500, + const importedSrp = new SecretMetadata(importedSrpData, { + type: SecretType.Mnemonic, + timestamp: 100, // skewed earliest }); - const v2ImportedSrp = new SecretMetadata(stringToBytes('impSrp'), { - dataType: EncAccountDataType.ImportedSrp, // new device - createdAt: T3, + const importedPk = new SecretMetadata(stringToBytes('pk'), { + type: SecretType.PrivateKey, timestamp: 200, }); - const sorted = sortAsc([ - oldDevicePrimary, - legacyImportedPk, - v2ImportedSrp, - ]); - - expect(sorted[0].type).toBe(SecretType.PrivateKey); - expect(isPrimaryCandidate(sorted[0])).toBe(false); // old code crashes - expect(sorted.find(isPrimaryCandidate)?.data).toStrictEqual(primaryData); - }); - - it('mixed eras: a tagged imported SRP can take slot 0 ahead of an untagged primary, and the fix still finds the primary', () => { - // Slot 0 IS a mnemonic, but its dataType is ImportedSrp, so the old - // code crashes anyway. The fix skips it and promotes the real primary. - const primaryData = stringToBytes('primary'); - const v2ImportedSrp = new SecretMetadata(stringToBytes('impSrp'), { - dataType: EncAccountDataType.ImportedSrp, - createdAt: T1, - timestamp: 100, - }); - const oldDevicePrimary = new SecretMetadata(primaryData, { - type: SecretType.Mnemonic, // untagged - createdAt: T2, - timestamp: 500, - }); - - const sorted = sortAsc([oldDevicePrimary, v2ImportedSrp]); + const sorted = sortAsc([primarySrp, importedSrp, importedPk]); + // Slot 0 is a mnemonic, so the old code accepts it... expect(sorted[0].type).toBe(SecretType.Mnemonic); - expect(sorted[0].dataType).toBe(EncAccountDataType.ImportedSrp); - expect(isPrimaryCandidate(sorted[0])).toBe(false); // old code crashes - expect(sorted.find(isPrimaryCandidate)?.data).toStrictEqual(primaryData); + expect(oldCodeAcceptsSlot0(sorted)).toBe(true); + // ...but it is the imported SRP, not the real primary. + expect(sorted[0].data).toStrictEqual(importedSrpData); + expect(sorted[0].data).not.toStrictEqual(primaryData); }); - it('mixed legacy mnemonics: without a dataType to separate primary from imported SRP, the fix recovers but may promote the wrong mnemonic', () => { - // Two legacy mnemonics: both are untagged Mnemonics, so neither the - // comparator nor the promote logic can tell the primary from the - // imported SRP. The fix avoids the crash but picks the first mnemonic - // by timestamp (the imported SRP here) — the same best-effort heuristic - // the dataType migration uses, and the same ambiguity the old code had. + it('not the root cause: an item with a real server createdAt cannot precede the legacy primary, so skew on it is harmless', () => { + // A private key added after onboarding gets a server `createdAt`. The + // legacy primary has a null createdAt, which the comparator treats as + // older, so the primary keeps slot 0 regardless of the key's skewed + // client timestamp. This is why only legacy-vs-legacy collisions break. const primaryData = stringToBytes('primary'); - const importedSrpData = stringToBytes('impSrp'); const legacyPrimary = new SecretMetadata(primaryData, { type: SecretType.Mnemonic, - timestamp: 300, // skewed latest - }); - const legacyImportedSrp = new SecretMetadata(importedSrpData, { - type: SecretType.Mnemonic, - timestamp: 200, + timestamp: 2000, + // legacy: null createdAt }); - const legacyImportedPk = new SecretMetadata(stringToBytes('pk'), { + const laterImportedPk = new SecretMetadata(stringToBytes('pk'), { type: SecretType.PrivateKey, - timestamp: 100, // skewed earliest + createdAt: T1, // server-assigned when added after onboarding + timestamp: 1000, // skewed earlier, but irrelevant once createdAt exists }); - const sorted = sortAsc([ - legacyPrimary, - legacyImportedSrp, - legacyImportedPk, - ]); + const sorted = sortAsc([legacyPrimary, laterImportedPk]); - expect(sorted[0].type).toBe(SecretType.PrivateKey); // old code crashes - // The promoted mnemonic is the imported SRP, NOT the true primary. - expect(sorted.find(isPrimaryCandidate)?.data).toStrictEqual( - importedSrpData, - ); - expect(sorted.find(isPrimaryCandidate)?.data).not.toStrictEqual( - primaryData, - ); + expect(sorted[0].data).toStrictEqual(primaryData); + expect(oldCodeAcceptsSlot0(sorted)).toBe(true); }); - it('mixed collection: a tagged PrimarySrp wins slot 0 over legacy imported SRPs and an earliest-timestamp legacy private key', () => { + it('not the root cause: once the dataType migration tags the primary, PrimarySrp wins slot 0 regardless of skew', () => { + // After migration the primary carries dataType=PrimarySrp, which the + // comparator ranks first unconditionally, so a skewed-earlier imported + // private key no longer reaches slot 0. The migration is the cure. const primaryData = stringToBytes('primary'); const taggedPrimary = new SecretMetadata(primaryData, { dataType: EncAccountDataType.PrimarySrp, - createdAt: T4, - timestamp: 999, - }); - const legacyImportedSrp1 = new SecretMetadata(stringToBytes('srp1'), { - type: SecretType.Mnemonic, - timestamp: 100, - }); - const legacyImportedSrp2 = new SecretMetadata(stringToBytes('srp2'), { - type: SecretType.Mnemonic, - timestamp: 200, + createdAt: T1, + timestamp: 2000, }); - const legacyImportedPk = new SecretMetadata(stringToBytes('pk'), { - type: SecretType.PrivateKey, - timestamp: 50, // earliest, but the PrimarySrp tag beats timestamp + const importedPk = new SecretMetadata(stringToBytes('pk'), { + dataType: EncAccountDataType.ImportedPrivateKey, + timestamp: 1000, // skewed earlier, but the PrimarySrp tag wins }); - const sorted = sortAsc([ - legacyImportedSrp1, - legacyImportedPk, - taggedPrimary, - legacyImportedSrp2, - ]); + const sorted = sortAsc([importedPk, taggedPrimary]); expect(sorted[0].data).toStrictEqual(primaryData); expect(sorted[0].dataType).toBe(EncAccountDataType.PrimarySrp); - expect(isPrimaryCandidate(sorted[0])).toBe(true); - }); - - it('no mnemonic present: the fix fails closed instead of promoting a private key', () => { - // Only private keys exist (one legacy, one v2). There is no primary - // candidate, so the controller correctly throws InvalidPrimarySecretDataType. - const legacyPk = new SecretMetadata(stringToBytes('pk1'), { - type: SecretType.PrivateKey, - timestamp: 100, - }); - const v2Pk = new SecretMetadata(stringToBytes('pk2'), { - dataType: EncAccountDataType.ImportedPrivateKey, - createdAt: T1, - timestamp: 200, - }); - - const sorted = sortAsc([legacyPk, v2Pk]); - - expect(sorted.find(isPrimaryCandidate)).toBeUndefined(); + expect(oldCodeAcceptsSlot0(sorted)).toBe(true); }); }); From e80844493258f50ce9093899359e7510816d8c7d Mon Sep 17 00:00:00 2001 From: huggingbot <83656073+huggingbot@users.noreply.github.com> Date: Wed, 24 Jun 2026 17:29:07 +0800 Subject: [PATCH 07/16] test(seedless-onboarding-controller): remove root-cause simulation tests Co-Authored-By: Claude Sonnet 4.6 --- .../src/SeedlessOnboardingController.test.ts | 163 ------------------ 1 file changed, 163 deletions(-) diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts index 05b5c9530b..542cf5ae91 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts @@ -4691,169 +4691,6 @@ describe('SeedlessOnboardingController', () => { }); }); - // These tests drill into the ROOT CAUSE of the `InvalidPrimarySecretDataType` - // crash as the code behaved BEFORE this PR: the controller sorted the fetched - // items, then assumed `results[0]` was the primary SRP and threw unless slot 0 - // was a mnemonic tagged PrimarySrp or untagged. - // - // Invariant that bounds the realistic cases: the primary SRP is created at - // onboarding before any imported item (`addNewSecretData` rejects PrimarySrp), - // so it always has the earliest server `createdAt` — or null if it predates - // the `created_at` column. A non-primary item therefore can never have an - // earlier `createdAt` than the primary, and `#migrateDataTypes` tags the whole - // collection at once. So the primary only loses slot 0 when it is a legacy - // (null `createdAt`) item, another legacy item exists, and client clock skew - // reorders them by the fallback client `timestamp`. Any item with a real - // `createdAt`, or a tagged item, can never sort ahead of the primary. - describe('compare - primary SRP slot-0 root cause (pre-fix behavior)', () => { - // A server-assigned TIMEUUID. Only ever appears on items created after the - // primary, so an item carrying one always sorts after the legacy primary. - const T1 = '00000001-0000-1000-8000-000000000001'; - - const sortAsc = ( - items: SecretMetadata[], - ): SecretMetadata[] => - [...items].sort((a, b) => SecretMetadata.compare(a, b, 'asc')); - - // Model the pre-PR validation: the old code looked ONLY at results[0] and - // threw InvalidPrimarySecretDataType unless it was a mnemonic whose - // `dataType` was PrimarySrp or unset. Returns true if the old code accepted - // slot 0 (no crash), false if it threw. - const oldCodeAcceptsSlot0 = ( - sorted: SecretMetadata[], - ): boolean => { - const first = sorted[0]; - const dataTypeOk = - first.dataType === undefined || - first.dataType === null || - first.dataType === EncAccountDataType.PrimarySrp; - return ( - dataTypeOk && SecretMetadata.matchesType(first, SecretType.Mnemonic) - ); - }; - - it('baseline (no skew): an all-legacy collection sorts the primary into slot 0 and the old code accepts it', () => { - // The primary is created first, so without skew it has the earliest - // client timestamp and sorts first across a legacy primary + imported - // SRP + imported private key. - const primaryData = stringToBytes('primary'); - const primarySrp = new SecretMetadata(primaryData, { - type: SecretType.Mnemonic, - timestamp: 100, - }); - const importedSrp = new SecretMetadata(stringToBytes('impSrp'), { - type: SecretType.Mnemonic, - timestamp: 200, - }); - const importedPk = new SecretMetadata(stringToBytes('pk'), { - type: SecretType.PrivateKey, - timestamp: 300, - }); - - const sorted = sortAsc([primarySrp, importedSrp, importedPk]); - - expect(sorted[0].data).toStrictEqual(primaryData); - expect(oldCodeAcceptsSlot0(sorted)).toBe(true); - }); - - it('root cause (crash): an all-legacy private key with a skewed-earlier timestamp sorts into slot 0 and the old code throws', () => { - // Both items are legacy (null createdAt), so ordering falls back to the - // client timestamp. The private key was imported on a skewed clock and - // sorts ahead of the primary -> slot 0 is a private key -> the old code - // throws InvalidPrimarySecretDataType. - const primarySrp = new SecretMetadata(stringToBytes('primary'), { - type: SecretType.Mnemonic, - timestamp: 300, - }); - const importedPk = new SecretMetadata(stringToBytes('pk'), { - type: SecretType.PrivateKey, - timestamp: 100, // skewed earlier - }); - - const sorted = sortAsc([primarySrp, importedPk]); - - expect(sorted[0].type).toBe(SecretType.PrivateKey); - // -> InvalidPrimarySecretDataType - expect(oldCodeAcceptsSlot0(sorted)).toBe(false); - }); - - it('root cause (silent wrong primary): an all-legacy imported SRP skewed into slot 0 is accepted as if it were the primary', () => { - // When the item skewed into slot 0 is an imported SRP (still a mnemonic), - // the old code does NOT crash — it silently treats the imported SRP as - // the primary. Legacy mnemonics carry no `dataType`, so primary and - // imported SRP are indistinguishable. This is a second failure mode of - // the same root cause. - const primaryData = stringToBytes('primary'); - const importedSrpData = stringToBytes('impSrp'); - const primarySrp = new SecretMetadata(primaryData, { - type: SecretType.Mnemonic, - timestamp: 300, - }); - const importedSrp = new SecretMetadata(importedSrpData, { - type: SecretType.Mnemonic, - timestamp: 100, // skewed earliest - }); - const importedPk = new SecretMetadata(stringToBytes('pk'), { - type: SecretType.PrivateKey, - timestamp: 200, - }); - - const sorted = sortAsc([primarySrp, importedSrp, importedPk]); - - // Slot 0 is a mnemonic, so the old code accepts it... - expect(sorted[0].type).toBe(SecretType.Mnemonic); - expect(oldCodeAcceptsSlot0(sorted)).toBe(true); - // ...but it is the imported SRP, not the real primary. - expect(sorted[0].data).toStrictEqual(importedSrpData); - expect(sorted[0].data).not.toStrictEqual(primaryData); - }); - - it('not the root cause: an item with a real server createdAt cannot precede the legacy primary, so skew on it is harmless', () => { - // A private key added after onboarding gets a server `createdAt`. The - // legacy primary has a null createdAt, which the comparator treats as - // older, so the primary keeps slot 0 regardless of the key's skewed - // client timestamp. This is why only legacy-vs-legacy collisions break. - const primaryData = stringToBytes('primary'); - const legacyPrimary = new SecretMetadata(primaryData, { - type: SecretType.Mnemonic, - timestamp: 2000, - // legacy: null createdAt - }); - const laterImportedPk = new SecretMetadata(stringToBytes('pk'), { - type: SecretType.PrivateKey, - createdAt: T1, // server-assigned when added after onboarding - timestamp: 1000, // skewed earlier, but irrelevant once createdAt exists - }); - - const sorted = sortAsc([legacyPrimary, laterImportedPk]); - - expect(sorted[0].data).toStrictEqual(primaryData); - expect(oldCodeAcceptsSlot0(sorted)).toBe(true); - }); - - it('not the root cause: once the dataType migration tags the primary, PrimarySrp wins slot 0 regardless of skew', () => { - // After migration the primary carries dataType=PrimarySrp, which the - // comparator ranks first unconditionally, so a skewed-earlier imported - // private key no longer reaches slot 0. The migration is the cure. - const primaryData = stringToBytes('primary'); - const taggedPrimary = new SecretMetadata(primaryData, { - dataType: EncAccountDataType.PrimarySrp, - createdAt: T1, - timestamp: 2000, - }); - const importedPk = new SecretMetadata(stringToBytes('pk'), { - dataType: EncAccountDataType.ImportedPrivateKey, - timestamp: 1000, // skewed earlier, but the PrimarySrp tag wins - }); - - const sorted = sortAsc([importedPk, taggedPrimary]); - - expect(sorted[0].data).toStrictEqual(primaryData); - expect(sorted[0].dataType).toBe(EncAccountDataType.PrimarySrp); - expect(oldCodeAcceptsSlot0(sorted)).toBe(true); - }); - }); - it('should default type to Mnemonic when parsing metadata without type field', () => { // Create raw metadata JSON without type field const rawMetadataWithoutType = JSON.stringify({ From b197251396676639619606144d4d89883593c0ca Mon Sep 17 00:00:00 2001 From: huggingbot <83656073+huggingbot@users.noreply.github.com> Date: Thu, 25 Jun 2026 15:23:00 +0800 Subject: [PATCH 08/16] refactor(seedless-onboarding-controller): simplify secret metadata comparison logic --- .../src/SecretMetadata.ts | 19 +-- .../src/SeedlessOnboardingController.test.ts | 149 ++++++++---------- .../src/SeedlessOnboardingController.ts | 53 +++---- .../src/utils.test.ts | 63 -------- .../src/utils.ts | 47 ------ 5 files changed, 98 insertions(+), 233 deletions(-) diff --git a/packages/seedless-onboarding-controller/src/SecretMetadata.ts b/packages/seedless-onboarding-controller/src/SecretMetadata.ts index ccc46d8e68..43c4897d22 100644 --- a/packages/seedless-onboarding-controller/src/SecretMetadata.ts +++ b/packages/seedless-onboarding-controller/src/SecretMetadata.ts @@ -12,7 +12,7 @@ import { SecretType, } from './constants'; import type { SecretDataType } from './types'; -import { compareTimeuuid, getSecretTypeFromDataType } from './utils'; +import { getSecretTypeFromDataType } from './utils'; type ISecretMetadata = { data: DataType; @@ -183,9 +183,7 @@ export class SecretMetadata< * * Ordering priority: * 1. PrimarySrp always comes first (regardless of order direction) - * 2. Server-side createdAt (TIMEUUID) if both have it - * 3. Legacy items (null createdAt) are considered older - * 4. Fall back to client-side timestamp + * 2. Fall back to client-side timestamp * * @param a - The first SecretMetadata instance. * @param b - The second SecretMetadata instance. @@ -209,18 +207,7 @@ export class SecretMetadata< if (bIsPrimary) { return 1; } - // Use server-side createdAt if available (TIMEUUID requires timestamp extraction) - if (a.createdAt && b.createdAt) { - return compareTimeuuid(a.createdAt, b.createdAt, order); - } - // Handle mixed createdAt: legacy items (null) are older - if (!a.createdAt && b.createdAt) { - return order === 'asc' ? -1 : 1; // a (legacy/older) comes before b in asc - } - if (a.createdAt && !b.createdAt) { - return order === 'asc' ? 1 : -1; // b (legacy/older) comes before a in asc - } - // Both null: fall back to client-side timestamp + // Fall back to client-side timestamp return SecretMetadata.compareByTimestamp(a, b, order); } diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts index 542cf5ae91..b43de7cec4 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts @@ -454,6 +454,22 @@ function mockChangeEncKey( const pwEncKey = mockToprfEncryptor.derivePwEncKey(newPassword); const authKeyPair = mockToprfEncryptor.deriveAuthKeyPair(newPassword); + // #changeEncryptionKey fetches items before re-inserting to sort them first + jest.spyOn(toprfClient, 'fetchAllSecretDataItems').mockResolvedValue([ + { + data: stringToBytes( + JSON.stringify({ + data: bytesToBase64(MOCK_SEED_PHRASE), + timestamp: 1000, + }), + ), + itemId: 'srp-1', + version: 'v1' as const, + dataType: undefined, + createdAt: undefined, + }, + ]); + jest.spyOn(toprfClient, 'changeEncKey').mockResolvedValueOnce({ encKey, pwEncKey, @@ -3473,57 +3489,6 @@ describe('SeedlessOnboardingController', () => { ); }); - it('should promote the primary SRP to first when a legacy non-mnemonic sorts ahead of it (clock skew)', async () => { - await withController( - { - state: getMockInitialControllerState({ - withMockAuthenticatedUser: true, - }), - }, - async ({ controller, toprfClient }) => { - mockRecoverEncKey(toprfClient, MOCK_PASSWORD); - - // Legacy data (no dataType, no createdAt). An imported private key - // has the earliest client timestamp (e.g. due to clock skew on the - // device that imported it), so it sorts ahead of the primary SRP. - // The fetch must still recover by promoting the primary SRP, not - // throw InvalidPrimarySecretDataType. - const mockSecretDataGet = handleMockSecretDataGet({ - status: 200, - body: createMockSecretDataGetResponse( - [ - { - data: new Uint8Array(Buffer.from('importedPk', 'utf-8')), - timestamp: 100, - type: SecretType.PrivateKey, - itemId: 'imported-pk-id', - // No dataType or createdAt (legacy) - }, - { - data: new Uint8Array(Buffer.from('primarySrp', 'utf-8')), - timestamp: 200, - type: SecretType.Mnemonic, - itemId: 'primary-srp-id', - // No dataType or createdAt (legacy) - }, - ], - MOCK_PASSWORD, - ), - }); - - const secretData = await controller.fetchAllSecretData(MOCK_PASSWORD); - - expect(mockSecretDataGet.isDone()).toBe(true); - expect(secretData).toHaveLength(2); - // Primary SRP (mnemonic) promoted to first despite later timestamp. - expect(secretData[0].type).toBe(SecretType.Mnemonic); - expect(secretData[0].data).toStrictEqual(stringToBytes('primarySrp')); - // The private key is preserved, not dropped. - expect(secretData[1].type).toBe(SecretType.PrivateKey); - expect(secretData[1].data).toStrictEqual(stringToBytes('importedPk')); - }, - ); - }); }); describe('submitPassword', () => { @@ -4630,34 +4595,6 @@ describe('SeedlessOnboardingController', () => { ); }); - it('should sort legacy items (null createdAt) before items with createdAt in asc order', () => { - const legacyItem = new SecretMetadata(MOCK_SEED_PHRASE, { - timestamp: 2000, - dataType: EncAccountDataType.ImportedSrp, - // no createdAt (legacy) - }); - const newItem = new SecretMetadata(MOCK_SEED_PHRASE, { - timestamp: 1000, - dataType: EncAccountDataType.ImportedSrp, - createdAt: '00000001-0000-1000-8000-000000000001', - }); - - expect(SecretMetadata.compare(legacyItem, newItem, 'asc')).toBeLessThan( - 0, - ); - expect( - SecretMetadata.compare(newItem, legacyItem, 'asc'), - ).toBeGreaterThan(0); - // In desc order, legacy item comes after new item - expect( - SecretMetadata.compare(legacyItem, newItem, 'desc'), - ).toBeGreaterThan(0); - // In desc order, new item comes before legacy item - expect( - SecretMetadata.compare(newItem, legacyItem, 'desc'), - ).toBeLessThan(0); - }); - it('should fall back to timestamp when both have null createdAt', () => { const earlier = new SecretMetadata(MOCK_SEED_PHRASE, { timestamp: 1000, @@ -6099,6 +6036,24 @@ describe('SeedlessOnboardingController', () => { // Mock the recover enc key mockRecoverEncKey(toprfClient, MOCK_PASSWORD); + // #changeEncryptionKey fetches items before re-inserting to sort them first + jest + .spyOn(toprfClient, 'fetchAllSecretDataItems') + .mockResolvedValue([ + { + data: stringToBytes( + JSON.stringify({ + data: bytesToBase64(MOCK_SEED_PHRASE), + timestamp: 1000, + }), + ), + itemId: 'srp-1', + version: 'v1' as const, + dataType: undefined, + createdAt: undefined, + }, + ]); + // Mock changeEncKey to fail first with token expired error, then succeed const mockToprfEncryptor = createMockToprfEncryptor(); const newEncKey = @@ -6184,6 +6139,24 @@ describe('SeedlessOnboardingController', () => { // Mock the recover enc key mockRecoverEncKey(toprfClient, MOCK_PASSWORD); + // #changeEncryptionKey fetches items before re-inserting to sort them first + jest + .spyOn(toprfClient, 'fetchAllSecretDataItems') + .mockResolvedValue([ + { + data: stringToBytes( + JSON.stringify({ + data: bytesToBase64(MOCK_SEED_PHRASE), + timestamp: 1000, + }), + ), + itemId: 'srp-1', + version: 'v1' as const, + dataType: undefined, + createdAt: undefined, + }, + ]); + // Mock changeEncKey to always fail with token expired error jest .spyOn(toprfClient, 'changeEncKey') @@ -6246,6 +6219,24 @@ describe('SeedlessOnboardingController', () => { // Mock the recover enc key mockRecoverEncKey(toprfClient, MOCK_PASSWORD); + // #changeEncryptionKey fetches items before re-inserting to sort them first + jest + .spyOn(toprfClient, 'fetchAllSecretDataItems') + .mockResolvedValue([ + { + data: stringToBytes( + JSON.stringify({ + data: bytesToBase64(MOCK_SEED_PHRASE), + timestamp: 1000, + }), + ), + itemId: 'srp-1', + version: 'v1' as const, + dataType: undefined, + createdAt: undefined, + }, + ]); + // Mock changeEncKey to fail with a non-token error jest .spyOn(toprfClient, 'changeEncKey') diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts index 6ec7f2e203..a99e021d86 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts @@ -1643,41 +1643,20 @@ export class SeedlessOnboardingController< }), ); - // Sort: PrimarySrp first, then by createdAt/timestamp (oldest first) + // Sort: PrimarySrp first, then by client timestamp (oldest first) results.sort((a, b) => SecretMetadata.compare(a, b, 'asc')); - // Find the primary SRP instead of assuming it is at index 0: legacy - // items have no `createdAt`, so ordering falls back to the client - // `timestamp`, which clock skew can sort a private key ahead of. - // A candidate is a mnemonic whose `dataType` is `PrimarySrp` or unset. - // `dataType` (a plaintext server field) is the only thing separating - // primary from imported SRP; legacy items lack it and the encrypted - // `SecretType` groups both, so among legacy mnemonics the primary is - // indistinguishable — we take the oldest (best-effort). - const primaryIndex = results.findIndex((item) => { - const isDataTypePrimary = - item.dataType === undefined || - item.dataType === null || - item.dataType === EncAccountDataType.PrimarySrp; - return ( - isDataTypePrimary && - SecretMetadata.matchesType(item, SecretType.Mnemonic) - ); - }); - - // No recoverable primary SRP exists in the metadata store. - if (primaryIndex === -1) { + const firstIsPrimary = + SecretMetadata.matchesType(results[0], SecretType.Mnemonic) && + (results[0].dataType === undefined || + results[0].dataType === null || + results[0].dataType === EncAccountDataType.PrimarySrp); + if (!firstIsPrimary) { throw new Error( SeedlessOnboardingControllerErrorMessage.InvalidPrimarySecretDataType, ); } - // Ensure the primary SRP is first; callers rely on `results[0]` being it. - if (primaryIndex > 0) { - const [primary] = results.splice(primaryIndex, 1); - results.unshift(primary); - } - return results; } @@ -1723,6 +1702,23 @@ export class SeedlessOnboardingController< keyShareIndex: globalKeyIndex, } = await this.#recoverEncKey(oldPassword)); } + // Sort before re-insert so the server stamps createdAt in creation order. + const rawItems = await this.toprfClient.fetchAllSecretDataItems({ + decKey: encKey, + authKeyPair, + }); + const existingDataItems = rawItems + .sort((a, b) => + SecretMetadata.compare( + SecretMetadata.fromRawMetadata(a.data, { dataType: a.dataType }), + SecretMetadata.fromRawMetadata(b.data, { dataType: b.dataType }), + 'asc', + ), + ) + .map(({ data, dataType, version }) => { + return { data, dataType, version: dataType === undefined ? 'v1' : version }; + }); + const result = await this.toprfClient.changeEncKey({ nodeAuthTokens: this.state.nodeAuthTokens, authConnectionId, @@ -1733,6 +1729,7 @@ export class SeedlessOnboardingController< oldAuthKeyPair: authKeyPair, newKeyShareIndex: globalKeyIndex, newPassword, + existingDataItems, }); return result; } diff --git a/packages/seedless-onboarding-controller/src/utils.test.ts b/packages/seedless-onboarding-controller/src/utils.test.ts index 095f6233d3..ecd047a0b8 100644 --- a/packages/seedless-onboarding-controller/src/utils.test.ts +++ b/packages/seedless-onboarding-controller/src/utils.test.ts @@ -5,12 +5,10 @@ import { utf8ToBytes } from '@noble/ciphers/utils'; import { createMockJWTToken } from '../tests/mocks/utils'; import type { DecodedNodeAuthToken } from './types'; import { - compareTimeuuid, decodeNodeAuthToken, decodeJWTToken, compareAndGetLatestToken, getSecretTypeFromDataType, - getTimestampFromTimeuuid, } from './utils'; describe('utils', () => { @@ -223,67 +221,6 @@ describe('utils', () => { }); }); - describe('getTimestampFromTimeuuid', () => { - it('should extract timestamp from TIMEUUID', () => { - const uuid = '00000001-0000-1000-8000-000000000001'; - const timestamp = getTimestampFromTimeuuid(uuid); - expect(timestamp).toBe(BigInt(1)); - }); - - it('should correctly parse TIMEUUID timestamps in chronological order', () => { - const uuid1 = 'c14cc4a0-d1cd-11f0-9878-3be4d8d3a8a0'; - const uuid2 = '04e72250-d1ce-11f0-9878-3be4d8d3a8a0'; - const uuid3 = '11765040-d1ce-11f0-9878-3be4d8d3a8a0'; - const uuid4 = 'b2649610-d1ce-11f0-9878-3be4d8d3a8a0'; - - const ts1 = getTimestampFromTimeuuid(uuid1); - const ts2 = getTimestampFromTimeuuid(uuid2); - const ts3 = getTimestampFromTimeuuid(uuid3); - const ts4 = getTimestampFromTimeuuid(uuid4); - - expect(ts1 < ts2).toBe(true); - expect(ts2 < ts3).toBe(true); - expect(ts3 < ts4).toBe(true); - }); - }); - - describe('compareTimeuuid', () => { - it('should return negative when a < b in ascending order', () => { - const earlier = '00000001-0000-1000-8000-000000000001'; - const later = '00000002-0000-1000-8000-000000000002'; - expect(compareTimeuuid(earlier, later, 'asc')).toBe(-1); - }); - - it('should return positive when a > b in ascending order', () => { - const earlier = '00000001-0000-1000-8000-000000000001'; - const later = '00000002-0000-1000-8000-000000000002'; - expect(compareTimeuuid(later, earlier, 'asc')).toBe(1); - }); - - it('should return zero when timestamps are equal', () => { - const uuid = '00000001-0000-1000-8000-000000000001'; - expect(compareTimeuuid(uuid, uuid, 'asc')).toBe(0); - }); - - it('should return positive when a < b in descending order', () => { - const earlier = '00000001-0000-1000-8000-000000000001'; - const later = '00000002-0000-1000-8000-000000000002'; - expect(compareTimeuuid(earlier, later, 'desc')).toBe(1); - }); - - it('should return negative when a > b in descending order', () => { - const earlier = '00000001-0000-1000-8000-000000000001'; - const later = '00000002-0000-1000-8000-000000000002'; - expect(compareTimeuuid(later, earlier, 'desc')).toBe(-1); - }); - - it('should default to ascending order', () => { - const earlier = '00000001-0000-1000-8000-000000000001'; - const later = '00000002-0000-1000-8000-000000000002'; - expect(compareTimeuuid(earlier, later)).toBe(-1); - }); - }); - describe('getSecretTypeFromDataType', () => { it('should throw an error for unknown EncAccountDataType', () => { const unknownDataType = 'UnknownType' as unknown as EncAccountDataType; diff --git a/packages/seedless-onboarding-controller/src/utils.ts b/packages/seedless-onboarding-controller/src/utils.ts index 035f405d60..149e689cd5 100644 --- a/packages/seedless-onboarding-controller/src/utils.ts +++ b/packages/seedless-onboarding-controller/src/utils.ts @@ -149,53 +149,6 @@ export function compareAndGetLatestToken( return jwtToken2; } -/** - * Extract the 60-bit timestamp from a TIMEUUID (version 1 UUID) string. - * - * TIMEUUID structure: xxxxxxxx-xxxx-1xxx-xxxx-xxxxxxxxxxxx - * - time_low (first 8 hex chars): least significant 32 bits of timestamp - * - time_mid (chars 9-12): middle 16 bits of timestamp - * - time_hi (chars 14-16, after version nibble): most significant 12 bits of timestamp - * - * @param uuid - The TIMEUUID string to extract timestamp from. - * @returns The 60-bit timestamp as a bigint. - */ -export function getTimestampFromTimeuuid(uuid: string): bigint { - const parts = uuid.split('-'); - const timeLow = parts[0]; // 32 bits (least significant) - const timeMid = parts[1]; // 16 bits - const timeHi = parts[2].slice(1); // 12 bits (remove version nibble '1') - // Reconstruct timestamp: timeHi | timeMid | timeLow - return BigInt(`0x${timeHi}${timeMid}${timeLow}`); -} - -/** - * Compare two TIMEUUID strings by their actual timestamps. - * - * Note: TIMEUUID strings are NOT lexicographically sortable because the - * least significant bits of the timestamp appear first in the string. - * - * @param a - First TIMEUUID string. - * @param b - Second TIMEUUID string. - * @param order - Sort order: 'asc' for oldest first, 'desc' for newest first. Default is 'asc'. - * @returns Negative if a < b (in ascending order), positive if a > b, zero if equal. - */ -export function compareTimeuuid( - a: string, - b: string, - order: 'asc' | 'desc' = 'asc', -): number { - const tsA = getTimestampFromTimeuuid(a); - const tsB = getTimestampFromTimeuuid(b); - if (tsA < tsB) { - return order === 'asc' ? -1 : 1; - } - if (tsA > tsB) { - return order === 'asc' ? 1 : -1; - } - return 0; -} - /** * Derive SecretType from EncAccountDataType. * From c7afd0dd40483dd2ec7cd305cf67d37204d66d91 Mon Sep 17 00:00:00 2001 From: huggingbot <83656073+huggingbot@users.noreply.github.com> Date: Thu, 25 Jun 2026 15:25:18 +0800 Subject: [PATCH 09/16] fix: Restore to original code --- .../src/SeedlessOnboardingController.ts | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts index a99e021d86..4fde3c3b48 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts @@ -1646,12 +1646,18 @@ export class SeedlessOnboardingController< // Sort: PrimarySrp first, then by client timestamp (oldest first) results.sort((a, b) => SecretMetadata.compare(a, b, 'asc')); - const firstIsPrimary = - SecretMetadata.matchesType(results[0], SecretType.Mnemonic) && - (results[0].dataType === undefined || - results[0].dataType === null || - results[0].dataType === EncAccountDataType.PrimarySrp); - if (!firstIsPrimary) { + // Validate the first item is the primary SRP + const firstItem = results[0]; + const isDataTypePrimary = + firstItem.dataType === undefined || + firstItem.dataType === null || + firstItem.dataType === EncAccountDataType.PrimarySrp; + const isMnemonic = SecretMetadata.matchesType( + firstItem, + SecretType.Mnemonic, + ); + + if (!isDataTypePrimary || !isMnemonic) { throw new Error( SeedlessOnboardingControllerErrorMessage.InvalidPrimarySecretDataType, ); From 9b16ff3428ef5f76c477da8ca173293f75910523 Mon Sep 17 00:00:00 2001 From: huggingbot <83656073+huggingbot@users.noreply.github.com> Date: Thu, 25 Jun 2026 15:26:14 +0800 Subject: [PATCH 10/16] fix: Restore to original code --- .../src/SeedlessOnboardingController.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts index 4fde3c3b48..91d05e9d6b 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts @@ -1662,7 +1662,6 @@ export class SeedlessOnboardingController< SeedlessOnboardingControllerErrorMessage.InvalidPrimarySecretDataType, ); } - return results; } From 3c5931e5ba3202777f0bd59109f61ad9dee96a07 Mon Sep 17 00:00:00 2001 From: huggingbot <83656073+huggingbot@users.noreply.github.com> Date: Thu, 25 Jun 2026 15:34:06 +0800 Subject: [PATCH 11/16] chore: Update changelog --- packages/seedless-onboarding-controller/CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/seedless-onboarding-controller/CHANGELOG.md b/packages/seedless-onboarding-controller/CHANGELOG.md index 9dcb4b2a63..1c2b4d64b1 100644 --- a/packages/seedless-onboarding-controller/CHANGELOG.md +++ b/packages/seedless-onboarding-controller/CHANGELOG.md @@ -11,10 +11,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Bump `@metamask/utils` from `^11.9.0` to `^11.11.0` ([#9074](https://github.com/MetaMask/core/pull/9074)) - Bump `@metamask/keyring-controller` from `^27.0.0` to `^27.1.0` ([#9129](https://github.com/MetaMask/core/pull/9129)) +- `SecretMetadata.compare` no longer uses `createdAt` (server-stamped TIMEUUID) for ordering; sort priority is now `PrimarySrp` tag first, then client-side timestamp ([#9247](https://github.com/MetaMask/core/pull/9247)) +- The password change flow now sorts secret data items before re-inserting them, passing the pre-sorted items to `changeEncKey` so the server stamps `createdAt` in creation order ([#9247](https://github.com/MetaMask/core/pull/9247)) ### Fixed -- Fix `InvalidPrimarySecretDataType` thrown for legacy accounts where client clock skew sorts a non-mnemonic item ahead of the primary SRP ([#9247](https://github.com/MetaMask/core/pull/9247)) +- Fix `InvalidPrimarySecretDataType` crash caused by `changeEncKey` re-inserting items without sorting, causing the server to stamp `createdAt` in arbitrary order on password change ([#9247](https://github.com/MetaMask/core/pull/9247)) ## [10.0.2] From 85d4f501adce545bde2f04f36be1fe50b1179124 Mon Sep 17 00:00:00 2001 From: huggingbot <83656073+huggingbot@users.noreply.github.com> Date: Thu, 25 Jun 2026 17:29:35 +0800 Subject: [PATCH 12/16] fix: promote primary SRP to first under clock skew --- .../src/SeedlessOnboardingController.test.ts | 47 +++++++++++++++++++ .../src/SeedlessOnboardingController.ts | 22 ++++----- 2 files changed, 58 insertions(+), 11 deletions(-) diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts index b43de7cec4..ca0675d913 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts @@ -3448,6 +3448,53 @@ describe('SeedlessOnboardingController', () => { ); }); + it('should promote the primary SRP to first when a legacy non-mnemonic sorts ahead of it (clock skew)', async () => { + await withController( + { + state: getMockInitialControllerState({ + withMockAuthenticatedUser: true, + }), + }, + async ({ controller, toprfClient }) => { + mockRecoverEncKey(toprfClient, MOCK_PASSWORD); + + // Private key has older timestamp and sorts before the primary SRP + // (simulates clock skew on a legacy account with no dataType tags) + const mockSecretDataGet = handleMockSecretDataGet({ + status: 200, + body: createMockSecretDataGetResponse( + [ + { + data: MOCK_PRIVATE_KEY, + timestamp: 100, + type: SecretType.PrivateKey, + itemId: 'pk-id', + // No dataType (legacy) + }, + { + data: MOCK_SEED_PHRASE, + timestamp: 200, + type: SecretType.Mnemonic, + itemId: 'primary-srp-id', + // No dataType (legacy) + }, + ], + MOCK_PASSWORD, + ), + }); + + const secretData = await controller.fetchAllSecretData(MOCK_PASSWORD); + + expect(mockSecretDataGet.isDone()).toBe(true); + expect(secretData).toHaveLength(2); + // Primary SRP promoted to first despite private key having older timestamp + expect(secretData[0].type).toBe(SecretType.Mnemonic); + expect(secretData[0].data).toStrictEqual(MOCK_SEED_PHRASE); + expect(secretData[1].type).toBe(SecretType.PrivateKey); + }, + ); + }); + it('should throw an error if the first item has non-primary dataType', async () => { await withController( { diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts index 91d05e9d6b..3be1b1245e 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts @@ -1646,22 +1646,22 @@ export class SeedlessOnboardingController< // Sort: PrimarySrp first, then by client timestamp (oldest first) results.sort((a, b) => SecretMetadata.compare(a, b, 'asc')); - // Validate the first item is the primary SRP - const firstItem = results[0]; - const isDataTypePrimary = - firstItem.dataType === undefined || - firstItem.dataType === null || - firstItem.dataType === EncAccountDataType.PrimarySrp; - const isMnemonic = SecretMetadata.matchesType( - firstItem, - SecretType.Mnemonic, + const primaryIndex = results.findIndex( + (result) => + SecretMetadata.matchesType(result, SecretType.Mnemonic) && + (result.dataType === undefined || + result.dataType === null || + result.dataType === EncAccountDataType.PrimarySrp), ); - - if (!isDataTypePrimary || !isMnemonic) { + if (primaryIndex === -1) { throw new Error( SeedlessOnboardingControllerErrorMessage.InvalidPrimarySecretDataType, ); } + if (primaryIndex !== 0) { + const [primary] = results.splice(primaryIndex, 1); + results.unshift(primary); + } return results; } From cba3056191af23898ce21665519bd53244a938a8 Mon Sep 17 00:00:00 2001 From: huggingbot <83656073+huggingbot@users.noreply.github.com> Date: Thu, 25 Jun 2026 17:52:54 +0800 Subject: [PATCH 13/16] refactor(seedless-onboarding-controller): update password change flow to sort data items within metadata lock --- .../CHANGELOG.md | 2 +- .../src/SeedlessOnboardingController.test.ts | 70 +------------------ .../src/SeedlessOnboardingController.ts | 32 ++++----- 3 files changed, 18 insertions(+), 86 deletions(-) diff --git a/packages/seedless-onboarding-controller/CHANGELOG.md b/packages/seedless-onboarding-controller/CHANGELOG.md index 1c2b4d64b1..d4766b275c 100644 --- a/packages/seedless-onboarding-controller/CHANGELOG.md +++ b/packages/seedless-onboarding-controller/CHANGELOG.md @@ -12,7 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Bump `@metamask/utils` from `^11.9.0` to `^11.11.0` ([#9074](https://github.com/MetaMask/core/pull/9074)) - Bump `@metamask/keyring-controller` from `^27.0.0` to `^27.1.0` ([#9129](https://github.com/MetaMask/core/pull/9129)) - `SecretMetadata.compare` no longer uses `createdAt` (server-stamped TIMEUUID) for ordering; sort priority is now `PrimarySrp` tag first, then client-side timestamp ([#9247](https://github.com/MetaMask/core/pull/9247)) -- The password change flow now sorts secret data items before re-inserting them, passing the pre-sorted items to `changeEncKey` so the server stamps `createdAt` in creation order ([#9247](https://github.com/MetaMask/core/pull/9247)) +- The password change flow passes a `transformDataItems` callback to `changeEncKey` so sorting happens inside the metadata lock, preventing data loss from concurrent writes ([#9247](https://github.com/MetaMask/core/pull/9247)) ### Fixed diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts index ca0675d913..c5832fddbc 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts @@ -454,22 +454,6 @@ function mockChangeEncKey( const pwEncKey = mockToprfEncryptor.derivePwEncKey(newPassword); const authKeyPair = mockToprfEncryptor.deriveAuthKeyPair(newPassword); - // #changeEncryptionKey fetches items before re-inserting to sort them first - jest.spyOn(toprfClient, 'fetchAllSecretDataItems').mockResolvedValue([ - { - data: stringToBytes( - JSON.stringify({ - data: bytesToBase64(MOCK_SEED_PHRASE), - timestamp: 1000, - }), - ), - itemId: 'srp-1', - version: 'v1' as const, - dataType: undefined, - createdAt: undefined, - }, - ]); - jest.spyOn(toprfClient, 'changeEncKey').mockResolvedValueOnce({ encKey, pwEncKey, @@ -6083,23 +6067,7 @@ describe('SeedlessOnboardingController', () => { // Mock the recover enc key mockRecoverEncKey(toprfClient, MOCK_PASSWORD); - // #changeEncryptionKey fetches items before re-inserting to sort them first - jest - .spyOn(toprfClient, 'fetchAllSecretDataItems') - .mockResolvedValue([ - { - data: stringToBytes( - JSON.stringify({ - data: bytesToBase64(MOCK_SEED_PHRASE), - timestamp: 1000, - }), - ), - itemId: 'srp-1', - version: 'v1' as const, - dataType: undefined, - createdAt: undefined, - }, - ]); + // Mock changeEncKey to fail first with token expired error, then succeed const mockToprfEncryptor = createMockToprfEncryptor(); @@ -6186,23 +6154,7 @@ describe('SeedlessOnboardingController', () => { // Mock the recover enc key mockRecoverEncKey(toprfClient, MOCK_PASSWORD); - // #changeEncryptionKey fetches items before re-inserting to sort them first - jest - .spyOn(toprfClient, 'fetchAllSecretDataItems') - .mockResolvedValue([ - { - data: stringToBytes( - JSON.stringify({ - data: bytesToBase64(MOCK_SEED_PHRASE), - timestamp: 1000, - }), - ), - itemId: 'srp-1', - version: 'v1' as const, - dataType: undefined, - createdAt: undefined, - }, - ]); + // Mock changeEncKey to always fail with token expired error jest @@ -6266,23 +6218,7 @@ describe('SeedlessOnboardingController', () => { // Mock the recover enc key mockRecoverEncKey(toprfClient, MOCK_PASSWORD); - // #changeEncryptionKey fetches items before re-inserting to sort them first - jest - .spyOn(toprfClient, 'fetchAllSecretDataItems') - .mockResolvedValue([ - { - data: stringToBytes( - JSON.stringify({ - data: bytesToBase64(MOCK_SEED_PHRASE), - timestamp: 1000, - }), - ), - itemId: 'srp-1', - version: 'v1' as const, - dataType: undefined, - createdAt: undefined, - }, - ]); + // Mock changeEncKey to fail with a non-token error jest diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts index 3be1b1245e..c778a1c613 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts @@ -1707,23 +1707,6 @@ export class SeedlessOnboardingController< keyShareIndex: globalKeyIndex, } = await this.#recoverEncKey(oldPassword)); } - // Sort before re-insert so the server stamps createdAt in creation order. - const rawItems = await this.toprfClient.fetchAllSecretDataItems({ - decKey: encKey, - authKeyPair, - }); - const existingDataItems = rawItems - .sort((a, b) => - SecretMetadata.compare( - SecretMetadata.fromRawMetadata(a.data, { dataType: a.dataType }), - SecretMetadata.fromRawMetadata(b.data, { dataType: b.dataType }), - 'asc', - ), - ) - .map(({ data, dataType, version }) => { - return { data, dataType, version: dataType === undefined ? 'v1' : version }; - }); - const result = await this.toprfClient.changeEncKey({ nodeAuthTokens: this.state.nodeAuthTokens, authConnectionId, @@ -1734,7 +1717,20 @@ export class SeedlessOnboardingController< oldAuthKeyPair: authKeyPair, newKeyShareIndex: globalKeyIndex, newPassword, - existingDataItems, + transformDataItems: (items) => + items + .sort((a, b) => + SecretMetadata.compare( + SecretMetadata.fromRawMetadata(a.data, { dataType: a.dataType }), + SecretMetadata.fromRawMetadata(b.data, { dataType: b.dataType }), + 'asc', + ), + ) + .map(({ data, dataType, version }) => ({ + data, + dataType, + version: dataType === undefined ? 'v1' : version, + })), }); return result; } From 3e7eae5a040bd9aa06ef54853296c4a6e02f82e9 Mon Sep 17 00:00:00 2001 From: huggingbot <83656073+huggingbot@users.noreply.github.com> Date: Thu, 25 Jun 2026 19:49:57 +0800 Subject: [PATCH 14/16] fix: Lint --- .../src/SeedlessOnboardingController.test.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts index c5832fddbc..eb7742c7ec 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts @@ -3519,7 +3519,6 @@ describe('SeedlessOnboardingController', () => { }, ); }); - }); describe('submitPassword', () => { @@ -6067,8 +6066,6 @@ describe('SeedlessOnboardingController', () => { // Mock the recover enc key mockRecoverEncKey(toprfClient, MOCK_PASSWORD); - - // Mock changeEncKey to fail first with token expired error, then succeed const mockToprfEncryptor = createMockToprfEncryptor(); const newEncKey = @@ -6154,8 +6151,6 @@ describe('SeedlessOnboardingController', () => { // Mock the recover enc key mockRecoverEncKey(toprfClient, MOCK_PASSWORD); - - // Mock changeEncKey to always fail with token expired error jest .spyOn(toprfClient, 'changeEncKey') @@ -6218,8 +6213,6 @@ describe('SeedlessOnboardingController', () => { // Mock the recover enc key mockRecoverEncKey(toprfClient, MOCK_PASSWORD); - - // Mock changeEncKey to fail with a non-token error jest .spyOn(toprfClient, 'changeEncKey') From 5feb43dad40131b7a61022014b3c02ec102a4ab1 Mon Sep 17 00:00:00 2001 From: lwin Date: Thu, 25 Jun 2026 22:00:22 +0800 Subject: [PATCH 15/16] feat: upgrade toprf-secure-backup to 1.1.0 --- .../package.json | 2 +- .../src/SeedlessOnboardingController.test.ts | 114 ++++++++++++++++++ yarn.lock | 10 +- 3 files changed, 120 insertions(+), 6 deletions(-) diff --git a/packages/seedless-onboarding-controller/package.json b/packages/seedless-onboarding-controller/package.json index 56d457266c..4f8af6144d 100644 --- a/packages/seedless-onboarding-controller/package.json +++ b/packages/seedless-onboarding-controller/package.json @@ -58,7 +58,7 @@ "@metamask/browser-passworder": "^6.0.0", "@metamask/keyring-controller": "^27.1.0", "@metamask/messenger": "^1.2.0", - "@metamask/toprf-secure-backup": "^1.0.0", + "@metamask/toprf-secure-backup": "^1.1.0", "@metamask/utils": "^11.11.0", "@noble/ciphers": "^1.3.0", "@noble/curves": "^1.9.2", diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts index eb7742c7ec..d687e607d2 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts @@ -4243,6 +4243,120 @@ describe('SeedlessOnboardingController', () => { ); }); + it('should pass transformDataItems to changeEncKey that sorts secret data', async () => { + await withController( + { + state: getMockInitialControllerState({ + withMockAuthenticatedUser: true, + withMockAuthPubKey: true, + }), + }, + async ({ controller, toprfClient, baseMessenger }) => { + await mockCreateToprfKeyAndBackupSeedPhrase( + toprfClient, + controller, + baseMessenger, + MOCK_PASSWORD, + MOCK_SEED_PHRASE, + MOCK_KEYRING_ID, + ); + + mockFetchAuthPubKey( + toprfClient, + base64ToBytes(controller.state.authPubKey as string), + ); + + mockChangeEncKey(toprfClient, NEW_MOCK_PASSWORD); + + const changeEncKeySpy = jest.spyOn(toprfClient, 'changeEncKey'); + + await baseMessenger.call( + 'SeedlessOnboardingController:changePassword', + NEW_MOCK_PASSWORD, + MOCK_PASSWORD, + ); + + expect(changeEncKeySpy).toHaveBeenCalledWith( + expect.objectContaining({ + transformDataItems: expect.any(Function), + }), + ); + + const { transformDataItems } = changeEncKeySpy.mock.calls[0][0]; + expect(transformDataItems).toBeDefined(); + + const importedSrp2 = stringToBytes('imported-seed-phrase-2'); + const importedSrp3 = stringToBytes('imported-seed-phrase-3'); + + const unsortedItems = [ + { + data: new SecretMetadata(importedSrp3, { + dataType: EncAccountDataType.ImportedSrp, + timestamp: 30, + }).toBytes(), + dataType: EncAccountDataType.ImportedSrp, + version: 'v2' as const, + createdAt: '00000003-0000-1000-8000-000000000003', + itemId: 'srp-3', + }, + { + data: new SecretMetadata(MOCK_SEED_PHRASE, { + dataType: EncAccountDataType.PrimarySrp, + timestamp: 10, + }).toBytes(), + dataType: EncAccountDataType.PrimarySrp, + version: 'v2' as const, + createdAt: '00000001-0000-1000-8000-000000000001', + itemId: 'srp-1', + }, + { + data: new SecretMetadata(importedSrp2, { + dataType: EncAccountDataType.ImportedSrp, + timestamp: 20, + }).toBytes(), + dataType: EncAccountDataType.ImportedSrp, + version: 'v2' as const, + createdAt: '00000002-0000-1000-8000-000000000002', + itemId: 'srp-2', + }, + ]; + + const transformed = transformDataItems?.(unsortedItems); + + const parseSecretData = (raw: Uint8Array): Uint8Array => + SecretMetadata.fromRawMetadata(raw, {}).data; + + expect(transformed).toHaveLength(3); + expect(transformed?.[0].dataType).toBe( + EncAccountDataType.PrimarySrp, + ); + expect(parseSecretData(transformed?.[0].data as Uint8Array)).toStrictEqual( + MOCK_SEED_PHRASE, + ); + expect(parseSecretData(transformed?.[1].data as Uint8Array)).toStrictEqual( + importedSrp2, + ); + expect(parseSecretData(transformed?.[2].data as Uint8Array)).toStrictEqual( + importedSrp3, + ); + expect(transformed?.[0].version).toBe('v2'); + + const legacyItem = { + data: new SecretMetadata(MOCK_SEED_PHRASE, { + type: SecretType.Mnemonic, + timestamp: 10, + }).toBytes(), + dataType: undefined, + version: 'v2' as const, + itemId: 'legacy-srp', + }; + + const [legacyTransformed] = transformDataItems?.([legacyItem]) ?? []; + expect(legacyTransformed?.version).toBe('v1'); + }, + ); + }); + it('should call recoverEncKey when keyIndex is missing', async () => { await withController( { diff --git a/yarn.lock b/yarn.lock index 0c92d077b6..75a5e94d80 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8271,7 +8271,7 @@ __metadata: "@metamask/browser-passworder": "npm:^6.0.0" "@metamask/keyring-controller": "npm:^27.1.0" "@metamask/messenger": "npm:^1.2.0" - "@metamask/toprf-secure-backup": "npm:^1.0.0" + "@metamask/toprf-secure-backup": "npm:^1.1.0" "@metamask/utils": "npm:^11.11.0" "@noble/ciphers": "npm:^1.3.0" "@noble/curves": "npm:^1.9.2" @@ -8688,9 +8688,9 @@ __metadata: languageName: node linkType: hard -"@metamask/toprf-secure-backup@npm:^1.0.0": - version: 1.0.0 - resolution: "@metamask/toprf-secure-backup@npm:1.0.0" +"@metamask/toprf-secure-backup@npm:^1.1.0": + version: 1.1.0 + resolution: "@metamask/toprf-secure-backup@npm:1.1.0" dependencies: "@metamask/auth-network-utils": "npm:^0.4.0" "@noble/ciphers": "npm:^1.2.1" @@ -8702,7 +8702,7 @@ __metadata: "@toruslabs/fetch-node-details": "npm:^15.0.0" "@toruslabs/http-helpers": "npm:^8.1.1" bn.js: "npm:^5.2.2" - checksum: 10/f34d788c9f411fff45f9f1e38de0c91e01f23be014b60d6d8747f73c7352c14c38f9ce6cd435ba90375f298e6fc593061adbb33c1752d5717b54fda65cb1e9aa + checksum: 10/2714a70eae53a1f46d293980c71a1ed2f59708247019444154e71fc15328e78ec95287ffc2d954a29f903224a1a9fe4d69fb17724e9039bcefd240752f1df26c languageName: node linkType: hard From 3c409bf02237f477005db3d4ca988558cef12744 Mon Sep 17 00:00:00 2001 From: huggingbot <83656073+huggingbot@users.noreply.github.com> Date: Thu, 25 Jun 2026 22:07:08 +0800 Subject: [PATCH 16/16] fix: Lint --- .../src/SeedlessOnboardingController.test.ts | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts index d687e607d2..ff08103317 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts @@ -4327,18 +4327,16 @@ describe('SeedlessOnboardingController', () => { SecretMetadata.fromRawMetadata(raw, {}).data; expect(transformed).toHaveLength(3); - expect(transformed?.[0].dataType).toBe( - EncAccountDataType.PrimarySrp, - ); - expect(parseSecretData(transformed?.[0].data as Uint8Array)).toStrictEqual( - MOCK_SEED_PHRASE, - ); - expect(parseSecretData(transformed?.[1].data as Uint8Array)).toStrictEqual( - importedSrp2, - ); - expect(parseSecretData(transformed?.[2].data as Uint8Array)).toStrictEqual( - importedSrp3, - ); + expect(transformed?.[0].dataType).toBe(EncAccountDataType.PrimarySrp); + expect( + parseSecretData(transformed?.[0].data as Uint8Array), + ).toStrictEqual(MOCK_SEED_PHRASE); + expect( + parseSecretData(transformed?.[1].data as Uint8Array), + ).toStrictEqual(importedSrp2); + expect( + parseSecretData(transformed?.[2].data as Uint8Array), + ).toStrictEqual(importedSrp3); expect(transformed?.[0].version).toBe('v2'); const legacyItem = {