.pr_agent_accepted_suggestions

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

PR 8458 (2026-11-07)

[correctness] Three race tests pass if requests drop

Three race tests pass if requests drop

The new chat-history, changeset-history, and saved-revision tests assert only that the unauthorized pad is unaffected, not that the held request completes against its authorized pad. If those requests are rejected or silently dropped after the raced join, all three assertions still pass despite losing the operation this change is meant to preserve.

PR 8433 (2026-11-04)

[correctness] Security credits list advisories from earlier releases

Security credits list advisories from earlier releases

The 'Thanks to our security reporters' list in the new 3.3.14 section is described as covering reports 'since mid-October', but it re-lists GHSA advisories already credited in the 3.3.3, 3.3.9, 3.3.11 and 3.3.12 entries (for example GHSA-35rj-7vrx-29wq for sun1024, GHSA-746x-9w89-7xv6 for MaBeFHDW, GHSA-g9xm-3pqr-wcph for baeseungwon1010, GHSA-mc8w-wjhw-45x5 for gcm-explo1t, GHSA-pp5v-mvwg-76mp for meifukun). Those advisories were published and fixed in earlier releases, so the section mis-states its time window and repeats credits that were already given. Because this is the public release note that goes out with the release, the wrong window and duplicate credits will be published as-is.

PR 8425 (2026-11-03)

[correctness] Unrelated log edits bypass update checks

Unrelated log edits bypass update checks

`isPorcelainClean` treats every path below `var/log/` as updater-owned instead of limiting the exemption to the update log. Any unrelated tracked or untracked file in that directory is therefore ignored during both HTTP and scheduler preflight checks, allowing Apply to proceed despite a dirty working tree.

Issue description

The clean-tree filter ignores the entire var/log/ directory, although the updater writes only its own update log there. Unrelated files under that directory can therefore bypass the working-tree preflight check.

Fix Focus Areas

src/node/updater/preflight.ts[58-67]

Recommended Fix

Restrict the updater-owned log path to the specific updater log file, such as var/log/update.log, and update the corresponding ignore configuration and tests so unrelated files under var/log/ remain detectable.

[correctness] Acknowledge can clear state during a crash-loop rollback

Acknowledge can clear state during a crash-loop rollback

`stalePending` treats `!isVerificationArmed()` as proof that `pending-verification` is stale, but the `bootCount > 2` crash-loop path also returns `armed: false` after starting `performRollback` without awaiting it. If an admin acknowledges before rollback finishes its checkout and install, the route writes `idle` with `bootCount: 0`, allowing a subsequent Apply whose checkout and install can run concurrently because rollback does not hold `var/update.lock`.

Issue description

The acknowledge route treats pending-verification as stale whenever isVerificationArmed() is false. On the crash-loop path (bootCount > 2), rollback is still running despite an armed: false result, so acknowledgement can reset the state to idle and permit a concurrent Apply.

Fix Focus Areas

src/node/updater/RollbackHandler.ts[160-204]
src/node/updater/index.ts[46-52]
src/node/hooks/express/updateActions.ts[353-365]

Recommended Fix

Track boot rollback activity separately from the health-check timer, for example with a rollingBack flag in CheckResult or a module-level flag set when fireRollback is invoked. Expose a function such as isBootVerificationActive() that covers both an armed timer and an in-progress boot rollback, and allow acknowledgement of pending-verification only when neither is active. Alternatively, persist a non-acknowledgeable rolling-back status before performRollback starts mutating files.

[correctness] Version check can still overwrite a concurrent transition

Version check can still overwrite a concurrent transition

`performCheck` re-reads state after `checkLatestRelease` but saves the entire snapshot, including `execution` and `bootCount`, after awaiting SMTP sends. If `markVerified`, Apply, or Acknowledge persists an execution transition during that wait, the later polling save can replace it with the older status, including restoring `pending-verification` after acknowledgement set `idle`.

Issue description

An in-flight version check can overwrite an execution transition persisted while it awaits SMTP delivery because performCheck later saves an older full-state snapshot.

Fix Focus Areas

src/node/updater/index.ts[133-238]
src/node/hooks/express/updateActions.ts[360-365]
src/node/updater/state.ts[160-174]

Recommended Fix

Serialize the state read-modify-write operation with acknowledgement. Immediately before polling saves, re-load the latest persisted state and merge only the fields performCheck owns (lastCheckAt, latest, lastEtag, and email). Write execution only if the scheduler decision changed it and the fresh execution status still matches the status that decision used; do not write bootCount from the snapshot. Add a test that pauses polling after its read, lets acknowledgement complete, and verifies that polling does not restore the earlier execution status.

PR 8423 (2026-11-03)

[correctness] Pre-registered shutdown handlers run twice

Pre-registered shutdown handlers run twice

`takeOverSignal` adds `listener` unconditionally, then skips every matching entry in `process.listeners(signal)`, leaving duplicate registrations if that function was already subscribed. When the same handler is registered before takeover, one signal invokes it twice instead of once, unlike the previous remove-all-then-add behavior.

