From 9c9195af6c762b1b0cd994bdc5a6520d3316c2a3 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 22 Sep 2026 13:24:02 -0700 Subject: [PATCH 1/4] fix(login): resume the login in flight instead of restarting it (CL-8857) Every attempt for a provider contends for the one registered redirect_uri, and the authorize page already open in the browser is bound to the state and PKCE verifier of the attempt that opened it. Starting over therefore either fails to bind the port or leaves that page redirecting into a listener that no longer recognises its state, refused as a mismatch. createLoginRegistry holds one live login per key and hands the same handle back to a second start; an entry is released when the login settles. The hub mount uses it, so a colliding start answers 200 with the pending login's id rather than 409. A login is resumed only for the tag that started it. A refused redirect is also now reported as a code -- state_mismatch, provider_error, no_code -- rather than prose, so each product maps it onto its own copy. --- README.md | 33 +++++++++++ src/callback-server.test.ts | 5 +- src/callback-server.ts | 61 +++++++++++++++---- src/hub/mount.ts | 62 ++++++++++++++----- src/index.ts | 5 ++ src/login-registry.test.ts | 115 ++++++++++++++++++++++++++++++++++++ src/login-registry.ts | 92 +++++++++++++++++++++++++++++ 7 files changed, 347 insertions(+), 26 deletions(-) create mode 100644 src/login-registry.test.ts create mode 100644 src/login-registry.ts 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..4de7e61 100644 --- a/src/callback-server.test.ts +++ b/src/callback-server.test.ts @@ -4,6 +4,8 @@ import { OAuthCallbackAddressUnavailableError, OAuthCallbackPortInUseError, startCallbackServer, + type CallbackFailure, + type CallbackServer, } from "./index.js"; const config = (port: number, host?: string) => ({ @@ -11,7 +13,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", () => { diff --git a/src/callback-server.ts b/src/callback-server.ts index 70db2f2..61ad4ee 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; } } @@ -156,21 +187,28 @@ export async function startCallbackServer( 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 +236,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; diff --git a/src/hub/mount.ts b/src/hub/mount.ts index 1da7651..805b4df 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, @@ -163,6 +164,12 @@ 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(); const ttlMs = opts.loginTtlMs ?? DEFAULT_LOGIN_TTL_MS; if (opts.callbackUrl !== undefined) assertSecureCallbackUrl(opts.callbackUrl); @@ -293,20 +300,27 @@ export function mountOAuthLogin( const { flow } = resolved; const abort = new AbortController(); - let handle; + let started; 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 +329,31 @@ 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, + cancel: () => { + inFlight.cancel(flow.provider); + loginIdOf.delete(flow.provider); + }, }); + loginIdOf.set(flow.provider, loginId); // Detached on purpose: the redirect lands minutes after this response. void handle.completed.then( @@ -341,6 +372,7 @@ export function mountOAuthLogin( metadata: flow.metadata(staged.profile.tokens), }); logins.settle(loginId, { status: "completed", credentialId }); + loginIdOf.delete(flow.provider); } catch (cause) { opts.onError?.(cause, { provider: flow.provider }); logins.settle(loginId, { @@ -350,6 +382,7 @@ export function mountOAuthLogin( ? cause.message : "the tokens could not be stored", }); + loginIdOf.delete(flow.provider); } }, (cause: unknown) => { @@ -358,6 +391,7 @@ export function mountOAuthLogin( status: "failed", message: cause instanceof Error ? cause.message : String(cause), }); + loginIdOf.delete(flow.provider); }, ); diff --git a/src/index.ts b/src/index.ts index 493c395..ac6380d 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"; + 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..9f4cd18 --- /dev/null +++ b/src/login-registry.ts @@ -0,0 +1,92 @@ +import type { OAuthLoginHandle } from "./login"; +import type { BaseTokens } from "./tokens"; + +/** + * 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; + }, + }; +} From d8f4c5be0bbc4cfd8e309e9b970abd26dd020d2f Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 22 Sep 2026 13:26:35 -0700 Subject: [PATCH 2/4] fix(callback): end the wait when the callback server is closed close() stopped the listener but left waitForCode pending, so a caller that gave up waited on a redirect that could no longer be received -- and anything tracking the login, such as the new registry, never learned it had ended and would resume a dead attempt. CL-8857 --- src/callback-server.test.ts | 10 ++++++++++ src/callback-server.ts | 14 +++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/callback-server.test.ts b/src/callback-server.test.ts index 4de7e61..ca4cc83 100644 --- a/src/callback-server.test.ts +++ b/src/callback-server.test.ts @@ -114,4 +114,14 @@ describe("Callback server startCallbackServer — state validation", () => { 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 61ad4ee..4152c6c 100644 --- a/src/callback-server.ts +++ b/src/callback-server.ts @@ -255,6 +255,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.", + ), + }); + }, }; } From cfbb540703c941f628a7b41a22925851cb014084 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 22 Sep 2026 14:36:04 -0700 Subject: [PATCH 3/4] fix(callback): refuse a mismatched redirect without ending the login The loopback listener is on a fixed port anything on the machine can reach. Ending the wait on a mismatched state let any stray or crafted request -- a stale tab, a page firing a link at localhost -- cancel a legitimate sign-in. The redirect is still refused with 400 and its code never trusted; the login keeps waiting for its own. CL-8857 --- src/callback-server.test.ts | 20 ++++++++++++-------- src/callback-server.ts | 5 +++++ src/hub/route-callback.ts | 33 +++++++++++++++++++++++---------- src/provider.ts | 25 +++++++++++++++++++++---- 4 files changed, 61 insertions(+), 22 deletions(-) diff --git a/src/callback-server.test.ts b/src/callback-server.test.ts index ca4cc83..6c0d720 100644 --- a/src/callback-server.test.ts +++ b/src/callback-server.test.ts @@ -5,7 +5,6 @@ import { OAuthCallbackPortInUseError, startCallbackServer, type CallbackFailure, - type CallbackServer, } from "./index.js"; const config = (port: number, host?: string) => ({ @@ -101,19 +100,24 @@ 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); diff --git a/src/callback-server.ts b/src/callback-server.ts index 4152c6c..30e82f2 100644 --- a/src/callback-server.ts +++ b/src/callback-server.ts @@ -184,6 +184,11 @@ 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"); 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/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"]; From 16e1d79e9944bdcaff793687ab451c03e4f11747 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 22 Sep 2026 14:36:04 -0700 Subject: [PATCH 4/4] fix(hub): clean up a login by its own identity, not its provider A login leaves the registry as soon as it settles, while its credential is still being stored. A newer login for the same provider, possibly another principal's, can be live by then: the TTL sweep reaching the older entry cancelled the newer one, and its completion deleted the newer login's id, so a retry could no longer be resumed. Cancel now targets the login's own handle, and the id is released only if it is still that login's. CL-8857 --- src/hub/mount.ts | 23 +++++++++++++++++------ src/index.ts | 2 +- src/login-registry.ts | 4 ++-- 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/src/hub/mount.ts b/src/hub/mount.ts index 805b4df..26f6bfe 100644 --- a/src/hub/mount.ts +++ b/src/hub/mount.ts @@ -18,6 +18,7 @@ import { type BaseTokens, type CallbackServer, type FetchLike, + type OAuthLoginHandle, type OAuthClientConfig, } from "../index.js"; import { @@ -170,6 +171,13 @@ export function mountOAuthLogin( // 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); @@ -300,7 +308,7 @@ export function mountOAuthLogin( const { flow } = resolved; const abort = new AbortController(); - let started; + let started: { handle: OAuthLoginHandle; resumed: boolean }; try { started = await inFlight.startOrResume( flow.provider, @@ -348,9 +356,12 @@ export function mountOAuthLogin( principalId, expiresAt: Date.now() + ttlMs, abort, + // 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: () => { - inFlight.cancel(flow.provider); - loginIdOf.delete(flow.provider); + handle.cancel(); + releaseLoginId(flow.provider, loginId); }, }); loginIdOf.set(flow.provider, loginId); @@ -372,7 +383,7 @@ export function mountOAuthLogin( metadata: flow.metadata(staged.profile.tokens), }); logins.settle(loginId, { status: "completed", credentialId }); - loginIdOf.delete(flow.provider); + releaseLoginId(flow.provider, loginId); } catch (cause) { opts.onError?.(cause, { provider: flow.provider }); logins.settle(loginId, { @@ -382,7 +393,7 @@ export function mountOAuthLogin( ? cause.message : "the tokens could not be stored", }); - loginIdOf.delete(flow.provider); + releaseLoginId(flow.provider, loginId); } }, (cause: unknown) => { @@ -391,7 +402,7 @@ export function mountOAuthLogin( status: "failed", message: cause instanceof Error ? cause.message : String(cause), }); - loginIdOf.delete(flow.provider); + releaseLoginId(flow.provider, loginId); }, ); diff --git a/src/index.ts b/src/index.ts index ac6380d..d28d698 100644 --- a/src/index.ts +++ b/src/index.ts @@ -44,7 +44,7 @@ export { type OAuthLoginProvider, } from "./provider.js"; -export { createLoginRegistry, type LoginRegistry } from "./login-registry"; +export { createLoginRegistry, type LoginRegistry } from "./login-registry.js"; export { createTokenSession, diff --git a/src/login-registry.ts b/src/login-registry.ts index 9f4cd18..802c230 100644 --- a/src/login-registry.ts +++ b/src/login-registry.ts @@ -1,5 +1,5 @@ -import type { OAuthLoginHandle } from "./login"; -import type { BaseTokens } from "./tokens"; +import type { OAuthLoginHandle } from "./login.js"; +import type { BaseTokens } from "./tokens.js"; /** * One live login per key, so a second attempt resumes the first.