fix(HTTP2Transport): Only enable HTTP2 when DSN is HTTPS (#3678)

Author: BYK

sentry_sdk/transport.py

@@ -215,15 +215,7 @@ def __init__(self, options):
         )  # type: DefaultDict[Tuple[EventDataCategory, str], int]
         self._last_client_report_sent = time.time()
 
-        self._pool = self._make_pool(
-            self.parsed_dsn,
-            http_proxy=options["http_proxy"],
-            https_proxy=options["https_proxy"],
-            ca_certs=options["ca_certs"],
-            cert_file=options["cert_file"],
-            key_file=options["key_file"],
-            proxy_headers=options["proxy_headers"],
-        )
+        self._pool = self._make_pool()
 
         # Backwards compatibility for deprecated `self.hub_class` attribute
         self._hub_cls = sentry_sdk.Hub
@@ -532,8 +524,8 @@ def _serialize_envelope(self, envelope):
 
         return content_encoding, body
 
-    def _get_pool_options(self, ca_certs, cert_file=None, key_file=None):
-        # type: (Self, Optional[Any], Optional[Any], Optional[Any]) -> Dict[str, Any]
+    def _get_pool_options(self):
+        # type: (Self) -> Dict[str, Any]
         raise NotImplementedError()
 
     def _in_no_proxy(self, parsed_dsn):
@@ -547,17 +539,8 @@ def _in_no_proxy(self, parsed_dsn):
                 return True
         return False
 
