[Security] setIfAbsent atomicity is backend-dependent under high contention

[pathosDev]

Severity / Size

• Severity: MEDIUM (downgraded to LOW after code review — see "Caveat")
• Size: S
• Threat model: closed-group deployment using Cache.setIfAbsent as a distributed lock / idempotency-key. Concurrent callers race for the same key; the contract guarantees exactly one wins.

Affected files

• src/cache/Cache.ts:44 — setIfAbsent interface declares boolean return: true if set, false if key existed.
• src/cache/RedisCache.ts:138-154 — uses SET key value [PX ttl] NX (atomic per Redis spec).
• src/cache/MemcachedCache.ts:123-137 — uses client.add(key, value) (atomic per Memcached spec).
• src/cache/InMemoryCache.ts:61- — single-threaded; trivially atomic.

Caveat — audit framing vs reality

The audit lists this as "Redis without Lua isn't atomic". Code inspection shows the framework already uses the atomic Redis primitive:

• RedisCache.setIfAbsent calls client.set(key, value, 'NX') (line 147) or client.set(key, value, 'PX', ttlMs, 'NX') (line 148). The SET ... NX [PX ttl] form has been atomic in Redis since 2.6.12 (2012). The framework doesn't need Lua for this case.
• MemcachedCache.setIfAbsent calls client.add(...) (line 132). Memcached add is documented atomic.
• InMemoryCache.setIfAbsent is in single-threaded JS — trivially atomic.

Classical "non-atomic setIfAbsent" isn't the bug. The audit is, on its face, a false positive on the atomicity claim.

But there are residual concerns worth addressing:

1. TTL on the absence-set is not enforced when ttlMs is omitted. Without TTL, a setIfAbsent "lock" never expires — caller crash leaves the key locked forever.
2. Memcached's add is atomic per node, not across the cluster. If the user's memcached deployment is multi-node with key-based sharding, add is consistent. But some configurations use replica clusters where add could resolve differently on different replicas.
3. No documented "atomicity contract" — users may expect setIfAbsent to behave the same across backends, but backend-specific semantics (TTL granularity, key-length limits, cluster topology) vary.
4. No integration-style test against real Redis / Memcached that proves the property under contention. Unit tests use the mock client; the mock could agree with the real semantics or not.

The fix is a documentation pass + an integration test + an optional opt-in stricter mode for Memcached cluster topology, not a re-implementation.

Background

setIfAbsent is the canonical "distributed primitive" for:

• Idempotency keys — block double-execution of a request: if (await cache.setIfAbsent('idem:' + key, true, 60_000)) { /* execute */ }.
• Leader election — short-TTL lock: if (await cache.setIfAbsent('leader', myId, 5_000)) { /* I am leader */ }.
• Once-per-window throttle — cache.setIfAbsent('rate:' + key, true, 1_000) and check return.

All three patterns rely on exactly-one-wins under contention + automatic expiry.

Exploit walkthrough (re-framed as "user surprise", not "attacker exploit")

Step 1 — App uses setIfAbsent as a distributed lock:

async function processPayment(id: string): Promise<void> {
  if (!await cache.setIfAbsent(`pay-lock:${id}`, true)) {
    throw new Error('payment already in flight');
  }
  try {
    await charge(id);
  } finally {
    await cache.delete(`pay-lock:${id}`);  // explicit cleanup
  }
}

Step 2 — Caller crashes between setIfAbsent and delete. The TTL wasn't set (default undefined → no expiry on Redis). The key persists forever. Future calls all return false ("already in flight").

Step 3 — Recovery requires manual key deletion, or the entire app's payment processing is dead. Manual recovery → 24/7 ops burden.

Realistic worst case: a single crash bricks a feature until human intervention. Not an attacker exploit — but a designed-in footgun that ops will hit in production.

Alternate framing: an attacker who can crash the app (DoS) → indirectly produces lock-stuck state.

How the 8 already-landed security fixes inform this

• Memcached CRLF guard — added a hardening pass at the wrapper layer rather than relying on the underlying lib. Same shape applies: add a TTL-required mode at the wrapper.
• Wire-frame DoS cap — chose a safe default (16 MB) and required explicit override. Same shape: require explicit TTL for the lock case; the helper is opt-in if you really want no-TTL.
• Idempotency body-fingerprint — tied the lock state to the request's identity. Same pattern: tie the cache key to a clear lifecycle.

Fix design

Track 1 — Documentation: spell out the contract per backend (primary). Add to Cache.ts JSDoc:

/**
 * setIfAbsent — atomically set `key` to `value` if the key does not exist.
 * Returns `true` if set; `false` if the key already exists.
 *
 * **Atomicity guarantees (per backend):**
 *
 *  - **InMemoryCache**: trivially atomic — single-threaded JS.
 *  - **RedisCache**: uses `SET ... NX [PX ttl]`.  Atomic per Redis spec
 *    (2.6.12+).  Holds in Cluster mode (per-key hash slot).  In replica
 *    failover, a write that succeeded on the primary but not yet
 *    replicated may be lost — same caveat as any Redis write.
 *  - **MemcachedCache**: uses `add`.  Atomic per Memcached spec.  When
 *    using a multi-node Memcached cluster with consistent hashing
 *    (default for memjs), atomicity holds per key.  Replica
 *    configurations (rare) may behave differently — verify your
 *    deployment topology if you rely on cross-replica consistency.
 *
 * **TTL recommendation**: pass `ttlMs` for lock-style usage.  A caller
 * that crashes between setIfAbsent and delete would otherwise leave the
 * key set forever.  Best practice: set `ttlMs` to twice the longest
 * operation you'd take under the lock.
 */
setIfAbsent<V = unknown>(key: string, value: V, ttlMs?: number): Promise<boolean>;

Track 2 — Optional "lock-style" helper. New convenience method:

/**
 * Lock-style wrapper for setIfAbsent: requires a TTL.  Returns true if
 * the lock was acquired, false if it was already held.  Recommended for
 * idempotency keys and short-lived distributed locks.
 */
async acquireLock(key: string, ttlMs: number): Promise<boolean> {
  if (!Number.isFinite(ttlMs) || ttlMs <= 0) {
    throw new Error(`acquireLock: ttlMs must be a positive finite number, got ${ttlMs}`);
  }
  return this.setIfAbsent(key, true, ttlMs);
}

Same shape across all three backends (lives in Cache.ts as a non-virtual default; implementations don't need to override).

Track 3 — Integration-style test against real backends. New test file tests/integration/cache-atomicity.test.ts that requires INTEGRATION=1 env var and:

• Spins up a real Redis + Memcached (testcontainers-style or expects them at REDIS_URL / MEMCACHED_URL).
• Runs 100 concurrent setIfAbsent calls against each backend.
• Asserts exactly one returns true.

If integration containers aren't available, skip the test (it.skip(...) with a clear log line).

Track 4 — setIfAbsentStrict opt-in for Memcached cluster topology. For deployments that use Memcached in a replicated topology, expose an opt-in:

new MemcachedCache(client, { requireConsistentHashing: true });

If true, the constructor verifies the client uses consistent hashing (probe client.hashring or fail-fast) and refuses to start otherwise.

API surface

// Existing — unchanged
cache.setIfAbsent('key', value, ttlMs?);

// New — recommended for locks
await cache.acquireLock('key', 5_000);   // mandatory TTL

// New (Memcached-only) — fail-fast on misconfigured topology
new MemcachedCache(client, { requireConsistentHashing: true });

Backward compatibility

Non-breaking. All additions are additive. Existing setIfAbsent calls behave identically.

Test plan

1. Documentation test — JSDoc on setIfAbsent mentions all three backends + TTL recommendation. Verify via existence test (lint rule that reads the JSDoc).
2. acquireLock happy path — await cache.acquireLock('k', 1000) returns true; a second call returns false; after 1.1 seconds, third call returns true again (expired).
3. acquireLock TTL validation — acquireLock('k', 0), acquireLock('k', -1), acquireLock('k', NaN) all throw.
4. 100-concurrent unit test — 100 concurrent setIfAbsent against InMemoryCache → exactly one returns true. Verify against the existing mock-backed RedisCache + MemcachedCache too.
5. Integration test (opt-in) — INTEGRATION=1 bun test cache-atomicity runs the 100-concurrent test against real Redis + Memcached.
6. Memcached requireConsistentHashing test — constructing with a non-consistent-hashing client throws; constructing with a consistent-hashing client succeeds.
7. Regression — existing cache tests pass.

Acceptance criteria

• Cache.setIfAbsent JSDoc lists per-backend atomicity properties + TTL recommendation.
• Cache.acquireLock(key, ttlMs) helper exported.
• Integration-style test under tests/integration/cache-atomicity.test.ts (opt-in via env var).
• Memcached requireConsistentHashing opt-in constructor flag.
• CHANGELOG entry under "Cache: atomicity contract documented + acquireLock helper".
• Test suite covers all 7 cases above.