From d822b774c5df9c1434a6ee4d65517271c91fd9e4 Mon Sep 17 00:00:00 2001 From: C0dertang Date: Tue, 23 Jun 2026 14:43:32 -0700 Subject: [PATCH 1/3] feat(site): rate-limit public write endpoints (#936) The contact form and CMS form endpoints had no throttling, leaving them open to spam and abuse. Adds reusable IP-based rate limiting backed by Upstash Redis. - lib/rate-limit.ts: a shared, fail-open limiter. `createRateLimiter(config)` builds named limiters that share one Redis client; `publicWriteRateLimit` is the default preset (5 requests / IP / 60s); `enforceRateLimit()` is a one-line server-action guard that resolves the client IP, checks the limit, and throws when exceeded. Fails open (allows the request) when the Upstash env vars are absent or Redis errors, so local dev, CI and preview keep working. - lib/utils/get-client-ip.ts: parses the first IP from `x-forwarded-for`. - Wires `await enforceRateLimit()` into `submitPublicInquiry` (contact) and `submitFormSubmission` (CMS forms) before any work is done. - Adds @upstash/ratelimit + @upstash/redis; documents the two env vars in env.example as optional (fail-open). Tests cover fail-open, allow, block, identifier passthrough, shared-client reuse, Redis-error fail-open, and the enforce wrapper; both action tests assert the blocked path skips downstream work. Closes #936 Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/site/env.example | 9 +- apps/site/package.json | 2 + .../(contact)/contact/actions.test.ts | 23 ++ .../[locale]/(contact)/contact/actions.ts | 3 + .../(marketing)/forms/[slug]/actions.test.ts | 89 +++++++ .../(marketing)/forms/[slug]/actions.ts | 3 + apps/site/src/lib/rate-limit.test.ts | 217 ++++++++++++++++++ apps/site/src/lib/rate-limit.ts | 156 +++++++++++++ apps/site/src/lib/utils/get-client-ip.test.ts | 26 +++ apps/site/src/lib/utils/get-client-ip.ts | 27 +++ bun.lock | 10 + 11 files changed, 564 insertions(+), 1 deletion(-) create mode 100644 apps/site/src/app/(unauthenticated)/[locale]/(marketing)/forms/[slug]/actions.test.ts create mode 100644 apps/site/src/lib/rate-limit.test.ts create mode 100644 apps/site/src/lib/rate-limit.ts create mode 100644 apps/site/src/lib/utils/get-client-ip.test.ts create mode 100644 apps/site/src/lib/utils/get-client-ip.ts diff --git a/apps/site/env.example b/apps/site/env.example index d415c497..e5ef09aa 100644 --- a/apps/site/env.example +++ b/apps/site/env.example @@ -95,4 +95,11 @@ INTERNAL_ORDER_EMAIL_TO="orders@toddagriscience.com" # New Relic keys for monitoring. Private. NEW_RELIC_ENABLED="false" NEW_RELIC_APP_NAME="TODD_SITE_TESTING" -NEW_RELIC_LICENSE_KEY="secret" \ No newline at end of file +NEW_RELIC_LICENSE_KEY="secret" + +# Upstash Redis for IP-based rate limiting on public write endpoints (contact +# form, CMS forms). Optional — when unset, rate limiting fails open (allows +# requests) so local dev / CI / preview keep working. Provision via the Vercel +# Marketplace Upstash integration, which injects these automatically. +UPSTASH_REDIS_REST_URL="https://your-db.upstash.io" +UPSTASH_REDIS_REST_TOKEN="your-token" \ No newline at end of file diff --git a/apps/site/package.json b/apps/site/package.json index 4b7f2f69..99f2bbfd 100644 --- a/apps/site/package.json +++ b/apps/site/package.json @@ -63,6 +63,8 @@ "@supabase/ssr": "^0.10.0", "@supabase/supabase-js": "^2.102.1", "@types/ua-parser-js": "^0.7.39", + "@upstash/ratelimit": "^2.0.8", + "@upstash/redis": "^1.38.0", "class-variance-authority": "^0.7.1", "clsx": "^2.1.1", "drizzle-orm": "^0.45.2", diff --git a/apps/site/src/app/(unauthenticated)/[locale]/(contact)/contact/actions.test.ts b/apps/site/src/app/(unauthenticated)/[locale]/(contact)/contact/actions.test.ts index 44795926..f731f9cf 100644 --- a/apps/site/src/app/(unauthenticated)/[locale]/(contact)/contact/actions.test.ts +++ b/apps/site/src/app/(unauthenticated)/[locale]/(contact)/contact/actions.test.ts @@ -5,6 +5,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; const mocks = vi.hoisted(() => ({ submitContactToSheets: vi.fn(), loggerError: vi.fn(), + enforceRateLimit: vi.fn(), })); vi.mock('@/lib/actions/googleSheets', () => ({ @@ -15,6 +16,10 @@ vi.mock('@/lib/logger', () => ({ default: { error: mocks.loggerError }, })); +vi.mock('@/lib/rate-limit', () => ({ + enforceRateLimit: mocks.enforceRateLimit, +})); + import { submitPublicInquiry } from './actions'; /** @@ -33,9 +38,27 @@ function makeFormData(fields: Record) { beforeEach(() => { vi.clearAllMocks(); + mocks.enforceRateLimit.mockResolvedValue(undefined); }); describe('submitPublicInquiry', () => { + it('throws and skips submission when rate limited', async () => { + mocks.enforceRateLimit.mockRejectedValueOnce( + new Error('Too many requests. Please try again shortly.') + ); + + const fd = makeFormData({ + name: 'Inban', + lastKnownEmail: 'inban@example.com', + response: 'Hello!', + }); + + await expect(submitPublicInquiry(fd)).rejects.toThrow( + 'Too many requests. Please try again shortly.' + ); + expect(mocks.submitContactToSheets).not.toHaveBeenCalled(); + }); + it('returns success and submits to the contact sheet when inputs are valid', async () => { mocks.submitContactToSheets.mockResolvedValueOnce(undefined); diff --git a/apps/site/src/app/(unauthenticated)/[locale]/(contact)/contact/actions.ts b/apps/site/src/app/(unauthenticated)/[locale]/(contact)/contact/actions.ts index b20419ca..838bbe87 100644 --- a/apps/site/src/app/(unauthenticated)/[locale]/(contact)/contact/actions.ts +++ b/apps/site/src/app/(unauthenticated)/[locale]/(contact)/contact/actions.ts @@ -4,6 +4,7 @@ import { submitContactToSheets } from '@/lib/actions/googleSheets'; import logger from '@/lib/logger'; +import { enforceRateLimit } from '@/lib/rate-limit'; import type { ActionResponse } from '@/lib/types/action-response'; import { throwActionError } from '@/lib/utils/actions'; import z from 'zod'; @@ -30,6 +31,8 @@ function getString(formData: FormData, key: string): string { export async function submitPublicInquiry( formData: FormData ): Promise { + await enforceRateLimit(); + const raw = { name: getString(formData, 'name'), lastKnownEmail: getString(formData, 'lastKnownEmail'), diff --git a/apps/site/src/app/(unauthenticated)/[locale]/(marketing)/forms/[slug]/actions.test.ts b/apps/site/src/app/(unauthenticated)/[locale]/(marketing)/forms/[slug]/actions.test.ts new file mode 100644 index 00000000..11eb4d7a --- /dev/null +++ b/apps/site/src/app/(unauthenticated)/[locale]/(marketing)/forms/[slug]/actions.test.ts @@ -0,0 +1,89 @@ +// Copyright © Todd Agriscience, Inc. All rights reserved. + +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + getFormBySlug: vi.fn(), + insert: vi.fn(), + values: vi.fn(), + returning: vi.fn(), + loggerWarn: vi.fn(), + loggerError: vi.fn(), + enforceRateLimit: vi.fn(), +})); + +vi.mock('@/lib/sanity/forms', () => ({ + getFormBySlug: mocks.getFormBySlug, +})); + +vi.mock('@nightcrawler/db', () => ({ + db: { insert: mocks.insert }, +})); + +vi.mock('@nightcrawler/db/schema', () => ({ + formSubmission: { id: 'id' }, +})); + +vi.mock('@nightcrawler/db/utils/extract-applicant-prefill', () => ({ + enrichStoredAnswersWithSignupPrefill: (answers: unknown) => answers, +})); + +vi.mock('@/lib/logger', () => ({ + logger: { warn: mocks.loggerWarn, error: mocks.loggerError }, + default: { warn: mocks.loggerWarn, error: mocks.loggerError }, +})); + +vi.mock('@/lib/rate-limit', () => ({ + enforceRateLimit: mocks.enforceRateLimit, +})); + +vi.mock('./utils', () => ({ + buildFormAnswersSchema: () => ({ + safeParse: (data: unknown) => ({ success: true, data }), + }), + buildStoredFormAnswers: (data: Record) => data, + flattenFormFields: () => [], + FORM_HONEYPOT_FIELD: '_hp', + readRetentionConsentFromValues: () => false, + resolveFormFooterCheckboxes: () => [], + resolveFormSections: () => [], +})); + +import { submitFormSubmission } from './actions'; + +beforeEach(() => { + vi.clearAllMocks(); + mocks.enforceRateLimit.mockResolvedValue(undefined); + mocks.getFormBySlug.mockResolvedValue({ workflowType: 'generic' }); + + // db.insert(...).values(...).returning(...) fluent chain + mocks.returning.mockResolvedValue([{ id: 'row-1' }]); + mocks.values.mockReturnValue({ returning: mocks.returning }); + mocks.insert.mockReturnValue({ values: mocks.values }); +}); + +describe('submitFormSubmission', () => { + it('throws and skips the db insert when rate limited', async () => { + mocks.enforceRateLimit.mockRejectedValueOnce( + new Error('Too many requests. Please try again shortly.') + ); + + await expect( + submitFormSubmission({ formSlug: 'demo', answers: {} }) + ).rejects.toThrow('Too many requests. Please try again shortly.'); + + expect(mocks.getFormBySlug).not.toHaveBeenCalled(); + expect(mocks.insert).not.toHaveBeenCalled(); + }); + + it('persists the submission when allowed', async () => { + const result = await submitFormSubmission({ + formSlug: 'demo', + answers: { foo: 'bar' }, + }); + + expect(mocks.enforceRateLimit).toHaveBeenCalledTimes(1); + expect(mocks.insert).toHaveBeenCalledTimes(1); + expect(result).toEqual({ data: { id: 'row-1' } }); + }); +}); diff --git a/apps/site/src/app/(unauthenticated)/[locale]/(marketing)/forms/[slug]/actions.ts b/apps/site/src/app/(unauthenticated)/[locale]/(marketing)/forms/[slug]/actions.ts index bc24a026..c655af43 100644 --- a/apps/site/src/app/(unauthenticated)/[locale]/(marketing)/forms/[slug]/actions.ts +++ b/apps/site/src/app/(unauthenticated)/[locale]/(marketing)/forms/[slug]/actions.ts @@ -7,6 +7,7 @@ import type { SanityFormWorkflowType } from '@/lib/sanity/form-types'; import { db } from '@nightcrawler/db'; import { formSubmission } from '@nightcrawler/db/schema'; import { logger } from '@/lib/logger'; +import { enforceRateLimit } from '@/lib/rate-limit'; import type { ActionResponse } from '@/lib/types/action-response'; import { throwActionError } from '@/lib/utils/actions'; import z from 'zod'; @@ -50,6 +51,8 @@ function resolveWorkflowType( export async function submitFormSubmission( input: SubmitFormSubmissionInput ): Promise { + await enforceRateLimit(); + const form = await getFormBySlug(input.formSlug, { cache: 'no-store' }); if (!form) { throwActionError('This form is not available.'); diff --git a/apps/site/src/lib/rate-limit.test.ts b/apps/site/src/lib/rate-limit.test.ts new file mode 100644 index 00000000..bdd2819e --- /dev/null +++ b/apps/site/src/lib/rate-limit.test.ts @@ -0,0 +1,217 @@ +// Copyright © Todd Agriscience, Inc. All rights reserved. + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + loggerWarn: vi.fn(), + loggerError: vi.fn(), + limit: vi.fn(), + fromEnv: vi.fn(() => ({})), + slidingWindow: vi.fn(() => ({})), + headers: vi.fn(), + throwActionError: vi.fn((message: string) => { + throw new Error(message); + }), +})); + +vi.mock('@/lib/logger', () => ({ + logger: { warn: mocks.loggerWarn, error: mocks.loggerError }, + default: { warn: mocks.loggerWarn, error: mocks.loggerError }, +})); + +vi.mock('@upstash/redis', () => ({ + Redis: { fromEnv: mocks.fromEnv }, +})); + +vi.mock('@upstash/ratelimit', () => ({ + Ratelimit: class { + static slidingWindow = mocks.slidingWindow; + limit = mocks.limit; + }, +})); + +vi.mock('next/headers', () => ({ + headers: mocks.headers, +})); + +vi.mock('@/lib/utils/actions', () => ({ + throwActionError: mocks.throwActionError, +})); + +/** + * Imports a fresh copy of the rate-limit module so module-level singletons + * (shared Redis client, limiter cache, one-time-warn flag) are reset between + * test cases. + * + * @returns The freshly evaluated module exports. + */ +async function importFresh() { + vi.resetModules(); + return import('./rate-limit'); +} + +beforeEach(() => { + vi.clearAllMocks(); + mocks.headers.mockResolvedValue( + new Headers({ 'x-forwarded-for': '1.2.3.4' }) + ); +}); + +afterEach(() => { + vi.unstubAllEnvs(); +}); + +describe('createRateLimiter', () => { + describe('when Upstash env vars are missing', () => { + beforeEach(() => { + vi.stubEnv('UPSTASH_REDIS_REST_URL', undefined); + vi.stubEnv('UPSTASH_REDIS_REST_TOKEN', undefined); + }); + + it('fails open, does not call the limiter, and warns once', async () => { + const { createRateLimiter } = await importFresh(); + const check = createRateLimiter({ + requests: 5, + window: '60 s', + prefix: 'test', + }); + + const first = await check('1.2.3.4'); + const second = await check('1.2.3.4'); + + expect(first).toEqual({ success: true }); + expect(second).toEqual({ success: true }); + expect(mocks.loggerWarn).toHaveBeenCalledTimes(1); + expect(mocks.limit).not.toHaveBeenCalled(); + }); + }); + + describe('when Upstash env vars are present', () => { + beforeEach(() => { + vi.stubEnv('UPSTASH_REDIS_REST_URL', 'https://example.upstash.io'); + vi.stubEnv('UPSTASH_REDIS_REST_TOKEN', 'token'); + }); + + it('allows the request when under the limit', async () => { + mocks.limit.mockResolvedValueOnce({ success: true }); + const { createRateLimiter } = await importFresh(); + const check = createRateLimiter({ + requests: 5, + window: '60 s', + prefix: 'test', + }); + + expect(await check('1.2.3.4')).toEqual({ success: true }); + }); + + it('blocks the request when over the limit', async () => { + mocks.limit.mockResolvedValueOnce({ success: false }); + const { createRateLimiter } = await importFresh(); + const check = createRateLimiter({ + requests: 5, + window: '60 s', + prefix: 'test', + }); + + expect(await check('1.2.3.4')).toEqual({ success: false }); + }); + + it('passes the identifier through to the limiter', async () => { + mocks.limit.mockResolvedValueOnce({ success: true }); + const { createRateLimiter } = await importFresh(); + const check = createRateLimiter({ + requests: 5, + window: '60 s', + prefix: 'test', + }); + + await check('9.9.9.9'); + + expect(mocks.limit).toHaveBeenCalledTimes(1); + expect(mocks.limit).toHaveBeenCalledWith('9.9.9.9'); + }); + + it('reuses a single Redis client across limiters', async () => { + mocks.limit.mockResolvedValue({ success: true }); + const { createRateLimiter } = await importFresh(); + + const a = createRateLimiter({ + requests: 5, + window: '60 s', + prefix: 'a', + }); + const b = createRateLimiter({ + requests: 1, + window: '10 s', + prefix: 'b', + }); + await a('1.1.1.1'); + await b('2.2.2.2'); + + expect(mocks.fromEnv).toHaveBeenCalledTimes(1); + }); + + it('fails open and logs when Redis throws', async () => { + mocks.limit.mockRejectedValueOnce(new Error('redis down')); + const { createRateLimiter } = await importFresh(); + const check = createRateLimiter({ + requests: 5, + window: '60 s', + prefix: 'test', + }); + + expect(await check('1.2.3.4')).toEqual({ success: true }); + expect(mocks.loggerError).toHaveBeenCalledTimes(1); + }); + }); +}); + +describe('enforceRateLimit', () => { + beforeEach(() => { + vi.stubEnv('UPSTASH_REDIS_REST_URL', 'https://example.upstash.io'); + vi.stubEnv('UPSTASH_REDIS_REST_TOKEN', 'token'); + }); + + it('does not throw when the request is under the limit', async () => { + mocks.limit.mockResolvedValueOnce({ success: true }); + const { enforceRateLimit } = await importFresh(); + + await expect(enforceRateLimit()).resolves.toBeUndefined(); + expect(mocks.throwActionError).not.toHaveBeenCalled(); + // Resolves the client IP from x-forwarded-for and forwards it. + expect(mocks.limit).toHaveBeenCalledWith('1.2.3.4'); + }); + + it('throws an action error when the limit is exceeded', async () => { + mocks.limit.mockResolvedValueOnce({ success: false }); + const { enforceRateLimit } = await importFresh(); + + await expect(enforceRateLimit()).rejects.toThrow( + 'Too many requests. Please try again shortly.' + ); + expect(mocks.throwActionError).toHaveBeenCalledTimes(1); + }); + + it('uses a custom limiter and message when provided', async () => { + mocks.limit.mockResolvedValueOnce({ success: false }); + const { createRateLimiter, enforceRateLimit } = await importFresh(); + const strict = createRateLimiter({ + requests: 1, + window: '60 s', + prefix: 'strict', + }); + + await expect(enforceRateLimit(strict, 'Slow down.')).rejects.toThrow( + 'Slow down.' + ); + }); + + it('fails open (no throw) when Upstash is not configured', async () => { + vi.stubEnv('UPSTASH_REDIS_REST_URL', undefined); + vi.stubEnv('UPSTASH_REDIS_REST_TOKEN', undefined); + const { enforceRateLimit } = await importFresh(); + + await expect(enforceRateLimit()).resolves.toBeUndefined(); + expect(mocks.throwActionError).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/site/src/lib/rate-limit.ts b/apps/site/src/lib/rate-limit.ts new file mode 100644 index 00000000..a7e54ae9 --- /dev/null +++ b/apps/site/src/lib/rate-limit.ts @@ -0,0 +1,156 @@ +// Copyright © Todd Agriscience, Inc. All rights reserved. + +import { Ratelimit } from '@upstash/ratelimit'; +import { Redis } from '@upstash/redis'; +import { headers } from 'next/headers'; +import { logger } from '@/lib/logger'; +import { throwActionError } from '@/lib/utils/actions'; +import { getClientIp } from '@/lib/utils/get-client-ip'; + +/** + * Reusable IP-based rate limiting for public, unauthenticated endpoints. + * + * Backed by Upstash Redis via `@upstash/ratelimit`. A single Redis client is + * shared across every limiter. Everything fails open: when the Upstash + * environment variables are missing (local dev, CI, preview deployments) or + * when Redis itself errors, requests are allowed through so legitimate users + * are never blocked by infrastructure. + * + * Typical usage from a server action: + * + * ```ts + * await enforceRateLimit(); // default public-write limit + * await enforceRateLimit(myStricterLimit); // a custom preset + * ``` + */ + +/** A sliding-window duration accepted by Upstash (e.g. `'60 s'`, `'1 m'`). */ +export type RateLimitWindow = `${number} ${'ms' | 's' | 'm' | 'h' | 'd'}`; + +/** Configuration for a single named rate limiter. */ +export interface RateLimitConfig { + /** Requests permitted per identifier per window. */ + requests: number; + /** Sliding-window duration. */ + window: RateLimitWindow; + /** Redis key prefix namespacing this limiter's buckets. */ + prefix: string; +} + +/** Checks a single identifier against a limiter; resolves to its verdict. */ +export type RateLimitCheck = ( + identifier: string +) => Promise<{ success: boolean }>; + +/** Shared Redis client, or `null` once resolved as unavailable (fail open). */ +let redisClient: Redis | null = null; + +/** Whether the shared Redis client has been resolved yet. */ +let redisResolved = false; + +/** Ensures the "rate limiting disabled" warning is logged at most once. */ +let warnedMissingEnv = false; + +/** Per-prefix limiter cache so each limiter is constructed only once. */ +const limiterCache = new Map(); + +/** + * Lazily resolves the shared Upstash Redis client. + * + * Returns `null` when the Upstash environment variables are absent, signalling + * callers to fail open. A one-time warning is logged in that case. + * + * @returns The shared Redis client, or `null` when rate limiting is disabled. + */ +function getRedis(): Redis | null { + if (redisResolved) { + return redisClient; + } + + redisResolved = true; + + if ( + !process.env.UPSTASH_REDIS_REST_URL || + !process.env.UPSTASH_REDIS_REST_TOKEN + ) { + if (!warnedMissingEnv) { + warnedMissingEnv = true; + logger.warn( + '[rate-limit] Upstash env vars missing; rate limiting disabled (failing open).' + ); + } + return null; + } + + redisClient = Redis.fromEnv(); + return redisClient; +} + +/** + * Builds a reusable rate-limit checker for the given configuration. + * + * The returned function checks one identifier (typically a client IP) against a + * sliding window. Limiters are cached per `prefix` and share one Redis client. + * Fails open when Upstash is unconfigured or when the Redis call throws. + * + * @param config - Requests, window, and key prefix for this limiter. + * @returns A checker resolving `{ success }` for a given identifier. + */ +export function createRateLimiter(config: RateLimitConfig): RateLimitCheck { + return async (identifier: string) => { + const redis = getRedis(); + if (!redis) { + return { success: true }; + } + + let limiter = limiterCache.get(config.prefix); + if (!limiter) { + limiter = new Ratelimit({ + redis, + limiter: Ratelimit.slidingWindow(config.requests, config.window), + prefix: config.prefix, + }); + limiterCache.set(config.prefix, limiter); + } + + try { + const { success } = await limiter.limit(identifier); + return { success }; + } catch (error) { + logger.error('[rate-limit] Redis error; failing open.', error); + return { success: true }; + } + }; +} + +/** + * Default limiter for public, unauthenticated write endpoints: 5 requests per + * IP per 60 seconds. Reuse this across the contact form, CMS forms, and any + * other public write action that should share one budget. + */ +export const publicWriteRateLimit = createRateLimiter({ + requests: 5, + window: '60 s', + prefix: 'site:public-write', +}); + +/** + * Enforces a rate limit for the current server action request. + * + * Resolves the client IP from request headers, checks it against the given + * limiter, and throws an action error when the limit is exceeded. This is the + * one-line guard server actions should call before doing any work. + * + * @param check - Limiter to enforce. Defaults to {@link publicWriteRateLimit}. + * @param message - Error surfaced to the client when rate limited. + */ +export async function enforceRateLimit( + check: RateLimitCheck = publicWriteRateLimit, + message = 'Too many requests. Please try again shortly.' +): Promise { + const ip = getClientIp(await headers()); + const { success } = await check(ip); + if (!success) { + throwActionError(message); + } +} diff --git a/apps/site/src/lib/utils/get-client-ip.test.ts b/apps/site/src/lib/utils/get-client-ip.test.ts new file mode 100644 index 00000000..84febcbf --- /dev/null +++ b/apps/site/src/lib/utils/get-client-ip.test.ts @@ -0,0 +1,26 @@ +// Copyright © Todd Agriscience, Inc. All rights reserved. + +import { describe, expect, it } from 'vitest'; +import { getClientIp } from './get-client-ip'; + +describe('getClientIp', () => { + it('returns the first IP from a comma-separated x-forwarded-for header', () => { + const headers = new Headers({ 'x-forwarded-for': '1.1.1.1, 2.2.2.2' }); + expect(getClientIp(headers)).toBe('1.1.1.1'); + }); + + it('returns the single IP when only one is present', () => { + const headers = new Headers({ 'x-forwarded-for': '3.3.3.3' }); + expect(getClientIp(headers)).toBe('3.3.3.3'); + }); + + it('falls back to "unknown" when the header is absent', () => { + const headers = new Headers(); + expect(getClientIp(headers)).toBe('unknown'); + }); + + it('falls back to "unknown" when the header is empty', () => { + const headers = new Headers({ 'x-forwarded-for': ' ' }); + expect(getClientIp(headers)).toBe('unknown'); + }); +}); diff --git a/apps/site/src/lib/utils/get-client-ip.ts b/apps/site/src/lib/utils/get-client-ip.ts new file mode 100644 index 00000000..5b864e2d --- /dev/null +++ b/apps/site/src/lib/utils/get-client-ip.ts @@ -0,0 +1,27 @@ +// Copyright © Todd Agriscience, Inc. All rights reserved. + +/** Fallback identifier used when no client IP can be determined. */ +const UNKNOWN_CLIENT_IP = 'unknown'; + +/** + * Extracts the originating client IP address from request headers. + * + * Reads the `x-forwarded-for` header (set by Vercel / upstream proxies), which + * may contain a comma-separated list of IPs; the first entry is the original + * client. Accepts a `Headers` object so callers can pass the result of + * `await headers()` and so it is trivially unit-testable. + * + * @param headerList - Request headers to read from. + * @returns The trimmed client IP, or `'unknown'` when unavailable. + */ +export function getClientIp(headerList: Headers): string { + const forwardedFor = headerList.get('x-forwarded-for'); + if (!forwardedFor) { + return UNKNOWN_CLIENT_IP; + } + + const [first] = forwardedFor.split(','); + const ip = first?.trim(); + + return ip ? ip : UNKNOWN_CLIENT_IP; +} diff --git a/bun.lock b/bun.lock index 6133f234..15e141f3 100644 --- a/bun.lock +++ b/bun.lock @@ -130,6 +130,8 @@ "@supabase/ssr": "^0.10.0", "@supabase/supabase-js": "^2.102.1", "@types/ua-parser-js": "^0.7.39", + "@upstash/ratelimit": "^2.0.8", + "@upstash/redis": "^1.38.0", "class-variance-authority": "^0.7.1", "clsx": "^2.1.1", "drizzle-orm": "^0.45.2", @@ -1674,6 +1676,12 @@ "@unrs/resolver-binding-win32-x64-msvc": ["@unrs/resolver-binding-win32-x64-msvc@1.11.1", "", { "os": "win32", "cpu": "x64" }, "sha512-lrW200hZdbfRtztbygyaq/6jP6AKE8qQN2KvPcJ+x7wiD038YtnYtZ82IMNJ69GJibV7bwL3y9FgK+5w/pYt6g=="], + "@upstash/core-analytics": ["@upstash/core-analytics@0.0.10", "", { "dependencies": { "@upstash/redis": "^1.28.3" } }, "sha512-7qJHGxpQgQr9/vmeS1PktEwvNAF7TI4iJDi8Pu2CFZ9YUGHZH4fOP5TfYlZ4aVxfopnELiE4BS4FBjyK7V1/xQ=="], + + "@upstash/ratelimit": ["@upstash/ratelimit@2.0.8", "", { "dependencies": { "@upstash/core-analytics": "^0.0.10" }, "peerDependencies": { "@upstash/redis": "^1.34.3" } }, "sha512-YSTMBJ1YIxsoPkUMX/P4DDks/xV5YYCswWMamU8ZIfK9ly6ppjRnVOyBhMDXBmzjODm4UQKcxsJPvaeFAijp5w=="], + + "@upstash/redis": ["@upstash/redis@1.38.0", "", { "dependencies": { "uncrypto": "^0.1.3" } }, "sha512-wu+dZBptlLy0+MCUEoHmzrY/TnmgDey3+c7EbIGwrLqAvkP8yi5MWZHYGIFtAygmL4Bkz2TdFu+eU0vFPncIcg=="], + "@vercel/edge": ["@vercel/edge@1.2.2", "", {}, "sha512-1+y+f6rk0Yc9ss9bRDgz/gdpLimwoRteKHhrcgHvEpjbP1nyT3ByqEMWm2BTcpIO5UtDmIFXc8zdq4LR190PDA=="], "@vercel/error-utils": ["@vercel/error-utils@2.0.3", "", {}, "sha512-CqC01WZxbLUxoiVdh9B/poPbNpY9U+tO1N9oWHwTl5YAZxcqXmmWJ8KNMFItJCUUWdY3J3xv8LvAuQv2KZ5YdQ=="], @@ -3468,6 +3476,8 @@ "unbox-primitive": ["unbox-primitive@1.1.0", "", { "dependencies": { "call-bound": "^1.0.3", "has-bigints": "^1.0.2", "has-symbols": "^1.1.0", "which-boxed-primitive": "^1.1.1" } }, "sha512-nWJ91DjeOkej/TA8pXQ3myruKpKEYgqvpw9lz4OPHj/NWFNluYrjbz9j01CJ8yKQd2g4jFoOkINCTW2I5LEEyw=="], + "uncrypto": ["uncrypto@0.1.3", "", {}, "sha512-Ql87qFHB3s/De2ClA9e0gsnS6zXG27SkTiSJwjCc9MebbfapQfuPzumMIUMi38ezPZVNFcHI9sUIepeQfw8J8Q=="], + "undici": ["undici@7.28.0", "", {}, "sha512-cRZYrTDwWznlnRiPjggAGxZXanty6M8RV1ff8Wm4LWXBp7/IG8v5DnOm74DtUBp9OONpK75YlPnIjQqX0dBDtA=="], "undici-types": ["undici-types@7.19.2", "", {}, "sha512-qYVnV5OEm2AW8cJMCpdV20CDyaN3g0AjDlOGf1OW4iaDEx8MwdtChUp4zu4H0VP3nDRF/8RKWH+IPp9uW0YGZg=="], From d5b5ac7315be6d6392d579b41037fcd14e47a932 Mon Sep 17 00:00:00 2001 From: C0dertang Date: Tue, 23 Jun 2026 14:52:59 -0700 Subject: [PATCH 2/3] test(site): add 100-req/s burst tests for rate limiter (#936) Simulate a ~100 requests/second flood against the public-write limiter using a stateful sliding-window fake: one IP firing 100 concurrent requests gets exactly 5 allowed and 95 blocked, and 100 requests spread across 10 IPs give each IP its own budget (50 allowed / 50 blocked). Proves the limiter caps bursts and keys per identifier. Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/site/src/lib/rate-limit.test.ts | 57 ++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/apps/site/src/lib/rate-limit.test.ts b/apps/site/src/lib/rate-limit.test.ts index bdd2819e..75996dfe 100644 --- a/apps/site/src/lib/rate-limit.test.ts +++ b/apps/site/src/lib/rate-limit.test.ts @@ -163,6 +163,63 @@ describe('createRateLimiter', () => { expect(await check('1.2.3.4')).toEqual({ success: true }); expect(mocks.loggerError).toHaveBeenCalledTimes(1); }); + + it('under a ~100 requests/second burst from one IP, allows only the configured limit and blocks the rest', async () => { + // Stateful fake mirroring a sliding window: the first `LIMIT` hits from an + // identifier within the window succeed; every subsequent hit is blocked. + const LIMIT = 5; + const counts = new Map(); + mocks.limit.mockImplementation(async (id: string) => { + const next = (counts.get(id) ?? 0) + 1; + counts.set(id, next); + return { success: next <= LIMIT }; + }); + + const { createRateLimiter } = await importFresh(); + const check = createRateLimiter({ + requests: LIMIT, + window: '60 s', + prefix: 'burst', + }); + + // Fire 100 concurrent requests in the same second from the same IP. + const REQUESTS = 100; + const results = await Promise.all( + Array.from({ length: REQUESTS }, () => check('1.2.3.4')) + ); + const allowed = results.filter((r) => r.success).length; + const blocked = results.filter((r) => !r.success).length; + + expect(allowed).toBe(LIMIT); + expect(blocked).toBe(REQUESTS - LIMIT); + expect(mocks.limit).toHaveBeenCalledTimes(REQUESTS); + }); + + it('gives each distinct IP its own budget under a burst', async () => { + const LIMIT = 5; + const counts = new Map(); + mocks.limit.mockImplementation(async (id: string) => { + const next = (counts.get(id) ?? 0) + 1; + counts.set(id, next); + return { success: next <= LIMIT }; + }); + + const { createRateLimiter } = await importFresh(); + const check = createRateLimiter({ + requests: LIMIT, + window: '60 s', + prefix: 'burst', + }); + + // 100 requests split across 10 IPs → each IP stays under its own limit. + const results = await Promise.all( + Array.from({ length: 100 }, (_, i) => check(`10.0.0.${i % 10}`)) + ); + + // 10 IPs × 5 allowed each = 50 allowed, 50 blocked. + expect(results.filter((r) => r.success).length).toBe(50); + expect(results.filter((r) => !r.success).length).toBe(50); + }); }); }); From 2906432aa039223784465424ed2cea754c2de954 Mon Sep 17 00:00:00 2001 From: C0dertang Date: Tue, 23 Jun 2026 15:04:53 -0700 Subject: [PATCH 3/3] refactor(site): harden rate limiter from code review (#936) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - getRedis now wraps Redis.fromEnv() in try/catch so a client-construction error fails open (matching the documented contract) instead of throwing. - Replace the global prefix-keyed limiter cache with a per-factory closure limiter. Removes a latent bug where two createRateLimiter calls sharing a prefix but differing in requests/window would silently reuse the first. - Document the x-forwarded-for trust assumption in getClientIp (proxy-set, defense-in-depth, fail-open — not for security-critical decisions). Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/site/src/lib/rate-limit.ts | 27 +++++++++++++++--------- apps/site/src/lib/utils/get-client-ip.ts | 6 ++++++ 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/apps/site/src/lib/rate-limit.ts b/apps/site/src/lib/rate-limit.ts index a7e54ae9..44c29449 100644 --- a/apps/site/src/lib/rate-limit.ts +++ b/apps/site/src/lib/rate-limit.ts @@ -51,14 +51,12 @@ let redisResolved = false; /** Ensures the "rate limiting disabled" warning is logged at most once. */ let warnedMissingEnv = false; -/** Per-prefix limiter cache so each limiter is constructed only once. */ -const limiterCache = new Map(); - /** * Lazily resolves the shared Upstash Redis client. * - * Returns `null` when the Upstash environment variables are absent, signalling - * callers to fail open. A one-time warning is logged in that case. + * Returns `null` when the Upstash environment variables are absent, or if + * constructing the client throws — in both cases callers fail open. The + * resolution runs once and the result (client or `null`) is cached. * * @returns The shared Redis client, or `null` when rate limiting is disabled. */ @@ -82,7 +80,15 @@ function getRedis(): Redis | null { return null; } - redisClient = Redis.fromEnv(); + try { + redisClient = Redis.fromEnv(); + } catch (error) { + logger.error( + '[rate-limit] Failed to initialize Upstash Redis; failing open.', + error + ); + redisClient = null; + } return redisClient; } @@ -90,27 +96,28 @@ function getRedis(): Redis | null { * Builds a reusable rate-limit checker for the given configuration. * * The returned function checks one identifier (typically a client IP) against a - * sliding window. Limiters are cached per `prefix` and share one Redis client. - * Fails open when Upstash is unconfigured or when the Redis call throws. + * sliding window. Each call to this factory owns its own limiter (built lazily + * on first use, after the shared Redis client resolves); all limiters share one + * Redis client. Fails open when Upstash is unconfigured or the Redis call throws. * * @param config - Requests, window, and key prefix for this limiter. * @returns A checker resolving `{ success }` for a given identifier. */ export function createRateLimiter(config: RateLimitConfig): RateLimitCheck { + let limiter: Ratelimit | null = null; + return async (identifier: string) => { const redis = getRedis(); if (!redis) { return { success: true }; } - let limiter = limiterCache.get(config.prefix); if (!limiter) { limiter = new Ratelimit({ redis, limiter: Ratelimit.slidingWindow(config.requests, config.window), prefix: config.prefix, }); - limiterCache.set(config.prefix, limiter); } try { diff --git a/apps/site/src/lib/utils/get-client-ip.ts b/apps/site/src/lib/utils/get-client-ip.ts index 5b864e2d..74a26ede 100644 --- a/apps/site/src/lib/utils/get-client-ip.ts +++ b/apps/site/src/lib/utils/get-client-ip.ts @@ -11,6 +11,12 @@ const UNKNOWN_CLIENT_IP = 'unknown'; * client. Accepts a `Headers` object so callers can pass the result of * `await headers()` and so it is trivially unit-testable. * + * Trust note: `x-forwarded-for` is client-supplied and only trustworthy because + * the hosting proxy (Vercel) sets it. Used here as a rate-limit key for + * defense-in-depth — not for auth — and the limiter fails open, so a spoofed + * header cannot deny service to legitimate users. Do not rely on this value for + * any security-critical decision without a trusted-proxy guarantee. + * * @param headerList - Request headers to read from. * @returns The trimmed client IP, or `'unknown'` when unavailable. */