http2: acceptable protocol update, tests (#4994)

scrapy/core/http2/protocol.py

@@ -33,13 +33,16 @@ from scrapy.spiders import Spider
 logger = logging.getLogger(__name__)
 
 
+PROTOCOL_NAME = b"h2"
+
+
 class InvalidNegotiatedProtocol(H2Error):
 
-    def __init__(self, negotiated_protocol: str) -> None:
+    def __init__(self, negotiated_protocol: bytes) -> None:
         self.negotiated_protocol = negotiated_protocol
 
     def __str__(self) -> str:
-        return f'InvalidNegotiatedProtocol: Expected h2 as negotiated protocol, received {self.negotiated_protocol!r}'
+        return (f"Expected {PROTOCOL_NAME!r}, received {self.negotiated_protocol!r}")
 
 
 class RemoteTerminatedConnection(H2Error):
@@ -52,7 +55,7 @@ class RemoteTerminatedConnection(H2Error):
         self.terminate_event = event
 
     def __str__(self) -> str:
-        return f'RemoteTerminatedConnection: Received GOAWAY frame from {self.remote_ip_address!r}'
+        return f'Received GOAWAY frame from {self.remote_ip_address!r}'
 
 
 class MethodNotAllowed405(H2Error):
@@ -60,7 +63,7 @@ class MethodNotAllowed405(H2Error):
         self.remote_ip_address = remote_ip_address
 
     def __str__(self) -> str:
-        return f"MethodNotAllowed405: Received 'HTTP/2.0 405 Method Not Allowed' from {self.remote_ip_address!r}"
+        return f"Received 'HTTP/2.0 405 Method Not Allowed' from {self.remote_ip_address!r}"
 
 
 @implementer(IHandshakeListener)
@@ -231,13 +234,12 @@ class H2ClientProtocol(Protocol, TimeoutMixin):
         self.transport.loseConnection()
 
     def handshakeCompleted(self) -> None:
-        """We close the connection with InvalidNegotiatedProtocol exception
-        when the connection was not made via h2 protocol"""
-        protocol = self.transport.negotiatedProtocol
-        if protocol is not None and protocol != b"h2":
-            # Here we have not initiated the connection yet
-            # So, no need to send a GOAWAY frame to the remote
-            self._lose_connection_with_error([InvalidNegotiatedProtocol(protocol.decode("utf-8"))])
+        """
+        Close the connection if it's not made via the expected protocol
+        """
+        if self.transport.negotiatedProtocol is not None and self.transport.negotiatedProtocol != PROTOCOL_NAME:
+            # we have not initiated the connection yet, no need to send a GOAWAY frame to the remote peer
+            self._lose_connection_with_error([InvalidNegotiatedProtocol(self.transport.negotiatedProtocol)])
 
     def _check_received_data(self, data: bytes) -> None:
         """Checks for edge cases where the connection to remote fails
@@ -414,4 +416,4 @@ class H2ClientFactory(Factory):
         return H2ClientProtocol(self.uri, self.settings, self.conn_lost_deferred)
 
     def acceptableProtocols(self) -> List[bytes]:
-        return [b'h2']
+        return [PROTOCOL_NAME]

tests/test_http2_client_protocol.py

@@ -5,6 +5,7 @@ import re
 import shutil
 import string
 from ipaddress import IPv4Address
+from unittest import mock
 from urllib.parse import urlencode
 
 from h2.exceptions import InvalidBodyLengthError
@@ -381,6 +382,13 @@ class Https2ClientProtocolTestCase(TestCase):
             200
         )
 
+    @inlineCallbacks
+    def test_invalid_negotiated_protocol(self):
+        with mock.patch("scrapy.core.http2.protocol.PROTOCOL_NAME", return_value=b"not-h2"):
+            request = Request(url=self.get_url('/status?n=200'))
+            with self.assertRaises(ResponseFailed):
+                yield self.make_request(request)
+
     def test_cancel_request(self):
         request = Request(url=self.get_url('/get-data-html-large'))