harden(comments): fix toggle race, allowlist reactions, document IP-bucket limit

Adversarial self-review follow-ups on the reactions prototype:

- toggle() no longer does read-then-write. A concurrent double-toggle from the
  same voter could both see "no row" and have the second INSERT violate the
  unique index, surfacing as a 500. Now an idempotent INSERT ... ON CONFLICT
  DO NOTHING branches on whether a row was written, so it never throws.
- handleReactionToggle rejects reactions outside an allowlist (currently just
  "like", matching the shipped widget) so a caller can't spam arbitrary
  reaction strings and bloat a comment's count map.
- The public reactions route documents that the salted-IP voter hash collapses
  to a shared "unknown" bucket without a trusted IP (per-visitor dedup is the
  Tier 2 visitor-identity primitive), mirroring the comment ingest route.

Adds handler-level tests (happy path, unsupported reaction, non-approved and
missing comment, concurrent-toggle invariant). lint + typecheck clean; 82
comment tests pass.

AI disclosure: implemented with assistance from Claude (claude-opus-4-8).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: Bob

packages/core/src/api/handlers/comment-reactions.ts

@@ -19,6 +19,14 @@ import type { ApiResult } from "../types.js";
 const REACTION_RATE_LIMIT = 30;
 const REACTION_RATE_WINDOW_MINUTES = 10;
 
+/**
+ * Reactions the system accepts. Positive-only for now (matches the shipped
+ * widget); kept as an allowlist so a voter can't spam arbitrary reaction
+ * strings and bloat a comment's count map. Extend (or make configurable) as
+ * the UI grows.
+ */
+const ALLOWED_REACTIONS: ReadonlySet<string> = new Set(["like"]);
+
 export interface ReactionToggleResult {
 	commentId: string;
 	reaction: string;
@@ -52,6 +60,13 @@ export async function handleReactionToggle(
 	try {
 		const { collection, contentId, commentId, reaction, voterHash } = params;
 
+		if (!ALLOWED_REACTIONS.has(reaction)) {
+			return {
+				success: false,
+				error: { code: "VALIDATION_ERROR", message: "Unsupported reaction" },
+			};
+		}
+
 		// The comment must exist, be approved, and belong to this content item.
 		const comment = await db
 			.selectFrom("_emdash_comments")

packages/core/src/astro/routes/api/comments/[collection]/[contentId]/reactions.ts

@@ -35,8 +35,12 @@ export const GET: APIRoute = async ({ params, request, locals }) => {
 	if (dbErr) return dbErr;
 
 	try {
-		// Salted voter hash from request IP (same primitive as comment ip_hash);
-		// shared "unknown" bucket when no trusted IP is available.
+		// Salted voter hash from request IP (same primitive as comment ip_hash).
+		// Behind Cloudflare (CF-Connecting-IP) or a configured trusted proxy this
+		// is per-visitor. Without a trusted IP it collapses to a shared "unknown"
+		// bucket, so reaction dedup degrades for those visitors — a real
+		// per-visitor token is Tier 2 (visitor identity). Operators should set
+		// trustedProxyHeaders; see the comment ingest route for the same note.
 		const meta = extractRequestMeta(request, emdash.config);
 		let voterHash = "unknown";
 		if (meta.ip) {

packages/core/src/database/repositories/comment-reaction.ts

@@ -29,20 +29,12 @@ export class CommentReactionRepository {
 	 *   if an existing reaction was removed.
 	 */
 	async toggle(input: ToggleReactionInput): Promise<{ reacted: boolean }> {
-		const existing = await this.db
-			.selectFrom("_emdash_comment_reactions")
-			.select("id")
-			.where("comment_id", "=", input.commentId)
-			.where("voter_hash", "=", input.voterHash)
-			.where("reaction", "=", input.reaction)
-			.executeTakeFirst();
-
-		if (existing) {
-			await this.db.deleteFrom("_emdash_comment_reactions").where("id", "=", existing.id).execute();
-			return { reacted: false };
-		}
-
-		await this.db
+		// Idempotent add: insert, or no-op if this voter already has this
+		// reaction. Using ON CONFLICT (rather than read-then-write) avoids a
+		// TOCTOU race where two concurrent toggles both see "no row" and the
+		// second INSERT violates the unique index — the old path surfaced that
+		// as a 500. The unique index doubles as the conflict target.
+		const inserted = await this.db
 			.insertInto("_emdash_comment_reactions")
 			.values({
 				id: ulid(),
@@ -51,8 +43,21 @@ export class CommentReactionRepository {
 				voter_hash: input.voterHash,
 				created_at: new Date().toISOString(),
 			})
+			.onConflict((oc) => oc.columns(["comment_id", "voter_hash", "reaction"]).doNothing())
+			.executeTakeFirst();
+
+		if ((inserted.numInsertedOrUpdatedRows ?? 0n) > 0n) {
+			return { reacted: true };
+		}
+
+		// The reaction already existed → toggle it off.
+		await this.db
+			.deleteFrom("_emdash_comment_reactions")
+			.where("comment_id", "=", input.commentId)
+			.where("voter_hash", "=", input.voterHash)
+			.where("reaction", "=", input.reaction)
 			.execute();
-		return { reacted: true };
+		return { reacted: false };
 	}
 
 	/**

packages/core/tests/integration/comments/reactions.test.ts

@@ -1,6 +1,7 @@
 import type { Kysely } from "kysely";
 import { afterEach, beforeEach, describe, expect, it } from "vitest";
 
+import { handleReactionToggle } from "../../../src/api/handlers/comment-reactions.js";
 import { getCommentsWithDb } from "../../../src/comments/query.js";
 import { CommentReactionRepository } from "../../../src/database/repositories/comment-reaction.js";
 import { CommentRepository } from "../../../src/database/repositories/comment.js";
@@ -131,4 +132,73 @@ describe("CommentReactionRepository", () => {
 			expect(items[0]?.reactions).toEqual({ like: 1 });
 		});
 	});
+
+	describe("handleReactionToggle", () => {
+		const base = { collection: "post", contentId: "content-1" };
+
+		it("toggles a reaction and returns updated counts", async () => {
+			const c = await approvedComment();
+			const r = await handleReactionToggle(db, {
+				...base,
+				commentId: c.id,
+				reaction: "like",
+				voterHash: "v1",
+			});
+			expect(r.success).toBe(true);
+			if (r.success) {
+				expect(r.data.reacted).toBe(true);
+				expect(r.data.counts).toEqual({ like: 1 });
+			}
+		});
+
+		it("rejects an unsupported reaction with VALIDATION_ERROR", async () => {
+			const c = await approvedComment();
+			const r = await handleReactionToggle(db, {
+				...base,
+				commentId: c.id,
+				reaction: "spam",
+				voterHash: "v1",
+			});
+			expect(r.success).toBe(false);
+			if (!r.success) expect(r.error.code).toBe("VALIDATION_ERROR");
+		});
+
+		it("rejects reacting to a non-approved comment", async () => {
+			const c = await approvedComment({ status: "pending" });
+			const r = await handleReactionToggle(db, {
+				...base,
+				commentId: c.id,
+				reaction: "like",
+				voterHash: "v1",
+			});
+			expect(r.success).toBe(false);
+			if (!r.success) expect(r.error.code).toBe("COMMENT_NOT_APPROVED");
+		});
+
+		it("rejects reacting to a missing comment", async () => {
+			const r = await handleReactionToggle(db, {
+				...base,
+				commentId: "does-not-exist",
+				reaction: "like",
+				voterHash: "v1",
+			});
+			expect(r.success).toBe(false);
+			if (!r.success) expect(r.error.code).toBe("NOT_FOUND");
+		});
+
+		it("does not error when two toggles for the same voter race (idempotent add)", async () => {
+			const c = await approvedComment();
+			// Fire both concurrently — the old read-then-write path could throw a
+			// unique-constraint 500 here; ON CONFLICT makes it safe.
+			const [a, b] = await Promise.all([
+				handleReactionToggle(db, { ...base, commentId: c.id, reaction: "like", voterHash: "v1" }),
+				handleReactionToggle(db, { ...base, commentId: c.id, reaction: "like", voterHash: "v1" }),
+			]);
+			expect(a.success).toBe(true);
+			expect(b.success).toBe(true);
+			// Invariant: never more than one row for the same (comment, voter, reaction).
+			const counts = await reactions.countsForComments([c.id]);
+			expect((counts.get(c.id)?.like ?? 0) <= 1).toBe(true);
+		});
+	});
 });