fix(acl): enforce ACL on the early-intercepted command families (H-3)

[pilotspacex-byte]

Summary

An adversarial ACL review (v1.0 hardening item H-3) found that Moon's per-family connection-handler intercepts (TXN/WS/MQ/TEMPORAL/CDC.READ + pub/sub) run before the phf command registry where category ACL checks normally fire — leaving four gaps. All four are fixed here.

1. CDC.READ bypassed ACL entirely on the monoio runtime (default build)

CDC.READ was dispatched ~117 lines before the ACL gate, so any authenticated user — even +get-only — could run CDC.READ <any-wal-dir> <lsn> and read arbitrary WAL directories off the server disk. Moved the intercept to after the ACL gate (the tokio/sharded handler already ordered it correctly). This is a real, shippable difference for the default runtime-monoio build.

2. Category carve-outs silently didn't cover the families (all runtimes)

TXN/WS/MQ/TEMPORAL/CDC.READ appeared in no get_category_commands() arm, so the common deny-list idiom (+@all -@dangerous, +@all -@transaction, +@all -@write) left them allowed (empty-allowed + non-empty-denied → default-allow). Added:

• ws, cdc.read → @admin + @dangerous
• txn, temporal → @transaction
• mq, txn → @write
• all five → @all

3. -@pubsub didn't block PUBLISH/SUBSCRIBE at the command level

The pub/sub intercepts consulted only the &pattern channel rule, so a -@pubsub carve-out was ineffective for a user with &*. Added a command-level check (shared::pubsub_command_acl_deny) to PUBLISH and SUBSCRIBE/PSUBSCRIBE in the monoio and tokio-single handlers (sharded already gated them post-ACL).

4. CLIENT TRACKING ran before the ACL gate on monoio

Moved to a post-ACL try_handle_client_tracking (it registers server-side invalidation state). CLIENT ID/SETNAME/GETNAME stay pre-ACL as connection-local metadata.

Tests (red/green TDD)

• Unit src/acl/table.rs::early_intercept_families_carved_out_by_categories — pins the category expansion for all five families (deny-list + allow-list). RED before the rules.rs change.
• Integration tests/integration.rs::test_acl_denies_cdc_read_via_dangerous_carveout + test_acl_denies_pubsub_command_carveout — end-to-end NOPERM on the connection handler.
• Existing test_workspace_acl_grant already proves the sharded handler runs the ACL gate before the WS intercept (allow-list). A sharded deny-list e2e test was intentionally omitted (documented in-file): reproducing a restricted user over redis-rs multiplexed connections is unreliable because redis-rs pipelines CLIENT SETINFO, which Moon answers "unknown subcommand" for a user that holds the client permission — an orthogonal test-infra limitation.

Verification

• cargo test --lib acl:: → 67/67
• cargo test --test integration test_acl → 14/14 (incl. 2 new)
• cargo test --test workspace_integration → 13/13
• cargo fmt --check, cargo clippy -- -D warnings (default + tokio,jemalloc) → clean

Out of scope (documented, not ACL defects)

CLIENT SETINFO is unsupported (unknown-subcommand); handler_single has no TXN/WS/MQ intercepts (falls through to the generic gate).

Summary by CodeRabbit

• Security

  • Strengthened ACL enforcement for early-intercepted commands, including CDC reads, transactions, workspaces, messaging, and temporal operations.
  • Added command-level permission checks for PUBLISH, SUBSCRIBE, and PSUBSCRIBE.
  • Restricted CLIENT TRACKING and CDC reads to authorized clients.

• Tests

  • Added unit and integration coverage for dangerous-command, transaction, write, and Pub/Sub ACL restrictions.
  • Verified denied commands return appropriate permission errors without performing the requested operation.

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[coderabbitai]

Warning

Review limit reached

@TinDang97, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 39 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 71517a40-1dc3-4f5d-aefb-a8217e59ec9f

📥 Commits

Reviewing files that changed from the base of the PR and between 7c795c2 and b67d9f0.

📒 Files selected for processing (3)

• src/server/conn/handler_monoio/dispatch.rs
• src/server/conn/handler_monoio/mod.rs
• src/server/conn/shared.rs

📝 Walkthrough

Walkthrough

ACL category mappings now cover early-intercepted commands. Monoio dispatch moves CLIENT TRACKING and CDC.READ behind ACL enforcement, while pub/sub handlers add command-level checks. Unit and integration tests validate deny-list behavior.

Changes

ACL enforcement for intercepted commands

Layer / File(s)	Summary
ACL category mappings and unit coverage src/acl/rules.rs, src/acl/table.rs	Adds Moon extension commands to all, write, admin, dangerous, and transaction categories, with tests for category-based denials and allowances.
Monoio intercepted command gating src/server/conn/handler_monoio/dispatch.rs, src/server/conn/handler_monoio/mod.rs	Moves CLIENT TRACKING and CDC.READ handling after ACL enforcement while preserving tracking state registration and response handling.
Pub/sub command authorization src/server/conn/shared.rs, src/server/conn/handler_monoio/pubsub.rs, src/server/conn/handler_single.rs	Adds command-level ACL checks for PUBLISH, SUBSCRIBE, and PSUBSCRIBE before channel checks or subscriber setup.
Integration validation and release notes tests/integration.rs, tests/workspace_integration.rs, CHANGELOG.md	Adds ACL integration coverage, documents workspace test coverage constraints, and records the security fixes.
Estimated code review effort: 4 (Complex)	~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant ConnectionHandler
  participant ACLTable
  participant PubSub
  Client->>ConnectionHandler: PUBLISH or SUBSCRIBE
  ConnectionHandler->>ACLTable: check command permission
  ACLTable-->>ConnectionHandler: allow or NOPERM
  ConnectionHandler->>PubSub: publish or enter subscriber mode

