Set-Cookie header parsing

Hi,

I just upgraded from 0.8 to 2.0.0 and some of my code stopped working.

I have a cookie that looks like this : 

Set-Cookie: clearance=xxxxxx; expires=Tue, 01-Oct-13 01:06:08 GMT; path=/; domain=.my_domain.com

with 0.8, r.cookies is set correctly, with 2.0.0 r.cookies is empty.

Interesting fact, if I remove the domain= and the cookie looks like this, it works 
Set-Cookie: clearance=xxxxxx; expires=Tue, 01-Oct-13 01:06:08 GMT; path=/

I also tested with 1.2.3 and it seems to have the same behavior.


> I also tested with 1.2.3 and it seems to have the same behavior.

Same behaviour as what?


"with 0.8, r.cookies is set correctly, with 2.0.0 r.cookies is empty."

=> same behavior in 1.2.3 as 2.0.0


Do you have a website you can share with cookies with domains like that? I presume this is entirely reproducible given a domain that returns a cookie like that.


I think it has to do with cookieJar not setting cookies if the domain "doesn't match" 

example:

``` pycon
>>> r = requests.get('http://127.0.0.1:32080', headers = { 'Host': 'fake.net'})
>>> r.cookies
<<class 'requests.cookies.RequestsCookieJar'>[]>

>>> r = requests.get('http://fake.net:32080')
>>> r.cookies
<<class 'requests.cookies.RequestsCookieJar'>[Cookie(version=0, name='uid', value='d3390f8e21cf85cdeb63fabc00b9c0c5b1380590207620', port=None, port_specified=False, domain='.fake.net', domain_specified=True, domain_initial_dot=True, path='/', path_specified=True, secure=False, expires=1577145000, discard=False, comment=None, comment_url=None, rest={}, rfc2109=False)]>
```


What kind of response do you get on the first request? (e.g., 200) Also what does `r.history` look like? I'm starting to wonder if this may be related to https://github.com/kennethreitz/requests/pull/1628


I don't think redirects are involved :

``` pycon
>>> r = requests.get('http://127.0.0.1:32080', headers = { 'Host': 'fake.net'})
>>> r.status_code
200
>>> r.history
[]
```

You can try it that way : 

``` pycon
>>> r = requests.get('http://4myrh.com', headers={ 'Host': '4myrh.com'})
>>> r.cookies
<<class 'requests.cookies.RequestsCookieJar'>[Cookie(version=0, name='__cfduid', value='d7f22f6bfec26598b35b8a3a6f6ce23c21380590976517', port=None, port_specified=False, domain='.4myrh.com', domain_specified=True, domain_initial_dot=True, path='/', path_specified=True, secure=False, expires=1577145000, discard=False, comment=None, comment_url=None, rest={}, rfc2109=False)]>

>>> r = requests.get('http://4myrh.com', headers={ 'Host': 'btzero.net'})
>>> r.cookies
<<class 'requests.cookies.RequestsCookieJar'>[]>
>>> r.headers['set-cookie']
'__cfduid=dd3460abe8fba1036628ad561e350c6811380590992633; expires=Mon, 23-Dec-2019 23:50:00 GMT; path=/; domain=.btzero.net'
```


This is default python `cookielib` behaviour:

``` pycon
>>> import requests
>>> s = requests.Session()
>>> import cookielib
>>> s.cookies
<<class 'requests.cookies.RequestsCookieJar'>[]>
>>> s.cookies = cookielib.CookieJar()
>>> s.cookies
<cookielib.CookieJar[]>
>>> r = s.get('http://4myrh.com', headers={'Host': 'btzero.net'})
>>> r.cookies
<<class 'requests.cookies.RequestsCookieJar'>[]>
>>> s.cookies
<cookielib.CookieJar[]>
```

This is not an issue in requests. Perhaps we did something different with the Host header previously and possibly ignored it, but that version is so ancient that I don't see this as a real issue. If @kennethreitz and @Lukasa agree they can close this issue.


