-
Notifications
You must be signed in to change notification settings - Fork 7.4k
fix(security): harden API proxy, CORS, SSRF, ReDoS, and sanitization #2226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,4 +1,5 @@ | ||||||
| import { getCorsHeaders, isDisallowedOrigin } from './_cors.js'; | ||||||
| import { checkRateLimit } from './_rate-limit.js'; | ||||||
| import { jsonResponse } from './_json-response.js'; | ||||||
|
|
||||||
| export const config = { runtime: 'edge' }; | ||||||
|
|
@@ -11,11 +12,16 @@ const MCP_PROTOCOL_VERSION = '2025-03-26'; | |||||
| const BLOCKED_HOST_PATTERNS = [ | ||||||
| /^localhost$/i, | ||||||
| /^127\./, | ||||||
| /^0\.0\.0\.0$/, // unspecified IPv4 — routes to loopback on many systems | ||||||
| /^0+$/, // zero in various forms | ||||||
| /^10\./, | ||||||
| /^172\.(1[6-9]|2\d|3[01])\./, | ||||||
| /^192\.168\./, | ||||||
| /^169\.254\./, // link-local + cloud metadata (AWS/GCP/Azure) | ||||||
| /^169\.254\./, // link-local + cloud metadata (AWS/GCP/Azure) | ||||||
| /^::1$/, | ||||||
| /^::$/, // unspecified IPv6 | ||||||
| /^::ffff:/i, // IPv4-mapped IPv6 (e.g. ::ffff:127.0.0.1) | ||||||
| /^\[/, // bracket-wrapped IPv6 in hostname | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The WHATWG URL parser (used in Cloudflare Workers / Vercel Edge) always strips brackets from IPv6 hostnames. The existing patterns (
Suggested change
Consider removing the check or adding a comment that it guards only hypothetical non-URL-parsed callers, so future reviewers don't rely on it for URL-derived hostnames. |
||||||
| /^fd[0-9a-f]{2}:/i, | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The RFC 4193 Unique Local Address (ULA) range is An attacker supplying a serverUrl like
Suggested change
This covers both |
||||||
| /^fe80:/i, | ||||||
| ]; | ||||||
|
|
@@ -42,6 +48,31 @@ function validateServerUrl(raw) { | |||||
| return url; | ||||||
| } | ||||||
|
|
||||||
| // Headers that must not be overridden by user-supplied custom headers. | ||||||
| // Allowing these to be set by the client could lead to SSRF (Host), auth | ||||||
| // hijacking, or request smuggling via hop-by-hop headers. | ||||||
| const BLOCKED_HEADER_NAMES = new Set([ | ||||||
| 'host', | ||||||
| 'cookie', | ||||||
| 'set-cookie', | ||||||
| 'transfer-encoding', | ||||||
| 'content-length', | ||||||
| 'connection', | ||||||
| 'keep-alive', | ||||||
| 'te', | ||||||
| 'trailer', | ||||||
| 'upgrade', | ||||||
| 'proxy-authorization', | ||||||
| 'proxy-authenticate', | ||||||
| 'via', | ||||||
| 'forwarded', | ||||||
| 'x-forwarded-for', | ||||||
| 'x-forwarded-host', | ||||||
| 'x-forwarded-proto', | ||||||
| 'x-real-ip', | ||||||
| 'cf-connecting-ip', | ||||||
| ]); | ||||||
|
|
||||||
| function buildHeaders(customHeaders) { | ||||||
| const h = { | ||||||
| 'Content-Type': 'application/json', | ||||||
|
|
@@ -54,7 +85,8 @@ function buildHeaders(customHeaders) { | |||||
| // Strip CRLF to prevent header injection | ||||||
| const safeKey = k.replace(/[\r\n]/g, ''); | ||||||
| const safeVal = v.replace(/[\r\n]/g, ''); | ||||||
| if (safeKey) h[safeKey] = safeVal; | ||||||
| if (!safeKey || BLOCKED_HEADER_NAMES.has(safeKey.toLowerCase())) continue; | ||||||
| h[safeKey] = safeVal; | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
@@ -334,6 +366,9 @@ export default async function handler(req) { | |||||
| if (req.method === 'OPTIONS') | ||||||
| return new Response(null, { status: 204, headers: cors }); | ||||||
|
|
||||||
| const rateLimitResponse = await checkRateLimit(req, cors); | ||||||
| if (rateLimitResponse) return rateLimitResponse; | ||||||
|
|
||||||
| try { | ||||||
| if (req.method === 'GET') { | ||||||
| const url = new URL(req.url); | ||||||
|
|
@@ -369,7 +404,9 @@ export default async function handler(req) { | |||||
| } catch (err) { | ||||||
| const msg = err instanceof Error ? err.message : String(err); | ||||||
| const isTimeout = msg.includes('TimeoutError') || msg.includes('timed out'); | ||||||
| // Return 422 (not 502) so Cloudflare proxy does not replace our JSON body with its own HTML error page | ||||||
| return jsonResponse({ error: isTimeout ? 'MCP server timed out' : msg }, isTimeout ? 504 : 422, cors); | ||||||
| console.error('[mcp-proxy] error:', msg); | ||||||
| // Return 422 (not 502) so Cloudflare proxy does not replace our JSON body with its own HTML error page. | ||||||
| // Avoid leaking internal error details to the client. | ||||||
| return jsonResponse({ error: isTimeout ? 'MCP server timed out' : 'MCP request failed' }, isTimeout ? 504 : 422, cors); | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -220,7 +220,9 @@ export function createDomainGateway( | |||||||||||||||||||||
| try { | ||||||||||||||||||||||
| corsHeaders = getCorsHeaders(request); | ||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||
| corsHeaders = { 'Access-Control-Allow-Origin': '*' }; | ||||||||||||||||||||||
| // Never fall back to wildcard CORS — that would bypass the origin allowlist. | ||||||||||||||||||||||
| // Use the hardcoded production origin as a safe default. | ||||||||||||||||||||||
| corsHeaders = { 'Access-Control-Allow-Origin': 'https://worldmonitor.app', 'Vary': 'Origin' }; | ||||||||||||||||||||||
|
Comment on lines
222
to
+225
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When The intent to avoid the previous wildcard fallback is correct. A safer approach is to echo back the request's
Suggested change
|
||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // OPTIONS preflight | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both the
wss://replacement and the fallbackreplace(/^[a-z]+:\/\//, '')use case-sensitive patterns. If an operator setsWS_RELAY_URL=WSS://relay.example.com(uppercase — valid in RFC 3986), neither regex matches:/^wss:\/\//→ no match →httpUrl = 'WSS://relay.example.com'httpUrl.startsWith('https://')→false/^[a-z]+:\/\//→ no match → strips nothing →'https://WSS://relay.example.com'This silently constructs a malformed URL that will cause all relay requests to fail with no clear error. The same issue exists in the mirrored copy at
server/_shared/relay.ts(line 5).The same fix should be applied to the identical logic in
server/_shared/relay.ts.