fix(webhooks): address wyattjoh review feedback

- Replace --headers (comma-separated) with repeatable -H/--header flag so
  duplicate Set-Cookie headers are preserved end-to-end via Headers.append()
- Strip hop-by-hop headers (host, connection, transfer-encoding, etc.) before
  forwarding so the request looks like it originated at the forward target
- Add .catch() on onTokenRotated() chain to prevent unhandled promise rejection
- Use Promise.withResolvers<void>() in RelayClient.start() and webhooksListen
- Add SIGINT double-press re-entrancy guard and 2s drain timeout in listen
- Upgrade dropped-delivery log from debug to warn
- Add IRelayClient interface so FakeRelayClient in tests can use implements
- Rename cliSigintHandler to CLI_SIGINT_HANDLER (global handler naming convention)
- Reword "Pin it:" to "Use this token:" for clarity

Author: rafa-thayto

packages/cli-core/src/cli.ts

@@ -1,9 +1,9 @@
 #!/usr/bin/env bun
 import { createProgram, runProgram } from "./cli-program.ts";
-import { cliSigintHandler } from "./lib/signals.ts";
+import { CLI_SIGINT_HANDLER } from "./lib/signals.ts";
 // Named handler (not an inline arrow) so `webhooks listen` can removeListener it
 // to install its own graceful-drain SIGINT handling.
-process.on("SIGINT", cliSigintHandler);
+process.on("SIGINT", CLI_SIGINT_HANDLER);
 
 // Fast path for shell completion — intercept before Commander parses
 // to avoid validation errors on partial input from Tab presses.

packages/cli-core/src/commands/webhooks/forward.test.ts

@@ -1,42 +1,29 @@
 import { test, expect, describe, afterEach } from "bun:test";
 import { CliError } from "../../lib/errors.ts";
 import { stubFetch, useCaptureLog } from "../../test/lib/stubs.ts";
-import { buildForwardHeaders, forwardDelivery, parseHeaderPairs } from "./forward.ts";
+import { buildForwardHeaders, forwardDelivery, parseHeaderFlag } from "./forward.ts";
 
 const originalFetch = globalThis.fetch;
 
