-
Notifications
You must be signed in to change notification settings - Fork 52
Catch error when unmarshaling instead of crashing #113
Conversation
src/keys/rsa.js
Outdated
} | ||
}) | ||
} catch (err) { | ||
callback(new Error('Key is invalid!')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs a return
kty: key.kty, | ||
n: key.n, | ||
e: key.e | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where would an exception be thrown in this operation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never underestimate the power of undefined: TypeError: Cannot read property 'e' of undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but in this case, it would never through that error, only if you did something like key.e.something
. As long as if key
exists and it is an object, key.e
will never throw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted this, but now inside a setImmediate
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And what about unmarshalPrivateKey(undefined, (err, res) => {})
??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkg20001 good point, fixing.
test/testCb.js
Outdated
cb() | ||
}) | ||
|
||
fnc(...args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not use this operator for now (we are still avoiding some ES6 features)
Going to push some changes here to help this move along. |
@diasdavid should be go to go. Please tell if there's anything else you'd like to see addressed here. |
Thanks @pgte :) @victorbjelkholm could you check what's wrong with Circle? |
@victorbjelkholm seems that ci-sync never got here. Mind checking? Going to merge for now. |
Related #112