fix(connection): block reserved internal event types from server messages#3378
Open
ckeshava wants to merge 6 commits into
Open
fix(connection): block reserved internal event types from server messages#3378ckeshava wants to merge 6 commits into
ckeshava wants to merge 6 commits into
Conversation
…ages
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
- 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.
Contributor
|
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 as they are similar to previous changes (2)
Walkthrough
ChangesReserved internal event injection guard
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
4 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/xrpl/test/connection.test.ts (1)
912-915: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd non-string reserved-name bypass cases.
The test only covers exact string
typevalues. Add cases liketype: ['error']andtype: ['connected']so the regression suite catches payloads that bypass a string-only denylist check but still resolve to reserved listener names at emission time.🧪 Suggested test shape
- it.each(['connected', 'disconnected', 'reconnect', 'reconnecting', 'error'])( + it.each([ + ['connected', { type: 'connected', error: 'spoof' }], + ['disconnected', { type: 'disconnected', error: 'spoof' }], + ['reconnect', { type: 'reconnect', error: 'spoof' }], + ['reconnecting', { type: 'reconnecting', error: 'spoof' }], + ['error', { type: 'error', error: 'spoof' }], + ['connected', { type: ['connected'], error: 'spoof' }], + ['error', { type: ['error'], error: 'spoof' }], + ])( 'drops server message with reserved internal event type "%s"', - async (reservedType) => { - const spoofedPayload = { type: reservedType, error: 'spoof' } + async (reservedType, spoofedPayload) => {🤖 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/test/connection.test.ts` around lines 912 - 915, The test for reserved internal event types currently only covers exact string values in the it.each array. Add additional test cases with non-string types that could bypass a string-only denylist check but still resolve to reserved listener names, such as type values that are arrays containing reserved names like ['error'] and ['connected']. Extend the it.each array to include these non-string variants alongside the existing string cases, and ensure the spoofedPayload is constructed appropriately for each test variant to verify these bypass scenarios are properly prevented.
🤖 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.
Inline comments:
In `@packages/xrpl/HISTORY.md`:
- Line 36: The release note in HISTORY.md lists only four blocked event names
(connected, disconnected, error, reconnect) but the implementation blocks five
names. Add the missing `reconnecting` event name to the list in the changelog
entry to accurately reflect all the internal state events that are now blocked
from being forwarded from the server, ensuring users see the complete behavior
change.
In `@packages/xrpl/src/client/connection.ts`:
- Around line 373-387: Add a runtime type validation check for data.type before
checking against RESERVED_INTERNAL_EVENTS, since the TypeScript type assertion
does not validate runtime type. Insert a guard that checks if typeof data.type
!== 'string' before the RESERVED_INTERNAL_EVENTS.has(type) check, and if the
type is not a string, emit an 'error' event with 'badMessage' to reject
non-string event types and prevent type coercion attacks that could bypass the
reserved-event security check.
---
Nitpick comments:
In `@packages/xrpl/test/connection.test.ts`:
- Around line 912-915: The test for reserved internal event types currently only
covers exact string values in the it.each array. Add additional test cases with
non-string types that could bypass a string-only denylist check but still
resolve to reserved listener names, such as type values that are arrays
containing reserved names like ['error'] and ['connected']. Extend the it.each
array to include these non-string variants alongside the existing string cases,
and ensure the spoofedPayload is constructed appropriately for each test variant
to verify these bypass scenarios are properly prevented.
🪄 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: db3e9818-ab62-4444-b5d8-b137bf47f34a
📒 Files selected for processing (3)
packages/xrpl/HISTORY.mdpackages/xrpl/src/client/connection.tspackages/xrpl/test/connection.test.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
A non-string `type` (e.g. `["error"]`) bypassed the reserved-event Set check but still coerced to a reserved name when passed to `emit`. Validate `type` is a string before forwarding and drop non-strings as `badMessage`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
/ai-reviewer |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/xrpl/test/connection.test.ts (1)
990-998: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winReject any non-
errorinternal event emission.For
connected/disconnected/reconnect/reconnecting, the listener should never fire at all; currently the test only fails when the forwarded argument is an object. Tighten this so a regression emitting the reserved event with no args or a primitive still fails, while preserving theerror/badMessagepath.Proposed assertion tightening
clientContext.client.connection.on(coercesTo, (forwarded) => { - if (forwarded != null && typeof forwarded === 'object') { + if ( + coercesTo !== 'error' || + (forwarded != null && typeof forwarded === 'object') + ) { reject( new XrplError( `Non-string server type was forwarded as internal event "${coercesTo}"`, ), ) }🤖 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/test/connection.test.ts` around lines 990 - 998, The listener attached to the internal event in the connection test currently only rejects when the forwarded argument is a non-null object, but it should reject whenever the listener fires for the internal events connected/disconnected/reconnect/reconnecting since these should never be forwarded at all. Replace the conditional check with an unconditional reject that fires whenever this listener is invoked for these specific events, while ensuring the error and badMessage event paths are excluded from this rejection logic so they can continue to be forwarded without triggering the test failure.
🤖 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.
Inline comments:
In `@packages/xrpl/test/connection.test.ts`:
- Around line 974-979: Add the missing `reconnecting` event case to the it.each
test matrix in the connection.test.ts file. The upstream reserved-event set
includes `reconnecting`, but the current non-string coercion test matrix does
not cover it. Add a new row to the test case array following the same pattern as
the existing entries (like `['connected', ['connected']]`), ensuring that
`reconnecting` is tested alongside the other reserved internal events
(`connected`, `disconnected`, `reconnect`, and `error`) to provide complete
regression test coverage.
---
Nitpick comments:
In `@packages/xrpl/test/connection.test.ts`:
- Around line 990-998: The listener attached to the internal event in the
connection test currently only rejects when the forwarded argument is a non-null
object, but it should reject whenever the listener fires for the internal events
connected/disconnected/reconnect/reconnecting since these should never be
forwarded at all. Replace the conditional check with an unconditional reject
that fires whenever this listener is invoked for these specific events, while
ensuring the error and badMessage event paths are excluded from this rejection
logic so they can continue to be forwarded without triggering the test failure.
🪄 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: 8d9ba3a6-ea2e-4447-956a-f438ae064c89
📒 Files selected for processing (2)
packages/xrpl/src/client/connection.tspackages/xrpl/test/connection.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xrpl/src/client/connection.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Include the offending `typeof data.type` in the non-string `badMessage` error to aid debugging. - Fail fast on unexpected `error` emissions in the reserved-event test instead of waiting out the 20s timeout. Addresses review feedback on XRPLF#3378. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cybele-ripple
approved these changes
Jun 24, 2026
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
3 participants
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.
Blocks server-supplied
data.typevalues that collide withConnection's reserved internal events (connected,disconnected,error,reconnect,reconnecting). Such messages are dropped and surfaced asbadMessage; unknown types are still forwarded so forward-compatibility is preserved.Continues the work in #3340.
Fixes #3318