From 53af0ed0820cdbdb6bad430d2dd5156ad13400b5 Mon Sep 17 00:00:00 2001 From: David Cain Date: Sat, 27 Aug 2022 12:54:03 -0700 Subject: [PATCH] Fix signing up with very-common password This prevents an integer-parsing error from causing 500s when trying to sign up with a password that's quite common in HIBP (so common that there are over 999 known pwnages and a comma is included in the count). I should probably add some unit tests, but I expect this to be a short-lived workaround, and I've done some end-to-end testing locally. --- ws/auth.py | 64 ++++++++++++++++++++++++++++++++++ ws/settings.py | 3 +- ws/tests/views/test_account.py | 22 ++++++------ ws/views/account.py | 34 ++---------------- 4 files changed, 80 insertions(+), 43 deletions(-) create mode 100644 ws/auth.py diff --git a/ws/auth.py b/ws/auth.py new file mode 100644 index 00000000..deef72fe --- /dev/null +++ b/ws/auth.py @@ -0,0 +1,64 @@ +import logging +import re +from typing import Optional + +from django.core.exceptions import ValidationError +from django.utils.translation import ngettext +from pwned_passwords_django.api import pwned_password +from pwned_passwords_django.validators import PwnedPasswordsValidator + +from ws import settings + +logger = logging.getLogger(__name__) + +# As of 2022-08-27, pwned_password sometimes includes commas in `times` +# This hack can go away once the library is updated to handle commas +# https://github.com/ubernostrum/pwned-passwords-django/issues/35 +INT_WITH_COMMA = re.compile( + r"invalid literal for int\(\) with base 10: '(?P[\d,]+)'" +) + + +def times_seen_in_hibp(password: str) -> Optional[int]: + """Return times password has been seen in HIBP.""" + if settings.DEBUG and password in settings.WHITELISTED_BAD_PASSWORDS: + return 0 + + try: + return pwned_password(password) + except ValueError as err: + has_comma = INT_WITH_COMMA.match(str(err)) + if not has_comma: + raise + return int(has_comma.group('times').replace(',', '')) + except Exception: # pylint: disable=broad-except + # Let Sentry know we had problems, but don't break the flow. + # Sentry should scrub `password` automatically. + logger.exception("Encountered an error with django-pwned-passwords") + return None + + +class CommaPwnedPasswordsValidator(PwnedPasswordsValidator): + """Temporarily work around an issue with `pwned-passwords-django`. + + This prevents 500s when trying to sign up with a very common password. + Specifically, if a comma is found in the count of times the password is + seen, we should fix the parsing to an integer. + """ + + def validate(self, password, user=None): + try: + super().validate(password, user=user) + except ValueError as err: + has_comma = INT_WITH_COMMA.match(str(err)) + if not has_comma: + raise + + amount = int(has_comma.group('times').replace(',', '')) + raise ValidationError( # pylint:disable=raise-missing-from + ngettext( + self.error_message["singular"], self.error_message["plural"], amount + ), + params={"amount": amount}, + code="pwned_password", + ) diff --git a/ws/settings.py b/ws/settings.py index 499ddda0..7a110907 100644 --- a/ws/settings.py +++ b/ws/settings.py @@ -76,7 +76,8 @@ ) AUTH_PASSWORD_VALIDATORS = [ - {'NAME': 'pwned_passwords_django.validators.PwnedPasswordsValidator'} + # {'NAME': 'pwned_passwords_django.validators.PwnedPasswordsValidator'} + {'NAME': 'ws.auth.CommaPwnedPasswordsValidator'} ] LOGIN_REDIRECT_URL = '/' diff --git a/ws/tests/views/test_account.py b/ws/tests/views/test_account.py index c68ccdfa..3c523670 100644 --- a/ws/tests/views/test_account.py +++ b/ws/tests/views/test_account.py @@ -7,7 +7,7 @@ from freezegun import freeze_time from pwned_passwords_django import api -from ws import models +from ws import auth, models from ws.tests import factories from ws.views import account @@ -35,7 +35,7 @@ def test_previously_insecure_password_marked_secure(self): """ par = factories.ParticipantFactory.create(user=self.user) factories.PasswordQualityFactory.create(participant=par, is_insecure=True) - with mock.patch.object(account, 'pwned_password') as pwned_password: + with mock.patch.object(auth, 'pwned_password') as pwned_password: pwned_password.return_value = None # (API was down) response = self._login() @@ -50,7 +50,7 @@ def test_previously_insecure_password_marked_secure(self): # Because the API was down, we did not write that we checked the password self.assertIsNone(quality.last_checked) - @mock.patch.object(account, 'settings') + @mock.patch.object(auth, 'settings') def test_known_bad_password(self, mocked_settings): """We include a debug mode that supports passing known bad passwords.""" par = factories.ParticipantFactory.create(user_id=self.user.pk) @@ -59,7 +59,7 @@ def test_known_bad_password(self, mocked_settings): mocked_settings.DEBUG = True mocked_settings.WHITELISTED_BAD_PASSWORDS = ['football'] - with mock.patch.object(account, 'pwned_password') as pwned_password: + with mock.patch.object(auth, 'pwned_password') as pwned_password: response = self.client.post('/accounts/login/', self.form_data) # We don't bother hitting the API! @@ -83,7 +83,7 @@ def test_known_bad_password(self, mocked_settings): self.assertEqual(mocked_settings.WHITELISTED_BAD_PASSWORDS, ['football']) # This time, we hit the API and mark the user as having an insecure password. - with mock.patch.object(account, 'pwned_password') as pwned_password: + with mock.patch.object(auth, 'pwned_password') as pwned_password: pwned_password.return_value = 2022 response = self.client.post('/accounts/login/', self.form_data) pwned_password.assert_called_once_with('football') @@ -96,7 +96,7 @@ def test_previously_secure_password(self): factories.PasswordQualityFactory.create( participant=participant, is_insecure=True ) - with mock.patch.object(account, 'pwned_password') as pwned_password: + with mock.patch.object(auth, 'pwned_password') as pwned_password: pwned_password.return_value = 15 # password found in 15 separate breaches! with mock.patch.object(account.logger, 'info') as logger_info: response = self._login() @@ -122,7 +122,7 @@ def test_user_without_participant(self): """Users may log in without having an associated participant!""" self.assertIsNone(models.Participant.from_user(self.user)) - with mock.patch.object(account, 'pwned_password') as pwned_password: + with mock.patch.object(auth, 'pwned_password') as pwned_password: pwned_password.return_value = 1 # password found in 1 breach! with mock.patch.object(account.logger, 'info') as logger_info: response = self._login() @@ -148,7 +148,7 @@ def test_password_is_marked_secure(self): """If the login's password returned no breaches, then we can mark it secure.""" factories.ParticipantFactory.create(user_id=self.user.pk) - with mock.patch.object(account, 'pwned_password') as pwned_password: + with mock.patch.object(auth, 'pwned_password') as pwned_password: pwned_password.return_value = 0 # (0 breaches total) response = self.client.post('/accounts/login/', self.form_data) pwned_password.assert_called_once_with('football') @@ -167,7 +167,7 @@ def test_password_is_marked_secure(self): def test_redirect_should_be_preserved(self): """If attempting to login with a redirect, it should be preserved!.""" - with mock.patch.object(account, 'pwned_password') as pwned_password: + with mock.patch.object(auth, 'pwned_password') as pwned_password: pwned_password.return_value = 1 # Password has been breached once! response = self.client.post( '/accounts/login/?next=/preferences/lottery/', self.form_data @@ -182,7 +182,7 @@ def test_password_over_999_times(self): """Test a quick hack for often-breached passwords that include commas in the count.""" participant = factories.ParticipantFactory.create(user_id=self.user.pk) - with mock.patch.object(account, 'pwned_password') as pwned_password: + with mock.patch.object(auth, 'pwned_password') as pwned_password: pwned_password.side_effect = lambda pw: int("1,234") self._login() pwned_password.assert_called_once_with('football') @@ -194,7 +194,7 @@ def test_uncaught_error_with_django_hipb(self): """We don't block login if there was an unhandled error.""" participant = factories.ParticipantFactory.create(user_id=self.user.pk) - with mock.patch.object(account, 'pwned_password') as pwned_password: + with mock.patch.object(auth, 'pwned_password') as pwned_password: pwned_password.side_effect = RuntimeError self._login() pwned_password.assert_called_once_with('football') diff --git a/ws/views/account.py b/ws/views/account.py index cb6c0774..36e23e7d 100644 --- a/ws/views/account.py +++ b/ws/views/account.py @@ -2,7 +2,6 @@ Views relating to account management. """ import logging -import re from typing import Optional, Tuple from urllib.parse import urlencode @@ -12,22 +11,14 @@ from django.http import HttpResponse from django.urls import reverse from django.utils.decorators import method_decorator -from pwned_passwords_django.api import pwned_password -from ws import models, settings +from ws import models +from ws.auth import times_seen_in_hibp from ws.utils.dates import local_now logger = logging.getLogger(__name__) -# As of 2022-08-27, pwned_password sometimes includes commas in `times` -# This hack can go away once the library is updated to handle commas -# https://github.com/ubernostrum/pwned-passwords-django/issues/35 -INT_WITH_COMMA = re.compile( - r"invalid literal for int\(\) with base 10: '(?P[\d,]+)'" -) - - class CustomPasswordChangeView(PasswordChangeView): """Custom password change view that makes two key changes: @@ -76,25 +67,6 @@ class CheckIfPwnedOnLoginView(LoginView): of breached passwords - we should check on every login. """ - @staticmethod - def _times_seen(password: str) -> Optional[int]: - """Return times password has been seen in HIBP.""" - if settings.DEBUG and password in settings.WHITELISTED_BAD_PASSWORDS: - return 0 - - try: - return pwned_password(password) - except ValueError as err: - has_comma = INT_WITH_COMMA.match(str(err)) - if not has_comma: - raise - return int(has_comma.group('times').replace(',', '')) - except Exception: # pylint: disable=broad-except - # Let Sentry know we had problems, but don't break the flow. - # Sentry should scrub `password` automatically. - logger.exception("Encountered an error with django-pwned-passwords") - return None - def _form_valid_perform_login(self, form) -> Tuple[Optional[int], HttpResponse]: """Performs login with a correct username/password. @@ -103,7 +75,7 @@ def _form_valid_perform_login(self, form) -> Tuple[Optional[int], HttpResponse]: As a side effect, this populates `self.request.user` """ - times_seen = self._times_seen(form.cleaned_data['password']) + times_seen = times_seen_in_hibp(form.cleaned_data['password']) if times_seen: change_password_url = reverse('account_change_password')