Issue description

If the target handler is already registered, takeOverSignal adds a second copy and preserves both, so one signal invokes it twice.

Fix Focus Areas

src/node/utils/takeOverSignal.ts[19-22]
src/tests/backend-new/specs/takeOverSignal.test.ts[15-27]

Recommended Fix

Keep the add-before-remove ordering so the signal handle never closes, but remove duplicate registrations of the target handler until exactly one remains. Add a test that registers the target handler before takeover and verifies both the final listener count and single invocation.

PR 8420 (2026-11-02)

[correctness] Rejected imports leave uploads on disk

Rejected imports leave uploads on disk

`doImport()` rethrows a creator-mismatch error before removing the uploaded `srcFile`. When an authorized user uploads a `.etherpad` file with a different revision-zero creator, the request returns `permission` while the temporary file remains on disk; repeated rejected uploads accumulate there.

Issue description

Creator-mismatch rejections exit the import handler without deleting the uploaded temporary file.

Fix Focus Areas

src/node/handler/ImportHandler.ts[221-226]
src/node/handler/ImportHandler.ts[320-346]

Recommended Fix

Ensure the uploaded source file is removed when setPadRaw() rejects the import, preferably by placing upload cleanup in a finally path that also covers other failures and successful direct-database imports. Preserve the permission response.

PR 8410 (2026-11-01)

[correctness] Playback speed text is cut off

Playback speed text is cut off

The new `text-overflow: ellipsis` is applied to the `.nice-select` wrapper while the selected label is rendered inside its separate `.current` span. When flexbox shrinks the control below the label's natural width, the wrapper clips that span without rendering an ellipsis, leaving users with an abruptly truncated playback-speed value.

PR 8409 (2026-10-31)

[correctness] The race test can pass without the catch-up

The race test can pass without the catch-up

`join()` resolves when it receives `CLIENT_VARS`, but `handleClientReady()` performs the catch-up afterward, so the 300 ms sleep does not establish that the joining handler reached the raced operation. If catch-up takes longer on a loaded runner, the test releases the writer first and can pass even with the original all-socket behavior.

PR 8397 (2026-10-28)

[correctness] The return button can reopen the timeslider

The return button can reopen the timeslider

`getReturnToPadUrl` accepts any same-origin pathname containing `/p/`, including `/p/testpad/timeslider?embed=1`, rather than checking for the current pad page. If that URL is the referrer, the return command follows it unchanged instead of navigating to `/p/testpad`; same-origin export routes also pass the check.

Issue description

The referrer check accepts same-origin timeslider and export URLs as pad pages, so Return to pad can navigate somewhere other than the current pad.

Fix Focus Areas

src/static/js/timesliderReturnUrl.ts[12-24]
src/tests/backend-new/specs/timesliderReturnUrl.test.ts[8-35]

Recommended Fix

Derive the current pad pathname from the timeslider URL and follow a referrer only when its origin and pathname match that pad page, accounting for any supported trailing slash. Preserve legitimate pad-page query strings, and add tests rejecting same-origin timeslider, export, and unrelated-pad referrers.

PR 8389 (2026-10-25)

[correctness] Latest snapshots include later chat

Latest snapshots include later chat

`getPadRaw` enters the new chat-clamping branch only when `revNum < padHead`, although a supplied revision equal to the head is still a revision-specific export. If chat arrives after the last edit without another revision, exporting that revision carries the later messages and leaves `chatHead` unchanged.

Issue description

A supplied revision equal to the pad head bypasses chat clamping, so chat posted after that revision remains in the export.

Fix Focus Areas

src/node/utils/ExportEtherpad.ts[36-49]
src/tests/backend/specs/ExportEtherpad.ts[108-126]

Recommended Fix

Apply the chat boundary whenever a valid revision is explicitly supplied, including the current head, while keeping exports without a revision unbounded. Add a test with chat posted after the last revision.

[correctness] Earlier chat disappears from snapshots

Earlier chat disappears from snapshots

`getPadRaw` stops scanning at the first chat record whose supplied timestamp is later than the bound revision. API callers can submit a future-dated message followed by an ordinary message before that revision; the latter has a qualifying timestamp but is excluded because chat indexes follow append order rather than timestamp order.

Issue description

A caller-supplied chat timestamp can make the first post-bound-timestamp record appear before qualifying records in chat-index order, causing the export loop to stop too soon.

Fix Focus Areas

src/node/utils/ExportEtherpad.ts[39-49]
src/node/db/API.ts[405-420]
src/node/db/Pad.ts[627-638]

Recommended Fix

Base revision-bounded selection on a recorded posting-order boundary rather than assuming supplied chat timestamps increase with indexes. Add a test with an API-provided future timestamp followed by an earlier-timestamped message.

[correctness] Large chats slow bounded exports

Large chats slow bounded exports

`getPadRaw` now awaits one database read per chat record before the timestamp cutoff, then fetches the included records again during serialization. For a revision preceded by a large chat history, these serialized round trips add latency to an export whose existing record-fetching path otherwise buffers concurrent reads.

