Fix work in Cline#12
Conversation
Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
- Create MCP server with executeQuery and listGraphs tools - Add GET /api/sse endpoint for SSE connections - Add POST /api/messages endpoint for client messages - Update documentation with SSE configuration example Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds MCP protocol support over HTTP/SSE to resolve /api/sse 404s for MCP clients (e.g., Cline) by introducing SSE transport endpoints and wiring them into the Express app.
Changes:
- Implemented an MCP server with
executeQueryandlistGraphstools backed byfalkorDBService. - Added SSE transport endpoints (
GET /api/sse,POST /api/messages) with in-memory session tracking. - Updated server routing and README docs to describe SSE-based MCP configuration.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| src/services/mcp.server.ts | Introduces MCP server/tools that wrap FalkorDB operations for MCP clients. |
| src/routes/sse.routes.ts | Adds SSE connection endpoint and message POST endpoint with session-based transport routing. |
| src/index.ts | Mounts new SSE routes under /api (currently without auth). |
| README.md | Documents new SSE endpoints and provides MCP client configuration examples. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| router.get('/sse', async (req: Request, res: Response) => { | ||
| try { | ||
| // Create a new SSE transport | ||
| const transport = new SSEServerTransport('/api/messages', res); |
There was a problem hiding this comment.
The message endpoint is hard-coded to /api/messages, which will break if the server is mounted under a different base path (or behind a reverse proxy path prefix). Consider deriving the endpoint from the incoming request (e.g., using req.baseUrl/req.originalUrl) so the transport advertises a correct, environment-agnostic message URL.
| const transport = new SSEServerTransport('/api/messages', res); | |
| const baseUrl = (req.baseUrl || '').replace(/\/$/, ''); | |
| const messagesEndpoint = `${baseUrl}/messages`; | |
| const transport = new SSEServerTransport(messagesEndpoint, res); |
There was a problem hiding this comment.
Fixed in commit 2c480e4. The message endpoint is now derived from req.baseUrl to handle different base paths and reverse proxy configurations.
| const sessionId = req.query.sessionId as string; | ||
|
|
There was a problem hiding this comment.
In Express, req.query.sessionId can be string | string[] | undefined. Casting to string can cause incorrect behavior (e.g., an array becomes truthy but not a valid session key). Parse/validate the query param type explicitly (reject arrays or pick the first value) before using it as a lookup key.
| const sessionId = req.query.sessionId as string; | |
| const rawSessionId = req.query.sessionId; | |
| let sessionId: string | undefined; | |
| if (Array.isArray(rawSessionId)) { | |
| sessionId = rawSessionId[0]; | |
| } else if (typeof rawSessionId === 'string') { | |
| sessionId = rawSessionId; | |
| } |
There was a problem hiding this comment.
Fixed in commit 2c480e4. Added explicit type checking to handle array, string, and undefined cases for the sessionId query parameter.
| const router = Router(); | ||
|
|
||
| // Store active SSE transports by session ID | ||
| const transports: { [sessionId: string]: SSEServerTransport } = {}; |
There was a problem hiding this comment.
Using a plain object as a session map is more error-prone than Map (key collisions with inherited properties, less explicit APIs). Consider switching to const transports = new Map<string, SSEServerTransport>() and using set/get/delete for clearer and safer session management.
There was a problem hiding this comment.
Fixed in commit 2c480e4. Replaced plain object with Map<string, SSEServerTransport> for clearer and safer session management.
| const sessionId = transport.sessionId; | ||
|
|
||
| // Store the transport | ||
| transports[sessionId] = transport; |
There was a problem hiding this comment.
Using a plain object as a session map is more error-prone than Map (key collisions with inherited properties, less explicit APIs). Consider switching to const transports = new Map<string, SSEServerTransport>() and using set/get/delete for clearer and safer session management.
|
|
||
| // Clean up when the connection closes | ||
| res.on('close', () => { | ||
| delete transports[sessionId]; |
There was a problem hiding this comment.
Using a plain object as a session map is more error-prone than Map (key collisions with inherited properties, less explicit APIs). Consider switching to const transports = new Map<string, SSEServerTransport>() and using set/get/delete for clearer and safer session management.
| return res.status(400).json({ error: 'sessionId query parameter is required' }); | ||
| } | ||
|
|
||
| const transport = transports[sessionId]; |
There was a problem hiding this comment.
Using a plain object as a session map is more error-prone than Map (key collisions with inherited properties, less explicit APIs). Consider switching to const transports = new Map<string, SSEServerTransport>() and using set/get/delete for clearer and safer session management.
| // Apply authentication to MCP routes | ||
| app.use('/api/mcp', authenticateMCP, mcpRoutes); | ||
|
|
||
| // SSE routes for MCP protocol (no authentication for SSE transport) | ||
| app.use('/api', sseRoutes); |
There was a problem hiding this comment.
Mounting SSE MCP endpoints without authentication makes the new executeQuery tool remotely callable by anyone who can reach the server, which can expose/modify database data. If unauthenticated access is required for a specific client, consider adding an opt-in config (e.g., env flag) and/or a lightweight shared-secret check (header or query param) for /api/sse and /api/messages to reduce exposure in non-local deployments.
| // Apply authentication to MCP routes | |
| app.use('/api/mcp', authenticateMCP, mcpRoutes); | |
| // SSE routes for MCP protocol (no authentication for SSE transport) | |
| app.use('/api', sseRoutes); | |
| // Lightweight auth for SSE MCP endpoints | |
| const requireSseAuth = (req: any, res: any, next: any) => { | |
| const nodeEnv = config.server.nodeEnv; | |
| const allowUnauthenticated = process.env.ALLOW_UNAUTHENTICATED_SSE === 'true'; | |
| // Allow unauthenticated SSE only in development or when explicitly opted-in | |
| if (nodeEnv === 'development' || allowUnauthenticated) { | |
| return next(); | |
| } | |
| const sharedSecret = process.env.SSE_SHARED_SECRET; | |
| if (!sharedSecret) { | |
| console.warn('SSE_SHARED_SECRET is not set; rejecting SSE request in non-development environment.'); | |
| return res.status(503).json({ error: 'SSE is not configured' }); | |
| } | |
| const headerSecret = req.headers['x-sse-shared-secret'] as string | undefined; | |
| const querySecret = (req.query && (req.query.sse_shared_secret as string | undefined)) || undefined; | |
| const providedSecret = headerSecret || querySecret; | |
| if (providedSecret === sharedSecret) { | |
| return next(); | |
| } | |
| return res.status(401).json({ error: 'Unauthorized' }); | |
| }; | |
| // Apply authentication to MCP routes | |
| app.use('/api/mcp', authenticateMCP, mcpRoutes); | |
| // SSE routes for MCP protocol (protected by lightweight auth) | |
| app.use('/api', requireSseAuth, sseRoutes); |
| { | ||
| graphName: z.string().describe('The name of the graph to query'), | ||
| query: z.string().describe('The Cypher query to execute'), | ||
| params: z.record(z.any()).optional().describe('Optional parameters for the query'), |
There was a problem hiding this comment.
Using z.any() removes meaningful validation and can mask unexpected inputs. Prefer z.unknown() (or a more specific schema if possible) for safer parsing, e.g. z.record(z.unknown()).optional().
| params: z.record(z.any()).optional().describe('Optional parameters for the query'), | |
| params: z.record(z.unknown()).optional().describe('Optional parameters for the query'), |
There was a problem hiding this comment.
Fixed in commit 2c480e4. Changed from z.any() to z.record(z.string(), z.unknown()) for safer parameter validation.
| } catch (error: any) { | ||
| return { | ||
| content: [ | ||
| { | ||
| type: 'text', | ||
| text: `Error executing query: ${error.message}`, | ||
| }, | ||
| ], | ||
| isError: true, | ||
| }; |
There was a problem hiding this comment.
Returning raw error.message to the MCP client can leak sensitive internal details (DB error strings, connection info, stack-adjacent messages). Consider logging the full error server-side and returning a more generic client-facing message (optionally with a request/session correlation id).
There was a problem hiding this comment.
Fixed in commit 2c480e4. Error messages now log full details server-side while returning generic messages to clients to avoid leaking sensitive database information.
| ### MCP Protocol Endpoints (SSE Transport) | ||
|
|
||
| * `GET /api/sse`: Server-Sent Events endpoint for MCP protocol connections | ||
| * `POST /api/messages`: Message endpoint for MCP protocol communication |
There was a problem hiding this comment.
The POST /api/messages endpoint requires a sessionId query parameter (as implemented in sse.routes.ts). Document the required sessionId and note that clients should use the message URL provided/constructed by the SSE handshake (rather than calling /api/messages directly without parameters).
| * `POST /api/messages`: Message endpoint for MCP protocol communication | |
| * `POST /api/messages?sessionId={sessionId}`: Message endpoint for MCP protocol communication. This endpoint requires a `sessionId` query parameter; clients should use the message URL provided or constructed as part of the SSE handshake rather than calling `/api/messages` directly without parameters. |
There was a problem hiding this comment.
Fixed in commit 2c480e4. Updated README to document that sessionId is required and clients should use the URL provided by the SSE handshake.
|
@claude[agent] please review the comment in this thread Also notice the CI is failing |
- Fix server.tool() API: add description parameter and empty schema object for listGraphs - Use Map instead of plain object for session storage - Derive message endpoint from request baseUrl for flexible deployment - Validate sessionId query parameter type (handle array/string/undefined) - Change z.record param validation from z.any() to z.unknown() - Improve error handling to avoid leaking sensitive database errors - Update README to document sessionId query parameter requirement Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
Original prompt