Merge pull request #13589 from thalesmg/20240807-r58-rule-id-null

fix(rule engine api): check that user provided id is valid

apps/emqx/rebar.config

@@ -31,7 +31,7 @@
     {esockd, {git, "https://github.com/emqx/esockd", {tag, "5.12.0"}}},
     {ekka, {git, "https://github.com/emqx/ekka", {tag, "0.19.5"}}},
     {gen_rpc, {git, "https://github.com/emqx/gen_rpc", {tag, "3.3.1"}}},
-    {hocon, {git, "https://github.com/emqx/hocon.git", {tag, "0.43.2"}}},
+    {hocon, {git, "https://github.com/emqx/hocon.git", {tag, "0.43.3"}}},
     {emqx_http_lib, {git, "https://github.com/emqx/emqx_http_lib.git", {tag, "0.5.3"}}},
     {pbkdf2, {git, "https://github.com/emqx/erlang-pbkdf2.git", {tag, "2.0.4"}}},
     {recon, {git, "https://github.com/ferd/recon", {tag, "2.5.1"}}},

apps/emqx/src/emqx_config.erl

@@ -621,16 +621,16 @@ save_to_config_map(Conf, RawConf) ->
     ?MODULE:put_raw(RawConf).
 
 -spec save_to_override_conf(boolean(), raw_config(), update_opts()) -> ok | {error, term()}.
-save_to_override_conf(_, undefined, _) ->
+save_to_override_conf(_HasDeprecatedFile, undefined, _) ->
     ok;
-save_to_override_conf(true, RawConf, Opts) ->
+save_to_override_conf(true = _HasDeprecatedFile, RawConf, Opts) ->
     case deprecated_conf_file(Opts) of
         undefined ->
             ok;
         FileName ->
             backup_and_write(FileName, hocon_pp:do(RawConf, Opts))
     end;
-save_to_override_conf(false, RawConf, Opts) ->
+save_to_override_conf(false = _HasDeprecatedFile, RawConf, Opts) ->
     case cluster_hocon_file() of
         undefined ->
             ok;

apps/emqx_rule_engine/src/emqx_rule_engine_api.erl

@@ -20,6 +20,7 @@
 -include_lib("emqx/include/logger.hrl").
 -include_lib("hocon/include/hoconsc.hrl").
 -include_lib("typerefl/include/types.hrl").
+-include_lib("emqx_utils/include/emqx_utils_api.hrl").
 
 -behaviour(minirest_api).
 
