From 7c179f9bf4390512196b3a2b2ad6d0f4cb625c8a Mon Sep 17 00:00:00 2001
From: Madelyn Olson <34459052+madolson@users.noreply.github.com>
Date: Wed, 9 Aug 2023 23:58:53 -0700
Subject: [PATCH] Fixed a bug where sequential matching ACL rules weren't
 compressed (#12472)

When adding a new ACL rule was added, an attempt was made to remove
any "overlapping" rules. However, there when a match was found, the search
was not resumed at the right location, but instead after the original position of
the original command.

For example, if the current rules were `-config +config|get` and a rule `+config`
was added. It would identify that `-config` was matched, but it would skip over
`+config|get`, leaving the compacted rule `-config +config`. This would be evaluated
safely, but looks weird.

This bug can only be triggered with subcommands, since that is the only way to
have sequential matching rules. Resolves #12470. This is also only present in 7.2.
I think there was also a minor risk of removing another valid rule, since it would start
the search of the next command at an arbitrary point. I couldn't find a valid offset that
would have cause a match using any of the existing commands that have subcommands
with another command.
---
 src/acl.c          | 4 +++-
 tests/unit/acl.tcl | 4 ++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/acl.c b/src/acl.c
index 0bffbe970..5fd956d23 100644
--- a/src/acl.c
+++ b/src/acl.c
@@ -563,7 +563,7 @@ void ACLSelectorRemoveCommandRule(aclSelector *selector, sds new_rule) {
          * as well if the command is removed. */
         char *rule_end = strchr(existing_rule, ' ');
         if (!rule_end) {
-            /* This is the last rule, so it it to the end of the string. */
+            /* This is the last rule, so move it to the end of the string. */
             rule_end = existing_rule + strlen(existing_rule);
 
             /* This approach can leave a trailing space if the last rule is removed,
@@ -580,6 +580,8 @@ void ACLSelectorRemoveCommandRule(aclSelector *selector, sds new_rule) {
                 /* Copy the remaining rules starting at the next rule to replace the rule to be
                  * deleted, including the terminating NULL character. */
                 memmove(copy_position, copy_end, strlen(copy_end) + 1);
+                existing_rule = copy_position;
+                continue;
             }
         }
         existing_rule = copy_end;
diff --git a/tests/unit/acl.tcl b/tests/unit/acl.tcl
index 6dcee8b94..36ef06370 100644
--- a/tests/unit/acl.tcl
+++ b/tests/unit/acl.tcl
@@ -615,6 +615,10 @@ start_server {tags {"acl external:skip"}} {
         # Unnecessary categories are retained for potentional future compatibility
         r ACL SETUSER adv-test -@all -@dangerous
         assert_equal "-@all -@dangerous" [dict get [r ACL getuser adv-test] commands]
+
+        # Duplicate categories are compressed, regression test for #12470
+        r ACL SETUSER adv-test -@all +config +config|get -config|set +config
+        assert_equal "-@all +config" [dict get [r ACL getuser adv-test] commands]
     }
 
     test "ACL CAT with illegal arguments" {