-
Notifications
You must be signed in to change notification settings - Fork 575
fix: Handle non-UTF-8 inputs without crashing #3268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -118,6 +118,27 @@ describe('Client subscription', function () { | |||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| it('Emits path_find from a non-Unicode mock websocket payload', async function () { | ||||||||||||||||||||||||||||||||
| await new Promise<void>((resolve) => { | ||||||||||||||||||||||||||||||||
| testContext.client.on('path_find', (path) => { | ||||||||||||||||||||||||||||||||
| assert(path.type === 'path_find') | ||||||||||||||||||||||||||||||||
| assert.strictEqual( | ||||||||||||||||||||||||||||||||
| (path as unknown as { message: string }).message, | ||||||||||||||||||||||||||||||||
| '�', | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
| resolve() | ||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| testContext.mockRippled!.socket.send( | ||||||||||||||||||||||||||||||||
| Buffer.from([ | ||||||||||||||||||||||||||||||||
| ...Buffer.from('{"type":"path_find","message":"'), | ||||||||||||||||||||||||||||||||
| 0xff, | ||||||||||||||||||||||||||||||||
| ...Buffer.from('"}'), | ||||||||||||||||||||||||||||||||
| ]), | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
|
Comment on lines
+132
to
+138
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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:
💡 Result: In the Node.js Practically:
On the receiving side, the Citations:
🏁 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, 💡 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| it('Emits validationReceived', async function () { | ||||||||||||||||||||||||||||||||
| await new Promise<void>((resolve) => { | ||||||||||||||||||||||||||||||||
| testContext.client.on('validationReceived', (path) => { | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this test,
socket.send(Buffer.from(...))will default to sending a binary WebSocket frame inws(since the payload is aBuffer). 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.