History

Reserve the OAuth sign-in window on the click, not after discovery (#1703)

The transparent DCR/CIMD connects and OAuth reconnect opened the popup only after their setup round trips answered. window.open needs transient user activation, which expires a few seconds after the click, so a slow API meant the browser refused the window and the connect ended silently. Claim the window on the click and navigate it when the authorization URL arrives; close it on the paths that end without signing in, and on cancel and unmount. Report a refused window instead of swallowing it, and render the sign-in error above the footer where the automatic flows can show it.

Rhys Sullivan committed 2 months ago

Commit 2bdbedf1 parent 9a38ab46 files · +593 −51Browse files

Unified · Split

6 files
  1. .changeset/oauth-popup-reserve-on-click.md +9 −0
  2. e2e/selfhost/mcp-oauth-slow-connect.test.ts +132 −0
  3. packages/react/src/components/accounts-section.tsx +8 −0
  4. packages/react/src/components/add-account-modal.test.ts +275 −4
  5. packages/react/src/components/add-account-modal.tsx +79 −27
  6. packages/react/src/plugins/oauth-sign-in.tsx +90 −20
.changeset/oauth-popup-reserve-on-click.md+9−0
@@ -0,0 +1,9 @@
1+---
2+"@executor-js/react": patch
3+---
4+
5+**A slow OAuth discovery no longer kills the connect with no popup and no error**
6+
7+The transparent connect flows opened the sign-in window only after their setup round trips had answered: DCR after probe and dynamic registration, CIMD after minting the client, reconnect after starting the session. `window.open` needs transient user activation, which browsers expire a few seconds after the click, so once the API was slow enough the browser refused the window and the connect ended with nothing on screen but the button returning to "Connect". Every MCP integration takes that path.
8+
9+The window is now claimed on the click itself and navigated when the authorization URL arrives, however long that takes, and it is closed again on the paths that end without signing in (failed probe, no registration endpoint, rejected registration, failed client mint) as well as on cancel and unmount. A window the browser does refuse is now reported instead of swallowed: the flows stop before their round trips, and the sign-in error renders above the dialog footer, where the automatic flows can actually show it, rather than inside a method tab panel they never render.
e2e/selfhost/mcp-oauth-slow-connect.test.ts+132−0
@@ -0,0 +1,132 @@
1+// Selfhost browser coverage for the transparent DCR connect when OAuth
2+// discovery is slow, the shape behind the report: "When I hit connect, it loads
3+// for a second, but then theres no user feedback and nothing happens."
4+//
5+// One click runs probe -> register -> start. The first two are network round
6+// trips, and `window.open` needs transient user activation, which a real
7+// browser expires a few seconds after the click. The shipped code opened the
8+// sign-in window AFTER both round trips, so once the API was slow the browser
9+// refused it and the connect died silently. The window is now claimed on the
10+// click and navigated later, so discovery latency no longer decides whether the
11+// user can connect.
12+//
13+// What this scenario guards: the whole slow path still works end to end, with
14+// the reservation threaded from the click through registration to a window that
15+// really lands on the discovered authorization server.
16+//
17+// What it CANNOT guard, and why: Playwright drives Chromium in automation mode,
18+// which never enforces the activation rule for `window.open` (verified headed
19+// and headless, and with `--disable-popup-blocking` removed via
20+// ignoreDefaultArgs). So a browser here opens the window no matter how stale
21+// the click is, and this scenario passes against the pre-fix ordering too. The
22+// ordering itself is pinned by unit tests over `runDcrConnect` in
23+// packages/react/src/components/add-account-modal.test.ts, which do fail when
24+// the reservation moves back after the round trips. See LEARNINGS.md.
25+import { randomBytes } from "node:crypto";
26+
27+import { expect } from "@effect/vitest";
28+import { Effect } from "effect";
29+import { composePluginApi } from "@executor-js/api/server";
30+import { deriveMcpNamespace } from "@executor-js/plugin-mcp";
31+import { mcpHttpPlugin } from "@executor-js/plugin-mcp/api";
32+import { makeGreetingMcpServer, serveMcpServerWithOAuth } from "@executor-js/plugin-mcp/testing";
33+import { IntegrationSlug } from "@executor-js/sdk/shared";
34+import { OAuthTestServer } from "@executor-js/sdk/testing";
35+
36+import { scenario } from "../src/scenario";
37+import { Api, Browser, Target } from "../src/services";
38+
39+const api = composePluginApi([mcpHttpPlugin()] as const);
40+
41+// Comfortably past Chromium's ~5s transient user activation once both land on
42+// the same click, and well under the step timeouts below.
43+const STALL_MS = 3_500;
44+
45+const isDiscoveryCall = (url: string): boolean =>
46+ url.includes("/api/oauth/probe") || url.includes("/api/oauth/clients/register-dynamic");
47+
48+scenario(
49+ "MCP OAuth · a slow discovery round trip still opens the sign-in window",
50+ { timeout: 240_000 },
51+ Effect.scoped(
52+ Effect.gen(function* () {
53+ const target = yield* Target;
54+ const browser = yield* Browser;
55+ const { client: makeApiClient } = yield* Api;
56+ const oauth = yield* OAuthTestServer;
57+ const server = yield* serveMcpServerWithOAuth(
58+ () => makeGreetingMcpServer({ name: "slow-connect-mcp" }),
59+ { path: "/mcp" },
60+ );
61+ const identity = yield* target.newIdentity();
62+ const client = yield* makeApiClient(api, identity);
63+ const displayName = `Slow MCP ${randomBytes(3).toString("hex")}`;
64+ const slug = IntegrationSlug.make(deriveMcpNamespace({ name: displayName }));
65+
66+ yield* Effect.gen(function* () {
67+ yield* browser.session(identity, async ({ page, step }) => {
68+ await step("Add an OAuth-protected MCP integration", async () => {
69+ const addUrl = new URL("/integrations/add/mcp", target.baseUrl);
70+ addUrl.searchParams.set("url", server.endpoint);
71+ await page.goto(addUrl.toString(), { waitUntil: "networkidle" });
72+ await page.getByText("How does this server authenticate?").waitFor({ timeout: 30_000 });
73+ await page.getByPlaceholder("e.g. Linear").fill(displayName);
74+ await page.getByRole("button", { name: "Add integration" }).click();
75+ await page.waitForURL(/\/integrations\/(?!add\b)[^/?]+$/, { timeout: 30_000 });
76+ await page.getByText("Connections").first().waitFor();
77+ });
78+
79+ await step("Make OAuth discovery slow, the way a degraded API is", async () => {
80+ // Delay the responses rather than the requests, so the app sees a
81+ // genuinely slow API and not a stalled network stack.
82+ await page.route(
83+ (url) => isDiscoveryCall(url.href),
84+ async (route) => {
85+ await new Promise((resolve) => setTimeout(resolve, STALL_MS));
86+ await route.continue();
87+ },
88+ );
89+ });
90+
91+ await step("Connect, and wait out the slow discovery", async () => {
92+ await page.getByRole("button", { name: "Add connection" }).first().click();
93+ await page.getByRole("heading", { name: /Add connection/ }).waitFor();
94+ await page.getByRole("tab", { name: "OAuth" }).waitFor();
95+
96+ const popupPromise = page.waitForEvent("popup", { timeout: 60_000 });
97+ await page.getByRole("button", { name: "Connect", exact: true }).click();
98+
99+ // In a real browser the two stalls outlast the click's user
100+ // activation, so this popup only exists because it was reserved on
101+ // the click. Automation-mode Chromium would open it either way;
102+ // the ordering is pinned by the unit tests named above.
103+ const popup = await popupPromise;
104+ await popup.waitForURL((url) => url.origin === new URL(oauth.issuerUrl).origin, {
105+ timeout: 60_000,
106+ });
107+ await popup.waitForLoadState("domcontentloaded", { timeout: 30_000 });
108+ expect(
109+ new URL(popup.url()).origin,
110+ "the reserved window reached the discovered authorization host",
111+ ).toBe(new URL(oauth.authorizationEndpoint).origin);
112+ await popup.close();
113+ });
114+ });
115+
116+ const oauthRequests = yield* oauth.requests;
117+ expect(
118+ oauthRequests.some(
119+ (request) => request.method === "POST" && request.path === "/register",
120+ ),
121+ "the slow connect still dynamically registered its OAuth client",
122+ ).toBe(true);
123+ expect(
124+ oauthRequests.some(
125+ (request) => request.method === "GET" && request.path === "/authorize",
126+ ),
127+ "the slow connect still reached the authorize endpoint",
128+ ).toBe(true);
129+ }).pipe(Effect.ensuring(client.mcp.removeServer({ params: { slug } }).pipe(Effect.ignore)));
130+ }),
131+ ).pipe(Effect.provide(OAuthTestServer.layer())),
132+);
packages/react/src/components/accounts-section.tsx+8−0
@@ -279,6 +279,11 @@ function OwnerAccounts(props: { · 278 unmodified lines
279279 }
280280 const payload = oauthReconnectPayload(connection);
281281 if (payload === null) return;
282+ // Claim the sign-in window on the click: `oauth.start` below is a network
283+ // round trip, and the browser's user activation can expire before it
284+ // answers, which would leave Reconnect silently doing nothing.
285+ const reservation = oauthPopup.reserve();
286+ if (reservation.kind === "blocked") return;
282287 // `oauth.start` discriminates the grant: client_credentials mints inline
283288 // (`status: "connected"`, no authorization URL) while authorization_code
284289 // returns a redirect the popup must complete. The popup hook only handles
@@ -291,6 +296,7 @@ function OwnerAccounts(props: { · 6 unmodified lines
291296 reactivityKeys: connectionWriteKeys,
292297 });
293298 if (Exit.isFailure(startExit)) {
299+ oauthPopup.releaseReservation();
294300 toast.error(messageFromExit(startExit, "Failed to reconnect"));
295301 trackEvent("connection_reconnected", {
296302 integration_slug: String(connection.integration),
@@ -301,6 +307,7 @@ function OwnerAccounts(props: { · 4 unmodified lines
301307 }
302308 const started = startExit.value;
303309 if (started.status === "connected") {
310+ oauthPopup.releaseReservation();
304311 toast.success("Reconnected");
305312 trackEvent("connection_reconnected", {
306313 integration_slug: String(connection.integration),
@@ -311,6 +318,7 @@ function OwnerAccounts(props: { · 4 unmodified lines
311318 }
312319 void oauthPopup.openAuthorization({
313320 owner: payload.owner,
321+ reservation,
314322 run: () =>
315323 Promise.resolve({
316324 state: started.state,
packages/react/src/components/add-account-modal.test.ts+275−4
@@ -9,6 +9,7 @@ import { · 8 unmodified lines
99 } from "@executor-js/sdk/shared";
1010
1111 import type { AuthMethod } from "../lib/auth-placements";
12+import type { OAuthPopupReservation } from "../plugins/oauth-sign-in";
1213 import {
1314 connectionNameFrom,
1415 connectionLabel,
@@ -57,7 +58,35 @@ type RegisterArgs = { · 42 unmodified lines
5758 readonly originIntegration?: IntegrationSlug;
5859 };
5960
60−type StartArgs = { readonly client: OAuthClientSlug; readonly owner: Owner };
61+type StartArgs = {
62+ readonly client: OAuthClientSlug;
63+ readonly owner: Owner;
64+ readonly reservation: OAuthPopupReservation;
65+};
66+
67+// A reservation the browser honoured. The desktop shape is the one that carries
68+// no `Window`, so the orchestrators' ordering is testable without a DOM.
69+const RESERVED: OAuthPopupReservation = {
70+ kind: "desktop",
71+ bridge: { openExternal: (): Promise<void> => Promise.resolve() },
72+};
73+
74+/** Records the reserve/release calls alongside the network steps, so a test can
75+ * assert the window is claimed BEFORE the first round trip and closed again
76+ * whenever the flow ends without signing in. */
77+const popupSpy = (reservation: OAuthPopupReservation = RESERVED) => {
78+ const calls: string[] = [];
79+ return {
80+ calls,
81+ reserve: (): OAuthPopupReservation => {
82+ calls.push("reserve");
83+ return reservation;
84+ },
85+ release: (): void => {
86+ calls.push("release");
87+ },
88+ };
89+};
6190
6291 type CimdCreateArgs = {
6392 readonly owner: Owner;
@@ -70,7 +99,11 @@ type CimdCreateArgs = { · 6 unmodified lines
7099 readonly clientSecret: "";
71100 };
72101
73−type CimdStartArgs = { readonly client: OAuthClientSlug; readonly owner: Owner };
102+type CimdStartArgs = {
103+ readonly client: OAuthClientSlug;
104+ readonly owner: Owner;
105+ readonly reservation: OAuthPopupReservation;
106+};
74107
75108 const TEST_INTEGRATION = IntegrationSlug.make("linear_mcp");
76109
@@ -310,6 +343,8 @@ describe("runCimdConnect", () => { · 233 unmodified lines
310343
311344 const outcome = await runCimdConnect(
312345 {
346+ reserve: (): OAuthPopupReservation => RESERVED,
347+ release: (): void => {},
313348 createClient: (args: CimdCreateArgs): Promise<OAuthClientSlug | null> => {
314349 createArgs = args;
315350 return Promise.resolve(args.slug);
@@ -352,6 +387,8 @@ describe("runCimdConnect", () => { · 36 unmodified lines
352387
353388 const outcome = await runCimdConnect(
354389 {
390+ reserve: (): OAuthPopupReservation => RESERVED,
391+ release: (): void => {},
355392 createClient: (): Promise<OAuthClientSlug | null> => {
356393 created = true;
357394 return Promise.resolve(OAuthClientSlug.make("new-client"));
@@ -383,7 +420,221 @@ describe("runCimdConnect", () => { · 25 unmodified lines
383420
384421 expect(outcome).toEqual({ kind: "started", client: existingSlug, reused: true });
385422 expect(created).toBe(false);
386− expect(startArgs).toEqual({ client: existingSlug, owner: "user" });
423+ expect(startArgs).toEqual({ client: existingSlug, owner: "user", reservation: RESERVED });
424+ });
425+});
426+
427+describe("runDcrConnect popup reservation", () => {
428+ const probeOk = (): Promise<ProbeResult> =>
429+ Promise.resolve({
430+ authorizationUrl: "https://auth.example.com/authorize",
431+ tokenUrl: "https://auth.example.com/token",
432+ registrationEndpoint: "https://auth.example.com/register",
433+ });
434+ const dcrInput = {
435+ discoveryUrl: "https://mcp.example.com/mcp",
436+ owner: "user" as Owner,
437+ integration: TEST_INTEGRATION,
438+ };
439+
440+ // The regression: probe and register are two round trips, and a browser stops
441+ // honouring `window.open` about five seconds after the click. Opening the
442+ // window after them is refused, so the connect ends with no popup, no error,
443+ // and the button back on "Connect". The window has to be claimed up front.
444+ it("claims the sign-in window BEFORE the first network call", async () => {
445+ const popup = popupSpy();
446+ const outcome = await runDcrConnect(
447+ {
448+ ...popup,
449+ probe: (): Promise<ProbeResult> => {
450+ popup.calls.push("probe");
451+ return probeOk();
452+ },
453+ register: (): Promise<OAuthClientSlug> => {
454+ popup.calls.push("register");
455+ return Promise.resolve(OAuthClientSlug.make("mcp-app"));
456+ },
457+ start: (): void => {
458+ popup.calls.push("start");
459+ },
460+ },
461+ dcrInput,
462+ );
463+
464+ expect(outcome).toEqual({ kind: "started" });
465+ expect(popup.calls).toEqual(["reserve", "probe", "register", "start"]);
466+ });
467+
468+ it("hands the reserved window to start, so the flow navigates it rather than opening a new one", async () => {
469+ let startArgs: StartArgs | null = null;
470+ const outcome = await runDcrConnect(
471+ {
472+ ...popupSpy(),
473+ probe: probeOk,
474+ register: (): Promise<OAuthClientSlug> => Promise.resolve(OAuthClientSlug.make("mcp-app")),
475+ start: (args: StartArgs): void => {
476+ startArgs = args;
477+ },
478+ },
479+ dcrInput,
480+ );
481+
482+ expect(outcome).toEqual({ kind: "started" });
483+ expect(startArgs!.reservation).toBe(RESERVED);
484+ });
485+
486+ it("gives up before any network call when the browser refuses the window", async () => {
487+ const popup = popupSpy({ kind: "blocked" });
488+ let probed = false;
489+ const outcome = await runDcrConnect(
490+ {
491+ ...popup,
492+ probe: (): Promise<ProbeResult> => {
493+ probed = true;
494+ return probeOk();
495+ },
496+ register: (): Promise<OAuthClientSlug> => Promise.resolve(OAuthClientSlug.make("mcp-app")),
497+ start: (): void => {},
498+ },
499+ dcrInput,
500+ );
501+
502+ // No BYO fallback: registering an app by hand cannot open a window either.
503+ expect(outcome).toEqual({ kind: "popup-blocked" });
504+ expect(probed).toBe(false);
505+ expect(popup.calls).toEqual(["reserve"]);
506+ });
507+
508+ // Reserving early means a flow that ends without signing in is holding an
509+ // about:blank window the user never asked for.
510+ it("closes the reserved window when the probe fails", async () => {
511+ const popup = popupSpy();
512+ const outcome = await runDcrConnect(
513+ {
514+ ...popup,
515+ probe: (): Promise<ProbeResult | null> => Promise.resolve(null),
516+ register: (): Promise<OAuthClientSlug> => Promise.resolve(OAuthClientSlug.make("mcp-app")),
517+ start: (): void => {},
518+ },
519+ dcrInput,
520+ );
521+
522+ expect(outcome).toEqual({ kind: "fallback", reason: "probe-failed" });
523+ expect(popup.calls).toEqual(["reserve", "release"]);
524+ });
525+
526+ it("closes the reserved window when the server advertises no registration endpoint", async () => {
527+ const popup = popupSpy();
528+ const outcome = await runDcrConnect(
529+ {
530+ ...popup,
531+ probe: (): Promise<ProbeResult> =>
532+ Promise.resolve({
533+ authorizationUrl: "https://auth.example.com/authorize",
534+ tokenUrl: "https://auth.example.com/token",
535+ }),
536+ register: (): Promise<OAuthClientSlug> => Promise.resolve(OAuthClientSlug.make("mcp-app")),
537+ start: (): void => {},
538+ },
539+ dcrInput,
540+ );
541+
542+ expect(outcome.kind).toBe("fallback");
543+ expect(popup.calls).toEqual(["reserve", "release"]);
544+ });
545+
546+ it("closes the reserved window when registration is rejected", async () => {
547+ const popup = popupSpy();
548+ const outcome = await runDcrConnect(
549+ {
550+ ...popup,
551+ probe: probeOk,
552+ register: (): Promise<{ readonly error: string }> =>
553+ Promise.resolve({ error: "redirect_uri not allowed" }),
554+ start: (): void => {},
555+ },
556+ dcrInput,
557+ );
558+
559+ expect(outcome).toMatchObject({ reason: "registration-failed" });
560+ expect(popup.calls).toEqual(["reserve", "release"]);
561+ });
562+
563+ it("keeps the reserved window open on the path that signs in", async () => {
564+ const popup = popupSpy();
565+ await runDcrConnect(
566+ {
567+ ...popup,
568+ probe: probeOk,
569+ register: (): Promise<OAuthClientSlug> => Promise.resolve(OAuthClientSlug.make("mcp-app")),
570+ start: (): void => {},
571+ },
572+ dcrInput,
573+ );
574+
575+ expect(popup.calls).not.toContain("release");
576+ });
577+});
578+
579+describe("runCimdConnect popup reservation", () => {
580+ const cimdInput = {
581+ owner: "user" as Owner,
582+ integrationName: "PostHog API",
583+ authorizationUrl: "https://us.posthog.com/oauth/authorize/",
584+ tokenUrl: "https://us.posthog.com/oauth/token/",
585+ resource: null,
586+ clientIdMetadataDocumentUrl: "https://executor.example/api/oauth/client-id-metadata.json",
587+ existingClients: [],
588+ };
589+
590+ it("claims the sign-in window before minting the client", async () => {
591+ const popup = popupSpy();
592+ const outcome = await runCimdConnect(
593+ {
594+ ...popup,
595+ createClient: (args: CimdCreateArgs): Promise<OAuthClientSlug> => {
596+ popup.calls.push("createClient");
597+ return Promise.resolve(args.slug);
598+ },
599+ start: (): void => {
600+ popup.calls.push("start");
601+ },
602+ },
603+ cimdInput,
604+ );
605+
606+ expect(outcome.kind).toBe("started");
607+ expect(popup.calls).toEqual(["reserve", "createClient", "start"]);
608+ });
609+
610+ it("closes the reserved window when minting the client fails", async () => {
611+ const popup = popupSpy();
612+ const outcome = await runCimdConnect(
613+ {
614+ ...popup,
615+ createClient: (): Promise<OAuthClientSlug | null> => Promise.resolve(null),
616+ start: (): void => {},
617+ },
618+ cimdInput,
619+ );
620+
621+ expect(outcome).toEqual({ kind: "failed", reason: "create-failed" });
622+ expect(popup.calls).toEqual(["reserve", "release"]);
623+ });
624+
625+ it("never claims a window when the method is missing its endpoints", async () => {
626+ const popup = popupSpy();
627+ const outcome = await runCimdConnect(
628+ {
629+ ...popup,
630+ createClient: (): Promise<OAuthClientSlug | null> => Promise.resolve(null),
631+ start: (): void => {},
632+ },
633+ { ...cimdInput, tokenUrl: " " },
634+ );
635+
636+ expect(outcome).toEqual({ kind: "failed", reason: "missing-endpoints" });
637+ expect(popup.calls).toEqual([]);
387638 });
388639 });
389640
@@ -416,7 +667,13 @@ describe("runDcrConnect", () => { · 26 unmodified lines
416667 };
417668
418669 const outcome = await runDcrConnect(
419− { probe, register, start },
670+ {
671+ reserve: (): OAuthPopupReservation => RESERVED,
672+ release: (): void => {},
673+ probe,
674+ register,
675+ start,
676+ },
420677 {
421678 discoveryUrl: "https://mcp.example.com/mcp",
422679 owner: "user",
@@ -454,6 +711,8 @@ describe("runDcrConnect", () => { · 31 unmodified lines
454711 let registerArgs: RegisterArgs | null = null;
455712 const outcome = await runDcrConnect(
456713 {
714+ reserve: (): OAuthPopupReservation => RESERVED,
715+ release: (): void => {},
457716 probe: (): Promise<ProbeResult | null> =>
458717 Promise.resolve({
459718 authorizationUrl: "https://auth.example.com/authorize",
@@ -490,6 +749,8 @@ describe("runDcrConnect", () => { · 30 unmodified lines
490749 let registerArgs: RegisterArgs | null = null;
491750 const outcome = await runDcrConnect(
492751 {
752+ reserve: (): OAuthPopupReservation => RESERVED,
753+ release: (): void => {},
493754 probe: (): Promise<ProbeResult | null> =>
494755 Promise.resolve({
495756 authorizationUrl: "https://auth.example.com/authorize",
@@ -517,6 +778,8 @@ describe("runDcrConnect", () => { · 21 unmodified lines
517778 const calls: string[] = [];
518779 const outcome = await runDcrConnect(
519780 {
781+ reserve: (): OAuthPopupReservation => RESERVED,
782+ release: (): void => {},
520783 probe: (): Promise<ProbeResult | null> => {
521784 calls.push("probe");
522785 return Promise.resolve({
@@ -554,6 +817,8 @@ describe("runDcrConnect", () => { · 31 unmodified lines
554817 const calls: string[] = [];
555818 const outcome = await runDcrConnect(
556819 {
820+ reserve: (): OAuthPopupReservation => RESERVED,
821+ release: (): void => {},
557822 probe: (): Promise<ProbeResult | null> => {
558823 calls.push("probe");
559824 return Promise.resolve(null);
@@ -580,6 +845,8 @@ describe("runDcrConnect", () => { · 20 unmodified lines
580845 const calls: string[] = [];
581846 const outcome = await runDcrConnect(
582847 {
848+ reserve: (): OAuthPopupReservation => RESERVED,
849+ release: (): void => {},
583850 probe: (): Promise<ProbeResult | null> =>
584851 Promise.resolve({
585852 authorizationUrl: "https://auth.example.com/authorize",
@@ -620,6 +887,8 @@ describe("runDcrConnect", () => { · 34 unmodified lines
620887 let started = false;
621888 const outcome = await runDcrConnect(
622889 {
890+ reserve: (): OAuthPopupReservation => RESERVED,
891+ release: (): void => {},
623892 probe: (): Promise<ProbeResult | null> =>
624893 Promise.resolve({
625894 authorizationUrl: "https://auth.example.com/authorize",
@@ -659,6 +928,8 @@ describe("runDcrConnect", () => { · 33 unmodified lines
659928 let registerArgs: RegisterArgs | null = null;
660929 const outcome = await runDcrConnect(
661930 {
931+ reserve: (): OAuthPopupReservation => RESERVED,
932+ release: (): void => {},
662933 probe: (): Promise<ProbeResult | null> =>
663934 Promise.resolve({
664935 issuer: "https://auth.example.com",
packages/react/src/components/add-account-modal.tsx+79−27
@@ -62,6 +62,7 @@ import { · 61 unmodified lines
6262 oauthClientIdMetadataDocumentUrl,
6363 useOAuthPopupFlow,
6464 type OAuthCompletionPayload,
65+ type OAuthPopupReservation,
6566 } from "../plugins/oauth-sign-in";
6667 import {
6768 clientDisplayName,
@@ -638,7 +639,17 @@ type CimdCreateClientArgs = { · 570 unmodified lines
638639
639640 type RunCimdConnectDeps = {
640641 readonly createClient: (args: CimdCreateClientArgs) => Promise<OAuthClientSlug | null>;
641− readonly start: (args: { readonly client: OAuthClientSlug; readonly owner: Owner }) => void;
642+ readonly start: (args: CimdStartArgs) => void;
643+ /** Claim the sign-in window before any await. See `useOAuthPopupFlow.reserve`. */
644+ readonly reserve: () => OAuthPopupReservation;
645+ /** Close a claimed window this flow turned out not to need. */
646+ readonly release: () => void;
647+};
648+
649+type CimdStartArgs = {
650+ readonly client: OAuthClientSlug;
651+ readonly owner: Owner;
652+ readonly reservation: OAuthPopupReservation;
642653 };
643654
644655 type RunCimdConnectInput = {
@@ -653,6 +664,7 @@ type RunCimdConnectInput = { · 8 unmodified lines
653664
654665 type CimdOutcome =
655666 | { readonly kind: "started"; readonly client: OAuthClientSlug; readonly reused: boolean }
667+ | { readonly kind: "popup-blocked" }
656668 | { readonly kind: "failed"; readonly reason: "missing-endpoints" | "create-failed" };
657669
658670 export async function runCimdConnect(
@@ -663,6 +675,12 @@ export async function runCimdConnect( · 4 unmodified lines
663675 return { kind: "failed", reason: "missing-endpoints" };
664676 }
665677
678+ // Claim the window while the click still counts. Minting the client is a
679+ // round trip, and the browser stops honouring `window.open` once the
680+ // activation from the click expires.
681+ const reservation = deps.reserve();
682+ if (reservation.kind === "blocked") return { kind: "popup-blocked" };
683+
666684 const resource = input.resource ?? null;
667685 const existing = input.existingClients.find(
668686 (client) =>
@@ -675,7 +693,7 @@ export async function runCimdConnect( · 6 unmodified lines
675693 );
676694
677695 if (existing) {
678− deps.start({ client: existing.slug, owner: input.owner });
696+ deps.start({ client: existing.slug, owner: input.owner, reservation });
679697 return { kind: "started", client: existing.slug, reused: true };
680698 }
681699
@@ -693,8 +711,11 @@ export async function runCimdConnect( · 11 unmodified lines
693711 clientId: input.clientIdMetadataDocumentUrl,
694712 clientSecret: "",
695713 });
696− if (created === null) return { kind: "failed", reason: "create-failed" };
697− deps.start({ client: created, owner: input.owner });
714+ if (created === null) {
715+ deps.release();
716+ return { kind: "failed", reason: "create-failed" };
717+ }
718+ deps.start({ client: created, owner: input.owner, reservation });
698719 return { kind: "started", client: created, reused: false };
699720 }
700721
@@ -741,16 +762,20 @@ type DcrRegisterArgs = { · 40 unmodified lines
741762 type DcrStartArgs = {
742763 readonly client: OAuthClientSlug;
743764 readonly owner: Owner;
765+ readonly reservation: OAuthPopupReservation;
744766 };
745767
746768 /** Outcome of the DCR orchestration. `"started"` means the OAuth flow handed
747− * off (the popup/inline start ran); `"fallback"` means we could not auto-set-up
748− * — a failed probe, no registration endpoint, or a failed registration — and
749− * the caller should fall back to the bring-your-own-app picker. A failed probe
769+ * off (the popup/inline start ran); `"popup-blocked"` means the browser refused
770+ * the sign-in window, which no app picker can fix, so the caller reports it
771+ * rather than falling back; `"fallback"` means we could not auto-set-up (a
772+ * failed probe, no registration endpoint, or a failed registration) and the
773+ * caller should fall back to the bring-your-own-app picker. A failed probe
750774 * carries no probe result; the other two reasons always carry the probe that
751775 * seeds the picker. */
752776 type DcrOutcome =
753777 | { readonly kind: "started" }
778+ | { readonly kind: "popup-blocked" }
754779 | { readonly kind: "fallback"; readonly reason: "probe-failed" }
755780 | {
756781 readonly kind: "fallback";
@@ -774,6 +799,10 @@ type RunDcrConnectDeps = { · 17 unmodified lines
774799 ) => Promise<OAuthClientSlug | { readonly error: string } | null>;
775800 /** Start the OAuth flow with the minted client (popup / inline). */
776801 readonly start: (args: DcrStartArgs) => void;
802+ /** Claim the sign-in window before any await. See `useOAuthPopupFlow.reserve`. */
803+ readonly reserve: () => OAuthPopupReservation;
804+ /** Close a claimed window this flow turned out not to need. */
805+ readonly release: () => void;
777806 };
778807
779808 type RunDcrConnectInput = {
@@ -804,8 +833,9 @@ type RunDcrConnectInput = { · 24 unmodified lines
804833 const DCR_CLIENT_NAME = "Executor";
805834
806835 /**
807− * Run the transparent DCR connect sequence: probe → register → start.
836+ * Run the transparent DCR connect sequence: reserve → probe → register → start.
808837 *
838+ * - Popup refused → `{ kind: "popup-blocked" }` before any network call.
809839 * - Probe failure → `{ kind: "fallback", reason: "probe-failed" }` (caller shows BYO).
810840 * - No registration endpoint → `{ kind: "fallback", reason: "no-registration-endpoint", probe }`.
811841 * - Register rejected with a message → `{ kind: "fallback", reason: "registration-failed", probe, message }`
@@ -817,10 +847,23 @@ export async function runDcrConnect( · 5 unmodified lines
817847 deps: RunDcrConnectDeps,
818848 input: RunDcrConnectInput,
819849 ): Promise<DcrOutcome> {
850+ // Claim the sign-in window FIRST, on the click that got us here. Probe and
851+ // register are two network round trips; the browser's user activation expires
852+ // well before they finish, so a window opened after them is refused and the
853+ // connect dies with nothing on screen.
854+ const reservation = deps.reserve();
855+ if (reservation.kind === "blocked") return { kind: "popup-blocked" };
856+
820857 const probe = await deps.probe(input.discoveryUrl);
821− if (probe === null) return { kind: "fallback", reason: "probe-failed" };
858+ if (probe === null) {
859+ deps.release();
860+ return { kind: "fallback", reason: "probe-failed" };
861+ }
822862 const registrationEndpoint = probe.registrationEndpoint;
823− if (!registrationEndpoint) return { kind: "fallback", reason: "no-registration-endpoint", probe };
863+ if (!registrationEndpoint) {
864+ deps.release();
865+ return { kind: "fallback", reason: "no-registration-endpoint", probe };
866+ }
824867
825868 const slug = optimisticDcrClientSlug(probe.issuer ?? registrationEndpoint);
826869 const scopes = registrationScopes(input.declaredScopes ?? [], probe.scopesSupported ?? []);
@@ -838,13 +881,17 @@ export async function runDcrConnect( · 11 unmodified lines
838881 redirectUri: input.redirectUri,
839882 originIntegration: input.integration,
840883 });
841− if (minted === null) return { kind: "fallback", reason: "registration-failed", probe };
884+ if (minted === null) {
885+ deps.release();
886+ return { kind: "fallback", reason: "registration-failed", probe };
887+ }
842888 // OAuthClientSlug is a branded string; an object return carries the failure
843889 // message (e.g. the server rejected the redirect URI) for the BYO fallback.
844890 if (typeof minted === "object") {
891+ deps.release();
845892 return { kind: "fallback", reason: "registration-failed", probe, message: minted.error };
846893 }
847− deps.start({ client: minted, owner: input.owner });
894+ deps.start({ client: minted, owner: input.owner, reservation });
848895 return { kind: "started" };
849896 }
850897
@@ -2083,6 +2130,8 @@ function AddAccountModalView(props: AddAccountModalProps) { · 1232 unmodified lines
20832130 setCimdBusy(true);
20842131 const outcome = await runCimdConnect(
20852132 {
2133+ reserve: oauthPopup.reserve,
2134+ release: oauthPopup.releaseReservation,
20862135 createClient: async (args: CimdCreateClientArgs): Promise<OAuthClientSlug | null> => {
20872136 const exit = await doCreateOAuthClient({
20882137 payload: {
@@ -2100,8 +2149,9 @@ function AddAccountModalView(props: AddAccountModalProps) { · 11 unmodified lines
21002149 if (Exit.isFailure(exit)) return null;
21012150 return exit.value.client;
21022151 },
2103− start: (args: { readonly client: OAuthClientSlug; readonly owner: Owner }): void => {
2152+ start: (args: CimdStartArgs): void => {
21042153 void oauthPopup.start({
2154+ reservation: args.reservation,
21052155 payload: {
21062156 client: args.client,
21072157 // CIMD creates the public client under the connection owner, so
@@ -2160,6 +2210,8 @@ function AddAccountModalView(props: AddAccountModalProps) { · 52 unmodified lines
21602210 setDcrBusy(true);
21612211 const outcome = await runDcrConnect(
21622212 {
2213+ reserve: oauthPopup.reserve,
2214+ release: oauthPopup.releaseReservation,
21632215 probe: async (url: string): Promise<DcrProbeResult | null> => {
21642216 const exit = await doProbe({ payload: { url }, reactivityKeys: [] });
21652217 if (Exit.isFailure(exit)) return null;
@@ -2194,6 +2246,7 @@ function AddAccountModalView(props: AddAccountModalProps) { · 28 unmodified lines
21942246 },
21952247 start: (args: DcrStartArgs): void => {
21962248 void oauthPopup.start({
2249+ reservation: args.reservation,
21972250 payload: {
21982251 client: args.client,
21992252 // DCR registers the client under the connection owner, so the app
@@ -2236,6 +2289,10 @@ function AddAccountModalView(props: AddAccountModalProps) { · 36 unmodified lines
22362289 success: outcome.kind === "started",
22372290 ...(outcome.kind === "fallback" ? { dcr_fallback: true } : {}),
22382291 });
2292+ // Deliberately absent: a "popup-blocked" branch. Registering an app by hand
2293+ // does not make the browser open a window, so dropping to the BYO picker
2294+ // would send the user down a path that cannot succeed either. `reserve`
2295+ // already put the reason in `oauthPopup.error`, which the footer renders.
22392296 if (outcome.kind === "fallback") {
22402297 setOAuthFallbackProbe("probe" in outcome ? outcome.probe : null);
22412298 setDcrFailed(true);
@@ -2497,17 +2554,6 @@ function AddAccountModalView(props: AddAccountModalProps) { · 255 unmodified lines
24972554 : `${integrationName} supports Client ID Metadata Document OAuth. We'll use this Executor host's public client metadata document and sign you in.`}
24982555 </p>
24992556 </div>
2500− ) : dcrActive ? (
2501− // Transparent DCR: no picker. We register an app for you and run
2502− // the OAuth flow with a single Connect click.
2503− <div className="space-y-2 rounded-lg border border-ring/40 bg-accent/30 px-3 py-3">
2504− <p className="text-sm font-medium">No app to choose</p>
2505− <p className="text-xs text-muted-foreground">
2506− {dcrConnecting
2507− ? `Connecting to ${integrationName}…`
2508− : `${integrationName} supports automatic setup. We register an app for you and sign you in — no client ID or app to pick.`}
2509− </p>
2510− </div>
25112557 ) : oauthLoading ? (
25122558 <p className="text-xs text-muted-foreground">Loading OAuth apps…</p>
25132559 ) : (
@@ -2739,9 +2785,6 @@ function AddAccountModalView(props: AddAccountModalProps) { · 225 unmodified lines
27392785 />
27402786 </div>
27412787 )}
2742− {isOAuth && oauthPopup.error ? (
2743− <p className="text-xs text-destructive">{oauthPopup.error}</p>
2744− ) : null}
27452788 </div>
27462789 )}
27472790 </TabsContent>
@@ -2909,6 +2952,15 @@ function AddAccountModalView(props: AddAccountModalProps) { · 161 unmodified lines
29092952 {continueError}
29102953 </p>
29112954 ) : null}
2955+ {/* Above the footer, not inside the method tab: the automatic
2956+ (CIMD/DCR) flows render no tab panel at all, and putting the
2957+ sign-in error in there left a blocked popup with nothing on
2958+ screen but the button returning to "Connect". */}
2959+ {isOAuth && oauthPopup.error ? (
2960+ <p role="alert" className="px-1 text-xs text-destructive">
2961+ {oauthPopup.error}
2962+ </p>
2963+ ) : null}
29122964 <DialogFooter>
29132965 <Button
29142966 type="button"
packages/react/src/plugins/oauth-sign-in.tsx+90−20
@@ -12,11 +12,12 @@ import { · 11 unmodified lines
1212 openOAuthSystemBrowser,
1313 reserveOAuthPopup,
1414 type OAuthPopupResult,
15+ type ReservedOAuthPopup,
1516 } from "../api/oauth-popup";
1617 import { connectionWriteKeys } from "../api/reactivity-keys";
1718 import { getActiveOrgSlug } from "../api/server-connection";
1819
19−type DesktopBridge = {
20+export type DesktopBridge = {
2021 readonly openExternal: (url: string) => Promise<void>;
2122 };
2223
@@ -80,8 +81,31 @@ export type StartOAuthPopupInput<TPayload extends OAuthCompletionPayload> = { · 57 unmodified lines
8081 readonly onSuccess: (payload: TPayload) => void | Promise<void>;
8182 readonly onError?: (error: string) => void;
8283 readonly onAuthorizationStarted?: (result: OAuthAuthorizationStartResult) => void;
84+ /** Window reserved on the click that began this flow. See `reserve`. */
85+ readonly reservation?: OAuthPopupReservation;
8386 };
8487
88+/**
89+ * A sign-in window claimed synchronously inside a click handler.
90+ *
91+ * `window.open` requires transient user activation, which Chromium expires
92+ * about five seconds after the click. Any flow that awaits the network before
93+ * it has an authorization URL (DCR probes then registers, CIMD creates a
94+ * client, reconnect starts a session) therefore cannot open the window when the
95+ * URL finally arrives: by then the activation is gone and the browser refuses
96+ * silently. Those flows reserve a blank window on the click and navigate it
97+ * later, however long the round trips take.
98+ *
99+ * Desktop hosts hand the URL to the real browser, so they reserve nothing and
100+ * carry the bridge that will open it instead.
101+ */
102+export type OAuthPopupReservation =
103+ | { readonly kind: "desktop"; readonly bridge: DesktopBridge }
104+ | { readonly kind: "window"; readonly popup: ReservedOAuthPopup }
105+ | { readonly kind: "blocked" };
106+
107+const POPUP_BLOCKED_MESSAGE = "Sign-in popup was blocked by the browser";
108+
85109 export type OAuthAuthorizationStartResult = {
86110 /** OAuth correlation token (was the v1 session id). */
87111 readonly state: string;
@@ -100,6 +124,8 @@ export type StartOAuthAuthorizationInput<TPayload extends OAuthCompletionPayload · 12 unmodified lines
100124 readonly onError?: (error: string, details?: string) => void;
101125 readonly onAuthorizationStarted?: (result: OAuthAuthorizationStartResult) => void;
102126 readonly reportMetadata?: Record<string, string | number | boolean | null | undefined>;
127+ /** Window reserved on the click that began this flow. See `reserve`. */
128+ readonly reservation?: OAuthPopupReservation;
103129 };
104130
105131 export function oauthCallbackUrl(path = "/api/oauth/callback"): string {
@@ -164,10 +190,21 @@ export function useOAuthPopupFlow< · 58 unmodified lines
164190 const doCancelOAuth = useAtomSet(cancelOAuth, { mode: "promiseExit" });
165191 const doOAuthConnectionCompleted = useAtomSet(oauthConnectionCompleted, { mode: "promiseExit" });
166192 const reportHandledError = useReportHandledError();
193+ const blockedMessage = popupBlockedMessage ?? POPUP_BLOCKED_MESSAGE;
167194 const [busy, setBusy] = useState(false);
168195 const [error, setError] = useState<string | null>(null);
169196 const cleanupRef = useRef<(() => void) | null>(null);
170197 const sessionRef = useRef<{ readonly state: string } | null>(null);
198+ // A window reserved on the click but not yet handed to a flow owns nothing
199+ // that would close it: `cleanupRef` is only set once the authorization URL
200+ // exists. Hold it here so cancel and unmount can close it, or abandoning a
201+ // connect mid-probe strands a blank popup on screen.
202+ const reservationRef = useRef<ReservedOAuthPopup | null>(null);
203+
204+ const releaseReservation = useCallback(() => {
205+ reservationRef.current?.popup.close();
206+ reservationRef.current = null;
207+ }, []);
171208
172209 const cancelSession = useCallback(
173210 (state: string) => {
@@ -181,9 +218,10 @@ export function useOAuthPopupFlow< · 7 unmodified lines
181218 cleanupRef.current?.();
182219 cleanupRef.current = null;
183220 sessionRef.current = null;
221+ releaseReservation();
184222 if (session) cancelSession(session.state);
185223 setBusy(false);
186− }, [cancelSession]);
224+ }, [cancelSession, releaseReservation]);
187225
188226 useEffect(
189227 () => () => {
@@ -191,29 +229,59 @@ export function useOAuthPopupFlow< · 1 unmodified line
191229 cleanupRef.current?.();
192230 cleanupRef.current = null;
193231 sessionRef.current = null;
232+ reservationRef.current?.popup.close();
233+ reservationRef.current = null;
194234 if (session) cancelSession(session.state);
195235 },
196236 [cancelSession],
197237 );
198238
239+ /**
240+ * Claim the sign-in window NOW, while the click still carries user
241+ * activation, and return what was claimed so the caller can abandon a flow
242+ * the browser already refused instead of running a doomed authorization.
243+ *
244+ * Callers that reach `openAuthorization` synchronously do not need this; it
245+ * reserves for itself.
246+ */
247+ const reserve = useCallback((): OAuthPopupReservation => {
248+ // Tear the previous flow down BEFORE opening: `window.open` reuses a window
249+ // already carrying `popupName`, so cancelling afterwards would close the
250+ // very window just reserved.
251+ cancel();
252+ const bridge = getDesktopBridge();
253+ if (bridge !== null) return { kind: "desktop", bridge };
254+ const popup = reserveOAuthPopup({ popupName });
255+ if (popup === null) {
256+ trackEvent("oauth_popup_blocked");
257+ setError(blockedMessage);
258+ return { kind: "blocked" };
259+ }
260+ reservationRef.current = popup;
261+ return { kind: "window", popup };
262+ }, [blockedMessage, cancel, popupName]);
263+
199264 const openAuthorization = useCallback(
200265 async (input: StartOAuthAuthorizationInput<TPayload>) => {
201− cancel();
202− setBusy(true);
203− setError(null);
204− const desktopBridge = getDesktopBridge();
205− // Desktop hosts open the auth URL in the user's real browser, so we skip
206− // the in-page popup reservation entirely and rely on the polling channel
207− // for the result.
208− const reservedPopup = desktopBridge ? null : reserveOAuthPopup({ popupName });
209− if (!desktopBridge && !reservedPopup) {
210− const message = popupBlockedMessage ?? "Sign-in popup was blocked by the browser";
211− trackEvent("oauth_popup_blocked");
266+ // A caller that reserved on its own click already ran `cancel` inside
267+ // `reserve`; cancelling again here would close the window it reserved.
268+ const reservation = input.reservation ?? reserve();
269+ if (reservation.kind === "blocked") {
212270 setBusy(false);
213− setError(message);
214− input.onError?.(message);
271+ setError(blockedMessage);
272+ input.onError?.(blockedMessage);
215273 return;
216274 }
275+ setBusy(true);
276+ setError(null);
277+ // Desktop hosts open the auth URL in the user's real browser, so they
278+ // reserve no in-page window and rely on the polling channel for the
279+ // result.
280+ const desktopBridge = reservation.kind === "desktop" ? reservation.bridge : null;
281+ const reservedPopup = reservation.kind === "window" ? reservation.popup : null;
282+ // The window's lifetime now belongs to this flow's teardown, which closes
283+ // it on every failure path below.
284+ reservationRef.current = null;
217285 const startExit = await Effect.runPromiseExit(
218286 Effect.tryPromise({
219287 try: input.run,
@@ -319,11 +387,10 @@ export function useOAuthPopupFlow< · 99 unmodified lines
319387 cleanupRef.current = null;
320388 sessionRef.current = null;
321389 cancelSession(response.state);
322− const message = popupBlockedMessage ?? "Sign-in popup was blocked by the browser";
323390 trackEvent("oauth_completed", { success: false });
324391 setBusy(false);
325− setError(message);
326− input.onError?.(message);
392+ setError(blockedMessage);
393+ input.onError?.(blockedMessage);
327394 };
328395
329396 cleanupRef.current = desktopBridge
@@ -348,15 +415,15 @@ export function useOAuthPopupFlow< · 18 unmodified lines
348415 });
349416 },
350417 [
351− cancel,
418+ blockedMessage,
352419 cancelSession,
353420 detectPopupClosed,
354421 doOAuthConnectionCompleted,
355422 noAuthorizationUrlMessage,
356− popupBlockedMessage,
357423 popupClosedMessage,
358424 popupName,
359425 reportHandledError,
426+ reserve,
360427 startErrorMessage,
361428 ],
362429 );
@@ -368,6 +435,7 @@ export function useOAuthPopupFlow< · 5 unmodified lines
368435 onSuccess: input.onSuccess,
369436 onError: input.onError,
370437 onAuthorizationStarted: input.onAuthorizationStarted,
438+ ...(input.reservation === undefined ? {} : { reservation: input.reservation }),
371439 reportMetadata: {
372440 client: String(input.payload.client),
373441 integration: String(input.payload.integration),
@@ -414,6 +482,8 @@ export function useOAuthPopupFlow< · 40 unmodified lines
414482 start,
415483 openAuthorization,
416484 cancel,
485+ reserve,
486+ releaseReservation,
417487 };
418488 }
419489