mirror of
https://github.com/simstudioai/sim.git
synced 2026-04-06 03:00:16 -04:00
fix(security): allow HTTP for localhost and loopback addresses (#3304)
* fix(security): allow localhost HTTP without weakening SSRF protections * fix(security): remove extraneous comments and fix failing SSRF test * fix(security): derive isLocalhost from hostname not resolved IP in validateUrlWithDNS * fix(security): verify resolved IP is loopback when hostname is localhost in validateUrlWithDNS --------- Co-authored-by: aayush598 <aayushgid598@gmail.com>
This commit is contained in:
@@ -211,7 +211,7 @@ describe('Function Execute API Route', () => {
|
||||
|
||||
it.concurrent('should block SSRF attacks through secure fetch wrapper', async () => {
|
||||
expect(validateProxyUrl('http://169.254.169.254/latest/meta-data/').isValid).toBe(false)
|
||||
expect(validateProxyUrl('http://127.0.0.1:8080/admin').isValid).toBe(false)
|
||||
expect(validateProxyUrl('http://127.0.0.1:8080/admin').isValid).toBe(true)
|
||||
expect(validateProxyUrl('http://192.168.1.1/config').isValid).toBe(false)
|
||||
expect(validateProxyUrl('http://10.0.0.1/internal').isValid).toBe(false)
|
||||
})
|
||||
|
||||
@@ -64,10 +64,31 @@ export async function validateUrlWithDNS(
|
||||
const parsedUrl = new URL(url!)
|
||||
const hostname = parsedUrl.hostname
|
||||
|
||||
try {
|
||||
const { address } = await dns.lookup(hostname)
|
||||
const hostnameLower = hostname.toLowerCase()
|
||||
const cleanHostname =
|
||||
hostnameLower.startsWith('[') && hostnameLower.endsWith(']')
|
||||
? hostnameLower.slice(1, -1)
|
||||
: hostnameLower
|
||||
|
||||
if (isPrivateOrReservedIP(address)) {
|
||||
let isLocalhost = cleanHostname === 'localhost'
|
||||
if (ipaddr.isValid(cleanHostname)) {
|
||||
const processedIP = ipaddr.process(cleanHostname).toString()
|
||||
if (processedIP === '127.0.0.1' || processedIP === '::1') {
|
||||
isLocalhost = true
|
||||
}
|
||||
}
|
||||
|
||||
try {
|
||||
const { address } = await dns.lookup(cleanHostname, { verbatim: true })
|
||||
|
||||
const resolvedIsLoopback =
|
||||
ipaddr.isValid(address) &&
|
||||
(() => {
|
||||
const ip = ipaddr.process(address).toString()
|
||||
return ip === '127.0.0.1' || ip === '::1'
|
||||
})()
|
||||
|
||||
if (isPrivateOrReservedIP(address) && !(isLocalhost && resolvedIsLoopback)) {
|
||||
logger.warn('URL resolves to blocked IP address', {
|
||||
paramName,
|
||||
hostname,
|
||||
@@ -189,8 +210,6 @@ export async function secureFetchWithPinnedIP(
|
||||
|
||||
const agent = isHttps ? new https.Agent(agentOptions) : new http.Agent(agentOptions)
|
||||
|
||||
// Remove accept-encoding since Node.js http/https doesn't auto-decompress
|
||||
// Headers are lowercase due to Web Headers API normalization in executeToolRequest
|
||||
const { 'accept-encoding': _, ...sanitizedHeaders } = options.headers ?? {}
|
||||
|
||||
const requestOptions: http.RequestOptions = {
|
||||
@@ -200,7 +219,7 @@ export async function secureFetchWithPinnedIP(
|
||||
method: options.method || 'GET',
|
||||
headers: sanitizedHeaders,
|
||||
agent,
|
||||
timeout: options.timeout || 300000, // Default 5 minutes
|
||||
timeout: options.timeout || 300000,
|
||||
}
|
||||
|
||||
const protocol = isHttps ? https : http
|
||||
|
||||
@@ -569,10 +569,28 @@ describe('validateUrlWithDNS', () => {
|
||||
expect(result.error).toContain('https://')
|
||||
})
|
||||
|
||||
it('should reject localhost URLs', async () => {
|
||||
it('should accept https localhost URLs', async () => {
|
||||
const result = await validateUrlWithDNS('https://localhost/api')
|
||||
expect(result.isValid).toBe(false)
|
||||
expect(result.error).toContain('localhost')
|
||||
expect(result.isValid).toBe(true)
|
||||
expect(result.resolvedIP).toBeDefined()
|
||||
})
|
||||
|
||||
it('should accept http localhost URLs', async () => {
|
||||
const result = await validateUrlWithDNS('http://localhost/api')
|
||||
expect(result.isValid).toBe(true)
|
||||
expect(result.resolvedIP).toBeDefined()
|
||||
})
|
||||
|
||||
it('should accept IPv4 loopback URLs', async () => {
|
||||
const result = await validateUrlWithDNS('http://127.0.0.1/api')
|
||||
expect(result.isValid).toBe(true)
|
||||
expect(result.resolvedIP).toBeDefined()
|
||||
})
|
||||
|
||||
it('should accept IPv6 loopback URLs', async () => {
|
||||
const result = await validateUrlWithDNS('http://[::1]/api')
|
||||
expect(result.isValid).toBe(true)
|
||||
expect(result.resolvedIP).toBeDefined()
|
||||
})
|
||||
|
||||
it('should reject private IP URLs', async () => {
|
||||
@@ -898,17 +916,37 @@ describe('validateExternalUrl', () => {
|
||||
expect(result.isValid).toBe(false)
|
||||
expect(result.error).toContain('valid URL')
|
||||
})
|
||||
})
|
||||
|
||||
it.concurrent('should reject localhost', () => {
|
||||
describe('localhost and loopback addresses', () => {
|
||||
it.concurrent('should accept https localhost', () => {
|
||||
const result = validateExternalUrl('https://localhost/api')
|
||||
expect(result.isValid).toBe(false)
|
||||
expect(result.error).toContain('localhost')
|
||||
expect(result.isValid).toBe(true)
|
||||
})
|
||||
|
||||
it.concurrent('should reject 127.0.0.1', () => {
|
||||
it.concurrent('should accept http localhost', () => {
|
||||
const result = validateExternalUrl('http://localhost/api')
|
||||
expect(result.isValid).toBe(true)
|
||||
})
|
||||
|
||||
it.concurrent('should accept https 127.0.0.1', () => {
|
||||
const result = validateExternalUrl('https://127.0.0.1/api')
|
||||
expect(result.isValid).toBe(false)
|
||||
expect(result.error).toContain('private IP')
|
||||
expect(result.isValid).toBe(true)
|
||||
})
|
||||
|
||||
it.concurrent('should accept http 127.0.0.1', () => {
|
||||
const result = validateExternalUrl('http://127.0.0.1/api')
|
||||
expect(result.isValid).toBe(true)
|
||||
})
|
||||
|
||||
it.concurrent('should accept https IPv6 loopback', () => {
|
||||
const result = validateExternalUrl('https://[::1]/api')
|
||||
expect(result.isValid).toBe(true)
|
||||
})
|
||||
|
||||
it.concurrent('should accept http IPv6 loopback', () => {
|
||||
const result = validateExternalUrl('http://[::1]/api')
|
||||
expect(result.isValid).toBe(true)
|
||||
})
|
||||
|
||||
it.concurrent('should reject 0.0.0.0', () => {
|
||||
@@ -989,9 +1027,9 @@ describe('validateImageUrl', () => {
|
||||
expect(result.isValid).toBe(true)
|
||||
})
|
||||
|
||||
it.concurrent('should reject localhost URLs', () => {
|
||||
it.concurrent('should accept localhost URLs', () => {
|
||||
const result = validateImageUrl('https://localhost/image.png')
|
||||
expect(result.isValid).toBe(false)
|
||||
expect(result.isValid).toBe(true)
|
||||
})
|
||||
|
||||
it.concurrent('should use imageUrl as default param name', () => {
|
||||
|
||||
@@ -89,9 +89,9 @@ export function validatePathSegment(
|
||||
const pathTraversalPatterns = [
|
||||
'..',
|
||||
'./',
|
||||
'.\\.', // Windows path traversal
|
||||
'%2e%2e', // URL encoded ..
|
||||
'%252e%252e', // Double URL encoded ..
|
||||
'.\\.',
|
||||
'%2e%2e',
|
||||
'%252e%252e',
|
||||
'..%2f',
|
||||
'..%5c',
|
||||
'%2e%2e%2f',
|
||||
@@ -391,7 +391,6 @@ export function validateHostname(
|
||||
|
||||
const lowerHostname = hostname.toLowerCase()
|
||||
|
||||
// Block localhost
|
||||
if (lowerHostname === 'localhost') {
|
||||
logger.warn('Hostname is localhost', { paramName })
|
||||
return {
|
||||
@@ -400,7 +399,6 @@ export function validateHostname(
|
||||
}
|
||||
}
|
||||
|
||||
// Use ipaddr.js to check if hostname is an IP and if it's private/reserved
|
||||
if (ipaddr.isValid(lowerHostname)) {
|
||||
if (isPrivateOrReservedIP(lowerHostname)) {
|
||||
logger.warn('Hostname matches blocked IP range', {
|
||||
@@ -414,7 +412,6 @@ export function validateHostname(
|
||||
}
|
||||
}
|
||||
|
||||
// Basic hostname format validation
|
||||
const hostnamePattern =
|
||||
/^[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?(\.[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?)*$/i
|
||||
|
||||
@@ -460,10 +457,7 @@ export function validateFileExtension(
|
||||
}
|
||||
}
|
||||
|
||||
// Remove leading dot if present
|
||||
const ext = extension.startsWith('.') ? extension.slice(1) : extension
|
||||
|
||||
// Normalize to lowercase
|
||||
const normalizedExt = ext.toLowerCase()
|
||||
|
||||
if (!allowedExtensions.map((e) => e.toLowerCase()).includes(normalizedExt)) {
|
||||
@@ -515,7 +509,6 @@ export function validateMicrosoftGraphId(
|
||||
}
|
||||
}
|
||||
|
||||
// Check for path traversal patterns (../)
|
||||
const pathTraversalPatterns = [
|
||||
'../',
|
||||
'..\\',
|
||||
@@ -525,7 +518,7 @@ export function validateMicrosoftGraphId(
|
||||
'%2e%2e%5c',
|
||||
'%2e%2e\\',
|
||||
'..%5c',
|
||||
'%252e%252e%252f', // double encoded
|
||||
'%252e%252e%252f',
|
||||
]
|
||||
|
||||
const lowerValue = value.toLowerCase()
|
||||
@@ -542,7 +535,6 @@ export function validateMicrosoftGraphId(
|
||||
}
|
||||
}
|
||||
|
||||
// Check for control characters and null bytes
|
||||
if (/[\x00-\x1f\x7f]/.test(value) || value.includes('%00')) {
|
||||
logger.warn('Control characters in Microsoft Graph ID', { paramName })
|
||||
return {
|
||||
@@ -551,7 +543,6 @@ export function validateMicrosoftGraphId(
|
||||
}
|
||||
}
|
||||
|
||||
// Check for newlines (which could be used for header injection)
|
||||
if (value.includes('\n') || value.includes('\r')) {
|
||||
return {
|
||||
isValid: false,
|
||||
@@ -559,8 +550,6 @@ export function validateMicrosoftGraphId(
|
||||
}
|
||||
}
|
||||
|
||||
// Microsoft Graph IDs can contain many characters, but not suspicious patterns
|
||||
// We've blocked path traversal, so allow the rest
|
||||
return { isValid: true, sanitized: value }
|
||||
}
|
||||
|
||||
@@ -583,7 +572,6 @@ export function validateJiraCloudId(
|
||||
value: string | null | undefined,
|
||||
paramName = 'cloudId'
|
||||
): ValidationResult {
|
||||
// Jira cloud IDs are alphanumeric with hyphens (UUID-like)
|
||||
return validatePathSegment(value, {
|
||||
paramName,
|
||||
allowHyphens: true,
|
||||
@@ -612,7 +600,6 @@ export function validateJiraIssueKey(
|
||||
value: string | null | undefined,
|
||||
paramName = 'issueKey'
|
||||
): ValidationResult {
|
||||
// Jira issue keys: letters, numbers, hyphens (PROJECT-123 format)
|
||||
return validatePathSegment(value, {
|
||||
paramName,
|
||||
allowHyphens: true,
|
||||
@@ -653,7 +640,6 @@ export function validateExternalUrl(
|
||||
}
|
||||
}
|
||||
|
||||
// Must be a valid URL
|
||||
let parsedUrl: URL
|
||||
try {
|
||||
parsedUrl = new URL(url)
|
||||
@@ -664,28 +650,29 @@ export function validateExternalUrl(
|
||||
}
|
||||
}
|
||||
|
||||
// Only allow https protocol
|
||||
if (parsedUrl.protocol !== 'https:') {
|
||||
const protocol = parsedUrl.protocol
|
||||
const hostname = parsedUrl.hostname.toLowerCase()
|
||||
|
||||
const cleanHostname =
|
||||
hostname.startsWith('[') && hostname.endsWith(']') ? hostname.slice(1, -1) : hostname
|
||||
|
||||
let isLocalhost = cleanHostname === 'localhost'
|
||||
if (ipaddr.isValid(cleanHostname)) {
|
||||
const processedIP = ipaddr.process(cleanHostname).toString()
|
||||
if (processedIP === '127.0.0.1' || processedIP === '::1') {
|
||||
isLocalhost = true
|
||||
}
|
||||
}
|
||||
|
||||
if (protocol !== 'https:' && !(protocol === 'http:' && isLocalhost)) {
|
||||
return {
|
||||
isValid: false,
|
||||
error: `${paramName} must use https:// protocol`,
|
||||
}
|
||||
}
|
||||
|
||||
// Block private IP ranges and localhost
|
||||
const hostname = parsedUrl.hostname.toLowerCase()
|
||||
|
||||
// Block localhost
|
||||
if (hostname === 'localhost') {
|
||||
return {
|
||||
isValid: false,
|
||||
error: `${paramName} cannot point to localhost`,
|
||||
}
|
||||
}
|
||||
|
||||
// Use ipaddr.js to check if hostname is an IP and if it's private/reserved
|
||||
if (ipaddr.isValid(hostname)) {
|
||||
if (isPrivateOrReservedIP(hostname)) {
|
||||
if (!isLocalhost && ipaddr.isValid(cleanHostname)) {
|
||||
if (isPrivateOrReservedIP(cleanHostname)) {
|
||||
return {
|
||||
isValid: false,
|
||||
error: `${paramName} cannot point to private IP addresses`,
|
||||
|
||||
Reference in New Issue
Block a user