Add rate limit to write endpoints#45
Merged
MrImmortal09 merged 9 commits intoApr 12, 2026
Merged
Conversation
Closed
4 tasks
|
@Ewan-Dkhar is attempting to deploy a commit to the mrimmortal09's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Upstash Redis configuration and dependencies, implements a sliding-window rate limiter (Upstash) and a Next.js route-handler wrapper enforcing per-route/IP limits (5 requests per minute) returning 429 with Retry-After when breached. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as withRateLimit
participant Limiter as lib/ratelimit.ts
participant Redis as Upstash\ Redis
Client->>Handler: HTTP request
Handler->>Handler: Extract IP & pathname\nConstruct key (pathname:ip)
Handler->>Limiter: limit(key, {maxRequests:5, window:"1 m"})
alt UPSTASH credentials present
Limiter->>Redis: Ratelimit check (sliding window)
Redis-->>Limiter: { success, reset, remaining }
alt success == true
Limiter-->>Handler: { success: true, ... }
Handler->>Handler: Call original handler
Handler-->>Client: Handler response
else success == false
Limiter->>Redis: INCR breach_count:key
Limiter->>Redis: EXPIRE breach_count:key 3600
Limiter-->>Handler: { success: false, retryAfter }
Handler-->>Client: 429 JSON + Retry-After header
end
else Missing UPSTASH credentials
Limiter-->>Handler: { success: true, limit, remaining } (bypass)
Handler->>Handler: Call original handler
Handler-->>Client: Handler response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Line 9: Add a final newline at end of file so the .env example ends with a
blank line to satisfy dotenv-linter's EndingBlankLine rule; specifically ensure
the file ends with a trailing newline after the UPSTASH_REDIS_REST_TOKEN= line
(the EOF for the .env example).
In `@app/api/health/route.ts`:
- Around line 17-30: Remove or disable the stub write handlers on the health
endpoint: instead of keeping the POST/PUT/DELETE exports that return a 200
success body, either delete the exported handlers (POST, PUT, DELETE) or change
them to return a 405 Method Not Allowed response (using the same withRateLimit
wrapper) so the health route only supports safe read semantics; update the
POST/PUT/DELETE functions that currently call Response.json({ success: true })
to instead return a Response with status 405 and an appropriate Allow header or
remove those exports entirely until real write logic is implemented.
In `@lib/audit-log.ts`:
- Around line 10-26: logToAudit currently only logs to console and the
persistence path is commented out; implement persistence by ensuring a MongoDB
connection (call or re-enable connectDB) and un-comment/use the AuditLog model
to create records (use AuditLog.create with fields severity, event, target,
message, timestamp). Locate the logToAudit function and the commented block that
references connectDB and AuditLog.create, re-enable connection logic and perform
AuditLog.create({ severity: params.severity, event: params.event, target:
params.target, message: params.message, timestamp: new Date() }) and handle
errors in the try/catch so failures are logged and do not crash the process.
In `@lib/ratelimit.ts`:
- Around line 5-8: The code uses non-null assertions when constructing the Redis
client (new Redis) with process.env.UPSTASH_REDIS_REST_URL and
process.env.UPSTASH_REDIS_REST_TOKEN which delays configuration errors to
runtime; instead add an explicit startup validation: check
process.env.UPSTASH_REDIS_REST_URL and process.env.UPSTASH_REDIS_REST_TOKEN and
if either is missing throw a clear Error (or log and exit) before calling new
Redis so the failure is fast and the Redis instance is only created when both
env vars are present.
- Around line 43-49: The current block logs CONSISTENT_RATE_LIMIT_BREACH every
time breachCount >= 3 causing audit floods; change it to escalate only once by
gating the log with a short-lived flag in your rate-store (e.g., an "escalated"
key) or a cooldown check before calling logToAudit: when breachCount reaches the
threshold (breachCount === 3) or when no "escalated" flag exists for the target
key, call logToAudit and then set the "escalated" flag with a TTL (e.g., 1
hour); reference breachCount, logToAudit, CONSISTENT_RATE_LIMIT_BREACH and key
to locate and update the logic so subsequent blocked requests skip logging until
the TTL expires.
In `@lib/with-ratelimit.ts`:
- Around line 12-15: The call to limit(ip, {...}) can throw and currently will
bubble up causing 500s; wrap that invocation in a try/catch inside the
withRateLimit (with-ratelimit) wrapper, catch errors from limit, log the error
(include the error object and context like ip and options), and apply an
explicit fallback policy (e.g., fail-open: set success = true and retryAfter = 0
to allow requests during backend outage, or fail-closed: set success = false and
a conservative retryAfter) so downstream code can continue to make a
deterministic decision; ensure you reference and override the same local
variables used later (success, retryAfter) so the rest of the handler uses the
fallback values.
- Line 9: The client key extraction in the with-rate-limit logic uses a raw
header and a shared "127.0.0.1" fallback which can broaden/poison throttling;
change how const ip is derived by parsing req.headers.get("x-forwarded-for") to
take the first comma-separated value and trim it, validate it (e.g., via
net.isIP) and only use it if valid; if missing/invalid, fall back to
req.headers.get("x-real-ip") or req.socket?.remoteAddress (again validating) and
do not default to a global "127.0.0.1" string—use undefined or a per-request
unique identifier instead so the rate limiter key (ip) remains specific and not
shared.
🪄 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: CHILL
Plan: Pro
Run ID: 4f224a92-a28b-4da7-b008-515dd50d94f2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
.env.exampleapp/api/health/route.tslib/audit-log.tslib/ratelimit.tslib/with-ratelimit.tspackage.json
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 12-15: Reorder and reformat the Mailgun block so dotenv-linter
passes: place MAILGUN_API_KEY, MAILGUN_DOMAIN and optional MAILGUN_URL first,
then EMAIL_FROM last, and remove the surrounding quotes from the EMAIL_FROM
value (use EMAIL_FROM=no-reply@yourdomain.com); ensure MAILGUN_URL remains
commented if unused.
🪄 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: CHILL
Plan: Pro
Run ID: 5ac5090e-a62e-4d86-9419-8704248fcbad
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
.env.examplepackage.json
✅ Files skipped from review due to trivial changes (1)
- package.json
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/ratelimit.ts (1)
5-8:⚠️ Potential issue | 🟠 MajorFail fast when Upstash env vars are missing.
Line 6 and Line 7 still rely on non-null assertions, so misconfiguration is deferred and less clear at runtime. Validate once before constructing
Redis.Suggested fix
+const upstashUrl = process.env.UPSTASH_REDIS_REST_URL; +const upstashToken = process.env.UPSTASH_REDIS_REST_TOKEN; + +if (!upstashUrl || !upstashToken) { + throw new Error('Missing UPSTASH_REDIS_REST_URL or UPSTASH_REDIS_REST_TOKEN'); +} + const redis = new Redis({ - url: process.env.UPSTASH_REDIS_REST_URL!, - token: process.env.UPSTASH_REDIS_REST_TOKEN!, + url: upstashUrl, + token: upstashToken, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ratelimit.ts` around lines 5 - 8, The code currently uses non-null assertions when constructing Redis (const redis = new Redis({...})) which defers misconfiguration; before calling new Redis, read process.env.UPSTASH_REDIS_REST_URL and process.env.UPSTASH_REDIS_REST_TOKEN into local variables, validate they are present (throw new Error or log and process.exit with a clear message mentioning UPSTASH_REDIS_REST_URL/UPSTASH_REDIS_REST_TOKEN), and then pass those validated variables into new Redis instead of using the ! operator.
🧹 Nitpick comments (1)
lib/ratelimit.ts (1)
21-25: CacheRatelimitinstances instead of recreating per request.Constructing a new limiter on every
limit(...)call adds avoidable overhead. A small module-level cache keyed by{maxRequests, window}keeps behavior the same with lower allocation/runtime cost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ratelimit.ts` around lines 21 - 25, The code currently creates a new Ratelimit instance inside the request path (new Ratelimit({...}) using Ratelimit.slidingWindow(...)), which causes unnecessary allocations; instead add a small module-level cache keyed by the limiter configuration (maxRequests and window) and reuse cached Ratelimit instances. Implement a map (e.g. Map<string,Ratelimit>) keyed by `${config.maxRequests}:${config.window}` and change the code that constructs `ratelimit` to first lookup the cache and only call `new Ratelimit({...})` and Ratelimit.slidingWindow(...) when missing; ensure the existing `limit(...)` call and analytics flag remain unchanged so behavior is identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ratelimit.ts`:
- Around line 44-48: The audit call is logging the raw request identifier via
target: key; change the logToAudit invocation in the rate limiter (where
logToAudit is called) to pseudonymize the key first (e.g., compute a stable
one-way hash or HMAC of key with an application secret/salt) and pass that
pseudonymized value as target instead of the raw key; keep the original key only
in volatile memory for enforcement and never write it to audit logs or
persistent stores.
---
Duplicate comments:
In `@lib/ratelimit.ts`:
- Around line 5-8: The code currently uses non-null assertions when constructing
Redis (const redis = new Redis({...})) which defers misconfiguration; before
calling new Redis, read process.env.UPSTASH_REDIS_REST_URL and
process.env.UPSTASH_REDIS_REST_TOKEN into local variables, validate they are
present (throw new Error or log and process.exit with a clear message mentioning
UPSTASH_REDIS_REST_URL/UPSTASH_REDIS_REST_TOKEN), and then pass those validated
variables into new Redis instead of using the ! operator.
---
Nitpick comments:
In `@lib/ratelimit.ts`:
- Around line 21-25: The code currently creates a new Ratelimit instance inside
the request path (new Ratelimit({...}) using Ratelimit.slidingWindow(...)),
which causes unnecessary allocations; instead add a small module-level cache
keyed by the limiter configuration (maxRequests and window) and reuse cached
Ratelimit instances. Implement a map (e.g. Map<string,Ratelimit>) keyed by
`${config.maxRequests}:${config.window}` and change the code that constructs
`ratelimit` to first lookup the cache and only call `new Ratelimit({...})` and
Ratelimit.slidingWindow(...) when missing; ensure the existing `limit(...)` call
and analytics flag remain unchanged so behavior is identical.
🪄 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: CHILL
Plan: Pro
Run ID: bbacc699-eece-447a-88ad-92d8ba29ea2b
📒 Files selected for processing (2)
lib/ratelimit.tslib/with-ratelimit.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/with-ratelimit.ts
Contributor
Author
|
@MrImmortal09 please review this pr |
whilstsomebody
requested changes
Apr 12, 2026
whilstsomebody
left a comment
whilstsomebody
left a comment
Collaborator
There was a problem hiding this comment.
Looks like a good PR by a beginner but still needs a lot of improvements.
Fix the posted comment and tag me again for further reviews.
Collaborator
|
@whilstsomebody should this be consider a hard issue? |
Collaborator
Yes, it's a hard issue. We are not going to merge this until a better rate limiting is reached. |
Contributor
Author
|
@whilstsomebody please review my changes and do give me feedback on how to improve it more if needed |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/ratelimit.ts (1)
56-63:⚠️ Potential issue | 🟠 MajorRaw IP address logged in audit - PII concern persists.
The
keycontains the client IP (format:${pathname}:${ip}fromwith-ratelimit.ts). Logging raw identifiers exposes PII in logs/audit records. Consider hashing or pseudonymizing the key before logging.Suggested fix
+import { createHash } from 'crypto'; + +function fingerprintKey(key: string): string { + return createHash('sha256').update(key).digest('hex').slice(0, 16); +} + // In the logging block: if (breachCount === 3) { console.log({ severity: 'WARNING', event: 'CONSISTENT_RATE_LIMIT_BREACH', - target: key, - message: `Key breached rate limits ${breachCount} times within the hour.`, + target: fingerprintKey(key), + message: `Rate-limit key fingerprint breached limits ${breachCount} times within the hour.`, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ratelimit.ts` around lines 56 - 63, The audit log currently emits the raw `key` (which contains client IP) when `breachCount === 3`, exposing PII; update the logging in the block that checks `breachCount` in ratelimit.ts to pseudonymize the identifier (e.g., compute a stable hash or HMAC of `key` using a server-side secret) and log that hashed value instead of `key` (keep the event name `CONSISTENT_RATE_LIMIT_BREACH`, include `targetHash` or similar), ensuring you reference the same symbols (`breachCount`, `key`) and do not change the external log structure beyond replacing the raw identifier.
🧹 Nitpick comments (1)
lib/ratelimit.ts (1)
49-54: Sliding TTL for breach count may be intentional but worth noting.
pipeline.expire(breachKey, 3600)resets the TTL on every breach, meaning persistent abusers never have their breach count reset to zero. If a fixed 1-hour window from first breach is desired, useNXflag orEXPIREAT:pipeline.expire(breachKey, 3600, 'NX'); // Only set if no TTL existsCurrent behavior may be acceptable if you want to track cumulative abuse.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ratelimit.ts` around lines 49 - 54, The current pipeline that increments breachKey and then sets expire on breachKey (see pipeline, breachKey, pipeline.expire, results, breachCount) resets the TTL on every breach which keeps the window sliding; if you want a fixed 1-hour window from the first breach change the expire call to only set TTL when none exists (use the NX option) or set an absolute expiry via EXPIREAT so the TTL is not refreshed on each incr; update the pipeline.expire invocation to use the NX flag or switch to EXPIREAT to enforce a fixed window instead of the current sliding behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ratelimit.ts`:
- Around line 73-82: The catch block in the rate limiter (the catch around the
Redis call that currently logs in console.error and returns success: true)
implements a fail-open behavior that can disable rate limiting under Redis
failures; change it to a fail-closed conservative response by returning success:
false (or otherwise treat the request as limited) and set remaining: 0,
retryAfter: a conservative backoff/reset interval, and keep a clear error log
via the same logger used elsewhere; update the catch in the function handling
rate checks (the block that currently returns { success: true, limit:
config.maxRequests, remaining: config.maxRequests, reset: 0, retryAfter: 0 }) to
return the conservative values and/or trigger the circuit-breaker path, and if
you intentionally want fail-open instead, add an explicit comment above the
catch explaining the availability/security trade-off and why success: true is
used.
---
Duplicate comments:
In `@lib/ratelimit.ts`:
- Around line 56-63: The audit log currently emits the raw `key` (which contains
client IP) when `breachCount === 3`, exposing PII; update the logging in the
block that checks `breachCount` in ratelimit.ts to pseudonymize the identifier
(e.g., compute a stable hash or HMAC of `key` using a server-side secret) and
log that hashed value instead of `key` (keep the event name
`CONSISTENT_RATE_LIMIT_BREACH`, include `targetHash` or similar), ensuring you
reference the same symbols (`breachCount`, `key`) and do not change the external
log structure beyond replacing the raw identifier.
---
Nitpick comments:
In `@lib/ratelimit.ts`:
- Around line 49-54: The current pipeline that increments breachKey and then
sets expire on breachKey (see pipeline, breachKey, pipeline.expire, results,
breachCount) resets the TTL on every breach which keeps the window sliding; if
you want a fixed 1-hour window from the first breach change the expire call to
only set TTL when none exists (use the NX option) or set an absolute expiry via
EXPIREAT so the TTL is not refreshed on each incr; update the pipeline.expire
invocation to use the NX flag or switch to EXPIREAT to enforce a fixed window
instead of the current sliding 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d89ca6b-2457-409b-840b-06362cf9e2d3
📒 Files selected for processing (2)
lib/ratelimit.tslib/with-ratelimit.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/with-ratelimit.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ratelimit.ts`:
- Around line 22-25: The current fail-open behavior in the rate limiter silently
bypasses limits when redisClient is falsy; update the logic in the rate limit
module (the redisClient check in lib/ratelimit.ts and any enclosing function
that returns the object with success/limit/remaining/reset/retryAfter) to avoid
silently disabling rate limiting in production: either throw an error or emit a
structured/critical log and fail startup when UPSTASH_REDIS_* credentials are
missing, or make the behavior configurable via an env flag (e.g.,
FAIL_OPEN_RATE_LIMIT=true) so production defaults to fail-closed; replace the
console.warn with a processLogger.error/throw and ensure any callers handle the
thrown error or environment flag accordingly.
- Around line 56-63: Replace the console.log call in the rate-limit breach
branch with a call to logToAudit using the same payload (severity: 'WARNING',
event: 'CONSISTENT_RATE_LIMIT_BREACH', target: key, message: ...); update the
call site in the function that checks breachCount (the block where breachCount
=== 3 references key) and ensure logToAudit is imported from the audit logging
module (add the import for logToAudit at the top of lib/ratelimit.ts if missing)
so the event is persisted to the AuditLog system instead of printed to stdout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Collaborator
|
@coderabbitai full-review |
|
✅ Actions performedFull review triggered. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
.env.example (1)
11-15:⚠️ Potential issue | 🟡 MinorFix the Mailgun block so dotenv-linter stays clean.
EMAIL_FROMis still quoted and still ordered after the Mailgun keys, so the sameQuoteCharacter/UnorderedKeywarnings remain in this block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example around lines 11 - 15, The Mailgun env block violates dotenv-linter rules: remove quotes from EMAIL_FROM and reorder keys so EMAIL_FROM is placed before the Mailgun-specific keys; specifically ensure EMAIL_FROM is unquoted (EMAIL_FROM=no-reply@yourdomain.com) and that MAILGUN_API_KEY, MAILGUN_DOMAIN, and optional MAILGUN_URL follow the correct ordering (MAILGUN_API_KEY, MAILGUN_DOMAIN, MAILGUN_URL) to clear the QuoteCharacter and UnorderedKey warnings for the symbols EMAIL_FROM, MAILGUN_API_KEY, MAILGUN_DOMAIN, and MAILGUN_URL.lib/ratelimit.ts (2)
21-25:⚠️ Potential issue | 🟠 MajorDon’t silently disable rate limiting when Upstash credentials are missing.
This branch returns
success: true, so every protected write endpoint becomes unlimited whenever the env vars are unset or misconfigured. For this feature, use an in-memory fallback or fail fast in production instead of bypassing enforcement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ratelimit.ts` around lines 21 - 25, The current limit function silently bypasses enforcement when redisClient is missing; update limit(key: string, config: RateLimitConfig) to: (a) detect missing redisClient and, depending on environment, either use an in-memory fallback rate limiter (e.g., a process-scoped Map with timestamps and counters keyed by the same key logic) to compute and return { success, limit, remaining, reset, retryAfter } or (b) in production fail fast by throwing an error so endpoints do not become unlimited; ensure the in-memory fallback respects config.maxRequests and config.windowMs and uses the same response shape, and keep the redisClient path unchanged so callers of limit() (and the RateLimitConfig type) behave consistently.
56-62:⚠️ Potential issue | 🟠 MajorPersist breach events to
AuditLogand avoid raw identifiers.
console.loghere does not meet the acceptance criterion to log consistent breaches toAuditLog, andtarget: keywrites the route/IP identifier verbatim. CalllogToAudit(...)instead and pass a pseudonymized fingerprint rather than the raw key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ratelimit.ts` around lines 56 - 62, Replace the console.log call that fires when breachCount === 3 with a call to logToAudit(...) recording the CONSISTENT_RATE_LIMIT_BREACH event to the AuditLog; do not pass the raw key—compute a pseudonymized fingerprint for the key (e.g., via existing createFingerprint or pseudonymizeIdentifier helper, or add a short SHA256/HMAC wrapper) and pass that fingerprint as the target, include breachCount in the payload, and ensure the event name matches "CONSISTENT_RATE_LIMIT_BREACH" so AuditLog ingestion can correlate it.lib/with-ratelimit.ts (1)
27-29:⚠️ Potential issue | 🟠 MajorNormalize the client key before rate limiting.
x-forwarded-forcan be a comma-separated chain, and the"127.0.0.1"fallback collapses unrelated callers into one bucket. Parse and validate the first forwarded IP, then avoid a shared fallback key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/with-ratelimit.ts` around lines 27 - 29, The current rate-limit key creation in with-ratelimit.ts uses raw headers and a hardcoded "127.0.0.1" fallback causing shared buckets; update the logic that builds ip and identifier so it parses x-forwarded-for as a comma-separated list and uses the first non-empty entry, validate it (e.g., simple IPv4/IPv6 format check), and only if validation fails use a per-request unique fallback (e.g., req.headers.get("x-real-ip") or the request's remote address) rather than the static "127.0.0.1"; change the variables ip and identifier to reflect this normalized/validated client key so identifier = `${req.nextUrl.pathname}:${normalizedIp}`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/with-ratelimit.ts`:
- Around line 23-45: The withRateLimit wrapper is implemented (function
withRateLimit and its use of limit and NextResponse.json) but the write-flow API
endpoints (e.g., sign-up, contact submission, job posting, avatar upload)
referenced by Issue `#29` are missing; create API route handlers for each form
action (register, contact, job posting, avatar upload) that accept NextRequest,
implement the necessary validation and persistence logic, and wrap each handler
with withRateLimit(...) so the limiter is enforced; ensure each handler returns
proper NextResponse JSON errors and includes Retry-After headers on 429 by using
the same pattern as in withRateLimit when limit returns success=false.
---
Duplicate comments:
In @.env.example:
- Around line 11-15: The Mailgun env block violates dotenv-linter rules: remove
quotes from EMAIL_FROM and reorder keys so EMAIL_FROM is placed before the
Mailgun-specific keys; specifically ensure EMAIL_FROM is unquoted
(EMAIL_FROM=no-reply@yourdomain.com) and that MAILGUN_API_KEY, MAILGUN_DOMAIN,
and optional MAILGUN_URL follow the correct ordering (MAILGUN_API_KEY,
MAILGUN_DOMAIN, MAILGUN_URL) to clear the QuoteCharacter and UnorderedKey
warnings for the symbols EMAIL_FROM, MAILGUN_API_KEY, MAILGUN_DOMAIN, and
MAILGUN_URL.
In `@lib/ratelimit.ts`:
- Around line 21-25: The current limit function silently bypasses enforcement
when redisClient is missing; update limit(key: string, config: RateLimitConfig)
to: (a) detect missing redisClient and, depending on environment, either use an
in-memory fallback rate limiter (e.g., a process-scoped Map with timestamps and
counters keyed by the same key logic) to compute and return { success, limit,
remaining, reset, retryAfter } or (b) in production fail fast by throwing an
error so endpoints do not become unlimited; ensure the in-memory fallback
respects config.maxRequests and config.windowMs and uses the same response
shape, and keep the redisClient path unchanged so callers of limit() (and the
RateLimitConfig type) behave consistently.
- Around line 56-62: Replace the console.log call that fires when breachCount
=== 3 with a call to logToAudit(...) recording the CONSISTENT_RATE_LIMIT_BREACH
event to the AuditLog; do not pass the raw key—compute a pseudonymized
fingerprint for the key (e.g., via existing createFingerprint or
pseudonymizeIdentifier helper, or add a short SHA256/HMAC wrapper) and pass that
fingerprint as the target, include breachCount in the payload, and ensure the
event name matches "CONSISTENT_RATE_LIMIT_BREACH" so AuditLog ingestion can
correlate it.
In `@lib/with-ratelimit.ts`:
- Around line 27-29: The current rate-limit key creation in with-ratelimit.ts
uses raw headers and a hardcoded "127.0.0.1" fallback causing shared buckets;
update the logic that builds ip and identifier so it parses x-forwarded-for as a
comma-separated list and uses the first non-empty entry, validate it (e.g.,
simple IPv4/IPv6 format check), and only if validation fails use a per-request
unique fallback (e.g., req.headers.get("x-real-ip") or the request's remote
address) rather than the static "127.0.0.1"; change the variables ip and
identifier to reflect this normalized/validated client key so identifier =
`${req.nextUrl.pathname}:${normalizedIp}`.
🪄 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: CHILL
Plan: Pro
Run ID: 30c4f685-f47f-420c-8b85-2d851daa35e6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
.env.examplelib/ratelimit.tslib/with-ratelimit.tspackage.json
13 tasks
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
3 participants
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
Added rate limit for write endpoints. This is important to implement as some people can abuse the api and call the api thousands of times. So inorder for that to not happen, the use of rate limiters is crucial.
Closes #29
Type of Change
Checklist
feat/,fix/,docs/, etc.)pnpm lintand there are no errorspnpm dev)Screenshots / Screen Recording
Summary by CodeRabbit
New Features
Chores