From 3331e2aecdbf575dd60abef4df79c52d78610a83 Mon Sep 17 00:00:00 2001 From: Bruce Merry Date: Thu, 28 Jun 2018 16:38:42 +0200 Subject: [PATCH 1/5] Strip Authorization header whenever root URL changes Previously the header was stripped only if the hostname changed, but in an https -> http redirect that can leak the credentials on the wire (#4716). Based on with RFC 7235 section 2.2, the header is now stripped if the "canonical root URL" (scheme+authority) has changed, by checking scheme, hostname and port. --- requests/sessions.py | 4 +++- tests/test_requests.py | 12 +++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/requests/sessions.py b/requests/sessions.py index dd525e2a..702cd73e 100644 --- a/requests/sessions.py +++ b/requests/sessions.py @@ -242,7 +242,9 @@ class SessionRedirectMixin(object): original_parsed = urlparse(response.request.url) redirect_parsed = urlparse(url) - if (original_parsed.hostname != redirect_parsed.hostname): + if (original_parsed.hostname != redirect_parsed.hostname + or original_parsed.port != redirect_parsed.port + or original_parsed.scheme != redirect_parsed.scheme): del headers['Authorization'] # .netrc might have more auth for us on our new host. diff --git a/tests/test_requests.py b/tests/test_requests.py index fd04ad27..b05d8ebb 100644 --- a/tests/test_requests.py +++ b/tests/test_requests.py @@ -1581,7 +1581,17 @@ class TestRequests: auth=('user', 'pass'), ) assert r.history[0].request.headers['Authorization'] - assert not r.request.headers.get('Authorization', '') + assert 'Authorization' not in r.request.headers + + def test_auth_is_stripped_on_scheme_redirect(self, httpbin, httpbin_secure, httpbin_ca_bundle): + r = requests.get( + httpbin_secure('redirect-to'), + params={'url': httpbin('get')}, + auth=('user', 'pass'), + verify=httpbin_ca_bundle + ) + assert r.history[0].request.headers['Authorization'] + assert 'Authorization' not in r.request.headers def test_auth_is_retained_for_redirect_on_host(self, httpbin): r = requests.get(httpbin('redirect/1'), auth=('user', 'pass')) From 857e9b7ac20c3accf4cc328f594aecb8b6a644a6 Mon Sep 17 00:00:00 2001 From: Bruce Merry Date: Tue, 14 Aug 2018 13:30:43 +0200 Subject: [PATCH 2/5] 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. --- 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 702cd73e..27d0e971 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 b05d8ebb..66043798 100644 --- a/tests/test_requests.py +++ b/tests/test_requests.py @@ -1573,17 +1573,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')}, @@ -1600,6 +1590,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) From 5ef429efe38b2a65946c9fd4cedcd69cfbc05553 Mon Sep 17 00:00:00 2001 From: Vinayak Mehta Date: Fri, 14 Sep 2018 06:34:56 +0530 Subject: [PATCH 3/5] Fix broken essay link --- docs/dev/contributing.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/dev/contributing.rst b/docs/dev/contributing.rst index efc79bbf..06d7d6ce 100644 --- a/docs/dev/contributing.rst +++ b/docs/dev/contributing.rst @@ -41,7 +41,7 @@ including reporting bugs or requesting features. This golden rule is **All contributions are welcome**, as long as everyone involved is treated with respect. -.. _be cordial or be on your way: http://kennethreitz.org/be-cordial-or-be-on-your-way/ +.. _be cordial or be on your way: https://www.kennethreitz.org/essays/be-cordial-or-be-on-your-way .. _early-feedback: From b9dfc95cf236471f1b620d5152a800753d764315 Mon Sep 17 00:00:00 2001 From: juozasget <32190964+juozasget@users.noreply.github.com> Date: Sat, 15 Sep 2018 13:26:59 +0300 Subject: [PATCH 4/5] Tuple test_requirements was too long, PEP8 Broken a long line into multiple lines inside tuple parenthesis. --- setup.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/setup.py b/setup.py index 670a7dc7..76aba273 100755 --- a/setup.py +++ b/setup.py @@ -54,7 +54,14 @@ requires = [ 'certifi>=2017.4.17' ] -test_requirements = ['pytest-httpbin==0.0.7', 'pytest-cov', 'pytest-mock', 'pytest-xdist', 'PySocks>=1.5.6, !=1.5.7', 'pytest>=2.8.0'] +test_requirements = [ + 'pytest-httpbin==0.0.7', + 'pytest-cov', + 'pytest-mock', + 'pytest-xdist', + 'PySocks>=1.5.6, !=1.5.7', + 'pytest>=2.8.0' +] about = {} with open(os.path.join(here, 'requests', '__version__.py'), 'r', 'utf-8') as f: From 86914e2ddab3da351e484a3211406962ce1922c3 Mon Sep 17 00:00:00 2001 From: Kenneth Reitz Date: Mon, 17 Sep 2018 05:56:15 -0400 Subject: [PATCH 5/5] Update README.rst --- README.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.rst b/README.rst index af4ce7e7..bdb1d93b 100644 --- a/README.rst +++ b/README.rst @@ -1,6 +1,8 @@ Requests: HTTP for Humans ========================= +**If you're interested in supporting Kenneth Reitz projects financially, visit `this link `_. It really helps with motivation!** + .. image:: https://img.shields.io/pypi/v/requests.svg :target: https://pypi.org/project/requests/ @@ -45,6 +47,8 @@ See `the similar code, sans Requests `_. .. image:: https://raw.githubusercontent.com/requests/requests/master/docs/_static/requests-logo-small.png :target: http://docs.python-requests.org/ + + Requests allows you to send *organic, grass-fed* HTTP/1.1 requests, without the