Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Constrain spaces before = to 256 #97

Merged
merged 2 commits into from Sep 21, 2017
Merged

Constrain spaces before = to 256 #97

merged 2 commits into from Sep 21, 2017

Conversation

stash-sfdc
Copy link
Contributor

@stash-sfdc stash-sfdc commented Sep 21, 2017

Side-steps ReDoS in Issue #92

Side-steps ReDoS in Issue #92
@stash-sfdc stash-sfdc requested a review from inikulin Sep 21, 2017
@stash-sfdc stash-sfdc self-assigned this Sep 21, 2017
@@ -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]*)/;
Copy link

@westy92 westy92 Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a comment mentioning why we have this new restriction?

Copy link
Contributor Author

@stash-sfdc stash-sfdc Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, will do

Copy link
Contributor

@inikulin inikulin left a comment

appart fro missing comment, lgtm

@stash-sfdc
Copy link
Contributor Author

@stash-sfdc stash-sfdc commented Sep 21, 2017

@inikulin @westy92 documented. LGTY?

@stash-sfdc stash-sfdc merged commit 98e0916 into master Sep 21, 2017
2 checks passed
@spasam
Copy link

@spasam spasam commented Sep 21, 2017

Will there be a separate commit for version (2.3.3?) bump?

@stash-sfdc
Copy link
Contributor Author

@stash-sfdc stash-sfdc commented Sep 21, 2017

@spasam yeah, it's on master (typically do it there once merged)

domenic pushed a commit to jsdom/jsdom that referenced this issue Sep 25, 2017
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.
@shamasis
Copy link

@shamasis shamasis commented Sep 26, 2017

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.

http://browsercookielimits.squawky.net/

@stash-sfdc
Copy link
Contributor Author

@stash-sfdc stash-sfdc commented Sep 26, 2017

@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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants