From b000d53855592c5470a8c2f2dcebc4915ec4d4d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Hoguin?= Date: Thu, 7 Dec 2017 22:12:34 +0100 Subject: Add more rfc7231 tests and a new max_skip_body_length option The option controls how much body we accept to skip for HTTP/1.1 connections when the user code did not consume the body fully. It defaults to 1MB. --- doc/src/manual/cowboy_http.asciidoc | 6 + src/cowboy_http.erl | 58 ++++++---- test/cowboy_test.erl | 16 +-- test/handlers/echo_h.erl | 3 + test/rfc7231_SUITE.erl | 218 +++++++++++++++++++++++++++++++++++- 5 files changed, 272 insertions(+), 29 deletions(-) diff --git a/doc/src/manual/cowboy_http.asciidoc b/doc/src/manual/cowboy_http.asciidoc index 0c67d9e..e1936fc 100644 --- a/doc/src/manual/cowboy_http.asciidoc +++ b/doc/src/manual/cowboy_http.asciidoc @@ -28,6 +28,7 @@ opts() :: #{ max_keepalive => non_neg_integer(), max_method_length => non_neg_integer(), max_request_line_length => non_neg_integer(), + max_skip_body_length => non_neg_integer(), middlewares => [module()], request_timeout => timeout(), shutdown_timeout => timeout(), @@ -79,6 +80,10 @@ max_method_length (32):: max_request_line_length (8000):: Maximum length of the request line. +max_skip_body_length (1000000):: + Maximum length Cowboy is willing to skip when the user code did not read the body fully. + When the remaining length is too large or unknown Cowboy will close the connection. + middlewares ([cowboy_router, cowboy_handler]):: Middlewares to run for every request. @@ -93,6 +98,7 @@ stream_handlers ([cowboy_stream_h]):: == Changelog +* *2.2*: The `max_skip_body_length` option was added. * *2.0*: The `timeout` option was renamed `request_timeout`. * *2.0*: The `idle_timeout`, `inactivity_timeout` and `shutdown_timeout` options were added. * *2.0*: The `max_method_length` option was added. diff --git a/src/cowboy_http.erl b/src/cowboy_http.erl index bc8fbb9..2301abb 100644 --- a/src/cowboy_http.erl +++ b/src/cowboy_http.erl @@ -58,6 +58,8 @@ %% by not reading from the socket when the window is empty). -record(ps_body, { + length :: non_neg_integer() | undefined, + received = 0 :: non_neg_integer(), %% @todo flow transfer_decode_fun :: fun(), %% @todo better type transfer_decode_state :: any() %% @todo better type @@ -305,7 +307,8 @@ after_parse({data, StreamID, IsFin, Data, State=#state{ stream_reset(State, StreamID, {internal_error, {Class, Exception}, 'Unhandled exception in cowboy_stream:data/4.'}) end; -%% No corresponding stream, skip. +%% No corresponding stream. We must skip the body of the previous request +%% in order to process the next one. after_parse({data, _, _, _, State, Buffer}) -> before_loop(State, Buffer); after_parse({more, State, Buffer}) -> @@ -667,6 +670,7 @@ request(Buffer, State0=#state{ref=Ref, transport=Transport, peer=Peer, sock=Sock State = case HasBody of true -> State0#state{in_state=#ps_body{ + length = BodyLength, transfer_decode_fun = TDecodeFun, transfer_decode_state = TDecodeState }}; @@ -735,7 +739,8 @@ http2_upgrade(State=#state{parent=Parent, ref=Ref, socket=Socket, transport=Tran %% Request body parsing. parse_body(Buffer, State=#state{in_streamid=StreamID, in_state= - PS=#ps_body{transfer_decode_fun=TDecode, transfer_decode_state=TState0}}) -> + PS=#ps_body{received=Received, transfer_decode_fun=TDecode, + transfer_decode_state=TState0}}) -> %% @todo Proper trailers. try TDecode(Buffer, TState0) of more -> @@ -744,15 +749,18 @@ parse_body(Buffer, State=#state{in_streamid=StreamID, in_state= {more, Data, TState} -> %% @todo Asks for 0 or more bytes. {data, StreamID, nofin, Data, State#state{in_state= - PS#ps_body{transfer_decode_state=TState}}, <<>>}; + PS#ps_body{received=Received + byte_size(Data), + transfer_decode_state=TState}}, <<>>}; {more, Data, _Length, TState} when is_integer(_Length) -> %% @todo Asks for Length more bytes. {data, StreamID, nofin, Data, State#state{in_state= - PS#ps_body{transfer_decode_state=TState}}, <<>>}; + PS#ps_body{received=Received + byte_size(Data), + transfer_decode_state=TState}}, <<>>}; {more, Data, Rest, TState} -> %% @todo Asks for 0 or more bytes. {data, StreamID, nofin, Data, State#state{in_state= - PS#ps_body{transfer_decode_state=TState}}, Rest}; + PS#ps_body{received=Received + byte_size(Data), + transfer_decode_state=TState}}, Rest}; {done, _HasTrailers, Rest} -> {data, StreamID, fin, <<>>, set_timeout( State#state{in_streamid=StreamID + 1, in_state=#ps_request_line{}}), Rest}; @@ -1043,8 +1051,8 @@ stream_reset(State, StreamID, StreamError={internal_error, _, _}) -> % stream_terminate(State#state{out_state=done}, StreamID, StreamError). stream_terminate(State, StreamID, StreamError). -stream_terminate(State0=#state{out_streamid=OutStreamID, out_state=OutState, - streams=Streams0, children=Children0}, StreamID, Reason) -> +stream_terminate(State0=#state{opts=Opts, in_state=InState, out_streamid=OutStreamID, + out_state=OutState, streams=Streams0, children=Children0}, StreamID, Reason) -> #stream{version=Version} = lists:keyfind(StreamID, #stream.id, Streams0), State1 = #state{streams=Streams1} = case OutState of wait when element(1, Reason) =:= internal_error -> @@ -1070,22 +1078,32 @@ stream_terminate(State0=#state{out_streamid=OutStreamID, out_state=OutState, [] -> set_timeout(State2); _ -> State2 end, - %% Move on to the next stream. - %% @todo Skip the body, if any, or drop the connection if too large. + %% We want to drop the connection if the body was not read fully + %% and we don't know its length or more remains to be read than + %% configuration allows. %% @todo Only do this if Current =:= StreamID. - NextOutStreamID = OutStreamID + 1, - case lists:keyfind(NextOutStreamID, #stream.id, Streams) of - false -> - %% @todo This is clearly wrong, if the stream is gone we need to check if - %% there used to be such a stream, and if there was to send an error. - State#state{out_streamid=NextOutStreamID, out_state=wait, streams=Streams, children=Children}; - #stream{queue=Commands} -> - %% @todo Remove queue from the stream. - commands(State#state{out_streamid=NextOutStreamID, out_state=wait, - streams=Streams, children=Children}, NextOutStreamID, Commands) + MaxSkipBodyLength = maps:get(max_skip_body_length, Opts, 1000000), + case InState of + #ps_body{length=undefined} -> + terminate(State#state{streams=Streams, children=Children}, skip_body_unknown_length); + #ps_body{length=Len, received=Received} when Received + MaxSkipBodyLength < Len -> + terminate(State#state{streams=Streams, children=Children}, skip_body_too_large); + _ -> + %% Move on to the next stream. + NextOutStreamID = OutStreamID + 1, + case lists:keyfind(NextOutStreamID, #stream.id, Streams) of + false -> + %% @todo This is clearly wrong, if the stream is gone we need to check if + %% there used to be such a stream, and if there was to send an error. + State#state{out_streamid=NextOutStreamID, out_state=wait, + streams=Streams, children=Children}; + #stream{queue=Commands} -> + %% @todo Remove queue from the stream. + commands(State#state{out_streamid=NextOutStreamID, out_state=wait, + streams=Streams, children=Children}, NextOutStreamID, Commands) + end end. -%% @todo Taken directly from _http2 stream_call_terminate(StreamID, Reason, StreamState) -> try cowboy_stream:terminate(StreamID, Reason, StreamState) diff --git a/test/cowboy_test.erl b/test/cowboy_test.erl index 8bd808a..f130451 100644 --- a/test/cowboy_test.erl +++ b/test/cowboy_test.erl @@ -66,40 +66,40 @@ common_groups(Tests) -> init_common_groups(Name = http, Config, Mod) -> init_http(Name, #{ env => #{dispatch => Mod:init_dispatch(Config)} - }, Config); + }, [{flavor, vanilla}|Config]); init_common_groups(Name = https, Config, Mod) -> init_https(Name, #{ env => #{dispatch => Mod:init_dispatch(Config)} - }, Config); + }, [{flavor, vanilla}|Config]); init_common_groups(Name = h2, Config, Mod) -> init_http2(Name, #{ env => #{dispatch => Mod:init_dispatch(Config)} - }, Config); + }, [{flavor, vanilla}|Config]); init_common_groups(Name = h2c, Config, Mod) -> Config1 = init_http(Name, #{ env => #{dispatch => Mod:init_dispatch(Config)} - }, Config), + }, [{flavor, vanilla}|Config]), lists:keyreplace(protocol, 1, Config1, {protocol, http2}); init_common_groups(Name = http_compress, Config, Mod) -> init_http(Name, #{ env => #{dispatch => Mod:init_dispatch(Config)}, stream_handlers => [cowboy_compress_h, cowboy_stream_h] - }, Config); + }, [{flavor, compress}|Config]); init_common_groups(Name = https_compress, Config, Mod) -> init_https(Name, #{ env => #{dispatch => Mod:init_dispatch(Config)}, stream_handlers => [cowboy_compress_h, cowboy_stream_h] - }, Config); + }, [{flavor, compress}|Config]); init_common_groups(Name = h2_compress, Config, Mod) -> init_http2(Name, #{ env => #{dispatch => Mod:init_dispatch(Config)}, stream_handlers => [cowboy_compress_h, cowboy_stream_h] - }, Config); + }, [{flavor, compress}|Config]); init_common_groups(Name = h2c_compress, Config, Mod) -> Config1 = init_http(Name, #{ env => #{dispatch => Mod:init_dispatch(Config)}, stream_handlers => [cowboy_compress_h, cowboy_stream_h] - }, Config), + }, [{flavor, compress}|Config]), lists:keyreplace(protocol, 1, Config1, {protocol, http2}). %% Support functions for testing using Gun. diff --git a/test/handlers/echo_h.erl b/test/handlers/echo_h.erl index a2ab31b..a116442 100644 --- a/test/handlers/echo_h.erl +++ b/test/handlers/echo_h.erl @@ -21,6 +21,9 @@ echo(<<"read_body">>, Req0, Opts) -> <<"/100-continue", _/bits>> -> cowboy_req:inform(100, Req0), cowboy_req:read_body(Req0); + <<"/delay", _/bits>> -> + timer:sleep(500), + cowboy_req:read_body(Req0); <<"/full", _/bits>> -> read_body(Req0, <<>>); <<"/length", _/bits>> -> {_, _, Req1} = read_body(Req0, <<>>), diff --git a/test/rfc7231_SUITE.erl b/test/rfc7231_SUITE.erl index 9eddf72..fca7443 100644 --- a/test/rfc7231_SUITE.erl +++ b/test/rfc7231_SUITE.erl @@ -41,6 +41,7 @@ init_dispatch(_) -> {"*", asterisk_h, []}, {"/", hello_h, []}, {"/echo/:key", echo_h, []}, + {"/delay/echo/:key", echo_h, []}, {"/resp/:key[/:arg]", resp_h, []}, {"/ws", ws_init_h, []} ]}]). @@ -48,6 +49,19 @@ init_dispatch(_) -> %% @todo The documentation should list what methods, headers and status codes %% are handled automatically so users can know what befalls to them to implement. +%% Representations. + +%% Cowboy has cowboy_compress_h that could be concerned with this. +%% However Cowboy will not attempt to compress if any content-coding +%% is already applied, regardless of what they are. +% +% If one or more encodings have been applied to a representation, the +% sender that applied the encodings MUST generate a Content-Encoding +% header field that lists the content codings in the order in which +% they were applied. Additional information about the encoding +% parameters can be provided by other header fields not defined by this +% specification. (RFC7231 3.1.2.2) + %% Methods. method_get(Config) -> @@ -201,7 +215,201 @@ method_trace(Config) -> %% Request headers. -%% @todo +expect(Config) -> + doc("A server that receives a 100-continue expectation should honor it. (RFC7231 5.1.1)"), + ConnPid = gun_open(Config), + Ref = gun:post(ConnPid, "/echo/read_body", [ + {<<"accept-encoding">>, <<"gzip">>}, + {<<"content-type">>, <<"application/x-www-form-urlencoded">>}, + {<<"expect">>, <<"100-continue">>} + ]), + {inform, 100, _} = gun:await(ConnPid, Ref), + ok. + +http10_expect(Config) -> + case config(protocol, Config) of + http -> + do_http10_expect(Config); + http2 -> + expect(Config) + end. + +do_http10_expect(Config) -> + doc("A server that receives a 100-continue expectation " + "in an HTTP/1.0 request must ignore it. (RFC7231 5.1.1)"), + Body = <<"hello=world">>, + ConnPid = gun_open([{http_opts, #{version => 'HTTP/1.0'}}|Config]), + Ref = gun:post(ConnPid, "/echo/read_body", [ + {<<"accept-encoding">>, <<"gzip">>}, + {<<"content-type">>, <<"application/x-www-form-urlencoded">>}, + {<<"content-length">>, integer_to_binary(byte_size(Body))}, + {<<"expect">>, <<"100-continue">>} + ]), + timer:sleep(500), + ok = gun:data(ConnPid, Ref, fin, Body), + {response, nofin, 200, _} = gun:await(ConnPid, Ref), + {ok, Body} = gun:await_body(ConnPid, Ref), + ok. + +%% Cowboy ignores the expect header when the value is not 100-continue. +% +% A server that receives an Expect field-value other than 100-continue +% MAY respond with a 417 (Expectation Failed) status code to indicate +% that the unexpected expectation cannot be met. + +expect_receive_body_omit_100_continue(Config) -> + doc("A server may omit sending a 100 Continue response if it has " + "already started receiving the request body. (RFC7231 5.1.1)"), + ConnPid = gun_open(Config), + Ref = gun:post(ConnPid, "/delay/echo/read_body", [ + {<<"accept-encoding">>, <<"gzip">>}, + {<<"content-type">>, <<"application/x-www-form-urlencoded">>}, + {<<"expect">>, <<"100-continue">>} + ], <<"hello=world">>), + %% We receive the response directly without a 100 Continue. + {response, nofin, 200, _} = gun:await(ConnPid, Ref), + {ok, <<"hello=world">>} = gun:await_body(ConnPid, Ref), + ok. + +expect_discard_body_skip(Config) -> + doc("A server that responds with a final status code before reading " + "the entire message body should keep the connection open and skip " + "the body when appropriate. (RFC7231 5.1.1)"), + ConnPid = gun_open(Config), + Ref1 = gun:post(ConnPid, "/echo/method", [ + {<<"accept-encoding">>, <<"gzip">>}, + {<<"content-type">>, <<"application/x-www-form-urlencoded">>}, + {<<"expect">>, <<"100-continue">>} + ], <<"hello=world">>), + {response, nofin, 200, _} = gun:await(ConnPid, Ref1), + {ok, <<"POST">>} = gun:await_body(ConnPid, Ref1), + Ref2 = gun:get(ConnPid, "/echo/method", [ + {<<"accept-encoding">>, <<"gzip">>}, + {<<"content-type">>, <<"application/x-www-form-urlencoded">>} + ]), + {response, nofin, 200, _} = gun:await(ConnPid, Ref2), + {ok, <<"GET">>} = gun:await_body(ConnPid, Ref2), + ok. + +expect_discard_body_close(Config) -> + case config(protocol, Config) of + http -> + do_expect_discard_body_close(Config); + http2 -> + doc("There's no reason to close the connection when using HTTP/2, " + "even if a stream body is too large. We just cancel the stream.") + end. + +do_expect_discard_body_close(Config) -> + doc("A server that responds with a final status code before reading " + "the entire message body may close the connection to avoid " + "reading a potentially large request body. (RFC7231 5.1.1, RFC7230 6.6)"), + ConnPid = gun_open(Config), + Ref1 = gun:post(ConnPid, "/echo/method", [ + {<<"accept-encoding">>, <<"gzip">>}, + {<<"content-length">>, <<"10000000">>}, + {<<"content-type">>, <<"application/x-www-form-urlencoded">>}, + {<<"expect">>, <<"100-continue">>} + ]), + {response, nofin, 200, Headers} = gun:await(ConnPid, Ref1), + %% Ideally we would send a connection: close. Cowboy however + %% cannot know the intent of the application until after we + %% sent the response. +% {_, <<"close">>} = lists:keyfind(<<"connection">>, 1, Headers), + {ok, <<"POST">>} = gun:await_body(ConnPid, Ref1), + %% The connection is gone. + receive + {gun_down, ConnPid, _, closed, _, _} -> + ok + after 1000 -> + error(timeout) + end. + +no_accept_encoding(Config) -> + doc("While a request with no accept-encoding header implies the " + "user agent has no preferences and any would be acceptable, " + "Cowboy will not serve content-codings by defaults to ensure " + "the content can safely be read. (RFC7231 5.3.4)"), + ConnPid = gun_open(Config), + Ref = gun:get(ConnPid, "/resp/stream_reply2/200"), + {response, nofin, 200, Headers} = gun:await(ConnPid, Ref), + false = lists:keyfind(<<"content-encoding">>, 1, Headers), + ok. + +%% Cowboy currently ignores any information about the identity content-coding +%% and instead considers it always acceptable. +% +% 2. If the representation has no content-coding, then it is +% acceptable by default unless specifically excluded by the +% Accept-Encoding field stating either "identity;q=0" or "*;q=0" +% without a more specific entry for "identity". + +accept_encoding_gzip(Config) -> + doc("No qvalue means the content-coding is acceptable. (RFC7231 5.3.4)"), + ConnPid = gun_open(Config), + Ref = gun:get(ConnPid, "/resp/stream_reply2/200", [ + {<<"accept-encoding">>, <<"gzip">>} + ]), + {response, nofin, 200, Headers} = gun:await(ConnPid, Ref), + _ = case config(flavor, Config) of + compress -> + {_, <<"gzip">>} = lists:keyfind(<<"content-encoding">>, 1, Headers); + _ -> + false = lists:keyfind(<<"content-encoding">>, 1, Headers) + end, + ok. + +accept_encoding_gzip_1(Config) -> + doc("A qvalue different than 0 means the content-coding is acceptable. (RFC7231 5.3.4)"), + ConnPid = gun_open(Config), + Ref = gun:get(ConnPid, "/resp/stream_reply2/200", [ + {<<"accept-encoding">>, <<"gzip;q=1.0">>} + ]), + {response, nofin, 200, Headers} = gun:await(ConnPid, Ref), + _ = case config(flavor, Config) of + compress -> + {_, <<"gzip">>} = lists:keyfind(<<"content-encoding">>, 1, Headers); + _ -> + false = lists:keyfind(<<"content-encoding">>, 1, Headers) + end, + ok. + +accept_encoding_gzip_0(Config) -> + doc("A qvalue of 0 means the content-coding is not acceptable. (RFC7231 5.3.1, RFC7231 5.3.4)"), + ConnPid = gun_open(Config), + Ref = gun:get(ConnPid, "/resp/stream_reply2/200", [ + {<<"accept-encoding">>, <<"gzip;q=0">>} + ]), + {response, nofin, 200, Headers} = gun:await(ConnPid, Ref), + false = lists:keyfind(<<"content-encoding">>, 1, Headers), + ok. + +%% Cowboy currently only supports gzip automatically via cowboy_compress_h. +% +% 4. If multiple content-codings are acceptable, then the acceptable +% content-coding with the highest non-zero qvalue is preferred. + +accept_encoding_empty(Config) -> + doc("An empty content-coding means that the user agent does not " + "want any content-coding applied to the response. (RFC7231 5.3.4)"), + ConnPid = gun_open(Config), + Ref = gun:get(ConnPid, "/resp/stream_reply2/200", [ + {<<"accept-encoding">>, <<>>} + ]), + {response, nofin, 200, Headers} = gun:await(ConnPid, Ref), + false = lists:keyfind(<<"content-encoding">>, 1, Headers), + ok. + +accept_encoding_unknown(Config) -> + doc("An accept-encoding header only containing unknown content-codings " + "should result in no content-coding being applied. (RFC7231 5.3.4)"), + ConnPid = gun_open(Config), + Ref = gun:get(ConnPid, "/resp/stream_reply2/200", [ + {<<"accept-encoding">>, <<"deflate">>} + ]), + {response, nofin, 200, Headers} = gun:await(ConnPid, Ref), + false = lists:keyfind(<<"content-encoding">>, 1, Headers), + ok. %% Status codes. @@ -587,3 +795,11 @@ status_code_505(Config) -> ]), {response, _, 505, _} = gun:await(ConnPid, Ref), ok. + +%% The 505 response code is supposed to be about the major HTTP version. +%% Cowboy instead rejects any version that isn't HTTP/1.0 or HTTP/1.1 +%% when expecting an h1 request. While this is not correct in theory +%% it works in practice because there are no other minor versions. +%% +%% Cowboy does not do version checking for HTTP/2 since the protocol +%% does not include a version number in the messages. -- cgit v1.2.3