History

Show an actionable message when MCP OAuth dynamic registration is rejected (#1121)

* 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) * 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) ---------

Jack Lau committed 4 months ago

Commit 5250f641 parent 0de01066 files · +195 −15Browse files

Unified · Split

6 files
  1. packages/core/sdk/src/oauth-helpers.ts +1 −1
  2. packages/core/sdk/src/oauth-register-dynamic.test.ts +87 −1
  3. packages/core/sdk/src/oauth-service.ts +19 −7
  4. packages/core/sdk/src/testing/oauth-test-server.ts +15 −0
  5. packages/react/src/components/add-account-modal.test.ts +43 −0
  6. packages/react/src/components/add-account-modal.tsx +30 −6
packages/core/sdk/src/oauth-helpers.ts+1−1
@@ -61,7 +61,7 @@ export interface OAuthEndpointUrlPolicy { · 60 unmodified lines
61 readonly allowHttp?: boolean;61 readonly allowHttp?: boolean;
62}62}
63 63
64const isLoopbackHttpUrl = (value: string): boolean => {64export const isLoopbackHttpUrl = (value: string): boolean => {
65 if (!URL.canParse(value)) return false;65 if (!URL.canParse(value)) return false;
66 const url = new URL(value);66 const url = new URL(value);
67 if (url.protocol !== "http:") return false;67 if (url.protocol !== "http:") return false;
packages/core/sdk/src/oauth-register-dynamic.test.ts+87−1
@@ -1,5 +1,5 @@
1import { describe, expect, it } from "@effect/vitest";1import { describe, expect, it } from "@effect/vitest";
2import { Effect } from "effect";2import { Effect, Predicate } from "effect";
3 3
4import {4import {
5 AuthTemplateSlug,5 AuthTemplateSlug,
@@ -8,6 +8,7 @@ import { · 2 unmodified lines
8 OAuthClientSlug,8 OAuthClientSlug,
9 ToolName,9 ToolName,
10} from "./ids";10} from "./ids";
11import { OAuthRegisterDynamicError } from "./oauth-client";
11import { definePlugin } from "./plugin";12import { definePlugin } from "./plugin";
12import { makeTestWorkspaceHarness, memoryCredentialsPlugin } from "./test-config";13import { makeTestWorkspaceHarness, memoryCredentialsPlugin } from "./test-config";
13import { serveOAuthTestServer } from "./testing/oauth-test-server";14import { serveOAuthTestServer } from "./testing/oauth-test-server";
@@ -143,4 +144,89 @@ describe("oauth.registerDynamicClient", () => { · 129 unmodified lines
143 }),144 }),
144 ),145 ),
145 );146 );
147
148 // Regression: issue #770. Vercel (and other RFC 8252-strict servers) only
149 // approve loopback redirect URIs for anonymous DCR, so a hosted/tailnet/LAN
150 // origin trips `invalid_redirect_uri`. The failure must explain the loopback
151 // requirement and name the offending URI, not dump the raw RFC code.
152 it.effect("DCR rejection of a non-loopback redirect URI yields an actionable loopback hint", () =>
153 Effect.scoped(
154 Effect.gen(function* () {
155 const server = yield* serveOAuthTestServer({
156 scopes: ["read"],
157 // Mirror Vercel: approve only loopback redirect URIs for anonymous DCR.
158 approveRedirectUri: (uri) =>
159 uri.startsWith("http://localhost") || uri.startsWith("http://127."),
160 });
161 const { executor } = yield* makeTestWorkspaceHarness({ plugins });
162 yield* executor.acme.seed();
163 const probe = yield* executor.oauth.probe({ url: server.mcpResourceUrl });
164
165 const nonLoopback = "https://app.example.com/api/oauth/callback";
166 const error = yield* Effect.flip(
167 executor.oauth.registerDynamicClient({
168 owner: "org",
169 slug: CLIENT,
170 registrationEndpoint: probe.registrationEndpoint!,
171 authorizationUrl: probe.authorizationUrl,
172 tokenUrl: probe.tokenUrl,
173 resource: probe.resource,
174 scopes: ["read"],
175 tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported,
176 clientName: "Acme DCR",
177 redirectUri: nonLoopback,
178 originIntegration: INTEG,
179 }),
180 );
181 // Predicate guard narrows the union so `.message` reads off a typed failure.
182 expect(Predicate.isTagged("OAuthRegisterDynamicError")(error)).toBe(true);
183 const registerError = error as OAuthRegisterDynamicError;
184 const message = registerError.message;
185 // Names the loopback-only requirement, the localhost fix, and the URI.
186 expect(message).toContain("loopback");
187 expect(message).toContain("http://localhost");
188 expect(message).toContain(nonLoopback);
189 }),
190 ),
191 );
192
193 // The loopback hint is gated on the redirect URI actually being non-loopback.
194 // A server that rejects even a loopback URI keeps the generic message so we
195 // never tell the user "use localhost" when they already are.
196 it.effect(
197 "DCR rejection of a loopback redirect URI keeps the generic message (no false hint)",
198 () =>
199 Effect.scoped(
200 Effect.gen(function* () {
201 const server = yield* serveOAuthTestServer({
202 scopes: ["read"],
203 approveRedirectUri: () => false, // reject every redirect URI, even loopback
204 });
205 const { executor } = yield* makeTestWorkspaceHarness({ plugins });
206 yield* executor.acme.seed();
207 const probe = yield* executor.oauth.probe({ url: server.mcpResourceUrl });
208
209 const error = yield* Effect.flip(
210 executor.oauth.registerDynamicClient({
211 owner: "org",
212 slug: CLIENT,
213 registrationEndpoint: probe.registrationEndpoint!,
214 authorizationUrl: probe.authorizationUrl,
215 tokenUrl: probe.tokenUrl,
216 resource: probe.resource,
217 scopes: ["read"],
218 tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported,
219 clientName: "Acme DCR",
220 redirectUri: "http://127.0.0.1:5394/api/oauth/callback",
221 originIntegration: INTEG,
222 }),
223 );
224 expect(Predicate.isTagged("OAuthRegisterDynamicError")(error)).toBe(true);
225 const registerError = error as OAuthRegisterDynamicError;
226 const message = registerError.message;
227 expect(message).toContain("Dynamic Client Registration failed: invalid_redirect_uri");
228 expect(message).not.toContain("Automatic OAuth setup failed");
229 }),
230 ),
231 );
146});232});
packages/core/sdk/src/oauth-service.ts+19−7
@@ -64,6 +64,7 @@ import { · 63 unmodified lines
64 createPkceCodeVerifier,64 createPkceCodeVerifier,
65 exchangeAuthorizationCode,65 exchangeAuthorizationCode,
66 exchangeClientCredentials,66 exchangeClientCredentials,
67 isLoopbackHttpUrl,
67 rebindTokenEndpointHostToCallbackDomain,68 rebindTokenEndpointHostToCallbackDomain,
68 type OAuth2TokenResponse,69 type OAuth2TokenResponse,
69 type OAuthEndpointUrlPolicy,70 type OAuthEndpointUrlPolicy,
@@ -573,13 +574,24 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { · 503 unmodified lines
573 },574 },
574 { httpClientLayer, endpointUrlPolicy: deps.endpointUrlPolicy },575 { httpClientLayer, endpointUrlPolicy: deps.endpointUrlPolicy },
575 ).pipe(576 ).pipe(
576 Effect.mapError(577 Effect.mapError((cause) => {
577 (cause) =>578 // Some authorization servers (Vercel, and others that follow RFC 8252
578 new OAuthRegisterDynamicError({579 // strictly) reject anonymous Dynamic Client Registration unless the
579 // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: OAuthDiscoveryError carries a typed `message` field580 // redirect URI is loopback (http://localhost or http://127.0.0.1).
580 message: `Dynamic Client Registration failed: ${cause.message}`,581 // Executor registers its browser origin, so any hosted, tailnet, or
581 }),582 // LAN origin trips `invalid_redirect_uri`. Turn that opaque RFC code
582 ),583 // into guidance the user can act on instead of the raw error.
584 // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: OAuthDiscoveryError carries a typed `message`
585 const rawMessage = cause.message;
586 const message =
587 cause.error === "invalid_redirect_uri" && !isLoopbackHttpUrl(flowRedirectUri)
588 ? `Automatic OAuth setup failed: this server only approves loopback redirect ` +
589 `URLs (http://localhost or http://127.0.0.1) for automatic registration, but ` +
590 `Executor is using ${flowRedirectUri}. Register an OAuth app manually with that ` +
591 `redirect URL approved by the server, or run Executor on http://localhost.`
592 : `Dynamic Client Registration failed: ${rawMessage}`;
593 return new OAuthRegisterDynamicError({ message });
594 }),
583 );595 );
584 596
585 // Persist the minted client. DCR-minted public clients have no secret; we597 // Persist the minted client. DCR-minted public clients have no secret; we
packages/core/sdk/src/testing/oauth-test-server.ts+15−0
@@ -53,6 +53,11 @@ export interface OAuthTestServerOptions { · 52 unmodified lines
53 readonly scopes?: readonly string[];53 readonly scopes?: readonly string[];
54 readonly omitTokenResponseScopes?: readonly string[];54 readonly omitTokenResponseScopes?: readonly string[];
55 readonly supportRefresh?: boolean;55 readonly supportRefresh?: boolean;
56 /** Gate Dynamic Client Registration on the requested redirect URIs. When set,
57 * `/register` returns `400 invalid_redirect_uri` unless every requested
58 * `redirect_uris` entry is approved. Mirrors authorization servers (e.g.
59 * Vercel) that only accept loopback redirect URIs for anonymous DCR. */
60 readonly approveRedirectUri?: (uri: string) => boolean;
56}61}
57 62
58export interface OAuthTestServerShape {63export interface OAuthTestServerShape {
@@ -503,6 +508,16 @@ export const serveOAuthTestServer = ( · 444 unmodified lines
503 ? `secret_${randomUUID()}`508 ? `secret_${randomUUID()}`
504 : null;509 : null;
505 const redirectUris = new Set(arrayOfStrings(json.redirect_uris));510 const redirectUris = new Set(arrayOfStrings(json.redirect_uris));
511 if (
512 options.approveRedirectUri &&
513 [...redirectUris].some((uri) => !options.approveRedirectUri!(uri))
514 ) {
515 return oauthError(
516 400,
517 "invalid_redirect_uri",
518 "The provided redirect URIs are not approved for use by this authorization server.",
519 );
520 }
506 clients.set(clientId, {521 clients.set(clientId, {
507 clientSecret,522 clientSecret,
508 redirectUris,523 redirectUris,
packages/react/src/components/add-account-modal.test.ts+43−0
@@ -429,4 +429,47 @@ describe("runDcrConnect", () => { · 428 unmodified lines
429 });429 });
430 expect(calls).toEqual(["register"]);430 expect(calls).toEqual(["register"]);
431 });431 });
432
433 it("threads the register failure message into the fallback (registration-failed)", async () => {
434 const message =
435 "Automatic OAuth setup failed: this server only approves loopback redirect URLs " +
436 "(http://localhost or http://127.0.0.1) for automatic registration, but Executor is " +
437 "using https://app.example.com/api/oauth/callback. Register an OAuth app manually with " +
438 "that redirect URL approved by the server, or run Executor on http://localhost.";
439 let started = false;
440 const outcome = await runDcrConnect(
441 {
442 probe: (): Promise<ProbeResult | null> =>
443 Promise.resolve({
444 authorizationUrl: "https://auth.example.com/authorize",
445 tokenUrl: "https://auth.example.com/token",
446 registrationEndpoint: "https://auth.example.com/register",
447 }),
448 register: (): Promise<{ readonly error: string }> => Promise.resolve({ error: message }),
449 start: (): void => {
450 started = true;
451 },
452 },
453 {
454 discoveryUrl: "https://mcp.example.com/mcp",
455 owner: "user",
456 integrationName: "App",
457 existingSlugs: [],
458 integration: TEST_INTEGRATION,
459 },
460 );
461 // The redirect-URI rejection reaches the caller verbatim so the BYO fallback
462 // can show why instead of the generic copy, and the OAuth start is skipped.
463 expect(outcome).toEqual({
464 kind: "fallback",
465 reason: "registration-failed",
466 probe: {
467 authorizationUrl: "https://auth.example.com/authorize",
468 tokenUrl: "https://auth.example.com/token",
469 registrationEndpoint: "https://auth.example.com/register",
470 },
471 message,
472 });
473 expect(started).toBe(false);
474 });
432});475});
packages/react/src/components/add-account-modal.tsx+30−6
@@ -514,14 +514,22 @@ type DcrOutcome = · 513 unmodified lines
514 readonly kind: "fallback";514 readonly kind: "fallback";
515 readonly reason: "no-registration-endpoint" | "registration-failed";515 readonly reason: "no-registration-endpoint" | "registration-failed";
516 readonly probe: DcrProbeResult;516 readonly probe: DcrProbeResult;
517 /** A specific, user-facing reason to surface instead of the generic
518 * "register an app" copy (e.g. a server that rejects the DCR redirect
519 * URI). Absent when the fallback carries no actionable detail. */
520 readonly message?: string;
517 };521 };
518 522
519type RunDcrConnectDeps = {523type RunDcrConnectDeps = {
520 /** Probe the discovery URL → resolved endpoints + (maybe) a registration524 /** Probe the discovery URL → resolved endpoints + (maybe) a registration
521 * endpoint. Resolves to null when the probe fails. */525 * endpoint. Resolves to null when the probe fails. */
522 readonly probe: (url: string) => Promise<DcrProbeResult | null>;526 readonly probe: (url: string) => Promise<DcrProbeResult | null>;
523 /** Register a DCR client → the minted client slug, or null on failure. */527 /** Register a DCR client → the minted client slug, `{ error }` with a
524 readonly register: (args: DcrRegisterArgs) => Promise<OAuthClientSlug | null>;528 * user-facing message when the server rejects registration, or null on an
529 * unexplained failure. */
530 readonly register: (
531 args: DcrRegisterArgs,
532 ) => Promise<OAuthClientSlug | { readonly error: string } | null>;
525 /** Start the OAuth flow with the minted client (popup / inline). */533 /** Start the OAuth flow with the minted client (popup / inline). */
526 readonly start: (args: DcrStartArgs) => void;534 readonly start: (args: DcrStartArgs) => void;
527};535};
@@ -557,7 +565,9 @@ export const dcrClientNameForIntegration = (integrationName: string): string => · 29 unmodified lines
557 *565 *
558 * - Probe failure → `{ kind: "fallback", reason: "probe-failed" }` (caller shows BYO).566 * - Probe failure → `{ kind: "fallback", reason: "probe-failed" }` (caller shows BYO).
559 * - No registration endpoint → `{ kind: "fallback", reason: "no-registration-endpoint", probe }`.567 * - No registration endpoint → `{ kind: "fallback", reason: "no-registration-endpoint", probe }`.
560 * - Registration failure → `{ kind: "fallback", reason: "registration-failed", probe }`.568 * - Register rejected with a message → `{ kind: "fallback", reason: "registration-failed", probe, message }`
569 * so the caller can show why (e.g. a redirect-URI rejection) over the generic copy.
570 * - Register failed without detail (null) → `{ kind: "fallback", reason: "registration-failed", probe }`.
561 * - Success → registers, calls `start`, returns `{ kind: "started" }`.571 * - Success → registers, calls `start`, returns `{ kind: "started" }`.
562 */572 */
563export async function runDcrConnect(573export async function runDcrConnect(
@@ -585,6 +595,11 @@ export async function runDcrConnect( · 21 unmodified lines
585 originIntegration: input.integration,595 originIntegration: input.integration,
586 });596 });
587 if (minted === null) return { kind: "fallback", reason: "registration-failed", probe };597 if (minted === null) return { kind: "fallback", reason: "registration-failed", probe };
598 // OAuthClientSlug is a branded string; an object return carries the failure
599 // message (e.g. the server rejected the redirect URI) for the BYO fallback.
600 if (typeof minted === "object") {
601 return { kind: "fallback", reason: "registration-failed", probe, message: minted.error };
602 }
588 deps.start({ client: minted, owner: input.owner });603 deps.start({ client: minted, owner: input.owner });
589 return { kind: "started" };604 return { kind: "started" };
590}605}
@@ -1179,7 +1194,9 @@ function AddAccountModalView(props: AddAccountModalProps) { · 588 unmodified lines
1179 if (Exit.isFailure(exit)) return null;1194 if (Exit.isFailure(exit)) return null;
1180 return exit.value;1195 return exit.value;
1181 },1196 },
1182 register: async (args: DcrRegisterArgs): Promise<OAuthClientSlug | null> => {1197 register: async (
1198 args: DcrRegisterArgs,
1199 ): Promise<OAuthClientSlug | { readonly error: string } | null> => {
1183 const exit = await doRegisterDynamic({1200 const exit = await doRegisterDynamic({
1184 payload: {1201 payload: {
1185 owner: args.owner,1202 owner: args.owner,
@@ -1196,7 +1213,11 @@ function AddAccountModalView(props: AddAccountModalProps) { · 10 unmodified lines
1196 },1213 },
1197 reactivityKeys: oauthClientWriteKeys,1214 reactivityKeys: oauthClientWriteKeys,
1198 });1215 });
1199 if (Exit.isFailure(exit)) return null;1216 if (Exit.isFailure(exit)) {
1217 return {
1218 error: messageFromExit(exit, "Automatic setup unavailable. Register an app instead."),
1219 };
1220 }
1200 return exit.value.client;1221 return exit.value.client;
1201 },1222 },
1202 start: (args: DcrStartArgs): void => {1223 start: (args: DcrStartArgs): void => {
@@ -1246,7 +1267,10 @@ function AddAccountModalView(props: AddAccountModalProps) { · 43 unmodified lines
1246 if (outcome.kind === "fallback") {1267 if (outcome.kind === "fallback") {
1247 setOAuthFallbackProbe("probe" in outcome ? outcome.probe : null);1268 setOAuthFallbackProbe("probe" in outcome ? outcome.probe : null);
1248 setDcrFailed(true);1269 setDcrFailed(true);
1249 toast.error("Automatic setup unavailable — register an app");1270 toast.error(
1271 ("message" in outcome ? outcome.message : undefined) ??
1272 "Automatic setup unavailable. Register an app instead.",
1273 );
1250 }1274 }
1251 };1275 };
1252 1276