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>

Author: JohnMcLear

src/node/handler/PadMessageHandler.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');
@@ -893,6 +893,12 @@ const handleUserChanges = async (socket:any, message: {
 
     // 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;
@@ -903,9 +909,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
@@ -921,6 +927,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
@@ -932,12 +954,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/tests/backend/specs/messages.ts

@@ -24,6 +24,7 @@ describe(__filename, function () {
   });
 
   let authorId: string;
+  let roAuthorId: string;
   beforeEach(async function () {
     backups.hooks = {handleMessageSecurity: plugins.hooks.handleMessageSecurity};
     plugins.hooks.handleMessageSecurity = [];
@@ -42,7 +43,13 @@ describe(__filename, function () {
     roPadId = await readOnlyManager.getReadOnlyId(padId);
     res = await agent.get(`/p/${roPadId}`).expect(200);
     roSocket = await common.connect(res);
-    await common.handshake(roSocket, roPadId);
+    const roHandshake = await common.handshake(roSocket, roPadId);
+    // Capture roSocket's own author so tests that send USER_CHANGES via
+    // roSocket can build apools that match thisSession.author server-side;
+    // otherwise the wire's *0 reference points at the writer-socket's
+    // author and the server rejects with badChangeset on the
+    // "author mismatch" check added in this PR.
+    roAuthorId = (roHandshake as any).data.userId;
     await new Promise(resolve => setTimeout(resolve, 1000));
   });
 
@@ -214,6 +221,34 @@ describe(__filename, function () {
       ]);
     });
 
+    it('changeset that would strand the trailing \\n is rejected', async function () {
+      // Defensive validation: every USER_CHANGES must leave the pad ending
+      // with '\n'. The pre-existing handler used to auto-append a
+      // correction revision when the trailing '\n' got stranded, but the
+      // first NEW_CHANGES broadcast (the malformed user revision) reached
+      // browsers BEFORE the correction did, and the browser's line
+      // assembler asserts "line assembler not finished" on a non-'\n'-
+      // terminated doc — kicking the watching session offline. Refuse such
+      // changesets up front instead.
+      //
+      // Seed the pad as 'hello\n' (6 chars, 1 line), then send a changeset
+      // that explicitly keeps all 6 chars (consuming the trailing '\n')
+      // and inserts 'X' AFTER. Projected text = 'hello\nX' — doesn't end
+      // with '\n', must be rejected. The keep spans 1 newline so the wire
+      // must carry the `|1` marker to be in canonical form.
+      await Promise.all([
+        assertAccepted(socket, rev + 1),
+        sendUserChanges(socket, 'Z:1>5*0+5$hello'),
+      ]);
+      assert.equal(pad!.text(), 'hello\n');
+      await Promise.all([
+        assertRejected(socket),
+        sendUserChanges(socket, 'Z:6>1|1=6*0+1$X'),
+      ]);
+      // Pad must be unchanged after the rejection.
+      assert.equal(pad!.text(), 'hello\n');
+    });
+
     it('retransmission is accepted, has no effect', async function () {
       const cs = 'Z:1>5*0+5$hello';
       await Promise.all([
@@ -254,9 +289,15 @@ describe(__filename, function () {
 
     it('handleMessageSecurity can grant one-time write access', async function () {
       const cs = 'Z:1>5*0+5$hello';
+      // Use roSocket's own author in the apool so the *0 reference matches
+      // thisSession.author server-side. Without this, the new author-attrib
+      // validation rejects the changeset before handleMessageSecurity gets
+      // a chance to permit it.
+      const roPool = () =>
+          ({numToAttrib: {0: ['author', roAuthorId]}, nextNum: 1});
       const errRegEx = /write attempt on read-only pad/;
       // First try to send a change and verify that it was dropped.
-      await assert.rejects(sendUserChanges(roSocket, cs), errRegEx);
+      await assert.rejects(sendUserChanges(roSocket, cs, roPool()), errRegEx);
       // sendUserChanges() waits for message ack, so if the message was accepted then head should
       // have already incremented by the time we get here.
       assert.equal(pad!.head, rev); // Not incremented.
@@ -265,13 +306,14 @@ describe(__filename, function () {
       plugins.hooks.handleMessageSecurity.push({hook_fn: () => 'permitOnce'});
       await Promise.all([
         assertAccepted(roSocket, rev + 1),
-        sendUserChanges(roSocket, cs),
+        sendUserChanges(roSocket, cs, roPool()),
       ]);
       assert.equal(pad!.text(), 'hello\n');
 
       // The next change should be dropped.
       plugins.hooks.handleMessageSecurity = [];
-      await assert.rejects(sendUserChanges(roSocket, 'Z:6>6=5*0+6$ world'), errRegEx);
+      await assert.rejects(
+          sendUserChanges(roSocket, 'Z:6>6=5*0+6$ world', roPool()), errRegEx);
       assert.equal(pad!.head, rev); // Not incremented.
       assert.equal(pad!.text(), 'hello\n');
     });