Repository navigation
Refuse bidi controls and zero-width characters in config keys - #657
Conversation
In a modpack settings block, keys carrying bidirectional controls or invisible zero-width characters pass the filename validation rules. Windows and Unix file systems admit these characters, but they alter visual rendering: bidi controls (such as U+202E right-to-left override) change the display order of following characters so an extension or stem displays as something else, while zero-width characters (such as U+200B zero-width space) make distinct names look identical or render as empty. Add BIDI_OR_ZERO_WIDTH_CHARACTER matching Unicode Bidi_Control, format characters (Cf), and the Mongolian vowel separator. In assertModConfigKey, refuse segments containing any of these characters alongside existing Windows forbidden characters. Add unit tests verifying refusal in assertModConfigKey and parseModpackSettings. Fixes #594
Pixnop
left a comment
There was a problem hiding this comment.
Reviewed at eb6f93e. Every bidi control and every format character is now refused, at every position in a key and in every segment, through the parse, the apply and the export, and the runs from #572's third round move only where they should. What stops me is the rest of the default-ignorable set: 4,036 code points still pass, and the launcher's own Chromium draws almost all of them as nothing at all.
Blocking
-
The default-ignorable characters that are not format characters still pass, and they hide a name as well as U+200B does. #594 asks that the player not be shown a different name than the one on disk, and the comment above
assertModConfigKeynow says a pack cannot do that. It still can. 4,036 code points carryDefault_Ignorable_Code_Pointwithout being in\p{Cf}, and each passes wherever a plain letter does (table below). Drawn by the launcher's Electron,config+ any of 4,031 of them +.jsongives the same pixels asconfig.json. Through the real import and apply handlers, a pack with eleven keys taken across the table, over an Installation with its ownconfig.json, imports with no refusal and writes all eleven files besideconfig.json, which keeps its bytes. In the real dialog, a pack carryingconfig.jsonandconfig+ U+034F +.jsongives two rows that readconfig.json: the first says "Replaces yours" and starts clear, the second says "This Installation has no file at this name" and starts ticked. A key U+3164 +.jsongives a ticked row that reads as a blank followed by.json, which is the "renders as nothing" case of the issue.Adding the property to the class fixes it:
const BIDI_OR_ZERO_WIDTH_CHARACTER = /[\p{Cf}\p{Default_Ignorable_Code_Point}]/u
\p{Cf}already holds everyBidi_Controland U+180E (see below). With that line all 4,174 are refused at every position, the pack above is refused asbad-key, the #572 keys and packs answer as they do at this head, and the whole suite passes. A test that walks one character from each row, in a later segment, would prove the rule the way the issue asks, and close the two test gaps below:for (const hidden of ["\u034F", "\u115F", "\u17B4", "\u180B", "\u180F", "\u2065", "\u3164", "\uFE0F", "\uFFA0", "\uFFF0", "\u{E0000}", "\u{E0080}", "\u{E0100}", "\u{E0FFF}"]) { assert.throws(() => assertModConfigKey(`Client/config${hidden}.json`), /Invalid mod config key/) assert.throws(() => assertModConfigKey(`Client/Sub${hidden}/config.json`), /Invalid mod config key/) }
What still passes at this head
Every code point with Default_Ignorable_Code_Point or Bidi_Control that the rule lets through. Each one passes at the start, middle and end of the stem, as the whole stem, at the start, middle and end of a folder, as a whole folder and in a second folder. The last column is config + the character + .json, drawn offscreen by Electron 44.1.1 (Chromium 152) with the renderer's font stack in bold 16 px, against config.json.
| Code points | Count | What they are | Drawn |
|---|---|---|---|
| U+034F | 1 | combining grapheme joiner | same pixels as config.json |
| U+115F, U+1160 | 2 | Hangul choseong and jungseong fillers | a blank gap, 14.7 px |
| U+17B4, U+17B5 | 2 | Khmer inherent vowels | same pixels |
| U+180B to U+180D | 3 | Mongolian free variation selectors one to three | same pixels |
| U+180F | 1 | Mongolian free variation selector four | a visible glyph |
| U+2065 | 1 | unassigned | same pixels |
| U+3164 | 1 | Hangul filler | a blank gap, 16 px |
| U+FE00 to U+FE0F | 16 | variation selectors 1 to 16 | same pixels |
| U+FFA0 | 1 | halfwidth Hangul filler | a blank gap, 14.7 px |
| U+FFF0 to U+FFF8 | 9 | unassigned | same pixels |
| U+E0000, U+E0002 to U+E001F | 31 | unassigned, in the tag block | same pixels |
| U+E0080 to U+E00FF | 128 | unassigned | same pixels |
| U+E0100 to U+E01EF | 240 | variation selectors 17 to 256 | same pixels |
| U+E01F0 to U+E0FFF | 3,600 | unassigned | same pixels |
4,036 in all, 267 of them assigned. "Same pixels" also holds for the character + .json against .json, so a stem made of them reads as .json alone.
Not blocking
- The export now refuses these names with a reason that is not true.
collectModConfigsruns the same rule, so a file in the player's ownModConfigwhose name holds one of these characters now stops the export asbad-name. Through the real handler that is the case for U+200B, U+202E and U+200D, all three of which exported at dev. The player then readsexportModpackConfigBadName, "{{name}} cannot travel in a modpack: Windows does not accept that file name". Windows does accept it, as the commit message says, and the name in the message draws without the character: with U+200B the notification lays out line for line like the one forzerowidth.json. With U+202E the override runs to the end of the paragraph, so the whole notification draws reversed: in a 15rem column its first line reads ":kcapdom a ni levart tonnac nosj.live". Unticking the file in the picker still gets the rest out, as for the Windows names, but the message points at the wrong problem. These are not only tricks either: U+200D sits inside many emoji, and U+200C in ordinary Persian words. A reason of its own, with the name's hidden characters spelled out (<U+200B>, say), would fix both. That needs a string in en-US and fr-FR and the translation status regenerated, so a follow-up is fine. - Two changes to the rule leave the whole suite green. Every key in the new tests carries its character in the first segment, so checking the first segment only (
segment === segments[0] && ...) passes everything, andClient/zero+ U+200B +width.jsoncould come back unnoticed. A class listing exactly the ten characters the tests name passes as well. The loop above, with the character in a later segment, catches both. \p{Bidi_Control}and U+180E are already inside\p{Cf}. In Unicode 16 (Node 22) and 17 (Electron 44) the class matches exactly the 170 code points of\p{Cf}alone, and dropping either part passes the whole suite. Harmless as written; the class suggested above drops both.- Outside #594: the Unicode spaces other than U+0020 (U+00A0, U+2000 to U+200A, U+202F, U+205F, U+3000) and U+2800 still pass, as before. They draw a gap of 1.6 to 16 px rather than nothing, but a stem or a folder made only of U+3000 or U+2800 still reads as a blank name. A follow-up if that is worth refusing.
- #656 moves this function. It takes
assertModConfigKeyandWINDOWS_FORBIDDEN_CHARACTERout ofsrc/ipc/handlers/modConfigs.tsintosrc/domain/mods/modConfigs.ts, so whichever of the two lands second conflicts here. The class then belongs in the new module, and its tests intests/domain/mods/modConfigs.test.ts.
Validation
The sweep. Every code point with Default_Ignorable_Code_Point or Bidi_Control (4,174), plus every Cf and every variation selector, went through the real assertModConfigKey at ten positions: the nine above and after .json. A plain letter at the same positions passes everywhere but after .json. 138 are refused at every position: U+00AD, U+061C, U+180E, U+200B to U+200F, U+202A to U+202E, U+2060 to U+2064, U+2066 to U+206F, U+FEFF, U+1BCA0 to U+1BCA3, U+1D173 to U+1D17A, U+E0001 and U+E0020 to U+E007F. That covers every name the issue and the description give, all twelve bidi controls among them. The other 4,036 are the table. After .json the extension rule refuses every character, at dev and here. The class also refuses 32 format characters that are not default-ignorable (Arabic number signs, hieroglyph format controls, interlinear annotation), none of which belongs in a config name. Swept again in Electron 44's own runtime, the class refuses the same set.
What is drawn. At dev, a pack key safe + U+202E + gnp.json draws as safenosj.png in a dialog row; at this head that pack's settings are refused as bad-key, and so are packs carrying U+200B, U+00AD, U+2061, U+2069 in a folder, U+E0001 or U+E0041. Of the refused 138, all but the four shorthand format controls draw nothing or reorder the name.
Where the names go. Settings come in through one path (IMPORT_MODPACK, parseModpackSettings), are written through one (APPLY_MOD_CONFIGS, which checks every key again) and go out through one (collectModConfigs), and all three call assertModConfigKey, so the rule sits where every path passes. The dialog rows, the failure list and the applied answer only hold keys that passed. The import's refusal notification does not name the key, so a refused name is never drawn there. The listing has no key rule by design and feeds the export picker and the dialog's captions with the player's own names. applied.txt and the #603 resolver use the spelling on disk, but only for a name that folds to a key that passed, and toLowerCase moves no code point into or out of the class or Default_Ignorable_Code_Point (checked over all of Unicode), so neither can bring a refused character in.
The #572 runs. The 73 hostile keys and the 35 made-up packs from #572's third round, at dev and here: two keys change, a + U+202E + nosj.json and a + U+200B + .json, both now refused. The 35 packs answer as they did in that round, at dev and here.
Mutations. 15 changes to the new rule against the eight feature files (139 tests), survivors against the whole suite. Caught: the check removed, the class narrowed to Bidi_Control or stripped of Cf, the class anchored at either end, the u flag dropped, the check on the file name only or on the folders only, and the parse skipping the key rule. Survived and equivalent: \p{Cf} alone, the class without Bidi_Control, without U+180E, and the whole key tested in place of each segment. Survived and not equivalent: the first segment only, and the list of the ten tested characters (above). The suggested class passes the feature files and the whole suite.
Gates. On this head, npm ci, typecheck, lint:ci (0 errors, the 12 known warnings), format:check and test:coverage pass: 279 files, 5016 passed, 4 skipped, 94.35 / 90.46 / 95.3 / 96.19, with modConfigs.ts at 94.52 / 91.51 / 88.88 / 96.94. CI is green on the four test runners (Ubuntu and Windows, Node 22 and 26), both builds, lint, typecheck and Sonar, with macOS skipped. The branch sits on dev's current head (9013f93) and merges without a conflict. No locale, dependency or migration changes.
Not run. The game, a packaged build, Windows and macOS by hand, and the drawing on Windows.
Changes requested, for the default-ignorable characters alone. The bidi controls and the format characters are refused at the one place every path goes through, and nothing else moved. With \p{Default_Ignorable_Code_Point} in the class and a test that walks one character from each row of the table in a later segment, I would approve; the export message can follow.
Translation statusen-US is the source and carries 903 keys.
Drafted values are the machine-drafted ones still waiting for a native review, listed per locale in |
|
Addressed Pixnop's review feedback in
This branch overlaps PR #656's move of key validation into the domain module. If #656 lands first, I will carry this Unicode rule and its tests into that module during the rebase. |
Pixnop
left a comment
There was a problem hiding this comment.
Reviewed again at ac63873. The class now carries Default_Ignorable_Code_Point, so every code point round one listed is refused at every position in a key, and the export names a file with its hidden characters spelled out. Nothing blocks. Two things belong to the merge rather than to the code: the new message needs #665 to read right, and the order against #656.
Round one, item by item
- Blocking, the default-ignorable characters: settled. All 4,174 code points with
Default_Ignorable_Code_PointorBidi_Controlare refused by the realassertModConfigKeyat the nine positions inside a key. Taken from the source, the class matches exactlyCfplusDefault_Ignorable_Code_Pointin Node 22 (Unicode 16) and in Electron 44's own runtime (Unicode 17), 4,206 code points, so this commit adds the 4,036 of round one's table and nothing else. Through the real import handler, round one's eleven-key pack over a tunedconfig.jsonis refused asbad-keyandconfig.jsonkeeps its bytes; a pack withconfig.jsonbesideconfig+ U+FE00 +.jsonis refused the same way. - The export's reason: settled in the handler. A file whose name holds one of these characters now stops the export as
hidden-character, namedzero<U+200B>width.json,<U+202E>evil.json,Sub<U+3164>/config.jsonortag<U+E0041><U+E0100>.json, and a Windows problem alone still readsbad-name. The notification no longer carries the character, so the U+202E case no longer draws backwards, and unticking the file still gets the rest out. What the player reads depends on #665 (below). - The two surviving mutants: settled. The first segment only and the ten-character class both fail the new loop.
- The redundant class parts: settled.
\p{Bidi_Control}and U+180E are gone.\p{Cf}stays, and is needed (below). - The Unicode spaces: unchanged, as expected for #594. U+00A0, U+1680, U+2000 to U+200A, U+202F, U+205F, U+3000 and U+2800 still pass, as do U+0085, U+2028 and U+2029. A follow-up if wanted.
- #656: still conflicts, see the last section.
New in this commit, not blocking
- The new message shows HTML entities until #665 lands. i18next escapes interpolated values here (#663), and every name this message carries holds
<and>, so it is hit every time, where its siblings are hit only by a name with a slash, a quote or an ampersand. Rendered through the real picker, hook and notifications, with the handler's answer for a fileconfig+ U+034F +.jsonin a folderClient, the player readsClient/config<U+034F>.json contains invisible Unicode characters and cannot travel in a modpack. .... With #665's line it readsClient/config<U+034F>.json contains invisible Unicode characters .... Merging #665 first, or in the same release, covers it; otherwise this call wantsinterpolation: { escapeValue: false }like the calls that already carry it. - Dropping
\p{Cf}from the class leaves the whole suite green, and the two are not equivalent. 32 format characters are not default-ignorable, and three of them, U+FFF9 to U+FFFB (interlinear annotation), draw as nothing:config+ U+FFF9 +.jsongives the same pixels asconfig.json. The comment above the class says the property "includes format characters", which invites dropping the other half. Adding"\uFFF9"tohiddenCharacterspins it: it passes at this head and fails without\p{Cf}. The comment could say why both properties are there. - The
<U+XXXX>spelling is tested on one BMP character. Splitting the name by UTF-16 unit instead of by code point also leaves the suite green, and would hand backtag+ U+E0041 +.jsonwith its tag character raw. A second character in the export test,config\u034F\u{E0100}.jsonexpectingconfig<U+034F><U+E0100>.json, catches it and passes at this head. - "Invisible" is not true of 34 of the 4,206. U+0600 to U+0605, U+06DD, U+070F, U+0890, U+0891, U+08E2, U+180F, U+110BD, U+110CD, U+13430 to U+1343F and U+1BCA0 to U+1BCA3 leave a visible mark. The spelled code point still finds the file, so this is wording only.
What a real name loses. Nothing beyond the table is newly refused. Inside it, a real config name could hold U+FE0F after an emoji (heart + U+2764 + U+FE0F + .json now stops the export as hidden-character), U+E0100 to U+E01EF in a CJK name written with a chosen glyph, the Mongolian variation selectors U+180B to U+180D and U+180F, U+034F in pointed Hebrew, or a Hangul filler in old Korean text. U+200D in emoji sequences, U+200C in Persian and Indic words and the tag characters of subdivision flags were already refused at round one's head, as were the 32 other format characters (Arabic, Syriac and Kaithi number, currency and verse marks, Egyptian hieroglyph controls, the interlinear annotation characters). Such a file stops the export with the new message, and a pack carrying one loses its settings at import as bad-key, like any other bad key. I think that is the right trade for #594.
#656 and the merge order
They conflict in the same four files whichever lands first: src/ipc/handlers/modConfigs.ts (#656 deletes the function this PR edits), en-US.json and fr-FR.json (adjacent lines: #656 rewords the bad-name and collides strings to say "Untick it", this PR adds its string between them), and the translation status table.
Simpler: this one first. #656 teaches the export picker to leave unsupported rows unticked, and its diagnoseModConfigKey calls every failure of assertModConfigKey bad-name. Once the class sits in the domain rule, a row such as config + U+034F + .json is disabled under "Windows does not accept this file name" and drawn as config.json: round one's false reason, in a new place. Rebasing #656 over this means carrying the class into the domain module (the tests here reach assertModConfigKey through #656's re-export, so they fail if the class is lost), exporting it for collectModConfigs, a hidden-character row with its own caption and the <U+XXXX> spelling, and the new export string brought to the "Untick it" wording. Landing #656 first moves that picker work into this PR.
Validation
The sweep. As in round one: every Default_Ignorable_Code_Point, Bidi_Control, Cf and variation selector through the real assertModConfigKey at ten positions (the nine inside a key and after .json), against a plain letter that passes everywhere but after .json. None of the 4,174 passes anywhere. Over all of Unicode, neither toLowerCase nor NFC, NFD, NFKC or NFKD moves a code point into or out of the class.
The #572 runs. The 73 hostile keys and the 35 made-up packs answer exactly as at eb6f93e. Against dev, the same two keys change as in round one.
Mutations. 24 changes to the rule and to the new export code against the eight feature files (141 tests), survivors against the whole suite. Caught: the check removed; the class as Bidi_Control alone, Cf alone, round one's class or round one's ten characters, anchored at either end, or without the u flag; the check on the file name only, the folders only, the first segment only, or the first and last only; the parse skipping the rule; the export branch removed or always taken; the spelling unchanged, unpadded or in lower case. Survived and equivalent: the whole key tested in place of each segment, and the whole name in place of each character (also the shorter way to write that line). Survived and not equivalent: \p{Cf} dropped and the UTF-16 split (above), a class of exactly the 24 characters the tests name, which any list of examples allows, and the hook's new branch, untested like every other reason in that hook.
Gates. On this head, npm ci, typecheck, lint:ci (0 errors, the 12 known warnings), format:check and test:coverage pass: 279 files, 5019 passed, 4 skipped, 94.32 / 90.41 / 95.31 / 96.16, with modConfigs.ts at 94.64 / 91.71 / 89.74 / 97. CI is green on ac63873: the four test runners (Ubuntu and Windows, Node 22 and 26), both builds, lint, typecheck and Sonar, with macOS skipped. Dev has moved to 682751b (#662, Optimum files only) and the branch merges into it without a conflict. One string added, in en-US and fr-FR only; drafted.json untouched; the status table is what the generator writes; all 14 locale files come back byte for byte from Weblate's JSON writer.
Not run. The game, a packaged build, Windows and macOS by hand.
Approved. The default-ignorable characters are refused at the one place every path goes through, the tests now hold the rule where round one found it loose, and nothing else moved. Please merge #665 before this one or with it. The two test lines above are worth taking now, and the picker caption goes with #656's rebase.
Summary
Modpack settings keys can contain Unicode characters that the launcher renders as empty, blank, or reordered text. This change rejects default-ignorable code points and format characters before a key reaches import, apply, or export.
Changes
assertModConfigKeychecks every file and folder segment againstCfandDefault_Ignorable_Code_Point.<U+...>so the player can identify and rename the file.Type
Checklist
dev, notmain.npm run typecheckpasses.npm run lint:cipasses.npm run format:checkpasses.npm run test:coveragepasses, coverage at or above the floor invitest.config.ts.npm run build:unpackpasses.Testing
Ran the RiftLauncher gates on Linux x64:
npm run typecheck: passed across all TypeScript configurations.npm run lint:ci: passed with 0 errors and 12 existing warnings.npm run format:check: passed.npm run test:coverage: passed, 94.31% statements, 90.39% branches, 95.31% functions, 96.16% lines. The full run passed 5,018 tests and skipped 5.npm run build:unpack: passed.npm run test -- --run tests/ipc/modConfigs.test.ts: passed, 64 tests.Related issues
Fixes #594