From b39c4a4dd8d7eb7297cffa0a14dee89597e19126 Mon Sep 17 00:00:00 2001 From: Adam Argyle Date: Mon, 7 Sep 2026 23:43:11 -0700 Subject: [PATCH 1/2] Add rate-limit headers to the contact API is-agentic flagged whiskey.fm for having no REST rate-limit headers on any probed endpoint. The two episode JSON routes are prerendered static files with no per-request server logic to attach headers to, but /api/contact is dynamic and the only endpoint that does real work (a Discord webhook call) on every hit, so it's the one worth limiting. Adds a small in-memory fixed-window limiter (5 req/min per client IP) and standard RateLimit-Limit/Remaining/Reset headers on every contact response, plus Retry-After on a 429. Scoped to a single instance by design - noted in the module docstring as a known limit if this ever needs to hold across regions. Co-Authored-By: Claude Sonnet 5 --- packages/starpod/src/lib/rate-limit.ts | 71 +++++++++++++++++ packages/starpod/src/pages/api/contact.ts | 37 +++++++-- tests/unit/contact-api.test.ts | 68 +++++++++++++++- tests/unit/rate-limit.test.ts | 95 +++++++++++++++++++++++ 4 files changed, 264 insertions(+), 7 deletions(-) create mode 100644 packages/starpod/src/lib/rate-limit.ts create mode 100644 tests/unit/rate-limit.test.ts diff --git a/packages/starpod/src/lib/rate-limit.ts b/packages/starpod/src/lib/rate-limit.ts new file mode 100644 index 0000000..08f67b4 --- /dev/null +++ b/packages/starpod/src/lib/rate-limit.ts @@ -0,0 +1,71 @@ +/** + * Fixed-window rate limiting for API routes, scoped to a single deployed + * instance. Good enough to signal real limits on the write-side contact + * endpoint; not a substitute for a shared store (Upstash, Vercel KV, etc.) + * if this ever needs to hold across multiple instances/regions. + */ + +interface RateLimitOptions { + limit: number; + windowMs: number; +} + +interface RateLimitResult { + allowed: boolean; + limit: number; + remaining: number; + /** Epoch seconds the current window resets at. */ + resetAt: number; +} + +const buckets = new Map(); + +export function checkRateLimit( + key: string, + { limit, windowMs }: RateLimitOptions +): RateLimitResult { + const now = Date.now(); + const bucket = buckets.get(key); + + if (!bucket || bucket.resetAt <= now) { + const resetAt = now + windowMs; + buckets.set(key, { count: 1, resetAt }); + return { + allowed: true, + limit, + remaining: limit - 1, + resetAt: Math.ceil(resetAt / 1000) + }; + } + + bucket.count += 1; + + return { + allowed: bucket.count <= limit, + limit, + remaining: Math.max(0, limit - bucket.count), + resetAt: Math.ceil(bucket.resetAt / 1000) + }; +} + +/** Derives a rate-limit key from the client's IP, falling back to a shared + * bucket if no forwarding header is present (e.g. local dev). */ +export function clientKey(request: Request): string { + const forwardedFor = request.headers.get('x-forwarded-for'); + return forwardedFor?.split(',')[0]?.trim() || 'unknown'; +} + +export function rateLimitHeaders( + result: RateLimitResult +): Record { + return { + 'RateLimit-Limit': String(result.limit), + 'RateLimit-Remaining': String(result.remaining), + 'RateLimit-Reset': String(result.resetAt) + }; +} + +/** Test-only: clears all bucket state between test cases. */ +export function __resetRateLimitBucketsForTests(): void { + buckets.clear(); +} diff --git a/packages/starpod/src/pages/api/contact.ts b/packages/starpod/src/pages/api/contact.ts index 8884e64..c2441ad 100644 --- a/packages/starpod/src/pages/api/contact.ts +++ b/packages/starpod/src/pages/api/contact.ts @@ -1,10 +1,33 @@ import type { APIRoute } from 'astro'; import { jsonError } from '../../lib/api-errors'; +import { + checkRateLimit, + clientKey, + rateLimitHeaders +} from '../../lib/rate-limit'; export const prerender = false; +// The contact form is the only mutating, costly endpoint on this site (it +// calls out to a Discord webhook), so it's the only one worth rate limiting. +const CONTACT_RATE_LIMIT = { limit: 5, windowMs: 60_000 }; + export const POST: APIRoute = async ({ request }) => { + const rate = checkRateLimit(clientKey(request), CONTACT_RATE_LIMIT); + const headers = rateLimitHeaders(rate); + + if (!rate.allowed) { + const retryAfter = Math.max(0, rate.resetAt - Math.floor(Date.now() / 1000)); + return jsonError( + 429, + 'rate_limited', + 'Too many contact form submissions from this client', + 'Wait a minute before trying again.', + { ...headers, 'Retry-After': String(retryAfter) } + ); + } + let data: FormData; try { data = await request.formData(); @@ -13,7 +36,8 @@ export const POST: APIRoute = async ({ request }) => { 400, 'invalid_body', 'Request body could not be parsed as form data', - 'Send a multipart/form-data or application/x-www-form-urlencoded body with name, email, and message fields.' + 'Send a multipart/form-data or application/x-www-form-urlencoded body with name, email, and message fields.', + headers ); } @@ -32,7 +56,8 @@ export const POST: APIRoute = async ({ request }) => { 400, 'missing_fields', `Missing required fields: ${missing.join(', ')}`, - 'Provide name, email, and message form fields.' + 'Provide name, email, and message form fields.', + headers ); } @@ -41,7 +66,8 @@ export const POST: APIRoute = async ({ request }) => { 500, 'not_configured', 'The contact form is not configured on this deployment', - 'Set the DISCORD_WEBHOOK environment variable, or reach the hosts via the links on /contact.' + 'Set the DISCORD_WEBHOOK environment variable, or reach the hosts via the links on /contact.', + headers ); } @@ -87,7 +113,8 @@ export const POST: APIRoute = async ({ request }) => { 502, 'delivery_failed', 'Your message could not be delivered', - 'Try again in a few minutes, or reach the hosts via the links on /contact.' + 'Try again in a few minutes, or reach the hosts via the links on /contact.', + headers ); } @@ -98,7 +125,7 @@ export const POST: APIRoute = async ({ request }) => { }), { status: 200, - headers: { 'Content-Type': 'application/json; charset=utf-8' } + headers: { 'Content-Type': 'application/json; charset=utf-8', ...headers } } ); }; diff --git a/tests/unit/contact-api.test.ts b/tests/unit/contact-api.test.ts index 5da9271..9e1facf 100644 --- a/tests/unit/contact-api.test.ts +++ b/tests/unit/contact-api.test.ts @@ -1,13 +1,18 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; import { ALL, POST } from 'starpod/src/pages/api/contact'; +import { __resetRateLimitBucketsForTests } from 'starpod/src/lib/rate-limit'; type ApiContext = Parameters[0]; -function postContext(body: FormData | string): ApiContext { +function postContext( + body: FormData | string, + headers?: Record +): ApiContext { const request = new Request('http://localhost/api/contact', { method: 'POST', - body + body, + headers }); return { request } as ApiContext; } @@ -24,6 +29,7 @@ describe('contact API', () => { afterEach(() => { vi.unstubAllEnvs(); vi.unstubAllGlobals(); + __resetRateLimitBucketsForTests(); }); it('returns structured JSON 400 when fields are missing', async () => { @@ -90,4 +96,62 @@ describe('contact API', () => { const body = await response.json(); expect(body.error.code).toBe('method_not_allowed'); }); + + it('attaches RateLimit-* headers to a successful response', async () => { + vi.stubEnv('DISCORD_WEBHOOK', 'https://discord.example.com/webhook'); + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue(new Response('ok', { status: 200 })) + ); + + const response = await POST( + postContext(validForm(), { 'x-forwarded-for': '203.0.113.10' }) + ); + + expect(response.status).toBe(200); + expect(response.headers.get('RateLimit-Limit')).toBe('5'); + expect(response.headers.get('RateLimit-Remaining')).toBe('4'); + expect(response.headers.get('RateLimit-Reset')).toBeTruthy(); + }); + + it('returns structured JSON 429 with Retry-After once the limit is exceeded', async () => { + vi.stubEnv('DISCORD_WEBHOOK', 'https://discord.example.com/webhook'); + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue(new Response('ok', { status: 200 })) + ); + + const ip = { 'x-forwarded-for': '203.0.113.11' }; + for (let i = 0; i < 5; i++) { + const response = await POST(postContext(validForm(), ip)); + expect(response.status).toBe(200); + } + + const limited = await POST(postContext(validForm(), ip)); + + expect(limited.status).toBe(429); + expect(limited.headers.get('RateLimit-Remaining')).toBe('0'); + expect(limited.headers.get('Retry-After')).toBeTruthy(); + const body = await limited.json(); + expect(body.error.code).toBe('rate_limited'); + }); + + it('tracks rate limits per client independently', async () => { + vi.stubEnv('DISCORD_WEBHOOK', 'https://discord.example.com/webhook'); + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue(new Response('ok', { status: 200 })) + ); + + const clientOne = { 'x-forwarded-for': '203.0.113.12' }; + const clientTwo = { 'x-forwarded-for': '203.0.113.13' }; + + for (let i = 0; i < 5; i++) { + await POST(postContext(validForm(), clientOne)); + } + + const stillAllowed = await POST(postContext(validForm(), clientTwo)); + + expect(stillAllowed.status).toBe(200); + }); }); diff --git a/tests/unit/rate-limit.test.ts b/tests/unit/rate-limit.test.ts new file mode 100644 index 0000000..f618121 --- /dev/null +++ b/tests/unit/rate-limit.test.ts @@ -0,0 +1,95 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import { + __resetRateLimitBucketsForTests, + checkRateLimit, + clientKey, + rateLimitHeaders +} from 'starpod/src/lib/rate-limit'; + +describe('rate-limit', () => { + afterEach(() => { + __resetRateLimitBucketsForTests(); + vi.useRealTimers(); + }); + + it('allows requests up to the limit within the window', () => { + const opts = { limit: 3, windowMs: 60_000 }; + + const first = checkRateLimit('client-a', opts); + const second = checkRateLimit('client-a', opts); + const third = checkRateLimit('client-a', opts); + + expect(first.allowed).toBe(true); + expect(first.remaining).toBe(2); + expect(second.remaining).toBe(1); + expect(third.remaining).toBe(0); + }); + + it('rejects requests once the limit is exceeded', () => { + const opts = { limit: 2, windowMs: 60_000 }; + + checkRateLimit('client-b', opts); + checkRateLimit('client-b', opts); + const third = checkRateLimit('client-b', opts); + + expect(third.allowed).toBe(false); + expect(third.remaining).toBe(0); + }); + + it('tracks separate clients independently', () => { + const opts = { limit: 1, windowMs: 60_000 }; + + const a = checkRateLimit('client-c', opts); + const b = checkRateLimit('client-d', opts); + + expect(a.allowed).toBe(true); + expect(b.allowed).toBe(true); + }); + + it('resets the window once it elapses', () => { + vi.useFakeTimers(); + const opts = { limit: 1, windowMs: 1_000 }; + + const first = checkRateLimit('client-e', opts); + expect(first.allowed).toBe(true); + + const blocked = checkRateLimit('client-e', opts); + expect(blocked.allowed).toBe(false); + + vi.advanceTimersByTime(1_001); + + const afterWindow = checkRateLimit('client-e', opts); + expect(afterWindow.allowed).toBe(true); + expect(afterWindow.remaining).toBe(0); + }); + + it('derives the client key from x-forwarded-for, taking the first hop', () => { + const request = new Request('http://localhost/api/contact', { + headers: { 'x-forwarded-for': '203.0.113.4, 10.0.0.1' } + }); + + expect(clientKey(request)).toBe('203.0.113.4'); + }); + + it('falls back to a shared key when there is no forwarding header', () => { + const request = new Request('http://localhost/api/contact'); + + expect(clientKey(request)).toBe('unknown'); + }); + + it('formats standard RateLimit-* headers', () => { + const headers = rateLimitHeaders({ + allowed: true, + limit: 5, + remaining: 4, + resetAt: 1_700_000_000 + }); + + expect(headers).toEqual({ + 'RateLimit-Limit': '5', + 'RateLimit-Remaining': '4', + 'RateLimit-Reset': '1700000000' + }); + }); +}); From e41b527d7b48314e0e4d6acc5832e236d4d96dbd Mon Sep 17 00:00:00 2001 From: Adam Argyle Date: Tue, 8 Sep 2026 10:24:47 -0700 Subject: [PATCH 2/2] Address CodeRabbit review: bounded reset header, unbounded map, IP trust Three real issues from the automated review on #68: - RateLimit-Reset was emitting an absolute epoch timestamp; the IETF RateLimit header draft specifies it as seconds-until-reset (delta). Fixed and covered with a fake-timers test for both a future and an already-elapsed window. - The bucket Map had no eviction, so it grew by one entry per unique client key ever seen and never shrank. Added a prune pass (drop expired buckets) on every checkRateLimit call - cheap at this endpoint's real traffic, and keeps the map bounded to clients currently inside an active window. - clientKey read x-forwarded-for, which is spoofable behind an arbitrary reverse proxy. Vercel (this project's actual deployment target) already strips client-supplied X-Forwarded-For at the edge, but x-vercel-forwarded-for is the more explicit, Vercel-computed header and stays correct even behind an extra proxy in front of Vercel - prefer it, fall back to x-forwarded-for otherwise. Co-Authored-By: Claude Sonnet 5 --- packages/starpod/src/lib/rate-limit.ts | 45 ++++++++++++++-- tests/unit/rate-limit.test.ts | 75 +++++++++++++++++++++++--- 2 files changed, 109 insertions(+), 11 deletions(-) diff --git a/packages/starpod/src/lib/rate-limit.ts b/packages/starpod/src/lib/rate-limit.ts index 08f67b4..0910031 100644 --- a/packages/starpod/src/lib/rate-limit.ts +++ b/packages/starpod/src/lib/rate-limit.ts @@ -20,11 +20,23 @@ interface RateLimitResult { const buckets = new Map(); +/** Drops any bucket whose window has already elapsed, so the map only ever + * holds entries for clients currently inside an active window - otherwise + * it grows forever, one entry per unique client key ever seen. */ +function pruneExpiredBuckets(now: number): void { + for (const [key, bucket] of buckets) { + if (bucket.resetAt <= now) { + buckets.delete(key); + } + } +} + export function checkRateLimit( key: string, { limit, windowMs }: RateLimitOptions ): RateLimitResult { const now = Date.now(); + pruneExpiredBuckets(now); const bucket = buckets.get(key); if (!bucket || bucket.resetAt <= now) { @@ -48,20 +60,40 @@ export function checkRateLimit( }; } -/** Derives a rate-limit key from the client's IP, falling back to a shared - * bucket if no forwarding header is present (e.g. local dev). */ +/** + * Derives a rate-limit key from the client's IP, falling back to a shared + * bucket if no forwarding header is present (e.g. local dev). + * + * Prefers `x-vercel-forwarded-for`: on Vercel (this project's target + * deployment), `x-forwarded-for` is already overwritten at the edge and + * client-supplied values are stripped, so both headers carry the same real + * IP there - but `x-vercel-forwarded-for` stays trustworthy even behind an + * extra reverse proxy in front of Vercel, where `x-forwarded-for` could be + * appended to instead of replaced. Deployed anywhere else (not Vercel), + * `x-forwarded-for` is only as trustworthy as whatever's terminating TLS in + * front of the app - fine for this project's actual target, worth keeping + * in mind for anyone self-hosting the OSS template differently. + */ export function clientKey(request: Request): string { - const forwardedFor = request.headers.get('x-forwarded-for'); + const forwardedFor = + request.headers.get('x-vercel-forwarded-for') ?? + request.headers.get('x-forwarded-for'); return forwardedFor?.split(',')[0]?.trim() || 'unknown'; } export function rateLimitHeaders( result: RateLimitResult ): Record { + const secondsUntilReset = Math.max( + 0, + result.resetAt - Math.floor(Date.now() / 1000) + ); return { 'RateLimit-Limit': String(result.limit), 'RateLimit-Remaining': String(result.remaining), - 'RateLimit-Reset': String(result.resetAt) + // Per the IETF RateLimit header draft, Reset is seconds until the + // window resets (delta), not an absolute epoch timestamp. + 'RateLimit-Reset': String(secondsUntilReset) }; } @@ -69,3 +101,8 @@ export function rateLimitHeaders( export function __resetRateLimitBucketsForTests(): void { buckets.clear(); } + +/** Test-only: exposes the current bucket count to verify pruning. */ +export function __debugBucketCount(): number { + return buckets.size; +} diff --git a/tests/unit/rate-limit.test.ts b/tests/unit/rate-limit.test.ts index f618121..b08d1ab 100644 --- a/tests/unit/rate-limit.test.ts +++ b/tests/unit/rate-limit.test.ts @@ -1,6 +1,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; import { + __debugBucketCount, __resetRateLimitBucketsForTests, checkRateLimit, clientKey, @@ -64,7 +65,15 @@ describe('rate-limit', () => { expect(afterWindow.remaining).toBe(0); }); - it('derives the client key from x-forwarded-for, taking the first hop', () => { + it('derives the client key from x-vercel-forwarded-for when present', () => { + const request = new Request('http://localhost/api/contact', { + headers: { 'x-vercel-forwarded-for': '203.0.113.4' } + }); + + expect(clientKey(request)).toBe('203.0.113.4'); + }); + + it('falls back to x-forwarded-for, taking the first hop, when there is no x-vercel-forwarded-for', () => { const request = new Request('http://localhost/api/contact', { headers: { 'x-forwarded-for': '203.0.113.4, 10.0.0.1' } }); @@ -72,24 +81,76 @@ describe('rate-limit', () => { expect(clientKey(request)).toBe('203.0.113.4'); }); + it('prefers x-vercel-forwarded-for over x-forwarded-for when both are present', () => { + const request = new Request('http://localhost/api/contact', { + headers: { + 'x-vercel-forwarded-for': '203.0.113.4', + 'x-forwarded-for': '198.51.100.9' + } + }); + + expect(clientKey(request)).toBe('203.0.113.4'); + }); + it('falls back to a shared key when there is no forwarding header', () => { const request = new Request('http://localhost/api/contact'); expect(clientKey(request)).toBe('unknown'); }); - it('formats standard RateLimit-* headers', () => { + it('formats RateLimit-Limit and RateLimit-Remaining as-is', () => { const headers = rateLimitHeaders({ allowed: true, limit: 5, remaining: 4, - resetAt: 1_700_000_000 + resetAt: Math.floor(Date.now() / 1000) + 30 }); - expect(headers).toEqual({ - 'RateLimit-Limit': '5', - 'RateLimit-Remaining': '4', - 'RateLimit-Reset': '1700000000' + expect(headers['RateLimit-Limit']).toBe('5'); + expect(headers['RateLimit-Remaining']).toBe('4'); + }); + + it('formats RateLimit-Reset as seconds until reset, not an absolute epoch', () => { + vi.useFakeTimers(); + vi.setSystemTime(1_700_000_000_000); + + const headers = rateLimitHeaders({ + allowed: true, + limit: 5, + remaining: 4, + resetAt: 1_700_000_030 // 30s after the current mocked time }); + + expect(headers['RateLimit-Reset']).toBe('30'); + }); + + it('never reports a negative RateLimit-Reset for an already-elapsed window', () => { + vi.useFakeTimers(); + vi.setSystemTime(1_700_000_100_000); + + const headers = rateLimitHeaders({ + allowed: true, + limit: 5, + remaining: 4, + resetAt: 1_700_000_000 // already in the past relative to mocked time + }); + + expect(headers['RateLimit-Reset']).toBe('0'); + }); + + it('prunes expired buckets so the store does not grow unbounded', () => { + vi.useFakeTimers(); + const opts = { limit: 1, windowMs: 1_000 }; + + checkRateLimit('client-f', opts); + checkRateLimit('client-g', opts); + expect(__debugBucketCount()).toBe(2); + + vi.advanceTimersByTime(1_001); + checkRateLimit('client-h', opts); + + // client-f and client-g's windows elapsed and get pruned on the next + // call; only the fresh client-h bucket should remain. + expect(__debugBucketCount()).toBe(1); }); });