From 85c616c5c71d4de01746f9c190274fe3d2ac9ed0 Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Thu, 14 May 2026 22:56:05 +0500 Subject: [PATCH] Fix certificate issuer verification on new Twisted. (#7520) --- pyproject.toml | 1 + scrapy/core/downloader/contextfactory.py | 1 - scrapy/core/downloader/tls.py | 19 ++--- tests/test_downloader_handler_httpx.py | 6 ++ .../test_downloader_handler_twisted_http11.py | 9 +++ .../test_downloader_handler_twisted_http2.py | 9 +++ tests/test_downloader_handlers_http_base.py | 69 ++++++++++++++++++- 7 files changed, 96 insertions(+), 18 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 6c9ff3100..76b8bbead 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -267,6 +267,7 @@ markers = [ "requires_botocore: marks tests that need botocore (but not boto3)", "requires_boto3: marks tests that need botocore and boto3", "requires_mitmproxy: marks tests that need mitmproxy", + "requires_internet: marks tests that need real Internet access", ] filterwarnings = [ "ignore::DeprecationWarning:twisted.web.static", diff --git a/scrapy/core/downloader/contextfactory.py b/scrapy/core/downloader/contextfactory.py index 7f7d34c21..d2626fd9d 100644 --- a/scrapy/core/downloader/contextfactory.py +++ b/scrapy/core/downloader/contextfactory.py @@ -131,7 +131,6 @@ class _ScrapyClientContextFactory(BrowserLikePolicyForHTTPS): return _ScrapyClientTLSOptions26( self._get_cert_options()._makeTLSConnection, hostname.decode("ascii"), - verbose_logging=self.tls_verbose_logging, ) return _ScrapyClientTLSOptions( hostname.decode("ascii"), # type: ignore[arg-type] diff --git a/scrapy/core/downloader/tls.py b/scrapy/core/downloader/tls.py index 779c05daa..cb06f81df 100644 --- a/scrapy/core/downloader/tls.py +++ b/scrapy/core/downloader/tls.py @@ -107,34 +107,23 @@ class _ScrapyClientTLSOptions26(ClientTLSOptions): logging warnings. Instances of this class are returned from - :class:`.ScrapyClientContextFactory`. + :class:`._ScrapyClientContextFactory`. This class is used on Twisted 26.4.0 and newer. """ - def __init__( - self, - createConnection: Callable[[TLSMemoryBIOProtocol], SSL.Connection], - hostname: str, - verbose_logging: bool = False, - ): - super().__init__(createConnection, hostname) - self.verbose_logging: bool = verbose_logging - def clientConnectionForTLS( self, tlsProtocol: TLSMemoryBIOProtocol ) -> SSL.Connection: """This method is needed to override the verify callback.""" conn = super().clientConnectionForTLS(tlsProtocol) - callback = self._verifyCB( - self._hostnameIsDnsName, self._hostnameASCII, self.verbose_logging - ) + callback = self._verifyCB(self._hostnameIsDnsName, self._hostnameASCII) conn.set_verify(SSL.VERIFY_PEER | SSL.VERIFY_FAIL_IF_NO_PEER_CERT, callback) return conn @staticmethod def _verifyCB( - hostIsDNS: bool, hostnameASCII: str, verbose_logging: bool + hostIsDNS: bool, hostnameASCII: str ) -> Callable[[SSL.Connection, X509, int, int, int], bool]: svcid: ServiceID = ( DNS_ID(hostnameASCII) if hostIsDNS else IPAddress_ID(hostnameASCII) @@ -145,7 +134,7 @@ class _ScrapyClientTLSOptions26(ClientTLSOptions): ) -> bool: if depth != 0: # We are only verifying the leaf certificate. - return bool(ok) + return True try: verify_service_identity(extract_patterns(cert), [svcid], []) diff --git a/tests/test_downloader_handler_httpx.py b/tests/test_downloader_handler_httpx.py index d4dfe8396..bc1acf9f9 100644 --- a/tests/test_downloader_handler_httpx.py +++ b/tests/test_downloader_handler_httpx.py @@ -18,6 +18,7 @@ from tests.test_downloader_handlers_http_base import ( TestHttpsWrongHostnameBase, TestHttpWithCrawlerBase, TestMitmProxyBase, + TestRealWebsiteBase, TestSimpleHttpsBase, ) from tests.utils.decorators import coroutine_test @@ -145,3 +146,8 @@ class TestHttpsProxy(HttpxDownloadHandlerMixin, TestHttpProxyBase): @pytest.mark.requires_mitmproxy class TestMitmProxy(HttpxDownloadHandlerMixin, TestMitmProxyBase): pass + + +@pytest.mark.requires_internet +class TestRealWebsite(HttpxDownloadHandlerMixin, TestRealWebsiteBase): + pass diff --git a/tests/test_downloader_handler_twisted_http11.py b/tests/test_downloader_handler_twisted_http11.py index 89cf58c96..fb3305945 100644 --- a/tests/test_downloader_handler_twisted_http11.py +++ b/tests/test_downloader_handler_twisted_http11.py @@ -2,6 +2,7 @@ from __future__ import annotations +import sys from typing import TYPE_CHECKING, Any import pytest @@ -20,6 +21,7 @@ from tests.test_downloader_handlers_http_base import ( TestHttpsWrongHostnameBase, TestHttpWithCrawlerBase, TestMitmProxyBase, + TestRealWebsiteBase, TestSimpleHttpsBase, ) @@ -103,3 +105,10 @@ class TestHttpsProxy(HTTP11DownloadHandlerMixin, TestHttpProxyBase): class TestMitmProxy(HTTP11DownloadHandlerMixin, TestMitmProxyBase): # not implemented handler_supports_tls_in_tls = False + + +@pytest.mark.requires_internet +class TestRealWebsite(HTTP11DownloadHandlerMixin, TestRealWebsiteBase): + @property + def platform_cert_store_works(self) -> bool: + return sys.platform != "win32" diff --git a/tests/test_downloader_handler_twisted_http2.py b/tests/test_downloader_handler_twisted_http2.py index 60b276166..24cb9b1aa 100644 --- a/tests/test_downloader_handler_twisted_http2.py +++ b/tests/test_downloader_handler_twisted_http2.py @@ -2,6 +2,7 @@ from __future__ import annotations +import sys from typing import TYPE_CHECKING, Any import pytest @@ -21,6 +22,7 @@ from tests.test_downloader_handlers_http_base import ( TestHttpsWrongHostnameBase, TestHttpWithCrawlerBase, TestMitmProxyBase, + TestRealWebsiteBase, ) from tests.utils.decorators import coroutine_test @@ -196,3 +198,10 @@ class TestHttp2Proxy(H2DownloadHandlerMixin, TestHttpProxyBase): @pytest.mark.requires_mitmproxy class TestMitmProxy(H2DownloadHandlerMixin, TestMitmProxyBase): pass + + +@pytest.mark.requires_internet +class TestRealWebsite(H2DownloadHandlerMixin, TestRealWebsiteBase): + @property + def platform_cert_store_works(self) -> bool: + return sys.platform != "win32" diff --git a/tests/test_downloader_handlers_http_base.py b/tests/test_downloader_handlers_http_base.py index 138ddd944..a8d002d30 100644 --- a/tests/test_downloader_handlers_http_base.py +++ b/tests/test_downloader_handlers_http_base.py @@ -6,7 +6,6 @@ import gzip import json import logging import os -import platform import re import sys from abc import ABC, abstractmethod @@ -524,7 +523,7 @@ class TestHttpBase(ABC): await download_handler.download_request(request) assert "download_latency" in request.meta latency = request.meta["download_latency"] - if sys.version_info < (3, 13) and platform.system() == "Windows": + if sys.version_info < (3, 13) and sys.platform == "win32": # time.monotonic() resolution is too low here: # https://docs.python.org/3/whatsnew/3.13.html#time assert latency >= 0 @@ -1266,3 +1265,69 @@ class TestMitmProxyBase(ABC): @staticmethod def _assert_got_auth_exception(log: str) -> None: assert "Proxy Authentication Required" in log or "407" in log + + +class TestRealWebsiteBase(ABC): + @property + @abstractmethod + def download_handler_cls(self) -> type[DownloadHandlerProtocol]: + raise NotImplementedError + + @property + @abstractmethod + def settings_dict(self) -> dict[str, Any] | None: + raise NotImplementedError + + @property + @abstractmethod + def platform_cert_store_works(self) -> bool: + """Whether valid certificates can be verified. + + Twisted on Windows cannot do that out of the box, see e.g. + https://github.com/twisted/twisted/issues/6371. + """ + return True + + @asynccontextmanager + async def get_dh( + self, settings_dict: dict[str, Any] | None = None + ) -> AsyncGenerator[DownloadHandlerProtocol]: + crawler = get_crawler(DefaultSpider, settings_dict) + crawler.spider = crawler._create_spider() + dh = build_from_crawler(self.download_handler_cls, crawler) + try: + yield dh + finally: + await dh.close() + + @coroutine_test + async def test_download(self) -> None: + request = Request("https://books.toscrape.com/") + async with self.get_dh() as download_handler: + response = await download_handler.download_request(request) + assert response.status == 200 + assert "All products | Books to Scrape - Sandbox" in response.text + + @coroutine_test + async def test_download_with_spider(self) -> None: + crawler = get_crawler(SingleRequestSpider, self.settings_dict) + await maybe_deferred_to_future( + crawler.crawl(seed=Request("https://books.toscrape.com/")) + ) + assert isinstance(crawler.spider, SingleRequestSpider) + failure = crawler.spider.meta.get("failure") + assert failure is None + reason = crawler.spider.meta["close_reason"] + assert reason == "finished" + + @coroutine_test + async def test_verify_certs(self) -> None: + if not self.platform_cert_store_works: + pytest.skip("Cannot verify certificates") + request = Request("https://books.toscrape.com/") + async with self.get_dh( + {"DOWNLOAD_VERIFY_CERTIFICATES": True} + ) as download_handler: + response = await download_handler.download_request(request) + assert response.status == 200 + assert "All products | Books to Scrape - Sandbox" in response.text