Opened 8 years ago

Closed 2 years ago

Last modified 2 years ago

#4833 closed Bug (fixed)

auth superuser setup rejects valid @localhost e-mail address

Reported by: vmole <steveg@…> Owned by: Claude Paroz <claude@…>
Component: Validators Version: master
Severity: Normal Keywords: auth email validator ip
Cc: maix42@…, jay@…, ikelly Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

You just installed Django's auth system, which means you don't have any superusers defined.
Would you like to create one now? (yes/no): yes
Username (Leave blank to use 'steveg'): 
E-mail address: steveg@localhost
Error: That e-mail address is invalid.

Uh, no it's not. It's a perfectly sensible address for local singleuser development server.

Attachments (8)

4833.diff (773 bytes) - added by ikelly 8 years ago.
localhost_email_validation_test_case.diff (641 bytes) - added by skabber 8 years ago.
I attached a simple test case for localhost email validation but the patch 4833.diff causes it to fail.
4833_test.diff (515 bytes) - added by ikelly 7 years ago.
Test case for 4833.diff
4833-2.diff (3.5 KB) - added by claudep 4 years ago.
Validate email addresses with localhost as domain
4833-3.diff (3.5 KB) - added by claudep 4 years ago.
Make idn validation also pass
4833-4.diff (4.1 KB) - added by claudep 4 years ago.
Refactored patch to not inherit from RegexValidator
4833-5.diff (4.2 KB) - added by claudep 4 years ago.
Make domain whitelist parametrizable
4833-6.diff (4.4 KB) - added by lrekucki 3 years ago.
Updated patch to master. Tests pass on 2.7 and 3.2.

Download all attachments as: .zip

Change History (37)

comment:1 Changed 8 years ago by Simon G. <dev@…>

  • Component changed from Uncategorized to Validators
  • Needs documentation unset
  • Needs tests unset
  • Owner changed from jacob to adrian
  • Patch needs improvement unset
  • Summary changed from auth superuser setup rejects valid e-mail address to auth superuser setup rejects valid @localhost e-mail address
  • Triage Stage changed from Unreviewed to Design decision needed

To fix this, we'll need to modify the regex in isValidEmail in django/core/validators.py, but quite frankly, that regex scares me.

comment:2 Changed 8 years ago by russellm

  • Triage Stage changed from Design decision needed to Accepted

comment:3 Changed 8 years ago by deepak <deep.thukral@…>

Don't think it is possible to do with re. Instead we should define a constant list.
allowed_domain = ['local', 'localhost', 'localhost.localdomain']

comment:4 Changed 8 years ago by mtredinnick

If we're going to fix this, we should do it properly. Single word domains are legal, not just restricted to localhost. So a list is not a good approach.

comment:5 Changed 8 years ago by deepak <deep.thukral@…>

Django is using common validators, it'd be problematic for Django apps which are running online. Either we shouldnt fix it or we should allow ONLY some specific domains.

Changed 8 years ago by ikelly

comment:6 Changed 8 years ago by ikelly

  • Has patch set
  • Keywords auth email validator added
  • Version changed from 0.96 to SVN

4833.diff solves the problem in a generic fashion and also accepts a superset of the currently accepted domains.

comment:7 Changed 8 years ago by deepak <deep.thukral@…>

  • Needs tests set

It looks fine, but please include regression test.

comment:8 Changed 8 years ago by maix

It also rejects IDNs (Internationalized Domain Name, domain with characters outside ascii) and umlauts in the first part of the email address (such as äöü (German, for example)). Maybe it should be chang to just test if there's an @ inside.

comment:9 Changed 8 years ago by anonymous

  • Cc maix42@… added

Changed 8 years ago by skabber

I attached a simple test case for localhost email validation but the patch 4833.diff causes it to fail.

comment:10 Changed 8 years ago by skabber

  • Cc jay@… added
  • Needs tests unset
  • Owner changed from nobody to ikelly
  • Patch needs improvement set

Changed 7 years ago by ikelly

Test case for 4833.diff

comment:11 Changed 7 years ago by ikelly

  • Cc ian.g.kelly@… added
  • Owner changed from ikelly to nobody

The test case uploaded by skabber isn't set up correctly as a doctest, so I'm not sure how it was expected to pass. 4833_test.diff adds a test for this in modeltests/validation, and it passes.

I don't know enough about IDNs to fix the problem pointed out by maix, although just checking for an @ seems wrong to me.

comment:12 Changed 7 years ago by MichaelBishop

From my understanding of the RFC doc, as below, 'steve@localhost' is not spec. In addition, these changes would break the current validation system which should be created with the production environment in mind. These changes would allow both 'steve@…' and 'steve@domaincom' to be used as a valid email address. Note the missing period in the second email address.

Two ideas for reaching a middle ground:

  1. Check for usual localhost domains like ['local', 'localhost', 'localhost.localdomain']. This would allow for 'steve@localhost' while still invalidating 'steve@domaincom'.
  2. Allow the email address to be empty.