Issue description

Finding the chat cutoff adds a sequential database read for every preceding message, followed by another read of each included message.

Fix Focus Areas

src/node/utils/ExportEtherpad.ts[43-49]
src/node/utils/ExportEtherpad.ts[74-85]

Recommended Fix

Use a persisted chat boundary if available, or determine the boundary with bounded concurrent reads and reuse fetched messages during serialization. Preserve posting-order selection and verify behavior on pads with long chat histories.

PR 8388 (2026-10-23)

[correctness] Exports create permanent authors on denial

Exports create permanent authors on denial

`checkAccess()` generates a fresh token and maps it to a persistent author before checking whether a group pad is private or can be created. A token-less export request for an inaccessible group pad reaches that mapping and leaves author records behind even though access is denied; repeated requests generate new records because the export route does not issue a token cookie.

Issue description

Token-less export requests can create persistent authors even when group-pad access is denied. Each such HTTP request gets a new token, so repeated requests grow the author database.

Fix Focus Areas

src/node/db/SecurityManager.ts[126-154]
src/node/hooks/express/importexport.ts[38-61]
src/node/db/AuthorManager.ts[112-128]

Recommended Fix

Perform all pad-access denial checks before allocating an author. Avoid allocating an author for read-only HTTP access checks that do not use an author ID, while preserving a distinct author for authorized token-less imports and socket sessions.

[correctness] Token-less requests bypass plugin handling

Token-less requests bypass plugin handling

`checkAccess()` replaces a missing token before `getAuthorId()` passes it to the author-mapping hooks as `context.token`. For token-less HTTP requests, plugins that handle a nullish token under the documented hook contract instead receive a generated token and no longer take their missing-token path.

Issue description

Generating a token before invoking the author-mapping hook changes its documented input for token-less requests, so plugins cannot identify those requests through context.token.

Fix Focus Areas

src/node/db/SecurityManager.ts[126-132]
src/node/db/AuthorManager.ts[145-154]
doc/api/hooks_server-side.md[219-238]

Recommended Fix

Keep the original missing-token value available to author-mapping hooks while supplying a distinct generated database key for core's fallback mapping. Ensure the change cannot restore a shared null author.

PR 8359 (2026-10-18)

[correctness] Releases fail after the app is enabled

Releases fail after the app is enabled

The app-token step requests access to `etherpad` but not `etherpad-lite`, which is the repository the first checkout uses. Once `RELEASE_APP_ID` is set, that checkout prefers the app token over the PAT, so it cannot authenticate and the release never reaches its tag pushes.

Issue description

The app token excludes the repository checked out and pushed by the release job.

Fix Focus Areas

.github/workflows/release.yml[33-43]

Recommended Fix

Request etherpad-lite instead of etherpad in the token step's repository list, and ensure the release app is installed on ether/etherpad-lite with write access.

PR 8326 (2026-10-10)

[correctness] Restricted viewers are promised editing

Restricted viewers are promised editing

`pad.share.readonly-explanation` says anyone with the editable link can edit, but the link does not override per-user restrictions. When authentication is required, `userCanModify()` rejects read-only users and users without modification authorization even if they open that URL.

Issue description

The Share dialog promises that anyone with the editable link can edit, even though authenticated users may have read-only access.

Fix Focus Areas

src/locales/en.json[368-368]

Recommended Fix

Revise the explanation to say the editable link remains usable for people who have permission to edit, while retaining the distinction between selecting a link and changing pad permissions.

PR 8325 (2026-10-09)

[correctness] Failed boots can leave test volumes behind

Failed boots can leave test volumes behind

The `finally` block calls the asserting `docker()` wrapper for `docker stop` before attempting either resource removal. If startup fails or the container exits before becoming healthy and that stop returns a nonzero status, cleanup replaces the startup failure with a stop error and skips removal of both the container and its volume.

Issue description

A failed docker stop during test cleanup throws before the test can remove its container or volume, and can mask the original startup failure.

Fix Focus Areas

src/tests/container/readonly-root.mjs[104-110]
src/tests/container/readonly-root.mjs[10-16]

Recommended Fix

Make cleanup tolerate a container that is already stopped, and attempt container and volume removal independently even if stopping or removing the container fails. Preserve the original test failure while reporting any cleanup failure.

PR 8319 (2026-10-07)

[correctness] Creators cannot retrieve hidden tokens

Creators cannot retrieve hidden tokens

`showDeletionTokenModalIfPresent()` returns before displaying the issued token, and the pad settings panel has no control to reveal or copy it. When `suppressPadDeletionTokenModal` is enabled, the creator receives the plaintext token only on first arrival; later visits receive `null`, leaving a creator who did not capture it unable to use token-based deletion from another device.

Issue description

Suppressing the modal leaves creators without a settings-based way to obtain the deletion token. The server issues its plaintext value only once and stores only a hash.

Fix Focus Areas

src/static/js/pad.ts[262-296]
src/templates/pad.html[397-440]
src/node/db/PadDeletionManager.ts[32-41]

