diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index e3b16cb2..35a4fe66 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -654,6 +654,7 @@ import { useServers, } from "@inspector/core/react/useServers.js"; import type { + InspectorClientOptions, InspectorServerSettings, MessageEntry, ServerEntry, @@ -702,20 +703,36 @@ const fetchLogInstances = ( // `requestRefreshToken` spread from App's `oauthFromServer` would leave the // checkbox persisted but inert with every other new test still green. describe("App wires the refresh-token opt-out into the client (#2068)", () => { - const HTTP_SERVER = { + // Fully-typed fixtures rather than a cast: `settings` is the whole + // `InspectorServerSettings`, so a field added to that interface later is a + // compile error here instead of a silently-absent value at runtime. + const HTTP_SERVER: ServerEntry = { id: "A", name: "PlotRocket", config: { type: "streamable-http", url: "https://api.example.com/mcp" }, connection: { status: "disconnected" }, }; - function mockServersWith(settings?: Partial) { + const BASE_SETTINGS: InspectorServerSettings = { + headers: [], + env: [], + metadata: {}, + connectionTimeout: 0, + requestTimeout: 0, + taskTtl: 60000, + maxFetchRequests: 1000, + roots: [], + }; + + function mockServersWith(overrides?: Partial) { vi.mocked(useServers).mockReturnValue({ servers: [ { ...HTTP_SERVER, - ...(settings ? { settings } : {}), - } as unknown as ServerEntry, + ...(overrides + ? { settings: { ...BASE_SETTINGS, ...overrides } } + : {}), + }, ], loading: false, error: undefined, @@ -729,13 +746,13 @@ describe("App wires the refresh-token opt-out into the client (#2068)", () => { }); } - /** The `oauth` option the mocked InspectorClient constructor was given. */ - function constructedOAuth(): { requestRefreshToken?: boolean } | undefined { - const call = vi.mocked(McpIndex.InspectorClient).mock.calls[0]; - const options = call?.[1] as - | { oauth?: { requestRefreshToken?: boolean } } - | undefined; - return options?.oauth; + /** + * The `oauth` option the mocked InspectorClient constructor was given. Read + * through the constructor's own parameter type, so a rename of the option + * fails to compile here rather than silently reading `undefined`. + */ + function constructedOAuth(): InspectorClientOptions["oauth"] { + return vi.mocked(McpIndex.InspectorClient).mock.calls[0]?.[1]?.oauth; } let previousUseServers: typeof useServers | undefined; diff --git a/clients/web/src/test/core/auth/providers.test.ts b/clients/web/src/test/core/auth/providers.test.ts index 1a4f68b8..2c178a28 100644 --- a/clients/web/src/test/core/auth/providers.test.ts +++ b/clients/web/src/test/core/auth/providers.test.ts @@ -496,9 +496,11 @@ describe("OAuthNavigation", () => { }); // The point of the opt-out: the SDK's determineScope() only appends - // `offline_access` when the client metadata declares `refresh_token`, and - // startAuthorization() only forces `prompt=consent` when `offline_access` - // is in scope. Dropping the grant is what breaks that chain. + // `offline_access` when the client metadata declares `refresh_token`. + // This asserts our half — the grant list. That the authorize URL then + // carries neither `offline_access` nor `prompt=consent` is proved against + // a real AS in the e2e suite (inspectorClient-oauth-e2e.test.ts), since + // the SDK, not this array, is what ultimately writes that URL. it("drops the refresh_token grant when disabled", () => { expect( makeProviderWithRefresh(false).clientMetadata.grant_types, 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 127b231b..821ef98e 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 @@ -1383,6 +1383,81 @@ describe("InspectorClient OAuth E2E", () => { if (!authUrl) throw new Error("Expected authorization URL"); const scope = authUrl.searchParams.get("scope") ?? ""; expect(scope.split(/\s+/)).toContain("offline_access"); + // The consequence of that scope, and the actual failure in #2068: the SDK + // forces an interactive consent prompt. Asserted here so the opt-out case + // below is a real before/after pair rather than a scope check alone. + expect(authUrl.searchParams.get("prompt")).toBe("consent"); + }); + + // #2068 — the same server, the same AS advertising `offline_access`, with + // the grant declined. This is the end-to-end proof the unit tests cannot + // give: they assert our `grant_types` array, while the behavior that + // actually unblocks an Entra tenant is what the SDK puts in the authorize + // URL. If the SDK ever stopped keying its augmentation off `grant_types`, + // every other test here would still pass and AADSTS90094 would remain. + it("omits offline_access and prompt=consent when the refresh-token grant is declined", async () => { + const transport = transports[1]!; + const staticClientId = "test-no-offline-access-client"; + const staticClientSecret = "test-no-offline-access-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", + }); + client = new InspectorClient( + { + type: transport.clientType, + url: `${serverUrl}${transport.endpoint}`, + } 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(); + // The configured scope still goes out — the opt-out drops the SDK's + // addition, not what the user asked for. + expect(scope.split(/\s+/)).toContain("mcp"); }); }); diff --git a/core/auth/providers.ts b/core/auth/providers.ts index 2c959e81..2223fa41 100644 --- a/core/auth/providers.ts +++ b/core/auth/providers.ts @@ -116,8 +116,13 @@ export type OAuthProviderConfig = { * `startAuthorization()` then appends `prompt=consent` whenever * `offline_access` is in scope. Against Microsoft Entra ID, that forced * consent prompt routes a non-admin user into the admin-consent workflow and - * fails with `AADSTS90094` even after a tenant admin has consented. Dropping - * the grant here breaks that chain at its source. + * fails with `AADSTS90094` even after a tenant admin has consented. + * + * It removes that *automatic* augmentation only. `startAuthorization()` reads + * the scope, never `grant_types`, so an `offline_access` the caller passed in + * `scope` — or one the resource advertises when `scope` is unset, which + * `determineScope` falls back to — still produces the prompt. This is not on + * its own a guarantee that `prompt=consent` is absent. */ requestRefreshToken?: boolean; }; @@ -205,9 +210,10 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { redirect_uris: this.redirect_uris, token_endpoint_auth_method: "none", // #2068: `refresh_token` is declared by default, and dropped when the - // server's "Request refresh token" setting is off — which also stops the - // SDK adding `offline_access` (and therefore `prompt=consent`) to the - // authorization request. See `OAuthProviderConfig.requestRefreshToken`. + // server's "Request refresh token" setting is off — which stops the SDK + // *adding* `offline_access` (and so the `prompt=consent` it forces). It + // does not remove an `offline_access` that reaches the scope another way. + // See `OAuthProviderConfig.requestRefreshToken`. grant_types: this.requestRefreshToken ? ["authorization_code", "refresh_token"] : ["authorization_code"], diff --git a/core/mcp/types.ts b/core/mcp/types.ts index 4f2afb5c..1fa91487 100644 --- a/core/mcp/types.ts +++ b/core/mcp/types.ts @@ -184,9 +184,12 @@ export type StoredMCPServer = MCPServerConfig & { * Whether the Inspector declares the `refresh_token` grant when it * registers. Defaults to `true`; only `false` is written to disk, so an * entry that never touched the setting keeps a minimal diff. Turning it off - * also stops the SDK adding `offline_access` to the requested scope and - * `prompt=consent` to the authorization request. Inspector-specific. - * (#2068) + * stops the SDK's *automatic* `offline_access` augmentation, and so the + * `prompt=consent` that scope forces — but only that one: an + * `offline_access` from `scopes` here, or from the resource's advertised + * scopes when `scopes` is unset, still reaches the authorization request. + * See {@link InspectorServerSettings.oauthRequestRefreshToken}. + * Inspector-specific. (#2068) */ requestRefreshToken?: boolean; /**