fix(login): resume the login in flight instead of restarting it (CL-8857)

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

src/callback-server.test.ts

@@ -4,14 +4,16 @@ import {
   OAuthCallbackAddressUnavailableError,
   OAuthCallbackPortInUseError,
   startCallbackServer,
+  type CallbackFailure,
 } from "./index.js";
 
 const config = (port: number, host?: string) => ({
   port,
   ...(host === undefined ? {} : { host }),
   path: "/callback",
   doneHtml: "<html>done</html>",
-  failedHtml: (reason: string) => `<html>failed: ${reason}</html>`,
+  failedHtml: (failure: CallbackFailure) =>
+    `<html>failed: ${failure.code}${failure.providerError === undefined ? "" : `:${failure.providerError}`}</html>`,
 });
 
 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/);
+  });
 });

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.",
+        ),
+      });
+    },
   };
 }

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<string, string>();
+  // 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<BaseTokens>; 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);
       },
     );