This repository was archived by the owner on May 12, 2026. It is now read-only.
Feat(auth): implement rate limiting for auth endpoints#10
Open
Nuel-09 wants to merge 14 commits into
Open
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Contributor
📝 WalkthroughWalkthroughAdds a Redis-backed NestJS RateLimitGuard that enforces per-IP global limits and per-email sensitive limits for auth endpoints, new RedisService ChangesRate Limiting Guard Feature
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
Pull request overview
Adds a new global NestJS guard intended to rate-limit authentication endpoints, with Redis support when available and an in-memory fallback, plus accompanying unit tests and environment documentation updates.
Changes:
- Introduces
RateLimitGuardto enforce global per-IP limits and stricter limits for selected auth endpoints. - Registers
RateLimitGuardas a globalAPP_GUARDinAppModule. - Adds unit tests for the guard and documents new rate-limit environment variables in
.env.example.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
src/guards/rate-limit.guard.ts |
Implements the rate-limiting guard (Redis-backed with in-memory fallback) and sets rate-limit response headers. |
src/guards/rate-limit.guard.spec.ts |
Adds unit tests for in-memory fallback behavior and header setting. |
src/app.module.ts |
Registers the new guard globally via APP_GUARD. |
.env.example |
Documents rate-limit configuration variables and trusted proxy setting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+101
to
+103
| const path: string = req.path || req.url || ''; | ||
| if (!path.startsWith('/auth')) return true; | ||
|
|
Comment on lines
+125
to
+127
| const isSensitive = | ||
| req.method === 'POST' && ['/auth/login', '/auth/register', '/auth/forgot-password'].includes(path); | ||
| if (isSensitive) { |
Comment on lines
+23
to
+41
| private async ensureRedis(): Promise<void> { | ||
| if (this.redisClient) return; | ||
| try { | ||
| const candidates = ['REDIS', 'REDIS_CLIENT', 'RedisService', 'IoredisClient']; | ||
| for (const token of candidates) { | ||
| try { | ||
| // @ts-ignore | ||
| const client = this.moduleRef.get(token, { strict: false }); | ||
| if (client) { | ||
| this.redisClient = client as RedisLike; | ||
| this.logger.log(`RateLimitGuard: using Redis provider "${token}"`); | ||
| return; | ||
| } | ||
| } catch (err) { | ||
| // ignore and try next | ||
| } | ||
| } | ||
| this.logger.warn('RateLimitGuard: no Redis provider found; using in-memory fallback (non-persistent)'); | ||
| } catch (err) { |
Comment on lines
+81
to
+90
| const trusted = (process.env.TRUSTED_PROXIES || '') | ||
| .split(',') | ||
| .map(s => s.trim()) | ||
| .filter(Boolean); | ||
| const forwarded = req.headers?.['x-forwarded-for']; | ||
| const remote = req.ip || req.connection?.remoteAddress || req.socket?.remoteAddress || ''; | ||
| if (forwarded && trusted.length > 0) { | ||
| const forwardedIp = String(forwarded).split(',')[0].trim(); | ||
| return forwardedIp || remote; | ||
| } |
Comment on lines
+67
to
+77
| const now = Date.now(); | ||
| const cur = this.inMemory.get(key); | ||
| if (!cur || cur.reset <= now) { | ||
| const reset = now + windowSec * 1000; | ||
| this.inMemory.set(key, { count: 1, reset }); | ||
| return { count: 1, ttl: windowSec, usingRedis: false }; | ||
| } | ||
| cur.count += 1; | ||
| this.inMemory.set(key, cur); | ||
| const ttl = Math.max(0, Math.ceil((cur.reset - now) / 1000)); | ||
| return { count: cur.count, ttl, usingRedis: false }; |
Comment on lines
+61
to
+64
| } catch (err) { | ||
| this.logger.error('RateLimitGuard: Redis error, falling back to in-memory (fail-open)', err as any); | ||
| this.redisClient = null; | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 @.env.example:
- Line 39: The TRUSTED_PROXIES entry in .env.example is malformed (contains free
text) which breaks parsing and RateLimitGuard.getIpFromRequest(); replace the
line with a proper env assignment like TRUSTED_PROXIES= (comma-separated IPs)
and move any descriptive text into a comment (prefix with #) above or beside it,
ensuring the variable exists as a valid KEY=VALUE so
RateLimitGuard.getIpFromRequest() can read and parse it correctly.
In `@src/guards/rate-limit.guard.spec.ts`:
- Around line 1-10: The eslint false positives come from missing Jest
environment settings causing undefined `require`/`test` flags in this spec;
update the linting context by either adding `"jest": true` to the ESLint env
config or add a top-of-file env directive (e.g., `/* eslint-env jest */`) so the
test file that imports RateLimitGuard via the require call (and uses Jest
globals like test) no longer triggers lint errors.
- Around line 31-42: The test in rate-limit.guard.spec.ts assumes
RATE_LIMIT_GLOBAL=2 but relies on external env; make the test self-contained by
setting the needed env vars in the test setup (e.g., in beforeEach) before
creating the guard and context: set process.env.RATE_LIMIT_GLOBAL='2' and any
related vars like process.env.RATE_LIMIT_SENSITIVE='1' so the guard instance
(created in the same setup where makeContext and guard are initialized) observes
those values; ensure you restore or clear any modified env after the test to
avoid leakage.
In `@src/guards/rate-limit.guard.ts`:
- Around line 54-57: The current rate-limit logic in rate-limit.guard.ts uses
redisClient.incr(key) then sets expire when count === 1, which can race; change
it to check TTL after increment and only set expiry if the key has no TTL: call
redisClient.incr(key) to get count, then call redisClient.ttl(key) and if ttl
=== -1 call redisClient.expire(key, windowSec). Update references in the guard
where redisClient.incr and redisClient.expire are used to use this sequence to
avoid resetting TTL on concurrent requests.
- Around line 61-64: The catch block in RateLimitGuard that sets
this.redisClient = null permanently disables Redis after any error; instead
implement a transient-failure strategy: do not null out redisClient immediately
in the catch inside the method handling Redis ops (refer to the catch where
this.redisClient is set to null), add lightweight retry/backoff logic (e.g.,
attempt N quick retries of the Redis operation using the same redisClient), and
if retries fail schedule a delayed health-check/reconnect (setTimeout) to
re-validate and restore redisClient rather than permanently clearing it;
alternatively detect error types (auth vs network) and only disable on permanent
authentication errors while treating network timeouts as transient and retryable
in the method(s) that call redisClient.
- Around line 128-130: The current extraction of email calls .toString() on
unvalidated fields and can produce unhelpful keys; change the logic that sets
email (used by sensitiveKeyBase) to only normalize string or numeric primitives:
check typeof body.email/username/identifier === 'string' (or === 'number' if you
want numeric IDs), then convert to string, trim and toLowerCase; otherwise treat
as empty string so sensitiveKeyBase falls back to the IP path. Update the code
that defines email and sensitiveKeyBase (the variables named email and
sensitiveKeyBase) accordingly.
- Around line 10-13: The environment parsing can produce NaN for
DEFAULT_GLOBAL_LIMIT, DEFAULT_WINDOW_SEC, DEFAULT_SENSITIVE_LIMIT, and
SENSITIVE_WINDOW_SEC; replace direct Number(...) calls with a safe
parse-and-validate approach (e.g., a small helper like
parsePositiveIntOrDefault) and use it to assign those constants so they fall
back to sensible numeric defaults when env values are missing or invalid; update
usages such as the global.count > DEFAULT_GLOBAL_LIMIT and any window
comparisons to rely on the validated constants.
- Around line 67-77: The in-memory rate limiter Map (inMemory) leaks because
expired entries are never removed; update the logic in the block around key,
cur, reset and windowSec so that when cur is missing or cur.reset <= now you
explicitly delete the old entry (this.inMemory.delete(key)) or overwrite and
then ensure expired entries are pruned, and add a periodic cleanup pass (e.g., a
setInterval in the guard constructor) that iterates this.inMemory and deletes
entries whose reset <= Date.now(); alternatively replace inMemory with a
time-aware cache library (e.g., node-cache) and use its TTL eviction.
- Around line 80-92: The getIpFromRequest function trusts x-forwarded-for
whenever TRUSTED_PROXIES is non-empty, allowing header spoofing; update
getIpFromRequest to validate that the immediate connection IP (req.ip or
req.connection?.remoteAddress or req.socket?.remoteAddress) is one of the parsed
TRUSTED_PROXIES before honoring req.headers['x-forwarded-for'], falling back to
the remote IP otherwise; parse TRUSTED_PROXIES into canonical IPs/CIDRs and
compare against the immediate remote address (use existing variables trusted,
forwarded, remote) and consider using a CIDR check helper or a library if CIDR
support is needed.
- Around line 29-30: Remove the `// `@ts-ignore`` and use proper typing for
ModuleRef.get: declare or import a concrete interface/type for the expected
client (e.g., RateLimitClient), then call
this.moduleRef.get<RateLimitClient>(token as string, { strict: false }) (or cast
token to the appropriate InjectionToken<string>), and handle the possibly
undefined return (e.g., guard against null/undefined before use); reference:
moduleRef.get and the token variable and create/consume a RateLimitClient type.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b303258e-1cb7-4dc8-90ca-7d931506c2e5
📒 Files selected for processing (4)
.env.examplesrc/app.module.tssrc/guards/rate-limit.guard.spec.tssrc/guards/rate-limit.guard.ts
added 4 commits
May 9, 2026 14:44
Remove the in-memory fallback. RateLimitGuard injects RedisService. RedisService gains ttl and expire. Return 503 when Redis operations fail. Adjust unit tests and RedisService specs.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/guards/rate-limit.guard.ts (3)
52-64:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift
x-forwarded-foris trusted without verifying the immediate connection peer.When
TRUSTED_PROXIESis non-empty, any client can spoofx-forwarded-forto bypass IP-based rate limiting because the immediateremoteaddress is never compared to the trusted set. Validate that the immediate peer is itself inTRUSTED_PROXIESbefore honoring the header (CIDR support is preferable since proxy fleets are often expressed as ranges).🤖 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/guards/rate-limit.guard.ts` around lines 52 - 64, The getIpFromRequest function currently trusts x-forwarded-for whenever TRUSTED_PROXIES is non-empty; change it to first verify that the immediate peer (remote, derived from req.ip / req.connection?.remoteAddress / req.socket?.remoteAddress) is inside the configured TRUSTED_PROXIES set before honoring the forwarded header. Update getIpFromRequest to parse TRUSTED_PROXIES into a list of CIDR/networks (or use an IP utility like ipaddr.js or ip-cidr) and perform CIDR/mask matching against remote; only if remote matches a trusted entry should you take the first IP from forwarded, otherwise ignore forwarded and return remote. Ensure you still fall back to remote when no forwarded header is present or when remote is not trusted.
105-107:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCoerce non-string
username/identifiersafely before keying.Calling
.toString()on an object/array silently produces values like"[object Object]"or"1,2,3", which create stable but meaningless rate-limit keys (a single bucket shared across many bad payloads, allowing bypass of the per-email cap). Restrict to actual string inputs.🛡️ Proposed fix
- const body = req.body || {}; - const email = (body.email || body.username || body.identifier || '').toString().toLowerCase().trim(); + const body = req.body || {}; + const raw = body.email ?? body.username ?? body.identifier; + const email = typeof raw === 'string' ? raw.toLowerCase().trim() : '';🤖 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/guards/rate-limit.guard.ts` around lines 105 - 107, The current keying uses (body.email || body.username || body.identifier || '').toString(), which will convert objects/arrays into meaningless keys; change the logic in the rate-limit guard to only accept actual string inputs: inspect body.email, body.username, body.identifier and pick the first value whose typeof is 'string' (optionally allow numbers by String(value) if desired), trim and toLowerCase that string into the variable email, otherwise set email = ''. Update the sensitiveKeyBase computation (sensitiveKeyBase = email ? `ratelimit:email:${email}` : `ratelimit:ip:${ip}`) to rely on this cleaned string; reference variables: body, email and sensitiveKeyBase in your change.
34-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRace / TTL-loss between
incrandexpire.If the process is interrupted (or
expirereturnsfalse) afterincrcreates the key, the counter persists without a TTL and subsequent requests for that key are permanently locked at the over-limit value until manual cleanup. Check TTL afterincrand set the expiry only when no TTL exists — this is also resilient to concurrent first-incr races.🔒 Proposed fix
private async incrementKey(key: string, windowSec: number): Promise<{ count: number; ttl: number }> { const count = await this.redis.incr(key); if (count === null) { this.logger.error(`RateLimitGuard: Redis INCR failed for key ${key}`); throw new HttpException('Rate limiting temporarily unavailable', HttpStatus.SERVICE_UNAVAILABLE); } - if (count === 1) { - const ok = await this.redis.expire(key, windowSec); - if (!ok) { - this.logger.error(`RateLimitGuard: Redis EXPIRE failed for key ${key}`); - throw new HttpException('Rate limiting temporarily unavailable', HttpStatus.SERVICE_UNAVAILABLE); - } - } - const rawTtl = await this.redis.ttl(key); - const ttl = typeof rawTtl === 'number' && rawTtl >= 0 ? rawTtl : windowSec; + let rawTtl = await this.redis.ttl(key); + if (rawTtl === -1) { + const ok = await this.redis.expire(key, windowSec); + if (!ok) { + this.logger.error(`RateLimitGuard: Redis EXPIRE failed for key ${key}`); + throw new HttpException('Rate limiting temporarily unavailable', HttpStatus.SERVICE_UNAVAILABLE); + } + rawTtl = windowSec; + } + const ttl = typeof rawTtl === 'number' && rawTtl >= 0 ? rawTtl : windowSec; return { count, ttl }; }🤖 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/guards/rate-limit.guard.ts` around lines 34 - 50, The incrementKey method can lose TTL if expire fails or due to races; after calling this.redis.incr(key) (inside incrementKey), immediately call this.redis.ttl(key) and only call this.redis.expire(key, windowSec) when ttl indicates "no TTL" (rawTtl === -1), then validate expire succeeded and throw/log like existing behavior if it didn't; compute the returned ttl from the rawTtl when >=0 otherwise use windowSec, and keep the existing null-count and error handling for incr/expire to preserve current exceptions and logs.
🤖 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/guards/rate-limit.guard.ts`:
- Line 74: The current check in the rate limit guard uses
path.startsWith('/auth') which also matches routes like /authenticate; update
the condition in the RateLimitGuard (where path is evaluated) to only treat the
/auth namespace boundary as authentication routes by checking for '/auth'
followed by either a slash or end-of-string (e.g., use a precise string/regex
check such as path === '/auth' or path starts with '/auth/' or a regex like
^/auth(/|$)) so unrelated routes like /authenticate are not considered auth
routes for global rate limiting.
---
Duplicate comments:
In `@src/guards/rate-limit.guard.ts`:
- Around line 52-64: The getIpFromRequest function currently trusts
x-forwarded-for whenever TRUSTED_PROXIES is non-empty; change it to first verify
that the immediate peer (remote, derived from req.ip /
req.connection?.remoteAddress / req.socket?.remoteAddress) is inside the
configured TRUSTED_PROXIES set before honoring the forwarded header. Update
getIpFromRequest to parse TRUSTED_PROXIES into a list of CIDR/networks (or use
an IP utility like ipaddr.js or ip-cidr) and perform CIDR/mask matching against
remote; only if remote matches a trusted entry should you take the first IP from
forwarded, otherwise ignore forwarded and return remote. Ensure you still fall
back to remote when no forwarded header is present or when remote is not
trusted.
- Around line 105-107: The current keying uses (body.email || body.username ||
body.identifier || '').toString(), which will convert objects/arrays into
meaningless keys; change the logic in the rate-limit guard to only accept actual
string inputs: inspect body.email, body.username, body.identifier and pick the
first value whose typeof is 'string' (optionally allow numbers by String(value)
if desired), trim and toLowerCase that string into the variable email, otherwise
set email = ''. Update the sensitiveKeyBase computation (sensitiveKeyBase =
email ? `ratelimit:email:${email}` : `ratelimit:ip:${ip}`) to rely on this
cleaned string; reference variables: body, email and sensitiveKeyBase in your
change.
- Around line 34-50: The incrementKey method can lose TTL if expire fails or due
to races; after calling this.redis.incr(key) (inside incrementKey), immediately
call this.redis.ttl(key) and only call this.redis.expire(key, windowSec) when
ttl indicates "no TTL" (rawTtl === -1), then validate expire succeeded and
throw/log like existing behavior if it didn't; compute the returned ttl from the
rawTtl when >=0 otherwise use windowSec, and keep the existing null-count and
error handling for incr/expire to preserve current exceptions and logs.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bc02cada-b175-4ee4-8a8d-ab83c4d2da91
📒 Files selected for processing (7)
src/app.module.tssrc/guards/rate-limit.guard.spec.tssrc/guards/rate-limit.guard.tssrc/modules/email/email.module.tssrc/modules/email/email.service.tssrc/modules/redis/services/redis.service.tssrc/modules/redis/tests/redis.service.spec.ts
💤 Files with no reviewable changes (1)
- src/modules/email/email.service.ts
Match the auth namespace by path segment so /authenticate is not limited. Prefer originalUrl for pathname so /api/v1/auth routes apply in production. Detect sensitive POST routes by segment after auth. Fix TRUSTED_PROXIES in .env.example for valid dotenv syntax. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep trusted proxies optional in copied envs until operators list proxy IPs. Adds newline at end of file. Co-authored-by: Cursor <cursoragent@cursor.com>
Nuel-09
force-pushed
the
feat/be-011-Rate-limit
branch
from
56d21e9 to
2c75746
Compare
Use a full-line comment so the placeholder is clearly inactive until proxies are set. Co-authored-by: Cursor <cursoragent@cursor.com>
Nuel-09
changed the title
Nuel-09
changed the title
sage-ali
approved these changes
May 10, 2026
ibraheembello
approved these changes
May 10, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app.module.ts (1)
48-55: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider registering
RateLimitGuardbeforeAuthGuardso rate limiting runs first.NestJS executes global guards in registration order, and every guard must return
truefor the request to proceed. With the current ordering,AuthGuardruns first andRateLimitGuardonly runs for paths thatAuthGuardlets through (i.e., routes marked@Public()).In practice this still works for
/auth/login,/auth/register, and/auth/forgot-passwordbecause they must be public, but the ordering has two downsides:
- Brute-force / DDoS traffic still pays the cost of
AuthGuardwork (decorator lookup, optional token decoding) before being shed by the limiter.- If a future auth route is accidentally registered as protected,
AuthGuardwill short-circuit with 401 and rate limiting silently stops applying to that endpoint — exactly the route most likely to need it.Swap the two providers so the limiter is the first gate:
♻️ Proposed reordering
{ provide: 'APP_GUARD', + useClass: RateLimitGuard, + }, + { + provide: 'APP_GUARD', useClass: AuthGuard, }, - { - provide: 'APP_GUARD', - useClass: RateLimitGuard, - },While here, both entries use the string
'APP_GUARD'; import and use theAPP_GUARDconstant from@nestjs/core(alongside the existingAPP_PIPEimport) for refactor safety.🤖 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/app.module.ts` around lines 48 - 55, The global guards are registered in the wrong order; swap the two provider entries so RateLimitGuard is registered before AuthGuard (so RateLimitGuard runs first), and replace the string 'APP_GUARD' with the exported APP_GUARD constant from `@nestjs/core` (alongside the existing APP_PIPE import) to avoid fragile string usage; update the providers array entries referencing useClass: RateLimitGuard and useClass: AuthGuard accordingly.
♻️ Duplicate comments (1)
src/guards/rate-limit.guard.ts (1)
144-146:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate that the email/identifier value is a string before normalizing.
Calling
.toString()onbody.email(orusername/identifier) without a type check still allows non-string payloads to produce surprising keys: a JSON array yields"1,2,3", an object yields"[object Object]", and a number yields a coerced string. All of these become validratelimit:email:*keys, which lets a caller bypass per-email throttling by submitting{"email": ["a@b.c"]}or{"email": {}}and having every request hash to the same shared bucket (or a new one each time).A
typeofguard fixes this with a single line and matches the past review's intent (the previous resolution marker on this line does not appear to have actually changed the code).🛡️ Proposed fix
- const body = req.body || {}; - const email = (body.email || body.username || body.identifier || '').toString().toLowerCase().trim(); + const body = req.body || {}; + const raw = body.email ?? body.username ?? body.identifier; + const email = typeof raw === 'string' ? raw.toLowerCase().trim() : ''; const sensitiveKeyBase = email ? `ratelimit:email:${email}` : `ratelimit:ip:${ip}`;🤖 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/guards/rate-limit.guard.ts` around lines 144 - 146, The code builds sensitiveKeyBase from req.body fields but calls toString() on potentially non-string values, allowing arrays/objects/numbers to create unintended keys; update the extraction for email (the variable assigned from body.email/body.username/body.identifier) to only use the value if typeof value === 'string' (otherwise treat it as empty string), then safely call .toLowerCase().trim(); adjust the same check for username/identifier resolution so sensitiveKeyBase remains `ratelimit:email:${email}` only when a validated string exists, otherwise use `ratelimit:ip:${ip}` as before (refer to the email variable and sensitiveKeyBase).
🤖 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/guards/rate-limit.guard.spec.ts`:
- Around line 57-63: The afterAll block currently restores environment variables
by assigning process.env.* = oldEnv.* which will set the literal string
"undefined" if a variable was originally unset; change the restore logic in the
afterAll block to check each saved value (oldEnv.RATE_LIMIT_GLOBAL,
oldEnv.RATE_LIMIT_WINDOW_SEC, oldEnv.RATE_LIMIT_SENSITIVE,
oldEnv.RATE_LIMIT_SENSITIVE_WINDOW_SEC, oldEnv.TRUSTED_PROXIES) and if the saved
value is undefined use delete process.env[KEY] to remove the env var, otherwise
set process.env[KEY] = savedValue; update the afterAll that references those
keys so it conditionally deletes or restores rather than assigning undefined
strings.
In `@src/guards/rate-limit.guard.ts`:
- Around line 35-51: The implementation of RateLimitGuard.incrementKey currently
throws SERVICE_UNAVAILABLE on Redis failures (fail-closed) but the PR
description claims fail-open with an in-memory fallback; decide which behavior
is intended and align code/docs accordingly — if fail-open is intended, modify
incrementKey (in class RateLimitGuard) to catch Redis errors around
this.redis.incr/expire/ttl and instead use an in-memory fallback counter (e.g.,
a Map storing {count, expiresAt} per key) that increments and sets TTL
atomically in JS, return {count, ttl} from the fallback and avoid throwing
SERVICE_UNAVAILABLE; add unit tests that simulate Redis errors to verify
fallback behavior; if fail-closed is intended, update the PR description and
JSDoc to state fail-closed and add tests for Redis failure returning 503.
In `@src/modules/email/email.module.ts`:
- Line 6: The import uses the package's internal path; replace the current
import of HandlebarsAdapter from
"@nestjs-modules/mailer/dist/adapters/handlebars.adapter" with the public API
path "@nestjs-modules/mailer/adapters/handlebars.adapter". Update the import
statement that references HandlebarsAdapter in email.module.ts to use the
official adapter path so the code relies on the documented public API and avoids
breakage on package updates.
---
Outside diff comments:
In `@src/app.module.ts`:
- Around line 48-55: The global guards are registered in the wrong order; swap
the two provider entries so RateLimitGuard is registered before AuthGuard (so
RateLimitGuard runs first), and replace the string 'APP_GUARD' with the exported
APP_GUARD constant from `@nestjs/core` (alongside the existing APP_PIPE import) to
avoid fragile string usage; update the providers array entries referencing
useClass: RateLimitGuard and useClass: AuthGuard accordingly.
---
Duplicate comments:
In `@src/guards/rate-limit.guard.ts`:
- Around line 144-146: The code builds sensitiveKeyBase from req.body fields but
calls toString() on potentially non-string values, allowing
arrays/objects/numbers to create unintended keys; update the extraction for
email (the variable assigned from body.email/body.username/body.identifier) to
only use the value if typeof value === 'string' (otherwise treat it as empty
string), then safely call .toLowerCase().trim(); adjust the same check for
username/identifier resolution so sensitiveKeyBase remains
`ratelimit:email:${email}` only when a validated string exists, otherwise use
`ratelimit:ip:${ip}` as before (refer to the email variable and
sensitiveKeyBase).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 987995d8-80f0-4224-828e-9db92ffa335f
📒 Files selected for processing (5)
.env.examplesrc/app.module.tssrc/guards/rate-limit.guard.spec.tssrc/guards/rate-limit.guard.tssrc/modules/email/email.module.ts
Comment on lines
+57
to
+63
| afterAll(() => { | ||
| process.env.RATE_LIMIT_GLOBAL = oldEnv.RATE_LIMIT_GLOBAL; | ||
| process.env.RATE_LIMIT_WINDOW_SEC = oldEnv.RATE_LIMIT_WINDOW_SEC; | ||
| process.env.RATE_LIMIT_SENSITIVE = oldEnv.RATE_LIMIT_SENSITIVE; | ||
| process.env.RATE_LIMIT_SENSITIVE_WINDOW_SEC = oldEnv.RATE_LIMIT_SENSITIVE_WINDOW_SEC; | ||
| process.env.TRUSTED_PROXIES = oldEnv.TRUSTED_PROXIES; | ||
| }); |
Contributor
There was a problem hiding this comment.
afterAll may leave process.env polluted with the literal string "undefined".
Assigning process.env.X = undefined in Node coerces the value to the string "undefined" (it does not unset the variable). If any of these keys were not set before the suite ran (likely for RATE_LIMIT_* and TRUSTED_PROXIES in CI), other test files in the same Jest worker will subsequently observe process.env.RATE_LIMIT_GLOBAL === "undefined" instead of an absent variable. The guard then computes Number("undefined") || 100, which masks the leak via fallback but can still cause confusing failures elsewhere.
Use delete when the snapshot was undefined:
🛡️ Proposed fix
afterAll(() => {
- process.env.RATE_LIMIT_GLOBAL = oldEnv.RATE_LIMIT_GLOBAL;
- process.env.RATE_LIMIT_WINDOW_SEC = oldEnv.RATE_LIMIT_WINDOW_SEC;
- process.env.RATE_LIMIT_SENSITIVE = oldEnv.RATE_LIMIT_SENSITIVE;
- process.env.RATE_LIMIT_SENSITIVE_WINDOW_SEC = oldEnv.RATE_LIMIT_SENSITIVE_WINDOW_SEC;
- process.env.TRUSTED_PROXIES = oldEnv.TRUSTED_PROXIES;
+ for (const key of [
+ 'RATE_LIMIT_GLOBAL',
+ 'RATE_LIMIT_WINDOW_SEC',
+ 'RATE_LIMIT_SENSITIVE',
+ 'RATE_LIMIT_SENSITIVE_WINDOW_SEC',
+ 'TRUSTED_PROXIES',
+ ] as const) {
+ if (oldEnv[key] === undefined) delete process.env[key];
+ else process.env[key] = oldEnv[key];
+ }
});📝 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
| afterAll(() => { | |
| process.env.RATE_LIMIT_GLOBAL = oldEnv.RATE_LIMIT_GLOBAL; | |
| process.env.RATE_LIMIT_WINDOW_SEC = oldEnv.RATE_LIMIT_WINDOW_SEC; | |
| process.env.RATE_LIMIT_SENSITIVE = oldEnv.RATE_LIMIT_SENSITIVE; | |
| process.env.RATE_LIMIT_SENSITIVE_WINDOW_SEC = oldEnv.RATE_LIMIT_SENSITIVE_WINDOW_SEC; | |
| process.env.TRUSTED_PROXIES = oldEnv.TRUSTED_PROXIES; | |
| }); | |
| afterAll(() => { | |
| for (const key of [ | |
| 'RATE_LIMIT_GLOBAL', | |
| 'RATE_LIMIT_WINDOW_SEC', | |
| 'RATE_LIMIT_SENSITIVE', | |
| 'RATE_LIMIT_SENSITIVE_WINDOW_SEC', | |
| 'TRUSTED_PROXIES', | |
| ] as const) { | |
| if (oldEnv[key] === undefined) delete process.env[key]; | |
| else process.env[key] = oldEnv[key]; | |
| } | |
| }); |
🤖 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/guards/rate-limit.guard.spec.ts` around lines 57 - 63, The afterAll block
currently restores environment variables by assigning process.env.* = oldEnv.*
which will set the literal string "undefined" if a variable was originally
unset; change the restore logic in the afterAll block to check each saved value
(oldEnv.RATE_LIMIT_GLOBAL, oldEnv.RATE_LIMIT_WINDOW_SEC,
oldEnv.RATE_LIMIT_SENSITIVE, oldEnv.RATE_LIMIT_SENSITIVE_WINDOW_SEC,
oldEnv.TRUSTED_PROXIES) and if the saved value is undefined use delete
process.env[KEY] to remove the env var, otherwise set process.env[KEY] =
savedValue; update the afterAll that references those keys so it conditionally
deletes or restores rather than assigning undefined strings.
Comment on lines
+35
to
+51
| private async incrementKey(key: string, windowSec: number): Promise<{ count: number; ttl: number }> { | ||
| const count = await this.redis.incr(key); | ||
| if (count === null) { | ||
| this.logger.error(`RateLimitGuard: Redis INCR failed for key ${key}`); | ||
| throw new HttpException('Rate limiting temporarily unavailable', HttpStatus.SERVICE_UNAVAILABLE); | ||
| } | ||
| if (count === 1) { | ||
| const ok = await this.redis.expire(key, windowSec); | ||
| if (!ok) { | ||
| this.logger.error(`RateLimitGuard: Redis EXPIRE failed for key ${key}`); | ||
| throw new HttpException('Rate limiting temporarily unavailable', HttpStatus.SERVICE_UNAVAILABLE); | ||
| } | ||
| } | ||
| const rawTtl = await this.redis.ttl(key); | ||
| const ttl = typeof rawTtl === 'number' && rawTtl >= 0 ? rawTtl : windowSec; | ||
| return { count, ttl }; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the full rate-limit.guard.ts file
head -100 src/guards/rate-limit.guard.tsRepository: hngprojects/flowbrand-be
Length of output: 4204
🏁 Script executed:
# Check for any in-memory fallback implementation in the file
wc -l src/guards/rate-limit.guard.tsRepository: hngprojects/flowbrand-be
Length of output: 103
🏁 Script executed:
# Search for in-memory fallback mentions in the codebase
rg -i "in-memory fallback|in memory fallback" --type ts --type jsRepository: hngprojects/flowbrand-be
Length of output: 50
🏁 Script executed:
# Look for PR description or commit messages
git log --oneline -20 | head -20Repository: hngprojects/flowbrand-be
Length of output: 123
🏁 Script executed:
# Check if there are test files for this guard
fd -e spec.ts -e test.ts | grep -i "rate.*limit"Repository: hngprojects/flowbrand-be
Length of output: 104
🏁 Script executed:
# Read the rest of the guard file
tail -n +101 src/guards/rate-limit.guard.tsRepository: hngprojects/flowbrand-be
Length of output: 2941
🏁 Script executed:
# Check the commit message for more context
git show c7a2f45 --quiet --format=fullerRepository: hngprojects/flowbrand-be
Length of output: 368
🏁 Script executed:
# Read the test file to see what behavior is expected
head -150 src/guards/rate-limit.guard.spec.tsRepository: hngprojects/flowbrand-be
Length of output: 4775
🏁 Script executed:
# Check for any mention of fallback in related files or READMEs
rg "fail.*open|fallback" --type ts --type md -A 2 -B 2Repository: hngprojects/flowbrand-be
Length of output: 651
Confirm whether fail-closed behavior is intentional — PR description claims fail-open with in-memory fallback but code documents and implements fail-closed.
The code's JSDoc comment explicitly states the design is fail-closed: "If Redis operations fail, responds with 503 so limits are not silently bypassed." The incrementKey method throws SERVICE_UNAVAILABLE on any Redis operation failure (incr returning null or expire returning false), with no in-memory fallback implemented. However, the PR description references "fail-open behavior when Redis is unavailable (in-memory fallback)," creating a contradiction.
If fail-closed is intentional, the PR description should be updated to reflect that. If fail-open with in-memory fallback is the intended design, the implementation needs to be restored—transient Redis outages will currently return 503 to all /auth/* traffic, blocking login and locking users out of the system.
Tests only mock successful Redis operations and do not verify failure scenarios.
🤖 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/guards/rate-limit.guard.ts` around lines 35 - 51, The implementation of
RateLimitGuard.incrementKey currently throws SERVICE_UNAVAILABLE on Redis
failures (fail-closed) but the PR description claims fail-open with an in-memory
fallback; decide which behavior is intended and align code/docs accordingly — if
fail-open is intended, modify incrementKey (in class RateLimitGuard) to catch
Redis errors around this.redis.incr/expire/ttl and instead use an in-memory
fallback counter (e.g., a Map storing {count, expiresAt} per key) that
increments and sets TTL atomically in JS, return {count, ttl} from the fallback
and avoid throwing SERVICE_UNAVAILABLE; add unit tests that simulate Redis
errors to verify fallback behavior; if fail-closed is intended, update the PR
description and JSDoc to state fail-closed and add tests for Redis failure
returning 503.
Remove duplicate expire block and close catch so RedisService parses correctly. Co-authored-by: Cursor <cursoragent@cursor.com>
…o feat/be-011-rate-limit
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Pull Request
Description
Added a top-level rate limiting guard for authentication routes and wired it into the app bootstrap through the global guard pipeline.
What changed
RateLimitGuardfor/auth/*endpointsPOST /auth/loginPOST /auth/registerPOST /auth/forgot-passwordX-RateLimit-LimitX-RateLimit-RemainingX-RateLimit-ResetRetry-Afteron429 Too Many Requestsapp.module.tsNotes
Related Issue
Fixes #(issue)
Type of Change
How Has This Been Tested?
.envduring test executionAdded and validated unit coverage for the
RateLimitGuardbehavior.What was tested
Global per-IP rate limiting for
/auth/*Stricter limits for sensitive auth endpoints
Correct
429 Too Many Requestsresponse when limits are exceededPresence of rate limit headers:
X-RateLimit-LimitX-RateLimit-RemainingX-RateLimit-ResetRetry-AfterFail-open behavior when Redis is unavailable
[x ] Unit tests
Integration tests
Manual tests
Test Evidence
Screenshots (if applicable)
Documentation Screenshots (if applicable)
Checklist
Additional Notes
Summary by CodeRabbit