From 8f69cb166d8c3bc062ee28a2a45176cf5f594955 Mon Sep 17 00:00:00 2001 From: Raphael Michel Date: Wed, 26 Nov 2025 19:39:32 +0100 Subject: [PATCH] [SECURITY] Fix old password not validated on password change --- src/pretix/base/forms/user.py | 21 +++++++----- src/tests/control/test_user.py | 62 ++++++++++++++++++++++------------ 2 files changed, 52 insertions(+), 31 deletions(-) diff --git a/src/pretix/base/forms/user.py b/src/pretix/base/forms/user.py index e3083322de..773f952203 100644 --- a/src/pretix/base/forms/user.py +++ b/src/pretix/base/forms/user.py @@ -89,8 +89,6 @@ class User2FADeviceAddForm(forms.Form): class UserPasswordChangeForm(forms.Form): error_messages = { - 'pw_current': _("Please enter your current password if you want to change your email address " - "or password."), 'pw_current_wrong': _("The current password you entered was not correct."), 'pw_mismatch': _("Please enter the same password twice"), 'rate_limit': _("For security reasons, please wait 5 minutes before you try again."), @@ -103,19 +101,19 @@ class UserPasswordChangeForm(forms.Form): attrs={'autocomplete': 'username'}, )) old_pw = forms.CharField(max_length=255, - required=False, + required=True, label=_("Your current password"), widget=forms.PasswordInput( attrs={'autocomplete': 'current-password'}, )) new_pw = forms.CharField(max_length=255, - required=False, + required=True, label=_("New password"), widget=forms.PasswordInput( attrs={'autocomplete': 'new-password'}, )) new_pw_repeat = forms.CharField(max_length=255, - required=False, + required=True, label=_("Repeat new password"), widget=forms.PasswordInput( attrs={'autocomplete': 'new-password'}, @@ -130,7 +128,7 @@ class UserPasswordChangeForm(forms.Form): def clean_old_pw(self): old_pw = self.cleaned_data.get('old_pw') - if old_pw and settings.HAS_REDIS: + if settings.HAS_REDIS: from django_redis import get_redis_connection rc = get_redis_connection("redis") cnt = rc.incr('pretix_pwchange_%s' % self.user.pk) @@ -141,7 +139,7 @@ class UserPasswordChangeForm(forms.Form): code='rate_limit', ) - if old_pw and not check_password(old_pw, self.user.password): + if not check_password(old_pw, self.user.password): raise forms.ValidationError( self.error_messages['pw_current_wrong'], code='pw_current_wrong', @@ -151,17 +149,22 @@ class UserPasswordChangeForm(forms.Form): def clean_new_pw(self): password1 = self.cleaned_data.get('new_pw', '') - if password1 and validate_password(password1, user=self.user) is not None: + if validate_password(password1, user=self.user) is not None: raise forms.ValidationError( _(password_validators_help_texts()), code='pw_invalid' ) + if self.user.check_password(password1): + raise forms.ValidationError( + self.error_messages['pw_equal'], + code='pw_equal', + ) return password1 def clean_new_pw_repeat(self): password1 = self.cleaned_data.get('new_pw') password2 = self.cleaned_data.get('new_pw_repeat') - if password1 and password1 != password2: + if password1 != password2: raise forms.ValidationError( self.error_messages['pw_mismatch'], code='pw_mismatch' diff --git a/src/tests/control/test_user.py b/src/tests/control/test_user.py index 966f5e6ba8..95d9023a50 100644 --- a/src/tests/control/test_user.py +++ b/src/tests/control/test_user.py @@ -42,8 +42,8 @@ from pretix.testutils.mock import mocker_context class UserSettingsTest(SoupTest): def setUp(self): super().setUp() - self.user = User.objects.create_user('dummy@dummy.dummy', 'barfoofoo') - self.client.login(email='dummy@dummy.dummy', password='barfoofoo') + self.user = User.objects.create_user('dummy@dummy.dummy', 'old_qvuSpukdKWUV7m7PoRrWwpCd2Taij9') + self.client.login(email='dummy@dummy.dummy', password='old_qvuSpukdKWUV7m7PoRrWwpCd2Taij9') doc = self.get_doc('/control/settings') self.form_data = extract_form_fields(doc.select('form[data-testid="usersettingsform"]')[0]) @@ -74,8 +74,8 @@ class UserSettingsTest(SoupTest): class UserEmailChangeTest(SoupTest): def setUp(self): super().setUp() - self.user = User.objects.create_user('dummy@dummy.dummy', 'barfoofoo') - self.client.login(email='dummy@dummy.dummy', password='barfoofoo') + self.user = User.objects.create_user('dummy@dummy.dummy', 'old_qvuSpukdKWUV7m7PoRrWwpCd2Taij9') + self.client.login(email='dummy@dummy.dummy', password='old_qvuSpukdKWUV7m7PoRrWwpCd2Taij9') session = self.client.session session['pretix_auth_login_time'] = int(time.time()) session.save() @@ -92,7 +92,7 @@ class UserEmailChangeTest(SoupTest): self.assertEqual(response.status_code, 302) response = self.client.post('/control/reauth/?next=/control/settings/email/change', { - 'password': 'barfoofoo' + 'password': 'old_qvuSpukdKWUV7m7PoRrWwpCd2Taij9' }) self.assertIn('/control/settings/email/change', response['Location']) self.assertEqual(response.status_code, 302) @@ -151,8 +151,8 @@ class UserEmailChangeTest(SoupTest): class UserPasswordChangeTest(SoupTest): def setUp(self): super().setUp() - self.user = User.objects.create_user('dummy@dummy.dummy', 'barfoofoo') - self.client.login(email='dummy@dummy.dummy', password='barfoofoo') + self.user = User.objects.create_user('dummy@dummy.dummy', 'old_qvuSpukdKWUV7m7PoRrWwpCd2Taij9') + self.client.login(email='dummy@dummy.dummy', password='old_qvuSpukdKWUV7m7PoRrWwpCd2Taij9') doc = self.get_doc('/control/settings/password/change') self.form_data = extract_form_fields(doc.select('.container-fluid form')[0]) @@ -163,10 +163,23 @@ class UserPasswordChangeTest(SoupTest): def test_change_password_require_password(self): doc = self.save({ - 'new_pw': 'foo', - 'new_pw_repeat': 'foo', + 'new_pw': 'f00barbarbar', + 'new_pw_repeat': 'f00barbarbar', }) assert doc.select(".alert-danger") + assert "This field is required." in doc.select(".has-error")[0].text + pw = self.user.password + self.user = User.objects.get(pk=self.user.pk) + assert self.user.password == pw + + def test_change_password_old_password_wrong(self): + doc = self.save({ + 'new_pw': 'f00barbarbar', + 'new_pw_repeat': 'f00barbarbar', + 'old_pw': 'lolwrong', + }) + assert doc.select(".alert-danger") + assert "The current password you entered was not correct." in doc.select(".has-error")[0].text pw = self.user.password self.user = User.objects.get(pk=self.user.pk) assert self.user.password == pw @@ -177,7 +190,7 @@ class UserPasswordChangeTest(SoupTest): self.save({ 'new_pw': 'f00barbarbar', 'new_pw_repeat': 'f00barbarbar', - 'old_pw': 'barfoofoo', + 'old_pw': 'old_qvuSpukdKWUV7m7PoRrWwpCd2Taij9', }) pw = self.user.password self.user = User.objects.get(pk=self.user.pk) @@ -187,7 +200,7 @@ class UserPasswordChangeTest(SoupTest): doc = self.save({ 'new_pw': 'f00barbarbar', 'new_pw_repeat': 'f00barbarbar', - 'old_pw': 'barfoofoo', + 'old_pw': 'old_qvuSpukdKWUV7m7PoRrWwpCd2Taij9', }) assert doc.select(".alert-success") self.user = User.objects.get(pk=self.user.pk) @@ -197,9 +210,10 @@ class UserPasswordChangeTest(SoupTest): doc = self.save({ 'new_pw': 'foo', 'new_pw_repeat': 'foo', - 'old_pw': 'barfoofoo', + 'old_pw': 'old_qvuSpukdKWUV7m7PoRrWwpCd2Taij9', }) assert doc.select(".alert-danger") + assert "This password is too short." in doc.select(".has-error")[0].text pw = self.user.password self.user = User.objects.get(pk=self.user.pk) assert self.user.password == pw @@ -208,37 +222,40 @@ class UserPasswordChangeTest(SoupTest): doc = self.save({ 'new_pw': 'dummy123', 'new_pw_repeat': 'dummy123', - 'old_pw': 'barfoofoo', + 'old_pw': 'old_qvuSpukdKWUV7m7PoRrWwpCd2Taij9', }) assert doc.select(".alert-danger") + assert "The password is too similar to the Email." in doc.select(".has-error")[0].text pw = self.user.password self.user = User.objects.get(pk=self.user.pk) assert self.user.password == pw def test_change_password_require_repeat(self): doc = self.save({ - 'new_pw': 'foooooooooooooo', - 'new_pw_repeat': 'baaaaaaaaaaaar', - 'old_pw': 'barfoofoo', + 'new_pw': 'foooooooooooooo1234', + 'new_pw_repeat': 'baaaaaaaaaaaar1234', + 'old_pw': 'old_qvuSpukdKWUV7m7PoRrWwpCd2Taij9', }) assert doc.select(".alert-danger") + assert "Please enter the same password twice" in doc.select(".has-error")[0].text pw = self.user.password self.user = User.objects.get(pk=self.user.pk) assert self.user.password == pw def test_change_password_require_new(self): doc = self.save({ - 'new_pw': 'barfoofoo', - 'new_pw_repeat': 'barfoofoo', - 'old_pw': 'barfoofoo', + 'new_pw': 'old_qvuSpukdKWUV7m7PoRrWwpCd2Taij9', + 'new_pw_repeat': 'old_qvuSpukdKWUV7m7PoRrWwpCd2Taij9', + 'old_pw': 'old_qvuSpukdKWUV7m7PoRrWwpCd2Taij9', }) - assert doc.select(".alert-danger") + assert doc.select(".has-error") + assert "Your password may not be the same as" in doc.select(".has-error")[0].text def test_change_password_history(self): doc = self.save({ 'new_pw': 'qvuSpukdKWUV7m7PoRrWwpCd2Taij9', 'new_pw_repeat': 'qvuSpukdKWUV7m7PoRrWwpCd2Taij9', - 'old_pw': 'barfoofoo', + 'old_pw': 'old_qvuSpukdKWUV7m7PoRrWwpCd2Taij9', }) assert doc.select(".alert-success") @@ -255,6 +272,7 @@ class UserPasswordChangeTest(SoupTest): 'old_pw': '9UQl4lSwHLMVUIMgw0L1X8XEFmyvdn', }) assert doc.select(".alert-danger") + assert "Your password may not be the same as one of your 4 previous passwords." in doc.select(".has-error")[0].text def test_needs_password_change_changed(self): self.user.needs_password_change = True @@ -262,7 +280,7 @@ class UserPasswordChangeTest(SoupTest): self.save({ 'new_pw': 'f00barbarbar', 'new_pw_repeat': 'f00barbarbar', - 'old_pw': 'barfoofoo' + 'old_pw': 'old_qvuSpukdKWUV7m7PoRrWwpCd2Taij9' }) self.user.refresh_from_db() assert self.user.needs_password_change is False