From 8f516bfd81f728328808c60e8844a2829abc22ab Mon Sep 17 00:00:00 2001 From: Maximilian Hils Date: Thu, 10 Dec 2020 14:28:45 +0100 Subject: [PATCH] [sans-io] HTTP/2: reset half-closed streams on error --- mitmproxy/proxy2/layers/http/_base.py | 5 ---- mitmproxy/proxy2/layers/http/_http1.py | 3 +- mitmproxy/proxy2/layers/http/_http2.py | 16 +++++++---- .../proxy2/layers/http/test_http2.py | 28 +++++++++++++++++-- 4 files changed, 39 insertions(+), 13 deletions(-) diff --git a/mitmproxy/proxy2/layers/http/_base.py b/mitmproxy/proxy2/layers/http/_base.py index 69726fbe2..71e12184f 100644 --- a/mitmproxy/proxy2/layers/http/_base.py +++ b/mitmproxy/proxy2/layers/http/_base.py @@ -11,11 +11,6 @@ class HttpEvent(events.Event): # we need stream ids on every event to avoid race conditions stream_id: StreamId - def __repr__(self) -> str: - x = self.__dict__.copy() - x.pop("stream_id") - return f"{type(self).__name__}({repr(x) if x else ''})" - class HttpConnection(layer.Layer): conn: Connection diff --git a/mitmproxy/proxy2/layers/http/_http1.py b/mitmproxy/proxy2/layers/http/_http1.py index 6ece32578..18c720b62 100644 --- a/mitmproxy/proxy2/layers/http/_http1.py +++ b/mitmproxy/proxy2/layers/http/_http1.py @@ -107,7 +107,8 @@ class Http1Connection(HttpConnection, metaclass=abc.ABCMeta): # see https://github.com/httpwg/http-core/issues/22 if event.connection.state is not ConnectionState.CLOSED: yield commands.CloseConnection(event.connection) - yield ReceiveHttp(self.ReceiveProtocolError(self.stream_id, f"Client disconnected.")) + yield ReceiveHttp(self.ReceiveProtocolError(self.stream_id, f"Client disconnected.", + code=status_codes.CLIENT_CLOSED_REQUEST)) else: # pragma: no cover raise AssertionError(f"Unexpected event: {event}") diff --git a/mitmproxy/proxy2/layers/http/_http2.py b/mitmproxy/proxy2/layers/http/_http2.py index 4f36c7c46..702cd165a 100644 --- a/mitmproxy/proxy2/layers/http/_http2.py +++ b/mitmproxy/proxy2/layers/http/_http2.py @@ -14,7 +14,7 @@ import h2.utilities from mitmproxy import http from mitmproxy.net import http as net_http -from mitmproxy.net.http import url +from mitmproxy.net.http import url, status_codes from mitmproxy.utils import human from . import RequestData, RequestEndOfMessage, RequestHeaders, RequestProtocolError, ResponseData, \ ResponseEndOfMessage, ResponseHeaders, ResponseProtocolError @@ -86,9 +86,11 @@ class Http2Connection(HttpConnection): self.streams.pop(event.stream_id, None) elif isinstance(event, self.SendProtocolError): stream = self.h2_conn.streams.get(event.stream_id) - if stream.state_machine.state not in (h2.stream.StreamState.HALF_CLOSED_LOCAL, - h2.stream.StreamState.CLOSED): - self.h2_conn.reset_stream(event.stream_id, h2.errors.ErrorCodes.INTERNAL_ERROR) + if stream.state_machine.state is not h2.stream.StreamState.CLOSED: + code = { + status_codes.CLIENT_CLOSED_REQUEST: h2.errors.ErrorCodes.CANCEL, + }.get(event.code, h2.errors.ErrorCodes.INTERNAL_ERROR) + self.h2_conn.reset_stream(event.stream_id, code) if self.is_closed(event.stream_id): self.streams.pop(event.stream_id, None) else: @@ -150,7 +152,11 @@ class Http2Connection(HttpConnection): err_str = h2.errors.ErrorCodes(event.error_code).name except ValueError: err_str = str(event.error_code) - yield ReceiveHttp(self.ReceiveProtocolError(event.stream_id, f"stream reset by client ({err_str})")) + err_code = { + h2.errors.ErrorCodes.CANCEL: status_codes.CLIENT_CLOSED_REQUEST, + }.get(event.error_code, self.ReceiveProtocolError.code) + yield ReceiveHttp(self.ReceiveProtocolError(event.stream_id, f"stream reset by client ({err_str})", + code=err_code)) self.streams.pop(event.stream_id) else: pass # We don't track priority frames which could be followed by a stream reset here. diff --git a/test/mitmproxy/proxy2/layers/http/test_http2.py b/test/mitmproxy/proxy2/layers/http/test_http2.py index 16c7bc42c..67985f536 100644 --- a/test/mitmproxy/proxy2/layers/http/test_http2.py +++ b/test/mitmproxy/proxy2/layers/http/test_http2.py @@ -8,13 +8,13 @@ from h2.errors import ErrorCodes from mitmproxy.flow import Error from mitmproxy.http import HTTPFlow -from mitmproxy.net.http import Headers +from mitmproxy.net.http import Headers, Request, status_codes from mitmproxy.proxy.protocol.http import HTTPMode from mitmproxy.proxy2.commands import CloseConnection, OpenConnection, SendData from mitmproxy.proxy2.context import Context, Server from mitmproxy.proxy2.events import ConnectionClosed, DataReceived from mitmproxy.proxy2.layers import http -from mitmproxy.proxy2.layers.http._http2 import split_pseudo_headers +from mitmproxy.proxy2.layers.http._http2 import split_pseudo_headers, Http2Client from test.mitmproxy.proxy2.layers.http.hyper_h2_test_helpers import FrameFactory from test.mitmproxy.proxy2.tutils import Placeholder, Playbook, reply @@ -504,3 +504,27 @@ def test_kill_stream(tctx): hyperframe.frame.SettingsFrame, hyperframe.frame.HeadersFrame, ] + + +class TestClient: + def test_no_data_on_closed_stream(self, tctx): + frame_factory = FrameFactory() + req = Request.make("GET", "http://example.com/") + resp = { + ":status" : 200 + } + assert ( + Playbook(Http2Client(tctx)) + << SendData(tctx.server, Placeholder(bytes)) # preamble + initial settings frame + >> DataReceived(tctx.server, frame_factory.build_settings_frame({}, ack=True).serialize()) + >> http.RequestHeaders(1, req, end_stream=True) + << SendData(tctx.server, b"\x00\x00\x06\x01\x05\x00\x00\x00\x01\x82\x86\x84\\\x81\x07") + >> http.RequestEndOfMessage(1) + >> DataReceived(tctx.server, frame_factory.build_headers_frame(resp).serialize()) + << http.ReceiveHttp(Placeholder(http.ResponseHeaders)) + >> http.RequestProtocolError(1, "cancelled", code=status_codes.CLIENT_CLOSED_REQUEST) + << SendData(tctx.server, frame_factory.build_rst_stream_frame(1, ErrorCodes.CANCEL).serialize()) + >> DataReceived(tctx.server, frame_factory.build_data_frame(b"foo").serialize()) + << SendData(tctx.server, frame_factory.build_rst_stream_frame(1, ErrorCodes.STREAM_CLOSED).serialize()) + # important: no ResponseData event here! + )