Dashboard/webhook/Slack hardening (B14, B19, B21)

src/__tests__/audit-web-hardening.test.ts

@@ -0,0 +1,155 @@
+import { describe, expect, test } from "bun:test";
+import { createHmac } from "node:crypto";
+import { checkAuth } from "../auth.js";
+import { escapeSlackText, formatFailure, formatTaskPickedUp } from "../slack.js";
+import { verifyHmacSignature, verifyJiraSignature, verifyLinearSignature } from "../webhook.js";
+
+function sign(body: string, secret: string): string {
+  return createHmac("sha256", secret).update(body).digest("hex");
+}
+
+// ── B19: HMAC verification never throws on malformed headers ──────────────────
+
+describe("verifyHmacSignature (B19)", () => {
+  const secret = "test-secret";
+  const body = '{"type":"Issue","action":"create"}';
+
+  test("returns true for a valid signature", () => {
+    expect(verifyHmacSignature(body, sign(body, secret), secret)).toBe(true);
+  });
+
+  test("returns false (does not throw) for a multi-byte header", () => {
+    // A multi-byte character makes JS string .length differ from byte length,
+    // which is exactly the case that made the old code throw a RangeError.
+    const multiByte = "café-signature-🔥-with-emoji";
+    expect(() => verifyHmacSignature(body, multiByte, secret)).not.toThrow();
+    expect(verifyHmacSignature(body, multiByte, secret)).toBe(false);
+  });
+
+  test("returns false (does not throw) for an odd-length hex header", () => {
+    expect(() => verifyHmacSignature(body, "abc", secret)).not.toThrow();
+    expect(verifyHmacSignature(body, "abc", secret)).toBe(false);
+  });
+
+  test("returns false (does not throw) for a 64-unit header whose byte length differs", () => {
+    // Reproduces the original B19 bug: the old guard compared JS string
+    // `.length` (UTF-16 code units). This header is exactly 64 code units —
+    // matching the 64-char hex digest length — but its multi-byte char makes the
+    // UTF-8 byte length 65, so the old code passed the length guard and then
+    // threw a RangeError inside timingSafeEqual on mismatched buffer sizes.
+    const header = `${"0".repeat(63)}Ω`;
+    expect(header.length).toBe(64);
+    expect(Buffer.byteLength(header, "utf8")).toBe(65);
+    expect(() => verifyHmacSignature(body, header, secret)).not.toThrow();
+    expect(verifyHmacSignature(body, header, secret)).toBe(false);
+  });
+
+  test("returns false for a wrong but valid-length hex signature", () => {
+    const wrong = "0".repeat(64);
+    expect(verifyHmacSignature(body, wrong, secret)).toBe(false);
+  });
+
+  test("verifyLinearSignature accepts a valid signature", () => {
+    expect(verifyLinearSignature(body, sign(body, secret), secret)).toBe(true);
+  });
+
+  test("verifyLinearSignature does not throw on a multi-byte header", () => {
+    expect(() => verifyLinearSignature(body, "Ω".repeat(10), secret)).not.toThrow();
+    expect(verifyLinearSignature(body, "Ω".repeat(10), secret)).toBe(false);
+  });
+
+  test("verifyJiraSignature strips the sha256= prefix and accepts a valid signature", () => {
+    expect(verifyJiraSignature(body, `sha256=${sign(body, secret)}`, secret)).toBe(true);
+  });
+
+  test("verifyJiraSignature does not throw on a multi-byte header", () => {
+    expect(() => verifyJiraSignature(body, "sha256=日本語", secret)).not.toThrow();
+    expect(verifyJiraSignature(body, "sha256=日本語", secret)).toBe(false);
+  });
+});
+
+// ── B21: Slack mrkdwn escaping neutralizes injection ──────────────────────────
+
+describe("escapeSlackText (B21)", () => {
+  test("neutralizes <!channel> broadcast injection", () => {
+    const out = escapeSlackText("<!channel>");
+    expect(out).toBe("&lt;!channel&gt;");
+    expect(out).not.toContain("<!channel>");
+  });
+
+  test("neutralizes <@U123> mention and <url|text> link injection", () => {
+    expect(escapeSlackText("<@U123>")).toBe("&lt;@U123&gt;");
+    expect(escapeSlackText("<http://evil|click>")).toBe("&lt;http://evil|click&gt;");
+  });
+
+  test("escapes & before < and > (order matters)", () => {
+    expect(escapeSlackText("a & b < c > d")).toBe("a &amp; b &lt; c &gt; d");
+  });
+
+  test("format helpers escape untrusted fields", () => {
+    const msg = formatFailure("ACK-1", "<!channel> please fix", "error <@U999>");
+    expect(msg).not.toContain("<!channel>");
+    expect(msg).not.toContain("<@U999>");
+    expect(msg).toContain("&lt;!channel&gt;");
+    expect(msg).toContain("&lt;@U999&gt;");
+  });
+
+  test("format helpers leave URL fields unescaped (so query-string '&' and auto-linking survive)", () => {
+    const url = "https://github.com/o/r/pull/1?a=1&b=2";
+    const msg = formatTaskPickedUp("ACK-1", "Title", url);
+    expect(msg).toContain(url);
+    expect(msg).not.toContain("&amp;");
+  });
+});
+
+// ── Timing-safe dashboard token comparison ────────────────────────────────────
+
+describe("checkAuth timing-safe comparison", () => {
+  const token = "my-secret-token";
+
+  test("accepts the correct Bearer token", () => {
+    const req = new Request("http://localhost/poll", {
+      method: "POST",
+      headers: { Authorization: `Bearer ${token}` },
+    });
+    expect(checkAuth(req, token)).toBeNull();
+  });
+
+  test("accepts the correct cookie token", () => {
+    const req = new Request("http://localhost/poll", {
+      method: "POST",
+      headers: { Cookie: `critters_token=${token}` },
+    });
+    expect(checkAuth(req, token)).toBeNull();
+  });
+
+  test("rejects a wrong token of equal length", () => {
+    const wrong = "x".repeat(token.length);
+    const req = new Request("http://localhost/poll", {
+      method: "POST",
+      headers: { Authorization: `Bearer ${wrong}` },
+    });
+    expect(checkAuth(req, token)?.status).toBe(401);
+  });
+
+  test("rejects a token of a different length without throwing", () => {
+    const req = new Request("http://localhost/poll", {
+      method: "POST",
+      headers: { Authorization: "Bearer short" },
+    });
+    expect(() => checkAuth(req, token)).not.toThrow();
+    expect(checkAuth(req, token)?.status).toBe(401);
+  });
+
+  test("rejects a decoded multi-byte cookie token without throwing", () => {
+    // Cookie values are percent-decoded, so this yields an actual multi-byte
+    // string whose byte length differs from the configured token's — the case
+    // that would make a naive timingSafeEqual throw.
+    const req = new Request("http://localhost/poll", {
+      method: "POST",
+      headers: { Cookie: "critters_token=%E6%97%A5%E6%9C%AC%E8%AA%9E" },
+    });
+    expect(() => checkAuth(req, token)).not.toThrow();
+    expect(checkAuth(req, token)?.status).toBe(401);
+  });
+});

src/__tests__/mcp.test.ts

@@ -1,4 +1,5 @@
 import { describe, expect, test } from "bun:test";
+import { homedir } from "node:os";
 import { ClaudeCodeAdapter } from "../cli/claude.js";
 import { CodexAdapter } from "../cli/codex.js";
 import { resolvePhaseMcpConfig } from "../cli/mcp.js";
@@ -25,7 +26,7 @@ describe("resolvePhaseMcpConfig", () => {
       } as Config,
     );
 
-    expect(result.mcpConfig).toEqual([`${process.env.HOME}/.critters/review-mcp.json`]);
+    expect(result.mcpConfig).toEqual([`${homedir()}/.critters/review-mcp.json`]);
     expect(result.strictMcpConfig).toBe(true);
   });
 

src/auth.ts

@@ -1,14 +1,31 @@
+import { timingSafeEqual } from "node:crypto";
+
 export function checkAuth(req: Request, token: string | undefined): Response | null {
   if (token === undefined) return null;
 
   const header = req.headers.get("Authorization");
-  if (header === `Bearer ${token}`) return null;
+  if (header?.startsWith("Bearer ") && safeEqual(header.slice("Bearer ".length), token)) {
+    return null;
+  }
 
-  if (readCookie(req, "critters_token") === token) return null;
+  const cookie = readCookie(req, "critters_token");
+  if (cookie !== null && safeEqual(cookie, token)) return null;
 
   return Response.json({ error: "Unauthorized" }, { status: 401 });
 }
 
