Fixed-window rate limiter allows boundary bursts and trusts req.ip without configuring Express trust proxy

[prodbycorne]

Overview

buildRateLimit() in src/middleware/rateLimit.js implements a classic fixed-window counter:

const identifier = req.ip || req.connection?.remoteAddress || 'unknown';
const bucket = Math.floor(Date.now() / 1000 / windowSeconds);
const key = `ratelimit:${keyPrefix}:${identifier}:${bucket}`;
const count = await redis.incr(key);
if (count === 1) await redis.expire(key, windowSeconds);
if (count > max) { ... 429 ... }

This has two independent, compounding problems:

1. Fixed-window boundary burst. A client can send max requests in the last instant of window N and another max requests in the first instant of window N+1 — effectively 2x max requests within a span far shorter than windowSeconds, because the algorithm resets the counter to zero at each bucket boundary with no memory of the previous window's tail. For WEBHOOK_TEST_RATELIMIT_MAX=5 (a deliberately tight limit specifically meant to bound how often /webhooks/:id/test can be used — itself a webhook-delivery-triggering, and per the companion SSRF issue, a potential internal-network-probing primitive), this halves the intended protection at exactly the moments an abuser would naturally time their bursts.

2. Untrusted req.ip in front of a proxy. src/index.js never calls app.set('trust proxy', ...). Express's default (trust proxy disabled) means req.ip is always the immediate socket peer address. In any real deployment behind a reverse proxy or load balancer (which the Docker/production setup implies — see docker-compose.yml and the README's Docker quick start), every request's req.ip will be the proxy's address, identical for all clients. This has two failure modes depending on configuration intent, both bad:

• If trust proxy is left disabled (current state) in a proxied deployment: every distinct real client collapses into a single rate-limit bucket, so legitimate traffic from many users can trip a 429 as a group — an accidental denial-of-service against legitimate users caused entirely by the rate limiter itself.
• If an operator "fixes" this by blindly trusting X-Forwarded-For (a common but incorrect quick fix) without restricting it to a known proxy hop count/list, any client can trivially spoof X-Forwarded-For to present a fresh identifier on every request, bypassing per-IP rate limiting entirely.

Requirements

• Replace the fixed-window counter with a sliding-window algorithm (e.g. a sliding log using a Redis sorted set of request timestamps trimmed to the window, or a sliding-window-counter approximation) so boundary bursts are eliminated.
• Explicitly configure Express's trust proxy setting in src/index.js based on a new env var (e.g. TRUST_PROXY_HOPS, default 0) so operators can correctly declare how many trusted proxy hops sit in front of the app — defaulting to not trusting any forwarded header when unset, and documented clearly so the "spoofable X-Forwarded-For" failure mode isn't reintroduced by a naive fix.
• Document in the README exactly how to set TRUST_PROXY_HOPS correctly for the Docker Compose setup and for a typical single-load-balancer production deployment.

Acceptance Criteria

• A client can no longer achieve more than max requests within any rolling windowSeconds interval, including across a fixed-window boundary (tested by sending max requests just before a window boundary and max more just after, and asserting the second batch is throttled appropriately).
• TRUST_PROXY_HOPS=0 (or unset) means req.ip reflects the direct socket peer and forwarded headers are ignored.
• TRUST_PROXY_HOPS=1 correctly extracts the real client IP from X-Forwarded-For when exactly one trusted proxy is in front of the app, and does not allow a client to spoof additional hops to escape rate limiting.
• Existing test/rateLimit.test.js continues to pass, plus new tests for the sliding-window and trust-proxy behavior.
• README documents the required TRUST_PROXY_HOPS configuration for common deployment topologies.

Additional Notes

More precise references

