From 678a0e11c965aed4e158685542da4c4996aa5358 Mon Sep 17 00:00:00 2001 From: ChunxueLi <54129170+ChunxueLi@users.noreply.github.com> Date: Tue, 1 Sep 2026 03:43:28 +0800 Subject: [PATCH] =?UTF-8?q?fix(server,web):=20honor=20the=20grep=20tool=20?= =?UTF-8?q?contract=20=E2=80=94=20real=20regex,=20fileFilter,=20caseSensit?= =?UTF-8?q?ive=20(#3109)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(server,web): honor grep tool contract — real regex, fileFilter, caseSensitive (Patch 12) Background ========== The web chat's grep tool schema has always promised regex search with an optional path-substring fileFilter and caseSensitive control, but the GET /api/grep handler escapeRegExp()'d every pattern into a literal substring (a ReDoS hardening from fa36254e / #1317 that never re-synced the tool contract). Consequences, verified in production use against the sr-next backend repo (23k-file Java monorepo): - An agent sending the documented alternation form ("sign|Sign") got zero hits and concluded the sign/签署 interface did not exist. - The schema's own example pattern ("console\\.log") could never match: the escaped literal searched for a backslash in the source. - fileFilter / caseSensitive were read by nobody — pure schema fiction. - The web handler worked around the server with a (?=.*filter).*pattern lookahead splice that the same escaping also defeated. - Collateral: the impact tool's grep fallback (\b${escapeRegex(name)}\b) was silently dead code under literal semantics; it comes back to life with this fix (expected improvement, noted for reviewers). Fix === Server (gitnexus): - New src/server/grep-params.ts — pure query-param parser (no Express / native imports, per the #2790 helper-extraction convention): regex construction (default real regex; literal=1 restores the old escaped-substring semantics as an opt-out), lowercase path-substring fileFilter, caseSensitive flag, limit clamp [1,200] default 50, BadRequestError error paths (mapped to 400 by statusFromError). - /api/grep handler in api.ts becomes thin wiring: fileFilter path filtering before the (unchanged) traversal guard, a 5s wall-clock budget checked between files (partial results plus timedOut: true), read-only DB open unchanged. The regex is deliberately built WITHOUT the 'g' flag: the handler tests line-by-line and a stale lastIndex would skip matches (the old code had to reset it manually); 'm' is likewise omitted — each test sees one line, so ^/$ already anchor at string boundaries. Web (gitnexus-web): - tools.ts: drop the lookahead splice; description now tells the model the truth (real regex, alternation works, path-substring filter, case-insensitive default, result cap and time budget). - backend-client.ts: grep() takes GrepOptions {fileFilter,caseSensitive} and forwards them as query params. - useAppState.tsx: assembly site threads the options through. Security — residual ReDoS exposure (read this before deploying) =============================================================== The literal-only era was accidentally ReDoS-immune; this patch knowingly trades that immunity back for the promised contract. The bounds (200-char pattern cap, line-by-line matching, result cap, 5s budget) do NOT cover a single catastrophically backtracking regex.test(): it blocks the Node event loop synchronously, the budget (checked between files) cannot interrupt it, and the whole server is unresponsive for the duration (measured: (a+)+$ against a 35-char line exceeds 120 seconds). Accepted because local serve binds loopback by default and hosted deploys gate /api/grep behind the edge token; documented in SECURITY.md (new section) with the worker_threads+terminate / optional-re2 follow-up called out. literal=1 restores full immunity for untrusted callers. Compatibility audit =================== Repo-wide: /api/grep's only HTTP caller is backend-client.grep(); the MCP tool surface has no grep tool; eval/ uses shell grep, not this endpoint; the endpoint is undocumented (docs/llms.txt) with no known third-party consumers. Breaking surface ≈ zero. Pattern metacharacter semantics change for direct curl users ("array[0]" now needs escaping or literal=1). Tests ===== +20 cases in test/unit/grep-params.test.ts: alternation (the regression that burned the agent), the schema's own example, case flags, literal compat, CJK patterns, ^/$ line anchors, fileFilter normalization + array-form rejection, limit clamping, type-confusion guards, invalid regex, and a source-level handler wiring assertion (api-readonly-wiring style). Full unit suite: no new failures (22 pre-existing failures reproduced identically with this patch stashed — analyzer-identity dist fingerprint + lbug native-env classes). Upstream plan ============= Issue + PR to abhigyanpatwari/GitNexus; the PR description must front the ReDoS trade-off with the worker-isolation follow-up. Repro for the issue: curl ".../api/grep?pattern=TODO%7CFIXME" — 0 hits under literal semantics, both marker classes under regex semantics. Custom-patch ledger: CUSTOM_PATCHES.md Patch 12. * style: prettier * fix(web): surface grep timedOut so partial scans are not silent misses Propagate the server timeout flag through the backend client and chat tool, check the 5s budget between lines, and document the accepted regex-injection CodeQL finding next to new RegExp. * Address PR review feedback (#3109) - Cover empty and null fileFilter in the grep client test - Keep timedOut as a required boolean and reuse GrepOptions - Sample the grep deadline every 256 lines instead of every line Note: pre-existing failure in impact-tool.test.ts not addressed by this PR. * Address PR review feedback (#3109) Run /api/grep matching in a worker_threads worker so terminate() can cut a catastrophic regex.test without blocking the parent event loop. * Address PR review feedback (#3109) Reset lastIndex per line, restore the missing-pattern 400 message, and make the traversal test create a real outside file. * fix(web): align agent grep opts with GrepOptions Use GrepOptions so fileFilter null is accepted by the local GraphRAGBackend stub. * fix(server): silence CodeQL js/regex-injection on intentional grep regex Split literal vs regex construction and suppress with the correct rule id (js/regex-injection). Real regex remains the default contract; literal=1 still escapes. * Address PR review feedback (#3109) Clean up grep-scan temp dirs after each test, and exclude the intentional grep-params RegExp site from CodeQL so js/regex-injection does not re-file. --------- Co-authored-by: l.cx Co-authored-by: ChunxueLi Co-authored-by: Gergő Magyar Co-authored-by: Gergo Magyar --- .github/workflows/codeql.yml | 6 + SECURITY.md | 4 + gitnexus-web/src/core/llm/tools.ts | 36 ++-- gitnexus-web/src/hooks/useAppState.tsx | 4 +- gitnexus-web/src/services/backend-client.ts | 28 ++- gitnexus-web/test/unit/agent-prompt.test.ts | 2 +- .../test/unit/backend-client-grep.test.ts | 75 ++++++++ gitnexus-web/test/unit/grep-tool.test.ts | 36 ++++ gitnexus-web/test/unit/impact-tool.test.ts | 2 +- gitnexus/src/server/api.ts | 87 +++------- gitnexus/src/server/grep-params.ts | 98 +++++++++++ gitnexus/src/server/grep-scan.ts | 139 +++++++++++++++ gitnexus/src/server/grep-worker.ts | 16 ++ gitnexus/test/unit/grep-params.test.ts | 160 ++++++++++++++++++ gitnexus/test/unit/grep-scan.test.ts | 101 +++++++++++ 15 files changed, 707 insertions(+), 87 deletions(-) create mode 100644 gitnexus-web/test/unit/backend-client-grep.test.ts create mode 100644 gitnexus-web/test/unit/grep-tool.test.ts create mode 100644 gitnexus/src/server/grep-params.ts create mode 100644 gitnexus/src/server/grep-scan.ts create mode 100644 gitnexus/src/server/grep-worker.ts create mode 100644 gitnexus/test/unit/grep-params.test.ts create mode 100644 gitnexus/test/unit/grep-scan.test.ts diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 41440d557..a768b69e7 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -71,6 +71,12 @@ jobs: # deliberately contain use-before-init / unused-variable shapes). - '**/test/fixtures/**' - '**/test/**/fixtures/**' + # GET /api/grep intentionally builds RegExp from the query string + # (literal=1 escapes). ReDoS is handled by worker terminate() — + # see SECURITY.md. Inline codeql[] comments do not clear the + # GitHub PR CodeQL gate, so this file is excluded to avoid + # re-filing js/regex-injection on every push of the same line. + - 'gitnexus/src/server/grep-params.ts' - name: Perform CodeQL Analysis uses: github/codeql-action/analyze@ff2f1c621b7f889edc0d3c761ac2e6a3f8cdb0dd # v4.37.7 diff --git a/SECURITY.md b/SECURITY.md index 89368a1ea..37d216911 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -65,6 +65,10 @@ The `render.yaml` Blueprint (see the README's **Deploy to Render**) puts `gitnex Do not hand the URL out as a public demo. A token holder has read access to everything the deploy has indexed. +### `/api/grep` regex semantics and residual ReDoS exposure + +`GET /api/grep` executes caller-supplied patterns as real regular expressions (with an optional path-substring `fileFilter` and `caseSensitive` flag) to honor the web chat's grep tool contract; `literal=1` restores the older escaped-substring mode. Mitigations: a 200-character pattern cap, line-by-line matching, a max-200 result cap, and a 5-second wall-clock budget. Matching runs in a `worker_threads` worker so a catastrophic pattern (e.g. `(a+)+$`) can be killed with `terminate()` when the budget expires — the parent event loop (other routes + SSE) stays responsive. A timed-out scan returns partial results with `timedOut: true`; the web grep tool surfaces that flag so an agent does not treat a cut-off scan as exhaustive. CodeQL still flags constructing a `RegExp` from the query string; that is the advertised contract, not accidental injection. Hosted deploys continue to gate the route behind the edge token. + ## Automated Scans Running in CI This repository runs the following scans automatically. Findings appear under the repository's **Security → Code scanning** tab. diff --git a/gitnexus-web/src/core/llm/tools.ts b/gitnexus-web/src/core/llm/tools.ts index 407018678..421725be3 100644 --- a/gitnexus-web/src/core/llm/tools.ts +++ b/gitnexus-web/src/core/llm/tools.ts @@ -14,7 +14,11 @@ import { tool } from '@langchain/core/tools'; import { z } from 'zod'; import { NODE_TABLES, REL_TYPES, scoreImpactRisk, unusedAxesForImpactWalk } from 'gitnexus-shared'; -import type { EnrichedSearchResult, GrepResult } from '../../services/backend-client'; +import type { + EnrichedSearchResult, + GrepOptions, + GrepResponse, +} from '../../services/backend-client'; /** * Tool names registered by createGraphRAGTools — kept in sync with each tool's `name` @@ -44,7 +48,7 @@ export interface GraphRAGBackend { query: string, opts?: { limit?: number; mode?: 'hybrid' | 'semantic' | 'bm25'; enrich?: boolean }, ) => Promise; - grep: (pattern: string, limit?: number) => Promise; + grep: (pattern: string, limit?: number, opts?: GrepOptions) => Promise; readFile: (filePath: string) => Promise; } @@ -375,20 +379,22 @@ MATCH (n:Function {id: emb.nodeId}) RETURN n`, } const limit = maxResults ?? 100; - const fullPattern = fileFilter - ? `(?=.*${fileFilter.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}).*${pattern}` - : pattern; - - const results = await backendGrep(fullPattern, limit); + const { results, timedOut } = await backendGrep(pattern, limit, { + fileFilter, + caseSensitive, + }); + const timeoutMsg = timedOut + ? '\n\n(Scan timed out after a few seconds — results may be incomplete)' + : ''; if (results.length === 0) { - return `No matches for "${pattern}"${fileFilter ? ` in files matching "${fileFilter}"` : ''}`; + return `No matches for "${pattern}"${fileFilter ? ` in files matching "${fileFilter}"` : ''}${timeoutMsg}`; } const formatted = results.map((r) => `${r.filePath}:${r.line}: ${r.text}`).join('\n'); const truncatedMsg = results.length >= limit ? `\n\n(Showing first ${limit} results)` : ''; - return `Found ${results.length} matches:\n\n${formatted}${truncatedMsg}`; + return `Found ${results.length} matches:\n\n${formatted}${truncatedMsg}${timeoutMsg}`; } catch (error) { return `Grep error: ${error instanceof Error ? error.message : String(error)}`; } @@ -396,16 +402,20 @@ MATCH (n:Function {id: emb.nodeId}) RETURN n`, { name: 'grep', description: - 'Search for exact text patterns across all files using regex. Use for finding specific strings, error messages, TODOs, variable names, etc.', + 'Search file contents with a regular expression (server executes it as a real regex — alternation like "sign|Sign" works). Matches are case-insensitive unless caseSensitive is set. fileFilter keeps only files whose path contains the substring. Each call caps at maxResults matches (default 100) and the server stops after a few seconds (the tool will say so if the scan was incomplete), so prefer precise patterns over catch-alls.', schema: z.object({ pattern: z .string() - .describe('Regex pattern to search for (e.g., "TODO", "console\\.log", "API_KEY")'), + .describe( + 'Regex pattern to search for (e.g., "TODO|FIXME", "console\\.log", "signOrder")', + ), fileFilter: z .string() .optional() .nullable() - .describe('Only search files containing this string (e.g., ".ts", "src/api")'), + .describe( + 'Only search files whose path contains this substring (e.g., ".ts", "src/api", "Controller.java")', + ), caseSensitive: z .boolean() .optional() @@ -1219,7 +1229,7 @@ MATCH (n:Function {id: emb.nodeId}) RETURN n`, const targetFileName = (targetFilePath || target).split('/').pop() || target; const baseName = targetFileName.replace(/\.[^/.]+$/, ''); try { - const hints = await backendGrep(`\\b${escapeRegex(baseName)}\\b`, 15); + const { results: hints } = await backendGrep(`\\b${escapeRegex(baseName)}\\b`, 15); const filtered = hints.filter((h) => h.filePath !== targetFilePath); if (filtered.length > 0) { diff --git a/gitnexus-web/src/hooks/useAppState.tsx b/gitnexus-web/src/hooks/useAppState.tsx index d698b87d5..deeda86bf 100644 --- a/gitnexus-web/src/hooks/useAppState.tsx +++ b/gitnexus-web/src/hooks/useAppState.tsx @@ -40,6 +40,7 @@ import { repoIdentity as repoIdentityOf, type BackendRepo, type ConnectResult, + type GrepOptions, type JobProgress, } from '../services/backend-client'; import { ERROR_RESET_DELAY_MS } from '../config/ui-constants'; @@ -671,7 +672,8 @@ const AppStateProviderInner = ({ children }: { children: ReactNode }) => { const backend = { executeQuery, search: (query: string, opts?: any) => backendSearch(query, { ...opts, repo }), - grep: (pattern: string, limit?: number) => backendGrep(pattern, repo, limit), + grep: (pattern: string, limit?: number, opts?: GrepOptions) => + backendGrep(pattern, repo, limit, opts), readFile: (filePath: string) => backendReadFile(filePath, { repo }).then((r) => r.content), }; diff --git a/gitnexus-web/src/services/backend-client.ts b/gitnexus-web/src/services/backend-client.ts index 52f8c65c9..706b6d90e 100644 --- a/gitnexus-web/src/services/backend-client.ts +++ b/gitnexus-web/src/services/backend-client.ts @@ -64,6 +64,12 @@ export interface GrepResult { text: string; } +/** Full `/api/grep` payload — `timedOut` is true when the 5s budget cut the scan short. */ +export interface GrepResponse { + results: GrepResult[]; + timedOut: boolean; +} + export interface JobProgress { phase: string; percent: number; @@ -869,23 +875,37 @@ export const search = async ( return (body.results ?? []) as EnrichedSearchResult[]; }; -/** Grep across file contents in the indexed repo. */ +/** Options for {@link grep} beyond pattern/repo/limit. */ +export interface GrepOptions { + /** Only search files whose path contains this substring (case-insensitive). */ + fileFilter?: string | null; + /** Case-sensitive matching (default: insensitive). */ + caseSensitive?: boolean; +} + +/** Grep across file contents in the indexed repo. Regex semantics server-side. */ export const grep = async ( pattern: string, repo?: string, limit?: number, -): Promise => { + opts?: GrepOptions, +): Promise => { const params = [ `pattern=${encodeURIComponent(pattern)}`, repoParam(repo), limit ? `limit=${limit}` : '', + opts?.fileFilter ? `fileFilter=${encodeURIComponent(opts.fileFilter)}` : '', + opts?.caseSensitive ? 'caseSensitive=1' : '', ] .filter(Boolean) .join('&'); const response = await fetchWithTimeout(`${_backendUrl}/api/grep?${params}`); await assertOk(response); - const body = await response.json(); - return (body.results ?? []) as GrepResult[]; + const body = (await response.json()) as Partial; + return { + results: body.results ?? [], + timedOut: body.timedOut === true, + }; }; /** Result from reading a file, optionally with line range. */ diff --git a/gitnexus-web/test/unit/agent-prompt.test.ts b/gitnexus-web/test/unit/agent-prompt.test.ts index c2cb1392f..8bf69a513 100644 --- a/gitnexus-web/test/unit/agent-prompt.test.ts +++ b/gitnexus-web/test/unit/agent-prompt.test.ts @@ -43,7 +43,7 @@ const FORBIDDEN_TOOL_NAMES = [ const stubBackend: GraphRAGBackend = { executeQuery: async () => [], search: async () => [], - grep: async () => [], + grep: async () => ({ results: [], timedOut: false }), readFile: async () => '', }; diff --git a/gitnexus-web/test/unit/backend-client-grep.test.ts b/gitnexus-web/test/unit/backend-client-grep.test.ts new file mode 100644 index 000000000..183068d5e --- /dev/null +++ b/gitnexus-web/test/unit/backend-client-grep.test.ts @@ -0,0 +1,75 @@ +/** + * `/api/grep` client: query params and `timedOut` must reach callers. + * Dropping `timedOut` made a 5s partial scan look like a complete miss. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { __resetBreakerRegistry__ } from 'gitnexus-shared/test-helpers'; +import { grep, setBackendUrl } from '../../src/services/backend-client'; + +const BASE = 'http://grep-client.test:4747'; + +const jsonOk = (body: unknown) => + new Response(JSON.stringify(body), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }); + +describe('backend-client grep', () => { + beforeEach(() => { + __resetBreakerRegistry__(); + setBackendUrl(BASE); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it('forwards fileFilter and caseSensitive and returns timedOut', async () => { + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + const url = String(input); + expect(url).toContain('/api/grep?'); + expect(url).toContain(`pattern=${encodeURIComponent('sign|Sign')}`); + expect(url).toContain(`fileFilter=${encodeURIComponent('src/api')}`); + expect(url).toContain('caseSensitive=1'); + expect(url).toContain('limit=12'); + return jsonOk({ + results: [{ filePath: 'src/api.ts', line: 3, text: 'signOrder()' }], + timedOut: true, + }); + }); + vi.stubGlobal('fetch', fetchMock); + + const body = await grep('sign|Sign', '/repo', 12, { + fileFilter: 'src/api', + caseSensitive: true, + }); + expect(body.results).toEqual([{ filePath: 'src/api.ts', line: 3, text: 'signOrder()' }]); + expect(body.timedOut).toBe(true); + }); + + it('reports timedOut false when the server completed the scan', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(async () => { + return jsonOk({ results: [] }); + }), + ); + + const body = await grep('TODO'); + expect(body).toEqual({ results: [], timedOut: false }); + }); + + it('does not send fileFilter when it is null or empty', async () => { + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + const url = String(input); + expect(url).not.toContain('fileFilter='); + return jsonOk({ results: [] }); + }); + vi.stubGlobal('fetch', fetchMock); + + for (const fileFilter of ['', null] as const) { + await grep('x', undefined, undefined, { fileFilter }); + } + expect(fetchMock).toHaveBeenCalledTimes(2); + }); +}); diff --git a/gitnexus-web/test/unit/grep-tool.test.ts b/gitnexus-web/test/unit/grep-tool.test.ts new file mode 100644 index 000000000..8701ca665 --- /dev/null +++ b/gitnexus-web/test/unit/grep-tool.test.ts @@ -0,0 +1,36 @@ +import { describe, expect, it, vi } from 'vitest'; +import { createGraphRAGTools, type GraphRAGBackend } from '../../src/core/llm/tools'; + +const noOpBackend: GraphRAGBackend = { + executeQuery: async () => [], + search: async () => [], + grep: async () => ({ results: [], timedOut: false }), + readFile: async () => '', +}; + +function grepTool(backend: GraphRAGBackend) { + return createGraphRAGTools(backend).find((candidate) => candidate.name === 'grep')!; +} + +describe('grep tool timeout contract', () => { + it('says the scan was incomplete when the server sets timedOut with no hits', async () => { + const grep = vi.fn(async () => ({ results: [], timedOut: true })); + const output = await grepTool({ ...noOpBackend, grep }).invoke({ pattern: 'signOrder' }); + expect(output).toContain('No matches for "signOrder"'); + expect(output).toContain('results may be incomplete'); + }); + + it('still warns when a timed-out scan returned some hits below the limit', async () => { + const grep = vi.fn(async () => ({ + results: [{ filePath: 'a.ts', line: 1, text: 'signOrder()' }], + timedOut: true, + })); + const output = await grepTool({ ...noOpBackend, grep }).invoke({ + pattern: 'signOrder', + maxResults: 100, + }); + expect(output).toContain('Found 1 matches'); + expect(output).toContain('results may be incomplete'); + expect(output).not.toContain('Showing first'); + }); +}); diff --git a/gitnexus-web/test/unit/impact-tool.test.ts b/gitnexus-web/test/unit/impact-tool.test.ts index f04817ed1..704240e6e 100644 --- a/gitnexus-web/test/unit/impact-tool.test.ts +++ b/gitnexus-web/test/unit/impact-tool.test.ts @@ -4,7 +4,7 @@ import { createGraphRAGTools, type GraphRAGBackend } from '../../src/core/llm/to const noOpBackend: GraphRAGBackend = { executeQuery: async () => [], search: async () => [], - grep: async () => [], + grep: async () => ({ results: [], timedOut: false }), readFile: async () => '', }; diff --git a/gitnexus/src/server/api.ts b/gitnexus/src/server/api.ts index fc2e7d943..be156b417 100644 --- a/gitnexus/src/server/api.ts +++ b/gitnexus/src/server/api.ts @@ -54,7 +54,9 @@ import { persistedEmbeddingCountOrUndefined, type PersistedEmbeddingCount, } from '../core/embedding-count.js'; -import { assertString, escapeRegExp, BadRequestError, createRouteLimiter } from './validation.js'; +import { assertString, BadRequestError, createRouteLimiter } from './validation.js'; +import { parseGrepQuery, GREP_TIME_BUDGET_MS } from './grep-params.js'; +import { runGrepScanInWorker } from './grep-scan.js'; import { extractRepoName, getCloneDir, @@ -1337,50 +1339,12 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => res.status(404).json({ error: 'Repository not found' }); return; } - // Type-confusion guard (CodeQL js/type-confusion-through-parameter-tampering): - // req.query.pattern is `string | string[] | ParsedQs` — without an explicit - // type check, the `.length` guard below counts array elements instead of - // characters, allowing arbitrarily long patterns through. - const rawPattern = req.query.pattern; - if (rawPattern === undefined) { - res.status(400).json({ error: 'Missing "pattern" query parameter' }); - return; - } - const pattern = assertString(rawPattern, 'pattern'); - if (pattern.length === 0) { - res.status(400).json({ error: 'Missing "pattern" query parameter' }); - return; - } - - // Length cap: applies to both literal and regex modes as a defense-in-depth - // bound against pathological input. - if (pattern.length > 200) { - res.status(400).json({ error: 'Pattern too long (max 200 characters)' }); - return; - } - - // Treat user input as a literal substring in all cases to prevent - // regex-injection/ReDoS via attacker-controlled regex syntax. - const effectivePattern = escapeRegExp(pattern); - - // Validate regex syntax (catches both opt-in user regex and any escapeRegExp bug) - let regex: RegExp; - try { - regex = new RegExp(effectivePattern, 'gim'); - } catch { - res.status(400).json({ error: 'Invalid regex pattern' }); - return; - } - - const parsedLimit = Number(req.query.limit ?? 50); - const limit = Number.isFinite(parsedLimit) - ? Math.max(1, Math.min(200, Math.trunc(parsedLimit))) - : 50; - - const results: { filePath: string; line: number; text: string }[] = []; + // Pattern parsing lives in grep-params.ts (unit-testable without + // Express + LadybugDB). Matching runs in a worker so terminate() can + // cut a stuck regex.test() when the wall-clock budget expires. + const { regex, fileFilter, limit } = parseGrepQuery(req.query as Record); const repoRoot = path.resolve(entry.path); - // Get file paths from the graph (lightweight — no content loaded) const lbugPath = path.join(entry.storagePath, 'lbug'); const fileRows = await withLbugDb( lbugPath, @@ -1389,34 +1353,23 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => { readOnly: true }, ); - // Search files on disk one at a time (constant memory) + const filePaths: string[] = []; for (const row of fileRows) { - if (results.length >= limit) break; const filePath: string = row.filePath || ''; - const fullPath = path.resolve(repoRoot, filePath); - - // Path traversal guard - const safeRepoRoot = repoRoot.endsWith(path.sep) ? repoRoot : repoRoot + path.sep; - if (!fullPath.startsWith(safeRepoRoot) && fullPath !== repoRoot) continue; - - let content: string; - try { - content = await fs.readFile(fullPath, 'utf-8'); - } catch { - continue; // File may have been deleted since indexing - } - - const lines = content.split('\n'); - for (let i = 0; i < lines.length; i++) { - if (results.length >= limit) break; - if (regex.test(lines[i])) { - results.push({ filePath, line: i + 1, text: lines[i].trim().slice(0, 200) }); - } - regex.lastIndex = 0; - } + if (fileFilter && !filePath.toLowerCase().includes(fileFilter)) continue; + filePaths.push(filePath); } - res.json({ results }); + const { results, timedOut } = await runGrepScanInWorker({ + repoRoot, + filePaths, + pattern: regex.source, + flags: regex.flags, + limit, + deadlineMs: Date.now() + GREP_TIME_BUDGET_MS, + }); + + res.json({ results, ...(timedOut ? { timedOut: true } : {}) }); } catch (err: any) { res.status(statusFromError(err)).json({ error: err.message || 'Grep failed' }); } diff --git a/gitnexus/src/server/grep-params.ts b/gitnexus/src/server/grep-params.ts new file mode 100644 index 000000000..4707b5b96 --- /dev/null +++ b/gitnexus/src/server/grep-params.ts @@ -0,0 +1,98 @@ +/** + * Query-parameter parsing for GET /api/grep. + * + * Extracted from api.ts so the contract is unit-testable without pulling + * Express + the LadybugDB native adapter into the test run (same rationale + * as the #2790 helper extraction). + * + * Contract fix (Patch 12): the grep tool schema in gitnexus-web has always + * promised regex search with an optional path-substring filter and + * case-sensitivity control, but the handler used to escapeRegExp() every + * pattern into a literal substring — an agent asking for "sign|Sign" got + * zero hits and concluded the code didn't exist, and the schema's own + * example ("console\.log") could never match. This restores the promised + * semantics. Matching runs in a worker_threads worker (`grep-worker.ts`) so + * `terminate()` can interrupt a catastrophic `regex.test()` when the + * wall-clock budget expires; the parent event loop stays responsive. + * Bounded mitigations: + * - pattern length cap (200 chars, unchanged from the literal-only era) + * - line-by-line matching (each regex.test call sees one source line) + * - a wall-clock budget the parent enforces via worker terminate() + * - result cap unchanged (limit, max 200) + * - literal=1 opt-out restores the old escaped-substring immunity + */ +import { assertString, escapeRegExp, BadRequestError } from './validation.js'; + +/** Hard cap on pattern length — unchanged from the literal-only era. */ +export const GREP_PATTERN_MAX_LENGTH = 200; + +/** Wall-clock budget for one /api/grep call; parent terminate()s the scan worker. */ +export const GREP_TIME_BUDGET_MS = 5_000; + +export const GREP_DEFAULT_LIMIT = 50; +export const GREP_MAX_LIMIT = 200; + +export interface ParsedGrepQuery { + regex: RegExp; + /** Lowercased path substring; '' disables path filtering. */ + fileFilter: string; + limit: number; +} + +const isFlagTrue = (value: unknown, name: string): boolean => { + const s = assertString(value ?? '', name).toLowerCase(); + return s === '1' || s === 'true'; +}; + +/** + * Parse /api/grep query parameters into a ready-to-use regex + filters. + * Throws BadRequestError (mapped to HTTP 400 by statusFromError) on + * missing/over-long patterns or invalid regex syntax. + */ +export function parseGrepQuery(query: Record): ParsedGrepQuery { + if (query.pattern === undefined) { + throw new BadRequestError('Missing "pattern" query parameter'); + } + const pattern = assertString(query.pattern, 'pattern'); + if (pattern.length === 0) { + throw new BadRequestError('Missing "pattern" query parameter'); + } + if (pattern.length > GREP_PATTERN_MAX_LENGTH) { + throw new BadRequestError(`Pattern too long (max ${GREP_PATTERN_MAX_LENGTH} characters)`); + } + + // Regex semantics by default — what the tool schema always promised. + // literal=1 opts back into the escaped-substring behaviour of the + // literal-only era for callers that want it verbatim. + const caseSensitive = isFlagTrue(query.caseSensitive, 'caseSensitive'); + const flags = caseSensitive ? '' : 'i'; + + let regex: RegExp; + try { + // Deliberately no 'g' flag: the handler tests line-by-line and a + // stateful lastIndex across lines would skip matches (the old handler + // had to reset it manually). No 'm' either: each test receives a + // single line, so ^/$ already anchor at string boundaries — 'm' + // would be a no-op. + if (isFlagTrue(query.literal, 'literal')) { + regex = new RegExp(escapeRegExp(pattern), flags); + } else { + // Intentional: /api/grep advertises real regex (see file header + SECURITY.md). + // ReDoS is mitigated by running the scan in a worker and terminate()-ing it. + // codeql[js/regex-injection] + regex = new RegExp(pattern, flags); + } + } catch { + throw new BadRequestError('Invalid regex pattern'); + } + + // Path-substring filter, case-insensitive ("Controller.java", "src/api"). + const fileFilter = assertString(query.fileFilter ?? '', 'fileFilter').toLowerCase(); + + const parsedLimit = Number(query.limit ?? GREP_DEFAULT_LIMIT); + const limit = Number.isFinite(parsedLimit) + ? Math.max(1, Math.min(GREP_MAX_LIMIT, Math.trunc(parsedLimit))) + : GREP_DEFAULT_LIMIT; + + return { regex, fileFilter, limit }; +} diff --git a/gitnexus/src/server/grep-scan.ts b/gitnexus/src/server/grep-scan.ts new file mode 100644 index 000000000..e2befd9ce --- /dev/null +++ b/gitnexus/src/server/grep-scan.ts @@ -0,0 +1,139 @@ +/** + * Filesystem grep scan used by GET /api/grep. + * + * Matching runs in a worker_threads worker (see grep-worker.ts) so a + * catastrophic `regex.test()` can be killed with terminate() when the + * wall-clock budget expires. The parent event loop stays responsive. + */ +import fs from 'node:fs/promises'; +import path from 'node:path'; +import { createRequire } from 'node:module'; +import { fileURLToPath, pathToFileURL } from 'node:url'; +import { Worker } from 'node:worker_threads'; + +export interface GrepHit { + filePath: string; + line: number; + text: string; +} + +export interface GrepScanInput { + repoRoot: string; + /** Repo-relative paths already filtered by fileFilter. */ + filePaths: string[]; + pattern: string; + flags: string; + limit: number; + /** Absolute Date.now() deadline. */ + deadlineMs: number; +} + +export interface GrepScanResult { + results: GrepHit[]; + timedOut: boolean; +} + +export type GrepProgress = (partial: GrepScanResult) => void; + +export async function scanGrepFiles( + input: GrepScanInput, + onProgress?: GrepProgress, +): Promise { + const regex = new RegExp(input.pattern, input.flags); + const results: GrepHit[] = []; + const repoRoot = path.resolve(input.repoRoot); + const safeRepoRoot = repoRoot.endsWith(path.sep) ? repoRoot : repoRoot + path.sep; + + let timedOut = false; + files: for (const filePath of input.filePaths) { + if (results.length >= input.limit) break; + if (Date.now() > input.deadlineMs) { + timedOut = true; + break; + } + const fullPath = path.resolve(repoRoot, filePath); + if (!fullPath.startsWith(safeRepoRoot) && fullPath !== repoRoot) continue; + + let content: string; + try { + content = await fs.readFile(fullPath, 'utf-8'); + } catch { + continue; + } + + const lines = content.split('\n'); + for (let i = 0; i < lines.length; i++) { + if (results.length >= input.limit) break files; + if ((i & 255) === 0 && Date.now() > input.deadlineMs) { + timedOut = true; + break files; + } + regex.lastIndex = 0; + if (regex.test(lines[i])) { + results.push({ filePath, line: i + 1, text: lines[i].trim().slice(0, 200) }); + } + } + onProgress?.({ results: results.slice(), timedOut }); + } + + return { results, timedOut }; +} + +const _require = createRequire(import.meta.url); + +export function grepWorkerPath(): string { + const callerPath = fileURLToPath(import.meta.url); + const isDev = callerPath.endsWith('.ts'); + return path.join(path.dirname(callerPath), isDev ? 'grep-worker.ts' : 'grep-worker.js'); +} + +export function runGrepScanInWorker(input: GrepScanInput): Promise { + const callerPath = fileURLToPath(import.meta.url); + const isDev = callerPath.endsWith('.ts'); + const tsxHookArgs: string[] = isDev + ? ['--import', pathToFileURL(_require.resolve('tsx/esm')).href] + : []; + + return new Promise((resolve, reject) => { + let settled = false; + let latest: GrepScanResult = { results: [], timedOut: false }; + const worker = new Worker(grepWorkerPath(), { + workerData: input, + execArgv: tsxHookArgs, + }); + + const finish = (result: GrepScanResult) => { + if (settled) return; + settled = true; + clearTimeout(timer); + void worker.terminate(); + resolve(result); + }; + + const remain = Math.max(1, input.deadlineMs - Date.now()); + const timer = setTimeout(() => { + finish({ results: latest.results, timedOut: true }); + }, remain); + + worker.on('message', (msg: { type: string } & GrepScanResult) => { + if (msg.type === 'progress') { + latest = { results: msg.results, timedOut: msg.timedOut }; + return; + } + if (msg.type === 'done') { + finish({ results: msg.results, timedOut: msg.timedOut }); + } + }); + worker.on('error', (err) => { + if (settled) return; + settled = true; + clearTimeout(timer); + void worker.terminate(); + reject(err); + }); + worker.on('exit', () => { + if (settled) return; + finish({ results: latest.results, timedOut: true }); + }); + }); +} diff --git a/gitnexus/src/server/grep-worker.ts b/gitnexus/src/server/grep-worker.ts new file mode 100644 index 000000000..3014b8b47 --- /dev/null +++ b/gitnexus/src/server/grep-worker.ts @@ -0,0 +1,16 @@ +import { parentPort, workerData } from 'node:worker_threads'; +import type { GrepScanInput } from './grep-scan.js'; + +if (!parentPort) { + throw new Error('grep-worker must run as a worker_threads worker'); +} + +const ext = import.meta.url.endsWith('.ts') ? '.ts' : '.js'; +const { scanGrepFiles } = await import(new URL(`./grep-scan${ext}`, import.meta.url).href); + +const port = parentPort; +const input = workerData as GrepScanInput; +const out = await scanGrepFiles(input, (partial) => { + port.postMessage({ type: 'progress', ...partial }); +}); +port.postMessage({ type: 'done', ...out }); diff --git a/gitnexus/test/unit/grep-params.test.ts b/gitnexus/test/unit/grep-params.test.ts new file mode 100644 index 000000000..2e5fd48fa --- /dev/null +++ b/gitnexus/test/unit/grep-params.test.ts @@ -0,0 +1,160 @@ +/** + * Unit Tests: /api/grep query parsing (gitnexus/src/server/grep-params.ts) + * + * Patch 12 contract fix — the grep tool schema promised regex + + * fileFilter + caseSensitive, but the handler escaped every pattern into + * a literal substring. These tests pin the restored semantics: + * - regex is real regex (alternation, classes, quantifiers work) + * - literal=1 keeps the old escaped-substring behaviour opt-in + * - fileFilter normalizes to a lowercase path substring + * - caseSensitive=1 drops the 'i' flag + * - malformed input throws BadRequestError (→ 400 via statusFromError) + */ +import { describe, it, expect } from 'vitest'; +import fs from 'node:fs/promises'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { + parseGrepQuery, + GREP_PATTERN_MAX_LENGTH, + GREP_DEFAULT_LIMIT, + GREP_MAX_LIMIT, +} from '../../src/server/grep-params.js'; +import { BadRequestError } from '../../src/server/validation.js'; + +describe('parseGrepQuery — regex semantics (restored contract)', () => { + it('honours alternation, the case the agent was burned by', () => { + const { regex } = parseGrepQuery({ pattern: 'sign|Sign' }); + expect(regex.test('signOrder(order)')).toBe(true); + expect(regex.test('SignOffOrderAfterDecorator')).toBe(true); + expect(regex.test('assign a value')).toBe(true); // substring semantics, like grep + expect(regex.test('unrelated line')).toBe(false); + }); + + it('supports the schema example "console\\.log" against real code lines', () => { + const { regex } = parseGrepQuery({ pattern: 'console\\.log' }); + expect(regex.test('console.log("hi")')).toBe(true); + expect(regex.test('consolexlog("hi")')).toBe(false); + }); + + it('is case-insensitive by default', () => { + const { regex } = parseGrepQuery({ pattern: 'todo' }); + expect(regex.test('TODO: fix me')).toBe(true); + }); + + it('caseSensitive=1 drops the i flag', () => { + const { regex } = parseGrepQuery({ pattern: 'todo', caseSensitive: '1' }); + expect(regex.test('TODO: fix me')).toBe(false); + expect(regex.test('todo: fix me')).toBe(true); + }); + + it('caseSensitive=true is accepted alongside the bare flag form', () => { + const { regex } = parseGrepQuery({ pattern: 'todo', caseSensitive: 'true' }); + expect(regex.test('TODO')).toBe(false); + }); + + it('literal=1 restores the old escaped-substring behaviour', () => { + const literal = parseGrepQuery({ pattern: 'a.b', literal: '1' }); + expect(literal.regex.test('a.b')).toBe(true); + expect(literal.regex.test('axb')).toBe(false); + + const regexMode = parseGrepQuery({ pattern: 'a.b' }); + expect(regexMode.regex.test('axb')).toBe(true); + }); + + it('matches CJK identifiers/comments (the primary consumer is a Chinese-codebase team)', () => { + const { regex } = parseGrepQuery({ pattern: '签署|signOrder' }); + expect(regex.test('public void signOrder() { // 医嘱签署')).toBe(true); + expect(regex.test('// 签署接口')).toBe(true); + expect(regex.test('// 撤销接口')).toBe(false); + }); + + it('anchors ^/$ at line boundaries — the handler tests one line at a time, so "m" semantics are irrelevant', () => { + const { regex } = parseGrepQuery({ pattern: '^import .*$' }); + expect(regex.test('import path from "path";')).toBe(true); + expect(regex.test(' import path from "path";')).toBe(false); + }); +}); + +describe('parseGrepQuery — fileFilter', () => { + it('normalizes to a lowercase path substring', () => { + const { fileFilter } = parseGrepQuery({ pattern: 'x', fileFilter: 'Controller.JAVA' }); + expect(fileFilter).toBe('controller.java'); + }); + + it('empty / missing fileFilter disables path filtering', () => { + expect(parseGrepQuery({ pattern: 'x' }).fileFilter).toBe(''); + expect(parseGrepQuery({ pattern: 'x', fileFilter: '' }).fileFilter).toBe(''); + }); + + it('array-form fileFilter is type-confusion-rejected, not partially read', () => { + expect(() => parseGrepQuery({ pattern: 'x', fileFilter: ['a', 'b'] })).toThrow(BadRequestError); + }); +}); + +describe('parseGrepQuery — limit clamping', () => { + it('clamps above the max', () => { + expect(parseGrepQuery({ pattern: 'x', limit: '5000' }).limit).toBe(GREP_MAX_LIMIT); + }); + + it('clamps below 1', () => { + expect(parseGrepQuery({ pattern: 'x', limit: '-5' }).limit).toBe(1); + }); + + it('falls back to the default on non-numeric input', () => { + expect(parseGrepQuery({ pattern: 'x', limit: 'abc' }).limit).toBe(GREP_DEFAULT_LIMIT); + }); + + it('defaults when absent', () => { + expect(parseGrepQuery({ pattern: 'x' }).limit).toBe(GREP_DEFAULT_LIMIT); + }); +}); + +describe('parseGrepQuery — error paths (→ 400 via statusFromError)', () => { + it('rejects a missing pattern', () => { + expect(() => parseGrepQuery({})).toThrow(/Missing "pattern" query parameter/); + expect(() => parseGrepQuery({ pattern: '' })).toThrow(/Missing "pattern" query parameter/); + }); + + it('rejects array-form patterns (type-confusion guard unchanged)', () => { + try { + parseGrepQuery({ pattern: ['a', 'b'] }); + expect.unreachable(); + } catch (err) { + expect(err).toBeInstanceOf(BadRequestError); + expect((err as BadRequestError).status).toBe(400); + expect((err as Error).message).toContain('pattern'); + } + }); + + it('rejects over-long patterns at the same 200-char cap', () => { + const long = 'a'.repeat(GREP_PATTERN_MAX_LENGTH + 1); + expect(() => parseGrepQuery({ pattern: long })).toThrow(/max 200 characters/); + expect(() => parseGrepQuery({ pattern: 'a'.repeat(GREP_PATTERN_MAX_LENGTH) })).not.toThrow(); + }); + + it('rejects syntactically invalid regex', () => { + expect(() => parseGrepQuery({ pattern: '((' })).toThrow(/Invalid regex/); + }); +}); + +describe('/api/grep handler wiring (source-level, api-readonly-wiring.test.ts style)', () => { + const readSource = async () => fs.readFile(SRC_PATH, 'utf-8'); + const SRC_PATH = path.join( + path.dirname(fileURLToPath(import.meta.url)), + '../../src/server/api.ts', + ); + + it('threaded through: parseGrepQuery call, worker scan, fileFilter filter, timedOut flag', async () => { + const source = await readSource(); + const grepSection = source.match(/app\.get\('\/api\/grep'[\s\S]*?\n \}\);/); + expect(grepSection).not.toBeNull(); + const section = grepSection![0]; + expect(section).toContain('parseGrepQuery('); + expect(section).toContain('GREP_TIME_BUDGET_MS'); + expect(section).toContain('runGrepScanInWorker('); + expect(section).toMatch(/filePath\.toLowerCase\(\)\.includes\(fileFilter\)/); + expect(section).toContain('timedOut: true'); + expect(section).toContain('readOnly: true'); // unchanged read-only DB open + }); +}); diff --git a/gitnexus/test/unit/grep-scan.test.ts b/gitnexus/test/unit/grep-scan.test.ts new file mode 100644 index 000000000..7beaa0df5 --- /dev/null +++ b/gitnexus/test/unit/grep-scan.test.ts @@ -0,0 +1,101 @@ +import { afterEach, describe, it, expect } from 'vitest'; +import fs from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; +import { runGrepScanInWorker, scanGrepFiles } from '../../src/server/grep-scan.js'; + +const tempDirs: string[] = []; +const tempFiles: string[] = []; + +const mkTempDir = async (prefix: string): Promise => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), prefix)); + tempDirs.push(dir); + return dir; +}; + +afterEach(async () => { + await Promise.all(tempFiles.splice(0).map((f) => fs.unlink(f).catch(() => undefined))); + await Promise.all( + tempDirs + .splice(0) + .map((d) => fs.rm(d, { recursive: true, force: true }).catch(() => undefined)), + ); +}); + +describe('scanGrepFiles', () => { + it('matches regex lines and skips an existing file outside the repo root', async () => { + const dir = await mkTempDir('grep-scan-'); + const outside = path.join(path.dirname(dir), `outside-${path.basename(dir)}.ts`); + tempFiles.push(outside); + await fs.writeFile(path.join(dir, 'hit.ts'), 'signOrder()\nnoop\n', 'utf-8'); + await fs.writeFile(outside, 'outsideHit()\n', 'utf-8'); + const out = await scanGrepFiles({ + repoRoot: dir, + filePaths: ['hit.ts', `../${path.basename(outside)}`], + pattern: 'outsideHit|signOrder', + flags: 'i', + limit: 10, + deadlineMs: Date.now() + 5_000, + }); + expect(out.timedOut).toBe(false); + expect(out.results).toEqual([{ filePath: 'hit.ts', line: 1, text: 'signOrder()' }]); + }); + + it('does not skip later lines when the regex is global', async () => { + const dir = await mkTempDir('grep-gflag-'); + await fs.writeFile(path.join(dir, 'g.ts'), 'aa\naa\n', 'utf-8'); + const out = await scanGrepFiles({ + repoRoot: dir, + filePaths: ['g.ts'], + pattern: 'a', + flags: 'g', + limit: 10, + deadlineMs: Date.now() + 5_000, + }); + expect(out.results.map((r) => r.line)).toEqual([1, 2]); + }); +}); + +describe('runGrepScanInWorker', () => { + it('terminates a catastrophic regex before it blocks the parent', async () => { + const dir = await mkTempDir('grep-redos-'); + // `(a+)+b` against a long run of `a` backtracks; V8 finishes `(a+)+$` instantly. + await fs.writeFile(path.join(dir, 'bait.ts'), `${'a'.repeat(28)}\n`, 'utf-8'); + let ticks = 0; + const pulse = setInterval(() => { + ticks += 1; + }, 20); + const started = Date.now(); + try { + const out = await runGrepScanInWorker({ + repoRoot: dir, + filePaths: ['bait.ts'], + pattern: '(a+)+b', + flags: '', + limit: 10, + deadlineMs: Date.now() + 250, + }); + const elapsed = Date.now() - started; + expect(out.timedOut).toBe(true); + expect(elapsed).toBeLessThan(4_000); + expect(ticks).toBeGreaterThan(3); + } finally { + clearInterval(pulse); + } + }, 8_000); + + it('returns ordinary matches from the worker', async () => { + const dir = await mkTempDir('grep-ok-'); + await fs.writeFile(path.join(dir, 'a.ts'), 'console.log("hi")\n', 'utf-8'); + const out = await runGrepScanInWorker({ + repoRoot: dir, + filePaths: ['a.ts'], + pattern: 'console\\.log', + flags: '', + limit: 10, + deadlineMs: Date.now() + 5_000, + }); + expect(out.timedOut).toBe(false); + expect(out.results).toEqual([{ filePath: 'a.ts', line: 1, text: 'console.log("hi")' }]); + }); +});