Constrain spaces before = to 256 #97
Conversation
Side-steps ReDoS in Issue #92
| @@ -58,11 +58,11 @@ var CONTROL_CHARS = /[\x00-\x1F]/; | |||
| // (see: https://github.com/ChromiumWebApps/chromium/blob/b3d3b4da8bb94c1b2e061600df106d590fda3620/net/cookies/parsed_cookie.cc#L60) | |||
| // '=' and ';' are attribute/values separators | |||
| // (see: https://github.com/ChromiumWebApps/chromium/blob/b3d3b4da8bb94c1b2e061600df106d590fda3620/net/cookies/parsed_cookie.cc#L64) | |||
| var COOKIE_PAIR = /^(([^=;]+))\s*=\s*([^\n\r\0]*)/; | |||
| var COOKIE_PAIR = /^(([^=;]+))\s{0,256}=\s*([^\n\r\0]*)/; | |||
There was a problem hiding this comment.
Should we add a comment mentioning why we have this new restriction?
|
Will there be a separate commit for version (2.3.3?) bump? |
|
@spasam yeah, it's on master (typically do it there once merged) |
See: https://www.versioneye.com/Node.JS/tough-cookie/2.3.2; salesforce/tough-cookie#97; salesforce/tough-cookie#92. (A large cookie could cause a slowdown.) This is not really necessary since fresh installs or installs using package-lock.json already get the 2.3.3 version, but we might as well.
|
Umm... one question... though late. Most browsers truncate cookies beyond a length of 4096 bytes. Why not put that restriction instead of this? We use tough-cookie inside Postman App and the edge-case dev usage of cookie data is wide-ranging. A more widely acceptable limitation is total cookie length. |
|
@shamasis it only limits the number of spaces, and most cookies won't contain that many, so almost every 4kiB cookie will work. I'm working to have the spaces limit lifted entirely in #100. P.S., it makes me happy to hear that you're using it in Postman App. I love hearing how folks are using tough-cookie (a nice change from the stream of bugs/changes/etc.). So, thank you :) |
Side-steps ReDoS in Issue #92