fix(redis): detect an existing connection by shape instead of instanceof

.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(<a live client>)`, 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.

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",

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(<a live client>)`; 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<IoRedis>
+  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<string, unknown>
+  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)
   }
 

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/,
+    )
+  })
 })