Refuse bidi controls and zero-width characters in config keys

docs/contribute/translation-status.md

@@ -17,22 +17,22 @@ Community translation happens on [Weblate](https://hosted.weblate.org/projects/r
 
 <!-- i18n-status:start -->
 
-en-US is the source and carries 902 keys.
+en-US is the source and carries 903 keys.
 
 | Locale | Keys | Missing | Stale | Drafted to review |
 | ------ | ---: | ------: | ----: | ----------------: |
-| be-BY  |  883 |      53 |     0 |               655 |
-| de-DE  |  856 |      46 |     0 |               707 |
-| es-ES  |  883 |      53 |     0 |               545 |
-| fr-FR  |  936 |       0 |     0 |                 1 |
-| hu-HU  |  856 |      46 |     0 |               590 |
-| it-IT  |  883 |      53 |     0 |               552 |
-| nl-NL  |  856 |      46 |     0 |               711 |
-| pl-PL  |  883 |      53 |     0 |               549 |
-| pt-BR  |  883 |      53 |     0 |               453 |
-| pt-PT  |  883 |      53 |     0 |               559 |
-| ru-RU  |  883 |      53 |     0 |               643 |
-| uk-UA  |  883 |      53 |     0 |               568 |
-| zh-CN  |  829 |      39 |     0 |               677 |
+| be-BY  |  883 |      54 |     0 |               655 |
+| de-DE  |  856 |      47 |     0 |               707 |
+| es-ES  |  883 |      54 |     0 |               545 |
+| fr-FR  |  937 |       0 |     0 |                 1 |
+| hu-HU  |  856 |      47 |     0 |               590 |
+| it-IT  |  883 |      54 |     0 |               552 |
+| nl-NL  |  856 |      47 |     0 |               711 |
+| pl-PL  |  883 |      54 |     0 |               549 |
+| pt-BR  |  883 |      54 |     0 |               453 |
+| pt-PT  |  883 |      54 |     0 |               559 |
+| ru-RU  |  883 |      54 |     0 |               643 |
+| uk-UA  |  883 |      54 |     0 |               568 |
+| zh-CN  |  829 |      40 |     0 |               677 |
 
 <!-- i18n-status:end -->

src/global.d.ts

@@ -747,13 +747,13 @@ declare global {
    * (the preload's own signature, its declaration and the renderer adapter) and a reason added to
    * one copy of a union is a reason the other two refuse to compare against.
    *
-   * - `too-large` is the pack's own byte ceiling. The other five are the config folder's, and each of
-   *   them names a file where one file is what went wrong.
-   * - `bad-name` and `collides` exist because the export and the import have to agree about which
-   *   keys a pack may hold: a name Windows would refuse, and two names differing only in case, are
-   *   both legal on the file system the file was created on and both unusable in a pack.
+   * - `too-large` is the pack's own byte ceiling. `unreadable-config` and `too-many` describe the
+   *   folder as a whole.
+   * - `not-utf8`, `bad-name`, `hidden-character`, and `collides` name the file a pack cannot carry:
+   *   its bytes are not text, its name is unusable on Windows, it contains hidden Unicode, or it
+   *   collides with another name on a case-insensitive file system.
    */
-  type ExportModpackRefusal = "unreadable-config" | "not-utf8" | "bad-name" | "collides" | "too-many" | "too-large"
+  type ExportModpackRefusal = "unreadable-config" | "not-utf8" | "bad-name" | "hidden-character" | "collides" | "too-many" | "too-large"
 
   /**
    * What one apply did, file by file.

src/ipc/handlers/modConfigs.ts

@@ -66,6 +66,25 @@ const WINDOWS_RESERVED_BASE_NAME = /^(con|prn|aux|nul|clock\$|com[0-9¹²³]|lpt
 // eslint-disable-next-line no-control-regex
 const WINDOWS_FORBIDDEN_CHARACTER = /[?*<>|":\u0000-\u001f\u007f]/
 
+/**
+ * Unicode characters a reader must not be allowed to hide or reorder in a file name.
+ *
+ * `Default_Ignorable_Code_Point` includes format characters and other code points that render as
+ * nothing or as blank fillers in the launcher's Chromium. This also covers bidi controls and U+180E.
+ */
+const HIDDEN_UNICODE_NAME_CHARACTER = /[\p{Cf}\p{Default_Ignorable_Code_Point}]/u
+
+/** Makes hidden code points in an on-disk name visible in an export refusal. */
+function showDefaultIgnorables(value: string): string {
+  return [...value]
+    .map((character) => {
+      if (!HIDDEN_UNICODE_NAME_CHARACTER.test(character)) return character
+      const codePoint = character.codePointAt(0) as number
+      return `<U+${codePoint.toString(16).toUpperCase().padStart(4, "0")}>`
+    })
+    .join("")
+}
+
 /** How many times a recovery folder name may be bumped before the apply gives up. */
 const MAX_RECOVERY_FOLDER_ATTEMPTS = 32
 
@@ -87,7 +106,8 @@ const APPLIED_RECORD_NAME = "applied.txt"
  *
  * Every segment is held to what `assertSafeFileName` already allows, and the rest is what Windows
  * adds on top of that: a device name, a character it will not put in a name, a space or a dot at
- * the end. Those are refused here rather than discovered on a Windows machine by everybody else.
+ * the end. Bidi controls and invisible zero-width characters are refused too, so a pack cannot
+ * display a different name to the player than the file on disk.
  *
  * @param value Key as a pack or a dialog sent it.
  * @returns The key, unchanged, once every rule has passed.
@@ -111,7 +131,9 @@ export function assertModConfigKey(value: unknown): string {
     // backslash a pack written on Windows may carry, and anything over 255 characters.
     assertSafeFileName(segment, "mod config key")
 
-    if (WINDOWS_FORBIDDEN_CHARACTER.test(segment)) throw new TypeError("Invalid mod config key")
+    if (WINDOWS_FORBIDDEN_CHARACTER.test(segment) || HIDDEN_UNICODE_NAME_CHARACTER.test(segment)) {
+      throw new TypeError("Invalid mod config key")
+    }
     // A device name and a trailing dot or space are not visible in a file manager, so two keys that
     // differ only by one are one file on Windows and the second write is the only one to land. The
     // regex's own optional extension is what lets a folder called Client past and a folder called
@@ -361,6 +383,9 @@ export async function collectModConfigs(
       assertModConfigKey(file.name)
     } catch (err) {
       logMessage("debug", `${LOG_PREFIX} [EXPORT_MODPACK] ${getErrorMessage(err)}`)
+      if ([...file.name].some((character) => HIDDEN_UNICODE_NAME_CHARACTER.test(character))) {
+        return { ok: false, reason: "hidden-character", name: showDefaultIgnorables(file.name) }
+      }
       return { ok: false, reason: "bad-name", name: file.name }
     }
     const lower = file.name.toLowerCase()

src/renderer/src/features/mods/hooks/useExportModpack.ts

@@ -56,6 +56,10 @@ export function useExportModpack(): ({
       addNotification(t("features.mods.exportModpackConfigBadName", { name: result.name ?? "" }), "error")
       return
     }
+    if (result.reason === "hidden-character") {
+      addNotification(t("features.mods.exportModpackConfigHiddenCharacter", { name: result.name ?? "" }), "error")
+      return
+    }
     if (result.reason === "collides") {
       addNotification(t("features.mods.exportModpackConfigCollides", { name: result.name ?? "" }), "error")
       return

src/renderer/src/locales/en-US.json

@@ -556,6 +556,7 @@
       "serverModsCountTruncated": "and more",
       "serverModsRemoveTruncated": "This folder holds more Mods than the list shows. Removing it clears everything inside it.",
       "exportModpackConfigBadName": "{{name}} cannot travel in a modpack: Windows does not accept that file name. Rename it in the ModConfig folder and export again.",
+      "exportModpackConfigHiddenCharacter": "{{name}} contains invisible Unicode characters and cannot travel in a modpack. The hidden characters are shown as <U+XXXX>; rename it in the ModConfig folder and export again.",
       "exportModpackConfigCollides": "{{name}} differs from another config file only in letter case, and on Windows the two are one file. Rename one of them and export again.",
       "exportModConfigsTitle": "Mod configs to export",
       "exportModConfigsDesc_one": "This modpack will carry {{count}} mod config file from this Installation. Untick the ones it should not carry.",

src/renderer/src/locales/fr-FR.json

@@ -584,6 +584,7 @@
       "serverModsCountTruncated": "et plus",
       "serverModsRemoveTruncated": "Ce dossier contient plus de mods que la liste n'en montre. Le supprimer efface tout ce qu'il contient.",
       "exportModpackConfigBadName": "{{name}} ne peut pas voyager dans un modpack : Windows n'accepte pas ce nom de fichier. Renommez-le dans le dossier ModConfig puis exportez à nouveau.",
+      "exportModpackConfigHiddenCharacter": "{{name}} contient des caractères Unicode invisibles et ne peut pas voyager dans un modpack. Les caractères cachés sont indiqués sous la forme <U+XXXX> ; renommez le fichier dans le dossier ModConfig puis exportez à nouveau.",
       "exportModpackConfigCollides": "{{name}} ne diffère d'un autre fichier de configuration que par la casse, et sous Windows les deux sont un seul fichier. Renommez l'un des deux puis exportez à nouveau.",
       "exportModConfigsTitle": "Configs de mods à exporter",
       "exportModConfigsDesc_one": "Ce modpack emportera {{count}} fichier de config de mod de cette installation. Décochez ceux qu'il ne doit pas emporter.",

tests/ipc/modConfigs.test.ts

@@ -207,6 +207,35 @@ describe("assertModConfigKey", () => {
     assert.throws(() => assertModConfigKey("what?.json"), /Invalid mod config key/)
     assert.equal(assertModConfigKey("ROOM.JSON"), "ROOM.JSON")
   })
+
+  it("refuses bidi controls and zero-width characters that disguise a name", async () => {
+    const { assertModConfigKey } = await import("@src/ipc/handlers/modConfigs")
+
+    // Right-to-left override and bidi controls:
+    assert.throws(() => assertModConfigKey("safe\u202Egnp.json"), /Invalid mod config key/)
+    assert.throws(() => assertModConfigKey("\u202Eevil.json"), /Invalid mod config key/)
+    assert.throws(() => assertModConfigKey("folder\u202A/config.json"), /Invalid mod config key/)
+    assert.throws(() => assertModConfigKey("bidi\u061C.json"), /Invalid mod config key/)
+    assert.throws(() => assertModConfigKey("isolate\u2066.json"), /Invalid mod config key/)
+
+    // Zero-width space and invisible format characters:
+    assert.throws(() => assertModConfigKey("zero\u200Bwidth.json"), /Invalid mod config key/)
+    assert.throws(() => assertModConfigKey("non\u200Cjoiner.json"), /Invalid mod config key/)
+    assert.throws(() => assertModConfigKey("joiner\u200D.json"), /Invalid mod config key/)
+    assert.throws(() => assertModConfigKey("word\u2060joiner.json"), /Invalid mod config key/)
+    assert.throws(() => assertModConfigKey("bom\uFEFF.json"), /Invalid mod config key/)
+    assert.throws(() => assertModConfigKey("mongolian\u180Espace.json"), /Invalid mod config key/)
+  })
+
+  it("refuses representative default-ignorable characters in file and folder segments", async () => {
+    const { assertModConfigKey } = await import("@src/ipc/handlers/modConfigs")
+    const hiddenCharacters = ["\u034F", "\u115F", "\u17B4", "\u180B", "\u180F", "\u2065", "\u3164", "\uFE0F", "\uFFA0", "\uFFF0", "\u{E0000}", "\u{E0080}", "\u{E0100}", "\u{E0FFF}"]
+
+    for (const hidden of hiddenCharacters) {
+      assert.throws(() => assertModConfigKey(`Client/config${hidden}.json`), /Invalid mod config key/)
+      assert.throws(() => assertModConfigKey(`Client/Sub${hidden}/config.json`), /Invalid mod config key/)
+    }
+  })
 })
 
 describe("parseModpackSettings", () => {
@@ -241,6 +270,18 @@ describe("parseModpackSettings", () => {
     assert.deepEqual(Object.keys(good.ok && good.settings), ["a.json"])
   })
 
+  it("refuses a pack carrying bidi controls or zero-width characters in a key and names it", async () => {
+    const { parseModpackSettings } = await import("@src/ipc/handlers/modConfigs")
+
+    const bidiRefused = parseModpackSettings({ "safe\u202Egnp.json": entry("{}") })
+    assert.equal(bidiRefused.ok, false)
+    assert.deepEqual(bidiRefused.ok === false && bidiRefused.refused, { reason: "bad-key", name: "safe\u202Egnp.json" })
+
+    const zeroWidthRefused = parseModpackSettings({ "zero\u200Bwidth.json": entry("{}") })
+    assert.equal(zeroWidthRefused.ok, false)
+    assert.deepEqual(zeroWidthRefused.ok === false && zeroWidthRefused.refused, { reason: "bad-key", name: "zero\u200Bwidth.json" })
+  })
+
   it("refuses two keys that are one file on a case-insensitive filesystem", async () => {
     const { parseModpackSettings } = await import("@src/ipc/handlers/modConfigs")
 
@@ -381,6 +422,16 @@ describe("collectModConfigs", () => {
     assert.deepEqual(collected, { ok: false, reason: "bad-name", name: "trailing .json" })
   })
 
+  it("shows default-ignorable code points in the export refusal", async () => {
+    const { collectModConfigs } = await import("@src/ipc/handlers/modConfigs")
+    mkdirSync(modConfigFolder(), { recursive: true })
+    writeFileSync(join(modConfigFolder(), "config\u034F.json"), "{}", "utf-8")
+
+    const collected = await collectModConfigs(installationPath)
+
+    assert.deepEqual(collected, { ok: false, reason: "hidden-character", name: "config<U+034F>.json" })
+  })
+
   it.skipIf(process.platform === "win32")("refuses two names that differ only in case, and says which one it found second", async () => {
     // Imported here, not at the top: beforeEach resets the module registry, so a top-level import
     // would be a second copy of this module, holding its own state the handlers never see.