Conversation
WalkthroughWinner-vote aggregation logic has been modified to group votes using a SHA-256 hash derived from both the winner choice and all player statistics, applying deterministic JSON serialization with a custom replacer function, replacing the previous direct stringification approach. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/GameServer.ts`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15a471d0-6a01-4965-bc50-81854c5bb650
📒 Files selected for processing (1)
src/server/GameServer.ts
| const winnerKey = createHash("sha256") | ||
| .update( | ||
| JSON.stringify( | ||
| { | ||
| winner: clientMsg.winner, | ||
| allPlayersStats: clientMsg.allPlayersStats, | ||
| }, | ||
| replacer, | ||
| ), | ||
| ) | ||
| .digest("hex"); |
There was a problem hiding this comment.
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.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
1 participant
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
Description:
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME