<!--IssueSummary start--> <details> <summary> Everyone can contribute. [Help move this issue forward](https://handbook.gitlab.com/handbook/marketing/developer-relations/engineering/community-contributors-workflows/#contributor-links) while earning points, leveling up and collecting rewards. </summary> - [Close this issue](https://contributors.gitlab.com/manage-issue?action=close&projectId=278964&issueIid=629400) </details> <!--IssueSummary end--> <:robot:> This issue is the single pre-enablement checklist for the `wiki_collaborative_editing` feature flag. Nothing here blocks the merge requests that are still in flight, and everything is behind the flag, which is off by default. The flag must not be enabled until these are resolved or consciously accepted. It began as review follow-ups from [!254876](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/254876) (slice 4). Items previously recorded on [!250607#note_3802654087](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/250607#note_3802654087) have been moved here, and that note now points back to this issue. Every item was raised in review. **Highest risk.** Items 8, 9, 14 and 16 can each lose or misattribute a user's work without telling anyone. They deserve attention before the rest. ## Must-do ### 1. Clear `#resendFullState` on server acknowledgement, not on local send `#send` returns `true` when ActionCable accepts the message locally, not when the server keeps it. The server silently discards on rate-limiting (`rate_limited?`), invalid payload (`valid_payload?`), or a stale/expired compaction claim (`store.replace` returning `false`). Because that return value clears the recovery flag, offline or dropped edits can be **permanently lost** while the client believes they were delivered. `CollaborationCursor` updates awareness on every selection change with no merging, so holding an arrow key on a large document can burn through the 600/min limit and trigger this. Suggested fix: clear `#resendFullState` only on a server-side acknowledgement rather than on local WebSocket delivery. Raised by @kivikakk (*"fine to do in a follow-up, but I think we should address it"*) and independently by the AppSec reviewer. See [note_3840186025](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/254876#note_3840186025). ### 2. Make the compaction-token exclusion explicit in `handle_snapshot` `handle_snapshot` passes the full `data` hash (including the client-supplied `token`) into `broadcast()`, relying on `broadcast`'s internal `data.slice('type', 'payload', 'clientId')` to drop it. If `broadcast` is ever extended to forward more fields, the token leaks to all channel subscribers, who could replay a snapshot replacement. Slice at the call site instead: ```ruby # Current broadcast(data.merge('type' => MESSAGE_TYPE_SYNC)) # Fixed broadcast(data.slice('payload', 'clientId').merge('type' => MESSAGE_TYPE_SYNC)) ``` AppSec design finding, agreed by @kivikakk (*"I do really like the explicitness proposed here, and also recommend adding it to a follow-up"*). See [note_3840258007](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/254876#note_3840258007). ## Lighter-weight ### 3. Reconsider the partial-apply tradeoff on malformed payloads The per-entry guard sits inside `doc.transact`, so a payload failing midway can leave a partially applied update in the document. Alternatives raised: (a) load server content non-collaboratively and disable sync, or (b) fail with a user-visible error message. *"quietly accepting partially applied updates doesn't feel super great."* See [note_3840186039](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/254876#note_3840186039). ### 4. Fix `awareness.destroy()` ordering on teardown `awareness.destroy()` runs after `unsubscribe()`, so the `setLocalState(null)` departure announcement never reaches peers; they keep showing the departed editor until the 30s timeout. Call `awareness.destroy()` before `unsubscribe()`. See [note_3840187778](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/254876#note_3840187778). ### 5. Document the cursor-mislabelling security property The sender chooses `clientId`, so a peer can mislabel someone else's cursor as their own (the name is server-stamped and cannot be spoofed). Limited impact, but the security properties should be written down. See [note_3840187789](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/254876#note_3840187789). Detail moved here from the !250607 review, which tracked this separately as "`clientId` identity spoofing". `CollaborativeEditing::BaseChannel#broadcast` slices `clientId` from client-supplied data and then attaches the server-derived `user`, so the server stamps the real user onto whatever `clientId` the sender supplies. The awareness payload carries client IDs too, which peers accept for any ID. A participant can put another participant's ID on a message and have peers show the wrong name against a cursor, or move someone else's cursor while it still shows their name. Everyone in a session already has write access, so this is not a privilege gain and it was rated low impact. It is not fully fixable server-side without parsing Yjs in Ruby, which this design deliberately avoids, and the review suggested accepting it. Partial mitigations, if wanted: pin the `clientId` per subscription on first message and drop later messages using a different one, allowing `0` while the subscription holds the seed; client side, drop awareness entries whose inner client ID does not match the message `clientId`; client side, give `CollaborationCursor` a `selectionRender` taking its colour from the server-stamped identity rather than peer-authored state. Also noted by @kivikakk in [!256514#note_3874176486](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/256514#note_3874176486), and recorded in the !256514 description. ### 6. Verify awareness re-announcement on reconnect Because `#markSynced` returns early, a reconnected client may not re-announce awareness. The author flagged this for slice 5. Note @kivikakk partially corrected the premise: `y-protocols` awareness does heartbeat every ~15s, so the "no heartbeat" framing is inaccurate; confirm whether a real gap remains before acting. See [note_3835428320](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/254876#note_3835428320) and [note_3840186030](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/254876#note_3840186030). ### 7. Merge-conflict coordination with !254804+ [!254804](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/254804) also changes `handle_snapshot`. Since !254876 has now merged, confirm the conflict was resolved correctly. See [note_3840186012](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/254876#note_3840186012). !254804 has since merged. `handle_snapshot` on `master` still reads `broadcast(data.merge('type' => MESSAGE_TYPE_SYNC))`, so the merge did not lose anything, and item 2 above remains outstanding. Item 7 needs no further work. ## Raised in !256514 review (slice 5) Items 8 to 10 risk silent loss or misattribution of a user's work, so they rank alongside the must-do items above. ### 8. The Redis document log can expire under a live editor The document log expires while an editor is still open, and the next save can then overwrite the page. `DocumentStore` sets a one hour TTL: ```ruby # lib/gitlab/collaborative_editing/document_store.rb TTL = 1.hour ``` Only `append` and `replace` refresh it; both call `expire` inside their Lua scripts. Awareness messages do not. `CollaborativeEditing::BaseChannel#receive` routes `MESSAGE_TYPE_AWARENESS` straight to `broadcast(data, identity: true)` and never touches the store. An editor left open for over an hour without typing loses its log. The next keystroke appends a lone delta to an empty list. Anyone joining after that replays only that delta and opens a near-empty document. Saving from that state writes the near-empty content over the real page. Raised by @kivikakk in [note_3895758001](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/256514#note_3895758001). ### 9. Cancel no longer discards edits Cancel leaves the abandoned draft in the document log. Without collaboration, Cancel discards the draft. With collaboration the log survives for an hour, so the next person to open the editor is seeded with that draft. If they save, the edits are committed under their name. This can be used deliberately to get text attributed to someone else. This is related to the draft-seeding design question in [note_3874176486](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/256514#note_3874176486), but that note concerns seeding from the author's own localStorage draft. This item is the server-side log persisting past Cancel. Raised by @kivikakk in [note_3895758001](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/256514#note_3895758001). ### 10. Switching rich text to plain text and back discards the plain text edits An RTE to PTE to RTE round trip loses everything typed in the plain text editor. The return to rich text reloads whatever is in the CRDT, and only the rich text editor is collaborative. This sharpens the general "behaviour seems odd when switching from PTE to RTE" observation in [note_3874176486](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/256514#note_3874176486). Raised by @kivikakk in [note_3895758001](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/256514#note_3895758001). ### 11. The conflict check is not meaningful during collaboration The conflict check fails a save without preventing anything. Both editors load the form with the same `wiki[last_commit_sha]`. The first save advances the page to a new commit. The second editor's save then fails the check in `app/models/wiki_page.rb:307`, which raises `WikiPage::PageChangedError`. Saving again succeeds, because the edits are restored from the Redis log. Decide whether to skip the check during collaboration, or to refresh the SHA for all participants after each save. The banner that conflict produces renders its i18n placeholders raw, so the user sees `%{wikiLinkStart}` on screen. That is a separate pre-existing defect on `master`, unrelated to this feature, and is tracked at [#630866](https://gitlab.com/gitlab-org/gitlab/-/work_items/630866). It does not need to be fixed before the flag is enabled, but collaboration makes it far easier to hit. Raised by @kivikakk in [note_3895758001](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/256514#note_3895758001). ### 12. No fallback when the WebSocket never connects The rich text editor becomes unusable when the WebSocket never connects. It shows a spinner for ten seconds, then "An error occured while trying to render the rich text editor" with a Retry button. Retry waits on the same promise, so it cannot succeed. Only the plain text editor remains usable. The remedy overlaps with item 3 above: load the server content non-collaboratively and disable sync, rather than failing the editor. Raised by @kivikakk in [note_3895758001](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/256514#note_3895758001). ### 13. A user sees themselves when the same page is open in two tabs A second tab appears as a separate collaborator. `collaborators_indicator.vue` filters the awareness states by `clientId !== localClientId`, then deduplicates by `identity.id`. A second tab belonging to the same user has a different `clientId`, so it survives the filter. Decide whether to filter by user id rather than client id. The spec name `excludes the local user from the list` in `spec/frontend/collaborative_editing/components/collaborators_indicator_spec.js:65` describes the intent, not the behaviour. It is being renamed to `excludes the local client from the list` in !256514. Raised by @kivikakk in [note_3895758001](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/256514#note_3895758001). ## Moved from the !250607 store and channel review Recorded originally at [!250607#note_3802654087](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/250607#note_3802654087). ### 14. Per-document byte budget A log holds `MAX_LOG_LENGTH` (2000) entries of up to `MAX_PAYLOAD_BYTES` (1 MB), so one document can reach roughly 1.95 GB of `Gitlab::Redis::SharedState`, which has no eviction policy. The rate limit is keyed on `[user, document]`, so the figure multiplies across pages, and any user can obtain `create_wiki` on their own project. A late joiner also receives the whole log in one message. Suggested: track bytes per document in `APPEND_SCRIPT` and return `full` above a budget (8-16 MB was suggested); split the payload cap by message type, with a much smaller cap for `sync` and 1 MB kept only for `snapshot`; add a second rate limit keyed on the user alone. Needs the `#g_durability` PREP review, which has not happened yet, per `doc/development/redis.md`. Also raised when the store was approved, at [!252572#note_3767041076](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/252572#note_3767041076). ### 15. Reject pages too large for collaborative editing Wiki pages may be 5 MB (`wiki_page_max_content_bytes`) while a payload is capped at 1 MB, and Yjs state is larger than the source it encodes. For such a page the seed and every snapshot exceed the cap and are dropped by `valid_payload?` with nothing sent back, so compaction never completes. The next joiner then opens an empty editor whose save carries a fresh SHA, passes the conflict check, and replaces the page. This was implemented and then deliberately rolled back, because the threshold should be derived rather than guessed: once `snapshot` has its own cap under item 14, the largest editable page is whatever source size encodes to just under it. That is a measurement against real pages. It is not blocked on discovery. `Wiki#find_page` already fetches the blob with `limit: 0` and discards `blob.size`, so `repository.blob_at(default_branch, page.path, limit: 0)` gives the true size with no content transfer. Note that `Blob` is a `SimpleDelegator`, so `blob.present?` is false for a blob loaded with `limit: 0` — check `nil?`. ### 16. The Redis log outlives the Git commit The document key is container plus slug, and the log lives an hour past the last write. If the page changes through the REST API, a Git push, or a plain save in that window, the next joiner is seeded from the stale log while their form carries the new commit SHA. The save passes the conflict check and the other change is lost silently. Suggested: add the page version to the document key so a Git write starts a fresh session, or delete the log from every wiki write path including the Git push service. This changes the key format, so it is worth doing alongside item 14 rather than twice. Item 8 is the same key design seen from the other side: there, the log expires while the editor is still live. A fix for one should be checked against the other. ### 17. Smaller store and channel items - Count and log messages dropped before the rate limiter runs, which also helps spot client-side bugs. - Wrap `Y.applyUpdate` and `applyAwarenessUpdate` in error handling so one bad payload cannot break every peer. ## Closed ### Protocol changes slice 4 had to absorb A `snapshot` message must carry the token from `request_snapshot`, and `document_full` is a server message the provider must handle. Both landed with [!254876](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/254876). --- 🤖 *This issue was generated by GitLab Duo from review discussion on !254876, and extended from review of !250607 and !256514.* </:robot:>