fix(server): restore security guards and error handling

Address code review feedback on the server-mode PR: Critical fixes: - Restore CORS whitelist (localhost + gitnexus.vercel.app only) - Bind to 127.0.0.1 by default; add --host CLI flag for opt-in remote access - Restore path traversal guard on /api/file (resolve + startsWith check) - Restore try/catch on all route handlers + global error middleware - Restore SIGINT/SIGTERM graceful shutdown handlers Bug fixes: - Add mutex to core initKuzu to prevent race conditions on concurrent DB switches (two requests for different repos no longer corrupt state) - Track ftsLoaded flag and reset on DB switch / close so FTS extension is reloaded for each new database connection - Restore input validation on /api/query (cypher required) and /api/search (query required) Improvements: - Add TTL-based cleanup for orphaned MCP sessions (30min idle eviction) to prevent memory leaks from network drops that skip onclose Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2026-02-26 12:42:00 +01:00
parent 7ee2dd1087
commit f047a84d82
5 changed files with 202 additions and 71 deletions
@@ -34,6 +34,7 @@ program
  .command('serve')
  .description('Start local HTTP server for web UI connection')
  .option('-p, --port <port>', 'Port number', '4747')
+  .option('--host <host>', 'Bind address (default: 127.0.0.1, use 0.0.0.0 for remote access)')
  .action(serveCommand);

 program
@@ -1,7 +1,7 @@
 import { createServer } from '../server/api.js';

