Catch error when unmarshaling instead of crashing #72
Conversation
ghost
assigned 
|
Currently more focused on libp2p-websocket-star. Will add the fix when I've released the new version ( |
|
Fixed it! |
src/index.js
Outdated
| if (err) { | ||
| return callback(err) | ||
| } | ||
| const pubKey = crypto.keys.unmarshalPublicKey(buf) |
There was a problem hiding this comment.
what about wrapping this line in a try/catch block?
There was a problem hiding this comment.
The whole thing is in try catch. Also included the Buffer.from because it throws an error when a non-base64 string is passed to it.
src/index.js
Outdated
| return callback(err) | ||
| } | ||
|
|
||
| callback(null, new PeerId(digest, null, pubKey)) |
There was a problem hiding this comment.
This is an anti-pattern, you should not call a callback from within a try/catch block because any future error that is thrown will circle back to this try catch.
There was a problem hiding this comment.
You need to move the callback to outside of the try/catch block
src/index.js
Outdated
| }) | ||
| } catch (e) { | ||
| callback(e) | ||
| } |
There was a problem hiding this comment.
Please always call errors, err so that the StandardJS linter catches them.
src/index.js
Outdated
| return callback(err) | ||
| } | ||
|
|
||
| callback(null, new PeerId(digest, privKey, privKey.public)) |
src/index.js
Outdated
| }) | ||
| } catch (e) { | ||
| callback(e) | ||
| } |
src/index.js
Outdated
| }) | ||
| } else { | ||
| callback(null, new PeerId(id, null, pub)) | ||
| } |
There was a problem hiding this comment.
One more. Golden rule, make try/catch blocks as small as they can be, only wrap the call that might throw and nothing else.
src/index.js
Outdated
| callback(null, new PeerId(id, null, pub)) | ||
| } | ||
| } catch (e) { | ||
| callback(e) |
|
Applied the requested changes but now it seems like libp2p-crypto also has the exact same issues with |
src/index.js
Outdated
| rawPubKey = obj.pubKey && Buffer.from(obj.pubKey, 'base64') | ||
| pub = rawPubKey && crypto.keys.unmarshalPublicKey(rawPubKey) | ||
| } catch (err) { | ||
| callback(err) |
|
@mkg20001 CI is failing, see https://circleci.com/gh/libp2p/js-peer-id/226 |
|
Yes I see. This is because, as I previously said, the libp2p-crypto module has the exact same error. Which is something that should be handled there. libp2p/js-libp2p-crypto#112 |
|
All tests seam to pass when using libp2p/js-libp2p-crypto#113 |
Releated #71