Merge pull request #8280 from thalesmg/acl-placeholder-nomatch-fix

fix(acl): do not leave placeholders unreplaced

CHANGES-4.3.md

@@ -23,6 +23,8 @@ File format:
 - Avoid repeated writing `loaded_plugins` file if the plugin enable stauts has not changed [#8179]
 - Correctly tally `connack.auth_error` metrics when a client uses MQTT
   3.1. [#8177]
+- Do not match ACL rules containing placeholders if there's no
+  information to fill them. [#8280]
 
 ## v4.3.15
 

src/emqx.appup.src

@@ -2,7 +2,9 @@
 %% Unless you know what you are doing, DO NOT edit manually!!
 {VSN,
   [{"4.3.16",
-    [{load_module,emqx_plugins,brutal_purge,soft_purge,[]},
+    [{load_module,emqx_app,brutal_purge,soft_purge,[]},
+     {load_module,emqx_access_rule,brutal_purge,soft_purge,[]},
+     {load_module,emqx_plugins,brutal_purge,soft_purge,[]},
      {load_module,emqx_metrics,brutal_purge,soft_purge,[]}]},
    {"4.3.15",
     [{add_module,emqx_calendar},
@@ -560,7 +562,9 @@
      {load_module,emqx_limiter,brutal_purge,soft_purge,[]}]},
    {<<".*">>,[]}],
   [{"4.3.16",
-    [{load_module,emqx_plugins,brutal_purge,soft_purge,[]},
+    [{load_module,emqx_app,brutal_purge,soft_purge,[]},
+     {load_module,emqx_access_rule,brutal_purge,soft_purge,[]},
+     {load_module,emqx_plugins,brutal_purge,soft_purge,[]},
      {load_module,emqx_metrics,brutal_purge,soft_purge,[]}]},
    {"4.3.15",
     [{delete_module,emqx_calendar},

src/emqx_access_rule.erl

@@ -133,9 +133,13 @@ match_who(_ClientInfo, _Who) ->
 match_topics(_ClientInfo, _Topic, []) ->
     false;
 match_topics(ClientInfo, Topic, [{pattern, PatternFilter}|Filters]) ->
-    TopicFilter = feed_var(ClientInfo, PatternFilter),
-    match_topic(emqx_topic:words(Topic), TopicFilter)
-        orelse match_topics(ClientInfo, Topic, Filters);
+    case feed_var(ClientInfo, PatternFilter) of
+        nomatch ->
+            false;
+        TopicFilter ->
+            match_topic(emqx_topic:words(Topic), TopicFilter)
+                orelse match_topics(ClientInfo, Topic, Filters)
+    end;
 match_topics(ClientInfo, Topic, [TopicFilter|Filters]) ->
    match_topic(emqx_topic:words(Topic), TopicFilter)
        orelse match_topics(ClientInfo, Topic, Filters).
@@ -149,12 +153,16 @@ feed_var(ClientInfo, Pattern) ->
     feed_var(ClientInfo, Pattern, []).
 feed_var(_ClientInfo, [], Acc) ->
     lists:reverse(Acc);
-feed_var(ClientInfo = #{clientid := undefined}, [<<"%c">>|Words], Acc) ->
-    feed_var(ClientInfo, Words, [<<"%c">>|Acc]);
+feed_var(#{clientid := undefined}, [<<"%c">>|_Words], _Acc) ->
+    %% we return an impossible to match value to avoid allowing a
+    %% client to pub/sub to the literal `%c' topic unintentionally.
+    nomatch;
 feed_var(ClientInfo = #{clientid := ClientId}, [<<"%c">>|Words], Acc) ->
     feed_var(ClientInfo, Words, [ClientId |Acc]);
-feed_var(ClientInfo = #{username := undefined}, [<<"%u">>|Words], Acc) ->
-    feed_var(ClientInfo, Words, [<<"%u">>|Acc]);
+feed_var(#{username := undefined}, [<<"%u">>|_Words], _Acc) ->
+    %% we return an impossible to match value to avoid allowing a
+    %% client to pub/sub to the literal `%u' topic unintentionally.
+    nomatch;
 feed_var(ClientInfo = #{username := Username}, [<<"%u">>|Words], Acc) ->
     feed_var(ClientInfo, Words, [Username|Acc]);
 feed_var(ClientInfo, [W|Words], Acc) ->

test/emqx_access_rule_SUITE.erl

@@ -57,6 +57,37 @@ t_compile(_) ->
     ?assertEqual(Compile4, emqx_access_rule:compile(Rule4)),
     ?assertEqual(Compile5, emqx_access_rule:compile(Rule5)).
 
+t_unmatching_placeholders(_Config) ->
+    EmptyClientInfo = #{ clientid => undefined
+                       , username => undefined
+                       },
+
+    Topic1 = <<"%u">>,
+    Rule1 = {allow, all, pubsub, <<"%u">>},
+    Compiled1 = emqx_access_rule:compile(Rule1),
+    ?assertEqual(
+       nomatch,
+       emqx_access_rule:match(EmptyClientInfo, Topic1, Compiled1)),
+    Rule2 = {allow, all, pubsub, [{eq, <<"%u">>}]},
+    Compiled2 = emqx_access_rule:compile(Rule2),
+    ?assertEqual(
+       {matched, allow},
+       emqx_access_rule:match(EmptyClientInfo, Topic1, Compiled2)),
+
+    Topic2 = <<"%c">>,
+    Rule3 = {allow, all, pubsub, <<"%c">>},
+    Compiled3 = emqx_access_rule:compile(Rule3),
+    ?assertEqual(
+       nomatch,
+       emqx_access_rule:match(EmptyClientInfo, Topic2, Compiled3)),
+    Rule4 = {allow, all, pubsub, [{eq, <<"%c">>}]},
+    Compiled4 = emqx_access_rule:compile(Rule4),
+    ?assertEqual(
+       {matched, allow},
+       emqx_access_rule:match(EmptyClientInfo, Topic2, Compiled4)),
+
+    ok.
+
 t_match(_) ->
     ClientInfo1 = #{zone => external,
                     clientid => <<"testClient">>,