feat(rate-limit): rate-limiter module#126
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a complete throttling subsystem: types and DI/metadata constants, decorators ( ChangesThrottler/Rate-Limiter Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant ThrottlerGuard
participant InMemoryThrottlerStorage
participant Response
Client->>ThrottlerGuard: HTTP request (context)
ThrottlerGuard->>ThrottlerGuard: Read `@SkipThrottle` / `@Throttle` metadata
ThrottlerGuard->>ThrottlerGuard: Resolve client key (x-forwarded-for / x-real-ip / remoteAddr)
ThrottlerGuard->>InMemoryThrottlerStorage: increment(key, ttl)
InMemoryThrottlerStorage->>ThrottlerGuard: { totalHits, timeToExpire }
alt totalHits > limit
ThrottlerGuard->>Response: throw ThrottlerException (429) + set Retry-After
else within limit
ThrottlerGuard->>Response: set X-RateLimit-Limit/Remaining/Reset headers
Response->>Client: proceed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
CLAUDE.md (1)
115-124: ⚡ Quick winMake the test example cleanup exception-safe with
try/finally.The sample calls
await app.close(), but iffetch()or an assertion throws first, cleanup is skipped. Wrapping the test body intry/finallyprevents leaked listeners in copied tests.Suggested doc patch
Deno.test('GET', async () => { const app = new DanetApplication(); - await app.init(MyModule); - const { port } = await app.listen(0); // 0 = random free port - - const res = await fetch(`http://localhost:${port}/nice-controller`); - assertEquals(await res.text(), 'OK GET'); - - await app.close(); // always clean up + try { + await app.init(MyModule); + const { port } = await app.listen(0); // 0 = random free port + + const res = await fetch(`http://localhost:${port}/nice-controller`); + assertEquals(await res.text(), 'OK GET'); + } finally { + await app.close(); // always clean up + } });🤖 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 `@CLAUDE.md` around lines 115 - 124, The test snippet using DanetApplication needs exception-safe cleanup: wrap the test body that calls app.init(MyModule) and app.listen(0) and performs fetch/assertions in a try/finally so that app.close() is always called; specifically, after creating const app = new DanetApplication() and awaiting app.init(...) and app.listen(...), move the fetch/assert logic into a try block and call await app.close() in the finally block to guarantee listener cleanup even if fetch() or assertions throw.src/throttler/guard.ts (1)
119-131: ⚡ Quick winBe aware of proxy header spoofing risk.
The
getTrackermethod trustsx-forwarded-forandx-real-ipheaders to identify clients behind proxies. These headers can be spoofed by malicious clients if the application is directly exposed to the internet without a trusted reverse proxy. This could allow attackers to:
- Bypass rate limits by rotating the header value
- Frame other IP addresses to exhaust their quotas
Mitigation:
- Deploy behind a trusted reverse proxy that strips/rewrites these headers
- Override
getTrackerto validate headers or use authenticated identity (API key, user ID) for critical routes- Consider using
remoteAddronly if not behind a proxyThe method is correctly marked
protectedto enable customization.🤖 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/throttler/guard.ts` around lines 119 - 131, The getTracker method currently trusts request headers ('x-forwarded-for', 'x-real-ip') directly; update it to mitigate header spoofing by: 1) checking a configured "trustedProxy" flag or list before using 'x-forwarded-for'/'x-real-ip' inside getTracker(ExecutionContext) and falling back to remoteAddr.hostname if not trusted; 2) optionally prefer authenticated identity (API key, user ID) when available (e.g., accept a token or context.user id) for rate-limiting; and 3) document/throw if headers are present but not from a trusted proxy so callers can override getTracker safely. Ensure changes reference getTracker, ExecutionContext, and the header names so reviewers can locate and customize behavior.spec/rate-limiter.test.ts (1)
212-224: 💤 Low valueConsider increasing the eviction wait time for test stability.
Line 222 waits 100ms for two 50ms timers to fire and evict their keys. While a 2× buffer is often sufficient, timer precision can vary on slower systems or under CI load. If this test becomes flaky, consider increasing the wait to 150ms or 200ms.
🤖 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 `@spec/rate-limiter.test.ts` around lines 212 - 224, The test that asserts eviction in InMemoryThrottlerStorage is waiting only 100ms for two 50ms timers to run and evict keys; increase the eviction wait in that test (the Promise/setTimeout after the storage.increment calls) to a larger value (e.g., 150ms or 200ms) so timer jitter under CI/slower machines won't cause flakes while keeping the same assertions against storage.size and the same use of InMemoryThrottlerStorage and storage.increment.
🤖 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/throttler/guard.ts`:
- Around line 49-92: The header suffix logic in canActivate incorrectly checks
throttler.name (which can be undefined) instead of the resolved name variable
(which uses DEFAULT_THROTTLER_NAME), causing unnamed throttlers to produce
identical headers and collide; fix by using the resolved name when building the
suffix (i.e., use name when computing the suffix/suffix condition for
X-RateLimit-* and Retry-After headers) so unnamed throttlers receive the
default-based suffix and headers remain unique in multi-throttler setups.
In `@src/throttler/storage.ts`:
- Around line 13-17: The records map in InMemoryThrottlerStorage declares
timeoutId as number but Deno's setTimeout returns a Timeout object, causing type
errors; update the timeoutId type to a portable type such as ReturnType<typeof
setTimeout> (or the union needed in your runtime) on the records entry so
assignments from setTimeout match the declared type, and adjust any
clearTimeout/uses accordingly to accept that type; target the records
declaration and any direct uses of timeoutId in InMemoryThrottlerStorage.
---
Nitpick comments:
In `@CLAUDE.md`:
- Around line 115-124: The test snippet using DanetApplication needs
exception-safe cleanup: wrap the test body that calls app.init(MyModule) and
app.listen(0) and performs fetch/assertions in a try/finally so that app.close()
is always called; specifically, after creating const app = new
DanetApplication() and awaiting app.init(...) and app.listen(...), move the
fetch/assert logic into a try block and call await app.close() in the finally
block to guarantee listener cleanup even if fetch() or assertions throw.
In `@spec/rate-limiter.test.ts`:
- Around line 212-224: The test that asserts eviction in
InMemoryThrottlerStorage is waiting only 100ms for two 50ms timers to run and
evict keys; increase the eviction wait in that test (the Promise/setTimeout
after the storage.increment calls) to a larger value (e.g., 150ms or 200ms) so
timer jitter under CI/slower machines won't cause flakes while keeping the same
assertions against storage.size and the same use of InMemoryThrottlerStorage and
storage.increment.
In `@src/throttler/guard.ts`:
- Around line 119-131: The getTracker method currently trusts request headers
('x-forwarded-for', 'x-real-ip') directly; update it to mitigate header spoofing
by: 1) checking a configured "trustedProxy" flag or list before using
'x-forwarded-for'/'x-real-ip' inside getTracker(ExecutionContext) and falling
back to remoteAddr.hostname if not trusted; 2) optionally prefer authenticated
identity (API key, user ID) when available (e.g., accept a token or context.user
id) for rate-limiting; and 3) document/throw if headers are present but not from
a trusted proxy so callers can override getTracker safely. Ensure changes
reference getTracker, ExecutionContext, and the header names so reviewers can
locate and customize behavior.
🪄 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: 793b2346-1947-43eb-ad60-27bbcd59463c
📒 Files selected for processing (10)
CLAUDE.mdspec/rate-limiter.test.tssrc/mod.tssrc/throttler/constants.tssrc/throttler/decorator.tssrc/throttler/guard.tssrc/throttler/interface.tssrc/throttler/mod.tssrc/throttler/module.tssrc/throttler/storage.ts
Comment on lines
+49
to
+92
| async canActivate(context: ExecutionContext): Promise<boolean> { | ||
| const handler = context.getHandler(); | ||
| const classRef = context.getClass(); | ||
|
|
||
| if (this.shouldSkip(handler) || this.shouldSkip(classRef)) { | ||
| return true; | ||
| } | ||
|
|
||
| const overrides = this.getOverrides(handler, classRef); | ||
| // When several throttlers are configured (or a single named one), the | ||
| // rate-limit headers are suffixed with the throttler name, matching | ||
| // the behavior of `@nestjs/throttler`. | ||
| const useSuffix = this.options.length > 1 || | ||
| Boolean(this.options[0]?.name); | ||
|
|
||
| for (const throttler of this.options) { | ||
| const name = throttler.name ?? DEFAULT_THROTTLER_NAME; | ||
| const override = overrides?.[name]; | ||
| const limit = override?.limit ?? throttler.limit; | ||
| const ttl = override?.ttl ?? throttler.ttl; | ||
|
|
||
| const key = this.generateKey(context, name); | ||
| const { totalHits, timeToExpire } = await this.storage.increment( | ||
| key, | ||
| ttl, | ||
| ); | ||
|
|
||
| const suffix = useSuffix && throttler.name ? `-${throttler.name}` : ''; | ||
| const resetSeconds = Math.ceil(timeToExpire / 1000); | ||
| context.header(`X-RateLimit-Limit${suffix}`, String(limit)); | ||
| context.header( | ||
| `X-RateLimit-Remaining${suffix}`, | ||
| String(Math.max(0, limit - totalHits)), | ||
| ); | ||
| context.header(`X-RateLimit-Reset${suffix}`, String(resetSeconds)); | ||
|
|
||
| if (totalHits > limit) { | ||
| context.header(`Retry-After${suffix}`, String(resetSeconds)); | ||
| throw new ThrottlerException(); | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Fix header suffix logic to prevent collisions in multi-throttler setups.
When multiple throttlers are configured and some lack explicit names, the current suffix logic on line 76 evaluates throttler.name (which is undefined for unnamed throttlers) instead of the name variable (which includes the DEFAULT_THROTTLER_NAME fallback from line 65). This causes multiple unnamed throttlers to set identical header names (X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset) without differentiation, leading to header overwrites and breaking multi-throttler observability.
The same issue affects the Retry-After header on line 86.
🔧 Proposed fix
- const suffix = useSuffix && throttler.name ? `-${throttler.name}` : '';
+ const suffix = useSuffix ? `-${name}` : '';This ensures that unnamed throttlers receive the -default suffix when useSuffix is true, preventing header collisions.
📝 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.
Suggested change
| async canActivate(context: ExecutionContext): Promise<boolean> { | |
| const handler = context.getHandler(); | |
| const classRef = context.getClass(); | |
| if (this.shouldSkip(handler) || this.shouldSkip(classRef)) { | |
| return true; | |
| } | |
| const overrides = this.getOverrides(handler, classRef); | |
| // When several throttlers are configured (or a single named one), the | |
| // rate-limit headers are suffixed with the throttler name, matching | |
| // the behavior of `@nestjs/throttler`. | |
| const useSuffix = this.options.length > 1 || | |
| Boolean(this.options[0]?.name); | |
| for (const throttler of this.options) { | |
| const name = throttler.name ?? DEFAULT_THROTTLER_NAME; | |
| const override = overrides?.[name]; | |
| const limit = override?.limit ?? throttler.limit; | |
| const ttl = override?.ttl ?? throttler.ttl; | |
| const key = this.generateKey(context, name); | |
| const { totalHits, timeToExpire } = await this.storage.increment( | |
| key, | |
| ttl, | |
| ); | |
| const suffix = useSuffix && throttler.name ? `-${throttler.name}` : ''; | |
| const resetSeconds = Math.ceil(timeToExpire / 1000); | |
| context.header(`X-RateLimit-Limit${suffix}`, String(limit)); | |
| context.header( | |
| `X-RateLimit-Remaining${suffix}`, | |
| String(Math.max(0, limit - totalHits)), | |
| ); | |
| context.header(`X-RateLimit-Reset${suffix}`, String(resetSeconds)); | |
| if (totalHits > limit) { | |
| context.header(`Retry-After${suffix}`, String(resetSeconds)); | |
| throw new ThrottlerException(); | |
| } | |
| } | |
| return true; | |
| } | |
| async canActivate(context: ExecutionContext): Promise<boolean> { | |
| const handler = context.getHandler(); | |
| const classRef = context.getClass(); | |
| if (this.shouldSkip(handler) || this.shouldSkip(classRef)) { | |
| return true; | |
| } | |
| const overrides = this.getOverrides(handler, classRef); | |
| // When several throttlers are configured (or a single named one), the | |
| // rate-limit headers are suffixed with the throttler name, matching | |
| // the behavior of `@nestjs/throttler`. | |
| const useSuffix = this.options.length > 1 || | |
| Boolean(this.options[0]?.name); | |
| for (const throttler of this.options) { | |
| const name = throttler.name ?? DEFAULT_THROTTLER_NAME; | |
| const override = overrides?.[name]; | |
| const limit = override?.limit ?? throttler.limit; | |
| const ttl = override?.ttl ?? throttler.ttl; | |
| const key = this.generateKey(context, name); | |
| const { totalHits, timeToExpire } = await this.storage.increment( | |
| key, | |
| ttl, | |
| ); | |
| const suffix = useSuffix ? `-${name}` : ''; | |
| const resetSeconds = Math.ceil(timeToExpire / 1000); | |
| context.header(`X-RateLimit-Limit${suffix}`, String(limit)); | |
| context.header( | |
| `X-RateLimit-Remaining${suffix}`, | |
| String(Math.max(0, limit - totalHits)), | |
| ); | |
| context.header(`X-RateLimit-Reset${suffix}`, String(resetSeconds)); | |
| if (totalHits > limit) { | |
| context.header(`Retry-After${suffix}`, String(resetSeconds)); | |
| throw new ThrottlerException(); | |
| } | |
| } | |
| return true; | |
| } |
🤖 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/throttler/guard.ts` around lines 49 - 92, The header suffix logic in
canActivate incorrectly checks throttler.name (which can be undefined) instead
of the resolved name variable (which uses DEFAULT_THROTTLER_NAME), causing
unnamed throttlers to produce identical headers and collide; fix by using the
resolved name when building the suffix (i.e., use name when computing the
suffix/suffix condition for X-RateLimit-* and Retry-After headers) so unnamed
throttlers receive the default-based suffix and headers remain unique in
multi-throttler setups.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Issue Ticket Number
Fixes #123
Type of change
not work as expected)
Checklist:
deno lintANDdeno fmtANDdeno task testand got noerrors.
in CONTRIBUTING.md
Pull Requests for the same
update/change?
Summary by CodeRabbit
New Features
Tests
Documentation
Bug Fixes