diff --git a/docs/DISTRIBUTED_RATE_LIMITING.md b/docs/DISTRIBUTED_RATE_LIMITING.md new file mode 100644 index 00000000..797dcf9c --- /dev/null +++ b/docs/DISTRIBUTED_RATE_LIMITING.md @@ -0,0 +1,236 @@ +# Distributed rate-limit correctness + +The API rate limiter is a shared security boundary. A process-local counter +allows a client to multiply its budget by sending requests to different +instances, and a multi-command increment/expiry sequence can leave a bucket +without an expiry if a worker stops between commands. + +## Guarantees + +The middleware now uses one Redis Lua script for each bucket decision. Redis +executes the script atomically on its command thread: + +1. increment the fixed-window counter; +2. set the expiry only for the first increment; and +3. read the remaining TTL for response headers. + +The limit decision is made from the returned count. If 10 requests are allowed, +exactly 10 requests can observe a count at or below 10 in a shared window; the +11th and later requests receive `429`. Multiple application instances use the +same Redis namespace and therefore share this decision. + +Small test adapters that only implement `INCR`, `EXPIRE`, and `TTL` remain +supported through a compatibility path. The production `redis` client exposes +`EVAL`, so deployed instances use the atomic path. An adapter for a production +deployment should implement `eval`; the fallback must not be mistaken for a +cross-process transaction. + +## Key dimensions + +Each tenant bucket includes both the resolved identity and the HTTP route. The +identity is an authenticated tenant/API-key owner where available, otherwise +the hashed API key, bearer token, or socket peer fallback. Each API-key bucket +includes the key ID, subscription tier, and route. The route dimension uses the +HTTP method, Express base URL, and path. + +The user-controlled dimensions are hashed before being placed in Redis keys. +This provides three useful properties: + +- tenant A cannot collide with tenant B because their complete identities are + independently represented in the digest; +- `/api/report` cannot exhaust `/api/export` when route-scoped limits are used; + and +- arbitrary tenant IDs and paths cannot create unbounded or separator- + ambiguous Redis key names. + +The digest is a key representation, not an authorization decision. Tenant +identity must still come from trusted authentication or gateway context. A +caller-provided tenant header is not used by the production extractor. + +## Two-bucket policy + +For an authenticated API key, the middleware evaluates: + +| Bucket | Dimensions | Purpose | +| --- | --- | --- | +| Tenant | namespace, tenant identity, route | Shared ceiling for all keys owned by a tenant. | +| API key | namespace, key ID, tier, route | Prevents one key from consuming the tenant's per-key allowance. | + +The tenant bucket is checked first. This preserves the existing rejection +precedence and avoids doing a second operation when the tenant has already +exhausted its shared budget. Remaining headers report the tighter of the two +budgets when both buckets are present. + +Tier changes are part of the API-key bucket identity. A key upgraded from free +to pro receives a fresh tier-scoped bucket instead of inheriting a counter +created under the old ceiling. Tenant overrides replace the tier limit and +window for the tenant bucket decision, and the selected window is passed to +the atomic script. + +## Forwarded-IP threat model + +Application code must not use the leftmost `X-Forwarded-For` value as the +security identity. When a request reaches the process through a reverse proxy, +that value can be supplied by the caller and changed on every attempt. + +`getClientIp` uses `req.socket.remoteAddress`, the TCP peer that delivered the +request. In a correctly configured deployment this is the trusted proxy; when +the application is directly exposed it is the client address. Proxy +normalization belongs at the trusted edge. Express's `trust proxy` setting +must be reviewed together with the network topology before deployment. + +## Dependency failure + +Redis failure is an explicit policy decision: + +- `failOpen: false` returns `503 SERVICE_UNAVAILABLE` and blocks the request. + Use this for sensitive routes where silently disabling protection is unsafe. +- `failOpen: true` allows the request through with headers showing the full + configured budget. Use this only for routes where availability is more + important than strict abuse prevention, and alert on the degraded mode. + +Both modes are observable through the `rate_limit_rejected_total` metric for +fail-closed rejection and application logs/health signals for dependency +health. A Redis outage must not silently look like a successful rate-limit +decision. + +Tenant override lookup failure is handled separately: the middleware falls +back to the configured tier ceiling, then still performs the shared Redis +check. This avoids making a control-plane outage disable the data-plane guard. + +## Expiry and boundary behavior + +The first increment assigns the configured expiry inside the same atomic +script. Subsequent increments do not extend the fixed window. If the key has +expired, Redis starts a new window and the first increment assigns a new TTL. + +The middleware reports a positive TTL in `Retry-After` and `X-RateLimit-Reset`. +If Redis reports a non-positive TTL due to an expiry race, the implementation +uses the configured window as a safe header fallback. It never treats a missing +TTL as permission to skip the counter. + +Fixed windows intentionally have a boundary burst characteristic: traffic at +the end of one window and the beginning of the next can be admitted by two +different windows. This is compatible with the existing API contract. A future +sliding-window design would be a separate migration because it changes quotas, +Redis data shape, and client retry timing. + +## Compatibility and migration + +The public middleware factory and `rateLimit` helper signatures remain +compatible. Existing minimal Redis test doubles continue to work. Redis key +names now use the `v2` segment and include route scope, so existing counters +are intentionally not carried into the new representation. This gives every +deployment a clean window after rollout and avoids mixing old and new key +semantics. + +No database migration is required for the counter itself. Tenant override +records continue to use the existing `tenant_rate_limit_overrides` table. The +application should deploy the code and Redis script support together; a mixed +fleet may temporarily use the compatibility command path while instances are +upgraded. + +## Rollback + +Rollback to the previous application version is safe at the data-schema level, +but it changes the key namespace and can restore the previous weaker race +behavior. During a rollback, operators should expect fresh counters and monitor +request rejection rates. Do not manually delete all Redis keys as a routine +rollback step; expiry will retire the versioned buckets naturally. + +If Redis is unavailable during rollout, keep fail-closed behavior for sensitive +routes. If availability requires fail-open for a non-sensitive route, record +that decision in deployment configuration and set an alert with an owner and +time limit. + +## Validation matrix + +The distributed rate-limit tests exercise: + +- concurrent requests distributed across two middleware instances sharing one + Redis object; +- exact admission at the limit and rejection beyond it; +- atomic script contents and one-key execution; +- tenant isolation and collision resistance at the middleware boundary; +- route isolation for distinct HTTP paths; +- tenant-specific limits and TTLs; +- fixed-window expiry and reset; +- forwarded-IP spoofing attempts; +- fail-closed dependency behavior; and +- explicit fail-open behavior. + +The existing route suite remains unchanged and continues to cover tier limits, +per-key limits, tenant overrides, headers, metrics, fallback adapters, and +legacy helper behavior. No security or CI check is disabled by this change. + +## Operational checklist + +Before enabling the release: + +1. Verify every instance points at the same Redis deployment and namespace. +2. Confirm the Redis role permits `EVAL`, `INCR`, `EXPIRE`, and `TTL`. +3. Confirm sensitive routes resolve to `failOpen: false`. +4. Exercise two instances with the same tenant and route. +5. Verify tenant A and tenant B receive independent counters. +6. Send a spoofed forwarded IP and confirm the socket identity remains stable. +7. Observe the rejection and dependency metrics during a controlled limit test. +8. Confirm `Retry-After` matches the selected override or configured window. +9. Keep the prior release available while watching the new `v2` key namespace. + +The key correctness rule is simple: every request that can consume a quota +must make its decision against a shared, atomically incremented bucket whose +identity includes the intended tenant and route. + +## Review notes for future adapters + +An adapter should keep the `eval` operation on the same Redis connection path +as the counters. A client-side transaction that queues commands but allows a +different process to interleave between them does not provide the same +guarantee. The script must remain constant and receive only the key and window +as arguments; do not interpolate tenant IDs, paths, or other request data into +Lua source. + +The script intentionally assigns expiry only when the returned count is one. +Refreshing expiry on every request would turn a fixed window into a sliding +window and would allow a continuously active client to retain a bucket forever. +Changing that behavior requires a new contract, new capacity analysis, and +new operational alerts. + +When adding another bucket dimension, update all three places together: the +identity derivation, the key-format documentation, and the multi-instance +test. A dimension omitted from one bucket can create surprising precedence +behavior even when the other bucket is correctly isolated. Keep secrets out of +labels, logs, and Redis key values; only digests or non-sensitive stable IDs +should be emitted to infrastructure systems. + +The route path is evaluated after Express has mounted the middleware. If a +future router uses a wildcard or rewrites `req.path`, the route identity must +be reviewed before enabling `includeRoute` for that router. The method is part +of the identity so a read and a write endpoint cannot unintentionally share a +quota when route scope is enabled. + +The fail-open option is deliberately visible at configuration construction. +Do not catch a Redis error inside the Lua adapter and return a synthetic count: +that would make dependency failure indistinguishable from an admitted request. +Instead, let the middleware apply the configured policy and expose the +degraded state to the deployment's health and alerting systems. + +For incident response, preserve the namespace, route, tenant digest, window +ID, and configured limit from a rejected request. Those fields identify the +bucket without exposing the tenant's raw credential. A support workflow can +then compare the Redis TTL and counter value against the response headers and +determine whether the issue is quota exhaustion, clock skew, or dependency +failure. + +The implementation is intentionally additive. Existing callers can continue +to use the factory without `includeRoute`, while the application-wide API +middleware opts into route scope explicitly. The authentication limiter keeps +its tenant-wide behavior because login and refresh are one shared abuse +surface. This split is documented in code through the option rather than +hidden in route-name conditionals. + +The compatibility path is useful for isolated unit tests and local adapters, +but a production readiness check should assert that the selected Redis client +supports `EVAL`. If that check fails, choose fail-closed for sensitive traffic +and page the operator rather than silently accepting a weaker distributed +guarantee. diff --git a/src/app.ts b/src/app.ts index 78ec8235..1f15be1d 100644 --- a/src/app.ts +++ b/src/app.ts @@ -128,7 +128,7 @@ try { failOpen: !isProd, }; } -const rateLimitMiddleware = createRateLimitMiddleware(rateLimitConfig); +const rateLimitMiddleware = createRateLimitMiddleware(rateLimitConfig, { includeRoute: true }); let authRateLimitConfig: { enabled: boolean; diff --git a/src/middleware/rateLimit.ts b/src/middleware/rateLimit.ts index ba7148e1..9b83d355 100644 --- a/src/middleware/rateLimit.ts +++ b/src/middleware/rateLimit.ts @@ -42,13 +42,23 @@ export interface RateLimitConfig { failOpen?: boolean /** Function to extract tenant identifier from request */ getTenantId?: (req: Request) => string | undefined + /** Include method/path in the bucket identity for route-scoped limits. */ + includeRoute?: boolean /** Function to resolve tenant-specific rate-limit override if configured */ getTenantOverride?: (tenantId: string) => Promise<{ rateLimit: number; windowSize: number } | null> /** * Optional Redis client getter — injected in tests to simulate failures. * Defaults to `RedisConnection.getInstance().getClient()`. */ - getRedis?: () => { incr(k: string): Promise; expire(k: string, s: number): Promise; ttl(k: string): Promise } + getRedis?: () => RateLimitRedis +} + +export interface RateLimitRedis { + incr(k: string): Promise + expire(k: string, s: number): Promise + ttl(k: string): Promise + /** Redis EVAL is optional so small unit-test doubles remain compatible. */ + eval?: (script: string, options: { keys: string[]; arguments: string[] }) => Promise } // ── Helpers ─────────────────────────────────────────────────────────────────── @@ -140,6 +150,20 @@ function setRateLimitHeaders( // ── Core fixed-window check ─────────────────────────────────────────────────── +/** + * INCR followed by EXPIRE is not a transaction: a worker can be interrupted + * between the two commands, leaving a bucket without an expiry. Lua executes + * the whole sequence atomically on Redis's single command thread. + */ +const ATOMIC_FIXED_WINDOW_SCRIPT = ` +local count = redis.call('INCR', KEYS[1]) +if count == 1 then + redis.call('EXPIRE', KEYS[1], ARGV[1]) +end +local ttl = redis.call('TTL', KEYS[1]) +return { count, ttl } +` + /** * Increment a fixed-window counter in Redis and return whether the request * is within the allowed budget. @@ -147,10 +171,28 @@ function setRateLimitHeaders( * Returns `{ count, ttl }` so the caller can set headers and decide to block. */ async function checkWindow( - redis: { incr(k: string): Promise; expire(k: string, s: number): Promise; ttl(k: string): Promise }, + redis: RateLimitRedis, key: string, windowSec: number, ): Promise<{ count: number; ttl: number }> { + if (redis.eval) { + const result = await redis.eval(ATOMIC_FIXED_WINDOW_SCRIPT, { + keys: [key], + arguments: [String(windowSec)], + }) + if (!Array.isArray(result) || result.length < 2) { + throw new Error('rate limiter returned an invalid atomic result') + } + const count = Number(result[0]) + const ttl = Number(result[1]) + if (!Number.isFinite(count) || !Number.isFinite(ttl)) { + throw new Error('rate limiter returned non-numeric atomic values') + } + return { count, ttl: ttl > 0 ? ttl : windowSec } + } + + // Compatibility path for minimal adapters and existing test doubles. The + // production redis client exposes eval and therefore uses the atomic path. const count = await redis.incr(key) if (count === 1) await redis.expire(key, windowSec) const ttl = await redis.ttl(key) @@ -182,6 +224,7 @@ export function createRateLimitMiddleware( namespace = 'ratelimit:api', windowSec = config.windowSec, getTenantId: customGetTenantId, + includeRoute = false, getRedis = () => RedisConnection.getInstance().getClient(), } = options ?? {} @@ -213,15 +256,23 @@ export function createRateLimitMiddleware( // Use getClientIp instead of req.ip to prevent X-Forwarded-For spoofing. // See getClientIp for the full threat model and rationale. const ip = getClientIp(req) - const tenantSegment = tenantId ? `tenant:${tenantId}` : `ip:${ip}` + const route = `${req.method}:${req.baseUrl}${req.path}` + const identity = tenantId ? `tenant:${tenantId}` : `ip:${ip}` + // Hash every user-controlled dimension before putting it in a Redis key: + // tenant IDs and paths cannot create ambiguous separators or unbounded key + // sizes, while route and tenant identity remain independently represented. + const routeScope = includeRoute ? `|route:${route}` : '' + const tenantScope = hashIdentifier(`${identity}${routeScope}`) const now = Math.floor(Date.now() / 1000) + const windowId = Math.floor(now / effectiveWindowSec) - const tenantKey = `${namespace}:${tenantSegment}` + const tenantKey = `${namespace}:tenant:${tenantScope}:${windowId}` // Per-key bucket keyed by key id + tier ceiling so a key that changes // tiers (e.g. upgrade from free to pro) gets a fresh counter scoped to // the new tier rather than inheriting the old tier's budget. - const keyBucket = keyId ? `${namespace}:key:${keyId}:${tier}` : null + const keyRoute = includeRoute ? `|route:${route}` : '' + const keyBucket = keyId ? `${namespace}:key:${keyId}:${tier}${keyRoute}:${windowId}` : null try { const redis = getRedis() diff --git a/tests/routes/rateLimitDistributed.test.ts b/tests/routes/rateLimitDistributed.test.ts new file mode 100644 index 00000000..bcb5e2ee --- /dev/null +++ b/tests/routes/rateLimitDistributed.test.ts @@ -0,0 +1,208 @@ +import { describe, expect, it } from 'vitest' +import express, { type Express } from 'express' +import request from 'supertest' +import { createRateLimitMiddleware, type RateLimitRedis } from '../../src/middleware/rateLimit.js' + +/** + * A small Redis-compatible test double. `eval` executes synchronously to model + * Redis's single command thread, while the middleware calls it through a + * Promise just like the real client. + */ +class AtomicRedis implements RateLimitRedis { + private values = new Map() + private expiries = new Map() + readonly scripts: Array<{ script: string; keys: string[]; args: string[] }> = [] + now = 0 + fail = false + + async incr(key: string): Promise { + this.removeExpired(key) + const count = (this.values.get(key) ?? 0) + 1 + this.values.set(key, count) + return count + } + + async expire(key: string, seconds: number): Promise { + this.expiries.set(key, this.now + seconds * 1000) + return 1 + } + + async ttl(key: string): Promise { + this.removeExpired(key) + const expiry = this.expiries.get(key) + if (expiry === undefined) return -1 + return Math.max(1, Math.ceil((expiry - this.now) / 1000)) + } + + async eval(script: string, options: { keys: string[]; arguments: string[] }): Promise { + if (this.fail) throw new Error('redis unavailable') + this.scripts.push({ script, keys: options.keys, args: options.arguments }) + const key = options.keys[0] + const seconds = Number(options.arguments[0]) + this.removeExpired(key) + const count = (this.values.get(key) ?? 0) + 1 + this.values.set(key, count) + if (count === 1) this.expiries.set(key, this.now + seconds * 1000) + return [count, await this.ttl(key)] + } + + advance(seconds: number) { + this.now += seconds * 1000 + } + + private removeExpired(key: string) { + const expiry = this.expiries.get(key) + if (expiry !== undefined && expiry <= this.now) { + this.values.delete(key) + this.expiries.delete(key) + } + } +} + +function config(overrides: Record = {}) { + return { + enabled: true, + windowSec: 60, + maxFree: 2, + maxPro: 2, + maxEnterprise: 2, + failOpen: false, + ...overrides, + } +} + +function buildApp( + redis: RateLimitRedis, + options: { + config?: Record + getTenantId?: (req: express.Request) => string | undefined + getTenantOverride?: (tenantId: string) => Promise<{ rateLimit: number; windowSize: number } | null> + routes?: string[] + } = {}, +): Express { + const app = express() + app.use(express.json()) + if (!options.getTenantId) { + app.use((req, _res, next) => { + ;(req as any).apiKeyRecord = { id: 'key-1', ownerId: 'tenant-a', tier: 'free' } + next() + }) + } + app.use('/api', createRateLimitMiddleware(config(options.config), { + namespace: 'ratelimit:distributed-test', + getRedis: () => redis, + getTenantId: options.getTenantId, + getTenantOverride: options.getTenantOverride, + includeRoute: true, + })) + for (const route of options.routes ?? ['/ping']) { + app.get(`/api${route}`, (_req, res) => res.json({ ok: true, route })) + } + app.use((_err: any, _req: any, res: any, _next: any) => { + res.status(_err.status ?? 500).json({ error: _err.message, code: _err.code }) + }) + return app +} + +describe('distributed rate-limit correctness', () => { + it('uses one atomic Redis script for increment and expiry', async () => { + const redis = new AtomicRedis() + const app = buildApp(redis) + + const response = await request(app).get('/api/ping') + + expect(response.status).toBe(200) + expect(redis.scripts).toHaveLength(2) + expect(redis.scripts[0].script).toContain("redis.call('INCR'") + expect(redis.scripts[0].script).toContain("redis.call('EXPIRE'") + expect(redis.scripts[0].script).toContain("redis.call('TTL'") + expect(redis.scripts[0].keys).toHaveLength(1) + }) + + it('enforces one shared limit across middleware instances without overshoot', async () => { + const redis = new AtomicRedis() + const appA = buildApp(redis, { config: { maxFree: 10, maxPro: 10, maxEnterprise: 10 } }) + const appB = buildApp(redis, { config: { maxFree: 10, maxPro: 10, maxEnterprise: 10 } }) + + const responses = await Promise.all( + Array.from({ length: 40 }, (_, index) => request(index % 2 ? appA : appB).get('/api/ping')), + ) + + expect(responses.filter((response) => response.status === 200)).toHaveLength(10) + expect(responses.filter((response) => response.status === 429)).toHaveLength(30) + expect(redis.scripts.length).toBeGreaterThanOrEqual(10) + }) + + it('separates tenant buckets and prevents cross-tenant collisions', async () => { + const redis = new AtomicRedis() + const app = buildApp(redis, { + config: { maxFree: 1, maxPro: 1, maxEnterprise: 1 }, + getTenantId: (req) => String(req.headers['x-tenant-id'] ?? ''), + }) + + expect((await request(app).get('/api/ping').set('x-tenant-id', 'tenant-a')).status).toBe(200) + expect((await request(app).get('/api/ping').set('x-tenant-id', 'tenant-b')).status).toBe(200) + expect((await request(app).get('/api/ping').set('x-tenant-id', 'tenant-a')).status).toBe(429) + }) + + it('includes route scope so one route cannot exhaust another route', async () => { + const redis = new AtomicRedis() + const app = buildApp(redis, { + config: { maxFree: 1, maxPro: 1, maxEnterprise: 1 }, + routes: ['/one', '/two'], + }) + + expect((await request(app).get('/api/one')).status).toBe(200) + expect((await request(app).get('/api/two')).status).toBe(200) + expect((await request(app).get('/api/one')).status).toBe(429) + }) + + it('applies tenant overrides to the shared atomic window and expiry', async () => { + const redis = new AtomicRedis() + const app = buildApp(redis, { + config: { maxFree: 10, maxPro: 10, maxEnterprise: 10 }, + getTenantOverride: async (tenantId) => tenantId === 'tenant-a' + ? { rateLimit: 1, windowSize: 30 } + : null, + }) + + expect((await request(app).get('/api/ping')).status).toBe(200) + const blocked = await request(app).get('/api/ping') + expect(blocked.status).toBe(429) + expect(blocked.headers['retry-after']).toBe('30') + expect(redis.scripts[0].args).toEqual(['30']) + + redis.advance(30) + expect((await request(app).get('/api/ping')).status).toBe(200) + }) + + it('uses the socket peer rather than spoofable forwarded headers', async () => { + const redis = new AtomicRedis() + const app = buildApp(redis, { config: { maxFree: 1, maxPro: 1, maxEnterprise: 1 } }) + + expect((await request(app).get('/api/ping').set('x-forwarded-for', '198.51.100.1')).status).toBe(200) + expect((await request(app).get('/api/ping').set('x-forwarded-for', '203.0.113.99')).status).toBe(429) + }) + + it('fails closed when the shared dependency is unavailable', async () => { + const redis = new AtomicRedis() + redis.fail = true + const app = buildApp(redis, { config: { failOpen: false } }) + + const response = await request(app).get('/api/ping') + + expect(response.status).toBe(503) + expect(response.body.code).toBe('service_unavailable') + }) + + it('supports explicit fail-open behavior for non-sensitive routes', async () => { + const redis = new AtomicRedis() + redis.fail = true + const app = buildApp(redis, { config: { failOpen: true } }) + + const response = await request(app).get('/api/ping') + + expect(response.status).toBe(200) + expect(response.headers['x-ratelimit-remaining']).toBe('2') + }) +})