From 81f853527a3130b2661aab2a370f65c90472cca4 Mon Sep 17 00:00:00 2001 From: Oleksander Piskun Date: Mon, 18 May 2026 17:30:20 +0000 Subject: [PATCH 1/3] fix: rate-limit /account/ email change and reject empty emails Signed-off-by: Oleksander Piskun --- nextcloudappstore/settings/base.py | 3 + nextcloudappstore/user/forms.py | 5 +- nextcloudappstore/user/tests.py | 184 ++++++++++++++++++++++++++++- nextcloudappstore/user/views.py | 29 ++++- 4 files changed, 214 insertions(+), 7 deletions(-) diff --git a/nextcloudappstore/settings/base.py b/nextcloudappstore/settings/base.py index 40725ec87c0..fcf913d62f6 100644 --- a/nextcloudappstore/settings/base.py +++ b/nextcloudappstore/settings/base.py @@ -134,6 +134,9 @@ ACCOUNT_RATE_LIMITS = { "reset_password": "1/m/ip", "login_failed": "10/h/ip", + # Shared by /account/ (custom AccountView) and allauth's /accounts/email/. + # Tighter than allauth's default (10/m/user) to mitigate confirmation-mail spam. + "manage_email": "3/h/user", } SITE_ID = 1 diff --git a/nextcloudappstore/user/forms.py b/nextcloudappstore/user/forms.py index 1acec8d97f7..fc35bbfbf8e 100644 --- a/nextcloudappstore/user/forms.py +++ b/nextcloudappstore/user/forms.py @@ -3,6 +3,7 @@ SPDX-License-Identifier: AGPL-3.0-or-later """ +from allauth.account.adapter import get_adapter from allauth.account.utils import ( filter_users_by_email, ) @@ -96,7 +97,9 @@ def __init__(self, *args, **kwargs): self.fields["subscribe_to_news"].initial = self.user.profile.subscribe_to_news def clean_email(self): - value = self.cleaned_data["email"] + # Defer to the adapter so empty/invalid emails are rejected the same way + # allauth's AddEmailForm rejects them at /accounts/email/. + value = get_adapter().clean_email(self.cleaned_data["email"]) users = filter_users_by_email(value) if [u for u in users if u.pk != self.instance.pk]: msg = _("This email address is already associated with another account.") diff --git a/nextcloudappstore/user/tests.py b/nextcloudappstore/user/tests.py index cd214df8925..2d267479962 100644 --- a/nextcloudappstore/user/tests.py +++ b/nextcloudappstore/user/tests.py @@ -1,8 +1,186 @@ """ -SPDX-FileCopyrightText: 2017 Nextcloud GmbH and Nextcloud contributors +SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors SPDX-License-Identifier: AGPL-3.0-or-later """ -from django.test import TestCase # noqa +from unittest.mock import patch -# Create your tests here. +from allauth.account.models import EmailAddress +from allauth.core import ratelimit +from django.contrib.auth import get_user_model +from django.core.cache import cache +from django.test import RequestFactory, TestCase, override_settings +from django.urls import reverse +from email_validator import validate_email as _real_validate_email + +from nextcloudappstore.user.facades import create_user + +PASSWORD = "Sup3rS3cret!" +PRIMARY_EMAIL = "user@example.com" + + +def _validate_email_no_dns(email, **kwargs): + # Run the real format/empty checks but skip the DNS-deliverability lookup + # so tests don't depend on external resolvers. + kwargs["check_deliverability"] = False + return _real_validate_email(email, **kwargs) + + +@override_settings( + ACCOUNT_RATE_LIMITS={ + "reset_password": "1/m/ip", + "login_failed": "10/h/ip", + "manage_email": "3/h/user", + }, +) +class AccountEmailChangeRateLimitTest(TestCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + cls._validate_patcher = patch( + "nextcloudappstore.user.adapters.validate_email", + side_effect=_validate_email_no_dns, + ) + cls._validate_patcher.start() + + @classmethod + def tearDownClass(cls): + cls._validate_patcher.stop() + super().tearDownClass() + + def setUp(self): + cache.clear() + self.user = create_user(username="testuser", password=PASSWORD, email=PRIMARY_EMAIL) + self.client.login(username="testuser", password=PASSWORD) + self.url = reverse("user:account") + + def _post(self, **overrides): + data = { + "first_name": "Test", + "last_name": "User", + "email": PRIMARY_EMAIL, + "subscribe_to_news": False, + "passwd": PASSWORD, + } + data.update(overrides) + return self.client.post(self.url, data) + + def test_no_email_change_does_not_consume_budget(self): + for i in range(10): + response = self._post(first_name=f"Name{i}") + self.assertEqual(response.status_code, 302, f"iteration {i}") + + def test_first_email_change_within_limit_succeeds(self): + response = self._post(email="new1@nextcloud.com") + self.assertEqual(response.status_code, 302) + self.assertTrue( + EmailAddress.objects.filter(user=self.user, email="new1@nextcloud.com", verified=False).exists() + ) + + def test_email_change_blocked_after_limit_exceeded(self): + for i in range(1, 4): + response = self._post(email=f"new{i}@nextcloud.com") + self.assertEqual(response.status_code, 302, f"iteration {i}") + + response = self._post(email="new4@nextcloud.com") + self.assertEqual(response.status_code, 200) + self.assertIn("email", response.context["form"].errors) + self.assertFalse(EmailAddress.objects.filter(email="new4@nextcloud.com").exists()) + + def test_user_email_unchanged_when_rate_limited(self): + for i in range(1, 4): + self._post(email=f"new{i}@nextcloud.com") + before = get_user_model().objects.get(pk=self.user.pk).email + + self._post(email="new4@nextcloud.com") + after = get_user_model().objects.get(pk=self.user.pk).email + + self.assertEqual(after, before) + self.assertNotEqual(after, "new4@nextcloud.com") + + def test_name_changes_persist_when_email_rate_limited(self): + for i in range(1, 4): + self._post(email=f"new{i}@nextcloud.com") + + self._post( + email="new4@nextcloud.com", + first_name="NewFirst", + last_name="NewLast", + ) + + updated = get_user_model().objects.get(pk=self.user.pk) + self.assertEqual(updated.first_name, "NewFirst") + self.assertEqual(updated.last_name, "NewLast") + + def test_rate_limit_independent_from_reset_password(self): + for i in range(1, 4): + self._post(email=f"new{i}@nextcloud.com") + + request = RequestFactory().post(self.url) + self.assertTrue(ratelimit.consume(request, action="reset_password")) + + def test_invalid_form_does_not_consume_budget(self): + for i in range(5): + response = self._post( + email=f"newX{i}@nextcloud.com", + passwd="wrong-password", # nosec + ) + self.assertEqual(response.status_code, 200) + self.assertTrue(response.context["form"].errors) + + for i in range(1, 4): + response = self._post(email=f"valid{i}@nextcloud.com") + self.assertEqual(response.status_code, 302, f"iteration {i}") + + def test_same_email_submission_does_not_consume_budget(self): + for _ in range(10): + response = self._post() + self.assertEqual(response.status_code, 302) + + for i in range(1, 4): + response = self._post(email=f"changed{i}@nextcloud.com") + self.assertEqual(response.status_code, 302, f"iteration {i}") + + def test_case_only_email_change_does_not_consume_budget(self): + # allauth stores EmailAddress.email lowercased and add_email() lowercases + # on the way in, so submitting the same address in different case is a + # silent no-op. The rate-limit budget must reflect that. + for _ in range(10): + response = self._post(email=PRIMARY_EMAIL.upper()) + self.assertEqual(response.status_code, 302) + for i in range(1, 4): + response = self._post(email=f"changed{i}@nextcloud.com") + self.assertEqual(response.status_code, 302, f"iteration {i}") + + def test_odoo_called_with_actual_email_when_rate_limited(self): + for i in range(1, 4): + self._post(email=f"new{i}@nextcloud.com") + + with patch("nextcloudappstore.user.models.subscribe_user_to_news") as mock_sub: + # Flipping subscribe_to_news triggers the pre_save handler in + # signals.py; it must not be passed the rejected new email. + self._post(email="new4@nextcloud.com", subscribe_to_news=True) + + if mock_sub.called: + called_with_email = mock_sub.call_args[0][0] + self.assertNotEqual(called_with_email, "new4@nextcloud.com") + + def test_empty_email_rejected_by_form(self): + response = self._post(email="") + self.assertEqual(response.status_code, 200) + self.assertIn("email", response.context["form"].errors) + self.assertFalse(EmailAddress.objects.filter(email="").exists()) + + def test_malformed_email_rejected_by_form(self): + response = self._post(email="not-an-email") + self.assertEqual(response.status_code, 200) + self.assertIn("email", response.context["form"].errors) + + def test_budget_shared_with_allauth_email_view(self): + for i in range(1, 4): + r = self._post(email=f"viaaccount{i}@nextcloud.com") + self.assertEqual(r.status_code, 302, f"iteration {i}") + + email_url = reverse("account_email") + r = self.client.post(email_url, {"email": "viaemail@nextcloud.com", "action_add": ""}) + self.assertEqual(r.status_code, 429) diff --git a/nextcloudappstore/user/views.py b/nextcloudappstore/user/views.py index 8ebc1b133e6..d7e9ef2ef04 100644 --- a/nextcloudappstore/user/views.py +++ b/nextcloudappstore/user/views.py @@ -5,6 +5,7 @@ from allauth.account.models import EmailAddress from allauth.account.views import PasswordChangeView +from allauth.core import ratelimit from django.contrib import messages from django.contrib.auth import logout from django.contrib.auth.mixins import LoginRequiredMixin @@ -13,6 +14,7 @@ from django.shortcuts import get_object_or_404, redirect, render from django.urls import reverse, reverse_lazy from django.utils.decorators import method_decorator +from django.utils.translation import gettext_lazy as _ from django.views.decorators.cache import never_cache from django.views.generic import TemplateView, UpdateView @@ -139,12 +141,33 @@ def form_invalid(self, form): return super().form_invalid(form) def form_valid(self, form): - message = "Account details saved." - user = self.request.user + # Compare case-insensitively: allauth stores EmailAddress.email lowercased + # and add_email() also lowercases. A case-only "change" is a no-op there, + # so it must not consume the rate-limit budget either. current_email = EmailAddress.objects.get_primary(user=user).email new_email = form.cleaned_data["email"] - if new_email != current_email: + email_changed = new_email.lower() != current_email.lower() + + if email_changed and not ratelimit.consume(self.request, action="manage_email"): + # Persist name/subscription changes but skip the email change and + # surface an inline error. User.email is intentionally not updated. + # Construct_instance set user.email to the rejected new email in + # memory; refresh it so the profile pre_save signal handler + # (signals.handle_subscription_change) does not leak that value to + # Odoo via instance.user.email when subscribe_to_news flips. + user.refresh_from_db(fields=["email"]) + user.first_name = form.cleaned_data["first_name"] + user.last_name = form.cleaned_data["last_name"] + user.save(update_fields=["first_name", "last_name"]) + user.profile.subscribe_to_news = form.cleaned_data["subscribe_to_news"] + user.profile.save(update_fields=["subscribe_to_news"]) + form.add_error("email", _("Too many email change attempts. Please try again later.")) + self.request.session["account_update_failed_count"] = 0 + return self.render_to_response(self.get_context_data(form=form)) + + message = "Account details saved." + if email_changed: # Delete other unconfirmed email addresses EmailAddress.objects.filter(user=user).exclude(primary=True).delete() # Add new email address, send confirmation email From 220819379b75424d8bab1edf3566cde7f6f9cecf Mon Sep 17 00:00:00 2001 From: Edward Ly Date: Wed, 20 May 2026 20:57:27 -0700 Subject: [PATCH 2/3] fix(user): return HTTP 429 status when rate limit on emails is exceeded Signed-off-by: Edward Ly --- nextcloudappstore/user/tests.py | 2 +- nextcloudappstore/user/views.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/nextcloudappstore/user/tests.py b/nextcloudappstore/user/tests.py index 2d267479962..27a76780d02 100644 --- a/nextcloudappstore/user/tests.py +++ b/nextcloudappstore/user/tests.py @@ -83,7 +83,7 @@ def test_email_change_blocked_after_limit_exceeded(self): self.assertEqual(response.status_code, 302, f"iteration {i}") response = self._post(email="new4@nextcloud.com") - self.assertEqual(response.status_code, 200) + self.assertEqual(response.status_code, 429) self.assertIn("email", response.context["form"].errors) self.assertFalse(EmailAddress.objects.filter(email="new4@nextcloud.com").exists()) diff --git a/nextcloudappstore/user/views.py b/nextcloudappstore/user/views.py index d7e9ef2ef04..0f237f6defc 100644 --- a/nextcloudappstore/user/views.py +++ b/nextcloudappstore/user/views.py @@ -164,7 +164,7 @@ def form_valid(self, form): user.profile.save(update_fields=["subscribe_to_news"]) form.add_error("email", _("Too many email change attempts. Please try again later.")) self.request.session["account_update_failed_count"] = 0 - return self.render_to_response(self.get_context_data(form=form)) + return self.render_to_response(self.get_context_data(form=form), status=429) message = "Account details saved." if email_changed: From 2579b3a7a604d999ac824588d1bcca0d88f90c1b Mon Sep 17 00:00:00 2001 From: Edward Ly Date: Fri, 22 May 2026 08:16:05 -0700 Subject: [PATCH 3/3] fix: poetry update, temporarily revert python to 3.12 Signed-off-by: Edward Ly --- poetry.lock | 49 +++++++++++++++++++++++-------------------------- pyproject.toml | 2 +- 2 files changed, 24 insertions(+), 27 deletions(-) diff --git a/poetry.lock b/poetry.lock index da01d555243..d0eec63cf09 100644 --- a/poetry.lock +++ b/poetry.lock @@ -69,13 +69,13 @@ css = ["tinycss2 (>=1.1.0,<1.5)"] [[package]] name = "certifi" -version = "2026.4.22" +version = "2026.5.20" description = "Python package for providing Mozilla's CA Bundle." optional = false python-versions = ">=3.7" files = [ - {file = "certifi-2026.4.22-py3-none-any.whl", hash = "sha256:3cb2210c8f88ba2318d29b0388d1023c8492ff72ecdde4ebdaddbb13a31b1c4a"}, - {file = "certifi-2026.4.22.tar.gz", hash = "sha256:8d455352a37b71bf76a79caa83a3d6c25afee4a385d632127b6afb3963f1c580"}, + {file = "certifi-2026.5.20-py3-none-any.whl", hash = "sha256:3c52e209ba0a4ad7aebe60436a4ab349c39e1e602e8c134221e546902ad25897"}, + {file = "certifi-2026.5.20.tar.gz", hash = "sha256:69dea482ab64caa7b9f6aba1c6bf48bb6a5448d1c0f1b17ab42ad8c763a5344d"}, ] [[package]] @@ -538,13 +538,13 @@ bcrypt = ["bcrypt"] [[package]] name = "django-allauth" -version = "65.16.1" +version = "65.17.0" description = "Integrated set of Django applications addressing authentication, registration, account management as well as 3rd party (social) account authentication." optional = false python-versions = ">=3.10" files = [ - {file = "django_allauth-65.16.1-py3-none-any.whl", hash = "sha256:e49df24056bf37c44e56aaad1e51f78994b7d175bc3476d65e8f8f58390a8ce8"}, - {file = "django_allauth-65.16.1.tar.gz", hash = "sha256:4425ac3088541c4c54983e16e08f6e3eb9f438dc1b1009534fa51c8bb739ed31"}, + {file = "django_allauth-65.17.0-py3-none-any.whl", hash = "sha256:ff041190bc6f9b2854aa5d7215a1f64f1fc8eb0907990690daad19144b7d2757"}, + {file = "django_allauth-65.17.0.tar.gz", hash = "sha256:72bea057eaaf2952e4e22cd0195e031ef8e7de2cbac065cf35bd0067ef50a1e7"}, ] [package.dependencies] @@ -885,13 +885,13 @@ files = [ [[package]] name = "idna" -version = "3.15" +version = "3.16" description = "Internationalized Domain Names in Applications (IDNA)" optional = false -python-versions = ">=3.8" +python-versions = ">=3.9" files = [ - {file = "idna-3.15-py3-none-any.whl", hash = "sha256:048adeaf8c2d788c40fee287673ccaa74c24ffd8dcf09ffa555a2fbb59f10ac8"}, - {file = "idna-3.15.tar.gz", hash = "sha256:ca962446ea538f7092a95e057da437618e886f4d349216d2b1e294abfdb65fdc"}, + {file = "idna-3.16-py3-none-any.whl", hash = "sha256:cc246e3a3f89580c3a951b5ad298ca4638078b2cdd4f115654332b5c26daded5"}, + {file = "idna-3.16.tar.gz", hash = "sha256:d7a6da03db833450fca25d2358ac9ff06cd624577a4aea3a596d5c0f77b8e03d"}, ] [package.extras] @@ -899,13 +899,13 @@ all = ["mypy (>=1.11.2)", "pytest (>=8.3.2)", "ruff (>=0.6.2)"] [[package]] name = "imagesize" -version = "1.5.0" -description = "Getting image size from png/jpeg/jpeg2000/gif file" +version = "2.0.0" +description = "Get image size from headers (BMP/PNG/JPEG/JPEG2000/GIF/TIFF/SVG/Netpbm/WebP/AVIF/HEIC/HEIF)" optional = false -python-versions = "!=3.0.*,!=3.1.*,!=3.2.*,!=3.3.*,>=2.7" +python-versions = "<3.15,>=3.10" files = [ - {file = "imagesize-1.5.0-py2.py3-none-any.whl", hash = "sha256:32677681b3f434c2cb496f00e89c5a291247b35b1f527589909e008057da5899"}, - {file = "imagesize-1.5.0.tar.gz", hash = "sha256:8bfc5363a7f2133a89f0098451e0bcb1cd71aba4dc02bbcecb39d99d40e1b94f"}, + {file = "imagesize-2.0.0-py2.py3-none-any.whl", hash = "sha256:5667c5bbb57ab3f1fa4bc366f4fbc971db3d5ed011fd2715fd8001f782718d96"}, + {file = "imagesize-2.0.0.tar.gz", hash = "sha256:8e8358c4a05c304f1fccf7ff96f036e7243a189e9e42e90851993c558cfe9ee3"}, ] [[package]] @@ -1534,13 +1534,13 @@ windows-terminal = ["colorama (>=0.4.6)"] [[package]] name = "pyjwt" -version = "2.12.1" +version = "2.13.0" description = "JSON Web Token implementation in Python" optional = false python-versions = ">=3.9" files = [ - {file = "pyjwt-2.12.1-py3-none-any.whl", hash = "sha256:28ca37c070cad8ba8cd9790cd940535d40274d22f80ab87f3ac6a713e6e8454c"}, - {file = "pyjwt-2.12.1.tar.gz", hash = "sha256:c74a7a2adf861c04d002db713dd85f84beb242228e671280bf709d765b03672b"}, + {file = "pyjwt-2.13.0-py3-none-any.whl", hash = "sha256:66adcc2aff09b3f1bbd95fc1e1577df8ac8723c978552fd43304c8a290ac5728"}, + {file = "pyjwt-2.13.0.tar.gz", hash = "sha256:41571c89ca91598c79e8ef18a2d07367d4810fbbd6f637794879baf1b7703423"}, ] [package.dependencies] @@ -1548,9 +1548,6 @@ cryptography = {version = ">=3.4.0", optional = true, markers = "extra == \"cryp [package.extras] crypto = ["cryptography (>=3.4.0)"] -dev = ["coverage[toml] (==7.10.7)", "cryptography (>=3.4.0)", "pre-commit", "pytest (>=8.4.2,<9.0.0)", "sphinx", "sphinx-rtd-theme", "zope.interface"] -docs = ["sphinx", "sphinx-rtd-theme", "zope.interface"] -tests = ["coverage[toml] (==7.10.7)", "pytest (>=8.4.2,<9.0.0)"] [[package]] name = "pymple" @@ -1771,13 +1768,13 @@ use-chardet-on-py3 = ["chardet (>=3.0.2,<8)"] [[package]] name = "responses" -version = "0.26.0" +version = "0.26.1" description = "A utility library for mocking out the `requests` Python library." optional = false python-versions = ">=3.8" files = [ - {file = "responses-0.26.0-py3-none-any.whl", hash = "sha256:03ec4409088cd5c66b71ecbbbd27fe2c58ddfad801c66203457b3e6a04868c37"}, - {file = "responses-0.26.0.tar.gz", hash = "sha256:c7f6923e6343ef3682816ba421c006626777893cb0d5e1434f674b649bac9eb4"}, + {file = "responses-0.26.1-py3-none-any.whl", hash = "sha256:8aacc4586eb08fb2208ef64a9eb4258d9b0c6e6f4260845f2f018ab847495345"}, + {file = "responses-0.26.1.tar.gz", hash = "sha256:2eb3218553cc8f79b57d257bac23af5e1bf381f5b9390b1767816f0843e01dc2"}, ] [package.dependencies] @@ -2224,5 +2221,5 @@ h11 = ">=0.16.0,<1" [metadata] lock-version = "2.0" -python-versions = "^3.12" -content-hash = "d1f36301d11f9158d33fb015ee4357e9051398d8da0b7079f591d62985a27fee" +python-versions = "~3.12" +content-hash = "3644fe2915bf3d6054666951b41114355a9c96955f0ba87179c525ab8b5347e3" diff --git a/pyproject.toml b/pyproject.toml index 16e2a762dc7..6212b354fbc 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -14,7 +14,7 @@ authors = [ "Alexander Piskun ", "Edward Ly ", ] -dependencies.python = "^3.12" +dependencies.python = "~3.12" dependencies.django = "~4.2.27" dependencies.django-braces = "~1.17.0" dependencies.djangorestframework-camel-case = "^1.3.0"