Enable loose cookie parsing in tough-cookie

[Sebmaster]

tough-cookie 2.1.0 got a loose cookie mode which is more how browsers actually behave. It implies an empty key for value-only cookies.

Not sure how request handles strict spec-compliance vs. practicality, so I thought I'd just get this started with a PR.

[simov]

Thanks, @Sebmaster, I just subscribed to the tough-cookie's PR thread.

@lalitkapoor want to chime in?

[stash-sfdc]

tough-cookie@2.2.0 now has the two PRs by @Sebmaster, specifically the changes to Cookie.parse and new CookieJar.

Tangental, but FYI: tough-cookie@2.1.0 updated the embedded public suffix list. Even if you don't want the loose parsing mode, request should probably update to depend on tough-cookie >= 2.1.0

[lalitkapoor]

thanks @stash-sfdc I'll review these shortly. Looks like we are already at 2.1.0. @greenkeeperio-bot will keep us notified when there is an updated :)

[lalitkapoor]

greenkeeper doing its thing: #1824 👍

[lalitkapoor]

This makes sense to me. We want to be reasonably flexible, and since most browsers are lenient in this manner. By default, request should be as well.

In the future it would be nice to eventually be able to specify these sort of things in a request option somewhere (in case someone does want to be strict).

Letting it sit for a day to give anyone else some time to chime in on this. Looks good to me.

👍 Thanks for the pr @Sebmaster :)

[Sebmaster]

After thinking this through a bit more with @domenic, it seems like providing {loose: true} explicitly to the setCookie method is a bad idea since it'll cause inconsistencies across the eco-system.

Instead, I'd suggest only enabling looseMode for wrapper cookie jars and if cookie jars are passed in from another source, just go with their specified behaviour. That means that users will have to specify looseMode: true in most cases themselves, until tough-cookie bumps the major and (hopefully) enables that behaviour by default. However, this solution will at least not produce any inconsistencies.

For more explanation regarding the inconsistencies:

[2015-10-07 15:21:05] <Sebmaster> also, you fine with https://github.com/tmpvar/jsdom/pull/1255 in its current state?
[2015-10-07 15:28:43] <Domenic> Sebmaster: I guess... I was hoping to expose the constructor directly in the future, so having createCookieJar act differently is a bummer.
[2015-10-07 15:28:52] <Domenic> Sebmaster: but we can work around it, e.g. create a derived class
[2015-10-07 15:29:11] <Domenic> Sebmaster: kind of a shame it isn't the default in tough-cookie though
[2015-10-07 15:29:46] <Domenic> Sebmaster: also confused why setCookieSync *also* needs { loose: true }
[2015-10-07 15:30:12] <Sebmaster> it's necessary for those outside-provided wrongly-initialized jars
[2015-10-07 15:30:51] <Sebmaster> we basically force them to behave correctly
[2015-10-07 15:32:21] <Domenic> Huh I see
[2015-10-07 15:32:30] <Domenic> Can we just not pass it to the constructor then?
[2015-10-07 15:33:25] <Sebmaster> only if request merges their PR; if they don't we have inconsistent behaviour
[2015-10-07 15:33:32] <Sebmaster> also inconsistent behaviour across other modules
[2015-10-07 15:33:39] <Domenic> gah, tableflip
[2015-10-07 15:33:43] <Sebmaster> hah
[2015-10-07 15:34:09] <Domenic> what about omitting it from setCookieSync, and saying that if you've passed in a bad cookie jar, you're a bad person and deserve what you get
[2015-10-07 15:34:09] <Sebmaster> setting that behaviour to be on by default would've been an easier transformation but would probably require a major :/
[2015-10-07 15:34:20] <Domenic> would it also cause inconsistency, or would it just impact bad people
[2015-10-07 15:34:45] <Sebmaster> if request does pull the PR, we're back at inconsistency
[2015-10-07 15:34:59] <Domenic> it seems like if you've passed in a new CookieJar(null, { loose: false }) then people should respect you
[2015-10-07 15:35:22] <Sebmaster> doesn't have to be explicitly false though
[2015-10-07 15:35:22] <Domenic> basically it seems like having this be configurable on both a per-cookie jar basis and a per-set basis is a bad idea
[2015-10-07 15:35:29] <Sebmaster> just new CookieJar() would produce the wrong behaviour
[2015-10-07 15:36:53] <Domenic> So I think I would prefer: kill the request PR; have jsdom pass { loose: true } to the constructor; if the user passes in a non-loose cookie jar then all their cookies are non-loose, oh well.
[2015-10-07 15:37:13] <Sebmaster> Right, that does sound like the better idea
[2015-10-07 15:37:30] <Domenic> and ideally, in tough cookie's next major: kill the loose option to setCookieSync; change the default behavior.

[lalitkapoor]

@Sebmaster Just for reference I'm leaving a note to also look at your comments here - jsdom/jsdom#1255

[Sebmaster]

Updated the PR to not force loose mode on the cookie jar, but instead create all cookie jars with loose mode enabled.

[lalitkapoor]

@Sebmaster is this reviewable again? And what did you change from before? Removed the loose option in setCookie? Thanks.

[simov]

@lalitkapoor that was my thinking, @Sebmaster probably it would be better to keep all commits in PRs at least until prior merging, otherwise the conversation seems broken, and you can't diff between the changes.

[Sebmaster]

Sorry about that. Only removed the loose: true on setCookie and instead put it on the cookie jar.

[lalitkapoor]

👍