fix(settings): expose plugin ep_* config blocks to CJS require (#8110)

[JohnMcLear]

Fixes #8109.
Fixes #8110.

Supersedes #8113 (@AkprasadoP), which proposed the same resync-after-reloadSettings() fix for #8109; credited in the changelog.

What's actually broken

Not the authenticate hook. /admin-auth/ is handled by webaccess.checkAccess (src/node/hooks/express/webaccess.ts:76), which is app.use'd globally, and it calls aCallFirst0('authenticate', ctx) at line 195 exactly like every other path. openapi-admin.ts is only the hand-authored OpenAPI document, not a handler. Verified by reproduction: ep_hash_auth does log on admin login attempts.

The real bug is that plugins can't see their own settings.json config block at all.

Plugins read configuration from a top-level ep_* block via require('ep_etherpad-lite/node/utils/Settings'). The CJS-compatibility shim added in #7421 installs accessor properties on module.exports for Object.keys(settings) — but it ran once, at module-evaluation time, and ep_* blocks are only merged onto the settings object later, by the reloadSettings() call at the bottom of that same module (storeSettings explicitly admits i.indexOf('ep_') === 0). No accessor was ever defined for them.

Measured in a running server with ep_hash_auth@11.0.24 installed and ep_hash_auth.hash_dir set in settings.json:

direct= undefined                          <- settings.ep_hash_auth  (what plugins read)
viaDefault= {"hash_dir":"…/users"}         <- settings.default.ep_hash_auth
hasOwn= false

Why it surfaced as a 401 on the admin panel

hash_dir silently reverted to ep_hash_auth's built-in /var/etherpad/users → every hash file read failed → the hook returned [false] → core's basic-auth fallback found no password on the settings.json user → 401. The reporter's diagnostic was further obscured because Settings.ts:72 rebinds console.log to the settings log4js category, so ep_hash_auth's own log lines print as [INFO] settings - … with nothing identifying the plugin.

Blast radius

Every plugin that reads settings.ep_<name> has been silently running on defaults since the shim shipped in 2.7.0 — ~20 in the ether org alone: ep_oauth, ep_helmet, ep_email_notifications, ep_themes, ep_search, ep_comments_page, ep_spellcheck, ep_stable_authorid, ep_pad_title, ep_author_hover, ep_author_follow, ep_headerauth, ep_mammoth, ep_print, ep_real_time_chat, ep_what_have_i_missed, …

The fix

Extract the shim into syncCjsExports() and call it again at the end of reloadSettings(), so keys that exist only because the operator put them in settings.json get accessors as soon as they are loaded.

Verification

Real server on :9014, fresh develop, ep_hash_auth@11.0.24 installed from npm, sha512 hash file under hash_dir:

Scenario	before	after
POST /admin-auth/ Basic, hash_dir user, .adm = true	401	200
session re-check with the resulting cookie	—	200
wrong password	401	401
users.<name>.hash in settings.json (never touches ep_hash_auth)	200	200

• New backend regression test fails on pre-fix code, passes after.
• Full backend suite: 1667 passing, 0 failing.
• tsc --noEmit clean.

Note for #8110 specifically

There is a second, independent gotcha on the plugin side (not core): ep_hash_auth's hash_dir branch replaces settings.users[username] with {username, is_admin: adm, displayname}, where adm comes from <hash_dir>/<user>/.adm or ep_hash_auth.hash_adm. So is_admin: true in settings.json is discarded and you get a 403 instead of a 200 unless .adm contains true. Measured and confirmed.

Reported by @tris-ots.

Follow-up (rebase)

• Rebased onto current develop; the changelog entry had ended up under the already-released 3.3.4 heading, so it now lives under a new 3.3.6 section and references Settings CJS mirror is built before reloadSettings(), so plugin settings hashes are never visible to CJS plugins #8109 too.
• Test additionally asserts the key is enumerable on the CJS export and that the accessor returns the same live object as .default.
• Verified again: regression test fails on develop's Settings.ts (plugin ep_* settings must be reachable via CJS require), passes with the fix; settings spec 33 passing; vitest 840/840; tsc --noEmit clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_012kA75NPq8nGRidAwhPXeCi

[qodo-code-review]

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

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

PR Summary by Qodo

Fix CJS Settings exports for plugin ep_* config blocks

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 10-20 Minutes

AI Description

• Re-sync CommonJS Settings exports after reload so top-level ep_* plugin blocks are visible.
• Add backend regression test covering reloadSettings() introducing new ep_* keys.
• Document the plugin-settings visibility fix in the changelog.

Diagram

graph TD
  A["Settings.ts eval"] --> B["syncCjsExports()"]
  C[/"settings.json"/] --> D["reloadSettings()"] --> E["settings object"] --> B --> F["module.exports (CJS)"] --> G(["Plugin require()"])
  subgraph Legend
    direction LR
    _file[/"File"/] ~~~ _fn["Function/module"] ~~~ _ext(["External consumer"])
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Proxy-based CJS export (dynamic property forwarding)

• ➕ Automatically exposes new keys without re-running a sync step after reloads
• ➕ Potentially reduces reliance on explicit accessor definition lifecycle
• ➖ Higher complexity and less familiar debugging ergonomics for plugin authors
• ➖ Potential compatibility/perf concerns vs. simple defineProperty accessors
• ➖ Harder to type and reason about under mixed ESM/CJS interop

Recommendation: The chosen approach (extract shim into syncCjsExports() and invoke it after each reloadSettings()) is the best fit: it preserves the existing CJS contract, stays simple/portable (no Proxy magic), and directly targets the lifecycle bug where ep_* keys appear only after the initial module-eval shim run.

Files changed (3) +60 / -10

Bug fix (1) +23 / -10

Settings.tsRe-run CJS export shim after 'reloadSettings()' +23/-10

Re-run CJS export shim after 'reloadSettings()'

• Refactors the CommonJS compatibility shim into 'syncCjsExports()' and runs it at module load and after every 'reloadSettings()' call so newly-merged top-level keys (notably plugin 'ep_*' blocks) become accessible via 'require()'.

src/node/utils/Settings.ts

Tests (1) +36 / -0

settings.tsAdd regression test for ep_* blocks after settings reload +36/-0

Add regression test for ep_* blocks after settings reload

• Introduces a backend regression test that writes a temporary settings file containing a new 'ep_*' block, runs 'reloadSettings()', and asserts the key is reachable on the CJS require result. Cleans up by removing both the settings key and the exported accessor to avoid cross-test pollution.

src/tests/backend/specs/settings.ts

Documentation (1) +1 / -0

CHANGELOG.mdDocument plugin 'settings.ep_*' CJS visibility fix +1/-0

Document plugin 'settings.ep_*' CJS visibility fix

• Adds a notable-fix entry explaining why top-level 'ep_*' plugin config blocks were invisible via 'require()' and that exports are now re-synced after settings loads.

CHANGELOG.md

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

Code Review by Qodo

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

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