Run mitmproxy-based tests for every handler and improve them.

conftest.py

@@ -11,6 +11,7 @@ from scrapy.utils.reactor import set_asyncio_event_loop_policy
 from scrapy.utils.reactorless import install_reactor_import_hook
 from tests.keys import generate_keys
 from tests.mockserver.http import MockServer
+from tests.mockserver.mitm_proxy import MitmProxy
 
 if TYPE_CHECKING:
     from collections.abc import Generator
@@ -72,6 +73,32 @@ def mockserver() -> Generator[MockServer]:
         yield mockserver
 
 
+@pytest.fixture  # function scope because it modifies os.environ
+def mitm_proxy_server(monkeypatch: pytest.MonkeyPatch) -> Generator[MitmProxy]:
+    proxy = MitmProxy()
+    url = proxy.start()
+    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(scope="session")
 def reactor_pytest(request) -> str:
     return request.config.getoption("--reactor")

docs/topics/download-handlers.rst

@@ -206,6 +206,8 @@ If you want to use this handler you need to replace the default one for the
 
     Known limitations of the HTTP/2 implementation in this handler include:
 
+    -   No support for proxies.
+
     -   No support for HTTP/2 Cleartext (h2c), since no major browser supports
         HTTP/2 unencrypted (refer `http2 faq`_).
 

tests/mockserver/mitm_proxy.py

@@ -0,0 +1,64 @@
+from __future__ import annotations
+
+import re
+import sys
+from pathlib import Path
+from subprocess import PIPE, Popen
+from urllib.parse import urlsplit, urlunsplit
+
+
+class MitmProxy:
+    auth_user = "scrapy"
+    auth_pass = "scrapy"
+
+    def start(self) -> str:
+        script = """
+import sys
+from mitmproxy.tools.main import mitmdump
+sys.argv[0] = "mitmdump"
+sys.exit(mitmdump())
+        """
+        cert_path = Path(__file__).parent.parent.resolve() / "keys"
+        args = [
+            "--listen-host",
+            "127.0.0.1",
+            "--listen-port",
+            "0",
+            "--proxyauth",
+            f"{self.auth_user}:{self.auth_pass}",
+            "--set",
+            f"confdir={cert_path}",
+            "--ssl-insecure",
+            "-s",
+            str(Path(__file__).with_name("mitm_proxy_addon.py")),
+        ]
+        self.proc: Popen[str] = Popen(
+            [
+                sys.executable,
+                "-u",
+                "-c",
+                script,
+                *args,
+            ],
+            stdout=PIPE,
+            text=True,
+        )
+        assert self.proc.stdout is not None
+        line = ""
+        for line in self.proc.stdout:
+            m = re.search(r"listening at (?:http://)?([^:]+:\d+)", line)
+            if m:
+                host_port = m.group(1)
+                return f"http://{self.auth_user}:{self.auth_pass}@{host_port}"
+        self.stop()
+        raise RuntimeError(f"Failed to parse mitmdump output: {line}")
+
+    def stop(self) -> None:
+        self.proc.kill()
+        self.proc.communicate()
+
+
+def wrong_credentials(proxy_url: str) -> str:
+    bad_auth_proxy = list(urlsplit(proxy_url))
+    bad_auth_proxy[1] = bad_auth_proxy[1].replace("scrapy:scrapy@", "wrong:wronger@")
+    return urlunsplit(bad_auth_proxy)

tests/mockserver/mitm_proxy_addon.py

@@ -0,0 +1,5 @@
+def response(flow) -> None:
+    # add custom headers to be able to check that the request went through the proxy
+    flow.response.headers["X-Via-Mitmproxy"] = "1"
+    if flow.client_conn.tls_established:
+        flow.response.headers["X-Via-Mitmproxy-TLS"] = "1"

tests/test_downloader_handler_httpx.py

@@ -17,6 +17,7 @@ from tests.test_downloader_handlers_http_base import (
     TestHttpsInvalidDNSPatternBase,
     TestHttpsWrongHostnameBase,
     TestHttpWithCrawlerBase,
+    TestMitmProxyBase,
     TestSimpleHttpsBase,
 )
 from tests.utils.decorators import coroutine_test
@@ -41,6 +42,15 @@ class HttpxDownloadHandlerMixin:
 
         return HttpxDownloadHandler
 
+    @property
+    def settings_dict(self) -> dict[str, Any] | None:
+        return {
+            "DOWNLOAD_HANDLERS": {
+                "http": "scrapy.core.downloader.handlers._httpx.HttpxDownloadHandler",
+                "https": "scrapy.core.downloader.handlers._httpx.HttpxDownloadHandler",
+            }
+        }
+
 
 class TestHttp(HttpxDownloadHandlerMixin, TestHttpBase):
     handler_supports_bindaddress_meta = False
@@ -108,15 +118,8 @@ class TestHttpsCustomCiphers(HttpxDownloadHandlerMixin, TestHttpsCustomCiphersBa
     pass
 
 
-class TestHttpWithCrawler(TestHttpWithCrawlerBase):
-    @property
-    def settings_dict(self) -> dict[str, Any] | None:
-        return {
-            "DOWNLOAD_HANDLERS": {
-                "http": "scrapy.core.downloader.handlers._httpx.HttpxDownloadHandler",
-                "https": "scrapy.core.downloader.handlers._httpx.HttpxDownloadHandler",
-            }
-        }
+class TestHttpWithCrawler(HttpxDownloadHandlerMixin, TestHttpWithCrawlerBase):
+    pass
 
 
 class TestHttpsWithCrawler(TestHttpWithCrawler):
@@ -136,3 +139,9 @@ class TestHttpProxy(HttpxDownloadHandlerMixin, TestHttpProxyBase):
 @pytest.mark.skip(reason="Proxy support is not implemented yet")
 class TestHttpsProxy(HttpxDownloadHandlerMixin, TestHttpProxyBase):
     is_secure = True
+
+
+@pytest.mark.skip(reason="Proxy support is not implemented yet")
+@pytest.mark.requires_mitmproxy
+class TestMitmProxy(HttpxDownloadHandlerMixin, TestMitmProxyBase):
+    pass

tests/test_downloader_handler_twisted_http11.py

@@ -19,6 +19,7 @@ from tests.test_downloader_handlers_http_base import (
     TestHttpsInvalidDNSPatternBase,
     TestHttpsWrongHostnameBase,
     TestHttpWithCrawlerBase,
+    TestMitmProxyBase,
     TestSimpleHttpsBase,
 )
 
@@ -34,6 +35,15 @@ class HTTP11DownloadHandlerMixin:
     def download_handler_cls(self) -> type[DownloadHandlerProtocol]:
         return HTTP11DownloadHandler
 
+    @property
+    def settings_dict(self) -> dict[str, Any] | None:
+        return {
+            "DOWNLOAD_HANDLERS": {
+                "http": "scrapy.core.downloader.handlers.http11.HTTP11DownloadHandler",
+                "https": "scrapy.core.downloader.handlers.http11.HTTP11DownloadHandler",
+            }
+        }
+
 
 def test_not_configured_without_reactor() -> None:
     crawler = Crawler(Spider, {"TWISTED_REACTOR_ENABLED": False})
@@ -71,15 +81,8 @@ class TestHttpsCustomCiphers(HTTP11DownloadHandlerMixin, TestHttpsCustomCiphersB
     pass
 
 
-class TestHttpWithCrawler(TestHttpWithCrawlerBase):
-    @property
-    def settings_dict(self) -> dict[str, Any] | None:
-        return {
-            "DOWNLOAD_HANDLERS": {
-                "http": "scrapy.core.downloader.handlers.http11.HTTP11DownloadHandler",
-                "https": "scrapy.core.downloader.handlers.http11.HTTP11DownloadHandler",
-            }
-        }
+class TestHttpWithCrawler(HTTP11DownloadHandlerMixin, TestHttpWithCrawlerBase):
+    pass
 
 
 class TestHttpsWithCrawler(TestHttpWithCrawler):
@@ -88,3 +91,9 @@ class TestHttpsWithCrawler(TestHttpWithCrawler):
 
 class TestHttpProxy(HTTP11DownloadHandlerMixin, TestHttpProxyBase):
     pass
+
+
+@pytest.mark.requires_mitmproxy
+class TestMitmProxy(HTTP11DownloadHandlerMixin, TestMitmProxyBase):
+    # not implemented
+    handler_supports_tls_in_tls = False

tests/test_downloader_handler_twisted_http2.py

@@ -10,13 +10,8 @@ from twisted.web.http import H2_ENABLED
 
 from scrapy import Spider
 from scrapy.crawler import Crawler
-from scrapy.exceptions import (
-    DownloadFailedError,
-    NotConfigured,
-    UnsupportedURLSchemeError,
-)
+from scrapy.exceptions import DownloadFailedError, NotConfigured
 from scrapy.http import Request
-from scrapy.utils.defer import maybe_deferred_to_future
 from tests.test_downloader_handlers_http_base import (
     TestHttpProxyBase,
     TestHttpsBase,
@@ -25,13 +20,13 @@ from tests.test_downloader_handlers_http_base import (
     TestHttpsInvalidDNSPatternBase,
     TestHttpsWrongHostnameBase,
     TestHttpWithCrawlerBase,
+    TestMitmProxyBase,
 )
 from tests.utils.decorators import coroutine_test
 
 if TYPE_CHECKING:
     from scrapy.core.downloader.handlers import DownloadHandlerProtocol
     from tests.mockserver.http import MockServer
-    from tests.mockserver.proxy_echo import ProxyEchoMockServer
 
 
 pytestmark = [
@@ -52,6 +47,15 @@ class H2DownloadHandlerMixin:
 
         return H2DownloadHandler
 
+    @property
+    def settings_dict(self) -> dict[str, Any] | None:
+        return {
+            "DOWNLOAD_HANDLERS": {
+                "http": None,
+                "https": "scrapy.core.downloader.handlers.http2.H2DownloadHandler",
+            }
+        }
+
 
 def test_not_configured_without_reactor() -> None:
     from scrapy.core.downloader.handlers.http2 import H2DownloadHandler  # noqa: PLC0415
@@ -167,16 +171,7 @@ class TestHttp2CustomCiphers(H2DownloadHandlerMixin, TestHttpsCustomCiphersBase)
     pass
 
 
-class TestHttp2WithCrawler(TestHttpWithCrawlerBase):
-    @property
-    def settings_dict(self) -> dict[str, Any] | None:
-        return {
-            "DOWNLOAD_HANDLERS": {
-                "http": None,
-                "https": "scrapy.core.downloader.handlers.http2.H2DownloadHandler",
-            }
-        }
-
+class TestHttp2WithCrawler(H2DownloadHandlerMixin, TestHttpWithCrawlerBase):
     is_secure = True
 
     def test_bytes_received_stop_download_callback(self) -> None:  # type: ignore[override]
@@ -192,24 +187,12 @@ class TestHttp2WithCrawler(TestHttpWithCrawlerBase):
         pytest.skip("headers_received support is not implemented")
 
 
+@pytest.mark.skip(reason="Proxy support is not implemented yet")
 class TestHttp2Proxy(H2DownloadHandlerMixin, TestHttpProxyBase):
     is_secure = True
-    expected_http_proxy_request_body = b"/"
 
-    @coroutine_test
-    async def test_download_with_proxy_https_timeout(
-        self, proxy_mockserver: ProxyEchoMockServer
-    ) -> None:
-        with pytest.raises(NotImplementedError):
-            await maybe_deferred_to_future(
-                super().test_download_with_proxy_https_timeout(proxy_mockserver)  # type: ignore[arg-type]
-            )
 
-    @coroutine_test
-    async def test_download_with_proxy_without_http_scheme(
-        self, proxy_mockserver: ProxyEchoMockServer
-    ) -> None:
-        with pytest.raises(UnsupportedURLSchemeError):
-            await maybe_deferred_to_future(
-                super().test_download_with_proxy_without_http_scheme(proxy_mockserver)  # type: ignore[arg-type]
-            )
+@pytest.mark.skip(reason="Proxy support is not implemented yet")
+@pytest.mark.requires_mitmproxy
+class TestMitmProxy(H2DownloadHandlerMixin, TestMitmProxyBase):
+    pass

tests/test_downloader_handlers_http_base.py

@@ -4,6 +4,8 @@ from __future__ import annotations
 
 import gzip
 import json
+import logging
+import os
 import platform
 import re
 import sys
@@ -36,6 +38,7 @@ from scrapy.utils.misc import build_from_crawler
 from scrapy.utils.spider import DefaultSpider
 from scrapy.utils.test import get_crawler
 from tests import NON_EXISTING_RESOLVABLE
+from tests.mockserver.mitm_proxy import MitmProxy, wrong_credentials
 from tests.mockserver.proxy_echo import ProxyEchoMockServer
 from tests.mockserver.simple_https import SimpleMockServer
 from tests.spiders import (
@@ -43,6 +46,7 @@ from tests.spiders import (
     BytesReceivedErrbackSpider,
     HeadersReceivedCallbackSpider,
     HeadersReceivedErrbackSpider,
+    SimpleSpider,
     SingleRequestSpider,
 )
 from tests.utils.decorators import coroutine_test
@@ -1143,3 +1147,118 @@ class TestHttpProxyBase(ABC):
         assert response.status == 200
         assert response.url == request.url
         assert response.body == self.expected_http_proxy_request_body
+
+
+class TestMitmProxyBase(ABC):
+    # whether the handler supports HTTPS proxies with HTTPS destinations
+    handler_supports_tls_in_tls: bool = True
+
+    @property
+    @abstractmethod
+    def settings_dict(self) -> dict[str, Any] | None:
+        raise NotImplementedError
+
+    @pytest.mark.parametrize(
+        "https_dest", [False, True], ids=["HTTP dest", "HTTPS dest"]
+    )
+    @coroutine_test
+    async def test_http_proxy(
+        self,
+        caplog: pytest.LogCaptureFixture,
+        mockserver: MockServer,
+        mitm_proxy_server: MitmProxy,
+        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"]
+    )
+    @coroutine_test
+    async def test_https_proxy(
+        self,
+        caplog: pytest.LogCaptureFixture,
+        mockserver: MockServer,
+        mitm_proxy_server_https: MitmProxy,
+        https_dest: bool,
+    ) -> None:
+        """HTTPS proxy, HTTP or HTTPS destination."""
+        if https_dest and not self.handler_supports_tls_in_tls:
+            pytest.skip("HTTPS proxy to HTTPS destination is 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"]
+    )
+    @coroutine_test
+    async def test_http_proxy_auth_error(
+        self,
+        caplog: pytest.LogCaptureFixture,
+        monkeypatch: pytest.MonkeyPatch,
+        mockserver: MockServer,
+        mitm_proxy_server: MitmProxy,
+        https_dest: bool,
+    ) -> None:
+        """HTTP proxy, HTTP or HTTPS destination, wrong proxy creds."""
+        envvar = "https_proxy" if https_dest else "http_proxy"
+        monkeypatch.setenv(envvar, wrong_credentials(os.environ[envvar]))
+        crawler = get_crawler(SimpleSpider, self.settings_dict)
+        with caplog.at_level(logging.DEBUG):
+            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)
+
+    @pytest.mark.parametrize(
+        "https_dest", [False, True], ids=["HTTP dest", "HTTPS dest"]
+    )
+    @coroutine_test
+    async def test_dont_leak_proxy_authorization_header(
+        self,
+        caplog: pytest.LogCaptureFixture,
+        mockserver: MockServer,
+        mitm_proxy_server: MitmProxy,
+        https_dest: bool,
+    ) -> None:
+        """HTTP proxy, HTTP or HTTPS destination. Check that the auth header
+        is not sent to the destination."""
+        request = Request(mockserver.url("/echo", is_secure=https_dest))
+        crawler = get_crawler(SingleRequestSpider, self.settings_dict)
+        with caplog.at_level(logging.DEBUG):
+            await crawler.crawl_async(seed=request)
+        assert isinstance(crawler.spider, SingleRequestSpider)
+        self._assert_got_response_code(200, caplog.text)
+        self._assert_headers(crawler.spider.meta["responses"][0].headers, https_dest)
+        echo = json.loads(crawler.spider.meta["responses"][0].text)
+        assert "Proxy-Authorization" not in echo["headers"]
+
+    @staticmethod
+    def _assert_headers(headers: Headers, https_dest: bool) -> None:
+        assert b"X-Via-Mitmproxy" in headers
+        if https_dest:
+            assert b"X-Via-Mitmproxy-TLS" in headers
+
+    @staticmethod
+    def _assert_got_response_code(code: int, log: str) -> None:
+        assert str(log).count(f"Crawled ({code})") == 1
+
+    @staticmethod
+    def _assert_got_auth_exception(log: str) -> None:
+        assert "Proxy Authentication Required" in log or "407" in log

tests/test_proxy_connect.py

@@ -1,125 +0,0 @@
-import json
-import os
-import re
-import sys
-from pathlib import Path
-from subprocess import PIPE, Popen
-from urllib.parse import urlsplit, urlunsplit
-
-import pytest
-from testfixtures import LogCapture
-
-from scrapy.http import Request
-from scrapy.utils.test import get_crawler
-from tests.mockserver.http import MockServer
-from tests.spiders import SimpleSpider, SingleRequestSpider
-from tests.utils.decorators import inline_callbacks_test
-
-
-class MitmProxy:
-    auth_user = "scrapy"
-    auth_pass = "scrapy"
-
-    def start(self) -> str:
-        script = """
-import sys
-from mitmproxy.tools.main import mitmdump
-sys.argv[0] = "mitmdump"
-sys.exit(mitmdump())
-        """
-        cert_path = Path(__file__).parent.resolve() / "keys"
-        args = [
-            "--listen-host",
-            "127.0.0.1",
-            "--listen-port",
-            "0",
-            "--proxyauth",
-            f"{self.auth_user}:{self.auth_pass}",
-            "--set",
-            f"confdir={cert_path}",
-            "--ssl-insecure",
-        ]
-        self.proc = Popen(
-            [
-                sys.executable,
-                "-u",
-                "-c",
-                script,
-                *args,
-            ],
-            stdout=PIPE,
-            text=True,
-        )
-        assert self.proc.stdout is not None
-        line = self.proc.stdout.readline()
-        m = re.search(r"listening at (?:http://)?([^:]+:\d+)", line)
-        if not m:
-            raise RuntimeError(f"Failed to parse mitmdump output: {line}")
-        host_port = m.group(1)
-        return f"http://{self.auth_user}:{self.auth_pass}@{host_port}"
-
-    def stop(self) -> None:
-        self.proc.kill()
-        self.proc.communicate()
-
-
-def _wrong_credentials(proxy_url: str) -> str:
-    bad_auth_proxy = list(urlsplit(proxy_url))
-    bad_auth_proxy[1] = bad_auth_proxy[1].replace("scrapy:scrapy@", "wrong:wronger@")
-    return urlunsplit(bad_auth_proxy)
-
-
-@pytest.mark.requires_mitmproxy
-class TestProxyConnect:
-    @classmethod
-    def setup_class(cls):
-        cls.mockserver = MockServer()
-        cls.mockserver.__enter__()
-
-    @classmethod
-    def teardown_class(cls):
-        cls.mockserver.__exit__(None, None, None)
-
-    def setup_method(self):
-        self._oldenv = os.environ.copy()
-        self._proxy = MitmProxy()
-        proxy_url = self._proxy.start()
-        os.environ["https_proxy"] = proxy_url
-        os.environ["http_proxy"] = proxy_url
-
-    def teardown_method(self):
-        self._proxy.stop()
-        os.environ = self._oldenv
-
-    @inline_callbacks_test
-    def test_https_connect_tunnel(self):
-        crawler = get_crawler(SimpleSpider)
-        with LogCapture() as log:
-            yield crawler.crawl(self.mockserver.url("/status?n=200", is_secure=True))
-        self._assert_got_response_code(200, log)
-
-    @inline_callbacks_test
-    def test_https_tunnel_auth_error(self):
-        os.environ["https_proxy"] = _wrong_credentials(os.environ["https_proxy"])
-        crawler = get_crawler(SimpleSpider)
-        with LogCapture() as log:
-            yield crawler.crawl(self.mockserver.url("/status?n=200", is_secure=True))
-        # The proxy returns a 407 error code but it does not reach the client;
-        # he just sees a TunnelError.
-        self._assert_got_tunnel_error(log)
-
-    @inline_callbacks_test
-    def test_https_tunnel_without_leak_proxy_authorization_header(self):
-        request = Request(self.mockserver.url("/echo", is_secure=True))
-        crawler = get_crawler(SingleRequestSpider)
-        with LogCapture() as log:
-            yield crawler.crawl(seed=request)
-        self._assert_got_response_code(200, log)
-        echo = json.loads(crawler.spider.meta["responses"][0].text)
-        assert "Proxy-Authorization" not in echo["headers"]
-
-    def _assert_got_response_code(self, code, log):
-        assert str(log).count(f"Crawled ({code})") == 1
-
-    def _assert_got_tunnel_error(self, log):
-        assert "TunnelError" in str(log)