From da63e570e55889ceb119c4a8276b6886e8035b32 Mon Sep 17 00:00:00 2001 From: catborise Date: Mon, 5 Oct 2026 20:06:02 +0300 Subject: [PATCH] fix(accounts): the "Lost OTP?" page cannot be used to flood or break /accounts/email_otp/ mails a user their OTP QR code and needs no login. Measured with the default settings (OTP disabled): - It was reachable although OTP was off, and created OTP devices. - Every request sent a mail: 20 requests for one address, 20 mails, so anyone who knew an address could fill that mailbox. - An address shared by two users, or a mail server that could not be reached, ended in an error page (500). - Inactive users were mailed too. The page now exists only while OTP_ENABLED is set. It mails a single active user whose address matches (case-insensitively), at most once every 5 minutes per user (Django's cache: with several workers the bound is per worker), and logs a failed delivery. The answer is the same in every case, so it does not reveal whether an address belongs to a user. The QR codes come from django-qr-code (segno); both the mail and the account page render them without the qrcode package removed earlier. The new tests fail on the previous code. --- accounts/test_email_otp.py | 61 ++++++++++++++++++++++++++++++++++++++ accounts/views.py | 30 +++++++++++++------ 2 files changed, 82 insertions(+), 9 deletions(-) create mode 100644 accounts/test_email_otp.py diff --git a/accounts/test_email_otp.py b/accounts/test_email_otp.py new file mode 100644 index 00000000..d6001eff --- /dev/null +++ b/accounts/test_email_otp.py @@ -0,0 +1,61 @@ +"""The "Lost OTP?" page mails a user their OTP QR code without a login. It +exists only while OTP is enabled, mails one exact match at most once per +interval, and never fails with an error page.""" + +from django.contrib.auth import get_user_model +from django.core import mail +from django.core.cache import cache +from django.test import TestCase, override_settings +from django.urls import reverse + +URL = reverse("accounts:email_otp") + + +@override_settings( + OTP_ENABLED=True, + EMAIL_BACKEND="django.core.mail.backends.locmem.EmailBackend", + CACHES={"default": {"BACKEND": "django.core.cache.backends.locmem.LocMemCache", "LOCATION": "email-otp-tests"}}, +) +class EmailOTPTests(TestCase): + def setUp(self): + cache.clear() + self.user = get_user_model().objects.create_user("otp-user", email="otp.user@example.com", password="x") + + def post(self, email): + response = self.client.post(URL, {"email": email}) + self.assertRedirects(response, reverse("accounts:login"), fetch_redirect_response=False) + return response + + def test_one_mail_to_the_user(self): + self.post("OTP.User@Example.com") # addresses match case-insensitively + self.assertEqual([m.to for m in mail.outbox], [["otp.user@example.com"]]) + + def test_repeated_requests_send_one_mail_per_interval(self): + for _ in range(20): + self.post("otp.user@example.com") + self.assertEqual(len(mail.outbox), 1) + + def test_an_address_of_two_users_sends_nothing(self): + get_user_model().objects.create_user("otp-twin", email="otp.user@example.com", password="x") + self.post("otp.user@example.com") + self.assertEqual(mail.outbox, []) + + def test_unknown_and_inactive_addresses_send_nothing(self): + get_user_model().objects.create_user("otp-gone", email="gone@example.com", password="x", is_active=False) + self.post("nobody@example.com") + self.post("gone@example.com") + self.assertEqual(mail.outbox, []) + + @override_settings(EMAIL_BACKEND="django.core.mail.backends.smtp.EmailBackend", EMAIL_HOST="127.0.0.1", EMAIL_PORT=1) + def test_a_mail_server_that_is_down_is_no_error_page(self): + with self.assertLogs("accounts.views", level="ERROR"): + self.post("otp.user@example.com") + + +@override_settings(OTP_ENABLED=False) +class EmailOTPDisabledTests(TestCase): + def test_the_page_does_not_exist_without_otp(self): + get_user_model().objects.create_user("otp-off", email="off@example.com", password="x") + self.assertEqual(self.client.get(URL).status_code, 404) + self.assertEqual(self.client.post(URL, {"email": "off@example.com"}).status_code, 404) + self.assertEqual(mail.outbox, []) diff --git a/accounts/views.py b/accounts/views.py index 8cb386bd..f5d6feac 100644 --- a/accounts/views.py +++ b/accounts/views.py @@ -11,7 +11,8 @@ from django.core.exceptions import PermissionDenied from django.db import transaction from django.shortcuts import get_object_or_404, redirect, render from django.urls import reverse -from django.http import HttpResponseRedirect +from django.core.cache import cache +from django.http import Http404, HttpResponseRedirect from django.utils.http import url_has_allowed_host_and_scheme from django.utils.translation import gettext_lazy as _ from django.views.decorators.http import require_POST @@ -201,18 +202,29 @@ def user_instance_delete(request, pk): ) +# a user gets at most one OTP mail per this many seconds +EMAIL_OTP_INTERVAL = 300 + + @login_not_required def email_otp(request): + """ + "Lost OTP?": mail the OTP QR code to the user with this address. Open to + anyone, so it mails only one exact, active match, at most once per + EMAIL_OTP_INTERVAL, and answers the same in every case. + """ + if not settings.OTP_ENABLED: + raise Http404 form = EmailOTPForm(request.POST or None) if form.is_valid(): - UserModel = get_user_model() - try: - user = UserModel.objects.get(email=form.cleaned_data["email"]) - except UserModel.DoesNotExist: - pass - else: - device = get_user_totp_device(user) - send_email_with_otp(user, device) + users = list(get_user_model().objects.filter(email__iexact=form.cleaned_data["email"], is_active=True)[:2]) + # the cache is per process: with several workers the bound is per worker + if len(users) == 1 and cache.add(f"email_otp:{users[0].pk}", True, EMAIL_OTP_INTERVAL): + user = users[0] + try: + send_email_with_otp(user, get_user_totp_device(user)) + except Exception: + logging.getLogger(__name__).exception("Could not mail the OTP QR code to user %s", user.pk) messages.success( request, _("OTP Sent to %(email)s") % {"email": form.cleaned_data["email"]}