From f51fbd7df38330877acc5f9ec3e1bbfcfbbc4abf Mon Sep 17 00:00:00 2001 From: Raphael Michel Date: Tue, 2 Jun 2026 12:19:29 +0200 Subject: [PATCH] Review notes --- src/pretix/base/forms/auth.py | 6 +++++- src/pretix/base/forms/user.py | 9 +++++++++ src/pretix/control/views/user.py | 1 + src/pretix/helpers/ratelimit.py | 22 ++++++++++++++-------- 4 files changed, 29 insertions(+), 9 deletions(-) diff --git a/src/pretix/base/forms/auth.py b/src/pretix/base/forms/auth.py index 69e9d59d7..41d7b82ee 100644 --- a/src/pretix/base/forms/auth.py +++ b/src/pretix/base/forms/auth.py @@ -84,7 +84,9 @@ class LoginForm(forms.Form): def clean(self): if all(k in self.cleaned_data for k, f in self.fields.items() if f.required): - if rate_limit("login", include_ip_from_request=self.request, max_num=10, expire_time=300): + rate_limit_kwargs = dict(include_ip_from_request=self.request, max_num=10, expire_time=300) + if rate_limit("login", **rate_limit_kwargs, increase=False): + # Check rate limit without counting up, we increase below only on failed logins pretix_failed_logins.inc(1, reason="ratelimit") logger.info("Backend login rejected due to rate limit.") raise forms.ValidationError(self.error_messages['rate_limit'], code='rate_limit') @@ -92,6 +94,8 @@ class LoginForm(forms.Form): if self.user_cache is None: logger.info("Backend login invalid.") pretix_failed_logins.inc(1, reason="invalid") + # Count towards rate limit (result is ignored, we are checking above) + rate_limit("login", **rate_limit_kwargs) raise forms.ValidationError( self.error_messages['invalid_login'], code='invalid_login' diff --git a/src/pretix/base/forms/user.py b/src/pretix/base/forms/user.py index 4b413ad51..4f7dc6102 100644 --- a/src/pretix/base/forms/user.py +++ b/src/pretix/base/forms/user.py @@ -177,10 +177,19 @@ class UserEmailChangeForm(forms.Form): def __init__(self, *args, **kwargs): self.user = kwargs.pop('user') + self.request = kwargs.pop('request') super().__init__(*args, **kwargs) def clean_new_email(self): email = self.cleaned_data['new_email'] + + if rate_limit("emailchange_attempt", include_ip_from_request=self.request, max_num=5, expire_time=300): + # Rate limit lookup for conflicting email addresses to make enumeration harder + raise forms.ValidationError( + self.error_messages['rate_limit'], + code='rate_limit', + ) + if User.objects.filter(Q(email__iexact=email) & ~Q(pk=self.user.pk)).exists(): raise forms.ValidationError( self.error_messages['duplicate_identifier'], diff --git a/src/pretix/control/views/user.py b/src/pretix/control/views/user.py index b7a28a73e..a0e30ee13 100644 --- a/src/pretix/control/views/user.py +++ b/src/pretix/control/views/user.py @@ -880,6 +880,7 @@ class UserEmailChangeView(RecentAuthenticationRequiredMixin, FormView): return { **super().get_form_kwargs(), + "request": self.request, "user": self.request.user, } diff --git a/src/pretix/helpers/ratelimit.py b/src/pretix/helpers/ratelimit.py index 45e8b6650..01d48ab24 100644 --- a/src/pretix/helpers/ratelimit.py +++ b/src/pretix/helpers/ratelimit.py @@ -50,16 +50,18 @@ def _get_ip(request): return str(client_ip) -def rate_limit(key: str, *parameters, include_ip_from_request: HttpRequest=None, max_num: int, expire_time: int): +def rate_limit(key: str, *parameters, include_ip_from_request: HttpRequest=None, max_num: int, expire_time: int, increase: bool = True): """ This is a shared utility to implement simple rate limiting in operations like - password resets. This is by far no perfect implementation of rate limiting, as - it the window is prolonge + password resets. - :param key: The key refering to the feature like "pwreset" + :param key: The key referring to the feature like "pwreset" :param parameters: Any number of things to be hashed as the bucket key + :param include_ip_from_request: Add IP address from request to the bucket key. If IP address cannot be determined, + rate limit is not applied. :param max_num: The maximum number of actions to performed within expire_time of the first action :param expire_time: The length of the time window in seconds + :param increase: Whether to count the call as an event counted towards the rate, or just check :return: """ if not settings.HAS_REDIS: @@ -77,10 +79,14 @@ def rate_limit(key: str, *parameters, include_ip_from_request: HttpRequest=None, parameters = (*parameters, ip) redis_key = _get_key(key, parameters) - p = rc.pipeline() - p.set(redis_key, 0, nx=True, ex=expire_time) # Start a rate limit window if none is running - p.incr(redis_key) - new_counter = p.execute()[1] + + if increase: + p = rc.pipeline() + p.set(redis_key, 0, nx=True, ex=expire_time) # Start a rate limit window if none is running + p.incr(redis_key) + new_counter = p.execute()[1] + else: + new_counter = int(rc.get(redis_key) or 0) if new_counter > max_num: return True