From 7f6c0465f51ec44c06aa08281f3d5aa1af7a0c26 Mon Sep 17 00:00:00 2001 From: Tuomas Date: Mon, 17 Feb 2025 15:57:02 -0600 Subject: [PATCH 01/14] Add timeout reset on progress notifications --- src/shared/protocol.test.ts | 176 ++++++++++++++++++++++++++++++++++++ src/shared/protocol.ts | 166 ++++++++++++++++++++++++---------- 2 files changed, 292 insertions(+), 50 deletions(-) diff --git a/src/shared/protocol.test.ts b/src/shared/protocol.test.ts index 3073d0af..bf0357b2 100644 --- a/src/shared/protocol.test.ts +++ b/src/shared/protocol.test.ts @@ -62,6 +62,182 @@ describe("protocol tests", () => { await transport.close(); expect(oncloseMock).toHaveBeenCalled(); }); + + test("should reset timeout when progress notification is received", async () => { + jest.useFakeTimers(); + + await protocol.connect(transport); + const request = { method: "example", params: {} }; + const mockSchema: ZodType<{ result: string }> = z.object({ + result: z.string(), + }); + + const onProgressMock = jest.fn(); + const requestPromise = protocol.request(request, mockSchema, { + timeout: 1000, // Increased timeout for more reliable testing + resetTimeoutOnProgress: true, + onprogress: onProgressMock, + }); + + // Advance time close to timeout + jest.advanceTimersByTime(800); + + // Send progress notification + if (transport.onmessage) { + transport.onmessage({ + jsonrpc: "2.0", + method: "notifications/progress", + params: { + progressToken: 0, + progress: 50, + total: 100, + }, + }); + } + + // Run all pending promises to ensure progress handler is called + await Promise.resolve(); + + // Verify progress handler was called + expect(onProgressMock).toHaveBeenCalledWith({ + progress: 50, + total: 100, + }); + + // Send success response + if (transport.onmessage) { + transport.onmessage({ + jsonrpc: "2.0", + id: 0, + result: { result: "success" }, + }); + } + + // Run all pending promises + await Promise.resolve(); + + await expect(requestPromise).resolves.toEqual({ result: "success" }); + + jest.useRealTimers(); + }); + + test("should respect maxTotalTimeout", async () => { + jest.useFakeTimers(); + + await protocol.connect(transport); + const request = { method: "example", params: {} }; + const mockSchema: ZodType<{ result: string }> = z.object({ + result: z.string(), + }); + + const onProgressMock = jest.fn(); + const requestPromise = protocol.request(request, mockSchema, { + timeout: 1000, + maxTotalTimeout: 100, + resetTimeoutOnProgress: true, + onprogress: onProgressMock, + }); + + // Advance time beyond maxTotalTimeout + jest.advanceTimersByTime(150); + + // Send progress notification after maxTotalTimeout + if (transport.onmessage) { + transport.onmessage({ + jsonrpc: "2.0", + method: "notifications/progress", + params: { + progressToken: 0, + progress: 50, + total: 100, + }, + }); + } + + await expect(requestPromise).rejects.toThrow("Maximum total timeout exceeded"); + expect(onProgressMock).not.toHaveBeenCalled(); + + jest.useRealTimers(); + }); + + test("should timeout if no progress received within timeout period", async () => { + jest.useFakeTimers(); + + await protocol.connect(transport); + const request = { method: "example", params: {} }; + const mockSchema: ZodType<{ result: string }> = z.object({ + result: z.string(), + }); + + const requestPromise = protocol.request(request, mockSchema, { + timeout: 100, + resetTimeoutOnProgress: true, + }); + + // Advance time beyond timeout + jest.advanceTimersByTime(101); + + await expect(requestPromise).rejects.toThrow("Request timed out"); + + jest.useRealTimers(); + }); + + test("should handle multiple progress notifications correctly", async () => { + jest.useFakeTimers(); + + await protocol.connect(transport); + const request = { method: "example", params: {} }; + const mockSchema: ZodType<{ result: string }> = z.object({ + result: z.string(), + }); + + const onProgressMock = jest.fn(); + const requestPromise = protocol.request(request, mockSchema, { + timeout: 1000, + resetTimeoutOnProgress: true, + onprogress: onProgressMock, + }); + + // Simulate multiple progress updates + for (let i = 1; i <= 3; i++) { + // Advance close to timeout + jest.advanceTimersByTime(800); + + // Send progress notification + if (transport.onmessage) { + transport.onmessage({ + jsonrpc: "2.0", + method: "notifications/progress", + params: { + progressToken: 0, + progress: i * 25, + total: 100, + }, + }); + } + + // Verify progress handler was called + await Promise.resolve(); + expect(onProgressMock).toHaveBeenNthCalledWith(i, { + progress: i * 25, + total: 100, + }); + } + + // Send success response + if (transport.onmessage) { + transport.onmessage({ + jsonrpc: "2.0", + id: 0, + result: { result: "success" }, + }); + } + + await Promise.resolve(); + await expect(requestPromise).resolves.toEqual({ result: "success" }); + + jest.useRealTimers(); + }); }); describe("mergeCapabilities", () => { diff --git a/src/shared/protocol.ts b/src/shared/protocol.ts index a4f211c6..7e58cbe1 100644 --- a/src/shared/protocol.ts +++ b/src/shared/protocol.ts @@ -64,6 +64,20 @@ export type RequestOptions = { * If not specified, `DEFAULT_REQUEST_TIMEOUT_MSEC` will be used as the timeout. */ timeout?: number; + + /** + * If true, receiving a progress notification will reset the request timeout. + * This is useful for long-running operations that send periodic progress updates. + * Default: false + */ + resetTimeoutOnProgress?: boolean; + + /** + * Maximum total time (in milliseconds) to wait for a response, even if progress notifications are received. + * Only used when resetTimeoutOnProgress is true. + * If not specified, there is no maximum total timeout. + */ + maxTotalTimeout?: number; }; /** @@ -76,6 +90,17 @@ export type RequestHandlerExtra = { signal: AbortSignal; }; +/** + * Information about a request's timeout state + */ +type TimeoutInfo = { + timeoutId: ReturnType; + startTime: number; + timeout: number; + maxTotalTimeout?: number; + onTimeout: () => void; +}; + /** * Implements MCP protocol framing on top of a pluggable transport, including * features like request/response linking, notifications, and progress. @@ -105,6 +130,7 @@ export abstract class Protocol< (response: JSONRPCResponse | Error) => void > = new Map(); private _progressHandlers: Map = new Map(); + private _timeoutInfo: Map = new Map(); /** * Callback for when the connection is closed for any reason. @@ -149,6 +175,49 @@ export abstract class Protocol< ); } + private _setupTimeout( + messageId: number, + timeout: number, + maxTotalTimeout: number | undefined, + onTimeout: () => void + ) { + this._timeoutInfo.set(messageId, { + timeoutId: setTimeout(onTimeout, timeout), + startTime: Date.now(), + timeout, + maxTotalTimeout, + onTimeout + }); + } + + private _resetTimeout(messageId: number, cancel: (reason: unknown) => void): boolean { + const info = this._timeoutInfo.get(messageId); + if (!info) return false; + + const totalElapsed = Date.now() - info.startTime; + if (info.maxTotalTimeout && totalElapsed >= info.maxTotalTimeout) { + this._timeoutInfo.delete(messageId); + cancel(new McpError( + ErrorCode.RequestTimeout, + "Maximum total timeout exceeded", + { maxTotalTimeout: info.maxTotalTimeout, totalElapsed } + )); + return false; + } + + clearTimeout(info.timeoutId); + info.timeoutId = setTimeout(info.onTimeout, info.timeout); + return true; + } + + private _cleanupTimeout(messageId: number) { + const info = this._timeoutInfo.get(messageId); + if (info) { + clearTimeout(info.timeoutId); + this._timeoutInfo.delete(messageId); + } + } + /** * Attaches to the given transport, starts it, and starts listening for messages. * @@ -281,22 +350,27 @@ export abstract class Protocol< private _onprogress(notification: ProgressNotification): void { const { progressToken, ...params } = notification.params; - const handler = this._progressHandlers.get(Number(progressToken)); - if (handler === undefined) { - this._onerror( - new Error( - `Received a progress notification for an unknown token: ${JSON.stringify(notification)}`, - ), - ); + const messageId = Number(progressToken); + + const handler = this._progressHandlers.get(messageId); + if (!handler) { + this._onerror(new Error(`Received a progress notification for an unknown token: ${JSON.stringify(notification)}`)); return; } + const responseHandler = this._responseHandlers.get(messageId); + if (this._timeoutInfo.has(messageId) && responseHandler) { + if (!this._resetTimeout(messageId, (reason) => responseHandler(reason as Error))) { + return; + } + } + handler(params); } private _onresponse(response: JSONRPCResponse | JSONRPCError): void { - const messageId = response.id; - const handler = this._responseHandlers.get(Number(messageId)); + const messageId = Number(response.id); + const handler = this._responseHandlers.get(messageId); if (handler === undefined) { this._onerror( new Error( @@ -306,8 +380,10 @@ export abstract class Protocol< return; } - this._responseHandlers.delete(Number(messageId)); - this._progressHandlers.delete(Number(messageId)); + this._responseHandlers.delete(messageId); + this._progressHandlers.delete(messageId); + this._cleanupTimeout(messageId); + if ("result" in response) { handler(response); } else { @@ -393,32 +469,10 @@ export abstract class Protocol< }; } - let timeoutId: ReturnType | undefined = undefined; - - this._responseHandlers.set(messageId, (response) => { - if (timeoutId !== undefined) { - clearTimeout(timeoutId); - } - - if (options?.signal?.aborted) { - return; - } - - if (response instanceof Error) { - return reject(response); - } - - try { - const result = resultSchema.parse(response.result); - resolve(result); - } catch (error) { - reject(error); - } - }); - const cancel = (reason: unknown) => { this._responseHandlers.delete(messageId); this._progressHandlers.delete(messageId); + this._cleanupTimeout(messageId); this._transport ?.send({ @@ -436,30 +490,42 @@ export abstract class Protocol< reject(reason); }; - options?.signal?.addEventListener("abort", () => { - if (timeoutId !== undefined) { - clearTimeout(timeoutId); + this._responseHandlers.set(messageId, (response) => { + if (options?.signal?.aborted) { + return; } + if (response instanceof Error) { + return reject(response); + } + + try { + const result = resultSchema.parse(response.result); + resolve(result); + } catch (error) { + reject(error); + } + }); + + options?.signal?.addEventListener("abort", () => { cancel(options?.signal?.reason); }); const timeout = options?.timeout ?? DEFAULT_REQUEST_TIMEOUT_MSEC; - timeoutId = setTimeout( - () => - cancel( - new McpError(ErrorCode.RequestTimeout, "Request timed out", { - timeout, - }), - ), - timeout, - ); + const timeoutHandler = () => cancel(new McpError( + ErrorCode.RequestTimeout, + "Request timed out", + { timeout } + )); + + if (options?.resetTimeoutOnProgress) { + this._setupTimeout(messageId, timeout, options.maxTotalTimeout, timeoutHandler); + } else { + this._setupTimeout(messageId, timeout, undefined, timeoutHandler); + } this._transport.send(jsonrpcRequest).catch((error) => { - if (timeoutId !== undefined) { - clearTimeout(timeoutId); - } - + this._cleanupTimeout(messageId); reject(error); }); }); From 28415d3917c1d31a13adeeaaeab5420602a2a8b6 Mon Sep 17 00:00:00 2001 From: Tuomas Date: Wed, 19 Feb 2025 12:23:16 -0600 Subject: [PATCH 02/14] Refactor progress notification timeout handling in protocol --- src/shared/protocol.test.ts | 284 +++++++++++++++++------------------- src/shared/protocol.ts | 24 +-- 2 files changed, 144 insertions(+), 164 deletions(-) diff --git a/src/shared/protocol.test.ts b/src/shared/protocol.test.ts index bf0357b2..1d037b98 100644 --- a/src/shared/protocol.test.ts +++ b/src/shared/protocol.test.ts @@ -63,180 +63,160 @@ describe("protocol tests", () => { expect(oncloseMock).toHaveBeenCalled(); }); - test("should reset timeout when progress notification is received", async () => { - jest.useFakeTimers(); - - await protocol.connect(transport); - const request = { method: "example", params: {} }; - const mockSchema: ZodType<{ result: string }> = z.object({ - result: z.string(), + describe("progress notification timeout behavior", () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + afterEach(() => { + jest.useRealTimers(); }); - const onProgressMock = jest.fn(); - const requestPromise = protocol.request(request, mockSchema, { - timeout: 1000, // Increased timeout for more reliable testing - resetTimeoutOnProgress: true, - onprogress: onProgressMock, - }); - - // Advance time close to timeout - jest.advanceTimersByTime(800); - - // Send progress notification - if (transport.onmessage) { - transport.onmessage({ - jsonrpc: "2.0", - method: "notifications/progress", - params: { - progressToken: 0, - progress: 50, - total: 100, - }, + test("should reset timeout when progress notification is received", async () => { + await protocol.connect(transport); + const request = { method: "example", params: {} }; + const mockSchema: ZodType<{ result: string }> = z.object({ + result: z.string(), }); - } - - // Run all pending promises to ensure progress handler is called - await Promise.resolve(); - - // Verify progress handler was called - expect(onProgressMock).toHaveBeenCalledWith({ - progress: 50, - total: 100, - }); - - // Send success response - if (transport.onmessage) { - transport.onmessage({ - jsonrpc: "2.0", - id: 0, - result: { result: "success" }, + const onProgressMock = jest.fn(); + const requestPromise = protocol.request(request, mockSchema, { + timeout: 1000, + resetTimeoutOnProgress: true, + onprogress: onProgressMock, }); - } - - // Run all pending promises - await Promise.resolve(); - - await expect(requestPromise).resolves.toEqual({ result: "success" }); - - jest.useRealTimers(); - }); - - test("should respect maxTotalTimeout", async () => { - jest.useFakeTimers(); - - await protocol.connect(transport); - const request = { method: "example", params: {} }; - const mockSchema: ZodType<{ result: string }> = z.object({ - result: z.string(), - }); - - const onProgressMock = jest.fn(); - const requestPromise = protocol.request(request, mockSchema, { - timeout: 1000, - maxTotalTimeout: 100, - resetTimeoutOnProgress: true, - onprogress: onProgressMock, - }); - - // Advance time beyond maxTotalTimeout - jest.advanceTimersByTime(150); - - // Send progress notification after maxTotalTimeout - if (transport.onmessage) { - transport.onmessage({ - jsonrpc: "2.0", - method: "notifications/progress", - params: { - progressToken: 0, - progress: 50, - total: 100, - }, - }); - } - - await expect(requestPromise).rejects.toThrow("Maximum total timeout exceeded"); - expect(onProgressMock).not.toHaveBeenCalled(); - - jest.useRealTimers(); - }); - - test("should timeout if no progress received within timeout period", async () => { - jest.useFakeTimers(); - - await protocol.connect(transport); - const request = { method: "example", params: {} }; - const mockSchema: ZodType<{ result: string }> = z.object({ - result: z.string(), - }); - - const requestPromise = protocol.request(request, mockSchema, { - timeout: 100, - resetTimeoutOnProgress: true, - }); - - // Advance time beyond timeout - jest.advanceTimersByTime(101); - - await expect(requestPromise).rejects.toThrow("Request timed out"); - - jest.useRealTimers(); - }); - - test("should handle multiple progress notifications correctly", async () => { - jest.useFakeTimers(); - - await protocol.connect(transport); - const request = { method: "example", params: {} }; - const mockSchema: ZodType<{ result: string }> = z.object({ - result: z.string(), - }); - - const onProgressMock = jest.fn(); - const requestPromise = protocol.request(request, mockSchema, { - timeout: 1000, - resetTimeoutOnProgress: true, - onprogress: onProgressMock, - }); - - // Simulate multiple progress updates - for (let i = 1; i <= 3; i++) { - // Advance close to timeout jest.advanceTimersByTime(800); - - // Send progress notification if (transport.onmessage) { transport.onmessage({ jsonrpc: "2.0", method: "notifications/progress", params: { progressToken: 0, - progress: i * 25, + progress: 50, total: 100, }, }); } - - // Verify progress handler was called await Promise.resolve(); - expect(onProgressMock).toHaveBeenNthCalledWith(i, { - progress: i * 25, + expect(onProgressMock).toHaveBeenCalledWith({ + progress: 50, total: 100, }); - } + jest.advanceTimersByTime(800); + if (transport.onmessage) { + transport.onmessage({ + jsonrpc: "2.0", + id: 0, + result: { result: "success" }, + }); + } + await Promise.resolve(); + await expect(requestPromise).resolves.toEqual({ result: "success" }); + }); - // Send success response - if (transport.onmessage) { - transport.onmessage({ - jsonrpc: "2.0", - id: 0, - result: { result: "success" }, + test("should respect maxTotalTimeout", async () => { + await protocol.connect(transport); + const request = { method: "example", params: {} }; + const mockSchema: ZodType<{ result: string }> = z.object({ + result: z.string(), + }); + const onProgressMock = jest.fn(); + const requestPromise = protocol.request(request, mockSchema, { + timeout: 1000, + maxTotalTimeout: 150, + resetTimeoutOnProgress: true, + onprogress: onProgressMock, }); - } - await Promise.resolve(); - await expect(requestPromise).resolves.toEqual({ result: "success" }); + // First progress notification should work + jest.advanceTimersByTime(80); + if (transport.onmessage) { + transport.onmessage({ + jsonrpc: "2.0", + method: "notifications/progress", + params: { + progressToken: 0, + progress: 50, + total: 100, + }, + }); + } + await Promise.resolve(); + expect(onProgressMock).toHaveBeenCalledWith({ + progress: 50, + total: 100, + }); + jest.advanceTimersByTime(80); + if (transport.onmessage) { + transport.onmessage({ + jsonrpc: "2.0", + method: "notifications/progress", + params: { + progressToken: 0, + progress: 75, + total: 100, + }, + }); + } + await expect(requestPromise).rejects.toThrow("Maximum total timeout exceeded"); + expect(onProgressMock).toHaveBeenCalledTimes(1); + }); - jest.useRealTimers(); + test("should timeout if no progress received within timeout period", async () => { + await protocol.connect(transport); + const request = { method: "example", params: {} }; + const mockSchema: ZodType<{ result: string }> = z.object({ + result: z.string(), + }); + const requestPromise = protocol.request(request, mockSchema, { + timeout: 100, + resetTimeoutOnProgress: true, + }); + jest.advanceTimersByTime(101); + await expect(requestPromise).rejects.toThrow("Request timed out"); + }); + + test("should handle multiple progress notifications correctly", async () => { + await protocol.connect(transport); + const request = { method: "example", params: {} }; + const mockSchema: ZodType<{ result: string }> = z.object({ + result: z.string(), + }); + const onProgressMock = jest.fn(); + const requestPromise = protocol.request(request, mockSchema, { + timeout: 1000, + resetTimeoutOnProgress: true, + onprogress: onProgressMock, + }); + + // Simulate multiple progress updates + for (let i = 1; i <= 3; i++) { + jest.advanceTimersByTime(800); + if (transport.onmessage) { + transport.onmessage({ + jsonrpc: "2.0", + method: "notifications/progress", + params: { + progressToken: 0, + progress: i * 25, + total: 100, + }, + }); + } + await Promise.resolve(); + expect(onProgressMock).toHaveBeenNthCalledWith(i, { + progress: i * 25, + total: 100, + }); + } + if (transport.onmessage) { + transport.onmessage({ + jsonrpc: "2.0", + id: 0, + result: { result: "success" }, + }); + } + await Promise.resolve(); + await expect(requestPromise).resolves.toEqual({ result: "success" }); + }); }); }); diff --git a/src/shared/protocol.ts b/src/shared/protocol.ts index 7e58cbe1..a0f3d075 100644 --- a/src/shared/protocol.ts +++ b/src/shared/protocol.ts @@ -73,8 +73,8 @@ export type RequestOptions = { resetTimeoutOnProgress?: boolean; /** - * Maximum total time (in milliseconds) to wait for a response, even if progress notifications are received. - * Only used when resetTimeoutOnProgress is true. + * Maximum total time (in milliseconds) to wait for a response. + * If exceeded, an McpError with code `RequestTimeout` will be raised, regardless of progress notifications. * If not specified, there is no maximum total timeout. */ maxTotalTimeout?: number; @@ -190,19 +190,18 @@ export abstract class Protocol< }); } - private _resetTimeout(messageId: number, cancel: (reason: unknown) => void): boolean { + private _resetTimeout(messageId: number): boolean { const info = this._timeoutInfo.get(messageId); if (!info) return false; const totalElapsed = Date.now() - info.startTime; if (info.maxTotalTimeout && totalElapsed >= info.maxTotalTimeout) { this._timeoutInfo.delete(messageId); - cancel(new McpError( + throw new McpError( ErrorCode.RequestTimeout, "Maximum total timeout exceeded", { maxTotalTimeout: info.maxTotalTimeout, totalElapsed } - )); - return false; + ); } clearTimeout(info.timeoutId); @@ -360,7 +359,12 @@ export abstract class Protocol< const responseHandler = this._responseHandlers.get(messageId); if (this._timeoutInfo.has(messageId) && responseHandler) { - if (!this._resetTimeout(messageId, (reason) => responseHandler(reason as Error))) { + try { + if (!this._resetTimeout(messageId)) { + return; + } + } catch (error) { + responseHandler(error as Error); return; } } @@ -518,11 +522,7 @@ export abstract class Protocol< { timeout } )); - if (options?.resetTimeoutOnProgress) { - this._setupTimeout(messageId, timeout, options.maxTotalTimeout, timeoutHandler); - } else { - this._setupTimeout(messageId, timeout, undefined, timeoutHandler); - } + this._setupTimeout(messageId, timeout, options?.maxTotalTimeout, timeoutHandler); this._transport.send(jsonrpcRequest).catch((error) => { this._cleanupTimeout(messageId); From b09240bc1082a30785f056162da8fa3a57b794f6 Mon Sep 17 00:00:00 2001 From: Tuomas Date: Thu, 20 Feb 2025 15:36:57 -0600 Subject: [PATCH 03/14] only return early from progress notification on actual timeout errors --- src/shared/protocol.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/shared/protocol.ts b/src/shared/protocol.ts index a0f3d075..97213bf0 100644 --- a/src/shared/protocol.ts +++ b/src/shared/protocol.ts @@ -360,9 +360,7 @@ export abstract class Protocol< const responseHandler = this._responseHandlers.get(messageId); if (this._timeoutInfo.has(messageId) && responseHandler) { try { - if (!this._resetTimeout(messageId)) { - return; - } + this._resetTimeout(messageId); } catch (error) { responseHandler(error as Error); return; From 1b8e94ca4091c2db442961c033ca512469aa9f6b Mon Sep 17 00:00:00 2001 From: Jerome Date: Sat, 22 Feb 2025 16:20:41 +0000 Subject: [PATCH 04/14] 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 c0bac9e8..5f5c860e 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 05/14] 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 14109e17..cd1b314a 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 06/14] 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 675e8733..7eca5b17 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 07/14] 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 97c9e14a..1a6ef5af 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 5faf1a4a..a961f654 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 8c0b595e..da2f5838 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 d5906b7cf206e2b66f4d8f843a24f213fe7a23dd Mon Sep 17 00:00:00 2001 From: Jerome Date: Mon, 24 Feb 2025 09:17:26 +0000 Subject: [PATCH 08/14] Bumping version for auth release --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 8e3771f2..21ed2505 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@modelcontextprotocol/sdk", - "version": "1.5.0", + "version": "1.6.0", "description": "Model Context Protocol implementation for TypeScript", "license": "MIT", "author": "Anthropic, PBC (https://anthropic.com)", From aad2badb2fd0ff293ba319eece74d77992001c7c Mon Sep 17 00:00:00 2001 From: Jerome Date: Mon, 24 Feb 2025 10:26:59 +0000 Subject: [PATCH 09/14] 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 7eca5b17..2a74b4a4 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 10/14] 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 5f5c860e..c7799429 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 11/14] 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 1a6ef5af..daa98de5 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 2a74b4a4..30b7cdf8 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 12/14] 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 daa98de5..1e892308 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 13/14] 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 1e892308..02026c4f 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) { From ac729189517d9eed026697e78cdd8766adc67dbc Mon Sep 17 00:00:00 2001 From: Ashwin Bhat Date: Wed, 26 Feb 2025 16:10:31 -0800 Subject: [PATCH 14/14] Add CLAUDE.md with SDK development guide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🤖 Generated with [Claude Code](https://docs.anthropic.com/s/claude-code) Co-Authored-By: Claude --- CLAUDE.md | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 CLAUDE.md diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 00000000..8e084134 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,25 @@ +# MCP TypeScript SDK Guide + +## Build & Test Commands +``` +npm run build # Build ESM and CJS versions +npm run lint # Run ESLint +npm test # Run all tests +npx jest path/to/file.test.ts # Run specific test file +npx jest -t "test name" # Run tests matching pattern +``` + +## Code Style Guidelines +- **TypeScript**: Strict type checking, ES modules, explicit return types +- **Naming**: PascalCase for classes/types, camelCase for functions/variables +- **Files**: Lowercase with hyphens, test files with `.test.ts` suffix +- **Imports**: ES module style, include `.js` extension, group imports logically +- **Error Handling**: Use TypeScript's strict mode, explicit error checking in tests +- **Formatting**: 2-space indentation, semicolons required, single quotes preferred +- **Testing**: Co-locate tests with source files, use descriptive test names +- **Comments**: JSDoc for public APIs, inline comments for complex logic + +## Project Structure +- `/src`: Source code with client, server, and shared modules +- Tests alongside source files with `.test.ts` suffix +- Node.js >= 18 required \ No newline at end of file