From ea037b52c5880a87f5f03c781f56bd247a669bee Mon Sep 17 00:00:00 2001 From: vedantggwp <138197457+vedantggwp@users.noreply.github.com> Date: Thu, 11 Jun 2026 16:20:34 +0100 Subject: [PATCH] fix(security): SSRF sub-resource + fail-closed, webhook 500, trusted rate-limit IP Post-merge follow-ups from the PR #1 review (3 sub-agents + verification). scan-service SSRF (scanner.ts -> request-guard.ts): - Validate EVERY http(s) request -- main nav, redirect hops, AND sub-resources -- via DNS-resolving checkHostSafety, not just literal-IP hosts. Closes the hostname sub-resource SSRF hole (e.g. an to a name resolving to a private/metadata IP). - Fail CLOSED: guard errors now abort the request (was req.continue()). - Pass through non-network schemes (data:/blob:). Per-scan host cache. - Extracted to request-guard.ts; 7 offline unit tests. webhook (route.ts): - Return 500 on non-23505 insert failures so Stripe retries; a 200 silently dropped a paid report. Pre-insert lookup + 23505 guard make retries safe. rate-limit (client-ip.ts + all 6 routes): - Derive client IP from x-real-ip / last XFF hop, not the spoofable left-most x-forwarded-for token. Verified: scan-service tsc clean + 59/59 tests; app tsc --noEmit clean. DNS-rebind IP-pinning + coupon max_uses atomicity tracked as follow-ups. --- MANIFEST.md | 9 +++- app/app/api/admin-login/route.ts | 4 +- app/app/api/coupon-validate/route.ts | 4 +- app/app/api/estimate/route.ts | 4 +- app/app/api/regenerate/route.ts | 4 +- app/app/api/report-status/route.ts | 4 +- app/app/api/scan/route.ts | 3 +- app/app/api/webhook/route.ts | 10 +++- app/lib/client-ip.ts | 28 +++++++++++ scan-service/src/request-guard.ts | 50 +++++++++++++++++++ scan-service/src/scanner.ts | 38 ++------------ scan-service/tests/request-guard.test.ts | 63 ++++++++++++++++++++++++ 12 files changed, 173 insertions(+), 48 deletions(-) create mode 100644 app/lib/client-ip.ts create mode 100644 scan-service/src/request-guard.ts create mode 100644 scan-service/tests/request-guard.test.ts diff --git a/MANIFEST.md b/MANIFEST.md index 713b42d..96143df 100644 --- a/MANIFEST.md +++ b/MANIFEST.md @@ -7,7 +7,7 @@ - `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. @@ -15,7 +15,8 @@ ### 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. diff --git a/app/app/api/admin-login/route.ts b/app/app/api/admin-login/route.ts index 14e276c..68a6f89 100644 --- a/app/app/api/admin-login/route.ts +++ b/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( diff --git a/app/app/api/coupon-validate/route.ts b/app/app/api/coupon-validate/route.ts index 1f6e897..727bbcc 100644 --- a/app/app/api/coupon-validate/route.ts +++ b/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( diff --git a/app/app/api/estimate/route.ts b/app/app/api/estimate/route.ts index 7fdb4d8..107d546 100644 --- a/app/app/api/estimate/route.ts +++ b/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( diff --git a/app/app/api/regenerate/route.ts b/app/app/api/regenerate/route.ts index 00dd8bb..da78363 100644 --- a/app/app/api/regenerate/route.ts +++ b/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( diff --git a/app/app/api/report-status/route.ts b/app/app/api/report-status/route.ts index 84bd5a9..b3e11b5 100644 --- a/app/app/api/report-status/route.ts +++ b/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." }, diff --git a/app/app/api/scan/route.ts b/app/app/api/scan/route.ts index 807296c..51ca138 100644 --- a/app/app/api/scan/route.ts +++ b/app/app/api/scan/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"; @@ -6,7 +7,7 @@ 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( diff --git a/app/app/api/webhook/route.ts b/app/app/api/webhook/route.ts index 2e672a1..9cd3f68 100644 --- a/app/app/api/webhook/route.ts +++ b/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. diff --git a/app/lib/client-ip.ts b/app/lib/client-ip.ts new file mode 100644 index 0000000..63cfc25 --- /dev/null +++ b/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"; +} diff --git a/scan-service/src/request-guard.ts b/scan-service/src/request-guard.ts new file mode 100644 index 0000000..02c9196 --- /dev/null +++ b/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, +): Promise { + 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) */ + } + } +} diff --git a/scan-service/src/scanner.ts b/scan-service/src/scanner.ts index 80583c4..648f98e 100644 --- a/scan-service/src/scanner.ts +++ b/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 { - 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 { @@ -104,8 +75,9 @@ async function scanUrlInternal(url: string): Promise { ); await page.setRequestInterception(true); + const safeHosts = new Map(); page.on('request', (req) => { - void guardRequest(page, req); + void guardRequest(req, safeHosts); }); await page.goto(validation.url, { waitUntil: 'networkidle2', timeout: 30_000 }); diff --git a/scan-service/tests/request-guard.test.ts b/scan-service/tests/request-guard.test.ts new file mode 100644 index 0000000..fc7093f --- /dev/null +++ b/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,

hi

'); + 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(); + 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); + }); +});