fix(desktop): reliable dev grida:// deep-link routing — unique dev bundle id + per-env grida-dev:// scheme (GRIDA-SEC-005)#956
Merged
Conversation
…ndle id + per-env grida-dev:// scheme (GRIDA-SEC-005)
Desktop sign-in (the GRIDA-SEC-005 PKCE deep link) was unreliable in dev on
macOS:
- `grida://auth/callback` opened the WRONG Electron app — every un-rebranded
dev Electron shares the generic `com.github.Electron` bundle id, and macOS
LaunchServices routes a URL scheme by bundle id, so the link could hit
another project's Electron (observed: a sibling repo's dev app).
- Or NO app — `app.setAsDefaultProtocolClient` is documented to take effect
only for PACKAGED apps on macOS, so an unpackaged dev build never registered
a handler at all ("There is no application set to open the URL grida://").
- And even once fixed, dev collided with an installed production Grida: both
declare `grida://`, so LaunchServices picks one default handler
nondeterministically (the /Applications app tends to win).
Fix, in two layers:
1) scripts/prepare-dev-electron-branding.mjs (already runs before every
`electron-forge start`) now stamps a globally-unique CFBundleIdentifier on
the dev Electron (co.grida.desktop.dev / co.grida.insiders.dev), DECLARES
the scheme via CFBundleURLTypes (the registration LaunchServices actually
reads — what forge bakes into packaged builds), ad-hoc re-signs so the
signature identifier matches the patched plist, and `lsregister -f`s it.
Isolates dev from other Electron projects and gives macOS a real
declarative handler despite the packaged-only runtime limitation.
2) Per-environment scheme: local dev/insiders register and return to
`grida-dev://`; production keeps `grida://`, so the two can never fight
over one OS handler when both are installed. Single source
DEEP_LINK_SCHEME in desktop/src/env.ts (mirrors EDITOR_BASE_URL); main.ts
registers it; the protocol router + argv classifier accept both;
forge.config bakes the channel-appropriate scheme for packaged builds; the
editor's DESKTOP_AUTH_REDIRECT is env-derived; supabase local allowlists
grida-dev://auth/callback.
GRIDA-SEC-005 reviewed, security-neutral: the router stays stateless /
fixed-target / verifier-gated for both schemes; the redirect target is a
build-time constant (NODE_ENV / app.isPackaged), never request input, so it
stays non-attacker-controllable; production is entirely unchanged. SECURITY.md
enforcement step + Files-bound list updated; the desktop skill aligned.
Verified: desktop `tsc` + 106 tests (incl. a grida-dev:// routing case);
editor `tsc` + 25 GRIDA-SEC-005 tests; supabase reloaded with the grida-dev
allowlist entry; the dev bundle is the sole grida-dev:// claimant.
Closes #955
|
The latest updates on your projects. Learn more about Vercel for GitHub. 6 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThis PR adds a development-specific ChangesDev-specific deep-link scheme support
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
desktop
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e0a92f70d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
desktop/src/env.ts (1)
18-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider centralizing all valid deep-link scheme literals.
DEEP_LINK_SCHEMEgives the single active scheme per build, butprotocol-router.ts("grida:","grida-dev:") andopen-handoff.ts("grida://","grida-dev://") independently hardcode the full set of accepted schemes. If a scheme is ever renamed or a third variant added, three files must be updated in sync. Consider exporting a companionDEEP_LINK_SCHEMES(or similar) array from this file as the single source of truth for "all schemes this router should accept," and have the other two files import it.♻️ Suggested consolidation
export const DEEP_LINK_SCHEME = IS_INSIDERS || IS_DEV ? "grida-dev" : "grida"; + +// All schemes any build's router should recognize (own scheme is chosen by +// DEEP_LINK_SCHEME above; routing accepts both intentionally per GRIDA-SEC-005). +export const DEEP_LINK_SCHEMES = ["grida", "grida-dev"] as const;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/src/env.ts` around lines 18 - 28, Centralize the accepted deep-link scheme literals to avoid duplicating them across routing code. Add a companion exported list in env.ts alongside DEEP_LINK_SCHEME that contains every valid scheme variant, then update protocol-router.ts and open-handoff.ts to import and use that shared source of truth instead of hardcoding “grida”, “grida-dev”, and their URL forms. Keep DEEP_LINK_SCHEME as the per-build active scheme, but make the router/hand-off acceptance set derive from the shared constant so future renames or new variants only need one update.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@desktop/src/env.ts`:
- Around line 18-28: Centralize the accepted deep-link scheme literals to avoid
duplicating them across routing code. Add a companion exported list in env.ts
alongside DEEP_LINK_SCHEME that contains every valid scheme variant, then update
protocol-router.ts and open-handoff.ts to import and use that shared source of
truth instead of hardcoding “grida”, “grida-dev”, and their URL forms. Keep
DEEP_LINK_SCHEME as the per-build active scheme, but make the router/hand-off
acceptance set derive from the shared constant so future renames or new variants
only need one update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05d16c1b-4978-4cc0-9202-13650b55581d
📒 Files selected for processing (11)
.agents/skills/desktop/SKILL.mdSECURITY.mddesktop/forge.config.tsdesktop/scripts/prepare-dev-electron-branding.mjsdesktop/src/env.tsdesktop/src/main.tsdesktop/src/main/open-handoff.tsdesktop/src/main/protocol-router.test.tsdesktop/src/main/protocol-router.tseditor/lib/desktop/auth-deeplink.tssupabase/config.toml
The repo-wide `oxfmt --check` (CI `pnpm fmt:check`) formats TOML too; the lefthook pre-commit glob doesn't cover .toml, so the long array line slipped through locally. No behavior change.
…eep-link schemes Addresses PR #956 review: - main.ts: register the scheme with the documented dev form `setAsDefaultProtocolClient(scheme, execPath, [appPath])` under `process.defaultApp`. The one-arg form recorded the bare Electron executable without the app entry, so on Windows dev a grida-dev:// link could relaunch a blank Electron instead of delivering the URL. (Codex P2) - Centralize the accepted scheme set in a new electron-free `deep-link.ts` (`DEEP_LINK_SCHEMES`), consumed by env.ts (type), protocol-router.ts, and open-handoff.ts — one source of truth instead of three hardcodes. Kept it out of env.ts (which imports electron) so the electron-free argv classifier stays testable without a mock. (CodeRabbit nitpick) Desktop tsc + 106 tests green; repo fmt clean.
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
1 participant
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.
Makes desktop sign-in's deep-link handoff route reliably to the dev app on macOS and coexist with an installed production Grida — the GRIDA-SEC-005 PKCE return path. Full context/history: #955.
Problem
Desktop sign-in returns through a
grida://auth/callbackdeep link. In dev on macOS this was unreliable in three compounding ways:com.github.Electronbundle id, and macOS LaunchServices resolves a URL scheme by bundle id — so the link could open another project's Electron.app.setAsDefaultProtocolClientis documented to take effect only for packaged apps on macOS, so an unpackaged dev build never registered a handler at all ("There is no application set to open the URL grida://").grida://, so LaunchServices picks a default handler nondeterministically (the/Applicationsapp tends to win).Fix (two layers)
1. Unique dev identity + declarative registration —
scripts/prepare-dev-electron-branding.mjs(already runs before everyelectron-forge start) now stamps a globally-uniqueCFBundleIdentifieron the dev Electron (co.grida.desktop.dev/co.grida.insiders.dev), declares the scheme viaCFBundleURLTypes(the registration LaunchServices actually reads — what forge bakes into packaged builds), ad-hoc re-signs so the signature id matches the patched plist, andlsregister -fs it.2. Per-environment scheme — local dev/insiders register and return to
grida-dev://; production keepsgrida://, so they can never fight over one OS handler when both are installed. Single sourceDEEP_LINK_SCHEMEindesktop/src/env.ts(mirrorsEDITOR_BASE_URL);main.tsregisters it; the protocol router + argv classifier accept both;forge.configbakes the channel-appropriate scheme for packaged builds; the editor'sDESKTOP_AUTH_REDIRECTis env-derived; supabase local allowlistsgrida-dev://auth/callback.Security — GRIDA-SEC-005
Reviewed and security-neutral:
NODE_ENV/app.isPackaged), never request input → stays non-attacker-controllable, the property the boundary depends on.grida-dev://with identical enforcement.SECURITY.md(enforcement step 6 + Files-bound list) and thedesktopskill are aligned in this PR.Verification
tsc+ 106 tests (incl. a newgrida-dev://routing case)tsc+ 25 GRIDA-SEC-005 testsGOTRUE_URI_ALLOW_LISTcarriesgrida-dev://auth/callbackgrida-dev://claimant, and the live sign-in round-trip lands in the dev appReviewer notes
DEEP_LINK_SCHEMEviasetAsDefaultProtocolClient+ forge MIME/protocols.grida-devtoo (forgeprotocolsis channel-aware) to stay aligned with the editor's dev redirect.grida://(prod).grida-devnever belongs in the hosted allowlist.Closes #955