diff --git a/apps/emqx_bridge/src/emqx_bridge.erl b/apps/emqx_bridge/src/emqx_bridge.erl index 51bdfb084..64bec3a4e 100644 --- a/apps/emqx_bridge/src/emqx_bridge.erl +++ b/apps/emqx_bridge/src/emqx_bridge.erl @@ -55,7 +55,6 @@ ]). -export([config_key_path/0]). --export([validate_bridge_name/1]). %% exported for `emqx_telemetry' -export([get_basic_usage_info/0]). @@ -268,7 +267,12 @@ config_key_path() -> pre_config_update([?ROOT_KEY], RawConf, RawConf) -> {ok, RawConf}; pre_config_update([?ROOT_KEY], NewConf, _RawConf) -> - {ok, convert_certs(NewConf)}. + case multi_validate_bridge_names(NewConf) of + ok -> + {ok, convert_certs(NewConf)}; + Error -> + Error + end. post_config_update([?ROOT_KEY], _Req, NewConf, OldConf, _AppEnv) -> #{added := Added, removed := Removed, changed := Updated} = @@ -657,17 +661,13 @@ 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 - }} +validate_bridge_name(BridgeName) -> + try + _ = emqx_resource:validate_name(to_bin(BridgeName)), + ok + catch + throw:Error -> + {error, Error} end. to_bin(A) when is_atom(A) -> atom_to_binary(A, utf8); @@ -675,3 +675,31 @@ to_bin(B) when is_binary(B) -> B. upgrade_type(Type) -> emqx_bridge_lib:upgrade_type(Type). + +multi_validate_bridge_names(Conf) -> + BridgeTypeAndNames = + [ + {Type, Name} + || {Type, NameToConf} <- maps:to_list(Conf), + {Name, _Conf} <- maps:to_list(NameToConf) + ], + BadBridges = + lists:filtermap( + fun({Type, Name}) -> + case validate_bridge_name(Name) of + ok -> false; + _Error -> {true, #{type => Type, name => Name}} + end + end, + BridgeTypeAndNames + ), + case BadBridges of + [] -> + ok; + [_ | _] -> + {error, #{ + kind => validation_error, + reason => bad_bridge_names, + bad_bridges => BadBridges + }} + end. diff --git a/apps/emqx_bridge/src/emqx_bridge_app.erl b/apps/emqx_bridge/src/emqx_bridge_app.erl index cd54d31e7..321f59f28 100644 --- a/apps/emqx_bridge/src/emqx_bridge_app.erl +++ b/apps/emqx_bridge/src/emqx_bridge_app.erl @@ -63,7 +63,7 @@ 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 validate_bridge_name(Path) of + case validate_bridge_name_in_config(Path) of ok -> case emqx_connector_ssl:convert_certs(filename:join(Path), Conf) of {error, Reason} -> @@ -104,11 +104,23 @@ post_config_update([bridges, BridgeType, BridgeName], _Req, NewConf, OldConf, _A operation_to_enable(disable) -> false; operation_to_enable(enable) -> true. -validate_bridge_name(Path) -> +validate_bridge_name_in_config(Path) -> [RootKey] = emqx_bridge:config_key_path(), case Path of [RootKey, _BridgeType, BridgeName] -> - emqx_bridge:validate_bridge_name(BridgeName); + validate_bridge_name(BridgeName); _ -> ok end. + +to_bin(A) when is_atom(A) -> atom_to_binary(A, utf8); +to_bin(B) when is_binary(B) -> B. + +validate_bridge_name(BridgeName) -> + try + _ = emqx_resource:validate_name(to_bin(BridgeName)), + ok + catch + throw:Error -> + {error, Error} + end. diff --git a/apps/emqx_bridge/test/emqx_bridge_SUITE.erl b/apps/emqx_bridge/test/emqx_bridge_SUITE.erl index 96c3c29ca..b29ba154e 100644 --- a/apps/emqx_bridge/test/emqx_bridge_SUITE.erl +++ b/apps/emqx_bridge/test/emqx_bridge_SUITE.erl @@ -199,13 +199,41 @@ t_create_with_bad_name(_Config) -> ?assertMatch( {error, {pre_config_update, emqx_bridge_app, #{ - reason := bad_bridge_name, + reason := <<"only 0-9a-zA-Z_- is allowed in resource name", _/binary>>, kind := validation_error }}}, emqx:update_config(Path, Conf) ), ok. +t_create_with_bad_name_root(_Config) -> + BadBridgeName = <<"test_哈哈">>, + BridgeConf = #{ + <<"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 + } + }, + Conf = #{<<"mqtt">> => #{BadBridgeName => BridgeConf}}, + Path = [bridges], + ?assertMatch( + {error, + {pre_config_update, _ConfigHandlerMod, #{ + kind := validation_error, + reason := bad_bridge_names, + bad_bridges := [#{type := <<"mqtt">>, name := BadBridgeName}] + }}}, + 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 c0339660e..99a2bc8cd 100644 --- a/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl +++ b/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl @@ -1362,7 +1362,13 @@ t_create_with_bad_name(Config) -> Config ), Msg = emqx_utils_json:decode(Msg0, [return_maps]), - ?assertMatch(#{<<"reason">> := <<"bad_bridge_name">>}, Msg), + ?assertMatch( + #{ + <<"kind">> := <<"validation_error">>, + <<"reason">> := <<"only 0-9a-zA-Z_- is allowed in resource name", _/binary>> + }, + Msg + ), ok. validate_resource_request_ttl(single, Timeout, Name) -> diff --git a/apps/emqx_bridge/test/emqx_bridge_v1_compatibility_layer_SUITE.erl b/apps/emqx_bridge/test/emqx_bridge_v1_compatibility_layer_SUITE.erl index f3b7fb685..c714b858a 100644 --- a/apps/emqx_bridge/test/emqx_bridge_v1_compatibility_layer_SUITE.erl +++ b/apps/emqx_bridge/test/emqx_bridge_v1_compatibility_layer_SUITE.erl @@ -150,7 +150,8 @@ con_schema() -> fields("connector") -> [ {enable, hoconsc:mk(any(), #{})}, - {resource_opts, hoconsc:mk(map(), #{})} + {resource_opts, hoconsc:mk(map(), #{})}, + {ssl, hoconsc:ref(ssl)} ]; fields("api_post") -> [ @@ -159,7 +160,9 @@ fields("api_post") -> {type, hoconsc:mk(bridge_type(), #{})}, {send_to, hoconsc:mk(atom(), #{})} | fields("connector") - ]. + ]; +fields(ssl) -> + emqx_schema:client_ssl_opts_schema(#{required => false}). con_config() -> #{ @@ -806,3 +809,27 @@ t_scenario_2(Config) -> ?assert(is_rule_enabled(RuleId2)), ok. + +t_create_with_bad_name(_Config) -> + BadBridgeName = <<"test_哈哈">>, + %% Note: must contain SSL options to trigger bug. + Cacertfile = emqx_common_test_helpers:app_path( + emqx, + filename:join(["etc", "certs", "cacert.pem"]) + ), + Opts = #{ + name => BadBridgeName, + overrides => #{ + <<"ssl">> => + #{<<"cacertfile">> => Cacertfile} + } + }, + {error, + {{_, 400, _}, _, #{ + <<"code">> := <<"BAD_REQUEST">>, + <<"message">> := #{ + <<"kind">> := <<"validation_error">>, + <<"reason">> := <<"only 0-9a-zA-Z_- is allowed in resource name", _/binary>> + } + }}} = create_bridge_http_api_v1(Opts), + ok. diff --git a/apps/emqx_connector/src/emqx_connector.app.src b/apps/emqx_connector/src/emqx_connector.app.src index 7ecabb0ff..d23a36e49 100644 --- a/apps/emqx_connector/src/emqx_connector.app.src +++ b/apps/emqx_connector/src/emqx_connector.app.src @@ -1,7 +1,7 @@ %% -*- mode: erlang -*- {application, emqx_connector, [ {description, "EMQX Data Integration Connectors"}, - {vsn, "0.1.33"}, + {vsn, "0.1.34"}, {registered, []}, {mod, {emqx_connector_app, []}}, {applications, [ diff --git a/apps/emqx_connector/src/emqx_connector.erl b/apps/emqx_connector/src/emqx_connector.erl index f07e038d2..30654bb13 100644 --- a/apps/emqx_connector/src/emqx_connector.erl +++ b/apps/emqx_connector/src/emqx_connector.erl @@ -108,18 +108,28 @@ config_key_path() -> pre_config_update([?ROOT_KEY], RawConf, RawConf) -> {ok, RawConf}; pre_config_update([?ROOT_KEY], NewConf, _RawConf) -> - {ok, convert_certs(NewConf)}; + case multi_validate_connector_names(NewConf) of + ok -> + {ok, convert_certs(NewConf)}; + Error -> + Error + end; pre_config_update(_, {_Oper, _, _}, undefined) -> {error, connector_not_found}; 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_connector_name_in_config(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. operation_to_enable(disable) -> false; @@ -458,3 +468,51 @@ ensure_no_channels(Configs) -> {error, Reason, _State} -> {error, Reason} end. + +to_bin(A) when is_atom(A) -> atom_to_binary(A, utf8); +to_bin(B) when is_binary(B) -> B. + +validate_connector_name(ConnectorName) -> + try + _ = emqx_resource:validate_name(to_bin(ConnectorName)), + ok + catch + throw:Error -> + {error, Error} + end. + +validate_connector_name_in_config(Path) -> + case Path of + [?ROOT_KEY, _ConnectorType, ConnectorName] -> + validate_connector_name(ConnectorName); + _ -> + ok + end. + +multi_validate_connector_names(Conf) -> + ConnectorTypeAndNames = + [ + {Type, Name} + || {Type, NameToConf} <- maps:to_list(Conf), + {Name, _Conf} <- maps:to_list(NameToConf) + ], + BadConnectors = + lists:filtermap( + fun({Type, Name}) -> + case validate_connector_name(Name) of + ok -> false; + _Error -> {true, #{type => Type, name => Name}} + end + end, + ConnectorTypeAndNames + ), + case BadConnectors of + [] -> + ok; + [_ | _] -> + {error, #{ + kind => validation_error, + reason => bad_connector_names, + bad_connectors => BadConnectors + }} + end. diff --git a/apps/emqx_connector/test/emqx_connector_SUITE.erl b/apps/emqx_connector/test/emqx_connector_SUITE.erl index a62b5ed95..ee7e29741 100644 --- a/apps/emqx_connector/test/emqx_connector_SUITE.erl +++ b/apps/emqx_connector/test/emqx_connector_SUITE.erl @@ -204,6 +204,71 @@ t_remove_fail(_Config) -> ), ok. +t_create_with_bad_name_direct_path({init, Config}) -> + meck:new(emqx_connector_ee_schema, [passthrough]), + meck:expect(emqx_connector_ee_schema, resource_type, 1, ?CONNECTOR), + meck:new(?CONNECTOR, [non_strict]), + meck:expect(?CONNECTOR, callback_mode, 0, async_if_possible), + meck:expect(?CONNECTOR, on_start, 2, {ok, connector_state}), + meck:expect(?CONNECTOR, on_stop, 2, ok), + meck:expect(?CONNECTOR, on_get_status, 2, connected), + Config; +t_create_with_bad_name_direct_path({'end', _Config}) -> + meck:unload(), + ok; +t_create_with_bad_name_direct_path(_Config) -> + Path = [connectors, kafka_producer, 'test_哈哈'], + ConnConfig0 = connector_config(), + %% Note: must contain SSL options to trigger original bug. + Cacertfile = emqx_common_test_helpers:app_path( + emqx, + filename:join(["etc", "certs", "cacert.pem"]) + ), + ConnConfig = ConnConfig0#{<<"ssl">> => #{<<"cacertfile">> => Cacertfile}}, + ?assertMatch( + {error, + {pre_config_update, _ConfigHandlerMod, #{ + kind := validation_error, + reason := <<"only 0-9a-zA-Z_- is allowed in resource name", _/binary>> + }}}, + emqx:update_config(Path, ConnConfig) + ), + ok. + +t_create_with_bad_name_root_path({init, Config}) -> + meck:new(emqx_connector_ee_schema, [passthrough]), + meck:expect(emqx_connector_ee_schema, resource_type, 1, ?CONNECTOR), + meck:new(?CONNECTOR, [non_strict]), + meck:expect(?CONNECTOR, callback_mode, 0, async_if_possible), + meck:expect(?CONNECTOR, on_start, 2, {ok, connector_state}), + meck:expect(?CONNECTOR, on_stop, 2, ok), + meck:expect(?CONNECTOR, on_get_status, 2, connected), + Config; +t_create_with_bad_name_root_path({'end', _Config}) -> + meck:unload(), + ok; +t_create_with_bad_name_root_path(_Config) -> + Path = [connectors], + BadConnectorName = <<"test_哈哈">>, + ConnConfig0 = connector_config(), + %% Note: must contain SSL options to trigger original bug. + Cacertfile = emqx_common_test_helpers:app_path( + emqx, + filename:join(["etc", "certs", "cacert.pem"]) + ), + ConnConfig = ConnConfig0#{<<"ssl">> => #{<<"cacertfile">> => Cacertfile}}, + Conf = #{<<"kafka_producer">> => #{BadConnectorName => ConnConfig}}, + ?assertMatch( + {error, + {pre_config_update, _ConfigHandlerMod, #{ + kind := validation_error, + reason := bad_connector_names, + bad_connectors := [#{type := <<"kafka_producer">>, name := BadConnectorName}] + }}}, + emqx:update_config(Path, Conf) + ), + ok. + %% helpers connector_config() -> diff --git a/apps/emqx_connector/test/emqx_connector_api_SUITE.erl b/apps/emqx_connector/test/emqx_connector_api_SUITE.erl index becbc8791..f6609808f 100644 --- a/apps/emqx_connector/test/emqx_connector_api_SUITE.erl +++ b/apps/emqx_connector/test/emqx_connector_api_SUITE.erl @@ -652,6 +652,28 @@ t_connectors_probe(Config) -> ), ok. +t_create_with_bad_name(Config) -> + ConnectorName = <<"test_哈哈">>, + Conf0 = ?KAFKA_CONNECTOR(ConnectorName), + %% Note: must contain SSL options to trigger original bug. + Cacertfile = emqx_common_test_helpers:app_path( + emqx, + filename:join(["etc", "certs", "cacert.pem"]) + ), + Conf = Conf0#{<<"ssl">> => #{<<"cacertfile">> => Cacertfile}}, + {ok, 400, #{ + <<"code">> := <<"BAD_REQUEST">>, + <<"message">> := Msg0 + }} = request_json( + post, + uri(["connectors"]), + Conf, + Config + ), + Msg = emqx_utils_json:decode(Msg0, [return_maps]), + ?assertMatch(#{<<"kind">> := <<"validation_error">>}, Msg), + ok. + %%% helpers listen_on_random_port() -> SockOpts = [binary, {active, false}, {packet, raw}, {reuseaddr, true}, {backlog, 1000}], diff --git a/apps/emqx_resource/src/emqx_resource.erl b/apps/emqx_resource/src/emqx_resource.erl index f5bf65c0f..90df229e4 100644 --- a/apps/emqx_resource/src/emqx_resource.erl +++ b/apps/emqx_resource/src/emqx_resource.erl @@ -815,29 +815,21 @@ validate_name(<<>>, _Opts) -> invalid_data("name cannot be empty string"); validate_name(Name, _Opts) when size(Name) >= 255 -> invalid_data("name length must be less than 255"); -validate_name(Name0, Opts) -> - Name = unicode:characters_to_list(Name0, utf8), - case lists:all(fun is_id_char/1, Name) of - true -> +validate_name(Name, Opts) -> + case re:run(Name, <<"^[-0-9a-zA-Z_]+$">>, [{capture, none}]) of + match -> case maps:get(atom_name, Opts, true) of - % NOTE - % Rule may be created before bridge, thus not `list_to_existing_atom/1`, - % also it is infrequent user input anyway. - true -> list_to_atom(Name); - false -> Name0 + %% NOTE + %% Rule may be created before bridge, thus not `list_to_existing_atom/1`, + %% also it is infrequent user input anyway. + true -> binary_to_atom(Name, utf8); + false -> Name end; - false -> + nomatch -> invalid_data( - <<"only 0-9a-zA-Z_- is allowed in resource name, got: ", Name0/binary>> + <<"only 0-9a-zA-Z_- is allowed in resource name, got: ", Name/binary>> ) end. -spec invalid_data(binary()) -> no_return(). invalid_data(Reason) -> throw(#{kind => validation_error, reason => Reason}). - -is_id_char(C) when C >= $0 andalso C =< $9 -> true; -is_id_char(C) when C >= $a andalso C =< $z -> true; -is_id_char(C) when C >= $A andalso C =< $Z -> true; -is_id_char($_) -> true; -is_id_char($-) -> true; -is_id_char(_) -> false.