From dbb62c6c6969079f90ceafa3a04d55e29bd5cd55 Mon Sep 17 00:00:00 2001 From: Tin Dang Date: Thu, 20 Aug 2026 14:51:43 +0700 Subject: [PATCH] fix(tracking): invalidate client-side caches for movablekeys commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CLIENT TRACKING silently ignored every movablekeys command — one whose keys are not at a fixed argument position — leaving RESP3 client-side caches permanently stale with no signal to the client. Such commands carry `first_key: 0` in COMMAND_META, which mirrors redis's own table and means "the keys are not at a FIXED position", NOT "there are no keys". `tracking::invalidation::command_keys` read it as the latter and returned an empty key list, which disabled both halves of the protocol: * a movablekeys READ (SINTERCARD, ZINTERCARD, ZDIFF/ZINTER/ZUNION, XREAD) never registered the client, so it cached a value it would never be told about; * a movablekeys WRITE (LMPOP, ZMPOP, BLMPOP, BZMPOP, XREADGROUP) never pushed an `invalidate`. `SORT src ... STORE dst` was a third shape: first_key=1 names the SOURCE, so Moon invalidated a key it had not written and missed the one it had — the same for GEORADIUS/GEORADIUSBYMEMBER ... STORE/STOREDIST. Client-side caching is a correctness contract: the client may serve its cached copy until told otherwise. A missed invalidation is therefore unbounded wrong data, not a slow path, and it is invisible to the client. Key extraction now delegates to the walker in `acl::keyspec`, which already understood every one of these layouts. That walker was refactored to report key POSITIONS (`command_key_positions`), with each consumer applying its own policy on top, because the consumers legitimately disagree about the same argv: `SORT ... BY ` reads runtime-computed key names, which ACL must refuse outright (a `~pattern` user could otherwise reach arbitrary keys) while cache invalidation must still act on the keys that ARE named, as redis does. That is the new `KeyPositions::AtPlusComputed` variant. ACL behaviour is unchanged by construction: `acl::keyspec::command_keys` is now a thin fail-closed policy over the shared walker that collapses both `AtPlusComputed` and `Unknown` to `Indeterminate`, i.e. exactly today's answer. All 81 ACL tests pass untouched. Verified against redis-server 8.0.5, which invalidates in all seven probed cases; Moon now matches all seven at --shards 1 and --shards 4 (the latter also exercises the cross-shard capture path, stable over three runs). Each case runs beside a fixed-position CONTROL, and the new integration test was confirmed non-vacuous: reverted against pre-fix sources it fails at the movablekeys assertions AFTER the controls pass, so the failures are the defect and not a dead harness. Closes #582 author: Tin Dang --- CHANGELOG.md | 24 +++ src/acl/keyspec.rs | 243 +++++++++++++++------------ src/tracking/invalidation.rs | 166 ++++++++++++++++--- tests/tracking_movablekeys.rs | 301 ++++++++++++++++++++++++++++++++++ 4 files changed, 605 insertions(+), 129 deletions(-) create mode 100644 tests/tracking_movablekeys.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index d80a9f008..9ec41f055 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- **`CLIENT TRACKING` never invalidated for movablekeys commands, so client-side caches went + permanently stale** (#582). Commands whose keys are not at a fixed argument position carry + `first_key: 0` in `COMMAND_META`, mirroring redis's own table — that means "the keys are not at a + FIXED position", not "there are no keys". The tracking hook's key extractor read it as the latter + and returned an empty key list, which disabled BOTH halves of the protocol: a movablekeys **read** + (`SINTERCARD`, `ZINTERCARD`, `ZDIFF`/`ZINTER`/`ZUNION`, `XREAD`) never registered the client, so + it cached a value it would never be told about; and a movablekeys **write** (`LMPOP`, `ZMPOP`, + `BLMPOP`, `BZMPOP`, `XREADGROUP`) never pushed an `invalidate`. `SORT src ... STORE dst` was a + third shape: `first_key = 1` names the SOURCE, so Moon invalidated a key it had not written and + missed the one it had — likewise `GEORADIUS`/`GEORADIUSBYMEMBER ... STORE`/`STOREDIST`. Because + client-side caching is a correctness contract (the client may serve its cached copy until told + otherwise), a missed invalidation is unbounded stale data with no signal to the client, rather + than a slow path. Measured against `redis-server 8.0.5`, which invalidates in all of these cases; + now verified on the wire at `--shards 1` and `--shards 4`, each case run beside a fixed-position + control so a silent harness cannot pass for a fix. Key extraction here is now **shared with** + `acl::keyspec`, which already understood every one of these layouts (`numkeys` vectors, the + `STREAMS` token, positional `STORE` clauses, subcommand-shaped positions). The shared walker + reports key POSITIONS and each consumer applies its own policy, because the consumers + legitimately disagree: `SORT ... BY ` reads runtime-computed key names, which ACL must + refuse outright while invalidation must still act on the keys that ARE named. ACL behaviour is + unchanged — the fail-closed contract, its error text and every existing key-permission test are + byte-identical. + ### Security - **ACL `~pattern` restrictions were silently unenforced for most multi-key commands** (#566). `AclTable::check_key_permission` read a command's keys from `extract_command_keys`, a diff --git a/src/acl/keyspec.rs b/src/acl/keyspec.rs index 225031282..aed00530b 100644 --- a/src/acl/keyspec.rs +++ b/src/acl/keyspec.rs @@ -83,14 +83,40 @@ const UNREGISTERED_KEYLESS: &[&[u8]] = &[ b"READWRITE", ]; -/// Extract the keys `cmd`/`args` touch. `args` EXCLUDES the command name, so -/// key-spec index `N` maps to `args[N - 1]`. -pub(crate) fn command_keys<'a>(cmd: &[u8], args: &'a [Frame]) -> CommandKeys<'a> { +/// Zero-based positions in `args` of the keys an invocation touches. +pub(crate) type KeyIdx = SmallVec<[usize; 4]>; + +/// Where the keys of an argv live, without interpreting them. +/// +/// This is the single walker every consumer shares (moon#582). It reports +/// FACTS; each caller applies its own POLICY, because the callers legitimately +/// disagree — see [`AtPlusComputed`](KeyPositions::AtPlusComputed). +#[derive(Debug)] +pub(crate) enum KeyPositions { + /// The command provably names no key. + None, + /// The positions of the keys this invocation touches (never empty). + At(KeyIdx), + /// The positions we CAN name, plus at least one key whose name is computed + /// at runtime and therefore cannot be named at all (`SORT ... BY w_*`). + /// + /// ACL must treat this as indeterminate: a `~pattern` user could otherwise + /// reach arbitrary keys through the pattern. Cache invalidation must NOT — + /// redis reports and invalidates the named keys either way, and dropping + /// them would leave a tracking client permanently stale. + AtPlusComputed(KeyIdx), + /// Keys exist (or may exist) but this argv could not be enumerated. + Unknown, +} + +/// Locate the key arguments of `cmd`/`args`. `args` EXCLUDES the command name, +/// so registry key-spec index `N` maps to `args[N - 1]`. +pub(crate) fn command_key_positions(cmd: &[u8], args: &[Frame]) -> KeyPositions { // Layouts a fixed first/last/step spec cannot express come first: some of // them (SORT, OBJECT, ZUNIONSTORE) DO have a spec, but it describes only // part of the truth. - if let Some(keys) = movable_keys(cmd, args) { - return keys; + if let Some(pos) = movable_positions(cmd, args) { + return pos; } let Some(meta) = metadata::lookup(cmd) else { @@ -103,13 +129,13 @@ pub(crate) fn command_keys<'a>(cmd: &[u8], args: &'a [Frame]) -> CommandKeys<'a> .iter() .any(|k| cmd.eq_ignore_ascii_case(k)) { - return CommandKeys::None; + return KeyPositions::None; } - return CommandKeys::Indeterminate; + return KeyPositions::Unknown; }; if meta.first_key <= 0 { - return CommandKeys::None; + return KeyPositions::None; } let argc = args.len(); @@ -121,26 +147,52 @@ pub(crate) fn command_keys<'a>(cmd: &[u8], args: &'a [Frame]) -> CommandKeys<'a> let back = (-meta.last_key) as usize; match (argc + 1).checked_sub(back) { Some(idx) => idx, - None => return CommandKeys::Indeterminate, + None => return KeyPositions::Unknown, } } else { meta.last_key as usize }; // A declared key position that the argv does not reach is a malformed - // invocation — deny rather than silently check fewer keys. + // invocation — report nothing rather than silently naming fewer keys. if first > last || last > argc { - return CommandKeys::Indeterminate; + return KeyPositions::Unknown; } let step = if meta.step > 0 { meta.step as usize } else { 1 }; - let mut keys = KeyVec::new(); + let mut idx = KeyIdx::new(); let mut i = first; while i <= last { - match key_bytes(&args[i - 1]) { + idx.push(i - 1); + i += step; + } + if idx.is_empty() { + return KeyPositions::Unknown; + } + KeyPositions::At(idx) +} + +/// Extract the keys `cmd`/`args` touch, for ACL `~pattern` enforcement. +/// +/// A thin, fail-closed policy over [`command_key_positions`]: anything that +/// cannot be fully enumerated — including an argv that also reaches +/// runtime-computed key names — is [`CommandKeys::Indeterminate`], so the +/// caller denies. +pub(crate) fn command_keys<'a>(cmd: &[u8], args: &'a [Frame]) -> CommandKeys<'a> { + let idx = match command_key_positions(cmd, args) { + KeyPositions::None => return CommandKeys::None, + KeyPositions::At(idx) => idx, + // Some key of this argv is unnameable, so `~pattern` cannot gate it. + KeyPositions::AtPlusComputed(_) | KeyPositions::Unknown => { + return CommandKeys::Indeterminate; + } + }; + let mut keys = KeyVec::new(); + for i in idx { + // A key position holding a non-string is a malformed invocation. + match args.get(i).and_then(key_bytes) { Some(k) => keys.push(k), None => return CommandKeys::Indeterminate, } - i += step; } if keys.is_empty() { return CommandKeys::Indeterminate; @@ -186,7 +238,7 @@ fn key_bytes(frame: &Frame) -> Option<&[u8]> { /// Returns `None` when the command is not one of them (the caller then uses /// the meta-derived walk). Matched on `(len, first byte)` first so the common /// single-key commands fall through after one integer compare. -fn movable_keys<'a>(cmd: &[u8], args: &'a [Frame]) -> Option> { +fn movable_positions(cmd: &[u8], args: &[Frame]) -> Option { let len = cmd.len(); if len == 0 { return None; @@ -195,32 +247,32 @@ fn movable_keys<'a>(cmd: &[u8], args: &'a [Frame]) -> Option> { let keys = match (len, b0) { // ---- numkeys-counted key vectors ---- // numkeys key [key ...] ... - (5, b'l') if cmd.eq_ignore_ascii_case(b"LMPOP") => numkeys_keys(args, 0, false), - (5, b'z') if cmd.eq_ignore_ascii_case(b"ZMPOP") => numkeys_keys(args, 0, false), - (5, b'z') if cmd.eq_ignore_ascii_case(b"ZDIFF") => numkeys_keys(args, 0, false), + (5, b'l') if cmd.eq_ignore_ascii_case(b"LMPOP") => numkeys_positions(args, 0, false), + (5, b'z') if cmd.eq_ignore_ascii_case(b"ZMPOP") => numkeys_positions(args, 0, false), + (5, b'z') if cmd.eq_ignore_ascii_case(b"ZDIFF") => numkeys_positions(args, 0, false), (6, b'z') if cmd.eq_ignore_ascii_case(b"ZINTER") || cmd.eq_ignore_ascii_case(b"ZUNION") => { - numkeys_keys(args, 0, false) + numkeys_positions(args, 0, false) } - (10, b'z') if cmd.eq_ignore_ascii_case(b"ZINTERCARD") => numkeys_keys(args, 0, false), - (10, b's') if cmd.eq_ignore_ascii_case(b"SINTERCARD") => numkeys_keys(args, 0, false), + (10, b'z') if cmd.eq_ignore_ascii_case(b"ZINTERCARD") => numkeys_positions(args, 0, false), + (10, b's') if cmd.eq_ignore_ascii_case(b"SINTERCARD") => numkeys_positions(args, 0, false), // timeout numkeys key [key ...] (6, b'b') if cmd.eq_ignore_ascii_case(b"BLMPOP") || cmd.eq_ignore_ascii_case(b"BZMPOP") => { - numkeys_keys(args, 1, false) + numkeys_positions(args, 1, false) } // script|sha|function numkeys key [key ...] [arg ...] - (4, b'e') if cmd.eq_ignore_ascii_case(b"EVAL") => numkeys_keys(args, 1, false), - (7, b'e') if cmd.eq_ignore_ascii_case(b"EVALSHA") => numkeys_keys(args, 1, false), - (5, b'f') if cmd.eq_ignore_ascii_case(b"FCALL") => numkeys_keys(args, 1, false), - (8, b'f') if cmd.eq_ignore_ascii_case(b"FCALL_RO") => numkeys_keys(args, 1, false), + (4, b'e') if cmd.eq_ignore_ascii_case(b"EVAL") => numkeys_positions(args, 1, false), + (7, b'e') if cmd.eq_ignore_ascii_case(b"EVALSHA") => numkeys_positions(args, 1, false), + (5, b'f') if cmd.eq_ignore_ascii_case(b"FCALL") => numkeys_positions(args, 1, false), + (8, b'f') if cmd.eq_ignore_ascii_case(b"FCALL_RO") => numkeys_positions(args, 1, false), // dest numkeys key [key ...] [WEIGHTS ...] [AGGREGATE ...] // The registry spec names only `dest` (first_key == last_key == 1). (11, b'z') if cmd.eq_ignore_ascii_case(b"ZUNIONSTORE") || cmd.eq_ignore_ascii_case(b"ZINTERSTORE") => { - numkeys_keys(args, 1, true) + numkeys_positions(args, 1, true) } - (10, b'z') if cmd.eq_ignore_ascii_case(b"ZDIFFSTORE") => numkeys_keys(args, 1, true), + (10, b'z') if cmd.eq_ignore_ascii_case(b"ZDIFFSTORE") => numkeys_positions(args, 1, true), // ---- positional STORE clauses (source key + optional destination) ---- (4, b's') if cmd.eq_ignore_ascii_case(b"SORT") => source_plus_store(args, true), @@ -230,12 +282,10 @@ fn movable_keys<'a>(cmd: &[u8], args: &'a [Frame]) -> Option> { source_plus_store(args, false) } - // ---- two-key move with no registry entry ---- - // `RPOPLPUSH src dst` is dispatched by moon#520's work but carries no - // COMMAND_META entry, so the meta-derived walk cannot see its keys and - // the fail-closed default would refuse it outright for key-restricted - // users. Its blocking twin BRPOPLPUSH is in the registry (1..2) and - // needs no arm here. Delete this arm once RPOPLPUSH gets a key spec. + // ---- two-key move ---- + // `RPOPLPUSH src dst` also carries a registry spec (1..2), which would + // give the identical answer; this arm is kept only so the layout is + // stated in one place with its siblings. (9, b'r') if cmd.eq_ignore_ascii_case(b"RPOPLPUSH") => two_keys(args), // ---- keys after the STREAMS token ---- @@ -255,14 +305,14 @@ fn movable_keys<'a>(cmd: &[u8], args: &'a [Frame]) -> Option> { /// `... numkeys key [key ...]` with `numkeys` at `nk_idx`, plus an optional /// destination key at `args[0]` (the `Z*STORE` family). -fn numkeys_keys(args: &[Frame], nk_idx: usize, dest_at_zero: bool) -> CommandKeys<'_> { +fn numkeys_positions(args: &[Frame], nk_idx: usize, dest_at_zero: bool) -> KeyPositions { let Some(nk) = args .get(nk_idx) .and_then(key_bytes) .and_then(|b| std::str::from_utf8(b).ok()) .and_then(|s| s.parse::().ok()) else { - return CommandKeys::Indeterminate; + return KeyPositions::Unknown; }; let first = nk_idx + 1; // `numkeys` larger than the argv is malformed; the command errors out @@ -270,46 +320,42 @@ fn numkeys_keys(args: &[Frame], nk_idx: usize, dest_at_zero: bool) -> CommandKey // reduce enforcement. `checked_add` is load-bearing, not defensive // decoration: `nk` is attacker-controlled, so `first + nk` can overflow // `usize` — in release (no overflow-checks) it WRAPS, the `<` guard then - // sees a tiny sum and the slice below panics `&args[1..0]`. A panic here - // is reachable by any key-restricted user = remote DoS inside ACL. + // sees a tiny sum and the range below panics. A panic here is reachable by + // any key-restricted user = remote DoS inside ACL. let Some(end) = first.checked_add(nk).filter(|&e| e <= args.len()) else { - return CommandKeys::Indeterminate; + return KeyPositions::Unknown; }; - let mut keys = KeyVec::new(); + let mut idx = KeyIdx::new(); if dest_at_zero { - match args.first().and_then(key_bytes) { - Some(dest) => keys.push(dest), - None => return CommandKeys::Indeterminate, + if args.is_empty() { + return KeyPositions::Unknown; } + idx.push(0); } - for frame in &args[first..end] { - match key_bytes(frame) { - Some(k) => keys.push(k), - None => return CommandKeys::Indeterminate, - } - } - if keys.is_empty() { + idx.extend(first..end); + if idx.is_empty() { // `EVAL