mirror of
https://github.com/ninenines/cowboy.git
synced 2025-07-14 12:20:24 +00:00
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.
This commit is contained in:
Loïc Hoguin
parent
15ceaf1edf
commit
3cbf885961
4 changed files with 139 additions and 32 deletions
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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};
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -83,9 +83,23 @@ do_body(Method, Path, Headers0, Body, Config) ->
|
|||
gun:close(ConnPid),
|
||||
do_decode(RespHeaders, RespBody).
|
||||
|
||||
do_get(Path, Config) ->
|
||||
do_body_error(Method, Path, Headers0, Body, Config) ->
|
||||
ConnPid = gun_open(Config),
|
||||
Ref = gun:get(ConnPid, Path, [{<<"accept-encoding">>, <<"gzip">>}]),
|
||||
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">>}|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)),
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue