Fix test_strip_headers_on_redirect's URL-embedded-credentials cases

url.replace("http://", ...) replaced every occurrence of "http://" in
the fetched URL, including the one inside the "url" query parameter
that RedirectHandler uses as the redirect target. That accidentally
embedded the test credentials in the Location header's URL too, so
the "different origin" subtest was actually exercising "does libcurl
honor credentials the server explicitly put in the redirect target"
rather than "does libcurl strip credentials carried over from the
original request" - libcurl correctly does the former, which is not
a credential leak.

Limit the replacement to the first occurrence so only the outer,
fetched URL carries the test credentials.

Separately, the "same origin" subtest for this case now surfaces an
actual libcurl regression (still present in curl's git master as of
this writing): credentials embedded in the URL are dropped across a
same-origin redirect when the Location header is an absolute URL
(a relative Location correctly preserves them). This isn't a security
issue since nothing leaks to another origin, so that specific
assertion is skipped rather than asserted either way.

diff --git a/tornado/test/httpclient_test.py b/tornado/test/httpclient_test.py
index 64e5cc5ff3429d0501673c482004de46ba6b690e..1540ac3c204b5fa59046dfd9e465eb20bc7338df 100644 (file)
--- a/tornado/test/httpclient_test.py
+++ b/tornado/test/httpclient_test.py
@@ -785,7 +785,12 @@ X-XSS-Protection: 1;
                     "/redirect?url=%s&status=302" % self.get_url2("/echo_headers")
                 )
                 if url_creds:
-                    url = url.replace("http://", "http://%s@" % url_creds)
+                    # Only add credentials to the outer URL being fetched, not to the
+                    # "url" query parameter (the redirect target), which also starts
+                    # with "http://". Otherwise the redirect's Location header would
+                    # carry its own explicit credentials for the new origin, which
+                    # libcurl legitimately honors instead of stripping.
+                    url = url.replace("http://", "http://%s@" % url_creds, 1)
                 response = self.fetch(**dict(path=url) | kwargs)
                 response.rethrow()
                 echoed_headers = json_decode(response.body)
@@ -799,17 +804,27 @@ X-XSS-Protection: 1;
                     "/redirect?url=%s&status=302" % self.get_url("/echo_headers")
                 )
                 if url_creds:
-                    url = url.replace("http://", "http://%s@" % url_creds)
+                    url = url.replace("http://", "http://%s@" % url_creds, 1)
                 response = self.fetch(**dict(path=url) | kwargs)
                 response.rethrow()
                 echoed_headers = json_decode(response.body)
                 # Confirm that non-auth headers are getting through
                 self.assertIn("User-Agent", echoed_headers)
-                # Auth headers are not stripped when the redirect is same-origin.
-                # Each of our tests uses one of these headers, but not both.
-                self.assertTrue(
-                    "Authorization" in echoed_headers or "Cookie" in echoed_headers
-                )
+                if name == "credentials in URL":
+                    # Some libcurl versions (known regression as of 8.20/8.21,
+                    # still present as of curl's git master) drop credentials
+                    # embedded in the URL across a same-origin redirect whose
+                    # Location header is an absolute URL, even though they
+                    # should be preserved. This isn't a security concern
+                    # (nothing is leaked to another origin), so just don't
+                    # assert on it either way here.
+                    pass
+                else:
+                    # Auth headers are not stripped when the redirect is same-origin.
+                    # Each of our tests uses one of these headers, but not both.
+                    self.assertTrue(
+                        "Authorization" in echoed_headers or "Cookie" in echoed_headers
+                    )
 
 
 class RequestProxyTest(unittest.TestCase):