Repository: boardgameio/boardgame.io (ts)

Issue:
Unauthenticated client is allowed to connect at the moment when an authenticated client connects
Hi!

I've got a bug in 0.43.1. When I start an unautheticated client it can't connect, but if I start valid client in a different browser, the impostor client connects at that moment

Pull request:
fix: Improve security of socket.io transport layer
Closes #889

- Delay adding clients to `SocketIO#clientInfo` until after they have been authenticated.
- Improve transport API to avoid potential for a single event to trigger exponential numbers of emits.
- Add test cases for scenarios discussed in #889.
- Make process of adding and removing clients consistent across event handlers.

There is still no expiration of client connections, so hypothetically the transport is still vulnerable to a client that keeps open its connection after leaving a credentialed match that someone else then joins (see #889 for discussion). The only way to avoid that currently would be to re-authenticate each client when sending anything to them, which would add significant overhead to what is supposed to be a lightweight transport layer. I think ultimately there should be a way for the lobby API to ping the transport and say that a specific playerID’s credentials have expired at which point clients with that playerID could be re-authenticated or removed.

Patch:
diff --git a/src/server/transport/socketio.ts b/src/server/transport/socketio.ts
index 03e648c..7bc20f2 100644
--- a/src/server/transport/socketio.ts
+++ b/src/server/transport/socketio.ts
@@ -11,8 +11,11 @@ import type IOTypes from 'socket.io';
 import type { ServerOptions as HttpsOptions } from 'https';
 import PQueue from 'p-queue';
 import { Master } from '../../master/master';
-import type { TransportAPI as MasterTransport } from '../../master/master';
-import type { Game, PlayerID, Server } from '../../types';
+import type {
+  TransportAPI as MasterTransport,
+  TransportData,
+} from '../../master/master';
+import type { Game, Server } from '../../types';
 
 const PING_TIMEOUT = 20 * 1e3;
 const PING_INTERVAL = 10 * 1e3;
@@ -25,33 +28,52 @@ export function TransportAPI(
   matchID: string,
   socket: IOTypes.Socket,
   clientInfo: SocketIO['clientInfo'],
-  roomInfo: SocketIO['roomInfo']
+  roomInfo: SocketIO['roomInfo'],
+  provisionalClient?: Client
 ): MasterTransport {
   /**
-   * Send a message to a specific client.
+   * Emit a socket.io event to the recipientID.
+   * If the recipient is the current socket, uses a basic emit, otherwise
+   * emits via the current socket’s `to` method.
+   */
+  const emit = (recipientID: string, { type, args }: TransportData) => {
+    const emitter = socket.id === recipientID ? socket : socket.to(recipientID);
+    emitter.emit(type, ...args);
+  };
+
+  /**
+   * Run a callback for each registered client for this match, including
+   * this transport’s provisionalClient if provided.
    */
-  const send: MasterTransport['send'] = ({ type, playerID, args }) => {
-    const clients = roomInfo.get(matchID).values();
-    for (const client of clients) {
-      const info = clientInfo.get(client);
-      if (info.playerID === playerID) {
-        if (socket.id === client) {
-          socket.emit.apply(socket, [type, ...args]);
-        } else {
-          socket.to(info.socket.id).emit.apply(socket, [type, ...args]);
-        }
-      }
+  const forEachClient = (clientCallback: (client: Client) => void) => {
+    const clientIDs = roomInfo.get(matchID);
+    if (clientIDs) {
+      clientIDs.forEach((clientID) => {
+        const client = clientInfo.get(clientID);
+        clientCallback(client);
+      });
+    }
+    if (provisionalClient) {
+      clientCallback(provisionalClient);
     }
   };
 
+  /**
+   * Send a message to a specific client.
+   */
+  const send: MasterTransport['send'] = ({ playerID, ...data }) => {
+    forEachClient((client) => {
+      if (client.playerID === playerID) emit(client.socket.id, data);
+    });
+  };
+
   /**
    * Send a message to all clients.
    */
   const sendAll: MasterTransport['sendAll'] = (makePlayerData) => {
-    roomInfo.get(matchID).forEach((c) => {
-      const playerID: PlayerID = clientInfo.get(c).playerID;
-      const data = makePlayerData(playerID);
-      send({ playerID, ...data });
+    forEachClient((client) => {
+      const data = makePlayerData(client.playerID);
+      emit(client.socket.id, data);
     });
   };
 
@@ -91,6 +113,42 @@ export class SocketIO {
     this.socketOpts = socketOpts;
   }
 
+  /**
+   * Unregister client data for a socket.
+   */
+  private removeClient(socketID: string): void {
+    // Get client data for this socket ID.
+    const client = this.clientInfo.get(socketID);
+    if (!client) return;
+    // Remove client from list of connected sockets for this match.
+    const { matchID } = client;
+    const matchClients = this.roomInfo.get(matchID);
+    matchClients.delete(socketID);
+    // If the match is now empty, delete its promise queue & client ID list.
+    if (matchClients.size === 0) {
+      this.roomInfo.delete(matchID);
+      this.deleteMatchQueue(matchID);
+    }
+    // Remove client data from the client map.
+    this.clientInfo.delete(socketID);
+  }
+
+  /**
+   * Register client data for a socket.
+   */
+  private addClient(client: Client): void {
+    const { matchID, socket } = client;
+    // Add client to list of connected sockets for this match.
+    let matchClients = this.roomInfo.get(matchID);
+    if (matchClients === undefined) {
+      matchClients = new Set<string>();
+      this.roomInfo.set(matchID, matchClients);
+    }
+    matchClients.add(socket.id);
+    // Register data for this socket in the client map.
+    this.clientInfo.set(socket.id, client);
+  }
+
   init(app: Server.App & { _io?: IOTypes.Server }, games: Game[]) {
     const io = new IO({
       ioOptions: {
@@ -129,48 +187,36 @@ export class SocketIO {
         socket.on('sync', async (...args: Parameters<Master['onSync']>) => {
           const [matchID, playerID, credentials] = args;
           socket.join(matchID);
+          this.removeClient(socket.id);
 
-          // Remove client from any previous game that it was a part of.
-          if (this.clientInfo.has(socket.id)) {
-            const { matchID: oldMatchID } = this.clientInfo.get(socket.id);
-            this.roomInfo.get(oldMatchID).delete(socket.id);
-          }
-
-          let roomClients = this.roomInfo.get(matchID);
-          if (roomClients === undefined) {
-            roomClients = new Set<string>();
-            this.roomInfo.set(matchID, roomClients);
-          }
-          roomClients.add(socket.id);
-
-          this.clientInfo.set(socket.id, {
+          const client = { socket, matchID, playerID, credentials };
+          const transport = TransportAPI(
             matchID,
-            playerID,
             socket,
-            credentials,
-          });
-
+            this.clientInfo,
+            this.roomInfo,
+            client
+          );
           const master = new Master(
             game,
             app.context.db,
-            TransportAPI(matchID, socket, this.clientInfo, this.roomInfo),
+            transport,
             app.context.auth
           );
-          await master.onSync(...args);
+
+          const syncResponse = await master.onSync(...args);
+          if (syncResponse && syncResponse.error === 'unauthorized') {
+            return;
+          }
           await master.onConnectionChange(matchID, playerID, credentials, true);
+          this.addClient(client);
         });
 
         socket.on('disconnect', async () => {
           const client = this.clientInfo.get(socket.id);
+          this.removeClient(socket.id);
           if (client) {
             const { matchID, playerID, credentials } = client;
-            this.roomInfo.get(matchID).delete(socket.id);
-            this.clientInfo.delete(socket.id);
-
-            if (!this.roomInfo.get(matchID).size) {
-              this.deleteMatchQueue(matchID);
-            }
-
             const master = new Master(
               game,
               app.context.db,
@@ -185,6 +231,7 @@ export class SocketIO {
             );
           }
         });
+
         socket.on(
           'chat',
           async (...args: Parameters<Master['onChatMessage']>) => {


Test patch:
diff --git a/src/server/transport/socketio-simultaneous.test.ts b/src/server/transport/socketio-simultaneous.test.ts
index 0df4f45..df8aca5 100644
--- a/src/server/transport/socketio-simultaneous.test.ts
+++ b/src/server/transport/socketio-simultaneous.test.ts
@@ -29,10 +29,12 @@ type SocketIOTestAdapterOpts = SocketOpts & {
 
 class InMemoryAsync extends Async {
   db: InMemory;
+  delays: number[];
 
   constructor() {
     super();
     this.db = new InMemory();
+    this.delays = [];
   }
 
   async connect() {
@@ -40,7 +42,10 @@ class InMemoryAsync extends Async {
   }
 
   private sleep(): Promise<void> {
-    const interval = Math.round(Math.random() * 50 + 50);
+    const interval =
+      this.delays.length > 0
+        ? this.delays.shift()
+        : Math.round(Math.random() * 50 + 50);
     return new Promise((resolve) => void setTimeout(resolve, interval));
   }
 
@@ -156,51 +161,51 @@ jest.mock('koa-socket-2', () => {
   return MockIO;
 });
 
-const game = {
-  name: 'test',
-  setup: () => {
-    const G = {
-      players: {
-        '0': {
-          cards: ['card3'],
-        },
-        '1': {
-          cards: [],
+describe('simultaneous moves on server game', () => {
+  const game = {
+    name: 'test',
+    setup: () => {
+      const G = {
+        players: {
+          '0': {
+            cards: ['card3'],
+          },
+          '1': {
+            cards: [],
+          },
         },
-      },
-      cards: ['card0', 'card1', 'card2'],
-      discardedCards: [],
-    };
-    return G;
-  },
-  playerView: PlayerView.STRIP_SECRETS,
-  turn: {
-    activePlayers: { currentPlayer: { stage: 'A' } },
-    stages: {
-      A: {
-        moves: {
-          A: {
-            client: false,
-            move: (G, ctx: Ctx) => {
-              const card = G.players[ctx.playerID].cards.shift();
-              G.discardedCards.push(card);
+        cards: ['card0', 'card1', 'card2'],
+        discardedCards: [],
+      };
+      return G;
+    },
+    playerView: PlayerView.STRIP_SECRETS,
+    turn: {
+      activePlayers: { currentPlayer: { stage: 'A' } },
+      stages: {
+        A: {
+          moves: {
+            A: {
+              client: false,
+              move: (G, ctx: Ctx) => {
+                const card = G.players[ctx.playerID].cards.shift();
+                G.discardedCards.push(card);
+              },
             },
-          },
-          B: {
-            client: false,
-            ignoreStaleStateID: true,
-            move: (G, ctx: Ctx) => {
-              const card = G.cards.pop();
-              G.players[ctx.playerID].cards.push(card);
+            B: {
+              client: false,
+              ignoreStaleStateID: true,
+              move: (G, ctx: Ctx) => {
+                const card = G.cards.pop();
+                G.players[ctx.playerID].cards.push(card);
+              },
             },
           },
         },
       },
     },
-  },
-};
+  };
 
-describe('simultaneous moves on server game', () => {
   let app;
   let transport: SocketIOTestAdapter;
   let clientInfo;
@@ -456,3 +461,146 @@ describe('simultaneous moves on server game', () => {
     await db.wipe('matchID');
   });
 });
+
+describe('inauthentic clients', () => {
+  const game = {
+    setup: () => ({
+      0: 'foo',
+      1: 'bar',
+    }),
+    playerView: (G, _ctx, playerID) => ({ [playerID]: G[playerID] }),
+  };
+
+  let app;
+  let db: InMemoryAsync;
+  let transport: SocketIOTestAdapter;
+  let clientInfo: Map<string, Record<string, any>>;
+  let roomInfo: Map<string, Set<string>>;
+  let io;
+  const matchID = 'matchID';
+  const cred0 = 'password-0';
+  const cred1 = 'password-1';
+
+  beforeEach(async () => {
+    clientInfo = new Map();
+    roomInfo = new Map();
+
+    db = new InMemoryAsync();
+    await db.connect();
+
+    app = { context: { db, auth: new Auth() } };
+    transport = new SocketIOTestAdapter({ clientInfo, roomInfo });
+    transport.init(app, [ProcessGameConfig(game)]);
+    io = app.context.io;
+
+    // Create credentialed match.
+    const metadata = createMetadata({ game, unlisted: false, numPlayers: 2 });
+    metadata.players[0].credentials = cred0;
+    metadata.players[1].credentials = cred1;
+    const initialState = InitializeGame({ game, numPlayers: 2 });
+    await db.createMatch(matchID, { initialState, metadata });
+  });
+
+  afterEach(async () => {
+    await db.wipe(matchID);
+  });
+
+  test('inauthentic client is not added to clientInfo', async () => {
+    const inauthenticID = '0';
+    const socket = io.sockets.get(inauthenticID);
+
+    const args: Parameters<Master['onSync']> = ['matchID', '0', undefined];
+    const doneReceiving = socket.receive('sync', ...args);
+    // Is not added before awaiting the game master.
+    expect(clientInfo.get(inauthenticID)).toBeUndefined();
+    await doneReceiving;
+    // Is not added after awaiting the game master.
+    expect(clientInfo.get(inauthenticID)).toBeUndefined();
+  });
+
+  test('connected inauthentic client doesn’t receive authentic client’s sync', async () => {
+    const inauthenticID = '0';
+    const authenticID = '1';
+    const inauthenticSocket = io.sockets.get(inauthenticID);
+    const authenticSocket = io.sockets.get(authenticID);
+
+    // Call sync for both players
+    {
+      const args: Parameters<Master['onSync']> = ['matchID', '0', undefined];
+      await inauthenticSocket.receive('sync', ...args);
+    }
+    {
+      const args: Parameters<Master['onSync']> = ['matchID', '0', cred0];
+      await authenticSocket.receive('sync', ...args);
+    }
+
+    expect(clientInfo.get(inauthenticID)).toBeUndefined();
+
+    expect(clientInfo.get(authenticID)).toEqual({
+      matchID,
+      playerID: '0',
+      credentials: cred0,
+      socket: authenticSocket,
+    });
+
+    expect(inauthenticSocket.emit).not.toHaveBeenCalled();
+
+    expect(authenticSocket.emit).toHaveBeenCalledWith(
+      'sync',
+      matchID,
+      expect.objectContaining({
+        state: expect.objectContaining({
+          G: {
+            0: 'foo',
+          },
+        }),
+      })
+    );
+
+    const syncEmits = authenticSocket.emit.mock.calls.filter(
+      ([type]) => type === 'sync'
+    );
+    expect(syncEmits).toHaveLength(1);
+  });
+
+  test('inauthentic client doesn’t receive authentic client’s sync while still syncing', async () => {
+    const inauthenticID = '0';
+    const authenticID = '1';
+    const inauthenticSocket = io.sockets.get(inauthenticID);
+    const authenticSocket = io.sockets.get(authenticID);
+
+    // Make db#fetch in Master#onSync for first sync resolve after second sync.
+    db.delays = [200, 0];
+
+    // Call sync for both players
+    await Promise.all([
+      (async () => {
+        const args: Parameters<Master['onSync']> = ['matchID', '0', undefined];
+        await inauthenticSocket.receive('sync', ...args);
+      })(),
+      (async () => {
+        const args: Parameters<Master['onSync']> = ['matchID', '0', cred0];
+        await authenticSocket.receive('sync', ...args);
+      })(),
+    ]);
+
+    expect(clientInfo.get(inauthenticID)).toBeUndefined();
+
+    expect(authenticSocket.emit).toHaveBeenCalledWith(
+      'sync',
+      matchID,
+      expect.objectContaining({
+        state: expect.objectContaining({
+          G: {
+            0: 'foo',
+          },
+        }),
+      })
+    );
+
+    const syncEmits = authenticSocket.emit.mock.calls.filter(
+      ([type]) => type === 'sync'
+    );
+    expect(syncEmits).toHaveLength(1);
+  });
+});
diff --git a/src/server/transport/socketio.test.ts b/src/server/transport/socketio.test.ts
index 5319900..7112c9a 100644
--- a/src/server/transport/socketio.test.ts
+++ b/src/server/transport/socketio.test.ts
@@ -228,14 +228,6 @@ describe('connect / disconnect', () => {
   let roomInfo;
   let io;
 
-  const toObj = (m) => {
-    const o = {};
-    m.forEach((value, key) => {
-      o[key] = value;
-    });
-    return o;
-  };
-
   beforeAll(() => {
     clientInfo = new Map();
     roomInfo = new Map();
@@ -252,11 +244,11 @@ describe('connect / disconnect', () => {
     const args1: SyncArgs = ['matchID', '1', undefined, 2];
     await io.socket.receive('sync', ...args1);
 
-    expect(toObj(clientInfo)['0']).toMatchObject({
+    expect(clientInfo.get('0')).toMatchObject({
       matchID: 'matchID',
       playerID: '0',
     });
-    expect(toObj(clientInfo)['1']).toMatchObject({
+    expect(clientInfo.get('1')).toMatchObject({
       matchID: 'matchID',
       playerID: '1',
     });
@@ -266,30 +258,30 @@ describe('connect / disconnect', () => {
     io.socket.id = '0';
     await io.socket.receive('disconnect');
 
-    expect(toObj(clientInfo)['0']).toBeUndefined();
-    expect(toObj(clientInfo)['1']).toMatchObject({
+    expect(clientInfo.get('0')).toBeUndefined();
+    expect(clientInfo.get('1')).toMatchObject({
       matchID: 'matchID',
       playerID: '1',
     });
-    expect(toObj(roomInfo.get('matchID'))).toEqual({ '1': '1' });
+    expect([...roomInfo.get('matchID')]).toEqual(['1']);
   });
 
   test('unknown player disconnects', async () => {
     io.socket.id = 'unknown';
     await io.socket.receive('disconnect');
 
-    expect(toObj(clientInfo)['0']).toBeUndefined();
-    expect(toObj(clientInfo)['1']).toMatchObject({
+    expect(clientInfo.get('0')).toBeUndefined();
+    expect(clientInfo.get('1')).toMatchObject({
       matchID: 'matchID',
       playerID: '1',
     });
-    expect(toObj(roomInfo.get('matchID'))).toEqual({ '1': '1' });
+    expect([...roomInfo.get('matchID')]).toEqual(['1']);
   });
 
   test('1 disconnects', async () => {
     io.socket.id = '1';
     await io.socket.receive('disconnect');
-    expect(toObj(clientInfo)).toEqual({});
-    expect(toObj(roomInfo.get('matchID'))).toEqual({});
+    expect([...clientInfo.keys()]).toHaveLength(0);
+    expect(roomInfo.get('matchID')).toBeUndefined();
   });
 });