diff --git a/apps/emqx_management/src/emqx_mgmt_api_trace.erl b/apps/emqx_management/src/emqx_mgmt_api_trace.erl index cc4a905a4..38ce9dcf2 100644 --- a/apps/emqx_management/src/emqx_mgmt_api_trace.erl +++ b/apps/emqx_management/src/emqx_mgmt_api_trace.erl @@ -47,9 +47,11 @@ get_trace_size/0 ]). +-define(MAX_SINT32, 2147483647). + -define(TO_BIN(_B_), iolist_to_binary(_B_)). -define(NOT_FOUND(N), {404, #{code => 'NOT_FOUND', message => ?TO_BIN([N, " NOT FOUND"])}}). --define(BAD_REQUEST(C, M), {400, #{code => C, message => ?TO_BIN(M)}}). +-define(SERVICE_UNAVAILABLE(C, M), {503, #{code => C, message => ?TO_BIN(M)}}). -define(TAGS, [<<"Trace">>]). namespace() -> "trace". @@ -148,8 +150,9 @@ schema("/trace/:name/download") -> #{schema => #{type => "string", format => "binary"}} } }, - 400 => emqx_dashboard_swagger:error_codes(['NODE_ERROR'], <<"Node Not Found">>), - 404 => emqx_dashboard_swagger:error_codes(['NOT_FOUND'], <<"Trace Name Not Found">>) + 404 => emqx_dashboard_swagger:error_codes( + ['NOT_FOUND', 'NODE_ERROR'], <<"Trace Name or Node Not Found">> + ) } } }; @@ -184,8 +187,15 @@ schema("/trace/:name/log") -> {items, hoconsc:mk(binary(), #{example => "TEXT-LOG-ITEMS"})}, {meta, fields(bytes) ++ fields(position)} ], - 400 => emqx_dashboard_swagger:error_codes(['NODE_ERROR'], <<"Trace Log Failed">>), - 404 => emqx_dashboard_swagger:error_codes(['NOT_FOUND'], <<"Trace Name Not Found">>) + 400 => emqx_dashboard_swagger:error_codes( + ['BAD_REQUEST'], <<"Bad input parameter">> + ), + 404 => emqx_dashboard_swagger:error_codes( + ['NOT_FOUND', 'NODE_ERROR'], <<"Trace Name or Node Not Found">> + ), + 503 => emqx_dashboard_swagger:error_codes( + ['SERVICE_UNAVAILABLE'], <<"Requested chunk size too big">> + ) } } }. @@ -313,12 +323,16 @@ fields(bytes) -> [ {bytes, hoconsc:mk( - integer(), + %% This seems to be the minimum max value we may encounter + %% across different OS + range(0, ?MAX_SINT32), #{ - desc => "Maximum number of bytes to store in request", + desc => "Maximum number of bytes to send in response", in => query, required => false, - default => 1000 + default => 1000, + minimum => 0, + maximum => ?MAX_SINT32 } )} ]; @@ -495,7 +509,7 @@ download_trace_log(get, #{bindings := #{name := Name}, query_string := Query}) - }, {200, Headers, {file_binary, ZipName, Binary}}; {error, not_found} -> - ?BAD_REQUEST('NODE_ERROR', <<"Node not found">>) + ?NOT_FOUND(<<"Node">>) end; {error, not_found} -> ?NOT_FOUND(Name) @@ -579,11 +593,19 @@ stream_log_file(get, #{bindings := #{name := Name}, query_string := Query}) -> {200, #{meta => Meta, items => <<"">>}}; {error, not_found} -> ?NOT_FOUND(Name); + {error, enomem} -> + ?SLOG(warning, #{ + code => not_enough_mem, + msg => "Requested chunk size too big", + bytes => Bytes, + name => Name + }), + ?SERVICE_UNAVAILABLE('SERVICE_UNAVAILABLE', <<"Requested chunk size too big">>); {badrpc, nodedown} -> - ?BAD_REQUEST('NODE_ERROR', <<"Node not found">>) + ?NOT_FOUND(<<"Node">>) end; {error, not_found} -> - ?BAD_REQUEST('NODE_ERROR', <<"Node not found">>) + ?NOT_FOUND(<<"Node">>) end. -spec get_trace_size() -> #{{node(), file:name_all()} => non_neg_integer()}. diff --git a/apps/emqx_management/test/emqx_mgmt_api_trace_SUITE.erl b/apps/emqx_management/test/emqx_mgmt_api_trace_SUITE.erl index 6962a9043..162d07aaa 100644 --- a/apps/emqx_management/test/emqx_mgmt_api_trace_SUITE.erl +++ b/apps/emqx_management/test/emqx_mgmt_api_trace_SUITE.erl @@ -19,9 +19,7 @@ -compile(export_all). -compile(nowarn_export_all). --include_lib("common_test/include/ct.hrl"). -include_lib("eunit/include/eunit.hrl"). --include_lib("emqx/include/emqx.hrl"). -include_lib("kernel/include/file.hrl"). -include_lib("stdlib/include/zip.hrl"). -include_lib("snabbkaffe/include/snabbkaffe.hrl"). @@ -225,12 +223,12 @@ t_log_file(_Config) -> ]}, zip:table(Binary2) ), - {error, {_, 400, _}} = + {error, {_, 404, _}} = request_api( get, - api_path("trace/test_client_id/download?node=unknonwn_node") + api_path("trace/test_client_id/download?node=unknown_node") ), - {error, {_, 400, _}} = + {error, {_, 404, _}} = request_api( get, % known atom but unknown node @@ -296,12 +294,21 @@ t_stream_log(_Config) -> #{<<"meta">> := Meta1, <<"items">> := Bin1} = json(Binary1), ?assertEqual(#{<<"position">> => 30, <<"bytes">> => 10}, Meta1), ?assertEqual(10, byte_size(Bin1)), - {error, {_, 400, _}} = + ct:pal("~p vs ~p", [Bin, Bin1]), + %% in theory they could be the same but we know they shouldn't + ?assertNotEqual(Bin, Bin1), + BadReqPath = api_path("trace/test_stream_log/log?&bytes=1000000000000"), + {error, {_, 400, _}} = request_api(get, BadReqPath), + meck:new(file, [passthrough, unstick]), + meck:expect(file, read, 2, {error, enomem}), + {error, {_, 503, _}} = request_api(get, Path), + meck:unload(file), + {error, {_, 404, _}} = request_api( get, - api_path("trace/test_stream_log/log?node=unknonwn_node") + api_path("trace/test_stream_log/log?node=unknown_node") ), - {error, {_, 400, _}} = + {error, {_, 404, _}} = request_api( get, % known atom but not a node diff --git a/changes/ce/fix-10009.en.md b/changes/ce/fix-10009.en.md new file mode 100644 index 000000000..37f33a958 --- /dev/null +++ b/changes/ce/fix-10009.en.md @@ -0,0 +1 @@ +Validate `bytes` param to `GET /trace/:name/log` to not exceed signed 32bit integer. diff --git a/changes/ce/fix-10009.zh.md b/changes/ce/fix-10009.zh.md new file mode 100644 index 000000000..bb55ea5b9 --- /dev/null +++ b/changes/ce/fix-10009.zh.md @@ -0,0 +1 @@ +验证 `GET /trace/:name/log` 的 `bytes` 参数,使其不超过有符号的32位整数。