From 1f7c02ecf58f1256db1a9e345bf5fb9a67ed1844 Mon Sep 17 00:00:00 2001
From: Stefan Strigler <stefan.strigler@emqx.io>
Date: Fri, 2 Dec 2022 15:08:15 +0100
Subject: [PATCH] fix: return 400 if node in query doesn't look like a known
 node

---
 .../src/emqx_mgmt_api_trace.erl               | 89 ++++++++++---------
 .../test/emqx_mgmt_api_trace_SUITE.erl        | 40 +++++++++
 changes/v5.0.12-en.md                         |  2 +
 changes/v5.0.12-zh.md                         |  2 +
 4 files changed, 89 insertions(+), 44 deletions(-)

diff --git a/apps/emqx_management/src/emqx_mgmt_api_trace.erl b/apps/emqx_management/src/emqx_mgmt_api_trace.erl
index 93d647753..cbebffd40 100644
--- a/apps/emqx_management/src/emqx_mgmt_api_trace.erl
+++ b/apps/emqx_management/src/emqx_mgmt_api_trace.erl
@@ -48,6 +48,7 @@
 
 -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(TAGS, [<<"Trace">>]).
 
 namespace() -> "trace".
@@ -141,6 +142,7 @@ 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">>)
             }
         }
@@ -176,9 +178,8 @@ schema("/trace/:name/log") ->
                         {items, hoconsc:mk(binary(), #{example => "TEXT-LOG-ITEMS"})},
                         {meta, fields(bytes) ++ fields(position)}
                     ],
-                400 => emqx_dashboard_swagger:error_codes(
-                    ['READ_FILE_ERROR', 'RPC_ERROR', 'NODE_ERROR'], <<"Trace Log Failed">>
-                )
+                400 => emqx_dashboard_swagger:error_codes(['NODE_ERROR'], <<"Trace Log Failed">>),
+                404 => emqx_dashboard_swagger:error_codes(['NOT_FOUND'], <<"Trace Name Not Found">>)
             }
         }
     }.
@@ -450,32 +451,33 @@ update_trace(put, #{bindings := #{name := Name}}) ->
 %% if HTTP request headers include accept-encoding: gzip and file size > 300 bytes.
 %% cowboy_compress_h will auto encode gzip format.
 download_trace_log(get, #{bindings := #{name := Name}, query_string := Query}) ->
-    Nodes =
-        case parse_node(Query, undefined) of
-            {ok, undefined} -> mria_mnesia:running_nodes();
-            {ok, Node0} -> [Node0];
-            {error, not_found} -> mria_mnesia:running_nodes()
-        end,
-    case emqx_trace:get_trace_filename(Name) of
-        {ok, TraceLog} ->
-            TraceFiles = collect_trace_file(Nodes, TraceLog),
-            ZipDir = emqx_trace:zip_dir(),
-            Zips = group_trace_file(ZipDir, TraceLog, TraceFiles),
-            FileName = binary_to_list(Name) ++ ".zip",
-            ZipFileName = filename:join([ZipDir, FileName]),
-            {ok, ZipFile} = zip:zip(ZipFileName, Zips, [{cwd, ZipDir}]),
-            %% emqx_trace:delete_files_after_send(ZipFileName, Zips),
-            %% TODO use file replace file_binary.(delete file after send is not ready now).
-            {ok, Binary} = file:read_file(ZipFile),
-            ZipName = filename:basename(ZipFile),
-            _ = file:delete(ZipFile),
-            Headers = #{
-                <<"content-type">> => <<"application/x-zip">>,
-                <<"content-disposition">> => iolist_to_binary("attachment; filename=" ++ ZipName)
-            },
-            {200, Headers, {file_binary, ZipName, Binary}};
+    case parse_node(Query, undefined) of
+        {ok, Node} ->
+            case emqx_trace:get_trace_filename(Name) of
+                {ok, TraceLog} ->
+                    TraceFiles = collect_trace_file(Node, TraceLog),
+                    ZipDir = emqx_trace:zip_dir(),
+                    Zips = group_trace_file(ZipDir, TraceLog, TraceFiles),
+                    FileName = binary_to_list(Name) ++ ".zip",
+                    ZipFileName = filename:join([ZipDir, FileName]),
+                    {ok, ZipFile} = zip:zip(ZipFileName, Zips, [{cwd, ZipDir}]),
+                    %% emqx_trace:delete_files_after_send(ZipFileName, Zips),
+                    %% TODO use file replace file_binary.(delete file after send is not ready now).
+                    {ok, Binary} = file:read_file(ZipFile),
+                    ZipName = filename:basename(ZipFile),
+                    _ = file:delete(ZipFile),
+                    Headers = #{
+                        <<"content-type">> => <<"application/x-zip">>,
+                        <<"content-disposition">> => iolist_to_binary(
+                            "attachment; filename=" ++ ZipName
+                        )
+                    },
+                    {200, Headers, {file_binary, ZipName, Binary}};
+                {error, not_found} ->
+                    ?NOT_FOUND(Name)
+            end;
         {error, not_found} ->
-            ?NOT_FOUND(Name)
+            ?BAD_REQUEST('NODE_ERROR', <<"Node not found">>)
     end.
 
 group_trace_file(ZipDir, TraceLog, TraceFiles) ->
@@ -503,8 +505,11 @@ group_trace_file(ZipDir, TraceLog, TraceFiles) ->
         TraceFiles
     ).
 
