Skip to content
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

fix: keyrings restore failure #5535

Merged
merged 13 commits into from
Mar 25, 2025
Merged

fix: keyrings restore failure #5535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
400c4b8
fix: add keyring to controller as last step when restoring
mikesposito Mar 24, 2025
cea598f
fix: clear `#unsupportedKeyrings` when clearing keyrings
mikesposito Mar 24, 2025
c0d5a15
fix: slice metadata in constructor
mikesposito Mar 24, 2025
14479a4
fix: metadata length check
mikesposito Mar 24, 2025
5c7e496
update lint rule threshold
mikesposito Mar 24, 2025
4955fc2
test: `submitPassword` with existing keyringsMetadata
mikesposito Mar 24, 2025
5b0b22f
fix: add metadata in the state after generating it
mikesposito Mar 24, 2025
ad1b253
test: add case for too many metadata objects
mikesposito Mar 24, 2025
b430926
fix: throw error when unlocking the wallet with too many metadata
mikesposito Mar 25, 2025
35af73b
Update packages/keyring-controller/src/KeyringController.ts
mikesposito Mar 25, 2025
b435e27
update changelog
mikesposito Mar 25, 2025
9c251df
fix changelog
mikesposito Mar 25, 2025
8a8331b
Merge branch 'main' into mikesposito/keyring-controller/restore-keyring
mikesposito Mar 25, 2025
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
2 changes: 1 addition & 1 deletion eslint-warning-thresholds.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@
},
"packages/keyring-controller/src/KeyringController.ts": {
"@typescript-eslint/no-unsafe-enum-comparison": 5,
"@typescript-eslint/no-unused-vars": 2
"@typescript-eslint/no-unused-vars": 1
},
"packages/keyring-controller/tests/mocks/mockKeyring.ts": {
"@typescript-eslint/prefer-readonly": 1
Expand Down
6 changes: 6 additions & 0 deletions packages/keyring-controller/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Fixed

- Fixed duplication of unsupported keyrings ([#5535](https://github.com/MetaMask/core/pull/5535))
- Enforce keyrings metadata alignment when unlocking existing vault ([#5535](https://github.com/MetaMask/core/pull/5535))
- Fixed frozen object mutation attempt when updating metadata ([#5535](https://github.com/MetaMask/core/pull/5535))

## [21.0.0]

### Changed
Expand Down
6 changes: 3 additions & 3 deletions packages/keyring-controller/jest.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ module.exports = merge(baseConfig, {
// An object that configures minimum threshold enforcement for coverage results
coverageThreshold: {
global: {
branches: 93.64,
branches: 93.18,
functions: 100,
lines: 98.76,
statements: 98.77,
lines: 98.62,
statements: 98.63,
},
},

Expand Down
210 changes: 123 additions & 87 deletions packages/keyring-controller/src/KeyringController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,17 +193,19 @@ describe('KeyringController', () => {
});

it('should throw an error if there is no primary keyring', async () => {
await withController(async ({ controller, encryptor }) => {
await controller.setLocked();
jest
.spyOn(encryptor, 'decrypt')
.mockResolvedValueOnce([{ type: 'Unsupported', data: '' }]);
await controller.submitPassword('123');
await withController(
{ skipVaultCreation: true, state: { vault: 'my vault' } },
async ({ controller, encryptor }) => {
jest
.spyOn(encryptor, 'decrypt')
.mockResolvedValueOnce([{ type: 'Unsupported', data: '' }]);
await controller.submitPassword('123');

await expect(controller.addNewAccount()).rejects.toThrow(
'No HD keyring found',
);
});
await expect(controller.addNewAccount()).rejects.toThrow(
'No HD keyring found',
);
},
);
});
});

Expand Down Expand Up @@ -260,46 +262,6 @@ describe('KeyringController', () => {
});
});

describe('when the keyringMetadata length is different from the number of keyrings', () => {
it('should throw an error if the keyring metadata length mismatch', async () => {
Comment on lines -263 to -264
Copy link
Member Author

Choose a reason for hiding this comment

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

This has been removed from addNewAccount test cases, because it should fail way earlier (i.e. when the controller is unlocked, as opposed to when adding an account)

const vaultWithOneKeyring = await withController(
async ({ controller }) => controller.state.vault,
);

await withController(
{
skipVaultCreation: true,
state: {
vault: vaultWithOneKeyring, // pass non-empty vault
keyringsMetadata: [
{ id: '1', name: '' },
{ id: '2', name: '' },
],
},
},
async ({ controller, encryptor }) => {
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
{
type: 'HD Key Tree',
data: {
keyrings: [
{
type: 'HD Key Tree',
accounts: ['0x123'],
},
],
},
},
]);
await controller.submitPassword(password);
await expect(controller.addNewAccount()).rejects.toThrow(
KeyringControllerError.KeyringMetadataLengthMismatch,
);
},
);
});
});

describe('addNewAccountForKeyring', () => {
describe('when accountCount is not provided', () => {
it('should add new account', async () => {
Expand Down Expand Up @@ -1088,21 +1050,23 @@ describe('KeyringController', () => {
});

it('should throw an error if there is no keyring', async () => {
await withController(async ({ controller, encryptor }) => {
await controller.setLocked();
jest
.spyOn(encryptor, 'decrypt')
.mockResolvedValueOnce([{ type: 'Unsupported', data: '' }]);
await controller.submitPassword('123');
await withController(
{ skipVaultCreation: true, state: { vault: 'my vault' } },
async ({ controller, encryptor }) => {
jest
.spyOn(encryptor, 'decrypt')
.mockResolvedValueOnce([{ type: 'Unsupported', data: '' }]);
await controller.submitPassword('123');

await expect(
controller.getKeyringForAccount(
'0x0000000000000000000000000000000000000000',
),
).rejects.toThrow(
'KeyringController - No keyring found. Error info: There are no keyrings',
);
});
await expect(
controller.getKeyringForAccount(
'0x0000000000000000000000000000000000000000',
),
).rejects.toThrow(
'KeyringController - No keyring found. Error info: There are no keyrings',
);
},
);
});

it('should throw an error if the controller is locked', async () => {
Expand Down Expand Up @@ -2586,9 +2550,12 @@ describe('KeyringController', () => {

it('should unlock also with unsupported keyrings', async () => {
await withController(
{ cacheEncryptionKey },
{
cacheEncryptionKey,
skipVaultCreation: true,
state: { vault: 'my vault' },
},
async ({ controller, encryptor }) => {
await controller.setLocked();
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
{
type: 'UnsupportedKeyring',
Expand All @@ -2605,9 +2572,12 @@ describe('KeyringController', () => {

it('should throw error if vault unlocked has an unexpected shape', async () => {
await withController(
{ cacheEncryptionKey },
{
cacheEncryptionKey,
skipVaultCreation: true,
state: { vault: 'my vault' },
},
async ({ controller, encryptor }) => {
await controller.setLocked();
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
{
foo: 'bar',
Expand All @@ -2632,6 +2602,60 @@ describe('KeyringController', () => {
);
});

it('should unlock succesfully when the controller is instantiated with an existing `keyringsMetadata`', async () => {
await withController(
{
cacheEncryptionKey,
state: { keyringsMetadata: [], vault: 'my vault' },
skipVaultCreation: true,
},
async ({ controller, encryptor }) => {
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
{
type: KeyringTypes.hd,
data: {
accounts: ['0x123'],
},
},
]);

await controller.submitPassword(password);

expect(controller.state.keyringsMetadata).toHaveLength(1);
},
);
});

it('should throw an error when the controller is instantiated with an existing `keyringsMetadata` with too many objects', async () => {
await withController(
{
cacheEncryptionKey,
state: {
keyringsMetadata: [
{ id: '123', name: '' },
{ id: '456', name: '' },
],
vault: 'my vault',
},
skipVaultCreation: true,
},
async ({ controller, encryptor }) => {
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
{
type: KeyringTypes.hd,
data: {
accounts: ['0x123'],
},
},
]);

await expect(controller.submitPassword(password)).rejects.toThrow(
KeyringControllerError.KeyringMetadataLengthMismatch,
);
},
);
});

!cacheEncryptionKey &&
it('should throw error if password is of wrong type', async () => {
await withController(
Expand Down Expand Up @@ -2676,9 +2700,17 @@ describe('KeyringController', () => {

it('should unlock also with unsupported keyrings', async () => {
await withController(
{ cacheEncryptionKey: true },
{
cacheEncryptionKey: true,
skipVaultCreation: true,
state: {
vault: JSON.stringify({ data: '0x123', salt: 'my salt' }),
// @ts-expect-error we want to force the controller to have an
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Strange that the constructor is typed to only allow a subset of state, seems like a mistake in the type definition. This shouldn't be an error.

Ah well, we can fix later.

// encryption salt equal to the one in the vault
encryptionSalt: 'my salt',
},
},
async ({ controller, initialState, encryptor }) => {
await controller.setLocked();
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
{
type: 'UnsupportedKeyring',
Expand Down Expand Up @@ -2805,17 +2837,19 @@ describe('KeyringController', () => {
});

it('should throw an error if there is no primary keyring', async () => {
await withController(async ({ controller, encryptor }) => {
await controller.setLocked();
jest
.spyOn(encryptor, 'decrypt')
.mockResolvedValueOnce([{ type: 'Unsupported', data: '' }]);
await controller.submitPassword('123');
await withController(
{ skipVaultCreation: true, state: { vault: 'my vault' } },
async ({ controller, encryptor }) => {
jest
.spyOn(encryptor, 'decrypt')
.mockResolvedValueOnce([{ type: 'Unsupported', data: '' }]);
await controller.submitPassword('123');

await expect(controller.verifySeedPhrase()).rejects.toThrow(
KeyringControllerError.KeyringNotFound,
);
});
await expect(controller.verifySeedPhrase()).rejects.toThrow(
KeyringControllerError.KeyringNotFound,
);
},
);
});

it('should throw error when the controller is locked', async () => {
Expand Down Expand Up @@ -4208,18 +4242,20 @@ describe('KeyringController', () => {
it('should rollback the controller keyrings if the keyring creation fails', async () => {
const mockAddress = '0x4584d2B4905087A100420AFfCe1b2d73fC69B8E4';
stubKeyringClassWithAccount(MockKeyring, mockAddress);

// Mocking the serialize method to throw an error will
// halt the controller everytime it tries to persist the keyring,
// making it impossible to update the vault
jest
.spyOn(MockKeyring.prototype, 'serialize')
.mockImplementation(async () => {
throw new Error('You will never be able to persist me!');
});
await withController(
{ keyringBuilders: [keyringBuilderFactory(MockKeyring)] },
async ({ controller, initialState }) => {
// We're mocking BaseController .update() to throw an error, as it's the last operation
// that is called before the function is rolled back.
jest.spyOn(controller, 'update' as never).mockImplementation(() => {
throw new Error('You will never be able to change me!');
});
await expect(
controller.addNewKeyring(MockKeyring.type),
).rejects.toThrow('You will never be able to change me!');
).rejects.toThrow('You will never be able to persist me!');

expect(controller.state).toStrictEqual(initialState);
await expect(
Expand Down
Loading
Loading