fix(client): let OAuth-derived Authorization override caller-supplied header - #2475
Conversation
… header Closes modelcontextprotocol#2208. StreamableHTTPClientTransport and SSEClientTransport both build the outgoing request's headers as: return new Headers({ ...headers, // common: Authorization (OAuth), mcp-session-id, mcp-protocol-version ...extraHeaders // caller-supplied via requestInit.headers }); This meant a caller-supplied Authorization placeholder (e.g. an env-var API key passed through the MCP config) was placed AFTER the SDK-derived common headers, which silently overrode OAuth-computed tokens and broke the auth-refresh flow once the placeholder went stale. Several MCP servers (Atlassian Rovo and others) let both API tokens and OAuth share the same Authorization header and rely on it being valid. Swap the spread order to so SDK-computed common headers (including the freshest OAuth token) take precedence, matching the merge order used elsewhere in the SDK. Caller-supplied non-auth headers still pass through unchanged. Test: a regression test in streamableHttp.test.ts seeds a stale Authorization placeholder in requestInit.headers alongside a working authProvider and asserts the request goes out with the OAuth-derived token. 722 client tests pass; lint + format clean.
🦋 Changeset detectedLatest commit: 86beff1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
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/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
…ntextprotocol#2475) Signed-off-by: sanjibani <18418553+sanjibani@users.noreply.github.com>
claude
Bot
added
the
v2
…ument requestInit precedence - add SSEClientTransport regression test for modelcontextprotocol#2208 (GET + POST) - extend the Streamable HTTP test to the POST path and mcp-protocol-version - add e2e requirement client-auth:authprovider:token-overrides-requestinit and scenario - document transport-managed header precedence on requestInit (JSDoc + migration guide) - tighten the merge-site comments
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the SSE EventSource reconnect wrapper (sse.ts:218-224, which replaces init.headers wholesale) and the per-request options.headers path in send() — both are unchanged by this PR, and the latter already refuses reserved names via RESERVED_REQUEST_HEADER_NAMES, so the new _commonHeaders() precedence is consistent with it rather than introducing a second policy.
Extended reasoning...
The spread-order flip itself is small and matches the PR description in both transports; the changeset and migration-guide prose accurately describe the new precedence. The inline findings (case-sensitivity of the plain-object merge for authorization vs Authorization, the stale placeholder still reaching the AS via _fetchWithInit, and the contradictory eventSourceInit NOTE) are the substantive concerns and are posted separately. Because this touches the Authorization header path on both client transports, a human maintainer should weigh whether the case-sensitivity gap needs to be closed in this PR before merge.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
packages/client/src/client/sse.ts— nit (pre-existing prose, made contradictory by this PR): theeventSourceInitNOTE tells users that anAuthorizationheader is not auto-attached wheneventSourceInitis set and to "work around" it by "setting theAuthorizationheader manually", while the newrequestInitJSDoc directly below says a manualAuthorizationis overridden wheneverauthProvideryields a token. Fix: delete or rewrite the NOTE so it matches_startOrAuth(): the wrappedfetchalways calls_commonHeaders()regardless ofeventSourceInit, so the provider token is attached and a manual header cannot override it. [also at: packages/client/src/client/sse.ts:104 - nit, pre-existing: theeventSourceInitNOTE directly above the newrequestInitJSDoc now contradicts it — it claims that…]Extended reasoning...
sse.ts:210-224 always wraps the EventSource
fetchwith_commonHeaders()(which setsAuthorization: Bearer <token>from the provider) even wheneventSourceInitis supplied, so the claim at lines 100-102 that settingeventSourceInitprevents auto-attachment is false on base already. After this PR, the suggested workaround (setAuthorizationmanually viarequestInit.headers) is exactly the input the new spread order at lines 204-207 overrides when the provider has a token, and the new JSDoc at lines 110-115 says so. A reader following the NOTE will expect their manual header to be sent and instead gets the provider token. No runtime effect; documentation contradiction within the same options type.Verification: nit — docs-only contradiction, triggered for any reader of
SSEClientTransportOptionsJSDoc who setseventSourceInittogether withauthProvider. Mechanism verified: the pre-existing NOTE at /home/claude/typescript-sdk/packages/client/src/client/sse.ts:100-103 says "Setting this property will prevent anAuthorizationheader from being automatically attached to the SSE request ... This can… -
🟣
packages/client/src/client/streamableHttp.ts— pre-existing, security-relevant: the new docs promise a staticAuthorizationplaceholder inrequestInit.headers"falls back to OAuth", but_fetchWithInit(built from the samerequestInithere and at sse.ts:174) sends that placeholder on everyauth()request to the AS origin (metadata, DCR, token endpoint), leaking the API key; withclient_secret_basicthe token request goes out asAuthorization: Bearer <stale>, Basic <creds>and is rejected, so the provider never obtains the token the fallback depends on. Fix: build_fetchWithInitfrom a copy ofrequestInitwith transport-owned headers (authorization,mcp-*) removed, in both transports, or drop the callerauthorizationinsidecreateFetchWithInit. [also at: packages/client/src/client/streamableHttp.ts:192 - Pre-existing, security-relevant: the configuration this JSDoc now endorses (staticAuthorizationin…; packages/client/src/client/sse.ts:174 - For SSEClientTransport the 'static API key falls back to OAuth' promise added in this PR's JSDoc (sse.ts:110-115) and…]Extended reasoning...
Path: constructor line 375
this._fetchWithInit = createFetchWithInit(opts?.fetch, opts?.requestInit)(sse.ts:174 identical).createFetchWithInit(packages/core-internal/src/shared/transport.ts:38-43) merges{...normalizeHeaders(baseInit.headers), ...normalizeHeaders(init.headers)}when the call passes headers, and passesbaseInit.headersverbatim when it does not._fetchWithInitis thefetchFnforonUnauthorized(line 589, sse.ts:246),_stepUpAuthorize(447) andfinishAuth;adaptOAuthProvider->handleOAuthUnauthorized(auth.ts:186-190) forwards it intoauth(), which uses it forfetchWithCorsRetry(auth.ts:1660, PRM + AS metadata discovery,{headers}often undefined so the raw requestInit.headers go out),registerClient(auth.ts:2514) andexecuteTokenRequest(auth.ts:2211). Trigger is exactly the #2208…Verification: pre-existing (security-relevant; base already leaks by the same route — line 375
this._fetchWithInit = createFetchWithInit(opts?.fetch, opts?.requestInit)and sse.ts:174 are untouched by this diff), triggered whenever a caller sets a staticAuthorizationinrequestInit.headersalongside anauthProvider— the exact #2208 setup the PR documents as supported. Mechanism, verified:…
…e; fix stale eventSourceInit note - build _commonHeaders() on `new Headers(requestInit.headers)` and `set()` the transport-managed values in both transports, so Authorization / mcp-session-id / mcp-protocol-version replace a same-named caller entry whatever its spelling. The plain-object spread kept `authorization` and `Authorization` side by side and the Headers constructor combined them into "Bearer <stale>, Bearer <fresh>" for callers passing a Headers instance or a lowercase key - drop the now-unused normalizeHeaders import from both transports - add regression tests for a Headers instance and a lowercase plain object - rewrite the SSEClientTransportOptions.eventSourceInit note: a custom fetch is always wrapped, so the provider token is attached and takes precedence over requestInit - reword the changeset to describe the behavior rather than the old spread order
Thanks, this one is real and I verified it end to end.
It is pre-existing on both Findings 1 and 3 are addressed in 52491ff: |
|
@claude review |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
packages/client/src/client/sse.ts— SSE clients that lose the stream never resume from where they left off: on reconnect theeventsourcelibrary passesLast-Event-IDininit.headers, and the wrapper at sse.ts:218-221 replacesinit.headerswholesale with the object returned by the rewritten_commonHeaders()(line 186), so the resumption header is dropped and the server replays from the start or loses events. Fix:_commonHeaders()(now the single producer of the outgoing Headers) should merge the EventSource-suppliedinit.headers(Last-Event-ID, Accept) into the Headers it returns instead of the wrapper discarding them.Extended reasoning...
Trigger: any SSEClientTransport connection where the network drops and the
eventsourcelibrary auto-reconnects (production rate: every transient disconnect on the legacy HTTP+SSE transport). The library calls the transport'sfetchcallback withinit.headerscontainingLast-Event-ID: <last id>. sse.ts:216 calls_commonHeaders()(rewritten by this PR at lines 180-204 to build the full Headers object), line 217 sets Accept, then lines 218-221 callfetchImpl(url, {...init, headers})— theheaderskey replacesinit.headersentirely, soLast-Event-IDnever reaches the server. The server cannot resume and either replays from the start or the client silently misses events emitted during the gap.I opened the base commit (git show 5ecc791:packages/client/src/client/sse.ts, lines 204-210): the wrapper already did
{...init, headers}with the old merged Headers, so the drop is pre-existing as the dismissing finder said. However this PR rewrites the function that now owns the final Headers object and documents (eventSourceInit JSDoc, lines 97-106) that the custom fetch 'is…Verification: pre-existing. Triggering condition: an SSEClientTransport connected to a third-party legacy-SSE server that emits
id:lines, when theeventsourcelibrary auto-reconnects after a stream drop (the SDK's own legacy server in /home/claude/typescript-sdk/packages/server-legacy/src/sse/sse.ts:95,189 writes onlyevent:/data:and never anid:, so against SDK servers there is no Last-Event-ID…
…ders()
`new Headers(null)` throws where the replaced normalizeHeaders() returned an empty
record, so a JS caller (or a JSON config forwarded verbatim) passing
`requestInit: { headers: null }` failed on every request. Map falsy to undefined in
both transports and add a regression test for each.
Confirmed the mechanism: Keeping it out of this PR. It is unrelated to header precedence and has no coverage here, and the impact is narrow: the SDK's own legacy SSE server never emits |
|
@claude review |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
The latest push addresses both of my earlier threads (case-insensitive Headers.set merge in both transports, and || undefined for a null headers value). Beyond the one inline finding on repeated tuple-array names, I also checked that normalizeHeaders is still consumed by createFetchWithInit in packages/core-internal/src/shared/transport.ts, so dropping the two transport imports leaves no dead export, and that the null-tolerance also holds on the authProvider path since _commonHeaders() is the only place requestInit.headers is read.
Extended reasoning...
This run confirmed from the diff that the two issues raised in prior reviews (case-sensitive plain-object spread producing combined Bearer stale, Bearer fresh values, and new Headers(null) throwing) are fixed in packages/client/src/client/streamableHttp.ts and packages/client/src/client/sse.ts. The remaining inline finding concerns repeated header names in tuple-array form now being comma-joined by the Headers constructor where the old Object.fromEntries kept only the last entry; that is a narrow behavioural difference the author should weigh, and since the change touches the Authorization header path in a security-sensitive transport, a human maintainer should still make the merge call rather than this review approving outright. Additional candidates ruled out: overriding a caller-pinned mcp-session-id/mcp-protocol-version is the documented intent (JSDoc, changeset, migration guide all agree), and normalizeHeaders remains live in createFetchWithInit.
…name combine semantics A repeated name in a tuple array is the one HeadersInit form that can express a multi-valued header. The transports build their headers with the Fetch Headers constructor, so caller duplicates combine exactly as a direct fetch() would, while a repeated transport-managed name (e.g. Authorization) is still replaced outright by the transport's own value. Cover the tuple-array form in the case-insensitivity matrix and pin the combine behaviour on GET and POST for both StreamableHTTPClientTransport and SSEClientTransport.
|
@claude review |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked that dropping the normalizeHeaders import leaves nothing broken (it is still consumed by createFetchWithInit in core-internal and is deliberately not part of the public surface), and that the createFetchWithInit wrapper does not reintroduce caller precedence — its per-call init.headers (the computed Headers) are spread after the base requestInit.headers, so the transport-set values still win on the wire.
Extended reasoning...
Findings are posted inline. This note records two adjacent concerns examined and ruled out this run: the removed normalizeHeaders import has no other broken consumers (packages/core-internal/src/shared/transport.ts:42 still uses it inside createFetchWithInit, and packages/core-internal/src/exports/public/index.ts explicitly excludes it from the public barrel), and the createFetchWithInit merge at that same line puts the per-call headers last, so the precedence flip in _commonHeaders is not undone by the wrapped fetch.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
| // Fetch `Headers` constructor would then combine them into one "stale, fresh" | ||
| // value.) This lets a stale static `Authorization` placeholder (e.g. an env-var | ||
| // API key) fall back to the OAuth token once the provider has one, and mirrors | ||
| // the per-request `RESERVED_REQUEST_HEADER_NAMES` guard in send(). See #2208. |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: The new comment claims _commonHeaders "mirrors the per-request RESERVED_REQUEST_HEADER_NAMES guard in send()", but only 3 of the 7 reserved names are actually overwritten (:473-482); dpop, mcp-method, mcp-name and content-type from requestInit.headers still reach the wire on every notification/legacy POST, the standalone GET and DELETE where nothing re-sets them (_applyBodyDerivedHeaders :494-499 returns early for non-enveloped messages), so a config-injected Mcp-Method/Mcp-Name produces a header/body disagreement the server's SEP-2243 cross-check rejects. …
A small fix can ride a push you are already making; otherwise a short reply is enough.
Extended reasoning...
…Fix: either delete the reserved names from the caller-seeded Headers in _commonHeaders (matching the guard the comment cites) or drop the 'mirrors' claim so the JSDoc/comment do not promise a guard that does not exist.
Path: _commonHeaders() at streamableHttp.ts:464 seeds new Headers(this._requestInit.headers) and then sets only Authorization/mcp-session-id/mcp-protocol-version (:473-482). send() at :983-1001 calls _applyBodyDerivedHeaders (which sets mcp-method/mcp-name only when the message is a single request carrying a PROTOCOL_VERSION_META_KEY envelope, :494-499) and applies the RESERVED_REQUEST_HEADER_NAMES filter (:268-276) only to options.headers, never to the seeded requestInit names. So on notifications/initialized (2nd POST of every connect), batch messages, legacy-era requests, the GET at :550 and DELETE at :1216, a caller-configured dpop/mcp-method/mcp-name goes out verbatim; content-type is overwritten at :1003 only in send(). Population: any host that lets end-user config populate requestInit.headers (examples/cli-client/host/host.ts:470 does…
Verification: pre-existing — triggered only when a caller puts one of the other reserved names (Mcp-Method, Mcp-Name, DPoP without a DPoP session) into the static requestInit.headers; the base branch already forwarded those exact same values by the same route, so this diff neither introduces nor widens the leak — the only thing new is the comment's wording. Mechanism verified.… | pre-existing — the…
…ngeset and comments The released base spread the transport-managed headers first, so a Headers instance or lowercase caller key produced 'Bearer <fresh>, Bearer <stale>', not the reversed order 52491ff described (that order only held for the branch's intermediate caller-first swap). State the real base output in the changeset and drop the ordering claim from the inline and test comments, which describe a hypothetical spread whose order is not fixed.
|
@claude review |
| if (token) { | ||
| headers['Authorization'] = `Bearer ${token}`; | ||
| headers.set('Authorization', `Bearer ${token}`); | ||
| } |
There was a problem hiding this comment.
🟡 (optional) Hosts that configure a working static API key in requestInit.headers.Authorization and also attach an OAuthClientProvider whose store still holds an old (expired, refresh-revoked) token now get a 401 on every connect and are pushed into a browser OAuth redirect / UnauthorizedError, where the base branch sent the API key and connected. Fix: precedence for the provider token must not disable the static credential for callers whose provider token is rejected — e.g. on a 401 with a caller-supplied Authorization, retry once with the caller value before escalating to onUnauthorized, or only override when the provider's token() is known-fresh. Path: _commonHeaders :473-475 unconditionally sets the provider token over the configured one.
Extended reasoning...
Trigger (external config): a host that wires a user's static Authorization: Bearer <api-key> from MCP config into requestInit.headers (examples/cli-client/host/host.ts does exactly this) and always passes an OAuthClientProvider for remote servers. If that provider's store holds tokens from an earlier OAuth session with the same server (user later switched to an API key, or the AS revoked the refresh token), adaptOAuthProvider().token() (packages/client/src/client/auth.ts:224-227) returns tokens?.access_token regardless of expiry. Base (git show 5ecc791:packages/client/src/client/streamableHttp.ts 455-467): {...headers, ...extraHeaders} let the caller's valid API key win, request succeeded, no 401, OAuth machinery never ran. New code streamableHttp.ts:473-475 (headers.set('Authorization', Bearer ${token})) and sse.ts:201-203 send the dead OAuth token on initialize, the standalone GET (:550) and every POST (:983). Server answers 401 -> onUnauthorized -> auth() attempts refresh; with a revoked refresh token auth() returns REDIRECT -> UnauthorizedError thrown /…
Verification: nit — conflicts with stated purpose: triggered only when a host hands the same transport BOTH a valid static Authorization in requestInit.headers AND an OAuthClientProvider whose store still holds dead tokens for that server (expired access token with no/revoked refresh token); for that population the base connected with the API key and the merged code fails or forces a re-auth. Mechanism…
There was a problem hiding this comment.
Accurate mechanism, but this is the change the PR exists to make. #2208 asks that the provider token override the configured Authorization, and the changeset and migration paragraph say so. The two populations are mirror images: #2208 (static key invalid, OAuth must win) was broken in base, and this one (static key valid, OAuth store dead) worked in base only because OAuth output was ignored entirely. No static precedence rule satisfies both.
What this population sees now is the normal OAuth lifecycle for a dead store: a valid refresh token repairs it after one 401, a revoked one fails the connect. The static key was masking that, not fixing it.
The cited example does the opposite of what's described: examples/cli-client/host/host.ts:467-475 passes static headers or a provider, never both ("No OAuth fallback"). That is the pattern for hosts that want static-first, and the changeset already gives the hook: the configured Authorization is sent while the provider has no token, so attach no provider when a key is set, clear the stale store, or use a provider whose token() returns undefined for a known-dead token.
Not taking either proposed fix. A retry with the caller credential means the transport sends two different credentials to the same server after a rejection, has to thread through the single-retry guard, step-up 403 and DPoP (proof is bound to the token hash), and leaves the dead token in the store so every connect pays a 401. "Known-fresh" is not computable at the transport: token() returns a bare string and OAuthTokens has no issued-at.
No change.
|
@claude review |
2 participants
Closes #2208.
StreamableHTTPClientTransport and SSEClientTransport both build the outgoing request's headers as:
This meant a caller-supplied Authorization placeholder (e.g. an env-var API key passed through the MCP config) was placed AFTER the SDK-derived common headers, which silently overrode OAuth-computed tokens and broke the auth-refresh flow once the placeholder went stale. Several MCP servers (Atlassian Rovo and others) let both API tokens and OAuth share the same Authorization header and rely on it being valid.
Swap the spread order to
...extraHeaders, ...headersso SDK-computed common headers (including the freshest OAuth token) take precedence, matching the merge order used elsewhere in the SDK. Caller-supplied non-auth headers still pass through unchanged.Test: a regression test in
streamableHttp.test.tsseeds a stale Authorization placeholder inrequestInit.headersalongside a workingauthProviderand asserts the request goes out with the OAuth-derived token. 722 client tests pass; eslint + prettier clean.Maintainer edit: Supersedes #2222 and #2223.