fix: allow undo of clear authorship colors without disconnect

[JohnMcLear]

Summary

• Server-side fix (PadMessageHandler.ts): The author validation now distinguishes between = ops (attribute changes on existing text — allows restoring other authors' attributes if they exist in the pad pool) and +/- ops (both reject foreign author IDs). The - op rejection prevents a pool injection attack where fabricated author IDs could be smuggled into the pad pool via delete attribs.
• Client-side workaround removed (undomodule.ts): The stopgap that prevented clearauthorship events from being pushed onto the undo stack is removed — users can now undo/redo clear authorship normally.
• Tests: 6 backend tests covering multi-author undo, clear with empty author, restore own author, impersonation rejection, fabricated author rejection, and - op pool injection prevention. 2 frontend Playwright tests for multi-user and single-user undo flows. Updated existing clear_authorship_color.spec.ts.

Context

Clicking "clear authorship colors" without any text selected clears the entire pad's authorship colors. When a user then presses Ctrl+Z to undo, the undo changeset re-applies the original author attributes for all authors who contributed text. The server's author validation rejected this because it treated any changeset containing another author's ID as impersonation, disconnecting the user with badChangeset.

The fix distinguishes between:

• = operations (applying attributes to existing text) — allows restoring previously-existing author attributes, gated by verifying the author ID exists in the pad's attribute pool
• + operations (inserting new text) — still rejects if attributed to another author
• - operations (deleting text) — still rejects if carrying another author's attribs (prevents pool injection: - attribs are discarded from the doc but added to pad.pool by moveOpsToNewPool, which could be exploited to bypass the = op pool check)

Known limitations

• A user can re-attribute existing text between authors who both already contributed to the pad via = ops. Full mitigation requires diffing against pad.atext at each character position (tracked separately). Timeslider would show correct author at previous revisions so if someone attempted to attribute text to an author this would be discoverable through the timeslider. Not ideal and something we need to patch moving forward. Because of that I will ask @SamTV12345 and @webzwo0i to review this and confirm if they are happy with this "fix" with the knowledge that in the future we should bake in more safety to ensure truthfulness in authorship attribution.

Test plan

• Backend: undo_clear_authorship.ts — 6 tests all passing locally
• Backend: existing messages.ts tests still pass (10/10)
• Frontend: clear_authorship_color.spec.ts updated — handles confirm dialog, expects undo to restore colors
• Frontend: undo_clear_authorship.spec.ts — multi-user and single-user undo scenarios
• Manual: Open pad with 2 users, both type, User B clears authorship (without selecting text), User B presses Ctrl+Z — should restore colors without disconnect

Fixes #2802

🤖 Generated with Claude Code

[JohnMcLear]

bump @SamTV12345 and @webzwo0i

[SamTV12345]

Is this MR ready for review? I could check it. Normally draft is not yet ready :) But I could also be mistaken.

[JohnMcLear]

@SamTV12345 yes please,

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Allow undo of clear authorship colors without disconnect

🐞 Bug fix

Walkthroughs

Description

• Allow undo of clear authorship colors without server disconnect
  - Distinguish between = ops (restore existing author attributes) and +/- ops (reject foreign
  authors)
  - Validate = ops against pad pool to prevent fabricated author attribution
• Remove client-side workaround blocking clear authorship from undo stack
• Add comprehensive backend and frontend tests for multi-author undo scenarios

Diagram

flowchart LR
  A["User clears authorship<br/>removes all author attribs"] -->|undo changeset| B["Re-applies author attribs<br/>for all contributors"]
  B -->|server validation| C{"Op type?"}
  C -->|"+ or -"| D["Reject if foreign author<br/>prevent impersonation"]
  C -->|"="| E["Check pad pool<br/>allow if author exists"]
  E -->|valid| F["Accept changeset<br/>undo succeeds"]
  D -->|invalid| G["Disconnect badChangeset"]
  E -->|fabricated| G

Loading

File Changes

1. src/node/handler/PadMessageHandler.ts 🐞 Bug fix +24/-3

Distinguish author validation by operation type

• Enhanced author validation to distinguish between operation types (=, +, -)
• = ops now allowed to restore other authors' attributes if they exist in pad pool
• + and - ops still reject foreign author IDs to prevent impersonation and pool injection
• Added detailed comments explaining the security rationale for each operation type

src/node/handler/PadMessageHandler.ts

---

2. src/static/js/undomodule.ts 🐞 Bug fix +1/-7

Remove clear authorship undo block workaround

• Removed client-side workaround that blocked clearauthorship events from undo stack
• Now all events including clear authorship are pushed to undo stack
• Allows users to undo/redo clear authorship normally

src/static/js/undomodule.ts

---

3. src/tests/backend/specs/undo_clear_authorship.ts 🧪 Tests +337/-0

Add backend tests for undo clear authorship bug

• New comprehensive backend test suite for multi-author undo scenarios
• Tests cover: multi-author undo without disconnect, empty author clear, own author restore,
 impersonation rejection, fabricated author rejection, pool injection prevention
• Implements helper functions for user connection, changeset submission, and message waiting
• Uses persistent socket listeners to avoid missing messages in CI

src/tests/backend/specs/undo_clear_authorship.ts

---

View more (2)

4. src/tests/frontend-new/specs/clear_authorship_color.spec.ts 🧪 Tests +19/-14

Update clear authorship test for undo support

• Updated existing test to verify undo of clear authorship restores author colors
• Changed from expecting undo to fail to expecting it to succeed
• Added dialog handler to accept confirm dialog triggered by whole-pad clear
• Uses robust Playwright assertions with regex patterns and auto-retry

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

