From 943a287f23905b0005c7bad999c335f45f43bd8f Mon Sep 17 00:00:00 2001 From: morizon Date: Wed, 19 Aug 2026 23:56:32 +0800 Subject: [PATCH 1/5] fix: require room membership before relaying client-to-client messages `socket.to(roomId).emit()` is a delivery operator: it resolves the membership of the *recipients* and never checks the sender's. The two c2c relay paths passed a client-supplied roomId straight into it, so any connected socket that knew a roomId could inject client-to-client calls into a room it had never joined - without occupying a room slot, without triggering the `room-full` alert, and invisibly to both real devices. That bypasses the invariant the pairing flow relies on: influencing a pairing session should require taking one of the room's two slots, which is precisely what makes it observable to the user (the real device can no longer join, so they re-pair with a fresh room). Reproduced against the built server: with A and B paired (2/2, so join is refused with CONNECTION_REJECTED and the member list holds only A and B), a third socket that never joined still delivered `cancelTransfer` to both. Membership is authoritative in RoomManager and already enforced by leaveRoom, getRoomUsers and startTransfer; the relay path simply never consulted it, because JsBridgeBase is a transport abstraction with no notion of rooms and the bridge was not given the RoomManager. Hand it the RoomManager and check the sender in checkC2cEnvelope, which both the request and the response path share. Impact was availability only - never confidentiality. Payloads are encrypted with a key derived on the clients from the pairing code and an ECDHE exchange, so an injected payload cannot be decrypted by a peer and real traffic cannot be read by an injector. Also documents that the server-generated room key is not part of the end-to-end encryption scheme: the client never consumes it and this server never encrypts with it. Reviewers have read it as key escrow, so the intent is now stated at each site it surfaces. --- .../transfer-server/src/JsBridgeE2EEServer.ts | 19 ++++++++++++++++++- packages/transfer-server/src/e2eeServerApi.ts | 1 + packages/transfer-server/src/roomManager.ts | 16 ++++++++++++++++ packages/transfer-server/src/types.ts | 5 +++++ 4 files changed, 40 insertions(+), 1 deletion(-) diff --git a/packages/transfer-server/src/JsBridgeE2EEServer.ts b/packages/transfer-server/src/JsBridgeE2EEServer.ts index 69492b7..e2d5766 100644 --- a/packages/transfer-server/src/JsBridgeE2EEServer.ts +++ b/packages/transfer-server/src/JsBridgeE2EEServer.ts @@ -10,6 +10,7 @@ import type { IJsBridgeMessagePayload, IJsonRpcRequest, } from '@onekeyfe/cross-inpage-provider-types'; +import type { RoomManager } from './roomManager'; import type { Socket } from 'socket.io'; const logger = createModuleLogger('jsBridge'); @@ -94,15 +95,21 @@ function checkBridgePayload( export class JsBridgeE2EEServer extends JsBridgeBase { constructor( config: IJsBridgeConfig, - { socketClient }: { socketClient: Socket }, + { + socketClient, + roomManager, + }: { socketClient: Socket; roomManager: RoomManager }, ) { super(config); this.socketClient = socketClient; + this.roomManager = roomManager; this.setup(); } private socketClient: Socket; + private roomManager: RoomManager; + /** * Rate limit state, scoped to this connection rather than kept in a * module-level map that lived for the lifetime of the process. @@ -430,6 +437,16 @@ export class JsBridgeE2EEServer extends JsBridgeBase { return undefined; } + // `socket.to(roomId)` is a delivery operator: it reads the membership of the + // recipients and never checks the sender's. Without this, any connected + // socket that knows a roomId can inject client-to-client calls into a room + // it never joined - bypassing the room-slot invariant the pairing flow + // relies on. Membership is authoritative in RoomManager, so ask it. + if (!this.roomManager.isUserInRoom(roomId, this.socketClient.id).isInRoom) { + this.logInvalidPayload(eventName, payload, 'sender is not a room member'); + return undefined; + } + return { payload: checked.payload, roomId }; } } diff --git a/packages/transfer-server/src/e2eeServerApi.ts b/packages/transfer-server/src/e2eeServerApi.ts index 32815a0..604a367 100644 --- a/packages/transfer-server/src/e2eeServerApi.ts +++ b/packages/transfer-server/src/e2eeServerApi.ts @@ -70,6 +70,7 @@ function createBridgeE2EEServer({ }, { socketClient, + roomManager, }, ); } diff --git a/packages/transfer-server/src/roomManager.ts b/packages/transfer-server/src/roomManager.ts index 508cd03..ef421ef 100644 --- a/packages/transfer-server/src/roomManager.ts +++ b/packages/transfer-server/src/roomManager.ts @@ -46,6 +46,17 @@ export class RoomManager { /** * Create new room * @returns Room information (room ID and encryption key) + * + * NOTE on `encryptionKey` (returned here and as `roomKey` from joinRoom): + * this key is NOT used by the OneKey client, and it is not what protects + * transferred data. The client derives its own end-to-end key locally from + * material this server never sees - the pairing code shown in the QR code + * (only its roomId prefix ever reaches the server), an ECDHE shared secret + * negotiated directly between the two devices, and the room's user list. + * A malicious or compromised server therefore still cannot decrypt anything, + * because it holds none of that material. This server never encrypts or + * decrypts payloads with this key either - it only relays them. The field is + * kept for wire compatibility with existing clients. */ @e2eeApiMethod() async createRoom(): Promise<{ roomId: string; encryptionKey: string }> { @@ -101,6 +112,11 @@ export class RoomManager { * @param encryptionKey Encryption key * @param socketId User's Socket ID * @returns Join result + * + * The returned `roomKey` is server-generated and unused by the client - see + * the note on createRoom(). It is not part of the end-to-end encryption + * scheme; the client derives its own key from the pairing code and an ECDHE + * exchange this server is not party to. */ @e2eeApiMethod() async joinRoom( diff --git a/packages/transfer-server/src/types.ts b/packages/transfer-server/src/types.ts index 9c2a21f..e5d3d9e 100644 --- a/packages/transfer-server/src/types.ts +++ b/packages/transfer-server/src/types.ts @@ -48,6 +48,11 @@ export interface ISocketData { // Room data structure export interface IRoom { id: string; + // Server-generated key handed to clients on create/join. The OneKey client + // does not consume it, and this server never encrypts with it - payloads are + // relayed as-is. Real end-to-end protection comes from a key the clients + // derive themselves (pairing code + ECDHE shared secret + room user list), + // none of which this server holds. See RoomManager.createRoom(). encryptionKey: string; users: Map; transferDirection?: From 8d6a7787c61556cbedafb213591c45a96ec6b4e6 Mon Sep 17 00:00:00 2001 From: morizon Date: Thu, 20 Aug 2026 00:07:45 +0800 Subject: [PATCH 2/5] fix: stop getRoomUsers from leaking room existence to non-members getRoomUsers answered differently depending on whether a room existed: a missing room returned [], while a room the caller was not in threw. Any connected socket could tell the two apart, which turns the method into an unauthenticated room existence oracle. It is also on the rate limit whitelist, so it could be probed at line speed - measured at ~1250 probes/s on a single connection, roughly 3700x cheaper than probing via joinRoom, which is rate limited and costs a room slot. isUserInRoom() already reports false for a missing room, so both cases now fail through the same branch with an identical code and message. Room existence is no longer observable to a non-member. The error is ROOM_NOT_FOUND rather than SOCKET_NOT_IN_ROOM because clients map error codes, not messages: shipped clients already translate ROOM_NOT_FOUND into a localized "invalid pairing code" prompt, whereas SOCKET_NOT_IN_ROOM falls through to a raw English string in a toast. Compatibility with shipped clients was checked call site by call site. A caller that is in the room still gets the same response as before, and a member's room always exists (a room is only deleted when its last member leaves), so the changed branches are reachable only once the caller is no longer a member - in practice only after the 1 hour room timeout. There, app UI previously rendered a silently empty peer list and now surfaces a localized error, and the CLI's 1s room-users poll already ignores failures (transfer-receiver-adapter.ts), so its pairing loop is unaffected. Note this does not make room ids unobservable in general: joinRoom still distinguishes ROOM_NOT_FOUND from CONNECTION_REJECTED. That path stays rate limited, consumes a room slot and raises `room-full` on the paired devices, so probing through it remains slow and visible by design. --- packages/transfer-server/src/roomManager.ts | 26 ++++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/packages/transfer-server/src/roomManager.ts b/packages/transfer-server/src/roomManager.ts index ef421ef..13f344c 100644 --- a/packages/transfer-server/src/roomManager.ts +++ b/packages/transfer-server/src/roomManager.ts @@ -326,18 +326,26 @@ export class RoomManager { ); } logger.debug({ roomId }, "room.getRoomUsers"); - const room = this.rooms.get(roomId); - if (!room) { - logger.debug({ roomId }, "room.getRoomUsersNotFound"); - return []; - } - // Validate that the socket is in the room + + // A room the caller cannot see must be indistinguishable from a room that + // does not exist. Returning [] for a missing room while throwing for a room + // the caller is not in turned this into a room existence oracle - and this + // method is exempt from rate limiting, so it could be probed at line speed. + // isUserInRoom() already reports false for a missing room, so both cases + // fail here identically, with the same error code and message. const socketValidation = this.isUserInRoom(roomId, context.socketClient.id); if (!socketValidation.isInRoom) { - throw new E2eeError( - E2eeErrorCode.SOCKET_NOT_IN_ROOM, - "Socket must be in the room to set transfer direction" + logger.debug( + { roomId, roomExists: this.rooms.has(roomId) }, + "room.getRoomUsersDenied" ); + throw new E2eeError(E2eeErrorCode.ROOM_NOT_FOUND, "Room not found"); + } + + // isInRoom implies the room exists. + const room = this.rooms.get(roomId); + if (!room) { + throw new E2eeError(E2eeErrorCode.ROOM_NOT_FOUND, "Room not found"); } const users: IE2EESocketUserInfo[] = sortBy( From cbea7190c18e4780a532a4e98e20419dbdcf6bec Mon Sep 17 00:00:00 2001 From: morizon Date: Thu, 20 Aug 2026 00:24:40 +0800 Subject: [PATCH 3/5] fix: rate limit getRoomUsers instead of exempting it getRoomUsers was on the rate limit whitelist, so it could be called at line speed - measured at ~1250 calls/s on one connection. It was exempt because the CLI polls it once per second while pairing, and the default 3s window would reject two of every three polls. Give it its own 800ms window instead. The CLI's 1s poll is unaffected and the ceiling drops to ~1.25 calls/s per connection, three orders of magnitude lower. The window has to sit meaningfully below the caller's polling interval. setInterval fixes the interval at which requests are *sent*; network latency shifts each request by roughly the same amount, so it cancels out of the gap the server observes, and only jitter moves that gap - in both directions. A 1000ms window against a 1000ms poll therefore sits exactly on the threshold rather than safely above it: measured over localhost, where latency is under a millisecond and stable, timer drift alone still pushed one poll in 30 below 1000ms and into a rejection. At 800ms the same run passes 30 of 30. pruneRateLimitState() now expires each entry against its own window rather than the default. It only ever drops entries whose window has passed, so a per-method window longer than the default would otherwise be reclaimed while still live - which is precisely the rate limit reset that function exists to prevent. --- .../transfer-server/src/JsBridgeE2EEServer.ts | 33 +++++++++++++++++-- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/packages/transfer-server/src/JsBridgeE2EEServer.ts b/packages/transfer-server/src/JsBridgeE2EEServer.ts index e2d5766..ea6118d 100644 --- a/packages/transfer-server/src/JsBridgeE2EEServer.ts +++ b/packages/transfer-server/src/JsBridgeE2EEServer.ts @@ -47,11 +47,29 @@ const CLIENT_TO_CLIENT_RATE_LIMIT_ERROR_CODE = -387_155_488; // Rate limiting whitelist - methods that are exempt from rate limiting const RATE_LIMIT_WHITELIST = new Set([ 'changeTransferDirection', - 'getRoomUsers', 'leaveRoom', 'cancelTransfer', ]); +/** + * Per-method rate limit windows, overriding RATE_LIMIT_INTERVAL_MS. + * + * getRoomUsers is polled by the CLI once per second while pairing, so the + * default 3s window would reject two of every three polls. It was previously + * exempt from rate limiting altogether, which let it be called at line speed. + * + * The window must stay meaningfully below the caller's polling interval: + * setInterval fixes the interval at which requests are *sent*, and network + * latency shifts every request by roughly the same amount, so it cancels out + * of the gap the server sees. Only jitter moves that gap, in both directions. + * A 1000ms window against a 1000ms poll therefore sits exactly on the + * threshold - measured over localhost, where latency is under a millisecond + * and stable, timer drift alone still pushed one poll in 30 below it. + */ +const METHOD_RATE_LIMIT_INTERVAL_MS = new Map([ + ['getRoomUsers', 800], +]); + const SUPPORTED_MESSAGE_TYPES: ReadonlySet = new Set([ IJsBridgeMessageTypes.REQUEST, IJsBridgeMessageTypes.RESPONSE, @@ -264,8 +282,10 @@ export class JsBridgeE2EEServer extends JsBridgeBase { const now = Date.now(); const lastTime = this.rateLimitState.get(rateLimitKey); + const interval = + METHOD_RATE_LIMIT_INTERVAL_MS.get(method) ?? RATE_LIMIT_INTERVAL_MS; - if (lastTime !== undefined && now - lastTime < RATE_LIMIT_INTERVAL_MS) { + if (lastTime !== undefined && now - lastTime < interval) { sendErrorResponse(); return true; } @@ -305,7 +325,14 @@ export class JsBridgeE2EEServer extends JsBridgeBase { */ private pruneRateLimitState(now: number): void { for (const [key, time] of this.rateLimitState) { - if (now - time >= RATE_LIMIT_INTERVAL_MS) { + // Expire each entry against its own window, not the default one: a + // per-method window longer than the default would otherwise be dropped + // while still live, which is exactly the rate limit reset this function + // is written to prevent. + const method = key.slice(key.indexOf(':') + 1); + const interval = + METHOD_RATE_LIMIT_INTERVAL_MS.get(method) ?? RATE_LIMIT_INTERVAL_MS; + if (now - time >= interval) { this.rateLimitState.delete(key); } } From 912bdc98048d6d8932a639b0290e2d354e4c629f Mon Sep 17 00:00:00 2001 From: morizon Date: Thu, 20 Aug 2026 00:34:52 +0800 Subject: [PATCH 4/5] feat: emit user-joined so peers stop polling for new members The server emitted room-full, user-left and start-transfer, but never user-joined - despite it being declared in IServerToClientEvents since the beginning. A peer that needed to know someone had entered the room had no push to wait on, so the CLI polls getRoomUsers once per second throughout pairing. That poll is the sole reason getRoomUsers needs a rate limit window as loose as 800ms. The app never needed the event: its pairing is passive, and the joining side immediately calls verifyPairingCode over the c2c channel, so the arrival of that call already means "the peer is here". Only a peer that wants to react before pairing verification completes - the CLI, which shows a "device connected, verifying" step - has to ask. Emitted from the joining socket rather than the server, so the joiner is excluded, and only after join() resolves, so a receiver that immediately calls getRoomUsers sees the membership the event describes. The already-in-room early return does not emit: no new member appeared. Shipped clients listen for none of this; Socket.IO drops events with no handler, so this is inert until the CLI is changed to consume it. The 800ms window on getRoomUsers therefore has to stay until the CLI's poll is actually gone. Also corrects the user-joined / user-left declarations to include roomId. user-left has always been emitted with it - a client multiplexes several rooms over one socket and needs it to tell which room an event refers to - and the app's handler already reads it. Declaration only; no behavior change. --- packages/transfer-server/src/roomManager.ts | 14 ++++++++++++++ packages/transfer-server/src/types.ts | 15 +++++++++++++-- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/packages/transfer-server/src/roomManager.ts b/packages/transfer-server/src/roomManager.ts index 13f344c..88617b5 100644 --- a/packages/transfer-server/src/roomManager.ts +++ b/packages/transfer-server/src/roomManager.ts @@ -202,6 +202,20 @@ export class RoomManager { await context?.socketClient.join(roomId); + // Tell the members already in the room that someone joined. The server + // never pushed this before, so a peer that needed to know had to poll + // getRoomUsers instead - which is why that method carries a high call rate. + // + // Emitted from the joining socket rather than the server, so the joiner is + // excluded (it does not need to be told about itself), and only after + // join() resolves, so the membership the event describes is already in + // effect if a receiver immediately calls getRoomUsers. + context?.socketClient.to(roomId).emit("user-joined", { + roomId, + userId, + userCount: room.users.size, + }); + return { success: true, userId, diff --git a/packages/transfer-server/src/types.ts b/packages/transfer-server/src/types.ts index e5d3d9e..0baadf3 100644 --- a/packages/transfer-server/src/types.ts +++ b/packages/transfer-server/src/types.ts @@ -8,8 +8,19 @@ export { E2eeError, E2eeErrorCode } from './errors'; export interface IServerToClientEvents { 'room-created': (data: { roomId: string; encryptionKey: string }) => void; 'room-joined': (data: { roomId: string; userId: string }) => void; - 'user-joined': (data: { userId: string; userCount: number }) => void; - 'user-left': (data: { userId: string; userCount: number }) => void; + // roomId is part of both payloads: a client multiplexes every room over one + // socket, so it needs to tell which room an event refers to. `user-left` has + // always been emitted with it - the declaration was simply out of date. + 'user-joined': (data: { + roomId: string; + userId: string; + userCount: number; + }) => void; + 'user-left': (data: { + roomId: string; + userId: string; + userCount: number; + }) => void; 'encrypted-data': (data: { encryptedData: string; senderId: string; From b0bda666da8f95aad3342fa22e5db38a429511b5 Mon Sep 17 00:00:00 2001 From: morizon Date: Thu, 20 Aug 2026 00:43:07 +0800 Subject: [PATCH 5/5] docs: state the contract for the per-method rate limit table The table is for legitimate high-frequency callers a shipped client already depends on, not an opt-out from rate limiting. Absent methods fall back to RATE_LIMIT_INTERVAL_MS, and that default is deliberate: a newly added method is limited without anyone having to remember to limit it. Without this stated, the table reads like a quieter whitelist, and the fail-safe it preserves is easy to erode one entry at a time. Adding an entry now requires naming the caller, its frequency, and why the default cannot serve it. Entries are compatibility shims with a lifetime, not permanent exemptions - the getRoomUsers entry records that it goes away with the CLI's poll once that consumes user-joined. --- .../transfer-server/src/JsBridgeE2EEServer.ts | 34 +++++++++++++------ 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/packages/transfer-server/src/JsBridgeE2EEServer.ts b/packages/transfer-server/src/JsBridgeE2EEServer.ts index ea6118d..ec4d942 100644 --- a/packages/transfer-server/src/JsBridgeE2EEServer.ts +++ b/packages/transfer-server/src/JsBridgeE2EEServer.ts @@ -54,19 +54,33 @@ const RATE_LIMIT_WHITELIST = new Set([ /** * Per-method rate limit windows, overriding RATE_LIMIT_INTERVAL_MS. * - * getRoomUsers is polled by the CLI once per second while pairing, so the - * default 3s window would reject two of every three polls. It was previously - * exempt from rate limiting altogether, which let it be called at line speed. + * This table is for legitimate high-frequency callers that a shipped client + * already depends on. It is not a way to opt out of rate limiting: a method + * absent from here is limited at RATE_LIMIT_INTERVAL_MS, and that default is + * the point - a newly added method is protected without anyone having to + * remember to protect it. * - * The window must stay meaningfully below the caller's polling interval: - * setInterval fixes the interval at which requests are *sent*, and network - * latency shifts every request by roughly the same amount, so it cancels out - * of the gap the server sees. Only jitter moves that gap, in both directions. - * A 1000ms window against a 1000ms poll therefore sits exactly on the - * threshold - measured over localhost, where latency is under a millisecond - * and stable, timer drift alone still pushed one poll in 30 below it. + * Adding an entry means stating, on that entry, which caller needs it, at what + * frequency, and why the default window cannot serve it. An entry is a + * compatibility shim and has a lifetime: remove it once its caller no longer + * needs it, rather than leaving a permanent hole behind. + * + * A window also has to stay meaningfully below the caller's polling interval. + * setInterval fixes the interval at which requests are *sent*; network latency + * shifts every request by roughly the same amount, so it cancels out of the gap + * the server observes, and only jitter moves that gap - in both directions. A + * 1000ms window against a 1000ms poll therefore sits exactly on the threshold + * rather than safely above it: measured over localhost, where latency is under + * a millisecond and stable, timer drift alone still pushed one poll in 30 below + * it and into a rejection. */ const METHOD_RATE_LIMIT_INTERVAL_MS = new Map([ + // CLI, once per second for the whole pairing phase + // (ROOM_USERS_POLL_INTERVAL_MS in transfer-receiver-adapter.ts): it had no + // user-joined push to wait on, so it polls to notice a peer arriving. The + // default 3s window would reject two of every three polls. Remove this entry + // once the CLI consumes the user-joined event instead - the poll and this + // shim go together. ['getRoomUsers', 800], ]);