diff --git a/mitmproxy/proxy2/layers/http/__init__.py b/mitmproxy/proxy2/layers/http/__init__.py index 1c0f41a0c..2f122b5ef 100644 --- a/mitmproxy/proxy2/layers/http/__init__.py +++ b/mitmproxy/proxy2/layers/http/__init__.py @@ -8,7 +8,7 @@ from mitmproxy.net import server_spec from mitmproxy.net.http import url from mitmproxy.proxy.protocol.http import HTTPMode from mitmproxy.proxy2 import commands, events, layer, tunnel -from mitmproxy.proxy2.context import Connection, Context, Server +from mitmproxy.proxy2.context import Connection, ConnectionState, Context, Server from mitmproxy.proxy2.layers import tls from mitmproxy.proxy2.layers.http import _upstream_proxy from mitmproxy.proxy2.utils import expect @@ -283,8 +283,15 @@ class HttpStream(layer.Layer): self.server_state = self.state_done def check_killed(self) -> layer.CommandGenerator[bool]: - if self.flow.error and self.flow.error.msg == flow.Error.KILLED_MESSAGE: - yield commands.CloseConnection(self.context.client) + killed_by_us = ( + self.flow.error and self.flow.error.msg == flow.Error.KILLED_MESSAGE + ) + killed_by_remote = ( + self.context.client.state is not ConnectionState.OPEN + ) + if killed_by_us or killed_by_remote: + if self.context.client.state & ConnectionState.CAN_WRITE: + yield commands.CloseConnection(self.context.client) self._handle_event = self.state_errored return True return False @@ -302,12 +309,29 @@ class HttpStream(layer.Layer): self, event: typing.Union[RequestProtocolError, ResponseProtocolError] ) -> layer.CommandGenerator[None]: - self.flow.error = flow.Error(event.message) - yield HttpErrorHook(self.flow) + is_client_error_but_we_already_talk_upstream = ( + isinstance(event, RequestProtocolError) and + self.client_state in (self.state_stream_request_body, self.state_done) + ) + if is_client_error_but_we_already_talk_upstream: + yield SendHttp(event, self.context.server) + self.client_state = self.state_errored + + response_hook_already_triggered = ( + self.server_state in (self.state_done, self.state_errored) + ) + if not response_hook_already_triggered: + # We don't want to trigger both a response hook and an error hook, + # so we need to check if the response is done yet or not. + self.flow.error = flow.Error(event.message) + yield HttpErrorHook(self.flow) + if (yield from self.check_killed()): return - elif isinstance(event, ResponseProtocolError): + + if isinstance(event, ResponseProtocolError): yield SendHttp(event, self.context.client) + self.server_state = self.state_errored def make_server_connection(self) -> layer.CommandGenerator[bool]: connection, err = yield GetHttpConnection( diff --git a/mitmproxy/proxy2/layers/http/_http2.py b/mitmproxy/proxy2/layers/http/_http2.py index ab1c1a401..25dabcd33 100644 --- a/mitmproxy/proxy2/layers/http/_http2.py +++ b/mitmproxy/proxy2/layers/http/_http2.py @@ -127,7 +127,11 @@ class Http2Connection(HttpConnection): self.streams.pop(event.stream_id, None) elif isinstance(event, h2.events.StreamReset): if event.stream_id in self.streams: - yield ReceiveHttp(self.ReceiveProtocolError(event.stream_id, f"Stream reset, error code {event.error_code}")) + try: + 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})")) 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_http.py b/test/mitmproxy/proxy2/layers/http/test_http.py index 4f5129331..e9ea0bf82 100644 --- a/test/mitmproxy/proxy2/layers/http/test_http.py +++ b/test/mitmproxy/proxy2/layers/http/test_http.py @@ -359,8 +359,6 @@ def test_request_streaming(tctx, response): << SendData(tctx.client, b"HTTP/1.1 413 Request Entity Too Large\r\nContent-Length: 0\r\n\r\n") >> ConnectionClosed(server) << CloseConnection(server) - << http.HttpErrorHook(flow) - >> reply() << CloseConnection(tctx.client) ) elif response == "early kill": @@ -717,13 +715,18 @@ def test_http_client_aborts(tctx, stream): ) else: assert playbook >> reply() - assert ( + ( playbook >> ConnectionClosed(tctx.client) << CloseConnection(tctx.client) + ) + if stream: + playbook << CloseConnection(server) + assert ( + playbook << http.HttpErrorHook(flow) >> reply() - + << None ) assert "peer closed connection" in flow().error.msg @@ -861,3 +864,16 @@ def test_kill_flow(tctx, when): return assert_kill() else: raise AssertionError + + +def test_close_during_connect_hook(tctx): + flow = Placeholder(HTTPFlow) + assert ( + Playbook(http.HttpLayer(tctx, HTTPMode.regular)) + >> DataReceived(tctx.client, + b'CONNECT hi.ls:443 HTTP/1.1\r\nProxy-Connection: keep-alive\r\nConnection: keep-alive\r\nHost: hi.ls:443\r\n\r\n') + << http.HttpConnectHook(flow) + >> ConnectionClosed(tctx.client) + << CloseConnection(tctx.client) + >> reply(to=-3) + ) diff --git a/test/mitmproxy/proxy2/layers/http/test_http2.py b/test/mitmproxy/proxy2/layers/http/test_http2.py index 0a512adf3..c0ed03c03 100644 --- a/test/mitmproxy/proxy2/layers/http/test_http2.py +++ b/test/mitmproxy/proxy2/layers/http/test_http2.py @@ -101,7 +101,7 @@ def test_simple(tctx): assert flow().response.text == "Hello, World!" -@pytest.mark.parametrize("stream", ["stream", "block"]) +@pytest.mark.parametrize("stream", ["stream", ""]) @pytest.mark.parametrize("when", ["request", "response"]) @pytest.mark.parametrize("how", ["RST", "disconnect", "RST+disconnect"]) def test_http2_client_aborts(tctx, stream, when, how): @@ -127,7 +127,7 @@ def test_http2_client_aborts(tctx, stream, when, how): >> DataReceived(tctx.client, cff.build_headers_frame(example_request_headers).serialize()) << http.HttpRequestHeadersHook(flow) ) - if stream == "stream" and when == "request": + if stream and when == "request": assert ( playbook >> reply(side_effect=enable_request_streaming) @@ -144,16 +144,21 @@ def test_http2_client_aborts(tctx, stream, when, how): if when == "request": if "RST" in how: playbook >> DataReceived(tctx.client, cff.build_rst_stream_frame(1, ErrorCodes.CANCEL).serialize()) - playbook << http.HttpErrorHook(flow) - playbook >> reply() - if "disconnect" in how: + else: playbook >> ConnectionClosed(tctx.client) playbook << CloseConnection(tctx.client) - if "RST" not in how: - playbook << http.HttpErrorHook(flow) - playbook >> reply() + + if stream: + playbook << CloseConnection(server) + playbook << http.HttpErrorHook(flow) + playbook >> reply() + + if how == "RST+disconnect": + playbook >> ConnectionClosed(tctx.client) + playbook << CloseConnection(tctx.client) + assert playbook - assert any(x in flow().error.msg for x in ["Stream reset", "peer closed connection"]) + assert "stream reset" in flow().error.msg or "peer closed connection" in flow().error.msg return assert ( @@ -168,7 +173,7 @@ def test_http2_client_aborts(tctx, stream, when, how): >> DataReceived(server, b"HTTP/1.1 200 OK\r\nContent-Length: 6\r\n\r\n123") << http.HttpResponseHeadersHook(flow) ) - if stream == "stream": + if stream: assert ( playbook >> reply(side_effect=enable_response_streaming) @@ -179,17 +184,28 @@ def test_http2_client_aborts(tctx, stream, when, how): if "RST" in how: playbook >> DataReceived(tctx.client, cff.build_rst_stream_frame(1, ErrorCodes.CANCEL).serialize()) - if "disconnect" in how: + else: playbook >> ConnectionClosed(tctx.client) playbook << CloseConnection(tctx.client) + assert ( playbook - >> DataReceived(server, b"456") - << http.HttpResponseHook(flow) + << CloseConnection(server) + << http.HttpErrorHook(flow) >> reply() ) - if stream != "stream": - assert flow().response.content == b"123456" + + if how == "RST+disconnect": + assert ( + playbook + >> ConnectionClosed(tctx.client) + << CloseConnection(tctx.client) + ) + + if "RST" in how: + assert "stream reset" in flow().error.msg + else: + assert "peer closed connection" in flow().error.msg def test_no_normalization(tctx): @@ -268,7 +284,6 @@ def test_rst_then_close(tctx): playbook, cff = start_h2_client(tctx) flow = Placeholder(HTTPFlow) server = Placeholder(Server) - open_conn = OpenConnection(server) assert ( playbook @@ -278,12 +293,13 @@ def test_rst_then_close(tctx): >> reply() << http.HttpRequestHook(flow) >> reply() - << open_conn + << OpenConnection(server) >> DataReceived(tctx.client, cff.build_data_frame(b"unexpected data frame").serialize()) << SendData(tctx.client, cff.build_rst_stream_frame(1, ErrorCodes.STREAM_CLOSED).serialize()) >> ConnectionClosed(tctx.client) << CloseConnection(tctx.client) - >> reply("connection cancelled", to=open_conn) + >> reply("connection cancelled", to=-5) << http.HttpErrorHook(flow) >> reply() ) + assert flow().error.msg == "connection cancelled"