diff options
| -rw-r--r-- | src/cowboy_http.erl | 11 | ||||
| -rw-r--r-- | src/cowboy_req.erl | 57 | ||||
| -rw-r--r-- | src/cowboy_stream_h.erl | 21 | ||||
| -rw-r--r-- | test/req_SUITE.erl | 80 |
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)), |