fix(core,server): resolve critical security, stability, and logic issues#3796
fix(core,server): resolve critical security, stability, and logic issues#3796berkelmali wants to merge 5 commits intoopenfrontio:mainfrom
Conversation
|
|
|
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:
WalkthroughCapture retreat state before attack deletion; add division-by-zero guards in win checks; clarify GameMap.bfs JSDoc and traversal; implement 60s rolling byte window in client rate limiter; tighten websocket sends and host-cheats update; remove duplicate JSON parser and add authenticated POST /api/start_game/:id; add tests for retreat stats and win-check fallout edge cases. ChangesAttack / Win-check / Map BFS & Tests
Server: Rate Limiter, GameServer, Worker API & Middleware
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Worker as Worker
participant Auth as AuthService
participant Store as GameStore
participant Engine as GameEngine
Client->>Worker: POST /api/start_game/:id (Bearer token)
Worker->>Auth: Verify Bearer token
Auth-->>Worker: persistentId or error
Worker->>Store: Fetch game by id
Store-->>Worker: Game data or not found
alt exists && not public && persistentId == lobbyCreatorClientID
Worker->>Engine: startGame(game)
Engine-->>Worker: started
Worker-->>Client: 200 { started: true }
else unauthorized or invalid
Worker-->>Client: 4xx error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/execution/WinCheckExecution.ts (1)
1-137:⚠️ Potential issue | 🟡 MinorPrettier failure is blocking CI for this file.
Please run formatting and commit the result for
src/core/execution/WinCheckExecution.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/WinCheckExecution.ts` around lines 1 - 137, The file fails Prettier formatting; run your project's formatter (e.g., prettier --write) on src/core/execution/WinCheckExecution.ts and commit the reformatted file so CI passes. Locate the WinCheckExecution class (and its methods init, tick, checkWinnerFFA, checkWinnerTeam) and ensure spacing/line breaks conform to the repo Prettier configuration, then stage and push the formatted changes.
🧹 Nitpick comments (2)
src/server/GameServer.ts (1)
582-582: Use error log level for caught exceptions in this path.Line 582 handles an exception path but logs at
info, which makes production triage harder.♻️ Proposed adjustment
- this.log.info( - `error handling websocket request in game server: ${error}`, - { - clientID: client.clientID, - }, - ); + this.log.error(`error handling websocket request in game server`, { + clientID: client.clientID, + error: String(error), + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/GameServer.ts` at line 582, The caught-exception path currently logs the template string `error handling websocket request in game server: ${error}` at info level; change that call to use error-level logging (e.g., logger.error or this.logger.error) and ensure the error object (or error.stack) is passed so the message includes stack/details instead of only the string interpolation; update the log invocation where the template is used so it emits an error-level message with full error details.src/core/game/GameMap.ts (1)
376-381: JSDoc clarification improves understanding.The updated documentation accurately describes the stack-based (DFS) traversal and clarifies that results are order-invariant. This helps readers understand the performance trade-off and deterministic behavior.
💡 Optional: Consider renaming for clarity
The method name
bfs(breadth-first search) doesn't match the DFS-style implementation. While the JSDoc now clarifies this, consider renaming to something likefloodFillorconnectedTilesin a future refactor to avoid confusion.This is purely optional and out of scope for this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/GameMap.ts` around lines 376 - 381, The method is documented as using stack-based DFS but its name is still bfs, which is misleading; rename the function symbol bfs to a clearer name such as floodFill or connectedTiles (update all references/call sites and exported identifiers accordingly) and adjust any tests/imports to use the new name so implementation, API, and JSDoc are consistent.
🤖 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/core/execution/AttackExecution.ts`:
- Around line 208-217: Add a regression test that exercises the AttackExecution
flow to ensure mg.stats().attackCancel(owner, target, survivors) is called only
when attack.retreated() returns true; specifically, create two test cases that
instantiate an AttackExecution, set up one scenario where attack.retreated() is
true before delete() and one where it is false, run the code path that calls
this.attack.delete() and sets this.active = false, and assert that
stats().attackCancel(...) is called only in the true-retreated case and not in
the false case; target the AttackExecution class and the sequence around
attack.retreated(), this.attack.delete(), and mg.stats().attackCancel to prevent
regressions.
In `@src/core/execution/WinCheckExecution.ts`:
- Around line 72-75: The percent calculation uses floating-point division in
WinCheckExecution (the percentOwned computation and the similar check later)
which breaks determinism; replace those (percentOwned = (max.numTilesOwned() /
numTilesWithoutFallout) * 100 and the later `(owned/total)*100 > threshold`)
with integer cross-multiplication comparisons: use expressions like
`max.numTilesOwned() * 100` compared against `threshold *
numTilesWithoutFallout` (or `owned * 100 > threshold * total`) so you avoid any
floating-point arithmetic while preserving the same logic in the methods where
percentOwned and the later percentage check are computed.
In `@src/server/ClientMsgRateLimiter.ts`:
- Around line 30-37: The current fixed-window reset logic in
ClientMsgRateLimiter (uses bucket.totalBytes and bucket.byteWindowStart with
BYTE_WINDOW_MS) allows bursts around window edges; replace it with a
rolling-window byte accounting: record per-message (timestamp, bytes) entries on
the bucket (or maintain a deque/circular buffer) and, on each new message, evict
entries older than now - BYTE_WINDOW_MS and sum remaining bytes to check against
the 2MB limit, updating the buffer with the new sample; alternatively implement
a token-bucket/leaky-bucket tied to BYTE_WINDOW_MS so consumption decays
smoothly instead of resetting at byteWindowStart.
In `@src/server/Worker.ts`:
- Around line 259-260: The handler unconditionally returns success after calling
game.start(), which can be a no-op if the game is already started; update the
logic to detect whether start actually changed state (e.g., check a boolean
return from Game.start() or inspect game.started/isRunning property) and return
a 409 or a JSON { success: false, reason: "already_started" } when no-op occurs,
otherwise return { success: true } as before; modify the Game.start()
implementation if necessary to return a status flag and have the route use that
flag to decide the HTTP response.
---
Outside diff comments:
In `@src/core/execution/WinCheckExecution.ts`:
- Around line 1-137: The file fails Prettier formatting; run your project's
formatter (e.g., prettier --write) on src/core/execution/WinCheckExecution.ts
and commit the reformatted file so CI passes. Locate the WinCheckExecution class
(and its methods init, tick, checkWinnerFFA, checkWinnerTeam) and ensure
spacing/line breaks conform to the repo Prettier configuration, then stage and
push the formatted changes.
---
Nitpick comments:
In `@src/core/game/GameMap.ts`:
- Around line 376-381: The method is documented as using stack-based DFS but its
name is still bfs, which is misleading; rename the function symbol bfs to a
clearer name such as floodFill or connectedTiles (update all references/call
sites and exported identifiers accordingly) and adjust any tests/imports to use
the new name so implementation, API, and JSDoc are consistent.
In `@src/server/GameServer.ts`:
- Line 582: The caught-exception path currently logs the template string `error
handling websocket request in game server: ${error}` at info level; change that
call to use error-level logging (e.g., logger.error or this.logger.error) and
ensure the error object (or error.stack) is passed so the message includes
stack/details instead of only the string interpolation; update the log
invocation where the template is used so it emits an error-level message with
full error details.
🪄 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: e9ae74d7-8c0a-4385-934a-594e6fdfadd7
📒 Files selected for processing (6)
src/core/execution/AttackExecution.tssrc/core/execution/WinCheckExecution.tssrc/core/game/GameMap.tssrc/server/ClientMsgRateLimiter.tssrc/server/GameServer.tssrc/server/Worker.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Worker.ts`:
- Around line 213-263: There are extra blank lines surrounding the new
app.post("/api/start_game/:id", ...) handler causing Prettier to fail; remove
the duplicated blank line(s) immediately before the handler and immediately
after its closing brace so the block sits flush with the surrounding code, then
re-run formatting (npx prettier --write) to ensure no trailing/leading blank
lines remain; locate the handler by searching for
app.post("/api/start_game/:id") and verify related symbols used inside
(verifyClientToken, ipAnonymize, gm.game, game.start) are unchanged.
- Around line 215-260: In the app.post("/api/start_game/:id") handler move the
Bearer token verification (use verifyClientToken and the Authorization header
parsing) to the top of the handler before any game lookup, logging, or calls to
gm.game(...) and ipAnonymize; after successful token verification, then call
gm.game(req.params.id), handle 404, then check game.isPublic() and only then
perform owner validation via game.getClientIdForPersistentId and compare to
game.gameInfo().lobbyCreatorClientID, and finally call game.start(); also remove
or relocate the initial log.info that reveals the lobby existence/visibility
before auth so unauthenticated callers cannot probe state.
🪄 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: 4f0fb9c9-0a56-4d90-a107-f84155b092ad
📒 Files selected for processing (1)
src/server/Worker.ts
There was a problem hiding this comment.
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/core/execution/WinCheckExecution.ts`:
- Around line 71-78: Add unit tests covering the edge case when
numTilesWithoutFallout === 0 so the percentage-based win check
(exceedsThreshold) is skipped and no winner is declared via the tile-percentage
path: create tests that exercise WinCheckExecution logic for both FFA and Team
modes ensuring max.numTilesOwned() * 100 >
this.mg.config().percentageTilesOwnedToWin() * numTilesWithoutFallout is not
evaluated when numTilesWithoutFallout is zero and that no winner is set by that
path; use the same setup helpers used by existing tests (simulate all tiles
having fallout, call the win-check invocation that uses exceedsThreshold) and
assert no winner for FFA and for Team (except keep the existing Bot team test
unchanged).
🪄 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: 63929242-6c12-4c16-8d74-c971bf66234f
📒 Files selected for processing (3)
src/core/execution/WinCheckExecution.tssrc/server/GameServer.tssrc/server/Worker.ts
|
I have read the CLA Document and I hereby sign the CLA |
|
Hi @berkelmali, if you'd want to join the Discord Dev server, there's an invite link in the readme here: https://github.com/openfrontio/OpenFrontIO#-contributing It could come in handy when discussing this PR for example. CodeRabbit's feedback can be valid btw, it can help to fix things before a maintainer reviews the PR. I see in the Description you've applied one fix but the rest is still open. You can use it's ready-to-apply commits or AI prompts, or tell it why its feedback is wrong and it will learn from that. For unrelated things it can also create a seperate Issue if you ask it to. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/core/execution/AttackExecution.ts (1)
208-218:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing test for the retreat/cancel stats path.
This core logic change has no accompanying test. As per coding guidelines: "All changes to
src/core/must include tests".🤖 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/core/execution/AttackExecution.ts` around lines 208 - 218, Add a unit test for the AttackExecution behavior that ensures when an attack is marked as retreated the code records cancellation stats before the attack is deleted: instantiate an AttackExecution (or use the factory used in existing tests), mark its Attack instance so retreated() returns true, call the execution path that triggers delete() and setting active=false (the method under test in AttackExecution), and assert that mg.stats().attackCancel(...) was called with the expected _owner, target and survivors; also add a negative test where retreated() is false to verify attackCancel is not called. Use the same test harness/mocks used by other core tests to mock this.attack.retreated(), this.attack.delete(), and mg.stats() to verify call order and invocation.
🤖 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.
Duplicate comments:
In `@src/core/execution/AttackExecution.ts`:
- Around line 208-218: Add a unit test for the AttackExecution behavior that
ensures when an attack is marked as retreated the code records cancellation
stats before the attack is deleted: instantiate an AttackExecution (or use the
factory used in existing tests), mark its Attack instance so retreated() returns
true, call the execution path that triggers delete() and setting active=false
(the method under test in AttackExecution), and assert that
mg.stats().attackCancel(...) was called with the expected _owner, target and
survivors; also add a negative test where retreated() is false to verify
attackCancel is not called. Use the same test harness/mocks used by other core
tests to mock this.attack.retreated(), this.attack.delete(), and mg.stats() to
verify call order and invocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e4881886-1f27-4a9f-9c3a-0fba943eae10
📒 Files selected for processing (2)
src/core/execution/AttackExecution.tssrc/server/GameServer.ts
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/server/Worker.ts (2)
247-256: 💤 Low value
callerPersistentIdis always defined here — drop the optional chain.
result.type === "error"already returned at line 224, soresult.persistentIdis astring. The?.on line 255 just hides that fact and weakens the type signal.♻️ Tiny tidy-up
- const callerPersistentId = result.persistentId; + const callerPersistentId: string = result.persistentId; @@ log.warn( - `Unauthorized start_game attempt by ${callerPersistentId?.substring(0, 8)}`, + `Unauthorized start_game attempt by ${callerPersistentId.substring(0, 8)}`, );🤖 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 247 - 256, The optional chaining on callerPersistentId in the log call is unnecessary because result.type === "error" was already handled and result.persistentId is a string; update the log.warn invocation to call substring directly on callerPersistentId (remove the `?.`) so the code reflects the non-nullable type; locate this in the start_game handling around the variables callerPersistentId, existingClientId, game.getClientIdForPersistentId and game.gameInfo().lobbyCreatorClientID, and adjust the log.warn message accordingly.
243-263: ⚡ Quick winRace condition exists in theory, but current implementation is already safe — consider defensive wrapping only if future changes risk idempotency.
A race exists between the
hasStarted()check at line 243 and thegame.start()call at line 262:GameManager.tick()(line 127) could callgame.start()via timeout in that window. However,game.start()already guards against double-invocation (line 726 checksif (this._hasStarted || this._hasEnded) return;), so a second call is a safe no-op.If you want defensive protection against future changes to
start()logic, wrap the call intry-catch. Otherwise, the current code is safe.Optional defensive guard (if desired)
+ try { game.start(); res.status(200).json({ success: true }); + } catch (error) { + log.error(`error starting game ${game.id} via API:`, error); + res.status(500).json({ error: "Failed to start game" }); + }🤖 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 243 - 263, Race window between game.hasStarted() and game.start() could let GameManager.tick() start the game concurrently; add a defensive guard by re-checking state and catching errors before calling start: after verifying caller identity (using getClientIdForPersistentId and game.gameInfo().lobbyCreatorClientID), if game.hasStarted() return 409, otherwise call game.start() inside a try-catch and on catch log the error and respond with a 409/500 as appropriate; alternatively perform an atomic re-check (if (!game.hasStarted()) try { game.start() } catch (e) { ... }) to ensure idempotent, safe behavior even if start() changes later.tests/AttackRetreatStats.test.ts (1)
77-86: ⚡ Quick winTiny gap: the second test passes even if the attack never finishes.
If
outgoingAttacks().lengthnever reaches0within 5000 ticks (e.g., due to a future regression that makes the attack stall), the loop simply exits becausemaxTickshit0, andattackCancelwould correctly not have been called — so the test still passes. Adding a guard that the loop exited because the attack actually completed makes the negative case meaningful.♻️ Suggested guard
// Execute until attack completes let maxTicks = 5000; while (player1.outgoingAttacks().length > 0 && maxTicks > 0) { game.executeNextTick(); maxTicks--; } + // Make sure the loop ended because the attack finished, not because we ran out of ticks. + expect(player1.outgoingAttacks().length).toBe(0); + // Verify attackCancel was NOT called expect(attackCancelSpy).not.toHaveBeenCalled();🤖 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/AttackRetreatStats.test.ts` around lines 77 - 86, The test's loop can end due to maxTicks expiring rather than the attack completing; after the while loop that uses player1.outgoingAttacks(), maxTicks and game.executeNextTick(), add an explicit guard that verifies the loop exited because the attack finished (e.g., assert player1.outgoingAttacks().length === 0 or fail if maxTicks === 0) before asserting attackCancelSpy.not.toHaveBeenCalled(); reference outgoingAttacks(), maxTicks, attackCancelSpy, and game.executeNextTick() to locate where to add this check.src/server/ClientMsgRateLimiter.ts (1)
32-41: 💤 Low valueOptional: track a running total to skip the per-message
reduce.Each
check()call walks the wholebyteEventsarray twice (eviction + reduce). With the current rate caps the array stays small, so this is purely cosmetic, but a running counter keeps things O(1) per message and reads a bit cleaner.♻️ Optional running-total variant
interface ClientBucket { perSecond: RateLimiter; perMinute: RateLimiter; byteEvents: Array<{ at: number; bytes: number }>; + totalBytes: number; } @@ const now = Date.now(); const cutoff = now - BYTE_WINDOW_MS; while (bucket.byteEvents.length > 0 && bucket.byteEvents[0].at < cutoff) { - bucket.byteEvents.shift(); + const evicted = bucket.byteEvents.shift()!; + bucket.totalBytes -= evicted.bytes; } bucket.byteEvents.push({ at: now, bytes }); + bucket.totalBytes += bytes; - const totalBytes = bucket.byteEvents.reduce((sum, e) => sum + e.bytes, 0); - if (totalBytes >= TOTAL_BYTES) return "kick"; + if (bucket.totalBytes >= TOTAL_BYTES) return "kick"; @@ byteEvents: [], + totalBytes: 0,🤖 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/ClientMsgRateLimiter.ts` around lines 32 - 41, The current check() implementation walks bucket.byteEvents twice (eviction loop + reduce) which is unnecessary; add and maintain a running counter (e.g., bucket.totalBytes) initialized when a bucket is created, decrement it when evicting old events in the while loop (subtract the shifted event.bytes) and increment it when pushing the new {at, bytes} event, then replace the reduce with a constant-time comparison against TOTAL_BYTES; update any bucket-creation logic to set totalBytes = 0 and ensure the eviction path correctly keeps totalBytes in sync to avoid drift.
🤖 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/ClientMsgRateLimiter.ts`:
- Around line 32-41: The current check() implementation walks bucket.byteEvents
twice (eviction loop + reduce) which is unnecessary; add and maintain a running
counter (e.g., bucket.totalBytes) initialized when a bucket is created,
decrement it when evicting old events in the while loop (subtract the shifted
event.bytes) and increment it when pushing the new {at, bytes} event, then
replace the reduce with a constant-time comparison against TOTAL_BYTES; update
any bucket-creation logic to set totalBytes = 0 and ensure the eviction path
correctly keeps totalBytes in sync to avoid drift.
In `@src/server/Worker.ts`:
- Around line 247-256: The optional chaining on callerPersistentId in the log
call is unnecessary because result.type === "error" was already handled and
result.persistentId is a string; update the log.warn invocation to call
substring directly on callerPersistentId (remove the `?.`) so the code reflects
the non-nullable type; locate this in the start_game handling around the
variables callerPersistentId, existingClientId, game.getClientIdForPersistentId
and game.gameInfo().lobbyCreatorClientID, and adjust the log.warn message
accordingly.
- Around line 243-263: Race window between game.hasStarted() and game.start()
could let GameManager.tick() start the game concurrently; add a defensive guard
by re-checking state and catching errors before calling start: after verifying
caller identity (using getClientIdForPersistentId and
game.gameInfo().lobbyCreatorClientID), if game.hasStarted() return 409,
otherwise call game.start() inside a try-catch and on catch log the error and
respond with a 409/500 as appropriate; alternatively perform an atomic re-check
(if (!game.hasStarted()) try { game.start() } catch (e) { ... }) to ensure
idempotent, safe behavior even if start() changes later.
In `@tests/AttackRetreatStats.test.ts`:
- Around line 77-86: The test's loop can end due to maxTicks expiring rather
than the attack completing; after the while loop that uses
player1.outgoingAttacks(), maxTicks and game.executeNextTick(), add an explicit
guard that verifies the loop exited because the attack finished (e.g., assert
player1.outgoingAttacks().length === 0 or fail if maxTicks === 0) before
asserting attackCancelSpy.not.toHaveBeenCalled(); reference outgoingAttacks(),
maxTicks, attackCancelSpy, and game.executeNextTick() to locate where to add
this check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7013e87e-ddbc-41f4-9a42-24a1dfa3493c
📒 Files selected for processing (4)
src/server/ClientMsgRateLimiter.tssrc/server/Worker.tstests/AttackRetreatStats.test.tstests/core/executions/WinCheckExecution.test.ts
This commit addresses the following: - SEC-02: Added auth to start_game endpoint - SEC-01: Rate limiter now uses sliding window for total bytes - BUG-02: Added readyState check to endTurn WebSocket broadcast - BUG-06: Guarded against division by zero in WinCheck execution - BUG-01: Fixed attack retreat stats recording sequence - BUG-03: Fixed typo in websocket error log - SUG-08: Removed duplicate express.json() middleware - SUG-06: Improved disconnectedTimeout readability - BUG-07: Clarified bfs() DFS behavior via JSDoc
- Fix Prettier formatting for WinCheckExecution.ts and Worker.ts - Replace floating-point division with integer cross-multiplication in WinCheckExecution for determinism - Change error log level from info to error for caught exceptions in GameServer.ts
- Move auth check before game lookup in start_game handler (prevents state probing) - Return 409 when game already started - Replace fixed-window with rolling-window byte accounting in rate limiter - Add regression tests for attack retreat/cancel stats path - Add tests for zero-fallout edge case in WinCheckExecution (FFA + Team)
- Remove unnecessary optional chain on callerPersistentId - Wrap game.start() in try-catch for race condition safety - Use running totalBytes counter in rate limiter for O(1) check - Add guard assertion in retreat test to verify attack completion
|
Hey @VariableVince, thanks for the heads-up! I've now addressed all CodeRabbit feedback across all review rounds:
Also rebased onto latest |
|
Why are we reverting this commit? |
Accidental revert of commit e0cc50d during audit changes. The if-guard must be removed so deselecting Host cheats actually disables all active cheats.
|
Sorry about that @babyboucher! The revert was unintentional - it happened during the initial audit changes when I modified |
|
@berkelmali I suggest you create a branch and merge from there and not from your main, it will be cleaner when you work on multiple pr My take on the different issues: |
|
please create a PR for each bug |
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
|
Closing this PR as requested by maintainers. Changes have been split into individual PRs:
Thanks for the feedback @VariableVince! |
6 participants
Description:
This PR addresses critical security vulnerabilities, stability issues, and logic bugs identified during a comprehensive code audit:
start_gameAPI endpoint to prevent unauthorized game startsreadyStatecheck toendTurnWebSocket broadcast to prevent crashes on closed connectionsWinCheckExecutionwhen all land tiles have fallout; replaced floating-point division with integer cross-multiplication for determinismretreated()is now checked beforedelete()to preserve attack stateexpress.json()middlewaredisconnectedTimeoutreadability with named constantbfs()DFS behavior via JSDocinfotoerrorfor caught exceptions in GameServer websocket handlerPlease complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
barfires