Conversation
…b elo vals always - Refactored Elo rating changes to store deltas instead of absolute values for player ratings in the game object. - Modified the `end` method to fetch players' old Elo ratings from the database asynchronously. - Adjusted the logic for updating players' Elo ratings based on game outcomes, ensuring correct calculations for both draws and wins.
Use RTT-based time sync with safe fallback and logging hooks to prevent large offset drift after tab suspension.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Server fixes
Documents a security flaw where attackers can hijack logged-in user sessions by exploiting the shared disconnectedPlayers map namespace. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Only accept UUID format rejoinCodes (containing dashes). This blocks attackers from using MongoDB accountIds as rejoinCodes to hijack logged-in users' disconnected sessions. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Validate rejoinCode is a string before calling .includes() to prevent TypeError crashes from malicious payloads. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- validateSecret.js: Add type check before MongoDB query
- googleAuth.js: Validate secret is string before findOne
- eloRank.js: Add type check for secret parameter
- mapHome.js: Move type validation BEFORE the database query
Attackers could send {"secret": {"$ne": null}} to bypass auth
and authenticate as any user in the database.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- eloRank.js: Add typeof check for username parameter - leaderboard.js: Add typeof check for myUsername query param Prevents NoSQL injection via ?username[$ne]=foo query strings. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This pull request implements several improvements to the game's infrastructure including a more accurate time synchronization system, critical ELO calculation fixes, and enhanced security measures against NoSQL injection attacks.
Changes:
- Implemented a round-trip time (RTT) based WebSocket time synchronization system to replace the less accurate single-timestamp fallback method
- Modified ELO calculation to store deltas instead of absolute ratings and fetch current ELO from database at game end to handle concurrent game scenarios
- Added NoSQL injection prevention by validating that user inputs (secret, username, rejoinCode) are strings before database queries
- Added validation to reject MongoDB ObjectId format rejoin codes, only accepting UUID format
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ws/ws.js | Added WebSocket handler for "timeSync" messages and updated ELO change calculation to store deltas instead of absolute values |
| ws/classes/Player.js | Enhanced rejoin code validation to only accept UUID format (containing dashes), rejecting MongoDB ObjectIds |
| ws/classes/Game.js | Made end() method async, fetched current ELO from database at game end, added oldElos field, reduced timer from 5000ms to 1000ms when all players finish |
| serverUtils/validateSecret.js | Added type validation to prevent NoSQL injection by ensuring secret is a string |
| components/home.js | Implemented client-side RTT-based time synchronization with periodic polling and visibility change handling |
| api/map/mapHome.js | Added NoSQL injection prevention for secret parameter |
| api/leaderboard.js | Added NoSQL injection prevention for username parameter |
| api/googleAuth.js | Added type check for secret parameter |
| api/eloRank.js | Added type checks for username and secret parameters |
| .gitignore | Added tmpclaude* pattern to ignore temporary files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else { | ||
| setElo(this.accountIds.p2, changes.newRating2, { draw: true, oldElo: p2OldElo }); | ||
| setElo(this.accountIds.p2, p2NewElo, { draw: true, oldElo: p2OldElo }); | ||
| } |
There was a problem hiding this comment.
The closing brace on line 808 closes the if(p2obj) block that starts on line 804, but the indentation and structure suggest it should close the outer if(draw) block. This causes the else on line 806 to be incorrectly paired with if(p2obj) instead of being part of the p2obj conditional logic. The correct structure should have a closing brace after line 808 to close the if(draw) block before the else if(winner) on line 809.
| } | |
| } |
| useEffect(() => { | ||
| if (!ws) return; | ||
| timeSyncRef.current = { bestRtt: Infinity, lastSyncAt: 0, lastServerNow: 0 }; | ||
| setTimeOffset(0); | ||
| sendTimeSync(); | ||
| const interval = setInterval(() => { | ||
| sendTimeSync(); | ||
| }, 30000); | ||
| return () => clearInterval(interval); | ||
| }, [ws]) | ||
|
|
||
| useEffect(() => { | ||
| const handleVisibility = () => { | ||
| if (document.visibilityState === "visible") { | ||
| sendTimeSync(); | ||
| } | ||
| }; | ||
| document.addEventListener("visibilitychange", handleVisibility); | ||
| return () => document.removeEventListener("visibilitychange", handleVisibility); | ||
| }, [ws]) |
There was a problem hiding this comment.
The sendTimeSync function is referenced in the dependency arrays of the useEffect hooks on lines 1020-1029 and 1031-1039, but it's not included in the dependency arrays. Since sendTimeSync is a function defined in the component body that depends on ws, it should either be wrapped in useCallback with appropriate dependencies, or the dependency arrays should be updated. Without this, the function references in the intervals and event listeners may become stale.
| let p2NewElo = p2OldElo; | ||
| // elo changes | ||
| if(this.eloChanges) { | ||
| if(this.eloChanges && p1OldElo && p2OldElo) { |
There was a problem hiding this comment.
The condition check includes p1OldElo && p2OldElo to ensure both ELO values exist, but the condition on line 860 also requires these values before saving the duel. However, if the database queries on lines 777-778 fail or return null/undefined ELO values, but the oldElos fallback also doesn't exist (e.g., if the game was created before this field was added), the ELO changes will be skipped silently without logging or notifying players. Consider adding logging when ELO changes are skipped due to missing values to help debug issues.
| const updateTimeOffsetFromSync = (serverNow, clientSentAt) => { | ||
| if (!serverNow || !clientSentAt) return; | ||
| const now = Date.now(); | ||
| const rtt = Math.max(0, now - clientSentAt); | ||
| const offset = serverNow - (clientSentAt + rtt / 2); | ||
| const sync = timeSyncRef.current; | ||
| const tooOld = now - sync.lastSyncAt > 60000; | ||
| const betterRtt = rtt <= sync.bestRtt + 25; | ||
| if (sync.lastSyncAt === 0 || betterRtt || tooOld) { | ||
| const prevBestRtt = sync.bestRtt; | ||
| sync.bestRtt = Math.min(sync.bestRtt, rtt); | ||
| sync.lastSyncAt = now; | ||
| sync.lastServerNow = serverNow; | ||
| if (window.debugTimeSync) { | ||
| console.log("[TimeSync] update", { | ||
| offset, | ||
| rtt, | ||
| serverNow, | ||
| clientSentAt, | ||
| prevBestRtt | ||
| }); | ||
| } | ||
| setTimeOffset(offset); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The updateTimeOffsetFromSync function is called from the WebSocket message handler (line 1442), but it's not wrapped in useCallback. This means a new function instance is created on every render. While this doesn't directly break functionality because it's called synchronously from the message handler, it's inconsistent with best practices. Consider wrapping it in useCallback for consistency and to prevent potential issues if it's used elsewhere in the future.
No description provided.