GitNexus/gitnexus/test/unit/rate-limit.test.ts
Copilot 60752de3e9
fix(ip): Scope write-route origin guard to server's own bound host (#2172)
* Initial plan

* Allow RFC1918 LAN origins in requireLocalhostOrigin

* Harden LAN origin parsing in middleware tests

* Refactor private IPv4 checks into shared server helper

* fix: scope origin guard to server's bound host, fix [::1], guard all write routes

- P1: Replace blanket RFC1918 trust with same-host check — only the server's
  own bound host is allowed (via `createLocalhostOriginGuard(host)`), not
  every device on the LAN.
- P2: Fix dead `::1` branch — compare against `'[::1]'` (with brackets) as
  returned by WHATWG URL parser.
- P3: Update 403 message to "same-host origins" and doc comments.
- Out-of-scope: Add `requireLocalhostOrigin` to `DELETE /api/repo`,
  `POST /api/embed`, `DELETE /api/embed/:jobId`, `DELETE /api/analyze/:jobId`.
- Tests: Add [::1] regression, ftp://, null origin, direct private-ip.ts
  unit tests, and createLocalhostOriginGuard bound-host tests.

* fix: cast route params to string when middleware breaks type inference

* chore(autofix): apply prettier + eslint fixes via /autofix command

* fix(test): update rate-limit test regex to match multi-line embed route registration

* fix(ip): normalize boundHost and keep wildcard binds loopback-only

The same-host write guard compared the raw `--host` string to the WHATWG
`URL.hostname` of the Origin, so it silently 403'd legitimate same-host
browser writes for several bind forms:
  - mixed-case hostnames (`MyHost.local` vs lowercased `myhost.local`)
  - non-loopback IPv6 (`fe80::1` vs bracketed `[fe80::1]`, and non-canonical
    forms like `fe80:0:0:0:0:0:0:1` / `::ffff:127.0.0.1`)
  - wildcard binds (`0.0.0.0` / `::`), the CLI-advertised remote-access config

Canonicalize boundHost once at guard construction through `new URL().hostname`
(provably the same form the Origin is parsed into), and treat wildcard binds as
having no single host identity → writes stay loopback-only. We deliberately do
NOT fall through to RFC1918 for wildcards (that would re-open whole-LAN reach).
`createServer` now warns when bound to a wildcard so a remote-access deployment
is not silently write-blocked.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(ip): tag origin-block 403 with a machine-readable code and surface it in the web client

The write-route Origin guard returned a 403 with only a human-readable
`error` string, so clients could not distinguish an origin block from any
other 403. The hosted web client (gitnexus.vercel.app driving a local
backend) swallowed the resulting failure: the repo delete button caught the
error and only `console.error`'d it, so it silently no-op'd.

- Server: add a stable `code: 'origin_not_allowed'` discriminator to the 403 body.
- Web client: `assertOk` reads `body.code` and maps `origin_not_allowed` to a new
  `BackendError` code `origin_blocked`; `formatBackendError` renders an actionable
  i18n message (en + zh-CN) instead of the generic client message.
- Header: surface the delete failure inline instead of swallowing it to console.

Scope note: the embedding-status badge (EmbeddingStatus.tsx) hides in backend
mode (its `serverBaseUrl` guard), so it is not the surface where an origin-block
embed error appears; a dedicated backend-mode embedding-error surface is deferred
with the broader hosted-UI mode-awareness follow-up.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* refactor(ip): remove unused isValidIpv4Address export

`isValidIpv4Address` had no `src/` consumer — only its own test imported it.
It was a leftover from the reverted RFC1918-middleware approach (the same-host
guard now compares against a canonicalized bound host, not an IPv4 validity
check). Remove the export and its orphaned test block. `parseIpv4Octets` stays
(it feeds `isRfc1918PrivateIpv4`, which CORS `isAllowedOrigin` still uses).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-13 09:24:03 +01:00

316 lines
13 KiB
TypeScript
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

