Skip to content

Commit 8945778

Browse files
author
g97iulio1609
committed
fix: accumulate OAuth scopes across 401/403 responses for progressive authorization
Replace scope overwrite with union-based accumulation in OAuthClientProvider. Both the 401 and 403 (insufficient_scope) handlers now merge new scopes with previously-granted scopes via merge_scopes(), preventing infinite re-authorization loops when a server uses per-operation scopes. Companion fix to modelcontextprotocol/typescript-sdk#1604. Closes modelcontextprotocol/typescript-sdk#1582
1 parent 62575ed commit 8945778

File tree

3 files changed

+77
-11
lines changed

3 files changed

+77
-11
lines changed

src/mcp/client/auth/oauth2.py

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
handle_registration_response,
3535
handle_token_response_scopes,
3636
is_valid_client_metadata_url,
37+
merge_scopes,
3738
should_use_client_metadata_url,
3839
)
3940
from mcp.client.streamable_http import MCP_PROTOCOL_VERSION
@@ -570,12 +571,13 @@ async def async_auth_flow(self, request: httpx.Request) -> AsyncGenerator[httpx.
570571
else:
571572
logger.debug(f"OAuth metadata discovery failed: {url}")
572573

573-
# Step 3: Apply scope selection strategy
574-
self.context.client_metadata.scope = get_client_metadata_scopes(
574+
# Step 3: Apply scope selection strategy, merging with existing scopes
575+
new_scope = get_client_metadata_scopes(
575576
extract_scope_from_www_auth(response),
576577
self.context.protected_resource_metadata,
577578
self.context.oauth_metadata,
578579
)
580+
self.context.client_metadata.scope = merge_scopes(self.context.client_metadata.scope, new_scope)
579581

580582
# Step 4: Register client or use URL-based client ID (CIMD)
581583
if not self.context.client_info:
@@ -619,10 +621,11 @@ async def async_auth_flow(self, request: httpx.Request) -> AsyncGenerator[httpx.
619621
# Step 2: Check if we need to step-up authorization
620622
if error == "insufficient_scope": # pragma: no branch
621623
try:
622-
# Step 2a: Update the required scopes
623-
self.context.client_metadata.scope = get_client_metadata_scopes(
624+
# Step 2a: Update the required scopes, merging with existing
625+
new_scope = get_client_metadata_scopes(
624626
extract_scope_from_www_auth(response), self.context.protected_resource_metadata
625627
)
628+
self.context.client_metadata.scope = merge_scopes(self.context.client_metadata.scope, new_scope)
626629

627630
# Step 2b: Perform (re-)authorization and token exchange
628631
token_response = yield await self._perform_authorization()

src/mcp/client/auth/utils.py

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,23 @@ def get_client_metadata_scopes(
119119
return None
120120

121121

122+
def merge_scopes(existing: str | None, incoming: str | None) -> str | None:
123+
"""Merge OAuth scopes by computing the union of space-delimited scope strings.
124+
125+
Per RFC 6749 §3.3, scopes are space-delimited, case-sensitive strings.
126+
This prevents the infinite re-authorization loop that occurs when a server
127+
uses per-operation scopes and the client overwrites previously-granted scopes.
128+
"""
129+
if not incoming:
130+
return existing
131+
if not existing:
132+
return incoming
133+
134+
existing_set = set(existing.split())
135+
existing_set.update(incoming.split())
136+
return " ".join(sorted(existing_set))
137+
138+
122139
def build_oauth_authorization_server_metadata_discovery_urls(auth_server_url: str | None, server_url: str) -> list[str]:
123140
"""Generate an ordered list of URLs for authorization server metadata discovery.
124141

tests/client/test_auth.py

Lines changed: 53 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1327,7 +1327,7 @@ async def test_403_insufficient_scope_updates_scope_from_header(
13271327
mock_storage: MockTokenStorage,
13281328
valid_tokens: OAuthToken,
13291329
):
1330-
"""Test that 403 response correctly updates scope from WWW-Authenticate header."""
1330+
"""Test that 403 response correctly accumulates scope from WWW-Authenticate header."""
13311331
# Pre-store valid tokens and client info
13321332
client_info = OAuthClientInformationFull(
13331333
client_id="test_client_id",
@@ -1350,10 +1350,10 @@ async def test_403_insufficient_scope_updates_scope_from_header(
13501350
async def capture_redirect(url: str) -> None:
13511351
nonlocal redirect_captured, captured_state
13521352
redirect_captured = True
1353-
# Verify the new scope is included in authorization URL
1354-
assert "scope=admin%3Awrite+admin%3Adelete" in url or "scope=admin:write+admin:delete" in url.replace(
1355-
"%3A", ":"
1356-
).replace("+", " ")
1353+
# Verify the accumulated scopes are included (original + new)
1354+
decoded = url.replace("%3A", ":").replace("+", " ")
1355+
for s in ["admin:write", "admin:delete", "read", "write"]:
1356+
assert s in decoded, f"Expected scope '{s}' in URL"
13571357
# Extract state from redirect URL
13581358
parsed = urlparse(url)
13591359
params = parse_qs(parsed.query)
@@ -1383,8 +1383,9 @@ async def mock_callback() -> tuple[str, str | None]:
13831383
# Trigger step-up - should get token exchange request
13841384
token_exchange_request = await auth_flow.asend(response_403)
13851385

1386-
# Verify scope was updated
1387-
assert oauth_provider.context.client_metadata.scope == "admin:write admin:delete"
1386+
# Verify scope was accumulated (original "read write" + new "admin:write admin:delete")
1387+
accumulated = set(oauth_provider.context.client_metadata.scope.split())
1388+
assert accumulated == {"admin:delete", "admin:write", "read", "write"}
13881389
assert redirect_captured
13891390

13901391
# Complete the flow with successful token response
@@ -2264,3 +2265,48 @@ async def callback_handler() -> tuple[str, str | None]:
22642265
await auth_flow.asend(final_response)
22652266
except StopAsyncIteration:
22662267
pass
2268+
2269+
2270+
class TestMergeScopes:
2271+
"""Tests for merge_scopes utility function."""
2272+
2273+
def test_merge_none_existing_returns_incoming(self):
2274+
from mcp.client.auth.utils import merge_scopes
2275+
2276+
assert merge_scopes(None, "mcp:tools:read") == "mcp:tools:read"
2277+
2278+
def test_merge_none_incoming_returns_existing(self):
2279+
from mcp.client.auth.utils import merge_scopes
2280+
2281+
assert merge_scopes("init", None) == "init"
2282+
2283+
def test_merge_both_none_returns_none(self):
2284+
from mcp.client.auth.utils import merge_scopes
2285+
2286+
assert merge_scopes(None, None) is None
2287+
2288+
def test_merge_disjoint_scopes(self):
2289+
from mcp.client.auth.utils import merge_scopes
2290+
2291+
result = merge_scopes("init", "mcp:tools:read")
2292+
scopes = set(result.split())
2293+
assert scopes == {"init", "mcp:tools:read"}
2294+
2295+
def test_merge_overlapping_scopes_deduplicates(self):
2296+
from mcp.client.auth.utils import merge_scopes
2297+
2298+
result = merge_scopes("init mcp:tools:read", "mcp:tools:read mcp:tools:write")
2299+
scopes = set(result.split())
2300+
assert scopes == {"init", "mcp:tools:read", "mcp:tools:write"}
2301+
2302+
def test_merge_identical_scopes(self):
2303+
from mcp.client.auth.utils import merge_scopes
2304+
2305+
result = merge_scopes("init", "init")
2306+
assert result == "init"
2307+
2308+
def test_merge_empty_strings(self):
2309+
from mcp.client.auth.utils import merge_scopes
2310+
2311+
assert merge_scopes("init", "") == "init"
2312+
assert merge_scopes("", "init") == "init"

0 commit comments

Comments
 (0)