diff --git a/gitnexus/src/server/api.ts b/gitnexus/src/server/api.ts index d9aa00ae6c..1202d6fefe 100644 --- a/gitnexus/src/server/api.ts +++ b/gitnexus/src/server/api.ts @@ -33,6 +33,7 @@ import { mountMCPEndpoints } from './mcp-http.js'; import { fork } from 'child_process'; import { fileURLToPath, pathToFileURL } from 'url'; import { JobManager } from './analyze-job.js'; +import { assertString, escapeRegExp, BadRequestError } from './validation.js'; import { extractRepoName, getCloneDir, cloneOrPull } from './git-clone.js'; const _require = createRequire(import.meta.url); @@ -506,6 +507,9 @@ const mountSSEProgress = (app: express.Express, routePath: string, jm: JobManage }; const statusFromError = (err: any): number => { + // Validation helpers throw BadRequestError / ForbiddenError with a typed + // .status field — honor it before falling back to message-string matching. + if (err instanceof BadRequestError) return err.status; const msg = String(err?.message ?? ''); if (msg.includes('No indexed repositories') || msg.includes('not found')) return 404; if (msg.includes('Multiple repositories')) return 400; @@ -1108,22 +1112,36 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => res.status(404).json({ error: 'Repository not found' }); return; } - const pattern = req.query.pattern as string; - if (!pattern) { + // 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; } - // ReDoS protection: reject overly long or dangerous patterns + // 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; } - // Validate regex syntax + // 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(pattern, 'gim'); + regex = new RegExp(effectivePattern, 'gim'); } catch { res.status(400).json({ error: 'Invalid regex pattern' }); return; @@ -1171,7 +1189,7 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => res.json({ results }); } catch (err: any) { - res.status(500).json({ error: err.message || 'Grep failed' }); + res.status(statusFromError(err)).json({ error: err.message || 'Grep failed' }); } }); diff --git a/gitnexus/src/server/validation.ts b/gitnexus/src/server/validation.ts new file mode 100644 index 0000000000..2954aa658b --- /dev/null +++ b/gitnexus/src/server/validation.ts @@ -0,0 +1,97 @@ +/** + * Server-side input validation helpers. + * + * Convention: helpers throw BadRequestError (or its 403 subclass ForbiddenError) + * when user input fails validation. Existing route handlers wrap their bodies in + * try/catch and translate the error to res.status(err.status).json({error: err.message}). + * This pattern was chosen over an asyncHandler middleware to stay compatible with + * Express 4's non-propagation of async-thrown errors and to match the existing + * try/catch shape used throughout api.ts. + * + * Scope (this PR — U1 of the security remediation plan): + * - assertString: closes js/type-confusion-through-parameter-tampering (api.ts:1118) + * - assertSafePath: consolidates the path-traversal guard from api.ts:1067-1077 + * for reuse across other path-injection findings (U2/U3) + * - escapeRegExp: utility for upcoming regex-injection fix at /api/grep (U5) + * + * Helpers added in later units (U3 git-clone hardening, U4 rate-limiting) live + * in this module too but are introduced with the dependency they require. + */ + +import path from 'node:path'; + +/** + * Thrown by validation helpers when user input is rejected. + * Routes catch via existing try/catch and convert with err.status / err.message. + */ +export class BadRequestError extends Error { + readonly status: number; + constructor(message: string, status = 400) { + super(message); + this.name = 'BadRequestError'; + this.status = status; + } +} + +export class ForbiddenError extends BadRequestError { + constructor(message: string) { + super(message, 403); + this.name = 'ForbiddenError'; + } +} + +/** + * Type guard for HTTP request parameters that must be a single string. + * + * Express's req.query and req.body parsers return `string | string[] | ParsedQs` + * for any field, but route handlers commonly cast to `string` and operate on + * `.length`. When the caller passes the same key twice (?x=a&x=b) the value + * arrives as an array, and a `.length` check intended for the string ends up + * counting array elements — bypassing length-based guards (CodeQL + * js/type-confusion-through-parameter-tampering, alert at api.ts:1118). + * + * @throws BadRequestError when value is not a string (array, object, undefined, etc.) + */ +export function assertString(value: unknown, fieldName: string): string { + if (typeof value !== 'string') { + if (Array.isArray(value)) { + throw new BadRequestError(`Parameter "${fieldName}" must be a single string, got an array`); + } + throw new BadRequestError(`Parameter "${fieldName}" must be a string`); + } + return value; +} + +/** + * Resolve a user-supplied relative path against an allowed root and verify it + * stays inside that root. Mirrors the existing guard at api.ts:1067-1077. + * + * Returns the absolute resolved path. Rejects empty paths, null bytes, and + * paths that resolve outside the root (e.g., `../../../etc/passwd`). + * + * @throws BadRequestError when the path is empty or contains a null byte + * @throws ForbiddenError when the resolved path escapes the root + */ +export function assertSafePath(rawPath: string, root: string): string { + if (rawPath.length === 0) { + throw new BadRequestError('Path must not be empty'); + } + if (rawPath.includes('\0')) { + throw new BadRequestError('Path must not contain null bytes'); + } + const resolvedRoot = path.resolve(root); + const fullPath = path.resolve(resolvedRoot, rawPath); + if (fullPath !== resolvedRoot && !fullPath.startsWith(resolvedRoot + path.sep)) { + throw new ForbiddenError('Path traversal denied'); + } + return fullPath; +} + +/** + * Escape regex metacharacters in a user-supplied string so it can be safely + * embedded as a literal in `new RegExp(...)`. Used by /api/grep's literal mode + * and any future endpoint that constructs a regex from caller input. + */ +export function escapeRegExp(input: string): string { + return input.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} diff --git a/gitnexus/test/unit/server-validation.test.ts b/gitnexus/test/unit/server-validation.test.ts new file mode 100644 index 0000000000..0e61983081 --- /dev/null +++ b/gitnexus/test/unit/server-validation.test.ts @@ -0,0 +1,119 @@ +/** + * 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 caller’s 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 + }); +});