diff --git a/packages/seedless-onboarding-controller/CHANGELOG.md b/packages/seedless-onboarding-controller/CHANGELOG.md index a991b2ac4a..d4766b275c 100644 --- a/packages/seedless-onboarding-controller/CHANGELOG.md +++ b/packages/seedless-onboarding-controller/CHANGELOG.md @@ -11,6 +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 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 + +- 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] 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/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 bc1406e3e6..ff08103317 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts @@ -3432,6 +3432,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( { @@ -4196,6 +4243,118 @@ 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( { @@ -4578,34 +4737,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, diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts index 91020b33f2..c778a1c613 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts @@ -1643,25 +1643,25 @@ 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')); - // 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; } @@ -1717,6 +1717,20 @@ export class SeedlessOnboardingController< oldAuthKeyPair: authKeyPair, newKeyShareIndex: globalKeyIndex, newPassword, + 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; } 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. * 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