fix(server): close critical type-confusion + add validation helper module

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' });
     }
   });
 

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, '\\$&');
+}

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
+  });
+});