Opened 9 years ago

Closed 4 years ago

Last modified 4 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@…, Ian Kelly 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 Ian Kelly 9 years ago.
localhost_email_validation_test_case.diff (641 bytes) - added by skabber 9 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 Ian Kelly 9 years ago.
Test case for 4833.diff
4833-2.diff (3.5 KB) - added by Claude Paroz 5 years ago.
Validate email addresses with localhost as domain
4833-3.diff (3.5 KB) - added by Claude Paroz 5 years ago.
Make idn validation also pass
4833-4.diff (4.1 KB) - added by Claude Paroz 5 years ago.
Refactored patch to not inherit from RegexValidator
4833-5.diff (4.2 KB) - added by Claude Paroz 5 years ago.
Make domain whitelist parametrizable
4833-6.diff (4.4 KB) - added by Łukasz Rekucki 4 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 9 years ago by Simon G. <dev@…>

Component: UncategorizedValidators
Owner: changed from Jacob to Adrian Holovaty
Summary: auth superuser setup rejects valid e-mail addressauth superuser setup rejects valid @localhost e-mail address
Triage Stage: UnreviewedDesign 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 9 years ago by Russell Keith-Magee

Triage Stage: Design decision neededAccepted

comment:3 Changed 9 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 9 years ago by Malcolm Tredinnick

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 9 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 9 years ago by Ian Kelly

Attachment: 4833.diff added

comment:6 Changed 9 years ago by Ian Kelly

Has patch: set
Keywords: auth email validator added
Version: 0.96SVN

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

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

Needs tests: set

It looks fine, but please include regression test.

comment:8 Changed 9 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 9 years ago by anonymous

Cc: maix42@… added

Changed 9 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 9 years ago by skabber

Cc: jay@… added
Needs tests: unset
Owner: changed from nobody to Ian Kelly
Patch needs improvement: set

Changed 9 years ago by Ian Kelly

Attachment: 4833_test.diff added

Test case for 4833.diff

comment:11 Changed 9 years ago by Ian Kelly

Cc: ian.g.kelly@… added
Owner: changed from Ian Kelly 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 9 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 9 years ago by Ian Kelly

Cc: Ian Kelly added; ian.g.kelly@… removed

comment:14 Changed 7 years ago by jaboja

Keywords: ip added
Owner: changed from nobody to jaboja
Status: newassigned

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 7 years ago by jaboja

Triage Stage: AcceptedDesign decision needed

comment:16 Changed 7 years ago by jaboja

Owner: jaboja deleted
Status: assignednew

comment:17 Changed 6 years ago by Malcolm Tredinnick

Triage Stage: Design decision neededAccepted

comment:18 Changed 6 years ago by Gabriel Hurley

Severity: Normal
Type: Bug

comment:19 Changed 5 years ago by Claude Paroz

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 5 years ago by Claude Paroz

Attachment: 4833-2.diff added

Validate email addresses with localhost as domain

comment:20 Changed 5 years ago by Preston Holmes

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 5 years ago by Claude Paroz

Attachment: 4833-3.diff added

Make idn validation also pass

comment:21 Changed 5 years ago by Claude Paroz

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 5 years ago by Preston Holmes

Triage Stage: AcceptedReady 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 5 years ago by Alex Gaynor

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 5 years ago by Alex Gaynor

Triage Stage: Ready for checkinAccepted

comment:25 Changed 5 years ago by Preston Holmes

Patch needs improvement: set

See Alex's notes

Changed 5 years ago by Claude Paroz

Attachment: 4833-4.diff added

Refactored patch to not inherit from RegexValidator

Changed 5 years ago by Claude Paroz

Attachment: 4833-5.diff added

Make domain whitelist parametrizable

comment:26 Changed 5 years ago by Claude Paroz

Patch needs improvement: unset

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

Changed 4 years ago by Łukasz Rekucki

Attachment: 4833-6.diff added

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

comment:27 Changed 4 years ago by Łukasz Rekucki

Triage Stage: AcceptedReady for checkin

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

Owner: set to Claude Paroz <claude@…>
Resolution: fixed
Status: newclosed

In 4e2e8f39d19d79a59c2696b2c40cb619a54fa745:

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

comment:29 Changed 4 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