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) <noreply@anthropic.com>
2026-04-19 20:59:26 +02:00
parent 08e7b6db71
commit 8757de0c36
8 changed files with 371 additions and 68 deletions
@@ -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(),
 })
@@ -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(),
 })

@@ -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,
+          })
+        }
+      }
+    }
+  })
@@ -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
+      }
+    }
+  })
+})
@@ -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<SafeFetchResult> {
+  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)
+  }
+}
@@ -101,17 +101,11 @@ type ImageBlock = {
 */
 async function resolveImageBlock(image: string): Promise<ImageBlock> {
  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 }
  }

@@ -75,17 +75,12 @@ type ImageBlock = {
 */
 async function resolveImageBlock(image: string): Promise<ImageBlock> {
  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 }
  }

@@ -58,17 +58,11 @@ type ImageBlock = {

 async function resolveImageBlock(image: string): Promise<ImageBlock> {
  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 }
  }