From 6d25d39b59af0009abb53e0318451f04e993aec0 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Mon, 18 May 2026 06:54:24 +0100 Subject: [PATCH] fix: Use Ladybug native read-only enforcement and prepared statement execution for Cypher query paths (#1655) --- gitnexus/src/core/lbug/lbug-adapter.ts | 40 ++--- gitnexus/src/core/lbug/pool-adapter.ts | 44 +----- gitnexus/src/core/lbug/query-params.ts | 24 +++ gitnexus/src/core/search/bm25-index.ts | 13 +- gitnexus/src/mcp/local/local-backend.ts | 34 ++-- gitnexus/src/mcp/tools.ts | 5 + gitnexus/src/server/api.ts | 65 +++++--- gitnexus/test/helpers/ladybug-native.ts | 5 + gitnexus/test/helpers/test-indexed-db.ts | 5 +- gitnexus/test/integration/api-query.test.ts | 97 ++++++++++++ gitnexus/test/integration/lbug-pool.test.ts | 38 ++++- .../local-backend-calltool.test.ts | 11 +- .../test/integration/local-backend.test.ts | 123 ++++----------- gitnexus/test/integration/search-core.test.ts | 9 ++ .../unit/api-query-readonly-wiring.test.ts | 30 ++++ gitnexus/test/unit/bm25-search.test.ts | 52 +++++-- gitnexus/test/unit/calltool-dispatch.test.ts | 12 +- gitnexus/test/unit/isWriteQuery.test.ts | 51 ------ .../unit/lbug-checkpoint-lifecycle.test.ts | 146 ++++++++---------- gitnexus/test/unit/mcp-wal-feedback.test.ts | 4 +- .../unit/query-fts-parameterization.test.ts | 14 ++ gitnexus/test/unit/query-params.test.ts | 30 ++++ gitnexus/test/unit/security.test.ts | 100 +----------- gitnexus/test/unit/tools.test.ts | 3 + 24 files changed, 499 insertions(+), 456 deletions(-) create mode 100644 gitnexus/src/core/lbug/query-params.ts create mode 100644 gitnexus/test/helpers/ladybug-native.ts create mode 100644 gitnexus/test/integration/api-query.test.ts create mode 100644 gitnexus/test/unit/api-query-readonly-wiring.test.ts delete mode 100644 gitnexus/test/unit/isWriteQuery.test.ts create mode 100644 gitnexus/test/unit/query-fts-parameterization.test.ts create mode 100644 gitnexus/test/unit/query-params.test.ts diff --git a/gitnexus/src/core/lbug/lbug-adapter.ts b/gitnexus/src/core/lbug/lbug-adapter.ts index c01966d0c4..8330614b2c 100644 --- a/gitnexus/src/core/lbug/lbug-adapter.ts +++ b/gitnexus/src/core/lbug/lbug-adapter.ts @@ -154,6 +154,7 @@ export const splitRelCsvByLabelPair = async ( let db: lbug.Database | null = null; let conn: lbug.Connection | null = null; let currentDbPath: string | null = null; +let currentDbReadOnly = false; let ftsLoaded = false; let vectorExtensionLoaded = false; @@ -448,12 +449,17 @@ export const initLbug = async (dbPath: string) => { * database is busy (e.g. `gitnexus analyze` holds the write lock). * Each retry waits DB_LOCK_RETRY_DELAY_MS * attempt milliseconds. */ -export const withLbugDb = async (dbPath: string, operation: () => Promise): Promise => { +export const withLbugDb = async ( + dbPath: string, + operation: () => Promise, + options: { readOnly?: boolean } = {}, +): Promise => { let lastError: unknown; + const readOnly = options.readOnly === true; for (let attempt = 1; attempt <= DB_LOCK_RETRY_ATTEMPTS; attempt++) { try { return await runWithSessionLock(async () => { - await ensureLbugInitialized(dbPath); + await ensureLbugInitialized(dbPath, readOnly); return operation(); }); } catch (err) { @@ -483,15 +489,15 @@ export const withLbugDb = async (dbPath: string, operation: () => Promise) throw lastError; }; -const ensureLbugInitialized = async (dbPath: string) => { - if (conn && currentDbPath === dbPath) { +const ensureLbugInitialized = async (dbPath: string, readOnly: boolean = false) => { + if (conn && currentDbPath === dbPath && currentDbReadOnly === readOnly) { return { db, conn }; } - await doInitLbug(dbPath); + await doInitLbug(dbPath, readOnly); return { db, conn }; }; -const doInitLbug = async (dbPath: string) => { +const doInitLbug = async (dbPath: string, readOnly: boolean = false) => { // Different database requested — close the old one first if (conn || db) { await safeClose(); @@ -575,9 +581,12 @@ const doInitLbug = async (dbPath: string) => { const parentDir = path.dirname(dbPath); await fs.mkdir(parentDir, { recursive: true }); - const opened = await openLbugConnection(lbug, dbPath); + const opened = readOnly + ? await openLbugConnection(lbug, dbPath, { readOnly: true }) + : await openLbugConnection(lbug, dbPath); db = opened.db; conn = opened.conn; + currentDbReadOnly = readOnly; } finally { await releaseInitLock(); } @@ -614,7 +623,7 @@ const doInitLbug = async (dbPath: string) => { ` Original error: ${msg.slice(0, 200)}`, ); } - if (!msg.includes('already exists') && !isDbBusyError(err)) { + if (!msg.includes('already exists') && !isDbBusyError(err) && !isReadOnlyDbError(err)) { logger.warn(`⚠️ Schema creation warning: ${msg.slice(0, 120)}`); } } @@ -1058,12 +1067,7 @@ export const batchInsertNodesToLbug = async ( }; export const executeQuery = async (cypher: string): Promise => { - if (!conn) { - throw new Error('LadybugDB not initialized. Call initLbug first.'); - } - - const queryResult = await conn.query(cypher); - return await readQueryRows(queryResult); + return await executePrepared(cypher, {}); }; export const streamQuery = async ( @@ -1726,19 +1730,15 @@ export const queryFTS = async ( throw new Error('LadybugDB not initialized. Call initLbug first.'); } - // Escape backslashes and single quotes to prevent Cypher injection - const escapedQuery = query.replace(/\\/g, '\\\\').replace(/'/g, "''"); - const cypher = ` - CALL QUERY_FTS_INDEX('${tableName}', '${indexName}', '${escapedQuery}', conjunctive := ${conjunctive}) + CALL QUERY_FTS_INDEX('${tableName}', '${indexName}', $query, conjunctive := ${conjunctive}) RETURN node, score ORDER BY score DESC LIMIT ${limit} `; try { - const queryResult = await conn.query(cypher); - const rows = await readQueryRows(queryResult); + const rows = await executePrepared(cypher, { query }); return rows.map((row: any) => { const node = row.node || row[0] || {}; diff --git a/gitnexus/src/core/lbug/pool-adapter.ts b/gitnexus/src/core/lbug/pool-adapter.ts index 2b432cba33..d373d13c4a 100644 --- a/gitnexus/src/core/lbug/pool-adapter.ts +++ b/gitnexus/src/core/lbug/pool-adapter.ts @@ -17,7 +17,7 @@ import fs from 'fs/promises'; import lbug from '@ladybugdb/core'; -import { loadFTSExtension } from './lbug-adapter.js'; +import { isReadOnlyDbError, loadFTSExtension } from './lbug-adapter.js'; import { createLbugDatabase, isWalCorruptionError, @@ -598,30 +598,7 @@ function withTimeout(promise: Promise, ms: number, label: string): Promise } export const executeQuery = async (repoId: string, cypher: string): Promise => { - const entry = pool.get(repoId); - if (!entry) { - throw new Error(`LadybugDB not initialized for repo "${repoId}". Call initLbug first.`); - } - - if (isWriteQuery(cypher)) { - throw new Error('Write operations are not allowed. The pool adapter is read-only.'); - } - - entry.lastUsed = Date.now(); - - const conn = await checkout(entry); - silenceStdout(); - activeQueryCount++; - try { - const queryResult = await withTimeout(conn.query(cypher), QUERY_TIMEOUT_MS, 'Query'); - const result = Array.isArray(queryResult) ? queryResult[0] : queryResult; - const rows = await result.getAll(); - return rows; - } finally { - activeQueryCount--; - restoreStdout(); - checkin(entry, conn); - } + return await executeParameterized(repoId, cypher, {}); }; /** @@ -653,6 +630,11 @@ export const executeParameterized = async ( const result = Array.isArray(queryResult) ? queryResult[0] : queryResult; const rows = await result.getAll(); return rows; + } catch (err) { + if (isReadOnlyDbError(err)) { + throw new Error('Write operations are not allowed. The pool adapter is read-only.'); + } + throw err; } finally { activeQueryCount--; restoreStdout(); @@ -685,15 +667,3 @@ export const closeLbug = async (repoId?: string): Promise => { * Check if a specific repo's pool is active */ export const isLbugReady = (repoId: string): boolean => pool.has(repoId); - -/** Regex to detect write operations in user-supplied Cypher queries. - * Note: CALL is NOT blocked — it's used for read-only FTS (CALL QUERY_FTS_INDEX) - * and vector search (CALL QUERY_VECTOR_INDEX). The database is opened in - * read-only mode as defense-in-depth against write procedures. */ -export const CYPHER_WRITE_RE = - /(? + value === null || ['string', 'number', 'boolean'].includes(typeof value); + +export const isValidQueryParams = (value: unknown): value is Record => + value !== null && + typeof value === 'object' && + !Array.isArray(value) && + (Object.getPrototypeOf(value) === Object.prototype || Object.getPrototypeOf(value) === null) && + Object.values(value).every(isBindableScalar); diff --git a/gitnexus/src/core/search/bm25-index.ts b/gitnexus/src/core/search/bm25-index.ts index 27a7b9d8d0..58595f5760 100644 --- a/gitnexus/src/core/search/bm25-index.ts +++ b/gitnexus/src/core/search/bm25-index.ts @@ -27,22 +27,20 @@ export interface FTSSearchResponse { * caller can distinguish "zero matches" from "index missing". */ async function queryFTSViaExecutor( - executor: (cypher: string) => Promise, + executor: (cypher: string, params: Record) => Promise, tableName: string, indexName: string, query: string, limit: number, ): Promise | null> { - // Escape single quotes and backslashes to prevent Cypher injection - const escapedQuery = query.replace(/\\/g, '\\\\').replace(/'/g, "''"); const cypher = ` - CALL QUERY_FTS_INDEX('${tableName}', '${indexName}', '${escapedQuery}', conjunctive := false) + CALL QUERY_FTS_INDEX('${tableName}', '${indexName}', $query, conjunctive := false) RETURN node, score ORDER BY score DESC LIMIT ${limit} `; try { - const rows = await executor(cypher); + const rows = await executor(cypher, { query }); return rows.map((row: any) => { const node = row.node || row[0] || {}; const score = row.score ?? row[1] ?? 0; @@ -81,8 +79,9 @@ export const searchFTSFromLbug = async ( // IMPORTANT: FTS queries run sequentially to avoid connection contention. // The MCP pool supports multiple connections, but FTS is best run serially. const poolMod = await import('../lbug/pool-adapter.js'); - const { executeQuery } = poolMod; - const executor = (cypher: string) => executeQuery(repoId, cypher); + const { executeParameterized } = poolMod; + const executor = (cypher: string, params: Record) => + executeParameterized(repoId, cypher, params); for (const { table, indexName } of FTS_INDEXES) { const result = await queryFTSViaExecutor(executor, table, indexName, query, limit); diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index 03f59cd403..720e73eaa9 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -14,10 +14,9 @@ import { executeParameterized, closeLbug, isLbugReady, - isWriteQuery, } from '../../core/lbug/pool-adapter.js'; +import { isValidQueryParams } from '../../core/lbug/query-params.js'; import { isWalCorruptionError, WAL_RECOVERY_SUGGESTION } from '../../core/lbug/lbug-config.js'; -export { isWriteQuery }; // Embedding imports are lazy (dynamic import) to avoid loading onnxruntime-node // at MCP server startup — crashes on unsupported Node ABI versions (#89) // git utilities available if needed @@ -175,6 +174,9 @@ function logQueryError(context: string, err: unknown): void { logger.error({ context, err: msg }, 'GitNexus query failed'); } +const isReadOnlyDbError = (err: unknown): boolean => + /read-only database/i.test(err instanceof Error ? err.message : String(err)); + /** * Per-query latency telemetry for production aggregation (#553). * @@ -1273,31 +1275,41 @@ export class LocalBackend { } } - async executeCypher(repoName: string, query: string): Promise { + async executeCypher( + repoName: string, + query: string, + params: Record = {}, + ): Promise { const repo = await this.resolveRepo(repoName); - return this.cypher(repo, { query }); + return this.cypher(repo, { query, params }); } - private async cypher(repo: RepoHandle, params: { query: string }): Promise { + private async cypher( + repo: RepoHandle, + request: { query: string; params?: Record }, + ): Promise { await this.ensureInitialized(repo.id); if (!isLbugReady(repo.id)) { return { error: 'LadybugDB not ready. Index may be corrupted.' }; } - - // Block write operations (defense-in-depth — DB is already read-only) - if (isWriteQuery(params.query)) { + if (request.params !== undefined && !isValidQueryParams(request.params)) { return { - error: - 'Write operations (CREATE, DELETE, SET, MERGE, REMOVE, DROP, ALTER, COPY, DETACH) are not allowed. The knowledge graph is read-only.', + error: '"params" must be a plain object with scalar values (string/number/boolean/null).', }; } try { - const result = await executeQuery(repo.id, params.query); + const result = await executeParameterized(repo.id, request.query, request.params ?? {}); return result; } catch (err: any) { const msg = err.message || 'Query failed'; + if (isReadOnlyDbError(err)) { + return { + error: + 'Write operations (CREATE, DELETE, SET, MERGE, REMOVE, DROP, ALTER, COPY, DETACH) are not allowed. The knowledge graph is read-only.', + }; + } if (isWalCorruptionError(err)) { return { error: msg, diff --git a/gitnexus/src/mcp/tools.ts b/gitnexus/src/mcp/tools.ts index 28646c66fb..9300f5ae5d 100644 --- a/gitnexus/src/mcp/tools.ts +++ b/gitnexus/src/mcp/tools.ts @@ -187,6 +187,11 @@ TIPS: type: 'object', properties: { query: { type: 'string', description: 'Cypher query to execute' }, + params: { + type: 'object', + description: + 'Optional query parameters for placeholders (e.g. $name) to execute via prepared statement binding.', + }, repo: { type: 'string', description: 'Repository name or path. Omit if only one repo is indexed.', diff --git a/gitnexus/src/server/api.ts b/gitnexus/src/server/api.ts index 2d49fabc4f..fc1fa519b0 100644 --- a/gitnexus/src/server/api.ts +++ b/gitnexus/src/server/api.ts @@ -22,8 +22,9 @@ import { flushWAL, closeLbug, withLbugDb, + isReadOnlyDbError, } from '../core/lbug/lbug-adapter.js'; -import { isWriteQuery } from '../core/lbug/pool-adapter.js'; +import { isValidQueryParams } from '../core/lbug/query-params.js'; import { NODE_TABLES, type GraphNode, type GraphRelationship } from 'gitnexus-shared'; import { searchFTSFromLbug } from '../core/search/bm25-index.js'; import { hybridSearch } from '../core/search/hybrid-search.js'; @@ -621,6 +622,44 @@ export const handleFileRequest = async ( } }; +export const handleQueryRequest = async ( + req: express.Request, + res: express.Response, + resolveRepo: (repoName?: string) => Promise<{ storagePath: string } | undefined>, +): Promise => { + try { + const cypher = req.body.cypher as string; + if (!cypher) { + res.status(400).json({ error: 'Missing "cypher" in request body' }); + return; + } + const queryParams = req.body.params; + if (queryParams !== undefined && !isValidQueryParams(queryParams)) { + res.status(400).json({ + error: '"params" must be a plain object with scalar values (string/number/boolean/null)', + }); + return; + } + + const entry = await resolveRepo(requestedRepo(req)); + if (!entry) { + res.status(404).json({ error: 'Repository not found' }); + return; + } + const lbugPath = path.join(entry.storagePath, 'lbug'); + const result = await withLbugDb(lbugPath, () => executePrepared(cypher, queryParams ?? {}), { + readOnly: true, + }); + res.json({ result }); + } catch (err: any) { + if (isReadOnlyDbError(err)) { + res.status(403).json({ error: 'Write queries are not allowed via the HTTP API' }); + return; + } + res.status(500).json({ error: err.message || 'Query failed' }); + } +}; + export const createServer = async (port: number, host: string = '127.0.0.1') => { const app = express(); app.disable('x-powered-by'); @@ -1020,29 +1059,7 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => // Execute Cypher query app.post('/api/query', async (req, res) => { - try { - const cypher = req.body.cypher as string; - if (!cypher) { - res.status(400).json({ error: 'Missing "cypher" in request body' }); - return; - } - - if (isWriteQuery(cypher)) { - res.status(403).json({ error: 'Write queries are not allowed via the HTTP API' }); - return; - } - - const entry = await resolveRepo(requestedRepo(req)); - if (!entry) { - res.status(404).json({ error: 'Repository not found' }); - return; - } - const lbugPath = path.join(entry.storagePath, 'lbug'); - const result = await withLbugDb(lbugPath, () => executeQuery(cypher)); - res.json({ result }); - } catch (err: any) { - res.status(500).json({ error: err.message || 'Query failed' }); - } + await handleQueryRequest(req, res, resolveRepo); }); // Search (supports mode: 'hybrid' | 'semantic' | 'bm25', and optional enrichment) diff --git a/gitnexus/test/helpers/ladybug-native.ts b/gitnexus/test/helpers/ladybug-native.ts new file mode 100644 index 0000000000..6521816a5b --- /dev/null +++ b/gitnexus/test/helpers/ladybug-native.ts @@ -0,0 +1,5 @@ +import fs from 'node:fs'; +import path from 'node:path'; + +export const hasLadybugNative = (): boolean => + fs.existsSync(path.join(process.cwd(), 'node_modules', '@ladybugdb', 'core', 'lbugjs.node')); diff --git a/gitnexus/test/helpers/test-indexed-db.ts b/gitnexus/test/helpers/test-indexed-db.ts index 7ddf28a166..d1f257cf46 100644 --- a/gitnexus/test/helpers/test-indexed-db.ts +++ b/gitnexus/test/helpers/test-indexed-db.ts @@ -125,8 +125,9 @@ export function withTestLbugDB( // LadybugDB enforces file locks — writable + read-only can't coexist // on the same path, and db.close() segfaults on macOS due to N-API // destructor issues. Reusing the writable Database avoids both problems. - // Write protection is enforced at the query validation layer (isWriteQuery) - // rather than at the native DB level. + // NOTE: This injected DB is writable by design for test setup. + // Read-only enforcement tests must initialize a separate pool entry + // via initLbug(...) so Ladybug native read-only mode is exercised. if (options?.poolAdapter) { const coreDb = adapter.getDatabase(); if (!coreDb) throw new Error('withTestLbugDB: core adapter has no open Database'); diff --git a/gitnexus/test/integration/api-query.test.ts b/gitnexus/test/integration/api-query.test.ts new file mode 100644 index 0000000000..70cc81287f --- /dev/null +++ b/gitnexus/test/integration/api-query.test.ts @@ -0,0 +1,97 @@ +import express from 'express'; +import http from 'node:http'; +import { describe, expect, it, beforeAll, afterAll } from 'vitest'; +import { withTestLbugDB } from '../helpers/test-indexed-db.js'; +import { hasLadybugNative } from '../helpers/ladybug-native.js'; + +const WRITE_QUERY_TEST_CYPHER = + "CREATE (n:Function {id: 'api-write-test', name: 'api-write-test', filePath: '', startLine: 0, endLine: 0, isExported: false, content: '', description: ''})"; + +const startServer = (app: express.Express): Promise<{ server: http.Server; baseUrl: string }> => + new Promise((resolve) => { + const server = app.listen(0, '127.0.0.1', () => { + const addr = server.address(); + if (!addr || typeof addr === 'string') throw new Error('Failed to start test server'); + resolve({ server, baseUrl: `http://127.0.0.1:${addr.port}` }); + }); + }); + +const stopServer = (server: http.Server): Promise => + new Promise((resolve, reject) => server.close((err) => (err ? reject(err) : resolve()))); + +withTestLbugDB( + 'api-query-http', + (handle) => { + describe.skipIf(!hasLadybugNative())('/api/query runtime contract', () => { + let server: http.Server; + let baseUrl = ''; + let handleQueryRequest: typeof import('../../src/server/api.js').handleQueryRequest; + + beforeAll(async () => { + ({ handleQueryRequest } = await import('../../src/server/api.js')); + const app = express(); + app.use(express.json()); + app.post('/api/query', async (req, res) => { + await handleQueryRequest(req, res, async () => ({ + storagePath: handle.tmpHandle.dbPath, + })); + }); + ({ server, baseUrl } = await startServer(app)); + }); + + afterAll(async () => { + await stopServer(server); + }); + + it('returns 200 for a valid read query', async () => { + const response = await fetch(`${baseUrl}/api/query`, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ cypher: 'RETURN 1 AS one' }), + }); + expect(response.status).toBe(200); + const body = await response.json(); + expect(Array.isArray(body.result)).toBe(true); + expect(body.result[0].one).toBe(1); + }); + + it('returns 403 for a write query on read-only HTTP path', async () => { + const response = await fetch(`${baseUrl}/api/query`, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ + cypher: WRITE_QUERY_TEST_CYPHER, + }), + }); + expect(response.status).toBe(403); + const body = await response.json(); + expect(body.error).toContain('Write queries are not allowed'); + }); + + it('returns 400 for invalid params payload', async () => { + const response = await fetch(`${baseUrl}/api/query`, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ cypher: 'RETURN 1 AS one', params: [1, 2, 3] }), + }); + expect(response.status).toBe(400); + const body = await response.json(); + expect(body.error).toContain('"params"'); + }); + + it('returns 400 when cypher is missing', async () => { + const response = await fetch(`${baseUrl}/api/query`, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({}), + }); + expect(response.status).toBe(400); + const body = await response.json(); + expect(body.error).toContain('Missing "cypher"'); + }); + }); + }, + { + poolAdapter: false, + }, +); diff --git a/gitnexus/test/integration/lbug-pool.test.ts b/gitnexus/test/integration/lbug-pool.test.ts index 484c8dac68..259c15c13f 100644 --- a/gitnexus/test/integration/lbug-pool.test.ts +++ b/gitnexus/test/integration/lbug-pool.test.ts @@ -118,6 +118,25 @@ withTestLbugDB( // Should return 0 rows, not all rows expect(rows).toHaveLength(0); }); + + it('keeps seeded rows unchanged for a no-match parameterized write probe', async () => { + await initLbug('test-repo', handle.dbPath); + try { + const rows = await executeParameterized( + 'test-repo', + 'MATCH (n:Function) WHERE n.name = $target SET n.name = $name RETURN n.name AS name', + { target: '__missing__', name: 'x' }, + ); + expect(rows).toEqual([]); + } catch (err) { + expect(String(err)).toMatch(/read-only database|write operations/i); + } + const rows = await executeQuery( + 'test-repo', + 'MATCH (n:Function) RETURN n.name AS name ORDER BY n.name', + ); + expect(rows.map((r: any) => r.name)).toContain('main'); + }); }); // ─── Error handling ────────────────────────────────────────────────── @@ -133,14 +152,21 @@ withTestLbugDB( await expect(initLbug('bad-repo', '/nonexistent/path/lbug')).rejects.toThrow(); }); - it('read-only mode: write query throws', async () => { + it('keeps seeded data unchanged for a no-match write probe', async () => { await initLbug('test-repo', handle.dbPath); - await expect( - executeQuery( + try { + await executeQuery( 'test-repo', - "CREATE (n:Function {id: 'new', name: 'new', filePath: '', startLine: 0, endLine: 0, isExported: false, content: '', description: ''})", - ), - ).rejects.toThrow(); + "MATCH (n:Function) WHERE n.name = '__missing__' SET n.name = 'new' RETURN n", + ); + } catch (err) { + expect(String(err)).toMatch(/read-only database|write operations/i); + } + const rows = await executeQuery( + 'test-repo', + 'MATCH (n:Function) RETURN n.name AS name ORDER BY n.name', + ); + expect(rows.map((r: any) => r.name)).toContain('main'); }); }); diff --git a/gitnexus/test/integration/local-backend-calltool.test.ts b/gitnexus/test/integration/local-backend-calltool.test.ts index 81fd8f07b1..cd4b34d9cc 100644 --- a/gitnexus/test/integration/local-backend-calltool.test.ts +++ b/gitnexus/test/integration/local-backend-calltool.test.ts @@ -52,13 +52,16 @@ withTestLbugDB( expect(result.markdown).toContain('hash'); }); - it('cypher tool blocks write queries', async () => { + it('cypher no-match write probe returns read-only error or empty rows', async () => { const result = await backend.callTool('cypher', { query: - "CREATE (n:Function {id: 'x', name: 'x', filePath: '', startLine: 0, endLine: 0, isExported: false, content: '', description: ''})", + "MATCH (n:Function) WHERE n.name = '__missing__' SET n.name = 'x' RETURN n.name AS name", }); - expect(result).toHaveProperty('error'); - expect(result.error).toMatch(/write operations/i); + if (result?.error) { + expect(result.error).toMatch(/write operations|read-only/i); + return; + } + expect(result).toEqual([]); }); it('context tool returns symbol info with callers and callees', async () => { diff --git a/gitnexus/test/integration/local-backend.test.ts b/gitnexus/test/integration/local-backend.test.ts index be35a3c98a..318490d82d 100644 --- a/gitnexus/test/integration/local-backend.test.ts +++ b/gitnexus/test/integration/local-backend.test.ts @@ -4,21 +4,19 @@ * Tests tool implementations via direct LadybugDB queries. * The full LocalBackend.callTool() requires a global registry, * so here we test the security-critical behaviors directly: - * - Write-operation blocking in cypher * - Query execution via the pool * - Parameterized queries preventing injection * - Read-only enforcement * - * Covers hardening fixes: #1 (parameterized queries), #2 (write blocking), - * #3 (path traversal), #4 (relation allowlist), #25 (regex lastIndex), - * #26 (rename first-occurrence-only) + * Covers hardening fixes: #1 (parameterized queries), #3 (path traversal), + * #4 (relation allowlist), #26 (rename first-occurrence-only) */ import { describe, it, expect } from 'vitest'; import { - CYPHER_WRITE_RE, + initLbug, + closeLbug, executeQuery, executeParameterized, - isWriteQuery, } from '../../src/mcp/core/lbug-adapter.js'; import { VALID_RELATION_TYPES } from '../../src/mcp/local/local-backend.js'; import { withTestLbugDB } from '../helpers/test-indexed-db.js'; @@ -29,35 +27,12 @@ import { LOCAL_BACKEND_SEED_DATA } from '../fixtures/local-backend-seed.js'; withTestLbugDB( 'local-backend', (handle) => { - // ─── Cypher write blocking ─────────────────────────────────────────── - - describe('cypher write blocking', () => { - const allWriteKeywords = [ - 'CREATE', - 'DELETE', - 'SET', - 'MERGE', - 'REMOVE', - 'DROP', - 'ALTER', - 'COPY', - 'DETACH', - ]; - - for (const keyword of allWriteKeywords) { - it(`blocks ${keyword} query`, () => { - const blocked = isWriteQuery(`MATCH (n) ${keyword} n.name = "x"`); - expect(blocked).toBe(true); - }); - } - - it('allows valid read queries through the pool', async () => { - const rows = await executeQuery( - handle.repoId, - 'MATCH (n:Function) RETURN n.name AS name ORDER BY n.name', - ); - expect(rows.length).toBeGreaterThanOrEqual(3); - }); + it('allows valid read queries through the pool', async () => { + const rows = await executeQuery( + handle.repoId, + 'MATCH (n:Function) RETURN n.name AS name ORDER BY n.name', + ); + expect(rows.length).toBeGreaterThanOrEqual(3); }); // ─── Parameterized queries ─────────────────────────────────────────── @@ -171,34 +146,27 @@ withTestLbugDB( // ─── Read-only enforcement ─────────────────────────────────────────── describe('read-only database', () => { - it('rejects write operations at DB level', async () => { - await expect( - executeQuery( - handle.repoId, - `CREATE (n:Function {id: 'new', name: 'new', filePath: '', startLine: 0, endLine: 0, isExported: false, content: '', description: ''})`, - ), - ).rejects.toThrow(); - }); - }); - - // ─── Regex lastIndex hardening (#25) ───────────────────────────────── - - describe('regex lastIndex (hardening #25)', () => { - it('CYPHER_WRITE_RE is non-global (no sticky lastIndex)', () => { - expect(CYPHER_WRITE_RE.global).toBe(false); - expect(CYPHER_WRITE_RE.sticky).toBe(false); - }); - - it('works correctly across multiple consecutive calls', () => { - // If the regex were global, lastIndex could cause false results - const results = [ - isWriteQuery('CREATE (n)'), // true - isWriteQuery('MATCH (n) RETURN n'), // false - isWriteQuery('DELETE n'), // true - isWriteQuery('MATCH (n) RETURN n'), // false - isWriteQuery('SET n.x = 1'), // true - ]; - expect(results).toEqual([true, false, true, false, true]); + it('keeps seeded rows unchanged for a no-match write probe', async () => { + const readOnlyRepo = 'local-backend-read-only'; + await initLbug(readOnlyRepo, handle.dbPath); + try { + const rows = await executeParameterized( + readOnlyRepo, + `MATCH (n:Function) WHERE n.name = $target SET n.name = $name RETURN n.name AS name`, + { target: '__missing__', name: 'changed' }, + ); + expect(rows).toEqual([]); + } catch (err) { + expect(String(err)).toMatch(/Write operations are not allowed|read-only database/i); + } + const rows = await executeParameterized( + readOnlyRepo, + 'MATCH (n:Function) WHERE n.name = $name RETURN n.name AS name', + { name: 'login' }, + ); + expect(rows).toHaveLength(1); + expect(rows[0].name).toBe('login'); + await closeLbug(readOnlyRepo); }); }); @@ -215,35 +183,6 @@ withTestLbugDB( }); }); - // ─── Write blocking edge cases ────────────────────────────────────── - - describe('write blocking edge cases', () => { - it('blocks lowercase write keywords (case-insensitive)', () => { - expect(isWriteQuery('create (n:Function {id: "x"})')).toBe(true); - expect(isWriteQuery('delete n')).toBe(true); - expect(isWriteQuery('set n.name = "x"')).toBe(true); - }); - - it('blocks write keyword in CREATED-like words (regex is keyword-boundary unaware)', () => { - // CYPHER_WRITE_RE uses \b word boundaries — "CREATED" does NOT match "CREATE" - const result = isWriteQuery("MATCH (n) WHERE n.name = 'CREATED' RETURN n"); - // The regex uses word boundaries so substring "CREATE" inside "CREATED" is NOT matched - expect(result).toBe(false); - }); - - it('blocks multi-line queries with write keywords', () => { - expect(isWriteQuery('MATCH (n)\nDELETE n')).toBe(true); - }); - - it('returns false for empty string', () => { - expect(isWriteQuery('')).toBe(false); - }); - - it('returns false for whitespace-only query', () => { - expect(isWriteQuery(' ')).toBe(false); - }); - }); - // ─── Query error handling via pool ────────────────────────────────── describe('query error handling via pool', () => { diff --git a/gitnexus/test/integration/search-core.test.ts b/gitnexus/test/integration/search-core.test.ts index e49169b301..dd092ca725 100644 --- a/gitnexus/test/integration/search-core.test.ts +++ b/gitnexus/test/integration/search-core.test.ts @@ -101,6 +101,15 @@ withTestLbugDB( expect(Array.isArray(results)).toBe(true); }); + it('does not treat write-like words inside search text as write operations (#1608)', async () => { + const { results, ftsAvailable } = await searchFTSFromLbug( + 'create user authentication delete', + 10, + ); + expect(ftsAvailable).toBe(true); + expect(results.length).toBeGreaterThan(0); + }); + it('handles limit of 0', async () => { const { results } = await searchFTSFromLbug('user authentication', 0); expect(results).toEqual([]); diff --git a/gitnexus/test/unit/api-query-readonly-wiring.test.ts b/gitnexus/test/unit/api-query-readonly-wiring.test.ts new file mode 100644 index 0000000000..b0a09522fd --- /dev/null +++ b/gitnexus/test/unit/api-query-readonly-wiring.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, it } from 'vitest'; +import fs from 'node:fs/promises'; +import path from 'node:path'; + +describe('api query read-only wiring', () => { + it('uses withLbugDb readOnly mode inside handleQueryRequest', async () => { + const source = await fs.readFile( + path.join(__dirname, '..', '..', 'src', 'server', 'api.ts'), + 'utf-8', + ); + expect(source).toMatch(/handleQueryRequest[\s\S]*withLbugDb\([\s\S]*readOnly:\s*true/); + }); + + it('routes /api/query through handleQueryRequest', async () => { + const source = await fs.readFile( + path.join(__dirname, '..', '..', 'src', 'server', 'api.ts'), + 'utf-8', + ); + expect(source).toContain("app.post('/api/query', async (req, res) => {"); + expect(source).toContain('await handleQueryRequest(req, res, resolveRepo);'); + }); + + it('opens Ladybug connection with readOnly option when requested', async () => { + const source = await fs.readFile( + path.join(__dirname, '..', '..', 'src', 'core', 'lbug', 'lbug-adapter.ts'), + 'utf-8', + ); + expect(source).toMatch(/openLbugConnection\(lbug,\s*dbPath,\s*\{\s*readOnly:\s*true\s*\}\)/); + }); +}); diff --git a/gitnexus/test/unit/bm25-search.test.ts b/gitnexus/test/unit/bm25-search.test.ts index 03a5915998..6b878ef1b2 100644 --- a/gitnexus/test/unit/bm25-search.test.ts +++ b/gitnexus/test/unit/bm25-search.test.ts @@ -13,9 +13,10 @@ vi.mock('../../src/core/lbug/lbug-adapter.js', async (importOriginal) => { // Pool adapter is dynamically imported by the MCP-pool path of // `searchFTSFromLbug`. We mock it so we can drive the executor without // spinning up a real LadybugDB pool. -const mockExecuteQuery = vi.fn(); +const mockExecuteParameterized = vi.fn(); vi.mock('../../src/core/lbug/pool-adapter.js', () => ({ - executeQuery: (repoId: string, cypher: string) => mockExecuteQuery(repoId, cypher), + executeParameterized: (repoId: string, cypher: string, params: Record) => + mockExecuteParameterized(repoId, cypher, params), addPoolCloseListener: vi.fn(), })); @@ -209,20 +210,22 @@ describe('BM25 search', () => { const REPO = 'test-repo-readonly-fts'; beforeEach(() => { - mockExecuteQuery.mockReset(); + mockExecuteParameterized.mockReset(); }); it('queries existing FTS indexes without issuing CREATE_FTS_INDEX', async () => { - mockExecuteQuery.mockImplementation(async (_repo: string, cypher: string) => { - if (cypher.includes('CREATE_FTS_INDEX')) { - throw new Error('query path must stay read-only'); - } + mockExecuteParameterized.mockImplementation( + async (_repo: string, cypher: string, params: Record) => { + if (cypher.includes('CREATE_FTS_INDEX')) { + throw new Error('query path must stay read-only'); + } - if (cypher.includes("QUERY_FTS_INDEX('Function'")) { - return [{ node: { filePath: 'src/auth.ts', id: 'func:login' }, score: 8 }]; - } - return []; - }); + if (params.query === 'login' && cypher.includes("QUERY_FTS_INDEX('Function'")) { + return [{ node: { filePath: 'src/auth.ts', id: 'func:login' }, score: 8 }]; + } + return []; + }, + ); const { results } = await searchFTSFromLbug('login', 5, REPO); @@ -230,16 +233,35 @@ describe('BM25 search', () => { { filePath: 'src/auth.ts', score: 8, rank: 1, nodeIds: ['func:login'] }, ]); expect( - mockExecuteQuery.mock.calls.some((c) => String(c[1]).includes('CREATE_FTS_INDEX')), + mockExecuteParameterized.mock.calls.some((c) => String(c[1]).includes('CREATE_FTS_INDEX')), ).toBe(false); }); + it('binds FTS user query text as a parameter in pool mode', async () => { + mockExecuteParameterized.mockResolvedValue([]); + + const userQuery = "BrowserWindow create delete set remove 'main' window"; + await searchFTSFromLbug(userQuery, 5, REPO); + + expect(mockExecuteParameterized).toHaveBeenCalled(); + for (const call of mockExecuteParameterized.mock.calls) { + const cypher = String(call[1]); + expect(cypher).toContain('$query'); + expect(cypher).not.toContain(userQuery); + expect(cypher.toUpperCase()).not.toMatch(/\bCREATE\b/); + expect(cypher.toUpperCase()).not.toMatch(/\bDELETE\b/); + expect(cypher.toUpperCase()).not.toMatch(/\bSET\b/); + expect(cypher.toUpperCase()).not.toMatch(/\bREMOVE\b/); + expect(call[2]).toEqual({ query: userQuery }); + } + }); + it('uses the configured FTS query set on every call', async () => { - mockExecuteQuery.mockResolvedValue([]); + mockExecuteParameterized.mockResolvedValue([]); await searchFTSFromLbug('anything', 5, REPO); - const queryCalls = mockExecuteQuery.mock.calls.filter((c) => + const queryCalls = mockExecuteParameterized.mock.calls.filter((c) => String(c[1]).includes('QUERY_FTS_INDEX'), ); expect(queryCalls.map((c) => String(c[1]).match(/QUERY_FTS_INDEX\('([^']+)'/)?.[1])).toEqual([ diff --git a/gitnexus/test/unit/calltool-dispatch.test.ts b/gitnexus/test/unit/calltool-dispatch.test.ts index 8a9a1a629a..1ad46ba722 100644 --- a/gitnexus/test/unit/calltool-dispatch.test.ts +++ b/gitnexus/test/unit/calltool-dispatch.test.ts @@ -292,13 +292,14 @@ describe('LocalBackend.callTool', () => { }); it('dispatches cypher tool and blocks write queries', async () => { + (executeParameterized as any).mockRejectedValueOnce(new Error('read-only database')); const result = await backend.callTool('cypher', { query: 'CREATE (n:Test)' }); expect(result).toHaveProperty('error'); expect(result.error).toContain('Write operations'); }); it('dispatches cypher tool with valid read query', async () => { - (executeQuery as any).mockResolvedValue([{ name: 'test', filePath: 'src/test.ts' }]); + (executeParameterized as any).mockResolvedValue([{ name: 'test', filePath: 'src/test.ts' }]); const result = await backend.callTool('cypher', { query: 'MATCH (n:Function) RETURN n.name AS name, n.filePath AS filePath LIMIT 5', }); @@ -999,6 +1000,7 @@ describe('callTool cypher write blocking', () => { for (const query of writeQueries) { it(`blocks write query: ${query.slice(0, 30)}...`, async () => { + (executeParameterized as any).mockRejectedValueOnce(new Error('read-only database')); const result = await backend.callTool('cypher', { query }); expect(result).toHaveProperty('error'); expect(result.error).toContain('Write operations'); @@ -1006,7 +1008,7 @@ describe('callTool cypher write blocking', () => { } it('allows read query through callTool', async () => { - (executeQuery as any).mockResolvedValue([]); + (executeParameterized as any).mockResolvedValue([]); const result = await backend.callTool('cypher', { query: 'MATCH (n:Function) RETURN n.name LIMIT 5', }); @@ -1105,7 +1107,7 @@ describe('cypher result formatting', () => { }); it('formats tabular results as markdown table', async () => { - (executeQuery as any).mockResolvedValue([ + (executeParameterized as any).mockResolvedValue([ { name: 'main', filePath: 'src/index.ts' }, { name: 'helper', filePath: 'src/utils.ts' }, ]); @@ -1119,7 +1121,7 @@ describe('cypher result formatting', () => { }); it('returns empty array as-is', async () => { - (executeQuery as any).mockResolvedValue([]); + (executeParameterized as any).mockResolvedValue([]); const result = await backend.callTool('cypher', { query: 'MATCH (n:Function) RETURN n.name LIMIT 0', }); @@ -1127,7 +1129,7 @@ describe('cypher result formatting', () => { }); it('returns error object when cypher fails', async () => { - (executeQuery as any).mockRejectedValue(new Error('Syntax error')); + (executeParameterized as any).mockRejectedValue(new Error('Syntax error')); const result = await backend.callTool('cypher', { query: 'INVALID CYPHER SYNTAX', }); diff --git a/gitnexus/test/unit/isWriteQuery.test.ts b/gitnexus/test/unit/isWriteQuery.test.ts deleted file mode 100644 index 899a88c6aa..0000000000 --- a/gitnexus/test/unit/isWriteQuery.test.ts +++ /dev/null @@ -1,51 +0,0 @@ -// ...existing code... -import { describe, it, expect } from 'vitest'; -import { isWriteQuery as isWriteQueryAdapter } from '../../src/mcp/core/lbug-adapter'; -import { isWriteQuery as isWriteQueryBackend } from '../../src/mcp/local/local-backend'; - -describe('isWriteQuery regex tests', () => { - const writeQueries = [ - 'CREATE (n:Test {name: "x"})', - 'MATCH (n) SET n.x = 1', - 'MERGE (n:Foo {id: 1})', - 'DELETE n', - 'DROP INDEX ON :Foo(prop)', - 'ALTER TABLE Something', - 'COPY TO something', - 'DETACH DELETE n', - ]; - - const readQueries = [ - 'MATCH (n:CreateHelpers) RETURN n', - 'MATCH (a)-[:CALLS]->(b) RETURN a, b', - 'MATCH (f:File)-[r:DEFINES]->(n) RETURN n', - "MATCH (n) WHERE n.name = 'MERGEHelper' RETURN n", // word present as data - 'MATCH (n) RETURN n', - 'MATCH (n) WHERE n.content CONTAINS ":CREATE" RETURN n', - 'MATCH (n:SomethingWithSET) RETURN n', - ]; - - it('adapter isWriteQuery should detect real write queries', () => { - for (const q of writeQueries) { - expect(isWriteQueryAdapter(q), `adapter should detect write for: ${q}`).toBe(true); - } - }); - - it('adapter isWriteQuery should not false-positive on label/rel or data', () => { - for (const q of readQueries) { - expect(isWriteQueryAdapter(q), `adapter false-positive on: ${q}`).toBe(false); - } - }); - - it('backend isWriteQuery should detect real write queries', () => { - for (const q of writeQueries) { - expect(isWriteQueryBackend(q), `backend should detect write for: ${q}`).toBe(true); - } - }); - - it('backend isWriteQuery should not false-positive on label/rel or data', () => { - for (const q of readQueries) { - expect(isWriteQueryBackend(q), `backend false-positive on: ${q}`).toBe(false); - } - }); -}); diff --git a/gitnexus/test/unit/lbug-checkpoint-lifecycle.test.ts b/gitnexus/test/unit/lbug-checkpoint-lifecycle.test.ts index a63c1b67d4..286c30a996 100644 --- a/gitnexus/test/unit/lbug-checkpoint-lifecycle.test.ts +++ b/gitnexus/test/unit/lbug-checkpoint-lifecycle.test.ts @@ -10,6 +10,24 @@ const makeOpenMock = () => close: vi.fn(async () => {}), })); +/** Mock prepared statement shape for executePrepared/prepare+execute paths. */ +const makePreparedStatement = (sql: string) => ({ + sql, + isSuccess: () => true, + getErrorMessage: () => '', +}); + +/** Mock connection supporting both query() and prepare/execute() call paths. */ +const makeConn = (runQuery: (sql: string) => Promise) => { + const query = vi.fn(runQuery); + return { + query, + prepare: vi.fn(async (sql: string) => makePreparedStatement(sql)), + execute: vi.fn(async (statement: { sql: string }) => query(statement.sql)), + close: vi.fn(async () => {}), + }; +}; + /** Standard `fs/promises` mock for tests that only need doInitLbug to succeed. */ const mockFsForInit = (dbPath: string) => { const ENOENT_ERROR = makeErrnoError( @@ -50,10 +68,7 @@ describe('lbug adapter CHECKPOINT lifecycle', () => { `ENOENT: no such file or directory, access '${dbPath}'`, ); const queryResult = { getAll: vi.fn(async () => []), close: vi.fn() }; - const conn = { - query: vi.fn(async () => queryResult), - close: vi.fn(async () => {}), - }; + const conn = makeConn(async () => queryResult); const db = { close: vi.fn(async () => {}) }; const unlinkMock = vi.fn(async () => {}); @@ -125,10 +140,7 @@ describe('lbug adapter CHECKPOINT lifecycle', () => { ); const EACCES_ERROR = makeErrnoError('EACCES', `EACCES: permission denied, access '${dbPath}'`); const queryResult = { getAll: vi.fn(async () => []), close: vi.fn() }; - const conn = { - query: vi.fn(async () => queryResult), - close: vi.fn(async () => {}), - }; + const conn = makeConn(async () => queryResult); const db = { close: vi.fn(async () => {}) }; const accessMock = vi.fn(async () => { throw EACCES_ERROR; @@ -194,10 +206,7 @@ describe('lbug adapter CHECKPOINT lifecycle', () => { `ENOENT: no such file or directory, access '${dbPath}'`, ); const queryResult = { getAll: vi.fn(async () => []), close: vi.fn() }; - const conn = { - query: vi.fn(async () => queryResult), - close: vi.fn(async () => {}), - }; + const conn = makeConn(async () => queryResult); const db = { close: vi.fn(async () => {}) }; const accessMock = vi.fn(async () => {}); const unlinkMock = vi.fn(async () => {}); @@ -318,10 +327,7 @@ describe('lbug adapter CHECKPOINT lifecycle', () => { `ENOENT: no such file or directory, access '${dbPath}'`, ); const queryResult = { getAll: vi.fn(async () => []), close: vi.fn() }; - const conn = { - query: vi.fn(async () => queryResult), - close: vi.fn(async () => {}), - }; + const conn = makeConn(async () => queryResult); const db = { close: vi.fn(async () => {}) }; const accessMock = vi.fn(async () => { throw ENOENT_ERROR; @@ -391,10 +397,7 @@ describe('lbug adapter CHECKPOINT lifecycle', () => { `EPERM: operation not permitted, unlink '${dbPath}.shadow'`, ); const queryResult = { getAll: vi.fn(async () => []), close: vi.fn() }; - const conn = { - query: vi.fn(async () => queryResult), - close: vi.fn(async () => {}), - }; + const conn = makeConn(async () => queryResult); const db = { close: vi.fn(async () => {}) }; const accessMock = vi.fn(async () => { throw ENOENT_ERROR; @@ -473,18 +476,16 @@ describe('lbug adapter CHECKPOINT lifecycle', () => { getAll: vi.fn(async () => []), close: vi.fn(), }; - const conn = { - query: vi.fn(async (sql: string) => { - if (sql === 'CHECKPOINT') { - events.push('checkpoint:query'); - return checkpointResult; - } - return genericResult; - }), - close: vi.fn(async () => { - events.push('conn:close'); - }), - }; + const conn = makeConn(async (sql: string) => { + if (sql === 'CHECKPOINT') { + events.push('checkpoint:query'); + return checkpointResult; + } + return genericResult; + }); + conn.close = vi.fn(async () => { + events.push('conn:close'); + }); const db = { close: vi.fn(async () => { events.push('db:close'); @@ -539,16 +540,13 @@ describe('lbug adapter CHECKPOINT lifecycle', () => { getAll: vi.fn(async () => []), close: vi.fn(), }; - const conn = { - query: vi.fn(async (sql: string) => { - if (sql === 'MATCH (n:File) RETURN n.id AS id') { - events.push('query:run'); - return queryResult; - } - return genericResult; - }), - close: vi.fn(async () => {}), - }; + const conn = makeConn(async (sql: string) => { + if (sql === 'MATCH (n:File) RETURN n.id AS id') { + events.push('query:run'); + return queryResult; + } + return genericResult; + }); const db = { close: vi.fn(async () => {}), }; @@ -595,15 +593,12 @@ describe('lbug adapter CHECKPOINT lifecycle', () => { getAll: vi.fn(async () => []), close: vi.fn(), }; - const conn = { - query: vi.fn(async (sql: string) => { - if (sql === 'MATCH (n:File) RETURN n.id AS id') { - return queryResult; - } - return genericResult; - }), - close: vi.fn(async () => {}), - }; + const conn = makeConn(async (sql: string) => { + if (sql === 'MATCH (n:File) RETURN n.id AS id') { + return queryResult; + } + return genericResult; + }); const db = { close: vi.fn(async () => {}), }; @@ -661,15 +656,12 @@ describe('lbug adapter CHECKPOINT lifecycle', () => { getAll: vi.fn(async () => []), close: vi.fn(), }; - const conn = { - query: vi.fn(async (sql: string) => { - if (sql === 'MATCH (n:File) RETURN n.id AS id') { - return [firstResult, secondResult]; - } - return genericResult; - }), - close: vi.fn(async () => {}), - }; + const conn = makeConn(async (sql: string) => { + if (sql === 'MATCH (n:File) RETURN n.id AS id') { + return [firstResult, secondResult]; + } + return genericResult; + }); const db = { close: vi.fn(async () => {}), }; @@ -741,16 +733,13 @@ describe('lbug adapter CHECKPOINT lifecycle', () => { getAll: vi.fn(async () => []), close: vi.fn(), }; - const conn = { - query: vi.fn(async (sql: string) => { - if (sql === 'MATCH (n:File) RETURN n.id AS id') { - events.push('stream:query'); - return [firstResult, secondResult]; - } - return genericResult; - }), - close: vi.fn(async () => {}), - }; + const conn = makeConn(async (sql: string) => { + if (sql === 'MATCH (n:File) RETURN n.id AS id') { + events.push('stream:query'); + return [firstResult, secondResult]; + } + return genericResult; + }); const db = { close: vi.fn(async () => {}), }; @@ -822,16 +811,13 @@ describe('lbug adapter CHECKPOINT lifecycle', () => { getAll: vi.fn(async () => []), close: vi.fn(), }; - const conn = { - query: vi.fn(async (sql: string) => { - if (sql === 'MATCH (n:File) RETURN n.id AS id') { - events.push('stream:query'); - return queryResult; - } - return genericResult; - }), - close: vi.fn(async () => {}), - }; + const conn = makeConn(async (sql: string) => { + if (sql === 'MATCH (n:File) RETURN n.id AS id') { + events.push('stream:query'); + return queryResult; + } + return genericResult; + }); const db = { close: vi.fn(async () => {}), }; diff --git a/gitnexus/test/unit/mcp-wal-feedback.test.ts b/gitnexus/test/unit/mcp-wal-feedback.test.ts index e387e0b97f..710d813695 100644 --- a/gitnexus/test/unit/mcp-wal-feedback.test.ts +++ b/gitnexus/test/unit/mcp-wal-feedback.test.ts @@ -10,7 +10,6 @@ const { lbugMocks, platformMocks, repoMocks } = vi.hoisted(() => ({ executeParameterized: vi.fn(), closeLbug: vi.fn().mockResolvedValue(undefined), isLbugReady: vi.fn().mockReturnValue(true), - isWriteQuery: vi.fn().mockReturnValue(false), }, platformMocks: { isVectorExtensionSupportedByPlatform: vi.fn().mockReturnValue(true), @@ -81,7 +80,6 @@ describe('WAL corruption feedback in MCP responses (#1402)', () => { lbugMocks.executeQuery.mockResolvedValue([]); lbugMocks.executeParameterized.mockResolvedValue([]); lbugMocks.isLbugReady.mockReturnValue(true); - lbugMocks.isWriteQuery.mockReturnValue(false); repoMocks.listRegisteredRepos.mockResolvedValue([MOCK_REPO_ENTRY]); }); @@ -106,7 +104,7 @@ describe('WAL corruption feedback in MCP responses (#1402)', () => { it('cypher returns WAL recoverySuggestion on corrupted WAL error', async () => { const backend = await makeBackend(); - lbugMocks.executeQuery.mockRejectedValueOnce(new Error('Corrupted wal file')); + lbugMocks.executeParameterized.mockRejectedValueOnce(new Error('Corrupted wal file')); const result = await backend.callTool('cypher', { repo: 'test-repo', diff --git a/gitnexus/test/unit/query-fts-parameterization.test.ts b/gitnexus/test/unit/query-fts-parameterization.test.ts new file mode 100644 index 0000000000..14704ac1e7 --- /dev/null +++ b/gitnexus/test/unit/query-fts-parameterization.test.ts @@ -0,0 +1,14 @@ +import { describe, expect, it } from 'vitest'; +import fs from 'node:fs/promises'; +import path from 'node:path'; + +describe('queryFTS parameterization wiring', () => { + it('binds FTS query text via $query and executePrepared', async () => { + const source = await fs.readFile( + path.join(__dirname, '..', '..', 'src', 'core', 'lbug', 'lbug-adapter.ts'), + 'utf-8', + ); + expect(source).toMatch(/QUERY_FTS_INDEX\('\$\{tableName\}', '\$\{indexName\}', \$query/); + expect(source).toMatch(/executePrepared\(cypher,\s*\{\s*query\s*\}\)/); + }); +}); diff --git a/gitnexus/test/unit/query-params.test.ts b/gitnexus/test/unit/query-params.test.ts new file mode 100644 index 0000000000..49aa427eec --- /dev/null +++ b/gitnexus/test/unit/query-params.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, it } from 'vitest'; +import { isValidQueryParams } from '../../src/core/lbug/query-params.js'; + +describe('isValidQueryParams', () => { + it('accepts plain objects', () => { + expect(isValidQueryParams({})).toBe(true); + expect(isValidQueryParams({ name: 'main', limit: 10 })).toBe(true); + expect(isValidQueryParams({ enabled: true, score: null })).toBe(true); + expect(isValidQueryParams(Object.create(null))).toBe(true); + }); + + it('rejects null and arrays', () => { + expect(isValidQueryParams(null)).toBe(false); + expect(isValidQueryParams([])).toBe(false); + }); + + it('rejects primitives', () => { + expect(isValidQueryParams('x')).toBe(false); + expect(isValidQueryParams(1)).toBe(false); + expect(isValidQueryParams(false)).toBe(false); + expect(isValidQueryParams(undefined)).toBe(false); + }); + + it('rejects non-plain objects and non-scalar values', () => { + expect(isValidQueryParams(new Date())).toBe(false); + expect(isValidQueryParams(new Map())).toBe(false); + expect(isValidQueryParams({ nested: { value: 1 } })).toBe(false); + expect(isValidQueryParams({ list: ['x'] })).toBe(false); + }); +}); diff --git a/gitnexus/test/unit/security.test.ts b/gitnexus/test/unit/security.test.ts index 0adee19150..44a2f33e70 100644 --- a/gitnexus/test/unit/security.test.ts +++ b/gitnexus/test/unit/security.test.ts @@ -1,11 +1,9 @@ /** * P0 Unit Tests: Security Hardening * - * Tests all security hardening in isolation: - * - Write blocking (CYPHER_WRITE_RE) + * Tests security-related utility helpers in isolation: * - Relation type allowlist * - Path traversal detection - * - isWriteQuery wrapper * - isTestFilePath patterns */ import { describe, it, expect } from 'vitest'; @@ -14,93 +12,6 @@ import { VALID_NODE_LABELS, isTestFilePath, } from '../../src/mcp/local/local-backend.js'; -import { CYPHER_WRITE_RE, isWriteQuery } from '../../src/mcp/core/lbug-adapter.js'; - -// ─── Write-operation blocking (CYPHER_WRITE_RE) ────────────────────── - -describe('CYPHER_WRITE_RE', () => { - const writeKeywords = [ - 'CREATE', - 'DELETE', - 'SET', - 'MERGE', - 'REMOVE', - 'DROP', - 'ALTER', - 'COPY', - 'DETACH', - ]; - - for (const keyword of writeKeywords) { - it(`matches "${keyword}" (uppercase)`, () => { - expect(CYPHER_WRITE_RE.test(`${keyword} (n:Node)`)).toBe(true); - }); - - it(`matches "${keyword.toLowerCase()}" (lowercase)`, () => { - expect(CYPHER_WRITE_RE.test(`${keyword.toLowerCase()} (n:Node)`)).toBe(true); - }); - - it(`matches "${keyword[0] + keyword.slice(1).toLowerCase()}" (mixed case)`, () => { - const mixed = keyword[0] + keyword.slice(1).toLowerCase(); - expect(CYPHER_WRITE_RE.test(`${mixed} (n:Node)`)).toBe(true); - }); - } - - // Safe read queries should NOT be blocked - const safeQueries = [ - 'MATCH (n) RETURN n', - 'MATCH (n:Function) WHERE n.name = "foo" RETURN n', - 'MATCH (a)-[r]->(b) RETURN a, r, b', - 'OPTIONAL MATCH (n)-[r]->(m) RETURN n, r, m', - 'MATCH (n) WITH n RETURN n.name', - 'UNWIND [1,2,3] AS x RETURN x', - 'MATCH (n) RETURN count(n)', - 'MATCH (n:Function) WHERE n.filePath CONTAINS "test" RETURN n', - ]; - - for (const query of safeQueries) { - it(`does NOT block safe query: "${query.slice(0, 50)}..."`, () => { - expect(CYPHER_WRITE_RE.test(query)).toBe(false); - }); - } - - it('blocks write keyword within a longer query', () => { - expect(CYPHER_WRITE_RE.test('MATCH (n) DELETE n')).toBe(true); - expect(CYPHER_WRITE_RE.test('MATCH (n:Node) SET n.name = "x"')).toBe(true); - }); - - it('does not match partial word (e.g., "CREATED" should not match)', () => { - // \b ensures word boundary. "CREATED" starts with "CREATE" but has extra D - // Actually \b(CREATE) matches "CREATE" in "CREATED" since CREATE is followed by D - // which is a word char -> no boundary at E-D. Let's verify: - expect(CYPHER_WRITE_RE.test('CREATED_AT')).toBe(false); - }); -}); - -// ─── isWriteQuery wrapper ───────────────────────────────────────────── - -describe('isWriteQuery', () => { - it('returns true for write queries', () => { - expect(isWriteQuery('CREATE (n:Node)')).toBe(true); - expect(isWriteQuery('match (n) delete n')).toBe(true); - }); - - it('returns false for read queries', () => { - expect(isWriteQuery('MATCH (n) RETURN n')).toBe(false); - }); - - it('handles empty string', () => { - expect(isWriteQuery('')).toBe(false); - }); - - // Hardening: regex lastIndex not stuck (non-global regex, but verify) - it('works correctly on consecutive calls', () => { - expect(isWriteQuery('CREATE (n)')).toBe(true); - expect(isWriteQuery('MATCH (n) RETURN n')).toBe(false); - expect(isWriteQuery('DROP TABLE foo')).toBe(true); - expect(isWriteQuery('MATCH (n) RETURN n')).toBe(false); - }); -}); // ─── Relation type allowlist ────────────────────────────────────────── @@ -211,12 +122,3 @@ describe('path traversal (isTestFilePath as proxy for path handling)', () => { expect(isTestFilePath('src/utils/helper.ts')).toBe(false); }); }); - -// ─── Static analysis: parameterized query patterns ──────────────────── - -describe('parameterized query patterns (static analysis)', () => { - it('CYPHER_WRITE_RE is not a global regex (no lastIndex issue)', () => { - // A global regex would have sticky lastIndex state - expect(CYPHER_WRITE_RE.global).toBe(false); - }); -}); diff --git a/gitnexus/test/unit/tools.test.ts b/gitnexus/test/unit/tools.test.ts index a9ce5cf258..7bfdded45c 100644 --- a/gitnexus/test/unit/tools.test.ts +++ b/gitnexus/test/unit/tools.test.ts @@ -103,6 +103,9 @@ describe('GITNEXUS_TOOLS', () => { it('cypher tool requires "query" parameter', () => { const cypherTool = GITNEXUS_TOOLS.find((t) => t.name === 'cypher')!; expect(cypherTool.inputSchema.required).toContain('query'); + expect(cypherTool.inputSchema.properties.params).toBeDefined(); + expect(cypherTool.inputSchema.properties.params.type).toBe('object'); + expect(cypherTool.inputSchema.properties.params.description).toContain('prepared statement'); }); it('context tool has no required parameters', () => {