ssbc / ssb-ref

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

fix message links with unbox query #20

Closed ahdinosaur closed 5 years ago

ahdinosaur commented 5 years ago

the existing test case didn't include an edge case:

if the unbox key includes a '+' character, when parsed as a query string the '+' becomes a ' ' character, which is no longer a correct unbox key.

this changes the test to match the real-world test case from https://github.com/ssbc/secure-scuttlebutt/pull/220 and fixes the behavior with a hack.

christianbundy commented 5 years ago

Nice catch! Can a query string have multiple + characters? If so, this may only be replacing the first search result.

ahdinosaur commented 5 years ago

Can a query string have multiple + characters?

@christianbundy yes, a '+' character has no special significance in the base64-encoded unbox key, but it does have semantic meaning in a query string (it means the ' ' character). so yes we want to replace all occurrences, i didn't realize string.replace didn't do that.

ahdinosaur commented 5 years ago

this is also a total hack, but we deserve the hack from using url unsafe characters and pretending it's a url. i'm really keen in any future protocol changes, if we expect to use something as a human-readable identifier, it should be encoded in base58, so we avoid using non-alphanumeric characters (like '+' and '/' which we use now, but also '~' and '.' which are technically url safe but why take that risk).

christianbundy commented 5 years ago

Yeah, I think String.replace() can be a bit of a footgun for that reason. I'll make an inline comment, not sure why I didn't start the conversation there in the first place.

I'm super excited at the idea of using real URLs for ssb:// content, there are lots of great solutions.

dominictarr commented 5 years ago

if you use a regular expression with g mode enabled, it will replace every occurance. as I did here: https://github.com/ssbc/secure-scuttlebutt/blob/b758c09655ae71d2e1bbc295842694425134ff41/index.js#L84

mmckegg commented 5 years ago

Okay, I'm merging this!

mmckegg commented 5 years ago

📦 ssb-ref@2.13.8