Skip to content

feat: make cookie names configurable with prefix setting - #7450

Merged
JohnMcLear merged 5 commits into
ether:developfrom
JohnMcLear:fix/customizable-cookie-names-664
Apr 5, 2026
Merged

feat: make cookie names configurable with prefix setting#7450
JohnMcLear merged 5 commits into
ether:developfrom
JohnMcLear:fix/customizable-cookie-names-664

Conversation

@JohnMcLear

@JohnMcLear JohnMcLear commented Apr 4, 2026

Copy link
Copy Markdown
Member

Summary

Adds a cookie.prefix setting (default "") that can be set to prefix all cookie names, preventing conflicts with other applications on the same domain.

This resolves a 13-year-old issue where generic cookie names like sessionID and token can conflict with other web frameworks' cookies when Etherpad shares a domain.

Changes

New setting: cookie.prefix in settings.json (default: "" — no prefix, fully backward compatible)

Example with "prefix": "ep_":

Before After
token ep_token
sessionID ep_sessionID
language ep_language
prefs / prefsHttp ep_prefs / ep_prefsHttp
express_sid ep_express_sid

Backward compatibility: Default prefix is empty string, so no cookies change on upgrade. Server-side cookie reads also fall back to unprefixed names for migration when switching to a prefix.

Test plan

  • Type check passes
  • Backend tests pass (754/754)

Fixes #664

🤖 Generated with Claude Code

Add cookie.prefix setting (default "ep_") that gets prepended to all
cookie names set by Etherpad. This prevents conflicts with other
applications on the same domain that use generic cookie names like
"sessionID" or "token".

Affected cookies: token, sessionID, language, prefs/prefsHttp,
express_sid.

The prefix is passed to the client via clientVars.cookiePrefix in the
bootstrap templates so it's available before the handshake. Server-side
cookie reads fall back to unprefixed names for backward compatibility
during migration.

Fixes ether#664

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Review Summary by Qodo

Make cookie names configurable with prefix setting

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Add configurable cookie.prefix setting (default "ep_") to prevent cookie name conflicts
• Prepend prefix to all Etherpad cookies: token, sessionID, language, prefs/prefsHttp,
  express_sid
• Pass cookiePrefix to client via clientVars for consistent client-side cookie handling
• Implement backward compatibility with fallback to unprefixed cookie names during migration
Diagram
flowchart LR
  A["Settings: cookie.prefix<br/>default: ep_"] -->|"passed to client"| B["clientVars.cookiePrefix"]
  A -->|"used by server"| C["Express session<br/>& cookie handlers"]
  B -->|"read by"| D["Client-side JS<br/>pad.ts, timeslider.ts<br/>pad_cookie.ts, pad_editor.ts"]
  C -->|"prefixed cookies"| E["token, sessionID<br/>language, prefs"]
  D -->|"prefixed cookies"| E
  C -->|"fallback to unprefixed"| F["Backward compatibility"]
Loading

Grey Divider

File Changes

1. src/node/utils/Settings.ts ⚙️ Configuration changes +2/-0

Add cookie prefix setting type and default

src/node/utils/Settings.ts


2. src/node/hooks/express.ts ✨ Enhancement +1/-1

Prefix express session cookie name

src/node/hooks/express.ts


3. src/node/hooks/express/importexport.ts ✨ Enhancement +5/-1

Read prefixed cookies with fallback

src/node/hooks/express/importexport.ts


View more (11)
4. src/node/hooks/express/tokenTransfer.ts ✨ Enhancement +4/-2

Write prefixed token and prefs cookies

src/node/hooks/express/tokenTransfer.ts


5. src/node/padaccess.ts ✨ Enhancement +6/-1

Read prefixed session and token cookies

src/node/padaccess.ts


6. src/node/handler/PadMessageHandler.ts ✨ Enhancement +1/-0

Pass cookie prefix to client via clientVars

src/node/handler/PadMessageHandler.ts


7. src/static/js/pad.ts ✨ Enhancement +6/-4

Use prefixed cookies for language and token

src/static/js/pad.ts


8. src/static/js/pad_cookie.ts ✨ Enhancement +2/-1

Prefix prefs cookie name on client side

src/static/js/pad_cookie.ts


9. src/static/js/pad_editor.ts ✨ Enhancement +2/-1

Use prefixed language cookie in editor

src/static/js/pad_editor.ts


10. src/static/js/timeslider.ts ✨ Enhancement +3/-2

Use prefixed token cookie in timeslider

src/static/js/timeslider.ts


11. src/templates/padBootstrap.js ✨ Enhancement +1/-0

Include cookie prefix in bootstrap clientVars

src/templates/padBootstrap.js


12. src/templates/padViteBootstrap.js ✨ Enhancement +1/-0

Add cookie prefix to Vite bootstrap template

src/templates/padViteBootstrap.js


13. src/templates/timeSliderBootstrap.js ✨ Enhancement +1/-0

Include cookie prefix in timeslider bootstrap

src/templates/timeSliderBootstrap.js


14. settings.json.template 📝 Documentation +7/-0

Document new cookie prefix configuration option

settings.json.template


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Apr 4, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (1) 🎨 UX Issues (0)

Grey Divider


Action required

1. sessionID still default cookie 📎 Requirement gap ⛨ Security
Description
Because settings.cookie.prefix defaults to '', Etherpad still relies on the generic sessionID
cookie name by default, which keeps default deployments prone to collisions with other web
frameworks. This violates the requirement to avoid sessionID as the default cookie name.
Code

src/node/padaccess.ts[R8-12]

+  const p = settings.cookie.prefix;
 const accessObj = await securityManager.checkAccess(
-      req.params.pad, req.cookies.sessionID, req.cookies.token, user);
+      req.params.pad,
+      req.cookies[`${p}sessionID`] || req.cookies.sessionID,
+      req.cookies[`${p}token`] || req.cookies.token,
Evidence
The checklist requires the default session cookie name to be Etherpad-specific. The default
configuration sets cookie.prefix to an empty string, and the access check still reads sessionID
from cookies (directly or via an empty-prefix lookup), so the default remains the generic
sessionID.

Avoid using the generic sessionID cookie name by default
src/node/utils/Settings.ts[532-536]
src/node/padaccess.ts[8-13]

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

## Issue description
Default deployments still use the generic `sessionID` cookie name because `cookie.prefix` defaults to `''`, which does not satisfy the requirement to avoid `sessionID` by default.
## Issue Context
The code already supports migration by falling back to unprefixed cookie names when reading (`prefixed || unprefixed`). This makes it feasible to change the default to an Etherpad-specific namespace while keeping compatibility for existing deployments/portals.
## Fix Focus Areas
- src/node/utils/Settings.ts[530-538]
- src/node/padaccess.ts[8-13]

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


2. Foreign token blocks access 🐞 Bug ≡ Correctness
Description
In pad access checks, when cookie.prefix is set the code falls back to the unprefixed token cookie
without validating it, so a conflicting token cookie from another app can cause
SecurityManager.checkAccess() to deny with “invalid author token”. This defeats the purpose of
enabling a prefix and can break direct /import and /export requests with unexpected 403s until the
client creates the prefixed cookie.
Code

src/node/padaccess.ts[R8-13]

+  const p = settings.cookie.prefix;
const accessObj = await securityManager.checkAccess(
-      req.params.pad, req.cookies.sessionID, req.cookies.token, user);
+      req.params.pad,
+      req.cookies[`${p}sessionID`] || req.cookies.sessionID,
+      req.cookies[`${p}token`] || req.cookies.token,
+      user);
Evidence
padaccess.ts (and similarly importexport.ts) passes req.cookies[${p}token] || req.cookies.token
into SecurityManager.checkAccess(). SecurityManager.checkAccess() explicitly denies access if a
token is non-null but fails padutils.isValidAuthorToken(), so any third-party token cookie value
will actively cause a denial rather than being ignored when a prefix is configured.

src/node/padaccess.ts[6-13]
src/node/hooks/express/importexport.ts[76-86]
src/node/db/SecurityManager.ts[112-126]

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