---

5. src/tests/frontend-new/specs/undo_clear_authorship.spec.ts 🧪 Tests +133/-0

Add frontend tests for undo clear authorship

• New Playwright test suite for multi-user and single-user undo scenarios
• Multi-user test: two users type, one clears authorship, undoes without disconnect
• Single-user test: verifies undo works even with only one author
• Uses persistent dialog handlers and auto-retrying assertions for CI reliability
• Includes 15s timeouts for cross-user sync and retries for flakiness

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

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. Flaky class attribute test ☑ 🐞 Bug ☼ Reliability

Description

The Playwright tests assert authorship clearing via toHaveAttribute('class', /^(?!.*author-)/),
which fails if the span ends up with no class attribute after clearing authorship. This can cause
CI failures/flakiness unrelated to the actual feature behavior.

Code

src/tests/frontend-new/specs/clear_authorship_color.spec.ts[R59-60]

+  // verify authorship is cleared
+  await expect(padBody.locator('div span').first()).toHaveAttribute('class', /^(?!.*author-)/, {timeout: 5000});

Evidence

Authorship classes are only generated when the author attribute value is truthy; when author is
set to the empty string, no author-* class is emitted. Therefore, after clearing authorship, the
span can have an empty/absent class attribute, but the tests currently require the class attribute
to exist and match a regex.

src/tests/frontend-new/specs/clear_authorship_color.spec.ts[49-67]
src/tests/frontend-new/specs/undo_clear_authorship.spec.ts[65-76]
src/static/js/linestylefilter.ts[75-101]
src/static/js/ace2_inner.ts[591-603]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Tests use `toHaveAttribute('class', /^(?!.*author-)/)` to assert authorship is cleared. This assertion requires a `class` attribute to be present, but after `author=''` there may be no class attribute at all.
### Issue Context
`linestylefilter` skips attribs with falsy values (`if (!key || !value) continue`), so `author=''` produces no `author-*` class.
### Fix Focus Areas
- src/tests/frontend-new/specs/clear_authorship_color.spec.ts[59-61]
- src/tests/frontend-new/specs/undo_clear_authorship.spec.ts[74-76]
### Suggested fix
Prefer assertions that pass when the attribute is missing, e.g.:
- `await expect(locator).not.toHaveAttribute('class', /author-/);`
- or `await expect(locator).not.toHaveClass(/author-/);`
- or explicitly allow null/empty: `const cls = await locator.getAttribute('class'); expect(cls ?? '').not.toMatch(/author-/);`

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Authorship reattribution possible 🐞 Bug ⛨ Security

Description

handleUserChanges now allows '=' ops to set the 'author' attribute to any author ID already present
in pad.pool, so any editor can re-attribute existing text to another real contributor. This is an
integrity/security risk because it can mislead users about who authored text (even if it’s an
intentional trade-off for undo support).

Code

src/node/handler/PadMessageHandler.ts[R672-689]

+        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}`);
+        }

Evidence

The new validation explicitly permits foreign author IDs on '=' ops if the author attribute exists
in the pad’s attribute pool. The pad’s pool is populated with each revision’s authorId, so any past
editor’s authorId will typically be “known” and therefore eligible to be applied to arbitrary
existing text via '=' ops.

src/node/handler/PadMessageHandler.ts[655-689]
src/node/db/Pad.ts[97-109]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Server now accepts `=` ops that set `author` to any ID already present in `pad.pool`. This enables intentional re-attribution of existing text to other real contributors (authorship integrity issue).
### Issue Context
The change is needed to support undo of `clearauthorship`, but the current check (`author exists in pad.pool`) is broader than “undo restore” and does not ensure the author attribution being applied matches the prior attribution of the affected text.
### Fix Focus Areas
- src/node/handler/PadMessageHandler.ts[655-689]
### Suggested direction
Limit the exception to the minimum necessary for undo/redo of clear-authorship, for example by:
- Allowing foreign `author` IDs on `=` ops only if the operation is restoring from `author=''` (or otherwise matches the pre-change attribution), which requires diffing against `pad.atext` over the affected ranges; or
- Special-casing only changesets that are the inverse of a clear-authorship operation, rather than allowing arbitrary `=` author mutations.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[SamTV12345]

So we now basically push all the events? Even if it is clearAuthorship?

[JohnMcLear]

Yes, take your time to read the Known limitations part... We're basically just moving the problem away from it being a broken UX to a potential untruth of authorship. It's highly unlikely anyone would ever exploit it AND there would be revision history to show who/how/when it was exploited so it's a bit like spitting on your fingerprints to hide your criminality.. All it takes is a curious eye to swab test for DNA and you are still busted....

[JohnMcLear]

Yes, intentionally. The old eventType !== 'clearauthorship' filter was a client-side workaround added in #2802 because the server was disconnecting users whose undo changeset reapplied another author's colors (it treated any foreign author ID as impersonation).

The real fix is server-side: Pad.ts now distinguishes + ops (new text — still reject foreign author) from = ops (attribute-only changes — allow restoring other authors' colors for undo). With that in place, the client filter is no longer needed and clear-authorship becomes undoable, which is the point of this PR.

Merge-same-type deduplication and identity-backset skipping still apply, so we're not "push all events unconditionally" — we just stopped singling out clearauthorship.

[SamTV12345]

True so I guess the impact is low. Yeah I am in favor of not creating some client side special exceptions for events. Good work. Feel free to merge when you have all the qodo warnings removed.