diff --git a/docker-server.mjs b/docker-server.mjs index adf84a07b..8143eb4b7 100644 --- a/docker-server.mjs +++ b/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) { diff --git a/docker-server.test.mjs b/docker-server.test.mjs index a1005b0e4..0fa84155b 100644 --- a/docker-server.test.mjs +++ b/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'); diff --git a/gitnexus/src/server/api.ts b/gitnexus/src/server/api.ts index 9546c0220..8678f3af9 100644 --- a/gitnexus/src/server/api.ts +++ b/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 => { + 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 diff --git a/gitnexus/test/unit/api-file-route.test.ts b/gitnexus/test/unit/api-file-route.test.ts new file mode 100644 index 000000000..71713ff8b --- /dev/null +++ b/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): 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'); + }); +});