-
Notifications
You must be signed in to change notification settings - Fork 3.6k
feat(security): add ALLOWED_PRIVATE_HOSTS allowlist for SSRF block #4677
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: staging
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 |
|---|---|---|
| @@ -0,0 +1,134 @@ | ||
| /** | ||
| * @vitest-environment node | ||
| */ | ||
| import { createEnvMock } from '@sim/testing' | ||
| import { afterEach, describe, expect, it, vi } from 'vitest' | ||
|
|
||
| vi.mock('@/lib/core/config/env', () => createEnvMock()) | ||
|
|
||
| import { env } from '@/lib/core/config/env' | ||
| import { | ||
| __resetAllowedPrivateHostsCacheForTest, | ||
| getAllowedPrivateHostsFromEnv, | ||
| isAllowlistedPrivateHost, | ||
| } from '@/lib/core/config/feature-flags' | ||
|
|
||
| function withAllowedPrivateHosts(value: string | undefined) { | ||
| ;(env as { ALLOWED_PRIVATE_HOSTS?: string }).ALLOWED_PRIVATE_HOSTS = value | ||
| __resetAllowedPrivateHostsCacheForTest() | ||
| } | ||
|
|
||
| describe('getAllowedPrivateHostsFromEnv', () => { | ||
| afterEach(() => { | ||
| withAllowedPrivateHosts(undefined) | ||
| }) | ||
|
|
||
| it('returns null when env var is unset', () => { | ||
| expect(getAllowedPrivateHostsFromEnv()).toBeNull() | ||
| }) | ||
|
|
||
| it('returns null when env var is empty after trimming', () => { | ||
| withAllowedPrivateHosts(' , , ') | ||
| expect(getAllowedPrivateHostsFromEnv()).toBeNull() | ||
| }) | ||
|
|
||
| it('parses bare hostnames into the hostname set (lowercased)', () => { | ||
| withAllowedPrivateHosts('Gitlab.Allot.Internal,siem.allot.internal') | ||
| const result = getAllowedPrivateHostsFromEnv() | ||
| expect(result?.hostnames).toEqual(new Set(['gitlab.allot.internal', 'siem.allot.internal'])) | ||
| expect(result?.cidrs).toEqual([]) | ||
| }) | ||
|
|
||
| it('parses literal IPs as exact /32 or /128 CIDRs', () => { | ||
| withAllowedPrivateHosts('10.112.12.56,fd00::1') | ||
| const result = getAllowedPrivateHostsFromEnv() | ||
| expect(result?.cidrs).toHaveLength(2) | ||
| expect(result?.cidrs[0][1]).toBe(32) | ||
| expect(result?.cidrs[1][1]).toBe(128) | ||
| }) | ||
|
|
||
| it('parses CIDR ranges', () => { | ||
| withAllowedPrivateHosts('10.0.0.0/8,fd00::/8') | ||
| const result = getAllowedPrivateHostsFromEnv() | ||
| expect(result?.cidrs).toHaveLength(2) | ||
| expect(result?.cidrs[0][1]).toBe(8) | ||
| expect(result?.cidrs[1][1]).toBe(8) | ||
| }) | ||
|
|
||
| it('mixes hostnames, IPs, and CIDRs in one list', () => { | ||
| withAllowedPrivateHosts('gitlab.internal, 10.0.0.0/8 ,10.112.12.56 ') | ||
| const result = getAllowedPrivateHostsFromEnv() | ||
| expect(result?.hostnames.has('gitlab.internal')).toBe(true) | ||
| expect(result?.cidrs).toHaveLength(2) | ||
| }) | ||
|
|
||
| it('falls back to hostname when CIDR parse fails', () => { | ||
| withAllowedPrivateHosts('not-a-cidr/bogus') | ||
| const result = getAllowedPrivateHostsFromEnv() | ||
| expect(result?.hostnames.has('not-a-cidr/bogus')).toBe(true) | ||
| }) | ||
|
|
||
| it('caches the parse result across calls', () => { | ||
| withAllowedPrivateHosts('gitlab.internal') | ||
| const first = getAllowedPrivateHostsFromEnv() | ||
| ;(env as { ALLOWED_PRIVATE_HOSTS?: string }).ALLOWED_PRIVATE_HOSTS = 'changed.internal' | ||
| expect(getAllowedPrivateHostsFromEnv()).toBe(first) | ||
| }) | ||
| }) | ||
|
|
||
| describe('isAllowlistedPrivateHost', () => { | ||
| afterEach(() => { | ||
| withAllowedPrivateHosts(undefined) | ||
| }) | ||
|
|
||
| it('returns false when env var is unset', () => { | ||
| expect(isAllowlistedPrivateHost({ ip: '10.0.0.1' })).toBe(false) | ||
| expect(isAllowlistedPrivateHost({ hostname: 'gitlab.internal' })).toBe(false) | ||
| }) | ||
|
|
||
| it('matches hostnames case-insensitively', () => { | ||
| withAllowedPrivateHosts('gitlab.allot.internal') | ||
| expect(isAllowlistedPrivateHost({ hostname: 'GITLAB.ALLOT.INTERNAL' })).toBe(true) | ||
| expect(isAllowlistedPrivateHost({ hostname: 'other.internal' })).toBe(false) | ||
| }) | ||
|
|
||
| it('matches literal IPv4 entries', () => { | ||
| withAllowedPrivateHosts('10.112.12.56') | ||
| expect(isAllowlistedPrivateHost({ ip: '10.112.12.56' })).toBe(true) | ||
| expect(isAllowlistedPrivateHost({ ip: '10.112.12.57' })).toBe(false) | ||
| }) | ||
|
|
||
| it('matches IPv4 CIDR ranges', () => { | ||
| withAllowedPrivateHosts('10.0.0.0/8') | ||
| expect(isAllowlistedPrivateHost({ ip: '10.0.0.1' })).toBe(true) | ||
| expect(isAllowlistedPrivateHost({ ip: '10.255.255.255' })).toBe(true) | ||
| expect(isAllowlistedPrivateHost({ ip: '11.0.0.1' })).toBe(false) | ||
| expect(isAllowlistedPrivateHost({ ip: '192.168.1.1' })).toBe(false) | ||
| }) | ||
|
|
||
| it('matches IPv6 CIDR ranges', () => { | ||
| withAllowedPrivateHosts('fc00::/7') | ||
| expect(isAllowlistedPrivateHost({ ip: 'fd00::1' })).toBe(true) | ||
| expect(isAllowlistedPrivateHost({ ip: 'fd12:3456::1' })).toBe(true) | ||
| expect(isAllowlistedPrivateHost({ ip: 'fc00::1' })).toBe(true) | ||
| expect(isAllowlistedPrivateHost({ ip: '2001:db8::1' })).toBe(false) | ||
| }) | ||
|
|
||
| it('does not cross-match IPv4 and IPv6 ranges', () => { | ||
| withAllowedPrivateHosts('10.0.0.0/8') | ||
| expect(isAllowlistedPrivateHost({ ip: 'fd00::1' })).toBe(false) | ||
| }) | ||
|
|
||
| it('returns true if either hostname or IP matches', () => { | ||
| withAllowedPrivateHosts('gitlab.allot.internal,10.0.0.0/8') | ||
| expect(isAllowlistedPrivateHost({ hostname: 'gitlab.allot.internal', ip: '8.8.8.8' })).toBe( | ||
| true | ||
| ) | ||
| expect(isAllowlistedPrivateHost({ hostname: 'other.internal', ip: '10.5.5.5' })).toBe(true) | ||
| }) | ||
|
|
||
| it('returns false for unparseable IPs', () => { | ||
| withAllowedPrivateHosts('10.0.0.0/8') | ||
| expect(isAllowlistedPrivateHost({ ip: 'not-an-ip' })).toBe(false) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import { createLogger } from '@sim/logger' | ||
| import * as ipaddr from 'ipaddr.js' | ||
| import { isHosted } from '@/lib/core/config/feature-flags' | ||
| import { isAllowlistedPrivateHost, isHosted } from '@/lib/core/config/feature-flags' | ||
|
|
||
| const logger = createLogger('InputValidation') | ||
|
|
||
|
|
@@ -401,7 +401,7 @@ export function validateHostname( | |
| } | ||
|
|
||
| if (ipaddr.isValid(lowerHostname)) { | ||
| if (isPrivateOrReservedIP(lowerHostname)) { | ||
| if (isPrivateOrReservedIP(lowerHostname) && !isAllowlistedPrivateHost({ ip: lowerHostname })) { | ||
| logger.warn('Hostname matches blocked IP range', { | ||
| paramName, | ||
| hostname: hostname.substring(0, 100), | ||
|
|
@@ -411,6 +411,8 @@ export function validateHostname( | |
| error: `${paramName} cannot be a private IP address or localhost`, | ||
| } | ||
| } | ||
| } else if (isAllowlistedPrivateHost({ hostname: lowerHostname })) { | ||
| return { isValid: true, sanitized: lowerHostname } | ||
| } | ||
|
Comment on lines
+414
to
416
Contributor
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 |
||
|
|
||
| const hostnamePattern = | ||
|
|
@@ -733,7 +735,7 @@ export function validateExternalUrl( | |
| } | ||
|
|
||
| if (!isLocalhost && ipaddr.isValid(cleanHostname)) { | ||
| if (isPrivateOrReservedIP(cleanHostname)) { | ||
| if (isPrivateOrReservedIP(cleanHostname) && !isAllowlistedPrivateHost({ ip: cleanHostname })) { | ||
| return { | ||
| isValid: false, | ||
| error: `${paramName} cannot point to private IP addresses`, | ||
|
|
||
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.
Allowlist bypasses loopback SSRF guard
High Severity
isAllowlistedPrivateHostcan clear blocks for loopback and other reserved targets because callers treat it as a blanket override onisPrivateOrReservedIP. On hosted, an allowlisted hostname that resolves to127.0.0.0/8can reach local services; MCP already rejects loopback before the allowlist, butvalidateUrlWithDNS,validateDatabaseHost, andvalidateHostnamedo not.Additional Locations (2)
apps/sim/lib/core/security/input-validation.server.ts#L185-L188apps/sim/lib/core/security/input-validation.ts#L402-L415Reviewed by Cursor Bugbot for commit 2ad06cb. Configure here.