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

* 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: https://github.com/ether/etherpad-lite/issues/2802

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

* fix: use robust Playwright assertions in authorship undo tests

- Use toHaveAttribute with regex instead of raw getAttribute + toContain
- Check div/span attributes within pad body instead of broad selectors
- Use Playwright auto-retry (expect with timeout) instead of toHaveCount(0)

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

* fix: handle confirm dialog and sync timing in Playwright tests

- Add page.on('dialog') handler to accept the confirm dialog triggered
  by clearAuthorship when no text is selected (clears whole pad)
- Use auto-retrying toHaveAttribute assertions instead of raw getAttribute
- Increase cross-user sync timeouts to 15s for CI reliability
- Add retries: 2 to multi-user test for CI flakiness
- Scope assertions to pad body spans instead of broad selectors

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

* fix: use persistent socket listeners to avoid missing messages in CI

Replace sequential waitForSocketEvent loops with single persistent
listeners that filter messages inline. This prevents race conditions
where messages arrive between off/on listener cycles, causing timeouts
on slower CI runners.

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

* fix: reject - ops with foreign author to prevent pool injection

The '-' op attribs are discarded from the document but still get 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 (bypassing
the pool existence check).

Now all non-'=' ops (+, -) with foreign author IDs are rejected.

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

* test: use not.toHaveClass for cleared authorship spans

Addresses Qodo review: linestylefilter skips attribs with empty values,
so a span with author='' has no class attribute at all. The previous
negative-lookahead regex on the class attribute failed against a null
attribute and was flaky in CI. Switch to not.toHaveClass(/author-/),
which also passes when the attribute is missing.

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

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
John McLear 2026-04-19 09:09:03 +01:00 committed by GitHub
parent 84060fb538
commit 4137109efe
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 516 additions and 24 deletions

View file

@ -651,7 +651,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
@ -663,8 +668,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}`);
}
}
}