From c25efe0f09dbcfabf46383e028f63e02ca9da83f Mon Sep 17 00:00:00 2001 From: David Walsh Date: Fri, 30 Sep 2022 13:52:01 -0500 Subject: [PATCH 01/55] Export key for encrypted key login --- index.js | 47 +++++++++++++++++++++++++++----------- test/index.js | 3 ++- test/lib/mock-encryptor.js | 4 ++++ 3 files changed, 40 insertions(+), 14 deletions(-) diff --git a/index.js b/index.js index 5f92493b..b3625825 100644 --- a/index.js +++ b/index.js @@ -14,6 +14,7 @@ const KEYRINGS_TYPE_MAP = { HD_KEYRING: 'HD Key Tree', SIMPLE_KEYRING: 'Simple Key Pair', }; + /** * Strip the hex prefix from an address, if present * @param {string} address - The address that might be hex prefixed. @@ -145,7 +146,7 @@ class KeyringController extends EventEmitter { */ async setLocked() { // set locked - this.password = null; + delete this.encryptionKey; this.memStore.updateState({ isUnlocked: false }); // remove keyrings this.keyrings = []; @@ -168,7 +169,15 @@ class KeyringController extends EventEmitter { * @returns {Promise} A Promise that resolves to the state. */ async submitPassword(password) { + await this.verifyPassword(password); this.keyrings = await this.unlockKeyrings(password); + await this.persistAllKeyrings(password); + this.setUnlocked(); + this.fullUpdate(); + } + + async submitEncryptionKey(encryptionKey) { + this.keyrings = await this.unlockKeyrings(undefined, encryptionKey); this.setUnlocked(); this.fullUpdate(); } @@ -202,7 +211,7 @@ class KeyringController extends EventEmitter { * @param {Object} opts - The constructor options for the keyring. * @returns {Promise} The new keyring. */ - async addNewKeyring(type, opts) { + async addNewKeyring(type, opts, password) { const Keyring = this.getKeyringClassForType(type); const keyring = new Keyring(opts); if ((!opts || !opts.mnemonic) && type === KEYRINGS_TYPE_MAP.HD_KEYRING) { @@ -214,7 +223,7 @@ class KeyringController extends EventEmitter { await this.checkForDuplicate(type, accounts); this.keyrings.push(keyring); - await this.persistAllKeyrings(); + await this.persistAllKeyrings(password); await this._updateMemStoreKeyrings(); this.fullUpdate(); @@ -491,10 +500,13 @@ class KeyringController extends EventEmitter { * @returns {Promise} - A promise that resolves if the operation was successful. */ async createFirstKeyTree(password) { - this.password = password; this.clearKeyrings(); - const keyring = await this.addNewKeyring(KEYRINGS_TYPE_MAP.HD_KEYRING); + const keyring = await this.addNewKeyring( + KEYRINGS_TYPE_MAP.HD_KEYRING, + {}, + password, + ); const [firstAccount] = await keyring.getAccounts(); if (!firstAccount) { throw new Error('KeyringController - No account found on keychain.'); @@ -516,12 +528,11 @@ class KeyringController extends EventEmitter { * @param {string} password - The keyring controller password. * @returns {Promise} Resolves to true once keyrings are persisted. */ - async persistAllKeyrings(password = this.password) { + async persistAllKeyrings(password) { if (typeof password !== 'string') { throw new Error('KeyringController - password is not a string'); } - this.password = password; const serializedKeyrings = await Promise.all( this.keyrings.map(async (keyring) => { const [type, data] = await Promise.all([ @@ -531,11 +542,13 @@ class KeyringController extends EventEmitter { return { type, data }; }), ); - const encryptedString = await this.encryptor.encrypt( - this.password, + + const vault = await this.encryptor.encrypt( + password, serializedKeyrings, ); - this.store.updateState({ vault: encryptedString }); + + this.store.updateState({ vault }); return true; } @@ -548,15 +561,23 @@ class KeyringController extends EventEmitter { * @param {string} password - The keyring controller password. * @returns {Promise>} The keyrings. */ - async unlockKeyrings(password) { + async unlockKeyrings(password, encryptionKey) { const encryptedVault = this.store.getState().vault; if (!encryptedVault) { throw new Error('Cannot unlock without a previous vault.'); } await this.clearKeyrings(); - const vault = await this.encryptor.decrypt(password, encryptedVault); - this.password = password; + + let vault; + if (password) { + const result = await this.encryptor.decrypt(password, encryptedVault); + vault = result.vault; + this.encryptionKey = result.extractedKeyString; + } else { + vault = await this.encryptor.decryptWithEncryptedKeyString(encryptionKey); + } + await Promise.all(vault.map(this._restoreKeyring.bind(this))); await this._updateMemStoreKeyrings(); return this.keyrings; diff --git a/test/index.js b/test/index.js index 35d8000e..5e60a457 100644 --- a/test/index.js +++ b/test/index.js @@ -29,6 +29,7 @@ describe('KeyringController', function () { }); await keyringController.createNewVaultAndKeychain(password); + await keyringController.submitPassword(password); }); afterEach(function () { @@ -63,7 +64,7 @@ describe('KeyringController', function () { describe('submitPassword', function () { it('should not create new keyrings when called in series', async function () { await keyringController.createNewVaultAndKeychain(password); - await keyringController.persistAllKeyrings(); + await keyringController.persistAllKeyrings(password); expect(keyringController.keyrings).toHaveLength(1); await keyringController.submitPassword(`${password}a`); diff --git a/test/lib/mock-encryptor.js b/test/lib/mock-encryptor.js index 6089a370..778945cd 100644 --- a/test/lib/mock-encryptor.js +++ b/test/lib/mock-encryptor.js @@ -10,6 +10,10 @@ module.exports = { return Promise.resolve(mockHex); }), + decryptWithEncryptedKeyString(_keyStr) { + return this.decrypt(); + }, + decrypt(_password, _text) { return Promise.resolve(cacheVal || {}); }, From e2397747e5b1d573253061d88fc6bbfa4424cf03 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 3 Oct 2022 09:49:17 -0500 Subject: [PATCH 02/55] Progress --- index.js | 22 +++++++++++++++++----- package.json | 2 +- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/index.js b/index.js index b3625825..48312737 100644 --- a/index.js +++ b/index.js @@ -126,6 +126,7 @@ class KeyringController extends EventEmitter { mnemonic: seedPhraseAsBuffer, numberOfAccounts: 1, }, + password ); const [firstAccount] = await firstKeyring.getAccounts(); if (!firstAccount) { @@ -172,14 +173,22 @@ class KeyringController extends EventEmitter { await this.verifyPassword(password); this.keyrings = await this.unlockKeyrings(password); await this.persistAllKeyrings(password); + + // Keeping this.password is required because it's called by `getKeyringForDevice` + // in the metamask extensions + this.password = password + this.setUnlocked(); this.fullUpdate(); } - async submitEncryptionKey(encryptionKey) { - this.keyrings = await this.unlockKeyrings(undefined, encryptionKey); + async submitEncryptionKey(encryptionKey, loginData) { + this.keyrings = await this.unlockKeyrings(undefined, encryptionKey, loginData); this.setUnlocked(); this.fullUpdate(); + + console.log("Encryption key login successful!") + return true; } /** @@ -211,7 +220,7 @@ class KeyringController extends EventEmitter { * @param {Object} opts - The constructor options for the keyring. * @returns {Promise} The new keyring. */ - async addNewKeyring(type, opts, password) { + async addNewKeyring(type, opts, password = this.password) { const Keyring = this.getKeyringClassForType(type); const keyring = new Keyring(opts); if ((!opts || !opts.mnemonic) && type === KEYRINGS_TYPE_MAP.HD_KEYRING) { @@ -548,7 +557,9 @@ class KeyringController extends EventEmitter { serializedKeyrings, ); + console.log("persistAllKeyrings: ", password, vault); this.store.updateState({ vault }); + return true; } @@ -561,7 +572,7 @@ class KeyringController extends EventEmitter { * @param {string} password - The keyring controller password. * @returns {Promise>} The keyrings. */ - async unlockKeyrings(password, encryptionKey) { + async unlockKeyrings(password, encryptionKey, loginData) { const encryptedVault = this.store.getState().vault; if (!encryptedVault) { throw new Error('Cannot unlock without a previous vault.'); @@ -574,8 +585,9 @@ class KeyringController extends EventEmitter { const result = await this.encryptor.decrypt(password, encryptedVault); vault = result.vault; this.encryptionKey = result.extractedKeyString; + this.encryptionData = result.data; } else { - vault = await this.encryptor.decryptWithEncryptedKeyString(encryptionKey); + vault = await this.encryptor.decryptWithEncryptedKeyString(encryptionKey, loginData); } await Promise.all(vault.map(this._restoreKeyring.bind(this))); diff --git a/package.json b/package.json index a1953c3e..f0df0cba 100644 --- a/package.json +++ b/package.json @@ -33,7 +33,7 @@ "dependencies": { "@metamask/bip39": "^4.0.0", "@metamask/eth-hd-keyring": "^4.0.2", - "browser-passworder": "^2.0.3", + "browser-passworder": "MetaMask/browser-passworder#export-key", "eth-sig-util": "^3.0.1", "eth-simple-keyring": "^4.2.0", "obs-store": "^4.0.3" From 3c4e88de8285e483bc91130be7cf0d095b5b50b3 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Tue, 4 Oct 2022 10:03:02 -0500 Subject: [PATCH 03/55] Delete encryption data upon lock --- index.js | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/index.js b/index.js index 48312737..e72515fa 100644 --- a/index.js +++ b/index.js @@ -126,7 +126,7 @@ class KeyringController extends EventEmitter { mnemonic: seedPhraseAsBuffer, numberOfAccounts: 1, }, - password + password, ); const [firstAccount] = await firstKeyring.getAccounts(); if (!firstAccount) { @@ -148,6 +148,7 @@ class KeyringController extends EventEmitter { async setLocked() { // set locked delete this.encryptionKey; + delete this.encryptionData; this.memStore.updateState({ isUnlocked: false }); // remove keyrings this.keyrings = []; @@ -176,18 +177,22 @@ class KeyringController extends EventEmitter { // Keeping this.password is required because it's called by `getKeyringForDevice` // in the metamask extensions - this.password = password + this.password = password; this.setUnlocked(); this.fullUpdate(); } async submitEncryptionKey(encryptionKey, loginData) { - this.keyrings = await this.unlockKeyrings(undefined, encryptionKey, loginData); + this.keyrings = await this.unlockKeyrings( + undefined, + encryptionKey, + loginData, + ); this.setUnlocked(); this.fullUpdate(); - console.log("Encryption key login successful!") + console.log('Encryption key login successful!'); return true; } @@ -552,12 +557,9 @@ class KeyringController extends EventEmitter { }), ); - const vault = await this.encryptor.encrypt( - password, - serializedKeyrings, - ); + const vault = await this.encryptor.encrypt(password, serializedKeyrings); - console.log("persistAllKeyrings: ", password, vault); + console.log('persistAllKeyrings: ', password, vault); this.store.updateState({ vault }); return true; @@ -587,7 +589,10 @@ class KeyringController extends EventEmitter { this.encryptionKey = result.extractedKeyString; this.encryptionData = result.data; } else { - vault = await this.encryptor.decryptWithEncryptedKeyString(encryptionKey, loginData); + vault = await this.encryptor.decryptWithEncryptedKeyString( + encryptionKey, + loginData, + ); } await Promise.all(vault.map(this._restoreKeyring.bind(this))); From eb8b3570530ba66625e0675edea2f1a23a8bb7e1 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Wed, 5 Oct 2022 14:47:17 -0500 Subject: [PATCH 04/55] Progress: Ensure addAccount and removeAccount work by allowing encryption via key --- index.js | 55 ++++++++++++++++++++++++++++++++++++--------------- test/index.js | 15 ++++++++++++++ 2 files changed, 54 insertions(+), 16 deletions(-) diff --git a/index.js b/index.js index e72515fa..e322e80d 100644 --- a/index.js +++ b/index.js @@ -79,8 +79,10 @@ class KeyringController extends EventEmitter { * @returns {Promise} A Promise that resolves to the state. */ async createNewVaultAndKeychain(password) { + this.password = password; + await this.createFirstKeyTree(password); - await this.persistAllKeyrings(password); + await this.persistAllKeyrings(); this.setUnlocked.bind(); this.fullUpdate(); } @@ -117,9 +119,11 @@ class KeyringController extends EventEmitter { throw new Error('Seed phrase is invalid.'); } + this.password = password; + this.clearKeyrings(); - await this.persistAllKeyrings(password); + await this.persistAllKeyrings(); const firstKeyring = await this.addNewKeyring( KEYRINGS_TYPE_MAP.HD_KEYRING, { @@ -133,7 +137,7 @@ class KeyringController extends EventEmitter { throw new Error('KeyringController - First Account not found.'); } - await this.persistAllKeyrings(password); + await this.persistAllKeyrings(); this.setUnlocked(); return this.fullUpdate(); } @@ -148,7 +152,6 @@ class KeyringController extends EventEmitter { async setLocked() { // set locked delete this.encryptionKey; - delete this.encryptionData; this.memStore.updateState({ isUnlocked: false }); // remove keyrings this.keyrings = []; @@ -173,10 +176,8 @@ class KeyringController extends EventEmitter { async submitPassword(password) { await this.verifyPassword(password); this.keyrings = await this.unlockKeyrings(password); - await this.persistAllKeyrings(password); + await this.persistAllKeyrings(); - // Keeping this.password is required because it's called by `getKeyringForDevice` - // in the metamask extensions this.password = password; this.setUnlocked(); @@ -225,7 +226,7 @@ class KeyringController extends EventEmitter { * @param {Object} opts - The constructor options for the keyring. * @returns {Promise} The new keyring. */ - async addNewKeyring(type, opts, password = this.password) { + async addNewKeyring(type, opts) { const Keyring = this.getKeyringClassForType(type); const keyring = new Keyring(opts); if ((!opts || !opts.mnemonic) && type === KEYRINGS_TYPE_MAP.HD_KEYRING) { @@ -237,7 +238,7 @@ class KeyringController extends EventEmitter { await this.checkForDuplicate(type, accounts); this.keyrings.push(keyring); - await this.persistAllKeyrings(password); + await this.persistAllKeyrings(); await this._updateMemStoreKeyrings(); this.fullUpdate(); @@ -542,9 +543,11 @@ class KeyringController extends EventEmitter { * @param {string} password - The keyring controller password. * @returns {Promise} Resolves to true once keyrings are persisted. */ - async persistAllKeyrings(password) { - if (typeof password !== 'string') { - throw new Error('KeyringController - password is not a string'); + async persistAllKeyrings() { + if (!this.password && !this.encryptionKey) { + throw new Error( + 'Cannot persist vault without password and encryption key', + ); } const serializedKeyrings = await Promise.all( @@ -557,9 +560,25 @@ class KeyringController extends EventEmitter { }), ); - const vault = await this.encryptor.encrypt(password, serializedKeyrings); + let vault; + if (this.password) { + vault = await this.encryptor.encrypt(this.password, serializedKeyrings); + + console.log('vault created with password is: ', vault); + } else if (this.encryptionKey) { + const key = await this.encryptor.createKeyFromString(this.encryptionKey); + vault = JSON.stringify( + await this.encryptor.encryptWithKey(key, serializedKeyrings), + ); + + console.log('vault created with encryptWithKey is: ', vault); + } + + if (!vault) { + throw new Error('Cannot persist vault without vault information'); + } - console.log('persistAllKeyrings: ', password, vault); + console.log('persistAllKeyrings: ', vault); this.store.updateState({ vault }); return true; @@ -574,7 +593,7 @@ class KeyringController extends EventEmitter { * @param {string} password - The keyring controller password. * @returns {Promise>} The keyrings. */ - async unlockKeyrings(password, encryptionKey, loginData) { + async unlockKeyrings(password, encryptionKey, encryptionData) { const encryptedVault = this.store.getState().vault; if (!encryptedVault) { throw new Error('Cannot unlock without a previous vault.'); @@ -586,13 +605,17 @@ class KeyringController extends EventEmitter { if (password) { const result = await this.encryptor.decrypt(password, encryptedVault); vault = result.vault; + this.encryptionKey = result.extractedKeyString; this.encryptionData = result.data; } else { vault = await this.encryptor.decryptWithEncryptedKeyString( encryptionKey, - loginData, + encryptionData, ); + + this.encryptionKey = encryptionKey; + this.encryptionData = encryptionData; } await Promise.all(vault.map(this._restoreKeyring.bind(this))); diff --git a/test/index.js b/test/index.js index 5e60a457..7f73a4b2 100644 --- a/test/index.js +++ b/test/index.js @@ -452,4 +452,19 @@ describe('KeyringController', function () { ); }); }); + + describe('submitEncryptionKey', function () { + it('unlocks the keyrings with valid information', async function () { + const MOCK_ENCRYPTION_KEY = + '{"alg":"A256GCM","ext":true,"k":"wYmxkxOOFBDP6F6VuuYFcRt_Po-tSLFHCWVolsHs4VI","key_ops":["encrypt","decrypt"],"kty":"oct"}'; + const MOCK_ENCRYPTION_DATA = + '{"data":"2fOOPRKClNrisB+tmqIcETyZvDuL2iIR1Hr1nO7XZHyMqVY1cDBetw2gY5C+cIo1qkpyv3bPp+4buUjp38VBsjbijM0F/FLOqWbcuKM9h9X0uwxsgsZ96uwcIf5I46NiMgoFlhppTTMZT0Nkocz+SnvHM0IgLsFan7JqBU++vSJvx2M1PDljZSunOsqyyL+DKmbYmM4umbouKV42dipUwrCvrQJmpiUZrSkpMJrPJk9ufDQO4CyIVo0qry3aNRdYFJ6rgSyq/k6rXMwGExCMHn8UlhNnAMuMKWPWR/ymK1bzNcNs4VU14iVjEXOZGPvD9cvqVe/VtcnIba6axNEEB4HWDOCdrDh5YNWwMlQVL7vSB2yOhPZByGhnEOloYsj2E5KEb9jFGskt7EKDEYNofr6t83G0c+B72VGYZeCvgtzXzgPwzIbhTtKkP+gdBmt2JNSYrTjLypT0q+v4C9BN1xWTxPmX6TTt0NzkI9pJxgN1VQAfSU9CyWTVpd4CBkgom2cSBsxZ2MNbdKF+qSWz3fQcmJ55hxM0EGJSt9+8eQOTuoJlBapRk4wdZKHR2jdKzPjSF2MAmyVD2kU51IKa/cVsckRFEes+m7dKyHRvlNwgT78W9tBDdZb5PSlfbZXnv8z5q1KtAj2lM2ogJ7brHBdevl4FISdTkObpwcUMcvACOOO0dj6CSYjSKr0ZJ2RLChVruZyPDxEhKGb/8Kv8trLOR3mck/et6d050/NugezycNk4nnzu5iP90gPbSzaqdZI=","iv":"qTGO1afGv3waHN9KoW34Eg==","salt":"HQ5sfhsb8XAQRJtD+UqcImT7Ve4n3YMagrh05YTOsjk="}'; + + await keyringController.submitEncryptionKey( + MOCK_ENCRYPTION_KEY, + MOCK_ENCRYPTION_DATA, + ); + expect(keyringController.keyrings).toHaveLength(1); + }); + }); }); From aed7c3bb78f8eaf8357cd5487b810beda17fcf2d Mon Sep 17 00:00:00 2001 From: David Walsh Date: Thu, 6 Oct 2022 11:15:54 -0500 Subject: [PATCH 05/55] Ensure the user is properly authenticated with encryption key. Also, lock the keyring when the user submits another password --- index.js | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/index.js b/index.js index e322e80d..748feb70 100644 --- a/index.js +++ b/index.js @@ -150,9 +150,14 @@ class KeyringController extends EventEmitter { * @returns {Promise} A Promise that resolves to the state. */ async setLocked() { - // set locked + delete this.encryptionKey; + delete this.encryptionData; + delete this.password; + + // set locked this.memStore.updateState({ isUnlocked: false }); + // remove keyrings this.keyrings = []; await this._updateMemStoreKeyrings(); @@ -174,12 +179,15 @@ class KeyringController extends EventEmitter { * @returns {Promise} A Promise that resolves to the state. */ async submitPassword(password) { + // If a user is trying to submit a password, we should lock + // in the event that the password is incorrect + this.setLocked() + await this.verifyPassword(password); this.keyrings = await this.unlockKeyrings(password); + await this.persistAllKeyrings(); - this.password = password; - this.setUnlocked(); this.fullUpdate(); } @@ -193,7 +201,6 @@ class KeyringController extends EventEmitter { this.setUnlocked(); this.fullUpdate(); - console.log('Encryption key login successful!'); return true; } @@ -564,21 +571,21 @@ class KeyringController extends EventEmitter { if (this.password) { vault = await this.encryptor.encrypt(this.password, serializedKeyrings); - console.log('vault created with password is: ', vault); } else if (this.encryptionKey) { const key = await this.encryptor.createKeyFromString(this.encryptionKey); - vault = JSON.stringify( - await this.encryptor.encryptWithKey(key, serializedKeyrings), + const vaultJSON = await this.encryptor.encryptWithKey( + key, + serializedKeyrings, ); + vaultJSON.salt = JSON.parse(this.encryptionData).salt; + vault = JSON.stringify(vaultJSON); - console.log('vault created with encryptWithKey is: ', vault); } if (!vault) { throw new Error('Cannot persist vault without vault information'); } - console.log('persistAllKeyrings: ', vault); this.store.updateState({ vault }); return true; @@ -606,6 +613,7 @@ class KeyringController extends EventEmitter { const result = await this.encryptor.decrypt(password, encryptedVault); vault = result.vault; + this.password = password; this.encryptionKey = result.extractedKeyString; this.encryptionData = result.data; } else { @@ -614,10 +622,16 @@ class KeyringController extends EventEmitter { encryptionData, ); + if(!vault) { + console.log('[unlockKeyrings] no vault via encrypted key string: ', encryptionKey); + } + this.encryptionKey = encryptionKey; this.encryptionData = encryptionData; } + // console.log('vault is: ', vault); + await Promise.all(vault.map(this._restoreKeyring.bind(this))); await this._updateMemStoreKeyrings(); return this.keyrings; From 97ebd514e6f64214c8be9cbbdc894a795af429af Mon Sep 17 00:00:00 2001 From: David Walsh Date: Thu, 6 Oct 2022 11:16:37 -0500 Subject: [PATCH 06/55] Progress: Tests --- test/index.js | 37 ++++++++++++++++++++----------------- test/lib/mock-encryptor.js | 2 +- 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/test/index.js b/test/index.js index 7f73a4b2..5979f923 100644 --- a/test/index.js +++ b/test/index.js @@ -8,7 +8,8 @@ const Wallet = require('ethereumjs-wallet').default; const KeyringController = require('..'); const mockEncryptor = require('./lib/mock-encryptor'); -const password = 'password123'; +const PASSWORD = 'password123'; + const walletOneSeedWords = 'puzzle seed penalty soldier say clay field arctic metal hen cage runway'; const walletOneAddresses = ['0xef35ca8ebb9669a35c31b5f6f249a9941a812ac1']; @@ -28,8 +29,8 @@ describe('KeyringController', function () { encryptor: mockEncryptor, }); - await keyringController.createNewVaultAndKeychain(password); - await keyringController.submitPassword(password); + await keyringController.createNewVaultAndKeychain(PASSWORD); + await keyringController.submitPassword(PASSWORD); }); afterEach(function () { @@ -46,7 +47,7 @@ describe('KeyringController', function () { await keyringController.setLocked(); - expect(keyringController.password).toBeNull(); + expect(keyringController.password).toBeUndefined(); expect(keyringController.memStore.getState().isUnlocked).toBe(false); expect(keyringController.keyrings).toHaveLength(0); }); @@ -63,15 +64,15 @@ describe('KeyringController', function () { describe('submitPassword', function () { it('should not create new keyrings when called in series', async function () { - await keyringController.createNewVaultAndKeychain(password); - await keyringController.persistAllKeyrings(password); + await keyringController.createNewVaultAndKeychain(PASSWORD); + await keyringController.persistAllKeyrings(); expect(keyringController.keyrings).toHaveLength(1); - await keyringController.submitPassword(`${password}a`); - expect(keyringController.keyrings).toHaveLength(1); + await keyringController.submitPassword(`${PASSWORD}a`); + expect(keyringController.keyrings).toHaveLength(0); await keyringController.submitPassword(''); - expect(keyringController.keyrings).toHaveLength(1); + expect(keyringController.keyrings).toHaveLength(0); }); it('emits "unlock" event', async function () { @@ -80,7 +81,7 @@ describe('KeyringController', function () { const spy = sinon.spy(); keyringController.on('unlock', spy); - await keyringController.submitPassword(password); + await keyringController.submitPassword(PASSWORD); expect(spy.calledOnce).toBe(true); }); }); @@ -90,7 +91,7 @@ describe('KeyringController', function () { keyringController.store.updateState({ vault: null }); assert(!keyringController.store.getState().vault, 'no previous vault'); - await keyringController.createNewVaultAndKeychain(password); + await keyringController.createNewVaultAndKeychain(PASSWORD); const { vault } = keyringController.store.getState(); // eslint-disable-next-line jest/no-restricted-matchers expect(vault).toBeTruthy(); @@ -100,12 +101,12 @@ describe('KeyringController', function () { keyringController.store.updateState({ vault: null }); assert(!keyringController.store.getState().vault, 'no previous vault'); - await keyringController.createNewVaultAndKeychain(password); + await keyringController.createNewVaultAndKeychain(PASSWORD); const { vault } = keyringController.store.getState(); // eslint-disable-next-line jest/no-restricted-matchers expect(vault).toBeTruthy(); keyringController.encryptor.encrypt.args.forEach(([actualPassword]) => { - expect(actualPassword).toBe(password); + expect(actualPassword).toBe(PASSWORD); }); }); }); @@ -120,7 +121,7 @@ describe('KeyringController', function () { expect(allAccounts).toHaveLength(2); await keyringController.createNewVaultAndRestore( - password, + PASSWORD, walletOneSeedWords, ); @@ -138,7 +139,7 @@ describe('KeyringController', function () { it('throws error if mnemonic passed is invalid', async function () { await expect(() => keyringController.createNewVaultAndRestore( - password, + PASSWORD, 'test test test palace city barely security section midnight wealth south deer', ), ).rejects.toThrow('Seed phrase is invalid.'); @@ -152,7 +153,7 @@ describe('KeyringController', function () { ); await keyringController.createNewVaultAndRestore( - password, + PASSWORD, mnemonicAsArrayOfNumbers, ); @@ -304,7 +305,7 @@ describe('KeyringController', function () { describe('unlockKeyrings', function () { it('returns the list of keyrings', async function () { await keyringController.setLocked(); - const keyrings = await keyringController.unlockKeyrings(password); + const keyrings = await keyringController.unlockKeyrings(PASSWORD); expect(keyrings).toHaveLength(1); keyrings.forEach((keyring) => { expect(keyring.wallets).toHaveLength(1); @@ -453,6 +454,7 @@ describe('KeyringController', function () { }); }); + /* describe('submitEncryptionKey', function () { it('unlocks the keyrings with valid information', async function () { const MOCK_ENCRYPTION_KEY = @@ -467,4 +469,5 @@ describe('KeyringController', function () { expect(keyringController.keyrings).toHaveLength(1); }); }); + */ }); diff --git a/test/lib/mock-encryptor.js b/test/lib/mock-encryptor.js index 778945cd..dedda73e 100644 --- a/test/lib/mock-encryptor.js +++ b/test/lib/mock-encryptor.js @@ -15,7 +15,7 @@ module.exports = { }, decrypt(_password, _text) { - return Promise.resolve(cacheVal || {}); + return Promise.resolve(cacheVal ? { vault: cacheVal } : {}); }, encryptWithKey(key, dataObj) { From 4d62780dd2c50990d433cd001ce6750d405c32a4 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Thu, 6 Oct 2022 16:12:06 -0500 Subject: [PATCH 07/55] Progress on tests --- index.js | 16 ++++------------ test/index.js | 38 +++++++++++++++++++++++++------------- test/lib/mock-encryptor.js | 22 +++++++++++++++++++--- 3 files changed, 48 insertions(+), 28 deletions(-) diff --git a/index.js b/index.js index 748feb70..f0d78f79 100644 --- a/index.js +++ b/index.js @@ -150,14 +150,13 @@ class KeyringController extends EventEmitter { * @returns {Promise} A Promise that resolves to the state. */ async setLocked() { - delete this.encryptionKey; delete this.encryptionData; delete this.password; // set locked this.memStore.updateState({ isUnlocked: false }); - + // remove keyrings this.keyrings = []; await this._updateMemStoreKeyrings(); @@ -181,11 +180,11 @@ class KeyringController extends EventEmitter { async submitPassword(password) { // If a user is trying to submit a password, we should lock // in the event that the password is incorrect - this.setLocked() + this.setLocked(); await this.verifyPassword(password); this.keyrings = await this.unlockKeyrings(password); - + await this.persistAllKeyrings(); this.setUnlocked(); @@ -570,7 +569,6 @@ class KeyringController extends EventEmitter { let vault; if (this.password) { vault = await this.encryptor.encrypt(this.password, serializedKeyrings); - } else if (this.encryptionKey) { const key = await this.encryptor.createKeyFromString(this.encryptionKey); const vaultJSON = await this.encryptor.encryptWithKey( @@ -579,7 +577,6 @@ class KeyringController extends EventEmitter { ); vaultJSON.salt = JSON.parse(this.encryptionData).salt; vault = JSON.stringify(vaultJSON); - } if (!vault) { @@ -611,6 +608,7 @@ class KeyringController extends EventEmitter { let vault; if (password) { const result = await this.encryptor.decrypt(password, encryptedVault); + vault = result.vault; this.password = password; @@ -622,16 +620,10 @@ class KeyringController extends EventEmitter { encryptionData, ); - if(!vault) { - console.log('[unlockKeyrings] no vault via encrypted key string: ', encryptionKey); - } - this.encryptionKey = encryptionKey; this.encryptionData = encryptionData; } - // console.log('vault is: ', vault); - await Promise.all(vault.map(this._restoreKeyring.bind(this))); await this._updateMemStoreKeyrings(); return this.keyrings; diff --git a/test/index.js b/test/index.js index 5979f923..87e87513 100644 --- a/test/index.js +++ b/test/index.js @@ -10,6 +10,12 @@ const mockEncryptor = require('./lib/mock-encryptor'); const PASSWORD = 'password123'; +const MOCK_ENCRYPTION_KEY = + '{"alg":"A256GCM","ext":true,"k":"wYmxkxOOFBDP6F6VuuYFcRt_Po-tSLFHCWVolsHs4VI","key_ops":["encrypt","decrypt"],"kty":"oct"}'; + const MOCK_ENCRYPTION_DATA = + '{"data":"2fOOPRKClNrisB+tmqIcETyZvDuL2iIR1Hr1nO7XZHyMqVY1cDBetw2gY5C+cIo1qkpyv3bPp+4buUjp38VBsjbijM0F/FLOqWbcuKM9h9X0uwxsgsZ96uwcIf5I46NiMgoFlhppTTMZT0Nkocz+SnvHM0IgLsFan7JqBU++vSJvx2M1PDljZSunOsqyyL+DKmbYmM4umbouKV42dipUwrCvrQJmpiUZrSkpMJrPJk9ufDQO4CyIVo0qry3aNRdYFJ6rgSyq/k6rXMwGExCMHn8UlhNnAMuMKWPWR/ymK1bzNcNs4VU14iVjEXOZGPvD9cvqVe/VtcnIba6axNEEB4HWDOCdrDh5YNWwMlQVL7vSB2yOhPZByGhnEOloYsj2E5KEb9jFGskt7EKDEYNofr6t83G0c+B72VGYZeCvgtzXzgPwzIbhTtKkP+gdBmt2JNSYrTjLypT0q+v4C9BN1xWTxPmX6TTt0NzkI9pJxgN1VQAfSU9CyWTVpd4CBkgom2cSBsxZ2MNbdKF+qSWz3fQcmJ55hxM0EGJSt9+8eQOTuoJlBapRk4wdZKHR2jdKzPjSF2MAmyVD2kU51IKa/cVsckRFEes+m7dKyHRvlNwgT78W9tBDdZb5PSlfbZXnv8z5q1KtAj2lM2ogJ7brHBdevl4FISdTkObpwcUMcvACOOO0dj6CSYjSKr0ZJ2RLChVruZyPDxEhKGb/8Kv8trLOR3mck/et6d050/NugezycNk4nnzu5iP90gPbSzaqdZI=","iv":"qTGO1afGv3waHN9KoW34Eg==","salt":"HQ5sfhsb8XAQRJtD+UqcImT7Ve4n3YMagrh05YTOsjk="}'; + + const walletOneSeedWords = 'puzzle seed penalty soldier say clay field arctic metal hen cage runway'; const walletOneAddresses = ['0xef35ca8ebb9669a35c31b5f6f249a9941a812ac1']; @@ -63,15 +69,15 @@ describe('KeyringController', function () { }); describe('submitPassword', function () { - it('should not create new keyrings when called in series', async function () { + it('should not loads keyrings when incorrect password', async function () { await keyringController.createNewVaultAndKeychain(PASSWORD); await keyringController.persistAllKeyrings(); expect(keyringController.keyrings).toHaveLength(1); - await keyringController.submitPassword(`${PASSWORD}a`); - expect(keyringController.keyrings).toHaveLength(0); - - await keyringController.submitPassword(''); + await expect( + keyringController.submitPassword(`${PASSWORD}a`), + ).rejects.toThrow('Incorrect password.'); + expect(keyringController.password).toBeUndefined(); expect(keyringController.keyrings).toHaveLength(0); }); @@ -454,20 +460,26 @@ describe('KeyringController', function () { }); }); - /* + describe('submitEncryptionKey', function () { - it('unlocks the keyrings with valid information', async function () { - const MOCK_ENCRYPTION_KEY = - '{"alg":"A256GCM","ext":true,"k":"wYmxkxOOFBDP6F6VuuYFcRt_Po-tSLFHCWVolsHs4VI","key_ops":["encrypt","decrypt"],"kty":"oct"}'; - const MOCK_ENCRYPTION_DATA = - '{"data":"2fOOPRKClNrisB+tmqIcETyZvDuL2iIR1Hr1nO7XZHyMqVY1cDBetw2gY5C+cIo1qkpyv3bPp+4buUjp38VBsjbijM0F/FLOqWbcuKM9h9X0uwxsgsZ96uwcIf5I46NiMgoFlhppTTMZT0Nkocz+SnvHM0IgLsFan7JqBU++vSJvx2M1PDljZSunOsqyyL+DKmbYmM4umbouKV42dipUwrCvrQJmpiUZrSkpMJrPJk9ufDQO4CyIVo0qry3aNRdYFJ6rgSyq/k6rXMwGExCMHn8UlhNnAMuMKWPWR/ymK1bzNcNs4VU14iVjEXOZGPvD9cvqVe/VtcnIba6axNEEB4HWDOCdrDh5YNWwMlQVL7vSB2yOhPZByGhnEOloYsj2E5KEb9jFGskt7EKDEYNofr6t83G0c+B72VGYZeCvgtzXzgPwzIbhTtKkP+gdBmt2JNSYrTjLypT0q+v4C9BN1xWTxPmX6TTt0NzkI9pJxgN1VQAfSU9CyWTVpd4CBkgom2cSBsxZ2MNbdKF+qSWz3fQcmJ55hxM0EGJSt9+8eQOTuoJlBapRk4wdZKHR2jdKzPjSF2MAmyVD2kU51IKa/cVsckRFEes+m7dKyHRvlNwgT78W9tBDdZb5PSlfbZXnv8z5q1KtAj2lM2ogJ7brHBdevl4FISdTkObpwcUMcvACOOO0dj6CSYjSKr0ZJ2RLChVruZyPDxEhKGb/8Kv8trLOR3mck/et6d050/NugezycNk4nnzu5iP90gPbSzaqdZI=","iv":"qTGO1afGv3waHN9KoW34Eg==","salt":"HQ5sfhsb8XAQRJtD+UqcImT7Ve4n3YMagrh05YTOsjk="}'; + it('sets encryption key data upon submitPassword', async function() { + await keyringController.submitPassword(PASSWORD); + + expect(keyringController.encryptionKey).toBe(MOCK_ENCRYPTION_KEY); + expect(keyringController.encryptionData).toBe(MOCK_ENCRYPTION_DATA); + }) + xit('unlocks the keyrings with valid information', async function () { + const stub = sinon.stub(keyringController.encryptor, 'decryptWithEncryptedKeyString') + stub.returns(await keyringController.encryptor.decrypt()); + await keyringController.submitEncryptionKey( MOCK_ENCRYPTION_KEY, MOCK_ENCRYPTION_DATA, ); - expect(keyringController.keyrings).toHaveLength(1); + + expect(keyringController.encryptor.decryptWithEncryptedKeyString.calledOnce).toBe(true); }); }); - */ + }); diff --git a/test/lib/mock-encryptor.js b/test/lib/mock-encryptor.js index dedda73e..e67cdfd2 100644 --- a/test/lib/mock-encryptor.js +++ b/test/lib/mock-encryptor.js @@ -1,5 +1,11 @@ const sinon = require('sinon'); +const PASSWORD = 'password123'; +const MOCK_ENCRYPTION_KEY = + '{"alg":"A256GCM","ext":true,"k":"wYmxkxOOFBDP6F6VuuYFcRt_Po-tSLFHCWVolsHs4VI","key_ops":["encrypt","decrypt"],"kty":"oct"}'; +const MOCK_ENCRYPTION_DATA = + '{"data":"2fOOPRKClNrisB+tmqIcETyZvDuL2iIR1Hr1nO7XZHyMqVY1cDBetw2gY5C+cIo1qkpyv3bPp+4buUjp38VBsjbijM0F/FLOqWbcuKM9h9X0uwxsgsZ96uwcIf5I46NiMgoFlhppTTMZT0Nkocz+SnvHM0IgLsFan7JqBU++vSJvx2M1PDljZSunOsqyyL+DKmbYmM4umbouKV42dipUwrCvrQJmpiUZrSkpMJrPJk9ufDQO4CyIVo0qry3aNRdYFJ6rgSyq/k6rXMwGExCMHn8UlhNnAMuMKWPWR/ymK1bzNcNs4VU14iVjEXOZGPvD9cvqVe/VtcnIba6axNEEB4HWDOCdrDh5YNWwMlQVL7vSB2yOhPZByGhnEOloYsj2E5KEb9jFGskt7EKDEYNofr6t83G0c+B72VGYZeCvgtzXzgPwzIbhTtKkP+gdBmt2JNSYrTjLypT0q+v4C9BN1xWTxPmX6TTt0NzkI9pJxgN1VQAfSU9CyWTVpd4CBkgom2cSBsxZ2MNbdKF+qSWz3fQcmJ55hxM0EGJSt9+8eQOTuoJlBapRk4wdZKHR2jdKzPjSF2MAmyVD2kU51IKa/cVsckRFEes+m7dKyHRvlNwgT78W9tBDdZb5PSlfbZXnv8z5q1KtAj2lM2ogJ7brHBdevl4FISdTkObpwcUMcvACOOO0dj6CSYjSKr0ZJ2RLChVruZyPDxEhKGb/8Kv8trLOR3mck/et6d050/NugezycNk4nnzu5iP90gPbSzaqdZI=","iv":"qTGO1afGv3waHN9KoW34Eg==","salt":"HQ5sfhsb8XAQRJtD+UqcImT7Ve4n3YMagrh05YTOsjk="}'; + const mockHex = '0xabcdef0123456789'; const mockKey = Buffer.alloc(32); let cacheVal; @@ -10,12 +16,22 @@ module.exports = { return Promise.resolve(mockHex); }), - decryptWithEncryptedKeyString(_keyStr) { - return this.decrypt(); + async decryptWithEncryptedKeyString(_keyStr) { + const { vault } = await this.decrypt(); + return vault; }, decrypt(_password, _text) { - return Promise.resolve(cacheVal ? { vault: cacheVal } : {}); + if (_password && _password !== PASSWORD) { + throw new Error('Incorrect password.'); + } + + const result = cacheVal ? { + vault: cacheVal, + extractedKeyString: MOCK_ENCRYPTION_KEY, + data: MOCK_ENCRYPTION_DATA, + } : {}; + return Promise.resolve(result); }, encryptWithKey(key, dataObj) { From fb461b53912aa88e1b188f7707298fba1cc36906 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Fri, 7 Oct 2022 11:28:49 -0500 Subject: [PATCH 08/55] Don't lock upon password --- index.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/index.js b/index.js index f0d78f79..d4758f46 100644 --- a/index.js +++ b/index.js @@ -178,10 +178,6 @@ class KeyringController extends EventEmitter { * @returns {Promise} A Promise that resolves to the state. */ async submitPassword(password) { - // If a user is trying to submit a password, we should lock - // in the event that the password is incorrect - this.setLocked(); - await this.verifyPassword(password); this.keyrings = await this.unlockKeyrings(password); From 1ff9e9ddffe0618bda14403ed35554bf5c18cf0f Mon Sep 17 00:00:00 2001 From: David Walsh Date: Fri, 7 Oct 2022 12:55:44 -0500 Subject: [PATCH 09/55] Add test to ensure decryptWithEncryptedKeyString --- index.js | 4 ++++ test/index.js | 30 +++++++++++++++++------------- test/lib/mock-encryptor.js | 14 ++++++++------ 3 files changed, 29 insertions(+), 19 deletions(-) diff --git a/index.js b/index.js index d4758f46..f0d78f79 100644 --- a/index.js +++ b/index.js @@ -178,6 +178,10 @@ class KeyringController extends EventEmitter { * @returns {Promise} A Promise that resolves to the state. */ async submitPassword(password) { + // If a user is trying to submit a password, we should lock + // in the event that the password is incorrect + this.setLocked(); + await this.verifyPassword(password); this.keyrings = await this.unlockKeyrings(password); diff --git a/test/index.js b/test/index.js index 87e87513..4c06cde6 100644 --- a/test/index.js +++ b/test/index.js @@ -11,10 +11,9 @@ const mockEncryptor = require('./lib/mock-encryptor'); const PASSWORD = 'password123'; const MOCK_ENCRYPTION_KEY = - '{"alg":"A256GCM","ext":true,"k":"wYmxkxOOFBDP6F6VuuYFcRt_Po-tSLFHCWVolsHs4VI","key_ops":["encrypt","decrypt"],"kty":"oct"}'; - const MOCK_ENCRYPTION_DATA = - '{"data":"2fOOPRKClNrisB+tmqIcETyZvDuL2iIR1Hr1nO7XZHyMqVY1cDBetw2gY5C+cIo1qkpyv3bPp+4buUjp38VBsjbijM0F/FLOqWbcuKM9h9X0uwxsgsZ96uwcIf5I46NiMgoFlhppTTMZT0Nkocz+SnvHM0IgLsFan7JqBU++vSJvx2M1PDljZSunOsqyyL+DKmbYmM4umbouKV42dipUwrCvrQJmpiUZrSkpMJrPJk9ufDQO4CyIVo0qry3aNRdYFJ6rgSyq/k6rXMwGExCMHn8UlhNnAMuMKWPWR/ymK1bzNcNs4VU14iVjEXOZGPvD9cvqVe/VtcnIba6axNEEB4HWDOCdrDh5YNWwMlQVL7vSB2yOhPZByGhnEOloYsj2E5KEb9jFGskt7EKDEYNofr6t83G0c+B72VGYZeCvgtzXzgPwzIbhTtKkP+gdBmt2JNSYrTjLypT0q+v4C9BN1xWTxPmX6TTt0NzkI9pJxgN1VQAfSU9CyWTVpd4CBkgom2cSBsxZ2MNbdKF+qSWz3fQcmJ55hxM0EGJSt9+8eQOTuoJlBapRk4wdZKHR2jdKzPjSF2MAmyVD2kU51IKa/cVsckRFEes+m7dKyHRvlNwgT78W9tBDdZb5PSlfbZXnv8z5q1KtAj2lM2ogJ7brHBdevl4FISdTkObpwcUMcvACOOO0dj6CSYjSKr0ZJ2RLChVruZyPDxEhKGb/8Kv8trLOR3mck/et6d050/NugezycNk4nnzu5iP90gPbSzaqdZI=","iv":"qTGO1afGv3waHN9KoW34Eg==","salt":"HQ5sfhsb8XAQRJtD+UqcImT7Ve4n3YMagrh05YTOsjk="}'; - + '{"alg":"A256GCM","ext":true,"k":"wYmxkxOOFBDP6F6VuuYFcRt_Po-tSLFHCWVolsHs4VI","key_ops":["encrypt","decrypt"],"kty":"oct"}'; +const MOCK_ENCRYPTION_DATA = + '{"data":"2fOOPRKClNrisB+tmqIcETyZvDuL2iIR1Hr1nO7XZHyMqVY1cDBetw2gY5C+cIo1qkpyv3bPp+4buUjp38VBsjbijM0F/FLOqWbcuKM9h9X0uwxsgsZ96uwcIf5I46NiMgoFlhppTTMZT0Nkocz+SnvHM0IgLsFan7JqBU++vSJvx2M1PDljZSunOsqyyL+DKmbYmM4umbouKV42dipUwrCvrQJmpiUZrSkpMJrPJk9ufDQO4CyIVo0qry3aNRdYFJ6rgSyq/k6rXMwGExCMHn8UlhNnAMuMKWPWR/ymK1bzNcNs4VU14iVjEXOZGPvD9cvqVe/VtcnIba6axNEEB4HWDOCdrDh5YNWwMlQVL7vSB2yOhPZByGhnEOloYsj2E5KEb9jFGskt7EKDEYNofr6t83G0c+B72VGYZeCvgtzXzgPwzIbhTtKkP+gdBmt2JNSYrTjLypT0q+v4C9BN1xWTxPmX6TTt0NzkI9pJxgN1VQAfSU9CyWTVpd4CBkgom2cSBsxZ2MNbdKF+qSWz3fQcmJ55hxM0EGJSt9+8eQOTuoJlBapRk4wdZKHR2jdKzPjSF2MAmyVD2kU51IKa/cVsckRFEes+m7dKyHRvlNwgT78W9tBDdZb5PSlfbZXnv8z5q1KtAj2lM2ogJ7brHBdevl4FISdTkObpwcUMcvACOOO0dj6CSYjSKr0ZJ2RLChVruZyPDxEhKGb/8Kv8trLOR3mck/et6d050/NugezycNk4nnzu5iP90gPbSzaqdZI=","iv":"qTGO1afGv3waHN9KoW34Eg==","salt":"HQ5sfhsb8XAQRJtD+UqcImT7Ve4n3YMagrh05YTOsjk="}'; const walletOneSeedWords = 'puzzle seed penalty soldier say clay field arctic metal hen cage runway'; @@ -460,26 +459,31 @@ describe('KeyringController', function () { }); }); - describe('submitEncryptionKey', function () { - it('sets encryption key data upon submitPassword', async function() { + it('sets encryption key data upon submitPassword', async function () { await keyringController.submitPassword(PASSWORD); expect(keyringController.encryptionKey).toBe(MOCK_ENCRYPTION_KEY); expect(keyringController.encryptionData).toBe(MOCK_ENCRYPTION_DATA); - }) + }); + + it('unlocks the keyrings with valid information', async function () { + const returnValue = + await keyringController.encryptor.decryptWithEncryptedKeyString(); + const stub = sinon.stub( + keyringController.encryptor, + 'decryptWithEncryptedKeyString', + ); + stub.resolves(Promise.resolve(returnValue)); - xit('unlocks the keyrings with valid information', async function () { - const stub = sinon.stub(keyringController.encryptor, 'decryptWithEncryptedKeyString') - stub.returns(await keyringController.encryptor.decrypt()); - await keyringController.submitEncryptionKey( MOCK_ENCRYPTION_KEY, MOCK_ENCRYPTION_DATA, ); - expect(keyringController.encryptor.decryptWithEncryptedKeyString.calledOnce).toBe(true); + expect( + keyringController.encryptor.decryptWithEncryptedKeyString.calledOnce, + ).toBe(true); }); }); - }); diff --git a/test/lib/mock-encryptor.js b/test/lib/mock-encryptor.js index e67cdfd2..51076572 100644 --- a/test/lib/mock-encryptor.js +++ b/test/lib/mock-encryptor.js @@ -2,7 +2,7 @@ const sinon = require('sinon'); const PASSWORD = 'password123'; const MOCK_ENCRYPTION_KEY = - '{"alg":"A256GCM","ext":true,"k":"wYmxkxOOFBDP6F6VuuYFcRt_Po-tSLFHCWVolsHs4VI","key_ops":["encrypt","decrypt"],"kty":"oct"}'; + '{"alg":"A256GCM","ext":true,"k":"wYmxkxOOFBDP6F6VuuYFcRt_Po-tSLFHCWVolsHs4VI","key_ops":["encrypt","decrypt"],"kty":"oct"}'; const MOCK_ENCRYPTION_DATA = '{"data":"2fOOPRKClNrisB+tmqIcETyZvDuL2iIR1Hr1nO7XZHyMqVY1cDBetw2gY5C+cIo1qkpyv3bPp+4buUjp38VBsjbijM0F/FLOqWbcuKM9h9X0uwxsgsZ96uwcIf5I46NiMgoFlhppTTMZT0Nkocz+SnvHM0IgLsFan7JqBU++vSJvx2M1PDljZSunOsqyyL+DKmbYmM4umbouKV42dipUwrCvrQJmpiUZrSkpMJrPJk9ufDQO4CyIVo0qry3aNRdYFJ6rgSyq/k6rXMwGExCMHn8UlhNnAMuMKWPWR/ymK1bzNcNs4VU14iVjEXOZGPvD9cvqVe/VtcnIba6axNEEB4HWDOCdrDh5YNWwMlQVL7vSB2yOhPZByGhnEOloYsj2E5KEb9jFGskt7EKDEYNofr6t83G0c+B72VGYZeCvgtzXzgPwzIbhTtKkP+gdBmt2JNSYrTjLypT0q+v4C9BN1xWTxPmX6TTt0NzkI9pJxgN1VQAfSU9CyWTVpd4CBkgom2cSBsxZ2MNbdKF+qSWz3fQcmJ55hxM0EGJSt9+8eQOTuoJlBapRk4wdZKHR2jdKzPjSF2MAmyVD2kU51IKa/cVsckRFEes+m7dKyHRvlNwgT78W9tBDdZb5PSlfbZXnv8z5q1KtAj2lM2ogJ7brHBdevl4FISdTkObpwcUMcvACOOO0dj6CSYjSKr0ZJ2RLChVruZyPDxEhKGb/8Kv8trLOR3mck/et6d050/NugezycNk4nnzu5iP90gPbSzaqdZI=","iv":"qTGO1afGv3waHN9KoW34Eg==","salt":"HQ5sfhsb8XAQRJtD+UqcImT7Ve4n3YMagrh05YTOsjk="}'; @@ -26,11 +26,13 @@ module.exports = { throw new Error('Incorrect password.'); } - const result = cacheVal ? { - vault: cacheVal, - extractedKeyString: MOCK_ENCRYPTION_KEY, - data: MOCK_ENCRYPTION_DATA, - } : {}; + const result = cacheVal + ? { + vault: cacheVal, + extractedKeyString: MOCK_ENCRYPTION_KEY, + data: MOCK_ENCRYPTION_DATA, + } + : {}; return Promise.resolve(result); }, From b200969ec3fa939dd041b7e1b6a522e137257901 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 10 Oct 2022 11:36:00 -0500 Subject: [PATCH 10/55] Add test to ensure persistAllKeyrings is being called --- test/index.js | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/test/index.js b/test/index.js index 4c06cde6..6ed4df86 100644 --- a/test/index.js +++ b/test/index.js @@ -485,5 +485,39 @@ describe('KeyringController', function () { keyringController.encryptor.decryptWithEncryptedKeyString.calledOnce, ).toBe(true); }); + + it('persists keyrings when actions are performed', async function () { + await keyringController.submitEncryptionKey( + MOCK_ENCRYPTION_KEY, + MOCK_ENCRYPTION_DATA, + ); + + const stub = sinon.stub(keyringController, 'persistAllKeyrings'); + stub.resolves(Promise.resolve(true)); + + const [firstKeyring] = keyringController.keyrings; + + await keyringController.addNewAccount(firstKeyring); + expect(stub.callCount).toBe(1); + + await keyringController.addNewAccount(firstKeyring); + expect(stub.callCount).toBe(2); + + const account = { + privateKey: + 'c87509a1c067bbde78beb793e6fa76530b6382a4c0241e5e4a9ec0a0f44dc0d3', + publicKey: '0x627306090abab3a6e1400e9345bc60c78a8bef57', + }; + + // Add a new keyring with one account + await keyringController.addNewKeyring('Simple Key Pair', [ + account.privateKey, + ]); + expect(stub.callCount).toBe(3); + + // remove that account that we just added + await keyringController.removeAccount(account.publicKey); + expect(stub.callCount).toBe(4); + }); }); }); From 52605f99341131721a3c4cc7ab1762ff665136a7 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Wed, 12 Oct 2022 14:25:30 -0500 Subject: [PATCH 11/55] Add encryption key to memStore so it's accessible to extension. Also update the encryption string during each save --- index.js | 24 ++++++++++++++---------- test/index.js | 32 ++++++++++++++++---------------- test/lib/mock-encryptor.js | 9 +++++---- 3 files changed, 35 insertions(+), 30 deletions(-) diff --git a/index.js b/index.js index f0d78f79..d01cd1fa 100644 --- a/index.js +++ b/index.js @@ -41,8 +41,9 @@ class KeyringController extends EventEmitter { this.store = new ObservableStore(initState); this.memStore = new ObservableStore({ isUnlocked: false, - keyringTypes: this.keyringTypes.map((krt) => krt.type), + keyringTypes: this.keyringTypes.map((keyring) => keyring.type), keyrings: [], + encryptionKey: null, }); this.encryptor = opts.encryptor || encryptor; @@ -150,12 +151,11 @@ class KeyringController extends EventEmitter { * @returns {Promise} A Promise that resolves to the state. */ async setLocked() { - delete this.encryptionKey; delete this.encryptionData; delete this.password; // set locked - this.memStore.updateState({ isUnlocked: false }); + this.memStore.updateState({ isUnlocked: false, encryptionKey: null }); // remove keyrings this.keyrings = []; @@ -550,7 +550,8 @@ class KeyringController extends EventEmitter { * @returns {Promise} Resolves to true once keyrings are persisted. */ async persistAllKeyrings() { - if (!this.password && !this.encryptionKey) { + const { encryptionKey } = this.memStore.getState(); + if (!this.password && !encryptionKey) { throw new Error( 'Cannot persist vault without password and encryption key', ); @@ -568,9 +569,12 @@ class KeyringController extends EventEmitter { let vault; if (this.password) { - vault = await this.encryptor.encrypt(this.password, serializedKeyrings); - } else if (this.encryptionKey) { - const key = await this.encryptor.createKeyFromString(this.encryptionKey); + const { vault: newVault, extractedKeyString } = + await this.encryptor.encrypt(this.password, serializedKeyrings); + vault = newVault; + this.memStore.updateState({ encryptionKey: extractedKeyString }); + } else if (encryptionKey) { + const key = await this.encryptor.createKeyFromString(encryptionKey); const vaultJSON = await this.encryptor.encryptWithKey( key, serializedKeyrings, @@ -612,16 +616,16 @@ class KeyringController extends EventEmitter { vault = result.vault; this.password = password; - this.encryptionKey = result.extractedKeyString; this.encryptionData = result.data; + this.memStore.updateState({ encryptionKey: result.extractedKeyString }); } else { vault = await this.encryptor.decryptWithEncryptedKeyString( encryptionKey, encryptionData, ); - this.encryptionKey = encryptionKey; this.encryptionData = encryptionData; + this.memStore.updateState({ encryptionKey }); } await Promise.all(vault.map(this._restoreKeyring.bind(this))); @@ -678,7 +682,7 @@ class KeyringController extends EventEmitter { * @returns {Keyring|undefined} The class, if it exists. */ getKeyringClassForType(type) { - return this.keyringTypes.find((kr) => kr.type === type); + return this.keyringTypes.find((keyring) => keyring.type === type); } /** diff --git a/test/index.js b/test/index.js index 6ed4df86..5877d278 100644 --- a/test/index.js +++ b/test/index.js @@ -15,17 +15,18 @@ const MOCK_ENCRYPTION_KEY = const MOCK_ENCRYPTION_DATA = '{"data":"2fOOPRKClNrisB+tmqIcETyZvDuL2iIR1Hr1nO7XZHyMqVY1cDBetw2gY5C+cIo1qkpyv3bPp+4buUjp38VBsjbijM0F/FLOqWbcuKM9h9X0uwxsgsZ96uwcIf5I46NiMgoFlhppTTMZT0Nkocz+SnvHM0IgLsFan7JqBU++vSJvx2M1PDljZSunOsqyyL+DKmbYmM4umbouKV42dipUwrCvrQJmpiUZrSkpMJrPJk9ufDQO4CyIVo0qry3aNRdYFJ6rgSyq/k6rXMwGExCMHn8UlhNnAMuMKWPWR/ymK1bzNcNs4VU14iVjEXOZGPvD9cvqVe/VtcnIba6axNEEB4HWDOCdrDh5YNWwMlQVL7vSB2yOhPZByGhnEOloYsj2E5KEb9jFGskt7EKDEYNofr6t83G0c+B72VGYZeCvgtzXzgPwzIbhTtKkP+gdBmt2JNSYrTjLypT0q+v4C9BN1xWTxPmX6TTt0NzkI9pJxgN1VQAfSU9CyWTVpd4CBkgom2cSBsxZ2MNbdKF+qSWz3fQcmJ55hxM0EGJSt9+8eQOTuoJlBapRk4wdZKHR2jdKzPjSF2MAmyVD2kU51IKa/cVsckRFEes+m7dKyHRvlNwgT78W9tBDdZb5PSlfbZXnv8z5q1KtAj2lM2ogJ7brHBdevl4FISdTkObpwcUMcvACOOO0dj6CSYjSKr0ZJ2RLChVruZyPDxEhKGb/8Kv8trLOR3mck/et6d050/NugezycNk4nnzu5iP90gPbSzaqdZI=","iv":"qTGO1afGv3waHN9KoW34Eg==","salt":"HQ5sfhsb8XAQRJtD+UqcImT7Ve4n3YMagrh05YTOsjk="}'; -const walletOneSeedWords = +const WALLET_ONE_SEED_WORDS = 'puzzle seed penalty soldier say clay field arctic metal hen cage runway'; -const walletOneAddresses = ['0xef35ca8ebb9669a35c31b5f6f249a9941a812ac1']; +const WALLET_ONE_ADDRESSES = ['0xef35ca8ebb9669a35c31b5f6f249a9941a812ac1']; -const walletTwoSeedWords = +const WALLET_TWO_SEED_WORDS = 'urge letter protect palace city barely security section midnight wealth south deer'; -const walletTwoAddresses = [ +const WALLET_TWO_ADDRESSES = [ '0xbbafcf3d00fb625b65bb1497c94bf42c1f4b3e78', '0x49dd2653f38f75d40fdbd51e83b9c9724c87f7eb', ]; + describe('KeyringController', function () { let keyringController; @@ -127,17 +128,17 @@ describe('KeyringController', function () { await keyringController.createNewVaultAndRestore( PASSWORD, - walletOneSeedWords, + WALLET_ONE_SEED_WORDS, ); const allAccountsAfter = await keyringController.getAccounts(); expect(allAccountsAfter).toHaveLength(1); - expect(allAccountsAfter[0]).toBe(walletOneAddresses[0]); + expect(allAccountsAfter[0]).toBe(WALLET_ONE_ADDRESSES[0]); }); it('throws error if argument password is not a string', async function () { await expect(() => - keyringController.createNewVaultAndRestore(12, walletTwoSeedWords), + keyringController.createNewVaultAndRestore(12, WALLET_TWO_SEED_WORDS), ).rejects.toThrow('Password must be text.'); }); @@ -152,9 +153,9 @@ describe('KeyringController', function () { it('accepts mnemonic passed as type array of numbers', async function () { const allAccountsBefore = await keyringController.getAccounts(); - expect(allAccountsBefore[0]).not.toBe(walletTwoAddresses[0]); + expect(allAccountsBefore[0]).not.toBe(WALLET_TWO_ADDRESSES[0]); const mnemonicAsArrayOfNumbers = Array.from( - Buffer.from(walletTwoSeedWords).values(), + Buffer.from(WALLET_TWO_SEED_WORDS).values(), ); await keyringController.createNewVaultAndRestore( @@ -164,7 +165,7 @@ describe('KeyringController', function () { const allAccountsAfter = await keyringController.getAccounts(); expect(allAccountsAfter).toHaveLength(1); - expect(allAccountsAfter[0]).toBe(walletTwoAddresses[0]); + expect(allAccountsAfter[0]).toBe(WALLET_TWO_ADDRESSES[0]); }); }); @@ -204,12 +205,12 @@ describe('KeyringController', function () { expect(previousAllAccounts).toHaveLength(1); const keyring = await keyringController.addNewKeyring('HD Key Tree', { numberOfAccounts: 2, - mnemonic: walletTwoSeedWords, + mnemonic: WALLET_TWO_SEED_WORDS, }); const keyringAccounts = await keyring.getAccounts(); expect(keyringAccounts).toHaveLength(2); - expect(keyringAccounts[0]).toStrictEqual(walletTwoAddresses[0]); - expect(keyringAccounts[1]).toStrictEqual(walletTwoAddresses[1]); + expect(keyringAccounts[0]).toStrictEqual(WALLET_TWO_ADDRESSES[0]); + expect(keyringAccounts[1]).toStrictEqual(WALLET_TWO_ADDRESSES[1]); const allAccounts = await keyringController.getAccounts(); expect(allAccounts).toHaveLength(3); }); @@ -220,7 +221,7 @@ describe('KeyringController', function () { const mockSerialized = { type: 'HD Key Tree', data: { - mnemonic: walletOneSeedWords, + mnemonic: WALLET_ONE_SEED_WORDS, numberOfAccounts: 1, }, }; @@ -229,7 +230,7 @@ describe('KeyringController', function () { expect(keyring.wallets).toHaveLength(1); const accounts = await keyring.getAccounts(); - expect(accounts[0]).toBe(walletOneAddresses[0]); + expect(accounts[0]).toBe(WALLET_ONE_ADDRESSES[0]); }); }); @@ -463,7 +464,6 @@ describe('KeyringController', function () { it('sets encryption key data upon submitPassword', async function () { await keyringController.submitPassword(PASSWORD); - expect(keyringController.encryptionKey).toBe(MOCK_ENCRYPTION_KEY); expect(keyringController.encryptionData).toBe(MOCK_ENCRYPTION_DATA); }); diff --git a/test/lib/mock-encryptor.js b/test/lib/mock-encryptor.js index 51076572..210237df 100644 --- a/test/lib/mock-encryptor.js +++ b/test/lib/mock-encryptor.js @@ -6,14 +6,15 @@ const MOCK_ENCRYPTION_KEY = const MOCK_ENCRYPTION_DATA = '{"data":"2fOOPRKClNrisB+tmqIcETyZvDuL2iIR1Hr1nO7XZHyMqVY1cDBetw2gY5C+cIo1qkpyv3bPp+4buUjp38VBsjbijM0F/FLOqWbcuKM9h9X0uwxsgsZ96uwcIf5I46NiMgoFlhppTTMZT0Nkocz+SnvHM0IgLsFan7JqBU++vSJvx2M1PDljZSunOsqyyL+DKmbYmM4umbouKV42dipUwrCvrQJmpiUZrSkpMJrPJk9ufDQO4CyIVo0qry3aNRdYFJ6rgSyq/k6rXMwGExCMHn8UlhNnAMuMKWPWR/ymK1bzNcNs4VU14iVjEXOZGPvD9cvqVe/VtcnIba6axNEEB4HWDOCdrDh5YNWwMlQVL7vSB2yOhPZByGhnEOloYsj2E5KEb9jFGskt7EKDEYNofr6t83G0c+B72VGYZeCvgtzXzgPwzIbhTtKkP+gdBmt2JNSYrTjLypT0q+v4C9BN1xWTxPmX6TTt0NzkI9pJxgN1VQAfSU9CyWTVpd4CBkgom2cSBsxZ2MNbdKF+qSWz3fQcmJ55hxM0EGJSt9+8eQOTuoJlBapRk4wdZKHR2jdKzPjSF2MAmyVD2kU51IKa/cVsckRFEes+m7dKyHRvlNwgT78W9tBDdZb5PSlfbZXnv8z5q1KtAj2lM2ogJ7brHBdevl4FISdTkObpwcUMcvACOOO0dj6CSYjSKr0ZJ2RLChVruZyPDxEhKGb/8Kv8trLOR3mck/et6d050/NugezycNk4nnzu5iP90gPbSzaqdZI=","iv":"qTGO1afGv3waHN9KoW34Eg==","salt":"HQ5sfhsb8XAQRJtD+UqcImT7Ve4n3YMagrh05YTOsjk="}'; -const mockHex = '0xabcdef0123456789'; -const mockKey = Buffer.alloc(32); +const MOCK_HEX = '0xabcdef0123456789'; +const MOCK_KEY = Buffer.alloc(32); let cacheVal; module.exports = { encrypt: sinon.stub().callsFake(function (_password, dataObj) { cacheVal = dataObj; - return Promise.resolve(mockHex); + + return Promise.resolve({ vault: MOCK_HEX, extractedKeyString: '' }); }), async decryptWithEncryptedKeyString(_keyStr) { @@ -45,7 +46,7 @@ module.exports = { }, keyFromPassword(_password) { - return Promise.resolve(mockKey); + return Promise.resolve(MOCK_KEY); }, generateSalt() { From 645ecd96ae4672395ae6436e997eb216bc70da85 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Thu, 13 Oct 2022 11:28:20 -0500 Subject: [PATCH 12/55] Add jsdoc for submitEncryptionKey, code cleanups --- index.js | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/index.js b/index.js index d01cd1fa..4ad196c6 100644 --- a/index.js +++ b/index.js @@ -41,7 +41,7 @@ class KeyringController extends EventEmitter { this.store = new ObservableStore(initState); this.memStore = new ObservableStore({ isUnlocked: false, - keyringTypes: this.keyringTypes.map((keyring) => keyring.type), + keyringTypes: this.keyringTypes.map((keyringType) => keyringType.type), keyrings: [], encryptionKey: null, }); @@ -191,6 +191,17 @@ class KeyringController extends EventEmitter { this.fullUpdate(); } + /** + * Submit Encryption Key + * + * Attempts to decrypt the current vault and load its keyrings + * into memory based on the vault and CryptoKey information + * + * @emits KeyringController#unlock + * @param {string} encryptionKey - The encrypted key information used to decrypt the vault + * @param {string} loginData - The vault data to decrypt + * @returns {Promise} A Promise that resolves to the state. + */ async submitEncryptionKey(encryptionKey, loginData) { this.keyrings = await this.unlockKeyrings( undefined, @@ -551,6 +562,7 @@ class KeyringController extends EventEmitter { */ async persistAllKeyrings() { const { encryptionKey } = this.memStore.getState(); + if (!this.password && !encryptionKey) { throw new Error( 'Cannot persist vault without password and encryption key', From 8e8e771f3849859c4409405ee66866a3e33de3fb Mon Sep 17 00:00:00 2001 From: David Walsh Date: Thu, 13 Oct 2022 18:46:27 -0500 Subject: [PATCH 13/55] Avoid using encryptionData when all that matters is salt --- index.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/index.js b/index.js index 4ad196c6..f8c1aa6d 100644 --- a/index.js +++ b/index.js @@ -151,7 +151,7 @@ class KeyringController extends EventEmitter { * @returns {Promise} A Promise that resolves to the state. */ async setLocked() { - delete this.encryptionData; + delete this.encryptionSalt; delete this.password; // set locked @@ -591,7 +591,7 @@ class KeyringController extends EventEmitter { key, serializedKeyrings, ); - vaultJSON.salt = JSON.parse(this.encryptionData).salt; + vaultJSON.salt = this.encryptionSalt; vault = JSON.stringify(vaultJSON); } @@ -628,7 +628,7 @@ class KeyringController extends EventEmitter { vault = result.vault; this.password = password; - this.encryptionData = result.data; + this.encryptionSalt = result.salt; this.memStore.updateState({ encryptionKey: result.extractedKeyString }); } else { vault = await this.encryptor.decryptWithEncryptedKeyString( @@ -636,7 +636,7 @@ class KeyringController extends EventEmitter { encryptionData, ); - this.encryptionData = encryptionData; + this.encryptionSalt = JSON.parse(encryptionData).salt; this.memStore.updateState({ encryptionKey }); } From 6e840ccbd1b619ed01f21b9878c8d45ee1d7c38b Mon Sep 17 00:00:00 2001 From: David Walsh Date: Thu, 13 Oct 2022 19:08:09 -0500 Subject: [PATCH 14/55] Fix test --- test/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/index.js b/test/index.js index 5877d278..8a2bbf70 100644 --- a/test/index.js +++ b/test/index.js @@ -464,7 +464,7 @@ describe('KeyringController', function () { it('sets encryption key data upon submitPassword', async function () { await keyringController.submitPassword(PASSWORD); - expect(keyringController.encryptionData).toBe(MOCK_ENCRYPTION_DATA); + expect(keyringController.salt).toBe(MOCK_ENCRYPTION_DATA.salt); }); it('unlocks the keyrings with valid information', async function () { From 76d1adf9ba76519d2ab56d42c877797da3865858 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Fri, 14 Oct 2022 09:31:24 -0500 Subject: [PATCH 15/55] Ensure keyring is unlocked when new vault is created --- index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/index.js b/index.js index f8c1aa6d..686e0a0f 100644 --- a/index.js +++ b/index.js @@ -84,7 +84,7 @@ class KeyringController extends EventEmitter { await this.createFirstKeyTree(password); await this.persistAllKeyrings(); - this.setUnlocked.bind(); + this.setUnlocked(); this.fullUpdate(); } From ef8dc4fce5c2638712c12039ab17ffd92ab189a4 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Fri, 14 Oct 2022 15:31:27 -0500 Subject: [PATCH 16/55] Call _updateMemStoreKeyrings inside persistAllKeyrings --- index.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/index.js b/index.js index 686e0a0f..60d78e69 100644 --- a/index.js +++ b/index.js @@ -257,7 +257,6 @@ class KeyringController extends EventEmitter { this.keyrings.push(keyring); await this.persistAllKeyrings(); - await this._updateMemStoreKeyrings(); this.fullUpdate(); return keyring; @@ -339,7 +338,6 @@ class KeyringController extends EventEmitter { }); await this.persistAllKeyrings(); - await this._updateMemStoreKeyrings(); this.fullUpdate(); } @@ -389,7 +387,6 @@ class KeyringController extends EventEmitter { } await this.persistAllKeyrings(); - await this._updateMemStoreKeyrings(); this.fullUpdate(); } @@ -580,11 +577,14 @@ class KeyringController extends EventEmitter { ); let vault; + let newExtractedKeyString; + if (this.password) { const { vault: newVault, extractedKeyString } = await this.encryptor.encrypt(this.password, serializedKeyrings); + vault = newVault; - this.memStore.updateState({ encryptionKey: extractedKeyString }); + newExtractedKeyString = extractedKeyString; } else if (encryptionKey) { const key = await this.encryptor.createKeyFromString(encryptionKey); const vaultJSON = await this.encryptor.encryptWithKey( @@ -601,6 +601,12 @@ class KeyringController extends EventEmitter { this.store.updateState({ vault }); + // The keyring updates need to be announced before updating the extractedKeyStrings + await this._updateMemStoreKeyrings(); + if (newExtractedKeyString) { + this.memStore.updateState({ encryptionKey: newExtractedKeyString }); + } + return true; } @@ -836,7 +842,6 @@ class KeyringController extends EventEmitter { if (keyring.forgetDevice) { keyring.forgetDevice(); this.persistAllKeyrings.bind(this)(); - this._updateMemStoreKeyrings.bind(this)(); } else { throw new Error( `KeyringController - keyring does not have method "forgetDevice", keyring type: ${keyring.type}`, From 2dafa98aa19b594027f25cfe4146689997b6d5e0 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 17 Oct 2022 15:26:39 -0500 Subject: [PATCH 17/55] Add, improve tests --- index.js | 2 ++ test/index.js | 13 ++++++++++++- test/lib/mock-encryptor.js | 4 +--- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/index.js b/index.js index 60d78e69..6b83d594 100644 --- a/index.js +++ b/index.js @@ -185,6 +185,8 @@ class KeyringController extends EventEmitter { await this.verifyPassword(password); this.keyrings = await this.unlockKeyrings(password); + this.password = password; + await this.persistAllKeyrings(); this.setUnlocked(); diff --git a/test/index.js b/test/index.js index 8a2bbf70..97fc2587 100644 --- a/test/index.js +++ b/test/index.js @@ -464,7 +464,8 @@ describe('KeyringController', function () { it('sets encryption key data upon submitPassword', async function () { await keyringController.submitPassword(PASSWORD); - expect(keyringController.salt).toBe(MOCK_ENCRYPTION_DATA.salt); + expect(keyringController.password).toBe(PASSWORD); + expect(keyringController.encryptionSalt).toBe('SALT'); }); it('unlocks the keyrings with valid information', async function () { @@ -519,5 +520,15 @@ describe('KeyringController', function () { await keyringController.removeAccount(account.publicKey); expect(stub.callCount).toBe(4); }); + + it('cleans up login artifacts upon lock', async function () { + await keyringController.submitPassword(PASSWORD); + + await keyringController.setLocked(); + + expect(keyringController.encryptionSalt).toBeUndefined(); + expect(keyringController.password).toBeUndefined(); + expect(keyringController.memStore.getState().encryptionKey).toBeNull(); + }); }); }); diff --git a/test/lib/mock-encryptor.js b/test/lib/mock-encryptor.js index 210237df..bca790c3 100644 --- a/test/lib/mock-encryptor.js +++ b/test/lib/mock-encryptor.js @@ -3,8 +3,6 @@ const sinon = require('sinon'); const PASSWORD = 'password123'; const MOCK_ENCRYPTION_KEY = '{"alg":"A256GCM","ext":true,"k":"wYmxkxOOFBDP6F6VuuYFcRt_Po-tSLFHCWVolsHs4VI","key_ops":["encrypt","decrypt"],"kty":"oct"}'; -const MOCK_ENCRYPTION_DATA = - '{"data":"2fOOPRKClNrisB+tmqIcETyZvDuL2iIR1Hr1nO7XZHyMqVY1cDBetw2gY5C+cIo1qkpyv3bPp+4buUjp38VBsjbijM0F/FLOqWbcuKM9h9X0uwxsgsZ96uwcIf5I46NiMgoFlhppTTMZT0Nkocz+SnvHM0IgLsFan7JqBU++vSJvx2M1PDljZSunOsqyyL+DKmbYmM4umbouKV42dipUwrCvrQJmpiUZrSkpMJrPJk9ufDQO4CyIVo0qry3aNRdYFJ6rgSyq/k6rXMwGExCMHn8UlhNnAMuMKWPWR/ymK1bzNcNs4VU14iVjEXOZGPvD9cvqVe/VtcnIba6axNEEB4HWDOCdrDh5YNWwMlQVL7vSB2yOhPZByGhnEOloYsj2E5KEb9jFGskt7EKDEYNofr6t83G0c+B72VGYZeCvgtzXzgPwzIbhTtKkP+gdBmt2JNSYrTjLypT0q+v4C9BN1xWTxPmX6TTt0NzkI9pJxgN1VQAfSU9CyWTVpd4CBkgom2cSBsxZ2MNbdKF+qSWz3fQcmJ55hxM0EGJSt9+8eQOTuoJlBapRk4wdZKHR2jdKzPjSF2MAmyVD2kU51IKa/cVsckRFEes+m7dKyHRvlNwgT78W9tBDdZb5PSlfbZXnv8z5q1KtAj2lM2ogJ7brHBdevl4FISdTkObpwcUMcvACOOO0dj6CSYjSKr0ZJ2RLChVruZyPDxEhKGb/8Kv8trLOR3mck/et6d050/NugezycNk4nnzu5iP90gPbSzaqdZI=","iv":"qTGO1afGv3waHN9KoW34Eg==","salt":"HQ5sfhsb8XAQRJtD+UqcImT7Ve4n3YMagrh05YTOsjk="}'; const MOCK_HEX = '0xabcdef0123456789'; const MOCK_KEY = Buffer.alloc(32); @@ -31,7 +29,7 @@ module.exports = { ? { vault: cacheVal, extractedKeyString: MOCK_ENCRYPTION_KEY, - data: MOCK_ENCRYPTION_DATA, + salt: 'SALT', } : {}; return Promise.resolve(result); From 751fbfef55d78d853b8780266c6b3d9eb1eaf986 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 17 Oct 2022 17:22:16 -0500 Subject: [PATCH 18/55] Remove persistAllKeyrings check which isn't necessary --- index.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/index.js b/index.js index 6b83d594..daa66bbd 100644 --- a/index.js +++ b/index.js @@ -187,8 +187,6 @@ class KeyringController extends EventEmitter { this.password = password; - await this.persistAllKeyrings(); - this.setUnlocked(); this.fullUpdate(); } @@ -632,11 +630,10 @@ class KeyringController extends EventEmitter { let vault; if (password) { const result = await this.encryptor.decrypt(password, encryptedVault); - vault = result.vault; - - this.password = password; this.encryptionSalt = result.salt; + this.password = password; + this.memStore.updateState({ encryptionKey: result.extractedKeyString }); } else { vault = await this.encryptor.decryptWithEncryptedKeyString( From db4dc1da3d2dc4b1eac7d1e5c252930d50934d9e Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 17 Oct 2022 17:31:53 -0500 Subject: [PATCH 19/55] Remove unnecessary password arguments --- index.js | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/index.js b/index.js index daa66bbd..4345ed2e 100644 --- a/index.js +++ b/index.js @@ -82,7 +82,7 @@ class KeyringController extends EventEmitter { async createNewVaultAndKeychain(password) { this.password = password; - await this.createFirstKeyTree(password); + await this.createFirstKeyTree(); await this.persistAllKeyrings(); this.setUnlocked(); this.fullUpdate(); @@ -131,7 +131,6 @@ class KeyringController extends EventEmitter { mnemonic: seedPhraseAsBuffer, numberOfAccounts: 1, }, - password, ); const [firstAccount] = await firstKeyring.getAccounts(); if (!firstAccount) { @@ -243,7 +242,7 @@ class KeyringController extends EventEmitter { * @param {Object} opts - The constructor options for the keyring. * @returns {Promise} The new keyring. */ - async addNewKeyring(type, opts) { + async addNewKeyring(type, opts = {}) { const Keyring = this.getKeyringClassForType(type); const keyring = new Keyring(opts); if ((!opts || !opts.mnemonic) && type === KEYRINGS_TYPE_MAP.HD_KEYRING) { @@ -528,14 +527,10 @@ class KeyringController extends EventEmitter { * @param {string} password - The keyring controller password. * @returns {Promise} - A promise that resolves if the operation was successful. */ - async createFirstKeyTree(password) { + async createFirstKeyTree() { this.clearKeyrings(); - const keyring = await this.addNewKeyring( - KEYRINGS_TYPE_MAP.HD_KEYRING, - {}, - password, - ); + const keyring = await this.addNewKeyring(KEYRINGS_TYPE_MAP.HD_KEYRING); const [firstAccount] = await keyring.getAccounts(); if (!firstAccount) { throw new Error('KeyringController - No account found on keychain.'); @@ -554,7 +549,6 @@ class KeyringController extends EventEmitter { * encrypts that array with the provided `password`, * and persists that encrypted string to storage. * - * @param {string} password - The keyring controller password. * @returns {Promise} Resolves to true once keyrings are persisted. */ async persistAllKeyrings() { From e141b845288c42e3590e278463ad66e72f6052bf Mon Sep 17 00:00:00 2001 From: David Walsh Date: Tue, 18 Oct 2022 10:11:14 -0500 Subject: [PATCH 20/55] Remove useless return value --- index.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/index.js b/index.js index 4345ed2e..e2dc351a 100644 --- a/index.js +++ b/index.js @@ -209,8 +209,6 @@ class KeyringController extends EventEmitter { ); this.setUnlocked(); this.fullUpdate(); - - return true; } /** From 2a2e53b24b0f77f9a10f505610350baa06984f5a Mon Sep 17 00:00:00 2001 From: David Walsh Date: Thu, 20 Oct 2022 09:50:07 -0500 Subject: [PATCH 21/55] Avoid breaking changes to browser-passworder --- index.js | 10 ++++++++-- test/lib/mock-encryptor.js | 14 ++++++++++++-- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/index.js b/index.js index e2dc351a..c64f999c 100644 --- a/index.js +++ b/index.js @@ -573,7 +573,10 @@ class KeyringController extends EventEmitter { if (this.password) { const { vault: newVault, extractedKeyString } = - await this.encryptor.encrypt(this.password, serializedKeyrings); + await this.encryptor.encryptWithDetail( + this.password, + serializedKeyrings, + ); vault = newVault; newExtractedKeyString = extractedKeyString; @@ -621,7 +624,10 @@ class KeyringController extends EventEmitter { let vault; if (password) { - const result = await this.encryptor.decrypt(password, encryptedVault); + const result = await this.encryptor.decryptWithDetail( + password, + encryptedVault, + ); vault = result.vault; this.encryptionSalt = result.salt; this.password = password; diff --git a/test/lib/mock-encryptor.js b/test/lib/mock-encryptor.js index bca790c3..7526fc2f 100644 --- a/test/lib/mock-encryptor.js +++ b/test/lib/mock-encryptor.js @@ -12,15 +12,25 @@ module.exports = { encrypt: sinon.stub().callsFake(function (_password, dataObj) { cacheVal = dataObj; + return Promise.resolve(MOCK_HEX); + }), + + encryptWithDetail: sinon.stub().callsFake(function (_password, dataObj) { + cacheVal = dataObj; + return Promise.resolve({ vault: MOCK_HEX, extractedKeyString: '' }); }), + async decrypt(_password, _text) { + return Promise.resolve(cacheVal || {}); + }, + async decryptWithEncryptedKeyString(_keyStr) { - const { vault } = await this.decrypt(); + const { vault } = await this.decryptWithDetail(); return vault; }, - decrypt(_password, _text) { + decryptWithDetail(_password, _text) { if (_password && _password !== PASSWORD) { throw new Error('Incorrect password.'); } From 0ace5f5093ea3a2b328105589e9f0758a6b87f67 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Wed, 26 Oct 2022 11:28:00 -0500 Subject: [PATCH 22/55] submitEncryptionKey doesn't need to require encryptedData -- we already have it --- index.js | 14 +++++--------- test/index.js | 12 ++++-------- 2 files changed, 9 insertions(+), 17 deletions(-) diff --git a/index.js b/index.js index c64f999c..19b02b1a 100644 --- a/index.js +++ b/index.js @@ -201,12 +201,8 @@ class KeyringController extends EventEmitter { * @param {string} loginData - The vault data to decrypt * @returns {Promise} A Promise that resolves to the state. */ - async submitEncryptionKey(encryptionKey, loginData) { - this.keyrings = await this.unlockKeyrings( - undefined, - encryptionKey, - loginData, - ); + async submitEncryptionKey(encryptionKey) { + this.keyrings = await this.unlockKeyrings(undefined, encryptionKey); this.setUnlocked(); this.fullUpdate(); } @@ -614,7 +610,7 @@ class KeyringController extends EventEmitter { * @param {string} password - The keyring controller password. * @returns {Promise>} The keyrings. */ - async unlockKeyrings(password, encryptionKey, encryptionData) { + async unlockKeyrings(password, encryptionKey) { const encryptedVault = this.store.getState().vault; if (!encryptedVault) { throw new Error('Cannot unlock without a previous vault.'); @@ -636,10 +632,10 @@ class KeyringController extends EventEmitter { } else { vault = await this.encryptor.decryptWithEncryptedKeyString( encryptionKey, - encryptionData, + encryptedVault, ); - this.encryptionSalt = JSON.parse(encryptionData).salt; + this.encryptionSalt = JSON.parse(encryptedVault).salt; this.memStore.updateState({ encryptionKey }); } diff --git a/test/index.js b/test/index.js index 97fc2587..5a9a0533 100644 --- a/test/index.js +++ b/test/index.js @@ -477,10 +477,8 @@ describe('KeyringController', function () { ); stub.resolves(Promise.resolve(returnValue)); - await keyringController.submitEncryptionKey( - MOCK_ENCRYPTION_KEY, - MOCK_ENCRYPTION_DATA, - ); + keyringController.store.updateState({ vault: MOCK_ENCRYPTION_DATA }); + await keyringController.submitEncryptionKey(MOCK_ENCRYPTION_KEY); expect( keyringController.encryptor.decryptWithEncryptedKeyString.calledOnce, @@ -488,10 +486,8 @@ describe('KeyringController', function () { }); it('persists keyrings when actions are performed', async function () { - await keyringController.submitEncryptionKey( - MOCK_ENCRYPTION_KEY, - MOCK_ENCRYPTION_DATA, - ); + keyringController.store.updateState({ vault: MOCK_ENCRYPTION_DATA }); + await keyringController.submitEncryptionKey(MOCK_ENCRYPTION_KEY); const stub = sinon.stub(keyringController, 'persistAllKeyrings'); stub.resolves(Promise.resolve(true)); From a5fe1dce1cb6aa203e459d555ad2cff8ccbd235a Mon Sep 17 00:00:00 2001 From: David Walsh Date: Wed, 26 Oct 2022 11:49:57 -0500 Subject: [PATCH 23/55] Remove usage of decryptWithEncryptedKeyString from encryptor --- index.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/index.js b/index.js index 19b02b1a..f003ff7d 100644 --- a/index.js +++ b/index.js @@ -202,7 +202,10 @@ class KeyringController extends EventEmitter { * @returns {Promise} A Promise that resolves to the state. */ async submitEncryptionKey(encryptionKey) { - this.keyrings = await this.unlockKeyrings(undefined, encryptionKey); + this.keyrings = await this.unlockKeyrings( + undefined, + encryptionKey + ); this.setUnlocked(); this.fullUpdate(); } @@ -630,10 +633,8 @@ class KeyringController extends EventEmitter { this.memStore.updateState({ encryptionKey: result.extractedKeyString }); } else { - vault = await this.encryptor.decryptWithEncryptedKeyString( - encryptionKey, - encryptedVault, - ); + const key = await this.encryptor.createKeyFromString(encryptionKey); + vault = await this.encryptor.decryptWithKey(key, JSON.parse(encryptedVault)); this.encryptionSalt = JSON.parse(encryptedVault).salt; this.memStore.updateState({ encryptionKey }); From e28b476446858b9cc264b253b25062338f342468 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Wed, 26 Oct 2022 12:34:15 -0500 Subject: [PATCH 24/55] Rename extractedKeyString to exportedKeyString --- index.js | 14 +++++++------- test/lib/mock-encryptor.js | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/index.js b/index.js index f003ff7d..45e95494 100644 --- a/index.js +++ b/index.js @@ -568,17 +568,17 @@ class KeyringController extends EventEmitter { ); let vault; - let newExtractedKeyString; + let newExportedKeyString; if (this.password) { - const { vault: newVault, extractedKeyString } = + const { vault: newVault, exportedKeyString } = await this.encryptor.encryptWithDetail( this.password, serializedKeyrings, ); vault = newVault; - newExtractedKeyString = extractedKeyString; + newExportedKeyString = exportedKeyString; } else if (encryptionKey) { const key = await this.encryptor.createKeyFromString(encryptionKey); const vaultJSON = await this.encryptor.encryptWithKey( @@ -595,10 +595,10 @@ class KeyringController extends EventEmitter { this.store.updateState({ vault }); - // The keyring updates need to be announced before updating the extractedKeyStrings + // The keyring updates need to be announced before updating the exportedKeyStrings await this._updateMemStoreKeyrings(); - if (newExtractedKeyString) { - this.memStore.updateState({ encryptionKey: newExtractedKeyString }); + if (newExportedKeyString) { + this.memStore.updateState({ encryptionKey: newExportedKeyString }); } return true; @@ -631,7 +631,7 @@ class KeyringController extends EventEmitter { this.encryptionSalt = result.salt; this.password = password; - this.memStore.updateState({ encryptionKey: result.extractedKeyString }); + this.memStore.updateState({ encryptionKey: result.exportedKeyString }); } else { const key = await this.encryptor.createKeyFromString(encryptionKey); vault = await this.encryptor.decryptWithKey(key, JSON.parse(encryptedVault)); diff --git a/test/lib/mock-encryptor.js b/test/lib/mock-encryptor.js index 7526fc2f..78331e2a 100644 --- a/test/lib/mock-encryptor.js +++ b/test/lib/mock-encryptor.js @@ -18,7 +18,7 @@ module.exports = { encryptWithDetail: sinon.stub().callsFake(function (_password, dataObj) { cacheVal = dataObj; - return Promise.resolve({ vault: MOCK_HEX, extractedKeyString: '' }); + return Promise.resolve({ vault: MOCK_HEX, exportedKeyString: '' }); }), async decrypt(_password, _text) { @@ -38,7 +38,7 @@ module.exports = { const result = cacheVal ? { vault: cacheVal, - extractedKeyString: MOCK_ENCRYPTION_KEY, + exportedKeyString: MOCK_ENCRYPTION_KEY, salt: 'SALT', } : {}; From 7cd4d40c87b156eb753079fc126e44febfbacfa9 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Thu, 27 Oct 2022 10:19:19 -0500 Subject: [PATCH 25/55] Pin version of browser-passworder --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index f0df0cba..fe6359d6 100644 --- a/package.json +++ b/package.json @@ -33,7 +33,7 @@ "dependencies": { "@metamask/bip39": "^4.0.0", "@metamask/eth-hd-keyring": "^4.0.2", - "browser-passworder": "MetaMask/browser-passworder#export-key", + "browser-passworder": "MetaMask/browser-passworder#92d97d82ead553f135232a0f81de81e1a4a69782", "eth-sig-util": "^3.0.1", "eth-simple-keyring": "^4.2.0", "obs-store": "^4.0.3" From dc73e9595f56c370d6e74d942922c78ec46d0b89 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 31 Oct 2022 12:12:10 -0500 Subject: [PATCH 26/55] Update build --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index fe6359d6..1af082e5 100644 --- a/package.json +++ b/package.json @@ -33,7 +33,7 @@ "dependencies": { "@metamask/bip39": "^4.0.0", "@metamask/eth-hd-keyring": "^4.0.2", - "browser-passworder": "MetaMask/browser-passworder#92d97d82ead553f135232a0f81de81e1a4a69782", + "browser-passworder": "MetaMask/browser-passworder#e37802b66135a9769177a7df237bf78cd99858ce", "eth-sig-util": "^3.0.1", "eth-simple-keyring": "^4.2.0", "obs-store": "^4.0.3" From a3214588072afc738ead6d461db28c9dffc5513e Mon Sep 17 00:00:00 2001 From: David Walsh Date: Tue, 1 Nov 2022 14:36:11 -0500 Subject: [PATCH 27/55] Make use of encryption key optional --- index.js | 79 ++++++++++++++++++++++---------------- test/index.js | 15 +++----- test/lib/mock-encryptor.js | 4 ++ 3 files changed, 56 insertions(+), 42 deletions(-) diff --git a/index.js b/index.js index 45e95494..b38ba53b 100644 --- a/index.js +++ b/index.js @@ -48,6 +48,10 @@ class KeyringController extends EventEmitter { this.encryptor = opts.encryptor || encryptor; this.keyrings = []; + + // This option allows the controller to cache an exported key + // for use in decrypting and encrypting data without password + this.cacheEncryptionKey = Boolean(opts.cacheEncryptionKey); } /** @@ -202,10 +206,7 @@ class KeyringController extends EventEmitter { * @returns {Promise} A Promise that resolves to the state. */ async submitEncryptionKey(encryptionKey) { - this.keyrings = await this.unlockKeyrings( - undefined, - encryptionKey - ); + this.keyrings = await this.unlockKeyrings(undefined, encryptionKey); this.setUnlocked(); this.fullUpdate(); } @@ -570,23 +571,27 @@ class KeyringController extends EventEmitter { let vault; let newExportedKeyString; - if (this.password) { - const { vault: newVault, exportedKeyString } = - await this.encryptor.encryptWithDetail( - this.password, + if (this.cacheEncryptionKey) { + if (this.password) { + const { vault: newVault, exportedKeyString } = + await this.encryptor.encryptWithDetail( + this.password, + serializedKeyrings, + ); + + vault = newVault; + newExportedKeyString = exportedKeyString; + } else if (encryptionKey) { + const key = await this.encryptor.createKeyFromString(encryptionKey); + const vaultJSON = await this.encryptor.encryptWithKey( + key, serializedKeyrings, ); - - vault = newVault; - newExportedKeyString = exportedKeyString; - } else if (encryptionKey) { - const key = await this.encryptor.createKeyFromString(encryptionKey); - const vaultJSON = await this.encryptor.encryptWithKey( - key, - serializedKeyrings, - ); - vaultJSON.salt = this.encryptionSalt; - vault = JSON.stringify(vaultJSON); + vaultJSON.salt = this.encryptionSalt; + vault = JSON.stringify(vaultJSON); + } + } else { + vault = await this.encryptor.encrypt(this.password, serializedKeyrings); } if (!vault) { @@ -622,22 +627,30 @@ class KeyringController extends EventEmitter { await this.clearKeyrings(); let vault; - if (password) { - const result = await this.encryptor.decryptWithDetail( - password, - encryptedVault, - ); - vault = result.vault; - this.encryptionSalt = result.salt; - this.password = password; - this.memStore.updateState({ encryptionKey: result.exportedKeyString }); - } else { - const key = await this.encryptor.createKeyFromString(encryptionKey); - vault = await this.encryptor.decryptWithKey(key, JSON.parse(encryptedVault)); + if (this.cacheEncryptionKey) { + if (password) { + const result = await this.encryptor.decryptWithDetail( + password, + encryptedVault, + ); + vault = result.vault; + this.encryptionSalt = result.salt; + this.password = password; + + this.memStore.updateState({ encryptionKey: result.exportedKeyString }); + } else { + const key = await this.encryptor.createKeyFromString(encryptionKey); + vault = await this.encryptor.decryptWithKey( + key, + JSON.parse(encryptedVault), + ); - this.encryptionSalt = JSON.parse(encryptedVault).salt; - this.memStore.updateState({ encryptionKey }); + this.encryptionSalt = JSON.parse(encryptedVault).salt; + this.memStore.updateState({ encryptionKey }); + } + } else { + vault = await this.encryptor.decrypt(password, encryptedVault); } await Promise.all(vault.map(this._restoreKeyring.bind(this))); diff --git a/test/index.js b/test/index.js index 5a9a0533..68f646dd 100644 --- a/test/index.js +++ b/test/index.js @@ -462,6 +462,7 @@ describe('KeyringController', function () { describe('submitEncryptionKey', function () { it('sets encryption key data upon submitPassword', async function () { + keyringController.cacheEncryptionKey = true; await keyringController.submitPassword(PASSWORD); expect(keyringController.password).toBe(PASSWORD); @@ -469,23 +470,19 @@ describe('KeyringController', function () { }); it('unlocks the keyrings with valid information', async function () { - const returnValue = - await keyringController.encryptor.decryptWithEncryptedKeyString(); - const stub = sinon.stub( - keyringController.encryptor, - 'decryptWithEncryptedKeyString', - ); + keyringController.cacheEncryptionKey = true; + const returnValue = await keyringController.encryptor.decryptWithKey(); + const stub = sinon.stub(keyringController.encryptor, 'decryptWithKey'); stub.resolves(Promise.resolve(returnValue)); keyringController.store.updateState({ vault: MOCK_ENCRYPTION_DATA }); await keyringController.submitEncryptionKey(MOCK_ENCRYPTION_KEY); - expect( - keyringController.encryptor.decryptWithEncryptedKeyString.calledOnce, - ).toBe(true); + expect(keyringController.encryptor.decryptWithKey.calledOnce).toBe(true); }); it('persists keyrings when actions are performed', async function () { + keyringController.cacheEncryptionKey = true; keyringController.store.updateState({ vault: MOCK_ENCRYPTION_DATA }); await keyringController.submitEncryptionKey(MOCK_ENCRYPTION_KEY); diff --git a/test/lib/mock-encryptor.js b/test/lib/mock-encryptor.js index 78331e2a..c1902873 100644 --- a/test/lib/mock-encryptor.js +++ b/test/lib/mock-encryptor.js @@ -45,6 +45,10 @@ module.exports = { return Promise.resolve(result); }, + createKeyFromString() { + return null; + }, + encryptWithKey(key, dataObj) { return this.encrypt(key, dataObj); }, From e5941700a091701f009e2021fe92eebbad8e908b Mon Sep 17 00:00:00 2001 From: David Walsh Date: Tue, 1 Nov 2022 19:02:30 -0500 Subject: [PATCH 28/55] Remove unnecessary password setting --- index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/index.js b/index.js index b38ba53b..d18f8724 100644 --- a/index.js +++ b/index.js @@ -188,8 +188,6 @@ class KeyringController extends EventEmitter { await this.verifyPassword(password); this.keyrings = await this.unlockKeyrings(password); - this.password = password; - this.setUnlocked(); this.fullUpdate(); } @@ -647,6 +645,8 @@ class KeyringController extends EventEmitter { ); this.encryptionSalt = JSON.parse(encryptedVault).salt; + // This call is required on the first call because encryptionKey + // is not yet inside the memStore this.memStore.updateState({ encryptionKey }); } } else { From 1093c170f6ab38c9bc623cc2feba3271022987e0 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Thu, 3 Nov 2022 08:55:17 -0500 Subject: [PATCH 29/55] Change to importKey --- index.js | 4 ++-- test/lib/mock-encryptor.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/index.js b/index.js index d18f8724..217f02f6 100644 --- a/index.js +++ b/index.js @@ -580,7 +580,7 @@ class KeyringController extends EventEmitter { vault = newVault; newExportedKeyString = exportedKeyString; } else if (encryptionKey) { - const key = await this.encryptor.createKeyFromString(encryptionKey); + const key = await this.encryptor.importKey(encryptionKey); const vaultJSON = await this.encryptor.encryptWithKey( key, serializedKeyrings, @@ -638,7 +638,7 @@ class KeyringController extends EventEmitter { this.memStore.updateState({ encryptionKey: result.exportedKeyString }); } else { - const key = await this.encryptor.createKeyFromString(encryptionKey); + const key = await this.encryptor.importKey(encryptionKey); vault = await this.encryptor.decryptWithKey( key, JSON.parse(encryptedVault), diff --git a/test/lib/mock-encryptor.js b/test/lib/mock-encryptor.js index c1902873..fb43c582 100644 --- a/test/lib/mock-encryptor.js +++ b/test/lib/mock-encryptor.js @@ -45,7 +45,7 @@ module.exports = { return Promise.resolve(result); }, - createKeyFromString() { + importKey() { return null; }, From 76c0123b7c5b8906002cbe97c8728a5122f60ae0 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Thu, 3 Nov 2022 11:33:54 -0500 Subject: [PATCH 30/55] Fix tests --- index.js | 5 +++++ test/index.js | 16 ++++++++++++++-- test/lib/mock-encryptor.js | 14 ++++++++++++-- 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/index.js b/index.js index 217f02f6..2ef6d638 100644 --- a/index.js +++ b/index.js @@ -204,6 +204,10 @@ class KeyringController extends EventEmitter { * @returns {Promise} A Promise that resolves to the state. */ async submitEncryptionKey(encryptionKey) { + // If a user is trying to submit an encrypted key, we should lock + // in the event that the encrypted key is invalid + this.setLocked(); + this.keyrings = await this.unlockKeyrings(undefined, encryptionKey); this.setUnlocked(); this.fullUpdate(); @@ -651,6 +655,7 @@ class KeyringController extends EventEmitter { } } else { vault = await this.encryptor.decrypt(password, encryptedVault); + this.password = password; } await Promise.all(vault.map(this._restoreKeyring.bind(this))); diff --git a/test/index.js b/test/index.js index 68f646dd..c51d2fd8 100644 --- a/test/index.js +++ b/test/index.js @@ -69,7 +69,7 @@ describe('KeyringController', function () { }); describe('submitPassword', function () { - it('should not loads keyrings when incorrect password', async function () { + it('should not load keyrings when incorrect password', async function () { await keyringController.createNewVaultAndKeychain(PASSWORD); await keyringController.persistAllKeyrings(); expect(keyringController.keyrings).toHaveLength(1); @@ -121,8 +121,8 @@ describe('KeyringController', function () { it('clears old keyrings and creates a one', async function () { const initialAccounts = await keyringController.getAccounts(); expect(initialAccounts).toHaveLength(1); - await keyringController.addNewKeyring('HD Key Tree'); + await keyringController.addNewKeyring('HD Key Tree'); const allAccounts = await keyringController.getAccounts(); expect(allAccounts).toHaveLength(2); @@ -479,6 +479,18 @@ describe('KeyringController', function () { await keyringController.submitEncryptionKey(MOCK_ENCRYPTION_KEY); expect(keyringController.encryptor.decryptWithKey.calledOnce).toBe(true); + expect(keyringController.keyrings).toHaveLength(1); + }); + + it('should not load keyrings when incorrect encryptionKey', async function () { + keyringController.cacheEncryptionKey = true; + keyringController.setLocked(true); + + await expect(keyringController.submitEncryptionKey(`{}`)).rejects.toThrow( + `Failed to execute 'importKey' on 'SubtleCrypto': The provided value is not of type '(ArrayBuffer or ArrayBufferView or JsonWebKey)'.`, + ); + expect(keyringController.password).toBeUndefined(); + expect(keyringController.keyrings).toHaveLength(0); }); it('persists keyrings when actions are performed', async function () { diff --git a/test/lib/mock-encryptor.js b/test/lib/mock-encryptor.js index fb43c582..f44388c4 100644 --- a/test/lib/mock-encryptor.js +++ b/test/lib/mock-encryptor.js @@ -3,6 +3,7 @@ const sinon = require('sinon'); const PASSWORD = 'password123'; const MOCK_ENCRYPTION_KEY = '{"alg":"A256GCM","ext":true,"k":"wYmxkxOOFBDP6F6VuuYFcRt_Po-tSLFHCWVolsHs4VI","key_ops":["encrypt","decrypt"],"kty":"oct"}'; +const INVALID_PASSWORD_ERROR = 'Incorrect password.'; const MOCK_HEX = '0xabcdef0123456789'; const MOCK_KEY = Buffer.alloc(32); @@ -22,6 +23,10 @@ module.exports = { }), async decrypt(_password, _text) { + if (_password && _password !== PASSWORD) { + throw new Error(INVALID_PASSWORD_ERROR); + } + return Promise.resolve(cacheVal || {}); }, @@ -32,7 +37,7 @@ module.exports = { decryptWithDetail(_password, _text) { if (_password && _password !== PASSWORD) { - throw new Error('Incorrect password.'); + throw new Error(INVALID_PASSWORD_ERROR); } const result = cacheVal @@ -45,7 +50,12 @@ module.exports = { return Promise.resolve(result); }, - importKey() { + importKey(keyString) { + if (keyString === '{}') { + throw new TypeError( + `Failed to execute 'importKey' on 'SubtleCrypto': The provided value is not of type '(ArrayBuffer or ArrayBufferView or JsonWebKey)'.`, + ); + } return null; }, From 72145ba392a175c326d5550cf115cab9233e0f02 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Thu, 3 Nov 2022 16:24:29 -0500 Subject: [PATCH 31/55] Update to latest browser-passworder --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 1af082e5..8a2a0d96 100644 --- a/package.json +++ b/package.json @@ -33,7 +33,7 @@ "dependencies": { "@metamask/bip39": "^4.0.0", "@metamask/eth-hd-keyring": "^4.0.2", - "browser-passworder": "MetaMask/browser-passworder#e37802b66135a9769177a7df237bf78cd99858ce", + "browser-passworder": "MetaMask/browser-passworder#e66f3ca0b57f4ef9de4acd782e92b850cb0fe6bb", "eth-sig-util": "^3.0.1", "eth-simple-keyring": "^4.2.0", "obs-store": "^4.0.3" From aedfbe1247e243f72122a620ea925afce9a290ac Mon Sep 17 00:00:00 2001 From: David Walsh Date: Fri, 4 Nov 2022 16:54:35 -0500 Subject: [PATCH 32/55] Add workable dist for testing extension --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 8a2a0d96..4b1a348a 100644 --- a/package.json +++ b/package.json @@ -33,7 +33,7 @@ "dependencies": { "@metamask/bip39": "^4.0.0", "@metamask/eth-hd-keyring": "^4.0.2", - "browser-passworder": "MetaMask/browser-passworder#e66f3ca0b57f4ef9de4acd782e92b850cb0fe6bb", + "browser-passworder": "darkwing/browser-passworder#mv3-login-dist", "eth-sig-util": "^3.0.1", "eth-simple-keyring": "^4.2.0", "obs-store": "^4.0.3" From 46f39dbc4082577d0b1e1146f9725a4ca8cd509a Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 7 Nov 2022 10:11:59 -0600 Subject: [PATCH 33/55] Add comment explaining memory store update --- index.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/index.js b/index.js index 2ef6d638..9601c63c 100644 --- a/index.js +++ b/index.js @@ -602,7 +602,10 @@ class KeyringController extends EventEmitter { this.store.updateState({ vault }); - // The keyring updates need to be announced before updating the exportedKeyStrings + // The keyring updates need to be announced before updating the encryptionKey + // so that the updated keyring gets propagated to the extension first. + // Not calling _updateMemStoreKeyrings results in the wrong account being selected + // in the extension. await this._updateMemStoreKeyrings(); if (newExportedKeyString) { this.memStore.updateState({ encryptionKey: newExportedKeyString }); From 76aa90d1456bf1a19c12c12f425ad6742a958bd2 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 7 Nov 2022 10:18:21 -0600 Subject: [PATCH 34/55] Update to v4.0.1 of browser-passworder --- index.js | 2 +- package.json | 2 +- yarn.lock | 17 +++++------------ 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/index.js b/index.js index 9601c63c..ac1d779a 100644 --- a/index.js +++ b/index.js @@ -2,7 +2,7 @@ const { EventEmitter } = require('events'); const { Buffer } = require('buffer'); const bip39 = require('@metamask/bip39'); const ObservableStore = require('obs-store'); -const encryptor = require('browser-passworder'); +const encryptor = require('@metamask/browser-passworder'); const { normalize: normalizeAddress } = require('eth-sig-util'); const SimpleKeyring = require('eth-simple-keyring'); diff --git a/package.json b/package.json index 4b1a348a..90720e62 100644 --- a/package.json +++ b/package.json @@ -33,7 +33,7 @@ "dependencies": { "@metamask/bip39": "^4.0.0", "@metamask/eth-hd-keyring": "^4.0.2", - "browser-passworder": "darkwing/browser-passworder#mv3-login-dist", + "@metamask/browser-passworder": "^4.0.1", "eth-sig-util": "^3.0.1", "eth-simple-keyring": "^4.2.0", "obs-store": "^4.0.3" diff --git a/yarn.lock b/yarn.lock index 8d6fd5be..3382f252 100644 --- a/yarn.lock +++ b/yarn.lock @@ -547,6 +547,11 @@ pbkdf2 "^3.0.9" randombytes "^2.0.1" +"@metamask/browser-passworder@^4.0.1": + version "4.0.1" + resolved "https://registry.yarnpkg.com/@metamask/browser-passworder/-/browser-passworder-4.0.1.tgz#bb2613bb1886774e18da4107ab94a21042e5e3da" + integrity sha512-mtekzCKph/S/15hRfWFDIUrpvz9mNoIU0CmH0SOlTHpLhalonEsZ+56MbQQUDshXbytzHp1eKr29OHrIx0u9iQ== + "@metamask/eslint-config-jest@^7.0.0": version "7.0.0" resolved "https://registry.yarnpkg.com/@metamask/eslint-config-jest/-/eslint-config-jest-7.0.0.tgz#81612aaf5307c3d65bb43366000233cd0b6e9db4" @@ -1204,13 +1209,6 @@ brorand@^1.1.0: resolved "https://registry.yarnpkg.com/brorand/-/brorand-1.1.0.tgz#12c25efe40a45e3c323eb8675a0a0ce57b22371f" integrity sha1-EsJe/kCkXjwyPrhnWgoM5XsiNx8= -browser-passworder@^2.0.3: - version "2.0.3" - resolved "https://registry.yarnpkg.com/browser-passworder/-/browser-passworder-2.0.3.tgz#6fdd2082e516a176edbcb3dcee0b7f9fce4f7917" - integrity sha1-b90gguUWoXbtvLPc7gt/n85PeRc= - dependencies: - browserify-unibabel "^3.0.0" - browser-process-hrtime@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/browser-process-hrtime/-/browser-process-hrtime-1.0.0.tgz#3c9b4b7d782c8121e56f10106d84c0d0ffc94626" @@ -1228,11 +1226,6 @@ browserify-aes@^1.0.6, browserify-aes@^1.2.0: inherits "^2.0.1" safe-buffer "^5.0.1" -browserify-unibabel@^3.0.0: - version "3.0.0" - resolved "https://registry.yarnpkg.com/browserify-unibabel/-/browserify-unibabel-3.0.0.tgz#5a6b8f0f704ce388d3927df47337e25830f71dda" - integrity sha1-WmuPD3BM44jTkn30czfiWDD3Hdo= - browserslist@^4.16.6: version "4.16.6" resolved "https://registry.yarnpkg.com/browserslist/-/browserslist-4.16.6.tgz#d7901277a5a88e554ed305b183ec9b0c08f66fa2" From 5ef3bb681567bc616f69d6c9a70d10bca1469186 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 7 Nov 2022 10:29:28 -0600 Subject: [PATCH 35/55] Lint fix --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 90720e62..2f1da1a6 100644 --- a/package.json +++ b/package.json @@ -32,8 +32,8 @@ }, "dependencies": { "@metamask/bip39": "^4.0.0", - "@metamask/eth-hd-keyring": "^4.0.2", "@metamask/browser-passworder": "^4.0.1", + "@metamask/eth-hd-keyring": "^4.0.2", "eth-sig-util": "^3.0.1", "eth-simple-keyring": "^4.2.0", "obs-store": "^4.0.3" From 3694ce55a0d1f21576ca5069ec6086bd877b664a Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 7 Nov 2022 13:46:31 -0600 Subject: [PATCH 36/55] Remove unnecessary call to persistAllKeyrings --- index.js | 1 - 1 file changed, 1 deletion(-) diff --git a/index.js b/index.js index ac1d779a..01d18a90 100644 --- a/index.js +++ b/index.js @@ -128,7 +128,6 @@ class KeyringController extends EventEmitter { this.clearKeyrings(); - await this.persistAllKeyrings(); const firstKeyring = await this.addNewKeyring( KEYRINGS_TYPE_MAP.HD_KEYRING, { From 4769f379ca929b7c57a8d407baf2be2586e6cce5 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 7 Nov 2022 14:40:57 -0600 Subject: [PATCH 37/55] Don't setLocked from submitPassword --- index.js | 8 -------- test/index.js | 2 ++ 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/index.js b/index.js index 01d18a90..7e19c6a5 100644 --- a/index.js +++ b/index.js @@ -180,10 +180,6 @@ class KeyringController extends EventEmitter { * @returns {Promise} A Promise that resolves to the state. */ async submitPassword(password) { - // If a user is trying to submit a password, we should lock - // in the event that the password is incorrect - this.setLocked(); - await this.verifyPassword(password); this.keyrings = await this.unlockKeyrings(password); @@ -203,10 +199,6 @@ class KeyringController extends EventEmitter { * @returns {Promise} A Promise that resolves to the state. */ async submitEncryptionKey(encryptionKey) { - // If a user is trying to submit an encrypted key, we should lock - // in the event that the encrypted key is invalid - this.setLocked(); - this.keyrings = await this.unlockKeyrings(undefined, encryptionKey); this.setUnlocked(); this.fullUpdate(); diff --git a/test/index.js b/test/index.js index c51d2fd8..0515a3ef 100644 --- a/test/index.js +++ b/test/index.js @@ -74,6 +74,8 @@ describe('KeyringController', function () { await keyringController.persistAllKeyrings(); expect(keyringController.keyrings).toHaveLength(1); + await keyringController.setLocked(); + await expect( keyringController.submitPassword(`${PASSWORD}a`), ).rejects.toThrow('Incorrect password.'); From a4e01d0706b53b169fcefaf668d6e724026d661a Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 7 Nov 2022 14:59:20 -0600 Subject: [PATCH 38/55] Revert unnecessary variable name changes --- test/index.js | 62 +++++++++++++++++++++++++-------------------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/test/index.js b/test/index.js index 0515a3ef..c5421818 100644 --- a/test/index.js +++ b/test/index.js @@ -8,21 +8,21 @@ const Wallet = require('ethereumjs-wallet').default; const KeyringController = require('..'); const mockEncryptor = require('./lib/mock-encryptor'); -const PASSWORD = 'password123'; +const password = 'password123'; const MOCK_ENCRYPTION_KEY = '{"alg":"A256GCM","ext":true,"k":"wYmxkxOOFBDP6F6VuuYFcRt_Po-tSLFHCWVolsHs4VI","key_ops":["encrypt","decrypt"],"kty":"oct"}'; const MOCK_ENCRYPTION_DATA = '{"data":"2fOOPRKClNrisB+tmqIcETyZvDuL2iIR1Hr1nO7XZHyMqVY1cDBetw2gY5C+cIo1qkpyv3bPp+4buUjp38VBsjbijM0F/FLOqWbcuKM9h9X0uwxsgsZ96uwcIf5I46NiMgoFlhppTTMZT0Nkocz+SnvHM0IgLsFan7JqBU++vSJvx2M1PDljZSunOsqyyL+DKmbYmM4umbouKV42dipUwrCvrQJmpiUZrSkpMJrPJk9ufDQO4CyIVo0qry3aNRdYFJ6rgSyq/k6rXMwGExCMHn8UlhNnAMuMKWPWR/ymK1bzNcNs4VU14iVjEXOZGPvD9cvqVe/VtcnIba6axNEEB4HWDOCdrDh5YNWwMlQVL7vSB2yOhPZByGhnEOloYsj2E5KEb9jFGskt7EKDEYNofr6t83G0c+B72VGYZeCvgtzXzgPwzIbhTtKkP+gdBmt2JNSYrTjLypT0q+v4C9BN1xWTxPmX6TTt0NzkI9pJxgN1VQAfSU9CyWTVpd4CBkgom2cSBsxZ2MNbdKF+qSWz3fQcmJ55hxM0EGJSt9+8eQOTuoJlBapRk4wdZKHR2jdKzPjSF2MAmyVD2kU51IKa/cVsckRFEes+m7dKyHRvlNwgT78W9tBDdZb5PSlfbZXnv8z5q1KtAj2lM2ogJ7brHBdevl4FISdTkObpwcUMcvACOOO0dj6CSYjSKr0ZJ2RLChVruZyPDxEhKGb/8Kv8trLOR3mck/et6d050/NugezycNk4nnzu5iP90gPbSzaqdZI=","iv":"qTGO1afGv3waHN9KoW34Eg==","salt":"HQ5sfhsb8XAQRJtD+UqcImT7Ve4n3YMagrh05YTOsjk="}'; -const WALLET_ONE_SEED_WORDS = +const walletOneSeedWords = 'puzzle seed penalty soldier say clay field arctic metal hen cage runway'; -const WALLET_ONE_ADDRESSES = ['0xef35ca8ebb9669a35c31b5f6f249a9941a812ac1']; +const walletOneAddresses = ['0xef35ca8ebb9669a35c31b5f6f249a9941a812ac1']; -const WALLET_TWO_SEED_WORDS = +const walletTwoSeedWords = 'urge letter protect palace city barely security section midnight wealth south deer'; -const WALLET_TWO_ADDRESSES = [ +const walletTwoAddresses = [ '0xbbafcf3d00fb625b65bb1497c94bf42c1f4b3e78', '0x49dd2653f38f75d40fdbd51e83b9c9724c87f7eb', ]; @@ -35,8 +35,8 @@ describe('KeyringController', function () { encryptor: mockEncryptor, }); - await keyringController.createNewVaultAndKeychain(PASSWORD); - await keyringController.submitPassword(PASSWORD); + await keyringController.createNewVaultAndKeychain(password); + await keyringController.submitPassword(password); }); afterEach(function () { @@ -70,14 +70,14 @@ describe('KeyringController', function () { describe('submitPassword', function () { it('should not load keyrings when incorrect password', async function () { - await keyringController.createNewVaultAndKeychain(PASSWORD); + await keyringController.createNewVaultAndKeychain(password); await keyringController.persistAllKeyrings(); expect(keyringController.keyrings).toHaveLength(1); await keyringController.setLocked(); await expect( - keyringController.submitPassword(`${PASSWORD}a`), + keyringController.submitPassword(`${password}a`), ).rejects.toThrow('Incorrect password.'); expect(keyringController.password).toBeUndefined(); expect(keyringController.keyrings).toHaveLength(0); @@ -89,7 +89,7 @@ describe('KeyringController', function () { const spy = sinon.spy(); keyringController.on('unlock', spy); - await keyringController.submitPassword(PASSWORD); + await keyringController.submitPassword(password); expect(spy.calledOnce).toBe(true); }); }); @@ -99,7 +99,7 @@ describe('KeyringController', function () { keyringController.store.updateState({ vault: null }); assert(!keyringController.store.getState().vault, 'no previous vault'); - await keyringController.createNewVaultAndKeychain(PASSWORD); + await keyringController.createNewVaultAndKeychain(password); const { vault } = keyringController.store.getState(); // eslint-disable-next-line jest/no-restricted-matchers expect(vault).toBeTruthy(); @@ -109,12 +109,12 @@ describe('KeyringController', function () { keyringController.store.updateState({ vault: null }); assert(!keyringController.store.getState().vault, 'no previous vault'); - await keyringController.createNewVaultAndKeychain(PASSWORD); + await keyringController.createNewVaultAndKeychain(password); const { vault } = keyringController.store.getState(); // eslint-disable-next-line jest/no-restricted-matchers expect(vault).toBeTruthy(); keyringController.encryptor.encrypt.args.forEach(([actualPassword]) => { - expect(actualPassword).toBe(PASSWORD); + expect(actualPassword).toBe(password); }); }); }); @@ -129,25 +129,25 @@ describe('KeyringController', function () { expect(allAccounts).toHaveLength(2); await keyringController.createNewVaultAndRestore( - PASSWORD, - WALLET_ONE_SEED_WORDS, + password, + walletOneSeedWords, ); const allAccountsAfter = await keyringController.getAccounts(); expect(allAccountsAfter).toHaveLength(1); - expect(allAccountsAfter[0]).toBe(WALLET_ONE_ADDRESSES[0]); + expect(allAccountsAfter[0]).toBe(walletOneAddresses[0]); }); it('throws error if argument password is not a string', async function () { await expect(() => - keyringController.createNewVaultAndRestore(12, WALLET_TWO_SEED_WORDS), + keyringController.createNewVaultAndRestore(12, walletTwoSeedWords), ).rejects.toThrow('Password must be text.'); }); it('throws error if mnemonic passed is invalid', async function () { await expect(() => keyringController.createNewVaultAndRestore( - PASSWORD, + password, 'test test test palace city barely security section midnight wealth south deer', ), ).rejects.toThrow('Seed phrase is invalid.'); @@ -155,19 +155,19 @@ describe('KeyringController', function () { it('accepts mnemonic passed as type array of numbers', async function () { const allAccountsBefore = await keyringController.getAccounts(); - expect(allAccountsBefore[0]).not.toBe(WALLET_TWO_ADDRESSES[0]); + expect(allAccountsBefore[0]).not.toBe(walletTwoAddresses[0]); const mnemonicAsArrayOfNumbers = Array.from( - Buffer.from(WALLET_TWO_SEED_WORDS).values(), + Buffer.from(walletTwoSeedWords).values(), ); await keyringController.createNewVaultAndRestore( - PASSWORD, + password, mnemonicAsArrayOfNumbers, ); const allAccountsAfter = await keyringController.getAccounts(); expect(allAccountsAfter).toHaveLength(1); - expect(allAccountsAfter[0]).toBe(WALLET_TWO_ADDRESSES[0]); + expect(allAccountsAfter[0]).toBe(walletTwoAddresses[0]); }); }); @@ -207,12 +207,12 @@ describe('KeyringController', function () { expect(previousAllAccounts).toHaveLength(1); const keyring = await keyringController.addNewKeyring('HD Key Tree', { numberOfAccounts: 2, - mnemonic: WALLET_TWO_SEED_WORDS, + mnemonic: walletTwoSeedWords, }); const keyringAccounts = await keyring.getAccounts(); expect(keyringAccounts).toHaveLength(2); - expect(keyringAccounts[0]).toStrictEqual(WALLET_TWO_ADDRESSES[0]); - expect(keyringAccounts[1]).toStrictEqual(WALLET_TWO_ADDRESSES[1]); + expect(keyringAccounts[0]).toStrictEqual(walletTwoAddresses[0]); + expect(keyringAccounts[1]).toStrictEqual(walletTwoAddresses[1]); const allAccounts = await keyringController.getAccounts(); expect(allAccounts).toHaveLength(3); }); @@ -223,7 +223,7 @@ describe('KeyringController', function () { const mockSerialized = { type: 'HD Key Tree', data: { - mnemonic: WALLET_ONE_SEED_WORDS, + mnemonic: walletOneSeedWords, numberOfAccounts: 1, }, }; @@ -232,7 +232,7 @@ describe('KeyringController', function () { expect(keyring.wallets).toHaveLength(1); const accounts = await keyring.getAccounts(); - expect(accounts[0]).toBe(WALLET_ONE_ADDRESSES[0]); + expect(accounts[0]).toBe(walletOneAddresses[0]); }); }); @@ -313,7 +313,7 @@ describe('KeyringController', function () { describe('unlockKeyrings', function () { it('returns the list of keyrings', async function () { await keyringController.setLocked(); - const keyrings = await keyringController.unlockKeyrings(PASSWORD); + const keyrings = await keyringController.unlockKeyrings(password); expect(keyrings).toHaveLength(1); keyrings.forEach((keyring) => { expect(keyring.wallets).toHaveLength(1); @@ -465,9 +465,9 @@ describe('KeyringController', function () { describe('submitEncryptionKey', function () { it('sets encryption key data upon submitPassword', async function () { keyringController.cacheEncryptionKey = true; - await keyringController.submitPassword(PASSWORD); + await keyringController.submitPassword(password); - expect(keyringController.password).toBe(PASSWORD); + expect(keyringController.password).toBe(password); expect(keyringController.encryptionSalt).toBe('SALT'); }); @@ -529,7 +529,7 @@ describe('KeyringController', function () { }); it('cleans up login artifacts upon lock', async function () { - await keyringController.submitPassword(PASSWORD); + await keyringController.submitPassword(password); await keyringController.setLocked(); From 37af169fc7844ebeba5939a9f3b31968ecbdf3a9 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 7 Nov 2022 15:15:20 -0600 Subject: [PATCH 39/55] Restore _updateMemStoreKeyrings call to forgetDevice --- index.js | 1 + 1 file changed, 1 insertion(+) diff --git a/index.js b/index.js index 7e19c6a5..95fa701c 100644 --- a/index.js +++ b/index.js @@ -848,6 +848,7 @@ class KeyringController extends EventEmitter { if (keyring.forgetDevice) { keyring.forgetDevice(); this.persistAllKeyrings.bind(this)(); + this._updateMemStoreKeyrings.bind(this)(); } else { throw new Error( `KeyringController - keyring does not have method "forgetDevice", keyring type: ${keyring.type}`, From 81cd50a6708cece844003515e792d00fb9a3cfd5 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 7 Nov 2022 17:35:35 -0600 Subject: [PATCH 40/55] Remove redundant verifyPassword call Co-authored-by: Mark Stacey --- index.js | 1 - 1 file changed, 1 deletion(-) diff --git a/index.js b/index.js index 0bd0820e..bc9fefa7 100644 --- a/index.js +++ b/index.js @@ -176,7 +176,6 @@ class KeyringController extends EventEmitter { * @returns {Promise} A Promise that resolves to the state. */ async submitPassword(password) { - await this.verifyPassword(password); this.keyrings = await this.unlockKeyrings(password); this.setUnlocked(); From 12d8365e6f110eee27616fbb823710e8ced94ec2 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 7 Nov 2022 17:36:03 -0600 Subject: [PATCH 41/55] Remove default value for `opts` with addNewKeyring Co-authored-by: Mark Stacey --- index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/index.js b/index.js index bc9fefa7..693f894c 100644 --- a/index.js +++ b/index.js @@ -228,7 +228,7 @@ class KeyringController extends EventEmitter { * @param {Object} opts - The constructor options for the keyring. * @returns {Promise} The new keyring. */ - async addNewKeyring(type, opts = {}) { + async addNewKeyring(type, opts) { const Keyring = this.getKeyringClassForType(type); const keyring = new Keyring(opts); if ((!opts || !opts.mnemonic) && type === KEYRINGS_TYPE_MAP.HD_KEYRING) { From a7a7bc7a4dcc4c187170a9007c722f663d13a602 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 7 Nov 2022 17:40:01 -0600 Subject: [PATCH 42/55] Remove unnecessary memstore keyring update --- index.js | 1 - 1 file changed, 1 deletion(-) diff --git a/index.js b/index.js index 693f894c..b50135c3 100644 --- a/index.js +++ b/index.js @@ -842,7 +842,6 @@ class KeyringController extends EventEmitter { if (keyring.forgetDevice) { keyring.forgetDevice(); this.persistAllKeyrings(); - this._updateMemStoreKeyrings.bind(this)(); } else { throw new Error( `KeyringController - keyring does not have method "forgetDevice", keyring type: ${keyring.type}`, From 2d4b51d7ae0748f5afc94c4f7512c2bb639ce8c4 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 7 Nov 2022 17:46:30 -0600 Subject: [PATCH 43/55] Avoid parsing the encryptedVault twice Co-authored-by: Mark Stacey --- index.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/index.js b/index.js index b50135c3..b75c3d94 100644 --- a/index.js +++ b/index.js @@ -631,12 +631,13 @@ class KeyringController extends EventEmitter { this.memStore.updateState({ encryptionKey: result.exportedKeyString }); } else { const key = await this.encryptor.importKey(encryptionKey); + const parsedEncryptedVault = JSON.parse(encryptedVault); vault = await this.encryptor.decryptWithKey( key, - JSON.parse(encryptedVault), + parsedEncryptedVault, ); - this.encryptionSalt = JSON.parse(encryptedVault).salt; + this.encryptionSalt = parsedEncryptedVault.salt; // This call is required on the first call because encryptionKey // is not yet inside the memStore this.memStore.updateState({ encryptionKey }); From 78412cb5e63daab52e53489cc4f389832de470fa Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 7 Nov 2022 19:17:18 -0600 Subject: [PATCH 44/55] Ensure encryptionKey is set, rename test set to cacheEncryptionKey --- test/index.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/index.js b/test/index.js index 28a4bd78..a0bba022 100644 --- a/test/index.js +++ b/test/index.js @@ -471,13 +471,15 @@ describe('KeyringController', function () { }); }); - describe('submitEncryptionKey', function () { + describe('cacheEncryptionKey', function () { it('sets encryption key data upon submitPassword', async function () { keyringController.cacheEncryptionKey = true; await keyringController.submitPassword(password); expect(keyringController.password).toBe(password); expect(keyringController.encryptionSalt).toBe('SALT'); + // eslint-disable-next-line jest/no-restricted-matchers + expect(keyringController.memStore.getState().encryptionKey).toBeTruthy(); }); it('unlocks the keyrings with valid information', async function () { From 9a0343e88fff0c8cf510f9b14ce724b14da17c10 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 7 Nov 2022 19:29:44 -0600 Subject: [PATCH 45/55] Add test coverage for attempting to persist without key or password --- index.js | 5 +---- test/index.js | 8 ++++++++ 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/index.js b/index.js index b75c3d94..101d3a43 100644 --- a/index.js +++ b/index.js @@ -632,10 +632,7 @@ class KeyringController extends EventEmitter { } else { const key = await this.encryptor.importKey(encryptionKey); const parsedEncryptedVault = JSON.parse(encryptedVault); - vault = await this.encryptor.decryptWithKey( - key, - parsedEncryptedVault, - ); + vault = await this.encryptor.decryptWithKey(key, parsedEncryptedVault); this.encryptionSalt = parsedEncryptedVault.salt; // This call is required on the first call because encryptionKey diff --git a/test/index.js b/test/index.js index a0bba022..b1bab246 100644 --- a/test/index.js +++ b/test/index.js @@ -539,8 +539,16 @@ describe('KeyringController', function () { expect(stub.callCount).toBe(4); }); + it('triggers an error when trying to persist without password or encryption key', async function () { + keyringController.password = undefined; + await expect(keyringController.persistAllKeyrings()).rejects.toThrow( + 'Cannot persist vault without password and encryption key', + ); + }); + it('cleans up login artifacts upon lock', async function () { await keyringController.submitPassword(password); + expect(keyringController.password).toBe(password); await keyringController.setLocked(); From 0071c0495581c052229a18795463862060103075 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 7 Nov 2022 19:33:38 -0600 Subject: [PATCH 46/55] Improve 'cleans up login artifacts upon lock' test to ensure encryption key is set --- test/index.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/index.js b/test/index.js index b1bab246..af9aafc8 100644 --- a/test/index.js +++ b/test/index.js @@ -547,8 +547,13 @@ describe('KeyringController', function () { }); it('cleans up login artifacts upon lock', async function () { + keyringController.cacheEncryptionKey = true; await keyringController.submitPassword(password); expect(keyringController.password).toBe(password); + // eslint-disable-next-line jest/no-restricted-matchers + expect(keyringController.encryptionSalt).toBeTruthy(); + // eslint-disable-next-line jest/no-restricted-matchers + expect(keyringController.memStore.getState().encryptionKey).toBeTruthy(); await keyringController.setLocked(); From 7907b346ecd9c3012620d86749456dd3fd68d007 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 7 Nov 2022 19:48:33 -0600 Subject: [PATCH 47/55] Lock keyringcontroller before submitting encryption key --- test/index.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/index.js b/test/index.js index af9aafc8..532138c8 100644 --- a/test/index.js +++ b/test/index.js @@ -489,6 +489,9 @@ describe('KeyringController', function () { stub.resolves(Promise.resolve(returnValue)); keyringController.store.updateState({ vault: MOCK_ENCRYPTION_DATA }); + + await keyringController.setLocked(); + await keyringController.submitEncryptionKey(MOCK_ENCRYPTION_KEY); expect(keyringController.encryptor.decryptWithKey.calledOnce).toBe(true); From 9178496ff1504a9c4ee5488e3c50f3044e811f1e Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 7 Nov 2022 20:11:22 -0600 Subject: [PATCH 48/55] Move encryptionSalt to memStore --- index.js | 23 +++++++++++++++-------- test/index.js | 6 +++--- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/index.js b/index.js index 101d3a43..fa26f711 100644 --- a/index.js +++ b/index.js @@ -149,11 +149,14 @@ class KeyringController extends EventEmitter { * @returns {Promise} A Promise that resolves to the state. */ async setLocked() { - delete this.encryptionSalt; delete this.password; // set locked - this.memStore.updateState({ isUnlocked: false, encryptionKey: null }); + this.memStore.updateState({ + isUnlocked: false, + encryptionKey: null, + encryptionSalt: null, + }); // remove keyrings this.keyrings = []; @@ -537,7 +540,7 @@ class KeyringController extends EventEmitter { * @returns {Promise} Resolves to true once keyrings are persisted. */ async persistAllKeyrings() { - const { encryptionKey } = this.memStore.getState(); + const { encryptionKey, encryptionSalt } = this.memStore.getState(); if (!this.password && !encryptionKey) { throw new Error( @@ -574,7 +577,7 @@ class KeyringController extends EventEmitter { key, serializedKeyrings, ); - vaultJSON.salt = this.encryptionSalt; + vaultJSON.salt = encryptionSalt; vault = JSON.stringify(vaultJSON); } } else { @@ -625,19 +628,23 @@ class KeyringController extends EventEmitter { encryptedVault, ); vault = result.vault; - this.encryptionSalt = result.salt; this.password = password; - this.memStore.updateState({ encryptionKey: result.exportedKeyString }); + this.memStore.updateState({ + encryptionKey: result.exportedKeyString, + encryptionSalt: result.salt, + }); } else { const key = await this.encryptor.importKey(encryptionKey); const parsedEncryptedVault = JSON.parse(encryptedVault); vault = await this.encryptor.decryptWithKey(key, parsedEncryptedVault); - this.encryptionSalt = parsedEncryptedVault.salt; // This call is required on the first call because encryptionKey // is not yet inside the memStore - this.memStore.updateState({ encryptionKey }); + this.memStore.updateState({ + encryptionKey, + encryptionSalt: parsedEncryptedVault.salt, + }); } } else { vault = await this.encryptor.decrypt(password, encryptedVault); diff --git a/test/index.js b/test/index.js index 532138c8..2e7c196b 100644 --- a/test/index.js +++ b/test/index.js @@ -477,7 +477,7 @@ describe('KeyringController', function () { await keyringController.submitPassword(password); expect(keyringController.password).toBe(password); - expect(keyringController.encryptionSalt).toBe('SALT'); + expect(keyringController.memStore.getState().encryptionSalt).toBe('SALT'); // eslint-disable-next-line jest/no-restricted-matchers expect(keyringController.memStore.getState().encryptionKey).toBeTruthy(); }); @@ -554,13 +554,13 @@ describe('KeyringController', function () { await keyringController.submitPassword(password); expect(keyringController.password).toBe(password); // eslint-disable-next-line jest/no-restricted-matchers - expect(keyringController.encryptionSalt).toBeTruthy(); + expect(keyringController.memStore.getState().encryptionSalt).toBeTruthy(); // eslint-disable-next-line jest/no-restricted-matchers expect(keyringController.memStore.getState().encryptionKey).toBeTruthy(); await keyringController.setLocked(); - expect(keyringController.encryptionSalt).toBeUndefined(); + expect(keyringController.memStore.getState().encryptionSalt).toBeNull(); expect(keyringController.password).toBeUndefined(); expect(keyringController.memStore.getState().encryptionKey).toBeNull(); }); From 88299effbd6131650f663380e46248f24a37550c Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 7 Nov 2022 20:24:08 -0600 Subject: [PATCH 49/55] Rename newExportedKey to newEncryptionKey --- index.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/index.js b/index.js index fa26f711..92fff9e1 100644 --- a/index.js +++ b/index.js @@ -559,7 +559,7 @@ class KeyringController extends EventEmitter { ); let vault; - let newExportedKeyString; + let newEncryptionKey; if (this.cacheEncryptionKey) { if (this.password) { @@ -570,7 +570,7 @@ class KeyringController extends EventEmitter { ); vault = newVault; - newExportedKeyString = exportedKeyString; + newEncryptionKey = exportedKeyString; } else if (encryptionKey) { const key = await this.encryptor.importKey(encryptionKey); const vaultJSON = await this.encryptor.encryptWithKey( @@ -595,8 +595,8 @@ class KeyringController extends EventEmitter { // Not calling _updateMemStoreKeyrings results in the wrong account being selected // in the extension. await this._updateMemStoreKeyrings(); - if (newExportedKeyString) { - this.memStore.updateState({ encryptionKey: newExportedKeyString }); + if (newEncryptionKey) { + this.memStore.updateState({ encryptionKey: newEncryptionKey }); } return true; From 3848e04ceadd8c4e29ed0492d336ae4c5434b233 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Tue, 8 Nov 2022 11:28:16 -0600 Subject: [PATCH 50/55] Validate freshness of encryption key by validating salt --- index.js | 23 +++++++++++++++++------ test/index.js | 36 ++++++++++++++++++++++++++++++------ 2 files changed, 47 insertions(+), 12 deletions(-) diff --git a/index.js b/index.js index 92fff9e1..55caa154 100644 --- a/index.js +++ b/index.js @@ -193,11 +193,15 @@ class KeyringController extends EventEmitter { * * @emits KeyringController#unlock * @param {string} encryptionKey - The encrypted key information used to decrypt the vault - * @param {string} loginData - The vault data to decrypt + * @param {string} encryptionSalt - The salt used to generate the last key * @returns {Promise} A Promise that resolves to the state. */ - async submitEncryptionKey(encryptionKey) { - this.keyrings = await this.unlockKeyrings(undefined, encryptionKey); + async submitEncryptionKey(encryptionKey, encryptionSalt) { + this.keyrings = await this.unlockKeyrings( + undefined, + encryptionKey, + encryptionSalt, + ); this.setUnlocked(); this.fullUpdate(); } @@ -609,9 +613,11 @@ class KeyringController extends EventEmitter { * initializing the persisted keyrings to RAM. * * @param {string} password - The keyring controller password. + * @param {string} encryptionKey - An exported key string to unlock keyrings with + * @param {string} encryptionSalt - The salt used to encrypt the vault * @returns {Promise>} The keyrings. */ - async unlockKeyrings(password, encryptionKey) { + async unlockKeyrings(password, encryptionKey, encryptionSalt) { const encryptedVault = this.store.getState().vault; if (!encryptedVault) { throw new Error('Cannot unlock without a previous vault.'); @@ -635,15 +641,20 @@ class KeyringController extends EventEmitter { encryptionSalt: result.salt, }); } else { - const key = await this.encryptor.importKey(encryptionKey); const parsedEncryptedVault = JSON.parse(encryptedVault); + + if (encryptionSalt !== parsedEncryptedVault.salt) { + throw new Error('Encryption key and salt provided are expired'); + } + + const key = await this.encryptor.importKey(encryptionKey); vault = await this.encryptor.decryptWithKey(key, parsedEncryptedVault); // This call is required on the first call because encryptionKey // is not yet inside the memStore this.memStore.updateState({ encryptionKey, - encryptionSalt: parsedEncryptedVault.salt, + encryptionSalt, }); } } else { diff --git a/test/index.js b/test/index.js index 2e7c196b..4d4989fa 100644 --- a/test/index.js +++ b/test/index.js @@ -12,8 +12,8 @@ const password = 'password123'; const MOCK_ENCRYPTION_KEY = '{"alg":"A256GCM","ext":true,"k":"wYmxkxOOFBDP6F6VuuYFcRt_Po-tSLFHCWVolsHs4VI","key_ops":["encrypt","decrypt"],"kty":"oct"}'; -const MOCK_ENCRYPTION_DATA = - '{"data":"2fOOPRKClNrisB+tmqIcETyZvDuL2iIR1Hr1nO7XZHyMqVY1cDBetw2gY5C+cIo1qkpyv3bPp+4buUjp38VBsjbijM0F/FLOqWbcuKM9h9X0uwxsgsZ96uwcIf5I46NiMgoFlhppTTMZT0Nkocz+SnvHM0IgLsFan7JqBU++vSJvx2M1PDljZSunOsqyyL+DKmbYmM4umbouKV42dipUwrCvrQJmpiUZrSkpMJrPJk9ufDQO4CyIVo0qry3aNRdYFJ6rgSyq/k6rXMwGExCMHn8UlhNnAMuMKWPWR/ymK1bzNcNs4VU14iVjEXOZGPvD9cvqVe/VtcnIba6axNEEB4HWDOCdrDh5YNWwMlQVL7vSB2yOhPZByGhnEOloYsj2E5KEb9jFGskt7EKDEYNofr6t83G0c+B72VGYZeCvgtzXzgPwzIbhTtKkP+gdBmt2JNSYrTjLypT0q+v4C9BN1xWTxPmX6TTt0NzkI9pJxgN1VQAfSU9CyWTVpd4CBkgom2cSBsxZ2MNbdKF+qSWz3fQcmJ55hxM0EGJSt9+8eQOTuoJlBapRk4wdZKHR2jdKzPjSF2MAmyVD2kU51IKa/cVsckRFEes+m7dKyHRvlNwgT78W9tBDdZb5PSlfbZXnv8z5q1KtAj2lM2ogJ7brHBdevl4FISdTkObpwcUMcvACOOO0dj6CSYjSKr0ZJ2RLChVruZyPDxEhKGb/8Kv8trLOR3mck/et6d050/NugezycNk4nnzu5iP90gPbSzaqdZI=","iv":"qTGO1afGv3waHN9KoW34Eg==","salt":"HQ5sfhsb8XAQRJtD+UqcImT7Ve4n3YMagrh05YTOsjk="}'; +const MOCK_ENCRYPTION_SALT = 'HQ5sfhsb8XAQRJtD+UqcImT7Ve4n3YMagrh05YTOsjk='; +const MOCK_ENCRYPTION_DATA = `{"data":"2fOOPRKClNrisB+tmqIcETyZvDuL2iIR1Hr1nO7XZHyMqVY1cDBetw2gY5C+cIo1qkpyv3bPp+4buUjp38VBsjbijM0F/FLOqWbcuKM9h9X0uwxsgsZ96uwcIf5I46NiMgoFlhppTTMZT0Nkocz+SnvHM0IgLsFan7JqBU++vSJvx2M1PDljZSunOsqyyL+DKmbYmM4umbouKV42dipUwrCvrQJmpiUZrSkpMJrPJk9ufDQO4CyIVo0qry3aNRdYFJ6rgSyq/k6rXMwGExCMHn8UlhNnAMuMKWPWR/ymK1bzNcNs4VU14iVjEXOZGPvD9cvqVe/VtcnIba6axNEEB4HWDOCdrDh5YNWwMlQVL7vSB2yOhPZByGhnEOloYsj2E5KEb9jFGskt7EKDEYNofr6t83G0c+B72VGYZeCvgtzXzgPwzIbhTtKkP+gdBmt2JNSYrTjLypT0q+v4C9BN1xWTxPmX6TTt0NzkI9pJxgN1VQAfSU9CyWTVpd4CBkgom2cSBsxZ2MNbdKF+qSWz3fQcmJ55hxM0EGJSt9+8eQOTuoJlBapRk4wdZKHR2jdKzPjSF2MAmyVD2kU51IKa/cVsckRFEes+m7dKyHRvlNwgT78W9tBDdZb5PSlfbZXnv8z5q1KtAj2lM2ogJ7brHBdevl4FISdTkObpwcUMcvACOOO0dj6CSYjSKr0ZJ2RLChVruZyPDxEhKGb/8Kv8trLOR3mck/et6d050/NugezycNk4nnzu5iP90gPbSzaqdZI=","iv":"qTGO1afGv3waHN9KoW34Eg==","salt":"${MOCK_ENCRYPTION_SALT}"}`; const walletOneSeedWords = 'puzzle seed penalty soldier say clay field arctic metal hen cage runway'; @@ -492,27 +492,51 @@ describe('KeyringController', function () { await keyringController.setLocked(); - await keyringController.submitEncryptionKey(MOCK_ENCRYPTION_KEY); + await keyringController.submitEncryptionKey( + MOCK_ENCRYPTION_KEY, + MOCK_ENCRYPTION_SALT, + ); expect(keyringController.encryptor.decryptWithKey.calledOnce).toBe(true); expect(keyringController.keyrings).toHaveLength(1); }); - it('should not load keyrings when incorrect encryptionKey', async function () { + it('should not load keyrings when invalid encryptionKey format', async function () { keyringController.cacheEncryptionKey = true; keyringController.setLocked(true); + keyringController.store.updateState({ vault: MOCK_ENCRYPTION_DATA }); - await expect(keyringController.submitEncryptionKey(`{}`)).rejects.toThrow( + await expect( + keyringController.submitEncryptionKey(`{}`, MOCK_ENCRYPTION_SALT), + ).rejects.toThrow( `Failed to execute 'importKey' on 'SubtleCrypto': The provided value is not of type '(ArrayBuffer or ArrayBufferView or JsonWebKey)'.`, ); expect(keyringController.password).toBeUndefined(); expect(keyringController.keyrings).toHaveLength(0); }); + it('should not load keyrings when encryptionKey is expired', async function () { + keyringController.cacheEncryptionKey = true; + keyringController.setLocked(true); + keyringController.store.updateState({ vault: MOCK_ENCRYPTION_DATA }); + + await expect( + keyringController.submitEncryptionKey( + MOCK_ENCRYPTION_KEY, + 'OUTDATED_SALT', + ), + ).rejects.toThrow('Encryption key and salt provided are expired'); + expect(keyringController.password).toBeUndefined(); + expect(keyringController.keyrings).toHaveLength(0); + }); + it('persists keyrings when actions are performed', async function () { keyringController.cacheEncryptionKey = true; keyringController.store.updateState({ vault: MOCK_ENCRYPTION_DATA }); - await keyringController.submitEncryptionKey(MOCK_ENCRYPTION_KEY); + await keyringController.submitEncryptionKey( + MOCK_ENCRYPTION_KEY, + MOCK_ENCRYPTION_SALT, + ); const stub = sinon.stub(keyringController, 'persistAllKeyrings'); stub.resolves(Promise.resolve(true)); From c137eae4fc5247a92f03fea79f12dbd63761ff17 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Tue, 8 Nov 2022 11:39:35 -0600 Subject: [PATCH 51/55] Use string equality matcher for expect --- test/index.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/index.js b/test/index.js index 4d4989fa..d786dda8 100644 --- a/test/index.js +++ b/test/index.js @@ -577,10 +577,12 @@ describe('KeyringController', function () { keyringController.cacheEncryptionKey = true; await keyringController.submitPassword(password); expect(keyringController.password).toBe(password); - // eslint-disable-next-line jest/no-restricted-matchers - expect(keyringController.memStore.getState().encryptionSalt).toBeTruthy(); - // eslint-disable-next-line jest/no-restricted-matchers - expect(keyringController.memStore.getState().encryptionKey).toBeTruthy(); + expect( + keyringController.memStore.getState().encryptionSalt, + ).toStrictEqual(expect.stringMatching('.+')); + expect(keyringController.memStore.getState().encryptionKey).toStrictEqual( + expect.stringMatching('.+'), + ); await keyringController.setLocked(); From 83e09732adcc9cbe9b722939469faea91a9e7a30 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Tue, 8 Nov 2022 13:00:39 -0600 Subject: [PATCH 52/55] Ensure the encryption key method is used in tests --- test/index.js | 16 +++++++--------- test/lib/mock-encryptor.js | 10 ++++++++-- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/test/index.js b/test/index.js index d786dda8..3dd62f8d 100644 --- a/test/index.js +++ b/test/index.js @@ -503,7 +503,7 @@ describe('KeyringController', function () { it('should not load keyrings when invalid encryptionKey format', async function () { keyringController.cacheEncryptionKey = true; - keyringController.setLocked(true); + keyringController.setLocked(); keyringController.store.updateState({ vault: MOCK_ENCRYPTION_DATA }); await expect( @@ -517,7 +517,7 @@ describe('KeyringController', function () { it('should not load keyrings when encryptionKey is expired', async function () { keyringController.cacheEncryptionKey = true; - keyringController.setLocked(true); + keyringController.setLocked(); keyringController.store.updateState({ vault: MOCK_ENCRYPTION_DATA }); await expect( @@ -531,6 +531,7 @@ describe('KeyringController', function () { }); it('persists keyrings when actions are performed', async function () { + keyringController.setLocked(); keyringController.cacheEncryptionKey = true; keyringController.store.updateState({ vault: MOCK_ENCRYPTION_DATA }); await keyringController.submitEncryptionKey( @@ -538,16 +539,13 @@ describe('KeyringController', function () { MOCK_ENCRYPTION_SALT, ); - const stub = sinon.stub(keyringController, 'persistAllKeyrings'); - stub.resolves(Promise.resolve(true)); - const [firstKeyring] = keyringController.keyrings; await keyringController.addNewAccount(firstKeyring); - expect(stub.callCount).toBe(1); + expect(await keyringController.getAccounts()).toHaveLength(2); await keyringController.addNewAccount(firstKeyring); - expect(stub.callCount).toBe(2); + expect(await keyringController.getAccounts()).toHaveLength(3); const account = { privateKey: @@ -559,11 +557,11 @@ describe('KeyringController', function () { await keyringController.addNewKeyring('Simple Key Pair', [ account.privateKey, ]); - expect(stub.callCount).toBe(3); + expect(await keyringController.getAccounts()).toHaveLength(4); // remove that account that we just added await keyringController.removeAccount(account.publicKey); - expect(stub.callCount).toBe(4); + expect(await keyringController.getAccounts()).toHaveLength(3); }); it('triggers an error when trying to persist without password or encryption key', async function () { diff --git a/test/lib/mock-encryptor.js b/test/lib/mock-encryptor.js index f44388c4..a319b39d 100644 --- a/test/lib/mock-encryptor.js +++ b/test/lib/mock-encryptor.js @@ -3,6 +3,9 @@ const sinon = require('sinon'); const PASSWORD = 'password123'; const MOCK_ENCRYPTION_KEY = '{"alg":"A256GCM","ext":true,"k":"wYmxkxOOFBDP6F6VuuYFcRt_Po-tSLFHCWVolsHs4VI","key_ops":["encrypt","decrypt"],"kty":"oct"}'; +const MOCK_ENCRYPTION_SALT = 'HQ5sfhsb8XAQRJtD+UqcImT7Ve4n3YMagrh05YTOsjk='; +const MOCK_ENCRYPTION_DATA = `{"data":"2fOOPRKClNrisB+tmqIcETyZvDuL2iIR1Hr1nO7XZHyMqVY1cDBetw2gY5C+cIo1qkpyv3bPp+4buUjp38VBsjbijM0F/FLOqWbcuKM9h9X0uwxsgsZ96uwcIf5I46NiMgoFlhppTTMZT0Nkocz+SnvHM0IgLsFan7JqBU++vSJvx2M1PDljZSunOsqyyL+DKmbYmM4umbouKV42dipUwrCvrQJmpiUZrSkpMJrPJk9ufDQO4CyIVo0qry3aNRdYFJ6rgSyq/k6rXMwGExCMHn8UlhNnAMuMKWPWR/ymK1bzNcNs4VU14iVjEXOZGPvD9cvqVe/VtcnIba6axNEEB4HWDOCdrDh5YNWwMlQVL7vSB2yOhPZByGhnEOloYsj2E5KEb9jFGskt7EKDEYNofr6t83G0c+B72VGYZeCvgtzXzgPwzIbhTtKkP+gdBmt2JNSYrTjLypT0q+v4C9BN1xWTxPmX6TTt0NzkI9pJxgN1VQAfSU9CyWTVpd4CBkgom2cSBsxZ2MNbdKF+qSWz3fQcmJ55hxM0EGJSt9+8eQOTuoJlBapRk4wdZKHR2jdKzPjSF2MAmyVD2kU51IKa/cVsckRFEes+m7dKyHRvlNwgT78W9tBDdZb5PSlfbZXnv8z5q1KtAj2lM2ogJ7brHBdevl4FISdTkObpwcUMcvACOOO0dj6CSYjSKr0ZJ2RLChVruZyPDxEhKGb/8Kv8trLOR3mck/et6d050/NugezycNk4nnzu5iP90gPbSzaqdZI=","iv":"qTGO1afGv3waHN9KoW34Eg==","salt":"${MOCK_ENCRYPTION_SALT}"}`; + const INVALID_PASSWORD_ERROR = 'Incorrect password.'; const MOCK_HEX = '0xabcdef0123456789'; @@ -59,8 +62,11 @@ module.exports = { return null; }, - encryptWithKey(key, dataObj) { - return this.encrypt(key, dataObj); + encryptWithKey() { + const data = JSON.parse(MOCK_ENCRYPTION_DATA); + // Salt is not provided from this method + delete data.salt; + return data; }, decryptWithKey(key, text) { From a5fe20768b0ecde04b4e1b77a4e39f9594d65383 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Tue, 8 Nov 2022 14:18:19 -0600 Subject: [PATCH 53/55] await setLocked --- test/index.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/index.js b/test/index.js index 3dd62f8d..387fffcd 100644 --- a/test/index.js +++ b/test/index.js @@ -503,7 +503,7 @@ describe('KeyringController', function () { it('should not load keyrings when invalid encryptionKey format', async function () { keyringController.cacheEncryptionKey = true; - keyringController.setLocked(); + await keyringController.setLocked(); keyringController.store.updateState({ vault: MOCK_ENCRYPTION_DATA }); await expect( @@ -517,7 +517,7 @@ describe('KeyringController', function () { it('should not load keyrings when encryptionKey is expired', async function () { keyringController.cacheEncryptionKey = true; - keyringController.setLocked(); + await keyringController.setLocked(); keyringController.store.updateState({ vault: MOCK_ENCRYPTION_DATA }); await expect( @@ -531,7 +531,7 @@ describe('KeyringController', function () { }); it('persists keyrings when actions are performed', async function () { - keyringController.setLocked(); + await keyringController.setLocked(); keyringController.cacheEncryptionKey = true; keyringController.store.updateState({ vault: MOCK_ENCRYPTION_DATA }); await keyringController.submitEncryptionKey( From aacd2a1abba07ae781b2eed390f9fc5c09490226 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Tue, 8 Nov 2022 14:19:19 -0600 Subject: [PATCH 54/55] Fix missing string matcher --- test/index.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/index.js b/test/index.js index 387fffcd..fd4bbae7 100644 --- a/test/index.js +++ b/test/index.js @@ -478,8 +478,9 @@ describe('KeyringController', function () { expect(keyringController.password).toBe(password); expect(keyringController.memStore.getState().encryptionSalt).toBe('SALT'); - // eslint-disable-next-line jest/no-restricted-matchers - expect(keyringController.memStore.getState().encryptionKey).toBeTruthy(); + expect(keyringController.memStore.getState().encryptionKey).toStrictEqual( + expect.stringMatching('.+'), + ); }); it('unlocks the keyrings with valid information', async function () { From ad5169d980266ed6a581d039dd013ae226d6c3b1 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Tue, 8 Nov 2022 14:38:59 -0600 Subject: [PATCH 55/55] Fix lint --- test/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/index.js b/test/index.js index fd4bbae7..bb0e2508 100644 --- a/test/index.js +++ b/test/index.js @@ -532,8 +532,8 @@ describe('KeyringController', function () { }); it('persists keyrings when actions are performed', async function () { - await keyringController.setLocked(); keyringController.cacheEncryptionKey = true; + await keyringController.setLocked(); keyringController.store.updateState({ vault: MOCK_ENCRYPTION_DATA }); await keyringController.submitEncryptionKey( MOCK_ENCRYPTION_KEY,