Skip to content

Commit 042db18

Browse files
committed
refactor(security): one DNS resolver for every SSRF guard, checking all addresses
Five independent `dns.lookup` bodies existed — four in apps/sim (validateUrlWithDNS, the database-host check, MCP domain-check, 1Password Connect) and one in apps/desktop's agent url-guard. The four in apps/sim were copy-paste identical, and two properties diverged in ways that mattered: They classified ONE address. `resolved.find(family === 4) ?? resolved[0]` picked an address to pin and then judged only that one, so a host publishing both a public and a private record passed whenever the public record sorted first. That is record order, not policy, and validateUrlWithDNS alone is reached from ~70 call sites. Now every address is classified and the IPv4-preferred one is still what gets returned to pin — the pinning rationale (Happy Eyeballs fallback is stripped, and a pinned IPv6 address hangs on IPv4-only egress) is untouched. They had no deadline. Only the desktop copy bounded the lookup, so a hung resolver could hold an apps/sim request handler open indefinitely. The shared helper carries the 5s deadline, the swallowed late rejection, and the always cleared timer. `resolveHostAddresses` lands in `@sim/security/dns` as its own subpath, so the `node:dns` dependency reaches only the servers that import it — apps/realtime pulls `@sim/security/compare` and nothing else, and the prune graph is unchanged at 14 workspaces. Two lookups deliberately stay as they are: `createSsrfGuardedLookup` is a socket-connect `LookupFunction` that needs raw entries with their family and already validates every address, and desktop's per-host verdict cache keeps its own loopback policy on top of the shared resolver. The localhost carve-out is tightened as a consequence: it applies only when every record is loopback, so `localhost` that also resolves to the LAN no longer rides it.
1 parent c18f4d3 commit 042db18

9 files changed

Lines changed: 283 additions & 74 deletions

File tree

apps/desktop/src/main/browser-agent/url-guard.test.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'
22

33
const { mockLookup } = vi.hoisted(() => ({ mockLookup: vi.fn() }))
44

5+
// The real resolveHostAddresses runs; only the resolver under it is mocked, so
6+
// its deadline and all-addresses behaviour stay covered here.
57
vi.mock('node:dns/promises', () => ({
68
default: { lookup: mockLookup },
79
}))

apps/desktop/src/main/browser-agent/url-guard.ts

