Merge branch 'v2/main' into v2/fix/2096-stdio-env-cwd-revert

clients/web/src/test/core/auth/ema/emaFlow.test.ts

@@ -70,6 +70,8 @@ function createMemoryStorage(
     saveTokens: vi.fn(async (url: string, tokens) => {
       savedTokens[url] = tokens;
     }),
+    getScope: vi.fn(async () => undefined),
+    saveScope: vi.fn(),
     clear: vi.fn(),
     clearEnterpriseManagedResourceServers: vi.fn(),
   } as unknown as OAuthStorage;
@@ -186,7 +188,7 @@ describe("emaFlow", () => {
     const idToken = jwtWithExp(exp);
     idpSessions[IDP_ISSUER] = { idToken };
 
-    const tokens = await mintEmaResourceTokens(
+    const { tokens } = await mintEmaResourceTokens(
       { ...baseConfig(storage), fetchFn: mockEmaFetch(idToken) },
       resourceContext(),
     );
@@ -318,7 +320,7 @@ describe("emaFlow", () => {
       },
     );
 
-    const tokens = await mintEmaResourceTokens(
+    const { tokens } = await mintEmaResourceTokens(
       { ...baseConfig(storage), fetchFn },
       resourceContext(),
     );
@@ -349,7 +351,7 @@ describe("emaFlow", () => {
     const idToken = jwtWithExp(exp);
     idpSessions[IDP_ISSUER] = { idToken };
 
-    const tokens = await mintEmaResourceTokens({
+    const { tokens } = await mintEmaResourceTokens({
       ...baseConfig(storage),
       fetchFn: mockEmaFetch(idToken),
     });
@@ -378,7 +380,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 +394,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")
@@ -438,6 +477,45 @@ describe("emaFlow", () => {
     );
   });
 
+  it("trySilentEmaAuth persists the scope its mint asked for (#2117)", async () => {
+    // The silent path is how an initial EMA authentication completes when the
+    // IdP session is still cached, so it is a moment a newly configured scope
+    // first takes effect -- and the mock AS answers without a `scope`.
+    const idToken = jwtWithExp(Math.floor(Date.now() / 1000) + 3600);
+    const storage = createMemoryStorage({ [IDP_ISSUER]: { idToken } });
+    const fetchFn = mockEmaFetch(idToken);
+
+    const result = await trySilentEmaAuth({
+      ...baseConfig(storage),
+      scope: "mcp weather:read",
+      fetchFn,
+    });
+
+    expect(result.status).toBe("success");
+    expect(storage.saveScope).toHaveBeenCalledWith(
+      SERVER_URL,
+      "mcp weather:read",
+    );
+  });
+
+  it("refreshEmaResourceTokens persists the scope its mint asked for (#2117)", async () => {
+    const idToken = jwtWithExp(Math.floor(Date.now() / 1000) + 3600);
+    const storage = createMemoryStorage({ [IDP_ISSUER]: { idToken } });
+    const fetchFn = mockEmaFetch(idToken);
+
+    const tokens = await refreshEmaResourceTokens({
+      ...baseConfig(storage),
+      scope: "mcp weather:read",
+      fetchFn,
+    });
+
+    expect(tokens?.access_token).toBe("resource-access-token");
+    expect(storage.saveScope).toHaveBeenCalledWith(
+      SERVER_URL,
+      "mcp weather:read",
+    );
+  });
+
   it("refreshEmaResourceTokens returns undefined without an IdP session", async () => {
     expect(await refreshEmaResourceTokens(baseConfig(storage))).toBeUndefined();
   });

clients/web/src/test/core/mcp/oauthManager.test.ts

@@ -736,9 +736,137 @@ describe("OAuthManager", () => {
     });
   });
 
+  describe("persisting the requested scope after an ordinary grant (#2117)", () => {
+    function capturedUrlWithScope(scope?: string): URL {
+      const url = new URL("https://auth.example.com/authorize?state=abc");
+      if (scope !== undefined) {
+        url.searchParams.set("scope", scope);
+      }
+      return url;
+    }
+
+    async function authorizeThenComplete(
+      storedScope: string | undefined,
+      tokenScope: string | undefined,
+      authorizeUrlScope?: string,
+    ) {
+      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(capturedUrlWithScope(authorizeUrlScope));
+
+      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("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);
+
+      expect(storageOf(params).saveScope).not.toHaveBeenCalled();
+    });
+
+    it("persists the scope the authorize URL actually carried, not the provider's", async () => {
+      // The SDK augments the request with `offline_access` when the AS
+      // advertises it and the client wants a refresh token, so the authorize
+      // URL is a superset of `provider.scope`. Persisting the provider's copy
+      // would understate the grant the AS implied by staying silent.
+      const params = await authorizeThenComplete(
+        "mcp",
+        undefined,
+        "mcp offline_access",
+      );
+
+      expect(storageOf(params).saveScope).toHaveBeenCalledWith(
+        SERVER_URL,
+        "mcp offline_access",
+      );
+    });
+  });
+
   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: {
@@ -753,7 +881,7 @@ describe("OAuthManager", () => {
 
       const mintSpy = vi
         .spyOn(emaFlow, "completeEmaIdpAuthorizationAndMint")
-        .mockResolvedValue(tokens);
+        .mockResolvedValue({ tokens });
 
       await manager.completeOAuthFlow("ema-code");
 
@@ -1146,6 +1274,14 @@ describe("OAuthManager", () => {
     // populated that fallback. So a filtered request whose AS omitted `scope`
     // persisted nothing, the stale `offline_access` survived in storage, and
     // the "self-healing" this feature documents never happened.
+    //
+    // #2068 fixed that only while the opt-out was on, which was the case where
+    // a stale scope caused a user-visible failure. #2117 dropped the gate, so
+    // the recording is now unconditional and reads the authorize URL rather
+    // than the provider — see `recordAuthorizationRequestScope`. The filter
+    // still applies with the opt-out on, because the SDK's `offline_access`
+    // augmentation is gated on the `refresh_token` grant type the provider
+    // drops there.
     describe("persisting the filtered scope after an ordinary grant (#2068)", () => {
       async function completeWith(
         tokenScope: string | undefined,
@@ -1197,12 +1333,18 @@ describe("OAuthManager", () => {
         );
       });
 
-      // With the grant declared there is nothing to heal, so the ordinary leg
-      // keeps its previous behavior rather than persisting an unconfirmed
-      // request.
-      it("does not persist a request the AS never confirmed while the grant is on", async () => {
+      // #2117 inverted this. #2068 gated the recording on the opt-out being
+      // on, so with the grant declared the ordinary leg persisted nothing;
+      // that gate is gone, and one wire case now has one behavior rather than
+      // two selected by a checkbox. Persisting here records no new claim: the
+      // request is what storage already held, so this rewrites the same value
+      // rather than asserting a scope the AS never mentioned.
+      it("persists the request even with the grant on, matching what storage held", async () => {
         const storage = await completeWith(undefined, undefined);
-        expect(storage.saveScope).not.toHaveBeenCalled();
+        expect(storage.saveScope).toHaveBeenLastCalledWith(
+          SERVER_URL,
+          "mcp offline_access",
+        );
       });
     });
 
@@ -1748,7 +1890,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: {
@@ -1783,10 +1928,9 @@ describe("OAuthManager", () => {
         "auth-code",
         "https://idp.example.com",
       );
-      expect(storageOf(params).saveScope).toHaveBeenCalledWith(
-        SERVER_URL,
-        "mcp tools:read weather:read",
-      );
+      // Persisting the granted scope is the EMA flow's job (saveMintedTokens),
+      // and it is mocked here -- the union reaching its config, asserted just
+      // above, is what this test owns. emaFlow.test.ts covers the persistence.
 
       silentSpy.mockRestore();
       startSpy.mockRestore();

core/auth/ema/emaFlow.ts

@@ -11,6 +11,7 @@ import {
   type EmaResourceContext,
 } from "./resourceContext.js";
 import { exchangeIdJag, redeemIdJagForAccessToken } from "./wire.js";
+import { resolvePersistedScopeAfterGrant } from "../scopes.js";
 
 export interface EmaFlowConfig {
   serverUrl: string;
@@ -35,10 +36,24 @@ function wrapEmaMintError(err: unknown): Error {
   });
 }
 
+/**
+ * A completed EMA mint, and the scope it actually asked for.
+ *
+ * The request is not always `config.scope`: with neither a configured nor a
+ * stored scope, {@link discoverEmaResourceContext} resolves it from the
+ * protected resource metadata's `scopes_supported`, and that is what both
+ * exchanges send. Callers need the resolved value to apply RFC 6749 §5.1 — a
+ * token response that omits `scope` granted exactly what was requested.
+ */
+export interface EmaMintResult {
+  tokens: OAuthTokens;
+  requestedScope?: string;
+}
+
 export async function mintEmaResourceTokens(
   config: EmaFlowConfig,
   resourceContext?: EmaResourceContext,
-): Promise<OAuthTokens> {
+): Promise<EmaMintResult> {
   const ctx =
     resourceContext ??
     (await discoverEmaResourceContext(
@@ -78,7 +93,7 @@ export async function mintEmaResourceTokens(
     fetchFn: config.fetchFn,
   });
 
-  return redeemIdJagForAccessToken({
+  const tokens = await redeemIdJagForAccessToken({
     resourceAsUrl: ctx.resourceAsUrl,
     idJag,
     resourceClientId: config.resourceClientId,
@@ -87,6 +102,31 @@ export async function mintEmaResourceTokens(
     scope: ctx.scope,
     fetchFn: config.fetchFn,
   });
+  return { tokens, requestedScope: ctx.scope };
+}
+
+/**
+ * Save a mint's tokens and the scope they carry, together.
+ *
+ * Every EMA mint ends here — silent, refresh, and interactive alike — because
+ * all three can be the moment a newly configured or metadata-derived scope
+ * first takes effect. Persisting tokens without the scope leaves the previous
+ * stored value standing as though it described the new token (#2117).
+ */
+async function saveMintedTokens(
+  config: EmaFlowConfig,
+  { tokens, requestedScope }: EmaMintResult,
+): Promise<void> {
+  await config.storage.saveTokens(config.serverUrl, tokens, {
+    enterpriseManaged: true,
+  });
+  const scopeToPersist = resolvePersistedScopeAfterGrant(
+    tokens.scope,
+    requestedScope,
+  );
+  if (scopeToPersist) {
+    await config.storage.saveScope(config.serverUrl, scopeToPersist);
+  }
 }
 
 /** Silent path: cached IdP session + legs 2–3. */
@@ -102,10 +142,7 @@ export async function trySilentEmaAuth(
     return { status: "no_idp_session" };
   }
   try {
-    const tokens = await mintEmaResourceTokens(config);
-    await config.storage.saveTokens(config.serverUrl, tokens, {
-      enterpriseManaged: true,
-    });
+    await saveMintedTokens(config, await mintEmaResourceTokens(config));
     return { status: "success" };
   } catch (err) {
     return { status: "mint_failed", error: wrapEmaMintError(err) };
@@ -128,7 +165,7 @@ export async function completeEmaIdpAuthorizationAndMint(
   config: EmaFlowConfig,
   authorizationCode: string,
   iss?: string,
-): Promise<OAuthTokens> {
+): Promise<EmaMintResult> {
   try {
     await completeIdpOidcAuthorization({
       idp: config.idp,
@@ -145,17 +182,15 @@ export async function completeEmaIdpAuthorizationAndMint(
     });
   }
 
-  let tokens: OAuthTokens;
+  let minted: EmaMintResult;
   try {
-    tokens = await mintEmaResourceTokens(config);
+    minted = await mintEmaResourceTokens(config);
   } catch (err) {
     throw wrapEmaMintError(err);
   }
 
-  await config.storage.saveTokens(config.serverUrl, tokens, {
-    enterpriseManaged: true,
-  });
-  return tokens;
+  await saveMintedTokens(config, minted);
+  return minted;
 }
 
 /** Re-run legs 2–3 on 401 when IdP session is still valid. */
@@ -170,9 +205,7 @@ export async function refreshEmaResourceTokens(
   if (!idToken) {
     return undefined;
   }
-  const tokens = await mintEmaResourceTokens(config);
-  await config.storage.saveTokens(config.serverUrl, tokens, {
-    enterpriseManaged: true,
-  });
-  return tokens;
+  const minted = await mintEmaResourceTokens(config);
+  await saveMintedTokens(config, minted);
+  return minted.tokens;
 }

core/mcp/oauthManager.ts

@@ -331,26 +331,13 @@ export class OAuthManager {
       );
     }
 
-    // #2068 — record the filtered request so `completeOAuthFlow` can persist
-    // it. `resolvePersistedScopeAfterGrant` falls back to the requested scope
-    // when the token response omits `scope` (RFC 6749 §5.1: omitted means the
-    // grant matched the request), and without this that fallback is `undefined`
-    // on the ordinary leg — only step-up ever set it. The old
-    // `offline_access` would then stay in storage, so the scope never
-    // self-heals and later satisfaction checks still read it as granted.
-    //
-    // Only when the filter is in effect. With the grant declared there is
-    // nothing to heal, and persisting a request the AS did not confirm would be
-    // a behavior change beyond this setting.
-    if (this.oauthConfig.requestRefreshToken === false && requestedScope) {
-      this.pendingAuthorizationScope = requestedScope;
-    }
-
     const capturedUrl = provider.getCapturedAuthUrl();
     if (!capturedUrl) {
       throw new Error("Failed to capture authorization URL");
     }
 
+    this.recordAuthorizationRequestScope(capturedUrl, requestedScope);
+
     const stateParam = capturedUrl.searchParams.get("state");
     if (stateParam && this.params.onBeforeOAuthRedirect) {
       const parsedState = parseOAuthState(stateParam);
@@ -369,6 +356,36 @@ 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.
+   *
+   * This composes with the #2068 refresh-token opt-out rather than defeating
+   * it. The SDK's augmentation is gated on `clientMetadata.grant_types`
+   * including `refresh_token`, which the provider drops when the opt-out is
+   * on — so with the grant declined the URL carries the *filtered* scope, and
+   * that is what gets persisted. #2068 recorded the same value here behind a
+   * `requestRefreshToken === false` gate; reading the URL makes the gate
+   * unnecessary, so one wire case now has one persistence behavior instead of
+   * two selected by a checkbox (#2117).
+   */
+  private recordAuthorizationRequestScope(
+    authorizationUrl: URL,
+    requestedScope: string | undefined,
+  ): void {
+    this.pendingAuthorizationScope =
+      authorizationUrl.searchParams.get("scope")?.trim() || requestedScope;
+  }
+
   async completeOAuthFlow(
     authorizationCode: string,
     iss?: string,
@@ -380,22 +397,13 @@ export class OAuthManager {
         const config = scopeForMint
           ? { ...emaConfig, scope: scopeForMint }
           : emaConfig;
-        const tokens = await completeEmaIdpAuthorizationAndMint(
+        // The EMA flow persists the granted scope alongside the tokens, for
+        // every mint rather than only this one — see saveMintedTokens.
+        const { tokens } = await completeEmaIdpAuthorizationAndMint(
           config,
           authorizationCode,
           iss,
         );
-        const requestedScope = this.pendingAuthorizationScope;
-        const scopeToPersist = resolvePersistedScopeAfterGrant(
-          tokens.scope,
-          requestedScope,
-        );
-        if (scopeToPersist) {
-          await this.requireStorage().saveScope(
-            this.getServerUrl(),
-            scopeToPersist,
-          );
-        }
         this.pendingAuthorizationScope = undefined;
         const completedAt = Date.now();
         this.oauthFlowState = {
@@ -892,9 +900,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(