Skip to content

ws: carry auth token in Sec-WebSocket-Protocol instead of the URL - #281

Merged
denny-il merged 1 commit into
mainfrom
dev/fix-214-ws-token-subprotocol
Aug 23, 2026
Merged

denny-il merged 1 commit into
mainfrom
dev/fix-214-ws-token-subprotocol

Conversation

@denny-il

@denny-il denny-il commented Jul 17, 2026 •

Copy link
Copy Markdown
Contributor

Closes #214 — the CORS and request-body items landed separately; this PR addresses the remaining WebSocket token leak against the current consolidated transport architecture.

What changed

  • packages/client/src/transports/ws sends auth only as a strict nmt.auth.<base64url> WebSocket subprotocol, so tokens never appear in connection URLs.
  • packages/protocol provides the shared encoder/matcher. Decoding is fatal UTF-8 and preserves a leading BOM, avoiding lossy credential normalization.
  • packages/transports/src/neemata/ws extracts the auth offer, echoes the exact selected subprotocol, and keeps connectionData as the standard Request contract.
  • Decoded WebSocket auth is exposed through connectionData.headers.get('authorization'), matching the existing HTTP Request convention without a custom request type or service.
  • There is no query-parameter compatibility path: clients and servers use the subprotocol contract exclusively.

Verification

  • Focused protocol/client/transports coverage under Node and Bun: 25 tests across 5 files per runtime.
  • Complete protocol, client, and transports suites: 530 tests across 59 files on Node; 510 tests across 55 files on Bun.
  • Client browser suites: 378 tests across 63 files.
  • pnpm run fmt
  • pnpm run check
  • Node and Bun shared-host handshakes verify exact echo end to end.
  • Bun and Deno adapter tests verify the exact selection reaches their native upgrade options.

@coderabbitai

coderabbitai Bot commented Jul 17, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

WebSocket authentication is encoded into a negotiated nmt.auth.* subprotocol. The client sends it by default, the server decodes and echoes it during upgrade, and the deprecated URL query fallback remains opt-in and supported.

Changes

WebSocket authentication

Layer / File(s) Summary
Auth subprotocol utilities
packages/protocol/src/common/ws.ts, packages/protocol/src/common/index.ts, packages/protocol/tests/common/ws.spec.ts
Adds base64url encoding, header matching, public re-exports, and coverage for valid, foreign, missing, malformed, BOM-prefixed, and invalid UTF-8 payloads.
Client subprotocol negotiation
packages/ws-client/src/index.ts, packages/ws-client/tests/client.spec.ts
Sends encoded auth through WebSocket protocols and supports the deprecated authQueryParam fallback.
Server upgrade authentication
packages/ws-transport/src/types.ts, packages/ws-transport/src/server.ts, packages/ws-transport/tests/auth-upgrade.spec.ts
Extracts and prioritizes subprotocol auth, echoes the selected protocol, exposes auth on connection requests, and validates live and legacy handshakes.

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

Sequence Diagram(s)

sequenceDiagram
  participant WsTransportClient
  participant WsTransportServer
  participant matchWsAuthSubprotocol
  participant onConnect
  WsTransportClient->>WsTransportServer: WebSocket upgrade with encoded auth subprotocol
  WsTransportServer->>matchWsAuthSubprotocol: Match Sec-WebSocket-Protocol
  matchWsAuthSubprotocol-->>WsTransportServer: Decoded auth and selected subprotocol
  WsTransportServer->>onConnect: Request containing decoded auth
  onConnect-->>WsTransportServer: Accept connection
  WsTransportServer-->>WsTransportClient: Echo selected subprotocol
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR implements the linked WS auth security fix with client, server, shared helpers, and tests.
Out of Scope Changes check ✅ Passed All changes support WebSocket auth transport and related compatibility; no unrelated scope is apparent.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the primary change: moving the WebSocket auth token from the URL to Sec-WebSocket-Protocol.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch dev/fix-214-ws-token-subprotocol

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

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@packages/protocol/src/common/ws.ts`:
- Around line 45-54: Update the auth decoding in the subprotocol parsing flow to
construct TextDecoder with UTF-8 fatal mode, so invalid byte sequences throw and
remain treated as foreign subprotocols by the existing catch. Add a regression
test covering malformed UTF-8 payload rejection.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e45829af-1a9f-45bd-9442-522ba3700033

📥 Commits

Reviewing files that changed from the base of the PR and between f564f54 and 342a89a.

📒 Files selected for processing (8)
  • packages/protocol/src/common/index.ts
  • packages/protocol/src/common/ws.ts
  • packages/protocol/tests/common/ws.spec.ts
  • packages/ws-client/src/index.ts
  • packages/ws-client/tests/client.spec.ts
  • packages/ws-transport/src/server.ts
  • packages/ws-transport/src/types.ts
  • packages/ws-transport/tests/auth-upgrade.spec.ts

Comment thread packages/protocol/src/common/ws.ts
@denny-il
denny-il force-pushed the dev/fix-214-ws-token-subprotocol branch from 738d455 to 867a135 Compare July 17, 2026 09:55
@denny-il
denny-il force-pushed the dev/fix-214-ws-token-subprotocol branch 2 times, most recently from c3ade35 to 7f751c7 Compare August 23, 2026 12:03
@denny-il
denny-il force-pushed the dev/fix-214-ws-token-subprotocol branch from 7f751c7 to 52f5ba3 Compare August 23, 2026 12:22
@denny-il
denny-il merged commit 02a3572 into main Aug 23, 2026
16 of 17 checks passed
@denny-il
denny-il deleted the dev/fix-214-ws-token-subprotocol branch August 23, 2026 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

security defaults: cors:true reflects any origin with credentials; no body size cap on node; WS auth in URL

1 participant