fix: accumulate OAuth scopes on 401/403 instead of overwriting

[rechedev9]

Summary

• Replaces raw this._scope = scope assignments with a mergeScopes() utility that unions existing and incoming scope strings (space-separated, Set-based deduplication, insertion-order stable)
• Fixes both StreamableHTTPClientTransport (lines 520, 553) and SSEClientTransport (lines 167, 281)
• Prevents infinite re-authorization loops when an MCP server uses per-operation progressive authorization per RFC 6750 §3.1

Root cause

The 401/403 handlers overwrote this._scope with only the scope from the current WWW-Authenticate header. With per-operation scopes (e.g., init for initialize, mcp:tools:read for 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:

1. Splits both scope strings on whitespace
2. Unions via Set<string> (deduplicates, preserves insertion order)
3. Returns undefined when the result is empty (matches _scope?: string semantics)

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 compatibility
• sse.test.ts: scope accumulation in EventSource reconnect and send paths

All 265 client tests pass. Typecheck and lint pass.

Fixes #1582

[changeset-bot]

🦋 Changeset detected

Latest commit: 7e2cf80

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@modelcontextprotocol/client	Patch
@modelcontextprotocol/core	Patch

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

[pkg-pr-new]

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@1657

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@1657

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@1657

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@1657

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@1657

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@1657

commit: 7e2cf80

[felixweinberger]

@claude review

[felixweinberger]

seems like this should be in core rather than defined in 2 places.

[claude]

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.

[rechedev9]

Changes in this update

Rebased on upstream/main and reworked the implementation + tests:

Source changes

• 5 merge sites (vs 4 in the original): also fixed the _startOrAuthSse 401 handler at streamableHttp.ts:240 which was overwriting scopes identically to the other sites
• mergeScopes() remains duplicated in each transport file (no shared module), matching the original design intent

Test improvements

• Refactored streamableHttp.test.ts tests to use 403 paths instead of 401 paths for scope accumulation assertions — the 401 path goes through handleOAuthUnauthorized → auth() as an internal ESM call, which vi.spyOn on the module namespace cannot intercept
• Used minimal AuthProvider (not OAuthClientProvider) in the SSE test to avoid adaptOAuthProvider wrapping, allowing direct UnauthorizedError-based scope inspection
• Extracted shared test helpers within the mergeScopes describe block: testMessage, getScope()/setScope(), sortedTokens(), and beforeEach/afterEach for the auth spy — reducing boilerplate

Verification

• 319/319 client tests pass
• Typecheck clean

[felixweinberger]

@claude review

[rechedev9]

Pushed follow-up commit 731ffae.

Addressed the latest review comments and the adjacent 401 metadata issue:

• preserve resourceMetadataUrl when a later 401 omits resource_metadata
• add @modelcontextprotocol/core to the changeset because mergeScopes is now exported there
• make the auth spies restore in streamableHttp tests deterministic with try/finally
• add regression coverage for repeated 401s in both streamableHttp and sse

Verified locally and again via the pre-push hook:

• focused client/core tests
• pnpm -r typecheck
• pnpm -r build
• pnpm -r lint

[felixweinberger]

@claude review

[felixweinberger]

Back to your queue to address claude's finding above, looks legit to me.