Remove Authorization header regardless of case when redirecting to cross-site #1511
Conversation
kyoshidajp
changed the title
kyoshidajp
force-pushed
the
support_case_insensitive_auth_header
branch
from
2354489
to
933d6b3
Compare
Codecov Report
@@ Coverage Diff @@
## master #1511 +/- ##
==========================================
- Coverage 67.04% 64.68% -2.36%
==========================================
Files 22 22
Lines 2761 2897 +136
==========================================
+ Hits 1851 1874 +23
- Misses 910 1023 +113
Continue to review full report at Codecov.
|
src/urllib3/util/retry.py
Outdated
| @@ -179,7 +179,7 @@ def __init__(self, total=10, connect=None, read=None, redirect=None, status=None | |||
| self.raise_on_status = raise_on_status | |||
| self.history = history or tuple() | |||
| self.respect_retry_after_header = respect_retry_after_header | |||
| self.remove_headers_on_redirect = remove_headers_on_redirect | |||
| self.remove_headers_on_redirect = [h.lower() for h in remove_headers_on_redirect] | |||
There was a problem hiding this comment.
I think we should change this structure into a frozenset to protect users from adding headers not through the constructor if we're going to enforce lowercase-only.
There was a problem hiding this comment.
You mean we should remove remove_headers_on_redirect parameter from the constructor?
There was a problem hiding this comment.
No, I mean converting the iterable we receive from remove_headers_on_redirect into a frozenset. Something like self.remove_headers_on_redirect = frozenset([h.lower() for h in remove_headers_on_redirect])
| @@ -342,8 +342,12 @@ def urlopen(self, method, url, redirect=True, **kw): | |||
| # conn.is_same_host() which may use socket.gethostbyname() in the future. | |||
| if (retries.remove_headers_on_redirect | |||
| and not conn.is_same_host(redirect_location)): | |||
| for header in retries.remove_headers_on_redirect: | |||
There was a problem hiding this comment.
Let's keep some of the original structure of this logic by instead looping through all the header names with six.viewkeys() (so we don't have to make a copy) and then seeing if that header name is within the remove_headers_on_redirect frozenset.
There was a problem hiding this comment.
I changed by 4357014. But made a copy by .copy() due to getting RuntimeError: dictionary changed size during iteration if without .copy().
|
Also don't worry about force-pushing, we squash all commits. :) |
|
I pushed some changes to your branch so we don't have to copy the entire set of headers, instead just the names. You might have to merge your changes if you also make changes to the same file without syncing your branch locally first. |
|
Thanks a lot! |
|
@sethmlarson Could you review again? |
|
Thanks for this! |
Fixes #1510.