aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorLoïc Hoguin <[email protected]>2017-09-14 13:42:17 +0200
committerLoïc Hoguin <[email protected]>2017-09-14 13:42:17 +0200
commit3cbf885961dc93df1b39d2de89f2a871402acbd5 (patch)
tree8f8f80ea6e6756fb5136cd6eff9bc03ea00cf481
parent15ceaf1edff2a68e461fafd8cd77fc7f98918c8a (diff)
downloadcowboy-3cbf885961dc93df1b39d2de89f2a871402acbd5.tar.gz
cowboy-3cbf885961dc93df1b39d2de89f2a871402acbd5.tar.bz2
cowboy-3cbf885961dc93df1b39d2de89f2a871402acbd5.zip
Improve how we detect request errors
When the request process exits with a {request_error, Reason, Human} exit reason, Cowboy will return a 400 status code instead of 500. Cowboy may also return a more specific status code depending on the error. Currently it may also return 408 or 413. This should prove to be more solid that looking inside the stack trace.
-rw-r--r--src/cowboy_http.erl11
-rw-r--r--src/cowboy_req.erl57
-rw-r--r--src/cowboy_stream_h.erl21
-rw-r--r--test/req_SUITE.erl80
4 files changed, 138 insertions, 31 deletions
diff --git a/src/cowboy_http.erl b/src/cowboy_http.erl
index 5ee5ceb..c9c6383 100644
--- a/src/cowboy_http.erl
+++ b/src/cowboy_http.erl
@@ -785,8 +785,15 @@ commands(State, StreamID, [{flow, _Length}|Tail]) ->
commands(State, StreamID, Tail);
%% Error responses are sent only if a response wasn't sent already.
-commands(State=#state{out_state=wait}, StreamID, [{error_response, StatusCode, Headers, Body}|Tail]) ->
- commands(State, StreamID, [{response, StatusCode, Headers, Body}|Tail]);
+commands(State=#state{out_state=wait}, StreamID, [{error_response, Status, Headers0, Body}|Tail]) ->
+ %% We close the connection when the error response is 408, as it
+ %% indicates a timeout and the RFC recommends that we stop here. (RFC7231 6.5.7)
+ Headers = case Status of
+ 408 -> Headers0#{<<"connection">> => <<"close">>};
+ <<"408", _/bits>> -> Headers0#{<<"connection">> => <<"close">>};
+ _ -> Headers0
+ end,
+ commands(State, StreamID, [{response, Status, Headers, Body}|Tail]);
commands(State, StreamID, [{error_response, _, _, _}|Tail]) ->
commands(State, StreamID, Tail);
%% Send an informational response.
diff --git a/src/cowboy_req.erl b/src/cowboy_req.erl
index 0106771..84e1b9d 100644
--- a/src/cowboy_req.erl
+++ b/src/cowboy_req.erl
@@ -184,7 +184,13 @@ qs(#{qs := Qs}) ->
%% @todo Might be useful to limit the number of keys.
-spec parse_qs(req()) -> [{binary(), binary() | true}].
parse_qs(#{qs := Qs}) ->
- cow_qs:parse_qs(Qs).
+ try
+ cow_qs:parse_qs(Qs)
+ catch _:_ ->
+ erlang:raise(exit, {request_error, qs,
+ 'Malformed query string; application/x-www-form-urlencoded expected.'
+ }, erlang:get_stacktrace())
+ end.
-spec match_qs(cowboy:fields(), req()) -> map().
match_qs(Fields, Req) ->
@@ -353,15 +359,21 @@ headers(#{headers := Headers}) ->
-spec parse_header(binary(), Req) -> any() when Req::req().
parse_header(Name = <<"content-length">>, Req) ->
- parse_header(Name, Req, 0, fun cow_http_hd:parse_content_length/1);
+ parse_header(Name, Req, 0);
parse_header(Name = <<"cookie">>, Req) ->
- parse_header(Name, Req, [], fun cow_cookie:parse_cookie/1);
+ parse_header(Name, Req, []);
parse_header(Name, Req) ->
parse_header(Name, Req, undefined).
-spec parse_header(binary(), Req, any()) -> any() when Req::req().
parse_header(Name, Req, Default) ->
- parse_header(Name, Req, Default, parse_header_fun(Name)).
+ try
+ parse_header(Name, Req, Default, parse_header_fun(Name))
+ catch _:_ ->
+ erlang:raise(exit, {request_error, {header, Name},
+ 'Malformed header. Please consult the relevant specification.'
+ }, erlang:get_stacktrace())
+ end.
parse_header_fun(<<"accept">>) -> fun cow_http_hd:parse_accept/1;
parse_header_fun(<<"accept-charset">>) -> fun cow_http_hd:parse_accept_charset/1;
@@ -447,8 +459,26 @@ read_urlencoded_body(Req) ->
-spec read_urlencoded_body(Req, read_body_opts()) -> {ok, [{binary(), binary() | true}], Req} when Req::req().
read_urlencoded_body(Req0, Opts) ->
- {ok, Body, Req} = read_body(Req0, Opts),
- {ok, cow_qs:parse_qs(Body), Req}.
+ case read_body(Req0, Opts) of
+ {ok, Body, Req} ->
+ try
+ {ok, cow_qs:parse_qs(Body), Req}
+ catch _:_ ->
+ erlang:raise(exit, {request_error, urlencoded_body,
+ 'Malformed body; application/x-www-form-urlencoded expected.'
+ }, erlang:get_stacktrace())
+ end;
+ {more, Body, _} ->
+ Length = maps:get(length, Opts, 64000),
+ if
+ byte_size(Body) < Length ->
+ exit({request_error, timeout,
+ 'The request body was not received within the configured time.'});
+ true ->
+ exit({request_error, payload_too_large,
+ 'The request body is larger than allowed by configuration.'})
+ end
+ end.
%% Multipart.
@@ -471,7 +501,7 @@ read_part(Req, Opts) ->
end.
read_part(Buffer, Opts, Req=#{multipart := {Boundary, _}}) ->
- case cow_multipart:parse_headers(Buffer, Boundary) of
+ try cow_multipart:parse_headers(Buffer, Boundary) of
more ->
{Data, Req2} = stream_multipart(Req, Opts),
read_part(<< Buffer/binary, Data/binary >>, Opts, Req2);
@@ -486,6 +516,10 @@ read_part(Buffer, Opts, Req=#{multipart := {Boundary, _}}) ->
%% Ignore epilogue.
{done, _} ->
{done, Req#{multipart => done}}
+ catch _:_ ->
+ erlang:raise(exit, {request_error, {multipart, headers},
+ 'Malformed body; multipart expected.'
+ }, erlang:get_stacktrace())
end.
-spec read_part_body(Req)
@@ -529,8 +563,13 @@ read_part_body(Buffer, Opts, Req=#{multipart := {Boundary, _}}, Acc) ->
init_multipart(Req) ->
{<<"multipart">>, _, Params} = parse_header(<<"content-type">>, Req),
- {_, Boundary} = lists:keyfind(<<"boundary">>, 1, Params),
- Req#{multipart => {Boundary, <<>>}}.
+ case lists:keyfind(<<"boundary">>, 1, Params) of
+ {_, Boundary} ->
+ Req#{multipart => {Boundary, <<>>}};
+ false ->
+ exit({request_error, {multipart, boundary},
+ 'Missing boundary parameter for multipart media type.'})
+ end.
stream_multipart(Req=#{multipart := done}, _) ->
{<<>>, Req};
diff --git a/src/cowboy_stream_h.erl b/src/cowboy_stream_h.erl
index 5113f2e..4459679 100644
--- a/src/cowboy_stream_h.erl
+++ b/src/cowboy_stream_h.erl
@@ -69,14 +69,17 @@ data(_StreamID, IsFin, Data, State=#state{pid=Pid, read_body_ref=Ref,
-spec info(cowboy_stream:streamid(), any(), State)
-> {cowboy_stream:commands(), State} when State::#state{}.
info(_StreamID, {'EXIT', Pid, normal}, State=#state{pid=Pid}) ->
- %% @todo Do we even reach this clause?
{[stop], State};
-info(_StreamID, {'EXIT', Pid, {_Reason, [T1, T2|_]}}, State=#state{pid=Pid})
- when element(1, T1) =:= cow_http_hd; element(1, T2) =:= cow_http_hd ->
- %% @todo Have an option to enable/disable this specific crash report?
+info(_StreamID, {'EXIT', Pid, {{request_error, Reason, _HumanReadable}, _}}, State=#state{pid=Pid}) ->
+ %% @todo Optionally report the crash to help debugging.
%%report_crash(Ref, StreamID, Pid, Reason, Stacktrace),
+ Status = case Reason of
+ timeout -> 408;
+ payload_too_large -> 413;
+ _ -> 400
+ end,
%% @todo Headers? Details in body? More stuff in debug only?
- {[{error_response, 400, #{<<"content-length">> => <<"0">>}, <<>>}, stop], State};
+ {[{error_response, Status, #{<<"content-length">> => <<"0">>}, <<>>}, stop], State};
info(StreamID, Exit = {'EXIT', Pid, {Reason, Stacktrace}}, State=#state{ref=Ref, pid=Pid}) ->
report_crash(Ref, StreamID, Pid, Reason, Stacktrace),
{[
@@ -162,11 +165,9 @@ report_crash(Ref, StreamID, Pid, Reason, Stacktrace) ->
proc_lib_hack(Req, Env, Middlewares) ->
try
execute(Req, Env, Middlewares)
- catch
- _:Reason when element(1, Reason) =:= cowboy_handler ->
- exit(Reason);
- _:Reason ->
- exit({Reason, erlang:get_stacktrace()})
+ catch _:Reason ->
+ %% @todo Have a way to identify OTP 20 to not do this twice?
+ exit({Reason, erlang:get_stacktrace()})
end.
%% @todo
diff --git a/test/req_SUITE.erl b/test/req_SUITE.erl
index c724dea..3a223d9 100644
--- a/test/req_SUITE.erl
+++ b/test/req_SUITE.erl
@@ -83,9 +83,23 @@ do_body(Method, Path, Headers0, Body, Config) ->
gun:close(ConnPid),
do_decode(RespHeaders, RespBody).
+do_body_error(Method, Path, Headers0, Body, Config) ->
+ ConnPid = gun_open(Config),
+ Headers = [{<<"accept-encoding">>, <<"gzip">>}|Headers0],
+ Ref = case Body of
+ <<>> -> gun:request(ConnPid, Method, Path, Headers);
+ _ -> gun:request(ConnPid, Method, Path, Headers, Body)
+ end,
+ {response, _, Status, RespHeaders} = gun:await(ConnPid, Ref),
+ gun:close(ConnPid),
+ {Status, RespHeaders}.
+
do_get(Path, Config) ->
+ do_get(Path, [], Config).
+
+do_get(Path, Headers, Config) ->
ConnPid = gun_open(Config),
- Ref = gun:get(ConnPid, Path, [{<<"accept-encoding">>, <<"gzip">>}]),
+ Ref = gun:get(ConnPid, Path, [{<<"accept-encoding">>, <<"gzip">>}|Headers]),
{response, IsFin, Status, RespHeaders} = gun:await(ConnPid, Ref),
{ok, RespBody} = case IsFin of
nofin -> gun:await_body(ConnPid, Ref);
@@ -178,13 +192,13 @@ method(Config) ->
do_method("/direct/method", Config).
do_method(Path, Config) ->
- <<"GET">> = do_body("GET", "/method", Config),
- <<>> = do_body("HEAD", "/method", Config),
- <<"OPTIONS">> = do_body("OPTIONS", "/method", Config),
- <<"PATCH">> = do_body("PATCH", "/method", Config),
- <<"POST">> = do_body("POST", "/method", Config),
- <<"PUT">> = do_body("PUT", "/method", Config),
- <<"ZZZZZZZZ">> = do_body("ZZZZZZZZ", "/method", Config),
+ <<"GET">> = do_body("GET", Path, Config),
+ <<>> = do_body("HEAD", Path, Config),
+ <<"OPTIONS">> = do_body("OPTIONS", Path, Config),
+ <<"PATCH">> = do_body("PATCH", Path, Config),
+ <<"POST">> = do_body("POST", Path, Config),
+ <<"PUT">> = do_body("PUT", Path, Config),
+ <<"ZZZZZZZZ">> = do_body("ZZZZZZZZ", Path, Config),
ok.
parse_cookies(Config) ->
@@ -197,6 +211,11 @@ parse_cookies(Config) ->
<<"[{<<\"cake\">>,<<\"strawberry\">>},{<<\"color\">>,<<\"blue\">>}]">>
= do_get_body("/parse_cookies",
[{<<"cookie">>, "cake=strawberry"}, {<<"cookie">>, "color=blue"}], Config),
+ %% Ensure parse errors result in a 400 response.
+ {400, _, _} = do_get("/parse_cookies",
+ [{<<"cookie">>, "bad name=strawberry"}], Config),
+ {400, _, _} = do_get("/parse_cookies",
+ [{<<"cookie">>, "goodname=strawberry\tmilkshake"}], Config),
ok.
parse_header(Config) ->
@@ -211,6 +230,9 @@ parse_header(Config) ->
<<"undefined">> = do_get_body("/args/parse_header/upgrade", Config),
%% Header in request and with default provided.
<<"100-continue">> = do_get_body("/args/parse_header/expect/100-continue", Config),
+ %% Ensure parse errors result in a 400 response.
+ {400, _, _} = do_get("/args/parse_header/accept",
+ [{<<"accept">>, "bad media type"}], Config),
ok.
parse_qs(Config) ->
@@ -218,6 +240,8 @@ parse_qs(Config) ->
<<"[]">> = do_get_body("/parse_qs", Config),
<<"[{<<\"abc\">>,true}]">> = do_get_body("/parse_qs?abc", Config),
<<"[{<<\"a\">>,<<\"b\">>},{<<\"c\">>,<<\"d e\">>}]">> = do_get_body("/parse_qs?a=b&c=d+e", Config),
+ %% Ensure parse errors result in a 400 response.
+ {400, _, _} = do_get("/parse_qs?%%%%%%%", Config),
ok.
path(Config) ->
@@ -389,6 +413,8 @@ read_urlencoded_body(Config) ->
ok = do_read_urlencoded_body_too_long("/crash/read_urlencoded_body/period", <<"abc">>, Config),
%% The timeout value is set too low on purpose to ensure a crash occurs.
ok = do_read_body_timeout("/opts/read_urlencoded_body/timeout", <<"abc">>, Config),
+ %% Ensure parse errors result in a 400 response.
+ {400, _} = do_body_error("POST", "/read_urlencoded_body", [], "%%%%%", Config),
ok.
%% We expect a crash.
@@ -398,7 +424,7 @@ do_read_urlencoded_body_too_large(Path, Body, Config) ->
{<<"content-length">>, integer_to_binary(iolist_size(Body))}
]),
gun:data(ConnPid, Ref, fin, Body),
- {response, _, 500, _} = gun:await(ConnPid, Ref),
+ {response, _, 413, _} = gun:await(ConnPid, Ref),
gun:close(ConnPid).
%% We expect a crash.
@@ -410,7 +436,14 @@ do_read_urlencoded_body_too_long(Path, Body, Config) ->
gun:data(ConnPid, Ref, nofin, Body),
timer:sleep(1100),
gun:data(ConnPid, Ref, fin, Body),
- {response, _, 500, _} = gun:await(ConnPid, Ref),
+ {response, _, 408, RespHeaders} = gun:await(ConnPid, Ref),
+ _ = case config(protocol, Config) of
+ http ->
+ %% 408 error responses should close HTTP/1.1 connections.
+ {_, <<"close">>} = lists:keyfind(<<"connection">>, 1, RespHeaders);
+ http2 ->
+ ok
+ end,
gun:close(ConnPid).
multipart(Config) ->
@@ -437,6 +470,33 @@ do_multipart(Path, Config) ->
} = LargeHeaders,
ok.
+multipart_error_headers(Config) ->
+ doc("Multipart request body with invalid part headers."),
+ ReqBody = [
+ "--deadbeef\r\nbad-header text/plain\r\n\r\nCowboy is an HTTP server.\r\n"
+ "--deadbeef--"
+ ],
+ %% Ensure parse errors result in a 400 response.
+ {400, _} = do_body_error("POST", "/multipart", [
+ {<<"content-type">>, <<"multipart/mixed; boundary=deadbeef">>}
+ ], ReqBody, Config),
+ ok.
+
+%% The function to parse the multipart body currently does not crash,
+%% as far as I can tell. There is therefore no test for it.
+
+multipart_missing_boundary(Config) ->
+ doc("Multipart request body without a boundary in the media type."),
+ ReqBody = [
+ "--deadbeef\r\nContent-Type: text/plain\r\n\r\nCowboy is an HTTP server.\r\n"
+ "--deadbeef--"
+ ],
+ %% Ensure parse errors result in a 400 response.
+ {400, _} = do_body_error("POST", "/multipart", [
+ {<<"content-type">>, <<"multipart/mixed">>}
+ ], ReqBody, Config),
+ ok.
+
read_part_skip_body(Config) ->
doc("Multipart request body skipping part bodies."),
LargeBody = iolist_to_binary(string:chars($a, 10000000)),