diff --git a/.changeset/fix-oauth-non-bearer-www-authenticate-fallback.md b/.changeset/fix-oauth-non-bearer-www-authenticate-fallback.md new file mode 100644 index 000000000..3fc3e8362 --- /dev/null +++ b/.changeset/fix-oauth-non-bearer-www-authenticate-fallback.md @@ -0,0 +1,5 @@ +--- +'@modelcontextprotocol/client': patch +--- + +Fixed OAuth discovery fall back to well-known URI when WWW-Authenticate carries a non-Bearer scheme (e.g. Negotiate). Previously a non-Bearer challenge could clear the `resource_metadata` URL stored from an earlier Bearer challenge, causing PRM discovery to start from the wrong endpoint. The stored URL is now preserved across non-Bearer 401s and passed through to the authorization handler via context. diff --git a/packages/client/src/client/auth.ts b/packages/client/src/client/auth.ts index 5f55fb7a0..73bc5af87 100644 --- a/packages/client/src/client/auth.ts +++ b/packages/client/src/client/auth.ts @@ -46,6 +46,12 @@ export interface UnauthorizedContext { serverUrl: URL; /** Fetch function configured with the transport's `requestInit`, for making auth requests. */ fetchFn: FetchLike; + /** + * Resource metadata URL previously established from an earlier Bearer challenge, if any. + * Used as a fallback when the current `WWW-Authenticate` header does not carry a + * `resource_metadata` parameter (e.g. a non-Bearer scheme such as `Negotiate`). + */ + resourceMetadataUrl?: URL; } /** @@ -101,7 +107,11 @@ export function isOAuthClientProvider(provider: AuthProvider | OAuthClientProvid * Used by {@linkcode adaptOAuthProvider} to bridge `OAuthClientProvider` to `AuthProvider`. */ export async function handleOAuthUnauthorized(provider: OAuthClientProvider, ctx: UnauthorizedContext): Promise { - const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(ctx.response); + const { resourceMetadataUrl: extractedUrl, scope } = extractWWWAuthenticateParams(ctx.response); + // When the current challenge carries no resource_metadata (e.g. a non-Bearer scheme + // like Negotiate), fall back to the URL the transport stored from an earlier Bearer + // challenge so that PRM discovery can still run with the right starting point. + const resourceMetadataUrl = extractedUrl ?? ctx.resourceMetadataUrl; const result = await auth(provider, { serverUrl: ctx.serverUrl, resourceMetadataUrl, diff --git a/packages/client/src/client/sse.ts b/packages/client/src/client/sse.ts index f441e9cdb..aa89737ba 100644 --- a/packages/client/src/client/sse.ts +++ b/packages/client/src/client/sse.ts @@ -135,8 +135,15 @@ export class SSEClientTransport implements Transport { this._last401Response = response; if (response.headers.has('www-authenticate')) { const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response); - this._resourceMetadataUrl = resourceMetadataUrl; - this._scope = scope; + // Only update when the Bearer challenge advertises a URL; a non-Bearer + // scheme (e.g. Negotiate) must not clear a previously-stored value so + // the well-known fallback can still run with the right starting point. + if (resourceMetadataUrl !== undefined) { + this._resourceMetadataUrl = resourceMetadataUrl; + } + if (scope !== undefined) { + this._scope = scope; + } } } @@ -151,15 +158,22 @@ export class SSEClientTransport implements Transport { const response = this._last401Response; this._last401Response = undefined; this._eventSource?.close(); - this._authProvider.onUnauthorized({ response, serverUrl: this._url, fetchFn: this._fetchWithInit }).then( - // onUnauthorized succeeded → retry fresh. Its onerror handles its own onerror?.() + reject. - () => this._startOrAuth().then(resolve, reject), - // onUnauthorized failed → not yet reported. - error => { - this.onerror?.(error); - reject(error); - } - ); + this._authProvider + .onUnauthorized({ + response, + serverUrl: this._url, + fetchFn: this._fetchWithInit, + resourceMetadataUrl: this._resourceMetadataUrl + }) + .then( + // onUnauthorized succeeded → retry fresh. Its onerror handles its own onerror?.() + reject. + () => this._startOrAuth().then(resolve, reject), + // onUnauthorized failed → not yet reported. + error => { + this.onerror?.(error); + reject(error); + } + ); return; } const error = new UnauthorizedError(); @@ -270,15 +284,23 @@ export class SSEClientTransport implements Transport { if (response.status === 401 && this._authProvider) { if (response.headers.has('www-authenticate')) { const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response); - this._resourceMetadataUrl = resourceMetadataUrl; - this._scope = scope; + // Only update when the Bearer challenge advertises a URL; a non-Bearer + // scheme (e.g. Negotiate) must not clear a previously-stored value so + // the well-known fallback can still run with the right starting point. + if (resourceMetadataUrl !== undefined) { + this._resourceMetadataUrl = resourceMetadataUrl; + } + if (scope !== undefined) { + this._scope = scope; + } } if (this._authProvider.onUnauthorized && !isAuthRetry) { await this._authProvider.onUnauthorized({ response, serverUrl: this._url, - fetchFn: this._fetchWithInit + fetchFn: this._fetchWithInit, + resourceMetadataUrl: this._resourceMetadataUrl }); await response.text?.().catch(() => {}); // Purposely _not_ awaited, so we don't call onerror twice diff --git a/packages/client/src/client/streamableHttp.ts b/packages/client/src/client/streamableHttp.ts index cd643c96d..e27a7c772 100644 --- a/packages/client/src/client/streamableHttp.ts +++ b/packages/client/src/client/streamableHttp.ts @@ -257,15 +257,23 @@ export class StreamableHTTPClientTransport implements Transport { if (response.status === 401 && this._authProvider) { if (response.headers.has('www-authenticate')) { const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response); - this._resourceMetadataUrl = resourceMetadataUrl; - this._scope = scope; + // Only update when the Bearer challenge advertises a URL; a non-Bearer + // scheme (e.g. Negotiate) must not clear a previously-stored value so + // the well-known fallback can still run with the right starting point. + if (resourceMetadataUrl !== undefined) { + this._resourceMetadataUrl = resourceMetadataUrl; + } + if (scope !== undefined) { + this._scope = scope; + } } if (this._authProvider.onUnauthorized && !isAuthRetry) { await this._authProvider.onUnauthorized({ response, serverUrl: this._url, - fetchFn: this._fetchWithInit + fetchFn: this._fetchWithInit, + resourceMetadataUrl: this._resourceMetadataUrl }); await response.text?.().catch(() => {}); // Purposely _not_ awaited, so we don't call onerror twice @@ -565,15 +573,23 @@ export class StreamableHTTPClientTransport implements Transport { // Store WWW-Authenticate params for interactive finishAuth() path if (response.headers.has('www-authenticate')) { const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response); - this._resourceMetadataUrl = resourceMetadataUrl; - this._scope = scope; + // Only update when the Bearer challenge advertises a URL; a non-Bearer + // scheme (e.g. Negotiate) must not clear a previously-stored value so + // the well-known fallback can still run with the right starting point. + if (resourceMetadataUrl !== undefined) { + this._resourceMetadataUrl = resourceMetadataUrl; + } + if (scope !== undefined) { + this._scope = scope; + } } if (this._authProvider.onUnauthorized && !isAuthRetry) { await this._authProvider.onUnauthorized({ response, serverUrl: this._url, - fetchFn: this._fetchWithInit + fetchFn: this._fetchWithInit, + resourceMetadataUrl: this._resourceMetadataUrl }); await response.text?.().catch(() => {}); // Purposely _not_ awaited, so we don't call onerror twice diff --git a/packages/client/test/client/streamableHttp.test.ts b/packages/client/test/client/streamableHttp.test.ts index b2138b3fa..6b3e675e6 100644 --- a/packages/client/test/client/streamableHttp.test.ts +++ b/packages/client/test/client/streamableHttp.test.ts @@ -2,7 +2,7 @@ import type { JSONRPCMessage, JSONRPCRequest } from '@modelcontextprotocol/core' import { OAuthError, OAuthErrorCode, SdkError, SdkErrorCode } from '@modelcontextprotocol/core'; import type { Mock, Mocked } from 'vitest'; -import type { OAuthClientProvider } from '../../src/client/auth.js'; +import type { AuthProvider, OAuthClientProvider } from '../../src/client/auth.js'; import { UnauthorizedError } from '../../src/client/auth.js'; import type { ReconnectionScheduler, StartSSEOptions, StreamableHTTPReconnectionOptions } from '../../src/client/streamableHttp.js'; import { StreamableHTTPClientTransport } from '../../src/client/streamableHttp.js'; @@ -783,6 +783,91 @@ describe('StreamableHTTPClientTransport', () => { expect(mockAuthProvider.redirectToAuthorization.mock.calls).toHaveLength(1); }); + it('falls back to well-known discovery on 401 with non-Bearer WWW-Authenticate (e.g. Negotiate)', async () => { + const message: JSONRPCMessage = { + jsonrpc: '2.0', + method: 'test', + params: {}, + id: 'test-id' + }; + + (globalThis.fetch as Mock) + .mockResolvedValueOnce({ + ok: false, + status: 401, + statusText: 'Unauthorized', + headers: new Headers({ 'WWW-Authenticate': 'Negotiate' }), + text: async () => 'Authentication required.' + }) + .mockResolvedValue({ + ok: false, + status: 404, + text: async () => { + throw 'dont read my body'; + } + }); + + await expect(transport.send(message)).rejects.toThrow(UnauthorizedError); + expect(mockAuthProvider.redirectToAuthorization.mock.calls).toHaveLength(1); + }); + + it('passes stored resource metadata URL to onUnauthorized context after non-Bearer 401', async () => { + // Regression test: a non-Bearer WWW-Authenticate (e.g. Negotiate) must not wipe + // out a Bearer resource_metadata URL stored from an earlier Bearer challenge. + // The stored URL must reach onUnauthorized via ctx.resourceMetadataUrl so PRM + // discovery can use the right starting point instead of falling back to the root + // well-known URI. + // + // Uses a plain AuthProvider (not OAuthClientProvider) so onUnauthorized is called + // directly by the transport — no adaptOAuthProvider wrapper whose internal closure + // would hide the context from a spy. + const message: JSONRPCMessage = { + jsonrpc: '2.0', + method: 'test', + params: {}, + id: 'test-id' + }; + const resourceMetadataUrl = 'http://example.com/.well-known/oauth-protected-resource'; + + const onUnauthorized = vi.fn().mockResolvedValue(undefined); + const plainTransport = new StreamableHTTPClientTransport(new URL('http://localhost:1234/mcp'), { + authProvider: { token: async () => 'test-token', onUnauthorized } + }); + + try { + // First request: Bearer 401 → stores _resourceMetadataUrl; onUnauthorized resolves, retry succeeds + (globalThis.fetch as Mock) + .mockResolvedValueOnce({ + ok: false, + status: 401, + headers: new Headers({ 'WWW-Authenticate': `Bearer resource_metadata="${resourceMetadataUrl}"` }), + text: async () => '' + }) + .mockResolvedValueOnce({ ok: true, status: 202, headers: new Headers() }); + + await plainTransport.send(message); + onUnauthorized.mockClear(); + + // Second request: Negotiate 401 → must NOT clear _resourceMetadataUrl; retry succeeds + (globalThis.fetch as Mock) + .mockResolvedValueOnce({ + ok: false, + status: 401, + headers: new Headers({ 'WWW-Authenticate': 'Negotiate' }), + text: async () => '' + }) + .mockResolvedValueOnce({ ok: true, status: 202, headers: new Headers() }); + + await plainTransport.send(message); + + // onUnauthorized must receive the stored Bearer URL in ctx.resourceMetadataUrl, + // not undefined, so PRM discovery uses the correct endpoint. + expect(onUnauthorized).toHaveBeenCalledWith(expect.objectContaining({ resourceMetadataUrl: new URL(resourceMetadataUrl) })); + } finally { + await plainTransport.close().catch(() => {}); + } + }); + it('attempts upscoping on 403 with WWW-Authenticate header', async () => { const message: JSONRPCMessage = { jsonrpc: '2.0',