From 51f88397605e61c407000fe00031b13730dfb77f Mon Sep 17 00:00:00 2001 From: Jack <72348727+Jack-GitHub12@users.noreply.github.com> Date: Tue, 23 Jun 2026 23:59:08 -0500 Subject: [PATCH 1/2] fix(oauth): actionable message when DCR rejects a non-loopback redirect URI Authorization servers that follow RFC 8252 strictly (e.g. Vercel MCP) only approve loopback redirect URIs for anonymous Dynamic Client Registration. Executor registers its browser origin, so a hosted, tailnet, or LAN origin trips invalid_redirect_uri. Map that opaque code to guidance: name the loopback-only requirement, the offending URI, and the two recovery paths. Gated on the redirect URI being non-loopback so we never tell a localhost user to use localhost. Adds an approveRedirectUri option to the OAuth test server to mimic the loopback-only allowlist. Goal: 001-fix-770-vercel-mcp-dcr-fallback (deliverable 1) --- packages/core/sdk/src/oauth-helpers.ts | 2 +- .../sdk/src/oauth-register-dynamic.test.ts | 82 +++++++++++++++++++ packages/core/sdk/src/oauth-service.ts | 25 ++++-- .../core/sdk/src/testing/oauth-test-server.ts | 15 ++++ 4 files changed, 116 insertions(+), 8 deletions(-) diff --git a/packages/core/sdk/src/oauth-helpers.ts b/packages/core/sdk/src/oauth-helpers.ts index c619704ff..077eb6ec4 100644 --- a/packages/core/sdk/src/oauth-helpers.ts +++ b/packages/core/sdk/src/oauth-helpers.ts @@ -61,7 +61,7 @@ export interface OAuthEndpointUrlPolicy { readonly allowHttp?: boolean; } -const isLoopbackHttpUrl = (value: string): boolean => { +export const isLoopbackHttpUrl = (value: string): boolean => { if (!URL.canParse(value)) return false; const url = new URL(value); if (url.protocol !== "http:") return false; diff --git a/packages/core/sdk/src/oauth-register-dynamic.test.ts b/packages/core/sdk/src/oauth-register-dynamic.test.ts index bc0298b19..149495c35 100644 --- a/packages/core/sdk/src/oauth-register-dynamic.test.ts +++ b/packages/core/sdk/src/oauth-register-dynamic.test.ts @@ -143,4 +143,86 @@ describe("oauth.registerDynamicClient", () => { }), ), ); + + // Regression: issue #770. Vercel (and other RFC 8252-strict servers) only + // approve loopback redirect URIs for anonymous DCR, so a hosted/tailnet/LAN + // origin trips `invalid_redirect_uri`. The failure must explain the loopback + // requirement and name the offending URI, not dump the raw RFC code. + it.effect("DCR rejection of a non-loopback redirect URI yields an actionable loopback hint", () => + Effect.scoped( + Effect.gen(function* () { + const server = yield* serveOAuthTestServer({ + scopes: ["read"], + // Mirror Vercel: approve only loopback redirect URIs for anonymous DCR. + approveRedirectUri: (uri) => + uri.startsWith("http://localhost") || uri.startsWith("http://127."), + }); + const { executor } = yield* makeTestWorkspaceHarness({ plugins }); + yield* executor.acme.seed(); + const probe = yield* executor.oauth.probe({ url: server.mcpResourceUrl }); + + const nonLoopback = "https://app.example.com/api/oauth/callback"; + const error = yield* Effect.flip( + executor.oauth.registerDynamicClient({ + owner: "org", + slug: CLIENT, + registrationEndpoint: probe.registrationEndpoint!, + authorizationUrl: probe.authorizationUrl, + tokenUrl: probe.tokenUrl, + resource: probe.resource, + scopes: ["read"], + tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported, + clientName: "Acme DCR", + redirectUri: nonLoopback, + originIntegration: INTEG, + }), + ); + expect(error._tag).toBe("OAuthRegisterDynamicError"); + const message = (error as { readonly message: string }).message; + // Names the loopback-only requirement, the localhost fix, and the URI. + expect(message).toContain("loopback"); + expect(message).toContain("http://localhost"); + expect(message).toContain(nonLoopback); + }), + ), + ); + + // The loopback hint is gated on the redirect URI actually being non-loopback. + // A server that rejects even a loopback URI keeps the generic message so we + // never tell the user "use localhost" when they already are. + it.effect( + "DCR rejection of a loopback redirect URI keeps the generic message (no false hint)", + () => + Effect.scoped( + Effect.gen(function* () { + const server = yield* serveOAuthTestServer({ + scopes: ["read"], + approveRedirectUri: () => false, // reject every redirect URI, even loopback + }); + const { executor } = yield* makeTestWorkspaceHarness({ plugins }); + yield* executor.acme.seed(); + const probe = yield* executor.oauth.probe({ url: server.mcpResourceUrl }); + + const error = yield* Effect.flip( + executor.oauth.registerDynamicClient({ + owner: "org", + slug: CLIENT, + registrationEndpoint: probe.registrationEndpoint!, + authorizationUrl: probe.authorizationUrl, + tokenUrl: probe.tokenUrl, + resource: probe.resource, + scopes: ["read"], + tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported, + clientName: "Acme DCR", + redirectUri: "http://127.0.0.1:5394/api/oauth/callback", + originIntegration: INTEG, + }), + ); + expect(error._tag).toBe("OAuthRegisterDynamicError"); + const message = (error as { readonly message: string }).message; + expect(message).toContain("Dynamic Client Registration failed: invalid_redirect_uri"); + expect(message).not.toContain("Automatic OAuth setup failed"); + }), + ), + ); }); diff --git a/packages/core/sdk/src/oauth-service.ts b/packages/core/sdk/src/oauth-service.ts index 008719b75..aa79f0255 100644 --- a/packages/core/sdk/src/oauth-service.ts +++ b/packages/core/sdk/src/oauth-service.ts @@ -63,6 +63,7 @@ import { createPkceCodeVerifier, exchangeAuthorizationCode, exchangeClientCredentials, + isLoopbackHttpUrl, rebindTokenEndpointHostToCallbackDomain, type OAuth2TokenResponse, type OAuthEndpointUrlPolicy, @@ -496,13 +497,23 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { }, { httpClientLayer, endpointUrlPolicy: deps.endpointUrlPolicy }, ).pipe( - Effect.mapError( - (cause) => - new OAuthRegisterDynamicError({ - // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: OAuthDiscoveryError carries a typed `message` field - message: `Dynamic Client Registration failed: ${cause.message}`, - }), - ), + Effect.mapError((cause) => { + // Some authorization servers (Vercel, and others that follow RFC 8252 + // strictly) reject anonymous Dynamic Client Registration unless the + // redirect URI is loopback (http://localhost or http://127.0.0.1). + // Executor registers its browser origin, so any hosted, tailnet, or + // LAN origin trips `invalid_redirect_uri`. Turn that opaque RFC code + // into guidance the user can act on instead of the raw error. + const message = + cause.error === "invalid_redirect_uri" && !isLoopbackHttpUrl(flowRedirectUri) + ? `Automatic OAuth setup failed: this server only approves loopback redirect ` + + `URLs (http://localhost or http://127.0.0.1) for automatic registration, but ` + + `Executor is using ${flowRedirectUri}. Register an OAuth app manually with that ` + + `redirect URL approved by the server, or run Executor on http://localhost.` + : `Dynamic Client Registration failed: ${cause.message}`; + // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: OAuthDiscoveryError carries a typed `message`/`error` + return new OAuthRegisterDynamicError({ message }); + }), ); // Persist the minted client. DCR-minted public clients have no secret; we diff --git a/packages/core/sdk/src/testing/oauth-test-server.ts b/packages/core/sdk/src/testing/oauth-test-server.ts index f5c943874..1c4ed5abc 100644 --- a/packages/core/sdk/src/testing/oauth-test-server.ts +++ b/packages/core/sdk/src/testing/oauth-test-server.ts @@ -53,6 +53,11 @@ export interface OAuthTestServerOptions { readonly scopes?: readonly string[]; readonly omitTokenResponseScopes?: readonly string[]; readonly supportRefresh?: boolean; + /** Gate Dynamic Client Registration on the requested redirect URIs. When set, + * `/register` returns `400 invalid_redirect_uri` unless every requested + * `redirect_uris` entry is approved. Mirrors authorization servers (e.g. + * Vercel) that only accept loopback redirect URIs for anonymous DCR. */ + readonly approveRedirectUri?: (uri: string) => boolean; } export interface OAuthTestServerShape { @@ -496,6 +501,16 @@ export const serveOAuthTestServer = ( ? `secret_${randomUUID()}` : null; const redirectUris = new Set(arrayOfStrings(json.redirect_uris)); + if ( + options.approveRedirectUri && + [...redirectUris].some((uri) => !options.approveRedirectUri!(uri)) + ) { + return oauthError( + 400, + "invalid_redirect_uri", + "The provided redirect URIs are not approved for use by this authorization server.", + ); + } clients.set(clientId, { clientSecret, redirectUris, From 13304142699d1097652ac4faa1d1f42b6df31645 Mon Sep 17 00:00:00 2001 From: Jack <72348727+Jack-GitHub12@users.noreply.github.com> Date: Wed, 24 Jun 2026 00:06:39 -0500 Subject: [PATCH 2/2] fix(react): surface the DCR rejection reason in the BYO fallback When transparent Dynamic Client Registration fails, runDcrConnect now returns the server's failure message in the fallback outcome instead of swallowing it, and the Add MCP Source / add-account modal shows that message (e.g. the loopback redirect-URI rejection) over the generic 'register an app' copy. The register dep widens to return { error } on failure; the OAuth start is skipped. Also narrows the new SDK test's error reads to the typed OAuthRegisterDynamicError and repositions the boundary lint directive. Goal: 001-fix-770-vercel-mcp-dcr-fallback (deliverable 2) --- .../sdk/src/oauth-register-dynamic.test.ts | 14 ++++--- packages/core/sdk/src/oauth-service.ts | 5 ++- .../src/components/add-account-modal.test.ts | 36 +++++++++++++++++- .../src/components/add-account-modal.tsx | 38 ++++++++++++++----- 4 files changed, 76 insertions(+), 17 deletions(-) diff --git a/packages/core/sdk/src/oauth-register-dynamic.test.ts b/packages/core/sdk/src/oauth-register-dynamic.test.ts index 149495c35..6a83986cb 100644 --- a/packages/core/sdk/src/oauth-register-dynamic.test.ts +++ b/packages/core/sdk/src/oauth-register-dynamic.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "@effect/vitest"; -import { Effect } from "effect"; +import { Effect, Predicate } from "effect"; import { AuthTemplateSlug, @@ -8,6 +8,7 @@ import { OAuthClientSlug, ToolName, } from "./ids"; +import { OAuthRegisterDynamicError } from "./oauth-client"; import { definePlugin } from "./plugin"; import { makeTestWorkspaceHarness, memoryCredentialsPlugin } from "./test-config"; import { serveOAuthTestServer } from "./testing/oauth-test-server"; @@ -177,8 +178,10 @@ describe("oauth.registerDynamicClient", () => { originIntegration: INTEG, }), ); - expect(error._tag).toBe("OAuthRegisterDynamicError"); - const message = (error as { readonly message: string }).message; + // Predicate guard narrows the union so `.message` reads off a typed failure. + expect(Predicate.isTagged("OAuthRegisterDynamicError")(error)).toBe(true); + const registerError = error as OAuthRegisterDynamicError; + const message = registerError.message; // Names the loopback-only requirement, the localhost fix, and the URI. expect(message).toContain("loopback"); expect(message).toContain("http://localhost"); @@ -218,8 +221,9 @@ describe("oauth.registerDynamicClient", () => { originIntegration: INTEG, }), ); - expect(error._tag).toBe("OAuthRegisterDynamicError"); - const message = (error as { readonly message: string }).message; + expect(Predicate.isTagged("OAuthRegisterDynamicError")(error)).toBe(true); + const registerError = error as OAuthRegisterDynamicError; + const message = registerError.message; expect(message).toContain("Dynamic Client Registration failed: invalid_redirect_uri"); expect(message).not.toContain("Automatic OAuth setup failed"); }), diff --git a/packages/core/sdk/src/oauth-service.ts b/packages/core/sdk/src/oauth-service.ts index aa79f0255..710bee750 100644 --- a/packages/core/sdk/src/oauth-service.ts +++ b/packages/core/sdk/src/oauth-service.ts @@ -504,14 +504,15 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { // Executor registers its browser origin, so any hosted, tailnet, or // LAN origin trips `invalid_redirect_uri`. Turn that opaque RFC code // into guidance the user can act on instead of the raw error. + // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: OAuthDiscoveryError carries a typed `message` + const rawMessage = cause.message; const message = cause.error === "invalid_redirect_uri" && !isLoopbackHttpUrl(flowRedirectUri) ? `Automatic OAuth setup failed: this server only approves loopback redirect ` + `URLs (http://localhost or http://127.0.0.1) for automatic registration, but ` + `Executor is using ${flowRedirectUri}. Register an OAuth app manually with that ` + `redirect URL approved by the server, or run Executor on http://localhost.` - : `Dynamic Client Registration failed: ${cause.message}`; - // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: OAuthDiscoveryError carries a typed `message`/`error` + : `Dynamic Client Registration failed: ${rawMessage}`; return new OAuthRegisterDynamicError({ message }); }), ); diff --git a/packages/react/src/components/add-account-modal.test.ts b/packages/react/src/components/add-account-modal.test.ts index 64ffd5290..9f22c2e84 100644 --- a/packages/react/src/components/add-account-modal.test.ts +++ b/packages/react/src/components/add-account-modal.test.ts @@ -371,7 +371,41 @@ describe("runDcrConnect", () => { integration: TEST_INTEGRATION, }, ); - expect(outcome.kind).toBe("fallback"); + expect(outcome).toEqual({ kind: "fallback", reason: "registration-failed" }); expect(calls).toEqual(["register"]); }); + + it("threads the register failure message into the fallback (registration-failed)", async () => { + const message = + "Automatic OAuth setup failed: this server only approves loopback redirect URLs " + + "(http://localhost or http://127.0.0.1) for automatic registration, but Executor is " + + "using https://app.example.com/api/oauth/callback. Register an OAuth app manually with " + + "that redirect URL approved by the server, or run Executor on http://localhost."; + let started = false; + const outcome = await runDcrConnect( + { + probe: (): Promise => + Promise.resolve({ + authorizationUrl: "https://auth.example.com/authorize", + tokenUrl: "https://auth.example.com/token", + registrationEndpoint: "https://auth.example.com/register", + }), + register: (): Promise<{ readonly error: string }> => Promise.resolve({ error: message }), + start: (): void => { + started = true; + }, + }, + { + discoveryUrl: "https://mcp.example.com/mcp", + owner: "user", + integrationName: "App", + existingSlugs: [], + integration: TEST_INTEGRATION, + }, + ); + // The redirect-URI rejection reaches the caller verbatim so the BYO fallback + // can show why instead of the generic copy, and the OAuth start is skipped. + expect(outcome).toEqual({ kind: "fallback", reason: "registration-failed", message }); + expect(started).toBe(false); + }); }); diff --git a/packages/react/src/components/add-account-modal.tsx b/packages/react/src/components/add-account-modal.tsx index f7927a230..7c460464f 100644 --- a/packages/react/src/components/add-account-modal.tsx +++ b/packages/react/src/components/add-account-modal.tsx @@ -454,15 +454,23 @@ type DcrOutcome = | { readonly kind: "started" } | { readonly kind: "fallback"; - readonly reason: "probe-failed" | "no-registration-endpoint"; + readonly reason: "probe-failed" | "no-registration-endpoint" | "registration-failed"; + /** A specific, user-facing reason to surface instead of the generic + * "register an app" copy (e.g. a server that rejects the DCR redirect + * URI). Absent when the fallback carries no actionable detail. */ + readonly message?: string; }; type RunDcrConnectDeps = { /** Probe the discovery URL → resolved endpoints + (maybe) a registration * endpoint. Resolves to null when the probe fails. */ readonly probe: (url: string) => Promise; - /** Register a DCR client → the minted client slug, or null on failure. */ - readonly register: (args: DcrRegisterArgs) => Promise; + /** Register a DCR client → the minted client slug, `{ error }` with a + * user-facing message when the server rejects registration, or null on an + * unexplained failure. */ + readonly register: ( + args: DcrRegisterArgs, + ) => Promise; /** Start the OAuth flow with the minted client (popup / inline). */ readonly start: (args: DcrStartArgs) => void; }; @@ -486,8 +494,9 @@ type RunDcrConnectInput = { * * - Probe failure → `{ kind: "fallback", reason: "probe-failed" }` (caller shows BYO). * - No registration endpoint → `{ kind: "fallback", reason: "no-registration-endpoint" }`. - * - Register failure → throws via the injected `register` rejecting; the caller - * treats a thrown/rejected register as fallback (kept out of the happy path). + * - Register rejected with a message → `{ kind: "fallback", reason: "registration-failed", 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" }`. * - Success → registers, calls `start`, returns `{ kind: "started" }`. */ export async function runDcrConnect( @@ -517,7 +526,12 @@ export async function runDcrConnect( redirectUri: input.redirectUri, originIntegration: input.integration, }); - if (minted === null) return { kind: "fallback", reason: "probe-failed" }; + if (minted === null) return { kind: "fallback", reason: "registration-failed" }; + // OAuthClientSlug is a branded string; an object return carries the failure + // message (e.g. the server rejected the redirect URI) for the BYO fallback. + if (typeof minted === "object") { + return { kind: "fallback", reason: "registration-failed", message: minted.error }; + } deps.start({ client: minted, owner: input.owner }); return { kind: "started" }; } @@ -1102,7 +1116,9 @@ export function AddAccountModal(props: { if (Exit.isFailure(exit)) return null; return exit.value; }, - register: async (args: DcrRegisterArgs): Promise => { + register: async ( + args: DcrRegisterArgs, + ): Promise => { const exit = await doRegisterDynamic({ payload: { owner: args.owner, @@ -1119,7 +1135,11 @@ export function AddAccountModal(props: { }, reactivityKeys: oauthClientWriteKeys, }); - if (Exit.isFailure(exit)) return null; + if (Exit.isFailure(exit)) { + return { + error: messageFromExit(exit, "Automatic setup unavailable. Register an app instead."), + }; + } return exit.value.client; }, start: (args: DcrStartArgs): void => { @@ -1164,7 +1184,7 @@ export function AddAccountModal(props: { }); if (outcome.kind === "fallback") { setDcrFailed(true); - toast.error("Automatic setup unavailable — register an app"); + toast.error(outcome.message ?? "Automatic setup unavailable. Register an app instead."); } };