From b7cd42da39834267d5b854a796cf6023021e1d95 Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Wed, 22 Apr 2026 19:01:01 +0500 Subject: [PATCH] Revert sharing of the SSL context in _ScrapyClientContextFactory. (#7450) --- scrapy/core/downloader/contextfactory.py | 32 ++++++++++++-------- scrapy/utils/_deps_compat.py | 4 ++- tests/test_core_downloader.py | 38 +++++++++++++++++++++++- 3 files changed, 60 insertions(+), 14 deletions(-) diff --git a/scrapy/core/downloader/contextfactory.py b/scrapy/core/downloader/contextfactory.py index ef0a46a8e..19fc77959 100644 --- a/scrapy/core/downloader/contextfactory.py +++ b/scrapy/core/downloader/contextfactory.py @@ -78,13 +78,6 @@ class _ScrapyClientContextFactory(BrowserLikePolicyForHTTPS): self.tls_ciphers = AcceptableCiphers.fromOpenSSLCipherString(tls_ciphers) else: self.tls_ciphers = DEFAULT_CIPHERS - with _filter_method_warning(): - self._certificate_options = CertificateOptions( - method=self._ssl_method, - fixBrokenPeers=True, - acceptableCiphers=self.tls_ciphers, - ) - self._ctx = self._get_context() self._verify_certificates = verify_certificates @classmethod @@ -109,23 +102,38 @@ class _ScrapyClientContextFactory(BrowserLikePolicyForHTTPS): **kwargs, ) + # should be removed together with ScrapyClientContextFactory def getCertificateOptions(self) -> CertificateOptions: # pragma: no cover - return self._certificate_options + return self._get_cert_options() + + def _get_cert_options(self) -> CertificateOptions: + with _filter_method_warning(): + return CertificateOptions( + method=self._ssl_method, + fixBrokenPeers=True, + acceptableCiphers=self.tls_ciphers, + ) # kept for old-style HTTP/1.0 downloader context twisted calls, # e.g. connectSSL() + # should be removed together with ScrapyClientContextFactory def getContext(self, hostname: Any = None, port: Any = None) -> SSL.Context: - return self._ctx + return self._get_context() def _get_context(self) -> SSL.Context: - ctx = self._certificate_options.getContext() + cert_options = self._get_cert_options() + ctx = cert_options.getContext() ctx.set_options(0x4) # OP_LEGACY_SERVER_CONNECT return ctx def creatorForNetloc(self, hostname: bytes, port: int) -> ClientTLSOptions: if not self._verify_certificates: - return _ScrapyClientTLSOptions(hostname.decode("ascii"), self._ctx) # type: ignore[no-untyped-call] - # Note that this doesn't use self._ctx + # _ScrapyClientTLSOptions is needed to skip verification errors + return _ScrapyClientTLSOptions( + hostname.decode("ascii"), self._get_context() + ) # type: ignore[no-untyped-call] + # Otherwise use the normal Twisted function. + # Note that this doesn't use self._get_context(). with _filter_method_warning(): return optionsForClientTLS( hostname=hostname.decode("ascii"), diff --git a/scrapy/utils/_deps_compat.py b/scrapy/utils/_deps_compat.py index 92399b31f..2ca6f657c 100644 --- a/scrapy/utils/_deps_compat.py +++ b/scrapy/utils/_deps_compat.py @@ -6,5 +6,7 @@ from twisted.python.versions import Version as TxVersion TWISTED_FAILURE_HAS_STACK = TWISTED_VERSION < TxVersion("twisted", 24, 10, 0) PYOPENSSL_VERSION = Version(PYOPENSSL_VERSION_STRING) -# SSL.Context.use_certificate wants an X509 object, SSL.Context.use_privatekey wants a PKey object +# SSL.Context.use_certificate() wants an X509 object, SSL.Context.use_privatekey() wants a PKey object PYOPENSSL_WANTS_X509_PKEY = PYOPENSSL_VERSION < Version("24.3.0") +# SSL.Context.set_cipher_list() creates a temporary connection, making the context immutable +PYOPENSSL_SET_CIPHER_LIST_TMP_CONN = PYOPENSSL_VERSION < Version("25.2.0") diff --git a/tests/test_core_downloader.py b/tests/test_core_downloader.py index cddc53b16..f9c5abf8a 100644 --- a/tests/test_core_downloader.py +++ b/tests/test_core_downloader.py @@ -17,6 +17,7 @@ from scrapy.core.downloader.contextfactory import ( ) from scrapy.core.downloader.handlers.http11 import _RequestBodyProducer from scrapy.exceptions import ScrapyDeprecationWarning +from scrapy.utils._deps_compat import PYOPENSSL_SET_CIPHER_LIST_TMP_CONN from scrapy.utils.defer import maybe_deferred_to_future from scrapy.utils.misc import build_from_crawler from scrapy.utils.python import to_bytes @@ -98,7 +99,7 @@ class TestContextFactoryBase: class TestContextFactory(TestContextFactoryBase): @coroutine_test - async def testPayload(self, server_url: str) -> None: + async def test_payload(self, server_url: str) -> None: s = "0123456789" * 10 crawler = get_crawler() client_context_factory = _load_context_factory_from_settings(crawler) @@ -107,6 +108,41 @@ class TestContextFactory(TestContextFactoryBase): ) assert body == to_bytes(s) + def test_no_context_sharing(self) -> None: + """Every call to creatorForNetloc() should give a fresh context.""" + crawler = get_crawler() + client_context_factory: _ScrapyClientContextFactory = ( + _load_context_factory_from_settings(crawler) + ) + creator1 = client_context_factory.creatorForNetloc(b"website1.tld", 443) + assert creator1._hostnameBytes == b"website1.tld" + creator2 = client_context_factory.creatorForNetloc(b"website2.tld", 443) + assert creator2._hostnameBytes == b"website2.tld" + assert creator1._ctx is not creator2._ctx + + @pytest.mark.skipif( + PYOPENSSL_SET_CIPHER_LIST_TMP_CONN, + reason="Fails or doesn't make sense on this pyOpenSSL version", + ) + def test_no_immutable_ctx_warning(self) -> None: + """There should be no pyOpenSSL context modification warning. + + pyOpenSSL < 25.1.0 doesn't produce this warning, and on 25.1.0 it's + always produced due to + https://github.com/scrapy/scrapy/issues/6859#issuecomment-4294917851. + """ + crawler = get_crawler() + client_context_factory: _ScrapyClientContextFactory = ( + _load_context_factory_from_settings(crawler) + ) + with warnings.catch_warnings(): + warnings.filterwarnings( + "error", + category=DeprecationWarning, + message="Attempting to mutate a Context after a Connection was created", + ) + client_context_factory.creatorForNetloc(b"website.tld", 443) + class TestContextFactoryTLSMethod(TestContextFactoryBase): async def _assert_factory_works(