acl: Escape special characters during ACL rule serialization

[murphyjacob4]

This PR fixes several critical bugs in ACL serialization and deserialization regarding command rules, selectors, and usernames containing special characters (like spaces, quotes, and parentheses).

Problem

1. Unquoted Parentheses: A selector containing a rule with a parenthesis, like (~key) +get), was serialized without quotes. On reload, ACL LOAD used a simple space-splitter (sdssplitlen) and a simple paren-merging logic (ACLMergeSelectorArguments) that got confused if a rule inside the selector contained a parenthesis (like ~key)), leading to loading failures.
2. Unescaped Command Rules: Command rules containing quotes (e.g., from module commands or deprecated first-arg rules like +get|text") were not escaped on save, leading to assertion failures or loading failures.
3. No support for quote-escaping in ACL LOAD: ACLLoadFromFile used sdssplitlen (splitting strictly by spaces), which could not handle quoted arguments, preventing simple fixes. This is different than the behavior of ACL SETUSER and prevents control planes from working around the problems above by directly editing the ACL file.

Solution

1. Root selector serialization: All ACL tokens that may contain special characters (i.e. users, command rules, key patterns, channel patterns) are checked for special characters (using sdsneedsrepr) and escaped and wrapped, if needed, during ACL LIST and ACL SAVE.
2. Non-root selectors serialization: All non-root selectors are wrapped in quotes and escaped if they contain special characters. Space counts as a special character, meaning most non-root selectors are now wrapped in quotes during serialization.
3. In memory representation: the aclSelector->command_rules suffered from the "stringly typed" anti-pattern. The in memory representation were rules separated by spaces. To properly support things like keys/command-arguments with spaces in them, it would have needed to be stored escaped in memory. To simplify this, we store it as a list of rules now, also simplifying things like rule removal.

Backwards comaptibility

• Updated ACLLoadFromFile to use a static heuristic ACLLineNeedsQuotedParser.
• If it detects a token starting with " (indicating new quoted format), it uses sdssplitargs to parse it correctly respecting quotes and escapes.
• Otherwise, it falls back to the legacy sdssplitlen to maintain backward compatibility with old unquoted files (preserving the fix Antirez made in Creating usernames with quotes can break ACL LOAD command after executing ACL SAVE redis/redis#7329).

Forwards compatibility

• ACL files are NOT forwards compatible (e.g. alldbs is added by default starting in 9.1).
• Therefore, forwards compatibility is not attempted.

Clients

• Important note: ACL LIST output and ACL file format is changing to properly escape in the ambiguous scenarios. These situations were not working as expected before, and in some cases caused crashes on serialization, so I think changing the format is reasonable for 10.0

Serialized Output Comparison

With this fix, rules and selectors containing special characters are properly quoted when saved (e.g., via ACL SAVE or ACL LIST).

Case	Command to Set	What It Should Do	Old Serialization (Broken)	New Serialization (Fixed)
Selector with Paren	ACL SETUSER tester on nopass -@all "(~key) +get)"	Grant access to GET on keys matching ~key)	user tester on ... (~key) resetchannels ... +get)	user tester on ... "(~key) resetchannels ... +get)"
Command with Quote	ACL SETUSER quoter on nopass "+get|text\""	Grant access to command get text" (first-arg rule)	=== ASSERTION FAILED ===	user quoter on ... "+get|text\""
Username with Quote	ACL SETUSER "test\"user" on nopass +@all	Create user named test"user	user test"user on ...	user "test\"user" on ...
Key with Quote	ACL SETUSER keyquoter on nopass "~key\"name"	Grant access to key key"name	user keyquoter on ... ~key"name	user keyquoter on ... "~key\"name"
Channel with Quote	ACL SETUSER chanquoter on nopass "&chan\"name"	Grant access to channel chan"name	user chanquoter on ... &chan"name	user chanquoter on ... "&chan\"name"

Tradeoffs/discussion points

1. Storing the command rules as a list will mean ACL LIST and ACL SAVE take a few more CPU cycles and involves more small memory allocations during the command execution. I still believe it is worth it since this is much less prone to bugs and will be easier to maintain. Alternatively, we can keep it a string and store the rules escaped in memory, at the cost of complexity.
2. In ACL LOAD - we do a O(n) pass to check if we should split with the quoted parser or the simple space parser. It also is a heuristic that adds some complexity. An alternative could have been to version the ACL rules in the file (e.g. user default on ver:2 ...). I chose the easier option for now, but curious what others think.
3. Escaping non-root selectors: I chose to do it for all non-root selectors, which I think is more readable. But we could limit this further by only escaping non-root selectors if some token ends in ) (this is the scenario that ACLMergeSelectorArguments fails in).

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.48%. Comparing base (f2f4e5d) to head (9adabc0).

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #3604      +/-   ##
============================================
- Coverage     76.65%   76.48%   -0.18%     
============================================
  Files           162      162              
  Lines         80612    80633      +21     
============================================
- Hits          61795    61669     -126     
- Misses        18817    18964     +147     

Files with missing lines	Coverage Δ
src/acl.c	92.56% <100.00%> (+0.03%)	⬆️

... and 20 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[madolson]

In memory representation: the aclSelector->command_rules suffered from the "stringly typed" anti-pattern. The in memory representation were rules separated by spaces. To properly support things like keys/command-arguments with spaces in them, it would have needed to be stored escaped in memory. To simplify this, we store it as a list of rules now, also simplifying things like rule removal.

The original point of this was so that we could verbatim reproduce what the end user gave us. Before this we were trying to regenerate it. I'm just trying to think if anyone would depend on this behavior now.