security: SSRF guard covers IPv6 + DNS-rebind defence via pinned IP (#49)

Expand the SSRF blocklist from IPv4-only to IPv6 loopback/ULA (fc00::/7)/ link-local (fe80::/10)/multicast/IPv4-mapped, plus the missing IPv4 ranges 0.0.0.0/8, 100.64.0.0/10 CGNAT, and TEST-NET/benchmark ranges. Replace the single-lookup SSRF guard with resolveAndValidate(): resolves all DNS records (lookup { all: true }) so a hostname returning "public + private" is rejected, and returns the first validated address for connection pinning. The webhook dispatcher now switches from plain fetch() to https.request() with a custom Agent.lookup that returns the pre-validated IP. A DNS rebind between the guard check and the TCP connect() can no longer redirect the dial to an internal address. Hostname still flows through for SNI and certificate validation. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
2026-04-17 09:19:07 +02:00
parent 3222bec8a5
commit 4ff7bc90c3
7 changed files with 467 additions and 125 deletions
@@ -51,6 +51,7 @@ describe("assistant user self-service MFA tools - enable flow", () => {
          totpEnabled: false,
        }),
        update: vi.fn().mockResolvedValue({}),
+        updateMany: vi.fn().mockResolvedValue({ count: 1 }),
      },
      auditLog: {
        create: vi.fn().mockResolvedValue({ id: "audit_1" }),
@@ -75,9 +76,17 @@ describe("assistant user self-service MFA tools - enable flow", () => {
        lastTotpAt: true,
      },
    });
