Merge pull request #10009 from sstrigler/EMQX-7994-get-trace-name-log-bytes-xxx-does-not-do-input-validation

get trace name log bytes xxx does not do input validation

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()}.

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

changes/ce/fix-10009.en.md

@@ -0,0 +1 @@
+Validate `bytes` param to `GET /trace/:name/log` to not exceed signed 32bit integer.

changes/ce/fix-10009.zh.md

@@ -0,0 +1 @@
+验证 `GET /trace/:name/log` 的 `bytes` 参数，使其不超过有符号的32位整数。