From e42a471148446d301973cd4f75b71704d5ac5aa2 Mon Sep 17 00:00:00 2001 From: coder Date: Mon, 20 Jul 2026 20:09:58 +0100 Subject: [PATCH] fix(ws): fail closed when WS_AUTH_SECRET is unset (#212) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `const WS_SECRET = WS_AUTH_SECRET || ""` passed the empty string to createHmac(). "" is a perfectly usable HMAC key, so an attacker who knows the secret is unset can compute a valid signature and authenticate. Production and WS_AUTH_REQUIRED=true both process.exit() at module load, so the reachable case is a non-production deploy with WS_AUTH_REQUIRED=false: the `auth` message handler calls verifyWsToken() regardless of whether auth is required, so a forged token still granted slab binding. Gates every HMAC operation on WS_SECRET_USABLE: - generateWsToken() throws rather than mint a forgeable token - verifyWsToken() returns invalid and logs, instead of verifying against "" Unauthenticated clients still connect in development: upgrade-time auto-authentication (`authenticated = !WS_AUTH_REQUIRED`) never consults a token, so only the forgeable path is removed. Also corrects the module doc comment, which claimed WS_AUTH_SECRET "falls back to dev-only default" — it fell back to "", which is what made this exploitable. Verified by driving a real HTTP + WebSocket server (same harness style as ws-ip-limits.test.ts) rather than adding a test-only export: against UNPATCHED src, the 2 vulnerability tests fail — forged token reply: expected 'error', received 'authenticated' generateWsToken: expected to throw, did not against PATCHED src, all 5 pass The 3 control tests (real secret configured) pass both before and after, showing legitimate auth is unaffected. Full suite: 255 -> 260 passed, no new failures. tsc: 26 pre-existing errors before and after, byte-identical, none introduced. The one failing file (tests/sdk-smoke.test.ts) fails identically on clean origin/main — it is the stale local @percolator/shared / SDK, unrelated to this change. Scope: this addresses the empty-secret half of #212 only. See the PR for why the IP-binding half needs to be sequenced behind #230/#231. Co-Authored-By: Claude Opus 4.8 --- src/routes/ws.ts | 39 ++++- .../routes/ws-auth-secret-fail-closed.test.ts | 159 ++++++++++++++++++ 2 files changed, 196 insertions(+), 2 deletions(-) create mode 100644 tests/routes/ws-auth-secret-fail-closed.test.ts diff --git a/src/routes/ws.ts b/src/routes/ws.ts index b2ee32e..0c555b9 100644 --- a/src/routes/ws.ts +++ b/src/routes/ws.ts @@ -43,7 +43,12 @@ const MAX_UNAUTHENTICATED_CONNECTIONS_PER_IP = safePositiveInt( * WS_AUTH_SECRET behavior: * - If set: Used for Bearer token validation. Secure, random 256-bit recommended. * - If not set in production: Startup fails with FATAL error (see lines 36-47) - * - If not set in development: Falls back to dev-only default. DO NOT use in production. + * - If not set in development: token signing and verification are DISABLED and + * fail closed (#212). There is no dev-only default secret — an earlier version + * of this comment claimed one, but the code fell back to the empty string, + * which is a usable HMAC key and therefore trivially forgeable. Unauthenticated + * clients still connect in development via WS_AUTH_REQUIRED=false, which + * auto-authenticates at upgrade time without consulting a token. * * DESIGN: Fail-closed for production. Any misconfiguration causes startup failure, * preventing accidental unauth deployments. @@ -72,8 +77,17 @@ if (!WS_AUTH_SECRET) { logger.warn("WS_AUTH_SECRET not set — token validation will be inactive"); } -// Use WS_AUTH_SECRET when available; empty string when auth is not required +// Use WS_AUTH_SECRET when available; empty string when auth is not required. +// +// #212: an empty string is a USABLE HMAC key, so signing/verifying with it +// produces signatures any attacker can reproduce. Production and +// WS_AUTH_REQUIRED=true both exit above, but a non-production deploy with +// WS_AUTH_REQUIRED=false still reached verifyWsToken() via the `auth` message +// path and accepted tokens signed with "". WS_SECRET_USABLE gates every HMAC +// operation so that an unset secret fails closed instead of silently +// authenticating forged tokens. const WS_SECRET = WS_AUTH_SECRET || ""; +const WS_SECRET_USABLE = WS_SECRET.length > 0; // BH2: Heartbeat configuration const HEARTBEAT_INTERVAL_MS = 30_000; // 30 seconds @@ -297,6 +311,14 @@ function getClientIp(req: IncomingMessage): string | null { * This is a simple token system - can be upgraded to JWT later */ export function generateWsToken(slabAddress: string): string { + // #212: refuse to mint a token keyed with the empty string — it would be + // forgeable by anyone. Throwing is correct here: a caller that cannot sign + // must not receive a token-shaped string it would treat as authoritative. + if (!WS_SECRET_USABLE) { + throw new Error( + "Refusing to generate a WS auth token: WS_AUTH_SECRET is not set" + ); + } const timestamp = Date.now(); const payload = `${slabAddress}:${timestamp}`; const hmac = createHmac("sha256", WS_SECRET); @@ -312,6 +334,19 @@ export function generateWsToken(slabAddress: string): string { */ function verifyWsToken(token: string, expectedSlab?: string): { isValid: boolean; slabAddress: string | null } { try { + // #212: fail closed when no secret is configured. Without this, HMACs are + // keyed with "" and any client can forge a token that verifies. This path + // is reachable in non-production with WS_AUTH_REQUIRED=false, where the + // `auth` message handler calls verifyWsToken() regardless of whether auth + // is required. Clients still connect fine in that mode — upgrade-time + // auto-authentication does not depend on a token. + if (!WS_SECRET_USABLE) { + logger.error( + "Refusing to verify WS auth token: WS_AUTH_SECRET is not set" + ); + return { isValid: false, slabAddress: null }; + } + const parts = token.split(":"); if (parts.length !== 3) return { isValid: false, slabAddress: null }; diff --git a/tests/routes/ws-auth-secret-fail-closed.test.ts b/tests/routes/ws-auth-secret-fail-closed.test.ts new file mode 100644 index 0000000..6c80813 --- /dev/null +++ b/tests/routes/ws-auth-secret-fail-closed.test.ts @@ -0,0 +1,159 @@ +/** + * #212: WS auth must fail closed when WS_AUTH_SECRET is unset. + * + * `WS_SECRET = WS_AUTH_SECRET || ""` fed the empty string to createHmac(). + * "" is a perfectly usable HMAC key, so an attacker who knows the secret is + * unset can compute a valid signature themselves and authenticate. + * + * Production (NODE_ENV=production) and WS_AUTH_REQUIRED=true both process.exit + * at module load, so the reachable case is a non-production deploy with + * WS_AUTH_REQUIRED=false: the `auth` message handler calls verifyWsToken() + * regardless of whether auth is required, so a forged token still granted + * slab binding. + * + * These tests drive a real HTTP + WebSocket server (same harness style as + * ws-ip-limits.test.ts) so the actual reachable path is exercised, rather than + * adding a test-only export to production code. + * + * Env vars used: + * NODE_ENV=test — disables the production-only startup guard + * WS_AUTH_REQUIRED=false — the reachable misconfiguration + * WS_AUTH_SECRET — set/unset per test + */ +import { describe, it, expect, afterEach, vi } from "vitest"; +import http from "node:http"; +import WebSocket from "ws"; +import { createHmac } from "node:crypto"; + +vi.mock("@percolator/shared", () => ({ + createLogger: vi.fn(() => ({ + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + })), + eventBus: { on: vi.fn() }, + getSupabase: vi.fn(() => ({ + from: vi.fn(() => ({ + select: vi.fn(() => ({ + eq: vi.fn(() => ({ + single: vi.fn(() => Promise.resolve({ data: null, error: null })), + })), + })), + })), + })), + sanitizeSlabAddress: vi.fn((s: string) => s), + sendInfoAlert: vi.fn(), +})); + +const SLAB = "So11111111111111111111111111111111111111112"; + +/** Forge a token exactly as an attacker would, given a known/guessed secret. */ +function forgeToken(slab: string, secret: string, timestamp = Date.now()): string { + const payload = `${slab}:${timestamp}`; + const sig = createHmac("sha256", secret).update(payload).digest("hex"); + return `${payload}:${sig}`; +} + +interface TestServer { + server: http.Server; + port: number; + generateWsToken: (slab: string) => string; +} + +async function startServer(secret?: string): Promise { + process.env.NODE_ENV = "test"; + process.env.WS_AUTH_REQUIRED = "false"; + if (secret === undefined) delete process.env.WS_AUTH_SECRET; + else process.env.WS_AUTH_SECRET = secret; + + vi.resetModules(); + const { setupWebSocket, generateWsToken } = await import("../../src/routes/ws.js"); + + const server = http.createServer((_req, res) => { + res.writeHead(200); + res.end("ok"); + }); + setupWebSocket(server as unknown as import("node:http").Server); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + const { port } = server.address() as { port: number }; + return { server, port, generateWsToken }; +} + +async function stopServer(ts: TestServer): Promise { + await new Promise((resolve, reject) => + ts.server.close((err) => (err ? reject(err) : resolve())) + ); +} + +/** Send an `auth` message with `token` and resolve the server's reply. */ +function authAttempt(port: number, token: string): Promise { + return new Promise((resolve, reject) => { + const ws = new WebSocket(`ws://127.0.0.1:${port}/`); + const timer = setTimeout(() => { + ws.close(); + reject(new Error("timed out waiting for auth reply")); + }, 5_000); + + ws.once("open", () => ws.send(JSON.stringify({ type: "auth", token }))); + ws.on("message", (raw) => { + const msg = JSON.parse(raw.toString()); + // Ignore unrelated server chatter (e.g. a welcome frame). + if (msg.type !== "authenticated" && msg.type !== "error") return; + clearTimeout(timer); + ws.close(); + resolve(msg); + }); + ws.once("error", (err) => { + clearTimeout(timer); + reject(err); + }); + }); +} + +describe("#212 WS auth fails closed without WS_AUTH_SECRET", () => { + let ts: TestServer | undefined; + + afterEach(async () => { + if (ts) await stopServer(ts); + ts = undefined; + delete process.env.WS_AUTH_SECRET; + delete process.env.WS_AUTH_REQUIRED; + vi.resetModules(); + }); + + it("rejects an empty-secret-forged token when the secret is unset", async () => { + ts = await startServer(undefined); + // The exact attack: sign with "" because that is what the server used. + const reply = await authAttempt(ts.port, forgeToken(SLAB, "")); + expect(reply.type).toBe("error"); + expect(reply.message).toMatch(/Invalid authentication token/); + }); + + it("refuses to mint a token when the secret is unset", async () => { + ts = await startServer(undefined); + expect(() => ts!.generateWsToken(SLAB)).toThrow(/WS_AUTH_SECRET is not set/); + }); + + it("accepts a legitimately signed token when a secret IS configured", async () => { + const secret = "a-real-secret-value"; + ts = await startServer(secret); + const reply = await authAttempt(ts.port, forgeToken(SLAB, secret)); + expect(reply.type).toBe("authenticated"); + expect(reply.slabBinding).toBe(SLAB); + }); + + it("rejects a token signed with the wrong secret when a secret IS configured", async () => { + ts = await startServer("a-real-secret-value"); + const reply = await authAttempt(ts.port, forgeToken(SLAB, "")); + expect(reply.type).toBe("error"); + }); + + it("mints a usable token when a secret IS configured", async () => { + ts = await startServer("a-real-secret-value"); + const token = ts.generateWsToken(SLAB); + expect(token.split(":")).toHaveLength(3); + const reply = await authAttempt(ts.port, token); + expect(reply.type).toBe("authenticated"); + }); +});