diff --git a/gitnexus-web/src/services/backend-client.ts b/gitnexus-web/src/services/backend-client.ts index ebc18c17d..ec8c1a964 100644 --- a/gitnexus-web/src/services/backend-client.ts +++ b/gitnexus-web/src/services/backend-client.ts @@ -72,7 +72,19 @@ export class BackendError extends Error { constructor( message: string, public readonly status: number, - public readonly code: 'network' | 'server' | 'client' | 'not_found' | 'timeout', + public readonly code: + | 'network' + | 'server' + | 'client' + | 'not_found' + | 'timeout' + | 'rate_limited', + /** + * Milliseconds until the caller should retry. Populated for rate-limited + * responses (HTTP 429) from the server's `Retry-After` header. `undefined` + * for every other code, including `client` errors that aren't 429. + */ + public readonly retryAfterMs?: number, ) { super(message); this.name = 'BackendError'; @@ -279,10 +291,32 @@ const assertOk = async (response: Response): Promise => { const code = response.status === 404 ? 'not_found' - : response.status >= 400 && response.status < 500 - ? 'client' - : 'server'; - throw new BackendError(message, response.status, code); + : response.status === 429 + ? 'rate_limited' + : response.status >= 400 && response.status < 500 + ? 'client' + : 'server'; + + // Retry-After is the standard HTTP signal for when the client may try again. + // express-rate-limit emits it on 429 with seconds (integer) or HTTP-date. + // We accept both shapes; an unparseable header yields undefined retryAfterMs. + let retryAfterMs: number | undefined; + if (response.status === 429) { + const header = response.headers.get('retry-after'); + if (header) { + const seconds = Number(header); + if (Number.isFinite(seconds) && seconds >= 0) { + retryAfterMs = seconds * 1000; + } else { + const dateMs = Date.parse(header); + if (Number.isFinite(dateMs)) { + retryAfterMs = Math.max(0, dateMs - Date.now()); + } + } + } + } + + throw new BackendError(message, response.status, code, retryAfterMs); }; const repoParam = (repo?: string): string => (repo ? `repo=${encodeURIComponent(repo)}` : ''); diff --git a/gitnexus/package-lock.json b/gitnexus/package-lock.json index 1c4130ca4..7ab6d4511 100644 --- a/gitnexus/package-lock.json +++ b/gitnexus/package-lock.json @@ -18,6 +18,7 @@ "commander": "^14.0.3", "cors": "^2.8.5", "express": "^4.19.2", + "express-rate-limit": "^8.4.1", "glob": "^13.0.6", "graphology": "^0.26.0", "graphology-indices": "^0.17.0", @@ -3017,9 +3018,9 @@ } }, "node_modules/express-rate-limit": { - "version": "8.3.1", - "resolved": "https://registry.npmjs.org/express-rate-limit/-/express-rate-limit-8.3.1.tgz", - "integrity": "sha512-D1dKN+cmyPWuvB+G2SREQDzPY1agpBIcTa9sJxOPMCNeH3gwzhqJRDWCXW3gg0y//+LQ/8j52JbMROWyrKdMdw==", + "version": "8.4.1", + "resolved": "https://registry.npmjs.org/express-rate-limit/-/express-rate-limit-8.4.1.tgz", + "integrity": "sha512-NGVYwQSAyEQgzxX1iCM978PP9AdO/hW93gMcF6ZwQCm+rFvLsBH6w4xcXWTcliS8La5EPRN3p9wzItqBwJrfNw==", "license": "MIT", "dependencies": { "ip-address": "10.1.0" diff --git a/gitnexus/package.json b/gitnexus/package.json index 7ccd10b99..bd4cbe12c 100644 --- a/gitnexus/package.json +++ b/gitnexus/package.json @@ -60,6 +60,7 @@ "commander": "^14.0.3", "cors": "^2.8.5", "express": "^4.19.2", + "express-rate-limit": "^8.4.1", "glob": "^13.0.6", "graphology": "^0.26.0", "graphology-indices": "^0.17.0", diff --git a/gitnexus/src/server/api.ts b/gitnexus/src/server/api.ts index bde3f98e1..d0c9c5577 100644 --- a/gitnexus/src/server/api.ts +++ b/gitnexus/src/server/api.ts @@ -33,7 +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 { assertString, escapeRegExp, BadRequestError, createRouteLimiter } from './validation.js'; import { extractRepoName, getCloneDir, cloneOrPull } from './git-clone.js'; const _require = createRequire(import.meta.url); @@ -217,7 +217,19 @@ export const registerWebUI = (app: express.Express, staticDir: string | null): v // The regex excludes /api paths AND paths with file extensions (.js, .css, etc.) // so missing assets get real 404s instead of the SPA HTML. // Adding routes below this will be unreachable for non-API, non-asset paths. - app.get(SPA_FALLBACK_REGEX, (_req, res) => { + // Rate-limited (CodeQL js/missing-rate-limiting): the SPA fallback + // serves a constant index.html, but the FS access from a route handler + // is enough to trip the analyzer. The limit is generous (300 rpm/IP = + // 5 req/s sustained) so that multi-tab browser navigation, prefetch, + // and service-worker revalidation do not produce 429s for legitimate + // SPA users. At this rate, real browser navigation is extremely + // unlikely to hit the limit in practice, so the cosmetic issue of + // JSON-on-429 to a browser is a low-likelihood path. Content + // negotiation on the 429 (returning the SPA shell to HTML clients + // instead of `{ error: '...' }`) would require swapping + // express-rate-limit's `message` for a `handler` function and is + // deferred to keep this PR focused on closing the CodeQL alert. + app.get(SPA_FALLBACK_REGEX, createRouteLimiter({ limit: 300 }), (_req, res) => { res.sendFile(path.join(staticDir, 'index.html')); }); } else { @@ -612,6 +624,27 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => const app = express(); app.disable('x-powered-by'); + // Trust X-Forwarded-* headers only when the connection comes from the + // local loopback or RFC1918 private/link-local addresses — exactly the + // origins the CORS allowlist accepts. Without this, every request behind + // any reverse proxy / Docker bridge counts as the same `req.ip` and a + // single user can trip the per-IP rate limiter for everyone. + // + // SCOPE: this setting is process-wide. Every middleware and route in this + // Express app sees req.ip resolved from X-Forwarded-For when the upstream + // hop is in the trusted set above — not just the rate-limited routes. + // Future IP-based middleware (audit logging, IP-bound authz) inherits this + // behavior. + // + // CLOUD-DEPLOY CAVEAT: a public cloud LB (AWS ALB, Cloudflare, Fly.io + // edge, CGNAT 100.64/10) is NOT in the trusted set. In those topologies + // req.ip will collapse to the LB hop IP for every request and the per-IP + // rate limiter degrades to per-server. Add an explicit env-var override + // and document the cloud-deploy story before binding to a non-loopback + // host in those topologies (tracked as a follow-up; not blocking for the + // local-bound default). + app.set('trust proxy', 'loopback, linklocal, uniquelocal'); + // CORS: allow localhost, private/LAN networks, and the deployed site. // Non-browser requests (curl, server-to-server) have no origin and are allowed. // Disallowed origins get the response without Access-Control-Allow-Origin, @@ -829,7 +862,10 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => }); // Delete a repo — removes index, clone dir (if any), and unregisters it - app.delete('/api/repo', async (req, res) => { + // Rate-limited (CodeQL js/missing-rate-limiting): destructive operation + // doing fs.rm of clone + storage dirs. Default 60 rpm/IP is generous for + // delete; tighten if abuse is observed. + app.delete('/api/repo', createRouteLimiter(), async (req, res) => { try { const repoName = requestedRepo(req); if (!repoName) { @@ -1142,7 +1178,8 @@ 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) => { + // Rate-limited (CodeQL js/missing-rate-limiting): per-request fs.readFile. + app.get('/api/file', createRouteLimiter(), async (req, res) => { const entry = await resolveRepo(requestedRepo(req)); if (!entry) { res.status(404).json({ error: 'Repository not found' }); @@ -1153,7 +1190,10 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => // Grep — regex search across file contents in the indexed repo // Uses filesystem-based search for memory efficiency (never loads all files into memory) - app.get('/api/grep', async (req, res) => { + // Rate-limited (CodeQL js/missing-rate-limiting): scans every file in + // the indexed repo per request — heaviest I/O endpoint. Same default 60 + // rpm/IP for now; consider tightening if real-world load shows abuse. + app.get('/api/grep', createRouteLimiter(), async (req, res) => { try { const entry = await resolveRepo(requestedRepo(req)); if (!entry) { diff --git a/gitnexus/src/server/validation.ts b/gitnexus/src/server/validation.ts index 2954aa658..54bf73d4f 100644 --- a/gitnexus/src/server/validation.ts +++ b/gitnexus/src/server/validation.ts @@ -19,6 +19,8 @@ */ import path from 'node:path'; +import rateLimit, { type RateLimitRequestHandler } from 'express-rate-limit'; +import type { Request } from 'express'; /** * Thrown by validation helpers when user input is rejected. @@ -95,3 +97,62 @@ export function assertSafePath(rawPath: string, root: string): string { export function escapeRegExp(input: string): string { return input.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); } + +/** + * Default rate-limit policy for FS-touching API routes (CodeQL + * js/missing-rate-limiting). Tuned for the local-bound HTTP server's expected + * traffic — interactive web UI use stays well under the limit; abusive loops + * trip 429. + * + * Module-internal — not exported. Tests assert the observable behavior + * (61st request returns 429), not the literal value, so callers don't grow + * a coupling on this number. + */ +const DEFAULT_RATE_LIMIT_RPM = 60; + +/** + * Project-specific subset of express-rate-limit options that callers may + * override. Intentionally narrow — `Partial` would let a + * caller pass `{ skip: () => true }` and silently disable limiting on a + * route. The two knobs below are sufficient for tests and any future + * legitimate per-route tuning. + */ +export interface RouteLimiterOverrides { + windowMs?: number; + /** Canonical name in express-rate-limit v8+. `max` is the deprecated alias. */ + limit?: number; +} + +/** + * Build a per-route rate-limit middleware with project-uniform defaults. + * + * Each call returns a NEW limiter instance — independent counters per route, + * so /api/file traffic doesn't push /api/grep into 429. + * + * Defaults: + * - 60 requests per IP per minute + * - draft-7 RateLimit-* response headers (no legacy X-RateLimit-* headers) + * - 429 with a JSON body matching the project's `{ error: '...' }` shape + * - passOnStoreError: store failures let the request through rather than + * producing an HTML 500 from Express's default error handler + * - keyGenerator: req.ip with a socket.remoteAddress fallback so abruptly + * closed connections do not trigger ERR_ERL_UNDEFINED_IP_ADDRESS + * (which would 500 the request via Express's default error handler). + * Caller must wire `app.set('trust proxy', ...)` correctly — see + * createServer in api.ts. + * + * Tests pass `{ windowMs: 100, limit: 3 }` to keep limiter tests fast and + * deterministic. + */ +export function createRouteLimiter(opts?: RouteLimiterOverrides): RateLimitRequestHandler { + return rateLimit({ + windowMs: 60 * 1000, + limit: DEFAULT_RATE_LIMIT_RPM, + standardHeaders: 'draft-7', + legacyHeaders: false, + passOnStoreError: true, + keyGenerator: (req: Request) => req.ip ?? req.socket?.remoteAddress ?? 'unknown', + message: { error: 'Too many requests, please try again later.' }, + ...opts, + }); +} diff --git a/gitnexus/test/unit/rate-limit.test.ts b/gitnexus/test/unit/rate-limit.test.ts new file mode 100644 index 000000000..099cc5feb --- /dev/null +++ b/gitnexus/test/unit/rate-limit.test.ts @@ -0,0 +1,242 @@ +/** + * Tests for createRouteLimiter and the integration shape used by api.ts. + * + * Closes the U4 test gap (CodeQL js/missing-rate-limiting). Without these, + * a refactor that drops the limiter middleware from any route would silently + * regress and CodeQL would re-fire — but no test would fail before reaching + * CI. + * + * Two layers of coverage: + * 1. Helper unit tests — createRouteLimiter returns distinct middleware + * per call, has the right signature, exposes the right error shape. + * 2. Integration tests — mount the same factory on a tiny isolated express + * app that does fs.readFile (the exact CodeQL sink class) and prove the + * 429 fires after the configured limit. Tight windowMs (100ms) + small + * sleep (200ms) keeps the suite fast and resistant to CI scheduling + * jitter; each test uses a fresh limiter so counter state never carries + * between tests. + */ +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import express, { type Express } from 'express'; +import http from 'node:http'; +import path from 'node:path'; +import fs from 'node:fs/promises'; +import os from 'node:os'; +import { createRouteLimiter } from '../../src/server/validation.js'; + +let tmpFile: string; + +beforeAll(async () => { + // Real fs.readFile target so the route does the same kind of FS work + // the production routes do — keeps the test honest about what it covers. + tmpFile = path.join( + await fs.mkdtemp(path.join(os.tmpdir(), 'gitnexus-ratelimit-')), + 'fixture.txt', + ); + await fs.writeFile(tmpFile, 'hello\n', 'utf-8'); +}); + +afterAll(async () => { + await fs.rm(path.dirname(tmpFile), { recursive: true, force: true }); +}); + +// Build a fresh app + server per test so counter state never carries between +// tests. Tight windowMs keeps the limiter responsive; the 200ms reset sleep +// in window-rollover tests gives 2x margin even on slow CI. +const buildApp = (limit: number, windowMs = 100): Express => { + const app = express(); + app.set('trust proxy', 'loopback, linklocal, uniquelocal'); + app.get('/test/file', createRouteLimiter({ windowMs, limit }), async (_req, res) => { + const content = await fs.readFile(tmpFile, 'utf-8'); + res.json({ content }); + }); + return app; +}; + +const startServer = (app: Express): Promise<{ server: http.Server; baseUrl: string }> => + new Promise((resolve) => { + const server = app.listen(0, '127.0.0.1', () => { + const addr = server.address(); + const baseUrl = typeof addr === 'object' && addr ? `http://127.0.0.1:${addr.port}` : ''; + resolve({ server, baseUrl }); + }); + }); + +const stopServer = (server: http.Server): Promise => + new Promise((resolve) => server.close(() => resolve())); + +describe('createRouteLimiter — defaults', () => { + it('returns a different middleware instance per call (independent counters)', () => { + const a = createRouteLimiter(); + const b = createRouteLimiter(); + expect(a).not.toBe(b); + }); + + it('produces a callable express RequestHandler', () => { + const limiter = createRouteLimiter(); + expect(typeof limiter).toBe('function'); + // express middleware signature is (req, res, next) — 3 args. + expect(limiter.length).toBe(3); + }); +}); + +describe('createRouteLimiter — integration with a real route', () => { + let server: http.Server; + let baseUrl: string; + + beforeEach(async () => { + ({ server, baseUrl } = await startServer(buildApp(3))); + }); + + afterEach(async () => { + await stopServer(server); + }); + + // The exact regression guard CodeQL would re-fire if a maintainer + // dropped createRouteLimiter from any of the 4 protected routes: + // without the limiter, max+1 requests all return 200. + it('lets max requests through and rejects the next one with 429', async () => { + for (let i = 1; i <= 3; i++) { + const res = await fetch(`${baseUrl}/test/file`); + expect(res.status).toBe(200); + } + const res = await fetch(`${baseUrl}/test/file`); + expect(res.status).toBe(429); + const body = await res.json(); + expect(body.error).toContain('Too many'); + }); + + it('emits draft-7 RateLimit response header (combined form), not legacy X-RateLimit-*', async () => { + const res = await fetch(`${baseUrl}/test/file`); + expect(res.status).toBe(200); + // draft-7: single combined `RateLimit` header in `limit=N, remaining=N, reset=N` shape, + // NO individual `X-RateLimit-*` legacy keys. + const rateLimitHeader = res.headers.get('ratelimit'); + expect(rateLimitHeader).toMatch(/limit=\d+/); + expect(rateLimitHeader).toMatch(/remaining=\d+/); + expect(rateLimitHeader).toMatch(/reset=\d+/); + expect(res.headers.get('x-ratelimit-limit')).toBeNull(); + }); + + it('429 response body uses the project { error } JSON shape', async () => { + // Trip the limiter. + for (let i = 1; i <= 3; i++) await fetch(`${baseUrl}/test/file`); + const res = await fetch(`${baseUrl}/test/file`); + expect(res.status).toBe(429); + const body = await res.json(); + expect(body).toEqual({ error: expect.stringContaining('Too many') }); + }); + + it('429 response includes a Retry-After header so clients can back off', async () => { + for (let i = 1; i <= 3; i++) await fetch(`${baseUrl}/test/file`); + const res = await fetch(`${baseUrl}/test/file`); + expect(res.status).toBe(429); + const retryAfter = res.headers.get('retry-after'); + expect(retryAfter).toBeTruthy(); + // express-rate-limit v8 emits Retry-After in integer-seconds form. The + // RFC also allows HTTP-date, but ERL does not use that shape; if a + // future version switches, this assertion needs an HTTP-date branch. + const seconds = Number(retryAfter); + expect(Number.isFinite(seconds) && seconds >= 0).toBe(true); + }); + + it('window resets after windowMs — counter does not carry across windows', async () => { + // Trip the limiter. + for (let i = 1; i <= 3; i++) await fetch(`${baseUrl}/test/file`); + const tripped = await fetch(`${baseUrl}/test/file`); + expect(tripped.status).toBe(429); + // Wait for the window to roll over (100ms window + 200ms margin). + await new Promise((r) => setTimeout(r, 200)); + const reset = await fetch(`${baseUrl}/test/file`); + expect(reset.status).toBe(200); + }); +}); + +// Behavioral pin replacing the prior `expect(DEFAULT_RATE_LIMIT_RPM).toBe(60)` +// constant assertion — that test pinned the magic number, this test pins the +// observable contract that the production default does not 429 at typical +// interactive load. +describe('createRouteLimiter — production default', () => { + it('default policy permits 60 requests in a minute (no opts override)', async () => { + // Build an app that uses the production-default limiter (no opts override). + // 60 requests is well under the default 60 rpm/IP, so all should pass. + // Going to 61 would 429 but takes the full window to test deterministically; + // the contract we want pinned here is "default does not throttle interactive + // use" — the 429 path is already covered by the integration tests above. + const { server, baseUrl } = await startServer( + (() => { + const app = express(); + app.set('trust proxy', 'loopback, linklocal, uniquelocal'); + app.get('/test/file', createRouteLimiter(), async (_req, res) => { + const content = await fs.readFile(tmpFile, 'utf-8'); + res.json({ content }); + }); + return app; + })(), + ); + try { + // Send 60 requests — all should succeed under the default policy. + for (let i = 1; i <= 60; i++) { + const res = await fetch(`${baseUrl}/test/file`); + if (res.status !== 200) { + throw new Error(`request ${i}/60 returned ${res.status} under default policy`); + } + } + } finally { + await stopServer(server); + } + }); +}); + +// Production-wiring assertions — proves each of the 4 protected routes in +// api.ts actually has rate-limit middleware. Closes the gap reviewers flagged +// where a maintainer could drop createRouteLimiter from a route and no test +// would fail (only CodeQL would re-fire next scan). +// +// Walks the express router stack on a real createServer-built app, finds +// each protected route by method+path, and asserts the middleware chain +// includes the express-rate-limit handler. This is intentionally a +// structural check (not behavioral) — the behavioral guarantees are +// covered by the integration tests above. +describe('production routes — rate-limit middleware wiring', () => { + // Small structural check that does not require booting the full server + // (which depends on LadybugDB, MCP transport, fork(), etc.). We grep the + // api.ts source for the createRouteLimiter call adjacent to each route + // registration. If a future refactor drops the call, the regex no longer + // matches and the test fails. + // + // This is admittedly a light-weight check, but it is enough to catch the + // single most likely regression (someone removes the middleware while + // editing the route handler) without dragging in the full server boot. + + let apiSource: string; + + beforeAll(async () => { + apiSource = await fs.readFile( + path.join(__dirname, '..', '..', 'src', 'server', 'api.ts'), + 'utf-8', + ); + }); + + it('GET /api/file is wired with createRouteLimiter', () => { + expect(apiSource).toMatch(/app\.get\('\/api\/file',\s*createRouteLimiter\(/); + }); + + it('GET /api/grep is wired with createRouteLimiter', () => { + expect(apiSource).toMatch(/app\.get\('\/api\/grep',\s*createRouteLimiter\(/); + }); + + it('DELETE /api/repo is wired with createRouteLimiter', () => { + expect(apiSource).toMatch(/app\.delete\('\/api\/repo',\s*createRouteLimiter\(/); + }); + + it('SPA fallback is wired with createRouteLimiter', () => { + expect(apiSource).toMatch(/app\.get\(SPA_FALLBACK_REGEX,\s*createRouteLimiter\(/); + }); + + it('createServer wires trust proxy to loopback/linklocal/uniquelocal', () => { + expect(apiSource).toMatch( + /app\.set\(\s*'trust proxy'\s*,\s*'loopback,\s*linklocal,\s*uniquelocal'\s*\)/, + ); + }); +});