Skip to content

Commit a8b57d4

Browse files
mikespositomikesposito
committed
[19.x] Backport fix: keyrings restore failure 
1 parent fb19332 commit a8b57d4

File tree

5 files changed

+154
-99
lines changed

5 files changed

+154
-99
lines changed

‎eslint-warning-thresholds.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,7 @@
247247
},
248248
"packages/keyring-controller/src/KeyringController.ts": {
249249
"@typescript-eslint/no-unsafe-enum-comparison": 5,
250-
"@typescript-eslint/no-unused-vars": 2
250+
"@typescript-eslint/no-unused-vars": 1
251251
},
252252
"packages/keyring-controller/tests/mocks/mockKeyring.ts": {
253253
"@typescript-eslint/prefer-readonly": 1

‎packages/keyring-controller/CHANGELOG.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Fixed
11+
12+
- Fixed duplication of unsupported keyrings ([#5535](https://github.com/MetaMask/core/pull/5535))
13+
- Enforce keyrings metadata alignment when unlocking existing vault ([#5535](https://github.com/MetaMask/core/pull/5535))
14+
- Fixed frozen object mutation attempt when updating metadata ([#5535](https://github.com/MetaMask/core/pull/5535))
15+
1016
## [19.2.1]
1117

1218
### Changed

‎packages/keyring-controller/jest.config.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,10 @@ module.exports = merge(baseConfig, {
1717
// An object that configures minimum threshold enforcement for coverage results
1818
coverageThreshold: {
1919
global: {
20-
branches: 93.56,
20+
branches: 93.14,
2121
functions: 100,
22-
lines: 98.73,
23-
statements: 98.74,
22+
lines: 98.61,
23+
statements: 98.62,
2424
},
2525
},
2626

‎packages/keyring-controller/src/KeyringController.test.ts‎

Lines changed: 123 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -198,17 +198,19 @@ describe('KeyringController', () => {
198198
});
199199

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

208-
await expect(controller.addNewAccount()).rejects.toThrow(
209-
'No HD keyring found',
210-
);
211-
});
209+
await expect(controller.addNewAccount()).rejects.toThrow(
210+
'No HD keyring found',
211+
);
212+
},
213+
);
212214
});
213215
});
214216

@@ -265,46 +267,6 @@ describe('KeyringController', () => {
265267
});
266268
});
267269