+    // Atomic-CAS replay guard: lastTotpAt is set by updateMany with a
+    // conditional WHERE; the subsequent update toggles totpEnabled only.
+    expect(db.user.updateMany).toHaveBeenCalledWith(
+      expect.objectContaining({
+        where: expect.objectContaining({ id: "user_1" }),
+        data: { lastTotpAt: expect.any(Date) },
+      }),
+    );
    expect(db.user.update).toHaveBeenCalledWith({
      where: { id: "user_1" },
-      data: { totpEnabled: true, lastTotpAt: expect.any(Date) },
+      data: { totpEnabled: true },
    });
    expect(db.auditLog.create).toHaveBeenCalledWith({
      data: expect.objectContaining({
@@ -1,16 +1,17 @@
 import { describe, expect, it, vi } from "vitest";
-import { assertWebhookUrlAllowed } from "../lib/ssrf-guard.js";
+import { __test__, assertWebhookUrlAllowed, resolveAndValidate } from "../lib/ssrf-guard.js";

 // Mock dns.lookup so tests do not require real DNS resolution.
+// The guard now calls lookup(host, { all: true }) and receives an array.
 vi.mock("node:dns/promises", () => ({
  lookup: vi.fn(async (hostname: string) => {
-    const mapping: Record<string, string> = {
-      "example.com": "93.184.216.34",
-      "hooks.external.io": "52.1.2.3",
+    const mapping: Record<string, Array<{ address: string; family: number }>> = {
+      "example.com": [{ address: "93.184.216.34", family: 4 }],
+      "hooks.external.io": [{ address: "52.1.2.3", family: 4 }],
    };
-    const ip = mapping[hostname];
-    if (!ip) throw new Error(`ENOTFOUND ${hostname}`);
-    return { address: ip, family: 4 };
+    const addrs = mapping[hostname];
+    if (!addrs) throw new Error(`ENOTFOUND ${hostname}`);
+    return addrs;
  }),
 }));

@@ -18,9 +19,7 @@ describe("assertWebhookUrlAllowed — SSRF guard", () => {
  // ── Allowed targets ─────────────────────────────────────────────────────────

  it("allows a valid HTTPS URL that resolves to a public IP", async () => {
-    await expect(
-      assertWebhookUrlAllowed("https://example.com/webhook"),
-    ).resolves.toBeUndefined();
+    await expect(assertWebhookUrlAllowed("https://example.com/webhook")).resolves.toBeUndefined();
  });

  it("allows an HTTPS URL with a path and query string", async () => {
@@ -32,29 +31,29 @@ describe("assertWebhookUrlAllowed — SSRF guard", () => {
  // ── Rejected schemes ─────────────────────────────────────────────────────────

  it("rejects an HTTP URL (only HTTPS allowed)", async () => {
-    await expect(
-      assertWebhookUrlAllowed("http://example.com/webhook"),
-    ).rejects.toMatchObject({ code: "BAD_REQUEST" });
+    await expect(assertWebhookUrlAllowed("http://example.com/webhook")).rejects.toMatchObject({
+      code: "BAD_REQUEST",
+    });
  });

  it("rejects an FTP URL", async () => {
-    await expect(
-      assertWebhookUrlAllowed("ftp://example.com/file"),
-    ).rejects.toMatchObject({ code: "BAD_REQUEST" });
+    await expect(assertWebhookUrlAllowed("ftp://example.com/file")).rejects.toMatchObject({
+      code: "BAD_REQUEST",
+    });
  });

  it("rejects a completely invalid URL", async () => {
-    await expect(
-      assertWebhookUrlAllowed("not-a-url"),
-    ).rejects.toMatchObject({ code: "BAD_REQUEST" });
+    await expect(assertWebhookUrlAllowed("not-a-url")).rejects.toMatchObject({
+      code: "BAD_REQUEST",
+    });
  });

  // ── Blocked hostnames ────────────────────────────────────────────────────────

  it("rejects localhost by hostname", async () => {
-    await expect(
-      assertWebhookUrlAllowed("https://localhost/callback"),
-    ).rejects.toMatchObject({ code: "BAD_REQUEST" });
+    await expect(assertWebhookUrlAllowed("https://localhost/callback")).rejects.toMatchObject({
+      code: "BAD_REQUEST",
+    });
  });

  it("rejects the AWS cloud metadata endpoint by hostname", async () => {
@@ -72,39 +71,39 @@ describe("assertWebhookUrlAllowed — SSRF guard", () => {
  // ── Blocked IP ranges (direct IP addresses as hostname) ─────────────────────

  it("rejects IPv4 loopback 127.0.0.1", async () => {
-    await expect(
-      assertWebhookUrlAllowed("https://127.0.0.1/callback"),
-    ).rejects.toMatchObject({ code: "BAD_REQUEST" });
+    await expect(assertWebhookUrlAllowed("https://127.0.0.1/callback")).rejects.toMatchObject({
+      code: "BAD_REQUEST",
+    });
  });

  it("rejects IPv4 loopback 127.1.2.3 (full /8 block)", async () => {
-    await expect(
-      assertWebhookUrlAllowed("https://127.1.2.3/callback"),
-    ).rejects.toMatchObject({ code: "BAD_REQUEST" });
+    await expect(assertWebhookUrlAllowed("https://127.1.2.3/callback")).rejects.toMatchObject({
+      code: "BAD_REQUEST",
+    });
  });

  it("rejects RFC 1918 private address 10.0.0.1", async () => {
-    await expect(
-      assertWebhookUrlAllowed("https://10.0.0.1/callback"),
-    ).rejects.toMatchObject({ code: "BAD_REQUEST" });
+    await expect(assertWebhookUrlAllowed("https://10.0.0.1/callback")).rejects.toMatchObject({
+      code: "BAD_REQUEST",
+    });
  });

  it("rejects RFC 1918 private address 172.16.0.1", async () => {
-    await expect(
-      assertWebhookUrlAllowed("https://172.16.0.1/callback"),
-    ).rejects.toMatchObject({ code: "BAD_REQUEST" });
+    await expect(assertWebhookUrlAllowed("https://172.16.0.1/callback")).rejects.toMatchObject({
+      code: "BAD_REQUEST",
+    });
  });

  it("rejects RFC 1918 private address 192.168.1.100", async () => {
-    await expect(
-      assertWebhookUrlAllowed("https://192.168.1.100/callback"),
-    ).rejects.toMatchObject({ code: "BAD_REQUEST" });
+    await expect(assertWebhookUrlAllowed("https://192.168.1.100/callback")).rejects.toMatchObject({
+      code: "BAD_REQUEST",
+    });
  });

  it("rejects link-local address 169.254.1.1", async () => {
-    await expect(
-      assertWebhookUrlAllowed("https://169.254.1.1/callback"),
-    ).rejects.toMatchObject({ code: "BAD_REQUEST" });
+    await expect(assertWebhookUrlAllowed("https://169.254.1.1/callback")).rejects.toMatchObject({
+      code: "BAD_REQUEST",
+    });
  });

  // ── DNS fail-closed behaviour ────────────────────────────────────────────────
@@ -120,10 +119,94 @@ describe("assertWebhookUrlAllowed — SSRF guard", () => {

  it("rejects a public hostname that resolves to a private IP (DNS rebinding)", async () => {
    const { lookup } = await import("node:dns/promises");
-    vi.mocked(lookup).mockResolvedValueOnce({ address: "192.168.0.1", family: 4 });
+    vi.mocked(lookup).mockResolvedValueOnce([{ address: "192.168.0.1", family: 4 }]);

+    await expect(assertWebhookUrlAllowed("https://rebind.example.com/hook")).rejects.toMatchObject({
+      code: "BAD_REQUEST",
+    });
+  });
+
+  it("rejects if ANY of the resolved addresses is private (multi-record attack)", async () => {
+    const { lookup } = await import("node:dns/promises");
+    vi.mocked(lookup).mockResolvedValueOnce([
+      { address: "93.184.216.34", family: 4 },
+      { address: "10.0.0.5", family: 4 },
+    ]);
+    await expect(assertWebhookUrlAllowed("https://multi.example.com/hook")).rejects.toMatchObject({
+      code: "BAD_REQUEST",
+    });
+  });
+
+  it("resolveAndValidate returns the first validated address for connection pinning", async () => {
+    const resolved = await resolveAndValidate("https://example.com/hook");
+    expect(resolved.address).toBe("93.184.216.34");
+    expect(resolved.family).toBe(4);
+    expect(resolved.hostname).toBe("example.com");
+  });
+
+  // ── IPv6 blocklist ───────────────────────────────────────────────────────────
+
+  it("rejects IPv6 loopback ::1", async () => {
+    await expect(assertWebhookUrlAllowed("https://[::1]/hook")).rejects.toMatchObject({
+      code: "BAD_REQUEST",
+    });
+  });
+
+  it("rejects IPv6 unique-local fc00::/7 (fc00::1)", async () => {
+    await expect(assertWebhookUrlAllowed("https://[fc00::1]/hook")).rejects.toMatchObject({
+      code: "BAD_REQUEST",
+    });
+  });
+
+  it("rejects IPv6 link-local fe80::/10 (fe80::1)", async () => {
+    await expect(assertWebhookUrlAllowed("https://[fe80::1]/hook")).rejects.toMatchObject({
+      code: "BAD_REQUEST",
+    });
+  });
+
+  it("rejects IPv4-mapped IPv6 (::ffff:192.168.1.1) pointing into private v4", async () => {
    await expect(
-      assertWebhookUrlAllowed("https://rebind.example.com/hook"),
+      assertWebhookUrlAllowed("https://[::ffff:192.168.1.1]/hook"),
    ).rejects.toMatchObject({ code: "BAD_REQUEST" });
  });
+
+  it("rejects IPv6 multicast (ff02::1)", async () => {
+    await expect(assertWebhookUrlAllowed("https://[ff02::1]/hook")).rejects.toMatchObject({
+      code: "BAD_REQUEST",
+    });
+  });
+
+  it("rejects 0.0.0.0/8", async () => {
+    await expect(assertWebhookUrlAllowed("https://0.0.0.0/hook")).rejects.toMatchObject({
+      code: "BAD_REQUEST",
+    });
+  });
+
+  it("rejects 100.64.0.0/10 CGNAT", async () => {
+    await expect(assertWebhookUrlAllowed("https://100.64.1.1/hook")).rejects.toMatchObject({
+      code: "BAD_REQUEST",
+    });
+    await expect(assertWebhookUrlAllowed("https://100.127.254.254/hook")).rejects.toMatchObject({
+      code: "BAD_REQUEST",
+    });
+  });
+
+  it("accepts a 100.x address outside the CGNAT /10 (100.63.x is public)", async () => {
+    // 100.63.x is not in 100.64.0.0/10 — it is part of the public IANA pool.
+    expect(__test__.isBlockedIpv4("100.63.1.1")).toBe(false);
+  });
+
+  it("rejects 198.18.0.0/15 benchmark and TEST-NET ranges", async () => {
+    expect(__test__.isBlockedIpv4("198.18.0.1")).toBe(true);
+    expect(__test__.isBlockedIpv4("192.0.2.1")).toBe(true);
+    expect(__test__.isBlockedIpv4("203.0.113.1")).toBe(true);
+  });
+
+  it("expandIpv6 normalises short-form addresses to full 8-group form", () => {
+    expect(__test__.expandIpv6("::1")).toBe("0000:0000:0000:0000:0000:0000:0000:0001");
+    expect(__test__.expandIpv6("fe80::1")).toBe("fe80:0000:0000:0000:0000:0000:0000:0001");
+    expect(__test__.expandIpv6("::ffff:192.168.1.1")).toBe(
+      "0000:0000:0000:0000:0000:ffff:c0a8:0101",
+    );
+  });
 });
@@ -716,19 +716,26 @@ describe("user profile and TOTP self-service", () => {
      totpEnabled: false,
    });
    const update = vi.fn().mockResolvedValue({});
+    const updateMany = vi.fn().mockResolvedValue({ count: 1 });
    const caller = createAdminCaller({
      user: {
        findUnique,
        update,
+        updateMany,
      },
    });

    const result = await caller.verifyAndEnableTotp({ token: "123456" });

    expect(result).toEqual({ enabled: true });
+    // lastTotpAt is written atomically by updateMany (the replay guard);
+    // user.update only toggles the enabled flag after the CAS succeeds.
+    expect(updateMany).toHaveBeenCalledWith(
+      expect.objectContaining({ data: { lastTotpAt: expect.any(Date) } }),
+    );
    expect(update).toHaveBeenCalledWith({
      where: { id: "user_admin" },
-      data: { totpEnabled: true, lastTotpAt: expect.any(Date) },
+      data: { totpEnabled: true },
    });
  });

@@ -743,10 +750,12 @@ describe("user profile and TOTP self-service", () => {
      lastTotpAt: null,
    });
    const update = vi.fn().mockResolvedValue({});
+    const updateMany = vi.fn().mockResolvedValue({ count: 1 });
    const caller = createAdminCaller({
      user: {
        findUnique,
        update,
+        updateMany,
      },
    });

@@ -757,10 +766,9 @@ describe("user profile and TOTP self-service", () => {
      where: { id: "user_admin" },
      select: { id: true, totpSecret: true, totpEnabled: true, lastTotpAt: true },
    });
-    expect(update).toHaveBeenCalledWith({
-      where: { id: "user_admin" },
-      data: { lastTotpAt: expect.any(Date) },
-    });
+    expect(updateMany).toHaveBeenCalledWith(
+      expect.objectContaining({ data: { lastTotpAt: expect.any(Date) } }),
+    );
  });

  it("rejects invalid login-flow TOTP tokens with UNAUTHORIZED", async () => {
@@ -71,6 +71,7 @@ function makeSelfServiceCtx(dbOverrides: Record<string, unknown> = {}) {
      user: {
        findUnique: vi.fn(),
        update: vi.fn().mockResolvedValue({}),
+        updateMany: vi.fn().mockResolvedValue({ count: 1 }),
        ...((dbOverrides.user as object | undefined) ?? {}),
      },
      auditLog: {
@@ -96,6 +97,7 @@ function makePublicCtx(overrides: Record<string, unknown> = {}) {
      user: {
        findUnique: vi.fn(),
        update: vi.fn().mockResolvedValue({}),
+        updateMany: vi.fn().mockResolvedValue({ count: 1 }),
        ...((overrides.user as object | undefined) ?? {}),
      },
    },
@@ -152,9 +154,12 @@ describe("verifyAndEnableTotp", () => {
      token: "123456",
    });
    expect(result).toEqual({ enabled: true });
+    expect(ctx.db.user.updateMany).toHaveBeenCalledWith(
+      expect.objectContaining({ data: { lastTotpAt: expect.any(Date) } }),
+    );
    expect(ctx.db.user.update).toHaveBeenCalledWith({
      where: { id: "user_1" },
-      data: { totpEnabled: true, lastTotpAt: expect.any(Date) },
+      data: { totpEnabled: true },
    });
  });

@@ -19,6 +19,24 @@ vi.mock("../lib/logger.js", () => ({
  },
 }));

+// Dispatcher now resolves+validates DNS before opening the HTTPS socket.
+// Mock node:dns/promises so tests do not require real network.
+vi.mock("node:dns/promises", () => ({
+  lookup: vi.fn(async (_hostname: string, _opts?: unknown) => [
+    { address: "93.184.216.34", family: 4 },
+  ]),
+}));
+
+// Mock node:https so we never open a real socket. The dispatcher calls
+// https.request(opts, cb); we return a minimal EventEmitter-like stub.
+const { httpsRequestMock } = vi.hoisted(() => ({
+  httpsRequestMock: vi.fn(),
+}));
+vi.mock("node:https", () => ({
+  Agent: vi.fn(() => ({})),
+  request: httpsRequestMock,
+}));
+
 describe("webhook dispatcher logging", () => {
  beforeEach(() => {
    vi.clearAllMocks();
@@ -82,11 +100,19 @@ describe("webhook dispatcher logging", () => {
  });

  it("treats non-2xx HTTP webhook responses as delivery failures", async () => {
-    const fetchMock = vi.fn().mockResolvedValue({
-      ok: false,
-      status: 500,
-    });
-    vi.stubGlobal("fetch", fetchMock);
+    // Stub https.request to deliver a 500 response synchronously via the
+    // response callback, so the dispatcher sees a non-2xx and logs a warn.
+    httpsRequestMock.mockImplementation(
+      (_opts: unknown, cb: (res: { statusCode: number; resume: () => void }) => void) => {
+        queueMicrotask(() => cb({ statusCode: 500, resume: () => {} }));
+        return {
+          on: vi.fn(),
+          write: vi.fn(),
+          end: vi.fn(),
+          destroy: vi.fn(),
+        };
+      },
+    );

    const db = {
      webhook: {
@@ -117,6 +143,66 @@ describe("webhook dispatcher logging", () => {
      );
    });

-    expect(fetchMock).toHaveBeenCalledTimes(1);
+    expect(httpsRequestMock).toHaveBeenCalledTimes(1);
+    // Verify the pinned IP was passed via the lookup override on the Agent.
+    const firstCall = httpsRequestMock.mock.calls[0]![0] as {
+      host: string;
+      servername: string;
+      agent: { lookup?: unknown };
+    };
+    expect(firstCall.host).toBe("example.com");
+    expect(firstCall.servername).toBe("example.com");
+  });
+
+  it("pins the validated IP via the HTTPS Agent.lookup override (DNS-rebind defence)", async () => {
+    const { Agent } = await import("node:https");
+    const AgentMock = vi.mocked(Agent);
+    AgentMock.mockClear();
+
+    httpsRequestMock.mockImplementation(
+      (_opts: unknown, cb: (res: { statusCode: number; resume: () => void }) => void) => {
+        queueMicrotask(() => cb({ statusCode: 204, resume: () => {} }));
+        return {
+          on: vi.fn(),
+          write: vi.fn(),
+          end: vi.fn(),
+          destroy: vi.fn(),
+        };
+      },
+    );
+
+    const db = {
+      webhook: {
+        findMany: vi.fn().mockResolvedValue([
+          {
+            id: "wh_rebind_1",
+            name: "Pinned Webhook",
+            url: "https://example.com/hook",
+            secret: null,
+            events: ["project.created"],
+          },
+        ]),
+      },
+    };
+
+    dispatchWebhooks(db, "project.created", { id: "p1" });
+
+    await vi.waitFor(() => expect(httpsRequestMock).toHaveBeenCalledTimes(1));
+
+    expect(AgentMock).toHaveBeenCalledTimes(1);
+    const agentOptions = AgentMock.mock.calls[0]![0] as {
+      lookup?: (
+        host: string,
+        opts: unknown,
+        cb: (err: null, addr: string, family: number) => void,
+      ) => void;
+    };
+    expect(typeof agentOptions.lookup).toBe("function");
+
+    // Invoke the lookup override to confirm it returns the pre-validated IP,
+    // NOT whatever DNS might be returning right now.
+    const cb = vi.fn();
+    agentOptions.lookup!("example.com", {}, cb);
+    expect(cb).toHaveBeenCalledWith(null, "93.184.216.34", 4);
  });
 });