feat(rate-limit): env-configurable limits + trusted-hop client keying - #237
feat(rate-limit): env-configurable limits + trusted-hop client keying#237henrique221 wants to merge 4 commits into
Conversation
… gitignore integration)
📝 WalkthroughWalkthroughRate limiting now supports environment-configured defaults, trusted proxy hop selection, socket-address fallback, and configurable bucket eviction. The bulk texts route uses these defaults, with added validation and middleware tests. The example environment and gitignore files were updated. ChangesRate limiter configuration and behavior
Repository scratch workspace hygiene
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant rateLimit
participant clientKey
participant BucketStore
Request->>rateLimit: submit request
rateLimit->>clientKey: resolve client identity
clientKey-->>rateLimit: proxy or socket address
rateLimit->>BucketStore: read or create bounded bucket
BucketStore-->>rateLimit: allowance or rejection
rateLimit-->>Request: continue or return 429
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/middlewares/rate-limit.ts`:
- Around line 46-61: Update clientKey so an x-forwarded-for chain shorter than
trustedHops falls back to socketAddress(c) ?? 'unknown' rather than selecting
the leftmost forwarded entry; preserve the existing trusted-hop candidate
selection when the chain is long enough.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ab65374b-6c23-4044-8093-be5a8768a4ef
📒 Files selected for processing (7)
.env.example.gitignoresrc/domains/bibles/bible-texts/bible-texts.route.tssrc/env.test.tssrc/env.tssrc/middlewares/rate-limit.test.tssrc/middlewares/rate-limit.ts
| // Resolve the client key from the deployment's trusted-proxy topology | ||
| // (standard proxy-addr semantics): walk [socket, ...x-forwarded-for entries | ||
| // right-to-left] and take the entry trustedHops steps out — each trusted | ||
| // proxy APPENDS exactly one entry (Azure App Service appends the real client | ||
| // IP as ip[:port]), so that position is the closest address a client cannot | ||
| // spoof, while leading entries are client-controlled. A chain shorter than | ||
| // trustedHops degrades to its furthest (leftmost) available entry. | ||
| function clientKey(c: Context<AppBindings>, trustedHops: number): string { | ||
| const forwarded = c.req.header('x-forwarded-for') ?? ''; | ||
| const entries = forwarded | ||
| .split(',') | ||
| .map((entry) => entry.trim()) | ||
| .filter(Boolean); | ||
| const candidates = [socketAddress(c) ?? 'unknown', ...entries.reverse()]; | ||
| return stripPort(candidates[Math.min(trustedHops, candidates.length - 1)]); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)rate-limit(\.test)?\.ts$|src/middlewares/rate-limit\.ts|package\.json|vitest|node_modules' || true
echo "== target file outline/size =="
wc -l src/middlewares/rate-limit.ts src/middlewares/rate-limit.test.ts 2>/dev/null || true
ast-grep outline src/middlewares/rate-limit.ts --view expanded 2>/dev/null || true
echo "== rate-limit.ts relevant =="
sed -n '1,130p' src/middlewares/rate-limit.ts
echo "== tests around claimed lines =="
sed -n '200,270p' src/middlewares/rate-limit.test.ts
echo "== searched issues/topo constants =="
rg -n "trustedHops|x-forwarded-f|clientKey|degrades to|socketAddress|stripPort" src -S || trueRepository: eten-tech-foundation/fluent-api
Length of output: 15421
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== environment/default trustedHops definitions =="
sed -n '1,130p' src/env.ts
echo "== deterministic clientKey behavior probe =="
python3 - <<'PY'
def strip_port(entry):
if entry.startswith('['):
i = entry.index(']')
return entry[1:i] if i != -1 else entry
parts = entry.split(':')
return parts[0] if len(parts) == 2 else entry
def client_key(socket_address, headers, trusted_hops):
forwarded = headers.get('x-forwarded-for', '')
entries = [e.strip() for e in forwarded.split(',') if e.strip()]
candidates = [socket_address if socket_address else 'unknown', *(e for e in reversed(entries))]
return strip_port(candidates[min(trusted_hops, len(candidates) - 1)])
for (hops, xff) in [
(0, '1.1.1.1'),
(1, '1.1.1.1'),
(2, 'spoof, 1.1.1.1, 10.0.0.7'),
(3, '1.1.1.1'),
(3, ''),
]:
print({
"trustedHops": hops,
"x_forwarded_for": xff or None,
"9_9_9_9": client_key("9.9.9.9", {'x-forwarded-for': xff}, hops),
"socket_missing": client_key(None, {'x-forwarded-for': xff}, hops),
})
PYRepository: eten-tech-foundation/fluent-api
Length of output: 7903
Fall back to the socket address when x-forwarded-for underflows trustedHops.
clientKey currently selects candidates[Math.min(trustedHops, candidates.length - 1)], so with trustedHops: 3 and a single header entry, both different sockets key on the attacker-supplied header value and share one bucket (trustedHops=3, x-forwarded-for='1.1.1.1' from 10.0.0.7 and 10.0.0.8). When the chain is shorter than the configured trusted hop count, use socketAddress(c) ?? 'unknown' instead of the leftmost header entry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/middlewares/rate-limit.ts` around lines 46 - 61, Update clientKey so an
x-forwarded-for chain shorter than trustedHops falls back to socketAddress(c) ??
'unknown' rather than selecting the leftmost forwarded entry; preserve the
existing trusted-hop candidate selection when the chain is long enough.
kaseywright
left a comment
kaseywright
left a comment
There was a problem hiding this comment.
Really good implementation. Only one issue blocking the merge.
| .map((entry) => entry.trim()) | ||
| .filter(Boolean); | ||
| const candidates = [socketAddress(c) ?? 'unknown', ...entries.reverse()]; | ||
| return stripPort(candidates[Math.min(trustedHops, candidates.length - 1)]); |
There was a problem hiding this comment.
It appears that this issue flagged by CodeRabbit is accurate.
clientKey degrades to an attacker-controlled value when trustedHops exceeds the actual chain length (src/middlewares/rate-limit.ts:53-61).
const candidates = [socketAddress(c) ?? 'unknown', ...entries.reverse()];
return stripPort(candidates[Math.min(trustedHops, candidates.length - 1)]);
When the XFF chain is shorter than trustedHops (misconfiguration, or fewer proxies than expected in front of a given instance), Math.min clamps to the last index in candidates — which is the leftmost, client-supplied XFF entry, not the socket address. The code comment even documents this as intentional ("degrades to its furthest (leftmost) available entry"), and rate-limit.test.ts's "degrades to the leftmost entry when hops exceed the chain length" test locks it in.
This matters because it's a rate-limit bypass, not just a mis-attribution: if trustedHops is ever configured higher than the real proxy count (easy to get wrong when the topology changes), an attacker can rotate the leftmost x-forwarded-for value on every request to get a fresh bucket every time, defeating the limiter entirely. CodeRabbit's suggested fix is correct — fall back to socketAddress(c) ?? 'unknown' when the chain underflows trustedHops, rather than to the leftmost header entry:
const idx = trustedHops < entries.length ? entries.length - trustedHops : -1;
return stripPort(idx >= 0 ? entries[idx] : (socketAddress(c) ?? 'unknown'));
This is the one thing I'd block on — everything else here is solid.
|
Superseded by #255, which is the same work rebased onto current main with the review feedback applied. Closing to keep the queue clean. |
2 participants
Closes #210.
Summary
Implements the two scale-out follow-ups from Kasey's review of #209: the rate limiter's hard-coded constants are now env-configurable (current values as defaults), and the trusted-proxy assumption in
clientKeyis a config knob instead of a baked-in "lastx-forwarded-forentry" rule.Default behavior is unchanged — with no
RATE_LIMIT_*vars set, the limiter still enforces 20 req/min per client IP with a 10k-bucket cap, keyed on the proxy-appended last XFF entry.Env vars (all optional)
RATE_LIMIT_WINDOW_MS60000RATE_LIMIT_MAX20RATE_LIMIT_MAX_BUCKETS10000RATE_LIMIT_TRUSTED_HOPS1Parsed via a new blank-safe
envInt()helper (same contract asenvBool): a bareRATE_LIMIT_MAX=line — exactly how.env.exampledocuments optional vars — counts as unset (default applies) instead of being coerced to0, which would otherwise fail boot for the positive vars and silently flipTRUSTED_HOPSto socket keying. Invalid values fail boot loudly through the existingEnvSchema.safeParse→ exit path.Trusted-hop client keying
clientKeynow implements standard proxy-addr semantics driven byRATE_LIMIT_TRUSTED_HOPS:1(default): last XFF entry — exactly today's Azure App Service behavior (App Service appends the real client IP; leading entries stay client-controlled/spoofable).2: second-from-last, for an extra appending LB layer in front.0: no trusted proxy — XFF ignored entirely, key on the socket address (via@hono/node-server'sgetConnInfo, try/caught so the middleware never throws at request time).'unknown'bucket (previous fallback).One deliberate improvement: local dev without a proxy now gets per-client socket buckets instead of one shared
'unknown'bucket.rateLimit()'s options are all optional now (unset fields resolve from env), the bulk-texts route uses the zero-arg form, and its OpenAPI 429 copy says "default 20 requests per minute" so operator overrides don't make the spec lie. TheTODO(#210)comment is gone.Testing
envIntunit cases; 9 middleware cases covering the full hops matrix:0/1/2, short chains, missing XFF, missing socket, env-default resolution, custommaxBucketseviction). All 7 pre-existing limiter tests pass byte-untouched — the hops=1 default is a strict generalization of the old rule.RATE_LIMIT_TRUSTED_HOPS=-1fails boot naming the field;RATE_LIMIT_MAX=5override is picked up.Non-blocking follow-up (from review)
env.test.tsexercisesenvIntthrough ad-hoc schemas; asserting blank⇒default against the concreteRATE_LIMIT_*schema fields would needEnvSchemaexported — worth a tiny follow-up if we want the wiring itself pinned.Summary by CodeRabbit
New Features
Bug Fixes
Documentation