From 1b8e94ca4091c2db442961c033ca512469aa9f6b Mon Sep 17 00:00:00 2001 From: Jerome Date: Sat, 22 Feb 2025 16:20:41 +0000 Subject: [PATCH 1/9] During metadata fetching, try with the MCP header, then without, then return undefined if both fail --- src/client/auth.ts | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/client/auth.ts b/src/client/auth.ts index c0bac9e8e..5f5c860e4 100644 --- a/src/client/auth.ts +++ b/src/client/auth.ts @@ -163,11 +163,20 @@ export async function discoverOAuthMetadata( opts?: { protocolVersion?: string }, ): Promise { const url = new URL("/.well-known/oauth-authorization-server", serverUrl); - const response = await fetch(url, { - headers: { - "MCP-Protocol-Version": opts?.protocolVersion ?? LATEST_PROTOCOL_VERSION + let response: Response; + try { + response = await fetch(url, { + headers: { + "MCP-Protocol-Version": opts?.protocolVersion ?? LATEST_PROTOCOL_VERSION + } + }); + } catch { + try { + response = await fetch(url); + } catch { + return undefined; } - }); + } if (response.status === 404) { return undefined; From 2dc7fa5df8a041ec374dff197d37ed5253eef730 Mon Sep 17 00:00:00 2001 From: Jerome Date: Sun, 23 Feb 2025 10:57:26 +0000 Subject: [PATCH 2/9] Check token expiry in bearer auth --- src/server/auth/middleware/bearerAuth.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/server/auth/middleware/bearerAuth.ts b/src/server/auth/middleware/bearerAuth.ts index 14109e174..cd1b314af 100644 --- a/src/server/auth/middleware/bearerAuth.ts +++ b/src/server/auth/middleware/bearerAuth.ts @@ -55,6 +55,11 @@ export function requireBearerAuth({ provider, requiredScopes = [] }: BearerAuthM } } + // Check if the token is expired + if (!!authInfo.expiresAt && authInfo.expiresAt < Date.now() / 1000) { + throw new InvalidTokenError("Token has expired"); + } + req.auth = authInfo; next(); } catch (error) { From 8a222e8b385ec027ac013ab927bb71ad3456004e Mon Sep 17 00:00:00 2001 From: Jerome Date: Sun, 23 Feb 2025 10:59:05 +0000 Subject: [PATCH 3/9] Don't set client secret expiry if public client --- src/server/auth/handlers/register.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/server/auth/handlers/register.ts b/src/server/auth/handlers/register.ts index 675e8733c..7eca5b176 100644 --- a/src/server/auth/handlers/register.ts +++ b/src/server/auth/handlers/register.ts @@ -75,10 +75,11 @@ export function clientRegistrationHandler({ } const clientMetadata = parseResult.data; + const isPublicClient = clientMetadata.token_endpoint_auth_method !== 'none' // Generate client credentials const clientId = crypto.randomUUID(); - const clientSecret = clientMetadata.token_endpoint_auth_method !== 'none' + const clientSecret = isPublicClient ? crypto.randomBytes(32).toString('hex') : undefined; const clientIdIssuedAt = Math.floor(Date.now() / 1000); @@ -88,7 +89,11 @@ export function clientRegistrationHandler({ client_id: clientId, client_secret: clientSecret, client_id_issued_at: clientIdIssuedAt, - client_secret_expires_at: clientSecretExpirySeconds > 0 ? clientIdIssuedAt + clientSecretExpirySeconds : 0 + client_secret_expires_at: isPublicClient + ? clientSecretExpirySeconds > 0 + ? clientIdIssuedAt + clientSecretExpirySeconds + : 0 + : undefined, }; clientInfo = await clientsStore.registerClient!(clientInfo); From 3b12961e2e0082b6cb240c17ae95d11114e1e973 Mon Sep 17 00:00:00 2001 From: Jerome Date: Sun, 23 Feb 2025 14:55:30 +0000 Subject: [PATCH 4/9] Added tests --- src/client/auth.test.ts | 30 +++++++++++ src/server/auth/handlers/register.test.ts | 31 +++++++++++ src/server/auth/middleware/bearerAuth.test.ts | 51 +++++++++++++++++++ 3 files changed, 112 insertions(+) diff --git a/src/client/auth.test.ts b/src/client/auth.test.ts index 97c9e14aa..1a6ef5afb 100644 --- a/src/client/auth.test.ts +++ b/src/client/auth.test.ts @@ -43,6 +43,36 @@ describe("OAuth Authorization", () => { }); }); + it("returns metadata when first fetch fails but second without MCP header succeeds", async () => { + // First request with MCP header fails + mockFetch.mockRejectedValueOnce(new Error("Network error")); + + // Second request without header succeeds + mockFetch.mockResolvedValueOnce({ + ok: true, + status: 200, + json: async () => validMetadata, + }); + + const metadata = await discoverOAuthMetadata("https://auth.example.com"); + expect(metadata).toEqual(validMetadata); + + // Verify second call was made without header + expect(mockFetch).toHaveBeenCalledTimes(2); + const secondCallOptions = mockFetch.mock.calls[1][1]; + expect(secondCallOptions).toBeUndefined(); // No options means no headers + }); + + it("returns undefined when all fetch attempts fail", async () => { + // Both requests fail + mockFetch.mockRejectedValueOnce(new Error("Network error")); + mockFetch.mockRejectedValueOnce(new Error("Network error")); + + const metadata = await discoverOAuthMetadata("https://auth.example.com"); + expect(metadata).toBeUndefined(); + expect(mockFetch).toHaveBeenCalledTimes(2); + }); + it("returns undefined when discovery endpoint returns 404", async () => { mockFetch.mockResolvedValueOnce({ ok: false, diff --git a/src/server/auth/handlers/register.test.ts b/src/server/auth/handlers/register.test.ts index 5faf1a4a4..a961f6543 100644 --- a/src/server/auth/handlers/register.test.ts +++ b/src/server/auth/handlers/register.test.ts @@ -141,6 +141,37 @@ describe('Client Registration Handler', () => { expect(response.status).toBe(201); expect(response.body.client_secret).toBeUndefined(); + expect(response.body.client_secret_expires_at).toBeUndefined(); + }); + + it('sets client_secret_expires_at for public clients only', async () => { + // Test for public client (token_endpoint_auth_method not 'none') + const publicClientMetadata: OAuthClientMetadata = { + redirect_uris: ['https://example.com/callback'], + token_endpoint_auth_method: 'client_secret_basic' + }; + + const publicResponse = await supertest(app) + .post('/register') + .send(publicClientMetadata); + + expect(publicResponse.status).toBe(201); + expect(publicResponse.body.client_secret).toBeDefined(); + expect(publicResponse.body.client_secret_expires_at).toBeDefined(); + + // Test for non-public client (token_endpoint_auth_method is 'none') + const nonPublicClientMetadata: OAuthClientMetadata = { + redirect_uris: ['https://example.com/callback'], + token_endpoint_auth_method: 'none' + }; + + const nonPublicResponse = await supertest(app) + .post('/register') + .send(nonPublicClientMetadata); + + expect(nonPublicResponse.status).toBe(201); + expect(nonPublicResponse.body.client_secret).toBeUndefined(); + expect(nonPublicResponse.body.client_secret_expires_at).toBeUndefined(); }); it('sets expiry based on clientSecretExpirySeconds', async () => { diff --git a/src/server/auth/middleware/bearerAuth.test.ts b/src/server/auth/middleware/bearerAuth.test.ts index 8c0b595e1..da2f58381 100644 --- a/src/server/auth/middleware/bearerAuth.test.ts +++ b/src/server/auth/middleware/bearerAuth.test.ts @@ -55,6 +55,57 @@ describe("requireBearerAuth middleware", () => { expect(mockResponse.status).not.toHaveBeenCalled(); expect(mockResponse.json).not.toHaveBeenCalled(); }); + + it("should reject expired tokens", async () => { + const expiredAuthInfo: AuthInfo = { + token: "expired-token", + clientId: "client-123", + scopes: ["read", "write"], + expiresAt: Math.floor(Date.now() / 1000) - 100, // Token expired 100 seconds ago + }; + mockVerifyAccessToken.mockResolvedValue(expiredAuthInfo); + + mockRequest.headers = { + authorization: "Bearer expired-token", + }; + + const middleware = requireBearerAuth({ provider: mockProvider }); + await middleware(mockRequest as Request, mockResponse as Response, nextFunction); + + expect(mockVerifyAccessToken).toHaveBeenCalledWith("expired-token"); + expect(mockResponse.status).toHaveBeenCalledWith(401); + expect(mockResponse.set).toHaveBeenCalledWith( + "WWW-Authenticate", + expect.stringContaining('Bearer error="invalid_token"') + ); + expect(mockResponse.json).toHaveBeenCalledWith( + expect.objectContaining({ error: "invalid_token", error_description: "Token has expired" }) + ); + expect(nextFunction).not.toHaveBeenCalled(); + }); + + it("should accept non-expired tokens", async () => { + const nonExpiredAuthInfo: AuthInfo = { + token: "valid-token", + clientId: "client-123", + scopes: ["read", "write"], + expiresAt: Math.floor(Date.now() / 1000) + 3600, // Token expires in an hour + }; + mockVerifyAccessToken.mockResolvedValue(nonExpiredAuthInfo); + + mockRequest.headers = { + authorization: "Bearer valid-token", + }; + + const middleware = requireBearerAuth({ provider: mockProvider }); + await middleware(mockRequest as Request, mockResponse as Response, nextFunction); + + expect(mockVerifyAccessToken).toHaveBeenCalledWith("valid-token"); + expect(mockRequest.auth).toEqual(nonExpiredAuthInfo); + expect(nextFunction).toHaveBeenCalled(); + expect(mockResponse.status).not.toHaveBeenCalled(); + expect(mockResponse.json).not.toHaveBeenCalled(); + }); it("should require specific scopes when configured", async () => { const authInfo: AuthInfo = { From aad2badb2fd0ff293ba319eece74d77992001c7c Mon Sep 17 00:00:00 2001 From: Jerome Date: Mon, 24 Feb 2025 10:26:59 +0000 Subject: [PATCH 5/9] Removed nested ternaries --- src/server/auth/handlers/register.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/server/auth/handlers/register.ts b/src/server/auth/handlers/register.ts index 7eca5b176..2a74b4a4e 100644 --- a/src/server/auth/handlers/register.ts +++ b/src/server/auth/handlers/register.ts @@ -84,16 +84,17 @@ export function clientRegistrationHandler({ : undefined; const clientIdIssuedAt = Math.floor(Date.now() / 1000); + // Calculate client secret expiry time + const clientsDoExpire = clientSecretExpirySeconds > 0 + const secretExpiryTime = clientsDoExpire ? clientIdIssuedAt + clientSecretExpirySeconds : 0 + const clientSecretExpiresAt = isPublicClient ? undefined : secretExpiryTime + let clientInfo: OAuthClientInformationFull = { ...clientMetadata, client_id: clientId, client_secret: clientSecret, client_id_issued_at: clientIdIssuedAt, - client_secret_expires_at: isPublicClient - ? clientSecretExpirySeconds > 0 - ? clientIdIssuedAt + clientSecretExpirySeconds - : 0 - : undefined, + client_secret_expires_at: clientSecretExpiresAt, }; clientInfo = await clientsStore.registerClient!(clientInfo); From 26b47438c60a673bf500728e7236d0dcd5173438 Mon Sep 17 00:00:00 2001 From: Jerome Date: Mon, 24 Feb 2025 10:39:38 +0000 Subject: [PATCH 6/9] Tidy up CORS error catching --- src/client/auth.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/client/auth.ts b/src/client/auth.ts index 5f5c860e4..c7799429e 100644 --- a/src/client/auth.ts +++ b/src/client/auth.ts @@ -170,11 +170,12 @@ export async function discoverOAuthMetadata( "MCP-Protocol-Version": opts?.protocolVersion ?? LATEST_PROTOCOL_VERSION } }); - } catch { - try { + } catch (error) { + // CORS errors come back as TypeError + if (error instanceof TypeError) { response = await fetch(url); - } catch { - return undefined; + } else { + throw error; } } From 885590c09973c736073b0d6af7df01f14c626c07 Mon Sep 17 00:00:00 2001 From: Jerome Date: Mon, 24 Feb 2025 13:39:58 +0000 Subject: [PATCH 7/9] Fixing tests --- src/client/auth.test.ts | 10 ++++++---- src/server/auth/handlers/register.ts | 6 +++--- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/client/auth.test.ts b/src/client/auth.test.ts index 1a6ef5afb..daa98de5a 100644 --- a/src/client/auth.test.ts +++ b/src/client/auth.test.ts @@ -60,16 +60,18 @@ describe("OAuth Authorization", () => { // Verify second call was made without header expect(mockFetch).toHaveBeenCalledTimes(2); const secondCallOptions = mockFetch.mock.calls[1][1]; - expect(secondCallOptions).toBeUndefined(); // No options means no headers + // The second call still has options but doesn't include MCP-Protocol-Version header + expect(secondCallOptions).toBeDefined(); + expect(secondCallOptions?.headers).toBeUndefined(); }); - it("returns undefined when all fetch attempts fail", async () => { + it("throws an error when all fetch attempts fail", async () => { // Both requests fail mockFetch.mockRejectedValueOnce(new Error("Network error")); mockFetch.mockRejectedValueOnce(new Error("Network error")); - const metadata = await discoverOAuthMetadata("https://auth.example.com"); - expect(metadata).toBeUndefined(); + await expect(discoverOAuthMetadata("https://auth.example.com")) + .rejects.toThrow("Network error"); expect(mockFetch).toHaveBeenCalledTimes(2); }); diff --git a/src/server/auth/handlers/register.ts b/src/server/auth/handlers/register.ts index 2a74b4a4e..30b7cdf8f 100644 --- a/src/server/auth/handlers/register.ts +++ b/src/server/auth/handlers/register.ts @@ -75,13 +75,13 @@ export function clientRegistrationHandler({ } const clientMetadata = parseResult.data; - const isPublicClient = clientMetadata.token_endpoint_auth_method !== 'none' + const isPublicClient = clientMetadata.token_endpoint_auth_method === 'none' // Generate client credentials const clientId = crypto.randomUUID(); const clientSecret = isPublicClient - ? crypto.randomBytes(32).toString('hex') - : undefined; + ? undefined + : crypto.randomBytes(32).toString('hex'); const clientIdIssuedAt = Math.floor(Date.now() / 1000); // Calculate client secret expiry time From 8dfa64338e072178b6cab2f56a9cb11bec4608c5 Mon Sep 17 00:00:00 2001 From: Jerome Date: Mon, 24 Feb 2025 13:48:24 +0000 Subject: [PATCH 8/9] Fixing up tests --- src/client/auth.test.ts | 58 +++++++++++++++++++++++++++++------------ 1 file changed, 42 insertions(+), 16 deletions(-) diff --git a/src/client/auth.test.ts b/src/client/auth.test.ts index daa98de5a..1e892308b 100644 --- a/src/client/auth.test.ts +++ b/src/client/auth.test.ts @@ -44,34 +44,60 @@ describe("OAuth Authorization", () => { }); it("returns metadata when first fetch fails but second without MCP header succeeds", async () => { - // First request with MCP header fails - mockFetch.mockRejectedValueOnce(new Error("Network error")); + // Set up a counter to control behavior + let callCount = 0; - // Second request without header succeeds - mockFetch.mockResolvedValueOnce({ - ok: true, - status: 200, - json: async () => validMetadata, + // Mock implementation that changes behavior based on call count + mockFetch.mockImplementation((url, options) => { + callCount++; + + if (callCount === 1) { + // First call with MCP header - fail with TypeError (simulating CORS error) + // We need to use TypeError specifically because that's what the implementation checks for + return Promise.reject(new TypeError("Network error")); + } else { + // Second call without header - succeed + return Promise.resolve({ + ok: true, + status: 200, + json: async () => validMetadata + }); + } }); + // Should succeed with the second call const metadata = await discoverOAuthMetadata("https://auth.example.com"); expect(metadata).toEqual(validMetadata); - // Verify second call was made without header + // Verify both calls were made expect(mockFetch).toHaveBeenCalledTimes(2); - const secondCallOptions = mockFetch.mock.calls[1][1]; - // The second call still has options but doesn't include MCP-Protocol-Version header - expect(secondCallOptions).toBeDefined(); - expect(secondCallOptions?.headers).toBeUndefined(); + + // Verify first call had MCP header + expect(mockFetch.mock.calls[0][1]?.headers).toHaveProperty("MCP-Protocol-Version"); }); it("throws an error when all fetch attempts fail", async () => { - // Both requests fail - mockFetch.mockRejectedValueOnce(new Error("Network error")); - mockFetch.mockRejectedValueOnce(new Error("Network error")); + // Set up a counter to control behavior + let callCount = 0; + + // Mock implementation that changes behavior based on call count + mockFetch.mockImplementation((url, options) => { + callCount++; + + if (callCount === 1) { + // First call - fail with TypeError + return Promise.reject(new TypeError("First failure")); + } else { + // Second call - fail with different error + return Promise.reject(new Error("Second failure")); + } + }); + // Should fail with the second error await expect(discoverOAuthMetadata("https://auth.example.com")) - .rejects.toThrow("Network error"); + .rejects.toThrow("Second failure"); + + // Verify both calls were made expect(mockFetch).toHaveBeenCalledTimes(2); }); From c521710b86006826d49924213a2a433c8a5dfc3a Mon Sep 17 00:00:00 2001 From: Jerome Date: Tue, 25 Feb 2025 09:51:17 +0000 Subject: [PATCH 9/9] Lint --- src/client/auth.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/client/auth.test.ts b/src/client/auth.test.ts index 1e892308b..02026c4f3 100644 --- a/src/client/auth.test.ts +++ b/src/client/auth.test.ts @@ -48,7 +48,7 @@ describe("OAuth Authorization", () => { let callCount = 0; // Mock implementation that changes behavior based on call count - mockFetch.mockImplementation((url, options) => { + mockFetch.mockImplementation((_url, _options) => { callCount++; if (callCount === 1) { @@ -81,7 +81,7 @@ describe("OAuth Authorization", () => { let callCount = 0; // Mock implementation that changes behavior based on call count - mockFetch.mockImplementation((url, options) => { + mockFetch.mockImplementation((_url, _options) => { callCount++; if (callCount === 1) {