## Issue description
When `cookie.prefix` is configured, server-side routes that call `SecurityManager.checkAccess()` fall back to the unprefixed `token` cookie even if it belongs to another app. If that cookie is present but not a valid Etherpad author token, `SecurityManager.checkAccess()` returns DENY, causing 403s on routes like import/export.
## Issue Context
The whole point of `cookie.prefix` is to avoid cookie collisions on shared domains. The current fallback logic can still be triggered by other frameworks’ generic `token` cookies, producing denials instead of ignoring the foreign value.
## Fix Focus Areas
- Add a small helper to compute the token passed to `checkAccess`:
- Prefer the prefixed cookie.
- Only fall back to the unprefixed cookie **if it passes** `padutils.isValidAuthorToken()` (or otherwise treat it as absent).
- Apply the helper consistently anywhere server-side reads `req.cookies.token` for access decisions.
### Suggested focus locations
- src/node/padaccess.ts[6-13]
- src/node/hooks/express/importexport.ts[76-86]
- src/node/db/SecurityManager.ts[112-126] (reference behavior; no change necessarily required here)

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


3. Unescaped prefix in RegExp🐞 Bug ≡ Correctness
Description
src/static/js/l10n.ts interpolates window.clientVars.cookiePrefix directly into a RegExp
constructor, so a configured prefix containing regex metacharacters can throw at runtime or match
the wrong cookie, breaking localization selection.
Code

src/static/js/l10n.ts[R6-8]

+const cp = (window as any).clientVars?.cookiePrefix || '';
+let language = document.cookie.match(new RegExp(`${cp}language=((\\w{2,3})(-\\w+)?)`))
+    || document.cookie.match(/language=((\w{2,3})(-\w+)?)/);
Evidence
The cookie prefix is explicitly configurable (settings.json.template), but l10n.ts uses it unescaped
in a dynamically constructed RegExp, which can become syntactically invalid or change matching
semantics when the prefix contains special characters.

settings.json.template[387-394]
src/static/js/l10n.ts[6-8]

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

