fix: accumulate OAuth scopes on 401/403 instead of overwriting#1657
fix: accumulate OAuth scopes on 401/403 instead of overwriting#1657rechedev9 wants to merge 3 commits intomodelcontextprotocol:mainfrom
Conversation
🦋 Changeset detectedLatest commit: c62ccfc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
@claude review |
| * Returns undefined when the resulting set is empty. | ||
| * Preserves insertion order of first occurrence for determinism. | ||
| */ | ||
| function mergeScopes(existing: string | undefined, incoming: string | undefined): string | undefined { |
There was a problem hiding this comment.
seems like this should be in core rather than defined in 2 places.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward scope-accumulation fix with good test coverage.
Extended reasoning...
Overview
This PR modifies 4 scope assignment lines across sse.ts and streamableHttp.ts, replacing direct overwrites (this._scope = scope) with a mergeScopes() call that unions existing and incoming OAuth scopes via a Set. The function is duplicated in both transport files (no new shared module). Eight regression tests cover accumulation across 401/403 sequences, deduplication, undefined-preservation, and circuit-breaker compatibility.
Security risks
The change touches OAuth auth code paths, but the modification is strictly conservative — it accumulates scopes instead of discarding them, which prevents the infinite re-auth loop described in the root cause. The mergeScopes function performs no network I/O and has no injection surface. All three bug reports from the bug hunting system are pre-existing issues that predate this PR.
Level of scrutiny
The actual code change is minimal (4 one-line substitutions plus a 10-line pure utility function duplicated in two files). The logic is easy to verify by inspection: split on whitespace, union via Set, join back. The test suite is thorough and covers the key scenarios. This warrants a standard review rather than deep scrutiny.
Other factors
- The changeset file is properly formatted for a patch release.
- The PR cleanly fixes issue #1582.
- No CODEOWNERS file restrictions appear to apply.
- No outstanding reviewer comments to address.
- The pre-existing bugs (fetchWithInit inconsistency on 403, unconditional resourceMetadataUrl overwrite, SSE missing circuit breaker) are valid observations but are out of scope for this PR and should be tracked separately.
| } | ||
|
|
||
| if (scope) { | ||
| this._scope = scope; | ||
| this._scope = mergeScopes(this._scope, scope); | ||
| } | ||
|
|
||
| if (resourceMetadataUrl) { |
There was a problem hiding this comment.
🟣 Pre-existing bug: The 403 upscoping handler at line 577 passes fetchFn: this._fetch to auth(), while all other call sites (lines 195, 466, 537) correctly pass fetchFn: this._fetchWithInit. This means requestInit options like credentials: "include" or custom headers are silently dropped during 403 upscoping re-auth, though they are correctly applied during 401 re-auth. Since this PR modifies the adjacent scope assignment at line 564, it would be a good opportunity to fix this inconsistency.
Extended reasoning...
What the bug is
In StreamableHTTPClientTransport.send(), there are two re-authentication paths: one for 401 responses and one for 403 insufficient_scope responses. The 401 handler (line 537) correctly passes fetchFn: this._fetchWithInit to auth(), but the 403 handler (line 577) passes fetchFn: this._fetch instead.
How _fetchWithInit differs from _fetch
_fetchWithInit is created via createFetchWithInit(opts?.fetch, opts?.requestInit) in the constructor. It wraps the raw fetch function with the requestInit options provided by the user — this includes credentials, mode, custom headers, and any other RequestInit properties. Using this._fetch bypasses this wrapping entirely.
The specific code path
When a server returns a 403 with WWW-Authenticate: Bearer error="insufficient_scope", the transport enters the upscoping code path starting at line 555. After merging scopes (line 564, modified by this PR) and updating resource metadata, it calls auth() at line 573 with fetchFn: this._fetch. The auth() function uses this fetch for OAuth metadata discovery, token endpoint requests, and other HTTP calls during the authorization flow. All of these calls will lack the requestInit options.
Concrete example
- User creates transport with
requestInit: { credentials: "include" }(needed for cookie-based auth proxies) - Transport sends a request, gets 401 → re-auths using
_fetchWithInit→ cookies are sent → auth succeeds - Transport retries, gets 403
insufficient_scope→ re-auths using_fetch→ cookies are NOT sent → auth may fail - The same user configuration produces different behavior depending on whether the server returns 401 vs 403
Verification across call sites
Every other auth() call in this file uses this._fetchWithInit:
- Line 195:
_authThenStart()→fetchFn: this._fetchWithInit - Line 466:
finishAuth()→fetchFn: this._fetchWithInit - Line 537: 401 handler in
send()→fetchFn: this._fetchWithInit - Line 577: 403 handler in
send()→fetchFn: this._fetch← inconsistent
Impact
Users who configure requestInit with credentials, mode, or custom headers will have those options silently dropped during 403-triggered re-authorization. This could cause auth failures in environments that require these options (e.g., CORS proxies, cookie-based auth). The bug is subtle because it only manifests on 403 upscoping, not on 401 re-auth.
Fix
Change line 577 from fetchFn: this._fetch to fetchFn: this._fetchWithInit to match all other call sites.
|
|
||
| const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response); | ||
| this._resourceMetadataUrl = resourceMetadataUrl; | ||
| this._scope = scope; | ||
| this._scope = mergeScopes(this._scope, scope); |
There was a problem hiding this comment.
🟣 Pre-existing: The 401 handlers unconditionally assign this._resourceMetadataUrl = resourceMetadataUrl (which can be undefined), potentially wiping a previously-set URL when a subsequent 401 omits resource_metadata from its WWW-Authenticate header. The 403 handler already guards against this with if (resourceMetadataUrl) { ... }. The same conditional pattern should be applied to the 401 handlers at sse.ts:169, sse.ts:283, and streamableHttp.ts:530 for consistency with the scope-merging fix in this PR.
Extended reasoning...
What the bug is
The extractWWWAuthenticateParams function returns resourceMetadataUrl as undefined when the WWW-Authenticate header does not contain a resource_metadata parameter. In the 401 handlers across all three locations (sse.ts:169, sse.ts:283, streamableHttp.ts:530), the code unconditionally assigns:
this._resourceMetadataUrl = resourceMetadataUrl;This means if resourceMetadataUrl is undefined, it overwrites whatever was previously stored.
The inconsistency with the 403 handler
The 403 handler in streamableHttp.ts:567-569 already does the right thing:
if (resourceMetadataUrl) {
this._resourceMetadataUrl = resourceMetadataUrl;
}This PR correctly applied mergeScopes() to preserve accumulated scopes across 401/403 responses, but did not apply the analogous defensive pattern to resourceMetadataUrl on the adjacent line.
Step-by-step proof
- Client sends request A to the server. Server responds with
401andWWW-Authenticate: Bearer resource_metadata="http://example.com/.well-known/oauth-protected-resource", scope="read:a". - The 401 handler sets
this._resourceMetadataUrl = new URL("http://example.com/...")andthis._scope = "read:a". Auth proceeds. - Client retries and sends request B. Server responds with
401andWWW-Authenticate: Bearer scope="read:b"(noresource_metadataparameter this time). extractWWWAuthenticateParamsreturns{ resourceMetadataUrl: undefined, scope: "read:b" }.- The 401 handler sets
this._resourceMetadataUrl = undefined(losing the URL from step 2) andthis._scope = mergeScopes("read:a", "read:b") = "read:a read:b"(correctly merged). - The subsequent
auth()call receivesresourceMetadataUrl: undefined, causing it to fall back to default discovery paths instead of using the previously-known resource metadata URL.
Impact
In the progressive authorization scenario this PR specifically targets, losing the resource metadata URL between 401s could cause the OAuth discovery flow to use default well-known paths instead of the server-specified resource metadata URL. This could lead to failed or incorrect auth discovery if the server relies on a non-default resource metadata location.
How to fix
Apply the same conditional pattern used in the 403 handler to all three 401 locations:
if (resourceMetadataUrl) {
this._resourceMetadataUrl = resourceMetadataUrl;
}Pre-existing nature
This inconsistency predates this PR — the unconditional assignment existed before, and this PR does not modify the resourceMetadataUrl lines. However, since the PR touches the immediately adjacent scope assignment lines and is specifically about preserving state across multiple auth challenges, it would be natural to fix this inconsistency here.
packages/client/src/client/sse.ts
Outdated
| if (response.status === 401 && this._authProvider) { | ||
| const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response); | ||
| this._resourceMetadataUrl = resourceMetadataUrl; | ||
| this._scope = scope; | ||
| this._scope = mergeScopes(this._scope, scope); | ||
|
|
||
| const result = await auth(this._authProvider, { | ||
| serverUrl: this._url, |
There was a problem hiding this comment.
🟣 Pre-existing bug: SSEClientTransport.send() (line 297) recursively calls this.send(message) after a 401→auth cycle with no circuit breaker to prevent infinite recursion. StreamableHTTPClientTransport has _hasCompletedAuthFlow protection (line 522) for this exact scenario, but SSE lacks the equivalent guard. A misbehaving server that always returns 401 after successful auth would cause a stack overflow.
Extended reasoning...
The SSEClientTransport.send() method handles 401 responses by calling auth(), and if authorization succeeds, recursively calling return this.send(message) at line 297. There is no guard to prevent this recursion from repeating indefinitely.
In contrast, StreamableHTTPClientTransport.send() has a _hasCompletedAuthFlow boolean (declared at line 162, checked at line 522, set at line 544) that acts as a circuit breaker. After a successful auth flow, the flag is set to true, and if the retry also returns 401, the code throws SdkError with message "Server returned 401 after successful authentication" instead of recursing again. The flag is reset to false on a successful response (line 597).
The SSE transport has no equivalent field or check. The code path is: server returns 401 → auth() succeeds → this.send(message) recurses → server returns 401 again → auth() succeeds again → recurse again → ... until stack overflow.
Step-by-step proof:
- Client calls
transport.send(message). - Server responds with HTTP 401 +
WWW-Authenticateheader. - Code enters the
if (response.status === 401 && this._authProvider)block at line 281. auth()is called and returns"AUTHORIZED".return this.send(message)is called at line 297, re-enteringsend().- Server again responds with HTTP 401 (misbehaving or misconfigured).
- Steps 3-6 repeat with no termination condition.
- Eventually the call stack overflows and the process crashes.
The impact is that a malicious or buggy server can crash the client by continuously returning 401 after successful auth. This is a denial-of-service vector for SSE transport users.
This is a pre-existing issue — the SSE transport never had this protection, and this PR does not introduce or worsen the problem. The PR does touch exactly this code path (changing line 284 from this._scope = scope to mergeScopes), and its stated goal is preventing infinite re-auth loops. It fixes one cause (scope overwrite) but misses this other cause (no circuit breaker for post-auth 401). Note that SSEClientTransport is deprecated in favor of StreamableHTTPClientTransport, which somewhat reduces urgency.
Replace direct this._scope = scope assignments with mergeScopes() that unions existing and incoming scope strings via Set deduplication. Prevents infinite re-auth loops when servers use per-operation progressive authorization (RFC 6750 §3.1). Fixes modelcontextprotocol#1582
rechedev9
force-pushed
the
fix/scope-overwrite-infinite-reauth
branch
from
cd84edd to
a4b48e1
Compare
Changes in this updateRebased on Source changes
Test improvements
Verification
|
2 participants
Summary
this._scope = scopeassignments with amergeScopes()utility that unions existing and incoming scope strings (space-separated,Set-based deduplication, insertion-order stable)StreamableHTTPClientTransport(lines 520, 553) andSSEClientTransport(lines 167, 281)Root cause
The 401/403 handlers overwrote
this._scopewith only the scope from the currentWWW-Authenticateheader. With per-operation scopes (e.g.,initfor initialize,mcp:tools:readfor tools/list), re-authorizing for the new scope dropped coverage of all prior scopes, causing an infinite loop between operations.Approach
A module-level unexported
mergeScopes(existing, incoming)function:Set<string>(deduplicates, preserves insertion order)undefinedwhen the result is empty (matches_scope?: stringsemantics)Duplicated in both transport files to keep each self-contained — no new shared modules or public API surface.
Testing
Added 8 regression tests:
streamableHttp.test.ts: mergeScopes edge cases, multi-scope accumulation across 401→403 sequences, circuit-breaker compatibilitysse.test.ts: scope accumulation in EventSource reconnect and send pathsAll 265 client tests pass. Typecheck and lint pass.
Fixes #1582