Implementation for SEP-2350 Client-side scope accumulation in step-up authorization (modelcontextprotocol#1591)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Tarek Mahmoud Sayed <tarekms@microsoft.com>

Author: 3 people

.github/skills/run-conformance-from-branch/SKILL.md

@@ -0,0 +1,76 @@
+---
+name: run-conformance-from-branch
+description: Run MCP conformance tests in the C# SDK against a conformance branch (including forks) instead of the published npm version, then restore pinned dependencies.
+compatibility: Requires npm, node, and dotnet SDK. Uses the csharp-sdk repo package.json/package-lock.json and tests/ModelContextProtocol.AspNetCore.Tests.
+---
+
+# Run Conformance From Branch
+
+Run C# SDK conformance tests against an unpublished `modelcontextprotocol/conformance` branch (including branches in forks).
+
+## Use Cases
+
+- Validate a conformance PR before it is published to npm
+- Validate C# SDK behavior against a fork with custom scenario changes
+- Reproduce failures caused by conformance changes
+
+## Safety / Repo Hygiene
+
+1. Start from a clean git state.
+2. Commit or stash local changes first.
+3. Restore pinned dependencies when done (`npm ci`).
+
+## Inputs
+
+- **Source type**: `upstream-branch` or `fork-branch`
+- **Source locator**:
+  - Upstream branch: `modelcontextprotocol/conformance#<branch>`
+  - Fork branch: `<owner>/conformance#<branch>`
+- **Scenario** (optional): e.g. `auth/scope-step-up`
+
+## Workflows
+
+### A) Install directly from GitHub branch (upstream or fork)
+
+From `csharp-sdk` root:
+
+```bash
+npm install --no-save @modelcontextprotocol/conformance@github:<owner>/conformance#<branch>
+```
+
+Examples:
+
+```bash
+npm install --no-save @modelcontextprotocol/conformance@github:modelcontextprotocol/conformance#main
+npm install --no-save @modelcontextprotocol/conformance@github:myuser/conformance#sep-2350-check
+```
+
+## Run Tests
+
+### Run client conformance tests with dotnet test filter:
+
+```bash
+dotnet test tests/ModelContextProtocol.AspNetCore.Tests/ModelContextProtocol.AspNetCore.Tests.csproj -f net10.0 --filter "FullyQualifiedName~ClientConformanceTests"
+```
+
+### Run server conformance tests with dotnet test filter:
+
+```bash
+dotnet test tests/ModelContextProtocol.AspNetCore.Tests/ModelContextProtocol.AspNetCore.Tests.csproj -f net10.0 --filter "FullyQualifiedName~ServerConformanceTests"
+```
+
+## Reporting
+
+Always report:
+
+1. Installed conformance source (`npm ls @modelcontextprotocol/conformance --depth=0`)
+2. Scenario results (pass/fail/warnings)
+3. Any new check IDs observed (for traceability)
+
+## Cleanup / Restore
+
+Return repo to pinned dependency state:
+
+```bash
+npm ci
+```

src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs

@@ -48,6 +48,14 @@ internal sealed partial class ClientOAuthProvider : McpHttpClient
     private string? _tokenEndpointAuthMethod;
     private ITokenCache _tokenCache;
     private AuthorizationServerMetadata? _authServerMetadata;
+    // The accumulated scope set lives for this provider's lifetime and is intentionally not keyed by
+    // resource or authorization server. This is safe today because one ClientOAuthProvider is created
+    // per HttpClientTransport, i.e. per endpoint/resource. If a provider were ever reused across
+    // multiple resources or auth servers, accumulated scopes could be sent to a server that rejects
+    // them (invalid_scope). Accumulation is scoped per "resource and operation" combination (SEP-2350).
+    private readonly HashSet<string> _accumulatedScopes = new(StringComparer.Ordinal);
+    private readonly object _scopeAccumulatorLock = new();
+    private bool _hasAttemptedStepUp;
 
     /// <summary>
     /// Initializes a new instance of the <see cref="ClientOAuthProvider"/> class using the specified options.
@@ -245,6 +253,28 @@ private async Task<string> GetAccessTokenAsync(HttpResponseMessage response, boo
             ThrowFailedToHandleUnauthorizedResponse("No authorization servers found in authentication challenge");
         }
 
+        // SEP-2350: A step-up may legitimately introduce new scopes, so at least one interactive
+        // (re-)authorization attempt is always allowed. However, once a step-up has already been
+        // attempted, a subsequent insufficient_scope challenge that introduces no scope beyond those
+        // already requested cannot make progress by re-running authorization. Treat that repeated,
+        // unproductive challenge as a permanent authorization failure instead of prompting the user
+        // again for the same resource and operation combination.
+        if (response.StatusCode == System.Net.HttpStatusCode.Forbidden)
+        {
+            bool introducesNewScopes = ChallengeIntroducesNewScopes(protectedResourceMetadata);
+            lock (_scopeAccumulatorLock)
+            {
+                if (_hasAttemptedStepUp && !introducesNewScopes)
+                {
+                    ThrowFailedToHandleUnauthorizedResponse(
+                        "A repeated insufficient_scope challenge added no scope beyond those already requested, " +
+                        "so step-up authorization cannot satisfy the request.");
+                }
+
+                _hasAttemptedStepUp = true;
+            }
+        }
+
         // Convert string URIs to Uri objects for the selector
         List<Uri> authServerUris = [];
         foreach (var serverUriString in availableAuthorizationServers)
@@ -729,17 +759,93 @@ private async Task PerformDynamicClientRegistrationAsync(
     }
 
     private string? GetScopeParameter(ProtectedResourceMetadata protectedResourceMetadata)
+    {
+        // Determine the scopes for the current operation from the challenge or metadata.
+        var currentOperationScopes = GetCurrentOperationScopes(protectedResourceMetadata);
+
+        if (currentOperationScopes.Count == 0)
+        {
+            lock (_scopeAccumulatorLock)
+            {
+                // If we have previously requested scopes but nothing new, return the accumulated set.
+                return _accumulatedScopes.Count > 0
+                    ? string.Join(" ", _accumulatedScopes.OrderBy(s => s, StringComparer.Ordinal))
+                    : null;
+            }
+        }
+
+        // Per SEP-2350: Compute the union of previously requested scopes and newly challenged scopes
+        // to avoid losing permissions needed for other operations during step-up authorization.
+        // Note: the accumulator stores only server-challenged / scopes_supported / configured scopes.
+        // offline_access (AugmentScopeWithOfflineAccess) and any ScopeSelector are applied per request
+        // in ComputeEffectiveScope and are intentionally not accumulated, so the selector always sees
+        // the full union and the operation stays idempotent.
+        lock (_scopeAccumulatorLock)
+        {
+            foreach (var scope in currentOperationScopes)
+            {
+                _accumulatedScopes.Add(scope);
+            }
+
+            // Sort scopes for stable, deterministic output (scopes are unordered per RFC 6749 §3.3).
+            return string.Join(" ", _accumulatedScopes.OrderBy(s => s, StringComparer.Ordinal));
+        }
+    }
+
+    /// <summary>
+    /// Determines the scopes required for the current operation, preferring the <c>WWW-Authenticate</c>
+    /// challenge scope, then <c>scopes_supported</c> from the protected resource metadata, then the
+    /// configured scopes. Returns the individual scope tokens so callers can compare and accumulate them
+    /// without re-joining and re-splitting. This does not mutate the accumulated scope set.
+    /// </summary>
+    private IReadOnlyList<string> GetCurrentOperationScopes(ProtectedResourceMetadata protectedResourceMetadata)
     {
         if (!string.IsNullOrEmpty(protectedResourceMetadata.WwwAuthenticateScope))
         {
-            return protectedResourceMetadata.WwwAuthenticateScope;
+            return SplitScopes(protectedResourceMetadata.WwwAuthenticateScope!);
         }
-        else if (protectedResourceMetadata.ScopesSupported.Count > 0)
+
+        var scopesSupported = protectedResourceMetadata.ScopesSupported;
+        if (scopesSupported.Count > 0)
         {
-            return string.Join(" ", protectedResourceMetadata.ScopesSupported);
+            // scopes_supported is already a list of individual scopes; avoid join/split round-tripping.
+            return scopesSupported as IReadOnlyList<string> ?? [.. scopesSupported];
         }
 
-        return _configuredScopes;
+        return _configuredScopes is null ? [] : SplitScopes(_configuredScopes);
+    }
+
+    private static string[] SplitScopes(string scopes) =>
+        scopes.Split(new[] { ' ' }, StringSplitOptions.RemoveEmptyEntries);
+
+    /// <summary>
+    /// Returns <see langword="true"/> if the current challenge requires at least one scope that has not
+    /// already been requested in a previous (re-)authorization. The caller combines this with step-up
+    /// attempt tracking: per SEP-2350, a step-up that adds a new scope is always allowed, but once a
+    /// step-up has been attempted, a later challenge that adds no new scope is treated as a permanent
+    /// failure because re-running interactive authorization cannot make progress.
+    /// </summary>
+    private bool ChallengeIntroducesNewScopes(ProtectedResourceMetadata protectedResourceMetadata)
+    {
+        var currentOperationScopes = GetCurrentOperationScopes(protectedResourceMetadata);
+        if (currentOperationScopes.Count == 0)
+        {
+            // No concrete scope to request, so a re-authorization cannot add anything new.
+            return false;
+        }
+
+        lock (_scopeAccumulatorLock)
+        {
+            foreach (var scope in currentOperationScopes)
+            {
+                if (!_accumulatedScopes.Contains(scope))
+                {
+                    return true;
+                }
+            }
+        }
+
+        return false;
     }
 
     /// <summary>

src/ModelContextProtocol.Core/Authentication/ProtectedResourceMetadata.cs

@@ -190,7 +190,8 @@ public sealed class ProtectedResourceMetadata
     /// The scopes included in the WWW-Authenticate challenge MAY match scopes_supported, be a subset or superset of it,
     /// or an alternative collection that is neither a strict subset nor superset. Clients MUST NOT assume any particular
     /// set relationship between the challenged scope set and scopes_supported. Clients MUST treat the scopes provided
-    /// in the challenge as authoritative for satisfying the current request.
+    /// in the challenge as authoritative for the current operation. When re-authorizing, clients SHOULD include these
+    /// scopes alongside any previously granted scopes to avoid losing permissions needed for other operations (SEP-2350).
     ///
     /// https://modelcontextprotocol.io/specification/2025-11-25/basic/authorization#protected-resource-metadata-discovery-requirements
     /// </summary>