fix(oauth): filter the step-up scope union too, not just the provider

Copilot review round 8 on #2114.

The provider's `scope` getter is not the only request path. Mid-session
`insufficient_scope` recovery builds its union from *raw* storage in
`enrichChallengeWithScopes` and hands it to `mcpAuth` directly, so it
never reads that getter. An `offline_access` inherited from an earlier
grant therefore reappeared during step-up and the SDK re-added
`prompt=consent` with the setting off — the failure this option exists
to prevent, on the one path that bypassed it. The comment claiming the
getter was "the one seam both readers go through" was wrong, and is
corrected to name both.

`handleAuthChallenge` now applies the same filter, gated on the setting
so the default path is untouched. The forced-retry reuses that value,
so filtering once at the computation site covers it.

`scopeForDeclinedRefreshGrant` gains the scopes a challenge requires,
honored exactly like a configured one: stripping a scope the server is
asking for would loop — re-authorize, earn the same challenge, strip it
again.

Three regressions; the inherited-scope one fails when the filter is
reverted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>

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

@@ -1140,6 +1140,81 @@ describe("OAuthManager", () => {
       ).toBe(false);
     });
 
+    // #2068 round 8 — the step-up path never reads the provider's filtering
+    // `scope` getter: `enrichChallengeWithScopes` unions the *raw* stored scope
+    // with the challenge and `handleAuthChallenge` hands that straight to
+    // `mcpAuth`. So an inherited `offline_access` reappeared here with the
+    // setting off, and the SDK re-added `prompt=consent` — the exact failure
+    // the option exists to prevent, on the one path that bypassed it.
+    describe("step-up scope with the refresh grant declined (#2068)", () => {
+      /** The `scope` this challenge run handed to the SDK. */
+      async function scopeSentForChallenge(
+        storedScope: string,
+        configuredScope: string | undefined,
+        requiredScopes: string[],
+        requestRefreshToken: boolean | undefined,
+      ): Promise<string | undefined> {
+        const params = createMockParams();
+        storageOf(params).getScope.mockResolvedValue(storedScope);
+        storageOf(params).getTokens.mockResolvedValue({
+          access_token: "tok",
+          token_type: "Bearer",
+          scope: storedScope,
+        });
+        storageOf(params).getClientInformation.mockResolvedValue({
+          client_id: "cid",
+        });
+        const manager = new OAuthManager(params);
+        manager.setOAuthConfig({
+          ...(configuredScope !== undefined && { scope: configuredScope }),
+          ...(requestRefreshToken !== undefined && { requestRefreshToken }),
+        });
+        mockedMcpAuth.mockResolvedValue("REDIRECT");
+
+        await manager.handleAuthChallenge({
+          reason: "insufficient_scope",
+          requiredScopes,
+        });
+
+        const call = mockedMcpAuth.mock.calls.at(-1);
+        return (call?.[1] as { scope?: string } | undefined)?.scope;
+      }
+
+      it("drops an inherited offline_access from the step-up union", async () => {
+        const sent = await scopeSentForChallenge(
+          "mcp offline_access",
+          "mcp",
+          ["tools:write"],
+          false,
+        );
+        expect(sent?.split(/\s+/)).not.toContain("offline_access");
+        // The step-up still asks for what the challenge demanded.
+        expect(sent?.split(/\s+/)).toContain("tools:write");
+      });
+
+      it("keeps offline_access in the step-up union while the grant is on", async () => {
+        const sent = await scopeSentForChallenge(
+          "mcp offline_access",
+          "mcp",
+          ["tools:write"],
+          undefined,
+        );
+        expect(sent?.split(/\s+/)).toContain("offline_access");
+      });
+
+      // Stripping a scope the challenge itself requires would loop:
+      // re-authorize, earn the same challenge, strip it again.
+      it("preserves an offline_access the challenge requires", async () => {
+        const sent = await scopeSentForChallenge(
+          "mcp",
+          "mcp",
+          ["offline_access"],
+          false,
+        );
+        expect(sent?.split(/\s+/)).toContain("offline_access");
+      });
+    });
+
     it("short-circuits handleAuthChallenge when scope already satisfied", async () => {
       const params = createMockParams();
       storageOf(params).getTokens.mockResolvedValue({

core/auth/providers.ts

@@ -208,10 +208,15 @@ 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.
+    // #2068: filter at the point of *request*, not in storage. Covers the two
+    // readers that go through this getter — `clientMetadata.scope` below, and
+    // the `scope:` argument `OAuthManager.authenticate` hands to the SDK.
+    //
+    // It is NOT the only request path: mid-session `insufficient_scope` step-up
+    // builds its union from raw storage and passes it to `mcpAuth` directly,
+    // never reading this getter. `OAuthManager.handleAuthChallenge` applies the
+    // same filter there. Both call `scopeForDeclinedRefreshGrant`, so the rule
+    // lives in one place even though it has to be applied twice.
     if (!this.requestRefreshToken) {
       return scopeForDeclinedRefreshGrant(
         this.cachedScope,

core/auth/scopes.ts

@@ -52,14 +52,28 @@ export const OFFLINE_ACCESS_SCOPE = "offline_access";
  * Widening the request with unrelated configured tokens is outside what this
  * setting governs and could push an otherwise-satisfied connection into a
  * step-up authorization.
+ *
+ * `requiredScopes` carries scopes a `403 insufficient_scope` challenge demands
+ * (SEP-2350). They are honored exactly like a configured one: the server is
+ * asking for them, and stripping a scope the challenge requires would loop —
+ * re-authorize, get the same challenge back, strip it again. The step-up path
+ * needs this because it does not read the scope through the provider at all:
+ * it unions the *raw* stored scope with the challenge and hands that to the
+ * SDK directly, so an inherited `offline_access` would otherwise reappear
+ * there with the setting off.
  */
 export function scopeForDeclinedRefreshGrant(
   effectiveScope: string | undefined,
   configuredScope: string | undefined,
+  requiredScopes?: readonly string[],
 ): string | undefined {
   if (!effectiveScope) return effectiveScope;
   const configured = configuredScope?.trim().split(/\s+/) ?? [];
-  if (configured.includes(OFFLINE_ACCESS_SCOPE)) {
+  const required = requiredScopes ?? [];
+  if (
+    configured.includes(OFFLINE_ACCESS_SCOPE) ||
+    required.includes(OFFLINE_ACCESS_SCOPE)
+  ) {
     const tokens = effectiveScope
       .trim()
       .split(/\s+/)
@@ -83,7 +97,7 @@ export function scopeForDeclinedRefreshGrant(
   // silent overwrite of the persisted value, so re-enabling the grant could no
   // longer restore it. Hand back the configured scope instead: it is the right
   // thing to request, and it leaves storage alone.
-  // Cannot itself contain `offline_access`: that case returned unchanged above.
+  // Cannot itself contain `offline_access`: that case returned above.
   const fallback = configuredScope?.trim();
   if (fallback) return fallback;
 

core/mcp/oauthManager.ts

@@ -48,6 +48,7 @@ import {
   isStrictScopeSuperset,
   resolveEffectiveGrantedScope,
   resolvePersistedScopeAfterGrant,
+  scopeForDeclinedRefreshGrant,
 } from "../auth/scopes.js";
 import { stepUpInsufficientScopeMessage } from "../auth/oauthUx.js";
 import type {
@@ -783,13 +784,29 @@ export class OAuthManager {
       resourceMetadataUrl,
     });
 
-    const scopeForAuth =
+    // #2068 — the step-up union is built from *raw* storage
+    // (`enrichChallengeWithScopes`) and handed to the SDK directly, so it never
+    // passes through the provider's filtering `scope` getter. Without this an
+    // `offline_access` inherited from an earlier grant reappears here and the
+    // SDK re-adds `prompt=consent` even with the setting off — the same failure
+    // the option exists to prevent, on the one path that bypasses it. A scope
+    // the challenge itself requires is preserved, since stripping that would
+    // just re-earn the same challenge.
+    const requestedScopeForAuth =
       enriched.reason === "insufficient_scope"
         ? enriched.authorizationScopes?.join(" ")
         : this.oauthConfig.scope?.trim() ||
           (enriched.requiredScopes?.length
             ? enriched.requiredScopes.join(" ")
             : undefined);
+    const scopeForAuth =
+      this.oauthConfig.requestRefreshToken === false
+        ? scopeForDeclinedRefreshGrant(
+            requestedScopeForAuth,
+            this.oauthConfig.scope,
+            enriched.requiredScopes,
+          )
+        : requestedScopeForAuth;
 
     provider.setSuppressAuthorizationNavigation(true);
     let result: Awaited<ReturnType<typeof mcpAuth>>;