From cf931718d8875294554b7e7a6d846d3a02933bea Mon Sep 17 00:00:00 2001 From: Chris Portscheller Date: Sat, 22 Aug 2026 09:11:32 -0500 Subject: [PATCH] test(sdk): pin the invariants a reviewer cannot see Every finding in the 0.12/0.13 batch was two places answering one question, with the surface picking the flattering answer. The leftmost-XFF bug survived in two adapters after the same class was fixed elsewhere, because each call site read perfectly reasonably on its own and nothing connected them. Four source-reading invariants: the client IP is resolved only in client-ip.ts, decisions are only ever built through the Decision class and never spread (a spread silently strips isDenied() and deniedBy()), every starvable rule has a NOT_RUN path, and no node: import reaches a package that ships to Workers. Writing them found two things, which is the point: - The first version flagged detection/detectors/headers.ts, which merely LISTS those header names for suspicious-shape detection. The rule now matches actual access -- headers['x-forwarded-for'] or .get(...) -- not a mention. - It then flagged the Next.js adapter, correctly, for my own code: the x-real-ip and x-vercel-forwarded-for fallbacks I left inline in the trusted-proxy fix were header reads outside the resolver, which is exactly the drift the invariant exists to stop. They now live in resolveClientIp, gated on a declared proxy -- under trustProxy: false we read no forwarding header at all, and X-Real-IP is as forgeable as the rest. Four tests cover the relocated behaviour. 386 core tests, 20/20 turbo tasks, three edge entry points still clean. --- packages/nextjs/src/captcha.ts | 9 +- packages/nextjs/src/middleware.ts | 15 +-- packages/webdecoy/src/client-ip.test.ts | 39 ++++++ packages/webdecoy/src/client-ip.ts | 19 ++- packages/webdecoy/src/invariants.test.ts | 151 +++++++++++++++++++++++ 5 files changed, 211 insertions(+), 22 deletions(-) create mode 100644 packages/webdecoy/src/invariants.test.ts diff --git a/packages/nextjs/src/captcha.ts b/packages/nextjs/src/captcha.ts index 07f4bfd..032a778 100644 --- a/packages/nextjs/src/captcha.ts +++ b/packages/nextjs/src/captcha.ts @@ -15,7 +15,6 @@ import { createCaptchaEndpoints, resolveClientIp, - normalizeIp, type CaptchaEndpointsOptions, type TrustedProxies, } from '@webdecoy/node'; @@ -28,13 +27,7 @@ import { * back on. */ function getIP(headers: Headers, trustProxy: TrustedProxies | undefined): string { - const fromChain = resolveClientIp({ headers, trustProxy: trustProxy ?? 1 }); - if (fromChain) return fromChain; - return ( - normalizeIp(headers.get('x-real-ip')) ?? - normalizeIp(headers.get('x-vercel-forwarded-for')?.split(',').pop()) ?? - '127.0.0.1' - ); + return resolveClientIp({ headers, trustProxy: trustProxy ?? 1 }) ?? '127.0.0.1'; } export interface NextCaptchaOptions extends CaptchaEndpointsOptions { diff --git a/packages/nextjs/src/middleware.ts b/packages/nextjs/src/middleware.ts index 5a1f9bc..10fefad 100644 --- a/packages/nextjs/src/middleware.ts +++ b/packages/nextjs/src/middleware.ts @@ -99,17 +99,10 @@ export interface WebDecoyMiddlewareOptions extends ProtectOptions { * `X-Forwarded-For`, never an override of one. */ function resolveIP(req: NextRequest, trustProxy: TrustedProxies | undefined): string { - const fromChain = resolveClientIp({ - headers: req.headers, - trustProxy: trustProxy ?? 1, - }); - if (fromChain) return fromChain; - - return ( - normalizeIp(req.headers.get('x-real-ip')) ?? - normalizeIp(req.headers.get('x-vercel-forwarded-for')?.split(',').pop()) ?? - '127.0.0.1' - ); + // One resolver. `x-real-ip` and the platform header used to be read here as a + // fallback; they now live inside resolveClientIp, so this adapter reads no + // forwarding header of its own and cannot drift from the others. + return resolveClientIp({ headers: req.headers, trustProxy: trustProxy ?? 1 }) ?? '127.0.0.1'; } /** diff --git a/packages/webdecoy/src/client-ip.test.ts b/packages/webdecoy/src/client-ip.test.ts index e79b69e..aa982e3 100644 --- a/packages/webdecoy/src/client-ip.test.ts +++ b/packages/webdecoy/src/client-ip.test.ts @@ -302,3 +302,42 @@ describe('resolveClientIp', () => { }); }); }); + +describe('the single-header fallback', () => { + it('uses X-Real-IP when a proxy is declared but sends no chain', () => { + // nginx's default: X-Real-IP and no X-Forwarded-For. + const ip = resolveClientIp({ + headers: h({ 'x-real-ip': '203.0.113.9' }), + peer: '10.0.0.1', + trustProxy: 1, + }); + expect(ip).toBe('203.0.113.9'); + }); + + it('takes the last value of the platform header, not the first', () => { + const ip = resolveClientIp({ + headers: h({ 'x-vercel-forwarded-for': '1.2.3.4, 203.0.113.9' }), + trustProxy: 1, + }); + expect(ip).toBe('203.0.113.9'); + }); + + it('prefers a real forwarding chain over either', () => { + const ip = resolveClientIp({ + headers: h({ 'x-forwarded-for': '198.51.100.7', 'x-real-ip': '203.0.113.9' }), + peer: '10.0.0.1', + trustProxy: 1, + }); + expect(ip).toBe('198.51.100.7'); + }); + + it('ignores both when no proxy is declared', () => { + // Under trustProxy: false we read no forwarding header at all. X-Real-IP is + // exactly as forgeable as the rest of them. + const ip = resolveClientIp({ + headers: h({ 'x-real-ip': '1.2.3.4' }), + peer: '198.51.100.7', + }); + expect(ip).toBe('198.51.100.7'); + }); +}); diff --git a/packages/webdecoy/src/client-ip.ts b/packages/webdecoy/src/client-ip.ts index f5aeed9..dba985c 100644 --- a/packages/webdecoy/src/client-ip.ts +++ b/packages/webdecoy/src/client-ip.ts @@ -262,14 +262,27 @@ export function resolveClientIp(options: ResolveClientIpOptions): string | undef const chain = forwardedChain(headers); + // A proxy that sets only `X-Real-IP` (nginx's default) or the platform's own + // header, with no forwarding chain to walk. Consulted here rather than in each + // adapter so there is one place that decides what counts as the client — the + // adapters reading these themselves is how the leftmost-XFF bug came to live + // in three copies. + // + // Only when the caller has said a proxy exists. Under `false` we read no + // forwarding header at all, and these are as forgeable as the rest. + const singleHeaderFallback = (): string | undefined => + normalizeIp(readHeader(headers, 'x-real-ip')) ?? + normalizeIp(readHeader(headers, 'x-vercel-forwarded-for')?.split(',').pop()) ?? + undefined; + if (typeof trustProxy === 'number') { if (!Number.isInteger(trustProxy) || trustProxy < 0) return peer; // The client is the Nth entry from the right. A chain shorter than the // configured depth means the request did not arrive the way the operator // described it, so we believe none of it. const index = chain.length - trustProxy; - if (index < 0 || index >= chain.length) return peer; - return chain[index] ?? peer; + if (index < 0 || index >= chain.length) return singleHeaderFallback() ?? peer; + return chain[index] ?? singleHeaderFallback() ?? peer; } // CIDR list: walk right to left, past addresses that belong to us. `peer` @@ -284,5 +297,5 @@ export function resolveClientIp(options: ResolveClientIpOptions): string | undef } // Every hop was trusted, which means the outermost one is as far as the chain // goes — that address is the client. - return (full[0] ?? undefined) ?? peer; + return (full[0] ?? undefined) ?? singleHeaderFallback() ?? peer; } diff --git a/packages/webdecoy/src/invariants.test.ts b/packages/webdecoy/src/invariants.test.ts new file mode 100644 index 0000000..425d2fe --- /dev/null +++ b/packages/webdecoy/src/invariants.test.ts @@ -0,0 +1,151 @@ +import { readdirSync, readFileSync, statSync } from 'node:fs'; +import { join, relative } from 'node:path'; + +/** + * Invariants a reviewer cannot see. + * + * Every finding in the 0.12.0 / 0.13.0 batch was the same shape: two places + * answering one question, and the surface picking the more flattering answer. + * The spoofable client IP survived in two adapters after the WordPress plugin + * had already fixed the same class of bug, because each call site read perfectly + * reasonably on its own and nothing connected them. + * + * These tests connect them. They read source rather than behaviour on purpose: + * the defect is never "this function is wrong", it is "there are two of these + * and they disagree", which no unit test of either one can catch. + */ + +const PACKAGES = join(__dirname, '..', '..'); + +/** Every shipped .ts file across the workspace, tests and builds excluded. */ +function sourceFiles(): string[] { + const out: string[] = []; + const walk = (dir: string): void => { + for (const entry of readdirSync(dir)) { + if (entry === 'node_modules' || entry === 'dist' || entry === '.turbo') continue; + const full = join(dir, entry); + if (statSync(full).isDirectory()) { + walk(full); + continue; + } + if (!entry.endsWith('.ts')) continue; + if (entry.endsWith('.test.ts') || entry.endsWith('.spec.ts')) continue; + if (entry.endsWith('.generated.ts')) continue; + out.push(full); + } + }; + walk(PACKAGES); + return out; +} + +/** Lines of `file` that contain `needle`, ignoring comments. */ +function hits(file: string, needle: RegExp): string[] { + const found: string[] = []; + readFileSync(file, 'utf8') + .split('\n') + .forEach((line, i) => { + const trimmed = line.trim(); + if (trimmed.startsWith('//') || trimmed.startsWith('*') || trimmed.startsWith('/*')) return; + if (needle.test(line)) found.push(`${relative(PACKAGES, file)}:${i + 1}`); + }); + return found; +} + +describe('one answer to "who is the client"', () => { + it('resolves the client IP only in client-ip.ts', () => { + // Reading a forwarding header anywhere else is how the leftmost-XFF bug + // lived in Express and Next.js after the same bug had been fixed elsewhere: + // three adapters, three copies, and no one place to correct. + const offenders = sourceFiles() + .filter((f) => !f.endsWith(join('webdecoy', 'src', 'client-ip.ts'))) + // Matches *reading* the header to derive an address — + // `headers['x-forwarded-for']` or `.get('x-forwarded-for')` — not merely + // naming it. The detection engine legitimately lists these among the + // headers it inspects for suspicious shapes, and that is not IP + // resolution. + .flatMap((f) => + hits( + f, + /(?:\.get\(|\[)\s*['"](?:x-forwarded-for|x-real-ip|cf-connecting-ip)['"]/i, + ), + ); + + if (offenders.length > 0) { + throw new Error( + 'These read a forwarding header directly instead of calling resolveClientIp().\n' + + 'The leftmost value of X-Forwarded-For is written by the client, so trusting\n' + + 'it hands an attacker the rate-limit key and the address on every detection.\n' + + 'Use resolveClientIp({ headers, peer, trustProxy }).\n\n ' + + offenders.join('\n '), + ); + } + }); +}); + +describe('one answer to "what did we decide"', () => { + it('builds decisions only through the Decision class', () => { + // protect() returning a plain object literal is how `allowed` and + // `conclusion` drift apart, and how a spread silently strips the narrowing + // helpers off the result. + const sdk = join(PACKAGES, 'webdecoy', 'src', 'sdk.ts'); + const source = readFileSync(sdk, 'utf8'); + + if (/return\s*\{\s*\n?\s*allowed:/.test(source)) { + throw new Error( + 'sdk.ts returns a bare object with an `allowed` key. Every decision must be\n' + + 'a `new Decision({...})`, or the conclusion and the boolean can disagree\n' + + 'and the narrowing helpers are lost on the way out.', + ); + } + }); + + it('never spreads a decision, which would drop its methods', () => { + const sdk = readFileSync(join(PACKAGES, 'webdecoy', 'src', 'sdk.ts'), 'utf8'); + + if (/\{\s*\.\.\.(result|decision)\s*,/.test(sdk)) { + throw new Error( + 'Spreading a Decision produces a plain object: `isDenied()` and `deniedBy()`\n' + + 'vanish and the adapter silently loses them. Use decision.withEdge(...) or\n' + + 'another method that returns a Decision.', + ); + } + }); +}); + +describe('one answer to "is this rule running"', () => { + it('every rule that can be starved reports NOT_RUN rather than ALLOW', () => { + // A rule that cannot evaluate must say so. Reporting ALLOW makes "checked + // and fine" indistinguishable from "never checked", which is how a filter + // rule with no enrichment looked like a passing IP reputation check. + const starvable = ['filter-rule.ts', 'web-bot-auth-rule.ts', 'rate-limit-rule.ts']; + + for (const name of starvable) { + const source = readFileSync(join(PACKAGES, 'webdecoy', 'src', 'rules', name), 'utf8'); + if (!source.includes("state: 'NOT_RUN'")) { + throw new Error( + `${name} has no NOT_RUN path. A rule that silently allows when its input ` + + `is missing is indistinguishable from one that ran and passed.`, + ); + } + } + }); +}); + +describe('the edge build stays edge-compatible', () => { + it('no node: import reaches a package that ships to Workers', () => { + // check:edge catches this at build time, but only for the entry points it + // is pointed at. This catches it in review, with the file named. + const edgePackages = ['webdecoy', 'nextjs', 'hono']; + const offenders = sourceFiles() + .filter((f) => edgePackages.some((p) => f.includes(join(PACKAGES, p, 'src')))) + .flatMap((f) => hits(f, /from ['"]node:/)); + + if (offenders.length > 0) { + throw new Error( + 'A `node:` import anywhere in these graphs breaks the bundle for Cloudflare\n' + + 'Workers and Vercel Edge, where most of this SDK is meant to run.\n\n ' + + offenders.join('\n '), + ); + } + }); +});