fix(api): rate-limit magic-code verify, bound per-token attempts (GHSA-9pvm-fcf6-9234)#9130
Open
sriramveeraghanta wants to merge 4 commits into
Open
fix(api): rate-limit magic-code verify, bound per-token attempts (GHSA-9pvm-fcf6-9234)#9130sriramveeraghanta wants to merge 4 commits into
sriramveeraghanta wants to merge 4 commits into
Conversation
…mpts The magic-link sign-in / sign-up endpoints accept a 6-digit numeric code (900k-value space, 600s TTL) but never increment a failure counter on a wrong-code verify and extend django.views.View rather than DRF APIView, so DRF's AuthenticationThrottle never runs against them. The space-side generate endpoint also lacked throttle_classes. Combined, this allowed an unauthenticated attacker who knew a victim's email to brute-force the code within the TTL window and log in as the victim. - Add MAX_VERIFY_ATTEMPTS=5 in MagicCodeProvider.set_user_data: failed comparisons now persist verify_attempts in Redis under the remaining TTL and, on hitting the limit, delete the key and raise EMAIL_CODE_ATTEMPT_EXHAUSTED. This is the load-bearing fix - it caps total attempts per issued token regardless of request rate. - Add authentication_throttle_allows() so plain Django Views can apply AuthenticationThrottle without converting to APIView (would change CSRF + request-parsing semantics for the redirect-flow endpoints). - Apply the throttle to MagicSignIn/UpEndpoint and the space variants; add throttle_classes to MagicGenerateSpaceEndpoint to match its app sibling. Refs GHSA-9pvm-fcf6-9234.
sriramveeraghanta
requested review from
dheeru0198 and
pablohashescobar
as code owners
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR enforces a per-token MAX_VERIFY_ATTEMPTS tracked in Redis and adds per-IP rate-limiting for magic sign-in/sign-up flows, with early throttle checks that redirect throttled requests containing a RATE_LIMIT_EXCEEDED error payload. ChangesMagic code attempt and rate limiting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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)
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Addresses GHSA-9pvm-fcf6-9234 by hardening magic-link authentication against brute-force attempts and ensuring rate limiting is applied consistently across both DRF APIView and plain django.views.View redirect-flow endpoints.
Changes:
- Add a per-token
verify_attemptscounter in Redis for magic-code verification and invalidate the token after too many wrong attempts. - Introduce
authentication_throttle_allows(request)to applyAuthenticationThrottleto non-DRF (View) endpoints. - Apply throttling to magic-link sign-in/sign-up redirect endpoints and add throttling to space magic-link generation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| apps/api/plane/authentication/views/space/magic.py | Adds AuthenticationThrottle to space magic generate endpoint and applies manual throttle checks to space sign-in/sign-up redirect endpoints. |
| apps/api/plane/authentication/views/app/magic.py | Applies manual throttle checks to app sign-in/sign-up redirect endpoints (which are plain View subclasses). |
| apps/api/plane/authentication/rate_limit.py | Adds authentication_throttle_allows() helper for applying AuthenticationThrottle to plain Django requests. |
| apps/api/plane/authentication/provider/credentials/magic_code.py | Persists and enforces a per-token wrong-code attempt counter (verify_attempts) with invalidation after reaching the cap. |
Comment on lines
+138
to
+142
| if verify_attempts >= self.MAX_VERIFY_ATTEMPTS: | ||
| # Invalidate the token so further attempts must regenerate | ||
| # (which is itself attempt-counted on the generation path). | ||
| ri.delete(self.key) | ||
| if user_exists: |
Member
Author
There was a problem hiding this comment.
Off-by-one resolved by updating the PR description's test plan to match the actual semantics: verify_attempts is checked with >= MAX_VERIFY_ATTEMPTS after increment, so the 5th wrong attempt itself returns EMAIL_CODE_ATTEMPT_EXHAUSTED_SIGN_IN/UP (4 INVALID responses + 1 EXHAUSTED). This matches the advisory's suggested fix. Updated test plan: "POST 4 wrong codes, then the 5th attempt redirects with EXHAUSTED."
![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: 1
🤖 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 `@apps/api/plane/authentication/provider/credentials/magic_code.py`:
- Around line 129-141: The current read-modify-write updating of verify_attempts
is racy; replace the JSON round-trip with an atomic Redis operation (e.g.
HINCRBY or a small EVAL Lua script) so increments and TTL management happen
atomically. Concretely, stop parsing/writing the whole JSON blob for
verify_attempts in the logic around verify_attempts/ri.set/ri.ttl/ri.delete and
instead store or update a numeric field atomically (use ri.hincrby(self.key,
"verify_attempts", 1) and ensure TTL with ri.expire(self.key, remaining_ttl) or
use ri.eval to increment the JSON field and reset TTL in one script), then check
the returned incremented value against self.MAX_VERIFY_ATTEMPTS and call
ri.delete(self.key) when it meets/exceeds the limit; apply the same atomic
approach to the regenerate counter path as well.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e300b9ae-05aa-4a81-833d-f4f0c17c5d2d
📒 Files selected for processing (4)
apps/api/plane/authentication/provider/credentials/magic_code.pyapps/api/plane/authentication/rate_limit.pyapps/api/plane/authentication/views/app/magic.pyapps/api/plane/authentication/views/space/magic.py
…via env Address PR review feedback: - Replace the JSON read-modify-write of verify_attempts with a Lua EVAL script that INCRs a dedicated counter key and EXPIREs it only on the first increment. The previous round-trip was racy: parallel wrong-code requests could read the same value and both write the same incremented count, letting an attacker exceed MAX_VERIFY_ATTEMPTS under concurrency. Counter is now reset on each new token issuance and cleared on successful verify / exhaustion. - Make AuthenticationThrottle.rate configurable via the AUTHENTICATION_RATE_LIMIT env var (default 10/minute, down from 30 to tighten the budget on unauth auth-adjacent endpoints). Document it in deployments/aio and deployments/cli variables.env.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/api/plane/authentication/provider/credentials/magic_code.py`:
- Around line 149-163: The code reads remaining_ttl = ri.ttl(self.key) and only
clamps when None or < 0, but Redis may return 0 which causes EXPIRE key 0 to
delete the verify-attempt counter; change the clamp to ensure remaining_ttl is
at least 1 (e.g. if remaining_ttl is None or remaining_ttl <= 0 set to 1, or
replace with remaining_ttl = max(1, int(remaining_ttl or 0))) before calling
ri.eval with self._INCREMENT_VERIFY_ATTEMPTS_SCRIPT and
self._verify_attempts_key(self.key) so verify_attempts logic is preserved.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f090eab-ba62-4ce2-a429-8a841da342dc
📒 Files selected for processing (4)
apps/api/plane/authentication/provider/credentials/magic_code.pyapps/api/plane/authentication/rate_limit.pydeployments/aio/community/variables.envdeployments/cli/community/variables.env
✅ Files skipped from review due to trivial changes (2)
- deployments/cli/community/variables.env
- deployments/aio/community/variables.env
…ttle
Add the contract tests called out in the PR test plan:
- TestMagicSignInVerifyAttempts:
- test_exhausted_after_max_wrong_attempts: after MAX_VERIFY_ATTEMPTS
wrong codes the next verify redirects with EMAIL_CODE_ATTEMPT_
EXHAUSTED_SIGN_IN and both Redis keys are deleted; a follow-up
verify reports EXPIRED.
- test_counter_increments_on_each_wrong_attempt: the dedicated
verify_attempts counter advances by exactly one per wrong POST,
matching the atomic Lua INCR.
- test_counter_resets_on_token_regeneration: regenerating the
magic-link clears the counter so the user isn't pre-locked-out by
a prior session's wrong attempts.
- TestMagicSignUpVerifyAttempts.test_signup_exhausted_after_max_wrong_attempts:
the sign-up endpoint returns EMAIL_CODE_ATTEMPT_EXHAUSTED_SIGN_UP on
the exhausting attempt.
- TestAuthenticationThrottle: exercises authentication_throttle_allows
on the plain-View redirect-flow endpoints by patching the rate down
and asserting RATE_LIMIT_EXCEEDED is appended to the redirect URL
once the per-IP budget is exceeded, for both magic-sign-in and
magic-sign-up.
Each new class clears Django cache (DRF throttle storage) and the
per-email Redis keys around every test so runs are independent.
ri.ttl() returns 0 when the token has less than one second remaining (Redis floors to whole seconds). The previous clamp only caught None and < 0, so a sub-second TTL would pass through and the Lua script's EXPIRE counter 0 would immediately delete the key — letting an attacker bypass MAX_VERIFY_ATTEMPTS during the final second of the token's life. Switch the comparison to <= 0. Narrow real-world impact (sub-second window, throttle still bounds the rate) but the cap should hold regardless of timing.
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.
Summary
Fixes the brute-force account-takeover primitive described in GHSA-9pvm-fcf6-9234.
The magic-link sign-in / sign-up endpoints accept a 6-digit numeric code (~900k-value space, 600s TTL) but (a) never increment a failure counter on a wrong-code verify, and (b) extend
django.views.Viewrather than DRFAPIView, so DRF'sAuthenticationThrottlenever runs against them. The space-sideMagicGenerateSpaceEndpointalso lackedthrottle_classes. Combined, an unauthenticated attacker who knew a victim's email could brute-force the code within the TTL window and log in as the victim — bypassing password and TOTP.Changes
MagicCodeProvider.set_user_data: on each wrong code, atomically increment a dedicatedmagic_<email>:verify_attemptscounter via a LuaEVALscript (INCR + first-time EXPIRE aligned with the token TTL). Once the count reachesMAX_VERIFY_ATTEMPTS = 5, delete the token + counter and raiseEMAIL_CODE_ATTEMPT_EXHAUSTED_SIGN_IN/UP. The counter is also reset on each new token issuance and on successful verify. This is the load-bearing fix — it caps brute-force attempts per issued token regardless of request rate, and the atomic INCR closes the concurrency window flagged in review.rate_limit.py: addauthentication_throttle_allows(request)so plaindjango.views.Viewsubclasses can applyAuthenticationThrottlewithout being converted toAPIView(which would change CSRF + request-parsing semantics for the redirect-flow endpoints). Throttle rate is now configurable via theAUTHENTICATION_RATE_LIMITenv var (default10/minute).views/app/magic.py: apply the throttle helper at the top ofMagicSignInEndpoint.postandMagicSignUpEndpoint.post.views/space/magic.py: addthrottle_classes = [AuthenticationThrottle]toMagicGenerateSpaceEndpointto match its app sibling; apply the throttle helper toMagicSignInSpaceEndpointandMagicSignUpSpaceEndpoint.deployments/aioanddeployments/clivariables.env: documentAUTHENTICATION_RATE_LIMIT=10/minute.tests/contract/app/test_authentication.py: new contract tests for the attempt cap (sign-in + sign-up), counter increment/reset semantics, and the per-IP throttle on bothmagic-sign-in/andmagic-sign-up/.After this change the worst-case brute-force budget against any single email is 5 wrong codes per issued token × at most 3 regenerations (existing protection in
MagicCodeProvider.initiate) — mathematically infeasible against a 900k-value space, on top of a 10-req/min anonymous throttle per IP.Test plan
apps/api/plane/tests/contract/app/test_authentication.py):TestMagicLinkGenerate,TestMagicSignIn,TestMagicSignUpshould pass unchanged.TestMagicSignInVerifyAttempts.test_exhausted_after_max_wrong_attempts: POST 4 wrong codes (eachINVALID_MAGIC_CODE_SIGN_IN), then the 5th attempt redirects withEMAIL_CODE_ATTEMPT_EXHAUSTED_SIGN_IN; bothmagic_<email>andmagic_<email>:verify_attemptskeys are deleted; the follow-up verify reportsEXPIRED.TestMagicSignInVerifyAttempts.test_counter_increments_on_each_wrong_attempt: counter advances by exactly one per wrong POST (validates the Lua INCR path).TestMagicSignInVerifyAttempts.test_counter_resets_on_token_regeneration: regenerating the magic-link clears the counter so users aren't pre-locked-out.TestMagicSignUpVerifyAttempts.test_signup_exhausted_after_max_wrong_attempts: the SIGN_UP variant of EXHAUSTED is returned on the exhausting attempt.TestAuthenticationThrottle.test_magic_sign_in_throttled/test_magic_sign_up_throttled: patchingAuthenticationThrottle.rateand posting past the limit appendsRATE_LIMIT_EXCEEDEDto the redirect URL.AUTHENTICATION_RATE_LIMIT(e.g.5/minute) in env and verify the throttle kicks in at the new threshold.Refs GHSA-9pvm-fcf6-9234.
Summary by CodeRabbit
Release Notes