diff --git a/CHANGES-4.3.md b/CHANGES-4.3.md index 0ecbd5623..eecfe30ac 100644 --- a/CHANGES-4.3.md +++ b/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 diff --git a/src/emqx.appup.src b/src/emqx.appup.src index 231275541..e1f573812 100644 --- a/src/emqx.appup.src +++ b/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}, diff --git a/src/emqx_access_rule.erl b/src/emqx_access_rule.erl index dbf4c66f3..decebb6b0 100644 --- a/src/emqx_access_rule.erl +++ b/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) -> diff --git a/test/emqx_access_rule_SUITE.erl b/test/emqx_access_rule_SUITE.erl index 9259307a7..449427590 100644 --- a/test/emqx_access_rule_SUITE.erl +++ b/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">>,