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:
WalkthroughMoves profanity detection and censoring from client-side validation into the server PrivilegeChecker; loads cosmetics and banned-words in parallel (with API key), applies server-side censorship when creating Clients (preserving uncensoredUsername), and propagates the assigned player name into GameView. Also adds many related server/client schema, lobby, routing, graphics layer, map-generator logging, and execution behavior changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Worker
participant PrivilegeRefresher
participant CosmeticsAPI as Cosmetics API
participant BannedWordsAPI as Banned Words API
participant PrivilegeChecker
participant WorkerRuntime as Worker (create Client)
participant GameView
Worker->>PrivilegeRefresher: start(cosmeticsEndpoint, profaneWordsEndpoint, apiKey)
par parallel fetch
PrivilegeRefresher->>CosmeticsAPI: GET /cosmetics
CosmeticsAPI-->>PrivilegeRefresher: cosmetics.json
and
PrivilegeRefresher->>BannedWordsAPI: GET /profane_words (with apiKey)
BannedWordsAPI-->>PrivilegeRefresher: bannedWords[]
end
PrivilegeRefresher->>PrivilegeChecker: init(cosmetics, bannedWords)
WorkerRuntime->>PrivilegeChecker: censorUsername(rawUsername)
PrivilegeChecker-->>WorkerRuntime: censoredUsername
WorkerRuntime->>WorkerRuntime: new Client(..., uncensoredUsername, censoredUsername, ...)
WorkerRuntime->>GameView: new GameView(..., playerName /* censored display name */)
GameView->>GameView: replace local player's username with playerName
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/server/UsernameCensor.ts`:
- Around line 72-96: The function censorUsername can return an empty name when
username is only a clan tag; update censorUsername to detect when
nameWithoutClan.trim() is empty and treat that as profane: compute censoredName
from shadowNames (e.g., shadowNames[simpleHash(username) % shadowNames.length]
to vary by full username) instead of using an empty string, then continue
restoring the clan tag (using clanTagIsProfane/isProfane,
getClanTagOriginalCase) and return `[${displayTag}] ${censoredName}`; reference
symbols: censorUsername, getClanTagOriginalCase, isProfane, shadowNames,
simpleHash.
- Around line 13-20: The RegExpMatcher is being constructed by spreading
TransformerContainer objects (resolveConfusablesTransformer,
skipNonAlphabeticTransformer, collapseDuplicatesTransformer,
resolveLeetSpeakTransformer) directly into the options; instead, remove those
spreads and pass any extra transformer functions as an array under
blacklistMatcherTransformers or whitelistMatcherTransformers (merging with
englishRecommendedTransformers.blacklistMatcherTransformers if present) while
keeping the base dataset from englishDataset.build() and the preset
englishRecommendedTransformers spread into the top-level options; alternatively
drop the redundant custom transformers if the preset already includes them.
🧹 Nitpick comments (2)
tests/UsernameCensor.test.ts (1)
103-111: Consider exportingshadowNamesfrom the implementation.The
shadowNamesarray is duplicated fromUsernameCensor.ts. If the implementation changes, tests could pass incorrectly or fail unexpectedly.Consider exporting
shadowNamesfrom the implementation file and importing it here, or use a different assertion strategy that does not rely on knowing the exact shadow names.src/server/PrivilegeRefresher.ts (1)
64-74: Consider validating the profane words response.The JSON response is directly assigned to
bannedWordswithout type validation. If the endpoint returns unexpected data (e.g., an object instead of an array), this could cause runtime errors inPrivilegeCheckerImpl.♻️ Proposed fix with validation
let bannedWords: string[] = []; if (profaneWordsResponse.ok) { - bannedWords = await profaneWordsResponse.json(); + const data = await profaneWordsResponse.json(); + if (Array.isArray(data) && data.every((w) => typeof w === "string")) { + bannedWords = data; + } else { + this.log.warn(`Invalid profane words format, using empty list`); + } this.log.info( `Loaded ${bannedWords.length} profane words from ${this.profaneWordsEndpoint}`, );
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
evanpelle
force-pushed
the
censor-names
branch
from
9420c57 to
cf1b8bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/core/Schemas.ts`:
- Around line 476-482: PlayerSchema currently exposes uncensoredUsername to
every client; remove uncensoredUsername from PlayerSchema and instead add a
per-recipient field on the start payload (e.g., add localUncensoredUsername or
recipientUncensoredUsername to GameStartInfoSchema / ServerStartGameMessage) so
the server can populate that field only for the owning player when sending the
ServerStartGameMessage; alternatively, if you prefer minimal change, strip
uncensoredUsername from each player object right before broadcasting in the code
that builds ServerStartGameMessage (ensure functions constructing
GameStartInfoSchema and any serializer use the new per-client field or the
stripped players list and update references to uncensoredUsername accordingly).
In `@src/server/Privilege.ts`:
- Around line 58-82: In censorUsernameWithMatcher, handle the edge case where
nameWithoutClan is empty after removing the clan tag: if nameWithoutClan.trim()
=== "" then substitute a non-empty placeholder (e.g., select from shadowNames
using simpleHash(username) or use a constant like "PLAYER") so you never return
a clan tag with a trailing empty name; preserve the existing clanTag profane
logic (use displayTag = clanTagIsProfane ? "XXX" : clanTag.toUpperCase()) and
return `[${displayTag}] ${placeholder}` (or just `[${displayTag}]` if you prefer
no space) to avoid producing `"[ABC] "` or an empty display name.
In `@src/server/PrivilegeRefresher.ts`:
- Around line 71-73: The log at PrivilegeRefresher (the this.log.info call that
prints `Loaded ${bannedWords.length} profane words from
${this.profaneWordsEndpoint}`) exposes the full profaneWordsEndpoint which may
include secrets; change the message to avoid printing the URL—log only the count
(bannedWords.length) and, if you need to indicate the source, log a sanitized
form (e.g., hostname or a fixed label) or omit profaneWordsEndpoint entirely so
no query params/API keys are emitted. Locate the this.log.info call in
PrivilegeRefresher and replace the interpolated profaneWordsEndpoint with a
non-sensitive identifier or remove it.
- Around line 68-78: In loadPrivilegeChecker, wrap the
profaneWordsResponse.json() call in its own try/catch and validate the result
before assigning to bannedWords: call profaneWordsResponse.json() inside a local
try block, ensure the returned value is an array and each item is a string
(filter/coerce to string or discard non-strings), and on any JSON parse error or
validation failure log a warning via this.log.warn and leave bannedWords as an
empty array so createMatcher receives a safe string[]; include the
profaneWordsEndpoint and any error message in the warn log for diagnostics.
🧹 Nitpick comments (6)
src/server/Privilege.ts (2)
23-31: Shadow names list is small — collisions will be common.With only 7 shadow names, many different profane usernames will map to the same replacement. This is deterministic (same input → same output), which is good, but players may notice identical shadow names. This might be intentional to keep it simple.
No action required — just flagging for awareness.
194-205: Module-leveldefaultMatcheris created at import time.
createMatcher([])runs when the module is first imported. This is fine for server code, but worth noting that it initializes theenglishDataseteagerly. If this module is imported in test or client bundles where theobscenitylibrary is not available, it would fail at import time.src/server/Client.ts (1)
20-21: Naming could be clearer:usernameis the censored version here.After this change,
usernameholds the censored name whileuncensoredUsernameholds the original. This inversion from the natural reading ("username = what the user typed") could trip up future maintainers. A comment on these fields clarifying the semantics would help.tests/UsernameCensor.test.ts (3)
67-75:shadowNamesis duplicated from source — will drift silently.This array is copy-pasted from
src/server/Privilege.ts. If the source list changes, this test will produce confusing failures. Consider exportingshadowNamesfrom the source module and importing it here.Suggested fix
In
src/server/Privilege.ts, export the array:-const shadowNames = [ +export const shadowNames = [Then in the test:
-import { createMatcher, PrivilegeCheckerImpl } from "../src/server/Privilege"; +import { createMatcher, PrivilegeCheckerImpl, shadowNames } from "../src/server/Privilege"; describe("censorUsername", () => { - const shadowNames = [ - "NicePeopleOnly", - "BeKindPlz", - "LearningManners", - "StayClassy", - "BeNicer", - "NeedHugs", - "MakeFriends", - ];
147-151: Empty banned words test does not verify built-in profanity is caught.The test only checks that a clean name passes through. It does not confirm that the
englishDataset(built-in profanity) is still active when custom banned words are empty. Adding a positive detection case would make this test meaningful.Suggested addition
test("empty banned words list still catches englishDataset profanity", () => { // The emptyChecker still uses englishDataset, so common profanity is caught expect(emptyChecker.censorUsername("CoolPlayer")).toBe("CoolPlayer"); + // Verify built-in profanity is still detected + const result = emptyChecker.censorUsername("asshole"); + expect(result).not.toBe("asshole"); });
50-52: Test usesöwhich is not allowed byUsernameSchema.The
UsernameSchemaregex only permitsüÜas special characters (notö). In production,"Adölf"would be rejected at validation before reachingcensorUsername. This test exercises a code path that cannot occur with real input. Not harmful, but it could give false confidence.
evanpelle
force-pushed
the
censor-names
branch
from
cf1b8bf to
1b86d94
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/server/Privilege.ts`:
- Around line 38-39: Remove the debug logging that prints banned words to
stdout: delete the console.log(word) call inside the loop that iterates over
bannedWords (the loop used when building the matcher in Privilege or the matcher
creation invoked by PrivilegeRefresher). If you want to retain telemetry,
replace it with a non-sensitive debug trace or a single count/log that does not
expose the words (e.g., log bannedWords.length) and ensure it uses the project's
logger at an appropriate level rather than console.log.
- Around line 44-52: The transformer objects are being spread into the top-level
config so their {transform,type} fields are ignored; update the RegExpMatcher
invocation (around customDataset.build()) to pass transformers under the
expected array properties: set blacklistMatcherTransformers to an array that
combines englishRecommendedTransformers.blacklistMatcherTransformers with
resolveConfusablesTransformer(), skipNonAlphabeticTransformer(),
collapseDuplicatesTransformer(), resolveLeetSpeakTransformer(), and set
whitelistMatcherTransformers to
englishRecommendedTransformers.whitelistMatcherTransformers so all transformers
are actually applied.
In `@src/server/PrivilegeRefresher.ts`:
- Around line 50-53: The cosmetics and profane words fetches are currently
grouped in Promise.all so any network error from the profane words endpoint
aborts cosmetics loading; change this so the cosmetics fetch
(fetchWithTimeout(this.cosmeticsEndpoint)) is awaited normally (or via
Promise.all for only cosmetics if needed) while the profane words fetch
(fetchWithTimeout(this.profaneWordsEndpoint)) is awaited inside a try/catch or
with .catch to swallow network errors and return a safe fallback (e.g., an empty
banned-words array or null). Update references to cosmeticsResponse and
profaneWordsResponse to handle the profaneWordsResponse possibly being the
fallback and ensure subsequent code in PrivilegeRefresher that parses/uses
profaneWords handles an empty list/null safely.
🧹 Nitpick comments (2)
src/server/Privilege.ts (2)
195-196: Module-levelcreateMatcher([])runs at import time — side effect on every import.
createMatcher([])is called when this module is first imported. It builds a fullRegExpMatcherfrom the English dataset. This is a non-trivial computation that runs as a side effect at import time, even ifFailOpenPrivilegeCheckeris never used. Consider making it lazy.Lazy init example
-const defaultMatcher = createMatcher([]); +let _defaultMatcher: RegExpMatcher | null = null; +function getDefaultMatcher(): RegExpMatcher { + if (!_defaultMatcher) { + _defaultMatcher = createMatcher([]); + } + return _defaultMatcher; +} export class FailOpenPrivilegeChecker implements PrivilegeChecker { ... censorUsername(username: string): string { - return censorUsernameWithMatcher(username, defaultMatcher); + return censorUsernameWithMatcher(username, getDefaultMatcher()); } }
23-31:shadowNameshas only 7 entries — easy to enumerate and reverse.Players will quickly notice a small fixed set of replacement names and may figure out which names trigger censorship. Not blocking, but worth noting for the moderation team. Adding more names or randomizing further would make the mapping less obvious.
evanpelle
force-pushed
the
censor-names
branch
from
1b86d94 to
79e9018
Compare
evanpelle
force-pushed
the
censor-names
branch
from
79e9018 to
12315fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/UsernameCensor.test.ts`:
- Around line 75-83: The test's local shadowNames array mismatches the real list
in Privilege.ts causing assertion failures; export the authoritative shadowNames
from src/server/Privilege.ts (the same array used by censorUsername) and update
tests to import that exported symbol instead of redefining it, or replace the
test's array with the exact 20-name list from Privilege.ts so
expect(shadowNames).toContain(result) uses the real values referenced by
censorUsername.
12315fa to
f9dc1b9
Compare
evanpelle
force-pushed
the
censor-names
branch
from
f9dc1b9 to
07e06b9
Compare
evanpelle
force-pushed
the
censor-names
branch
from
07e06b9 to
433c341
Compare
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)
src/server/Worker.ts (1)
422-433:⚠️ Potential issue | 🔴 CriticalParameter order swap:
censoredUsernameandclientMsg.usernameare reversed.The
Clientconstructor expects parameters 7–8 asusernameanduncensoredUsername, but Worker.ts passescensoredUsernametousernameandclientMsg.usernametouncensoredUsername. This swap silently assigns the censored value tousernameand the uncensored value touncensoredUsername— the opposite of what the parameter names indicate. Correct the order to match the constructor signature.
🤖 Fix all issues with AI agents
In `@tests/UsernameCensor.test.ts`:
- Around line 155-158: The test name claims profanity from englishDataset is
caught but only asserts a clean name; add an assertion using
emptyChecker.censorUsername with a known English profane word (e.g., "shit" or
"asshole") and assert the returned value is censored (for example, not equal to
the original input or matches the expected censored form) to confirm
englishDataset is still active.
🧹 Nitpick comments (1)
tests/UsernameCensor.test.ts (1)
17-23: Consider typing the mock to match the realCosmeticsinterface.
mockCosmeticsis a plain object literal{ patterns: {}, colorPalettes: {} }. If theCosmeticstype changes shape later, this test will silently drift. Useas Cosmetics(importing the type) or add an explicit type annotation so TypeScript catches shape mismatches at compile time.
evanpelle
force-pushed
the
censor-names
branch
from
433c341 to
e81c641
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/server/Privilege.ts`:
- Around line 109-112: The code currently uppercases non-profane clan tags in
Privilege.ts by using clanTag.toUpperCase() when building displayTag (variables:
clanTag, clanTagIsProfane, displayTag), which conflicts with the comment that
says non-profane tags are preserved; either remove the .toUpperCase() so
displayTag uses the original clanTag (preserve case), or if you intend to
normalize to uppercase instead, update the function comment to state that
non-profane tags are normalized to uppercase; make the change in the branch that
constructs displayTag and keep the rest of the logic unchanged.
🧹 Nitpick comments (1)
src/core/game/GameView.ts (1)
606-618: The_myUsernamefield is only used inside the constructor — consider a plain parameter instead ofprivate.Marking the parameter as
privatestores it on the instance forever, but nothing readsthis._myUsernameafter the constructor finishes. A regular parameter avoids the unnecessary retained reference.Also, mutating
h.usernamein place changes the sharedhumansarray objects. If any other code reads those objects afterGameViewis constructed, the mutation leaks out.Suggested change
constructor( public worker: WorkerClient, private _config: Config, private _mapData: TerrainMapData, private _myClientID: ClientID, - private _myUsername: string, + myUsername: string, private _gameID: GameID, private humans: Player[], ) { this._map = this._mapData.gameMap; this.lastUpdate = null; this.unitGrid = new UnitGrid(this._map); // Replace the local player's username with their own stored username for (const h of this.humans) { if (h.clientID === this._myClientID) { - h.username = this._myUsername; + h.username = myUsername; } }
evanpelle
force-pushed
the
censor-names
branch
from
e81c641 to
d9e778f
Compare
evanpelle
force-pushed
the
censor-names
branch
from
d9e778f to
4c09ca4
Compare
evanpelle
force-pushed
the
censor-names
branch
from
4c09ca4 to
54957fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/server/Privilege.ts`:
- Around line 58-67: The matcher config applies normalization transformers only
to blacklist entries (blacklistMatcherTransformers) but leaves whitelist
transformers empty, causing whitelist suppression to fail; update the
RegExpMatcher config returned from customDataset.build() to also pass
whitelistMatcherTransformers using the library's englishRecommendedTransformers
(i.e., spread englishRecommendedTransformers into both
blacklistMatcherTransformers and whitelistMatcherTransformers) so whitelist
terms are normalized the same way as blacklist terms and false positives are
suppressed.
🧹 Nitpick comments (2)
src/core/game/GameView.ts (1)
613-618: Mutating shared input objects — consider a shallow copy.The loop writes directly to
h.usernameon the passed-inhumansarray elements. If any caller holds a reference to the samePlayerobjects and relies on their originalusername, this mutation could cause surprises.A safer pattern is to copy only the matching entry:
Suggested fix
- // Replace the local player's username with their own stored username - for (const h of this.humans) { - if (h.clientID === this._myClientID) { - h.username = this._myUsername; - } - } + // Replace the local player's username with their own stored username + this.humans = this.humans.map((h) => + h.clientID === this._myClientID + ? { ...h, username: this._myUsername } + : h, + );src/server/Privilege.ts (1)
23-45: ExportshadowNamesso the test file can import it instead of duplicating.This 21-entry array is copied verbatim into
tests/Privilege.test.ts. Any future change here will silently desync the test. Mark itexport constto keep a single source of truth.Suggested fix
-const shadowNames = [ +export const shadowNames = [
evanpelle
force-pushed
the
censor-names
branch
from
54957fe to
e9baa55
Compare
evanpelle
force-pushed
the
censor-names
branch
from
e9baa55 to
faa0f03
Compare
evanpelle
force-pushed
the
censor-names
branch
from
faa0f03 to
757db89
Compare
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
2 participants
Description:
Many inapropriate names bypass the current filter. This PR does the following:
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan