Skip to content

fix: refuse to claim http(s) as a deep-link protocol - #11

Merged
nicknisi merged 2 commits into
mainfrom
fix/reject-http-redirect-uri
Aug 14, 2026
Merged

fix: refuse to claim http(s) as a deep-link protocol#11
nicknisi merged 2 commits into
mainfrom
fix/reject-http-redirect-uri

Conversation

@nicknisi

Copy link
Copy Markdown
Member

Problem

schemeFromRedirectUri accepts any scheme:// prefix, including https. So an https://auth.example.com/callback redirect URI flowed straight into setAsDefaultProtocolClient('https') — which does not capture an OAuth callback. It asks the OS to make the app the user's default browser (actively hostile on Windows). The callback then never arrived, with no error to explain why.

This came up as feedback on the launch thread: WorkOS/macOS now allow https:// callbacks via ASWebAuthenticationSession, so people will reasonably try one here. Today that silently strands sign-in.

Change

  • deep-link.ts: new isWebScheme() (exported via internals); registerProtocol() throws for http/https rather than reaching setAsDefaultProtocolClient.
  • create-auth-kit.ts: registerProtocol() is mode-aware for an http(s) redirect URI —
    • ceremony: { mode: 'window' }no-op. The window intercepts the redirect before it commits, so there is no protocol to claim and an https redirect URI already works there.
    • system-browser (default) → throws, naming both escapes: a custom scheme (workos-auth://callback) or window mode. Nothing else could ever hear the callback, so a silent no-op would be worse.

No behavior change for the custom-scheme path.

Tests

+6 across deep-link.spec.ts / create-auth-kit.spec.ts: refusal is case-insensitive, setAsDefaultProtocolClient is never reached, and window mode wires no open-url listener.

typecheck, lint, and all 157 tests pass.

Not in scope

Real ASWebAuthenticationSession support (native addon, a ceremony: 'native' mode, macOS 14.4+ with the webcredentials associated-domains entitlement for https callbacks, plus passkeys working in the default browser). Worth a separate issue — Electron has no built-in API for it, so it means shipping a native module from a package that currently has none.

An `https://` redirectUri passed `schemeFromRedirectUri` unchallenged, so
`registerProtocol()` called `setAsDefaultProtocolClient('https')` — which
does not capture an OAuth callback, it asks the OS to make the app the
user's default browser (actively hostile on Windows). The callback then
never arrived and the failure was silent.

`registerProtocol()` in deep-link.ts now throws for `http`/`https`, and
`createAuthKit`'s `registerProtocol()` is mode-aware: a no-op under
`ceremony: { mode: 'window' }` (the window intercepts the redirect itself,
so there is no protocol to claim) and a throw under `system-browser`,
naming both escapes — a custom scheme, or window mode.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

Open in Devin Review

Comment on lines +204 to +207
if (isWebScheme(scheme)) {
if (config.ceremony?.mode === 'window') {
return;
}

Copy link
Copy Markdown

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 in ceremony.mode: 'window', registerProtocol() returns before calling wireDeepLinks, which is the only place app.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 on registerProtocol() 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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Member Author

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 }) in deep-link.ts and made wireDeepLinks layer 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, forwards onSecondInstance, and releases it from cleanup() — 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-instance quit() path).

Comment thread src/main/create-auth-kit.ts Outdated
Comment on lines +205 to +214
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.`,
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 config.ceremony?.mode === 'window', not the ceremony instance actually in use. A consumer who passes a custom/window-capable Ceremony via opts.ceremony (exported publicly through CreateAuthKitOptions in src/index.ts) while leaving config.ceremony unset will get a thrown error from registerProtocol() even though their ceremony captures the callback in-app. If opts.ceremony is meant purely as a test seam this is fine; otherwise consider detecting self-capturing ceremonies (or documenting that config.ceremony.mode must be set alongside an injected ceremony).

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair — CreateAuthKitOptions is publicly exported from src/index.ts, so treating opts.ceremony as test-only was an assumption not worth relying on. Fixed in 9e3172c by asking the collaborator instead of a parallel config field.

Ceremony now declares capturesCallback?: booleantrue on the window ceremony (navigation interception means the redirect never escapes to the OS), explicitly false on the system-browser one. registerProtocol() checks ceremony.capturesCallback and no longer reads config.ceremony?.mode at all, so an injected self-capturing ceremony is never told its redirect URI is broken. Optional rather than required so it is not a breaking change for anyone implementing the interface; omitted reads as false, which is the safe default (you get the loud error rather than a silently dead callback). New test injects a bare self-capturing ceremony with an https redirect URI and asserts no throw.

@greptile-apps

greptile-apps Bot commented Aug 12, 2026

Copy link
Copy Markdown

Greptile Summary

The PR prevents Electron applications from claiming browser-owned HTTP(S) protocols while preserving HTTP(S) callbacks for self-capturing window ceremonies.

  • Rejects HTTP and HTTPS in low-level protocol registration.
  • Makes registration ceremony-aware, throwing for system-browser mode and skipping OS deep-link wiring in window mode.
  • Extracts single-instance lock handling so window mode retains second-instance lifecycle behavior.
  • Adds regression coverage for protocol refusal, callback capture modes, and lock cleanup.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
src/main/create-auth-kit.ts Adds ceremony-aware handling that rejects unusable HTTP(S) system-browser callbacks while retaining in-window callback capture and single-instance locking.
src/main/deep-link.ts Prevents registration of browser-owned schemes and cleanly extracts reusable single-instance lock lifecycle handling.
src/main/ceremony/window.ts Declares that window ceremonies capture callback navigation directly.
src/main/ceremony/system-browser.ts Declares that system-browser ceremonies depend on OS deep-link delivery.
src/main/tests/create-auth-kit.spec.ts Covers mode-aware HTTP(S) registration behavior and confirms window mode avoids protocol and open-url registration.
src/main/tests/deep-link.spec.ts Covers case-insensitive web-scheme refusal and single-instance listener acquisition and cleanup.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[registerProtocol] --> B{Redirect scheme is HTTP or HTTPS?}
  B -- No --> C[Register custom protocol]
  C --> D[Wire open-url and second-instance listeners]
  B -- Yes --> E{Ceremony captures callback?}
  E -- No --> F[Throw configuration error]
  E -- Yes --> G[Acquire single-instance lock only]
  G --> H[Window ceremony intercepts callback navigation]
  H --> I[Deliver URL through ceremony onCallback]
Loading

Reviews (2): Last reviewed commit: "fix: keep the single-instance lock, and ..." | Re-trigger Greptile

Review follow-ups:

- `registerProtocol()` used to acquire the single-instance lock only as a
  side effect of wiring the deep-link matrix, so the https + window-mode
  early return silently allowed a second app instance. The lock is not a
  deep-link concern: extract `acquireSingleInstanceLock()` and take it on
  both paths. `wireDeepLinks` now layers URL extraction on top of it, so
  the lock has exactly one owner.
- The no-op/throw decision keyed off `config.ceremony?.mode`, which
  ignored a self-capturing ceremony injected through the (publicly
  exported) `CreateAuthKitOptions.ceremony`. `Ceremony` now declares
  `capturesCallback`, and the guard asks the ceremony in use.

Also stop formatting CHANGELOG.md: release-please generates it in a shape
oxfmt rejects, which has had `format:check` failing on main since 9530d70
and would re-break at every release if reformatted by hand.
@nicknisi
nicknisi merged commit e73bde0 into main Aug 14, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant