valid_email_address() is not RFC compliant [#265548]

A collegue of mine, Robert, told me that Drupal's valid_email_address() function allowes domains with a trailing dot(.) like 'x@x.com.'.

I've attached a patch that fixes the regex for the domain part of the email address.

I've also altered the return value to be a boolean instead of an integer, just like the function description says... (this is in fact a issue with a couple of other functions, valid_url() for example, I'll create separate issues for those functions)

I've also written a simpletest, but since common.test still does not exist, I'll post it here. (more info: http://drupal.org/node/151902#comment-863664)

class CommonValidEmailAddressTestCase extends DrupalWebTestCase {

  /**
   * Implementation of getInfo().
   */
  function getInfo() {
    return array(
      'name' => t('Valid email address test'),
      'description' => t('Check predefined email addresses and compare the result with the expected result.'),
      'group' => t('System')
    );
  }

  /**
   * Implementation of setUp().
   */
  function setUp() {
    $this->test_cases = array( // '<user@domain.ext>' => (bool)'<expected result>',
      'x@x.com'   => TRUE,
      'x@x-x.com' => TRUE,
      'x@x.co.uk' => TRUE,
      'x@x.com.'  => FALSE,
      'x@x.com..' => FALSE,
      'x@-x.com'  => FALSE,
      'x@x-.com'  => FALSE,
      'x@x-.'     => FALSE,
      '@x.com'    => FALSE,
      'x@.x.com'  => FALSE,
      'x@x.com-'  => FALSE,
      'x@com'     => FALSE,
    );
    parent::setUp();
  }

  /**
   * testCommonValidEmailAddress
   */
  function testCommonValidEmailAddress() {
    foreach ($this->test_cases as $address => $expected) {
      if ($expected === TRUE) {
        $this->assertTrue(
          (bool)valid_email_address($address),
          "Checking valid email addres: '". $address ."' %s"
        );
      }
      else {
        $this->assertFalse(
          (bool)valid_email_address($address),
          "Checking invalid email addres: '". $address ."' %s"
        );
      }
    }
  }
}

Comment	File	Size	Author
#31	valid_email_address-n265548-d6.patch	11.68 KB	DamienMcKenna
#31
#22	valid_email_address.patch	21.81 KB	JeremyFrench
#22
#19	isemail.zip	19.68 KB	dominicsayers
#6	valid_email_address.patch	2.94 KB	MrHaroldA
#6
#5	commontest.patch	1.84 KB	catch
#5
#3	commontest.patch	1.8 KB	catch
#3
	valid_email_address.patch	919 bytes	MrHaroldA

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

MrHaroldA CreditAttribution: MrHaroldA commented 2 June 2008 at 11:31

Status:

Needs review

» Closed (works as designed)

Just checked the rfc and did some tests and I've discovered that domains may end with a dot(.). So 'x@x.com.' is a valid email address...

What to do with the unit test?

Comment #2

catch

he/him

English

CreditAttribution: catch commented 3 June 2008 at 11:20

Title:	valid_email_address() allows trailing dot(.) in domain	» Tests for valid_email_address() (and add common.test)
Category:	bug	» task
Status:	Closed (works as designed)	» Needs review

The test is still good (although it fails on foo@example). I opened a separate issue over at http://drupal.org/node/265563 with a stub for common.test but Dries didn't like the idea of a stub.

So, I've taken the test case and stuck it in a patch with some minor fixes, this also adds the common.test file in CVS.

Comment #3

catch

he/him

English

CreditAttribution: catch commented 3 June 2008 at 11:22

File	Size
commontest.patch	1.8 KB

oops.

Comment #4

Damien Tournoud CreditAttribution: Damien Tournoud commented 3 June 2008 at 12:30

About #1, there is no real life situations where you want to accept email addresses with a domain ending with a dot.

By the way, we should also enforce the domain part to be a real domain name, and not an IP address.

Ok, that's not a valid email address according to the RFC 2822, but that the addresses forms that should be accepted by real-life websites (both internet and intranet ones, by the way).

About #3, I think the last test should be foo@example and not foo@com

Comment #5

catch

he/him

English

CreditAttribution: catch commented 3 June 2008 at 13:20

File	Size
commontest.patch	1.84 KB

That all makes sense to me, attached patch makes foo@example.com. expect FALSE again, and I added an e-mail with an IP address as the domain.

Comment #6

MrHaroldA CreditAttribution: MrHaroldA commented 3 June 2008 at 21:14

File	Size
valid_email_address.patch	2.94 KB

The attached patch replaces the valid_email_address.patch from above and passes on all tests listed in common.test.patch.

I've also added empty address, single letter, single digit, four letter extension (.info) and IPv6 addresses to the test. (and corrected the spacing in the array;)

Edit:
Replaced the 2 patches by a single one. NOTE: this will not apply to an existing common.test! See http://drupal.org/node/265563 for more info...

Comment #7

catch

he/him

English

CreditAttribution: catch commented 3 June 2008 at 16:38

Title:

Tests for valid_email_address() (and add common.test)

» Replacement code and tests for valid_email_address() (and add common.test)

This looks good to me, although it should probably all be rolled into one patch.

Comment #8

MrHaroldA CreditAttribution: MrHaroldA commented 3 June 2008 at 21:15

@catch: look up! ;)

