search

ssbc / ssb-ref

check if a string is a valid ssb-reference
MIT License
14 stars 10 forks source link

catch undefined crash #24

Closed cryptix closed 5 years ago

cryptix commented 5 years ago

somhow I'm getting http:// urls through this code path which don't have a multiaddr representation. addr is then false / can't be indexed and thus my node crashes.

It looks like the markdown parser tries to check if these are invites or something?!

Trace: got no addr from http://standardjs.com/
    at /home/cryptix/ssb-ref/index.js:101:12
    at /home/cryptix/ssb-ref/index.js:88:15
    at parseMultiServerInvite (/home/cryptix/ssb-ref/index.js:258:14)
    at exports.isMultiServerInvite (/home/cryptix/ssb-ref/index.js:215:14)
    at exports.isInvite (/home/cryptix/ssb-ref/index.js:221:36)
    at Object.exports.type (/home/cryptix/ssb-ref/index.js:292:13)
    at /home/cryptix/patchbay/node_modules/ssb-backlinks/emit-links.js:20:13
    at walk (/home/cryptix/patchbay/node_modules/ssb-backlinks/emit-links.js:53:5)
    at walk (/home/cryptix/patchbay/node_modules/ssb-backlinks/emit-links.js:50:7)
    at emitLinks (/home/cryptix/patchbay/node_modules/ssb-backlinks/emit-links.js:10:3)
Trace: got no addr from https://github.com/NixOS/nixpkgs/pull/46062/files
    at /home/cryptix/ssb-ref/index.js:101:12
    at /home/cryptix/ssb-ref/index.js:88:15
    at parseMultiServerInvite (/home/cryptix/ssb-ref/index.js:258:14)
    at exports.isMultiServerInvite (/home/cryptix/ssb-ref/index.js:215:14)
    at exports.isInvite (/home/cryptix/ssb-ref/index.js:221:36)
    at Object.exports.type (/home/cryptix/ssb-ref/index.js:292:13)
    at /home/cryptix/patchbay/node_modules/ssb-backlinks/emit-links.js:20:13
    at walk (/home/cryptix/patchbay/node_modules/ssb-backlinks/emit-links.js:53:5)
    at walk (/home/cryptix/patchbay/node_modules/ssb-backlinks/emit-links.js:50:7)
    at emitLinks (/home/cryptix/patchbay/node_modules/ssb-backlinks/emit-links.js:10:3)
Trace: got no addr from https://github.com/ssbc/ssb-blobs/blob/master/inject.js
    at /home/cryptix/ssb-ref/index.js:101:12
    at /home/cryptix/ssb-ref/index.js:88:15
    at parseMultiServerInvite (/home/cryptix/ssb-ref/index.js:258:14)
    at exports.isMultiServerInvite (/home/cryptix/ssb-ref/index.js:215:14)
    at exports.isInvite (/home/cryptix/ssb-ref/index.js:221:36)
    at Object.exports.type (/home/cryptix/ssb-ref/index.js:292:13)
    at /home/cryptix/patchbay/node_modules/ssb-backlinks/emit-links.js:20:13
    at walk (/home/cryptix/patchbay/node_modules/ssb-backlinks/emit-links.js:53:5)
    at walk (/home/cryptix/patchbay/node_modules/ssb-backlinks/emit-links.js:50:7)
    at emitLinks (/home/cryptix/patchbay/node_modules/ssb-backlinks/emit-links.js:10:3)
cryptix commented 5 years ago

here is the output of npm_ls.txt of that patchbay installation. 'm not sure why it says ssb-ref is unmet. I linked in my local checkout with this fix.

christianbundy commented 5 years ago

@cryptix Is there a way to repro this crash with just ssb-ref?

cryptix commented 5 years ago

I wish I knew... for me it started when I moved my key and resynced everything.

christianbundy commented 5 years ago

Is the fact that it's returning null the problem, or is ssb-ref crashing Node? Sorry, trying to be helpful but I'm not sure I completely understand.

const ssbRef = require('ssb-ref')
ssbRef.parseMultiServerInvite('http://standardjs.com/') // => null
cryptix commented 5 years ago

I’m not sure why it’s checking them for invites in the first place but the big problem for me is that it’s crashing sbot, yes.

I guess a ws:// addr might count as an multiserver address invite so I get why the pattern matches at least.

I’d say we should merge this. It’s an okay validation, I think... or make a try {} around this area.

christianbundy commented 5 years ago

Oh, I was mostly just thinking I'd like to write a test for it, but the !Array.isArray() check LGTM.

I might suggest either adding instructions to the console.trace or removing it -- is there something that developers using ssb-ref should be doing differently if they see the trace, or was that just for your debugging? We could also just check if (addr == null), since it should be returning undefined on failure.

dominictarr commented 5 years ago

aha, the problem was that a http url is technically a multiserver address. This isn't really what was intended, so to stay consistent with previous behaviour. I've added a check that an address must be have at least an transport and transform part, by checking for the presense of an unescaped ~ separator. I'm not sure if I want to go down the road of making ssb-ref be aware of the supported protocols yet. Maybe that is a good idea, but I'd rather leave it to this for now.

dominictarr commented 5 years ago

merged into 2.13.2 with test coverage