diff --git a/clients/web/src/test/core/mcp/oauthManager.test.ts b/clients/web/src/test/core/mcp/oauthManager.test.ts index 02280d1e..eadca821 100644 --- a/clients/web/src/test/core/mcp/oauthManager.test.ts +++ b/clients/web/src/test/core/mcp/oauthManager.test.ts @@ -1140,6 +1140,81 @@ describe("OAuthManager", () => { ).toBe(false); }); + // #2068 round 8 — the step-up path never reads the provider's filtering + // `scope` getter: `enrichChallengeWithScopes` unions the *raw* stored scope + // with the challenge and `handleAuthChallenge` hands that straight to + // `mcpAuth`. So an inherited `offline_access` reappeared here with the + // setting off, and the SDK re-added `prompt=consent` — the exact failure + // the option exists to prevent, on the one path that bypassed it. + describe("step-up scope with the refresh grant declined (#2068)", () => { + /** The `scope` this challenge run handed to the SDK. */ + async function scopeSentForChallenge( + storedScope: string, + configuredScope: string | undefined, + requiredScopes: string[], + requestRefreshToken: boolean | undefined, + ): Promise { + const params = createMockParams(); + storageOf(params).getScope.mockResolvedValue(storedScope); + storageOf(params).getTokens.mockResolvedValue({ + access_token: "tok", + token_type: "Bearer", + scope: storedScope, + }); + storageOf(params).getClientInformation.mockResolvedValue({ + client_id: "cid", + }); + const manager = new OAuthManager(params); + manager.setOAuthConfig({ + ...(configuredScope !== undefined && { scope: configuredScope }), + ...(requestRefreshToken !== undefined && { requestRefreshToken }), + }); + mockedMcpAuth.mockResolvedValue("REDIRECT"); + + await manager.handleAuthChallenge({ + reason: "insufficient_scope", + requiredScopes, + }); + + const call = mockedMcpAuth.mock.calls.at(-1); + return (call?.[1] as { scope?: string } | undefined)?.scope; + } + + it("drops an inherited offline_access from the step-up union", async () => { + const sent = await scopeSentForChallenge( + "mcp offline_access", + "mcp", + ["tools:write"], + false, + ); + expect(sent?.split(/\s+/)).not.toContain("offline_access"); + // The step-up still asks for what the challenge demanded. + expect(sent?.split(/\s+/)).toContain("tools:write"); + }); + + it("keeps offline_access in the step-up union while the grant is on", async () => { + const sent = await scopeSentForChallenge( + "mcp offline_access", + "mcp", + ["tools:write"], + undefined, + ); + expect(sent?.split(/\s+/)).toContain("offline_access"); + }); + + // Stripping a scope the challenge itself requires would loop: + // re-authorize, earn the same challenge, strip it again. + it("preserves an offline_access the challenge requires", async () => { + const sent = await scopeSentForChallenge( + "mcp", + "mcp", + ["offline_access"], + false, + ); + expect(sent?.split(/\s+/)).toContain("offline_access"); + }); + }); + it("short-circuits handleAuthChallenge when scope already satisfied", async () => { const params = createMockParams(); storageOf(params).getTokens.mockResolvedValue({ diff --git a/core/auth/providers.ts b/core/auth/providers.ts index 59381c6a..0447b729 100644 --- a/core/auth/providers.ts +++ b/core/auth/providers.ts @@ -208,10 +208,15 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { } get scope(): string | undefined { - // #2068: filter at the point of *request*, not in storage. This getter is - // the one seam both readers go through — `clientMetadata.scope` below, and - // the `scope:` argument `OAuthManager.authenticate` hands to the SDK — so - // filtering here covers both without a second derivation that could drift. + // #2068: filter at the point of *request*, not in storage. Covers the two + // readers that go through this getter — `clientMetadata.scope` below, and + // the `scope:` argument `OAuthManager.authenticate` hands to the SDK. + // + // It is NOT the only request path: mid-session `insufficient_scope` step-up + // builds its union from raw storage and passes it to `mcpAuth` directly, + // never reading this getter. `OAuthManager.handleAuthChallenge` applies the + // same filter there. Both call `scopeForDeclinedRefreshGrant`, so the rule + // lives in one place even though it has to be applied twice. if (!this.requestRefreshToken) { return scopeForDeclinedRefreshGrant( this.cachedScope, diff --git a/core/auth/scopes.ts b/core/auth/scopes.ts index 7cc0bfa7..e8f94e0e 100644 --- a/core/auth/scopes.ts +++ b/core/auth/scopes.ts @@ -52,14 +52,28 @@ export const OFFLINE_ACCESS_SCOPE = "offline_access"; * Widening the request with unrelated configured tokens is outside what this * setting governs and could push an otherwise-satisfied connection into a * step-up authorization. + * + * `requiredScopes` carries scopes a `403 insufficient_scope` challenge demands + * (SEP-2350). They are honored exactly like a configured one: the server is + * asking for them, and stripping a scope the challenge requires would loop — + * re-authorize, get the same challenge back, strip it again. The step-up path + * needs this because it does not read the scope through the provider at all: + * it unions the *raw* stored scope with the challenge and hands that to the + * SDK directly, so an inherited `offline_access` would otherwise reappear + * there with the setting off. */ export function scopeForDeclinedRefreshGrant( effectiveScope: string | undefined, configuredScope: string | undefined, + requiredScopes?: readonly string[], ): string | undefined { if (!effectiveScope) return effectiveScope; const configured = configuredScope?.trim().split(/\s+/) ?? []; - if (configured.includes(OFFLINE_ACCESS_SCOPE)) { + const required = requiredScopes ?? []; + if ( + configured.includes(OFFLINE_ACCESS_SCOPE) || + required.includes(OFFLINE_ACCESS_SCOPE) + ) { const tokens = effectiveScope .trim() .split(/\s+/) @@ -83,7 +97,7 @@ export function scopeForDeclinedRefreshGrant( // silent overwrite of the persisted value, so re-enabling the grant could no // longer restore it. Hand back the configured scope instead: it is the right // thing to request, and it leaves storage alone. - // Cannot itself contain `offline_access`: that case returned unchanged above. + // Cannot itself contain `offline_access`: that case returned above. const fallback = configuredScope?.trim(); if (fallback) return fallback; diff --git a/core/mcp/oauthManager.ts b/core/mcp/oauthManager.ts index 8aaac9a3..0f0b4727 100644 --- a/core/mcp/oauthManager.ts +++ b/core/mcp/oauthManager.ts @@ -48,6 +48,7 @@ import { isStrictScopeSuperset, resolveEffectiveGrantedScope, resolvePersistedScopeAfterGrant, + scopeForDeclinedRefreshGrant, } from "../auth/scopes.js"; import { stepUpInsufficientScopeMessage } from "../auth/oauthUx.js"; import type { @@ -783,13 +784,29 @@ export class OAuthManager { resourceMetadataUrl, }); - const scopeForAuth = + // #2068 — the step-up union is built from *raw* storage + // (`enrichChallengeWithScopes`) and handed to the SDK directly, so it never + // passes through the provider's filtering `scope` getter. Without this an + // `offline_access` inherited from an earlier grant reappears here and the + // SDK re-adds `prompt=consent` even with the setting off — the same failure + // the option exists to prevent, on the one path that bypasses it. A scope + // the challenge itself requires is preserved, since stripping that would + // just re-earn the same challenge. + const requestedScopeForAuth = enriched.reason === "insufficient_scope" ? enriched.authorizationScopes?.join(" ") : this.oauthConfig.scope?.trim() || (enriched.requiredScopes?.length ? enriched.requiredScopes.join(" ") : undefined); + const scopeForAuth = + this.oauthConfig.requestRefreshToken === false + ? scopeForDeclinedRefreshGrant( + requestedScopeForAuth, + this.oauthConfig.scope, + enriched.requiredScopes, + ) + : requestedScopeForAuth; provider.setSuppressAuthorizationNavigation(true); let result: Awaited>;