From 7b3d441046669ad61d95c987f06c32fa0b13e28c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 12 Oct 2025 07:45:21 +0000 Subject: [PATCH] Add per-session server instance support to fix concurrent user isolation Co-authored-by: samanhappy <2755122+samanhappy@users.noreply.github.com> --- docs/configuration/mcp-settings.mdx | 8 +- docs/zh/configuration/mcp-settings.mdx | 10 +- mcp_settings.json | 6 +- src/services/mcpService.ts | 224 +++++++++++++++++++++-- src/types/index.ts | 2 + tests/services/perSessionServers.test.ts | 112 ++++++++++++ 6 files changed, 343 insertions(+), 19 deletions(-) create mode 100644 tests/services/perSessionServers.test.ts diff --git a/docs/configuration/mcp-settings.mdx b/docs/configuration/mcp-settings.mdx index 49cc9c6..eec8850 100644 --- a/docs/configuration/mcp-settings.mdx +++ b/docs/configuration/mcp-settings.mdx @@ -47,7 +47,8 @@ MCPHub uses several configuration files: }, "playwright": { "command": "npx", - "args": ["@playwright/mcp@latest", "--headless"] + "args": ["@playwright/mcp@latest", "--headless", "--isolated"], + "perSession": true }, "slack": { "command": "npx", @@ -101,8 +102,9 @@ MCPHub uses several configuration files: { "playwright": { "command": "npx", - "args": ["@playwright/mcp@latest", "--headless"], + "args": ["@playwright/mcp@latest", "--headless", "--isolated"], "timeout": 60000, + "perSession": true, "env": { "PLAYWRIGHT_BROWSERS_PATH": "/tmp/browsers" } @@ -110,6 +112,8 @@ MCPHub uses several configuration files: } ``` +**Note**: The `--isolated` flag ensures each browser session is isolated, and `perSession: true` creates a separate server instance for each user session, preventing state leakage between concurrent users. + ### File and System Servers #### Filesystem Server diff --git a/docs/zh/configuration/mcp-settings.mdx b/docs/zh/configuration/mcp-settings.mdx index fdfdd89..9519691 100644 --- a/docs/zh/configuration/mcp-settings.mdx +++ b/docs/zh/configuration/mcp-settings.mdx @@ -50,8 +50,9 @@ MCPHub 使用几个配置文件： }, "playwright": { "command": "npx", - "args": ["@playwright/mcp@latest", "--headless"], - "timeout": 60000 + "args": ["@playwright/mcp@latest", "--headless", "--isolated"], + "timeout": 60000, + "perSession": true }, "slack": { "command": "npx", @@ -111,8 +112,9 @@ MCPHub 使用几个配置文件： { "playwright": { "command": "npx", - "args": ["@playwright/mcp@latest", "--headless"], + "args": ["@playwright/mcp@latest", "--headless", "--isolated"], "timeout": 60000, + "perSession": true, "env": { "PLAYWRIGHT_BROWSERS_PATH": "/tmp/browsers" } @@ -120,6 +122,8 @@ MCPHub 使用几个配置文件： } ``` +**注意**: `--isolated` 标志确保每个浏览器会话是隔离的，而 `perSession: true` 为每个用户会话创建单独的服务器实例，防止并发用户之间的状态泄漏。 + ### 文件和系统服务器 #### 文件系统服务器 diff --git a/mcp_settings.json b/mcp_settings.json index c7c15b1..9dd1621 100644 --- a/mcp_settings.json +++ b/mcp_settings.json @@ -14,8 +14,10 @@ "command": "npx", "args": [ "@playwright/mcp@latest", - "--headless" - ] + "--headless", + "--isolated" + ], + "perSession": true }, "fetch": { "command": "uvx", diff --git a/src/services/mcpService.ts b/src/services/mcpService.ts index 0042ac6..f3fc195 100644 --- a/src/services/mcpService.ts +++ b/src/services/mcpService.ts @@ -24,6 +24,10 @@ import { getServerDao, ServerConfigWithName } from '../dao/index.js'; const servers: { [sessionId: string]: Server } = {}; +// Per-session server instances for servers with perSession=true +// Key format: `${sessionId}:${serverName}` +const perSessionServerInfos: { [key: string]: ServerInfo } = {}; + const serverDao = getServerDao(); // Helper function to set up keep-alive ping for SSE connections @@ -79,6 +83,8 @@ export const getMcpServer = (sessionId?: string, group?: string): Server => { export const deleteMcpServer = (sessionId: string): void => { delete servers[sessionId]; + // Clean up any per-session servers for this session + cleanupPerSessionServers(sessionId); }; export const notifyToolChanged = async (name?: string) => { @@ -223,6 +229,144 @@ const createTransportFromConfig = (name: string, conf: ServerConfig): any => { return transport; }; +// Helper function to get or create per-session server instance +export const getOrCreatePerSessionServer = async ( + sessionId: string, + serverName: string, + serverConfig: ServerConfig, +): Promise => { + const key = `${sessionId}:${serverName}`; + + // Return existing session server if it exists + if (perSessionServerInfos[key]) { + return perSessionServerInfos[key]; + } + + console.log(`Creating per-session server instance for session ${sessionId}, server ${serverName}`); + + // Create new transport for this session + const transport = createTransportFromConfig(serverName, serverConfig); + + const client = new Client( + { + name: `mcp-client-${serverName}-${sessionId}`, + version: '1.0.0', + }, + { + capabilities: { + prompts: {}, + resources: {}, + tools: {}, + }, + }, + ); + + // Get request options from server configuration, with fallbacks + const serverRequestOptions = serverConfig.options || {}; + const requestOptions = { + timeout: serverRequestOptions.timeout || 60000, + resetTimeoutOnProgress: serverRequestOptions.resetTimeoutOnProgress || false, + maxTotalTimeout: serverRequestOptions.maxTotalTimeout, + }; + + // Create server info for this session + const serverInfo: ServerInfo = { + name: serverName, + owner: serverConfig.owner, + status: 'connecting', + error: null, + tools: [], + prompts: [], + client, + transport, + options: requestOptions, + createTime: Date.now(), + config: serverConfig, + sessionId: sessionId, + }; + + perSessionServerInfos[key] = serverInfo; + + // Connect asynchronously + client + .connect(transport, requestOptions) + .then(() => { + console.log(`Successfully connected per-session client for server: ${serverName}, session: ${sessionId}`); + const capabilities = client.getServerCapabilities(); + + if (capabilities?.tools) { + client + .listTools({}, requestOptions) + .then((tools) => { + console.log(`Successfully listed ${tools.tools.length} tools for per-session server: ${serverName}, session: ${sessionId}`); + serverInfo.tools = tools.tools.map((tool) => ({ + name: `${serverName}${getNameSeparator()}${tool.name}`, + description: tool.description || '', + inputSchema: cleanInputSchema(tool.inputSchema || {}), + })); + }) + .catch((error) => { + console.error(`Failed to list tools for per-session server ${serverName}, session ${sessionId}:`, error); + }); + } + + if (capabilities?.prompts) { + client + .listPrompts({}, requestOptions) + .then((prompts) => { + console.log(`Successfully listed ${prompts.prompts.length} prompts for per-session server: ${serverName}, session: ${sessionId}`); + serverInfo.prompts = prompts.prompts.map((prompt) => ({ + name: `${serverName}${getNameSeparator()}${prompt.name}`, + title: prompt.title, + description: prompt.description, + arguments: prompt.arguments, + })); + }) + .catch((error) => { + console.error(`Failed to list prompts for per-session server ${serverName}, session ${sessionId}:`, error); + }); + } + + serverInfo.status = 'connected'; + serverInfo.error = null; + }) + .catch((error) => { + console.error(`Failed to connect per-session client for server ${serverName}, session ${sessionId}:`, error); + serverInfo.status = 'disconnected'; + serverInfo.error = `Failed to connect: ${error.stack}`; + }); + + return serverInfo; +}; + +// Helper function to clean up per-session servers for a session +export const cleanupPerSessionServers = (sessionId: string): void => { + const keysToDelete: string[] = []; + + for (const key in perSessionServerInfos) { + if (key.startsWith(`${sessionId}:`)) { + const serverInfo = perSessionServerInfos[key]; + try { + if (serverInfo.client) { + serverInfo.client.close(); + } + if (serverInfo.transport) { + serverInfo.transport.close(); + } + if (serverInfo.keepAliveIntervalId) { + clearInterval(serverInfo.keepAliveIntervalId); + } + } catch (error) { + console.warn(`Error closing per-session server ${key}:`, error); + } + keysToDelete.push(key); + } + } + + keysToDelete.forEach(key => delete perSessionServerInfos[key]); + console.log(`Cleaned up ${keysToDelete.length} per-session servers for session ${sessionId}`); +}; + // Helper function to handle client.callTool with reconnection logic const callToolWithReconnect = async ( serverInfo: ServerInfo, @@ -625,6 +769,45 @@ export const getServerByName = (name: string): ServerInfo | undefined => { return serverInfos.find((serverInfo) => serverInfo.name === name); }; +// Get server by name with session support (for per-session servers) +const getServerByNameWithSession = async (name: string, sessionId?: string): Promise => { + // First check if this server is configured for per-session instances + const serverConfig = await serverDao.findById(name); + + if (serverConfig?.perSession && sessionId) { + // Try to get or create per-session server + const key = `${sessionId}:${name}`; + if (perSessionServerInfos[key]) { + return perSessionServerInfos[key]; + } + // Create new per-session server instance + return await getOrCreatePerSessionServer(sessionId, name, serverConfig); + } + + // Fall back to shared server + return serverInfos.find((serverInfo) => serverInfo.name === name && !serverInfo.sessionId); +}; + +// Get server by tool name with session support (for per-session servers) +const getServerByToolWithSession = async (toolName: string, sessionId?: string): Promise => { + // First try to find in per-session servers if sessionId is provided + if (sessionId) { + for (const key in perSessionServerInfos) { + if (key.startsWith(`${sessionId}:`)) { + const serverInfo = perSessionServerInfos[key]; + if (serverInfo.tools.some((tool) => tool.name === toolName)) { + return serverInfo; + } + } + } + } + + // Fall back to shared servers + return serverInfos.find((serverInfo) => + !serverInfo.sessionId && serverInfo.tools.some((tool) => tool.name === toolName) + ); +}; + // Filter tools by server configuration const filterToolsByConfig = async (serverName: string, tools: Tool[]): Promise => { const serverConfig = await serverDao.findById(serverName); @@ -640,8 +823,8 @@ const filterToolsByConfig = async (serverName: string, tools: Tool[]): Promise { +// Get server by tool name (legacy - use getServerByToolWithSession instead) +const _getServerByTool = (toolName: string): ServerInfo | undefined => { return serverInfos.find((serverInfo) => serverInfo.tools.some((tool) => tool.name === toolName)); }; @@ -826,15 +1009,35 @@ Available servers: ${serversList}`; }; } - const allServerInfos = getDataService() + // Get shared servers + let allServerInfos = getDataService() .filterData(serverInfos) .filter((serverInfo) => { + if (serverInfo.enabled === false) return false; + if (serverInfo.sessionId) return false; // Exclude per-session servers from shared list + if (!group) return true; + const serversInGroup = getServersInGroup(group); + if (!serversInGroup || serversInGroup.length === 0) return serverInfo.name === group; + return serversInGroup.includes(serverInfo.name); + }); + + // Add per-session servers for this session + if (sessionId) { + const sessionServers = Object.values(perSessionServerInfos).filter( + (serverInfo) => serverInfo.sessionId === sessionId && serverInfo.status === 'connected' + ); + + // Filter session servers by group if applicable + const filteredSessionServers = sessionServers.filter((serverInfo) => { if (serverInfo.enabled === false) return false; if (!group) return true; const serversInGroup = getServersInGroup(group); if (!serversInGroup || serversInGroup.length === 0) return serverInfo.name === group; return serversInGroup.includes(serverInfo.name); }); + + allServerInfos = [...allServerInfos, ...filteredSessionServers]; + } const allTools = []; for (const serverInfo of allServerInfos) { @@ -998,17 +1201,13 @@ export const handleCallToolRequest = async (request: any, extra: any) => { } const { arguments: toolArgs = {} } = request.params.arguments || {}; + const sessionId = extra?.sessionId; let targetServerInfo: ServerInfo | undefined; if (extra && extra.server) { - targetServerInfo = getServerByName(extra.server); + targetServerInfo = await getServerByNameWithSession(extra.server, sessionId); } else { - // Find the first server that has this tool - targetServerInfo = serverInfos.find( - (serverInfo) => - serverInfo.status === 'connected' && - serverInfo.enabled !== false && - serverInfo.tools.some((tool) => tool.name === toolName), - ); + // Find the first server that has this tool (session-aware) + targetServerInfo = await getServerByToolWithSession(toolName, sessionId); } if (!targetServerInfo) { @@ -1114,7 +1313,8 @@ export const handleCallToolRequest = async (request: any, extra: any) => { } // Regular tool handling - const serverInfo = getServerByTool(request.params.name); + const sessionId = extra?.sessionId; + const serverInfo = await getServerByToolWithSession(request.params.name, sessionId); if (!serverInfo) { throw new Error(`Server not found: ${request.params.name}`); } diff --git a/src/types/index.ts b/src/types/index.ts index f21bbb0..2115d56 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -178,6 +178,7 @@ export interface ServerConfig { enabled?: boolean; // Flag to enable/disable the server owner?: string; // Owner of the server, defaults to 'admin' user keepAliveInterval?: number; // Keep-alive ping interval in milliseconds (default: 60000ms for SSE servers) + perSession?: boolean; // If true, creates a separate server instance for each session (useful for stateful servers like playwright) tools?: Record; // Tool-specific configurations with enable/disable state and custom descriptions prompts?: Record; // Prompt-specific configurations with enable/disable state and custom descriptions options?: Partial>; // MCP request options configuration @@ -239,6 +240,7 @@ export interface ServerInfo { enabled?: boolean; // Flag to indicate if the server is enabled keepAliveIntervalId?: NodeJS.Timeout; // Timer ID for keep-alive ping interval config?: ServerConfig; // Reference to the original server configuration for OpenAPI passthrough headers + sessionId?: string; // Session ID for per-session server instances (undefined for shared servers) } // Details about a tool available on the server diff --git a/tests/services/perSessionServers.test.ts b/tests/services/perSessionServers.test.ts new file mode 100644 index 0000000..c1fd532 --- /dev/null +++ b/tests/services/perSessionServers.test.ts @@ -0,0 +1,112 @@ +import { + getOrCreatePerSessionServer, + cleanupPerSessionServers, + handleCallToolRequest, +} from '../../src/services/mcpService'; +import { ServerConfig } from '../../src/types'; + +// Mock the serverDao +jest.mock('../../src/dao/index.js', () => ({ + getServerDao: () => ({ + findById: jest.fn((name: string) => { + if (name === 'playwright') { + return Promise.resolve({ + name: 'playwright', + command: 'npx', + args: ['@playwright/mcp@latest', '--headless', '--isolated'], + perSession: true, + enabled: true, + }); + } + return Promise.resolve(null); + }), + findAll: jest.fn(() => Promise.resolve([])), + }), +})); + +// Mock the Client and Transport classes +jest.mock('@modelcontextprotocol/sdk/client/index.js', () => ({ + Client: jest.fn().mockImplementation(() => ({ + connect: jest.fn(() => Promise.resolve()), + close: jest.fn(), + listTools: jest.fn(() => Promise.resolve({ tools: [] })), + listPrompts: jest.fn(() => Promise.resolve({ prompts: [] })), + getServerCapabilities: jest.fn(() => ({ tools: true, prompts: true })), + callTool: jest.fn((params) => Promise.resolve({ content: [{ type: 'text', text: `Tool ${params.name} called` }] })), + })), +})); + +jest.mock('@modelcontextprotocol/sdk/client/stdio.js', () => ({ + StdioClientTransport: jest.fn().mockImplementation(() => ({ + close: jest.fn(), + stderr: { + on: jest.fn(), + }, + })), +})); + +describe('Per-Session Server Instances', () => { + afterEach(() => { + // Clean up any created sessions + cleanupPerSessionServers('session1'); + cleanupPerSessionServers('session2'); + }); + + it('should create separate server instances for different sessions', async () => { + const config: ServerConfig = { + command: 'npx', + args: ['@playwright/mcp@latest', '--headless', '--isolated'], + perSession: true, + }; + + // Create server for session1 + const server1 = await getOrCreatePerSessionServer('session1', 'playwright', config); + expect(server1).toBeDefined(); + expect(server1.sessionId).toBe('session1'); + + // Create server for session2 + const server2 = await getOrCreatePerSessionServer('session2', 'playwright', config); + expect(server2).toBeDefined(); + expect(server2.sessionId).toBe('session2'); + + // They should be different instances + expect(server1).not.toBe(server2); + }); + + it('should reuse existing per-session server for the same session', async () => { + const config: ServerConfig = { + command: 'npx', + args: ['@playwright/mcp@latest', '--headless', '--isolated'], + perSession: true, + }; + + // Create server for session1 + const server1 = await getOrCreatePerSessionServer('session1', 'playwright', config); + + // Request the same server again + const server2 = await getOrCreatePerSessionServer('session1', 'playwright', config); + + // Should be the same instance + expect(server1).toBe(server2); + }); + + it('should clean up per-session servers when session ends', async () => { + const config: ServerConfig = { + command: 'npx', + args: ['@playwright/mcp@latest', '--headless', '--isolated'], + perSession: true, + }; + + // Create server for session1 + const server1 = await getOrCreatePerSessionServer('session1', 'playwright', config); + expect(server1).toBeDefined(); + + // Clean up session1 + cleanupPerSessionServers('session1'); + + // Create again should create a new instance (not the same object) + const server2 = await getOrCreatePerSessionServer('session1', 'playwright', config); + expect(server2).toBeDefined(); + expect(server2).not.toBe(server1); + }); +});