fix(desktop): origin settings for prod, github login improvements - #6050
Merged
Conversation
Resolved by taking staging's copilot refactor (mv/mkdir/cp consolidation, sim_key chips, CredentialLinkDisplay extraction, streaming-arg titles) and re-grafting the desktop-only additions on top: desktopLocalFilesystem + browserCapable payload params, local-filesystem tool schemas, local_*/browser_* tool display titles, and the folder_access grant chip. Dropped the user-skill (load_user_skill) wiring — staging removed lib/mothership/skills in mothership v0.8. Generated tool catalog/schemas regenerated from the copilot contract with staging's sync script. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The desktop-release reusable-workflow call requested contents: write, which ci.yml's permission grant (contents: read) rejects — invalidating the whole CI workflow. Desktop is tested locally for now; signed builds remain available manually via desktop-release.yml workflow_dispatch, and desktop e2e via its own workflow_dispatch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
electron@43.1.1 (published 2026-07-14) is exact-pinned for the desktop shell and blocked by minimumReleaseAge until 2026-07-21. Excluded with a drop-after date, following the vetted-typescript precedent. Verified the rest of the desktop dependency set clears the 7-day gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- build/icon.icns regenerated from public/logo/primary/large.png on the Apple icon grid (824px body, r=185.4, centered on a transparent 1024 canvas), compiled with iconutil - dev runs set the same mark via app.dock.setIcon (static/dock-icon.png) — unpackaged Electron otherwise shows its default atom icon - un-ignore apps/desktop/build: it holds electron-builder INPUTS (icon, entitlements), which the /apps/**/build output rule was swallowing — the icns and entitlements were never actually tracked - revert resetAdHocDarwinSignature fuse: it corrupts the packaged binary signature (app killed at launch on arm64); the local ad-hoc deep-sign flow doesn't need it Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
White rounded tile with the black sim wordmark (from public/logo/b&w/large.png), replacing the purple variant. Same Apple icon grid geometry (824px body, r=185.4, 1024 canvas). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…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)
0264/0265 on this branch collide with staging's 0264-0270 on both the journal idx slots and the meta snapshot filenames. Reverting the migration artifacts to the merge-base lets staging's chain merge cleanly; schema.ts keeps the copilot changes and drizzle-kit regenerates a single migration on top of 0270 after the merge. Co-Authored-By: Claude <noreply@anthropic.com>
Resolved 24 conflicts. Notable decisions, all made by checking what the merged tree actually does rather than preferring a side: - MCP prompt text (process-contents.ts): took staging's wording. Staging's `defer_loading: false` in mcp-tools.ts won the (unconflicted) merge, and `load_mcp_tool` is absent from the generated tool catalog, so the branch's "load before calling" instruction described behavior that no longer exists. Fixed the same stale instruction in mothership/execute/route.ts, which had merged cleanly and so was never flagged as a conflict. - Session cookie cache (auth.ts): kept the branch's 5-minute maxAge AND staging's org security-policy `version` callback. They solve different halves of revocation latency — `version` only covers org-wide invalidation, so the short TTL remains the only bound on per-device sign-out. - E2B -> sandbox rename: adopted staging's `isDocSandboxEnabled` everywhere. - xlsx mothership-beta gate: stayed removed (the branch deleted it deliberately); dropped the now-dead `isFeatureEnabled` imports and the beta-flag arm of the test. - delete_file tool, workflow-alias machinery, and `isPrivateOrReservedIP` were all deleted by this branch while staging kept using them. Staging's new call sites merged in cleanly and referenced missing symbols; rewired them (`isPrivateOrReservedIP` -> `isPrivateIp` from @sim/security/ssrf) or removed them where the feature they guarded no longer exists. - special-tags.tsx: unioned both feature sets (branch's folder_access / browser_takeover / terminal_handoff, staging's service_account) — the already-merged dispatcher references all four. - add-resource-dropdown.tsx: dropped the branch's `isOpen` plumbing; staging's `select()` derives the same key from `existingKeys` internally. - Added the branch's `isCopilotToolPermissionsEnabled` to the shared env-flags test mock, which staging's mock migration did not know about. DB migrations were reverted to the merge-base in the preceding commit so staging's 0264-0270 chain applies cleanly; the branch's schema change is regenerated on top in the next commit. Co-Authored-By: Claude <noreply@anthropic.com>
Replaces the branch's old 0264/0265 (dropped pre-merge so staging's 0264-0270 chain could apply cleanly) with a single 0271 generated against staging's schema: the permission-decision enum, the two copilot_async_tool_calls decision columns, and copilot_chats.auto_allowed_tools. Deliberately does NOT drop copilot_chats.plan_artifact. The branch removed every reader, but the currently-deployed code still SELECTs that column, so dropping it in the same deploy breaks the old app version during blue/green overlap — `check:migrations` flags it for exactly this reason, and the honest fix is to defer rather than annotate around it. The column is retained in schema.ts marked @deprecated; drop it in a follow-up once this has rolled out. Also in this commit, all fallout from the merge itself: - pinned-fetch/revoke tests: their private-IP stub moved to @sim/security/ssrf alongside the source change. Worth noting the stub exists because the suite's 203.0.113.10 is TEST-NET-3, which the real classifier correctly calls reserved — the old stub had been quietly disagreeing with production. - materialize-file test: dropped the reserved-system-folder case, which covered the workflow-alias backing folders this branch deleted. - api-validation route ratchet 977 -> 983 (this branch's new routes). Co-Authored-By: Claude <noreply@anthropic.com>
Both sides independently claimed idx 0271, so the snapshot and journal would conflict add/add. Ours is plain additive DDL that drizzle regenerates from schema.ts; staging's is a hand-written CONCURRENTLY index build that cannot be regenerated. Dropping ours and re-generating on top of staging's is the only order that preserves both. schema.ts is deliberately untouched — it is the source of the regeneration. Co-Authored-By: Claude <noreply@anthropic.com>
Resolutions:
- packages/db/migrations — no conflict by construction; the branch's 0271 was
reverted to the merge-base in the preceding commit, so staging's 0271
(a hand-written CONCURRENTLY index build) applied cleanly and the branch's
additive DDL regenerated on top as 0272. Regenerated content is byte-identical
to what it replaced, and drizzle reports no drift.
- workflow-alias-backing.{ts,test.ts} — kept deleted. The branch removed this
subsystem deliberately (ac55b1f); staging kept evolving it. Verified all
three exported symbols have zero references in the merged tree before
resolving, so nothing is left dangling.
- workspace-file-manager.ts — took HEAD for the two alias-module imports, but
KEPT staging's generateRequestId import: staging's new code at the call site
merged in cleanly and uses it, so taking our side wholesale would have
compiled to a missing binding.
- check-api-validation-contracts.ts — the route-count ratchet. Neither side's
number is right: the merge unions both sets, so the baseline is 985, measured
from the merged tree rather than picked from a side.
- bun.lock — regenerated from the merged manifests with the pinned bun (1.3.13)
rather than hand-resolved; --frozen-lockfile verifies clean.
Co-Authored-By: Claude <noreply@anthropic.com>
Both sides independently claimed idx 0272, so the snapshot and journal would conflict add/add. Ours is plain additive DDL (one enum, two columns, one jsonb default) that drizzle regenerates from schema.ts; staging's is a hand-written migration with DO blocks and CONCURRENTLY index builds that cannot be regenerated. Dropping ours and re-generating on top of staging's is the only order that preserves both. schema.ts is deliberately untouched — it is the source of the regeneration. Co-Authored-By: Claude <noreply@anthropic.com>
Conflicts resolved: - workspace-file-folder-manager.ts: staging kept the workflow-alias import while ac55b1f deleted that module here, so only staging's collectDescendantFolderIds import survives — the alias symbol has no remaining callers. - check-api-validation-contracts.ts: route baseline is additive on both sides (979 base + 6 ours + 2 staging); audit reports exactly 987. - bun.lock: regenerated from the merged package.json files rather than hand-merged. Our 0272 was dropped in the preceding commit and regenerated as 0273 on top of staging's 0272; the emitted SQL is byte-identical to what was dropped. Co-Authored-By: Claude <noreply@anthropic.com>
drizzle-kit emits _journal.json and the snapshot with expanded arrays, which biome check rejects. The merge commit used --no-verify, so lint-staged never formatted them and CI's lint step failed on exactly these two files. Whitespace only — both files are byte-identical under `jq -S -c`. Co-Authored-By: Claude <noreply@anthropic.com>
promptForSecret gates Touch ID on process.platform === 'darwin'. The suite mocked electron's systemPreferences but inherited the runner's real platform, so the eight biometric expectations passed on a Mac and failed on Linux CI, where every call fell through to the confirmation dialog instead. Pins the platform per-test and restores it after, and adds a case for the gate itself — the branch whose absence from the suite is what let this through. Co-Authored-By: Claude <noreply@anthropic.com>
# Conflicts: # apps/sim/lib/copilot/vfs/workflow-alias-resolver.ts # bun.lock # scripts/check-api-validation-contracts.ts
…rigin # Conflicts: # .github/workflows/ci.yml # apps/desktop/README.md # apps/desktop/scripts/build.ts # apps/desktop/scripts/install-local.ts # apps/desktop/src/main/config.ts # apps/desktop/src/main/menu.test.ts # apps/desktop/src/main/menu.ts # apps/desktop/src/main/navigation.test.ts # apps/desktop/src/main/navigation.ts # apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.ts # packages/db/migrations/meta/_journal.json
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview Sign-in navigation always uses the system-browser handoff from any auth surface (including GitHub and other formerly in-window IdPs); Reviewed by Cursor Bugbot for commit 045007a. Configure here. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Greptile SummaryFixes prod desktop origin and always routes sign-in through the system browser.
Confidence Score: 3/5Not safe to merge until existing apex settings are migrated to www and mid-flow off-origin redirects onto system-browser IdP hosts no longer cancel in-window connects. Upgraded prod users can keep https://sim.ai in settings and still hit the off-origin login stranding this PR targets; in-window connect redirect chains that hop onto Google/Microsoft hosts are newly intercepted as connect dialogs mid-flow. Files Needing Attention: apps/desktop/src/main/config.ts, apps/desktop/src/main/navigation.ts
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/navigation.ts | Auth always system-browser; dropped off-origin in-window short-circuit can interrupt mid-connect SSO hops onto blocked IdP hosts. |
| apps/desktop/src/main/config.ts | New default www origin; existing settings can keep apex with no migration, leaving the off-origin prod failure for upgrades. |
| apps/desktop/src/main/menu.ts | Adds Back via navigationHistory; matches Electron 43 usage elsewhere. |
| apps/desktop/scripts/install-local.ts | --prod leaves origin unset so it matches baked DEFAULT_ORIGIN. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Nav[will-navigate / will-redirect]
Nav --> Class[classifyNavigation]
Class -->|same origin| InApp[in-app]
Class -->|from auth surface| Login[idp-system-login → handoff]
Class -->|SYSTEM_BROWSER IdP host| Connect[idp-system-connect → dialog]
Class -->|other off-origin| InWin[idp-in-window]
Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'origin/sta..." | Re-trigger Greptile
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 045007a. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Prod desktop builds baked
https://sim.aias the app origin, but the ALB 301s the apex tohttps://www.sim.ai— so every page ran off-origin, which reclassified social sign-in as an integration connect and stranded it in the browser with no way back (dev/staging were unaffected; their release origins already carrywww).Sign-in now always takes the system-browser handoff whatever the IdP — embedding one is a one-way door in a chrome-less shell and splits better-auth's OAuth state across two cookie jars (
state_mismatch) — which makesIN_WINDOW_IDP_HOSTSdead code; also adds a Back menu item for in-window connects.Type of Change
Testing
Tested manually
Checklist