feat(desktop): SSRF hardening for the browser agent + review fixes#5763
Conversation
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.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview For the desktop browser agent, adds Shell hardening: local-only crashReporter, CSP fallback on app-origin documents missing a policy, MCP OAuth Reviewed by Cursor Bugbot for commit a266eba. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR centralises SSRF protection across the monorepo by extracting a shared
Confidence Score: 5/5Safe to merge. All changes are narrowly security-hardening; no regressions were found across the guard logic, handoff flow, CSP injection, or updater paths. The shared SSRF primitive consolidates three previously divergent implementations and adds IPv4-compatible-IPv6 coverage that was missing from the input-validation.ts copy. The browser-agent guard is architecturally correct — onBeforeRequest is the right interception point, the async DNS path fails closed, and the synchronous subresource backstop is limited to literal IPs. The handoff self-DoS fix, CDP per-tab routing, MCP popup HTTPS gate, and updater refactor are all well-scoped with thorough test coverage. No files require special attention. The dns.lookup in url-guard.ts has no explicit timeout, but the function correctly fails closed regardless of how the DNS query resolves. Important Files Changed
Reviews (4): Last reviewed commit: "fix(desktop): fail closed on DNS resolut..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…osed 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.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 486f918. Configure here.
…concile 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.
|
Pushed a follow-up cleanup wave (whole-desktop audit): shared |
|
@cursor review |
… 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.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit a266eba. Configure here.
…5763) (#5784) * feat: re-home @sim/security/ssrf + sim SSRF dedup onto dev (clean core) * 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. * chore(desktop): biome format install-local.ts (pre-existing dev lint failure) * refactor: apply audit cleanup (reuse + simplify) - domain-check: drop the redundant isIpLiteral guard (isLoopbackIp already validates and returns false for non-literals). - session.ts: use shared getErrorMessage instead of the local error ternary (the file already imports it). - tray.ts: use shared sleep() instead of a hand-rolled setTimeout promise. - updater.ts: distinguish the synchronous-throw log from the async-rejection log on the manual update check. * refactor: /simplify pass + review fixes - url-guard: bound the SSRF dns.lookup with a 5s deadline (fails closed on timeout) so a slow/hung resolver can't suspend the check and the onBeforeRequest callback indefinitely (Greptile P2); + test. - Finish the reuse consolidation the earlier pass missed: session.ts second error ternary → getErrorMessage; the bracket-strip idiom → unwrapIpv6Brackets in input-validation.ts, input-validation.server.ts (×2), onepassword/utils.ts (fixes the check:utils banned-pattern CI failure). - driver: document why the tool-level checkAgentUrl coexists with the onBeforeRequest enforcement seam (clean model error; loadURL rejection is swallowed). * fix(desktop): swallow late DNS rejection after the SSRF lookup timeout (Cursor)
1 participant
What
Security hardening for the new desktop Electron app, plus the shared-package extraction and review fixes that came out of a deep review of the
develectron branch. Targetsdev(the branch the desktop app lives on).Shared SSRF primitive (
@sim/security/ssrf)Extracts IP classification into one audited,
ipaddr.js-based module —isPrivateIp(resolved IP, fail-closed),isPrivateIpHost(host literal, fail-open on DNS names),isIpLiteral,unwrapIpv6Brackets. This de-duplicates three previously-divergent implementations:isPrivateOrReservedIPininput-validation.server.tsinput-validation.ts(missing the IPv4-compatible-IPv6 embedded check)isPrivateIPv4/isPrivateOrLoopbackIPv6insap_concur,zoominfo, and thesapcontractAll callers (apps/sim outbound-fetch validation, MCP SSRF, DB-host validation, and the desktop shell) now share one primitive. The comprehensive edge-case matrix moved into the package test.
Browser-agent SSRF guard
The embedded agent browser is model/tool-driven, so a navigation to an internal host would let the page be read back via the snapshot/read tools. Fix:
onBeforeRequestis the single authoritative choke point: document navigations (top-level + iframes) get a full DNS-resolving check — covering page-initiated navigation the tool layer never sees (server redirects, link clicks,location.href, meta-refresh) — while subresources get a cheap synchronous literal-IP backstop.browser_navigate/browser_open_tabalso validate up front so the model gets a clean error instead of a silent load failure.169.254.169.254), and obfuscated / IPv4-mapped forms; hostnames are resolved (DNS-rebinding aware).Other hardening
uploadToServer:false, no backend); dump dir recorded in the event log.frame-ancestors 'self'; object-src 'none'; base-uri 'self') injected only when an app-origin document ships no CSP of its own.httpsso a renderer-controlled frame name can't ride anhttpURL into an in-app window.configureAutoUpdaterre-assertschannel/allowDowngradeon both the launch and manual-check paths.WebContentscallbacks so a background tab's events reach its own driver.::1loopback entry.Testing
@sim/security/ssrf: 67 testsapps/desktop: 200 testsapps/simsuites (input-validation, domain-check, mock consumers): greenapps/desktop,packages/security, and touchedapps/simfilesNotes for review
idp-in-windownavigation default (tightening breaks same-window OAuth connect), and the cross-origin MCP popup (same-origin-only would break the real provider OAuth URL — gated tohttpsinstead).