Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/xrpl/HISTORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ Subscribe to [the **xrpl-announce** mailing list](https://groups.google.com/g/xr
* Disallow the input of Authorization Credentials over insecure WebSocket connections (`ws[+unix]?://`) to prevent MITM eavesdropping of sensitive data.
* Fix incorrect `MPTAmount` field type to `string` instead of `MPTAmount`.
* Fix `Client.getServerInfo()` swallowing errors from the underlying `server_info` request, which left `client.networkID` undefined and caused `autofill()` to silently omit the `NetworkID` field — producing signed transactions valid on the wrong network (cross-network replay risk). The method now throws on request failure or when the response is missing `network_id`. ([#3321](https://github.com/XRPLF/xrpl.js/issues/3321))
* Refuse to forward server-supplied `data.type` values that collide with `Connection`'s internal state events (`connected`, `disconnected`, `error`, `reconnect`). Previously a malicious or compromised server could inject `{"type":"connected"}` (and friends) over the wire and trigger spoofed local state transitions; the message is now dropped and surfaced as a `badMessage` error. ([#3318](https://github.com/XRPLF/xrpl.js/issues/3318))

## 4.6.0 (2026-02-12)

Expand Down
31 changes: 30 additions & 1 deletion packages/xrpl/src/client/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,21 @@ const SECONDS_PER_MINUTE = 60
const TIMEOUT = 20
const CONNECTION_TIMEOUT = 5

/**
* Event names that are emitted internally by Connection to signal local
* client/socket state transitions. These must never be emitted as a result
* of a server-supplied `data.type`, otherwise a malicious or compromised
* rippled server could spoof connection state (e.g. claim the client is
* `connected` when it is not, or inject a fake `error`/`disconnected`/
* `reconnect` to drive the client into a bad state). See DGE-6740.
*/
const RESERVED_INTERNAL_EVENTS: ReadonlySet<string> = new Set([
Comment thread
kuan121 marked this conversation as resolved.
'connected',
'disconnected',
'error',
'reconnect',
])

/**
* ConnectionOptions is the configuration for the Connection class.
*/
Expand Down Expand Up @@ -354,7 +369,21 @@ export class Connection extends EventEmitter {
}
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
Comment on lines 371 to +373

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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.

Suggested change
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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@achowdhry-ripple This sounds like a legit comment

// 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.
Comment thread
kuan121 marked this conversation as resolved.
Outdated
if (RESERVED_INTERNAL_EVENTS.has(type)) {
this.emit(
'error',
'badMessage',
`Server message used reserved internal event type "${type}"; dropping.`,
message,
)
return
}
this.emit(type, data)
}
if (data.type === 'response') {
try {
Expand Down
91 changes: 91 additions & 0 deletions packages/xrpl/test/connection.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -903,6 +903,97 @@ describe('Connection', function () {
TIMEOUT,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

browser tests are failing

)

// DGE-6740: Server messages whose `type` collides with a reserved internal
Comment thread
kuan121 marked this conversation as resolved.
Outdated
// event name (`connected`, `disconnected`, `error`, `reconnect`) must not
// be forwarded as that internal event. Otherwise a rogue server could spoof
// connection state.
it.each(['connected', 'disconnected', 'reconnect'])(
'drops server message with reserved internal event type "%s"',
async (reservedType) => {
// If the fix regresses, the reserved listener will fire and we'll fail.
const spoofTrigger = new Promise<never>((_resolve, reject) => {
clientContext.client.connection.on(reservedType, () => {
reject(
new XrplError(
`Reserved internal event "${reservedType}" was emitted from a server message`,
),
)
})
})

const errorReceived = new Promise<void>((resolve) => {
clientContext.client.connection.on(
'error',
(errorCode, errorMessage) => {
if (
errorCode === 'badMessage' &&
typeof errorMessage === 'string' &&
errorMessage.includes(reservedType)
) {
resolve()
}
},
)
})

// @ts-expect-error -- Testing private member
clientContext.client.connection.onMessage(
JSON.stringify({ type: reservedType }),
)

await Promise.race([errorReceived, spoofTrigger])
},
TIMEOUT,
)

// DGE-6740: A spoofed `{"type":"error"}` payload must not be forwarded to
Comment thread
kuan121 marked this conversation as resolved.
Outdated
// 'error' listeners as the raw payload. Instead the fix emits an 'error'
// event whose first arg is the string code 'badMessage'. We verify the
// 'error' listener never receives the raw spoofed object.
it(
'drops server message with reserved internal event type "error"',
async () => {
Comment thread
kuan121 marked this conversation as resolved.
Outdated
const spoofedPayload = { type: 'error', error: 'spoof' }

const result = new Promise<void>((resolve, reject) => {
clientContext.client.connection.on(
'error',
(errorCodeOrPayload, errorMessage) => {
// The fix's emission: emit('error', 'badMessage', '<msg>', rawJson).
if (
errorCodeOrPayload === 'badMessage' &&
typeof errorMessage === 'string' &&
errorMessage.includes('error')
) {
resolve()
return
}
// Anything that looks like the spoofed payload object indicates
// the raw server message reached 'error' listeners, which is the
// exact bug being fixed.
if (
errorCodeOrPayload != null &&
typeof errorCodeOrPayload === 'object' &&
(errorCodeOrPayload as { type?: unknown }).type === 'error'
) {
reject(
new XrplError(
'Spoofed server payload was forwarded to error listeners',
),
)
}
},
)
})

// @ts-expect-error -- Testing private member
clientContext.client.connection.onMessage(JSON.stringify(spoofedPayload))

await result
},
TIMEOUT,
)

// it('should clean up websocket connection if error after websocket is opened', async function () {
// await clientContext.client.disconnect()
// // fail on connection
Expand Down
Loading