Merge origin/main into feat/multi-repo-unified-graph

docker-server.mjs

@@ -1,11 +1,11 @@
 import { createReadStream } from 'node:fs';
 import { stat } from 'node:fs/promises';
 import { createServer } from 'node:http';
-import { extname, join, normalize, sep } from 'node:path';
+import { extname, isAbsolute, normalize, relative, resolve } from 'node:path';
 
 const host = '0.0.0.0';
 const port = Number(process.env.PORT || '4173');
-const root = join(process.cwd(), 'dist');
+const root = resolve(process.cwd(), 'dist');
 
 const contentTypes = {
   '.css': 'text/css; charset=utf-8',
@@ -20,39 +20,78 @@ const contentTypes = {
   '.woff2': 'font/woff2',
 };
 
-function resolvePath(urlPath) {
+// Static asset server for the gitnexus-web Docker image.
+//
+// Path-injection containment: the request handler is intentionally a single
+// inline pipeline with no helper functions on the path-data flow. Each
+// filesystem sink (stat, createReadStream) is immediately preceded by the
+// canonical `path.relative` containment check that CodeQL's
+// `js/path-injection` query recognizes as a sanitizer barrier:
+//
+//     const rel = relative(root, candidate);
+//     if (rel.startsWith('..') || isAbsolute(rel)) reject;
+//     // candidate is now proven inside `root`
+//
+// Earlier iterations of this file used a helper (`resolveWithinRoot`) and a
+// `startsWith(root + sep)` check. Both were semantically correct but neither
+// was recognized by CodeQL: `startsWith(root + sep)` is not in the analyzer's
+// barrier-pattern set, and helper-based sanitization is not followed across
+// the request handler's reassignment paths in vanilla JS. The inline-at-sink
+// shape below is the documented analyzer-friendly idiom.
+const server = createServer(async (req, res) => {
+  const urlPath = req.url?.split('?')[0] || '/';
+
   let decoded;
   try {
     decoded = decodeURIComponent(urlPath);
   } catch {
-    return null;
+    res.writeHead(400);
+    res.end('Bad request');
+    return;
   }
-  if (decoded.includes('\0')) return null;
+  if (decoded.includes('\0')) {
+    res.writeHead(400);
+    res.end('Bad request');
+    return;
+  }
+
   const cleanPath = normalize(decoded.replace(/^\/+/, ''));
-  const candidate = join(root, cleanPath);
-  if (candidate !== root && !candidate.startsWith(root + sep)) return null;
-  return candidate;
-}
+  const initialPath = resolve(root, cleanPath);
 
-const server = createServer(async (req, res) => {
-  const requestPath = req.url?.split('?')[0] || '/';
-  let filePath = resolvePath(requestPath);
-
-  if (!filePath) {
+  // Sanitizer barrier #1 — guards the first stat() sink.
+  const initialRel = relative(root, initialPath);
+  if (initialRel.startsWith('..') || isAbsolute(initialRel)) {
     res.writeHead(400);
     res.end('Bad request');
     return;
   }
 
   try {
-    const fileStat = await stat(filePath).catch(() => null);
-    if (fileStat?.isDirectory()) {
-      filePath = join(filePath, 'index.html');
-    } else if (!fileStat?.isFile()) {
-      filePath = join(root, 'index.html');
+    const initialStat = await stat(initialPath).catch(() => null);
+
+    // Pick the path we actually serve. Note: any branch reassigns to a
+    // freshly-resolved path; the next sanitizer barrier re-validates.
+    let finalPath;
+    if (initialStat?.isDirectory()) {
+      finalPath = resolve(initialPath, 'index.html');
+    } else if (!initialStat?.isFile()) {
+      finalPath = resolve(root, 'index.html');
+    } else {
+      finalPath = initialPath;
     }
 
-    const finalStat = await stat(filePath).catch(() => null);
+    // Sanitizer barrier #2 — guards both the second stat() and the
+    // createReadStream() sinks. No reassignment of finalPath happens
+    // between this guard and either sink, so the analyzer can prove
+    // containment for both.
+    const finalRel = relative(root, finalPath);
+    if (finalRel.startsWith('..') || isAbsolute(finalRel)) {
+      res.writeHead(400);
+      res.end('Bad request');
+      return;
+    }
+
+    const finalStat = await stat(finalPath).catch(() => null);
     if (!finalStat?.isFile()) {
       res.writeHead(404);
       res.end('Not found');
@@ -60,14 +99,14 @@ const server = createServer(async (req, res) => {
     }
 
     res.writeHead(200, {
-      'Cache-Control': filePath.includes('/assets/')
+      'Cache-Control': finalPath.includes('/assets/')
         ? 'public, max-age=31536000, immutable'
         : 'no-cache',
-      'Content-Type': contentTypes[extname(filePath)] || 'application/octet-stream',
+      'Content-Type': contentTypes[extname(finalPath)] || 'application/octet-stream',
       'Cross-Origin-Opener-Policy': 'same-origin',
       'Cross-Origin-Embedder-Policy': 'require-corp',
     });
-    const stream = createReadStream(filePath);
+    const stream = createReadStream(finalPath);
     stream.on('error', () => res.destroy());
     stream.pipe(res);
   } catch (error) {

docker-server.test.mjs

@@ -100,6 +100,23 @@ it('rejects percent-encoded null bytes with 400', async () => {
   assert.equal(res.status, 400);
 });
 
+it('rejects percent-encoded path traversal with 400', async () => {
+  // %2e%2e%2f decodes to '../'. Without the path.relative inline barrier,
+  // a naive string check on the raw URL would let this through and only
+  // the lexical-decoded path.resolve would catch it. Confirm the barrier
+  // does its job after decodeURIComponent.
+  const res = await rawGet(serverPort, '/%2e%2e%2f%2e%2e%2fetc%2fpasswd');
+  assert.equal(res.status, 400);
+});
+
+it('rejects malformed percent-encoding with 400', async () => {
+  // %GG is not a valid percent-encoded sequence — decodeURIComponent throws.
+  // The handler's try/catch around decode must convert this to a 400 rather
+  // than an unhandled rejection.
+  const res = await rawGet(serverPort, '/foo%GGbar');
+  assert.equal(res.status, 400);
+});
+
 it('returns 404 when dist/index.html is missing', async () => {
   await unlink(join(tmpDir, 'dist', 'index.html'));
   const res = await rawGet(serverPort, '/nonexistent-page');

gitnexus/src/server/api.ts

@@ -527,6 +527,87 @@ const requestedRepo = (req: express.Request): string | undefined => {
   return undefined;
 };
 
+/**
+ * Handle a GET /api/file request body. Extracted from createServer's route
+ * registration so it can be unit-tested without spinning up an HTTP server
+ * — calling app.get(...) inside a test triggers CodeQL's
+ * js/missing-rate-limiting query, which is appropriate for production
+ * route handlers but a false positive for tests of the handler logic.
+ *
+ * The function takes the express req and res (typed loosely so test code
+ * can pass minimal mocks) plus the resolved repo path. All path-traversal
+ * containment is done inline at the readFile sink with the canonical
+ * path.relative idiom for CodeQL js/path-injection recognition.
+ */
+export const handleFileRequest = async (
+  req: { query: any },
+  res: {
+    status: (code: number) => { json: (body: any) => void };
+    json: (body: any) => void;
+  },
+  repoPath: string,
+): Promise<void> => {
+  try {
+    // Type-confusion guard — req.query.path is `string | string[] | ParsedQs`.
+    // Without this, an attacker could pass `?path=a&path=b` to bypass the
+    // length-bound traversal check below (CodeQL js/type-confusion-through-
+    // parameter-tampering, same class as the /api/grep critical fix).
+    const rawFilePath = req.query.path;
+    if (rawFilePath === undefined || rawFilePath === '') {
+      res.status(400).json({ error: 'Missing path' });
+      return;
+    }
+    const filePath = assertString(rawFilePath, 'path');
+
+    // Path-injection containment — inline at the sink with the canonical
+    // path.relative idiom that CodeQL's js/path-injection sanitizer
+    // recognizes. assertSafePath in validation.ts performs the equivalent
+    // check, but cross-module helpers are not followed by CodeQL's
+    // interprocedural analysis for path-traversal sanitization in JS, so
+    // the barrier must be visible inline at the readFile sink.
+    const repoRoot = path.resolve(repoPath);
+    const fullPath = path.resolve(repoRoot, filePath);
+    const fullRel = path.relative(repoRoot, fullPath);
+    if (fullRel.startsWith('..') || path.isAbsolute(fullRel)) {
+      res.status(403).json({ error: 'Path traversal denied' });
+      return;
+    }
+
+    const raw = await fs.readFile(fullPath, 'utf-8');
+
+    // Optional line-range support: ?startLine=10&endLine=50
+    // Returns only the requested slice (0-indexed), plus metadata.
+    const startLine = req.query.startLine !== undefined ? Number(req.query.startLine) : undefined;
+    const endLine = req.query.endLine !== undefined ? Number(req.query.endLine) : undefined;
+
+    if (startLine !== undefined && Number.isFinite(startLine)) {
+      const lines = raw.split('\n');
+      const start = Math.max(0, startLine);
+      const end =
+        endLine !== undefined && Number.isFinite(endLine)
+          ? Math.min(lines.length, endLine + 1)
+          : lines.length;
+      res.json({
+        content: lines.slice(start, end).join('\n'),
+        startLine: start,
+        endLine: end - 1,
+        totalLines: lines.length,
+      });
+    } else {
+      res.json({ content: raw, totalLines: raw.split('\n').length });
+    }
+  } catch (err: any) {
+    if (err.code === 'ENOENT') {
+      res.status(404).json({ error: 'File not found' });
+    } else {
+      // statusFromError returns err.status for BadRequestError / ForbiddenError
+      // (assertString → 400 on array-form ?path=a&path=b; ForbiddenError → 403
+      // on traversal). Falls back to 500 for unrecognized failures.
+      res.status(statusFromError(err)).json({ error: err.message || 'Failed to read file' });
+    }
+  }
+};
+
 export const createServer = async (port: number, host: string = '127.0.0.1') => {
   const app = express();
   app.disable('x-powered-by');
@@ -1179,56 +1260,12 @@ export const createServer = async (port: number, host: string = '127.0.0.1') =>
 
   // Read file — with path traversal guard
   app.get('/api/file', async (req, res) => {
-    try {
-      const entry = await resolveRepo(requestedRepo(req));
-      if (!entry) {
-        res.status(404).json({ error: 'Repository not found' });
-        return;
-      }
-      const filePath = req.query.path as string;
-      if (!filePath) {
-        res.status(400).json({ error: 'Missing path' });
-        return;
-      }
-
-      // Prevent path traversal — resolve and verify the path stays within the repo root
-      const repoRoot = path.resolve(entry.path);
-      const fullPath = path.resolve(repoRoot, filePath);
-      if (!fullPath.startsWith(repoRoot + path.sep) && fullPath !== repoRoot) {
-        res.status(403).json({ error: 'Path traversal denied' });
-        return;
-      }
-
-      const raw = await fs.readFile(fullPath, 'utf-8');
-
-      // Optional line-range support: ?startLine=10&endLine=50
-      // Returns only the requested slice (0-indexed), plus metadata.
-      const startLine = req.query.startLine !== undefined ? Number(req.query.startLine) : undefined;
-      const endLine = req.query.endLine !== undefined ? Number(req.query.endLine) : undefined;
-
-      if (startLine !== undefined && Number.isFinite(startLine)) {
-        const lines = raw.split('\n');
-        const start = Math.max(0, startLine);
-        const end =
-          endLine !== undefined && Number.isFinite(endLine)
-            ? Math.min(lines.length, endLine + 1)
-            : lines.length;
-        res.json({
-          content: lines.slice(start, end).join('\n'),
-          startLine: start,
-          endLine: end - 1,
-          totalLines: lines.length,
-        });
-      } else {
-        res.json({ content: raw, totalLines: raw.split('\n').length });
-      }
-    } catch (err: any) {
-      if (err.code === 'ENOENT') {
-        res.status(404).json({ error: 'File not found' });
-      } else {
-        res.status(500).json({ error: err.message || 'Failed to read file' });
-      }
+    const entry = await resolveRepo(requestedRepo(req));
+    if (!entry) {
+      res.status(404).json({ error: 'Repository not found' });
+      return;
     }
+    await handleFileRequest(req, res, entry.path);
   });
 
   // Grep — regex search across file contents in the indexed repo

gitnexus/test/unit/api-file-route.test.ts

@@ -0,0 +1,130 @@
+/**
+ * Unit tests for the /api/file handler — handleFileRequest.
+ *
+ * Calls the handler directly with mock req/res rather than mounting it on
+ * an Express app and binding a port. This is an intentional design choice:
+ *   - Mounting the handler via app.get(...) inside a test triggers CodeQL's
+ *     js/missing-rate-limiting query, which is correct for production
+ *     route handlers but a false positive on tests of the handler logic.
+ *   - Direct invocation also runs faster (no port allocation, no listen)
+ *     and exercises the same code path used in production via createServer.
+ *
+ * Covers the gaps the PR #1322 review identified:
+ *   - `?path=a&path=b` (array form) returns 400, not 500 — proves the catch
+ *     block correctly routes BadRequestError via statusFromError.
+ *   - `?path=../../../etc/passwd` returns 403 — traversal rejection.
+ *   - `?path=%2e%2e%2fsecret` (encoded traversal) returns 403 — Express
+ *     decodes the query string before the handler sees it.
+ *   - Valid relative path returns 200 with file content.
+ *   - Missing path returns 400.
+ *   - Common-prefix sibling escape returns 403 (the path.relative idiom
+ *     catches what startsWith(root + sep) would have missed).
+ */
+import { afterAll, beforeAll, describe, expect, it } from 'vitest';
+import path from 'node:path';
+import fs from 'node:fs/promises';
+import os from 'node:os';
+import { handleFileRequest } from '../../src/server/api.js';
+
+let tmpRoot: string;
+
+beforeAll(async () => {
+  tmpRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'gitnexus-api-file-test-'));
+  await fs.writeFile(path.join(tmpRoot, 'hello.txt'), 'hello world\n', 'utf-8');
+  await fs.mkdir(path.join(tmpRoot, 'sub'), { recursive: true });
+  await fs.writeFile(path.join(tmpRoot, 'sub', 'nested.txt'), 'nested\n', 'utf-8');
+});
+
+afterAll(async () => {
+  await fs.rm(tmpRoot, { recursive: true, force: true });
+});
+
+// Minimal express-shaped mock that captures status() / json() calls in a
+// shape compatible with the handler's expected interface. Returns the
+// final status (default 200 for naked res.json) and JSON body.
+const invoke = async (query: Record<string, unknown>): Promise<{ status: number; body: any }> => {
+  let capturedStatus = 200;
+  let capturedBody: any = undefined;
+  const res = {
+    status(code: number) {
+      capturedStatus = code;
+      return this;
+    },
+    json(body: any) {
+      capturedBody = body;
+    },
+  };
+  await handleFileRequest({ query }, res, tmpRoot);
+  return { status: capturedStatus, body: capturedBody };
+};
+
+describe('handleFileRequest — security wiring', () => {
+  it('returns 200 with content for a valid relative path', async () => {
+    const { status, body } = await invoke({ path: 'hello.txt' });
+    expect(status).toBe(200);
+    expect(body.content).toBe('hello world\n');
+  });
+
+  it('returns 200 for a nested valid path', async () => {
+    const { status, body } = await invoke({ path: 'sub/nested.txt' });
+    expect(status).toBe(200);
+    expect(body.content).toBe('nested\n');
+  });
+
+  it('returns 400 when path is missing', async () => {
+    const { status, body } = await invoke({});
+    expect(status).toBe(400);
+    expect(body.error).toBe('Missing path');
+  });
+
+  it('returns 400 when path is an empty string', async () => {
+    const { status, body } = await invoke({ path: '' });
+    expect(status).toBe(400);
+    expect(body.error).toBe('Missing path');
+  });
+
+  // Reproducer for the PR #1322 review's HIGH finding #1.
+  // Before the catch-block fix this returned 500.
+  it('returns 400 when path is an array (?path=a&path=b)', async () => {
+    const { status, body } = await invoke({ path: ['a', 'b'] });
+    expect(status).toBe(400);
+    expect(body.error).toContain('path');
+    expect(body.error).toContain('array');
+  });
+
+  it('returns 403 for parent-directory traversal', async () => {
+    const { status, body } = await invoke({ path: '../../../etc/passwd' });
+    expect(status).toBe(403);
+    expect(body.error).toBe('Path traversal denied');
+  });
+
+  it('returns 403 for already-decoded traversal segments', async () => {
+    // Express decodes the query string before the handler sees it, so the
+    // analogue of a percent-encoded `%2e%2e%2f` arrives at the handler as
+    // '../'. Confirm the barrier still rejects.
+    const { status, body } = await invoke({ path: '../etc/passwd' });
+    expect(status).toBe(403);
+    expect(body.error).toBe('Path traversal denied');
+  });
+
+  it('returns 403 for an absolute path that escapes the root', async () => {
+    const { status, body } = await invoke({ path: '/etc/passwd' });
+    expect(status).toBe(403);
+    expect(body.error).toBe('Path traversal denied');
+  });
+
+  it('returns 404 for a path that resolves inside root but does not exist', async () => {
+    const { status, body } = await invoke({ path: 'does-not-exist.txt' });
+    expect(status).toBe(404);
+    expect(body.error).toBe('File not found');
+  });
+
+  it('rejects a common-prefix sibling directory escape (path.relative idiom)', async () => {
+    // The classic pitfall of `startsWith(root + sep)` is that '/tmp/repo' does
+    // not catch '/tmp/repo-evil/x'. The path.relative idiom does.
+    const sibling = path.basename(tmpRoot) + '-evil/secret';
+    const { status, body } = await invoke({ path: `../${sibling}` });
+    expect(status).toBe(403);
+    expect(body.error).toBe('Path traversal denied');
+  });
+});