fix(shared): dispatch redis TTL and Lua calls on the detected client

.changeset/shared-redis-ttl-and-eval.md

@@ -0,0 +1,22 @@
+---
+'@exortek/oauth2': patch
+'@exortek/opaque': patch
+'@exortek/challenge': patch
+'@exortek/magic-link': patch
+---
+
+Fix Redis TTL and Lua handling so both supported clients behave the same.
+
+The internal helper that writes a key with an expiry tried the `ioredis`
+argument form and fell back only if it threw. node-redis does not throw on
+that form — it accepts the call and stores the key with **no expiry at all**,
+so the fallback never ran and the TTL was silently dropped. Anything given a
+lifetime through this path never expired on node-redis: OAuth 2 authorization
+codes, PAR request URIs and device codes among them.
+
+The shared counter behind `challenge` and `magic-link` rate limiting had the
+same problem in its Lua call, where it surfaced as a failure rather than
+silence, and let the driver's own error escape instead of the package's.
+
+Both now dispatch on the detected client, and counter failures are reported as
+the calling package's error type with a `code` you can branch on.

packages/challenge/tests/stores/redis.integration.test.js

@@ -18,13 +18,8 @@ import { forEachRedisDriver } from '@exortek/shared/test-helpers/redis-drivers';
 
 import { redisStore } from '../../src/stores/redis.js';
 
