feat(desktop): SSRF hardening for the browser agent + review fixes (#5763)

* feat(desktop): SSRF hardening for the browser agent + review fixes

Shared SSRF primitive:
- Extract IP classification into @sim/security/ssrf (isPrivateIp,
  isPrivateIpHost, isIpLiteral, unwrapIpv6Brackets). Collapses the two
  divergent isPrivateOrReservedIP copies in apps/sim and the ad-hoc regex
  copies in sap_concur/zoominfo/sap contracts into one audited, ipaddr.js-based
  implementation consumed by both apps. All callers migrated; no logic left
  behind.

Browser-agent SSRF guard (apps/desktop):
- The agent partition's onBeforeRequest is now the authoritative choke point:
  document navigations (top-level + iframes) get a full DNS-resolving check —
  covering page-initiated navigation the driver never sees (redirects, link
  clicks, location.href, meta-refresh) — and subresources get a cheap
  synchronous literal-IP backstop.
- browser_navigate / browser_open_tab validate up front so the model gets a
  clean error instead of a silent load failure.
- Blocks loopback, RFC1918, link-local/cloud-metadata (169.254.169.254), and
  obfuscated/IPv4-mapped forms; hostnames are resolved (DNS-rebinding aware).

Other hardening:
- crashReporter: local-only native minidumps (no backend, uploadToServer:false),
  crash-dump dir recorded in the event log.
- CSP fallback: a minimal, non-drifting policy injected only when an app-origin
  document response ships no CSP of its own.
- window.open (MCP OAuth) gated to https so a renderer-controlled frame name
  can't ride an http URL into an in-app window.
- updater: shared configureAutoUpdater re-asserts channel/allowDowngrade on both
  the launch and manual-check paths.
- handoff: the loopback listener now tears down only after the CSRF state
  validates, not on any format-valid request (self-DoS guard).
- cdp: per-WebContents callbacks so a background tab's events reach its own
  driver.
- config: drop the unreachable bare '::1' loopback entry.
- README: correct the App Sandbox / security-scoped-bookmark note for the
  Developer ID build.

Tests: @sim/security/ssrf (67), desktop suite (200), affected apps/sim suites
green; type-check + biome clean across desktop, security, and touched sim files.

* fix(desktop): address review findings (handoff callback gate, fail-closed guards)

- handoff: gate onCallback on a non-consuming state match so a format-valid but
  wrong/expired state no longer fires the callback (which surfaced a spurious
  "sign-in failed" UI while the genuine callback was still pending). The
  one-shot listener still survives wrong-state requests (self-DoS guard);
  consume() re-validates and tears it down on the real callback.
- session: the agent-partition onBeforeRequest SSRF check now fails closed —
  an unexpected rejection cancels the request instead of leaving it suspended.
- updater: the manual checkForUpdates() now has a .catch() that logs and shows
  an error dialog instead of silently swallowing network/manifest failures.

* refactor: desktop cleanup, shared loopback helpers, api-validation reconcile

Desktop reuse / dead-code (whole-app audit):
- driver.ts: use shared `sleep` (@sim/utils/helpers) and `getErrorMessage`
  (@sim/utils/errors) instead of local reimplementations; drop 7 banner
  separators.
- Remove dead code: LocalFilesystemService.clear(), launcher isVisible(),
  the unused `url` arg on onDownloadBlocked, and decideStartRoute's always-
  'unknown' sessionState param + its dead branch (no launch-time probe by
  design — see README).
- Extract the duplicated launcher load-failure tail into failLaunchLoad().
- handoff.ts: promote a loose comment to TSDoc.

Shared loopback helpers (@sim/security/ssrf):
- Add isLoopbackIp (full 127/8 + ::1 range) and isLoopbackHostname /
  LOOPBACK_HOSTNAMES (exact-set), consolidating three duplicated copies:
  mcp/domain-check.ts (now uses isLoopbackIp + isIpLiteral + unwrapIpv6Brackets,
  drops its ipaddr import), connectors/s3.ts, and desktop config.ts /
  navigation.ts (drop the local LOCAL_HOSTNAMES set). urls.ts keeps its own
  client-side set to avoid pulling ipaddr.js into client bundles.

api-validation:strict reconcile (pre-existing dev drift, not from these changes):
- Allowlist speech/tts (raw-read streaming route that validates inline) in
  INDIRECT_ZOD_ROUTES; annotate the launcher's validated-envelope double-cast.
  Bump the route-count baseline 964→966 for dev's two already-merged speech
  routes. Non-Zod and double-cast ratchets stay at baseline.

* fix(desktop): fail closed on DNS resolution failure in the agent SSRF guard

checkAgentUrl now blocks when dns.lookup throws instead of allowing the load.
Chromium resolves DNS independently of our Node lookup, so a host our resolver
can't resolve could still reach a private address via Chromium's resolver —
allowing it was a fail-open gap. Matches validateUrlWithDNS in apps/sim.

Author: waleedlatif1

apps/desktop/README.md

@@ -137,7 +137,7 @@ Good fits for the bridge: OS notifications + dock badge on workflow completion,
 The main Copilot agent can inspect a user-selected local directory in the desktop app through request-local client tools (`local_mount_directory`, `local_list_mounts`, `local_list`, `local_glob`, `local_read`, `local_grep`, `local_stat`, and `local_forget_mount`). This capability is:
 
 - **Explicit and read-only:** the native folder picker creates the grant; there are no write/delete/execute operations.
-- **Remembered securely:** grants are encrypted in Electron's private app data with OS-backed `safeStorage` and restored with the same opaque URI after a normal app restart. Sandboxed macOS builds also retain the security-scoped bookmark. There is no plaintext fallback: when secure storage is unavailable, the returned mount has `remembered: false` and lasts only for that app session.
+- **Remembered securely:** grants are encrypted in Electron's private app data with OS-backed `safeStorage` and restored with the same opaque URI after a normal app restart. (A security-scoped bookmark is stored alongside each grant, but it is a no-op in the current Developer ID build — only the macOS App Sandbox consumes it — and is kept purely for forward-compatibility should a sandboxed/MAS build ever ship.) There is no plaintext fallback: when secure storage is unavailable, the returned mount has `remembered: false` and lasts only for that app session.
 - **Revocable:** `local_forget_mount` removes one grant. All grants are removed on explicit sign-out or server-origin change so another Sim account or server cannot inherit them. Normal app quit only releases active OS handles and keeps the encrypted grants.
 - **Opaque:** renderer and model see only `localfs://<mount-id>/...` URIs. Electron resolves every URI, checks lexical and realpath containment, and refuses symlink escapes.
 - **Desktop-only:** the web app advertises these request-local tools only when `window.simDesktop.localFilesystem` is present. They are available directly to the main agent and are not added to subagent allowlists.

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

@@ -27,7 +27,9 @@ export interface CdpCallbacks {
   onFileChooser: () => void
 }
 
-let callbacks: CdpCallbacks | null = null
+/** Per-tab callbacks, so a background tab's events reach ITS driver, not the
+ * most-recently-instrumented tab's. */
+const callbacksByContents = new WeakMap<WebContents, CdpCallbacks>()
 /** Contents already instrumented (attach survives for the tab's lifetime). */
 const instrumented = new WeakSet<WebContents>()
 
@@ -41,7 +43,7 @@ async function send<T = unknown>(
 
 /** Idempotently instruments a tab's WebContents. */
 export async function ensureInstrumented(contents: WebContents, cb: CdpCallbacks): Promise<void> {
-  callbacks = cb
+  callbacksByContents.set(contents, cb)
   if (instrumented.has(contents) && contents.debugger.isAttached()) return
 
   if (!contents.debugger.isAttached()) {
@@ -65,6 +67,7 @@ function handleDebuggerEvent(
   method: string,
   params: Record<string, unknown>
 ): void {
+  const callbacks = callbacksByContents.get(contents)
   if (method === 'Page.javascriptDialogOpening') {
     const type = String(params.type ?? 'dialog')
     const message = String(params.message ?? '').slice(0, 500)

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

@@ -22,6 +22,8 @@ import type {
   BrowserToolName,
 } from '@sim/browser-protocol'
 import { createLogger } from '@sim/logger'
+import { getErrorMessage } from '@sim/utils/errors'
+import { sleep } from '@sim/utils/helpers'
 import type { BrowserWindow, WebContents } from 'electron'
 import * as cdp from '@/main/browser-agent/cdp'
 import {
@@ -39,6 +41,7 @@ import {
   typeIntoElement,
 } from '@/main/browser-agent/page-functions'
 import * as session from '@/main/browser-agent/session'
+import { checkAgentUrl } from '@/main/browser-agent/url-guard'
 
 const logger = createLogger('BrowserAgentDriver')
 
@@ -118,7 +121,7 @@ function instrumentTab(contents: WebContents): void {
     })
     .catch((error) => {
       logger.warn('CDP instrumentation failed', {
-        error: error instanceof Error ? error.message : String(error),
+        error: getErrorMessage(error),
       })
     })
   for (const event of [
@@ -160,10 +163,6 @@ export function setPanelBounds(bounds: BrowserPanelBounds | null): void {
   session.setPanelBounds(bounds)
 }
 
-function sleep(ms: number): Promise<void> {
-  return new Promise((resolve) => setTimeout(resolve, ms))
-}
-
 function str(params: Record<string, unknown>, key: string): string | undefined {
   const value = params[key]
   return typeof value === 'string' && value.length > 0 ? value : undefined
@@ -191,10 +190,6 @@ function requireNum(params: Record<string, unknown>, key: string): number {
   return value
 }
 
-// ---------------------------------------------------------------------------
-// Page-function execution
-// ---------------------------------------------------------------------------
-
 /**
  * Serializes a self-contained page function and executes it in the page's
  * main world with JSON-encoded arguments (Electron's executeJavaScript has no
@@ -209,7 +204,7 @@ async function execInPage<Args extends unknown[], Result>(
   try {
     return (await contents.executeJavaScript(expression, true)) as Result
   } catch (error) {
-    const message = error instanceof Error ? error.message : String(error)
+    const message = getErrorMessage(error)
     throw new ToolError(
       `Cannot act on this page (${message}). Browser-internal pages cannot be automated — ` +
         'navigate to a regular website first.'
@@ -249,10 +244,6 @@ function unwrapPageResult(result: unknown): unknown {
   return result
 }
 
-// ---------------------------------------------------------------------------
-// Navigation
-// ---------------------------------------------------------------------------
-
 function waitForLoadComplete(contents: WebContents, timeoutMs: number): Promise<void> {
   return new Promise((resolve) => {
     let settled = false
@@ -278,10 +269,6 @@ async function navigationResult(contents: WebContents): Promise<Record<string, u
   return { url: contents.getURL(), title: contents.getTitle() }
 }
 
-// ---------------------------------------------------------------------------
-// Key parsing for browser_press_key
-// ---------------------------------------------------------------------------
-
 interface KeyDescriptor {
   key: string
   code: string
@@ -349,10 +336,6 @@ export function parseKeyCombo(combo: string): ParsedCombo {
   throw new ToolError(`Unrecognized key: "${keyPart}"`)
 }
 
-// ---------------------------------------------------------------------------
-// Trusted key dispatch (CDP Input.dispatchKeyEvent)
-// ---------------------------------------------------------------------------
-
 /** CDP `Input` modifier bitmask: Alt=1, Ctrl=2, Meta=4, Shift=8. */
 function cdpModifiers(combo: ParsedCombo): number {
   return (combo.alt ? 1 : 0) | (combo.ctrl ? 2 : 0) | (combo.meta ? 4 : 0) | (combo.shift ? 8 : 0)
@@ -444,10 +427,6 @@ async function activeElementState(contents: WebContents): Promise<Record<string,
   return typeof state === 'object' && state !== null ? (state as Record<string, unknown>) : {}
 }
 
-// ---------------------------------------------------------------------------
-// Takeover
-// ---------------------------------------------------------------------------
-
 /**
  * Hands control to the user: the page is already natively interactive in the
  * panel, and the chat's takeover tool row shows the reason with a Done chip.
@@ -481,19 +460,16 @@ async function runTakeover(): Promise<unknown> {
   }
 }
 
-// ---------------------------------------------------------------------------
-// Tool implementations
-// ---------------------------------------------------------------------------
-
 async function executeToolInner(
   tool: BrowserToolName,
   params: Record<string, unknown>
 ): Promise<unknown> {
   switch (tool) {
     case 'browser_navigate': {
       const url = requireStr(params, 'url')
-      if (!/^https?:\/\//i.test(url)) {
-        throw new ToolError('URL must be absolute and start with http:// or https://')
+      const guard = await checkAgentUrl(url)
+      if (!guard.ok) {
+        throw new ToolError(guard.error ?? 'That address was blocked.')
       }
       const tab = session.ensureTab()
       const contents = tab.view.webContents
@@ -521,12 +497,15 @@ async function executeToolInner(
 
     case 'browser_open_tab': {
       const url = str(params, 'url')
+      if (url) {
+        const guard = await checkAgentUrl(url)
+        if (!guard.ok) {
+          throw new ToolError(guard.error ?? 'That address was blocked.')
+        }
+      }
       const tab = session.addTab()
       const contents = tab.view.webContents
       if (url) {
-        if (!/^https?:\/\//i.test(url)) {
-          throw new ToolError('URL must be absolute and start with http:// or https://')
-        }
         void contents.loadURL(url).catch(() => {})
         const result = await navigationResult(contents)
         return { tabId: tab.id, ...result }
@@ -723,10 +702,6 @@ function withNotices(result: unknown): unknown {
   return { value: result, notices }
 }
 
-// ---------------------------------------------------------------------------
-// Public entry points
-// ---------------------------------------------------------------------------
-
 /** One real browser can only do one thing at a time — serialize tool calls. */
 let toolQueue: Promise<unknown> = Promise.resolve()
 
@@ -756,7 +731,7 @@ export async function executeTool(
   try {
     return { ok: true, result: await settled }
   } catch (error) {
-    const message = error instanceof Error ? error.message : String(error)
+    const message = getErrorMessage(error)
     logger.warn('Browser tool failed', { tool, error: message })
     return { ok: false, error: message }
   }

apps/desktop/src/main/browser-agent/session.test.ts

@@ -2,6 +2,13 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'
 
 vi.mock('electron', () => import('@/test/electron-mock'))
 
+vi.mock('@/main/browser-agent/url-guard', () => ({
+  checkAgentUrl: vi.fn(async (url: string) => ({
+    ok: /^https?:\/\//i.test(url) && !url.includes('169.254.169.254'),
+  })),
+  isBlockedRequestUrl: vi.fn((url: string) => url.includes('169.254.169.254')),
+}))
+
 import { BrowserWindow } from 'electron'
 
 type SessionModule = typeof import('@/main/browser-agent/session')
@@ -11,6 +18,7 @@ interface MockView {
     session: {
       setPermissionRequestHandler: ReturnType<typeof vi.fn>
       setPermissionCheckHandler: ReturnType<typeof vi.fn>
+      webRequest: { onBeforeRequest: ReturnType<typeof vi.fn> }
     }
     setWindowOpenHandler: ReturnType<typeof vi.fn>
     loadURL: ReturnType<typeof vi.fn>
@@ -133,6 +141,8 @@ describe('browser-agent session', () => {
     const openHandler = contents.setWindowOpenHandler.mock.calls[0][0] as (details: {
       url: string
     }) => { action: string }
+    // window.open collapses into the same view; the SSRF check runs on the
+    // resulting document request in onBeforeRequest, not here.
     expect(openHandler({ url: 'https://example.com/popup' })).toEqual({ action: 'deny' })
     expect(contents.loadURL).toHaveBeenCalledWith('https://example.com/popup')
     // Non-http(s) popups are denied without navigating anywhere.
@@ -141,6 +151,38 @@ describe('browser-agent session', () => {
     expect(contents.loadURL).not.toHaveBeenCalled()
   })
 
+  it('gates agent-partition requests: DNS-checks documents, literal-checks subresources', async () => {
+    const tab = session.ensureTab()
+    const contents = (tab.view as unknown as MockView).webContents
+    const onBeforeRequest = contents.session.webRequest.onBeforeRequest.mock.calls[0][0] as (
+      details: { url: string; resourceType: string },
+      callback: (response: { cancel?: boolean }) => void
+    ) => void
+
+    // A document navigation to a private host is cancelled by the DNS check.
+    const blockedDoc = vi.fn()
+    onBeforeRequest(
+      { url: 'http://169.254.169.254/latest/meta-data', resourceType: 'mainFrame' },
+      blockedDoc
+    )
+    await vi.waitFor(() => expect(blockedDoc).toHaveBeenCalledWith({ cancel: true }))
+
+    // A document navigation to a public host is allowed.
+    const allowedDoc = vi.fn()
+    onBeforeRequest({ url: 'https://example.com/', resourceType: 'subFrame' }, allowedDoc)
+    await vi.waitFor(() => expect(allowedDoc).toHaveBeenCalledWith({ cancel: false }))
+
+    // A subresource to a literal private IP is cancelled synchronously.
+    const blockedSub = vi.fn()
+    onBeforeRequest({ url: 'http://169.254.169.254/x.js', resourceType: 'image' }, blockedSub)
+    expect(blockedSub).toHaveBeenCalledWith({ cancel: true })
+
+    // A public subresource passes.
+    const allowedSub = vi.fn()
+    onBeforeRequest({ url: 'https://example.com/x.js', resourceType: 'script' }, allowedSub)
+    expect(allowedSub).toHaveBeenCalledWith({ cancel: false })
+  })
+
   it('permission handlers deny every request on the agent partition', () => {
     const tab = session.ensureTab()
     const ses = (tab.view as unknown as MockView).webContents.session

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

@@ -3,6 +3,7 @@ import { createLogger } from '@sim/logger'
 import type { BrowserWindow, Session, WebContents } from 'electron'
 import { WebContentsView } from 'electron'
 import { registerAgentWebContents } from '@/main/browser-agent/registry'
+import { checkAgentUrl, isBlockedRequestUrl } from '@/main/browser-agent/url-guard'
 
 const logger = createLogger('BrowserAgentSession')
 
@@ -24,7 +25,7 @@ export interface AgentSessionEvents {
   /** The active tab changed (new tab, switch, close). */
   onActiveTabChanged: (contents: WebContents) => void
   /** A download was blocked on the agent partition. */
-  onDownloadBlocked: (filename: string, url: string) => void
+  onDownloadBlocked: (filename: string) => void
 }
 
 /**
@@ -68,12 +69,38 @@ function configureAgentPartition(ses: Session): void {
   partitionConfigured = true
   ses.setPermissionRequestHandler((_wc, _permission, callback) => callback(false))
   ses.setPermissionCheckHandler(() => false)
+  // The SSRF choke point for the agent partition. Document navigations
+  // (top-level + iframes) get the full DNS-resolving check — this is the ONE
+  // seam every navigation passes through, including the page-initiated ones the
+  // driver never sees (server redirects, link clicks, location.href,
+  // meta-refresh), so an internal-resolving hostname can't slip in that way.
+  // Subresources take the cheap synchronous literal-IP backstop instead of a
+  // DNS lookup per asset (a hostname subresource is already gated by its
+  // document's check).
+  ses.webRequest.onBeforeRequest((details, callback) => {
+    if (details.resourceType === 'mainFrame' || details.resourceType === 'subFrame') {
+      void checkAgentUrl(details.url)
+        .then((guard) => {
+          if (!guard.ok) {
+            logger.warn('Blocked agent document navigation to a private host')
+          }
+          callback({ cancel: !guard.ok })
+        })
+        .catch((error) => {
+          // Fail closed: an unexpected rejection must cancel, never leave the
+          // request suspended with no callback.
+          logger.error('Agent SSRF check failed; cancelling request', { error })
+          callback({ cancel: true })
+        })
+      return
+    }
+    callback({ cancel: isBlockedRequestUrl(details.url) })
+  })
   ses.on('will-download', (_event, item) => {
     const filename = item.getFilename()
-    const url = item.getURL()
     logger.info('Blocked download in agent browser', { filename })
     item.cancel()
-    events?.onDownloadBlocked(filename, url)
+    events?.onDownloadBlocked(filename)
   })
 }
 
@@ -97,7 +124,9 @@ function createTabView(): WebContentsView {
   configureAgentPartition(contents.session)
 
   // Popup-neutralize: window.open and target=_blank navigate the SAME view
-  // instead of spawning windows — the agent browses one page per tab.
+  // instead of spawning windows — the agent browses one page per tab. The
+  // resulting navigation is a document request, so the partition's
+  // onBeforeRequest SSRF check gates it; no per-call guard is needed here.
   contents.setWindowOpenHandler((details) => {
     if (/^https?:\/\//i.test(details.url)) {
       void contents.loadURL(details.url).catch(() => {})