-collect_trace_file(Nodes, TraceLog) ->
-    wrap_rpc(emqx_mgmt_trace_proto_v2:trace_file(Nodes, TraceLog)).
+collect_trace_file(undefined, TraceLog) ->
+    Nodes = mria_mnesia:running_nodes(),
+    wrap_rpc(emqx_mgmt_trace_proto_v2:trace_file(Nodes, TraceLog));
+collect_trace_file(Node, TraceLog) ->
+    wrap_rpc(emqx_mgmt_trace_proto_v2:trace_file([Node], TraceLog)).
 
 collect_trace_file_detail(TraceLog) ->
     Nodes = mria_mnesia:running_nodes(),
@@ -551,21 +556,13 @@ stream_log_file(get, #{bindings := #{name := Name}, query_string := Query}) ->
                 {error, enoent} ->
                     Meta = #{<<"position">> => Position, <<"bytes">> => Bytes},
                     {200, #{meta => Meta, items => <<"">>}};
-                {error, Reason} ->
-                    ?SLOG(error, #{
-                        msg => "read_file_failed",
-                        node => Node,
-                        name => Name,
-                        reason => Reason,
-                        position => Position,
-                        bytes => Bytes
-                    }),
-                    {400, #{code => 'READ_FILE_ERROR', message => Reason}};
+                {error, not_found} ->
+                    ?NOT_FOUND(Name);
                 {badrpc, nodedown} ->
-                    {400, #{code => 'RPC_ERROR', message => "BadRpc node down"}}
+                    ?BAD_REQUEST('NODE_ERROR', <<"Node not found">>)
             end;
         {error, not_found} ->
-            {400, #{code => 'NODE_ERROR', message => <<"Node not found">>}}
+            ?BAD_REQUEST('NODE_ERROR', <<"Node not found">>)
     end.
 
 -spec get_trace_size() -> #{{node(), file:name_all()} => non_neg_integer()}.
@@ -633,8 +630,12 @@ read_file(Path, Offset, Bytes) ->
 parse_node(Query, Default) ->
     try
         case maps:find(<<"node">>, Query) of
-            error -> {ok, Default};
-            {ok, Node} -> {ok, binary_to_existing_atom(Node)}
+            error ->
+                {ok, Default};
+            {ok, NodeBin} ->
+                Node = binary_to_existing_atom(NodeBin),
+                true = lists:member(Node, mria_mnesia:running_nodes()),
+                {ok, Node}
         end
     catch
         _:_ ->
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 72737ba60..68c998c47 100644
--- a/apps/emqx_management/test/emqx_mgmt_api_trace_SUITE.erl
+++ b/apps/emqx_management/test/emqx_mgmt_api_trace_SUITE.erl
@@ -213,6 +213,27 @@ t_log_file(_Config) ->
     Path = api_path("trace/test_client_id/download?node=" ++ atom_to_list(node())),
     {ok, Binary2} = request_api(get, Path, Header),
     ?assertEqual(ZipTab, zip:table(Binary2)),
+    {error, {_, 400, _}, _} =
+        request_api(
+            get,
+            api_path("trace/test_client_id/download?node=unknonwn_node"),
+            Header
+        ),
+    {error, {_, 400, _}, _} =
+        request_api(
+            get,
+            % known atom but unknown node
+            api_path("trace/test_client_id/download?node=undefined"),
+            Header
+        ),
+    ?assertMatch(
+        {error, {"HTTP/1.1", 404, "Not Found"}, _},
+        request_api(
+            get,
+            api_path("trace/test_client_not_found/download?node=" ++ atom_to_list(node())),
+            Header
+        )
+    ),
     ok = emqtt:disconnect(Client),
     ok.
 
@@ -267,6 +288,25 @@ t_stream_log(_Config) ->
     #{<<"meta">> := Meta1, <<"items">> := Bin1} = json(Binary1),
     ?assertEqual(#{<<"position">> => 30, <<"bytes">> => 10}, Meta1),
     ?assertEqual(10, byte_size(Bin1)),
+    {error, {_, 400, _}, _} =
+        request_api(
+            get,
+            api_path("trace/test_stream_log/log?node=unknonwn_node"),
+            Header
+        ),
+    {error, {_, 400, _}, _} =
+        request_api(
+            get,
+            % known atom but not a node
+            api_path("trace/test_stream_log/log?node=undefined"),
+            Header
+        ),
+    {error, {_, 404, _}, _} =
+        request_api(
+            get,
+            api_path("trace/test_stream_log_not_found/log"),
+            Header
+        ),
     unload(),
     ok.
 
diff --git a/changes/v5.0.12-en.md b/changes/v5.0.12-en.md
index b1a643708..d3a0e7a27 100644
--- a/changes/v5.0.12-en.md
+++ b/changes/v5.0.12-en.md
@@ -38,3 +38,5 @@
 
 - Fix shadowing `'client.authenticate'` callbacks by `emqx_authenticator`. Now `emqx_authenticator`
   passes execution to the further callbacks if none of the authenticators matches [#9496](https://github.com/emqx/emqx/pull/9496).
+
+- Return `400` if query param `node` is not a known in `/trace/:id/download?node={node}` [#9478](https://github.com/emqx/emqx/pull/9478).
diff --git a/changes/v5.0.12-zh.md b/changes/v5.0.12-zh.md
index 4d945d81a..26208a7cc 100644
--- a/changes/v5.0.12-zh.md
+++ b/changes/v5.0.12-zh.md
@@ -37,3 +37,5 @@
   - 修复了 EMQX Helm Chart 通过环境变量修改部分 EMQX 的配置项时的错误
 
 - 通过 `emqx_authenticator` 修复隐藏 `'client.authenticate'` 回调。 现在 `emqx_authenticator` 如果没有任何验证器匹配，则将执行传递给进一步的回调 [#9496](https://github.com/emqx/emqx/pull/9496)。
+
+- 如果在调用 `/trace/:id/download?node={node}` 时，`node` 不存在，则会返回 `400` [#9478](https://github.com/emqx/emqx/pull/9478).