Skip to content

Make EmailValidator allowed_domains actually restrict domains#2252

Open
uttam12331 wants to merge 1 commit into
tortoise:developfrom
uttam12331:fix-email-validator-allowed-domains
Open

Make EmailValidator allowed_domains actually restrict domains#2252
uttam12331 wants to merge 1 commit into
tortoise:developfrom
uttam12331:fix-email-validator-allowed-domains

Conversation

@uttam12331

Copy link
Copy Markdown

Description

EmailValidator(allowed_domains=[...]) never restricts anything. The domain check was:

if domain_part not in self.allowed_domains and not self._validate_domain_part(domain_part):
    raise InvalidEmailAddress()

Rejection requires both operands to be true, so any syntactically valid domain makes the second operand False and 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:

>>> v = EmailValidator(allowed_domains=["example.com"])
>>> v("attacker@evil.com")            # accepted
>>> v("user@totally-unrelated.co.uk") # accepted

This contradicts the documented behaviour:

:param allowed_domains: List of allowed domain names. If provided, only emails from these domains will be accepted.

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_domains instead:

if self.allowed_domains:
    if domain_part not in self.allowed_domains:
        raise InvalidEmailAddress()
elif not self._validate_domain_part(domain_part):
    raise InvalidEmailAddress()

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 existing test_email_validator_invalid_allowed_domains only 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 check and ruff format --check are clean.

No changelog entry: EmailValidator is 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:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added the changelog accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant