GeezerVison

The headline example applies no limit at all to the attack it is meant to stop.

ctx.session.$id is null until the session is first written to (createSessionController: id = initialId from the cookie, and ensureId() only runs on markDirty()). A password-guessing script sends no session cookie, so keyBy returns null on every attempt, and the middleware does if (identity === null ...) return next() — the limit is skipped entirely. Even a client that does have a session simply drops the cookie.

Verified against this branch:

app.use(session({ secret }));
app.post('/login', handler, [rateLimit({ window: 15*60_000, max: 5, keyBy: (ctx) => ctx.session?.$id ?? null, skipSuccessfulRequests: true })]);
// 20 cookie-less POSTs → 20 reach the handler, 0 × 429

The same shape recurs in the keyBy table below (line 404): ctx.state.user?.id ?? null is null for exactly the unauthenticated attacker, and headers.get('x-api-key') is null for a request that omits the header. Every recommended key fails open for the caller you most want to throttle.

Worth reconsidering both the docs and the API: a null key meaning "unlimited" is a footgun for a security primitive. Options are a documented fallback bucket (e.g. ctx.session?.$id ?? 'anon' plus a second discriminator), or making the skip explicit (false skips, null is an error) so it cannot happen by accident.

Generated by Claude Code

Same problem as the guide example: ctx.session?.$id is null for a client that sends no session cookie, and a null key skips the limit. The recipe this PR describes as previously "unprotected against brute force" is still unprotected after the change — a script that just never sends a cookie gets unlimited attempts (verified: 20/20 POSTs reach handleLogin).

Generated by Claude Code

consume() is a read-modify-write with an await in the middle, so concurrent requests on the same key all read the same counter and the limit does not hold.

Every in-flight request for one key that reaches await store.get(key) before any of them reaches store.set reads the same count and writes count + 1, so N concurrent requests advance the counter by 1. This is not theoretical or specific to async stores — the default memoryStore() is synchronous and it still happens, because await on a non-promise yields to the microtask queue:

app.get('/c', handler, [rateLimit({ window: 60_000, max: 2, keyBy: () => 'k' })]);
await Promise.all(Array.from({ length: 10 }, () => app.handle('/c')));
// → 200,200,200,200,200,200,200,200,200,200  (expected 2 × 200, 8 × 429)

Ten simultaneous connections is what a brute-force or credential-stuffing tool does by default, so the limiter can be bypassed outright rather than merely overshot. With a Redis-backed SessionStore the window is wider still (two network round trips per request).

SessionStore has no atomic increment, so a full fix needs either an optional incr-style hook on the store or per-key serialization. Even a process-local in-flight promise chain keyed by key (chain = chain.then(() => doConsume())) would close the single-process case that the default store and the docs' own login recipe run in. Whatever the fix, this deserves a test using Promise.all — the current suite only ever awaits requests one at a time, which is why it passes.

Generated by Claude Code

The header priority makes trustProxy: true spoofable in exactly the deployments it is meant for.

x-forwarded-for is checked first and the leftmost entry is taken. But the common proxies append rather than overwrite: Cloudflare appends the connecting IP to a client-supplied X-Forwarded-For (and reports the real one in CF-Connecting-IP), and nginx's standard proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for does the same. So behind such a proxy the leftmost XFF entry is attacker-controlled, and it wins over the trustworthy cf-connecting-ip / true-client-ip that this list itself anticipates:

app.get('/p', handler, [rateLimit({ window: 60_000, max: 1, trustProxy: true })]);
// 5 requests, each with x-forwarded-for: 9.9.9.<i> and a fixed cf-connecting-ip: 7.7.7.7
// → 200,200,200,200,200

That is the exact failure mode the PR description argues against ("a limiter an attacker bypasses by sending a different header per request"), still present in the opt-in path. Suggest ordering the single-value, proxy-set headers first (cf-connecting-ip, true-client-ip, x-real-ip) and only then x-forwarded-for, and/or letting the caller name the header to trust (plus a hop count for XFF, taking the Nth entry from the right) rather than a fixed preference list. The guide's wording ("Behind a proxy that overwrites X-Forwarded-For") should also be sharper, since the two most widely deployed proxies do not overwrite it.

Generated by Claude Code

With trustProxy, a request carrying none of the forwarding headers is not limited at all — forwardedAddress() returns null and this line skips straight to next(). The test at tests/server-rate-limit.test.ts:248 pins that as intended ("skips ... rather than lumping them together"), but the consequence is a fail-open: any request that reaches the origin without passing through the proxy (origin port exposed alongside the CDN, a health-check path, an internal hop, a proxy misconfigured after a deploy) bypasses the limiter entirely, and the app still reports itself as protected.

For a keyBy the caller wrote, "return null to skip" is a deliberate choice; for the built-in trustProxy path the safer default is to fail closed — count header-less requests under one shared bucket (they are, by definition, not distinguishable) or reject them — with skipping available via skip if someone really wants it.

Generated by Claude Code

The default memoryStore() grows without bound under the traffic a limiter exists to absorb.

memoryStore() here is constructed with no maxEntries, and it only ever drops an expired entry lazily, inside get(id) for that same id (src/server/session.ts:105-115); there is no sweep. A rate-limit key is attacker-chosen (a forwarded address, an API key, a user id) and typically never seen twice, so each distinct key leaves a Map entry that is expired but never collected — one entry per unique key, for the lifetime of the process. An attacker rotating X-Forwarded-For (or x-api-key) turns the limiter into a memory-exhaustion vector, which is worse than the request flood it was added to stop.

memoryStore({ maxEntries }) already exists; defaulting to a bounded store (with the cap documented) would fix it. Note also the eviction is insertion-order, so if someone follows the guide's advice to pass the session store here, rate-limit churn will evict live sessions and log users out — worth either a warning in the guide or keeping the default counters in their own store.

Generated by Claude Code

status < 400 refunds redirects, which silently disables the limit on the most common login-form shape. The POST/redirect/GET pattern the login-form recipe is about answers a failed login with 302 /login?error=1, so every failed attempt is refunded:

app.post('/login', (ctx) => ctx.redirect('/login?error=1', 302), [
  rateLimit({ window: 60_000, max: 2, keyBy: () => 'k', skipSuccessfulRequests: true }),
]);
// 8 attempts → 302 × 8, never a 429

skipSuccessfulRequests is a security-relevant switch, so "success" should mean what the route means by it — consider a predicate (skipSuccessful?: (res) => boolean) or at least restricting the default to 2xx and documenting the redirect trap in the guide's options table.

Two further issues in this block:

• The refund never checks that the stored record is still the window the request was counted in. If the window rolls over while the handler runs (slow password hash near a boundary), current is the new window's counter and the decrement steals a request from it — that window then allows max + 1. Guarding with current.resetAt === state.resetAt fixes it.
• The get + set here is the same non-atomic read-modify-write as consume(), so concurrent refunds on one key lose decrements (or, combined with a concurrent consume, lose increments).

Generated by Claude Code

headers: false also drops Retry-After from the 429, which the option does not advertise — it is documented as "Emit RateLimit-Limit, RateLimit-Remaining and RateLimit-Reset", and the guide states unconditionally that an over-limit request "is answered 429 Too Many Requests with Retry-After". Verified: with headers: false, the 429 has retry-after: null, so a client has nothing to back off on.

Retry-After is a standard part of a 429 rather than one of the RateLimit-* informational headers; suggest always applying it to the rejection and letting headers govern only the three RateLimit-* ones.

(with withRateLimitHeaders taking a flag for the RateLimit-* trio, or a small helper that sets only retry-after.)

Generated by Claude Code

This @example has the same defect as the guide and cookbook copies: on a login route the caller has no session yet, $id is null until the session is first written, and a null key skips the limit — so the documented way to "protect a login route" protects nothing. Worth replacing with a key that exists for an unauthenticated request (e.g. the submitted username plus a coarse bucket, or trustProxy behind a proxy) once the null-skips-everything semantics are settled.

Generated by Claude Code