Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions apps/sim/lib/auth/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Comment on lines +175 to +177
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 When SIGNUP_RATE_LIMIT_MAX or SIGNUP_RATE_LIMIT_WINDOW_SECONDS is set to "0", Number("0") evaluates to 0, which is falsy, so the || fallback silently restores the default instead of respecting the operator's explicit zero.

Suggested change
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
const signupRateLimitWindowSeconds =
(env.SIGNUP_RATE_LIMIT_WINDOW_SECONDS !== undefined
? Number(env.SIGNUP_RATE_LIMIT_WINDOW_SECONDS)
: NaN) || DEFAULT_SIGNUP_RATE_LIMIT_WINDOW_SECONDS
const signupRateLimitMax =
(env.SIGNUP_RATE_LIMIT_MAX !== undefined ? Number(env.SIGNUP_RATE_LIMIT_MAX) : NaN) ||
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()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Thread signupRateLimitWindowSeconds into buildRedisRateLimitStorage so the Redis TTL is always at least as long as the configured window, preventing mid-window key expiry when the window exceeds the current hardcoded 3600 s.

Suggested change
const authRateLimitStorage = buildRedisRateLimitStorage()
const authRateLimitStorage = buildRedisRateLimitStorage(signupRateLimitWindowSeconds)


const additionalTrustedOrigins = parseOriginList(env.TRUSTED_ORIGINS, (value) =>
logger.warn('Ignoring invalid entry in TRUSTED_ORIGINS', { value })
)
Expand Down Expand Up @@ -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 } : {}),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 customStorage scopes wider than /sign-up/email

customStorage is a top-level rateLimit option in better-auth and applies to all rate-limit rules — including the built-in burst-protection rule (/sign-up* at 3 req / 10 s) and any other special rules the library applies globally. The PR description presents this as targeted signup storage, but in practice every auth-path rate-limit key hits Redis on every auth request. This is likely fine for the intended deployment, but worth being deliberate: if only the long-window signup counter should be Redis-backed, better-auth doesn't currently offer per-rule storage isolation (confirmed in issue #4497).

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,
Expand Down
67 changes: 67 additions & 0 deletions apps/sim/lib/auth/rate-limit-storage.test.ts
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()
})
})
72 changes: 72 additions & 0 deletions apps/sim/lib/auth/rate-limit-storage.ts
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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 The TTL is hardcoded to 3600 but the rate-limit window is operator-configurable via SIGNUP_RATE_LIMIT_WINDOW_SECONDS. The code comment correctly states "it must be at least as long as the largest configured window so a key never expires mid-window and under-counts" — but the hardcoded value violates that guarantee for any window > 3600 s. If an operator sets SIGNUP_RATE_LIMIT_WINDOW_SECONDS=7200, keys expire after one hour, the counter resets mid-window, and the IP gets a fresh 20-signup budget in the second hour. Accepting ttlSeconds as a parameter (with a 3600 default) and threading signupRateLimitWindowSeconds from auth.ts into the call site fixes this.

Suggested change
/**
* 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 {
/**
* 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.
*
* @param ttlSeconds Redis key TTL. Must be the largest configured rate-limit
* window so keys never expire mid-window and under-count. Defaults to 3600 s.
*/
export function buildRedisRateLimitStorage(ttlSeconds = 3600): RateLimitStorage | undefined {

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)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redis TTL ignores signup window

Medium Severity

REDIS_TTL_SECONDS is fixed at 3600 while /sign-up/email uses SIGNUP_RATE_LIMIT_WINDOW_SECONDS, which can be longer. When a Redis key expires before that window ends, get returns nothing and the per-IP counter resets, so the sustained signup cap is weaker than configured.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 75c2c6d. Configure here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Update the set implementation to use the ttlSeconds parameter instead of the now-removed module constant.

Suggested change
await redis.set(`${REDIS_KEY_PREFIX}${key}`, JSON.stringify(value), 'EX', REDIS_TTL_SECONDS)
await redis.set(`${REDIS_KEY_PREFIX}${key}`, JSON.stringify(value), 'EX', ttlSeconds)

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) })
}
},
}
}
2 changes: 2 additions & 0 deletions apps/sim/lib/core/config/env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ export const env = createEnv({
BLOCKED_SIGNUP_DOMAINS: z.string().optional(), // Comma-separated list of email domains blocked from signing up (e.g., "gmail.com,yahoo.com")
SIGNUP_MX_VALIDATION_ENABLED: z.boolean().optional(), // Opt-in: validate the email's MX backend at signup (blocks no-MX domains and denylisted shared spam backends). Off by default; enable on hosted/abuse-targeted deployments.
BLOCKED_EMAIL_MX_HOSTS: z.string().optional(), // Comma-separated MX-host substrings blocked from signing up; matched against the domain's resolved MX backend to catch throwaway domains that share a mail backend. No defaults — operators supply their own list. Only used when SIGNUP_MX_VALIDATION_ENABLED is set.
SIGNUP_RATE_LIMIT_WINDOW_SECONDS: z.string().optional(), // Per-IP rate-limit window (seconds) for /sign-up/email. Default 3600 (1h). Backed by Redis when REDIS_URL is set (consistent across replicas); falls back to better-auth's in-memory store otherwise.
SIGNUP_RATE_LIMIT_MAX: z.string().optional(), // Max email signups per IP per window before HTTP 429. Default 20 — generous for enterprise office NATs; SSO and org-invite onboarding use different endpoints and are unaffected.
TRUSTED_ORIGINS: z.string().optional(), // Comma-separated additional origins to trust for auth (e.g., "https://app.example.com,https://www.example.com"). Merged into Better Auth trustedOrigins.
TURNSTILE_SECRET_KEY: z.string().min(1).optional(), // Cloudflare Turnstile secret key for captcha verification
SIGNUP_EMAIL_VALIDATION_ENABLED: z.boolean().optional(), // Enable disposable email blocking via better-auth-harmony (55K+ domains)
Expand Down
Loading