Skip to content

.pr_agent_accepted_suggestions

Jump to bottom
qodo-merge-bot edited this page Jul 27, 2026 · 133 revisions
                     PR 8074 (2026-07-25)                    
[security] Reauth skips session rotation
Reauth skips session rotation `checkAccess()` only regenerates the session when `wasAlreadyAuthenticated` is false, but Step 3 explicitly supports reauthentication with different credentials, so an authenticated session can change identity/privilege without rotating the session id. This can leave a session-fixation window across a privilege upgrade (for example, non-admin session upgrading to admin via `/admin-auth/`).

Issue description

webaccess.checkAccess() currently skips req.session.regenerate() for any request that already has req.session.user before authentication runs. This fails to rotate the session id when a request reauthenticates into a different user or elevated privileges (e.g., non-admin -> admin), which is still an authentication boundary that should invalidate any attacker-known/planted session id.

Issue Context

Step 3 explicitly supports reauthentication with different credentials. /admin-auth/ is documented as a route that can establish an admin session cookie.

Fix Focus Areas

  • src/node/hooks/express/webaccess.ts[177-244]

Implementation notes

  • Capture a snapshot of the pre-auth identity/privilege before running the authenticate logic (e.g., preUsername, preIsAdmin).
  • After authentication, compare with the post-auth identity/privilege (postUsername, postIsAdmin).
  • Regenerate when:
  • there was no authenticated user before (preUsername == null), or
  • the authenticated identity changed (preUsername !== postUsername), or
  • privilege materially increased (at minimum preIsAdmin === false && postIsAdmin === true).
  • Keep the existing error handling (500 on regeneration failure) consistent with your security posture.
[security] No rotation on privilege-upgrade
No rotation on privilege-upgrade checkAccess only regenerates when the request started with no req.session.user, but Step 3 explicitly supports re-authentication with different credentials, so an already-authenticated session that upgrades identity/privileges (e.g. non-admin → admin for /admin-auth) will keep the same session id. This leaves a session fixation window across an authentication/privilege boundary that the PR intends to protect.

Issue description

checkAccess() skips session id rotation whenever req.session.user was already set at the start of Step 3. However, Step 3 is documented/implemented to allow re-authentication with different credentials, which can change the authenticated principal or upgrade privileges (for example, non-admin→admin on /admin-auth). In those cases, keeping the old session id defeats the session fixation protection across the privilege boundary.

Issue Context

  • Step 2 can fail for an already-authenticated non-admin user attempting to access an admin-only resource.
  • Step 3 then authenticates again and can replace/upgrade req.session.user.
  • Current logic only rotates when the pre-Step-3 session had no user.

Fix approach

  • Capture a minimal "pre-auth" identity snapshot before running the authenticate hook (e.g., prevUser = req.session?.user and fields like username, is_admin, readOnly).
  • After authentication, compare prevUser vs req.session.user.
  • Regenerate the session if:
  • prevUser was null/undefined (fresh login), OR
  • the authenticated principal changed (e.g., username changed), OR
  • privileges increased (e.g., prevUser.is_admin !== true and newUser.is_admin === true).
  • Consider adding a regression test for the non-admin→admin transition (or any identity change) to ensure the session id rotates on privilege upgrades.

Fix Focus Areas

  • src/node/hooks/express/webaccess.ts[175-244]
[maintainability] Test ignores cookie prefix
Test ignores cookie prefix `sessionFixation.ts` hard-codes the session cookie name as `express_sid`, but the express-session middleware sets the cookie name to `${settings.cookie.prefix}express_sid`. This makes the new regression test fail or become misleading under non-empty cookie prefixes.

Issue description

The new sessionFixation spec assumes the session cookie name is always express_sid, but production config prefixes it with settings.cookie.prefix. The test should derive the cookie name from settings to remain valid when prefixing is enabled.

Issue Context

Core express-session config uses name: ${settings.cookie.prefix}express_sid``.

Fix Focus Areas

  • src/tests/backend/specs/sessionFixation.ts[67-96]

Implementation notes

  • Add a helper similar to other tests:
  • const cookiePrefix = () => settings.cookie?.prefix || '';
  • const sidCookieName = ${cookiePrefix()}express_sid;
  • Replace all uses of 'express_sid' and Cookie: express_sid=... with sidCookieName.
[security] No rotation on privilege-upgrade
No rotation on privilege-upgrade checkAccess only regenerates when the request started with no req.session.user, but Step 3 explicitly supports re-authentication with different credentials, so an already-authenticated session that upgrades identity/privileges (e.g. non-admin → admin for /admin-auth) will keep the same session id. This leaves a session fixation window across an authentication/privilege boundary that the PR intends to protect.

Issue description

checkAccess() skips session id rotation whenever req.session.user was already set at the start of Step 3. However, Step 3 is documented/implemented to allow re-authentication with different credentials, which can change the authenticated principal or upgrade privileges (for example, non-admin→admin on /admin-auth). In those cases, keeping the old session id defeats the session fixation protection across the privilege boundary.

Issue Context

  • Step 2 can fail for an already-authenticated non-admin user attempting to access an admin-only resource.
  • Step 3 then authenticates again and can replace/upgrade req.session.user.
  • Current logic only rotates when the pre-Step-3 session had no user.

Fix approach

  • Capture a minimal "pre-auth" identity snapshot before running the authenticate hook (e.g., prevUser = req.session?.user and fields like username, is_admin, readOnly).
  • After authentication, compare prevUser vs req.session.user.
  • Regenerate the session if:
  • prevUser was null/undefined (fresh login), OR
  • the authenticated principal changed (e.g., username changed), OR
  • privileges increased (e.g., prevUser.is_admin !== true and newUser.is_admin === true).
  • Consider adding a regression test for the non-admin→admin transition (or any identity change) to ensure the session id rotates on privilege upgrades.

Fix Focus Areas

  • src/node/hooks/express/webaccess.ts[175-244]
[maintainability] Test ignores cookie prefix
Test ignores cookie prefix The new sessionFixation.ts hard-codes the session cookie name as 'express_sid', but Etherpad configures express-session to use `${settings.cookie.prefix}express_sid`. The test will fail under valid configurations that set a non-empty cookie prefix.

Issue description

The new session fixation regression test assumes the session cookie name is always express_sid. In production, the cookie name is derived from configuration (settings.cookie.prefix), so the test is not robust to non-default cookie prefix settings.

Issue Context

src/node/hooks/express.ts configures express-session cookie name as ${settings.cookie.prefix}express_sid, so tests should compute the cookie name the same way.

Fix approach

  • In sessionFixation.ts, compute const sidCookieName = ${settings.cookie.prefix || ''}express_sid;.
  • Use sidCookieName in getSetCookie() calls and when setting the Cookie request header.

Fix Focus Areas

  • src/tests/backend/specs/sessionFixation.ts[26-96]
  • src/node/hooks/express.ts[208-217]
                     PR 8073 (2026-07-25)                    
[correctness] Legacy ':' redirect regresses
Legacy ':' redirect regresses The `/p/:pad` route rejects pad IDs containing `:` before calling `sanitizePadId()`, so legacy colon→underscore canonicalization no longer runs and the route returns 404. This regression is triggered by `isValidPadId()` now forbidding `:` even though `sanitizePadId()` still has a `/:+/g -> '_'` transform intended to preserve legacy access.

Issue description

padurlsanitize validates the raw URL padId with isValidPadId() before it calls sanitizePadId(). With isValidPadId() now rejecting :, requests that previously would have been normalized (e.g., foo:bar → foo_bar) are rejected early.

Issue Context

sanitizePadId() still contains a legacy transform that maps : to _, but it is now bypassed for HTTP pad URLs.

Fix Focus Areas

  • src/node/hooks/express/padurlsanitize.ts[9-28]
  • src/node/db/PadManager.ts[169-173]
  • src/node/db/PadManager.ts[195-209]

Suggested fix

In padurlsanitize.ts, compute sanitizedPadId = await padManager.sanitizePadId(padId) first, then validate sanitizedPadId with isValidPadId(sanitizedPadId).

  • If sanitizedPadId !== padId, redirect to sanitizedPadId (as before).
  • If !isValidPadId(sanitizedPadId) (e.g., an actual stored legacy pad ID containing :), return 404. This preserves legacy :→_ redirect behavior while still preventing : from being treated as a valid canonical pad ID.
[correctness] Legacy ':' redirect regresses
Legacy ':' redirect regresses The `/p/:pad` route rejects pad IDs containing `:` before calling `sanitizePadId()`, so legacy colon→underscore canonicalization no longer runs and the route returns 404. This regression is triggered by `isValidPadId()` now forbidding `:` even though `sanitizePadId()` still has a `/:+/g -> '_'` transform intended to preserve legacy access.

Issue description

padurlsanitize validates the raw URL padId with isValidPadId() before it calls sanitizePadId(). With isValidPadId() now rejecting :, requests that previously would have been normalized (e.g., foo:bar → foo_bar) are rejected early.

Issue Context

sanitizePadId() still contains a legacy transform that maps : to _, but it is now bypassed for HTTP pad URLs.

Fix Focus Areas

  • src/node/hooks/express/padurlsanitize.ts[9-28]
  • src/node/db/PadManager.ts[169-173]
  • src/node/db/PadManager.ts[195-209]

Suggested fix

In padurlsanitize.ts, compute sanitizedPadId = await padManager.sanitizePadId(padId) first, then validate sanitizedPadId with isValidPadId(sanitizedPadId).

  • If sanitizedPadId !== padId, redirect to sanitizedPadId (as before).
  • If !isValidPadId(sanitizedPadId) (e.g., an actual stored legacy pad ID containing :), return 404. This preserves legacy :→_ redirect behavior while still preventing : from being treated as a valid canonical pad ID.
[correctness] Legacy ':' redirect regresses
Legacy ':' redirect regresses The `/p/:pad` route rejects pad IDs containing `:` before calling `sanitizePadId()`, so legacy colon→underscore canonicalization no longer runs and the route returns 404. This regression is triggered by `isValidPadId()` now forbidding `:` even though `sanitizePadId()` still has a `/:+/g -> '_'` transform intended to preserve legacy access.

Issue description

padurlsanitize validates the raw URL padId with isValidPadId() before it calls sanitizePadId(). With isValidPadId() now rejecting :, requests that previously would have been normalized (e.g., foo:bar → foo_bar) are rejected early.

Issue Context

sanitizePadId() still contains a legacy transform that maps : to _, but it is now bypassed for HTTP pad URLs.

Fix Focus Areas

  • src/node/hooks/express/padurlsanitize.ts[9-28]
  • src/node/db/PadManager.ts[169-173]
  • src/node/db/PadManager.ts[195-209]

Suggested fix

In padurlsanitize.ts, compute sanitizedPadId = await padManager.sanitizePadId(padId) first, then validate sanitizedPadId with isValidPadId(sanitizedPadId).

  • If sanitizedPadId !== padId, redirect to sanitizedPadId (as before).
  • If !isValidPadId(sanitizedPadId) (e.g., an actual stored legacy pad ID containing :), return 404. This preserves legacy :→_ redirect behavior while still preventing : from being treated as a valid canonical pad ID.
                     PR 8072 (2026-07-25)                    
[performance] Over-broad Vary headers
Over-broad Vary headers `specialpages.ts` now varies public responses on `x-forwarded-prefix` and `x-ingress-path` even when `sanitizeProxyPath()` will ignore those headers unless `settings.trustProxy` is enabled, unnecessarily fragmenting shared caches on attacker-supplied headers. This reduces cache hit rate and, in some shared-cache/CDN setups, can increase cache churn/resource usage without improving correctness when `trustProxy` is false.

Issue description

varyOnProxyPath() unconditionally varies on x-forwarded-prefix and x-ingress-path, but sanitizeProxyPath() only consults those headers when settings.trustProxy is true. When trustProxy is false, the response representation does not depend on those headers, so advertising them in Vary can cause unnecessary cache-key fragmentation.

Issue Context

  • sanitizeProxyPath() gates x-forwarded-prefix and x-ingress-path on trustProxy.
  • specialpages.ts now unconditionally calls res.vary([...]) for all public routes.

Fix Focus Areas

  • src/node/hooks/express/specialpages.ts[28-35]

Suggested fix

Make varyOnProxyPath() configuration-aware:

  • Always res.vary('x-proxy-path').
  • Only add x-forwarded-prefix and x-ingress-path when settings.trustProxy is true (or when you explicitly enable those headers in sanitizeProxyPath() for the current deployment). Example:
[maintainability] Test omits extra Vary
Test omits extra Vary The new `proxyPathVary.ts` test only asserts `Vary` contains `x-proxy-path`, so it will not catch regressions where `x-forwarded-prefix` or `x-ingress-path` are accidentally removed from the production `PROXY_PATH_VARY_HEADERS` list. This weakens the intended coverage of the newly added multi-header vary behavior.

Issue description

The test helper assertVariesOnProxyPath() only checks for x-proxy-path, even though production code varies on three headers. The test suite would still pass if x-forwarded-prefix or x-ingress-path were removed.

Issue Context

Production defines:

  • ['x-proxy-path', 'x-forwarded-prefix', 'x-ingress-path'] Test asserts only:
  • 'x-proxy-path'

Fix Focus Areas

  • src/tests/backend/specs/proxyPathVary.ts[21-29]
  • src/node/hooks/express/specialpages.ts[33-34]

Suggested fix

Update the test to assert all intended vary headers are present. Options:

  1. Hardcode the expected list in the test and assert each is included.
  2. (Better) If behavior is meant to be configuration-dependent, add a test variant where trustProxy is enabled and verify x-forwarded-prefix + x-ingress-path are included (and/or actually influence output), while still always expecting x-proxy-path. Example assertion:
                     PR 8071 (2026-07-25)                    
[correctness] Sanitizer drops doctype/comments
Sanitizer drops doctype/comments ExportHandler now runs stripRemoteImages() on the full export HTML for the soffice path, but stripRemoteImages reserializes via htmlparser2 without emitting processing instructions or comments, so the template’s "" (and any HTML comments) are removed. This is a behavior change from the prior verbatim temp file and can subtly affect LibreOffice’s HTML parsing/rendering for all soffice exports.

