Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .oxfmtrc.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{
"singleQuote": true,
"ignorePatterns": ["dist", "coverage", "node_modules"]
"ignorePatterns": ["dist", "coverage", "node_modules", "CHANGELOG.md"]
}
8 changes: 7 additions & 1 deletion src/internals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,13 @@ export { createCeremony } from './main/ceremony/index.js';
export type { Ceremony, CreateCeremonyOptions, ShellLike } from './main/ceremony/index.js';

// Deep-link capture matrix + callback URL parsing.
export { parseCallback, registerProtocol, wireDeepLinks } from './main/deep-link.js';
export {
acquireSingleInstanceLock,
isWebScheme,
parseCallback,
registerProtocol,
wireDeepLinks,
} from './main/deep-link.js';
export type { AppLike, ProcessLike, WireDeepLinksOptions } from './main/deep-link.js';

// IPC handlers + auth-change / auth-error broadcast.
Expand Down
75 changes: 75 additions & 0 deletions src/main/__tests__/create-auth-kit.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,81 @@ describe('createAuthKit — wiring', () => {
expect(app.setAsDefaultProtocolClient).toHaveBeenCalledTimes(1);
});

// Regression: schemeFromRedirectUri happily returns "https", which used to
// flow into setAsDefaultProtocolClient('https') — a request to become the
// user's default browser, and a callback that never arrives.
it('registerProtocol throws for an https redirectUri in system-browser mode', () => {
const app = makeApp();
const kit = createAuthKit(
{ clientId: 'client_test', redirectUri: 'https://auth.example.com/callback' },
{
storage: makeStorage(),
client: { userManagement: {} } as unknown as WorkOS,
ipcMain: makeIpcMain(),
app,
process: { argv: ['electron'], execPath: '/e' },
},
);

expect(() => kit.registerProtocol()).toThrow(/workos-auth:\/\/callback|mode: 'window'/);
expect(app.setAsDefaultProtocolClient).not.toHaveBeenCalled();
});

it('registerProtocol claims no protocol for an https redirectUri in window mode', () => {
const app = makeApp();
const kit = createAuthKit(
{
clientId: 'client_test',
redirectUri: 'https://auth.example.com/callback',
ceremony: { mode: 'window' },
},
{
storage: makeStorage(),
client: { userManagement: {} } as unknown as WorkOS,
ipcMain: makeIpcMain(),
app,
process: { argv: ['electron'], execPath: '/e' },
},
);

// The window ceremony intercepts the redirect in-window, so there is no
// protocol to claim and no deep-link listener to wire...
expect(() => kit.registerProtocol()).not.toThrow();
expect(app.setAsDefaultProtocolClient).not.toHaveBeenCalled();
expect(app.listeners['open-url']).toBeUndefined();
// ...but the single-instance lock is not a deep-link concern, and callers
// rely on registerProtocol() for it, so it is still acquired and still
// released by cleanup().
expect(typeof app.listeners['second-instance']).toBe('function');
expect(() => kit.cleanup()).not.toThrow();
});

// The decision keys off the ceremony instance, not config.ceremony.mode, so an
// injected self-capturing ceremony is not told its redirect URI is broken.
it('registerProtocol respects an injected self-capturing ceremony', () => {
const app = makeApp();
const selfCapturing: Ceremony = {
capturesCallback: true,
open: vi.fn(async () => {}),
endSession: vi.fn(async () => {}),
onCallback: () => () => {},
};
const kit = createAuthKit(
{ clientId: 'client_test', redirectUri: 'https://auth.example.com/callback' },
{
storage: makeStorage(),
client: { userManagement: {} } as unknown as WorkOS,
ipcMain: makeIpcMain(),
ceremony: selfCapturing,
app,
process: { argv: ['electron'], execPath: '/e' },
},
);

expect(() => kit.registerProtocol()).not.toThrow();
expect(app.setAsDefaultProtocolClient).not.toHaveBeenCalled();
});