/**
* 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. windowMs (2 000 ms) is generous
* enough that 4 sequential requests fit inside one window even on slow
* Windows CI runners; 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. windowMs = 2 000 ms gives ample headroom for Windows CI where
// sequential loopback HTTP requests can take 50–80 ms each.
const buildApp = (limit: number, windowMs = 2000): 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<void> =>
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);
});
// Regression guard for #1360 — createRouteLimiter must not throw
// ERR_ERL_KEY_GEN_IPV6. The validation fires at construction time
// (inside `rateLimit()`), so a simple `createRouteLimiter()` call is
// the canary: if the keyGenerator references `req.ip` without using
// `ipKeyGenerator`, the `rateLimit()` constructor throws before the
// middleware is ever invoked.
it('does not throw ERR_ERL_KEY_GEN_IPV6 on construction (#1360)', () => {
expect(() => createRouteLimiter()).not.toThrow();
});
});
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 (2 000 ms window + 200 ms margin).
await new Promise((r) => setTimeout(r, 2200));
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('POST /api/analyze is wired with createRouteLimiter', () => {
// Tolerate Prettier wrapping the registration across lines (it does once
// the route carries extra middleware like requireLocalhostOrigin).
expect(apiSource).toMatch(/app\.post\(\s*'\/api\/analyze',\s*createRouteLimiter\(/);
});
it('POST /api/embed is wired with createRouteLimiter', () => {
// Tolerate Prettier wrapping the registration across lines (it does once
// the route carries extra middleware like requireLocalhostOrigin).
expect(apiSource).toMatch(/app\.post\(\s*'\/api\/embed',\s*createRouteLimiter\(/);
});
it('SPA fallback is wired with createRouteLimiter', () => {
expect(apiSource).toMatch(/app\.get\(SPA_FALLBACK_REGEX,\s*createRouteLimiter\(/);
});
it('GET /api/health is registered (Docker healthcheck, #1147)', () => {
expect(apiSource).toMatch(/app\.get\('\/api\/health',\s*\(_req,\s*res\)\s*=>/);
});
it('does not register a bare wildcard OPTIONS route under Express 5', () => {
expect(apiSource).not.toContain("app.options('*'");
expect(apiSource).not.toMatch(/app\.options\(\s*'\/\*'/);
});
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*\)/,
);
});
it('does not register Express-4-only app.options("*") (Express 5 path-to-regexp)', () => {
expect(apiSource).not.toMatch(/app\.options\(\s*'\*'/);
expect(apiSource).not.toMatch(/app\.options\(\s*'\/\*'/);
});
it('sets PNA header middleware before cors (preflight must include Allow-Private-Network)', () => {
expect(apiSource).toMatch(
/Access-Control-Allow-Private-Network[\s\S]*?app\.use\(\s*\n?\s*cors\(/,
);
});
it('embed route flushes WAL via flushWAL, not inline executeQuery (#1376)', () => {
// The embed handler must call the consolidated helper, not hand-roll
// its own try/catch around executeQuery('CHECKPOINT').
expect(apiSource).toMatch(/await flushWAL\(\)/);
expect(apiSource).not.toMatch(/executeQuery\('CHECKPOINT'\)/);
});
});
// Structural guard for #1360 — validates that the validation module uses
// `ipKeyGenerator` so IPv6 addresses are normalised to their /56 subnet.
// Without this, each IPv6 address gets an independent counter and the
// rate-limit is trivially bypassed. The construction-time test above
// catches the same regression behaviourally; this source-grep test catches
// it structurally so the failure message is immediately obvious.
describe('validation.ts — IPv6 key normalisation (#1360)', () => {
let validationSource: string;
beforeAll(async () => {
validationSource = await fs.readFile(
path.join(__dirname, '..', '..', 'src', 'server', 'validation.ts'),
'utf-8',
);
});
it('imports ipKeyGenerator from express-rate-limit', () => {
expect(validationSource).toMatch(/import.*ipKeyGenerator.*from\s+'express-rate-limit'/);
});
it('keyGenerator body calls ipKeyGenerator', () => {
expect(validationSource).toMatch(/ipKeyGenerator\(ip\)/);
});
});