Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughWebSocket handling now tolerates malformed UTF-8 by enabling Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Pull request overview
This PR prevents xrpl’s WebSocket connection layer from crashing when rippled sends malformed UTF-8 in text frames by disabling UTF-8 validation at the WebSocket layer and decoding any Buffer message payloads as UTF-8 (allowing replacement characters). It also updates the test mock to emit raw WebSocket frames and adds regression tests.
Changes:
- Enable
skipUTF8Validationfor WebSocket connections and decodeBuffermessage payloads to UTF-8 strings before JSON parsing. - Extend the mock rippled server to optionally send raw WebSocket frames (including malformed UTF-8) and route responses through a unified sender.
- Add tests covering malformed UTF-8 handling for requests and (attempted) subscription events; update
HISTORY.md.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/xrpl/src/client/connection.ts | Enables skip UTF-8 validation and decodes Buffer messages to avoid crashes on malformed UTF-8. |
| packages/xrpl/test/createMockRippled.ts | Adds support for sending raw frames (Buffer/string) from the mock server for malformed UTF-8 tests. |
| packages/xrpl/test/connection.test.ts | Adds a regression test for malformed UTF-8 in a text WebSocket response. |
| packages/xrpl/test/client/subscribe.test.ts | Adds a subscription-event test intended to cover non-Unicode payloads. |
| packages/xrpl/test/client/submitAndWait.test.ts | Adds a regression test for malformed UTF-8 while polling tx during submitAndWait. |
| packages/xrpl/HISTORY.md | Documents the fix in Unreleased notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ...Buffer.from('{"type":"path_find","message":"'), | ||
| 0xff, | ||
| ...Buffer.from('"}'), | ||
| ]), |
There was a problem hiding this comment.
In this test, socket.send(Buffer.from(...)) will default to sending a binary WebSocket frame in ws (since the payload is a Buffer). That means it doesn't exercise malformed UTF-8 handling for text frames (the UTF-8 validation logic only applies to text messages). Consider sending with { binary: false } (or using the new RawMockFrame support in the mock server) so this test actually covers malformed UTF-8 text messages.
| ]), | |
| ]), | |
| { binary: false }, |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/xrpl/test/client/subscribe.test.ts`:
- Around line 132-138: The test is sending the malformed payload as a binary
WebSocket frame (testContext.mockRippled!.socket.send(Buffer...)) but real
rippled sends JSON as text frames; update the send call to send a text frame by
passing the options object { binary: false } (or send the JSON string directly)
to testContext.mockRippled!.socket.send so the mocked response is delivered as a
text frame matching real rippled behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1526bdbf-f3b9-47df-ad87-7da6fe1939b4
📒 Files selected for processing (6)
packages/xrpl/HISTORY.mdpackages/xrpl/src/client/connection.tspackages/xrpl/test/client/submitAndWait.test.tspackages/xrpl/test/client/subscribe.test.tspackages/xrpl/test/connection.test.tspackages/xrpl/test/createMockRippled.ts
| testContext.mockRippled!.socket.send( | ||
| Buffer.from([ | ||
| ...Buffer.from('{"type":"path_find","message":"'), | ||
| 0xff, | ||
| ...Buffer.from('"}'), | ||
| ]), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: locate Buffer-based websocket sends in tests.
rg -n "mockRippled!?\\.socket\\.send\\(|\\.send\\(\\s*Buffer\\.from\\(" packages/xrpl/test -C2Repository: XRPLF/xrpl.js
Length of output: 433
🏁 Script executed:
# Get the full test case to understand what it's testing
sed -n '121,140p' packages/xrpl/test/client/subscribe.test.tsRepository: XRPLF/xrpl.js
Length of output: 628
🏁 Script executed:
# Check ws library version and documentation about send options
cat packages/xrpl/package.json | grep -A2 -B2 '"ws"'Repository: XRPLF/xrpl.js
Length of output: 236
🌐 Web query:
ws npm library socket.send Buffer binary frame default behavior
💡 Result:
In the Node.js ws npm library, socket.send(data[, options][, cb]) decides whether to send a text frame (opcode 0x1) or a binary frame (opcode 0x2) via the options.binary flag, whose default is “autodetected” based on the type of data you pass. (github.com)
Practically:
socket.send(Buffer.from(...))(or other binary-like types such asArrayBuffer,TypedArray,DataView) will be sent as binary by default (autodetect). (github.com)socket.send("...")will be sent as text by default (autodetect). (github.com)- You can override the autodetect with
socket.send(data, { binary: true/false }). (github.com)
On the receiving side, the 'message' event provides isBinary so you can tell which kind of message arrived. (github.com)
Citations:
- 1: https://github.com/websockets/ws/blob/master/doc/ws.md
- 2: https://github.com/websockets/ws/blob/master/doc/ws.md
- 3: https://github.com/websockets/ws/blob/master/doc/ws.md
- 4: https://github.com/websockets/ws/blob/master/doc/ws.md
- 5: https://github.com/websockets/ws/blob/master/doc/ws.md
🏁 Script executed:
# Check for other socket.send calls in the test file to see patterns
grep -n "socket.send" packages/xrpl/test/client/subscribe.test.tsRepository: XRPLF/xrpl.js
Length of output: 104
🏁 Script executed:
# Check how the client handles the 'message' event - does it care about frame type?
rg -n "on\(['\"]message['\"]|isBinary" packages/xrpl/src -C3Repository: XRPLF/xrpl.js
Length of output: 681
🏁 Script executed:
# Check what onMessage does with the message - does it parse differently based on type?
sed -n '395,450p' packages/xrpl/src/client/connection.tsRepository: XRPLF/xrpl.js
Length of output: 2215
🏁 Script executed:
# Search for the onMessage method implementation
rg -n "onMessage\s*\(" packages/xrpl/src/client/connection.ts -A15Repository: XRPLF/xrpl.js
Length of output: 1377
Send this malformed payload explicitly as a text frame.
At line 132, socket.send(Buffer) in the ws library defaults to a binary frame. Real rippled sends JSON responses as text frames, so pass { binary: false } to match that behavior.
💡 Suggested patch
- testContext.mockRippled!.socket.send(
- Buffer.from([
- ...Buffer.from('{"type":"path_find","message":"'),
- 0xff,
- ...Buffer.from('"}'),
- ]),
- )
+ testContext.mockRippled!.socket.send(
+ Buffer.from([
+ ...Buffer.from('{"type":"path_find","message":"'),
+ 0xff,
+ ...Buffer.from('"}'),
+ ]),
+ { binary: false },
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| testContext.mockRippled!.socket.send( | |
| Buffer.from([ | |
| ...Buffer.from('{"type":"path_find","message":"'), | |
| 0xff, | |
| ...Buffer.from('"}'), | |
| ]), | |
| ) | |
| testContext.mockRippled!.socket.send( | |
| Buffer.from([ | |
| ...Buffer.from('{"type":"path_find","message":"'), | |
| 0xff, | |
| ...Buffer.from('"}'), | |
| ]), | |
| { binary: false }, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/xrpl/test/client/subscribe.test.ts` around lines 132 - 138, The test
is sending the malformed payload as a binary WebSocket frame
(testContext.mockRippled!.socket.send(Buffer...)) but real rippled sends JSON as
text frames; update the send call to send a text frame by passing the options
object { binary: false } (or send the JSON string directly) to
testContext.mockRippled!.socket.send so the mocked response is delivered as a
text frame matching real rippled behavior.
High Level Overview of Change
This PR adds better handling for non-UTF-8 inputs from rippled. Previously, xrpl.js would crash when it encountered these inputs.
Context of Change
Transia-RnD#2
Type of Change
Did you update HISTORY.md?
Test Plan
Added tests. It no longer crashes.