Fix session authorization to account for clusterIds

.vscode/settings.json

@@ -1,5 +1,9 @@
 {
     "editor.codeActionsOnSave": {
         "source.fixAll.eslint": "always" // Always ensure fixes on both explicit and auto saves.
-    }
+    },
+    // Use the workspace TypeScript (node_modules/typescript) instead of the one
+    // bundled with VS Code.
+    "typescript.tsdk": "node_modules/typescript/lib",
+    "typescript.enablePromptUseWorkspaceTsdk": true
 }

apps/frontend/tsconfig.app.json

@@ -2,10 +2,10 @@
   "extends": "../../tsconfig.base.json",
   "compilerOptions": {
     "tsBuildInfoFile": "./node_modules/.tmp/tsconfig.app.tsbuildinfo",
-    "target": "ES2022",
+    "target": "ES2025",
     "useDefineForClassFields": true,
     "lib": [
-      "ES2022",
+      "ES2025",
       "DOM",
       "DOM.Iterable"
     ],

apps/frontend/tsconfig.node.json

@@ -1,8 +1,8 @@
 {
   "compilerOptions": {
     "tsBuildInfoFile": "./node_modules/.tmp/tsconfig.node.tsbuildinfo",
-    "target": "ES2023",
-    "lib": ["ES2023"],
+    "target": "ES2025",
+    "lib": ["ES2025"],
     "module": "ESNext",
     "skipLibCheck": true,
 

apps/frontend/tsconfig.test.json

@@ -1,8 +1,8 @@
 {
   "extends": "../../tsconfig.base.json",
   "compilerOptions": {
-    "target": "ES2022",
-    "lib": ["ES2022", "DOM", "DOM.Iterable"],
+    "target": "ES2025",
+    "lib": ["ES2025", "DOM", "DOM.Iterable"],
     "module": "ESNext",
     "skipLibCheck": true,
     "baseUrl": ".",

apps/server/package.json

@@ -29,7 +29,7 @@
     "@types/ws": "^8.18.1",
     "ts-node": "^10.9.2",
     "tsup": "^8.5.1",
-    "typescript": "~5.8.3",
+    "typescript": "^6.0.3",
     "vite": "^7.1.7"
   }
 }

apps/server/src/__integration__/session-authorization.integration.test.ts

@@ -0,0 +1,147 @@
+import { after, describe, it } from "node:test"
+import assert from "node:assert/strict"
+import { buildConnectionId, toNodeId, COMMANDLOG_TYPE, MONITOR_ACTION, VALKEY } from "valkey-common"
+import { WsClient } from "./harness/wsClient"
+import {
+  defaultConnectionDetails,
+  defaultStandaloneConnectionDetails,
+  WS_URL
+} from "./harness/fixture"
+
+/**
+ * Session ownership of cluster-scoped actions.
+ *
+ * Two WsClients are two sessions: the harness sends no cookie, so each upgrade
+ * mints a fresh `vk_sid`. `owner` connects to the cluster; `other` connects only
+ * to the standalone node, so it never earns the cluster.
+ *
+ * A rejected action produces no reply at all — the guard returns without
+ * sending. Absence within a window is therefore the assertion. The allow cases
+ * below exercise the same code path and reply well inside it.
+ */
+describe("integration / session authorization (cluster scope)", async () => {
+  const owner = await WsClient.connect(WS_URL)
+  const other = await WsClient.connect(WS_URL)
+
+  const clusterDetails = defaultConnectionDetails()
+  const clusterConnectionId = buildConnectionId(clusterDetails.host, clusterDetails.port, 0)
+  const standaloneDetails = defaultStandaloneConnectionDetails(0)
+  const standaloneConnectionId = buildConnectionId(standaloneDetails.host, standaloneDetails.port, 0)
+
+  let clusterId: string | undefined
+
+  after(async () => {
+    await owner.close()
+    await other.close()
+  })
+
+  it("establishes one cluster session and one unrelated standalone session", async () => {
+    owner.send({
+      type: VALKEY.CONNECTION.connectPending,
+      payload: { connectionId: clusterConnectionId, connectionDetails: clusterDetails },
+    })
+    const connected = await owner.waitFor(VALKEY.CONNECTION.clusterConnectFulfilled, 30000)
+    clusterId = connected.payload?.connectionDetails?.clusterId as string
+    assert.ok(clusterId, "clusterId must be present after cluster connect")
+
+    other.send({
+      type: VALKEY.CONNECTION.connectPending,
+      payload: { connectionId: standaloneConnectionId, connectionDetails: standaloneDetails },
+    })
+    const standalone = await other.waitFor(VALKEY.CONNECTION.standaloneConnectFulfilled, 30000)
+    assert.equal(standalone.payload?.connectionId, standaloneConnectionId)
+  })
+
+  it("rejects a clusterId with no connectionId to derive ownership from", async () => {
+    assert.ok(clusterId, "setup must have run")
+    other.send({
+      type: VALKEY.COMMANDLOGS.commandLogsRequested,
+      payload: { clusterId, commandLogType: COMMANDLOG_TYPE.SLOW },
+    })
+
+    const fulfilled = await other.collectFor(VALKEY.COMMANDLOGS.commandLogsFulfilled, 3000)
+    const errored = await other.collectFor(VALKEY.COMMANDLOGS.commandLogsError, 100)
+    assert.equal(
+      fulfilled.length + errored.length,
+      0,
+      "a clusterId-only payload must be rejected before the handler runs",
+    )
+  })
+
+  it("rejects a foreign clusterId paired with an owned connectionId", async () => {
+    assert.ok(clusterId, "setup must have run")
+    other.send({
+      type: VALKEY.COMMANDLOGS.commandLogsRequested,
+      payload: {
+        connectionId: standaloneConnectionId, // owned by this session
+        clusterId,                            // but belongs to the other session's cluster
+        commandLogType: COMMANDLOG_TYPE.SLOW,
+      },
+    })
+
+    const fulfilled = await other.collectFor(VALKEY.COMMANDLOGS.commandLogsFulfilled, 3000)
+    const errored = await other.collectFor(VALKEY.COMMANDLOGS.commandLogsError, 100)
+    assert.equal(
+      fulfilled.length + errored.length,
+      0,
+      "an owned connectionId must not authorize a cluster it does not belong to",
+    )
+  })
+
+  it("allows a db-less nodeId paired with its own clusterId (monitor banner path)", async () => {
+    assert.ok(clusterId, "setup must have run")
+    // The monitor banner names the node, not the connection: it sends the
+    // db-less nodeId in the `connectionId` field.
+    owner.send({
+      type: VALKEY.MONITOR.monitorRequested,
+      payload: {
+        connectionId: toNodeId(clusterConnectionId),
+        clusterId,
+        monitorAction: MONITOR_ACTION.STATUS,
+      },
+    })
+
+    const replies = await owner.collectFor(VALKEY.MONITOR.monitorFulfilled, 10000)
+    assert.ok(replies.length >= 1, `expected at least one node to answer STATUS; got ${replies.length}`)
+  })
+
+  it("allows an empty-string clusterId on a standalone connection", async () => {
+    // The metrics-retry epic sends `clusterId: details?.clusterId ?? ""`, so a
+    // standalone connection puts "" on the wire. The guard must treat that as
+    // absent — tightening it to `!== undefined` would reject every standalone
+    // metrics refresh.
+    //
+    // Probed with commandLogs rather than monitor on purpose: commandLogs reads
+    // `clusterNodesRegistry[clusterId]` and gets `undefined` for "", so it falls
+    // through to the standalone path and always answers. `monitorRequested`
+    // branches on `typeof clusterId === "string"`, which is true for "", so it
+    // takes the cluster path, resolves zero nodes and replies nothing — which
+    // would be indistinguishable from the guard rejecting the action.
+    other.send({
+      type: VALKEY.COMMANDLOGS.commandLogsRequested,
+      payload: {
+        connectionId: standaloneConnectionId,
+        clusterId: "",
+        commandLogType: COMMANDLOG_TYPE.SLOW,
+      },
+    })
+
+    const fulfilled = await other.collectFor(VALKEY.COMMANDLOGS.commandLogsFulfilled, 10000)
+    const errored = await other.collectFor(VALKEY.COMMANDLOGS.commandLogsError, 100)
+    assert.ok(
+      fulfilled.length + errored.length >= 1,
+      "an empty-string clusterId must not be treated as a cluster claim",
+    )
+  })
+
+  it("keeps the socket usable after a rejection", async () => {
+    // The guard returns; it must not close the connection.
+    other.send({
+      type: VALKEY.MONITOR.monitorRequested,
+      payload: { connectionId: standaloneConnectionId, monitorAction: MONITOR_ACTION.STATUS },
+    })
+    const fulfilled = await other.collectFor(VALKEY.MONITOR.monitorFulfilled, 10000)
+    const errored = await other.collectFor(VALKEY.MONITOR.monitorError, 100)
+    assert.ok(fulfilled.length + errored.length >= 1, "session must still work after rejected actions")
+  })
+})

apps/server/src/actions/utils.ts

@@ -23,7 +23,10 @@ export type ReduxAction = {
 }
 
 export type WsActionMessage = {
-  payload: { connectionId: string },
+  payload: {
+    connectionId: string,
+    clusterId?: string,
+  },
   type: string
 }
 

apps/server/src/index.ts

@@ -3,7 +3,7 @@ import express from "express"
 import helmet from "helmet"
 import path from "path"
 import http from "http"
-import { VALKEY, CONNECTION_TEARDOWN_DELAY_MS } from "valkey-common"
+import { VALKEY, CONNECTION_TEARDOWN_DELAY_MS, isNodeId, toNodeId } from "valkey-common"
 import { fileURLToPath } from "url"
 import rateLimit from "express-rate-limit"
 import { connectPending, resetConnection, closeConnection } from "./actions/connection"
@@ -286,12 +286,41 @@ wss.on("connection", (ws: AliveWebSocket) => {
       // Connection-establishing actions are exempt (authorization happens after successful connect).
       const exempt = action.type === VALKEY.CONNECTION.connectPending
         || action.type === VALKEY.TOPOLOGY.discoveryEndpointPending
-      const targetConnectionId = action.payload?.connectionId
-      if (!exempt && targetConnectionId && !isConnectionAuthorized(ws.sessionId, targetConnectionId)) {
-        console.warn(`Rejected: session does not own connection ${targetConnectionId}`)
-        return
-      }
 
+      if (!exempt) {
+        const targetClusterId = action.payload?.clusterId
+
+        if (connectionId && !isConnectionAuthorized(ws.sessionId, connectionId)) {
+          console.warn(`Rejected: session does not own connection ${connectionId}`)
+          return
+        }
+
+        // clusterId only legitimate if it's the cluster of the connection the session proved it owns
+        if (targetClusterId) {
+          if (!connectionId) {
+            console.warn(`Rejected: session does not have connectionId for clusterId ${targetClusterId}`)
+            return
+          }
+
+          let connectionIdClient = clients.get(connectionId)
+          if (!connectionIdClient?.clusterId && isNodeId(connectionId)) { // connectionId is a nodeId (which is not stored in clients)
+            for (const id of clients.keys()) {
+              if (toNodeId(id) !== connectionId) continue
+              const entry = clients.get(id)
+              if (entry?.clusterId) {
+                connectionIdClient = entry
+                break
+              }
+            }
+          }
+
+          if (connectionIdClient?.clusterId !== targetClusterId) {
+            console.warn(`Rejected: connection ${connectionId} is not part of cluster ${targetClusterId}`)
+            return
+          }
+        }
+      }
+
       await handler(
         { ws,
           clients,

apps/server/tsconfig.json

@@ -2,14 +2,20 @@
   "extends": "../../tsconfig.base.json",
   "compilerOptions": {
     "tsBuildInfoFile": "../../node_modules/.tmp/tsconfig.server.tsbuildinfo",
-    "target": "ES2022",
+    "target": "ES2025",
     "module": "ESNext",
     "moduleResolution": "bundler",
     "lib": [
-      "ES2022"
+      "ES2025"
     ],
     "allowJs": false,
-    "outDir": "dist",
+    // Type-check only: emit is handled by tsup (`tsup src/index.ts --out-dir dist`),
+    // never by tsc. A tsc `outDir` here would be dead config, and since the
+    // `valkey-common` path mapping pulls common/src into this program, the inferred
+    // common source directory spans both packages — which TypeScript 6.0 rejects
+    // with TS5011 unless `rootDir` is set explicitly. Setting `noEmit` instead keeps
+    // this config a pure type-check surface and avoids both TS5011 and TS6059.
+    "noEmit": true,
     "esModuleInterop": true,
     "resolveJsonModule": true,
     "forceConsistentCasingInFileNames": true,

common/package.json

@@ -17,7 +17,8 @@
     "test": "npx tsx --test src/__tests__/*.test.ts"
   },
   "devDependencies": {
-    "tsup": "^7.0.0",
-    "typescript": "^5.5.0"
+    "@types/node": "^24.2.1",
+    "tsup": "^8.5.1",
+    "typescript": "^6.0.3"
   }
 }

common/src/__tests__/connection-id.test.ts

@@ -1,6 +1,6 @@
 import { describe, it } from "node:test"
 import assert from "node:assert"
-import { toNodeId } from "../connection-id"
+import { isNodeId, toNodeId } from "../connection-id"
 
 describe("toNodeId", () => {
   // The shared db-strip helper that turns a Connection_Identifier
@@ -29,3 +29,39 @@ describe("toNodeId", () => {
     assert.strictEqual(toNodeId(""), "")
   })
 })
+
+describe("isNodeId", () => {
+  // True only for a db-less, sanitized id: the metrics-node-id form.
+  it("accepts a db-less node id", () => {
+    assert.strictEqual(isNodeId("127-0-0-1-6379"), true)
+  })
+
+  it("rejects a db-suffixed connection id", () => {
+    assert.strictEqual(isNodeId("127-0-0-1-6379-db0"), false)
+    assert.strictEqual(isNodeId("valkey-7001-7001-db15"), false)
+  })
+
+  it("accepts a non-trailing -db<N> token (not a db suffix)", () => {
+    assert.strictEqual(isNodeId("dbserver-db5-host-6379"), true)
+  })
+
+  it("accepts -db followed by non-digits (not a db suffix)", () => {
+    assert.strictEqual(isNodeId("host-6379-dbx"), true)
+  })
+
+  it("accepts underscores", () => {
+    assert.strictEqual(isNodeId("my_host-6379"), true)
+  })
+
+  it("rejects the empty string", () => {
+    assert.strictEqual(isNodeId(""), false)
+  })
+
+  it("rejects ids outside the sanitized charset", () => {
+    // `sanitizeUrl` collapses everything outside [a-zA-Z0-9_-], so these can
+    // only arrive from a hand-crafted payload.
+    assert.strictEqual(isNodeId("host.example-6379"), false)
+    assert.strictEqual(isNodeId("host/6379"), false)
+    assert.strictEqual(isNodeId("host 6379"), false)
+  })
+})

common/src/connection-id.ts

@@ -43,3 +43,11 @@ export const toNodeId = (id: string): string => id.replace(/-db\d+$/, "")
  */
 export const isValidDatabaseIndex = (db: unknown): db is number =>
   typeof db === "number" && Number.isInteger(db) && db >= 0
+
+/**
+ * Validates it's nodeId (without the -db<number> suffix).
+ * @param id Connection or Node id
+ * @returns if id is not a connection id.
+ */
+export const isNodeId = (id: string): boolean =>
+  id.length > 0 && id === toNodeId(id) && /^[a-zA-Z0-9_-]+$/.test(id)

common/tsconfig.json

@@ -3,11 +3,14 @@
     "declaration": true,
     "outDir": "dist",
     "rootDir": "src",
-    "target": "ES2022",
+    "target": "ES2025",
+    "lib": ["ES2025"],
     "module": "ESNext",
     "moduleResolution": "bundler",
     "esModuleInterop": true,
     "strict": true,
+    "ignoreDeprecations": "6.0",
+    "types": ["node"],
   },
   "include": ["src"]
 }