268-
describe('when the keyringMetadata length is different from the number of keyrings', () => {
269-
it('should throw an error if the keyring metadata length mismatch', async () => {
270-
const vaultWithOneKeyring = await withController(
271-
async ({ controller }) => controller.state.vault,
272-
);
273-
274-
await withController(
275-
{
276-
skipVaultCreation: true,
277-
state: {
278-
vault: vaultWithOneKeyring, // pass non-empty vault
279-
keyringsMetadata: [
280-
{ id: '1', name: '' },
281-
{ id: '2', name: '' },
282-
],
283-
},
284-
},
285-
async ({ controller, encryptor }) => {
286-
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
287-
{
288-
type: 'HD Key Tree',
289-
data: {
290-
keyrings: [
291-
{
292-
type: 'HD Key Tree',
293-
accounts: ['0x123'],
294-
},
295-
],
296-
},
297-
},
298-
]);
299-
await controller.submitPassword(password);
300-
await expect(controller.addNewAccount()).rejects.toThrow(
301-
KeyringControllerError.KeyringMetadataLengthMismatch,
302-
);
303-
},
304-
);
305-
});
306-
});
307-
308270
describe('addNewAccountForKeyring', () => {
309271
describe('when accountCount is not provided', () => {
310272
it('should add new account', async () => {
@@ -1083,21 +1045,23 @@ describe('KeyringController', () => {
10831045
});
10841046

10851047
it('should throw an error if there is no keyring', async () => {
1086-
await withController(async ({ controller, encryptor }) => {
1087-
await controller.setLocked();
1088-
jest
1089-
.spyOn(encryptor, 'decrypt')
1090-
.mockResolvedValueOnce([{ type: 'Unsupported', data: '' }]);
1091-
await controller.submitPassword('123');
1048+
await withController(
1049+
{ skipVaultCreation: true, state: { vault: 'my vault' } },
1050+
async ({ controller, encryptor }) => {
1051+
jest
1052+
.spyOn(encryptor, 'decrypt')
1053+
.mockResolvedValueOnce([{ type: 'Unsupported', data: '' }]);
1054+
await controller.submitPassword('123');
10921055

1093-
await expect(
1094-
controller.getKeyringForAccount(
1095-
'0x0000000000000000000000000000000000000000',
1096-
),
1097-
).rejects.toThrow(
1098-
'KeyringController - No keyring found. Error info: There are no keyrings',
1099-
);
1100-
});
1056+
await expect(
1057+
controller.getKeyringForAccount(
1058+
'0x0000000000000000000000000000000000000000',
1059+
),
1060+
).rejects.toThrow(
1061+
'KeyringController - No keyring found. Error info: There are no keyrings',
1062+
);
1063+
},
1064+
);
11011065
});
11021066

11031067
it('should throw an error if the controller is locked', async () => {
@@ -2579,9 +2543,12 @@ describe('KeyringController', () => {
25792543

25802544
it('should unlock also with unsupported keyrings', async () => {
25812545
await withController(
2582-
{ cacheEncryptionKey },
2546+
{
2547+
cacheEncryptionKey,
2548+
skipVaultCreation: true,
2549+
state: { vault: 'my vault' },
2550+
},
25832551
async ({ controller, encryptor }) => {
2584-
await controller.setLocked();
25852552
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
25862553
{
25872554
type: 'UnsupportedKeyring',
@@ -2598,9 +2565,12 @@ describe('KeyringController', () => {
25982565

25992566
it('should throw error if vault unlocked has an unexpected shape', async () => {
26002567
await withController(
2601-
{ cacheEncryptionKey },
2568+
{
2569+
cacheEncryptionKey,
2570+
skipVaultCreation: true,
2571+
state: { vault: 'my vault' },
2572+
},
26022573
async ({ controller, encryptor }) => {
2603-
await controller.setLocked();
26042574
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
26052575
{
26062576
foo: 'bar',
@@ -2625,6 +2595,60 @@ describe('KeyringController', () => {
26252595
);
26262596
});
26272597

2598+
it('should unlock succesfully when the controller is instantiated with an existing `keyringsMetadata`', async () => {
2599+
await withController(
2600+
{
2601+
cacheEncryptionKey,
2602+
state: { keyringsMetadata: [], vault: 'my vault' },
2603+
skipVaultCreation: true,
2604+
},
2605+
async ({ controller, encryptor }) => {
2606+
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
2607+
{
2608+
type: KeyringTypes.hd,
2609+
data: {
2610+
accounts: ['0x123'],
2611+
},
2612+
},
2613+
]);
2614+
2615+
await controller.submitPassword(password);
2616+
2617+
expect(controller.state.keyringsMetadata).toHaveLength(1);
2618+
},
2619+
);
2620+
});
2621+
2622+
it('should throw an error when the controller is instantiated with an existing `keyringsMetadata` with too many objects', async () => {
2623+
await withController(
2624+
{
2625+
cacheEncryptionKey,
2626+
state: {
2627+
keyringsMetadata: [
2628+
{ id: '123', name: '' },
2629+
{ id: '456', name: '' },
2630+
],
2631+
vault: 'my vault',
2632+
},
2633+
skipVaultCreation: true,
2634+
},
2635+
async ({ controller, encryptor }) => {
2636+
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
2637+
{
2638+
type: KeyringTypes.hd,
2639+
data: {
2640+
accounts: ['0x123'],
2641+
},
2642+
},
2643+
]);
2644+
2645+
await expect(controller.submitPassword(password)).rejects.toThrow(
2646+
KeyringControllerError.KeyringMetadataLengthMismatch,
2647+
);
2648+
},
2649+
);
2650+
});
2651+
26282652
!cacheEncryptionKey &&
26292653
it('should throw error if password is of wrong type', async () => {
26302654
await withController(
@@ -2669,9 +2693,17 @@ describe('KeyringController', () => {
26692693

26702694
it('should unlock also with unsupported keyrings', async () => {
26712695
await withController(
2672-
{ cacheEncryptionKey: true },
2696+
{
2697+
cacheEncryptionKey: true,
2698+
skipVaultCreation: true,
2699+
state: {
2700+
vault: JSON.stringify({ data: '0x123', salt: 'my salt' }),
2701+
// @ts-expect-error we want to force the controller to have an
2702+
// encryption salt equal to the one in the vault
2703+
encryptionSalt: 'my salt',
2704+
},
2705+
},
26732706
async ({ controller, initialState, encryptor }) => {
2674-
await controller.setLocked();
26752707
jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([
26762708
{
26772709
type: 'UnsupportedKeyring',
@@ -2798,17 +2830,19 @@ describe('KeyringController', () => {
27982830
});
27992831

28002832
it('should throw an error if there is no primary keyring', async () => {
2801-
await withController(async ({ controller, encryptor }) => {
2802-
await controller.setLocked();
2803-
jest
2804-
.spyOn(encryptor, 'decrypt')
2805-
.mockResolvedValueOnce([{ type: 'Unsupported', data: '' }]);
2806-
await controller.submitPassword('123');
2833+
await withController(
2834+
{ skipVaultCreation: true, state: { vault: 'my vault' } },
2835+
async ({ controller, encryptor }) => {
2836+
jest
2837+
.spyOn(encryptor, 'decrypt')
2838+
.mockResolvedValueOnce([{ type: 'Unsupported', data: '' }]);
2839+
await controller.submitPassword('123');
28072840

2808-
await expect(controller.verifySeedPhrase()).rejects.toThrow(
2809-
KeyringControllerError.KeyringNotFound,
2810-
);
2811-
});
2841+
await expect(controller.verifySeedPhrase()).rejects.toThrow(
2842+
KeyringControllerError.KeyringNotFound,
2843+
);
2844+
},
2845+
);
28122846
});
28132847

28142848
it('should throw error when the controller is locked', async () => {
@@ -4191,18 +4225,20 @@ describe('KeyringController', () => {
41914225
it('should rollback the controller keyrings if the keyring creation fails', async () => {
41924226
const mockAddress = '0x4584d2B4905087A100420AFfCe1b2d73fC69B8E4';
41934227
stubKeyringClassWithAccount(MockKeyring, mockAddress);
4194-
4228+
// Mocking the serialize method to throw an error will
4229+
// halt the controller everytime it tries to persist the keyring,
4230+
// making it impossible to update the vault
4231+
jest
4232+
.spyOn(MockKeyring.prototype, 'serialize')
4233+
.mockImplementation(async () => {
4234+
throw new Error('You will never be able to persist me!');
4235+
});
41954236
await withController(
41964237
{ keyringBuilders: [keyringBuilderFactory(MockKeyring)] },
41974238
async ({ controller, initialState }) => {
4198-
// We're mocking BaseController .update() to throw an error, as it's the last operation
4199-
// that is called before the function is rolled back.
4200-
jest.spyOn(controller, 'update' as never).mockImplementation(() => {
4201-
throw new Error('You will never be able to change me!');
4202-
});
42034239
await expect(
42044240
controller.addNewKeyring(MockKeyring.type),
4205-
).rejects.toThrow('You will never be able to change me!');
4241+
).rejects.toThrow('You will never be able to persist me!');
42064242

42074243
expect(controller.state).toStrictEqual(initialState);
42084244
await expect(

0 commit comments

Comments
 (0)