harden: reject USER_CHANGES inserts without an author attribute (#7773)

* harden: reject USER_CHANGES inserts without an author attribute

Insert ops MUST carry the author attribute reference so that pad.atext.text
and pad.atext.attribs stay in lock-step. An accepted insert with empty
attribs would grow text without contributing matching attribute markers,
leaving the stored AText in a state where the two iterables disagree on
length when reconstructed. Downstream clients then fail reconciliation in
ace2_inner.ts:setDocAText with 'mismatch error setting raw text in
setDocAText' on every subsequent pad load — making the affected pad
effectively unloadable until manually repaired.

This commit adds a single defensive check inside the existing per-op
validation loop in handleUserChanges: when an op is a '+' (insert) and
its attribs string doesn't yield an 'author' entry via
AttributeMap.fromString, reject with badChangeset. The check piggybacks
on the wireApool that was already constructed for the prior author-match
validation, so no extra parsing.

Test fixtures in messages.ts were updated to send proper author-attributed
inserts plus the matching apool (mirroring what the JS web client always
does). A new regression test 'insert without author attribute is rejected'
locks in the new behaviour.

* harden: also close the HTTP API / plugin path via stable system author

The first commit closed the socket.io USER_CHANGES hole. This commit closes
the parallel path through Pad.spliceText (used by API.setText, API.appendText,
the import flow, and plugins like ep_post_data) where an unattributed insert
would otherwise produce a malformed AText.

Approach: instead of REJECTING (which would break ep_post_data and many
existing tests that call setText/appendText without an authorId), substitute
a stable system author when none is provided. The resulting changeset is
properly attributed, the AText stays well-formed, and existing callers
continue to work unchanged. Plugins that want named author attribution
should still pass an explicit authorId (e.g., one allocated via
authorManager.createAuthor).

Pad.SYSTEM_AUTHOR_ID = 'a.etherpad-system' — a stable identifier that
appears in the pad's attribute pool when internal callers (HTTP API,
plugins, server-side imports) write text without naming an author. The
existing 'attribute changes by another author' protections still apply
to socket.io USER_CHANGES paths — a remote client can't impersonate the
system author for inserts (their session author check fires first).

Test:
- Pad.ts spec adds 'spliceText with empty authorId attributes to the
  system author' — verifies pad text lands AND the pool contains the
  system-author binding. Existing tests that pass an authorId are
  unaffected.

* harden: reject USER_CHANGES that would strand the trailing newline

Etherpad's pad text always ends with '\n'. _handleUserChanges previously
appended a separate `nlChangeset` correction revision whenever the
applied USER_CHANGES left the pad without a trailing '\n'. The stored
pad ended up well-formed, but the FIRST NEW_CHANGES broadcast (the
malformed user revision itself) reached browsers BEFORE the correction
did, and applyToAttribution's MergingOpAssembler aborts with
"line assembler not finished" on a non-'\n'-terminated doc — the
watching browser session then dropped the changeset and any subsequent
edits silently no-op'd until the user reloaded.

Replace the silent auto-correction with an explicit reject. Compute
`applyToText(rebasedChangeset, prevText)` before appendRevision; if the
result doesn't end with '\n', throw -> badChangeset disconnect. Clients
must emit USER_CHANGES whose application preserves the invariant —
this matches what the JS web client already does and forces non-JS
clients (etherpad-pad, third-party integrations) to surface their bugs
in their own logs instead of stranding the trailing newline in pad
revision history.

Also fixes a latent retransmission-detection bug surfaced by this PR's
author-attrib changes: moveOpsToNewPool renumbers `*N` references to
whatever slot the pad pool assigns, which can differ from the wire
form's slot. Comparing the raw client wire against the stored revision
form (`changeset === c`) then misses legitimate retransmissions and
the same edit gets duplicated. Snapshot the post-pool-mapping form
(`canonicalCs`) and compare that against `c` instead.

Backend test additions:
- 'changeset that would strand the trailing \\n is rejected' covers
  the new rejection path with wire `Z:6>1|1=6*0+1$X` against
  `hello\n`.
- handleMessageSecurity test now captures roSocket's own authorId and
  uses it in the apool sent through roSocket, because the prior PR
  commit made `*0` referencing the wrong author a hard reject.

