Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions packages/seedless-onboarding-controller/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Bump `@metamask/utils` from `^11.12.0` to `^12.0.0` ([#10192](https://github.com/MetaMask/core/pull/10192))
- Bump `@metamask/keyring-controller` from `^28.0.0` to `^28.1.0` ([#10418](https://github.com/MetaMask/core/pull/10418))

### Fixed

- Prevent initial TOPRF seed phrase backup retries from skipping the remote metadata write after a partial failure ([#10419](https://github.com/MetaMask/core/pull/10419))
- Reject duplicate TOPRF key and seed phrase backup attempts when a vault already exists ([#10419](https://github.com/MetaMask/core/pull/10419))

## [11.0.0]

### Changed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1402,6 +1402,105 @@ describe('SeedlessOnboardingController', () => {
);
});

it('should retry the remote backup after vault creation fails', async () => {
await withController(
{
state: getMockInitialControllerState({
withMockAuthenticatedUser: true,
}),
},
async ({ controller, toprfClient, encryptor, baseMessenger }) => {
const { createLocalKeySpy } = mockcreateLocalKey(
toprfClient,
MOCK_PASSWORD,
);
const retryToprfEncryptor = createMockToprfEncryptor();
createLocalKeySpy.mockResolvedValueOnce({
encKey: retryToprfEncryptor.deriveEncKey('retry-password'),
pwEncKey: retryToprfEncryptor.derivePwEncKey('retry-password'),
authKeyPair:
retryToprfEncryptor.deriveAuthKeyPair('retry-password'),
oprfKey: BigInt(1),
seed: stringToBytes('retry-password'),
});

const persistLocalKeySpy = jest
.spyOn(toprfClient, 'persistLocalKey')
.mockResolvedValue();

// The first attempt writes the remote metadata, then fails while
// creating the vault. The retry must therefore write the metadata
// again using the newly generated TOPRF key.
const firstMockSecretDataAdd = handleMockSecretDataAdd();
const retryMockSecretDataAdd = handleMockSecretDataAdd();
jest
.spyOn(encryptor, 'encryptWithDetail')
.mockRejectedValueOnce(new Error('Failed to create vault'));

await expect(
baseMessenger.call(
'SeedlessOnboardingController:createToprfKeyAndBackupSeedPhrase',
MOCK_PASSWORD,
MOCK_SEED_PHRASE,
MOCK_KEYRING_ID,
),
).rejects.toThrow('Failed to create vault');

expect(firstMockSecretDataAdd.isDone()).toBe(true);
expect(controller.state.socialBackupsMetadata).toStrictEqual([]);

await baseMessenger.call(
'SeedlessOnboardingController:createToprfKeyAndBackupSeedPhrase',
MOCK_PASSWORD,
MOCK_SEED_PHRASE,
MOCK_KEYRING_ID,
);

expect(retryMockSecretDataAdd.isDone()).toBe(true);
expect(createLocalKeySpy).toHaveBeenCalledTimes(2);
expect(persistLocalKeySpy).toHaveBeenCalledTimes(2);
expect(controller.state.socialBackupsMetadata).toStrictEqual([
{
type: SecretType.Mnemonic,
keyringId: MOCK_KEYRING_ID,
hash: keccak256AndHexify(MOCK_SEED_PHRASE),
},
]);
},
);
});

it('should throw if a vault already exists', async () => {
await withController(
{
state: getMockInitialControllerState({
withMockAuthenticatedUser: true,
}),
},
async ({ controller, toprfClient, baseMessenger }) => {
await mockCreateToprfKeyAndBackupSeedPhrase(
toprfClient,
controller,
baseMessenger,
MOCK_PASSWORD,
MOCK_SEED_PHRASE,
MOCK_KEYRING_ID,
);

await expect(
baseMessenger.call(
'SeedlessOnboardingController:createToprfKeyAndBackupSeedPhrase',
MOCK_PASSWORD,
MOCK_SEED_PHRASE,
MOCK_KEYRING_ID,
),
).rejects.toThrow(
SeedlessOnboardingControllerErrorMessage.VaultAlreadyExists,
);
},
);
});

it('should store accessToken in the vault during backup creation', async () => {
await withController(
{
Expand Down Expand Up @@ -1470,6 +1569,7 @@ describe('SeedlessOnboardingController', () => {
.mockResolvedValueOnce();
// encrypt and store the secret data
const mockSecretDataAdd = handleMockSecretDataAdd();
const retryMockSecretDataAdd = handleMockSecretDataAdd();

const authenticateSpy = jest
.spyOn(toprfClient, 'authenticate')
Expand All @@ -1486,6 +1586,7 @@ describe('SeedlessOnboardingController', () => {
);

expect(mockSecretDataAdd.isDone()).toBe(true);
expect(retryMockSecretDataAdd.isDone()).toBe(true);

// should call persistLocalKey twice and authenticate once
expect(persistLocalKeySpy).toHaveBeenCalledTimes(2); // should call persistLocalKey twice for the first fail attempt due to invalid auth token error and the second attempt succeeds
Expand Down Expand Up @@ -1982,6 +2083,16 @@ describe('SeedlessOnboardingController', () => {
},
]);

// do not add a duplicate backup
await baseMessenger.call(
'SeedlessOnboardingController:addNewSecretData',
NEW_KEY_RING_1.seedPhrase,
EncAccountDataType.ImportedSrp,
{
keyringId: NEW_KEY_RING_1.id,
},
);

// add another seed phrase backup
const mockSecretDataAdd2 = handleMockSecretDataAdd();
await baseMessenger.call(
Expand Down Expand Up @@ -3939,7 +4050,6 @@ describe('SeedlessOnboardingController', () => {
describe('changePassword', () => {
const MOCK_PASSWORD = 'mock-password';
const NEW_MOCK_PASSWORD = 'new-mock-password';
const MOCK_VAULT = JSON.stringify({ foo: 'bar' });

it('should be able to update new password', async () => {
await withController(
Expand Down Expand Up @@ -4116,7 +4226,6 @@ describe('SeedlessOnboardingController', () => {
await withController(
{
state: getMockInitialControllerState({
vault: MOCK_VAULT,
authPubKey: MOCK_AUTH_PUB_KEY_OUTDATED,
withMockAuthenticatedUser: true,
}),
Expand Down Expand Up @@ -5039,19 +5148,9 @@ describe('SeedlessOnboardingController', () => {
state: getMockInitialControllerState({
withMockAuthenticatedUser: true,
withMockAuthPubKey: true,
vault: 'mock-vault',
}),
},
async ({ controller, toprfClient, baseMessenger }) => {
await expect(
baseMessenger.call(
'SeedlessOnboardingController:storeKeyringEncryptionKey',
'',
),
).rejects.toThrow(
SeedlessOnboardingControllerErrorMessage.WrongPasswordType,
);

// Setup and store keyring encryption key.
await mockCreateToprfKeyAndBackupSeedPhrase(
toprfClient,
Expand Down Expand Up @@ -6831,7 +6930,7 @@ describe('SeedlessOnboardingController', () => {
);

expect(mockRefreshJWTToken).toHaveBeenCalled();
expect(addSecretDataItemSpy).toHaveBeenCalledTimes(1);
expect(addSecretDataItemSpy).toHaveBeenCalledTimes(2);
expect(authenticateSpy).toHaveBeenCalled();
// should call persistLocalKey twice, once for the first call and another from the refresh token
expect(persistLocalKeySpy).toHaveBeenCalledTimes(2);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -646,21 +646,29 @@ export class SeedlessOnboardingController<
// assert that the user is authenticated before creating the TOPRF key and backing up the seed phrase
this.#assertIsAuthenticatedUser(this.state);

if (this.state.vault) {
throw new Error(
SeedlessOnboardingControllerErrorMessage.VaultAlreadyExists,
);
}

// locally evaluate the encryption key from the password
const { encKey, pwEncKey, authKeyPair, oprfKey } =
await this.toprfClient.createLocalKey({
password,
});
const performKeyCreationAndBackup = async (): Promise<void> => {
// encrypt and store the secret data
await this.#encryptAndStoreSecretData({
const secretMetadata = await this.#encryptAndStoreSecretData({
data: seedPhrase,
dataType: EncAccountDataType.PrimarySrp,
encKey,
authKeyPair,
options: {
keyringId,
},
// Persist local backup state only after the full operation succeeds.
skipStatePersist: true,
});

// store/persist the encryption key shares
Expand All @@ -678,6 +686,13 @@ export class SeedlessOnboardingController<

// Mark migration as complete since this new backup was created with the new data format (dataType)
this.#setMigrationVersion(SeedlessOnboardingMigrationVersion.V1);

// Persist local backup state after all operations succeed.
this.#filterDupesAndUpdateSocialBackupsMetadata({
keyringId,
data: seedPhrase,
type: secretMetadata.type,
});
};

Comment on lines 687 to 697

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.

Missing test for the exact bug scenario. This defers local state until after everything succeeds but there's no test that simulates: remote metadata writes → OPRF/vault creation fails → retry with a new TOPRF key → assert remote write happens again. That's the specific failure this fixes and it should have a dedicated test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

addd here, 57804de

await this.#executeWithTokenRefresh(
Expand Down Expand Up @@ -1744,8 +1759,10 @@ export class SeedlessOnboardingController<
* @param params.authKeyPair - The authentication key pair to store.
* @param params.options - Optional options object.
* @param params.options.keyringId - The keyring id of the backup keyring (required for SRP types).
* @param params.skipStatePersist - Whether to persist the backup state after
* the remote metadata is stored. When true, the backup metadata is not persisted to the controller state.
*
* @returns A promise that resolves to the success of the operation.
* @returns A promise that resolves to the backup metadata.
*/
async #encryptAndStoreSecretData(params: {
data: Uint8Array;
Expand All @@ -1755,16 +1772,24 @@ export class SeedlessOnboardingController<
options?: {
keyringId?: string;
};
}): Promise<void> {
const { options, data, encKey, authKeyPair, dataType } = params;
skipStatePersist?: boolean;
}): Promise<SecretMetadata> {
const {
options,
data,
encKey,
authKeyPair,
dataType,
skipStatePersist = false,
} = params;

const secretMetadata = new SecretMetadata(data, { dataType });
const { type } = secretMetadata;

// before encrypting and create backup, we will check the state if the secret data is already backed up
const backupState = this.getSecretDataBackupState(data, type);
if (backupState) {
return;
return secretMetadata;
}

const secretData = secretMetadata.toBytes();
Expand All @@ -1777,7 +1802,9 @@ export class SeedlessOnboardingController<
}

try {
await this.#withPersistedSecretMetadataBackupsState(async () => {
const persistMetadataFn = async (): Promise<
Omit<SocialBackupsMetadata, 'hash'> & { data: Uint8Array }
> => {
await this.toprfClient.addSecretDataItem({
encKey,
secretData,
Expand All @@ -1789,7 +1816,16 @@ export class SeedlessOnboardingController<
data,
type,
};
Comment on lines 1804 to 1818

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.

When skipStatePersist: true, we call persistMetadataFn directly and skip #withPersistedSecretMetadataBackupsState entirely. Does that wrapper do anything beyond persisting state: locking, error normalization, cleanup? If so, we're silently skipping it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

#withPersistedSecretMetadataBackupsState this only store the secret hash to the state.

});
};

// Skip local persistence if `skipStatePersist` is true.
if (skipStatePersist) {
await persistMetadataFn();
} else {
await this.#withPersistedSecretMetadataBackupsState(persistMetadataFn);
}

return secretMetadata;
} catch (error) {
if (this.#isAuthTokenError(error)) {
throw error;
Expand Down
1 change: 1 addition & 0 deletions packages/seedless-onboarding-controller/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export enum SeedlessOnboardingControllerErrorMessage {
InvalidVaultData = `${controllerName} - Invalid vault data`,
VaultDataError = `${controllerName} - The decrypted vault has an unexpected shape.`,
VaultError = `${controllerName} - Cannot unlock without a previous vault.`,
VaultAlreadyExists = `${controllerName} - Vault already exists.`,
InvalidSecretMetadata = `${controllerName} - Invalid secret metadata`,
MissingKeyringId = `${controllerName} - Keyring ID is required to store SRP backups.`,
FailedToEncryptAndStoreSecretData = `${controllerName} - Failed to encrypt and store secret data`,
Expand Down
Loading