Skip to content

Commit 4597fcb

Browse files
committed
feat(desktop): re-integrate SSRF guard + hardening onto rewritten dev
Re-applies the browser-agent SSRF guard and hardening onto dev's evolved desktop files (dev rewrote session/driver/handoff/index and split out errors.ts/keyboard.ts): - session.ts: agent-partition onBeforeRequest is the SSRF choke point — DNS-resolving check (fail-closed) for document navigations, synchronous literal-IP backstop for subresources. - driver.ts: browser_navigate/browser_open_tab validate via checkAgentUrl for a clean model error; also adopt shared sleep/getErrorMessage and drop the local reimplementations + banner separators. - index.ts: local-only crashReporter (native minidumps, no upload) + CSP fallback wired into the app session. - window.ts: record the crash-dump dir on renderer_gone. - config.ts: drop the local LOCAL_HOSTNAMES set for the shared isLoopbackHostname (also removes the dead bare '::1'). - cdp.ts: per-WebContents callbacks so a background tab's events reach its own driver. - updater.ts: the manual check now surfaces network/manifest failures instead of silently swallowing them. - README: correct the App Sandbox / security-scoped-bookmark note. - electron-mock: webRequest.onBeforeRequest + crashReporter stubs. - api-validation: annotate dev's validated-envelope double-cast; bump the route-count baseline 964→965 for dev's already-merged route (ratchets stay tight; non-Zod and double-cast at baseline). Skipped as moot (dev already did them independently): launcher isVisible removal, decideStartRoute param drop, local-filesystem clear() removal.
1 parent 6d42cc8 commit 4597fcb

11 files changed

Lines changed: 89 additions & 49 deletions

File tree

apps/desktop/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ Good fits for the bridge: OS notifications + dock badge on workflow completion,
138138
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:
139139

140140
- **Explicit and read-only:** the native folder picker creates the grant; there are no write/delete/execute operations.
141-
- **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.
141+
- **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.
142142
- **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.
143143
- **Opaque:** renderer and model see only `localfs://<mount-id>/...` URIs. Electron resolves every URI, checks lexical and realpath containment, and refuses symlink escapes.
144144
- **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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,9 @@ export interface CdpCallbacks {
2828
onFileChooser: () => void
2929
}
3030

31-
let callbacks: CdpCallbacks | null = null
31+
/** Per-tab callbacks, so a background tab's events reach ITS driver, not the
32+
* most-recently-instrumented tab's. */
33+
const callbacksByContents = new WeakMap<WebContents, CdpCallbacks>()
3234
/** Contents already instrumented (attach survives for the tab's lifetime). */
3335
const instrumented = new WeakSet<WebContents>()
3436

