* docker: prepare plugin volume mountpoint
Create src/plugin_packages in the development and production runtime image stages so a fresh Docker named volume is initialized with Etherpad user ownership instead of root-only ownership.
Fixes#8026
* tests: align docker plugin volume regression test with project style
Replace CommonJS imports with Node.js module imports and reformat the
test to match the project's coding style. This change is purely
stylistic and does not alter the regression test's behavior.
* tests: make docker plugin volume test less brittle
Match Dockerfile instructions with regexes instead of exact substrings so
the regression test validates the required behavior without rejecting
harmless formatting or equivalent command changes.
* docker: explain the plugin_packages mountpoint, drop the Dockerfile-text test
The reason the two `mkdir`s exist is not obvious from the Dockerfile, so state it
where the next reader will be: a named volume inherits the mountpoint's ownership
from the image, and USER is already etherpad at that point.
Removes dockerfilePluginVolume.ts. It asserted that the Dockerfile *text* contains
`mkdir -p ./src/plugin_packages` after the `COPY ./src` line — it fails when the
Dockerfile is refactored (a `.gitkeep`, a `VOLUME`, a different order all keep the
behaviour intact) and it cannot fail when the actual bug returns, since it never
inspects a built image. The build-test job already builds both runtime stages.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: SamTV12345 <40429738+samtv12345@users.noreply.github.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
#7907 made the production docker-compose require ADMIN_PASSWORD and the DB
password (no insecure fallback) and defaulted TRUST_PROXY to false, but only
changed docker-compose.yml. This brings the docs in line:
- .env.default: document DOCKER_COMPOSE_APP_TRUST_PROXY (set true behind a
trusted reverse proxy) and note ADMIN_PASSWORD is required (compose won't
start while it's empty).
- .env.dev.default: document the dev DOCKER_COMPOSE_APP_DEV_ENV_TRUST_PROXY.
- README.md / doc/docker.md: update the embedded compose snippets to match the
merged file (required ADMIN_PASSWORD/DB password, TRUST_PROXY default false).
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* perf: don't log settings.loadTest warning per-message (#7756)
CPU profile of develop (and of the open #7775 branch) at the
100-400 author dive sweep attributed ~4% of total process CPU to
log4js inside SecurityManager.checkAccess. Tracing the actual log
call: line 79-80 emits `console.warn('bypassing socket.io
authentication...')` on every checkAccess invocation when
settings.loadTest is true — once per inbound message. With log4js's
replaceConsole + cluster-mode dispatch enabled, that warning
allocated, formatted, and dispatched a LogEvent through
sendToListeners -> sendLogEventToAppender for every CLIENT_READY,
COMMIT_CHANGESET, USERINFO_UPDATE, etc.
settings.loadTest is a configuration choice, not a per-request
condition. The warning belongs at startup. Move it to Settings.ts
init alongside the other "you set X, beware" warnings, and drop
the per-message branch (the loadTest short-circuit still applies).
Test plan:
- tests/backend/specs/api/sessionsAndGroups.ts: 32 passing
- tests/backend/specs/socketio.ts: 39 passing (handleMessage paths)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fixup: address Qodo review on #7776
Three issues flagged:
1. Indentation: outdented the continuation lines inside the new
`if (settings.loadTest)` block from 10 spaces to 8 (one level
from `logger.warn(`), matching 2-space indent rule for added
code.
2. Warning scope: the original wording said only socket.io
authn/authz is bypassed, but settings.loadTest short-circuits
SecurityManager.checkAccess() which is called from both HTTP
(padaccess, importexport) and socket.io (PadMessageHandler)
paths. Reword to "SecurityManager.checkAccess() will bypass
authentication and authorization for both HTTP and socket.io
requests".
3. Misleading "fires once at startup" comment in
SecurityManager.ts: the warning is logged from Settings.ts
reloadSettings(), which is also called on admin restart and
plugin install. Rephrase to "logged from Settings.ts during
settings load/reload, not on every request".
All three issues are accurate. No behaviour change for the fix
itself; only comment + warning text.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CPU profile of the SUT at the 100-400 author dive sweep
(load-test workflow run 25956384097) attributed about 6% of total
process CPU to the throw + catch around getSessionInfo:
- ~1.82% to `new CustomError('sessionID does not exist', 'apierror')`
construction (stack trace capture)
- ~4.12% downstream, via the catch block's `console.debug(...)`
routed through log4js -> sendToListeners -> sendLogEventToAppender
Both call sites (`findAuthorID` on every CLIENT_READY, and
`listSessionsWithDBKey` on session listing) immediately caught
`apierror` and discarded it. The public `exports.getSessionInfo`
contract still has to throw for the HTTP API (returning code:1 for
missing sessionID), so introduce a private `getSessionInfoOrNull`
helper that returns null and have the hot-path callers use it
directly. `exports.getSessionInfo` is kept as a thin wrapper that
preserves the existing throw semantics.
No behaviour change for the HTTP API — sessionsAndGroups.ts test
file (32 cases, including "getSessionInfo of deleted session"
expecting code:1) passes unchanged.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The page up/down handler advances the caret by numberOfLinesInViewport
computed from scroll.getVisibleLineRange(). That helper returns indices
into rep.lines (logical lines, not visual/wrapped rows), so when one
wrapped logical line fills the viewport — e.g., three consecutive lines
of ~2000 chars each — the range collapses to [n, n] and the advance
count becomes 0. The caret stays on line n, scroll stays at 0, and the
user sees "PageDown does nothing".
Clamp the advance to at least one logical line so the caret and viewport
always move.
Includes a Playwright regression test covering the reporter's repro
(three very long lines, Ctrl+Home, PageDown).
Closes#4562
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Migrate server to TypeScript 7 (tsgo)
Bump typescript to ^7.0.0 in ep_etherpad-lite and bin. Fix the two
diagnostics the new compiler reports: an unguarded spread of
splitTextLines' nullable result in TextLinesMutator (silenced with
@ts-ignore, matching the sibling push calls), and per-element overload
errors on the hooks.ts test-case concat that a single @ts-ignore no
longer covers (replaced with an explicit any[] cast).
admin and ui intentionally stay on TypeScript 6: their build depends on
openapi-typescript, which uses the JS compiler API that tsgo does not
expose until 7.1.
pnpm-workspace.yaml gains minimum-release-age exclusions for the
freshly released typescript 7 packages.
* Address review: null-safe splitTextLines spread, typed hook test cases
Replace the @ts-ignore-only approach with a real null guard
(splitTextLines(text) ?? []) in TextLinesMutator.insert, and replace
the any[] cast in hooks.ts with an explicit HookFnTestCase type so the
concatenated test cases stay shape-checked.
* Type _curSplice as [number, number, ...string[]], drop 21 ts-ignores
The tuple was declared [number, number?] but it actually carries the
lines to insert after the two splice numbers (the JSDoc already said
so). Typing it correctly removes every curSplice-related ts-ignore in
the file, including the one added earlier in this branch. The two
remaining ignores are about StringArrayLike lines, unrelated to this
migration.
No behavior change: types and casts only. tsc --noEmit clean,
easysync mutation tests pass.
---------
Co-authored-by: SamTV12345 <40429738+samtv12345@users.noreply.github.com>
* security(oidc): stop shipping a hardcoded OIDC cookie key and reflecting all CORS origins
The embedded OIDC provider signed its interaction/session/grant cookies
with the committed literal key `['oidc']`, so anyone with the public
source could forge valid `.sig` cookies (defeating the provider's
cookie-integrity boundary), and `clientBasedCORS` returned `true` for
every origin, reflecting arbitrary `Origin` values into
`Access-Control-Allow-Origin` on the token/userinfo endpoints.
Adds OidcProviderSecurity.ts with two unit-tested pure helpers:
- resolveOidcCookieKeys(): prefers an operator-supplied
`settings.sso.cookieKeys` (ordered array for rotation), otherwise
derives a secret key from the persisted session secret via a
domain-separated SHA-256 — stable across restarts and multi-pod, never
committed to source; falls back to an ephemeral random key.
- isOriginAllowedForOidcClient(): allows a CORS origin only when it
matches the origin of one of the client's registered redirect URIs.
Verified end-to-end against a real oidc-provider@9.9.1 instance
(cookies.keys accepted, Client.redirectUris read correctly, attacker
origin blocked). Reported privately by meifukun.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* security(oidc): use DB-backed SecretRotator for cookie keys under default rotation config
Addresses Qodo review on #8070. With Etherpad's default cookie settings
(keyRotationInterval + sessionLifetime set), key rotation is enabled and
`settings.sessionKey` stays null unless SESSIONKEY.txt is provisioned, so
the previous fallback handed the OIDC provider a per-process random key —
breaking in-flight OIDC cookies on restart and across horizontally-scaled
pods on a default install.
resolveOidcCookieKeys() now takes an optional rotatedSecrets array
(priority: operator cookieKeys > rotated secrets > session-key
derivation > random) and returns it by reference so a live rotation
propagates to keygrip. OAuth2Provider.expressCreateServer() creates a
dedicated `oidcCookieSecrets` SecretRotator — the same DB-backed
mechanism the Express session cookies use — when the operator hasn't
pinned settings.sso.cookieKeys and rotation is enabled.
Adds unit tests for the rotatedSecrets priority/by-reference behavior and
an integration test proving the default config yields stable DB-backed
secrets rather than a random key.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(settings): give sso.cookieKeys a real key in the template
The doc block sat above `sso.clients` with no `cookieKeys` property, so the admin
UI's template-comment extractor (which attaches leading comments to the next
property node) would have shown it as documentation for `clients`. An unset
OIDC_COOKIE_KEY yields [""], which resolveOidcCookieKeys() filters out, so the
derived/rotated key path is unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: SamTV12345 <40429738+samtv12345@users.noreply.github.com>
* security(padid): reject ueberdb delimiter ':' in pad ids (copyPad/movePad injection)
GHSA-wg58-mhwv-35pq. copyPad / movePad / copyPadWithoutHistory take their
destination via the `destinationID` API field, which — unlike padID /
padName — is never run through sanitizePadId. Because isValidPadId only
forbade `$`, a destinationID like `victim:revs:0` survived into the
engine: the embedded `:` (the ueberdb key-namespace delimiter) let it
address another pad's internal `pad:<id>:revs:<n>` records. That both
bypassed the force=false "destination already exists" guard (which checks
only the top-level `atext`) and clobbered the victim pad's revision
history.
- isValidPadId now rejects `:` (never legal in a pad id; it's the DB
key delimiter). The name portion excludes `$` and `:`.
- Pad.copy() and Pad.copyPadWithoutHistory() validate destinationID via
isValidPadId before any db write; movePad routes through copy(), so the
check runs before the source is removed.
Adds isValidPadId unit cases and an integration test proving copyPad /
copyPadWithoutHistory reject a `:`-bearing destinationID with force=false
and leave the victim's rev-0 changeset untouched, while a normal copy
still works. Reported privately (finder credited on the advisory).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* security(padid): sanitize pad URL before validating so legacy ":" URLs still redirect
Addresses Qodo review on #8073. Rejecting ":" in isValidPadId made
padurlsanitize's validate-first ordering return 404 for a browser
visiting a legacy `/p/<id with ":">` URL, instead of redirecting it to
the sanitized `_` form.
Reorder padurlsanitize to sanitize FIRST (sanitizePadId maps whitespace
and ":" to "_"), then validate the sanitized id. `/p/foo:bar` now
redirects to `/p/foo_bar` as before; an id that stays invalid after
sanitizing (e.g. containing "$") is still forbidden. This restores the
pre-fix redirect behavior while keeping ":" out of stored pad ids.
Adds a regression test for the ":" redirect, the "$" 404, and a clean id.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* security(padid): keep existing pads with ":" reachable
Rejecting ":" in isValidPadId also gates padManager.getPad(), so pads whose id
contains a ":" — legal before GHSA-wg58-mhwv-35pq, which is why padIdTransforms
maps ":" at all — became unopenable, not just uncreatable: sanitizePadId returns
such an id unchanged once the pad exists, and getPad then threw.
Refuse an invalid id only when no pad with that exact id exists (in getPad and in
the pad-URL param). The injection primitive stays closed: doesPadExist() requires
a top-level `atext`, which the `pad:<id>:revs:<n>` sub-records an injected
destinationID addresses do not have. Copy destinations keep the strict check, so
no new ":" id can be created.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: SamTV12345 <40429738+samtv12345@users.noreply.github.com>
* security(export): strip remote images on the soffice path to match the native path
Defense-in-depth / consistency fix — not a core vulnerability on its own.
Core Etherpad never emits <img> tags in export HTML; they only appear
when a plugin/hook injects them, so sanitising such content is primarily
the injecting plugin's responsibility.
However, the native in-process export path (issue #7538) already calls
stripRemoteImages() defensively, while the LibreOffice (soffice) path
wrote the HTML to the temp file verbatim. soffice is the only export
path that actually performs outbound fetches for remote <img> URLs during
conversion, so a plugin-injected remote image there becomes a blind SSRF
sink. This makes the soffice path consistent with the native path core
already ships.
Adds a regression test that injects a remote image via
exportHTMLAdditionalContent, intercepts the temp file via the
exportConvert hook, and asserts no remote image URL survives.
Remote-image behaviour reported privately by meifukun.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* export: preserve doctype and comments in stripRemoteImages
Addresses Qodo review on #8071. Applying stripRemoteImages() to the full
export document for the soffice path dropped `<!doctype html>` and HTML
comments, because the htmlparser2 handler only emitted open/text/close
tags. A missing doctype can flip LibreOffice's HTML import into quirks
mode, subtly changing rendering for all soffice exports.
Add onprocessinginstruction (doctype) and oncomment handlers so the
serializer round-trips document directives and comments while still
stripping remote images. The native path is unaffected (it strips only
extractBody() output, which has no doctype). Adds regression tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* security(proxy-path): set Vary on public routes that echo x-proxy-path (cache poisoning)
The home page, pad page and timeslider (and the legacy timeslider
redirect) echo the sanitised x-proxy-path prefix into rendered URLs,
social-preview metadata, manifest links and the redirect target, but did
not advertise Vary. A shared cache/CDN keyed on URL alone could store a
prefix injected by one client and serve it to others.
The admin routes already emit Vary: x-proxy-path (GHSA-fjgc-3mj7-8rg8);
this extends the same protection to the public routes the earlier fix
left uncovered. The value is still passed through sanitizeProxyPath
(quotes/angle-brackets/protocol-relative/.. already blocked), so this is
cache-correctness hardening, not XSS/open-redirect.
Adds a varyOnProxyPath() helper applied at every sanitizeProxyPath call
site, plus a regression test asserting Vary: x-proxy-path on /, /p/:pad,
the timeslider embed page and the legacy redirect. Reported by meifukun.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* security(proxy-path): only Vary on trust-gated headers when trustProxy is enabled
Addresses Qodo review on #8072. varyOnProxyPath() unconditionally varied
on x-forwarded-prefix and x-ingress-path, but sanitizeProxyPath() ignores
those two headers unless settings.trustProxy is true. Advertising them in
Vary when trustProxy is false does not reflect a real dependency and only
fragments shared caches on attacker-supplied header values.
Always vary on x-proxy-path (always honored); add the two standard
headers only when trustProxy is enabled. Test asserts the exclusion when
trustProxy=false and inclusion when true.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* security(auth): regenerate session id on authentication (session fixation)
GHSA-73h9-c5xp-gfg4. Etherpad never rotated the express-session id when
an anonymous session was upgraded to an authenticated one. Any auth
scheme that establishes a pre-authentication session — every OIDC/OAuth
RP, including the official ep_openid_connect plugin, which persists OAuth
state before redirecting to the IdP — was exposed to CWE-384: an attacker
who planted or captured the pre-auth cookie ends up owning the victim's
authenticated (possibly admin) session after they log in.
webaccess now calls req.session.regenerate() at the authentication
boundary (after the authenticate hook / HTTP-Basic path establishes
req.session.user), preserving the session data onto the new id. It fires
only on a *fresh* login (already-authenticated requests short-circuit in
Step 2) and no-ops when the store doesn't expose regenerate(). This lives
in core, so it protects every SSO/auth plugin.
Adds a regression test proving the session id changes across the auth
boundary and that the rotated session stays authenticated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* security(auth): rotate session on identity/privilege change, not just anon->auth
Addresses Qodo review on #8074. The first cut only regenerated when the
request started with no session.user, so a privilege upgrade reaching the
authenticate step for an already-authenticated session (e.g. non-admin ->
admin re-authentication) would NOT rotate the id, leaving a fixation
window on the privilege change.
Rotate whenever authentication changes the principal — anonymous -> user,
or a username / is_admin change — while still leaving a no-op re-auth of
the same principal alone (no per-request churn). Adds a deterministic
test for the non-admin -> admin rotation. Also derives the session cookie
name from settings.cookie.prefix in the test instead of hardcoding
'express_sid'.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* security(collab): apply queued USER_CHANGES to the enqueue-time pad (cross-pad write TOCTOU)
GHSA-6mcx-x5h6-rpw2. handleUserChanges re-read the mutable
sessioninfos[socket.id].padId at apply time, while the authorization /
read-only gate and the channel key were taken at enqueue time. Because
per-socket messages are processed concurrently and a thrown handler does
not disconnect the socket, a same-socket CLIENT_READY could swap the
session's padId between enqueue and apply — redirecting a queued write
onto a read-only or otherwise unauthorized pad (runtime-proven cross-pad
write; a read-only share holder could overwrite the pad).
Thread the enqueue-time pad id (already the padChannels channel key) into
handleUserChanges as `authorizedPadId` and use it for the write instead
of re-reading the session. The gate and the write now refer to the same
pad, closing the TOCTOU window. Normal (non-racing) flows are unaffected
because the session padId equals the enqueue-time padId.
Adds a deterministic regression test that invokes handleUserChanges with
the session padId already swapped to a victim pad and asserts the change
lands on the authorized (argument) pad, never the victim. Verified it
fails when the vulnerable read is reintroduced.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* security(collab): pin pad snapshot before awaits to fully close cross-pad write TOCTOU
Addresses Qodo review on #8075: the first cut used the padChannels channel
key as the write target, but that key was still read from thisSession.padId
at enqueue time — AFTER the awaits in handleMessage() (checkAccess and the
handleMessageSecurity/handleMessage hooks). A concurrent same-socket
CLIENT_READY can mutate sessioninfos[socket.id] in place during those awaits
(the object-identity disconnect check does not catch an in-place mutation),
so the queue key — and thus the write — could still be redirected onto a
read-only / unauthorized pad after the read-only gate passed.
Pin the pad id and read-only flag into a consistent snapshot (messagePadId /
messageReadonly) together with `auth`, BEFORE any of those awaits, and use
the snapshot for the read-only gate, the hook context, the queue key and the
write. The gate, the queue key and the write now all refer to the same pad
regardless of concurrent CLIENT_READY swaps.
handleUserChanges keeps using its authorizedPadId argument (defence for the
enqueue->apply window). The test now drives a real USER_CHANGES over a
socket and swaps the session padId mid-message via a handleMessageSecurity
hook (the concurrent-CLIENT_READY race), asserting the write lands on the
authorized pad and never the victim — verified RED when the pinned key is
reverted to thisSession.padId. This also drops the handleUserChanges export
and the direct-call test (Qodo maintainability / pendingEdits / cleanup
findings).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Thanks to every contributor: PRs, issues, reviews, translations, plugins, answered questions. This project runs on volunteer effort, not one person or one company. That's by design and it's why it's lasted.
Thanks to everyone running an instance: schools, newsrooms, gov departments, or just yourself. You trusted us with your documents. We don't take that lightly.
Thanks to the wider FOSS community. We've relied on other people's open source work more than we can credit individually, and tried to give some back. If you've never contributed to a FOSS project, this is your sign. Code, docs, translations, bug reports, or just running an instance and telling people about it, all of it counts.
The migration to modern JS tooling has been one of the best things to happen to this codebase. Easier to contribute to, easier to maintain, easier to build on, which is what matters for a 15+ year old project that intends to keep going.
Going forward: keep Etherpad boring, stable, self-hostable, no enshittification, while staying open to the plugins and integrations that make it useful. We need more maintainers and more instances. If that's you, open an issue.
Here's to the next 10,000. John McLear