-// The shared counter store issues its Lua call in the ioredis positional form
-// for every client, so the whole suite fails on node-redis. Fixed in the
-// follow-up commit.
-const WRONG_EVAL_FORM = { todo: { 'node-redis': 'incr-store dialect branch — fixed in a follow-up' } };
-
 forEachRedisDriver('challenge redis store', ({ test, client, ns }) => {
-  test('incr counts up from one and reports a future expiry', WRONG_EVAL_FORM, async () => {
+  test('incr counts up from one and reports a future expiry', async () => {
     const store = redisStore(client(), { keyPrefix: ns('a') });
 
     const first = await store.incr('user-1', 60_000);
@@ -35,7 +30,7 @@ forEachRedisDriver('challenge redis store', ({ test, client, ns }) => {
     assert.equal(second.count, 2, 'the counter must be shared across calls');
   });
 
-  test('separate keys keep independent counters', WRONG_EVAL_FORM, async () => {
+  test('separate keys keep independent counters', async () => {
     const store = redisStore(client(), { keyPrefix: ns('b') });
 
     await store.incr('user-a', 60_000);
@@ -45,7 +40,7 @@ forEachRedisDriver('challenge redis store', ({ test, client, ns }) => {
     assert.equal(other.count, 1, 'a different key must start its own counter');
   });
 
-  test('the TTL is bound on the first increment', WRONG_EVAL_FORM, async () => {
+  test('the TTL is bound on the first increment', async () => {
     const store = redisStore(client(), { keyPrefix: ns('c') });
 
     const { expiresAt } = await store.incr('user-ttl', 5_000);

packages/challenge/tests/stores/redis.test.js

@@ -70,6 +70,8 @@ test('incr: falls back to ttlMs when pttl is missing / non-positive', async () =
 test('keyPrefix: prepended before every call', async () => {
   let seenKey;
   const client = {
+    // ioredis-shaped: positional EVAL args.
+    status: 'ready',
     async eval(_s, _n, key) {
       seenKey = key;
       return [1, 1000];

packages/magic-link/tests/stores/redis.integration.test.js

@@ -44,7 +44,7 @@ function newRecord(id, email, extras = {}) {
   };
 }
 
-forEachRedisDriver('magic-link redis store', ({ test, client, ns }) => {
+forEachRedisDriver('magic-link redis store', ({ test, client, ns, raw }) => {
   test('put + getById + consume atomicity', WRONG_EVAL_FORM, async () => {
     const store = redisStore(client(), { keyPrefix: ns('a') });
     await store.put(newRecord('id1', 'a@x.com'));
@@ -70,7 +70,7 @@ forEachRedisDriver('magic-link redis store', ({ test, client, ns }) => {
     );
   });
 
-  test('incrRate increments and PEXPIRE binds a TTL', WRONG_EVAL_FORM, async () => {
+  test('incrRate increments and PEXPIRE binds a TTL', async () => {
     const keyPrefix = ns('c');
     const store = redisStore(client(), { keyPrefix });
 
@@ -80,7 +80,7 @@ forEachRedisDriver('magic-link redis store', ({ test, client, ns }) => {
     assert.equal(b.count, 2);
 
     // Real Redis PTTL reports the remaining ms — must be > 0 and ≤ 60_000.
-    const pttl = await client().pttl(`${keyPrefix}rate:u@x.com`);
+    const pttl = await raw.pttl(`${keyPrefix}rate:u@x.com`);
     assert.ok(pttl > 0 && pttl <= 60_000, `expected a bounded TTL, got ${pttl}`);
   });
 

packages/oauth2/tests/stores/redis.integration.test.js

@@ -11,12 +11,6 @@ import { forEachRedisDriver } from '@exortek/shared/test-helpers/redis-drivers';
 
 import { createRedisAuthCodeStore, createRedisRefreshStore, createRedisDeviceStore } from '../../src/server/index.js';
 
-// The shared `setWithTTL` helper tries the ioredis positional form first and
-// falls back on throw — but node-redis does not throw, it accepts the call and
-// silently stores the key with no expiry. Short-lived grant material therefore
-// persists indefinitely. Fixed in the follow-up commit.
-const TTL_DROPPED = { todo: { 'node-redis': 'setWithTTL dialect branch — fixed in a follow-up' } };
-
 forEachRedisDriver('oauth2 server stores', ({ test, client, ns, raw }) => {
   test('auth-code single-use consume', async () => {
     const store = createRedisAuthCodeStore(client(), { keyPrefix: ns('a') });
@@ -26,7 +20,7 @@ forEachRedisDriver('oauth2 server stores', ({ test, client, ns, raw }) => {
     assert.equal(await store.consume('c1'), undefined, 'a code must not be redeemable twice');
   });
 
-  test('an authorization code carries its expiry into Redis', TTL_DROPPED, async () => {
+  test('an authorization code carries its expiry into Redis', async () => {
     const keyPrefix = ns('b');
     const store = createRedisAuthCodeStore(client(), { keyPrefix });
     await store.save('c-ttl', { clientId: 'app' }, 60_000);

packages/opaque/tests/stores/redis.integration.test.js

@@ -13,20 +13,14 @@ import { forEachRedisDriver } from '@exortek/shared/test-helpers/redis-drivers';
 
 import { redisStore } from '../../src/stores/redis.js';
 
-// The shared `setWithTTL` helper tries the ioredis positional form first and
-// falls back on throw — but node-redis does not throw, it accepts the call and
-// silently stores the key with no expiry. So the fallback never runs and the
-// TTL is dropped. Fixed in the follow-up commit.
-const TTL_DROPPED = { todo: { 'node-redis': 'setWithTTL dialect branch — fixed in a follow-up' } };
-
 forEachRedisDriver('opaque redis store', ({ test, client, ns }) => {
   test('set + get round-trip', async () => {
     const store = redisStore(client(), { keyPrefix: ns('a') });
     await store.set('hash1', { userId: 'usr_1' });
     assert.deepEqual(await store.get('hash1'), { userId: 'usr_1' });
   });
 
-  test('entries expire via native Redis TTL', TTL_DROPPED, async () => {
+  test('entries expire via native Redis TTL', async () => {
     const store = redisStore(client(), { keyPrefix: ns('b') });
     await store.set('hash1', { a: 1 }, { expiresIn: 50 });
     assert.deepEqual(await store.get('hash1'), { a: 1 });

packages/opaque/tests/stores/redis.test.js

@@ -15,6 +15,9 @@ function fakeClient() {
   const kv = new Map();
   return {
     kv,
+    // `status` is part of ioredis's surface and is how the store recognises
+    // the positional argument convention this fake implements.
+    status: 'ready',
     async get(k) {
       const entry = kv.get(k);
       if (!entry) return null;
@@ -35,9 +38,10 @@ function fakeClient() {
 }
 
 /**
- * Mimics node-redis v4's object-options `set` signature — throws on
- * the ioredis-style `set(k, v, 'PX', ms)` call so `setWithTTL`'s
- * try/catch actually falls through to the object-options form.
+ * Mimics node-redis's object-options `set` signature. It exposes none of
+ * ioredis's surface, so the store detects it as node-redis and passes the
+ * options form. It throws on a positional call to make a regression loud —
+ * the real client would accept it and silently drop the TTL.
  */
 function fakeNodeRedisV4Client() {
   const kv = new Map();

packages/shared/src/incr-store.js

@@ -11,6 +11,7 @@
 
 import { isFunction, isInteger, isUndefined } from './predicates.js';
 import { assertRedisClient } from './redis-guard.js';
+import { evalScript } from './redis-helpers.js';
 
 // Memory
 
@@ -157,7 +158,20 @@ export function createRedisIncrStore(client, options = {}, wrap) {
 
   return {
     async incr(key, ttlMs) {
-      const raw = await client.eval(INCR_SCRIPT, 1, k(key), String(Math.max(1, Math.ceil(ttlMs))));
+      let raw;
+      try {
+        raw = await evalScript(client, INCR_SCRIPT, [k(key)], [String(Math.max(1, Math.ceil(ttlMs)))]);
+      } catch (err) {
+        // Surface driver failures as the binding package's own error rather
+        // than letting a raw `ErrorReply` escape — callers branch on
+        // `err instanceof <Pkg>Error` and on `err.code`, and a driver error
+        // satisfies neither.
+        const message = `incr-store.incr: EVAL failed — ${err instanceof Error ? err.message : String(err)}`;
+        if (wrap) {
+          wrap(message, { cause: err });
+        }
+        throw err;
+      }
       const arr = Array.isArray(raw) ? raw : [raw, ttlMs];
       const count = Number(arr[0]);
       const pttl = Number(arr[1]);

packages/shared/src/redis-helpers.js

@@ -13,6 +13,57 @@
 
 import { isFunction, isString } from './predicates.js';
 
+/**
+ * Which calling convention a client expects for the commands whose
+ * signatures differ between drivers — Lua (`eval`) and `SET` with a TTL.
+ *
+ * Most helpers here can dispatch on a method name (`mget` vs `mGet`), but
+ * these two share a name and differ only in how arguments are passed, so
+ * they need an explicit answer.
+ *
+ * A real `ioredis` instance reports `constructor.name === 'EventEmitter'`
+ * — it extends EventEmitter and no class name survives to the instance —
+ * so a constructor-name probe alone never matches it. Detect on the API
+ * surface instead (`scanStream` / the `status` string), neither of which
+ * node-redis has, and keep the name check as a fallback for wrapped
+ * clients. Anything unrecognised is treated as node-redis, whose
+ * options-object form is also what `@upstash/redis` accepts.
+ *
+ * @param {any} client
+ * @returns {'ioredis' | 'node-redis'}
+ */
+export function detectDialect(client) {
+  if (client && (isFunction(client.scanStream) || isString(client.status))) {
+    return 'ioredis';
+  }
+  const name = (client && client.constructor && client.constructor.name) || '';
+  if (name === 'Redis' || name === 'Cluster') {
+    return 'ioredis';
+  }
+  return 'node-redis';
+}
+
+/**
+ * Run a Lua script, passing its keys and arguments the way `client`
+ * expects.
+ *
+ * Getting this wrong is silent on node-redis: handed the ioredis
+ * positional form it does not throw, it sends `EVAL <script> 0` and the
+ * script runs against an empty `KEYS`/`ARGV`.
+ *
+ * @param {any} client
+ * @param {string} script
+ * @param {string[]} keys
+ * @param {string[]} args
+ * @param {'ioredis' | 'node-redis'} [dialect]
+ * @returns {Promise<any>}
+ */
+export function evalScript(client, script, keys, args, dialect = detectDialect(client)) {
+  return dialect === 'ioredis'
+    ? client.eval(script, keys.length, ...keys, ...args)
+    : client.eval(script, { keys, arguments: args });
+}
+
 /**
  * @param {object} client  A Redis-compatible client instance.
  * @returns {{
@@ -26,7 +77,11 @@ import { isFunction, isString } from './predicates.js';
  * }}
  */
 export function createRedisHelpers(client) {
+  const dialect = detectDialect(client);
+
   return {
+    dialect,
+
     async mget(keys) {
       if (keys.length === 0) {
         return [];
@@ -72,9 +127,13 @@ export function createRedisHelpers(client) {
 
     async setWithTTL(key, value, ttlMs) {
       const px = Math.max(1, Math.ceil(ttlMs));
-      try {
+      // Must dispatch on the dialect, not on a try/catch: node-redis accepts
+      // the ioredis positional form without complaint and simply stores the
+      // key with no expiry, so a catch-based fallback never fires and the TTL
+      // is silently dropped.
+      if (dialect === 'ioredis') {
         await client.set(key, value, 'PX', px);
-      } catch {
+      } else {
         await client.set(key, value, { PX: px });
       }
     },

packages/shared/tests/incr-store.test.js

@@ -84,8 +84,9 @@ describe('createRedisIncrStore', () => {
     assert.throws(() => createRedisIncrStore({ get: () => {} }), /eval/);
   });
 
-  test('incr calls eval with the Lua script and returns parsed result', async () => {
+  test('incr passes node-redis its options form and returns the parsed result', async () => {
     let capturedArgs;
+    // No ioredis surface, so the store treats it as node-redis.
     const fakeClient = {
       eval: async (...args) => {
         capturedArgs = args;
@@ -97,12 +98,41 @@ describe('createRedisIncrStore', () => {
 
     assert.equal(res.count, 1);
     assert.ok(res.expiresAt > Date.now() - 100);
-    // verify the key was prefixed
+    assert.deepEqual(capturedArgs[1], { keys: ['test:mykey'], arguments: ['10000'] });
+  });
+
+  test('incr passes ioredis its positional form', async () => {
+    let capturedArgs;
+    const fakeClient = {
+      // `status` is part of ioredis's surface and selects the positional form.
+      status: 'ready',
+      eval: async (...args) => {
+        capturedArgs = args;
+        return [1, 5000];
+      },
+    };
+    const store = createRedisIncrStore(fakeClient, { keyPrefix: 'test:' });
+    await store.incr('mykey', 10_000);
+
+    assert.equal(capturedArgs[1], 1, 'numKeys');
     assert.equal(capturedArgs[2], 'test:mykey');
-    // verify ttlMs was passed as string
     assert.equal(capturedArgs[3], '10000');
   });
 
+  test("a driver failure surfaces through the caller's wrap, not as a raw reply", async () => {
+    class FakeReply extends Error {}
+    const fakeClient = {
+      eval: async () => {
+        throw new FakeReply('ERR something went wrong');
+      },
+    };
+    const store = createRedisIncrStore(fakeClient, {}, msg => {
+      throw new RangeError(`WRAPPED: ${msg}`);
+    });
+
+    await assert.rejects(() => store.incr('k', 1000), /WRAPPED: incr-store.incr: EVAL failed/);
+  });
+
   test('handles Upstash string responses', async () => {
     const fakeClient = {
       eval: async () => ['3', '4500'],
@@ -122,16 +152,16 @@ describe('createRedisIncrStore', () => {
   });
 
   test('default keyPrefix is empty string', async () => {
-    let capturedKey;
+    let capturedKeys;
     const fakeClient = {
-      eval: async (...args) => {
-        capturedKey = args[2];
+      eval: async (_script, options) => {
+        capturedKeys = options.keys;
         return [1, 5000];
       },
     };
     const store = createRedisIncrStore(fakeClient);
     await store.incr('raw', 1000);
-    assert.equal(capturedKey, 'raw');
+    assert.deepEqual(capturedKeys, ['raw']);
   });
 
   test('custom wrap callback for client validation', () => {