From f8d6c456e0669ea5344e93fe9206bd1ffebc2008 Mon Sep 17 00:00:00 2001 From: Tsubasa Umeuchi Date: Sat, 2 Mar 2024 14:02:27 +0900 Subject: [PATCH 1/3] Fixed logic for handling headers on redirect --- scrapy/downloadermiddlewares/redirect.py | 29 +++++++++++-- tests/test_downloadermiddleware_redirect.py | 47 ++++++++++++++++++++- 2 files changed, 71 insertions(+), 5 deletions(-) diff --git a/scrapy/downloadermiddlewares/redirect.py b/scrapy/downloadermiddlewares/redirect.py index 3176ed930..494c98271 100644 --- a/scrapy/downloadermiddlewares/redirect.py +++ b/scrapy/downloadermiddlewares/redirect.py @@ -20,14 +20,37 @@ def _build_redirect_request(source_request, *, url, **kwargs): has_cookie_header = "Cookie" in redirect_request.headers has_authorization_header = "Authorization" in redirect_request.headers if has_cookie_header or has_authorization_header: - source_request_netloc = urlparse_cached(source_request).netloc - redirect_request_netloc = urlparse_cached(redirect_request).netloc - if source_request_netloc != redirect_request_netloc: + default_ports = {"http": 80, "https": 443} + + parsed_source_request = urlparse_cached(source_request) + source_scheme, source_host, source_port = ( + parsed_source_request.scheme, + parsed_source_request.hostname, + parsed_source_request.port + or default_ports.get(parsed_source_request.scheme), + ) + + parsed_redirect_request = urlparse_cached(redirect_request) + redirect_scheme, redirect_host, redirect_port = ( + parsed_redirect_request.scheme, + parsed_redirect_request.hostname, + parsed_redirect_request.port + or default_ports.get(parsed_redirect_request.scheme), + ) + + if source_scheme != redirect_scheme or source_host != redirect_host: if has_cookie_header: del redirect_request.headers["Cookie"] + + if ( + source_scheme != redirect_scheme + or source_host != redirect_host + or source_port != redirect_port + ): # https://fetch.spec.whatwg.org/#ref-for-cors-non-wildcard-request-header-name if has_authorization_header: del redirect_request.headers["Authorization"] + return redirect_request diff --git a/tests/test_downloadermiddleware_redirect.py b/tests/test_downloadermiddleware_redirect.py index 10b8ca9af..5292a02ba 100644 --- a/tests/test_downloadermiddleware_redirect.py +++ b/tests/test_downloadermiddleware_redirect.py @@ -247,11 +247,14 @@ class RedirectMiddlewareTest(unittest.TestCase): perc_encoded_utf8_url = "http://scrapytest.org/a%C3%A7%C3%A3o" self.assertEqual(perc_encoded_utf8_url, req_result.url) - def test_cross_domain_header_dropping(self): + def test_cross_origin_header_dropping(self): safe_headers = {"A": "B"} + cookie_header = {"Cookie": "a=b"} + authorization_header = {"Authorization": "Bearer 123456"} + original_request = Request( "https://example.com", - headers={"Cookie": "a=b", "Authorization": "a", **safe_headers}, + headers={**safe_headers, **cookie_header, **authorization_header}, ) internal_response = Response( @@ -265,6 +268,33 @@ class RedirectMiddlewareTest(unittest.TestCase): self.assertIsInstance(internal_redirect_request, Request) self.assertEqual(original_request.headers, internal_redirect_request.headers) + default_port_response = Response( + "https://example.com", + headers={"Location": "https://example.com:443/a"}, + status=301, + ) + default_port_redirect_request = self.mw.process_response( + original_request, default_port_response, self.spider + ) + self.assertIsInstance(default_port_redirect_request, Request) + self.assertEqual( + original_request.headers, default_port_redirect_request.headers + ) + + different_port_response = Response( + "https://example.com", + headers={"Location": "https://example.com:8080/a"}, + status=301, + ) + different_port_redirect_request = self.mw.process_response( + original_request, different_port_response, self.spider + ) + self.assertIsInstance(different_port_redirect_request, Request) + self.assertEqual( + {**safe_headers, **cookie_header}, + different_port_redirect_request.headers.to_unicode_dict(), + ) + external_response = Response( "https://example.com", headers={"Location": "https://example.org/a"}, @@ -278,6 +308,19 @@ class RedirectMiddlewareTest(unittest.TestCase): safe_headers, external_redirect_request.headers.to_unicode_dict() ) + downgrade_response = Response( + "https://example.com", + headers={"Location": "http://example.com/a"}, + status=301, + ) + downgrade_redirect_request = self.mw.process_response( + original_request, downgrade_response, self.spider + ) + self.assertIsInstance(downgrade_redirect_request, Request) + self.assertEqual( + safe_headers, downgrade_redirect_request.headers.to_unicode_dict() + ) + class MetaRefreshMiddlewareTest(unittest.TestCase): def setUp(self): From 6499214a4f6817e1845073bd167deb33ed5261af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Chaves?= Date: Wed, 6 Mar 2024 15:40:13 +0100 Subject: [PATCH 2/3] Keep the Cookie header on scheme upgrades --- scrapy/downloadermiddlewares/redirect.py | 2 +- tests/test_downloadermiddleware_redirect.py | 66 +++++++++++++++++++-- 2 files changed, 61 insertions(+), 7 deletions(-) diff --git a/scrapy/downloadermiddlewares/redirect.py b/scrapy/downloadermiddlewares/redirect.py index 494c98271..a26715e12 100644 --- a/scrapy/downloadermiddlewares/redirect.py +++ b/scrapy/downloadermiddlewares/redirect.py @@ -38,7 +38,7 @@ def _build_redirect_request(source_request, *, url, **kwargs): or default_ports.get(parsed_redirect_request.scheme), ) - if source_scheme != redirect_scheme or source_host != redirect_host: + if redirect_scheme != "https" or source_host != redirect_host: if has_cookie_header: del redirect_request.headers["Cookie"] diff --git a/tests/test_downloadermiddleware_redirect.py b/tests/test_downloadermiddleware_redirect.py index 5292a02ba..c14f87d91 100644 --- a/tests/test_downloadermiddleware_redirect.py +++ b/tests/test_downloadermiddleware_redirect.py @@ -257,6 +257,8 @@ class RedirectMiddlewareTest(unittest.TestCase): headers={**safe_headers, **cookie_header, **authorization_header}, ) + # Redirects to the same origin (same scheme, same domain, same port) + # keep all headers. internal_response = Response( "https://example.com", headers={"Location": "https://example.com/a"}, @@ -268,19 +270,39 @@ class RedirectMiddlewareTest(unittest.TestCase): self.assertIsInstance(internal_redirect_request, Request) self.assertEqual(original_request.headers, internal_redirect_request.headers) - default_port_response = Response( + # For default ports, whether the port is explicit or implicit does not + # affect the outcome, it is still the same origin. + to_explicit_port_response = Response( "https://example.com", headers={"Location": "https://example.com:443/a"}, status=301, ) - default_port_redirect_request = self.mw.process_response( - original_request, default_port_response, self.spider + to_explicit_port_redirect_request = self.mw.process_response( + original_request, to_explicit_port_response, self.spider ) - self.assertIsInstance(default_port_redirect_request, Request) + self.assertIsInstance(to_explicit_port_redirect_request, Request) self.assertEqual( - original_request.headers, default_port_redirect_request.headers + original_request.headers, to_explicit_port_redirect_request.headers ) + # For default ports, whether the port is explicit or implicit does not + # affect the outcome, it is still the same origin. + to_implicit_port_response = Response( + "https://example.com:433", + headers={"Location": "https://example.com/a"}, + status=301, + ) + to_implicit_port_redirect_request = self.mw.process_response( + original_request, to_implicit_port_response, self.spider + ) + self.assertIsInstance(to_implicit_port_redirect_request, Request) + self.assertEqual( + original_request.headers, to_implicit_port_redirect_request.headers + ) + + # A port change drops the Authorization header because the origin + # changes, but keeps the Cookie header because the domain remains the + # same. different_port_response = Response( "https://example.com", headers={"Location": "https://example.com:8080/a"}, @@ -295,6 +317,7 @@ class RedirectMiddlewareTest(unittest.TestCase): different_port_redirect_request.headers.to_unicode_dict(), ) + # A domain change drops both the Authorization and the Cookie header. external_response = Response( "https://example.com", headers={"Location": "https://example.org/a"}, @@ -308,6 +331,36 @@ class RedirectMiddlewareTest(unittest.TestCase): safe_headers, external_redirect_request.headers.to_unicode_dict() ) + # A scheme upgrade (http → https) drops the Authorization header + # because the origin changes, but keeps the Cookie header because the + # domain remains the same. + http_request = Request( + "http://example.com", + headers={**safe_headers, **cookie_header, **authorization_header}, + ) + upgrade_response = Response( + "http://example.com", + headers={"Location": "https://example.com/a"}, + status=301, + ) + upgrade_redirect_request = self.mw.process_response( + http_request, upgrade_response, self.spider + ) + self.assertIsInstance(upgrade_redirect_request, Request) + self.assertEqual( + {**safe_headers, **cookie_header}, + upgrade_redirect_request.headers.to_unicode_dict(), + ) + + # A scheme downgrade (https → http) drops the Authorization header + # because the origin changes, and the Cookie header because its value + # cannot indicate whether the cookies were secure (HTTPS-only) or not. + # + # Note: If the Cookie header is set by the cookie management + # middleware, as recommended in the docs, the dropping of Cookie on + # scheme downgrade is not an issue, because the cookie management + # middleware will add again the Cookie header to the new request if + # appropriate. downgrade_response = Response( "https://example.com", headers={"Location": "http://example.com/a"}, @@ -318,7 +371,8 @@ class RedirectMiddlewareTest(unittest.TestCase): ) self.assertIsInstance(downgrade_redirect_request, Request) self.assertEqual( - safe_headers, downgrade_redirect_request.headers.to_unicode_dict() + safe_headers, + downgrade_redirect_request.headers.to_unicode_dict(), ) From 7a1ab7e1be2187daf047f3bf5ed8e9192751b145 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Chaves?= Date: Wed, 6 Mar 2024 16:19:19 +0100 Subject: [PATCH 3/3] =?UTF-8?q?Do=20not=20drop=20Cookie=20on=20http=20?= =?UTF-8?q?=E2=86=92=20http?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- scrapy/downloadermiddlewares/redirect.py | 15 ++++++++------- tests/test_downloadermiddleware_redirect.py | 21 +++++++++++++++++---- 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/scrapy/downloadermiddlewares/redirect.py b/scrapy/downloadermiddlewares/redirect.py index a26715e12..36bd9a849 100644 --- a/scrapy/downloadermiddlewares/redirect.py +++ b/scrapy/downloadermiddlewares/redirect.py @@ -38,18 +38,19 @@ def _build_redirect_request(source_request, *, url, **kwargs): or default_ports.get(parsed_redirect_request.scheme), ) - if redirect_scheme != "https" or source_host != redirect_host: - if has_cookie_header: - del redirect_request.headers["Cookie"] + if has_cookie_header and ( + (source_scheme != redirect_scheme and redirect_scheme != "https") + or source_host != redirect_host + ): + del redirect_request.headers["Cookie"] - if ( + # https://fetch.spec.whatwg.org/#ref-for-cors-non-wildcard-request-header-name + if has_authorization_header and ( source_scheme != redirect_scheme or source_host != redirect_host or source_port != redirect_port ): - # https://fetch.spec.whatwg.org/#ref-for-cors-non-wildcard-request-header-name - if has_authorization_header: - del redirect_request.headers["Authorization"] + del redirect_request.headers["Authorization"] return redirect_request diff --git a/tests/test_downloadermiddleware_redirect.py b/tests/test_downloadermiddleware_redirect.py index c14f87d91..3b0b910ee 100644 --- a/tests/test_downloadermiddleware_redirect.py +++ b/tests/test_downloadermiddleware_redirect.py @@ -270,6 +270,23 @@ class RedirectMiddlewareTest(unittest.TestCase): self.assertIsInstance(internal_redirect_request, Request) self.assertEqual(original_request.headers, internal_redirect_request.headers) + # Redirects to the same origin (same scheme, same domain, same port) + # keep all headers also when the scheme is http. + http_request = Request( + "http://example.com", + headers={**safe_headers, **cookie_header, **authorization_header}, + ) + http_response = Response( + "http://example.com", + headers={"Location": "http://example.com/a"}, + status=301, + ) + http_redirect_request = self.mw.process_response( + http_request, http_response, self.spider + ) + self.assertIsInstance(http_redirect_request, Request) + self.assertEqual(http_request.headers, http_redirect_request.headers) + # For default ports, whether the port is explicit or implicit does not # affect the outcome, it is still the same origin. to_explicit_port_response = Response( @@ -334,10 +351,6 @@ class RedirectMiddlewareTest(unittest.TestCase): # A scheme upgrade (http → https) drops the Authorization header # because the origin changes, but keeps the Cookie header because the # domain remains the same. - http_request = Request( - "http://example.com", - headers={**safe_headers, **cookie_header, **authorization_header}, - ) upgrade_response = Response( "http://example.com", headers={"Location": "https://example.com/a"},