diff --git a/.changeset/redis-connection-cross-major-detection.md b/.changeset/redis-connection-cross-major-detection.md new file mode 100644 index 0000000..2275bcf --- /dev/null +++ b/.changeset/redis-connection-cross-major-detection.md @@ -0,0 +1,9 @@ +--- +'bentocache': patch +--- + +Detect an existing ioredis connection by shape instead of `instanceof` + +`RedisDriver` and `redisBusDriver` decided whether `connection` was a live client or a plain options object with `connection instanceof IoRedis || connection instanceof IoRedisCluster`. That check is evaluated against the `ioredis` copy **bentocache** resolved, so it returns `false` for a perfectly valid client whenever the host application resolved a different `ioredis` major. The driver then fell through to `new IoRedis()`, ioredis ignored the unrecognised properties and silently connected to `127.0.0.1:6379`. + +Both call sites now duck-type on `duplicate` / `sendCommand`, which are present on `Redis` and `Cluster` in every ioredis major and are not valid option names. A value that looks like a client but is not recognisable now throws instead of silently building a connection to localhost. diff --git a/packages/bentocache/package.json b/packages/bentocache/package.json index 7d248af..b96f91f 100644 --- a/packages/bentocache/package.json +++ b/packages/bentocache/package.json @@ -106,6 +106,7 @@ "dayjs": "^1.11.20", "emittery": "^1.2.1", "ioredis": "^5.10.1", + "ioredis-v6": "npm:ioredis@^6.0.0", "knex": "^3.2.7", "kysely": "^0.28.14", "mysql2": "^3.20.0", diff --git a/packages/bentocache/src/drivers/redis.ts b/packages/bentocache/src/drivers/redis.ts index 997a736..cccba38 100644 --- a/packages/bentocache/src/drivers/redis.ts +++ b/packages/bentocache/src/drivers/redis.ts @@ -1,7 +1,8 @@ -import type { RedisOptions as IoRedisOptions } from 'ioredis' +import { Redis as IoRedis } from 'ioredis' +import { InvalidArgumentsException } from '@poppinss/exception' import { RedisTransport } from '@boringnode/bus/transports/redis' -import { Redis as IoRedis, Cluster as IoRedisCluster } from 'ioredis' import type { RedisTransportConfig } from '@boringnode/bus/types/main' +import type { Cluster as IoRedisCluster, RedisOptions as IoRedisOptions } from 'ioredis' import { BaseDriver } from './base_driver.js' import { BinaryEncoder } from '../bus/encoders/binary_encoder.js' @@ -13,6 +14,65 @@ import type { RedisConfig, } from '../types/main.js' +/** + * Detect an already-instantiated ioredis client ( `Redis` or `Cluster` ) as + * opposed to a plain connection options object. + * + * We deliberately do *not* use `instanceof` here. `instanceof` is evaluated + * against the `ioredis` copy that *bentocache* resolved, so it returns `false` + * for a perfectly valid client whenever the host application resolved a + * different `ioredis` major (two copies in the tree). The old code then fell + * through to `new IoRedis()`; ioredis ignores the unrecognised + * properties and silently connects to `127.0.0.1:6379`. + * + * `duplicate` and `sendCommand` are defined on the prototype of both `Redis` + * and `Cluster` in every ioredis major, and neither name exists in + * `RedisOptions` / `ClusterOptions`, so an options object can never be + * misclassified as a client. + * + * Note that `constructor.name` is not usable as a discriminator either: ioredis + * builds its clients through a mixin, so it reports `EventEmitter` for both + * `Redis` and `Cluster`. + */ +function isIoRedisClient(connection: unknown): connection is IoRedis | IoRedisCluster { + if (typeof connection !== 'object' || connection === null) return false + + const candidate = connection as Partial + return typeof candidate.duplicate === 'function' && typeof candidate.sendCommand === 'function' +} + +/** + * Guard against the failure mode that made the `instanceof` bug so expensive: + * silently building a connection to `127.0.0.1:6379` out of something that was + * never an options object. + * + * If the value was not recognised as a client but still carries the markers of + * an event-emitting, stateful client ( a `status` string, an `options` bag and + * `emit` ), we refuse loudly instead of dialing localhost. None of these three + * names exist in `RedisOptions` / `ClusterOptions`, so a legitimate options + * object never trips this. + */ +function assertIsConnectionOptions(connection: unknown): void { + if (typeof connection !== 'object' || connection === null) return + + const candidate = connection as Record + const looksLikeAClient = + typeof candidate.status === 'string' && + typeof candidate.options === 'object' && + candidate.options !== null && + typeof candidate.emit === 'function' + + if (!looksLikeAClient) return + + throw new InvalidArgumentsException( + 'The `connection` given to the Redis driver looks like a Redis client, but is not a ' + + 'recognizable ioredis client. This usually means an incompatible or unsupported ' + + '`ioredis` build was used. Refusing to fall back to a new connection on ' + + '127.0.0.1:6379 - pass either an ioredis `Redis`/`Cluster` instance or a plain ' + + 'connection options object.', + ) +} + /** * Create a new cache redis driver */ @@ -35,12 +95,14 @@ export function redisBusDriver( /** * If an existing Redis or Cluster instance is passed, use it directly */ - if (options.connection instanceof IoRedis || options.connection instanceof IoRedisCluster) { + if (isIoRedisClient(options.connection)) { return new RedisTransport(options.connection, new BinaryEncoder(), { useMessageBuffer: true, }) } + assertIsConnectionOptions(options.connection) + return new RedisTransport( { ...options.connection, useMessageBuffer: true } as RedisTransportConfig, new BinaryEncoder(), @@ -60,11 +122,13 @@ export class RedisDriver extends BaseDriver implements L2CacheDriver { constructor(config: RedisConfig) { super(config) - if (config.connection instanceof IoRedis || config.connection instanceof IoRedisCluster) { + if (isIoRedisClient(config.connection)) { this.#connection = config.connection return } + assertIsConnectionOptions(config.connection) + this.#connection = new IoRedis(config.connection) } diff --git a/packages/bentocache/tests/drivers/redis.spec.ts b/packages/bentocache/tests/drivers/redis.spec.ts index 84e80c6..114f355 100644 --- a/packages/bentocache/tests/drivers/redis.spec.ts +++ b/packages/bentocache/tests/drivers/redis.spec.ts @@ -1,8 +1,9 @@ import { test } from '@japa/runner' import { Redis as IoRedis, Cluster as IoRedisCluster } from 'ioredis' +import { Redis as IoRedisV6, Cluster as IoRedisV6Cluster } from 'ioredis-v6' import { REDIS_CREDENTIALS } from '../helpers/index.js' -import { RedisDriver } from '../../src/drivers/redis.js' +import { RedisDriver, redisBusDriver } from '../../src/drivers/redis.js' import { registerCacheDriverTestSuite } from '../helpers/driver_test_suite.js' test.group('Redis driver', (group) => { @@ -62,4 +63,159 @@ test.group('Redis driver', (group) => { assert.equal(r2, 'value2') assert.equal(r3, null) }) + + /** + * `ioredis-v6` is a second, genuine ioredis install ( `"ioredis-v6": + * "npm:ioredis@^6.0.0"` in devDependencies ) living side by side with the + * `ioredis@5` the driver itself resolves. + * + * A client built from it is exactly what a host application on a different + * `ioredis` major hands to bentocache, and the `instanceof` checks the driver + * used to rely on classified it as a plain connection options object: the + * driver silently built a brand new connection to `127.0.0.1:6379` instead of + * reusing the client it was given. + */ + test('should reuse a client built by another ioredis major', async ({ assert, cleanup }) => { + /** + * Any database but `0`. The failure mode we guard against ends up on + * `127.0.0.1:6379` **db 0**, so writing on another database is what proves + * the write went through the client we were handed rather than through a + * connection the driver conjured up on its own. + */ + const foreignClient = new IoRedisV6({ ...REDIS_CREDENTIALS, db: 3 }) + const fallbackClient = new IoRedis(REDIS_CREDENTIALS) + + cleanup(async () => { + await foreignClient.flushdb() + foreignClient.disconnect() + await fallbackClient.quit() + }) + + /** + * The whole point of the fixture: a real client that is genuinely not of + * the class the driver would test against. + */ + assert.instanceOf(foreignClient, IoRedisV6) + assert.notInstanceOf(foreignClient, IoRedis) + + const driver = new RedisDriver({ + connection: foreignClient as unknown as IoRedis, + prefix: 'japa', + }) + + assert.equal(driver.getConnection(), foreignClient) + + await driver.set('foreign', 'value') + + /** + * Delivery landed on the server the given client is connected to... + */ + assert.equal(await foreignClient.get('japa:foreign'), 'value') + + /** + * ...and not on the `127.0.0.1:6379` db 0 the silent fallback would have + * used. + */ + assert.isNull(await fallbackClient.get('japa:foreign')) + }) + + test('should reuse a Cluster built by another ioredis major', async ({ assert, cleanup }) => { + const foreignCluster = new IoRedisV6Cluster([{ host: '127.0.0.1', port: 7000 }], { + lazyConnect: true, + }) + cleanup(() => foreignCluster.disconnect()) + + assert.instanceOf(foreignCluster, IoRedisV6Cluster) + assert.notInstanceOf(foreignCluster, IoRedisCluster) + + const driver = new RedisDriver({ connection: foreignCluster as unknown as IoRedisCluster }) + + assert.equal(driver.getConnection(), foreignCluster) + }) + + test('should build a new connection when given connection options', async ({ + assert, + cleanup, + }) => { + const connection = { ...REDIS_CREDENTIALS, keyPrefix: 'opts:' } + const driver = new RedisDriver({ connection }) + cleanup(() => driver.disconnect()) + + const created = driver.getConnection() as IoRedis + + assert.notStrictEqual(created as any, connection) + assert.instanceOf(created, IoRedis) + assert.equal(created.options.host, connection.host) + assert.equal(created.options.port, connection.port) + assert.equal(created.options.keyPrefix, 'opts:') + }) + + test('should throw instead of silently connecting to localhost on an unrecognized client', async ({ + assert, + }) => { + /** + * Not reachable with a real client of any major: shaped like a client + * (`status` / `options` / `emit`) but missing the methods we discriminate + * on. We must refuse rather than treat it as an options bag and dial + * 127.0.0.1:6379. + */ + const unknownClient = { status: 'ready', options: { host: 'redis.internal' }, emit: () => true } + + assert.throws( + () => new RedisDriver({ connection: unknownClient as any }), + /looks like a Redis client/, + ) + }) + + test('bus driver should forward a client built by another ioredis major', async ({ + assert, + cleanup, + }) => { + const foreignClient = new IoRedisV6({ ...REDIS_CREDENTIALS, lazyConnect: true }) + + assert.instanceOf(foreignClient, IoRedisV6) + assert.notInstanceOf(foreignClient, IoRedis) + + /** + * Unlike `RedisDriver`, the bus driver hands the connection to + * `RedisTransport`, which keeps it private. And a bare v6 client cannot + * discriminate the two code paths from the outside: `RedisTransport` + * performs the very same `instanceof` check, so it also fails to recognize + * a foreign-major client and builds its own connection either way. That + * one has to be fixed one layer down, in `@boringnode/bus` + * ( boringnode/bus#71 ). + * + * So we observe the only thing that is ours to get right: the client is + * passed *by reference* instead of being shallow-copied into an options + * bag. The real v6 client is fronted by a recorder over a null-prototype + * target, which has no own enumerable key: `{ ...connection }` would + * therefore read nothing, and any property access proves the object itself + * was forwarded. + */ + let connectionWasForwarded = false + const recordedClient = new Proxy(Object.create(null) as IoRedis, { + get(_target, property) { + connectionWasForwarded = true + const value = (foreignClient as any)[property] + return typeof value === 'function' ? value.bind(foreignClient) : value + }, + }) + + const bus = redisBusDriver({ connection: recordedClient }).factory(null as any) + cleanup(async () => { + await bus.disconnect().catch(() => {}) + foreignClient.disconnect() + }) + + assert.isTrue(connectionWasForwarded) + }) + + test('bus driver should throw on an unrecognized client', async ({ assert }) => { + const unknownClient = { status: 'ready', options: { host: 'redis.internal' }, emit: () => true } + + assert.throws( + () => redisBusDriver({ connection: unknownClient as any }).factory(null as any), + /looks like a Redis client/, + ) + }) }) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 6ac1eb0..2257620 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -220,6 +220,9 @@ importers: ioredis: specifier: ^5.10.1 version: 5.10.1 + ioredis-v6: + specifier: npm:ioredis@^6.0.0 + version: ioredis@6.0.0 knex: specifier: ^3.2.7 version: 3.2.7(better-sqlite3@12.8.0)(mysql2@3.20.0(@types/node@25.5.0))(pg@8.20.0)(sqlite3@6.0.1) @@ -1663,6 +1666,9 @@ packages: '@ioredis/commands@1.5.1': resolution: {integrity: sha512-JH8ZL/ywcJyR9MmJ5BNqZllXNZQqQbnVZOqpPQqE1vHiFgAw4NHbvE0FOduNU8IX9babitBT46571OnPTT0Zcw==} + '@ioredis/commands@2.0.0': + resolution: {integrity: sha512-vrx0AE/T0h7cRZwfo1M39Cr+ZhZrkf0V8mQN75wucKCxCLD9l/VX6no3gFvrLqD1IlG/1LtzWovqEw3t0Vr9zg==} + '@isaacs/cliui@8.0.2': resolution: {integrity: sha512-O8jcjabXaleOG9DQ0+ARXWZBTfnP4WNAqzuiJK7ll44AmxGKv/J2M4TPjxjY3znBCfvBXFzucm1twdyFybFqEA==} engines: {node: '>=12'} @@ -3741,6 +3747,10 @@ packages: resolution: {integrity: sha512-BSeNnyus75C4//NQ9gQt1/csTXyo/8Sb+afLAkzAptFuMsod9HFokGNudZpi/oQV73hnVK+sR+5PVRMd+Dr7YQ==} engines: {node: '>=12'} + cluster-key-slot@1.1.1: + resolution: {integrity: sha512-rwHwUfXL40Chm1r08yrhU3qpUvdVlgkKNeyeGPOxnW8/SyVDvgRaed/Uz54AqWNaTCAThlj6QAs3TZcKI0xDEw==} + engines: {node: '>=0.10.0'} + cluster-key-slot@1.1.2: resolution: {integrity: sha512-RMr0FhtfXemyinomL4hrWcYJxmX6deFdCxpJzhDttxgO1+bcCnkk+9drydLVDmAMG7NE6aN/fl4F7ucU/90gAA==} engines: {node: '>=0.10.0'} @@ -4970,6 +4980,10 @@ packages: resolution: {integrity: sha512-C6uC+kleiIMmjViJINWk80sOQw5lEzse1ZmvD+S/s8p8CWapftSaC+kocGTx6xrbrJ4WmYQGC08ffHLr6ToR6Q==} engines: {node: '>=12.22.0'} + ioredis@6.0.0: + resolution: {integrity: sha512-f+Dtubxfpf6KYFq7WVXJoOLn0bk4TJrMrN9SzeE+jrWrCWj7XX3fA6vkryafhADX+GMymRxgDJDOI33COkJc0w==} + engines: {node: '>=20.0.0'} + ip-address@9.0.5: resolution: {integrity: sha512-zHtQzGojZXTwZTHQqra+ETKd4Sn3vgi7uBmlPoXVWZqYvuKmtI0l/VZTjqGmJY9x88GGOaZ9+G9ES8hC4T4X8g==} engines: {node: '>= 12'} @@ -9253,6 +9267,8 @@ snapshots: '@ioredis/commands@1.5.1': {} + '@ioredis/commands@2.0.0': {} + '@isaacs/cliui@8.0.2': dependencies: string-width: 5.1.2 @@ -10790,7 +10806,7 @@ snapshots: '@typescript-eslint/types': 8.23.0 '@typescript-eslint/typescript-estree': 8.23.0(typescript@5.9.3) '@typescript-eslint/visitor-keys': 8.23.0 - debug: 4.4.0 + debug: 4.4.3 eslint: 9.39.4(jiti@2.6.1) typescript: 5.9.3 transitivePeerDependencies: @@ -11631,6 +11647,8 @@ snapshots: strip-ansi: 6.0.1 wrap-ansi: 7.0.0 + cluster-key-slot@1.1.1: {} + cluster-key-slot@1.1.2: {} code-block-writer@13.0.1: {} @@ -12169,7 +12187,7 @@ snapshots: '@types/doctrine': 0.0.9 '@typescript-eslint/scope-manager': 8.22.0 '@typescript-eslint/utils': 8.22.0(eslint@9.39.4(jiti@2.6.1))(typescript@5.9.3) - debug: 4.4.0 + debug: 4.4.3 doctrine: 3.0.0 enhanced-resolve: 5.18.0 eslint: 9.39.4(jiti@2.6.1) @@ -12189,7 +12207,7 @@ snapshots: '@es-joy/jsdoccomment': 0.49.0 are-docs-informative: 0.0.2 comment-parser: 1.4.1 - debug: 4.4.0 + debug: 4.4.3 escape-string-regexp: 4.0.0 eslint: 9.39.4(jiti@2.6.1) espree: 10.3.0 @@ -13006,6 +13024,17 @@ snapshots: transitivePeerDependencies: - supports-color + ioredis@6.0.0: + dependencies: + '@ioredis/commands': 2.0.0 + cluster-key-slot: 1.1.1 + debug: 4.4.3 + denque: 2.1.0 + redis-errors: 1.2.0 + standard-as-callback: 2.1.0 + transitivePeerDependencies: + - supports-color + ip-address@9.0.5: dependencies: jsbn: 1.1.0 @@ -15859,7 +15888,7 @@ snapshots: vue-eslint-parser@9.4.3(eslint@9.39.4(jiti@2.6.1)): dependencies: - debug: 4.4.0 + debug: 4.4.3 eslint: 9.39.4(jiti@2.6.1) eslint-scope: 7.2.2 eslint-visitor-keys: 3.4.3