From fada8be1db6479e235d7e06afa057632dbc2adf1 Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Wed, 17 Jun 2026 14:56:22 +0500 Subject: [PATCH] Fix Proxy-Authorization handling in BaseStreamingDownloadHandler (#7630) --- conftest.py | 37 +---- .../downloader/handlers/_base_streaming.py | 16 +- scrapy/core/downloader/handlers/_httpx.py | 3 +- tests/test_downloader_handlers_http_base.py | 152 +++++++++--------- 4 files changed, 96 insertions(+), 112 deletions(-) diff --git a/conftest.py b/conftest.py index cf0568111..532f83f56 100644 --- a/conftest.py +++ b/conftest.py @@ -74,40 +74,19 @@ def mockserver() -> Generator[MockServer]: @pytest.fixture # function scope because it modifies os.environ -def mitm_proxy_server(monkeypatch: pytest.MonkeyPatch) -> Generator[MitmProxy]: - proxy = MitmProxy() +def proxy_server( + request: pytest.FixtureRequest, monkeypatch: pytest.MonkeyPatch +) -> Generator[str]: + kind = request.param + proxy = MitmProxy(mode="socks5" if kind == "socks5" else None) url = proxy.start() + if kind == "https": + url = url.replace("http://", "https://") monkeypatch.setenv("http_proxy", url) monkeypatch.setenv("https_proxy", url) try: - yield proxy - finally: - proxy.stop() - - -@pytest.fixture # function scope because it modifies os.environ -def mitm_proxy_server_https(monkeypatch: pytest.MonkeyPatch) -> Generator[MitmProxy]: - proxy = MitmProxy() - url = proxy.start().replace("http://", "https://") - monkeypatch.setenv("http_proxy", url) - monkeypatch.setenv("https_proxy", url) - - try: - yield proxy - finally: - proxy.stop() - - -@pytest.fixture # function scope because it modifies os.environ -def socks5_proxy_server(monkeypatch: pytest.MonkeyPatch) -> Generator[MitmProxy]: - proxy = MitmProxy(mode="socks5") - url = proxy.start() - monkeypatch.setenv("http_proxy", url) - monkeypatch.setenv("https_proxy", url) - - try: - yield proxy + yield kind finally: proxy.stop() diff --git a/scrapy/core/downloader/handlers/_base_streaming.py b/scrapy/core/downloader/handlers/_base_streaming.py index 16b655716..5a669c732 100644 --- a/scrapy/core/downloader/handlers/_base_streaming.py +++ b/scrapy/core/downloader/handlers/_base_streaming.py @@ -241,6 +241,16 @@ class BaseStreamingDownloadHandler(BaseHttpDownloadHandler, ABC, Generic[_Respon body=response_body.getvalue(), ) + @staticmethod + def _request_headers(request: Request) -> Headers: + """Get a prepared copy of the request headers. + + This removes the Proxy-Authorization header. + """ + headers = request.headers.copy() + headers.pop(b"Proxy-Authorization", None) + return headers + def _get_bind_address_host(self) -> str | None: """Return the host portion of the bind address. @@ -279,10 +289,8 @@ class BaseStreamingDownloadHandler(BaseHttpDownloadHandler, ABC, Generic[_Respon if not proxy: return None, None proxy = add_http_if_no_scheme(proxy) - auth_header: list[bytes] | None = request.headers.pop( - b"Proxy-Authorization", None - ) - return proxy, auth_header[0].decode("ascii") if auth_header else None + auth_header: bytes | None = request.headers.get(b"Proxy-Authorization") + return proxy, auth_header.decode("ascii") if auth_header else None def _extract_proxy_url_with_creds(self, request: Request) -> str | None: """Return the proxy URL with the userinfo added based on the diff --git a/scrapy/core/downloader/handlers/_httpx.py b/scrapy/core/downloader/handlers/_httpx.py index c960fc152..9b54b44d8 100644 --- a/scrapy/core/downloader/handlers/_httpx.py +++ b/scrapy/core/downloader/handlers/_httpx.py @@ -152,13 +152,14 @@ class HttpxDownloadHandler(_Base): f"SOCKS proxy support in {type(self).__name__} requires the 'httpx[socks]' extra to be installed." ) client = self._get_client(proxy) + headers = self._request_headers(request).to_tuple_list() try: async with client.stream( request.method, request.url, content=request.body, - headers=request.headers.to_tuple_list(), + headers=headers, timeout=timeout, ) as response: yield response diff --git a/tests/test_downloader_handlers_http_base.py b/tests/test_downloader_handlers_http_base.py index 304ef9f2b..2ae9206a0 100644 --- a/tests/test_downloader_handlers_http_base.py +++ b/tests/test_downloader_handlers_http_base.py @@ -1328,6 +1328,9 @@ class TestHttpProxyBase(ABC): assert response.body == self.expected_http_proxy_request_body +PROXY_KINDS = ["http", "https", "socks5"] + + class TestMitmProxyBase(ABC): # whether the handler supports HTTPS proxies with HTTPS destinations handler_supports_tls_in_tls: bool = True @@ -1338,57 +1341,54 @@ class TestMitmProxyBase(ABC): def settings_dict(self) -> dict[str, Any] | None: raise NotImplementedError - @pytest.mark.parametrize( - "https_dest", [False, True], ids=["HTTP dest", "HTTPS dest"] - ) - @pytest.mark.usefixtures("mitm_proxy_server") - @coroutine_test - async def test_http_proxy( - self, caplog: pytest.LogCaptureFixture, mockserver: MockServer, https_dest: bool - ) -> None: - """HTTP proxy, HTTP or HTTPS destination.""" - crawler = get_crawler(SingleRequestSpider, self.settings_dict) - with caplog.at_level(logging.DEBUG): - await crawler.crawl_async( - seed=mockserver.url("/status?n=200", is_secure=https_dest) - ) - assert isinstance(crawler.spider, SingleRequestSpider) - self._assert_got_response_code(200, caplog.text) - self._assert_headers(crawler.spider.meta["responses"][0].headers, https_dest) - - @pytest.mark.parametrize( - "https_dest", [False, True], ids=["HTTP dest", "HTTPS dest"] - ) - @pytest.mark.usefixtures("mitm_proxy_server_https") - @coroutine_test - async def test_https_proxy( - self, caplog: pytest.LogCaptureFixture, mockserver: MockServer, https_dest: bool - ) -> None: - """HTTPS proxy, HTTP or HTTPS destination.""" - if https_dest and not self.handler_supports_tls_in_tls: + def _maybe_skip(self, proxy_kind: str, https_dest: bool) -> None: + if proxy_kind == "socks5" and not self.handler_supports_socks: + pytest.skip("SOCKS proxies are not supported") + if ( + proxy_kind == "https" + and https_dest + and not self.handler_supports_tls_in_tls + ): pytest.skip("HTTPS proxies for HTTPS destinations are not supported") - crawler = get_crawler(SingleRequestSpider, self.settings_dict) - with caplog.at_level(logging.DEBUG): - await crawler.crawl_async( - seed=mockserver.url("/status?n=200", is_secure=https_dest) - ) - assert isinstance(crawler.spider, SingleRequestSpider) - self._assert_got_response_code(200, caplog.text) - self._assert_headers(crawler.spider.meta["responses"][0].headers, https_dest) + @pytest.mark.parametrize("proxy_server", PROXY_KINDS, indirect=True) @pytest.mark.parametrize( "https_dest", [False, True], ids=["HTTP dest", "HTTPS dest"] ) - @pytest.mark.usefixtures("mitm_proxy_server") @coroutine_test - async def test_http_proxy_auth_error( + async def test_proxy( self, caplog: pytest.LogCaptureFixture, - monkeypatch: pytest.MonkeyPatch, + proxy_server: str, mockserver: MockServer, https_dest: bool, ) -> None: - """HTTP proxy, HTTP or HTTPS destination, wrong proxy creds.""" + """HTTP/HTTPS/SOCKS5 proxy, HTTP or HTTPS destination.""" + self._maybe_skip(proxy_server, https_dest) + crawler = get_crawler(SingleRequestSpider, self.settings_dict) + with caplog.at_level(logging.DEBUG): + await crawler.crawl_async( + seed=mockserver.url("/status?n=200", is_secure=https_dest) + ) + assert isinstance(crawler.spider, SingleRequestSpider) + self._assert_got_response_code(200, caplog.text) + self._assert_headers(crawler.spider.meta["responses"][0].headers, https_dest) + + @pytest.mark.parametrize("proxy_server", PROXY_KINDS, indirect=True) + @pytest.mark.parametrize( + "https_dest", [False, True], ids=["HTTP dest", "HTTPS dest"] + ) + @coroutine_test + async def test_proxy_auth_error( + self, + caplog: pytest.LogCaptureFixture, + monkeypatch: pytest.MonkeyPatch, + proxy_server: str, + mockserver: MockServer, + https_dest: bool, + ) -> None: + """HTTP/HTTPS/SOCKS5 proxy, HTTP or HTTPS destination, wrong proxy creds.""" + self._maybe_skip(proxy_server, https_dest) envvar = "https_proxy" if https_dest else "http_proxy" monkeypatch.setenv(envvar, wrong_credentials(os.environ[envvar])) crawler = get_crawler(SimpleSpider, self.settings_dict) @@ -1396,20 +1396,28 @@ class TestMitmProxyBase(ABC): await crawler.crawl_async( mockserver.url("/status?n=200", is_secure=https_dest) ) - # The proxy returns a 407 error code but it does not reach the client; - # it just sees an exception. - self._assert_got_auth_exception(caplog.text) + if proxy_server == "socks5": + assert "DownloadConnectionRefusedError" in caplog.text + else: + # The proxy returns a 407 error code but it does not reach the + # client; it just sees an exception. + self._assert_got_auth_exception(caplog.text) + @pytest.mark.parametrize("proxy_server", PROXY_KINDS, indirect=True) @pytest.mark.parametrize( "https_dest", [False, True], ids=["HTTP dest", "HTTPS dest"] ) - @pytest.mark.usefixtures("mitm_proxy_server") @coroutine_test - async def test_dont_leak_proxy_authorization_header( - self, caplog: pytest.LogCaptureFixture, mockserver: MockServer, https_dest: bool + async def test_proxy_dont_leak_auth_header( + self, + caplog: pytest.LogCaptureFixture, + proxy_server: str, + mockserver: MockServer, + https_dest: bool, ) -> None: - """HTTP proxy, HTTP or HTTPS destination. Check that the auth header - is not sent to the destination.""" + """HTTP/HTTPS/SOCKS5 proxy, HTTP or HTTPS destination. Check that the + auth header is not sent to the destination.""" + self._maybe_skip(proxy_server, https_dest) request = Request(mockserver.url("/echo", is_secure=https_dest)) crawler = get_crawler(SingleRequestSpider, self.settings_dict) with caplog.at_level(logging.DEBUG): @@ -1420,48 +1428,36 @@ class TestMitmProxyBase(ABC): echo = json.loads(crawler.spider.meta["responses"][0].text) assert "Proxy-Authorization" not in echo["headers"] + @pytest.mark.parametrize("proxy_server", PROXY_KINDS, indirect=True) @pytest.mark.parametrize( "https_dest", [False, True], ids=["HTTP dest", "HTTPS dest"] ) - @pytest.mark.usefixtures("socks5_proxy_server") @coroutine_test - async def test_download_with_socks_proxy( - self, caplog: pytest.LogCaptureFixture, mockserver: MockServer, https_dest: bool - ) -> None: - """SOCKS5 proxy, HTTP or HTTPS destination.""" - if not self.handler_supports_socks: - pytest.skip("SOCKS proxies are not supported") - crawler = get_crawler(SingleRequestSpider, self.settings_dict) - with caplog.at_level(logging.DEBUG): - await crawler.crawl_async( - seed=mockserver.url("/status?n=200", is_secure=https_dest) - ) - assert isinstance(crawler.spider, SingleRequestSpider) - self._assert_got_response_code(200, caplog.text) - self._assert_headers(crawler.spider.meta["responses"][0].headers, https_dest) - - @pytest.mark.parametrize( - "https_dest", [False, True], ids=["HTTP dest", "HTTPS dest"] - ) - @pytest.mark.usefixtures("socks5_proxy_server") - @coroutine_test - async def test_socks_proxy_auth_error( + async def test_proxy_redirect( self, caplog: pytest.LogCaptureFixture, - monkeypatch: pytest.MonkeyPatch, + proxy_server: str, mockserver: MockServer, https_dest: bool, ) -> None: - if not self.handler_supports_socks: - pytest.skip("SOCKS proxies are not supported") - envvar = "https_proxy" if https_dest else "http_proxy" - monkeypatch.setenv(envvar, wrong_credentials(os.environ[envvar])) - crawler = get_crawler(SimpleSpider, self.settings_dict) + """HTTP/HTTPS/SOCKS5 proxy, HTTP or HTTPS destination, following a + redirect. Check that the redirected request still goes through the + proxy and doesn't lose the proxy auth. + """ + self._maybe_skip(proxy_server, https_dest) + crawler = get_crawler(SingleRequestSpider, self.settings_dict) with caplog.at_level(logging.DEBUG): await crawler.crawl_async( - mockserver.url("/status?n=200", is_secure=https_dest) + seed=mockserver.url("/redirect", is_secure=https_dest) ) - assert "DownloadConnectionRefusedError" in caplog.text + assert isinstance(crawler.spider, SingleRequestSpider) + assert crawler.spider.meta.get("failure") is None + responses = crawler.spider.meta.get("responses", []) + assert len(responses) == 1 + assert responses[0].status == 200 + assert responses[0].url == mockserver.url("/redirected", is_secure=https_dest) + self._assert_got_response_code(200, caplog.text) + self._assert_headers(responses[0].headers, https_dest) @staticmethod def _assert_headers(headers: Headers, https_dest: bool) -> None: