diff --git a/tests/collab-comment-validate.test.ts b/tests/collab-comment-validate.test.ts new file mode 100644 index 000000000..ebe051b0d --- /dev/null +++ b/tests/collab-comment-validate.test.ts @@ -0,0 +1,451 @@ +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; +import { + MAX_COMMENT_BODY_LENGTH, + MAX_COMMENT_AUTHOR_LENGTH, + MAX_COMMENTS_PER_SESSION, + MAX_ID_LENGTH, + MAX_REPLIES_PER_COMMENT, + MIN_COMMENT_INTERVAL_MS, + preserveStoredComments, + validateAnchor, + validateAuthor, + validateComment, + validateReply, +} from "../workers/collab/src/comment-validate"; + +// -- constants ---------------------------------------------------------------- + +describe("comment-validate constants", () => { + it("exports expected limits", () => { + assert.equal(MAX_COMMENT_BODY_LENGTH, 2000); + assert.equal(MAX_COMMENT_AUTHOR_LENGTH, 120); + assert.equal(MAX_ID_LENGTH, 200); + assert.equal(MIN_COMMENT_INTERVAL_MS, 250); + assert.equal(MAX_REPLIES_PER_COMMENT, 100); + assert.equal(MAX_COMMENTS_PER_SESSION, 500); + }); +}); + +// -- preserveStoredComments --------------------------------------------------- + +describe("preserveStoredComments", () => { + const stored = { comments: [{ id: "c1", body: "Persisted" }] }; + + it("carries stored comments into a project that omits the key", () => { + const result = preserveStoredComments({ layers: [] }, stored) as Record; + assert.deepEqual(result.comments, stored.comments); + assert.deepEqual(result.layers, []); + }); + + it("keeps the incoming comments when the project supplies its own", () => { + const incoming = { layers: [], comments: [{ id: "c2" }] }; + assert.deepEqual(preserveStoredComments(incoming, stored), incoming); + }); + + it("does not resurrect comments the sender explicitly cleared", () => { + const incoming = { layers: [], comments: [] }; + const result = preserveStoredComments(incoming, stored) as Record; + assert.deepEqual(result.comments, []); + }); + + it("leaves the project alone when storage is empty or corrupt", () => { + assert.deepEqual(preserveStoredComments({ layers: [] }, null), { layers: [] }); + assert.deepEqual(preserveStoredComments({ layers: [] }, "not an object"), { layers: [] }); + assert.deepEqual(preserveStoredComments({ layers: [] }, { comments: [] }), { layers: [] }); + assert.deepEqual(preserveStoredComments({ layers: [] }, { comments: "nope" }), { layers: [] }); + }); + + it("passes a null or non-object project through untouched", () => { + assert.equal(preserveStoredComments(null, stored), null); + assert.deepEqual(preserveStoredComments([1, 2], stored), [1, 2]); + }); +}); + +// -- validateAnchor ----------------------------------------------------------- + +describe("validateAnchor", () => { + it("accepts a valid point anchor", () => { + const result = validateAnchor({ type: "point", lngLat: [-122.4, 37.8] }); + assert.deepEqual(result, { type: "point", lngLat: [-122.4, 37.8] }); + }); + + it("rejects a point anchor with non-finite coordinates", () => { + assert.equal(validateAnchor({ type: "point", lngLat: [NaN, 37.8] }), null); + assert.equal(validateAnchor({ type: "point", lngLat: [Infinity, 37.8] }), null); + assert.equal(validateAnchor({ type: "point", lngLat: [-122.4, -Infinity] }), null); + }); + + it("rejects a point anchor with wrong lngLat length", () => { + assert.equal(validateAnchor({ type: "point", lngLat: [1] }), null); + assert.equal(validateAnchor({ type: "point", lngLat: [1, 2, 3] }), null); + }); + + it("rejects a point anchor with non-number coordinates", () => { + assert.equal(validateAnchor({ type: "point", lngLat: ["a", "b"] }), null); + }); + + it("rejects a point anchor with no lngLat", () => { + assert.equal(validateAnchor({ type: "point" }), null); + }); + + it("accepts a feature anchor with string featureId", () => { + const result = validateAnchor({ + type: "feature", + layerId: "layer-1", + featureId: "feat-42", + }); + assert.deepEqual(result, { + type: "feature", + layerId: "layer-1", + featureId: "feat-42", + }); + }); + + it("accepts a feature anchor with numeric featureId", () => { + const result = validateAnchor({ + type: "feature", + layerId: "layer-1", + featureId: 42, + }); + assert.deepEqual(result, { + type: "feature", + layerId: "layer-1", + featureId: 42, + }); + }); + + it("includes lngLat on a feature anchor when valid", () => { + const result = validateAnchor({ + type: "feature", + layerId: "L", + featureId: "F", + lngLat: [10, 20], + }); + assert.deepEqual(result, { + type: "feature", + layerId: "L", + featureId: "F", + lngLat: [10, 20], + }); + }); + + it("drops non-finite lngLat on a feature anchor silently", () => { + const result = validateAnchor({ + type: "feature", + layerId: "L", + featureId: "F", + lngLat: [NaN, 20], + }); + assert.deepEqual(result, { type: "feature", layerId: "L", featureId: "F" }); + }); + + it("rejects a feature anchor with empty layerId", () => { + assert.equal(validateAnchor({ type: "feature", layerId: "", featureId: "f" }), null); + }); + + it("rejects a feature anchor with empty string featureId", () => { + assert.equal(validateAnchor({ type: "feature", layerId: "L", featureId: "" }), null); + }); + + it("rejects a feature anchor with non-string/non-number featureId", () => { + assert.equal(validateAnchor({ type: "feature", layerId: "L", featureId: true }), null); + }); + + it("rejects a feature anchor with NaN featureId", () => { + assert.equal(validateAnchor({ type: "feature", layerId: "L", featureId: NaN }), null); + }); + + it("rejects a feature anchor with Infinity featureId", () => { + assert.equal(validateAnchor({ type: "feature", layerId: "L", featureId: Infinity }), null); + }); + + it("rejects a feature anchor with oversized layerId or featureId", () => { + const long = "x".repeat(MAX_ID_LENGTH + 1); + assert.equal(validateAnchor({ type: "feature", layerId: long, featureId: "f" }), null); + assert.equal(validateAnchor({ type: "feature", layerId: "L", featureId: long }), null); + }); + + it("rejects unknown anchor types", () => { + assert.equal(validateAnchor({ type: "polygon", coords: [] }), null); + }); + + it("rejects null / undefined / primitives", () => { + assert.equal(validateAnchor(null), null); + assert.equal(validateAnchor(undefined), null); + assert.equal(validateAnchor("string"), null); + assert.equal(validateAnchor(42), null); + }); +}); + +// -- validateAuthor ----------------------------------------------------------- + +describe("validateAuthor", () => { + it("accepts a valid author", () => { + const result = validateAuthor({ name: "Alice", color: "#ff0000" }); + assert.deepEqual(result, { name: "Alice", color: "#ff0000" }); + }); + + it("accepts a 3-digit hex color", () => { + const result = validateAuthor({ name: "Bob", color: "#abc" }); + assert.deepEqual(result, { name: "Bob", color: "#abc" }); + }); + + it("trims the name", () => { + const result = validateAuthor({ name: " Spaced ", color: "#000" }); + assert.equal(result?.name, "Spaced"); + }); + + it("truncates a long name", () => { + const longName = "X".repeat(200); + const result = validateAuthor({ name: longName, color: "#000" }); + assert.equal(result?.name.length, MAX_COMMENT_AUTHOR_LENGTH); + }); + + it("rejects an empty name after trimming", () => { + assert.equal(validateAuthor({ name: " ", color: "#000" }), null); + }); + + it("rejects a non-string name", () => { + assert.equal(validateAuthor({ name: 42, color: "#000" }), null); + }); + + it("rejects an invalid hex color", () => { + assert.equal(validateAuthor({ name: "A", color: "red" }), null); + assert.equal(validateAuthor({ name: "A", color: "#gggggg" }), null); + assert.equal(validateAuthor({ name: "A", color: "#12345" }), null); + }); + + it("rejects a non-string color", () => { + assert.equal(validateAuthor({ name: "A", color: 0xff0000 }), null); + }); + + it("rejects null / undefined / primitives", () => { + assert.equal(validateAuthor(null), null); + assert.equal(validateAuthor(undefined), null); + assert.equal(validateAuthor("str"), null); + }); +}); + +// -- validateReply ------------------------------------------------------------ + +describe("validateReply", () => { + const validReply = { + id: "r-1", + author: { name: "Alice", color: "#abc" }, + body: "Good point", + createdAt: "2026-01-01T00:00:00.000Z", + }; + + it("accepts a valid reply", () => { + const result = validateReply(validReply); + assert.deepEqual(result, validReply); + }); + + it("truncates a long body", () => { + const result = validateReply({ ...validReply, body: "Z".repeat(3000) }); + assert.equal(result?.body.length, MAX_COMMENT_BODY_LENGTH); + }); + + it("rejects a whitespace-only body", () => { + assert.equal(validateReply({ ...validReply, body: " " }), null); + }); + + it("rejects a non-string body", () => { + assert.equal(validateReply({ ...validReply, body: 123 }), null); + }); + + it("rejects an empty id", () => { + assert.equal(validateReply({ ...validReply, id: "" }), null); + }); + + it("rejects a non-string id", () => { + assert.equal(validateReply({ ...validReply, id: 42 }), null); + }); + + it("rejects an oversized id", () => { + assert.equal(validateReply({ ...validReply, id: "r".repeat(MAX_ID_LENGTH + 1) }), null); + }); + + it("rejects an invalid author", () => { + assert.equal(validateReply({ ...validReply, author: { name: "", color: "#000" } }), null); + assert.equal(validateReply({ ...validReply, author: null }), null); + }); + + it("falls back to now for missing createdAt", () => { + const { createdAt: _, ...noCreated } = validReply; + const result = validateReply(noCreated); + assert.ok(result); + assert.ok(result.createdAt); + assert.ok(!isNaN(Date.parse(result.createdAt))); + }); + + it("falls back to now for unparseable createdAt", () => { + const result = validateReply({ ...validReply, createdAt: "not-a-date" }); + assert.ok(result); + assert.notEqual(result.createdAt, "not-a-date"); + assert.ok(!isNaN(Date.parse(result.createdAt))); + }); + + it("rejects null / undefined / primitives", () => { + assert.equal(validateReply(null), null); + assert.equal(validateReply(undefined), null); + assert.equal(validateReply(42), null); + assert.equal(validateReply("str"), null); + }); +}); + +// -- validateComment ---------------------------------------------------------- + +describe("validateComment", () => { + const validComment = { + id: "c-1", + anchor: { type: "point" as const, lngLat: [10, 20] }, + author: { name: "Alice", color: "#ff0000" }, + body: "Fix this road segment", + createdAt: "2026-06-15T12:00:00.000Z", + resolved: false, + replies: [], + }; + + it("accepts a valid comment", () => { + const result = validateComment(validComment); + assert.deepEqual(result, validComment); + }); + + it("truncates a long body", () => { + const result = validateComment({ ...validComment, body: "B".repeat(3000) }); + assert.ok(result); + assert.equal(result.body.length, MAX_COMMENT_BODY_LENGTH); + }); + + it("rejects a whitespace-only body", () => { + assert.equal(validateComment({ ...validComment, body: " \n\t " }), null); + }); + + it("rejects a non-string body", () => { + assert.equal(validateComment({ ...validComment, body: false }), null); + }); + + it("rejects an empty id", () => { + assert.equal(validateComment({ ...validComment, id: "" }), null); + }); + + it("rejects a non-string id", () => { + assert.equal(validateComment({ ...validComment, id: 99 }), null); + }); + + it("rejects an oversized id", () => { + assert.equal(validateComment({ ...validComment, id: "c".repeat(MAX_ID_LENGTH + 1) }), null); + }); + + it("rejects an invalid anchor", () => { + assert.equal(validateComment({ ...validComment, anchor: { type: "bad" } }), null); + assert.equal(validateComment({ ...validComment, anchor: null }), null); + }); + + it("rejects an invalid author", () => { + assert.equal( + validateComment({ ...validComment, author: { name: "X", color: "not-hex" } }), + null, + ); + }); + + it("coerces resolved to boolean", () => { + const result = validateComment({ ...validComment, resolved: 1 }); + assert.equal(result?.resolved, true); + const result2 = validateComment({ ...validComment, resolved: undefined }); + assert.equal(result2?.resolved, false); + }); + + it("validates replies within a comment", () => { + const withReplies = { + ...validComment, + replies: [ + { + id: "r-1", + author: { name: "Bob", color: "#abc" }, + body: "Agreed", + createdAt: "2026-06-15T12:30:00.000Z", + }, + { id: "", author: { name: "Bad", color: "#abc" }, body: "nope", createdAt: "x" }, + null, + 42, + ], + }; + const result = validateComment(withReplies); + assert.ok(result); + assert.equal(result.replies.length, 1); + assert.equal(result.replies[0]?.id, "r-1"); + }); + + it("accepts a comment with a feature anchor", () => { + const result = validateComment({ + ...validComment, + anchor: { type: "feature", layerId: "L", featureId: 7, lngLat: [1, 2] }, + }); + assert.ok(result); + assert.equal(result.anchor.type, "feature"); + }); + + it("falls back to now for missing createdAt", () => { + const { createdAt: _, ...noCreated } = validComment; + const result = validateComment(noCreated); + assert.ok(result); + assert.ok(!isNaN(Date.parse(result.createdAt))); + }); + + it("rejects null / undefined / primitives", () => { + assert.equal(validateComment(null), null); + assert.equal(validateComment(undefined), null); + assert.equal(validateComment("string"), null); + assert.equal(validateComment(42), null); + }); + + it("caps replies at MAX_REPLIES_PER_COMMENT", () => { + const manyReplies = Array.from({ length: MAX_REPLIES_PER_COMMENT + 20 }, (_, i) => ({ + id: `r-${i}`, + author: { name: "Bob", color: "#abc" }, + body: `Reply ${i}`, + createdAt: "2026-06-15T12:30:00.000Z", + })); + const result = validateComment({ ...validComment, replies: manyReplies }); + assert.ok(result); + assert.equal(result.replies.length, MAX_REPLIES_PER_COMMENT); + }); + + it("only inspects the first MAX_REPLIES_PER_COMMENT entries", () => { + const invalid = Array.from({ length: MAX_REPLIES_PER_COMMENT }, (_, i) => ({ + id: `bad-${i}`, + author: { name: "", color: "#abc" }, + body: `Reply ${i}`, + createdAt: "2026-06-15T12:30:00.000Z", + })); + const valid = Array.from({ length: 5 }, (_, i) => ({ + id: `good-${i}`, + author: { name: "Alice", color: "#abc" }, + body: `Reply ${i}`, + createdAt: "2026-06-15T12:30:00.000Z", + })); + const result = validateComment({ ...validComment, replies: [...invalid, ...valid] }); + assert.ok(result); + assert.equal(result.replies.length, 0); + }); + + it("falls back to now for unparseable createdAt", () => { + const result = validateComment({ ...validComment, createdAt: "garbage" }); + assert.ok(result); + assert.notEqual(result.createdAt, "garbage"); + assert.ok(!isNaN(Date.parse(result.createdAt))); + }); + + it("strips extra fields (returns only known shape)", () => { + const result = validateComment({ + ...validComment, + extraField: "should not appear", + __proto__: { evil: true }, + }); + assert.ok(result); + assert.equal((result as Record).extraField, undefined); + }); +}); diff --git a/workers/collab/src/comment-validate.ts b/workers/collab/src/comment-validate.ts new file mode 100644 index 000000000..b57cb555a --- /dev/null +++ b/workers/collab/src/comment-validate.ts @@ -0,0 +1,203 @@ +// Pure validators for comment-mutation payloads. These mirror the +// `ProjectComment` / `CommentReply` shapes from `@geolibre/core` but operate on +// untrusted `unknown` input, returning a sanitized object or `null`. + +/** Body length cap — matches the chat limit so comments can't store unbounded text. */ +export const MAX_COMMENT_BODY_LENGTH = 2000; + +/** Author name length cap — generous for display names but bounded. */ +export const MAX_COMMENT_AUTHOR_LENGTH = 120; + +/** Identifier length cap for comment/reply ids, layerId, and string featureId. */ +export const MAX_ID_LENGTH = 200; + +/** Minimum gap between a socket's comment-mutation frames (ms). */ +export const MIN_COMMENT_INTERVAL_MS = 250; + +/** Maximum number of replies stored per comment. */ +export const MAX_REPLIES_PER_COMMENT = 100; + +/** Maximum number of comments stored per session. Bounds the snapshot growth a + * sustained stream of "add" mutations can cause, the way `CHAT_HISTORY_LIMIT` + * bounds the chat log. */ +export const MAX_COMMENTS_PER_SESSION = 500; + +/** True when `value` is a non-empty string within {@link MAX_ID_LENGTH}. */ +export function isBoundedId(value: unknown): value is string { + return typeof value === "string" && value.length > 0 && value.length <= MAX_ID_LENGTH; +} + +/** Carry the stored `comments` list into an incoming full-project snapshot that + * doesn't supply one of its own. + * + * The relay writes comments straight into the stored snapshot (see + * `handleCommentMutation`), but `serializeProject` omits the key entirely when + * a peer holds none — so a peer that hasn't merged those broadcasts yet (a race + * with its debounced snapshot, or a client that joined before them) would + * otherwise replace the persisted comments with nothing. A project that carries + * its own `comments` still wins, so a delete is never resurrected. + * + * `stored` is the already-parsed stored snapshot, or `null` when absent/corrupt. + */ +export function preserveStoredComments(project: unknown, stored: unknown): unknown { + if (!project || typeof project !== "object" || Array.isArray(project)) return project; + if ("comments" in project) return project; + if (!stored || typeof stored !== "object" || Array.isArray(stored)) return project; + const comments = (stored as Record).comments; + if (!Array.isArray(comments) || comments.length === 0) return project; + return { ...(project as Record), comments }; +} + +const HEX_COLOR_RE = /^#([0-9a-fA-F]{3}|[0-9a-fA-F]{6})$/; + +function finite(n: unknown): n is number { + return typeof n === "number" && Number.isFinite(n); +} + +// -- anchor ------------------------------------------------------------------- + +interface PointAnchor { + type: "point"; + lngLat: [number, number]; +} + +interface FeatureAnchor { + type: "feature"; + layerId: string; + featureId: string | number; + lngLat?: [number, number]; +} + +export type ValidatedAnchor = PointAnchor | FeatureAnchor; + +export function validateAnchor(raw: unknown): ValidatedAnchor | null { + if (!raw || typeof raw !== "object") return null; + const o = raw as Record; + + if (o.type === "point") { + if (!Array.isArray(o.lngLat) || o.lngLat.length !== 2) return null; + const [lng, lat] = o.lngLat; + if (!finite(lng) || !finite(lat)) return null; + return { type: "point", lngLat: [lng, lat] }; + } + + if (o.type === "feature") { + if (!isBoundedId(o.layerId)) return null; + if (typeof o.featureId !== "string" && typeof o.featureId !== "number") return null; + if (typeof o.featureId === "string" && !isBoundedId(o.featureId)) return null; + if (typeof o.featureId === "number" && !finite(o.featureId)) return null; + const anchor: FeatureAnchor = { + type: "feature", + layerId: o.layerId, + featureId: o.featureId, + }; + if (Array.isArray(o.lngLat) && o.lngLat.length === 2) { + const [lng, lat] = o.lngLat; + if (finite(lng) && finite(lat)) { + anchor.lngLat = [lng, lat]; + } + } + return anchor; + } + + return null; +} + +// -- author ------------------------------------------------------------------- + +export interface ValidatedAuthor { + name: string; + color: string; +} + +export function validateAuthor(raw: unknown): ValidatedAuthor | null { + if (!raw || typeof raw !== "object") return null; + const o = raw as Record; + if (typeof o.name !== "string") return null; + const name = o.name.trim().slice(0, MAX_COMMENT_AUTHOR_LENGTH); + if (!name) return null; + if (typeof o.color !== "string" || !HEX_COLOR_RE.test(o.color)) return null; + return { name, color: o.color }; +} + +// -- comment ------------------------------------------------------------------ + +export interface ValidatedComment { + id: string; + anchor: ValidatedAnchor; + author: ValidatedAuthor; + body: string; + createdAt: string; + resolved: boolean; + replies: ValidatedReply[]; +} + +export function validateComment(raw: unknown): ValidatedComment | null { + if (!raw || typeof raw !== "object") return null; + const o = raw as Record; + + if (!isBoundedId(o.id)) return null; + + const anchor = validateAnchor(o.anchor); + if (!anchor) return null; + + const author = validateAuthor(o.author); + if (!author) return null; + + if (typeof o.body !== "string") return null; + const body = o.body.slice(0, MAX_COMMENT_BODY_LENGTH); + if (!body.trim()) return null; + + const createdAt = + typeof o.createdAt === "string" && !Number.isNaN(Date.parse(o.createdAt)) + ? o.createdAt + : new Date().toISOString(); + + const replies: ValidatedReply[] = []; + if (Array.isArray(o.replies)) { + for (const r of o.replies.slice(0, MAX_REPLIES_PER_COMMENT)) { + const validated = validateReply(r); + if (validated) replies.push(validated); + } + } + + return { + id: o.id, + anchor, + author, + body, + createdAt, + resolved: Boolean(o.resolved), + replies, + }; +} + +// -- reply -------------------------------------------------------------------- + +export interface ValidatedReply { + id: string; + author: ValidatedAuthor; + body: string; + createdAt: string; +} + +export function validateReply(raw: unknown): ValidatedReply | null { + if (!raw || typeof raw !== "object") return null; + const o = raw as Record; + + if (!isBoundedId(o.id)) return null; + + const author = validateAuthor(o.author); + if (!author) return null; + + if (typeof o.body !== "string") return null; + const body = o.body.slice(0, MAX_COMMENT_BODY_LENGTH); + if (!body.trim()) return null; + + const createdAt = + typeof o.createdAt === "string" && !Number.isNaN(Date.parse(o.createdAt)) + ? o.createdAt + : new Date().toISOString(); + + return { id: o.id, author, body, createdAt }; +} diff --git a/workers/collab/src/session.ts b/workers/collab/src/session.ts index b32b1fbdd..7b9f637f9 100644 --- a/workers/collab/src/session.ts +++ b/workers/collab/src/session.ts @@ -10,6 +10,15 @@ import type { PresenceEntry, ServerMessage, } from "./protocol"; +import { + isBoundedId, + MAX_COMMENTS_PER_SESSION, + MAX_REPLIES_PER_COMMENT, + MIN_COMMENT_INTERVAL_MS, + preserveStoredComments, + validateComment, + validateReply, +} from "./comment-validate"; function finite(n: unknown): n is number { return typeof n === "number" && Number.isFinite(n); @@ -107,6 +116,8 @@ interface SocketAttachment { editOverride?: boolean; /** Epoch-ms of this socket's last accepted chat frame, for rate-limiting. */ lastChatTs?: number; + /** Epoch-ms of this socket's last accepted comment-mutation frame. */ + lastCommentTs?: number; } /** Effective edit permission: the host always edits; otherwise a host-set @@ -415,9 +426,15 @@ export class CollabSession extends DurableObject { } // The project was parsed in webSocketMessage; re-serialize it only to - // persist a string for storage and forward the object verbatim. The relay - // never reads any field inside the project. - const project = message.project ?? null; + // persist a string for storage and forward the object verbatim. `comments` + // is the one field the relay touches (it also writes it directly in + // `handleCommentMutation`), so preserve the stored list when this snapshot + // doesn't carry one — see `preserveStoredComments`. Peers get the merged + // project below, which heals a sender that had drifted. + const project = preserveStoredComments( + message.project ?? null, + parseStoredSnapshot(await this.ctx.storage.get("snapshot")), + ); // `rev` is written during /init before any socket can join, so the stored // value is always present; the `?? 0` is a defensive floor, never the // client's counter (a server-owned monotonic value must not trust input). @@ -661,6 +678,84 @@ export class CollabSession extends DurableObject { attachment: SocketAttachment, message: Extract, ): Promise { + const action = message.action; + if (!action || typeof action !== "object") { + this.send(ws, { + type: "error", + code: "bad-message", + message: "Missing or invalid comment-mutation action.", + }); + return; + } + + // Validate payloads for add/reply; toggle-resolve and delete only need a + // string commentId and carry no untrusted object bodies. Do this before + // rate-limiting so invalid frames always get bad-message and never consume + // the per-socket interval. + let sanitizedAction = action; + if (action.type === "add") { + const validated = validateComment(action.comment); + if (!validated) { + this.send(ws, { + type: "error", + code: "bad-message", + message: "Invalid comment payload.", + }); + return; + } + sanitizedAction = { type: "add", comment: validated }; + } else if (action.type === "reply") { + if (!isBoundedId(action.commentId)) { + this.send(ws, { + type: "error", + code: "bad-message", + message: "Invalid reply target.", + }); + return; + } + const validated = validateReply(action.reply); + if (!validated) { + this.send(ws, { + type: "error", + code: "bad-message", + message: "Invalid reply payload.", + }); + return; + } + sanitizedAction = { type: "reply", commentId: action.commentId, reply: validated }; + } else if (action.type === "toggle-resolve") { + if (!isBoundedId(action.commentId)) { + this.send(ws, { + type: "error", + code: "bad-message", + message: "Invalid toggle-resolve target.", + }); + return; + } + sanitizedAction = { + type: "toggle-resolve", + commentId: action.commentId, + ...(action.resolved !== undefined ? { resolved: action.resolved === true } : {}), + }; + } else if (action.type === "delete") { + if (!isBoundedId(action.commentId)) { + this.send(ws, { + type: "error", + code: "bad-message", + message: "Invalid delete target.", + }); + return; + } + sanitizedAction = { type: "delete", commentId: action.commentId }; + } else { + this.send(ws, { + type: "error", + code: "bad-message", + message: "Unsupported comment-mutation action type.", + }); + return; + } + const mode = (await this.ctx.storage.get("mode")) ?? "co-edit"; if (!canEdit(attachment, mode)) { this.send(ws, { @@ -671,53 +766,132 @@ export class CollabSession extends DurableObject { return; } + // Rate-limit accepted, authorized 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 sanitizedMessage: Extract = { + type: "comment-mutation", + action: sanitizedAction, + }; + + // Persist even when no full project snapshot has been written yet, so early + // comments survive late joiners / reconnects. Seed an empty object when + // storage is empty or corrupt — the relay never inspects other project fields. const rawSnapshot = await this.ctx.storage.get("snapshot"); + let parsed: Record = { comments: [] }; if (rawSnapshot) { try { - const parsed = JSON.parse(rawSnapshot) as Record; - const comments = Array.isArray(parsed.comments) - ? (parsed.comments as Record[]) + const value = JSON.parse(rawSnapshot) as unknown; + if (value && typeof value === "object" && !Array.isArray(value)) { + parsed = value as Record; + } + } catch { + // Fall through with the empty seed; still fan out below. + } + } + + try { + const comments = Array.isArray(parsed.comments) + ? (parsed.comments as Record[]) + : []; + let updatedComments = comments; + + if (sanitizedAction.type === "add") { + const newComment = sanitizedAction.comment as Record; + const exists = comments.some((c) => c && typeof c === "object" && c.id === newComment.id); + if (!exists && comments.length >= MAX_COMMENTS_PER_SESSION) { + this.send(ws, { + type: "error", + code: "bad-message", + message: "Comment limit reached for this session.", + }); + return; + } + updatedComments = exists ? comments : [...comments, newComment]; + } else if (sanitizedAction.type === "reply") { + const replyObj = sanitizedAction.reply as Record; + const target = comments.find( + (c) => c && typeof c === "object" && c.id === sanitizedAction.commentId, + ); + if (!target) { + this.send(ws, { + type: "error", + code: "bad-message", + message: "Invalid reply target.", + }); + return; + } + const targetReplies = Array.isArray(target.replies) + ? (target.replies as Record[]) : []; - const action = message.action; - if (!action || typeof action !== "object") return; - let updatedComments = comments; - - if (action.type === "add") { - if (!action.comment || typeof action.comment !== "object") return; - updatedComments = [...comments, action.comment as Record]; - } else if (action.type === "reply") { - if (!action.reply || typeof action.reply !== "object") return; - const replyObj = action.reply as Record; - updatedComments = comments.map((c) => { - if (!c || typeof c !== "object" || c.id !== action.commentId) return c; - const existingReplies = Array.isArray(c.replies) - ? (c.replies as Record[]) - : []; - if (existingReplies.some((r) => r && typeof r === "object" && r.id === replyObj.id)) - return c; - return { ...c, replies: [...existingReplies, replyObj] }; + if (targetReplies.length >= MAX_REPLIES_PER_COMMENT) { + this.send(ws, { + type: "error", + code: "bad-message", + message: "Reply limit reached for this comment.", }); - } else if (action.type === "toggle-resolve") { - updatedComments = comments.map((c) => - c && typeof c === "object" && c.id === action.commentId - ? { ...c, resolved: action.resolved !== undefined ? action.resolved : !c.resolved } - : c, - ); - } else if (action.type === "delete") { - updatedComments = comments.filter( - (c) => c && typeof c === "object" && c.id !== action.commentId, - ); + return; } + updatedComments = comments.map((c) => { + if (!c || typeof c !== "object" || c.id !== sanitizedAction.commentId) return c; + const existingReplies = Array.isArray(c.replies) + ? (c.replies as Record[]) + : []; + if (existingReplies.some((r) => r && typeof r === "object" && r.id === replyObj.id)) + return c; + return { ...c, replies: [...existingReplies, replyObj] }; + }); + } else if (sanitizedAction.type === "toggle-resolve") { + updatedComments = comments.map((c) => + c && typeof c === "object" && c.id === sanitizedAction.commentId + ? { + ...c, + resolved: + sanitizedAction.resolved !== undefined ? sanitizedAction.resolved : !c.resolved, + } + : c, + ); + } else if (sanitizedAction.type === "delete") { + updatedComments = comments.filter( + (c) => c && typeof c === "object" && c.id !== sanitizedAction.commentId, + ); + } - parsed.comments = updatedComments; - await this.ctx.storage.put("snapshot", JSON.stringify(parsed)); - } catch { - // Ignore snapshot mutation update errors defensively + parsed.comments = updatedComments; + const serialized = JSON.stringify(parsed); + // `handleSnapshot` bounds a full project the same way. Check before the + // put so an oversized value surfaces as an error the sender can see + // rather than a throw the catch below would have to guess at. + if (ENCODER.encode(serialized).length > MAX_SNAPSHOT_BYTES) { + this.send(ws, { + type: "error", + code: "bad-message", + message: "Project is too large to store this comment.", + }); + return; } + await this.ctx.storage.put("snapshot", serialized); + } catch { + // The mutation never reached storage, so a late joiner or a reconnect + // (both of which read from storage) would not see it. Tell the sender and + // skip the fan-out rather than leaving connected peers holding a comment + // that isn't persisted anywhere. + this.send(ws, { + type: "error", + code: "bad-message", + message: "Could not save the comment. Try again.", + }); + return; } - // 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); } }