-export const serveCommand = async (options?: { port?: string }) => {
+export const serveCommand = async (options?: { port?: string; host?: string }) => {
  const port = Number(options?.port ?? 4747);
-  await createServer(port);
+  const host = options?.host ?? '127.0.0.1';
+  await createServer(port, host);
 };
-
@@ -14,13 +14,32 @@ import { generateAllCSVs } from './csv-generator.js';
 let db: kuzu.Database | null = null;
 let conn: kuzu.Connection | null = null;
 let currentDbPath: string | null = null;
+let ftsLoaded = false;
+
+// Mutex: prevents concurrent initKuzu calls from racing on module-level globals.
+// Two simultaneous requests for different repos would otherwise close each other's connections.
+let initLock: Promise<{ db: kuzu.Database | null; conn: kuzu.Connection | null }> | null = null;

 const normalizeCopyPath = (filePath: string): string => filePath.replace(/\\/g, '/');

 export const initKuzu = async (dbPath: string) => {
-  // If already connected to the SAME database, reuse
+  // Fast path: already connected to this database
  if (conn && currentDbPath === dbPath) return { db, conn };

+  // Serialize concurrent callers through the lock
+  if (initLock) await initLock;
+  // Re-check after awaiting — another caller may have opened what we need
+  if (conn && currentDbPath === dbPath) return { db, conn };
+
+  initLock = doInitKuzu(dbPath);
+  try {
+    return await initLock;
+  } finally {
+    initLock = null;
+  }
+};
+
+const doInitKuzu = async (dbPath: string) => {
  // Different database requested — close the old one first
  if (conn || db) {
    try { if (conn) await conn.close(); } catch {}
@@ -28,6 +47,7 @@ export const initKuzu = async (dbPath: string) => {
    conn = null;
    db = null;
    currentDbPath = null;
+    ftsLoaded = false;
  }

  // kuzu v0.11 stores the database as a single file (not a directory).
@@ -538,6 +558,7 @@ export const closeKuzu = async (): Promise<void> => {
    db = null;
  }
  currentDbPath = null;
+  ftsLoaded = false;
 };

 export const isKuzuReady = (): boolean => conn !== null && db !== null;
@@ -629,11 +650,14 @@ export const loadFTSExtension = async (): Promise<void> => {
  if (!conn) {
    throw new Error('KuzuDB not initialized. Call initKuzu first.');
  }
+  if (ftsLoaded) return;
  try {
    await conn.query('INSTALL fts');
    await conn.query('LOAD EXTENSION fts');
+    ftsLoaded = true;
  } catch {
    // Extension may already be loaded
+    ftsLoaded = true;
  }
 };

@@ -3,6 +3,9 @@
 *
 * REST API for browser-based clients to query the local .gitnexus/ index.
 * Also hosts the MCP server over StreamableHTTP for remote AI tool access.
+ *
+ * Security: binds to 127.0.0.1 by default (use --host to override).
+ * CORS is restricted to localhost and the deployed site.
 */

 import express from 'express';
@@ -10,7 +13,7 @@ import cors from 'cors';
 import path from 'path';
 import fs from 'fs/promises';
 import { findRepo, loadMeta, listRegisteredRepos } from '../storage/repo-manager.js';
-import { initKuzu, executeQuery } from '../core/kuzu/kuzu-adapter.js';
+import { initKuzu, executeQuery, closeKuzu } from '../core/kuzu/kuzu-adapter.js';
 import { NODE_TABLES } from '../core/kuzu/schema.js';
 import { GraphNode, GraphRelationship } from '../core/graph/types.js';
 import { searchFTSFromKuzu } from '../core/search/bm25-index.js';
@@ -83,9 +86,25 @@ const buildGraph = async (): Promise<{ nodes: GraphNode[]; relationships: GraphR
  return { nodes, relationships };
 };

-export const createServer = async (port: number) => {
+export const createServer = async (port: number, host: string = '127.0.0.1') => {
  const app = express();
-  app.use(cors());
+
+  // CORS: only allow localhost origins and the deployed site.
+  // Non-browser requests (curl, server-to-server) have no origin and are allowed.
+  app.use(cors({
+    origin: (origin, callback) => {
+      if (
+        !origin
+        || origin.startsWith('http://localhost:')
+        || origin.startsWith('http://127.0.0.1:')
+        || origin === 'https://gitnexus.vercel.app'
+      ) {
+        callback(null, true);
+      } else {
+        callback(new Error('Not allowed by CORS'));
+      }
+    }
+  }));
  app.use(express.json({ limit: '10mb' }));

  // Initialize MCP backend (multi-repo, shared across all MCP sessions)
@@ -103,97 +122,160 @@ export const createServer = async (port: number) => {

  // List all registered repos
  app.get('/api/repos', async (_req, res) => {
-    const repos = await listRegisteredRepos();
-    res.json(repos.map(r => ({
-      name: r.name, path: r.path, indexedAt: r.indexedAt,
-      lastCommit: r.lastCommit, stats: r.stats,
-    })));
+    try {
+      const repos = await listRegisteredRepos();
+      res.json(repos.map(r => ({
+        name: r.name, path: r.path, indexedAt: r.indexedAt,
+        lastCommit: r.lastCommit, stats: r.stats,
+      })));
+    } catch (err: any) {
+      res.status(500).json({ error: err.message || 'Failed to list repos' });
+    }
  });

  // Get repo info
  app.get('/api/repo', async (req, res) => {
-    const entry = await resolveRepo(req.query.repo as string | undefined);
-    if (!entry) {
-      res.status(404).json({ error: 'Repository not found. Run: gitnexus analyze' });
-      return;
+    try {
+      const entry = await resolveRepo(req.query.repo as string | undefined);
+      if (!entry) {
+        res.status(404).json({ error: 'Repository not found. Run: gitnexus analyze' });
+        return;
+      }
+      const meta = await loadMeta(entry.storagePath);
+      res.json({
+        name: entry.name,
+        repoPath: entry.path,
+        indexedAt: meta?.indexedAt ?? entry.indexedAt,
+        stats: meta?.stats ?? entry.stats ?? {},
+      });
+    } catch (err: any) {
+      res.status(500).json({ error: err.message || 'Failed to get repo info' });
    }
-    const meta = await loadMeta(entry.storagePath);
-    res.json({
-      name: entry.name,
-      repoPath: entry.path,
-      indexedAt: meta?.indexedAt ?? entry.indexedAt,
-      stats: meta?.stats ?? entry.stats ?? {},
-    });
  });

  // Get full graph
  app.get('/api/graph', async (req, res) => {
-    const entry = await resolveRepo(req.query.repo as string | undefined);
-    if (!entry) {
-      res.status(404).json({ error: 'Repository not found' });
-      return;
+    try {
+      const entry = await resolveRepo(req.query.repo as string | undefined);
+      if (!entry) {
+        res.status(404).json({ error: 'Repository not found' });
+        return;
+      }
+      const kuzuPath = path.join(entry.storagePath, 'kuzu');
+      await initKuzu(kuzuPath);
+      const graph = await buildGraph();
+      res.json(graph);
+    } catch (err: any) {
+      res.status(500).json({ error: err.message || 'Failed to build graph' });
    }
-    const kuzuPath = path.join(entry.storagePath, 'kuzu');
-    await initKuzu(kuzuPath);
-    const graph = await buildGraph();
-    res.json(graph);
  });

  // Execute Cypher query
  app.post('/api/query', async (req, res) => {
-    const entry = await resolveRepo(req.query.repo as string | undefined);
-    if (!entry) {
-      res.status(404).json({ error: 'Repository not found' });
-      return;
+    try {
+      const cypher = req.body.cypher as string;
+      if (!cypher) {
+        res.status(400).json({ error: 'Missing "cypher" in request body' });
+        return;
+      }
+
+      const entry = await resolveRepo(req.query.repo as string | undefined);
+      if (!entry) {
+        res.status(404).json({ error: 'Repository not found' });
+        return;
+      }
+      const kuzuPath = path.join(entry.storagePath, 'kuzu');
+      await initKuzu(kuzuPath);
+      const result = await executeQuery(cypher);
+      res.json({ result });
+    } catch (err: any) {
+      res.status(500).json({ error: err.message || 'Query failed' });
    }
-    const kuzuPath = path.join(entry.storagePath, 'kuzu');
-    await initKuzu(kuzuPath);
-    const result = await executeQuery(req.body.cypher);
-    res.json({ result });
  });

  // Search
  app.post('/api/search', async (req, res) => {
-    const entry = await resolveRepo(req.query.repo as string | undefined);
-    if (!entry) {
-      res.status(404).json({ error: 'Repository not found' });
-      return;
-    }
-    const kuzuPath = path.join(entry.storagePath, 'kuzu');
-    await initKuzu(kuzuPath);
+    try {
+      const query = (req.body.query ?? '').trim();
+      if (!query) {
+        res.status(400).json({ error: 'Missing "query" in request body' });
+        return;
+      }

-    const query = req.body.query ?? '';
-    const limit = req.body.limit ?? 10;
+      const entry = await resolveRepo(req.query.repo as string | undefined);
+      if (!entry) {
+        res.status(404).json({ error: 'Repository not found' });
+        return;
+      }
+      const kuzuPath = path.join(entry.storagePath, 'kuzu');
+      await initKuzu(kuzuPath);

-    if (isEmbedderReady()) {
-      const results = await hybridSearch(query, limit, executeQuery, semanticSearch);
+      const limit = req.body.limit ?? 10;
+
+      if (isEmbedderReady()) {
+        const results = await hybridSearch(query, limit, executeQuery, semanticSearch);
+        res.json({ results });
+        return;
+      }
+
+      // FTS-only fallback when embeddings aren't loaded
+      const results = await searchFTSFromKuzu(query, limit);
      res.json({ results });
-      return;
+    } catch (err: any) {
+      res.status(500).json({ error: err.message || 'Search failed' });
    }
-
-    // FTS-only fallback when embeddings aren't loaded
-    const results = await searchFTSFromKuzu(query, limit);
-    res.json({ results });
  });

-  // Read file
+  // Read file — with path traversal guard
  app.get('/api/file', async (req, res) => {
-    const entry = await resolveRepo(req.query.repo as string | undefined);
-    if (!entry) {
-      res.status(404).json({ error: 'Repository not found' });
-      return;
+    try {
+      const entry = await resolveRepo(req.query.repo as string | undefined);
+      if (!entry) {
+        res.status(404).json({ error: 'Repository not found' });
+        return;
+      }
+      const filePath = req.query.path as string;
+      if (!filePath) {
+        res.status(400).json({ error: 'Missing path' });
+        return;
+      }
+
+      // Prevent path traversal — resolve and verify the path stays within the repo root
+      const repoRoot = path.resolve(entry.path);
+      const fullPath = path.resolve(repoRoot, filePath);
+      if (!fullPath.startsWith(repoRoot + path.sep) && fullPath !== repoRoot) {
+        res.status(403).json({ error: 'Path traversal denied' });
+        return;
+      }
+
+      const content = await fs.readFile(fullPath, 'utf-8');
+      res.json({ content });
+    } catch (err: any) {
+      if (err.code === 'ENOENT') {
+        res.status(404).json({ error: 'File not found' });
+      } else {
+        res.status(500).json({ error: err.message || 'Failed to read file' });
+      }
    }
-    const filePath = req.query.path as string;
-    if (!filePath) {
-      res.status(400).json({ error: 'Missing path' });
-      return;
-    }
-    const fullPath = path.join(entry.path, filePath);
-    const content = await fs.readFile(fullPath, 'utf-8');
-    res.json({ content });
  });

-  app.listen(port, () => {
-    console.log(`GitNexus server running on http://localhost:${port}`);
+  // Global error handler — catch anything the route handlers miss
+  app.use((err: any, _req: express.Request, res: express.Response, _next: express.NextFunction) => {
+    console.error('Unhandled error:', err);
+    res.status(500).json({ error: 'Internal server error' });
  });
+
+  const server = app.listen(port, host, () => {
+    console.log(`GitNexus server running on http://${host}:${port}`);
+  });
+
+  // Graceful shutdown — close Express + KuzuDB cleanly
+  const shutdown = async () => {
+    server.close();
+    await closeKuzu();
+    await backend.disconnect();
+    process.exit(0);
+  };
+  process.once('SIGINT', shutdown);
+  process.once('SIGTERM', shutdown);
 };
@@ -4,6 +4,9 @@
 * Mounts the GitNexus MCP server on Express using StreamableHTTP transport.
 * Each connecting client gets its own stateful session; the LocalBackend
 * is shared across all sessions (thread-safe — lazy KuzuDB per repo).
+ *
+ * Sessions are cleaned up on explicit close or after SESSION_TTL_MS of inactivity
+ * (guards against network drops that never trigger onclose).
 */

 import type { Express, Request, Response } from 'express';
@@ -16,17 +19,38 @@ import { randomUUID } from 'crypto';
 interface MCPSession {
  server: Server;
  transport: StreamableHTTPServerTransport;
+  lastActivity: number;
 }

+/** Idle sessions are evicted after 30 minutes */
+const SESSION_TTL_MS = 30 * 60 * 1000;
+/** Cleanup sweep runs every 5 minutes */
+const CLEANUP_INTERVAL_MS = 5 * 60 * 1000;
+
 export function mountMCPEndpoints(app: Express, backend: LocalBackend): void {
  const sessions = new Map<string, MCPSession>();

+  // Periodic cleanup of idle sessions (guards against network drops)
+  const cleanupTimer = setInterval(() => {
+    const now = Date.now();
+    for (const [id, session] of sessions) {
+      if (now - session.lastActivity > SESSION_TTL_MS) {
+        try { session.server.close(); } catch {}
+        sessions.delete(id);
+      }
+    }
+  }, CLEANUP_INTERVAL_MS);
+  if (cleanupTimer && typeof cleanupTimer === 'object' && 'unref' in cleanupTimer) {
+    (cleanupTimer as NodeJS.Timeout).unref();
+  }
+
  app.all('/api/mcp', async (req: Request, res: Response) => {
    const sessionId = req.headers['mcp-session-id'] as string | undefined;

    if (sessionId && sessions.has(sessionId)) {
      // Existing session — delegate to its transport
      const session = sessions.get(sessionId)!;
+      session.lastActivity = Date.now();
      await session.transport.handleRequest(req, res, req.body);
    } else if (sessionId) {
      // Unknown/expired session ID — tell client to re-initialize (per MCP spec)
@@ -45,7 +69,7 @@ export function mountMCPEndpoints(app: Express, backend: LocalBackend): void {
      await transport.handleRequest(req, res, req.body);

      if (transport.sessionId) {
-        sessions.set(transport.sessionId, { server, transport });
+        sessions.set(transport.sessionId, { server, transport, lastActivity: Date.now() });
        transport.onclose = () => {
          sessions.delete(transport.sessionId!);
        };