From 7051b9c530821e1fd21e28fffee5a17f1e2d46e9 Mon Sep 17 00:00:00 2001
From: Alex Potsides <alex@achingbrain.net>
Date: Thu, 13 Jun 2019 14:35:12 +0100
Subject: [PATCH] fix: throw errors with correct stack trace (#35)

The stack trace of thrown error objects is created when the object is
instantiated - if we defer to a function to create the error we end
up with misleading stack traces.

This PR instantiates errors where errors occur and also uses the
`err-code` module to add a `.code` property so we don't have to depend
on string error messages for the type of error that was thrown.
---
 package.json          |  1 +
 src/cms.js            | 14 +++++++------
 src/keychain.js       | 49 ++++++++++++++++++++++---------------------
 test/keychain.spec.js | 14 +++++++++++++
 4 files changed, 48 insertions(+), 30 deletions(-)

diff --git a/package.json b/package.json
index e048791..7521e6d 100644
--- a/package.json
+++ b/package.json
@@ -43,6 +43,7 @@
   "homepage": "https://github.com/libp2p/js-libp2p-keychain#readme",
   "dependencies": {
     "async": "^2.6.2",
+    "err-code": "^1.1.2",
     "interface-datastore": "~0.6.0",
     "libp2p-crypto": "~0.16.1",
     "merge-options": "^1.0.1",
diff --git a/src/cms.js b/src/cms.js
index 90d7d85..d086407 100644
--- a/src/cms.js
+++ b/src/cms.js
@@ -8,6 +8,7 @@ require('node-forge/lib/pkcs7')
 require('node-forge/lib/pbe')
 const forge = require('node-forge/lib/forge')
 const util = require('./util')
+const errcode = require('err-code')
 
 /**
  * Cryptographic Message Syntax (aka PKCS #7)
@@ -26,7 +27,7 @@ class CMS {
    */
   constructor (keychain) {
     if (!keychain) {
-      throw new Error('keychain is required')
+      throw errcode(new Error('keychain is required'), 'ERR_KEYCHAIN_REQUIRED')
     }
 
     this.keychain = keychain
@@ -47,7 +48,7 @@ class CMS {
     const done = (err, result) => setImmediate(() => callback(err, result))
 
     if (!Buffer.isBuffer(plain)) {
-      return done(new Error('Plain data must be a Buffer'))
+      return done(errcode(new Error('Plain data must be a Buffer'), 'ERR_INVALID_PARAMS'))
     }
 
     series([
@@ -93,7 +94,7 @@ class CMS {
     const done = (err, result) => setImmediate(() => callback(err, result))
 
     if (!Buffer.isBuffer(cmsData)) {
-      return done(new Error('CMS data is required'))
+      return done(errcode(new Error('CMS data is required'), 'ERR_INVALID_PARAMS'))
     }
 
     const self = this
@@ -103,7 +104,7 @@ class CMS {
       const obj = forge.asn1.fromDer(buf)
       cms = forge.pkcs7.messageFromAsn1(obj)
     } catch (err) {
-      return done(new Error('Invalid CMS: ' + err.message))
+      return done(errcode(new Error('Invalid CMS: ' + err.message), 'ERR_INVALID_CMS'))
     }
 
     // Find a recipient whose key we hold. We only deal with recipient certs
@@ -124,8 +125,9 @@ class CMS {
         if (err) return done(err)
         if (!r) {
           const missingKeys = recipients.map(r => r.keyId)
-          err = new Error('Decryption needs one of the key(s): ' + missingKeys.join(', '))
-          err.missingKeys = missingKeys
+          err = errcode(new Error('Decryption needs one of the key(s): ' + missingKeys.join(', ')), 'ERR_MISSING_KEYS', {
+            missingKeys
+          })
           return done(err)
         }
 
diff --git a/src/keychain.js b/src/keychain.js
index cecc320..f8f8889 100644
--- a/src/keychain.js
+++ b/src/keychain.js
@@ -8,6 +8,7 @@ const DS = require('interface-datastore')
 const collect = require('pull-stream/sinks/collect')
 const pull = require('pull-stream/pull')
 const CMS = require('./cms')
+const errcode = require('err-code')
 
 const keyPrefix = '/pkcs8/'
 const infoPrefix = '/info/'
@@ -50,7 +51,7 @@ function _error (callback, err) {
   const min = 200
   const max = 1000
   const delay = Math.random() * (max - min) + min
-  if (typeof err === 'string') err = new Error(err)
+
   setTimeout(callback, delay, err, null)
 }
 
@@ -181,26 +182,26 @@ class Keychain {
     const self = this
 
     if (!validateKeyName(name) || name === 'self') {
-      return _error(callback, `Invalid key name '${name}'`)
+      return _error(callback, errcode(new Error(`Invalid key name '${name}'`), 'ERR_INVALID_KEY_NAME'))
     }
 
     if (typeof type !== 'string') {
-      return _error(callback, `Invalid key type '${type}'`)
+      return _error(callback, errcode(new Error(`Invalid key type '${type}'`), 'ERR_INVALID_KEY_TYPE'))
     }
 
     if (!Number.isSafeInteger(size)) {
-      return _error(callback, `Invalid key size '${size}'`)
+      return _error(callback, errcode(new Error(`Invalid key size '${size}'`), 'ERR_INVALID_KEY_SIZE'))
     }
 
     const dsname = DsName(name)
     self.store.has(dsname, (err, exists) => {
       if (err) return _error(callback, err)
-      if (exists) return _error(callback, `Key '${name}' already exists`)
+      if (exists) return _error(callback, errcode(new Error(`Key '${name}' already exists`), 'ERR_KEY_ALREADY_EXISTS'))
 
       switch (type.toLowerCase()) {
         case 'rsa':
           if (size < 2048) {
-            return _error(callback, `Invalid RSA key size ${size}`)
+            return _error(callback, errcode(new Error(`Invalid RSA key size ${size}`), 'ERR_INVALID_KEY_SIZE'))
           }
           break
         default:
@@ -278,13 +279,13 @@ class Keychain {
    */
   findKeyByName (name, callback) {
     if (!validateKeyName(name)) {
-      return _error(callback, `Invalid key name '${name}'`)
+      return _error(callback, errcode(new Error(`Invalid key name '${name}'`), 'ERR_INVALID_KEY_NAME'))
     }
 
     const dsname = DsInfoName(name)
     this.store.get(dsname, (err, res) => {
       if (err) {
-        return _error(callback, `Key '${name}' does not exist. ${err.message}`)
+        return _error(callback, errcode(new Error(`Key '${name}' does not exist. ${err.message}`), 'ERR_KEY_NOT_FOUND'))
       }
 
       callback(null, JSON.parse(res.toString()))
@@ -301,7 +302,7 @@ class Keychain {
   removeKey (name, callback) {
     const self = this
     if (!validateKeyName(name) || name === 'self') {
-      return _error(callback, `Invalid key name '${name}'`)
+      return _error(callback, errcode(new Error(`Invalid key name '${name}'`), 'ERR_INVALID_KEY_NAME'))
     }
     const dsname = DsName(name)
     self.findKeyByName(name, (err, keyinfo) => {
@@ -327,10 +328,10 @@ class Keychain {
   renameKey (oldName, newName, callback) {
     const self = this
     if (!validateKeyName(oldName) || oldName === 'self') {
-      return _error(callback, `Invalid old key name '${oldName}'`)
+      return _error(callback, errcode(new Error(`Invalid old key name '${oldName}'`), 'ERR_OLD_KEY_NAME_INVALID'))
     }
     if (!validateKeyName(newName) || newName === 'self') {
-      return _error(callback, `Invalid new key name '${newName}'`)
+      return _error(callback, errcode(new Error(`Invalid new key name '${newName}'`), 'ERR_NEW_KEY_NAME_INVALID'))
     }
     const oldDsname = DsName(oldName)
     const newDsname = DsName(newName)
@@ -338,12 +339,12 @@ class Keychain {
     const newInfoName = DsInfoName(newName)
     this.store.get(oldDsname, (err, res) => {
       if (err) {
-        return _error(callback, `Key '${oldName}' does not exist. ${err.message}`)
+        return _error(callback, errcode(new Error(`Key '${oldName}' does not exist. ${err.message}`), 'ERR_KEY_NOT_FOUND'))
       }
       const pem = res.toString()
       self.store.has(newDsname, (err, exists) => {
         if (err) return _error(callback, err)
-        if (exists) return _error(callback, `Key '${newName}' already exists`)
+        if (exists) return _error(callback, errcode(new Error(`Key '${newName}' already exists`), 'ERR_KEY_ALREADY_EXISTS'))
 
         self.store.get(oldInfoName, (err, res) => {
           if (err) return _error(callback, err)
@@ -374,16 +375,16 @@ class Keychain {
    */
   exportKey (name, password, callback) {
     if (!validateKeyName(name)) {
-      return _error(callback, `Invalid key name '${name}'`)
+      return _error(callback, errcode(new Error(`Invalid key name '${name}'`), 'ERR_INVALID_KEY_NAME'))
     }
     if (!password) {
-      return _error(callback, 'Password is required')
+      return _error(callback, errcode(new Error('Password is required'), 'ERR_PASSWORD_REQUIRED'))
     }
 
     const dsname = DsName(name)
     this.store.get(dsname, (err, res) => {
       if (err) {
-        return _error(callback, `Key '${name}' does not exist. ${err.message}`)
+        return _error(callback, errcode(new Error(`Key '${name}' does not exist. ${err.message}`), 'ERR_KEY_NOT_FOUND'))
       }
       const pem = res.toString()
       crypto.keys.import(pem, this._(), (err, privateKey) => {
@@ -405,7 +406,7 @@ class Keychain {
   importKey (name, pem, password, callback) {
     const self = this
     if (!validateKeyName(name) || name === 'self') {
-      return _error(callback, `Invalid key name '${name}'`)
+      return _error(callback, errcode(new Error(`Invalid key name '${name}'`), 'ERR_INVALID_KEY_NAME'))
     }
     if (!pem) {
       return _error(callback, 'PEM encoded key is required')
@@ -413,9 +414,9 @@ class Keychain {
     const dsname = DsName(name)
     self.store.has(dsname, (err, exists) => {
       if (err) return _error(callback, err)
-      if (exists) return _error(callback, `Key '${name}' already exists`)
+      if (exists) return _error(callback, errcode(new Error(`Key '${name}' already exists`), 'ERR_KEY_ALREADY_EXISTS'))
       crypto.keys.import(pem, password, (err, privateKey) => {
-        if (err) return _error(callback, 'Cannot read the key, most likely the password is wrong')
+        if (err) return _error(callback, errcode(new Error('Cannot read the key, most likely the password is wrong'), 'ERR_CANNOT_READ_KEY'))
         privateKey.id((err, kid) => {
           if (err) return _error(callback, err)
           privateKey.export(this._(), (err, pem) => {
@@ -441,17 +442,17 @@ class Keychain {
   importPeer (name, peer, callback) {
     const self = this
     if (!validateKeyName(name)) {
-      return _error(callback, `Invalid key name '${name}'`)
+      return _error(callback, errcode(new Error(`Invalid key name '${name}'`), 'ERR_INVALID_KEY_NAME'))
     }
     if (!peer || !peer.privKey) {
-      return _error(callback, 'Peer.privKey is required')
+      return _error(callback, errcode(new Error('Peer.privKey is required'), 'ERR_MISSING_PRIVATE_KEY'))
     }
 
     const privateKey = peer.privKey
     const dsname = DsName(name)
     self.store.has(dsname, (err, exists) => {
       if (err) return _error(callback, err)
-      if (exists) return _error(callback, `Key '${name}' already exists`)
+      if (exists) return _error(callback, errcode(new Error(`Key '${name}' already exists`), 'ERR_KEY_ALREADY_EXISTS'))
 
       privateKey.id((err, kid) => {
         if (err) return _error(callback, err)
@@ -484,11 +485,11 @@ class Keychain {
    */
   _getPrivateKey (name, callback) {
     if (!validateKeyName(name)) {
-      return _error(callback, `Invalid key name '${name}'`)
+      return _error(callback, errcode(new Error(`Invalid key name '${name}'`), 'ERR_INVALID_KEY_NAME'))
     }
     this.store.get(DsName(name), (err, res) => {
       if (err) {
-        return _error(callback, `Key '${name}' does not exist. ${err.message}`)
+        return _error(callback, errcode(new Error(`Key '${name}' does not exist. ${err.message}`), 'ERR_KEY_NOT_FOUND'))
       }
       callback(null, res.toString())
     })
diff --git a/test/keychain.spec.js b/test/keychain.spec.js
index ed6f1a8..bcaa667 100644
--- a/test/keychain.spec.js
+++ b/test/keychain.spec.js
@@ -59,22 +59,27 @@ module.exports = (datastore1, datastore2) => {
         ks.removeKey('../../nasty', (err) => {
           expect(err).to.exist()
           expect(err).to.have.property('message', 'Invalid key name \'../../nasty\'')
+          expect(err).to.have.property('code', 'ERR_INVALID_KEY_NAME')
         })
         ks.removeKey('', (err) => {
           expect(err).to.exist()
           expect(err).to.have.property('message', 'Invalid key name \'\'')
+          expect(err).to.have.property('code', 'ERR_INVALID_KEY_NAME')
         })
         ks.removeKey('    ', (err) => {
           expect(err).to.exist()
           expect(err).to.have.property('message', 'Invalid key name \'    \'')
+          expect(err).to.have.property('code', 'ERR_INVALID_KEY_NAME')
         })
         ks.removeKey(null, (err) => {
           expect(err).to.exist()
           expect(err).to.have.property('message', 'Invalid key name \'null\'')
+          expect(err).to.have.property('code', 'ERR_INVALID_KEY_NAME')
         })
         ks.removeKey(undefined, (err) => {
           expect(err).to.exist()
           expect(err).to.have.property('message', 'Invalid key name \'undefined\'')
+          expect(err).to.have.property('code', 'ERR_INVALID_KEY_NAME')
         })
       })
     })
@@ -106,6 +111,7 @@ module.exports = (datastore1, datastore2) => {
       it('does not overwrite existing key', (done) => {
         ks.createKey(rsaKeyName, 'rsa', 2048, (err) => {
           expect(err).to.exist()
+          expect(err).to.have.property('code', 'ERR_KEY_ALREADY_EXISTS')
           done()
         })
       })
@@ -146,6 +152,7 @@ module.exports = (datastore1, datastore2) => {
           ks.createKey('bad-nist-rsa', 'rsa', 1024, (err) => {
             expect(err).to.exist()
             expect(err).to.have.property('message', 'Invalid RSA key size 1024')
+            expect(err).to.have.property('code', 'ERR_INVALID_KEY_SIZE')
             done()
           })
         })
@@ -246,6 +253,7 @@ module.exports = (datastore1, datastore2) => {
           expect(err).to.exist()
           expect(err).to.have.property('missingKeys')
           expect(err.missingKeys).to.eql([rsaKeyInfo.id])
+          expect(err).to.have.property('code', 'ERR_MISSING_KEYS')
           done()
         })
       })
@@ -344,6 +352,7 @@ module.exports = (datastore1, datastore2) => {
       it('requires an existing key name', (done) => {
         ks.renameKey('not-there', renamedRsaKeyName, (err) => {
           expect(err).to.exist()
+          expect(err).to.have.property('code', 'ERR_KEY_NOT_FOUND')
           done()
         })
       })
@@ -351,6 +360,7 @@ module.exports = (datastore1, datastore2) => {
       it('requires a valid new key name', (done) => {
         ks.renameKey(rsaKeyName, '..\not-valid', (err) => {
           expect(err).to.exist()
+          expect(err).to.have.property('code', 'ERR_NEW_KEY_NAME_INVALID')
           done()
         })
       })
@@ -358,6 +368,7 @@ module.exports = (datastore1, datastore2) => {
       it('does not overwrite existing key', (done) => {
         ks.renameKey(rsaKeyName, rsaKeyName, (err) => {
           expect(err).to.exist()
+          expect(err).to.have.property('code', 'ERR_KEY_ALREADY_EXISTS')
           done()
         })
       })
@@ -365,6 +376,7 @@ module.exports = (datastore1, datastore2) => {
       it('cannot create the "self" key', (done) => {
         ks.renameKey(rsaKeyName, 'self', (err) => {
           expect(err).to.exist()
+          expect(err).to.have.property('code', 'ERR_NEW_KEY_NAME_INVALID')
           done()
         })
       })
@@ -406,6 +418,7 @@ module.exports = (datastore1, datastore2) => {
       it('cannot remove the "self" key', (done) => {
         ks.removeKey('self', (err) => {
           expect(err).to.exist()
+          expect(err).to.have.property('code', 'ERR_INVALID_KEY_NAME')
           done()
         })
       })
@@ -413,6 +426,7 @@ module.exports = (datastore1, datastore2) => {
       it('cannot remove an unknown key', (done) => {
         ks.removeKey('not-there', (err) => {
           expect(err).to.exist()
+          expect(err).to.have.property('code', 'ERR_KEY_NOT_FOUND')
           done()
         })
       })