Enable parsing cookies with an empty or no key

[Sebmaster]

This is basically a proposal for fixing #10 since I came across an instance where I'd need this.

I'm not sure about the handling of the "re-introduced" strict parameter. In order to not change behaviour when not given I set it to default to true for now, but I guess we'd need a better solution here?

[stash-sfdc]

Hi @Sebmaster. Thanks for the pull request! If you're interested, it needs a bit of work. Please note that SFDC requires a CLA to be signed in order to accept any PR (either by you or your employer).

We got rid of a similar strict flag when we moved from 1.x to 2.x, so I'd be hesitant to reintroduce it on the parse method. The problem is that folks do stuff like arrayOfStrings.map(Cookie.parse) which leads to some odd behavior since the function passed to Array#map has the signature function(currentValue, index, array). Since 0 is implicitly false, the very first string in the array gets parsed in "non-strict" and the subsequent, non-0 indexes are "strict". So, I'd like to please see a different API implementation. Perhaps tough-cookie needs some kind of global configuration options? Maybe change from "Class-static" to a "default-Factory-method" pattern (similar to how request and node's http.Agent allocate a default global to use)?

I'd like to try to maintain RFC compliance if possible, so unless this is how the RFC says to parse it (I honestly haven't looked yet), I don't think this feature should be "on" by default. You've implemented it as "off" by default, which is great, but I wanted to be clear that we can't just switch to LOOSE_COOKIE_PAIR all the time. Specifically, can you confirm that in the case of both cookie-strings abc and =abc that it's got no key but value abc?

Thanks again for taking the time to contribute this PR 💯

[Sebmaster]

So, I'd like to please see a different API implementation.

I actually thought of another method in the meanwhile: Make the parameter an options object instead and check for explicit existence of a strict property in that object. If the given param is either not an object, or that key doesn't exist, we just ignore it.

Specifically, can you confirm that in the case of both cookie-strings abc and =abc that it's got no key but value abc?

So, according to the RFC this is not actually a valid cookie - I think. However, all browsers can properly parse this cookie. Sending a Set-Cookie equal to "=abc", leads browsers to send a Cookie: abc header back, so we can say "abc" == "=abc". Since setting a cookie with abc= sets a different cookie (abc=), we can conclude that the name of the cookie is empty and the value is abc in both parsing cases.

Please note that SFDC requires a CLA to be signed in order to accept any PR (either by you or your employer).

Where can I find this CLA?

[inikulin]

@stash-sfdc @Sebmaster the thing is that nobody fully implements RFC spec 🎉 As far as I remember even IETF tests violate it at some cases in favor of common browser behavior. So, if it doesn't break existing tests and if this behavior is consistent across browsers, I'd rather keep it as non-optional.

[stash-sfdc]

@inikulin OK, agreed: if it's both IETF-test-compliant and consistent, then it should be the default behavior.

[Sebmaster]

So we remove the strict param altogether? As far as I remember this starts breaking the (IETF part of the) test suite, so what should I do about that?

[stash-sfdc]

@Sebmaster please see https://github.com/forcedotcom/idecore/wiki/Contributing-Code regarding the CLA.

[stash-sfdc]

@Sebmaster OK, i follow the key-versus-value logic. Thanks for explaining that clearly!

If on-by-default breaks the suite, then perhaps an options Object as the second parameter (that handles the Array#map use-case) would be best.

[inikulin]

@Sebmaster I have a feeling what we can make work both, we just need a more sophisticated regexp. No facts, just an assumption 🚶

[Sebmaster]

If I remember correctly the IETF suite explicitly tests to disallow the format we want to allow here, so I doubt it. Will look into it though.

Yeah, see for example: https://github.com/SalesforceEng/tough-cookie/blob/master/test/ietf_data/parser.json#L1515, which fails.

[Sebmaster]

Moved to options object and also added another test I found in the IETF tests which looked like a pretty interesting scenario to support (verified in browsers).

Will get the CLA done tomorrow.

[Sebmaster]

CLA is done too now.

[stash-sfdc]

Confirm CLA accepted! Welcome @Sebmaster!

[stash-sfdc]

Should this pass {loose: options.loose} or maybe just options entirely? On a hunch this might require code coverage -- try adding jar.setCookieSync('=a', url) and jar.setCookieSync('=a',url,{loose:true}) and make sure those behave as expected?

[Sebmaster]

Nice catch, fixed and added tests for it.

[stash-sfdc]

@Sebmaster looks good to me. Ready to be merged?

[Sebmaster]

Yep, should be good. 🎉