From 9cc881ce5b3891a4e913a843e48dea341a0fc186 Mon Sep 17 00:00:00 2001 From: roboomp Date: Wed, 19 Aug 2026 09:34:51 +0000 Subject: [PATCH] fix(mcp): refresh broker-backed MCP OAuth credentials Remote OAuth MCP servers dropped out of /mcp under `omp auth-broker serve` once their access token expired: neither the client nor the broker could complete the refresh. - Client: the MCP manager threw on the broker-redacted refresh sentinel (REMOTE_REFRESH_SENTINEL) instead of asking the broker to refresh. It now routes redacted MCP refreshes through AuthStorage.forceRefreshCredentialById, which calls back to the broker (the real refresh token never leaves the broker host). - Broker: the serve process had no mcp_oauth:* refresh path, so POST /v1/credential/:id/refresh answered "Unknown OAuth provider". Its AuthStorage is now built with a refreshOAuthCredential override that refreshes MCP credentials with a generic refresh_token grant from the credential's embedded token endpoint and client id. The background refresher keeps MCP tokens live through the same path. Extract shared refreshManagedMcpOAuthCredential and mcpOAuthServerUrlFromCredentialId helpers so both paths use identical refresh material selection and RFC 8707 fallback-resource logic. Fixes #8933 --- packages/coding-agent/CHANGELOG.md | 1 + .../coding-agent/src/cli/auth-broker-cli.ts | 37 ++++- packages/coding-agent/src/mcp/manager.ts | 61 +++++--- .../coding-agent/src/mcp/oauth-credentials.ts | 38 +++++ packages/coding-agent/src/mcp/oauth-flow.ts | 21 +++ .../test/mcp-broker-oauth-refresh.test.ts | 133 ++++++++++++++++++ 6 files changed, 270 insertions(+), 21 deletions(-) create mode 100644 packages/coding-agent/test/mcp-broker-oauth-refresh.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index fdd5ae758..41997c0e6 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -12,6 +12,7 @@ ### Fixed +- Fixed broker-backed MCP OAuth credentials never refreshing, so remote OAuth MCP servers dropped out of `/mcp` once their access token expired under `omp auth-broker serve`. The client threw on the broker-redacted refresh sentinel instead of asking the broker to refresh, and the broker had no `mcp_oauth:*` refresh path (`POST /v1/credential/:id/refresh` answered `Unknown OAuth provider`). The client now routes redacted MCP refreshes through the broker, and the broker refreshes MCP credentials with a generic `refresh_token` grant from the credential's embedded token endpoint and client id — so the background refresher also keeps MCP tokens live ([#8933](https://github.com/can1357/oh-my-pi/issues/8933)). - Fixed Claude Code marketplace plugins ignoring the `enabledPlugins` switch in `~/.claude/settings.json` and `.claude/settings(.local).json`: a plugin turned off for a project no longer loads there, and a local-scope install enabled for a project loads even when its recorded `projectPath` is a different directory - Fixed task and eval subagents discovering newly added agent definitions while resolving their role aliases from stale startup settings. Subagent preflight now atomically reloads persisted settings before agent discovery while preserving live runtime overrides. - Fixed images returned by tools mounted under `xd://` rendering only as file links instead of inline terminal graphics. diff --git a/packages/coding-agent/src/cli/auth-broker-cli.ts b/packages/coding-agent/src/cli/auth-broker-cli.ts index f7b80602f..78b8b06f9 100644 --- a/packages/coding-agent/src/cli/auth-broker-cli.ts +++ b/packages/coding-agent/src/cli/auth-broker-cli.ts @@ -33,10 +33,14 @@ import { SqliteAuthCredentialStore, } from "@oh-my-pi/pi-ai"; import { AuthBrokerClient, DEFAULT_AUTH_BROKER_BIND, startAuthBroker } from "@oh-my-pi/pi-ai/auth-broker"; +import { refreshOAuthToken } from "@oh-my-pi/pi-ai/oauth"; +import type { OAuthCredentials } from "@oh-my-pi/pi-ai/oauth/types"; import { $which, APP_NAME, getAgentDbPath, getConfigRootDir, isEnoent, logger, VERSION } from "@oh-my-pi/pi-utils"; import chalk from "@oh-my-pi/pi-utils/chalk"; import { setTransports as setLoggerTransports } from "@oh-my-pi/pi-utils/logger"; import { $ } from "bun"; +import { refreshManagedMcpOAuthCredential } from "../mcp/oauth-credentials"; +import { isManagedMCPOAuthCredentialId, mcpOAuthServerUrlFromCredentialId } from "../mcp/oauth-flow"; import { resolveAuthBrokerConfig } from "../session/auth-broker-config"; export type AuthBrokerAction = "serve" | "token" | "login" | "logout" | "status" | "import" | "migrate" | "list"; @@ -119,6 +123,34 @@ async function ensureToken(): Promise { return token; } +/** + * OAuth refresh handler for `omp auth-broker serve`'s {@link AuthStorage}. + * + * The vault holds provider OAuth rows AND OMP-managed `mcp_oauth:*` rows. + * Provider rows refresh through the per-provider registry. MCP rows are + * self-describing — the embedded token endpoint and client credentials are the + * only refresh material — so they refresh with a generic `refresh_token` grant. + * The serve process never loads the MCP manager, so this is the only place that + * teaches the broker to refresh MCP tokens; without it + * `POST /v1/credential/:id/refresh` fails with "Unknown OAuth provider" and the + * background refresher lets MCP access tokens expire (issue #8933). + */ +export function refreshBrokerOAuthCredential( + provider: string, + credential: OAuthCredential, + signal?: AbortSignal, +): Promise { + if (isManagedMCPOAuthCredentialId(provider)) { + return refreshManagedMcpOAuthCredential(credential, { + serverUrl: mcpOAuthServerUrlFromCredentialId(provider), + signal, + }); + } + // Non-MCP rows: same per-provider path AuthStorage would take by default + // (the serve process registers no custom OAuth providers). + return refreshOAuthToken(provider as OAuthProvider, credential); +} + async function runServe(flags: AuthBrokerCommandArgs["flags"]): Promise { // The broker is a long-running headless service: route structured logs to // stdout so a process supervisor (pm2, journald, k8s) captures them, and @@ -129,7 +161,10 @@ async function runServe(flags: AuthBrokerCommandArgs["flags"]): Promise { const token = await ensureToken(); const dbPath = getAgentDbPath(); const store = await SqliteAuthCredentialStore.open(dbPath); - const storage = new AuthStorage(store); + const storage = new AuthStorage(store, { + refreshOAuthCredential: (provider, _credentialId, credential, signal) => + refreshBrokerOAuthCredential(provider, credential, signal), + }); await storage.reload(); const handle = startAuthBroker({ storage, diff --git a/packages/coding-agent/src/mcp/manager.ts b/packages/coding-agent/src/mcp/manager.ts index 8775464cb..7ccfed5f0 100644 --- a/packages/coding-agent/src/mcp/manager.ts +++ b/packages/coding-agent/src/mcp/manager.ts @@ -7,6 +7,7 @@ import * as path from "node:path"; import * as url from "node:url"; import { isDefinitiveOAuthFailure, type TSchema } from "@oh-my-pi/pi-ai"; +import type { OAuthCredentials } from "@oh-my-pi/pi-ai/oauth/types"; import { logger } from "@oh-my-pi/pi-utils"; import type { SourceMeta } from "../capability/types"; import { resolveConfigValue } from "../config/resolve-config-value"; @@ -30,9 +31,10 @@ import { type LoadMCPConfigsResult, loadAllMCPConfigs, validateServerConfig } fr import { lookupMcpOAuthCredential, type MCPOAuthCredentialLookup, + refreshManagedMcpOAuthCredential, selectMcpOAuthRefreshMaterial, } from "./oauth-credentials"; -import { type MCPStoredOAuthCredential, refreshMCPOAuthToken } from "./oauth-flow"; +import type { MCPStoredOAuthCredential } from "./oauth-flow"; import type { McpConnectionStatusEvent } from "./startup-events"; import type { MCPToolDetails } from "./tool-bridge"; import { DeferredMCPTool, MCPTool } from "./tool-bridge"; @@ -1404,6 +1406,36 @@ export class MCPManager { }; } + /** + * Refresh a broker-redacted MCP OAuth credential through the auth-broker. + * + * When running in broker mode the client only ever holds the redacted + * refresh sentinel; the real refresh token lives on the broker. Delegating + * to {@link AuthStorage.forceRefreshCredentialById} makes the broker run the + * `refresh_token` grant and return a fresh access token, which the client + * uses while keeping {@link REMOTE_REFRESH_SENTINEL} in the refresh slot. + */ + async #refreshBrokeredMcpCredential(credentialId: string, signal?: AbortSignal): Promise { + const storage = this.#authStorage; + if (!storage) throw new Error("MCP OAuth broker refresh requires an auth storage"); + const row = storage.listStoredCredentials(credentialId).find(entry => entry.credential.type === "oauth"); + if (!row) throw new Error(`No broker credential row for ${credentialId}`); + const entry = await storage.forceRefreshCredentialById(row.id, signal); + if (entry.credential.type !== "oauth") { + throw new Error(`Broker returned non-OAuth credential for ${credentialId}`); + } + const refreshed = entry.credential; + return { + access: refreshed.access, + refresh: REMOTE_REFRESH_SENTINEL, + expires: refreshed.expires, + accountId: refreshed.accountId, + email: refreshed.email, + projectId: refreshed.projectId, + enterpriseUrl: refreshed.enterpriseUrl, + }; + } + /** * Resolve OAuth credentials and shell commands in config. * `oauth: false` skips credential injection (reauth's unauthenticated probe); @@ -1435,24 +1467,15 @@ export class MCPManager { return Boolean(current.refresh && material?.tokenUrl); }, refresh: (current, signal) => { + // Broker-backed credentials redact the refresh token + // (REMOTE_REFRESH_SENTINEL); the broker holds the real one, so + // route the refresh through it instead of failing locally. if (current.refresh === REMOTE_REFRESH_SENTINEL) { - throw new Error("MCP OAuth refresh token is broker-redacted; local refresh is unavailable"); + return this.#refreshBrokeredMcpCredential(credentialId, signal); } - const material = selectMcpOAuthRefreshMaterial(current, auth); - const tokenUrl = material?.tokenUrl; - if (!current.refresh || !tokenUrl) { - throw new Error("MCP OAuth credential is missing refresh material"); - } - const clientId = material?.clientId; - const clientSecret = material?.clientSecret; - const authorizationUrl = - material && "authorizationUrl" in material ? material.authorizationUrl : undefined; - const resourceIsFallback = - !material?.resource && (config.type === "http" || config.type === "sse") && Boolean(config.url); - const resource = material?.resource ?? (resourceIsFallback ? config.url : undefined); - return refreshMCPOAuthToken(tokenUrl, current.refresh, clientId, clientSecret, resource, { - authorizationUrl, - stripSameOriginResource: resourceIsFallback, + return refreshManagedMcpOAuthCredential(current, { + serverUrl: config.type === "http" || config.type === "sse" ? config.url : undefined, + auth, signal, }); }, @@ -1480,10 +1503,8 @@ export class MCPManager { isDefinitiveOAuthFailure(error instanceof Error ? error.message : String(error)), disabledCause: error => `oauth refresh failed: ${error instanceof Error ? error.message : String(error)}`, - keepCredentialOnRefreshFailure: error => - !(error instanceof Error && error.message.includes("broker-redacted")), + keepCredentialOnRefreshFailure: true, onRefreshFailure: refreshError => { - if (refreshError instanceof Error && refreshError.message.includes("broker-redacted")) return; logger.warn("MCP OAuth refresh failed, using existing token", { credentialId, error: refreshError, diff --git a/packages/coding-agent/src/mcp/oauth-credentials.ts b/packages/coding-agent/src/mcp/oauth-credentials.ts index 2c78ca276..bcce60b1f 100644 --- a/packages/coding-agent/src/mcp/oauth-credentials.ts +++ b/packages/coding-agent/src/mcp/oauth-credentials.ts @@ -1,3 +1,4 @@ +import type { OAuthCredentials } from "@oh-my-pi/pi-ai/oauth/types"; import { getActiveProfile } from "@oh-my-pi/pi-utils/dirs"; import { expandEnvVarsDeep } from "../discovery/helpers"; import type { AuthStorage } from "../session/auth-storage"; @@ -6,6 +7,7 @@ import { type MCPStoredOAuthCredential, mcpOAuthCredentialId, mcpOAuthCredentialProfile, + refreshMCPOAuthToken, } from "./oauth-flow"; import type { MCPAuthConfig, MCPServerConfig } from "./types"; @@ -80,6 +82,42 @@ export function selectMcpOAuthRefreshMaterial( return credential.tokenUrl ? credential : auth; } +/** + * Refresh a stored MCP OAuth credential via the standard `refresh_token` grant. + * + * Refresh material is taken from the credential itself (self-contained modern + * credentials embed `tokenUrl`/`clientId`/`clientSecret`/`resource`) or, for + * legacy credentials that carry none, the server's `auth` block. Shared by the + * local MCP manager and the `omp auth-broker serve` refresh path so a broker + * with no access to the MCP config can still refresh `mcp_oauth:*` credentials + * from the vault. + * + * `serverUrl` supplies the RFC 8707 fallback resource indicator when neither + * the credential nor the auth block advertised one; the manager passes the + * configured server URL, the broker recovers it from the credential id via + * {@link mcpOAuthServerUrlFromCredentialId}. + * + * @throws when no usable refresh token or token endpoint is available. + */ +export function refreshManagedMcpOAuthCredential( + credential: MCPStoredOAuthCredential, + opts: { serverUrl?: string; auth?: MCPAuthConfig; signal?: AbortSignal } = {}, +): Promise { + const material = selectMcpOAuthRefreshMaterial(credential, opts.auth); + const tokenUrl = material?.tokenUrl; + if (!credential.refresh || !tokenUrl) { + throw new Error("MCP OAuth credential is missing refresh material"); + } + const authorizationUrl = material && "authorizationUrl" in material ? material.authorizationUrl : undefined; + const resourceIsFallback = !material?.resource && Boolean(opts.serverUrl); + const resource = material?.resource ?? (resourceIsFallback ? opts.serverUrl : undefined); + return refreshMCPOAuthToken(tokenUrl, credential.refresh, material?.clientId, material?.clientSecret, resource, { + authorizationUrl, + stripSameOriginResource: resourceIsFallback, + signal: opts.signal, + }); +} + export async function removeManagedMcpOAuthCredential( authStorage: AuthStorage, credentialId: string | undefined, diff --git a/packages/coding-agent/src/mcp/oauth-flow.ts b/packages/coding-agent/src/mcp/oauth-flow.ts index e312a395b..838361720 100644 --- a/packages/coding-agent/src/mcp/oauth-flow.ts +++ b/packages/coding-agent/src/mcp/oauth-flow.ts @@ -53,6 +53,27 @@ export function mcpOAuthCredentialProfile(credentialId: string): string | undefi return separator === -1 ? undefined : credentialId.slice(MCP_OAUTH_PROFILE_CREDENTIAL_PREFIX.length, separator); } +/** + * Server URL embedded in a managed MCP OAuth credential id, or `undefined` + * for legacy random ids (`mcp_oauth_`) minted before URL-keyed ids. + * + * Inverse of {@link mcpOAuthCredentialId}. Mirrors {@link mcpOAuthCredentialProfile}: + * the URL contains `:` and `/`, so for profile-scoped ids the URL is everything + * after the profile segment; for legacy url-keyed ids (`mcp_oauth:`) it is + * everything after the prefix. Lets the auth-broker — which never sees the MCP + * config — recover the server URL for the RFC 8707 fallback resource on refresh. + */ +export function mcpOAuthServerUrlFromCredentialId(credentialId: string): string | undefined { + if (credentialId.startsWith(MCP_OAUTH_PROFILE_CREDENTIAL_PREFIX)) { + const separator = credentialId.indexOf(":", MCP_OAUTH_PROFILE_CREDENTIAL_PREFIX.length); + return separator === -1 ? undefined : credentialId.slice(separator + 1) || undefined; + } + if (credentialId.startsWith(MCP_OAUTH_URL_CREDENTIAL_PREFIX)) { + return credentialId.slice(MCP_OAUTH_URL_CREDENTIAL_PREFIX.length) || undefined; + } + return undefined; +} + /** * Stored MCP OAuth credential. Refresh material is embedded so token refresh * works without any `auth` block persisted in (possibly shared) config files. diff --git a/packages/coding-agent/test/mcp-broker-oauth-refresh.test.ts b/packages/coding-agent/test/mcp-broker-oauth-refresh.test.ts new file mode 100644 index 000000000..b7c3a76b8 --- /dev/null +++ b/packages/coding-agent/test/mcp-broker-oauth-refresh.test.ts @@ -0,0 +1,133 @@ +/** + * End-to-end regression for broker-backed MCP OAuth refresh (issue #8933). + * + * Topology mirrors `omp auth-broker serve` fronting a sandboxed client: + * client (RemoteAuthCredentialStore) → broker (SqliteAuthCredentialStore + * + refreshBrokerOAuthCredential override) → MCP token endpoint. + * + * Two gaps used to break this once the ~6h access token expired: + * A. the client threw on the `__remote__` refresh sentinel instead of asking + * the broker to refresh (mcp/manager.ts); + * B. the broker had no `mcp_oauth:*` refresh path, so it answered + * `Unknown OAuth provider` (auth-broker-cli.ts / auth-storage.ts). + * + * The test proves the fixed contract: a remote OAuth MCP server whose access + * token has expired refreshes through the broker (which holds the only real + * refresh token) and the client injects the freshly minted Bearer. + */ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { AuthStorage, type OAuthCredential, REMOTE_REFRESH_SENTINEL, SqliteAuthCredentialStore } from "@oh-my-pi/pi-ai"; +import { + AuthBrokerClient, + type AuthBrokerServerHandle, + RemoteAuthCredentialStore, + startAuthBroker, +} from "@oh-my-pi/pi-ai/auth-broker"; +import { refreshBrokerOAuthCredential } from "@oh-my-pi/pi-coding-agent/cli/auth-broker-cli"; +import { MCPManager } from "@oh-my-pi/pi-coding-agent/mcp/manager"; +import { mcpOAuthCredentialId } from "@oh-my-pi/pi-coding-agent/mcp/oauth-flow"; +import type { MCPServerConfig } from "@oh-my-pi/pi-coding-agent/mcp/types"; +import { removeWithRetries } from "@oh-my-pi/pi-utils"; +import type { Server } from "bun"; + +const SERVER_URL = "https://mcp.granola.ai/mcp"; +const MCP_PROVIDER = mcpOAuthCredentialId(SERVER_URL, "default"); +const BEARER = "e2e-broker-mcp-token"; + +function getAuthorizationHeader(config: MCPServerConfig): string | undefined { + if (config.type !== "http" && config.type !== "sse") return undefined; + return config.headers?.Authorization; +} + +describe("broker-backed MCP OAuth refresh", () => { + let tempDir = ""; + let tokenServer: Server | undefined; + let tokenRequests: URLSearchParams[] = []; + let serverStore: SqliteAuthCredentialStore | undefined; + let serverStorage: AuthStorage | undefined; + let handle: AuthBrokerServerHandle | undefined; + let remote: RemoteAuthCredentialStore | undefined; + let clientStorage: AuthStorage | undefined; + let manager: MCPManager | undefined; + + beforeEach(async () => { + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-broker-mcp-refresh-")); + tokenRequests = []; + const server = Bun.serve({ + port: 0, + async fetch(req) { + tokenRequests.push(new URLSearchParams(await req.text())); + return Response.json({ access_token: "fresh-access", refresh_token: "rotated-refresh", expires_in: 3600 }); + }, + }); + tokenServer = server; + const tokenUrl = `http://127.0.0.1:${server.port}/token`; + + serverStore = await SqliteAuthCredentialStore.open(path.join(tempDir, "broker.db")); + // The serve process constructs AuthStorage with this exact override. + serverStorage = new AuthStorage(serverStore, { + refreshOAuthCredential: (provider, _credentialId, credential, signal) => + refreshBrokerOAuthCredential(provider, credential, signal), + }); + await serverStorage.reload(); + + // Expired MCP OAuth credential with embedded refresh material, as the + // vault holds it. Spread bypasses the excess-property check for the + // MCP-only extension fields the base OAuthCredential type omits. + const credential: OAuthCredential = { + ...{ type: "oauth", access: "stale-access", refresh: "real-refresh-token", expires: Date.now() - 60_000 }, + ...{ tokenUrl, clientId: "client-xyz" }, + }; + await serverStorage.set(MCP_PROVIDER, credential); + + handle = startAuthBroker({ + storage: serverStorage, + bind: "127.0.0.1:0", + bearerTokens: [BEARER], + disableRefresher: true, + }); + + remote = new RemoteAuthCredentialStore({ + client: new AuthBrokerClient({ url: handle.url, token: BEARER }), + streamSnapshots: false, + }); + clientStorage = new AuthStorage(remote); + await clientStorage.revalidateCredentials(); + + manager = new MCPManager(process.cwd()); + manager.setAuthStorage(clientStorage); + }); + + afterEach(async () => { + clientStorage?.close(); + await handle?.close(); + serverStorage?.close(); + serverStore?.close(); + tokenServer?.stop(true); + await removeWithRetries(tempDir); + }); + + test("expired remote MCP token refreshes through the broker and injects the fresh Bearer", async () => { + // Sanity: the client only ever sees the redacted refresh token. + const stored = clientStorage!.get(MCP_PROVIDER); + expect(stored?.type === "oauth" ? stored.refresh : undefined).toBe(REMOTE_REFRESH_SENTINEL); + + const prepared = await manager!.prepareConfig({ + type: "http", + url: SERVER_URL, + auth: { type: "oauth", credentialId: MCP_PROVIDER }, + }); + + // Gap A + B fixed: fresh access token minted and injected. + expect(getAuthorizationHeader(prepared)).toBe("Bearer fresh-access"); + + // The grant ran on the BROKER with the real refresh token — the client + // never held it, and the broker no longer answers "Unknown OAuth provider". + expect(tokenRequests).toHaveLength(1); + expect(tokenRequests[0].get("grant_type")).toBe("refresh_token"); + expect(tokenRequests[0].get("refresh_token")).toBe("real-refresh-token"); + }); +});