fix(security): filter refused DNS records instead of failing the host

validateUrlWithDNS rejected a host outright when any resolved address was
private, while createSsrfGuardedLookup in the same file filters the private
entries and connects to what remains. Filtering is equally safe — you pin a
surviving public address — and rejecting broke a split-horizon resolver that
answers with a private record alongside the public one, on a path with ~70 call
sites and no operator opt-out. The pin is re-preferred over the surviving set so
it can never be an address the filter just refused.

Also from the review round: the browser preload's isPasswordField and
findIdentifierField now split autocomplete tokens like the agent guards they
claim parity with (a WebAuthn `current-password webauthn` field was invisible to
credential fill); the subresource verdict cache holds the promise rather than the
boolean, so the requests one page fires at a host share a lookup instead of each
queueing its own getaddrinfo on the four-slot libuv threadpool that main's fs
calls also use; trailing-dot hosts normalize to one cache entry; the resolver
carries a distinct DnsTimeoutError so an outage is not reported as a missing
host; and clearHostVerdictCache is wired into both the profile wipe and the
cache-clear path, since a resolved-host classification is browsing-trail data.

Author: waleedlatif1

apps/desktop/src/main/browser-agent/panel.ts

@@ -333,6 +333,12 @@ function capturePanelSnapshot(onSettled?: () => void): void {
       // picture of the page, so it goes to the window still showing the
       // browser or nowhere at all.
       if (panelWindow() !== win || win.isDestroyed()) return
+      // Downscale and JPEG-encode before crossing IPC. capturePage returns a
+      // device-pixel PNG — on a retina half-window that is millions of pixels,
+      // and toDataURL's PNG encode is synchronous on the main process, so a
+      // full-size encode stalls every window's input for the frame. This is a
+      // placeholder shown under a transient overlay, so a downscaled JPEG is
+      // indistinguishable and an order of magnitude cheaper to encode and send.
       const snapshot: BrowserPanelSnapshot = { dataUrl: encodeSnapshot(image), tabId }
       win.webContents.send('browser-agent:panel-snapshot', snapshot)
     })

apps/desktop/src/main/ipc.test.ts

@@ -1,6 +1,6 @@
 import { readFileSync } from 'node:fs'
 import { fileURLToPath } from 'node:url'
-import { beforeEach, describe, expect, it, vi } from 'vitest'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
 
 vi.mock('electron', () => import('@/test/electron-mock'))
 
