From 22838f027ae2435ca27163fd6f84bb69a2aaf9d4 Mon Sep 17 00:00:00 2001 From: zmstone Date: Fri, 22 Mar 2024 20:57:36 +0100 Subject: [PATCH] fix: mountpoint template render should not replace unknown as undefined For backward compatibility, the unknown vars used in mountpoint is kept unchanged. e.g. '${unknown}/foo/bar' should be rendered as '${unknown}/foo/bar' but not 'undefined/foo/bar' --- apps/emqx/include/emqx_placeholder.hrl | 6 ++- apps/emqx/src/emqx_mountpoint.erl | 42 +++++++++++++++---- apps/emqx/test/emqx_mountpoint_SUITE.erl | 28 +++++++++++++ .../src/emqx_authz/emqx_authz_utils.erl | 16 +------ apps/emqx_utils/src/emqx_template.erl | 23 ++++++++++ 5 files changed, 90 insertions(+), 25 deletions(-) diff --git a/apps/emqx/include/emqx_placeholder.hrl b/apps/emqx/include/emqx_placeholder.hrl index e2711be69..31ce6a070 100644 --- a/apps/emqx/include/emqx_placeholder.hrl +++ b/apps/emqx/include/emqx_placeholder.hrl @@ -31,12 +31,14 @@ -define(PH_CERT_SUBJECT, ?PH(?VAR_CERT_SUBJECT)). -define(PH_CERT_CN_NAME, ?PH(?VAR_CERT_CN_NAME)). -%% MQTT +%% MQTT/Gateway -define(VAR_PASSWORD, "password"). -define(VAR_CLIENTID, "clientid"). -define(VAR_USERNAME, "username"). -define(VAR_TOPIC, "topic"). +-define(VAR_ENDPOINT_NAME, "endpoint_name"). -define(VAR_NS_CLIENT_ATTRS, {var_namespace, "client_attrs"}). + -define(PH_PASSWORD, ?PH(?VAR_PASSWORD)). -define(PH_CLIENTID, ?PH(?VAR_CLIENTID)). -define(PH_FROM_CLIENTID, ?PH("from_clientid")). @@ -90,7 +92,7 @@ -define(PH_NODE, ?PH("node")). -define(PH_REASON, ?PH("reason")). --define(PH_ENDPOINT_NAME, ?PH("endpoint_name")). +-define(PH_ENDPOINT_NAME, ?PH(?VAR_ENDPOINT_NAME)). -define(VAR_RETAIN, "retain"). -define(PH_RETAIN, ?PH(?VAR_RETAIN)). diff --git a/apps/emqx/src/emqx_mountpoint.erl b/apps/emqx/src/emqx_mountpoint.erl index c9ed9efc5..9d2790b9e 100644 --- a/apps/emqx/src/emqx_mountpoint.erl +++ b/apps/emqx/src/emqx_mountpoint.erl @@ -28,10 +28,19 @@ -export([replvar/2]). +-export([lookup/2]). + -export_type([mountpoint/0]). -type mountpoint() :: binary(). +-define(ALLOWED_VARS, [ + ?VAR_CLIENTID, + ?VAR_USERNAME, + ?VAR_ENDPOINT_NAME, + ?VAR_NS_CLIENT_ATTRS +]). + -spec mount(option(mountpoint()), Any) -> Any when Any :: emqx_types:topic() @@ -87,12 +96,29 @@ unmount_maybe_share(MountPoint, TopicFilter = #share{topic = Topic}) when -spec replvar(option(mountpoint()), map()) -> option(mountpoint()). replvar(undefined, _Vars) -> undefined; -replvar(MountPoint, Vars0) -> - Allowed = [clientid, username, endpoint_name, client_attrs], - Vars = maps:filter( - fun(K, V) -> V =/= undefined andalso lists:member(K, Allowed) end, - Vars0 - ), - Template = emqx_template:parse(MountPoint), - {String, _Errors} = emqx_template:render(Template, Vars), +replvar(MountPoint, Vars) -> + Template = parse(MountPoint), + {String, _Errors} = emqx_template:render(Template, {?MODULE, Vars}), unicode:characters_to_binary(String). + +lookup([<>], #{clientid := ClientId}) when is_binary(ClientId) -> + {ok, ClientId}; +lookup([<>], #{username := Username}) when is_binary(Username) -> + {ok, Username}; +lookup([<>], #{endpoint_name := Name}) when is_binary(Name) -> + {ok, Name}; +lookup([<<"client_attrs">>, AttrName], #{client_attrs := Attrs}) when is_map(Attrs) -> + Original = iolist_to_binary(["${client_attrs.", AttrName, "}"]), + {ok, maps:get(AttrName, Attrs, Original)}; +lookup(Accessor, _) -> + {ok, iolist_to_binary(["${", lists:join(".", Accessor), "}"])}. + +parse(Template) -> + Parsed = emqx_template:parse(Template), + case emqx_template:validate(?ALLOWED_VARS, Parsed) of + ok -> + Parsed; + {error, _Disallowed} -> + Escaped = emqx_template:escape_disallowed(Parsed, ?ALLOWED_VARS), + emqx_template:parse(Escaped) + end. diff --git a/apps/emqx/test/emqx_mountpoint_SUITE.erl b/apps/emqx/test/emqx_mountpoint_SUITE.erl index 06913a35c..756bfeb59 100644 --- a/apps/emqx/test/emqx_mountpoint_SUITE.erl +++ b/apps/emqx/test/emqx_mountpoint_SUITE.erl @@ -116,4 +116,32 @@ t_replvar(_) -> username => undefined } ) + ), + ?assertEqual( + <<"mount/g1/clientid/">>, + replvar( + <<"mount/${client_attrs.group}/${clientid}/">>, + #{ + clientid => <<"clientid">>, + client_attrs => #{<<"group">> => <<"g1">>} + } + ) + ), + ?assertEqual( + <<"mount/${client_attrs.group}/clientid/">>, + replvar( + <<"mount/${client_attrs.group}/${clientid}/">>, + #{ + clientid => <<"clientid">> + } + ) + ), + ?assertEqual( + <<"mount/${not.allowed}/clientid/">>, + replvar( + <<"mount/${not.allowed}/${clientid}/">>, + #{ + clientid => <<"clientid">> + } + ) ). diff --git a/apps/emqx_auth/src/emqx_authz/emqx_authz_utils.erl b/apps/emqx_auth/src/emqx_authz/emqx_authz_utils.erl index 9a795d434..26b8d000f 100644 --- a/apps/emqx_auth/src/emqx_authz/emqx_authz_utils.erl +++ b/apps/emqx_auth/src/emqx_authz/emqx_authz_utils.erl @@ -139,7 +139,7 @@ handle_disallowed_placeholders(Template, Source, Allowed) -> " However, consider using `${$}` escaping for literal `$` where" " needed to avoid unexpected results." }), - Result = prerender_disallowed_placeholders(Template, Allowed), + Result = emqx_template:escape_disallowed(Template, Allowed), case Source of {string, _} -> emqx_template:parse(Result); @@ -148,20 +148,6 @@ handle_disallowed_placeholders(Template, Source, Allowed) -> end end. -prerender_disallowed_placeholders(Template, Allowed) -> - {Result, _} = emqx_template:render(Template, #{}, #{ - var_trans => fun(Name, _) -> - % NOTE - % Rendering disallowed placeholders in escaped form, which will then - % parse as a literal string. - case lists:member(Name, Allowed) of - true -> "${" ++ Name ++ "}"; - false -> "${$}{" ++ Name ++ "}" - end - end - }), - Result. - render_deep(Template, Values) -> % NOTE % Ignoring errors here, undefined bindings will be replaced with empty string. diff --git a/apps/emqx_utils/src/emqx_template.erl b/apps/emqx_utils/src/emqx_template.erl index b0272b1bb..1383f90e1 100644 --- a/apps/emqx_utils/src/emqx_template.erl +++ b/apps/emqx_utils/src/emqx_template.erl @@ -32,6 +32,7 @@ -export([lookup/2]). -export([to_string/1]). +-export([escape_disallowed/2]). -export_type([t/0]). -export_type([str/0]). @@ -157,9 +158,31 @@ validate(Allowed, Template) -> {error, [{Var, disallowed} || Var <- Disallowed]} end. +%% @doc Escape `$' with `${$}' for the variable references +%% which are not allowed, so the original variable name +%% can be preserved instead of rendered as `undefined'. +%% E.g. to render `${var1}/${clientid}', if only `clientid' +%% is allowed, the rendering result should be `${var1}/client1' +%% but not `undefined/client1'. +escape_disallowed(Template, Allowed) -> + {Result, _} = render(Template, #{}, #{ + var_trans => fun(Name, _) -> + case is_allowed(Name, Allowed) of + true -> "${" ++ Name ++ "}"; + false -> "${$}{" ++ Name ++ "}" + end + end + }), + Result. + find_disallowed(Vars, Allowed) -> lists:filter(fun(Var) -> not is_allowed(Var, Allowed) end, Vars). +%% @private Return 'true' if a variable reference matches +%% at least one allowed variables. +%% For `"${var_name}"' kind of reference, its a `=:=' compare +%% for `{var_namespace, "namespace"}' kind of reference +%% it matches the `"namespace."' prefix. is_allowed(_Var, []) -> false; is_allowed(Var, [{var_namespace, VarPrefix} | Allowed]) ->