Merge pull request #4513 from MetaMask/ValidateEmptyKey

Validate empty key

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## Current Master
 
+- Attempting to import an empty private key will now show a clear error.
 - Fix bug where metamask data would stop being written to disk after prolonged use
 - Fix bug where account reset did not work with custom RPC providers.
 - Fix bug where nonce mutex was never released

app/scripts/account-import-strategies/index.js

@@ -16,7 +16,18 @@ const accountImporter = {
 
   strategies: {
     'Private Key': (privateKey) => {
-      const stripped = ethUtil.stripHexPrefix(privateKey)
+      if (!privateKey) {
+        throw new Error('Cannot import an empty key.')
+      }
+
+      const prefixed = ethUtil.addHexPrefix(privateKey)
+      const buffer = ethUtil.toBuffer(prefixed)
+
+      if (!ethUtil.isValidPrivate(buffer)) {
+        throw new Error('Cannot import invalid private key.')
+      }
+
+      const stripped = ethUtil.stripHexPrefix(prefixed)
       return stripped
     },
     'JSON File': (input, password) => {

package-lock.json

@@ -8262,6 +8262,7 @@
       "resolved": "https://registry.npmjs.org/eth-sig-util/-/eth-sig-util-1.4.2.tgz",
       "integrity": "sha1-jZWCAsftuq6Dlwf7pvCf8ydgYhA=",
       "requires": {
+        "ethereumjs-abi": "git+https://github.com/ethereumjs/ethereumjs-abi.git#4ea2fdfed09e8f99117d9362d17c6b01b64a2bcf",
         "ethereumjs-util": "^5.1.1"
       },
       "dependencies": {
@@ -31331,6 +31332,7 @@
       "resolved": "https://registry.npmjs.org/web3/-/web3-0.20.3.tgz",
       "integrity": "sha1-yqRDc9yIFayHZ73ba6cwc5ZMqos=",
       "requires": {
+        "bignumber.js": "git+https://github.com/frozeman/bignumber.js-nolookahead.git#57692b3ecfc98bbdd6b3a516cb2353652ea49934",
         "crypto-js": "^3.1.4",
         "utf8": "^2.1.1",
         "xhr2": "*",

test/unit/app/account-import-strategies.spec.js

@@ -1,31 +1,59 @@
 const assert = require('assert')
 const path = require('path')
-const accountImporter = require('../../../app/scripts/account-import-strategies/index')
 const ethUtil = require('ethereumjs-util')
+const accountImporter = require('../../../app/scripts/account-import-strategies/index')
+const { assertRejects } = require('../test-utils')
 
 describe('Account Import Strategies', function () {
   const privkey = '0x4cfd3e90fc78b0f86bf7524722150bb8da9c60cd532564d7ff43f5716514f553'
   const json = '{"version":3,"id":"dbb54385-0a99-437f-83c0-647de9f244c3","address":"a7f92ce3fba24196cf6f4bd2e1eb3db282ba998c","Crypto":{"ciphertext":"bde13d9ade5c82df80281ca363320ce254a8a3a06535bbf6ffdeaf0726b1312c","cipherparams":{"iv":"fbf93718a57f26051b292f072f2e5b41"},"cipher":"aes-128-ctr","kdf":"scrypt","kdfparams":{"dklen":32,"salt":"7ffe00488319dec48e4c49a120ca49c6afbde9272854c64d9541c83fc6acdffe","n":8192,"r":8,"p":1},"mac":"2adfd9c4bc1cdac4c85bddfb31d9e21a684e0e050247a70c5698facf6b7d4681"}}'
 
-  it('imports a private key and strips 0x prefix', async function () {
-    const importPrivKey = await accountImporter.importAccount('Private Key', [ privkey ])
-    assert.equal(importPrivKey, ethUtil.stripHexPrefix(privkey))
+  describe('private key import', function () {
+    it('imports a private key and strips 0x prefix', async function () {
+      const importPrivKey = await accountImporter.importAccount('Private Key', [ privkey ])
+      assert.equal(importPrivKey, ethUtil.stripHexPrefix(privkey))
+    })
+
+    it('throws an error for empty string private key', async () => {
+      assertRejects(async function() {
+        await accountImporter.importAccount('Private Key', [ '' ])
+      }, Error, 'no empty strings')
+    })
+
+    it('throws an error for undefined string private key', async () => {
+      assertRejects(async function () {
+        await accountImporter.importAccount('Private Key', [ undefined ])
+      })
+    })
+
+    it('throws an error for undefined string private key', async () => {
+      assertRejects(async function () {
+        await accountImporter.importAccount('Private Key', [])
+      })
+    })
+
+    it('throws an error for invalid private key', async () => {
+      assertRejects(async function () {
+        await accountImporter.importAccount('Private Key', [ 'popcorn' ])
+      })
+    })
   })
 
-  it('fails when password is incorrect for keystore', async function () {
-    const wrongPassword = 'password2'
+  describe('JSON keystore import', function () {
+    it('fails when password is incorrect for keystore', async function () {
+      const wrongPassword = 'password2'
 
-    try {
-      await accountImporter.importAccount('JSON File', [ json, wrongPassword])
-    } catch (error) {
-      assert.equal(error.message, 'Key derivation failed - possibly wrong passphrase')
-    }
+      try {
+        await accountImporter.importAccount('JSON File', [ json, wrongPassword])
+      } catch (error) {
+        assert.equal(error.message, 'Key derivation failed - possibly wrong passphrase')
+      }
+    })
+
+    it('imports json string and password to return a private key', async function () {
+      const fileContentsPassword = 'password1'
+      const importJson = await accountImporter.importAccount('JSON File', [ json, fileContentsPassword])
+      assert.equal(importJson, '0x5733876abe94146069ce8bcbabbde2677f2e35fa33e875e92041ed2ac87e5bc7')
+    })
   })
-
-  it('imports json string and password to return a private key', async function () {
-    const fileContentsPassword = 'password1'
-    const importJson = await accountImporter.importAccount('JSON File', [ json, fileContentsPassword])
-    assert.equal(importJson, '0x5733876abe94146069ce8bcbabbde2677f2e35fa33e875e92041ed2ac87e5bc7')
-  })
-
 })

test/unit/test-utils.js

@@ -0,0 +1,17 @@
+const assert = require('assert')
+
+module.exports = {
+  assertRejects,
+}
+
+// assert.rejects added in node v10
+async function assertRejects (asyncFn, regExp) {
+  let f = () => {}
+  try {
+    await asyncFn()
+  } catch (error) {
+    f = () => { throw error }
+  } finally {
+    assert.throws(f, regExp)
+  }
+}