• src/middleware/rateLimit.js:22-45 (buildRateLimit's returned middleware): confirmed exact fixed-window logic — bucket = Math.floor(Date.now() / 1000 / windowSeconds), key includes identifier and bucket, redis.incr(key) + conditional redis.expire(key, windowSeconds) on first increment.
• src/middleware/rateLimit.js:23 — const identifier = req.ip || req.connection?.remoteAddress || 'unknown'; confirmed as the sole identity signal.
• src/index.js — confirmed no app.set('trust proxy', ...) call anywhere in the file (full file read this pass); helmet() (line 27) and CORS middleware are configured but neither touches trust proxy.
• src/routes/webhooks.js:15-25 — confirmed both manageLimit and testLimit are built via buildRateLimit, so both inherit the same fixed-window and untrusted-req.ip issues; any fix to buildRateLimit itself automatically fixes both call sites (and any future ones, e.g. the rate limiters proposed for airdrops in multer() is configured with no file size limit — unbounded CSV upload enables memory-exhaustion DoS #85).
• The rate-limit middleware already fails open on Redis errors (catch block, lines 41-44, logger.warn('Rate limit fail-open...')) — worth explicitly confirming this fail-open behavior is preserved by whatever sliding-window implementation replaces the counter logic, since it's a deliberate availability trade-off already made in this codebase and not something this issue should silently regress.

Additional edge cases

• A sliding-log approach (Redis sorted set of timestamps, trimmed via ZREMRANGEBYSCORE then ZCARD/ZADD) needs its own atomicity consideration: the naive sequence of ZREMRANGEBYSCORE → ZCARD → conditionally ZADD is itself a multi-step non-atomic sequence prone to the exact same race-condition class flagged in Repository create() functions perform non-atomic two-step Redis writes, risking orphaned or dangling records on crash #84 (repository non-atomic writes) — under concurrent requests from the same identifier, two requests could both read a ZCARD just under max before either adds its own entry, letting both through. This should be implemented as a single Lua script (EVAL) for correctness under concurrency, not just for the boundary-burst fix in isolation — worth flagging explicitly since the issue's requirements don't mention this specific consistency detail.
• TRUST_PROXY_HOPS needs precise semantics: Express's trust proxy setting accepts a number (hop count) directly, so app.set('trust proxy', Number(process.env.TRUST_PROXY_HOPS) || 0) is likely sufficient, but worth testing the exact interaction with a chain of multiple X-Forwarded-For entries (e.g. X-Forwarded-For: client, proxy1 when hops=1 vs the same header value when hops=2) to confirm Express's off-by-one semantics (hop count = number of trusted proxies to skip from the right) are documented correctly in the README, since this is a commonly-misconfigured setting even outside this codebase.
• IPv6 and IPv4-mapped-IPv6 addresses (::ffff:127.0.0.1 vs 127.0.0.1) can appear as different-looking req.ip values for what's actually the same client depending on Node's net/http stack version and OS — worth a defensive normalization step so a client can't trivially get two separate rate-limit buckets by alternating between IPv4 and IPv6 loopback-mapped representations if any test environment surfaces this.
• WEBHOOK_TEST_RATELIMIT_MAX (default 5/min per the companion SSRF-oracle issue, Webhook test endpoint can be abused as a blind SSRF timing/status oracle to probe internal network services #96) is exactly the kind of tight limit where boundary-burst doubling (5 → 10 effective) matters most in practice — this issue's fix directly reduces the severity of Webhook test endpoint can be abused as a blind SSRF timing/status oracle to probe internal network services #96's reconnaissance-oracle concern, even though neither issue depends on the other to be individually valid.

Implementation sketch

1. Add TRUST_PROXY_HOPS to src/config.js (default 0), and in src/index.js add app.set('trust proxy', config.trustProxyHops); immediately after const app = express(); and before any middleware that reads req.ip.
2. Replace buildRateLimit's counter logic with a Lua script executed via redis.eval(...) (ioredis supports defineCommand for reusable scripts) implementing: trim the sorted set to entries within [now - windowSeconds*1000, now], count remaining entries, and if count < max, add the new entry with score now and return { allowed: true, count: count + 1 }, else return { allowed: false, count } — all atomically in one round trip.
3. Preserve the existing X-RateLimit-* response headers and the fail-open try/catch wrapper around the Redis call.
4. README: add a "Deployment behind a proxy" section under the existing Docker Compose docs, explaining TRUST_PROXY_HOPS=1 for the bundled docker-compose.yml topology (app behind one reverse-proxy hop) versus 0 for direct exposure, with an explicit warning about the spoofing risk of setting it higher than the actual number of trusted hops.

Test/reproduction plan

• Send max requests at T=windowSeconds - 0.01s then max more at T=windowSeconds + 0.01s (simulated via a fake/frozen clock or Redis TIME mock); assert the second batch is throttled once the rolling window (not the fixed bucket) would exceed max.
• Concurrency test: fire max + 5 requests simultaneously (via Promise.all) against the same identifier; assert exactly max succeed and the rest 429 — this specifically exercises the Lua-script atomicity fix, since a naive non-atomic sliding-window-counter implementation would likely let more than max through under true concurrency.
• TRUST_PROXY_HOPS=0: send a request with a spoofed X-Forwarded-For: 1.2.3.4; assert req.ip is the direct socket peer, not the spoofed header.
• TRUST_PROXY_HOPS=1: simulate a request through one proxy hop (X-Forwarded-For: realclient, proxy) and assert req.ip resolves to realclient; then simulate a client directly spoofing an extra hop (X-Forwarded-For: fake1, fake2) with no real proxy involved and confirm it cannot manufacture additional trusted hops to escape rate limiting (this requires the test harness to actually route through Express's trust proxy resolution rather than mocking req.ip directly, to catch real misconfiguration).
• Confirm test/rateLimit.test.js still passes with the new Lua-script-backed implementation, including the existing fail-open-on-Redis-error test if one exists.

Cross-references

• Webhook test endpoint can be abused as a blind SSRF timing/status oracle to probe internal network services #96 (webhook test SSRF oracle) directly benefits from this fix — the boundary-burst doubling of WEBHOOK_TEST_RATELIMIT_MAX is one of the concrete mechanisms making that endpoint's abuse easier; note in Webhook test endpoint can be abused as a blind SSRF timing/status oracle to probe internal network services #96 that this fix (once landed) should be verified against Webhook test endpoint can be abused as a blind SSRF timing/status oracle to probe internal network services #96's "aggregate test-request budget" acceptance criterion.
• Repository create() functions perform non-atomic two-step Redis writes, risking orphaned or dangling records on crash #84 (non-atomic Redis writes) — the Lua-script/atomicity concern for the sliding-window counter is conceptually the same class of problem Repository create() functions perform non-atomic two-step Redis writes, risking orphaned or dangling records on crash #84 addresses for repository writes; worth referencing Repository create() functions perform non-atomic two-step Redis writes, risking orphaned or dangling records on crash #84's chosen atomicity pattern (MULTI/EXEC vs Lua) for consistency in style across the codebase, though rate limiting's read-then-conditionally-write requirement specifically needs Lua (not just MULTI/EXEC, which can't do a conditional based on a value read earlier in the same transaction) — a good concrete example of the "where atomicity via MULTI isn't sufficient" case Repository create() functions perform non-atomic two-step Redis writes, risking orphaned or dangling records on crash #84 asks implementers to watch for.
• multer() is configured with no file size limit — unbounded CSV upload enables memory-exhaustion DoS #85 (airdrop upload rate limiting) will reuse buildRateLimit for new POST /airdrops and POST /airdrops/:id/recipients limiters — sequence Fixed-window rate limiter allows boundary bursts and trusts req.ip without configuring Express trust proxy #90's fix first (or at least design them compatibly) so multer() is configured with no file size limit — unbounded CSV upload enables memory-exhaustion DoS #85 doesn't build new limiters against the soon-to-be-replaced fixed-window implementation and then need rework.