feat(desktop): SSRF hardening + shared @sim/security/ssrf (re-home of #5763)#5784
Conversation
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.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Desktop agent browser gets new SSRF controls: DNS-resolving Other desktop changes: minimal CSP fallback on app-origin documents missing a policy; local-only Minor: shared Reviewed by Cursor Bugbot for commit aa6e587. Configure here. |
Greptile SummaryThis PR adds shared SSRF protection and hardens the desktop browser agent. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (4): Last reviewed commit: "fix(desktop): swallow late DNS rejection..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- 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.
|
Ran a comprehensive 3-angle audit (regression, consolidation/reuse, cleanliness) over the full diff — verdict GO, no must-fix. Every SSRF seam verified correct (fails closed, single onBeforeRequest listener on the agent partition, no dev work clobbered), migration behavior equivalent-or-stronger, all gates green. Applied the safe cleanup findings: dropped a redundant |
|
@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 6303465. Configure here.
- 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).
|
@cursor review |
|
@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 aa6e587. Configure here.
1 participant
What
Re-homes the SSRF-hardening work from #5763 onto the current
dev. #5763 was merged thendevwas force-pushed to a new history that dropped it, so these changes are gone fromdevand are re-applied here onto dev's evolved code.devcurrently has no SSRF protection in the browser-agent and still carries all the duplicated private-IP/loopback logic, so this is not redundant.Shared SSRF primitive (
@sim/security/ssrf)isPrivateIp/isPrivateIpHost/isIpLiteral/isLoopbackIp/isLoopbackHostname/unwrapIpv6Brackets— one audited,ipaddr.js-based module that dedupes the divergent copies acrossapps/sim(input-validation ×2, mcp/domain-check, sap_concur/zoominfo/sap, s3, onepassword) and the desktop shell. All callers migrated; the edge-case matrix lives in the package test.Browser-agent SSRF guard
onBeforeRequestis the authoritative choke point: DNS-resolving check (fail-closed) for document navigations — covering page-initiated redirects/clicks the tool layer never sees — and a cheap synchronous literal-IP backstop for subresources.browser_navigate/browser_open_tabvalidate up front for a clean model error.169.254.169.254), obfuscated/IPv4-mapped forms; hostnames are DNS-resolved (rebinding-aware).Other hardening
isLoopbackHostname(drops the dead::1); cdp: per-tab callbacks; updater: manual check no longer swallows errors; driver: sharedsleep/getErrorMessage, banners removed; README App-Sandbox note corrected.Re-integration notes
isVisible,decideStartRoute,local-filesystem.clear()) so those are omitted as moot.handoff.tswith a new OAuth-connect route table, and re-deriving that minor hardening onto the new structure is out of scope here.Testing
@sim/security/ssrf(106) · desktop (228) · affectedapps/simsuites (462) — all green. Type-check (desktop + fullapps/sim), Biome, andcheck:api-validation:strictall pass.