Comment #9

catch

he/him

English

CreditAttribution: catch commented 4 June 2008 at 09:18

Status:

Needs review

» Reviewed & tested by the community

Patch works well and it successfully created common.test on my system, so RTBC.

Comment #10

catch

he/him

English

CreditAttribution: catch commented 11 June 2008 at 12:07

Status:

Reviewed & tested by the community

» Needs work

Now that there's a common.test, this no longer applies.

Additionally, I just saw this old issue: http://drupal.org/node/90902 - the function allows e-mails like this but do we want to add foo'bar@example.com to the test cases? (I hacked one in and it passed).

Comment #11

MrHaroldA CreditAttribution: MrHaroldA commented 11 June 2008 at 12:22

I'll update the patch and add a test case for a email address like !#$%&'*+-/=?^_`.{|}~@example.com to it... (as soon as I find the time; volunteers?)

Comment #12

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna commented 6 April 2009 at 11:50

Category:

task

» bug

MadHarold: any update for your patch? Given this allows users to register using invalid email addresses, this should be considered a bug.

Comment #13

Berdir

German

Switzerland

CreditAttribution: Berdir commented 6 April 2009 at 12:25

See #413852: Change contact.test to correctly test email validation on PHP 5.3. D7 does now use filter_validate() to validate e-mails.

Also, filter_validate() validates e-mails inconsistent (see php bug report in above issue), which means that we can't test for user@site, because it gives different results based on the php version.

As a side note, because I reported the change as a "bug", filter_validate() does currently _not_ allow "user@site." in php-5.3 (snapshots). After reading this, I'm not sure anymore which is correct, so you may want to re-open my bug report.

But anyway, because the validation is currently different based on the php version, we can't write tests for it. I'd even say that it's not our job to test filter_validate().

Comment #14

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna commented 6 April 2009 at 15:05

Two parts to this:

* Drupal 7 now uses a built-in function in PHP 5.2 (filter_var) but it isn't RFC-compliant.

* Here's a page which describes in incredible detail a regex to use to match email addresses, which Drupal should be using instead of its own lame attempt:
http://www.regular-expressions.info/email.html

Comment #15

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna commented 6 April 2009 at 15:50

I would consider the following page to be pretty authoritative:
* http://www.dominicsayers.com/isemail/

Comment #16

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna commented 6 April 2009 at 16:18

Title:	Replacement code and tests for valid_email_address() (and add common.test)	» valid_email_address() is not RFC compliant
Issue tags:		+RFC compliance, +PHP bug

Updating the issue title to highlight the key issue - RFC compliance. This is a bug in all versions of Drupal, and even v7 due to a bug in PHP. While it is understandable that we shouldn't be expected to fix bugs in the underlying language, that it is a bug in older versions and could be resolved across all versions with one patch is IMHO worth considering.

Comment #17

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna commented 6 April 2009 at 16:24

I'd really like to be able to directly use Dominic Sayers' code but it's licensed under the CPAL, which is not GPL compatible:
* http://ostatic.com/blog/cpal-whats-that

I've emailed him to see if he'd be willing to dual-license it.

Comment #18

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna commented 6 April 2009 at 19:18

One train of thought.. while the ability to use "joe@localhost"-style hostnames might be useful for local testing:

There are other ways of testing, you can give your machine a more correct hostname,
You should not be able to use hostnames without a TLD on a production, publicly accessible site,

and the key problem is that Drupal has no way of differentiating between the two use cases.

How about the following:

By default the email validator is strict, does not allow "joe@localhost"-style addresses,
An option be available to allow "joe@localhost"-style addresses for local testing.

An alternative would be to have a hook, e.g. "hook_valid_email_address()", which could be used by a 3rd party module to do the joe@localhost style, or other weird ones (e.g. to support some crazy Lotus Notes or FDN-style hostnames).

Thoughts?

Comment #19

dominicsayers CreditAttribution: dominicsayers commented 6 April 2009 at 20:38

File	Size
isemail.zip	19.68 KB

I am happy for my is_email() code to be used under the GPL if this allows it to be included in Drupal. My intention in writing the function was for it to be used as widely as possible and I would not want the license to stand in the way of that.

http://www.dominicsayers.com/isemail

Comment #20

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna commented 7 April 2009 at 00:46

@dominicsayers: Thank you! That means a great deal to us! (well, me anyway)

I'll put together a patch based on this code and see what everyone things.

Comment #21

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna commented 14 April 2009 at 18:50

FYI, for a D6 site we're launching in a week we needed to do firstname.last-name@example.com for someone who had a hyphenated name. It failed with the built-in D6 code. The patch will be available within a few days.

Comment #22

JeremyFrench CreditAttribution: JeremyFrench commented 15 April 2009 at 16:31

Assigned:	Unassigned	» JeremyFrench
Status:	Needs work	» Needs review

File	Size
valid_email_address.patch	21.81 KB

I have built a patch using the dominicsayers function and adapting it to drupal coding standards (or trying to), I have also added all of the test cases to simpletest for common.

It is my first non trivial patch so probably needs a good looking at, also I am not sure if I have put the correct notice in for using third party GPL code.

Comment #23

stewsnooze

he / him

CreditAttribution: stewsnooze commented 15 April 2009 at 16:33

Comment #24

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna commented 15 April 2009 at 16:45

@JeremyFrench - thanks for beating me to it :) I'll review later.

Comment #25

Dries CreditAttribution: Dries commented 15 April 2009 at 17:00

I'd prefer to stick with filter_var($mail, FILTER_VALIDATE_EMAIL) and see if they can fix it upstream in PHP-land.

Comment #26

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna commented 15 April 2009 at 17:24

@Dries: thanks for chiming in on this.

Would you be adversed to using the patch provided by JeremyFrench to fix the D6 & earlier problems if it proves stable?

Comment #27

JeremyFrench CreditAttribution: JeremyFrench commented 16 April 2009 at 08:17

I've looked at the tests which fail with the filter_var function.

They mostly seem to be cases with escaped quotes and whitespace, although having some chars at the end of the domain string also seems not to be caught. So personally I think we can live with filter_var, especially as most mail servers won’t be able to work with the edge cases anyway.

If we need a D6 patch of the dominicsayers function I am happy to create it.

Comment #28

taz88ny CreditAttribution: taz88ny commented 16 April 2009 at 14:19

Comment #29

sharda_ram CreditAttribution: sharda_ram commented 16 April 2009 at 17:36

Comment #30

Dries CreditAttribution: Dries commented 17 April 2009 at 06:53

Version:

7.x-dev

» 6.x-dev

I'll leave the D6 decision to Gabor to decide. I'm declined to say "don't bother". Lowering the version number.

Comment #31

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna commented 21 April 2009 at 17:21

File	Size
valid_email_address-n265548-d6.patch	11.68 KB

I've attached a D6 version of this patch, which correctly identifies e.g. "peter.o'toole@example.com" as a valid email address.

Comment #32

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna commented 21 April 2009 at 17:28

FYI for the D6 patch I just uploaded, I started with Jeremy's patch, made it fit D6 and improved some of the formatting/syntax to be more Drupal-compliant.

Also, there are lots of valid use cases that cause the current system to fail, e.g. someone who has an apostrophe in their name (perfectly valid and common in e.g. Ireland). I'd very inclined to side with email servers being more flexible than they're being given credit for.

Comment #33

spydmobile CreditAttribution: spydmobile commented 10 May 2009 at 21:38

Wow, I have had no end of headaches with the apostrophe in email issue, My main project is for our local govt and many regional aboriginal names (and some irish ones too) have an apostrophe in the local part of the email address, The Email managers have allowed this in the email addresses, but Drupal does not allow this, so I have had to make excuses and make all kinds of crazy workarounds totally defeating the time saving adminstration workflow improvements introducing drupal to my government has brought. Is there any intention to have this patch rolled into a drupal release?

thanks for your time
BTW I am totally gunshy of manually patching my live drupal.....
Franco