mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-09-07 08:26:11 +00:00
678a0e11c9
* 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 <l.cx@winning.com.cn>
Co-authored-by: ChunxueLi <mecoloud@users.noreply.gitee.com>
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com>
36 lines
1.4 KiB
TypeScript
36 lines
1.4 KiB
TypeScript
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');
|
|
});
|
|
});
|