Merge pull request #2043 from Kriechi/http2

http2: disable priority forwarding

4 files changed, 77 insertions, 143 deletions

diff --git a/mitmproxy/options.py b/mitmproxy/options.py
index 630d2964..2467b9dd 100644
--- a/mitmproxy/options.py
+++ b/mitmproxy/options.py
@@ -54,6 +54,7 @@ class Options(optmanager.OptManager):
         server_replay_ignore_params: Sequence[str] = [],
         server_replay_ignore_payload_params: Sequence[str] = [],
         server_replay_ignore_host: bool = False,
+
         # Proxy options
         auth_nonanonymous: bool = False,
         auth_singleuser: Optional[str] = None,
@@ -65,15 +66,18 @@ class Options(optmanager.OptManager):
         ciphers_client: str=DEFAULT_CLIENT_CIPHERS,
         ciphers_server: Optional[str]=None,
         clientcerts: Optional[str] = None,
-        http2: bool = True,
         ignore_hosts: Sequence[str] = [],
         listen_host: str = "",
         listen_port: int = LISTEN_PORT,
         upstream_bind_address: str = "",
         mode: str = "regular",
         no_upstream_cert: bool = False,
-        rawtcp: bool = False,
+
+        http2: bool = True,
+        http2_priority: bool = False,
         websocket: bool = True,
+        rawtcp: bool = False,
+
         spoof_source_address: bool = False,
         upstream_server: Optional[str] = None,
         upstream_auth: Optional[str] = None,
@@ -152,15 +156,18 @@ class Options(optmanager.OptManager):
         self.ciphers_client = ciphers_client
         self.ciphers_server = ciphers_server
         self.clientcerts = clientcerts
-        self.http2 = http2
         self.ignore_hosts = ignore_hosts
         self.listen_host = listen_host
         self.listen_port = listen_port
         self.upstream_bind_address = upstream_bind_address
         self.mode = mode
         self.no_upstream_cert = no_upstream_cert
-        self.rawtcp = rawtcp
+
+        self.http2 = http2
+        self.http2_priority = http2_priority
         self.websocket = websocket
+        self.rawtcp = rawtcp
+
         self.spoof_source_address = spoof_source_address
         self.upstream_server = upstream_server
         self.upstream_auth = upstream_auth
diff --git a/mitmproxy/proxy/protocol/http2.py b/mitmproxy/proxy/protocol/http2.py
index b7548221..cdce24b3 100644
--- a/mitmproxy/proxy/protocol/http2.py
+++ b/mitmproxy/proxy/protocol/http2.py
@@ -268,6 +268,10 @@ class Http2Layer(base.Layer):
         return True
 
     def _handle_priority_updated(self, eid, event):
+        if not self.config.options.http2_priority:
+            self.log("HTTP/2 PRIORITY frame surpressed. Use --http2-priority to enable forwarding.", "debug")
+            return True
+
         if eid in self.streams and self.streams[eid].handled_priority_event is event:
             # this event was already handled during stream creation
             # HeadersFrame + Priority information as RequestReceived