+/**
+ * Constant-time string comparison. Guards on byte length first (timingSafeEqual
+ * throws on length mismatch) so the dashboard token can't be probed via a timing
+ * oracle when the daemon is exposed through a tunnel.
+ */
+function safeEqual(a: string, b: string): boolean {
+  const aBuf = Buffer.from(a);
+  const bBuf = Buffer.from(b);
+  if (aBuf.length !== bBuf.length) return false;
+  return timingSafeEqual(aBuf, bBuf);
+}
+
 function readCookie(req: Request, name: string): string | null {
   const cookie = req.headers.get("Cookie");
   if (!cookie) return null;

src/health.ts

@@ -344,7 +344,12 @@ export function startHealthServer(
                 const stillActive = currentStatus.activeCritterDetails.some((d) => d.identifier === identifier);
                 if (!stillActive) {
                   send(JSON.stringify({ event: "done" }));
+                  // Producer-side close: flip `closed` and tear down BOTH timers.
+                  // `cancel()` only fires on consumer cancel, so without this the
+                  // 15s heartbeat would keep firing forever after the critter ends.
+                  closed = true;
                   clearInterval(pollTimer);
+                  clearInterval(heartbeatTimer);
                   try { controller.close(); } catch {}
                 }
               }, 500);

src/slack.ts

@@ -146,33 +146,45 @@ export async function sendSlackNotification(
   }
 }
 
