| From 698c2fa850bfc8b3bdb768e1c1cd6d57e643811d Mon Sep 17 00:00:00 2001 |
| From: Bruce Merry <bmerry@ska.ac.za> |
| Date: Tue, 14 Aug 2018 13:30:43 +0200 |
| Subject: [PATCH 2/2] Rework authorization stripping logic as discussed |
| |
| The exception for http->https upgrade now requires the standard HTTP(S) |
| ports to be used, either implicitly (no port specified) or explicitly. |
| |
| Upstream-Status: Backport |
| |
| Follow-up fix for CVE-2018-18074 |
| |
| Signed-off-by: Chen Qi <Qi.Chen@windriver.com> |
| --- |
| requests/sessions.py | 26 ++++++++++++++++++-------- |
| tests/test_requests.py | 33 ++++++++++++++++++++++----------- |
| 2 files changed, 40 insertions(+), 19 deletions(-) |
| |
| diff --git a/requests/sessions.py b/requests/sessions.py |
| index 2969d83..c11a3a2 100644 |
| --- a/requests/sessions.py |
| +++ b/requests/sessions.py |
| @@ -115,6 +115,22 @@ class SessionRedirectMixin(object): |
| return to_native_string(location, 'utf8') |
| return None |
| |
| + def should_strip_auth(self, old_url, new_url): |
| + """Decide whether Authorization header should be removed when redirecting""" |
| + old_parsed = urlparse(old_url) |
| + new_parsed = urlparse(new_url) |
| + if old_parsed.hostname != new_parsed.hostname: |
| + return True |
| + # Special case: allow http -> https redirect when using the standard |
| + # ports. This isn't specified by RFC 7235, but is kept to avoid |
| + # breaking backwards compatibility with older versions of requests |
| + # that allowed any redirects on the same host. |
| + if (old_parsed.scheme == 'http' and old_parsed.port in (80, None) |
| + and new_parsed.scheme == 'https' and new_parsed.port in (443, None)): |
| + return False |
| + # Standard case: root URI must match |
| + return old_parsed.port != new_parsed.port or old_parsed.scheme != new_parsed.scheme |
| + |
| def resolve_redirects(self, resp, req, stream=False, timeout=None, |
| verify=True, cert=None, proxies=None, yield_requests=False, **adapter_kwargs): |
| """Receives a Response. Returns a generator of Responses or Requests.""" |
| @@ -236,16 +252,10 @@ class SessionRedirectMixin(object): |
| headers = prepared_request.headers |
| url = prepared_request.url |
| |
| - if 'Authorization' in headers: |
| + if 'Authorization' in headers and self.should_strip_auth(response.request.url, url): |
| # If we get redirected to a new host, we should strip out any |
| # authentication headers. |
| - original_parsed = urlparse(response.request.url) |
| - redirect_parsed = urlparse(url) |
| - |
| - if (original_parsed.hostname != redirect_parsed.hostname |
| - or original_parsed.port != redirect_parsed.port |
| - or original_parsed.scheme != redirect_parsed.scheme): |
| - del headers['Authorization'] |
| + del headers['Authorization'] |
| |
| # .netrc might have more auth for us on our new host. |
| new_auth = get_netrc_auth(url) if self.trust_env else None |
| diff --git a/tests/test_requests.py b/tests/test_requests.py |
| index e0e801a..148067b 100644 |
| --- a/tests/test_requests.py |
| +++ b/tests/test_requests.py |
| @@ -1567,17 +1567,7 @@ class TestRequests: |
| preq = req.prepare() |
| assert test_url == preq.url |
| |
| - @pytest.mark.xfail(raises=ConnectionError) |
| - def test_auth_is_stripped_on_redirect_off_host(self, httpbin): |
| - r = requests.get( |
| - httpbin('redirect-to'), |
| - params={'url': 'http://www.google.co.uk'}, |
| - auth=('user', 'pass'), |
| - ) |
| - assert r.history[0].request.headers['Authorization'] |
| - assert 'Authorization' not in r.request.headers |
| - |
| - def test_auth_is_stripped_on_scheme_redirect(self, httpbin, httpbin_secure, httpbin_ca_bundle): |
| + def test_auth_is_stripped_on_http_downgrade(self, httpbin, httpbin_secure, httpbin_ca_bundle): |
| r = requests.get( |
| httpbin_secure('redirect-to'), |
| params={'url': httpbin('get')}, |
| @@ -1594,6 +1584,27 @@ class TestRequests: |
| |
| assert h1 == h2 |
| |
| + def test_should_strip_auth_host_change(self): |
| + s = requests.Session() |
| + assert s.should_strip_auth('http://example.com/foo', 'http://another.example.com/') |
| + |
| + def test_should_strip_auth_http_downgrade(self): |
| + s = requests.Session() |
| + assert s.should_strip_auth('https://example.com/foo', 'http://example.com/bar') |
| + |
| + def test_should_strip_auth_https_upgrade(self): |
| + s = requests.Session() |
| + assert not s.should_strip_auth('http://example.com/foo', 'https://example.com/bar') |
| + assert not s.should_strip_auth('http://example.com:80/foo', 'https://example.com/bar') |
| + assert not s.should_strip_auth('http://example.com/foo', 'https://example.com:443/bar') |
| + # Non-standard ports should trigger stripping |
| + assert s.should_strip_auth('http://example.com:8080/foo', 'https://example.com/bar') |
| + assert s.should_strip_auth('http://example.com/foo', 'https://example.com:8443/bar') |
| + |
| + def test_should_strip_auth_port_change(self): |
| + s = requests.Session() |
| + assert s.should_strip_auth('http://example.com:1234/foo', 'https://example.com:4321/bar') |
| + |
| def test_manual_redirect_with_partial_body_read(self, httpbin): |
| s = requests.Session() |
| r1 = s.get(httpbin('redirect/2'), allow_redirects=False, stream=True) |
| -- |
| 2.7.4 |
| |