Issue description

ExportHandler now writes stripRemoteImages(html) to the LibreOffice (soffice) temp file. stripRemoteImages() rebuilds markup via htmlparser2 but does not preserve processing instructions (e.g., <!doctype html>) or comments, so the exported HTML handed to LibreOffice is no longer identical to the prior output aside from removed remote <img> tags.

Issue Context

The export template includes <!doctype html> and other document-level nodes. stripRemoteImages() only handles open tags, text, and close tags, so document directives are dropped.

Fix Focus Areas

  • src/node/handler/ExportHandler.ts[162-173]
  • src/node/utils/ExportSanitizeHtml.ts[179-215]
  • src/templates/export_html.html[1-10]

Suggested fix approaches

Choose one:

  1. Preserve doctype/comments in stripRemoteImages():
  • Add onprocessinginstruction and oncomment handlers to append the raw directive/comment to out.
  • Ensure doctype is preserved exactly once.
  1. Sanitize only the <body> content for soffice while keeping the original document wrapper:
  • Extract the <body>...</body> portion, run stripRemoteImages() on that fragment, then splice it back into the original HTML document so the doctype/head stay intact.
  • If there is no <body>, fall back to sanitizing the whole string. Either approach keeps the SSRF hardening while minimizing unintended output changes to the soffice conversion input.
                     PR 8070 (2026-07-25)                    
[reliability] OIDC cookie key unstable
OIDC cookie key unstable expressCreateServer() derives oidc-provider cookie signing keys from settings.sessionKey, but Settings defaults enable cookie key rotation and keep sessionKey null if SESSIONKEY.txt is absent, so resolveOidcCookieKeys() falls back to a per-process random key. This invalidates in-flight OIDC interaction/session cookies on restart and prevents horizontally-scaled pods from sharing valid OIDC cookies unless sso.cookieKeys is explicitly set or SESSIONKEY.txt is provisioned.

Issue description

resolveOidcCookieKeys() falls back to a random key whenever settings.sessionKey is null/empty and settings.sso.cookieKeys is unset/invalid. With the repo’s default cookie rotation settings, settings.sessionKey commonly remains null on fresh installs (no SESSIONKEY.txt created), making the embedded OIDC provider’s cookie-signing key change on every restart (and differ per pod).

Issue Context

The Express stack already has a stable cookie-signing secret when rotation is enabled (via SecretRotator), but OAuth2Provider.ts does not reuse it.

Fix Focus Areas

  • src/node/security/OAuth2Provider.ts[100-109]
  • src/node/utils/Settings.ts[736-761]
  • src/node/utils/Settings.ts[1328-1341]
  • src/node/hooks/express.ts[186-201]

Implementation direction

  • Prefer a stable, deployment-wide secret source even when settings.sessionKey is null:
  • Option A (recommended): Create a dedicated SecretRotator namespace for OIDC cookie keys (e.g. oidcCookieSecrets) stored in the DB, and pass its secrets array to cookies.keys (supports rotation + multi-pod stability).
  • Option B: Plumb the Express cookie signing secret(s) (the secret/secretRotator.secrets) to OAuth2Provider.ts and use that as the fallback input to resolveOidcCookieKeys().
  • Option C: Generate and persist an OIDC cookie secret to disk (separate from deprecated SESSIONKEY) and derive from that.
  • Ensure key rotation semantics are preserved (first key signs; older keys still verify).
[maintainability] Template comment misattached
Template comment misattached settings.json.template documents sso.cookieKeys but does not include an actual cookieKeys property, so the comment block becomes the leading comment for the next real key (sso.clients) in the Admin UI’s template-comment extraction. This can mis-document sso.clients and make cookieKeys harder to discover/configure correctly.

Issue description

The cookieKeys documentation block is inserted above sso.clients, but there is no actual "cookieKeys": ... key in the template. The Admin UI’s comment extractor associates leading comments with the next property node, so this block will be shown for sso.clients.

Issue Context

Admin settings comments are extracted from settings.json.template using jsonc-parser node offsets and adjacent comment lookup.

Fix Focus Areas

  • settings.json.template[967-977]
  • admin/src/components/settings/templateComments.ts[17-37]
  • admin/src/components/settings/comments.ts[33-70]

Implementation direction

  • Add a real "cookieKeys" property in settings.json.template (for example: "cookieKeys": ["${OIDC_COOKIE_KEY:}"],) and place the comment immediately above it.
  • Keep resolveOidcCookieKeys()’s existing filtering so an empty env var value safely falls through to the derived/persisted behavior.
                     PR 8042 (2026-07-10)                    
[maintainability] Brittle Dockerfile test matching
Brittle Dockerfile test matching The new docker regression test uses `indexOf()` on exact Dockerfile instruction strings, so harmless refactors (spacing, `install -d` instead of `mkdir`, added flags) will fail CI even if behavior remains correct. This makes future Dockerfile changes unnecessarily risky.

Issue description

dockerfilePluginVolume.ts asserts Dockerfile behavior via exact substring matches, making the test fragile to non-semantic Dockerfile edits.

Issue Context

The intent is to ensure the runtime stages create src/plugin_packages after copying src, not to lock down the exact shell command text.

Fix Focus Areas

  • src/tests/backend/specs/dockerfilePluginVolume.ts[18-58]

Suggested fix

  • Replace the indexOf('COPY --chown=etherpad:etherpad ./src') and indexOf('RUN mkdir -p ./src/plugin_packages') checks with regex-based matches that tolerate formatting and allow equivalent commands.
  • Example approach:
  • Find the first match index for copying ./src with something like /^COPY\s+--chown=etherpad:etherpad\s+\.\/src\/?\s+\.\/src\/?/m.
  • Find a later match for creating the directory with something like /^RUN\s+.*\b(mkdir\s+-p|install\s+-d)\s+\.\/src\/plugin_packages\b/m.
  • Keep the ordering assertion (mkdirMatch.index > srcCopyMatch.index).
                     PR 8039 (2026-07-09)                    
[reliability] Nullable spread suppressed
Nullable spread suppressed In TextLinesMutator.insert(), the PR adds a new "// @ts-ignore" to silence that splitTextLines() can return null, but the code still spreads the value into Array.push(). If splitTextLines() ever returns null (for example, empty string input), push(...newLines) will throw a TypeError at runtime.

Issue description

splitTextLines() is implemented via String.prototype.match() and therefore has a null return possibility, but callers (including TextLinesMutator.insert()) assume an array and spread it. This PR adds a new // @ts-ignore at the call site instead of fixing the contract, leaving a latent runtime crash.

Issue Context

splitTextLines() is documented as returning string[] but currently returns RegExpMatchArray | null. At minimum, it should be made non-null (either by guaranteeing it returns []/[''] for empty input, or by throwing/asserting on invalid input), and then callers can remove the suppression.

Fix Focus Areas

  • src/static/js/TextLinesMutator.ts[278-307]
  • src/static/js/Changeset.ts[739-746]
[maintainability] any[] erases test types
any[] erases test types In hooks.ts, casting `supportedSyncHookFunctions` to `any[]` before concat removes compile-time validation of the test-case object shape. This makes it easier for future edits to accidentally add malformed cases without TypeScript catching it.

Issue description

The new (supportedSyncHookFunctions as any[]) cast disables type checking for the concatenated test cases, reducing maintainability of the test suite.

Issue Context

The tests later assume every element has name, fn, and either want or wantErr. You can preserve TS checking by defining a HookTestCase type and typing both arrays (or using satisfies HookTestCase[]).

Fix Focus Areas

  • src/tests/backend/specs/hooks.ts[565-660]
                     PR 7992 (2026-06-21)                    
[correctness] Stale unorm bundle entry
Stale unorm bundle entry The PR removes `unorm` from the static-file whitelist, but the client bundle manifest still lists `$unorm/lib/unorm.js` under `ace2_inner.js`, so the server will try to serve a static asset that no longer exists and clients can hit 404s when loading that bundle.

Issue description

unorm was removed from dependencies and from LIBRARY_WHITELIST, but src/node/utils/tar.json still references $unorm/lib/unorm.js for the ace2_inner.js bundle. This leaves a bundle manifest entry pointing to a non-existent static asset, causing 404s/broken bundle loads.

Issue Context

  • getTar() reads tar.json at runtime to build the list of files that make up client bundles.
  • Minify.ts no longer allows serving unorm from node_modules, and unorm is no longer installed.

Fix Focus Areas

  • src/node/utils/tar.json[61-84]
  • (optional verification) src/node/hooks/express/static.ts[13-31]
  • (optional verification) src/node/utils/Minify.ts[39-46]
                     PR 7989 (2026-06-21)                    
[maintainability] Help link still AbiWord page
Help link still AbiWord page The updated import dialog string still links to the AbiWord-era wiki page slug, which the checklist flags as outdated/unhelpful for current import tooling guidance. Users may be sent to documentation that does not accurately reflect current LibreOffice vs native import behavior.

Issue description

The import dialog help link still points to an AbiWord-focused wiki page slug (...-with-AbiWord), which violates the requirement that the link target be relevant and accurate for current import/export tooling.

Issue Context

The PR updated the message text but kept the How-to-enable-importing-and-exporting-different-file-formats-with-AbiWord destination. The compliance checklist requires updating this link away from the current AbiWord page.

Fix Focus Areas

  • src/locales/en.json[317-317]
                     PR 7967 (2026-06-15)                    
[reliability] Unbounded basic-ftp override
Unbounded basic-ftp override The override `basic-ftp@=5.3.1'` is open-ended, which already caused pnpm to resolve `basic-ftp@6.0.1` (a major upgrade) rather than the minimal fixed 5.3.1. This increases the chance of unintended breaking upgrades on future lockfile regenerations and impacts Etherpad’s runtime plugin installation path via live-plugin-manager → proxy-agent → get-uri → basic-ftp.

Issue description

The pnpm override for basic-ftp uses an unbounded target range (>=5.3.1), which allows pnpm to select newer major versions (as seen with 6.0.1). This can introduce breaking changes during lockfile refreshes without an explicit decision.

Issue Context

basic-ftp is pulled in transitively via get-uri which is part of the proxy-agent chain used by live-plugin-manager (a runtime dependency used for plugin installation).

Fix

Choose one of the following and regenerate the lockfile:

  1. Min-risk security floor: pin to the minimal patched major: basic-ftp@<5.3.1: '5.3.1' (or ^5.3.1).
  2. If major v6 is intended: pin to the known-good version used in this PR: basic-ftp@<5.3.1: '6.0.1' (or constrain to <7 if you want controlled majors). Then run pnpm install to update pnpm-lock.yaml accordingly.

Fix Focus Areas

  • pnpm-workspace.yaml[21-45]
  • pnpm-lock.yaml[7-29]
                     PR 7960 (2026-06-15)                    
[security] Readonly pad can delete
Readonly pad can delete If allowPadDeletionByAllUsers is enabled, readonly sessions will receive clientVars.canDeletePad=true and the UI will unhide #delete-pad even on readonly pads. The backend PAD_DELETE handler also allows token-less deletion under allowPadDeletionByAllUsers without checking session.readonly, so anyone with a readonly link can delete pads (data loss).

Issue description

With allowPadDeletionByAllUsers: true, readonly sessions can see and use the token-less delete button because canDeletePad ignores sessionInfo.readonly, and the server-side handlePadDelete() flagOk branch also ignores session.readonly.

Issue Context

allowPadDeletionByAllUsers is documented as allowing any user who can edit a pad to delete it; readonly viewers should not gain deletion rights.

Fix Focus Areas

  • src/node/handler/PadMessageHandler.ts[1323-1333]
  • src/node/handler/PadMessageHandler.ts[290-314]
  • src/static/js/pad_editor.ts[152-168]

Suggested fix

  1. In handleClientReady, compute canDeletePad so it is false for readonly sessions, for example:
  • const canDeletePad = !sessionInfo.readonly && (isCreator || settings.allowPadDeletionByAllUsers); (or equivalent).
  1. In handlePadDelete, prevent token-less deletion from readonly sessions by adding !session.readonly to the non-token authorization paths (at minimum the flagOk branch; consider also creatorOk for consistency with the UI).
  2. Optionally add/adjust a test to cover that a readonly session does not receive canDeletePad=true under allowPadDeletionByAllUsers, and that PAD_DELETE is rejected from readonly when no token is supplied.
                     PR 7949 (2026-06-12)                    
[reliability] Locale-fragile banner assertions
Locale-fragile banner assertions The new timeslider Playwright specs assert English-only strings (e.g., `Version N`, `Saved …`) and parse localized UI text with `Date()`, which will fail when the browser locale is not English or uses a non-JS-Date-parseable format. These files do not pin `locale`, unlike other existing Playwright specs that do so when asserting English text.

Issue description

New specs assert exact English UI strings ("Version N", "Saved …") and parse the localized timer/banner text via new Date(...). This breaks when Playwright runs with a non-English locale or with a locale whose timeslider.dateformat / timeslider.saved strings are not parseable by JS Date.

Issue Context

  • #history-banner-rev mirrors the inner timeslider #revision_label, which is set via html10n.get('timeslider.version', ...).
  • #history-banner-date mirrors #revision_date, which is localized via html10n.get('timeslider.saved', ...) and is not guaranteed to start with "Saved ".
  • #history-timer mirrors #timer, which uses html10n.get('timeslider.dateformat', ...) and varies significantly by locale.

Fix Focus Areas

  • src/tests/frontend-new/specs/timeslider_revision_labels.spec.ts[24-67]
  • src/tests/frontend-new/specs/timeslider_export_links.spec.ts[40-46]
  • src/tests/frontend-new/specs/timeslider_deeplink.spec.ts[16-24]

Suggested fix approach

  1. Add test.use({locale: 'en-US'}); near the top of each new spec file that compares against English strings.
  2. Prefer locale-independent assertions:
  • Instead of parsing with new Date(text).getTime(), assert text is non-empty and does not contain NaN.
  • For the revision label, assert it matches a pattern like /\d+/ (or read the slider value and only assert the label contains the revision number).
[reliability] Numeric padId collision risk
Numeric padId collision risk `timeslider_export_links.spec.ts` generates a numeric padId with only 1000 possible random suffixes, which can collide across Playwright’s parallel workers and multiple browser projects, causing tests to share the same pad and interfere with each other. This is avoidable flakiness given other helpers already use UUID-backed pad IDs for uniqueness.

Issue description

The export-links test uses 735773577357 + Math.floor(Math.random() * 1000) for its numeric pad id, giving only 1000 possible ids. Under Playwright parallelism (multiple workers + multiple browser projects), collisions are possible and can make test runs interfere by operating on the same pad.

Issue Context

  • Playwright is configured to run fully parallel with multiple workers and at least two projects (chromium + firefox).
  • Other helpers already generate essentially-collision-free pad ids with randomUUID().

Fix Focus Areas

  • src/tests/frontend-new/specs/timeslider_export_links.spec.ts[52-57]

Suggested fix approach

Generate a numeric id with much higher entropy, e.g.:

  • const padId = String(Date.now()) + String(Math.floor(Math.random() * 1_000_000));
  • Or derive digits from a UUID (strip non-digits / hash to integer) while keeping the “numeric pad id” property. (Keep the pad id numeric to preserve the original regression guard.)
                     PR 7948 (2026-06-12)                    
[correctness] Stars miss live updates
Stars miss live updates renderSavedRevisionStars() renders markers from innerWin.clientVars.savedRevisions, but the timeslider’s NEW_SAVEDREV path only adds an inner DOM star and does not update clientVars, so the outer overlay won’t reflect saved revisions created after entering history mode (e.g., by collaborators).

Issue description

The outer in-pad history slider markers are rendered from innerWin.clientVars.savedRevisions, but that array is never updated when the iframe receives NEW_SAVEDREV. As a result, new saved revisions created while the user is already in history mode (including from other collaborators) will not appear as markers on the outer slider.

Issue Context

The iframe timeslider receives NEW_SAVEDREV and calls BroadcastSlider.addSavedRevision(...), which updates the inner hidden slider DOM but does not mutate clientVars.savedRevisions. pad_mode.ts relies on clientVars.savedRevisions as its source of truth.

Fix Focus Areas

  • src/static/js/pad_mode.ts[402-449]
  • src/static/js/broadcast.ts[515-518]
  • src/static/js/broadcast_slider.ts[227-242]

Suggested implementation direction

  • Ensure the iframe’s clientVars.savedRevisions stays updated when NEW_SAVEDREV arrives (e.g., in broadcast.ts add the saved revision object to window.clientVars.savedRevisions with dedupe/sort).
  • Trigger a re-render of the outer stars when a saved revision is added (e.g., in pad_mode.ts wrap/monkey-patch inner.BroadcastSlider.addSavedRevision once to: (a) update inner.clientVars.savedRevisions and (b) call renderSavedRevisionStars(innerWin)).
[correctness] Rev0 marker never renders
Rev0 marker never renders renderSavedRevisionStars() clears the overlay when `max <= 0`, which prevents rendering a valid saved revision at `revNum === 0` when the document has only one revision (`max === 0`).

Issue description

renderSavedRevisionStars() returns early when max <= 0, which blocks rendering a saved revision marker at revision 0 when the slider range is [0..0].

Issue Context

Saved revisions can legitimately be at revision 0. When the pad’s current head revision is 0, the slider max is 0.

Fix Focus Areas

  • src/static/js/pad_mode.ts[415-434]

Suggested implementation direction

  • Change the guard to allow max === 0 (only treat max < 0 as invalid).
  • Keep the existing frac handling (max === 0 ? 0 : revNum / max) so rev 0 renders at left: 0%.
[reliability] Test allows left=0
Test allows left=0 The new Playwright test’s position assertion uses `parseFloat(style.left) || getBoundingClientRect().left`, so a marker with `style.left` of `0%` falls back to a positive page X coordinate and the test passes even if the marker is collapsed to the origin.

Issue description

The test is intended to fail if a marker is rendered at left: 0%, but the current code treats 0 as falsy and falls back to getBoundingClientRect().left, which is almost always > 0.

Issue Context

This weakens the regression protection: a degenerate render can still pass.

Fix Focus Areas

  • src/tests/frontend-new/specs/timeslider_saved_revisions.spec.ts[52-56]

Suggested implementation direction

  • Explicitly assert on the parsed percentage value and avoid || fallback on 0.
  • Example: const leftPct = await marker.first().evaluate(el => parseFloat((el as HTMLElement).style.left)); expect(leftPct).toBeGreaterThan(0); expect(leftPct).toBeLessThan(100);
  • Or compare marker X against the slider track’s bounding box (relative check).
                     PR 7938 (2026-06-10)                    
[correctness] Unreadable pads can't delete
Unreadable pads can't delete padLoad now returns unreadable pad IDs with zeroed metadata, but the existing deletePad flow only deletes pads that pass doesPadExists() (requires value.atext) and can be hydrated via getPad(), so migration-corrupted pad records stored as strings will remain listed but undeletable and will not emit any delete result.

Issue description

padLoad now surfaces unreadable pads (zeroed metadata) so admins can delete them, but deletePad currently refuses to delete if doesPadExists() returns false (which happens for non-object corrupt records) and otherwise attempts getPad() (which throws for unreadable records). This makes the surfaced corrupt pad effectively undeletable via the admin UI.

Issue Context

  • Corrupt records can be present under pad:* keys and trigger Pad.init failures.
  • The admin UI’s delete action goes through the /settings socket deletePad handler.

Fix Focus Areas

  • Update delete handler to support deleting unreadable/corrupt pad records by key/prefix without requiring hydration (getPad).
  • Ensure the handler always emits a terminal results:deletePad reply (success or error) so the UI does not silently do nothing.
  • Consider using padManager.removePad(padId) (updates padList) and, if needed, a targeted prefix cleanup for pad:${padId}:revs:*, pad2readonly:*, readonly2pad:*, etc., when hydration is impossible.
  • src/node/hooks/express/adminsettings.ts[175-196]
  • src/node/hooks/express/adminsettings.ts[286-294]
  • src/node/db/PadManager.ts[155-205]
  • src/node/db/Pad.ts[584-592]
[security] Logs may expose corrupt value
Logs may expose corrupt value The new error handling logs `${err}` for unreadable pads and for padLoad failures; for the reported corruption mode, the thrown error message includes the raw stored value (the corrupt JSON string), which can leak sensitive pad content into logs and cause log spam if the value is large.

Issue description

padLoad now catches errors but logs them via string interpolation (${err}). For the corruption case described (non-object value under pad:${id}), the thrown message can include the raw stored value, which may be a large JSON string (potentially containing pad content). Logging it verbatim can leak sensitive data and bloat logs.

Issue Context

  • The failure originates from Pad.init when the stored value is not an object.
  • Both the per-pad catch and the outer handler catch log the raw error.

Fix Focus Areas

  • Log only safe fields (err.name, a sanitized/truncated err.message) and strip newlines.
  • Avoid including the raw corrupt value in logs; if needed, include a short hash/length instead.
  • Apply the same sanitization to both the per-pad warning and the outer handler error log.
  • src/node/hooks/express/adminsettings.ts[193-196]
  • src/node/hooks/express/adminsettings.ts[274-281]
  • src/node/db/Pad.ts[584-592]
  • src/tests/backend/specs/admin/padLoadResilience.ts[7-19]
                     PR 7930 (2026-06-09)                    
[correctness] Token disclosure still shown
Token disclosure still shown With `allowPadDeletionByAllUsers=true`, the UI still renders the token-based deletion disclosure (including the `Pad deletion token` field), which conflicts with the requirement to suppress token-related UI/wording in this mode.

Issue description

PR Compliance ID 1 requires that when allowPadDeletionByAllUsers=true, token-related deletion UI (modal and token wording/controls) must not appear. The PR suppresses token issuance and relabels the disclosure summary, but the token disclosure still renders a Pad deletion token label and input, so token UI/wording is still present.

Issue Context

  • The template always renders the token disclosure when !settings.requireAuthentication, regardless of allowPadDeletionByAllUsers.
  • The new client-side change only renames the disclosure summary when clientVars.canDeleteWithoutToken is true; it does not remove/hide the token label/input.

Fix Focus Areas

  • src/templates/pad.html[390-398]
  • src/static/js/pad_editor.ts[152-191]
                     PR 7928 (2026-06-09)                    
[reliability] Unasserted locale check
Unasserted locale check The new regression test checks whether the Bold button tooltip is German via locator.evaluate(), but it never asserts the boolean result, so the test will not fail if the UI is not actually rendered in German. This weakens the regression coverage and can allow language-detection regressions to slip through.

Issue description

The regression test dropdown reflects the browser-detected language calls locator.evaluate() with a boolean expression, but the returned boolean is ignored. This makes the “UI rendered in German” verification non-functional.

Issue Context

Other frontend tests assert evaluate() results via expect(...), so this test should do the same (or use a more direct Playwright assertion like toHaveAttribute).

Fix Focus Areas

  • src/tests/frontend-new/specs/language.spec.ts[104-107]

Suggested change

Replace the unused await locator.evaluate(...) call with an assertion, for example:

                     PR 7927 (2026-06-09)                    
[reliability] Child bash not strict
Child bash not strict `run-clients.sh` sets `set -euo pipefail`, but the manifest commands now run under a fresh `bash -c` without `-e/-u/-o pipefail`, so masked failures (especially pipeline stage failures) or unset-variable mistakes inside `vectorTest`/`smokeCmd` may not surface as non-zero exits.

Issue description

run-clients.sh is in strict mode (set -euo pipefail), but the newly introduced bash -c "$vectorTest" / bash -c "$smokeCmd" executes those manifest commands in a new Bash process that does not automatically inherit -e, -u, or pipefail. This can allow certain failures inside the command string (notably pipeline stage failures without pipefail) to be missed.

Issue Context

The manifest commands come from clients.json and are intended to be robustly checked. The outer script only sees the exit status of the child bash -c process.

Fix Focus Areas

  • src/tests/downstream/run-clients.sh[68-76]

Suggested change

Update the command execution to enable strict mode in the child bash, for example:

  • bash -eu -o pipefail -c "$vectorTest" || exit 1
  • bash -eu -o pipefail -c "$smokeCmd" || exit 1 (Apply similarly for both rust and node|desktop cases.)
                     PR 7923 (2026-06-09)                    
[reliability] Unverified sed settings rewrite
Unverified sed settings rewrite The workflow rewrites settings.json.template via `sed` but never checks that the substitutions succeeded, so a template change can silently leave port/auth unchanged and cause confusing smoke failures.

Issue description

The CI boot step uses sed to rewrite the template’s port and authentication method, but sed exits successfully even if the patterns don’t match. If settings.json.template changes format, the job can boot with the wrong port/auth and fail later (healthcheck/self-check) without a clear root cause.

Issue Context

The workflow assumes literal strings exist in the template and does not validate the generated settings.json.

Fix Focus Areas

  • .github/workflows/downstream-smoke.yml[52-63]

Suggested fix

After generating settings.json, validate expected settings and fail fast if missing:

[performance] Docs ignore path mismatch
Docs ignore path mismatch The downstream-smoke workflow ignores only "doc/**" changes, but this PR’s documentation lives under "docs/**", so docs-only PRs will still run the full smoke boot job unnecessarily.

Issue description

