Multiple Location headers in response are concatenated

tested with requests 2.9
when a http server resonds with multiple Location headers in 301 redirect the locations are concatenated 
eg
Location: http://foo.com
Location: http://foo.com

results in a redirect to 
"http://foo.com,http://foo.com"

pretty sure this behavior is not present in any other http clients


A web server MUST NOT respond with multiple location header fields. From [RFC 7320 Section 3.2.2](https://tools.ietf.org/html/rfc7230#section-3.2.2):

> A sender MUST NOT generate multiple header fields with the same field name in a message unless either the entire field value for that header field is defined as a comma-separated list [i.e., #(values)] or the header field is a well-known exception (as noted below).

There is exactly _one_ well-known exception: `Set-Cookie`. `Location` is not `Set-Cookie`, so that doesn't apply. So it would only be acceptable to send multiple `Location` headers if the syntax allowed multiple comma-separated values. `Location` is defined in [RFC 7231 Section 7.1.2](https://tools.ietf.org/html/rfc7231#section-7.1.2):

>    Location = URI-reference
> 
> The field value consists of a single URI-reference.

The server in this example is violating the HTTP specification. In the face of such violation, a user-agent is simply not required to take any specific action. In particular, while the example you provided above has a very simple course of action (both URLs are the same), if both URLs were _not_ the same requests would be unable to take any sensible action: the situation is too ambiguous to allow a correct decision.

At the level of requests actually catching this and treating it as an error condition is tricky, because requests only sees the header string `http://foo.com,http://foo.com`, which perversely does parse as a valid URL in the case of most Python libraries because in principle the comma is an allowed character in the host part of the URL. This puts us in a bit of a bind. However, `urllib3`'s `HTTPHeaderDict` structure could enforce this requirement and treat this case as an error on receiving it. That might be a sensible way to go, but it's us going out of our way to provide a slightly nicer error when a server does something it really really shouldn't do. @sigmavirus24, thoughts?


I guess then the web server is doing this to break python-urllib clients as it does not seem to affect web browsers or curl


@L2501 To be clear, we _could_ handle this case. I just don't think we should. The server is very wrong, and it is not our job to work around the failures of that server. In this instance there is a clear acceptable solution, but the general case is not solve able. We can handle this, and we're looking at how best to do that, but if you want to find someone to blame (and your tone suggests you do) then I recommend directing your blame at the website operator whose website is broken, rather than us.


Well i just expected it to only count the first header and igonre the rest to prevent CRLF Injection attacks...


Yes, @L2501. Please continue to act aggressively towards us for not having encountered such a poorly configured server. Please don't take any action to correct that server.

---

@Lukasa There was a proposed version of the Header Dict that ignored multiple headers for anything other Set-Cookie headers. We could fix this faster in requests by using `.getlist` on the original Header Dict from urllib3 and throwing an exception if it returns more than one value.


@L2501 That doesn't prevent CRLF injection attacks, it just keeps functioning in the face of them. My proposed solution above would actually avoid _some portion_ CRLF injection attacks by throwing an exception when an invalid header block is received. Of course, we cannot _generally_ prevent CRLF injection attacks and also re-use TCP connections, so any client that _does_ re-use TCP connections is vulnerable to a well-crafted CRLF injection attack. Generally speaking, defending against CRLF injection attacks is the responsibility of the server operator, not the client.

@sigmavirus24 The better long-term fix might be to promote urllib3's `HeaderDict` to use in requests. The risk with doing it your way is that we assume that the `raw` object exists and is a `urllib3` one. I suppose we could have a conditional path that _attempts_ to use urllib3's but falls back to using ours.

We could in fact do that, and then plan in 3.0.0 to promote urllib3's `HeaderDict` to use in requests which would allow us to remove the conditional code path in 3.0.0.


> The risk with doing it your way is that we assume that the raw object exists and is a urllib3 one.

Are you thinking about a potential AppEngineAdapter case where it returns an object that is nothing like urllib3 and further breaks urllib3's contract? We can fix this on the adapter level for requests instead of making requests handle that complexity through a whole lot of conditional cases.


@sigmavirus24 No, I'm thinking of test libraries or anything else that returns a `Response` object from a `HTTPAdapter` subclass that is not actually backed by a urllib3 response object.


@Lukasa I'm not as concerned about them. Any test library that hasn't been half-written and thrown up on PyPI will handle this just fine (requests-mock, betamax, vcrpy, all do just fine).


@sigmavirus24 Be that as it may, in general our code assumes that [`.raw` may not come from urllib3](https://github.com/kennethreitz/requests/blob/fc8fa1aa265bb14d59c68eb68a179bce17953967/requests/models.py#L657-L667), and we should endeavour to maintain that if possible.


So, that code was added in https://github.com/kennethreitz/requests/pull/1425 but there wasn't much justification provided at the time for having a conditional like that. The only other remotely similar piece of the library that I'm aware of is [our cookie handling](https://github.com/kennethreitz/requests/blob/f7cb7962417f6e106a8ead214fb556e63bb71014/requests/cookies.py#L116..L130) which just refuses to do anything if it isn't anything like a `urllib3` response.

We've held an illusion of urllib3 being an "implementation" detail but never supported anything other than that in reality. The likelihood that the raw response will not be a urllib3 object seems very close to 0 to me, but I'm open to being convinced otherwise.


@sigmavirus24 The common situation is where the response object is `StringIO` or something similar. I think going out of our way to avoid breaking that is a good idea. If we were going straight to 3.0.0 I'd be fine with breaking it, but ideally we'd deal with this sooner.


> The common situation is where the response object is StringIO or something similar.

I understand we're hoping to catch the case where it's more file-like than `urllib3.HTTPResponse` like, but I'm not sure how common that case is, is what I'm saying.

I also don't want to just remove support for this, but I'd like to see much better encapsulation of these shims honestly.


Agreed, I don't think we've done well enough in the past, but sadly I also don't think that's an excuse for making the mistake here. We know it's a risk, we should account for it.

That said, it's also fairly easy to have a fallback.


@sigmavirus24

> There was a proposed version of the Header Dict that ignored multiple headers for anything other Set-Cookie headers. We could fix this faster in requests by using `.getlist` on the original Header Dict from urllib3 and throwing an exception if it returns more than one value.

This would break the list-like headers though? The ones defined as `#(values)`, which RFC 7230 [permits](https://tools.ietf.org/html/rfc7230#section-3.2.2) to split into multiple fields.


@vfaronov That isn't being proposed _in general_, it's being proposed purely for the `Location` header.


Supporting multiple location headers is /not/ valid practice, see [here](http://stackoverflow.com/questions/11309444/can-the-location-header-be-used-for-multiple-resource-locations-in-a-201-created), and should be rejected.

However, supporting multiple headers in general is valid as per RFC2616, and requests should support that, see [here](http://stackoverflow.com/questions/4371328/are-duplicate-http-response-headers-acceptable).


@foxx I appreciate that you were trying to be helpful, but please read the discussion on this issue before commenting. If you look at the first comment in this issue you'll see that it begins with:

> A web server MUST NOT respond with multiple location header fields.

The proposed action in this thread is that requests should error on this condition, rather than doing what it currently does which is to attempt to parse the multiple `Location` fields as though they were one. Note also that that behaviour means that requests already _does_ support multiple headers.

Finally, RFC 2616 is deprecated. RFC 7230 and RFC 7231 have replaced it. =)


> please read the discussion on this issue before commenting.

Fwiw, I'd spent about 10 minutes reading through and researching before I posted, but it looks like I misread some of the further comments, apologies.

> The proposed action in this thread is that requests should error on this condition

Given that the RFCs are pretty clear on the situation of multiple `Location` headers, e.g. MUST NOT, I would have thought the default would be to raise a parser error? 

> Finally, RFC 2616 is deprecated. RFC 7230 and RFC 7231 have replaced it. =)

Oh dang, my bad.


Yeah, that would be ideal, but we don't own the parser: we use `httplib`. That's a source of ongoing frustration, but the upshot is that we have to do this validation higher up the stack, in requests itself, and the nature of the datastructures and relationships used make it somewhat tricky to do that.


Actually just been thinking some more about this, and perhaps it makes more sense for requests to follow what the majority of other libraries and browsers are doing? Merging these multiple headers is certainly not the correct behaviour, but I'm not sure what the alternative should be.

In use cases where requests is being used as a scraper, you'd expect requests to behave in the same way as the browser, this is despite the RFCs treating multiple location headers with the same severity as a missing header colon.

Some relevant details [here](https://bugzilla.mozilla.org/show_bug.cgi?id=655389), and [here](http://www.gossamer-threads.com/lists/nanog/users/175684).

Apparently chrome throws an error, as does Firefox.

```
Error code: ERR_RESPONSE_HEADERS_MULTIPLE_LOCATION 
```

If the major browsers are throwing errors, then perhaps requests should too? 

Thoughts?


Just tried this myself in Chrome;

```
➜  ~  nc -l 8080
GET / HTTP/1.1
Host: 127.0.0.1:8080
Connection: keep-alive
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8
Upgrade-Insecure-Requests: 1
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/47.0.2526.111 Safari/537.36
Accept-Encoding: gzip, deflate, sdch
Accept-Language: en-US,en;q=0.8

HTTP/1.1 200 OK
Content-Length: 0
Connection: Closed
Location: http://example.com
Location: http://example.com/2
Location: http://example.com/3
```

![image](https://cloud.githubusercontent.com/assets/651797/12616007/d680ce00-c500-11e5-811a-7d6357d18cb1.png)


That would be a 3.0 change but I'm amenable to that. @Lukasa, what about you?


I think that's what was already proposed here. =)


We should consider the possibility of raising this error only if we are following redirects (e.g. utilizing the `Location` header). If the `Location` header isn't going to trigger any automatic behavior in the client, then perhaps representing the (broken) multiple headers the way we do is okay, as it would allow the user to still access to resource, instead of making it impossible. 


Yup, the error would be raised in the `resolve_redirects` code?


@kennethreitz Yup agreed


This was fixed in 3.0 by #3417. I'm closing this as a result and tagging the correct milestone for it.