diff --git a/clients/web/src/test/core/auth/ema/emaFlow.test.ts b/clients/web/src/test/core/auth/ema/emaFlow.test.ts index 2b71f484..95c67eab 100644 --- a/clients/web/src/test/core/auth/ema/emaFlow.test.ts +++ b/clients/web/src/test/core/auth/ema/emaFlow.test.ts @@ -378,7 +378,7 @@ describe("emaFlow", () => { // Seed code verifier / client info / metadata for the callback exchange. await startEmaIdpAuthorization(config); - const tokens = await completeEmaIdpAuthorizationAndMint( + const { tokens } = await completeEmaIdpAuthorizationAndMint( config, "auth-code", IDP_ISSUER, @@ -392,6 +392,43 @@ describe("emaFlow", () => { ); }); + it("completeEmaIdpAuthorizationAndMint reports a metadata-derived scope (#2117)", async () => { + // With no configured scope, discoverEmaResourceContext resolves the + // request from the protected resource metadata's `scopes_supported`. That + // is what both exchanges send, so it is what an omitted `scope` in the + // response implies was granted -- and the caller can only apply RFC 6749 + // 5.1 if the flow hands it back. + const idToken = jwtWithExp(Math.floor(Date.now() / 1000) + 3600); + const storage = createMemoryStorage(); + const inner = mockEmaFetch(idToken); + const fetchFn = vi.fn( + async (input: RequestInfo | URL, init?: RequestInit) => { + if (String(input).includes("/.well-known/oauth-protected-resource")) { + return new Response( + JSON.stringify({ + resource: MCP_RESOURCE, + authorization_servers: [AS_ISSUER], + scopes_supported: ["mcp:read", "mcp:write"], + }), + ); + } + return inner(input, init); + }, + ); + const config = { ...baseConfig(storage), scope: undefined, fetchFn }; + + await startEmaIdpAuthorization(config); + + const { tokens, requestedScope } = await completeEmaIdpAuthorizationAndMint( + config, + "auth-code", + IDP_ISSUER, + ); + + expect(tokens.access_token).toBe("resource-access-token"); + expect(requestedScope).toBe("mcp:read mcp:write"); + }); + it("completeEmaIdpAuthorizationAndMint wraps leg 1 errors", async () => { const errorSpy = vi .spyOn(console, "error") diff --git a/clients/web/src/test/core/mcp/oauthManager.test.ts b/clients/web/src/test/core/mcp/oauthManager.test.ts index 7e8e4de6..c8597381 100644 --- a/clients/web/src/test/core/mcp/oauthManager.test.ts +++ b/clients/web/src/test/core/mcp/oauthManager.test.ts @@ -798,6 +798,48 @@ describe("OAuthManager", () => { ); }); + it("records the request for a plain challenge redirect too, not just step-up", async () => { + // A `token_expired` / `unauthorized` challenge can return an interactive + // redirect whose callback lands in this same completeOAuthFlow. It used + // to record nothing, so a silent token response left the old stored + // scope standing — the same defect on a sibling entry point. + mockedMcpAuth.mockResolvedValue("REDIRECT"); + const params = createMockParams(); + storageOf(params).getScope.mockResolvedValue("stale:scope"); + storageOf(params).getClientInformation.mockResolvedValue({ + client_id: "cid", + }); + const manager = new OAuthManager(params); + manager.setOAuthConfig({ scope: "catalog:scope" }); + const captureSpy = vi + .spyOn( + (await import("@inspector/core/auth/providers.js")) + .BaseOAuthClientProvider.prototype, + "getCapturedAuthUrl", + ) + .mockReturnValue(capturedUrlWithScope("catalog:scope offline_access")); + + const outcome = await manager.handleAuthChallenge({ + reason: "token_expired", + }); + expect(outcome.kind).toBe("interactive"); + + mockedMcpAuth.mockResolvedValue("AUTHORIZED"); + storageOf(params).getTokens.mockResolvedValue({ + access_token: "access", + token_type: "Bearer", + }); + storageOf(params).saveScope.mockClear(); + + await manager.completeOAuthFlow("code"); + + expect(storageOf(params).saveScope).toHaveBeenCalledWith( + SERVER_URL, + "catalog:scope offline_access", + ); + captureSpy.mockRestore(); + }); + it("persists nothing when neither the request nor the response names a scope", async () => { const params = await authorizeThenComplete(undefined, undefined); @@ -824,7 +866,7 @@ describe("OAuthManager", () => { describe("completeOAuthFlow (EMA)", () => { it("mints resource tokens via the EMA path and dispatches complete", async () => { - const tokens = { access_token: "EMA", token_type: "Bearer" }; + const tokens = { access_token: "EMA", token_type: "Bearer" as const }; const params = createMockParams({ enterpriseManagedAuth: { idp: { @@ -839,7 +881,7 @@ describe("OAuthManager", () => { const mintSpy = vi .spyOn(emaFlow, "completeEmaIdpAuthorizationAndMint") - .mockResolvedValue(tokens); + .mockResolvedValue({ tokens }); await manager.completeOAuthFlow("ema-code"); @@ -866,7 +908,10 @@ describe("OAuthManager", () => { const mintSpy = vi .spyOn(emaFlow, "completeEmaIdpAuthorizationAndMint") - .mockResolvedValue({ access_token: "EMA", token_type: "Bearer" }); + .mockResolvedValue({ + tokens: { access_token: "EMA", token_type: "Bearer" }, + requestedScope: "mcp weather:read", + }); storageOf(params).saveScope.mockClear(); await manager.completeOAuthFlow("ema-code"); @@ -877,6 +922,42 @@ describe("OAuthManager", () => { ); mintSpy.mockRestore(); }); + + it("persists a mint scope that came from resource metadata, not config (#2117)", async () => { + // With neither a configured nor a stored scope, discoverEmaResourceContext + // resolves the request from the protected resource metadata's + // `scopes_supported`. That value never reaches the manager's own + // `scopeForMint`, so only what the mint reports can be persisted. + const params = createMockParams({ + enterpriseManagedAuth: { + idp: { + issuer: "https://idp.example.com", + clientId: "app-client", + clientSecret: "secret", + }, + }, + }); + params.initialConfig.scope = undefined; + storageOf(params).getScope.mockResolvedValue(undefined); + const manager = new OAuthManager(params); + manager.setOAuthConfig({ enterpriseManaged: true }); + + const mintSpy = vi + .spyOn(emaFlow, "completeEmaIdpAuthorizationAndMint") + .mockResolvedValue({ + tokens: { access_token: "EMA", token_type: "Bearer" }, + requestedScope: "mcp:read mcp:write", + }); + storageOf(params).saveScope.mockClear(); + + await manager.completeOAuthFlow("ema-code"); + + expect(storageOf(params).saveScope).toHaveBeenCalledWith( + SERVER_URL, + "mcp:read mcp:write", + ); + mintSpy.mockRestore(); + }); }); describe("trySilentEnterpriseManagedAuth", () => { @@ -1704,7 +1785,10 @@ describe("OAuthManager", () => { .mockResolvedValue(authUrl); const mintSpy = vi .spyOn(emaFlow, "completeEmaIdpAuthorizationAndMint") - .mockResolvedValue({ access_token: "tok", token_type: "Bearer" }); + .mockResolvedValue({ + tokens: { access_token: "tok", token_type: "Bearer" }, + requestedScope: "mcp tools:read weather:read", + }); const params = createMockParams({ enterpriseManagedAuth: { idp: { diff --git a/core/auth/ema/emaFlow.ts b/core/auth/ema/emaFlow.ts index 99f77301..475e603f 100644 --- a/core/auth/ema/emaFlow.ts +++ b/core/auth/ema/emaFlow.ts @@ -124,11 +124,26 @@ export async function startEmaIdpAuthorization( return authorizationUrl; } +/** + * The outcome of a completed EMA authorization. + * + * `requestedScope` is the scope the mint actually asked for, which is not + * always `config.scope`: when neither the server config nor storage supplies + * one, {@link discoverEmaResourceContext} resolves it from the protected + * resource metadata's `scopes_supported`. Callers need that resolved value to + * apply RFC 6749 §5.1 — a token response that omits `scope` granted exactly + * what was requested — so it is returned rather than left inside the flow. + */ +export interface EmaAuthorizationResult { + tokens: OAuthTokens; + requestedScope?: string; +} + export async function completeEmaIdpAuthorizationAndMint( config: EmaFlowConfig, authorizationCode: string, iss?: string, -): Promise { +): Promise { try { await completeIdpOidcAuthorization({ idp: config.idp, @@ -146,8 +161,17 @@ export async function completeEmaIdpAuthorizationAndMint( } let tokens: OAuthTokens; + // Resolve the resource context here rather than inside the mint, so the + // scope it settles on is visible to the caller. It is passed straight + // through, so this costs no extra discovery. + let resourceContext: EmaResourceContext; try { - tokens = await mintEmaResourceTokens(config); + resourceContext = await discoverEmaResourceContext( + config.serverUrl, + config.scope, + config.fetchFn, + ); + tokens = await mintEmaResourceTokens(config, resourceContext); } catch (err) { throw wrapEmaMintError(err); } @@ -155,7 +179,7 @@ export async function completeEmaIdpAuthorizationAndMint( await config.storage.saveTokens(config.serverUrl, tokens, { enterpriseManaged: true, }); - return tokens; + return { tokens, requestedScope: resourceContext.scope }; } /** Re-run legs 2–3 on 401 when IdP session is still valid. */ diff --git a/core/mcp/oauthManager.ts b/core/mcp/oauthManager.ts index 4dbfbb9d..a1e6eea6 100644 --- a/core/mcp/oauthManager.ts +++ b/core/mcp/oauthManager.ts @@ -324,20 +324,7 @@ 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. - // - // Read it off the authorize URL rather than from `provider.scope`: the - // SDK augments the request with `offline_access` when the AS advertises - // it and the client asks for a refresh token, so the provider's scope can - // be a strict subset of what was actually asked for — and persisting the - // subset would understate the grant in exactly the way this fixes. - this.pendingAuthorizationScope = - capturedUrl.searchParams.get("scope")?.trim() || requestedScope; + this.recordAuthorizationRequestScope(capturedUrl, requestedScope); const stateParam = capturedUrl.searchParams.get("state"); if (stateParam && this.params.onBeforeOAuthRedirect) { @@ -357,6 +344,27 @@ export class OAuthManager { return capturedUrl; } + /** + * Remember what an interactive authorization asked for, so the callback can + * apply RFC 6749 §5.1 — a token response that omits `scope` granted exactly + * the requested scope (#2117). Without it, an ordinary grant whose AS echoes + * no scope leaves the previous stored scope standing as though it were what + * the current token carries. + * + * The authorize URL is the authority, not the scope handed to `auth()`: the + * SDK augments the request with `offline_access` when the AS advertises it + * and the client asks for a refresh token, so the input can be a strict + * subset of what was actually requested. The fallback covers an authorize + * URL that carries no `scope` parameter at all. + */ + private recordAuthorizationRequestScope( + authorizationUrl: URL, + requestedScope: string | undefined, + ): void { + this.pendingAuthorizationScope = + authorizationUrl.searchParams.get("scope")?.trim() || requestedScope; + } + async completeOAuthFlow( authorizationCode: string, iss?: string, @@ -368,18 +376,19 @@ export class OAuthManager { const config = scopeForMint ? { ...emaConfig, scope: scopeForMint } : emaConfig; - const tokens = await completeEmaIdpAuthorizationAndMint( - config, - authorizationCode, - iss, - ); - // `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 { tokens, requestedScope } = + await completeEmaIdpAuthorizationAndMint( + config, + authorizationCode, + iss, + ); + // The mint reports what it actually asked for, which is neither + // `pendingAuthorizationScope` (undefined on the ordinary leg) nor + // always `scopeForMint` — with no configured or stored scope the + // resource metadata's `scopes_supported` is what gets requested. const scopeToPersist = resolvePersistedScopeAfterGrant( tokens.scope, - scopeForMint, + requestedScope, ); if (scopeToPersist) { await this.requireStorage().saveScope( @@ -859,9 +868,12 @@ export class OAuthManager { }; } - if (enriched.reason === "insufficient_scope" && scopeForAuth) { - this.pendingAuthorizationScope = scopeForAuth; - } + // Every interactive redirect from here lands back in completeOAuthFlow, + // so every one of them needs its request recorded — not just the step-up + // (#2117). A plain `unauthorized` / `token_expired` challenge can request + // a scope that differs from what storage holds, and used to persist + // nothing when the AS answered without a `scope`. + this.recordAuthorizationRequestScope(capturedUrl, scopeForAuth); const clientInfo = await provider.clientInformation(); await this.recordAuthorizationCodeFlowState(