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/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});
@@ -867,6 +936,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) => {

src/node/hooks/express/tokenTransfer.ts

@@ -7,10 +7,17 @@ import settings from '../../utils/Settings';
 type TokenTransferRequest = {
   token: string;
   prefsHttp: string,
-  createdAt?: number;
+  createdAt: number;
 }
 
-const tokenTransferKey = "tokenTransfer:";
+// Keep the legacy on-the-wire key shape so any in-flight transfers
+// created before this change are still redeemable.
+const tokenTransferKey = (id: string) => `tokenTransfer::${id}`;
+
+// Transfer records have a hard TTL — the legitimate flow is "scan a QR
+// code on another device and click within a few minutes". A stale id
+// should not be redeemable indefinitely.
+const TRANSFER_TTL_MS = 5 * 60 * 1000;
 
 export const expressCreateServer =  (hookName:string, {app}:ArgsExpressType) => {
   app.post('/tokenTransfer', async (req: any, res) => {
@@ -33,7 +40,7 @@ export const expressCreateServer =  (hookName:string, {app}:ArgsExpressType) =>
       createdAt: Date.now(),
     };
 
-    await db.set(`${tokenTransferKey}:${id}`, token);
+    await db.set(tokenTransferKey(id), token);
     res.send({id});
   })
 
@@ -43,11 +50,26 @@ export const expressCreateServer =  (hookName:string, {app}:ArgsExpressType) =>
       return res.status(400).send({error: 'Invalid request'});
     }
 
-    const tokenData = await db.get(`${tokenTransferKey}:${id}`);
+    const key = tokenTransferKey(id);
+    const tokenData: TokenTransferRequest | undefined = await db.get(key);
     if (!tokenData) {
       return res.status(404).send({error: 'Token not found'});
     }
 
+    // Single-use: remove the record BEFORE the response is sent, so a
+    // parallel request that wins the race observes an already-redeemed
+    // transfer rather than a second usable copy.
+    await db.remove(key);
+
+    // Enforce the TTL. Absent/non-numeric createdAt is treated as
+    // expired so legacy records that pre-date this code path are
+    // rejected on the safe side.
+    const createdAt = typeof tokenData.createdAt === 'number'
+        ? tokenData.createdAt : 0;
+    if (Date.now() - createdAt > TRANSFER_TTL_MS) {
+      return res.status(410).send({error: 'Token expired'});
+    }
+
     const p = settings.cookie.prefix;
     // Re-issue the author token on the new device as an HttpOnly cookie to
     // match the /p/:pad path (ether/etherpad#6701 PR3). Without this, the
@@ -63,6 +85,9 @@ export const expressCreateServer =  (hookName:string, {app}:ArgsExpressType) =>
     res.cookie(`${p}prefsHttp`, tokenData.prefsHttp, {
       path: '/', maxAge: 1000 * 60 * 60 * 24 * 365,
     });
-    res.send(tokenData);
+    // Body must NOT echo the author token — the HttpOnly cookie above
+    // is the only channel. Body advertises only the non-secret prefs
+    // the client needs to wire up locally.
+    res.send({ok: true, prefsHttp: tokenData.prefsHttp});
   })
 }

src/node/utils/ImportHtml.ts

@@ -16,12 +16,17 @@
  */
 
 import log4js from 'log4js';
+import AttributeMap from '../../static/js/AttributeMap';
 import {deserializeOps} from '../../static/js/Changeset';
 const contentcollector = require('../../static/js/contentcollector');
 import jsdom from 'jsdom';
 import {PadType} from "../types/PadType";
 import {Builder} from "../../static/js/Builder";
 
+// Mirror of `Pad.SYSTEM_AUTHOR_ID`. Imported as a literal to avoid a
+// circular require between Pad and ImportHtml during module init.
+const SYSTEM_AUTHOR_ID = 'a.etherpad-system';
+
 const apiLogger = log4js.getLogger('ImportHtml');
 let processor:any;
 
@@ -72,6 +77,15 @@ exports.setPadHTML = async (pad: PadType, html:string, authorId = '') => {
   // create a new changeset with a helper builder object
   const builder = new Builder(1);
 
+  // Every insert op needs an `author` attribute (the appendRevision
+  // precondition). The contentcollector tags ops with style
+  // attributes (bold, italic, etc.) but doesn't add an author; for
+  // server-side imports the author is implicit in the caller, so
+  // substitute the system author when no explicit one was supplied —
+  // same pattern setText/spliceText already use.
+  const effectiveAuthorId =
+      (newText.length > 0 && !authorId) ? SYSTEM_AUTHOR_ID : authorId;
+
   // assemble each line into the builder
   let textIndex = 0;
   const newTextStart = 0;
@@ -81,7 +95,16 @@ exports.setPadHTML = async (pad: PadType, html:string, authorId = '') => {
     if (!(nextIndex <= newTextStart || textIndex >= newTextEnd)) {
       const start = Math.max(newTextStart, textIndex);
       const end = Math.min(newTextEnd, nextIndex);
-      builder.insert(newText.substring(start, end), op.attribs);
+      // Merge via AttributeMap so the result is in canonical order
+      // (sorted by pool index) — a raw `*N` prefix could violate
+      // checkRep's canonical-form assertion.
+      let mergedAttribs = op.attribs;
+      if (effectiveAuthorId) {
+        mergedAttribs = AttributeMap.fromString(op.attribs, pad.pool)
+            .set('author', effectiveAuthorId)
+            .toString();
+      }
+      builder.insert(newText.substring(start, end), mergedAttribs);
     }
     textIndex = nextIndex;
   }