Skip to content

Commit 7650adc

Browse files
committed
fix(jwt): detect the redis client correctly and branch markUsed on it
The store picked its dialect from `client.constructor.name`, matching only 'Redis' or 'Cluster'. A real ioredis instance reports 'EventEmitter' — it extends EventEmitter and no class name survives to the instance — so the probe never matched and every ioredis client was treated as node-redis, sending `SET key value [object Object]`. `markUsed` had a second, independent defect: it never consulted the detected dialect and always used the ioredis positional `eval` form. node-redis does not reject that call, it sends `EVAL <script> 0`, so the script ran against an empty KEYS and failed inside Redis. Between the two, the blacklist and the RFC 6749 §10.4 refresh-reuse registry were unusable on every supported client. Detection now probes the API surface (`scanStream` / `status`) as paseto's store already did, and `markUsed` branches like its neighbours. Also seeds the `deleteAll` SCAN cursor as a string. node-redis typed cursors as numbers through v5 but requires a string from v6, and the peer range (`>=4.0.0`) admits v6. The integration suite's todo markers are lifted and it now covers deleteAll, which had no live-Redis coverage at all.
1 parent 11ee674 commit 7650adc

3 files changed

Lines changed: 63 additions & 29 deletions

File tree

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
---
2+
'@exortek/jwt': patch
3+
---
4+
5+
Fix the Redis store's client detection so the blacklist and refresh registry
6+
work with both supported clients.
7+
8+
`createStore('redis', …)` identified an `ioredis` client by its constructor
9+
name, which never matches a real instance — so every ioredis client took the
10+
node-redis code path and `add()` sent the wrong `SET` argument form.
11+
Separately, `markUsed()` ignored the detected dialect entirely and always used
12+
the ioredis `eval` form, which node-redis accepts but executes with no keys.
13+
Detection now probes the client's API surface, `markUsed()` branches like the
14+
rest of the store, and `deleteAll()` seeds its `SCAN` cursor as a string
15+
(node-redis requires this from v6, which the declared peer range admits).
16+
17+
If you previously worked around this by passing `dialect` explicitly, that
18+
option still works and still takes precedence.

packages/jwt/src/internal/redis-store.js

Lines changed: 30 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
* family membership out of band and delete keys explicitly.
1515
*/
1616

17+
import { isFunction, isString } from '@exortek/shared/predicates';
18+
1719
import { JwtError, ErrorCode } from './errors.js';
1820

