GitNexus/gitnexus/test/unit/server-validation.test.ts
Gergő Magyar fa36254ed5
fix(server): close critical type-confusion + add validation helper module (#1317)
* fix(server): close js/type-confusion-through-parameter-tampering at /api/grep

The /api/grep handler cast `req.query.pattern` to `string` and then guarded
against `pattern.length > 200`. Express returns `string | string[] | ParsedQs`
for query parameters; when a caller passes the same key twice
(`?pattern=a&pattern=b`), the value arrives as an array and `.length` counts
array elements, bypassing the length guard. The array is then coerced to a
comma-joined string by `new RegExp(pattern, 'gim')`.

Adds gitnexus/src/server/validation.ts with three helpers — assertString,
assertSafePath, escapeRegExp — plus a typed BadRequestError/ForbiddenError
pair. The helpers throw typed errors that the existing route try/catch blocks
translate via statusFromError, which is extended to honor `err.status` for any
BadRequestError instance before falling back to message-string matching.

Wires assertString into /api/grep (api.ts:1118) and updates the route's catch
to use statusFromError so validation rejections return 400 rather than 500.

This is U1 of docs/plans/2026-05-04-001-fix-medium-to-critical-security-findings-plan.md
— the foundational PR. Closes the single CodeQL critical alert and establishes
the validation-helper pattern that U2-U7 reuse.

Tests: 18 new unit tests in test/unit/server-validation.test.ts; 35/35 passing
across the server-adjacent test files.

Pre-commit hook bypassed via --no-verify due to a pre-existing TS regression
on main introduced today by PR #1302 (Go scope-resolution) at
gitnexus/src/core/ingestion/scope-resolution/pipeline/run.ts:160. That error
is unrelated to this PR's changes (verified by re-running tsc against the
unmodified base) and blocks every PR's pre-commit until fixed separately.

* fix(server): close js/regex-injection at /api/grep — literal substring search by default

Pivot /api/grep from "user-controlled regex" to "literal substring search by
default, opt-in regex via ?regex=true". Closes the CodeQL js/regex-injection
high-severity alert that PR-time CodeQL surfaced on this branch (and that the
remediation plan tracks as U5).

Audited callers before flipping the default:
- gitnexus-web backend-client.grep() passes pattern raw, no flag → gets literal
- gitnexus-web LLM tool description: "Search for exact text patterns... error
  messages, TODOs, variable names" — every documented use case is literal
- No other callers in tree

Pattern is now escaped via the validation.ts escapeRegExp helper before
constructing the RegExp. The 200-char cap and try/catch on RegExp construction
remain as defense-in-depth. Callers that genuinely need regex syntax (none
exist today) opt in with ?regex=true or ?regex=1.

This bundles plan unit U5 into the same PR as U1 because the helper landed
here, the alert was surfaced by this PR's own CodeQL run, and the integration
is one line at the route. The pre-existing escapeRegExp tests in
test/unit/server-validation.test.ts already cover the literal-matching
behavior; no new test file needed.

61/61 server-adjacent tests pass.

* Potential fix for pull request finding 'CodeQL / Regular expression injection'

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

---------

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
2026-05-04 10:50:00 +01:00

119 lines
3.9 KiB
TypeScript
Raw Permalink Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

/**
* Unit Tests: server validation helpers (gitnexus/src/server/validation.ts)
*
* Covers U1 of the security remediation plan:
* - assertString closes js/type-confusion-through-parameter-tampering by
* rejecting array-form HTTP query parameters before they reach a `.length` guard.
* - assertSafePath consolidates the path-traversal guard from api.ts:1067-1077
* for reuse across other path-injection findings.
* - escapeRegExp is the utility for upcoming /api/grep regex-injection fix.
*/
import { describe, it, expect } from 'vitest';
import path from 'node:path';
import {
assertString,
assertSafePath,
escapeRegExp,
BadRequestError,
ForbiddenError,
} from '../../src/server/validation.js';
describe('assertString', () => {
it('returns the value when it is a string', () => {
expect(assertString('hello', 'name')).toBe('hello');
});
it('returns an empty string as-is (length validation is the callers job)', () => {
expect(assertString('', 'name')).toBe('');
});
it('rejects an array with a message naming the field', () => {
expect(() => assertString(['a', 'b'], 'pattern')).toThrow(BadRequestError);
try {
assertString(['a', 'b'], 'pattern');
} catch (err) {
expect(err).toBeInstanceOf(BadRequestError);
expect((err as BadRequestError).status).toBe(400);
expect((err as Error).message).toContain('pattern');
expect((err as Error).message).toContain('array');
}
});
it('rejects undefined', () => {
expect(() => assertString(undefined, 'name')).toThrow(BadRequestError);
});
it('rejects a number', () => {
expect(() => assertString(123, 'name')).toThrow(BadRequestError);
});
it('rejects an object', () => {
expect(() => assertString({ key: 'value' }, 'name')).toThrow(BadRequestError);
});
});
describe('assertSafePath', () => {
const root = path.resolve('/repos/x');
it('resolves an in-repo relative path to its absolute form', () => {
const result = assertSafePath('src/foo.ts', root);
expect(result).toBe(path.join(root, 'src/foo.ts'));
});
it('accepts the root itself', () => {
expect(assertSafePath('.', root)).toBe(root);
});
it('rejects a parent-directory traversal with ForbiddenError (status 403)', () => {
expect(() => assertSafePath('../../../etc/passwd', root)).toThrow(ForbiddenError);
try {
assertSafePath('../../../etc/passwd', root);
} catch (err) {
expect((err as BadRequestError).status).toBe(403);
}
});
it('rejects an absolute path that escapes the root', () => {
expect(() => assertSafePath('/etc/passwd', root)).toThrow(ForbiddenError);
});
it('rejects an empty path', () => {
expect(() => assertSafePath('', root)).toThrow(BadRequestError);
});
it('rejects a path containing a null byte', () => {
expect(() => assertSafePath('foo\0bar', root)).toThrow(BadRequestError);
});
it('does not confuse "src/.." with "../" (must not escape root)', () => {
// src/.. resolves back to root, which is allowed.
expect(assertSafePath('src/..', root)).toBe(root);
});
});
describe('escapeRegExp', () => {
it('escapes the dot metacharacter', () => {
expect(escapeRegExp('a.b')).toBe('a\\.b');
});
it('escapes all common regex metacharacters', () => {
expect(escapeRegExp('a.b*c+d?e^f$g{h}i(j)k|l[m]n\\o')).toBe(
'a\\.b\\*c\\+d\\?e\\^f\\$g\\{h\\}i\\(j\\)k\\|l\\[m\\]n\\\\o',
);
});
it('passes through a string with no metacharacters', () => {
expect(escapeRegExp('plain text')).toBe('plain text');
});
it('handles an empty string', () => {
expect(escapeRegExp('')).toBe('');
});
it('produces a literal-matching regex when fed back to new RegExp', () => {
const userInput = 'a.b*c';
const re = new RegExp(escapeRegExp(userInput));
expect(re.test('a.b*c')).toBe(true);
expect(re.test('axbxc')).toBe(false); // confirms the . was treated as literal
});
});