fix(auth): fix uri path handling

Fix uri path handling `emqx_connector_http`,
 HTTP authentication and authorization backends.

apps/emqx_authn/src/emqx_authn_utils.erl

@@ -28,6 +28,7 @@
     parse_sql/2,
     render_deep/2,
     render_str/2,
+    render_urlencoded_str/2,
     render_sql_params/2,
     is_superuser/1,
     bin/1,
@@ -129,6 +130,13 @@ render_str(Template, Credential) ->
         #{return => full_binary, var_trans => fun handle_var/2}
     ).
 
+render_urlencoded_str(Template, Credential) ->
+    emqx_placeholder:proc_tmpl(
+        Template,
+        mapping_credential(Credential),
+        #{return => full_binary, var_trans => fun urlencode_var/2}
+    ).
+
 render_sql_params(ParamList, Credential) ->
     emqx_placeholder:proc_tmpl(
         ParamList,
@@ -217,6 +225,11 @@ without_password(Credential, [Name | Rest]) ->
             without_password(Credential, Rest)
     end.
 
+urlencode_var({var, _} = Var, Value) ->
+    emqx_http_lib:uri_encode(handle_var(Var, Value));
+urlencode_var(Var, Value) ->
+    handle_var(Var, Value).
+
 handle_var({var, _Name}, undefined) ->
     <<>>;
 handle_var({var, <<"peerhost">>}, PeerHost) ->

apps/emqx_authn/src/simple_authn/emqx_authn_http.erl

@@ -285,9 +285,9 @@ parse_url(Url) ->
                     BaseUrl = iolist_to_binary([Scheme, "//", HostPort]),
                     case string:split(Remaining, "?", leading) of
                         [Path, QueryString] ->
-                            {BaseUrl, Path, QueryString};
+                            {BaseUrl, <<"/", Path/binary>>, QueryString};
                         [Path] ->
-                            {BaseUrl, Path, <<>>}
+                            {BaseUrl, <<"/", Path/binary>>, <<>>}
                     end;
                 [HostPort] ->
                     {iolist_to_binary([Scheme, "//", HostPort]), <<>>, <<>>}
@@ -328,7 +328,7 @@ generate_request(Credential, #{
     body_template := BodyTemplate
 }) ->
     Headers = maps:to_list(Headers0),
-    Path = emqx_authn_utils:render_str(BasePathTemplate, Credential),
+    Path = emqx_authn_utils:render_urlencoded_str(BasePathTemplate, Credential),
     Query = emqx_authn_utils:render_deep(BaseQueryTemplate, Credential),
     Body = emqx_authn_utils:render_deep(BodyTemplate, Credential),
     case Method of
@@ -343,9 +343,9 @@ generate_request(Credential, #{
     end.
 
 append_query(Path, []) ->
-    encode_path(Path);
+    Path;
 append_query(Path, Query) ->
-    encode_path(Path) ++ "?" ++ binary_to_list(qs(Query)).
+    Path ++ "?" ++ binary_to_list(qs(Query)).
 
 qs(KVs) ->
     qs(KVs, []).
@@ -407,10 +407,6 @@ parse_body(ContentType, _) ->
 uri_encode(T) ->
     emqx_http_lib:uri_encode(to_list(T)).
 
-encode_path(Path) ->
-    Parts = string:split(Path, "/", all),
-    lists:flatten(["/" ++ Part || Part <- lists:map(fun uri_encode/1, Parts)]).
-
 request_for_log(Credential, #{url := Url} = State) ->
     SafeCredential = emqx_authn_utils:without_password(Credential),
     case generate_request(SafeCredential, State) of

apps/emqx_authn/test/emqx_authn_http_SUITE.erl

@@ -47,7 +47,6 @@
     })
 ).
 