-describe("parseHeaderPairs", () => {
-  const parseCases: Array<{
-    label: string;
-    value: string | undefined;
-    expected: Record<string, string>;
-  }> = [
-    { label: "undefined", value: undefined, expected: {} },
-    { label: "empty string", value: "", expected: {} },
-    { label: "single pair", value: "x-env:dev", expected: { "x-env": "dev" } },
-    {
-      label: "multiple pairs with whitespace",
-      value: " x-env : dev , x-team:core ",
-      expected: { "x-env": "dev", "x-team": "core" },
-    },
+describe("parseHeaderFlag", () => {
+  test.each<{ label: string; value: string; expected: [string, string] }>([
+    { label: "simple pair", value: "x-env:dev", expected: ["x-env", "dev"] },
     {
       label: "value containing colons (split on FIRST colon)",
       value: "authorization:Bearer abc:def",
-      expected: { authorization: "Bearer abc:def" },
+      expected: ["authorization", "Bearer abc:def"],
     },
-    { label: "trailing comma", value: "x-env:dev,", expected: { "x-env": "dev" } },
-    { label: "empty value", value: "x-empty:", expected: { "x-empty": "" } },
-  ];
-
-  test.each(parseCases)("parses $label", ({ value, expected }) => {
-    expect(parseHeaderPairs(value)).toEqual(expected);
+    { label: "empty value", value: "x-empty:", expected: ["x-empty", ""] },
+    { label: "trims whitespace", value: " x-env : dev ", expected: ["x-env", "dev"] },
+  ])("parses $label", ({ value, expected }) => {
+    expect(parseHeaderFlag(value)).toEqual(expected);
   });
 
   test.each([
     { label: "pair without a colon", value: "not-a-pair" },
     { label: "pair with an empty key", value: ":value" },
   ])("throws a usage error on $label", ({ value }) => {
-    expect(() => parseHeaderPairs(value)).toThrow(CliError);
+    expect(() => parseHeaderFlag(value)).toThrow(CliError);
   });
 });
 
@@ -49,15 +36,17 @@ describe("buildForwardHeaders", () => {
   };
 
   test("preserves delivery headers and adds extras", () => {
-    const headers = buildForwardHeaders(eventHeaders, { "x-env": "dev" });
+    const extra = new Headers({ "x-env": "dev" });
+    const headers = buildForwardHeaders(eventHeaders, extra);
 
     expect(headers.get("svix-id")).toBe("msg_1");
     expect(headers.get("content-type")).toBe("application/json");
     expect(headers.get("x-env")).toBe("dev");
   });
 
   test("extras may override non-svix delivery headers", () => {
-    const headers = buildForwardHeaders(eventHeaders, { "Content-Type": "text/plain" });
+    const extra = new Headers({ "Content-Type": "text/plain" });
+    const headers = buildForwardHeaders(eventHeaders, extra);
 
     expect(headers.get("content-type")).toBe("text/plain");
   });
@@ -67,10 +56,35 @@ describe("buildForwardHeaders", () => {
     { label: "uppercase", key: "SVIX-SIGNATURE" },
     { label: "mixed case", key: "Svix-Signature" },
   ])("extras can never override svix-* headers ($label)", ({ key }) => {
-    const headers = buildForwardHeaders(eventHeaders, { [key]: "v1,forged" });
+    const extra = new Headers({ [key]: "v1,forged" });
+    const headers = buildForwardHeaders(eventHeaders, extra);
 
     expect(headers.get("svix-signature")).toBe("v1,abc");
   });
+
+  test("strips hop-by-hop headers from event headers", () => {
+    const withHopByHop = {
+      ...eventHeaders,
+      host: "svix-relay.example.com",
+      connection: "keep-alive",
+      "transfer-encoding": "chunked",
+    };
+    const headers = buildForwardHeaders(withHopByHop, new Headers());
+
+    expect(headers.has("host")).toBe(false);
+    expect(headers.has("connection")).toBe(false);
+    expect(headers.has("transfer-encoding")).toBe(false);
+    expect(headers.get("svix-id")).toBe("msg_1");
+  });
+
+  test("allows duplicate extra headers via append", () => {
+    const extra = new Headers();
+    extra.append("x-custom", "first");
+    extra.append("x-custom", "second");
+    const headers = buildForwardHeaders({}, extra);
+
+    expect(headers.get("x-custom")).toBe("first, second");
+  });
 });
 
 describe("forwardDelivery", () => {
@@ -95,7 +109,7 @@ describe("forwardDelivery", () => {
     const outcome = await forwardDelivery({
       forwardTo: "http://localhost:3000/api/webhooks",
       method: "POST",
-      headers: buildForwardHeaders({ "svix-id": "msg_1" }, {}),
+      headers: buildForwardHeaders({ "svix-id": "msg_1" }, new Headers()),
       body: '{"type":"user.created"}',
     });
 

packages/cli-core/src/commands/webhooks/forward.ts

@@ -11,39 +11,62 @@ export interface ForwardOutcome {
   failed: boolean;
 }
 
-/** Comma-separated `k:v` pairs, split on the FIRST colon, whitespace trimmed. */
-export function parseHeaderPairs(value: string | undefined): Record<string, string> {
-  if (!value) return {};
-  const headers: Record<string, string> = {};
-  for (const pair of value.split(",")) {
-    const trimmed = pair.trim();
-    if (!trimmed) continue;
-    const colonIndex = trimmed.indexOf(":");
-    const key = colonIndex === -1 ? "" : trimmed.slice(0, colonIndex).trim();
-    if (!key) {
-      throwUsageError(
-        `Invalid --headers pair "${trimmed}". Expected key:value. Commas separate pairs and can't appear in values — use multiple --headers flags instead.`,
-      );
-    }
-    headers[key] = trimmed.slice(colonIndex + 1).trim();
+/**
+ * Hop-by-hop headers that must not be forwarded to the target. These are
+ * meaningful only for the single transport hop and would confuse the target
+ * server or cause protocol violations (e.g. chunked encoding mismatch).
+ */
+const HOP_BY_HOP_HEADERS = new Set([
+  "connection",
+  "keep-alive",
+  "proxy-authenticate",
+  "proxy-authorization",
+  "te",
+  "trailers",
+  "transfer-encoding",
+  "upgrade",
+  "host",
+]);
+
+/**
+ * Parse one `--header` value (`key:value`, split on the FIRST colon, whitespace
+ * trimmed) into a [key, value] pair. Throws a usage error on malformed input.
+ */
+export function parseHeaderFlag(value: string): [string, string] {
+  const colonIndex = value.indexOf(":");
+  const key = colonIndex === -1 ? "" : value.slice(0, colonIndex).trim();
+  if (!key) {
+    throwUsageError(`Invalid --header "${value}". Expected key:value (e.g. --header x-env:dev).`);
   }
-  return headers;
+  return [key, value.slice(colonIndex + 1).trim()];
 }
 
 /**
- * Delivery headers plus `--headers` extras. Extras may override non-svix
- * delivery headers, but the delivery's `svix-*` headers always win — they are
- * what `verify` (and the user's handler) authenticate against.
+ * Delivery headers plus `--header` extras, with hop-by-hop headers stripped.
+ * Extras may override non-svix delivery headers, but the delivery's `svix-*`
+ * headers always win — they are what `verify` (and the user's handler)
+ * authenticate against.
  */
 export function buildForwardHeaders(
   eventHeaders: Record<string, string>,
-  extraHeaders: Record<string, string>,
+  extraHeaders: Headers,
 ): Headers {
-  const headers = new Headers(eventHeaders);
-  for (const [key, value] of Object.entries(extraHeaders)) {
-    if (key.toLowerCase().startsWith("svix-")) continue;
+  const headers = new Headers();
+  for (const [key, value] of Object.entries(eventHeaders)) {
+    if (HOP_BY_HOP_HEADERS.has(key.toLowerCase())) continue;
     headers.set(key, value);
   }
+  const seenExtra = new Set<string>();
+  for (const [key, value] of extraHeaders) {
+    const lower = key.toLowerCase();
+    if (lower.startsWith("svix-")) continue;
+    if (seenExtra.has(lower)) {
+      headers.append(key, value);
+    } else {
+      headers.set(key, value);
+      seenExtra.add(lower);
+    }
+  }
   return headers;
 }
 

packages/cli-core/src/commands/webhooks/index.ts

@@ -53,8 +53,9 @@ export function registerWebhooks(program: Program): void {
       "Pin the relay token so the inbox URL stays fixed across machines. Format: c_ + 10 base62 chars (generate with `clerk webhooks token`)",
     )
     .option(
-      "--headers <pairs>",
-      "Extra headers for the forwarded request, comma-separated k:v pairs; values can't contain commas (svix-* can't be overridden)",
+      "-H, --header <key:value>",
+      "Extra header for the forwarded request (key:value); repeat for multiple headers (svix-* can't be overridden)",
+      (val: string, prev: string[] = []) => [...prev, val],
     )
     .option("--json", "Output as NDJSON (agent/pipe mode)")
     .setExamples([

packages/cli-core/src/commands/webhooks/listen.test.ts

@@ -1,26 +1,18 @@
 import { test, expect, describe, beforeEach, afterEach, mock, spyOn } from "bun:test";
 import { ERROR_CODE } from "../../lib/errors.ts";
 import { stubFetch, useCaptureLog } from "../../test/lib/stubs.ts";
+import type { IRelayClient, RelayClientOptions } from "./relay-client.ts";
 import type { RelayEventFrame } from "./relay-protocol.ts";
 
-type EventHandler = (event: RelayEventFrame, reply: (frame: string) => void) => void;
-
-interface FakeClientOptions {
-  token: string;
-  onEvent: EventHandler;
-  onTokenRotated: (token: string) => Promise<void>;
-  onReconnect: () => void;
-}
-
 const relayClients: FakeRelayClient[] = [];
 const lastClient = () => relayClients.at(-1);
 
-class FakeRelayClient {
+class FakeRelayClient implements IRelayClient {
   token: string;
   started = false;
   stopped = false;
 
-  constructor(readonly options: FakeClientOptions) {
+  constructor(readonly options: RelayClientOptions) {
     this.token = options.token;
     relayClients.push(this);
   }
@@ -110,8 +102,8 @@ describe("webhooks listen (V1, relay-only)", () => {
     mockIsAgent.mockReset();
   });
 
-  test("invalid --headers is a usage error before any persistence", async () => {
-    await expect(webhooksListen({ headers: "not-a-pair" })).rejects.toMatchObject({
+  test("invalid --header is a usage error before any persistence", async () => {
+    await expect(webhooksListen({ header: ["not-a-pair"] })).rejects.toMatchObject({
       code: ERROR_CODE.USAGE_ERROR,
     });
     expect(mockGetRelayEntry).not.toHaveBeenCalled();
@@ -208,8 +200,8 @@ describe("webhooks listen (V1, relay-only)", () => {
     },
   );
 
-  test("svix-* in --headers warns at startup that it can't be overridden", async () => {
-    await startListen({ headers: "svix-id:forged" }, captured);
+  test("svix-* in --header warns at startup that it can't be overridden", async () => {
+    await startListen({ header: ["svix-id:forged"] }, captured);
     expect(captured.err).toContain("can't be overridden");
   });
 
@@ -226,7 +218,7 @@ describe("webhooks listen (V1, relay-only)", () => {
     });
 
     await startListen(
-      { forwardTo: "http://localhost:3000/api/webhooks", headers: "x-env:dev" },
+      { forwardTo: "http://localhost:3000/api/webhooks", header: ["x-env:dev"] },
       captured,
     );
     captured.clear();

packages/cli-core/src/commands/webhooks/listen.ts

@@ -1,11 +1,11 @@
 import { getRelayEntry, setRelayEntry } from "../../lib/config.ts";
 import { EXIT_CODE, errorMessage, throwUsageError } from "../../lib/errors.ts";
-import { cliSigintHandler } from "../../lib/signals.ts";
+import { CLI_SIGINT_HANDLER } from "../../lib/signals.ts";
 import { withSpinner } from "../../lib/spinner.ts";
 import { dim } from "../../lib/color.ts";
 import { log } from "../../lib/log.ts";
 import { isAgent } from "../../mode.ts";
-import { buildForwardHeaders, forwardDelivery, parseHeaderPairs } from "./forward.ts";
+import { buildForwardHeaders, forwardDelivery, parseHeaderFlag } from "./forward.ts";
 import { RelayClient } from "./relay-client.ts";
 import {
   decodeEventBody,
@@ -27,7 +27,7 @@ import type { WebhooksGlobalOptions } from "./shared.ts";
 
 export interface WebhooksListenOptions extends WebhooksGlobalOptions {
   forwardTo?: string;
-  headers?: string;
+  header?: string[];
   token?: string;
 }
 
@@ -84,17 +84,18 @@ function forwardPath(forwardTo: string): string {
 
 export async function webhooksListen(options: WebhooksListenOptions = {}): Promise<void> {
   const ndjson = Boolean(options.json) || isAgent();
-  const extraHeaders = parseHeaderPairs(options.headers);
   const forwardTo = assertForwardTo(options.forwardTo);
 
   if (options.token) assertRelayToken(options.token);
 
-  // svix-* headers can't be overridden (delivery headers always win) — warn once
-  // at startup instead of silently dropping them.
-  for (const key of Object.keys(extraHeaders)) {
+  const extraHeaders = new Headers();
+  for (const raw of options.header ?? []) {
+    const [key, value] = parseHeaderFlag(raw);
     if (key.toLowerCase().startsWith("svix-")) {
-      log.warn(`--headers: "${key}" can't be overridden — delivery svix-* headers always win.`);
+      log.warn(`--header: "${key}" can't be overridden — delivery svix-* headers always win.`);
+      continue;
     }
+    extraHeaders.append(key, value);
   }
 
   // Persist the token so the inbox URL stays stable across runs; --token pins
@@ -111,21 +112,26 @@ export async function webhooksListen(options: WebhooksListenOptions = {}): Promi
   // Deliveries can arrive the moment the relay handshake completes, but the
   // ready banner/line must print first. Gate processing until setup is done;
   // the SIGINT path also resolves it so the drain can never hang.
-  let resolveSetupGate!: () => void;
-  const setupGate = new Promise<void>((resolve) => {
-    resolveSetupGate = resolve;
-  });
+  const { promise: setupGate, resolve: resolveSetupGate } = Promise.withResolvers<void>();
 
   // Own SIGINT handling, registered BEFORE the socket opens. The global handler
   // (cli.ts) is a cleanup-free exit(130) and would fire first, so remove it:
   // close the socket, drain in-flight forwards, then exit 130.
-  process.removeListener("SIGINT", cliSigintHandler);
+  process.removeListener("SIGINT", CLI_SIGINT_HANDLER);
   process.on("SIGINT", () => {
+    if (shuttingDown) {
+      // Double Ctrl+C: force-quit immediately.
+      process.exit(EXIT_CODE.SIGINT);
+    }
     void (async () => {
       shuttingDown = true; // MUST precede resolveSetupGate so processDelivery short-circuits
       resolveSetupGate(); // gated deliveries must settle or the drain hangs
       client?.stop();
-      await Promise.allSettled([...inFlight, ...(tokenRotationTask ? [tokenRotationTask] : [])]);
+      const pending = [...inFlight, ...(tokenRotationTask ? [tokenRotationTask] : [])];
+      await Promise.race([
+        Promise.allSettled(pending),
+        new Promise<void>((resolve) => setTimeout(resolve, 2_000)),
+      ]);
       process.exit(EXIT_CODE.SIGINT);
     })();
   });
@@ -180,7 +186,7 @@ export async function webhooksListen(options: WebhooksListenOptions = {}): Promi
     token,
     onEvent: (event, reply) => {
       const task = processDelivery(event, reply).catch((error) => {
-        log.debug(`relay: delivery handling failed: ${errorMessage(error)}`);
+        log.warn(`relay: delivery dropped: ${errorMessage(error)}`);
       });
       inFlight.add(task);
       void task.finally(() => inFlight.delete(task));