From 8ae444006122442971952c5fca2d7c1aea54cbce Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Mon, 20 Feb 2023 16:51:50 +0100 Subject: [PATCH 1/4] style: fix API description for bytes parameter --- apps/emqx_management/src/emqx_mgmt_api_trace.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/emqx_management/src/emqx_mgmt_api_trace.erl b/apps/emqx_management/src/emqx_mgmt_api_trace.erl index cc4a905a4..624e0bdb8 100644 --- a/apps/emqx_management/src/emqx_mgmt_api_trace.erl +++ b/apps/emqx_management/src/emqx_mgmt_api_trace.erl @@ -315,7 +315,7 @@ fields(bytes) -> hoconsc:mk( integer(), #{ - desc => "Maximum number of bytes to store in request", + desc => "Maximum number of bytes to send in response", in => query, required => false, default => 1000 From 9ecf154a711ca13be0beb879274db45646c68e42 Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Mon, 20 Feb 2023 17:05:41 +0100 Subject: [PATCH 2/4] fix: limit bytes param to signed 32bit int We still need to check if chunk we're reading fits in memory --- .../src/emqx_mgmt_api_trace.erl | 30 ++++++++++++++++--- .../test/emqx_mgmt_api_trace_SUITE.erl | 11 +++++-- 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/apps/emqx_management/src/emqx_mgmt_api_trace.erl b/apps/emqx_management/src/emqx_mgmt_api_trace.erl index 624e0bdb8..594750d06 100644 --- a/apps/emqx_management/src/emqx_mgmt_api_trace.erl +++ b/apps/emqx_management/src/emqx_mgmt_api_trace.erl @@ -47,9 +47,12 @@ 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". @@ -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', 'NODE_ERROR'], <<"Bad input parameter">> + ), + 404 => emqx_dashboard_swagger:error_codes( + ['NOT_FOUND'], <<"Trace Name 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 send in response", in => query, required => false, - default => 1000 + default => 1000, + minimum => 0, + maximum => ?MAX_SINT32 } )} ]; @@ -579,6 +593,14 @@ 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">>) end; 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..92b35db99 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"). @@ -296,6 +294,15 @@ t_stream_log(_Config) -> #{<<"meta">> := Meta1, <<"items">> := Bin1} = json(Binary1), ?assertEqual(#{<<"position">> => 30, <<"bytes">> => 10}, Meta1), ?assertEqual(10, byte_size(Bin1)), + 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, {_, 400, _}} = request_api( get, From e78c2c2869d77f246e2d840f1b11d6c0a24911fb Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Mon, 20 Feb 2023 17:07:36 +0100 Subject: [PATCH 3/4] fix: return 404 in case node is not found --- apps/emqx_management/src/emqx_mgmt_api_trace.erl | 16 ++++++++-------- .../test/emqx_mgmt_api_trace_SUITE.erl | 12 ++++++------ 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/apps/emqx_management/src/emqx_mgmt_api_trace.erl b/apps/emqx_management/src/emqx_mgmt_api_trace.erl index 594750d06..38ce9dcf2 100644 --- a/apps/emqx_management/src/emqx_mgmt_api_trace.erl +++ b/apps/emqx_management/src/emqx_mgmt_api_trace.erl @@ -51,7 +51,6 @@ -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">>]). @@ -151,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">> + ) } } }; @@ -188,10 +188,10 @@ schema("/trace/:name/log") -> {meta, fields(bytes) ++ fields(position)} ], 400 => emqx_dashboard_swagger:error_codes( - ['BAD_REQUEST', 'NODE_ERROR'], <<"Bad input parameter">> + ['BAD_REQUEST'], <<"Bad input parameter">> ), 404 => emqx_dashboard_swagger:error_codes( - ['NOT_FOUND'], <<"Trace Name Not Found">> + ['NOT_FOUND', 'NODE_ERROR'], <<"Trace Name or Node Not Found">> ), 503 => emqx_dashboard_swagger:error_codes( ['SERVICE_UNAVAILABLE'], <<"Requested chunk size too big">> @@ -509,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) @@ -602,10 +602,10 @@ stream_log_file(get, #{bindings := #{name := Name}, query_string := Query}) -> }), ?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 92b35db99..162d07aaa 100644 --- a/apps/emqx_management/test/emqx_mgmt_api_trace_SUITE.erl +++ b/apps/emqx_management/test/emqx_mgmt_api_trace_SUITE.erl @@ -223,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 @@ -303,12 +303,12 @@ t_stream_log(_Config) -> meck:expect(file, read, 2, {error, enomem}), {error, {_, 503, _}} = request_api(get, Path), meck:unload(file), - {error, {_, 400, _}} = + {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 From 7502e570668e9efc9b6c9bcb1f0a715341ec42ae Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Tue, 21 Feb 2023 10:41:59 +0100 Subject: [PATCH 4/4] chore: add changelog --- changes/ce/fix-10009.en.md | 1 + changes/ce/fix-10009.zh.md | 1 + 2 files changed, 2 insertions(+) create mode 100644 changes/ce/fix-10009.en.md create mode 100644 changes/ce/fix-10009.zh.md 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位整数。