Recommended Fix

Add an on-demand reveal or copy action in pad settings, with a secure creator-authorized way to obtain a usable token after the initial arrival. Verify that the suppressed modal stays hidden and the settings action works on a later visit.

[correctness] Readers see corrupted release notes

Readers see corrupted release notes

The `CHANGELOG.md` edit replaces existing punctuation and symbols with mojibake such as `ŌĆö` and `ŌĆ”`. The corruption extends across historical release entries, including security notes, making those records harder to read and search.

Issue description

The PR corrupts existing changelog characters across historical release notes, replacing punctuation and symbols with mojibake.

Fix Focus Areas

CHANGELOG.md[7-12]
CHANGELOG.md[493-496]

Recommended Fix

Restore the pre-existing changelog text with its original UTF-8 characters, retaining only the new pad-deletion entry.

PR 8302 (2026-10-05)

[correctness] Negative list starts reset to one

Negative list starts reset to one

The new `/^\d+$/` check in `attribsToClasses` drops a `start` value such as `-2`, although `domline.appendSpan` can parse that value and emit it as an ordered-list start attribute. When a pad's attribute pool contains a signed list start, the rendered list loses its stored starting position and uses the default numbering.

Issue description

The new start-value validation discards negative integers that the renderer previously supported, changing numbered-list display for pads containing those values.

Fix Focus Areas

src/static/js/linestylefilter.ts[93-95]
src/static/js/domline.ts[130-145]

Recommended Fix

Accept an optional leading minus sign while still rejecting whitespace and nonnumeric characters, and add a rendering test for a negative start.

[correctness] Security tests miss removed pool checks

Security tests miss removed pool checks

The `start` and `list` injection tests use an attribute-bearing `tag:` payload that the separate `domline` check rejects even if both attribute-pool filters are removed. If either filter regresses while the tag-name check remains, both tests still pass despite pool values again being able to forge class tokens such as `tag:b`.

Issue description

The attribute-pool regression tests pass without the filtering they are intended to protect because a separate tag-name check blocks their payload.

Fix Focus Areas

src/tests/backend/specs/domline_class_token_injection.ts[22-22]
src/tests/backend/specs/domline_class_token_injection.ts[51-65]

Recommended Fix

Add list and start cases that attempt to forge a valid bare token such as tag:b, and assert that it does not create the corresponding element.

PR 8273 (2026-09-25)

[correctness] Users cannot open the copied value

Users cannot open the copied value

`handleTransferOfSession` writes the bare UUID in `transferData.id` to the clipboard while the localized controls still call it a link that should be opened. After successful creation, users who follow that instruction receive no usable destination URL, and the surrounding text also describes the transferred author identity and preferences as a full session.

Issue description

The successful transfer flow copies a bare UUID, but the interface describes it as an openable link and calls the limited author identity and preferences payload a full session.

Fix Focus Areas

src/locales/en.json[225-232]
src/templates/index.html[188-205]
src/static/js/welcome.ts[98-106]

Recommended Fix

Rename the copy controls and instructions to describe a transfer code, tell users to paste it into the receiving interface's Code field, and clarify that the operation transfers the Etherpad author identity/token and client preferences rather than a full browser or login session. Update corresponding localized strings, or alternatively generate and copy a valid destination URL containing the transfer code.

[correctness] Closing settings loses transfer codes

Closing settings loses transfer codes

`handleTransferOfSession` captures `copyLinkSection` and the error element before awaiting the creation request, while dialog closure replaces those nodes. If the dialog is closed while the request is pending, the server-created ID is rendered into the detached section and is unavailable when the user reopens settings.

Issue description

Closing the settings dialog while transfer creation is pending replaces the DOM nodes captured by the request handler, so a successful transfer ID is applied to detached controls and cannot be copied.

Fix Focus Areas

src/static/js/welcome.ts[57-117]
src/static/js/welcome.ts[152-169]

Recommended Fix

Preserve the completed transfer result independently of dialog nodes and re-query the current dialog controls after each await before rendering success or failure. Ensure the current copy section receives the generated ID handler even when the dialog was closed and rebuilt during the request.

[correctness] Old failures replace newer feedback

Old failures replace newer feedback

`redeemTransferCode` disables the button while awaiting a request, but the input handler immediately re-enables it whenever a newly entered code is valid. If another redemption is started before the first completes, either failure can subsequently overwrite the newer request's error and button state.

Issue description

Editing the field to another valid code during an active redemption re-enables the button, allowing overlapping requests whose completion handlers update the same error and button controls without checking request identity.

Fix Focus Areas

src/static/js/welcome.ts[122-150]
src/static/js/welcome.ts[186-205]

Recommended Fix

Track whether a redemption is pending and keep the button disabled during that request regardless of input validity. Clear the pending state on failure, then derive the button state from the current input; alternatively cancel or generation-tag superseded requests and ignore their completions.

PR 8250 (2026-09-20)

[correctness] Older clients install deprecated plugins

Older clients install deprecated plugins

`install` normalizes npm's `deprecated` field but only logs the resulting message before continuing to `linkInstaller.installPlugin()`. A stale admin page or older client can therefore submit a package that the refreshed catalog would hide, and the server still installs it.

Issue description

The server-side install path only warns when npm marks a plugin deprecated, so stale or older admin clients can bypass the catalog filter and install it.

Fix Focus Areas

src/static/js/pluginfw/installer.ts[265-272]

Recommended Fix

When normalizeDeprecation(meta?.deprecated) returns a message, reject the admin installation before calling linkInstaller.installPlugin(). Preserve any separate explicit command-line override outside this admin request path.

[performance] Opening the catalog floods npm

Opening the catalog floods npm

`getAvailablePlugins()` starts `hideUninstallablePlugins()` independently for every concurrent cache miss, while `fetchPluginDeprecations()` caches only completed lookups and does not share in-flight work. The admin page emits `getInstalled` and `search` together on initial load, so both paths can sweep the entire catalog before either populates the cache, duplicating roughly one request per listed plugin and increasing page latency and npm load.

Issue description

Concurrent initial catalog requests independently fetch npm metadata for every plugin because neither getAvailablePlugins() nor the deprecation cache shares in-flight work. Opening the admin plugin page therefore creates duplicate full-catalog npm request bursts.

Fix Focus Areas

src/static/js/pluginfw/installer.ts[190-254]
src/static/js/pluginfw/installer.ts[283-310]
admin/src/pages/HomePage.tsx[79-128]

Recommended Fix

Add a shared in-flight promise for catalog loading or per-name@version lookup promises, and have concurrent callers await that work. Clear the in-flight state in finally so failed requests can be retried while completed deprecation results continue using the existing TTL cache.

[correctness] Installed plugins keep obsolete warnings

Installed plugins keep obsolete warnings

The `getInstalled` handler mutates package objects retained in `pluginDefs.plugins` and assigns `plugin.deprecated` only when the latest warning map contains a truthy entry, without clearing transient status fields first. When npm removes a deprecation, a lookup fails, or catalog checks are disabled after a successful check, later socket responses reuse the retained value and the admin UI renders the obsolete badge.

Issue description

getInstalled writes transient warning and update fields onto package objects retained by pluginDefs.plugins, but does not remove them when a warning disappears, a lookup fails, or catalog checking is disabled. Ensure every installed-plugin response starts without stale deprecated or updatable values while preserving the package metadata required by the UI.

Fix Focus Areas

src/node/hooks/express/adminplugins.ts[55-74]

Recommended Fix

Clone each package while constructing the installed array and apply current deprecated and updatable values only to those response-local objects. Alternatively, explicitly delete or reset both transient fields before applying the latest check results; in either case, ensure the catalog-disabled path also emits objects without stale status fields.

PR 8249 (2026-09-20)

[correctness] Important font styles are lost in PDFs

Important font styles are lost in PDFs

`parseFontFamily()` uses a non-global match for only the first `font-family` declaration and retains any trailing `!important` token, producing lookup keys such as `serif !important` instead of applying CSS declaration order and priority. Repeated or prioritized declarations can therefore resolve a different family than CSS specifies or miss both built-in and operator-configured families, causing the native PDF renderer to inherit the surrounding font.

Issue description

The inline-style parser selects only the first font-family declaration and retains a trailing !important annotation during normalization. As a result, repeated or prioritized declarations can select the wrong PDF font or fail exact lookup for built-in and configured families.

Fix Focus Areas

src/node/utils/ExportPdfNative.ts[139-149]
src/node/utils/ExportPdfNative.ts[483-492]

Recommended Fix

Parse every font-family declaration, remove a trailing case-insensitive !important token before splitting and normalizing family candidates, and choose the last declaration at the highest applicable priority. Add coverage for repeated declarations, earlier and later !important declarations, and prioritized built-in and configured families.

[reliability] Updated font files stay stale in PDFs

Updated font files stay stale in PDFs

`readFontFile` caches both font buffers and failed reads forever using only the absolute filename, without invalidating that cache when settings are reloaded. If an operator replaces a font or fixes a missing file at the same path through the live settings workflow, later exports keep using the old bytes or fallback until Etherpad is restarted.

Issue description

Process-wide font-file caching survives production settings reloads, leaving corrected or replaced files at an unchanged path invisible to PDF exports.

Fix Focus Areas

src/node/utils/ExportPdfNative.ts[193-227]
src/node/utils/Settings.ts[1203-1210]
src/node/hooks/express/adminsettings.ts[486-490]

Recommended Fix

Invalidate cached font buffers whenever the font configuration is rebuilt, or key entries by file metadata and retry cached failures after changes. Ensure the production settings-reload path triggers invalidation, and test replacing or creating a font at the same configured path without restarting the process.

PR 8240 (2026-09-17)

[correctness] Plugin lines lose paragraph navigation

Plugin lines lose paragraph navigation

`writeHTML` treats every nonempty `preHtml` or `postHtml` value as a semantic block wrapper and consequently omits `role="paragraph"`. When a line-attribute plugin surrounds content with non-semantic inline or styling markup, that line is omitted from paragraph navigation even though no heading or list semantics replace it.

Issue description

hasBlockWrapper currently assumes any hook-provided preHtml or postHtml string supplies block semantics. The hook contract permits arbitrary wrapper HTML, so inline or styling wrappers can incorrectly suppress the line's paragraph role.

Fix Focus Areas

src/static/js/domline.ts[89-135]
src/static/js/domline.ts[221-242]
doc/api/hooks_client-side.adoc[15-57]

Recommended Fix

Suppress role="paragraph" only when the rendered line has a known semantic block wrapper, such as a list, heading, preformatted block, or an explicitly declared semantic wrapper. Preserve the paragraph role for inline, styling-only, empty, or otherwise non-semantic hook wrappers, and add tests covering both semantic and inline plugin wrappers.

PR 8239 (2026-09-17)

[correctness] Valid closing braces disappear

Valid closing braces disappear

`EnvPill` decodes a raw `\u007d` default into `}` and then the added `sanitize(decoded)` call removes that character before rebuilding the placeholder. This silently corrupts valid environment defaults containing escaped closing braces, even though the backend placeholder parser permits braces within its greedily captured default.

Issue description

Decoding JSON escapes turns \u007d into a closing brace, after which EnvPill removes it from the default. The frontend placeholder matcher must also continue recognizing the reconstructed placeholder when a decoded default contains a brace.

Fix Focus Areas

admin/src/components/settings/widgets/EnvPill.tsx[69-76]
admin/src/components/settings/envPill.ts[12-20]

Recommended Fix

Stop stripping closing braces from decoded defaults and update the frontend placeholder matcher to capture defaults through the final placeholder terminator, consistent with the backend parser. Add a round-trip test for an environment default containing \u007d.

[correctness] Rejected defaults remain visibly unsaved

Rejected defaults remain visibly unsaved

`EnvPill` keeps the draft after `unescapeFromInput()` rejects an invalid or incomplete escape, while the new callback deliberately suppresses the parent update. When the user blurs the field after typing a trailing backslash or `\q`, the displayed value therefore disagrees with the unchanged settings JSON and can mislead them into saving a value that was never applied.

Issue description

EnvPill retains invalid or incomplete escaped text after blur even though that text was not propagated to the settings state. The field can therefore display a value different from what will be saved.

Fix Focus Areas

admin/src/components/settings/widgets/EnvPill.tsx[67-76]

Recommended Fix

Track whether the draft decodes successfully, expose rejection through aria-invalid, and reset the draft to the latest canonical initial value on blur, matching StringInput's behavior.

PR 8237 (2026-09-17)

[correctness] Encoded recent pads survive deletion

Encoded recent pads survive deletion

`forgetRecentPad()` compares each raw stored `p.name` only with the decoded URL name and canonical pad ID. When an older entry contains URL encoding such as `notes%26ideas`, deleting `notes&ideas` removes any newer decoded entry but leaves the encoded entry available on the welcome screen.

Issue description

Legacy recentPads entries may contain URL-encoded names, but deletion compares their raw names against decoded identifiers. This leaves a supported legacy entry visible after its pad is deleted.

Fix Focus Areas

src/static/js/pad.ts[302-314]
src/static/skins/colibris/index.js[73-81]

Recommended Fix

Normalize each non-null stored pad name before comparison, using the same guarded decodeURIComponent behavior as the welcome screen. Remove an entry when either its raw or normalized name matches padName or pad.getPadId(), while preserving safe handling for malformed percent encoding.

PR 8230 (2026-09-16)

[correctness] Imported authors lose diff colors

Imported authors lose diff colors

`isValidColorId()` accepts every non-negative integer and digit-only string without checking whether it indexes the configured palette. When an import supplies an out-of-range value such as `999`, color resolution preserves that value and the new export allowlist omits its CSS rule instead of replacing it with a usable palette color.

Issue description

Imported numeric color identifiers are accepted without palette bounds checking, so out-of-range values survive validation and later produce no author-color rule.

Fix Focus Areas

src/node/utils/ImportEtherpad.ts[35-37]
src/node/utils/ImportEtherpad.ts[266-270]

Recommended Fix

Parse numeric color identifiers and accept them only when they are integers from zero through authorManager.getColorPalette().length - 1; continue accepting supported hexadecimal colors. Replace all other values through the existing fallback and add tests for out-of-range numbers and numeric strings.

PR 8179 (2026-08-30)

[correctness] Italian macro remains unfixed

Italian macro remains unfixed

The new validation records Italian's `plurale` macro as unsupported and then requires an empty failure list, so this test fails while the translation that freezes Italian-configured pads remains in the corpus. Replace `plurale`/`uno`/`altro` with the supported `plural`/`one`/`other` syntax so the test passes and the runtime loop is removed.

Issue description

The new locale validation detects the existing Italian plurale macro and fails, but the PR does not correct the translation responsible for the Italian pad freeze.