RFC 2821 (http://tools.ietf.org/html/rfc2821):

3.6 Domains

   Only resolvable, fully-qualified, domain names (FQDNs) are permitted
   when domain names are used in SMTP.  In other words, names that can
   be resolved to MX RRs or A RRs (as discussed in section 5) are
   permitted, as are CNAME RRs whose targets can be resolved, in turn,
   to MX or A RRs.  Local nicknames or unqualified names MUST NOT be
   used.  There are two exceptions to the rule requiring FQDNs:

   -  The domain name given in the EHLO command MUST BE either a primary
      host name (a domain name that resolves to an A RR) or, if the host
      has no name, an address literal as described in section 4.1.1.1.

   -  The reserved mailbox name "postmaster" may be used in a RCPT
      command without domain qualification (see section 4.1.1.3) and
      MUST be accepted if so used.

comment:13 Changed 7 years ago by ikelly

  • Cc ikelly added; ian.g.kelly@… removed

comment:14 Changed 6 years ago by jaboja

  • Keywords ip added
  • Owner changed from nobody to jaboja
  • Status changed from new to assigned

What is more it rejects also e-mails with IPs after @. I totally do not understand why, as also in production environment, even if localhost is not threated as a valid value, there is no point to threat emails on servers woithout associated domain name as invalid.

comment:15 Changed 6 years ago by jaboja

  • Triage Stage changed from Accepted to Design decision needed

comment:16 Changed 6 years ago by jaboja

  • Owner jaboja deleted
  • Status changed from assigned to new

comment:17 Changed 5 years ago by mtredinnick

  • Triage Stage changed from Design decision needed to Accepted

comment:18 Changed 4 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to Bug

comment:19 Changed 4 years ago by claudep

  • Easy pickings unset
  • Patch needs improvement unset
  • UI/UX unset

The following patch takes a slightly different approach. I'm splitting email addresses at @ in all cases before validation, so we can give special treatment to domain (as it was already done for IDN validation).

It remains unclear for me if the fact of giving EmailValidator a tuple of regexes is breaking some backwards compatibility (didn't find EmailValidator in docs). Anyway, that's just my proposal and it may still need some work...

Changed 4 years ago by claudep

Validate email addresses with localhost as domain

comment:20 Changed 4 years ago by ptone

  • Patch needs improvement set

The most recent patch has 2 problems:

It is currently failing to apply to trunk - but updating that is pretty mild.

The bigger problem is that the replacement regex tuple causes a current test to fail, because the patch would consider the following email invalid:

local@…äöüßabc.part.com

which is a valid international address.

ERROR: test_emailfield_1 (regressiontests.forms.tests.fields.FieldsTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/preston/Projects/code/forks/django/tests/regressiontests/forms/tests/fields.py", line 509, in test_emailfield_1
    self.assertEqual(u'local@domain.with.idn.xyz\xe4\xf6\xfc\xdfabc.part.com', f.clean('local@domain.with.idn.xyzäöüßabc.part.com'))
  File "/Users/preston/Projects/code/forks/django/django/forms/fields.py", line 455, in clean
    return super(EmailField, self).clean(value)
  File "/Users/preston/Projects/code/forks/django/django/forms/fields.py", line 153, in clean
    self.run_validators(value)
  File "/Users/preston/Projects/code/forks/django/django/forms/fields.py", line 142, in run_validators
    raise ValidationError(errors)
ValidationError: [u'Enter a valid e-mail address.']

Changed 4 years ago by claudep

Make idn validation also pass

comment:21 Changed 4 years ago by claudep

  • Patch needs improvement unset

Thanks ptone for catching this. The regex was right, but there was a missing "return" in my code when the validation succeeded.

comment:22 Changed 4 years ago by ptone

  • Triage Stage changed from Accepted to Ready for checkin

The current patch applies cleanly, passes tests, and addresses the specific problem of the ticket. There are potentially other valid needs for other whitelisted local domains, but in the specific case of this narrow scope for the super user - a simple, specific check for localhost seems valid.

Some minor concern remains about the general test coverage for email validation, and whether the new regex will hit trouble with edge cases not considered in the tests.

This patch does not seem to warrant any change in the documentation.

I'll let a core dev consider the weight of that concern and will mark this RFC.

comment:23 Changed 4 years ago by Alex

Bumping this back to accepted, subclassing regexvalidator, but then making regex be a tuple rather than the usual format doesn't seem like good design. Either this shouldn't subclass regex validator, or it should communicate the second regex in a different way (not calling super __call__ at any point is another good indicator of that).

comment:24 Changed 4 years ago by Alex

  • Triage Stage changed from Ready for checkin to Accepted

comment:25 Changed 4 years ago by ptone

  • Patch needs improvement set

See Alex's notes

Changed 4 years ago by claudep

Refactored patch to not inherit from RegexValidator

Changed 4 years ago by claudep

Make domain whitelist parametrizable

comment:26 Changed 4 years ago by claudep

  • Patch needs improvement unset

Alex's comment was accurate. Inheriting from RegexValidator didn't bring anything. Hopefully it is in better shape now.

Changed 3 years ago by lrekucki

Updated patch to master. Tests pass on 2.7 and 3.2.

comment:27 Changed 3 years ago by lrekucki

  • Triage Stage changed from Accepted to Ready for checkin

comment:28 Changed 2 years ago by Claude Paroz <claude@…>

  • Owner set to Claude Paroz <claude@…>
  • Resolution set to fixed
  • Status changed from new to closed

In 4e2e8f39d19d79a59c2696b2c40cb619a54fa745:

Fixed #4833 -- Validate email addresses with localhost as domain

comment:29 Changed 2 years ago by Aymeric Augustin <aymeric.augustin@…>

In 424eb67867162032d92e0bfe3403f051765de805:

Fixed validation of email addresses when the local part contains an @.

See also BaseUserManager.normalize_email -- it uses rsplit.

Refs #4833.

Note: See TracTickets for help on using tickets.
Back to Top