feat(rate-limit): update @routup/rate-limit for routup v5#766
feat(rate-limit): update @routup/rate-limit for routup v5#766
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughMigrates rate-limit packages to routup v5: updates peer/dev dependencies, refactors handlers and types from (req,res,next) to IRoutupEvent/defineCoreHandler, moves per-request storage to event.store, adjusts tests to Fetch API, and awaits Redis commands in the Redis store. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router as Router/Handler
participant Store as In-Memory / Redis Store
participant Redis as Redis Client
participant Response
Client->>Router: HTTP request -> defineCoreHandler(event)
Router->>Store: build key / increment(key)
Store->>Redis: await client.set(key, value) rgba(100,149,237,0.5)
Redis-->>Store: OK
Store-->>Router: totalHits, remaining, reset
Router->>Router: if exceeded -> return handler(event, options)
Router->>Router: else await event.next()
Router->>Response: downstream response
Router->>Store: conditional decrement based on response (decrement -> Redis del/set)
Store->>Redis: await client.del/set(...) rgba(60,179,113,0.5)
Redis-->>Store: OK
Router-->>Client: final response with rate-limit headers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
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 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/rate-limit-redis/package.json (1)
47-60:⚠️ Potential issue | 🟠 MajorAlign the redis package metadata with the new core major.
This now requires
@routup/rate-limit ^3.0.0androutup ^5.0.0-beta.3, but Line 3 still publishes3.1.2and Line 58 still develops against@routup/rate-limit ^2.4.2. Please bump this package to the next major and test/build it against v3 as well, otherwise local validation is happening against a different API than the one you publish.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rate-limit-redis/package.json` around lines 47 - 60, The package.json is inconsistent: peerDependencies require `@routup/rate-limit` ^3.0.0 and routup ^5.0.0-beta.3 but devDependencies still reference `@routup/rate-limit` ^2.4.2 and redis-extension ^2.0.4; bump this package version to the next major, update devDependencies to match the published peers (set `@routup/rate-limit` to ^3.0.0, set routup to ^5.0.0-beta.3, and align redis-extension to the peer range or make it optional like in peerDependencies), then run the project’s test/build against the v3 `@routup/rate-limit` API to ensure compatibility before publishing.packages/rate-limit/src/type.ts (1)
123-129:⚠️ Potential issue | 🟠 MajorProvide event context in
requestWasSuccessfulfor consistency.This is the only user hook that cannot access the routup event. All other hooks (
keyGenerator,skip,handler,max,message) can access the event through their signatures. This makes it impossible for users to implement predicates that depend on both request metadata and the final response. Change the signature to(event: IRoutupEvent, response: Response) => booleanto align with other hooks and enable this use case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rate-limit/src/type.ts` around lines 123 - 129, Update the requestWasSuccessful hook signature from (response: Response) => boolean to (event: IRoutupEvent, response: Response) => boolean in the rate-limit types so it matches other hooks; change the declaration of requestWasSuccessful in the relevant interface in packages/rate-limit/src/type.ts to accept (event: IRoutupEvent, response: Response), then update all call sites (where requestWasSuccessful(...) is invoked) to pass the current IRoutupEvent along with the Response and adjust any default implementation or tests to the new two-argument form; ensure imports/reference to IRoutupEvent and Response are added or preserved so types compile.packages/rate-limit-redis/src/module.ts (2)
76-87:⚠️ Potential issue | 🟠 MajorAwait the Redis write in
decrement().The
increment()method (line 54) properly awaitsthis.client.set(), butdecrement()(line 82) does not, despite both performing write operations. Whilehandler.tsline 75 awaitsoptions.store.decrement(key), the promise resolves before the write completes, allowing subsequent requests to observe stale quota state.Suggested fix
async decrement(key: string): Promise<void> { key = this.buildKey(key); const entry = await this.client.get(key); if (entry) { const totalHits = Number.parseInt(entry, 10) - 1; - this.client.set( + await this.client.set( key, totalHits, 'PX', this.options.windowMs, ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rate-limit-redis/src/module.ts` around lines 76 - 87, The decrement() method currently calls this.client.set(...) without awaiting it, causing the promise to resolve before the Redis write finishes; update the decrement implementation in the class (method: decrement, uses buildKey and this.client.set with this.options.windowMs) to await the Redis write (i.e., await this.client.set(...)) so the method only resolves after the write completes, ensuring callers that await options.store.decrement(key) see the updated quota state.
43-59:⚠️ Potential issue | 🔴 Critical
increment()uses non-atomic read-then-write, allowing lost updates under concurrent requests.Lines 45 and 54 split the counter update across two Redis commands:
get()followed byset(). Under concurrent load, multiple requests can read the same value, all increment it independently in JavaScript, and write back the same count—undercounting hits and enabling rate-limit bypass. Use Redis's atomicincr()orincrby()method (available via the ioredis client) instead, paired withexpire()orpexpire()for TTL, or use a Lua script to atomically combine increment and expiry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rate-limit-redis/src/module.ts` around lines 43 - 59, The increment() implementation currently does a non-atomic this.client.get(...) followed by this.client.set(...), which allows lost updates under concurrency; replace that read-then-write with an atomic Redis increment and TTL operation: call this.client.incr(key) (or incrby) to get the updated totalHits and then set the TTL with this.client.pexpire(key, this.options.windowMs) (or use a single EVAL Lua script that does INCR and sets PX only when needed) so the counter update and expiry are atomic; update references to totalHits to use the value returned by incr and remove the get/set pair.
🧹 Nitpick comments (1)
packages/rate-limit/src/utils/options.ts (1)
24-32: UseoptionsUsedin the handler instead of closing overoptions.On Line [24]-Line [32], the default handler ignores its
optionsUsedparameter and reads from outeroptions. UsingoptionsUseddirectly reduces coupling and avoids surprises if invocation-time options differ.Refactor proposal
- const message: unknown = typeof options.message === 'function' ? - await (options.message as ValueDeterminingMiddleware<any>)( + const message: unknown = typeof optionsUsed.message === 'function' ? + await (optionsUsed.message as ValueDeterminingMiddleware<any>)( event, ) : - options.message; + optionsUsed.message; - event.response.status = options.statusCode; + event.response.status = optionsUsed.statusCode;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rate-limit/src/utils/options.ts` around lines 24 - 32, The default handler is closing over the outer options variable instead of using its handler parameter optionsUsed; update the handler to read from optionsUsed (use optionsUsed.message and optionsUsed.statusCode) when computing message and setting event.response.status so invocation-time options are respected — e.g. await (optionsUsed.message as ValueDeterminingMiddleware<any>)(event) when message is a function, set event.response.status = optionsUsed.statusCode, and keep the existing fallback string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rate-limit/package.json`:
- Around line 48-52: The package metadata is inconsistent:
packages/rate-limit/package.json still has "version": "2.4.2" while dependent
packages (packages/rate-limit-redis) peer on `@routup/rate-limit` ^3.0.0 and this
release is routup v5-compatible, so bump the major version in
packages/rate-limit/package.json to the new major (e.g., set "version" to
"3.0.0"), update any internal references/changelog/release notes to reflect the
major bump, and ensure peerDependencies and devDependencies remain correct for
routup v5 compatibility before publishing the new major.
In `@packages/rate-limit/src/handler.ts`:
- Around line 49-52: The rate-limit check currently short-circuits when maxHits
is falsy (so max: 0 is ignored); update the conditional that uses `maxHits &&
totalHits > maxHits` to remove the truthy check and directly compare `totalHits
> maxHits` (i.e., change the guard in the rate-limit handler that references
`maxHits` and `totalHits` so 0 is treated as a valid limit and the handler
blocks when totalHits > maxHits).
In `@packages/rate-limit/src/request.ts`:
- Around line 6-18: Update the overloads for useRequestRateLimitInfo to reflect
that the rate-limit state may be missing: change the full-record overload to
return Partial<RateLimitInfo> and change the keyed overload to return
RateLimitInfo[K] | undefined; in addition adjust the implementation (the
function body using symbol and event.store) to match these signatures (i.e.,
allow returning {} as Partial<RateLimitInfo> and allow returning possibly
undefined for keyed lookups from (event.store[symbol] as Record<string,
unknown>)[key]). Ensure references to IRoutupEvent, RateLimitInfo, symbol, and
event.store remain correct.
In `@packages/rate-limit/src/utils/options.ts`:
- Line 19: Add a new boolean field trustProxy to the Options type (in the
rate-limit package's type.ts) with a comment that it defaults to false and
controls whether forwarded headers are trusted; then update
buildHandlerOptions() to set trustProxy: false in its returned/default options
and change the keyGenerator in options.ts to call getRequestIP(event, {
trustProxy: options.trustProxy }) (instead of the hardcoded true) so the
behavior is configurable via Options.
---
Outside diff comments:
In `@packages/rate-limit-redis/package.json`:
- Around line 47-60: The package.json is inconsistent: peerDependencies require
`@routup/rate-limit` ^3.0.0 and routup ^5.0.0-beta.3 but devDependencies still
reference `@routup/rate-limit` ^2.4.2 and redis-extension ^2.0.4; bump this
package version to the next major, update devDependencies to match the published
peers (set `@routup/rate-limit` to ^3.0.0, set routup to ^5.0.0-beta.3, and align
redis-extension to the peer range or make it optional like in peerDependencies),
then run the project’s test/build against the v3 `@routup/rate-limit` API to
ensure compatibility before publishing.
In `@packages/rate-limit-redis/src/module.ts`:
- Around line 76-87: The decrement() method currently calls this.client.set(...)
without awaiting it, causing the promise to resolve before the Redis write
finishes; update the decrement implementation in the class (method: decrement,
uses buildKey and this.client.set with this.options.windowMs) to await the Redis
write (i.e., await this.client.set(...)) so the method only resolves after the
write completes, ensuring callers that await options.store.decrement(key) see
the updated quota state.
- Around line 43-59: The increment() implementation currently does a non-atomic
this.client.get(...) followed by this.client.set(...), which allows lost updates
under concurrency; replace that read-then-write with an atomic Redis increment
and TTL operation: call this.client.incr(key) (or incrby) to get the updated
totalHits and then set the TTL with this.client.pexpire(key,
this.options.windowMs) (or use a single EVAL Lua script that does INCR and sets
PX only when needed) so the counter update and expiry are atomic; update
references to totalHits to use the value returned by incr and remove the get/set
pair.
In `@packages/rate-limit/src/type.ts`:
- Around line 123-129: Update the requestWasSuccessful hook signature from
(response: Response) => boolean to (event: IRoutupEvent, response: Response) =>
boolean in the rate-limit types so it matches other hooks; change the
declaration of requestWasSuccessful in the relevant interface in
packages/rate-limit/src/type.ts to accept (event: IRoutupEvent, response:
Response), then update all call sites (where requestWasSuccessful(...) is
invoked) to pass the current IRoutupEvent along with the Response and adjust any
default implementation or tests to the new two-argument form; ensure
imports/reference to IRoutupEvent and Response are added or preserved so types
compile.
---
Nitpick comments:
In `@packages/rate-limit/src/utils/options.ts`:
- Around line 24-32: The default handler is closing over the outer options
variable instead of using its handler parameter optionsUsed; update the handler
to read from optionsUsed (use optionsUsed.message and optionsUsed.statusCode)
when computing message and setting event.response.status so invocation-time
options are respected — e.g. await (optionsUsed.message as
ValueDeterminingMiddleware<any>)(event) when message is a function, set
event.response.status = optionsUsed.statusCode, and keep the existing fallback
string.
🪄 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: ad166094-4a6e-46f4-991e-e8a1b1ebc19b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
packages/rate-limit-redis/package.jsonpackages/rate-limit-redis/src/module.tspackages/rate-limit/package.jsonpackages/rate-limit/src/handler.tspackages/rate-limit/src/module.tspackages/rate-limit/src/request.tspackages/rate-limit/src/type.tspackages/rate-limit/src/utils/options.tspackages/rate-limit/test/middleware.tspackages/rate-limit/test/unit/module.spec.ts
💤 Files with no reviewable changes (1)
- packages/rate-limit/test/middleware.ts
| "peerDependencies": { | ||
| "routup": "^4.0.1" | ||
| "routup": "^5.0.0-beta.3" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/supertest": "^7.2.0", | ||
| "routup": "^4.0.1", | ||
| "supertest": "^7.1.4" | ||
| "routup": "^5.0.0-beta.3" |
There was a problem hiding this comment.
Publish this as the next major.
These dependency changes make this the routup v5-compatible release, but Line 3 still says 2.4.2. packages/rate-limit-redis/package.json now peers on @routup/rate-limit ^3.0.0, so leaving this package on v2 makes the metadata internally inconsistent and ships a breaking API under the old major.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rate-limit/package.json` around lines 48 - 52, The package metadata
is inconsistent: packages/rate-limit/package.json still has "version": "2.4.2"
while dependent packages (packages/rate-limit-redis) peer on `@routup/rate-limit`
^3.0.0 and this release is routup v5-compatible, so bump the major version in
packages/rate-limit/package.json to the new major (e.g., set "version" to
"3.0.0"), update any internal references/changelog/release notes to reflect the
major bump, and ensure peerDependencies and devDependencies remain correct for
routup v5 compatibility before publishing the new major.
| if ( | ||
| options.skipFailedRequest || | ||
| options.skipSuccessfulRequest | ||
| maxHits && | ||
| totalHits > maxHits | ||
| ) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "handler.ts" | grep -E "rate-limit|rate_limit"Repository: routup/plugins
Length of output: 94
🏁 Script executed:
cat -n packages/rate-limit/src/handler.tsRepository: routup/plugins
Length of output: 3145
🏁 Script executed:
fd -t f "utils\." packages/rate-limit/src/ | head -10Repository: routup/plugins
Length of output: 96
🏁 Script executed:
cat -n packages/rate-limit/src/utils.tsRepository: routup/plugins
Length of output: 122
🏁 Script executed:
rg "buildHandlerOptions" packages/rate-limit/src/ -lRepository: routup/plugins
Length of output: 133
🏁 Script executed:
rg "export.*buildHandlerOptions" packages/rate-limit/src/ -A 20Repository: routup/plugins
Length of output: 1802
🏁 Script executed:
cat -n packages/rate-limit/src/utils/options.tsRepository: routup/plugins
Length of output: 1755
🏁 Script executed:
cat -n packages/rate-limit/src/type.tsRepository: routup/plugins
Length of output: 4609
🏁 Script executed:
rg "max.*0\|max:.*0" packages/rate-limit/ -B 2 -A 2Repository: routup/plugins
Length of output: 40
🏁 Script executed:
rg "test\|spec" packages/rate-limit/ -l | head -5Repository: routup/plugins
Length of output: 40
🏁 Script executed:
find packages/rate-limit -type f -name "*.test.*" -o -name "*.spec.*" 2>/dev/nullRepository: routup/plugins
Length of output: 102
🏁 Script executed:
ls -la packages/rate-limit/Repository: routup/plugins
Length of output: 725
🏁 Script executed:
cat -n packages/rate-limit/test/unit/module.spec.tsRepository: routup/plugins
Length of output: 4381
Remove the truthy check on maxHits to handle max: 0 correctly.
When max is set to 0, the condition maxHits && totalHits > maxHits evaluates to false since 0 is falsy in JavaScript, bypassing the rate-limit handler entirely. This means a configuration of max: 0 (which should block all requests) instead allows them all. Remove the truthy check to directly compare totalHits > maxHits.
Suggested fix
- if (
- maxHits &&
- totalHits > maxHits
- ) {
+ if (totalHits > maxHits) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| options.skipFailedRequest || | |
| options.skipSuccessfulRequest | |
| maxHits && | |
| totalHits > maxHits | |
| ) { | |
| if (totalHits > maxHits) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rate-limit/src/handler.ts` around lines 49 - 52, The rate-limit
check currently short-circuits when maxHits is falsy (so max: 0 is ignored);
update the conditional that uses `maxHits && totalHits > maxHits` to remove the
truthy check and directly compare `totalHits > maxHits` (i.e., change the guard
in the rate-limit handler that references `maxHits` and `totalHits` so 0 is
treated as a valid limit and the handler blocks when totalHits > maxHits).
| keyGenerator: (request: Request, _response: Response): string => getRequestIP(request, { trustProxy: true }), | ||
| requestWasSuccessful: (response: Response): boolean => response.status < 400, | ||
| skip: (_event: IRoutupEvent): boolean => false, | ||
| keyGenerator: (event: IRoutupEvent): string => getRequestIP(event, { trustProxy: true }) || '127.0.0.1', |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In routup v5, what are the security recommendations for using getRequestIP(event, { trustProxy: true }) in rate-limiting middleware?
💡 Result:
In Routup v5, getRequestIP(event, { trustProxy: true }) will fall back to inspecting the X-Forwarded-For header to determine the client IP (instead of only using the direct/“runtime” remote address) when trustProxy is enabled. Use it only when your deployment makes that header trustworthy. [1]
Security recommendations (for rate-limiting keys):
- Enable
trustProxyonly behind a reverse proxy/load balancer you control (whereX-Forwarded-Foris set/managed by that proxy). Otherwise clients can spoofX-Forwarded-Forand evade per-IP limits. [2][3] - Ensure the app is not directly reachable from the internet (only the proxy can connect to it), and configure the proxy to strip/overwrite incoming
X-Forwarded-Forfrom clients before adding its own value. [2] - Treat
X-Forwarded-Foras a trust-boundary configuration problem: only trust forwarded-IP headers from known proxy sources, and guard against long/attacker-controlled chains (misreading the “leftmost” entry is a common failure mode). [3] - If you use Routup’s rate-limit middleware default of “IP-based keying” (or a custom
keyGeneratorusinggetRequestIP(..., { trustProxy: true })), apply the above hardening; otherwise IP-based limits can be bypassed. [4][1]
Sources: Routup request helpers (getRequestIP + trustProxy) [1], forwarded-header spoofing/trust-boundary guidance [3], and common proxy-hardening guidance (only proxy can reach app + strip X-Forwarded-For) [2], plus Routup rate-limit docs showing IP-based key generation via getRequestIP(..., { trustProxy: true }) [4]. [1][2][3][4]
🏁 Script executed:
cat -n packages/rate-limit/src/utils/options.tsRepository: routup/plugins
Length of output: 1755
🏁 Script executed:
rg -A 10 -B 10 "keyGenerator.*getRequestIP" packages/rate-limit/src/utils/options.tsRepository: routup/plugins
Length of output: 880
🏁 Script executed:
rg "type Options\|interface Options" packages/rate-limit/src/utils/options.ts -A 20Repository: routup/plugins
Length of output: 40
🏁 Script executed:
rg "trustProxy" packages/rate-limit/src/ -rRepository: routup/plugins
Length of output: 471
🏁 Script executed:
find packages/rate-limit/src -name "type.ts" -o -name "types.ts" | head -5Repository: routup/plugins
Length of output: 127
🏁 Script executed:
cat -n packages/rate-limit/src/type.tsRepository: routup/plugins
Length of output: 4609
🏁 Script executed:
rg "trustProxy" packages/rate-limit/Repository: routup/plugins
Length of output: 327
🏁 Script executed:
rg -l "rate-limit\|buildHandlerOptions" packages/rate-limit/test packages/rate-limit/README.md 2>/dev/null | head -5Repository: routup/plugins
Length of output: 40
🏁 Script executed:
cat packages/rate-limit/README.md | grep -A 20 -B 5 "trustProxy\|getRequestIP"Repository: routup/plugins
Length of output: 692
🏁 Script executed:
fd "\.test\.|\.spec\." packages/rate-limit/src --type f | head -3 | xargs grep -l "buildHandlerOptions\|keyGenerator" 2>/dev/nullRepository: routup/plugins
Length of output: 42
Add trustProxy field to Options and make it configurable to mitigate rate-limit bypass risk.
Hardcoding trustProxy: true enables inspection of forwarded headers by default, allowing clients to spoof IPs and evade throttling in non-proxied or misconfigured setups. This needs to be configurable and default to false.
Required changes:
- Add
trustProxy: booleanfield to theOptionstype inpackages/rate-limit/src/type.ts - Add
trustProxy: falseas the default inbuildHandlerOptions()at line 19 - Update the
keyGeneratorto useoptions.trustProxyinstead of hardcodedtrue
Suggested implementation
In type.ts, add to the Options type:
/**
* Whether to trust forwarded IP headers (X-Forwarded-For, etc).
* Only enable when behind a trusted reverse proxy that manages these headers.
*
* Defaults to `false`.
*/
trustProxy: booleanIn options.ts, update line 19:
keyGenerator: (event: IRoutupEvent): string => getRequestIP(event, { trustProxy: options.trustProxy }) || '127.0.0.1',And add the default value in the options object:
const options : Options = {
+ trustProxy: false,
windowMs: 60 * 1000,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rate-limit/src/utils/options.ts` at line 19, Add a new boolean field
trustProxy to the Options type (in the rate-limit package's type.ts) with a
comment that it defaults to false and controls whether forwarded headers are
trusted; then update buildHandlerOptions() to set trustProxy: false in its
returned/default options and change the keyGenerator in options.ts to call
getRequestIP(event, { trustProxy: options.trustProxy }) (instead of the
hardcoded true) so the behavior is configurable via Options.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
closes #749
closes #750
Summary by CodeRabbit
Breaking Changes
Bug Fixes
Chores
Documentation