@@ -385,16 +386,16 @@ param_path_id() ->
     case maps:get(<<"id">>, Params0, list_to_binary(emqx_utils:gen_id(8))) of
         <<>> ->
             {400, #{code => 'BAD_REQUEST', message => <<"empty rule id is not allowed">>}};
-        Id ->
+        Id when is_binary(Id) ->
             Params = filter_out_request_body(add_metadata(Params0)),
             case emqx_rule_engine:get_rule(Id) of
                 {ok, _Rule} ->
-                    {400, #{code => 'BAD_REQUEST', message => <<"rule id already exists">>}};
+                    ?BAD_REQUEST(<<"rule id already exists">>);
                 not_found ->
                     ConfPath = ?RULE_PATH(Id),
                     case emqx_conf:update(ConfPath, Params, #{override_to => cluster}) of
                         {ok, #{post_config_update := #{emqx_rule_engine := Rule}}} ->
-                            {201, format_rule_info_resp(Rule)};
+                            ?CREATED(format_rule_info_resp(Rule));
                         {error, Reason} ->
                             ?SLOG(
                                 info,
@@ -405,9 +406,11 @@ param_path_id() ->
                                 },
                                 #{tag => ?TAG}
                             ),
-                            {400, #{code => 'BAD_REQUEST', message => ?ERR_BADARGS(Reason)}}
+                            ?BAD_REQUEST(?ERR_BADARGS(Reason))
                     end
-            end
+            end;
+        _BadId ->
+            ?BAD_REQUEST(<<"rule id must be a string">>)
     end.
 
 '/rule_test'(post, #{body := Params}) ->

apps/emqx_rule_engine/test/emqx_rule_engine_api_2_SUITE.erl

@@ -21,6 +21,8 @@
 -include_lib("eunit/include/eunit.hrl").
 -include_lib("common_test/include/ct.hrl").
 
+-import(emqx_common_test_helpers, [on_exit/1]).
+
 %%------------------------------------------------------------------------------
 %% CT boilerplate
 %%------------------------------------------------------------------------------
@@ -48,6 +50,13 @@ app_specs() ->
         emqx_mgmt_api_test_util:emqx_dashboard()
     ].
 
+init_per_testcase(_TestCase, Config) ->
+    Config.
+
+end_per_testcase(_TestCase, _Config) ->
+    emqx_common_test_helpers:call_janitor(),
+    ok.
+
 %%------------------------------------------------------------------------------
 %% Helper fns
 %%------------------------------------------------------------------------------
@@ -124,7 +133,13 @@ create_rule(Overrides) ->
     Method = post,
     Path = emqx_mgmt_api_test_util:api_path(["rules"]),
     Res = request(Method, Path, Params),
-    emqx_mgmt_api_test_util:simplify_result(Res).
+    case emqx_mgmt_api_test_util:simplify_result(Res) of
+        {201, #{<<"id">> := RuleId}} = SRes ->
+            on_exit(fun() -> ok = emqx_rule_engine:delete_rule(RuleId) end),
+            SRes;
+        SRes ->
+            SRes
+    end.
 
 sources_sql(Sources) ->
     Froms = iolist_to_binary(lists:join(<<", ">>, lists:map(fun source_from/1, Sources))),
@@ -586,3 +601,21 @@ t_filter_by_source_and_action(_Config) ->
     ),
 
     ok.
+
+%% Checks that creating a rule with a `null' JSON value id is forbidden.
+t_create_rule_with_null_id(_Config) ->
+    ?assertMatch(
+        {400, #{<<"message">> := <<"rule id must be a string">>}},
+        create_rule(#{<<"id">> => null})
+    ),
+    %% The string `"null"' should be fine.
+    ?assertMatch(
+        {201, _},
+        create_rule(#{<<"id">> => <<"null">>})
+    ),
+    ?assertMatch({201, _}, create_rule(#{})),
+    ?assertMatch(
+        {200, #{<<"data">> := [_, _]}},
+        list_rules([])
+    ),
+    ok.

changes/ce/fix-13589.en.md

@@ -0,0 +1 @@
+Fixed an issue where creating a rule with a null id via the HTTP API was allowed, which could lead to an inconsistent configuration.

mix.exs

@@ -184,7 +184,7 @@ defmodule EMQXUmbrella.MixProject do
   def common_dep(:ekka), do: {:ekka, github: "emqx/ekka", tag: "0.19.5", override: true}
   def common_dep(:esockd), do: {:esockd, github: "emqx/esockd", tag: "5.12.0", override: true}
   def common_dep(:gproc), do: {:gproc, github: "emqx/gproc", tag: "0.9.0.1", override: true}
-  def common_dep(:hocon), do: {:hocon, github: "emqx/hocon", tag: "0.43.2", override: true}
+  def common_dep(:hocon), do: {:hocon, github: "emqx/hocon", tag: "0.43.3", override: true}
   def common_dep(:lc), do: {:lc, github: "emqx/lc", tag: "0.3.2", override: true}
   # in conflict by ehttpc and emqtt
   def common_dep(:gun), do: {:gun, github: "emqx/gun", tag: "1.3.11", override: true}

rebar.config

@@ -98,7 +98,7 @@
     {system_monitor, {git, "https://github.com/ieQu1/system_monitor", {tag, "3.0.5"}}},
     {getopt, "1.0.2"},
     {snabbkaffe, {git, "https://github.com/kafka4beam/snabbkaffe.git", {tag, "1.0.10"}}},
-    {hocon, {git, "https://github.com/emqx/hocon.git", {tag, "0.43.2"}}},
+    {hocon, {git, "https://github.com/emqx/hocon.git", {tag, "0.43.3"}}},
     {emqx_http_lib, {git, "https://github.com/emqx/emqx_http_lib.git", {tag, "0.5.3"}}},
     {esasl, {git, "https://github.com/emqx/esasl", {tag, "0.2.1"}}},
     {jose, {git, "https://github.com/potatosalad/erlang-jose", {tag, "1.11.2"}}},