From c6a057073b76ee359baaa4da2b7526e746e45dbc Mon Sep 17 00:00:00 2001 From: roboomp Date: Mon, 3 Aug 2026 00:32:47 +0000 Subject: [PATCH] fix(mcp): expand env vars in reauth oauth credentials and reject empty tokens /mcp reauth read OAuth clientId/clientSecret from the raw, unexpanded config while URL and resource used expandEnvVarsDeep, so `${VAR}` placeholders were sent literally to the token exchange. MCPOAuthFlow.exchangeToken() also accepted any HTTP-success body, storing an empty access token when a provider signals failure with HTTP 200 (e.g. Slack `{ ok: false, error }`), surfacing only later as invalid_token. - Select flow client credentials from runtimeBaseConfig / expanded auth block; keep the raw placeholder for the persisted config file. - Reject token responses without a non-empty access_token, including the sanitized provider error when present. - Add regression tests for env-expanded reauth credentials and HTTP-200 token error bodies. Fixes #7440 --- packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/src/mcp/oauth-flow.ts | 12 +++- .../controllers/mcp-command-controller.ts | 16 +++-- .../test/mcp-command-reauth.test.ts | 64 +++++++++++++++++++ packages/coding-agent/test/oauth-flow.test.ts | 38 +++++++++++ 5 files changed, 125 insertions(+), 6 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 6058ffc56..185597bd7 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -24,6 +24,7 @@ ### Fixed +- Fixed `/mcp reauth` sending literal `${VAR}` placeholders instead of env-expanded OAuth client credentials during the token exchange, and `MCPOAuthFlow.exchangeToken()` accepting an HTTP-200 token response with no `access_token` (e.g. a Slack `{ ok: false, error }` body), which stored an empty access token and only surfaced `invalid_token` on a later MCP request ([#7440](https://github.com/can1357/oh-my-pi/issues/7440)). - Fixed template argument substitution (`substituteArgs`) executing recursive placeholder expansion when positional argument values contain literal `$@` or `$ARGUMENTS` tokens. - Fixed focused-agent status bar dimming darkening Powerline end caps. - Fixed the browser relay creating duplicate "omp" tab groups: the bridge now keeps at most one group RPC in flight (a queued drain replaces fire-and-forget per-tab requests), so concurrent requests can no longer race the extension's non-atomic query→create→set-title sequence in the same window. Also fixed an extension reconnect (relay daemon restart, service-worker recycle) being misread as the user dragging every tab out of the omp group — grouping state is reset when the extension socket closes, so tabs regroup on the next hello instead of being permanently opted out. diff --git a/packages/coding-agent/src/mcp/oauth-flow.ts b/packages/coding-agent/src/mcp/oauth-flow.ts index 9a30d4c44..e312a395b 100644 --- a/packages/coding-agent/src/mcp/oauth-flow.ts +++ b/packages/coding-agent/src/mcp/oauth-flow.ts @@ -489,12 +489,22 @@ export class MCPOAuthFlow extends OAuthCallbackFlow { } const data = (await response.json()) as { - access_token: string; + access_token?: string; refresh_token?: string; expires_in?: number; token_type?: string; + error?: string; + error_description?: string; }; + // Some providers (e.g. the Slack Web API) signal failure with HTTP 200 and + // an `{ ok: false, error }` body. Accepting such a response would store an + // empty access token and only surface `invalid_token` on a later request. + if (typeof data.access_token !== "string" || data.access_token.length === 0) { + const providerError = data.error_description ?? data.error; + throw new Error(`Token exchange returned no access token${providerError ? `: ${providerError}` : ""}`); + } + // Calculate expiry timestamp const expiresIn = data.expires_in ?? 3600; // Default to 1 hour const expires = Date.now() + expiresIn * 1000; diff --git a/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts b/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts index 317352089..fa5e1f5d1 100644 --- a/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts +++ b/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts @@ -1793,16 +1793,22 @@ export class MCPCommandController { const oauth = await this.#resolveOAuthEndpointsFromServer(runtimeBaseConfig, options.authChallenge); const serverUrl = runtimeBaseConfig.type === "http" || runtimeBaseConfig.type === "sse" ? runtimeBaseConfig.url : undefined; - // A user-supplied client secret may live in either block (the wizard - // writes it to auth.clientSecret); DCR secrets are embedded in the - // stored credential and never echoed back into config files. - const configuredClientId = found.config.oauth?.clientId ?? currentAuth?.clientId; + // Client credentials drive the token exchange, so they must come from the + // env-expanded runtime config; `found.config`/`currentAuth` may still hold + // `${...}` placeholders (the wizard writes the secret to auth.clientSecret). + // DCR secrets are embedded in the stored credential and never echoed back + // into config files. + const runtimeAuth = currentAuth ? expandEnvVarsDeep(currentAuth) : undefined; + const configuredClientId = runtimeBaseConfig.oauth?.clientId ?? runtimeAuth?.clientId; const existingCredential = lookupMcpOAuthCredentialForServer(authStorage, currentAuth, serverUrl)?.credential; const flowClientId = oauth.clientId ?? configuredClientId ?? existingCredential?.clientId ?? ""; const storedClientSecret = existingCredential?.clientId === flowClientId ? existingCredential.clientSecret : undefined; + const flowClientSecret = + runtimeBaseConfig.oauth?.clientSecret ?? runtimeAuth?.clientSecret ?? storedClientSecret ?? ""; + // Persisted separately below: keep the raw `${...}` placeholder in the file + // rather than writing the resolved secret back to (possibly shared) config. const userClientSecret = found.config.oauth?.clientSecret ?? currentAuth?.clientSecret; - const flowClientSecret = userClientSecret ?? storedClientSecret ?? ""; if (!options.silent) { this.#showMessage(["", theme.fg("muted", `Reauthorizing "${name}"...`), ""].join("\n")); diff --git a/packages/coding-agent/test/mcp-command-reauth.test.ts b/packages/coding-agent/test/mcp-command-reauth.test.ts index da63a69d3..ee5526684 100644 --- a/packages/coding-agent/test/mcp-command-reauth.test.ts +++ b/packages/coding-agent/test/mcp-command-reauth.test.ts @@ -557,4 +557,68 @@ describe("/mcp auth commands", () => { ) as TestConfigFile; expect(userConfig.mcpServers?.discovered).toBeUndefined(); }); + + test("passes env-expanded OAuth client credentials to the reauth flow", async () => { + const authStorage = freshAuthStorage(); + await authStorage.reload(); + Bun.env.MCP_OAUTH_CLIENT_ID = "expanded-client-id"; + Bun.env.MCP_OAUTH_CLIENT_SECRET = "expanded-client-secret"; + await Bun.write( + configPath, + `${JSON.stringify( + { + mcpServers: { + envserver: { + type: "http", + url: RAW_SERVER_URL, + oauth: { + clientId: "${MCP_OAUTH_CLIENT_ID}", + clientSecret: "${MCP_OAUTH_CLIENT_SECRET}", + }, + }, + }, + }, + null, + 2, + )}\n`, + ); + try { + vi.spyOn(mcpClient, "connectToServer").mockRejectedValue(AUTH_ERROR); + let flowClientId: string | undefined; + let flowClientSecret: string | undefined; + vi.spyOn(oauthFlow.MCPOAuthFlow.prototype, "login").mockImplementation(async function ( + this: oauthFlow.MCPOAuthFlow, + ) { + // MCPOAuthFlow keeps its config private; read it back to assert the + // resolved credentials the flow will use. Structurally known shape, + // no runtime validation is meaningful here. + const flow = this as unknown as { config: { clientId?: string; clientSecret?: string } }; + flowClientId = flow.config.clientId; + flowClientSecret = flow.config.clientSecret; + return { + access: "fresh-access", + refresh: "fresh-refresh", + expires: Date.now() + 3_600_000, + }; + }); + const { controller, showError } = createController(authStorage); + + await controller.handle("/mcp reauth envserver"); + + expect(showError).not.toHaveBeenCalled(); + // The token exchange must receive the resolved secret, not the literal + // `${...}` placeholder. + expect(flowClientId).toBe("expanded-client-id"); + expect(flowClientSecret).toBe("expanded-client-secret"); + + // The config file keeps the placeholder; only the flow sees the value. + const saved = JSON.parse(await Bun.file(configPath).text()) as TestConfigFile; + const savedServer = saved.mcpServers?.envserver; + expect(savedServer?.oauth?.clientSecret).toBe("${MCP_OAUTH_CLIENT_SECRET}"); + expect(savedServer?.oauth?.clientId).toBe("${MCP_OAUTH_CLIENT_ID}"); + } finally { + delete Bun.env.MCP_OAUTH_CLIENT_ID; + delete Bun.env.MCP_OAUTH_CLIENT_SECRET; + } + }); }); diff --git a/packages/coding-agent/test/oauth-flow.test.ts b/packages/coding-agent/test/oauth-flow.test.ts index 07fe97f0f..23225bbf9 100644 --- a/packages/coding-agent/test/oauth-flow.test.ts +++ b/packages/coding-agent/test/oauth-flow.test.ts @@ -344,6 +344,44 @@ describe("mcp oauth flow", () => { }); }); + it("rejects an HTTP-200 token response that carries no access token", async () => { + let observedRedirectUri = ""; + const flow = new MCPOAuthFlow( + { + authorizationUrl: "https://provider.example/authorize", + tokenUrl: "https://provider.example/token", + clientId: "client-id", + clientSecret: "client-secret", + callbackPort: 14569, + fetch: async input => { + const url = String(input); + if (url === "https://provider.example/token") { + // Slack Web API error shape: HTTP 200 with `ok:false` and no + // `access_token`. Accepting it would store an empty credential. + return new Response(JSON.stringify({ ok: false, error: "bad_client_secret" }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); + } + throw new Error(`Unexpected fetch: ${url}`); + }, + }, + { + onAuth: info => { + const authUrl = new URL(info.url); + observedRedirectUri = authUrl.searchParams.get("redirect_uri") ?? ""; + const state = authUrl.searchParams.get("state") ?? ""; + queueMicrotask(() => { + void completeLocalOAuthCallback(`${observedRedirectUri}?code=test-code&state=${state}`); + }); + }, + signal: AbortSignal.timeout(1_000), + }, + ); + + await expect(flow.login()).rejects.toThrow(/bad_client_secret/); + }); + it("preserves root redirectUri values without adding a trailing slash", async () => { let observedRedirectUri = ""; let tokenRequestBody = "";