Loading

Possibly related PRs

• pilotspace/moon#173: Updates related monoio handling for TXN.ABORT and TEMPORAL.INVALIDATE.
• pilotspace/moon#234: Related pub/sub ACL gating for PUBLISH and channel permission checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is detailed, but it does not follow the template because the Checklist and Performance Impact sections are missing.	Add the missing Checklist and Performance Impact sections, and include Notes if there are trade-offs or follow-up details.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly matches the main change: tightening ACL enforcement for early-intercepted command families.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/acl-early-intercept-coverage

---

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.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/server/conn/handler_monoio/mod.rs (2)

281-331: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Enforce the command-level -@pubsub gate in subscriber mode SUBSCRIBE/PSUBSCRIBE here still only check channel ACLs, so an already-subscribed session can keep adding channels after a mid-session ACL revocation. Apply the same command check used by the subscribe entry path in this branch too; the same gap exists in the handler_single.rs subscriber loop.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/conn/handler_monoio/mod.rs` around lines 281 - 331, Add the
command-level -@pubsub ACL check to the subscriber-mode SUBSCRIBE and PSUBSCRIBE
handling before processing channels, matching the existing subscribe entry-path
check and returning the appropriate denial response without subscribing. Apply
the same change to the corresponding subscriber loop in handler_single.rs, using
the relevant command-dispatch and ACL-check symbols.

---

970-980: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Persistence commands need ACL before the intercept

BGSAVE/SAVE/LASTSAVE/BGREWRITEAOF are @admin commands, so +@all -@admin users should be denied. Running try_handle_persistence() before try_enforce_acl() lets them bypass that deny-list entirely. Move the ACL check ahead of this intercept or pass the ACL context into the handler.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/conn/handler_monoio/mod.rs` around lines 970 - 980, Reorder the
dispatch flow in the monoio connection handler so try_enforce_acl runs before
try_handle_persistence, ensuring BGSAVE, SAVE, LASTSAVE, and BGREWRITEAOF honor
admin-command permissions. Preserve the existing continue behavior and keep the
ACL check ahead of all privileged intercepts, matching handler_sharded ordering.

🧹 Nitpick comments (1)

src/server/conn/handler_single.rs (1)

1069-1080: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Denied PUBLISH/SUBSCRIBE/PSUBSCRIBE aren't recorded in conn.acl_log.

The generic ACL gate a few dozen lines below (~1417-1434) logs every denial to conn.acl_log for ACL LOG visibility, but these new pub/sub command-level denials skip that step entirely, silently under-reporting exactly the kind of abuse (-@pubsub carve-out violations) this PR is meant to catch.

♻️ Proposed fix: log pub/sub command-level denials

                             if let Some(err) = crate::server::conn::shared::pubsub_command_acl_deny(
                                 &acl_table,
                                 &conn.current_user,
                                 cmd,
                                 cmd_args,
                             ) {
+                                conn.acl_log.push(crate::acl::AclLogEntry {
+                                    reason: "command".to_string(),
+                                    object: String::from_utf8_lossy(cmd).to_ascii_lowercase(),
+                                    username: conn.current_user.clone(),
+                                    client_addr: peer_addr.clone(),
+                                    timestamp_ms: std::time::SystemTime::now()
+                                        .duration_since(std::time::UNIX_EPOCH)
+                                        .unwrap_or_default()
+                                        .as_millis() as u64,
+                                });
                                 let _ = framed.send(err).await;
                                 continue;
                             }

Apply the analogous change at the PUBLISH site (~1144-1152, pushing to responses instead of sending directly).

Also applies to: 1142-1152

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/conn/handler_single.rs` around lines 1069 - 1080, Record
command-level pub/sub ACL denials in conn.acl_log before returning the error.
Update the denial branches in the pub/sub handling of the single-command
handler, including the direct framed.send path and the PUBLISH path that pushes
to responses, to create the same ACL log entry as the generic ACL gate while
preserving existing error delivery.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/server/conn/handler_monoio/mod.rs`:
- Around line 281-331: Add the command-level -@pubsub ACL check to the
subscriber-mode SUBSCRIBE and PSUBSCRIBE handling before processing channels,
matching the existing subscribe entry-path check and returning the appropriate
denial response without subscribing. Apply the same change to the corresponding
subscriber loop in handler_single.rs, using the relevant command-dispatch and
ACL-check symbols.
- Around line 970-980: Reorder the dispatch flow in the monoio connection
handler so try_enforce_acl runs before try_handle_persistence, ensuring BGSAVE,
SAVE, LASTSAVE, and BGREWRITEAOF honor admin-command permissions. Preserve the
existing continue behavior and keep the ACL check ahead of all privileged
intercepts, matching handler_sharded ordering.

---

Nitpick comments:
In `@src/server/conn/handler_single.rs`:
- Around line 1069-1080: Record command-level pub/sub ACL denials in
conn.acl_log before returning the error. Update the denial branches in the
pub/sub handling of the single-command handler, including the direct framed.send
path and the PUBLISH path that pushes to responses, to create the same ACL log
entry as the generic ACL gate while preserving existing error delivery.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 161f453c-9917-4b13-8b87-ac2c5d855e78

📥 Commits

Reviewing files that changed from the base of the PR and between 78e719b and 7c795c2.

📒 Files selected for processing (10)

• CHANGELOG.md
• src/acl/rules.rs
• src/acl/table.rs
• src/server/conn/handler_monoio/dispatch.rs
• src/server/conn/handler_monoio/mod.rs
• src/server/conn/handler_monoio/pubsub.rs
• src/server/conn/handler_single.rs
• src/server/conn/shared.rs
• tests/integration.rs
• tests/workspace_integration.rs