feat(desktop): SSRF hardening + shared @sim/security/ssrf (re-home of #5763)

apps/desktop/README.md

@@ -138,7 +138,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/scripts/install-local.ts

@@ -104,14 +104,7 @@ run('bun', ['run', 'build'])
 // framework), which turns local signing into a multi-minute stall. Local
 // installs don't need timestamped signatures — only notarized distribution
 // builds do.
-run('bunx', [
-  'electron-builder',
-  '--mac',
-  'dir',
-  '--publish',
-  'never',
-  '-c.mac.timestamp=none',
-])
+run('bunx', ['electron-builder', '--mac', 'dir', '--publish', 'never', '-c.mac.timestamp=none'])
 
 const builtApp = RELEASE_DIRS.map((dir) => join(dir, APP_NAME)).find(existsSync)
 if (!builtApp) {

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

@@ -28,7 +28,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>()
 
@@ -42,7 +44,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()) {
@@ -76,6 +78,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 { ToolError } from '@/main/browser-agent/errors'
@@ -41,6 +43,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')
 
@@ -126,7 +129,7 @@ function instrumentTab(contents: WebContents): void {
     .then(() => cdp.setColorScheme(contents, session.getBrowserTheme()))
     .catch((error) => {
       logger.warn('CDP instrumentation failed', {
-        error: error instanceof Error ? error.message : String(error),
+        error: getErrorMessage(error),
       })
     })
   for (const event of [
@@ -160,7 +163,7 @@ export function initDriver(
       onTabThemeChanged: (contents, theme) => {
         void cdp.setColorScheme(contents, theme).catch((error) => {
           logger.warn('Could not update browser tab theme', {
-            error: error instanceof Error ? error.message : String(error),
+            error: getErrorMessage(error),
           })
         })
       },
@@ -174,10 +177,6 @@ export function initDriver(
   )
 }
 
-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
@@ -205,10 +204,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
@@ -223,7 +218,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.'
@@ -263,10 +258,6 @@ function unwrapPageResult(result: unknown): unknown {
   return result
 }
 
-// ---------------------------------------------------------------------------
-// Navigation
-// ---------------------------------------------------------------------------
-
 function waitForLoadComplete(contents: WebContents, timeoutMs: number): Promise<void> {
   return new Promise((resolve) => {
     let settled = false
@@ -301,10 +292,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.
@@ -338,19 +325,21 @@ 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://')
+      // Up-front SSRF check for a clean model-facing error. The partition's
+      // onBeforeRequest is the actual enforcement seam (it also catches
+      // page-initiated navigations); this pre-check exists because loadURL's
+      // rejection is swallowed below, so a blocked nav would otherwise surface
+      // as a blank page with no error.
+      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
@@ -378,12 +367,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 }
@@ -580,10 +572,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()
 
@@ -623,7 +611,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.ts

@@ -12,6 +12,7 @@ import { getErrorMessage } from '@sim/utils/errors'
 import type { BrowserWindow, Input, Session, WebContents } from 'electron'
 import { nativeTheme, WebContentsView } from 'electron'
 import { registerAgentWebContents } from '@/main/browser-agent/registry'
+import { checkAgentUrl, isBlockedRequestUrl } from '@/main/browser-agent/url-guard'
 
 const logger = createLogger('BrowserAgentSession')
 
@@ -127,6 +128,31 @@ function configureAgentPartition(ses: Session): void {
   partitionConfigured = true
   ses.setPermissionRequestHandler((_wc, _permission, callback) => callback(false))
   ses.setPermissionCheckHandler(() => false)
+  // SSRF choke point for the agent partition. Document navigations (top-level +
+  // iframes) get the full DNS-resolving check — the one seam every navigation
+  // passes through, including page-initiated ones the driver never sees (server
+  // redirects, link clicks, location.href, meta-refresh) — so an internal host
+  // can't slip in that way. Subresources take the cheap synchronous literal-IP
+  // backstop instead of a DNS lookup per asset.
+  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()
@@ -172,7 +198,7 @@ function createTabView(): WebContentsView {
         void tab.view.webContents.loadURL(details.url).catch(() => {})
       } catch (error) {
         logger.warn('Could not open browser popup in a new tab', {
-          error: error instanceof Error ? error.message : String(error),
+          error: getErrorMessage(error),
         })
       }
     }
@@ -326,7 +352,7 @@ function capturePanelSnapshot(): void {
     })
     .catch((error) => {
       logger.warn('Could not capture browser panel snapshot', {
-        error: error instanceof Error ? error.message : String(error),
+        error: getErrorMessage(error),
       })
     })
 }

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

@@ -0,0 +1,92 @@
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+const { mockLookup } = vi.hoisted(() => ({ mockLookup: vi.fn() }))
+
+vi.mock('node:dns/promises', () => ({
+  default: { lookup: mockLookup },
+}))
+
+import { checkAgentUrl, isBlockedRequestUrl } from '@/main/browser-agent/url-guard'
+
+describe('checkAgentUrl', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    mockLookup.mockResolvedValue([{ address: '93.184.216.34', family: 4 }])
+  })
+
+  it('rejects non-http(s) schemes without resolving', async () => {
+    const result = await checkAgentUrl('file:///etc/passwd')
+    expect(result.ok).toBe(false)
+    expect(mockLookup).not.toHaveBeenCalled()
+  })
+
+  it('rejects malformed URLs', async () => {
+    expect((await checkAgentUrl('not a url')).ok).toBe(false)
+  })
+
+  it('blocks private IP literals without resolving', async () => {
+    expect((await checkAgentUrl('http://127.0.0.1/')).ok).toBe(false)
+    expect((await checkAgentUrl('http://169.254.169.254/latest/meta-data')).ok).toBe(false)
+    expect((await checkAgentUrl('http://10.0.0.5/')).ok).toBe(false)
+    expect((await checkAgentUrl('http://[::1]/')).ok).toBe(false)
+    expect(mockLookup).not.toHaveBeenCalled()
+  })
+
+  it('allows public IP literals without resolving', async () => {
+    expect((await checkAgentUrl('https://8.8.8.8/')).ok).toBe(true)
+    expect(mockLookup).not.toHaveBeenCalled()
+  })
+
+  it('allows hostnames that resolve to public addresses', async () => {
+    const result = await checkAgentUrl('https://example.com/page')
+    expect(result.ok).toBe(true)
+    expect(mockLookup).toHaveBeenCalledWith('example.com', { all: true, verbatim: true })
+  })
+
+  it('blocks hostnames that resolve to a private address (DNS rebinding)', async () => {
+    mockLookup.mockResolvedValue([{ address: '10.1.2.3', family: 4 }])
+    expect((await checkAgentUrl('https://rebind.evil.test/')).ok).toBe(false)
+  })
+
+  it('blocks when any resolved address is private', async () => {
+    mockLookup.mockResolvedValue([
+      { address: '93.184.216.34', family: 4 },
+      { address: '192.168.0.9', family: 4 },
+    ])
+    expect((await checkAgentUrl('https://mixed.test/')).ok).toBe(false)
+  })
+
+  it('fails closed when DNS resolution fails', async () => {
+    mockLookup.mockRejectedValue(new Error('ENOTFOUND'))
+    expect((await checkAgentUrl('https://nope.invalid/')).ok).toBe(false)
+  })
+
+  it('fails closed when the DNS lookup exceeds the deadline', async () => {
+    vi.useFakeTimers()
+    try {
+      mockLookup.mockReturnValue(new Promise(() => {})) // never resolves
+      const pending = checkAgentUrl('https://slow.test/')
+      await vi.advanceTimersByTimeAsync(5_000)
+      expect((await pending).ok).toBe(false)
+    } finally {
+      vi.useRealTimers()
+    }
+  })
+})
+
+describe('isBlockedRequestUrl', () => {
+  it('blocks literal private/reserved hosts', () => {
+    expect(isBlockedRequestUrl('http://169.254.169.254/latest/meta-data')).toBe(true)
+    expect(isBlockedRequestUrl('http://127.0.0.1:8080/x')).toBe(true)
+    expect(isBlockedRequestUrl('https://[fd00::1]/')).toBe(true)
+  })
+
+  it('allows public literals and hostnames (classified at nav time)', () => {
+    expect(isBlockedRequestUrl('https://8.8.8.8/')).toBe(false)
+    expect(isBlockedRequestUrl('https://example.com/x')).toBe(false)
+  })
+
+  it('does not throw on malformed input', () => {
+    expect(isBlockedRequestUrl('::::')).toBe(false)
+  })
+})