feat: opt-in WebSocket subprotocol negotiation#203
Merged
Conversation
Browsers reject a WebSocket connection unless the handshake response echoes one of the subprotocols they offered in `Sec-WebSocket-Protocol`. crossws negotiated none by default, breaking browser clients using protocols like `graphql-transport-ws` (#176). Adds two opt-in ways to accept a subprotocol, resolved centrally in `hooks.upgrade()` and folded into the upgrade headers — every adapter already turns that into the accepted subprotocol, so Node, Bun and Deno stay consistent with no per-adapter logic: - `handleProtocols(protocols, request)` adapter option — global default, mirroring ws's selector signature. - `{ protocol }` returned from the `upgrade` hook — per-connection override. Default stays strict (accept none) so a server never claims a protocol the app didn't opt into. Closes #176 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds WebSocket subprotocol negotiation support via a new ChangesSubprotocol negotiation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
The negotiated subprotocol is folded into `upgradeHeaders`, which every adapter reads; the extra `protocol` field on the result was never consumed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
pi0
approved these changes
Jul 3, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/tests.ts (1)
90-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extending coverage to uws/bunny.
These new tests are scoped to
node|bun|deno, matching the docs' stated compatibility note. However, the providedbunny.ts/uws.tssnippets show both adapters already forwardsec-websocket-protocolfromupgradeHeaders, so the centralized resolution inhooks.upgrade()may already work for them too — worth confirming whether the omission is a deliberate runtime limitation or just missing test coverage.🤖 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 `@test/tests.ts` around lines 90 - 115, The new sub-protocol negotiation tests are unnecessarily limited to the node|bun|deno adapters even though the bunny and uws adapters appear to forward sec-websocket-protocol through upgradeHeaders too. Update the coverage around wsConnect/getURL and the upgrade()/handleProtocols paths to include uws and bunny if they support the same behavior, or otherwise document and encode the runtime limitation explicitly in the tests so the adapter scope matches the actual implementation.
🤖 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 `@src/hooks.ts`:
- Around line 171-198: The _resolveProtocol method in hooks.ts is accepting
protocolFromHook and handleProtocols results without verifying they were
actually offered by the client. Update _resolveProtocol to parse the offered
subprotocols once from the request headers, then validate any selected protocol
from protocolFromHook or handleProtocols against that offered set before
returning it. If the chosen value is not in the offered set, ignore it and fall
back to undefined so the handshake only echoes a client-requested protocol.
---
Nitpick comments:
In `@test/tests.ts`:
- Around line 90-115: The new sub-protocol negotiation tests are unnecessarily
limited to the node|bun|deno adapters even though the bunny and uws adapters
appear to forward sec-websocket-protocol through upgradeHeaders too. Update the
coverage around wsConnect/getURL and the upgrade()/handleProtocols paths to
include uws and bunny if they support the same behavior, or otherwise document
and encode the runtime limitation explicitly in the tests so the adapter scope
matches the actual implementation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb398c2a-54bd-4eb9-831e-641ca9ec6777
📒 Files selected for processing (6)
docs/1.guide/2.hooks.mddocs/2.adapters/1.index.mdsrc/adapter.tssrc/hooks.tstest/fixture/_shared.tstest/tests.ts
Comment on lines
+171
to
+198
| async _resolveProtocol( | ||
| request: Request, | ||
| upgradeHeaders: HeadersInit | undefined, | ||
| protocolFromHook: string | undefined, | ||
| ): Promise<string | undefined> { | ||
| if (protocolFromHook) { | ||
| return protocolFromHook; | ||
| } | ||
| if (upgradeHeaders) { | ||
| const headers = | ||
| upgradeHeaders instanceof Headers ? upgradeHeaders : new Headers(upgradeHeaders); | ||
| const fromHeader = headers.get("sec-websocket-protocol"); | ||
| if (fromHeader) { | ||
| return fromHeader; | ||
| } | ||
| } | ||
| const handleProtocols = this.options.handleProtocols; | ||
| if (handleProtocols) { | ||
| const offered = _parseProtocols(request.headers.get("sec-websocket-protocol")); | ||
| if (offered.size > 0) { | ||
| const chosen = await handleProtocols(offered, request); | ||
| if (chosen) { | ||
| return chosen; | ||
| } | ||
| } | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Selected protocol isn't validated against the client-offered set.
Neither protocolFromHook nor the value returned by handleProtocols is checked against offered. Per the JSDoc in src/adapter.ts ("must be one of the offered values") and RFC 6455 §4.2.2, a server must not echo a subprotocol the client didn't request. A misconfigured hook/option (e.g. hardcoding a protocol regardless of what was offered) will silently produce a spec-violating handshake — the same class of browser-rejection bug this PR sets out to fix, just from the opposite direction.
🛡️ Proposed fix: validate against offered protocols
async _resolveProtocol(
request: Request,
upgradeHeaders: HeadersInit | undefined,
protocolFromHook: string | undefined,
): Promise<string | undefined> {
+ const offered = _parseProtocols(request.headers.get("sec-websocket-protocol"));
if (protocolFromHook) {
- return protocolFromHook;
+ return offered.has(protocolFromHook) ? protocolFromHook : undefined;
}
if (upgradeHeaders) {
const headers =
upgradeHeaders instanceof Headers ? upgradeHeaders : new Headers(upgradeHeaders);
const fromHeader = headers.get("sec-websocket-protocol");
if (fromHeader) {
return fromHeader;
}
}
const handleProtocols = this.options.handleProtocols;
- if (handleProtocols) {
- const offered = _parseProtocols(request.headers.get("sec-websocket-protocol"));
+ if (handleProtocols) {
if (offered.size > 0) {
const chosen = await handleProtocols(offered, request);
- if (chosen) {
+ if (chosen && offered.has(chosen)) {
return chosen;
}
}
}
return undefined;
}🤖 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 `@src/hooks.ts` around lines 171 - 198, The _resolveProtocol method in hooks.ts
is accepting protocolFromHook and handleProtocols results without verifying they
were actually offered by the client. Update _resolveProtocol to parse the
offered subprotocols once from the request headers, then validate any selected
protocol from protocolFromHook or handleProtocols against that offered set
before returning it. If the chosen value is not in the offered set, ignore it
and fall back to undefined so the handshake only echoes a client-requested
protocol.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #176.
Problem
Browsers open connections with
new WebSocket(url, protocols)and send the offer in theSec-WebSocket-Protocolrequest header. They reject the connection unless the server's101echoes one of the offered subprotocols back. crossws negotiated none by default (Node hardcodedhandleProtocols: () => false, Deno/others didn't echo either), so browser clients using standard protocols likegraphql-transport-ws/graphql-ws(GraphQL Yoga, Apollo, …) failed to connect.The existing workaround — setting the header yourself in the
upgradehook — worked forwsclients but was undiscoverable boilerplate.Change
Two opt-in ways to accept a subprotocol:
The choice is resolved centrally in
hooks.upgrade()and folded into the upgrade headers. Every adapter already turns asec-websocket-protocolentry in the upgrade headers into the accepted subprotocol (Node re-emits it via ws'sheadersevent, Deno reads it into its nativeprotocol, Bun forwards it toserver.upgrade), so Node, Bun and Deno stay consistent with no per-adapter logic.Resolution precedence:
upgrade(){ protocol }→ asec-websocket-protocolheader the hook set directly (pre-existing way, kept working) →handleProtocolsoption → none.Default stays strict (accept none) so a server never claims a subprotocol the app didn't opt into.
Verified on the wire (Node)
Tests / docs
{ protocol }, header,handleProtocols); new tests gated to node/bun/deno.handleProtocolsin shared adapter options.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes