P0: Add per-IP request rate limiting (#166) - #185
Conversation
|
Nice work — this lands the shape we want for the mina-explorer client: the 600 req / 60s default comfortably absorbs the Explorer's per-page fallback bursts (full→basic→minimal + per-block detail is well under 600/min), One hardening worth doing before we call #166 done: The subtlety is that honoring XFF is also what keeps NAT'd / LB-fronted Explorer users in their own buckets instead of collapsing onto one shared IP — so the fix isn't to drop XFF, it's to bound the trust with a hop-count knob and fall back to the socket address: // TRUST_PROXY = number of trusted proxy hops in front of us.
// Behind an LB/ingress (our deployment): set it to the hop count so real client IPs are used
// (per-user Explorer buckets preserved). Set 0 on a directly-exposed server to ignore XFF.
function clientId(request, serverContext) {
const hops = Number(process.env.TRUST_PROXY ?? 0);
if (hops > 0) {
const xff = request.headers.get('x-forwarded-for')?.split(',').map(s => s.trim()).filter(Boolean) ?? [];
const ip = xff[xff.length - hops]; // count from the right → ignores attacker-prepended entries
if (ip) return ip;
}
const ctx = serverContext;
return ctx?.req?.socket?.remoteAddress ?? ctx?.socket?.remoteAddress ?? 'unknown';
}Two smaller, non-blocking notes: (1) worth confirming the short-circuited 429 still picks up |
X-Forwarded-For was trusted unconditionally and read left-to-right, so a single source could rotate the header to mint a fresh bucket per request and bypass the limit entirely — the abuse this plugin exists to stop — while each forged value also added a Map entry that only cleared at window end. Dropping the header isn't the fix: honouring it is what keeps NAT'd and LB-fronted clients in their own buckets rather than collapsing onto one address. Instead TRUST_PROXY declares how many proxy hops sit in front of the API, and the client is read as that many entries from the right — the portion our own proxies appended — so prepended values are ignored. It defaults to 0, which ignores forwarding headers and keys on the socket address: the safe reading for a directly-exposed server. X-Real-IP is gated the same way, and a chain shorter than the hop count falls back to the socket rather than trusting a caller-supplied entry. Also pins two contracts the limiter must not regress: the healthcheck is never throttled, and the short-circuited 429 still carries CORS headers (verified — yoga's CORS runs on onResponse), so cross-origin clients get a readable 429 instead of an opaque CORS error. Addresses review feedback on #185. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @SanabriaRusso — the XFF bypass was real and is fixed in
One deviation from your snippet: I threaded it through // one LB in front, so only the last entry is ours; varying the prepended value must not help
assert.strictEqual((await request('a.a.a.a, 7.7.7.7')).status, 200);
assert.strictEqual((await request('b.b.b.b, 7.7.7.7')).status, 200);
assert.strictEqual((await request('c.c.c.c, 7.7.7.7')).status, 429);Your ACAO question — confirmed, no fix needed. Your reasoning was right: yoga's CORS runs on Pinned as a test, since it's the kind of thing a plugin-order change could quietly break. One thing worth a second opinion: On |
Three corrections, all of which would have misled operators: CORS. The checklist told operators to set an allowlist "or leave unset — not *", which would block every cross-origin browser client, the mina-explorer included, with no server-side symptom. For a public read-only API over already-public data, CORS_ORIGIN=* is the correct setting rather than a lapse: CORS constrains browsers, not curl, so it is not an access control. Adds a section making the choice explicit and notes these controls arrive in 1.0.0 — on 0.0.x, CORS_ORIGIN defaults to '*', so the protections table describes a version most operators are not yet running. TRUST_PROXY. The doc described X-Forwarded-For as read "first hop", which is the behaviour removed in #185 as a rate-limit bypass. Documents the hop-count model and the deny-by-default reading instead. Read replicas. The README claimed the server "fans queries across" multiple PG_CONN hosts. It does not: postgres.js scopes hostIndex per Connection (src/connection.js:89), so every pooled connection starts at host[0] and only advances on failure — failover, not fan-out. As written it promised read scaling that adding replicas cannot deliver. Addresses review feedback on #186. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…Y row Scaling told operators to "add read replicas and point PG_CONN at them before scaling the API further", which reads as added read capacity. It isn't: postgres.js scopes hostIndex per Connection, so every pooled connection starts at host[0] and only advances on failure. Extra hosts buy redundancy, not throughput — real read scaling needs a balancer in front of Postgres. The failover section now says so plainly rather than leaving "connects to an available host" open to the throughput reading. Adds a version scope note. Nearly everything the runbook says to observe or tune ships in 1.0.0; on 0.0.x, /readiness 404s, the tuning knobs are no-ops, and SIGTERM skips the drain. A runbook that misdirects mid- incident is worse than no runbook, and the published image today is 0.0.6. Scoping by version rather than by in-flight PR numbers keeps the note true after the merge train lands. Splits the 429 incident row: after #185, mass 429s across unrelated clients most likely means TRUST_PROXY is unset behind a gateway, collapsing every client into one bucket — a different fix from a single client exceeding the limit. Addresses review feedback on #197. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Verdict: NOT MERGEABLE (blocking) 🚫 The XFF hardening from the first pass is genuinely fixed and the hop-count-from-the-right logic is correct. One blocker remains, and it is the interaction between the two defaults rather than either one alone. What I checked
Blocker:
|
X-Forwarded-For was trusted unconditionally and read left-to-right, so a single source could rotate the header to mint a fresh bucket per request and bypass the limit entirely — the abuse this plugin exists to stop — while each forged value also added a Map entry that only cleared at window end. Dropping the header isn't the fix: honouring it is what keeps NAT'd and LB-fronted clients in their own buckets rather than collapsing onto one address. Instead TRUST_PROXY declares how many proxy hops sit in front of the API, and the client is read as that many entries from the right — the portion our own proxies appended — so prepended values are ignored. It defaults to 0, which ignores forwarding headers and keys on the socket address: the safe reading for a directly-exposed server. X-Real-IP is gated the same way, and a chain shorter than the hop count falls back to the socket rather than trusting a caller-supplied entry. Also pins two contracts the limiter must not regress: the healthcheck is never throttled, and the short-circuited 429 still carries CORS headers (verified — yoga's CORS runs on onResponse), so cross-origin clients get a readable 429 instead of an opaque CORS error. Addresses review feedback on #185. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5f49bf4 to
cf9eb0f
Compare
…Y row Scaling told operators to "add read replicas and point PG_CONN at them before scaling the API further", which reads as added read capacity. It isn't: postgres.js scopes hostIndex per Connection, so every pooled connection starts at host[0] and only advances on failure. Extra hosts buy redundancy, not throughput — real read scaling needs a balancer in front of Postgres. The failover section now says so plainly rather than leaving "connects to an available host" open to the throughput reading. Adds a version scope note. Nearly everything the runbook says to observe or tune ships in 1.0.0; on 0.0.x, /readiness 404s, the tuning knobs are no-ops, and SIGTERM skips the drain. A runbook that misdirects mid- incident is worse than no runbook, and the published image today is 0.0.6. Scoping by version rather than by in-flight PR numbers keeps the note true after the merge train lands. Splits the 429 incident row: after #185, mass 429s across unrelated clients most likely means TRUST_PROXY is unset behind a gateway, collapsing every client into one bucket — a different fix from a single client exceeding the limit. Addresses review feedback on #197. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three corrections, all of which would have misled operators: CORS. The checklist told operators to set an allowlist "or leave unset — not *", which would block every cross-origin browser client, the mina-explorer included, with no server-side symptom. For a public read-only API over already-public data, CORS_ORIGIN=* is the correct setting rather than a lapse: CORS constrains browsers, not curl, so it is not an access control. Adds a section making the choice explicit and notes these controls arrive in 1.0.0 — on 0.0.x, CORS_ORIGIN defaults to '*', so the protections table describes a version most operators are not yet running. TRUST_PROXY. The doc described X-Forwarded-For as read "first hop", which is the behaviour removed in #185 as a rate-limit bypass. Documents the hop-count model and the deny-by-default reading instead. Read replicas. The README claimed the server "fans queries across" multiple PG_CONN hosts. It does not: postgres.js scopes hostIndex per Connection (src/connection.js:89), so every pooled connection starts at host[0] and only advances on failure — failover, not fan-out. As written it promised read scaling that adding replicas cannot deliver. Addresses review feedback on #186. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SanabriaRusso
left a comment
There was a problem hiding this comment.
Approving. All four round-1 unblock items verified fixed, plus the two "verified OK" items confirmed to have survived the rebase.
Verified fixed
-
Hop count is correct and configurable.
src/server/rate-limit.ts:175keys onforwarded[forwarded.length - trustProxy]— the Nth entry from the right. GCP's external ALB emitsX-Forwarded-For: <supplied>,<client-ip>,<lb-ip>, soTRUST_PROXY=2selects the client IP and anything the caller prepends only shifts the index harmlessly. The docs and.env.example.compose:15-16both say 2 for a bare GCP ALB plus 1 per extra in-cluster hop — the number is right, checked against the indexing expression rather than taken on faith. -
The global-ceiling failure mode is gone, by re-scoping rather than raising.
TRUST_PROXYnow has no default (rate-limit.ts:85) anduseRateLimitreturns an inert plugin when it is unset (:196-209) after logging a "rate limiting is DISABLED" warning. The shipped default cannot produce a 600 req/min global bucket because it produces no limiter at all. For the record, the configuration that would still fail —TRUST_PROXY=0+RATE_LIMIT_MAX=600behind the LB — works out to: explorer-api backfill 4 rps = 240/min = 40% of one shared bucket, and a mina-explorer page load is 4–6 requests, so ~6 concurrent browser sessions exhaust the remaining 360/min and the backfill starts 429ing too. That now requires an operator to explicitly writeTRUST_PROXY=0into a proxied topology, against an inline comment in the same file telling them to use 2, with a one-shot runtime warning reporting the observed chain length (:216-231). That clears the bar. -
Both probes are exempt.
EXEMPT_PATHS(:12) covers/healthcheckand/readiness, checked at:238./healthcheckis the liveness probe — it is the only health endpoint onmain(src/server/server.ts:20, yoga'shealthCheckEndpoint), so liveness is covered too. -
The bucket map is bounded.
prune()(:124) on an unref'dsetIntervalat the window length (:233) — O(n) once per window, not per request, so it does not degrade under attack. -
429 shape survived the rebase (
:255-267): HTTP 429, body{errors:[{message:"Too many requests. Please retry later.", extensions:{code:"RATE_LIMITED"}}]}, withretry-afterandx-ratelimit-*. It still cannot be mistaken for a schema error — the text matches none of mina-explorer-api'sSCHEMA_ERROR_MARKERS(app/upstream/graphql.py:31-35: "Cannot query field", "Unknown argument", "Unknown type"), so it classifies as a generic upstream error rather than poisoning the capability cache. OPTIONS exemption survived at:242.
One thing that changed underneath this PR and is worth knowing
rate-limit.ts:242's OPTIONS check is now load-bearing, not defence in depth. graphql-yoga/cjs/server.js:171 is options?.cors !== false && useCORS(...), so once #184 lands with its secure default (cors: false) yoga's CORS plugin — and its OPTIONS stopEarly() short-circuit — is not registered at all. Confirmed empirically: with cors: false a preflight traverses the whole user-plugin chain. Round 1's "preflight is exempt because useCORS runs first" no longer holds; your local check is what saves it. Please adjust the comment above it so nobody later deletes it as redundant.
This matters for mina-explorer specifically: it sends Content-Type: application/json (src/services/api/daemon.ts:36), which is not a CORS-safelisted value, so every GraphQL POST from the browser is preceded by a preflight. Preflights are roughly half the request count from that consumer, not an edge case.
Non-blocking notes
x-real-ipfallback (:181) contradicts the comment above it (:172-174), which says the short-chain case falls through to the socket "rather than trust an attacker-chosen entry" — butx-real-ipis equally caller-controlled. Unreachable behind the ALB (it always appends XFF, so the early return fires), so this only matters for direct-to-pod traffic, which is already a trust-boundary breach. Consider dropping the branch or gating it separately.- No max-size cap on the bucket map. TTL eviction is right; within one window the map still grows with unique source IPs, bounded in practice by connection rate rather than by the limiter. A
MAX_BUCKETSceiling would close the residual cheaply. - Add
'/metrics'toEXEMPT_PATHSonce #191 lands. In-cluster scrapers send no XFF so they key on the socket address, and a 15–30 s scrape is ~4/min against 600 — no real risk, but a throttled scrape produces a metrics gap exactly when you most want one. - Trailing slash: #187 normalizes
/readiness/→/readiness;EXEMPT_PATHS.has(url.pathname)does not. Cosmetic — kubelet uses the literal manifest path. Retry-After/X-RateLimit-*are unreadable cross-origin. They are not CORS-safelisted and neither this PR nor #184 setsexposedHeaders, so a browser client sees the 429 status but not the backoff hint.- Test gap worth closing before merge. The suite pins the rotation attack at
TRUST_PROXY=1and the short-chain fallback at2, but there is no positive test atTRUST_PROXY=2with a two-entry chain — the exact configuration the docs tell every GCP operator to use. An off-by-one there would silently reinstate the original blocker with no failing test. Suggested:
test('with a GCP ALB (2 hops) the client is the second entry from the right', async () => {
// Chain as GCP writes it: <caller-supplied>, <client-ip>, <lb-ip>
const request = makeServer('2');
assert.strictEqual((await request('a.a.a.a, 1.2.3.4, 35.1.1.1')).status, 200);
assert.strictEqual((await request('b.b.b.b, 1.2.3.4, 35.1.1.1')).status, 200);
// Same real client, rotated prefix -> still the same bucket.
assert.strictEqual((await request('c.c.c.c, 1.2.3.4, 35.1.1.1')).status, 429);
// A different real client is unaffected.
assert.strictEqual((await request('c.c.c.c, 9.9.9.9, 35.1.1.1')).status, 200);
});Downstream: no impact at the shipped default, because the limiter is inert until TRUST_PROXY is set. Once enabled with a correct hop count, explorer-api's 4 rps backfill gets its own bucket (240/min vs 600) and browser traffic buckets per end-user IP.
One thing to be aware of when you do enable it: mina-explorer-api does not retry 4xx (app/upstream/graphql.py, _execute retries only on transport error / timeout / HTTP ≥ 500), and a 429 without a GraphQL errors[] array raises the base UpstreamError, which query() maps to outcome="unavailable" → breaker.record_failure(). Your 429 does carry an errors[] array, so it takes the classify_graphql_errors path instead — still a generic UpstreamError, still a breaker failure. Either way a sustained 429 burst drives that consumer's per-endpoint circuit breaker toward OPEN, after which it short-circuits every call with zero I/O for the cooldown. That is correct limiter behaviour, not a bug here — but it means the first production value for RATE_LIMIT_MAX should be set generously and watched, because the downstream failure is a step function rather than a gradual degradation.
…Y row Scaling told operators to "add read replicas and point PG_CONN at them before scaling the API further", which reads as added read capacity. It isn't: postgres.js scopes hostIndex per Connection, so every pooled connection starts at host[0] and only advances on failure. Extra hosts buy redundancy, not throughput — real read scaling needs a balancer in front of Postgres. The failover section now says so plainly rather than leaving "connects to an available host" open to the throughput reading. Adds a version scope note. Nearly everything the runbook says to observe or tune ships in 1.0.0; on 0.0.x, /readiness 404s, the tuning knobs are no-ops, and SIGTERM skips the drain. A runbook that misdirects mid- incident is worse than no runbook, and the published image today is 0.0.6. Scoping by version rather than by in-flight PR numbers keeps the note true after the merge train lands. Splits the 429 incident row: after #185, mass 429s across unrelated clients most likely means TRUST_PROXY is unset behind a gateway, collapsing every client into one bucket — a different fix from a single client exceeding the limit. Addresses review feedback on #197. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…Y row Scaling told operators to "add read replicas and point PG_CONN at them before scaling the API further", which reads as added read capacity. It isn't: postgres.js scopes hostIndex per Connection, so every pooled connection starts at host[0] and only advances on failure. Extra hosts buy redundancy, not throughput — real read scaling needs a balancer in front of Postgres. The failover section now says so plainly rather than leaving "connects to an available host" open to the throughput reading. Adds a version scope note. Nearly everything the runbook says to observe or tune ships in 1.0.0; on 0.0.x, /readiness 404s, the tuning knobs are no-ops, and SIGTERM skips the drain. A runbook that misdirects mid- incident is worse than no runbook, and the published image today is 0.0.6. Scoping by version rather than by in-flight PR numbers keeps the note true after the merge train lands. Splits the 429 incident row: after #185, mass 429s across unrelated clients most likely means TRUST_PROXY is unset behind a gateway, collapsing every client into one bucket — a different fix from a single client exceeding the limit. Addresses review feedback on #197. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…Y row Scaling told operators to "add read replicas and point PG_CONN at them before scaling the API further", which reads as added read capacity. It isn't: postgres.js scopes hostIndex per Connection, so every pooled connection starts at host[0] and only advances on failure. Extra hosts buy redundancy, not throughput — real read scaling needs a balancer in front of Postgres. The failover section now says so plainly rather than leaving "connects to an available host" open to the throughput reading. Adds a version scope note. Nearly everything the runbook says to observe or tune ships in 1.0.0; on 0.0.x, /readiness 404s, the tuning knobs are no-ops, and SIGTERM skips the drain. A runbook that misdirects mid- incident is worse than no runbook, and the published image today is 0.0.6. Scoping by version rather than by in-flight PR numbers keeps the note true after the merge train lands. Splits the 429 incident row: after #185, mass 429s across unrelated clients most likely means TRUST_PROXY is unset behind a gateway, collapsing every client into one bucket — a different fix from a single client exceeding the limit. Addresses review feedback on #197. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
X-Forwarded-For was trusted unconditionally and read left-to-right, so a single source could rotate the header to mint a fresh bucket per request and bypass the limit entirely — the abuse this plugin exists to stop — while each forged value also added a Map entry that only cleared at window end. Dropping the header isn't the fix: honouring it is what keeps NAT'd and LB-fronted clients in their own buckets rather than collapsing onto one address. Instead TRUST_PROXY declares how many proxy hops sit in front of the API, and the client is read as that many entries from the right — the portion our own proxies appended — so prepended values are ignored. It defaults to 0, which ignores forwarding headers and keys on the socket address: the safe reading for a directly-exposed server. X-Real-IP is gated the same way, and a chain shorter than the hop count falls back to the socket rather than trusting a caller-supplied entry. Also pins two contracts the limiter must not regress: the healthcheck is never throttled, and the short-circuited 429 still carries CORS headers (verified — yoga's CORS runs on onResponse), so cross-origin clients get a readable 429 instead of an opaque CORS error. Addresses review feedback on #185. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cf9eb0f to
994fe30
Compare
Three corrections, all of which would have misled operators: CORS. The checklist told operators to set an allowlist "or leave unset — not *", which would block every cross-origin browser client, the mina-explorer included, with no server-side symptom. For a public read-only API over already-public data, CORS_ORIGIN=* is the correct setting rather than a lapse: CORS constrains browsers, not curl, so it is not an access control. Adds a section making the choice explicit and notes these controls arrive in 1.0.0 — on 0.0.x, CORS_ORIGIN defaults to '*', so the protections table describes a version most operators are not yet running. TRUST_PROXY. The doc described X-Forwarded-For as read "first hop", which is the behaviour removed in #185 as a rate-limit bypass. Documents the hop-count model and the deny-by-default reading instead. Read replicas. The README claimed the server "fans queries across" multiple PG_CONN hosts. It does not: postgres.js scopes hostIndex per Connection (src/connection.js:89), so every pooled connection starts at host[0] and only advances on failure — failover, not fan-out. As written it promised read scaling that adding replicas cannot deliver. Addresses review feedback on #186. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
X-Forwarded-For was trusted unconditionally and read left-to-right, so a single source could rotate the header to mint a fresh bucket per request and bypass the limit entirely — the abuse this plugin exists to stop — while each forged value also added a Map entry that only cleared at window end. Dropping the header isn't the fix: honouring it is what keeps NAT'd and LB-fronted clients in their own buckets rather than collapsing onto one address. Instead TRUST_PROXY declares how many proxy hops sit in front of the API, and the client is read as that many entries from the right — the portion our own proxies appended — so prepended values are ignored. It defaults to 0, which ignores forwarding headers and keys on the socket address: the safe reading for a directly-exposed server. X-Real-IP is gated the same way, and a chain shorter than the hop count falls back to the socket rather than trusting a caller-supplied entry. Also pins two contracts the limiter must not regress: the healthcheck is never throttled, and the short-circuited 429 still carries CORS headers (verified — yoga's CORS runs on onResponse), so cross-origin clients get a readable 429 instead of an opaque CORS error. Addresses review feedback on #185. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
994fe30 to
ec86cbb
Compare
Three corrections, all of which would have misled operators: CORS. The checklist told operators to set an allowlist "or leave unset — not *", which would block every cross-origin browser client, the mina-explorer included, with no server-side symptom. For a public read-only API over already-public data, CORS_ORIGIN=* is the correct setting rather than a lapse: CORS constrains browsers, not curl, so it is not an access control. Adds a section making the choice explicit and notes these controls arrive in 1.0.0 — on 0.0.x, CORS_ORIGIN defaults to '*', so the protections table describes a version most operators are not yet running. TRUST_PROXY. The doc described X-Forwarded-For as read "first hop", which is the behaviour removed in #185 as a rate-limit bypass. Documents the hop-count model and the deny-by-default reading instead. Read replicas. The README claimed the server "fans queries across" multiple PG_CONN hosts. It does not: postgres.js scopes hostIndex per Connection (src/connection.js:89), so every pooled connection starts at host[0] and only advances on failure — failover, not fan-out. As written it promised read scaling that adding replicas cannot deliver. Addresses review feedback on #186. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SanabriaRusso
left a comment
There was a problem hiding this comment.
Re-approving after the rebase (the previous approval was dismissed by the force-push).
Re-verified: the PR content at ec86cbb differs from the commit I approved (cf9eb0f) by one blank line in src/server/plugins.ts — absorbed while rebasing over #191's metrics plugin. No semantic change.
Carrying forward the round-2 verification unchanged:
- Hop count is the Nth entry from the right; docs say
2for a GCP ALB, and that matches the code. - The global-ceiling failure mode is gone by re-scoping:
TRUST_PROXYhas no default and the limiter is inert until it is set (rate-limit.ts:198returns an empty plugin with a loud warning). The shipped default cannot produce a 600/min global bucket because it produces no limiter at all. - Both probes exempt; the bucket map is bounded by a TTL sweep that is O(n) once per window, not per request.
Non-blocking, but two are worth taking in this PR:
EXEMPT_PATHSis['/healthcheck', '/readiness']—/metricsis still missing, and it now exists on main (#191 merged). A Prometheus scraper hitting it every 15s from one source IP burns rate-limit budget for no reason. One-word fix.- No positive test at
TRUST_PROXY=2with a two-entry chain — the exact config every GCP operator is told to use. An off-by-one there would silently reinstate the round-1 blocker. Test supplied in the earlier comment. - The
x-real-ipfallback contradicts its own comment.
Rollout note: mina-explorer-api does not retry 4xx, and a 429 without a GraphQL errors[] array raises the base UpstreamError → outcome="unavailable" → breaker.record_failure(). A sustained 429 burst opens that consumer's per-endpoint circuit breaker, after which every call short-circuits with zero I/O for the cooldown — a step function, not gradual throttling. Set the first production RATE_LIMIT_MAX generously and watch it.
A public GraphQL endpoint with no throttle lets a single client monopolise the server and the backing Postgres. Add a global, per-client-IP rate limiter that runs on every request before GraphQL parsing, rejecting over-limit traffic with HTTP 429 as cheaply as possible. - RATE_LIMIT_MAX (requests per client per window, default 600; 0 disables) - RATE_LIMIT_WINDOW_MS (window length in ms, default 60000) The client IP is taken from X-Forwarded-For (first hop), then X-Real-IP, then the socket address, falling back to a shared `unknown` bucket so unproxied traffic is still bounded. Health checks are never throttled. The fixed-window counter is in-memory and per-instance; a shared store for exact cross-replica limits is left as deployment hardening and noted in the docs. Implemented as a custom plugin rather than @envelop/rate-limiter, whose directive model bakes limits into the schema and is per-field rather than a global per-IP bucket. Malformed env values fall back to safe defaults. Unit tests cover config parsing, the windowing/reset/prune logic with an injected clock, and prove end-to-end through Yoga that the (max+1)th request from an IP gets 429 while other clients are unaffected. Closes #166. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QSuak9smCHbp4N17xjjLF6
X-Forwarded-For was trusted unconditionally and read left-to-right, so a single source could rotate the header to mint a fresh bucket per request and bypass the limit entirely — the abuse this plugin exists to stop — while each forged value also added a Map entry that only cleared at window end. Dropping the header isn't the fix: honouring it is what keeps NAT'd and LB-fronted clients in their own buckets rather than collapsing onto one address. Instead TRUST_PROXY declares how many proxy hops sit in front of the API, and the client is read as that many entries from the right — the portion our own proxies appended — so prepended values are ignored. It defaults to 0, which ignores forwarding headers and keys on the socket address: the safe reading for a directly-exposed server. X-Real-IP is gated the same way, and a chain shorter than the hop count falls back to the socket rather than trusting a caller-supplied entry. Also pins two contracts the limiter must not regress: the healthcheck is never throttled, and the short-circuited 429 still carries CORS headers (verified — yoga's CORS runs on onResponse), so cross-origin clients get a readable 429 instead of an opaque CORS error. Addresses review feedback on #185. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The secure default is wrong for the expected production topology, where it collapses every client onto the load balancer's address. That symptom surfaces only as unexplained throttling, so warn once on the first forwarded request — once rather than per request, since this sits on the hot path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ec86cbb to
2cd627b
Compare
What & why
Part of the production-readiness epic (#163). Closes #166.
A public GraphQL endpoint with no throttle lets a single client monopolise the server and the backing Postgres. This adds a global, per-client-IP rate limiter that runs on every request before GraphQL parsing, rejecting over-limit traffic with HTTP 429 as cheaply as possible.
RATE_LIMIT_MAX6000disablesRATE_LIMIT_WINDOW_MS60000Design
X-Forwarded-For(first hop) →X-Real-IP→ socket address → sharedunknownbucket. Run behind a proxy that setsX-Forwarded-Forfor correct per-client identification (documented).RATE_LIMIT_MAX. A shared store (Redis) for exact cross-replica limits is left as deployment hardening and noted in the docs./healthcheck) are never throttled.Retry-After,X-RateLimit-Limit,X-RateLimit-Remaining.Why not @envelop/rate-limiter
That library is directive-based: limits are baked into the schema per field and it's per-field rather than a global per-IP bucket, plus it requires a schema/codegen change. A small custom plugin gives a true global per-IP DoS bucket that's env-tunable with no schema impact. (Discussed and chosen during implementation.)
Testing
npm run build— cleannpm run test:unit— all pass (config parsing; windowing/reset/prune with an injected clock; end-to-end through Yoga proving the (max+1)th request returns 429 and other clients are unaffected)npm run lint— cleannpx prettier --debug-check .— exit 0No new runtime dependency.
🤖 Generated with Claude Code