diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 64c3c57ae..db6f2b482 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -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 diff --git a/packages/coding-agent/src/mcp/oauth-flow.ts b/packages/coding-agent/src/mcp/oauth-flow.ts index db8d195f6..36e6f9bf7 100644 --- a/packages/coding-agent/src/mcp/oauth-flow.ts +++ b/packages/coding-agent/src/mcp/oauth-flow.ts @@ -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 { + 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 { 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}`, + ); + } } /** diff --git a/packages/coding-agent/test/oauth-flow.test.ts b/packages/coding-agent/test/oauth-flow.test.ts index 4efcf4b00..d022e3ad4 100644 --- a/packages/coding-agent/test/oauth-flow.test.ts +++ b/packages/coding-agent/test/oauth-flow.test.ts @@ -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 = "";