diff --git a/packages/sdk-socket-server-next/e2e/api-config.test.ts b/packages/sdk-socket-server-next/e2e/api-config.test.ts index ad3fc01ee..8a514e31f 100644 --- a/packages/sdk-socket-server-next/e2e/api-config.test.ts +++ b/packages/sdk-socket-server-next/e2e/api-config.test.ts @@ -1,5 +1,5 @@ import request from 'supertest'; -import { app } from '../src/api-config'; +import { app } from '../src/analytics-api'; jest.mock('analytics-node'); diff --git a/packages/sdk-socket-server-next/jest.config.ts b/packages/sdk-socket-server-next/jest.config.ts new file mode 100644 index 000000000..e0415ac1d --- /dev/null +++ b/packages/sdk-socket-server-next/jest.config.ts @@ -0,0 +1,18 @@ +import baseConfig from '../../jest.config.base'; + +module.exports = { + ...baseConfig, + testMatch: [ + '**/__tests__/**/*.[jt]s?(x)', + '**/?(*.)+(spec|test).[tj]s?(x)', + ], + testPathIgnorePatterns: ['/node_modules/', '/dist/'], + setupFiles: ['/jest.setup.ts'], + moduleNameMapper: { + '^analytics-node$': '/e2e/analytics-node.ts', + }, + clearMocks: true, + resetMocks: false, + restoreMocks: false, + watchman: false, +}; diff --git a/packages/sdk-socket-server-next/jest.setup.ts b/packages/sdk-socket-server-next/jest.setup.ts new file mode 100644 index 000000000..6131c906e --- /dev/null +++ b/packages/sdk-socket-server-next/jest.setup.ts @@ -0,0 +1,12 @@ +/* eslint-disable node/no-process-env */ +// Jest setup: ensure required environment variables are present so that +// `analytics-api` does not call `process.exit(1)` when it loads. +process.env.REDIS_NODES = + process.env.REDIS_NODES ?? 'redis://localhost:6379'; +process.env.NODE_ENV = process.env.NODE_ENV ?? 'test'; + +// Some tests mock `./analytics-api` so the side-effectful logger setup in +// `./config` never runs. Initialise the logger here so `getLogger()` returns +// a real winston logger from any module under test. +// eslint-disable-next-line import/no-unassigned-import +import './src/config'; diff --git a/packages/sdk-socket-server-next/src/protocol/handleChannelRejected.test.ts b/packages/sdk-socket-server-next/src/protocol/handleChannelRejected.test.ts new file mode 100644 index 000000000..419413bfd --- /dev/null +++ b/packages/sdk-socket-server-next/src/protocol/handleChannelRejected.test.ts @@ -0,0 +1,191 @@ +/* eslint-disable jsdoc/require-jsdoc */ +import { v4 as uuidv4 } from 'uuid'; + +const mockPubClient = { + get: jest.fn(), + setex: jest.fn(), +}; + +jest.mock('../analytics-api', () => ({ + pubClient: mockPubClient, +})); + +jest.mock('@socket.io/redis-adapter', () => ({ + createAdapter: jest.fn(), +})); + +import { + handleChannelRejected, + ChannelRejectedParams, +} from './handleChannelRejected'; +import { ChannelConfig } from './handleJoinChannel'; + +function makeSocket({ + rooms, + socketId = 'socket-id-1', +}: { + rooms: string[]; + socketId?: string; +}) { + const broadcastToEmit = jest.fn(); + return { + id: socketId, + request: { socket: { remoteAddress: '127.0.0.1' } }, + rooms: new Set(rooms), + broadcast: { + to: jest.fn(() => ({ emit: broadcastToEmit })), + }, + } as any; +} + +describe('handleChannelRejected participant check (HackerOne 3604630)', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('rejects requests from a non-participant socket on a fresh channel', async () => { + const channelId = uuidv4(); + const socket = makeSocket({ rooms: [] }); + + // No existing channelConfig: this is a fresh channelId an attacker is + // poking at. + mockPubClient.get.mockResolvedValueOnce(null); + + const callback = jest.fn(); + const params: ChannelRejectedParams = { + io: {} as any, + socket, + channelId, + }; + + await handleChannelRejected(params, callback); + + expect(callback).toHaveBeenCalledWith('not authorized', undefined); + // Must not write any rejected entry to redis on a non-participant request. + expect(mockPubClient.setex).not.toHaveBeenCalled(); + expect(socket.broadcast.to).not.toHaveBeenCalled(); + }); + + it('rejects requests from a non-participant socket even when a channelConfig with no wallet exists', async () => { + const channelId = uuidv4(); + const socket = makeSocket({ rooms: [] }); + + // Existing config but no wallet recorded yet (e.g. only the dapp has + // joined). The reconnect-and-reject flow only applies to wallets that + // had previously joined. + const existingConfig: ChannelConfig = { + clients: { dapp: 'dapp-socket-id', wallet: '' }, + createdAt: 1, + updatedAt: 1, + }; + mockPubClient.get.mockResolvedValueOnce(JSON.stringify(existingConfig)); + + const callback = jest.fn(); + const params: ChannelRejectedParams = { + io: {} as any, + socket, + channelId, + }; + + await handleChannelRejected(params, callback); + + expect(callback).toHaveBeenCalledWith('not authorized', undefined); + expect(mockPubClient.setex).not.toHaveBeenCalled(); + }); + + it('rejects requests with an invalid channelId', async () => { + const socket = makeSocket({ rooms: [] }); + const callback = jest.fn(); + const params: ChannelRejectedParams = { + io: {} as any, + socket, + channelId: 'not-a-uuid', + }; + + await handleChannelRejected(params, callback); + + expect(callback).toHaveBeenCalledWith('error_id', undefined); + expect(mockPubClient.get).not.toHaveBeenCalled(); + expect(mockPubClient.setex).not.toHaveBeenCalled(); + }); + + it('allows the request when the socket is a live in-room participant', async () => { + const channelId = uuidv4(); + const socket = makeSocket({ rooms: [channelId] }); + + mockPubClient.get.mockResolvedValueOnce(null); + mockPubClient.setex.mockResolvedValueOnce('OK'); + + const callback = jest.fn(); + const params: ChannelRejectedParams = { + io: {} as any, + socket, + channelId, + }; + + await handleChannelRejected(params, callback); + + expect(mockPubClient.setex).toHaveBeenCalledTimes(1); + expect(callback).toHaveBeenCalledWith(null, { success: true }); + }); + + it('allows the post-reconnect reject flow when channelConfig has a known wallet', async () => { + const channelId = uuidv4(); + // Wallet has reconnected, so its socket is not in the room. + const socket = makeSocket({ + rooms: [], + socketId: 'wallet-reconnected-socket-id', + }); + + const existingConfig: ChannelConfig = { + clients: { + wallet: 'previous-wallet-socket-id', + dapp: 'dapp-socket-id', + }, + createdAt: 1, + updatedAt: 1, + }; + mockPubClient.get.mockResolvedValueOnce(JSON.stringify(existingConfig)); + mockPubClient.setex.mockResolvedValueOnce('OK'); + + const callback = jest.fn(); + const params: ChannelRejectedParams = { + io: {} as any, + socket, + channelId, + }; + + await handleChannelRejected(params, callback); + + expect(mockPubClient.setex).toHaveBeenCalledTimes(1); + const payload = JSON.parse(mockPubClient.setex.mock.calls[0][2]); + expect(payload.rejected).toBe(true); + + expect(socket.broadcast.to).toHaveBeenCalledWith(channelId); + expect(callback).toHaveBeenCalledWith(null, { success: true }); + }); + + it('does not modify a channel that is already in the ready state', async () => { + const channelId = uuidv4(); + const socket = makeSocket({ rooms: [channelId] }); + + const existingConfig: ChannelConfig = { + clients: { wallet: 'wallet-id', dapp: 'dapp-id' }, + ready: true, + createdAt: 1, + updatedAt: 1, + }; + mockPubClient.get.mockResolvedValueOnce(JSON.stringify(existingConfig)); + + const callback = jest.fn(); + const params: ChannelRejectedParams = { + io: {} as any, + socket, + channelId, + }; + + await handleChannelRejected(params, callback); + + expect(mockPubClient.setex).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/sdk-socket-server-next/src/protocol/handleChannelRejected.ts b/packages/sdk-socket-server-next/src/protocol/handleChannelRejected.ts index 1c96b2889..3c53a66ca 100644 --- a/packages/sdk-socket-server-next/src/protocol/handleChannelRejected.ts +++ b/packages/sdk-socket-server-next/src/protocol/handleChannelRejected.ts @@ -1,4 +1,5 @@ import { Server, Socket } from 'socket.io'; +import { validate } from 'uuid'; import { pubClient } from '../analytics-api'; import { config } from '../config'; import { getLogger } from '../logger'; @@ -27,6 +28,16 @@ export const handleChannelRejected = async ( const socketId = socket.id; const clientIp = socket.request.socket.remoteAddress; + if (!validate(channelId)) { + logger.warn(`[handleChannelRejected] ${channelId} invalid channelId`, { + channelId, + socketId, + clientIp, + }); + callback?.('error_id', undefined); + return; + } + // Force keys into the same hash slot in Redis Cluster, using a hash tag (a substring enclosed in curly braces {}) const channelConfigKey = `channel_config:{${channelId}}`; const existingConfig = await pubClient.get(channelConfigKey); @@ -34,6 +45,21 @@ export const handleChannelRejected = async ( ? (JSON.parse(existingConfig) as ChannelConfig) : null; + const isInRoom = socket.rooms.has(channelId); + const isKnownWalletParticipant = Boolean(channelConfig?.clients?.wallet); + if (!isInRoom && !isKnownWalletParticipant) { + logger.warn( + `[handleChannelRejected] non-participant rejected request for ${channelId}`, + { + channelId, + socketId, + clientIp, + }, + ); + callback?.('not authorized', undefined); + return; + } + if (channelConfig) { logger.debug( `[handleChannelRejected] Channel already exists: ${channelId}`, diff --git a/packages/sdk-socket-server-next/src/protocol/handleMessage.test.ts b/packages/sdk-socket-server-next/src/protocol/handleMessage.test.ts new file mode 100644 index 000000000..8b3cd401f --- /dev/null +++ b/packages/sdk-socket-server-next/src/protocol/handleMessage.test.ts @@ -0,0 +1,95 @@ +/* eslint-disable jsdoc/require-jsdoc */ +import { v4 as uuidv4 } from 'uuid'; + +const mockPubClient = { + get: jest.fn(), + set: jest.fn(), + rpush: jest.fn(), + expire: jest.fn(), +}; + +jest.mock('../analytics-api', () => ({ + pubClient: mockPubClient, +})); + +jest.mock('../rate-limiter', () => ({ + rateLimiterMessage: { consume: jest.fn().mockResolvedValue(undefined) }, + resetRateLimits: jest.fn(), + increaseRateLimits: jest.fn(), + setLastConnectionErrorTimestamp: jest.fn(), +})); + +jest.mock('@socket.io/redis-adapter', () => ({ + createAdapter: jest.fn(), +})); + +import { handleMessage, MessageParams } from './handleMessage'; + +type Emit = jest.Mock; + +function makeSocket(channelId: string) { + const broadcastToEmit: Emit = jest.fn(); + const broadcastEmit: Emit = jest.fn(); + + const broadcast = { + to: jest.fn(() => ({ emit: broadcastToEmit })), + emit: broadcastEmit, + }; + + return { + socket: { + id: 'socket-id-1', + handshake: { address: '127.0.0.1' }, + request: { socket: { remoteAddress: '127.0.0.1' } }, + rooms: new Set([channelId]), + broadcast, + emit: jest.fn(), + } as any, + broadcastToEmit, + broadcastEmit, + broadcastTo: broadcast.to, + }; +} + +describe('handleMessage error path', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('scopes the error broadcast to the channel room (regression: HackerOne 3604630)', async () => { + const channelId = uuidv4(); + const { socket, broadcastTo, broadcastToEmit, broadcastEmit } = + makeSocket(channelId); + + // Force handleMessage to throw inside its try-block by making + // pubClient.get reject. + mockPubClient.get.mockRejectedValueOnce(new Error('boom')); + + const callback = jest.fn(); + + const params: MessageParams = { + io: {} as any, + socket, + channelId, + clientType: 'dapp', + context: 'dapp', + message: 'encrypted-string', + hasRateLimit: false, + callback, + }; + + await handleMessage(params); + + expect(broadcastTo).toHaveBeenCalledWith(channelId); + expect(broadcastToEmit).toHaveBeenCalledWith( + `message-${channelId}`, + expect.objectContaining({ error: 'boom' }), + ); + + // CRITICAL: error must NOT be broadcast to all sockets (which would + // leak the active channel ID). + expect(broadcastEmit).not.toHaveBeenCalled(); + + expect(callback).toHaveBeenCalledWith('boom'); + }); +}); diff --git a/packages/sdk-socket-server-next/src/protocol/handleMessage.ts b/packages/sdk-socket-server-next/src/protocol/handleMessage.ts index 5ed0a6668..1c87489fc 100644 --- a/packages/sdk-socket-server-next/src/protocol/handleMessage.ts +++ b/packages/sdk-socket-server-next/src/protocol/handleMessage.ts @@ -114,7 +114,7 @@ export const handleMessage = async ({ ackId = uuidv4(); // Store in the correct message queue const otherQueue = clientType === 'dapp' ? 'wallet' : 'dapp'; - // Force keys into the same hash slot in Redis Cluster, using a hash tag (a substring enclosed in curly braces {}) + // Force keys into the same hash slot in Redis Cluster, using a hash tag (a substring enclosed in curly braces {}) const queueKey = `queue:{${channelId}}:${otherQueue}`; const persistedMsg: QueuedMessage = { message, @@ -174,8 +174,7 @@ export const handleMessage = async ({ clientIp, }); - // emit an error message back to the client, if appropriate - socket.broadcast.emit(`message-${channelId}`, { + socket.broadcast.to(channelId).emit(`message-${channelId}`, { error: error instanceof Error ? error.message : 'Unknown error occurred', }); diff --git a/packages/sdk-socket-server-next/src/socket-config.test.ts b/packages/sdk-socket-server-next/src/socket-config.test.ts new file mode 100644 index 000000000..e0f4b0587 --- /dev/null +++ b/packages/sdk-socket-server-next/src/socket-config.test.ts @@ -0,0 +1,217 @@ +/* eslint-disable jsdoc/require-jsdoc */ +import { createServer } from 'http'; +import { AddressInfo } from 'net'; +import { v4 as uuidv4 } from 'uuid'; +import { Server as IOServer } from 'socket.io'; +import { io as ioClient, Socket as ClientSocket } from 'socket.io-client'; + +const mockPubClient = { + on: jest.fn(), + duplicate: jest.fn(() => ({ on: jest.fn() })), + get: jest.fn(), + set: jest.fn(), + setex: jest.fn(), + incrby: jest.fn().mockResolvedValue(1), + del: jest.fn(), +}; + +jest.mock('./analytics-api', () => ({ + pubClient: mockPubClient, +})); + +jest.mock('@socket.io/redis-adapter', () => ({ + createAdapter: () => undefined, +})); + +const handleAck = jest.fn().mockResolvedValue(undefined); +const handlePing = jest.fn().mockResolvedValue(undefined); +const handleMessage = jest.fn().mockResolvedValue(undefined); +const handleChannelRejected = jest.fn().mockResolvedValue(undefined); +const handleJoinChannel = jest.fn().mockResolvedValue(undefined); +const handleCheckRoom = jest.fn().mockResolvedValue(undefined); + +jest.mock('./protocol/handleAck', () => ({ + handleAck: (...args: unknown[]) => handleAck(...args), +})); +jest.mock('./protocol/handlePing', () => ({ + handlePing: (...args: unknown[]) => handlePing(...args), +})); +jest.mock('./protocol/handleMessage', () => ({ + handleMessage: (...args: unknown[]) => handleMessage(...args), +})); +jest.mock('./protocol/handleChannelRejected', () => ({ + handleChannelRejected: (...args: unknown[]) => handleChannelRejected(...args), +})); +jest.mock('./protocol/handleJoinChannel', () => ({ + handleJoinChannel: (...args: unknown[]) => handleJoinChannel(...args), +})); +jest.mock('./protocol/handleCheckRoom', () => ({ + handleCheckRoom: (...args: unknown[]) => handleCheckRoom(...args), +})); + +import { configureSocketServer } from './socket-config'; + +type ServerHandle = { + ioServer: IOServer; + close: () => Promise; + port: number; +}; + +async function startServer(): Promise { + const httpServer = createServer(); + const ioServer = await configureSocketServer(httpServer); + + await new Promise((resolve) => { + httpServer.listen(0, '127.0.0.1', () => resolve()); + }); + + const { port } = httpServer.address() as AddressInfo; + + return { + ioServer, + port, + close: async () => { + // socket.io's `Server.close` also closes the underlying http + // server, so we don't call httpServer.close() separately. + await new Promise((resolve) => { + ioServer.close(() => resolve()); + }); + }, + }; +} + +function connectClient(port: number): Promise { + return new Promise((resolve, reject) => { + const client = ioClient(`http://127.0.0.1:${port}`, { + transports: ['websocket'], + forceNew: true, + reconnection: false, + }); + client.on('connect', () => resolve(client)); + client.on('connect_error', reject); + }); +} + +async function settle() { + // Wait long enough for the server to process the emitted event. + await new Promise((resolve) => setTimeout(resolve, 50)); +} + +describe('socket-config room-membership guards (HackerOne 3604630)', () => { + let handle: ServerHandle | undefined; + let client: ClientSocket | undefined; + + beforeEach(async () => { + jest.clearAllMocks(); + handle = await startServer(); + }); + + afterEach(async () => { + client?.disconnect(); + client = undefined; + if (handle) { + await handle.close(); + handle = undefined; + } + }); + + it('blocks ping from a socket that has not joined the channel room', async () => { + if (!handle) { + throw new Error('server not initialised'); + } + const channelId = uuidv4(); + client = await connectClient(handle.port); + + client.emit( + 'ping', + { id: channelId, clientType: 'dapp' }, + () => undefined, + ); + await settle(); + + expect(handlePing).not.toHaveBeenCalled(); + }); + + it('blocks ack from a socket that has not joined the channel room', async () => { + if (!handle) { + throw new Error('server not initialised'); + } + const channelId = uuidv4(); + client = await connectClient(handle.port); + + client.emit('ack', { + channelId, + ackId: uuidv4(), + clientType: 'dapp', + }); + await settle(); + + expect(handleAck).not.toHaveBeenCalled(); + }); + + it('blocks message from a socket that has not joined the channel room', async () => { + if (!handle) { + throw new Error('server not initialised'); + } + const channelId = uuidv4(); + client = await connectClient(handle.port); + + client.emit( + 'message', + { + id: channelId, + message: 'encrypted', + context: 'dapp', + clientType: 'dapp', + plaintext: '', + }, + () => undefined, + ); + await settle(); + + expect(handleMessage).not.toHaveBeenCalled(); + }); + + it('forwards ping and ack to the protocol handlers when the socket is in the channel room', async () => { + if (!handle) { + throw new Error('server not initialised'); + } + const channelId = uuidv4(); + client = await connectClient(handle.port); + + // Move the corresponding server-side socket into the channel room + // directly (bypassing the mocked join_channel handler) so we can + // verify the positive case for both guards. + const sockets = await handle.ioServer.fetchSockets(); + expect(sockets).toHaveLength(1); + await sockets[0].join(channelId); + + client.emit( + 'ping', + { id: channelId, clientType: 'dapp' }, + () => undefined, + ); + client.emit('ack', { + channelId, + ackId: uuidv4(), + clientType: 'dapp', + }); + await settle(); + + expect(handlePing).toHaveBeenCalledTimes(1); + expect(handleAck).toHaveBeenCalledTimes(1); + }); + + it('still forwards rejected to handleChannelRejected (participant check is inside the handler)', async () => { + if (!handle) { + throw new Error('server not initialised'); + } + const channelId = uuidv4(); + client = await connectClient(handle.port); + + client.emit('rejected', { channelId }); + await settle(); + + expect(handleChannelRejected).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/sdk-socket-server-next/src/socket-config.ts b/packages/sdk-socket-server-next/src/socket-config.ts index c99b6803c..6f684cf47 100644 --- a/packages/sdk-socket-server-next/src/socket-config.ts +++ b/packages/sdk-socket-server-next/src/socket-config.ts @@ -228,6 +228,17 @@ export const configureSocketServer = async ( const start = Date.now(); incrementAck(); + // only ack if we are in the room (prevents non-member sockets from + // permanently deleting queued messages for arbitrary channels) + if (!socket.rooms.has(channelId)) { + logger.warn(`ack ${channelId} not in room`, { + channelId, + socketId, + clientIp, + }); + return; + } + const ackParams: ACKParams = { io, socket, @@ -305,6 +316,17 @@ export const configureSocketServer = async ( const start = Date.now(); incrementPing(); + // only ping if we are in the room (prevents non-member sockets from + // retrieving queued messages for arbitrary channels) + if (!socket.rooms.has(id)) { + logger.warn(`ping ${id} not in room`, { + channelId: id, + socketId, + clientIp, + }); + return; + } + handlePing({ channelId: id, socket,