Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions src/client/LocalServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -228,7 +228,7 @@ export class LocalServer {
}
if (clientMsg.type === "winner") {
this.winner = clientMsg;
this.allPlayersStats = clientMsg.allPlayersStats;
this.allPlayersStats = clientMsg.allPlayersStats ?? {};
}
}

Expand Down Expand Up @@ -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
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anonymous users do have jwts

// 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) => {
Expand Down
17 changes: 12 additions & 5 deletions src/client/Transport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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:",
Expand Down
6 changes: 5 additions & 1 deletion src/core/Schemas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down
27 changes: 14 additions & 13 deletions src/server/GameServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
ServerTurnMessage,
StampedIntent,
Turn,
Winner,
} from "../core/Schemas";
import { createPartialGameRecord } from "../core/Util";
import { archive, finalizeGameRecord } from "./Archive";
Expand Down Expand Up @@ -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;
Expand All @@ -86,10 +87,8 @@ export class GameServer {

private websockets: Set<WebSocket> = new Set();

private winnerVotes: Map<
string,
{ winner: ClientSendWinnerMessage; ips: Set<string> }
> = new Map();
private winnerVotes: Map<string, { winner: Winner; ips: Set<string> }> =
new Map();

private _hasEnded = false;

Expand Down Expand Up @@ -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;
},
Expand All @@ -1125,7 +1123,7 @@ export class GameServer {
this.turns,
this._startTime ?? 0,
Date.now(),
this.winner?.winner,
this.winner ?? undefined,
this.createdAt,
this.visibleAt,
),
Expand Down Expand Up @@ -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);
Expand Down
28 changes: 28 additions & 0 deletions src/server/Worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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,
});
Expand Down
62 changes: 62 additions & 0 deletions tests/ClientSendWinnerSchema.test.ts
Original file line number Diff line number Diff line change
@@ -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();
}
});
});
127 changes: 127 additions & 0 deletions tests/server/WinnerSecurity.test.ts
Original file line number Diff line number Diff line change
@@ -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<string, (...args: any[]) => 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<typeof makeMockWs> } {
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();
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.

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<typeof vi.fn>).mock
.calls[0][0];
for (const player of archivedRecord.info.players) {
expect(player.stats).toBeUndefined();
}
});
});
Loading