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