Turnstile #2572
Turnstile #2572
Conversation
WalkthroughThis PR integrates Cloudflare Turnstile token verification for client authentication, refactors username and clan tag censorship logic, introduces YouTube tutorials in the win modal for early-stage players, and applies various UI refinements and configuration updates across the codebase. Changes
Sequence DiagramsequenceDiagram
participant Client as Client Browser
participant Server as Game Server
participant Cloudflare as Cloudflare<br/>Turnstile API
Client->>Client: Generate Turnstile token<br/>(user completes CAPTCHA)
Client->>Server: WebSocket: join message<br/>+ turnstileToken payload
activate Server
Server->>Server: Extract client IP<br/>& token from message
Server->>Cloudflare: POST /siteverify<br/>(token, secret, IP)
activate Cloudflare
Cloudflare->>Cloudflare: Validate token &<br/>verify challenge
Cloudflare-->>Server: {success: true/false,<br/>error-codes?}
deactivate Cloudflare
alt Token Valid
Server->>Server: Create Client instance<br/>proceed with game join
Server-->>Client: Connection accepted
else Token Invalid or Missing
Server->>Server: Log warning/error
Server-->>Client: WebSocket close<br/>(code 1002, "Unauthorized")
else Verification Error
Server->>Server: Log error,<br/>close connection
Server-->>Client: WebSocket close
end
deactivate Server
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/Main.ts (1)
464-485: Add guard clause for Turnstile to prevent crashes on script load failure
getTurnstileTokendirectly callswindow.turnstile.render()andwindow.turnstile.execute()without checking if the Turnstile script loaded successfully. If the CDN request fails or is slow, these calls will throw and aborthandleJoinLobbywithout a clear error path.Add a guard clause at the start of
getTurnstileToken:async function getTurnstileToken(): Promise<string> { const config = await getServerConfigFromClient(); + + if (!window.turnstile || typeof window.turnstile.render !== "function") { + alert("Something went wrong, please refresh the page and try again."); + throw new Error("Turnstile not loaded"); + } + const widgetId = window.turnstile.render("#turnstile-container", {
🧹 Nitpick comments (8)
src/client/Main.ts (1)
48-51: Makewindow.turnstileoptional and tidy the commented initDeclaring
turnstileas a requiredanydoes not match reality: the script may not be loaded yet or could fail. Consider:
- Making it optional and minimally typed, e.g.
turnstile?: { render: (...args: any[]) => string; execute: (...args: any[]) => void; }, so TypeScript forces a runtime presence check.- Either implement
initTurnstile()or remove the commented call before merging, to avoid dead-code noise.Also applies to: 112-114
src/core/Schemas.ts (1)
526-537: Consider makingturnstileTokenoptional for more robust joinsAdding
turnstileToken: z.string().nullable()keeps the client/server types in sync, but it also means any join message (including tests, tools, or older clients) must now send this property or be rejected by validation.If you want to be defensive while rolling this out, consider:
turnstileToken: z.string().nullable().optional(),and then treating
undefinedthe same asnullon the server. If you’re confident everything that constructsClientJoinMessageis updated, the current stricter form is fine; just double-check tests and any manual JSON payloads.src/client/graphics/layers/WinModal.ts (1)
4-8: Early YouTube tutorial flow looks solid; only minor polish possible
- The new
!this.isWin && getGamesPlayed() < 3branch is a nice way to show help only to new players, and the initialisInIframe()guard avoids iframe/embed problems.- Binding the
iframesrctothis.isVisibleis a good trick to prevent the video from playing in the background when the modal is hidden.- The new translation key
win_modal.youtube_tutorialandwin_modal.join_servermatch the usages here.If you want small polish later, you could:
- Add an explicit return type
renderYoutubeTutorial(): TemplateResultfor clarity.- Hoist the YouTube video ID to a constant so it’s easier to change without scanning markup.
Also applies to: 112-114, 126-146, 260-277
src/client/index.html (1)
93-99: Turnstile script and container wiring look consistent with client usageThe Turnstile script is loaded with
async/defer, and the centered#turnstile-containermatches howgetTurnstileToken()renders the widget.One small optional improvement: consider adding an accessible description or
aria-hidden="true"to the empty container if you find it shows up in some screen readers before Turnstile renders its own accessible UI, but this is not blocking.Also applies to: 211-214
src/core/configuration/DefaultConfig.ts (1)
82-88: Turnstile env accessors are consistent; consider stricter validation for secretsThese two methods follow the same pattern as other env-based getters, so the implementation is fine. The only thing to think about is that returning
""for a missing Turnstile secret can hide a misconfigured server; you may want a central config check (at startup) that asserts these env vars are set in environments where Turnstile is required, instead of relying on downstream code to infer it from an empty string. Longer term, a plain data config object composed into the server rather than an abstract base class would also make this type of validation easier to test and reason about.src/core/validations/username.ts (1)
11-12: Clan‑tag aware censoring is well factored; add tests for the documented casesThe new
censorNameWithClanTaghelper reads cleanly: you sanitize once, derive the clan tag in original case, strip it from the name, run profanity checks on clan and base name separately, and only reattach the clan tag when it is non‑profane. The behavior matches the comment examples and composes nicely with existing utilities (sanitize,getClanTagOriginalCase,fixProfaneUsername), without adding new class hierarchies.To lock this in, I would add a small test table that covers at least the examples in the doc comment (no clan, good clan + good/bad name, bad clan + good/bad name), plus one or two edge cases (multiple bracketed segments, extra whitespace) so future refactors cannot accidentally break clan/tag handling.
Also applies to: 48-95
src/server/Turnstile.ts (1)
1-53: Typed union result and Turnstile call look goodThe
verifyTurnstileTokenhelper is small and clear: you use a discriminated union for the outcome, handle the “no token” case up front, call the Turnstilesiteverifyendpoint with a JSON body, and mapsuccesspluserror-codesinto eitherapprovedorrejected, with all unexpected failures falling into theerrorbranch. This is a nice, idiomatic TypeScript-style helper without extra classes.The only thing to keep in mind is how the
reasonfield is consumed; if it is ever shown to end users, you may want to log the detailederror-codes/ exception separately and return a more generic message to clients.src/core/Util.ts (1)
341-357: Shared clan tag matcher simplifies parsingRefactoring
getClanTagto use a sharedclanMatchhelper and addinggetClanTagOriginalCasegives you one source of truth for the clan‑tag regex, while still returning uppercase tags fromgetClanTagto match existing callers. The early bracket check keepsclanMatchcheap for non‑clan names, and the regex itself is straightforward.I would just make sure there are unit tests that cover both functions (mixed‑case tags, no tags, multiple tags), so future changes to the regex cannot silently change behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
resources/lang/en.json(2 hunks)src/client/ClientGameRunner.ts(1 hunks)src/client/Main.ts(6 hunks)src/client/Transport.ts(1 hunks)src/client/graphics/PlayerIcons.ts(1 hunks)src/client/graphics/layers/AdTimer.ts(1 hunks)src/client/graphics/layers/NameLayer.ts(1 hunks)src/client/graphics/layers/WinModal.ts(3 hunks)src/client/index.html(2 hunks)src/core/GameRunner.ts(2 hunks)src/core/Schemas.ts(1 hunks)src/core/Util.ts(2 hunks)src/core/configuration/Config.ts(1 hunks)src/core/configuration/DefaultConfig.ts(1 hunks)src/core/configuration/DevConfig.ts(2 hunks)src/core/configuration/PreprodConfig.ts(1 hunks)src/core/configuration/ProdConfig.ts(1 hunks)src/core/game/UnitImpl.ts(2 hunks)src/core/validations/username.ts(2 hunks)src/server/MapPlaylist.ts(0 hunks)src/server/Turnstile.ts(1 hunks)src/server/Worker.ts(2 hunks)tests/util/TestServerConfig.ts(1 hunks)webpack.config.js(1 hunks)
💤 Files with no reviewable changes (1)
- src/server/MapPlaylist.ts
🧰 Additional context used
🧠 Learnings (14)
📚 Learning: 2025-11-12T23:11:34.445Z
Learnt from: MaxHT0x
Repo: openfrontio/OpenFrontIO PR: 2262
File: src/core/configuration/DefaultConfig.ts:806-832
Timestamp: 2025-11-12T23:11:34.445Z
Learning: In src/core/configuration/DefaultConfig.ts, JSDoc documentation for configuration methods should not be added inline, as it was requested that documentation be placed elsewhere in the codebase.
Applied to files:
src/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/core/configuration/DevConfig.tssrc/core/configuration/PreprodConfig.tssrc/core/configuration/ProdConfig.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as `z.infer<typeof PlayerStatsSchema>` where PlayerStatsSchema has `.optional()` applied at the object level, making PlayerStats a union type that already includes undefined (PlayerStats | undefined).
Applied to files:
src/core/Schemas.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
Applied to files:
src/core/game/UnitImpl.ts
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
src/core/game/UnitImpl.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/core/GameRunner.ts
📚 Learning: 2025-10-26T15:37:07.732Z
Learnt from: GlacialDrift
Repo: openfrontio/OpenFrontIO PR: 2298
File: src/client/graphics/layers/TerritoryLayer.ts:200-210
Timestamp: 2025-10-26T15:37:07.732Z
Learning: In GameImpl.ts lines 124-139, team assignment logic varies by number of teams: when numPlayerTeams < 8, teams are assigned ColoredTeams values (Red, Blue, Yellow, Green, Purple, Orange, Teal); when numPlayerTeams >= 8, teams are assigned generic string identifiers like "Team 1", "Team 2", etc., which are not members of ColoredTeams.
Applied to files:
src/core/GameRunner.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/configuration/DevConfig.tssrc/client/graphics/layers/AdTimer.ts
📚 Learning: 2025-05-19T06:00:38.007Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:125-134
Timestamp: 2025-05-19T06:00:38.007Z
Learning: In StatsImpl.ts, unused parameters in boat/stats-related methods are intentionally kept for future use and shouldn't be removed.
Applied to files:
src/core/configuration/DevConfig.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/core/configuration/DevConfig.tssrc/client/ClientGameRunner.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/configuration/DevConfig.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/graphics/layers/WinModal.tssrc/client/Main.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/ClientGameRunner.ts
📚 Learning: 2025-08-16T10:52:08.292Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 884
File: resources/lang/en.json:456-461
Timestamp: 2025-08-16T10:52:08.292Z
Learning: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files. Regular PRs should only update en.json when adding or modifying translation keys.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
resources/lang/en.json
🧬 Code graph analysis (5)
src/server/Worker.ts (1)
src/server/Turnstile.ts (1)
verifyTurnstileToken(1-53)
src/core/validations/username.ts (1)
src/core/Util.ts (2)
sanitize(173-177)getClanTagOriginalCase(347-350)
src/core/GameRunner.ts (3)
src/core/game/Game.ts (1)
PlayerInfo(404-418)src/core/Util.ts (1)
sanitize(173-177)src/core/validations/username.ts (1)
censorNameWithClanTag(68-95)
src/client/graphics/layers/WinModal.ts (1)
src/client/Utils.ts (2)
getGamesPlayed(222-229)translateText(92-147)
src/client/Main.ts (1)
src/core/configuration/ConfigLoader.ts (1)
getServerConfigFromClient(28-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (17)
src/client/graphics/layers/AdTimer.ts (1)
4-4: Ad timeout constant change looks consistent with comment
AD_SHOW_TICKS = 5 * 60 * 10matches the// 5 minutescomment if the game runs at 10 ticks per second, and the rest of the control flow is unchanged, so behavior should just delay ad cleanup from the previous threshold. Please just double‑check that the assumed tick rate is still 10/sec so the timeout really is 5 minutes.src/client/graphics/layers/NameLayer.ts (1)
439-446: Alliance icon wrapper update keeps layout stableSetting
allianceWrapper.style.flexShrink = "0"here matches the behavior increateAllianceProgressIconand helps keep the alliance icon from shrinking in the flex.player-iconscontainer when its size is updated. Change looks correct and consistent with the shared icon helper.src/client/graphics/PlayerIcons.ts (1)
164-173: Make alliance progress icon non‑shrinking in flex rowsAdding
wrapper.style.flexShrink = "0"ensures the alliance progress icon keeps its intended size when used inside flex layouts (like the nameplate icon row). This pairs well with the width/height settings and matches how the wrapper is later updated inNameLayer. Looks good.src/core/game/UnitImpl.ts (2)
71-80: AddingFactoryto build stats keeps structures consistentIncluding
UnitType.Factoryhere makes build stats match how City/Port/etc. are already tracked and matches the destroy switch below. Looks good and keeps stats consistent for all base structures.
188-201: Capture/lose stats forFactorynow match destroy statsAdding
UnitType.Factoryhere makes capture/lose stats consistent with other structures and with the destroy logic already indelete(). Ownership values at this point are correct (old owner still in_owner, new owner passed in), so the stats calls look safe.resources/lang/en.json (1)
270-270: i18n keys align with UI usage and translation workflow
host_modal.compact_mapnow matches the existingsingle_modal.compact_maplabel, which keeps wording consistent.- New keys
win_modal.join_serverandwin_modal.youtube_tutorialline up with the strings used inWinModal.ts.Only
en.jsonwas updated, which matches the project’s rule to keep non-English files for dedicated translation PRs. Based on learnings, this looks correct.Also applies to: 533-535
webpack.config.js (1)
140-142: EnsureTURNSTILE_SITE_KEYis set in all build environmentsWire-up via
DefinePluginlooks consistent with your other env vars. Just make sure:
TURNSTILE_SITE_KEYis defined wherever you build the client, otherwise the injected value will beundefinedand the frontend will try to render Turnstile with a missing site key.- You’re comfortable exposing the site key client-side (which is normally fine for Turnstile; only the secret key must stay server-side).
src/client/Transport.ts (1)
379-389: Join payload Turnstile token wiring looks correctPlumbing
turnstileTokenthroughjoinGame:
- Matches the updated
ClientJoinMessageschema (string | null).- Keeps local/singleplayer flows safe, since they can pass
nulland the server won’t be involved.Just make sure every
LobbyConfigcreator setsturnstileTokenexplicitly (typicallynulloutside the Turnstile path) so you don’t end up withundefinedat runtime.src/core/configuration/DevConfig.ts (2)
26-28: Game creation rate increased from 5s to 20s.This change quadruples the delay between game creation cycles. If this is intentional for dev environment tuning, looks fine.
7-12: Correct Cloudflare Turnstile test keys.These match Cloudflare's official test keys:
1x00000000000000000000AA(always passes) for the site key and1x0000000000000000000000000000000AAfor the secret key. These generate dummy tokens and are appropriate for the dev environment.src/core/configuration/Config.ts (1)
30-31: Clean interface extension for Turnstile config.The new methods are placed correctly in
ServerConfigsince Turnstile verification happens server-side. Return types are appropriately non-nullable.src/core/GameRunner.ts (1)
49-58: Good separation: local player uncensored, others censored with clan-tag awareness.The logic correctly:
- Shows local player their own uncensored (but sanitized) name
- Censors other players' names using
censorNameWithClanTag, which handles clan tags separately from usernamesThis is cleaner than the previous approach and properly handles the case where a clan tag might be profane but the username is not (or vice versa).
src/client/ClientGameRunner.ts (1)
61-61: Appropriate nullable type for turnstile token.The
string | nulltype is correct since the token may not be available in all scenarios (e.g., reconnection wherelastTurn > 0).src/server/Worker.ts (2)
29-29: Import looks correct.
394-414: Good: Turnstile verification only on initial join, not reconnects.The
clientMsg.lastTurn === 0check correctly skips Turnstile verification for reconnecting clients. This avoids requiring users to re-verify when recovering from disconnects.tests/util/TestServerConfig.ts (1)
7-12: Test stub kept in sync with ServerConfigAdding
turnstileSiteKeyandturnstileSecretKeyhere matches the existing “throw by default” pattern inTestServerConfig, so tests will fail loudly if they rely on these without configuring a proper test double. This is consistent and safe.src/core/Util.ts (1)
307-320: Emoji table update is safeThis is a simple data tweak to
emojiTable; the structure stays the same andflattenedEmojiTablewill pick up the new entries automatically. No issues here.
| turnstileSiteKey(): string { | ||
| return "TODO"; | ||
| } |
There was a problem hiding this comment.
Placeholder value "TODO" will break Turnstile in preprod.
The turnstileSiteKey() returns "TODO", which is not a valid Cloudflare Turnstile site key. The client-side Turnstile widget will fail to initialize with this value.
Before merging, replace this with the actual preprod Turnstile site key or use Cloudflare's test keys if preprod should bypass verification.
🤖 Prompt for AI Agents
In src/core/configuration/PreprodConfig.ts around lines 11 to 13, the
turnstileSiteKey() currently returns the placeholder "TODO" which will break
Cloudflare Turnstile in preprod; replace this by reading the real preprod
Turnstile site key from a secure source (e.g.,
process.env.PREPROD_TURNSTILE_SITE_KEY) or, if you intentionally want to bypass
verification in preprod, use Cloudflare's documented test site key as a
fallback; update the method to return the env-driven key (with a clear fallback
to the test key) and ensure the corresponding environment variable is set in the
preprod deployment configuration.
| turnstileSiteKey(): string { | ||
| return "TODO"; | ||
| } |
There was a problem hiding this comment.
Replace the "TODO" Turnstile site key before enabling in prod
Overriding turnstileSiteKey() in prodConfig to return "TODO" means:
- The frontend will render Turnstile with an invalid site key.
- The server will likely reject tokens, effectively blocking joins once Turnstile enforcement is enabled.
Until you have the real production key wired, it’s safer to either:
- Remove this override and use the default env-based implementation from
DefaultServerConfig, or - Implement this method to return the real site key from config/env (e.g. delegating to
super.turnstileSiteKey()).
For example:
- turnstileSiteKey(): string {
- return "TODO";
- }
+ turnstileSiteKey(): string {
+ return super.turnstileSiteKey();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| turnstileSiteKey(): string { | |
| return "TODO"; | |
| } | |
| turnstileSiteKey(): string { | |
| return super.turnstileSiteKey(); | |
| } |
🤖 Prompt for AI Agents
In src/core/configuration/ProdConfig.ts around lines 14 to 16, the override of
turnstileSiteKey() returns the literal "TODO", which will cause an invalid
Turnstile site key in production; change the implementation to not hardcode
"TODO" and instead retrieve the real key (e.g., delegate to
super.turnstileSiteKey() or read from the production env/config) so the correct
site key is used when Turnstile is enabled.
| // Verify turnstile token for new clients. | ||
| if (clientMsg.lastTurn === 0) { | ||
| const result = await verifyTurnstileToken( | ||
| ip, | ||
| clientMsg.turnstileToken, | ||
| config.turnstileSecretKey(), | ||
| ); | ||
| switch (result.status) { | ||
| case "approved": | ||
| break; | ||
| case "rejected": | ||
| log.warn("Unauthorized: Turnstile token rejected", { | ||
| clientID: clientMsg.clientID, | ||
| }); | ||
| ws.close(1002, "Unauthorized"); | ||
| return; | ||
| case "error": | ||
| log.error("Turnstile token error", { | ||
| clientID: clientMsg.clientID, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
The "error" case allows clients through - is this intentional?
When Turnstile verification returns status: "error" (e.g., network failure reaching Cloudflare), the code logs the error but does not close the connection or return. The client proceeds to join the game.
If this fail-open behavior is intentional (for availability), add a comment explaining the decision. If fail-closed is intended (for security), add the missing connection close:
case "error":
log.error("Turnstile token error", {
clientID: clientMsg.clientID,
});
+ ws.close(1002, "Unauthorized");
+ return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Verify turnstile token for new clients. | |
| if (clientMsg.lastTurn === 0) { | |
| const result = await verifyTurnstileToken( | |
| ip, | |
| clientMsg.turnstileToken, | |
| config.turnstileSecretKey(), | |
| ); | |
| switch (result.status) { | |
| case "approved": | |
| break; | |
| case "rejected": | |
| log.warn("Unauthorized: Turnstile token rejected", { | |
| clientID: clientMsg.clientID, | |
| }); | |
| ws.close(1002, "Unauthorized"); | |
| return; | |
| case "error": | |
| log.error("Turnstile token error", { | |
| clientID: clientMsg.clientID, | |
| }); | |
| } | |
| } | |
| // Verify turnstile token for new clients. | |
| if (clientMsg.lastTurn === 0) { | |
| const result = await verifyTurnstileToken( | |
| ip, | |
| clientMsg.turnstileToken, | |
| config.turnstileSecretKey(), | |
| ); | |
| switch (result.status) { | |
| case "approved": | |
| break; | |
| case "rejected": | |
| log.warn("Unauthorized: Turnstile token rejected", { | |
| clientID: clientMsg.clientID, | |
| }); | |
| ws.close(1002, "Unauthorized"); | |
| return; | |
| case "error": | |
| log.error("Turnstile token error", { | |
| clientID: clientMsg.clientID, | |
| }); | |
| ws.close(1002, "Unauthorized"); | |
| return; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/server/Worker.ts around lines 393 to 414, the Turnstile verification
"error" branch currently only logs and allows the client to proceed (fail-open);
decide and implement the intended behavior: either add a clear comment
explaining that fail-open is intentional for availability, or enforce
fail-closed by closing the websocket and returning (e.g., call ws.close(1002,
"Unauthorized") and return) so clients are not allowed through on verification
errors; also ensure the error log includes any underlying error details for
debugging when keeping the log.
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
Description:
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME