From 45254d7d85c9dd2c0f75052b5216563718c73d96 Mon Sep 17 00:00:00 2001 From: Serge Tupchii Date: Thu, 20 Apr 2023 16:30:43 +0300 Subject: [PATCH] fix(emqx_bridge): validate Webhook bad URL and return 'BAD_REQUEST' if it's invalid Fixes: EMQX-9310 --- apps/emqx_bridge/src/emqx_bridge.app.src | 2 +- apps/emqx_bridge/src/emqx_bridge_api.erl | 4 +- apps/emqx_bridge/src/emqx_bridge_resource.erl | 47 ++++++++++------ .../test/emqx_bridge_api_SUITE.erl | 56 +++++++++++++++++++ changes/ce/fix-10463.en.md | 2 + 5 files changed, 93 insertions(+), 18 deletions(-) create mode 100644 changes/ce/fix-10463.en.md diff --git a/apps/emqx_bridge/src/emqx_bridge.app.src b/apps/emqx_bridge/src/emqx_bridge.app.src index 04f2b58a7..d6c140fef 100644 --- a/apps/emqx_bridge/src/emqx_bridge.app.src +++ b/apps/emqx_bridge/src/emqx_bridge.app.src @@ -1,7 +1,7 @@ %% -*- mode: erlang -*- {application, emqx_bridge, [ {description, "EMQX bridges"}, - {vsn, "0.1.16"}, + {vsn, "0.1.17"}, {registered, [emqx_bridge_sup]}, {mod, {emqx_bridge_app, []}}, {applications, [ diff --git a/apps/emqx_bridge/src/emqx_bridge_api.erl b/apps/emqx_bridge/src/emqx_bridge_api.erl index c87ea8ea6..09d1159bd 100644 --- a/apps/emqx_bridge/src/emqx_bridge_api.erl +++ b/apps/emqx_bridge/src/emqx_bridge_api.erl @@ -64,7 +64,7 @@ {BridgeType, BridgeName} -> EXPR catch - throw:{invalid_bridge_id, Reason} -> + throw:#{reason := Reason} -> ?NOT_FOUND(<<"Invalid bridge ID, ", Reason/binary>>) end ). @@ -546,6 +546,8 @@ schema("/bridges_probe") -> case emqx_bridge_resource:create_dry_run(ConnType, maps:remove(<<"type">>, Params1)) of ok -> ?NO_CONTENT; + {error, #{kind := validation_error} = Reason} -> + ?BAD_REQUEST('TEST_FAILED', map_to_json(Reason)); {error, Reason} when not is_tuple(Reason); element(1, Reason) =/= 'exit' -> ?BAD_REQUEST('TEST_FAILED', Reason) end; diff --git a/apps/emqx_bridge/src/emqx_bridge_resource.erl b/apps/emqx_bridge/src/emqx_bridge_resource.erl index 347f9d973..1ad024c40 100644 --- a/apps/emqx_bridge/src/emqx_bridge_resource.erl +++ b/apps/emqx_bridge/src/emqx_bridge_resource.erl @@ -87,7 +87,7 @@ parse_bridge_id(BridgeId) -> [Type, Name] -> {to_type_atom(Type), validate_name(Name)}; _ -> - invalid_bridge_id( + invalid_data( <<"should be of pattern {type}:{name}, but got ", BridgeId/binary>> ) end. @@ -108,14 +108,14 @@ validate_name(Name0) -> true -> Name0; false -> - invalid_bridge_id(<<"bad name: ", Name0/binary>>) + invalid_data(<<"bad name: ", Name0/binary>>) end; false -> - invalid_bridge_id(<<"only 0-9a-zA-Z_-. is allowed in name: ", Name0/binary>>) + invalid_data(<<"only 0-9a-zA-Z_-. is allowed in name: ", Name0/binary>>) end. --spec invalid_bridge_id(binary()) -> no_return(). -invalid_bridge_id(Reason) -> throw({?FUNCTION_NAME, Reason}). +-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; @@ -130,7 +130,7 @@ to_type_atom(Type) -> erlang:binary_to_existing_atom(Type, utf8) catch _:_ -> - invalid_bridge_id(<<"unknown type: ", Type/binary>>) + invalid_data(<<"unknown bridge type: ", Type/binary>>) end. reset_metrics(ResourceId) -> @@ -243,12 +243,19 @@ create_dry_run(Type, Conf0) -> {error, Reason} -> {error, Reason}; {ok, ConfNew} -> - ParseConf = parse_confs(bin(Type), TmpPath, ConfNew), - Res = emqx_resource:create_dry_run_local( - bridge_to_resource_type(Type), ParseConf - ), - _ = maybe_clear_certs(TmpPath, ConfNew), - Res + try + ParseConf = parse_confs(bin(Type), TmpPath, ConfNew), + Res = emqx_resource:create_dry_run_local( + bridge_to_resource_type(Type), ParseConf + ), + Res + catch + %% validation errors + throw:Reason -> + {error, Reason} + after + _ = maybe_clear_certs(TmpPath, ConfNew) + end end. remove(BridgeId) -> @@ -300,10 +307,18 @@ parse_confs( max_retries := Retry } = Conf ) -> - {BaseUrl, Path} = parse_url(Url), - {ok, BaseUrl2} = emqx_http_lib:uri_parse(BaseUrl), + Url1 = bin(Url), + {BaseUrl, Path} = parse_url(Url1), + BaseUrl1 = + case emqx_http_lib:uri_parse(BaseUrl) of + {ok, BUrl} -> + BUrl; + {error, Reason} -> + Reason1 = emqx_utils:readable_error_msg(Reason), + invalid_data(<<"Invalid URL: ", Url1/binary, ", details: ", Reason1/binary>>) + end, Conf#{ - base_url => BaseUrl2, + base_url => BaseUrl1, request => #{ path => Path, @@ -338,7 +353,7 @@ parse_url(Url) -> {iolist_to_binary([Scheme, "//", HostPort]), <<>>} end; [Url] -> - error({invalid_url, Url}) + invalid_data(<<"Missing scheme in URL: ", Url/binary>>) end. str(Bin) when is_binary(Bin) -> binary_to_list(Bin); diff --git a/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl b/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl index 3afe17080..d55b92138 100644 --- a/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl +++ b/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl @@ -414,6 +414,18 @@ t_http_crud_apis(Config) -> }, json(maps:get(<<"message">>, PutFail2)) ), + {ok, 400, _} = request_json( + put, + uri(["bridges", BridgeID]), + ?HTTP_BRIDGE(<<"localhost:1234/foo">>, Name), + Config + ), + {ok, 400, _} = request_json( + put, + uri(["bridges", BridgeID]), + ?HTTP_BRIDGE(<<"htpp://localhost:12341234/foo">>, Name), + Config + ), %% delete the bridge {ok, 204, <<>>} = request(delete, uri(["bridges", BridgeID]), Config), @@ -498,6 +510,22 @@ t_http_crud_apis(Config) -> %% Try create bridge with bad characters as name {ok, 400, _} = request(post, uri(["bridges"]), ?HTTP_BRIDGE(URL1, <<"隋达"/utf8>>), Config), + %% Missing scheme in URL + {ok, 400, _} = request( + post, + uri(["bridges"]), + ?HTTP_BRIDGE(<<"localhost:1234/foo">>, <<"missing_url_scheme">>), + Config + ), + + %% Invalid port + {ok, 400, _} = request( + post, + uri(["bridges"]), + ?HTTP_BRIDGE(<<"http://localhost:12341234/foo">>, <<"invalid_port">>), + Config + ), + {ok, 204, <<>>} = request(delete, uri(["bridges", BridgeID]), Config). t_http_bridges_local_topic(Config) -> @@ -1016,6 +1044,34 @@ t_bridges_probe(Config) -> ) ), + %% Missing scheme in URL + ?assertMatch( + {ok, 400, #{ + <<"code">> := <<"TEST_FAILED">>, + <<"message">> := _ + }}, + request_json( + post, + uri(["bridges_probe"]), + ?HTTP_BRIDGE(<<"203.0.113.3:1234/foo">>), + Config + ) + ), + + %% Invalid port + ?assertMatch( + {ok, 400, #{ + <<"code">> := <<"TEST_FAILED">>, + <<"message">> := _ + }}, + request_json( + post, + uri(["bridges_probe"]), + ?HTTP_BRIDGE(<<"http://203.0.113.3:12341234/foo">>), + Config + ) + ), + {ok, 204, _} = request( post, uri(["bridges_probe"]), diff --git a/changes/ce/fix-10463.en.md b/changes/ce/fix-10463.en.md new file mode 100644 index 000000000..9d57bc1b0 --- /dev/null +++ b/changes/ce/fix-10463.en.md @@ -0,0 +1,2 @@ +Improve bridges API error handling. +If Webhook bridge URL is not valid, bridges API will return '400' error instead of '500'.