diff --git a/app/scripts/controllers/permissions/enums.js b/app/scripts/controllers/permissions/enums.js index 12dc74a70..efd994c10 100644 --- a/app/scripts/controllers/permissions/enums.js +++ b/app/scripts/controllers/permissions/enums.js @@ -75,6 +75,7 @@ export const SAFE_METHODS = [ 'eth_sendTransaction', 'eth_sign', 'personal_sign', + 'personal_ecRecover', 'eth_signTypedData', 'eth_signTypedData_v1', 'eth_signTypedData_v3', diff --git a/app/scripts/controllers/transactions/lib/util.js b/app/scripts/controllers/transactions/lib/util.js index c77d70dd4..da58bf6f3 100644 --- a/app/scripts/controllers/transactions/lib/util.js +++ b/app/scripts/controllers/transactions/lib/util.js @@ -1,8 +1,8 @@ import { addHexPrefix, isValidAddress } from 'ethereumjs-util' const normalizers = { - from: (from, lowerCase = true) => (lowerCase ? addHexPrefix(from).toLowerCase() : addHexPrefix(from)), - to: (to, lowerCase = true) => (lowerCase ? addHexPrefix(to).toLowerCase() : addHexPrefix(to)), + from: (from) => addHexPrefix(from), + to: (to, lowerCase) => (lowerCase ? addHexPrefix(to).toLowerCase() : addHexPrefix(to)), nonce: (nonce) => addHexPrefix(nonce), value: (value) => addHexPrefix(value), data: (data) => addHexPrefix(data), @@ -12,11 +12,12 @@ const normalizers = { /** * Normalizes the given txParams - * @param {Object} txParams - the tx params - * @param {boolean} lowerCase - whether to return the addresses lower cased + * @param {Object} txParams - The transaction params + * @param {boolean} [lowerCase] - Whether to lowercase the 'to' address. + * Default: true * @returns {Object} the normalized tx params */ -export function normalizeTxParams (txParams, lowerCase) { +export function normalizeTxParams (txParams, lowerCase = true) { // apply only keys in the normalizers const normalizedTxParams = {} for (const key in normalizers) { diff --git a/app/scripts/lib/encryption-public-key-manager.js b/app/scripts/lib/encryption-public-key-manager.js index 0ae12442b..f568c62db 100644 --- a/app/scripts/lib/encryption-public-key-manager.js +++ b/app/scripts/lib/encryption-public-key-manager.js @@ -283,5 +283,4 @@ export default class EncryptionPublicKeyManager extends EventEmitter { this.memStore.updateState({ unapprovedEncryptionPublicKeyMsgs, unapprovedEncryptionPublicKeyMsgCount }) this.emit('updateBadge') } - } diff --git a/app/scripts/lib/typed-message-manager.js b/app/scripts/lib/typed-message-manager.js index 147180ffd..49e71e59f 100644 --- a/app/scripts/lib/typed-message-manager.js +++ b/app/scripts/lib/typed-message-manager.js @@ -4,9 +4,11 @@ import createId from './random-id' import assert from 'assert' import { ethErrors } from 'eth-json-rpc-errors' import sigUtil from 'eth-sig-util' +import { isValidAddress } from 'ethereumjs-util' import log from 'loglevel' import jsonschema from 'jsonschema' import { MESSAGE_TYPE } from './enums' + /** * Represents, and contains data about, an 'eth_signTypedData' type signature request. These are created when a * signature for an eth_signTypedData call is requested. @@ -102,14 +104,15 @@ export default class TypedMessageManager extends EventEmitter { * */ addUnapprovedMessage (msgParams, req, version) { + msgParams.version = version - this.validateParams(msgParams) - // add origin from request if (req) { msgParams.origin = req.origin } + this.validateParams(msgParams) log.debug(`TypedMessageManager addUnapprovedMessage: ${JSON.stringify(msgParams)}`) + // create txData obj with parameters and meta data const time = (new Date()).getTime() const msgId = createId() @@ -134,37 +137,38 @@ export default class TypedMessageManager extends EventEmitter { * */ validateParams (params) { + + assert.ok(params && typeof params === 'object', 'Params must be an object.') + assert.ok('data' in params, 'Params must include a "data" field.') + assert.ok('from' in params, 'Params must include a "from" field.') + assert.ok( + typeof params.from === 'string' && isValidAddress(params.from), + '"from" field must be a valid, lowercase, hexadecimal Ethereum address string.' + ) + switch (params.version) { case 'V1': - assert.equal(typeof params, 'object', 'Params should ben an object.') - assert.ok('data' in params, 'Params must include a data field.') - assert.ok('from' in params, 'Params must include a from field.') - assert.ok(Array.isArray(params.data), 'Data should be an array.') - assert.equal(typeof params.from, 'string', 'From field must be a string.') + assert.ok(Array.isArray(params.data), '"params.data" must be an array.') assert.doesNotThrow(() => { sigUtil.typedSignatureHash(params.data) - }, 'Expected EIP712 typed data') + }, 'Signing data must be valid EIP-712 typed data.') break case 'V3': case 'V4': + assert.equal(typeof params.data, 'string', '"params.data" must be a string.') let data - assert.equal(typeof params, 'object', 'Params should be an object.') - assert.ok('data' in params, 'Params must include a data field.') - assert.ok('from' in params, 'Params must include a from field.') - assert.equal(typeof params.from, 'string', 'From field must be a string.') - assert.equal(typeof params.data, 'string', 'Data must be passed as a valid JSON string.') assert.doesNotThrow(() => { data = JSON.parse(params.data) - }, 'Data must be passed as a valid JSON string.') + }, '"data" must be a valid JSON string.') const validation = jsonschema.validate(data, sigUtil.TYPED_MESSAGE_SCHEMA) assert.ok(data.primaryType in data.types, `Primary type of "${data.primaryType}" has no type definition.`) - assert.equal(validation.errors.length, 0, 'Data must conform to EIP-712 schema. See https://git.io/fNtcx.') + assert.equal(validation.errors.length, 0, 'Signing data must conform to EIP-712 schema. See https://git.io/fNtcx.') const chainId = data.domain.chainId const activeChainId = parseInt(this.networkController.getNetworkState()) - chainId && assert.equal(chainId, activeChainId, `Provided chainId (${chainId}) must match the active chainId (${activeChainId})`) + chainId && assert.equal(chainId, activeChainId, `Provided chainId "${chainId}" must match the active chainId "${activeChainId}"`) break default: - assert.fail(`Unknown params.version ${params.version}`) + assert.fail(`Unknown typed data version "${params.version}"`) } } diff --git a/package.json b/package.json index 2692d8af8..1a68dfdb9 100644 --- a/package.json +++ b/package.json @@ -106,7 +106,7 @@ "eth-json-rpc-errors": "^2.0.2", "eth-json-rpc-filters": "^4.1.1", "eth-json-rpc-infura": "^4.0.2", - "eth-json-rpc-middleware": "^4.4.1", + "eth-json-rpc-middleware": "^5.0.0", "eth-keyring-controller": "^6.0.0", "eth-method-registry": "^1.2.0", "eth-phishing-detect": "^1.1.4", diff --git a/test/unit/app/controllers/metamask-controller-test.js b/test/unit/app/controllers/metamask-controller-test.js index 262202419..4628aabc0 100644 --- a/test/unit/app/controllers/metamask-controller-test.js +++ b/test/unit/app/controllers/metamask-controller-test.js @@ -35,9 +35,33 @@ const ExtensionizerMock = { }, } +let loggerMiddlewareMock +const initializeMockMiddlewareLog = () => { + loggerMiddlewareMock = { + requests: [], + responses: [], + } +} +const tearDownMockMiddlewareLog = () => { + loggerMiddlewareMock = undefined +} + +const createLoggerMiddlewareMock = () => (req, res, next) => { + if (loggerMiddlewareMock) { + loggerMiddlewareMock.requests.push(req) + next((cb) => { + loggerMiddlewareMock.responses.push(res) + cb() + }) + } else { + next() + } +} + const MetaMaskController = proxyquire('../../../../app/scripts/metamask-controller', { './controllers/threebox': { default: ThreeBoxControllerMock }, 'extensionizer': ExtensionizerMock, + './lib/createLoggerMiddleware': { default: createLoggerMiddlewareMock }, }).default const currentNetworkId = 42 @@ -96,7 +120,6 @@ describe('MetaMaskController', function () { // add sinon method spies sandbox.spy(metamaskController.keyringController, 'createNewVaultAndKeychain') sandbox.spy(metamaskController.keyringController, 'createNewVaultAndRestore') - sandbox.spy(metamaskController.txController, 'newUnapprovedTransaction') }) afterEach(function () { @@ -776,6 +799,17 @@ describe('MetaMaskController', function () { }) describe('#setupUntrustedCommunication', function () { + + const mockTxParams = { from: TEST_ADDRESS } + + beforeEach(function () { + initializeMockMiddlewareLog() + }) + + after(function () { + tearDownMockMiddlewareLog() + }) + it('sets up phishing stream for untrusted communication', async function () { const phishingMessageSender = { url: 'http://myethereumwalletntw.com', @@ -815,7 +849,7 @@ describe('MetaMaskController', function () { const message = { id: 1999133338649204, jsonrpc: '2.0', - params: ['mock tx params'], + params: [{ ...mockTxParams }], method: 'eth_sendTransaction', } streamTest.write({ @@ -824,15 +858,12 @@ describe('MetaMaskController', function () { }, null, () => { setTimeout(() => { assert.deepStrictEqual( - metamaskController.txController.newUnapprovedTransaction.getCall(0).args, - [ - 'mock tx params', - { - ...message, - origin: 'http://mycrypto.com', - tabId: 456, - }, - ] + loggerMiddlewareMock.requests[0], + { + ...message, + origin: 'http://mycrypto.com', + tabId: 456, + }, ) done() }) @@ -856,7 +887,7 @@ describe('MetaMaskController', function () { const message = { id: 1999133338649204, jsonrpc: '2.0', - params: ['mock tx params'], + params: [{ ...mockTxParams }], method: 'eth_sendTransaction', } streamTest.write({ @@ -865,14 +896,11 @@ describe('MetaMaskController', function () { }, null, () => { setTimeout(() => { assert.deepStrictEqual( - metamaskController.txController.newUnapprovedTransaction.getCall(0).args, - [ - 'mock tx params', - { - ...message, - origin: 'http://mycrypto.com', - }, - ] + loggerMiddlewareMock.requests[0], + { + ...message, + origin: 'http://mycrypto.com', + }, ) done() }) diff --git a/yarn.lock b/yarn.lock index f766b7a2c..f5cb5825c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -10174,7 +10174,7 @@ eth-json-rpc-middleware@^1.5.0: promise-to-callback "^1.0.0" tape "^4.6.3" -eth-json-rpc-middleware@^4.1.4, eth-json-rpc-middleware@^4.1.5, eth-json-rpc-middleware@^4.4.1: +eth-json-rpc-middleware@^4.1.4, eth-json-rpc-middleware@^4.1.5: version "4.4.1" resolved "https://registry.yarnpkg.com/eth-json-rpc-middleware/-/eth-json-rpc-middleware-4.4.1.tgz#07d3dd0724c24a8d31e4a172ee96271da71b4228" integrity sha512-yoSuRgEYYGFdVeZg3poWOwAlRI+MoBIltmOB86MtpoZjvLbou9EB/qWMOWSmH2ryCWLW97VYY6NWsmWm3OAA7A== @@ -10194,6 +10194,26 @@ eth-json-rpc-middleware@^4.1.4, eth-json-rpc-middleware@^4.1.5, eth-json-rpc-mid pify "^3.0.0" safe-event-emitter "^1.0.1" +eth-json-rpc-middleware@^5.0.0: + version "5.0.0" + resolved "https://registry.yarnpkg.com/eth-json-rpc-middleware/-/eth-json-rpc-middleware-5.0.0.tgz#9a202a41a2b1678c094cbffd0db0da619ec86f7b" + integrity sha512-31zLHvElrQV7opFZs47UqjMGFM4QCgMms7HJSQAZ3wjPuE3rFLxFUTeGHXwM3fAp9zh7n0M+B2gK7Ds4OUKfog== + dependencies: + btoa "^1.2.1" + clone "^2.1.1" + eth-query "^2.1.2" + eth-rpc-errors "^2.1.1" + eth-sig-util "^1.4.2" + ethereumjs-block "^1.6.0" + ethereumjs-tx "^1.3.7" + ethereumjs-util "^5.1.2" + ethereumjs-vm "^2.6.0" + fetch-ponyfill "^4.0.0" + json-rpc-engine "^5.1.3" + json-stable-stringify "^1.0.1" + pify "^3.0.0" + safe-event-emitter "^1.0.1" + eth-keyring-controller@^5.3.0, eth-keyring-controller@^5.6.1: version "5.6.1" resolved "https://registry.yarnpkg.com/eth-keyring-controller/-/eth-keyring-controller-5.6.1.tgz#7b7268400704c8f5ce98a055910341177dd207ca" @@ -10275,6 +10295,13 @@ eth-query@^2.0.2, eth-query@^2.1.0, eth-query@^2.1.2: json-rpc-random-id "^1.0.0" xtend "^4.0.1" +eth-rpc-errors@^2.1.1: + version "2.1.1" + resolved "https://registry.yarnpkg.com/eth-rpc-errors/-/eth-rpc-errors-2.1.1.tgz#00a7d6c8a9c864a8ab7d0356be20964e5bee4b13" + integrity sha512-MY3zAa5ZF8hvgQu1HOF9agaK5GgigBRGpTJ8H0oVlE0NqMu13CW6syyjLXdeIDCGQTbUeHliU1z9dVmvMKx1Tg== + dependencies: + fast-safe-stringify "^2.0.6" + eth-sig-util@2.3.0, eth-sig-util@^2.3.0: version "2.3.0" resolved "https://registry.yarnpkg.com/eth-sig-util/-/eth-sig-util-2.3.0.tgz#c54a6ac8e8796f7e25f59cf436982a930e645231"