feat(electron): add http loopback redirect strategy for native OAuth#9044
Draft
nicolas-angelo wants to merge 2 commits into
Draft
feat(electron): add http loopback redirect strategy for native OAuth#9044nicolas-angelo wants to merge 2 commits into
nicolas-angelo wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 838b9cd The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited) 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:
📝 WalkthroughWalkthroughAdds an HTTP loopback redirect strategy ( ChangesHTTP Loopback OAuth Redirect Strategy
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
Contributor
API Changes Report
Summary
No API Changes DetectedAll packages have stable APIs with no detected changes. Report generated by Break Check Last ran on |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/electron/src/main/__tests__/oauth-transport.test.ts (1)
44-50: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffHardcoded loopback ports may cause flaky CI runs.
Each test binds the loopback server to a fixed port (
47654–47658). If any of these ports is already in use on the CI host, the server bind will fail and the test will become non-deterministically flaky. The handler derivesredirectUrlsynchronously from the configured port, so an ephemeral port (0) isn't directly usable here, but consider acquiring a free port at runtime (e.g. via a transientnet.Serverlisten-on-0 +address().port) before constructing the bridge to remove the collision risk.Also applies to: 66-72, 84-90, 108-112
🤖 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 `@packages/electron/src/main/__tests__/oauth-transport.test.ts` around lines 44 - 50, The OAuth transport tests are using fixed loopback ports, which can collide on shared CI hosts and make the suite flaky. Update the affected cases in oauth-transport.test.ts that call setupOAuthTransportIpcHandlers, getHandlers, and openExternal to obtain an available port at runtime instead of hardcoding values like 47654–47658; use a transient net.Server bound to port 0 to read back the assigned port, then pass that port into the redirect config before asserting getRedirectUrl and exercising open.
🤖 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.
Inline comments:
In `@packages/electron/src/main/oauth-transport.ts`:
- Around line 226-238: The error handler on the server created in
oauth-transport is not being removed correctly because removeListener is using a
different function reference than the one passed to once('error', ...). Update
the createServer/listen flow to assign the error callback to a named const and
use that same reference for both attaching and removing it so the listener is
actually detached after listen succeeds, and keep the server state cleanup logic
intact in the next server setup.
---
Nitpick comments:
In `@packages/electron/src/main/__tests__/oauth-transport.test.ts`:
- Around line 44-50: The OAuth transport tests are using fixed loopback ports,
which can collide on shared CI hosts and make the suite flaky. Update the
affected cases in oauth-transport.test.ts that call
setupOAuthTransportIpcHandlers, getHandlers, and openExternal to obtain an
available port at runtime instead of hardcoding values like 47654–47658; use a
transient net.Server bound to port 0 to read back the assigned port, then pass
that port into the redirect config before asserting getRedirectUrl and
exercising open.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 912cc93f-9931-4f20-980a-c9a52bd525ed
📒 Files selected for processing (10)
.changeset/loopback-oauth-transport.mdpackages/clerk-js/src/utils/__tests__/authenticateWithTransport.test.tspackages/clerk-js/src/utils/authenticateWithTransport.tspackages/electron/src/index.tspackages/electron/src/main/__tests__/create-clerk-bridge.test.tspackages/electron/src/main/__tests__/oauth-transport.test.tspackages/electron/src/main/create-clerk-bridge.tspackages/electron/src/main/oauth-redirect.tspackages/electron/src/main/oauth-transport.tspackages/electron/src/shared/types.ts
Comment on lines
+226
to
+238
| const next = createServer(requestListener); | ||
|
|
||
| next.once('error', error => { | ||
| server = null; | ||
| reject(error); | ||
| }); | ||
|
|
||
| next.listen(port, host, () => { | ||
| next.removeListener('error', reject); | ||
| server = next; | ||
| resolve(); | ||
| }); | ||
| }), |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- file slice ---\n'
sed -n '180,270p' packages/electron/src/main/oauth-transport.ts | cat -n
printf '\n--- search once(error) patterns ---\n'
rg -n "once\\('error'|removeListener\\('error'" packages/electron/src/main -S
printf '\n--- node eventemitter probe ---\n'
node - <<'JS'
const { EventEmitter } = require('events');
const ee = new EventEmitter();
function reject(err) { console.log('reject', err.message); }
const onceHandler = err => console.log('once', err.message);
ee.once('error', onceHandler);
console.log('listeners after once', ee.listeners('error').length);
console.log('raw listeners after once', ee.rawListeners('error').length);
console.log('listener identity matches original?', ee.listeners('error')[0] === onceHandler);
console.log('raw listener identity matches original?', ee.rawListeners('error')[0] === onceHandler);
ee.removeListener('error', reject);
console.log('listeners after removeListener(reject)', ee.listeners('error').length);
console.log('raw listeners after removeListener(reject)', ee.rawListeners('error').length);
ee.removeListener('error', onceHandler);
console.log('listeners after removeListener(onceHandler)', ee.listeners('error').length);
console.log('raw listeners after removeListener(onceHandler)', ee.rawListeners('error').length);
JSRepository: clerk/javascript
Length of output: 245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- file outline ---'
ast-grep outline packages/electron/src/main/oauth-transport.ts --view expanded || true
echo
echo '--- wider slice around the relevant helper ---'
sed -n '1,340p' packages/electron/src/main/oauth-transport.ts | cat -n
echo
echo '--- EventEmitter listener-removal probe ---'
node - <<'JS'
const { EventEmitter } = require('events');
const ee = new EventEmitter();
function reject(err) { console.log('reject', err.message); }
const onError = err => console.log('onError', err.message);
ee.once('error', onError);
console.log('listeners length after once:', ee.listeners('error').length);
console.log('rawListeners length after once:', ee.rawListeners('error').length);
console.log('listeners[0] === onError:', ee.listeners('error')[0] === onError);
console.log('rawListeners[0] === onError:', ee.rawListeners('error')[0] === onError);
ee.removeListener('error', reject);
console.log('after removeListener(reject): listeners=', ee.listeners('error').length, 'rawListeners=', ee.rawListeners('error').length);
ee.removeListener('error', onError);
console.log('after removeListener(onError): listeners=', ee.listeners('error').length, 'rawListeners=', ee.rawListeners('error').length);
JSRepository: clerk/javascript
Length of output: 12783
removeListener('error', reject) is a no-op here. The once('error', …) handler stays attached after listen succeeds, so a later server error can still clear server and leave cleanup unable to close it. Store the callback in a named const and remove that same reference.
🤖 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 `@packages/electron/src/main/oauth-transport.ts` around lines 226 - 238, The
error handler on the server created in oauth-transport is not being removed
correctly because removeListener is using a different function reference than
the one passed to once('error', ...). Update the createServer/listen flow to
assign the error callback to a named const and use that same reference for both
attaching and removing it so the listener is actually detached after listen
succeeds, and keep the server state cleanup logic intact in the next server
setup.
nicolas-angelo
force-pushed
the
feat/electron-loopback-oauth-transport
branch
from
c6e7c36 to
31df9ba
Compare
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
2 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.
Description
This builds off @wobsoriano's work; the loopback OAuth approach in the
loopbackbranch of hisclerk-electron-example.Adds an
httploopback redirect strategy for native OAuth in@clerk/electron(alongside the existing, default custom-scheme deep link) — and fixes the browser-side completion so the OAuth flow's hosted callback page resolves instead of hanging.Completing the OAuth handshake (the key fix)
Example:
After attempting sign-in with Vercel, the OAuth callback page would hang in a perpetual "waiting" state in the system browser — even though the Electron app had already signed the user in.
The native transport set only
redirect_urlto the transport callback and leftaction_complete_redirect_urlpointing at the in-app URL (theclerk://app/custom scheme). The system browser can't navigate to a custom scheme, so Clerk's final (action-complete) redirect never resolved — the flow never signaled completion in the browser, leaving the tab stalled.This PR backs both
redirect_urlandaction_complete_redirect_urlwith the transport's redirect URL, so the provider's action-complete redirect lands on a reachable listener and the browser tab resolves to a real completion page.New
httploopback strategyReceives the callback on a short-lived
http://127.0.0.1:<port>server (RFC 8252 §7.3) and serves a completion page (or302s to your own URL). Needs no OS protocol registration. Selected via a discriminatedoauth.redirectunion, withhttpRedirectStrategy()/deepLinkRedirectStrategy()helpers. The custom-scheme deep link remains the default.Usage
For the
httpstrategy, addhttp://127.0.0.1:<port>/sso-callbackto your instance's allowed redirect URLs.Testing: sign in with an OAuth provider from the Electron app and confirm the browser tab resolves to the completion page (no "waiting" hang) and the app ends up signed in — with both the default deep link and
httpRedirectStrategy().Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change