feat(api): add rate limiting for high-risk public routes#66
feat(api): add rate limiting for high-risk public routes#66Caritajoe18 wants to merge 2 commits into
Conversation
|
@Caritajoe18 is attempting to deploy a commit to the flamki's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Review limit reached
More reviews will be available in 54 minutes and 39 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThis PR implements rate limiting for public API routes by adding configurable per-endpoint request throttling via a fixed-window middleware. Requests are identified from headers, tracked in memory, and excess traffic returns HTTP 429 with retry hints. Three environment variables control limits for ChangesRate Limiting for Public API Routes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
@Caritajoe18 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.env.example (1)
46-49: ⚡ Quick winDocument the configurable window-size overrides.
src/config.jsreadsRATE_LIMIT_DEFAULT_WINDOW_SEC,RATE_LIMIT_ORCHESTRATE_WINDOW_SEC, andRATE_LIMIT_APIKEY_WINDOW_SEC, but only the*_MAXcounterparts are documented here. Adding the window vars keeps the example complete and discoverable.📝 Suggested addition
# ─── Rate Limiting ────────────────────────────────────── +RATE_LIMIT_DEFAULT_WINDOW_SEC=60 +RATE_LIMIT_DEFAULT_MAX=60 +RATE_LIMIT_ORCHESTRATE_WINDOW_SEC=60 RATE_LIMIT_ORCHESTRATE_MAX=10 +RATE_LIMIT_APIKEY_WINDOW_SEC=60 RATE_LIMIT_APIKEY_MAX=5 -RATE_LIMIT_DEFAULT_MAX=60🤖 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 @.env.example around lines 46 - 49, The .env.example is missing the window-size environment variables referenced by src/config.js; add RATE_LIMIT_DEFAULT_WINDOW_SEC, RATE_LIMIT_ORCHESTRATE_WINDOW_SEC, and RATE_LIMIT_APIKEY_WINDOW_SEC (with sensible default values) next to the existing RATE_LIMIT_*_MAX entries so the example matches the variables read by the application and is discoverable for users.
🤖 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/middleware/rateLimiter.js`:
- Around line 37-38: The console.warn in src/middleware/rateLimiter.js is
currently logging the raw "id" (which can be "key:<raw api-key/authorization>")
and thus leaks credentials; update the RateLimiter logging to never include the
raw id but instead log a deterministic redacted identifier (e.g., a short hash
or truncated token) — compute a hash using Node's crypto
(crypto.createHash('sha256').update(id).digest('hex') and slice to 6–8 chars, or
consistently truncate/prefix the id) and emit that redactedId in the
console.warn along with requestId, path (req.originalUrl), method, max and
windowSec; ensure the original id variable is not printed or serialized
elsewhere in this log.
- Around line 15-33: The custom fixed-window limiter createFixedWindowLimiter
has an unbounded Map named store keyed by getIdentifier(req), which lets
attackers grow memory by rotating identifiers; update limiter to evict stale
buckets and cap growth: when processing in limiter check store entries and
delete keys whose entry.windowStart + windowMs < now (or maintain a small
LRU/size limit and delete oldest when store.size > maxEntries), and/or enforce a
configurable maxEntries param to refuse or fallback when exceeded; alternatively
replace this custom limiter with express-rate-limit (ensure version >= 8.3.0)
and wire it into the same middleware path to get maintained eviction/IPv4-mapped
IPv6 fixes.
- Around line 6-13: getIdentifier currently trusts client-controlled headers and
returns raw values which enables bypass and leaks sensitive data; change
getIdentifier in rateLimiter.js to derive a stable server-side identifier (use
req.ip or the last element of req.ips only if trust proxy is properly
configured) and remove direct parsing of X-Forwarded-For or raw Authorization
headers unless the key has been validated/authorized first (only then map to a
server-side key like userId). Replace the plain in-memory Map with a
bounded/evicting store (LRU or TTL-backed map) or enforce a maximum size and
eviction policy to prevent unbounded growth, and stop logging the full
identifier on 429s—log a non-sensitive masked or generic label instead.
---
Nitpick comments:
In @.env.example:
- Around line 46-49: The .env.example is missing the window-size environment
variables referenced by src/config.js; add RATE_LIMIT_DEFAULT_WINDOW_SEC,
RATE_LIMIT_ORCHESTRATE_WINDOW_SEC, and RATE_LIMIT_APIKEY_WINDOW_SEC (with
sensible default values) next to the existing RATE_LIMIT_*_MAX entries so the
example matches the variables read by the application and is discoverable for
users.
🪄 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
Run ID: 4bba47d5-d8d4-4306-9df1-9d8c3736e80b
📒 Files selected for processing (4)
.env.examplesrc/config.jssrc/middleware/rateLimiter.jssrc/server.js
| function getIdentifier(req) { | ||
| const hdr = req.headers['x-request-key'] || req.headers['x-api-key'] || req.headers['authorization']; | ||
| if (hdr && typeof hdr === 'string') return `key:${hdr}`; | ||
| // Prefer X-Forwarded-For if present | ||
| const xff = req.headers['x-forwarded-for']; | ||
| if (xff && typeof xff === 'string') return `ip:${xff.split(',')[0].trim()}`; | ||
| return `ip:${req.ip || 'unknown'}`; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Express 5 trust proxy req.ip X-Forwarded-For handling and spoofing
💡 Result:
In Express.js, the trust proxy setting dictates how the application identifies the client's IP address (req.ip) and connection information when sitting behind one or more reverse proxies [1][2][3]. By default, this setting is false, meaning Express treats the application as directly facing the client and derives the IP address solely from the socket connection (req.socket.remoteAddress) [1][2]. When trust proxy is enabled, Express trusts the X-Forwarded-* headers (e.g., X-Forwarded-For, X-Forwarded-Proto, X-Forwarded-Host) provided by the proxy [1][3]. Enabling this is necessary for req.ip and req.ips to correctly reflect the client's original address rather than the internal IP of the reverse proxy [1][2]. Security Risks and Spoofing The primary security risk is that X-Forwarded-For is a request header that can be easily manipulated by an end-user [3][4]. If trust proxy is misconfigured, a malicious client can inject an arbitrary IP address into the X-Forwarded-For header, and Express will accept it as the genuine client IP [3][5]. This can lead to security vulnerabilities in applications that rely on req.ip for rate limiting, access control, or logging [4][6][7]. Recommended Configuration To safely configure trust proxy, you should avoid setting it to the boolean true in production, as this instructs Express to trust the left-most entry in the X-Forwarded-For header regardless of how many proxies are in the chain [1][4][5]. Instead, use the following approaches based on your infrastructure: 1. Integer (Hop Count): If you know exactly how many proxies are between the client and your Express application, set trust proxy to that number [1][4]. Express will then count backwards from the socket connection to identify the client IP [1]. For example, app.set('trust proxy', 1) trusts the first hop (the immediate proxy) [8][6]. 2. IP/Subnet List (Most Secure): You can specify the exact IP addresses or subnets of your trusted reverse proxies [1][5]. This ensures that only headers originating from your known infrastructure are trusted, making it much harder for an attacker to spoof the client IP [1]. Crucially, regardless of your trust proxy setting, your front-facing infrastructure must be configured to remove or overwrite any incoming X-Forwarded-For, X-Forwarded-Host, and X-Forwarded-Proto headers from the client [1][2][9]. If your proxy does not do this, a client can "double-up" these headers, potentially confusing your application and bypassing your trust settings [1][9].
Citations:
- 1: https://expressjs.com/en/5x/guide/behind-proxies/
- 2: https://expressjs.com/en/guide/behind-proxies/
- 3: https://stackoverflow.com/questions/23413401/what-does-trust-proxy-actually-do-in-express-js-and-do-i-need-to-use-it
- 4: https://dev.to/alanwest/how-to-build-reliable-geo-restrictions-that-actually-hold-up-in-production-1n83
- 5: Added security advisory on setting trust proxy to true on production. expressjs/expressjs.com#1203
- 6: https://expressjs.com/en/advanced/best-practice-security/
- 7: Add Option for The 'X-Forwarded-For' header for Express 'trust proxy' zwave-js/zwave-js-ui#4535
- 8: https://dev.to/armorbreak/nodejs-express-building-real-apis-that-scale-2026-2b2b
- 9: https://community.fly.io/t/recommended-setting-for-trust-proxy-on-express/6346
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la
sed -n '1,120p' src/middleware/rateLimiter.js
sed -n '120,240p' src/middleware/rateLimiter.jsRepository: Flamki/stellarmind
Length of output: 4391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# find other references to getIdentifier or "x-request-key" in the repo
rg -n "getIdentifier|x-request-key|x-api-key|x-forwarded-for|rateLimiter" -S src || trueRepository: Flamki/stellarmind
Length of output: 790
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# check where rateLimiter is used / any trust proxy setup
rg -n "trust proxy|trust_proxy|set\\('trust proxy'|app\\.set\\('trust proxy'|app\\.set\\(\"trust proxy\"" -S . || trueRepository: Flamki/stellarmind
Length of output: 44
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# look for logging of identifiers or "key:" in server code
rg -n "key:|x-request-key|x-api-key|authorization|x-forwarded-for|rate limit|rateLimiter" -S src || trueRepository: Flamki/stellarmind
Length of output: 912
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' src/server.js
sed -n '200,330p' src/server.js
# Look for trust proxy and auth middleware ordering
rg -n "trust proxy|set\\('trust proxy'|authorization|apikeyLimiter|orchestrateLimiter" src/server.js src -SRepository: Flamki/stellarmind
Length of output: 9231
Rate limiter identifier is client-controlled (enables bypass), leaks it in logs, and can grow memory unbounded.
In src/middleware/rateLimiter.js, getIdentifier(req) uses x-request-key/x-api-key/authorization as key:<value> and falls back to directly trusting/parsing x-forwarded-for as ip:<value>, so a client can rotate/spoof these headers to target unlimited distinct buckets and defeat throttling. The in-memory Map never evicts entries (only resets counts), so unique identifiers can grow without bound. On 429s it also logs id (which includes the header value), contradicting the “Clear sensitive details in logs” comment and risking token/key leakage.
Use a server-derived, stable key (e.g., req.ip / req.ips with correctly configured trust proxy) and avoid parsing X-Forwarded-For directly; if using API keys for rate limiting, validate/authorize the key before using it. Add eviction/size limits to the store and don’t log the identifier value.
🤖 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/middleware/rateLimiter.js` around lines 6 - 13, getIdentifier currently
trusts client-controlled headers and returns raw values which enables bypass and
leaks sensitive data; change getIdentifier in rateLimiter.js to derive a stable
server-side identifier (use req.ip or the last element of req.ips only if trust
proxy is properly configured) and remove direct parsing of X-Forwarded-For or
raw Authorization headers unless the key has been validated/authorized first
(only then map to a server-side key like userId). Replace the plain in-memory
Map with a bounded/evicting store (LRU or TTL-backed map) or enforce a maximum
size and eviction policy to prevent unbounded growth, and stop logging the full
identifier on 429s—log a non-sensitive masked or generic label instead.
| function createFixedWindowLimiter({ windowSec, max }) { | ||
| const windowMs = windowSec * 1000; | ||
| // Map identifier -> { count, windowStart } | ||
| const store = new Map(); | ||
|
|
||
| return function limiter(req, res, next) { | ||
| try { | ||
| const id = getIdentifier(req); | ||
| const now = Date.now(); | ||
| const entry = store.get(id) || { count: 0, windowStart: now }; | ||
|
|
||
| // Reset if window elapsed | ||
| if (now - entry.windowStart >= windowMs) { | ||
| entry.count = 0; | ||
| entry.windowStart = now; | ||
| } | ||
|
|
||
| entry.count += 1; | ||
| store.set(id, entry); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
express-rate-limit compatibility with Express 5 latest version
💡 Result:
express-rate-limit is compatible with Express 5’s latest major versions as long as you use a current express-rate-limit release; it declares a peer dependency of express >= 4.11 (so it does not exclude Express 5) [1][2]. However, there are two practical “gotchas” to consider when running with Express 5: 1) Version/upgrade expectations: Express 5 does have breaking changes vs Express 4, so your app (not necessarily express-rate-limit) may need adjustments when upgrading to Express 5 [3]. 2) IPv4-mapped IPv6 behavior (rate-limit bucketing): express-rate-limit’s default keyGenerator can collapse IPv4 clients into a single bucket on dual-stack servers; the project explicitly tested the behavior with Express 5.2.1 and explains that it occurs for request.ip values like::ffff:127.0.0.1, and was fixed by default in express-rate-limit v8.3.0 (and backported to 8.2.2 / 8.1.1 / 8.0.2) [4]. What to do (recommended): - Use the latest express-rate-limit version available (today the npm page shows 8.5.2 published May 14, 2026) [2]. - If you deploy on dual-stack (listen on::) and haven’t upgraded past the fix window, upgrade to >= 8.3.0 so you get the IPv4-mapped IPv6 fix included [4]. - Run your Express 5 migration/tests because Express 5 changes can break application code/middleware patterns even if the rate limiter itself is compatible [3].
Citations:
- 1: https://www.npmjs.com/package/express-rate-limit
- 2: https://registry.npmjs.org/express-rate-limit
- 3: https://expressjs.com/en/guide/migrating-5/
- 4: https://www.blogarama.com/software-blogs/1361666-wordpress-development-blog/76490616-nodejs-rest-api-build-fast-apis-express
Fix unbounded per-identifier growth in src/middleware/rateLimiter.js
store is a Map that only resets per-key counters; it never deletes expired buckets. If getIdentifier(req) can be influenced (e.g., via a request header), rotating identifiers can grow the Map without bound and exhaust memory (DoS). Add bounded eviction (e.g., delete entries whose windowStart is older than windowMs, optionally with a max size), or use a maintained limiter instead—express-rate-limit works with Express 5 (peer dep express >= 4.11) and the latest releases include fixes like the IPv4-mapped IPv6 bucketing behavior (use a current express-rate-limit, ideally >= 8.3.0).
♻️ Minimal eviction sketch (if keeping the custom limiter)
return function limiter(req, res, next) {
try {
const id = getIdentifier(req);
const now = Date.now();
const entry = store.get(id) || { count: 0, windowStart: now };
// Reset if window elapsed
if (now - entry.windowStart >= windowMs) {
entry.count = 0;
entry.windowStart = now;
}
+
+ // Opportunistically drop stale buckets to bound memory
+ if (store.size > 10000) {
+ for (const [k, v] of store) {
+ if (now - v.windowStart >= windowMs) store.delete(k);
+ }
+ }
entry.count += 1;
store.set(id, 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/middleware/rateLimiter.js` around lines 15 - 33, The custom fixed-window
limiter createFixedWindowLimiter has an unbounded Map named store keyed by
getIdentifier(req), which lets attackers grow memory by rotating identifiers;
update limiter to evict stale buckets and cap growth: when processing in limiter
check store entries and delete keys whose entry.windowStart + windowMs < now (or
maintain a small LRU/size limit and delete oldest when store.size > maxEntries),
and/or enforce a configurable maxEntries param to refuse or fallback when
exceeded; alternatively replace this custom limiter with express-rate-limit
(ensure version >= 8.3.0) and wire it into the same middleware path to get
maintained eviction/IPv4-mapped IPv6 fixes.
| // Clear sensitive details in logs | ||
| console.warn(`Rate limit exceeded`, { requestId: req.requestId, id, path: req.originalUrl, method: req.method, max, windowSec }); |
There was a problem hiding this comment.
Logging id leaks API keys / Authorization tokens.
When the identifier comes from a header, id is key:<raw x-api-key / authorization / x-request-key value>. Logging it here writes the full credential to stdout, contradicting the comment on Line 37 ("Clear sensitive details in logs") and the deliberate secret-safe logging in src/server.js (Lines 376-377, "never log the key itself"). Log a hashed or truncated identifier instead.
🛡️ Proposed fix
- console.warn(`Rate limit exceeded`, { requestId: req.requestId, id, path: req.originalUrl, method: req.method, max, windowSec });
+ // Avoid logging raw credentials; redact key-based identifiers
+ const safeId = id.startsWith('key:') ? 'key:<redacted>' : id;
+ console.warn('Rate limit exceeded', { requestId: req.requestId, id: safeId, path: req.originalUrl, method: req.method, max, windowSec });🤖 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/middleware/rateLimiter.js` around lines 37 - 38, The console.warn in
src/middleware/rateLimiter.js is currently logging the raw "id" (which can be
"key:<raw api-key/authorization>") and thus leaks credentials; update the
RateLimiter logging to never include the raw id but instead log a deterministic
redacted identifier (e.g., a short hash or truncated token) — compute a hash
using Node's crypto (crypto.createHash('sha256').update(id).digest('hex') and
slice to 6–8 chars, or consistently truncate/prefix the id) and emit that
redactedId in the console.warn along with requestId, path (req.originalUrl),
method, max and windowSec; ensure the original id variable is not printed or
serialized elsewhere in this log.
1 participant
Closes #11
Add a simple in-memory fixed-window rate limiter and apply it to high-risk public endpoints to protect against API abuse and cost spikes.
Summary by CodeRabbit
Release Notes