From e529a82c98fad679c6e20c03ffbb0bc7896fecb9 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Thu, 1 Oct 2026 10:22:33 +0200 Subject: [PATCH] fix(mcp): keep granted scopes on step-up authorization A step-up sign-in requested only the scopes of the insufficient_scope challenge, so the new token lost the scopes granted before and other requests asked for sign-in again. Tokens now record their granted scope and step-up requests the union (SEP-2350). --- packages/coding-agent/CHANGELOG.md | 1 + .../coding-agent/src/extensions/mcp/oauth.ts | 12 ++++-- .../test/mcp-oauth-refresh.test.ts | 41 ++++++++++++++++++- packages/mcp/CHANGELOG.md | 2 + packages/mcp/src/oauth/flow.ts | 39 ++++++++++++++---- packages/mcp/src/oauth/index.ts | 1 + packages/mcp/test/oauth.test.ts | 7 +++- 7 files changed, 88 insertions(+), 15 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index dc726321d..e0468ed80 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -12,6 +12,7 @@ - Fixed MCP OAuth sign-in failing with `Invalid scope` when the token response contains `"scope": ""`, and similar failures for other empty or `null` optional OAuth fields ([#10266](https://github.com/earendil-works/pi/issues/10266)). - Fixed the sign-in URL printed by `/mcp login` not being clickable when it wraps ([#10186](https://github.com/earendil-works/pi/issues/10186)). - Fixed `--provider` without `--model` being silently ignored and running the default model from another provider; it now fails with an error ([#10236](https://github.com/earendil-works/pi/issues/10236)). +- Fixed MCP servers that ask for more scope (`insufficient_scope`) requesting sign-in over and over. The new sign-in requested only the missing scopes, so the new token lost access the previous one had; it now keeps the granted scopes. ## [0.99.2] - 2026-09-30 diff --git a/packages/coding-agent/src/extensions/mcp/oauth.ts b/packages/coding-agent/src/extensions/mcp/oauth.ts index 565d7b064..7d7cd146c 100644 --- a/packages/coding-agent/src/extensions/mcp/oauth.ts +++ b/packages/coding-agent/src/extensions/mcp/oauth.ts @@ -26,6 +26,7 @@ import { type OAuthChallenge, type OAuthClientInformationMixed, parseWwwAuthenticate, + stepUpScope, } from "@earendil-works/pi-mcp/oauth"; import lockfile from "proper-lockfile"; import { APP_NAME, getAgentDir } from "../../config.ts"; @@ -386,6 +387,7 @@ export async function signInMcpServer(options: { }): Promise { const { serverUrl, store, settings } = options; const stored = await store.load(); + const stepUp = options.challenge?.error === "insufficient_scope"; const callbackOptions = callbackSettings(settings); // Reuse the port of the registered redirect URI so the registered client stays valid. const registered = registeredRedirectUrls(stored?.clientInformation)[0]; @@ -415,11 +417,15 @@ export async function signInMcpServer(options: { serverUrl, resourceMetadataUrl: options.challenge?.resourceMetadataUrl, authorizationServerMetadataUrl: settings.authServerMetadataUrl, - // A server asking for more scope gets it on top of the configured scope. - scope: mergeScopes(settings.scope, options.challenge?.scope), + // A server asking for more scope gets it on top of the configured scope and, since the challenge + // may list only the missing scopes, on top of the scope granted so far. + scope: mergeScopes( + settings.scope, + stepUp ? stepUpScope(stored?.tokens?.scope, options.challenge?.scope) : options.challenge?.scope, + ), }; // A refresh keeps the granted scope; a server asking for more needs the browser flow. - const skipRefresh = options.challenge?.error === "insufficient_scope"; + const skipRefresh = stepUp; if ((await authorizeMcp(provider, { ...flow, skipRefresh })) === "AUTHORIZED") return; if (!authorizationUrl) throw new Error("OAuth flow did not produce an authorization URL"); diff --git a/packages/coding-agent/test/mcp-oauth-refresh.test.ts b/packages/coding-agent/test/mcp-oauth-refresh.test.ts index f7eb22655..5bee26540 100644 --- a/packages/coding-agent/test/mcp-oauth-refresh.test.ts +++ b/packages/coding-agent/test/mcp-oauth-refresh.test.ts @@ -1,7 +1,7 @@ import { mkdtempSync, readdirSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { OAuthIssuerMismatchError } from "@earendil-works/pi-mcp/oauth"; +import { type OAuthChallenge, OAuthIssuerMismatchError } from "@earendil-works/pi-mcp/oauth"; import { afterEach, describe, expect, it } from "vitest"; import { InMemoryAuthStorageBackend } from "../src/core/auth-storage.ts"; import { @@ -112,6 +112,45 @@ describe("MCP OAuth sign-in", () => { await expect(signIn({ iss: "https://attacker.example" })).rejects.toBeInstanceOf(OAuthIssuerMismatchError); }); + it("keeps the granted scope when the server asks for more", async () => { + const server = await startOAuthMcpServer(); + try { + const store = new McpOAuthCredentialStore(new InMemoryAuthStorageBackend()).forServer(server.url); + const opened: URL[] = []; + const signInWith = (challenge: OAuthChallenge) => + signInMcpServer({ + serverUrl: server.url, + store, + settings: {}, + challenge, + prompt: { + showAuthorizationUrl: (url) => { + opened.push(url); + void fetch(url); + }, + promptForRedirectUrl: (signal) => + new Promise((resolve) => + signal.addEventListener("abort", () => resolve(undefined), { once: true }), + ), + }, + }); + + await signInWith({ scope: "issues:read" }); + // The token response names no scope, so the grant has the requested one. + expect((await store.load())?.tokens?.scope).toBe("issues:read"); + // The step-up challenge lists only the missing scope. Requesting just that would lose + // issues:read, so the next request would ask for sign-in again. + await signInWith({ error: "insufficient_scope", scope: "issues:write" }); + expect(opened.map((url) => url.searchParams.get("scope"))).toEqual([ + "issues:read", + "issues:read issues:write", + ]); + expect((await store.load())?.tokens?.scope).toBe("issues:read issues:write"); + } finally { + await server.close(); + } + }); + // #10172 it("uses the configured authorization server metadata URL", async () => { const settings = (serverUrl: string) => ({ authServerMetadataUrl: new URL("/missing", serverUrl) }); diff --git a/packages/mcp/CHANGELOG.md b/packages/mcp/CHANGELOG.md index 749c81488..9a9ee1ab0 100644 --- a/packages/mcp/CHANGELOG.md +++ b/packages/mcp/CHANGELOG.md @@ -6,6 +6,7 @@ - Added an `authorizationServerMetadataUrl` option to `authorizeMcp()` to use a configured authorization server metadata document instead of discovery ([#10172](https://github.com/earendil-works/pi/issues/10172)). - Added an `iss` option to `authorizeMcp()`. The authorization code is only exchanged when `iss` names the flow's authorization server, or is absent and the server's metadata does not set `authorization_response_iss_parameter_supported` (RFC 9207). +- Added `stepUpScope()`, which combines the scopes of an `insufficient_scope` challenge with the scopes granted so far. ### Fixed @@ -14,6 +15,7 @@ - Fixed invalid URLs in protected resource metadata aborting OAuth discovery instead of falling back to the MCP server's origin. - Fixed an empty requested scope, from `scope=""` in a `WWW-Authenticate` challenge or an empty `scopes_supported`, overriding the next scope source. - Fixed list pagination failing with a duplicate cursor error when a server ends pagination with `nextCursor: ""` or `null`. +- Fixed step-up authorization in `adaptOAuthProvider()` requesting only the scopes of the `insufficient_scope` challenge. The new token lost the scopes granted before, so other requests failed with `insufficient_scope` again. Tokens now record their granted scope, which is the requested scope when the token response omits `scope`. ## [0.99.2] - 2026-09-30 diff --git a/packages/mcp/src/oauth/flow.ts b/packages/mcp/src/oauth/flow.ts index c92766c87..ab504cda5 100644 --- a/packages/mcp/src/oauth/flow.ts +++ b/packages/mcp/src/oauth/flow.ts @@ -263,6 +263,21 @@ export async function refreshAuthorization( return { refresh_token: options.refreshToken, ...tokens }; } +function withScope(tokens: OAuthTokens, scope: string | undefined): OAuthTokens { + return tokens.scope === undefined && scope ? { ...tokens, scope } : tokens; +} + +/** + * Scopes for a step-up authorization: the challenged scopes plus the ones granted so far, since a + * challenge may list only the missing scopes and a token with just those would lose access the old + * one had (SEP-2350). Without challenged scopes, `undefined` lets the flow pick its default. + */ +export function stepUpScope(granted: string | undefined, challenged: string | undefined): string | undefined { + if (!challenged) return undefined; + const scopes = [granted, challenged].flatMap((scope) => scope?.split(/\s+/).filter(Boolean) ?? []); + return [...new Set(scopes)].join(" "); +} + async function runFlow(provider: OAuthClientProvider, options: OAuthFlowOptions): Promise { const metadataUrl = options.authorizationServerMetadataUrl && secureEndpoint(options.authorizationServerMetadataUrl); // With a configured metadata URL, discovery is not cached, so changing the URL applies at once. @@ -333,7 +348,9 @@ async function runFlow(provider: OAuthClientProvider, options: OAuthFlowOptions) codeVerifier: await provider.codeVerifier(), redirectUrl: provider.redirectUrl, }); - await provider.saveTokens(tokens); + // A response without `scope` grants the requested scope (RFC 6749 §5.1). Recorded so a step-up can + // keep it. Callers pass the options of the authorization request, so `scope` is what was requested. + await provider.saveTokens(withScope(tokens, scope)); return "AUTHORIZED"; } const existing = options.skipRefresh ? undefined : await provider.tokens(); @@ -343,7 +360,8 @@ async function runFlow(provider: OAuthClientProvider, options: OAuthFlowOptions) ...tokenOptions, refreshToken: existing.refresh_token, }); - await provider.saveTokens(tokens); + // A refresh without `scope` keeps the scope of the grant (RFC 6749 §6). + await provider.saveTokens(withScope(tokens, existing.scope)); return "AUTHORIZED"; } catch (error) { if (error instanceof OAuthInsecureEndpointError) throw error; @@ -397,13 +415,16 @@ export function adaptOAuthProvider(provider: OAuthClientProvider): AuthProvider const current = (await provider.tokens())?.access_token; if (current !== undefined && current !== context.token) return; } - inFlight ??= authorizeMcp(provider, { - serverUrl: context.serverUrl, - resourceMetadataUrl: challenge.resourceMetadataUrl, - scope: challenge.scope, - fetch: context.fetch, - skipRefresh: insufficientScope, - }) + inFlight ??= Promise.resolve(insufficientScope ? provider.tokens() : undefined) + .then((granted) => + authorizeMcp(provider, { + serverUrl: context.serverUrl, + resourceMetadataUrl: challenge.resourceMetadataUrl, + scope: insufficientScope ? stepUpScope(granted?.scope, challenge.scope) : challenge.scope, + fetch: context.fetch, + skipRefresh: insufficientScope, + }), + ) .then((result) => { if (result === "REDIRECT") throw new McpOAuthAuthorizationRequiredError(); }) diff --git a/packages/mcp/src/oauth/index.ts b/packages/mcp/src/oauth/index.ts index 0e19478ba..287581811 100644 --- a/packages/mcp/src/oauth/index.ts +++ b/packages/mcp/src/oauth/index.ts @@ -31,6 +31,7 @@ export { refreshAuthorization, registerClient, startAuthorization, + stepUpScope, type TokenRequestOptions, } from "./flow.ts"; export { diff --git a/packages/mcp/test/oauth.test.ts b/packages/mcp/test/oauth.test.ts index 9b9b7eedb..0f7e5f685 100644 --- a/packages/mcp/test/oauth.test.ts +++ b/packages/mcp/test/oauth.test.ts @@ -265,10 +265,12 @@ describe("MCP OAuth", () => { openGetStream: false, }), ); + // Neither token response names a scope, so the grant has the requested scope. expect(provider.tokenSet).toEqual({ access_token: "refreshed-token", refresh_token: "refresh-token", token_type: "Bearer", + scope: "org:read", }); expect(refreshes).toBe(1); await refreshedClient.close(); @@ -347,7 +349,7 @@ describe("MCP OAuth", () => { it("asks for authorization instead of refreshing when the server needs more scope", async () => { const provider = new TestOAuthProvider("http://127.0.0.1/callback"); provider.client = { client_id: "client" }; - provider.tokenSet = { access_token: "a1", refresh_token: "r1", token_type: "Bearer" }; + provider.tokenSet = { access_token: "a1", refresh_token: "r1", token_type: "Bearer", scope: "repo read:org" }; const origin = await listen(async (request, response, serverOrigin) => { const url = new URL(request.url ?? "/", serverOrigin); if (url.pathname === "/.well-known/oauth-authorization-server") { @@ -377,7 +379,8 @@ describe("MCP OAuth", () => { token: "a1", }), ).rejects.toBeInstanceOf(McpOAuthAuthorizationRequiredError); - expect(provider.authorizationUrl?.searchParams.get("scope")).toBe("repo admin"); + // The challenge may list only the missing scopes; the new grant keeps the old ones too. + expect(provider.authorizationUrl?.searchParams.get("scope")).toBe("repo read:org admin"); // The working grant is kept until the user authorizes the new scope. expect(provider.tokenSet?.access_token).toBe("a1"); });