fix(coding-agent): persist dynamically registered MCP OAuth client_id

When an MCP server uses OAuth Dynamic Client Registration (RFC 7591) and
no client_id is pre-configured, MCPOAuthFlow registers a fresh public
PKCE client on each authorize, captures the issued client_id into a
private field, then discards it once the flow object goes out of scope.
At refresh time, MCPManager#resolveAuthConfig calls refreshMCPOAuthToken
with auth.clientId from mcp.json — which is empty for these servers —
so providers that require client_id on the refresh grant (e.g. Linear at
mcp.linear.app/token) reject with HTTP 401 invalid_client. The user is
forced to /mcp reauth manually every time the access token expires.

This change threads the resolved/registered client credentials back out
of the OAuth flow and persists them into mcp.json so refresh has what
it needs indefinitely:

- MCPOAuthFlow exposes resolvedClientId / registeredClientSecret getters.
- MCPCommandController#handleOAuthFlow returns OAuthFlowResult with
  credentialId + clientId + clientSecret, populated from the flow's
  post-login state.
- The initial-connect non-wizard path and /mcp reauth path persist the
  returned client credentials into both auth.{clientId,clientSecret}
  (used at refresh) and oauth.{clientId,clientSecret} (used by future
  /mcp reauth to skip re-registration).
- The wizard's onOAuth callback signature now returns the same shape;
  #launchOAuthFlow folds the registered credentials into wizard state so
  the final mcp.json entry built by #buildServerConfigWithAuth includes
  them under auth.{clientId,clientSecret}.

Servers that configure a static oauth.clientId in mcp.json (Notion,
Slack, Datadog) are unaffected: #tryRegisterClient short-circuits, the
returned clientId equals the configured one, and the write-back is a
no-op.

Adds two MCPOAuthFlow unit tests covering both paths.
This commit is contained in:
Vilmos Nebehaj
2026-05-13 14:21:47 -07:00
parent 453071d34d
commit a7f73ee645
5 changed files with 165 additions and 15 deletions
+4
View File
@@ -19,6 +19,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);
@@ -1348,7 +1375,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 ?? "",
@@ -1359,14 +1386,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);
});
});