All 1130 backend tests pass.

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

* chore: bump etherpad-cli-client to ^4.0.3

4.0.3 sends author-attributed inserts and preserves the trailing
newline, complying with this PR's tightened USER_CHANGES validation.
The rate-limit CI workflow drives the test pad via this client, so
without the bump the new server-side rejects fire on the very first
\`pad.append()\` and the rate-limit disconnect never gets a chance to
arrive — testlimits.sh exits 0 instead of 1 and the rate-limit job
fails with "ratelimit was not triggered when sending every 99 ms".

Refs ether/etherpad-cli-client#131

* harden: reject USER_CHANGES that name the reserved system author

The session-author equality check already rejects wire `*N` that
names a different real user, but `a.etherpad-system` is server-
internal — it's only used when spliceText / setText is called with
an empty authorId from HTTP API or plugin paths. A wire op that
names it is either a confused client or an attempt to launder
edits through a reserved attribution slot. Refuse.

Backend test 'insert claiming the reserved system author is
rejected' locks in the new behavior with wire `Z:1>5*0+5$hello`
plus an apool that maps slot 0 to `a.etherpad-system`. All 1131
backend tests pass.

Inline literal `'a.etherpad-system'` rather than importing the
constant from `Pad.SYSTEM_AUTHOR_ID` — `require('../db/Pad')` at
PadMessageHandler module scope returned a partially-initialized
class via the padManager circular path, leaving the static-field
access undefined at runtime.

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

---------

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

Author: JohnMcLear

pnpm-lock.yaml

@@ -93,6 +93,17 @@ exports.cleanText = (txt:string): string => txt.replace(/\r\n/g, '\n')
     .replace(/\t/g, '        ');
 
 class Pad {
+  /**
+   * Stable author id used to attribute inserts coming from internal callers
+   * (HTTP API setText/appendText with no authorId, plugins like ep_post_data,
+   * server-side import flows). Without ANY author attribute, pad.atext.text
+   * and pad.atext.attribs drift out of sync — clients then fail
+   * setDocAText reconciliation in ace2_inner.ts when loading the pad. Using
+   * a fixed system author keeps the AText well-formed without requiring
+   * every plugin to allocate its own author up-front.
+   */
+  static readonly SYSTEM_AUTHOR_ID = 'a.etherpad-system';
+
   private db: Database;
   private atext: AText;
   private pool: AttributePool;
@@ -415,9 +426,19 @@ class Pad {
         (!ins && start > 0 && orig[start - 1] === '\n');
     if (!willEndWithNewline) ins += '\n';
     if (ndel === 0 && ins.length === 0) return;
-    const attribs = authorId ? [['author', authorId] as [string, string]] : undefined;
+    // An unattributed insert (empty authorId + non-empty ins) would produce
+    // an AText where `text` and `attribs` disagree on length — clients fail
+    // setDocAText reconciliation on load. Backward-compat fix: if the caller
+    // didn't provide an authorId, attribute the insert to a stable system
+    // author. ep_post_data and other plugins that want named attribution
+    // should still pass an explicit authorId.
+    const effectiveAuthorId =
+        (ins.length > 0 && !authorId) ? Pad.SYSTEM_AUTHOR_ID : authorId;
+    const attribs = effectiveAuthorId
+        ? [['author', effectiveAuthorId] as [string, string]]
+        : undefined;
     const changeset = makeSplice(orig, start, ndel, ins, attribs, this.pool);
-    await this.appendRevision(changeset, authorId);
+    await this.appendRevision(changeset, effectiveAuthorId);
   }
 
   /**

src/node/db/Pad.ts

@@ -24,7 +24,7 @@ import {MapArrayType} from "../types/MapType";
 import AttributeMap from '../../static/js/AttributeMap';
 const padManager = require('../db/PadManager');
 const padDeletionManager = require('../db/PadDeletionManager');
-import {checkRep, cloneAText, compose, deserializeOps, follow, identity, inverse, makeAText, makeSplice, moveOpsToNewPool, mutateAttributionLines, mutateTextLines, oldLen, prepareForWire, splitAttributionLines, splitTextLines, unpack} from '../../static/js/Changeset';
+import {applyToText, checkRep, cloneAText, compose, deserializeOps, follow, identity, inverse, makeAText, moveOpsToNewPool, mutateAttributionLines, mutateTextLines, oldLen, prepareForWire, splitAttributionLines, splitTextLines, unpack} from '../../static/js/Changeset';
 import ChatMessage from '../../static/js/ChatMessage';
 import AttributePool from '../../static/js/AttributePool';
 const AttributeManager = require('../../static/js/AttributeManager');
@@ -875,12 +875,49 @@ const handleUserChanges = async (socket:any, message: {
                           `${opAuthorId} in changeset ${changeset}`);
         }
       }
+      // Reject '+' ops that do not carry the author attribute. The standard
+      // JS client always tags inserts with the author; rejecting unattributed
+      // inserts here keeps pad.atext.text and pad.atext.attribs in lock-step.
+      // Without this check, an insert op with empty attribs grows the text
+      // without contributing matching markers to the attribs string, leaving
+      // the stored AText in a state where the two iterables disagree on
+      // length — applyToAText then desyncs and breaks reconciliation in
+      // every client that later loads the pad.
+      if (op.opcode === '+' && !opAuthorId) {
+        throw new Error(`Author ${thisSession.author} submitted an insert without an ` +
+                        `author attribute in changeset ${changeset}`);
+      }
+      // Defense-in-depth: reject any wire-borne `*N` that resolves to the
+      // system author. The session-author equality check above already
+      // catches the case where `*N` claims a different real user as
+      // author, but `Pad.SYSTEM_AUTHOR_ID` is server-internal — it's
+      // only used when `spliceText` / `setText` are called with an empty
+      // authorId from HTTP API or plugin paths. No legitimate
+      // socket.io session ever writes as the system author, so a wire
+      // op that names it is either a confused client or an attempt to
+      // launder writes through a reserved attribution slot. Either way,
+      // refuse.
+      // Hardcoded mirror of `Pad.SYSTEM_AUTHOR_ID` from src/node/db/Pad.ts.
+      // A `const {Pad} = require('../db/Pad')` at module scope returned
+      // a partially-initialized class here (circular load via padManager),
+      // so the static-field access ended up undefined and short-circuited
+      // the check at runtime. Inline literal is the simplest fix.
+      if (opAuthorId === 'a.etherpad-system') {
+        throw new Error(`Author ${thisSession.author} attempted to submit changes as the ` +
+                        `reserved system author ${opAuthorId} in changeset ${changeset}`);
+      }
     }
 
     // ex. adoptChangesetAttribs
 
     // Afaik, it copies the new attributes from the changeset, to the global Attribute Pool
     let rebasedChangeset = moveOpsToNewPool(changeset, wireApool, pad.pool);
+    // Snapshot the post-pool-mapping form so the retransmission check below
+    // can recognise our changeset against the stored revision form. Comparing
+    // the raw client `changeset` against `c` would miss legitimate
+    // retransmissions whenever moveOpsToNewPool renumbered an attribute
+    // (e.g. `*0` -> `*1` because the pad pool already had something at slot 0).
+    const canonicalCs = rebasedChangeset;
 
     // ex. applyUserChanges
     let r = baseRev;
@@ -891,9 +928,9 @@ const handleUserChanges = async (socket:any, message: {
     while (r < pad.getHeadRevisionNumber()) {
       r++;
       const {changeset: c, meta: {author: authorId}} = await pad.getRevision(r);
-      if (changeset === c && thisSession.author === authorId) {
+      if (canonicalCs === c && thisSession.author === authorId) {
         // Assume this is a retransmission of an already applied changeset.
-        rebasedChangeset = identity(unpack(changeset).oldLen);
+        rebasedChangeset = identity(unpack(canonicalCs).oldLen);
       }
       // At this point, both "c" (from the pad) and "changeset" (from the
       // client) are relative to revision r - 1. The follow function
@@ -909,6 +946,22 @@ const handleUserChanges = async (socket:any, message: {
           `Can't apply changeset ${rebasedChangeset} with oldLen ` +
           `${oldLen(rebasedChangeset)} to document of length ${prevText.length}`);
     }
+    // Defensive: reject any rebased changeset whose application would leave
+    // the pad text not ending with '\n'. Previously the server silently
+    // appended a separate `nlChangeset` correction revision; that worked
+    // for the stored pad but the FIRST broadcast (the malformed user
+    // revision) reached browsers BEFORE the correction did, and the
+    // browser's line assembler asserts "line assembler not finished" on
+    // a doc that doesn't end with '\n', taking the session out. Refuse to
+    // accept such changesets — clients must always preserve the
+    // trailing-newline invariant.
+    const projectedText = applyToText(rebasedChangeset, prevText);
+    if (!projectedText.endsWith('\n')) {
+      throw new Error(
+          `Rejected USER_CHANGES whose application would leave the pad ` +
+          `without a trailing '\\n' (length ${projectedText.length}). ` +
+          `Every USER_CHANGES must preserve the "doc ends with \\n" invariant.`);
+    }
 
     const newRev = await pad.appendRevision(rebasedChangeset, thisSession.author);
     // The head revision will either stay the same or increase by 1 depending on whether the
@@ -920,12 +973,6 @@ const handleUserChanges = async (socket:any, message: {
       await pad.appendRevision(correctionChangeset, thisSession.author);
     }
 
-    // Make sure the pad always ends with an empty line.
-    if (pad.text().lastIndexOf('\n') !== pad.text().length - 1) {
-      const nlChangeset = makeSplice(pad.text(), pad.text().length - 1, 0, '\n');
-      await pad.appendRevision(nlChangeset, thisSession.author);
-    }
-
     // The client assumes that ACCEPT_COMMIT and NEW_CHANGES messages arrive in order. Make sure we
     // have already sent any previous ACCEPT_COMMIT and NEW_CHANGES messages.
     assert.equal(thisSession.rev, r);

src/node/handler/PadMessageHandler.ts

@@ -125,7 +125,7 @@
     "chokidar": "^5.0.0",
     "eslint": "^10.4.0",
     "eslint-config-etherpad": "^4.0.5",
-    "etherpad-cli-client": "^4.0.2",
+    "etherpad-cli-client": "^4.0.3",
     "mocha": "^11.7.5",
     "mocha-froth": "^0.2.10",
     "nodeify": "^1.0.1",

src/package.json

@@ -61,15 +61,36 @@ describe(__filename, function () {
       pad = await padManager.getPad(padId, '');
       // spliceText is an existing runtime Pad method; cast avoids
       // adding a type-only declaration to PadType in this PR.
-      await (pad as any).spliceText(0, 0, '100\u00a0km');
+      await (pad as any).spliceText(0, 0, '100\u00a0km', 'a.test');
       assert.equal(pad!.text(), '100\u00a0km\n');
     });
 
     it('setText round-trips U+00A0', async function () {
       pad = await padManager.getPad(padId, '');
-      await pad!.setText('a\u00a0b\n');
+      await pad!.setText('a\u00a0b\n', 'a.test');
       assert.equal(pad!.text(), 'a\u00a0b\n');
     });
+
+    it('spliceText with empty authorId attributes to the system author', async function () {
+      pad = await padManager.getPad(padId, '');
+      // An unattributed insert (empty authorId, non-empty ins) used to
+      // produce an AText where text and attribs disagreed on length \u2014
+      // clients then failed setDocAText reconciliation on load. The
+      // server now substitutes a stable system author so the AText stays
+      // well-formed without forcing every caller to allocate one up-front.
+      await (pad as any).spliceText(0, 0, 'plugin-text', '');
+      assert.equal(pad!.text(), 'plugin-text\n');
+      const pool: any = (pad as any).pool;
+      let sawSystemAuthor = false;
+      for (const k of Object.keys(pool.numToAttrib || {})) {
+        const a = pool.numToAttrib[k];
+        if (a[0] === 'author' && a[1] === 'a.etherpad-system') {
+          sawSystemAuthor = true;
+          break;
+        }
+      }
+      assert(sawSystemAuthor, 'expected system-author binding in pad pool');
+    });
   });
 
   describe('padDefaultContent hook', function () {