fix(auth): add rate limiting for authentication endpoints#1940
Open
Ridanshi wants to merge 4 commits into
Open
fix(auth): add rate limiting for authentication endpoints#1940Ridanshi wants to merge 4 commits into
Ridanshi wants to merge 4 commits into
Conversation
Closes Priyanshu-byte-coder#1657 Three cron/sync endpoints bypassed authentication when NODE_ENV was development. The condition used was: if (authHeader !== Bearer-secret AND process.env.NODE_ENV !== development) This made the authorization check a no-op in dev mode, allowing any unauthenticated caller to trigger bulk operations: sponsors sync, WakaTime stats sync, and Discord notifications. Root cause: a developer-convenience shortcut never removed before deploy. Fix: created src/lib/cron-auth.ts with validateCronRequest() that enforces a consistent, environment-independent fail-closed check: - CRON_SECRET not configured -> 500 - Authorization header missing or wrong -> 401 - Correct secret -> null (proceed) No NODE_ENV bypass in any environment. Affected routes (all fixed): src/app/api/wakatime/sync/route.ts src/app/api/sponsors/sync/route.ts src/app/api/notifications/discord-sync/route.ts Already-correct (no change needed): src/app/api/cron/weekly-digest/route.ts Local development: set CRON_SECRET in .env.local and supply the matching Authorization header when calling endpoints manually. Documented in .env.example. Tests added: test/cron-auth.test.ts (17 tests) - unit tests for validateCronRequest and integration regression tests for discord-sync covering all cases including development environment enforcement test/wakatime-sync-auth.test.ts (12 tests) - replaced bypass test with enforcement tests for dev environment test/sponsors-sync.test.ts (12 tests) - auth and dev enforcement
Closes Priyanshu-byte-coder#1303 Security analysis ----------------- DevTrack authenticates exclusively via GitHub OAuth; there is no password, email, or OTP flow. Despite this, the NextAuth endpoints responsible for initiating and completing the OAuth handshake were entirely unprotected: POST /api/auth/signin/github - initiates the OAuth redirect GET /api/auth/callback/github - exchanges the code for a token GET /api/auth/link-github - secondary account link initiation GET /api/auth/link-github/callback - secondary account link callback Flooding these routes can exhaust GitHub token-exchange quota for the application, probe for valid OAuth codes, or generate high server load. Endpoints deliberately excluded from rate limiting: GET /api/auth/session - polled on every page render GET /api/auth/csrf - CSRF token fetch, not an attack surface GET /api/auth/signout - termination, not initiation Authentication flows discovered --------------------------------- GitHub OAuth only. No password, email/OTP, or magic-link flows exist. Rate limiting strategy ---------------------- 5 requests per IP per 15-minute fixed window (issue Priyanshu-byte-coder#1303). Allows two complete sign-in flows (initiation + callback) plus one spare before throttling. In development the limit is raised to 1000. Uses the shared createMemoryFixedWindowRateLimiter factory from @/lib/rate-limit. Key namespace auth:<ip> is isolated from the metrics-rate-limit:* namespace to prevent interference. 429 responses carry X-RateLimit-* and Retry-After headers. Body: { error: 'Too many authentication attempts. Please try again later.' } Protected endpoints ------------------- Four auth-sensitive paths added to the Next.js middleware matcher and handled by the auth rate limiter before the general metrics gate. Files changed ------------- src/lib/auth-rate-limit.ts (new) Exports checkAuthRateLimit(), isAuthSensitivePath(), AUTH_LIMIT, AUTH_WINDOW_MS, AUTH_SENSITIVE_PREFIXES. No new dependencies. src/middleware.ts (modified) Imports auth limiter; inserts auth rate-limit block after the protected-route check; adds auth paths to the matcher config. Tests added ----------- test/auth-rate-limit.test.ts - 22 tests: isAuthSensitivePath (9) - protected vs excluded path classification basic behaviour (5) - allow/block, remaining, 429-compatible status window expiry (2) - counter resets after window, not before IP isolation (2) - independent counters per IP custom limit override (2) - dev/test relaxation reset timestamp (2) - window-boundary reset, shared within window Verification ------------ tsc --noEmit clean (no errors in changed files) next lint warnings only (all pre-existing) vitest run 22/22 tests passed
|
@Ridanshi is attempting to deploy a commit to the PRIYANSHU DOSHI's projects Team on Vercel. A member of the Team first needs to authorize it. |
GSSoC Label Checklist 🏷️@Priyanshu-byte-coder — please apply the appropriate labels before merging: Difficulty (pick one):
Quality (optional):
Validation (required to score):
|
Owner
|
This PR has merge conflicts with git fetch origin
git rebase origin/main
git push --force-with-lease |
Upstream rewrote the in-memory rate-limiter: replaced the external createMemoryFixedWindowRateLimiter factory with inline memoryBuckets, added pruneMemoryBuckets, and inlined getIp header extraction. This PR added per-IP rate limiting on OAuth auth endpoints using checkAuthRateLimit / isAuthSensitivePath / AUTH_LIMIT. Both edits touched the same lines in src/middleware.ts. Resolution keeps all upstream improvements: - inline memoryBuckets / pruneMemoryBuckets / checkMemoryLimit - getIp reads headers directly (no external getClientIp) - removes createMemoryFixedWindowRateLimiter dependency from middleware And preserves all PR auth rate-limiting functionality: - checkAuthRateLimit / isAuthSensitivePath / AUTH_LIMIT imports - runtime = nodejs - isAuthSensitivePath block intercepting OAuth endpoints - /api/:path* matcher so auth paths are covered
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
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.
Closes #1303
Security analysis
DevTrack authenticates exclusively via GitHub OAuth — there is no password, email, OTP, or magic-link authentication. Despite this, the NextAuth endpoints responsible for initiating and completing the OAuth handshake were completely unprotected:
POST /api/auth/signin/githubGET /api/auth/callback/githubGET /api/auth/link-githubGET /api/auth/link-github/callbackEndpoints intentionally excluded from rate limiting (they are called on every page render and are not attack surfaces):
GET /api/auth/sessionGET /api/auth/csrfGET /api/auth/signoutAuthentication flows discovered
GitHub OAuth only. No password, email/OTP, or magic-link flows exist in this codebase.
Rate limiting strategy
5 requests per IP per 15-minute fixed window (matching issue #1303).
A complete GitHub OAuth sign-in consumes 2 requests (initiation + callback), so the limit allows two full sign-in attempts plus one spare before throttling. In
NODE_ENV=developmentthe limit is relaxed to 1 000 so test suites and local sign-in flows are never blocked.The implementation uses the shared
createMemoryFixedWindowRateLimiterfactory already present in the codebase (@/lib/rate-limit), keeping behaviour consistent with the metrics and contact rate limiters. The key namespaceauth:<ip>is isolated frommetrics-rate-limit:*to prevent any interference.429 responses include
X-RateLimit-Limit,X-RateLimit-Remaining,X-RateLimit-Reset, andRetry-Afterheaders. Response body:{ "error": "Too many authentication attempts. Please try again later." }Files changed
src/lib/auth-rate-limit.ts(new)Standalone auth rate limiter module. Exports
checkAuthRateLimit(),isAuthSensitivePath(),AUTH_LIMIT,AUTH_WINDOW_MS, andAUTH_SENSITIVE_PREFIXES. No new dependencies.src/middleware.ts(modified)checkAuthRateLimit,isAuthSensitivePath,AUTH_LIMITfrom@/lib/auth-rate-limit.config.matcherarray.Tests added
test/auth-rate-limit.test.ts— 22 tests:isAuthSensitivePath(9 tests) — correct classification of protected paths (signin, callback, link-github) and excluded paths (session, csrf, signout, unrelated API routes); consistency withAUTH_SENSITIVE_PREFIXES.AUTH_WINDOW_MS, does not reset before window ends.Verification