fix(seedless-onboarding-controller): fix InvalidPrimarySecretDataType for legacy accounts with client clock skew

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]
 

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",

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<DataType extends SecretDataType = Uint8Array> = {
   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);
   }
 

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<Uint8Array>(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,

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;
   }