@@ -42,7 +44,7 @@ async function send<T = unknown>(
4244

4345
/** Idempotently instruments a tab's WebContents. */
4446
export async function ensureInstrumented(contents: WebContents, cb: CdpCallbacks): Promise<void> {
45-
callbacks = cb
47+
callbacksByContents.set(contents, cb)
4648
if (instrumented.has(contents) && contents.debugger.isAttached()) return
4749

4850
if (!contents.debugger.isAttached()) {
@@ -76,6 +78,7 @@ function handleDebuggerEvent(
7678
method: string,
7779
params: Record<string, unknown>
7880
): void {
81+
const callbacks = callbacksByContents.get(contents)
7982
if (method === 'Page.javascriptDialogOpening') {
8083
const type = String(params.type ?? 'dialog')
8184
const message = String(params.message ?? '').slice(0, 500)

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

Lines changed: 16 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ import type {
2222
BrowserToolName,
2323
} from '@sim/browser-protocol'
2424
import { createLogger } from '@sim/logger'
25+
import { getErrorMessage } from '@sim/utils/errors'
26+
import { sleep } from '@sim/utils/helpers'
2527
import type { BrowserWindow, WebContents } from 'electron'
2628
import * as cdp from '@/main/browser-agent/cdp'
2729
import { ToolError } from '@/main/browser-agent/errors'
@@ -41,6 +43,7 @@ import {
4143
typeIntoElement,
4244
} from '@/main/browser-agent/page-functions'
4345
import * as session from '@/main/browser-agent/session'
46+
import { checkAgentUrl } from '@/main/browser-agent/url-guard'
4447

4548
const logger = createLogger('BrowserAgentDriver')
4649

@@ -126,7 +129,7 @@ function instrumentTab(contents: WebContents): void {
126129
.then(() => cdp.setColorScheme(contents, session.getBrowserTheme()))
127130
.catch((error) => {
128131
logger.warn('CDP instrumentation failed', {
129-
error: error instanceof Error ? error.message : String(error),
132+
error: getErrorMessage(error),
130133
})
131134
})
132135
for (const event of [
@@ -160,7 +163,7 @@ export function initDriver(
160163
onTabThemeChanged: (contents, theme) => {
161164
void cdp.setColorScheme(contents, theme).catch((error) => {
162165
logger.warn('Could not update browser tab theme', {
163-
error: error instanceof Error ? error.message : String(error),
166+
error: getErrorMessage(error),
164167
})
165168
})
166169
},
@@ -174,10 +177,6 @@ export function initDriver(
174177
)
175178
}
176179

177-
function sleep(ms: number): Promise<void> {
178-
return new Promise((resolve) => setTimeout(resolve, ms))
179-
}
180-
181180
function str(params: Record<string, unknown>, key: string): string | undefined {
182181
const value = params[key]
183182
return typeof value === 'string' && value.length > 0 ? value : undefined
@@ -205,10 +204,6 @@ function requireNum(params: Record<string, unknown>, key: string): number {
205204
return value
206205
}
207206

208-
// ---------------------------------------------------------------------------
209-
// Page-function execution
210-
// ---------------------------------------------------------------------------
211-
212207
/**
213208
* Serializes a self-contained page function and executes it in the page's
214209
* main world with JSON-encoded arguments (Electron's executeJavaScript has no
@@ -223,7 +218,7 @@ async function execInPage<Args extends unknown[], Result>(
223218
try {
224219
return (await contents.executeJavaScript(expression, true)) as Result
225220
} catch (error) {
226-
const message = error instanceof Error ? error.message : String(error)
221+
const message = getErrorMessage(error)
227222
throw new ToolError(
228223
`Cannot act on this page (${message}). Browser-internal pages cannot be automated — ` +
229224
'navigate to a regular website first.'
@@ -263,10 +258,6 @@ function unwrapPageResult(result: unknown): unknown {
263258
return result
264259
}
265260

266-
// ---------------------------------------------------------------------------
267-
// Navigation
268-
// ---------------------------------------------------------------------------
269-
270261
function waitForLoadComplete(contents: WebContents, timeoutMs: number): Promise<void> {
271262
return new Promise((resolve) => {
272263
let settled = false
@@ -301,10 +292,6 @@ async function activeElementState(contents: WebContents): Promise<Record<string,
301292
return typeof state === 'object' && state !== null ? (state as Record<string, unknown>) : {}
302293
}
303294

304-
// ---------------------------------------------------------------------------
305-
// Takeover
306-
// ---------------------------------------------------------------------------
307-
308295
/**
309296
* Hands control to the user: the page is already natively interactive in the
310297
* panel, and the chat's takeover tool row shows the reason with a Done chip.
@@ -338,19 +325,16 @@ async function runTakeover(): Promise<unknown> {
338325
}
339326
}
340327

341-
// ---------------------------------------------------------------------------
342-
// Tool implementations
343-
// ---------------------------------------------------------------------------
344-
345328
async function executeToolInner(
346329
tool: BrowserToolName,
347330
params: Record<string, unknown>
348331
): Promise<unknown> {
349332
switch (tool) {
350333
case 'browser_navigate': {
351334
const url = requireStr(params, 'url')
352-
if (!/^https?:\/\//i.test(url)) {
353-
throw new ToolError('URL must be absolute and start with http:// or https://')
335+
const guard = await checkAgentUrl(url)
336+
if (!guard.ok) {
337+
throw new ToolError(guard.error ?? 'That address was blocked.')
354338
}
355339
const tab = session.ensureTab()
356340
const contents = tab.view.webContents
@@ -378,12 +362,15 @@ async function executeToolInner(
378362

379363
case 'browser_open_tab': {
380364
const url = str(params, 'url')
365+
if (url) {
366+
const guard = await checkAgentUrl(url)
367+
if (!guard.ok) {
368+
throw new ToolError(guard.error ?? 'That address was blocked.')
369+
}
370+
}
381371
const tab = session.addTab()
382372
const contents = tab.view.webContents
383373
if (url) {
384-
if (!/^https?:\/\//i.test(url)) {
385-
throw new ToolError('URL must be absolute and start with http:// or https://')
386-
}
387374
void contents.loadURL(url).catch(() => {})
388375
const result = await navigationResult(contents)
389376
return { tabId: tab.id, ...result }
@@ -580,10 +567,6 @@ function withNotices(result: unknown): unknown {
580567
return { value: result, notices }
581568
}
582569

583-
// ---------------------------------------------------------------------------
584-
// Public entry points
585-
// ---------------------------------------------------------------------------
586-
587570
/** One real browser can only do one thing at a time — serialize tool calls. */
588571
let toolQueue: Promise<unknown> = Promise.resolve()
589572

@@ -623,7 +606,7 @@ export async function executeTool(
623606
try {
624607
return { ok: true, result: await settled }
625608
} catch (error) {
626-
const message = error instanceof Error ? error.message : String(error)
609+
const message = getErrorMessage(error)
627610
logger.warn('Browser tool failed', { tool, error: message })
628611
return { ok: false, error: message }
629612
}

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import { getErrorMessage } from '@sim/utils/errors'
1212
import type { BrowserWindow, Input, Session, WebContents } from 'electron'
1313
import { nativeTheme, WebContentsView } from 'electron'
1414
import { registerAgentWebContents } from '@/main/browser-agent/registry'
15+
import { checkAgentUrl, isBlockedRequestUrl } from '@/main/browser-agent/url-guard'
1516

1617
const logger = createLogger('BrowserAgentSession')
1718

@@ -127,6 +128,31 @@ function configureAgentPartition(ses: Session): void {
127128
partitionConfigured = true
128129
ses.setPermissionRequestHandler((_wc, _permission, callback) => callback(false))
129130
ses.setPermissionCheckHandler(() => false)
131+
// SSRF choke point for the agent partition. Document navigations (top-level +
132+
// iframes) get the full DNS-resolving check — the one seam every navigation
133+
// passes through, including page-initiated ones the driver never sees (server
134+
// redirects, link clicks, location.href, meta-refresh) — so an internal host
135+
// can't slip in that way. Subresources take the cheap synchronous literal-IP
136+
// backstop instead of a DNS lookup per asset.
137+
ses.webRequest.onBeforeRequest((details, callback) => {
138+
if (details.resourceType === 'mainFrame' || details.resourceType === 'subFrame') {
139+
void checkAgentUrl(details.url)
140+
.then((guard) => {
141+
if (!guard.ok) {
142+
logger.warn('Blocked agent document navigation to a private host')
143+
}
144+
callback({ cancel: !guard.ok })
145+
})
146+
.catch((error) => {
147+
// Fail closed: an unexpected rejection must cancel, never leave the
148+
// request suspended with no callback.
149+
logger.error('Agent SSRF check failed; cancelling request', { error })
150+
callback({ cancel: true })
151+
})
152+
return
153+
}
154+
callback({ cancel: isBlockedRequestUrl(details.url) })
155+
})
130156
ses.on('will-download', (_event, item) => {
131157
const filename = item.getFilename()
132158
const url = item.getURL()

apps/desktop/src/main/config.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,12 @@
11
import { mkdirSync, readFileSync, renameSync, writeFileSync } from 'node:fs'
22
import { dirname } from 'node:path'
33
import { createLogger } from '@sim/logger'
4+
import { isLoopbackHostname } from '@sim/security/ssrf'
45

56
const logger = createLogger('DesktopConfig')
67

78
export const DEFAULT_ORIGIN = 'https://sim.ai'
89

9-
/** Loopback hostnames that may use plain HTTP (dev + self-host testing). */
10-
export const LOCAL_HOSTNAMES = new Set(['localhost', '127.0.0.1', '[::1]', '::1'])
11-
1210
export interface WindowBounds {
1311
x?: number
1412
y?: number
@@ -57,7 +55,7 @@ export function validateOriginInput(raw: string): OriginValidation {
5755
if (url.protocol === 'https:') {
5856
return { ok: true, origin: url.origin }
5957
}
60-
if (url.protocol === 'http:' && LOCAL_HOSTNAMES.has(url.hostname)) {
58+
if (url.protocol === 'http:' && isLoopbackHostname(url.hostname)) {
6159
return { ok: true, origin: url.origin }
6260
}
6361
return { ok: false, error: 'Server URL must use HTTPS (HTTP is allowed for localhost only)' }

apps/desktop/src/main/index.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
11
import { join } from 'node:path'
22
import { createLogger } from '@sim/logger'
33
import type { BrowserWindow } from 'electron'
4-
import { app, net, session } from 'electron'
4+
import { app, crashReporter, net, session } from 'electron'
55
import { initDriver as initBrowserAgentDriver } from '@/main/browser-agent/driver'
66
import { setPanelBounds as setBrowserAgentPanelBounds } from '@/main/browser-agent/session'
77
import { createConfigStore, partitionForOrigin } from '@/main/config'
88
import { attachContextMenu } from '@/main/context-menu'
9+
import { attachCspFallback } from '@/main/csp'
910
import { createDesktopSettingsService } from '@/main/desktop-settings'
1011
import { attachDownloadHandling } from '@/main/downloads'
1112
import { createAuthFlow, createConnectFlow, createHandoffManager } from '@/main/handoff'
@@ -127,6 +128,7 @@ function main(): void {
127128
}
128129
configuredPartitions.add(partition)
129130
setupPermissionHandlers(ses, appOrigin)
131+
attachCspFallback(ses, appOrigin)
130132
attachDownloadHandling(ses, events)
131133
attachTelemetryPolicy(ses, config.get('blockThirdPartyAnalytics') ?? true)
132134
ses.setSpellCheckerLanguages(['en-US'])
@@ -388,6 +390,13 @@ if (process.env.SIM_DESKTOP_USER_DATA) {
388390
app.setPath('userData', process.env.SIM_DESKTOP_USER_DATA)
389391
}
390392

393+
// Capture native minidumps for main/renderer/GPU crashes. Local-only: there is
394+
// no crash-ingest backend, so nothing is uploaded — the dumps land under
395+
// userData/Crashpad and the event log records where. Must start before the app
396+
// is ready so Crashpad initializes first. Set after userData so dumps follow
397+
// any test/instance override.
398+
crashReporter.start({ uploadToServer: false, compress: true })
399+
391400
const gotSingleInstanceLock = app.requestSingleInstanceLock()
392401
if (!gotSingleInstanceLock) {
393402
app.quit()

apps/desktop/src/main/updater.ts

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -221,11 +221,22 @@ export function checkForUpdatesInteractive(deps: UpdaterDeps): void {
221221
deps.events.record('update_check', { manual: true })
222222
try {
223223
const { autoUpdater } = require('electron-updater') as typeof import('electron-updater')
224-
void autoUpdater.checkForUpdates().then((result) => {
225-
if (!result || result.updateInfo.version === app.getVersion()) {
226-
void dialog.showMessageBox({ type: 'info', message: 'Sim is up to date' })
227-
}
228-
})
224+
void autoUpdater
225+
.checkForUpdates()
226+
.then((result) => {
227+
if (!result || result.updateInfo.version === app.getVersion()) {
228+
void dialog.showMessageBox({ type: 'info', message: 'Sim is up to date' })
229+
}
230+
})
231+
.catch((error) => {
232+
// Surface network/manifest/cert failures instead of silently swallowing.
233+
logger.error('Manual update check failed', { error })
234+
void dialog.showMessageBox({
235+
type: 'error',
236+
message: 'Could not check for updates',
237+
detail: 'Something went wrong reaching the update server. Try again later.',
238+
})
239+
})
229240
} catch (error) {
230241
logger.error('Manual update check failed', { error })
231242
}

apps/desktop/src/main/window.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,11 @@ export function createMainWindow(deps: CreateMainWindowDeps): BrowserWindow {
264264
if (details.reason === 'clean-exit') {
265265
return
266266
}
267-
deps.events.record('renderer_gone', { reason: details.reason, exitCode: details.exitCode })
267+
deps.events.record('renderer_gone', {
268+
reason: details.reason,
269+
exitCode: details.exitCode,
270+
crashDumpDir: app.getPath('crashDumps'),
271+
})
268272
setTimeout(() => {
269273
if (win.isDestroyed()) {
270274
return

apps/desktop/src/test/electron-mock.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,10 @@ export const app = {
3030
dock: { downloadFinished: vi.fn() },
3131
}
3232

33+
export const crashReporter = {
34+
start: vi.fn(),
35+
}
36+
3337
export const shell = {
3438
openExternal: vi.fn(() => Promise.resolve()),
3539
showItemInFolder: vi.fn(),
@@ -177,6 +181,7 @@ function createWebContentsMock() {
177181
session: {
178182
setPermissionRequestHandler: vi.fn(),
179183
setPermissionCheckHandler: vi.fn(),
184+
webRequest: { onBeforeRequest: vi.fn() },
180185
on: vi.fn(),
181186
},
182187
}

apps/sim/app/desktop/launcher/use-launcher-chat.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,7 @@ export function useLauncherChat() {
226226
logger.info('Skipping unrecognized stream event', { reason: parsed.reason })
227227
return undefined
228228
}
229+
// double-cast-allowed: parsePersistedStreamEventEnvelope validates the envelope structurally; EnvelopeLike is the local narrowed view for this read-only consumer
229230
const event = parsed.event as unknown as EnvelopeLike
230231

231232
if (event.type === 'session' && event.payload.kind === 'chat') {

0 commit comments

Comments
 (0)