Skip to content

stats #3800

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
evanpelle wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

stats #3800

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 14 additions & 2 deletions src/server/GameServer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import ipAnonymize from "ip-anonymize";
import { createHash } from "node:crypto";
import { Logger } from "winston";
import WebSocket from "ws";
import { z } from "zod";
Expand All @@ -24,7 +25,7 @@ import {
StampedIntent,
Turn,
} from "../core/Schemas";
import { createPartialGameRecord } from "../core/Util";
import { createPartialGameRecord, replacer } from "../core/Util";
import { archive, finalizeGameRecord } from "./Archive";
import { Client } from "./Client";
import { ClientMsgRateLimiter } from "./ClientMsgRateLimiter";
Expand Down Expand Up @@ -1200,7 +1201,18 @@ export class GameServer {
client.reportedWinner = clientMsg.winner;

// Add client vote
const winnerKey = JSON.stringify(clientMsg.winner);
const winnerKey = createHash("sha256")
.update(
JSON.stringify(
{
winner: clientMsg.winner,
allPlayersStats: clientMsg.allPlayersStats,
},
replacer,
),
)
.digest("hex");
Comment on lines +1204 to +1214
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 29, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Do not canonicalize winner votes before grouping.

Using replacer on Line 1211 removes key-order differences, so a modified client can submit the same winner payload with reordered properties and still be counted with honest votes. This vote path intentionally used raw JSON.stringify ordering as part of the tamper signal, so keep the hash if you want, but hash the raw serialization instead.

🔧 Proposed fix 
     const winnerKey = createHash("sha256")
       .update(
-        JSON.stringify(
-          {
-            winner: clientMsg.winner,
-            allPlayersStats: clientMsg.allPlayersStats,
-          },
-          replacer,
-        ),
+        JSON.stringify({
+          winner: clientMsg.winner,
+          allPlayersStats: clientMsg.allPlayersStats,
+        }),
       )
       .digest("hex");

Based on learnings, "In voting systems, using JSON.stringify for object comparison can be intentionally kept to detect modified clients - if property ordering differs from expected, it may indicate client tampering and should be treated as separate votes for security reasons."

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/server/GameServer.ts` around lines 1204 - 1214, The code that computes
winnerKey currently canonicalizes the payload using the replacer before hashing,
which removes property-order differences used as a tamper signal; in the
createHash(...) block that builds winnerKey (referencing createHash, winnerKey,
replacer, clientMsg.winner, clientMsg.allPlayersStats, and JSON.stringify), stop
passing the replacer to JSON.stringify so the raw serialization (including
original property order) is hashed instead—keep the same
createHash(...).digest("hex") flow but use JSON.stringify({...}) without the
replacer to preserve ordering-based tamper detection.


if (!this.winnerVotes.has(winnerKey)) {
this.winnerVotes.set(winnerKey, { ips: new Set(), winner: clientMsg });
}
Expand Down
Loading