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..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"; @@ -143,4 +144,89 @@ 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, + }), + ); + // 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"); + 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(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 008719b75..710bee750 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,24 @@ 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. + // 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: ${rawMessage}`; + 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, 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."); } };