AOF loading must not check ACL permissions in exec

[lukepalmer]

Permissions must not be checked when replaying commands from an AOF because all that matters is that the commands were allowed at the time they were originally performed. Checking permissions can result in silent data loss.

Fixes #3983

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 866f3835-078b-4a12-8c9e-fcaba7f160f6

📥 Commits

Reviewing files that changed from the base of the PR and between e0494c3 and ee42c85.

📒 Files selected for processing (1)

• src/acl.c

---

📝 Walkthrough

Walkthrough

ACLCheckAllPerm() in src/acl.c gains a three-line early return that immediately yields ACL_OK when the client is the AOF replay client (CLIENT_ID_AOF), bypassing all permission evaluation. A new integration test in tests/integration/aof.tcl exercises this path by loading an AOF that contains a MULTI/EXEC transaction while the default user is disabled.

Changes

AOF MULTI/EXEC ACL Exemption

Layer / File(s)	Summary
ACL bypass for AOF replay clients src/acl.c	ACLCheckAllPerm() short-circuits and returns ACL_OK when c->id == CLIENT_ID_AOF, so AOF replay commands are never subjected to ACL evaluation regardless of the default user's state.
Regression test: MULTI/EXEC AOF load with disabled default user tests/integration/aof.tcl	New test configures ACLs with default disabled and a separate user granted broad permissions, writes keys inside and outside a MULTI/EXEC transaction, then reloads the AOF and asserts all writes are present.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: preventing ACL permission checks during AOF loading for EXEC commands.
Description check	✅ Passed	The description clearly explains why permissions should not be checked during AOF replay and references the related issue.
Linked Issues check	✅ Passed	The PR addresses all coding requirements from issue #3983: preventing ACL checks during AOF replay to prevent silent data loss in MULTI/EXEC transactions.
Out of Scope Changes check	✅ Passed	All changes are scoped to the stated objective: modifying ACL checking logic for AOF replay and adding a test to verify the fix.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.70%. Comparing base (86acf8d) to head (ee42c85).
⚠️ Report is 5 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #3984      +/-   ##
============================================
+ Coverage     76.54%   76.70%   +0.15%     
============================================
  Files           162      162              
  Lines         80775    80789      +14     
============================================
+ Hits          61833    61970     +137     
+ Misses        18942    18819     -123     

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

... and 22 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.

[dvkashapov]

Overall I agree with proposal here but this change may require major decision from core team because this behaviour is undocumented and may change something that users rely on...