blob: ef069fb97b2ceceb224406082ef383466ba9f88e [file] [log] [blame]
Brad Bishop1a4b7ee2018-12-16 17:11:34 -08001From 698c2fa850bfc8b3bdb768e1c1cd6d57e643811d Mon Sep 17 00:00:00 2001
2From: Bruce Merry <bmerry@ska.ac.za>
3Date: Tue, 14 Aug 2018 13:30:43 +0200
4Subject: [PATCH 2/2] Rework authorization stripping logic as discussed
5
6The exception for http->https upgrade now requires the standard HTTP(S)
7ports to be used, either implicitly (no port specified) or explicitly.
8
9Upstream-Status: Backport
10
11Follow-up fix for CVE-2018-18074
12
13Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
14---
15 requests/sessions.py | 26 ++++++++++++++++++--------
16 tests/test_requests.py | 33 ++++++++++++++++++++++-----------
17 2 files changed, 40 insertions(+), 19 deletions(-)
18
19diff --git a/requests/sessions.py b/requests/sessions.py
20index 2969d83..c11a3a2 100644
21--- a/requests/sessions.py
22+++ b/requests/sessions.py
23@@ -115,6 +115,22 @@ class SessionRedirectMixin(object):
24 return to_native_string(location, 'utf8')
25 return None
26
27+ def should_strip_auth(self, old_url, new_url):
28+ """Decide whether Authorization header should be removed when redirecting"""
29+ old_parsed = urlparse(old_url)
30+ new_parsed = urlparse(new_url)
31+ if old_parsed.hostname != new_parsed.hostname:
32+ return True
33+ # Special case: allow http -> https redirect when using the standard
34+ # ports. This isn't specified by RFC 7235, but is kept to avoid
35+ # breaking backwards compatibility with older versions of requests
36+ # that allowed any redirects on the same host.
37+ if (old_parsed.scheme == 'http' and old_parsed.port in (80, None)
38+ and new_parsed.scheme == 'https' and new_parsed.port in (443, None)):
39+ return False
40+ # Standard case: root URI must match
41+ return old_parsed.port != new_parsed.port or old_parsed.scheme != new_parsed.scheme
42+
43 def resolve_redirects(self, resp, req, stream=False, timeout=None,
44 verify=True, cert=None, proxies=None, yield_requests=False, **adapter_kwargs):
45 """Receives a Response. Returns a generator of Responses or Requests."""
46@@ -236,16 +252,10 @@ class SessionRedirectMixin(object):
47 headers = prepared_request.headers
48 url = prepared_request.url
49
50- if 'Authorization' in headers:
51+ if 'Authorization' in headers and self.should_strip_auth(response.request.url, url):
52 # If we get redirected to a new host, we should strip out any
53 # authentication headers.
54- original_parsed = urlparse(response.request.url)
55- redirect_parsed = urlparse(url)
56-
57- if (original_parsed.hostname != redirect_parsed.hostname
58- or original_parsed.port != redirect_parsed.port
59- or original_parsed.scheme != redirect_parsed.scheme):
60- del headers['Authorization']
61+ del headers['Authorization']
62
63 # .netrc might have more auth for us on our new host.
64 new_auth = get_netrc_auth(url) if self.trust_env else None
65diff --git a/tests/test_requests.py b/tests/test_requests.py
66index e0e801a..148067b 100644
67--- a/tests/test_requests.py
68+++ b/tests/test_requests.py
69@@ -1567,17 +1567,7 @@ class TestRequests:
70 preq = req.prepare()
71 assert test_url == preq.url
72
73- @pytest.mark.xfail(raises=ConnectionError)
74- def test_auth_is_stripped_on_redirect_off_host(self, httpbin):
75- r = requests.get(
76- httpbin('redirect-to'),
77- params={'url': 'http://www.google.co.uk'},
78- auth=('user', 'pass'),
79- )
80- assert r.history[0].request.headers['Authorization']
81- assert 'Authorization' not in r.request.headers
82-
83- def test_auth_is_stripped_on_scheme_redirect(self, httpbin, httpbin_secure, httpbin_ca_bundle):
84+ def test_auth_is_stripped_on_http_downgrade(self, httpbin, httpbin_secure, httpbin_ca_bundle):
85 r = requests.get(
86 httpbin_secure('redirect-to'),
87 params={'url': httpbin('get')},
88@@ -1594,6 +1584,27 @@ class TestRequests:
89
90 assert h1 == h2
91
92+ def test_should_strip_auth_host_change(self):
93+ s = requests.Session()
94+ assert s.should_strip_auth('http://example.com/foo', 'http://another.example.com/')
95+
96+ def test_should_strip_auth_http_downgrade(self):
97+ s = requests.Session()
98+ assert s.should_strip_auth('https://example.com/foo', 'http://example.com/bar')
99+
100+ def test_should_strip_auth_https_upgrade(self):
101+ s = requests.Session()
102+ assert not s.should_strip_auth('http://example.com/foo', 'https://example.com/bar')
103+ assert not s.should_strip_auth('http://example.com:80/foo', 'https://example.com/bar')
104+ assert not s.should_strip_auth('http://example.com/foo', 'https://example.com:443/bar')
105+ # Non-standard ports should trigger stripping
106+ assert s.should_strip_auth('http://example.com:8080/foo', 'https://example.com/bar')
107+ assert s.should_strip_auth('http://example.com/foo', 'https://example.com:8443/bar')
108+
109+ def test_should_strip_auth_port_change(self):
110+ s = requests.Session()
111+ assert s.should_strip_auth('http://example.com:1234/foo', 'https://example.com:4321/bar')
112+
113 def test_manual_redirect_with_partial_body_read(self, httpbin):
114 s = requests.Session()
115 r1 = s.get(httpbin('redirect/2'), allow_redirects=False, stream=True)
116--
1172.7.4
118