Merge pull request #452 from python-hyper/issue-451

Resolve issue with plaintext upgrade on the server side

Author: Lukasa

HISTORY.rst

@@ -25,6 +25,9 @@ Bugfixes
 - Resolved issue where the ``HTTP2-Settings`` header value for plaintext
   upgrade that was emitted by ``initiate_upgrade_connection`` included the
   *entire* ``SETTINGS`` frame, instead of just the payload.
+- Resolved issue where the ``HTTP2-Settings`` header value sent by a client for
+  plaintext upgrade would be ignored by ``initiate_upgrade_connection``, rather
+  than have those settings applied appropriately.
 
 2.5.0 (2016-10-25)
 ------------------

h2/connection.py

@@ -588,19 +588,29 @@ def initiate_upgrade_connection(self, settings_header=None):
         """
         frame_data = None
 
+        # Begin by getting the preamble in place.
+        self.initiate_connection()
+
         if self.config.client_side:
             f = SettingsFrame(0)
             for setting, value in self.local_settings.items():
                 f.settings[setting] = value
 
             frame_data = f.serialize_body()
             frame_data = base64.urlsafe_b64encode(frame_data)
+        elif settings_header:
+            # We have a settings header from the client. This needs to be
+            # applied, but we want to throw away the ACK. We do this by
+            # inserting the data into a Settings frame and then passing it to
+            # the state machine, but ignoring the return value.
+            settings_header = base64.urlsafe_b64decode(settings_header)
+            f = SettingsFrame(0)
+            f.parse_body(settings_header)
+            self._receive_settings_frame(f)
 
         # Set up appropriate state. Stream 1 in a half-closed state:
         # half-closed(local) for clients, half-closed(remote) for servers.
         # Additionally, we need to set up the Connection state machine.
-        self.initiate_connection()
-
         connection_input = (
             ConnectionInputs.SEND_HEADERS if self.config.client_side
             else ConnectionInputs.RECV_HEADERS

test/test_h2_upgrade.py

@@ -160,7 +160,7 @@ def test_returns_nothing(self, frame_factory):
         Calling initiate_upgrade_connection returns nothing.
         """
         conn = h2.connection.H2Connection(client_side=False)
-        curl_header = "AAMAAABkAAQAAP__"
+        curl_header = b"AAMAAABkAAQAAP__"
         data = conn.initiate_upgrade_connection(curl_header)
         assert data is None
 
@@ -263,3 +263,42 @@ def test_cannot_receive_data_stream_1(self, frame_factory):
             error_code=h2.errors.ErrorCodes.STREAM_CLOSED,
         )
         assert c.data_to_send() == expected_frame.serialize()
+
+    def test_client_settings_are_applied(self, frame_factory):
+        """
+        The settings provided by the client are applied and immediately
+        ACK'ed.
+        """
+        server = h2.connection.H2Connection(client_side=False)
+        client = h2.connection.H2Connection(client_side=True)
+
+        # As a precaution, let's confirm that the server and client, at the
+        # start of the connection, do not agree on their initial settings
+        # state.
+        assert (
+            client.local_settings._settings != server.remote_settings._settings
+        )
+
+        # Get the client header data and pass it to the server.
+        header_data = client.initiate_upgrade_connection()
+        server.initiate_upgrade_connection(header_data)
+
+        # This gets complex, but here we go.
+        # RFC 7540 § 3.2.1 says that "explicit acknowledgement" of the settings
+        # in the header is "not necessary". That's annoyingly vague, but we
+        # interpret that to mean "should not be sent". So to test that this
+        # worked we need to test that the server has only sent the preamble,
+        # and has not sent a SETTINGS ack, and also that the server has the
+        # correct settings.
+        expected_frame = frame_factory.build_settings_frame(
+            server.local_settings
+        )
+        assert server.data_to_send() == expected_frame.serialize()
+
+        # We violate abstraction layers here, but I don't think defining __eq__
+        # for this is worth it. In this case, both the client and server should
+        # agree that these settings have been ACK'd, so their underlying
+        # dictionaries should be identical.
+        assert (
+            client.local_settings._settings == server.remote_settings._settings
+        )