feat: add rate limiting on auth and API endpoints#117
Merged
ryota-murakami merged 3 commits intomainfrom Feb 13, 2026
Merged
Conversation
Add sliding-window rate limiting to protect against brute-force and abuse: - OAuth callback: 10 req/min per IP (proxy.ts edge) - signInWithGitHub: 10 req/hour per IP - deleteAccount: 3 req/hour per user - GitHub API proxy: 30 req/min per IP - addRepositoriesToBoard: 10 req/min per user - DnD batch operations: 60 req/min per user - Board CRUD: 20 req/min per user Zero new dependencies — in-memory implementation with same interface as Upstash for easy future swap.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a configurable rate-limiting system and integrates it into auth, server actions, GitHub proxy, and the OAuth middleware; includes in-memory sliding-window and edge-compatible limiters, per-endpoint configs, logging of rate_limited security events, tests, and new auth-wrapper variants enforcing rate limits. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Proxy as Proxy/Middleware
participant RateLimitEdge as Edge Rate Limiter
participant Guard as Auth Guard
participant RateLimitServer as Server Rate Limiter
participant Action as Server Action
participant Security as Security Events
Client->>Proxy: HTTP request (e.g., /auth/callback or action)
Proxy->>RateLimitEdge: edgeRateLimit(key, clientIp)
alt Edge limit exceeded
RateLimitEdge->>Security: log rate_limited
Security-->>Proxy: logged
Proxy-->>Client: 429 + Retry-After
else Edge allowed
Proxy->>Guard: withAuthRateLimit/withAuthResultRateLimit
Guard->>RateLimitServer: checkRateLimit(key, userId or ip)
alt Server limit exceeded
RateLimitServer->>Security: log rate_limited
Security-->>Guard: logged
Guard-->>Client: ActionResult error / throw
else Server allowed
Guard->>Action: execute protected action
Action-->>Guard: result
Guard-->>Client: success response
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 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 |
🤖 Morph Preview Test⚡ Looks like you hit your rate limits! Please upgrade your limits here, or wait a few minutes and try again. If you need help, reach out to support@morphllm.com. Automated testing by Morph |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #117 +/- ##
==========================================
- Coverage 73.74% 72.51% -1.24%
==========================================
Files 138 142 +4
Lines 4125 4202 +77
Branches 1078 1128 +50
==========================================
+ Hits 3042 3047 +5
- Misses 1062 1134 +72
Partials 21 21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
🤖 Fix all issues with AI agents
In `@src/lib/actions/auth.ts`:
- Around line 30-39: The fallback IP "127.0.0.1" used in the sign-in flow can
cause a shared rate-limit bucket when x-forwarded-for is missing; update the
code around headers()/headerStore and the ip variable in the signInWithGitHub
rate-limiting block to detect when x-forwarded-for is absent and emit a
warning/telemetry (e.g., processLogger.warn or Sentry.captureMessage) with
context (headers or request ID) before using a safer per-request fallback (such
as generating a short random token or including another header like
cf-connecting-ip) and then pass that value into
checkRateLimit('signInWithGitHub', ip); ensure the added logging/telemetry call
is non-blocking and does not expose sensitive header contents.
In `@src/lib/rate-limit/check.ts`:
- Around line 76-93: The rate-limit handler currently emits two Sentry events
because logSecurityEvent('rate_limited', ...) already calls
Sentry.captureMessage; remove the explicit Sentry.captureMessage(...) block in
checkRateLimit (the code that builds `Rate limit hit: ${key}` with tags {
category: 'rate_limit', action: key }) so only logSecurityEvent is used; if you
need the extra Sentry tags/metadata, extend logSecurityEvent (or its call site)
to accept and forward an extra tags/extra payload (e.g., include action/key and
identifier) rather than duplicating a direct Sentry.captureMessage call.
🧹 Nitpick comments (5)
src/lib/rate-limit/check.ts (2)
79-82:userIdfield populated with IP addresses is misleading.For IP-based rate limiting (e.g.,
githubApi,auth/callback),identifieris an IP address, but it's logged asuserIdin the security event context. This will produce confusing audit trails in Sentry where IP addresses appear in theuserIdfield.Consider using the
ipfield fromSecurityEventContextwhen the identifier is an IP:Suggested approach
logSecurityEvent('rate_limited', { - userId: identifier, + ip: identifier, path: key, })If you need to distinguish between user-ID-keyed and IP-keyed limiters, you could pass both fields or add a discriminator.
23-28:RateLimitResultcould benefit from a discriminated union for type safety.Currently callers use
rlResult.error!(non-null assertion) after checking!rlResult.allowed. A discriminated union would eliminate the need for!:export type RateLimitResult = | { allowed: true } | { allowed: false; error: string }This is a minor ergonomic improvement — not blocking.
src/lib/actions/github.ts (1)
24-33: SeparategetClientIpfor server actions is reasonable but duplicates logic.The edge-runtime version in
edge-memory.tstakes aRequestparameter, while this one uses Next.jsheaders()— different contexts justify separate implementations. That said, the IP-parsing logic (x-forwarded-forsplit + trim + fallback) is duplicated. If you ever need to change the parsing (e.g., addx-real-ipfallback likeedge-memory.tsdoes), you'll need to update both.Consider extracting the parsing into a shared utility, or at minimum, note the edge version also checks
x-real-ipwhile this one does not — a minor inconsistency.src/lib/rate-limit/edge-memory.ts (1)
14-46: No eviction for stale entries — acceptable for edge, but worth a passive cleanup.Expired entries are lazily overwritten when the same identifier returns (line 35-37), but IPs that visit once and never return will linger. In a short-lived edge worker this is a non-issue. If the function stays warm for extended periods, consider opportunistic eviction — e.g., delete a few expired entries when
store.sizeexceeds a threshold insideedgeRateLimititself (nosetIntervalneeded).src/lib/actions/auth-guard.ts (1)
135-168: Optional: reduce auth boilerplate duplication via composition.
withAuthResultRateLimitandwithAuthRateLimiteach duplicate the 8-line auth block from their non-rate-limited counterparts. You could compose them:♻️ Sketch (optional)
export async function withAuthResultRateLimit<T>( rateLimitKey: RateLimitKey, action: (supabase: SupabaseClient<Database>, user: User) => Promise<T>, ): Promise<ActionResult<T>> { - const supabase = await createClient() - const { - data: { user }, - error: authError, - } = await supabase.auth.getUser() - - if (authError || !user) { - return { success: false, error: 'Authentication required' } - } - - // Rate limit check using user.id - const rlResult = checkRateLimit(rateLimitKey, user.id) - if (!rlResult.allowed) { - return { success: false, error: rlResult.error! } - } - - try { - const data = await action(supabase, user) - return { success: true, data } - } catch (error) { - Sentry.captureException(error, { - extra: { context: `withAuthResultRateLimit:${rateLimitKey}` }, - }) - return { - success: false, - error: - error instanceof Error ? error.message : 'An unexpected error occurred', - } - } + return withAuthResult(async (supabase, user) => { + const rlResult = checkRateLimit(rateLimitKey, user.id) + if (!rlResult.allowed) { + throw new Error(rlResult.error!) + } + return action(supabase, user) + }) }This keeps the rate-limit error surfacing through the existing
withAuthResultcatch block. Up to you whether the indirection is worth the dedup.
🧪 E2E Coverage Report (Sharded: 12 parallel jobs)
📊 Full report available in workflow artifacts |
- Remove duplicate Sentry.captureMessage in check.ts (logSecurityEvent already emits to Sentry internally) - Add warning log when x-forwarded-for header is missing in auth.ts and github.ts to detect proxy misconfiguration in production
🤖 Morph Preview Test⚡ Looks like you hit your rate limits! Please upgrade your limits here, or wait a few minutes and try again. If you need help, reach out to support@morphllm.com. Automated testing by Morph |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lib/rate-limit/check.ts`:
- Around line 62-69: In checkRateLimit, add a guard for a missing/empty
identifier (the identifier parameter) so we don't use an empty string as a
shared rate-limit bucket; if identifier is falsy/empty, return a RateLimitResult
that denies the request (e.g., { allowed: false, reason: 'missing identifier' })
and log or record a warning before doing any bucket lookups; update any tests or
call sites that expect different behavior accordingly.
🧹 Nitpick comments (1)
src/lib/rate-limit/check.ts (1)
77-80:userIdfield holds an IP for unauthenticated endpoints — consider clarity.When
identifieris a client IP (e.g.,signInWithGitHub,auth/callback), logging it asuserIdin Sentry could confuse triage. A field likeidentifieror splitting intouserId/ipwould make dashboards clearer. Low priority — just a nit.
Prevent empty string identifiers from creating a shared rate-limit bucket. Log a warning via module logger when this edge case occurs.
🤖 Morph Preview Test⚡ Looks like you hit your rate limits! Please upgrade your limits here, or wait a few minutes and try again. If you need help, reach out to support@morphllm.com. Automated testing by Morph |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lib/rate-limit/check.ts`:
- Around line 69-73: The guard that bypasses rate-limiting when identifier is
empty currently only calls log.warn and returns { allowed: true }; add a Sentry
security event before returning by invoking the existing logSecurityEvent
function (e.g., logSecurityEvent('empty_rate_limit_identifier', { key })) so the
empty-identifier bypass is recorded like other rate-limit events; place this
call in the same block where identifier and key are available (near the existing
log.warn) prior to returning.
🧹 Nitpick comments (1)
src/lib/rate-limit/check.ts (1)
1-97: Clean implementation with good docs and past issues addressed.The duplicate Sentry call is removed, and the empty-identifier guard is in place. JSDoc coverage is solid.
One minor note: on line 87, the field
userIdreceives theidentifierparam, which may be an IP address for unauthenticated endpoints (e.g.,signInWithGitHub). This can be misleading when reviewing security events. Consider renaming to a neutral field likesubjectoridentifierif theSecurityEventContexttype allows it.
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
2 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.
Summary
Closes #77
Add in-memory sliding-window rate limiting to protect against brute-force enumeration of board IDs, excessive GitHub API consumption, and denial of service via expensive queries.
Zero new dependencies — pure TypeScript in-memory implementation with identical interface to
@upstash/ratelimitfor easy future swap if needed.Rate Limits Applied
/auth/callback(proxy.ts)signInWithGitHub()deleteAccount()addRepositoriesToBoard()Architecture
src/lib/rate-limit/config.ts— Centralized rate limit definitionssrc/lib/rate-limit/memory.ts—SlidingWindowLimiterclass (Map-based sliding window with periodic cleanup)src/lib/rate-limit/check.ts— Main entry point with test-mode bypass and Sentry loggingsrc/lib/rate-limit/edge-memory.ts— Edge-runtime compatible fixed-window limiter forproxy.tssrc/lib/actions/auth-guard.ts— NewwithAuthRateLimitandwithAuthResultRateLimitwrappersError Handling
Retry-After: 60/login?error=rate_limited{ success: false, error: "Too many X requests..." }→ toastDesign Decisions
isTestMode()auto-bypasses all rate limiting in E2E/unit testsTest plan
pnpm typecheck— passespnpm lint— passes (0 warnings)pnpm test— 1282 tests pass (includes newSlidingWindowLimiterunit tests)pnpm build— successfulpnpm e2e— rate limiting correctly bypassed in test mode (276 passed, 46 pre-existing flaky)Summary by CodeRabbit