fix(security): SSRF sub-resource + fail-closed, webhook 500, trusted rate-limit IP

MANIFEST.md

@@ -7,15 +7,16 @@
 - `app/` - Next.js frontend. Deployed on Vercel; public URL `app-beta-fawn.vercel.app` (intended domain `neuroedge.co.uk` is DOWN — DNS zone broken, see health-check 2026-05-23).
 - `scan-service/` - Node/Fastify engine. Deployed on VPS (openclaw) behind Caddy.
 - `mcp-server/` - Open-source, standalone MCP server (BYO-AI accessibility auditor). Self-contained; no Supabase/LLM deps. Stdio transport.
-- `supabase/` - DB migrations (`001_initial.sql`, `002_*.sql`, `003_lockdown_rls.sql`, `004_reports_unique_session.sql`). `003` applied to live DB 2026-06-10: revokes all anon/authenticated grants + enables RLS (closed the world-writable/PII-readable hole). Anon now has zero table access; all app reads go server-side via service role. `004` NOT yet applied: adds UNIQUE on `reports.stripe_session_id` for webhook idempotency.
+- `supabase/` - DB migrations (`001_initial.sql`, `002_*.sql`, `003_lockdown_rls.sql`, `004_reports_unique_session.sql`). `003` applied to live DB 2026-06-10: revokes all anon/authenticated grants + enables RLS (closed the world-writable/PII-readable hole). Anon now has zero table access; all app reads go server-side via service role. `004` applied to live DB 2026-06-11 (restore→apply→verify→re-pause): adds UNIQUE on `reports.stripe_session_id` (NULLs distinct) for webhook idempotency.
 - `docs/` - Plans, playbooks, audit reports.
 - `brand/`, `concepts/`, `PitchDeck/` - Pitch and brand assets.
 - `video/` - Pitch video Remotion project.
 - `DESIGN-BRIEF.md` - Product design brief.
 
 ### scan-service
 - `scan-service/src/server.ts` - Fastify app. Routes: `POST /api/scan`, `POST /api/generate-report`, `GET /health`. Optional `x-api-key` header gate.
-- `scan-service/src/scanner.ts` - Puppeteer + axe-core runner. Returns score, violations (with sampleNodes), CMS, screenshots.
+- `scan-service/src/scanner.ts` - Puppeteer + axe-core runner. Returns score, violations (with sampleNodes), CMS, screenshots. Delegates per-request SSRF interception to `request-guard.ts`.
+- `scan-service/src/request-guard.ts` - SSRF guard for Puppeteer requests: validates every http(s) request (nav, redirect, AND sub-resource) via DNS-resolving `checkHostSafety`; fails CLOSED; per-scan host cache. Extracted from scanner.ts 2026-06-11.
 - `scan-service/src/industry-detector.ts` - Schema.org + keyword-based industry classification. Word-boundary regex matching (fixed 2026-04-19).
 - `scan-service/src/score.ts` - Accessibility score formula. Pass-ratio 60% + deduction penalty 40% using hyperbolic curve `d / (d + k*R)` (fixed 2026-04-19).
 - `scan-service/src/translator.ts` - LLM plain-English + business-impact translation. Local source is Anthropic-only; VPS runs a multi-provider patched version.
@@ -43,6 +44,10 @@
 - `mcp-server/README.md` - OSS docs: BYO-AI rationale, Claude Desktop config, tool reference, security.
 
 ## Recent Changes
+- 2026-06-11: Created `scan-service/src/request-guard.ts` + `tests/request-guard.test.ts` — extracted SSRF guard from scanner.ts; now validates EVERY http(s) request (sub-resources too, DNS-resolved), fails CLOSED, per-host cache. 7 unit tests. Closes sub-resource SSRF + fail-open holes from the PR #1 review. (DNS-rebind IP-pinning still tracked as a follow-up — needs an integration harness.)
+- 2026-06-11: Created `app/lib/client-ip.ts` (`getClientIp`) — derives client IP from `x-real-ip` / last XFF hop, not the spoofable left-most `x-forwarded-for`; adopted across all 6 rate-limited routes (admin-login, scan, coupon-validate, estimate, regenerate, report-status).
+- 2026-06-11: Updated `app/app/api/webhook/route.ts` — return 500 on non-23505 insert failures so Stripe retries (a 200 silently dropped a *paid* report).
+- 2026-06-11: Applied `supabase/migrations/004_reports_unique_session.sql` to live DB — `reports_stripe_session_id_key UNIQUE (stripe_session_id)` live, old index dropped, table empty; project re-paused.
 - 2026-06-11: Created `app/lib/admin-auth.ts` — HMAC-SHA256 signed token helpers (`issueToken`, `verifyToken`) replacing plaintext-password cookie (C5 fix).
 - 2026-06-11: Updated `app/app/api/admin-login/route.ts` — rate-limited (5/15 min), timing-safe password check, cookie set to signed token.
 - 2026-06-11: Updated `app/app/(admin)/layout.tsx` — verify cookie with `verifyToken()` instead of comparing raw password.

