-
Notifications
You must be signed in to change notification settings - Fork 3.6k
feat(auth): per-IP signup rate limit with Redis-backed storage #4828
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: staging
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -38,6 +38,7 @@ import { | |||||
| resolveClientMetadata, | ||||||
| upsertCimdClient, | ||||||
| } from '@/lib/auth/cimd' | ||||||
| import { buildRedisRateLimitStorage } from '@/lib/auth/rate-limit-storage' | ||||||
| import { sendPlanWelcomeEmail } from '@/lib/billing' | ||||||
| import { authorizeSubscriptionReference } from '@/lib/billing/authorization' | ||||||
| import { | ||||||
|
|
@@ -168,6 +169,20 @@ function isSignupEmailBlocked(email: string | undefined | null): boolean { | |||||
| return isEmailInDenylist(email, blockedSignupDomains) | ||||||
| } | ||||||
|
|
||||||
| const DEFAULT_SIGNUP_RATE_LIMIT_WINDOW_SECONDS = 3600 | ||||||
| const DEFAULT_SIGNUP_RATE_LIMIT_MAX = 20 | ||||||
|
|
||||||
| const signupRateLimitWindowSeconds = | ||||||
| Number(env.SIGNUP_RATE_LIMIT_WINDOW_SECONDS) || DEFAULT_SIGNUP_RATE_LIMIT_WINDOW_SECONDS | ||||||
| const signupRateLimitMax = Number(env.SIGNUP_RATE_LIMIT_MAX) || DEFAULT_SIGNUP_RATE_LIMIT_MAX | ||||||
|
|
||||||
| /** | ||||||
| * Shared store for the auth rate limiter. Redis-backed when configured so per-IP | ||||||
| * counts hold across replicas and deploys; otherwise `undefined`, leaving | ||||||
| * better-auth on its default in-memory store. | ||||||
| */ | ||||||
| const authRateLimitStorage = buildRedisRateLimitStorage() | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
|
||||||
| const additionalTrustedOrigins = parseOriginList(env.TRUSTED_ORIGINS, (value) => | ||||||
| logger.warn('Ignoring invalid entry in TRUSTED_ORIGINS', { value }) | ||||||
| ) | ||||||
|
|
@@ -204,6 +219,28 @@ export const auth = betterAuth({ | |||||
| provider: 'pg', | ||||||
| schema, | ||||||
| }), | ||||||
| rateLimit: { | ||||||
| // Mirror better-auth's default (on in production, off in dev/test) so local | ||||||
| // signup flows stay unthrottled. | ||||||
| enabled: env.NODE_ENV === 'production', | ||||||
| customRules: { | ||||||
| // Long-window per-IP cap on email signup. better-auth's built-in rule only | ||||||
| // guards bursts (3 / 10s), which a slow drip from one IP sails past; this | ||||||
| // bounds sustained abuse. Deliberately generous — an enterprise office | ||||||
| // behind a single NAT signs up well under this, and SSO / org-invite | ||||||
| // onboarding hit different endpoints that this rule does not touch. | ||||||
| '/sign-up/email': { window: signupRateLimitWindowSeconds, max: signupRateLimitMax }, | ||||||
| }, | ||||||
| ...(authRateLimitStorage ? { customStorage: authRateLimitStorage } : {}), | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||||||
| }, | ||||||
| advanced: { | ||||||
| ipAddress: { | ||||||
| // Prefer Cloudflare's client IP (set by the edge, not forgeable by the | ||||||
| // caller) so the rate-limit key is the real client. Fall back to | ||||||
| // X-Forwarded-For for deployments without Cloudflare in front. | ||||||
| ipAddressHeaders: ['cf-connecting-ip', 'x-forwarded-for'], | ||||||
| }, | ||||||
| }, | ||||||
| session: { | ||||||
| cookieCache: { | ||||||
| enabled: true, | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| /** | ||
| * @vitest-environment node | ||
| */ | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest' | ||
|
|
||
| const { mockGetRedisClient, redis } = vi.hoisted(() => { | ||
| const redis = { | ||
| get: vi.fn(), | ||
| set: vi.fn(), | ||
| } | ||
| return { mockGetRedisClient: vi.fn(), redis } | ||
| }) | ||
|
|
||
| vi.mock('@/lib/core/config/redis', () => ({ | ||
| getRedisClient: mockGetRedisClient, | ||
| })) | ||
|
|
||
| import { buildRedisRateLimitStorage } from '@/lib/auth/rate-limit-storage' | ||
|
|
||
| describe('buildRedisRateLimitStorage', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
| mockGetRedisClient.mockReturnValue(redis) | ||
| }) | ||
|
|
||
| it('returns undefined when Redis is not configured (falls back to in-memory)', () => { | ||
| mockGetRedisClient.mockReturnValue(null) | ||
| expect(buildRedisRateLimitStorage()).toBeUndefined() | ||
| }) | ||
|
|
||
| it('reads and parses a stored counter', async () => { | ||
| const storage = buildRedisRateLimitStorage() | ||
| redis.get.mockResolvedValue(JSON.stringify({ key: 'k', count: 4, lastRequest: 123 })) | ||
| const value = await storage?.get('k') | ||
| expect(redis.get).toHaveBeenCalledWith('auth-rl:k') | ||
| expect(value).toEqual({ key: 'k', count: 4, lastRequest: 123 }) | ||
| }) | ||
|
|
||
| it('returns undefined when no counter is stored', async () => { | ||
| const storage = buildRedisRateLimitStorage() | ||
| redis.get.mockResolvedValue(null) | ||
| expect(await storage?.get('missing')).toBeUndefined() | ||
| }) | ||
|
|
||
| it('writes the counter with a bounding TTL', async () => { | ||
| const storage = buildRedisRateLimitStorage() | ||
| await storage?.set('k', { key: 'k', count: 1, lastRequest: 999 }) | ||
| expect(redis.set).toHaveBeenCalledWith( | ||
| 'auth-rl:k', | ||
| JSON.stringify({ key: 'k', count: 1, lastRequest: 999 }), | ||
| 'EX', | ||
| 3600 | ||
| ) | ||
| }) | ||
|
|
||
| it('fails open on a read error (allows the request)', async () => { | ||
| const storage = buildRedisRateLimitStorage() | ||
| redis.get.mockRejectedValue(new Error('redis down')) | ||
| expect(await storage?.get('k')).toBeUndefined() | ||
| }) | ||
|
|
||
| it('swallows write errors so a Redis outage never blocks auth', async () => { | ||
| const storage = buildRedisRateLimitStorage() | ||
| redis.set.mockRejectedValue(new Error('redis down')) | ||
| await expect(storage?.set('k', { key: 'k', count: 1, lastRequest: 1 })).resolves.toBeUndefined() | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,72 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { createLogger } from '@sim/logger' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { getErrorMessage } from '@sim/utils/errors' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { getRedisClient } from '@/lib/core/config/redis' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const logger = createLogger('AuthRateLimitStorage') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** Counter shape better-auth persists per rate-limit key. */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| interface RateLimitRecord { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| key: string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| count: number | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| lastRequest: number | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** Structural match for better-auth's `rateLimit.customStorage` option. */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| interface RateLimitStorage { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| get: (key: string) => Promise<RateLimitRecord | undefined> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| set: (key: string, value: RateLimitRecord, update?: boolean) => Promise<void> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const REDIS_KEY_PREFIX = 'auth-rl:' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * TTL for stored counters. Correctness comes from the `lastRequest` timestamp | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * comparison in better-auth's limiter, not from expiry — the TTL only bounds | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * key growth. It must be at least as long as the largest configured window so a | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * key never expires mid-window and under-counts. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const REDIS_TTL_SECONDS = 3600 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Redis-backed storage for better-auth's rate limiter. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Returns `undefined` when no Redis is configured, in which case better-auth | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * falls back to its default in-memory store. That store is per-process and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * resets on deploy, so a long-window per-IP cap only holds across a multi-replica | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * deployment when counts live in shared storage. Backing the limiter with Redis | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * (rather than the primary database) keeps the per-request counter I/O off the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Postgres primary. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Fail-open: any Redis error resolves `get` to `undefined` (treated as no prior | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * requests) and makes `set` a no-op, so a Redis outage degrades to "allow" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * rather than locking users out of authentication. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function buildRedisRateLimitStorage(): RateLimitStorage | undefined { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+22
to
+44
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!getRedisClient()) return undefined | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| get: async (key) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const redis = getRedisClient() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!redis) return undefined | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const raw = await redis.get(`${REDIS_KEY_PREFIX}${key}`) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!raw) return undefined | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return JSON.parse(raw) as RateLimitRecord | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.warn('Rate-limit storage read failed; allowing request', { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| error: getErrorMessage(error), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return undefined | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| set: async (key, value) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const redis = getRedisClient() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!redis) return | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await redis.set(`${REDIS_KEY_PREFIX}${key}`, JSON.stringify(value), 'EX', REDIS_TTL_SECONDS) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Redis TTL ignores signup windowMedium Severity
Additional Locations (1)Reviewed by Cursor Bugbot for commit 75c2c6d. Configure here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.warn('Rate-limit storage write failed', { error: getErrorMessage(error) }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SIGNUP_RATE_LIMIT_MAXorSIGNUP_RATE_LIMIT_WINDOW_SECONDSis set to"0",Number("0")evaluates to0, which is falsy, so the||fallback silently restores the default instead of respecting the operator's explicit zero.