From 6752dc17972bb1e566e1449f1b82b9c5de0698ca Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 09:18:32 +0000 Subject: [PATCH 1/2] fix: harden c2c relay and error handling - Reject client-to-client messages whose sender socket has not joined the target room, closing an unauthenticated cross-room injection path (cancelTransfer / verifyPairingCode / forged-response hijack) that was gated only by knowledge of the room id. - Drop the stack trace from E2eeError.toJSON so server internals are not serialized to clients; pino still records it server-side via err.stack. - Document that the room encryptionKey is not the E2EE key and is currently unused, reserved for a future transport-layer encryption. - Add a smoke-test assertion that a non-member cannot inject c2c traffic. --- .../transfer-server/src/JsBridgeE2EEServer.ts | 12 ++++++ packages/transfer-server/src/errors.ts | 8 +++- packages/transfer-server/src/roomManager.ts | 6 +++ packages/transfer-server/test/smoke.ts | 43 +++++++++++++++++++ 4 files changed, 67 insertions(+), 2 deletions(-) diff --git a/packages/transfer-server/src/JsBridgeE2EEServer.ts b/packages/transfer-server/src/JsBridgeE2EEServer.ts index 69492b7..ca529c0 100644 --- a/packages/transfer-server/src/JsBridgeE2EEServer.ts +++ b/packages/transfer-server/src/JsBridgeE2EEServer.ts @@ -424,6 +424,18 @@ export class JsBridgeE2EEServer extends JsBridgeBase { return undefined; } + // The relay targets `roomId` verbatim (socket.to(roomId).emit), but roomId + // comes from the client envelope. Without this check any connected socket + // could inject c2c traffic into a room it never joined - forcing peers to + // cancel/abort (cancelTransfer is rate-limit exempt), burning their pairing + // attempt budget, or hijacking an in-flight response by id. A legitimate + // peer is always joined to its Socket.IO room via roomManager.joinRoom + // before it sends any c2c message, so a genuine member is never rejected. + if (!this.socketClient.rooms.has(roomId)) { + this.logInvalidPayload(eventName, payload, 'sender is not a member of roomId'); + return undefined; + } + const checked = checkBridgePayload(payload, { requireMethod }); if (!checked.valid) { this.logInvalidPayload(eventName, payload, checked.reason); diff --git a/packages/transfer-server/src/errors.ts b/packages/transfer-server/src/errors.ts index 334e468..0a50d1d 100644 --- a/packages/transfer-server/src/errors.ts +++ b/packages/transfer-server/src/errors.ts @@ -64,13 +64,17 @@ export class E2eeError extends Error { return new E2eeError(code, message); } - // Convert to JSON for serialization + // Convert to JSON for serialization. + // + // This runs when the error is JSON-serialized onto a socket.io payload and + // sent to the client, so it must not leak the server stack trace (file paths, + // internal structure). `stack` stays on the instance for server-side pino + // logging, which reads err.stack directly rather than through toJSON. toJSON() { return { name: this.name, message: this.message, code: this.code, - stack: this.stack, }; } } \ No newline at end of file diff --git a/packages/transfer-server/src/roomManager.ts b/packages/transfer-server/src/roomManager.ts index 508cd03..b362ebe 100644 --- a/packages/transfer-server/src/roomManager.ts +++ b/packages/transfer-server/src/roomManager.ts @@ -63,6 +63,12 @@ export class RoomManager { } } while (this.rooms.has(roomId)); + // NOTE: this is NOT the end-to-end encryption key. The real E2EE key never + // reaches the server - clients derive it themselves from the out-of-band + // pairing code plus an ECDHE exchange, so the server cannot decrypt transfer + // payloads. This server-generated key is currently unused by clients and is + // reserved for a future additional (transport-layer) encryption layer. Do + // not treat it as the secret that protects user data. const encryptionKey = cryptoUtils.generateEncryptionKey(); const room: IRoom = { diff --git a/packages/transfer-server/test/smoke.ts b/packages/transfer-server/test/smoke.ts index 653a314..dfcbe3b 100644 --- a/packages/transfer-server/test/smoke.ts +++ b/packages/transfer-server/test/smoke.ts @@ -407,6 +407,49 @@ async function main(): Promise { // --- baseline: peers can talk both ways before anything goes wrong --- await checkBidirectional(clientA, clientB, room.roomId, 'before'); + // --- security: a socket that never joined the room must not be able to + // inject c2c traffic into it. The relay emits to the client-supplied + // roomId, so without a membership check any connected socket could push + // cancelTransfer / verifyPairingCode / a forged response into a live + // session it only knows the id of. The outsider knows room.roomId but + // never called joinRoom, so both channels must be dropped, and the two + // real members must be unaffected. --- + const outsider = makeClient('smoke-client-outsider'); + await outsider.ready; + const injectToken = `inject-${Date.now()}`; + const seenBefore = { + aReq: clientA.c2cRequests.length, + bReq: clientB.c2cRequests.length, + aRes: clientA.c2cResponses.length, + bRes: clientB.c2cResponses.length, + }; + outsider.socket.emit('e2ee-c2c-request', { + payload: { + id: Date.now(), + type: 'REQUEST', + data: { module: 'peer', method: `inject_${injectToken}`, params: [injectToken] }, + }, + roomId: room.roomId, + }); + outsider.socket.emit('e2ee-c2c-response', { + payload: { id: Date.now(), type: 'RESPONSE', data: { result: injectToken } }, + roomId: room.roomId, + }); + await wait(1000); + const injected = + clientA.c2cRequests.length > seenBefore.aReq || + clientB.c2cRequests.length > seenBefore.bReq || + clientA.c2cResponses.length > seenBefore.aRes || + clientB.c2cResponses.length > seenBefore.bRes; + check( + !injected, + 'non-member cannot inject c2c into a room it never joined', + injected ? 'INJECTED - membership check bypassed' : 'both channels dropped', + ); + check((await health()) === 200, 'server alive after c2c injection attempt'); + await checkBidirectional(clientA, clientB, room.roomId, 'after injection attempt'); + outsider.socket.disconnect(); + // --- both clients attack every listener --- console.log( `\nfiring ${MALFORMED_PACKETS.length} malformed packets from each client (${ From 20ff7bc35d977180ee930e4d4c1fe87eaf4331e8 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 09:32:35 +0000 Subject: [PATCH 2/2] refactor: remove misleading unenforced CORS allowlist The corsOrigins allowlist (read from CORS_ORIGINS) was built but never enforced: the origin callback returned true for every origin, with the reject branch commented out. Replace it with an explicit permissive config and document why Origin is not an auth boundary here - the primary clients (native, desktop file://) send no usable Origin, there are no cookie credentials to protect, and Origin is forgeable by non-browser clients. Access control remains the out-of-band pairing code plus the c2c room membership check. --- CLAUDE.md | 7 +++-- packages/transfer-server/src/server.ts | 41 +++++++++++--------------- packages/transfer-server/src/types.ts | 1 - 3 files changed, 23 insertions(+), 26 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 154ec4c..040afac 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -57,7 +57,6 @@ The transfer server is a Socket.IO-based real-time communication server with end **Configuration (via environment variables):** - `PORT` (default: 3868) -- `CORS_ORIGINS` (comma-separated list) - `MAX_USERS_PER_ROOM` (default: 2) - `ROOM_TIMEOUT` (default: 3600000ms) - `MAX_MESSAGE_SIZE` (default: 10485760 bytes) @@ -100,7 +99,11 @@ A Midway.js-based component for OneKey Prime synchronization functionality. 2. **Error Handling**: The transfer-server includes custom error codes (see `errors.ts`). The cloud-sync-server uses Midway.js error handling patterns. 3. **Security**: - - CORS is configured but currently allows all origins in development + - CORS is intentionally permissive; the `Origin` header is not an auth + boundary here (native/desktop clients send no usable Origin, and there are + no cookie credentials to protect). Access control is the out-of-band + pairing code plus the room membership check on the c2c relay. See the + comment on `corsOptions` in `server.ts`. - Message size limits are enforced - Room timeouts prevent resource exhaustion diff --git a/packages/transfer-server/src/server.ts b/packages/transfer-server/src/server.ts index c6b42ea..e74f687 100644 --- a/packages/transfer-server/src/server.ts +++ b/packages/transfer-server/src/server.ts @@ -47,19 +47,6 @@ class E2EEServer { constructor() { this.config = { port: parseInt(process.env.PORT || '3868', 10), - corsOrigins: process.env.CORS_ORIGINS?.split(',') || [ - 'http://localhost:3000', - 'http://localhost:3001', - 'http://localhost:3868', - 'null', - 'chrome-extension://*', - 'moz-extension://*', - 'ws://*', - 'wss://*', - 'http://*', - 'https://*', - '*', - ], roomConfig: { maxUsers: parseInt(process.env.MAX_USERS_PER_ROOM || '2', 10), roomTimeout: parseInt(process.env.ROOM_TIMEOUT || '3600000', 10), // 1 hour @@ -76,18 +63,26 @@ class E2EEServer { this.setupMiddleware(); this.setupRoutes(); + // CORS is intentionally permissive: the Origin header is not a security + // boundary for this server, so it is not used to gate connections. This + // replaces an allowlist that was built but never enforced (the callback + // returned true for every origin, with the reject branch commented out) - + // keeping the same allow-all behavior, without pretending to filter. + // + // Why Origin cannot be the boundary here: + // - The primary clients send no usable Origin: iOS/Android native (React + // Native sends no Origin header) and the desktop production build + // (file:// -> Origin: null), so an allowlist could never admit them. + // - There is no ambient credential to protect: the client connects with + // withCredentials=false and there are no cookies/sessions, so a + // cross-origin browser page gains nothing over a direct connection. + // - Origin is only trustworthy for real browsers; a non-browser client + // (the realistic attacker against a relay) can forge or omit it. + // The actual access control is the out-of-band pairing code plus the room + // membership check enforced on the client-to-client relay. this.corsOptions = { - origin: (origin, callback) => { - // eslint-disable-next-line @typescript-eslint/no-unsafe-argument - if (!origin || this.config.corsOrigins.includes(origin)) { - callback(null, true); - } else { - callback(null, true); - // callback(new Error('Invalid CORS request')); - } - }, + origin: true, methods: ['GET', 'POST'], - credentials: true, }; this.socketServer = new SocketIOServer< diff --git a/packages/transfer-server/src/types.ts b/packages/transfer-server/src/types.ts index 9c2a21f..f80b06b 100644 --- a/packages/transfer-server/src/types.ts +++ b/packages/transfer-server/src/types.ts @@ -101,7 +101,6 @@ export interface IRoomConfig { // Server configuration export interface IServerConfig { port: number; - corsOrigins: string[]; roomConfig: IRoomConfig; }