it('cleanup removes handlers without throwing', () => {
const kit = createAuthKit(config, {
storage: makeStorage(),
Expand Down
52 changes: 51 additions & 1 deletion src/main/__tests__/deep-link.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
import { describe, expect, it, vi } from 'vitest';
import type { AppLike, ProcessLike } from '../deep-link.js';
import { parseCallback, registerProtocol, wireDeepLinks } from '../deep-link.js';
import {
acquireSingleInstanceLock,
isWebScheme,
parseCallback,
registerProtocol,
wireDeepLinks,
} from '../deep-link.js';

// The module imports `app` from electron at the top; tests inject their own
// `app`, so this mock just prevents the native binding from loading.
Expand Down Expand Up @@ -89,6 +95,50 @@ describe('registerProtocol', () => {
registerProtocol(SCHEME, { app, process: proc({ defaultApp: false }) });
expect(app.setAsDefaultProtocolClient).toHaveBeenCalledWith('workos-auth');
});

// Regression: an https redirectUri used to reach setAsDefaultProtocolClient,
// which asks the OS to make the app the DEFAULT BROWSER (hostile on Windows)
// and can never deliver an OAuth callback.
it('refuses http/https and never touches setAsDefaultProtocolClient', () => {
for (const scheme of ['http', 'https', 'HTTPS']) {
const app = makeApp();
expect(() => registerProtocol(scheme, { app, process: proc() })).toThrow(/default browser/);
expect(app.setAsDefaultProtocolClient).not.toHaveBeenCalled();
}
});
});

describe('acquireSingleInstanceLock', () => {
it('forwards a second instance argv and releases the listener on cleanup', () => {
const app = makeApp();
const onSecondInstance = vi.fn();

const release = acquireSingleInstanceLock(onSecondInstance, { app });

expect(release).not.toBeNull();
app.listeners['second-instance']?.({} as never, ['electron', '--flag'] as never);
expect(onSecondInstance).toHaveBeenCalledWith(['electron', '--flag']);

release?.();
expect(app.removed).toContain('second-instance');
});

it('quits and returns null when the lock is already held', () => {
const app = makeApp(false);

expect(acquireSingleInstanceLock(vi.fn(), { app })).toBeNull();
expect(app.quit).toHaveBeenCalledTimes(1);
expect(app.listeners['second-instance']).toBeUndefined();
});
});

describe('isWebScheme', () => {
it('matches http/https case-insensitively and nothing else', () => {
expect(isWebScheme('http')).toBe(true);
expect(isWebScheme('Https')).toBe(true);
expect(isWebScheme('workos-auth')).toBe(false);
expect(isWebScheme('httpsx')).toBe(false);
});
});

describe('wireDeepLinks', () => {
Expand Down
13 changes: 13 additions & 0 deletions src/main/ceremony/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,19 @@ import {
} from './window.js';

export interface Ceremony {
/**
* Does this ceremony capture its own callback URL (delivering it through
* {@link Ceremony.onCallback}) rather than relying on the OS deep-link
* handler? True for window mode, false for the system browser.
*
* `createAuthKit` reads this to decide what a redirect URI the OS will never
* hand back — an `http(s)` one — means: irrelevant for a self-capturing
* ceremony, a fatal misconfiguration for one that depends on the deep link.
* Keying off the ceremony instance (rather than `config.ceremony.mode`) means
* an injected self-capturing ceremony behaves correctly too. Treated as false
* when omitted.
*/
capturesCallback?: boolean;
/** Take the user to the authorization URL to begin authentication. */
open(url: string): Promise<void>;
/**
Expand Down
3 changes: 3 additions & 0 deletions src/main/ceremony/system-browser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ export function createSystemBrowserCeremony(opts: CreateCeremonyOptions = {}): C
const shell: ShellLike = opts.shell ?? electronShell;

return {
// The callback comes back out-of-band through the OS deep-link handler, so
// this ceremony depends on a claimable custom protocol.
capturesCallback: false,
async open(url: string): Promise<void> {
await shell.openExternal(url);
},
Expand Down
3 changes: 3 additions & 0 deletions src/main/ceremony/window.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,9 @@ export function createWindowCeremony(opts: CreateWindowCeremonyOptions): Ceremon
};

return {
// Navigation interception means the redirect never escapes to the OS, so no
// custom protocol needs claiming for this ceremony's callback to arrive.
capturesCallback: true,
async open(url: string): Promise<void> {
const win = createWindow({ parent: opts.parent, modal: opts.parent !== undefined });

Expand Down
37 changes: 33 additions & 4 deletions src/main/create-auth-kit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import { type Ceremony, type CreateCeremonyOptions, createCeremony } from './cer
import { assertValidClientId, createPublicWorkOS, toAuthKitConfig } from './config.js';
import {
type AppLike,
acquireSingleInstanceLock,
isWebScheme,
registerProtocol as registerProtocolImpl,
wireDeepLinks,
} from './deep-link.js';
Expand Down Expand Up @@ -47,6 +49,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: a
* self-capturing ceremony (`ceremony.mode: 'window'`) still acquires the
* single-instance lock but claims no protocol, while `system-browser` mode
* throws, because nothing would ever capture the callback.
*/
registerProtocol(): void;
/** Remove IPC handlers and deep-link listeners. Call on app shutdown. */
Expand Down Expand Up @@ -139,7 +146,7 @@ export function createAuthKit(
});

const scheme = schemeFromRedirectUri(config.redirectUri);
let removeDeepLinks: (() => void) | null = null;
let removeOsListeners: (() => void) | null = null;
let protocolRegistered = false;

/** Parse a deep-link callback URL and complete the OAuth round-trip. */
Expand Down Expand Up @@ -191,8 +198,30 @@ export function createAuthKit(
if (protocolRegistered) {
return;
}
// An http(s) redirectUri is not a deep link. A self-capturing ceremony gets
// its callback in-window, so there is nothing to claim; one that depends on
// the deep link 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 (!ceremony.capturesCallback) {
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.`,
);
}
Comment on lines +205 to +214

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).

// No protocol to claim — but the single-instance lock is not a deep-link
// concern, and callers rely on registerProtocol() for it, so still take it.
removeOsListeners = acquireSingleInstanceLock(() => opts.onSecondInstance?.(), {
app: opts.app,
});
protocolRegistered = true;
return;
}
registerProtocolImpl(scheme, { app: opts.app, process: opts.process });
removeDeepLinks = wireDeepLinks(
removeOsListeners = wireDeepLinks(
scheme,
(url) => {
void handleCallbackUrl(url);
Expand All @@ -205,8 +234,8 @@ export function createAuthKit(
function cleanup(): void {
removeIpcHandlers();
removeCeremonyCallback();
removeDeepLinks?.();
removeDeepLinks = null;
removeOsListeners?.();
removeOsListeners = null;
protocolRegistered = false;
}

Expand Down
Loading
Loading