diff --git a/.gitignore b/.gitignore index 85c2d15d6..1806b1932 100644 --- a/.gitignore +++ b/.gitignore @@ -24,4 +24,7 @@ test/background.js test/bundle.js test/test-bundle.js -notes.txt \ No newline at end of file +notes.txt + +.coveralls.yml +.nyc_output \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index bf18bb361..ab5d635d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ ## Current Master +## 3.9.2 2017-7-26 + +- Fix bugs that could sometimes result in failed transactions after switching networks. +- Include stack traces in txMeta's to better understand the life cycle of transactions +- Enhance blacklister functionality to include levenshtein logic. (credit to @sogoiii and @409H for their help!) + ## 3.9.1 2017-7-19 - No longer automatically request 1 ropsten ether for the first account in a new vault. diff --git a/README.md b/README.md index d7086ae91..9aded09c1 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ -# MetaMask Plugin [![Build Status](https://circleci.com/gh/MetaMask/metamask-extension.svg?style=shield&circle-token=a1ddcf3cd38e29267f254c9c59d556d513e3a1fd)](https://circleci.com/gh/MetaMask/metamask-extension) +# MetaMask Plugin [![Build Status](https://circleci.com/gh/MetaMask/metamask-extension.svg?style=shield&circle-token=a1ddcf3cd38e29267f254c9c59d556d513e3a1fd)](https://circleci.com/gh/MetaMask/metamask-extension) [![Coverage Status](https://coveralls.io/repos/github/MetaMask/metamask-extension/badge.svg?branch=master)](https://coveralls.io/github/MetaMask/metamask-extension?branch=master) ## Support diff --git a/app/manifest.json b/app/manifest.json index eadd99590..55e1eb5b1 100644 --- a/app/manifest.json +++ b/app/manifest.json @@ -1,7 +1,7 @@ { "name": "MetaMask", "short_name": "Metamask", - "version": "3.9.1", + "version": "3.9.2", "manifest_version": 2, "author": "https://metamask.io", "description": "Ethereum Browser Extension", diff --git a/app/scripts/blacklister.js b/app/scripts/blacklister.js index a45265a75..9337599cc 100644 --- a/app/scripts/blacklister.js +++ b/app/scripts/blacklister.js @@ -1,13 +1,41 @@ -const blacklistedDomains = require('etheraddresslookup/blacklists/domains.json') +const levenshtein = require('fast-levenshtein') +const blacklistedMetaMaskDomains = ['metamask.com'] +const blacklistedDomains = require('etheraddresslookup/blacklists/domains.json').concat(blacklistedMetaMaskDomains) +const whitelistedMetaMaskDomains = ['metamask.io', 'www.metamask.io'] +const whitelistedDomains = require('etheraddresslookup/whitelists/domains.json').concat(whitelistedMetaMaskDomains) +const LEVENSHTEIN_TOLERANCE = 4 +const LEVENSHTEIN_CHECKS = ['myetherwallet', 'myetheroll', 'ledgerwallet', 'metamask'] -function detectBlacklistedDomain() { - var strCurrentTab = window.location.hostname - if (blacklistedDomains && blacklistedDomains.includes(strCurrentTab)) { - window.location.href = 'https://metamask.io/phishing.html' - } + +// credit to @sogoiii and @409H for their help! +// Return a boolean on whether or not a phish is detected. +function isPhish(hostname) { + var strCurrentTab = hostname + + // check if the domain is part of the whitelist. + if (whitelistedDomains && whitelistedDomains.includes(strCurrentTab)) { return false } + + // check if the domain is part of the blacklist. + var isBlacklisted = blacklistedDomains && blacklistedDomains.includes(strCurrentTab) + + // check for similar values. + var levenshteinMatched = false + var levenshteinForm = strCurrentTab.replace(/\./g, '') + LEVENSHTEIN_CHECKS.forEach((element) => { + if (levenshtein.get(element, levenshteinForm) < LEVENSHTEIN_TOLERANCE) { + levenshteinMatched = true + } + }) + + return isBlacklisted || levenshteinMatched } -window.addEventListener('load', function() { - detectBlacklistedDomain() +window.addEventListener('load', function () { + var hostnameToCheck = window.location.hostname + if (isPhish(hostnameToCheck)) { + // redirect to our phishing warning page. + window.location.href = 'https://metamask.io/phishing.html' + } }) +module.exports = isPhish diff --git a/app/scripts/controllers/network.js b/app/scripts/controllers/network.js index c07f13b8d..0a3e5e26b 100644 --- a/app/scripts/controllers/network.js +++ b/app/scripts/controllers/network.js @@ -28,9 +28,9 @@ module.exports = class NetworkController extends EventEmitter { this._provider = provider } - initializeProvider (opts) { + initializeProvider (opts, providerContructor = MetaMaskProvider) { this.providerInit = opts - this._provider = MetaMaskProvider(opts) + this._provider = providerContructor(opts) this._proxy = new Proxy(this._provider, { get: (obj, name) => { if (name === 'on') return this._on.bind(this) @@ -38,6 +38,7 @@ module.exports = class NetworkController extends EventEmitter { }, set: (obj, name, value) => { this._provider[name] = value + return value }, }) this.provider.on('block', this._logBlock.bind(this)) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 5f3d84ebe..f71659042 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -1,10 +1,12 @@ const EventEmitter = require('events') const async = require('async') const extend = require('xtend') +const clone = require('clone') const ObservableStore = require('obs-store') const ethUtil = require('ethereumjs-util') const pify = require('pify') const TxProviderUtil = require('../lib/tx-utils') +const getStack = require('../lib/util').getStack const createId = require('../lib/random-id') const NonceTracker = require('../lib/nonce-tracker') @@ -22,7 +24,6 @@ module.exports = class TransactionController extends EventEmitter { this.blockTracker = opts.blockTracker this.nonceTracker = new NonceTracker({ provider: this.provider, - blockTracker: this.provider._blockTracker, getPendingTransactions: (address) => { return this.getFilteredTxList({ from: address, @@ -117,6 +118,17 @@ module.exports = class TransactionController extends EventEmitter { // updateTx (txMeta) { + // create txMeta snapshot for history + const txMetaForHistory = clone(txMeta) + // dont include previous history in this snapshot + delete txMetaForHistory.history + // add stack to help understand why tx was updated + txMetaForHistory.stack = getStack() + // add snapshot to tx history + if (!txMeta.history) txMeta.history = [] + txMeta.history.push(txMetaForHistory) + + // update the tx var txId = txMeta.id var txList = this.getFullTxList() var index = txList.findIndex(txData => txData.id === txId) @@ -134,7 +146,7 @@ module.exports = class TransactionController extends EventEmitter { } addUnapprovedTransaction (txParams, done) { - let txMeta + let txMeta = {} async.waterfall([ // validate (cb) => this.txProviderUtils.validateTxParams(txParams, cb), @@ -146,6 +158,7 @@ module.exports = class TransactionController extends EventEmitter { status: 'unapproved', metamaskNetworkId: this.getNetwork(), txParams: txParams, + history: [], } cb() }, @@ -165,6 +178,7 @@ module.exports = class TransactionController extends EventEmitter { txParams.value = txParams.value || '0x0' if (!txParams.gasPrice) { this.query.gasPrice((err, gasPrice) => { + if (err) return cb(err) // set gasPrice txParams.gasPrice = gasPrice @@ -191,8 +205,12 @@ module.exports = class TransactionController extends EventEmitter { // get next nonce const txMeta = this.getTx(txId) const fromAddress = txMeta.txParams.from + // wait for a nonce nonceLock = await this.nonceTracker.getNonceLock(fromAddress) + // add nonce to txParams txMeta.txParams.nonce = nonceLock.nextNonce + // add nonce debugging information to txMeta + txMeta.nonceDetails = nonceLock.nonceDetails this.updateTx(txMeta) // sign transaction const rawTx = await this.signTransaction(txId) @@ -201,6 +219,7 @@ module.exports = class TransactionController extends EventEmitter { nonceLock.releaseLock() } catch (err) { this.setTxStatusFailed(txId, { + stack: err.stack || err.message, errCode: err.errCode || err, message: err.message || 'Transaction failed during approval', }) @@ -364,11 +383,11 @@ module.exports = class TransactionController extends EventEmitter { var txId = txMeta.id if (!txHash) { - const errReason = { + return this.setTxStatusFailed(txId, { + stack: 'checkForTxInBlock: custom tx-controller error message', errCode: 'No hash was provided', message: 'We had an error while submitting this transaction, please try again.', - } - return this.setTxStatusFailed(txId, errReason) + }) } block.transactions.forEach((tx) => { @@ -452,13 +471,14 @@ module.exports = class TransactionController extends EventEmitter { if (isKnownTx) return // encountered real error - transition to error state this.setTxStatusFailed(txMeta.id, { + stack: err.stack || err.message, errCode: err.errCode || err, message: err.message, }) })) } - async _resubmitTx (txMeta, cb) { + async _resubmitTx (txMeta) { const address = txMeta.txParams.from const balance = this.ethStore.getState().accounts[address].balance if (!('retryCount' in txMeta)) txMeta.retryCount = 0 @@ -466,18 +486,21 @@ module.exports = class TransactionController extends EventEmitter { // if the value of the transaction is greater then the balance, fail. if (!this.txProviderUtils.sufficientBalance(txMeta.txParams, balance)) { const message = 'Insufficient balance.' - this.setTxStatusFailed(txMeta.id, { message }) - cb() - return log.error(message) + this.setTxStatusFailed(txMeta.id, { + stack: '_resubmitTx: custom tx-controller error', + message, + }) + log.error(message) + return } // Only auto-submit already-signed txs: - if (!('rawTx' in txMeta)) return cb() + if (!('rawTx' in txMeta)) return // Increment a try counter. txMeta.retryCount++ const rawTx = txMeta.rawTx - return await this.txProviderUtils.publishTransaction(rawTx, cb) + return await this.txProviderUtils.publishTransaction(rawTx) } // checks the network for signed txs and @@ -501,11 +524,11 @@ module.exports = class TransactionController extends EventEmitter { // extra check in case there was an uncaught error during the // signature and submission process if (!txHash) { - const errReason = { + this.setTxStatusFailed(txId, { + stack: '_checkPendingTxs: custom tx-controller error message', errCode: 'No hash was provided', message: 'We had an error while submitting this transaction, please try again.', - } - this.setTxStatusFailed(txId, errReason) + }) return } // get latest transaction status diff --git a/app/scripts/lib/nonce-tracker.js b/app/scripts/lib/nonce-tracker.js index b76dac4e8..8328e81ec 100644 --- a/app/scripts/lib/nonce-tracker.js +++ b/app/scripts/lib/nonce-tracker.js @@ -4,8 +4,8 @@ const Mutex = require('await-semaphore').Mutex class NonceTracker { - constructor ({ blockTracker, provider, getPendingTransactions }) { - this.blockTracker = blockTracker + constructor ({ provider, getPendingTransactions }) { + this.provider = provider this.ethQuery = new EthQuery(provider) this.getPendingTransactions = getPendingTransactions this.lockMap = {} @@ -31,21 +31,25 @@ class NonceTracker { const currentBlock = await this._getCurrentBlock() const pendingTransactions = this.getPendingTransactions(address) const pendingCount = pendingTransactions.length - assert(Number.isInteger(pendingCount), 'nonce-tracker - pendingCount is an integer') + assert(Number.isInteger(pendingCount), `nonce-tracker - pendingCount is not an integer - got: (${typeof pendingCount}) "${pendingCount}"`) const baseCountHex = await this._getTxCount(address, currentBlock) const baseCount = parseInt(baseCountHex, 16) - assert(Number.isInteger(baseCount), 'nonce-tracker - baseCount is an integer') + assert(Number.isInteger(baseCount), `nonce-tracker - baseCount is not an integer - got: (${typeof baseCount}) "${baseCount}"`) const nextNonce = baseCount + pendingCount - assert(Number.isInteger(nextNonce), 'nonce-tracker - nextNonce is an integer') - // return next nonce and release cb - return { nextNonce, releaseLock } + assert(Number.isInteger(nextNonce), `nonce-tracker - nextNonce is not an integer - got: (${typeof nextNonce}) "${nextNonce}"`) + // collect the numbers used to calculate the nonce for debugging + const blockNumber = currentBlock.number + const nonceDetails = { blockNumber, baseCount, baseCountHex, pendingCount } + // return nonce and release cb + return { nextNonce, nonceDetails, releaseLock } } async _getCurrentBlock () { - const currentBlock = this.blockTracker.getCurrentBlock() + const blockTracker = this._getBlockTracker() + const currentBlock = blockTracker.getCurrentBlock() if (currentBlock) return currentBlock return await Promise((reject, resolve) => { - this.blockTracker.once('latest', resolve) + blockTracker.once('latest', resolve) }) } @@ -79,6 +83,12 @@ class NonceTracker { return mutex } + // this is a hotfix for the fact that the blockTracker will + // change when the network changes + _getBlockTracker () { + return this.provider._blockTracker + } + } module.exports = NonceTracker diff --git a/app/scripts/lib/util.js b/app/scripts/lib/util.js new file mode 100644 index 000000000..bddd60ee8 --- /dev/null +++ b/app/scripts/lib/util.js @@ -0,0 +1,8 @@ +module.exports = { + getStack, +} + +function getStack () { + const stack = new Error('Stack trace generator - not an error').stack + return stack +} diff --git a/circle.yml b/circle.yml index 66eed17d7..2ea60bb9d 100644 --- a/circle.yml +++ b/circle.yml @@ -5,3 +5,6 @@ dependencies: pre: - "npm i -g testem" - "npm i -g mocha" +test: + override: + - "npm run ci" \ No newline at end of file diff --git a/package.json b/package.json index 219653a98..29db4dace 100644 --- a/package.json +++ b/package.json @@ -13,6 +13,8 @@ "test-unit": "METAMASK_ENV=test mocha --require test/helper.js --recursive \"test/unit/**/*.js\"", "single-test": "METAMASK_ENV=test mocha --require test/helper.js", "test-integration": "npm run buildMock && npm run buildCiUnits && testem ci -P 2", + "test-coverage": "nyc npm run test-unit && nyc report --reporter=text-lcov | coveralls", + "ci": "npm run lint && npm run test-coverage && npm run test-integration", "lint": "gulp lint", "buildCiUnits": "node test/integration/index.js", "watch": "mocha watch --recursive \"test/unit/**/*.js\"", @@ -78,6 +80,7 @@ "express": "^4.14.0", "extension-link-enabler": "^1.0.0", "extensionizer": "^1.0.0", + "fast-levenshtein": "^2.0.6", "gulp-eslint": "^2.0.0", "hat": "0.0.3", "idb-global": "^1.0.0", @@ -126,7 +129,7 @@ "valid-url": "^1.0.9", "vreme": "^3.0.2", "web3": "0.19.1", - "web3-provider-engine": "^13.2.8", + "web3-provider-engine": "^13.2.9", "web3-stream-provider": "^3.0.1", "xtend": "^4.0.1" }, @@ -143,6 +146,7 @@ "brfs": "^1.4.3", "browserify": "^13.0.0", "chai": "^3.5.0", + "coveralls": "^2.13.1", "deep-freeze-strict": "^1.1.1", "del": "^2.2.0", "envify": "^4.0.0", @@ -169,6 +173,7 @@ "mocha-jsdom": "^1.1.0", "mocha-sinon": "^1.1.5", "nock": "^8.0.0", + "nyc": "^11.0.3", "open": "0.0.5", "prompt": "^1.0.0", "qs": "^6.2.0", diff --git a/test/unit/blacklister-test.js b/test/unit/blacklister-test.js new file mode 100644 index 000000000..d9290795c --- /dev/null +++ b/test/unit/blacklister-test.js @@ -0,0 +1,24 @@ +const assert = require('assert') +const Blacklister = require('../../app/scripts/blacklister') + + +describe('blacklister', function () { + describe('#isPhish', function () { + it('should not flag whitelisted values', function () { + var result = Blacklister('www.metamask.io') + assert(!result) + }) + it('should flag explicit values', function () { + var result = Blacklister('metamask.com') + assert(result) + }) + it('should flag levenshtein values', function () { + var result = Blacklister('metmask.io') + assert(result) + }) + it('should not flag not-even-close values', function () { + var result = Blacklister('example.com') + assert(!result) + }) + }) +}) diff --git a/test/unit/network-contoller-test.js b/test/unit/network-contoller-test.js index 0c7ee9d70..87c2ee7a3 100644 --- a/test/unit/network-contoller-test.js +++ b/test/unit/network-contoller-test.js @@ -3,6 +3,9 @@ const NetworkController = require('../../app/scripts/controllers/network') describe('# Network Controller', function () { let networkController + const networkControllerProviderInit = { + getAccounts: () => {}, + } beforeEach(function () { networkController = new NetworkController({ @@ -10,26 +13,13 @@ describe('# Network Controller', function () { type: 'rinkeby', }, }) - // stub out provider - networkController._provider = new Proxy({}, { - get: (obj, name) => { - return () => {} - }, - }) - networkController.providerInit = { - getAccounts: () => {}, - } - networkController.ethQuery = new Proxy({}, { - get: (obj, name) => { - return () => {} - }, - }) + networkController.initializeProvider(networkControllerProviderInit, dummyProviderConstructor) }) describe('network', function () { describe('#provider', function () { it('provider should be updatable without reassignment', function () { - networkController.initializeProvider(networkController.providerInit) + networkController.initializeProvider(networkControllerProviderInit, dummyProviderConstructor) const provider = networkController.provider networkController._provider = {test: true} assert.ok(provider.test) @@ -75,3 +65,19 @@ describe('# Network Controller', function () { }) }) }) + +function dummyProviderConstructor() { + return { + // provider + sendAsync: noop, + // block tracker + start: noop, + stop: noop, + on: noop, + addListener: noop, + once: noop, + removeAllListeners: noop, + } +} + +function noop() {} \ No newline at end of file diff --git a/test/unit/nonce-tracker-test.js b/test/unit/nonce-tracker-test.js index 16cd6d008..b0283e159 100644 --- a/test/unit/nonce-tracker-test.js +++ b/test/unit/nonce-tracker-test.js @@ -18,11 +18,13 @@ describe('Nonce Tracker', function () { getPendingTransactions = () => pendingTxs - provider = { sendAsync: (_, cb) => { cb(undefined, {result: '0x0'}) } } - nonceTracker = new NonceTracker({ - blockTracker: { + provider = { + sendAsync: (_, cb) => { cb(undefined, {result: '0x0'}) }, + _blockTracker: { getCurrentBlock: () => '0x11b568', }, + } + nonceTracker = new NonceTracker({ provider, getPendingTransactions, }) diff --git a/test/unit/tx-controller-test.js b/test/unit/tx-controller-test.js index 7b86cfe14..31908569a 100644 --- a/test/unit/tx-controller-test.js +++ b/test/unit/tx-controller-test.js @@ -343,13 +343,17 @@ describe('Transaction Controller', function () { // Adding the fake tx: txController.addTx(clone(txMeta)) - txController._resubmitTx(txMeta, function (err) { - assert.ifError(err, 'should not throw an error') + txController._resubmitTx(txMeta) + .then(() => { const updatedMeta = txController.getTx(txMeta.id) assert.notEqual(updatedMeta.status, txMeta.status, 'status changed.') assert.equal(updatedMeta.status, 'failed', 'tx set to failed.') done() }) + .catch((err) => { + assert.ifError(err, 'should not throw an error') + done() + }) }) }) }) diff --git a/ui/app/reducers.js b/ui/app/reducers.js index 11efca529..36045772f 100644 --- a/ui/app/reducers.js +++ b/ui/app/reducers.js @@ -43,7 +43,6 @@ function rootReducer (state, action) { window.logState = function () { var stateString = JSON.stringify(window.METAMASK_CACHED_LOG_STATE, removeSeedWords, 2) - console.log(stateString) return stateString }