fix: allow undo of clear authorship colors without disconnect (#2802)

When a user clears authorship colors and then undoes, the undo changeset
re-applies author attributes for all authors who contributed text. The
server was rejecting this because it treated any changeset containing
another author's ID as impersonation, disconnecting the user.

The fix distinguishes between:
- '+' ops (new text): still reject if attributed to another author
- '=' ops (attribute changes on existing text): allow restoring other
  authors' attributes, which is needed for undo of clear authorship

Also removes the client-side workaround in undomodule.ts that prevented
clear authorship from being undone at all, and adds backend + frontend
tests covering the multi-author undo scenario.

Fixes: #2802

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

Author: JohnMcLear

src/node/handler/PadMessageHandler.ts

@@ -652,7 +652,10 @@ const handleUserChanges = async (socket:any, message: {
     // Verify that the changeset has valid syntax and is in canonical form
     checkRep(changeset);
 
-    // Validate all added 'author' attribs to be the same value as the current user
+    // Validate all added 'author' attribs to be the same value as the current user.
+    // Exception: '=' ops (attribute changes on existing text) are allowed to set other authors'
+    // IDs. This is necessary for undoing "clear authorship colors", which re-applies the original
+    // author attributes for all authors. See https://github.com/ether/etherpad-lite/issues/2802
     for (const op of deserializeOps(unpack(changeset).ops)) {
       // + can add text with attribs
       // = can change or add attribs
@@ -664,8 +667,12 @@ const handleUserChanges = async (socket:any, message: {
       // an attribute number isn't in the pool).
       const opAuthorId = AttributeMap.fromString(op.attribs, wireApool).get('author');
       if (opAuthorId && opAuthorId !== thisSession.author) {
-        throw new Error(`Author ${thisSession.author} tried to submit changes as author ` +
-                        `${opAuthorId} in changeset ${changeset}`);
+        // Allow '=' ops to restore other authors' attributes on existing text (undo of clear
+        // authorship). Only reject '+' ops that insert new text as another author.
+        if (op.opcode === '+') {
+          throw new Error(`Author ${thisSession.author} tried to submit changes as author ` +
+                          `${opAuthorId} in changeset ${changeset}`);
+        }
       }
     }
 

src/static/js/undomodule.ts

@@ -212,13 +212,7 @@ const undoModule = (() => {
         }
       }
       if (!merged) {
-        /*
-         * Push the event on the undo stack only if it exists, and if it's
-         * not a "clearauthorship". This disallows undoing the removal of the
-         * authorship colors, but is a necessary stopgap measure against
-         * https://github.com/ether/etherpad-lite/issues/2802
-         */
-        if (event && (event.eventType !== 'clearauthorship')) {
+        if (event) {
           stack.pushEvent(event);
         }
       }

src/tests/backend/specs/undo_clear_authorship.ts

@@ -0,0 +1,274 @@
+'use strict';
+
+/**
+ * Tests for https://github.com/ether/etherpad-lite/issues/2802
+ *
+ * When User B clears authorship colors (removing all author attributes) and then undoes,
+ * the undo changeset re-applies author attributes for ALL authors (A and B). The server
+ * rejects this because User B is submitting changes containing User A's author ID, causing
+ * a disconnect with "badChangeset".
+ *
+ * The server should allow undo of clear authorship without disconnecting the user.
+ */
+
+import {PadType} from "../../../node/types/PadType";
+
+const assert = require('assert').strict;
+const common = require('../common');
+const padManager = require('../../../node/db/PadManager');
+import AttributePool from '../../../static/js/AttributePool';
+import padutils from '../../../static/js/pad_utils';
+
+describe(__filename, function () {
+  let agent: any;
+  let pad: PadType | null;
+  let padId: string;
+  let socketA: any;
+  let socketB: any;
+  let revA: number;
+  let revB: number;
+
+  before(async function () {
+    agent = await common.init();
+  });
+
+  beforeEach(async function () {
+    padId = common.randomString();
+    assert(!await padManager.doesPadExist(padId));
+    pad = await padManager.getPad(padId, '');
+    await pad!.setText('\n');
+    assert.equal(pad!.text(), '\n');
+  });
+
+  afterEach(async function () {
+    if (socketA != null) socketA.close();
+    socketA = null;
+    if (socketB != null) socketB.close();
+    socketB = null;
+    if (pad != null) await pad.remove();
+    pad = null;
+  });
+
+  /**
+   * Connect a user to the pad with a unique author token.
+   */
+  const connectUser = async () => {
+    const res = await agent.get(`/p/${padId}`).expect(200);
+    const socket = await common.connect(res);
+    const token = padutils.generateAuthorToken();
+    const {type, data: clientVars} = await common.handshake(socket, padId, token);
+    assert.equal(type, 'CLIENT_VARS');
+    const rev = clientVars.collab_client_vars.rev;
+    const author = clientVars.userId;
+    return {socket, rev, author};
+  };
+
+  const sendUserChanges = async (socket: any, baseRev: number, changeset: string, apool?: any) => {
+    await common.sendUserChanges(socket, {
+      baseRev,
+      changeset,
+      ...(apool ? {apool} : {}),
+    });
+  };
+
+  /**
+   * Wait for an ACCEPT_COMMIT message, skipping any other COLLABROOM messages
+   * (like USER_NEWINFO, NEW_CHANGES, etc.) that may arrive first.
+   */
+  const waitForAcceptCommit = async (socket: any, wantRev: number) => {
+    for (;;) {
+      const msg = await common.waitForSocketEvent(socket, 'message');
+      if (msg.disconnect) {
+        throw new Error(`Unexpected disconnect: ${JSON.stringify(msg)}`);
+      }
+      if (msg.type === 'COLLABROOM' && msg.data?.type === 'ACCEPT_COMMIT') {
+        assert.equal(msg.data.newRev, wantRev);
+        return;
+      }
+      // Skip non-ACCEPT_COMMIT messages (USER_NEWINFO, NEW_CHANGES, etc.)
+    }
+  };
+
+  /**
+   * Drain messages from a socket until we get an ACCEPT_COMMIT or disconnect.
+   * Returns the message for assertion.
+   */
+  const waitForNextCommitOrDisconnect = async (socket: any): Promise<any> => {
+    for (;;) {
+      const msg = await common.waitForSocketEvent(socket, 'message');
+      if (msg.disconnect) return msg;
+      if (msg.type === 'COLLABROOM' && msg.data?.type === 'ACCEPT_COMMIT') return msg;
+      // Skip USER_NEWINFO, NEW_CHANGES, etc.
+    }
+  };
+
+  /**
+   * Drain non-ACCEPT_COMMIT messages so the socket is ready for the next operation.
+   * Waits briefly then consumes any queued messages.
+   */
+  const drainMessages = async (socket: any) => {
+    await new Promise(resolve => setTimeout(resolve, 500));
+  };
+
+  describe('undo of clear authorship colors (bug #2802)', function () {
+    it('should not disconnect when undoing clear authorship with multiple authors', async function () {
+      this.timeout(30000);
+
+      // Step 1: Connect User A
+      const userA = await connectUser();
+      socketA = userA.socket;
+      revA = userA.rev;
+
+      // Step 2: User A types "hello" with their author attribute
+      const apoolA = new AttributePool();
+      apoolA.putAttrib(['author', userA.author]);
+      await Promise.all([
+        waitForAcceptCommit(socketA, revA + 1),
+        sendUserChanges(socketA, revA, 'Z:1>5*0+5$hello', apoolA),
+      ]);
+      revA += 1;
+
+      // Step 3: Connect User B (after User A's text is committed)
+      await drainMessages(socketA);
+      const userB = await connectUser();
+      socketB = userB.socket;
+      revB = userB.rev;
+      // User B joins and sees the pad at the current head revision
+      await drainMessages(socketA);
+
+      // Step 4: User B types " world" with their author attribute
+      const apoolB = new AttributePool();
+      apoolB.putAttrib(['author', userB.author]);
+      await Promise.all([
+        waitForAcceptCommit(socketB, revB + 1),
+        sendUserChanges(socketB, revB, 'Z:6>6=5*0+6$ world', apoolB),
+      ]);
+      revB += 1;
+
+      // Wait for User A to see the change
+      await drainMessages(socketA);
+      revA = revB;
+
+      // The pad now has "hello world\n" with two different authors
+      assert.equal(pad!.text(), 'hello world\n');
+
+      // Step 5: User B clears authorship colors (sets author to '' on all text)
+      const clearPool = new AttributePool();
+      clearPool.putAttrib(['author', '']);
+      await Promise.all([
+        waitForAcceptCommit(socketB, revB + 1),
+        sendUserChanges(socketB, revB, 'Z:c>0*0=b$', clearPool),
+      ]);
+      revB += 1;
+      await drainMessages(socketA);
+      revA = revB;
+
+      // Step 6: User B undoes the clear authorship
+      // This is the critical part - the undo changeset re-applies the original
+      // author attributes, which include User A's author ID.
+      // The server currently rejects this because User B is submitting changes
+      // with User A's author ID.
+      const undoPool = new AttributePool();
+      undoPool.putAttrib(['author', userA.author]); // 0 = author A
+      undoPool.putAttrib(['author', userB.author]); // 1 = author B
+      // Undo restores: "hello" with author A (5 chars), " world" with author B (6 chars)
+      const undoChangeset = 'Z:c>0*0=5*1=6$';
+
+      // This should NOT disconnect User B - that's the bug (#2802)
+      const result = await Promise.all([
+        waitForNextCommitOrDisconnect(socketB),
+        sendUserChanges(socketB, revB, undoChangeset, undoPool),
+      ]);
+
+      const msg = result[0];
+      assert.notDeepEqual(msg, {disconnect: 'badChangeset'},
+          'User was disconnected with badChangeset - bug #2802');
+      assert.equal(msg.type, 'COLLABROOM');
+      assert.equal(msg.data.type, 'ACCEPT_COMMIT');
+    });
+
+    it('should allow clear authorship changeset with empty author from any user', async function () {
+      // Connect one user, write text, then clear authorship
+      const userA = await connectUser();
+      socketA = userA.socket;
+      revA = userA.rev;
+
+      // User A types text
+      const apoolA = new AttributePool();
+      apoolA.putAttrib(['author', userA.author]);
+      await Promise.all([
+        waitForAcceptCommit(socketA, revA + 1),
+        sendUserChanges(socketA, revA, 'Z:1>5*0+5$hello', apoolA),
+      ]);
+      revA += 1;
+
+      // User A clears authorship (sets author='')
+      // This should always be allowed since author='' is not impersonation
+      const clearPool = new AttributePool();
+      clearPool.putAttrib(['author', '']);
+      await Promise.all([
+        waitForAcceptCommit(socketA, revA + 1),
+        sendUserChanges(socketA, revA, 'Z:6>0*0=5$', clearPool),
+      ]);
+    });
+
+    it('changeset restoring own author after clear should be accepted', async function () {
+      // User clears their own authorship and then undoes (restoring own author attr)
+      const userA = await connectUser();
+      socketA = userA.socket;
+      revA = userA.rev;
+
+      // User A types text with author attribute
+      const apoolA = new AttributePool();
+      apoolA.putAttrib(['author', userA.author]);
+      await Promise.all([
+        waitForAcceptCommit(socketA, revA + 1),
+        sendUserChanges(socketA, revA, 'Z:1>5*0+5$hello', apoolA),
+      ]);
+      revA += 1;
+
+      // User A clears authorship (sets author='')
+      const clearPool = new AttributePool();
+      clearPool.putAttrib(['author', '']);
+      await Promise.all([
+        waitForAcceptCommit(socketA, revA + 1),
+        sendUserChanges(socketA, revA, 'Z:6>0*0=5$', clearPool),
+      ]);
+      revA += 1;
+
+      // User A undoes the clear - restoring their own author attribute
+      // This should be accepted since it's their own author ID
+      await Promise.all([
+        waitForAcceptCommit(socketA, revA + 1),
+        sendUserChanges(socketA, revA, 'Z:6>0*0=5$', apoolA),
+      ]);
+    });
+
+    it('changeset impersonating another author for new text should still be rejected', async function () {
+      // Security: a user should NOT be able to write NEW text attributed to another author
+      const userA = await connectUser();
+      socketA = userA.socket;
+      revA = userA.rev;
+
+      await drainMessages(socketA);
+
+      const userB = await connectUser();
+      socketB = userB.socket;
+      revB = userB.rev;
+
+      await drainMessages(socketA);
+
+      // User B tries to insert text attributed to User A - this should be rejected
+      const fakePool = new AttributePool();
+      fakePool.putAttrib(['author', userA.author]);
+
+      const result = await Promise.all([
+        waitForNextCommitOrDisconnect(socketB),
+        sendUserChanges(socketB, revB, 'Z:1>5*0+5$hello', fakePool),
+      ]);
+
+      assert.deepEqual(result[0], {disconnect: 'badChangeset'},
+          'Should reject changeset that impersonates another author for new text');
+    });
+  });
+});

src/tests/frontend-new/specs/clear_authorship_color.spec.ts

@@ -38,7 +38,10 @@ test('clear authorship color', async ({page}) => {
 })
 
 
-test("makes text clear authorship colors and checks it can't be undone", async function ({page}) {
+test("clear authorship colors can be undone to restore author colors", async function ({page}) {
+  // Fix for https://github.com/ether/etherpad-lite/issues/2802
+  // Previously, undo of clear authorship was blocked as a workaround.
+  // Now the server properly allows it, so undo should restore author colors.
   const innnerPad = await getPadBody(page);
   const padText = "Hello"
 
@@ -51,19 +54,20 @@ test("makes text clear authorship colors and checks it can't be undone", async f
   const retrievedClasses = await innnerPad.locator('div span').nth(0).getAttribute('class')
   expect(retrievedClasses).toContain('author');
 
-
   await firstDivClass.focus()
   await clearAuthorship(page);
   expect(await firstDivClass.getAttribute('class')).not.toContain('author');
 
+  // Undo should restore authorship colors
   await undoChanges(page);
-  const changedFirstDiv = innnerPad.locator('div').nth(0)
-  expect(await changedFirstDiv.getAttribute('class')).not.toContain('author');
-
-
-  await pressUndoButton(page);
-  const secondChangedFirstDiv = innnerPad.locator('div').nth(0)
-  expect(await secondChangedFirstDiv.getAttribute('class')).not.toContain('author');
+  await page.waitForTimeout(1000);
+  const spanAfterUndo = innnerPad.locator('div span').nth(0);
+  const classesAfterUndo = await spanAfterUndo.getAttribute('class');
+  expect(classesAfterUndo).toContain('author');
+
+  // User should not be disconnected
+  const disconnected = page.locator('.disconnected, .unreachable');
+  expect(await disconnected.isVisible()).toBe(false);
 });