Skip to content

Commit 58c7cdd

Browse files
authored
Task: Unify username validation and remove username sanitation (openfrontio#2622)
## Description: This PR centralizes all username validation using UsernameSchema with a set maximum, minimum, and a regex pattern, It also removes sanitization, as all places where the username would be sanitized on the server have been gatekept, so no unvalidated usernames can get onto the server past the ClientMessageSchema safeParse in GameServer's on message func. Here is how the errors look if that happens, Note that if the client is funtioning correctly and the user doesn't manually send a WS message, they should never see this. The screenshots are from a debug build where client uname validation was disabled. <img height="300" alt="error message too short" src="https://github.com/user-attachments/assets/1b7ac32c-2f03-40fb-8ce9-1f4ab66100bd" /> <img height="300" alt="error message bad regex" src="https://github.com/user-attachments/assets/c78b4114-7e4b-4d39-a135-4cab3ad52c0b" /> Profanity sanitization was not changed. Additionally, the censor tests were updated to reflect the new expectations. Jose was added to the jest config as an allowed transform pattern, as it didn't make sense to me to mock a zod schema. The UsernameSchema pattern was set to `^[a-zA-Z0-9_ \[\]üÜ]+$`, I personally think either we should allow all latin characters (regex has a pattern for this, `\p{L}` or `\p{sc=Latin}`) and then we'd use some kind of library to normalize all latin characters into regular ascii for name filtering, or we should only keep ascii letters. ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: Lavodan (I just realized sanitization isn't a word, it's supposed to be sanitation, sorry.)
1 parent 690b7e1 commit 58c7cdd

6 files changed

Lines changed: 43 additions & 73 deletions

File tree

jest.config.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ export default {
1515
"^.+\\.js$": ["@swc/jest"],
1616
},
1717
transformIgnorePatterns: [
18-
"node_modules/(?!(nanoid|@jsep|fastpriorityqueue|@datastructures-js)/)",
18+
"node_modules/(?!(nanoid|@jsep|fastpriorityqueue|@datastructures-js|jose)/)",
1919
],
2020
collectCoverageFrom: ["src/**/*.ts", "!src/**/*.d.ts"],
2121
coverageThreshold: {

src/core/GameRunner.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ import {
2929
import { loadTerrainMap as loadGameMap } from "./game/TerrainMapLoader";
3030
import { PseudoRandom } from "./PseudoRandom";
3131
import { ClientID, GameStartInfo, Turn } from "./Schemas";
32-
import { sanitize, simpleHash } from "./Util";
32+
import { simpleHash } from "./Util";
3333
import { censorNameWithClanTag } from "./validations/username";
3434

3535
export async function createGameRunner(
@@ -48,9 +48,7 @@ export async function createGameRunner(
4848

4949
const humans = gameStart.players.map((p) => {
5050
return new PlayerInfo(
51-
p.clientID === clientID
52-
? sanitize(p.username)
53-
: censorNameWithClanTag(p.username),
51+
p.clientID === clientID ? p.username : censorNameWithClanTag(p.username),
5452
PlayerType.Human,
5553
p.clientID,
5654
random.nextID(),

src/core/Schemas.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,11 @@ export const ID = z
209209

210210
export const AllPlayersStatsSchema = z.record(ID, PlayerStatsSchema);
211211

212-
export const UsernameSchema = SafeString;
212+
export const UsernameSchema = z
213+
.string()
214+
.regex(/^[a-zA-Z0-9_ [\]üÜ]+$/u)
215+
.min(3)
216+
.max(27);
213217
const countryCodes = countries.filter((c) => !c.restricted).map((c) => c.code);
214218

215219
export const QuickChatKeySchema = z.enum(

src/core/game/PlayerImpl.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import {
99
toInt,
1010
within,
1111
} from "../Util";
12-
import { sanitizeUsername } from "../validations/username";
1312
import { AttackImpl } from "./AttackImpl";
1413
import {
1514
Alliance,
@@ -111,7 +110,7 @@ export class PlayerImpl implements Player {
111110
startTroops: number,
112111
private readonly _team: Team | null,
113112
) {
114-
this._name = sanitizeUsername(playerInfo.name);
113+
this._name = playerInfo.name;
115114
this._troops = toInt(startTroops);
116115
this._gold = 0n;
117116
this._displayName = this._name;

src/core/validations/username.ts

Lines changed: 34 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,8 @@ import {
88
skipNonAlphabeticTransformer,
99
} from "obscenity";
1010
import { translateText } from "../../client/Utils";
11-
import { getClanTagOriginalCase, sanitize, simpleHash } from "../Util";
11+
import { UsernameSchema } from "../Schemas";
12+
import { getClanTagOriginalCase, simpleHash } from "../Util";
1213

1314
const matcher = new RegExpMatcher({
1415
...englishDataset.build(),
@@ -22,8 +23,6 @@ const matcher = new RegExpMatcher({
2223
export const MIN_USERNAME_LENGTH = 3;
2324
export const MAX_USERNAME_LENGTH = 27;
2425

25-
const validPattern = /^[a-zA-Z0-9_[\] üÜ]+$/u; // Don't allow more than in UserNameSchema
26-
2726
const shadowNames = [
2827
"NicePeopleOnly",
2928
"BeKindPlz",
@@ -58,22 +57,19 @@ export function isProfaneUsername(username: string): boolean {
5857
*
5958
* Examples:
6059
* - "GoodName" -> "GoodName"
61-
* - "Good$Name" -> "GoodName"
6260
* - "BadName" -> "Censored"
6361
* - "[CLAN]GoodName" -> "[CLAN]GoodName"
6462
* - "[CLaN]BadName" -> "[CLaN] Censored"
6563
* - "[BAD]GoodName" -> "GoodName"
6664
* - "[BAD]BadName" -> "Censored"
6765
*/
6866
export function censorNameWithClanTag(username: string): string {
69-
const sanitizedUsername = sanitize(username);
70-
7167
// Don't use getClanTag because that returns upperCase and if original isn't, str replace `[{$clanTag}]` won't match
72-
const clanTag = getClanTagOriginalCase(sanitizedUsername);
68+
const clanTag = getClanTagOriginalCase(username);
7369

7470
const nameWithoutClan = clanTag
75-
? sanitizedUsername.replace(`[${clanTag}]`, "").trim()
76-
: sanitizedUsername;
71+
? username.replace(`[${clanTag}]`, "").trim()
72+
: username;
7773

7874
const clanTagIsProfane = clanTag ? isProfaneUsername(clanTag) : false;
7975
const usernameIsProfane = isProfaneUsername(nameWithoutClan);
@@ -87,7 +83,7 @@ export function censorNameWithClanTag(username: string): string {
8783
if (usernameIsProfane) {
8884
return `[${clanTag}] ${censoredNameWithoutClan}`;
8985
}
90-
return sanitizedUsername;
86+
return username;
9187
}
9288

9389
// Don't restore profane or nonexistent clan tag
@@ -98,43 +94,39 @@ export function validateUsername(username: string): {
9894
isValid: boolean;
9995
error?: string;
10096
} {
101-
if (typeof username !== "string") {
102-
return { isValid: false, error: translateText("username.not_string") };
103-
}
97+
const parsed = UsernameSchema.safeParse(username);
10498

105-
if (username.length < MIN_USERNAME_LENGTH) {
106-
return {
107-
isValid: false,
108-
error: translateText("username.too_short", {
109-
min: MIN_USERNAME_LENGTH,
110-
}),
111-
};
112-
}
99+
if (!parsed.success) {
100+
const errType = parsed.error.issues[0].code;
113101

114-
if (username.length > MAX_USERNAME_LENGTH) {
115-
return {
116-
isValid: false,
117-
error: translateText("username.too_long", {
118-
max: MAX_USERNAME_LENGTH,
119-
}),
120-
};
121-
}
102+
if (errType === "invalid_type") {
103+
return { isValid: false, error: translateText("username.not_string") };
104+
}
122105

123-
if (!validPattern.test(username)) {
124-
return {
125-
isValid: false,
126-
error: translateText("username.invalid_chars"),
127-
};
106+
if (errType === "too_small") {
107+
return {
108+
isValid: false,
109+
error: translateText("username.too_short", {
110+
min: MIN_USERNAME_LENGTH,
111+
}),
112+
};
113+
}
114+
115+
if (errType === "too_big") {
116+
return {
117+
isValid: false,
118+
error: translateText("username.too_long", {
119+
max: MAX_USERNAME_LENGTH,
120+
}),
121+
};
122+
}
123+
124+
// Invalid regex, or any other issue
125+
else {
126+
return { isValid: false, error: translateText("username.invalid_chars") };
127+
}
128128
}
129129

130130
// All checks passed
131131
return { isValid: true };
132132
}
133-
134-
export function sanitizeUsername(str: string): string {
135-
const sanitized = Array.from(str)
136-
.filter((ch) => validPattern.test(ch))
137-
.join("")
138-
.slice(0, MAX_USERNAME_LENGTH);
139-
return sanitized.padEnd(MIN_USERNAME_LENGTH, "x");
140-
}

tests/Censor.test.ts

Lines changed: 0 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,6 @@ import {
3535
fixProfaneUsername,
3636
isProfaneUsername,
3737
MAX_USERNAME_LENGTH,
38-
MIN_USERNAME_LENGTH,
39-
sanitizeUsername,
4038
validateUsername,
4139
} from "../src/core/validations/username";
4240

@@ -110,25 +108,4 @@ describe("username.ts functions", () => {
110108
expect(res.isValid).toBe(true);
111109
});
112110
});
113-
114-
describe("sanitizeUsername", () => {
115-
test.each([
116-
{ input: "GoodName", expected: "GoodName" },
117-
{ input: "a!", expected: "axx" },
118-
{ input: "a$%b", expected: "abx" },
119-
{
120-
input: "abc".repeat(10),
121-
expected: "abc"
122-
.repeat(Math.floor(MAX_USERNAME_LENGTH / 3))
123-
.slice(0, MAX_USERNAME_LENGTH),
124-
},
125-
{ input: "", expected: "xxx" },
126-
{ input: "Ünicode Test!", expected: "Ünicode Test" },
127-
])('sanitizeUsername("%s") → "%s"', ({ input, expected }) => {
128-
const out = sanitizeUsername(input);
129-
expect(out).toBe(expected);
130-
expect(out.length).toBeGreaterThanOrEqual(MIN_USERNAME_LENGTH);
131-
expect(out.length).toBeLessThanOrEqual(MAX_USERNAME_LENGTH);
132-
});
133-
});
134111
});

0 commit comments

Comments
 (0)