Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
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
32 changes: 31 additions & 1 deletion packages/xrpl/src/client/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,22 @@ 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`/`reconnecting` to drive the client into a bad state).
*/
const RESERVED_INTERNAL_EVENTS: ReadonlySet<string> = new Set([
Comment thread
kuan121 marked this conversation as resolved.
'connected',
'disconnected',
'error',
'reconnect',
'reconnecting',
])

/**
* ConnectionOptions is the configuration for the Connection class.
*/
Expand Down Expand Up @@ -354,7 +370,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"}`.
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
62 changes: 62 additions & 0 deletions packages/xrpl/test/connection.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -903,6 +903,68 @@ 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

)

// Server messages whose `type` collides with a reserved internal event
// name (`connected`, `disconnected`, `error`, `reconnect`, `reconnecting`)
// must not be forwarded as that internal event. Otherwise a rogue server
// could spoof connection state. For `error` specifically, we also verify
// the raw spoofed payload object never reaches 'error' listeners — only
// the synthesized `('error', 'badMessage', ...)` emission is allowed.
it.each(['connected', 'disconnected', 'reconnect', 'reconnecting', 'error'])(
'drops server message with reserved internal event type "%s"',
async (reservedType) => {
const spoofedPayload = { type: reservedType, error: 'spoof' }

const result = new Promise<void>((resolve, reject) => {
// For non-'error' reserved events, a direct listener on that event
// must never fire from a server message. (We can't attach this for
// 'error' because the fix itself emits 'error' to surface badMessage.)
if (reservedType !== 'error') {
clientContext.client.connection.on(reservedType, () => {
reject(
new XrplError(
`Reserved internal event "${reservedType}" was emitted from a server message`,
),
)
})
}

clientContext.client.connection.on(
'error',
(errorCodeOrPayload, errorMessage) => {
// The fix's emission: emit('error', 'badMessage', '<msg>', rawJson).
if (
errorCodeOrPayload === 'badMessage' &&
typeof errorMessage === 'string' &&
errorMessage.includes(reservedType)
) {
resolve()
return
}
// If the raw spoofed payload object reaches 'error' listeners,
// the type:"error" spoof bug has regressed.
if (
errorCodeOrPayload != null &&
typeof errorCodeOrPayload === 'object' &&
(errorCodeOrPayload as { type?: unknown }).type === reservedType
) {
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