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

[dcccrypto]

Addresses the empty-secret half of #212. The IP-binding half is deliberately not included — reasoning below.

The bug

const WS_SECRET = WS_AUTH_SECRET || "";   // src/routes/ws.ts:76

"" is a perfectly usable HMAC key. Anyone who knows the secret is unset can compute a valid signature themselves 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.

The fix

Gates every HMAC operation on WS_SECRET_USABLE:

• generateWsToken() throws rather than mint a token it cannot meaningfully sign
• verifyWsToken() returns invalid and logs, instead of verifying against ""

No legitimate flow breaks. Unauthenticated clients still connect in development, because upgrade-time auto-authentication (authenticated = !WS_AUTH_REQUIRED) never consults a token. Only the forgeable path is removed.

Also corrects the module doc comment, which claimed the secret "falls back to dev-only default" — there is no such default; it fell back to "", which is precisely what made this exploitable.

How to test

Driven through a real HTTP + WebSocket server (same harness style as ws-ip-limits.test.ts) rather than adding a test-only export to production code, so the actual reachable path is exercised.

The tests were run against unpatched source to confirm they genuinely catch the bug:

test	unpatched origin/main	with this PR
forged empty-secret token is rejected	❌ expected 'error', received 'authenticated'	✅
generateWsToken refuses to mint	❌ expected to throw, did not	✅
legit token accepted (secret set)	✅	✅
wrong-secret token rejected (secret set)	✅	✅
minted token is usable (secret set)	✅	✅

received 'authenticated' on unpatched code is the exploit, demonstrated end-to-end over a real socket. The 3 control tests pass both before and after, so legitimate auth is provably unaffected.

Suite and typecheck, measured against a clean origin/main baseline:

	baseline	with fix
tests passed	255	260 (+5)
test failures	0	0
tsc errors	26	26 — byte-identical diff, 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. Note CI will also still be red at install until #233 lands.

Why the IP-binding half is NOT here

#212 also asks to bind tokens to the client IP. I looked into it and it should be sequenced, not bundled — three findings that change the calculus:

1. generateWsToken has zero callers. It is exported but never invoked anywhere in src/ or tests/, so tokens are minted by something outside this repo. Adding an IP field makes the token 4 parts and trips the parts.length !== 3 check — that is a breaking cross-service format change I cannot verify against an issuer I cannot see. Shipping it blind risks locking out every legitimate WS client.

2. The IP itself is currently spoofable. getClientIp() trusts X-Forwarded-For per TRUSTED_PROXY_DEPTH, which is exactly what [HIGH-CRITICAL]Bug: Default TRUSTED_PROXY_DEPTH trusts client-supplied X-Forwarded-For, enabling spoofed-IP bypass of HTTP rate limits, IP blocklist, and WebSocket abuse controls #230 reports and PR fix(security): default TRUSTED_PROXY_DEPTH to fail closed #231 fixes. Binding a token to an attacker-controllable value buys little while adding real breakage risk. fix(security): default TRUSTED_PROXY_DEPTH to fail closed #231 should land first.

3. A naive single-use nonce would self-DoS. verifyWsToken() is called twice for one auth message — the slab-rebinding peek at ws.ts:797 and the real check at ws.ts:810. Any replay cache must live at the connection level, not inside verifyWsToken.

Also worth flagging for whoever picks that up: tokens arrive in the URL query string (url.searchParams.get("token")), so they leak into access logs, proxies and Referer headers — which is what makes the replay window worth closing in the first place.

I would rather ship the half that is provably correct and complete than half-ship the half that needs a cross-service decision. Happy to take the follow-up once #231 is in.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • WebSocket authentication now fails closed when no signing secret is configured.
  • Prevented token generation and validation with an empty or missing secret.
  • Valid tokens continue to work correctly when a valid secret is configured.

• Tests

  • Added coverage for missing-secret, forged-token, valid-token, and invalid-signature scenarios.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
percolator-api	Error		Jul 20, 2026 7:10pm

[coderabbitai]

📝 Walkthrough

Walkthrough

WebSocket authentication now fails closed when WS_AUTH_SECRET is unset or empty. Token generation refuses to create tokens, verification rejects tokens, and integration tests cover missing-secret and valid-secret scenarios.

Changes

WebSocket authentication hardening

Layer / File(s)	Summary
Secret usability contract src/routes/ws.ts	Configuration documentation and secret derivation define when WebSocket HMAC operations are allowed.
Token operation enforcement src/routes/ws.ts	generateWsToken() throws without a usable secret, while verifyWsToken() logs and returns an invalid result.
Authentication integration coverage tests/routes/ws-auth-secret-fail-closed.test.ts	Tests cover forged empty-secret tokens, token minting failures, valid and invalid signatures, and generated-token authentication.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

• WS auth token is replayable within 5-min window and falls back to empty HMAC secret #212 — Directly covers the empty-secret HMAC fallback addressed by this PR.

Possibly related PRs

• dcccrypto/percolator-api#54 — Also changes WebSocket connect-time token verification failure behavior.

Suggested reviewers: 0x-squidsol

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: WebSocket auth now fails closed when WS_AUTH_SECRET is unset.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/api-212-ws-token-binding

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

tests/routes/ws-auth-secret-fail-closed.test.ts (1)

35-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Mock eventBus.off for defensive test coverage.

Although eventBus.off isn't invoked during this specific test run (because vi.resetModules() resets the listener variables to null before setupWebSocket inspects them), src/routes/ws.ts relies on eventBus.off during cleanup. Mocking it prevents potential runtime errors if a test manually invokes cleanupEventBusListeners or imports the module differently in the future.

♻️ Proposed refactor

-  eventBus: { on: vi.fn() },
+  eventBus: { on: vi.fn(), off: vi.fn() },

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/routes/ws-auth-secret-fail-closed.test.ts` at line 35, Extend the
eventBus mock in ws-auth-secret-fail-closed.test.ts to include an off vi.fn()
method alongside on, so cleanupEventBusListeners can safely call the mocked
cleanup API in defensive or alternate test flows.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/routes/ws-auth-secret-fail-closed.test.ts`:
- Line 35: Extend the eventBus mock in ws-auth-secret-fail-closed.test.ts to
include an off vi.fn() method alongside on, so cleanupEventBusListeners can
safely call the mocked cleanup API in defensive or alternate test flows.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a8f2ae6b-8539-432e-80bb-cb606d010494

📥 Commits

Reviewing files that changed from the base of the PR and between b2751f4 and e42a471.

📒 Files selected for processing (2)

• src/routes/ws.ts
• tests/routes/ws-auth-secret-fail-closed.test.ts