Skip to content

fix(desktop): restore Windows OAuth callbacks in unpackaged builds - #22486

Open
AskPlays wants to merge 4 commits into
pingdotgg:mainfrom
AskPlays:fix/windows-oauth-runtime-verified
Open

AskPlays wants to merge 4 commits into
pingdotgg:mainfrom
AskPlays:fix/windows-oauth-runtime-verified

Conversation

@AskPlays

@AskPlays AskPlays commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Windows browser OAuth callbacks in unpackaged production builds launch bare Electron with the callback URL as its app path, producing “Unable to find Electron app.” Register the scheme after Clerk initializes with the running executable and absolute dist-electron/main.cjs. Development and packaged builds remain excluded.

Restores #15995 after the batch rollback in #22222, following the request for a fresh PR with real desktop evidence. Cherry-picks the original fix, development exclusion and boundary tests, preserving author attribution and current main's early single-instance locking.

Windows production runtime proof

Tested on Windows 11 Pro x64 (26200), Electron 44.4.5, against baseline dac3437d94 and this branch's 51f255299a. Built the full app with vp run build:desktop; after applying the fix, rebuilt the production desktop bundles with vp pack, reusing the unchanged server/web artifacts. Both runs used the actual built desktop window and local backend, without VITE_DEV_SERVER_URL.

The sandbox started with existing Windows state copied from the installed app: Electron profile, desktop/client preferences, keybindings, and a read-only database snapshot containing 3 projects, 9 stopped/settled threads, 304 messages and 5,116 events. Live/recoverable work, scheduled tasks and pending effects were excluded. Existing settled threads appeared in the native production window before and after the fix; the Browser panel also loaded the sandbox's production client successfully.

Before, real Windows registration omitted the entry point (paths abbreviated):

"electron.exe" "%1"
callback invocation: t3code://app/?__clerk_ticket=synthetic-production-pr15995
Error launching app: Unable to find Electron app at ...\t3code:\app\?...
callback process exit: 1
primary Clerk callback: not delivered

The invocation used the registered executable with a diagnostic --require module that captured Electron's native error dialog instead of blocking on it.

After, the same real build and copied state registered and delivered the callback through Windows:

"electron.exe" "<checkout>\apps\desktop\dist-electron\main.cjs" "%1"
shell.openExternal -> Windows protocol association -> app entry
second-instance: primary PID 46644, argv includes main.cjs and exact callback URL
Clerk callback resolved: PID 46644, exact synthetic callback URL
native renderer: still alive; existing settled history still visible

A diagnostic observer used the real Clerk IPC transport and actual main-window WebContents. Only the outgoing synthetic HTTPS authorization launch was suppressed; callback registration, Windows shell dispatch, secondary-process launch, single-instance forwarding and Clerk resolution used the real production runtime. This initial check verified synthetic callback transport.

Manual real sign-in also passed: @AskPlays subsequently completed browser sign-in from the built desktop app and confirmed that sign-in worked. Runtime diagnostics recorded the browser callback reaching the existing primary desktop process (PID 52492), with no renderer failure. The manual run used the same copied existing state. A diagnostic prelude pinned only the sandbox home/profile environment for browser-launched callback processes; production OAuth and protocol-registration logic were unchanged. Callback parameters were redacted from diagnostics.

After both runs, project/thread/message/event counts were unchanged and the SHA256 of all message projection rows matched the pre-run snapshot (70dd258fc3047032a39293346c41544e51b528f0226217ca1ddacea286b27d03). The original installed Nightly protocol handler was restored. The live app and database were untouched. Private thread screenshots were retained locally rather than published.

Focused checks

  • DesktopClerk.test.ts: 22 pass / 1 fails with the registration block removed; 23 pass with the fix restored.
  • Desktop tsc --noEmit, lint/formatting for the two changed files, and git diff --check: pass.
  • Boundary tests cover Windows paths containing spaces and the development/packaged/macOS/Linux exclusions. Only unpackaged, non-development Windows changes; those excluded platforms were not exercised live. Web/mobile authentication and provider adapters are unchanged.

Restoration and Windows verification: GPT-6.1-Sol via the Codex harness in T3 Code.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Oct 20, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This PR changes Windows protocol registration used to deliver OAuth callbacks in unpackaged builds, with focused tests for the affected boundaries. Because it modifies authentication-related runtime behavior, human review is warranted despite the limited scope.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 62ee675c-4b38-44db-bbe9-13da4c6587dd
📥 Commits

Reviewing files that changed from the base of the PR and between 51f2552 and 8983b04.

📒 Files selected for processing: 1
  • apps/desktop/src/app/DesktopClerk.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

DesktopClerk registers the desktop protocol client on unpackaged, non-development Windows installations. Parameterized tests cover Windows, macOS, and Linux across packaged and development configurations.

Changes: Windows protocol registration

Layer / File(s) Summary
Protocol registration and coverage
apps/desktop/src/app/DesktopClerk.ts, apps/desktop/src/app/DesktopClerk.test.ts
DesktopClerk.configure registers the desktop protocol client on unpackaged, non-development Windows. Tests verify eligible and excluded configurations with platform-specific path dependencies.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 8983b

The Windows callback registration uses a supported desktop entry path. No issue identified here prevents merging after normal checks.

Security architecture risk: 🔵 Low · up to 51f25

The change is narrowly limited to unpackaged, non-development Windows builds and preserves early single-instance locking. No introduced security defect was established. Failure and rollback behavior remain unverified, and the locally available authentication bridge differs from the version required by the application.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The directly affected security boundary is Windows dispatch into the unpackaged desktop process and its authentication and deep-link consumers. Restoring dispatch makes externally supplied URLs effective on this launch path, so unchanged consumers still matter to the assessment; the change itself introduces no new URL-derived executable or bundle selection.

Trust Boundaries and Controls

  • observed — The application-owned handlers retain explicit restrictions: Codex handoffs validate the scheme, host, authorization endpoint, loopback redirect and request parameters; callback delivery checks the expected state. Provider returns restrict destinations and paths, and DesktopClerk additionally requires its current desktop origin. These controls do not establish the separate Clerk ticket consumer's behavior.

Resilience and Maintainability Implications

  • observed — Existing lock ownership prevents secondary processes from creating another Clerk bridge or reaching the new registration. Bridge cleanup is scoped, and early application handoff arguments are buffered. These process-lifecycle controls do not prove atomicity or recovery of the separate Windows association update.
Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: restoring Windows OAuth callbacks for unpackaged desktop builds.
Description check Passed The description explains the problem, implementation, scope, approval context, Windows runtime evidence, focused tests, and limitations. It does not use the template's exact section headings, but it p…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description explains the problem, implementation, scope, approval context, Windows runtime evidence, focused tests, and limitations. It does not use the template's exact section headings, but it provides the required information in a complete and relevant form.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants