From db23bc9b97d002d66e920a48ee3c8f71193fda23 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 25 Aug 2026 00:35:10 -0400 Subject: [PATCH] fix(oauth): persist the requested scope when the grant omits it RFC 6749 5.1 defines an omitted `scope` in a token response as a grant identical to what was requested, and `resolvePersistedScopeAfterGrant` already encodes that rule -- but the ordinary authorization leg never gave it a `requestedScope` to fall back to, so nothing was persisted and the previous stored scope kept being reported as what the current token carries. `authenticate()` now records `provider.scope` as the pending authorization scope once the authorization URL has been captured, so it survives the redirect and reaches `completeOAuthFlow`. The enterprise-managed leg had the same defect for the same reason and is fixed one level up: it now falls back to `scopeForMint`, which is what the mint actually asked for, rather than to the raw `pendingAuthorizationScope` that is undefined on this leg. A response that does carry `scope` still wins over the request, on both legs. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall --- .../src/test/core/mcp/oauthManager.test.ts | 93 +++++++++++++++++++ core/mcp/oauthManager.ts | 18 +++- 2 files changed, 108 insertions(+), 3 deletions(-) diff --git a/clients/web/src/test/core/mcp/oauthManager.test.ts b/clients/web/src/test/core/mcp/oauthManager.test.ts index 716638dd..de7770a5 100644 --- a/clients/web/src/test/core/mcp/oauthManager.test.ts +++ b/clients/web/src/test/core/mcp/oauthManager.test.ts @@ -736,6 +736,70 @@ describe("OAuthManager", () => { }); }); + describe("persisting the requested scope after an ordinary grant (#2117)", () => { + const CAPTURED_URL = new URL( + "https://auth.example.com/authorize?state=abc", + ); + + async function authorizeThenComplete( + storedScope: string | undefined, + tokenScope: string | undefined, + ) { + mockedMcpAuth.mockResolvedValue("REDIRECT"); + const params = createMockParams(); + storageOf(params).getScope.mockResolvedValue(storedScope); + storageOf(params).getClientInformation.mockResolvedValue({ + client_id: "cid", + }); + const manager = new OAuthManager(params); + const captureSpy = vi + .spyOn( + (await import("@inspector/core/auth/providers.js")) + .BaseOAuthClientProvider.prototype, + "getCapturedAuthUrl", + ) + .mockReturnValue(CAPTURED_URL); + + await manager.authenticate(); + + mockedMcpAuth.mockResolvedValue("AUTHORIZED"); + storageOf(params).getTokens.mockResolvedValue({ + access_token: "access", + token_type: "Bearer", + ...(tokenScope === undefined ? {} : { scope: tokenScope }), + }); + storageOf(params).saveScope.mockClear(); + + await manager.completeOAuthFlow("code"); + captureSpy.mockRestore(); + return params; + } + + it("persists the requested scope when the token response omits scope", async () => { + const params = await authorizeThenComplete("mcp weather:read", undefined); + + expect(storageOf(params).saveScope).toHaveBeenCalledWith( + SERVER_URL, + "mcp weather:read", + ); + }); + + it("still prefers the granted scope when the AS echoes one", async () => { + const params = await authorizeThenComplete("mcp weather:read", "mcp"); + + expect(storageOf(params).saveScope).toHaveBeenCalledWith( + SERVER_URL, + "mcp", + ); + }); + + it("persists nothing when neither the request nor the response names a scope", async () => { + const params = await authorizeThenComplete(undefined, undefined); + + expect(storageOf(params).saveScope).not.toHaveBeenCalled(); + }); + }); + describe("completeOAuthFlow (EMA)", () => { it("mints resource tokens via the EMA path and dispatches complete", async () => { const tokens = { access_token: "EMA", token_type: "Bearer" }; @@ -762,6 +826,35 @@ describe("OAuthManager", () => { expect(manager.getOAuthFlowStep()).toBe("complete"); mintSpy.mockRestore(); }); + + it("persists the minted scope request when the mint response omits scope (#2117)", async () => { + const params = createMockParams({ + enterpriseManagedAuth: { + idp: { + issuer: "https://idp.example.com", + clientId: "app-client", + clientSecret: "secret", + }, + }, + }); + params.initialConfig.scope = "mcp weather:read"; + storageOf(params).getScope.mockResolvedValue(undefined); + const manager = new OAuthManager(params); + manager.setOAuthConfig({ enterpriseManaged: true }); + + const mintSpy = vi + .spyOn(emaFlow, "completeEmaIdpAuthorizationAndMint") + .mockResolvedValue({ access_token: "EMA", token_type: "Bearer" }); + storageOf(params).saveScope.mockClear(); + + await manager.completeOAuthFlow("ema-code"); + + expect(storageOf(params).saveScope).toHaveBeenCalledWith( + SERVER_URL, + "mcp weather:read", + ); + mintSpy.mockRestore(); + }); }); describe("trySilentEnterpriseManagedAuth", () => { diff --git a/core/mcp/oauthManager.ts b/core/mcp/oauthManager.ts index 530f3fed..5a8955ff 100644 --- a/core/mcp/oauthManager.ts +++ b/core/mcp/oauthManager.ts @@ -305,9 +305,10 @@ export class OAuthManager { resourceMetadataUrl, }); + const requestedScope = provider.scope; const result = await mcpAuth(provider, { serverUrl, - scope: provider.scope, + scope: requestedScope, fetchFn: this.params.effectiveAuthFetch, resourceMetadataUrl, }); @@ -323,6 +324,14 @@ export class OAuthManager { throw new Error("Failed to capture authorization URL"); } + // RFC 6749 5.1: an omitted `scope` in the token response means the grant + // is identical to what was requested. Carry the request across the + // redirect so completeOAuthFlow has that fallback to persist (#2117) -- + // the step-up paths already do this, and without it an ordinary grant + // whose AS echoes no scope leaves the previous stored scope standing as + // if it were still what the current token carries. + this.pendingAuthorizationScope = requestedScope; + const stateParam = capturedUrl.searchParams.get("state"); if (stateParam && this.params.onBeforeOAuthRedirect) { const parsedState = parseOAuthState(stateParam); @@ -357,10 +366,13 @@ export class OAuthManager { authorizationCode, iss, ); - const requestedScope = this.pendingAuthorizationScope; + // `scopeForMint` -- not `pendingAuthorizationScope` -- is what the + // mint actually asked for, so it is the RFC 6749 5.1 fallback here + // (the two differ on the ordinary leg, where only the config carries + // a scope). const scopeToPersist = resolvePersistedScopeAfterGrant( tokens.scope, - requestedScope, + scopeForMint, ); if (scopeToPersist) { await this.requireStorage().saveScope(