fix: allow undo of clear authorship colors without disconnect#7430
Draft
JohnMcLear wants to merge 5 commits intoether:developfrom
Draft
fix: allow undo of clear authorship colors without disconnect#7430JohnMcLear wants to merge 5 commits intoether:developfrom
JohnMcLear wants to merge 5 commits intoether:developfrom
Conversation
JohnMcLear
force-pushed
the
fix/undo-clear-authorship-2802
branch
from
891d1b8 to
6e83dd7
Compare
Member
Author
|
bump @SamTV12345 and @webzwo0i |
…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: ether#2802 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- 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>
- 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>
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>
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>
JohnMcLear
force-pushed
the
fix/undo-clear-authorship-2802
branch
from
eba9b12 to
1aefafa
Compare
Member
|
Is this MR ready for review? I could check it. Normally draft is not yet ready :) But I could also be mistaken. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
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.undomodule.ts): The stopgap that preventedclearauthorshipevents from being pushed onto the undo stack is removed — users can now undo/redo clear authorship normally.-op pool injection prevention. 2 frontend Playwright tests for multi-user and single-user undo flows. Updated existingclear_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 topad.poolbymoveOpsToNewPool, which could be exploited to bypass the=op pool check)Known limitations
=ops. Full mitigation requires diffing againstpad.atextat 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
undo_clear_authorship.ts— 6 tests all passing locallymessages.tstests still pass (10/10)clear_authorship_color.spec.tsupdated — handles confirm dialog, expects undo to restore colorsundo_clear_authorship.spec.ts— multi-user and single-user undo scenariosFixes #2802
🤖 Generated with Claude Code