fix(ws): fail closed when WS_AUTH_SECRET is unset (#212)

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 };
 

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<TestServer> {
+  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<void>((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<void> {
+  await new Promise<void>((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<any> {
+  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");
+  });
+});