--define(SERVER_RESPONSE_URLENCODE(Result), ?SERVER_RESPONSE_URLENCODE(Result, false)).
 -define(SERVER_RESPONSE_URLENCODE(Result, IsSuperuser),
     list_to_binary(
         "result=" ++
@@ -166,6 +165,54 @@ test_user_auth(#{
         ?GLOBAL
     ).
 
+t_authenticate_path_placeholders(_Config) ->
+    ok = emqx_authn_http_test_server:stop(),
+    {ok, _} = emqx_authn_http_test_server:start_link(?HTTP_PORT, <<"/[...]">>),
+    ok = emqx_authn_http_test_server:set_handler(
+        fun(Req0, State) ->
+            Req =
+                case cowboy_req:path(Req0) of
+                    <<"/my/p%20ath//us%20er/auth//">> ->
+                        cowboy_req:reply(
+                            200,
+                            #{<<"content-type">> => <<"application/json">>},
+                            emqx_utils_json:encode(#{result => allow, is_superuser => false}),
+                            Req0
+                        );
+                    Path ->
+                        ct:pal("Unexpected path: ~p", [Path]),
+                        cowboy_req:reply(403, Req0)
+                end,
+            {ok, Req, State}
+        end
+    ),
+
+    Credentials = ?CREDENTIALS#{
+        username => <<"us er">>
+    },
+
+    AuthConfig = maps:merge(
+        raw_http_auth_config(),
+        #{
+            <<"url">> => <<"http://127.0.0.1:32333/my/p%20ath//${username}/auth//">>,
+            <<"body">> => #{}
+        }
+    ),
+    {ok, _} = emqx:update_config(
+        ?PATH,
+        {create_authenticator, ?GLOBAL, AuthConfig}
+    ),
+
+    ?assertMatch(
+        {ok, #{is_superuser := false}},
+        emqx_access_control:authenticate(Credentials)
+    ),
+
+    _ = emqx_authn_test_lib:delete_authenticators(
+        [authentication],
+        ?GLOBAL
+    ).
+
 t_no_value_for_placeholder(_Config) ->
     Handler = fun(Req0, State) ->
         {ok, RawBody, Req1} = cowboy_req:read_body(Req0),

apps/emqx_authz/src/emqx_authz_http.erl

@@ -161,9 +161,9 @@ parse_url(Url) ->
                     BaseUrl = iolist_to_binary([Scheme, "//", HostPort]),
                     case string:split(Remaining, "?", leading) of
                         [Path, QueryString] ->
-                            {BaseUrl, Path, QueryString};
+                            {BaseUrl, <<"/", Path/binary>>, QueryString};
                         [Path] ->
-                            {BaseUrl, Path, <<>>}
+                            {BaseUrl, <<"/", Path/binary>>, <<>>}
                     end;
                 [HostPort] ->
                     {iolist_to_binary([Scheme, "//", HostPort]), <<>>, <<>>}
@@ -185,7 +185,7 @@ generate_request(
     }
 ) ->
     Values = client_vars(Client, PubSub, Topic),
-    Path = emqx_authz_utils:render_str(BasePathTemplate, Values),
+    Path = emqx_authz_utils:render_urlencoded_str(BasePathTemplate, Values),
     Query = emqx_authz_utils:render_deep(BaseQueryTemplate, Values),
     Body = emqx_authz_utils:render_deep(BodyTemplate, Values),
     case Method of
@@ -202,9 +202,9 @@ generate_request(
     end.
 
 append_query(Path, []) ->
-    encode_path(Path);
+    to_list(Path);
 append_query(Path, Query) ->
-    encode_path(Path) ++ "?" ++ to_list(query_string(Query)).
+    to_list(Path) ++ "?" ++ to_list(query_string(Query)).
 
 query_string(Body) ->
     query_string(Body, []).
@@ -222,10 +222,6 @@ query_string([{K, V} | More], Acc) ->
 uri_encode(T) ->
     emqx_http_lib:uri_encode(to_list(T)).
 
-encode_path(Path) ->
-    Parts = string:split(Path, "/", all),
-    lists:flatten(["/" ++ Part || Part <- lists:map(fun uri_encode/1, Parts)]).
-
 serialize_body(<<"application/json">>, Body) ->
     emqx_utils_json:encode(Body);
 serialize_body(<<"application/x-www-form-urlencoded">>, Body) ->

apps/emqx_authz/src/emqx_authz_utils.erl

@@ -16,7 +16,6 @@
 
 -module(emqx_authz_utils).
 
--include_lib("emqx/include/emqx_placeholder.hrl").
 -include_lib("emqx_authz.hrl").
 
 -export([
@@ -28,6 +27,7 @@
     update_config/2,
     parse_deep/2,
     parse_str/2,
+    render_urlencoded_str/2,
     parse_sql/3,
     render_deep/2,
     render_str/2,
@@ -128,6 +128,13 @@ render_str(Template, Values) ->
         #{return => full_binary, var_trans => fun handle_var/2}
     ).
 
+render_urlencoded_str(Template, Values) ->
+    emqx_placeholder:proc_tmpl(
+        Template,
+        client_vars(Values),
+        #{return => full_binary, var_trans => fun urlencode_var/2}
+    ).
+
 render_sql_params(ParamList, Values) ->
     emqx_placeholder:proc_tmpl(
         ParamList,
@@ -181,6 +188,11 @@ convert_client_var({dn, DN}) -> {cert_subject, DN};
 convert_client_var({protocol, Proto}) -> {proto_name, Proto};
 convert_client_var(Other) -> Other.
 
+urlencode_var({var, _} = Var, Value) ->
+    emqx_http_lib:uri_encode(handle_var(Var, Value));
+urlencode_var(Var, Value) ->
+    handle_var(Var, Value).
+
 handle_var({var, _Name}, undefined) ->
     <<>>;
 handle_var({var, <<"peerhost">>}, IpAddr) ->

apps/emqx_authz/test/emqx_authz_http_SUITE.erl

@@ -199,7 +199,7 @@ t_query_params(_Config) ->
                 peerhost := <<"127.0.0.1">>,
                 proto_name := <<"MQTT">>,
                 mountpoint := <<"MOUNTPOINT">>,
-                topic := <<"t">>,
+                topic := <<"t/1">>,
                 action := <<"publish">>
             } = cowboy_req:match_qs(
                 [
@@ -241,7 +241,7 @@ t_query_params(_Config) ->
 
     ?assertEqual(
         allow,
-        emqx_access_control:authorize(ClientInfo, publish, <<"t">>)
+        emqx_access_control:authorize(ClientInfo, publish, <<"t/1">>)
     ).
 
 t_path(_Config) ->
@@ -249,13 +249,13 @@ t_path(_Config) ->
         fun(Req0, State) ->
             ?assertEqual(
                 <<
-                    "/authz/users/"
+                    "/authz/use%20rs/"
                     "user%20name/"
                     "client%20id/"
                     "127.0.0.1/"
                     "MQTT/"
                     "MOUNTPOINT/"
-                    "t/1/"
+                    "t%2F1/"
                     "publish"
                 >>,
                 cowboy_req:path(Req0)
@@ -264,7 +264,7 @@ t_path(_Config) ->
         end,
         #{
             <<"url">> => <<
-                "http://127.0.0.1:33333/authz/users/"
+                "http://127.0.0.1:33333/authz/use%20rs/"
                 "${username}/"
                 "${clientid}/"
                 "${peerhost}/"

apps/emqx_connector/src/emqx_connector_http.erl

@@ -47,7 +47,7 @@
     namespace/0
 ]).
 
--export([check_ssl_opts/2, validate_method/1]).
+-export([check_ssl_opts/2, validate_method/1, join_paths/2]).
 
 -type connect_timeout() :: emqx_schema:duration() | infinity.
 -type pool_type() :: random | hash.
@@ -458,7 +458,7 @@ preprocess_request(
     } = Req
 ) ->
     #{
-        method => emqx_plugin_libs_rule:preproc_tmpl(bin(Method)),
+        method => emqx_plugin_libs_rule:preproc_tmpl(to_bin(Method)),
         path => emqx_plugin_libs_rule:preproc_tmpl(Path),
         body => maybe_preproc_tmpl(body, Req),
         headers => preproc_headers(Headers),
@@ -471,8 +471,8 @@ preproc_headers(Headers) when is_map(Headers) ->
         fun(K, V, Acc) ->
             [
                 {
-                    emqx_plugin_libs_rule:preproc_tmpl(bin(K)),
+                    emqx_plugin_libs_rule:preproc_tmpl(to_bin(K)),
-                    emqx_plugin_libs_rule:preproc_tmpl(bin(V))
+                    emqx_plugin_libs_rule:preproc_tmpl(to_bin(V))
                 }
                 | Acc
             ]
@@ -484,8 +484,8 @@ preproc_headers(Headers) when is_list(Headers) ->
     lists:map(
         fun({K, V}) ->
             {
-                emqx_plugin_libs_rule:preproc_tmpl(bin(K)),
+                emqx_plugin_libs_rule:preproc_tmpl(to_bin(K)),
-                emqx_plugin_libs_rule:preproc_tmpl(bin(V))
+                emqx_plugin_libs_rule:preproc_tmpl(to_bin(V))
             }
         end,
         Headers
@@ -553,15 +553,41 @@ formalize_request(Method, BasePath, {Path, Headers, _Body}) when
 ->
     formalize_request(Method, BasePath, {Path, Headers});
 formalize_request(_Method, BasePath, {Path, Headers, Body}) ->
-    {filename:join(BasePath, Path), Headers, Body};
+    {join_paths(BasePath, Path), Headers, Body};
 formalize_request(_Method, BasePath, {Path, Headers}) ->
-    {filename:join(BasePath, Path), Headers}.
+    {join_paths(BasePath, Path), Headers}.
 
-bin(Bin) when is_binary(Bin) ->
+%% By default, we cannot treat HTTP paths as "file" or "resource" paths,
+%% because an HTTP server may handle paths like
+%% "/a/b/c/", "/a/b/c" and "/a//b/c" differently.
+%%
+%% So we try to avoid unneccessary path normalization.
+%%
+%% See also: `join_paths_test_/0`
+join_paths(Path1, Path2) ->
+    do_join_paths(lists:reverse(to_list(Path1)), to_list(Path2)).
+
+%% "abc/" + "/cde"
+do_join_paths([$/ | Path1], [$/ | Path2]) ->
+    lists:reverse(Path1) ++ [$/ | Path2];
+%% "abc/" + "cde"
+do_join_paths([$/ | Path1], Path2) ->
+    lists:reverse(Path1) ++ [$/ | Path2];
+%% "abc" + "/cde"
+do_join_paths(Path1, [$/ | Path2]) ->
+    lists:reverse(Path1) ++ [$/ | Path2];
+%% "abc" + "cde"
+do_join_paths(Path1, Path2) ->
+    lists:reverse(Path1) ++ [$/ | Path2].
+
+to_list(List) when is_list(List) -> List;
+to_list(Bin) when is_binary(Bin) -> binary_to_list(Bin).
+
+to_bin(Bin) when is_binary(Bin) ->
     Bin;
-bin(Str) when is_list(Str) ->
+to_bin(Str) when is_list(Str) ->
     list_to_binary(Str);
-bin(Atom) when is_atom(Atom) ->
+to_bin(Atom) when is_atom(Atom) ->
     atom_to_binary(Atom, utf8).
 
 reply_delegator(ReplyFunAndArgs, Result) ->
@@ -642,4 +668,21 @@ redact_test_() ->
         ?_assertNotEqual(TestData2, redact(TestData2))
     ].
 
+join_paths_test_() ->
+    [
+        ?_assertEqual("abc/cde", join_paths("abc", "cde")),
+        ?_assertEqual("abc/cde", join_paths("abc", "/cde")),
+        ?_assertEqual("abc/cde", join_paths("abc/", "cde")),
+        ?_assertEqual("abc/cde", join_paths("abc/", "/cde")),
+
+        ?_assertEqual("/", join_paths("", "")),
+        ?_assertEqual("/cde", join_paths("", "cde")),
+        ?_assertEqual("/cde", join_paths("", "/cde")),
+        ?_assertEqual("/cde", join_paths("/", "cde")),
+        ?_assertEqual("/cde", join_paths("/", "/cde")),
+
+        ?_assertEqual("//cde/", join_paths("/", "//cde/")),
+        ?_assertEqual("abc///cde/", join_paths("abc//", "//cde/"))
+    ].
+
 -endif.

changes/ce/fix-10420.en.md

@@ -0,0 +1,3 @@
+Fix HTTP path handling when composing the URL for the HTTP requests in authentication and authorization modules.
+* Avoid unnecessary URL normalization since we cannot assume that external servers treat original and normalized URLs equally. This led to bugs like [#10411](https://github.com/emqx/emqx/issues/10411).
+* Fix the issue that path segments could be HTTP encoded twice.