mirror of
https://github.com/simstudioai/sim.git
synced 2026-04-06 03:00:16 -04:00
fix(security): pentest remediation — condition escaping, SSRF hardening, ReDoS protection (#3820)
* fix(executor): escape newline characters in condition expression strings Unescaped newline/carriage-return characters in resolved string values cause unterminated string literals in generated JS, crashing condition evaluation with a SyntaxError. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): prevent ReDoS in guardrails regex validation Add safe-regex2 to reject catastrophic backtracking patterns before execution and cap input length at 10k characters. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): SSRF localhost hardening and regex DoS protection Block localhost/loopback URLs in hosted environments using isHosted flag instead of allowHttp. Add safe-regex2 validation and input length limits to regex guardrails to prevent catastrophic backtracking. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): validate regex syntax before safety check Move new RegExp() before safe() so invalid patterns get a proper syntax error instead of a misleading "catastrophic backtracking" message. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): address PR review feedback - Hoist isLocalhost && isHosted guard to single early-return before protocol checks, removing redundant duplicate block - Move regex syntax validation (new RegExp) before safe-regex2 check so invalid patterns get proper syntax error instead of misleading "catastrophic backtracking" message * fix(security): remove input length cap from regex validation The 10k character cap would block legitimate guardrail checks on long LLM outputs. Input length doesn't affect ReDoS risk — the safe-regex2 pattern check already prevents catastrophic backtracking. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(tests): mock isHosted in input-validation and function-execute tests Tests that assert self-hosted localhost behavior need isHosted=false, which is not guaranteed in CI where NEXT_PUBLIC_APP_URL is set to the hosted domain. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -26,6 +26,14 @@ vi.mock('@/lib/execution/e2b', () => ({
|
||||
executeInE2B: mockExecuteInE2B,
|
||||
}))
|
||||
|
||||
vi.mock('@/lib/core/config/feature-flags', () => ({
|
||||
isHosted: false,
|
||||
isE2bEnabled: false,
|
||||
isProd: false,
|
||||
isDev: false,
|
||||
isTest: true,
|
||||
}))
|
||||
|
||||
import { validateProxyUrl } from '@/lib/core/security/input-validation'
|
||||
import { POST } from '@/app/api/function/execute/route'
|
||||
|
||||
|
||||
@@ -236,7 +236,13 @@ export class VariableResolver {
|
||||
}
|
||||
|
||||
if (typeof resolved === 'string') {
|
||||
const escaped = resolved.replace(/\\/g, '\\\\').replace(/'/g, "\\'")
|
||||
const escaped = resolved
|
||||
.replace(/\\/g, '\\\\')
|
||||
.replace(/'/g, "\\'")
|
||||
.replace(/\n/g, '\\n')
|
||||
.replace(/\r/g, '\\r')
|
||||
.replace(/\u2028/g, '\\u2028')
|
||||
.replace(/\u2029/g, '\\u2029')
|
||||
return `'${escaped}'`
|
||||
}
|
||||
if (typeof resolved === 'object' && resolved !== null) {
|
||||
|
||||
@@ -4,6 +4,7 @@ import https from 'https'
|
||||
import type { LookupFunction } from 'net'
|
||||
import { createLogger } from '@sim/logger'
|
||||
import * as ipaddr from 'ipaddr.js'
|
||||
import { isHosted } from '@/lib/core/config/feature-flags'
|
||||
import { type ValidationResult, validateExternalUrl } from '@/lib/core/security/input-validation'
|
||||
|
||||
const logger = createLogger('InputValidation')
|
||||
@@ -89,10 +90,7 @@ export async function validateUrlWithDNS(
|
||||
return ip === '127.0.0.1' || ip === '::1'
|
||||
})()
|
||||
|
||||
if (
|
||||
isPrivateOrReservedIP(address) &&
|
||||
!(isLocalhost && resolvedIsLoopback && !options.allowHttp)
|
||||
) {
|
||||
if (isPrivateOrReservedIP(address) && !(isLocalhost && resolvedIsLoopback && !isHosted)) {
|
||||
logger.warn('URL resolves to blocked IP address', {
|
||||
paramName,
|
||||
hostname,
|
||||
|
||||
@@ -23,6 +23,9 @@ import { validateUrlWithDNS } from '@/lib/core/security/input-validation.server'
|
||||
import { sanitizeForLogging } from '@/lib/core/security/redaction'
|
||||
|
||||
vi.mock('@sim/logger', () => loggerMock)
|
||||
vi.mock('@/lib/core/config/feature-flags', () => ({
|
||||
isHosted: false,
|
||||
}))
|
||||
|
||||
describe('validatePathSegment', () => {
|
||||
describe('valid inputs', () => {
|
||||
@@ -569,25 +572,25 @@ describe('validateUrlWithDNS', () => {
|
||||
expect(result.error).toContain('https://')
|
||||
})
|
||||
|
||||
it('should accept https localhost URLs', async () => {
|
||||
it('should accept https localhost URLs (self-hosted)', async () => {
|
||||
const result = await validateUrlWithDNS('https://localhost/api')
|
||||
expect(result.isValid).toBe(true)
|
||||
expect(result.resolvedIP).toBeDefined()
|
||||
})
|
||||
|
||||
it('should accept http localhost URLs', async () => {
|
||||
it('should accept http localhost URLs (self-hosted)', async () => {
|
||||
const result = await validateUrlWithDNS('http://localhost/api')
|
||||
expect(result.isValid).toBe(true)
|
||||
expect(result.resolvedIP).toBeDefined()
|
||||
})
|
||||
|
||||
it('should accept IPv4 loopback URLs', async () => {
|
||||
it('should accept IPv4 loopback URLs (self-hosted)', 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 () => {
|
||||
it('should accept IPv6 loopback URLs (self-hosted)', async () => {
|
||||
const result = await validateUrlWithDNS('http://[::1]/api')
|
||||
expect(result.isValid).toBe(true)
|
||||
expect(result.resolvedIP).toBeDefined()
|
||||
@@ -918,7 +921,7 @@ describe('validateExternalUrl', () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('localhost and loopback addresses', () => {
|
||||
describe('localhost and loopback addresses (self-hosted)', () => {
|
||||
it.concurrent('should accept https localhost', () => {
|
||||
const result = validateExternalUrl('https://localhost/api')
|
||||
expect(result.isValid).toBe(true)
|
||||
@@ -1027,7 +1030,7 @@ describe('validateImageUrl', () => {
|
||||
expect(result.isValid).toBe(true)
|
||||
})
|
||||
|
||||
it.concurrent('should accept localhost URLs', () => {
|
||||
it.concurrent('should accept localhost URLs (self-hosted)', () => {
|
||||
const result = validateImageUrl('https://localhost/image.png')
|
||||
expect(result.isValid).toBe(true)
|
||||
})
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import { createLogger } from '@sim/logger'
|
||||
import * as ipaddr from 'ipaddr.js'
|
||||
import { isHosted } from '@/lib/core/config/feature-flags'
|
||||
|
||||
const logger = createLogger('InputValidation')
|
||||
|
||||
@@ -710,6 +711,13 @@ export function validateExternalUrl(
|
||||
}
|
||||
}
|
||||
|
||||
if (isLocalhost && isHosted) {
|
||||
return {
|
||||
isValid: false,
|
||||
error: `${paramName} cannot point to localhost`,
|
||||
}
|
||||
}
|
||||
|
||||
if (options.allowHttp) {
|
||||
if (protocol !== 'https:' && protocol !== 'http:') {
|
||||
return {
|
||||
@@ -717,13 +725,7 @@ export function validateExternalUrl(
|
||||
error: `${paramName} must use http:// or https:// protocol`,
|
||||
}
|
||||
}
|
||||
if (isLocalhost) {
|
||||
return {
|
||||
isValid: false,
|
||||
error: `${paramName} cannot point to localhost`,
|
||||
}
|
||||
}
|
||||
} else if (protocol !== 'https:' && !(protocol === 'http:' && isLocalhost)) {
|
||||
} else if (protocol !== 'https:' && !(protocol === 'http:' && isLocalhost && !isHosted)) {
|
||||
return {
|
||||
isValid: false,
|
||||
error: `${paramName} must use https:// protocol`,
|
||||
|
||||
@@ -1,3 +1,5 @@
|
||||
import safe from 'safe-regex2'
|
||||
|
||||
/**
|
||||
* Validate if input matches regex pattern
|
||||
*/
|
||||
@@ -7,15 +9,23 @@ export interface ValidationResult {
|
||||
}
|
||||
|
||||
export function validateRegex(inputStr: string, pattern: string): ValidationResult {
|
||||
let regex: RegExp
|
||||
try {
|
||||
const regex = new RegExp(pattern)
|
||||
const match = regex.test(inputStr)
|
||||
|
||||
if (match) {
|
||||
return { passed: true }
|
||||
}
|
||||
return { passed: false, error: 'Input does not match regex pattern' }
|
||||
regex = new RegExp(pattern)
|
||||
} catch (error: any) {
|
||||
return { passed: false, error: `Invalid regex pattern: ${error.message}` }
|
||||
}
|
||||
|
||||
if (!safe(pattern)) {
|
||||
return {
|
||||
passed: false,
|
||||
error: 'Regex pattern rejected: potentially unsafe (catastrophic backtracking)',
|
||||
}
|
||||
}
|
||||
|
||||
const match = regex.test(inputStr)
|
||||
if (match) {
|
||||
return { passed: true }
|
||||
}
|
||||
return { passed: false, error: 'Input does not match regex pattern' }
|
||||
}
|
||||
|
||||
@@ -116,8 +116,8 @@
|
||||
"es-toolkit": "1.45.1",
|
||||
"ffmpeg-static": "5.3.0",
|
||||
"fluent-ffmpeg": "2.1.3",
|
||||
"free-email-domains": "1.2.25",
|
||||
"framer-motion": "^12.5.0",
|
||||
"free-email-domains": "1.2.25",
|
||||
"google-auth-library": "10.5.0",
|
||||
"gray-matter": "^4.0.3",
|
||||
"groq-sdk": "^0.15.0",
|
||||
@@ -174,6 +174,7 @@
|
||||
"remark-gfm": "4.0.1",
|
||||
"resend": "^4.1.2",
|
||||
"rss-parser": "3.13.0",
|
||||
"safe-regex2": "5.1.0",
|
||||
"sharp": "0.34.3",
|
||||
"soap": "1.8.0",
|
||||
"socket.io": "^4.8.1",
|
||||
|
||||
5
bun.lock
5
bun.lock
@@ -193,6 +193,7 @@
|
||||
"remark-gfm": "4.0.1",
|
||||
"resend": "^4.1.2",
|
||||
"rss-parser": "3.13.0",
|
||||
"safe-regex2": "5.1.0",
|
||||
"sharp": "0.34.3",
|
||||
"soap": "1.8.0",
|
||||
"socket.io": "^4.8.1",
|
||||
@@ -3320,6 +3321,8 @@
|
||||
|
||||
"restore-cursor": ["restore-cursor@3.1.0", "", { "dependencies": { "onetime": "^5.1.0", "signal-exit": "^3.0.2" } }, "sha512-l+sSefzHpj5qimhFSE5a8nufZYAM3sBSVMAPtYkmC+4EH2anSGaEMXSD0izRQbu9nfyQ9y5JrVmp7E8oZrUjvA=="],
|
||||
|
||||
"ret": ["ret@0.5.0", "", {}, "sha512-I1XxrZSQ+oErkRR4jYbAyEEu2I0avBvvMM5JN+6EBprOGRCs63ENqZ3vjavq8fBw2+62G5LF5XelKwuJpcvcxw=="],
|
||||
|
||||
"retry": ["retry@0.13.1", "", {}, "sha512-XQBQ3I8W1Cge0Seh+6gjj03LbmRFWuoszgK9ooCpwYIrhhoO80pfq4cUkU5DkknwfOfFteRwlZ56PYOGYyFWdg=="],
|
||||
|
||||
"reusify": ["reusify@1.1.0", "", {}, "sha512-g6QUff04oZpHs0eG5p83rFLhHeV00ug/Yf9nZM6fLeUrPguBTkTQOdpAWWspMh55TZfVQDPaN3NQJfbVRAxdIw=="],
|
||||
@@ -3346,6 +3349,8 @@
|
||||
|
||||
"safe-buffer": ["safe-buffer@5.2.1", "", {}, "sha512-rp3So07KcdmmKbGvgaNxQSJr7bGVSVk5S9Eq1F+ppbRo70+YeaDxkw5Dd8NPN+GD6bjnYm2VuPuCXmpuYvmCXQ=="],
|
||||
|
||||
"safe-regex2": ["safe-regex2@5.1.0", "", { "dependencies": { "ret": "~0.5.0" }, "bin": { "safe-regex2": "bin/safe-regex2.js" } }, "sha512-pNHAuBW7TrcleFHsxBr5QMi/Iyp0ENjUKz7GCcX1UO7cMh+NmVK6HxQckNL1tJp1XAJVjG6B8OKIPqodqj9rtw=="],
|
||||
|
||||
"safe-stable-stringify": ["safe-stable-stringify@2.5.0", "", {}, "sha512-b3rppTKm9T+PsVCBEOUR46GWI7fdOs00VKZ1+9c1EWDaDMvjQc6tUwuFyIprgGgTcWoVHSKrU8H31ZHA2e0RHA=="],
|
||||
|
||||
"safer-buffer": ["safer-buffer@2.1.2", "", {}, "sha512-YZo3K82SD7Riyi0E1EQPojLz7kpepnSQI9IyPbHHg1XXXevb5dJI7tpyN2ADxGcQbHG7vcyRHk0cbwqcQriUtg=="],
|
||||
|
||||
Reference in New Issue
Block a user