Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/oauth-header-spread-order.md
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.
7 changes: 7 additions & 0 deletions docs/migration/upgrade-to-v2.md
Original file line number Diff line number Diff line change
Expand Up @@ -808,6 +808,13 @@ value to the spec-required `application/json, text/event-stream` (v1 let it repl
them). The required media types are always present; additional types are kept for
proxy/gateway routing.

Transport-managed headers now take precedence over same-named entries in
`requestInit.headers`: `Authorization` when `authProvider` yields a token,
`mcp-protocol-version`, and (Streamable HTTP) `mcp-session-id`. v1 let the configured
header win, so a static `Authorization` placeholder kept overriding the OAuth token even
after the provider obtained one. A configured `Authorization` value is still sent while
the provider has no token, which is what lets a static API key fall back to OAuth.

`hostHeaderValidation()` and `localhostHostValidation()` moved to
`@modelcontextprotocol/express`. The `(allowedHostnames: string[])` signature is the
same as every released v1.x — only the import path changes. Framework-agnostic helpers
Expand Down
43 changes: 28 additions & 15 deletions packages/client/src/client/sse.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import {
brandedHasInstance,
createFetchWithInit,
JSONRPCMessageSchema,
normalizeHeaders,
SdkError,
SdkErrorCode,
SdkHttpError,
Expand Down Expand Up @@ -97,15 +96,23 @@ export type SSEClientTransportOptions = {
/**
* Customizes the initial SSE request to the server (the request that begins the stream).
*
* NOTE: Setting this property will prevent an `Authorization` header from
* being automatically attached to the SSE request, if an {@linkcode SSEClientTransportOptions.authProvider | authProvider} is
* also given. This can be worked around by setting the `Authorization` header
* manually.
* A custom `fetch` supplied here is still wrapped by the transport: the
* transport-managed headers, including the `Authorization` header derived from
* {@linkcode SSEClientTransportOptions.authProvider | authProvider}, are attached to the
* SSE request and take precedence over a same-named entry in `requestInit.headers`
* (see {@linkcode SSEClientTransportOptions.requestInit | requestInit}).
*/
eventSourceInit?: EventSourceInit;

/**
* Customizes recurring `POST` requests to the server.
*
* The transport-managed headers take precedence over a same-named entry in
* `headers`: `Authorization` when
* {@linkcode SSEClientTransportOptions.authProvider | authProvider} yields a token, 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;

Expand Down Expand Up @@ -170,7 +177,19 @@ export class SSEClientTransport implements Transport {
private _last401Response?: Response;

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-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 keeps this transport in step
// with StreamableHTTPClientTransport. 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);
let token: string | undefined;
try {
token = await this._authProvider?.token();
Expand All @@ -180,18 +199,12 @@ export class SSEClientTransport implements Transport {
throw markAuthSeamEscape(error);
}
if (token) {
headers['Authorization'] = `Bearer ${token}`;
headers.set('Authorization', `Bearer ${token}`);
}
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;
}

private _startOrAuth(): Promise<void> {
Expand Down
37 changes: 24 additions & 13 deletions packages/client/src/client/streamableHttp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import {
JSONRPCMessageSchema,
mcpNameSource,
mediaTypeEssence,
normalizeHeaders,
PROTOCOL_VERSION_META_KEY,
SdkError,
SdkErrorCode,
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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.

Copy link
Copy Markdown
Contributor

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-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…

// `|| 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);
Comment thread
KKonstantinov marked this conversation as resolved.
let token: string | undefined;
try {
token = await this._authProvider?.token();
Expand All @@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 (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…

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 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.


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;
}

/**
Expand Down
137 changes: 137 additions & 0 deletions packages/client/test/client/sse.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,26 @@ describe('SSEClientTransport', () => {
expect(lastServerRequest.headers.authorization).toBe(authToken);
});

it('tolerates requestInit.headers set to null by a JavaScript caller', async () => {
// The TS type excludes null, but a JS caller or a JSON config forwarded verbatim can
// pass it. `new Headers(null)` throws, so the transport must map falsy to undefined.
transport = new SSEClientTransport(resourceBaseUrl, {
requestInit: { headers: null as unknown as RequestInit['headers'] }
});

await transport.start();
expect(lastServerRequest.headers.accept).toBe('text/event-stream');

const message: JSONRPCMessage = {
jsonrpc: '2.0',
id: '1',
method: 'test',
params: {}
};
await transport.send(message);
expect(lastServerRequest.headers['content-type']).toBe('application/json');
});

it('passes custom headers to fetch requests', async () => {
const customHeaders = {
Authorization: 'Bearer test-token',
Expand Down Expand Up @@ -660,6 +680,123 @@ describe('SSEClientTransport', () => {
expect(lastServerRequest.headers['x-custom-header']).toBe('custom-value');
});

it('lets the auth provider token override a stale caller-supplied Authorization header', async () => {
// Regression test for #2208: transport-managed headers are merged on top of
// requestInit.headers, so a static Authorization placeholder (e.g. an env-var
// API key) gives way to the provider's token on both the SSE GET and POSTs.
mockAuthProvider.tokens.mockResolvedValue({
access_token: 'fresh-token',
token_type: 'Bearer'
});

transport = new SSEClientTransport(resourceBaseUrl, {
authProvider: mockAuthProvider,
requestInit: {
headers: {
Authorization: 'Bearer stale-placeholder',
'X-Custom-Header': 'custom-value'
}
}
});

await transport.start();

// SSE GET
expect(lastServerRequest.headers.authorization).toBe('Bearer fresh-token');
expect(lastServerRequest.headers['x-custom-header']).toBe('custom-value');

const message: JSONRPCMessage = {
jsonrpc: '2.0',
id: '1',
method: 'test',
params: {}
};

await transport.send(message);

// POST
expect(lastServerRequest.headers.authorization).toBe('Bearer fresh-token');
expect(lastServerRequest.headers['x-custom-header']).toBe('custom-value');
});

it('replaces a caller-supplied Authorization header regardless of name casing (Headers instance)', async () => {
// #2208 follow-up: `Headers` normalizes names to lowercase; the transport must
// still send exactly its own token, not a combined two-token value.
mockAuthProvider.tokens.mockResolvedValue({
access_token: 'fresh-token',
token_type: 'Bearer'
});

transport = new SSEClientTransport(resourceBaseUrl, {
authProvider: mockAuthProvider,
requestInit: {
headers: new Headers({
authorization: 'Bearer stale-placeholder',
'x-custom-header': 'custom-value'
})
}
});

await transport.start();

expect(lastServerRequest.headers.authorization).toBe('Bearer fresh-token');
expect(lastServerRequest.headers['x-custom-header']).toBe('custom-value');

const message: JSONRPCMessage = {
jsonrpc: '2.0',
id: '1',
method: 'test',
params: {}
};

await transport.send(message);

expect(lastServerRequest.headers.authorization).toBe('Bearer fresh-token');
expect(lastServerRequest.headers['x-custom-header']).toBe('custom-value');
});

it('keeps Fetch Headers combine semantics for a repeated name in a tuple array', async () => {
// A repeated name in a tuple array is the one `HeadersInit` form that can express a
// multi-valued header, and `fetch(url, { headers: [['x', 'a'], ['x', 'b']] })` sends
// "x: a, b". The transport builds its headers with the same `Headers` constructor, so
// a caller-supplied repeated name is combined exactly as a direct `fetch` would, while
// a repeated *transport-managed* name is still replaced outright by the transport's
// own value rather than combined with it.
mockAuthProvider.tokens.mockResolvedValue({
access_token: 'fresh-token',
token_type: 'Bearer'
});

transport = new SSEClientTransport(resourceBaseUrl, {
authProvider: mockAuthProvider,
requestInit: {
headers: [
['Authorization', 'Bearer stale-1'],
['Authorization', 'Bearer stale-2'],
['x-multi', 'a'],
['x-multi', 'b']
]
}
});

await transport.start();

expect(lastServerRequest.headers.authorization).toBe('Bearer fresh-token');
expect(lastServerRequest.headers['x-multi']).toBe('a, b');

const message: JSONRPCMessage = {
jsonrpc: '2.0',
id: '1',
method: 'test',
params: {}
};

await transport.send(message);

expect(lastServerRequest.headers.authorization).toBe('Bearer fresh-token');
expect(lastServerRequest.headers['x-multi']).toBe('a, b');
});

it('refreshes expired token during SSE connection', async () => {
// Mock tokens() to return expired token until saveTokens is called
let currentTokens: OAuthTokens = {
Expand Down
Loading
Loading