fix(7686): username 'false' / 'malformed color: false' for legacy settings.json

[JohnMcLear]

Summary

Closes #7686.

Settings.json files generated before December 2021 still ship "userName": false / "userColor": false for padOptions. These two options are documented as strings, so the boolean was always meaningless — commit 8c857a85a switched the template default to null and explicitly warned: "this change has no effect due to a bug in how pad options are processed; that bug will be fixed in a future commit." The follow-up never landed.

The cascade:

1. clientVars.padOptions.userName === false (legacy) reaches pad.ts:getParams().
2. if (serverValue == null) continue; — false != null, doesn't skip.
3. serverValue = serverValue.toString(); → "false".
4. settings.globalUserName = "false", clientVars.userColor = "false".
5. pad.myUserInfo.colorId = clientVars.userColor → "false".
6. if (settings.globalUserName !== false) → "false" !== false is true → notifyChangeName("false") ships USERINFO_UPDATE with name="false", colorId="false".
7. Server's hex regex throws Error: COLLABROOM: USERINFO_UPDATE: malformed color: false.

The user sees their name as false and a white swatch.

Fix (defense in depth)

• Server boundary — Settings.ts::reloadSettings() coerces padOptions.userName === false and padOptions.userColor === false to null and warns once, matching the existing disableIPlogging shim pattern.
• Client guard — pad.ts userName/userColor callbacks reject empty / literal "false" so URL params like ?userName=false are also a no-op.
• Regression test — tests/backend/specs/padOptionsLegacyDefaults.ts mirrors the shim and asserts it normalizes the legacy boolean, leaves explicit string/null values intact, doesn't touch showChat: false/rtl: false/etc., and does not coerce the string "false" (that's the client guard's job).

Test plan

• pnpm exec tsc --noEmit — clean
• pnpm exec mocha tests/backend/specs/padOptionsLegacyDefaults.ts — 8 passing
• pnpm exec mocha tests/backend/specs/settings.ts tests/backend/specs/ipLoggingSetting.ts tests/backend/specs/padOptionsLegacyDefaults.ts — 48 passing
• CI runs the full backend + Playwright suite
• Reporter (@tpokorra) confirms the warning appears and the bug is gone after upgrading

🤖 Generated with Claude Code

[qodo-code-review]

ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one.

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Fix legacy padOptions boolean false breaking user info updates

🐞 Bug fix

Walkthroughs

Description

• Fix legacy padOptions.userName/userColor=false coercion to string "false"
• Server-side shim normalizes boolean false to null in Settings.ts
• Client-side guard rejects literal "false" string in pad.ts callbacks
• Comprehensive regression test ensures legacy defaults are properly handled

Diagram

flowchart LR
  A["Legacy settings.json<br/>userName/userColor=false"] -->|"getParams() coerces<br/>to string"| B["String 'false'<br/>propagates"]
  B -->|"Fails sentinel check<br/>!== false"| C["USERINFO_UPDATE<br/>with name='false'"]
  C -->|"Hex regex rejects"| D["Error: malformed color"]
  E["Server shim:<br/>false → null"] -->|"Normalizes at boundary"| F["Downstream sees<br/>sentinel null"]
  G["Client guard:<br/>rejects 'false' string"] -->|"URL params no-op"| F
  F -->|"Prevents propagation"| H["Bug fixed"]

Loading

File Changes

1. src/node/utils/Settings.ts 🐞 Bug fix +18/-0

Server-side shim for legacy boolean false normalization

• Added shim in reloadSettings() to coerce legacy padOptions.userName and padOptions.userColor
 boolean false values to null
• Logs warning once per occurrence to inform operators about legacy settings.json files
• Matches existing disableIPlogging shim pattern for consistency

src/node/utils/Settings.ts

---

2. src/static/js/pad.ts 🐞 Bug fix +9/-0

Client-side guards reject false sentinel string

• Added guard in userName callback to reject empty values and literal string "false"
• Added guard in userColor callback to reject empty values and literal string "false"
• Prevents URL parameters like ?userName=false from propagating as user name/color

src/static/js/pad.ts

---

3. src/tests/backend/specs/padOptionsLegacyDefaults.ts 🧪 Tests +76/-0

Comprehensive regression tests for legacy defaults shim

• New regression test file with 8 test cases covering the legacy boolean false shim
• Tests verify coercion of userName and userColor false to null
• Tests confirm other padOptions keys with legitimate false values are unaffected
• Tests ensure string "false" is not coerced by server shim (client guard responsibility)

src/tests/backend/specs/padOptionsLegacyDefaults.ts

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. padOptions shim can crash ✓ Resolved 🐞 Bug ☼ Reliability

Description

Settings.reloadSettings() unconditionally indexes and assigns settings.padOptions[key]; if
settings.padOptions is null or a primitive (possible via settings.json), reloadSettings() will throw
a TypeError during startup/reload.

Code

src/node/utils/Settings.ts[R1098-1105]

+    for (const key of ['userName', 'userColor'] as const) {
+      if ((settings.padOptions as any)[key] === false) {
+        logger.warn(
+            `padOptions.${key}=false is a legacy default (pre-2021) and is ` +
+            `now treated as null. Update settings.json to use null instead ` +
+            `to silence this warning.`);
+        (settings.padOptions as any)[key] = null;
+      }

Evidence

The new shim directly reads and writes properties on settings.padOptions. Separately,
storeSettings() will overwrite any top-level setting (including padOptions) with a non-object value
from settings.json, so padOptions can become null/boolean/string; property access/assignment on such
values can throw (especially null / strict-mode primitive assignment).

src/node/utils/Settings.ts[1098-1105]
src/node/utils/Settings.ts[811-833]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`reloadSettings()` assumes `settings.padOptions` is a non-null object when normalizing legacy `userName/userColor` values. If `settings.json` sets `padOptions` to `null` or another non-object, the shim can throw (TypeError) during startup or settings reload.
### Issue Context
`storeSettings()` can overwrite `settings.padOptions` with any non-object value from `settings.json`, so a bad config can reach this new code path.
### Fix Focus Areas
- src/node/utils/Settings.ts[1098-1105]
- src/node/utils/Settings.ts[811-833]
### Suggested change
Add a type/shape guard before indexing/assigning:
- Only run the shim if `settings.padOptions != null && typeof settings.padOptions === 'object' && !Array.isArray(settings.padOptions)`.
- Otherwise, skip the shim (and optionally log a clear warning/error about invalid `padOptions` type).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[JohnMcLear]

@qodo-free-for-open-source-projects re: padOptions shim crash —

Verified. storeSettings() overwrites settings.padOptions raw if settings.json declares it as anything other than a plain object. (Existing call sites in Pad.ts already assume padOptions is an object, so a primitive padOptions would already kill Etherpad on the first pad open — the shim isn't a new failure mode, just an earlier one.)

Fixed in 2f875af: added a shape guard (!= null && typeof === 'object' && !Array.isArray) around the shim, and extended padOptionsLegacyDefaults.ts with three cases asserting the guard handles null / primitives / arrays without throwing. 11 specs passing.