diff --git a/README.md b/README.md index 7e8d71a..317487b 100644 --- a/README.md +++ b/README.md @@ -166,6 +166,39 @@ mountOAuthLogin(oauthLoginApi, { `callbackUrl` is the absolute public URL of that route; it is registered verbatim as the redirect_uri. +A refused redirect is reported as a code — `state_mismatch`, `provider_error` +(carrying the authorization server's own `error`), `no_code` — not as prose. +What to tell the person differs by product and brand; what the redirect was +does not. + +#### Retrying a sign-in + +The authorization server accepts one registered redirect_uri per client, so +every attempt for a provider contends for the same loopback port, and the +authorize page already open in the browser is bound to the state and PKCE +verifier of the attempt that opened it. A second attempt should therefore +resume the first, not replace it. `createLoginRegistry` holds one live login +per key and does that: + +```ts +import { createLoginRegistry } from "@corbits/oauth-core"; + +const logins = createLoginRegistry(); + +const { handle, resumed } = await logins.startOrResume( + "codex", + () => startOAuthLogin({ profile: "default", signal }, deps), + // Only the principal who started a login resumes it. + { tag: `${tenantId}:${principalId}` }, +); +``` + +An entry is released when its login settles, so the next attempt after a +completed, failed or cancelled one starts clean. `logins.cancel(key)` ends +the login in flight and frees the port. `mountOAuthLogin` uses this already: +a start that collides with a pending login answers `200` with that login's +id rather than `409`. + ## Upgrading from 0.1 - `OAuthLoginProvider` and `callbackTargetFor` import from `@corbits/oauth-core`, not `@corbits/oauth-core/hub`. diff --git a/src/callback-server.test.ts b/src/callback-server.test.ts index 133b5ef..6c0d720 100644 --- a/src/callback-server.test.ts +++ b/src/callback-server.test.ts @@ -4,6 +4,7 @@ import { OAuthCallbackAddressUnavailableError, OAuthCallbackPortInUseError, startCallbackServer, + type CallbackFailure, } from "./index.js"; const config = (port: number, host?: string) => ({ @@ -11,7 +12,8 @@ const config = (port: number, host?: string) => ({ ...(host === undefined ? {} : { host }), path: "/callback", doneHtml: "done", - failedHtml: (reason: string) => `failed: ${reason}`, + failedHtml: (failure: CallbackFailure) => + `failed: ${failure.code}${failure.providerError === undefined ? "" : `:${failure.providerError}`}`, }); describe("Callback server startCallbackServer — state validation", () => { @@ -98,17 +100,32 @@ describe("Callback server startCallbackServer — state validation", () => { const server = await startCallbackServer("expected-state", config(0)); try { const waiting = server.waitForCode(new AbortController().signal); - const forged = await fetch( - `http://127.0.0.1:${server.port}/callback?code=forged&state=wrong-state`, + const refused = await fetch( + `http://127.0.0.1:${server.port}/callback?code=attacker-code&state=wrong-state`, ); - expect(forged.status).toBe(400); - const real = await fetch( - `http://127.0.0.1:${server.port}/callback?code=real&state=expected-state`, + expect(refused.status).toBe(400); + expect(await refused.text()).toContain("state_mismatch"); + + // The refusal does not end the login: anything on the machine can reach + // this port, so a stray redirect must not be able to cancel the real one. + const genuine = await fetch( + `http://127.0.0.1:${server.port}/callback?code=real-code&state=expected-state`, ); - expect(real.status).toBe(200); - expect(await waiting).toBe("real"); + expect(genuine.status).toBe(200); + expect(await waiting).toBe("real-code"); } finally { server.close(); } }); + + test("closing before a redirect ends the wait", async () => { + const server = await startCallbackServer("state", config(0)); + const waiting = server.waitForCode(new AbortController().signal); + server.close(); + + // Without this a caller that gave up would await a redirect that can no + // longer be received, and anything tracking the login would never learn + // it had ended. + await expect(waiting).rejects.toThrow(/closed before an authorization/); + }); }); diff --git a/src/callback-server.ts b/src/callback-server.ts index 70db2f2..30e82f2 100644 --- a/src/callback-server.ts +++ b/src/callback-server.ts @@ -55,18 +55,49 @@ export type CallbackServer = { close: () => void; }; +/** + * Why a redirect was refused, as a code rather than prose. + * + * What a person should be told differs by product and by brand, and none of + * it is this module's to decide; what the redirect actually was is. Callers + * map these onto their own copy. + * + * - `state_mismatch`: the redirect carried a state this listener did not + * issue, so the authorization it belongs to is not the one being waited on. + * - `provider_error`: the authorization server itself reported a failure; + * `providerError` carries its `error` parameter verbatim. + * - `no_code`: the redirect arrived with neither an authorization code nor + * an error to explain its absence. + */ +export type CallbackFailureCode = + | "state_mismatch" + | "provider_error" + | "no_code"; + +export type CallbackFailure = { + readonly code: CallbackFailureCode; + /** The authorization server's own `error` parameter, when it sent one. */ + readonly providerError?: string; +}; + export type CallbackServerConfig = { port: number; host?: string; path: string; doneHtml: string; - failedHtml: (reason: string) => string; + failedHtml: (failure: CallbackFailure) => string; }; +/** Why the wait for a code ended: a refused redirect, or the caller aborting. */ +export type CallbackErrorCode = CallbackFailureCode | "aborted"; + export class OAuthCallbackError extends Error { - constructor(message: string) { + readonly code: CallbackErrorCode; + + constructor(code: CallbackErrorCode, message: string) { super(message); this.name = "OAuthCallbackError"; + this.code = code; } } @@ -153,24 +184,36 @@ export async function startCallbackServer( const error = url.searchParams.get("error"); const state = url.searchParams.get("state"); + // Refused, but the wait goes on. The listener is on a fixed loopback + // port anything on the machine can reach, and a redirect that is not + // this login's -- a stale tab, or a page firing a crafted link -- says + // nothing about the one being waited on. Ending the login here would let + // any such request cancel a legitimate sign-in. if (state !== expectedState) { res.statusCode = 400; res.setHeader("content-type", "text/html; charset=utf-8"); - res.end(config.failedHtml("state mismatch")); + res.end(config.failedHtml({ code: "state_mismatch" })); // Not this login's redirect: refuse it and keep waiting for the real one. return; } - const reason = - error ?? (isNonEmptyCode(code) ? undefined : "no code returned"); - res.statusCode = reason === undefined ? 200 : 400; + const failure: CallbackFailure | undefined = + error !== null + ? { code: "provider_error", providerError: error } + : isNonEmptyCode(code) + ? undefined + : { code: "no_code" }; + res.statusCode = failure === undefined ? 200 : 400; res.setHeader("content-type", "text/html; charset=utf-8"); - res.end(reason === undefined ? config.doneHtml : config.failedHtml(reason)); + res.end( + failure === undefined ? config.doneHtml : config.failedHtml(failure), + ); - if (reason !== undefined || !isNonEmptyCode(code)) { + if (failure !== undefined || !isNonEmptyCode(code)) { finish({ error: new OAuthCallbackError( - `Authorization failed: ${reason ?? "no code returned"}`, + failure?.code ?? "no_code", + `Authorization failed: ${failure?.providerError ?? "no code returned"}`, ), }); } else { @@ -198,11 +241,12 @@ export async function startCallbackServer( return { port: address.port, waitForCode: (signal: AbortSignal) => { - if (signal.aborted) finish({ error: new OAuthCallbackError("aborted") }); + if (signal.aborted) + finish({ error: new OAuthCallbackError("aborted", "aborted") }); else signal.addEventListener( "abort", - () => finish({ error: new OAuthCallbackError("aborted") }), + () => finish({ error: new OAuthCallbackError("aborted", "aborted") }), { once: true }, ); if (wait !== undefined) return wait; @@ -216,6 +260,18 @@ export async function startCallbackServer( }); return wait; }, - close: () => server.close(), + // Closing the listener ends the wait: a caller that closes before a code + // arrives has given up on it, and anything awaiting `waitForCode` must + // hear that rather than wait for a redirect that can no longer land. + // A no-op once a code (or a refusal) has already settled the outcome. + close: () => { + server.close(); + finish({ + error: new OAuthCallbackError( + "aborted", + "The callback server was closed before an authorization arrived.", + ), + }); + }, }; } diff --git a/src/hub/mount.ts b/src/hub/mount.ts index 1da7651..26f6bfe 100644 --- a/src/hub/mount.ts +++ b/src/hub/mount.ts @@ -7,6 +7,7 @@ import { Hono, type MiddlewareHandler } from "hono"; import { baseTokensFromResponse, buildAuthorizeUrl, + createLoginRegistry, discoverMcpLoginEntry, exchangeCode, mcpClientConfig, @@ -17,6 +18,7 @@ import { type BaseTokens, type CallbackServer, type FetchLike, + type OAuthLoginHandle, type OAuthClientConfig, } from "../index.js"; import { @@ -163,6 +165,19 @@ export function mountOAuthLogin( opts: MountOAuthLoginOpts, ): void { const { logins, callbacks } = opts.store ?? createOAuthLoginStore(); + // One live login per provider: its callback server binds the one port the + // authorization server will redirect to, so a second attempt has nowhere + // to listen. Resuming is also what keeps the authorize page already open + // in the operator's browser valid (CL-8857). + const inFlight = createLoginRegistry(); + const loginIdOf = new Map(); + // Cleanup names the login it belongs to, never just its provider. A login + // leaves the registry the moment it settles, but its credential is still + // being stored; a newer login for the same provider -- possibly another + // principal's -- can already be live by then, and must not lose its id. + const releaseLoginId = (provider: string, loginId: string): void => { + if (loginIdOf.get(provider) === loginId) loginIdOf.delete(provider); + }; const ttlMs = opts.loginTtlMs ?? DEFAULT_LOGIN_TTL_MS; if (opts.callbackUrl !== undefined) assertSecureCallbackUrl(opts.callbackUrl); @@ -293,20 +308,27 @@ export function mountOAuthLogin( const { flow } = resolved; const abort = new AbortController(); - let handle; + let started: { handle: OAuthLoginHandle; resumed: boolean }; try { - handle = await startOAuthLogin( - { profile: body.credentialName, signal: abort.signal }, - { - startCallbackServer: flow.startCallbackServer, - buildAuthorizeUrl: (pkce, state) => - buildAuthorizeUrl(flow.config, pkce, state), - exchangeCode: flow.exchange, - // Persistence runs below once the login id exists to report against. - saveProfile: () => Promise.resolve(), - // The browser opens the authorize URL; the hub never owns a display. - openInBrowser: () => undefined, - }, + started = await inFlight.startOrResume( + flow.provider, + () => + startOAuthLogin( + { profile: body.credentialName, signal: abort.signal }, + { + startCallbackServer: flow.startCallbackServer, + buildAuthorizeUrl: (pkce, state) => + buildAuthorizeUrl(flow.config, pkce, state), + exchangeCode: flow.exchange, + // Persistence runs below once the login id exists to report against. + saveProfile: () => Promise.resolve(), + // The browser opens the authorize URL; the hub never owns a display. + openInBrowser: () => undefined, + }, + ), + // A login belongs to the principal who started it: resuming is for + // them, not for whoever asks next. + { tag: `${tenantId}:${principalId}` }, ); } catch (cause) { opts.onError?.(cause, { provider: flow.provider }); @@ -315,14 +337,34 @@ export function mountOAuthLogin( 409, ); } + const handle = started.handle; + if (started.resumed) { + const loginId = loginIdOf.get(flow.provider); + // Recorded below in the same tick a login is registered, so a live + // entry always has one; saying so beats inventing a second login. + if (loginId === undefined) { + return c.json( + { error: "a login is in flight but cannot be resumed" }, + 409, + ); + } + return c.json({ loginId, authorizeUrl: handle.authorizeUrl }, 200); + } const loginId = logins.create({ tenantId, principalId, expiresAt: Date.now() + ttlMs, abort, - cancel: handle.cancel, + // This login's own handle, not whatever is live under the provider: + // the TTL sweep can reach an expired entry after a newer login has + // taken the port, and cancelling by provider would end that one. + cancel: () => { + handle.cancel(); + releaseLoginId(flow.provider, loginId); + }, }); + loginIdOf.set(flow.provider, loginId); // Detached on purpose: the redirect lands minutes after this response. void handle.completed.then( @@ -341,6 +383,7 @@ export function mountOAuthLogin( metadata: flow.metadata(staged.profile.tokens), }); logins.settle(loginId, { status: "completed", credentialId }); + releaseLoginId(flow.provider, loginId); } catch (cause) { opts.onError?.(cause, { provider: flow.provider }); logins.settle(loginId, { @@ -350,6 +393,7 @@ export function mountOAuthLogin( ? cause.message : "the tokens could not be stored", }); + releaseLoginId(flow.provider, loginId); } }, (cause: unknown) => { @@ -358,6 +402,7 @@ export function mountOAuthLogin( status: "failed", message: cause instanceof Error ? cause.message : String(cause), }); + releaseLoginId(flow.provider, loginId); }, ); diff --git a/src/hub/route-callback.ts b/src/hub/route-callback.ts index b37deee..1128dda 100644 --- a/src/hub/route-callback.ts +++ b/src/hub/route-callback.ts @@ -1,4 +1,8 @@ -import { OAuthCallbackError, type CallbackServer } from "../index.js"; +import { + OAuthCallbackError, + type CallbackFailure, + type CallbackServer, +} from "../index.js"; import { signedInHtml, signInFailedHtml } from "../provider.js"; type Outcome = { code: string } | { error: Error }; @@ -68,7 +72,7 @@ export function createRouteCallbacks(): RouteCallbacks { port: 0, waitForCode(signal) { const aborted = () => - finish({ error: new OAuthCallbackError("aborted") }); + finish({ error: new OAuthCallbackError("aborted", "aborted") }); if (signal.aborted) aborted(); else signal.addEventListener("abort", aborted, { once: true }); wait ??= new Promise((resolve, reject) => { @@ -88,24 +92,33 @@ export function createRouteCallbacks(): RouteCallbacks { const entry = state === undefined ? undefined : pending.get(state); // Not a login in flight (or a stale one): refuse without touching any. if (entry === undefined) - return { status: 400, html: signInFailedHtml("state mismatch") }; - const failed = (reason: string): CallbackResponse => { + return { + status: 400, + html: signInFailedHtml({ code: "state_mismatch" }), + }; + const failed = (failure: CallbackFailure): CallbackResponse => { entry.finish({ - error: new OAuthCallbackError(`Authorization failed: ${reason}`), + error: new OAuthCallbackError( + failure.code, + `Authorization failed: ${failure.providerError ?? failure.code}`, + ), }); - return { status: 400, html: signInFailedHtml(reason) }; + return { status: 400, html: signInFailedHtml(failure) }; }; // RFC 9207 §2.4: a client that knows the server sends `iss` must // reject a response whose `iss` is absent or names another issuer. if (entry.expectedIssuer !== undefined && iss !== entry.expectedIssuer) - return failed("issuer mismatch"); + return failed({ code: "state_mismatch" }); if (error !== undefined) return failed( errorDescription === undefined - ? error - : `${error}: ${errorDescription}`, + ? { code: "provider_error", providerError: error } + : { + code: "provider_error", + providerError: `${error}: ${errorDescription}`, + }, ); - if (code === undefined || code === "") return failed("no code returned"); + if (code === undefined || code === "") return failed({ code: "no_code" }); entry.finish({ code }); return { status: 200, html: signedInHtml }; }, diff --git a/src/index.ts b/src/index.ts index 493c395..d28d698 100644 --- a/src/index.ts +++ b/src/index.ts @@ -5,6 +5,9 @@ export { openInBrowser } from "./browser.js"; export { startCallbackServer, OAuthCallbackError, + type CallbackErrorCode, + type CallbackFailure, + type CallbackFailureCode, OAuthCallbackPortInUseError, OAuthCallbackAddressUnavailableError, type CallbackServer, @@ -41,6 +44,8 @@ export { type OAuthLoginProvider, } from "./provider.js"; +export { createLoginRegistry, type LoginRegistry } from "./login-registry.js"; + export { createTokenSession, isTokenExpired, diff --git a/src/login-registry.test.ts b/src/login-registry.test.ts new file mode 100644 index 0000000..7ac0d48 --- /dev/null +++ b/src/login-registry.test.ts @@ -0,0 +1,115 @@ +import { describe, expect, test } from "bun:test"; + +import { + createLoginRegistry, + type BaseTokens, + type OAuthLoginHandle, + type StagedOAuthProfile, +} from "./index"; + +type Settle = { + readonly handle: OAuthLoginHandle; + readonly finish: () => void; + readonly fail: (cause: Error) => void; +}; + +/** A login handle whose completion the test drives. */ +function stubLogin(authorizeUrl: string, onCancel?: () => void): Settle { + let finish!: () => void; + let fail!: (cause: Error) => void; + const completed = new Promise>((_, reject) => { + fail = reject; + finish = () => { + reject(new Error("completed")); + }; + }); + return { + handle: { + authorizeUrl, + completed, + cancel: () => onCancel?.(), + }, + finish, + fail, + }; +} + +describe("createLoginRegistry", () => { + test("a second start for the same key resumes the login in flight", async () => { + const registry = createLoginRegistry(); + const first = stubLogin("https://auth.example/authorize?state=first"); + let starts = 0; + + const begun = await registry.startOrResume("codex", () => { + starts += 1; + return Promise.resolve(first.handle); + }); + const again = await registry.startOrResume("codex", () => { + starts += 1; + return Promise.resolve( + stubLogin("https://auth.example/authorize?state=second").handle, + ); + }); + + // One attempt, so one state and one PKCE pair: the authorize page already + // open still matches what the loopback expects. + expect(starts).toBe(1); + expect(again.resumed).toBe(true); + expect(again.handle.authorizeUrl).toBe(begun.handle.authorizeUrl); + }); + + test("a login started by someone else is not resumed for another tag", async () => { + const registry = createLoginRegistry(); + let starts = 0; + const begin = (url: string) => () => { + starts += 1; + return Promise.resolve(stubLogin(url).handle); + }; + + await registry.startOrResume("codex", begin("https://auth.example/a"), { + tag: "tenant:alice", + }); + const other = await registry.startOrResume( + "codex", + begin("https://auth.example/b"), + { + tag: "tenant:bob", + }, + ); + + expect(starts).toBe(2); + expect(other.resumed).toBe(false); + }); + + test("a settled login is not resumed; the next start is a new one", async () => { + const registry = createLoginRegistry(); + const first = stubLogin("https://auth.example/a"); + + await registry.startOrResume("codex", () => Promise.resolve(first.handle)); + first.fail(new Error("authorization failed")); + // Let the registry observe the rejection before asking again. + await Promise.resolve(); + await Promise.resolve(); + + expect(registry.has("codex")).toBe(false); + const next = await registry.startOrResume("codex", () => + Promise.resolve(stubLogin("https://auth.example/b").handle), + ); + expect(next.resumed).toBe(false); + }); + + test("cancelling releases the key and cancels the login", async () => { + const registry = createLoginRegistry(); + let cancelled = 0; + const first = stubLogin("https://auth.example/a", () => { + cancelled += 1; + }); + + await registry.startOrResume("codex", () => Promise.resolve(first.handle)); + expect(registry.cancel("codex")).toBe(true); + + expect(cancelled).toBe(1); + expect(registry.has("codex")).toBe(false); + expect(registry.cancel("codex")).toBe(false); + }); +}); diff --git a/src/login-registry.ts b/src/login-registry.ts new file mode 100644 index 0000000..802c230 --- /dev/null +++ b/src/login-registry.ts @@ -0,0 +1,92 @@ +import type { OAuthLoginHandle } from "./login.js"; +import type { BaseTokens } from "./tokens.js"; + +/** + * One live login per key, so a second attempt resumes the first. + * + * A loopback login is not a fresh resource each time it is asked for: the + * authorization server only accepts one registered redirect_uri per client, + * so every attempt for a provider contends for the same fixed port. Starting + * over therefore costs more than it looks — the earlier attempt's authorize + * page is already open in the operator's browser, bound to the state and + * PKCE verifier that attempt generated, and a restart leaves that page + * pointing at a listener that either no longer exists or no longer + * recognises it. Resuming hands back the attempt already in flight, which is + * what the page in front of them belongs to. + * + * An entry lives until its login settles — completed, failed, or aborted — + * so nothing here needs sweeping; `completed` settling is the signal. + */ +export type LoginRegistry = { + /** + * The live login for `key`, or a new one from `begin`. `resumed` says + * which, so a caller can report the login it already knows about rather + * than announcing a second one. + * + * `tag` identifies who the login belongs to. An entry is resumed only for + * the same tag; a different one falls through to `begin`, which contends + * for the port and fails the way it does today. One person's authorization + * is not another's to continue. + */ + startOrResume: ( + key: string, + begin: () => Promise>, + opts?: { readonly tag?: string }, + ) => Promise<{ + readonly handle: OAuthLoginHandle; + readonly resumed: boolean; + }>; + /** Whether a login is live under `key`. */ + has: (key: string) => boolean; + /** Cancel the live login under `key`; false when there is none. */ + cancel: (key: string) => boolean; +}; + +export function createLoginRegistry< + TTokens extends BaseTokens = BaseTokens, +>(): LoginRegistry { + type Entry = { + readonly handle: OAuthLoginHandle; + readonly tag: string | undefined; + }; + const live = new Map(); + + const drop = (key: string, entry: Entry): void => { + if (live.get(key) === entry) live.delete(key); + }; + + return { + async startOrResume(key, begin, opts) { + const tag = opts?.tag; + const existing = live.get(key); + if (existing !== undefined && existing.tag === tag) { + return { handle: existing.handle, resumed: true }; + } + + const handle = await begin(); + const entry: Entry = { handle, tag }; + live.set(key, entry); + // Observing `completed` here is also what keeps a rejection from + // landing unhandled when the caller only ever reads `authorizeUrl`. + void handle.completed.then( + () => { + drop(key, entry); + }, + () => { + drop(key, entry); + }, + ); + return { handle, resumed: false }; + }, + has(key) { + return live.has(key); + }, + cancel(key) { + const entry = live.get(key); + if (entry === undefined) return false; + entry.handle.cancel(); + drop(key, entry); + return true; + }, + }; +} diff --git a/src/provider.ts b/src/provider.ts index 7dea9be..00c9360 100644 --- a/src/provider.ts +++ b/src/provider.ts @@ -1,4 +1,7 @@ -import { startCallbackServer } from "./callback-server.js"; +import { + startCallbackServer, + type CallbackFailure, +} from "./callback-server.js"; import { buildAuthorizeUrl, type OAuthClientConfig } from "./client.js"; import { startOAuthLogin, type OAuthLoginDeps } from "./login.js"; import type { AuthProfile, BaseTokens } from "./tokens.js"; @@ -60,9 +63,23 @@ export function callbackTargetFor(config: OAuthClientConfig): { export const signedInHtml = "Signed in

Signed in. You can close this tab."; -// `reason` echoes the redirect's `error` parameter, which anyone can set. -export const signInFailedHtml = (reason: string): string => - `Sign-in failed

Sign-in failed: ${reason.replace(/[<&]/g, "")}`; +/** + * A refused redirect is reported as a code, not prose: what to tell the + * person differs by product and brand; what the redirect was does not. The + * provider's own `error` parameter is echoed only as escaped text. + */ +export const signInFailedHtml = (failure: CallbackFailure): string => { + const detail = + failure.providerError === undefined + ? "" + : ` (${failure.providerError.replace(/[<&]/g, "")})`; + const text = { + state_mismatch: "this page is no longer the sign-in being waited for", + provider_error: `the provider refused the authorization${detail}`, + no_code: "the provider sent no authorization back", + }[failure.code]; + return `Sign-in failed

Sign-in failed: ${text}.`; +}; export type LoginWithProviderOptions = { save: OAuthLoginDeps["saveProfile"];