Merge pull request #10463 from SergeTupchiy/EMQX-9310-webhook-port-validation

fix(emqx_bridge): validate Webhook bad URL

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, [

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;

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} ->
+            try
                 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
+            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);

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"]),

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'.