## Issue description
`src/static/js/l10n.ts` constructs a `RegExp` using the configured cookie prefix without escaping. If the prefix contains any regex metacharacters (for example `.`, `+`, `[`, `\`), `new RegExp(...)` can throw or match incorrectly, breaking language detection.
### Issue Context
The prefix is a user setting (`cookie.prefix`) and is not constrained to regex-safe characters.
### Fix Focus Areas
- src/static/js/l10n.ts[6-8]
### Suggested fix
- Escape the prefix before inserting it into the regex, e.g.:
- `const esc = (s) => s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');`
- `new RegExp(`${esc(cp)}language=((\\w{2,3})(-\\w+)?)`)`
- Keep the unprefixed fallback match as-is for migration.

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


View more (5)
4. cookie.prefix enabled by default📘 Rule violation ☼ Reliability
Description
The new cookie-prefixing behavior is enabled by default via prefix: 'ep_', which changes cookie
names even if an admin does not opt in. This violates the requirement that new features be behind a
feature flag and disabled by default.
Code

src/node/utils/Settings.ts[R532-535]

cookie: {
keyRotationInterval: 1 * 24 * 60 * 60 * 1000,
+    prefix: 'ep_',
sameSite: 'lax',
Evidence
PR Compliance ID 5 requires new features to be disabled by default. The default settings now set
settings.cookie.prefix to ep_, and this value is used throughout the PR to change cookie names.

src/node/utils/Settings.ts[532-535]
Best Practice: Repository guidelines

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

## Issue description
Cookie name prefixing is enabled by default (`prefix: 'ep_'`), which changes behavior without an explicit opt-in.
## Issue Context
Compliance requires new features to be behind a feature flag and disabled by default.
## Fix Focus Areas
- src/node/utils/Settings.ts[532-535]
- settings.json.template[385-395]

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


5. express_sid cookie rename breaks📘 Rule violation ☼ Reliability
Description
The express-session cookie name is changed to ${settings.cookie.prefix}express_sid with no
migration fallback, which will invalidate existing login sessions on upgrade. This is a
backward-compatibility break introduced by a config/default change without a safe migration path.
Code

src/node/hooks/express.ts[R210-214]

// Set the cookie name to a javascript identifier compatible string. Makes code handling it
// cleaner :)
-    name: 'express_sid',
+    name: `${settings.cookie.prefix}express_sid`,
cookie: {
maxAge: sessionLifetime || undefined, // Convert 0 to null.
Evidence
PR Compliance ID 3 requires backward compatibility where possible for config-facing changes. The PR
changes the express-session cookie name to a prefixed value, but existing deployments will still
have express_sid cookies (as documented), and the new code does not show compatibility handling
for the old cookie name.

src/node/hooks/express.ts[210-214]
doc/cookies.md[5-10]
Best Practice: Repository guidelines

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

## Issue description
Changing the express-session cookie name to a prefixed value invalidates existing sessions because the old `express_sid` cookie will no longer be read.
## Issue Context
This PR introduces `settings.cookie.prefix` and applies it to the express-session cookie name. Backward compatibility should be preserved where feasible (or the default must not break existing behavior).
## Fix Focus Areas
- src/node/hooks/express.ts[203-235]
- src/node/utils/Settings.ts[532-535]
- doc/cookies.md[5-10]

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


6. Language cookie not read🐞 Bug ≡ Correctness
Description
The UI now persists language to ${cookiePrefix}language, but the l10n bootstrap still only looks
for an unprefixed language cookie, so language selection will not persist across reloads when
cookie.prefix is set. Additionally, padBootstrap.js loads l10n before
window.clientVars.cookiePrefix is set, preventing l10n from using the configured prefix at
initialization time.
Code

src/static/js/pad.ts[R147-148]

+      const prefix = (window as any).clientVars?.cookiePrefix || '';
+      Cookies.set(`${prefix}language`, val);
Evidence
pad.ts now writes the language cookie using the configured prefix, but l10n.ts still matches
only language=... in document.cookie, so it will miss ep_language (or any other prefixed
name). On pad pages, padBootstrap.js requires the l10n module before it assigns
window.clientVars.cookiePrefix, so l10n cannot reliably consult clientVars during its
initialization.

src/static/js/pad.ts[141-149]
src/static/js/l10n.ts[4-11]
src/templates/padBootstrap.js[2-12]

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

## Issue description
Language is now stored in a prefixed cookie (e.g., `ep_language`), but the l10n bootstrap reads only the unprefixed `language` cookie, so language preference will not persist across reloads.
### Issue Context
- `padBootstrap.js` currently `require()`s the l10n module before setting `window.clientVars.cookiePrefix`, so l10n cannot depend on `clientVars` unless the bootstrap order is changed.
### Fix Focus Areas
- src/templates/padBootstrap.js[4-12]
- src/static/js/l10n.ts[4-11]
- src/static/js/pad.ts[142-149]
### Expected fix
1. Ensure `window.clientVars.cookiePrefix` is set before loading l10n on pad pages (swap the order in `padBootstrap.js`).
2. Update `src/static/js/l10n.ts` to read `${cookiePrefix}language` with fallback to `language` (for migration/back-compat).

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


7. Token transfer ignores prefix🐞 Bug ≡ Correctness
Description
welcome.ts posts token and prefsHttp from unprefixed cookies, but the token transfer endpoint
now sets only prefixed cookies (${prefix}token, ${prefix}prefsHttp), so token transfer breaks
when only prefixed cookies exist. This will fail on fresh installs (default ep_) or once
unprefixed cookies are cleared.
Code

src/node/hooks/express/tokenTransfer.ts[R42-44]

+    const p = settings.cookie.prefix;
+    res.cookie(`${p}token`, tokenData.token, {path: '/', maxAge: 1000*60*60*24*365});
+    res.cookie(`${p}prefsHttp`, tokenData.prefsHttp, {path: '/', maxAge: 1000*60*60*24*365});
Evidence
welcome.ts reads only token and prefsHttp from document.cookie and sends them to
/tokenTransfer. Server-side /tokenTransfer/:token now writes back only prefixed cookie names,
and pad_cookie.ts also uses the prefixed cookie name for prefs, so the welcome flow will not find
the values once the system is operating purely with prefixed cookies. indexBootstrap.js does not
provide cookiePrefix to the index/welcome scripts, so they have no way to know the prefix.

src/static/js/welcome.ts[22-31]
src/node/hooks/express/tokenTransfer.ts[15-46]
src/static/js/pad_cookie.ts[22-26]
src/templates/indexBootstrap.js[2-7]

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

## Issue description
The welcome/tokenTransfer client code reads unprefixed cookies (`token`, `prefsHttp`) but the server and other client code now use prefixed cookie names, causing token transfer to fail when only prefixed cookies exist.
### Issue Context
- Index page bootstrap (`indexBootstrap.js`) does not currently expose `cookiePrefix`, unlike pad/timeslider bootstraps.
### Fix Focus Areas
- src/static/js/welcome.ts[22-31]
- src/templates/indexBootstrap.js[1-7]
- src/node/hooks/express/tokenTransfer.ts[15-46]
### Expected fix
1. Expose `cookiePrefix` to the welcome/index page scripts (similar to `padBootstrap.js` / `timeSliderBootstrap.js`).
2. Update `welcome.ts` to read `${cookiePrefix}token` / `${cookiePrefix}prefsHttp` with fallback to unprefixed names for migration.
3. (Optional) Consider accepting both prefixed and unprefixed values server-side if the body is missing fields, to ease migration.

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


8. Timeslider drops sessionID🐞 Bug ≡ Correctness
Description
timeslider.ts now uses cookiePrefix for the token cookie but still sends `sessionID:
Cookies.get('sessionID')` unprefixed in its socket messages, so HTTP API sessions stop working when
cookie.prefix is non-empty. This can deny access to private/group pads when
settings.requireSession is enabled because the server never receives the session cookie value.
Code

src/static/js/timeslider.ts[R51-56]

+    const cp = (window as any).clientVars?.cookiePrefix || '';
+    token = Cookies.get(`${cp}token`) || Cookies.get('token');
if (token == null) {
token = `t.${randomString()}`;
-      Cookies.set('token', token, {expires: 60});
+      Cookies.set(`${cp}token`, token, {expires: 60});
}
Evidence
The timeslider client still emits an unprefixed sessionID from cookies in its socket message
payload. The server’s SecurityManager.checkAccess() uses the sessionCookie to find the author
and will deny access when settings.requireSession is true and no session author is found, so
omitting the prefixed cookie breaks authenticated/private access flows.

src/static/js/timeslider.ts[49-56]
src/static/js/timeslider.ts[97-107]
src/node/db/SecurityManager.ts[112-116]

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

## Issue description
Timeslider socket messages still send `sessionID` from the unprefixed cookie name, which breaks HTTP API session authorization when `cookie.prefix` is non-empty.
### Issue Context
- `SecurityManager.checkAccess()` depends on the session cookie to authorize when `settings.requireSession` is enabled (and for certain group/private pad flows).
### Fix Focus Areas
- src/static/js/timeslider.ts[49-56]
- src/static/js/timeslider.ts[97-107]
### Expected fix
- Compute `cp = window.clientVars?.cookiePrefix || ''` in a scope accessible to `sendSocketMsg`.
- Send `sessionID: Cookies.get(`${cp}sessionID`) || Cookies.get('sessionID')` (matching the pad page behavior).

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



Remediation recommended

9. Vite prefix hardcoded empty🐞 Bug ≡ Correctness
Description
src/templates/padViteBootstrap.js hardcodes cookiePrefix to "" and overwrites window.clientVars, so
Vite-based pad loads (ui/pad.html) cannot honor cookie.prefix and will continue to read/write
unprefixed token/sessionID cookies during CLIENT_READY.
Code

src/templates/padViteBootstrap.js[R5-9]

// This is needed to fetch /pluginfw/plugin-definitions.json, which happens before the server
// sends the CLIENT_VARS message.
randomVersionString: "7a7bdbad",
+  cookiePrefix: "",
};
Evidence
The Vite pad entrypoint (ui/pad.html) loads padViteBootstrap.js, which sets cookiePrefix to an empty
string. pad.ts reads window.clientVars.cookiePrefix during sendClientReady to decide the cookie
names for token/sessionID, so this path will keep using unprefixed cookies regardless of the
server-side setting (defeating the feature in that workflow).

src/templates/padViteBootstrap.js[4-9]
ui/pad.html[681-685]
src/static/js/pad.ts[187-192]
src/static/js/pad.ts[204-210]

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

## Issue description
The Vite bootstrap sets `window.clientVars.cookiePrefix` to `""` unconditionally, which prevents the Vite pad flow from using the configured cookie prefix.
### Issue Context
- `ui/pad.html` imports `padViteBootstrap.js` directly.
- `pad.ts` reads `window.clientVars.cookiePrefix` before sending `CLIENT_READY` to decide which cookie names to use.
### Fix Focus Areas
- src/templates/padViteBootstrap.js[4-9]
- ui/pad.html[681-685]
### Suggested fix
- In `padViteBootstrap.js`, do not overwrite an existing `window.clientVars.cookiePrefix`. For example:
- Initialize with merge/optional assignment:
- `window.clientVars = window.clientVars || {};`
- `window.clientVars.randomVersionString ||= '...';`
- `window.clientVars.cookiePrefix ||= '';`
- Optionally (to make this actually configurable in the Vite UI), set `window.clientVars.cookiePrefix` in `ui/pad.html` before the module import (e.g., from a build-time env substitution or a small inline script), so developers can test `cookie.prefix` with the Vite UI.

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


10. Prefs cookie loses migration🐞 Bug ☼ Reliability
Description
pad_cookie.ts now unconditionally switches to prefixed prefs/prefsHttp cookie names, so existing
user prefs cookies will be ignored when enabling cookie.prefix. This will reset user preferences
(theme/view options/etc.) after prefix is turned on.
Code

src/static/js/pad_cookie.ts[R24-25]

+    const prefix = (window as any).clientVars?.cookiePrefix || '';
+    this.cookieName_ = prefix + (window.location.protocol === 'https:' ? 'prefs' : 'prefsHttp');
Evidence
pad_cookie reads and writes preferences only using this.cookieName_, which is now prefixed, and
there is no fallback read for the old cookie name. This differs from the token/sessionID behavior,
which explicitly falls back to unprefixed names for migration.

src/static/js/pad_cookie.ts[22-57]
src/static/js/pad.ts[187-212]

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

## Issue description
When `cookie.prefix` is enabled, existing `prefs`/`prefsHttp` cookies are no longer read because pad_cookie now uses only the prefixed cookie name.
### Issue Context
Other cookies (token/sessionID) implement a prefixed-or-unprefixed fallback to support migration when enabling a prefix.
### Fix Focus Areas
- src/static/js/pad_cookie.ts[22-57]
### Suggested fix
In `pad_cookie` initialization:
- If `cookiePrefix` is non-empty and the prefixed prefs cookie is missing, try reading the legacy unprefixed cookie.
- If legacy prefs exist, write them to the prefixed cookie (optionally deleting the old cookie), then proceed using the prefixed name.

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


11. No tests for cookie.prefix 📘 Rule violation ☼ Reliability
Description
This change modifies authentication- and session-related cookie handling (names, reads, and writes)
but does not include an automated regression test to ensure old (unprefixed) cookies continue to
work and that new (prefixed) cookies are used consistently. Without a regression test, future
refactors could reintroduce cookie-name conflicts or break migrations undetected.
Code

src/node/utils/Settings.ts[R532-535]

cookie: {
keyRotationInterval: 1 * 24 * 60 * 60 * 1000,
+    prefix: 'ep_',
sameSite: 'lax',
Evidence
PR Compliance ID 2 requires a regression test for bug fixes/behavioral fixes. The PR introduces new
cookie-name behavior via settings.cookie.prefix and uses it in critical cookie paths, but the PR
diff does not show any added/updated tests covering this behavior.

src/node/utils/Settings.ts[532-535]
src/node/hooks/express/importexport.ts[79-84]
Best Practice: Repository guidelines

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

## Issue description
There is no automated test coverage ensuring prefixed cookies are set/used and that fallback to unprefixed cookie names continues to work.
## Issue Context
The PR changes cookie naming for `token`, `sessionID`, and `express_sid`-related flows. This is easy to regress and impacts authentication/access control.
## Fix Focus Areas
- src/node/hooks/express/importexport.ts[79-84]
- src/node/padaccess.ts[8-13]
- src/node/hooks/express.ts[203-235]
- src/static/js/pad.ts[187-210]

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


View more (1)
12. doc/cookies.md missing cookie prefix📘 Rule violation ⚙ Maintainability
Description
The cookie documentation still lists unprefixed cookie names (for example, express_sid, token,
language, sessionID) and does not mention cookie.prefix or how it affects cookie names. This
is a documentation gap for a configuration-facing behavior change.
Code

src/node/utils/Settings.ts[R532-535]

cookie: {
keyRotationInterval: 1 * 24 * 60 * 60 * 1000,
+    prefix: 'ep_',
sameSite: 'lax',
Evidence
PR Compliance ID 3 requires config changes to be documented. The PR introduces
settings.cookie.prefix (defaulting to ep_), but doc/cookies.md does not document the prefix
setting and continues to describe the old, unprefixed cookie names.

src/node/utils/Settings.ts[532-535]
doc/cookies.md[5-18]
Best Practice: Repository guidelines

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

## Issue description
`doc/cookies.md` does not mention `cookie.prefix` and lists cookie names without considering the prefix behavior.
## Issue Context
The PR adds a new configuration setting that changes externally visible cookie names; docs should explain the setting, defaults, and how names change.
## Fix Focus Areas
- doc/cookies.md[1-18]
- settings.json.template[385-395]
- src/node/utils/Settings.ts[532-538]

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



Advisory comments

13. cookie.prefix not validated🐞 Bug ⚙ Maintainability
Description
cookie.prefix is concatenated into cookie names across server and client without validation, so an
invalid prefix can generate cookie names that break cookie parsing/setting and lead to hard-to-debug
auth/session issues. This is especially risky because the feature is explicitly user-configurable.
Code

src/node/utils/Settings.ts[R532-535]

cookie: {
keyRotationInterval: 1 * 24 * 60 * 60 * 1000,
+    prefix: 'ep_',
sameSite: 'lax',
Evidence
Settings introduces a configurable cookie.prefix with a default value, and multiple call sites
concatenate it directly into cookie names. The settings reload path contains various validations for
other settings but does not validate settings.cookie.prefix, so invalid values will propagate into
cookie names.

src/node/utils/Settings.ts[529-538]
src/node/utils/Settings.ts[971-980]
src/node/hooks/express.ts[210-214]
src/node/hooks/express/tokenTransfer.ts[42-44]

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

## Issue description
An invalid `cookie.prefix` value can silently break cookie handling because it is concatenated directly into cookie names.
### Issue Context
- `reloadSettings()` already validates certain settings (e.g., `skinName`, `socketTransportProtocols`) but does not validate cookie-related naming.
### Fix Focus Areas
- src/node/utils/Settings.ts[529-538]
- src/node/utils/Settings.ts[956-1104]
### Expected fix
- Add validation for `settings.cookie.prefix` during settings reload (e.g., restrict to a safe subset such as `[A-Za-z0-9_]*` or RFC cookie-name token characters).
- If invalid, log an error/warn and fall back to a safe default (or reject startup with a clear error).

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


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Changing the default to "ep_" would invalidate all existing sessions
on upgrade since express-session only looks for the configured cookie
name. Default to "" (no prefix) so upgrades are non-breaking — users
opt-in to prefixed names by setting cookie.prefix in settings.json.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comment thread src/node/utils/Settings.ts
Comment thread src/node/hooks/express.ts
Comment thread src/static/js/pad.ts
Comment thread src/node/hooks/express/tokenTransfer.ts
Comment thread src/static/js/timeslider.ts Outdated
@JohnMcLear

Copy link
Copy Markdown
Member Author

/review

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Apr 4, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 146c2e1

- l10n.ts: Read prefixed language cookie with fallback to unprefixed
- welcome.ts: Use cookiePrefix for token transfer reads
- timeslider.ts: Use prefix for sessionID in socket messages
- pad_cookie.ts: Fall back to unprefixed prefs cookie for migration
- indexBootstrap.js: Pass cookiePrefix via clientVars to welcome page
- specialpages.ts: Pass settings to indexBootstrap template

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@JohnMcLear

Copy link
Copy Markdown
Member Author

/review

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Apr 4, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit fb66edc

Comment thread src/static/js/l10n.ts Outdated
…code

- l10n.ts: Escape special regex characters in cookiePrefix before using
  it in RegExp constructor to prevent runtime errors
- padViteBootstrap.js: Add comment noting the hardcoded prefix is
  dev-only and must match settings.json

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@JohnMcLear

Copy link
Copy Markdown
Member Author

/review

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Apr 4, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit ef25e75

Comment thread src/node/padaccess.ts
Comment on lines +8 to +13
const p = settings.cookie.prefix;
const accessObj = await securityManager.checkAccess(
req.params.pad, req.cookies.sessionID, req.cookies.token, user);
req.params.pad,
req.cookies[`${p}sessionID`] || req.cookies.sessionID,
req.cookies[`${p}token`] || req.cookies.token,
user);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Foreign token blocks access 🐞 Bug ≡ Correctness

In pad access checks, when cookie.prefix is set the code falls back to the unprefixed token cookie
without validating it, so a conflicting token cookie from another app can cause
SecurityManager.checkAccess() to deny with “invalid author token”. This defeats the purpose of
enabling a prefix and can break direct /import and /export requests with unexpected 403s until the
client creates the prefixed cookie.
Agent Prompt
## Issue description
When `cookie.prefix` is configured, server-side routes that call `SecurityManager.checkAccess()` fall back to the unprefixed `token` cookie even if it belongs to another app. If that cookie is present but not a valid Etherpad author token, `SecurityManager.checkAccess()` returns DENY, causing 403s on routes like import/export.

## Issue Context
The whole point of `cookie.prefix` is to avoid cookie collisions on shared domains. The current fallback logic can still be triggered by other frameworks’ generic `token` cookies, producing denials instead of ignoring the foreign value.

## Fix Focus Areas
- Add a small helper to compute the token passed to `checkAccess`:
  - Prefer the prefixed cookie.
  - Only fall back to the unprefixed cookie **if it passes** `padutils.isValidAuthorToken()` (or otherwise treat it as absent).
- Apply the helper consistently anywhere server-side reads `req.cookies.token` for access decisions.

### Suggested focus locations
- src/node/padaccess.ts[6-13]
- src/node/hooks/express/importexport.ts[76-86]
- src/node/db/SecurityManager.ts[112-126] (reference behavior; no change necessarily required here)

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't fix — with default prefix "", the fallback reads the same cookie name. When a user sets a custom prefix, the fallback to unprefixed name ensures existing tokens continue to work during migration. A foreign/invalid token simply fails auth as before.

Reject cookie.prefix values containing characters outside
[a-zA-Z0-9_-] to prevent HTTP header injection via crafted cookie
names (e.g., \r\n sequences). Falls back to empty prefix with an
error log.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@JohnMcLear

Copy link
Copy Markdown
Member Author

/review

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Apr 4, 2026

Copy link
Copy Markdown

Persistent review updated to latest commit 22a0c6d

Comment thread src/node/padaccess.ts
Comment on lines +8 to +12
const p = settings.cookie.prefix;
const accessObj = await securityManager.checkAccess(
req.params.pad, req.cookies.sessionID, req.cookies.token, user);
req.params.pad,
req.cookies[`${p}sessionID`] || req.cookies.sessionID,
req.cookies[`${p}token`] || req.cookies.token,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. sessionid still default cookie 📎 Requirement gap ⛨ Security

Because settings.cookie.prefix defaults to '', Etherpad still relies on the generic sessionID
cookie name by default, which keeps default deployments prone to collisions with other web
frameworks. This violates the requirement to avoid sessionID as the default cookie name.
Agent Prompt
## Issue description
Default deployments still use the generic `sessionID` cookie name because `cookie.prefix` defaults to `''`, which does not satisfy the requirement to avoid `sessionID` by default.

## Issue Context
The code already supports migration by falling back to unprefixed cookie names when reading (`prefixed || unprefixed`). This makes it feasible to change the default to an Etherpad-specific namespace while keeping compatibility for existing deployments/portals.

## Fix Focus Areas
- src/node/utils/Settings.ts[530-538]
- src/node/padaccess.ts[8-13]

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — cookie prefix is now validated to only allow [a-zA-Z0-9_-] characters, preventing header injection.

@JohnMcLear
JohnMcLear merged commit 474918a into ether:develop Apr 5, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cookies' names should be customizable

1 participant