fix(mgmt api): validate clientid to avoid crashes and 500 HTTP errors

apps/emqx_management/src/emqx_management.app.src

@@ -1,6 +1,6 @@
 {application, emqx_management,
  [{description, "EMQ X Management API and CLI"},
-  {vsn, "4.3.6"}, % strict semver, bump manually!
+  {vsn, "4.3.7"}, % strict semver, bump manually!
   {modules, []},
   {registered, [emqx_management_sup]},
   {applications, [kernel,stdlib,minirest]},

apps/emqx_management/src/emqx_management.appup.src

@@ -1,13 +1,13 @@
 %% -*- mode: erlang -*-
 {VSN,
- [ {<<"4.3.[0-5]">>,
+ [ {<<"4.3.[0-6]">>,
     [ {apply,{minirest,stop_http,['http:management']}},
       {apply,{minirest,stop_http,['https:management']}},
       {restart_application, emqx_management}
     ]},
    {<<".*">>, []}
  ],
- [ {<<"4.3.[0-5]">>,
+ [ {<<"4.3.[0-6]">>,
     [ {apply,{minirest,stop_http,['http:management']}},
       {apply,{minirest,stop_http,['https:management']}},
       {restart_application, emqx_management}

apps/emqx_management/src/emqx_mgmt_api_pubsub.erl

@@ -147,6 +147,8 @@ loop_unsubscribe([Params | ParamsN], Acc) ->
                code => Code},
     loop_unsubscribe(ParamsN, [Result | Acc]).
 
+do_subscribe(ClientId, _Topics, _QoS) when not is_binary(ClientId) ->
+    {ok, ?ERROR8, <<"bad clientid: must be string">>};
 do_subscribe(_ClientId, [], _QoS) ->
     {ok, ?ERROR15, bad_topic};
 do_subscribe(ClientId, Topics, QoS) ->
@@ -156,6 +158,8 @@ do_subscribe(ClientId, Topics, QoS) ->
         _ -> ok
     end.
 
+do_publish(ClientId, _Topics, _Qos, _Retain, _Payload) when not is_binary(ClientId) ->
+    {ok, ?ERROR8, <<"bad clientid: must be string">>};
 do_publish(_ClientId, [], _Qos, _Retain, _Payload) ->
     {ok, ?ERROR15, bad_topic};
 do_publish(ClientId, Topics, Qos, Retain, Payload) ->
@@ -166,6 +170,8 @@ do_publish(ClientId, Topics, Qos, Retain, Payload) ->
     end, Topics),
     {ok, MsgIds}.
 
+do_unsubscribe(ClientId, _Topic) when not is_binary(ClientId) ->
+    {ok, ?ERROR8, <<"bad clientid: must be string">>};
 do_unsubscribe(ClientId, Topic) ->
     case validate_by_filter(Topic) of
         true ->

apps/emqx_management/test/emqx_mgmt_api_SUITE.erl

@@ -403,6 +403,26 @@ t_pubsub(_) ->
                                    <<"topic">> => <<"">>}),
     ?assertEqual(?ERROR15, get(<<"code">>, BadTopic3)),
 
+
+    {ok, BadClient1} = request_api(post, api_path(["mqtt/subscribe"]), [], auth_header_(),
+                                 #{<<"clientid">> => 1,
+                                   <<"topics">> => <<"mytopic">>,
+                                   <<"qos">> => 2}),
+    ?assertEqual(?ERROR8, get(<<"code">>, BadClient1)),
+
+    {ok, BadClient2} = request_api(post, api_path(["mqtt/publish"]), [], auth_header_(),
+                                 #{<<"clientid">> => 1,
+                                   <<"topics">> => <<"mytopic">>,
+                                   <<"qos">> => 1,
+                                   <<"payload">> => <<"hello">>}),
+    ?assertEqual(?ERROR8, get(<<"code">>, BadClient2)),
+
+    {ok, BadClient3} = request_api(post, api_path(["mqtt/unsubscribe"]), [], auth_header_(),
+                                 #{<<"clientid">> => 1,
+                                   <<"topic">> => <<"mytopic">>}),
+    ?assertEqual(?ERROR8, get(<<"code">>, BadClient3)),
+
+
     meck:new(emqx_mgmt, [passthrough, no_history]),
     meck:expect(emqx_mgmt, unsubscribe, 2, fun(_, _) -> {error, undefined} end),
     {ok, NotFound2} = request_api(post, api_path(["mqtt/unsubscribe"]), [], auth_header_(),