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 diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index e4004126882..8dd04bb80f4 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 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 438530bf346..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', @@ -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( @@ -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 + // 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', @@ -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 () => { @@ -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( diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 2dd18fece54..37e779be5be 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; @@ -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 @@ -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); + } } /** @@ -2473,6 +2477,7 @@ export class KeyringController extends BaseController< await this.#destroyKeyring(keyring); } this.#keyrings = []; + this.#unsupportedKeyrings = []; } /** @@ -2490,15 +2495,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) { + 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; } @@ -2597,6 +2605,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`); } @@ -2651,9 +2664,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; }