fix(collab): validate and rate-limit comment mutations - #1629
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds validators for untrusted comment payloads and applies validation, sanitization, duplicate checks, reply limits, and per-socket rate limiting before persistence and broadcast. ChangesComment validation and mutation handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Session
participant Validators
participant Persistence
participant Broadcast
Client->>Session: comment mutation
Session->>Validators: validate and sanitize payload
Validators-->>Session: validated mutation or null
Session->>Persistence: persist sanitized mutation
Session->>Broadcast: broadcast sanitized mutation
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 Cloudflare PR preview
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@workers/collab/src/comment-validate.ts`:
- Around line 87-132: Add a MAX_REPLIES_PER_COMMENT bound in validateComment and
only validate/persist replies within that limit, preserving the existing reply
validation behavior. Update handleCommentMutation to enforce a total
comments/replies byte-size budget before persistence, mirroring the limits
applied by handleSnapshot and chat history handling, and reject or truncate
oversized input consistently.
In `@workers/collab/src/session.ts`:
- Around line 731-743: Update the validation branches for “toggle-resolve” and
“delete” in the action sanitization flow to send the established “bad-message”
error frame before returning when commentId is missing or invalid, matching the
existing add/reply behavior. Preserve the current sanitized actions for valid
targets.
- Around line 759-761: Update the “add” action handling in the session action
processor to check whether comments already contains a comment with
sanitizedAction.comment.id before appending. Preserve the existing comments
unchanged when that ID is present, matching the duplicate-prevention behavior
used by the “reply” branch.
- Around line 750-796: Update handleCommentMutation and handleSnapshot so
comment mutations remain persisted when no snapshot exists and cannot be
overwritten by later full-project snapshot replacements. Use a single consistent
snapshot update path, or preserve the previously stored comments field whenever
handleSnapshot replaces project data, while retaining incoming comments when
explicitly provided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 88e5c1b1-04cb-4ce4-95e4-4afbae5cbbc4
📒 Files selected for processing (3)
tests/collab-comment-validate.test.tsworkers/collab/src/comment-validate.tsworkers/collab/src/session.ts
🔍 GitHub Pages PR preview
|
|
/claude-review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Exclude the sender (ws) so they don't receive their own mutation back. | ||
| // The sender already applied the change locally before calling sendCommentMutation. | ||
| this.broadcast(message, ws); | ||
| this.broadcast(sanitizedMessage, ws); |
There was a problem hiding this comment.
Since sanitizedMessage can now legitimately differ from what the sender sent (body truncated to 2000 chars, author name trimmed/truncated, resolved coerced to a strict boolean) and it's only broadcast to other peers (this.broadcast(sanitizedMessage, ws) excludes the sender), the sender's own optimistic local state (applied via addComment/etc. before calling sendCommentMutation, see useCommentTool.ts) can drift from what's actually persisted/seen by everyone else — e.g. a >2000-char comment body. A later full-project snapshot resync (from the host) could then silently overwrite the sender's local copy with the truncated version. This mirrors the pre-existing chat pattern, so it may be an accepted tradeoff, but worth confirming it's intentional for comments too, since they're persisted rather than ephemeral.
Confidence: low.
There was a problem hiding this comment.
Confirming it's intentional, and leaving this thread open for a maintainer to weigh in rather than resolving it myself.
The truncation/coercion is deliberate and does mirror the chat path: the relay must not fan out an unbounded string, and the sender is the one client that already has the content, so echoing a corrected copy back would need the client store to replace on echo (addComment/replyToComment currently de-dupe by id and ignore it) — a client-side change beyond the scope of this hardening PR.
Two things narrow the practical window since you flagged it:
- The resync you describe is now less likely to be lossy in the other direction — as of 0d75313
handleSnapshotpreserves the storedcommentswhen an incoming project omits them, so a host resync no longer silently drops comments it hasn't merged. - Drift can only originate from a >2000-char body (or a name over 120 chars), which the normal UI path doesn't produce on its own; the comment textareas have no
maxLength, so a paste can still exceed it.
If we want to close it properly, the clean fix is a client-side maxLength on the comment/reply textareas backed by a shared constant, so the server cap is never the first place a user learns about the limit. Happy to do that as a follow-up if you'd like it in this PR.
Code reviewBugs
Security / Performance
Quality
CLAUDE.md
No security issues (e.g., injection, secret leakage) or unaddressed performance regressions were found beyond the storage-growth note above. Type-safety of the new validators against |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@workers/collab/src/comment-validate.ts`:
- Around line 122-128: Update reply validation around validateReply to inspect
no more than MAX_REPLIES_PER_COMMENT input entries, regardless of how many
validate successfully. In handleCommentMutation, reject incremental reply
mutations when the target comment already has MAX_REPLIES_PER_COMMENT stored
replies before appending. Add a session-level regression test covering this
existing-limit mutation path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fc755b15-d33d-40ad-a571-19bd39f84501
📒 Files selected for processing (3)
tests/collab-comment-validate.test.tsworkers/collab/src/comment-validate.tsworkers/collab/src/session.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
workers/collab/src/session.ts (3)
755-756: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn
bad-messagefor unsupported action types.The fallback at Lines 755-756 silently returns for an unknown or missing
action.type. This violates the invalid-payload contract. Send an error frame before returning, as the other validation branches do.Proposed fix
} else if (action.type === "delete") { if (typeof action.commentId !== "string" || !action.commentId) { this.send(ws, { type: "error", code: "bad-message", message: "Invalid delete target.", }); return; } sanitizedAction = { type: "delete", commentId: action.commentId }; } else { - return; + this.send(ws, { + type: "error", + code: "bad-message", + message: "Unknown comment-mutation action.", + }); + return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workers/collab/src/session.ts` around lines 755 - 756, Update the fallback branch handling unsupported or missing action.type in the session message validation flow to send the same bad-message error frame used by the other validation branches before returning, preserving the existing behavior for supported action types.
777-787: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEnforce the per-comment reply limit on incremental replies.
workers/collab/src/comment-validate.tscaps initial replies atMAX_REPLIES_PER_COMMENT. This branch appends new replies without checkingexistingReplies.length. Repeated validreplyactions can bypass the bound and grow the persisted snapshot. Reject only new replies at the limit, and do not broadcast a mutation that was not applied.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workers/collab/src/session.ts` around lines 777 - 787, Update the incremental reply handling in the sanitizedAction.type === "reply" branch to enforce MAX_REPLIES_PER_COMMENT before appending a new reply. Preserve duplicate-reply behavior, reject only non-duplicate replies when existingReplies has reached the limit, and ensure no mutation broadcast occurs when the reply is not applied.
677-696: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove validation and throttling before the mode storage read.
handleCommentMutationreadsmodebefore the rate-limit gate. A view-only socket can therefore send comment frames without a per-socket limit and force one storage read plus oneforbiddenresponse per frame. The code also saveslastCommentTsbefore validatingaction, so a rejected frame consumes the quota and can suppress a following valid mutation. Validate the action first, apply the rate limit, then readmodeand authorize the mutation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workers/collab/src/session.ts` around lines 677 - 696, Update handleCommentMutation so it validates message.action before any throttling or storage access, applies the per-socket rate-limit gate only after validation, and persists lastCommentTs only for an accepted action. Move the mode storage read and authorization check after validation and throttling, preserving the existing forbidden response for view-only sockets.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@workers/collab/src/session.ts`:
- Around line 755-756: Update the fallback branch handling unsupported or
missing action.type in the session message validation flow to send the same
bad-message error frame used by the other validation branches before returning,
preserving the existing behavior for supported action types.
- Around line 777-787: Update the incremental reply handling in the
sanitizedAction.type === "reply" branch to enforce MAX_REPLIES_PER_COMMENT
before appending a new reply. Preserve duplicate-reply behavior, reject only
non-duplicate replies when existingReplies has reached the limit, and ensure no
mutation broadcast occurs when the reply is not applied.
- Around line 677-696: Update handleCommentMutation so it validates
message.action before any throttling or storage access, applies the per-socket
rate-limit gate only after validation, and persists lastCommentTs only for an
accepted action. Move the mode storage read and authorization check after
validation and throttling, preserving the existing forbidden response for
view-only sockets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 485888c0-dfd2-4837-aecc-a96457a7bfea
📒 Files selected for processing (1)
workers/collab/src/session.ts
1432202 to
02322e8
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workers/collab/src/session.ts (1)
682-691: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate the action before updating the rate-limit state.
Lines 684-688 return before the handler checks
message.action. An invalid mutation inside the interval gets nobad-messageresponse. An invalid mutation sent first also consumes the interval and can drop the next valid mutation.Move the rate-limit block after action validation and before the snapshot read. The PR objective states that invalid payloads return
bad-message.Proposed fix
- // Rate-limit: same pattern as chat to prevent storage-op exhaustion. - const now = Date.now(); - if ( - attachment.lastCommentTs !== undefined && - now - attachment.lastCommentTs < MIN_COMMENT_INTERVAL_MS - ) { - return; - } - attachment.lastCommentTs = now; - ws.serializeAttachment(attachment); - const action = message.action; if (!action || typeof action !== "object") { // ... } // Validate and sanitize action branches. + // Rate-limit accepted mutations before storage work. + const now = Date.now(); + if ( + attachment.lastCommentTs !== undefined && + now - attachment.lastCommentTs < MIN_COMMENT_INTERVAL_MS + ) { + return; + } + attachment.lastCommentTs = now; + ws.serializeAttachment(attachment); + const rawSnapshot = await this.ctx.storage.get<string>("snapshot");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workers/collab/src/session.ts` around lines 682 - 691, Move the rate-limit check and `attachment.lastCommentTs` update in the session message handler to after `message.action` validation, but before the snapshot read. Ensure invalid actions always return `bad-message` without consuming or being blocked by the rate-limit state, while valid mutations retain the existing interval behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@workers/collab/src/session.ts`:
- Around line 682-691: Move the rate-limit check and `attachment.lastCommentTs`
update in the session message handler to after `message.action` validation, but
before the snapshot read. Ensure invalid actions always return `bad-message`
without consuming or being blocked by the rate-limit state, while valid
mutations retain the existing interval behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b03492ac-59a9-4c73-a9af-b0e7545aacfc
📒 Files selected for processing (3)
tests/collab-comment-validate.test.tsworkers/collab/src/comment-validate.tsworkers/collab/src/session.ts
Reject malformed comment/reply payloads, cap body length like chat, and throttle mutations so peers cannot inflate the shared snapshot.
…rgets - Add MAX_REPLIES_PER_COMMENT (100) to bound reply arrays - Reject NaN/Infinity numeric featureIds in anchor validation - Validate createdAt is parseable (fall back to now if not) - Send bad-message error for invalid toggle-resolve/delete targets - Deduplicate add action by comment id (matches reply branch) - Add regression tests for all new validations
…unknown action types - validateComment now slices o.replies to MAX_REPLIES_PER_COMMENT before iterating, capping inspected input length instead of only valid count. - handleCommentMutation reply path checks target.replies.length against MAX_REPLIES_PER_COMMENT before appending; returns bad-message if full. - Unsupported comment-mutation action types now return bad-message instead of silently dropping. - Added validator test proving entries past the slice boundary are ignored.
Invalid payloads always get bad-message and no longer consume the per-socket interval or block a following valid mutation.
0068905 to
e45e87b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
workers/collab/src/session.ts (2)
776-778: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftComment mutations before the first snapshot are still broadcast, never persisted.
handleCommentMutationonly updates storage whenrawSnapshotalready exists (Line 777). If no project snapshot has been written yet, the whole persistence block is skipped, butthis.broadcast(sanitizedMessage, ws)at Line 840 still runs. The mutation reaches connected peers but is never stored, so a late joiner (or anyone reconnecting) loses it. This is the same gap raised in a prior review round on this file ("Preserve comment mutations in the snapshot writes"), which remains unresolved.Give comment mutations their own persistence path (independent of whether a full snapshot has been written yet), or seed an empty stored project before applying the mutation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workers/collab/src/session.ts` around lines 776 - 778, Update handleCommentMutation so comment mutations are persisted even when storage.get("snapshot") returns no rawSnapshot. Add an independent comment-mutation persistence path or initialize an empty project snapshot before applying the mutation, while preserving the existing broadcast behavior and ensuring late joiners receive the stored mutation.
785-833: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftBound the total size of the persisted comments array.
Unlike
handleChat(MAX_CHAT_STORAGE_BYTESeviction) andhandleSnapshot(MAX_SNAPSHOT_BYTEScheck), this read-modify-write path has no cap on the total serialized size or count ofparsed.comments.MAX_REPLIES_PER_COMMENTbounds replies per comment, but the number of comments and their combined byte size are unbounded, so a burst of "add" mutations (even at the 250 ms floor) can growcommentswithout limit. Ifstorage.puteventually throws (over the per-key storage limit), the failure is swallowed by thecatchat Line 834 and the mutation is still broadcast at Line 840, leaving connected peers out of sync with what late joiners see. This matches an unresolved concern raised in a prior review round on this file.Add a comments-array size/byte budget (mirroring chat's eviction) and treat a persistence failure here consistently with the broadcast path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workers/collab/src/session.ts` around lines 785 - 833, Bound the read-modify-write flow around parsed.comments using a comments count/serialized-byte budget, reusing the eviction approach and relevant limits from handleChat rather than allowing unbounded growth. Apply the cap before storage.put("snapshot"), and handle storage persistence failures consistently with the existing broadcast path: report the error and do not broadcast a mutation that was not successfully persisted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@workers/collab/src/comment-validate.ts`:
- Around line 101-139: Cap all identifier fields validated by validateComment,
validateAnchor, and validateReply to a small bounded length such as 200
characters: reject or truncate oversized values for comment id, anchor layerId
and featureId, and reply id. Preserve existing validation behavior while
ensuring no unbounded identifier string is stored in snapshots.
In `@workers/collab/src/session.ts`:
- Around line 785-815: Update the reply handling branch around the target lookup
and reply-limit check to send the same bad-message error and return immediately
when target is undefined. Preserve the existing reply-limit behavior for found
comments, preventing nonexistent-target replies from reaching the subsequent
comments.map and broadcast flow.
---
Duplicate comments:
In `@workers/collab/src/session.ts`:
- Around line 776-778: Update handleCommentMutation so comment mutations are
persisted even when storage.get("snapshot") returns no rawSnapshot. Add an
independent comment-mutation persistence path or initialize an empty project
snapshot before applying the mutation, while preserving the existing broadcast
behavior and ensuring late joiners receive the stored mutation.
- Around line 785-833: Bound the read-modify-write flow around parsed.comments
using a comments count/serialized-byte budget, reusing the eviction approach and
relevant limits from handleChat rather than allowing unbounded growth. Apply the
cap before storage.put("snapshot"), and handle storage persistence failures
consistently with the existing broadcast path: report the error and do not
broadcast a mutation that was not successfully persisted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8c164b27-f14b-4382-9fe7-a659fedcb683
📒 Files selected for processing (3)
tests/collab-comment-validate.test.tsworkers/collab/src/comment-validate.tsworkers/collab/src/session.ts
Seed an empty snapshot when none exists so comment mutations are stored before the first full project sync, reject replies to missing comments, and cap id/layerId/featureId lengths at 200 characters.
- Preserve stored comments across full-project snapshots. New pure helper `preserveStoredComments` merges the persisted `comments` list into an incoming snapshot that omits the key (`serializeProject` drops it when a peer holds none), so a peer that has not merged comment-mutation broadcasts can no longer clobber them. A project that carries its own `comments` still wins, so a delete is never resurrected. The merged project is broadcast, healing a drifted sender. - Bound total comment growth with `MAX_COMMENTS_PER_SESSION` (500), mirroring `CHAT_HISTORY_LIMIT` for the chat log; an "add" past the cap gets a `bad-message` error instead of growing the snapshot forever. - Check the serialized snapshot against `MAX_SNAPSHOT_BYTES` before the storage write, matching `handleSnapshot`. - Stop broadcasting a comment mutation whose persistence failed. The sender now gets an error and the fan-out is skipped, so connected peers no longer hold a comment that a late joiner or reconnect (both of which read from storage) would never see. - Cover `preserveStoredComments` and the new limit in tests/collab-comment-validate.test.ts.
2 participants
Summary
bad-messageerror to the sender on invalid payloads.Test plan
node --import tsx --test tests/collab-comment-validate.test.ts tests/collab-protocol.test.tsSummary by CodeRabbit
New Features
Bug Fixes
Tests