fix(core,server): resolve critical security, stability, and logic issues

src/core/execution/AttackExecution.ts

@@ -205,11 +205,14 @@ export class AttackExecution implements Execution {
 
     const survivors = this.attack.troops() - deaths;
     this._owner.addTroops(survivors);
+
+    // BUG-01: Check retreated() before delete() to preserve attack state for stat recording
+    const wasRetreated = this.attack.retreated();
     this.attack.delete();
     this.active = false;
 
     // Not all retreats are canceled attacks
-    if (this.attack.retreated()) {
+    if (wasRetreated) {
       // Record stats
       this.mg.stats().attackCancel(this._owner, this.target, survivors);
     }

src/core/execution/WinCheckExecution.ts

@@ -68,9 +68,14 @@ export class WinCheckExecution implements Execution {
       (this.mg.ticks() - this.mg.config().numSpawnPhaseTurns()) / 10;
     const numTilesWithoutFallout =
       this.mg.numLandTiles() - this.mg.numTilesWithFallout();
+    // Guard against division by zero when all land tiles have fallout
+    // Use integer cross-multiplication to avoid floating-point determinism issues
+    const exceedsThreshold =
+      numTilesWithoutFallout > 0 &&
+      max.numTilesOwned() * 100 >
+        this.mg.config().percentageTilesOwnedToWin() * numTilesWithoutFallout;
     if (
-      (max.numTilesOwned() / numTilesWithoutFallout) * 100 >
-        this.mg.config().percentageTilesOwnedToWin() ||
+      exceedsThreshold ||
       (this.mg.config().gameConfig().maxTimerValue !== undefined &&
         timeElapsed - this.mg.config().gameConfig().maxTimerValue! * 60 >= 0) ||
       timeElapsed >= WinCheckExecution.HARD_TIME_LIMIT_SECONDS
@@ -104,9 +109,14 @@ export class WinCheckExecution implements Execution {
       (this.mg.ticks() - this.mg.config().numSpawnPhaseTurns()) / 10;
     const numTilesWithoutFallout =
       this.mg.numLandTiles() - this.mg.numTilesWithFallout();
-    const percentage = (max[1] / numTilesWithoutFallout) * 100;
+    // Guard against division by zero when all land tiles have fallout
+    // Use integer cross-multiplication to avoid floating-point determinism issues
+    const exceedsThreshold =
+      numTilesWithoutFallout > 0 &&
+      max[1] * 100 >
+        this.mg.config().percentageTilesOwnedToWin() * numTilesWithoutFallout;
     if (
-      percentage > this.mg.config().percentageTilesOwnedToWin() ||
+      exceedsThreshold ||
       (this.mg.config().gameConfig().maxTimerValue !== undefined &&
         timeElapsed - this.mg.config().gameConfig().maxTimerValue! * 60 >= 0) ||
       timeElapsed >= WinCheckExecution.HARD_TIME_LIMIT_SECONDS

src/core/game/GameMap.ts

@@ -373,6 +373,12 @@ export class GameMapImpl implements GameMap {
     }
     return tiles;
   }
+  /**
+   * Flood-fill search starting from `tile`, collecting all connected tiles
+   * accepted by `filter`. Uses stack-based (DFS) traversal for performance
+   * — Array.pop() is O(1) vs Array.shift() O(n). The returned Set is
+   * identical regardless of traversal order.
+   */
   bfs(
     tile: TileRef,
     filter: (gm: GameMap, tile: TileRef) => boolean,
@@ -385,8 +391,7 @@ export class GameMapImpl implements GameMap {
     }
 
     while (q.length > 0) {
-      const curr = q.pop();
-      if (curr === undefined) continue;
+      const curr = q.pop()!;
       for (const n of this.neighbors(curr)) {
         if (!seen.has(n) && filter(this, n)) {
           seen.add(n);

src/server/ClientMsgRateLimiter.ts

@@ -5,12 +5,14 @@ const INTENTS_PER_SECOND = 10;
 const INTENTS_PER_MINUTE = 150;
 const MAX_INTENT_SIZE = 500;
 const MAX_CONFIG_INTENT_SIZE = 2000;
-const TOTAL_BYTES = 2 * 1024 * 1024; // 2MB per client
+const TOTAL_BYTES = 2 * 1024 * 1024; // 2MB per client per window
+const BYTE_WINDOW_MS = 60_000; // Reset byte counter every 60 seconds
 export type RateLimitResult = "ok" | "limit" | "kick";
 
 interface ClientBucket {
   perSecond: RateLimiter;
   perMinute: RateLimiter;
+  byteEvents: Array<{ at: number; bytes: number }>;
   totalBytes: number;
 }
 
@@ -24,6 +26,18 @@ export class ClientMsgRateLimiter {
     intentType?: string,
   ): RateLimitResult {
     const bucket = this.getOrCreate(clientID);
+
+    // Rolling-window byte accounting: evict events older than BYTE_WINDOW_MS
+    // so throughput is measured over a true sliding window instead of
+    // a fixed window that allows burst bypass at boundaries.
+    const now = Date.now();
+    const cutoff = now - BYTE_WINDOW_MS;
+    while (bucket.byteEvents.length > 0 && bucket.byteEvents[0].at < cutoff) {
+      const evicted = bucket.byteEvents.shift()!;
+      bucket.totalBytes -= evicted.bytes;
+    }
+
+    bucket.byteEvents.push({ at: now, bytes });
     bucket.totalBytes += bytes;
 
     if (bucket.totalBytes >= TOTAL_BYTES) return "kick";
@@ -68,6 +82,7 @@ export class ClientMsgRateLimiter {
         tokensPerInterval: INTENTS_PER_MINUTE,
         interval: "minute",
       }),
+      byteEvents: [],
       totalBytes: 0,
     };
     this.buckets.set(clientID, bucket);

src/server/GameServer.ts

@@ -48,7 +48,7 @@ export class GameServer {
 
   private maxGameDuration = 3 * 60 * 60 * 1000; // 3 hours
 
-  private disconnectedTimeout = 1 * 30 * 1000; // 30 seconds
+  private disconnectedTimeout = 30_000; // 30 seconds
 
   private turns: Turn[] = [];
   private intents: StampedIntent[] = [];
@@ -576,12 +576,10 @@ export class GameServer {
           }
         }
       } catch (error) {
-        this.log.info(
-          `error handline websocket request in game server: ${error}`,
-          {
-            clientID: client.clientID,
-          },
-        );
+        this.log.error(`error handling websocket request in game server`, {
+          clientID: client.clientID,
+          error: String(error),
+        });
       }
     });
     client.ws.on("close", () => {
@@ -826,7 +824,9 @@ export class GameServer {
       turn: pastTurn,
     } satisfies ServerTurnMessage);
     this.activeClients.forEach((c) => {
-      c.ws.send(msg);
+      if (c.ws.readyState === WebSocket.OPEN) {
+        c.ws.send(msg);
+      }
     });
   }
 

src/server/Worker.ts

@@ -106,7 +106,7 @@ export async function startWorker() {
 
   app.set("trust proxy", 3);
   app.use(compression());
-  app.use(express.json());
+  // Note: express.json({ limit: "5mb" }) is already applied above (line 51)
 
   app.use(
     express.static(path.join(__dirname, "../../out"), {
@@ -210,6 +210,63 @@ export async function startWorker() {
     res.json(game.gameInfo());
   });
 
+  // Add other endpoints from your original server
+  app.post("/api/start_game/:id", async (req, res) => {
+    // SEC-02: Verify the caller before touching any game state.
+    const authHeader = req.headers.authorization;
+    if (!authHeader?.startsWith("Bearer ")) {
+      return res
+        .status(401)
+        .json({ error: "Authorization header required to start a game" });
+    }
+    const token = authHeader.substring("Bearer ".length);
+    const result = await verifyClientToken(token, config);
+    if (result.type === "error") {
+      log.warn(`Invalid token for start_game: ${result.message}`);
+      return res.status(401).json({ error: "Invalid token" });
+    }
+
+    log.info(`starting private lobby with id ${req.params.id}`);
+    const game = gm.game(req.params.id);
+    if (!game) {
+      return res.status(404).json({ error: "Game not found" });
+    }
+    if (game.isPublic()) {
+      // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
+      const clientIP = req.ip || req.socket.remoteAddress || "unknown";
+      log.info(
+        `cannot start public game ${game.id}, game is public, ip: ${ipAnonymize(clientIP)}`,
+      );
+      return res.status(400).json({ error: "Cannot start public game" });
+    }
+
+    const callerPersistentId = result.persistentId;
+    const existingClientId =
+      game.getClientIdForPersistentId(callerPersistentId);
+    if (
+      existingClientId === null ||
+      existingClientId !== game.gameInfo().lobbyCreatorClientID
+    ) {
+      log.warn(
+        `Unauthorized start_game attempt by ${callerPersistentId.substring(0, 8)}`,
+      );
+      return res
+        .status(403)
+        .json({ error: "Only the lobby creator can start the game" });
+    }
+
+    try {
+      if (game.hasStarted()) {
+        return res.status(409).json({ error: "Game already started" });
+      }
+      game.start();
+      res.status(200).json({ success: true });
+    } catch (error) {
+      log.error(`Error starting game ${req.params.id}:`, error);
+      return res.status(500).json({ error: "Failed to start game" });
+    }
+  });
+
   app.get("/api/game/:id/exists", async (req, res) => {
     const lobbyId = req.params.id;
     res.json({

tests/AttackRetreatStats.test.ts

@@ -0,0 +1,89 @@
+import { AttackExecution } from "../src/core/execution/AttackExecution";
+import { RetreatExecution } from "../src/core/execution/RetreatExecution";
+import { SpawnExecution } from "../src/core/execution/SpawnExecution";
+import { Game, Player, PlayerInfo, PlayerType } from "../src/core/game/Game";
+import { GameID } from "../src/core/Schemas";
+import { setup } from "./util/Setup";
+
+let game: Game;
+const gameID: GameID = "game_id";
+let player1: Player;
+let player2: Player;
+
+describe("AttackRetreatStats", () => {
+  beforeEach(async () => {
+    game = await setup("plains", {}, [
+      new PlayerInfo("player1", PlayerType.Human, "player1", "player1"),
+      new PlayerInfo("player2", PlayerType.Human, "player2", "player2"),
+    ]);
+
+    player1 = game.player("player1");
+    player2 = game.player("player2");
+
+    game.addExecution(
+      new SpawnExecution(gameID, player1.info(), game.ref(50, 50)),
+    );
+    game.addExecution(
+      new SpawnExecution(gameID, player2.info(), game.ref(50, 55)),
+    );
+
+    while (game.inSpawnPhase()) {
+      game.executeNextTick();
+    }
+  });
+
+  test("should call attackCancel when attack is retreated", () => {
+    // Attack terraNullius so the attack doesn't end quickly from troop loss
+    const attackCancelSpy = vi.spyOn(game.stats(), "attackCancel");
+
+    game.addExecution(
+      new AttackExecution(player1.troops(), player1, game.terraNullius().id()),
+    );
+
+    // Execute one tick so the attack is initialized
+    game.executeNextTick();
+
+    const attacks = player1.outgoingAttacks();
+    expect(attacks.length).toBeGreaterThan(0);
+    const attackId = attacks[0].id();
+
+    // Add retreat execution immediately
+    game.addExecution(new RetreatExecution(player1, attackId));
+
+    // Execute ticks until the attack finishes retreating (cancelDelay=20 + a few more)
+    for (let i = 0; i < 50; i++) {
+      game.executeNextTick();
+    }
+
+    // Verify attackCancel was called (retreat stats recorded)
+    expect(attackCancelSpy).toHaveBeenCalled();
+    expect(attackCancelSpy).toHaveBeenCalledWith(
+      player1,
+      expect.anything(),
+      expect.any(Number),
+    );
+  });
+
+  test("should NOT call attackCancel when attack completes without retreat", () => {
+    expect(player1.sharesBorderWith(player2)).toBeTruthy();
+
+    const attackCancelSpy = vi.spyOn(game.stats(), "attackCancel");
+
+    // Start a full-troop attack against the other player (no retreat)
+    game.addExecution(
+      new AttackExecution(player1.troops(), player1, player2.id()),
+    );
+
+    // Execute until attack completes
+    let maxTicks = 5000;
+    while (player1.outgoingAttacks().length > 0 && maxTicks > 0) {
+      game.executeNextTick();
+      maxTicks--;
+    }
+    // Make sure the loop ended because the attack finished, not because we ran out of ticks.
+    expect(player1.outgoingAttacks().length).toBe(0);
+
+    // Verify attackCancel was NOT called
+    expect(attackCancelSpy).not.toHaveBeenCalled();
+  });
+});

tests/core/executions/WinCheckExecution.test.ts

@@ -87,6 +87,55 @@ describe("WinCheckExecution", () => {
   it("should return false for activeDuringSpawnPhase", () => {
     expect(winCheck.activeDuringSpawnPhase()).toBe(false);
   });
+
+  it("should not set winner via tile percentage when all land tiles have fallout (FFA)", () => {
+    const player = {
+      numTilesOwned: vi.fn(() => 100),
+      name: vi.fn(() => "P1"),
+    };
+    mg.players = vi.fn(() => [player]);
+    mg.numLandTiles = vi.fn(() => 100);
+    // All tiles have fallout => numTilesWithoutFallout === 0
+    mg.numTilesWithFallout = vi.fn(() => 100);
+    mg.config = vi.fn(() => ({
+      gameConfig: vi.fn(() => ({
+        gameMode: GameMode.FFA,
+        maxTimerValue: undefined,
+      })),
+      percentageTilesOwnedToWin: vi.fn(() => 80),
+      numSpawnPhaseTurns: vi.fn(() => 0),
+    }));
+    mg.ticks = vi.fn(() => 0);
+    mg.stats = vi.fn(() => ({ stats: () => ({}) }));
+    winCheck.init(mg, 0);
+    winCheck.checkWinnerFFA();
+    expect(mg.setWinner).not.toHaveBeenCalled();
+  });
+
+  it("should not set winner via tile percentage when all land tiles have fallout (Team)", () => {
+    const player = {
+      numTilesOwned: vi.fn(() => 100),
+      name: vi.fn(() => "P1"),
+      team: vi.fn(() => "Red"),
+    };
+    mg.players = vi.fn(() => [player]);
+    mg.numLandTiles = vi.fn(() => 100);
+    // All tiles have fallout => numTilesWithoutFallout === 0
+    mg.numTilesWithFallout = vi.fn(() => 100);
+    mg.config = vi.fn(() => ({
+      gameConfig: vi.fn(() => ({
+        gameMode: GameMode.Team,
+        maxTimerValue: undefined,
+      })),
+      percentageTilesOwnedToWin: vi.fn(() => 80),
+      numSpawnPhaseTurns: vi.fn(() => 0),
+    }));
+    mg.ticks = vi.fn(() => 0);
+    mg.stats = vi.fn(() => ({ stats: () => ({}) }));
+    winCheck.init(mg, 0);
+    winCheck.checkWinnerTeam();
+    expect(mg.setWinner).not.toHaveBeenCalled();
+  });
 });
 
 describe("WinCheckExecution - Nation Winners", () => {