The downstream smoke workflow is configured to ignore only doc/**, but this PR adds docs under docs/**. As a result, documentation-only PRs will still trigger the workflow and boot Etherpad.

Issue Context

Repo contains both doc/ and docs/ trees; the new plan/spec files are under docs/.

Fix Focus Areas

  • .github/workflows/downstream-smoke.yml[8-13]

Suggested fix

Update paths-ignore to include docs/** (and optionally keep doc/**):

                     PR 7917 (2026-06-08)                    
[maintainability] Outbound-call doc mislinked
Outbound-call doc mislinked New docs and settings-file comments direct operators to `doc/privacy.md` for outbound network call details, but the outbound-call inventory and disable instructions are in the repo-root `PRIVACY.md`. This can mislead offline operators and conflicts with runtime messages that explicitly reference `PRIVACY.md`.

Issue description

Multiple newly-added references say outbound network calls are documented in doc/privacy.md, but that file is about user data/logging; the outbound-call documentation is in the repo-root PRIVACY.md. This can send operators to the wrong document when trying to disable outbound requests.

Issue Context

  • PRIVACY.md explicitly documents the version check + plugin catalog outbound calls and how to disable them.
  • Runtime errors/info messages also say “see PRIVACY.md”.
  • doc/privacy.md is a different document (data-processing statement), so the current links/comments are misleading.

Fix Focus Areas

  • doc/docker.md[121-135]
  • settings.json.docker[328-337]
  • settings.json.template[448-455] Suggested change: replace doc/privacy.md references with PRIVACY.md (and in doc/docker.md use the correct relative link to the repo root, e.g. ../PRIVACY.md).
[maintainability] Overstates tier=off effect
Overstates tier=off effect `doc/admin/updates.md` claims `updates.tier="off"` means no HTTP request will leave the instance, but `UpdateCheck.ts` will still fetch `${updateServer}/info.json` unless `privacy.updateCheck` is disabled. This can cause unexpected outbound attempts/log noise in air-gapped deployments that only set `UPDATES_TIER=off`.

Issue description

The docs currently state that setting updates.tier to off stops all HTTP requests. In reality, it only disables the self-updater polling (GitHub Releases); the separate updateServer-based version check still runs unless privacy.updateCheck=false.

Issue Context

  • src/node/updater/index.ts skips updater polling when settings.updates.tier === 'off'.
  • src/node/utils/UpdateCheck.ts performs a fetch to settings.updateServer gated only by settings.privacy.updateCheck.

Fix Focus Areas

  • doc/admin/updates.md[103-108] Suggested change: adjust wording to something like:
  • "Set updates.tier to off to disable the self-updater (GitHub Releases polling)."
  • "To stop the separate version check, also set privacy.updateCheck=false (or PRIVACY_UPDATE_CHECK=false in Docker)."
                     PR 7906 (2026-06-07)                    
[correctness] Auth fallback inconsistent
Auth fallback inconsistent `/api/2` only copies `headers.authorization` into `fields.authorization` when the field is null/undefined, but the OpenAPI router falls back to the header when the field is any falsy value. This can cause the same request to authenticate differently depending on which router handles it (e.g., body/query includes `authorization: ""`).

Issue description

src/node/handler/RestAPI.ts forwards the authorization header only when fields.authorization == null, but the OpenAPI router (src/node/hooks/express/openapi.ts) uses a falsy check (fields.authorization || headers.authorization). This creates inconsistent behavior across API entrypoints.

Issue Context

  • RestAPI: fields.authorization is set only if it is null/undefined.
  • OpenAPI router: header is used as a fallback if fields.authorization is falsy (including empty string).

Fix Focus Areas

  • src/node/handler/RestAPI.ts[1474-1480]

Suggested change

Update RestAPI to match the OpenAPI behavior, for example:

  • if (headers?.authorization && !fields.authorization) fields.authorization = headers.authorization; (or fields.authorization ||= headers.authorization), so that empty-string (and other falsy) values don’t suppress the header.
                     PR 7901 (2026-06-05)                    
[reliability] Flaky ace_inner frame access
Flaky ace_inner frame access The new RTL test dereferences `page.frame('ace_inner')!` immediately after navigation, but `appendQueryParams()` does not wait for the inner iframe/body to be ready, so the test can intermittently throw (null frame) or race against iframe initialization.

Issue description

The test uses page.frame('ace_inner')! without waiting for the inner frame to exist/be ready. appendQueryParams() only waits for the outer iframe and #editorcontainer.initialized, and the repo already documents that this is not sufficient to guarantee the inner editor is ready.

Issue Context

There is an existing waitForEditorReady() implementation that waits for #innerdocbody[contenteditable="true"], but appendQueryParams() does not call it.

Fix Focus Areas

  • src/tests/frontend-new/specs/rtl_url_param.spec.ts[38-47]
  • src/tests/frontend-new/helper/padHelper.ts[107-115]
  • src/tests/frontend-new/helper/padHelper.ts[117-133]

Suggested fix

Use a frameLocator chain (which Playwright will auto-wait for) and/or explicitly wait for #innerdocbody readiness before asserting:

[reliability] Hardcoded LTR dir assertion
Hardcoded LTR dir assertion The test asserts the top-level `` has `dir="ltr"`, but page direction is set by localization (cookie / `navigator.language`) and can legitimately be `rtl`, making the test environment-dependent and prone to false failures.

Issue description

The test hard-codes dir="ltr" on the top-level <html>, but the app sets document.documentElement.dir from html10n.getDirection() which depends on localization inputs. This can be rtl on RTL locales, so the assertion is brittle.

Issue Context

l10n.ts sets the page dir when localization completes, and Playwright config does not force a locale globally.

Fix Focus Areas

  • src/tests/frontend-new/specs/rtl_url_param.spec.ts[38-52]
  • src/static/js/l10n.ts[11-18]
  • src/playwright.config.ts[46-69]

Suggested fix

Instead of asserting LTR specifically, assert that toggling the pad RTL option does not change whatever the UI language chose:

                     PR 7895 (2026-06-04)                    
[correctness] Recent pads double-encoding
Recent pads double-encoding colibris recent pad links now apply `encodeURIComponent(pad.name)` to values loaded from `recentPads`, so pre-existing entries that already contain percent-escapes (e.g. `Test%2F123`) will become double-encoded (`%252F`) and open the wrong pad. This is triggered by the storage format changing to decoded names in `colibris/pad.js` without migrating older stored values.

Issue description

recentPads entries from older versions can already be URL-encoded. The new code encodes pad.name again when building the href, producing double-encoded URLs and broken navigation.

Issue Context

  • colibris/pad.js now stores a decoded pad name.
  • colibris/index.js encodes whatever is stored in localStorage when generating recent pad URLs.
  • Existing localStorage data is not migrated/normalized.

Fix

When reading recentPads from localStorage in colibris/index.js, normalize each entry’s name to a decoded form using a safe decode (try/catch). Then:

  • Use the normalized (decoded) name for link.innerText.
  • Use encodeURIComponent(normalizedName) for the href.
  • Optionally rewrite localStorage once with normalized names so the fix is persistent and deduping works consistently.

Fix Focus Areas

  • src/static/skins/colibris/index.js[34-79]
  • src/static/skins/colibris/pad.js[10-34]
[security] Missing noopener on _blank
Missing noopener on _blank The Admin Pads “Open” button uses `window.open(..., '_blank')` without `noopener,noreferrer`, allowing the opened page to access `window.opener` and potentially redirect the admin tab (reverse tabnabbing). The codebase already uses `noopener,noreferrer` for safe `_blank` opens elsewhere.

Issue description

window.open(url, '_blank') without noopener allows the new page to control window.opener, enabling reverse tabnabbing if the target is compromised or redirected.

Issue Context

The repo already mitigates this risk in other UI code by passing 'noopener,noreferrer' as the third argument to window.open.

Fix

Update the Admin Pads “Open” handler to:

  • window.open(url, '_blank', 'noopener,noreferrer') Optionally, for extra defense-in-depth, set newWindow.opener = null if a window handle is returned.

Fix Focus Areas

  • admin/src/pages/PadPage.tsx[419-423]
                     PR 7888 (2026-06-02)                    
[correctness] Nonexistent server.js example
Nonexistent server.js example `doc/configuration.md` documents overriding settings via `node src/node/server.js --settings ...`, but this repo runs `src/node/server.ts` via the `tsx` loader, so the documented command will fail if copy/pasted.

Issue description

doc/configuration.md shows an example command that references src/node/server.js, which is not the entrypoint used by this repo.

Issue Context

The runtime entrypoint is src/node/server.ts and the package scripts execute it with tsx.

Fix Focus Areas

  • doc/configuration.md[14-16]
  • src/package.json[150-157]
[maintainability] CLI help references .js
CLI help references .js The new `compactStalePads` pnpm script runs `compactStalePads.ts` via `tsx`, but the script’s own usage text still instructs `node bin/compactStalePads.js ...`, which points users to a non-existent `.js` file.

Issue description

The compactStalePads CLI is now exposed as a pnpm script that executes a .ts file via tsx, but bin/compactStalePads.ts still prints .js-based usage instructions.

Issue Context

Docs now instruct users to run these tools via pnpm run --filter bin ..., so the CLI’s help text should match.

Fix Focus Areas

  • bin/compactStalePads.ts[256-261]
  • bin/package.json[21-29]
  • doc/cli.md[70-75]
                     PR 7887 (2026-06-02)                    
[correctness] Creator privileges lost
Creator privileges lost For new pads created via CLIENT_READY, Pad.init now writes revision 0 with Pad.SYSTEM_AUTHOR_ID, so no real user will ever match getRevisionAuthor(0). PadMessageHandler uses revision 0 author to gate creator-only features (deletion token issuance, creator delete path, and pad-wide settings), so the actual creator loses those capabilities.

Issue description

New pads created via getPad(padId, null, session.author) now store revision 0’s meta.author as the system author (a.etherpad-system). The server uses getRevisionAuthor(0) as the canonical pad creator identity, so creator-only features (deletion token issuance, pad deletion by creator, and editing pad-wide settings) stop working for the real creating user.

Issue Context

The PR changes Pad.init() to attribute default content to Pad.SYSTEM_AUTHOR_ID by passing effectiveAuthorId into appendRevision(). appendRevision() stores that author into revision 0 meta.

Fix Focus Areas

  • src/node/db/Pad.ts[593-620]
  • src/node/handler/PadMessageHandler.ts[290-369]
  • src/node/handler/PadMessageHandler.ts[1126-1132]
  • src/node/handler/PadMessageHandler.ts[1284-1300]

Proposed fix

  1. Introduce a persisted creatorId (or similar) field on the pad object that is set at creation time from the initiating real author (e.g., session author in CLIENT_READY).
  2. Keep default-text attribution as system author for changeset ops, but do not use the system author as the creator identity.
  3. Update creator checks in PadMessageHandler (token issuance, isPadCreator(), and delete logic) to use pad.creatorId when present, with a fallback to getRevisionAuthor(0) for older pads.
  4. Add/adjust backend tests to ensure:
  • Default welcome text is owned by system author.
  • The creating user is still recognized as creator for pad deletion token issuance and pad-wide settings permissions.
                     PR 7842 (2026-05-25)                    
[observability] Mid-snapshot suppresses boundary report
Mid-snapshot suppresses boundary report The new tryWriteReport('mt', 0) updates the shared lastReportT timestamp, which can cause the next test's tryWriteReport('be', 100) to be throttled and skipped if it starts within 100ms after the mid-test snapshot. This reduces boundary report coverage specifically after slow tests (the exact tests most likely to generate mt reports).

Issue description

tryWriteReport() uses a single global lastReportT for all report types. The newly added mid-test report (mt) updates lastReportT, which can inadvertently throttle and suppress subsequent boundary reports (be) in the next test’s beforeEach.

Issue Context

beforeEach always calls tryWriteReport('be', 100) and now additionally schedules a mid-test tryWriteReport('mt', 0).

Fix Focus Areas

  • src/tests/backend/diagnostics.ts[97-121]
  • src/tests/backend/diagnostics.ts[192-224]

Suggested fix direction

Introduce separate throttle state for boundary vs. mid-test reports (e.g., lastBoundaryReportT for be and a different timestamp for hb/mt), or add an option so mt writes do not advance the timestamp used for be throttling.

[performance] Unconditional mid-test timer overhead
Unconditional mid-test timer overhead beforeEach schedules a setTimeout for every test even when canWriteReport is false, so the callback work is guaranteed to be wasted on runs without node-report enabled. This adds unnecessary timer/callback churn across the whole suite for no possible diagnostic output.

Issue description

The new mid-test snapshot timer is scheduled unconditionally, but the callback can never produce a report when canWriteReport is false because tryWriteReport() returns immediately.

Issue Context

canWriteReport already gates report writing, but the timer is still created per test.

Fix Focus Areas

  • src/tests/backend/diagnostics.ts[97-110]
  • src/tests/backend/diagnostics.ts[192-224]

Suggested fix direction

Only schedule the 150ms setTimeout when canWriteReport is true (or inline-check before scheduling). This preserves the diagnostic behavior in CI configurations that enable node-report while avoiding per-test timers in runs where reports are disabled.

                     PR 7839 (2026-05-25)                    
[maintainability] `tests` symlink removal undocumented
`tests` symlink removal undocumented The PR deletes the root `tests` symlink, but `CONTRIBUTING.md` still instructs contributors to find front-end tests under `tests/frontend/`, which will no longer exist and misleads new contributors. This violates the requirement to update documentation alongside user-facing project structure changes, as the tests now live under `src/tests/frontend/` and the contributing guide must reflect that actual path.

Issue description

The root tests symlink is removed by this PR, but contributor documentation still references tests/frontend/ as the location of front-end tests, making it a dead path that will mislead new contributors.

Issue Context

Tests live under src/tests/... (and other docs already reflect that), so all references to tests/frontend/ in contributor guides should be updated to point to src/tests/frontend/ instead.

Fix Focus Areas

  • CONTRIBUTING.md[127-131]
  • bin/plugins/lib/CONTRIBUTING.md[115-121]
[maintainability] Stale best_practices mention
Stale best_practices mention A design doc under `docs/` still cites `best_practices.md` as a policy source even though this PR deletes that file. Readers will be left with misleading documentation and an implied policy reference that no longer exists.

Issue description

best_practices.md is deleted, but at least one in-repo doc still references it as an authoritative policy document.

Issue Context

The policy appears to live in CONTRIBUTING.md / AGENTS.MD now, so the reference should be removed or replaced.

Fix Focus Areas

  • docs/superpowers/specs/2026-05-08-issue-7693-admin-openapi-design.md[198-201]
                     PR 7838 (2026-05-25)                    
[reliability] Heartbeat optional-chain TypeError
Heartbeat optional-chain TypeError The heartbeat computes handle/request counts via `(process as any)._getActiveHandles?.().length` and `_getActiveRequests?.().length`, but `?.()` only guards the call — if the method is missing it returns `undefined` and the subsequent `.length` access throws a TypeError. Because `diagnostics.ts` is loaded via `mocha --require` in the main backend test script, this would take down the whole test run on Node builds where those private APIs are absent/changed.

Issue description

The heartbeat uses optional chaining only on the private API call (?.()), but not on the returned value before accessing .length, so the fallback ?? -1 is never reached if the API is missing.

Issue Context

This file is always loaded for backend tests via --require ./tests/backend/diagnostics.ts, so an exception inside the heartbeat interval can crash the test process.

Fix

Change both expressions to optionally chain the returned value as well (or otherwise guard the length access). Example:

                     PR 7837 (2026-05-25)                    
[correctness] Storage type too narrow
Storage type too narrow `storage` is typed as `AdapterPayload | string[]`, but `upsert()` stores a string `id` for the userCode→id index, which will fail TypeScript checking (`tsc --noEmit`) and obscures the real stored value types. This is introduced by narrowing the cache type union while leaving string writes in place.

Issue description

storage is declared as LRUCache<string, AdapterPayload | string[]>, but the adapter stores string values for the userCode index (storage.set(userCodeKeyFor(...), id)). This creates a compile-time type error (and hides legitimate runtime values).

Issue Context

The repo runs ts-check via tsc --noEmit, so this mismatch can block builds.

Fix Focus Areas

  • src/node/security/OIDCAdapter.ts[14-16]
  • src/node/security/OIDCAdapter.ts[88-90]
  • src/package.json[150-158]

Suggested fix

  • Update the cache value type to include string (e.g., AdapterPayload | string[] | string) and adjust get() casts accordingly.
  • Optionally, tighten the API by creating separate caches (payload vs. indexes) to avoid widening unions, but the minimal change is adding string back.
                     PR 7831 (2026-05-22)                    
[reliability] Missing DB method hidden
Missing DB method hidden DB.init() now silently skips exporting findKeysPaged if it is missing, but SessionStore._cleanup() calls DB.findKeysPaged unconditionally, which will crash later at runtime with a TypeError instead of failing fast at startup.

Issue description

src/node/db/DB.ts now does if (typeof f !== 'function') continue; when wiring DB methods. If findKeysPaged is missing for any reason (backend mismatch, dependency resolution issue), DB.findKeysPaged will be undefined, but SessionStore._cleanup() calls it unconditionally, causing a delayed runtime crash.

Issue Context

findKeysPaged is now a hard requirement for cleanup. Skipping export hides misconfiguration and shifts failure from startup to the first cleanup run.

Fix Focus Areas

  • Require findKeysPaged to exist during DB.init() and throw an explicit error if missing.
  • Consider keeping the guard for truly-optional methods, but not for methods that the app requires.

Fix Focus Areas (code references)

  • src/node/db/DB.ts[39-56]
  • src/node/db/SessionStore.ts[87-99]
[reliability] Cleanup can run unbounded
Cleanup can run unbounded SessionStore._cleanup() now stops only when findKeysPaged returns an empty page, so under sustained creation of new sessionstorage keys it can run for an unbounded time and delay the next scheduled cleanup indefinitely.

Issue description

SessionStore._cleanup() uses while (true) and terminates only when findKeysPaged() returns an empty page. If new session keys are continually being created while cleanup runs, the loop can keep discovering more keys and take unbounded time, which also delays the next cleanup run because scheduling happens only after completion.

Issue Context

Session cleanup runs in-process alongside request handling, and the next cleanup run is scheduled only after the current run finishes.

Fix Focus Areas

  • Add an upper bound to each cleanup run (time budget and/or max keys/pages scanned).
  • Once the budget is hit, log that cleanup yielded early and let the next scheduled run continue later.

Suggested approach

  • Track const start = Date.now() and break if Date.now() - start > MAX_CLEANUP_RUNTIME_MS.
  • Or track scanned and break after MAX_SCANNED_KEYS.

Fix Focus Areas (code references)

  • src/node/db/SessionStore.ts[54-66]
  • src/node/db/SessionStore.ts[87-129]
[reliability] Test cleanup not symmetric
Test cleanup not symmetric The new paging regression test only deletes validSids in finally; if cleanup regresses or throws before removals, expiredSids can be left behind and accumulate across runs (even though they are tagged).

Issue description

The new test seeds 25 expired + 25 valid sessionstorage rows. In the finally block it removes only validSids. If _cleanup() fails or the test fails before assertions, the expired rows may remain and accumulate across repeated test runs.

Issue Context

The rows are tagged, so functional interference is limited, but leaving DB state behind can still slow tests over time.

Fix Focus Areas

  • In finally, delete both expiredSids and validSids (or delete by the shared tag prefix).

Fix Focus Areas (code references)

  • src/tests/backend/specs/SessionStore.ts[318-362]
                     PR 7826 (2026-05-20)                    
[reliability] Auth error reconnect loop
Auth error reconnect loop Non-admin clients are now forcibly disconnected from /settings, but the admin SPA reconnects on server-initiated disconnects, which can create an infinite reconnect loop with repeated toasts and a stuck loading overlay. This can spam the server with reconnect attempts and prevent the UI from settling into a stable “not authorized” state.

Issue description

The backend now emits admin_auth_error and then calls socket.disconnect(true) for non-admin sockets. In the admin SPA, the existing disconnect handler reconnects whenever the reason is 'io server disconnect', which is exactly what happens for server-initiated disconnects. This can cause an infinite reconnect loop for unauthorized sessions (toast spam + loading overlay toggling + unnecessary server load).

Issue Context

  • Server change disconnects non-admin sockets.
  • Client reconnect logic is intended for transient server disconnects, but should not apply to an authorization failure.

Fix Focus Areas

  • admin/src/App.tsx[47-50]
  • admin/src/App.tsx[80-91]
  • (optional) src/node/hooks/express/adminsettings.ts[53-62]

Suggested fix

  1. In admin/src/App.tsx, set a flag when admin_auth_error is received (or immediately disable reconnection via settingSocket.io.opts.reconnection = false), and in the disconnect handler skip settingSocket.connect() when that flag is set.
  2. Consider navigating the user to /login (or showing a stable logged-out state) on admin_auth_error.
  3. Optionally, adjust the server to include a machine-readable reason (e.g. {code: 'ADMIN_AUTH_REQUIRED'}) so the client can reliably disable reconnect without string matching.
                     PR 7821 (2026-05-19)                    
[correctness] Admin auth endpoint wrong
Admin auth endpoint wrong adminCookieHeader() requests GET /admin/ with Basic Auth, but when requireAuthentication is false (Docker default) that route is authorized without running the authentication flow, so no admin session cookie is created and the /settings socket never registers handlers. The test will hang/timeout on load() because adminsettings.ts ignores non-admin socket connections.

Issue description

The container spec tries to establish an admin session by requesting GET /admin/ with Basic Auth, but /admin/ does not force authentication when requireAuthentication is false (the default in settings.json.docker). As a result, no admin session cookie is set and the /settings namespace handlers never get attached (because the server checks session.user.is_admin).

Issue Context

/admin-auth/ is the dedicated endpoint for establishing/verifying an admin session cookie.

Fix Focus Areas

  • src/tests/container/specs/api/adminSettings_7819.ts[23-31]
  • src/node/hooks/express/openapi-admin.ts[30-38]
  • src/node/hooks/express/webaccess.ts[59-66]
  • src/node/hooks/express/webaccess.ts[128-132]

Suggested fix

  • Change the request in adminCookieHeader() to hit /admin-auth/ (GET is used elsewhere in tests; POST is also acceptable) with Basic Auth.
  • Assert that the response indicates success (e.g., HTTP 200) and that at least one cookie is returned; otherwise fail fast with a clear error message.
[correctness] Brace splice misses comments
Brace splice misses comments The marker injection uses `originalRaw.replace(/^(\s*\{)/, ...)`, which only matches if the file begins with `{` after whitespace. In the Docker image, `settings.json` is copied from `settings.json.docker`, which starts with comment blocks, so the splice becomes a no-op and the test fails its own `assert.notEqual()`.

Issue description

The test tries to inject a top-level ep_oauth block by replacing only an opening { at the very start of the file. Docker’s settings.json begins with comment blocks (copied verbatim from settings.json.docker), so the regex never matches and the injection does not happen.

Issue Context

The /settings socket load path emits the raw settings.json file contents from disk, including comments.

Fix Focus Areas

  • src/tests/container/specs/api/adminSettings_7819.ts[89-99]
  • settings.json.docker[1-12]
  • Dockerfile[46-47]
  • Dockerfile[126-128]
  • src/node/hooks/express/adminsettings.ts[55-71]

Suggested fix

  • Replace the anchored regex with something that finds the opening brace of the actual JSON object even when there are leading comments.
  • Example approach: match the first { that appears at the start of a line: /(^|\n)(\s*\{)/ and inject after $2.
  • Avoid a naive indexOf('{') because { can appear inside comment examples.
  • Keep the existing assert.notEqual(...) to ensure the injection actually happened.
                     PR 7798 (2026-05-17)                    
[correctness] Stale query overwrites filter
Stale query overwrites filter `PadPage` writes `filter` into `searchParams`, but multiple updates use `setSearchParams({...searchParams, ...})` with a captured (stale) `searchParams`. A pending debounced pattern update can run after a chip click/sort/page change and overwrite the newer `filter/offset`, causing the server to re-fetch the wrong universe.

Issue description

searchParams is updated via object spread from a render-time snapshot (e.g., in setFilter), while pattern changes are applied via a delayed debounce callback that also spreads from its own captured snapshot. Because filter now lives inside searchParams, the delayed update can unintentionally revert a chip selection (and/or pagination offset) and trigger a wrong padLoad request.

Issue Context

This became user-visible due to moving the filter chip state into searchParams to round-trip through the server.

Fix Focus Areas

  • Use functional updaters for every setSearchParams call so updates merge against the latest state.
  • Consider resetting pagination (offset + currentPage) when pattern, sortBy, or filter changes.
  • file/path[start_line-end_line]
  • admin/src/pages/PadPage.tsx[62-116]
  • admin/src/pages/PadPage.tsx[269-287]
  • admin/src/pages/PadPage.tsx[430-446]
[reliability] Unbounded pad hydration
Unbounded pad hydration The `/settings` socket `padLoad` handler hydrates metadata for every candidate pad using `Promise.all(candidateNames.map(loadMeta))` when `filter !== 'all'` or `sortBy !== 'padName'`. On large deployments this can trigger thousands of concurrent `padManager.getPad()` + DB reads, risking resource saturation and request timeouts.

Issue description

hydrateAll() uses Promise.all() across the entire candidate pad list, which can create unbounded parallelism. Each loadMeta() calls padManager.getPad() and pad.getLastEdit(), which hit the database; doing this for thousands of pads concurrently can overload the DB/event loop.

Issue Context

This path is taken whenever a non-name sort is requested (including the current default lastEdited) or any filter chip other than all is selected.

Fix Focus Areas

  • Replace Promise.all(candidateNames.map(loadMeta)) with a concurrency-limited mapper (e.g., p-limit, async.mapLimit, or a custom pool) tuned for your DB.
  • Keep behavior identical (same outputs), but bound in-flight getPad/getLastEdit calls.
  • file/path[start_line-end_line]
  • src/node/hooks/express/adminsettings.ts[135-152]
  • src/node/db/PadManager.ts[109-144]
  • src/node/db/Pad.ts[352-357]
[reliability] Test admin user persists
Test admin user persists The new backend spec adds `settings.users['test-admin']`, but the suite teardown only reassigns `settings.users` back to the previously saved reference, which does not remove the injected key if `settings.users` was already an object. This can leak credentials/state into later backend specs.

Issue description

The test mutates the global Settings singleton by inserting a test-admin user. The after() hook restores settings.users by reassigning the same object reference saved earlier, so the inserted key remains if the original settings.users object existed.

Issue Context

This can cause cross-test contamination in backend suites that share the same Node process.

Fix Focus Areas

  • Snapshot settings.users via deep clone (or at least shallow clone) before mutation, then restore from the clone.
  • Alternatively, explicitly delete settings.users['test-admin'] in after() (and restore requireAuthentication as you already do).
  • file/path[start_line-end_line]
  • src/tests/backend/specs/admin/padLoadFilter.ts[19-31]
  • src/tests/backend/specs/admin/padLoadFilter.ts[100-146]
                     PR 7797 (2026-05-17)                    
[correctness] Regression test misses bug
Regression test misses bug The new Playwright test uses `data-l10n-id = 'pad.toolbar.bold.title'`, which makes `translateNode()` translate the `title` property (prop != 'textContent') and bypasses the textContent/text-node hunt path where the "could not translate element content" warning occurs. This test therefore won’t fail on the pre-fix behavior and won’t prevent regressions of the SELECT short-circuit logic.

Issue description

The new regression test intends to cover the warning emitted when translating a <select data-l10n-id="…"> with only <option> children, but it uses a l10n key that ends with .title. In Html10n.translateNode(), keys ending in .title translate the element’s title property and do not enter the prop === 'textContent' branch where the warning happens.

Issue Context

To reproduce the warning reliably, the test must use a translation key whose last segment is not one of the special attribute names (title, innerHTML, alt, textContent, value, placeholder), so prop defaults to textContent.

Fix Focus Areas

  • src/tests/frontend-new/specs/html10n_form_controls_aria.spec.ts[73-95]
  • src/static/js/vendors/html10n.ts[643-710]
[reliability] Flaky timeout-based waiting
Flaky timeout-based waiting The test uses a fixed 50ms sleep to “give html10n a tick”, but `html10n.localize()` performs work in an asynchronous callback after `build()`, so the warning could be emitted after the sleep on slow CI. This can cause nondeterministic failures or false passes.

Issue description

The test relies on waitForTimeout(50) instead of synchronizing on a deterministic signal that localization has finished.

Issue Context

html10n.localize() calls build(..., cb) and performs translation inside the callback, so the timing depends on loader/cache and event loop scheduling.

Fix Focus Areas

  • src/tests/frontend-new/specs/html10n_form_controls_aria.spec.ts[88-95]
  • src/static/js/vendors/html10n.ts[474-509]
                     PR 7796 (2026-05-17)                    
[reliability] `hasEpHashAuth` skips admin tests
`hasEpHashAuth` skips admin tests The new `before` hook calls `this.skip()` when `ep_hash_auth` is installed, making the anonymizeAuthorSocket suite pending and preventing validation of the `/settings` admin socket emit/reply behavior in the with-plugins CI matrix. This both violates the requirement that all 7 tests complete successfully with plugins enabled (without hanging) and removes the intended regression coverage for the underlying `/settings` hang.

Issue description

The anonymizeAuthorSocket test suite is currently skipped when ep_hash_auth is installed, which avoids the /settings hang by bypassing test coverage rather than ensuring the /settings admin socket emit/reply round-trip works correctly with plugins enabled. This results in pending tests in the with-plugins CI matrix instead of 7 successful, non-pending test completions, and it removes the regression test signal needed to catch the hang if it persists or returns.

Issue Context

Compliance requires the /settings socket emit/reply round-trip to complete successfully with plugins (including ep_hash_auth) enabled, and for the anonymizeAuthorSocket test suite to run and pass in the with-plugins CI matrix without hanging. Current comments indicate the /settings emit/reply can hang until Mocha timeouts when ep_hash_auth is installed; a proper fix should keep the suite enabled under this plugin configuration and add/retain a test that would fail on the buggy hang and pass after the fix.

Fix Focus Areas

  • src/tests/backend/specs/admin/anonymizeAuthorSocket.ts[79-102]
  • src/node/hooks/express/socketio.ts[115-144]
  • src/node/handler/SocketIORouter.ts[80-92]
[reliability] Teardown uses undefined backups
Teardown uses undefined backups If `hasEpHashAuth` is true, the suite’s `before()` calls `this.skip()` and returns before initializing `originalFlag`, `savedUsers`, and `savedRequireAuthentication`, but `after()` unconditionally restores Settings from those variables. This can set `settings.gdprAuthorErasure.enabled`, `settings.users`, and `settings.requireAuthentication` to `undefined`, breaking later tests in the same mocha process.

Issue description

The suite’s before() can exit early via this.skip() when hasEpHashAuth is true, which bypasses initialization of backup variables used by after(). The after() hook then restores Settings from undefined values, potentially corrupting global test state.

Issue Context

This test file mutates settings.gdprAuthorErasure.enabled, settings.users, and settings.requireAuthentication during setup, and relies on module-scoped backups to restore them in after(). With the new early-return skip path, those backups are never assigned.

Fix Focus Areas

  • Add a guard in after() to no-op when the suite was skipped (e.g., if (hasEpHashAuth) return; or a didSetup flag set after backups are captured).
  • Alternatively, initialize backup variables before the skip decision (without mutating Settings) so that after() can safely restore. Fix focus areas:
  • src/tests/backend/specs/admin/anonymizeAuthorSocket.ts[79-113]
                     PR 7794 (2026-05-17)                    
[reliability] Brittle path stripping
Brittle path stripping `seen` is derived by stripping `${srcRoot}${sep}` via `startsWith()` and then replacing only the current platform separator, so if Mocha prints paths with different separators or drive-letter casing on Windows, the prefix will not be removed and the test will fail with absolute paths in `seen`. This undermines the PR’s goal of making the assertions stable across Linux/Windows runners.

Issue description

The test normalizes Mocha’s --list-files output by doing a string startsWith(prefix) check and then replacing only path.sep. This can fail on Windows if the output uses different separators (e.g., /) or if drive-letter casing differs, leaving absolute paths that break the toContain() assertions.

Issue Context

The logic currently does:

  • build prefix = ${srcRoot}${sep}`
  • strip prefix with startsWith(prefix)
  • convert separators with split(sep).join('/')

Fix Focus Areas

  • src/tests/backend-new/specs/backend-tests-glob.test.ts[51-56]

Suggested implementation direction

  • Normalize each line with path.normalize(l).
  • Convert to repo-relative using path.relative(srcRoot, normalized) when the path is absolute.
  • Finally convert to POSIX separators (e.g., rel.split(path.sep).join('/')). This avoids depending on exact string prefix/casing/separator matches.
                     PR 7793 (2026-05-17)                    
[maintainability] `listAuthorsOfPad` filtering undocumented
`listAuthorsOfPad` filtering undocumented `listAuthorsOfPad` now filters out the synthetic `Pad.SYSTEM_AUTHOR_ID` from the returned `authorIDs`, changing public HTTP API behavior. The HTTP API documentation under `doc/api/http_api.*` is not updated to reflect this, risking client confusion and incorrect integrations.

Issue description

listAuthorsOfPad behavior changed to exclude the synthetic Pad.SYSTEM_AUTHOR_ID (a.etherpad-system), but the public HTTP API documentation still states it returns authors who contributed to the pad without noting this exclusion.

Issue Context

This PR intentionally hides the system author from the public API surface by filtering it out in src/node/db/API.ts. Documentation in doc/ should be updated in the same PR to match the new behavior.

Fix Focus Areas

  • doc/api/http_api.md[696-704]
  • doc/api/http_api.adoc[652-661]
                     PR 7792 (2026-05-17)                    
[reliability] Non-`apierror` export errors unhandled
Non-`apierror` export errors unhandled The export route only converts `apierror` exceptions into a deterministic plain-text response; all other export errors are passed to `next()` and can still fall back to Express's default HTML error page. This can reintroduce non-deterministic HTML error bodies for other export failures.

Issue description

The export route only special-cases err.name === 'apierror' and calls next(err) for everything else, which can still trigger Express's default HTML error renderer.

Issue Context

Compliance requires explicit error handling for export-route failures so clients/tests get deterministic, non-HTML error bodies (at minimum containing err.message) rather than Express's default HTML page.

Fix Focus Areas

  • src/node/hooks/express/importexport.ts[74-82]
[correctness] Error downloads as attachment
Error downloads as attachment If `checkValidRev()` throws, `ExportHandler.doExport()` has already set `Content-Disposition: attachment`, so the new plain-text 500 body can still be delivered as a downloadable file rather than a visible error. This makes the surfaced error message harder to notice and can confuse callers about whether an export succeeded.

Issue description

ExportHandler.doExport() calls res.attachment(...) before validating req.params.rev. If checkValidRev() throws, the new apierror catch returns a plain-text 500 but keeps Content-Disposition: attachment, so clients (especially browsers) may download the error body as an export file.

Issue Context

  • res.attachment() is invoked before checkValidRev().
  • The route-level catch sends the error message but does not clear Content-Disposition.

Fix Focus Areas

  • src/node/hooks/express/importexport.ts[74-82]
  • src/node/handler/ExportHandler.ts[58-65]

Implementation notes

Prefer one of:

  1. Move rev validation (checkValidRev) before res.attachment(...) in ExportHandler.doExport() so invalid :rev never sets download headers.
  2. Or, in the apierror catch branch, call res.removeHeader('Content-Disposition') (and optionally clear any previously-set export Content-Type) before sending the plain-text 500.
                     PR 7777 (2026-05-16)                    
[correctness] History toolbar i18n regression
History toolbar i18n regression #history-controls now uses aria-labelledby, which overrides the aria-label that pad_mode.ts sets from the already-translated pad.historyMode.controlsLabel key. Because the new pad.editor.toolbar.history key currently exists only in en.json, non-English locales will likely announce the English fallback instead of the existing localized label.

Issue description

#history-controls previously got a localized accessible name via pad_mode.ts setting aria-label from pad.historyMode.controlsLabel. This PR adds aria-labelledby="editbar-history-label", which takes precedence over aria-label and points to a new data-l10n-id="pad.editor.toolbar.history" span. In the current repo state, pad.editor.toolbar.history exists only in en.json, while pad.historyMode.controlsLabel is already translated in many locales. This means non-English locales will regress from localized naming to an English fallback.

Issue Context

  • aria-labelledby overrides aria-label in accessible name computation.
  • The repo already contains translations for pad.historyMode.controlsLabel in multiple locale files.
  • The new history-toolbar key is currently only present in English.

Fix Focus Areas

  • src/templates/pad.html[87-94]
  • src/templates/pad.html[116-117]
  • src/static/js/pad_mode.ts[97-120]
  • src/locales/en.json[365-368]

Recommended fix

Prefer reusing the already-translated history controls label key:

  1. Change #editbar-history-label to use data-l10n-id="pad.historyMode.controlsLabel" (and update its fallback text accordingly, e.g. "Pad history controls").
  2. Keep aria-labelledby="editbar-history-label" on #history-controls.
  3. Update the Playwright expectation for the history toolbar label to match the new (reused) string.

Alternative fix

If you need the specific new phrasing, add pad.editor.toolbar.history to all locale JSON files (or ensure your translation import pipeline will populate them at the same time as this change).

                     PR 7776 (2026-05-16)                    
[maintainability] `settings.loadTest` block misindented
`settings.loadTest` block misindented New code in `Settings.ts` uses 4+ space indentation rather than the required 2-space indentation. This introduces formatting inconsistency and violates the no-tabs/2-space indentation standard for modified lines.

Issue description

Modified lines in src/node/utils/Settings.ts use indentation that is not 2-space (e.g., the added if (settings.loadTest) block is indented with 4+ spaces). The compliance checklist requires spaces-only and 2-space indentation for all changed/added code.

Issue Context

This PR adds a startup logger.warn(...) when settings.loadTest is enabled. The logic is fine, but the indentation of the newly added lines does not match the 2-space requirement.

Fix Focus Areas

  • src/node/utils/Settings.ts[1196-1200]
[observability] LoadTest warning scope wrong
LoadTest warning scope wrong The new `Settings.reloadSettings()` warning says only socket.io authn/authz is bypassed, but `settings.loadTest` short-circuits `SecurityManager.checkAccess()` which is used by both socket.io and HTTP handlers. This warning therefore understates the security-impact surface area of enabling `loadTest`.

Issue description

The warning message for settings.loadTest currently claims that only socket.io authentication/authorization checks are bypassed. In reality, settings.loadTest affects SecurityManager.checkAccess(), which is used by both socket.io message handling and HTTP routes (e.g., pad access and import/export).

Issue Context

Accurate operator-facing warnings matter for a security-bypass setting like loadTest. The current wording can mislead an operator into thinking the bypass is limited to socket.io.

Fix Focus Areas

  • src/node/utils/Settings.ts[1196-1200]
  • src/node/db/SecurityManager.ts[60-101]
  • src/node/handler/PadMessageHandler.ts[496-508]
  • src/node/padaccess.ts[6-14]
  • src/node/hooks/express/importexport.ts[77-92]

Suggested fix

Adjust the warning string to reflect the real scope, e.g.:

  • "settings.loadTest is true: authentication/authorization checks in SecurityManager.checkAccess() will be bypassed (affects both HTTP and socket.io). Do NOT enable this in production." Optionally mention that it applies to pad access checks generally, not a single transport.
[maintainability] Misleading startup-only comment
Misleading startup-only comment `SecurityManager.checkAccess()` now states the loadTest warning "fires once at startup", but the warning is emitted from `reloadSettings()`, which is also called on admin restart and after plugin install. This inaccurate comment can mislead future maintainers about when the warning will appear.

Issue description

A comment in SecurityManager.checkAccess() claims the settings.loadTest warning "fires once at startup", but the warning is logged from reloadSettings(), which can be called multiple times during runtime (admin restart, plugin operations). This makes the comment factually incorrect.

Issue Context

The PR moved the warning from the per-message hot path to settings reload time, which is good. However, the comment now over-specifies behavior that isn’t true.

Fix Focus Areas

  • src/node/db/SecurityManager.ts[77-85]
  • src/node/utils/Settings.ts[1192-1201]
  • src/node/hooks/express/adminsettings.ts[395-401]
  • src/static/js/pluginfw/installer.ts[29-35]

Suggested fix

Update the comment to something like: "the warning is logged during settings load/reload (see Settings.ts), not per request" (or remove the timing claim entirely).

                     PR 7773 (2026-05-16)                    
[correctness] Wrong author in roSocket
Wrong author in roSocket The USER_CHANGES test helper always builds the apool using `authorId` from the first socket’s CLIENT_VARS, but `roSocket` can have a different author identity because `common.connect()` only forwards cookies from the *current* HTTP response and `/p/:pad` does not re-issue the author-token cookie if it already exists. With the new server validation requiring an `author` attribute on '+' ops, `sendUserChanges(roSocket, ...)` can now be rejected as “changes as another author”, breaking the `permitOnce` test.

Issue description

src/tests/backend/specs/messages.ts defaults the USER_CHANGES apool to the main socket’s authorId for all sockets. But roSocket can end up with a different authorId (no token cookie forwarded on its socket.io connection), so sendUserChanges(roSocket, '...*0+...') may be rejected by the new server-side validation.

Issue Context

  • authorId is captured only from the first socket’s CLIENT_VARS; the roSocket CLIENT_VARS is ignored.
  • common.connect(res) forwards only cookies present in the provided HTTP response’s set-cookie headers.
  • /p/:pad uses ensureAuthorTokenCookie(), which does not set a Set-Cookie header if the request already has a valid token cookie—so the second GET (to the read-only URL) often won’t provide the token cookie to common.connect().

Fix Focus Areas

  • src/tests/backend/specs/messages.ts[26-46]
  • src/tests/backend/specs/messages.ts[171-179]
  • src/tests/backend/specs/messages.ts[255-270]

Suggested fix

  1. Capture roAuthorId from common.handshake(roSocket, roPadId) (read CLIENT_VARS).
  2. Change authorPool() to accept an explicit authorId (or create two pools: one for socket, one for roSocket).
  3. Ensure sendUserChanges(roSocket, ...) uses roAuthorId’s pool (or explicitly pass an apool argument in the roSocket test).
                     PR 7771 (2026-05-16)                    
[reliability] Restart triggered on failure
Restart triggered on failure installer.install() now calls its wrapped callback with an error, which decrements the global task counter and triggers onAllTasksFinished() even for failures, causing hooks.aCallAll('restartServer') to run. This can restart Etherpad (disconnecting users) on any failed install attempt, including the intentional EngineIncompatibleError short-circuit where nothing was installed.

Issue description

wrapTaskCb() triggers onAllTasksFinished() whenever the task counter reaches zero, regardless of whether the task completed successfully. With this PR, install() now invokes cb(err) on failures, so failed installs (including preflight incompatibility) can cause a full server restart even though nothing changed.

Issue Context

  • wrapTaskCb() decrements tasks after invoking the callback and calls onAllTasksFinished() when tasks === 0.
  • onAllTasksFinished() calls hooks.aCallAll('restartServer'), which closes and recreates the HTTP(S) server.
  • After this PR, install() calls cb(err) in the catch block, so failures now complete the task and can trigger the restart path.

Fix Focus Areas

  • src/static/js/pluginfw/installer.ts[34-58]
  • src/static/js/pluginfw/installer.ts[182-199]

Suggested approach

  • Track whether a task actually made changes (e.g., needsRestart / didMutate flag set only after successful install/uninstall steps), and only call onAllTasksFinished() when tasks === 0 && needsRestart.
  • For install(), set needsRestart = true only after linkInstaller.installPlugin() and hooks.aCallAll('pluginInstall', ...) succeed.
  • For the engine-incompatibility short-circuit (and other errors), ensure the task counter still decrements, but do not restart.
  • Alternatively, change the wrapper to pass through error state and skip onAllTasksFinished() if the first callback argument is truthy, while still decrementing tasks in a finally-like manner.
[reliability] Registry fetch can hang
Registry fetch can hang fetchPluginEnginesNode() awaits a registry.npmjs.org fetch with no timeout/abort, and install() awaits it before proceeding. A stalled DNS/network connection can therefore hang the entire install path and prevent the finished:install socket event from ever being emitted.

Issue description

fetchPluginEnginesNode() performs a network fetch without any timeout or abort mechanism. If the request stalls (DNS, proxy, captive portal, registry hang), install() will await forever and the admin UI will never receive finished:install.

Issue Context

The comment says the lookup is best-effort and should fall through on failure, but an unbounded await is not a failure and can block indefinitely.

Fix Focus Areas

  • src/static/js/pluginfw/installer.ts[164-190]

Suggested approach

  • Use an AbortController with a short timeout (e.g., 3–10 seconds):
  • Create controller, setTimeout(() => controller.abort(), timeoutMs)
  • Pass {headers, signal: controller.signal} to fetch()
  • In finally, clear the timeout
  • On abort (or any error), return undefined so install continues down the existing path.
  • Optionally log at debug level when the preflight times out to aid diagnosis without spamming logs.
                     PR 7762 (2026-05-15)                    
[maintainability] New Prometheus metrics lack flag
New Prometheus metrics lack flag The PR introduces new Prometheus metrics (`etherpad_pad_users`, `etherpad_changeset_apply_duration_seconds`, `etherpad_socket_emits_total`) that are registered and emitted unconditionally. This violates the requirement that new features be gated behind a feature flag and disabled by default.

Issue description

New Prometheus metrics are always enabled, but compliance requires new features to be behind a feature flag and disabled by default.

Issue Context

This PR registers three new metrics and emits them on the hot path and in the Prometheus monitor loop without any enable/disable mechanism.

Fix Focus Areas

  • src/node/prometheus.ts[26-48]
  • src/node/handler/PadMessageHandler.ts[50-50]
  • src/node/handler/PadMessageHandler.ts[609-609]
  • src/node/handler/PadMessageHandler.ts[630-630]
  • src/node/handler/PadMessageHandler.ts[671-671]
  • src/node/handler/PadMessageHandler.ts[791-791]
  • src/node/handler/PadMessageHandler.ts[904-904]
  • src/node/handler/PadMessageHandler.ts[957-957]
[correctness] Histogram includes fan-out
Histogram includes fan-out handleUserChanges() starts etherpad_changeset_apply_duration_seconds before any processing and only stops it in finally, after updatePadClients() completes, so the histogram includes broadcast/fan-out time rather than isolating the apply path. This prevents the scaling-dive analysis from distinguishing “apply is slow” vs “fan-out is slow”.

Issue description

etherpad_changeset_apply_duration_seconds is intended to time the apply path, but the timer currently spans the entire handleUserChanges() including await exports.updatePadClients(pad) (fan-out). This makes the metric misleading and defeats the PR’s stated purpose.

Issue Context

  • The timer is started near the beginning of handleUserChanges() and stopped in finally.
  • updatePadClients() is awaited before the timer is stopped, so its work is included in the duration.

Fix Focus Areas

  • src/node/handler/PadMessageHandler.ts[788-905]

Implementation notes

  • Move recordChangesetApply() start to immediately before the “apply” work you want to measure (e.g., just before pad.appendRevision(...)).
  • Call the returned stopHistogram() immediately after the apply work completes and before any socket emits / updatePadClients() fan-out.
  • If you also want total end-to-end latency, keep using the existing stats.timer('edits') (or add a second histogram explicitly for fan-out/total).
[reliability] Unbounded metrics label
Unbounded metrics label handleCustomMessage() increments etherpad_socket_emits_total with msgString (an HTTP API parameter) as the label value, allowing an API caller to generate unbounded distinct label values and potentially exhaust memory/CPU via high-cardinality time series. Although the endpoint is API-key protected, this still creates a sharp footgun if the key is leaked or a client misbehaves.

Issue description

etherpad_socket_emits_total{type=...} uses the message type as a Prometheus label. In handleCustomMessage(), that type comes directly from the HTTP API msg parameter, which means label cardinality is unbounded.

Issue Context

  • sendClientsMessage(padID, msg) passes msg directly into PadMessageHandler.handleCustomMessage(padID, msg).
  • handleCustomMessage() sets data.type = msgString and then calls recordSocketEmit(msg.data.type).
  • recordSocketEmit() uses socketEmitsTotal.labels(type).inc(), creating a new time series per distinct type.

Fix Focus Areas

  • src/node/handler/PadMessageHandler.ts[620-631]
  • src/node/db/API.ts[863-866]
  • src/node/prom-instruments.ts[17-21]
  • src/node/prom-instruments.ts[34-36]

Implementation notes

Pick one of:

  1. Normalize/bucket: Map any unknown/untrusted type to a small bounded set (e.g., API_CUSTOM_MESSAGE, CUSTOM, other, unknown).
  2. Allowlist: Only allow a fixed set of label values (e.g., NEW_CHANGES, CHAT_MESSAGE, etc.); everything else becomes other.
  3. Remove label at this site: For handleCustomMessage(), increment with a constant label regardless of msgString. Also consider enforcing a max length/character set if you keep any user-provided label value.
                     PR 7757 (2026-05-15)                    
[correctness] Promise sent to admin
Promise sent to admin In the admin plugin socket handler, `checkUpdates` emits the unresolved Promise returned by `checkPluginForUpdates()` because it is not awaited, so the admin UI will throw when it calls `.includes()` on `data.updatable`. Additionally, the error path emits `{}` for `updatable`, which also violates the UI's `string[]` expectation and can throw on transient errors.

Issue description

The checkUpdates socket event emits an unresolved Promise (missing await) and emits the wrong type ({}) on error. This breaks the admin plugins page because the client expects updatable to be a string[] and calls .includes().

Issue Context

  • checkPluginForUpdates is async.
  • Admin UI (HomePage.tsx) assumes updatable: string[].

Fix Focus Areas

  • src/node/hooks/express/adminplugins.ts[74-82]
  • admin/src/pages/HomePage.tsx[66-70]
                     PR 7755 (2026-05-15)                    
[security] Unsafe cookie decoding
Unsafe cookie decoding PadMessageHandler.handleMessage() decodes untrusted handshake Cookie values with decodeURIComponent() without handling URIError, so a malformed cookie (e.g. sessionID=%ZZ) will throw and abort CLIENT_READY processing for that socket. This enables repeated connection attempts to spam server error logs and prevent clients from joining pads.

Issue description

readCookie() uses decodeURIComponent() on raw Cookie header values without error handling. If the cookie value contains malformed percent-encoding, decodeURIComponent() throws a URIError, causing CLIENT_READY to fail and generating server error logs.

Issue Context

Cookie headers are client-controlled; malicious or buggy clients can supply invalid encodings. The error propagates up to the socket message handler, which logs the exception.

Fix Focus Areas

  • src/node/handler/PadMessageHandler.ts[406-434]

Suggested fix

  • Wrap decodeURIComponent(...) in a try/catch and treat decode failures as null (or fall back to the raw string).
  • Consider rate-limiting or lowering log severity for repeated decode failures to reduce log amplification.
  • Add a backend regression test that connects with a malformed cookie value (for sessionID and/or token) and asserts CLIENT_READY does not throw and auth.sessionID resolves to null (or expected fallback).
                     PR 7753 (2026-05-15)                    
[correctness] Misleading deferred subtitle
Misleading deferred subtitle `UpdatePage` renders the "Outside maintenance window" subtitle for autonomous tier whenever `scheduledFor` is more than 60 seconds away, which is also true for a normal in-window grace period (e.g., 15 minutes), so the UI can incorrectly claim the delay is due to being outside the window.

Issue description

UpdatePage currently shows the maintenance-window deferral subtitle based on a fixed scheduledFor > now + 60s heuristic. This misfires when the update is scheduled inside the maintenance window but has a grace period longer than 60s (common per docs), leading to incorrect admin messaging.

Issue Context

Backend Tier 4 only snaps scheduledFor to the next window opening if now + grace is outside the window; otherwise it keeps scheduledFor = now + grace. The status endpoint also always provides nextWindowOpensAt for autonomous tier (when the window parses), so the UI needs a stronger signal than “scheduledFor is far away”.

Fix Focus Areas

  • admin/src/pages/UpdatePage.tsx[186-207]
  • src/node/updater/Scheduler.ts[79-88]
  • src/node/hooks/express/updateStatus.ts[115-119]
  • doc/admin/updates.md[202-208]

Suggested implementation direction

Change the subtitle condition to reflect actual deferral, e.g. only show it when scheduled.scheduledFor is effectively the next window opening (compare equality to us.nextWindowOpensAt, possibly with a small tolerance), rather than using a fixed > now + 60s threshold.

[correctness] Preflight detail dropped
Preflight detail dropped `applyUpdate()` saves a detailed preflight failure reason (`reasonStr`) to state and logs but returns only `pf.reason`, causing `/admin/update/apply` responses and failure-notification emails to lose the detail that was just computed.

Issue description

On preflight failure, applyUpdate() computes reasonStr (including optional pf.detail) and persists it, but returns {reason: pf.reason}. Callers (HTTP route + notify path) use the returned result.reason, so they miss the detail string.

Issue Context

  • State/logs store the enriched string, so the admin UI eventually shows it.
  • Immediate HTTP 409 responses and notifyApplyFailure() emails use the returned value and therefore lose important diagnostics (e.g., Node engine mismatch details).

Fix Focus Areas

  • src/node/updater/applyPipeline.ts[89-103]
  • src/node/hooks/express/updateActions.ts[274-301]
  • src/node/updater/Notifier.ts[156-162]

Suggested implementation direction

  • Change applyUpdate() to return reason: reasonStr for preflight-failed.
  • Ensure any downstream callers that surface result.reason (HTTP + email) now get the enriched detail.
[reliability] SMTP cache misses config
SMTP cache misses config `sendEmailViaSmtp()` only rebuilds the cached nodemailer transport when `settings.mail.host` changes, so runtime changes to `port`/`secure`/`auth` after `reloadSettings()` will keep using a stale transport configuration.

Issue description

The cached nodemailer transport is invalidated only on host changes. If an operator updates SMTP credentials or port (without changing host) and triggers reloadSettings(), future emails will continue using the old transport settings.

Issue Context

The admin settings flow can call reloadSettings() at runtime, so mail settings are expected to be mutable without a process restart.

Fix Focus Areas

  • src/node/updater/index.ts[46-78]
  • src/node/hooks/express/adminsettings.ts[395-401]

Suggested implementation direction

  • Expand the cache key to include host, port, secure, and a stable representation of auth (and optionally from).
  • Alternatively, clear transportCache whenever reloadSettings() runs (if there is a suitable hook point), forcing a rebuild on next send.
                     PR 7750 (2026-05-15)                    
[reliability] No test for plugin_packages migration
No test for plugin_packages migration This PR changes Debian postinstall/systemd behavior for `src/plugin_packages` but does not add/update an automated regression test to validate the new directory layout and upgrade migration. Without a test, future packaging changes could reintroduce the `MODULE_NOT_FOUND` failure or break upgrades unnoticed.

Issue description

The PR fixes a Debian packaging bug by keeping src/plugin_packages in-tree and adjusting permissions/systemd sandboxing, but there is no updated regression test to enforce the new behavior.

Issue Context

packaging/test-local.sh currently asserts the old symlink-based layout for /opt/etherpad/src/plugin_packages, so it will not validate (and will likely fail under) the new intended behavior.

Fix Focus Areas

  • packaging/test-local.sh[133-150]
  • packaging/scripts/postinstall.sh[69-103]
  • packaging/systemd/etherpad.service[43-51]
[correctness] CI symlink assertions stale
CI symlink assertions stale packaging/scripts/postinstall.sh no longer creates /opt/etherpad/src/plugin_packages as a symlink, but the Debian CI workflow and packaging/test-local.sh still assert it is a symlink to /var/lib/etherpad/plugin_packages, causing CI/test failures.

Issue description

postinstall.sh now migrates away from a symlinked src/plugin_packages and ensures it is a real directory. CI and local packaging tests still check for the old symlink layout, so they will fail.

Issue Context

The deb packaging validation in .github/workflows/deb-package.yml and packaging/test-local.sh must be updated to match the new in-tree directory layout and permissions.

Fix Focus Areas

  • .github/workflows/deb-package.yml[170-179]
  • packaging/test-local.sh[133-146]

What to change

  • Replace test -L /opt/etherpad/src/plugin_packages + readlink assertions with test -d /opt/etherpad/src/plugin_packages.
  • Replace /var/lib/etherpad/plugin_packages existence/ownership checks with permission/group checks on /opt/etherpad/src/plugin_packages (and optionally /opt/etherpad/src/plugin_packages/.versions if you decide to pre-create it), consistent with postinstall.sh (group=etherpad, mode 2775).
                     PR 7748 (2026-05-15)                    
[reliability] Mocha --exit masks handle leaks
Mocha --exit masks handle leaks Adding `--exit` forces the test process to terminate even if timers/sockets/handles are still open, so backend tests will no longer hang or otherwise surface leaked handles during the post-suite event-loop drain. This weakens regression detection for known classes of leaks (for example, socket.io-client reconnect timers) and can let new leaks slip by silently.

Issue description

mocha --exit improves flake mitigation, but it also prevents the test runner from naturally exposing leaked handles (timers/sockets) via a non-draining event loop.

Issue Context

At least one backend spec explicitly calls out that leaked socket.io-client reconnect timers keep the event loop alive past Mocha’s “passing” output; --exit would make similar future leaks much harder to detect.

Fix Focus Areas

  • src/package.json[149-152]
  • src/tests/backend/specs/lowerCasePadIds.ts[20-26]

Suggested fix options

  • Make --exit conditional (e.g., only in CI, or only for Windows+Node24) via an env var in the workflow and a small script wrapper.
  • If --exit must remain on, add an explicit leak-detection phase (e.g., fail the run if known handle types remain, or run an open-handle dump right before forced exit) so regressions are still caught.
[observability] Diagnostics exit matrix misleading
Diagnostics exit matrix misleading Backend tests now run Mocha with `--exit`, so successful runs will typically skip `beforeExit` and always emit only `exit`, but `tests/backend/diagnostics.ts` documents “only exit” as meaning `process.exit()` was called unexpectedly. This makes the diagnostics output/documentation inaccurate and can mislead future debugging of backend-test failures.

Issue description

Mocha is now invoked with --exit, which changes the meaning of the beforeExit/exit event pattern that src/tests/backend/diagnostics.ts documents and uses for interpretation.

Issue Context

With --exit, Mocha calls process.exit(...) at the end of the run. That means beforeExit usually won’t run on success, and “only exit” becomes the expected behavior, contradicting diagnostics.ts’s current matrix.

Fix Focus Areas

  • src/tests/backend/diagnostics.ts[23-27]
  • src/tests/backend/diagnostics.ts[66-73]
  • src/package.json[149-152]

Suggested fix

  • Update the comment matrix (and optionally the log output) to explicitly account for --exit.
  • Optionally, have diagnostics.ts detect --exit (e.g., via process.argv.includes('--exit')) and log an extra line like diag('mocha --exit enabled; beforeExit will not fire on success') so the runner log remains self-explanatory.
                     PR 7727 (2026-05-11)                    
[reliability] `CMD` change lacks regression test
`CMD` change lacks regression test The PR changes the container startup path to bypass `pnpm run prod`, but it does not add a regression test that would fail if the old `pnpm`-based `CMD` were restored. Without an automated reproduction (e.g., the `docker-compose.yml` named-volume layout on `src/plugin_packages`), this bug can silently regress.

Issue description

This PR fixes a Docker runtime crash by bypassing pnpm run prod, but it does not include a regression test to ensure the container still boots under the problematic volume layout (notably the named volume mounted to /opt/etherpad-lite/src/plugin_packages).

Issue Context

The changed CMD in Dockerfile is intended to avoid pnpm 11's runDepsStatusCheck behavior at runtime. A regression test should exercise the same startup conditions (ideally including the docker-compose.yml named-volume layout) so reverting the fix would cause CI to fail.

Fix Focus Areas

  • Dockerfile[217-228]
  • .github/workflows/docker.yml[66-82]
  • docker-compose.yml[2-30]
                     PR 7726 (2026-05-11)                    
[observability] Missing nginx logs on timeout
Missing nginx logs on timeout On timeout the step only prints `etherpad-docker` logs, but the polled endpoint is the nginx container on 8081 and that container is started without a name. If nginx is the problem, the workflow won’t capture its logs, slowing triage.

Issue description

The timeout handler dumps only etherpad logs, but failures at http://127.0.0.1:8081/ can be caused by nginx itself (crash, bad config, network). Because nginx is started without --name, it’s awkward to reliably capture its logs during failure.

Issue Context

Nginx is the container bound to host port 8081 and proxies to etherpad.

Fix Focus Areas

  • .github/workflows/rate-limit.yml[58-84]

Suggested fix

  1. Start nginx with a stable name, e.g. docker run --name nginx-docker -p 8081:80 ... -d nginx-latest.
  2. On readiness timeout, print both:
  • docker logs nginx-docker || true
  • docker logs etherpad-docker || true Optionally add docker ps -a for quick container state visibility.
[performance] Wait can reach 4m
Wait can reach 4m The readiness poll can delay job failure up to ~240s because each of the 60 iterations includes a curl with `--max-time 2` plus `sleep 2`. This unnecessarily prolongs CI runs when etherpad never becomes ready.

Issue description

The readiness loop’s wall-clock upper bound is ~4 minutes (60 * (2s curl timeout + 2s sleep)), which can slow down failure feedback.

Issue Context

This is the new “Wait for etherpad behind nginx to be ready” step.

Fix Focus Areas

  • .github/workflows/rate-limit.yml[66-84]

Suggested fix

Use a single wall-clock timeout (e.g., timeout 120s bash -c 'until curl ...; do sleep 2; done') or reduce either the per-try curl timeout or the sleep interval so the maximum wait matches your intended bound (for example, 120s total). Also consider printing the final HTTP status/connection error to make timeouts easier to interpret.

                     PR 7720 (2026-05-11)                    
[reliability] Timer skips state recheck
Timer skips state recheck The scheduler timer’s fire path calls applyUpdate() without re-checking persisted execution state or current policy, so an update can start even if the admin cancels at the timer boundary or the policy/tier changes during the grace window. This contradicts the SchedulerRunnerDeps contract comment that triggerApply should re-check state before acting.

Issue description

The scheduler timer fire path (triggerApply) does not verify that:

  1. the persisted state is still scheduled for the same tag, and
  2. policy still allows auto-apply. This can cause updates to start despite last-moment cancellation or a tier/policy flip during the grace window.

Issue Context

  • SchedulerRunnerDeps.triggerApply is documented to re-check persisted state.
  • The production wiring in index.ts currently calls applyUpdate() directly.
  • applyUpdate() validates state shape/tag/lock, but it does not evaluate update policy.

Fix Focus Areas

  • src/node/updater/Scheduler.ts[96-101]
  • src/node/updater/index.ts[320-331]
  • src/node/updater/applyPipeline.ts[48-77]

What to change

In triggerApply (index.ts):

  • Load the latest persisted state.
  • Abort unless state.execution.status === 'scheduled' AND state.execution.targetTag === targetTag.
  • Re-evaluate policy (using current settings + install method + current version) and abort unless policy.canAuto.
  • Optionally, if aborting due to policy denial, clear scheduled state to idle and persist it so the UI doesn’t show a stale scheduled state. This keeps the timer callback safe under races and matches the documented contract.
[reliability] scheduledFor format unchecked
scheduledFor format unchecked State validation for the new scheduled execution status only enforces that scheduledFor is a non-empty string, not a valid timestamp, so a hand-edited state file can pass validation but result in NaN delay calculations and effectively immediate timer firing. This is low-likelihood but easy to harden given scheduled status is newly introduced.

Issue description

execution.status === 'scheduled' requires scheduledFor and startedAt, but state validation only checks they are non-empty strings, not valid timestamps.

Issue Context

  • Scheduler delay uses new Date(scheduledFor).getTime().
  • Invalid date strings yield NaN, which can lead to unintended immediate scheduling behavior.

Fix Focus Areas

  • src/node/updater/state.ts[15-39]

What to change

  • Enhance execution validation for statuses that store timestamps (including scheduled) to verify fields are parseable dates (e.g., !Number.isNaN(Date.parse(value))).
  • Apply the stricter check only to known timestamp fields (scheduledFor/startedAt/drainEndsAt/etc.) to avoid changing semantics for non-timestamp strings like reason. This is defense-in-depth against corrupted/hand-edited state.
                     PR 7716 (2026-05-10)                    
[security] Hardcoded `https://www.npmjs.com`
Hardcoded `https://www.npmjs.com` The new admin UI link uses a protocol-specific external URL (`https://...`) instead of a protocol-independent URL. This violates the checklist requirement to use protocol-independent URLs where applicable to avoid protocol-coupling issues.

Issue description

A new external link hard-codes https:// (https://www.npmjs.com/...) instead of using a protocol-independent URL as required by the compliance checklist.

Issue Context

This URL is used for the npm search link in the admin plugins page.

Fix Focus Areas

  • admin/src/pages/HomePage.tsx[138-145]
[reliability] Plugin listeners not cleaned
Plugin listeners not cleaned HomePage registers multiple Socket.IO handlers inside effects that re-run (e.g., when `searchParams` or the socket instance changes) but only clears an interval on unmount, so listeners are never detached. This allows handlers to fire after unmount and to accumulate over time, causing duplicate state updates and unbounded listener growth.

Issue description

HomePage attaches Socket.IO listeners (e.g., results:installed, results:updatable, results:search, results:searcherror, finished:*, connect) inside React effects, including effects that re-run when searchParams changes, but it does not unregister those listeners in cleanup. Update the component so every registered .on() handler is removed appropriately to prevent duplicate event handling, unbounded listener growth, and handlers firing after unmount.

Issue Context

The current effect cleanup only clears a setInterval, leaving all Socket.IO .on() handlers attached. Because Socket.IO listeners persist until explicitly removed, any effect that re-runs (such as one dependent on searchParams, or one that might re-run if the socket instance changes) will accumulate additional listeners unless off(event, handler) is called, leading to multiple state updates per single server response.

Fix Focus Areas

  • admin/src/pages/HomePage.tsx[58-89]
  • admin/src/pages/HomePage.tsx[91-100]
                     PR 7714 (2026-05-10)                    
[correctness] REST checkToken path changed
REST checkToken path changed `checkToken` was moved into the new `server` resource, which changes its REST-style runtime route from `/rest//pad/checkToken` to `/rest//server/checkToken`. This breaks REST-style backward compatibility (clients will get `code: 3` “no such function”) and contradicts the design note that REST URLs for existing operations should remain unchanged via per-op tag overrides.

Issue description

checkToken now routes under /rest/<ver>/server/checkToken, breaking existing REST-style callers that previously used /rest/<ver>/pad/checkToken.

Issue Context

REST-style paths are computed from the resource/action keys (_restPath = /${resource}/${action}) and used by the runtime OpenAPIBackend router for /rest/`.

Fix Focus Areas

  • src/node/hooks/express/openapi.ts[156-319]

Suggested fix

  • Move checkToken back under the existing pad resource (restoring /pad/checkToken routing), and set tags: ['server'] on that operation to achieve the desired grouping without changing the REST URL.
  • Keep the server resource for truly new server-only operations like getStats.
  • (Optional safety) Add a regression test that /rest/<ver>/pad/checkToken still resolves (similar to the existing /api/<ver>/checkToken assertions).
                     PR 7710 (2026-05-09)                    
[security] Hardcoded https Google Fonts
Hardcoded https Google Fonts External font resources are added using hardcoded `https://` URLs instead of protocol-independent `//` URLs. This can cause mixed-content/compatibility issues in non-HTTPS deployments and violates the project convention.

Issue description

pad.html adds external Google Fonts resources using https:// instead of protocol-independent // URLs.

Issue Context

The compliance checklist requires protocol-independent URLs where appropriate to avoid mixed content issues and improve deployment compatibility.

Fix Focus Areas

  • src/templates/pad.html[24-26]
[maintainability] Timeslider URL change undocumented
Timeslider URL change undocumented The behavior of `/p/:pad/timeslider` has changed (302 redirect unless `?embed=1`), but no documentation updates are included. This makes existing documentation about timeslider routes inaccurate and can break adopters who rely on the documented URL.

Issue description

The timeslider endpoint behavior changed: /p/:pad/timeslider now redirects to the pad unless ?embed=1 is provided, but docs still imply the timeslider is directly served at /p/:padid/timeslider.

Issue Context

This is a public, user- and integrator-facing URL change. Documentation should mention the redirect and provide the correct URL for embedding/direct HTML (?embed=1) with an example.

Fix Focus Areas

  • doc/skins.md[5-10]
  • src/node/hooks/express/specialpages.ts[402-408]
  • src/node/hooks/express/specialpages.ts[420-420]
[correctness] Latest hash opens rev0
Latest hash opens rev0 `PadModeController.onOuterHashChange()` maps `#rev/latest` to revision 0 via `rev < 0 ? 0 : rev`, which forces the embedded timeslider to jump to the first revision instead of the latest. Users manually navigating to `#rev/latest` (or any future link that sets it) will see the wrong historical content.

Issue description

#rev/latest is parsed as -1 but then converted to 0 when syncing the outer hash into the embedded timeslider, which incorrectly jumps to revision 0.

Issue Context

The embedded timeslider interprets window.location.hash = "#N" as the desired revision number.

Fix Focus Areas

  • src/static/js/pad_mode.ts[214-226]
  • src/static/js/pad_mode.ts[196-201]

Implementation notes

  • When rev < 0, clear the inner hash (win.location.hash = "") instead of setting #0, or compute the latest revision number from the embedded frame (if available) and set that.
  • Ensure the outer URL remains canonical (#rev/latest is fine), but don’t mis-target the embedded slider.
[security] External Google Fonts added
External Google Fonts added `pad.html` now unconditionally loads multiple Google Fonts via `fonts.googleapis.com`/`fonts.gstatic.com`, adding third‑party network requests on every pad view. This can break offline/self-hosted deployments and leaks user IP/metadata to a third party by default.

Issue description

The core pad HTML template now hard-depends on Google Fonts, introducing third-party requests and potential breakage for offline/CSP-restricted installations.

Issue Context

Etherpad is commonly self-hosted; loading external fonts by default is a privacy and reliability regression.

Fix Focus Areas

  • src/templates/pad.html[16-27]

Implementation notes

  • Remove the Google Fonts <link> tags entirely, or gate them behind an explicit setting/skin.
  • If these fonts are required for a theme, bundle/self-host them under src/static/ and reference local URLs instead.
[reliability] Redirect embed bypass too loose
Redirect embed bypass too loose The server treats any truthy `?embed=` value as an iframe request and skips the redirect, so direct visits can still access `/p/:pad/timeslider` by adding `?embed=0` or similar. Additionally, the redirect target is built as a relative path containing the padId, which can normalize unexpectedly for edge pad IDs like `..`.

Issue description

/p/:pad/timeslider bypasses the redirect for any truthy embed query value and uses a relative redirect target that can normalize oddly for edge pad IDs.

Issue Context

The embed path is intended only for the in-pad iframe (?embed=1).

Fix Focus Areas

  • src/node/hooks/express/specialpages.ts[230-237]
  • src/node/hooks/express/specialpages.ts[401-408]

Implementation notes

  • Change the condition to something strict like if (req.query.embed !== '1') redirect....
  • Prefer a simple redirect target of .. (from /p/:pad/timeslider this resolves to /p/:pad) or an absolute /p/${encodeURIComponent(req.params.pad)} to avoid composing ../${pad}.
  • Keep behavior consistent in both dev live-reload and production handlers.

General

Resources

For Developers

How to's

Set up

Advanced steps

Integrating Etherpad in your web app

for Developers

Clone this wiki locally