Repository navigation
fix: refuse to claim http(s) as a deep-link protocol #11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ import { type Ceremony, type CreateCeremonyOptions, createCeremony } from './cer | |
| import { assertValidClientId, createPublicWorkOS, toAuthKitConfig } from './config.js'; | ||
| import { | ||
| type AppLike, | ||
| isWebScheme, | ||
| registerProtocol as registerProtocolImpl, | ||
| wireDeepLinks, | ||
| } from './deep-link.js'; | ||
|
|
@@ -47,6 +48,11 @@ export interface CreateAuthKitResult { | |
| * after the first call (the listeners are wired exactly once), so it never | ||
| * leaks duplicate `open-url`/`second-instance` handlers. The deep-link | ||
| * listeners are removed by {@link CreateAuthKitResult.cleanup}. | ||
| * | ||
| * With an `http(s)` `redirectUri` there is no custom protocol to register: | ||
| * in `ceremony.mode: 'window'` (which captures its callback in-window) this | ||
| * is a no-op, and in `system-browser` mode it throws, because nothing would | ||
| * ever capture the callback. | ||
| */ | ||
| registerProtocol(): void; | ||
| /** Remove IPC handlers and deep-link listeners. Call on app shutdown. */ | ||
|
|
@@ -191,6 +197,22 @@ export function createAuthKit( | |
| if (protocolRegistered) { | ||
| return; | ||
| } | ||
| // An http(s) redirectUri is not a deep link. The window ceremony captures | ||
| // it in-window, so registration is simply unnecessary there; the | ||
| // system-browser ceremony has no other way to hear the callback, so a | ||
| // silent no-op would strand every sign-in — fail loudly instead. | ||
| if (isWebScheme(scheme)) { | ||
| if (config.ceremony?.mode === 'window') { | ||
| return; | ||
| } | ||
| throw new Error( | ||
| `redirectUri "${config.redirectUri}" uses the "${scheme}" scheme, which the OS ` + | ||
| `hands to the default browser, not to this app — the system-browser ceremony ` + | ||
| `would never receive the callback. Use a custom-protocol redirect URI ` + | ||
| `(e.g. "workos-auth://callback") and register it in the WorkOS Dashboard, or ` + | ||
| `switch to ceremony: { mode: 'window' }, which captures an https redirect in-app.`, | ||
| ); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔍 Mode detection uses config only, ignoring an injected ceremony The no-op/throw decision keys off Was this helpful? React with 👍 or 👎 to provide feedback.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair —
|
||
| } | ||
| registerProtocolImpl(scheme, { app: opts.app, process: opts.process }); | ||
| removeDeepLinks = wireDeepLinks( | ||
| scheme, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔍 Window-mode + https now skips the single-instance lock
For an
http(s)redirect URI inceremony.mode: 'window',registerProtocol()returns before callingwireDeepLinks, which is the only placeapp.requestSingleInstanceLock()/app.quit()are invoked (src/main/deep-link.ts:134-138). Previously (even in window mode) the lock was acquired as a side effect of wiring the deep-link matrix, so apps relying onregisterProtocol()for single-instance behavior will now allow multiple instances when configured with an https redirect URI. Worth documenting, or acquiring the lock independently of deep-link wiring.Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch — fixed in 9e3172c. The lock was only ever acquired as a side effect of
wireDeepLinks, which made it invisible collateral of the early return.Rather than duplicate the lock logic, I extracted
acquireSingleInstanceLock(onSecondInstance, { app })indeep-link.tsand madewireDeepLinkslayer URL extraction on top of it, so there is exactly one owner of the lock. The https + self-capturing-ceremony path now takes the lock, forwardsonSecondInstance, and releases it fromcleanup()— it just claims no protocol. Covered by a new assertion in the window-mode test plus two unit tests for the helper (forwarding + the duplicate-instancequit()path).