fix(desktop): Register and route t3code:// links - #4908
Conversation
Clicking a t3code:// link did nothing outside macOS. The build config declared the scheme only under mac, so the Linux desktop entry carried no x-scheme-handler for xdg to resolve, and nothing consumed an incoming URL on any platform: setAsDefaultProtocolClient had no callers, no open-url handler existed, and the second-instance handler dropped the argv that Linux and Windows deliver links through. Declaring protocols at the top level covers the macOS Info.plist and the Linux desktop entry from one source. NSIS reads it from neither, so Windows relies on the runtime registration that DesktopDeepLinks now performs. That service also captures links from argv, second-instance, and open-url, and holds a cold-start link until bootstrap has opened a renderer able to receive it. A link is untrusted input from any web page, so only our own scheme and host survive parsing and the renderer only ever receives a path. A protocol-relative path is rejected because a router would read it as another origin. Delivery goes through the renderer so navigation stays client-side rather than reloading the window and discarding session state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the new DesktopDeepLinks service and the touched call sites against the Effect service conventions. Structure (namespace subpath imports, inline Context.Service interface, exported make/layer, environment-acquired dependencies, Effect.runPromiseWith confined to Electron native callbacks as in DesktopClerk) all look correct. One finding on log/telemetry payload safety.
Posted via Macroscope — Effect Service Conventions
| return; | ||
| } | ||
|
|
||
| yield* logDeepLinkInfo("deep link received", { target: target.value }); |
There was a problem hiding this comment.
The logged target is untrusted external input and, per parseDeepLinkTarget, includes search and hash — so any token or credential carried in a t3code:// link's query string or fragment lands verbatim in the desktop logs. Consider logging only bounded, sanitized diagnostics (path plus presence/length indicators). The same value is also copied onto the span in DesktopWindow.dispatchDeepLink (Effect.annotateCurrentSpan({ target })), which is worth narrowing alongside it.
| yield* logDeepLinkInfo("deep link received", { target: target.value }); | |
| yield* logDeepLinkInfo("deep link received", { | |
| path: target.value.split(/[?#]/, 1)[0], | |
| hasQuery: target.value.includes("?"), | |
| hasFragment: target.value.includes("#"), | |
| }); |
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Fixed in dc752af. Agreed — `parseDeepLinkTarget` keeps `search` and `hash` on purpose, so a one-time token in either would have gone to the logs verbatim.
Added `describeDeepLinkTarget` (apps/desktop/src/app/deepLinkTarget.ts), which reduces a target to `{ path, hasQuery, hasFragment }`, and both the log call and the `dispatchDeepLink` span annotation now use it — thanks for flagging the span, it would have leaked the same value into telemetry. Covered by a test that asserts neither a query token nor an `#id_token=` fragment survives.
I put the helper in a new leaf module rather than `DesktopDeepLinks.ts` because `DesktopWindow` needs it for the span and `DesktopDeepLinks` already imports `DesktopWindow`; the service module re-exports it so the public surface is unchanged.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
| }; | ||
| }, [navigate, pathname]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
🟠 High components/AppSidebarLayout.tsx:185
The onDeepLink listener is registered inside AppSidebarLayout, which RootRouteView does not render on /pair, /connect, /connect/*, or unauthenticated gate states. When the desktop app receives a valid deep link while one of those screens is showing, the IPC message is dispatched but no onDeepLink listener exists, so the navigation target is silently lost and the app does not navigate. Register this listener in an always-mounted root component (or retain/retry links until the renderer acknowledges them).
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/AppSidebarLayout.tsx around line 185:
The `onDeepLink` listener is registered inside `AppSidebarLayout`, which `RootRouteView` does not render on `/pair`, `/connect`, `/connect/*`, or unauthenticated gate states. When the desktop app receives a valid deep link while one of those screens is showing, the IPC message is dispatched but no `onDeepLink` listener exists, so the navigation target is silently lost and the app does not navigate. Register this listener in an always-mounted root component (or retain/retry links until the renderer acknowledges them).
There was a problem hiding this comment.
Confirmed and fixed in dc752af. `RootRouteView` returns early for `/pair`, `/connect`, `/connect/*` and for any non-authenticated/hosted-static gate state, so the listener was missing on exactly the screens a pairing link would land on.
The subscription moved into `useDesktopDeepLinks` (apps/web/src/hooks/useDesktopDeepLinks.ts), called from `RootRouteView` above those early returns, so it is mounted on every branch. It also pulls once on mount, so a link that arrived before the renderer existed is not lost either.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
| const desktopWindow = yield* DesktopWindow.DesktopWindow; | ||
|
|
||
| const scheme = getDesktopScheme(environment.isDevelopment); | ||
| const pendingTargetRef = yield* Ref.make(Option.none<string>()); |
There was a problem hiding this comment.
🟠 High app/DesktopDeepLinks.ts:88
captureTarget stores pending links in a single Option ref, so when multiple deep links arrive during cold startup (before any renderer exists), each Ref.set overwrites the previous pending target. The first user-requested navigation is silently lost — only the last link survives to be delivered. Consider using a queue instead of a single slot so every captured target is delivered.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/desktop/src/app/DesktopDeepLinks.ts around line 88:
`captureTarget` stores pending links in a single `Option` ref, so when multiple deep links arrive during cold startup (before any renderer exists), each `Ref.set` overwrites the previous pending target. The first user-requested navigation is silently lost — only the last link survives to be delivered. Consider using a queue instead of a single slot so every captured target is delivered.
There was a problem hiding this comment.
Deliberate, and I have documented it rather than switching to a queue — happy to change it if you disagree.
These are navigation requests, and a queue converges on the same place: delivering A then B leaves the renderer on B, which is where delivering only B leaves it. The difference is that the queue first renders A, so the user watches a route they no longer asked for flash past. Coalescing to the newest link is the behaviour I would want as a user, and it matches how the OS treats repeated activations.
I did fix a related correctness bug you led me to, though: the clear-after-delivery step now only clears the slot if it still holds the target that was delivered, so a link arriving while a send is in flight is no longer discarded. Comment added at DesktopDeepLinks.ts:43 explaining the single slot.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 8 blocking correctness issues found. This PR introduces new deep link (URL protocol) handling - a security-sensitive feature processing external input. Multiple unresolved review comments identify a critical security vulnerability (URL bypass allowing navigation to malicious origins) and several high-severity bugs causing links to be lost or duplicated. You can customize Macroscope's approvability policy. Learn more. |
Review found the cold-start path never completed. Bootstrap only requests the backend start; the renderer is created later from handleBackendReady, so delivering right after bootstrap always found no renderer and left the link pending forever. sendToMainWindow also reported a deferred send as delivered, which let the sender discard a link that a closing window would never receive. The renderer now claims a pending link itself when it mounts, which is the first moment one can be received, and a push only counts once it reaches a loaded renderer. The listener moves to the always-rendered root view because the sidebar layout is absent on the pairing, connect, and unauthenticated screens, where a pushed link was dropped. Routing passes href rather than to so a link's query and fragment survive. Packaging declares only the production scheme. A packaged build resolves t3code from isDevelopment being false, so advertising t3code-dev claimed links it would then reject and outranked a dev run's registration. Logs and spans now record a link's route shape instead of its value, since the query or fragment of an externally supplied link can carry a one-time token. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks both — five of the six findings were real, and two of them (cold-start delivery, deferred-send accounting) meant deep links still would not have worked on a normal launch. Pushed dc752af with per-finding replies inline. The shape of the fix changed as a result: delivery is now pull-on-mount rather than push-from-startup. The renderer claims a pending link through a new Summary:
Verification: 420 desktop tests (5 new for the pull model and the log sanitiser), 684 web component tests, 210 contracts tests, |
There was a problem hiding this comment.
Effect service conventions review of the deep-link service and its call sites. The service definition (Context.Service tag with inline interface, make, layer), namespace imports, dependency acquisition from the environment, and the runPromiseWith usage at the Electron event boundary (matching DesktopClerk.configure) all look consistent with the repo's conventions. The earlier finding about the raw link target reaching logs and spans has been addressed via describeDeepLinkTarget. One minor deviation noted inline.
Posted via Macroscope — Effect Service Conventions
| import * as DesktopEnvironment from "./DesktopEnvironment.ts"; | ||
| import { makeComponentLogger } from "./DesktopObservability.ts"; | ||
|
|
||
| export { describeDeepLinkTarget, findDeepLinkTarget, parseDeepLinkTarget }; |
There was a problem hiding this comment.
These pure helpers now live in deepLinkTarget.ts (which is where DesktopWindow.ts imports describeDeepLinkTarget from), so this re-export leaves two import paths for the same functions and exists only so DesktopDeepLinks.test.ts can keep calling DesktopDeepLinks.parseDeepLinkTarget. Consider dropping the shim and importing the helpers from the canonical module in the test instead.
| export { describeDeepLinkTarget, findDeepLinkTarget, parseDeepLinkTarget }; |
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit dc752af. Configure here.
| return () => { | ||
| unmounted = true; | ||
| unsubscribe?.(); | ||
| }; |
There was a problem hiding this comment.
StrictMode drops pending deep link
Medium Severity
In development, StrictMode causes the useDesktopDeepLinks effect to run twice. The initial effect run calls takePendingDeepLink, clearing the pending deep link, but its navigation is skipped because the unmounted flag is set during cleanup. The subsequent effect run finds no pending link, causing launch-time deep links to be dropped.
Reviewed by Cursor Bugbot for commit dc752af. Configure here.
| const delivery = yield* sendToMainWindow(DEEP_LINK_CHANNEL, target, { | ||
| deferWhileLoading: false, | ||
| }); | ||
| return delivery === "sent"; |
There was a problem hiding this comment.
Deep link stuck while loading
Medium Severity
When a link arrives while the main window’s main frame is still loading, dispatchDeepLink returns without sending and leaves the link pending, but the renderer only calls takePendingDeepLink once on mount and otherwise waits for onDeepLink. If the hook already ran with nothing pending, there is no retry after load finishes and the route never opens.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit dc752af. Configure here.
| // registers the scheme on Windows. | ||
| if (environment.platform !== "darwin") { | ||
| yield* environment.isPackaged | ||
| ? electronApp.setAsDefaultProtocolClient(scheme) |
There was a problem hiding this comment.
🟡 Medium app/DesktopDeepLinks.ts:99
configure discards the boolean returned by electronApp.setAsDefaultProtocolClient, so a failed protocol registration is silently treated as success. On Windows, when the scheme registration fails (Electron returns false rather than throwing), t3code:// links remain unhandled with no startup error or diagnostic. Consider checking the return value and logging or failing the effect when it is false.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/desktop/src/app/DesktopDeepLinks.ts around line 99:
`configure` discards the boolean returned by `electronApp.setAsDefaultProtocolClient`, so a failed protocol registration is silently treated as success. On Windows, when the scheme registration fails (Electron returns `false` rather than throwing), `t3code://` links remain unhandled with no startup error or diagnostic. Consider checking the return value and logging or failing the effect when it is `false`.
| } | ||
|
|
||
| const target = `${parsedUrl.pathname}${parsedUrl.search}${parsedUrl.hash}`; | ||
| if (!target.startsWith("/") || target.startsWith("//")) { |
There was a problem hiding this comment.
🔴 Critical app/deepLinkTarget.ts:30
parseDeepLinkTarget only rejects targets starting with // but not /\, so a URL like t3code://app/\evil.example/phish yields the target /\evil.example/phish, passes the check, and is forwarded to the renderer. When the browser resolves a path starting with /\ against an HTTP(S) origin, it treats it like //, navigating to evil.example. This lets an untrusted deep link redirect the renderer to an attacker-controlled origin. Reject targets starting with \\ (or containing backslashes in the authority-like prefix), or resolve the target against the renderer origin and verify the resulting origin.
| if (!target.startsWith("/") || target.startsWith("//")) { | |
| if (!target.startsWith("/") || target.startsWith("//") || target.startsWith("/\\")) { |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/desktop/src/app/deepLinkTarget.ts around line 30:
`parseDeepLinkTarget` only rejects targets starting with `//` but not `/\`, so a URL like `t3code://app/\evil.example/phish` yields the target `/\evil.example/phish`, passes the check, and is forwarded to the renderer. When the browser resolves a path starting with `/\` against an HTTP(S) origin, it treats it like `//`, navigating to `evil.example`. This lets an untrusted deep link redirect the renderer to an attacker-controlled origin. Reject targets starting with `\\` (or containing backslashes in the authority-like prefix), or resolve the target against the renderer origin and verify the resulting origin.
| // Tries the renderer immediately for links that arrive while the app runs. | ||
| // Anything that cannot be handed over right now stays pending for the next | ||
| // renderer to pull, so nothing is dropped on the way. | ||
| const deliverPending = Effect.gen(function* () { |
There was a problem hiding this comment.
🟡 Medium app/DesktopDeepLinks.ts:55
deliverPending sends the pending link to the renderer and only clears the slot afterward, so during the in-flight dispatchDeepLink the renderer can call takePending and receive the same value — the deep link is dispatched twice, duplicating one-time-token navigations. The slot should be claimed atomically before delivery and restored only if delivery fails.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/desktop/src/app/DesktopDeepLinks.ts around line 55:
`deliverPending` sends the pending link to the renderer and only clears the slot afterward, so during the in-flight `dispatchDeepLink` the renderer can call `takePending` and receive the same value — the deep link is dispatched twice, duplicating one-time-token navigations. The slot should be claimed atomically before delivery and restored only if delivery fails.
1 participant
What Changed
Bug fix:
t3code://links now actually reach the app.protocolsmoves from themacblock to the top level of the desktop build config, so the Linux desktop entry gainsMimeType=x-scheme-handler/t3code;x-scheme-handler/t3code-dev;alongside the existing macOSCFBundleURLTypes, and a newDesktopDeepLinksservice claims the scheme at runtime and routes an incoming link to the renderer.Why
The scheme was declared for macOS only and nothing consumed an incoming URL, so clicking a
t3code://link did nothing on Linux or Windows and dropped its payload everywhere. Two independent gaps:MimeTypefromconfig.protocols+linux.protocols(LinuxTargetHelper.js:266) and the macOS Info.plist fromconfig.protocols+mac.protocols(electronMac.js:143). With the declaration undermac, Linux got nox-scheme-handlerandxdg-mime query default x-scheme-handler/t3coderesolved to nothing. NSIS readsprotocolsfrom neither, so Windows needs a runtime registration —setAsDefaultProtocolClientwas wrapped inElectronApp.tsbut had no callers.open-urllistener for macOS, and thesecond-instancelistener only revealed the window, discarding the argv that Linux and Windows deliver links through.Design notes for review:
apphost and forwards a path — never a full URL. A protocol-relative path (t3code://app//evil.example) is rejected because a router reads it as another origin.dispatchDeepLinkreports whether the send landed instead of silently dropping it;dispatchMenuActionand it now share onesendToMainWindowhelper.desktop:deep-linkchannel rather thanloadURL, so navigating doesn't reload the window and discard session state.clerk.configure, which owns the single-instance lock a second launch needs in order to hand its link to the running process.Verification
Built a Linux artifact and confirmed the packaged desktop entry in both the AppImage and the
.deb:13 new tests in
DesktopDeepLinks.test.tscover parsing (including the protocol-relative and foreign-host rejections), argv extraction, per-platform registration, and the cold-start hold-and-flush. Full desktop suite 417/417,build-desktop-artifact31/31,vp run -r typecheckclean across all 15 packages. The macOSopen-urland Windows registration paths are covered by tests but unverified on real hardware — I only have Linux here.Checklist
Note
Medium Risk
Touches untrusted URL input, protocol registration, and navigation without full-window reload; parsing and logging are constrained to safe paths, but OS handler registration and second-instance ordering add platform risk.
Overview
t3code://links now open the desktop app and navigate the renderer instead of being ignored on Linux/Windows or never consumed on macOS.A new
DesktopDeepLinksservice runs after Clerk’s single-instance lock: it registers the URL scheme on Linux/Windows (setAsDefaultProtocolClient), listens foropen-url(macOS) andsecond-instance(argv on Linux/Windows), and parses untrusted URLs into in-app paths only (t3code://app/...→/path?query#hash), rejecting foreign hosts and protocol-relative paths.Cold-start and delivery: one pending link slot (latest wins); the renderer pulls via
takePendingDeepLinkon mount and subscribes toonDeepLinkpushes.DesktopWindow.dispatchDeepLinksends over IPC without deferring while loading (unlike menu actions); failed delivery keeps the link pending.Web:
useDesktopDeepLinksin the root route navigates via TanStack Routerhrefso pairing/sign-in screens still receive links.Build:
protocolsmoves to the top-level electron-builder config with production scheme only (t3code); dev usest3code-devat runtime. Windows still registers at runtime (NSIS ignores build-time protocols).Reviewed by Cursor Bugbot for commit dc752af. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Register and route
t3code://deep links in the desktop appDesktopDeepLinksservice (DesktopDeepLinks.ts) that registers thet3codeURL scheme at startup, listens foropen-url(macOS) andsecond-instance(Linux/Windows) events, and holds a single pending link if no renderer is ready.parseDeepLinkTarget/findDeepLinkTargetutilities (deepLinkTarget.ts) to safely parse and discover deep-link targets from URLs andargv.DesktopWindow(DesktopWindow.ts) withdispatchDeepLink, which pushes a link to the loaded renderer over a newDEEP_LINK_CHANNELIPC channel and reveals the window.takePendingDeepLinkIPC method so the renderer can pull a cold-start link on mount, and auseDesktopDeepLinkshook (useDesktopDeepLinks.ts) that handles both push and pull navigation.t3codeas a top-level protocol for all platforms, replacing the prior mac-only block.did-finish-loadare held as pending and must be pulled via IPC rather than pushed.Macroscope summarized dc752af.