From da87a4fca2ebf8ebd627c2305cecb2470d5d6c21 Mon Sep 17 00:00:00 2001 From: John McLear Date: Thu, 7 May 2026 16:32:17 +0800 Subject: [PATCH 01/25] fix(7686): username 'false' / 'malformed color: false' for legacy settings.json (#7688) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(7686): legacy padOptions.userName/userColor=false breaks pad Settings.json files generated before December 2021 used `false` as the default for these two string options (commit 8c857a85a switched the template default to `null` and noted "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). pad.ts:getParams() then runs `false.toString()`, the resulting string "false" passes the `!== false` sentinel check at _afterHandshake, and notifyChangeName ships USERINFO_UPDATE with name="false" and colorId="false" (clobbered via clientVars.userColor). The server's hex regex rejects the colour and throws `malformed color: false`; the user sees their name as "false" and a white swatch. Defense in depth: - Server: Settings.ts::reloadSettings() coerces legacy boolean false to null for padOptions.userName / padOptions.userColor and warns the operator, matching the existing disableIPlogging shim pattern. - Client: the pad.ts userName / userColor callbacks reject the literal "false" string so URL params (?userName=false) and any other path that surfaces the sentinel as a string are also no-ops. - Backend regression test mirrors the shim and asserts it normalizes legacy false, leaves explicit values intact, leaves null untouched, does not coerce other padOptions keys, and does not coerce the string "false" (that path is the client guard's responsibility). Closes #7686 Co-Authored-By: Claude Opus 4.7 (1M context) * fix(7686): guard padOptions shim against non-object config (Qodo) Qodo flagged that storeSettings() will overwrite settings.padOptions raw with whatever settings.json supplies — including null, primitives, or arrays — which would make the new userName/userColor shim crash on property access. Add a shape guard so the shim is a no-op for malformed padOptions, and extend the regression test to cover null / primitive / array shapes. This doesn't change which configs work (Pad.ts also assumes padOptions is an object and would already crash on a null padOptions when a pad is opened) but it stops the shim from being the loud thing in the stack trace if someone hits it. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- src/node/utils/Settings.ts | 24 +++++ src/static/js/pad.ts | 9 ++ .../backend/specs/padOptionsLegacyDefaults.ts | 98 +++++++++++++++++++ 3 files changed, 131 insertions(+) create mode 100644 src/tests/backend/specs/padOptionsLegacyDefaults.ts diff --git a/src/node/utils/Settings.ts b/src/node/utils/Settings.ts index 71bb4dd1e7d..54840f03a7a 100644 --- a/src/node/utils/Settings.ts +++ b/src/node/utils/Settings.ts @@ -1087,6 +1087,30 @@ export const reloadSettings = () => { settings.privacyBanner.dismissal = 'dismissible'; } + // Settings.json files generated before December 2021 used `false` as the + // default for these string options. The client treats the boolean `false` + // as a sentinel meaning "no enforced value", but the dispatch in + // pad.ts:getParams() coerces the boolean to the string "false" before + // applying it, which then propagates as the user's name and color and + // triggers `malformed color: false` on the server (#7686). Normalize + // legacy booleans to null at the boundary so downstream code sees the + // expected sentinel. Guard against a malformed padOptions (null, array, + // primitive) — storeSettings() will overwrite it raw if settings.json + // declares it as anything other than a plain object. + if (settings.padOptions != null + && typeof settings.padOptions === 'object' + && !Array.isArray(settings.padOptions)) { + 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; + } + } + } + // Init logging config settings.logconfig = defaultLogConfig( settings.loglevel ? settings.loglevel : defaultLogLevel, diff --git a/src/static/js/pad.ts b/src/static/js/pad.ts index 72364c26149..12406197ecf 100644 --- a/src/static/js/pad.ts +++ b/src/static/js/pad.ts @@ -136,6 +136,14 @@ const getParameters = [ name: 'userName', checkVal: null, callback: (val) => { + // The default for globalUserName/globalUserColor is the boolean `false` + // (sentinel meaning "no enforced value"). Older settings.json files used + // boolean `false` for these options too, which getParams() coerces to + // the string "false" — that fooled the !== false sentinel checks at + // _afterHandshake and shipped the literal string "false" as the user's + // name and color (#7686). Reject the sentinel string here so URL + // parameters like ?userName=false also no-op. + if (!val || val === 'false') return; settings.globalUserName = val; clientVars.userName = val; }, @@ -144,6 +152,7 @@ const getParameters = [ name: 'userColor', checkVal: null, callback: (val) => { + if (!val || val === 'false') return; settings.globalUserColor = val; clientVars.userColor = val; }, diff --git a/src/tests/backend/specs/padOptionsLegacyDefaults.ts b/src/tests/backend/specs/padOptionsLegacyDefaults.ts new file mode 100644 index 00000000000..73d0a08d588 --- /dev/null +++ b/src/tests/backend/specs/padOptionsLegacyDefaults.ts @@ -0,0 +1,98 @@ +'use strict'; + +import {strict as assert} from 'assert'; + +describe(__filename, function () { + // Replicates the shim block from Settings.ts::reloadSettings so we can + // assert the mapping without rebooting the whole server in this spec. + // Keep this in lockstep with the production block — if you change the + // shim there, change it here too. + const applyShim = (padOptions: any) => { + if (padOptions != null && typeof padOptions === 'object' && !Array.isArray(padOptions)) { + for (const key of ['userName', 'userColor']) { + if (padOptions[key] === false) padOptions[key] = null; + } + } + return padOptions; + }; + + describe('legacy padOptions.userName/userColor=false → null shim', function () { + it('coerces userName=false to null', function () { + const out = applyShim({userName: false}); + assert.strictEqual(out.userName, null); + }); + + it('coerces userColor=false to null', function () { + const out = applyShim({userColor: false}); + assert.strictEqual(out.userColor, null); + }); + + it('coerces both legacy defaults in one pass', function () { + const out = applyShim({userName: false, userColor: false}); + assert.strictEqual(out.userName, null); + assert.strictEqual(out.userColor, null); + }); + + it('leaves an explicit string userName intact', function () { + const out = applyShim({userName: 'Etherpad User'}); + assert.strictEqual(out.userName, 'Etherpad User'); + }); + + it('leaves an explicit hex userColor intact', function () { + const out = applyShim({userColor: '#ff9900'}); + assert.strictEqual(out.userColor, '#ff9900'); + }); + + it('leaves null values untouched', function () { + const out = applyShim({userName: null, userColor: null}); + assert.strictEqual(out.userName, null); + assert.strictEqual(out.userColor, null); + }); + + it('does not affect other padOptions keys that legitimately use false', function () { + // showChat:false, rtl:false, etc. are real, meaningful values — only + // the two string options carry the legacy boolean sentinel. + const out = applyShim({ + userName: false, + userColor: false, + showChat: false, + rtl: false, + useMonospaceFont: false, + }); + assert.strictEqual(out.userName, null); + assert.strictEqual(out.userColor, null); + assert.strictEqual(out.showChat, false); + assert.strictEqual(out.rtl, false); + assert.strictEqual(out.useMonospaceFont, false); + }); + + it('does not coerce the string "false" — that is handled in the client guard', function () { + // The server-side shim only normalizes the boolean sentinel from + // legacy settings.json. URL-supplied or stringified "false" is + // rejected by pad.ts::getParameters.userName/userColor callbacks. + const out = applyShim({userName: 'false', userColor: 'false'}); + assert.strictEqual(out.userName, 'false'); + assert.strictEqual(out.userColor, 'false'); + }); + + it('skips the shim if padOptions is null', function () { + // storeSettings() overwrites settings.padOptions raw if settings.json + // declares it as a non-object — the shim must not throw on that. + assert.doesNotThrow(() => applyShim(null)); + assert.strictEqual(applyShim(null), null); + }); + + it('skips the shim if padOptions is a primitive', function () { + assert.doesNotThrow(() => applyShim(false)); + assert.doesNotThrow(() => applyShim('not an object')); + assert.doesNotThrow(() => applyShim(42)); + }); + + it('skips the shim if padOptions is an array', function () { + const arr: any = [false, false]; + assert.doesNotThrow(() => applyShim(arr)); + // Arrays pass through untouched — index 0/1 (numeric) are not coerced. + assert.deepEqual(applyShim(arr), [false, false]); + }); + }); +}); From b751fd7b9e705c5d94592718d121f80e8db20b15 Mon Sep 17 00:00:00 2001 From: John McLear Date: Thu, 7 May 2026 17:01:04 +0800 Subject: [PATCH 02/25] feat(settings): enable Pad-wide Settings by default; fix misleading modal title (#7679) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(settings): enable Pad-wide Settings by default; fix misleading modal title The creator-owned Pad-wide Settings feature (#7545) shipped behind a flag that defaulted to false. With the flag off the modal still rendered an H1 of `pad.settings.padSettings` ("Pad-wide Settings") for *every* user, even though no pad-wide controls were ever shown. Two readers in different browsers both saw "Pad-wide Settings" as the modal title, which looked like a creator-gate regression but was just a copy bug. Two changes: 1. Flip the default of `enablePadWideSettings` to `true` (Settings.ts plus both settings templates). With the feature on, the creator (revision-0 author) gets a real "Pad-wide Settings" section gated by `clientVars.canEditPadSettings`, while every other user sees only "User Settings" — matching the design intent of #7545. This is a behavior change, so the settings comments are expanded to describe what the toggle now does. 2. Drop the conditional H1 in `src/templates/pad.html` and always use `pad.settings.title` ("Settings"). Operators who explicitly disable the feature shouldn't see a label that lies about a section that isn't rendered. Adds backend regression coverage in `tests/backend/specs/socketio.ts`: - Different browsers (different cookie jars => different authorIDs): only the first joiner gets `canEditPadSettings: true`. - Same browser, two tabs (shared HttpOnly token cookie => same authorID): both connections are the same identity, both correctly land on the creator path. * test(settings): regression coverage for the settings modal H1 Asserts the rendered `/p/` HTML always uses `data-l10n-id="pad.settings.title"` for the modal heading, regardless of `enablePadWideSettings`. Catches a re-introduction of the old conditional that printed "Pad-wide Settings" for every user when the feature was off. Action of Qodo review feedback on PR #7679. --- settings.json.docker | 7 ++- settings.json.template | 7 ++- src/node/utils/Settings.ts | 2 +- src/templates/pad.html | 4 -- .../backend/specs/settingsModalHeading.ts | 45 ++++++++++++++ src/tests/backend/specs/socketio.ts | 62 ++++++++++++++++++- 6 files changed, 117 insertions(+), 10 deletions(-) create mode 100644 src/tests/backend/specs/settingsModalHeading.ts diff --git a/settings.json.docker b/settings.json.docker index 8755d99e3a9..d640536a817 100644 --- a/settings.json.docker +++ b/settings.json.docker @@ -247,9 +247,12 @@ /** * Enable creator-owned Pad-wide Settings and new-pad default seeding from My View. - * Disabled by default to preserve the legacy single-settings behavior. + * The pad creator (revision-0 author) gets the "Pad-wide Settings" section, + * which lets them set defaults and optionally enforce them for other users. + * Other users see only "User Settings" (their own view options). + * Set to false to revert to the legacy single-settings behavior. **/ - "enablePadWideSettings": "${ENABLE_PAD_WIDE_SETTINGS:false}", + "enablePadWideSettings": "${ENABLE_PAD_WIDE_SETTINGS:true}", /* * Optional privacy banner. See settings.json.template for full field docs. diff --git a/settings.json.template b/settings.json.template index 209b1721f39..61be12fbf73 100644 --- a/settings.json.template +++ b/settings.json.template @@ -733,9 +733,12 @@ /** * Enable creator-owned Pad-wide Settings and new-pad default seeding from My View. - * Disabled by default to preserve the legacy single-settings behavior. + * The pad creator (revision-0 author) gets the "Pad-wide Settings" section, + * which lets them set defaults and optionally enforce them for other users. + * Other users see only "User Settings" (their own view options). + * Set to false to revert to the legacy single-settings behavior. **/ - "enablePadWideSettings": "${ENABLE_PAD_WIDE_SETTINGS:false}", + "enablePadWideSettings": "${ENABLE_PAD_WIDE_SETTINGS:true}", /* * Optional privacy banner shown once the pad loads. Disabled by default. diff --git a/src/node/utils/Settings.ts b/src/node/utils/Settings.ts index 54840f03a7a..68f51da1fc6 100644 --- a/src/node/utils/Settings.ts +++ b/src/node/utils/Settings.ts @@ -369,7 +369,7 @@ const settings: SettingsType = { }, updateServer: "https://static.etherpad.org", enableDarkMode: true, - enablePadWideSettings: false, + enablePadWideSettings: true, allowPadDeletionByAllUsers: false, privacyBanner: { enabled: false, diff --git a/src/templates/pad.html b/src/templates/pad.html index ecac35ad607..5212b004606 100644 --- a/src/templates/pad.html +++ b/src/templates/pad.html @@ -127,11 +127,7 @@