diff --git a/packages/xrpl/HISTORY.md b/packages/xrpl/HISTORY.md index b5b37ddbf1..df97c3beb3 100644 --- a/packages/xrpl/HISTORY.md +++ b/packages/xrpl/HISTORY.md @@ -33,6 +33,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`, `reconnecting`). 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) diff --git a/packages/xrpl/src/client/connection.ts b/packages/xrpl/src/client/connection.ts index 54ad7a288f..1beed84411 100644 --- a/packages/xrpl/src/client/connection.ts +++ b/packages/xrpl/src/client/connection.ts @@ -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 = new Set([ + 'connected', + 'disconnected', + 'error', + 'reconnect', + 'reconnecting', +]) + /** * ConnectionOptions is the configuration for the Connection class. */ @@ -353,8 +369,7 @@ export class Connection extends EventEmitter { return } if (data.type) { - // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- Should be true - this.emit(data.type as string, data) + this.forwardServerMessage(data, message) } if (data.type === 'response') { try { @@ -369,6 +384,52 @@ export class Connection extends EventEmitter { } } + /** + * Forwards a server message to listeners keyed by its `data.type`, after + * validating the server-controlled `type`. Messages whose `type` is not a + * string, or whose `type` collides with one of Connection's reserved + * internal event names, are dropped and surfaced as a `badMessage` error + * rather than forwarded. + * + * @param data - The parsed server message. + * @param message - The raw message string, used for diagnostics. + */ + private forwardServerMessage( + data: Record, + message: string, + ): void { + // `data.type` is server-controlled, so its runtime type must be checked + // before it is used as an event name. A compile-time `as string` is not + // enough: a payload like `{"type":["error"]}` slips past + // `RESERVED_INTERNAL_EVENTS.has(...)` (an array never matches a string in + // the Set) yet still coerces to the string "error" when passed to `emit`, + // re-opening the spoofing hole. Reject any non-string `type` outright. + if (typeof data.type !== 'string') { + this.emit( + 'error', + 'badMessage', + `Server message 'type' must be a string (got ${typeof data.type}); dropping.`, + 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"}`. + 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) + } + /** * Handler for what to do once the connection to the server is open. * diff --git a/packages/xrpl/test/connection.test.ts b/packages/xrpl/test/connection.test.ts index 085e49be7a..aca7ff2f28 100644 --- a/packages/xrpl/test/connection.test.ts +++ b/packages/xrpl/test/connection.test.ts @@ -903,6 +903,125 @@ describe('Connection', function () { TIMEOUT, ) + // 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((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', '', 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', + ), + ) + return + } + // Any other 'error' emission is unexpected — fail fast instead of + // waiting for the test to time out. + reject( + new XrplError( + `Unexpected 'error' emission: ${JSON.stringify(errorCodeOrPayload)}`, + ), + ) + }, + ) + }) + + // @ts-expect-error -- Testing private member + clientContext.client.connection.onMessage(JSON.stringify(spoofedPayload)) + + await result + }, + TIMEOUT, + ) + + // A non-string `type` must never be used as an event name. Arrays are the + // dangerous case: `{"type":["error"]}` fails the RESERVED_INTERNAL_EVENTS + // Set check (an array is never === a string) but coerces to the string + // "error" when passed to `emit`, which would otherwise re-open the spoofing + // hole the reserved-event guard closes. All non-string types are dropped and + // surfaced as `badMessage`. + it.each([ + ['connected', ['connected']], + ['disconnected', ['disconnected']], + ['reconnect', [['reconnect']]], + ['reconnecting', ['reconnecting']], + ['error', ['error']], + ])( + 'drops server message whose non-string type coerces to "%s"', + async (coercesTo, spoofedType) => { + const spoofedPayload = { type: spoofedType, error: 'spoof' } + + const result = new Promise((resolve, reject) => { + // Regression guard: the event the non-string type coerces to must never + // receive the raw server payload. Keying off the forwarded payload + // object (rather than the event merely firing) lets this same check + // cover the `error` case, where the event is also the channel the fix + // uses to report `badMessage`. + clientContext.client.connection.on(coercesTo, (forwarded) => { + if (forwarded != null && typeof forwarded === 'object') { + reject( + new XrplError( + `Non-string server type was forwarded as internal event "${coercesTo}"`, + ), + ) + } + }) + + // Success: the message is dropped and surfaced as a badMessage error. + clientContext.client.connection.on('error', (errorCode) => { + if (errorCode === 'badMessage') { + resolve() + } + }) + }) + + // @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