diff --git a/apps/emqx_bridge/src/emqx_bridge.erl b/apps/emqx_bridge/src/emqx_bridge.erl index edd21a239..4fa6cd346 100644 --- a/apps/emqx_bridge/src/emqx_bridge.erl +++ b/apps/emqx_bridge/src/emqx_bridge.erl @@ -55,6 +55,7 @@ ]). -export([config_key_path/0]). +-export([validate_bridge_name/1]). %% exported for `emqx_telemetry' -export([get_basic_usage_info/0]). @@ -96,6 +97,9 @@ -define(ROOT_KEY, bridges). +%% See `hocon_tconf` +-define(MAP_KEY_RE, <<"^[A-Za-z0-9]+[A-Za-z0-9-_]*$">>). + load() -> Bridges = emqx:get_config([?ROOT_KEY], #{}), lists:foreach( @@ -580,3 +584,19 @@ get_basic_usage_info() -> _:_ -> InitialAcc end. + +validate_bridge_name(BridgeName0) -> + BridgeName = to_bin(BridgeName0), + case re:run(BridgeName, ?MAP_KEY_RE, [{capture, none}]) of + match -> + ok; + nomatch -> + {error, #{ + kind => validation_error, + reason => bad_bridge_name, + value => BridgeName + }} + end. + +to_bin(A) when is_atom(A) -> atom_to_binary(A, utf8); +to_bin(B) when is_binary(B) -> B. diff --git a/apps/emqx_bridge/src/emqx_bridge_api.erl b/apps/emqx_bridge/src/emqx_bridge_api.erl index 3190a2ef9..e49b54d67 100644 --- a/apps/emqx_bridge/src/emqx_bridge_api.erl +++ b/apps/emqx_bridge/src/emqx_bridge_api.erl @@ -609,6 +609,8 @@ create_or_update_bridge(BridgeType, BridgeName, Conf, HttpStatusCode) -> case emqx_bridge:create(BridgeType, BridgeName, Conf) of {ok, _} -> lookup_from_all_nodes(BridgeType, BridgeName, HttpStatusCode); + {error, {pre_config_update, _HandlerMod, Reason}} when is_map(Reason) -> + ?BAD_REQUEST(map_to_json(redact(Reason))); {error, Reason} when is_map(Reason) -> ?BAD_REQUEST(map_to_json(redact(Reason))) end. diff --git a/apps/emqx_bridge/src/emqx_bridge_app.erl b/apps/emqx_bridge/src/emqx_bridge_app.erl index 3bae55090..d0dd7da2b 100644 --- a/apps/emqx_bridge/src/emqx_bridge_app.erl +++ b/apps/emqx_bridge/src/emqx_bridge_app.erl @@ -62,11 +62,16 @@ pre_config_update(_, {Oper, _Type, _Name}, OldConfig) -> %% to save the 'enable' to the config files {ok, OldConfig#{<<"enable">> => operation_to_enable(Oper)}}; pre_config_update(Path, Conf, _OldConfig) when is_map(Conf) -> - case emqx_connector_ssl:convert_certs(filename:join(Path), Conf) of - {error, Reason} -> - {error, Reason}; - {ok, ConfNew} -> - {ok, ConfNew} + case validate_bridge_name(Path) of + ok -> + case emqx_connector_ssl:convert_certs(filename:join(Path), Conf) of + {error, Reason} -> + {error, Reason}; + {ok, ConfNew} -> + {ok, ConfNew} + end; + Error -> + Error end. post_config_update([bridges, BridgeType, BridgeName], '$remove', _, _OldConf, _AppEnvs) -> @@ -97,3 +102,12 @@ post_config_update([bridges, BridgeType, BridgeName], _Req, NewConf, OldConf, _A %% internal functions operation_to_enable(disable) -> false; operation_to_enable(enable) -> true. + +validate_bridge_name(Path) -> + [RootKey] = emqx_bridge:config_key_path(), + case Path of + [RootKey, _BridgeType, BridgeName] -> + emqx_bridge:validate_bridge_name(BridgeName); + _ -> + ok + end. diff --git a/apps/emqx_bridge/test/emqx_bridge_SUITE.erl b/apps/emqx_bridge/test/emqx_bridge_SUITE.erl index 1cbc94d24..c9157d9e6 100644 --- a/apps/emqx_bridge/test/emqx_bridge_SUITE.erl +++ b/apps/emqx_bridge/test/emqx_bridge_SUITE.erl @@ -26,19 +26,20 @@ all() -> emqx_common_test_helpers:all(?MODULE). init_per_suite(Config) -> - _ = application:load(emqx_conf), - %% to avoid inter-suite dependencies - application:stop(emqx_connector), - ok = emqx_common_test_helpers:start_apps([emqx, emqx_bridge]), - Config. + Apps = emqx_cth_suite:start( + [ + emqx, + emqx_conf, + emqx_bridge + ], + #{work_dir => ?config(priv_dir, Config)} + ), + [{apps, Apps} | Config]. -end_per_suite(_Config) -> - emqx_common_test_helpers:stop_apps([ - emqx, - emqx_bridge, - emqx_resource, - emqx_connector - ]). +end_per_suite(Config) -> + Apps = ?config(apps, Config), + ok = emqx_cth_suite:stop(Apps), + ok. init_per_testcase(t_get_basic_usage_info_1, Config) -> {ok, _} = emqx_cluster_rpc:start_link(node(), emqx_cluster_rpc, 1000), @@ -180,6 +181,31 @@ t_update_ssl_conf(Config) -> ?assertMatch({error, enoent}, file:list_dir(CertDir)), ok. +t_create_with_bad_name(_Config) -> + Path = [bridges, mqtt, 'test_哈哈'], + Conf = #{ + <<"bridge_mode">> => false, + <<"clean_start">> => true, + <<"keepalive">> => <<"60s">>, + <<"proto_ver">> => <<"v4">>, + <<"server">> => <<"127.0.0.1:1883">>, + <<"ssl">> => + #{ + %% needed to trigger pre_config_update + <<"certfile">> => cert_file("certfile"), + <<"enable">> => true + } + }, + ?assertMatch( + {error, + {pre_config_update, emqx_bridge_app, #{ + reason := bad_bridge_name, + kind := validation_error + }}}, + emqx:update_config(Path, Conf) + ), + ok. + data_file(Name) -> Dir = code:lib_dir(emqx_bridge, test), {ok, Bin} = file:read_file(filename:join([Dir, "data", Name])), diff --git a/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl b/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl index d8e697987..31c6bcab1 100644 --- a/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl +++ b/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl @@ -1335,6 +1335,35 @@ t_cluster_later_join_metrics(Config) -> ), ok. +t_create_with_bad_name(Config) -> + Port = ?config(port, Config), + URL1 = ?URL(Port, "path1"), + Name = <<"test_哈哈">>, + BadBridgeParams = + emqx_utils_maps:deep_merge( + ?HTTP_BRIDGE(URL1, Name), + #{ + <<"ssl">> => + #{ + <<"enable">> => true, + <<"certfile">> => cert_file("certfile") + } + } + ), + {ok, 400, #{ + <<"code">> := <<"BAD_REQUEST">>, + <<"message">> := Msg0 + }} = + request_json( + post, + uri(["bridges"]), + BadBridgeParams, + Config + ), + Msg = emqx_utils_json:decode(Msg0, [return_maps]), + ?assertMatch(#{<<"reason">> := <<"bad_bridge_name">>}, Msg), + ok. + validate_resource_request_ttl(single, Timeout, Name) -> SentData = #{payload => <<"Hello EMQX">>, timestamp => 1668602148000}, BridgeID = emqx_bridge_resource:bridge_id(?BRIDGE_TYPE_HTTP, Name), @@ -1418,3 +1447,11 @@ str(S) when is_binary(S) -> binary_to_list(S). json(B) when is_binary(B) -> emqx_utils_json:decode(B, [return_maps]). + +data_file(Name) -> + Dir = code:lib_dir(emqx_bridge, test), + {ok, Bin} = file:read_file(filename:join([Dir, "data", Name])), + Bin. + +cert_file(Name) -> + data_file(filename:join(["certs", Name])). diff --git a/changes/ce/fix-11540.en.md b/changes/ce/fix-11540.en.md new file mode 100644 index 000000000..7486e7133 --- /dev/null +++ b/changes/ce/fix-11540.en.md @@ -0,0 +1 @@ +Improved HTTP response when attempting to create a bridge with an invalid name.