Repository: boardgameio/boardgame.io
Pull request #961 (2021-07-19 09:37:18)

fix: Don’t leak `STRIP_TRANSIENTS` action
#940 introduced an internal `STRIP_TRANSIENTS` action that strips error state from the final reducer output, so it can be used to trigger side effects. This action type was being included in the actions forwarded by the client to multiplayer transports. Because this action does not have a `payload` field, this caused the master to crash in various places were it assumed a `payload` object would be defined.

This PR adds logic to ignore the `STRIP_TRANSIENTS` object when forwarding actions to multiplayer transports.

For robustness, it also allows the master to gracefully handle cases where it receives an undefined `action` or `action.payload` as in theory crashing on undefined user input could be an attack vector.

---

CC: @shaoster
Reported [on Gitter](https://gitter.im/boardgame-io/General?at=60f37ddf82dd9050f5f8c428) by @kevinddchen


Patch:
diff --git a/src/client/client.ts b/src/client/client.ts
index 5f07265..028ff66 100644
--- a/src/client/client.ts
+++ b/src/client/client.ts
@@ -43,7 +43,10 @@ type ClientAction =
   | ActionShape.Sync
   | ActionShape.Update
   | ActionShape.Patch;
-type Action = CredentialedActionShape.Any | ClientAction;
+type Action =
+  | CredentialedActionShape.Any
+  | ActionShape.StripTransients
+  | ClientAction;
 
 export interface DebugOpt {
   target?: HTMLElement;
@@ -287,7 +290,10 @@ export class _ClientImpl<G extends any = any> {
       const baseState = store.getState();
       const result = next(action);
 
-      if (!('clientOnly' in action)) {
+      if (
+        !('clientOnly' in action) &&
+        action.type !== Actions.STRIP_TRANSIENTS
+      ) {
         this.transport.onAction(baseState, action);
       }
 
diff --git a/src/master/master.ts b/src/master/master.ts
index ddb5960..c1a692c 100644
--- a/src/master/master.ts
+++ b/src/master/master.ts
@@ -161,6 +161,10 @@ export class Master {
     matchID: string,
     playerID: string
   ): Promise<void | { error: string }> {
+    if (!credAction || !credAction.payload) {
+      return { error: 'missing action or action payload' };
+    }
+
     let metadata: Server.MatchData | undefined;
     if (StorageAPI.isSynchronous(this.storageAPI)) {
       ({ metadata } = this.storageAPI.fetch(matchID, { metadata: true }));
diff --git a/src/types.ts b/src/types.ts
index 7c0b306..85257e2 100644
--- a/src/types.ts
+++ b/src/types.ts
@@ -431,7 +431,9 @@ export namespace ActionShape {
   export type Redo = StripCredentials<CredentialedActionShape.Redo>;
   // Private type used only for internal error processing.
   // Included here to preserve type-checking of reducer inputs.
-  type _StripTransients = ReturnType<typeof ActionCreators.stripTransients>;
+  export type StripTransients = ReturnType<
+    typeof ActionCreators.stripTransients
+  >;
   export type Any =
     | MakeMove
     | GameEvent
@@ -443,7 +445,7 @@ export namespace ActionShape {
     | Undo
     | Redo
     | Plugin
-    | _StripTransients;
+    | StripTransients;
 }
 
 export namespace ActionPayload {


Test patch:
diff --git a/src/client/client.test.ts b/src/client/client.test.ts
index 7c5b54e..62c8125 100644
--- a/src/client/client.test.ts
+++ b/src/client/client.test.ts
@@ -22,6 +22,7 @@ import {
   gameEvent,
   patch,
 } from '../core/action-creators';
+import * as Actions from '../core/action-types';
 import Debug from './debug/Debug.svelte';
 import { error } from '../core/logger';
 import type { LogEntry, State, SyncInfo } from '../types';
@@ -144,7 +145,7 @@ describe('multiplayer', () => {
 
     beforeAll(() => {
       client = Client({
-        game: { moves: { A: () => {} } },
+        game: { moves: { A: () => {}, Invalid: () => INVALID_MOVE } },
         multiplayer: SocketIO({ server: host + ':' + port }),
       });
       client.start();
@@ -173,6 +174,18 @@ describe('multiplayer', () => {
       expect(client.transport.onAction).toHaveBeenCalled();
     });
 
+    test('strip transients action not sent to transport', () => {
+      jest.spyOn(client.transport, 'onAction');
+      const state = { G: {}, ctx: { phase: '' }, plugins: {} };
+      const filteredMetadata = [];
+      client.store.dispatch(sync({ state, filteredMetadata } as SyncInfo));
+      client.moves.Invalid();
+      expect(client.transport.onAction).not.toHaveBeenCalledWith(
+        expect.any(Object),
+        { type: Actions.STRIP_TRANSIENTS }
+      );
+    });
+
     test('Sends and receives chat messages', () => {
       jest.spyOn(client.transport, 'onAction');
       client.updatePlayerID('0');
diff --git a/src/master/master.test.ts b/src/master/master.test.ts
index fb2021b..95c88e3 100644
--- a/src/master/master.test.ts
+++ b/src/master/master.test.ts
@@ -233,6 +233,18 @@ describe('update', () => {
     ]);
   });
 
+  test('missing action', async () => {
+    const { error } = await master.onUpdate(null, 0, 'matchID', '0');
+    expect(sendAll).not.toHaveBeenCalled();
+    expect(error).toBe('missing action or action payload');
+  });
+
+  test('missing action payload', async () => {
+    const { error } = await master.onUpdate({}, 0, 'matchID', '0');
+    expect(sendAll).not.toHaveBeenCalled();
+    expect(error).toBe('missing action or action payload');
+  });
+
   test('invalid matchID', async () => {
     await master.onUpdate(action, 0, 'default:unknown', '1');
     expect(sendAll).not.toHaveBeenCalled();