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>
This commit is contained in:
Adrian Perez
co-authored by
Claude Opus 4.7
Claude Opus 4.7
parent
08e7b6db71
commit
8757de0c36
@@ -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 }
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user