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

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(() => {})