-
Notifications
You must be signed in to change notification settings - Fork 4.4k
fix(api): rate-limit magic-code verify, bound per-token attempts (GHSA-9pvm-fcf6-9234) #9130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: preview
Are you sure you want to change the base?
Changes from all commits
f817880
4d836df
c14182b
8cdbeb9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,27 @@ | |
| class MagicCodeProvider(CredentialAdapter): | ||
| provider = "magic-code" | ||
|
|
||
| # Max wrong-code verification attempts per issued token before the token | ||
| # is invalidated. Prevents brute-forcing the 6-digit code space within | ||
| # the token TTL window. | ||
| MAX_VERIFY_ATTEMPTS = 5 | ||
|
|
||
| # Atomic INCR + first-time EXPIRE for the verify-attempt counter. | ||
| # Using a dedicated counter key with this script makes the increment | ||
| # safe under concurrent wrong-code requests; a plain JSON read/modify/ | ||
| # write would race and let parallel attackers exceed the cap. | ||
| _INCREMENT_VERIFY_ATTEMPTS_SCRIPT = ( | ||
| 'local count = redis.call("INCR", KEYS[1]) ' | ||
| 'if count == 1 then ' | ||
| ' redis.call("EXPIRE", KEYS[1], tonumber(ARGV[1])) ' | ||
| 'end ' | ||
| 'return count' | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def _verify_attempts_key(token_key): | ||
| return f"{token_key}:verify_attempts" | ||
|
|
||
| def __init__(self, request, key, code=None, callback=None): | ||
| (EMAIL_HOST, ENABLE_MAGIC_LINK_LOGIN) = get_configuration_value( | ||
| [ | ||
|
|
@@ -92,6 +113,9 @@ def initiate(self): | |
| expiry = 600 | ||
|
|
||
| ri.set(key, json.dumps(value), ex=expiry) | ||
| # Reset the verify-attempt counter so each newly issued token starts | ||
| # with a fresh budget of MAX_VERIFY_ATTEMPTS. | ||
| ri.delete(self._verify_attempts_key(key)) | ||
| return key, token | ||
|
|
||
| def set_user_data(self): | ||
|
|
@@ -114,12 +138,52 @@ def set_user_data(self): | |
| }, | ||
| } | ||
| ) | ||
| # Delete the token from redis if the code match is successful | ||
| # Delete the token and its counter from redis on success. | ||
| ri.delete(self.key) | ||
| ri.delete(self._verify_attempts_key(self.key)) | ||
| return | ||
| else: | ||
| email = str(self.key).replace("magic_", "", 1) | ||
| if User.objects.filter(email=email).exists(): | ||
| user_exists = User.objects.filter(email=email).exists() | ||
|
|
||
| # Atomically increment the verify-attempt counter in Redis. | ||
| # The Lua script sets the TTL only on the first increment so | ||
| # the lockout window matches the remaining token TTL and does | ||
| # not get extended by every wrong-code attempt. | ||
| # ri.ttl() returns -2 (missing), -1 (no expiry), 0 (sub-second | ||
| # remaining; Redis floors to whole seconds), or a positive int. | ||
| # Clamp to >=1 because EXPIRE key 0 immediately deletes the key | ||
| # and would let an attacker bypass the cap in the final second. | ||
| remaining_ttl = ri.ttl(self.key) | ||
| if remaining_ttl is None or remaining_ttl <= 0: | ||
| remaining_ttl = 1 | ||
| verify_attempts = int( | ||
| ri.eval( | ||
| self._INCREMENT_VERIFY_ATTEMPTS_SCRIPT, | ||
| 1, | ||
| self._verify_attempts_key(self.key), | ||
| remaining_ttl, | ||
| ) | ||
| ) | ||
|
|
||
| if verify_attempts >= self.MAX_VERIFY_ATTEMPTS: | ||
| # Invalidate the token (and counter) so further attempts | ||
| # must regenerate; regeneration is itself attempt-counted. | ||
| ri.delete(self.key) | ||
| ri.delete(self._verify_attempts_key(self.key)) | ||
| if user_exists: | ||
|
Comment on lines
+169
to
+174
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Off-by-one resolved by updating the PR description's test plan to match the actual semantics: |
||
| raise AuthenticationException( | ||
| error_code=AUTHENTICATION_ERROR_CODES["EMAIL_CODE_ATTEMPT_EXHAUSTED_SIGN_IN"], | ||
| error_message="EMAIL_CODE_ATTEMPT_EXHAUSTED_SIGN_IN", | ||
| payload={"email": str(email)}, | ||
| ) | ||
| raise AuthenticationException( | ||
| error_code=AUTHENTICATION_ERROR_CODES["EMAIL_CODE_ATTEMPT_EXHAUSTED_SIGN_UP"], | ||
| error_message="EMAIL_CODE_ATTEMPT_EXHAUSTED_SIGN_UP", | ||
| payload={"email": str(email)}, | ||
| ) | ||
|
sriramveeraghanta marked this conversation as resolved.
|
||
|
|
||
| if user_exists: | ||
| raise AuthenticationException( | ||
| error_code=AUTHENTICATION_ERROR_CODES["INVALID_MAGIC_CODE_SIGN_IN"], | ||
| error_message="INVALID_MAGIC_CODE_SIGN_IN", | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.