Could you eventually point me at a way to accomplish this that doesn't involve staying stuck in the past or resorting to urllib2? 


If I can figure it out myself I will. :)


Here is an ugly hack to override the request_host function in cookie lib (http://hg.python.org/cpython/file/94628dc3b05c/Lib/cookielib.py#l582) 

I think it would make sense for requests library to consider adjusting what is returned by the MockRequests object gives too cookie jar.  I can't think of a reason why the Host header wouldn't be authoritative. 

```
>>> import requests
>>> import cookielib
>>>
>>> def request_host(request):
...     host = request.get_header("Host", "")
...     host = cookielib.cut_port_re.sub("", host, 1)
...     return host.lower()
...
>>> requests.cookies.cookielib.request_host = request_host
>>> r = requests.get('http://4myrh.com', headers={'Host': 'btzero.net'})
>>> r.cookies
<<class 'requests.cookies.RequestsCookieJar'>[Cookie(version=0, name='__cfduid', value='d5aada69de9d23090afeb425b150829331380603630290', port=None, port_specified=False, domain='.btzero.net', domain_specified=True, domain_initial_dot=True, path='/', path_specified=True, secure=False, expires=1577145000, discard=False, comment=None, comment_url=None, rest={}, rfc2109=False)]>
```


@dknecht We're in a tough place here. We absolutely should not monkeypatch cookielib, that's craziness. However, the default implementation of `request_host` calls `Request.get_full_url()`. It might be possible to adjust what we return from there, but that seems pretty crappy.


@Lukasa  Yeah i wasn't suggesting monkeypatch cookielib.  That was just for a quick work around in case anybody needed it.  My comment around about what requests might consider was as you said adjusting what get_full_url returns in MockRequests.  (comment clarified above)


@Lukasa Also wanted to mention that the use case by @mtourne was meant as easy example.  The main use case is being able to test sites where we need to change the IP from what the domain would normally resolve to.  

Maybe the right way to solve this is how curl solved it recently?

Previously in curl you would do `curl -H'Host: fake.net' http://127.0.0.1` and now you can do `curl -v --resolve fake.net:80:127.0.0.1 http://fake.net``

```
   --resolve <host:port:address>
          Provide  a  custom  address  for a specific host and port pair. Using this, you can make the curl requests(s) use a specified address and prevent the otherwise
          normally resolved address to be used. Consider it a sort of /etc/hosts alternative provided on the command line. The port number should be the number used  for
          the specific protocol the host will be used for. It means you need several entries if you want to provide address for the same host but different ports.

          This option can be used many times to add many host names to resolve.

          (Added in 7.21.3)
```


@dknecht Oh I absolutely realised that. =) My thing is that it feels like the wrong way of doing it. cURL's solution also seems like an ugly hack for what is fundamentally a solved problem: just edit your hosts file. =D


@Lukasa  Cool.  Just trying to throw out other options that are cleaner.  We broke most of our test suite with when we upgraded to get some of the other new features and trying to avoid downgrading.  


I'm going to investigate the `get_full_url` solution soon to see if that works cleanly. It makes me pretty sad, but I think it might be the simplest way to get what we want here.


So I think another idea is that [hooks are fired before cookies are extracted](https://github.com/kennethreitz/requests/blob/master/requests/sessions.py#L465). Would it make sense to include a hook that people could use for this? I'm not sure if this is something people should expect to work in the first place. Opinions?


I don't think we should change the actual URL on the `PreparedRequest`. Moreover, I don't think we should force people to use a hook for this. We should either do it (if it can work cleanly) or we should just say 'use a hosts file'.


I think `use a hosts file` is a bit short sighted. If people are testing something and need to perform this god awful trick of specifying a URL and setting the `Host` header they may not have complete control over their CI system. Even if there is just a rough version of the hook in the docs for the person to use in that case I think it is better. On top of that, due to the fact that there were two major (backwards incompatible) version changes between when this worked and now, I'm not as sympathetic as you are.

The reality is that people will use requests in far worse ways (and I am arguably one of those people, see: sigmavirus24/betamax) but at the same time, nothing obligates us to actually handle each case like this that may arise. It would be awesome and such, but it will lead to potential regressions that are unlikely to be well tested and further bug reports like this one where we accidentally (or maybe not so accidentally) removed the behaviour and never heard a peep about it. I'm leaning towards at the very least documenting this behaviour if not also a hook. I would expect that anyone who might use this trick would also have read the documentation and seen (what I hope to be) strong admonitions against it and the caveat that if you absolutely must do it, here's a hook and don't file a bug report because it isn't a bug (in my humble opinion).


Hmm, I think you're probably right. My objection to using a hook is that it's so overwhelmingly special-case, in that it could only possibly have one use-case. That kind of special-case behaviour is abhorrent to me unless it's widely used, which as you point out this can't possibly be. =D

Why not take a third option: add a section to the documentation talking about edge cases like this one, and provide code snippets that can help with the pain, like the one proposed above.


@Lukasa agreed. 


This is honestly a pretty frequent situation for people that use your library for testing. The reason it hasn't been reported is people don't usually upgarde their tests suites dependencies that often.  The two backwards incompatible changes were both with in the last year so that is probably the reason you haven't heard much.  I think the fact that CURL added in functionality speaks to the fact that this is common case.  People choose this library because it is "HTTP for Humans" and shouldn't require reading the docs for hacks to common use cases.  This seems very similar to how we use "verify=false" for testing SSL related issues.


> The reason it hasn't been reported is people don't usually upgarde their tests suites dependencies that often.

From personal experience I've found this to be about 50% true. Some people upgrade their test dependencies for various reasons, others don't care to because they don't feel it is entirely necessary. About the same number of people never upgrade their development dependencies either, especially when they see no immediate benefit in doing so.

> The two backwards incompatible changes were both with in the last year so that is probably the reason you haven't heard much. I think the fact that CURL added in functionality speaks to the fact that this is common case.

Maybe it's a common case for cURL users but we judge how common it is for our users based on the feedback we get. Frankly two people who work together does not make a use-case common. Also, cURL is the kitchen sink that has no bottom. They add pretty much everything they feel like and while it's certainly a solid tool, they also have far more people contributing and maintaining it. Requests has always done its best to avoid becoming cURL or a kitchen sink. We simply cannot maintain every corner case that people tell us about. We'll do our best to catch the ones that affect the largest number of people but we really cannot appeal to 100% of our users and we simply won't. It's been a driving tenant that we don't.

> People choose this library because it is "HTTP for Humans" and shouldn't require reading the docs for hacks to common use cases.

Trying to insult developers who work on software in their free time into action is never a solid choice and reflects poorly on you and the company you're representing.

> This seems very similar to how we use "verify=false" for testing SSL related issues.

Without context this means almost nothing to me. `verify=False` is used for far more than testing (just as requests is) and this statement means absolutely nothing.


I am really sorry that you took that as an insult as it was not meant to be.  This is a great library.  The reason we reported the issue and put lots of details in about our debugging process and proposed solutions was because we wanted to see if to continue that way.  We have already put a hack in our code to deal with this so a fix in the library really won't make much difference to us besides trying to help out others. 


@dknecht can you confirm that [this](https://github.com/sigmavirus24/requests/commit/0bb8be0e5ba58dfff9648b4058acaa927837e605) fixes the issue? I'm still not convinced this is common, but at this point, I'd rather just close out this issue on a somewhat positive note.


@sigmavirus24 Go ahead and open the PR with that commit. =)


@sigmavirus24  Thank you.  I tested and it works.  


@Lukasa Do you think we can get this merged in?  =)


@dknecht I do. =) The relevant PR is #1660. The way Requests works is that Ian and I triage and code review, but do not merge code: Kenneth does. This gives him final say. I'm confident he'll merge it, but we have to wait until he gets some free time to take a look. Sorry about the delay!


I believe the PR for this was merged and this can be closed. Any objections from anyone?