raise InvalidHeader on multiple Location values#3417
raise InvalidHeader on multiple Location values#3417sigmavirus24 merged 3 commits intopsf:proposed/3.0.0from
Conversation
nateprewitt
force-pushed
the
2939_multiple_location_headers
branch
from
6bf0345 to
25da055
Compare
|
@nateprewitt Thanks for starting this! Unfortunately, quite a lot of people use non-urllib3 file-like objects to back requests, and I'd like to avoid breaking that if we can. Can you rearrange this to support urllib3 if possible but otherwise fallback to some other behaviour, per my comment. |
nateprewitt
force-pushed
the
2939_multiple_location_headers
branch
2 times, most recently
from
43ad63d to
25da055
Compare
requests/sessions.py
Outdated
| request = response.request | ||
|
|
||
| while response.is_redirect: | ||
| if len(response.raw.headers.getlist('location')) > 1: |
There was a problem hiding this comment.
This will need to include a getattr check. Frankly, it's a bit implementation detaily, so we may want to hide this in a utility function.
nateprewitt
force-pushed
the
2939_multiple_location_headers
branch
from
fc19fbb to
2b307de
Compare
|
This looks good to me. |
|
Sadly we have no CI for this though, so one or both of us should pull this down and test it. |
|
Sorry, @sigmavirus24, I just pushed a new copy with partially finished changes. I have a couple of questions on how we're expecting the code to function that I was going to annotate for discussion. |
requests/utils.py
Outdated
| elif hasattr(response.raw, 'read'): | ||
| response.raw.seek(0) # Move to beginning of file | ||
| data = response.raw.read() | ||
| # parse_headers(data) # try to parse headers out of raw response |
There was a problem hiding this comment.
Going through a few libraries that implement different adapters, the raw field seems considerably varied in it's use.
In the event we end up with something file-like, such as StringIO, we can parse the headers out, but only if it's the complete response. In the limited testing I did though, this was typically just the response body. I'm not sure if we can do anything useful in that case. Also if it is the response body and happens to contain contents mimicking an HTTP response, then we could in theory flag this incorrectly.
I'm just seeing a lot of unknowns here that I'm not sure have a clean fix. Perhaps just returning True when we can't make a reasonable discernment would be best.
There was a problem hiding this comment.
I think this is more than we need. If we can't get headers in the above case, don't worry about it.
Mostly what I'm concerned about here is not exploding in the non-urllib3 case: we don't need to be too clever, or even really "handle" it in any meaningful way.
There was a problem hiding this comment.
Ok, great, that helps narrow the scope considerably. I'll shorten this down to just catching the urllib3 case then. Thanks!
nateprewitt
force-pushed
the
2939_multiple_location_headers
branch
from
2b307de to
c7eb191
Compare
|
Ok, I think this is probably ready for another glance. I'm getting an |
nateprewitt
force-pushed
the
2939_multiple_location_headers
branch
from
c7eb191 to
7e42d65
Compare
requests/utils.py
Outdated
| returned from the last response. | ||
| """ | ||
| if hasattr(response.raw, 'headers'): | ||
| if hasattr(response.raw.headers, 'getlist'): |
There was a problem hiding this comment.
So we shouldn't use hasattr, we should use getattr. This is because of this.
There was a problem hiding this comment.
To be clear, I suggest the getattr pattern in this case.
There was a problem hiding this comment.
Per usual, I prefer the Python 2 behavior here hahaha
There was a problem hiding this comment.
Yeah, I'm onboard with this. I updated the PR with a somewhat verbose implementation of getattr.
nateprewitt
force-pushed
the
2939_multiple_location_headers
branch
from
7e42d65 to
c418c4c
Compare
|
Ok, I'm happy with this. @sigmavirus24? |
|
On a quick side note, I PR'd this against |
|
@nateprewitt Yeah, this is a breaking change. =) |
|
Hey @Lukasa @sigmavirus24, just wanted to check in on this. Let me know if you're waiting on anything from me. |
|
No problem from me. @sigmavirus24? |
|
Looks fine as a start. Thanks @nateprewitt @Lukasa since we're sans CI, have you pulled this and run the tests? |
|
@sigmavirus24 I haven't, would you like me to? |
|
I'll do it. Just wanted to make sure someone was doing it. |
|
It's still failing on #3442 because I haven't rebased on to master since the merge. Let me do that really quickly. |
|
@nateprewitt I merged the original branch into the head of proposed/3.0.0 and it passed just fine. No need to rebase |
|
Oh I see you already did, effectively invalidating the tests. Let me run them again |
|
Sorry, I'd already pushed it before the message. |
|
Tests passed both times. |
4 participants
Addresses issue raised in #2939 with a fix for multiple Location headers in response. This currently breaks on
test_requests_are_updated_each_timebecause theRedirectSessionpopulates it'srawattribute withStringIOrather than a urllib3HTTPResponse. I can convert the_build_rawmethod to fix the test, if this looks like a workable solution.