feat: introduce minimal AuthProvider interface with OAuthClientProvider adapter#1710
Open
felixweinberger wants to merge 13 commits intomainfrom
Open
feat: introduce minimal AuthProvider interface with OAuthClientProvider adapter#1710felixweinberger wants to merge 13 commits intomainfrom
felixweinberger wants to merge 13 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: a4d268a 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: |
Contributor
Author
|
📌 Commit It's presented as a delta on top of the Who breaks: only users who hand-implement async token() { return (await this.tokens())?.access_token; }Built-in providers ( What stays scoped out: Happy to drop this commit if we prefer the additive path, or drop the earlier commits if we take this one. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
felixweinberger
commented
Mar 19, 2026
| * For OAuth flows, use {@linkcode OAuthClientProvider} which extends this interface, | ||
| * or one of the built-in providers ({@linkcode index.ClientCredentialsProvider | ClientCredentialsProvider} etc.). | ||
| */ | ||
| export interface AuthProvider { |
Contributor
Author
There was a problem hiding this comment.
The core new abstraction
Adds a minimal `() => Promise<string | undefined>` function type as a lightweight alternative to OAuthClientProvider, for scenarios where bearer tokens are managed externally (gateway/proxy patterns, service accounts, API keys). - New TokenProvider type + withBearerAuth(getToken, fetchFn?) helper - New tokenProvider option on StreamableHTTPClientTransport and SSEClientTransport, used as fallback after authProvider in _commonHeaders(). authProvider takes precedence when both set. - On 401 with tokenProvider (no authProvider), transports throw UnauthorizedError — no retry, since tokenProvider() is already called before every request and would likely return the same rejected token. Callers catch UnauthorizedError, invalidate external cache, reconnect. - Exported previously-internal auth helpers for building custom flows: applyBasicAuth, applyPostAuth, applyPublicAuth, executeTokenRequest. - Tests, example, docs, changeset. Zero breakage. Bughunter fleet review: 28 findings submitted, 2 confirmed, both addressed.
felixweinberger
force-pushed
the
fweinberger/auth-composability
branch
from
689e8c4 to
3f4d125
Compare
Transports now accept AuthProvider { token(), onUnauthorized() } instead
of being typed as OAuthClientProvider. OAuthClientProvider extends
AuthProvider, so built-in providers work unchanged — custom
implementations add two methods (both TypeScript-enforced).
Core changes:
- New AuthProvider interface — transports only need token() +
onUnauthorized(), not the full 21-member OAuth interface
- OAuthClientProvider extends AuthProvider; onUnauthorized() is
required (not optional) on OAuthClientProvider since OAuth providers
that omit it lose all 401 recovery. The 4 built-in providers
implement both methods, delegating to new handleOAuthUnauthorized
helper.
- Transports call authProvider.token() in _commonHeaders() — one code
path, no precedence rules
- Transports call authProvider.onUnauthorized() on 401, retry once —
~50 lines of inline OAuth orchestration removed per transport.
Circuit breaker via _authRetryInFlight (reset in outer catch so
transient onUnauthorized failures don't permanently disable
retries).
- Response body consumption deferred until after the onUnauthorized
branch so custom implementations can read ctx.response.text()
- WWW-Authenticate extraction guarded with headers.has() check
(pre-existing inconsistency; the SSE connect path already did this)
- finishAuth() and 403 upscoping gated on isOAuthClientProvider()
- TokenProvider type + tokenProvider option deleted — subsumed by
{ token: async () => ... } as authProvider
Simple case: { authProvider: { token: async () => apiKey } } — no
class needed, TypeScript structural typing.
auth() and authInternal() (227 LOC of OAuth orchestration) untouched.
They still take OAuthClientProvider. Only the transport/provider
boundary moved.
See docs/migration.md and docs/migration-SKILL.md for before/after.
felixweinberger
force-pushed
the
fweinberger/auth-composability
branch
from
3f4d125 to
2961101
Compare
travisbreaks
left a comment
travisbreaks
left a comment
There was a problem hiding this comment.
Thorough and well-documented refactoring. The AuthProvider abstraction is a clear improvement: the one-liner bearer token pattern ({ token: async () => key }) eliminates the 8-member stub problem that every non-OAuth user hit. Migration docs are excellent.
A few observations on the design:
1. token() called before every request
The docs say transports call token() before every request. If token() involves async work (cache lookup, refresh check, remote call), this adds latency to every single MCP request. Consider:
- Documenting that
token()should be fast (return cached value, refresh in background) - Or adding a
tokenCacheDurationoption so the transport can skip callingtoken()on rapid successive requests
2. Concurrent 401 handling
The _authRetryInFlight circuit breaker prevents infinite retry loops, but what happens when multiple concurrent requests all get 401 simultaneously? If onUnauthorized does a token refresh, N concurrent requests could trigger N parallel refresh flows. Only the first should refresh; others should wait for the result.
This was likely also a problem with the old _hasCompletedAuthFlow approach, but worth addressing in this refactoring if possible. A shared promise that coalesces concurrent onUnauthorized calls would prevent token endpoint flooding.
3. Breaking change surface area
The changeset says @modelcontextprotocol/client: major. Custom OAuthClientProvider implementations must add token() and onUnauthorized(). The migration guide covers this well, but worth considering: could token() have a default implementation on OAuthClientProvider that calls this.tokens()?.access_token? That would reduce the breaking surface to zero for the common case.
4. Type guard pattern
isOAuthClientProvider() is a runtime type guard. Since OAuthClientProvider extends AuthProvider, this is the right approach for gating OAuth-specific features (like finishAuth()). Clean.
5. Exported auth helpers
Exposing applyBasicAuth, applyPostAuth, applyPublicAuth, executeTokenRequest is a good move for custom flow builders. These were previously internal, so worth noting in the docs that they are now part of the public API surface and subject to semver.
Strong PR. The main concern is the concurrent 401 coalescing question.
Alternative to the breaking 'extends AuthProvider' approach. Instead of
requiring OAuthClientProvider implementations to add token() +
onUnauthorized(), the transport constructor classifies the authProvider
option once and adapts OAuth providers via adaptOAuthProvider().
- OAuthClientProvider interface is unchanged from v1
- Transport option: authProvider?: AuthProvider | OAuthClientProvider
- Constructor: if OAuth, store both original (for finishAuth/403) and
adapted (for _commonHeaders/401) — classification happens once, no
runtime type guards in the hot path
- 4 built-in providers no longer need token()/onUnauthorized()
- migration.md/migration-SKILL.md entries removed — nothing to migrate
- Changeset downgraded to minor
Net -142 lines vs the breaking approach. Same transport simplification,
zero migration burden. Duck-typing via isOAuthClientProvider()
('tokens' + 'clientMetadata' in provider) at construction only.
Four fixes from claude[bot] review on the AuthProvider approach: 1. Drain 401 response body after onUnauthorized() succeeds, before the retry. Unconsumed bodies block socket recycling in undici. All three 401 sites now drain before return. 2. _startOrAuthSse() 401 retry was return await, causing onerror to fire twice (recursive call's catch + outer catch both fire). Changed to return (not awaited) matching the send() pattern. Removed the try/finally, added flag reset to success path + outer catch instead. 3. Migration docs still referenced SdkErrorCode.ClientHttpAuthentication for the 401-after-auth case, but that throw site was replaced by _authRetryInFlight which throws UnauthorizedError. Updated both migration.md and migration-SKILL.md. 4. Pre-existing: 403 upscoping auth() call passed this._fetch instead of this._fetchWithInit, dropping custom requestInit options during token requests. All other auth() calls in this transport already used _fetchWithInit.
The 401-after-re-auth case (circuit breaker trips) should throw a distinct error from the normal 'token rejected' case: - First 401 with no onUnauthorized → UnauthorizedError — caller re-auths externally and reconnects - Second 401 after onUnauthorized succeeded → SdkError with ClientHttpAuthentication — server is misbehaving, don't blindly retry, escalate The previous commit collapsed these into UnauthorizedError, which risks callers catching it, re-authing, and looping. Restored the SdkError throw at all three 401 sites when _authRetryInFlight is already set. Reverted migration doc changes — ClientHttpAuthentication is not dead code.
Three fixes from claude[bot] review: 1. _startOrAuthSse 405 return doesn't reset _authRetryInFlight — 401 → onUnauthorized → retry → 405 would leave the flag set forever, disabling onUnauthorized for subsequent send() 401s. Added reset before the 405 return. 2. SSE onerror handler — when onUnauthorized rejects in the connect path, the error went through .then(resolve, reject) without calling this.onerror. Every other error path in both transports calls both. Added this.onerror?.() before reject. 3. 401 with no authProvider drained body twice — the 401 block ran unconditionally, drained at line 232/285/521, fell through (no authProvider = no throw), then the generic error handler drained again (empty) and produced 'HTTP 401: null'. Gated the entire 401 block on this._authProvider (matching pre-PR structure) so no-auth 401s hit the generic error directly with intact body text.
felixweinberger
force-pushed
the
fweinberger/auth-composability
branch
from
f008255 to
6291f32
Compare
Demonstrates two auth setups through the same authProvider slot:
MODE A (host-managed): Enclosing app owns the token. Minimal
AuthProvider with { token, onUnauthorized } — onUnauthorized signals
the host UI and throws instead of refreshing, since the host owns
the token lifecycle.
MODE B (user-configured): OAuth credentials supplied directly. Passes
a ClientCredentialsProvider; transport adapts it to AuthProvider via
adaptOAuthProvider (synthesizing token()/onUnauthorized()).
Same connectAndList() caller code for both — the transport abstracts
the difference. Validates the decomposition holds with zero branching
in user code.
Check typeof === 'function' on two required methods (tokens + clientInformation) instead of bare 'in' operator. Slightly more robust — verifies they're actually callable, not just properties with those names. Same semantics, reads cleaner.
felixweinberger
force-pushed
the
fweinberger/auth-composability
branch
from
6291f32 to
ce5703a
Compare
… on retry Two fixes from claude[bot] round-6 review: 1. Shared _authRetryInFlight between _startOrAuthSse() and send() created a race: if the fire-and-forget GET SSE gets 401 and sets the flag while awaiting onUnauthorized(), a concurrent POST send() that also gets 401 would see flag=true and throw ClientHttpAuthentication without ever attempting its own re-auth. The old _hasCompletedAuthFlow was only set in send() — I introduced the regression when adding 401 handling to _startOrAuthSse. Split into _authRetryInFlight (send path) and _sseAuthRetryInFlight (GET-SSE path). 2. Pre-existing: send() 401/403 retries called this.send(message) without forwarding the options parameter, dropping onresumptiontoken on the retried request. Added options to both call sites.
Proof-of-life that both auth shapes work against a real HTTP server:
- MODE A (minimal AuthProvider): { token: () => 'token' } → server
sees Authorization: Bearer token
- MODE A 401: onUnauthorized signals UI and throws → caller sees the
thrown error (the host-managed pattern where the enclosing app
handles reauth)
- MODE B (OAuthClientProvider): passed directly, adapter synthesizes
token() from tokens() → server sees Authorization: Bearer
<access_token>
- Combined: same constructor option slot, same send() call, both
shapes hit the same server
Uses real node:http server (not fetch mocks) to verify the
Authorization header actually reaches the wire.
Removes eslint-disable suppression. process.exitCode = 1 lets the event loop drain before exit; process.exit(1) kills immediately and can cut off pending writes.
felixweinberger
commented
Mar 20, 2026
| }); | ||
| }); | ||
|
|
||
| describe('AuthProvider integration — both modes against a real server', () => { |
Contributor
Author
There was a problem hiding this comment.
@pcarleton: added tests covering both authentication modes here
…ameter Stops the 10-comment whack-a-mole around flag lifecycle. A mutable boolean class field is the wrong primitive for 'retry once per operation' when operations are concurrent and recursive — every reset point creates a race, every missed reset creates a stuck flag. Now all four 401 paths use parameter-passed isAuthRetry: - StreamableHTTP _startOrAuthSse(options, isAuthRetry = false): recursion passes true. No class field, no reset sites. - StreamableHTTP send() delegates to private _send(message, options, isAuthRetry). Recursion passes true. No class field. - SSE _startOrAuth(isAuthRetry = false): onerror callback captures isAuthRetry from closure; retry calls _startOrAuth(true). - SSE send() delegates to private _send(message, isAuthRetry). Per-operation state dies with the stack frame. Concurrent operations cannot observe each other's retry state. 12 reset sites deleted. Also makes SSE onerror fallback consistent with other paths — throws SdkError(ClientHttpAuthentication) for the circuit-breaker case instead of plain UnauthorizedError. Not addressed (noted for auth() cleanup): concurrent 401s still each call onUnauthorized() independently. Deduplicating that (in-flight promise pattern) would be a behavior change.
felixweinberger
changed the title
The isAuthRetry parameter approach works for recursive method calls (send, _startOrAuthSse) but not for the EventSource onerror callback. Passing _startOrAuth(true) on retry permanently captures isAuthRetry=true in the new EventSource's closure — if that EventSource auto-reconnects later (network blip on a long-lived stream) and gets 401, onUnauthorized is skipped and the transport cannot recover. Verified against eventsource lib: non-200 → failConnection (CLOSED, no reconnect); stream end after OPEN → scheduleReconnect → reconnect attempt can get 401 → failConnection → onerror fires. The 'hours later' scenario is real. Fix: retry always calls _startOrAuth() fresh (no parameter). Matches pre-PR _authThenStart() behavior. Trade-off: no circuit breaker on the SSE connect path — if onUnauthorized succeeds but server keeps 401ing, it loops (same as pre-PR). Also fixes double-onerror: two-arg .then(onSuccess, onFail) separates retry failures (inner _startOrAuth already fired onerror) from onUnauthorized failures (not yet reported). Added close + clear _last401Response before retry for hygiene. Two regression tests added, both verified to FAIL against the buggy code: - 401→401→200: onUnauthorized called TWICE, start() resolves - 401→onUnauthorized succeeds→401→onUnauthorized throws: onerror fires ONCE with the thrown error
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
AuthProvider— a minimal two-method interface — as the transport's auth abstraction.OAuthClientProvideris adapted at the transport boundary, so existing code passing OAuth providers is unchanged.Non-breaking.
OAuthClientProviderkeeps its existing shape.adaptOAuthProvider()synthesizestoken()fromtokens()andonUnauthorized()fromhandleOAuthUnauthorized()when the transport receives a full OAuth provider.Motivation and Context
OAuthClientProviderassumes an interactive browser-redirect flow. Many deployments don't fit: gateway/proxy patterns, service accounts with pre-provisioned tokens, enterprise SSO where tokens come from a separate pipeline. Today those users either stub out unusedOAuthClientProvidermethods or wrapfetchmanually.The minimal interface covers the transport's actual needs: "give me a token" and "the token was rejected, do something." Everything OAuth-specific (discovery, refresh, redirect) lives in the provider, not the transport.
What changed
Transports (
sse.ts,streamableHttp.ts):AuthProvider | OAuthClientProviderin theauthProvideroptionisOAuthClientProvider()type guard and adapt them in the constructoronUnauthorized()if present, retry once, then give up. Retry state is a stack-localisAuthRetryparameter (not a class field) — per-operation isolation, no lifecycle bugs, no concurrent-op races.onUnauthorized()(not pre-consumed), then drained before retryauth.ts:AuthProviderinterface andUnauthorizedContexttypeadaptOAuthProvider()wraps anOAuthClientProviderinto anAuthProviderhandleOAuthUnauthorized(provider, ctx)is the extractedonUnauthorizedimplementation for OAuth — discovery, refresh, or redirect as appropriateapplyBasicAuth,applyPostAuth,applyPublicAuth,executeTokenRequestDocs:
docs/client.md—### Token providersection with snippetexamples/client/src/simpleTokenProvider.ts— env-var token patternexamples/client/src/dualModeAuth.ts— same option slot, two provider shapes (host-managed signal-on-401 vs OAuth client credentials)docs/migration.md+docs/migration-SKILL.md— error-mapping tables updated;UnauthorizedErrorreplacesSdkError(ClientHttpAuthentication)for 401-after-auth in docsTests (
packages/client/test/client/tokenProvider.test.ts):node:httpserver — both shapes hit the same server through the same option and theAuthorizationheader arrives correctlyHow Has This Been Tested?
Breaking Changes
None.
OAuthClientProvideris unchanged.authProvideroption widened to a union.Out of scope (noted for the
auth()cleanup doc)Concurrent 401s each call
onUnauthorized()independently (thundering herd). Deduplicating via an in-flight-promise pattern would be a behavior change and belongs in the broaderauth()refactor.Types of changes
Checklist
Earlier drafts preserved in commits
9aea20fb(additiveTokenProvidersidecar) and29611017(breakingextendsapproach). Both superseded by the adapter design above.