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/README.md b/README.md index 7b80023..34ae9c7 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 @@ -300,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 98f5c9b..c70ed26 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 @@ -130,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 @@ -276,8 +276,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 diff --git a/packages/transfer-server/src/JsBridgeE2EEServer.ts b/packages/transfer-server/src/JsBridgeE2EEServer.ts index 69492b7..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); @@ -424,6 +431,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..f8197f6 100644 --- a/packages/transfer-server/src/errors.ts +++ b/packages/transfer-server/src/errors.ts @@ -64,13 +64,21 @@ 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 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, 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/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; } diff --git a/packages/transfer-server/test/smoke.ts b/packages/transfer-server/test/smoke.ts index 653a314..012da04 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 (${ @@ -507,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');