Merge pull request #1061 from ldx/fix/mcp-oauth-persist-dynamic-client

fix(coding-agent): persist dynamically registered MCP OAuth client_id
2026-05-14 04:43:08 +02:00
parent 38255a4e6e a7f73ee645
commit 9530f94a36
5 changed files with 165 additions and 15 deletions
@@ -20,6 +20,10 @@
 - Changed search truncation metadata/renderer output from match/result-based limits to file-based limits (`fileLimitReached`, `perFileLimitReached`) and updated truncation labels accordingly
 - Lowered `read.defaultLimit` default from `500` to `300` lines, and split the per-range context padding into asymmetric `RANGE_LEADING_CONTEXT_LINES = 1` / `RANGE_TRAILING_CONTEXT_LINES = 3` (was symmetric `RANGE_CONTEXT_LINES = 3`). Replay analysis over post-summarizer sessions (`scripts/session-stats/optimize_read_config.py`) showed that bare-path reads are over-provisioned at the median (file p50 = 220 lines) and that most follow-up reads are disjoint hops rather than adjacent extensions — so a smaller default plus narrower leading context reclaims tokens without measurably changing first-cover rate. Trailing context stays at 3 lines to keep anchor-stale recovery on narrow reads. Explicit `read.defaultLimit` overrides in settings are honoured unchanged.

+### Fixed
+
+- Fixed MCP OAuth refresh failing with `HTTP 401 invalid_client` for servers that require Dynamic Client Registration (RFC 7591) and have no `oauth.clientId` configured (e.g. `mcp.linear.app`). `MCPOAuthFlow` registered a fresh public PKCE client on each authorize and discarded the issued `client_id` once the flow object went out of scope; refresh then called the provider's `/token` endpoint without a `client_id`. The flow now exposes `resolvedClientId` / `registeredClientSecret` getters, `MCPCommandController#handleOAuthFlow` returns them alongside `credentialId`, and both the initial-connect and `/mcp reauth` paths persist them into `auth.{clientId,clientSecret}` (used at refresh) and `oauth.{clientId,clientSecret}` (used by subsequent `/mcp reauth` to skip re-registration). The `MCPAddWizard` `onOAuth` callback type is now `Promise<MCPAddWizardOAuthResult>` and `#launchOAuthFlow` folds the registered credentials into wizard state. Servers with a statically-configured `oauth.clientId` (Notion, Slack, Datadog) are unaffected — `#tryRegisterClient` short-circuits and the write-back is a no-op. ([#1061](https://github.com/can1357/oh-my-pi/pull/1061) by [@ldx](https://github.com/ldx)).
+
 ## [15.0.0] - 2026-05-13
 ### Breaking Changes

@@ -133,6 +133,26 @@ export class MCPOAuthFlow extends OAuthCallbackFlow {
 		this.#resolvedClientId = this.#resolveClientId(config);
 	}

