fix(mcp): explain missing client_id when DCR was rejected

MCPOAuthFlow silently swallowed a rejected dynamic client-registration response and then threw the opaque "OAuth provider requires client_id" error from the fallback authorize probe. Users adding Figma's MCP server hit this because Figma's DCR endpoint 403s every unlisted client (per the Figma MCP catalog gate) and the surfaced string gave them nothing actionable to do. Capture the DCR endpoint + HTTP status (or the thrown transport error) on the flow instance and, when the authorize probe confirms client_id is required, throw an error that names the endpoint, the outcome, and the manual oauth.clientId/oauth.clientSecret workaround. The no-DCR- endpoint branch gets its own message so users know the server never advertised registration in the first place. Fixes #4307
2026-07-02 11:58:03 +00:00
parent 0ea6ea630b
commit 7d1ab692a4
3 changed files with 162 additions and 4 deletions
@@ -2,6 +2,10 @@

 ## [Unreleased]

+### Fixed
+
+- Fixed MCP OAuth reauth surfacing an opaque `OAuth provider requires client_id` error when the provider closes dynamic client registration (e.g. Figma, which 403s all unlisted clients per the MCP catalog). The error now names the DCR endpoint + HTTP status and directs users to configure `oauth.clientId`/`oauth.clientSecret` on the server entry ([#4307](https://github.com/can1357/oh-my-pi/issues/4307)).
+
 ## [16.3.1] - 2026-07-02

 ### Breaking Changes
@@ -79,6 +79,31 @@ function hasOAuthScope(scopes: string | null | undefined, scope: string): boolea
 	return !!scopes && scopes.split(/\s+/).includes(scope);
 }

+/**
+ * Trim a DCR failure body / thrown error message to a single, short line the
+ * caller can splice into an error string. `undefined` when nothing salvageable
+ * remains after stripping whitespace.
+ */
+function truncateDetail(raw: string | undefined): string | undefined {
+	if (!raw) return undefined;
+	const firstLine = raw.split(/\r?\n/, 1)[0]?.trim();
+	if (!firstLine) return undefined;
+	return firstLine.length > 200 ? `${firstLine.slice(0, 200)}…` : firstLine;
+}
+
+/**
+ * Read the response body of a rejected DCR request as a short diagnostic
+ * string. Never throws — the caller is already building an error and cannot
+ * afford to trade the actual failure for a "read body" one.
+ */
+async function readRegistrationFailureDetail(response: Response): Promise<string | undefined> {
+	try {
+		return truncateDetail(await response.text());
+	} catch {
+		return undefined;
+	}
+}
+
 function isLoopbackHostname(hostname: string): boolean {
 	return hostname === "localhost" || hostname === "127.0.0.1";
 }
@@ -294,6 +319,22 @@ export class MCPOAuthFlow extends OAuthCallbackFlow {
 	#codeVerifier?: string;
 	#fetch: FetchImpl;
 	#resource?: string;
+	/**
+	 * Details of a rejected dynamic client-registration attempt. Populated by
+	 * {@link #tryRegisterClient} when the provider advertises a registration
+	 * endpoint but returns a non-2xx / throws (e.g. Figma's DCR endpoint 403s
+	 * every request because only catalog-approved clients may connect). Reused
+	 * by {@link #missingClientIdError} to explain why the fallback probe now
+	 * requires a manually configured `oauth.clientId`, replacing the opaque
+	 * "OAuth provider requires client_id" message.
+	 */
+	#registrationFailure?: {
+		endpoint: string;
+		/** HTTP status returned by the endpoint; `0` when the request threw. */
+		status: number;
+		/** First line of the response body (or thrown error message), trimmed. */
+		detail?: string;
+	};

 	constructor(
 		private config: MCPOAuthConfig,
@@ -507,6 +548,13 @@ export class MCPOAuthFlow extends OAuthCallbackFlow {

 	/**
 	 * Try OAuth dynamic client registration when provider requires a client_id.
+	 *
+	 * Records rejection details on {@link #registrationFailure} so that when
+	 * DCR is intentionally closed (Figma's `mcp:connect` endpoint returns 403 to
+	 * every unlisted client — see https://developers.figma.com/docs/figma-mcp-server/,
+	 * "Only clients listed in the Figma MCP Catalog can connect"), the fallback
+	 * probe surfaces a message that names the endpoint and status instead of
+	 * the historical opaque "OAuth provider requires client_id".
 	 */
 	async #tryRegisterClient(redirectUri: string): Promise<void> {
 		const registrationEndpoint = await this.#resolveRegistrationEndpoint();
@@ -530,7 +578,14 @@ export class MCPOAuthFlow extends OAuthCallbackFlow {
 				}),
 			});

-			if (!response.ok) return;
+			if (!response.ok) {
+				this.#registrationFailure = {
+					endpoint: registrationEndpoint,
+					status: response.status,
+					detail: await readRegistrationFailureDetail(response),
+				};
+				return;
+			}

 			const data = (await response.json()) as {
 				client_id?: string;
@@ -543,8 +598,14 @@ export class MCPOAuthFlow extends OAuthCallbackFlow {
 			if (data.client_secret && data.client_secret.trim() !== "") {
 				this.#registeredClientSecret = data.client_secret;
 			}
-		} catch {
-			// Ignore registration failures and continue without client registration.
+		} catch (error) {
+			// Distinguish real transport/parse failures from a benign no-DCR
+			// response so #missingClientIdError can surface what went wrong.
+			this.#registrationFailure = {
+				endpoint: registrationEndpoint,
+				status: 0,
+				detail: error instanceof Error ? truncateDetail(error.message) : undefined,
+			};
 		}
 	}

