diff --git a/src/node/handler/PadMessageHandler.ts b/src/node/handler/PadMessageHandler.ts index 080ab044a0c..1894a689e7a 100644 --- a/src/node/handler/PadMessageHandler.ts +++ b/src/node/handler/PadMessageHandler.ts @@ -652,7 +652,12 @@ 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 restore other authors' + // IDs, but only if that author already exists in the pad's pool (i.e., they genuinely + // contributed to this pad). 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 +669,24 @@ 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}`); + if (op.opcode === '=') { + // Allow restoring author attributes on existing text (undo of clear authorship), + // but only if the author ID is already known to this pad. This prevents a user + // from attributing text to a fabricated author who never contributed to the pad. + const knownAuthor = pad.pool.putAttrib(['author', opAuthorId], true) !== -1; + if (!knownAuthor) { + throw new Error(`Author ${thisSession.author} tried to set unknown author ` + + `${opAuthorId} on existing text in changeset ${changeset}`); + } + } else { + // Reject '+' ops (inserting new text as another author) and '-' ops (deleting + // with another author's attribs). While '-' attribs are discarded from the + // document, they are added to the pad's attribute pool by moveOpsToNewPool, + // which could be exploited to inject fabricated author IDs into the pool and + // bypass the '=' op pool check above. + throw new Error(`Author ${thisSession.author} tried to submit changes as author ` + + `${opAuthorId} in changeset ${changeset}`); + } } } diff --git a/src/static/js/undomodule.ts b/src/static/js/undomodule.ts index c3bdd2fab9d..542fb7157ff 100644 --- a/src/static/js/undomodule.ts +++ b/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); } } diff --git a/src/tests/backend/specs/undo_clear_authorship.ts b/src/tests/backend/specs/undo_clear_authorship.ts new file mode 100644 index 00000000000..5ec73ca17f1 --- /dev/null +++ b/src/tests/backend/specs/undo_clear_authorship.ts @@ -0,0 +1,337 @@ +'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 or disconnect message, ignoring other messages. + * Uses a single persistent listener to avoid missing messages between on/off cycles. + */ + const waitForCommitOrDisconnect = (socket: any, timeoutMs = 10000): Promise => { + return new Promise((resolve, reject) => { + const timeout = setTimeout(() => { + socket.off('message', handler); + reject(new Error(`timed out waiting for ACCEPT_COMMIT or disconnect after ${timeoutMs}ms`)); + }, timeoutMs); + const handler = (msg: any) => { + if (msg.disconnect) { + clearTimeout(timeout); + socket.off('message', handler); + resolve(msg); + } else if (msg.type === 'COLLABROOM' && msg.data?.type === 'ACCEPT_COMMIT') { + clearTimeout(timeout); + socket.off('message', handler); + resolve(msg); + } + // Ignore USER_NEWINFO, NEW_CHANGES, etc. + }; + socket.on('message', handler); + }); + }; + + const waitForAcceptCommit = async (socket: any, wantRev: number) => { + const msg = await waitForCommitOrDisconnect(socket); + if (msg.disconnect) { + throw new Error(`Unexpected disconnect: ${JSON.stringify(msg)}`); + } + assert.equal(msg.data.newRev, wantRev); + }; + + /** + * Wait for a specific message type, ignoring others. Used for cross-user sync. + */ + const waitForNewChanges = (socket: any, timeoutMs = 10000): Promise => { + return new Promise((resolve, reject) => { + const timeout = setTimeout(() => { + socket.off('message', handler); + reject(new Error(`timed out waiting for NEW_CHANGES after ${timeoutMs}ms`)); + }, timeoutMs); + const handler = (msg: any) => { + if (msg.type === 'COLLABROOM' && msg.data?.type === 'NEW_CHANGES') { + clearTimeout(timeout); + socket.off('message', handler); + resolve(); + } + }; + socket.on('message', handler); + }); + }; + + 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) + const userB = await connectUser(); + socketB = userB.socket; + revB = userB.rev; + + // Step 4: User B types " world" with their author attribute + const apoolB = new AttributePool(); + apoolB.putAttrib(['author', userB.author]); + const userASeesB = waitForNewChanges(socketA); + 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 User B's change + await userASeesB; + 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; + + // 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. + 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 resultP = waitForCommitOrDisconnect(socketB); + await sendUserChanges(socketB, revB, undoChangeset, undoPool); + const msg = await resultP; + + 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; + + const userB = await connectUser(); + socketB = userB.socket; + revB = userB.rev; + + // User B tries to insert text attributed to User A - this should be rejected + const fakePool = new AttributePool(); + fakePool.putAttrib(['author', userA.author]); + + const resultP = waitForCommitOrDisconnect(socketB); + await sendUserChanges(socketB, revB, 'Z:1>5*0+5$hello', fakePool); + const msg = await resultP; + + assert.deepEqual(msg, {disconnect: 'badChangeset'}, + 'Should reject changeset that impersonates another author for new text'); + }); + + it('should reject = op with fabricated author who never contributed to the pad', async function () { + // Security: even for = ops, reject author IDs that don't exist in the pad's pool. + // This prevents attributing text to users who never touched the pad. + 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 tries to set a fabricated author ID on existing text via a = op + const fakePool = new AttributePool(); + fakePool.putAttrib(['author', 'a.fabricatedAuthorId']); + + const resultP = waitForCommitOrDisconnect(socketA); + await sendUserChanges(socketA, revA, 'Z:6>0*0=5$', fakePool); + const msg = await resultP; + + assert.deepEqual(msg, {disconnect: 'badChangeset'}, + 'Should reject = op with fabricated author not in pad pool'); + }); + + it('should reject - op with foreign author to prevent pool injection', async function () { + // Security: a '-' op with a foreign author's attribs should be rejected. + // While '-' attribs are discarded from the document, they are added to the + // pad's attribute pool by moveOpsToNewPool. Without this check, an attacker + // could inject a fabricated author ID into the pool via a '-' op, then use + // a '=' op to attribute text to that fabricated author. + 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 tries to delete a char with a fabricated author attrib via a - op + // This would inject the fabricated author into the pad pool + const fakePool = new AttributePool(); + fakePool.putAttrib(['author', 'a.fabricatedAuthorId']); + + const resultP = waitForCommitOrDisconnect(socketA); + // Delete 1 char with fabricated author attrib + await sendUserChanges(socketA, revA, 'Z:6<1*0-1$', fakePool); + const msg = await resultP; + + assert.deepEqual(msg, {disconnect: 'badChangeset'}, + 'Should reject - op with fabricated author to prevent pool injection'); + }); + }); +}); diff --git a/src/tests/frontend-new/specs/clear_authorship_color.spec.ts b/src/tests/frontend-new/specs/clear_authorship_color.spec.ts index 961deb4f4fc..f97f9e280d7 100644 --- a/src/tests/frontend-new/specs/clear_authorship_color.spec.ts +++ b/src/tests/frontend-new/specs/clear_authorship_color.spec.ts @@ -35,32 +35,37 @@ test('clear authorship color', async ({page}) => { }) -test("makes text clear authorship colors and checks it can't be undone", async function ({page}) { - const innnerPad = await getPadBody(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 padBody = await getPadBody(page); const padText = "Hello" // type some text await clearPadContent(page); await writeToPad(page, padText); - // get the first text element out of the inner iframe - const firstDivClass = innnerPad.locator('div').nth(0) - const retrievedClasses = await innnerPad.locator('div span').nth(0).getAttribute('class') - expect(retrievedClasses).toContain('author'); + // verify authorship exists on the span + await expect(padBody.locator('div span').first()).toHaveAttribute('class', /author-/); + // Accept the confirm dialog triggered by clearAuthorship when no text is selected + page.on('dialog', dialog => dialog.accept()); - await firstDivClass.focus() + // Click somewhere in the pad to deselect, then clear (triggers whole-pad clear via confirm) + await padBody.click(); await clearAuthorship(page); - expect(await firstDivClass.getAttribute('class')).not.toContain('author'); - await undoChanges(page); - const changedFirstDiv = innnerPad.locator('div').nth(0) - expect(await changedFirstDiv.getAttribute('class')).not.toContain('author'); + // verify authorship is cleared + await expect(padBody.locator('div span').first()).toHaveAttribute('class', /^(?!.*author-)/, {timeout: 5000}); + // Undo should restore authorship colors + await undoChanges(page); - await pressUndoButton(page); - const secondChangedFirstDiv = innnerPad.locator('div').nth(0) - expect(await secondChangedFirstDiv.getAttribute('class')).not.toContain('author'); + // verify authorship is restored and user is not disconnected + await expect(padBody.locator('div span').first()).toHaveAttribute('class', /author-/, {timeout: 5000}); + const disconnected = page.locator('.disconnected, .unreachable'); + await expect(disconnected).not.toBeVisible(); }); diff --git a/src/tests/frontend-new/specs/undo_clear_authorship.spec.ts b/src/tests/frontend-new/specs/undo_clear_authorship.spec.ts new file mode 100644 index 00000000000..099624405a9 --- /dev/null +++ b/src/tests/frontend-new/specs/undo_clear_authorship.spec.ts @@ -0,0 +1,133 @@ +import {expect, test} from "@playwright/test"; +import { + clearAuthorship, + clearPadContent, + getPadBody, + goToNewPad, + goToPad, + selectAllText, + undoChanges, + writeToPad +} from "../helper/padHelper"; + +/** + * Tests for https://github.com/ether/etherpad-lite/issues/2802 + * + * Reproduction steps from the bug report: + * 1. User A logs in, enables author colors, types something + * 2. User B logs in to same pad, enables author colors, types something + * 3. User B clicks "clear authorship colors" + * 4. User B clicks "undo" + * => User B is disconnected from the pad + * + * The undo of clear authorship re-applies author attributes for all authors, + * but the server rejects it because User B is submitting changes containing + * User A's author ID. + */ +test.describe('undo clear authorship colors with multiple authors (bug #2802)', function () { + test.describe.configure({ retries: 2 }); + let padId: string; + + test('User B should not be disconnected after undoing clear authorship', async function ({browser}) { + // User 1 creates a pad and types text + const context1 = await browser.newContext(); + const page1 = await context1.newPage(); + padId = await goToNewPad(page1); + const body1 = await getPadBody(page1); + await body1.click(); + await clearPadContent(page1); + await writeToPad(page1, 'Hello from User A'); + + // Wait for text to be committed + await page1.waitForTimeout(1000); + + // Verify User A's text has authorship + await expect(body1.locator('div span').first()).toHaveAttribute('class', /author-/); + + // User 2 joins the same pad in a different browser context (different author) + const context2 = await browser.newContext(); + const page2 = await context2.newPage(); + await goToPad(page2, padId); + const body2 = await getPadBody(page2); + + // Wait for User A's text to appear for User B + await expect(body2.locator('div').first()).toContainText('Hello from User A', {timeout: 10000}); + + // User B types on a new line + await body2.click(); + await page2.keyboard.press('End'); + await page2.keyboard.press('Enter'); + await page2.keyboard.type('Hello from User B'); + + // Both users should see both lines + await expect(body1.locator('div').nth(1)).toContainText('Hello from User B', {timeout: 15000}); + + // Verify we have authorship colors from two different authors + await expect(body2.locator('div span').first()).toHaveAttribute('class', /author-/); + + // Accept the confirm dialog that clearAuthorship triggers + page2.on('dialog', dialog => dialog.accept()); + + // User B clears authorship colors (without selecting - clears whole pad) + await clearAuthorship(page2); + + // Wait for clear to propagate and verify authorship is cleared + await expect(body2.locator('div span').first()).toHaveAttribute('class', /^(?!.*author-)/, {timeout: 5000}); + + // THIS IS THE BUG: User B undoes the clear authorship + await undoChanges(page2); + + // User B should NOT be disconnected + const disconnectedBanner = page2.locator('.disconnected, .unreachable'); + await expect(disconnectedBanner).not.toBeVisible(); + + // The authorship colors should be restored after undo + await expect(body2.locator('div span').first()).toHaveAttribute('class', /author-/, {timeout: 5000}); + + // User B should still be able to type (not disconnected) + await body2.click(); + await page2.keyboard.press('End'); + await page2.keyboard.press('Enter'); + await page2.keyboard.type('Still connected!'); + + // The text should appear for User A too (proves User B is still connected and syncing) + await expect(body1.locator('div').nth(2)).toContainText('Still connected!', {timeout: 15000}); + + // Cleanup + await context1.close(); + await context2.close(); + }); + + test('single user can undo clear authorship without disconnect', async function ({page}) { + // Even with a single user, undo of clear authorship should work + await goToNewPad(page); + const body = await getPadBody(page); + await body.click(); + await clearPadContent(page); + await writeToPad(page, 'Some text with authorship'); + + await page.waitForTimeout(500); + + // Verify authorship exists + await expect(body.locator('div span').first()).toHaveAttribute('class', /author-/); + + // Accept the confirm dialog + page.on('dialog', dialog => dialog.accept()); + + // Clear authorship (no selection - clears whole pad) + await clearAuthorship(page); + + // Verify cleared + await expect(body.locator('div span').first()).toHaveAttribute('class', /^(?!.*author-)/, {timeout: 5000}); + + // Undo should restore authorship + await undoChanges(page); + + // Should not be disconnected + const disconnectedBanner = page.locator('.disconnected, .unreachable'); + await expect(disconnectedBanner).not.toBeVisible(); + + // Authorship should be restored + await expect(body.locator('div span').first()).toHaveAttribute('class', /author-/, {timeout: 5000}); + }); +});