Make EmailValidator allowed_domains actually restrict domains#2252
Open
uttam12331 wants to merge 1 commit into
Open
Make EmailValidator allowed_domains actually restrict domains#2252uttam12331 wants to merge 1 commit into
uttam12331 wants to merge 1 commit into
Conversation
The domain check was 'domain_part not in allowed_domains and not _validate_domain_part(domain_part)', so rejection required both operands. Any syntactically valid domain made the second operand False, meaning the allowlist could only ever widen acceptance and never restrict: EmailValidator(allowed_domains=['example.com']) accepted attacker@evil.com, contradicting the documented behaviour that 'only emails from these domains will be accepted'. Branch on allowed_domains instead, so a configured allowlist is enforced while allowlisted entries still bypass the domain syntax check (e.g. allowed_domains=['localhost']). Also drops a leftover debug print() in _domain_regex that wrote to stdout on first use.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
EmailValidator(allowed_domains=[...])never restricts anything. The domain check was:Rejection requires both operands to be true, so any syntactically valid domain makes the second operand
Falseand the address is accepted regardless of the allowlist. The allowlist can therefore only ever widen acceptance (letting through domains that fail the syntax regex), never restrict:This contradicts the documented behaviour:
With the default
allowed_domains=[]the first operand is always true, so the expression degrades to plain syntax validation — which is why the default path works and hid this.This branches on
allowed_domainsinstead:An allowlist is now enforced, while allowlisted entries still bypass the domain syntax check, so
allowed_domains=["localhost"]keeps working.Also removes a leftover
print("evaluating domain regex!!!")in_domain_regex, which wrote to stdout the first time each validator checked a domain.Motivation and Context
No open issue. The validator is advertised as supporting "domain allowlisting for restricting to specific domains", and code that relies on it for restriction is silently not restricting. Both problems are in code added for 1.1.8 and not yet released, so they can be fixed before the release rather than shipped.
How Has This Been Tested?
pytest tests/test_validators.py— 71 passed (69 existing, unchanged, plus 2 added).Added two tests:
test_email_validator_rejects_domain_outside_allowed_domains— a well-formed address on a non-allowlisted domain must raise. This fails on the current code and passes with the fix. The existingtest_email_validator_invalid_allowed_domainsonly fed malformed addresses (user@,user@invalid..com), which is why the gap wasn't caught.test_email_validator_allowed_domains_bypass_domain_syntax— guards the preserved behaviour that an allowlisted domain (localhost) is still accepted.ruff checkandruff format --checkare clean.No changelog entry:
EmailValidatoris itself listed under Added in the unreleased 1.1.8 section, so this ships as part of that feature rather than as a fix to released behaviour. Happy to add one if you'd prefer.Checklist: