fix(api): secure /api/archive_singleplayer_game endpoint with JWT validation#3954
Closed
berkelmali wants to merge 3 commits into
Closed
fix(api): secure /api/archive_singleplayer_game endpoint with JWT validation#3954berkelmali wants to merge 3 commits into
berkelmali wants to merge 3 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
WalkthroughAdds client and server Bearer-token authentication to singleplayer game archival: the client skips archival when not logged in, fetches a play token and posts gzip payload with Authorization; the server verifies the token and enforces that the record's player persistentID matches the authenticated identity. ChangesGame Archival Authentication
Sequence Diagram(s)sequenceDiagram
participant Client
participant LocalServer as Client LocalServer
participant ArchiveAPI as /api/archive_singleplayer_game
participant Worker as Worker.verifyClientToken
participant Archive as archive()
Client->>LocalServer: endGame()
LocalServer->>LocalServer: isLoggedIn()
alt not logged in
LocalServer-->>Client: skip archival
else logged in
par
LocalServer->>LocalServer: gzip(record)
LocalServer->>LocalServer: getPlayToken()
end
LocalServer->>ArchiveAPI: POST gzip with Authorization: Bearer token
ArchiveAPI->>Worker: verifyClientToken
alt token valid
Worker-->>ArchiveAPI: persistentID
ArchiveAPI->>ArchiveAPI: check record.player.persistentID == persistentID
alt matches
ArchiveAPI->>Archive: forward record with x-api-key
Archive-->>ArchiveAPI: success
else mismatch
ArchiveAPI-->>LocalServer: 403 Unauthorized user for this record
end
else invalid
ArchiveAPI-->>LocalServer: 401 Unauthorized
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/server/Worker.ts (1)
281-290: 💤 Low valueSmall terminology nit: 403 error message says "Unauthorized" but should say "Forbidden".
HTTP 401 is "Unauthorized" (authentication failure), HTTP 403 is "Forbidden" (authorization failure). The message "Unauthorized user for this record" is slightly confusing when returned with status 403.
Consider using "Forbidden" or "Not allowed to archive this record" for clarity.
📝 Suggested terminology fix
return res .status(403) - .json({ error: "Unauthorized user for this record" }); + .json({ error: "Forbidden: cannot archive another user's record" });🤖 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/Worker.ts` around lines 281 - 290, The response body for the 403 branch uses wording "Unauthorized user for this record" which conflicts with the 403 semantics; update the message returned in the res.status(403).json call to use a "Forbidden" phrasing (e.g., "Forbidden: user not allowed to archive this record" or "Forbidden user for this record") and ensure any log.warn context (tokenUser/recordUser) remains unchanged; locate the check around player.persistentID !== persistentID in the function handling the request and replace only the error string passed to res.status(403).json.
🤖 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/server/Worker.ts`:
- Around line 281-290: The response body for the 403 branch uses wording
"Unauthorized user for this record" which conflicts with the 403 semantics;
update the message returned in the res.status(403).json call to use a
"Forbidden" phrasing (e.g., "Forbidden: user not allowed to archive this record"
or "Forbidden user for this record") and ensure any log.warn context
(tokenUser/recordUser) remains unchanged; locate the check around
player.persistentID !== persistentID in the function handling the request and
replace only the error string passed to res.status(403).json.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a65233c-b319-421e-a71a-8e772121c74c
📒 Files selected for processing (2)
src/server/GameManager.tssrc/server/Worker.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/LocalServer.ts (1)
314-329: 💤 Low valueConsider logging HTTP error responses for easier debugging.
The
.catch()only handles network failures. If the server returns 401, 403, or 400, the fetch promise still resolves and these errors go unlogged. Withkeepalive: truethe page may unload before response arrives, but when it does arrive, having logs helps debug auth or validation failures.🔧 Optional: add response status check
Promise.all([compress(jsonString), getPlayToken()]) .then(([compressedData, token]) => { return fetch(`/${workerPath}/api/archive_singleplayer_game`, { method: "POST", headers: { "Content-Type": "application/json", "Content-Encoding": "gzip", Authorization: `Bearer ${token}`, }, body: compressedData, keepalive: true, // Ensures request completes even if page unloads }); }) + .then((response) => { + if (!response.ok) { + console.error( + `Archive request failed with status ${response.status}`, + ); + } + }) .catch((error) => { console.error("Failed to archive singleplayer game:", error); });🤖 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 314 - 329, The current Promise chain (Promise.all([compress(jsonString), getPlayToken()]) ... fetch(...)) only logs network failures, so HTTP error responses (non-2xx) from the archive_singleplayer_game endpoint go unreported; update the then-handler that performs fetch (after compress and getPlayToken) to inspect the fetch Response: check response.ok and, if false, read and log the response.status, response.statusText and response body (e.g., response.text() or json()) along with a contextual message; keep the existing catch to handle network errors but ensure the response check happens before finishing the chain so 400/401/403/500 errors are logged for debugging.
🤖 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 314-329: The current Promise chain
(Promise.all([compress(jsonString), getPlayToken()]) ... fetch(...)) only logs
network failures, so HTTP error responses (non-2xx) from the
archive_singleplayer_game endpoint go unreported; update the then-handler that
performs fetch (after compress and getPlayToken) to inspect the fetch Response:
check response.ok and, if false, read and log the response.status,
response.statusText and response body (e.g., response.text() or json()) along
with a contextual message; keep the existing catch to handle network errors but
ensure the response check happens before finishing the chain so 400/401/403/500
errors are logged for debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 305e3768-0a89-4c29-957c-f09ca1162b37
📒 Files selected for processing (1)
src/client/LocalServer.ts
added 3 commits
June 3, 2026 00:17
github-project-automation
Bot
moved this from Triage
to Complete
in OpenFront Release Management
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #3957
Description:
This PR patches a critical unauthenticated API endpoint at
/api/archive_singleplayer_gameinWorker.ts. The endpoint was accepting game records directly from the request body without any authentication and forwarding them to the central database, enabling anyone to spoof and pollute singleplayer statistics.A
verifyClientTokenauthentication middleware check has been added to the endpoint, ensuring that only authenticated requests from verified clients are processed and archived.Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
barfires