From fef8f5ca5aef1130efd2fc3f3d9b36832cd8aa73 Mon Sep 17 00:00:00 2001
From: cliffhall <cliff@futurescale.com>
Date: Mon, 24 Aug 2026 23:35:00 -0400
Subject: [PATCH] fix(oauth): pass the raw challenge scopes to the step-up
 scope filter
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Copilot review round 10 on #2114 — a defect in round 8's fix, and the
precise loop that fix's extra argument was added to prevent.

`enrichChallengeWithScopes` narrows `requiredScopes` to the *missing*
subset, so a challenge requiring `offline_access tools:write` against a
stored scope that already carries `offline_access` arrives as just
`["tools:write"]`. Passing that narrowed list made the filter read the
server's explicit requirement as inherited and strip it, so
re-authorization would earn the same challenge forever.

Passes `challenge.requiredScopes` — the raw list — instead. The mixed
case is now covered, and fails against the narrowed list.

Also stops the checkbox promising more than the setting delivers: it
does not register anything for a preconfigured or CIMD client, and the
authorization server decides issuance either way. It now says it
declares the grant so the server *may* issue a refresh token.

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>
---
 .../ServerSettingsForm/ServerSettingsForm.tsx  |  2 +-
 .../web/src/test/core/mcp/oauthManager.test.ts | 18 ++++++++++++++++++
 core/mcp/oauthManager.ts                       | 10 +++++++++-
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx
index 69e3b87e..fec3f25a 100644
--- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx
+++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx
@@ -913,7 +913,7 @@ export function ServerSettingsForm({
               />
               <Checkbox
                 label="Request refresh token"
-                description={`Registers the refresh_token grant, so access tokens renew without a new sign-in. Uncheck it for authorization servers that reject the offline_access scope and forced consent prompt the grant brings with it. Applies on the next connect.${refreshTokenEmaNote}`}
+                description={`Declares the refresh_token grant, so the authorization server may issue a refresh token and renew access tokens without a new sign-in. Uncheck it for authorization servers that reject the offline_access scope and forced consent prompt the grant brings with it. Applies on the next connect.${refreshTokenEmaNote}`}
                 checked={settings.oauthRequestRefreshToken ?? true}
                 onChange={handleRequestRefreshTokenChange}
               />
diff --git a/clients/web/src/test/core/mcp/oauthManager.test.ts b/clients/web/src/test/core/mcp/oauthManager.test.ts
index eadca821..76a01e88 100644
--- a/clients/web/src/test/core/mcp/oauthManager.test.ts
+++ b/clients/web/src/test/core/mcp/oauthManager.test.ts
@@ -1213,6 +1213,24 @@ describe("OAuthManager", () => {
         );
         expect(sent?.split(/\s+/)).toContain("offline_access");
       });
+
+      // The mixed case, and the one that made the guard above insufficient:
+      // `enrichChallengeWithScopes` narrows `requiredScopes` to the *missing*
+      // subset, so a challenge requiring both — against a stored scope that
+      // already carries `offline_access` — arrives as just `["tools:write"]`.
+      // Reading the narrowed list treats the server's explicit requirement as
+      // inherited and strips it, and re-authorization then earns the same
+      // challenge forever.
+      it("preserves a required offline_access when the challenge also names a missing scope", async () => {
+        const sent = await scopeSentForChallenge(
+          "mcp offline_access",
+          "mcp",
+          ["offline_access", "tools:write"],
+          false,
+        );
+        expect(sent?.split(/\s+/)).toContain("offline_access");
+        expect(sent?.split(/\s+/)).toContain("tools:write");
+      });
     });
 
     it("short-circuits handleAuthChallenge when scope already satisfied", async () => {
diff --git a/core/mcp/oauthManager.ts b/core/mcp/oauthManager.ts
index 0f0b4727..c80033a5 100644
--- a/core/mcp/oauthManager.ts
+++ b/core/mcp/oauthManager.ts
@@ -804,7 +804,15 @@ export class OAuthManager {
         ? scopeForDeclinedRefreshGrant(
             requestedScopeForAuth,
             this.oauthConfig.scope,
-            enriched.requiredScopes,
+            // The RAW challenge scopes, not `enriched.requiredScopes`:
+            // `enrichChallengeWithScopes` narrows that to the *missing* subset,
+            // so a challenge requiring `offline_access tools:write` against a
+            // stored scope that already has `offline_access` arrives here as
+            // just `["tools:write"]`. Passing the narrowed list would read the
+            // server's explicit requirement as inherited and strip it — then
+            // re-authorization earns the same challenge forever, which is the
+            // loop this argument exists to prevent.
+            challenge.requiredScopes,
           )
         : requestedScopeForAuth;