test(fuzz): fuzz the shared key-position walker, and unhide new fuzz targets

.github/workflows/fuzz.yml

@@ -39,6 +39,8 @@ jobs:
           - ws_registry_record
           - mq_registry_blob
           - redis_rdb_load
+          - term_fst_sidecar
+          - acl_keyspec
     steps:
       - uses: actions/checkout@v7
       - uses: dtolnay/rust-toolchain@nightly
@@ -103,6 +105,8 @@ jobs:
           - ws_registry_record
           - mq_registry_blob
           - redis_rdb_load
+          - term_fst_sidecar
+          - acl_keyspec
     steps:
       - uses: actions/checkout@v7
       - uses: dtolnay/rust-toolchain@nightly

.gitignore

@@ -79,7 +79,14 @@ moon_*.log
 ssh
 .qdrant-initialized
 libnull.rlib
-fuzz
+# Ignore fuzzing BUILD OUTPUT, not the fuzzers themselves. A bare `fuzz` silently
+# ignored every newly-added target under fuzz/fuzz_targets/ — already-tracked
+# files are unaffected by .gitignore, so the 17 existing targets stayed visible
+# and the trap only sprang on the 18th (moon#576): the target committed clean
+# locally, then CI failed with "no such fuzz target".
+fuzz/target/
+fuzz/corpus/
+fuzz/artifacts/
 shard-*/
 .serena/
 console/tsconfig.app.tsbuildinfo

CHANGELOG.md

@@ -6,6 +6,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Security
+- **Fuzz the shared key-position walker, and stop a bare `.gitignore` entry from hiding new
+  fuzzers** (#576). `acl::keyspec::command_key_positions` parses attacker-controlled argv on behalf
+  of three consumers — ACL key-pattern enforcement, client-side cache invalidation, and command
+  introspection — so one bounds bug is a remote panic in three places at once. PR #571's review had
+  already found exactly that: a `numkeys` `usize` overflow that wrapped `first + nk` in release
+  builds and sliced `&args[1..0]`, reachable by any key-restricted authenticated user. The new
+  `acl_keyspec` target asserts the properties those callers rely on — every reported position
+  indexes `args`; `At` is never empty; and, the security-relevant one, `Unknown` and
+  `AtPlusComputed` must reach ACL as `Indeterminate`, so a `~pattern` user can never be granted a
+  key whose name is computed at runtime (`SORT k BY w_*`). Proven non-vacuous by reverting the
+  `checked_add` guard: the target reproduces the #571 crash from the seed corpus alone, minimizing
+  to the original attack string `LMPOP 18446744073709551615 a LEFT`. Clean over 3.27M executions
+  after restoring it. Two coverage gaps closed alongside: `.gitignore` matched a bare `fuzz`, which
+  ignores only NEW files (already-tracked ones are unaffected), so the 17 existing targets stayed
+  visible while the 18th would have committed clean locally and failed CI as "no such fuzz target";
+  and `term_fst_sidecar` had been present in the tree but listed in neither CI matrix, so it had
+  never actually run.
+
 ### 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

CLAUDE.md

@@ -188,7 +188,7 @@ orb run -m moon-dev bash -c 'sudo apt-get update -qq && sudo apt-get install -y
 - Every new command needs at least one unit test and one consistency test entry.
 - Integration tests use real server instances — no mocking.
 - Benchmarks use Criterion with `black_box()` on inputs and outputs.
-- **Fuzzing:** 12 `cargo-fuzz` targets in `fuzz/fuzz_targets/`. Any new parser, decoder, or deserialization function MUST have a fuzz target. CI runs 15 min/target on PRs and 6h nightly.
+- **Fuzzing:** 18 `cargo-fuzz` targets in `fuzz/fuzz_targets/`. Any new parser, decoder, or deserialization function MUST have a fuzz target, AND an entry in BOTH matrices in `.github/workflows/fuzz.yml` — a target that exists but is not listed never runs (`term_fst_sidecar` sat unlisted until moon#576). CI runs 15 min/target on PRs and 6h nightly.
 - **Loom:** model tests in `tests/loom_response_slot.rs` for lock-free data structures. Any new atomic state machine MUST have a loom model.
 
 ### Module Structure
@@ -263,7 +263,7 @@ legs run locally via `scripts/ci-local.sh` before every push, and again on Actio
 - Client compat (self-hosted, real redis-server oracle)
 - Check (macOS), Check (Windows), Check (console feature)
 
-**Scheduled:** fuzz nightly 6h (12 targets, nightly compiler, `rust-toolchain.toml` removed for the job), Crash Matrix nightly + weekly soak, CodeQL weekly, supply-chain weekly.
+**Scheduled:** fuzz nightly 6h (18 targets, nightly compiler, `rust-toolchain.toml` removed for the job), Crash Matrix nightly + weekly soak, CodeQL weekly, supply-chain weekly.
 
 ### Local CI (the merge bar)
 

fuzz/Cargo.toml

@@ -108,3 +108,8 @@ doc = false
 name = "term_fst_sidecar"
 path = "fuzz_targets/term_fst_sidecar.rs"
 doc = false
+
+[[bin]]
+name = "acl_keyspec"
+path = "fuzz_targets/acl_keyspec.rs"
+doc = false

fuzz/corpus/acl_keyspec/empty_argv

@@ -0,0 +1 @@
+GET

fuzz/corpus/acl_keyspec/ping

@@ -0,0 +1 @@
+PING

fuzz/fuzz_targets/acl_keyspec.rs

@@ -0,0 +1,138 @@
+#![no_main]
+use libfuzzer_sys::fuzz_target;
+
+use bytes::Bytes;
+use moon::acl::keyspec::{CommandKeys, KeyPositions, command_key_positions, command_keys};
+use moon::protocol::Frame;
+use moon::tracking::invalidation;
+
+/// Fuzz the shared key-position walker (moon#576).
+///
+/// `command_key_positions` parses attacker-controlled argv on behalf of THREE
+/// consumers — ACL key-pattern checks, client-side cache invalidation, and
+/// command introspection — so a single bounds bug is a remote panic in three
+/// places at once. PR #571's review already found one of exactly this class: a
+/// `numkeys` usize overflow that produced `&args[1..0]` (fixed in d0423747).
+///
+/// Beyond "never panics", the properties below are the contract the callers
+/// rely on. Two of them are security-relevant, not merely tidy:
+///
+///   * every reported position must index `args` — the bounds property;
+///   * `Unknown` and `AtPlusComputed` must reach ACL as `Indeterminate`.
+///     `AtPlusComputed` means at least one key name is computed at runtime
+///     (`SORT k BY w_*`), so a `~pattern` user could otherwise reach keys the
+///     pattern was never meant to cover. Cache invalidation deliberately does
+///     the opposite with the same value, which is why the walker reports facts
+///     and each caller applies its own policy.
+const MAX_ARGS: usize = 256;
+
+/// Decode `data` into a command name and its argv.
+///
+/// Fields are NUL-separated: the first is the command name, the rest are the
+/// arguments (which EXCLUDE the command name, matching the walker's contract).
+/// A leading tag byte picks the frame type so the fuzzer can reach the
+/// non-string branches — a key position holding an `Integer` is a malformed
+/// invocation the walker still has to survive.
+fn decode(data: &[u8]) -> Option<(Vec<u8>, Vec<Frame>)> {
+    let mut fields = data.split(|&b| b == 0);
+    let cmd = fields.next()?.to_vec();
+    let args = fields
+        .take(MAX_ARGS)
+        .map(|f| match f.split_first() {
+            Some((0x01, rest)) => {
+                // Reach the numkeys/count walkers with values they must clamp:
+                // 0, 1, usize::MAX and its neighbours all live here.
+                let mut n = [0u8; 8];
+                let take = rest.len().min(8);
+                n[..take].copy_from_slice(&rest[..take]);
+                Frame::Integer(i64::from_le_bytes(n))
+            }
+            Some((0x02, _)) => Frame::Null,
+            Some((0x03, rest)) => Frame::SimpleString(Bytes::copy_from_slice(rest)),
+            _ => Frame::BulkString(Bytes::copy_from_slice(f)),
+        })
+        .collect();
+    Some((cmd, args))
+}
+
+fuzz_target!(|data: &[u8]| {
+    let Some((cmd, args)) = decode(data) else {
+        return;
+    };
+    let argc = args.len();
+
+    let positions = command_key_positions(&cmd, &args);
+
+    // Bounds: the property whose violation is a remote panic downstream.
+    match &positions {
+        KeyPositions::At(idx) => {
+            assert!(!idx.is_empty(), "At is documented as never empty");
+            for &i in idx.iter() {
+                assert!(i < argc, "At position {i} out of bounds for argc {argc}");
+            }
+        }
+        KeyPositions::AtPlusComputed(idx) => {
+            for &i in idx.iter() {
+                assert!(
+                    i < argc,
+                    "AtPlusComputed position {i} out of bounds for argc {argc}"
+                );
+            }
+        }
+        KeyPositions::None | KeyPositions::Unknown => {}
+    }
+
+    // The walker is a pure function of its inputs; a consumer that calls it
+    // twice (ACL then tracking, on the same command) must see the same answer.
+    let again = command_key_positions(&cmd, &args);
+    assert_eq!(
+        std::mem::discriminant(&positions),
+        std::mem::discriminant(&again),
+        "walker is not deterministic"
+    );
+
+    // --- consumer 1: ACL, which must fail CLOSED ---
+    let acl = command_keys(&cmd, &args);
+    match (&positions, &acl) {
+        (KeyPositions::None, CommandKeys::None) => {}
+        (KeyPositions::None, other) => {
+            panic!("provably keyless command reached ACL as {other:?}")
+        }
+        // A named position that is not a string cannot be checked against a
+        // key pattern, so `At` is allowed to degrade to Indeterminate.
+        (KeyPositions::At(_), CommandKeys::Keys(k)) => {
+            assert!(!k.is_empty(), "Keys is documented as never empty");
+        }
+        (KeyPositions::At(_), CommandKeys::Indeterminate) => {}
+        (KeyPositions::At(_), CommandKeys::None) => {
+            panic!("command with key positions reached ACL as keyless")
+        }
+        // The security property: neither of these may ever name keys to ACL.
+        (KeyPositions::AtPlusComputed(_) | KeyPositions::Unknown, CommandKeys::Indeterminate) => {}
+        (KeyPositions::AtPlusComputed(_) | KeyPositions::Unknown, other) => {
+            panic!("unenumerable argv must deny, reached ACL as {other:?}")
+        }
+    }
+
+    // --- consumer 2: cache invalidation, which must NOT fail closed ---
+    let tracked = invalidation::command_keys(&cmd, &args);
+    match &positions {
+        KeyPositions::None | KeyPositions::Unknown => {
+            assert!(
+                tracked.is_empty(),
+                "nothing to invalidate, got {} keys",
+                tracked.len()
+            );
+        }
+        KeyPositions::At(idx) | KeyPositions::AtPlusComputed(idx) => {
+            // Non-string positions are skipped, so this is a ceiling, not an
+            // equality — but inventing a key would be an over-invalidation bug.
+            assert!(
+                tracked.len() <= idx.len(),
+                "invalidated {} keys from {} positions",
+                tracked.len(),
+                idx.len()
+            );
+        }
+    }
+});

src/acl/keyspec.rs

@@ -53,11 +53,11 @@ use crate::protocol::Frame;
 
 /// Borrowed key slices. Four inline slots cover every fixed-arity keyed
 /// command in the registry; only variadic forms (`DEL a b c d e`) spill.
-pub(crate) type KeyVec<'a> = SmallVec<[&'a [u8]; 4]>;
+pub type KeyVec<'a> = SmallVec<[&'a [u8]; 4]>;
 
 /// Outcome of key extraction. See the module docs for the contract.
 #[derive(Debug)]
