From dd67c814ef3c973fb00b4df697428246b11e5d41 Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Sat, 17 Oct 2026 09:31:26 -0400 Subject: [PATCH 01/10] Add dependency on disposable-email-domains --- pyproject.toml | 1 + uv.lock | 11 +++++++++++ 2 files changed, 12 insertions(+) diff --git a/pyproject.toml b/pyproject.toml index dc00dc86..41e5d69b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -6,6 +6,7 @@ dependencies = [ "bleach[css]", "boto3", "chartkick", + "disposable-email-domains>=0.0.281", "django", "django-countries", "django-filter", diff --git a/uv.lock b/uv.lock index 0bc26942..f1eb256d 100644 --- a/uv.lock +++ b/uv.lock @@ -201,6 +201,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/b5/a5/36ebf05de5ae042cc10923f8d78189364076ed5c3f58f0dfe1e639d3ef10/cssbeautifier-2.0.3-py3-none-any.whl", hash = "sha256:14650d9c8a47adcb80a7697a866a3921fe0332ef74a92e1039d09086aa0c5001", size = 122645, upload-time = "2026-06-30T15:41:10.519Z" }, ] +[[package]] +name = "disposable-email-domains" +version = "0.0.281" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/03/7e/c4c7f9672e7c2ef813d79dedc70a0b1226b124cf48f5ca420fc9475f5dd8/disposable_email_domains-0.0.281.tar.gz", hash = "sha256:9fc890a1708c17399b7125de8f67624541601918bb5f3545d14d552151f39d4b", size = 119652, upload-time = "2026-10-16T17:02:52.27Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/7b/5d/75be3e22fd06dc0cc692bde8d23ad341ca9c1b3305e9ab0e15a9252ffb66/disposable_email_domains-0.0.281-py3-none-any.whl", hash = "sha256:872cdbce38e163788cc7d699256c2a2c9ad9f5dba2a088cfc3951522377765d9", size = 61473, upload-time = "2026-10-16T17:02:50.643Z" }, +] + [[package]] name = "distlib" version = "0.4.3" @@ -668,6 +677,7 @@ dependencies = [ { name = "bleach", extra = ["css"] }, { name = "boto3" }, { name = "chartkick" }, + { name = "disposable-email-domains" }, { name = "django" }, { name = "django-countries" }, { name = "django-filter" }, @@ -714,6 +724,7 @@ requires-dist = [ { name = "bleach", extras = ["css"] }, { name = "boto3" }, { name = "chartkick" }, + { name = "disposable-email-domains", specifier = ">=0.0.281" }, { name = "django" }, { name = "django-countries" }, { name = "django-filter" }, From 5c93e99eec109f3e9960003063255a0fa61ea5cc Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Sat, 17 Oct 2026 09:33:14 -0400 Subject: [PATCH 02/10] Add env variable for UserCheck Drop RAPID_API_* env variables and USERCHECK_API_KEY --- .env.example | 5 ++--- .github/workflows/ci.yml | 3 +-- metron.env.example | 5 ++--- metron/settings.py | 3 +-- 4 files changed, 6 insertions(+), 10 deletions(-) diff --git a/.env.example b/.env.example index 634ca2b0..40250d38 100644 --- a/.env.example +++ b/.env.example @@ -48,10 +48,9 @@ PUSHOVER_TOKEN= # Comma-separated to notify multiple users/groups, e.g. , PUSHOVER_USER_KEY= -# ── hCaptcha / Rapid API ─────────────────────────────────────────────────────── +# ── hCaptcha / UserCheck ─────────────────────────────────────────────────────── HCAPTCHA_SECRET_KEY= -RAPID_API_KEY= -RAPID_API_HOST= +USERCHECK_API_KEY= # ── OpenCollective donor sync ────────────────────────────────────────────────── OPENCOLLECTIVE_API_KEY= diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 81df6d9f..6f707919 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -69,8 +69,7 @@ jobs: PUSHOVER_TOKEN: "dummy" PUSHOVER_USER_KEY: "dummy" HCAPTCHA_SECRET_KEY: "dummy" - RAPID_API_KEY: "dummy" - RAPID_API_HOST: "dummy" + USERCHECK_API_KEY: "dummy" OPENCOLLECTIVE_API_KEY: "dummy" STATIC_ROOT: "/tmp/static" MEDIA_ROOT: "/tmp/media" diff --git a/metron.env.example b/metron.env.example index 1bea6524..6d3725db 100644 --- a/metron.env.example +++ b/metron.env.example @@ -46,10 +46,9 @@ EMAIL_FROM= PUSHOVER_TOKEN= PUSHOVER_USER_KEY= -# ── hCaptcha / Rapid API ─────────────────────────────────────────────────────── +# ── hCaptcha / UserCheck ─────────────────────────────────────────────────────── HCAPTCHA_SECRET_KEY= -RAPID_API_KEY= -RAPID_API_HOST= +USERCHECK_API_KEY= # ── OpenCollective donor sync ────────────────────────────────────────────────── OPENCOLLECTIVE_API_KEY= diff --git a/metron/settings.py b/metron/settings.py index d8444c34..84027f0c 100644 --- a/metron/settings.py +++ b/metron/settings.py @@ -368,8 +368,7 @@ } HCAPTCHA_SECRET_KEY = config("HCAPTCHA_SECRET_KEY") -RAPID_API_KEY = config("RAPID_API_KEY") -RAPID_API_HOST = config("RAPID_API_HOST") +USERCHECK_API_KEY = config("USERCHECK_API_KEY") OPENCOLLECTIVE_API_KEY = config("OPENCOLLECTIVE_API_KEY") OPENCOLLECTIVE_SLUG = config("OPENCOLLECTIVE_SLUG", default="metron") From 3680fd5291edcd7c52999961bd24b32b8ce48eb3 Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Sat, 17 Oct 2026 09:35:49 -0400 Subject: [PATCH 03/10] Check signup emails with UserCheck and disposable-email-domains Replace the MailCheck lookup through RapidAPI with UserCheck's own API, and check the disposable-email-domains blocklist first so known throwaway domains are rejected without spending a credit. - Reject an address if its domain, or any domain above it, is on the hand-kept blocklist or the package's list. Domain matching now ignores case, so Duck.com no longer gets past the blocklist. - Reject disposable, spam and relay domains, and domains without MX records. When UserCheck has a typo fix, suggest it ("Did you mean...?"). - Cache UserCheck results per domain in Redis for 30 days to save credits. Only successful lookups are cached. - On a 429, retry once after a second to cover the 1 request/second limit. - Fail open when UserCheck can't answer: allow the address, log it and send at most one Pushover alert an hour. - Add Italian translations for the new messages. - Keep tests off the real API and give each test its own cache keys. Closes #656 --- locale/it/LC_MESSAGES/django.po | 11 ++- tests/conftest.py | 24 +++++ tests/users/test_email_change.py | 10 +- tests/users/test_email_validation.py | 90 ++++++++++++++++++ tests/users/test_login_throttle.py | 2 +- tests/users/test_utils.py | 135 +++++++++++++++++++++++---- users/forms.py | 62 ++++++++---- users/utils.py | 108 +++++++++++++++------ 8 files changed, 374 insertions(+), 68 deletions(-) create mode 100644 tests/users/test_email_validation.py diff --git a/locale/it/LC_MESSAGES/django.po b/locale/it/LC_MESSAGES/django.po index fbebbc32..869f1b0a 100644 --- a/locale/it/LC_MESSAGES/django.po +++ b/locale/it/LC_MESSAGES/django.po @@ -6515,9 +6515,14 @@ msgstr "L'email esiste già" msgid "Temporary email addresses are not allowed." msgstr "Gli indirizzi email temporanei non sono ammessi." -#: users/forms.py:61 -msgid "Error creating account. Contact the site administrator." -msgstr "Errore nella creazione dell'account. Contatta l'amministratore del sito." +#: users/forms.py +msgid "Email forwarding addresses are not allowed." +msgstr "Gli indirizzi email di inoltro non sono ammessi." + +#: users/forms.py +#, python-format +msgid "Did you mean %(suggestion)s?" +msgstr "Intendevi %(suggestion)s?" #: users/forms.py:78 msgid "Required. Enter a valid email address." diff --git a/tests/conftest.py b/tests/conftest.py index f66ecaa3..46db5b97 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -57,6 +57,30 @@ def isolated_login_throttle(): yield +CLEAN_EMAIL_VERDICT = { + "disposable": False, + "spam": False, + "relay_domain": False, + "mx": True, + "suggested_domain": None, +} + + +@pytest.fixture(autouse=True) +def isolated_usercheck(): + """Keep tests off the real UserCheck API, and give each test its own verdict + cache and alert flag in the shared Redis cache. Forms see a clean verdict for + any address not caught by the local blocklists; tests that need another verdict + re-patch users.forms.check_email_domain, and the inner patch takes precedence.""" + suffix = uuid.uuid4().hex + with ( + patch("users.utils.CACHE_PREFIX", f"usercheck:test:{suffix}"), + patch("users.utils.ALERT_KEY", f"usercheck:alerted:test:{suffix}"), + patch("users.forms.check_email_domain", return_value=CLEAN_EMAIL_VERDICT), + ): + yield + + @pytest.fixture(autouse=True) def isolated_api_throttle(): """Give each test its own DRF throttle history. It's keyed by user pk in the diff --git a/tests/users/test_email_change.py b/tests/users/test_email_change.py index e2b98287..12de0343 100644 --- a/tests/users/test_email_change.py +++ b/tests/users/test_email_change.py @@ -15,7 +15,7 @@ HTML_OK_CODE = 200 HTTP_REDIRECT_FOUND_CODE = 302 -NEW_EMAIL = "new-address@gmail.com" # Allowlisted domain: skips the RapidAPI check. +NEW_EMAIL = "new-address@gmail.com" # Allowlisted domain: skips the UserCheck lookup. def _profile_data(user, **overrides): @@ -138,7 +138,13 @@ def test_rejects_disposable_address(self, auto_login_user, test_password): def test_runs_domain_check_for_unlisted_domains(self, auto_login_user, test_password): client, user = auto_login_user() - blocked = {"block": True, "disposable": True} + blocked = { + "disposable": True, + "spam": False, + "relay_domain": False, + "mx": True, + "suggested_domain": None, + } with patch("users.forms.check_email_domain", return_value=blocked) as mock_check: resp = client.post( reverse("change_profile"), diff --git a/tests/users/test_email_validation.py b/tests/users/test_email_validation.py new file mode 100644 index 00000000..52601009 --- /dev/null +++ b/tests/users/test_email_validation.py @@ -0,0 +1,90 @@ +"""Tests for users.forms.validate_email_allowed, shared by signup and email changes.""" + +from unittest.mock import patch + +import pytest +from django.core.exceptions import ValidationError + +from users.forms import validate_email_allowed + +CHECK = "users.forms.check_email_domain" +CLEAN = { + "disposable": False, + "spam": False, + "relay_domain": False, + "mx": True, + "suggested_domain": None, +} + +pytestmark = pytest.mark.django_db + + +def _error(email, verdict=CLEAN): + with patch(CHECK, return_value=verdict) as mock_check: + try: + validate_email_allowed(email) + except ValidationError as exc: + return exc.messages[0], mock_check + return None, mock_check + + +@pytest.mark.parametrize( + "email", + [ + "x@mailinator.com", # disposable-email-domains package + "x@inbox.mailinator.com", # subdomain of a package-listed domain + "x@duck.com", # hand-kept blocklist + "x@Duck.COM", + ], +) +def test_local_blocklists_reject_without_lookup(email): + message, mock_check = _error(email) + assert message == "Temporary email addresses are not allowed." + mock_check.assert_not_called() + + +def test_allowlisted_domain_skips_lookup(): + message, mock_check = _error("x@Gmail.com") + assert message is None + mock_check.assert_not_called() + + +def test_clean_verdict_is_allowed(): + message, mock_check = _error("x@example.org") + assert message is None + mock_check.assert_called_once_with("x@example.org") + + +def test_fails_open_without_verdict(): + message, _mock = _error("x@example.org", verdict=None) + assert message is None + + +@pytest.mark.parametrize( + ("overrides", "expected"), + [ + ({"disposable": True}, "Temporary email addresses are not allowed."), + ({"spam": True}, "Temporary email addresses are not allowed."), + ({"relay_domain": True}, "Email forwarding addresses are not allowed."), + ({"mx": False}, "Email address is not valid."), + ({"mx": False, "suggested_domain": "gmail.com"}, "Did you mean x@gmail.com?"), + # An unknown MX result isn't grounds to reject. + ({"mx": None}, None), + # A suggestion alone doesn't block an address that can receive mail. + ({"suggested_domain": "gmail.com"}, None), + ], +) +def test_verdict_mapping(overrides, expected): + message, _mock = _error("x@example.org", verdict={**CLEAN, **overrides}) + assert message == expected + + +def test_malformed_address_is_rejected(): + message, _mock = _error("x@example.org", verdict={"invalid": True}) + assert message == "Email address is not valid." + + +def test_address_without_domain_is_rejected(): + message, mock_check = _error("no-at-sign") + assert message == "Email address is not valid." + mock_check.assert_not_called() diff --git a/tests/users/test_login_throttle.py b/tests/users/test_login_throttle.py index 572e15e2..34480073 100644 --- a/tests/users/test_login_throttle.py +++ b/tests/users/test_login_throttle.py @@ -14,7 +14,7 @@ HTML_OK_CODE = 200 HTTP_REDIRECT_FOUND_CODE = 302 LOCKED_TEXT = "Too many failed attempts" -NEW_EMAIL = "new-address@gmail.com" # Allowlisted domain: skips the RapidAPI check. +NEW_EMAIL = "new-address@gmail.com" # Allowlisted domain: skips the UserCheck lookup. def _login(client, username, password, ip="203.0.113.10"): diff --git a/tests/users/test_utils.py b/tests/users/test_utils.py index 9b689b9c..8560ddbd 100644 --- a/tests/users/test_utils.py +++ b/tests/users/test_utils.py @@ -8,50 +8,149 @@ from users.utils import check_email_domain HTTPS_CONNECTION = "users.utils.http.client.HTTPSConnection" +SLEEP = "users.utils.time.sleep" +PUSHOVER = "users.utils.send_pushover" +USERCHECK_BODY = { + "status": 200, + "email": "x@example.org", + "domain": "example.org", + "mx": True, + "disposable": False, + "public_domain": False, + "relay_domain": False, + "spam": False, + "did_you_mean": None, +} -def _mock_connection(status=200, body=None): + +def _mock_connection(*statuses, body=None): + """A connection whose successive responses have the given statuses (default: one 200).""" conn = MagicMock() - response = conn.getresponse.return_value - response.status = status - response.reason = "OK" if status == 200 else "Error" - response.read.return_value = json.dumps(body or {"block": False}).encode() + responses = [] + for status in statuses or (200,): + response = MagicMock() + response.status = status + response.reason = "OK" if status == 200 else "Error" + response.read.return_value = json.dumps( + (body or USERCHECK_BODY) if status == 200 else {"status": status, "error": "Error"} + ).encode() + responses.append(response) + conn.getresponse.side_effect = responses return conn @pytest.mark.parametrize( - ("email", "expected_query"), + ("email", "expected_path"), [ - ("someone@example.org", "domain=someone%40example.org"), - # A quoted local part may contain characters that are special in a query. - ('"a&b=c#d"@example.org', "domain=%22a%26b%3Dc%23d%22%40example.org"), - ('"first last"@example.org', "domain=%22first+last%22%40example.org"), + ("someone@example.org", "/email/someone%40example.org"), + # A quoted local part may contain characters that are special in a path. + ('"a/b?c#d"@example.org', "/email/%22a%2Fb%3Fc%23d%22%40example.org"), + ('"first last"@example.org', "/email/%22first%20last%22%40example.org"), ], ) -def test_check_email_domain_encodes_query(email, expected_query): +def test_check_email_domain_encodes_path(email, expected_path): conn = _mock_connection() with patch(HTTPS_CONNECTION, return_value=conn): check_email_domain(email) method, path = conn.request.call_args.args assert method == "GET" - assert path == f"/?{expected_query}" + assert path == expected_path + +def test_check_email_domain_sends_bearer_token(settings): + settings.USERCHECK_API_KEY = "secret" + conn = _mock_connection() + with patch(HTTPS_CONNECTION, return_value=conn): + check_email_domain("x@example.org") -def test_check_email_domain_returns_parsed_response(): - body = {"block": True, "disposable": True} + assert conn.request.call_args.kwargs["headers"] == {"Authorization": "Bearer secret"} + + +def test_check_email_domain_returns_normalized_verdict(): + body = { + **USERCHECK_BODY, + "disposable": True, + "relay_domain": True, + "mx": False, + "did_you_mean": "x@gmail.com", + } with patch(HTTPS_CONNECTION, return_value=_mock_connection(body=body)): - assert check_email_domain("x@example.org") == body + assert check_email_domain("x@gmial.com") == { + "disposable": True, + "spam": False, + "relay_domain": True, + "mx": False, + "suggested_domain": "gmail.com", + } + +def test_check_email_domain_caches_verdict_per_domain(): + conn = _mock_connection() + with patch(HTTPS_CONNECTION, return_value=conn) as mock_conn: + first = check_email_domain("x@example.org") + # Another mailbox on the same domain, differently cased, hits the cache. + second = check_email_domain("y@Example.ORG") + + assert first == second + mock_conn.assert_called_once() -def test_check_email_domain_returns_none_on_error_status(): - with patch(HTTPS_CONNECTION, return_value=_mock_connection(status=500)): + +def test_check_email_domain_does_not_cache_failures(): + with patch(HTTPS_CONNECTION, side_effect=[_mock_connection(500), _mock_connection()]): assert check_email_domain("x@example.org") is None + assert check_email_domain("x@example.org") is not None + + +def test_check_email_domain_flags_malformed_address(): + with patch(HTTPS_CONNECTION, return_value=_mock_connection(400)): + assert check_email_domain("x@example.org") == {"invalid": True} + + +def test_check_email_domain_retries_once_when_rate_limited(): + with ( + patch(HTTPS_CONNECTION, side_effect=[_mock_connection(429), _mock_connection()]), + patch(SLEEP) as mock_sleep, + patch(PUSHOVER) as mock_pushover, + ): + assert check_email_domain("x@example.org") is not None + + mock_sleep.assert_called_once() + mock_pushover.assert_not_called() + + +def test_check_email_domain_alerts_when_still_rate_limited(): + with ( + patch(HTTPS_CONNECTION, side_effect=[_mock_connection(429), _mock_connection(429)]), + patch(SLEEP), + patch(PUSHOVER) as mock_pushover, + ): + assert check_email_domain("x@example.org") is None + + mock_pushover.assert_called_once() + + +def test_check_email_domain_alerts_at_most_once_per_interval(): + with ( + patch(HTTPS_CONNECTION, side_effect=lambda *a, **kw: _mock_connection(500)), + patch(PUSHOVER) as mock_pushover, + ): + assert check_email_domain("x@example.org") is None + assert check_email_domain("y@example.net") is None + + mock_pushover.assert_called_once() def test_check_email_domain_returns_none_on_network_error(): conn = _mock_connection() conn.request.side_effect = TimeoutError("timed out") - with patch(HTTPS_CONNECTION, return_value=conn): + with patch(HTTPS_CONNECTION, return_value=conn), patch(PUSHOVER) as mock_pushover: assert check_email_domain("x@example.org") is None conn.close.assert_called_once() + mock_pushover.assert_called_once() + + +def test_check_email_domain_returns_none_when_connection_fails(): + with patch(HTTPS_CONNECTION, side_effect=OSError("unreachable")): + assert check_email_domain("x@example.org") is None diff --git a/users/forms.py b/users/forms.py index 35815fcc..4457919c 100644 --- a/users/forms.py +++ b/users/forms.py @@ -1,6 +1,7 @@ import logging from typing import Any +from disposable_email_domains import blocklist as DISPOSABLE_DOMAINS # noqa: N812 from django.contrib.admin.forms import AdminAuthenticationForm from django.contrib.auth.forms import AuthenticationForm, UserChangeForm, UserCreationForm from django.core.exceptions import ValidationError @@ -20,7 +21,8 @@ LOGGER = logging.getLogger(__name__) -# Providers that hand out temporary email addresses. +# Providers that hand out temporary email addresses, on top of the +# disposable-email-domains package's list. EMAIL_DOMAIN_BLOCKLIST = { "cloudinbox.top", "duck.com", @@ -50,26 +52,52 @@ def validate_email_allowed(email: str, exclude_pk: int | None = None) -> None: LOGGER.warning("'%s' already exists", email) raise ValidationError(_("Email already exists")) - try: - _unused, domain = email.split("@") - except ValueError as exc: - LOGGER.warning("Email: %s | Error: %s", email, exc) - raise ValidationError(_("Email address is not valid.")) from exc + # Split on the last "@": a quoted local part may itself legally contain one. + local, sep, domain = email.rpartition("@") + if not sep or not local or not domain: + LOGGER.warning("'%s' has no local part or domain", email) + raise ValidationError(_("Email address is not valid.")) + domain = domain.lower() - if domain in EMAIL_DOMAIN_BLOCKLIST: + if _is_blocklisted(domain): LOGGER.warning("'%s' is a temporary email address.", email) raise ValidationError(_("Temporary email addresses are not allowed.")) - if domain not in EMAIL_DOMAIN_ALLOWLIST: - resp = check_email_domain(email) - if resp is None: - raise ValidationError(_("Error creating account. Contact the site administrator.")) - if resp["block"] is True: - if resp["disposable"] is True: - LOGGER.warning("'%s' is a temporary email address.", email) - raise ValidationError(_("Temporary email addresses are not allowed.")) - LOGGER.warning("'%s' is not a valid email address.", email) - raise ValidationError(_("Email address is not valid.")) + if domain in EMAIL_DOMAIN_ALLOWLIST: + return + + verdict = check_email_domain(email) + if verdict is None: + # Fail open: the address already passed the local blocklists, and + # check_email_domain has alerted us that the remote check is down. + LOGGER.warning("Allowing '%s' without a UserCheck verdict", email) + return + if verdict.get("invalid"): + LOGGER.warning("'%s' is not a valid email address.", email) + raise ValidationError(_("Email address is not valid.")) + if verdict["disposable"] or verdict["spam"]: + LOGGER.warning("'%s' is a temporary or spam email address.", email) + raise ValidationError(_("Temporary email addresses are not allowed.")) + if verdict["relay_domain"]: + LOGGER.warning("'%s' is an email forwarding address.", email) + raise ValidationError(_("Email forwarding addresses are not allowed.")) + if verdict["mx"] is False: + LOGGER.warning("'%s' can't receive mail.", email) + if suggested := verdict["suggested_domain"]: + raise ValidationError( + _("Did you mean %(suggestion)s?"), params={"suggestion": f"{local}@{suggested}"} + ) + raise ValidationError(_("Email address is not valid.")) + + +def _is_blocklisted(domain: str) -> bool: + """Whether ``domain``, or any domain it's a subdomain of, is a known throwaway provider.""" + labels = domain.split(".") + for i in range(len(labels) - 1): + candidate = ".".join(labels[i:]) + if candidate in EMAIL_DOMAIN_BLOCKLIST or candidate in DISPOSABLE_DOMAINS: + return True + return False class CustomUserCreationForm(UserCreationForm): diff --git a/users/utils.py b/users/utils.py index c412f62a..755fe23b 100644 --- a/users/utils.py +++ b/users/utils.py @@ -2,14 +2,25 @@ import json import logging import ssl +import time import urllib.parse from django.conf import settings - -from metron.settings import RAPID_API_HOST, RAPID_API_KEY +from django.core.cache import cache LOGGER = logging.getLogger(__name__) +USERCHECK_HOST = "api.usercheck.com" +# Verdicts are about the domain, not the mailbox, so one lookup covers every +# address on it - caching them saves credits on the (metered) UserCheck plan. +CACHE_PREFIX = "usercheck:domain" +CACHE_TIMEOUT = 60 * 60 * 24 * 30 +# The free plan allows 1 request/second, so a 429 is often just two lookups +# landing in the same second; one retry after this delay covers that. +RETRY_DELAY = 1 +ALERT_KEY = "usercheck:alerted" +ALERT_INTERVAL = 60 * 60 + def client_ip(request): # nginx (nginx/nginx.conf) always overwrites X-Real-IP with its own @@ -20,37 +31,80 @@ def client_ip(request): return request.META.get("HTTP_X_REAL_IP") or request.META.get("REMOTE_ADDR", "unknown") -def check_email_domain(email: str): - result = None +def _alert_usercheck_failure(reason: str) -> None: + LOGGER.error("UserCheck lookup failed: %s", reason) + # cache.add only succeeds once per interval, so an outage (or running out of + # credits) sends one notification rather than one per signup attempt. + if cache.add(ALERT_KEY, True, timeout=ALERT_INTERVAL): + send_pushover( + f"UserCheck email lookups are failing ({reason}). " + "Signups are being allowed without the remote check." + ) + + +def _usercheck_request(email: str) -> tuple[int, str, bytes]: + conn = None try: # Without a timeout a stalled API blocks the gunicorn worker indefinitely. - conn = http.client.HTTPSConnection("mailcheck.p.rapidapi.com", timeout=10) + conn = http.client.HTTPSConnection(USERCHECK_HOST, timeout=10) + # Encode the address: a quoted local part may legally contain "/", "?", "#" + # or spaces, which would otherwise alter or break the path. + path = f"/email/{urllib.parse.quote(email, safe='')}" + headers = {"Authorization": f"Bearer {settings.USERCHECK_API_KEY}"} + conn.request("GET", path, headers=headers) + res = conn.getresponse() + return res.status, res.reason, res.read() + finally: + if conn is not None: + conn.close() - headers = { - "X-RapidAPI-Key": RAPID_API_KEY, - "X-RapidAPI-Host": RAPID_API_HOST, - } - # Encode the address: a quoted local part may legally contain "&", "#", "=" - # or spaces, which would otherwise alter or break the query string. - query = urllib.parse.urlencode({"domain": email}) - conn.request("GET", f"/?{query}", headers=headers) +def check_email_domain(email: str) -> dict | None: + """Look up ``email`` with UserCheck and return a verdict for its domain. - res = conn.getresponse() - match res.status: - case 200: - data = res.read() - result = json.loads(data.decode("utf-8")) - case _: - LOGGER.error("Bad response from RapidAPI: %s %s", res.status, res.reason) - except http.client.HTTPException as e: - LOGGER.error("HTTP error: %s", e) - except Exception as e: # NOQA: BLE001 - LOGGER.error("An error occurred: %s", e) - finally: - conn.close() + The verdict has ``disposable``, ``spam``, ``relay_domain``, ``mx`` and + ``suggested_domain`` keys, or is ``{"invalid": True}`` for an address UserCheck + rejects as malformed. Returns None when no answer could be had (network error, + out of credits, ...), leaving the caller to decide whether to fail open. + """ + domain = email.rsplit("@", 1)[-1].lower() + cache_key = f"{CACHE_PREFIX}:{domain}" + if (cached := cache.get(cache_key)) is not None: + return cached + + try: + status, reason, body = _usercheck_request(email) + if status == 429: # noqa: PLR2004 + time.sleep(RETRY_DELAY) + status, reason, body = _usercheck_request(email) + except (http.client.HTTPException, OSError) as e: + _alert_usercheck_failure(f"{type(e).__name__}: {e}") + return None - return result + match status: + case 200: + try: + data = json.loads(body.decode("utf-8")) + except ValueError as e: + _alert_usercheck_failure(f"invalid JSON: {e}") + return None + did_you_mean = data.get("did_you_mean") + verdict = { + "disposable": bool(data.get("disposable")), + "spam": bool(data.get("spam")), + "relay_domain": bool(data.get("relay_domain")), + # None means UserCheck couldn't tell, which isn't grounds to reject. + "mx": data.get("mx"), + "suggested_domain": did_you_mean.rsplit("@", 1)[-1] if did_you_mean else None, + } + cache.set(cache_key, verdict, timeout=CACHE_TIMEOUT) + return verdict + case 400: + # Specific to the address, so not cached. + return {"invalid": True} + case _: + _alert_usercheck_failure(f"{status} {reason}") + return None def send_pushover(message): From 4f686513690180c1bb58ae943613667fe75ba28d Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Sat, 17 Oct 2026 09:58:28 -0400 Subject: [PATCH 04/10] Harden UserCheck lookups so signups fail open - Catch Pushover errors in the UserCheck failure alert, and give send_pushover a timeout and close its connections, so an alert that fails or hangs can't turn a signup into a 500. - Make USERCHECK_API_KEY optional. Without it, the remote check is skipped and signups are checked against the local blocklists only, so a deploy with an old env file still starts. - Handle 200 responses whose body isn't a JSON object, or whose did_you_mean isn't a string, by alerting and allowing the signup instead of raising AttributeError. --- metron/settings.py | 3 +- tests/users/test_utils.py | 39 +++++++++++++++++ users/utils.py | 88 ++++++++++++++++++++++++--------------- 3 files changed, 96 insertions(+), 34 deletions(-) diff --git a/metron/settings.py b/metron/settings.py index 84027f0c..2f5aa537 100644 --- a/metron/settings.py +++ b/metron/settings.py @@ -368,7 +368,8 @@ } HCAPTCHA_SECRET_KEY = config("HCAPTCHA_SECRET_KEY") -USERCHECK_API_KEY = config("USERCHECK_API_KEY") +# Optional: without a key, signups are checked against the local blocklists only. +USERCHECK_API_KEY = config("USERCHECK_API_KEY", default="") OPENCOLLECTIVE_API_KEY = config("OPENCOLLECTIVE_API_KEY") OPENCOLLECTIVE_SLUG = config("OPENCOLLECTIVE_SLUG", default="metron") diff --git a/tests/users/test_utils.py b/tests/users/test_utils.py index 8560ddbd..8ca9f7f4 100644 --- a/tests/users/test_utils.py +++ b/tests/users/test_utils.py @@ -24,6 +24,11 @@ } +@pytest.fixture(autouse=True) +def usercheck_api_key(settings): + settings.USERCHECK_API_KEY = "test-key" + + def _mock_connection(*statuses, body=None): """A connection whose successive responses have the given statuses (default: one 200).""" conn = MagicMock() @@ -154,3 +159,37 @@ def test_check_email_domain_returns_none_on_network_error(): def test_check_email_domain_returns_none_when_connection_fails(): with patch(HTTPS_CONNECTION, side_effect=OSError("unreachable")): assert check_email_domain("x@example.org") is None + + +def test_check_email_domain_skips_lookup_without_api_key(settings): + settings.USERCHECK_API_KEY = "" + with patch(HTTPS_CONNECTION) as mock_conn, patch(PUSHOVER) as mock_pushover: + assert check_email_domain("x@example.org") is None + mock_conn.assert_not_called() + mock_pushover.assert_not_called() + + +def test_check_email_domain_fails_open_when_alert_fails(): + with ( + patch(HTTPS_CONNECTION, side_effect=OSError("unreachable")), + patch(PUSHOVER, side_effect=OSError("pushover unreachable")) as mock_pushover, + ): + assert check_email_domain("x@example.org") is None + mock_pushover.assert_called_once() + + +@pytest.mark.parametrize("body", [[], None, "ok"]) +def test_check_email_domain_returns_none_on_unexpected_response_shape(body): + conn = _mock_connection() + conn.getresponse.side_effect = None + conn.getresponse.return_value.status = 200 + conn.getresponse.return_value.read.return_value = json.dumps(body).encode() + with patch(HTTPS_CONNECTION, return_value=conn), patch(PUSHOVER) as mock_pushover: + assert check_email_domain("x@example.org") is None + mock_pushover.assert_called_once() + + +def test_check_email_domain_ignores_non_string_suggestion(): + body = {**USERCHECK_BODY, "did_you_mean": {"email": "x@gmail.com"}} + with patch(HTTPS_CONNECTION, return_value=_mock_connection(body=body)): + assert check_email_domain("x@example.org")["suggested_domain"] is None diff --git a/users/utils.py b/users/utils.py index 755fe23b..e3616698 100644 --- a/users/utils.py +++ b/users/utils.py @@ -36,10 +36,15 @@ def _alert_usercheck_failure(reason: str) -> None: # cache.add only succeeds once per interval, so an outage (or running out of # credits) sends one notification rather than one per signup attempt. if cache.add(ALERT_KEY, True, timeout=ALERT_INTERVAL): - send_pushover( - f"UserCheck email lookups are failing ({reason}). " - "Signups are being allowed without the remote check." - ) + # This runs when the network is likely unhealthy, so Pushover may be + # failing too - that mustn't turn the caller's fail-open into a 500. + try: + send_pushover( + f"UserCheck email lookups are failing ({reason}). " + "Signups are being allowed without the remote check." + ) + except http.client.HTTPException, OSError: + LOGGER.exception("Failed to send UserCheck failure alert") def _usercheck_request(email: str) -> tuple[int, str, bytes]: @@ -59,6 +64,28 @@ def _usercheck_request(email: str) -> tuple[int, str, bytes]: conn.close() +def _parse_verdict(body: bytes) -> dict | None: + try: + data = json.loads(body.decode("utf-8")) + except ValueError as e: + _alert_usercheck_failure(f"invalid JSON: {e}") + return None + if not isinstance(data, dict): + _alert_usercheck_failure(f"unexpected response: {type(data).__name__}") + return None + did_you_mean = data.get("did_you_mean") + if not isinstance(did_you_mean, str): + did_you_mean = None + return { + "disposable": bool(data.get("disposable")), + "spam": bool(data.get("spam")), + "relay_domain": bool(data.get("relay_domain")), + # None means UserCheck couldn't tell, which isn't grounds to reject. + "mx": data.get("mx"), + "suggested_domain": did_you_mean.rsplit("@", 1)[-1] if did_you_mean else None, + } + + def check_email_domain(email: str) -> dict | None: """Look up ``email`` with UserCheck and return a verdict for its domain. @@ -67,6 +94,10 @@ def check_email_domain(email: str) -> dict | None: rejects as malformed. Returns None when no answer could be had (network error, out of credits, ...), leaving the caller to decide whether to fail open. """ + if not settings.USERCHECK_API_KEY: + LOGGER.warning("USERCHECK_API_KEY is not set; skipping UserCheck lookup") + return None + domain = email.rsplit("@", 1)[-1].lower() cache_key = f"{CACHE_PREFIX}:{domain}" if (cached := cache.get(cache_key)) is not None: @@ -83,21 +114,9 @@ def check_email_domain(email: str) -> dict | None: match status: case 200: - try: - data = json.loads(body.decode("utf-8")) - except ValueError as e: - _alert_usercheck_failure(f"invalid JSON: {e}") - return None - did_you_mean = data.get("did_you_mean") - verdict = { - "disposable": bool(data.get("disposable")), - "spam": bool(data.get("spam")), - "relay_domain": bool(data.get("relay_domain")), - # None means UserCheck couldn't tell, which isn't grounds to reject. - "mx": data.get("mx"), - "suggested_domain": did_you_mean.rsplit("@", 1)[-1] if did_you_mean else None, - } - cache.set(cache_key, verdict, timeout=CACHE_TIMEOUT) + verdict = _parse_verdict(body) + if verdict is not None: + cache.set(cache_key, verdict, timeout=CACHE_TIMEOUT) return verdict case 400: # Specific to the address, so not cached. @@ -113,17 +132,20 @@ def send_pushover(message): user_keys = [key.strip() for key in settings.PUSHOVER_USER_KEY.split(",") if key.strip()] for user_key in user_keys: context = ssl.create_default_context() - conn = http.client.HTTPSConnection("api.pushover.net:443", context=context) - conn.request( - "POST", - "/1/messages.json", - urllib.parse.urlencode( - { - "token": settings.PUSHOVER_TOKEN, - "user": user_key, - "message": message, - } - ), - {"Content-type": "application/x-www-form-urlencoded"}, - ) - conn.getresponse() + conn = http.client.HTTPSConnection("api.pushover.net:443", context=context, timeout=10) + try: + conn.request( + "POST", + "/1/messages.json", + urllib.parse.urlencode( + { + "token": settings.PUSHOVER_TOKEN, + "user": user_key, + "message": message, + } + ), + {"Content-type": "application/x-www-form-urlencoded"}, + ) + conn.getresponse() + finally: + conn.close() From 565ab35080840625e61241d836be27d359bfa8f4 Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Sat, 17 Oct 2026 10:00:30 -0400 Subject: [PATCH 05/10] Cache missing-MX UserCheck verdicts for an hour, not 30 days A domain with mx False may just have had a brief DNS failure. Caching that verdict for the full 30 days blocked every address on the domain until the entry expired. --- tests/users/test_utils.py | 17 ++++++++++++++++- users/utils.py | 6 +++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/tests/users/test_utils.py b/tests/users/test_utils.py index 8ca9f7f4..e43cf806 100644 --- a/tests/users/test_utils.py +++ b/tests/users/test_utils.py @@ -4,8 +4,9 @@ from unittest.mock import MagicMock, patch import pytest +from django.core.cache import cache -from users.utils import check_email_domain +from users.utils import CACHE_TIMEOUT, NO_MX_CACHE_TIMEOUT, check_email_domain HTTPS_CONNECTION = "users.utils.http.client.HTTPSConnection" SLEEP = "users.utils.time.sleep" @@ -102,6 +103,20 @@ def test_check_email_domain_caches_verdict_per_domain(): mock_conn.assert_called_once() +@pytest.mark.parametrize( + ("mx", "expected_timeout"), + [(True, CACHE_TIMEOUT), (None, CACHE_TIMEOUT), (False, NO_MX_CACHE_TIMEOUT)], +) +def test_check_email_domain_caches_missing_mx_briefly(mx, expected_timeout): + body = {**USERCHECK_BODY, "mx": mx} + with ( + patch(HTTPS_CONNECTION, return_value=_mock_connection(body=body)), + patch.object(cache, "set") as mock_set, + ): + check_email_domain("x@example.org") + assert mock_set.call_args.kwargs["timeout"] == expected_timeout + + def test_check_email_domain_does_not_cache_failures(): with patch(HTTPS_CONNECTION, side_effect=[_mock_connection(500), _mock_connection()]): assert check_email_domain("x@example.org") is None diff --git a/users/utils.py b/users/utils.py index e3616698..689d12de 100644 --- a/users/utils.py +++ b/users/utils.py @@ -15,6 +15,9 @@ # address on it - caching them saves credits on the (metered) UserCheck plan. CACHE_PREFIX = "usercheck:domain" CACHE_TIMEOUT = 60 * 60 * 24 * 30 +# A missing MX record can be a passing DNS failure, and caching it for the full +# period would block every address on the domain until it expired. +NO_MX_CACHE_TIMEOUT = 60 * 60 # The free plan allows 1 request/second, so a 429 is often just two lookups # landing in the same second; one retry after this delay covers that. RETRY_DELAY = 1 @@ -116,7 +119,8 @@ def check_email_domain(email: str) -> dict | None: case 200: verdict = _parse_verdict(body) if verdict is not None: - cache.set(cache_key, verdict, timeout=CACHE_TIMEOUT) + timeout = NO_MX_CACHE_TIMEOUT if verdict["mx"] is False else CACHE_TIMEOUT + cache.set(cache_key, verdict, timeout=timeout) return verdict case 400: # Specific to the address, so not cached. From f5e6cfd955e6cec037746e847b76a8d5003c3d94 Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Sat, 17 Oct 2026 10:03:55 -0400 Subject: [PATCH 06/10] Lower UserCheck request timeout from 10s to 3s Lookups run inside the signup request, so a slow UserCheck tied up a worker for up to about 21s (two 10s timeouts plus the 429 retry delay). A healthy lookup takes well under a second, and slower ones fail open anyway. --- users/utils.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/users/utils.py b/users/utils.py index 689d12de..144a928b 100644 --- a/users/utils.py +++ b/users/utils.py @@ -11,6 +11,9 @@ LOGGER = logging.getLogger(__name__) USERCHECK_HOST = "api.usercheck.com" +# Lookups run inside the signup request, so keep this short: a healthy lookup +# takes well under a second, and if it's slower we fail open anyway. +REQUEST_TIMEOUT = 3 # Verdicts are about the domain, not the mailbox, so one lookup covers every # address on it - caching them saves credits on the (metered) UserCheck plan. CACHE_PREFIX = "usercheck:domain" @@ -54,7 +57,7 @@ def _usercheck_request(email: str) -> tuple[int, str, bytes]: conn = None try: # Without a timeout a stalled API blocks the gunicorn worker indefinitely. - conn = http.client.HTTPSConnection(USERCHECK_HOST, timeout=10) + conn = http.client.HTTPSConnection(USERCHECK_HOST, timeout=REQUEST_TIMEOUT) # Encode the address: a quoted local part may legally contain "/", "?", "#" # or spaces, which would otherwise alter or break the path. path = f"/email/{urllib.parse.quote(email, safe='')}" From 2d1d88fea9d6f756d9fa97373feade032743b6f8 Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Sat, 17 Oct 2026 10:05:38 -0400 Subject: [PATCH 07/10] Pass the normalized domain into check_email_domain validate_email_allowed already extracts and lowercases the domain to check the blocklists. check_email_domain was parsing it again with slightly different code, so the cache key and the blocklist check could drift apart. Parse it in one place and pass it in. --- tests/users/test_email_change.py | 2 +- tests/users/test_email_validation.py | 8 +++++- tests/users/test_utils.py | 40 ++++++++++++++-------------- users/forms.py | 2 +- users/utils.py | 6 +++-- 5 files changed, 33 insertions(+), 25 deletions(-) diff --git a/tests/users/test_email_change.py b/tests/users/test_email_change.py index 12de0343..71d49ab3 100644 --- a/tests/users/test_email_change.py +++ b/tests/users/test_email_change.py @@ -150,7 +150,7 @@ def test_runs_domain_check_for_unlisted_domains(self, auto_login_user, test_pass reverse("change_profile"), _profile_data(user, email="x@example.org", current_password=test_password), ) - mock_check.assert_called_once_with("x@example.org") + mock_check.assert_called_once_with("x@example.org", "example.org") assert resp.context["form"].errors["email"] def test_send_failure_saves_nothing(self, auto_login_user, test_password, test_email): diff --git a/tests/users/test_email_validation.py b/tests/users/test_email_validation.py index 52601009..1835c8e3 100644 --- a/tests/users/test_email_validation.py +++ b/tests/users/test_email_validation.py @@ -52,7 +52,13 @@ def test_allowlisted_domain_skips_lookup(): def test_clean_verdict_is_allowed(): message, mock_check = _error("x@example.org") assert message is None - mock_check.assert_called_once_with("x@example.org") + mock_check.assert_called_once_with("x@example.org", "example.org") + + +def test_lookup_gets_normalized_domain(): + message, mock_check = _error("x@Example.ORG") + assert message is None + mock_check.assert_called_once_with("x@Example.ORG", "example.org") def test_fails_open_without_verdict(): diff --git a/tests/users/test_utils.py b/tests/users/test_utils.py index e43cf806..fb69b54d 100644 --- a/tests/users/test_utils.py +++ b/tests/users/test_utils.py @@ -58,7 +58,7 @@ def _mock_connection(*statuses, body=None): def test_check_email_domain_encodes_path(email, expected_path): conn = _mock_connection() with patch(HTTPS_CONNECTION, return_value=conn): - check_email_domain(email) + check_email_domain(email, "example.org") method, path = conn.request.call_args.args assert method == "GET" @@ -69,7 +69,7 @@ def test_check_email_domain_sends_bearer_token(settings): settings.USERCHECK_API_KEY = "secret" conn = _mock_connection() with patch(HTTPS_CONNECTION, return_value=conn): - check_email_domain("x@example.org") + check_email_domain("x@example.org", "example.org") assert conn.request.call_args.kwargs["headers"] == {"Authorization": "Bearer secret"} @@ -83,7 +83,7 @@ def test_check_email_domain_returns_normalized_verdict(): "did_you_mean": "x@gmail.com", } with patch(HTTPS_CONNECTION, return_value=_mock_connection(body=body)): - assert check_email_domain("x@gmial.com") == { + assert check_email_domain("x@gmial.com", "gmial.com") == { "disposable": True, "spam": False, "relay_domain": True, @@ -95,9 +95,9 @@ def test_check_email_domain_returns_normalized_verdict(): def test_check_email_domain_caches_verdict_per_domain(): conn = _mock_connection() with patch(HTTPS_CONNECTION, return_value=conn) as mock_conn: - first = check_email_domain("x@example.org") - # Another mailbox on the same domain, differently cased, hits the cache. - second = check_email_domain("y@Example.ORG") + first = check_email_domain("x@example.org", "example.org") + # Another mailbox on the same domain hits the cache. + second = check_email_domain("y@example.org", "example.org") assert first == second mock_conn.assert_called_once() @@ -113,19 +113,19 @@ def test_check_email_domain_caches_missing_mx_briefly(mx, expected_timeout): patch(HTTPS_CONNECTION, return_value=_mock_connection(body=body)), patch.object(cache, "set") as mock_set, ): - check_email_domain("x@example.org") + check_email_domain("x@example.org", "example.org") assert mock_set.call_args.kwargs["timeout"] == expected_timeout def test_check_email_domain_does_not_cache_failures(): with patch(HTTPS_CONNECTION, side_effect=[_mock_connection(500), _mock_connection()]): - assert check_email_domain("x@example.org") is None - assert check_email_domain("x@example.org") is not None + assert check_email_domain("x@example.org", "example.org") is None + assert check_email_domain("x@example.org", "example.org") is not None def test_check_email_domain_flags_malformed_address(): with patch(HTTPS_CONNECTION, return_value=_mock_connection(400)): - assert check_email_domain("x@example.org") == {"invalid": True} + assert check_email_domain("x@example.org", "example.org") == {"invalid": True} def test_check_email_domain_retries_once_when_rate_limited(): @@ -134,7 +134,7 @@ def test_check_email_domain_retries_once_when_rate_limited(): patch(SLEEP) as mock_sleep, patch(PUSHOVER) as mock_pushover, ): - assert check_email_domain("x@example.org") is not None + assert check_email_domain("x@example.org", "example.org") is not None mock_sleep.assert_called_once() mock_pushover.assert_not_called() @@ -146,7 +146,7 @@ def test_check_email_domain_alerts_when_still_rate_limited(): patch(SLEEP), patch(PUSHOVER) as mock_pushover, ): - assert check_email_domain("x@example.org") is None + assert check_email_domain("x@example.org", "example.org") is None mock_pushover.assert_called_once() @@ -156,8 +156,8 @@ def test_check_email_domain_alerts_at_most_once_per_interval(): patch(HTTPS_CONNECTION, side_effect=lambda *a, **kw: _mock_connection(500)), patch(PUSHOVER) as mock_pushover, ): - assert check_email_domain("x@example.org") is None - assert check_email_domain("y@example.net") is None + assert check_email_domain("x@example.org", "example.org") is None + assert check_email_domain("y@example.net", "example.net") is None mock_pushover.assert_called_once() @@ -166,20 +166,20 @@ def test_check_email_domain_returns_none_on_network_error(): conn = _mock_connection() conn.request.side_effect = TimeoutError("timed out") with patch(HTTPS_CONNECTION, return_value=conn), patch(PUSHOVER) as mock_pushover: - assert check_email_domain("x@example.org") is None + assert check_email_domain("x@example.org", "example.org") is None conn.close.assert_called_once() mock_pushover.assert_called_once() def test_check_email_domain_returns_none_when_connection_fails(): with patch(HTTPS_CONNECTION, side_effect=OSError("unreachable")): - assert check_email_domain("x@example.org") is None + assert check_email_domain("x@example.org", "example.org") is None def test_check_email_domain_skips_lookup_without_api_key(settings): settings.USERCHECK_API_KEY = "" with patch(HTTPS_CONNECTION) as mock_conn, patch(PUSHOVER) as mock_pushover: - assert check_email_domain("x@example.org") is None + assert check_email_domain("x@example.org", "example.org") is None mock_conn.assert_not_called() mock_pushover.assert_not_called() @@ -189,7 +189,7 @@ def test_check_email_domain_fails_open_when_alert_fails(): patch(HTTPS_CONNECTION, side_effect=OSError("unreachable")), patch(PUSHOVER, side_effect=OSError("pushover unreachable")) as mock_pushover, ): - assert check_email_domain("x@example.org") is None + assert check_email_domain("x@example.org", "example.org") is None mock_pushover.assert_called_once() @@ -200,11 +200,11 @@ def test_check_email_domain_returns_none_on_unexpected_response_shape(body): conn.getresponse.return_value.status = 200 conn.getresponse.return_value.read.return_value = json.dumps(body).encode() with patch(HTTPS_CONNECTION, return_value=conn), patch(PUSHOVER) as mock_pushover: - assert check_email_domain("x@example.org") is None + assert check_email_domain("x@example.org", "example.org") is None mock_pushover.assert_called_once() def test_check_email_domain_ignores_non_string_suggestion(): body = {**USERCHECK_BODY, "did_you_mean": {"email": "x@gmail.com"}} with patch(HTTPS_CONNECTION, return_value=_mock_connection(body=body)): - assert check_email_domain("x@example.org")["suggested_domain"] is None + assert check_email_domain("x@example.org", "example.org")["suggested_domain"] is None diff --git a/users/forms.py b/users/forms.py index 4457919c..dd2934d9 100644 --- a/users/forms.py +++ b/users/forms.py @@ -66,7 +66,7 @@ def validate_email_allowed(email: str, exclude_pk: int | None = None) -> None: if domain in EMAIL_DOMAIN_ALLOWLIST: return - verdict = check_email_domain(email) + verdict = check_email_domain(email, domain) if verdict is None: # Fail open: the address already passed the local blocklists, and # check_email_domain has alerted us that the remote check is down. diff --git a/users/utils.py b/users/utils.py index 144a928b..8e0a40f3 100644 --- a/users/utils.py +++ b/users/utils.py @@ -92,9 +92,12 @@ def _parse_verdict(body: bytes) -> dict | None: } -def check_email_domain(email: str) -> dict | None: +def check_email_domain(email: str, domain: str) -> dict | None: """Look up ``email`` with UserCheck and return a verdict for its domain. + ``domain`` is the caller's already-normalized (lowercased) domain of ``email``, + used as the cache key so it matches what the caller checked its blocklists with. + The verdict has ``disposable``, ``spam``, ``relay_domain``, ``mx`` and ``suggested_domain`` keys, or is ``{"invalid": True}`` for an address UserCheck rejects as malformed. Returns None when no answer could be had (network error, @@ -104,7 +107,6 @@ def check_email_domain(email: str) -> dict | None: LOGGER.warning("USERCHECK_API_KEY is not set; skipping UserCheck lookup") return None - domain = email.rsplit("@", 1)[-1].lower() cache_key = f"{CACHE_PREFIX}:{domain}" if (cached := cache.get(cache_key)) is not None: return cached From bec3a4cf2491c2743c1321657238fede2a8cbe68 Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Sat, 17 Oct 2026 10:22:55 -0400 Subject: [PATCH 08/10] Verify the signup captcha before the UserCheck lookup, and reject rate-limited lookups while credits remain The email lookup ran during form validation, before the captcha was checked. Unauthenticated POSTs could spend metered UserCheck credits and force 429s, which the form treated as "allow without the check". - Check the captcha first, so every lookup costs a solved captcha. On an hCaptcha outage, re-render an unbound form so the errors shown don't trigger validation. - When lookups still get a 429 after the retry, use /status to tell the per-second limit from an exhausted quota. While credits remain, ask the user to retry instead of allowing the signup. When the quota is used up, keep allowing signups and send the alert. --- locale/it/LC_MESSAGES/django.po | 8 ++ tests/conftest.py | 3 +- tests/users/test_email_validation.py | 7 ++ tests/users/test_utils.py | 76 +++++++++++++++- tests/users/test_views.py | 41 ++++++++- users/forms.py | 5 ++ users/utils.py | 57 ++++++++++-- users/views.py | 125 +++++++++++++++------------ 8 files changed, 254 insertions(+), 68 deletions(-) diff --git a/locale/it/LC_MESSAGES/django.po b/locale/it/LC_MESSAGES/django.po index 869f1b0a..d1b246ae 100644 --- a/locale/it/LC_MESSAGES/django.po +++ b/locale/it/LC_MESSAGES/django.po @@ -6524,6 +6524,14 @@ msgstr "Gli indirizzi email di inoltro non sono ammessi." msgid "Did you mean %(suggestion)s?" msgstr "Intendevi %(suggestion)s?" +#: users/forms.py +msgid "" +"We couldn't verify your email address right now. Please try again in a " +"moment." +msgstr "" +"Al momento non è stato possibile verificare il tuo indirizzo email. Riprova " +"tra qualche istante." + #: users/forms.py:78 msgid "Required. Enter a valid email address." msgstr "Obbligatorio. Inserisci un indirizzo email valido." diff --git a/tests/conftest.py b/tests/conftest.py index 46db5b97..3aebd54f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -69,13 +69,14 @@ def isolated_login_throttle(): @pytest.fixture(autouse=True) def isolated_usercheck(): """Keep tests off the real UserCheck API, and give each test its own verdict - cache and alert flag in the shared Redis cache. Forms see a clean verdict for + cache, alert flag and credit status in the shared Redis cache. Forms see a clean verdict for any address not caught by the local blocklists; tests that need another verdict re-patch users.forms.check_email_domain, and the inner patch takes precedence.""" suffix = uuid.uuid4().hex with ( patch("users.utils.CACHE_PREFIX", f"usercheck:test:{suffix}"), patch("users.utils.ALERT_KEY", f"usercheck:alerted:test:{suffix}"), + patch("users.utils.CREDITS_KEY", f"usercheck:has_credits:test:{suffix}"), patch("users.forms.check_email_domain", return_value=CLEAN_EMAIL_VERDICT), ): yield diff --git a/tests/users/test_email_validation.py b/tests/users/test_email_validation.py index 1835c8e3..f8348477 100644 --- a/tests/users/test_email_validation.py +++ b/tests/users/test_email_validation.py @@ -90,6 +90,13 @@ def test_malformed_address_is_rejected(): assert message == "Email address is not valid." +def test_rate_limited_lookup_asks_user_to_retry(): + message, _mock = _error("x@example.org", verdict={"rate_limited": True}) + assert message == ( + "We couldn't verify your email address right now. Please try again in a moment." + ) + + def test_address_without_domain_is_rejected(): message, mock_check = _error("no-at-sign") assert message == "Email address is not valid." diff --git a/tests/users/test_utils.py b/tests/users/test_utils.py index fb69b54d..b33992e6 100644 --- a/tests/users/test_utils.py +++ b/tests/users/test_utils.py @@ -140,9 +140,46 @@ def test_check_email_domain_retries_once_when_rate_limited(): mock_pushover.assert_not_called() -def test_check_email_domain_alerts_when_still_rate_limited(): +def _mock_status(remaining=None, status=200): + """A connection answering the /status endpoint with ``remaining`` credits.""" + conn = MagicMock() + response = conn.getresponse.return_value + response.status = status + response.reason = "OK" if status == 200 else "Error" + response.read.return_value = json.dumps({"usage": {"remaining": remaining}}).encode() + return conn + + +def test_check_email_domain_rejects_when_rate_limited_with_credits_left(): + status_conn = _mock_status(remaining=500) + with ( + patch( + HTTPS_CONNECTION, + side_effect=[_mock_connection(429), _mock_connection(429), status_conn], + ), + patch(SLEEP), + patch(PUSHOVER) as mock_pushover, + ): + assert check_email_domain("x@example.org", "example.org") == {"rate_limited": True} + + assert status_conn.request.call_args.args[:2] == ("GET", "/status") + mock_pushover.assert_not_called() + + +@pytest.mark.parametrize( + "status_conn", + [ + _mock_status(remaining=0), # quota exhausted + _mock_status(status=500), # /status itself failing + _mock_status(remaining="lots"), # unexpected body + ], +) +def test_check_email_domain_alerts_when_rate_limited_without_known_credits(status_conn): with ( - patch(HTTPS_CONNECTION, side_effect=[_mock_connection(429), _mock_connection(429)]), + patch( + HTTPS_CONNECTION, + side_effect=[_mock_connection(429), _mock_connection(429), status_conn], + ), patch(SLEEP), patch(PUSHOVER) as mock_pushover, ): @@ -151,6 +188,41 @@ def test_check_email_domain_alerts_when_still_rate_limited(): mock_pushover.assert_called_once() +def test_check_email_domain_alerts_when_status_lookup_errors(): + with ( + patch( + HTTPS_CONNECTION, + side_effect=[_mock_connection(429), _mock_connection(429), OSError("unreachable")], + ), + patch(SLEEP), + patch(PUSHOVER) as mock_pushover, + ): + assert check_email_domain("x@example.org", "example.org") is None + + mock_pushover.assert_called_once() + + +def test_check_email_domain_caches_credit_status(): + with ( + patch( + HTTPS_CONNECTION, + side_effect=[ + _mock_connection(429), + _mock_connection(429), + _mock_status(remaining=500), + # The second lookup reuses the cached credit status: no /status call. + _mock_connection(429), + _mock_connection(429), + ], + ) as mock_conn, + patch(SLEEP), + ): + assert check_email_domain("x@example.org", "example.org") == {"rate_limited": True} + assert check_email_domain("y@example.net", "example.net") == {"rate_limited": True} + + assert mock_conn.call_count == 5 + + def test_check_email_domain_alerts_at_most_once_per_interval(): with ( patch(HTTPS_CONNECTION, side_effect=lambda *a, **kw: _mock_connection(500)), diff --git a/tests/users/test_views.py b/tests/users/test_views.py index f788b750..6a9558e3 100644 --- a/tests/users/test_views.py +++ b/tests/users/test_views.py @@ -240,7 +240,7 @@ def test_signup_hcaptcha_unavailable_shows_error(db, client): assert resp.status_code == HTML_OK_CODE assertTemplateUsed(resp, "registration/signup.html") - assert "unable to verify the captcha" in str(resp.context["form"].non_field_errors()) + assert "unable to verify the captcha" in resp.content.decode() assert CustomUser.objects.count() == user_count_before # The outage doesn't count against the IP limit, so a retry still works. @@ -250,6 +250,45 @@ def test_signup_hcaptcha_unavailable_shows_error(db, client): assert CustomUser.objects.count() == user_count_before + 1 +def test_signup_failed_captcha_skips_email_lookup(db, client): + user_count_before = CustomUser.objects.count() + with ( + patch("users.views.get_recaptcha_auth", return_value={"success": False}), + patch("users.forms.check_email_domain") as mock_check, + ): + resp = client.post(reverse("signup"), _signup_payload("bot"), REMOTE_ADDR=_unique_ip()) + + assert resp.status_code == HTTP_REDIRECT_FOUND_CODE + mock_check.assert_not_called() + assert CustomUser.objects.count() == user_count_before + + +def test_signup_hcaptcha_unavailable_skips_email_lookup(db, client): + unavailable = {"success": False, "error-codes": [HCAPTCHA_UNAVAILABLE]} + payload = _signup_payload("outage-lookup") + with ( + patch("users.views.get_recaptcha_auth", return_value=unavailable), + patch("users.forms.check_email_domain") as mock_check, + ): + resp = client.post(reverse("signup"), payload, REMOTE_ADDR=_unique_ip()) + # Rendering the form's errors must not validate it either. + content = resp.content.decode() + + mock_check.assert_not_called() + assert payload["email"] in content + + +def test_signup_invalid_form_after_captcha_shows_errors(db, client): + user_count_before = CustomUser.objects.count() + payload = {**_signup_payload("mismatch"), "password2": "something-else-entirely-2"} + with patch("users.views.get_recaptcha_auth", return_value={"success": True}): + resp = client.post(reverse("signup"), payload, REMOTE_ADDR=_unique_ip()) + + assert resp.status_code == HTML_OK_CODE + assert resp.context["form"].errors["password2"] + assert CustomUser.objects.count() == user_count_before + + def test_signup_failed_hcaptcha_still_redirects_silently(db, client): user_count_before = CustomUser.objects.count() failed = {"success": False, "error-codes": ["invalid-input-response"]} diff --git a/users/forms.py b/users/forms.py index dd2934d9..a7ba793f 100644 --- a/users/forms.py +++ b/users/forms.py @@ -72,6 +72,11 @@ def validate_email_allowed(email: str, exclude_pk: int | None = None) -> None: # check_email_domain has alerted us that the remote check is down. LOGGER.warning("Allowing '%s' without a UserCheck verdict", email) return + if verdict.get("rate_limited"): + LOGGER.warning("'%s' couldn't be checked: UserCheck is rate-limiting lookups", email) + raise ValidationError( + _("We couldn't verify your email address right now. Please try again in a moment.") + ) if verdict.get("invalid"): LOGGER.warning("'%s' is not a valid email address.", email) raise ValidationError(_("Email address is not valid.")) diff --git a/users/utils.py b/users/utils.py index 8e0a40f3..a59764c0 100644 --- a/users/utils.py +++ b/users/utils.py @@ -26,6 +26,10 @@ RETRY_DELAY = 1 ALERT_KEY = "usercheck:alerted" ALERT_INTERVAL = 60 * 60 +# Whether the plan has credits left, from the /status endpoint - only consulted +# when lookups keep getting a 429, so a short cache is plenty. +CREDITS_KEY = "usercheck:has_credits" +CREDITS_CACHE_TIMEOUT = 60 * 10 def client_ip(request): @@ -53,14 +57,11 @@ def _alert_usercheck_failure(reason: str) -> None: LOGGER.exception("Failed to send UserCheck failure alert") -def _usercheck_request(email: str) -> tuple[int, str, bytes]: +def _usercheck_request(path: str) -> tuple[int, str, bytes]: conn = None try: # Without a timeout a stalled API blocks the gunicorn worker indefinitely. conn = http.client.HTTPSConnection(USERCHECK_HOST, timeout=REQUEST_TIMEOUT) - # Encode the address: a quoted local part may legally contain "/", "?", "#" - # or spaces, which would otherwise alter or break the path. - path = f"/email/{urllib.parse.quote(email, safe='')}" headers = {"Authorization": f"Bearer {settings.USERCHECK_API_KEY}"} conn.request("GET", path, headers=headers) res = conn.getresponse() @@ -70,6 +71,30 @@ def _usercheck_request(email: str) -> tuple[int, str, bytes]: conn.close() +def _has_credits() -> bool | None: + """Whether the UserCheck plan has credits left, or None if that can't be told. + + The /email endpoint answers 429 both for the per-second limit and for an + exhausted monthly quota, with identical bodies; /status tells them apart. + """ + if (cached := cache.get(CREDITS_KEY)) is not None: + return cached + try: + status, reason, body = _usercheck_request("/status") + data = json.loads(body.decode("utf-8")) if status == 200 else None # noqa: PLR2004 + except (http.client.HTTPException, OSError, ValueError) as e: + LOGGER.warning("UserCheck status lookup failed: %s: %s", type(e).__name__, e) + return None + usage = data.get("usage") if isinstance(data, dict) else None + remaining = usage.get("remaining") if isinstance(usage, dict) else None + if not isinstance(remaining, int | float): + LOGGER.warning("UserCheck status lookup failed: %s %s", status, reason) + return None + has_credits = remaining > 0 + cache.set(CREDITS_KEY, has_credits, timeout=CREDITS_CACHE_TIMEOUT) + return has_credits + + def _parse_verdict(body: bytes) -> dict | None: try: data = json.loads(body.decode("utf-8")) @@ -100,8 +125,10 @@ def check_email_domain(email: str, domain: str) -> dict | None: The verdict has ``disposable``, ``spam``, ``relay_domain``, ``mx`` and ``suggested_domain`` keys, or is ``{"invalid": True}`` for an address UserCheck - rejects as malformed. Returns None when no answer could be had (network error, - out of credits, ...), leaving the caller to decide whether to fail open. + rejects as malformed, or ``{"rate_limited": True}`` when lookups are still being + throttled after a retry although the plan has credits left. Returns None when no + answer could be had (network error, out of credits, ...), leaving the caller to + decide whether to fail open. """ if not settings.USERCHECK_API_KEY: LOGGER.warning("USERCHECK_API_KEY is not set; skipping UserCheck lookup") @@ -111,15 +138,22 @@ def check_email_domain(email: str, domain: str) -> dict | None: if (cached := cache.get(cache_key)) is not None: return cached + # Encode the address: a quoted local part may legally contain "/", "?", "#" + # or spaces, which would otherwise alter or break the path. + path = f"/email/{urllib.parse.quote(email, safe='')}" try: - status, reason, body = _usercheck_request(email) + status, reason, body = _usercheck_request(path) if status == 429: # noqa: PLR2004 time.sleep(RETRY_DELAY) - status, reason, body = _usercheck_request(email) + status, reason, body = _usercheck_request(path) except (http.client.HTTPException, OSError) as e: _alert_usercheck_failure(f"{type(e).__name__}: {e}") return None + return _verdict_from_response(status, reason, body, cache_key) + + +def _verdict_from_response(status: int, reason: str, body: bytes, cache_key: str) -> dict | None: match status: case 200: verdict = _parse_verdict(body) @@ -130,7 +164,14 @@ def check_email_domain(email: str, domain: str) -> dict | None: case 400: # Specific to the address, so not cached. return {"invalid": True} + case 429 if _has_credits(): + # Credits remain, so this is the per-second limit rather than an exhausted + # quota. Failing open here would let anyone who can trigger it (by flooding + # lookups) skip the check, so the caller asks the user to retry instead. + LOGGER.warning("UserCheck lookups are still rate-limited after a retry") + return {"rate_limited": True} case _: + # Includes a 429 from an exhausted quota (or one /status couldn't explain). _alert_usercheck_failure(f"{status} {reason}") return None diff --git a/users/views.py b/users/views.py index 3fa760ce..4cc0af26 100644 --- a/users/views.py +++ b/users/views.py @@ -175,7 +175,52 @@ def activate(request, uidb64, token): return redirect("home") -def signup(request): # sourcery skip: extract-method +def _create_account(request, form): + """Create an inactive account from a valid signup ``form`` and email its activation link. + + Returns False, with an error added to ``form``, if the email couldn't be sent. + """ + user: CustomUser = form.save(commit=False) + user.is_active = False + user.save() + context = { + "user": user, + "uid": urlsafe_base64_encode(force_bytes(user.pk)), + "token": account_activation_token.make_token(user), + } + try: + _send_email( + request, + _("Activate Your Metron Account"), + "registration/account_activation_email", + context, + user.email, + ) + except smtplib.SMTPException, OSError: + # There's no way to resend the activation email, so don't leave + # behind an inactive account holding the username/email, and + # don't count it against the IP limit, so the user can retry. + logger.exception("Failed to send activation email to %s", user.username) + user.delete() + form.add_error( + None, + _("We were unable to send your activation email. Please try again in at a later time."), + ) + return False + _record_signup(request, user.username) + # Let's send a pushover notice that a user requested an account. + send_pushover(f"{user} signed up for an account on Metron.") + ip = client_ip(request) + logger.info( + "User signed up for an account on Metron (user=%s, ip=%s)", + user.username, + ip, + extra={"username": user.username, "ip": ip}, + ) + return True + + +def signup(request): settings_obj = SignupSettings.get_solo() if not settings_obj.signups_enabled: return render( @@ -190,63 +235,31 @@ def signup(request): # sourcery skip: extract-method return render(request, "registration/signup_rate_limited.html", status=429) form = CustomUserCreationForm(request.POST) - if form.is_valid(): - result = get_recaptcha_auth(request) + # Verify the captcha before validating the form: validation looks the email + # up with UserCheck, so checking first makes every (metered) lookup cost a + # solved captcha rather than just a POST. + result = get_recaptcha_auth(request) + + if HCAPTCHA_UNAVAILABLE in result.get("error-codes", []): + # An hCaptcha outage, not a failed check: tell the user to retry + # rather than falling through to "activation email sent". Re-render an + # unbound form, since displaying a bound one's errors would validate it. + form = CustomUserCreationForm( + initial={field: request.POST.get(field, "") for field in ("username", "email")} + ) + messages.error( + request, + _( + "We were unable to verify the captcha right now. " + "Please try again in a few minutes." + ), + ) + return render(request, "registration/signup.html", {"form": form}) - if HCAPTCHA_UNAVAILABLE in result.get("error-codes", []): - # An hCaptcha outage, not a failed check: tell the user to retry - # rather than falling through to "activation email sent". - form.add_error( - None, - _( - "We were unable to verify the captcha right now. " - "Please try again in a few minutes." - ), - ) - return render(request, "registration/signup.html", {"form": form}) - - if result["success"]: - user: CustomUser = form.save(commit=False) - user.is_active = False - user.save() - context = { - "user": user, - "uid": urlsafe_base64_encode(force_bytes(user.pk)), - "token": account_activation_token.make_token(user), - } - try: - _send_email( - request, - _("Activate Your Metron Account"), - "registration/account_activation_email", - context, - user.email, - ) - except smtplib.SMTPException, OSError: - # There's no way to resend the activation email, so don't leave - # behind an inactive account holding the username/email, and - # don't count it against the IP limit, so the user can retry. - logger.exception("Failed to send activation email to %s", user.username) - user.delete() - form.add_error( - None, - _( - "We were unable to send your activation email. " - "Please try again in at a later time." - ), - ) - return render(request, "registration/signup.html", {"form": form}) - _record_signup(request, user.username) - # Let's send a pushover notice that a user requested an account. - send_pushover(f"{user} signed up for an account on Metron.") - ip = client_ip(request) - logger.info( - "User signed up for an account on Metron (user=%s, ip=%s)", - user.username, - ip, - extra={"username": user.username, "ip": ip}, - ) + if not result["success"]: + return redirect("account_activation_sent") + if form.is_valid() and _create_account(request, form): return redirect("account_activation_sent") else: form = CustomUserCreationForm() From 0e55f13e0d8dd77178af857732498648408b7ec8 Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Sat, 17 Oct 2026 10:32:45 -0400 Subject: [PATCH 09/10] Gate profile email lookups on the password, and show an error on a failed signup captcha - The profile form ran the UserCheck lookup in clean_email, before clean() checked the current password, so any logged-in session could spend credits by posting new addresses. Run only the local checks there, and the remote check once the password is confirmed. - With the captcha now checked before form validation, a failed captcha redirected to "activation email sent" even when the form had errors. Re-render the form with "Please complete the captcha and try again." instead, without validating it. --- locale/it/LC_MESSAGES/django.po | 4 ++++ tests/users/test_email_change.py | 11 +++++++++ tests/users/test_views.py | 15 ++++++++---- users/forms.py | 41 +++++++++++++++++++++++++------- users/views.py | 28 +++++++++++++--------- 5 files changed, 75 insertions(+), 24 deletions(-) diff --git a/locale/it/LC_MESSAGES/django.po b/locale/it/LC_MESSAGES/django.po index d1b246ae..fe138715 100644 --- a/locale/it/LC_MESSAGES/django.po +++ b/locale/it/LC_MESSAGES/django.po @@ -6606,6 +6606,10 @@ msgstr "" "Al momento non è stato possibile verificare il captcha. Riprova tra qualche " "minuto." +#: users/views.py +msgid "Please complete the captcha and try again." +msgstr "Completa il captcha e riprova." + #: users/login_throttle.py msgid "Too many failed attempts. Please try again in 15 minutes." msgstr "Troppi tentativi non riusciti. Riprova tra 15 minuti." diff --git a/tests/users/test_email_change.py b/tests/users/test_email_change.py index 71d49ab3..91c41e56 100644 --- a/tests/users/test_email_change.py +++ b/tests/users/test_email_change.py @@ -153,6 +153,17 @@ def test_runs_domain_check_for_unlisted_domains(self, auto_login_user, test_pass mock_check.assert_called_once_with("x@example.org", "example.org") assert resp.context["form"].errors["email"] + @pytest.mark.parametrize("password", ["", "not-my-password"]) + def test_lookup_waits_for_correct_password(self, auto_login_user, password): + client, user = auto_login_user() + with patch("users.forms.check_email_domain") as mock_check: + resp = client.post( + reverse("change_profile"), + _profile_data(user, email="x@example.org", current_password=password), + ) + mock_check.assert_not_called() + assert resp.context["form"].errors["current_password"] + def test_send_failure_saves_nothing(self, auto_login_user, test_password, test_email): client, user = auto_login_user() with patch( diff --git a/tests/users/test_views.py b/tests/users/test_views.py index 6a9558e3..c435939b 100644 --- a/tests/users/test_views.py +++ b/tests/users/test_views.py @@ -252,14 +252,17 @@ def test_signup_hcaptcha_unavailable_shows_error(db, client): def test_signup_failed_captcha_skips_email_lookup(db, client): user_count_before = CustomUser.objects.count() + payload = _signup_payload("bot") with ( patch("users.views.get_recaptcha_auth", return_value={"success": False}), patch("users.forms.check_email_domain") as mock_check, ): - resp = client.post(reverse("signup"), _signup_payload("bot"), REMOTE_ADDR=_unique_ip()) + resp = client.post(reverse("signup"), payload, REMOTE_ADDR=_unique_ip()) + # Rendering the form must not validate it either. + content = resp.content.decode() - assert resp.status_code == HTTP_REDIRECT_FOUND_CODE mock_check.assert_not_called() + assert payload["email"] in content assert CustomUser.objects.count() == user_count_before @@ -289,13 +292,17 @@ def test_signup_invalid_form_after_captcha_shows_errors(db, client): assert CustomUser.objects.count() == user_count_before -def test_signup_failed_hcaptcha_still_redirects_silently(db, client): +def test_signup_failed_hcaptcha_asks_to_complete_captcha(db, client): user_count_before = CustomUser.objects.count() failed = {"success": False, "error-codes": ["invalid-input-response"]} with patch("users.views.get_recaptcha_auth", return_value=failed): resp = client.post(reverse("signup"), _signup_payload("bot"), REMOTE_ADDR=_unique_ip()) - assert resp.status_code == HTTP_REDIRECT_FOUND_CODE + # Not the "activation email sent" redirect: no account was created, so a real + # user would otherwise wait for an email that never comes. + assert resp.status_code == HTML_OK_CODE + assertTemplateUsed(resp, "registration/signup.html") + assert "Please complete the captcha and try again." in resp.content.decode() assert CustomUser.objects.count() == user_count_before diff --git a/users/forms.py b/users/forms.py index a7ba793f..d6c26b74 100644 --- a/users/forms.py +++ b/users/forms.py @@ -39,11 +39,15 @@ EMAIL_DOMAIN_ALLOWLIST = {"gmail.com", "yahoo.com", "proton.me"} -def validate_email_allowed(email: str, exclude_pk: int | None = None) -> None: +def validate_email_allowed( + email: str, exclude_pk: int | None = None, *, remote: bool = True +) -> None: """Raise ValidationError unless ``email`` may be used for an account. Shared by signup and email changes so a profile edit can't bypass the signup checks. ``exclude_pk`` skips the user changing their own address. + With ``remote=False`` only the local checks run, and the caller must call + validate_email_remotely() itself once it's ok to spend a (metered) lookup. """ others = CustomUser.objects.filter(email__iexact=email) if exclude_pk is not None: @@ -52,17 +56,18 @@ def validate_email_allowed(email: str, exclude_pk: int | None = None) -> None: LOGGER.warning("'%s' already exists", email) raise ValidationError(_("Email already exists")) - # Split on the last "@": a quoted local part may itself legally contain one. - local, sep, domain = email.rpartition("@") - if not sep or not local or not domain: - LOGGER.warning("'%s' has no local part or domain", email) - raise ValidationError(_("Email address is not valid.")) - domain = domain.lower() - + _local, domain = _split_email(email) if _is_blocklisted(domain): LOGGER.warning("'%s' is a temporary email address.", email) raise ValidationError(_("Temporary email addresses are not allowed.")) + if remote: + validate_email_remotely(email) + + +def validate_email_remotely(email: str) -> None: + """Raise ValidationError if UserCheck says ``email`` can't be used for an account.""" + local, domain = _split_email(email) if domain in EMAIL_DOMAIN_ALLOWLIST: return @@ -95,6 +100,16 @@ def validate_email_allowed(email: str, exclude_pk: int | None = None) -> None: raise ValidationError(_("Email address is not valid.")) +def _split_email(email: str) -> tuple[str, str]: + """Split ``email`` into its local part and lowercased domain.""" + # Split on the last "@": a quoted local part may itself legally contain one. + local, sep, domain = email.rpartition("@") + if not sep or not local or not domain: + LOGGER.warning("'%s' has no local part or domain", email) + raise ValidationError(_("Email address is not valid.")) + return local, domain.lower() + + def _is_blocklisted(domain: str) -> bool: """Whether ``domain``, or any domain it's a subdomain of, is a known throwaway provider.""" labels = domain.split(".") @@ -168,7 +183,8 @@ def pending_email(self) -> str | None: def clean_email(self): email = self.cleaned_data["email"] if email != self.original_email: - validate_email_allowed(email, exclude_pk=self.instance.pk) + # The UserCheck lookup waits for clean(), once the password checks out. + validate_email_allowed(email, exclude_pk=self.instance.pk, remote=False) return email def clean(self): @@ -189,6 +205,13 @@ def clean(self): "current_password", _("Enter your current password to change your email address."), ) + else: + # Only spend a (metered) UserCheck lookup on a confirmed password, so + # a logged-in session alone can't be used to burn through credits. + try: + validate_email_remotely(self.pending_email) + except ValidationError as e: + self.add_error("email", e) return cleaned_data def save(self, commit=True): diff --git a/users/views.py b/users/views.py index 4cc0af26..fd36abdb 100644 --- a/users/views.py +++ b/users/views.py @@ -220,6 +220,19 @@ def _create_account(request, form): return True +def _rerender_signup(request, message): + """Show the signup page again with ``message``, keeping the username and email. + + The form is unbound: displaying a bound form's errors would validate it, which + looks the email up with UserCheck - the lookup a failed captcha mustn't reach. + """ + form = CustomUserCreationForm( + initial={field: request.POST.get(field, "") for field in ("username", "email")} + ) + messages.error(request, message) + return render(request, "registration/signup.html", {"form": form}) + + def signup(request): settings_obj = SignupSettings.get_solo() if not settings_obj.signups_enabled: @@ -234,31 +247,24 @@ def signup(request): _notify_signup_rate_limit_hit(request) return render(request, "registration/signup_rate_limited.html", status=429) - form = CustomUserCreationForm(request.POST) # Verify the captcha before validating the form: validation looks the email # up with UserCheck, so checking first makes every (metered) lookup cost a # solved captcha rather than just a POST. result = get_recaptcha_auth(request) if HCAPTCHA_UNAVAILABLE in result.get("error-codes", []): - # An hCaptcha outage, not a failed check: tell the user to retry - # rather than falling through to "activation email sent". Re-render an - # unbound form, since displaying a bound one's errors would validate it. - form = CustomUserCreationForm( - initial={field: request.POST.get(field, "") for field in ("username", "email")} - ) - messages.error( + # An hCaptcha outage, not a failed check, so the user can just retry. + return _rerender_signup( request, _( "We were unable to verify the captcha right now. " "Please try again in a few minutes." ), ) - return render(request, "registration/signup.html", {"form": form}) - if not result["success"]: - return redirect("account_activation_sent") + return _rerender_signup(request, _("Please complete the captcha and try again.")) + form = CustomUserCreationForm(request.POST) if form.is_valid() and _create_account(request, form): return redirect("account_activation_sent") else: From deb4c65891667e2eb11fed51d444b8c005d0412e Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Sat, 17 Oct 2026 10:36:22 -0400 Subject: [PATCH 10/10] Only treat UserCheck's malformed-address 400 as an invalid email Every 400 hard-rejected the address, with no alert. A 400 caused by the request itself, or by an API change, would silently block real users. Reject only on the error UserCheck documents for a malformed address. Alert on any other 400 and allow the signup, as for other unexpected responses. --- tests/users/test_utils.py | 41 ++++++++++++++++++++++++++++++++++----- users/utils.py | 16 +++++++++++++-- 2 files changed, 50 insertions(+), 7 deletions(-) diff --git a/tests/users/test_utils.py b/tests/users/test_utils.py index b33992e6..09495992 100644 --- a/tests/users/test_utils.py +++ b/tests/users/test_utils.py @@ -31,16 +31,24 @@ def usercheck_api_key(settings): def _mock_connection(*statuses, body=None): - """A connection whose successive responses have the given statuses (default: one 200).""" + """A connection whose successive responses have the given statuses (default: one 200). + + ``body`` replaces every response's body (default: a clean verdict for a 200, a + generic error otherwise). + """ conn = MagicMock() responses = [] for status in statuses or (200,): response = MagicMock() response.status = status response.reason = "OK" if status == 200 else "Error" - response.read.return_value = json.dumps( - (body or USERCHECK_BODY) if status == 200 else {"status": status, "error": "Error"} - ).encode() + if body is not None: + content = body + elif status == 200: + content = USERCHECK_BODY + else: + content = {"status": status, "error": "Error"} + response.read.return_value = json.dumps(content).encode() responses.append(response) conn.getresponse.side_effect = responses return conn @@ -124,8 +132,31 @@ def test_check_email_domain_does_not_cache_failures(): def test_check_email_domain_flags_malformed_address(): - with patch(HTTPS_CONNECTION, return_value=_mock_connection(400)): + body = {"status": 400, "error": "The email address is invalid."} + with ( + patch(HTTPS_CONNECTION, return_value=_mock_connection(400, body=body)), + patch(PUSHOVER) as mock_pushover, + ): assert check_email_domain("x@example.org", "example.org") == {"invalid": True} + mock_pushover.assert_not_called() + + +@pytest.mark.parametrize( + "body", + [ + {"status": 400, "error": "Missing or malformed path."}, + {"status": 400}, + "Bad Request", + [], + ], +) +def test_check_email_domain_fails_open_on_other_400s(body): + with ( + patch(HTTPS_CONNECTION, return_value=_mock_connection(400, body=body)), + patch(PUSHOVER) as mock_pushover, + ): + assert check_email_domain("x@example.org", "example.org") is None + mock_pushover.assert_called_once() def test_check_email_domain_retries_once_when_rate_limited(): diff --git a/users/utils.py b/users/utils.py index a59764c0..1be33664 100644 --- a/users/utils.py +++ b/users/utils.py @@ -30,6 +30,9 @@ # when lookups keep getting a 429, so a short cache is plenty. CREDITS_KEY = "usercheck:has_credits" CREDITS_CACHE_TIMEOUT = 60 * 10 +# The error UserCheck documents for its only 400: a malformed address. Any other +# 400 points at our request (or an API change), which mustn't block the user. +MALFORMED_EMAIL_ERROR = "The email address is invalid." def client_ip(request): @@ -95,6 +98,14 @@ def _has_credits() -> bool | None: return has_credits +def _is_malformed_email_error(body: bytes) -> bool: + try: + data = json.loads(body.decode("utf-8")) + except ValueError: + return False + return isinstance(data, dict) and data.get("error") == MALFORMED_EMAIL_ERROR + + def _parse_verdict(body: bytes) -> dict | None: try: data = json.loads(body.decode("utf-8")) @@ -161,7 +172,7 @@ def _verdict_from_response(status: int, reason: str, body: bytes, cache_key: str timeout = NO_MX_CACHE_TIMEOUT if verdict["mx"] is False else CACHE_TIMEOUT cache.set(cache_key, verdict, timeout=timeout) return verdict - case 400: + case 400 if _is_malformed_email_error(body): # Specific to the address, so not cached. return {"invalid": True} case 429 if _has_credits(): @@ -171,7 +182,8 @@ def _verdict_from_response(status: int, reason: str, body: bytes, cache_key: str LOGGER.warning("UserCheck lookups are still rate-limited after a retry") return {"rate_limited": True} case _: - # Includes a 429 from an exhausted quota (or one /status couldn't explain). + # Includes a 429 from an exhausted quota (or one /status couldn't explain), + # and a 400 for anything other than a malformed address. _alert_usercheck_failure(f"{status} {reason}") return None