Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lwin-kyaw merged 16 commits into from
Jun 25, 2026
Merged

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

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
b3f2828
feat: promote primary SRP in secret data fetch to handle legacy items
huggingbot Jun 24, 2026
311dc79
chore(seedless-onboarding-controller): add changelog entry for primar…
huggingbot Jun 24, 2026
4570150
chore(seedless-onboarding-controller): fix changelog section order
huggingbot Jun 24, 2026
19df57a
test(seedless-onboarding-controller): add compare slot-0 reproduction…
huggingbot Jun 24, 2026
713c2f5
test(seedless-onboarding-controller): add mixed-collection compare sc…
huggingbot Jun 24, 2026
196a323
test(seedless-onboarding-controller): refocus compare tests on realis…
huggingbot Jun 24, 2026
e808444
test(seedless-onboarding-controller): remove root-cause simulation tests
huggingbot Jun 24, 2026
b197251
refactor(seedless-onboarding-controller): simplify secret metadata co…
huggingbot Jun 25, 2026
c7afd0d
fix: Restore to original code
huggingbot Jun 25, 2026
9b16ff3
fix: Restore to original code
huggingbot Jun 25, 2026
3c5931e
chore: Update changelog
huggingbot Jun 25, 2026
85d4f50
fix: promote primary SRP to first under clock skew
huggingbot Jun 25, 2026
cba3056
refactor(seedless-onboarding-controller): update password change flow…
huggingbot Jun 25, 2026
3e7eae5
fix: Lint
huggingbot Jun 25, 2026
5feb43d
feat: upgrade toprf-secure-backup to 1.1.0
lwin-kyaw Jun 25, 2026
3c409bf
fix: Lint
huggingbot Jun 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/seedless-onboarding-controller/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 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` 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]

Expand Down
19 changes: 3 additions & 16 deletions packages/seedless-onboarding-controller/src/SecretMetadata.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand All @@ -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);
}

Expand Down
99 changes: 71 additions & 28 deletions packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -3472,6 +3488,7 @@ describe('SeedlessOnboardingController', () => {
},
);
});

});

describe('submitPassword', () => {
Expand Down Expand Up @@ -4578,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,
Expand Down Expand Up @@ -6047,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 =
Expand Down Expand Up @@ -6132,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')
Expand Down Expand Up @@ -6194,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')
Expand Down
20 changes: 19 additions & 1 deletion packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1643,7 +1643,7 @@ 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
Expand Down Expand Up @@ -1707,6 +1707,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,
Expand All @@ -1717,6 +1734,7 @@ export class SeedlessOnboardingController<
oldAuthKeyPair: authKeyPair,
newKeyShareIndex: globalKeyIndex,
newPassword,
existingDataItems,

@lwin-kyaw lwin-kyaw Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a slim chance of race condition here.

After we fetched and sorted the existing data items and before we acquire the metadata lock, user could add new secret data and the existingDataItems isn't updated anymore.

This could cause the data loss or crash (if we don't handle decryption very well).

});
return result;
}
Expand Down
63 changes: 0 additions & 63 deletions packages/seedless-onboarding-controller/src/utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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;
Expand Down
47 changes: 0 additions & 47 deletions packages/seedless-onboarding-controller/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down
Loading