1921
// Atomically stamp metadata.usedAt if it is currently null/absent.
@@ -148,7 +150,10 @@ export function createRedisStore(options) {
148150
);
149151
}
150152
const pattern = `${keyPrefix}*`;
151-
let cursor = dialect === 'ioredis' ? '0' : 0;
153+
// Redis cursors are protocol strings. node-redis typed them as numbers
154+
// through v5 but requires a string from v6 on, and the declared peer
155+
// range admits v6 — so seed a string for both dialects.
156+
let cursor = '0';
152157
let count = 0;
153158
try {
154159
do {
@@ -185,7 +190,7 @@ export function createRedisStore(options) {
185190
count++;
186191
}
187192
}
188-
} while (dialect === 'ioredis' ? cursor !== '0' : Number(cursor) !== 0);
193+
} while (String(cursor) !== '0');
189194
} catch (err) {
190195
if (err instanceof JwtError) {
191196
throw err;
@@ -200,7 +205,14 @@ export function createRedisStore(options) {
200205
},
201206
async markUsed(key, nowSec) {
202207
try {
203-
const result = await client.eval(MARK_USED_LUA, 1, build(key), String(nowSec));
208+
// ioredis takes positional EVAL args; node-redis takes an options
209+
// object. Passing the positional form to node-redis does not throw —
210+
// it sends `EVAL <script> 0`, so the script runs with an empty KEYS
211+
// and errors inside Redis instead.
212+
const result =
213+
dialect === 'ioredis'
214+
? await client.eval(MARK_USED_LUA, 1, build(key), String(nowSec))
215+
: await client.eval(MARK_USED_LUA, { keys: [build(key)], arguments: [String(nowSec)] });
204216
if (result == null) {
205217
return null;
206218
}
@@ -237,19 +249,28 @@ export function createRedisStore(options) {
237249
}
238250

239251
/**
240-
* Detect whether the client is `ioredis`-style or `redis@4`-style.
241-
* `ioredis`'s `set` takes positional args (`'EX', ttl`); `redis@4`'s
242-
* takes an options object. We probe the constructor name; users with
243-
* a wrapped client can force via `options.dialect` if we add that
244-
* later.
252+
* Detect whether the client is `ioredis`-style or `node-redis`-style.
253+
* `ioredis`'s `set` takes positional args (`'EX', ttl`) and its `eval`
254+
* takes `(script, numKeys, ...keys, ...args)`; node-redis takes an
255+
* options object for both.
256+
*
257+
* A real `ioredis` instance reports `constructor.name === 'EventEmitter'`
258+
* — it extends EventEmitter and does not set a class name that survives
259+
* to the instance — so a constructor-name probe alone never matches it.
260+
* Detect on the API surface instead (`scanStream` / the `status` string),
261+
* both of which node-redis lacks, and keep the name check as a fallback
262+
* for wrapped clients. Callers with an exotic client can still force the
263+
* answer with `options.dialect`.
245264
*
246265
* @param {any} client
247266
* @returns {'ioredis' | 'node-redis'}
248267
*/
249268
function _detectDialect(client) {
269+
if (client && (isFunction(client.scanStream) || isString(client.status))) {
270+
return 'ioredis';
271+
}
250272
const name = (client && client.constructor && client.constructor.name) || '';
251273
if (name === 'Redis' || name === 'Cluster') {
252-
// ioredis — top-level classes are `Redis` and `Cluster`.
253274
return 'ioredis';
254275
}
255276
return 'node-redis';

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

Lines changed: 15 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -19,24 +19,8 @@ import { createStore } from '../../src/stores.js';
1919

2020
const nowSec = () => Math.floor(Date.now() / 1000);
2121

22-
// Two known defects, scheduled for the follow-up commit. Each case is marked
23-
// only for the driver it actually breaks, so the rest keep asserting:
24-
//
25-
// ioredis — the store misdetects the client as node-redis, so every
26-
// SET-with-TTL path (`add`) sends the wrong argument form.
27-
// node-redis — `markUsed` never consults the detected dialect, so its Lua
28-
// call always uses the ioredis positional form.
29-
const WRONG_SET_FORM = { todo: { ioredis: 'redis dialect detection — fixed in a follow-up' } };
30-
const WRONG_EVAL_FORM = { todo: { 'node-redis': 'markUsed dialect branch — fixed in a follow-up' } };
31-
const BOTH = {
32-
todo: {
33-
ioredis: 'redis dialect detection — fixed in a follow-up',
34-
'node-redis': 'markUsed dialect branch — fixed in a follow-up',
35-
},
36-
};
37-
3822
forEachRedisDriver('jwt redis store', ({ test, client, ns }) => {
39-
test('add + has + get round trip', WRONG_SET_FORM, async () => {
23+
test('add + has + get round trip', async () => {
4024
const store = createStore('redis', { client: client(), keyPrefix: ns('a') });
4125
await store.add('jti-1', nowSec() + 60, { userId: 'u1' });
4226

@@ -47,7 +31,7 @@ forEachRedisDriver('jwt redis store', ({ test, client, ns }) => {
4731
assert.equal(record.metadata.userId, 'u1');
4832
});
4933

50-
test('markUsed flips the record once and reports reuse on the second call', BOTH, async () => {
34+
test('markUsed flips the record once and reports reuse on the second call', async () => {
5135
const store = createStore('redis', { client: client(), keyPrefix: ns('b') });
5236
await store.add('jti-2', nowSec() + 60, {});
5337

@@ -58,15 +42,26 @@ forEachRedisDriver('jwt redis store', ({ test, client, ns }) => {
5842
assert.equal(second.swapped, false, 'second markUsed must report the token as already used');
5943
});
6044

61-
test('markUsed on an absent key resolves to null', WRONG_EVAL_FORM, async () => {
45+
test('markUsed on an absent key resolves to null', async () => {
6246
const store = createStore('redis', { client: client(), keyPrefix: ns('c') });
6347
assert.equal(await store.markUsed('never-added', nowSec()), null);
6448
});
6549

66-
test('delete removes the entry', WRONG_SET_FORM, async () => {
50+
test('delete removes the entry', async () => {
6751
const store = createStore('redis', { client: client(), keyPrefix: ns('d') });
6852
await store.add('jti-3', nowSec() + 60, {});
6953
await store.delete('jti-3');
7054
assert.equal(await store.has('jti-3'), false);
7155
});
56+
57+
test('deleteAll sweeps entries matching a metadata filter', async () => {
58+
const store = createStore('redis', { client: client(), keyPrefix: ns('e') });
59+
await store.add('keep', nowSec() + 60, { familyId: 'f2' });
60+
await store.add('drop-1', nowSec() + 60, { familyId: 'f1' });
61+
await store.add('drop-2', nowSec() + 60, { familyId: 'f1' });
62+
63+
assert.equal(await store.deleteAll({ familyId: 'f1' }), 2);
64+
assert.equal(await store.has('drop-1'), false);
65+
assert.equal(await store.has('keep'), true, 'a non-matching entry must survive');
66+
});
7267
});

0 commit comments

Comments
 (0)