From 400c4b852ad9642cc9143eb0fbb46c1413d57450 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Mon, 24 Mar 2025 19:14:31 +0100 Subject: [PATCH 01/12] fix: add keyring to controller as last step when restoring --- packages/keyring-controller/src/KeyringController.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 2dd18fece54..ebaa3740c40 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2490,15 +2490,18 @@ export class KeyringController extends BaseController< try { const { type, data } = serialized; const keyring = await this.#createKeyring(type, data); - this.#keyrings.push(keyring); // If metadata is missing, assume the data is from an installation before // we had keyring metadata. if (this.#keyringsMetadata.length < this.#keyrings.length) { console.log(`Adding missing metadata for '${type}' keyring`); this.#keyringsMetadata.push(getDefaultKeyringMetadata()); } + // The keyring is added to the keyrings array only if it's successfully restored + // and the metadata is successfully added to the controller + this.#keyrings.push(keyring); return keyring; - } catch (_) { + } catch (error) { + console.error(error); this.#unsupportedKeyrings.push(serialized); return undefined; } From cea598fa638d12d088bc52ec7726a0ed7ab57609 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Mon, 24 Mar 2025 19:19:38 +0100 Subject: [PATCH 02/12] fix: clear `#unsupportedKeyrings` when clearing keyrings --- packages/keyring-controller/src/KeyringController.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index ebaa3740c40..70b4f5fac9e 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -620,14 +620,14 @@ export class KeyringController extends BaseController< readonly #keyringBuilders: { (): EthKeyring; type: string }[]; - readonly #unsupportedKeyrings: SerializedKeyring[]; - readonly #encryptor: GenericEncryptor | ExportableKeyEncryptor; readonly #cacheEncryptionKey: boolean; #keyrings: EthKeyring[]; + #unsupportedKeyrings: SerializedKeyring[]; + #keyringsMetadata: KeyringMetadata[]; #password?: string; @@ -2473,6 +2473,7 @@ export class KeyringController extends BaseController< await this.#destroyKeyring(keyring); } this.#keyrings = []; + this.#unsupportedKeyrings = []; } /** From c0d5a1573a01724fae312975156f0207af0179b1 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Mon, 24 Mar 2025 20:15:21 +0100 Subject: [PATCH 03/12] fix: slice metadata in constructor --- packages/keyring-controller/src/KeyringController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 70b4f5fac9e..9018f2f0375 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -677,7 +677,7 @@ export class KeyringController extends BaseController< this.#encryptor = encryptor; this.#keyrings = []; - this.#keyringsMetadata = state?.keyringsMetadata ?? []; + this.#keyringsMetadata = state?.keyringsMetadata?.slice() ?? []; this.#unsupportedKeyrings = []; // This option allows the controller to cache an exported key From 14479a4b1d70571cea3af870adc99aa18dfecd0e Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Mon, 24 Mar 2025 20:32:10 +0100 Subject: [PATCH 04/12] fix: metadata length check --- packages/keyring-controller/src/KeyringController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 9018f2f0375..e0996b2223b 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2493,7 +2493,7 @@ export class KeyringController extends BaseController< const keyring = await this.#createKeyring(type, data); // If metadata is missing, assume the data is from an installation before // we had keyring metadata. - if (this.#keyringsMetadata.length < this.#keyrings.length) { + if (this.#keyringsMetadata.length <= this.#keyrings.length) { console.log(`Adding missing metadata for '${type}' keyring`); this.#keyringsMetadata.push(getDefaultKeyringMetadata()); } From 5c7e49695a26cf2eceb4ec8fb659f19ebcbc9749 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Mon, 24 Mar 2025 20:39:30 +0100 Subject: [PATCH 05/12] update lint rule threshold --- eslint-warning-thresholds.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eslint-warning-thresholds.json b/eslint-warning-thresholds.json index 192779b6586..cc14b1131aa 100644 --- a/eslint-warning-thresholds.json +++ b/eslint-warning-thresholds.json @@ -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 From 4955fc249f1ce29b9a966fb9fd89d052bb1f4823 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Mon, 24 Mar 2025 21:29:21 +0100 Subject: [PATCH 06/12] test: `submitPassword` with existing keyringsMetadata --- .../src/KeyringController.test.ts | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 438530bf346..bed5f362455 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -2632,6 +2632,30 @@ 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); + }, + ); + }); + !cacheEncryptionKey && it('should throw error if password is of wrong type', async () => { await withController( From 5b0b22fd703b00da56bb209d3669f91f241be936 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Mon, 24 Mar 2025 21:43:01 +0100 Subject: [PATCH 07/12] fix: add metadata in the state after generating it --- packages/keyring-controller/src/KeyringController.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index e0996b2223b..4cea22c072e 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2601,6 +2601,11 @@ export class KeyringController extends BaseController< this.update((state) => { state.isUnlocked = true; + // If new keyringsMetadata was generated during the unlock operation, + // we'll have to update the state with the new array + if (this.#keyringsMetadata.length > state.keyringsMetadata.length) { + state.keyringsMetadata = this.#keyringsMetadata.slice(); + } }); this.messagingSystem.publish(`${name}:unlock`); } From ad1b253f449d22a34dea31879df701f7e1dcf39d Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Mon, 24 Mar 2025 22:16:29 +0100 Subject: [PATCH 08/12] test: add case for too many metadata objects --- .../src/KeyringController.test.ts | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index bed5f362455..a351168b140 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -2656,6 +2656,36 @@ describe('KeyringController', () => { ); }); + 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( From b430926592e013fa77239f73a6dcc59f1a2082b5 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 25 Mar 2025 13:00:44 +0100 Subject: [PATCH 09/12] fix: throw error when unlocking the wallet with too many metadata --- packages/keyring-controller/jest.config.js | 6 +- .../src/KeyringController.test.ts | 156 ++++++++---------- .../src/KeyringController.ts | 9 +- 3 files changed, 79 insertions(+), 92 deletions(-) diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js index 0923b71addf..f67f6e2a6f7 100644 --- a/packages/keyring-controller/jest.config.js +++ b/packages/keyring-controller/jest.config.js @@ -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, }, }, diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index a351168b140..813181d0b48 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -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', + ); + }, + ); }); }); @@ -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 () => { - 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 () => { @@ -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 () => { @@ -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', @@ -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', @@ -2730,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 + // 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', @@ -2859,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 () => { @@ -4262,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( diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 4cea22c072e..8b2a7681290 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2155,6 +2155,10 @@ export class KeyringController extends BaseController< for (const serializedKeyring of serializedKeyrings) { await this.#restoreKeyring(serializedKeyring); } + + if (this.#keyrings.length !== this.#keyringsMetadata.length) { + throw new Error(KeyringControllerError.KeyringMetadataLengthMismatch); + } } /** @@ -2397,6 +2401,7 @@ export class KeyringController extends BaseController< this.#keyrings.push(keyring); this.#keyringsMetadata.push(getDefaultKeyringMetadata()); + return keyring; } @@ -2660,9 +2665,9 @@ export class KeyringController extends BaseController< return await callback({ releaseLock }); } catch (e) { // Keyrings and password are restored to their previous state - await this.#restoreSerializedKeyrings(currentSerializedKeyrings); - this.#password = currentPassword; this.#keyringsMetadata = currentKeyringsMetadata; + this.#password = currentPassword; + await this.#restoreSerializedKeyrings(currentSerializedKeyrings); throw e; } From 35af73be649512b7c4f4d8bdef135045bd127232 Mon Sep 17 00:00:00 2001 From: Michele Esposito <34438276+mikesposito@users.noreply.github.com> Date: Tue, 25 Mar 2025 13:06:07 +0100 Subject: [PATCH 10/12] Update packages/keyring-controller/src/KeyringController.ts --- packages/keyring-controller/src/KeyringController.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 8b2a7681290..37e779be5be 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2401,7 +2401,6 @@ export class KeyringController extends BaseController< this.#keyrings.push(keyring); this.#keyringsMetadata.push(getDefaultKeyringMetadata()); - return keyring; } From b435e276343d5527f16669a822ab7c0061a34039 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 25 Mar 2025 13:16:00 +0100 Subject: [PATCH 11/12] update changelog --- packages/keyring-controller/CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index e4004126882..dedda1181fe 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -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 From 9c251dfcd0646dfabf9f11842c104ea50fb76622 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 25 Mar 2025 13:24:44 +0100 Subject: [PATCH 12/12] fix changelog --- packages/keyring-controller/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index dedda1181fe..8dd04bb80f4 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -11,7 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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)) +- Fixed frozen object mutation attempt when updating metadata ([#5535](https://github.com/MetaMask/core/pull/5535)) ## [21.0.0]