fix(connection): block reserved internal event types from server messages#3340
fix(connection): block reserved internal event types from server messages#3340achowdhry-ripple wants to merge 1 commit into
Conversation
…ages (DGE-6740)
A malicious or compromised rippled server could previously send
`{"type":"connected"}`, `{"type":"disconnected"}`, `{"type":"error"}`,
or `{"type":"reconnect"}` and have `Connection.onMessage` forward those
events to internal Connection/Client listeners — spoofing connection
state, faking errors, or triggering reconnect logic.
`onMessage` now refuses to forward server-supplied `data.type` values
that collide with these reserved internal event names. Such messages
are dropped and surfaced as a `badMessage` error. Unknown server event
types (e.g. future rippled stream types) continue to be forwarded so
forward-compatibility is preserved.
Fixes XRPLF#3318
WalkthroughThis PR fixes a critical server-supplied event injection vulnerability by preventing a rogue or compromised server from emitting spoofed internal connection state events. The client now maintains a reserved events list and filters all server-supplied message types against it, rejecting collisions and emitting a badMessage error instead. ChangesServer-Supplied Event Injection Prevention
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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)
packages/xrpl/src/client/connection.ts (2)
370-386: ⚡ Quick winAdd type validation for
data.typebefore checking reserved events.The current code uses a type assertion (
data.type as string) without runtime validation. While JSON-parsed messages will typically have string types, a malicious or malformed message could havedata.typeas a number, boolean, array, or object. For example,{"type": 123}would bypass the reserved-event check and emit event"123". Although non-string types won't collide with reserved event names (so this isn't a security bypass), adding explicit string validation improves robustness and makes the code's assumptions explicit.Suggested validation
if (data.type) { - const type = data.type as string + if (typeof data.type !== 'string') { + this.emit('error', 'badMessage', 'data.type must be a string', message) + return + } + const type = data.type // Refuse to forward server-supplied event names that collide with // Connection's internal state events. Otherwise a rogue server could // spoof local connection state by sending e.g. `{"type":"connected"}` // or `{"type":"error"}`. See DGE-6740. if (RESERVED_INTERNAL_EVENTS.has(type)) {🤖 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 `@packages/xrpl/src/client/connection.ts` around lines 370 - 386, The code assumes data.type is a string by asserting `data.type as string`; instead perform a runtime check on `data.type` before using it: verify typeof data.type === 'string' and only then assign to a local `type` variable and run the `RESERVED_INTERNAL_EVENTS.has(type)` check and `this.emit(type, data)`; if `data.type` is missing or not a string, call `this.emit('error', 'badMessage', 'Invalid or non-string message type; dropping.', message)` (or similar error handling used elsewhere) and return to avoid emitting non-string event names.
33-37: ⚡ Quick winConsider adding 'reconnecting' to the reserved events list.
Line 488 shows that
'reconnecting'is also emitted internally during automatic reconnection attempts. While less critical than the four reserved events (which directly affect connection state), a malicious server could send{"type":"reconnecting"}to spoof automatic reconnection behavior and confuse monitoring code. For consistency and defense in depth, consider adding it to the reserved set.Suggested addition
const RESERVED_INTERNAL_EVENTS: ReadonlySet<string> = new Set([ 'connected', 'disconnected', 'error', 'reconnect', + 'reconnecting', ])🤖 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 `@packages/xrpl/src/client/connection.ts` around lines 33 - 37, The RESERVED_INTERNAL_EVENTS set (RESERVED_INTERNAL_EVENTS) currently protects internal event names but omits the internally-emitted 'reconnecting' event; add the string 'reconnecting' to that ReadonlySet so external messages cannot spoof the client's internal reconnection signal (ensure the literal 'reconnecting' is added alongside 'connected','disconnected','error','reconnect' in the RESERVED_INTERNAL_EVENTS declaration).
🤖 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.
Nitpick comments:
In `@packages/xrpl/src/client/connection.ts`:
- Around line 370-386: The code assumes data.type is a string by asserting
`data.type as string`; instead perform a runtime check on `data.type` before
using it: verify typeof data.type === 'string' and only then assign to a local
`type` variable and run the `RESERVED_INTERNAL_EVENTS.has(type)` check and
`this.emit(type, data)`; if `data.type` is missing or not a string, call
`this.emit('error', 'badMessage', 'Invalid or non-string message type;
dropping.', message)` (or similar error handling used elsewhere) and return to
avoid emitting non-string event names.
- Around line 33-37: The RESERVED_INTERNAL_EVENTS set (RESERVED_INTERNAL_EVENTS)
currently protects internal event names but omits the internally-emitted
'reconnecting' event; add the string 'reconnecting' to that ReadonlySet so
external messages cannot spoof the client's internal reconnection signal (ensure
the literal 'reconnecting' is added alongside
'connected','disconnected','error','reconnect' in the RESERVED_INTERNAL_EVENTS
declaration).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 98c777fd-3f48-4e0c-a3c6-2d1ecb312d4e
📒 Files selected for processing (3)
packages/xrpl/HISTORY.mdpackages/xrpl/src/client/connection.tspackages/xrpl/test/connection.test.ts
1 participant
Summary
Closes #3318
Connection.onMessagepreviously calledthis.emit(data.type as string, data)with zero validation ofdata.type. A malicious or compromised rippled server could therefore send{"type":"connected"},{"type":"disconnected"},{"type":"error"}, or{"type":"reconnect"}and triggerConnection's internal state-event listeners (registered inClient), spoofing the local connection state, faking errors, or driving reconnect logic.This PR introduces a denylist of reserved internal event names and refuses to forward server messages whose
typecollides with any of them. Such messages are dropped and surfaced via the existing('error', 'badMessage', ...)channel so they remain observable.A denylist (rather than the whitelist suggested in the issue) was chosen deliberately to preserve the existing "unrecognized message type" forward-compatibility contract in
packages/xrpl/test/connection.test.ts. Unknown future rippled stream types continue to be forwarded as before; only reserved local event names are blocked.Changes
packages/xrpl/src/client/connection.ts: addRESERVED_INTERNAL_EVENTSset and guard the emit inonMessageagainst it.packages/xrpl/test/connection.test.ts: add unit tests covering all four reserved event names, including the subtletype: "error"case (verifies the raw payload object never reacheserrorlisteners).packages/xrpl/HISTORY.md: changelog entry under Unreleased -> Fixed.Test plan
cd packages/xrpl && npm test -- connection.test.ts- new "drops server message with reserved internal event type" cases pass.npm run lintinpackages/xrplclean.