From 6326178bc5824e8c08d84b6543de6977a16fb8d2 Mon Sep 17 00:00:00 2001
From: Eugenio Lacuesta <1731933+elacuesta@users.noreply.github.com>
Date: Mon, 22 Feb 2021 12:50:51 -0300
Subject: [PATCH] http2: acceptable protocol update, tests (#4994)

---
 scrapy/core/http2/protocol.py       | 26 ++++++++++++++------------
 tests/test_http2_client_protocol.py |  8 ++++++++
 2 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/scrapy/core/http2/protocol.py b/scrapy/core/http2/protocol.py
index 9d7da14c1..6ca69b23b 100644
--- a/scrapy/core/http2/protocol.py
+++ b/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]
diff --git a/tests/test_http2_client_protocol.py b/tests/test_http2_client_protocol.py
index d9ab553f0..8b2f6a11d 100644
--- a/tests/test_http2_client_protocol.py
+++ b/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'))