security: bound attacker-controlled scan fan-out and rate-limit write race (#539)

* security: bound attacker-controlled scan fan-out and rate-limit write race

Hardens three DoS / race-condition findings surfaced by a deep security
audit and tracked as private draft advisories. All three are reachable by
an unauthenticated request and turn one cheap, rate-limited request into
disproportionate backend work.

- DKIM selector fan-out (GHSA-6fqp-4vhc-59mf): parseSelectors and the MCP
  parseSelectorsFromArray now cap the custom selector list to 16 items and
  63 chars each (RFC 1035 label limit) before analyzeDkim fires one DNS
  lookup per selector. The MCP inputSchema advertises maxItems/maxLength to
  match, but enforcement is server-side regardless (the schema is advisory).

- MTA-STS unbounded body (GHSA-p676-gc7j-96mx): fetchPolicy now reads the
  policy via arrayBuffer() truncated to MAX_POLICY_BYTES (64KB), mirroring
  the ceiling security-txt already applies, so an attacker-controlled
  mta-sts.<domain> can't stream an unbounded body into the isolate. The
  redirect:"manual" posture (RFC 8461 §3.3) is unchanged.

- Rate-limit counter write (GHSA-v7qc-7qh8-h69g): the Cache-API write is
  awaited before returning instead of deferred via waitUntil, closing the
  intra-isolate read-modify-write window the deferral widened. A fully
  atomic cross-isolate counter (Durable Object / native binding) remains
  tracked in the advisory and is out of scope here.

Also gitignores .security-scans/ so point-in-time scan reports are never
committed to this public repo.

Tests: 1305 passing, 0 failing. Typecheck + biome lint clean. Adds
regression tests for each cap and for the awaited rate-limit write.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: schmug <38227427+schmug@users.noreply.github.com>

* security: scope this PR to the per-analyzer DoS caps; drop the rate-limit write-race fix (superseded by #549)

The Cache-API await fix here (GHSA-v7qc-7qh8-h69g) is only a partial mitigation; #549 fully supersedes it with an atomic Durable Object rate limiter and deletes the checkRateLimitCache path this touched. Revert the rate-limit.ts change, the rateLimitMiddleware hunk in index.ts, and the f30 test so this PR scopes cleanly to the per-analyzer caps that nothing else duplicates: DKIM selector count/length (GHSA-6fqp-4vhc-59mf) and MTA-STS policy body size (GHSA-p676-gc7j-96mx). Avoids a same-line conflict with #549's middleware rewrite.

Signed-off-by: schmug <38227427+schmug@users.noreply.github.com>

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Signed-off-by: schmug <38227427+schmug@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: schmug

.gitignore

@@ -20,6 +20,7 @@ TRIAGE.json
 TRIAGE.md
 .triage-state/
 .threat-model-state/
+.security-scans/
 
 # CycloneDX SBOM is generated in CI per release and attached as a release
 # asset — never committed (release.yml builds it from the lockfile).

src/analyzers/mta-sts.ts

@@ -2,6 +2,12 @@ import { queryTxt } from "../dns/client.js";
 import type { ScanBudget } from "../dns/scan-budget.js";
 import type { MtaStsPolicy, MtaStsResult, Validation } from "./types.js";
 
+// Cap the fetched policy body before decoding it (GHSA-p676-gc7j-96mx). RFC 8461
+// policies are tiny, but an attacker who controls mta-sts.<domain> could
+// otherwise stream an unbounded body into the isolate. This mirrors the
+// MAX_BODY_BYTES ceiling that src/analyzers/security-txt.ts already applies.
+export const MAX_POLICY_BYTES = 64 * 1024;
+
 export async function analyzeMtaSts(
   domain: string,
   budget?: ScanBudget,
@@ -119,7 +125,18 @@ async function fetchPolicy(domain: string): Promise<MtaStsPolicy | null> {
     if ((resp.type as string) === "opaqueredirect") return null;
     if (!resp.ok) return null;
 
-    const text = await resp.text();
+    // Bound the body before decoding — reading via arrayBuffer()+slice caps
+    // memory regardless of a lying Content-Length or a slow infinite stream
+    // (the 3s AbortSignal bounds time, not bytes). Same control as security-txt.
+    const buffer = await resp.arrayBuffer();
+    const slice =
+      buffer.byteLength > MAX_POLICY_BYTES
+        ? buffer.slice(0, MAX_POLICY_BYTES)
+        : buffer;
+    const text = new TextDecoder("utf-8", {
+      fatal: false,
+      ignoreBOM: false,
+    }).decode(slice);
     return parsePolicy(text);
   } catch {
     return null;

src/index.ts

@@ -1453,12 +1453,27 @@ export { normalizeDomain };
 // is dropped silently — an invalid selector cannot match a real DKIM key.
 const VALID_SELECTOR = /^[A-Za-z0-9._-]+$/;
 
+// DoS guard (GHSA-6fqp-4vhc-59mf): every custom selector becomes one concurrent
+// DNS lookup in analyzeDkim, so an unbounded attacker-supplied list is a DNS
+// amplification vector charged against a single rate-limit token. Bound both
+// the per-item length (RFC 1035 label limit) and the count. 16 custom selectors
+// is generous given ~37 built-in COMMON_SELECTORS. parseSelectorsFromArray in
+// src/mcp/handler.ts mirrors these limits for the MCP path.
+export const MAX_SELECTOR_LENGTH = 63;
+export const MAX_SELECTORS = 16;
+
 export function parseSelectors(raw: string | undefined): string[] {
   if (!raw) return [];
   return raw
     .split(",")
     .map((s) => s.trim())
-    .filter((s) => s.length > 0 && VALID_SELECTOR.test(s));
+    .filter(
+      (s) =>
+        s.length > 0 &&
+        s.length <= MAX_SELECTOR_LENGTH &&
+        VALID_SELECTOR.test(s),
+    )
+    .slice(0, MAX_SELECTORS);
 }
 
 // Cron handler — runs nightly per the `[triggers] crons` entry in wrangler.toml.

src/mcp/handler.ts

@@ -11,8 +11,22 @@ import type { ScoringConfig } from "../shared/scoring.js";
 // DKIM selector charset per RFC 6376 §3.1 — mirrors VALID_SELECTOR in index.ts.
 const VALID_SELECTOR = /^[A-Za-z0-9._-]+$/;
 
-function parseSelectorsFromArray(raw: string[]): string[] {
-  return raw.filter((s) => s.length > 0 && VALID_SELECTOR.test(s));
+// Mirrors MAX_SELECTOR_LENGTH / MAX_SELECTORS in src/index.ts
+// (GHSA-6fqp-4vhc-59mf). Enforced server-side regardless of the inputSchema
+// maxItems/maxLength below, which are advisory only — handleToolCall never
+// validates arguments against the schema.
+const MAX_SELECTOR_LENGTH = 63;
+const MAX_SELECTORS = 16;
+
+export function parseSelectorsFromArray(raw: string[]): string[] {
+  return raw
+    .filter(
+      (s) =>
+        s.length > 0 &&
+        s.length <= MAX_SELECTOR_LENGTH &&
+        VALID_SELECTOR.test(s),
+    )
+    .slice(0, MAX_SELECTORS);
 }
 
 export const MCP_PROTOCOL_VERSION = "2025-03-26";
@@ -46,7 +60,12 @@ const SCAN_DOMAIN_TOOL = {
       },
       dkim_selectors: {
         type: "array",
-        items: { type: "string", pattern: "^[A-Za-z0-9._-]+$" },
+        maxItems: MAX_SELECTORS,
+        items: {
+          type: "string",
+          pattern: "^[A-Za-z0-9._-]+$",
+          maxLength: MAX_SELECTOR_LENGTH,
+        },
         description:
           "Extra DKIM selectors to probe beyond the built-in defaults.",
       },

test/index.test.ts

@@ -1,6 +1,12 @@
 import { beforeEach, describe, expect, it, vi } from "vitest";
 import { COMMON_SELECTORS } from "../src/analyzers/dkim.js";
-import { app, normalizeDomain, parseSelectors } from "../src/index.js";
+import {
+  app,
+  MAX_SELECTOR_LENGTH,
+  MAX_SELECTORS,
+  normalizeDomain,
+  parseSelectors,
+} from "../src/index.js";
 import { _memoryStore } from "../src/rate-limit.js";
 import { LEARN_SIBLINGS } from "../src/views/learn.js";
 
@@ -124,6 +130,21 @@ describe("parseSelectors", () => {
   it("returns single selector", () => {
     expect(parseSelectors("google")).toEqual(["google"]);
   });
+
+  // f27 / GHSA-6fqp-4vhc-59mf — attacker-controlled selector lists must not
+  // fan out into one DNS lookup each with no ceiling.
+  it("caps the number of selectors at MAX_SELECTORS", () => {
+    const many = Array.from({ length: 500 }, (_, i) => `s${i}`).join(",");
+    const result = parseSelectors(many);
+    expect(result.length).toBe(MAX_SELECTORS);
+    expect(result[0]).toBe("s0"); // keeps the first N, deterministically
+  });
+
+  it("drops selectors longer than MAX_SELECTOR_LENGTH", () => {
+    const tooLong = "a".repeat(MAX_SELECTOR_LENGTH + 1);
+    const ok = "b".repeat(MAX_SELECTOR_LENGTH);
+    expect(parseSelectors(`${tooLong},${ok},google`)).toEqual([ok, "google"]);
+  });
 });
 
 describe("normalizeDomain — extended edge cases", () => {

test/mcp.test.ts

@@ -1,4 +1,5 @@
 import { describe, expect, it, vi } from "vitest";
+import { MAX_SELECTORS } from "../src/index.js";
 import {
   handleMcpRequest,
   MCP_PROTOCOL_VERSION,
@@ -214,6 +215,26 @@ describe("handleMcpRequest — tools/call", () => {
     expect(vi.mocked(scan)).toHaveBeenCalled();
   });
 
+  // f27 / GHSA-6fqp-4vhc-59mf — the MCP path must enforce the selector count
+  // cap server-side (the inputSchema maxItems is advisory only).
+  it("caps dkim_selectors at MAX_SELECTORS before calling scan", async () => {
+    const { scan } = await import("../src/orchestrator.js");
+    vi.mocked(scan).mockClear();
+    const many = Array.from({ length: 500 }, (_, i) => `s${i}`);
+    const { json } = await rpc({
+      jsonrpc: "2.0",
+      id: 71,
+      method: "tools/call",
+      params: {
+        name: "scan_domain",
+        arguments: { domain: "example.com", dkim_selectors: many },
+      },
+    });
+    expect((json.result as { isError: boolean }).isError).toBe(false);
+    const selectorsArg = vi.mocked(scan).mock.calls[0]?.[1] as string[];
+    expect(selectorsArg.length).toBe(MAX_SELECTORS);
+  });
+
   it("preserves JSON-RPC id across the full call", async () => {
     const { json } = await rpc({
       jsonrpc: "2.0",

test/mta-sts.test.ts

@@ -5,7 +5,7 @@ vi.mock("../src/dns/client.js", () => ({
   queryMx: vi.fn(),
 }));
 
-import { analyzeMtaSts } from "../src/analyzers/mta-sts.js";
+import { analyzeMtaSts, MAX_POLICY_BYTES } from "../src/analyzers/mta-sts.js";
 import { queryTxt } from "../src/dns/client.js";
 
 const mockQueryTxt = vi.mocked(queryTxt);
@@ -15,11 +15,22 @@ beforeEach(() => {
   vi.restoreAllMocks();
 });
 
+// fetchPolicy reads the body via resp.arrayBuffer() (size-capped, mirroring
+// security-txt) — so the mock must expose arrayBuffer, not just text.
 function mockFetchPolicy(body: string | null, ok = true) {
   vi.spyOn(globalThis, "fetch").mockResolvedValue(
     body === null
-      ? ({ ok: false, text: async () => "" } as Response)
-      : ({ ok, text: async () => body } as Response),
+      ? ({
+          ok: false,
+          text: async () => "",
+          arrayBuffer: async () => new ArrayBuffer(0),
+        } as Response)
+      : ({
+          ok,
+          text: async () => body,
+          arrayBuffer: async () =>
+            new TextEncoder().encode(body).buffer as ArrayBuffer,
+        } as Response),
   );
 }
 
@@ -57,6 +68,28 @@ describe("analyzeMtaSts", () => {
     ).toBe(true);
   });
 
+  // f29 / GHSA-p676-gc7j-96mx — an attacker who controls mta-sts.<domain> must
+  // not be able to stream an unbounded body into memory. The fetch body is
+  // capped at MAX_POLICY_BYTES; anything past the cap is never parsed.
+  it("caps the policy body and ignores content beyond MAX_POLICY_BYTES", async () => {
+    mockQueryTxt.mockResolvedValue({
+      entries: ["v=STSv1; id=20240101"],
+      raw: "v=STSv1; id=20240101",
+    });
+    // Valid directives in the first <64KB; a sentinel mx line pushed past the
+    // cap by a giant colon-less (therefore ignored-by-parser) filler line.
+    const prefix =
+      "version: STSv1\nmode: enforce\nmx: legit.example.com\nmax_age: 86400\n";
+    const filler = `${"#".repeat(MAX_POLICY_BYTES)}\n`;
+    const sentinel = "mx: sneaky.evil.example\n";
+    mockFetchPolicy(prefix + filler + sentinel);
+
+    const result = await analyzeMtaSts("example.com");
+    expect(result.policy?.mx).toContain("legit.example.com");
+    // The sentinel sits beyond the byte cap, so it must never reach the parser.
+    expect(result.policy?.mx).not.toContain("sneaky.evil.example");
+  });
+
   it("passes when DNS record found with v=STSv1", async () => {
     mockQueryTxt.mockResolvedValue({
       entries: ["v=STSv1; id=20240101"],