Issue Context

The runtime supports plural with CLDR option names. Update Italian's pad.userlist.onlineCount from plurale(count) uno: ... altro: ... to plural(count) one: ... other: ..., preserving the Italian display text.

Fix Focus Areas

src/locales/it.json[398-398]
src/tests/backend-new/specs/locale-files.test.ts[26-39]

[correctness] Malformed macros evade validation

Malformed macros evade validation

The validation loop only checks strings that fully match `macroRe`, so malformed macro candidates are silently skipped; existing Hungarian and Unicode locale entries pass this test even though the runtime cannot expand them and displays the raw `{[...]}` markup. This prevents the test from enforcing its stated guarantee that the locale corpus contains no unsupported macros.

Issue description

The locale validator only inspects complete macroRe matches, allowing malformed or runtime-unsupported {[...]} expressions to pass unnoticed.

Issue Context

The runtime uses similarly restrictive syntax and leaves unmatched macro text in the rendered translation. Scan for every macro candidate, validate that the entire candidate conforms to supported syntax, and report malformed candidates; update currently invalid locale entries so the strengthened test passes.

Fix Focus Areas

src/tests/backend-new/specs/locale-files.test.ts[13-39]
src/locales/hu.json[161-161]
src/locales/diq.json[162-162]
src/locales/shn.json[109-109]
src/locales/my.json[152-152]

PR 8154 (2026-08-23)

[security] Percent-whitespace URL bypass

Percent-whitespace URL bypass

`canonicalizeUrl()` strips C0/space before `decodeURIComponent()`, so percent-encoded whitespace/control (for example `%20http://…`) is reintroduced after decoding and can cause `isRemoteUrl()` / `isSafeNavUrl()` to misclassify an absolute or dangerous-scheme URL as “no scheme”, letting it survive sanitization.

Issue description

canonicalizeUrl() removes URL_IGNORED_RE characters before percent-decoding, but percent-decoding can reintroduce those characters (e.g. %20, %09, %0a). This can under-classify URLs as non-absolute / safe.

Issue Context

The sanitizer relies on canonicalizeUrl() for both subresource URL detection (isRemoteUrl / escapesBase) and navigational URL scheme filtering (isSafeNavUrl). If canonicalizeUrl() returns a value that starts with a decoded whitespace/control character, SCHEME_RE won’t match and the URL can be incorrectly allowed.

Fix Focus Areas

src/node/utils/ExportSanitizeHtml.ts[186-207]

Suggested change

Restructure canonicalization so ignored chars are stripped after entity decoding and percent-decoding (or strip both before and after). For example:

const entityDecoded = decodeHTML(raw);
const pctDecoded = try decodeURIComponent(entityDecoded) else entityDecoded;
return pctDecoded.replace(URL_IGNORED_RE, ''); Add a unit case like <img src="%20http://evil.example/a.png"> and <a href="%20javascript:alert(1)">x</a> to ensure they are treated the same as their literal-whitespace equivalents.

[security] Escaped CSS url() bypass

Escaped CSS url() bypass

`sanitizeCssText()` only removes literal `@import` and `url(` patterns, so CSS-escaped or comment-split forms (e.g. `\75 rl(` or `u/**/rl(`) can survive inside `` blocks and still resolve to fetchable URLs when parsed by downstream CSS consumers.

Issue description

sanitizeCssText() is intended to prevent CSS-triggered fetches via url() and @import in <style> elements, but it only matches literal spellings. CSS escapes (backslash hex escapes) and comment splitting can encode these tokens in ways that most CSS parsers normalize back to url() / @import.

Issue Context

CSS_FETCH_RE explicitly accounts for CSS escapes (backslash) for style="..." attributes, but <style> content uses sanitizeCssText() which does not.
<style> text is passed through sanitizeCssText() via the parser’s ontext handler when inStyle is true.

Fix Focus Areas

src/node/utils/ExportSanitizeHtml.ts[257-274]
src/node/utils/ExportSanitizeHtml.ts[338-341]

Suggested change

Make <style> sanitization robust against CSS escapes/comments. Options (in increasing complexity):

