diff --git a/src/client/LocalServer.ts b/src/client/LocalServer.ts index 0304224bff..5840adceab 100644 --- a/src/client/LocalServer.ts +++ b/src/client/LocalServer.ts @@ -18,7 +18,7 @@ import { decompressGameRecord, replacer, } from "../core/Util"; -import { getPersistentID } from "./Auth"; +import { getAuthHeader, getPersistentID } from "./Auth"; import { LobbyConfig } from "./ClientGameRunner"; import { GameSpeedDownIntentEvent, @@ -228,7 +228,7 @@ export class LocalServer { } if (clientMsg.type === "winner") { this.winner = clientMsg; - this.allPlayersStats = clientMsg.allPlayersStats; + this.allPlayersStats = clientMsg.allPlayersStats ?? {}; } } @@ -303,16 +303,23 @@ export class LocalServer { const jsonString = JSON.stringify(result.data, replacer); - compress(jsonString) - .then((compressedData) => { + getAuthHeader() + .then(async (authHeader) => { + // Anonymous users have no JWT. The archive endpoint requires auth to + // prevent spoofing, so skip silently rather than sending a request + // that would be rejected with 401. Logged-in singleplayer archiving + // is intentionally preserved; anonymous singleplayer is not archived. + if (!authHeader) return; + const compressedData = await compress(jsonString); return fetch(`/${workerPath}/api/archive_singleplayer_game`, { method: "POST", headers: { "Content-Type": "application/json", "Content-Encoding": "gzip", + Authorization: authHeader, }, body: compressedData, - keepalive: true, // Ensures request completes even if page unloads + keepalive: true, }); }) .catch((error) => { diff --git a/src/client/Transport.ts b/src/client/Transport.ts index fee60b966c..c6ab6b568e 100644 --- a/src/client/Transport.ts +++ b/src/client/Transport.ts @@ -574,11 +574,18 @@ export class Transport { private onSendWinnerEvent(event: SendWinnerEvent) { if (this.isLocal || this.socket?.readyState === WebSocket.OPEN) { - this.sendMsg({ - type: "winner", - winner: event.winner, - allPlayersStats: event.allPlayersStats, - } satisfies ClientSendWinnerMessage); + // allPlayersStats is stripped on the multiplayer path: the server does + // not trust client-reported stats (prevents fake stat injection). + // For local (singleplayer) games allPlayersStats must be kept — it is + // the only source of stats that LocalServer.endGame() archives. + const msg: ClientSendWinnerMessage = this.isLocal + ? { + type: "winner", + winner: event.winner, + allPlayersStats: event.allPlayersStats, + } + : { type: "winner", winner: event.winner }; + this.sendMsg(msg); } else { console.log( "WebSocket is not open. Current state:", diff --git a/src/core/Schemas.ts b/src/core/Schemas.ts index 39f207e19b..30ea21eb4f 100644 --- a/src/core/Schemas.ts +++ b/src/core/Schemas.ts @@ -649,7 +649,11 @@ export const ServerMessageSchema = z.discriminatedUnion("type", [ export const ClientSendWinnerSchema = z.object({ type: z.literal("winner"), winner: WinnerSchema, - allPlayersStats: AllPlayersStatsSchema, + // allPlayersStats is optional and intentionally ignored by the multiplayer + // server (GameServer.handleWinner). It is only used by LocalServer for + // offline singleplayer archiving. Clients must not rely on the server + // accepting or trusting this field in multiplayer. + allPlayersStats: AllPlayersStatsSchema.optional(), }); export const ClientHashSchema = z.object({ diff --git a/src/server/GameServer.ts b/src/server/GameServer.ts index 21279f856d..6ae069ce8e 100644 --- a/src/server/GameServer.ts +++ b/src/server/GameServer.ts @@ -23,6 +23,7 @@ import { ServerTurnMessage, StampedIntent, Turn, + Winner, } from "../core/Schemas"; import { createPartialGameRecord } from "../core/Util"; import { archive, finalizeGameRecord } from "./Archive"; @@ -66,7 +67,7 @@ export class GameServer { private lastPingUpdate = 0; - private winner: ClientSendWinnerMessage | null = null; + private winner: Winner | null = null; // Note: This can be undefined if accessed before the game starts. private gameStartInfo!: GameStartInfo; @@ -86,10 +87,8 @@ export class GameServer { private websockets: Set = new Set(); - private winnerVotes: Map< - string, - { winner: ClientSendWinnerMessage; ips: Set } - > = new Map(); + private winnerVotes: Map }> = + new Map(); private _hasEnded = false; @@ -1095,23 +1094,22 @@ export class GameServer { private archiveGame() { this.log.info("archiving game", { gameID: this.id, - winner: this.winner?.winner, + winner: this.winner, }); // Players must stay in the same order as the game start info. + // Stats are intentionally left undefined: client-reported allPlayersStats + // were removed from ClientSendWinnerMessage to prevent fake stat injection. + // A future server-side stats tracker should populate this field instead. const playerRecords: PlayerRecord[] = this.gameStartInfo.players.map( (player) => { - const stats = this.winner?.allPlayersStats[player.clientID]; - if (stats === undefined) { - this.log.warn(`Unable to find stats for clientID ${player.clientID}`); - } return { clientID: player.clientID, username: player.username, clanTag: player.clanTag, persistentID: this.allClients.get(player.clientID)?.persistentID ?? "", - stats, + stats: undefined, cosmetics: player.cosmetics, } satisfies PlayerRecord; }, @@ -1125,7 +1123,7 @@ export class GameServer { this.turns, this._startTime ?? 0, Date.now(), - this.winner?.winner, + this.winner ?? undefined, this.createdAt, this.visibleAt, ), @@ -1248,7 +1246,10 @@ export class GameServer { // Add client vote const winnerKey = JSON.stringify(clientMsg.winner); if (!this.winnerVotes.has(winnerKey)) { - this.winnerVotes.set(winnerKey, { ips: new Set(), winner: clientMsg }); + this.winnerVotes.set(winnerKey, { + ips: new Set(), + winner: clientMsg.winner, + }); } const potentialWinner = this.winnerVotes.get(winnerKey)!; potentialWinner.ips.add(client.ip); diff --git a/src/server/Worker.ts b/src/server/Worker.ts index 59626396a3..bb2ff2f06d 100644 --- a/src/server/Worker.ts +++ b/src/server/Worker.ts @@ -233,6 +233,24 @@ export async function startWorker() { app.post("/api/archive_singleplayer_game", async (req, res) => { try { + // Require a valid JWT so only the actual player can archive their own game. + // Without this, any unauthenticated client could submit fake records for + // arbitrary persistentIDs (see security audit finding #1). + const authHeader = req.headers.authorization; + if (!authHeader?.startsWith("Bearer ")) { + log.warn("archive_singleplayer_game: missing Authorization header"); + return res.status(401).json({ error: "Unauthorized" }); + } + const tokenResult = await verifyClientToken( + authHeader.substring("Bearer ".length), + ); + if (tokenResult.type === "error") { + log.warn( + `archive_singleplayer_game: invalid token — ${tokenResult.message}`, + ); + return res.status(401).json({ error: "Unauthorized" }); + } + const record = req.body; const result = PartialGameRecordSchema.safeParse(record); @@ -260,6 +278,16 @@ export async function startWorker() { return res.status(400).json({ error: "Invalid request" }); } + // Ensure the token owner matches the player in the record. + const recordPersistentID = result.data.info.players[0]?.persistentID; + if (recordPersistentID !== tokenResult.persistentId) { + log.warn( + `archive_singleplayer_game: persistentID mismatch — token: ${tokenResult.persistentId?.substring(0, 8)}, record: ${recordPersistentID?.substring(0, 8)}`, + { gameID: gameRecord.info.gameID }, + ); + return res.status(403).json({ error: "Forbidden" }); + } + log.info("archiving singleplayer game", { gameID: gameRecord.info.gameID, }); diff --git a/tests/ClientSendWinnerSchema.test.ts b/tests/ClientSendWinnerSchema.test.ts new file mode 100644 index 0000000000..55760107d3 --- /dev/null +++ b/tests/ClientSendWinnerSchema.test.ts @@ -0,0 +1,62 @@ +import { ClientSendWinnerSchema } from "../src/core/Schemas"; + +describe("ClientSendWinnerSchema", () => { + // ID must match /^[A-Za-z0-9]{8}$/ + const id1 = "AAAAAAAA"; + const id2 = "BBBBBBBB"; + const validWinner = ["player", id1, id2] as const; + + test("accepts a winner message without allPlayersStats", () => { + const result = ClientSendWinnerSchema.safeParse({ + type: "winner", + winner: validWinner, + }); + expect(result.success).toBe(true); + }); + + test("accepts a winner message with allPlayersStats (singleplayer path)", () => { + const result = ClientSendWinnerSchema.safeParse({ + type: "winner", + winner: validWinner, + allPlayersStats: { + [id1]: {}, + }, + }); + expect(result.success).toBe(true); + }); + + test("accepts a winner message with undefined winner (draw)", () => { + const result = ClientSendWinnerSchema.safeParse({ + type: "winner", + winner: undefined, + }); + expect(result.success).toBe(true); + }); + + test("rejects a message with wrong type", () => { + const result = ClientSendWinnerSchema.safeParse({ + type: "not_winner", + winner: validWinner, + }); + expect(result.success).toBe(false); + }); + + test("rejects a message with invalid winner format", () => { + const result = ClientSendWinnerSchema.safeParse({ + type: "winner", + winner: "invalid", + }); + expect(result.success).toBe(false); + }); + + test("allPlayersStats is absent (undefined) when not provided by sender", () => { + const result = ClientSendWinnerSchema.safeParse({ + type: "winner", + winner: validWinner, + }); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.allPlayersStats).toBeUndefined(); + } + }); +}); diff --git a/tests/server/WinnerSecurity.test.ts b/tests/server/WinnerSecurity.test.ts new file mode 100644 index 0000000000..cad4dd4d4c --- /dev/null +++ b/tests/server/WinnerSecurity.test.ts @@ -0,0 +1,127 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("../../src/core/Schemas", async () => { + const actual = (await vi.importActual("../../src/core/Schemas")) as any; + return { + ...actual, + GameStartInfoSchema: { + safeParse: (data: any) => ({ success: true, data: data }), + }, + ServerPrestartMessageSchema: { + safeParse: (data: any) => ({ success: true, data: data }), + }, + ClientMessageSchema: { + safeParse: (data: any) => ({ success: true, data: data }), + }, + }; +}); + +vi.mock("../../src/server/Archive", () => ({ + archive: vi.fn(), + finalizeGameRecord: (r: any) => r, +})); + +import { GameType } from "../../src/core/game/Game"; +import { archive } from "../../src/server/Archive"; +import { Client } from "../../src/server/Client"; +import { GameServer } from "../../src/server/GameServer"; + +function makeMockWs(ip = "1.2.3.4") { + const handlers: Record any> = {}; + return { + on: (event: string, handler: (...args: any[]) => any) => { + handlers[event] = handler; + }, + removeAllListeners: (_event: string) => {}, + send: vi.fn(), + close: vi.fn(), + readyState: 1, + trigger: (event: string, ...args: any[]) => handlers[event]?.(...args), + }; +} + +function makeClient( + clientID: string, + persistentID: string, + ip = "1.2.3.4", +): { client: Client; ws: ReturnType } { + const ws = makeMockWs(ip); + const client = new Client( + clientID, + persistentID, + null, + null, + undefined, + ip, + "TestUser", + null, + ws as any, + undefined, + undefined, + [], + ); + return { client, ws }; +} + +describe("GameServer - winner message security", () => { + let mockLogger: any; + + beforeEach(() => { + vi.useFakeTimers(); + mockLogger = { + child: vi.fn().mockReturnThis(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + }; + }); + + afterEach(() => { + vi.restoreAllMocks(); + vi.clearAllTimers(); + vi.useRealTimers(); + }); + + it("archives with undefined stats regardless of client-supplied allPlayersStats", async () => { + const game = new GameServer("test-game", mockLogger, Date.now(), { + gameType: GameType.Private, + } as any); + + const { client: c1, ws: ws1 } = makeClient("cid-1", "pid-1", "1.2.3.4"); + const { client: c2, ws: ws2 } = makeClient("cid-2", "pid-2", "5.6.7.8"); + game.joinClient(c1); + game.joinClient(c2); + game.start(); + + // Both clients vote for the same winner and attach fabricated stats. + // Majority threshold is met so archiveGame() is triggered. + const fabricatedStats = { + "cid-1": { kills: 9999, gold: 9999 } as any, + "cid-2": { kills: 9999, gold: 9999 } as any, + }; + + await ws1.trigger( + "message", + JSON.stringify({ + type: "winner", + winner: ["player", "cid-1"], + allPlayersStats: fabricatedStats, + }), + ); + await ws2.trigger( + "message", + JSON.stringify({ + type: "winner", + winner: ["player", "cid-1"], + allPlayersStats: fabricatedStats, + }), + ); + + expect(archive).toHaveBeenCalledOnce(); + const archivedRecord = (archive as ReturnType).mock + .calls[0][0]; + for (const player of archivedRecord.info.players) { + expect(player.stats).toBeUndefined(); + } + }); +});