-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix(client): let OAuth-derived Authorization override caller-supplied header #2475
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
96b4af9
0003f27
bfab122
f8d9814
4e8ebc7
52491ff
db9a66c
dcc7928
86beff1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@modelcontextprotocol/client': patch | ||
| --- | ||
|
|
||
| `StreamableHTTPClientTransport` and `SSEClientTransport` now give their transport-managed headers precedence over same-named entries in `requestInit.headers`: `Authorization` when `authProvider` yields a token, `mcp-protocol-version`, and (Streamable HTTP) `mcp-session-id`. Header names compare case-insensitively and every `HeadersInit` form is covered (plain object, tuple array, `Headers` instance). Previously the caller-supplied value won, so a static `Authorization` placeholder (e.g. an env-var API key) kept overriding the OAuth token even after the provider obtained one and the fallback-to-OAuth flow never completed; a `Headers` instance or lowercase key produced a combined `Bearer <fresh>, Bearer <stale>` value instead. A configured `Authorization` is still sent while the provider has no token, and other configured headers pass through unchanged. Closes #2208. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,6 @@ import { | |
| JSONRPCMessageSchema, | ||
| mcpNameSource, | ||
| mediaTypeEssence, | ||
| normalizeHeaders, | ||
| PROTOCOL_VERSION_META_KEY, | ||
| SdkError, | ||
| SdkErrorCode, | ||
|
|
@@ -183,6 +182,13 @@ export type StreamableHTTPClientTransportOptions = { | |
|
|
||
| /** | ||
| * Customizes HTTP requests to the server. | ||
| * | ||
| * `headers` are sent on every request, but the transport-managed headers take | ||
| * precedence over a same-named entry here: `Authorization` when | ||
| * {@linkcode StreamableHTTPClientTransportOptions.authProvider | authProvider} yields a | ||
| * token, `mcp-session-id`, and `mcp-protocol-version`. A caller-supplied `Authorization` | ||
| * value is therefore only sent while the provider has no token, which lets a static API | ||
| * key fall back to OAuth once the provider obtains one. | ||
| */ | ||
| requestInit?: RequestInit; | ||
|
|
||
|
|
@@ -443,7 +449,19 @@ export class StreamableHTTPClientTransport implements Transport { | |
| } | ||
|
|
||
| private async _commonHeaders(): Promise<Headers> { | ||
| const headers: RequestInit['headers'] & Record<string, string> = {}; | ||
| // Start from the caller-supplied `requestInit.headers` and `set()` the | ||
| // transport-managed headers on top. `Headers.set` compares names | ||
| // case-insensitively, so Authorization / mcp-session-id / mcp-protocol-version | ||
| // replace a same-named caller entry whatever its spelling. (A plain-object | ||
| // spread would keep `authorization` and `Authorization` side by side, and the | ||
| // Fetch `Headers` constructor would then combine them into one two-token | ||
| // 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. | ||
| // `|| undefined` keeps the old tolerance for a falsy `headers` value (e.g. `null` | ||
| // from a JS caller or a JSON config forwarded verbatim): the Fetch `Headers` | ||
| // constructor accepts `undefined` but throws on `null`. | ||
| const headers = new Headers(this._requestInit?.headers || undefined); | ||
|
KKonstantinov marked this conversation as resolved.
|
||
| let token: string | undefined; | ||
| try { | ||
| token = await this._authProvider?.token(); | ||
|
|
@@ -453,22 +471,15 @@ export class StreamableHTTPClientTransport implements Transport { | |
| throw markAuthSeamEscape(error); | ||
| } | ||
| if (token) { | ||
| headers['Authorization'] = `Bearer ${token}`; | ||
| headers.set('Authorization', `Bearer ${token}`); | ||
| } | ||
|
Comment on lines
473
to
475
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 (optional) Hosts that configure a working static API key in Extended reasoning...Trigger (external config): a host that wires a user's static Verification: nit — conflicts with stated purpose: triggered only when a host hands the same transport BOTH a valid static
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Accurate mechanism, but this is the change the PR exists to make. #2208 asks that the provider token override the configured 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: 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: No change. |
||
|
|
||
| if (this._sessionId) { | ||
| headers['mcp-session-id'] = this._sessionId; | ||
| headers.set('mcp-session-id', this._sessionId); | ||
| } | ||
| if (this._protocolVersion) { | ||
| headers['mcp-protocol-version'] = this._protocolVersion; | ||
| headers.set('mcp-protocol-version', this._protocolVersion); | ||
| } | ||
|
|
||
| const extraHeaders = normalizeHeaders(this._requestInit?.headers); | ||
|
|
||
| return new Headers({ | ||
| ...headers, | ||
| ...extraHeaders | ||
| }); | ||
| return headers; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟣 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-nameandcontent-typefromrequestInit.headersstill 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-injectedMcp-Method/Mcp-Nameproduces 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
Headersin_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 seedsnew 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 aPROTOCOL_VERSION_META_KEYenvelope, :494-499) and applies the RESERVED_REQUEST_HEADER_NAMES filter (:268-276) only tooptions.headers, never to the seeded requestInit names. So onnotifications/initialized(2nd POST of every connect), batch messages, legacy-era requests, the GET at :550 and DELETE at :1216, a caller-configureddpop/mcp-method/mcp-namegoes out verbatim;content-typeis overwritten at :1003 only in send(). Population: any host that lets end-user config populaterequestInit.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,DPoPwithout a DPoP session) into the staticrequestInit.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…