Lines changed: 9 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1-
import dns from 'node:dns/promises'
21
import { createLogger } from '@sim/logger'
2+
import { resolveHostAddresses } from '@sim/security/dns'
33
import {
44
isIpLiteral,
55
isLoopbackIp,
@@ -12,31 +12,6 @@ import { parseHttpUrl } from '@/main/navigation'
1212

1313
const logger = createLogger('BrowserAgentUrlGuard')
1414

15-
/** Hard deadline on the SSRF DNS lookup so a slow/hung resolver can't suspend
16-
* the check — and the onBeforeRequest callback that awaits it — indefinitely.
17-
* A timeout rejects, which fails closed (blocks) via the caller's catch. */
18-
const DNS_TIMEOUT_MS = 5_000
19-
20-
/** dns.lookup bounded by {@link DNS_TIMEOUT_MS}; the timer is always cleared so a
21-
* won race never leaves a dangling rejection. */
22-
async function resolveHost(host: string) {
23-
const lookup = dns.lookup(host, { all: true, verbatim: true })
24-
// If the timeout wins the race the lookup stays pending; swallow its eventual
25-
// settlement so a late rejection can't surface as an unhandled rejection.
26-
lookup.catch(() => {})
27-
let timer: NodeJS.Timeout | undefined
28-
try {
29-
return await Promise.race([
30-
lookup,
31-
new Promise<never>((_, reject) => {
32-
timer = setTimeout(() => reject(new Error('DNS lookup timed out')), DNS_TIMEOUT_MS)
33-
}),
34-
])
35-
} finally {
36-
clearTimeout(timer)
37-
}
38-
}
39-
4015
export interface UrlGuardResult {
4116
ok: boolean
4217
error?: string
@@ -103,8 +78,8 @@ export async function checkAgentUrl(rawUrl: string): Promise<UrlGuardResult> {
10378
}
10479

10580
try {
106-
const resolved = await resolveHost(host)
107-
if (resolved.some(({ address }) => isBlockedAddress(address))) {
81+
const { addresses } = await resolveHostAddresses(host)
82+
if (addresses.some((address) => isBlockedAddress(address))) {
10883
logger.warn('Blocked agent navigation resolving to private IP', { host })
10984
return BLOCKED
11085
}
@@ -163,9 +138,9 @@ const MAX_HOST_VERDICTS = 256
163138
* request that arrived before the first lookup settled to start its own, and
164139
* `dns.lookup` is `getaddrinfo` on the libuv threadpool — four slots by
165140
* default, shared with every `fs` call in the main process. A page naming a few
166-
* hundred hostnames could then queue hundreds of blocking jobs, each up to
167-
* {@link DNS_TIMEOUT_MS}, and stall unrelated work like the settings write or
168-
* the credential vault.
141+
* hundred hostnames could then queue hundreds of blocking jobs, each up to the
142+
* resolver deadline, and stall unrelated work like the settings write or the
143+
* credential vault.
169144
*/
170145
const hostVerdicts = new Map<string, { verdict: Promise<boolean>; expiry: number }>()
171146

@@ -227,9 +202,9 @@ export async function isBlockedSubresourceUrl(rawUrl: string): Promise<boolean>
227202
const cached = hostVerdicts.get(host)
228203
if (cached && Date.now() < cached.expiry) return cached.verdict
229204

230-
const verdict = resolveHost(host)
231-
.then((resolved) => {
232-
const blocked = resolved.some(({ address }) => isBlockedAddress(address))
205+
const verdict = resolveHostAddresses(host)
206+
.then(({ addresses }) => {
207+
const blocked = addresses.some((address) => isBlockedAddress(address))
233208
if (blocked) {
234209
logger.warn('Blocked agent subresource resolving to private IP', { host })
235210
}

apps/sim/app/api/tools/onepassword/utils.ts

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
import dns from 'dns/promises'
21
import type {
32
FileAttributes,
43
Item,
@@ -12,6 +11,7 @@ import type {
1211
Website,
1312
} from '@1password/sdk'
1413
import { createLogger } from '@sim/logger'
14+
import { resolveHostAddresses } from '@sim/security/dns'
1515
import { isPrivateIp, unwrapIpv6Brackets } from '@sim/security/ssrf'
1616
import { toError } from '@sim/utils/errors'
1717
import { generateId } from '@sim/utils/id'
@@ -312,12 +312,12 @@ export async function validateConnectServerUrl(serverUrl: string): Promise<strin
312312
return clean
313313
}
314314

315+
let addresses: string[]
315316
let address: string
316317
try {
317-
// Prefer IPv4: pinning strips Happy Eyeballs' fallback, and a pinned IPv6 address hangs
318-
// on IPv4-only egress (e.g. AWS NAT gateways).
319-
const resolved = await dns.lookup(clean, { all: true, verbatim: true })
320-
address = (resolved.find((entry) => entry.family === 4) ?? resolved[0]).address
318+
const resolved = await resolveHostAddresses(clean)
319+
addresses = resolved.addresses
320+
address = resolved.preferred
321321
} catch (error) {
322322
connectLogger.warn('DNS lookup failed for 1Password Connect server URL', {
323323
hostname: clean,
@@ -326,7 +326,11 @@ export async function validateConnectServerUrl(serverUrl: string): Promise<strin
326326
throw new Error('1Password server URL hostname could not be resolved')
327327
}
328328

329-
assertConnectIpAllowed(address, clean)
329+
// Asserted on every address, pinned on the IPv4-preferred one: a host with both
330+
// a public and a private record must not pass on record order alone.
331+
for (const candidate of addresses) {
332+
assertConnectIpAllowed(candidate, clean)
333+
}
330334
return address
331335
}
332336

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { beforeEach, describe, expect, it, vi } from 'vitest'
5+
6+
const { mockResolve } = vi.hoisted(() => ({ mockResolve: vi.fn() }))
7+
8+
vi.mock('@sim/security/dns', () => ({
9+
resolveHostAddresses: mockResolve,
10+
}))
11+
12+
vi.mock('@/lib/core/config/env-flags', () => ({
13+
isHosted: false,
14+
isPrivateDatabaseHostsAllowed: false,
15+
getProxyUrl: () => undefined,
16+
}))
17+
18+
import { validateUrlWithDNS } from '@/lib/core/security/input-validation.server'
19+
20+
/** Shapes a resolver answer the way `resolveHostAddresses` does. */
21+
function resolved(addresses: string[]) {
22+
return { addresses, preferred: addresses[0] }
23+
}
24+
25+
describe('validateUrlWithDNS address classification', () => {
26+
beforeEach(() => {
27+
vi.clearAllMocks()
28+
})
29+
30+
it('rejects a host that also publishes a private record', async () => {
31+
// The gap this closes: only one address used to be classified, so a host
32+
// publishing both got through whenever the public record sorted first.
33+
mockResolve.mockResolvedValue(resolved(['93.184.216.34', '10.0.0.5']))
34+
35+
const result = await validateUrlWithDNS('https://mixed.example/api')
36+
37+
expect(result.isValid).toBe(false)
38+
expect(result.error).toContain('blocked IP address')
39+
})
40+
41+
it('rejects it regardless of which record comes first', async () => {
42+
mockResolve.mockResolvedValue(resolved(['10.0.0.5', '93.184.216.34']))
43+
44+
expect((await validateUrlWithDNS('https://mixed.example/api')).isValid).toBe(false)
45+
})
46+
47+
it('accepts a host whose every record is public, pinning the preferred one', async () => {
48+
mockResolve.mockResolvedValue(resolved(['93.184.216.34', '93.184.216.35']))
49+
50+
const result = await validateUrlWithDNS('https://example.com/api')
51+
52+
expect(result.isValid).toBe(true)
53+
expect(result.resolvedIP).toBe('93.184.216.34')
54+
})
55+
56+
it('keeps the self-hosted localhost carve-out when every record is loopback', async () => {
57+
mockResolve.mockResolvedValue(resolved(['127.0.0.1', '::1']))
58+
59+
expect((await validateUrlWithDNS('https://localhost/api')).isValid).toBe(true)
60+
})
61+
62+
it('denies localhost the carve-out when it also resolves off-loopback', async () => {
63+
// `localhost` pointing at the LAN as well is not the case the carve-out was
64+
// written for, and riding it there would reach another machine.
65+
mockResolve.mockResolvedValue(resolved(['127.0.0.1', '10.0.0.5']))
66+
67+
expect((await validateUrlWithDNS('https://localhost/api')).isValid).toBe(false)
68+
})
69+
70+
it('reports an unresolvable host rather than treating it as public', async () => {
71+
mockResolve.mockRejectedValue(new Error('ENOTFOUND'))
72+
73+
expect((await validateUrlWithDNS('https://missing.example/api')).isValid).toBe(false)
74+
})
75+
})

apps/sim/lib/core/security/input-validation.server.ts

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
1+
import dns from 'node:dns/promises'
12
import { Readable } from 'node:stream'
23
import zlib from 'node:zlib'
3-
import dns from 'dns/promises'
44
import http from 'http'
55
import https from 'https'
66
import type { LookupFunction } from 'net'
77
import { createLogger } from '@sim/logger'
8+
import { resolveHostAddresses } from '@sim/security/dns'
89
import { isLoopbackIp, isPrivateIp, isPrivateIpHost, unwrapIpv6Brackets } from '@sim/security/ssrf'
910
import { toError } from '@sim/utils/errors'
1011
import { omit } from '@sim/utils/object'
@@ -65,19 +66,23 @@ export async function validateUrlWithDNS(
6566
const isLocalhost = cleanHostname === 'localhost' || isLoopbackIp(cleanHostname)
6667

6768
try {
68-
// Prefer IPv4: pinning strips Happy Eyeballs' fallback, and a pinned IPv6 address hangs
69-
// on IPv4-only egress (e.g. AWS NAT gateways) — still-pinned consumers (providers, SSO,
70-
// A2A) depend on this ordering.
71-
const resolved = await dns.lookup(cleanHostname, { all: true, verbatim: true })
72-
const { address } = resolved.find((entry) => entry.family === 4) ?? resolved[0]
73-
74-
const resolvedIsLoopback = isLoopbackIp(address)
75-
76-
if (isPrivateIp(address) && !(isLocalhost && resolvedIsLoopback && !isHosted)) {
69+
// Every address is classified, but only the IPv4-preferred one is returned to
70+
// pin: checking a single address let a host publishing both a public and a
71+
// private record through whenever the public one sorted first, which is
72+
// record order rather than policy.
73+
const { addresses, preferred } = await resolveHostAddresses(cleanHostname)
74+
const blocked = addresses.filter((address) => isPrivateIp(address))
75+
76+
// The localhost carve-out still applies only when every address is loopback,
77+
// so `localhost` with an extra RFC1918 record does not ride it.
78+
if (
79+
blocked.length > 0 &&
80+
!(isLocalhost && !isHosted && blocked.every((address) => isLoopbackIp(address)))
81+
) {
7782
logger.warn('URL resolves to blocked IP address', {
7883
paramName,
7984
hostname,
80-
resolvedIP: address,
85+
resolvedIP: blocked[0],
8186
})
8287
return {
8388
isValid: false,
@@ -87,7 +92,7 @@ export async function validateUrlWithDNS(
8792

8893
return {
8994
isValid: true,
90-
resolvedIP: address,
95+
resolvedIP: preferred,
9196
originalHostname: hostname,
9297
}
9398
} catch (error) {
@@ -206,16 +211,15 @@ export async function validateDatabaseHost(
206211
}
207212

208213
try {
209-
// Prefer IPv4: pinning strips Happy Eyeballs' fallback, and a pinned IPv6 address hangs
210-
// on IPv4-only egress (e.g. AWS NAT gateways).
211-
const resolved = await dns.lookup(cleanHost, { all: true, verbatim: true })
212-
const { address } = resolved.find((entry) => entry.family === 4) ?? resolved[0]
214+
const { addresses, preferred } = await resolveHostAddresses(cleanHost)
215+
const address = preferred
216+
const blockedAddress = addresses.find((candidate) => isPrivateIp(candidate))
213217

214-
if (isPrivateIp(address) && !isPrivateDatabaseHostsAllowed) {
218+
if (blockedAddress !== undefined && !isPrivateDatabaseHostsAllowed) {
215219
logger.warn('Database host resolves to blocked IP address', {
216220
paramName,
217221
hostname: host,
218-
resolvedIP: address,
222+
resolvedIP: blockedAddress,
219223
})
220224
return {
221225
isValid: false,

apps/sim/lib/mcp/domain-check.ts

Lines changed: 21 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1-
import dns from 'dns/promises'
21
import { createLogger } from '@sim/logger'
2+
import { resolveHostAddresses } from '@sim/security/dns'
33
import { isIpLiteral, isLoopbackIp, isPrivateIp, unwrapIpv6Brackets } from '@sim/security/ssrf'
44
import { toError } from '@sim/utils/errors'
55
import { getAllowedMcpDomainsFromEnv, isHosted } from '@/lib/core/config/env-flags'
@@ -172,12 +172,12 @@ export async function validateMcpServerSsrf(url: string | undefined): Promise<st
172172
return cleanHostname
173173
}
174174

175+
let addresses: string[]
175176
let address: string
176177
try {
177-
// Prefer IPv4: pinning strips Happy Eyeballs' fallback, and a pinned IPv6 address
178-
// (which `verbatim` returns first for dual-stack hosts) hangs on IPv4-only egress.
179-
const resolved = await dns.lookup(cleanHostname, { all: true, verbatim: true })
180-
address = (resolved.find((entry) => entry.family === 4) ?? resolved[0]).address
178+
const resolved = await resolveHostAddresses(cleanHostname)
179+
addresses = resolved.addresses
180+
address = resolved.preferred
181181
} catch (error) {
182182
logger.warn('DNS lookup failed for MCP server URL', {
183183
hostname,
@@ -186,20 +186,25 @@ export async function validateMcpServerSsrf(url: string | undefined): Promise<st
186186
throw new McpDnsResolutionError(cleanHostname)
187187
}
188188

189-
if (isLoopbackIp(address)) {
190-
if (isHosted) {
191-
logger.warn('MCP server URL resolves to loopback address', {
189+
// Every address is judged, not just the pinned one: a host publishing both a
190+
// public and a private record would otherwise pass on record order alone. The
191+
// pin stays on the IPv4-preferred address, which is what callers connect to.
192+
for (const candidate of addresses) {
193+
if (isLoopbackIp(candidate)) {
194+
if (isHosted) {
195+
logger.warn('MCP server URL resolves to loopback address', {
196+
hostname,
197+
resolvedIP: candidate,
198+
})
199+
throw new McpSsrfError('MCP server URL resolves to a loopback address')
200+
}
201+
} else if (isPrivateIp(candidate)) {
202+
logger.warn('MCP server URL resolves to blocked IP address', {
192203
hostname,
193-
resolvedIP: address,
204+
resolvedIP: candidate,
194205
})
195-
throw new McpSsrfError('MCP server URL resolves to a loopback address')
206+
throw new McpSsrfError('MCP server URL resolves to a blocked IP address')
196207
}
197-
} else if (isPrivateIp(address)) {
198-
logger.warn('MCP server URL resolves to blocked IP address', {
199-
hostname,
200-
resolvedIP: address,
201-
})
202-
throw new McpSsrfError('MCP server URL resolves to a blocked IP address')
203208
}
204209

205210
return address

packages/security/package.json

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,10 @@
1414
"types": "./src/compare.ts",
1515
"default": "./src/compare.ts"
1616
},
17+
"./dns": {
18+
"types": "./src/dns.ts",
19+
"default": "./src/dns.ts"
20+
},
1721
"./encryption": {
1822
"types": "./src/encryption.ts",
1923
"default": "./src/encryption.ts"

0 commit comments

Comments
 (0)