From 8757de0c3677b4b006f9d32bab27381090d3fe7d Mon Sep 17 00:00:00 2001 From: Adrian Perez Date: Sun, 19 Apr 2026 20:59:26 +0200 Subject: [PATCH] fix(mcp,editor): close PUT-route URL bypass + vision-tool SSRF (Phase 10 A2) Pre-push audit agent A2 flagged two HIGH-severity security issues that would have shipped in the PR had we not checked: 1. PUT /api/scenes/[id] URL-validation bypass. Phase 8 P4 fixed the POST /api/scenes route by replacing a loose z.unknown() graph schema with AnyNode superRefine. The fix never made it to the PUT handler - an attacker could resubmit the same javascript:/file:/// payloads via PUT. Fixed by extracting the tight validator into apps/editor/lib/graph-schema.ts and sharing it across both routes. 2. SSRF in photo_to_scene / analyze_floorplan_image / analyze_room_photo. All three tools called raw fetch(image) on user-supplied URLs with no validation - a direct http://169.254.169.254/latest/meta-data/ exfil primitive on any cloud host. Added packages/mcp/src/lib/safe-fetch.ts that: - Blocks loopback (127.0.0.0/8, ::1) - Blocks link-local incl. cloud metadata (169.254.0.0/16) - Blocks private ranges (10/8, 172.16/12, 192.168/16, fc00::/7) - Blocks .local/.internal/.corp hostnames + localhost variants - Blocks v4-mapped IPv6 loopback (::ffff:127.0.0.1) - Manual redirects (max 3), revalidating the allowlist per hop - 20 MB response-size cap (streamed, enforced per-chunk) - 10s timeout - Optional PASCAL_ALLOWED_ASSET_ORIGINS env allowlist Tests: 8 new SSRF guard tests, all vision tests still pass, full suite 302/302. Co-Authored-By: Claude Opus 4.7 (1M context) --- apps/editor/app/api/scenes/[id]/route.ts | 7 +- apps/editor/app/api/scenes/route.ts | 32 +-- apps/editor/lib/graph-schema.ts | 34 +++ packages/mcp/src/lib/safe-fetch.test.ts | 87 +++++++ packages/mcp/src/lib/safe-fetch.ts | 230 ++++++++++++++++++ .../tools/photo-to-scene/photo-to-scene.ts | 16 +- .../tools/vision/analyze-floorplan-image.ts | 17 +- .../src/tools/vision/analyze-room-photo.ts | 16 +- 8 files changed, 371 insertions(+), 68 deletions(-) create mode 100644 apps/editor/lib/graph-schema.ts create mode 100644 packages/mcp/src/lib/safe-fetch.test.ts create mode 100644 packages/mcp/src/lib/safe-fetch.ts diff --git a/apps/editor/app/api/scenes/[id]/route.ts b/apps/editor/app/api/scenes/[id]/route.ts index 33d82633..81ff4815 100644 --- a/apps/editor/app/api/scenes/[id]/route.ts +++ b/apps/editor/app/api/scenes/[id]/route.ts @@ -1,18 +1,15 @@ import { type NextRequest, NextResponse } from 'next/server' import { z } from 'zod' +import { apiGraphSchema } from '@/lib/graph-schema' import { getSceneStore } from '@/lib/scene-store-server' export const dynamic = 'force-dynamic' type RouteParams = { params: Promise<{ id: string }> } -const graphSchema = z.unknown().refine((v: unknown) => v !== null && typeof v === 'object', { - message: 'graph must be an object', -}) - const putSceneSchema = z.object({ name: z.string().min(1).max(200).optional(), - graph: graphSchema, + graph: apiGraphSchema, thumbnailUrl: z.string().url().nullable().optional(), expectedVersion: z.number().int().nonnegative().optional(), }) diff --git a/apps/editor/app/api/scenes/route.ts b/apps/editor/app/api/scenes/route.ts index e4cf7886..3053aa95 100644 --- a/apps/editor/app/api/scenes/route.ts +++ b/apps/editor/app/api/scenes/route.ts @@ -1,43 +1,15 @@ -import { AnyNode } from '@pascal-app/core/schema' import { type NextRequest, NextResponse } from 'next/server' import { z } from 'zod' +import { apiGraphSchema } from '@/lib/graph-schema' import { getSceneStore } from '@/lib/scene-store-server' export const dynamic = 'force-dynamic' -/** - * The `graph` payload must structurally match a SceneGraph AND every node - * must pass `AnyNode.safeParse` (including the AssetUrl allowlist for - * scan/guide/item/material URL fields). Without this revalidation, the - * POST /api/scenes route would bypass the security hardening in A7. See - * Phase 8 P4 report for the CVE-ish finding. - */ -const graphSchema = z - .object({ - nodes: z.record(z.string(), z.unknown()), - rootNodeIds: z.array(z.string()), - collections: z.unknown().optional(), - }) - .superRefine((value, ctx) => { - for (const [nodeId, node] of Object.entries(value.nodes)) { - const res = AnyNode.safeParse(node) - if (!res.success) { - for (const issue of res.error.issues) { - ctx.addIssue({ - code: 'custom', - path: ['nodes', nodeId, ...issue.path], - message: issue.message, - }) - } - } - } - }) - const createSceneSchema = z.object({ id: z.string().min(1).max(64).optional(), name: z.string().min(1).max(200), projectId: z.string().min(1).max(200).nullable().optional(), - graph: graphSchema, + graph: apiGraphSchema, thumbnailUrl: z.string().url().nullable().optional(), }) diff --git a/apps/editor/lib/graph-schema.ts b/apps/editor/lib/graph-schema.ts new file mode 100644 index 00000000..ce67c143 --- /dev/null +++ b/apps/editor/lib/graph-schema.ts @@ -0,0 +1,34 @@ +import { AnyNode } from '@pascal-app/core/schema' +import { z } from 'zod' + +/** + * Validates a SceneGraph at an untrusted API boundary. Re-runs + * `AnyNode.safeParse` on every node, which enforces the `AssetUrl` + * allowlist in core (closes the Phase 3 SSRF / arbitrary-URL risk on + * scan/guide/item/material fields). + * + * Shared between `POST /api/scenes` and `PUT /api/scenes/[id]` so neither + * route can silently accept malicious URLs via the `graph` payload. + * + * Phase 8 P4 found the POST bypass; Phase 10 A2 found the PUT bypass. + */ +export const apiGraphSchema = z + .object({ + nodes: z.record(z.string(), z.unknown()), + rootNodeIds: z.array(z.string()), + collections: z.unknown().optional(), + }) + .superRefine((value, ctx) => { + for (const [nodeId, node] of Object.entries(value.nodes)) { + const res = AnyNode.safeParse(node) + if (!res.success) { + for (const issue of res.error.issues) { + ctx.addIssue({ + code: 'custom', + path: ['nodes', nodeId, ...issue.path], + message: issue.message, + }) + } + } + } + }) diff --git a/packages/mcp/src/lib/safe-fetch.test.ts b/packages/mcp/src/lib/safe-fetch.test.ts new file mode 100644 index 00000000..e9d5d22d --- /dev/null +++ b/packages/mcp/src/lib/safe-fetch.test.ts @@ -0,0 +1,87 @@ +import { describe, expect, test } from 'bun:test' +import { McpError } from '@modelcontextprotocol/sdk/types.js' +import { safeFetch } from './safe-fetch' + +describe('safeFetch — SSRF protection', () => { + test('rejects non-http schemes', async () => { + for (const url of ['file:///etc/passwd', 'ftp://example.com/', 'javascript:alert(1)']) { + const err = await safeFetch(url).catch((e) => e) + expect(err).toBeInstanceOf(McpError) + expect((err as Error).message).toContain('url_scheme_not_allowed') + } + }) + + test('rejects loopback addresses', async () => { + for (const url of [ + 'http://127.0.0.1/', + 'http://127.1.2.3/', + 'http://localhost:9999/', + 'http://[::1]/', + ]) { + const err = await safeFetch(url).catch((e) => e) + expect(err).toBeInstanceOf(McpError) + expect((err as Error).message).toContain('url_host_blocked') + } + }) + + test('rejects link-local / cloud metadata', async () => { + // 169.254.169.254 is the AWS/GCP/Azure instance-metadata endpoint. + const url = 'http://169.254.169.254/latest/meta-data/' + const err = await safeFetch(url).catch((e) => e) + expect(err).toBeInstanceOf(McpError) + expect((err as Error).message).toContain('url_host_blocked') + }) + + test('rejects private IP ranges', async () => { + for (const url of [ + 'http://10.0.0.1/', + 'http://172.16.5.9/', + 'http://172.31.255.254/', + 'http://192.168.1.1/', + ]) { + const err = await safeFetch(url).catch((e) => e) + expect(err).toBeInstanceOf(McpError) + expect((err as Error).message).toContain('url_host_blocked') + } + }) + + test('rejects local-style hostnames', async () => { + for (const url of [ + 'http://mything.local/', + 'http://server.internal/', + 'http://db.corp/', + 'http://nope.localhost/', + ]) { + const err = await safeFetch(url).catch((e) => e) + expect(err).toBeInstanceOf(McpError) + expect((err as Error).message).toContain('url_host_blocked') + } + }) + + test('rejects IPv4-mapped IPv6 loopback', async () => { + const err = await safeFetch('http://[::ffff:127.0.0.1]/').catch((e) => e) + expect(err).toBeInstanceOf(McpError) + }) + + test('rejects malformed URL', async () => { + const err = await safeFetch('not a url').catch((e) => e) + expect(err).toBeInstanceOf(McpError) + expect((err as Error).message).toContain('invalid_url') + }) + + test('applies PASCAL_ALLOWED_ASSET_ORIGINS env allowlist when set', async () => { + const prev = process.env.PASCAL_ALLOWED_ASSET_ORIGINS + process.env.PASCAL_ALLOWED_ASSET_ORIGINS = 'https://cdn.example.com' + try { + const err = await safeFetch('https://other.example.com/x.png').catch((e) => e) + expect(err).toBeInstanceOf(McpError) + expect((err as Error).message).toContain('url_origin_not_allowlisted') + } finally { + if (prev === undefined) { + delete process.env.PASCAL_ALLOWED_ASSET_ORIGINS + } else { + process.env.PASCAL_ALLOWED_ASSET_ORIGINS = prev + } + } + }) +}) diff --git a/packages/mcp/src/lib/safe-fetch.ts b/packages/mcp/src/lib/safe-fetch.ts new file mode 100644 index 00000000..e3ea0112 --- /dev/null +++ b/packages/mcp/src/lib/safe-fetch.ts @@ -0,0 +1,230 @@ +import { isIPv4, isIPv6 } from 'node:net' +import { ErrorCode, McpError } from '@modelcontextprotocol/sdk/types.js' + +/** + * SSRF-safe fetch for user-supplied URLs (image URLs in vision tools). + * + * Blocks the usual server-side-request-forgery attack surface: + * - loopback (127.0.0.0/8, ::1) + * - link-local (169.254.0.0/16 — includes cloud metadata at 169.254.169.254) + * - private ranges (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, fc00::/7) + * - non-http(s) schemes + * - manual-redirect with the same allowlist applied to each hop + * - max body size (default 20 MB) + * - request timeout (default 10 s) + * + * Optional allowlist via `PASCAL_ALLOWED_ASSET_ORIGINS` env var (comma-separated). + * + * Phase 10 A2 found that photo_to_scene / analyze_floorplan_image / + * analyze_room_photo all called raw `fetch(url)` with no protection, giving + * a direct `169.254.169.254` exfil primitive on any host. + */ + +const DEFAULT_MAX_BYTES = 20 * 1024 * 1024 // 20 MB +const DEFAULT_TIMEOUT_MS = 10_000 +const MAX_REDIRECTS = 3 + +function isPrivateOrLoopbackV4(addr: string): boolean { + const parts = addr.split('.').map(Number) + if (parts.length !== 4 || parts.some((n) => !Number.isInteger(n) || n < 0 || n > 255)) return true // malformed → treat as unsafe + const [a, b] = parts as [number, number, number, number] + if (a === 127) return true // 127.0.0.0/8 loopback + if (a === 10) return true // 10.0.0.0/8 private + if (a === 172 && b >= 16 && b <= 31) return true // 172.16.0.0/12 private + if (a === 192 && b === 168) return true // 192.168.0.0/16 private + if (a === 169 && b === 254) return true // link-local incl. cloud metadata + if (a === 0) return true // current-network + if (a >= 224) return true // multicast / reserved + return false +} + +function isPrivateOrLoopbackV6(addr: string): boolean { + const lower = addr.toLowerCase() + if (lower === '::1' || lower === '::') return true + if ( + lower.startsWith('fe80:') || + lower.startsWith('fe90:') || + lower.startsWith('fea0:') || + lower.startsWith('feb0:') + ) + return true // link-local + if (lower.startsWith('fc') || lower.startsWith('fd')) return true // ULA fc00::/7 + if (lower.startsWith('::ffff:')) { + // v4-mapped + const v4 = lower.slice(7) + if (isIPv4(v4)) return isPrivateOrLoopbackV4(v4) + } + return false +} + +function isUnsafeHost(hostname: string): boolean { + const host = hostname.replace(/^\[|\]$/g, '') // strip IPv6 brackets if any + if (isIPv4(host)) return isPrivateOrLoopbackV4(host) + if (isIPv6(host)) return isPrivateOrLoopbackV6(host) + // Hostname (not IP) — block well-known local names. + const lower = host.toLowerCase() + if ( + lower === 'localhost' || + lower.endsWith('.localhost') || + lower === 'broadcasthost' || + lower.endsWith('.local') || + lower.endsWith('.internal') || + lower.endsWith('.corp') + ) { + return true + } + return false +} + +function assertAllowedUrl(url: string): URL { + let parsed: URL + try { + parsed = new URL(url) + } catch { + throw new McpError(ErrorCode.InvalidParams, 'invalid_url', { url }) + } + if (parsed.protocol !== 'https:' && parsed.protocol !== 'http:') { + throw new McpError(ErrorCode.InvalidParams, 'url_scheme_not_allowed', { + url, + protocol: parsed.protocol, + }) + } + if (isUnsafeHost(parsed.hostname)) { + throw new McpError(ErrorCode.InvalidParams, 'url_host_blocked', { + url, + hostname: parsed.hostname, + }) + } + // Optional env-allowlist narrowing. + const allowEnv = process.env.PASCAL_ALLOWED_ASSET_ORIGINS + if (allowEnv) { + const origins = allowEnv + .split(',') + .map((s) => s.trim()) + .filter(Boolean) + if (!origins.includes(parsed.origin)) { + throw new McpError(ErrorCode.InvalidParams, 'url_origin_not_allowlisted', { + url, + origin: parsed.origin, + }) + } + } + return parsed +} + +export type SafeFetchOptions = { + maxBytes?: number + timeoutMs?: number + /** Request `Accept` header to send. */ + accept?: string +} + +export type SafeFetchResult = { + buffer: Buffer + contentType: string | null + finalUrl: string + hops: string[] +} + +/** + * SSRF-safe fetch that follows redirects manually, revalidating the host + * allowlist + private-IP check on every hop. Throws `McpError` for blocked + * URLs, non-2xx responses, oversize bodies, or timeouts. + */ +export async function safeFetch( + urlStr: string, + opts: SafeFetchOptions = {}, +): Promise { + const maxBytes = opts.maxBytes ?? DEFAULT_MAX_BYTES + const timeoutMs = opts.timeoutMs ?? DEFAULT_TIMEOUT_MS + const hops: string[] = [] + let current = urlStr + const controller = new AbortController() + const timer = setTimeout(() => controller.abort(), timeoutMs) + try { + for (let i = 0; i <= MAX_REDIRECTS; i++) { + const parsed = assertAllowedUrl(current) + hops.push(parsed.toString()) + const res = await fetch(parsed, { + redirect: 'manual', + signal: controller.signal, + headers: opts.accept ? { Accept: opts.accept } : undefined, + }) + // Manual redirect handling + if (res.status >= 300 && res.status < 400) { + const location = res.headers.get('location') + if (!location) { + throw new McpError(ErrorCode.InvalidParams, 'redirect_without_location', { + url: parsed.toString(), + status: res.status, + }) + } + current = new URL(location, parsed).toString() + continue + } + if (!res.ok) { + throw new McpError(ErrorCode.InvalidParams, 'fetch_failed', { + url: parsed.toString(), + status: res.status, + statusText: res.statusText, + }) + } + // Enforce Content-Length up front if present. + const declared = Number(res.headers.get('content-length')) + if (Number.isFinite(declared) && declared > maxBytes) { + throw new McpError(ErrorCode.InvalidParams, 'response_too_large', { + url: parsed.toString(), + declared, + maxBytes, + }) + } + // Stream with a running cap so servers that lie about length still get bounded. + const reader = res.body?.getReader() + if (!reader) { + throw new McpError(ErrorCode.InvalidParams, 'empty_response', { + url: parsed.toString(), + }) + } + const chunks: Uint8Array[] = [] + let total = 0 + while (true) { + const { done, value } = await reader.read() + if (done) break + if (value) { + total += value.byteLength + if (total > maxBytes) { + try { + await reader.cancel() + } catch { + // ignore + } + throw new McpError(ErrorCode.InvalidParams, 'response_too_large', { + url: parsed.toString(), + received: total, + maxBytes, + }) + } + chunks.push(value) + } + } + return { + buffer: Buffer.concat(chunks.map((c) => Buffer.from(c))), + contentType: res.headers.get('content-type'), + finalUrl: parsed.toString(), + hops, + } + } + throw new McpError(ErrorCode.InvalidParams, 'too_many_redirects', { + hops: hops.slice(0, MAX_REDIRECTS + 1), + }) + } catch (err) { + if (err instanceof McpError) throw err + if ((err as { name?: string }).name === 'AbortError') { + throw new McpError(ErrorCode.InvalidParams, 'fetch_timeout', { url: urlStr, timeoutMs }) + } + const message = err instanceof Error ? err.message : String(err) + throw new McpError(ErrorCode.InvalidParams, 'fetch_error', { url: urlStr, message }) + } finally { + clearTimeout(timer) + } +} diff --git a/packages/mcp/src/tools/photo-to-scene/photo-to-scene.ts b/packages/mcp/src/tools/photo-to-scene/photo-to-scene.ts index d481bc26..8ee25c15 100644 --- a/packages/mcp/src/tools/photo-to-scene/photo-to-scene.ts +++ b/packages/mcp/src/tools/photo-to-scene/photo-to-scene.ts @@ -101,17 +101,11 @@ type ImageBlock = { */ async function resolveImageBlock(image: string): Promise { if (/^https?:\/\//i.test(image)) { - const res = await fetch(image) - if (!res.ok) { - throw new McpError( - ErrorCode.InvalidParams, - `failed to fetch image: ${res.status} ${res.statusText}`, - { url: image, status: res.status }, - ) - } - const buf = Buffer.from(await res.arrayBuffer()) - const data = buf.toString('base64') - const mimeType = res.headers.get('content-type') ?? 'image/jpeg' + // SSRF-safe fetch (see packages/mcp/src/lib/safe-fetch.ts). + const { safeFetch } = await import('../../lib/safe-fetch') + const res = await safeFetch(image, { accept: 'image/*' }) + const data = res.buffer.toString('base64') + const mimeType = res.contentType ?? 'image/jpeg' return { type: 'image', data, mimeType } } diff --git a/packages/mcp/src/tools/vision/analyze-floorplan-image.ts b/packages/mcp/src/tools/vision/analyze-floorplan-image.ts index 9f50bc7c..98c7d9f7 100644 --- a/packages/mcp/src/tools/vision/analyze-floorplan-image.ts +++ b/packages/mcp/src/tools/vision/analyze-floorplan-image.ts @@ -75,17 +75,12 @@ type ImageBlock = { */ async function resolveImageBlock(image: string): Promise { if (/^https?:\/\//i.test(image)) { - const res = await fetch(image) - if (!res.ok) { - throw new McpError( - ErrorCode.InvalidParams, - `failed to fetch image: ${res.status} ${res.statusText}`, - { url: image, status: res.status }, - ) - } - const buf = Buffer.from(await res.arrayBuffer()) - const data = buf.toString('base64') - const mimeType = res.headers.get('content-type') ?? 'image/jpeg' + // SSRF-safe fetch: blocks loopback / private / link-local / metadata IPs, + // caps size at 20 MB, times out at 10s, validates each redirect hop. + const { safeFetch } = await import('../../lib/safe-fetch') + const res = await safeFetch(image, { accept: 'image/*' }) + const data = res.buffer.toString('base64') + const mimeType = res.contentType ?? 'image/jpeg' return { type: 'image', data, mimeType } } diff --git a/packages/mcp/src/tools/vision/analyze-room-photo.ts b/packages/mcp/src/tools/vision/analyze-room-photo.ts index d1020673..a22f2280 100644 --- a/packages/mcp/src/tools/vision/analyze-room-photo.ts +++ b/packages/mcp/src/tools/vision/analyze-room-photo.ts @@ -58,17 +58,11 @@ type ImageBlock = { async function resolveImageBlock(image: string): Promise { if (/^https?:\/\//i.test(image)) { - const res = await fetch(image) - if (!res.ok) { - throw new McpError( - ErrorCode.InvalidParams, - `failed to fetch image: ${res.status} ${res.statusText}`, - { url: image, status: res.status }, - ) - } - const buf = Buffer.from(await res.arrayBuffer()) - const data = buf.toString('base64') - const mimeType = res.headers.get('content-type') ?? 'image/jpeg' + // SSRF-safe fetch (see packages/mcp/src/lib/safe-fetch.ts). + const { safeFetch } = await import('../../lib/safe-fetch') + const res = await safeFetch(image, { accept: 'image/*' }) + const data = res.buffer.toString('base64') + const mimeType = res.contentType ?? 'image/jpeg' return { type: 'image', data, mimeType } }