fix(game): patch Winner Spoofing vulnerability with strict majority consensus

[berkelmali]

Resolves #3958

Description:

This PR fixes a critical "Winner Spoofing" vulnerability in 1v1 matches. Previously, a malicious client could send a winner message to force an immediate win because the voting logic (potentialWinner.ips.size * 2 < activeUniqueIPs.size) failed to return in a 1v1 scenario (1 * 2 < 2 evaluates to false), bypassing intended consensus.

The logic has been updated to require a strict majority consensus (potentialWinner.ips.size * 2 <= activeUniqueIPs.size). In a 1v1 game, a single vote will no longer trigger an immediate win. Furthermore, to prevent legitimate wins from being stuck if a losing player rage-quits before voting, a new checkWinnerConsensus() routine runs on client disconnects to automatically declare a win if the remaining votes constitute a strict majority.

Please complete the following:

• I have added screenshots for all UI updates
• I process any text displayed to the user through translateText() and I've added it to the en.json file
• I have added relevant tests to the test directory
• I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced

Please put your Discord username so you can be contacted if a bug or regression is found:

barfires

[coderabbitai]

Walkthrough

Game termination now handles errors asynchronously via promise rejection, and winner consensus logic tightens the voting threshold to strict majority while re-evaluating winner status when clients disconnect to catch delayed consensus formations.

Changes

Game Completion and Winner Consensus

Layer / File(s)	Summary
Async error handling for game termination src/server/GameManager.ts	Game shutdown transitions from synchronous try/catch to promise-based .catch() error logging, allowing async end operations to fail gracefully without blocking tick logic.
Winner consensus logic and disconnect re-check src/server/GameServer.ts	Vote acceptance threshold tightened from equality (votes*2 == activeUniqueIPs) to strict majority (votes*2 > activeUniqueIPs). New checkWinnerConsensus() method re-evaluates all recorded winner votes against current active IPs and immediately finalizes a winner if threshold is reached. Disconnect handler now triggers this re-check to resolve consensus after client departures.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🎮 When players leave mid-game, the scores still fight,
No ties allowed—strict votes win outright,
Async errors whisper softly to the logs,
While consensus checks emerge from disconnect bogs,
Game completion flows with clarity and might! ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: patching a Winner Spoofing vulnerability with strict majority consensus, which aligns with the changeset.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The pull request description clearly describes the Winner Spoofing vulnerability being fixed and explains the changes to voting logic and disconnect handling.

_{✏️ 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/server/GameServer.ts`:
- Around line 609-612: The winner-consensus logic incorrectly uses
potentialWinner.ips (all historical voter IPs) instead of only currently
connected voters; update checkWinnerConsensus (and the consensus computation
where potentialWinner.ips is used) to compute votes by intersecting
potentialWinner.ips with the set of currently connected client IPs (e.g.,
this.clients / this.connectedClients / this.players map) and use that active-IP
count when comparing against votesNeeded so disconnected voters no longer count
toward consensus.

🪄 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: 7961b4f4-54d2-4e54-8cce-bfed4363b175

📥 Commits

Reviewing files that changed from the base of the PR and between 2e17fb5 and a47e16a.

📒 Files selected for processing (2)

• src/server/GameManager.ts
• src/server/GameServer.ts

[coderabbitai]

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Disconnect consensus must count only active voter IPs.

At Line 611, consensus is re-checked after disconnect. But at Lines 1246-1248, it uses all historical voter IPs (potentialWinner.ips.size), including disconnected voters. In 1v1, a client can vote and disconnect, and still force winner consensus.

🔧 Proposed fix

-      } else {
+      } else if (!this._hasEnded) {
         // Check if remaining clients have reached a winner consensus
         this.checkWinnerConsensus();
       }
@@
   private checkWinnerConsensus() {
-    if (this.winner !== null) {
+    if (this.winner !== null || this._hasEnded) {
       return;
     }

     const activeUniqueIPs = new Set(this.activeClients.map((c) => c.ip));
+    if (activeUniqueIPs.size === 0) {
+      return;
+    }

     for (const [winnerKey, potentialWinner] of this.winnerVotes.entries()) {
-      if (potentialWinner.ips.size * 2 > activeUniqueIPs.size) {
+      const activeVotes = [...potentialWinner.ips].filter((ip) =>
+        activeUniqueIPs.has(ip),
+      ).length;
+      if (activeVotes * 2 > activeUniqueIPs.size) {
         this.winner = potentialWinner.winner;
         this.log.info(
-          `Winner determined by ${potentialWinner.ips.size}/${activeUniqueIPs.size} active IPs after client disconnect`,
+          `Winner determined by ${activeVotes}/${activeUniqueIPs.size} active IPs after client disconnect`,
           {
             winnerKey: winnerKey,
           },
         );

Also applies to: 1244-1256

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/GameServer.ts` around lines 609 - 612, The winner-consensus logic
incorrectly uses potentialWinner.ips (all historical voter IPs) instead of only
currently connected voters; update checkWinnerConsensus (and the consensus
computation where potentialWinner.ips is used) to compute votes by intersecting
potentialWinner.ips with the set of currently connected client IPs (e.g.,
this.clients / this.connectedClients / this.players map) and use that active-IP
count when comparing against votesNeeded so disconnected voters no longer count
toward consensus.