harden: assorted security tightening across server entry points

A bundle of defence-in-depth hardening picked up during an internal
audit pass. Each change is small on its own; landing them together
keeps the diff cohesive for review and the release notes simple.

Production-side changes:
  - src/node/handler/APIHandler.ts: tighten the OAuth JWT validation
    path on the HTTP API. Verify the signature before reading any
    claim off the payload, and require the admin claim to be strictly
    true (not just present). Switch the apikey comparison to
    crypto.timingSafeEqual.
  - src/node/handler/{Import,Export}Handler.ts: derive temp-file path
    tokens from crypto.randomBytes(16) instead of Math.random.
  - src/node/hooks/express/tokenTransfer.ts: enforce a 5-minute TTL
    on transfer records, make redemption single-use (remove before
    response), and drop the author token from the response body —
    the HttpOnly cookie is the only delivery channel.
  - src/node/utils/sanitizeProxyPath.ts (new): shared sanitiser for
    the `x-proxy-path` header. Used by admin.ts (HTML/JS/CSS
    substitution) and specialpages.ts (legacy timeslider redirect).
    Strips characters outside [A-Za-z0-9_./-], collapses leading
    `//+` to a single `/`, rejects `..` traversal. admin.ts also
    emits Vary: x-proxy-path and Cache-Control: private, no-store.
  - src/node/db/Pad.ts + src/node/utils/ImportHtml.ts: centralise
    the "every insert op carries an author attribute" invariant in
    Pad.appendRevision so all non-wire callers (setText, setHTML,
    restoreRevision, plugin paths) get the same check the socket
    handler already enforces. Pad.init and setPadHTML now
    substitute SYSTEM_AUTHOR_ID when no author is supplied — same
    pattern setText/spliceText already use.

Tests:
  - src/tests/backend/specs/api/jwtAdminClaim.ts (5 cases)
  - src/tests/backend/specs/tokenTransfer.ts (6 cases)
  - src/tests/backend/specs/proxyPathRedirect.ts (5 cases)
  - src/tests/backend/specs/padInsertAuthorInvariant.ts (4 cases)
  - src/tests/backend-new/specs/sanitizeProxyPath.test.ts (14 vitest cases)
  - src/tests/backend/common.ts: add generateJWTTokenAdminFalse helper.

Regression sweep across 16 backend spec files: same 5 pre-existing
failures on develop reproduce after this change; +20 new passing
tests; no new failures introduced.

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

Author: JohnMcLear

src/node/db/API.ts

@@ -19,10 +19,15 @@
  * limitations under the License.
  */
 
+import AttributeMap from '../../static/js/AttributeMap';
 import {deserializeOps} from '../../static/js/Changeset';
 import ChatMessage from '../../static/js/ChatMessage';
 import {Builder} from "../../static/js/Builder";
 import {Attribute} from "../../static/js/types/Attribute";
+
+// Mirror of `Pad.SYSTEM_AUTHOR_ID`. Inlined to avoid a circular load
+// (API <-> Pad) at module init time.
+const SYSTEM_AUTHOR_ID = 'a.etherpad-system';
 import settings from '../utils/Settings';
 const CustomError = require('../utils/customError');
 const padManager = require('./PadManager');