@@ -170,6 +170,10 @@ describe('registerIpcHandlers', () => {
   let deps: IpcDeps
 
   beforeEach(() => {
+    // Frozen so the input-recency windows cannot lapse mid-test: the gates read
+    // wall-clock, and a loaded machine pausing between this press and an
+    // assertion would flip them closed for reasons unrelated to the test.
+    vi.useFakeTimers()
     activeSender.press()
     activeChooserSender.press()
     vi.mocked(ipcMain.handle).mockClear()
@@ -223,6 +227,10 @@ describe('registerIpcHandlers', () => {
     registerIpcHandlers(deps)
   })
 
+  afterEach(() => {
+    vi.useRealTimers()
+  })
+
   it('validates open-external URLs regardless of sender', async () => {
     const { invoke } = collectHandlers()
     expect(await invoke.get('desktop:open-external')?.(evilEvent, 'https://docs.sim.ai')).toBe(true)

apps/desktop/src/main/updater.ts

@@ -21,41 +21,40 @@ export type UpdateChannel = 'latest' | 'beta' | 'alpha'
  * staging beta, prod stable — so the environment, not the client, is the
  * channel. Returns null for origins that can't host a feed.
  */
+export function feedUrlForOrigin(origin: string): string | null {
+  try {
+    const url = new URL(origin)
+    if (url.protocol !== 'https:' && url.protocol !== 'http:') {
+      return null
+    }
+    return `${url.origin}/api/desktop/update`
+  } catch {
+    return null
+  }
+}
+
 /**
  * Where the feed rewrites every manifest entry to. Downloads are constrained to
  * this prefix rather than to https alone, so a feed that serves an attacker's
  * host cannot get a bundle in front of the user's Download button.
  */
-const RELEASE_ASSET_PREFIX = 'https://github.com/simstudioai/sim/releases/download/'
+const RELEASE_ASSET_ORIGIN = 'https://github.com'
+const RELEASE_ASSET_PATH = '/simstudioai/sim/releases/download/'
 
 /** Whether a manifest url is one of our own release assets. */
-export function isReleaseAssetUrl(rawUrl: string): boolean {
+function isReleaseAssetUrl(rawUrl: string): boolean {
   if (!isSafeExternalUrl(rawUrl)) return false
   try {
     const url = new URL(rawUrl)
-    // Compared on the parsed origin, never by prefix on the raw string, so
-    // `https://github.com.evil.example/…` cannot pass.
-    return (
-      url.origin === 'https://github.com' &&
-      url.pathname.startsWith(new URL(RELEASE_ASSET_PREFIX).pathname)
-    )
+    // Compared on the parsed origin and the parsed pathname, never by prefix on
+    // the raw string: `https://github.com.evil.example/…` must not pass, and
+    // `URL` has already normalized away any `..` segments by this point.
+    return url.origin === RELEASE_ASSET_ORIGIN && url.pathname.startsWith(RELEASE_ASSET_PATH)
   } catch {
     return false
   }
 }
 
-export function feedUrlForOrigin(origin: string): string | null {
-  try {
-    const url = new URL(origin)
-    if (url.protocol !== 'https:' && url.protocol !== 'http:') {
-      return null
-    }
-    return `${url.origin}/api/desktop/update`
-  } catch {
-    return null
-  }
-}
-
 /**
  * Maps the running version to its update channel: prerelease builds follow
  * their prerelease channel, stable builds only ever see stable releases.
@@ -465,7 +464,7 @@ export function initUpdater(deps: UpdaterDeps): UpdaterHandle {
         // an available update.
         //
         // Constrained to the release host, not merely to https. The feed rewrites
-        // every entry to an absolute `${RELEASE_ASSET_PREFIX}` URL, so nothing
+        // every entry to an absolute release-asset URL, so nothing
         // legitimate is excluded — while scheme-only validation would still admit
         // `https://attacker.example/Sim.dmg`, and the download dialog walks the
         // user through installing whatever it hands to the browser. That is a

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

@@ -7,6 +7,11 @@ const { mockResolve } = vi.hoisted(() => ({ mockResolve: vi.fn() }))
 
 vi.mock('@sim/security/dns', () => ({
   resolveHostAddresses: mockResolve,
+  // Mirrors the real rule so a pin taken over a narrowed set behaves as it does
+  // in production; a stub returning addresses[0] would hide the divergence the
+  // preference exists for.
+  preferIpv4: (addresses: string[]) =>
+    addresses.find((address) => address.includes('.')) ?? addresses[0],
 }))
 
 vi.mock('@/lib/core/config/env-flags', () => ({
@@ -17,31 +22,50 @@ vi.mock('@/lib/core/config/env-flags', () => ({
 
 import { validateUrlWithDNS } from '@/lib/core/security/input-validation.server'
 
-/** Shapes a resolver answer the way `resolveHostAddresses` does. */
+/**
+ * Shapes a resolver answer the way `resolveHostAddresses` does, including its
+ * IPv4-first preference — so `preferred` can differ from `addresses[0]`, which
+ * is the whole reason the field exists.
+ */
 function resolved(addresses: string[]) {
-  return { addresses, preferred: addresses[0] }
+  const preferred = addresses.find((address) => address.includes('.')) ?? addresses[0]
+  return { addresses, preferred }
 }
 
 describe('validateUrlWithDNS address classification', () => {
   beforeEach(() => {
     vi.clearAllMocks()
   })
 
-  it('rejects a host that also publishes a private record', async () => {
-    // The gap this closes: only one address used to be classified, so a host
-    // publishing both got through whenever the public record sorted first.
+  it('drops a private co-record and pins the public one', async () => {
+    // The gap this closes: one address used to be classified, so which record
+    // got judged was a matter of resolver order.
     mockResolve.mockResolvedValue(resolved(['93.184.216.34', '10.0.0.5']))
 
     const result = await validateUrlWithDNS('https://mixed.example/api')
 
+    expect(result.isValid).toBe(true)
+    expect(result.resolvedIP).toBe('93.184.216.34')
+  })
+
+  it('rejects when every record is private', async () => {
+    mockResolve.mockResolvedValue(resolved(['10.0.0.5', '192.168.1.9']))
+
+    const result = await validateUrlWithDNS('https://internal.example/api')
+
     expect(result.isValid).toBe(false)
     expect(result.error).toContain('blocked IP address')
   })
 
-  it('rejects it regardless of which record comes first', async () => {
-    mockResolve.mockResolvedValue(resolved(['10.0.0.5', '93.184.216.34']))
+  it('never pins an address the filter refused', async () => {
+    // The private record sorts first AND is the IPv4 one, so a pin taken from
+    // the unfiltered set would land on 10.0.0.5.
+    mockResolve.mockResolvedValue(resolved(['10.0.0.5', '2606:2800:220:1::248']))
+
+    const result = await validateUrlWithDNS('https://mixed.example/api')
 
-    expect((await validateUrlWithDNS('https://mixed.example/api')).isValid).toBe(false)
+    expect(result.isValid).toBe(true)
+    expect(result.resolvedIP).toBe('2606:2800:220:1::248')
   })
 
   it('accepts a host whose every record is public, pinning the preferred one', async () => {
@@ -59,12 +83,15 @@ describe('validateUrlWithDNS address classification', () => {
     expect((await validateUrlWithDNS('https://localhost/api')).isValid).toBe(true)
   })
 
-  it('denies localhost the carve-out when it also resolves off-loopback', async () => {
-    // `localhost` pointing at the LAN as well is not the case the carve-out was
-    // written for, and riding it there would reach another machine.
+  it('drops an off-loopback record from localhost rather than pinning it', async () => {
+    // The carve-out covers loopback only, so the LAN record is filtered out and
+    // the pin stays on the machine the carve-out was written for.
     mockResolve.mockResolvedValue(resolved(['127.0.0.1', '10.0.0.5']))
 
-    expect((await validateUrlWithDNS('https://localhost/api')).isValid).toBe(false)
+    const result = await validateUrlWithDNS('https://localhost/api')
+
+    expect(result.isValid).toBe(true)
+    expect(result.resolvedIP).toBe('127.0.0.1')
   })
 
   it('reports an unresolvable host rather than treating it as public', async () => {

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

@@ -5,7 +5,7 @@ import http from 'http'
 import https from 'https'
 import type { LookupFunction } from 'net'
 import { createLogger } from '@sim/logger'
-import { resolveHostAddresses } from '@sim/security/dns'
+import { preferIpv4, resolveHostAddresses } from '@sim/security/dns'
 import { isLoopbackIp, isPrivateIp, isPrivateIpHost, unwrapIpv6Brackets } from '@sim/security/ssrf'
 import { toError } from '@sim/utils/errors'
 import { omit } from '@sim/utils/object'
@@ -66,23 +66,26 @@ export async function validateUrlWithDNS(
   const isLocalhost = cleanHostname === 'localhost' || isLoopbackIp(cleanHostname)
 
   try {
-    // Every address is classified, but only the IPv4-preferred one is returned to
-    // pin: checking a single address let a host publishing both a public and a
-    // private record through whenever the public one sorted first, which is
-    // record order rather than policy.
-    const { addresses, preferred } = await resolveHostAddresses(cleanHostname)
-    const blocked = addresses.filter((address) => isPrivateIp(address))
-
-    // The localhost carve-out still applies only when every address is loopback,
-    // so `localhost` with an extra RFC1918 record does not ride it.
-    if (
-      blocked.length > 0 &&
-      !(isLocalhost && !isHosted && blocked.every((address) => isLoopbackIp(address)))
-    ) {
+    // Every address is judged, not just the pinned one: classifying a single
+    // address let a host publishing both a public and a private record through
+    // whenever the public one sorted first, which is record order rather than
+    // policy.
+    //
+    // Refused records are filtered rather than failing the whole host, matching
+    // createSsrfGuardedLookup below. Pinning to a surviving public address is
+    // just as safe as refusing outright, and rejecting the host would break a
+    // split-horizon resolver that answers with a private record alongside the
+    // public one — with no operator opt-out on this path.
+    const { addresses } = await resolveHostAddresses(cleanHostname)
+    const usable = addresses.filter(
+      (address) => !isPrivateIp(address) || (isLocalhost && !isHosted && isLoopbackIp(address))
+    )
+
+    if (usable.length === 0) {
       logger.warn('URL resolves to blocked IP address', {
         paramName,
         hostname,
-        resolvedIP: blocked[0],
+        resolvedIP: addresses[0],
       })
       return {
         isValid: false,
@@ -92,7 +95,9 @@ export async function validateUrlWithDNS(
 
     return {
       isValid: true,
-      resolvedIP: preferred,
+      // Re-preferred over the surviving set so the pin is never an address the
+      // filter above just refused.
+      resolvedIP: preferIpv4(usable),
       originalHostname: hostname,
     }
   } catch (error) {
@@ -212,7 +217,6 @@ export async function validateDatabaseHost(
 
   try {
     const { addresses, preferred } = await resolveHostAddresses(cleanHost)
-    const address = preferred
     const blockedAddress = addresses.find((candidate) => isPrivateIp(candidate))
 
     if (blockedAddress !== undefined && !isPrivateDatabaseHostsAllowed) {
@@ -229,7 +233,7 @@ export async function validateDatabaseHost(
 
     return {
       isValid: true,
-      resolvedIP: address,
+      resolvedIP: preferred,
       originalHostname: host,
     }
   } catch (error) {

packages/security/src/dns.test.ts

@@ -6,7 +6,7 @@ vi.mock('node:dns/promises', () => ({
   default: { lookup: mockLookup },
 }))
 
-import { resolveHostAddresses } from './dns'
+import { DnsTimeoutError, resolveHostAddresses } from './dns'
 
 describe('resolveHostAddresses', () => {
   beforeEach(() => {
@@ -56,6 +56,51 @@ describe('resolveHostAddresses', () => {
     await expect(resolveHostAddresses('missing.example')).rejects.toThrow('ENOTFOUND')
   })
 
+  it('clears the deadline timer once the lookup succeeds', async () => {
+    // A leaked timer holds the event loop open for the full window and is
+    // invisible to every other assertion here.
+    vi.useFakeTimers()
+    try {
+      mockLookup.mockResolvedValue([{ address: '93.184.216.34', family: 4 }])
+
+      await resolveHostAddresses('example.com')
+
+      expect(vi.getTimerCount()).toBe(0)
+    } finally {
+      vi.useRealTimers()
+    }
+  })
+
+  it('swallows a lookup that rejects after the deadline already fired', async () => {
+    // Without the pre-race `.catch`, the loser of the race surfaces as an
+    // unhandled rejection well after the caller has moved on.
+    vi.useFakeTimers()
+    const unhandled = vi.fn()
+    process.on('unhandledRejection', unhandled)
+    try {
+      let failLookup: (error: Error) => void = () => {}
+      mockLookup.mockReturnValue(
+        new Promise((_resolve, reject) => {
+          failLookup = reject
+        })
+      )
+
+      const pending = resolveHostAddresses('slow.example', { timeoutMs: 1_000 })
+      const assertion = expect(pending).rejects.toThrow('timed out')
+      await vi.advanceTimersByTimeAsync(1_000)
+      await assertion
+
+      failLookup(new Error('ENOTFOUND'))
+      await vi.advanceTimersByTimeAsync(0)
+      await Promise.resolve()
+
+      expect(unhandled).not.toHaveBeenCalled()
+    } finally {
+      process.off('unhandledRejection', unhandled)
+      vi.useRealTimers()
+    }
+  })
+
   it('rejects on the deadline instead of waiting for a hung resolver', async () => {
     vi.useFakeTimers()
     try {
@@ -69,4 +114,18 @@ describe('resolveHostAddresses', () => {
       vi.useRealTimers()
     }
   })
+
+  it('reports a deadline distinctly from a missing host', async () => {
+    vi.useFakeTimers()
+    try {
+      mockLookup.mockReturnValue(new Promise(() => {}))
+
+      const pending = resolveHostAddresses('slow.example', { timeoutMs: 1_000 })
+      const assertion = expect(pending).rejects.toBeInstanceOf(DnsTimeoutError)
+      await vi.advanceTimersByTimeAsync(1_000)
+      await assertion
+    } finally {
+      vi.useRealTimers()
+    }
+  })
 })