Conservative: if the <style> text contains any backslash (\\) or /*, drop the entire <style> block (or strip those constructs) since core’s preserved export CSS shouldn’t need them.
Preprocess CSS by removing comments and normalizing CSS escapes before applying the @import/url() filters.
Use a CSS parser/tokenizer to reliably find @import and url() tokens and rewrite/drop them. Add regression tests such as:

<style>body{background:\\75 rl(http://evil.example/a.png)}</style>
<style>body{background:u/**/rl(http://evil.example/a.png)}</style> ensuring evil.example does not survive.

PR 8112 (2026-08-07)

[reliability] Browser-only getHomeUrl default

Browser-only getHomeUrl default

`getHomeUrl()` defaults `fromHref` to `window.location.href`, so calling `getHomeUrl()` without an argument in a non-browser runtime will throw `ReferenceError: window is not defined`. This is a latent reliability/maintainability footgun now that the helper is imported from backend tests and could be reused elsewhere; it is safe as long as non-browser callers always pass `fromHref`.

Issue description

getHomeUrl() uses window.location.href as a default parameter value. That makes getHomeUrl() crash if it is ever invoked without an argument in a non-browser environment (Node, server-side rendering, some test environments).

Issue Context

This helper is now imported by Node-run backend tests, which increases the chance of it being reused outside the browser in the future.

Fix Focus Areas

src/static/js/getHomeUrl.ts[11-12]

Suggested fix

Make the default safe by guarding access to window, for example:

Change the signature to require fromHref (no default) and keep browser call sites passing window.location.href, or
Keep an optional parameter but implement a guarded default:
const href = fromHref ?? (typeof window !== 'undefined' ? window.location.href : '');
If href is empty, throw a clear error message (or return '/' depending on desired semantics). This preserves the current browser behavior while preventing surprising crashes in non-browser callers.

PR 8089 (2026-08-04)

[reliability] Premature token deletion

Premature token deletion

transferDeletionToken() deletes the source pad's deletionToken record before movePad attempts pad.remove(), so a pad.remove() failure can leave the source pad still present but without its original deletion token. This regresses movePad's failure-mode behavior by potentially stranding creators who rely on the saved token to delete the original pad.

Issue description

transferDeletionToken() removes the source deletion-token record (pad:<src>:deletionToken) before pad.remove() runs. If pad.remove() rejects (it executes multiple DB operations in parallel via Promise.all()), the source pad may remain (fully or partially) but its token is already gone.

Issue Context

API.movePad() does: copy() → transferDeletionToken() → remove().
Pad.remove() performs many removals concurrently; any failure rejects the whole operation.

Fix Focus Areas

Adjust token transfer so the source token is not deleted until the move is definitively complete.
Prefer relying on Pad.remove() to delete the source token on success, or implement rollback/restore if the move fails after token manipulation.

Fix Focus Areas (code refs)

src/node/db/PadDeletionManager.ts[53-63]
src/node/db/API.ts[766-775]
src/node/db/Pad.ts[819-878]

[reliability] Source token removed early

Source token removed early

PadDeletionManager.transferDeletionToken() deletes the source pad’s deletionToken before API.movePad() calls pad.remove(); if pad.remove() fails (or the process crashes) after the transfer, the source pad can remain but its saved token is already gone. This strands token-based deletion for the surviving source pad even though Pad.remove() already handles token cleanup on successful completion.

Issue description

transferDeletionToken() currently removes the source pad’s deletion token immediately after writing the destination token. Because API.movePad() invokes this before pad.remove(), any failure after the transfer but before successful completion of pad.remove() can leave the source pad still present but without its deletion token.

Issue Context

Pad.remove() already calls padDeletionManager.removeDeletionToken(padID) as part of the pad deletion flow, so removing the source token inside transferDeletionToken() is redundant on success but harmful on partial failure.

Fix Focus Areas

src/node/db/PadDeletionManager.ts[47-66]
src/node/db/API.ts[754-776]

Suggested change

In transferDeletionToken(src, dst), keep the set(dstKey, stored) but do not remove srcKey there; rely on pad.remove() to remove the source token when the move completes.
If you still want eager cleanup, only remove the source token after pad.remove() succeeds (or implement rollback on failure).

[reliability] Source token removed early

Source token removed early

Issue description

Issue Context

Fix Focus Areas

src/node/db/PadDeletionManager.ts[47-66]
src/node/db/API.ts[754-776]

Suggested change

In transferDeletionToken(src, dst), keep the set(dstKey, stored) but do not remove srcKey there; rely on pad.remove() to remove the source token when the move completes.
If you still want eager cleanup, only remove the source token after pad.remove() succeeds (or implement rollback on failure).

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

Issue description

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

Issue description

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

Issue description

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:

Hardcode the expected list in the test and assert each is included.
(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:

Preserve doctype/comments in stripRemoteImages():

Add onprocessinginstruction and oncomment handlers to append the raw directive/comment to out.
Ensure doctype is preserved exactly once.

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:

Min-risk security floor: pin to the minimal patched major: basic-ftp@<5.3.1: '5.3.1' (or ^5.3.1).
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

In handleClientReady, compute canDeletePad so it is false for readonly sessions, for example:

const canDeletePad = !sessionInfo.readonly && (isCreator || settings.allowPadDeletionByAllUsers); (or equivalent).

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).
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

Add test.use({locale: 'en-US'}); near the top of each new spec file that compares against English strings.
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

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).
Keep default-text attribution as system author for changeset ops, but do not use the system author as the creator identity.
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.
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

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.
Consider navigating the user to /login (or showing a stable logged-out state) on admin_auth_error.
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]

General

Resources

For Developers

How to's

Set up

Advanced steps

Integrating Etherpad in your web app

Embedding a pad using the jQuery plugin
Using Embed Parameters
Integrating Etherpad in a third party app (Drupal, MediaWiki, WordPress, Atlassian, PmWiki)
HTTP API client libraries