fix(security): ACL bypass + silent CLIENT TRACKING failure on the inline GET path

CHANGELOG.md

@@ -6,6 +6,70 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Security
+- **ACL bypass on the inline GET fast path (monoio runtime).** An
+  authenticated but restricted user could read any key with plain `GET`:
+  `try_inline_dispatch` answered the `*2 $3 GET` shape straight from the shard
+  map, running neither the ACL command check nor the ACL key-pattern check.
+  A `-@all` user read arbitrary keys by name, and a `~app:*` user read outside
+  its pattern; no ACL LOG entry was produced. Writes were already gated on
+  `can_inline_writes` (which folds in `conn.acl_skip_allowed()`) — reads were
+  gated on nothing. Not single-shard-only: at `--shards 4` every key hashing
+  to the connection's own shard leaked (measured 24/160 and 44/160 in the new
+  suite); `--shards 1` — the config recommended for non-pipelined workloads —
+  leaked 160/160. Reads are now gated on the same `acl_skip_allowed()` latch,
+  so a restricted connection falls through to generic dispatch where both ACL
+  checks run. The tokio handlers were never affected (they gate correctly at
+  `handler_single.rs` / `handler_sharded/mod.rs`), which is why no CI job
+  caught this: every CI test job builds tokio. New suite:
+  `tests/acl_inline_read_enforcement.rs` (deny-all and key-pattern users, at
+  `--shards 1` and `--shards 4`).
+
+### Fixed
+- **`GET` inside `MULTI` was executed instead of queued (monoio).** Third
+  defect from the same ungated inline read path, and the one most visible to a
+  working client: `MULTI; GET k; EXEC` answered `+OK`, `$1 v`, `*0` where Redis
+  answers `+OK`, `+QUEUED`, `*1[$1 v]`. The client receives a value where it
+  expects `+QUEUED`, and then an `EXEC` that silently omits the read — a
+  redis-py/go-redis transaction returns an empty result set for an exchange it
+  believes succeeded. `can_inline_writes` already carried `!conn.in_multi`
+  (so `SET` queued correctly); `can_inline_reads` now carries it too. `MGET`,
+  not being inline-eligible, queued correctly throughout, which is what
+  isolated the path. Found by the new `scripts/test-client-compat.sh` raw-RESP
+  harness on its first run against a real redis-server. New suite:
+  `tests/multi_queues_inline_get.rs`, including controls pinning that `MGET`
+  and `SET` still queue and that a plain `GET` outside a transaction still
+  takes the fast path (measured: 2000/2000 GETs still `local_inline`, before
+  and after a completed transaction).
+- **`CLIENT TRACKING` answered `+OK` and then never invalidated (monoio).**
+  Same root cause: the inline GET path also skips
+  `tracking::invalidation::track_read_keys`, so a client-side-caching client's
+  own `GET`s were never registered and nothing ever invalidated them — the
+  cache served stale data indefinitely. At `--shards 1` this was total; at
+  `--shards 4` it depended on whether the key hashed to the reader's own
+  shard, which also made the existing `mset_invalidates_every_second_arg_key`
+  test flaky (observed failing 2 of 3 runs on 0.8.5). Reads from a connection
+  with tracking enabled now take the generic path. Deliberately gated on this
+  connection's own `tracking_state.enabled`, not the process-global
+  `tracking_active()`: only a connection's own reads populate its invalidation
+  set, so one caching client must not push every other connection off the fast
+  path.
+- **`CLIENT TRACKING ON BCAST` with no `PREFIX` never invalidated anything.**
+  Redis treats prefix-less BCAST as "invalidate me for every key", but the
+  handlers only registered broadcast interest inside
+  `for prefix in &config_parsed.prefixes`, so a prefix-less BCAST client
+  registered nothing — at any shard count, and regardless of what it read
+  (BCAST does not depend on reads). `parse_tracking_args` now normalises
+  prefix-less BCAST to the empty prefix, which `TrackingTable`'s
+  `key.starts_with(prefix)` match treats as "all keys"; one change fixes all
+  three handlers.
+
+  The default (unrestricted, non-tracking) connection keeps the inline fast
+  path byte-for-byte, verified against
+  `moon_dispatch_path_total{path="local_inline"}`: 2000/2000 GETs still
+  inlined with and without `--requirepass`, 0/2000 for restricted and
+  tracking connections.
+
 ## [0.8.5] — 2026-08-08
 
 ### Added

