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 2 commits 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
|
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)
WalkthroughThis PR prevents server-supplied messages whose 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/xrpl/src/client/connection.tsParsing error: The keyword 'import' is reserved packages/xrpl/test/connection.test.tsParsing error: The keyword 'import' is reserved 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
| @@ -903,6 +903,97 @@ describe('Connection', function () { | |||
| TIMEOUT, | |||
There was a problem hiding this comment.
browser tests are failing
- Include 'reconnecting' in the reserved internal-event denylist, since Connection also emits it during automatic reconnect attempts. - Drop internal ticket reference from inline comments. - Collapse the separate type:"error" test into the parameterized it.each so all reserved types (including 'error' and 'reconnecting') share one test body. The 'error' iteration retains the extra check that the raw spoofed payload never reaches 'error' listeners.
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| if (data.type) { | ||
| // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- Should be true | ||
| this.emit(data.type as string, data) | ||
| const type = data.type as string |
There was a problem hiding this comment.
🟠 Severity: HIGH
No runtime typeof check on data.type. Since JSON.parse can produce arrays, a malicious server sending {"type":["connected"]} bypasses Set.has() (strict equality, array ≠ string) but eventemitter3's property-based lookup implicitly calls .toString(), resolving ["connected"] to "connected" — triggering the reserved internal event and fully defeating this denylist guard.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Add a runtime typeof check to ensure data.type is actually a string before using it. Replace if (data.type) with if (typeof data.type === 'string'). This prevents non-string values (e.g., arrays) from bypassing the Set.has() denylist check, since typeof will return 'object' for arrays and the block will be skipped entirely.
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| if (data.type) { | |
| // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- Should be true | |
| this.emit(data.type as string, data) | |
| const type = data.type as string | |
| if (typeof data.type === 'string') { | |
| const type = data.type |
There was a problem hiding this comment.
@achowdhry-ripple This sounds like a legit comment
2 participants
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.