-pub(crate) enum CommandKeys<'a> {
+pub enum CommandKeys<'a> {
     /// The command provably names no key.
     None,
     /// The keys this invocation touches (never empty).
@@ -84,15 +84,15 @@ const UNREGISTERED_KEYLESS: &[&[u8]] = &[
 ];
 
 /// Zero-based positions in `args` of the keys an invocation touches.
-pub(crate) type KeyIdx = SmallVec<[usize; 4]>;
+pub 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 {
+pub enum KeyPositions {
     /// The command provably names no key.
     None,
     /// The positions of the keys this invocation touches (never empty).
@@ -111,7 +111,7 @@ pub(crate) enum KeyPositions {
 
 /// 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 {
+pub 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.
@@ -177,7 +177,7 @@ pub(crate) fn command_key_positions(cmd: &[u8], args: &[Frame]) -> KeyPositions
 /// 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> {
+pub 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,

src/acl/mod.rs

@@ -1,5 +1,9 @@
 pub mod io;
-pub(crate) mod keyspec;
+// `pub` rather than `pub(crate)`: the key-position walker is a contract shared
+// by ACL, cache invalidation and command introspection, and it parses attacker
+// -controlled argv. `fuzz/fuzz_targets/acl_keyspec.rs` drives it directly
+// (moon#576) — a bounds bug here is a remote panic in three places at once.
+pub mod keyspec;
 pub mod log;
 pub mod rules;
 pub mod table;