src/command/client.rs

@@ -99,6 +99,20 @@ pub fn parse_tracking_args(args: &[Frame]) -> Result<TrackingConfig, Frame> {
         )));
     }
 
+    // `BCAST` with no `PREFIX` means "invalidate me for EVERY key" in Redis.
+    // The handlers register broadcast interest with
+    // `TrackingTable::register_prefix` inside `for prefix in &prefixes`, so a
+    // prefix-less BCAST client used to register nothing and then never
+    // received an invalidation — at any shard count, and regardless of what it
+    // read (BCAST does not depend on reads at all). `TrackingTable` matches
+    // with `key.starts_with(prefix)`, for which the empty prefix is exactly
+    // "all keys". Normalising here fixes all three handlers at once
+    // (monoio/dispatch.rs, handler_single.rs, handler_sharded/dispatch.rs)
+    // and keeps the semantics in one place.
+    if bcast && prefixes.is_empty() {
+        prefixes.push(Bytes::new());
+    }
+
     Ok(TrackingConfig {
         enable,
         bcast,

src/server/conn/blocking.rs

@@ -1621,6 +1621,7 @@ pub(crate) fn try_inline_dispatch(
     >,
     now_ms: u64,
     num_shards: usize,
+    can_inline_reads: bool,
     can_inline_writes: bool,
     runtime_config: &parking_lot::RwLock<crate::config::RuntimeConfig>,
 ) -> usize {
@@ -1651,7 +1652,15 @@ pub(crate) fn try_inline_dispatch(
     if buf[2] != b'\r' || buf[3] != b'\n' {
         return 0;
     }
-    let is_get = argc == b'2';
+    // `can_inline_reads` is NOT optional bookkeeping: this path answers GET
+    // without entering generic dispatch, so it runs neither the ACL
+    // command/key check nor `tracking::invalidation::track_read_keys`.
+    // Ungated it was (a) an ACL bypass — a `-@all` user read any key by name
+    // — and (b) a silent client-side-caching failure: a tracking client's own
+    // GETs were never registered, so nothing ever invalidated them.
+    // See `tests/acl_inline_read_enforcement.rs` and the inline-GET cases in
+    // `tests/client_tracking_invalidation.rs`.
+    let is_get = argc == b'2' && can_inline_reads;
     let is_set = argc == b'3' && can_inline_writes;
     if !is_get && !is_set {
         return 0;
@@ -1997,6 +2006,7 @@ pub(crate) fn try_inline_dispatch_loop(
     >,
     now_ms: u64,
     num_shards: usize,
+    can_inline_reads: bool,
     can_inline_writes: bool,
     cluster_enabled: bool,
     runtime_config: &parking_lot::RwLock<crate::config::RuntimeConfig>,
@@ -2016,6 +2026,7 @@ pub(crate) fn try_inline_dispatch_loop(
             repl_state,
             now_ms,
             num_shards,
+            can_inline_reads,
             can_inline_writes,
             runtime_config,
         );

src/server/conn/handler_monoio/mod.rs

@@ -1123,10 +1123,39 @@ pub(crate) async fn handle_connection_sharded_monoio<
             // for the rest of the process once any replica has ever begun
             // attaching, matching the existing `ctx.spill_sender.is_none()`
             // precedent of "fall back to the full path when it must do more
-            // than this fast path knows how to do". GET inlining is
-            // unaffected (`is_get` in `try_inline_dispatch` doesn't check
-            // `can_inline_writes`).
-            let can_inline_writes = conn.acl_skip_allowed()
+            // than this fast path knows how to do". These write-only
+            // conditions do not gate GET, which has its own `can_inline_reads`
+            // gate below — reads must never be inlined for a restricted or
+            // tracking connection, but a replica/spill/fanout master may still
+            // serve them from the fast path.
+            //
+            // Hoisted: both gates need it, and it is one Acquire load plus a
+            // compare (`cached_acl_unrestricted && acl_cache_fresh()`), so
+            // computing it once keeps the read gate free relative to the
+            // pre-fix code, which already paid for it on the write gate.
+            let acl_unrestricted = conn.acl_skip_allowed();
+            // Reads may be inlined only when this connection can provably skip
+            // the ACL check AND is not itself a client-side-caching client:
+            // the inline path calls neither the ACL gate nor `track_read_keys`.
+            // A restricted user, or a `CLIENT TRACKING ON` connection, falls
+            // back to generic dispatch where both run. Deliberately NOT gated
+            // on the process-global `tracking_active()` — only THIS
+            // connection's own reads populate its invalidation set, so one
+            // tracking client must not push every other connection off the
+            // fast path (writes still use the global gate, since a
+            // non-tracking writer must invalidate everyone else).
+            //
+            // `!conn.in_multi` is shared with the write gate and is not
+            // optional: inside an open transaction a command must be QUEUED,
+            // and the inline path answers it instead. A client then receives
+            // the value where it expects `+QUEUED`, and `EXEC` omits the read
+            // entirely — `MULTI; GET k; EXEC` returned `*0` rather than
+            // `*1[$1 v]`. `MGET`, not being inline-eligible, queued correctly
+            // all along, which is what isolated the path.
+            // See `tests/multi_queues_inline_get.rs`.
+            let can_inline_reads =
+                acl_unrestricted && !conn.in_multi && !conn.tracking_state.enabled;
+            let can_inline_writes = acl_unrestricted
                 && !conn.in_multi
                 && !conn.tracking_state.enabled
                 && !crate::tracking::tracking_active()
@@ -1143,6 +1172,7 @@ pub(crate) async fn handle_connection_sharded_monoio<
                 &ctx.repl_state,
                 ctx.cached_clock.ms(),
                 ctx.num_shards,
+                can_inline_reads,
                 can_inline_writes,
                 // R6: cluster mode disables the inline fast path entirely —
                 // GET/SET must reach try_handle_cluster_routing for MOVED/ASK.

src/server/conn/tests.rs

@@ -52,7 +52,8 @@ fn test_inline_get_hit() {
         &None,
         0,
         1,
-        false,
+        true,  // can_inline_reads
+        false, // can_inline_writes
         &rt_config,
     );
     assert_eq!(result, 1);
@@ -91,7 +92,8 @@ fn test_inline_get_hit_byte_parity_sizes() {
             &None,
             0,
             1,
-            false,
+            true,  // can_inline_reads
+            false, // can_inline_writes
             &rt_config,
         );
 
@@ -136,7 +138,8 @@ fn test_inline_get_miss() {
         &None,
         0,
         1,
-        false,
+        true,  // can_inline_reads
+        false, // can_inline_writes
         &rt_config,
     );
     assert_eq!(result, 1);
@@ -165,7 +168,8 @@ fn test_inline_set_falls_through_when_writes_disabled() {
         &None,
         0,
         1,
-        false,
+        true,  // can_inline_reads
+        false, // can_inline_writes
         &rt_config,
     );
     assert_eq!(result, 0, "SET should fall through inline dispatch");
@@ -193,7 +197,8 @@ fn test_inline_set_executes_when_writes_enabled() {
         &None,
         0,
         1,
-        true,
+        true, // can_inline_reads
+        true, // can_inline_writes
         &rt_config,
     );
     assert_eq!(result, 1, "SET should be inlined");
@@ -234,7 +239,8 @@ fn test_inline_set_with_options_falls_through() {
         &None,
         0,
         1,
-        true,
+        true, // can_inline_reads
+        true, // can_inline_writes
         &rt_config,
     );
     assert_eq!(result, 0, "SET with options should fall through");
@@ -261,7 +267,8 @@ fn test_inline_fallthrough() {
         &None,
         0,
         1,
-        false,
+        true,  // can_inline_reads
+        false, // can_inline_writes
         &rt_config,
     );
     assert_eq!(result, 0);
@@ -297,15 +304,64 @@ fn test_inline_mixed_batch() {
         &None,
         0,
         1,
-        false,
-        false,
+        true,  // can_inline_reads: unrestricted, non-tracking connection
+        false, // can_inline_writes
+        false, // cluster_enabled
         &rt_config,
     );
     assert_eq!(total, 1);
     assert_eq!(&write_buf[..], b"$3\r\nbar\r\n");
     assert_eq!(&read_buf[..], b"*1\r\n$4\r\nPING\r\n");
 }
 
+/// A restricted (or client-side-caching) connection must NOT have its GET
+/// answered here: the inline path runs neither the ACL command/key check nor
+/// `track_read_keys`, so the command has to fall through to generic dispatch.
+/// End-to-end coverage lives in `tests/acl_inline_read_enforcement.rs`; this
+/// pins the gate itself so a future refactor cannot quietly drop it.
+#[test]
+fn test_inline_get_refused_when_reads_not_inlinable() {
+    let dbs = make_dbs();
+    crate::shard::slice::with_shard_db(0, |db| {
+        db.set(
+            Bytes::from_static(b"foo"),
+            Entry::new_string(Bytes::from_static(b"bar")),
+        );
+    });
+    let mut read_buf = BytesMut::new();
+    read_buf.extend_from_slice(b"*2\r\n$3\r\nGET\r\n$3\r\nfoo\r\n");
+    let original_len = read_buf.len();
+    let mut write_buf = BytesMut::new();
+    let aof_pool: Option<std::sync::Arc<crate::persistence::aof::AofWriterPool>> = None;
+    let rt_config = make_rt_config();
+
+    let total = try_inline_dispatch_loop(
+        &mut read_buf,
+        &mut write_buf,
+        &dbs,
+        0,
+        0,
+        &aof_pool,
+        &None,
+        0,
+        1,
+        false, // can_inline_reads: restricted ACL or CLIENT TRACKING conn
+        false, // can_inline_writes
+        false, // cluster_enabled
+        &rt_config,
+    );
+    assert_eq!(total, 0, "GET must not be inlined when reads are gated off");
+    assert_eq!(
+        read_buf.len(),
+        original_len,
+        "the command must be left in the buffer for generic dispatch"
+    );
+    assert!(
+        write_buf.is_empty(),
+        "no value may reach the wire from the inline path"
+    );
+}
+
 #[test]
 fn test_inline_case_insensitive() {
     let dbs = make_dbs();
@@ -330,7 +386,8 @@ fn test_inline_case_insensitive() {
         &None,
         0,
         1,
-        false,
+        true,  // can_inline_reads
+        false, // can_inline_writes
         &rt_config,
     );
     assert_eq!(result, 1);
@@ -358,7 +415,8 @@ fn test_inline_partial() {
         &None,
         0,
         1,
-        false,
+        true,  // can_inline_reads
+        false, // can_inline_writes
         &rt_config,
     );
     assert_eq!(result, 0);
@@ -391,7 +449,8 @@ fn test_inline_set_with_aof_falls_through_when_writes_disabled() {
         &None,
         0,
         1,
-        false,
+        true,  // can_inline_reads
+        false, // can_inline_writes
         &rt_config,
     );
     assert_eq!(
@@ -432,8 +491,9 @@ fn test_inline_multiple_gets() {
         &None,
         0,
         1,
-        false,
-        false,
+        true,  // can_inline_reads: unrestricted, non-tracking connection
+        false, // can_inline_writes
+        false, // cluster_enabled
         &rt_config,
     );
     assert_eq!(total, 2);
@@ -468,7 +528,8 @@ fn test_inline_loop_disabled_in_cluster_mode() {
         &None,
         0,
         1,
-        true, // even with writes inlinable...
+        true, // even with reads inlinable...
+        true, // ...and writes inlinable...
         true, // ...cluster mode wins: nothing may inline
         &rt_config,
     );
@@ -548,7 +609,8 @@ fn test_inline_get_declines_for_cold_key_instead_of_blocking() {
         &None,
         0,
         1,
-        false,
+        true,  // can_inline_reads
+        false, // can_inline_writes
         &rt_config,
     );
     let elapsed = start.elapsed();
@@ -599,7 +661,8 @@ fn test_inline_get_genuine_miss_still_answers_inline() {
         &None,
         0,
         1,
-        false,
+        true,  // can_inline_reads
+        false, // can_inline_writes
         &rt_config,
     );
     assert_eq!(result, 1);