feat(auth): harden register with turnstile and rate limiting#165
Merged
feat(auth): harden register with turnstile and rate limiting#165
Conversation
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds Cloudflare Turnstile CAPTCHA to registration (frontend + backend), enforces server-side captcha verification with timeout and schema checks, introduces IP- and per-email rate limiting on register, tightens password rules to 8+ chars, updates config to require TURNSTILE_SECRET_KEY (except in tests), and extends tests for captcha and rate-limit cases. Changes
Sequence DiagramsequenceDiagram
participant User
participant Frontend as Register Page
participant Turnstile as Turnstile Widget
participant Backend as API
participant RateLimiter as Rate Limiter
participant TurnstileSvc as Turnstile Service
participant DB as Database
User->>Frontend: Open registration page
Frontend->>Turnstile: Load script & render widget
Turnstile->>User: Show challenge
User->>Turnstile: Complete challenge
Turnstile->>Frontend: Callback with captchaToken
Frontend->>Frontend: Store captchaToken
User->>Frontend: Submit form (name,email,password,captchaToken)
Frontend->>Backend: POST REGISTER_MUTATION
Backend->>Backend: Normalize email & extract IP
Backend->>RateLimiter: Check IP rate limit
alt IP limit exceeded
RateLimiter->>Backend: Reject
Backend->>Frontend: Rate limit error
else
Backend->>RateLimiter: Check email rate limit
alt Email limit exceeded
RateLimiter->>Backend: Reject
Backend->>Frontend: Rate limit error
else
Backend->>TurnstileSvc: verifyTurnstileToken(token, ip)
TurnstileSvc->>Turnstile: POST verification
alt Verified
Turnstile->>TurnstileSvc: success:true
TurnstileSvc->>Backend: OK
Backend->>DB: Create user (normalized email)
DB->>Backend: User created
Backend->>Frontend: Success
else Verification failed
Turnstile->>TurnstileSvc: success:false
TurnstileSvc->>Backend: Throw badInput
Backend->>Frontend: Captcha error
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/app/(auth)/register/page.tsx (1)
80-84:⚠️ Potential issue | 🟡 MinorPassword hint lies to the user — fix the mismatch.
The regex on line 80 requires
{4,}characters, but the UI hint on line 162 tells users "at least 8 characters." One of these is wrong, and either way, you're setting users up for confusion. Pick a lane and stay hard.🔧 Proposed fix: align regex with hint (assuming 8 is correct)
- const passwordRegex = /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[@$!%*?&])[A-Za-z\d@$!%*?&]{4,}$/; + const passwordRegex = /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[@$!%*?&])[A-Za-z\d@$!%*?&]{8,}$/;Also applies to: 161-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/`(auth)/register/page.tsx around lines 80 - 84, The password validation currently uses passwordRegex with a quantifier of {4,} which contradicts the UI hint; update passwordRegex in page.tsx to require at least 8 characters (change {4,} to {8,}) and also update the setFormError message text used by the register page to mention "at least 8 characters" so the error string and regex (passwordRegex) are consistent; ensure you also update the duplicate hint/error instances referenced around lines 161-163 to the same "8 characters" wording.frontend/src/lib/auth/AuthContext.tsx (1)
147-157:⚠️ Potential issue | 🟡 MinorError handling gap: non-ApolloError exceptions fall through silently.
If something throws that isn't an
ApolloError, the catch block does nothing — no error set, no feedback, user just stares at a spinner. That's weak. Handle the fallback case.🐛 Proposed fix: handle non-ApolloError exceptions
} catch (err: unknown) { if (err instanceof ApolloError) { if (err.graphQLErrors && err.graphQLErrors.length > 0) { setError(err.graphQLErrors[0].message); } else if (err.networkError) { setError(`Network error: ${err.networkError.message}`); } else { setError(err.message); } + } else if (err instanceof Error) { + setError(err.message); + } else { + setError('An unexpected error occurred. Please try again.'); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/auth/AuthContext.tsx` around lines 147 - 157, The catch block in AuthContext.tsx only handles ApolloError and ignores all other exceptions, so update the catch in the function where setError is used (the catch handling around the Apollo call in AuthContext) to handle non-ApolloError cases: add an else branch that sets a user-visible error via setError (e.g. a fallback message using err instanceof Error ? err.message : String(err) or JSON.stringify(err)) and log the raw error (console.error or a logger) so unexpected exceptions don't leave the UI spinning with no feedback.
🧹 Nitpick comments (2)
backend/src/__tests__/integration/authResolvers.test.ts (1)
53-131: The limiter still hasn't done any test reps.These cases cover captcha pass/fail, but the new IP/email throttles never get exercised. Add one test that trips the email bucket and one that varies emails to hit the IP bucket, or this guardrail is basically doing push-ups in your imagination.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/__tests__/integration/authResolvers.test.ts` around lines 53 - 131, Add two tests in authResolvers.test.ts that exercise the new throttles: one that repeatedly calls executeOperation(REGISTER_MUTATION, variables) using the same email until the email-rate-limit error is returned (assert error message contains the email-throttle message), and another that sends registrations with different emails but the same client IP (set via executeOperation/request headers) to hit the IP-rate-limit guardrail and assert the IP-throttle error is returned; keep using the same captcha-token flow used by other tests and reuse symbols REGISTER_MUTATION and executeOperation to locate where to add these loops and assertions.frontend/src/app/(auth)/register/page.tsx (1)
85-94: Good validation gates — but consider button disable for better UX.The validation logic is tight: no site key = bail, no token = bail. However, you could also disable the submit button when
!captchaToken && turnstileSiteKeyto prevent the error message dance. Just a nice-to-have, not required.✨ Optional: disable button until CAPTCHA completed
<Button type="submit" className="w-full" - disabled={loading} + disabled={loading || (!!turnstileSiteKey && !captchaToken)} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/`(auth)/register/page.tsx around lines 85 - 94, Add a UX improvement: disable the form submit button when the site key exists but the CAPTCHA hasn't been completed to avoid the error bounce; in the register page component update the submit button render to set disabled when turnstileSiteKey is truthy and captchaToken is falsy (e.g., disabled={turnstileSiteKey && !captchaToken}) and ensure any visual/aria state (aria-disabled or a disabled class) reflects this change so users cannot click until captchaToken is present; keep the existing validation gates (setFormError checks) as a fallback on submit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/config/config.ts`:
- Line 23: The TURNSTILE secret is not enforced at startup: add
TURNSTILE_SECRET_KEY to the list checked by requiredEnvVars (or the function
that builds it) so the app fails fast when missing, but maintain the current
test bypass by only requiring TURNSTILE_SECRET_KEY when process.env.NODE_ENV !==
'test'; update the checks where turnstileSecretKey and TURNSTILE_SECRET_KEY are
referenced (symbols: turnstileSecretKey, requiredEnvVars, TURNSTILE_SECRET_KEY)
so both config construction and the runtime validation paths enforce the
presence outside of tests.
In `@backend/src/resolvers/users/mutations.ts`:
- Around line 11-13: The login path still uses the raw email so mixed-case
registrations fail to authenticate; mirror the register() normalization by
trimming and lowercasing the incoming login email before any lookup. In the
resolver (where input is destructured into name, email, password, captchaToken)
replace uses of email for queries with the existing normalizedEmail (or create
one if missing) and use that normalizedEmail in authentication/user-lookup logic
(same approach as register()) so both registration and login are canonicalized.
- Around line 13-18: The current code uses a fallback 'unknown' for
context.clientIp which causes a global shared rate-limit key; to fix, stop using
'unknown' and only apply the IP-based limiter when a real IP exists: replace the
clientIp assignment (clientIp) and wrap the call to
rateLimiter.limit(`register:${clientIp}`, 5, 3600) so it only runs when
context?.clientIp is truthy/valid (e.g., if (context?.clientIp) { await
rateLimiter.limit(`register:${context.clientIp}`, 5, 3600) ... }), otherwise
skip the IP limiter (and rely on other limits like user/email) to avoid the
shared register:unknown choke point.
- Around line 20-25: The per-email rate limiter call using
rateLimiter.limit(`register-email:${normalizedEmail}`, 3, 3600) is executed
before verifyTurnstileToken(captchaToken, clientIp), which allows attackers to
exhaust an email's bucket with invalid captchas; move the email limiter check
and subsequent throw (the code referencing normalizedEmail and
emailRateLimit.success) to run after verifyTurnstileToken(captchaToken,
clientIp) succeeds (you may still keep any IP-based limiter before the captcha),
so only validated captcha attempts decrement the per-email counter.
In `@backend/src/services/turnstile.ts`:
- Around line 33-53: Add an AbortController-based timeout around the fetch call
to TURNSTILE_VERIFY_URL (cancel the request after a configurable ms) and handle
AbortError as a failed verification with logger.error and Errors.internal; wrap
the response.json() call in try/catch to guard against malformed JSON and treat
parse failures as Errors.internal with logging; define a Zod schema for the
expected TurnstileVerificationResponse (including success:boolean and optional
"error-codes": string[]), validate the parsed JSON with that schema and if
validation fails log details and throw Errors.internal, and if validation passes
but success is false log the error codes and throw Errors.badInput('Captcha
verification failed'). Ensure the fetch uses the AbortController.signal and you
clear the timeout on success.
In `@frontend/src/app/`(auth)/register/page.tsx:
- Around line 58-60: The Turnstile widget rendered by renderTurnstileWidget()
needs explicit teardown: update the useEffect that calls renderTurnstileWidget
to capture its returned widget id (or handle) and return a cleanup function that
calls window.turnstile.remove(widgetId) (guarded by existence checks) to destroy
the widget on unmount; also extend the global Window interface to include the
turnstile.remove method signature so TypeScript knows about it. Ensure you
reference renderTurnstileWidget, the useEffect cleanup return,
window.turnstile.remove, and the Window interface when making the changes.
---
Outside diff comments:
In `@frontend/src/app/`(auth)/register/page.tsx:
- Around line 80-84: The password validation currently uses passwordRegex with a
quantifier of {4,} which contradicts the UI hint; update passwordRegex in
page.tsx to require at least 8 characters (change {4,} to {8,}) and also update
the setFormError message text used by the register page to mention "at least 8
characters" so the error string and regex (passwordRegex) are consistent; ensure
you also update the duplicate hint/error instances referenced around lines
161-163 to the same "8 characters" wording.
In `@frontend/src/lib/auth/AuthContext.tsx`:
- Around line 147-157: The catch block in AuthContext.tsx only handles
ApolloError and ignores all other exceptions, so update the catch in the
function where setError is used (the catch handling around the Apollo call in
AuthContext) to handle non-ApolloError cases: add an else branch that sets a
user-visible error via setError (e.g. a fallback message using err instanceof
Error ? err.message : String(err) or JSON.stringify(err)) and log the raw error
(console.error or a logger) so unexpected exceptions don't leave the UI spinning
with no feedback.
---
Nitpick comments:
In `@backend/src/__tests__/integration/authResolvers.test.ts`:
- Around line 53-131: Add two tests in authResolvers.test.ts that exercise the
new throttles: one that repeatedly calls executeOperation(REGISTER_MUTATION,
variables) using the same email until the email-rate-limit error is returned
(assert error message contains the email-throttle message), and another that
sends registrations with different emails but the same client IP (set via
executeOperation/request headers) to hit the IP-rate-limit guardrail and assert
the IP-throttle error is returned; keep using the same captcha-token flow used
by other tests and reuse symbols REGISTER_MUTATION and executeOperation to
locate where to add these loops and assertions.
In `@frontend/src/app/`(auth)/register/page.tsx:
- Around line 85-94: Add a UX improvement: disable the form submit button when
the site key exists but the CAPTCHA hasn't been completed to avoid the error
bounce; in the register page component update the submit button render to set
disabled when turnstileSiteKey is truthy and captchaToken is falsy (e.g.,
disabled={turnstileSiteKey && !captchaToken}) and ensure any visual/aria state
(aria-disabled or a disabled class) reflects this change so users cannot click
until captchaToken is present; keep the existing validation gates (setFormError
checks) as a fallback on submit.
🪄 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: 68e58325-4287-41f5-b295-10a594e8cb84
📒 Files selected for processing (10)
backend/.env.examplebackend/src/__tests__/integration/authResolvers.test.tsbackend/src/config/config.tsbackend/src/resolvers/users/mutations.tsbackend/src/schemas/types/userTypes.tsbackend/src/services/turnstile.tsbackend/src/validation/schemas/user.schema.tsfrontend/src/__tests__/app/RegisterPage.test.tsxfrontend/src/app/(auth)/register/page.tsxfrontend/src/lib/auth/AuthContext.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/src/__tests__/integration/authResolvers.test.ts (1)
135-166: The rate limit mocking logic is correct but could be clearer.You're mocking two consecutive calls: first returns
success: true(IP passes), second returnssuccess: false(email blocked). This tests the exact flow in mutations.ts where IP check happens before email check. Smart.But future you (or any teammate) might stare at this wondering "why two mocks?" A quick comment would save some head-scratching.
💡 Consider a clarifying comment
it('should fail when email rate limit is exceeded', async () => { + // First mock: IP rate limit passes + // Second mock: Email rate limit fails jest .spyOn(rateLimiter, 'limit') .mockResolvedValueOnce({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/__tests__/integration/authResolvers.test.ts` around lines 135 - 166, Test readability: add a brief clarifying comment in the test "should fail when email rate limit is exceeded" above the jest.spyOn(rateLimiter, 'limit') mock that explains why two consecutive mockResolvedValueOnce calls are used (first simulates the IP check returning success, second simulates the email check returning a block), referencing the test name, rateLimiter.limit, and REGISTER_MUTATION/executeOperation so future readers understand the order of checks being asserted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/resolvers/users/mutations.ts`:
- Line 10: Replace the untyped resolver parameters by importing and using the
existing types: import RegisterInput from the schema (RegisterInput) and
import/export a complete MyContext from backend/src/index.ts, then change the
resolver signatures such as register(...) to use (_: unknown, { input }: {
input: RegisterInput }, context: MyContext); apply the same pattern to other
resolvers in this file (e.g., the resolver at line ~68 and any other untyped
ones) so you stop using any for parameters and ensure MyContext includes req and
res and is exported for reuse.
In `@backend/src/services/turnstile.ts`:
- Line 8: The TURNSTILE_TIMEOUT_MS constant is created with
Number(process.env.TURNSTILE_TIMEOUT_MS || 5000) which yields NaN for invalid
values and breaks setTimeout; change the logic that defines TURNSTILE_TIMEOUT_MS
to parse and validate the environment value (e.g., parseInt or Number, ensure
it's a finite positive integer within reasonable bounds) and fall back to a safe
default (5000) when the env is missing, NaN, non-finite, or non-positive so that
setTimeout(..., TURNSTILE_TIMEOUT_MS) and any controller.abort() call behave
predictably; update any code referencing TURNSTILE_TIMEOUT_MS to rely on this
validated constant.
---
Nitpick comments:
In `@backend/src/__tests__/integration/authResolvers.test.ts`:
- Around line 135-166: Test readability: add a brief clarifying comment in the
test "should fail when email rate limit is exceeded" above the
jest.spyOn(rateLimiter, 'limit') mock that explains why two consecutive
mockResolvedValueOnce calls are used (first simulates the IP check returning
success, second simulates the email check returning a block), referencing the
test name, rateLimiter.limit, and REGISTER_MUTATION/executeOperation so future
readers understand the order of checks being asserted.
🪄 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: 9eeca93d-d77d-43f5-a605-06420bbbe67a
📒 Files selected for processing (9)
backend/src/__tests__/integration/authResolvers.test.tsbackend/src/__tests__/unit/userValidation.test.tsbackend/src/config/config.tsbackend/src/resolvers/users/mutations.tsbackend/src/services/turnstile.tsbackend/src/validation/schemas/user.schema.tsfrontend/src/__tests__/app/RegisterPage.test.tsxfrontend/src/app/(auth)/register/page.tsxfrontend/src/lib/auth/AuthContext.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/tests/app/RegisterPage.test.tsx
- backend/src/config/config.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/resolvers/users/mutations.ts (1)
24-194:⚠️ Potential issue | 🟠 MajorResolvers are missing
createErrorHandlerarmor.Right now these are raw async resolvers. The project rule requires wrapping all resolvers with
createErrorHandler, and this file is still running bare-metal error paths.Suggested pattern
-import { Errors } from '../../utils/errors'; +import { Errors, createErrorHandler } from '../../utils/errors'; export const userMutations = { - register: async (_: unknown, { input }: RegisterArgs, context: ResolverContext) => { + register: createErrorHandler(async (_: unknown, { input }: RegisterArgs, context: ResolverContext) => { // ... - }, + }), // repeat for login/logout/updateUserRole/deleteUser };As per coding guidelines, "Always wrap GraphQL resolvers with
createErrorHandlerfrombackend/src/utils/errors/— never create raw GraphQLError instances."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/resolvers/users/mutations.ts` around lines 24 - 194, Import createErrorHandler from backend/src/utils/errors and wrap each resolver function in the userMutations object (register, login, logout, updateUserRole, deleteUser) with createErrorHandler so they are not raw async functions; preserve the existing logic and return shape, but replace the raw async resolver values with createErrorHandler(async (...) => { ... }) for each named resolver and keep the exported userMutations object unchanged.
♻️ Duplicate comments (1)
backend/src/resolvers/users/mutations.ts (1)
29-35:⚠️ Potential issue | 🟠 Major
unknownIP can still become a shared choke-point.
clientIpis checked for truthiness only, so'unknown'still gets rate-limited as one global bucket. That means unrelated users can bench each other when IP resolution falls back.Suggested fix
- const clientIp = context?.clientIp; + const clientIp = context?.clientIp; - if (clientIp) { + if (clientIp && clientIp !== 'unknown') { const ipRateLimit = await rateLimiter.limit(`register:${clientIp}`, 5, 3600); if (!ipRateLimit.success) { throw Errors.badInput('Too many registration attempts. Please try again later.'); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/resolvers/users/mutations.ts` around lines 29 - 35, The code is treating the literal string 'unknown' as a valid clientIp and thus collapsing many clients into one rate-limit bucket; update the registration rate-limiting check in the resolver so you only call rateLimiter.limit when clientIp is present and not equal to 'unknown' (e.g., check clientIp && clientIp.toLowerCase() !== 'unknown'), and keep using rateLimiter.limit(`register:${clientIp}`, 5, 3600) when that condition passes; reference the clientIp variable and the rateLimiter.limit call in mutations.ts to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@backend/src/resolvers/users/mutations.ts`:
- Around line 24-194: Import createErrorHandler from backend/src/utils/errors
and wrap each resolver function in the userMutations object (register, login,
logout, updateUserRole, deleteUser) with createErrorHandler so they are not raw
async functions; preserve the existing logic and return shape, but replace the
raw async resolver values with createErrorHandler(async (...) => { ... }) for
each named resolver and keep the exported userMutations object unchanged.
---
Duplicate comments:
In `@backend/src/resolvers/users/mutations.ts`:
- Around line 29-35: The code is treating the literal string 'unknown' as a
valid clientIp and thus collapsing many clients into one rate-limit bucket;
update the registration rate-limiting check in the resolver so you only call
rateLimiter.limit when clientIp is present and not equal to 'unknown' (e.g.,
check clientIp && clientIp.toLowerCase() !== 'unknown'), and keep using
rateLimiter.limit(`register:${clientIp}`, 5, 3600) when that condition passes;
reference the clientIp variable and the rateLimiter.limit call in mutations.ts
to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11843708-7391-4de9-bc57-e0626f76b70b
📒 Files selected for processing (2)
backend/src/resolvers/users/mutations.tsbackend/src/services/turnstile.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Behavior & Reliability
Tests