app/app/api/admin-login/route.ts

@@ -1,11 +1,11 @@
 import { NextRequest, NextResponse } from "next/server";
+import { getClientIp } from "@/lib/client-ip";
 import { timingSafeEqual } from "node:crypto";
 import { checkRateLimit } from "@/lib/rate-limit";
 import { issueToken } from "@/lib/admin-auth";
 
 export async function POST(req: NextRequest) {
-  const clientIp =
-    req.headers.get("x-forwarded-for")?.split(",")[0]?.trim() ?? "unknown";
+  const clientIp = getClientIp(req);
   const rateCheck = checkRateLimit(`admin-login:${clientIp}`, 5, 900_000);
   if (!rateCheck.allowed) {
     return NextResponse.json(

app/app/api/coupon-validate/route.ts

@@ -1,4 +1,5 @@
 import { NextRequest, NextResponse } from "next/server";
+import { getClientIp } from "@/lib/client-ip";
 import { createServerClient } from "@/lib/supabase";
 import { checkRateLimit } from "@/lib/rate-limit";
 
@@ -7,8 +8,7 @@ interface ValidateBody {
 }
 
 export async function POST(req: NextRequest) {
-  const clientIp =
-    req.headers.get("x-forwarded-for")?.split(",")[0]?.trim() ?? "unknown";
+  const clientIp = getClientIp(req);
   const rateCheck = checkRateLimit(`coupon-validate:${clientIp}`, 20, 60_000);
   if (!rateCheck.allowed) {
     return NextResponse.json(

app/app/api/estimate/route.ts

@@ -1,4 +1,5 @@
 import { NextRequest, NextResponse } from "next/server";
+import { getClientIp } from "@/lib/client-ip";
 import { createServerClient } from "@/lib/supabase";
 import { calculateRevenueUplift, type RevenueInput } from "@/lib/revenue";
 import { checkRateLimit } from "@/lib/rate-limit";
@@ -29,8 +30,7 @@ function isValidBody(body: unknown): body is EstimateRequestBody {
 }
 
 export async function POST(req: NextRequest) {
-  const clientIp =
-    req.headers.get("x-forwarded-for")?.split(",")[0]?.trim() ?? "unknown";
+  const clientIp = getClientIp(req);
   const rateCheck = checkRateLimit(`estimate:${clientIp}`, 20, 60_000);
   if (!rateCheck.allowed) {
     return NextResponse.json(

app/app/api/regenerate/route.ts

@@ -1,4 +1,5 @@
 import { NextRequest, NextResponse } from "next/server";
+import { getClientIp } from "@/lib/client-ip";
 import { createServerClient } from "@/lib/supabase";
 import { checkRateLimit } from "@/lib/rate-limit";
 
@@ -8,8 +9,7 @@ interface RegenerateRequestBody {
 }
 
 export async function POST(req: NextRequest) {
-  const clientIp =
-    req.headers.get("x-forwarded-for")?.split(",")[0]?.trim() ?? "unknown";
+  const clientIp = getClientIp(req);
   const rateCheck = checkRateLimit(`regenerate:${clientIp}`, 20, 60_000);
   if (!rateCheck.allowed) {
     return NextResponse.json(

app/app/api/report-status/route.ts

@@ -1,4 +1,5 @@
 import { NextResponse } from "next/server";
+import { getClientIp } from "@/lib/client-ip";
 import { createServerClient } from "@/lib/supabase";
 import { checkRateLimit } from "@/lib/rate-limit";
 
@@ -20,8 +21,7 @@ interface ReportStatusBody {
 const REPORT_COLUMNS = "id, scan_id, email, status, sent_at, created_at";
 
 export async function POST(req: Request) {
-  const clientIp =
-    req.headers.get("x-forwarded-for")?.split(",")[0]?.trim() ?? "unknown";
+  const clientIp = getClientIp(req);
   if (!checkRateLimit(`report-status:${clientIp}`, 30, 60_000).allowed) {
     return NextResponse.json(
       { error: "Too many requests. Please wait a moment and try again." },

app/app/api/scan/route.ts

@@ -1,12 +1,13 @@
 import { NextResponse } from "next/server";
+import { getClientIp } from "@/lib/client-ip";
 import { createServerClient } from "@/lib/supabase";
 import { checkRateLimit } from "@/lib/rate-limit";
 
 const SCAN_SERVICE_URL = process.env.SCAN_SERVICE_URL ?? "";
 const SCAN_TIMEOUT_MS = 60_000;
 
 export async function POST(req: Request) {
-  const clientIp = req.headers.get("x-forwarded-for")?.split(",")[0]?.trim() ?? "unknown";
+  const clientIp = getClientIp(req);
   const rateCheck = checkRateLimit(clientIp, 5, 60_000);
   if (!rateCheck.allowed) {
     return NextResponse.json(

app/app/api/webhook/route.ts

@@ -99,8 +99,14 @@ export async function POST(req: NextRequest) {
         return NextResponse.json({ received: true });
       }
       console.error("Webhook: failed to insert report record:", insertError);
-      // Return 200 anyway — Stripe will not retry on 5xx if we've already processed
-      return NextResponse.json({ received: true });
+      // Genuine insert failure (NOT a 23505 idempotency hit) — return 500 so
+      // Stripe retries delivery. The pre-insert lookup + the 23505 guard above
+      // make retries safe (no duplicate report, no double coupon increment), so
+      // a 200 here would silently drop a *paid* report with no recovery.
+      return NextResponse.json(
+        { error: "Failed to persist report" },
+        { status: 500 },
+      );
     }
 
     // Confirmed brand-new insert — increment coupon usage exactly once.

app/lib/client-ip.ts

@@ -0,0 +1,28 @@
+/**
+ * Derive the client IP from a *trusted* source for rate-limiting.
+ *
+ * The LEFT-most `x-forwarded-for` token is the value the client sent and is
+ * trivially spoofable — rotating it defeats any per-IP limit. The trustworthy
+ * signals are `x-real-ip` (set by the edge/proxy to the real peer) and the
+ * RIGHT-most `x-forwarded-for` hop (appended by the closest proxy). Prefer
+ * those; fall back to "unknown".
+ *
+ * NOTE: this hardens key derivation only. The limiter store is still in-process
+ * (per-instance); a shared store (e.g. Upstash/Redis) is still needed for the
+ * limit to hold across serverless instances.
+ */
+export function getClientIp(req: Request): string {
+  const realIp = req.headers.get("x-real-ip")?.trim();
+  if (realIp) return realIp;
+
+  const xff = req.headers.get("x-forwarded-for");
+  if (xff) {
+    const hops = xff
+      .split(",")
+      .map((h) => h.trim())
+      .filter((h) => h.length > 0);
+    if (hops.length > 0) return hops.at(-1) ?? "unknown";
+  }
+
+  return "unknown";
+}

scan-service/src/request-guard.ts

@@ -0,0 +1,50 @@
+import { type HTTPRequest } from 'puppeteer';
+import { checkHostSafety } from './url-validator.js';
+
+/**
+ * Re-validate EVERY http(s) request Chromium makes during a scan — main-frame
+ * navigations, redirect hops, AND sub-resources — against the SSRF guard.
+ * Hostnames are DNS-resolved (via checkHostSafety), so names that resolve to a
+ * private/reserved address are blocked too, not just literal-IP hosts. Verdicts
+ * are cached per scan (one lookup per unique host). Non-network schemes (data:,
+ * blob:, about:) pass through untouched.
+ *
+ * Fails CLOSED: any error in the guard aborts the request rather than letting
+ * it proceed unvalidated.
+ */
+export async function guardRequest(
+  req: HTTPRequest,
+  safeHosts: Map<string, boolean>,
+): Promise<void> {
+  try {
+    const parsed = new URL(req.url());
+
+    // data:, blob:, about:, etc. never hit the network — not SSRF vectors.
+    if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') {
+      await req.continue();
+      return;
+    }
+
+    const host = parsed.hostname.replace(/^\[|\]$/g, '');
+
+    let safe = safeHosts.get(host);
+    if (safe === undefined) {
+      const verdict = await checkHostSafety(host);
+      safe = verdict.valid;
+      safeHosts.set(host, safe);
+    }
+
+    if (!safe) {
+      await req.abort('addressunreachable');
+      return;
+    }
+    await req.continue();
+  } catch {
+    // Fail closed — never continue() an unvalidated request on error.
+    try {
+      await req.abort('addressunreachable');
+    } catch {
+      /* request already handled (e.g. redirect already resolved) */
+    }
+  }
+}

scan-service/src/scanner.ts

@@ -1,7 +1,8 @@
-import puppeteer, { type Browser, type Page, type HTTPRequest } from 'puppeteer';
+import puppeteer, { type Browser } from 'puppeteer';
 import { AxePuppeteer } from '@axe-core/puppeteer';
 import type { AxeResults, NodeResult, TagValue } from 'axe-core';
-import { validateUrlWithDns, checkHostSafety, isPrivateIp } from './url-validator.js';
+import { validateUrlWithDns } from './url-validator.js';
+import { guardRequest } from './request-guard.js';
 import { calculateScore } from './score.js';
 import { detectCMS } from './cms-detector.js';
 import { captureAnnotatedScreenshot } from './screenshot.js';
@@ -37,36 +38,6 @@ export interface ScanResult {
   };
 }
 
-/**
- * Re-validate every main-frame navigation against the SSRF guard, so a public
- * URL that 302-redirects (or DNS-rebinds) to a private address is aborted
- * mid-flight. Literal-IP sub-resources are also cheaply blocked.
- */
-async function guardRequest(page: Page, req: HTTPRequest): Promise<void> {
-  try {
-    const host = new URL(req.url()).hostname;
-    const isMainNavigation = req.isNavigationRequest() && req.frame() === page.mainFrame();
-
-    if (isMainNavigation) {
-      const verdict = await checkHostSafety(host);
-      if (!verdict.valid) {
-        await req.abort('addressunreachable');
-        return;
-      }
-    } else if (host && isPrivateIp(host.replace(/^\[|\]$/g, ''))) {
-      await req.abort('addressunreachable');
-      return;
-    }
-    await req.continue();
-  } catch {
-    try {
-      await req.continue();
-    } catch {
-      /* request already handled */
-    }
-  }
-}
-
 let browser: Browser | null = null;
 
 async function getBrowser(): Promise<Browser> {
@@ -104,8 +75,9 @@ async function scanUrlInternal(url: string): Promise<ScanResult> {
     );
 
     await page.setRequestInterception(true);
+    const safeHosts = new Map<string, boolean>();
     page.on('request', (req) => {
-      void guardRequest(page, req);
+      void guardRequest(req, safeHosts);
     });
 
     await page.goto(validation.url, { waitUntil: 'networkidle2', timeout: 30_000 });

scan-service/tests/request-guard.test.ts

@@ -0,0 +1,63 @@
+import { describe, it, expect, vi } from 'vitest';
+import type { HTTPRequest } from 'puppeteer';
+import { guardRequest } from '../src/request-guard.js';
+
+/** Minimal fake of Puppeteer's HTTPRequest exposing url()/abort()/continue(). */
+function makeReq(url: string) {
+  const abort = vi.fn(async () => {});
+  const cont = vi.fn(async () => {});
+  const req = { url: () => url, abort, continue: cont } as unknown as HTTPRequest;
+  return { req, abort, cont };
+}
+
+describe('guardRequest — SSRF enforcement on every request', () => {
+  it('aborts a literal private IP (loopback)', async () => {
+    const { req, abort, cont } = makeReq('http://127.0.0.1/');
+    await guardRequest(req, new Map());
+    expect(abort).toHaveBeenCalledOnce();
+    expect(cont).not.toHaveBeenCalled();
+  });
+
+  it('aborts the cloud-metadata IP', async () => {
+    const { req, abort } = makeReq('http://169.254.169.254/latest/meta-data/');
+    await guardRequest(req, new Map());
+    expect(abort).toHaveBeenCalledOnce();
+  });
+
+  it('aborts localhost', async () => {
+    const { req, abort } = makeReq('http://localhost:8080/');
+    await guardRequest(req, new Map());
+    expect(abort).toHaveBeenCalledOnce();
+  });
+
+  it('allows a public literal IP', async () => {
+    const { req, cont, abort } = makeReq('http://8.8.8.8/');
+    await guardRequest(req, new Map());
+    expect(cont).toHaveBeenCalledOnce();
+    expect(abort).not.toHaveBeenCalled();
+  });
+
+  it('passes through non-network schemes (data:)', async () => {
+    const { req, cont, abort } = makeReq('data:text/html,<p>hi</p>');
+    await guardRequest(req, new Map());
+    expect(cont).toHaveBeenCalledOnce();
+    expect(abort).not.toHaveBeenCalled();
+  });
+
+  it('fails CLOSED on a malformed URL (aborts, never continues)', async () => {
+    const { req, abort, cont } = makeReq('not-a-valid-url');
+    await guardRequest(req, new Map());
+    expect(abort).toHaveBeenCalledOnce();
+    expect(cont).not.toHaveBeenCalled();
+  });
+
+  it('caches the verdict per host', async () => {
+    const safeHosts = new Map<string, boolean>();
+    const a = makeReq('http://8.8.8.8/a');
+    await guardRequest(a.req, safeHosts);
+    expect(safeHosts.get('8.8.8.8')).toBe(true);
+    const b = makeReq('http://127.0.0.1/');
+    await guardRequest(b.req, safeHosts);
+    expect(safeHosts.get('127.0.0.1')).toBe(false);
+  });
+});