Conversation
Returns standard crossws hooks that forward incoming peers to an upstream ws:// or wss:// target. Supports static or dynamic targets, subprotocol forwarding, and message buffering during upstream connect.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a WebSocket proxy utility Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/proxy.ts (2)
143-154: Optional chaining onpeer.requestis unnecessary.Based on the
Peerclass definition,requestis a required property getter that always returns aRequestobject. The optional chaining on line 148 is defensive but technically unnecessary.♻️ Minor: Remove unnecessary optional chaining
function _resolveProtocols( peer: Peer, forwardProtocol: boolean | undefined, ): string[] | undefined { if (forwardProtocol === false) return; - const header = peer.request?.headers.get("sec-websocket-protocol"); + const header = peer.request.headers.get("sec-websocket-protocol"); if (!header) return; return header .split(",") .map((p) => p.trim()) .filter(Boolean); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy.ts` around lines 143 - 154, In _resolveProtocols, remove the unnecessary optional chaining on peer.request since Peer.request is guaranteed to return a Request; change any use of peer.request?.headers.get(...) to peer.request.headers.get(...) in the _resolveProtocols function so it directly accesses the headers, preserving the existing early returns and behavior.
89-101: Consider adding a buffer size limit to prevent memory exhaustion.If a client sends many messages before the upstream connection opens (e.g., slow upstream or intentional abuse), the buffer grows unboundedly. While this may be acceptable for typical use cases, a malicious or misbehaving client could exhaust server memory.
💡 Optional: Add buffer size limit
+const MAX_BUFFER_SIZE = 100; // or make configurable via options + message(peer, message) { const state = upstreams.get(peer.id); if (!state) return; const data = typeof message.rawData === "string" ? message.rawData : message.uint8Array(); if (state.open) { state.ws.send(data as Parameters<WebSocket["send"]>[0]); } else { + if (state.buffer.length >= MAX_BUFFER_SIZE) { + _safeClose(peer, 1008, "Buffer overflow"); + upstreams.delete(peer.id); + state.ws.close(); + return; + } state.buffer.push(data); } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy.ts` around lines 89 - 101, The message(peer, message) handler currently pushes incoming messages into state.buffer unboundedly while state.open is false; add a max buffer size constant (e.g., MAX_BUFFER_SIZE) and, before pushing to state.buffer, check state.buffer.length and enforce the limit—either drop the oldest item (shift) and push the new item, or log and close the client/upstream connection (use upstreams, state, and peer.id to locate code) to prevent memory exhaustion; ensure you also log why the buffer was dropped/connection closed for observability and update any tests or comments referencing state.buffer behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/proxy.ts`:
- Around line 143-154: In _resolveProtocols, remove the unnecessary optional
chaining on peer.request since Peer.request is guaranteed to return a Request;
change any use of peer.request?.headers.get(...) to
peer.request.headers.get(...) in the _resolveProtocols function so it directly
accesses the headers, preserving the existing early returns and behavior.
- Around line 89-101: The message(peer, message) handler currently pushes
incoming messages into state.buffer unboundedly while state.open is false; add a
max buffer size constant (e.g., MAX_BUFFER_SIZE) and, before pushing to
state.buffer, check state.buffer.length and enforce the limit—either drop the
oldest item (shift) and push the new item, or log and close the client/upstream
connection (use upstreams, state, and peer.id to locate code) to prevent memory
exhaustion; ensure you also log why the buffer was dropped/connection closed for
observability and update any tests or comments referencing state.buffer
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8c5d376-b4a2-4f55-aed0-5c0f71577ee1
📒 Files selected for processing (4)
docs/1.guide/7.proxy.mdsrc/index.tssrc/proxy.tstest/proxy.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/proxy.test.ts (2)
183-190: ExerciseforwardProtocol: falseas well.The suite covers the default subprotocol passthrough, but not the opt-out branch of the new public option. I'd add a second proxy fixture with
forwardProtocol: falseand assert that no protocol is negotiated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/proxy.test.ts` around lines 183 - 190, Add a second test-case that exercises the opt-out branch by creating a proxy fixture configured with forwardProtocol: false, then open a WebSocket via wsConnect (same as the existing test) to that proxy URL and assert the proxy does not forward/negotiate a subprotocol; for example, check ws.inspector.headers does not contain "sec-websocket-protocol" (or that the negotiated protocol field is undefined/empty). Update or add a proxy fixture name/reference and use forwardProtocol: false so the test validates the non-passthrough behavior alongside the existing forwardProtocol default test.
176-180: Add the missing client → upstream binary case.This only proves upstream → client binary delivery. It doesn't verify that an inbound binary frame stays binary through
src/adapters/node.tsandsrc/proxy.ts, so a regression in that direction would still pass.➕ Suggested coverage
+ test("forwards client binary frames as binary", async () => { + const ws = await wsConnect(proxyURL, { skip: 1 }); + await ws.send(new TextEncoder().encode("type")); + expect(await ws.next()).toBe("type:binary"); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/proxy.test.ts` around lines 176 - 180, Add a complementary test in test/proxy.test.ts that exercises the client→upstream binary path: using wsConnect (same as existing test) send a raw binary payload (e.g., a Buffer/Uint8Array) instead of UTF-8 text and assert the upstream handler (or mock) receives raw binary bytes (or that the upstream reply preserves binary framing back to the client); ensure the test references the same wsConnect and proxyURL setup and verifies that src/adapters/node.ts / src/proxy.ts do not convert the outbound binary frame to text by checking the actual byte content or binary marker on the upstream side.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/proxy.test.ts`:
- Around line 183-190: Add a second test-case that exercises the opt-out branch
by creating a proxy fixture configured with forwardProtocol: false, then open a
WebSocket via wsConnect (same as the existing test) to that proxy URL and assert
the proxy does not forward/negotiate a subprotocol; for example, check
ws.inspector.headers does not contain "sec-websocket-protocol" (or that the
negotiated protocol field is undefined/empty). Update or add a proxy fixture
name/reference and use forwardProtocol: false so the test validates the
non-passthrough behavior alongside the existing forwardProtocol default test.
- Around line 176-180: Add a complementary test in test/proxy.test.ts that
exercises the client→upstream binary path: using wsConnect (same as existing
test) send a raw binary payload (e.g., a Buffer/Uint8Array) instead of UTF-8
text and assert the upstream handler (or mock) receives raw binary bytes (or
that the upstream reply preserves binary framing back to the client); ensure the
test references the same wsConnect and proxyURL setup and verifies that
src/adapters/node.ts / src/proxy.ts do not convert the outbound binary frame to
text by checking the actual byte content or binary marker on the upstream side.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 979071b0-4bcc-4af0-ae7e-2afb92c65a71
📒 Files selected for processing (4)
docs/1.guide/7.proxy.mdsrc/adapters/node.tssrc/proxy.tstest/proxy.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/1.guide/7.proxy.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/proxy.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/proxy.ts (2)
92-95: Handle potential empty subprotocol from malformed headers.If the
sec-websocket-protocolheader is malformed (e.g.,", chat"with a leading comma),split(",")[0].trim()yields an empty string, which would set an empty protocol header. Consider filtering out empty values.Suggested fix
// Accept the first requested subprotocol so the upgrade handshake // echoes a value the client expects. Upstream must support it too. - const accepted = reqProtocol.split(",")[0]!.trim(); + const accepted = reqProtocol + .split(",") + .map((p) => p.trim()) + .find(Boolean); + if (!accepted) return; return { headers: { "sec-websocket-protocol": accepted } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy.ts` around lines 92 - 95, The current logic picks the first token from reqProtocol via reqProtocol.split(",")[0]!.trim() which can produce an empty string for malformed headers (e.g., ", chat"); update the selection to filter tokens: split reqProtocol by ",", map(trim), filter out empty strings, then pick the first non-empty token and set it to the "sec-websocket-protocol" header (variable referenced as accepted); if no non-empty token exists, do not include the sec-websocket-protocol header in the returned object to avoid sending an empty protocol value.
168-179: Buffer size calculation may undercount multi-byte characters.For strings,
raw.lengthreturns character count, not byte count. With multi-byte UTF-8 characters (emoji, CJK, etc.), the actual memory usage exceeds the calculated size. This could allow buffers to grow slightly beyondmaxBufferSizein bytes.If strict byte-based enforcement is needed:
Suggested fix using TextEncoder
- const size = typeof raw === "string" ? raw.length : raw.byteLength; + const size = + typeof raw === "string" + ? new TextEncoder().encode(raw).length + : raw.byteLength;Note: This adds encoding overhead per message. The current approach is a reasonable approximation for most use cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy.ts` around lines 168 - 179, The size calculation uses raw.length for strings which counts UTF-16 code units, not UTF-8 bytes, so compute the true byte length with a TextEncoder and push the encoded Uint8Array into state.buffer instead of the original string; update the branch around size = ... and the subsequent state.buffer.push and state.bufferSize increment so that when typeof raw === "string" you call TextEncoder().encode(raw) (use its length for size and push the resulting Uint8Array), otherwise keep the existing Uint8Array.from(raw) behavior; ensure references: state.bufferSize, options.maxBufferSize, DEFAULT_MAX_BUFFER_SIZE, _cleanupState, _safeClose, and state.buffer are adjusted accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/proxy.ts`:
- Around line 92-95: The current logic picks the first token from reqProtocol
via reqProtocol.split(",")[0]!.trim() which can produce an empty string for
malformed headers (e.g., ", chat"); update the selection to filter tokens: split
reqProtocol by ",", map(trim), filter out empty strings, then pick the first
non-empty token and set it to the "sec-websocket-protocol" header (variable
referenced as accepted); if no non-empty token exists, do not include the
sec-websocket-protocol header in the returned object to avoid sending an empty
protocol value.
- Around line 168-179: The size calculation uses raw.length for strings which
counts UTF-16 code units, not UTF-8 bytes, so compute the true byte length with
a TextEncoder and push the encoded Uint8Array into state.buffer instead of the
original string; update the branch around size = ... and the subsequent
state.buffer.push and state.bufferSize increment so that when typeof raw ===
"string" you call TextEncoder().encode(raw) (use its length for size and push
the resulting Uint8Array), otherwise keep the existing Uint8Array.from(raw)
behavior; ensure references: state.bufferSize, options.maxBufferSize,
DEFAULT_MAX_BUFFER_SIZE, _cleanupState, _safeClose, and state.buffer are
adjusted accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 48e85591-b91f-4ac4-a5e4-d723d79ffba3
📒 Files selected for processing (3)
docs/1.guide/7.proxy.mdsrc/proxy.tstest/proxy.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/1.guide/7.proxy.md
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
1 participant
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 #93.
Adds
createWebSocketProxy(target)insrc/proxy.ts— returns a standard crosswsPartial<Hooks>object that forwards every peer to an upstreamws://orwss://target.URLinstances, or(peer) => urlresolversmaxBufferSizelimit)connectTimeout,forwardProtocol,headers, and customWebSocketconstructor optionsdocs/1.guide/7.proxy.md, including SSRF and open-relay warningsNote: node adapter behavior change
src/adapters/node.tsnow decodes text frames tostringbefore dispatching, matching every other adapter. Previouslymessage.rawDatawas aBufferfor text frames on Node — anyone branching ontypeof message.rawDatawill see different results. This is a prerequisite for the proxy to forward text frames as text (Node'swsdelivers them asBufferotherwise), but it is a standalone fix worth calling out in the changelog.