From 568199a46ff4f14e22d69239017a199ad7fc6262 Mon Sep 17 00:00:00 2001 From: Paul Carleton Date: Fri, 15 Aug 2025 10:03:39 +0100 Subject: [PATCH 1/3] fallback on 4xx not just 404 --- src/client/auth.test.ts | 8 ++-- src/client/auth.ts | 2 +- src/client/streamableHttp.test.ts | 70 +++++++++++++++++-------------- 3 files changed, 43 insertions(+), 37 deletions(-) diff --git a/src/client/auth.test.ts b/src/client/auth.test.ts index fb9b31006..f5ccca523 100644 --- a/src/client/auth.test.ts +++ b/src/client/auth.test.ts @@ -212,11 +212,11 @@ describe("OAuth Authorization", () => { expect(url.toString()).toBe("https://resource.example.com/.well-known/oauth-protected-resource/path?param=value"); }); - it("falls back to root discovery when path-aware discovery returns 404", async () => { - // First call (path-aware) returns 404 + it("falls back to root discovery when path-aware discovery fails", async () => { + // First call (path-aware) returns 4xx mockFetch.mockResolvedValueOnce({ ok: false, - status: 404, + status: 401, }); // Second call (root fallback) succeeds @@ -907,7 +907,7 @@ describe("OAuth Authorization", () => { const metadata = await discoverAuthorizationServerMetadata("https://auth.example.com/tenant1"); expect(metadata).toBeUndefined(); - + // Verify that all discovery URLs were attempted expect(mockFetch).toHaveBeenCalledTimes(8); // 4 URLs × 2 attempts each (with and without headers) }); diff --git a/src/client/auth.ts b/src/client/auth.ts index 8ac9ddd1e..cba365f27 100644 --- a/src/client/auth.ts +++ b/src/client/auth.ts @@ -571,7 +571,7 @@ async function tryMetadataDiscovery( * Determines if fallback to root discovery should be attempted */ function shouldAttemptFallback(response: Response | undefined, pathname: string): boolean { - return !response || response.status === 404 && pathname !== '/'; + return !response || (response.status >= 400 && response.status < 500 ) && pathname !== '/'; } /** diff --git a/src/client/streamableHttp.test.ts b/src/client/streamableHttp.test.ts index 88fd48017..fdd35ed3f 100644 --- a/src/client/streamableHttp.test.ts +++ b/src/client/streamableHttp.test.ts @@ -465,7 +465,7 @@ describe("StreamableHTTPClientTransport", () => { // Verify custom fetch was used expect(customFetch).toHaveBeenCalled(); - + // Global fetch should never have been called expect(global.fetch).not.toHaveBeenCalled(); }); @@ -589,32 +589,32 @@ describe("StreamableHTTPClientTransport", () => { await expect(transport.send(message)).rejects.toThrow(UnauthorizedError); expect(mockAuthProvider.redirectToAuthorization.mock.calls).toHaveLength(1); }); - + describe('Reconnection Logic', () => { let transport: StreamableHTTPClientTransport; - + // Use fake timers to control setTimeout and make the test instant. beforeEach(() => jest.useFakeTimers()); afterEach(() => jest.useRealTimers()); - + it('should reconnect a GET-initiated notification stream that fails', async () => { // ARRANGE transport = new StreamableHTTPClientTransport(new URL("http://localhost:1234/mcp"), { reconnectionOptions: { - initialReconnectionDelay: 10, - maxRetries: 1, + initialReconnectionDelay: 10, + maxRetries: 1, maxReconnectionDelay: 1000, // Ensure it doesn't retry indefinitely reconnectionDelayGrowFactor: 1 // No exponential backoff for simplicity } }); - + const errorSpy = jest.fn(); transport.onerror = errorSpy; - + const failingStream = new ReadableStream({ start(controller) { controller.error(new Error("Network failure")); } }); - + const fetchMock = global.fetch as jest.Mock; // Mock the initial GET request, which will fail. fetchMock.mockResolvedValueOnce({ @@ -628,13 +628,13 @@ describe("StreamableHTTPClientTransport", () => { headers: new Headers({ "content-type": "text/event-stream" }), body: new ReadableStream(), }); - + // ACT await transport.start(); // Trigger the GET stream directly using the internal method for a clean test. await transport["_startOrAuthSse"]({}); await jest.advanceTimersByTimeAsync(20); // Trigger reconnection timeout - + // ASSERT expect(errorSpy).toHaveBeenCalledWith(expect.objectContaining({ message: expect.stringContaining('SSE stream disconnected: Error: Network failure'), @@ -644,25 +644,25 @@ describe("StreamableHTTPClientTransport", () => { expect(fetchMock.mock.calls[0][1]?.method).toBe('GET'); expect(fetchMock.mock.calls[1][1]?.method).toBe('GET'); }); - + it('should NOT reconnect a POST-initiated stream that fails', async () => { // ARRANGE transport = new StreamableHTTPClientTransport(new URL("http://localhost:1234/mcp"), { - reconnectionOptions: { - initialReconnectionDelay: 10, - maxRetries: 1, + reconnectionOptions: { + initialReconnectionDelay: 10, + maxRetries: 1, maxReconnectionDelay: 1000, // Ensure it doesn't retry indefinitely reconnectionDelayGrowFactor: 1 // No exponential backoff for simplicity } }); - + const errorSpy = jest.fn(); transport.onerror = errorSpy; - + const failingStream = new ReadableStream({ start(controller) { controller.error(new Error("Network failure")); } }); - + const fetchMock = global.fetch as jest.Mock; // Mock the POST request. It returns a streaming content-type but a failing body. fetchMock.mockResolvedValueOnce({ @@ -670,7 +670,7 @@ describe("StreamableHTTPClientTransport", () => { headers: new Headers({ "content-type": "text/event-stream" }), body: failingStream, }); - + // A dummy request message to trigger the `send` logic. const requestMessage: JSONRPCRequest = { jsonrpc: '2.0', @@ -678,13 +678,13 @@ describe("StreamableHTTPClientTransport", () => { id: 'request-1', params: {}, }; - + // ACT await transport.start(); // Use the public `send` method to initiate a POST that gets a stream response. await transport.send(requestMessage); await jest.advanceTimersByTimeAsync(20); // Advance time to check for reconnections - + // ASSERT expect(errorSpy).toHaveBeenCalledWith(expect.objectContaining({ message: expect.stringContaining('SSE stream disconnected: Error: Network failure'), @@ -718,7 +718,9 @@ describe("StreamableHTTPClientTransport", () => { (global.fetch as jest.Mock) // Initial connection .mockResolvedValueOnce(unauthedResponse) - // Resource discovery + // Resource discovery, path aware + .mockResolvedValueOnce(unauthedResponse) + // Resource discovery, root .mockResolvedValueOnce(unauthedResponse) // OAuth metadata discovery .mockResolvedValueOnce({ @@ -770,7 +772,9 @@ describe("StreamableHTTPClientTransport", () => { (global.fetch as jest.Mock) // Initial connection .mockResolvedValueOnce(unauthedResponse) - // Resource discovery + // Resource discovery, path aware + .mockResolvedValueOnce(unauthedResponse) + // Resource discovery, root .mockResolvedValueOnce(unauthedResponse) // OAuth metadata discovery .mockResolvedValueOnce({ @@ -822,7 +826,9 @@ describe("StreamableHTTPClientTransport", () => { (global.fetch as jest.Mock) // Initial connection .mockResolvedValueOnce(unauthedResponse) - // Resource discovery + // Resource discovery, path aware + .mockResolvedValueOnce(unauthedResponse) + // Resource discovery, root .mockResolvedValueOnce(unauthedResponse) // OAuth metadata discovery .mockResolvedValueOnce({ @@ -888,7 +894,7 @@ describe("StreamableHTTPClientTransport", () => { ok: false, status: 404 }); - + // Create transport instance transport = new StreamableHTTPClientTransport(new URL("http://localhost:1234/mcp"), { authProvider: mockAuthProvider, @@ -901,14 +907,14 @@ describe("StreamableHTTPClientTransport", () => { // Verify custom fetch was used expect(customFetch).toHaveBeenCalled(); - + // Verify specific OAuth endpoints were called with custom fetch const customFetchCalls = customFetch.mock.calls; const callUrls = customFetchCalls.map(([url]) => url.toString()); - + // Should have called resource metadata discovery expect(callUrls.some(url => url.includes('/.well-known/oauth-protected-resource'))).toBe(true); - + // Should have called OAuth authorization server metadata discovery expect(callUrls.some(url => url.includes('/.well-known/oauth-authorization-server'))).toBe(true); @@ -966,19 +972,19 @@ describe("StreamableHTTPClientTransport", () => { // Verify custom fetch was used expect(customFetch).toHaveBeenCalled(); - + // Verify specific OAuth endpoints were called with custom fetch const customFetchCalls = customFetch.mock.calls; const callUrls = customFetchCalls.map(([url]) => url.toString()); - + // Should have called resource metadata discovery expect(callUrls.some(url => url.includes('/.well-known/oauth-protected-resource'))).toBe(true); - + // Should have called OAuth authorization server metadata discovery expect(callUrls.some(url => url.includes('/.well-known/oauth-authorization-server'))).toBe(true); // Should have called token endpoint for authorization code exchange - const tokenCalls = customFetchCalls.filter(([url, options]) => + const tokenCalls = customFetchCalls.filter(([url, options]) => url.toString().includes('/token') && options?.method === "POST" ); expect(tokenCalls.length).toBeGreaterThan(0); From d4da6c6c1940a2b6e89d245264ab6c373649a6f6 Mon Sep 17 00:00:00 2001 From: Paul Carleton Date: Fri, 15 Aug 2025 14:08:18 +0100 Subject: [PATCH 2/3] Update src/client/auth.ts Co-authored-by: adam jones --- src/client/auth.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/auth.ts b/src/client/auth.ts index cba365f27..fcc320f17 100644 --- a/src/client/auth.ts +++ b/src/client/auth.ts @@ -571,7 +571,7 @@ async function tryMetadataDiscovery( * Determines if fallback to root discovery should be attempted */ function shouldAttemptFallback(response: Response | undefined, pathname: string): boolean { - return !response || (response.status >= 400 && response.status < 500 ) && pathname !== '/'; + return !response || (response.status >= 400 && response.status < 500) && pathname !== '/'; } /** From c2d714a1dcdc776e4652687eabb11e335bb6a422 Mon Sep 17 00:00:00 2001 From: Paul Carleton Date: Tue, 19 Aug 2025 15:07:09 +0100 Subject: [PATCH 3/3] add test for 500 --- src/client/auth.test.ts | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/client/auth.test.ts b/src/client/auth.test.ts index f5ccca523..f28163d14 100644 --- a/src/client/auth.test.ts +++ b/src/client/auth.test.ts @@ -212,11 +212,11 @@ describe("OAuth Authorization", () => { expect(url.toString()).toBe("https://resource.example.com/.well-known/oauth-protected-resource/path?param=value"); }); - it("falls back to root discovery when path-aware discovery fails", async () => { + it.each([400, 401, 403, 404, 410, 422, 429])("falls back to root discovery when path-aware discovery returns %d", async (statusCode) => { // First call (path-aware) returns 4xx mockFetch.mockResolvedValueOnce({ ok: false, - status: 401, + status: statusCode, }); // Second call (root fallback) succeeds @@ -267,6 +267,20 @@ describe("OAuth Authorization", () => { expect(calls.length).toBe(2); }); + it("throws error on 500 status and does not fallback", async () => { + // First call (path-aware) returns 500 + mockFetch.mockResolvedValueOnce({ + ok: false, + status: 500, + }); + + await expect(discoverOAuthProtectedResourceMetadata("https://resource.example.com/path/name")) + .rejects.toThrow(); + + const calls = mockFetch.mock.calls; + expect(calls.length).toBe(1); // Should not attempt fallback + }); + it("does not fallback when the original URL is already at root path", async () => { // First call (path-aware for root) returns 404 mockFetch.mockResolvedValueOnce({