+/**
+ * Escape text for Slack mrkdwn. Per Slack's rules only `&`, `<` and `>` must be
+ * encoded (in that order — `&` first). This neutralizes injection of control
+ * sequences like `<!channel>`, `<@U123>` and `<url|text>` via untrusted
+ * free-text fields (issue titles, error messages, reasons). URL fields are left
+ * verbatim: escaping `&` to `&amp;` would corrupt query strings and break
+ * Slack's auto-linking.
+ */
+export function escapeSlackText(text: string): string {
+  return text.replace(/&/g, "&amp;").replace(/</g, "&lt;").replace(/>/g, "&gt;");
+}
+
 export function formatSuccess(identifier: string, title: string, prUrl: string, duration?: string): string {
   const durationSuffix = duration ? ` (completed in ${duration})` : "";
-  return `*${identifier}* — ${title}\nPR created: ${prUrl}${durationSuffix}`;
+  return `*${escapeSlackText(identifier)}* — ${escapeSlackText(title)}\nPR created: ${prUrl}${durationSuffix}`;
 }
 
 export function formatFailure(identifier: string, title: string, error: string, duration?: string): string {
   const durationPrefix = duration ? ` after ${duration}` : "";
-  return `*${identifier}* — ${title}\nFailed${durationPrefix}: ${error}`;
+  return `*${escapeSlackText(identifier)}* — ${escapeSlackText(title)}\nFailed${durationPrefix}: ${escapeSlackText(error)}`;
 }
 
 export function formatReviewMerged(identifier: string, title: string, prUrl: string, duration?: string): string {
   const durationSuffix = duration ? ` (reviewed in ${duration})` : "";
-  return `*${identifier}* — ${title}\nPR merged: ${prUrl}${durationSuffix}`;
+  return `*${escapeSlackText(identifier)}* — ${escapeSlackText(title)}\nPR merged: ${prUrl}${durationSuffix}`;
 }
 
 export function formatReviewNeedsChanges(identifier: string, title: string, reason: string, duration?: string): string {
   const durationSuffix = duration ? ` (reviewed in ${duration})` : "";
-  return `*${identifier}* — ${title}\nNeeds changes: ${reason}${durationSuffix}`;
+  return `*${escapeSlackText(identifier)}* — ${escapeSlackText(title)}\nNeeds changes: ${escapeSlackText(reason)}${durationSuffix}`;
 }
 
 export function formatReviewFailure(identifier: string, title: string, error: string, duration?: string): string {
   const durationPrefix = duration ? ` after ${duration}` : "";
-  return `*${identifier}* — ${title}\nReview failed${durationPrefix}: ${error}`;
+  return `*${escapeSlackText(identifier)}* — ${escapeSlackText(title)}\nReview failed${durationPrefix}: ${escapeSlackText(error)}`;
 }
 
 export function formatTaskPickedUp(identifier: string, title: string, repoUrl: string): string {
-  return `*${identifier}* — ${title}\nPicked up — cloning ${repoUrl}...`;
+  return `*${escapeSlackText(identifier)}* — ${escapeSlackText(title)}\nPicked up — cloning ${repoUrl}...`;
 }
 
 export function formatPlanningComplete(
@@ -185,11 +197,11 @@ export function formatPlanningComplete(
   if (numTurns != null) stats.push(`${numTurns} turns`);
   if (costUsd != null) stats.push(`$${costUsd.toFixed(2)}`);
   const suffix = stats.length > 0 ? ` (${stats.join(", ")})` : "";
-  return `*${identifier}* — ${title}\nPlanning complete${suffix} — executing...`;
+  return `*${escapeSlackText(identifier)}* — ${escapeSlackText(title)}\nPlanning complete${suffix} — executing...`;
 }
 
 export function formatReviewStarted(identifier: string, title: string, prUrl: string): string {
-  return `*${identifier}* — ${title}\nReview started: ${prUrl}`;
+  return `*${escapeSlackText(identifier)}* — ${escapeSlackText(title)}\nReview started: ${prUrl}`;
 }
 
 export function formatTimeoutWarning(
@@ -198,7 +210,7 @@ export function formatTimeoutWarning(
   elapsedMinutes: number,
   timeoutMinutes: number,
 ): string {
-  return `*${identifier}* — ${title}\n⚠️ Running for ${elapsedMinutes}/${timeoutMinutes} minutes`;
+  return `*${escapeSlackText(identifier)}* — ${escapeSlackText(title)}\n⚠️ Running for ${elapsedMinutes}/${timeoutMinutes} minutes`;
 }
 
 export function formatCostBudgetExceeded(
@@ -208,7 +220,7 @@ export function formatCostBudgetExceeded(
   budget: number,
   currentPhase: string,
 ): string {
-  return `*${identifier}* — ${title}\n:no_entry: Killed: cost budget exceeded ($${costUsd.toFixed(2)} spent, $${budget.toFixed(2)} budget) — killed during phase: ${currentPhase}`;
+  return `*${escapeSlackText(identifier)}* — ${escapeSlackText(title)}\n:no_entry: Killed: cost budget exceeded ($${costUsd.toFixed(2)} spent, $${budget.toFixed(2)} budget) — killed during phase: ${escapeSlackText(currentPhase)}`;
 }
 
 export function formatCostAlert(
@@ -218,5 +230,5 @@ export function formatCostAlert(
   threshold: number,
   currentPhase: string,
 ): string {
-  return `*${identifier}* — ${title}\n⚠️ Cost alert: spent *$${costUsd.toFixed(2)}* (threshold: $${threshold.toFixed(2)}) — currently in phase: ${currentPhase}`;
+  return `*${escapeSlackText(identifier)}* — ${escapeSlackText(title)}\n⚠️ Cost alert: spent *$${costUsd.toFixed(2)}* (threshold: $${threshold.toFixed(2)}) — currently in phase: ${escapeSlackText(currentPhase)}`;
 }

src/webhook.ts

@@ -19,10 +19,29 @@ export interface LinearWebhookPayload {
   };
 }
 
+/**
+ * Verify an HMAC-SHA256 signature in constant time.
+ *
+ * Comparison happens on the decoded *bytes*, not the hex strings, and never
+ * throws: a malformed (e.g. odd-length or multi-byte) header decodes to a
+ * different byte length and is rejected cleanly with `false` instead of letting
+ * `timingSafeEqual` raise a RangeError (which would surface as an HTTP 500
+ * rather than a clean 401). A `sha256=` prefix is stripped (Jira sends one,
+ * Linear does not).
+ */
+export function verifyHmacSignature(body: string, header: string, secret: string): boolean {
+  const expected = Buffer.from(createHmac("sha256", secret).update(body).digest("hex"), "hex");
+  const providedHex = header.replace(/^sha256=/, "");
+  // `Buffer.from(..., "hex")` never throws in Bun — malformed/odd-length hex is
+  // silently truncated, so the byte-length compare below is what rejects bad
+  // input. The decoded byte lengths must match before timingSafeEqual runs.
+  const provided = Buffer.from(providedHex, "hex");
+  if (provided.length !== expected.length) return false;
+  return timingSafeEqual(provided, expected);
+}
+
 export function verifyLinearSignature(body: string, signature: string, secret: string): boolean {
-  const expected = createHmac("sha256", secret).update(body).digest("hex");
-  if (expected.length !== signature.length) return false;
-  return timingSafeEqual(Buffer.from(expected), Buffer.from(signature));
+  return verifyHmacSignature(body, signature, secret);
 }
 
 export function extractLinearWebhookTrigger(
@@ -83,10 +102,7 @@ export interface JiraWebhookPayload {
 }
 
 export function verifyJiraSignature(body: string, signatureHeader: string, secret: string): boolean {
-  const expected = createHmac("sha256", secret).update(body).digest("hex");
-  const provided = signatureHeader.replace(/^sha256=/, "");
-  if (expected.length !== provided.length) return false;
-  return timingSafeEqual(Buffer.from(expected), Buffer.from(provided));
+  return verifyHmacSignature(body, signatureHeader, secret);
 }
 
 export function extractJiraWebhookTrigger(