From b28950d5e701055f536f66c70b581c3cf806f3dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Hoguin?= Date: Fri, 6 Nov 2020 11:52:03 +0100 Subject: Fix IsFin mismatch in HTTP/2 CONNECT response The response ends when the tunnel is established, even if the stream itself does not. The data coming in on the stream after is not part of the response. This makes both HTTP/1.1 and HTTP/2 send 'fin' to successful CONNECT responses. --- src/gun_http2.erl | 2 +- test/event_SUITE.erl | 69 ++++++++++++++++------------------------------- test/raw_SUITE.erl | 4 +-- test/rfc6265bis_SUITE.erl | 6 ++--- test/rfc7540_SUITE.erl | 9 +++---- test/socks_SUITE.erl | 2 +- test/tunnel_SUITE.erl | 6 ++--- 7 files changed, 35 insertions(+), 63 deletions(-) diff --git a/src/gun_http2.erl b/src/gun_http2.erl index 037c193..8f0632b 100644 --- a/src/gun_http2.erl +++ b/src/gun_http2.erl @@ -510,7 +510,7 @@ headers_frame_connect(State=#http2_state{transport=Transport, opts=Opts, tunnel_ origin_host => DestHost, origin_port => DestPort }, - ReplyTo ! {gun_response, self(), RealStreamRef, nofin, Status, Headers}, + ReplyTo ! {gun_response, self(), RealStreamRef, fin, Status, Headers}, EvHandlerState1 = EvHandler:response_headers(#{ stream_ref => RealStreamRef, reply_to => ReplyTo, diff --git a/test/event_SUITE.erl b/test/event_SUITE.erl index 8be10b1..ca7c55d 100644 --- a/test/event_SUITE.erl +++ b/test/event_SUITE.erl @@ -530,8 +530,7 @@ do_request_event_connect(Config, EventName) -> %% Gun doesn't send headers with an HTTP/2 CONNECT request %% so we only check that the headers are given as a list. true = is_list(Headers1), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, Protocol} = gun:await(ConnPid, StreamRef1), StreamRef2 = gun:get(ConnPid, "/", [], #{tunnel => StreamRef1}), #{ @@ -575,8 +574,7 @@ do_request_event_headers_connect(Config, EventName) -> %% Gun doesn't send headers with an HTTP/2 CONNECT request %% so we only check that the headers are given as a list. true = is_list(Headers1), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, Protocol} = gun:await(ConnPid, StreamRef1), StreamRef2 = gun:put(ConnPid, "/", [ {<<"content-type">>, <<"text/plain">>} @@ -685,8 +683,7 @@ do_request_end_connect(Config, EventName) -> stream_ref := StreamRef1, reply_to := ReplyTo } = do_receive_event(EventName), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, Protocol} = gun:await(ConnPid, StreamRef1), StreamRef2 = gun:get(ConnPid, "/", [], #{tunnel => StreamRef1}), #{ @@ -715,8 +712,7 @@ do_request_end_headers_connect(Config, EventName) -> stream_ref := StreamRef1, reply_to := ReplyTo } = do_receive_event(EventName), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, Protocol} = gun:await(ConnPid, StreamRef1), StreamRef2 = gun:put(ConnPid, "/", [ {<<"content-type">>, <<"text/plain">>} @@ -749,8 +745,7 @@ do_request_end_headers_content_length_connect(Config, EventName) -> stream_ref := StreamRef1, reply_to := ReplyTo } = do_receive_event(EventName), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, Protocol} = gun:await(ConnPid, StreamRef1), StreamRef2 = gun:put(ConnPid, "/", [ {<<"content-type">>, <<"text/plain">>}, @@ -784,8 +779,7 @@ do_request_end_headers_content_length_0_connect(Config, EventName) -> stream_ref := StreamRef1, reply_to := ReplyTo } = do_receive_event(EventName), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, Protocol} = gun:await(ConnPid, StreamRef1), StreamRef2 = gun:put(ConnPid, "/", [ {<<"content-type">>, <<"text/plain">>}, @@ -836,8 +830,7 @@ do_push_promise_start_connect(Config, ProxyProtocol) -> port => OriginPort, protocols => [http2] }, []), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, http2} = gun:await(ConnPid, StreamRef1), StreamRef2 = gun:get(ConnPid, "/push", [], #{tunnel => StreamRef1}), ReplyTo = self(), @@ -895,8 +888,7 @@ do_push_promise_end_connect(Config, ProxyProtocol) -> port => OriginPort, protocols => [http2] }, []), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, http2} = gun:await(ConnPid, StreamRef1), StreamRef2 = gun:get(ConnPid, "/push", [], #{tunnel => StreamRef1}), ReplyTo = self(), @@ -966,8 +958,7 @@ response_start_connect(Config) -> stream_ref := StreamRef1, reply_to := ReplyTo } = do_receive_event(response_start), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, Protocol} = gun:await(ConnPid, StreamRef1), StreamRef2 = gun:get(ConnPid, "/", [], #{tunnel => StreamRef1}), #{ @@ -1014,8 +1005,7 @@ response_inform_connect(Config) -> port => OriginPort, protocols => [Protocol] }, []), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, Protocol} = gun:await(ConnPid, StreamRef1), StreamRef2 = gun:get(ConnPid, "/inform", [], #{tunnel => StreamRef1}), #{ @@ -1071,8 +1061,7 @@ response_headers_connect(Config) -> headers := Headers1 } = do_receive_event(response_headers), true = is_list(Headers1), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, Protocol} = gun:await(ConnPid, StreamRef1), StreamRef2 = gun:get(ConnPid, "/", [], #{tunnel => StreamRef1}), #{ @@ -1101,8 +1090,7 @@ response_trailers(Config) -> port => OriginPort, protocols => [Protocol] }, []), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, Protocol} = gun:await(ConnPid, StreamRef1), StreamRef2 = gun:get(ConnPid, "/trailers", [{<<"te">>, <<"trailers">>}], #{tunnel => StreamRef1}), #{ @@ -1159,8 +1147,7 @@ do_response_end_connect(Config, EventName, Path) -> % stream_ref := StreamRef1, % reply_to := ReplyTo % } = do_receive_event(EventName), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, Protocol} = gun:await(ConnPid, StreamRef1), StreamRef2 = gun:get(ConnPid, Path, [{<<"te">>, <<"trailers">>}], #{tunnel => StreamRef1}), #{ @@ -1213,8 +1200,7 @@ http1_response_end_body_close(Config) -> %% stream_ref := StreamRef1, %% reply_to := ReplyTo %% } = do_receive_event(EventName), -% %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... -% {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), +% {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), % {up, http} = gun:await(ConnPid, StreamRef1), % StreamRef2 = gun:get(ConnPid, "/stream", [], #{tunnel => StreamRef1}), % #{ @@ -1265,8 +1251,7 @@ do_ws_upgrade_connect(Config, ProxyProtocol) -> http2 -> {http2, #{notify_settings_changed => true}} end] }, []), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, OriginProtocol} = gun:await(ConnPid, StreamRef1), ws_SUITE:do_await_enable_connect_protocol(OriginProtocol, ConnPid), StreamRef2 = gun:ws_upgrade(ConnPid, "/ws", [], #{tunnel => StreamRef1}), @@ -1374,8 +1359,7 @@ do_ws_upgrade_all_events_connect(Config, ProxyProtocol) -> http2 -> {http2, #{notify_settings_changed => true}} end] }, []), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, OriginProtocol} = gun:await(ConnPid, StreamRef1), ws_SUITE:do_await_enable_connect_protocol(OriginProtocol, ConnPid), %% Skip all CONNECT-related events that may conflict. @@ -1502,8 +1486,7 @@ do_ws_recv_frame_start_connect(Config, ProxyProtocol) -> http2 -> {http2, #{notify_settings_changed => true}} end] }, []), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, OriginProtocol} = gun:await(ConnPid, StreamRef1), ws_SUITE:do_await_enable_connect_protocol(OriginProtocol, ConnPid), StreamRef2 = gun:ws_upgrade(ConnPid, "/ws", [], #{tunnel => StreamRef1}), @@ -1564,8 +1547,7 @@ do_ws_recv_frame_header_connect(Config, ProxyProtocol) -> http2 -> {http2, #{notify_settings_changed => true}} end] }, []), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, OriginProtocol} = gun:await(ConnPid, StreamRef1), ws_SUITE:do_await_enable_connect_protocol(OriginProtocol, ConnPid), StreamRef2 = gun:ws_upgrade(ConnPid, "/ws", [], #{tunnel => StreamRef1}), @@ -1627,8 +1609,7 @@ do_ws_recv_frame_end_connect(Config, ProxyProtocol) -> http2 -> {http2, #{notify_settings_changed => true}} end] }, []), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, OriginProtocol} = gun:await(ConnPid, StreamRef1), ws_SUITE:do_await_enable_connect_protocol(OriginProtocol, ConnPid), StreamRef2 = gun:ws_upgrade(ConnPid, "/ws", [], #{tunnel => StreamRef1}), @@ -1701,8 +1682,7 @@ do_ws_send_frame_connect(Config, ProxyProtocol, EventName) -> http2 -> {http2, #{notify_settings_changed => true}} end] }, []), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, OriginProtocol} = gun:await(ConnPid, StreamRef1), ws_SUITE:do_await_enable_connect_protocol(OriginProtocol, ConnPid), StreamRef2 = gun:ws_upgrade(ConnPid, "/ws", [], #{tunnel => StreamRef1}), @@ -1754,8 +1734,7 @@ do_ws_protocol_changed_connect(Config, ProxyProtocol) -> http2 -> {http2, #{notify_settings_changed => true}} end] }, []), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, OriginProtocol} = gun:await(ConnPid, StreamRef1), ws_SUITE:do_await_enable_connect_protocol(OriginProtocol, ConnPid), #{ @@ -1955,8 +1934,7 @@ cancel_connect(Config) -> port => OriginPort, protocols => [Protocol] }), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, Protocol} = gun:await(ConnPid, StreamRef1), StreamRef2 = gun:post(ConnPid, "/stream", [], #{tunnel => StreamRef1}), gun:cancel(ConnPid, StreamRef2), @@ -1987,8 +1965,7 @@ cancel_remote_connect(Config) -> port => OriginPort, protocols => [Protocol] }), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, Protocol} = gun:await(ConnPid, StreamRef1), StreamRef2 = gun:post(ConnPid, "/stream", [], #{tunnel => StreamRef1}), ReplyTo = self(), diff --git a/test/raw_SUITE.erl b/test/raw_SUITE.erl index d909dce..a3b06c7 100644 --- a/test/raw_SUITE.erl +++ b/test/raw_SUITE.erl @@ -139,7 +139,7 @@ do_connect_raw(OriginTransport, ProxyTransport) -> protocols => [raw] }), {request, <<"CONNECT">>, Authority, 'HTTP/1.1', _} = receive_from(ProxyPid), - {response, fin, 200, _} = gun:await(ConnPid, StreamRef), %% @todo Why fin? + {response, fin, 200, _} = gun:await(ConnPid, StreamRef), handshake_completed = receive_from(OriginPid), {up, raw} = gun:await(ConnPid, StreamRef), gun:data(ConnPid, StreamRef, nofin, <<"Hello world!">>), @@ -294,7 +294,7 @@ do_http2_connect_raw(OriginTransport, ProxyScheme, ProxyTransport) -> <<":method">> := <<"CONNECT">>, <<":authority">> := Authority }} = receive_from(ProxyPid), - {response, nofin, 200, _} = gun:await(ConnPid, StreamRef), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef), handshake_completed = receive_from(OriginPid), {up, raw} = gun:await(ConnPid, StreamRef), gun:data(ConnPid, StreamRef, nofin, <<"Hello world!">>), diff --git a/test/rfc6265bis_SUITE.erl b/test/rfc6265bis_SUITE.erl index b4b443d..4301fc3 100644 --- a/test/rfc6265bis_SUITE.erl +++ b/test/rfc6265bis_SUITE.erl @@ -193,8 +193,7 @@ set_cookie_connect_tcp(Config) -> transport => Transport, protocols => [Protocol] }), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, Protocol} = gun:await(ConnPid, StreamRef1), StreamRef2 = gun:get(ConnPid, "/cookie-set?prefix", #{ <<"please-set-cookie">> => <<"a=b">> @@ -226,8 +225,7 @@ set_cookie_connect_tls(Config) -> transport => Transport, protocols => [Protocol] }), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, Protocol} = gun:await(ConnPid, StreamRef1), StreamRef2 = gun:get(ConnPid, "/cookie-set?prefix", #{ <<"please-set-cookie">> => <<"a=b">> diff --git a/test/rfc7540_SUITE.erl b/test/rfc7540_SUITE.erl index 8680031..161c59f 100644 --- a/test/rfc7540_SUITE.erl +++ b/test/rfc7540_SUITE.erl @@ -521,7 +521,7 @@ do_connect_http(OriginScheme, OriginTransport, OriginProtocol, ProxyScheme, Prox <<":method">> := <<"CONNECT">>, <<":authority">> := Authority }} = receive_from(ProxyPid), - {response, nofin, 200, _} = gun:await(ConnPid, StreamRef), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef), handshake_completed = receive_from(OriginPid), {up, OriginProtocol} = gun:await(ConnPid, StreamRef), ProxiedStreamRef = gun:get(ConnPid, "/proxied", #{}, #{tunnel => StreamRef}), @@ -624,7 +624,7 @@ do_connect_cowboy(_OriginScheme, OriginTransport, OriginProtocol, _ProxyScheme, <<":method">> := <<"CONNECT">>, <<":authority">> := Authority }} = receive_from(ProxyPid), - {response, nofin, 200, _} = gun:await(ConnPid, StreamRef), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef), {up, OriginProtocol} = gun:await(ConnPid, StreamRef), ProxiedStreamRef = gun:get(ConnPid, "/proxied", #{}, #{tunnel => StreamRef}), timer:sleep(1000), %% @todo Why? @@ -673,7 +673,7 @@ connect_handshake_timeout(_) -> port => OriginPort, protocols => [{http2, #{preface_timeout => 500}}] }), - {response, nofin, 200, _} = gun:await(ConnPid, StreamRef), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef), {up, http2} = gun:await(ConnPid, StreamRef), %% @todo The error should be normalized. %% @todo Do we want to indicate that a connection_error occurred within the tunnel stream? @@ -721,7 +721,7 @@ do_connect_via_multiple_proxies(OriginTransport, OriginProtocol, <<":method">> := <<"CONNECT">>, <<":authority">> := Authority1 }} = receive_from(Proxy1Pid), - {response, nofin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, Proxy2Protocol} = gun:await(ConnPid, StreamRef1), %% Origin. StreamRef2 = gun:connect(ConnPid, #{ @@ -732,7 +732,6 @@ do_connect_via_multiple_proxies(OriginTransport, OriginProtocol, }, [], #{tunnel => StreamRef1}), Authority2 = iolist_to_binary(["localhost:", integer_to_binary(OriginPort)]), {request, <<"CONNECT">>, Authority2, 'HTTP/1.1', _} = receive_from(Proxy2Pid), - %% @todo OK there's a mismatch between HTTP/1.1 (fin) and HTTP/2 (nofin). {response, fin, 200, _} = gun:await(ConnPid, StreamRef2), {up, OriginProtocol} = gun:await(ConnPid, StreamRef2), %% Tunneled request to the origin. diff --git a/test/socks_SUITE.erl b/test/socks_SUITE.erl index f6686a3..e23124f 100644 --- a/test/socks_SUITE.erl +++ b/test/socks_SUITE.erl @@ -449,7 +449,7 @@ do_socks5_through_h2_connect_proxy(_OriginScheme, OriginTransport, ProxyScheme, <<":method">> := <<"CONNECT">>, <<":authority">> := Authority1 }} = receive_from(Proxy1Pid), - {response, nofin, 200, _} = gun:await(ConnPid, StreamRef), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef), %% First the HTTP/2 tunnel is up, then the SOCKS tunnel to the origin HTTP server. {up, socks} = gun:await(ConnPid, StreamRef), {up, http} = gun:await(ConnPid, StreamRef), diff --git a/test/tunnel_SUITE.erl b/test/tunnel_SUITE.erl index 6cc50f8..e5fad34 100644 --- a/test/tunnel_SUITE.erl +++ b/test/tunnel_SUITE.erl @@ -886,8 +886,7 @@ do_proxy2(State=#st{proxy2=Type, proxy2_pid=Proxy2Pid, proxy2_port=Port}, ConnPi Protocol end] }), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef1), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef1), {up, Protocol} = gun:await(ConnPid, StreamRef1), do_handshake_completed(Protocol, Proxy2Pid), StreamRef1. @@ -908,8 +907,7 @@ do_origin(#st{origin=Type, origin_port=Port}, ConnPid, StreamRef1) -> transport => Transport, protocols => [Protocol] }, [], #{tunnel => StreamRef1}), - %% @todo _IsFin is 'fin' for HTTP and 'nofin' for HTTP/2... - {response, _IsFin, 200, _} = gun:await(ConnPid, StreamRef2), + {response, fin, 200, _} = gun:await(ConnPid, StreamRef2), {up, Protocol} = gun:await(ConnPid, StreamRef2), StreamRef2. -- cgit v1.2.3