Validity of unquoted space in mail address

[bor0]

We have:

boro@bor0:~/dev$ php test.php
/Users/boro/dev/test.php:4:
bool(true)
boro@bor0:~/dev$ cat test.php
<?php
require('./vendor/phpmailer/phpmailer/PHPMailerAutoload.php');
var_dump(PHPMailer::validateAddress("test @test.com"));

According to the tests written, it seems like this was done on purpose.

Using OS X Mail, it complains it's not a valid e-mail address.
Also, per the RFC:

Comments and folding white space SHOULD NOT be used around the "@" in the addr-spec.

In my understanding, SHOULD NOT is used because it is allowed in some cases (e.g. when surrounded by quotes), which is not the case for test @test.com.

Additionally, there are inconsistencies when using different patterns:

<?php
require('./vendor/phpmailer/phpmailer/PHPMailerAutoload.php');

$patterns = array(
    'pcre8',
    'pcre',
    'html5',
    'noregex',
    'php',
);

foreach ($patterns as $pattern) {
    echo "$pattern => " . (PHPMailer::validateAddress("test @test.com", $pattern) ? 'ok' : 'nok') . "\n";
}

Returns:

pcre8 => ok
pcre => nok
html5 => nok
noregex => ok
php => nok

So in my opinion, pcre8 should also return nok for that e-mail address (it's okay that noregex returns ok since that one is a really simple one).

[Synchro]

I agree - if you'd like to tweak the regex to not allow this specific case, be my guest, but not like your #805 PR just did!

[bor0]

As far as I investigated, the issue seems to be around this on the page:

// Comments and folding white spaces

'/^((?>(?>(?>((?>(?>(?>\x0D\x0A)?[\t ])+|(?>[\t ]*\x0D\x0A)?[\t ]+)?)(\((?>(?2)(?>[\x01-\x08\x0B\x0C\x0E-\'*-\[\]-\x7F]|\\\[\x00-\x7F]|(?3)))*(?2)\)))+(?2))|(?2))?)$/iD'

However, dealing with the regex is a bit tricky. Do you have any recommendations @Synchro? I tried using https://regex101.com/ but it's still complex :)

[Synchro]

It's a shame that Squiloople has gone, so good thinking on looking for it in wayback! You can see that I asked exactly the question you did in May 2013. you can also see that Rasmus (of PHP) was there too, which is why that pattern ended up in filter_var.

It's very tempting to remove all support for comments and FWS since they are mainly only for presentation purposes, which isn't PHPMailer's business - neither are valid for actual sending in 5321. I suspect that allowing them is the underlying problem in #429 and is also what was the cause of the security issue fixed in 6687a96. I wonder if there is an RFC-recommended method for removing comments and FWS?

Sqiloople did suggest a pattern that dropped FWS and comments that might be a more relevant default pattern to use, so that might be worth a look. At the time it was written the main difference between the sqiloople pattern and PHP's stock one was that it allowed dotless a@b addresses; these have now been outlawed by ICANN for some time, so it might be time to drop support for them too, in which case we're more or less back at PHP's default pattern!

We could just do that in PHPMailer 6, since it's still not final.

[Synchro]

@bor0 What do you think of this? I've simplified it a lot, removed quite a bit of old stuff. It defaults to straight filter_var, but retains the pcre8 pattern for anyone that wants more complex options. The API remains the same, so no BC breaks, but different validations may apply than in old versions. I broke out some of the patterns in the tests to make it more obvious what will work and not work.

[bor0]

@Synchro, looks good! But the Kept for BC comment doesn't make sense, since it's a breaking change right? So backwards compatibility using a broken change kind of contradicts.

If we go ahead with that change imo we should remove those comments and also document what would break as you said.

[Synchro]

Those comments are just signposts for those updating legacy code - it's common for some to use pcre as the value for $validator, but if that is simply dropped, it will fall through to the new default of php, rather than going to the more obvious alternative of pcre8 as I've made it do (since even pcre was more permissive than php). I think it's better to handle old values explicitly rather than just letting it fall through without saying anything.

That said, anyone looking at the code might wonder why those options even appear in the switch if they were not aware it used to be an option in old versions, hence the comments. We could just call it something else "this used to be an option" or something.