+	/**
+	 * Client id used during the authorization request. Returns the value supplied
+	 * via {@link MCPOAuthConfig.clientId} or, when the server required dynamic
+	 * client registration, the id issued during registration. `undefined` until
+	 * {@link generateAuthUrl} (or {@link login}) has run for a server that needs
+	 * a client id.
+	 */
+	get resolvedClientId(): string | undefined {
+		return this.#resolvedClientId;
+	}
+
+	/**
+	 * Client secret issued by dynamic client registration, if any. Always
+	 * `undefined` for PKCE-only/public clients and when the caller supplies the
+	 * client id via config.
+	 */
+	get registeredClientSecret(): string | undefined {
+		return this.#registeredClientSecret;
+	}
+
 	async generateAuthUrl(state: string, redirectUri: string): Promise<{ url: string; instructions?: string }> {
 		if (!this.#resolvedClientId) {
 			await this.#tryRegisterClient(redirectUri);
@@ -47,6 +47,18 @@ type WizardStep =
 	| "scope"
 	| "confirm";

+/**
+ * Result of the wizard's OAuth callback. `credentialId` is mandatory;
+ * `clientId`/`clientSecret` are populated when the OAuth provider performed
+ * dynamic client registration (or when the caller pre-supplied them) so the
+ * wizard can fold them into the final `mcp.json` entry for refresh.
+ */
+export interface MCPAddWizardOAuthResult {
+	credentialId: string;
+	clientId?: string;
+	clientSecret?: string;
+}
+
 interface WizardState {
 	name: string;
 	transport: TransportType | null;
@@ -104,7 +116,13 @@ export class MCPAddWizard extends Container {
 	#onCompleteCallback: (name: string, config: MCPServerConfig, scope: Scope) => void;
 	#onCancelCallback: () => void;
 	#onOAuthCallback:
-		| ((authUrl: string, tokenUrl: string, clientId: string, clientSecret: string, scopes: string) => Promise<string>)
+		| ((
+				authUrl: string,
+				tokenUrl: string,
+				clientId: string,
+				clientSecret: string,
+				scopes: string,
+		  ) => Promise<MCPAddWizardOAuthResult>)
 		| null = null;
 	#onTestConnectionCallback: ((config: MCPServerConfig) => Promise<void>) | null = null;
 	#onRenderCallback: (() => void) | null = null;
@@ -118,7 +136,7 @@ export class MCPAddWizard extends Container {
 			clientId: string,
 			clientSecret: string,
 			scopes: string,
-		) => Promise<string>,
+		) => Promise<MCPAddWizardOAuthResult>,
 		onTestConnection?: (config: MCPServerConfig) => Promise<void>,
 		onRender?: () => void,
 		initialName?: string,
@@ -1120,7 +1138,7 @@ export class MCPAddWizard extends Container {

 		try {
 			// Call OAuth handler
-			const credentialId = await this.#onOAuthCallback(
+			const oauthResult = await this.#onOAuthCallback(
 				this.#state.oauthAuthUrl,
 				this.#state.oauthTokenUrl,
 				this.#state.oauthClientId,
@@ -1128,8 +1146,11 @@ export class MCPAddWizard extends Container {
 				this.#state.oauthScopes,
 			);

-			// Store credential ID
-			this.#state.oauthCredentialId = credentialId;
+			// Store credential ID + any dynamically-registered client credentials,
+			// so the final mcp.json entry persists everything needed for refresh.
+			this.#state.oauthCredentialId = oauthResult.credentialId;
+			if (oauthResult.clientId) this.#state.oauthClientId = oauthResult.clientId;
+			if (oauthResult.clientSecret) this.#state.oauthClientSecret = oauthResult.clientSecret;

 			// Show success message
 			this.#contentContainer.clear();
@@ -49,6 +49,22 @@ function withTimeout<T>(promise: Promise<T>, timeoutMs: number, message: string)
 	return Promise.race([promise, timeoutPromise]).finally(() => clearTimeout(timer));
 }

+/**
+ * Outcome of {@link MCPCommandController}'s OAuth handler.
+ *
+ * `clientId`/`clientSecret` are populated when the OAuth provider required (or
+ * accepted) dynamic client registration; callers MUST persist them alongside
+ * `credentialId` so subsequent token refreshes and reauthorizations can reuse
+ * the same registered client. Both are also set when the caller pre-supplied a
+ * client id via the wizard or `oauth.clientId` in `mcp.json`, in which case the
+ * write-back is a no-op.
+ */
+interface OAuthFlowResult {
+	credentialId: string;
+	clientId?: string;
+	clientSecret?: string;
+}
+
 type MCPAddScope = "user" | "project";
 type MCPAddTransport = "http" | "sse";

@@ -406,7 +422,7 @@ export class MCPCommandController {

 						try {
 							const oauthClientSecret = finalConfig.oauth?.clientSecret ?? "";
-							const credentialId = await this.#handleOAuthFlow(
+							const oauthResult = await this.#handleOAuthFlow(
 								oauth.authorizationUrl,
 								oauth.tokenUrl,
 								oauth.clientId ?? finalConfig.oauth?.clientId ?? "",
@@ -416,14 +432,21 @@ export class MCPCommandController {
 								finalConfig.oauth?.callbackPath,
 								finalConfig.oauth?.redirectUri,
 							);
+							const persistedClientId = oauthResult.clientId ?? oauth.clientId ?? finalConfig.oauth?.clientId;
+							const persistedClientSecret = oauthResult.clientSecret ?? finalConfig.oauth?.clientSecret;
 							finalConfig = {
 								...finalConfig,
 								auth: {
 									type: "oauth",
-									credentialId,
+									credentialId: oauthResult.credentialId,
 									tokenUrl: oauth.tokenUrl,
-									clientId: oauth.clientId ?? finalConfig.oauth?.clientId,
-									clientSecret: finalConfig.oauth?.clientSecret,
+									clientId: persistedClientId,
+									clientSecret: persistedClientSecret,
+								},
+								oauth: {
+									...finalConfig.oauth,
+									clientId: persistedClientId ?? finalConfig.oauth?.clientId,
+									clientSecret: persistedClientSecret ?? finalConfig.oauth?.clientSecret,
 								},
 							};
 						} catch (oauthError) {
@@ -488,7 +511,7 @@ export class MCPCommandController {
 		callbackPort?: number,
 		callbackPath?: string,
 		redirectUri?: string,
-	): Promise<string> {
+	): Promise<OAuthFlowResult> {
 		const authStorage = this.ctx.session.modelRegistry.authStorage;
 		let parsedAuthUrl: URL;

@@ -600,7 +623,11 @@ export class MCPCommandController {
 			// Store under a synthetic provider name
 			await authStorage.set(credentialId, oauthCredential);

-			return credentialId;
+			return {
+				credentialId,
+				clientId: flow.resolvedClientId,
+				clientSecret: flow.registeredClientSecret,
+			};
 		} catch (error) {
 			const errorMsg = error instanceof Error ? error.message : String(error);

@@ -1312,7 +1339,7 @@ export class MCPCommandController {

 			this.#showMessage(["", theme.fg("muted", `Reauthorizing "${name}"...`), ""].join("\n"));

-			const credentialId = await this.#handleOAuthFlow(
+			const oauthResult = await this.#handleOAuthFlow(
 				oauth.authorizationUrl,
 				oauth.tokenUrl,
 				oauth.clientId ?? found.config.oauth?.clientId ?? "",
@@ -1323,14 +1350,22 @@ export class MCPCommandController {
 				found.config.oauth?.redirectUri,
 			);

+			const persistedClientId = oauthResult.clientId ?? oauth.clientId ?? found.config.oauth?.clientId;
+			const persistedClientSecret = oauthResult.clientSecret ?? (oauthClientSecret || undefined);
+
 			const updated: MCPServerConfig = {
 				...baseConfig,
 				auth: {
 					type: "oauth",
-					credentialId,
+					credentialId: oauthResult.credentialId,
 					tokenUrl: oauth.tokenUrl,
-					clientId: oauth.clientId ?? found.config.oauth?.clientId,
-					clientSecret: oauthClientSecret || undefined,
+					clientId: persistedClientId,
+					clientSecret: persistedClientSecret,
+				},
+				oauth: {
+					...found.config.oauth,
+					clientId: persistedClientId ?? found.config.oauth?.clientId,
+					clientSecret: persistedClientSecret ?? found.config.oauth?.clientSecret,
 				},
 			};
 			await updateMCPServer(found.filePath, name, updated);
@@ -309,4 +309,74 @@ describe("mcp oauth flow", () => {

 		await expect(flow.login()).rejects.toThrow("cannot fall back to a random port when oauth.redirectUri is set");
 	});
+
+	it("exposes the dynamically registered client_id and client_secret after generateAuthUrl", async () => {
+		using _hook = hookFetch(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(
+					JSON.stringify({
+						client_id: "registered-client-id",
+						client_secret: "registered-client-secret",
+					}),
+					{ status: 200, headers: { "Content-Type": "application/json" } },
+				);
+			}
+			return new Response("not found", { status: 404 });
+		});
+
+		const flow = new MCPOAuthFlow(
+			{
+				authorizationUrl: "https://www.figma.com/oauth/mcp",
+				tokenUrl: "https://api.figma.com/v1/oauth/token",
+			},
+			{},
+		);
+
+		expect(flow.resolvedClientId).toBeUndefined();
+		expect(flow.registeredClientSecret).toBeUndefined();
+
+		await flow.generateAuthUrl("test-state", "http://127.0.0.1:53173/callback");
+
+		expect(flow.resolvedClientId).toBe("registered-client-id");
+		expect(flow.registeredClientSecret).toBe("registered-client-secret");
+	});
+
+	it("returns the configured client_id from resolvedClientId without triggering registration", async () => {
+		let registrationCalled = false;
+		using _hook = hookFetch(input => {
+			const url = String(input);
+			if (url.includes("/.well-known/")) {
+				return new Response("{}", { status: 200, headers: { "Content-Type": "application/json" } });
+			}
+			if (url.endsWith("/register")) {
+				registrationCalled = true;
+			}
+			return new Response("not found", { status: 404 });
+		});
+
+		const flow = new MCPOAuthFlow(
+			{
+				authorizationUrl: "https://provider.example/authorize",
+				tokenUrl: "https://provider.example/token",
+				clientId: "configured-client-id",
+			},
+			{},
+		);
+
+		expect(flow.resolvedClientId).toBe("configured-client-id");
+		expect(flow.registeredClientSecret).toBeUndefined();
+
+		await flow.generateAuthUrl("test-state", "http://127.0.0.1:53174/callback");
+
+		expect(flow.resolvedClientId).toBe("configured-client-id");
+		expect(flow.registeredClientSecret).toBeUndefined();
+		expect(registrationCalled).toBe(false);
+	});
 });