@@ -527,9 +531,12 @@ class Http2SingleStreamLayer(httpbase._HttpTransmissionLayer, basethread.BaseThr
         if self.handled_priority_event:
             # only send priority information if they actually came with the original HeadersFrame
             # and not if they got updated before/after with a PriorityFrame
-            priority_exclusive = self.priority_exclusive
-            priority_depends_on = self._map_depends_on_stream_id(self.server_stream_id, self.priority_depends_on)
-            priority_weight = self.priority_weight
+            if not self.config.options.http2_priority:
+                self.log("HTTP/2 PRIORITY information in HEADERS frame surpressed. Use --http2-priority to enable forwarding.", "debug")
+            else:
+                priority_exclusive = self.priority_exclusive
+                priority_depends_on = self._map_depends_on_stream_id(self.server_stream_id, self.priority_depends_on)
+                priority_weight = self.priority_weight
 
         try:
             self.connections[self.server_conn].safe_send_headers(
@@ -610,7 +617,7 @@ class Http2SingleStreamLayer(httpbase._HttpTransmissionLayer, basethread.BaseThr
             chunks
         )
 
-    def __call__(self):
+    def __call__(self):  # pragma: no cover
         raise EnvironmentError('Http2SingleStreamLayer must be run as thread')
 
     def run(self):
diff --git a/mitmproxy/tools/cmdline.py b/mitmproxy/tools/cmdline.py
index 1c620fd6..bb0bb17a 100644
--- a/mitmproxy/tools/cmdline.py
+++ b/mitmproxy/tools/cmdline.py
@@ -137,7 +137,6 @@ def get_common_options(args):
         ciphers_client = args.ciphers_client,
         ciphers_server = args.ciphers_server,
         clientcerts = args.clientcerts,
-        http2 = args.http2,
         ignore_hosts = args.ignore_hosts,
         listen_host = args.addr,
         listen_port = args.port,
@@ -145,8 +144,12 @@ def get_common_options(args):
         mode = mode,
         no_upstream_cert = args.no_upstream_cert,
         spoof_source_address = args.spoof_source_address,
-        rawtcp = args.rawtcp,
+
+        http2 = args.http2,
+        http2_priority = args.http2_priority,
         websocket = args.websocket,
+        rawtcp = args.rawtcp,
+
         upstream_server = upstream_server,
         upstream_auth = args.upstream_auth,
         ssl_version_client = args.ssl_version_client,
@@ -334,18 +337,26 @@ def proxy_options(parser):
     )
 
     http2 = group.add_mutually_exclusive_group()
-    http2.add_argument("--no-http2", action="store_false", dest="http2",
+    http2.add_argument("--no-http2", action="store_false", dest="http2")
+    http2.add_argument("--http2", action="store_true", dest="http2",
                        help="Explicitly enable/disable HTTP/2 support. "
-                            "Enabled by default."
+                            "HTTP/2 support is enabled by default.",
                        )
-    http2.add_argument("--http2", action="store_true", dest="http2")
+
+    http2_priority = group.add_mutually_exclusive_group()
+    http2_priority.add_argument("--http2-priority", action="store_true", dest="http2_priority")
+    http2_priority.add_argument("--no-http2-priority", action="store_false", dest="http2_priority",
+                                help="Explicitly enable/disable PRIORITY forwarding for HTTP/2 connections. "
+                                     "PRIORITY forwarding is disabled by default, "
+                                     "because some webservers fail at implementing the RFC properly.",
+                                )
 
     websocket = group.add_mutually_exclusive_group()
-    websocket.add_argument("--no-websocket", action="store_false", dest="websocket",
+    websocket.add_argument("--no-websocket", action="store_false", dest="websocket")
+    websocket.add_argument("--websocket", action="store_true", dest="websocket",
                            help="Explicitly enable/disable WebSocket support. "
-                                "Enabled by default."
+                                "WebSocket support is enabled by default.",
                            )
-    websocket.add_argument("--websocket", action="store_true", dest="websocket")
 
     parser.add_argument(
         "--upstream-auth",
diff --git a/test/mitmproxy/proxy/protocol/test_http2.py b/test/mitmproxy/proxy/protocol/test_http2.py
index eec7af89..cb9c0474 100644
--- a/test/mitmproxy/proxy/protocol/test_http2.py
+++ b/test/mitmproxy/proxy/protocol/test_http2.py
@@ -4,7 +4,7 @@
 import os
 import tempfile
 import traceback
-
+import pytest
 import h2
 
 from mitmproxy import options
@@ -369,49 +369,15 @@ class TestRequestWithPriority(_Http2Test):
             wfile.flush()
         return True
 
-    def test_request_with_priority(self):
-        client, h2_conn = self._setup_connection()
-
-        self._send_request(
-            client.wfile,
-            h2_conn,
-            headers=[
-                (':authority', "127.0.0.1:{}".format(self.server.server.address.port)),
-                (':method', 'GET'),
-                (':scheme', 'https'),
-                (':path', '/'),
-            ],
-            priority_exclusive=True,
-            priority_depends_on=42424242,
-            priority_weight=42,
-        )
-
-        done = False
-        while not done:
-            try:
-                raw = b''.join(http2.read_raw_frame(client.rfile))
-                events = h2_conn.receive_data(raw)
-            except exceptions.HttpException:
-                print(traceback.format_exc())
-                assert False
-
-            client.wfile.write(h2_conn.data_to_send())
-            client.wfile.flush()
+    @pytest.mark.parametrize("http2_priority_enabled, priority, expected_priority", [
+        (True, (True, 42424242, 42), ('True', '42424242', '42')),
+        (False, (True, 42424242, 42), (None, None, None)),
+        (True, (None, None, None), (None, None, None)),
+        (False, (None, None, None), (None, None, None)),
+    ])
+    def test_request_with_priority(self, http2_priority_enabled, priority, expected_priority):
+        self.config.options.http2_priority = http2_priority_enabled
 
-            for event in events:
-                if isinstance(event, h2.events.StreamEnded):
-                    done = True
-
-        h2_conn.close_connection()
-        client.wfile.write(h2_conn.data_to_send())
-        client.wfile.flush()
-
-        assert len(self.master.state.flows) == 1
-        assert self.master.state.flows[0].response.headers['priority_exclusive'] == 'True'
-        assert self.master.state.flows[0].response.headers['priority_depends_on'] == '42424242'
-        assert self.master.state.flows[0].response.headers['priority_weight'] == '42'
-
-    def test_request_without_priority(self):
         client, h2_conn = self._setup_connection()
 
         self._send_request(
@@ -423,6 +389,9 @@ class TestRequestWithPriority(_Http2Test):
                 (':scheme', 'https'),
                 (':path', '/'),
             ],
+            priority_exclusive=priority[0],
+            priority_depends_on=priority[1],
+            priority_weight=priority[2],
         )
 
         done = False
@@ -446,21 +415,22 @@ class TestRequestWithPriority(_Http2Test):
         client.wfile.flush()
 
         assert len(self.master.state.flows) == 1
-        assert 'priority_exclusive' not in self.master.state.flows[0].response.headers
-        assert 'priority_depends_on' not in self.master.state.flows[0].response.headers
-        assert 'priority_weight' not in self.master.state.flows[0].response.headers
+
+        resp = self.master.state.flows[0].response
+        assert resp.headers.get('priority_exclusive', None) == expected_priority[0]
+        assert resp.headers.get('priority_depends_on', None) == expected_priority[1]
+        assert resp.headers.get('priority_weight', None) == expected_priority[2]
 
 
 @requires_alpn
 class TestPriority(_Http2Test):
-    priority_data = None
 
     @classmethod
     def handle_server_event(cls, event, h2_conn, rfile, wfile):
         if isinstance(event, h2.events.ConnectionTerminated):
             return False
         elif isinstance(event, h2.events.PriorityUpdated):
-            cls.priority_data = (event.exclusive, event.depends_on, event.weight)
+            cls.priority_data.append((event.exclusive, event.depends_on, event.weight))
         elif isinstance(event, h2.events.RequestReceived):
             import warnings
             with warnings.catch_warnings():
@@ -479,84 +449,22 @@ class TestPriority(_Http2Test):
             wfile.flush()
         return True
 
-    def test_priority(self):
-        client, h2_conn = self._setup_connection()
-
-        h2_conn.prioritize(1, exclusive=True, depends_on=0, weight=42)
-        client.wfile.write(h2_conn.data_to_send())
-        client.wfile.flush()
+    @pytest.mark.parametrize("prioritize_before", [True, False])
+    @pytest.mark.parametrize("http2_priority_enabled, priority, expected_priority", [
+        (True, (True, 42424242, 42), [(True, 42424242, 42)]),
+        (False, (True, 42424242, 42), []),
+    ])
+    def test_priority(self, prioritize_before, http2_priority_enabled, priority, expected_priority):
+        self.config.options.http2_priority = http2_priority_enabled
+        self.__class__.priority_data = []
 
-        self._send_request(
-            client.wfile,
-            h2_conn,
-            headers=[
-                (':authority', "127.0.0.1:{}".format(self.server.server.address.port)),
-                (':method', 'GET'),
-                (':scheme', 'https'),
-                (':path', '/'),
-            ],
-        )
-
-        done = False
-        while not done:
-            try:
-                raw = b''.join(http2.read_raw_frame(client.rfile))
-                events = h2_conn.receive_data(raw)
-            except exceptions.HttpException:
-                print(traceback.format_exc())
-                assert False
+        client, h2_conn = self._setup_connection()
 
+        if prioritize_before:
+            h2_conn.prioritize(1, exclusive=priority[0], depends_on=priority[1], weight=priority[2])
             client.wfile.write(h2_conn.data_to_send())
             client.wfile.flush()
 
-            for event in events:
-                if isinstance(event, h2.events.StreamEnded):
-                    done = True
-
-        h2_conn.close_connection()
-        client.wfile.write(h2_conn.data_to_send())
-        client.wfile.flush()
-
-        assert len(self.master.state.flows) == 1
-        assert self.priority_data == (True, 0, 42)
-
-
-@requires_alpn
-class TestPriorityWithExistingStream(_Http2Test):
-    priority_data = []
-
-    @classmethod
-    def handle_server_event(cls, event, h2_conn, rfile, wfile):
-        if isinstance(event, h2.events.ConnectionTerminated):
-            return False
-        elif isinstance(event, h2.events.PriorityUpdated):
-            cls.priority_data.append((event.exclusive, event.depends_on, event.weight))
-        elif isinstance(event, h2.events.RequestReceived):
-            assert not event.priority_updated
-
-            import warnings
-            with warnings.catch_warnings():
-                # Ignore UnicodeWarning:
-                # h2/utilities.py:64: UnicodeWarning: Unicode equal comparison
-                # failed to convert both arguments to Unicode - interpreting
-                # them as being unequal.
-                #     elif header[0] in (b'cookie', u'cookie') and len(header[1]) < 20:
-
-                warnings.simplefilter("ignore")
-
-                headers = [(':status', '200')]
-                h2_conn.send_headers(event.stream_id, headers)
-            wfile.write(h2_conn.data_to_send())
-            wfile.flush()
-        elif isinstance(event, h2.events.StreamEnded):
-            h2_conn.end_stream(event.stream_id)
-            wfile.write(h2_conn.data_to_send())
-            wfile.flush()
-        return True
-
-    def test_priority_with_existing_stream(self):
-        client, h2_conn = self._setup_connection()
-
         self._send_request(
             client.wfile,
             h2_conn,
@@ -566,13 +474,14 @@ class TestPriorityWithExistingStream(_Http2Test):
                 (':scheme', 'https'),
                 (':path', '/'),
             ],
-            end_stream=False,
+            end_stream=prioritize_before,
         )
 
-        h2_conn.prioritize(1, exclusive=True, depends_on=0, weight=42)
-        h2_conn.end_stream(1)
-        client.wfile.write(h2_conn.data_to_send())
-        client.wfile.flush()
+        if not prioritize_before:
+            h2_conn.prioritize(1, exclusive=priority[0], depends_on=priority[1], weight=priority[2])
+            h2_conn.end_stream(1)
+            client.wfile.write(h2_conn.data_to_send())
+            client.wfile.flush()
 
         done = False
         while not done:
@@ -595,7 +504,7 @@ class TestPriorityWithExistingStream(_Http2Test):
         client.wfile.flush()
 
         assert len(self.master.state.flows) == 1
-        assert self.priority_data == [(True, 0, 42)]
+        assert self.priority_data == expected_priority
 
 
 @requires_alpn