diff --git a/apps/emqx/rebar.config b/apps/emqx/rebar.config index 6e629e9fa..404c48817 100644 --- a/apps/emqx/rebar.config +++ b/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"}}}, diff --git a/apps/emqx/src/emqx_config.erl b/apps/emqx/src/emqx_config.erl index 378696d2c..95d764014 100644 --- a/apps/emqx/src/emqx_config.erl +++ b/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; diff --git a/apps/emqx_rule_engine/src/emqx_rule_engine_api.erl b/apps/emqx_rule_engine/src/emqx_rule_engine_api.erl index 6c286ff2b..f95cb1f53 100644 --- a/apps/emqx_rule_engine/src/emqx_rule_engine_api.erl +++ b/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}) -> diff --git a/apps/emqx_rule_engine/test/emqx_rule_engine_api_2_SUITE.erl b/apps/emqx_rule_engine/test/emqx_rule_engine_api_2_SUITE.erl index 8a866cc69..6154cfad1 100644 --- a/apps/emqx_rule_engine/test/emqx_rule_engine_api_2_SUITE.erl +++ b/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. diff --git a/changes/ce/fix-13589.en.md b/changes/ce/fix-13589.en.md new file mode 100644 index 000000000..a6df6dad4 --- /dev/null +++ b/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. diff --git a/mix.exs b/mix.exs index d292e0069..c7d126416 100644 --- a/mix.exs +++ b/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} diff --git a/rebar.config b/rebar.config index ad70d128d..7c05b2ad0 100644 --- a/rebar.config +++ b/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"}}},