diff --git a/__tests__/unit/client-ip-key.test.ts b/__tests__/unit/client-ip-key.test.ts new file mode 100644 index 000000000..37f155c46 --- /dev/null +++ b/__tests__/unit/client-ip-key.test.ts @@ -0,0 +1,56 @@ +/** + * clientIpKey — the limiter is only as honest as its notion of "who". + * + * Caddy APPENDS to X-Forwarded-For, so a request that reached us through it + * carries `, `. Reading the + * first entry — which three payment routes did, while rate-limit.ts read the + * header whole — keys every limiter on a value the caller chooses. + * + * The consequence is not a weakened limiter. Vary the header per request and + * each request lands in a fresh bucket, so no bucket ever fills: no limiter at + * all, on routes that mint a real Lightning invoice through the recipient's + * wallet. bitbaum/orangecat#563 finding 2. + */ + +import { clientIpKey } from '@/lib/client-ip'; + +const req = (headers: Record) => + ({ headers: { get: (k: string) => headers[k.toLowerCase()] ?? null } }) as unknown as Request; + +describe('clientIpKey', () => { + it('returns the hop Caddy wrote, not the one the caller sent', () => { + expect(clientIpKey(req({ 'x-forwarded-for': '9.9.9.9, 203.0.113.7' }))).toBe('203.0.113.7'); + }); + + it('gives one key for one client however the prefix is spoofed', () => { + // The bypass itself: three requests from the same client, three forged + // prefixes. Before the fix these produced three keys and no bucket filled. + const keys = new Set( + ['1.1.1.1', '2.2.2.2', 'not-even-an-ip'].map((spoof) => + clientIpKey(req({ 'x-forwarded-for': `${spoof}, 203.0.113.7` })), + ), + ); + expect([...keys]).toEqual(['203.0.113.7']); + }); + + it('handles the ordinary single-hop request', () => { + expect(clientIpKey(req({ 'x-forwarded-for': '203.0.113.7' }))).toBe('203.0.113.7'); + }); + + it('tolerates the whitespace Caddy leaves after the comma', () => { + expect(clientIpKey(req({ 'x-forwarded-for': '9.9.9.9, 203.0.113.7 ' }))).toBe('203.0.113.7'); + }); + + it('falls back to x-real-ip, then to one shared bucket', () => { + expect(clientIpKey(req({ 'x-real-ip': '8.8.8.8' }))).toBe('8.8.8.8'); + expect(clientIpKey(req({}))).toBe('anonymous'); + }); + + it('never returns an empty key from a malformed header', () => { + // An empty key would collapse every caller into one bucket silently, or + // worse, produce the key "l402:" and look like a real identity. + expect(clientIpKey(req({ 'x-forwarded-for': '' }))).toBe('anonymous'); + expect(clientIpKey(req({ 'x-forwarded-for': ' , , ' }))).toBe('anonymous'); + expect(clientIpKey(req({ 'x-forwarded-for': ',' , 'x-real-ip': '8.8.8.8' }))).toBe('8.8.8.8'); + }); +}); diff --git a/package.json b/package.json index f3ba6861a..d47f4c6dc 100644 --- a/package.json +++ b/package.json @@ -37,9 +37,10 @@ "check:schema-columns": "node scripts/check-schema-columns.mjs", "check:currency-units": "node scripts/check-currency-units.mjs", "check:rpc-exists": "node scripts/check-rpc-exists.mjs", + "check:client-ip": "node scripts/check-client-ip.mjs", "check:ai-models": "node scripts/check-ai-models.mjs", "check:mdx": "node scripts/check-mdx.mjs", - "verify": "npm run ci:docs && npm run check:accent-ink && npm run type-check && npm run type-check:scripts && npm run check:sizes && npm run audit:routes && npm run lint && npm run check:duplication && npm run check:dead-fields && npm run check:migration-versions && npm run check:schema-columns && npm run check:currency-units && npm run check:rpc-exists && npm run check:mdx && npm run test:unit -- --watchAll=false", + "verify": "npm run ci:docs && npm run check:accent-ink && npm run type-check && npm run type-check:scripts && npm run check:sizes && npm run audit:routes && npm run lint && npm run check:duplication && npm run check:dead-fields && npm run check:migration-versions && npm run check:schema-columns && npm run check:currency-units && npm run check:rpc-exists && npm run check:client-ip && npm run check:mdx && npm run test:unit -- --watchAll=false", "audit:schema": "node scripts/db/audit-schema-drift.mjs", "audit:routes": "node scripts/audit-routes.mjs", "gen:types": "bash scripts/db/gen-types.sh", diff --git a/scripts/check-client-ip.mjs b/scripts/check-client-ip.mjs new file mode 100644 index 000000000..48e451713 --- /dev/null +++ b/scripts/check-client-ip.mjs @@ -0,0 +1,69 @@ +#!/usr/bin/env node +/* eslint-disable no-console */ +/** + * check-client-ip.mjs — nothing parses `x-forwarded-for` except the one helper. + * + * `X-Forwarded-For` is a LIST, and Caddy APPENDS to it. A request that reached + * us through Caddy carries `, `, so the only entry the caller cannot forge is the LAST one. + * + * Six places in this repo read it, and every one of them read it wrong: + * + * - `rate-limit.ts` used the header WHOLE, so any value at all was a new key; + * - three payment routes each copied `split(',')[0]` — the caller's own value; + * - the entity audit log recorded that value, an audit trail the subject + * writes; + * - the captcha route forwarded it to the provider as `remoteip`. + * + * The limiter consequence is the sharp one: vary the header per request and + * every request lands in a fresh bucket, so no bucket ever fills. That is not a + * weakened limiter, it is no limiter, while the route reads as protected — and + * on `/api/v1/pay/...` and `POST /api/v1/payments/public` each such request + * mints a real Lightning invoice through the recipient's own wallet, which is + * how you get a seller rate-limited by their wallet provider. + * + * Six instances is far past the point where fixing them one more time is the + * answer. `clientIpKey()` in src/lib/rate-limit.ts is the single definition; + * this gate keeps it single. See bitbaum/orangecat#563 finding 2, and + * `limitkit@0.2.0`, which carried the identical bug for the same reason. + */ + +import { readFileSync } from 'node:fs'; +import { execSync } from 'node:child_process'; + +// The one file allowed to look at the raw header: the definition itself. +const OWNER = 'src/lib/client-ip.ts'; + +// Any read of the header. Deliberately broad — the failure was never one +// spelling, it was six people each reaching for the header directly. +const PATTERN = /['"`]x-forwarded-for['"`]/i; + +const files = execSync( + "git ls-files 'src/**/*.ts' 'src/**/*.tsx'", + { encoding: 'utf8' }, +) + .split('\n') + .filter(Boolean) + .filter((f) => f !== OWNER); + +const offenders = []; +for (const file of files) { + const lines = readFileSync(file, 'utf8').split('\n'); + lines.forEach((line, i) => { + if (PATTERN.test(line)) offenders.push(`${file}:${i + 1}: ${line.trim()}`); + }); +} + +if (offenders.length > 0) { + console.error('✗ x-forwarded-for is read outside the one helper that knows which hop to trust:'); + for (const o of offenders) console.error(` ${o}`); + console.error(''); + console.error(` Caddy APPENDS to that header, so the first entry is whatever the caller`); + console.error(` sent and only the last one is evidence. Use clientIpKey(request) from`); + console.error(` ${OWNER} — it counts from the right and falls back honestly.`); + console.error(''); + console.error(' A limiter keyed on a caller-controlled value can never be tripped.'); + process.exit(1); +} + +console.log(`✓ client IP: ${files.length} file(s) checked, x-forwarded-for read only in ${OWNER}`); diff --git a/src/app/api/auth/verify-captcha/route.ts b/src/app/api/auth/verify-captcha/route.ts index 786c5424f..e10402b04 100644 --- a/src/app/api/auth/verify-captcha/route.ts +++ b/src/app/api/auth/verify-captcha/route.ts @@ -3,6 +3,7 @@ import { verifyCaptchaToken } from '@/lib/captcha'; import { logger } from '@/utils/logger'; import { apiSuccess, apiBadRequest, apiInternalError } from '@/lib/api/standardResponse'; import { rateLimit, createRateLimitResponse } from '@/lib/rate-limit'; +import { clientIpOrUndefined } from '@/lib/client-ip'; /** * POST /api/auth/verify-captcha @@ -32,9 +33,10 @@ export async function POST(request: NextRequest) { return apiBadRequest('CAPTCHA token is required'); } - // Get client IP for additional validation - const forwardedFor = request.headers.get('x-forwarded-for'); - const remoteIp = forwardedFor?.split(',')[0]?.trim(); + // Get client IP for additional validation — the hop Caddy wrote, not the + // caller-supplied first one, or the provider's IP heuristics are being fed + // whatever the caller chose. + const remoteIp = clientIpOrUndefined(request); const result = await verifyCaptchaToken(token, remoteIp); diff --git a/src/app/api/v1/pay/[entity_type]/[entity_id]/route.ts b/src/app/api/v1/pay/[entity_type]/[entity_id]/route.ts index 1f7e76456..fca51cfaa 100644 --- a/src/app/api/v1/pay/[entity_type]/[entity_id]/route.ts +++ b/src/app/api/v1/pay/[entity_type]/[entity_id]/route.ts @@ -12,6 +12,7 @@ import { publicSupportCreateSchema } from '@/lib/validation/finance'; import { createPublicClient } from '@/lib/supabase/public'; import { rateLimitWriteAsync, retryAfterSeconds } from '@/lib/rate-limit'; import { logger } from '@/utils/logger'; +import { clientIpKey } from '@/lib/client-ip'; /** * GET /api/v1/pay/{entity_type}/{entity_id}?amount_btc=X — HTTP 402 inline payment. @@ -31,8 +32,7 @@ import { logger } from '@/utils/logger'; */ function requestKey(request: Request): string { - const forwarded = request.headers.get('x-forwarded-for')?.split(',')[0]?.trim(); - return `l402:${forwarded || request.headers.get('x-real-ip') || 'anonymous'}`; + return `l402:${clientIpKey(request)}`; } export async function GET( diff --git a/src/app/api/v1/payments/public/[id]/route.ts b/src/app/api/v1/payments/public/[id]/route.ts index e4a028913..0480c6b8d 100644 --- a/src/app/api/v1/payments/public/[id]/route.ts +++ b/src/app/api/v1/payments/public/[id]/route.ts @@ -11,11 +11,11 @@ import { import { publicPaymentActionSchema } from '@/lib/validation/finance'; import { validateUUID, getValidationError } from '@/lib/api/validation'; import { rateLimitWriteAsync, retryAfterSeconds } from '@/lib/rate-limit'; +import { clientIpKey } from '@/lib/client-ip'; // Same per-IP keying as ../route.ts — anonymous callers, so IP is the only handle. function requestKey(request: Request): string { - const forwarded = request.headers.get('x-forwarded-for')?.split(',')[0]?.trim(); - return `public-payment-action:${forwarded || request.headers.get('x-real-ip') || 'anonymous'}`; + return `public-payment-action:${clientIpKey(request)}`; } function readToken(request: Request): string | null { diff --git a/src/app/api/v1/payments/public/route.ts b/src/app/api/v1/payments/public/route.ts index 171f82ef2..f687a52f7 100644 --- a/src/app/api/v1/payments/public/route.ts +++ b/src/app/api/v1/payments/public/route.ts @@ -9,10 +9,10 @@ import { publicSupportCreateSchema } from '@/lib/validation/finance'; import { createPublicClient } from '@/lib/supabase/public'; import { rateLimitWriteAsync, retryAfterSeconds } from '@/lib/rate-limit'; import { logger } from '@/utils/logger'; +import { clientIpKey } from '@/lib/client-ip'; function requestKey(request: Request): string { - const forwarded = request.headers.get('x-forwarded-for')?.split(',')[0]?.trim(); - return `public-support:${forwarded || request.headers.get('x-real-ip') || 'anonymous'}`; + return `public-support:${clientIpKey(request)}`; } export async function POST(request: Request) { diff --git a/src/lib/api/entityPostHandler.ts b/src/lib/api/entityPostHandler.ts index 8b0b81a92..b9cd99d55 100644 --- a/src/lib/api/entityPostHandler.ts +++ b/src/lib/api/entityPostHandler.ts @@ -57,6 +57,7 @@ import { } from '@/services/idempotency/idempotencyResults'; import { enqueueWebhookEvent } from '@/services/webhooks/deliveryService'; import { auditLog, AUDIT_ACTIONS } from '@/lib/api/auditLog'; +import { clientIpOrUndefined } from '@/lib/client-ip'; // Type for the awaited Supabase client type SupabaseClient = Awaited>; @@ -334,7 +335,9 @@ export function createEntityPostHandler(config: EntityPostHandlerConfig) { entityType, entityId, metadata: { actorId: resolvedActor.id, source: auth.source }, - ipAddress: request.headers.get('x-forwarded-for')?.split(',')[0]?.trim() || undefined, + // The hop Caddy wrote. The first hop is caller-supplied, so recording + // it let anyone choose what the audit trail said about them. + ipAddress: clientIpOrUndefined(request), userAgent: request.headers.get('user-agent') || undefined, }); }; diff --git a/src/lib/client-ip.ts b/src/lib/client-ip.ts new file mode 100644 index 000000000..a17e8529d --- /dev/null +++ b/src/lib/client-ip.ts @@ -0,0 +1,65 @@ +/** + * Who is calling — the one definition, and the one place that reads + * `x-forwarded-for`. + * + * WHICH HOP, AND WHY NOT THE FIRST + * + * `X-Forwarded-For` is a list, and Caddy APPENDS to it. A request that arrived + * through Caddy carries `, `, so the only entry the caller cannot forge is the LAST one. + * + * Six places here read that header and every one read it wrong: `rate-limit.ts` + * used it whole, so any value at all was a fresh key; three payment routes each + * copied `split(',')[0]`, the caller's own value; the entity audit log recorded + * that value, making the trail writable by its subject; and the captcha route + * forwarded it to the provider as `remoteip`. + * + * The limiter consequence is the sharp one. Vary the header per request and + * every request lands in a fresh bucket, so no bucket ever fills — not a + * weakened limiter but no limiter, on a route that reads as protected. On + * `GET /api/v1/pay/...` and `POST /api/v1/payments/public` each such request + * mints a real Lightning invoice through the recipient's own wallet, which is + * how an attacker gets a seller throttled or banned by their wallet provider. + * + * Verified rather than assumed, 2026-08-28: orangecat.ch resolves straight to + * the box, no CDN in front (`via: 1.1 Caddy`), one Caddy 2.11.4 with no + * `trusted_proxies` configured — exactly one appended hop. + * + * Its own module, not a corner of `rate-limit.ts`, so the audit log and the + * captcha route can ask who is calling without importing an Upstash client. + * + * See bitbaum/orangecat#563 finding 2. `limitkit@0.2.0` ships the same + * correction as `clientIp()`; adopting it here is ADR-0002's remaining work. + */ + +type HeadersLike = { get(name: string): string | null }; +type RequestLike = { headers: HeadersLike }; + +/** + * The caller's IP as seen by Caddy, or a shared `anonymous` bucket. + * + * The fallback throttles unattributable traffic collectively rather than + * letting it past, and is never empty — an empty key would read as a real + * identity in `l402:${key}` while collapsing every caller into one bucket. + */ +export function clientIpKey(request: RequestLike): string { + const hops = (request.headers.get('x-forwarded-for') ?? '') + .split(',') + .map((hop) => hop.trim()) + .filter(Boolean); + + // One trusted proxy (Caddy), so the rightmost hop is the one it wrote. + if (hops.length > 0) return hops[hops.length - 1]!; + + return request.headers.get('x-real-ip')?.trim() || 'anonymous'; +} + +/** + * The same answer where an ADDRESS is wanted rather than a bucket key — + * an audit row or a captcha `remoteip` wants nothing at all rather than the + * word "anonymous", which is a limiter concept and not a place. + */ +export function clientIpOrUndefined(request: RequestLike): string | undefined { + const key = clientIpKey(request); + return key === 'anonymous' ? undefined : key; +} diff --git a/src/lib/rate-limit.ts b/src/lib/rate-limit.ts index 21e15cf6e..b105d33d9 100644 --- a/src/lib/rate-limit.ts +++ b/src/lib/rate-limit.ts @@ -17,6 +17,7 @@ import { Ratelimit } from '@upstash/ratelimit'; import { Redis } from '@upstash/redis'; import { logger } from '@/utils/logger'; +import { clientIpKey } from '@/lib/client-ip'; // ==================== TYPES ==================== @@ -176,14 +177,9 @@ const fallbackDomainSearchLimiter = new InMemoryRateLimiter({ // ==================== RATE LIMIT FUNCTIONS ==================== -/** - * The caller's IP, as seen through Caddy. - * - * One definition, because a per-IP limiter is only as correct as its notion of - * "IP" — and three limiters had already copied these two lines verbatim. - */ +/** The caller's IP — defined once in ./client-ip, re-exported for callers. */ function clientIp(request: RequestLike): string { - return request.headers.get('x-forwarded-for') || request.headers.get('x-real-ip') || 'anonymous'; + return clientIpKey(request); } /** @@ -507,3 +503,5 @@ export function applyRateLimitHeaders(response: T, result: R } return response; } + +export { clientIpKey };