diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 08e726c34fb..1ebaf55af28 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -37,7 +37,9 @@ import { keyringBuilderFactory, } from './KeyringController'; import MockEncryptor, { + DECRYPTION_ERROR, MOCK_ENCRYPTION_KEY, + SALT, } from '../tests/mocks/mockEncryptor'; import { MockErc4337Keyring } from '../tests/mocks/mockErc4337Keyring'; import { MockKeyring } from '../tests/mocks/mockKeyring'; @@ -67,6 +69,8 @@ const uint8ArraySeed = new Uint8Array( const privateKey = '1e4e6a4c0c077f4ae8ddfbf372918e61dd0fb4a4cfa592cb16e7546d505e68fc'; const password = 'password123'; +const freshVault = + '{"data":"{\\"tag\\":{\\"key\\":{\\"password\\":\\"password123\\",\\"salt\\":\\"salt\\"},\\"iv\\":\\"iv\\"},\\"value\\":[{\\"type\\":\\"HD Key Tree\\",\\"data\\":{\\"mnemonic\\":[119,97,114,114,105,111,114,32,108,97,110,103,117,97,103,101,32,106,111,107,101,32,98,111,110,117,115,32,117,110,102,97,105,114,32,97,114,116,105,115,116,32,107,97,110,103,97,114,111,111,32,99,105,114,99,108,101,32,101,120,112,97,110,100,32,104,111,112,101,32,109,105,100,100,108,101,32,103,97,117,103,101],\\"numberOfAccounts\\":1,\\"hdPath\\":\\"m/44\'/60\'/0\'/0\\"},\\"metadata\\":{\\"id\\":\\"01JXEFM7DAX2VJ0YFR4ESNY3GQ\\",\\"name\\":\\"\\"}}]}","iv":"iv","salt":"salt"}'; const commonConfig = { chain: Chain.Goerli, hardfork: Hardfork.Berlin }; @@ -553,7 +557,6 @@ describe('KeyringController', () => { await withController( { cacheEncryptionKey }, async ({ controller, initialState }) => { - const initialVault = controller.state.vault; const initialKeyrings = controller.state.keyrings; await controller.createNewVaultAndRestore( password, @@ -561,7 +564,6 @@ describe('KeyringController', () => { ); expect(controller.state).not.toBe(initialState); expect(controller.state.vault).toBeDefined(); - expect(controller.state.vault).toStrictEqual(initialVault); expect(controller.state.keyrings).toHaveLength( initialKeyrings.length, ); @@ -577,7 +579,7 @@ describe('KeyringController', () => { await withController( { cacheEncryptionKey }, async ({ controller, encryptor }) => { - const encryptSpy = jest.spyOn(encryptor, 'encrypt'); + const encryptSpy = jest.spyOn(encryptor, 'encryptWithKey'); const serializedKeyring = await controller.withKeyring( { type: 'HD Key Tree' }, async ({ keyring }) => keyring.serialize(), @@ -590,7 +592,8 @@ describe('KeyringController', () => { currentSeedWord, ); - expect(encryptSpy).toHaveBeenCalledWith(password, [ + const key = JSON.parse(MOCK_ENCRYPTION_KEY); + expect(encryptSpy).toHaveBeenCalledWith(key, [ { data: serializedKeyring, type: 'HD Key Tree', @@ -1302,7 +1305,15 @@ describe('KeyringController', () => { }, ], }; - expect(controller.state).toStrictEqual(modifiedState); + const modifiedStateWithoutVault = { + ...modifiedState, + vault: undefined, + }; + const stateWithoutVault = { + ...controller.state, + vault: undefined, + }; + expect(stateWithoutVault).toStrictEqual(modifiedStateWithoutVault); expect(importedAccountAddress).toBe(address); }); }); @@ -1381,7 +1392,15 @@ describe('KeyringController', () => { }, ], }; - expect(controller.state).toStrictEqual(modifiedState); + const modifiedStateWithoutVault = { + ...modifiedState, + vault: undefined, + }; + const stateWithoutVault = { + ...controller.state, + vault: undefined, + }; + expect(stateWithoutVault).toStrictEqual(modifiedStateWithoutVault); expect(importedAccountAddress).toBe(address); }); }); @@ -2678,10 +2697,10 @@ describe('KeyringController', () => { { cacheEncryptionKey, skipVaultCreation: true, - state: { vault: 'my vault' }, + state: { vault: freshVault }, }, async ({ controller, encryptor }) => { - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: 'UnsupportedKeyring', data: '0x1234', @@ -2700,10 +2719,10 @@ describe('KeyringController', () => { { cacheEncryptionKey, skipVaultCreation: true, - state: { vault: 'my vault' }, + state: { vault: freshVault }, }, async ({ controller, encryptor }) => { - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { foo: 'bar', }, @@ -2733,11 +2752,11 @@ describe('KeyringController', () => { await withController( { cacheEncryptionKey, - state: { vault: 'my vault' }, + state: { vault: freshVault }, skipVaultCreation: true, }, async ({ controller, encryptor }) => { - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: KeyringTypes.hd, data: { @@ -2776,7 +2795,7 @@ describe('KeyringController', () => { { cacheEncryptionKey: true, state: { - vault: 'my vault', + vault: freshVault, }, skipVaultCreation: true, }, @@ -2787,9 +2806,8 @@ describe('KeyringController', () => { ); jest .spyOn(encryptor, 'importKey') - // @ts-expect-error we are assigning a mock value .mockResolvedValue('imported key'); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: KeyringTypes.hd, data: { @@ -2840,7 +2858,7 @@ describe('KeyringController', () => { { cacheEncryptionKey: false, state: { - vault: 'my vault', + vault: freshVault, }, skipVaultCreation: true, }, @@ -2895,14 +2913,14 @@ describe('KeyringController', () => { { skipVaultCreation: true, cacheEncryptionKey, - state: { vault: 'my vault' }, + state: { vault: freshVault }, keyringBuilders: [keyringBuilderFactory(MockKeyring)], }, async ({ controller, encryptor, messenger }) => { const unlockListener = jest.fn(); messenger.subscribe('KeyringController:unlock', unlockListener); jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: KeyringTypes.hd, data: {}, @@ -2926,12 +2944,12 @@ describe('KeyringController', () => { { skipVaultCreation: true, cacheEncryptionKey, - state: { vault: 'my vault' }, + state: { vault: freshVault }, }, async ({ controller, encryptor }) => { jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false); jest.spyOn(encryptor, 'encrypt').mockRejectedValue(new Error()); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: KeyringTypes.hd, data: { @@ -2955,13 +2973,13 @@ describe('KeyringController', () => { { skipVaultCreation: true, cacheEncryptionKey, - state: { vault: 'my vault' }, + state: { vault: freshVault }, keyringBuilders: [keyringBuilderFactory(MockKeyring)], }, async ({ controller, encryptor, messenger }) => { const unlockListener = jest.fn(); messenger.subscribe('KeyringController:unlock', unlockListener); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: KeyringTypes.hd, data: {}, @@ -2986,12 +3004,12 @@ describe('KeyringController', () => { { skipVaultCreation: true, cacheEncryptionKey, - state: { vault: 'my vault' }, + state: { vault: freshVault }, }, async ({ controller, encryptor }) => { jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false); - const encryptSpy = jest.spyOn(encryptor, 'encrypt'); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + const encryptSpy = jest.spyOn(encryptor, 'encryptWithKey'); + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: KeyringTypes.hd, data: { @@ -3013,12 +3031,12 @@ describe('KeyringController', () => { { skipVaultCreation: true, cacheEncryptionKey, - state: { vault: 'my vault' }, + state: { vault: freshVault }, }, async ({ controller, encryptor }) => { jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(true); const encryptSpy = jest.spyOn(encryptor, 'encrypt'); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: KeyringTypes.hd, data: { @@ -3027,6 +3045,10 @@ describe('KeyringController', () => { }, ]); + // TODO actually this does trigger re-encryption. The catch is + // that this test is run with cacheEncryptionKey enabled, so + // `encryptWithKey` is being used instead of `encrypt`. Hence, + // the spy on `encrypt` doesn't trigger. await controller.submitPassword(password); expect(encryptSpy).not.toHaveBeenCalled(); @@ -3040,7 +3062,7 @@ describe('KeyringController', () => { { skipVaultCreation: true, cacheEncryptionKey, - state: { vault: 'my vault' }, + state: { vault: freshVault }, }, async ({ controller, encryptor }) => { jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false); @@ -3066,7 +3088,7 @@ describe('KeyringController', () => { { skipVaultCreation: true, cacheEncryptionKey, - state: { vault: 'my vault' }, + state: { vault: freshVault }, }, async ({ controller, encryptor }) => { jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(true); @@ -3119,14 +3141,19 @@ describe('KeyringController', () => { it('should throw error when using the wrong password', async () => { await withController( { - skipVaultCreation: true, cacheEncryptionKey, - state: { vault: 'my vault' }, + skipVaultCreation: true, + state: { + vault: freshVault, + // @ts-expect-error we want to force the controller to have an + // encryption salt equal to the one in the vault + encryptionSalt: SALT, + }, }, async ({ controller }) => { await expect( controller.submitPassword('wrong password'), - ).rejects.toThrow('Incorrect password.'); + ).rejects.toThrow(DECRYPTION_ERROR); }, ); }); @@ -3154,14 +3181,14 @@ describe('KeyringController', () => { cacheEncryptionKey: true, skipVaultCreation: true, state: { - vault: JSON.stringify({ data: '0x123', salt: 'my salt' }), + vault: freshVault, // @ts-expect-error we want to force the controller to have an // encryption salt equal to the one in the vault - encryptionSalt: 'my salt', + encryptionSalt: SALT, }, }, async ({ controller, initialState, encryptor }) => { - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: 'UnsupportedKeyring', data: '0x1234', @@ -3188,19 +3215,15 @@ describe('KeyringController', () => { cacheEncryptionKey: true, skipVaultCreation: true, state: { - vault: JSON.stringify({ data: '0x123', salt: 'my salt' }), + vault: freshVault, // @ts-expect-error we want to force the controller to have an // encryption salt equal to the one in the vault - encryptionSalt: 'my salt', + encryptionSalt: SALT, }, }, async ({ controller, initialState, encryptor }) => { const encryptWithKeySpy = jest.spyOn(encryptor, 'encryptWithKey'); - jest - .spyOn(encryptor, 'importKey') - // @ts-expect-error we are assigning a mock value - .mockResolvedValue('imported key'); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: KeyringTypes.hd, data: '0x123', @@ -3213,18 +3236,21 @@ describe('KeyringController', () => { ); expect(controller.state.isUnlocked).toBe(true); - expect(encryptWithKeySpy).toHaveBeenCalledWith('imported key', [ - { - type: KeyringTypes.hd, - data: { - accounts: ['0x123'], - }, - metadata: { - id: expect.any(String), - name: '', + expect(encryptWithKeySpy).toHaveBeenCalledWith( + JSON.parse(MOCK_ENCRYPTION_KEY), + [ + { + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], + }, + metadata: { + id: expect.any(String), + name: '', + }, }, - }, - ]); + ], + ); }, ); }); @@ -3233,7 +3259,7 @@ describe('KeyringController', () => { await withController( { cacheEncryptionKey: true }, async ({ controller, initialState, encryptor }) => { - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { foo: 'bar', }, diff --git a/packages/keyring-controller/tests/mocks/mockEncryptor.ts b/packages/keyring-controller/tests/mocks/mockEncryptor.ts index 034d9e32d1d..e8aaf09d81a 100644 --- a/packages/keyring-controller/tests/mocks/mockEncryptor.ts +++ b/packages/keyring-controller/tests/mocks/mockEncryptor.ts @@ -1,84 +1,104 @@ +// Omitting jsdoc because mock is only internal and simple enough. +/* eslint-disable jsdoc/require-jsdoc */ + +import type { + DetailedDecryptResult, + DetailedEncryptionResult, + EncryptionResult, +} from '@metamask/browser-passworder'; +import type { Json } from '@metamask/utils'; +import { isEqual } from 'lodash'; + import type { ExportableKeyEncryptor } from '../../src/KeyringController'; export const PASSWORD = 'password123'; +export const SALT = 'salt'; export const MOCK_ENCRYPTION_KEY = JSON.stringify({ - alg: 'A256GCM', - ext: true, - k: 'wYmxkxOOFBDP6F6VuuYFcRt_Po-tSLFHCWVolsHs4VI', - // eslint-disable-next-line @typescript-eslint/naming-convention - key_ops: ['encrypt', 'decrypt'], - kty: 'oct', + password: PASSWORD, + salt: SALT, }); -export const MOCK_ENCRYPTION_SALT = - 'HQ5sfhsb8XAQRJtD+UqcImT7Ve4n3YMagrh05YTOsjk='; -export const MOCK_HARDCODED_KEY = 'key'; -export const MOCK_HEX = '0xabcdef0123456789'; -// eslint-disable-next-line no-restricted-globals -const MOCK_KEY = Buffer.alloc(32); -const INVALID_PASSWORD_ERROR = 'Incorrect password.'; -export default class MockEncryptor implements ExportableKeyEncryptor { - cacheVal?: string; +export const DECRYPTION_ERROR = 'Decryption failed.'; + +function deriveKey(password: string, salt: string) { + return { + password, + salt, + }; +} - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - async encrypt(password: string, dataObj: any) { +export default class MockEncryptor implements ExportableKeyEncryptor { + async encrypt(password: string, dataObj: Json): Promise { + const salt = generateSalt(); + const key = deriveKey(password, salt); + const result = await this.encryptWithKey(key, dataObj); return JSON.stringify({ - ...(await this.encryptWithKey(password, dataObj)), - salt: this.generateSalt(), + ...result, + salt, }); } - async decrypt(_password: string, _text: string) { - if (_password && _password !== PASSWORD) { - throw new Error(INVALID_PASSWORD_ERROR); - } - - return JSON.parse(this.cacheVal || '') ?? {}; + async decrypt(password: string, text: string): Promise { + const { salt } = JSON.parse(text); + const key = deriveKey(password, salt); + return await this.decryptWithKey(key, text); } - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - async encryptWithKey(_key: unknown, dataObj: any) { - this.cacheVal = JSON.stringify(dataObj); + async encryptWithDetail( + password: string, + dataObj: Json, + salt?: string, + ): Promise { + const _salt = salt ?? generateSalt(); + const key = deriveKey(password, _salt); + const result = await this.encryptWithKey(key, dataObj); return { - data: MOCK_HEX, - iv: 'anIv', + vault: JSON.stringify({ + ...result, + salt: _salt, + }), + exportedKeyString: JSON.stringify(key), }; } - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - async encryptWithDetail(key: string, dataObj: any) { + async decryptWithDetail( + password: string, + text: string, + ): Promise { + const { salt } = JSON.parse(text); + const key = deriveKey(password, salt); return { - vault: await this.encrypt(key, dataObj), - exportedKeyString: MOCK_ENCRYPTION_KEY, + vault: await this.decryptWithKey(key, text), + salt, + exportedKeyString: JSON.stringify(key), }; } - async decryptWithDetail(key: string, text: string) { + async encryptWithKey(key: unknown, dataObj: Json): Promise { + const iv = generateIV(); return { - vault: await this.decrypt(key, text), - salt: MOCK_ENCRYPTION_SALT, - exportedKeyString: MOCK_ENCRYPTION_KEY, + data: JSON.stringify({ + tag: { key, iv }, + value: dataObj, + }), + iv, }; } - async decryptWithKey(key: unknown, text: string) { - return this.decrypt(key as string, text); - } - - async keyFromPassword(_password: string) { - return MOCK_KEY; + async decryptWithKey(key: unknown, ciphertext: string): Promise { + // This conditional assignment is required because sometimes the keyring + // controller passes in the parsed object instead of the string. + const ciphertextObj = + typeof ciphertext === 'string' ? JSON.parse(ciphertext) : ciphertext; + const data = JSON.parse(ciphertextObj.data); + if (!isEqual(data.tag, { key, iv: ciphertextObj.iv })) { + throw new Error(DECRYPTION_ERROR); + } + return data.value; } async importKey(key: string) { - if (key === '{}') { - throw new TypeError( - `Failed to execute 'importKey' on 'SubtleCrypto': The provided value is not of type '(ArrayBuffer or ArrayBufferView or JsonWebKey)'.`, - ); - } - return null; + return JSON.parse(key); } async updateVault(_vault: string, _password: string) { @@ -88,8 +108,18 @@ export default class MockEncryptor implements ExportableKeyEncryptor { isVaultUpdated(_vault: string) { return true; } +} - generateSalt() { - return MOCK_ENCRYPTION_SALT; - } +function generateSalt() { + // Generate random salt. + + // return crypto.randomUUID(); + return SALT; // TODO some tests rely on fixed salt, but wouldn't it be better to generate random value here? +} + +function generateIV() { + // Generate random salt. + + // return crypto.randomUUID(); + return 'iv'; // TODO some tests rely on fixed iv, but wouldn't it be better to generate random value here? }