Opened 17 years ago

Closed 11 years ago

Last modified 11 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: dev
Severity: Normal Keywords: auth email validator ip
Cc: maix42@…, jay@…, Erin 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 Erin Kelly 17 years ago.
localhost_email_validation_test_case.diff (641 bytes ) - added by skabber 16 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 Erin Kelly 16 years ago.
Test case for 4833.diff
4833-2.diff (3.5 KB ) - added by Claude Paroz 13 years ago.
Validate email addresses with localhost as domain
4833-3.diff (3.5 KB ) - added by Claude Paroz 13 years ago.
Make idn validation also pass
4833-4.diff (4.1 KB ) - added by Claude Paroz 13 years ago.
Refactored patch to not inherit from RegexValidator
4833-5.diff (4.2 KB ) - added by Claude Paroz 13 years ago.
Make domain whitelist parametrizable
4833-6.diff (4.4 KB ) - added by Łukasz Rekucki 11 years ago.
Updated patch to master. Tests pass on 2.7 and 3.2.

Download all attachments as: .zip

Change History (37)

comment:1 by Simon G. <dev@…>, 17 years ago

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 by Russell Keith-Magee, 17 years ago

Triage Stage: Design decision neededAccepted

comment:3 by deepak <deep.thukral@…>, 17 years ago

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 by Malcolm Tredinnick, 17 years ago

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 by deepak <deep.thukral@…>, 17 years ago

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.

by Erin Kelly, 17 years ago

Attachment: 4833.diff added

comment:6 by Erin Kelly, 17 years ago

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 by deepak <deep.thukral@…>, 17 years ago

Needs tests: set

It looks fine, but please include regression test.

comment:8 by maix, 16 years ago

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 by anonymous, 16 years ago

Cc: maix42@… added

by skabber, 16 years ago

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

comment:10 by skabber, 16 years ago

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

by Erin Kelly, 16 years ago

Attachment: 4833_test.diff added

Test case for 4833.diff

comment:11 by Erin Kelly, 16 years ago

Cc: ian.g.kelly@… added
Owner: changed from Erin 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 by MichaelBishop, 16 years ago

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 by Erin Kelly, 16 years ago

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

comment:14 by jaboja, 14 years ago

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

Triage Stage: AcceptedDesign decision needed

comment:16 by jaboja, 14 years ago

Owner: jaboja removed
Status: assignednew

comment:17 by Malcolm Tredinnick, 14 years ago

Triage Stage: Design decision neededAccepted

comment:18 by Gabriel Hurley, 13 years ago

Severity: Normal
Type: Bug

comment:19 by Claude Paroz, 13 years ago

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...

by Claude Paroz, 13 years ago

Attachment: 4833-2.diff added

Validate email addresses with localhost as domain

comment:20 by Preston Holmes, 13 years ago

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.']

by Claude Paroz, 13 years ago

Attachment: 4833-3.diff added

Make idn validation also pass

comment:21 by Claude Paroz, 13 years ago

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

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

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

Triage Stage: Ready for checkinAccepted

comment:25 by Preston Holmes, 13 years ago

Patch needs improvement: set

See Alex's notes

by Claude Paroz, 13 years ago

Attachment: 4833-4.diff added

Refactored patch to not inherit from RegexValidator

by Claude Paroz, 13 years ago

Attachment: 4833-5.diff added

Make domain whitelist parametrizable

comment:26 by Claude Paroz, 13 years ago

Patch needs improvement: unset

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

by Łukasz Rekucki, 11 years ago

Attachment: 4833-6.diff added

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

comment:27 by Łukasz Rekucki, 11 years ago

Triage Stage: AcceptedReady for checkin

comment:28 by Claude Paroz <claude@…>, 11 years ago

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

In 4e2e8f39d19d79a59c2696b2c40cb619a54fa745:

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

comment:29 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

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