search

tj / node-cookie-signature

cookie signing
MIT License
185 stars 35 forks source link

Question - Base64 and removed chars #34

Closed ncou closed 3 years ago

ncou commented 3 years ago

Hi,

Nice piece of code. Just by curiosity i can't understand why you remove the characters "+" and "=" in your code : https://github.com/tj/node-cookie-signature/blob/master/index.js#L23

A) Why doesn't you replace the "+" character them by some "safe" characters like "-" and "_" for a base64 url safe mecanisum (the = is for the padding so mostly useless) ? B) Why doesn't you remove the character "/" that can happen in a base64 string. C) Does the fact to remove thoses characters improve security ?

Thanks and keep up the good work.

natevw commented 3 years ago

Interesting question. It looks like this dates all the way back to the ± original commit of the code in d6127bf3ac25eadf681149e53595f409e91aacc5.

One hunch is that it had something to do with trying to make the result safe for use in a cookie header. But that doesn't make a ton of sense in the end since for starters any of the non-alphanumeric base64 characters +, /, = should be fine in a cookie value (see https://stackoverflow.com/a/1969339/179583) not to mention that the input val is left as-is regardless….

natevw commented 3 years ago

Oh, heh! We were both just reading the regex wrong at first:

.replace(/\=+$/, '')

does not remove "+" (or "$"). The only thing it does is strip one or more [regex +] trailing padding = characters found at the end [regex $]. See https://regexper.com/#%2F%5C%3D%2B%24%2F for diagram.

So to answer each question:

A) the + doesn't get removed and so no need to trade anything in its place B) the / doesn't need removing either, it's also a valid character inside a cookie C) since all it's doing is removing the useless = padding, it should have no effect on security as is only for efficiency/aesthetic purposes. [The base64 transform is only ever used one way, so there's no decoding of the cleaned up result anyway — i.e. no concern like https://stackoverflow.com/a/4797861/179583 ever comes into play.]

Hope that helps! As you can see in the tests there's actually a result that includes the "+" character which is when I realized I should review the regex more carefully 😀

ncou commented 3 years ago

It all make sense now ! :+1: Thank you for your answers.