@@ -620,9 +625,28 @@ exports.restoreRevision = async (padID: string, rev: number, authorId = '') => {
   // create a new changeset with a helper builder object
   const builder = new Builder(oldText.length);
 
+  // The author to attribute inserts to. If the caller supplied an
+  // explicit authorId, that wins; otherwise fall back to the stable
+  // system author. The replayed atext was built from historical
+  // revisions that may legitimately have insert ops without an
+  // author attribute (legacy server-internal flows / .etherpad
+  // imports); appendRevision now requires every insert to carry
+  // one, so we merge the marker in below.
+  const replayAuthorId = authorId || SYSTEM_AUTHOR_ID;
+
   // assemble each line into the builder
-  eachAttribRun(atext.attribs, (start: number, end: number, attribs:Attribute[]) => {
-    builder.insert(atext.text.substring(start, end), attribs);
+  eachAttribRun(atext.attribs, (start: number, end: number, attribs:string) => {
+    // attribs here is the op.attribs *string* (the eachAttribRun
+    // callback receives it as the third arg). Use AttributeMap to
+    // merge in `author` while preserving canonical (sorted) order
+    // so checkRep doesn't reject the result. The `.set` call is a
+    // no-op when the existing attribs already contain an `author`
+    // attribute that matches; when they contain a *different*
+    // author it preserves the historical attribution (we only
+    // set author when it's missing).
+    const map = AttributeMap.fromString(attribs, pad.pool);
+    if (!map.get('author')) map.set('author', replayAuthorId);
+    builder.insert(atext.text.substring(start, end), map.toString());
   });
 
   const lastNewlinePos = oldText.lastIndexOf('\n');

src/node/db/Pad.ts

@@ -104,6 +104,58 @@ class Pad {
    */
   static readonly SYSTEM_AUTHOR_ID = 'a.etherpad-system';
 
+  /**
+   * Validate that every `+` (insert) op in `aChangeset` carries an
+   * `author` attribute that resolves through `pool`. Callers that have
+   * already rebased onto pad.pool pass the post-rebase changeset, so
+   * we accept the pad's own pool here.
+   *
+   * Throws an Error if any insert op is missing an author attribute,
+   * carries an empty author, or references an attribute number that
+   * is not present in the pool.
+   *
+   * Tolerates `=` and `-` ops with empty attribs (those are the
+   * canonical form for keeps/deletes that don't change attribution).
+   * Also tolerates pure-newline `+` ops: the client's line assembler
+   * handles those regardless of attribs, and the API restoreRevision
+   * path emits them at line boundaries.
+   */
+  private static _assertInsertOpsCarryAuthor(aChangeset: string, pool: AttributePool) {
+    let unpacked;
+    try {
+      unpacked = unpack(aChangeset);
+    } catch (e: any) {
+      // unpack already throws a descriptive error; rethrow as-is so the
+      // caller's failure mode stays the same.
+      throw e;
+    }
+    for (const op of deserializeOps(unpacked.ops)) {
+      if (op.opcode !== '+') continue;
+      // Pure-newline inserts (e.g. `|1+1` for a single line break) are
+      // tolerated — the client's line assembler handles them regardless
+      // of attribs, and the API restoreRevision path emits them at
+      // line boundaries.
+      if (op.lines > 0 && op.chars === op.lines) continue;
+      if (!op.attribs) {
+        throw new Error(
+            'insert op without an author attribute ' +
+            `(empty attribs): ${aChangeset}`);
+      }
+      let authorIdSeen: string | undefined;
+      try {
+        authorIdSeen = AttributeMap.fromString(op.attribs, pool).get('author');
+      } catch (e: any) {
+        throw new Error(
+            'insert op references an attribute number ' +
+            `not present in the pool: ${aChangeset} (${e && e.message || e})`);
+      }
+      if (!authorIdSeen) {
+        throw new Error(
+            'insert op without an author attribute: ' + aChangeset);
+      }
+    }
+  }
+
   private db: Database;
   private atext: AText;
   private pool: AttributePool;
@@ -226,6 +278,13 @@ class Pad {
    * @return {Promise<number|string>}
    */
   async appendRevision(aChangeset:string, authorId = '') {
+    // Centralised "every insert op carries an author attribute"
+    // invariant. The socket handler enforces the same rule at the wire
+    // boundary; checking here covers the non-wire callers (HTTP API
+    // setHTML/setText/restoreRevision, plugin paths that call
+    // appendRevision directly).
+    Pad._assertInsertOpsCarryAuthor(aChangeset, this.pool);
+
     const newAText = applyToAText(aChangeset, this.atext, this.pool);
     if (newAText.text === this.atext.text && newAText.attribs === this.atext.attribs &&
         this.head !== -1) {
@@ -537,9 +596,19 @@ class Pad {
         if (context.type !== 'text') throw new Error(`unsupported content type: ${context.type}`);
         text = exports.cleanText(context.content);
       }
-      const firstAttribs = authorId ? [['author', authorId] as [string, string]] : undefined;
+      // When the initial pad text is non-empty but no authorId was
+      // supplied (internal getPad calls during HTTP API setup,
+      // padDefaultContent flows, plugin-driven pad creation), fall back
+      // to the stable system author so the initial changeset's insert
+      // op carries an `author` attribute. Mirrors the same substitution
+      // setText/appendText already do via spliceText.
+      const effectiveAuthorId =
+          (text.length > 0 && !authorId) ? Pad.SYSTEM_AUTHOR_ID : authorId;
+      const firstAttribs = effectiveAuthorId
+          ? [['author', effectiveAuthorId] as [string, string]]
+          : undefined;
       const firstChangeset = makeSplice('\n', 0, 0, text, firstAttribs, this.pool);
-      await this.appendRevision(firstChangeset, authorId);
+      await this.appendRevision(firstChangeset, effectiveAuthorId);
     }
     this.padSettings = Pad.normalizePadSettings(this.padSettings);
     await hooks.aCallAll('padLoad', {pad: this});
@@ -665,9 +734,25 @@ class Pad {
 
     const oldAText = this.atext;
 
+    // The author to attribute inserts to when the historical op lacks
+    // one (legacy server-internal flows / .etherpad imports). Caller-
+    // supplied authorId wins; otherwise the stable system author.
+    // appendRevision now requires every insert to carry an author, so
+    // unattributed ops in the source pad would otherwise throw here.
+    const replayAuthorId = authorId || Pad.SYSTEM_AUTHOR_ID;
+
     // based on Changeset.makeSplice
     const assem = new SmartOpAssembler();
-    for (const op of opsFromAText(oldAText)) assem.append(op);
+    for (const op of opsFromAText(oldAText)) {
+      if (op.opcode === '+') {
+        const map = AttributeMap.fromString(op.attribs, dstPad.pool);
+        if (!map.get('author')) {
+          map.set('author', replayAuthorId);
+          op.attribs = map.toString();
+        }
+      }
+      assem.append(op);
+    }
     assem.endDocument();
 
     // although we have instantiated the dstPad with '\n', an additional '\n' is
@@ -867,6 +952,12 @@ class Pad {
         assert(changeset != null);
         assert.equal(typeof changeset, 'string');
         checkRep(changeset);
+        // NOTE: pad.check() intentionally does not invoke
+        // _assertInsertOpsCarryAuthor — it runs against historical
+        // stored data (including legacy .etherpad files) where some
+        // server-internal flows did not previously substitute the
+        // system author. The write-time guard in appendRevision is
+        // where the invariant is enforced for new content.
         const unpacked = unpack(changeset);
         let text = atext.text;
         for (const op of deserializeOps(unpacked.ops)) {

src/node/handler/APIHandler.ts

@@ -20,7 +20,6 @@
  */
 
 import {MapArrayType} from "../types/MapType";
-import { jwtDecode } from "jwt-decode";
 const api = require('../db/API');
 const padManager = require('../db/PadManager');
 import settings from '../utils/Settings';
@@ -29,6 +28,7 @@ import {Http2ServerRequest} from "node:http2";
 import {publicKeyExported} from "../security/OAuth2Provider";
 import {jwtVerify} from "jose";
 import {APIFields, apikey} from './APIKeyHandler'
+import crypto from 'node:crypto';
 // a list of all functions
 const version:MapArrayType<any> = {};
 
@@ -179,27 +179,41 @@ exports.handle = async function (apiVersion: string, functionName: string, field
 
   if (apikey !== null && apikey.trim().length > 0) {
     fields.apikey = fields.apikey || fields.api_key || fields.authorization;
-    // API key is configured, check if it is valid
-    if (fields.apikey !== apikey!.trim()) {
+    // Constant-time compare — see crypto.timingSafeEqual docs.
+    const provided = Buffer.from(String(fields.apikey ?? ''), 'utf8');
+    const want = Buffer.from(apikey!.trim(), 'utf8');
+    const ok = provided.length === want.length &&
+        crypto.timingSafeEqual(provided, want);
+    if (!ok) {
       throw new createHTTPError.Unauthorized('no or wrong API Key');
     }
   } else {
-    if(!req.headers.authorization) {
+    if (!req.headers.authorization) {
       throw new createHTTPError.Unauthorized('no or wrong API Key');
     }
     try {
-      const clientIds: string[] = settings.sso.clients?.map((client: {client_id: string}) => client.client_id) ?? [];
-      const jwtToCheck = req.headers.authorization.replace("Bearer ", "")
-      const payload = jwtDecode(jwtToCheck)
-      // client_credentials
-      if (clientIds.includes(<string>payload.sub)) {
-        await jwtVerify(jwtToCheck, publicKeyExported!, {algorithms: ['RS256']})
-      } else {
-        // authorization_code
-        await jwtVerify(jwtToCheck, publicKeyExported!, {algorithms: ['RS256'],
-          requiredClaims: ["admin"]})
+      const clientIds: string[] = settings.sso.clients?.map(
+          (client: {client_id: string}) => client.client_id) ?? [];
+      const jwtToCheck = req.headers.authorization.replace('Bearer ', '');
+      // Verify the JWT signature first, then read claims off the verified
+      // payload only.
+      const {payload: verified} = await jwtVerify(
+          jwtToCheck, publicKeyExported!, {algorithms: ['RS256']});
+      const isClientCredentials =
+          clientIds.includes(verified.sub as string);
+      if (!isClientCredentials) {
+        // authorization_code branch: require the admin claim to be
+        // strictly true. Checking only that the claim is present is not
+        // sufficient — the provider issues it as `admin: is_admin`, so
+        // non-admin users would have it set to false.
+        if (verified.admin !== true) {
+          throw new createHTTPError.Unauthorized(
+              'admin claim missing or not true');
+        }
       }
     } catch (e) {
+      // Single error string regardless of the underlying failure so we
+      // don't leak which check rejected the token.
       throw new createHTTPError.Unauthorized('no or wrong OAuth token');
     }
   }

src/node/handler/ExportHandler.ts

@@ -23,6 +23,7 @@
 const exporthtml = require('../utils/ExportHtml');
 const exporttxt = require('../utils/ExportTxt');
 const exportEtherpad = require('../utils/ExportEtherpad');
+import crypto from 'node:crypto';
 import fs from 'fs';
 import settings from '../utils/Settings';
 import os from 'os';
@@ -155,8 +156,9 @@ exports.doExport = async (req: any, res: any, padId: string, readOnlyId: string,
       }
     }
 
-    // soffice path — write the html export to a file
-    const randNum = Math.floor(Math.random() * 0xFFFFFFFF);
+    // soffice path — write the html export to a file. Use CSPRNG output
+    // for the temp path token (see matching note in ImportHandler.ts).
+    const randNum = crypto.randomBytes(16).toString('hex');
     const srcFile = `${tempDirectory}/etherpad_export_${randNum}.html`;
     await fsp_writeFile(srcFile, html);
 

src/node/handler/ImportHandler.ts

@@ -23,6 +23,7 @@
 
 const padManager = require('../db/PadManager');
 const padMessageHandler = require('./PadMessageHandler');
+import crypto from 'node:crypto';
 import {promises as fs} from 'fs';
 import path from 'path';
 import settings from '../utils/Settings';
@@ -83,7 +84,10 @@ const doImport = async (req:any, res:any, padId:string, authorId:string) => {
   // pipe to a file
   // convert file to html via soffice
   // set html in the pad
-  const randNum = Math.floor(Math.random() * 0xFFFFFFFF);
+  //
+  // Use CSPRNG output for the temp path token so the destination path
+  // can't be predicted by another process on the same host.
+  const randNum = crypto.randomBytes(16).toString('hex');
 
   // setting flag for whether to use converter or not
   let useConverter = (converter != null);

src/node/hooks/express/admin.ts

@@ -5,6 +5,7 @@ import fs from "fs";
 import {MapArrayType} from "../../types/MapType";
 
 import settings from 'ep_etherpad-lite/node/utils/Settings';
+import {sanitizeProxyPath} from '../../utils/sanitizeProxyPath';
 
 const ADMIN_PATH = path.join(settings.root, 'src', 'templates');
 const PROXY_HEADER = "x-proxy-path"
@@ -72,11 +73,19 @@ exports.expressCreateServer = (hookName: string, args: ArgsExpressType, cb: Func
           // if the file is found, set Content-type and send data
           res.setHeader('Content-type', map[ext] || 'text/plain');
           if (ext === ".html" || ext === ".js" || ext === ".css") {
-            if (req.header(PROXY_HEADER)) {
+            // The proxy-path header is woven into the response body, so
+            // it must be sanitised before substitution and downstream
+            // caches must not collapse responses across different
+            // header values.
+            const proxyPath = sanitizeProxyPath(req);
+            if (proxyPath) {
               let string = data.toString()
-              dataToSend = string.replaceAll("/admin", req.header(PROXY_HEADER) + "/admin")
-              dataToSend = dataToSend.replaceAll("/socket.io", req.header(PROXY_HEADER) + "/socket.io")
+              dataToSend = string.replaceAll("/admin", proxyPath + "/admin")
+              dataToSend = dataToSend.replaceAll(
+                  "/socket.io", proxyPath + "/socket.io")
             }
+            res.setHeader('Vary', 'x-proxy-path');
+            res.setHeader('Cache-Control', 'private, no-store');
           }
           res.end(dataToSend);
         }

src/node/hooks/express/specialpages.ts

@@ -20,12 +20,10 @@ import prometheus from "../../prometheus";
 
 let ioI: { sockets: { sockets: any[]; }; } | null = null
 
-// Sanitize x-proxy-path header to prevent XSS via header injection.
-// Only allow path-like characters (letters, digits, hyphens, underscores, slashes, dots).
-const sanitizeProxyPath = (req: any): string => {
-  const raw = req.header('x-proxy-path') || '';
-  return raw.replace(/[^a-zA-Z0-9\-_\/\.]/g, '');
-};
+// Shared sanitizer for the `x-proxy-path` header. See the helper for the
+// allowed character class and the protocol-relative / traversal rejection
+// rules. Reused by admin.ts so both call sites share one definition.
+import {sanitizeProxyPath} from '../../utils/sanitizeProxyPath';
 
 
 exports.socketio = (hookName: string, {io}: any) => {