Skip to content

Commit

Permalink
fix: throw errors with correct stack trace (#35)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@achingbrain @vasco-santos
achingbrain authored and vasco-santos committed Jun 13, 2019
1 parent ef47374 commit 7051b9c
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 30 deletions.
1 change: 1 addition & 0 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
14 changes: 8 additions & 6 deletions src/cms.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand All @@ -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([
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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)
}

Expand Down
49 changes: 25 additions & 24 deletions src/keychain.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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/'
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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()))
Expand All @@ -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) => {
Expand All @@ -327,23 +328,23 @@ 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)
const oldInfoName = DsInfoName(oldName)
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)
Expand Down Expand Up @@ -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) => {
Expand All @@ -405,17 +406,17 @@ 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')
}
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) => {
Expand All @@ -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)
Expand Down Expand Up @@ -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())
})
Expand Down
14 changes: 14 additions & 0 deletions test/keychain.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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')
})
})
})
Expand Down Expand Up @@ -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()
})
})
Expand Down Expand Up @@ -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()
})
})
Expand Down Expand Up @@ -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()
})
})
Expand Down Expand Up @@ -344,27 +352,31 @@ 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()
})
})

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()
})
})

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()
})
})

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()
})
})
Expand Down Expand Up @@ -406,13 +418,15 @@ 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()
})
})

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()
})
})
Expand Down

0 comments on commit 7051b9c

Please sign in to comment.