-    def _make_pool(
-        self,
-        parsed_dsn,  # type: Dsn
-        http_proxy,  # type: Optional[str]
-        https_proxy,  # type: Optional[str]
-        ca_certs,  # type: Optional[Any]
-        cert_file,  # type: Optional[Any]
-        key_file,  # type: Optional[Any]
-        proxy_headers,  # type: Optional[Dict[str, str]]
-    ):
-        # type: (...) -> Union[PoolManager, ProxyManager, httpcore.SOCKSProxy, httpcore.HTTPProxy, httpcore.ConnectionPool]
+    def _make_pool(self):
+        # type: (Self) -> Union[PoolManager, ProxyManager, httpcore.SOCKSProxy, httpcore.HTTPProxy, httpcore.ConnectionPool]
         raise NotImplementedError()
 
     def _request(
@@ -631,8 +614,8 @@ class HttpTransport(BaseHttpTransport):
     if TYPE_CHECKING:
         _pool: Union[PoolManager, ProxyManager]
 
-    def _get_pool_options(self, ca_certs, cert_file=None, key_file=None):
-        # type: (Self, Any, Any, Any) -> Dict[str, Any]
+    def _get_pool_options(self):
+        # type: (Self) -> Dict[str, Any]
 
         num_pools = self.options.get("_experiments", {}).get("transport_num_pools")
         options = {
@@ -658,42 +641,43 @@ def _get_pool_options(self, ca_certs, cert_file=None, key_file=None):
             options["socket_options"] = socket_options
 
         options["ca_certs"] = (
-            ca_certs  # User-provided bundle from the SDK init
+            self.options["ca_certs"]  # User-provided bundle from the SDK init
             or os.environ.get("SSL_CERT_FILE")
             or os.environ.get("REQUESTS_CA_BUNDLE")
             or certifi.where()
         )
 
-        options["cert_file"] = cert_file or os.environ.get("CLIENT_CERT_FILE")
-        options["key_file"] = key_file or os.environ.get("CLIENT_KEY_FILE")
+        options["cert_file"] = self.options["cert_file"] or os.environ.get(
+            "CLIENT_CERT_FILE"
+        )
+        options["key_file"] = self.options["key_file"] or os.environ.get(
+            "CLIENT_KEY_FILE"
+        )
 
         return options
 
-    def _make_pool(
-        self,
-        parsed_dsn,  # type: Dsn
-        http_proxy,  # type: Optional[str]
-        https_proxy,  # type: Optional[str]
-        ca_certs,  # type: Any
-        cert_file,  # type: Any
-        key_file,  # type: Any
-        proxy_headers,  # type: Optional[Dict[str, str]]
-    ):
-        # type: (...) -> Union[PoolManager, ProxyManager]
+    def _make_pool(self):
+        # type: (Self) -> Union[PoolManager, ProxyManager]
+        if self.parsed_dsn is None:
+            raise ValueError("Cannot create HTTP-based transport without valid DSN")
+
         proxy = None
-        no_proxy = self._in_no_proxy(parsed_dsn)
+        no_proxy = self._in_no_proxy(self.parsed_dsn)
 
         # try HTTPS first
-        if parsed_dsn.scheme == "https" and (https_proxy != ""):
+        https_proxy = self.options["https_proxy"]
+        if self.parsed_dsn.scheme == "https" and (https_proxy != ""):
             proxy = https_proxy or (not no_proxy and getproxies().get("https"))
 
         # maybe fallback to HTTP proxy
+        http_proxy = self.options["http_proxy"]
         if not proxy and (http_proxy != ""):
             proxy = http_proxy or (not no_proxy and getproxies().get("http"))
 
-        opts = self._get_pool_options(ca_certs, cert_file, key_file)
+        opts = self._get_pool_options()
 
         if proxy:
+            proxy_headers = self.options["proxy_headers"]
             if proxy_headers:
                 opts["proxy_headers"] = proxy_headers
 
@@ -783,10 +767,11 @@ def _request(
             )
             return response
 
-        def _get_pool_options(self, ca_certs, cert_file=None, key_file=None):
-            # type: (Any, Any, Any) -> Dict[str, Any]
+        def _get_pool_options(self):
+            # type: (Self) -> Dict[str, Any]
             options = {
-                "http2": True,
+                "http2": self.parsed_dsn is not None
+                and self.parsed_dsn.scheme == "https",
                 "retries": 3,
             }  # type: Dict[str, Any]
 
@@ -805,45 +790,41 @@ def _get_pool_options(self, ca_certs, cert_file=None, key_file=None):
 
             ssl_context = ssl.create_default_context()
             ssl_context.load_verify_locations(
-                ca_certs  # User-provided bundle from the SDK init
+                self.options["ca_certs"]  # User-provided bundle from the SDK init
                 or os.environ.get("SSL_CERT_FILE")
                 or os.environ.get("REQUESTS_CA_BUNDLE")
                 or certifi.where()
             )
-            cert_file = cert_file or os.environ.get("CLIENT_CERT_FILE")
-            key_file = key_file or os.environ.get("CLIENT_KEY_FILE")
+            cert_file = self.options["cert_file"] or os.environ.get("CLIENT_CERT_FILE")
+            key_file = self.options["key_file"] or os.environ.get("CLIENT_KEY_FILE")
             if cert_file is not None:
                 ssl_context.load_cert_chain(cert_file, key_file)
 
             options["ssl_context"] = ssl_context
 
             return options
 
-        def _make_pool(
-            self,
-            parsed_dsn,  # type: Dsn
-            http_proxy,  # type: Optional[str]
-            https_proxy,  # type: Optional[str]
-            ca_certs,  # type: Any
-            cert_file,  # type: Any
-            key_file,  # type: Any
-            proxy_headers,  # type: Optional[Dict[str, str]]
-        ):
-            # type: (...) -> Union[httpcore.SOCKSProxy, httpcore.HTTPProxy, httpcore.ConnectionPool]
+        def _make_pool(self):
+            # type: (Self) -> Union[httpcore.SOCKSProxy, httpcore.HTTPProxy, httpcore.ConnectionPool]
+            if self.parsed_dsn is None:
+                raise ValueError("Cannot create HTTP-based transport without valid DSN")
             proxy = None
-            no_proxy = self._in_no_proxy(parsed_dsn)
+            no_proxy = self._in_no_proxy(self.parsed_dsn)
 
             # try HTTPS first
-            if parsed_dsn.scheme == "https" and (https_proxy != ""):
+            https_proxy = self.options["https_proxy"]
+            if self.parsed_dsn.scheme == "https" and (https_proxy != ""):
                 proxy = https_proxy or (not no_proxy and getproxies().get("https"))
 
             # maybe fallback to HTTP proxy
+            http_proxy = self.options["http_proxy"]
             if not proxy and (http_proxy != ""):
                 proxy = http_proxy or (not no_proxy and getproxies().get("http"))
 
-            opts = self._get_pool_options(ca_certs, cert_file, key_file)
+            opts = self._get_pool_options()
 
             if proxy:
+                proxy_headers = self.options["proxy_headers"]
                 if proxy_headers:
                     opts["proxy_headers"] = proxy_headers
 

tests/test_transport.py

@@ -219,7 +219,7 @@ def test_transport_num_pools(make_client, num_pools, expected_num_pools):
 
     client = make_client(_experiments=_experiments)
 
-    options = client.transport._get_pool_options([])
+    options = client.transport._get_pool_options()
     assert options["num_pools"] == expected_num_pools
 
 
@@ -231,12 +231,15 @@ def test_two_way_ssl_authentication(make_client, http2):
     if http2:
         _experiments["transport_http2"] = True
 
-    client = make_client(_experiments=_experiments)
-
     current_dir = os.path.dirname(__file__)
     cert_file = f"{current_dir}/test.pem"
     key_file = f"{current_dir}/test.key"
-    options = client.transport._get_pool_options([], cert_file, key_file)
+    client = make_client(
+        cert_file=cert_file,
+        key_file=key_file,
+        _experiments=_experiments,
+    )
+    options = client.transport._get_pool_options()
 
     if http2:
         assert options["ssl_context"] is not None
@@ -254,23 +257,39 @@ def test_socket_options(make_client):
 
     client = make_client(socket_options=socket_options)
 
-    options = client.transport._get_pool_options([])
+    options = client.transport._get_pool_options()
     assert options["socket_options"] == socket_options
 
 
 def test_keep_alive_true(make_client):
     client = make_client(keep_alive=True)
 
-    options = client.transport._get_pool_options([])
+    options = client.transport._get_pool_options()
     assert options["socket_options"] == KEEP_ALIVE_SOCKET_OPTIONS
 
 
 def test_keep_alive_on_by_default(make_client):
     client = make_client()
-    options = client.transport._get_pool_options([])
+    options = client.transport._get_pool_options()
     assert "socket_options" not in options
 
 
+@pytest.mark.skipif(not PY38, reason="HTTP2 libraries are only available in py3.8+")
+def test_http2_with_https_dsn(make_client):
+    client = make_client(_experiments={"transport_http2": True})
+    client.transport.parsed_dsn.scheme = "https"
+    options = client.transport._get_pool_options()
+    assert options["http2"] is True
+
+
+@pytest.mark.skipif(not PY38, reason="HTTP2 libraries are only available in py3.8+")
+def test_no_http2_with_http_dsn(make_client):
+    client = make_client(_experiments={"transport_http2": True})
+    client.transport.parsed_dsn.scheme = "http"
+    options = client.transport._get_pool_options()
+    assert options["http2"] is False
+
+
 def test_socket_options_override_keep_alive(make_client):
     socket_options = [
         (socket.SOL_SOCKET, socket.SO_KEEPALIVE, 1),
@@ -280,7 +299,7 @@ def test_socket_options_override_keep_alive(make_client):
 
     client = make_client(socket_options=socket_options, keep_alive=False)
 
-    options = client.transport._get_pool_options([])
+    options = client.transport._get_pool_options()
     assert options["socket_options"] == socket_options
 
 
@@ -292,7 +311,7 @@ def test_socket_options_merge_with_keep_alive(make_client):
 
     client = make_client(socket_options=socket_options, keep_alive=True)
 
-    options = client.transport._get_pool_options([])
+    options = client.transport._get_pool_options()
     try:
         assert options["socket_options"] == [
             (socket.SOL_SOCKET, socket.SO_KEEPALIVE, 42),
@@ -314,7 +333,7 @@ def test_socket_options_override_defaults(make_client):
     # socket option defaults, so we need to set this and not ignore it.
     client = make_client(socket_options=[])
 
-    options = client.transport._get_pool_options([])
+    options = client.transport._get_pool_options()
     assert options["socket_options"] == []