Merge pull request #6800 from terry-xiaoyu/rule_empty_ids

fix(rule): deny POST rules or resources with empty ids

apps/emqx_rule_engine/src/emqx_rule_engine.appup.src

@@ -6,6 +6,7 @@
     , {load_module,emqx_rule_events,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_engine_api,brutal_purge,soft_purge,[]}
     ]},
    {"4.3.5",
     [ {update, emqx_rule_metrics, {advanced, ["4.3.5"]}}
@@ -13,6 +14,7 @@
     , {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_engine_api,brutal_purge,soft_purge,[]}
     ]},
    {"4.3.4",
     [ {update, emqx_rule_metrics, {advanced, ["4.3.4"]}}
@@ -20,6 +22,7 @@
     , {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_engine_api,brutal_purge,soft_purge,[]}
     ]},
    {"4.3.3",
     [ {update, emqx_rule_metrics, {advanced, ["4.3.3"]}}
@@ -28,6 +31,7 @@
     , {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_engine_api,brutal_purge,soft_purge,[]}
     ]},
    {"4.3.2",
     [ {update, emqx_rule_metrics, {advanced, ["4.3.2"]}}
@@ -37,6 +41,7 @@
     , {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_engine_api,brutal_purge,soft_purge,[]}
     ]},
    {"4.3.1",
     [ {update, emqx_rule_metrics, {advanced, ["4.3.1"]}}
@@ -46,6 +51,7 @@
     , {apply,{emqx_stats,cancel_update,[rule_registery_stats]}}
     , {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_engine_api,brutal_purge,soft_purge,[]}
     ]},
    {"4.3.0",
     [ {update, emqx_rule_metrics, {advanced, ["4.3.0"]}}
@@ -56,6 +62,7 @@
     , {apply,{emqx_stats,cancel_update,[rule_registery_stats]}}
     , {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_engine_api,brutal_purge,soft_purge,[]}
     ]},
    {<<".*">>, []}
   ],
@@ -65,6 +72,7 @@
     , {load_module,emqx_rule_events,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_engine_api,brutal_purge,soft_purge,[]}
     ]},
    {"4.3.5",
     [ {update, emqx_rule_metrics, {advanced, ["4.3.5"]}}
@@ -72,6 +80,7 @@
     , {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_engine_api,brutal_purge,soft_purge,[]}
     ]},
    {"4.3.4",
     [ {update, emqx_rule_metrics, {advanced, ["4.3.4"]}}
@@ -79,6 +88,7 @@
     , {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_engine_api,brutal_purge,soft_purge,[]}
     ]},
    {"4.3.3",
     [ {update, emqx_rule_metrics, {advanced, ["4.3.3"]}}
@@ -87,6 +97,7 @@
     , {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_engine_api,brutal_purge,soft_purge,[]}
     ]},
    {"4.3.2",
     [ {update, emqx_rule_metrics, {advanced, ["4.3.2"]}}
@@ -96,6 +107,7 @@
     , {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_engine_api,brutal_purge,soft_purge,[]}
     ]},
    {"4.3.1",
     [ {update, emqx_rule_metrics, {advanced, ["4.3.1"]}}
@@ -105,6 +117,7 @@
     , {apply,{emqx_stats,cancel_update,[rule_registery_stats]}}
     , {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_engine_api,brutal_purge,soft_purge,[]}
     ]},
    {"4.3.0",
     [ {update, emqx_rule_metrics, {advanced, ["4.3.0"]}}
@@ -115,6 +128,7 @@
     , {apply,{emqx_stats,cancel_update,[rule_registery_stats]}}
     , {load_module,emqx_rule_actions,brutal_purge,soft_purge,[]}
     , {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}
+    , {load_module,emqx_rule_engine_api,brutal_purge,soft_purge,[]}
     ]},
    {<<".*">>, []}
   ]

apps/emqx_rule_engine/src/emqx_rule_engine_api.erl

@@ -211,20 +211,32 @@ test_rule_sql(Params) ->
     end.
 
 do_create_rule(Params) ->
-    case emqx_rule_engine:create_rule(parse_rule_params(Params)) of
+    case parse_rule_params(Params) of
-        {ok, Rule} -> return({ok, record_to_map(Rule)});
+        {ok, ParsedParams} ->
-        {error, {action_not_found, ActionName}} ->
+            case emqx_rule_engine:create_rule(ParsedParams) of
-            return({error, 400, ?ERR_NO_ACTION(ActionName)});
+                {ok, Rule} -> return({ok, record_to_map(Rule)});
+                {error, {action_not_found, ActionName}} ->
+                    return({error, 400, ?ERR_NO_ACTION(ActionName)});
+                {error, Reason} ->
+                    ?LOG(error, "~p failed: ~0p", [?FUNCTION_NAME, Reason]),
+                    return({error, 400, ?ERR_BADARGS(Reason)})
+            end;
         {error, Reason} ->
             ?LOG(error, "~p failed: ~0p", [?FUNCTION_NAME, Reason]),
             return({error, 400, ?ERR_BADARGS(Reason)})
     end.
 
 update_rule(#{id := Id}, Params) ->
-    case emqx_rule_engine:update_rule(parse_rule_params(Params, #{id => Id})) of
+    case parse_rule_params(Params, #{id => Id}) of
-        {ok, Rule} -> return({ok, record_to_map(Rule)});
+        {ok, ParsedParams} ->
-        {error, {not_found, RuleId}} ->
+            case emqx_rule_engine:update_rule(ParsedParams) of
-            return({error, 400, ?ERR_NO_RULE(RuleId)});
+                {ok, Rule} -> return({ok, record_to_map(Rule)});
+                {error, {not_found, RuleId}} ->
+                    return({error, 400, ?ERR_NO_RULE(RuleId)});
+                {error, Reason} ->
+                    ?LOG(error, "~p failed: ~0p", [?FUNCTION_NAME, Reason]),
+                    return({error, 400, ?ERR_BADARGS(Reason)})
+            end;
         {error, Reason} ->
             ?LOG(error, "~p failed: ~0p", [?FUNCTION_NAME, Reason]),
             return({error, 400, ?ERR_BADARGS(Reason)})
@@ -481,7 +493,9 @@ printable_actions(Actions) ->
 parse_rule_params(Params) ->
     parse_rule_params(Params, #{description => <<"">>}).
 parse_rule_params([], Rule) ->
-    Rule;
+    {ok, Rule};
+parse_rule_params([{<<"id">>, <<>>} | _], _) ->
+    {error, {empty_string_not_allowed, id}};
 parse_rule_params([{<<"id">>, Id} | Params], Rule) ->
     parse_rule_params(Params, Rule#{id => Id});
 parse_rule_params([{<<"rawsql">>, RawSQL} | Params], Rule) ->
@@ -516,6 +530,8 @@ parse_resource_params(Params) ->
     parse_resource_params(Params, #{config => #{}, description => <<"">>}).
 parse_resource_params([], Res) ->
     {ok, Res};
+parse_resource_params([{<<"id">>, <<>>} | _], _Res) ->
+    {error, {empty_string_not_allowed, id}};
 parse_resource_params([{<<"id">>, Id} | Params], Res) ->
     parse_resource_params(Params, Res#{id => Id});
 parse_resource_params([{<<"type">>, ResourceType} | Params], Res) ->