From 89dedd8dea6d8296fae8e820e408ea7a901b2bdf Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 24 Aug 2026 21:30:29 -0400 Subject: [PATCH] fix(oauth): drop an inherited offline_access when the grant is declined MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 4 on #2114 — a real gap, not a wording one. Declining the grant stops the SDK *adding* `offline_access`, but the Inspector's own storage puts it back. A successful default-on authorization persists the scope the AS granted (`resolvePersistedScopeAfterGrant`), `offline_access` included; `prepareForAuth` reloads it and `authenticate` hands it to the SDK as `scope`, which sees the token in scope and appends `prompt=consent` again. So any server that had authorized once could uncheck the box and still hit AADSTS90094 — the users most likely to reach for the setting were the ones it did not help. `scopeForDeclinedRefreshGrant` filters it out of the *request* only, applied in the provider's `scope` getter — the one seam both `clientMetadata.scope` and `authenticate`'s `scope:` argument read through. An `offline_access` the user configured is honored; storage is never rewritten, so re-checking the box restores the old behavior instead of finding the value destroyed, and scope-satisfaction checks still see the real grant. Verified by reverting the filter: the new e2e regression fails. Also from that round, both previously suppressed: The checkbox looked effective under enterprise-managed authorization, which never builds this provider at all — it authorizes against the enterprise IdP with a fixed `openid offline_access` (`ema/constants.ts:16`). It now annotates itself the way the endpoint overrides do. The existing count assertion covered two such notices and was named for the overrides alone; it is renamed and expects three, with a dedicated case pinning the notice to this control so the count cannot be met by three copies elsewhere. And "obtains no new refresh token" was another overstatement — the AS decides issuance, and a preconfigured client is reused with whatever its registration already permits. Replaced with what the setting actually controls. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall --- .../ServerSettingsForm.test.tsx | 35 ++++++++- .../ServerSettingsForm/ServerSettingsForm.tsx | 11 ++- .../web/src/test/core/auth/providers.test.ts | 75 ++++++++++++++++++ clients/web/src/test/core/auth/scopes.test.ts | 52 +++++++++++++ .../mcp/inspectorClient-oauth-e2e.test.ts | 77 +++++++++++++++++++ core/auth/providers.ts | 24 ++++++ core/auth/scopes.ts | 42 ++++++++++ core/mcp/oauthManager.ts | 3 + docs/mcp-server-configuration.md | 8 +- 9 files changed, 323 insertions(+), 4 deletions(-) diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx index 6198aedf..a0330516 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx @@ -818,6 +818,33 @@ describe("ServerSettingsForm", () => { ).not.toBeInTheDocument(); }); + // #2068 — EMA never builds the Inspector's OAuth provider at all; it + // authorizes against the enterprise IdP with a fixed `openid offline_access`. + // The checkbox therefore cannot affect that request and must say so. + it("says the refresh-token setting is not applied under EMA", () => { + const { rerender } = renderWithMantine( + , + ); + expect( + screen.getByText(/which this setting does not reach/), + ).toBeInTheDocument(); + + rerender( + , + ); + expect( + screen.queryByText(/which this setting does not reach/), + ).not.toBeInTheDocument(); + }); + // #2018 — custom authorization-request parameters. it("shows the empty hint for authorization parameters when none are set", () => { renderWithMantine( @@ -1110,7 +1137,7 @@ describe("ServerSettingsForm", () => { ); }); - it("says the endpoint overrides are unused under enterprise-managed auth", () => { + it("says the EMA-suppressed controls are unused under enterprise-managed auth", () => { const { rerender } = renderWithMantine( { expandedSections={["oauth"]} />, ); + // Three controls carry this notice: the two endpoint overrides (#1906) and + // the refresh-token checkbox (#2068). The count is asserted rather than + // merely "present" so a control that stops annotating itself — or a new one + // that never starts — is caught here. expect( screen.getAllByText( /Not applied while Enterprise-managed authorization is on/, ), - ).toHaveLength(2); + ).toHaveLength(3); rerender( p.key.trim()) .filter((key) => isReservedAuthorizationParam(key)); @@ -892,7 +901,7 @@ export function ServerSettingsForm({ /> onOAuthChange({ diff --git a/clients/web/src/test/core/auth/providers.test.ts b/clients/web/src/test/core/auth/providers.test.ts index 2c178a28..fa2d81a2 100644 --- a/clients/web/src/test/core/auth/providers.test.ts +++ b/clients/web/src/test/core/auth/providers.test.ts @@ -506,6 +506,81 @@ describe("OAuthNavigation", () => { makeProviderWithRefresh(false).clientMetadata.grant_types, ).toEqual(["authorization_code"]); }); + + // The regression that makes the opt-out actually work for a server that + // already authorized once. A successful default-on grant persists the + // AS-granted scope — `offline_access` included — and the provider reloads + // it here. Handing that straight back to the SDK puts the token in scope + // again, and `startAuthorization` appends `prompt=consent` off the scope + // alone, so the box would be unchecked and AADSTS90094 would persist. + describe("scope inherited from an earlier default-on grant", () => { + function makeProviderWithScopes( + storedScope: string, + configuredScope: string | undefined, + requestRefreshToken: boolean, + ): { provider: BaseOAuthClientProvider; storage: OAuthStorage } { + const storage = makeStorage(); + vi.mocked(storage.getScope).mockResolvedValue(storedScope); + const provider = new BaseOAuthClientProvider(SERVER, { + storage, + redirectUrlProvider: new MutableRedirectUrlProvider(), + navigation: new CallbackNavigation(vi.fn()), + requestRefreshToken, + configuredScope, + }); + return { provider, storage }; + } + + it("drops the inherited offline_access from the requested scope", async () => { + const { provider } = makeProviderWithScopes( + "mcp offline_access", + "mcp", + false, + ); + await provider.prepareForAuth(); + + expect(provider.scope).toBe("mcp"); + expect(provider.clientMetadata.scope).toBe("mcp"); + }); + + it("honors an offline_access the user configured", async () => { + const { provider } = makeProviderWithScopes( + "mcp offline_access", + "mcp offline_access", + false, + ); + await provider.prepareForAuth(); + + expect(provider.scope).toBe("mcp offline_access"); + }); + + it("leaves the scope alone while the grant is declared", async () => { + const { provider } = makeProviderWithScopes( + "mcp offline_access", + "mcp", + true, + ); + await provider.prepareForAuth(); + + expect(provider.scope).toBe("mcp offline_access"); + }); + + // Filtered at the point of request, never in storage: re-checking the + // box must restore the previous behavior rather than find the value + // destroyed, and the persisted scope is also what scope-satisfaction + // checks read. + it("does not rewrite the persisted scope", async () => { + const { provider, storage } = makeProviderWithScopes( + "mcp offline_access", + "mcp", + false, + ); + await provider.prepareForAuth(); + void provider.scope; + + expect(storage.saveScope).not.toHaveBeenCalled(); + }); + }); }); describe("SEP-2352 issuer threading", () => { diff --git a/clients/web/src/test/core/auth/scopes.test.ts b/clients/web/src/test/core/auth/scopes.test.ts index bc906661..dfb304d0 100644 --- a/clients/web/src/test/core/auth/scopes.test.ts +++ b/clients/web/src/test/core/auth/scopes.test.ts @@ -4,6 +4,7 @@ import { isStrictScopeSuperset, resolveEffectiveGrantedScope, resolvePersistedScopeAfterGrant, + scopeForDeclinedRefreshGrant, } from "@inspector/core/auth/scopes.js"; describe("scopes", () => { @@ -65,3 +66,54 @@ describe("scopes", () => { }); }); }); + +describe("scopeForDeclinedRefreshGrant (#2068)", () => { + it("drops an inherited offline_access from the requested scope", () => { + expect(scopeForDeclinedRefreshGrant("mcp offline_access", "mcp")).toBe( + "mcp", + ); + }); + + // The persisted scope is what a previous default-on grant left behind, and + // the configured scope may be unset entirely (DCR + resource-advertised + // scopes). An inherited token is still inherited. + it("drops it when no scope is configured at all", () => { + expect(scopeForDeclinedRefreshGrant("mcp offline_access", undefined)).toBe( + "mcp", + ); + }); + + it("keeps an offline_access the user explicitly configured", () => { + expect( + scopeForDeclinedRefreshGrant("mcp offline_access", "mcp offline_access"), + ).toBe("mcp offline_access"); + }); + + it("leaves a scope without offline_access untouched", () => { + expect(scopeForDeclinedRefreshGrant("mcp tools:read", "mcp")).toBe( + "mcp tools:read", + ); + }); + + // Requesting `scope=` empty is not the same as omitting it; collapse to + // undefined so the SDK falls back to its own resolution. + it("collapses to undefined when offline_access was the only scope", () => { + expect(scopeForDeclinedRefreshGrant("offline_access", "")).toBeUndefined(); + }); + + it("passes an absent scope through", () => { + expect(scopeForDeclinedRefreshGrant(undefined, "mcp")).toBeUndefined(); + }); + + it("matches whole tokens, not substrings", () => { + expect( + scopeForDeclinedRefreshGrant("mcp offline_access_extra", "mcp"), + ).toBe("mcp offline_access_extra"); + }); + + it("tolerates irregular whitespace", () => { + expect( + scopeForDeclinedRefreshGrant(" mcp offline_access ", "mcp"), + ).toBe("mcp"); + }); +}); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-oauth-e2e.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-oauth-e2e.test.ts index 821ef98e..a2778b87 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-oauth-e2e.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-oauth-e2e.test.ts @@ -1459,6 +1459,83 @@ describe("InspectorClient OAuth E2E", () => { // addition, not what the user asked for. expect(scope.split(/\s+/)).toContain("mcp"); }); + + // #2068 — the case that makes the opt-out worth anything to the user who + // reported it: a server that already authorized successfully. That grant + // persists the AS-granted scope, `offline_access` included, and the + // provider reloads it on the next connect. Handing it back unfiltered puts + // the token in scope again and `startAuthorization` re-adds + // `prompt=consent` off the scope alone — box unchecked, AADSTS90094 intact. + it("drops an offline_access inherited from an earlier grant's stored scope", async () => { + const transport = transports[1]!; + const staticClientId = "test-inherited-offline-client"; + const staticClientSecret = "test-inherited-offline-secret"; + const serverConfig = { + ...getDefaultServerConfig(), + serverType: transport.serverType, + ...createOAuthTestServerConfig({ + requireAuth: true, + supportRefreshTokens: true, + scopesSupported: ["mcp", "offline_access"], + staticClients: [ + { + clientId: staticClientId, + clientSecret: staticClientSecret, + redirectUris: [testRedirectUrl], + }, + ], + }), + }; + server = new TestServerHttp(serverConfig); + const port = await server.start(); + const serverUrl = `http://localhost:${port}`; + await waitForOAuthWellKnown(serverUrl); + + const oauthConfig = createTestOAuthConfig({ + mode: "static", + clientId: staticClientId, + clientSecret: staticClientSecret, + redirectUrl: testRedirectUrl, + scope: "mcp", + }); + // Exactly what a completed default-on authorization leaves behind (see + // `resolvePersistedScopeAfterGrant`): the scope the AS granted. + const mcpUrl = `${serverUrl}${transport.endpoint}`; + await oauthConfig.storage.saveScope(mcpUrl, "mcp offline_access"); + + client = new InspectorClient( + { type: transport.clientType, url: mcpUrl } as MCPServerConfig, + { + environment: { + transport: createTransportNode, + oauth: { + storage: oauthConfig.storage, + navigation: oauthConfig.navigation, + redirectUrlProvider: oauthConfig.redirectUrlProvider, + }, + }, + oauth: { + clientId: oauthConfig.clientId, + clientSecret: oauthConfig.clientSecret, + scope: oauthConfig.scope, + requestRefreshToken: false, + }, + }, + ); + + const authUrl = await client.authenticate(); + if (!authUrl) throw new Error("Expected authorization URL"); + const scope = authUrl.searchParams.get("scope") ?? ""; + expect(scope.split(/\s+/)).not.toContain("offline_access"); + expect(authUrl.searchParams.get("prompt")).toBeNull(); + expect(scope.split(/\s+/)).toContain("mcp"); + + // Filtered on the way out, not in storage — re-checking the box has to + // restore the old behavior rather than find the scope destroyed. + expect(await oauthConfig.storage.getScope(mcpUrl)).toBe( + "mcp offline_access", + ); + }); }); describe("SEP-2352 AS migration (Streamable HTTP)", () => { diff --git a/core/auth/providers.ts b/core/auth/providers.ts index 2223fa41..59381c6a 100644 --- a/core/auth/providers.ts +++ b/core/auth/providers.ts @@ -12,6 +12,7 @@ import type { import type { OAuthStorage, SaveClientInformationOptions } from "./storage.js"; import { generateOAuthState } from "./utils.js"; import { applyAuthorizationParams } from "./authorizationParams.js"; +import { scopeForDeclinedRefreshGrant } from "./scopes.js"; /** * Redirect URL provider. Returns the redirect URL for OAuth flows. @@ -125,6 +126,16 @@ export type OAuthProviderConfig = { * its own a guarantee that `prompt=consent` is absent. */ requestRefreshToken?: boolean; + /** + * The scope configured for this server (mcp.json / Server Settings), as + * distinct from the scope carried in OAuth storage after a previous grant. + * + * Used only to decide whether an `offline_access` in the effective scope was + * *asked for* or merely inherited — see {@link scopeForDeclinedRefreshGrant}. + * An explicitly configured one is honored; an inherited one is dropped from + * the request when {@link requestRefreshToken} is false. (#2068) + */ + configuredScope?: string; }; /** @@ -148,6 +159,8 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { protected authorizationParams?: Record; /** Declare the `refresh_token` grant in {@link clientMetadata} (#2068). */ protected requestRefreshToken: boolean; + /** Server-configured scope, for the #2068 `offline_access` distinction. */ + protected configuredScope?: string; constructor(serverUrl: string, oauthConfig: OAuthProviderConfig) { this.serverUrl = serverUrl; @@ -157,6 +170,7 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { this.clientMetadataUrl = oauthConfig.clientMetadataUrl; this.authorizationParams = oauthConfig.authorizationParams; this.requestRefreshToken = oauthConfig.requestRefreshToken ?? true; + this.configuredScope = oauthConfig.configuredScope; } /** @@ -194,6 +208,16 @@ 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. + if (!this.requestRefreshToken) { + return scopeForDeclinedRefreshGrant( + this.cachedScope, + this.configuredScope, + ); + } return this.cachedScope; } diff --git a/core/auth/scopes.ts b/core/auth/scopes.ts index 2ffbe817..d658ea8a 100644 --- a/core/auth/scopes.ts +++ b/core/auth/scopes.ts @@ -18,6 +18,48 @@ export const computeScopeUnion = sdkComputeScopeUnion; */ export const isStrictScopeSuperset = sdkIsStrictScopeSuperset; +/** The scope token whose presence makes the SDK force `prompt=consent`. */ +export const OFFLINE_ACCESS_SCOPE = "offline_access"; + +/** + * The scope to *request* when the `refresh_token` grant has been declined + * (#2068). + * + * Declining the grant stops the SDK adding `offline_access`, but it cannot + * remove one that is already in the scope — and the Inspector's own storage is + * the way it gets there. A successful authorization persists the scope the AS + * granted (`resolvePersistedScopeAfterGrant`), which for a default-on client + * includes `offline_access`; the provider reloads that on the next connect and + * hands it back to the SDK, which sees the token in scope and appends + * `prompt=consent` again. So a server that authorized once before the opt-out + * would keep hitting AADSTS90094 with the box unchecked — the exact failure the + * setting exists to prevent. + * + * Dropping it from the *request* (not from storage) is what breaks that loop: + * the next grant comes back without `offline_access` and the persisted scope + * self-heals, while re-checking the box restores the old behavior from the + * user's own configuration rather than from a value we destroyed. + * + * A scope the user explicitly configured is left alone — they asked for it, and + * the web form warns that it keeps the consent prompt alive. + */ +export function scopeForDeclinedRefreshGrant( + effectiveScope: string | undefined, + configuredScope: string | undefined, +): string | undefined { + if (!effectiveScope) return effectiveScope; + const configured = configuredScope?.trim().split(/\s+/) ?? []; + if (configured.includes(OFFLINE_ACCESS_SCOPE)) return effectiveScope; + + const kept = effectiveScope + .trim() + .split(/\s+/) + .filter((token) => token !== "" && token !== OFFLINE_ACCESS_SCOPE); + // An all-`offline_access` scope collapses to undefined rather than "", so the + // SDK falls back to its own resolution instead of requesting an empty scope. + return kept.length > 0 ? kept.join(" ") : undefined; +} + /** * Scope to persist after a successful token grant (RFC 6749 §5.1). * When the AS returns `scope`, it is the authoritative full grant. diff --git a/core/mcp/oauthManager.ts b/core/mcp/oauthManager.ts index 711b7e3f..400d39fd 100644 --- a/core/mcp/oauthManager.ts +++ b/core/mcp/oauthManager.ts @@ -175,6 +175,9 @@ export class OAuthManager { // `authorizationParams` this is pure config off mcp.json, so it lives on // the provider rather than in OAuth storage. requestRefreshToken: this.oauthConfig.requestRefreshToken, + // #2068: lets the provider tell an `offline_access` the user asked for + // from one inherited from a previous grant's persisted scope. + configuredScope: this.oauthConfig.scope, }); provider.setEventTarget(this.params.getEventTarget()); diff --git a/docs/mcp-server-configuration.md b/docs/mcp-server-configuration.md index 0ed960bf..9a1fed29 100644 --- a/docs/mcp-server-configuration.md +++ b/docs/mcp-server-configuration.md @@ -195,12 +195,18 @@ These have no analog in the broader `mcp.json` ecosystem. Each is **omitted on w `oauth.requestRefreshToken` (default `true`) controls whether the Inspector declares the `refresh_token` grant when it registers its OAuth client. Set it to `false` — or uncheck **Request refresh token** in Server Settings → Authorization — when that grant costs you more than it buys. -The chain it breaks runs through the SDK. `determineScope()` appends `offline_access` to the requested scope when the authorization server advertises that scope **and** the client metadata declares `refresh_token`; `startAuthorization()` then appends `prompt=consent` whenever `offline_access` is in the effective scope. On Microsoft Entra ID a forced `prompt=consent` from a non-admin user, for an app whose permissions require admin consent, is routed into the admin-consent workflow and rejected with `AADSTS90094` — even after a tenant admin has already granted consent ([#2068](https://github.com/modelcontextprotocol/inspector/issues/2068)). Going forward the Inspector then obtains no new refresh token, so an expired access token needs a fresh interactive sign-in rather than a silent refresh. Only `false` is written to disk, so a server that never touched the setting keeps a minimal entry. +The chain it breaks runs through the SDK. `determineScope()` appends `offline_access` to the requested scope when the authorization server advertises that scope **and** the client metadata declares `refresh_token`; `startAuthorization()` then appends `prompt=consent` whenever `offline_access` is in the effective scope. On Microsoft Entra ID a forced `prompt=consent` from a non-admin user, for an app whose permissions require admin consent, is routed into the admin-consent workflow and rejected with `AADSTS90094` — even after a tenant admin has already granted consent ([#2068](https://github.com/modelcontextprotocol/inspector/issues/2068)). Only `false` is written to disk, so a server that never touched the setting keeps a minimal entry. + +The setting suppresses the grant declaration and the SDK's scope augmentation — that is the whole of what it controls. It does not decide what the authorization server issues: token issuance is the AS's call, and a preconfigured client is reused with whatever its server-side registration already permits, so a refresh token can still come back and the Inspector will store and use it. Expect a fresh interactive sign-in on expiry where the AS honors the narrowed request; don't rely on it. > **It removes the SDK's *automatic* `offline_access`, and nothing more.** `prompt=consent` keys off `offline_access` being in the effective scope; it never looks at `grant_types`. So the prompt still goes out if `offline_access` reaches that scope by another route — you listed it yourself in **Scopes**, or you left Scopes blank and the resource server's metadata advertises it in `scopes_supported`. Unchecking this box is not on its own a guarantee that the consent prompt is gone: check the Scopes field too. The web form flags the first case inline. +> **Not applied under enterprise-managed authorization** (`oauth.enterpriseManaged`) — and for a stronger reason than the endpoint overrides above: that leg never builds the Inspector's OAuth client provider at all. It authorizes against the enterprise IdP with a fixed `openid offline_access` scope, which this setting does not reach. The web form says so inline when EMA is on. + > **It applies on the next connect, not to the live connection.** OAuth is connection-time configuration here — like the client credentials, the scopes, and the endpoint overrides beside it — so a client that is already connected keeps the metadata it was built with. Reconnect for the change to reach the authorization flow. +> **A scope inherited from an earlier grant is dropped from the request.** A successful authorization persists the scope the authorization server granted, which for a default-on client includes `offline_access`. Left alone, that value would come straight back on the next connect and re-trigger `prompt=consent` with the box unchecked. So when the grant is declined the Inspector filters `offline_access` out of what it *requests* — unless you configured it in **Scopes** yourself, which it honors. The stored scope is not rewritten: re-checking the box restores the previous behavior instead of finding the value destroyed. + > **Two pieces of state outlive the setting, and neither is cleared by unchecking it.** A refresh token issued before the opt-out stays usable — the SDK's refresh path posts the stored token with `grant_type=refresh_token` without consulting the client metadata, so such a server can keep refreshing silently and you may not observe the cost described above at all. And the registration already held at the authorization server still lists the grant, because turning the setting off changes what the Inspector *declares*, not what the AS has recorded. > > **Clear stored OAuth state** (Server Settings → Authorization) clears the Inspector's **local** copies — the tokens and the client information — which is enough for the first. It is not a revocation: it cannot reach the authorization server. For the second, what happens next depends on how the client was obtained. A **dynamically registered** client is registered afresh on the next connect, and the new registration declares only `authorization_code`; the old one still exists at the AS, unused. A **preconfigured `oauth.clientId`** is reused as-is, so changing what that client declares is done at the authorization server, not here.