Security: prevent winner stats spoofing and auth-gate singleplayer archive#4004
Security: prevent winner stats spoofing and auth-gate singleplayer archive#4004Radomir-Aksenenko wants to merge 11 commits into
Conversation
Two attack vectors closed: 1. allPlayersStats in ClientSendWinnerMessage was fully client-controlled, allowing a modified client (PoC browser extension confirmed) to archive arbitrary stats for any player account in multiplayer games. Fix: GameServer now ignores allPlayersStats entirely; Transport no longer sends it in multiplayer winner messages; the field is marked optional in the schema so LocalServer (offline singleplayer) continues to work. 2. POST /api/archive_singleplayer_game accepted game records with any persistentID without authentication, allowing unauthenticated clients to submit fake records for other players' accounts. Fix: endpoint now requires a valid JWT (Bearer token); the persistentID in the record must match the token owner. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
LocalServer.endGame() now attaches the Authorization: Bearer token to the POST /api/archive_singleplayer_game request, which the endpoint now requires after the security hardening in the previous commit. If the user is not logged in the header is omitted gracefully. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Verify that the winner message schema accepts/rejects correct inputs and that allPlayersStats is optional (not required on the wire). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPer-player stats are optional and dropped from multiplayer winner messages; server stores a normalized Winner and no longer copies client-supplied stats into archived player records. Singleplayer archive uploads use gzipped JSON and require a matching bearer JWT. ChangesWinner Messaging and Archive Security
Sequence DiagramsequenceDiagram
participant Client
participant LocalServer
participant ArchiveEndpoint
participant TokenVerifier
participant GameRecord
Client->>LocalServer: send winner (no allPlayersStats over network)
LocalServer->>ArchiveEndpoint: POST gzipped game (Authorization: Bearer token)
ArchiveEndpoint->>TokenVerifier: verifyClientToken(token)
TokenVerifier-->>ArchiveEndpoint: { persistentId } or Error
ArchiveEndpoint->>GameRecord: compare persistentId with game's singleplayer persistentID
alt Owner Match
ArchiveEndpoint->>GameRecord: store archive (player.stats = undefined)
else Owner Mismatch
ArchiveEndpoint-->>Client: 403 Forbidden
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/LocalServer.ts (1)
306-317: ⚡ Quick winDecouple optional auth lookup from archive upload.
Promise.allmakes archive upload fail whengetAuthHeader()fails, even though auth is optional in this client path. Prefer a non-blocking auth lookup fallback.Suggested change
- Promise.all([compress(jsonString), getAuthHeader()]) - .then(([compressedData, authHeader]) => { + Promise.all([ + compress(jsonString), + getAuthHeader().catch(() => ""), + ]) + .then(([compressedData, authHeader]) => { const headers: HeadersInit = { "Content-Type": "application/json", "Content-Encoding": "gzip", }; if (authHeader) { headers["Authorization"] = authHeader; }🤖 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/client/LocalServer.ts` around lines 306 - 317, The current Promise.all([compress(jsonString), getAuthHeader()]) causes the entire archive upload to fail if getAuthHeader() rejects; instead decouple auth lookup from the upload by awaiting or resolving compress(jsonString) first and then performing getAuthHeader() in a non-blocking way (e.g., call getAuthHeader().catch(() => undefined) or use Promise.allSettled and ignore a failed auth), then build headers (including Authorization only if authHeader is truthy) and call fetch to POST to `/${workerPath}/api/archive_singleplayer_game`; update the code around compress, getAuthHeader, headers, and the fetch invocation accordingly so a failed auth lookup does not abort the upload.
🤖 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.
Nitpick comments:
In `@src/client/LocalServer.ts`:
- Around line 306-317: The current Promise.all([compress(jsonString),
getAuthHeader()]) causes the entire archive upload to fail if getAuthHeader()
rejects; instead decouple auth lookup from the upload by awaiting or resolving
compress(jsonString) first and then performing getAuthHeader() in a non-blocking
way (e.g., call getAuthHeader().catch(() => undefined) or use Promise.allSettled
and ignore a failed auth), then build headers (including Authorization only if
authHeader is truthy) and call fetch to POST to
`/${workerPath}/api/archive_singleplayer_game`; update the code around compress,
getAuthHeader, headers, and the fetch invocation accordingly so a failed auth
lookup does not abort the upload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef96fc9b-9d17-435a-91f0-69570b1ce448
📒 Files selected for processing (6)
src/client/LocalServer.tssrc/client/Transport.tssrc/core/Schemas.tssrc/server/GameServer.tssrc/server/Worker.tstests/ClientSendWinnerSchema.test.ts
If getAuthHeader() rejects (e.g. token refresh network error), the upload must still proceed rather than being silently aborted. Compress first, then resolve auth with .catch(() => "") so a failed auth only results in an unauthenticated request, not a dropped upload. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@Celant Can you review please |
Celant
left a comment
Celant
left a comment
There was a problem hiding this comment.
Thanks for digging into this — the actual spoofing fix is right, handleWinner only keeping clientMsg.winner is exactly what we want, and the persistentID/JWT match checks out (sub gets transformed to uuid form on both sides, so logged-in archiving lines up). But there are a couple of things I need sorted before this can go in.
Singleplayer stats are getting dropped. onSendWinnerEvent strips allPlayersStats off every winner message, but that same sendMsg is what feeds LocalServer.onMessage for offline games — and clientMsg.allPlayersStats is the only place LocalServer ever gets stats from. So this.allPlayersStats ends up {} and endGame() archives stats: undefined. We just auth-gated the singleplayer archive specifically to store the player's stats, and now there's nothing to store. The comments saying "LocalServer handles stats separately" aren't true as written. Easiest fix is to only strip it on the multiplayer branch and keep it when isLocal.
Logged-out singleplayer won't archive anymore. Anonymous users have no JWT, so getAuthHeader() returns "" and they just get a 401. If we're fine cutting anonymous archiving, say so explicitly — but I don't think that's the intent. Worth a look.
Description doesn't match the code. It says archiveGame "reads stats from the server-side map populated during simulation," but it actually just sets stats: undefined with a TODO for a future tracker. Which is a reasonable trade-off, but it means multiplayer records lose per-player stats entirely for now — please fix the wording so nobody merges this thinking stats are preserved, and link an issue for the server-side tracker.
Minor: the tests only cover schema parsing, none of the security behavior (server ignoring client stats, the 401, the 403). Would be nice to have at least one of those covered given what this PR is. And the "allPlayersStats is stripped/ignored" test isn't really stripping anything, it's just absent — rename.
The singleplayer stats one is the blocker. Rest is cleanup + a question.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
- LocalServer: skip archive silently when no JWT instead of sending a request that would be rejected with 401. Anonymous singleplayer archiving is intentionally not supported — the endpoint requires auth to prevent spoofing. - ClientSendWinnerSchema.test: rename misleading "stripped/ignored" test to clarify the field is simply absent when not provided by the sender. - tests/server/WinnerSecurity: new test verifying that the server archives stats: undefined regardless of client-supplied allPlayersStats. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…m' into fix/winner-stats-auth-upstream
The previous commit stripped allPlayersStats from all winner messages, but LocalServer.endGame() relies on it as the sole source of per-player stats for singleplayer archiving. Only strip it on the multiplayer path (socket open); preserve it when isLocal so archived singleplayer records retain the player's stats. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/server/WinnerSecurity.test.ts (1)
1-130:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix Prettier violations in this file to unblock CI.
This file is currently failing the Prettier check in CI; please format it (for example,
npx prettier --write tests/server/WinnerSecurity.test.ts).🤖 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 `@tests/server/WinnerSecurity.test.ts` around lines 1 - 130, This test file fails Prettier; reformat tests/server/WinnerSecurity.test.ts to match the project's Prettier config (e.g., run npx prettier --write tests/server/WinnerSecurity.test.ts or apply your editor's auto-format) so the code around makeMockWs, makeClient and the "GameServer - winner message security" test case is consistently styled; commit the formatted file to resolve the CI Prettier check.
🧹 Nitpick comments (1)
tests/server/WinnerSecurity.test.ts (1)
85-96: ⚡ Quick winUse the shared
setup()helper instead of manualGameServer/Clientwiring.This test builds game/server clients directly; for
tests/**/*.test.ts, the suite should usesetup()fromtests/util/Setup.tsto keep test harness behavior consistent.As per coding guidelines "
tests/**/*.test.ts: Use thesetup()helper fromtests/util/Setup.tsto create test game instances and exercise core simulation directly".🤖 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 `@tests/server/WinnerSecurity.test.ts` around lines 85 - 96, Replace the manual GameServer and client wiring in WinnerSecurity.test.ts with the shared setup() helper from tests/util/Setup.ts: remove new GameServer(...) and makeClient(...) calls and instead call setup(...) to obtain the test game instance and client/ws pairs, then use the returned game object and clients (and start/advance the simulation per existing tests) so the test uses the standardized harness; specifically update the section creating GameServer, c1/c2 and ws1/ws2 to use setup() and its returned game/client/ws values and drop direct game.joinClient/start calls.
🤖 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 `@tests/server/WinnerSecurity.test.ts`:
- Around line 79-82: The test enables fake timers with vi.useFakeTimers() in the
beforeEach but the afterEach only restores mocks and clears timers; update the
teardown to call vi.useRealTimers() (add vi.useRealTimers() to afterEach
alongside vi.restoreAllMocks() and vi.clearAllTimers()) to avoid leaking
fake-timer mode into later tests, and ensure the test uses the common test setup
helper by calling setup() from the shared test helper (the setup() function in
tests/util/Setup.ts) in the beforeEach so the test follows the
tests/**/*.test.ts guideline.
---
Outside diff comments:
In `@tests/server/WinnerSecurity.test.ts`:
- Around line 1-130: This test file fails Prettier; reformat
tests/server/WinnerSecurity.test.ts to match the project's Prettier config
(e.g., run npx prettier --write tests/server/WinnerSecurity.test.ts or apply
your editor's auto-format) so the code around makeMockWs, makeClient and the
"GameServer - winner message security" test case is consistently styled; commit
the formatted file to resolve the CI Prettier check.
---
Nitpick comments:
In `@tests/server/WinnerSecurity.test.ts`:
- Around line 85-96: Replace the manual GameServer and client wiring in
WinnerSecurity.test.ts with the shared setup() helper from tests/util/Setup.ts:
remove new GameServer(...) and makeClient(...) calls and instead call setup(...)
to obtain the test game instance and client/ws pairs, then use the returned game
object and clients (and start/advance the simulation per existing tests) so the
test uses the standardized harness; specifically update the section creating
GameServer, c1/c2 and ws1/ws2 to use setup() and its returned game/client/ws
values and drop direct game.joinClient/start calls.
🪄 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: 19dc48e7-a7d3-407f-a43a-cf0486e427d4
📒 Files selected for processing (3)
src/client/LocalServer.tstests/ClientSendWinnerSchema.test.tstests/server/WinnerSecurity.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/ClientSendWinnerSchema.test.ts
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@Celant Ready |
Celant
left a comment
Celant
left a comment
There was a problem hiding this comment.
Re-reviewed — the stats fix and the anonymous-archive handling both look right now, and I like that there's a test actually asserting the fabricated stats get dropped. Two small things and then I'm happy to approve.
CodeRabbit's timer point is fair: beforeEach flips on vi.useFakeTimers() but afterEach only calls restoreAllMocks() and clearAllTimers(), neither of which turns fake timers back off. File isolation means it's not breaking anything today, but it'll bite the moment someone adds a second test here. Just add vi.useRealTimers() to the teardown.
(Ignore its other suggestion about routing this through setup() — that helper is for core-sim tests that need a real Game from map data. This is a server-side GameServer test, setup() doesn't fit.)
Other thing: the description still doesn't match the code. It says allPlayersStats "no longer exists on the wire" (it's optional now and still sent for singleplayer) and that archiveGame "reads stats from the server-side map populated during simulation" (it sets stats: undefined — the server-side tracker is still TODO, as your own comment says). Can you fix those two bullets so nobody reads this later and thinks multiplayer stats are still being recorded?
Fix the timer teardown and tidy the description and I'll approve.
afterEach cleared timers but left fake timers enabled, which would leak into any future test added to this file. Add vi.useRealTimers(). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@Celant Ready |
| .then((compressedData) => { | ||
| getAuthHeader() | ||
| .then(async (authHeader) => { | ||
| // Anonymous users have no JWT. The archive endpoint requires auth to |
There was a problem hiding this comment.
anonymous users do have jwts
4 participants
Description:
Fixes a vulnerability where a malicious client could send a crafted
winnerWebSocket message with fakeallPlayersStats, causing fabricated per-player statistics to be stored in game records.allPlayersStatsis now optional inClientSendWinnerSchema. It is stripped from multiplayer winner messages (the server ignores client-reported stats) but still sent for singleplayer, whereLocalServerneeds it for offline archiving.GameServer.archiveGame()no longer trusts client stats — it archivesstats: undefinedfor all players. A server-side stats tracker is still TODO (see code comment); multiplayer per-player stats are not recorded for now.POST /api/archive_singleplayer_gamenow requires a JWT Bearer token and verifies thepersistentIDmatches the token owner. Anonymous singleplayer games are no longer archived (intentional).LocalServer.endGame()attachesAuthorization: Bearer <token>when archiving singleplayer games, and skips the upload entirely if the user is logged out.Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
radomir3434