From 1880cda9b95f3274d56e1f4abc513d1d7ddd59c2 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 30 Nov 2016 19:34:17 -0800 Subject: [PATCH] Fix vault encrypting & unlocking bug This is only a bug in dev, but was committed yesterday. Sometimes the `encrypt` method was being passed values other than the password as the encryption key, leading to un-unlockable vaults. To find this, and avoid it for all time hereafter, I added several more steps to our oft-neglected integration test suite, which now fully initializes a vault, locks it, and unlocks it again, to make sure all of those steps definitely work always. --- app/scripts/keyring-controller.js | 16 ++++++++++++---- app/scripts/lib/config-manager.js | 5 +++-- test/integration/lib/encryptor-test.js | 4 ++++ test/integration/lib/first-time.js | 2 ++ ui/app/actions.js | 25 +++++++++++++++++++------ ui/app/reducers/app.js | 14 +++++++++++++- 6 files changed, 53 insertions(+), 13 deletions(-) diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index ac9409dbb..9cfc3af97 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -167,7 +167,7 @@ module.exports = class KeyringController extends EventEmitter { this.configManager.setSelectedAccount(hexAccount) return this.setupAccounts(accounts) }) - .then(this.persistAllKeyrings.bind(this)) + .then(this.persistAllKeyrings.bind(this, password)) .then(this.fullUpdate.bind(this)) } @@ -226,9 +226,11 @@ module.exports = class KeyringController extends EventEmitter { }) .then((keyrings) => { this.keyrings = keyrings - return this.setupAccounts() + return this.fullUpdate() + }) + .catch((reason) => { + return reason }) - .then(this.fullUpdate.bind(this)) } // Add New Keyring @@ -250,6 +252,7 @@ module.exports = class KeyringController extends EventEmitter { this.keyrings.push(keyring) return this.setupAccounts(accounts) }) + .then(() => { return this.password }) .then(this.persistAllKeyrings.bind(this)) .then(() => { return keyring @@ -692,6 +695,9 @@ module.exports = class KeyringController extends EventEmitter { // Takes an account address and an iterator representing // the current number of named accounts. getBalanceAndNickname (account) { + if (!account) { + throw new Error('Problem loading account.') + } const address = normalize(account) this.ethStore.addAccount(address) return this.createNickname(address) @@ -725,7 +731,9 @@ module.exports = class KeyringController extends EventEmitter { // encrypts that array with the provided `password`, // and persists that encrypted string to storage. persistAllKeyrings (password = this.password) { - this.password = password + if (typeof password === 'string') { + this.password = password + } return Promise.all(this.keyrings.map((keyring) => { return Promise.all([keyring.type, keyring.serialize()]) .then((serializedKeyringArray) => { diff --git a/app/scripts/lib/config-manager.js b/app/scripts/lib/config-manager.js index 8e63265d2..59cc2b63c 100644 --- a/app/scripts/lib/config-manager.js +++ b/app/scripts/lib/config-manager.js @@ -3,6 +3,7 @@ const MetamaskConfig = require('../config.js') const migrations = require('./migrations') const rp = require('request-promise') const ethUtil = require('ethereumjs-util') +const normalize = require('./sig-util').normalize const TESTNET_RPC = MetamaskConfig.network.testnet const MAINNET_RPC = MetamaskConfig.network.mainnet @@ -273,13 +274,13 @@ ConfigManager.prototype.getWalletNicknames = function () { } ConfigManager.prototype.nicknameForWallet = function (account) { - const address = ethUtil.addHexPrefix(account.toLowerCase()) + const address = normalize(account) const nicknames = this.getWalletNicknames() return nicknames[address] } ConfigManager.prototype.setNicknameForWallet = function (account, nickname) { - const address = ethUtil.addHexPrefix(account.toLowerCase()) + const address = normalize(account) const nicknames = this.getWalletNicknames() nicknames[address] = nickname var data = this.getData() diff --git a/test/integration/lib/encryptor-test.js b/test/integration/lib/encryptor-test.js index d42608152..897d22740 100644 --- a/test/integration/lib/encryptor-test.js +++ b/test/integration/lib/encryptor-test.js @@ -1,5 +1,7 @@ var encryptor = require('../../../app/scripts/lib/encryptor') +QUnit.module('encryptor') + QUnit.test('encryptor:serializeBufferForStorage', function (assert) { assert.expect(1) var buf = new Buffer(2) @@ -65,3 +67,5 @@ QUnit.test('encryptor:encrypt & decrypt with wrong password', function(assert) { done() }) }) + + diff --git a/test/integration/lib/first-time.js b/test/integration/lib/first-time.js index d2fe31878..12c573db1 100644 --- a/test/integration/lib/first-time.js +++ b/test/integration/lib/first-time.js @@ -1,5 +1,7 @@ const PASSWORD = 'password123' +QUnit.module('first time usage') + QUnit.test('agree to terms', function (assert) { var done = assert.async() let app diff --git a/ui/app/actions.js b/ui/app/actions.js index 0cc55136f..41be1004c 100644 --- a/ui/app/actions.js +++ b/ui/app/actions.js @@ -5,7 +5,11 @@ var actions = { goHome: goHome, // menu state getNetworkStatus: 'getNetworkStatus', - + // transition state + TRANSITION_FORWARD: 'TRANSITION_FORWARD', + TRANSITION_BACKWARD: 'TRANSITION_BACKWARD', + transitionForward, + transitionBackward, // remote state UPDATE_METAMASK_STATE: 'UPDATE_METAMASK_STATE', updateMetamaskState: updateMetamaskState, @@ -166,16 +170,25 @@ function tryUnlockMetamask (password) { if (err) { dispatch(actions.unlockFailed(err.message)) } else { - let selectedAccount - try { - selectedAccount = newState.metamask.selectedAccount - } catch (e) {} - dispatch(actions.unlockMetamask(selectedAccount)) + dispatch(actions.transitionForward()) + dispatch(actions.updateMetamaskState(newState)) } }) } } +function transitionForward() { + return { + type: this.TRANSITION_FORWARD, + } +} + +function transitionBackward() { + return { + type: this.TRANSITION_BACKWARD, + } +} + function confirmSeedWords () { return (dispatch) => { dispatch(actions.showLoadingIndication()) diff --git a/ui/app/reducers/app.js b/ui/app/reducers/app.js index 1f40e90b3..67a926948 100644 --- a/ui/app/reducers/app.js +++ b/ui/app/reducers/app.js @@ -43,7 +43,19 @@ function reduceApp (state, action) { switch (action.type) { - // intialize + // transition methods + + case actions.TRANSITION_FORWARD: + return extend(appState, { + transForward: true, + }) + + case actions.TRANSITION_BACKWARD: + return extend(appState, { + transForward: false, + }) + + // intialize case actions.SHOW_CREATE_VAULT: return extend(appState, {