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

@@ -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.

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,
+stream_terminate(State0=#state{opts=Opts, in_state=InState, out_streamid=OutStreamID,
-		streams=Streams0, children=Children0}, StreamID, Reason) ->
+		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.
+	%% We want to drop the connection if the body was not read fully
-	%% @todo Skip the body, if any, or drop the connection if too large.
+	%% 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,
+	MaxSkipBodyLength = maps:get(max_skip_body_length, Opts, 1000000),
-	case lists:keyfind(NextOutStreamID, #stream.id, Streams) of
+	case InState of
-		false ->
+		#ps_body{length=undefined} ->
-			%% @todo This is clearly wrong, if the stream is gone we need to check if
+			terminate(State#state{streams=Streams, children=Children}, skip_body_unknown_length);
-			%% there used to be such a stream, and if there was to send an error.
+		#ps_body{length=Len, received=Received} when Received + MaxSkipBodyLength < Len ->
-			State#state{out_streamid=NextOutStreamID, out_state=wait, streams=Streams, children=Children};
+			terminate(State#state{streams=Streams, children=Children}, skip_body_too_large);
-		#stream{queue=Commands} ->
+		_ ->
-			%% @todo Remove queue from the stream.
+			%% Move on to the next stream.
-			commands(State#state{out_streamid=NextOutStreamID, out_state=wait,
+			NextOutStreamID = OutStreamID + 1,
-				streams=Streams, children=Children}, NextOutStreamID, Commands)
+			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)

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.

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, <<>>),

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.