mirror of
https://github.com/badlogic/pi-mono.git
synced 2026-11-05 23:37:25 +00:00
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).
This commit is contained in:
parent
0c453048bd
commit
e529a82c98
7 changed files with 88 additions and 15 deletions
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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<void> {
|
||||
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");
|
||||
|
||||
|
|
|
|||
|
|
@ -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) });
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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<OAuthFlowResult> {
|
||||
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();
|
||||
})
|
||||
|
|
|
|||
|
|
@ -31,6 +31,7 @@ export {
|
|||
refreshAuthorization,
|
||||
registerClient,
|
||||
startAuthorization,
|
||||
stepUpScope,
|
||||
type TokenRequestOptions,
|
||||
} from "./flow.ts";
|
||||
export {
|
||||
|
|
|
|||
|
|
@ -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");
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue