Ignore cookie parsing and domain errors #801

Merged
merged 4 commits into from Feb 6, 2014

Projects

None yet

5 participants

@lalitkapoor
Member

this addresses #794

/cc @stash @mikeal

@mikeal
Member
mikeal commented Feb 6, 2014

+1

@lalitkapoor
Member

@stash everything look good to you too?

@stash
Contributor
stash commented Feb 6, 2014

Seems OK

Also seems to me it would be great if we could make the constructor have this preference via request.jar({ignoreErrors:true/false}) a thing eventually.

@stash
Contributor
stash commented Feb 6, 2014

@lalitkapoor maybe have this._ignoreErrors = true in the constructor and then pass that in to this._jar.setCookieSync ?

@lalitkapoor
Member

@stash Thanks for the feedback 😄

Hmm, I think we should discuss possible options for the jar constructor in another issue, because if this is going to be tweak-able, then someone may also want to tweak other options as well - such as those in setCookie and getCookies. In general though I'm usually pretty cautious about adding more nobs - but I'm totally down if we can conclude strongly for it.

I suggest we merge this in and open another issue to discuss the cookie jar constructor options. Does that sounds good to you?

@lalitkapoor
Member

Looks like we're all okay with this pr. There are some additional things that can be discussed in a new issue. I don't see a reason to delay further - so I'm going to merge this in.

@mikeal I didn't notice a contributing.md or anything similar. I wanted to check to see if there are any established norms for this repo that I should follow before pushing the green button :) Thanks.

@mikeal
Member
mikeal commented Feb 6, 2014

I just push the big green button :)

I usually let things marinate a little bit in master (enough people run on master that we get some testing) and then push a release as soon as someone says "I need this." This system is the result of my own time constraints and not because it is the best process, so I'm open to changing it :)

@lalitkapoor
Member

👍

@lalitkapoor lalitkapoor merged commit 650e64b into request:master Feb 6, 2014

1 check passed

default The Travis CI build passed
Details
@lalitkapoor lalitkapoor deleted the lalitkapoor:794-ignore-cookie-parsing-and-domain-errors branch Feb 6, 2014
@stash stash referenced this pull request in salesforce/tough-cookie Feb 19, 2014
Closed

Some valid hostnames get rejected as having a public suffix #16

@mbrock
mbrock commented on 7fb1647 Jul 3, 2014

Hmm, what's the purpose of this commit? I had to revert this manually to find out that my cookies were rejected because of an error coming from tough-cookie.

Member

people mess up their cookie header more often than you think and, because this is default, it causes a lot of errors most people don't care about.

@mbrock
mbrock commented Jul 3, 2014

Oh, I was only looking at the specific commit through Git blame. Now that I've read the pull request and discussion:

Why make silently throwing away errors the default behavior? Should we really consider invalid cookies to be a normal, negligible part of HTTP requests—when the user has explicitly chosen to maintain a cookie jar?

If I'm writing an application that makes HTTP requests with a cookie jar, expecting some delicious cookies, and those cookies are rejected by the HTTP library, I expect to find out about this. Or at least to be able to find out!

@apsavin
apsavin commented Mar 22, 2015

+1 for change the default behavior (we should not ignore errors by default)
We need to provide an option to do not ignore errors here at least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment