Block sign-ups from disposable email domains - #2511
Conversation
There was a problem hiding this comment.
Solid implementation: registrable-domain + punycode matching, three-state mode, and good coverage across the three self-serve entry points.
Improvement: Magic-link endpoint docstring says it always returns 200, but enforce mode now returns 400 for disposable domains (new-user branch).
Improvement: Consider making AUTH_BLOCK_DISPOSABLE_EMAILS case-insensitive for OFF/LOG/ENFORCE (validator already handles other string normalizations).
Found 2 issues (0 critical, 2 improvements).
| @@ -916,6 +923,17 @@ def request_magic_link( | |||
| is_new_user = False | |||
|
|
|||
| if not user: | |||
There was a problem hiding this comment.
Improvement: request_magic_link()’s docstring still says “Always returns 200 to prevent email enumeration”, but with this new DisposableEmailError path it can now return 400 (in enforce mode, and only for new users).
Fix: update the docstring to match the new contract, or (if you want to preserve enumeration-safety) keep returning 200 here and only log/drop the request.
There was a problem hiding this comment.
Fixed the docstring in ab55a81 — it now states the actual contract rather than claiming an unconditional 200.
On your second option (keep returning 200 and log/drop): still genuinely open, and it is called out in the PR description under "Additional Context" as a deliberate departure. The constraint is that the check has to sit inside the if not user: branch or existing accounts on a disposable domain get locked out, which the issue lists as a non-goal — so the choice is between a clearer 400 that leaks existence for refused domains, or a silent 200 where a false-positive legitimate user waits for mail that never arrives. Leaving it as a 400 for now and flagging it for a human reviewer to weigh in on.
There was a problem hiding this comment.
Fixed in 9e097d6 — docstring now reflects the 400-in-enforce exception for disposable domains on the sign-up branch.
There was a problem hiding this comment.
Thanks — docstring fix sounds good.
On the 200 vs 400: given enforce is an explicit “policy rejection” mode (and you’re already defaulting to log to measure false positives first), I’m okay with the current 400-on-new-user behavior. It’s a small enumeration leak limited to domains you’re refusing anyway, and it avoids the “wait for an email that will never arrive” UX, especially if the blocklist ever has a false positive.
| registration_enabled: bool = Field(default=True, alias="AUTH_REGISTRATION_ENABLED") | ||
| # Starts at "log" so we can measure the false-positive rate of the community | ||
| # blocklist from logs for a week before it starts turning real users away. | ||
| block_disposable_emails: Literal["off", "log", "enforce"] = Field( |
There was a problem hiding this comment.
Improvement: block_disposable_emails is a Literal["off","log","enforce"], but the validator only lowercases for legacy boolean synonyms. If someone sets AUTH_BLOCK_DISPOSABLE_EMAILS=LOG/ENFORCE/OFF (common for env vars), it’ll fail validation.
Fix: in
_accept_boolean_form, lowercase any string first (likebackend_envdoes), then return the lowered value if it’s already one of the allowed literals.
There was a problem hiding this comment.
Fixed in a5cf549. Confirmed it was worse than "fails validation" — AUTH_BLOCK_DISPOSABLE_EMAILS=ENFORCE failed AuthSettings construction outright, so it would have taken the app down at startup rather than falling back to a default. The validator now lowercases any string, following the backend_env precedent you pointed at. A real typo (enfroce) still raises instead of silently defaulting, covered by a test.
There was a problem hiding this comment.
Makes sense — thanks for confirming the failure mode and fixing it by lowercasing/stripping any string. Agree it’s better to still raise on real typos (and the test coverage for that is exactly what I’d want). This resolves my concern here.
Screen self-serve sign-ups against the disposable-email-domains community list, merged with an in-repo supplement file and a comma-separated env override. Applies to password register, magic link, and OAuth first login; admin invites (POST /users/) stay unscreened. Matching strips subdomain labels down to the registrable domain and compares the punycode form, since validate_and_normalize_email returns the Unicode domain and would never match the ASCII entries the upstream list stores. AUTH_BLOCK_DISPOSABLE_EMAILS is three-state and defaults to "log" so the false-positive rate of the community list can be measured from logs before it starts rejecting real users. The boolean spellings still work. Refs #2237 Signed-off-by: Nicolai Bohn <nicolai@rhesis.ai>
Unit tests for registrable-domain matching, IDN/punycode normalisation, the custom list file, the env override, and the off/log/enforce modes. Route tests check each self-serve path is wired to the screener, that existing users on a disposable domain are not locked out, and that admin invites are not screened. Refs #2237 Signed-off-by: Nicolai Bohn <nicolai@rhesis.ai>
AUTH_BLOCK_DISPOSABLE_EMAILS=ENFORCE failed Literal validation and took AuthSettings construction down at startup. Lowercase any string value, the way backend_env already does. A genuine typo still fails loudly rather than falling back to a default. Refs #2237 Signed-off-by: Nicolai Bohn <nicolai@rhesis.ai>
Refs #2237 Signed-off-by: Nicolai Bohn <nicolai@rhesis.ai>
The endpoint no longer always returns 200: a disposable domain under enforce mode returns 400 on the sign-up branch, which does reveal account existence for those domains. Say so instead of claiming otherwise. Refs #2237 Signed-off-by: Nicolai Bohn <nicolai@rhesis.ai>
Non-secret config, so it belongs in the ConfigMap rather than external-secrets — same treatment ARCHITECT_HELP_ARTICLE_URLS got in #2492. Blocking a domain becomes a values change plus a rollout, with no image rebuild and no Secret Manager entry. Refs #2237 Signed-off-by: Nicolai Bohn <nicolai@rhesis.ai>
1dbbb98 to
1089c4c
Compare
There was a problem hiding this comment.
Looks good. Nice integration of the upstream disposable-domain list + custom/env overlays, with registrable-domain + punycode matching and solid wiring across password/magic-link/OAuth flows. The follow-up fixes for the magic-link docstring and case-insensitive AUTH_BLOCK_DISPOSABLE_EMAILS handling address the earlier concerns. Ship it.
Purpose
Closes #2237. Disposable-email domains are used to create throwaway accounts that spam or abuse the platform. Sign-up currently validates email format and MX deliverability only, so temporary inboxes like
mailinator.compass —mailinator.comhas real MX records and clears the existing check.What Changed
app/auth/disposable_email.py. Merges thedisposable-email-domainscommunity list (~8.2k domains, MIT, near-daily releases) with an in-repodisposable_domains_custom.txtand a comma-separated env override, cached in onefrozenset.mail.smtp.mailinator.comhits themailinator.comentry — otherwise any subdomain bypasses the list. Stripping stops at two labels so a bare TLD slipping into the list can't block every address under it. Comparison uses.ascii_domain, not.normalized: the latter returns the Unicode form, sofoo@münchen.dewould never match anxn--mnchen-3ya.deentry.EmailProvider.register(),request_magic_link()(which had no domain validation at all), and the new-user branch offind_or_create_user_from_auth(). Admin invites viaPOST /users/are not screened.AUTH_BLOCK_DISPOSABLE_EMAILSis three-state —off/log/enforce— defaulting tolog. Amode="before"validator still accepts the boolean spellings from the issue (true/false/1/0/yes/no), soAUTH_BLOCK_DISPOSABLE_EMAILS=falsefully disables the check as specified.DisposableEmailError(ValueError). SubclassingValueErrormeans the call sites that already map a bad address to a 400 needed no change, while the OAuth callback catches it specifically so a policy rejection isn't logged as an error with a traceback.AUTH_BLOCK_DISPOSABLE_EMAILSandAUTH_DISPOSABLE_EMAIL_EXTRA_DOMAINSadded to.env.exampleanddocker-compose.yml.Additional Context
Two deliberate departures from the issue, both worth a look during review.
1. Default is
log, notenforce, which contradicts acceptance criterion 1 on day one. Community lists occasionally carry domains real users have, and learning that false-positive rate from a log query beats learning it from a support ticket. Log mode records the redacted address, the registrable domain, the matched entry, and which sign-up path it came through. Proposal: run it for a week, grep the logs, then flip toenforce. Everything needed for the flip is already here.2. The magic-link 400 is a narrow enumeration leak. The screener has to sit inside the
if not user:branch, or existing accounts on a disposable domain get locked out — which the issue lists as a non-goal. That means a disposable-domain address gets a 400 when unregistered and a 200 when registered, on an endpoint that is otherwise strictly enumeration-safe. It only leaks for domains we're refusing to register anyway, so I took the clearer error message over the silent 200, but that's a judgement call and easy to reverse. Flagged with a comment at the call site.On the review discussion in the issue:
kubernetes/baseuses external-secrets, so changing it is a secret update plus a rollout restart — no image build, no code deploy. It does what the issue wanted..conf: there is no Celery beat in this repo, so it means new scheduled-job infra plus a runtime network fetch in an auth path, to buy back a lag of a couple of days. Skipped the DB-table-plus-poll for the custom list too — right answer once there's an admin UI, but no audit trail has been asked for yet. The disposable-MX-target denylist is a good idea that needs data first; the log-only phase should tell us whether it's worth it.Testing
37 new tests, all passing. Nothing else in
tests/backend/auth/ortests/backend/routes/test_auth.pybroke (485 pass).cd apps/backend uv run pytest ../../tests/backend/auth/test_disposable_email.py -v uv run pytest ../../tests/backend/routes/test_auth.py::TestDisposableEmailSignupBlocking -vVerified by hand against a local backend. In the default
logmode, all three sign-ups return 200 and the two disposable ones produce a log line while the legitimate one produces none:In
enforcemode:The existing-user exemption, same domain back to back:
And the admin invite on the same domain the sign-up paths just refused:
No tracebacks logged for any of the rejections. The OAuth path can't be driven over HTTP locally (no provider credentials in local
.env), so it's covered by a test callingfind_or_create_user_from_authdirectly with aGOOGLEAuthUserrather than by hand.