@@ -609,7 +670,7 @@ export class MCPOAuthFlow extends OAuthCallbackFlow {
 			if (response.status < 400) return;
 			const body = await response.text();
 			if (/client[_-]?id/i.test(body) && /(required|missing|invalid)/i.test(body)) {
-				throw new Error("OAuth provider requires client_id");
+				throw this.#missingClientIdError();
 			}
 		} catch (error) {
 			if (error instanceof Error && /client[_-]?id/i.test(error.message)) {
@@ -618,6 +679,34 @@ export class MCPOAuthFlow extends OAuthCallbackFlow {
 			// Ignore network/probe failures to avoid blocking flows that still work.
 		}
 	}
+
+	/**
+	 * Build the error thrown when the authorize probe confirms the provider
+	 * demands a `client_id`. When dynamic client registration was attempted and
+	 * rejected (e.g. Figma's 403 for unlisted clients), fold the endpoint + HTTP
+	 * status into the message and point the user at the manual `oauth.clientId`
+	 * workaround. Refs issue #4307.
+	 */
+	#missingClientIdError(): Error {
+		const failure = this.#registrationFailure;
+		const manualHint =
+			"Configure `oauth.clientId` (and `oauth.clientSecret` if the flow needs one) on the MCP server entry in mcp.json.";
+		if (!failure) {
+			return new Error(
+				`OAuth provider requires client_id, and no dynamic-client-registration endpoint was advertised. ${manualHint}`,
+			);
+		}
+		const outcome =
+			failure.status > 0
+				? `HTTP ${failure.status}${failure.detail ? ` — ${failure.detail}` : ""}`
+				: failure.detail
+					? `network error — ${failure.detail}`
+					: "network error";
+		return new Error(
+			`OAuth provider requires client_id, and dynamic client registration was rejected ` +
+				`(POST ${failure.endpoint} → ${outcome}). The server likely restricts registration to pre-approved clients. ${manualHint}`,
+		);
+	}
 }

 /**
@@ -631,6 +631,71 @@ describe("mcp oauth flow", () => {
 		expect(registrationCalled).toBe(false);
 	});

+	// Issue #4307: Figma's DCR endpoint 403s every request (only catalog-approved
+	// clients may connect). The old flow swallowed the 403 and threw a bare
+	// "OAuth provider requires client_id" with no way for the user to see that
+	// DCR was tried and rejected. The rewritten error must name the endpoint and
+	// status and point at the `oauth.clientId` workaround.
+	it("surfaces DCR endpoint and status when registration is rejected", async () => {
+		const fetchImpl: FetchImpl = async input => {
+			const url = String(input);
+			if (url === "https://www.figma.com/.well-known/oauth-authorization-server") {
+				return new Response(
+					JSON.stringify({ registration_endpoint: "https://api.figma.com/v1/oauth/mcp/register" }),
+					{ status: 200, headers: { "Content-Type": "application/json" } },
+				);
+			}
+			if (url === "https://api.figma.com/v1/oauth/mcp/register") {
+				return new Response("Forbidden", { status: 403 });
+			}
+			if (url.startsWith("https://www.figma.com/oauth/mcp?")) {
+				return new Response("Parameter client_id is required", { status: 400 });
+			}
+			throw new Error(`Unexpected fetch: ${url}`);
+		};
+		const flow = new MCPOAuthFlow(
+			{
+				authorizationUrl: "https://www.figma.com/oauth/mcp",
+				tokenUrl: "https://api.figma.com/v1/oauth/token",
+				fetch: fetchImpl,
+			},
+			{},
+		);
+
+		await expect(flow.generateAuthUrl("state", "http://127.0.0.1:53190/callback")).rejects.toThrow(
+			/dynamic client registration was rejected \(POST https:\/\/api\.figma\.com\/v1\/oauth\/mcp\/register → HTTP 403 — Forbidden\).*oauth\.clientId/s,
+		);
+	});
+
+	it("names the missing-DCR case when no registration endpoint is advertised", async () => {
+		const fetchImpl: FetchImpl = async input => {
+			const url = String(input);
+			// Well-known metadata exists but omits `registration_endpoint`.
+			if (url === "https://provider.example/.well-known/oauth-authorization-server") {
+				return new Response(JSON.stringify({ issuer: "https://provider.example" }), {
+					status: 200,
+					headers: { "Content-Type": "application/json" },
+				});
+			}
+			if (url.startsWith("https://provider.example/authorize?")) {
+				return new Response("client_id is required", { status: 400 });
+			}
+			throw new Error(`Unexpected fetch: ${url}`);
+		};
+		const flow = new MCPOAuthFlow(
+			{
+				authorizationUrl: "https://provider.example/authorize",
+				tokenUrl: "https://provider.example/token",
+				fetch: fetchImpl,
+			},
+			{},
+		);
+
+		await expect(flow.generateAuthUrl("state", "http://127.0.0.1:53191/callback")).rejects.toThrow(
+			/no dynamic-client-registration endpoint was advertised.*oauth\.clientId/s,
+		);
+	});
+
 	it("accepts pasted redirect URLs through manual input", async () => {
 		let tokenRequestBody = "";
 		let manualAuthUrl = "";