From 6752dc17972bb1e566e1449f1b82b9c5de0698ca Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 09:18:32 +0000 Subject: [PATCH 1/5] 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/5] 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; } From 2e4b7f9772fc0b323cf60ce2490f55c54a3a8c99 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 04:05:33 +0000 Subject: [PATCH 3/5] fix: strip server stack traces at the socket egress point Removing `stack` from E2eeError.toJSON() did not stop the leak: toJSON() is never reached on the wire path. JsBridgeBase.createPayload() replaces payload.error with its own plain copy first (toPlainError), and that copy reads err.stack straight off the instance, so responseError -> send -> createPayload -> sendPayload emitted the full server stack. Verified against the previous build: a joinRoom call with an invalid roomId returned `['name', 'message', 'stack', 'code']` with absolute server paths, and rate-limit errors leaked the bridge internals the same way. Strip `stack` in JsBridgeE2EEServer.sendPayload() instead - the single point every response passes through, so it covers E2eeError, rate-limit errors, and anything else thrown inside an API method. toJSON() keeps its stack-free shape as defence in depth for direct serialization, with its comment corrected to stop claiming it is what protects the socket path. Add a smoke assertion on a dedicated client (so the per-connection, per-method rate limiter does not perturb the other checks) that an error response carries no `stack`. Confirmed it fails when the strip is removed and passes with it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AuowqYp863fJaNNjEKvhPT --- .../transfer-server/src/JsBridgeE2EEServer.ts | 7 ++++++ packages/transfer-server/src/errors.ts | 16 ++++++++----- packages/transfer-server/test/smoke.ts | 23 +++++++++++++++++++ 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/packages/transfer-server/src/JsBridgeE2EEServer.ts b/packages/transfer-server/src/JsBridgeE2EEServer.ts index ca529c0..e244067 100644 --- a/packages/transfer-server/src/JsBridgeE2EEServer.ts +++ b/packages/transfer-server/src/JsBridgeE2EEServer.ts @@ -125,6 +125,13 @@ export class JsBridgeE2EEServer extends JsBridgeBase { sendPayload(payload: IJsBridgeMessagePayload | string): void { const p = payload as IJsBridgeMessagePayload; + // JsBridgeBase.createPayload() has already replaced `error` with a plain + // copy (toPlainError) that carries err.stack verbatim, so + // E2eeError.toJSON() never runs on this path. Strip the server stack here, + // at the single egress point, for every error type. + if (p?.error && typeof p.error === 'object') { + delete (p.error as { stack?: string }).stack; + } const e = p?.error as { message: string; code: number } | undefined; if (e && e?.code && e?.code === CLIENT_TO_CLIENT_RATE_LIMIT_ERROR_CODE) { this.socketClient.emit('e2ee-c2c-response', payload); diff --git a/packages/transfer-server/src/errors.ts b/packages/transfer-server/src/errors.ts index 0a50d1d..f8197f6 100644 --- a/packages/transfer-server/src/errors.ts +++ b/packages/transfer-server/src/errors.ts @@ -64,12 +64,16 @@ export class E2eeError extends Error { return new E2eeError(code, message); } - // Convert to JSON for serialization. + // Convert to JSON for serialization, without the server stack trace. // - // 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. + // This is not what keeps the stack off the socket path: JsBridgeBase + // .createPayload() replaces payload.error with its own plain copy + // (toPlainError), reading err.stack off the instance directly, so toJSON() + // never runs before a response is emitted. The stack is stripped at the + // single egress point instead - see JsBridgeE2EEServer.sendPayload(). This + // stays as defence in depth for any path that serializes an E2eeError + // directly. `stack` remains on the instance for server-side pino logging, + // which also reads err.stack rather than going through toJSON. toJSON() { return { name: this.name, @@ -77,4 +81,4 @@ export class E2eeError extends Error { code: this.code, }; } -} \ No newline at end of file +} diff --git a/packages/transfer-server/test/smoke.ts b/packages/transfer-server/test/smoke.ts index dfcbe3b..012da04 100644 --- a/packages/transfer-server/test/smoke.ts +++ b/packages/transfer-server/test/smoke.ts @@ -550,6 +550,29 @@ async function main(): Promise { check(stillWorks.length === 2, 'session unaffected by oversized-log flood'); logFlooder.socket.disconnect(); + // --- an error response must never carry the server stack trace. It cannot + // be stopped by E2eeError.toJSON(): JsBridgeBase.createPayload() + // replaces payload.error with its own plain copy first, and that copy + // (toPlainError) reads err.stack straight off the instance. So it is + // stripped in sendPayload(), and it has to stay stripped. + // A fresh client keeps this off the rate-limit windows used above. --- + const errorProbe = makeClient('smoke-client-F'); + await errorProbe.ready; + const errorPayload = await errorProbe.callRaw('roomManager', 'joinRoom', [ + { roomId: 'not-a-valid-room-id', ...appInfo('F') }, + ]); + check( + Boolean(errorPayload.error), + 'invalid roomId is rejected with an error', + `code=${String(errorPayload.error?.code)}`, + ); + check( + errorPayload.error?.stack === undefined, + 'error response carries no server stack trace', + errorPayload.error?.stack ? 'LEAKED - server stack sent to client' : 'no stack field', + ); + errorProbe.socket.disconnect(); + const clientC = makeClient('smoke-client-C'); await clientC.ready; check(clientC.socket.connected, 'new client can still connect'); From d8fdd627782a79785b028692cd04e72c8ba91cb8 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 04:05:40 +0000 Subject: [PATCH 4/5] docs: drop the removed CORS_ORIGINS env var CORS_ORIGINS no longer exists - the allowlist it fed was never enforced and was replaced by an explicit permissive config. The docs still advertised it, so a deployer would set it and believe origins were being filtered. Remove it from the transfer-server config table, both example .env blocks, and env.example, and rewrite the "CORS Issues" troubleshooting entry to say that CORS is deliberately permissive with nothing to configure, pointing at the corsOptions comment in src/server.ts for why Origin is not the auth boundary. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AuowqYp863fJaNNjEKvhPT --- README.md | 1 - packages/transfer-server/README.md | 9 +++++---- packages/transfer-server/env.example | 3 --- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 7b80023..e7b370b 100644 --- a/README.md +++ b/README.md @@ -133,7 +133,6 @@ Each package can be configured using environment variables. Create `.env` files #### transfer-server ```env PORT=3868 -CORS_ORIGINS=http://localhost:3000 MAX_USERS_PER_ROOM=2 ROOM_TIMEOUT=3600000 MAX_MESSAGE_SIZE=10485760 diff --git a/packages/transfer-server/README.md b/packages/transfer-server/README.md index 98f5c9b..44a9fa7 100644 --- a/packages/transfer-server/README.md +++ b/packages/transfer-server/README.md @@ -49,7 +49,6 @@ The server can be configured using environment variables: | Variable | Default | Description | |----------|---------|-------------| | `PORT` | `3868` | Server listening port | -| `CORS_ORIGINS` | `*` | Comma-separated list of allowed CORS origins | | `MAX_USERS_PER_ROOM` | `2` | Maximum users allowed per room | | `ROOM_TIMEOUT` | `3600000` | Room timeout in milliseconds (1 hour) | | `MAX_MESSAGE_SIZE` | `10485760` | Maximum message size in bytes (10MB) | @@ -57,7 +56,6 @@ The server can be configured using environment variables: Example `.env` file: ```env PORT=3868 -CORS_ORIGINS=http://localhost:3000,https://app.onekey.so MAX_USERS_PER_ROOM=2 ROOM_TIMEOUT=3600000 MAX_MESSAGE_SIZE=10485760 @@ -276,8 +274,11 @@ curl http://localhost:3868/stats ``` 2. **CORS Issues** - - Ensure `CORS_ORIGINS` environment variable is properly configured - - Check that client origin matches allowed origins + - CORS is intentionally permissive: every origin is accepted and there is no + allowlist to configure + - `Origin` is not the auth boundary here - access control is the out-of-band + pairing code plus the room membership check on the client-to-client relay. + See the comment on `corsOptions` in `src/server.ts` for why 3. **Connection Timeouts** - Verify firewall settings diff --git a/packages/transfer-server/env.example b/packages/transfer-server/env.example index ab8f1fe..9ad361e 100644 --- a/packages/transfer-server/env.example +++ b/packages/transfer-server/env.example @@ -3,9 +3,6 @@ # Server port (default: 3868) PORT=3868 -# CORS allowed origins, comma-separated -CORS_ORIGINS=http://localhost:3000,http://localhost:3001,http://localhost:3868 - # Maximum users per room (default: 2) MAX_USERS_PER_ROOM=2 From a1ce68e40dd860e02609e19ffe9384b8631b9f5e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 04:09:39 +0000 Subject: [PATCH 5/5] docs: stop presenting CORS as configurable protection The CORS_ORIGINS cleanup missed the two prose claims that sent a deployer looking for the knob it removed: the transfer-server security feature list called CORS "Configurable CORS origins" and both READMEs advised configuring origins appropriately. Neither was ever true - the allowlist was built but never enforced, and it is gone now. Replace the feature-list entry with the control that actually gates the relay (the c2c room membership check) and rewrite the best-practice lines to say Origin is not the auth boundary here, pointing at the corsOptions comment in src/server.ts. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AuowqYp863fJaNNjEKvhPT --- README.md | 3 ++- packages/transfer-server/README.md | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index e7b370b..34ae9c7 100644 --- a/README.md +++ b/README.md @@ -299,7 +299,8 @@ chore: Update dependencies - All sensitive configuration should use environment variables - Never commit `.env` files - Use HTTPS in production -- Configure CORS appropriately +- Do not treat CORS as access control in transfer-server: it is deliberately + permissive (see `corsOptions` in `packages/transfer-server/src/server.ts`) - Implement rate limiting - Regular dependency updates diff --git a/packages/transfer-server/README.md b/packages/transfer-server/README.md index 44a9fa7..c70ed26 100644 --- a/packages/transfer-server/README.md +++ b/packages/transfer-server/README.md @@ -128,13 +128,15 @@ MAX_MESSAGE_SIZE=10485760 1. **Message Size Limits**: Prevents DoS attacks by limiting message sizes 2. **Room Timeouts**: Automatic cleanup of inactive rooms 3. **User Limits**: Configurable maximum users per room -4. **CORS Protection**: Configurable CORS origins +4. **Room Membership Enforcement**: Client-to-client messages are relayed only + for a sender that has actually joined the target room 5. **Input Validation**: Automatic validation of all API inputs ### Best Practices - Always use HTTPS in production -- Configure CORS origins appropriately +- Do not treat CORS as access control: it is deliberately permissive and + `Origin` is not the auth boundary here (see `corsOptions` in `src/server.ts`) - Implement rate limiting with a reverse proxy - Monitor room creation patterns for abuse - Use environment variables for sensitive configuration