From 903126aa4e5da2a5ed263d1e13edeb0350a4fc2a Mon Sep 17 00:00:00 2001 From: Aditya kumar singh <143548997+Adityakk9031@users.noreply.github.com> Date: Mon, 10 Aug 2026 17:02:25 +0530 Subject: [PATCH 1/8] fix(oauth): route DCR reconnect through registration flow --- .../src/components/add-account-modal.tsx | 215 ++++++++++++------ 1 file changed, 144 insertions(+), 71 deletions(-) diff --git a/packages/react/src/components/add-account-modal.tsx b/packages/react/src/components/add-account-modal.tsx index a79eafe56..32bc14770 100644 --- a/packages/react/src/components/add-account-modal.tsx +++ b/packages/react/src/components/add-account-modal.tsx @@ -936,7 +936,12 @@ interface AddAccountModalProps { * which cancels a dangling server OAuth session. That is why abandoning an * OAuth popup can't wedge a later open: the stuck flow died with its instance. * The parent owns only open/route intent (deep links, the reconnect handoff). */ -export function AddAccountModal(props: AddAccountModalProps) { +export const hasDcr = (method: AuthMethod | undefined | null): boolean => { + if (!method || method.kind !== "oauth") return false; + return method.oauth?.supportsDynamicRegistration === true || method.oauth?.discoveryUrl != null; +}; + +export const AddAccountModal = (props: AddAccountModalProps) => { return props.open ? : null; } @@ -1485,10 +1490,7 @@ function AddAccountModalView(props: AddAccountModalProps) { // DCR-capable: the integration advertises dynamic registration (MCP oauth2), // OR carries a discovery URL we can probe at connect time. When DCR-capable // and not yet fallen back, we skip the app picker entirely (Option A). - const isDcr = - !cimdActive && - isOAuth && - (method?.oauth?.supportsDynamicRegistration === true || method?.oauth?.discoveryUrl != null); + const isDcr = !cimdActive && hasDcr(method); const dcrActive = isDcr && !dcrFailed; const automaticOAuthActive = cimdActive || dcrActive; @@ -1699,6 +1701,19 @@ function AddAccountModalView(props: AddAccountModalProps) { oauthReconnectOpenedKey.current = handoff.key; setMethodId(oauthMethod.id); + + if (hasDcr(oauthMethod)) { + void executeDcrConnect({ + method: oauthMethod, + connectionName: ConnectionName.make(connectionName), + identityLabel: handoff.identityLabel, + dcrOwner: connectionOwner, + isReconnect: true, + handoffKey: handoff.key, + }); + return; + } + void oauthPopup.start({ payload: { client: OAuthClientSlug.make(client), @@ -1728,6 +1743,7 @@ function AddAccountModalView(props: AddAccountModalProps) { close(); }, }); + // eslint-disable-next-line react-hooks/exhaustive-deps }, [initialState, allMethods, integration, oauthPopup, close]); const probeAndAutoNameOAuthConnection = async ( @@ -2128,19 +2144,19 @@ function AddAccountModalView(props: AddAccountModalProps) { } }; - // Transparent DCR connect: probe → register → start, no app picker. On any - // failure (probe error, no registration endpoint, or registration failure) we - // flip `dcrFailed` so the bring-your-own-app picker renders as the recovery - // path with name/owner kept. - const handleDcrConnect = async () => { - const discoveryUrl = method?.oauth?.discoveryUrl ?? method?.oauth?.tokenUrl; - if (!method || !discoveryUrl) { - setDcrFailed(true); + const executeDcrConnect = async (args: { + readonly method: AuthMethod; + readonly connectionName: string; + readonly identityLabel: string | undefined; + readonly dcrOwner: Owner; + readonly isReconnect: boolean; + readonly handoffKey?: string; + }) => { + const discoveryUrl = args.method.oauth?.discoveryUrl ?? args.method.oauth?.tokenUrl; + if (!discoveryUrl) { + if (!args.isReconnect) setDcrFailed(true); return; } - const dcrOwner = owner; - const connectionName = previewConnectionName(label, dcrOwner); - const identityLabel = typedIdentityLabel(label); setDcrBusy(true); const outcome = await runDcrConnect( { @@ -2150,22 +2166,22 @@ function AddAccountModalView(props: AddAccountModalProps) { return exit.value; }, register: async ( - args: DcrRegisterArgs, + rArgs: DcrRegisterArgs, ): Promise => { const exit = await doRegisterDynamic({ payload: { - owner: args.owner, - slug: args.slug, - issuer: args.issuer ?? null, - registrationEndpoint: args.registrationEndpoint, - authorizationUrl: args.authorizationUrl, - tokenUrl: args.tokenUrl, - resource: args.resource ?? null, - scopes: args.scopes, - tokenEndpointAuthMethodsSupported: args.tokenEndpointAuthMethodsSupported, - clientName: args.clientName, - redirectUri: args.redirectUri, - originIntegration: args.originIntegration, + owner: rArgs.owner, + slug: rArgs.slug, + issuer: rArgs.issuer ?? null, + registrationEndpoint: rArgs.registrationEndpoint, + authorizationUrl: rArgs.authorizationUrl, + tokenUrl: rArgs.tokenUrl, + resource: rArgs.resource ?? null, + scopes: rArgs.scopes, + tokenEndpointAuthMethodsSupported: rArgs.tokenEndpointAuthMethodsSupported, + clientName: rArgs.clientName, + redirectUri: rArgs.redirectUri, + originIntegration: rArgs.originIntegration, }, reactivityKeys: oauthClientWriteKeys, }); @@ -2176,60 +2192,117 @@ function AddAccountModalView(props: AddAccountModalProps) { } return exit.value.client; }, - start: (args: DcrStartArgs): void => { - void oauthPopup.start({ - payload: { - client: args.client, - // DCR registers the client under the connection owner, so the app - // and connection share one owner. - clientOwner: args.owner, - owner: args.owner, - name: connectionName, - integration, - template: method.template, - newConnection: true, - ...(identityLabel !== undefined ? { identityLabel } : {}), - }, - onSuccess: async (connection: OAuthCompletionPayload) => { - await probeAndAutoNameOAuthConnection(connection, label); - toast.success("Connection added"); - close(); - }, - }); + start: (sArgs: DcrStartArgs): void => { + if (args.isReconnect) { + void oauthPopup.start({ + payload: { + client: sArgs.client, + clientOwner: sArgs.owner, + owner: args.dcrOwner, + name: args.connectionName, + integration, + template: args.method.template, + ...(args.identityLabel !== undefined ? { identityLabel: args.identityLabel } : {}), + }, + onAuthorizationStarted: () => { + trackEvent("connection_reconnected", { + integration_slug: String(integration), + owner: args.dcrOwner, + success: true, + }); + }, + onError: () => { + trackEvent("connection_reconnected", { + integration_slug: String(integration), + owner: args.dcrOwner, + success: false, + }); + }, + onSuccess: () => { + toast.success("Reconnected"); + close(); + }, + }); + } else { + void oauthPopup.start({ + payload: { + client: sArgs.client, + clientOwner: sArgs.owner, + owner: args.dcrOwner, + name: args.connectionName, + integration, + template: args.method.template, + newConnection: true, + ...(args.identityLabel !== undefined ? { identityLabel: args.identityLabel } : {}), + }, + onSuccess: async (connection: OAuthCompletionPayload) => { + await probeAndAutoNameOAuthConnection(connection, label); + toast.success("Connection added"); + close(); + }, + }); + } }, }, { + owner: args.dcrOwner, + integrationName, + authorizationUrl: args.method.oauth?.authorizationUrl, + tokenUrl: args.method.oauth?.tokenUrl, discoveryUrl, - // Only a genuine discovery URL (MCP) seeds the RFC 8707 resource - // indicator; the token-endpoint fallback baked into `discoveryUrl` must - // not, so pass the un-collapsed method value here. - resourceFallback: method.oauth?.discoveryUrl, - owner: dcrOwner, - // DCR slugs are server-keyed (Part A): the connect path no longer depends - // on the picker's app list, so it need not be threaded here. - declaredScopes: method.oauth?.scopes, + resourceFallback: args.method.oauth?.discoveryUrl, + declaredScopes: args.method.oauth?.scopes, redirectUri: oauthCallbackUrl(), integration, }, ); setDcrBusy(false); - trackEvent("connection_oauth_started", { - integration_slug: String(integration), - owner: dcrOwner, - flow: "dcr", - success: outcome.kind === "started", - ...(outcome.kind === "fallback" ? { dcr_fallback: true } : {}), - }); - if (outcome.kind === "fallback") { - setOAuthFallbackProbe("probe" in outcome ? outcome.probe : null); - setDcrFailed(true); - // Surface the server's actionable rejection reason on the recovery view as - // an inline error card. Generic fallbacks (no message) fall through to the - // "register an app" empty state, which already guides the user. - setDcrFallbackMessage("message" in outcome ? (outcome.message ?? null) : null); + + if (args.isReconnect) { + if (outcome.kind === "fallback" || outcome.kind === "failed") { + if (args.handoffKey) { + oauthReconnectOpenedKey.current = null; + } + toast.error( + outcome.kind === "fallback" && "message" in outcome && outcome.message + ? outcome.message + : "Reconnect failed: automatic setup unavailable", + ); + } + } else { + trackEvent("connection_oauth_started", { + integration_slug: String(integration), + owner: args.dcrOwner, + flow: "dcr", + success: outcome.kind === "started", + ...(outcome.kind === "fallback" ? { dcr_fallback: true } : {}), + }); + if (outcome.kind === "fallback") { + setOAuthFallbackProbe("probe" in outcome ? outcome.probe : null); + setDcrFailed(true); + setDcrFallbackMessage("message" in outcome ? (outcome.message ?? null) : null); + } else if (outcome.kind === "failed") { + setDcrFailed(true); + toast.error("Automatic setup failed"); + } } }; + // Transparent DCR connect: probe → register → start, no app picker. On any + // failure (probe error, no registration endpoint, or registration failure) we + // flip `dcrFailed` so the bring-your-own-app picker renders as the recovery + // path with name/owner kept. + const handleDcrConnect = async () => { + if (!method) return; + await executeDcrConnect({ + method, + connectionName: previewConnectionName(label, owner), + identityLabel: typedIdentityLabel(label), + dcrOwner: owner, + isReconnect: false, + }); + }; + return ( // Non-modal for the same reason as the health-check editor sheet: a modal // dialog's react-remove-scroll locks the wheel to the dialog subtree, so From 7bf45c04444d06eaab239f39c324456a3739bd0c Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 27 Aug 2026 21:46:20 -0700 Subject: [PATCH 2/8] Route DCR reconnect through the registration flow A dynamically registered OAuth client is bound to the redirect URI it registered with, so once the app's callback origin changed (127.0.0.1 to localhost) Reconnect kept starting the flow against the stored client and the authorization server rejected it, leaving no way to repair the connection. Reconnect now takes the same probe -> CIMD-or-register -> start route as the initial connect, sharing one runner rather than a second copy of the orchestration. Methods with a fixed, hand-registered app are unchanged. Fixes #1542 --- .../dcr-reconnect-through-registration.md | 9 + .../src/components/add-account-modal.test.ts | 118 ++++ .../src/components/add-account-modal.tsx | 549 +++++++++++------- 3 files changed, 459 insertions(+), 217 deletions(-) create mode 100644 .changeset/dcr-reconnect-through-registration.md diff --git a/.changeset/dcr-reconnect-through-registration.md b/.changeset/dcr-reconnect-through-registration.md new file mode 100644 index 000000000..dd0f1d815 --- /dev/null +++ b/.changeset/dcr-reconnect-through-registration.md @@ -0,0 +1,9 @@ +--- +"@executor-js/react": patch +--- + +**Reconnecting a DCR connection now re-registers instead of reusing a stranded client** + +A dynamically registered OAuth client is bound to the redirect URI it registered with. Once the app's callback origin changed (127.0.0.1 to localhost), Reconnect still started the flow against the stored client, and the authorization server rejected it — leaving no way to repair the connection. + +Reconnect now takes the same probe → CIMD-or-register → start route as the initial connect, so the registration gateway replaces the stranded client against the current redirect URI. Methods with a fixed, hand-registered app are unaffected and keep using their stored client. diff --git a/packages/react/src/components/add-account-modal.test.ts b/packages/react/src/components/add-account-modal.test.ts index 7094deeaf..77114d521 100644 --- a/packages/react/src/components/add-account-modal.test.ts +++ b/packages/react/src/components/add-account-modal.test.ts @@ -17,6 +17,7 @@ import { connectionLabelForHost, createCredentialPayloadOrigin, DEFAULT_CONNECTION_OWNER, + hasDcr, mergeCustomMethods, oauthIdentityLabelFromHealth, runAutomaticOAuthConnect, @@ -499,6 +500,123 @@ describe("runAutomaticOAuthConnect", () => { }); }); +// --------------------------------------------------------------------------- +// Reconnect (issue #1542). A DCR client is bound to the redirect URI it +// registered with, so when the app's callback origin moves (127.0.0.1 -> +// localhost) re-authorizing against the STORED client is rejected by the +// authorization server and the connection can never be repaired. Reconnect +// therefore takes the same probe -> (CIMD | register) -> start route as the +// initial connect, which re-registers against the CURRENT redirect URI. +// +// `hasDcr` is the routing decision the modal's reconnect handoff makes; the +// orchestrator run below is what that decision buys. +// --------------------------------------------------------------------------- +describe("hasDcr (which methods reconnect through the automatic path)", () => { + const oauthMethod = (oauth: NonNullable): AuthMethod => ({ + id: "oauth", + label: "OAuth", + kind: "oauth", + source: "spec", + template: AuthTemplateSlug.make("oauth"), + placements: [{ carrier: "header", name: "Authorization", prefix: "Bearer " }], + oauth, + }); + + it("routes a method advertising dynamic registration", () => { + expect(hasDcr(oauthMethod({ supportsDynamicRegistration: true }))).toBe(true); + }); + + it("routes a method carrying a discovery URL we can probe at connect time", () => { + expect(hasDcr(oauthMethod({ discoveryUrl: "https://mcp.example.com/mcp" }))).toBe(true); + }); + + // A fixed, hand-registered app has no stranded-client problem: its redirect + // URI is whatever the human entered, so reconnect keeps using it directly. + it("leaves a plain registered-app OAuth method on the stored-client path", () => { + expect(hasDcr(oauthMethod({ authorizationUrl: "https://auth.example.com/authorize" }))).toBe( + false, + ); + expect(hasDcr(oauthMethod({ supportsDynamicRegistration: false }))).toBe(false); + }); + + it("never routes a non-OAuth or absent method", () => { + expect(hasDcr(apiKeyMethod("api", "spec"))).toBe(false); + expect(hasDcr(undefined)).toBe(false); + expect(hasDcr(null)).toBe(false); + }); +}); + +describe("runAutomaticOAuthConnect (reconnect)", () => { + // The fix for #1542: reconnect must MINT a client against the redirect URI in + // force now, not reuse the one the connection was originally bound to. + it("re-registers against the current redirect URI and starts on the fresh client", async () => { + const popup = popupSpy(); + let registerArgs: RegisterArgs | null = null; + let startArgs: StartArgs | null = null; + + const outcome = await runDcrConnect( + { + ...popup, + probe: (): Promise => + Promise.resolve({ + authorizationUrl: "https://auth.example.com/authorize", + tokenUrl: "https://auth.example.com/token", + registrationEndpoint: "https://auth.example.com/register", + }), + register: (args: RegisterArgs): Promise => { + registerArgs = args; + return Promise.resolve(OAuthClientSlug.make("reconnected-app")); + }, + start: (args: StartArgs): void => { + startArgs = args; + }, + }, + { + discoveryUrl: "https://mcp.example.com/mcp", + // The connection was registered under the old origin; this is the one + // the app serves its callback on now. + redirectUri: "http://localhost:4788/api/oauth/callback", + owner: "user" as Owner, + integration: TEST_INTEGRATION, + }, + ); + + expect(outcome).toEqual({ kind: "started", flow: "dcr" }); + expect(registerArgs!.redirectUri).toBe("http://localhost:4788/api/oauth/callback"); + // The stranded client is replaced, not reused. + expect(startArgs!.client).toBe(OAuthClientSlug.make("reconnected-app")); + expect(startArgs!.owner).toBe("user"); + }); + + // Reconnect keeps the BYO picker as its recovery path, exactly as connect + // does, rather than dead-ending on a server that cannot self-register. + it("falls back with the probe when the server advertises no registration endpoint", async () => { + const popup = popupSpy(); + const outcome = await runDcrConnect( + { + ...popup, + probe: (): Promise => + Promise.resolve({ + authorizationUrl: "https://auth.example.com/authorize", + tokenUrl: "https://auth.example.com/token", + }), + register: (): Promise => + Promise.resolve(OAuthClientSlug.make("unexpected")), + start: (): void => {}, + }, + { + discoveryUrl: "https://mcp.example.com/mcp", + redirectUri: "http://localhost:4788/api/oauth/callback", + owner: "user" as Owner, + integration: TEST_INTEGRATION, + }, + ); + + expect(outcome).toMatchObject({ kind: "fallback", reason: "no-registration-endpoint" }); + expect(popup.calls).toEqual(["reserve", "release"]); + }); +}); + describe("runDcrConnect popup reservation", () => { const probeOk = (): Promise => Promise.resolve({ diff --git a/packages/react/src/components/add-account-modal.tsx b/packages/react/src/components/add-account-modal.tsx index 43f72ef5e..3213fcf36 100644 --- a/packages/react/src/components/add-account-modal.tsx +++ b/packages/react/src/components/add-account-modal.tsx @@ -933,6 +933,32 @@ export async function runAutomaticOAuthConnect( return { kind: "started", flow: "dcr" }; } +/** + * Can this method go through {@link runAutomaticOAuthConnect} at all? + * + * True when the integration advertises dynamic registration (MCP oauth2) OR + * carries a discovery URL we can probe at connect time — the probe decides + * between CIMD and DCR from there. Shared by the connect button and the + * reconnect handoff so both take the same route for the same method. + */ +export const hasDcr = (method: AuthMethod | undefined | null): boolean => + method?.kind === "oauth" && + (method.oauth?.supportsDynamicRegistration === true || method.oauth?.discoveryUrl != null); + +/** What a caller of the modal's `startAutomaticOAuthConnect` decides for itself. */ +type AutomaticOAuthConnectRequest = { + readonly method: AuthMethod; + readonly owner: Owner; + readonly connectionName: ConnectionName; + /** Stored on the connection; undefined leaves it untouched. */ + readonly identityLabel: string | undefined; + /** What the user typed, used to auto-name a NEW connection after connect. */ + readonly typedLabel: string; + /** A reconnect re-authorizes a connection that already exists; a connect + * creates one. The only behavioral difference between the two paths. */ + readonly mode: "connect" | "reconnect"; +}; + // --------------------------------------------------------------------------- // One row in the OAuth app picker: a radio-select Label plus an actions menu // (Edit / Remove) so the registered app can be managed inline. The page that @@ -1574,13 +1600,9 @@ function AddAccountModalView(props: AddAccountModalProps) { : `?${placement.name || "api_key"}=`; return `${lead}${placement.prefix ?? ""}`; }, [method, singleInput, isEnvMethod]); - // DCR-capable: the integration advertises dynamic registration (MCP oauth2), - // OR carries a discovery URL we can probe at connect time. When DCR-capable - // and not yet fallen back, we skip the app picker entirely (Option A). - const isDcr = - !cimdActive && - isOAuth && - (method?.oauth?.supportsDynamicRegistration === true || method?.oauth?.discoveryUrl != null); + // DCR-capable (see `hasDcr`). When DCR-capable and not yet fallen back, we + // skip the app picker entirely (Option A). + const isDcr = !cimdActive && hasDcr(method); const dcrActive = isDcr && !dcrFailed; const automaticOAuthActive = cimdActive || dcrActive; @@ -1780,90 +1802,42 @@ function AddAccountModalView(props: AddAccountModalProps) { // OAuth popup flow's busy state die with this instance. const close = useCallback(() => onOpenChange(false), [onOpenChange]); - useEffect(() => { - const handoff = initialState; - const oauthClient = handoff?.oauthClient; - if (!handoff || oauthClient?.action !== "reconnect") return; - if (oauthReconnectOpenedKey.current === handoff.key) return; - const client = oauthClient.slug; - const clientOwner = oauthClient.owner ?? handoff.owner; - const connectionOwner = handoff.owner; - const connectionName = handoff.label; - const oauthMethod = handoff.template - ? allMethods.find( - (m: AuthMethod) => - m.kind === "oauth" && - (m.id === handoff.template || String(m.template) === handoff.template), - ) - : allMethods.find((m: AuthMethod) => m.kind === "oauth"); - if (!client || !clientOwner || !connectionOwner || !connectionName || !oauthMethod) return; - - oauthReconnectOpenedKey.current = handoff.key; - setMethodId(oauthMethod.id); - void oauthPopup.start({ - payload: { - client: OAuthClientSlug.make(client), - clientOwner, - owner: connectionOwner, - name: ConnectionName.make(connectionName), - integration, - template: oauthMethod.template, - ...(handoff.identityLabel !== undefined ? { identityLabel: handoff.identityLabel } : {}), - }, - onAuthorizationStarted: () => { - trackEvent("connection_reconnected", { - integration_slug: String(integration), - owner: connectionOwner, - success: true, - }); - }, - onError: () => { - trackEvent("connection_reconnected", { - integration_slug: String(integration), - owner: connectionOwner, - success: false, - }); - }, - onSuccess: () => { - toast.success("Reconnected"); - close(); - }, - }); - }, [initialState, allMethods, integration, oauthPopup, close]); - - const probeAndAutoNameOAuthConnection = async ( - connection: OAuthCompletionPayload, - typedLabel: string, - ): Promise => { - const check = await doCheckConnectionHealth({ - params: { - owner: connection.owner, - integration: connection.integration, - name: connection.name, - }, - query: {}, - reactivityKeys: connectionCheckKeys, - }); - if (Exit.isFailure(check)) return; - const nextIdentityLabel = oauthIdentityLabelFromHealth({ - result: check.value, - typedLabel, - storedIdentityLabel: connection.identityLabel, - }); - if (nextIdentityLabel === null) return; - const updated = await doUpdateConnection({ - params: { - owner: connection.owner, - integration: connection.integration, - name: connection.name, - }, - payload: { identityLabel: nextIdentityLabel }, - reactivityKeys: connectionWriteKeys, - }); - if (Exit.isFailure(updated)) { - toast.error(messageFromExit(updated, "Couldn't update connection name")); - } - }; + // Stable identity: the reconnect effect below reaches this through + // `startAutomaticOAuthConnect`, so an identity that changed every render + // would re-run that effect on every keystroke. + const probeAndAutoNameOAuthConnection = useCallback( + async (connection: OAuthCompletionPayload, typedLabel: string): Promise => { + const check = await doCheckConnectionHealth({ + params: { + owner: connection.owner, + integration: connection.integration, + name: connection.name, + }, + query: {}, + reactivityKeys: connectionCheckKeys, + }); + if (Exit.isFailure(check)) return; + const nextIdentityLabel = oauthIdentityLabelFromHealth({ + result: check.value, + typedLabel, + storedIdentityLabel: connection.identityLabel, + }); + if (nextIdentityLabel === null) return; + const updated = await doUpdateConnection({ + params: { + owner: connection.owner, + integration: connection.integration, + name: connection.name, + }, + payload: { identityLabel: nextIdentityLabel }, + reactivityKeys: connectionWriteKeys, + }); + if (Exit.isFailure(updated)) { + toast.error(messageFromExit(updated, "Couldn't update connection name")); + } + }, + [doCheckConnectionHealth, doUpdateConnection], + ); const credentialPayloadOrigin = createCredentialPayloadOrigin({ origin: credentialOrigin, @@ -2155,23 +2129,27 @@ function AddAccountModalView(props: AddAccountModalProps) { }); }; - const createCimdClient = async (args: CimdCreateClientArgs): Promise => { - const exit = await doCreateOAuthClient({ - payload: { - owner: args.owner, - slug: args.slug, - authorizationUrl: args.authorizationUrl, - tokenUrl: args.tokenUrl, - resource: args.resource ?? null, - grant: args.grant, - clientId: args.clientId, - clientSecret: args.clientSecret, - }, - reactivityKeys: oauthClientWriteKeys, - }); - if (Exit.isFailure(exit)) return null; - return exit.value.client; - }; + // Stable identity for the same reason as probeAndAutoNameOAuthConnection. + const createCimdClient = useCallback( + async (args: CimdCreateClientArgs): Promise => { + const exit = await doCreateOAuthClient({ + payload: { + owner: args.owner, + slug: args.slug, + authorizationUrl: args.authorizationUrl, + tokenUrl: args.tokenUrl, + resource: args.resource ?? null, + grant: args.grant, + clientId: args.clientId, + clientSecret: args.clientSecret, + }, + reactivityKeys: oauthClientWriteKeys, + }); + if (Exit.isFailure(exit)) return null; + return exit.value.client; + }, + [doCreateOAuthClient], + ); const handleCimdConnect = async () => { const authorizationUrl = method?.oauth?.authorizationUrl; @@ -2234,124 +2212,261 @@ function AddAccountModalView(props: AddAccountModalProps) { } }; - // Automatic discovered OAuth connect: probe once, then prefer CIMD or use DCR + // Automatic discovered OAuth: probe once, then prefer CIMD or use DCR // according to the authorization server's advertised metadata. On failure we // flip `dcrFailed` so the bring-your-own-app picker remains the recovery path. + // + // Reconnect runs through here too, and must (issue #1542): a DCR client is + // bound to the redirect URI it registered with, so once the app's callback + // origin moves (127.0.0.1 -> localhost) re-authorizing against the STORED + // client fails at the authorization server. Re-probing and re-registering is + // what replaces that stranded client, and it is exactly what the connect path + // already does — so both take one route rather than two that drift. + const startAutomaticOAuthConnect = useCallback( + async (request: AutomaticOAuthConnectRequest): Promise => { + const { method: requestMethod, owner: dcrOwner, mode } = request; + const reconnect = mode === "reconnect"; + const discoveryUrl = requestMethod.oauth?.discoveryUrl ?? requestMethod.oauth?.tokenUrl; + if (!discoveryUrl) { + setDcrFailed(true); + return; + } + setDcrBusy(true); + const outcome = await runAutomaticOAuthConnect( + { + reserve: oauthPopup.reserve, + release: oauthPopup.releaseReservation, + probe: async (url: string): Promise => { + const exit = await doProbe({ payload: { url }, reactivityKeys: [] }); + if (Exit.isFailure(exit)) return null; + return exit.value; + }, + createCimdClient, + register: async ( + args: DcrRegisterArgs, + ): Promise => { + const exit = await doRegisterDynamic({ + payload: { + owner: args.owner, + slug: args.slug, + issuer: args.issuer ?? null, + registrationEndpoint: args.registrationEndpoint, + authorizationUrl: args.authorizationUrl, + tokenUrl: args.tokenUrl, + resource: args.resource ?? null, + scopes: args.scopes, + tokenEndpointAuthMethodsSupported: args.tokenEndpointAuthMethodsSupported, + clientName: args.clientName, + redirectUri: args.redirectUri, + originIntegration: args.originIntegration, + }, + reactivityKeys: oauthClientWriteKeys, + }); + if (Exit.isFailure(exit)) { + return { + error: messageFromExit( + exit, + "Automatic setup unavailable. Register an app instead.", + ), + }; + } + return exit.value.client; + }, + start: (args: DcrStartArgs): void => { + void oauthPopup.start({ + reservation: args.reservation, + payload: { + client: args.client, + // DCR/CIMD mints the client under the connection owner, so the + // app and connection share one owner. + clientOwner: args.owner, + owner: dcrOwner, + name: request.connectionName, + integration, + template: requestMethod.template, + ...(reconnect ? {} : { newConnection: true }), + ...(request.identityLabel !== undefined + ? { identityLabel: request.identityLabel } + : {}), + }, + ...(reconnect + ? { + onAuthorizationStarted: () => { + trackEvent("connection_reconnected", { + integration_slug: String(integration), + owner: dcrOwner, + success: true, + }); + }, + onError: () => { + trackEvent("connection_reconnected", { + integration_slug: String(integration), + owner: dcrOwner, + success: false, + }); + }, + } + : {}), + onSuccess: async (connection: OAuthCompletionPayload) => { + // A reconnect keeps the connection's existing name; only a new + // connection gets auto-named from what was probed. + if (!reconnect) { + await probeAndAutoNameOAuthConnection(connection, request.typedLabel); + } + toast.success(reconnect ? "Reconnected" : "Connection added"); + close(); + }, + }); + }, + }, + { + discoveryUrl, + // Only a genuine discovery URL (MCP) seeds the RFC 8707 resource + // indicator; the token-endpoint fallback baked into `discoveryUrl` must + // not, so pass the un-collapsed method value here. + resourceFallback: requestMethod.oauth?.discoveryUrl, + owner: dcrOwner, + // DCR slugs are server-keyed (Part A): the connect path no longer depends + // on the picker's app list, so it need not be threaded here. + declaredScopes: requestMethod.oauth?.scopes, + redirectUri: oauthCallbackUrl(), + integration, + cimd: { + integrationName, + clientIdMetadataDocumentUrl: oauthClientIdMetadataDocumentUrl(), + existingClients: clientSummaries, + }, + }, + ); + setDcrBusy(false); + // `connection_oauth_started` measures the connect funnel; a reconnect + // reports through `connection_reconnected` on the popup callbacks above, + // so it must not also land here. + if (!reconnect) { + trackEvent("connection_oauth_started", { + integration_slug: String(integration), + owner: dcrOwner, + flow: + outcome.kind === "started" + ? outcome.flow + : "probe" in outcome && outcome.probe.clientIdMetadataDocumentSupported === true + ? "cimd" + : "dcr", + success: outcome.kind === "started", + ...(outcome.kind === "fallback" ? { dcr_fallback: true } : {}), + }); + } + // Deliberately absent: a "popup-blocked" branch. Registering an app by hand + // does not make the browser open a window, so dropping to the BYO picker + // would send the user down a path that cannot succeed either. `reserve` + // already put the reason in `oauthPopup.error`, which the footer renders. + if (outcome.kind === "fallback") { + setOAuthFallbackProbe("probe" in outcome ? outcome.probe : null); + setDcrFailed(true); + // Surface the server's actionable rejection reason on the recovery view as + // an inline error card. Generic fallbacks (no message) fall through to the + // "register an app" empty state, which already guides the user. + setDcrFallbackMessage("message" in outcome ? (outcome.message ?? null) : null); + } + }, + [ + close, + clientSummaries, + createCimdClient, + doProbe, + doRegisterDynamic, + integration, + integrationName, + oauthPopup, + probeAndAutoNameOAuthConnection, + ], + ); + const handleAutomaticOAuthConnect = async () => { - const discoveryUrl = method?.oauth?.discoveryUrl ?? method?.oauth?.tokenUrl; - if (!method || !discoveryUrl) { + if (!method) { setDcrFailed(true); return; } - const dcrOwner = owner; - const connectionName = previewConnectionName(label, dcrOwner); - const identityLabel = typedIdentityLabel(label); - setDcrBusy(true); - const outcome = await runAutomaticOAuthConnect( - { - reserve: oauthPopup.reserve, - release: oauthPopup.releaseReservation, - probe: async (url: string): Promise => { - const exit = await doProbe({ payload: { url }, reactivityKeys: [] }); - if (Exit.isFailure(exit)) return null; - return exit.value; - }, - createCimdClient, - register: async ( - args: DcrRegisterArgs, - ): Promise => { - const exit = await doRegisterDynamic({ - payload: { - owner: args.owner, - slug: args.slug, - issuer: args.issuer ?? null, - registrationEndpoint: args.registrationEndpoint, - authorizationUrl: args.authorizationUrl, - tokenUrl: args.tokenUrl, - resource: args.resource ?? null, - scopes: args.scopes, - tokenEndpointAuthMethodsSupported: args.tokenEndpointAuthMethodsSupported, - clientName: args.clientName, - redirectUri: args.redirectUri, - originIntegration: args.originIntegration, - }, - reactivityKeys: oauthClientWriteKeys, - }); - if (Exit.isFailure(exit)) { - return { - error: messageFromExit(exit, "Automatic setup unavailable. Register an app instead."), - }; - } - return exit.value.client; - }, - start: (args: DcrStartArgs): void => { - void oauthPopup.start({ - reservation: args.reservation, - payload: { - client: args.client, - // DCR registers the client under the connection owner, so the app - // and connection share one owner. - clientOwner: args.owner, - owner: args.owner, - name: connectionName, - integration, - template: method.template, - newConnection: true, - ...(identityLabel !== undefined ? { identityLabel } : {}), - }, - onSuccess: async (connection: OAuthCompletionPayload) => { - await probeAndAutoNameOAuthConnection(connection, label); - toast.success("Connection added"); - close(); - }, - }); - }, - }, - { - discoveryUrl, - // Only a genuine discovery URL (MCP) seeds the RFC 8707 resource - // indicator; the token-endpoint fallback baked into `discoveryUrl` must - // not, so pass the un-collapsed method value here. - resourceFallback: method.oauth?.discoveryUrl, - owner: dcrOwner, - // DCR slugs are server-keyed (Part A): the connect path no longer depends - // on the picker's app list, so it need not be threaded here. - declaredScopes: method.oauth?.scopes, - redirectUri: oauthCallbackUrl(), + await startAutomaticOAuthConnect({ + method, + owner, + connectionName: previewConnectionName(label, owner), + identityLabel: typedIdentityLabel(label), + typedLabel: label, + mode: "connect", + }); + }; + + // The reconnect handoff: a connection asked to be re-authorized, so open its + // OAuth flow immediately. Fires once per handoff key (tracked by ref), which + // is also what makes a re-render mid-flight harmless. + // + // A DCR-capable method re-runs the automatic path rather than reusing the + // stored client — see `startAutomaticOAuthConnect`. Everything else has a + // fixed, registered app, so it starts the popup against that client directly. + useEffect(() => { + const handoff = initialState; + const oauthClient = handoff?.oauthClient; + if (!handoff || oauthClient?.action !== "reconnect") return; + if (oauthReconnectOpenedKey.current === handoff.key) return; + const client = oauthClient.slug; + const clientOwner = oauthClient.owner ?? handoff.owner; + const connectionOwner = handoff.owner; + const connectionName = handoff.label; + const oauthMethod = handoff.template + ? allMethods.find( + (m: AuthMethod) => + m.kind === "oauth" && + (m.id === handoff.template || String(m.template) === handoff.template), + ) + : allMethods.find((m: AuthMethod) => m.kind === "oauth"); + if (!client || !clientOwner || !connectionOwner || !connectionName || !oauthMethod) return; + + oauthReconnectOpenedKey.current = handoff.key; + setMethodId(oauthMethod.id); + + if (hasDcr(oauthMethod)) { + void startAutomaticOAuthConnect({ + method: oauthMethod, + owner: connectionOwner, + connectionName: ConnectionName.make(connectionName), + identityLabel: handoff.identityLabel, + typedLabel: connectionName, + mode: "reconnect", + }); + return; + } + + void oauthPopup.start({ + payload: { + client: OAuthClientSlug.make(client), + clientOwner, + owner: connectionOwner, + name: ConnectionName.make(connectionName), integration, - cimd: { - integrationName, - clientIdMetadataDocumentUrl: oauthClientIdMetadataDocumentUrl(), - existingClients: clientSummaries, - }, + template: oauthMethod.template, + ...(handoff.identityLabel !== undefined ? { identityLabel: handoff.identityLabel } : {}), + }, + onAuthorizationStarted: () => { + trackEvent("connection_reconnected", { + integration_slug: String(integration), + owner: connectionOwner, + success: true, + }); + }, + onError: () => { + trackEvent("connection_reconnected", { + integration_slug: String(integration), + owner: connectionOwner, + success: false, + }); + }, + onSuccess: () => { + toast.success("Reconnected"); + close(); }, - ); - setDcrBusy(false); - trackEvent("connection_oauth_started", { - integration_slug: String(integration), - owner: dcrOwner, - flow: - outcome.kind === "started" - ? outcome.flow - : "probe" in outcome && outcome.probe.clientIdMetadataDocumentSupported === true - ? "cimd" - : "dcr", - success: outcome.kind === "started", - ...(outcome.kind === "fallback" ? { dcr_fallback: true } : {}), }); - // Deliberately absent: a "popup-blocked" branch. Registering an app by hand - // does not make the browser open a window, so dropping to the BYO picker - // would send the user down a path that cannot succeed either. `reserve` - // already put the reason in `oauthPopup.error`, which the footer renders. - if (outcome.kind === "fallback") { - setOAuthFallbackProbe("probe" in outcome ? outcome.probe : null); - setDcrFailed(true); - // Surface the server's actionable rejection reason on the recovery view as - // an inline error card. Generic fallbacks (no message) fall through to the - // "register an app" empty state, which already guides the user. - setDcrFallbackMessage("message" in outcome ? (outcome.message ?? null) : null); - } - }; + }, [initialState, allMethods, integration, oauthPopup, close, startAutomaticOAuthConnect]); return ( // Non-modal for the same reason as the health-check editor sheet: a modal From f1e13df01efee9b3817a1de066b888c412eaf948 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Fri, 28 Aug 2026 23:45:03 -0700 Subject: [PATCH 3/8] Add an e2e repro for DCR reconnect after callback-origin drift --- .../mcp-oauth-reconnect-origin-drift.test.ts | 276 ++++++++++++++++++ 1 file changed, 276 insertions(+) create mode 100644 e2e/selfhost/mcp-oauth-reconnect-origin-drift.test.ts diff --git a/e2e/selfhost/mcp-oauth-reconnect-origin-drift.test.ts b/e2e/selfhost/mcp-oauth-reconnect-origin-drift.test.ts new file mode 100644 index 000000000..0658e4b72 --- /dev/null +++ b/e2e/selfhost/mcp-oauth-reconnect-origin-drift.test.ts @@ -0,0 +1,276 @@ +// Selfhost repro for #1542: a dynamically registered (RFC 7591) OAuth client is +// bound to the exact callback URL it registered with the authorization server. +// Executor's callback origin is not stable for the life of a connection (a +// desktop sidecar on 127.0.0.1 vs a CLI daemon on localhost, or a self-hosted +// instance that moves domain), so a DCR client can outlive the origin it was +// registered for. Reconnect used to re-send that stranded client on every +// attempt — the authorization server answered `invalid_request: redirect_uri +// is not registered` forever, and the only exit was deleting the OAuth app by +// hand. Add connection recovered (probe → register-dynamic → start reaches the +// #1443 reuse gate); Reconnect did not. +// +// The contract under test: Reconnect takes the same probe → register-dynamic → +// start route as the initial connect, so the registration gate declines the +// redirect-mismatched client, mints a fresh one bound to the CURRENT callback, +// and re-minting rebinds the SAME connection row — no orphaned grant state. +import { randomBytes } from "node:crypto"; + +import { expect } from "@effect/vitest"; +import { Effect } from "effect"; +import type { Page } from "playwright"; +import { composePluginApi } from "@executor-js/api/server"; +import { mcpHttpPlugin } from "@executor-js/plugin-mcp/api"; +import { + AuthTemplateSlug, + ConnectionName, + IntegrationSlug, + OAuthClientSlug, +} from "@executor-js/sdk/shared"; +import { serveOAuthTestServer } from "@executor-js/sdk/testing"; + +import { scenario } from "../src/scenario"; +import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; + +const api = composePluginApi([mcpHttpPlugin()] as const); + +const name = ConnectionName.make("main"); +const template = AuthTemplateSlug.make("oauth2"); + +/** The callback the DCR client was registered under — an origin the app no + * longer serves. Only its string identity matters: seeding completes the + * authorization out of band, so nothing ever listens here (mirrors the + * desktop-sidecar origin from the report). */ +const STALE_REDIRECT_URI = "http://127.0.0.1:64999/api/oauth/callback"; + +const connectionsSection = (page: Page) => + page.locator("section").filter({ + has: page.getByRole("heading", { level: 3, name: "Connections" }), + }); + +const requiredRedirect = (response: Response, from: string): string => { + const location = response.headers.get("location"); + if (!location) { + throw new Error(`Expected redirect from ${from}, got HTTP ${response.status}`); + } + return new URL(location, from).toString(); +}; + +/** The test server's login page is plain text with Basic-auth POST — nothing a + * browser can click. Complete it out of band and hand back the callback URL. */ +const submitProviderLogin = async (loginUrl: string): Promise => { + const credentials = Buffer.from("alice:password").toString("base64"); + const response = await fetch(loginUrl, { + method: "POST", + redirect: "manual", + headers: { authorization: `Basic ${credentials}` }, + }); + const location = response.headers.get("location"); + if (response.status !== 302 || !location) { + throw new Error(`provider login did not redirect (${response.status})`); + } + return new URL(location, loginUrl).toString(); +}; + +const completeAuthorization = (authorizationUrl: string) => + Effect.promise(async () => { + const login = await fetch(authorizationUrl, { redirect: "manual" }); + const loginUrl = requiredRedirect(login, authorizationUrl); + const callbackUrl = await submitProviderLogin(loginUrl); + const parsed = new URL(callbackUrl); + const code = parsed.searchParams.get("code"); + if (!code) throw new Error(`OAuth callback did not include a code: ${callbackUrl}`); + return { code }; + }); + +scenario( + "MCP OAuth · reconnect recovers a DCR connection whose callback origin changed", + { timeout: 240_000 }, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const browser = yield* Browser; + const { client: makeApiClient } = yield* Api; + const identity = yield* target.newIdentity(); + const client = yield* makeApiClient(api, identity); + + const oauth = yield* serveOAuthTestServer({ + scopes: ["channels:history", "users:read"], + }); + + const slug = IntegrationSlug.make(`mcp-origin-drift-${randomBytes(4).toString("hex")}`); + const clientSlug = OAuthClientSlug.make(`origin-drift-${randomBytes(4).toString("hex")}`); + + yield* client.mcp.addServer({ + payload: { + transport: "remote", + name: `Origin drift repro ${String(slug)}`, + endpoint: oauth.mcpResourceUrl, + slug: String(slug), + authenticationTemplate: [{ kind: "oauth2" }], + }, + }); + yield* Effect.addFinalizer(() => + client.mcp.removeServer({ params: { slug } }).pipe(Effect.ignore), + ); + // The recovery mints a fresh server-slugged client mid-scenario; the org + // is scenario-fresh, so reap every client it accumulated rather than + // guessing derived slugs. + yield* Effect.addFinalizer(() => + client.oauth.listClients().pipe( + Effect.flatMap((clients) => + Effect.forEach(clients, (candidate) => + client.oauth + .removeClient({ + params: { slug: candidate.slug }, + payload: { owner: candidate.owner }, + }) + .pipe(Effect.ignore), + ), + ), + Effect.ignore, + ), + ); + + // Seed the stranded state from the report: the DCR client registers the + // STALE callback with the authorization server (as if minted weeks ago + // under the old origin), and the connection completes against it out of + // band — so the connection is live while its client is bound to a + // callback the app no longer serves. + const probe = yield* client.oauth.probe({ payload: { url: oauth.mcpResourceUrl } }); + if (!probe.registrationEndpoint) { + return yield* Effect.die("OAuth probe did not discover a DCR registration endpoint"); + } + const registered = yield* client.oauth.registerDynamic({ + payload: { + owner: "org", + slug: clientSlug, + issuer: probe.issuer ?? null, + registrationEndpoint: probe.registrationEndpoint, + authorizationUrl: probe.authorizationUrl, + tokenUrl: probe.tokenUrl, + resource: probe.resource ?? oauth.mcpResourceUrl, + scopes: probe.scopesSupported ?? [], + tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported, + clientName: "Executor e2e origin drift repro", + redirectUri: STALE_REDIRECT_URI, + originIntegration: slug, + }, + }); + const started = yield* client.oauth.start({ + payload: { + owner: "org", + client: registered.client, + clientOwner: "org", + name, + integration: slug, + template, + redirectUri: STALE_REDIRECT_URI, + }, + }); + expect(started.status, "seeding starts an authorization-code redirect").toBe("redirect"); + if (started.status !== "redirect") return yield* Effect.die("OAuth start did not redirect"); + const callback = yield* completeAuthorization(started.authorizationUrl); + yield* client.oauth.complete({ payload: { state: started.state, code: callback.code } }); + yield* Effect.addFinalizer(() => + client.connections + .remove({ params: { owner: "org", integration: slug, name } }) + .pipe(Effect.ignore), + ); + + // Wire truth for the seeded trap: the AS holds exactly one registration, + // and it is bound to the stale callback. + const seedRequests = yield* oauth.requests; + const seedRegistrations = seedRequests.filter( + (request) => request.method === "POST" && request.path === "/register", + ); + expect(seedRegistrations, "seeding registered exactly one client").toHaveLength(1); + expect( + seedRegistrations[0]?.body ?? "", + "the seeded client is bound to the stale callback", + ).toContain(STALE_REDIRECT_URI); + yield* oauth.clearRequests; + + yield* browser.session(identity, async ({ page, step }) => { + const connections = connectionsSection(page); + const menuTrigger = connections.locator('button[aria-haspopup="menu"]').first(); + + await step("Open the MCP integration with its origin-drifted connection", async () => { + await visit(page, `/integrations/${String(slug)}`); + await connections.getByText("main", { exact: true }).waitFor({ timeout: 30_000 }); + }); + + await step("Reconnect and complete the OAuth flow in the popup", async () => { + const popupPromise = page.waitForEvent("popup", { timeout: 30_000 }); + await menuTrigger.click(); + await page.getByRole("menuitem", { name: "Reconnect" }).click(); + const popup = await popupPromise; + + // #1542's dead end lived here: reconnect re-sent the stranded client, + // so the popup landed on the authorization server's `invalid_request: + // redirect_uri is not registered` JSON and never reached the login + // page. Surface that page instead of a bare timeout when it regresses. + try { + await popup.waitForURL(/\/login\?/, { timeout: 30_000 }); + } catch (cause) { + const body = await popup + .locator("body") + .innerText() + .catch(() => ""); + throw new Error( + `Reconnect dead-ended before the provider login page. ` + + `Popup URL: ${popup.url()}; body: ${body}`, + { cause }, + ); + } + // The test AS login page is plain text driven by Basic-auth POST, so + // complete it out of band and drive the popup to the callback — the + // same journey a user's click-through consent takes. + const callbackUrl = await submitProviderLogin(popup.url()); + await popup.goto(callbackUrl); + await page.getByText("Reconnected", { exact: true }).waitFor({ timeout: 30_000 }); + }); + }); + + // Wire truth for the recovery: reconnect re-registered a client for the + // callback the app serves NOW instead of re-sending the stranded one, and + // the authorization it started used that fresh registration. + const requests = yield* oauth.requests; + const registration = requests.find( + (request) => request.method === "POST" && request.path === "/register", + ); + expect( + registration, + "reconnect registers a fresh client instead of re-sending the stranded one", + ).toBeDefined(); + expect( + registration?.body ?? "", + "the fresh registration is bound to the current callback, not the stale one", + ).not.toContain(STALE_REDIRECT_URI); + expect( + registration?.body ?? "", + "the fresh registration carries the app's callback path", + ).toContain("/api/oauth/callback"); + const authorize = requests.find( + (request) => request.method === "GET" && request.path === "/authorize", + ); + expect(authorize, "the popup reached the authorize endpoint").toBeDefined(); + + // No orphaned grant state: the SAME connection row is rebound to the + // fresh client and its new grant is healthy end to end. + const health = yield* client.connections.checkHealth({ + params: { owner: "org", integration: slug, name }, + query: {}, + }); + expect(health.status, "the reconnected grant is healthy").toBe("healthy"); + // The stranded client row deliberately survives (it stays valid for + // refresh, and `createClient` must never clobber it) — the recovery adds + // a second, freshly bound client rather than editing the stranded one. + const clients = yield* client.oauth.listClients(); + expect( + clients.length, + "the stranded client survives alongside the freshly registered one", + ).toBeGreaterThanOrEqual(2); + }), + ), +); From 3a9c631ae3e15f0e12ca11414762108454f53e66 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Sat, 29 Aug 2026 00:26:47 -0700 Subject: [PATCH 4/8] Route reconnect by stored client binding, reuse drift-recovery DCR clients, abort automatic connect on modal close --- .../mcp-oauth-reconnect-origin-drift.test.ts | 254 +++++++++++++----- packages/core/sdk/src/client.ts | 13 +- .../sdk/src/oauth-register-dynamic.test.ts | 50 ++++ packages/core/sdk/src/oauth-service.ts | 19 +- .../react/src/components/accounts-section.tsx | 59 +++- .../src/components/add-account-modal.test.ts | 188 ++++++++++++- .../src/components/add-account-modal.tsx | 94 ++++++- .../react/src/plugins/oauth-reconnect.test.ts | 79 ++++++ packages/react/src/plugins/oauth-reconnect.ts | 35 ++- 9 files changed, 709 insertions(+), 82 deletions(-) diff --git a/e2e/selfhost/mcp-oauth-reconnect-origin-drift.test.ts b/e2e/selfhost/mcp-oauth-reconnect-origin-drift.test.ts index 0658e4b72..30f016e8a 100644 --- a/e2e/selfhost/mcp-oauth-reconnect-origin-drift.test.ts +++ b/e2e/selfhost/mcp-oauth-reconnect-origin-drift.test.ts @@ -43,11 +43,23 @@ const template = AuthTemplateSlug.make("oauth2"); * desktop-sidecar origin from the report). */ const STALE_REDIRECT_URI = "http://127.0.0.1:64999/api/oauth/callback"; +/** Client the authorization server knows UP FRONT (`clients` option below) — + * the bring-your-own-app case. Reconnect must keep using it directly: routing + * it through probe/registration would rebind the connection to an auto-minted + * client behind the user's back. */ +const STATIC_CLIENT_ID = "static-byo-client"; +const STATIC_CLIENT_SECRET = "static-byo-secret"; + const connectionsSection = (page: Page) => page.locator("section").filter({ has: page.getByRole("heading", { level: 3, name: "Connections" }), }); +const connectionRow = (page: Page, label: string) => + connectionsSection(page) + .locator('[data-slot="card-stack-entry"]') + .filter({ has: page.getByText(label, { exact: true }) }); + const requiredRedirect = (response: Response, from: string): string => { const location = response.headers.get("location"); if (!location) { @@ -85,7 +97,7 @@ const completeAuthorization = (authorizationUrl: string) => scenario( "MCP OAuth · reconnect recovers a DCR connection whose callback origin changed", - { timeout: 240_000 }, + { timeout: 420_000 }, Effect.scoped( Effect.gen(function* () { const target = yield* Target; @@ -96,6 +108,7 @@ scenario( const oauth = yield* serveOAuthTestServer({ scopes: ["channels:history", "users:read"], + clients: { [STATIC_CLIENT_ID]: STATIC_CLIENT_SECRET }, }); const slug = IntegrationSlug.make(`mcp-origin-drift-${randomBytes(4).toString("hex")}`); @@ -189,88 +202,209 @@ scenario( seedRegistrations[0]?.body ?? "", "the seeded client is bound to the stale callback", ).toContain(STALE_REDIRECT_URI); - yield* oauth.clearRequests; - yield* browser.session(identity, async ({ page, step }) => { - const connections = connectionsSection(page); - const menuTrigger = connections.locator('button[aria-haspopup="menu"]').first(); + // A second connection on the SAME discovery-capable integration, backed + // by a STATIC (BYO) client the authorization server already knows. Its + // seeding start omits `redirectUri`, so it binds to the callback the app + // serves NOW — nothing about it needs recovery, and reconnect must leave + // its client binding alone. + const staticName = ConnectionName.make("static"); + const staticSlug = OAuthClientSlug.make(`static-byo-${randomBytes(4).toString("hex")}`); + yield* client.oauth.createClient({ + payload: { + owner: "org", + slug: staticSlug, + authorizationUrl: probe.authorizationUrl, + tokenUrl: probe.tokenUrl, + grant: "authorization_code", + clientId: STATIC_CLIENT_ID, + clientSecret: STATIC_CLIENT_SECRET, + originIntegration: slug, + }, + }); + const staticStarted = yield* client.oauth.start({ + payload: { + owner: "org", + client: staticSlug, + clientOwner: "org", + name: staticName, + integration: slug, + template, + }, + }); + expect(staticStarted.status, "static seeding starts an authorization-code redirect").toBe( + "redirect", + ); + if (staticStarted.status !== "redirect") { + return yield* Effect.die("static OAuth start did not redirect"); + } + const staticCallback = yield* completeAuthorization(staticStarted.authorizationUrl); + yield* client.oauth.complete({ + payload: { state: staticStarted.state, code: staticCallback.code }, + }); + yield* Effect.addFinalizer(() => + client.connections + .remove({ params: { owner: "org", integration: slug, name: staticName } }) + .pipe(Effect.ignore), + ); + yield* oauth.clearRequests; - await step("Open the MCP integration with its origin-drifted connection", async () => { - await visit(page, `/integrations/${String(slug)}`); - await connections.getByText("main", { exact: true }).waitFor({ timeout: 30_000 }); - }); + // One Reconnect journey: open the row's menu, complete the provider + // login in the popup, and wait for the success toast. Visits the + // integration page first so a toast from an earlier reconnect can never + // satisfy this one's wait. + const reconnectThroughUi = (label: string, description: string) => + browser.session(identity, async ({ page, step }) => { + const connections = connectionsSection(page); + await step(`Open the MCP integration and find the ${label} connection`, async () => { + await visit(page, `/integrations/${String(slug)}`); + await connections.getByText(label, { exact: true }).waitFor({ timeout: 30_000 }); + }); - await step("Reconnect and complete the OAuth flow in the popup", async () => { - const popupPromise = page.waitForEvent("popup", { timeout: 30_000 }); - await menuTrigger.click(); - await page.getByRole("menuitem", { name: "Reconnect" }).click(); - const popup = await popupPromise; + await step(description, async () => { + const popupPromise = page.waitForEvent("popup", { timeout: 30_000 }); + await connectionRow(page, label).locator('button[aria-haspopup="menu"]').click(); + await page.getByRole("menuitem", { name: "Reconnect" }).click(); + const popup = await popupPromise; - // #1542's dead end lived here: reconnect re-sent the stranded client, - // so the popup landed on the authorization server's `invalid_request: - // redirect_uri is not registered` JSON and never reached the login - // page. Surface that page instead of a bare timeout when it regresses. - try { - await popup.waitForURL(/\/login\?/, { timeout: 30_000 }); - } catch (cause) { - const body = await popup - .locator("body") - .innerText() - .catch(() => ""); - throw new Error( - `Reconnect dead-ended before the provider login page. ` + - `Popup URL: ${popup.url()}; body: ${body}`, - { cause }, - ); - } - // The test AS login page is plain text driven by Basic-auth POST, so - // complete it out of band and drive the popup to the callback — the - // same journey a user's click-through consent takes. - const callbackUrl = await submitProviderLogin(popup.url()); - await popup.goto(callbackUrl); - await page.getByText("Reconnected", { exact: true }).waitFor({ timeout: 30_000 }); + // #1542's dead end lived here: reconnect re-sent the stranded client, + // so the popup landed on the authorization server's `invalid_request: + // redirect_uri is not registered` JSON and never reached the login + // page. Surface that page instead of a bare timeout when it regresses. + try { + await popup.waitForURL(/\/login\?/, { timeout: 30_000 }); + } catch (cause) { + const body = await popup + .locator("body") + .innerText() + .catch(() => ""); + throw new Error( + `Reconnect dead-ended before the provider login page. ` + + `Popup URL: ${popup.url()}; body: ${body}`, + { cause }, + ); + } + // The test AS login page is plain text driven by Basic-auth POST, so + // complete it out of band and drive the popup to the callback — the + // same journey a user's click-through consent takes. + const callbackUrl = await submitProviderLogin(popup.url()); + await popup.goto(callbackUrl); + await page.getByText("Reconnected", { exact: true }).waitFor({ timeout: 30_000 }); + }); }); - }); - // Wire truth for the recovery: reconnect re-registered a client for the - // callback the app serves NOW instead of re-sending the stranded one, and - // the authorization it started used that fresh registration. - const requests = yield* oauth.requests; - const registration = requests.find( - (request) => request.method === "POST" && request.path === "/register", + const registrationsIn = ( + requests: readonly R[], + ): readonly R[] => + requests.filter((request) => request.method === "POST" && request.path === "/register"); + const dcrClientCount = Effect.map( + client.oauth.listClients(), + (clients) => + clients.filter((candidate) => candidate.origin.kind === "dynamic_client_registration") + .length, ); + const mainConnection = client.connections.get({ + params: { owner: "org", integration: slug, name }, + }); + + yield* reconnectThroughUi("main", "Reconnect the drifted connection and complete OAuth"); + + // Wire truth for the recovery: reconnect re-registered EXACTLY ONE client + // for the callback the app serves NOW instead of re-sending the stranded + // one, and the authorization it started used that fresh registration. + const recoveryRequests = yield* oauth.requests; + const recoveryRegistrations = registrationsIn(recoveryRequests); expect( - registration, - "reconnect registers a fresh client instead of re-sending the stranded one", - ).toBeDefined(); + recoveryRegistrations, + "the drift reconnect registers exactly one fresh client", + ).toHaveLength(1); expect( - registration?.body ?? "", + recoveryRegistrations[0]?.body ?? "", "the fresh registration is bound to the current callback, not the stale one", ).not.toContain(STALE_REDIRECT_URI); expect( - registration?.body ?? "", + recoveryRegistrations[0]?.body ?? "", "the fresh registration carries the app's callback path", ).toContain("/api/oauth/callback"); - const authorize = requests.find( + const authorize = recoveryRequests.find( (request) => request.method === "GET" && request.path === "/authorize", ); expect(authorize, "the popup reached the authorize endpoint").toBeDefined(); + // The stranded client row deliberately survives (it stays valid for + // refresh, and `createClient` must never clobber it) — the recovery adds + // exactly ONE freshly bound client rather than editing the stranded one. + expect( + yield* dcrClientCount, + "the stranded DCR client survives alongside exactly one fresh registration", + ).toBe(2); + const mainAfterRecovery = yield* mainConnection; + expect( + String(mainAfterRecovery.oauthClient), + "the connection is rebound off the stranded client", + ).not.toBe(String(clientSlug)); + yield* oauth.clearRequests; - // No orphaned grant state: the SAME connection row is rebound to the - // fresh client and its new grant is healthy end to end. + // A SECOND reconnect at the SAME origin must reuse the recovery client: + // zero additional registrations, zero additional client rows, and the + // connection keeps the binding the recovery minted. Registering again on + // every reconnect after the first drift is the regression class here. + yield* reconnectThroughUi("main", "Reconnect again at the same origin"); + const repeatRequests = yield* oauth.requests; + expect( + registrationsIn(repeatRequests), + "a reconnect at an unchanged origin registers nothing", + ).toHaveLength(0); + expect( + repeatRequests.filter( + (request) => request.method === "GET" && request.path === "/authorize", + ), + "the repeat reconnect still ran a real authorization", + ).toHaveLength(1); + expect(yield* dcrClientCount, "no additional DCR client row is minted").toBe(2); + const mainAfterRepeat = yield* mainConnection; + expect( + String(mainAfterRepeat.oauthClient), + "the repeat reconnect reuses the recovery client binding", + ).toBe(String(mainAfterRecovery.oauthClient)); + yield* oauth.clearRequests; + + // The static (BYO) connection reconnects DIRECTLY through its stored + // client: no probe-driven registration, and the binding is untouched — + // a discovery-capable integration must not hijack a static binding into + // the automatic flow. + yield* reconnectThroughUi("static", "Reconnect the static-client connection"); + const staticRequests = yield* oauth.requests; + expect( + registrationsIn(staticRequests), + "a static-client reconnect never touches the registration endpoint", + ).toHaveLength(0); + expect( + staticRequests.filter( + (request) => request.method === "GET" && request.path === "/authorize", + ), + "the static reconnect ran a real authorization", + ).toHaveLength(1); + expect(yield* dcrClientCount, "the static reconnect mints no DCR client").toBe(2); + const staticConnection = yield* client.connections.get({ + params: { owner: "org", integration: slug, name: staticName }, + }); + expect( + String(staticConnection.oauthClient), + "the static connection keeps its BYO client binding", + ).toBe(String(staticSlug)); + + // No orphaned grant state: the SAME connection rows are rebound and both + // grants are healthy end to end. const health = yield* client.connections.checkHealth({ params: { owner: "org", integration: slug, name }, query: {}, }); expect(health.status, "the reconnected grant is healthy").toBe("healthy"); - // The stranded client row deliberately survives (it stays valid for - // refresh, and `createClient` must never clobber it) — the recovery adds - // a second, freshly bound client rather than editing the stranded one. - const clients = yield* client.oauth.listClients(); - expect( - clients.length, - "the stranded client survives alongside the freshly registered one", - ).toBeGreaterThanOrEqual(2); + const staticHealth = yield* client.connections.checkHealth({ + params: { owner: "org", integration: slug, name: staticName }, + query: {}, + }); + expect(staticHealth.status, "the static grant is healthy after reconnect").toBe("healthy"); }), ), ); diff --git a/packages/core/sdk/src/client.ts b/packages/core/sdk/src/client.ts index 34e3ded3b..42bdd462f 100644 --- a/packages/core/sdk/src/client.ts +++ b/packages/core/sdk/src/client.ts @@ -168,7 +168,18 @@ export interface IntegrationAccountHandoff { readonly clientId?: string; readonly authorizationUrl?: string; readonly tokenUrl?: string; - readonly resource?: string; + /** RFC 8707 resource indicator. On a reconnect handoff this is the STORED + * client's value, and an EXPLICIT null means the stored client was + * registered WITHOUT a resource indicator — that absence must survive a + * re-registration (some servers reject any `resource` parameter). + * Undefined means no stored value was carried. */ + readonly resource?: string | null; + /** Reconnect only: the stored client binding is an auto-minted DCR client + * (or its row is gone), so the modal may re-run the automatic + * probe/registration flow. Absent or false pins the reconnect to the + * stored client — a static/BYO or first-party binding must never be + * silently rebound to an automatic client. */ + readonly dynamicRegistration?: boolean; }; } diff --git a/packages/core/sdk/src/oauth-register-dynamic.test.ts b/packages/core/sdk/src/oauth-register-dynamic.test.ts index 182c031c1..f52f5b993 100644 --- a/packages/core/sdk/src/oauth-register-dynamic.test.ts +++ b/packages/core/sdk/src/oauth-register-dynamic.test.ts @@ -656,6 +656,56 @@ describe("oauth.registerDynamicClient", () => { ), ); + // After the A→B drift recovery above, the owner holds TWO matching-resource + // clients: the stale one (bound to redirect A, oldest) and the recovery one + // (bound to redirect B). The reuse decision must prefer a candidate matching + // resource AND the current redirect across ALL candidates — taking only the + // OLDEST matching-resource candidate and then checking its redirect would + // mint yet another client on EVERY reconnect after the first drift. + it.effect( + "reuses the drift-recovery client on later reconnects instead of registering again", + () => + Effect.scoped( + Effect.gen(function* () { + const server = yield* serveOAuthTestServer({ scopes: ["read"] }); + const { executor } = yield* makeTestWorkspaceHarness({ plugins }); + yield* executor.acme.seed(); + const probe = yield* executor.oauth.probe({ url: server.mcpResourceUrl }); + + const registerAt = (slug: string, redirectUri: string) => + executor.oauth.registerDynamicClient({ + owner: "org", + slug: OAuthClientSlug.make(slug), + issuer: probe.issuer, + registrationEndpoint: probe.registrationEndpoint!, + authorizationUrl: probe.authorizationUrl, + tokenUrl: probe.tokenUrl, + resource: probe.resource, + scopes: ["read"], + tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported, + clientName: "Acme DCR", + redirectUri, + originIntegration: INTEG, + }); + + // Original sandbox at redirect A, then the drift recovery at redirect B. + yield* registerAt("original-sandbox", FLOW_REDIRECT_URI); + const driftedRedirectUri = "https://localhost:6410/api/oauth/callback"; + const recovered = yield* registerAt("recreated-sandbox", driftedRedirectUri); + yield* server.clearRequests; + + // A later reconnect at the SAME (current) redirect B: the recovery + // client already matches resource + redirect, so it is reused — no + // third registration, no third row. + const reused = yield* registerAt("later-reconnect", driftedRedirectUri); + expect(registerRequestCount(yield* server.requests)).toBe(0); + expect(String(reused)).toBe(String(recovered)); + const clients = yield* executor.oauth.listClients(); + expect(clients).toHaveLength(2); + }), + ), + ); + // Regression: Mercury's authorization server vets `client_name` and rejects // any value containing its own brand with `invalid_client_metadata`, which // the old auto-generated "Executor for Mercury MCP" always tripped. The UI diff --git a/packages/core/sdk/src/oauth-service.ts b/packages/core/sdk/src/oauth-service.ts index 054bde564..5d7b0ef8a 100644 --- a/packages/core/sdk/src/oauth-service.ts +++ b/packages/core/sdk/src/oauth-service.ts @@ -1144,9 +1144,22 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { // token grant doesn't involve the redirect URI). const takenSlugs = new Set(candidates.map((client) => String(client.slug))); if (resource !== null) { - const matchingResource = candidates.find((client) => client.resource === resource); - if (matchingResource && redirectMatches(matchingResource)) { - return { existingSlug: matchingResource.slug, registrationSlug: matchingResource.slug }; + // Prefer a candidate matching resource AND the current redirect across + // ALL candidates (mirroring the resource-less branch below). Candidates + // are oldest-first, so after an origin drift the oldest matching- + // resource row is the STRANDED one — but the first drift recovery + // already minted a client bound to the CURRENT callback, and later + // reconnects must reuse that instead of registering another duplicate + // each time. Known limitation: the legacy null-redirect rule in + // `redirectMatches` (a legacy row with no stored redirect matches any + // flow redirect) still lets such a row win over a later, exactly- + // matching one; kept deliberately so upgrades don't re-register every + // client whose callback never changed. + const reusable = candidates.find( + (client) => client.resource === resource && redirectMatches(client), + ); + if (reusable) { + return { existingSlug: reusable.slug, registrationSlug: reusable.slug }; } const slug = uniqueDcrSlug( dcrClientSlug(issuer, candidates.length > 0 ? resource : null, input.slug), diff --git a/packages/react/src/components/accounts-section.tsx b/packages/react/src/components/accounts-section.tsx index d6fe45b05..26ec69912 100644 --- a/packages/react/src/components/accounts-section.tsx +++ b/packages/react/src/components/accounts-section.tsx @@ -2,13 +2,19 @@ import { useEffect, useMemo, useState } from "react"; import { useAtomValue, useAtomSet } from "@effect/atom-react"; import * as AsyncResult from "effect/unstable/reactivity/AsyncResult"; import * as Exit from "effect/Exit"; -import { IntegrationSlug, type Connection, type Owner } from "@executor-js/sdk/shared"; +import { + IntegrationSlug, + type Connection, + type OAuthClientSummary, + type Owner, +} from "@executor-js/sdk/shared"; import type { IntegrationAccountHandoff } from "@executor-js/sdk/client"; import { toast } from "sonner"; import { addConnectionOptimistic, connectionsForIntegrationAtom, + oauthClientsOptimisticAtom, refreshConnection, removeConnectionOptimistic, startOAuth, @@ -23,11 +29,13 @@ import type { AuthMethod } from "../lib/auth-placements"; import { connectionNeedsReconsent, oauthReconnectPayload, + reconnectAllowsAutomaticRegistration, reconnectMode, + reconnectStoredClient, reconsentRequiredScopes, } from "../plugins/oauth-reconnect"; import { useOAuthPopupFlow } from "../plugins/oauth-sign-in"; -import { AddAccountModal } from "./add-account-modal"; +import { AddAccountModal, hasDcr } from "./add-account-modal"; import { ConnectionEditSheet } from "./metadata-edit-sheet"; import type { CreateCustomMethod } from "./add-custom-method-modal"; import { @@ -289,13 +297,24 @@ function OwnerAccounts(props: { readonly showOwnerLabels: boolean; readonly methods: readonly AuthMethod[]; readonly onEdit: (connection: Connection) => void; - readonly onDcrReconnect: (connection: Connection) => void; + /** Hand the connection to the modal's automatic reconnect flow. Only called + * once the stored binding was vetted as auto-minted DCR (or gone); + * `storedClient` is that binding's summary — undefined when the row is + * gone — so the handoff can carry its resource. */ + readonly onDcrReconnect: ( + connection: Connection, + storedClient: OAuthClientSummary | undefined, + ) => void; /** The integration's declared oauth scopes — compared against each connection's * granted `oauthScope` to flag connections that must reconnect for new access. */ readonly declaredScopes: readonly string[] | undefined; }) { const { integration, owner } = props; const connections = useAtomValue(connectionsForIntegrationAtom({ integration, owner })); + // Registered-app summaries: the Reconnect routing below inspects the STORED + // client binding (its origin kind and resource), not just the method's + // capability, before sending a connection into the automatic flow. + const allClients = useAtomValue(oauthClientsOptimisticAtom); // Removal confirms in a dialog. State lives here (not in the row) because the // Remove menu item closes its dropdown on click, which would unmount a dialog // nested inside it — so the row only nominates the connection to remove. @@ -327,11 +346,23 @@ function OwnerAccounts(props: { (candidate: AuthMethod) => candidate.kind === "oauth" && String(candidate.template) === String(connection.template), ); + // Route through the automatic probe/registration flow ONLY when the + // STORED binding is itself an auto-minted DCR client, or its row is + // known to be gone (nothing left to start directly against). A + // static/BYO or first-party binding takes the direct path below even on + // a discovery-capable integration — re-registering would silently + // rebind the connection to an automatic client. While the client list + // is still loading the binding is unknown, so the direct path wins + // (never rebind on a guess). + const stored = AsyncResult.isSuccess(allClients) + ? reconnectStoredClient(allClients.value, connection) + : undefined; if ( - method?.oauth?.supportsDynamicRegistration === true || - method?.oauth?.discoveryUrl != null + AsyncResult.isSuccess(allClients) && + hasDcr(method) && + reconnectAllowsAutomaticRegistration(stored) ) { - props.onDcrReconnect(connection); + props.onDcrReconnect(connection, stored); return; } const payload = oauthReconnectPayload(connection); @@ -632,7 +663,10 @@ export function AccountsSection(props: { showOwnerLabels={ownerDisplay.showOwnerLabels} methods={methods} onEdit={setEditingConnection} - onDcrReconnect={(connection: Connection) => { + onDcrReconnect={( + connection: Connection, + storedClient: OAuthClientSummary | undefined, + ) => { if (connection.oauthClient == null) return; setReconnectHandoff({ key: `reconnect:${connection.owner}:${String(connection.integration)}:${String( @@ -648,6 +682,17 @@ export function AccountsSection(props: { action: "reconnect", slug: String(connection.oauthClient), owner: connection.oauthClientOwner ?? connection.owner, + // Vetted by the routing in `handleReconnect`: the stored + // binding is auto-minted DCR (or its row is gone), so the + // modal may re-run the automatic flow. + dynamicRegistration: true, + // The stored client's RFC 8707 resource — an EXPLICIT null + // for a client registered WITHOUT a resource indicator, so + // reuse matches the stored row and a re-registration + // preserves the absence. Omitted when the row is gone. + ...(storedClient !== undefined + ? { resource: storedClient.resource ?? null } + : {}), }, }); }} diff --git a/packages/react/src/components/add-account-modal.test.ts b/packages/react/src/components/add-account-modal.test.ts index 77114d521..822bc7f6a 100644 --- a/packages/react/src/components/add-account-modal.test.ts +++ b/packages/react/src/components/add-account-modal.test.ts @@ -86,13 +86,16 @@ type AutomaticOAuthDeps = Parameters[0]; type AutomaticOAuthInput = Parameters[1]; /** DCR-focused tests use defaults for the CIMD branch they intentionally do - * not exercise. Discovery-level tests call the orchestrator directly. */ + * not exercise (and an always-mounted surface unless a test closes it). + * Discovery-level tests call the orchestrator directly. */ const runDcrConnect = ( - deps: Omit, + deps: Omit & + Partial>, input: Omit, ) => runAutomaticOAuthConnect( { + isActive: (): boolean => true, ...deps, createCimdClient: (): Promise => Promise.resolve(null), }, @@ -452,6 +455,7 @@ describe("runAutomaticOAuthConnect", () => { const outcome = await runAutomaticOAuthConnect( { ...popup, + isActive: (): boolean => true, probe: (): Promise => { calls.push("probe"); return Promise.resolve({ @@ -615,6 +619,186 @@ describe("runAutomaticOAuthConnect (reconnect)", () => { expect(outcome).toMatchObject({ kind: "fallback", reason: "no-registration-endpoint" }); expect(popup.calls).toEqual(["reserve", "release"]); }); + + // #1822: a stored client registered WITHOUT a resource indicator (explicit + // null) must stay resource-less through a reconnect — the probe's advertised + // resource (or the discovery-URL fallback) must not resurrect the parameter + // some servers reject. + it("preserves the stored client's explicit resource absence through re-registration", async () => { + let registerArgs: RegisterArgs | null = null; + const outcome = await runDcrConnect( + { + ...popupSpy(), + probe: (): Promise => + Promise.resolve({ + authorizationUrl: "https://auth.example.com/authorize", + tokenUrl: "https://auth.example.com/token", + registrationEndpoint: "https://auth.example.com/register", + resource: "https://mcp.example.com/mcp", + }), + register: (args: RegisterArgs): Promise => { + registerArgs = args; + return Promise.resolve(OAuthClientSlug.make("reconnected-app")); + }, + start: (): void => {}, + }, + { + discoveryUrl: "https://mcp.example.com/mcp", + resourceFallback: "https://mcp.example.com/mcp", + redirectUri: "http://localhost:4788/api/oauth/callback", + owner: "user" as Owner, + integration: TEST_INTEGRATION, + storedResource: null, + }, + ); + + expect(outcome).toEqual({ kind: "started", flow: "dcr" }); + expect(registerArgs!.resource).toBeNull(); + }); + + it("re-registers under the stored client's resource, not the probe's", async () => { + let registerArgs: RegisterArgs | null = null; + await runDcrConnect( + { + ...popupSpy(), + probe: (): Promise => + Promise.resolve({ + authorizationUrl: "https://auth.example.com/authorize", + tokenUrl: "https://auth.example.com/token", + registrationEndpoint: "https://auth.example.com/register", + resource: "https://probed.example.com/mcp", + }), + register: (args: RegisterArgs): Promise => { + registerArgs = args; + return Promise.resolve(OAuthClientSlug.make("reconnected-app")); + }, + start: (): void => {}, + }, + { + discoveryUrl: "https://mcp.example.com/mcp", + redirectUri: "http://localhost:4788/api/oauth/callback", + owner: "user" as Owner, + integration: TEST_INTEGRATION, + storedResource: "https://stored.example.com/mcp", + }, + ); + + expect(registerArgs!.resource).toBe("https://stored.example.com/mcp"); + }); +}); + +describe("runAutomaticOAuthConnect (modal closed mid-flight)", () => { + const probeOk = (): Promise => + Promise.resolve({ + authorizationUrl: "https://auth.example.com/authorize", + tokenUrl: "https://auth.example.com/token", + registrationEndpoint: "https://auth.example.com/register", + }); + const input = { + discoveryUrl: "https://mcp.example.com/mcp", + redirectUri: "http://localhost:4788/api/oauth/callback", + owner: "user" as Owner, + integration: TEST_INTEGRATION, + }; + + // Closing the modal unmounts the view, but the awaited sequence keeps + // running. It must stop at the next checkpoint: nothing registered, nothing + // launched, the claimed window given back. + it("stops after the probe: no client is registered and no popup launches", async () => { + const popup = popupSpy(); + let active = true; + let registered = 0; + const outcome = await runDcrConnect( + { + ...popup, + isActive: (): boolean => active, + probe: (): Promise => { + // The modal closes while the probe is in flight. + active = false; + return probeOk(); + }, + register: (): Promise => { + registered += 1; + return Promise.resolve(OAuthClientSlug.make("mcp-app")); + }, + start: (): void => { + popup.calls.push("start"); + }, + }, + input, + ); + + expect(outcome).toEqual({ kind: "aborted" }); + expect(registered).toBe(0); + expect(popup.calls).toEqual(["reserve", "release"]); + }); + + // A close racing the registration itself cannot unmint the client (no + // server-side cancel exists; the row is inert, reusable DCR plumbing), but + // the sign-in popup must never launch after the modal is gone. + it("never launches the popup when the close raced the registration", async () => { + const popup = popupSpy(); + let active = true; + const outcome = await runDcrConnect( + { + ...popup, + isActive: (): boolean => active, + probe: probeOk, + register: (): Promise => { + // The modal closes while the registration is in flight. + active = false; + return Promise.resolve(OAuthClientSlug.make("mcp-app")); + }, + start: (): void => { + popup.calls.push("start"); + }, + }, + input, + ); + + expect(outcome).toEqual({ kind: "aborted" }); + expect(popup.calls).toEqual(["reserve", "release"]); + }); + + it("never starts a CIMD flow after the close", async () => { + const popup = popupSpy(); + let active = true; + const outcome = await runAutomaticOAuthConnect( + { + ...popup, + isActive: (): boolean => active, + probe: (): Promise => + Promise.resolve({ + authorizationUrl: "https://auth.example.com/authorize", + tokenUrl: "https://auth.example.com/token", + clientIdMetadataDocumentSupported: true, + }), + createCimdClient: (args: CimdCreateArgs): Promise => { + // The modal closes while the local CIMD client is being minted. + active = false; + return Promise.resolve(args.slug); + }, + register: (): Promise => + Promise.resolve(OAuthClientSlug.make("unexpected")), + start: (): void => { + popup.calls.push("start"); + }, + }, + { + discoveryUrl: "https://mcp.example.com/mcp", + owner: "user" as Owner, + integration: TEST_INTEGRATION, + cimd: { + integrationName: "Test MCP", + clientIdMetadataDocumentUrl: "https://executor.example/api/oauth/client-id-metadata.json", + existingClients: [], + }, + }, + ); + + expect(outcome).toEqual({ kind: "aborted" }); + expect(popup.calls).toEqual(["reserve", "release"]); + }); }); describe("runDcrConnect popup reservation", () => { diff --git a/packages/react/src/components/add-account-modal.tsx b/packages/react/src/components/add-account-modal.tsx index 61943e661..21ad814d4 100644 --- a/packages/react/src/components/add-account-modal.tsx +++ b/packages/react/src/components/add-account-modal.tsx @@ -856,6 +856,10 @@ type DcrStartArgs = { type AutomaticOAuthOutcome = | { readonly kind: "started"; readonly flow: "cimd" | "dcr" } | { readonly kind: "popup-blocked" } + /** The owning surface went away mid-flight (`isActive` turned false): the + * sequence stopped before its next side effect and released the window. + * Nothing to report — the modal that would show it is gone. */ + | { readonly kind: "aborted" } | { readonly kind: "fallback"; readonly reason: "probe-failed" } | { readonly kind: "fallback"; @@ -888,6 +892,10 @@ type RunAutomaticOAuthConnectDeps = { readonly reserve: () => OAuthPopupReservation; /** Close a claimed window this flow turned out not to need. */ readonly release: () => void; + /** Whether the surface that started this connect still exists. Checked + * after every awaited round trip: closing the modal mid-flight must not + * register a client or launch the popup afterwards. */ + readonly isActive: () => boolean; }; type RunAutomaticOAuthConnectInput = { @@ -912,6 +920,14 @@ type RunAutomaticOAuthConnectInput = { RunCimdConnectInput, "integrationName" | "clientIdMetadataDocumentUrl" | "existingClients" >; + /** A reconnect carries the STORED client's RFC 8707 resource — including an + * EXPLICIT null for a client registered WITHOUT a resource indicator + * (#1822: some servers reject any `resource` parameter). When present it + * overrides the probe-derived value, so client reuse matches the stored + * row and a genuinely needed re-registration preserves the absence. + * Undefined (a fresh connect, or the stored row is gone) keeps the probed + * behavior. */ + readonly storedResource?: string | null; }; /** RFC 7591 `client_name` sent for every dynamic registration. Deliberately @@ -933,6 +949,8 @@ const DCR_CLIENT_NAME = "Executor"; * - Register rejected with a message → `{ kind: "fallback", reason: "registration-failed", probe, message }` * so the caller can show why (e.g. a redirect-URI rejection) over the generic copy. * - Register failed without detail (null) → `{ kind: "fallback", reason: "registration-failed", probe }`. + * - Surface gone after any await (`isActive` false) → `{ kind: "aborted" }`, + * window released, popup never launched. * - Success → calls `start` and reports which automatic flow was used. */ export async function runAutomaticOAuthConnect( @@ -947,10 +965,24 @@ export async function runAutomaticOAuthConnect( if (reservation.kind === "blocked") return { kind: "popup-blocked" }; const probe = await deps.probe(input.discoveryUrl); + // The modal may have closed while the probe was in flight. Stop BEFORE the + // next side effect (registration persists a client; start launches the + // popup) and give the claimed window back. + if (!deps.isActive()) { + deps.release(); + return { kind: "aborted" }; + } if (probe === null) { deps.release(); return { kind: "fallback", reason: "probe-failed" }; } + // The flow's RFC 8707 resource indicator: a reconnect's stored value wins + // (see `storedResource`), otherwise the probe's advertised resource with the + // discovery-URL fallback. + const resource = + input.storedResource !== undefined + ? input.storedResource + : (probe.resource ?? input.resourceFallback ?? null); if (probe.clientIdMetadataDocumentSupported === true) { const resolved = await resolveCimdClient( { createClient: deps.createCimdClient }, @@ -959,7 +991,7 @@ export async function runAutomaticOAuthConnect( integrationName: input.cimd.integrationName, authorizationUrl: probe.authorizationUrl, tokenUrl: probe.tokenUrl, - resource: probe.resource ?? input.resourceFallback ?? null, + resource, clientIdMetadataDocumentUrl: input.cimd.clientIdMetadataDocumentUrl, existingClients: input.cimd.existingClients, }, @@ -968,6 +1000,10 @@ export async function runAutomaticOAuthConnect( deps.release(); return { kind: "fallback", reason: "client-metadata-failed", probe }; } + if (!deps.isActive()) { + deps.release(); + return { kind: "aborted" }; + } deps.start({ client: resolved.client, owner: input.owner, reservation }); return { kind: "started", flow: "cimd" }; } @@ -986,7 +1022,7 @@ export async function runAutomaticOAuthConnect( registrationEndpoint, authorizationUrl: probe.authorizationUrl, tokenUrl: probe.tokenUrl, - resource: probe.resource ?? input.resourceFallback ?? null, + resource, scopes, tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported, clientName: DCR_CLIENT_NAME, @@ -1003,6 +1039,13 @@ export async function runAutomaticOAuthConnect( deps.release(); return { kind: "fallback", reason: "registration-failed", probe, message: minted.error }; } + // A close that raced the registration itself cannot unmint the client (there + // is no server-side cancel) — the row is inert, reusable DCR plumbing — but + // the sign-in popup must never launch after the modal is gone. + if (!deps.isActive()) { + deps.release(); + return { kind: "aborted" }; + } deps.start({ client: minted, owner: input.owner, reservation }); return { kind: "started", flow: "dcr" }; } @@ -1012,8 +1055,11 @@ export async function runAutomaticOAuthConnect( * * True when the integration advertises dynamic registration (MCP oauth2) OR * carries a discovery URL we can probe at connect time — the probe decides - * between CIMD and DCR from there. Shared by the connect button and the - * reconnect handoff so both take the same route for the same method. + * between CIMD and DCR from there. This is a METHOD capability only: a + * reconnect additionally requires the STORED client binding to be an + * auto-minted DCR one (`reconnectAllowsAutomaticRegistration`) before taking + * the automatic route, so a static/BYO or first-party binding is never + * silently rebound. */ export const hasDcr = (method: AuthMethod | undefined | null): boolean => method?.kind === "oauth" && @@ -1031,6 +1077,10 @@ type AutomaticOAuthConnectRequest = { /** A reconnect re-authorizes a connection that already exists; a connect * creates one. The only behavioral difference between the two paths. */ readonly mode: "connect" | "reconnect"; + /** Reconnect only: the stored client's RFC 8707 resource, an EXPLICIT null + * when it was registered WITHOUT one. See + * `RunAutomaticOAuthConnectInput.storedResource`. */ + readonly storedResource?: string | null; }; // --------------------------------------------------------------------------- @@ -2302,6 +2352,19 @@ function AddAccountModalView(props: AddAccountModalProps) { } }; + // Whether this view is still mounted. Closing the modal unmounts it (see + // `AddAccountModal`), and the automatic connect sequence below polls this + // between its awaited round trips so a close mid-flight aborts instead of + // registering a client / launching the popup into a dead surface. The ref is + // re-armed in the effect body so a StrictMode remount stays active. + const viewMountedRef = useRef(true); + useEffect(() => { + viewMountedRef.current = true; + return () => { + viewMountedRef.current = false; + }; + }, []); + // Automatic discovered OAuth: probe once, then prefer CIMD or use DCR // according to the authorization server's advertised metadata. On failure we // flip `dcrFailed` so the bring-your-own-app picker remains the recovery path. @@ -2326,6 +2389,11 @@ function AddAccountModalView(props: AddAccountModalProps) { { reserve: oauthPopup.reserve, release: oauthPopup.releaseReservation, + // Closing the modal genuinely unmounts this view (see + // `AddAccountModal`), so "still mounted" is exactly "still open". + // The sequence checks it between round trips: a close mid-flight + // must not register a client or launch the popup afterwards. + isActive: () => viewMountedRef.current, probe: async (url: string): Promise => { const exit = await doProbe({ payload: { url }, reactivityKeys: [] }); if (Exit.isFailure(exit)) return null; @@ -2426,9 +2494,15 @@ function AddAccountModalView(props: AddAccountModalProps) { clientIdMetadataDocumentUrl: oauthClientIdMetadataDocumentUrl(), existingClients: clientSummaries, }, + ...(request.storedResource !== undefined + ? { storedResource: request.storedResource } + : {}), }, ); setDcrBusy(false); + // The modal closed mid-flight: this view is unmounted, so there is + // nothing to report and no fallback to show. + if (outcome.kind === "aborted") return; // `connection_oauth_started` measures the connect funnel; a reconnect // reports through `connection_reconnected` on the popup callbacks above, // so it must not also land here. @@ -2491,9 +2565,12 @@ function AddAccountModalView(props: AddAccountModalProps) { // OAuth flow immediately. Fires once per handoff key (tracked by ref), which // is also what makes a re-render mid-flight harmless. // - // A DCR-capable method re-runs the automatic path rather than reusing the - // stored client — see `startAutomaticOAuthConnect`. Everything else has a - // fixed, registered app, so it starts the popup against that client directly. + // Routing follows the STORED binding, not just the method: only a handoff + // whose stored client is auto-minted DCR (vetted by the accounts section, + // `dynamicRegistration: true`) re-runs the automatic path — see + // `startAutomaticOAuthConnect`. Everything else (static/BYO, first-party) + // has a fixed, registered app, so it starts the popup against that client + // directly and is never rebound to an automatic one. useEffect(() => { const handoff = initialState; const oauthClient = handoff?.oauthClient; @@ -2515,7 +2592,7 @@ function AddAccountModalView(props: AddAccountModalProps) { oauthReconnectOpenedKey.current = handoff.key; setMethodId(oauthMethod.id); - if (hasDcr(oauthMethod)) { + if (hasDcr(oauthMethod) && oauthClient.dynamicRegistration === true) { void startAutomaticOAuthConnect({ method: oauthMethod, owner: connectionOwner, @@ -2523,6 +2600,7 @@ function AddAccountModalView(props: AddAccountModalProps) { identityLabel: handoff.identityLabel, typedLabel: connectionName, mode: "reconnect", + ...(oauthClient.resource !== undefined ? { storedResource: oauthClient.resource } : {}), }); return; } diff --git a/packages/react/src/plugins/oauth-reconnect.test.ts b/packages/react/src/plugins/oauth-reconnect.test.ts index d57812d91..461a6ab85 100644 --- a/packages/react/src/plugins/oauth-reconnect.test.ts +++ b/packages/react/src/plugins/oauth-reconnect.test.ts @@ -7,12 +7,15 @@ import { OAuthClientSlug, ProviderKey, type Connection, + type OAuthClientSummary, } from "@executor-js/sdk/shared"; import { missingScopes, oauthReconnectPayload, + reconnectAllowsAutomaticRegistration, reconnectMode, + reconnectStoredClient, reconsentRequiredScopes, } from "./oauth-reconnect"; @@ -70,6 +73,82 @@ describe("oauthReconnectPayload (re-mint the SAME connection)", () => { }); }); +const clientSummary = (overrides: Partial = {}): OAuthClientSummary => ({ + owner: "user", + slug: OAuthClientSlug.make("github-app"), + grant: "authorization_code", + authorizationUrl: "https://auth.example.com/authorize", + tokenUrl: "https://auth.example.com/token", + resource: null, + clientId: "client-123", + origin: { kind: "manual", integration: null }, + ...overrides, +}); + +describe("reconnectStoredClient (resolve a connection's stored app)", () => { + it("finds the stored row by slug and the app's stored owner", () => { + const stored = clientSummary(); + expect(reconnectStoredClient([clientSummary({ owner: "org" }), stored], connection())).toBe( + stored, + ); + }); + + it("matches against oauthClientOwner when the app is shared (org app, user connection)", () => { + const shared = clientSummary({ owner: "org" }); + expect(reconnectStoredClient([shared], connection({ oauthClientOwner: "org" }))).toBe(shared); + // Without the stored app owner, the connection's own owner is the key. + expect(reconnectStoredClient([shared], connection())).toBeUndefined(); + }); + + it("matches a first-party app on slug alone (config-declared, deployment-scoped)", () => { + const firstParty = clientSummary({ + owner: "org", + slug: OAuthClientSlug.make("first-party:github"), + origin: { kind: "first_party" }, + }); + expect( + reconnectStoredClient( + [firstParty], + connection({ oauthClient: OAuthClientSlug.make("first-party:github") }), + ), + ).toBe(firstParty); + }); + + it("is undefined for a non-OAuth connection and for a binding whose row is gone", () => { + expect( + reconnectStoredClient([clientSummary()], connection({ oauthClient: null })), + ).toBeUndefined(); + expect(reconnectStoredClient([], connection())).toBeUndefined(); + }); +}); + +describe("reconnectAllowsAutomaticRegistration (which bindings may re-register)", () => { + // Only an auto-minted DCR binding may re-run probe/registration; a manual + // (static/BYO) or first-party binding must keep the direct stored-client + // path — re-registering would silently rebind the connection. + it("allows an auto-minted DCR binding", () => { + expect( + reconnectAllowsAutomaticRegistration( + clientSummary({ origin: { kind: "dynamic_client_registration", integration: null } }), + ), + ).toBe(true); + }); + + it("allows a binding whose stored row is gone (nothing to start directly against)", () => { + expect(reconnectAllowsAutomaticRegistration(undefined)).toBe(true); + }); + + it("keeps a manual (static/BYO) binding on the direct path", () => { + expect(reconnectAllowsAutomaticRegistration(clientSummary())).toBe(false); + }); + + it("keeps a first-party binding on the direct path", () => { + expect( + reconnectAllowsAutomaticRegistration(clientSummary({ origin: { kind: "first_party" } })), + ).toBe(false); + }); +}); + describe("missingScopes (Part 2 informational subset warning)", () => { // The app's scopes are a STRICT subset of the integration's → list what's // missing, in the integration's declared order. diff --git a/packages/react/src/plugins/oauth-reconnect.ts b/packages/react/src/plugins/oauth-reconnect.ts index 46bb067a1..19823fcd1 100644 --- a/packages/react/src/plugins/oauth-reconnect.ts +++ b/packages/react/src/plugins/oauth-reconnect.ts @@ -1,4 +1,4 @@ -import type { Connection } from "@executor-js/sdk/shared"; +import type { Connection, OAuthClientSummary } from "@executor-js/sdk/shared"; import type { OAuthStartPayload } from "./oauth-sign-in"; @@ -43,6 +43,39 @@ export function oauthReconnectPayload(connection: Connection): OAuthStartPayload }; } +/** The stored OAuth app backing a connection, resolved from the loaded client + * summaries. First-party apps are config-declared and deployment-scoped, so + * they match on slug alone; stored rows are owner-scoped, matched against the + * app's stored owner (a Personal connection may be backed by a shared + * Workspace app). Undefined when the connection is not OAuth or its client + * row is gone (e.g. removed by hand). */ +export function reconnectStoredClient( + clients: readonly OAuthClientSummary[], + connection: Connection, +): OAuthClientSummary | undefined { + if (connection.oauthClient == null) return undefined; + const slug = String(connection.oauthClient); + const owner = connection.oauthClientOwner ?? connection.owner; + return clients.find( + (client) => + String(client.slug) === slug && + (client.origin.kind === "first_party" || client.owner === owner), + ); +} + +/** Whether Reconnect may take the automatic probe/CIMD/DCR route for this + * stored binding. Only an auto-minted DCR client may be re-registered (its + * whole lifecycle is automatic), and a binding whose row is GONE has nothing + * to start directly against, so re-registration is its only recovery. A + * manual (static/BYO) or first-party binding must keep the direct + * stored-client path — routing it through registration would silently rebind + * the connection to an automatic client. */ +export function reconnectAllowsAutomaticRegistration( + stored: OAuthClientSummary | undefined, +): boolean { + return stored === undefined || stored.origin.kind === "dynamic_client_registration"; +} + // --------------------------------------------------------------------------- // Subset-scope warning (Part 2). At connect, when the chosen OAuth app's // DECLARED scopes are a STRICT subset of the integration's declared scopes, the From 18574d5eebf75fc64b563c766adf605d26b8cc7c Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Sat, 29 Aug 2026 00:53:41 -0700 Subject: [PATCH 5/8] Route reconnect by binding origin, reconcile stored resource with probe, abort before failure fallbacks --- .../react/src/components/accounts-section.tsx | 55 +++++---- .../src/components/add-account-modal.test.ts | 108 +++++++++++++++++- .../src/components/add-account-modal.tsx | 76 +++++++----- .../react/src/plugins/oauth-reconnect.test.ts | 58 +++++++--- packages/react/src/plugins/oauth-reconnect.ts | 52 +++++++-- 5 files changed, 272 insertions(+), 77 deletions(-) diff --git a/packages/react/src/components/accounts-section.tsx b/packages/react/src/components/accounts-section.tsx index 26ec69912..41d5a3d2a 100644 --- a/packages/react/src/components/accounts-section.tsx +++ b/packages/react/src/components/accounts-section.tsx @@ -29,9 +29,8 @@ import type { AuthMethod } from "../lib/auth-placements"; import { connectionNeedsReconsent, oauthReconnectPayload, - reconnectAllowsAutomaticRegistration, reconnectMode, - reconnectStoredClient, + reconnectRoute, reconsentRequiredScopes, } from "../plugins/oauth-reconnect"; import { useOAuthPopupFlow } from "../plugins/oauth-sign-in"; @@ -125,6 +124,10 @@ function AccountRow(props: { readonly showOwnerLabel: boolean; readonly onEdit: () => void; readonly onReconnect: () => void; + /** Reconnect routing needs the stored client binding; while the client + * summaries are still loading the route is unknown, so the action is + * disabled rather than guessed (same idiom as "Check now" above). */ + readonly reconnectDisabled: boolean; readonly onRemove: () => void; }) { const { connection, needsReconsent } = props; @@ -278,7 +281,11 @@ function AccountRow(props: { Edit - + Reconnect @@ -346,23 +353,23 @@ function OwnerAccounts(props: { (candidate: AuthMethod) => candidate.kind === "oauth" && String(candidate.template) === String(connection.template), ); - // Route through the automatic probe/registration flow ONLY when the - // STORED binding is itself an auto-minted DCR client, or its row is - // known to be gone (nothing left to start directly against). A + // Route by the STORED binding's origin (`reconnectRoute`): an + // auto-minted DCR binding re-runs the automatic probe/registration flow + // (direct reuse dead-ends once the callback origin drifts, #1542); a // static/BYO or first-party binding takes the direct path below even on // a discovery-capable integration — re-registering would silently // rebind the connection to an automatic client. While the client list - // is still loading the binding is unknown, so the direct path wins - // (never rebind on a guess). - const stored = AsyncResult.isSuccess(allClients) - ? reconnectStoredClient(allClients.value, connection) - : undefined; - if ( - AsyncResult.isSuccess(allClients) && - hasDcr(method) && - reconnectAllowsAutomaticRegistration(stored) - ) { - props.onDcrReconnect(connection, stored); + // is still loading the binding is UNKNOWN and no route may be chosen: + // the menu item is disabled until then, and this guard backstops a + // race — a permanent wrong choice on a guess is never acceptable. + const route = reconnectRoute( + AsyncResult.isSuccess(allClients) ? allClients.value : undefined, + connection, + hasDcr(method), + ); + if (route.kind === "unknown") return; + if (route.kind === "automatic") { + props.onDcrReconnect(connection, route.stored); return; } const payload = oauthReconnectPayload(connection); @@ -482,6 +489,12 @@ function OwnerAccounts(props: { showOwnerLabel={props.showOwnerLabels} onEdit={() => props.onEdit(connection)} onReconnect={() => void handleReconnect(connection)} + // An OAuth Reconnect routes by the stored client binding; until + // the summaries load the route is unknown, so the action waits. + // Static-credential rows refresh without the binding. + reconnectDisabled={ + reconnectMode(connection) === "oauth" && !AsyncResult.isSuccess(allClients) + } onRemove={() => setRemovingConnection(connection)} /> ))} @@ -687,9 +700,11 @@ export function AccountsSection(props: { // modal may re-run the automatic flow. dynamicRegistration: true, // The stored client's RFC 8707 resource — an EXPLICIT null - // for a client registered WITHOUT a resource indicator, so - // reuse matches the stored row and a re-registration - // preserves the absence. Omitted when the row is gone. + // for a client registered WITHOUT a resource indicator + // (that absence always survives re-registration); a stored + // value is reconciled against the probe downstream, so a + // migrated resource follows the server. Omitted when the + // row is gone. ...(storedClient !== undefined ? { resource: storedClient.resource ?? null } : {}), diff --git a/packages/react/src/components/add-account-modal.test.ts b/packages/react/src/components/add-account-modal.test.ts index 822bc7f6a..0fdedf498 100644 --- a/packages/react/src/components/add-account-modal.test.ts +++ b/packages/react/src/components/add-account-modal.test.ts @@ -656,7 +656,10 @@ describe("runAutomaticOAuthConnect (reconnect)", () => { expect(registerArgs!.resource).toBeNull(); }); - it("re-registers under the stored client's resource, not the probe's", async () => { + // The server MIGRATED its protected resource (R1 → R2) since the client was + // stored: the probe's freshly advertised value is the truth, so the stored + // one must not pin the reconnect to the old resource forever. + it("follows a migrated resource: the probe's advertised value beats the stored one", async () => { let registerArgs: RegisterArgs | null = null; await runDcrConnect( { @@ -683,6 +686,38 @@ describe("runAutomaticOAuthConnect (reconnect)", () => { }, ); + expect(registerArgs!.resource).toBe("https://probed.example.com/mcp"); + }); + + // When the probe advertises NO resource, the stored one stands — including + // over the discovery-URL fallback, which is our own guess, not the server's. + it("keeps the stored resource when the probe advertises none", async () => { + let registerArgs: RegisterArgs | null = null; + await runDcrConnect( + { + ...popupSpy(), + probe: (): Promise => + Promise.resolve({ + authorizationUrl: "https://auth.example.com/authorize", + tokenUrl: "https://auth.example.com/token", + registrationEndpoint: "https://auth.example.com/register", + }), + register: (args: RegisterArgs): Promise => { + registerArgs = args; + return Promise.resolve(OAuthClientSlug.make("reconnected-app")); + }, + start: (): void => {}, + }, + { + discoveryUrl: "https://mcp.example.com/mcp", + resourceFallback: "https://mcp.example.com/mcp", + redirectUri: "http://localhost:4788/api/oauth/callback", + owner: "user" as Owner, + integration: TEST_INTEGRATION, + storedResource: "https://stored.example.com/mcp", + }, + ); + expect(registerArgs!.resource).toBe("https://stored.example.com/mcp"); }); }); @@ -799,6 +834,77 @@ describe("runAutomaticOAuthConnect (modal closed mid-flight)", () => { expect(outcome).toEqual({ kind: "aborted" }); expect(popup.calls).toEqual(["reserve", "release"]); }); + + // A close racing a FAILED registration must still abort: a "fallback" + // outcome would make the caller write recovery state into a modal that no + // longer exists. + it("aborts (not fallback) when the close races a failed registration", async () => { + const popup = popupSpy(); + let active = true; + const outcome = await runDcrConnect( + { + ...popup, + isActive: (): boolean => active, + probe: probeOk, + register: (): Promise => { + // The modal closes while the registration is in flight, AND the + // server rejects it. + active = false; + return Promise.resolve(null); + }, + start: (): void => { + popup.calls.push("start"); + }, + }, + input, + ); + + expect(outcome).toEqual({ kind: "aborted" }); + expect(popup.calls).toEqual(["reserve", "release"]); + }); + + // Same for the CIMD branch: a failed mint racing the close is an abort, not + // a client-metadata-failed fallback for the unmounted modal to render. + it("aborts (not fallback) when the close races a failed CIMD mint", async () => { + const popup = popupSpy(); + let active = true; + const outcome = await runAutomaticOAuthConnect( + { + ...popup, + isActive: (): boolean => active, + probe: (): Promise => + Promise.resolve({ + authorizationUrl: "https://auth.example.com/authorize", + tokenUrl: "https://auth.example.com/token", + clientIdMetadataDocumentSupported: true, + }), + createCimdClient: (): Promise => { + // The modal closes while the local CIMD client is being minted, AND + // the mint fails. + active = false; + return Promise.resolve(null); + }, + register: (): Promise => + Promise.resolve(OAuthClientSlug.make("unexpected")), + start: (): void => { + popup.calls.push("start"); + }, + }, + { + discoveryUrl: "https://mcp.example.com/mcp", + owner: "user" as Owner, + integration: TEST_INTEGRATION, + cimd: { + integrationName: "Test MCP", + clientIdMetadataDocumentUrl: "https://executor.example/api/oauth/client-id-metadata.json", + existingClients: [], + }, + }, + ); + + expect(outcome).toEqual({ kind: "aborted" }); + expect(popup.calls).toEqual(["reserve", "release"]); + }); }); describe("runDcrConnect popup reservation", () => { diff --git a/packages/react/src/components/add-account-modal.tsx b/packages/react/src/components/add-account-modal.tsx index 21ad814d4..7bc26aa7c 100644 --- a/packages/react/src/components/add-account-modal.tsx +++ b/packages/react/src/components/add-account-modal.tsx @@ -922,11 +922,12 @@ type RunAutomaticOAuthConnectInput = { >; /** A reconnect carries the STORED client's RFC 8707 resource — including an * EXPLICIT null for a client registered WITHOUT a resource indicator - * (#1822: some servers reject any `resource` parameter). When present it - * overrides the probe-derived value, so client reuse matches the stored - * row and a genuinely needed re-registration preserves the absence. - * Undefined (a fresh connect, or the stored row is gone) keeps the probed - * behavior. */ + * (#1822: some servers reject any `resource` parameter). The flow + * RECONCILES it with the probe: an explicit null always wins (the + * deliberate absence is preserved), a probed resource beats a stored one + * (the server migrated), and the stored one stands when the probe + * advertises none. Undefined (a fresh connect, or the stored row is gone) + * keeps the probed behavior. */ readonly storedResource?: string | null; }; @@ -950,7 +951,10 @@ const DCR_CLIENT_NAME = "Executor"; * so the caller can show why (e.g. a redirect-URI rejection) over the generic copy. * - Register failed without detail (null) → `{ kind: "fallback", reason: "registration-failed", probe }`. * - Surface gone after any await (`isActive` false) → `{ kind: "aborted" }`, - * window released, popup never launched. + * window released, popup never launched. Checked BEFORE inspecting that + * await's result, so aborted wins even when the round trip failed — a + * failure outcome would have the caller write fallback state for a modal + * that no longer exists. * - Success → calls `start` and reports which automatic flow was used. */ export async function runAutomaticOAuthConnect( @@ -976,12 +980,19 @@ export async function runAutomaticOAuthConnect( deps.release(); return { kind: "fallback", reason: "probe-failed" }; } - // The flow's RFC 8707 resource indicator: a reconnect's stored value wins - // (see `storedResource`), otherwise the probe's advertised resource with the + // The flow's RFC 8707 resource indicator. A reconnect RECONCILES the stored + // value with the probe rather than pinning it: a stored EXPLICIT null always + // wins (#1822 — the deliberate absence some servers require), a probed + // resource beats a stored one (the server migrated its protected resource, + // and the probe is the fresh truth), and a stored resource stands when the + // probe advertises none — including over the discovery-URL fallback, which + // is our own guess. A fresh connect keeps the probed value with the // discovery-URL fallback. const resource = input.storedResource !== undefined - ? input.storedResource + ? input.storedResource === null + ? null + : (probe.resource ?? input.storedResource) : (probe.resource ?? input.resourceFallback ?? null); if (probe.clientIdMetadataDocumentSupported === true) { const resolved = await resolveCimdClient( @@ -996,14 +1007,16 @@ export async function runAutomaticOAuthConnect( existingClients: input.cimd.existingClients, }, ); - if (resolved.kind === "failed") { - deps.release(); - return { kind: "fallback", reason: "client-metadata-failed", probe }; - } + // Aborted wins over failure: a "fallback" outcome makes the caller write + // recovery state, and the modal that would render it is gone. if (!deps.isActive()) { deps.release(); return { kind: "aborted" }; } + if (resolved.kind === "failed") { + deps.release(); + return { kind: "fallback", reason: "client-metadata-failed", probe }; + } deps.start({ client: resolved.client, owner: input.owner, reservation }); return { kind: "started", flow: "cimd" }; } @@ -1029,6 +1042,14 @@ export async function runAutomaticOAuthConnect( redirectUri: input.redirectUri, originIntegration: input.integration, }); + // A close that raced the registration itself cannot unmint the client (there + // is no server-side cancel) — the row is inert, reusable DCR plumbing — but + // nothing may land on the closed surface afterwards: not the sign-in popup, + // and not a failure fallback either, so aborted wins over the mint result. + if (!deps.isActive()) { + deps.release(); + return { kind: "aborted" }; + } if (minted === null) { deps.release(); return { kind: "fallback", reason: "registration-failed", probe }; @@ -1039,13 +1060,6 @@ export async function runAutomaticOAuthConnect( deps.release(); return { kind: "fallback", reason: "registration-failed", probe, message: minted.error }; } - // A close that raced the registration itself cannot unmint the client (there - // is no server-side cancel) — the row is inert, reusable DCR plumbing — but - // the sign-in popup must never launch after the modal is gone. - if (!deps.isActive()) { - deps.release(); - return { kind: "aborted" }; - } deps.start({ client: minted, owner: input.owner, reservation }); return { kind: "started", flow: "dcr" }; } @@ -1056,10 +1070,10 @@ export async function runAutomaticOAuthConnect( * True when the integration advertises dynamic registration (MCP oauth2) OR * carries a discovery URL we can probe at connect time — the probe decides * between CIMD and DCR from there. This is a METHOD capability only: a - * reconnect additionally requires the STORED client binding to be an - * auto-minted DCR one (`reconnectAllowsAutomaticRegistration`) before taking - * the automatic route, so a static/BYO or first-party binding is never - * silently rebound. + * reconnect routes by the STORED client binding instead (`reconnectRoute`), + * so a static/BYO or first-party binding is never silently rebound, and an + * auto-minted DCR binding re-registers even when the method declares no + * capability (the probe falls back to the token URL). */ export const hasDcr = (method: AuthMethod | undefined | null): boolean => method?.kind === "oauth" && @@ -2499,10 +2513,11 @@ function AddAccountModalView(props: AddAccountModalProps) { : {}), }, ); - setDcrBusy(false); - // The modal closed mid-flight: this view is unmounted, so there is - // nothing to report and no fallback to show. + // The modal closed mid-flight: this view is unmounted, so no state may + // be written at all — not the fallback below, and not even the busy + // flag, which belongs to the surface that is gone. if (outcome.kind === "aborted") return; + setDcrBusy(false); // `connection_oauth_started` measures the connect funnel; a reconnect // reports through `connection_reconnected` on the popup callbacks above, // so it must not also land here. @@ -2592,7 +2607,12 @@ function AddAccountModalView(props: AddAccountModalProps) { oauthReconnectOpenedKey.current = handoff.key; setMethodId(oauthMethod.id); - if (hasDcr(oauthMethod) && oauthClient.dynamicRegistration === true) { + // The accounts section vetted the STORED binding as auto-minted DCR + // (`dynamicRegistration: true`), and that alone routes: the automatic + // flow probes the method's token URL even when it declares no + // discovery/DCR capability, whereas the direct path below would dead-end + // an origin-drifted DCR client (#1542). + if (oauthClient.dynamicRegistration === true) { void startAutomaticOAuthConnect({ method: oauthMethod, owner: connectionOwner, diff --git a/packages/react/src/plugins/oauth-reconnect.test.ts b/packages/react/src/plugins/oauth-reconnect.test.ts index 461a6ab85..183a5f9eb 100644 --- a/packages/react/src/plugins/oauth-reconnect.test.ts +++ b/packages/react/src/plugins/oauth-reconnect.test.ts @@ -13,8 +13,8 @@ import { import { missingScopes, oauthReconnectPayload, - reconnectAllowsAutomaticRegistration, reconnectMode, + reconnectRoute, reconnectStoredClient, reconsentRequiredScopes, } from "./oauth-reconnect"; @@ -122,30 +122,54 @@ describe("reconnectStoredClient (resolve a connection's stored app)", () => { }); }); -describe("reconnectAllowsAutomaticRegistration (which bindings may re-register)", () => { - // Only an auto-minted DCR binding may re-run probe/registration; a manual - // (static/BYO) or first-party binding must keep the direct stored-client - // path — re-registering would silently rebind the connection. - it("allows an auto-minted DCR binding", () => { - expect( - reconnectAllowsAutomaticRegistration( - clientSummary({ origin: { kind: "dynamic_client_registration", integration: null } }), - ), - ).toBe(true); +describe("reconnectRoute (where an OAuth Reconnect goes)", () => { + const dcrBinding = clientSummary({ + origin: { kind: "dynamic_client_registration", integration: null }, }); - it("allows a binding whose stored row is gone (nothing to start directly against)", () => { - expect(reconnectAllowsAutomaticRegistration(undefined)).toBe(true); + // The summaries have not loaded → the stored binding is UNKNOWN, and no + // route may be chosen. The old routing chose "direct" here, permanently + // dead-ending an origin-drifted DCR client exactly as #1542 described. + it("waits (never chooses direct) while the client summaries are loading", () => { + expect(reconnectRoute(undefined, connection(), true)).toEqual({ kind: "unknown" }); + expect(reconnectRoute(undefined, connection(), false)).toEqual({ kind: "unknown" }); }); - it("keeps a manual (static/BYO) binding on the direct path", () => { - expect(reconnectAllowsAutomaticRegistration(clientSummary())).toBe(false); + // The BINDING's origin routes, not the method's capability flags: the + // automatic flow can probe the token URL even when the method declares no + // discovery/DCR support. + it("routes a DCR-origin binding to the automatic path regardless of method capability", () => { + expect(reconnectRoute([dcrBinding], connection(), false)).toEqual({ + kind: "automatic", + stored: dcrBinding, + }); + expect(reconnectRoute([dcrBinding], connection(), true)).toEqual({ + kind: "automatic", + stored: dcrBinding, + }); + }); + + // A manual (static/BYO) or first-party binding must keep the direct + // stored-client path — re-registering would silently rebind the connection. + it("keeps a manual (static/BYO) binding on the direct path even on a capable method", () => { + expect(reconnectRoute([clientSummary()], connection(), true)).toEqual({ kind: "direct" }); }); it("keeps a first-party binding on the direct path", () => { expect( - reconnectAllowsAutomaticRegistration(clientSummary({ origin: { kind: "first_party" } })), - ).toBe(false); + reconnectRoute([clientSummary({ origin: { kind: "first_party" } })], connection(), true), + ).toEqual({ kind: "direct" }); + }); + + // A binding whose row is GONE has nothing to start directly against, so + // re-registration is its only recovery — when the method supports the + // automatic flow at all. + it("re-registers a gone binding when the method supports the automatic flow", () => { + expect(reconnectRoute([], connection(), true)).toEqual({ + kind: "automatic", + stored: undefined, + }); + expect(reconnectRoute([], connection(), false)).toEqual({ kind: "direct" }); }); }); diff --git a/packages/react/src/plugins/oauth-reconnect.ts b/packages/react/src/plugins/oauth-reconnect.ts index 19823fcd1..230bed6fd 100644 --- a/packages/react/src/plugins/oauth-reconnect.ts +++ b/packages/react/src/plugins/oauth-reconnect.ts @@ -63,17 +63,47 @@ export function reconnectStoredClient( ); } -/** Whether Reconnect may take the automatic probe/CIMD/DCR route for this - * stored binding. Only an auto-minted DCR client may be re-registered (its - * whole lifecycle is automatic), and a binding whose row is GONE has nothing - * to start directly against, so re-registration is its only recovery. A - * manual (static/BYO) or first-party binding must keep the direct - * stored-client path — routing it through registration would silently rebind - * the connection to an automatic client. */ -export function reconnectAllowsAutomaticRegistration( - stored: OAuthClientSummary | undefined, -): boolean { - return stored === undefined || stored.origin.kind === "dynamic_client_registration"; +/** Where an OAuth connection's Reconnect goes. */ +export type ReconnectRoute = + /** The client summaries have not loaded, so the stored binding is unknown. + * NO route may be chosen yet: guessing "direct" dead-ends an origin-drifted + * DCR client (#1542), and guessing "automatic" would rebind a manual app. + * The caller keeps the action unavailable until the summaries resolve. */ + | { readonly kind: "unknown" } + /** Re-run the automatic probe/CIMD/DCR flow. `stored` is the binding's + * summary — undefined when its row is gone — so the handoff can carry its + * resource. */ + | { readonly kind: "automatic"; readonly stored: OAuthClientSummary | undefined } + /** Start the OAuth flow directly against the stored client. */ + | { readonly kind: "direct" }; + +/** Decide the Reconnect route from the STORED client binding. + * + * The binding's ORIGIN is what routes, not the method's capability flags: + * - An auto-minted DCR binding re-registers (its whole lifecycle is + * automatic, and the automatic flow can probe the token URL even when the + * method declares no discovery/DCR support). Reusing it directly dead-ends + * once the callback origin drifts (#1542). + * - A manual (static/BYO) or first-party binding keeps the direct + * stored-client path — re-registering would silently rebind the connection + * to an automatic client. + * - A binding whose row is GONE has nothing to start directly against, so it + * re-registers when the method supports the automatic flow at all. + * Pass `clients: undefined` while the summaries are loading: the decision is + * then `"unknown"`, never a guess. */ +export function reconnectRoute( + clients: readonly OAuthClientSummary[] | undefined, + connection: Connection, + methodSupportsAutomatic: boolean, +): ReconnectRoute { + if (clients === undefined) return { kind: "unknown" }; + const stored = reconnectStoredClient(clients, connection); + if (stored !== undefined) { + return stored.origin.kind === "dynamic_client_registration" + ? { kind: "automatic", stored } + : { kind: "direct" }; + } + return methodSupportsAutomatic ? { kind: "automatic", stored: undefined } : { kind: "direct" }; } // --------------------------------------------------------------------------- From 98606d41ab1a583e8d9748d6c52ddff8d6f6b017 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Sat, 29 Aug 2026 01:07:53 -0700 Subject: [PATCH 6/8] Route reconnect by stale client summaries on refresh failure, surface and retry data-less failures --- .../react/src/components/accounts-section.tsx | 54 ++++++++++---- .../react/src/plugins/oauth-reconnect.test.ts | 70 +++++++++++++++++++ packages/react/src/plugins/oauth-reconnect.ts | 42 +++++++++++ 3 files changed, 154 insertions(+), 12 deletions(-) diff --git a/packages/react/src/components/accounts-section.tsx b/packages/react/src/components/accounts-section.tsx index 41d5a3d2a..27eb49d6f 100644 --- a/packages/react/src/components/accounts-section.tsx +++ b/packages/react/src/components/accounts-section.tsx @@ -1,5 +1,5 @@ import { useEffect, useMemo, useState } from "react"; -import { useAtomValue, useAtomSet } from "@effect/atom-react"; +import { useAtomValue, useAtomRefresh, useAtomSet } from "@effect/atom-react"; import * as AsyncResult from "effect/unstable/reactivity/AsyncResult"; import * as Exit from "effect/Exit"; import { @@ -29,9 +29,11 @@ import type { AuthMethod } from "../lib/auth-placements"; import { connectionNeedsReconsent, oauthReconnectPayload, + reconnectClientsView, reconnectMode, reconnectRoute, reconsentRequiredScopes, + retryReconnectClientsOnMenuOpen, } from "../plugins/oauth-reconnect"; import { useOAuthPopupFlow } from "../plugins/oauth-sign-in"; import { AddAccountModal, hasDcr } from "./add-account-modal"; @@ -125,9 +127,17 @@ function AccountRow(props: { readonly onEdit: () => void; readonly onReconnect: () => void; /** Reconnect routing needs the stored client binding; while the client - * summaries are still loading the route is unknown, so the action is + * summaries carry no data the route is unknown, so the action is * disabled rather than guessed (same idiom as "Check now" above). */ readonly reconnectDisabled: boolean; + /** The summaries query failed with NO data to route by: the Reconnect item + * stays disabled but says so (never a silently dead action). Opening the + * menu retries the query via `onMenuOpenChange`, so the hint reflects a + * retry that just failed, not a permanently stuck state. */ + readonly reconnectFailed: boolean; + /** Forwarded to the row menu; the owner uses the OPEN transition to retry + * a failed client-summaries query. */ + readonly onMenuOpenChange: (open: boolean) => void; readonly onRemove: () => void; }) { const { connection, needsReconsent } = props; @@ -256,7 +266,7 @@ function AccountRow(props: { {props.showOwnerLabel ? ( {ownerLabel(connection.owner)} ) : null} - +