From 801fa31905ce2a0e1966f4a5551cd9a33461a899 Mon Sep 17 00:00:00 2001 From: Chris Funderburg Date: Fri, 16 Oct 2026 00:54:25 +0100 Subject: [PATCH 1/2] fix(mcp): return 405/404 per spec for sessionless and unknown-session requests GET /mcp without a session now returns 405 (no standalone SSE stream), and requests with an unknown or expired session return 404 with JSON-RPC code -32001, so clients re-initialize instead of retrying. The repeated 400s were tripping fail2ban. Extract createMcpHttpServer() so tests can exercise the real handler. --- server/src/mcp/server.ts | 51 ++++++++++++------ server/tests/mcp.test.ts | 113 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 148 insertions(+), 16 deletions(-) diff --git a/server/src/mcp/server.ts b/server/src/mcp/server.ts index 2a3514d..8d7a454 100644 --- a/server/src/mcp/server.ts +++ b/server/src/mcp/server.ts @@ -13,7 +13,7 @@ * Protocol versions: 2025-03-26, 2024-11-05 */ -import { createServer, IncomingMessage, ServerResponse } from "http"; +import { createServer, IncomingMessage, Server, ServerResponse } from "http"; import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { StreamableHTTPServerTransport } from "@modelcontextprotocol/sdk/server/streamableHttp.js"; import { config } from "./config"; @@ -96,6 +96,22 @@ function sendJson(res: ServerResponse, status: number, data: unknown): void { res.end(JSON.stringify(data)); } +/** + * Send 404 for an unknown or expired session. + * Per the MCP spec, clients must re-initialize when they receive this. + * Matches the SDK's own session-not-found response. + */ +function sendSessionNotFound(res: ServerResponse): void { + sendJson(res, 404, { + jsonrpc: "2.0", + error: { + code: -32001, + message: "Session not found", + }, + id: null, + }); +} + /** * Parse request body as JSON */ @@ -123,10 +139,10 @@ interface SessionContext { const sessions = new Map(); /** - * Start the MCP server + * Create the MCP HTTP server without starting it */ -export function startMcpServer(): void { - const server = createServer( +export function createMcpHttpServer(): Server { + return createServer( async (req: IncomingMessage, res: ServerResponse) => { // CORS headers res.setHeader("Access-Control-Allow-Origin", "*"); @@ -225,14 +241,7 @@ export function startMcpServer(): void { transport = sessionCtx.transport; } else if (sessionId) { // Session ID provided but transport not found - session expired - sendJson(res, 400, { - jsonrpc: "2.0", - error: { - code: -32000, - message: "Bad Request: Session not found or expired", - }, - id: null, - }); + sendSessionNotFound(res); return; } else { // No session ID - this should be an initialize request @@ -283,16 +292,19 @@ export function startMcpServer(): void { // Handle GET - SSE streaming (for server-initiated messages) if (req.method === "GET") { - if (!sessionId || !sessions.has(sessionId)) { - sendJson(res, 400, { - error: "Session ID required for SSE connection", + // No session: we offer no standalone SSE stream, so per the MCP + // spec answer 405 rather than 400 + if (!sessionId) { + res.setHeader("Allow", "POST, DELETE"); + sendJson(res, 405, { + error: "SSE stream requires an initialized session", }); return; } const sessionCtx = sessions.get(sessionId); if (!sessionCtx) { - sendJson(res, 400, { error: "Session not found" }); + sendSessionNotFound(res); return; } await sessionCtx.transport.handleRequest(req, res); @@ -308,6 +320,13 @@ export function startMcpServer(): void { sendJson(res, 404, { error: "Not found" }); }, ); +} + +/** + * Start the MCP server + */ +export function startMcpServer(): void { + const server = createMcpHttpServer(); server.listen(config.port, config.host, () => { console.log( diff --git a/server/tests/mcp.test.ts b/server/tests/mcp.test.ts index 1c55798..d98f84b 100644 --- a/server/tests/mcp.test.ts +++ b/server/tests/mcp.test.ts @@ -14,6 +14,8 @@ import path from 'path'; import { SessionStore, sessionStore as globalSessionStore } from '../src/mcp/sessions'; import { loadConfig } from '../src/mcp/config'; import { tools, getTool, executeTool } from '../src/mcp/tools/registry'; +import { createMcpHttpServer } from '../src/mcp/server'; +import { AddressInfo } from 'net'; const TEST_DATA_DIR = path.join(__dirname, '../test-data-mcp'); @@ -607,6 +609,117 @@ describe('MCP HTTP Server Integration', () => { }); }); +// ============================================================================ +// Integration Tests: MCP Session Status Codes (real server) +// ============================================================================ + +describe('MCP Session Status Codes', () => { + let server: Server; + let baseUrl: string; + + // Only needed when MCP_API_KEY is set in the environment + const authHeaders: Record = process.env.MCP_API_KEY + ? { Authorization: `Bearer ${process.env.MCP_API_KEY}` } + : {}; + + beforeAll(async () => { + server = createMcpHttpServer(); + await new Promise((resolve) => server.listen(0, '127.0.0.1', () => resolve())); + const { port } = server.address() as AddressInfo; + baseUrl = `http://127.0.0.1:${port}/mcp`; + }); + + afterAll(async () => { + server.closeAllConnections(); + await new Promise((resolve) => server.close(() => resolve())); + }); + + async function initializeSession(): Promise { + const response = await fetch(baseUrl, { + method: 'POST', + headers: { + ...authHeaders, + 'Content-Type': 'application/json', + Accept: 'application/json, text/event-stream', + }, + body: JSON.stringify({ + jsonrpc: '2.0', + id: 1, + method: 'initialize', + params: { + protocolVersion: '2025-06-18', + capabilities: {}, + clientInfo: { name: 'codex-test', version: '1.0.0' }, + }, + }), + }); + + expect(response.status).toBe(200); + const sessionId = response.headers.get('mcp-session-id'); + if (!sessionId) throw new Error('No session ID returned'); + return sessionId; + } + + it('should return 405 for GET without a session ID', async () => { + const response = await fetch(baseUrl, { + headers: { ...authHeaders, Accept: 'text/event-stream' }, + }); + + expect(response.status).toBe(405); + expect(response.headers.get('allow')).toBe('POST, DELETE'); + }); + + it('should return 404 for GET with an unknown session ID', async () => { + const response = await fetch(baseUrl, { + headers: { + ...authHeaders, + Accept: 'text/event-stream', + 'Mcp-Session-Id': 'unknown-session', + }, + }); + const data = await response.json() as { error: { code: number } }; + + expect(response.status).toBe(404); + expect(data.error.code).toBe(-32001); + }); + + it('should return 404 for POST with an unknown session ID', async () => { + const response = await fetch(baseUrl, { + method: 'POST', + headers: { + ...authHeaders, + 'Content-Type': 'application/json', + Accept: 'application/json, text/event-stream', + 'Mcp-Session-Id': 'unknown-session', + }, + body: JSON.stringify({ jsonrpc: '2.0', id: 2, method: 'tools/list' }), + }); + const data = await response.json() as { error: { code: number } }; + + expect(response.status).toBe(404); + expect(data.error.code).toBe(-32001); + }); + + it('should open an SSE stream for GET with a valid session ID', async () => { + const sessionId = await initializeSession(); + const controller = new AbortController(); + + const response = await fetch(baseUrl, { + headers: { + ...authHeaders, + Accept: 'text/event-stream', + 'Mcp-Session-Id': sessionId, + 'Mcp-Protocol-Version': '2025-06-18', + }, + signal: controller.signal, + }); + + expect(response.status).toBe(200); + expect(response.headers.get('content-type')).toContain('text/event-stream'); + controller.abort(); + }); +}); + // ============================================================================ // Integration Tests: Tool Execution with FileSystem // ============================================================================ From 568529156906fca655c57b0ff427a02cfa01d4f4 Mon Sep 17 00:00:00 2001 From: Chris Funderburg Date: Fri, 16 Oct 2026 00:57:56 +0100 Subject: [PATCH 2/2] fix(mcp): close transports when the session store drops a session The transport map and sessionStore drifted apart: TTL expiry, the cleanup interval and per-key eviction removed store entries but left transports open forever, and requests never refreshed the TTL. Add SessionStore.onDelete() and route every removal through delete(). The server closes the matching transport from that listener, and looks up sessions via the store so each request refreshes the TTL. --- server/src/mcp/server.ts | 41 +++++++++---- server/src/mcp/sessions.ts | 23 ++++++- server/tests/mcp.test.ts | 120 ++++++++++++++++++++++++++++++++++++- 3 files changed, 167 insertions(+), 17 deletions(-) diff --git a/server/src/mcp/server.ts b/server/src/mcp/server.ts index 8d7a454..0b331f9 100644 --- a/server/src/mcp/server.ts +++ b/server/src/mcp/server.ts @@ -138,6 +138,27 @@ interface SessionContext { } const sessions = new Map(); +// sessionStore owns TTL and per-key limits. Close the transport whenever +// it drops a session, so the two maps never drift apart. +sessionStore.onDelete((sessionId) => { + const sessionCtx = sessions.get(sessionId); + if (!sessionCtx) return; + + sessions.delete(sessionId); + sessionCtx.transport.close().catch((error) => { + console.error("[MCP] Error closing transport:", error); + }); +}); + +/** + * Look up a live session. Goes through sessionStore so each request + * refreshes the TTL and an expired session is dropped. + */ +function getActiveSession(sessionId: string): SessionContext | undefined { + if (!sessionStore.validate(sessionId)) return undefined; + return sessions.get(sessionId); +} + /** * Create the MCP HTTP server without starting it */ @@ -214,11 +235,7 @@ export function createMcpHttpServer(): Server { // Handle DELETE - session termination if (req.method === "DELETE") { if (sessionId) { - const sessionCtx = sessions.get(sessionId); - if (sessionCtx) { - await sessionCtx.transport.close(); - sessions.delete(sessionId); - } + // The onDelete listener closes the transport sessionStore.delete(sessionId); } res.writeHead(204); @@ -232,17 +249,15 @@ export function createMcpHttpServer(): Server { // Get or create transport for this session let transport: StreamableHTTPServerTransport; - if (sessionId && sessions.has(sessionId)) { + if (sessionId) { // Existing session - reuse transport - const sessionCtx = sessions.get(sessionId); + const sessionCtx = getActiveSession(sessionId); if (!sessionCtx) { - throw new Error("Session not found despite has() check"); + // Unknown, expired, or evicted session + sendSessionNotFound(res); + return; } transport = sessionCtx.transport; - } else if (sessionId) { - // Session ID provided but transport not found - session expired - sendSessionNotFound(res); - return; } else { // No session ID - this should be an initialize request // Create new MCP server instance for this session @@ -302,7 +317,7 @@ export function createMcpHttpServer(): Server { return; } - const sessionCtx = sessions.get(sessionId); + const sessionCtx = getActiveSession(sessionId); if (!sessionCtx) { sendSessionNotFound(res); return; diff --git a/server/src/mcp/sessions.ts b/server/src/mcp/sessions.ts index 26847e3..71c5ebb 100644 --- a/server/src/mcp/sessions.ts +++ b/server/src/mcp/sessions.ts @@ -14,11 +14,14 @@ interface Session { initialized: boolean; } +type SessionDeleteListener = (sessionId: string) => void; + /** * Simple in-memory session store with TTL and limits */ export class SessionStore { private sessions = new Map(); + private deleteListeners: SessionDeleteListener[] = []; private cleanupInterval: ReturnType | null = null; constructor() { @@ -92,18 +95,32 @@ export class SessionStore { return true; } + /** + * Register a callback that runs whenever a session is removed + * (explicit delete, TTL expiry, or per-key eviction) + */ + onDelete(listener: SessionDeleteListener): void { + this.deleteListeners.push(listener); + } + /** * Delete a session */ delete(sessionId: string): boolean { const deleted = this.sessions.delete(sessionId); - if (deleted && config.debug) { + if (!deleted) return false; + + if (config.debug) { // Sanitize sessionId before logging to prevent log injection (CWE-117) // eslint-disable-next-line no-control-regex const safeId = sessionId.replace(/[\r\n\t\x00-\x1f\x7f]/g, "_"); console.log(`[MCP] Session deleted: ${safeId}`); } - return deleted; + + for (const listener of this.deleteListeners) { + listener(sessionId); + } + return true; } /** @@ -131,7 +148,7 @@ export class SessionStore { for (const [id, session] of this.sessions) { if (now - session.lastAccessedAt > config.sessionTtlMs) { - this.sessions.delete(id); + this.delete(id); cleaned++; } } diff --git a/server/tests/mcp.test.ts b/server/tests/mcp.test.ts index d98f84b..eb5f946 100644 --- a/server/tests/mcp.test.ts +++ b/server/tests/mcp.test.ts @@ -12,7 +12,7 @@ import path from 'path'; // Import MCP components for unit testing import { SessionStore, sessionStore as globalSessionStore } from '../src/mcp/sessions'; -import { loadConfig } from '../src/mcp/config'; +import { config, loadConfig } from '../src/mcp/config'; import { tools, getTool, executeTool } from '../src/mcp/tools/registry'; import { createMcpHttpServer } from '../src/mcp/server'; import { AddressInfo } from 'net'; @@ -217,6 +217,70 @@ describe('MCP Session Store', () => { expect(store.count()).toBe(6); }); }); + + describe('Delete Listeners', () => { + afterEach(() => { + jest.useRealTimers(); + }); + + it('should notify listeners on explicit delete', () => { + const listener = jest.fn(); + store.onDelete(listener); + const session = store.create('test-api-key'); + + store.delete(session.id); + + expect(listener).toHaveBeenCalledTimes(1); + expect(listener).toHaveBeenCalledWith(session.id); + }); + + it('should not notify listeners when deleting a non-existent session', () => { + const listener = jest.fn(); + store.onDelete(listener); + + store.delete('non-existent'); + + expect(listener).not.toHaveBeenCalled(); + }); + + it('should notify listeners when a session is evicted by the per-key limit', () => { + const listener = jest.fn(); + store.onDelete(listener); + const oldest = store.create('same-api-key'); + for (let i = 0; i < config.maxSessionsPerKey; i++) { + store.create('same-api-key'); + } + + expect(listener).toHaveBeenCalledWith(oldest.id); + }); + + it('should notify listeners when get() finds an expired session', () => { + jest.useFakeTimers({ now: Date.now() }); + const listener = jest.fn(); + store.onDelete(listener); + const session = store.create('test-api-key'); + + jest.setSystemTime(Date.now() + config.sessionTtlMs + 1); + + expect(store.get(session.id)).toBeUndefined(); + expect(listener).toHaveBeenCalledWith(session.id); + }); + + it('should notify listeners when the cleanup interval removes an expired session', () => { + jest.useFakeTimers({ now: Date.now() }); + // Create after faking timers so the cleanup interval is faked too + const timedStore = new SessionStore(); + const listener = jest.fn(); + timedStore.onDelete(listener); + const session = timedStore.create('test-api-key'); + + jest.advanceTimersByTime(config.sessionTtlMs + 60000); + + expect(timedStore.count()).toBe(0); + expect(listener).toHaveBeenCalledWith(session.id); + timedStore.stopCleanup(); + }); + }); }); // ============================================================================ @@ -718,6 +782,60 @@ describe('MCP Session Status Codes', () => { expect(response.headers.get('content-type')).toContain('text/event-stream'); controller.abort(); }); + + async function getWithSession(sessionId: string): Promise { + return fetch(baseUrl, { + headers: { + ...authHeaders, + Accept: 'text/event-stream', + 'Mcp-Session-Id': sessionId, + }, + }); + } + + it('should return 404 after the session is terminated with DELETE', async () => { + const sessionId = await initializeSession(); + + const deleteResponse = await fetch(baseUrl, { + method: 'DELETE', + headers: { ...authHeaders, 'Mcp-Session-Id': sessionId }, + }); + expect(deleteResponse.status).toBe(204); + expect(globalSessionStore.validate(sessionId)).toBe(false); + + const response = await getWithSession(sessionId); + expect(response.status).toBe(404); + }); + + it('should return 404 after the session store drops the session', async () => { + const sessionId = await initializeSession(); + + // Same path as TTL expiry and the cleanup interval + globalSessionStore.delete(sessionId); + + const response = await getWithSession(sessionId); + expect(response.status).toBe(404); + }); + + it('should return 404 for a session evicted by the per-key limit', async () => { + const oldestId = await initializeSession(); + for (let i = 0; i < config.maxSessionsPerKey; i++) { + await initializeSession(); + } + + const response = await fetch(baseUrl, { + method: 'POST', + headers: { + ...authHeaders, + 'Content-Type': 'application/json', + Accept: 'application/json, text/event-stream', + 'Mcp-Session-Id': oldestId, + }, + body: JSON.stringify({ jsonrpc: '2.0', id: 3, method: 'tools/list' }), + }); + + expect(response.status).toBe(404); + }); }); // ============================================================================