diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 048971022..06bd23556 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -13,7 +13,7 @@ - Fixed ollama-cloud task/subagent fan-out exceeding the provider's three-request concurrency cap by adding a provider-specific subagent limiter, and let configured task/smol/advisor model roles inherit the default retry fallback chain when they do not define their own chain. ([#3464](https://github.com/can1357/oh-my-pi/issues/3464)) - Fixed the per-provider subagent concurrency limiter (e.g. `providers.ollama-cloud.maxConcurrency`) being replaced with a fresh semaphore whenever the configured limit changed, which orphaned the in-flight slots on the old instance and let a runtime or mixed limit value exceed the cap. The limiter now resizes a single shared semaphore in place — raising the ceiling admits queued waiters immediately, lowering it drains in-flight holders without admitting past the new cap. ([#3464](https://github.com/can1357/oh-my-pi/issues/3464)) - Fixed a background-task spawn slot leaking from the `task.maxConcurrency` limiter when progress reporting threw between acquiring the slot and entering the guarded run: `markRunning`/`reportProgress` now run inside the try whose `finally` releases the semaphore, so a failed progress report can no longer permanently shrink subagent concurrency. ([#3464](https://github.com/can1357/oh-my-pi/issues/3464)) -- Fixed MCP OAuth authorization failing with `Authorization failed: An unexpected error occurred` against authorization servers (Plane is the live example) that reject same-origin `resource` indicators, including path-bearing MCP endpoint URLs such as `https://mcp.plane.so/http/mcp`. Per RFC 8707 §2 the resource indicator distinguishes *other* resource servers from the authorization server, so OMP's `MCPOAuthFlow` now drops same-origin resource values across the authorize URL, the matching token-exchange request, **and** the refresh-token request — filtered against the authorization-server origin (persisted on the credential as `authorizationUrl`), with `tokenUrl`'s origin as the legacy fallback so the filter stays correct even when RFC 8414's optional authorize/token-endpoint origin split is in play. ([#3502](https://github.com/can1357/oh-my-pi/issues/3502)) +- Fixed MCP OAuth authorization failing with `Authorization failed: An unexpected error occurred` against authorization servers (Plane is the live example) that reject redundant `resource` indicators. OMP now drops exact authorization-server-origin resources for every MCP OAuth flow, and also drops same-origin path resources only when OMP synthesized them from the server URL fallback (e.g. `https://mcp.plane.so/http/mcp`). Provider-advertised path-scoped resources from protected-resource discovery are preserved so gateway-hosted MCP services can still request the audience they advertised. The refresh-token path uses the same policy, filtered against the authorization-server origin persisted on the credential as `authorizationUrl`, with `tokenUrl`'s origin as the legacy fallback when that field is absent. ([#3502](https://github.com/can1357/oh-my-pi/issues/3502)) ## [16.1.19] - 2026-06-25 diff --git a/packages/coding-agent/src/mcp/manager.ts b/packages/coding-agent/src/mcp/manager.ts index a12e69b9f..6fb49cb41 100644 --- a/packages/coding-agent/src/mcp/manager.ts +++ b/packages/coding-agent/src/mcp/manager.ts @@ -1235,8 +1235,9 @@ export class MCPManager { // token endpoints sit on different origins (issue #3502 review // follow-up). const authorizationUrl = material && "authorizationUrl" in material ? material.authorizationUrl : undefined; - const resource = - material?.resource ?? (config.type === "http" || config.type === "sse" ? config.url : undefined); + const resourceIsFallback = + !material?.resource && (config.type === "http" || config.type === "sse") && Boolean(config.url); + const resource = material?.resource ?? (resourceIsFallback ? config.url : undefined); // Proactive refresh: 5-minute buffer before expiry // Force refresh: on 401/403 auth errors (revoked tokens, clock skew, missing expires) const REFRESH_BUFFER_MS = 5 * 60_000; @@ -1250,7 +1251,7 @@ export class MCPManager { clientId, clientSecret, resource, - { authorizationUrl }, + { authorizationUrl, stripSameOriginResource: resourceIsFallback }, ); // Spread the old credential first so embedded refresh material survives rotation. const refreshedCredential: MCPStoredOAuthCredential = { @@ -1259,7 +1260,7 @@ export class MCPManager { tokenUrl, clientId, clientSecret, - resource, + resource: resourceIsFallback ? undefined : resource, authorizationUrl, }; await this.#authStorage.set(credentialId, refreshedCredential); diff --git a/packages/coding-agent/src/mcp/oauth-flow.ts b/packages/coding-agent/src/mcp/oauth-flow.ts index 788177643..9373d36ed 100644 --- a/packages/coding-agent/src/mcp/oauth-flow.ts +++ b/packages/coding-agent/src/mcp/oauth-flow.ts @@ -177,26 +177,38 @@ function resolveResourceUri(resource: string | undefined): string | undefined { return trimmed; } +interface ResourceIndicatorFilterOptions { + /** Strip any resource URL on the same origin as the authorization server. */ + stripSameOriginResource?: boolean; +} + /** - * Drop a resource indicator hosted on the same origin as {@link serverUrl}. + * Drop a redundant resource indicator relative to {@link serverUrl}. * - * Some authorization servers (Plane is the live example, see issue #3502) - * reject same-origin resource indicators with `server_error` before the - * consent screen, including path-bearing values such as - * `https://mcp.plane.so/http/mcp`. Per RFC 8707 §2 the indicator - * distinguishes *other* resource servers from the authorization server, so - * same-origin values are redundant for these MCP servers — silently strip - * them to stay compatible with strict implementations. + * Exact auth-server-origin values are always redundant: they don't identify a + * distinct resource server. Path-bearing same-origin resources are different: + * protected-resource discovery can advertise them for gateway-hosted MCP + * services (`https://gateway.example.com/my-service/mcp`), and those values + * must be preserved because they identify the service audience by path. * - * New credentials pass the original authorization URL so refresh filters - * against the same issuer origin as the initial grant. Legacy credentials - * fall back to the token URL's origin. + * Plane is stricter for OMP-synthesized fallback resources (e.g. using the + * configured server URL `https://mcp.plane.so/http/mcp` as `resource`), so + * fallback callers opt into `stripSameOriginResource` while provider-advertised + * `oauth.resource` values keep the path-preserving default. */ -function filterSameOriginResource(resource: string | undefined, serverUrl: string): string | undefined { +function filterResourceIndicator( + resource: string | undefined, + serverUrl: string, + options: ResourceIndicatorFilterOptions = {}, +): string | undefined { if (!resource) return undefined; try { const origin = new URL(serverUrl).origin; - if (new URL(resource).origin === origin) return undefined; + const parsedResource = new URL(resource); + if (parsedResource.origin !== origin) return resource; + if (options.stripSameOriginResource || (parsedResource.pathname === "/" && parsedResource.search === "")) { + return undefined; + } } catch { // Malformed serverUrl will fail elsewhere; fall through. } @@ -231,6 +243,13 @@ export interface MCPOAuthConfig { callbackPath?: string; /** MCP resource URI for RFC 8707 resource indicators */ resource?: string; + /** + * True when `resource` was synthesized from the server URL fallback rather + * than advertised by OAuth/protected-resource metadata. Fallback resources + * are stripped when same-origin with the authorization server; advertised + * path-scoped resources are preserved. + */ + stripSameOriginResource?: boolean; /** Fetch implementation for token exchange and discovery requests. */ fetch?: FetchImpl; } @@ -450,13 +469,14 @@ export class MCPOAuthFlow extends OAuthCallbackFlow { } /** - * Drop resource indicators hosted on the authorization-server origin. - * Delegates to {@link filterSameOriginResource}; the refresh-token leg uses - * the same helper with the persisted authorization URL so initial grant and - * refresh stay in lock-step (RFC 8707 §2.2 requires matching indicators). + * Drop redundant resource indicators for this authorization server. + * Provider-advertised path-scoped values are preserved; fallback server-URL + * values opt into same-origin stripping via `stripSameOriginResource`. */ #filterResourceIndicator(resource: string | undefined): string | undefined { - return filterSameOriginResource(resource, this.config.authorizationUrl); + return filterResourceIndicator(resource, this.config.authorizationUrl, { + stripSameOriginResource: this.config.stripSameOriginResource, + }); } /** @@ -583,6 +603,12 @@ export interface RefreshMCPOAuthTokenOptions { * origin when omitted for legacy credentials. */ authorizationUrl?: string; + /** + * True when the refresh `resource` was synthesized from the server URL + * fallback because the credential/auth material carried no resource. + * Preserved advertised resources leave this false/undefined. + */ + stripSameOriginResource?: boolean; } /** @@ -610,9 +636,11 @@ export async function refreshMCPOAuthToken( refresh_token: refreshToken, }); if (clientId) params.set("client_id", clientId); - // Drop same-origin indicators so refresh stays consistent with the initial - // grant; see {@link filterSameOriginResource} for context. - const resolvedResource = filterSameOriginResource(resolveResourceUri(resource), filterAnchor); + // Drop redundant indicators so refresh stays consistent with the initial + // grant; see {@link filterResourceIndicator} for context. + const resolvedResource = filterResourceIndicator(resolveResourceUri(resource), filterAnchor, { + stripSameOriginResource: optsFromTrailing?.stripSameOriginResource, + }); if (resolvedResource) params.set("resource", resolvedResource); if (clientSecret) params.set("client_secret", clientSecret); 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 5edac562e..55e85f1fb 100644 --- a/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts +++ b/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts @@ -496,6 +496,7 @@ export class MCPCommandController { try { const oauthResource = oauth.resource ?? finalConfig.url; + const oauthResourceIsFallback = !oauth.resource; const oauthResult = await this.#handleOAuthFlow( oauth.authorizationUrl, oauth.tokenUrl, @@ -509,11 +510,13 @@ export class MCPCommandController { prompt: finalConfig.oauth?.prompt, serverUrl: finalConfig.url, resource: oauthResource, + stripSameOriginResource: oauthResourceIsFallback, }, ); finalConfig = this.#persistOAuthResult(finalConfig, oauthResult, { tokenUrl: oauth.tokenUrl, resource: oauthResource, + stripSameOriginResource: oauthResourceIsFallback, clientId: oauth.clientId, userClientSecret: finalConfig.oauth?.clientSecret, }); @@ -583,6 +586,7 @@ export class MCPCommandController { prompt?: string; serverUrl?: string; resource?: string; + stripSameOriginResource?: boolean; }, ): Promise { const authStorage = this.ctx.session.modelRegistry.authStorage; @@ -624,6 +628,7 @@ export class MCPCommandController { callbackPort: opts?.callbackPort, callbackPath: opts?.callbackPath, resource: opts?.resource, + stripSameOriginResource: opts?.stripSameOriginResource, }, { onAuth: (info: { url: string; instructions?: string }) => { @@ -752,10 +757,17 @@ export class MCPCommandController { #persistOAuthResult( config: MCPServerConfig, result: OAuthFlowResult, - opts: { tokenUrl: string; resource?: string; clientId?: string; userClientSecret?: string }, + opts: { + tokenUrl: string; + resource?: string; + stripSameOriginResource?: boolean; + clientId?: string; + userClientSecret?: string; + }, ): MCPServerConfig { const clientId = result.clientId ?? opts.clientId ?? config.oauth?.clientId; - const resource = result.resource ?? opts.resource ?? config.auth?.resource; + const resource = + result.resource ?? (opts.stripSameOriginResource ? undefined : opts.resource) ?? config.auth?.resource; return { ...config, auth: { @@ -1559,6 +1571,7 @@ export class MCPCommandController { const currentAuthResource = currentAuth?.resource ? expandEnvVarsDeep(currentAuth.resource) : undefined; const oauthResource = oauth.resource ?? currentAuthResource ?? ("url" in runtimeBaseConfig ? runtimeBaseConfig.url : undefined); + const oauthResourceIsFallback = !oauth.resource && !currentAuthResource; const oauthResult = await this.#handleOAuthFlow( oauth.authorizationUrl, @@ -1573,6 +1586,7 @@ export class MCPCommandController { prompt: found.config.oauth?.prompt, serverUrl, resource: oauthResource, + stripSameOriginResource: oauthResourceIsFallback, }, ); @@ -1593,6 +1607,7 @@ export class MCPCommandController { clientId: oauth.clientId, userClientSecret, resource: oauthResource, + stripSameOriginResource: oauthResourceIsFallback, }); await updateMCPServer(found.filePath, name, updated); } diff --git a/packages/coding-agent/test/mcp-manager-oauth-refresh.test.ts b/packages/coding-agent/test/mcp-manager-oauth-refresh.test.ts index a52ea3f7c..c2f7c1ecc 100644 --- a/packages/coding-agent/test/mcp-manager-oauth-refresh.test.ts +++ b/packages/coding-agent/test/mcp-manager-oauth-refresh.test.ts @@ -84,7 +84,7 @@ describe("MCPManager OAuth refresh failure", () => { undefined, undefined, "https://logfire.example.com/mcp", - { authorizationUrl: undefined }, + { authorizationUrl: undefined, stripSameOriginResource: true }, ); // The poisoned Bearer must not be re-injected — that is the loop the user // reported (#1908). diff --git a/packages/coding-agent/test/oauth-flow.test.ts b/packages/coding-agent/test/oauth-flow.test.ts index b8a98f36d..c9abbb0ac 100644 --- a/packages/coding-agent/test/oauth-flow.test.ts +++ b/packages/coding-agent/test/oauth-flow.test.ts @@ -551,11 +551,10 @@ describe("mcp oauth flow", () => { expect(tokenParams.get("resource")).toBeNull(); }); describe("RFC 8707 resource indicator", () => { - // return `server_error` when the resource indicator is same-origin, - // including path-bearing resources such as - // `https://mcp.plane.so/http/mcp`. Per RFC 8707 §2 the indicator - // distinguishes *other* resource servers, so same-origin values are - // redundant for these MCP servers. + // Exact auth-server-origin values are redundant, but path-scoped + // same-host values from protected-resource discovery identify a distinct + // MCP service and must be preserved. Plane's fallback-resource case opts + // into same-origin path stripping separately. const REDIRECT_URI = "http://127.0.0.1:14580/callback"; @@ -563,6 +562,7 @@ describe("mcp oauth flow", () => { authorizationUrl: string; resource?: string; onTokenBody?: (body: string) => void; + stripSameOriginResource?: boolean; }): Promise { return new MCPOAuthFlow( { @@ -570,6 +570,7 @@ describe("mcp oauth flow", () => { tokenUrl: "https://provider.example/token", clientId: "client-id", resource: config.resource, + stripSameOriginResource: config.stripSameOriginResource, callbackPort: 14580, fetch: mockProviderTokenEndpoint(body => config.onTokenBody?.(body)), }, @@ -601,7 +602,7 @@ describe("mcp oauth flow", () => { expect(flow.resource).toBeUndefined(); }); - it("strips a self-referential resource that was pre-populated on the authorization URL", async () => { + it("strips an origin-only resource that was pre-populated on the authorization URL", async () => { const flow = await buildFlow({ authorizationUrl: "https://mcp.plane.so/authorize?resource=https%3A%2F%2Fmcp.plane.so", }); @@ -632,11 +633,31 @@ describe("mcp oauth flow", () => { expect(tokenParams.get("resource")).toBeNull(); }); - it("strips the resource when it points at a path under the auth-server origin", async () => { + it("keeps a discovered path-scoped resource under the auth-server origin", async () => { + let tokenRequestBody = ""; + const flow = await buildFlow({ + authorizationUrl: "https://gateway.example.com/authorize", + resource: "https://gateway.example.com/my-service/mcp", + onTokenBody: body => { + tokenRequestBody = body; + }, + }); + + const { url } = await flow.generateAuthUrl("state-x", REDIRECT_URI); + await flow.exchangeToken("test-code", "state-x", REDIRECT_URI); + const tokenParams = new URLSearchParams(tokenRequestBody); + + expect(new URL(url).searchParams.get("resource")).toBe("https://gateway.example.com/my-service/mcp"); + expect(flow.resource).toBe("https://gateway.example.com/my-service/mcp"); + expect(tokenParams.get("resource")).toBe("https://gateway.example.com/my-service/mcp"); + }); + + it("strips a fallback server URL resource when it points at a path under the auth-server origin", async () => { let tokenRequestBody = ""; const flow = await buildFlow({ authorizationUrl: "https://mcp.plane.so/authorize", resource: "https://mcp.plane.so/http/mcp", + stripSameOriginResource: true, onTokenBody: body => { tokenRequestBody = body; }, @@ -666,13 +687,12 @@ describe("mcp oauth flow", () => { describe("RFC 8707 resource indicator (refresh)", () => { // Regression for the review on PR #3503: the initial grant stores - // `resource: undefined` for same-origin Plane resources, but - // `MCPManager.prepareConfig` (manager.ts:1232-1233) falls back to - // `config.url` when the stored material has no resource, which would - // re-introduce the same value at refresh time. `refreshMCPOAuthToken` - // must apply the same-origin filter against the original - // authorization-server origin (falling back to `tokenUrl` for legacy - // credentials) so initial grant and refresh stay in lock-step. + // `resource: undefined` for fallback same-origin Plane resources, but + // `MCPManager.prepareConfig` falls back to `config.url` when the stored + // material has no resource. `refreshMCPOAuthToken` must apply the + // fallback same-origin filter against the original authorization-server + // origin (falling back to `tokenUrl` for legacy credentials) while + // preserving advertised path-scoped resources. function mockArbitraryTokenEndpoint(targetUrl: string, onBody: (body: string) => void): FetchImpl { return async (input, init) => { @@ -732,7 +752,27 @@ describe("mcp oauth flow", () => { expect(tokenParams.get("resource")).toBeNull(); }); - it("strips a refresh resource that points at a different path under the token-server origin", async () => { + it("keeps an advertised refresh resource that points at a path under the token-server origin", async () => { + let tokenRequestBody = ""; + + await refreshMCPOAuthToken( + "https://gateway.example.com/token", + "refresh-token", + "client-id", + undefined, + "https://gateway.example.com/my-service/mcp", + { + fetch: mockArbitraryTokenEndpoint("https://gateway.example.com/token", body => { + tokenRequestBody = body; + }), + }, + ); + const tokenParams = new URLSearchParams(tokenRequestBody); + + expect(tokenParams.get("resource")).toBe("https://gateway.example.com/my-service/mcp"); + }); + + it("strips a fallback refresh resource that points at a path under the token-server origin", async () => { let tokenRequestBody = ""; await refreshMCPOAuthToken( @@ -742,6 +782,7 @@ describe("mcp oauth flow", () => { undefined, "https://mcp.plane.so/http/mcp", { + stripSameOriginResource: true, fetch: mockArbitraryTokenEndpoint("https://mcp.plane.so/token", body => { tokenRequestBody = body; }), @@ -797,11 +838,10 @@ describe("mcp oauth flow", () => { expect(tokenParams.get("resource")).toBe("https://api.example.com"); }); - it("falls back to tokenUrl-anchored filtering for legacy credentials without authorizationUrl", async () => { - // Documents the legacy path: credentials minted before this fix - // don't carry `authorizationUrl`, so refresh still filters against - // `tokenUrl`'s origin — preserves the same-origin behavior of - // commit 1. + it("falls back to tokenUrl-anchored exact-origin filtering for legacy credentials without authorizationUrl", async () => { + // Documents the legacy path: credentials minted before this fix don't + // carry `authorizationUrl`, so refresh still strips exact origin-only + // resources against `tokenUrl`. let tokenRequestBody = ""; await refreshMCPOAuthToken(