JMailHelper:isEmailAddress checked the IPv4 and IPv6 address in the domain #1833

Merged
merged 6 commits into from Apr 5, 2013

Projects

None yet

7 participants

@fitorec
Contributor
fitorec commented Feb 16, 2013

JMailHelper:isEmailAddress checked the IPv4 and IPv6 address in the domain

Please check:

joomla/joomla-cms#724

@fitorec fitorec Update libraries/joomla/mail/helper.php
JMailHelper:isEmailAddress checked the IPv4 and IPv6 address in the domain

Please check:

joomla/joomla-cms#724
9100268
@fitorec
Contributor
fitorec commented Feb 16, 2013

This is a verbatim copy of:

joomla/joomla-cms#724


I realized that IPs are not entirely correct validating, for example:

<?php
$domain = '192.23.243.23.2';
$regex = '/^[0-9\.]+$/';
preg_match($regex, $domain);
//TRUE

One option is to change the regular expression to catch four numbers separated by a dot:

<?php
$domain = '192.23.243.23.2';
$regex = '/^([0-9]{1,3}\.){3}[1-9]+$/';
preg_match($regex, $domain);
//FALSE

Another problem is that soon there will be more IPv6 addresses so we would have to change the regular expression to something like:

<?php
$regex = '/^(([0-9]{1,3}\.){3}[1-9]+)|([0-9a-f]*::[0-9]+)$/';

Fortunately PHP5 has a function (inet_pton) that returns false if the IP address is wrong, I think this is a better solution.

What do you think?

@fitorec fitorec closed this Feb 16, 2013
@fitorec fitorec reopened this Feb 16, 2013
@piotr-cz
Contributor

Do you think filter_var with FILTER_VALIDATE_EMAIL or FILTER_VALIDATE_IP would help?

filter_var($domain, FILTER_VALIDATE_EMAIL);
@AmyStephen
Contributor

+1 @piotr-cz on using filter_var.

There are flags for both IPv4 and 6, too http://www.php.net/manual/en/filter.filters.validate.php

@fitorec fitorec Updated the JMailHelper with the filter_var function
Please read the following URL for more information:
>  joomla#1833
931aaea
@fitorec
Contributor
fitorec commented Feb 18, 2013

👍 @piotr-cz, @AmyStephen yep, tnks for the tip ☺️.

Would it be appropriate to change the function to something like? ( Using FILTER_VALIDATE_EMAIL flag ):

<?php
abstract class JMailHelper
{
    //...
    function isEmailAddress($email)
    {
        return (boolean) filter_var($email, FILTER_VALIDATE_EMAIL);
    }
}

😳 What do you think?

@AmyStephen
Contributor

Clever, nice and simple. =) Works good, had to try it out.

@piotr-cz
Contributor

@fitorec
I think this is perfect example of moving Platform methods to use native PHP ones.

Initial method was pretty complex, I think best thing to prove it may be simplified to such extend is to make sure this passes JMailHelper Unit tests.

You might add some more cases for test (with IPv4 and IPv6 as domain). Results are in Pull Tests Results.
I won't help you with running unit test locally, still have to learn how to :)

@fitorec
Contributor
fitorec commented Feb 18, 2013

:octocat: I just made a corresponding change

@AmyStephen
Contributor

That is a spectacular patch.

@mbabker
Member
mbabker commented Feb 19, 2013

Looks like we've got a few failing unit tests now. Looks as though what we were testing doesn't match what is returned by filter_var(). Would you mind fixing the tests as well?

Here's the results from our pull tester - http://developer.joomla.org/pulls/pulls/1833.html

@dongilbert
Contributor

@piotr-cz this is a perfect example of being a PHP dev and not strictly a "Joomla Dev"

@fitorec
Contributor
fitorec commented Feb 19, 2013

@dongilbert, @piotr-cz, Thanks, now I see, I will return to the previous commit and add more cases for test for IPv4 and IPv6

@piotr-cz
Contributor

@fitorec
I'd be interested in knowing which test cases failed.

PHPMailer 2.2.1 and 2.2.3 and thus JMail is using filter_var($address, FILTER_VALIDATE_EMAIL) already when being instantiated by JFactory

@AmyStephen
Contributor

I saved them -- was going to look at them when I get a few minutes this week: The first one you can see is wrong. The next three I needed to look up - but they could very well be correct, too. (For example, the escaped failure -- was wondering if maybe PHP tests unescaped, not sure.).

Email addresses are not easy and I trust PHP to do this better than our collective best guess. Frankly, if we find PHP's edits to be wrong, we should bring a patch to them. I hope we go back to @fitorec's PHP filter patch.

The previous errors:

JMailHelperTest::testIsEmailAddress with data set #1 ('joe@home', true)
Failed asserting that false matches expected true.

.../tests/suites/unit/joomla/mail/JMailHelperTest.php:298


JMailHelperTest::testIsEmailAddress with data set #14 ('~joe@bob.com', false)
Failed asserting that true matches expected false.

.../tests/suites/unit/joomla/mail/JMailHelperTest.php:298

JMailHelperTest::testIsEmailAddress with data set #15 ('joe$@bob.com', false)
Failed asserting that true matches expected false.

.../tests/suites/unit/joomla/mail/JMailHelperTest.php:298

JMailHelperTest::testIsEmailAddress with data set #17 ('o\'reilly@there.com', false)
Failed asserting that true matches expected false.

.../tests/suites/unit/joomla/mail/JMailHelperTest.php:298
@piotr-cz
Contributor

@AmyStephen
One comment at php manual mentions, that FILTER_VALIDATE_EMAIL validates emails according to standards, but rules for creating addresses are up to organizations.

Maybe solution would be to give a strict option to method: JMailHelper::isEmailAddress($strict = false).
When enabled, we'd use filter_var. When not, use current method with patch @fitorec prepared.

@AmyStephen
Contributor

An organization, to me, would be like a business that needs to ensure their domain is used for the email address, or something like that. I can't imagine why Joomla and PHP would not follow the same standards.

Regardless, the real question is - are those four errors really errors? The first one is clearly not an email address, and the PHP edit threw it out. I think PHP is right, don't you?

I'm not sure about the other three. But, if those are valid email addresses, who are we to argue? But, if they are incorrect, it would be better to submit as issues to PHP, add an edit following the PHP edit, and then link to the issue report so we can remove the edit when it makes it into the minimum PHP version supported.

@elinw
Contributor
elinw commented Feb 20, 2013

We have very detailed email validation in jform that match html5 standards and the RFC from IETF. If you have something after @ (it is not necessary to to so) there must be at least two characters.. The first @home email address most certainly does meet standards., The others do as well as far as I know, but you can check the RFC and also the W3C standards documents which were the basis for the validation used. I agree we need to add compliance with ipv6

@AmyStephen
Contributor

To the best of my knowledge, the (W3C datatype for email)[http://www.w3.org/TR/html-markup/datatypes.html#form.data.emailaddress] has not changed:

e-mail address
Any string that matches the following ABNF production:

1*( atext / "." ) "@" ldh-str 1*( "." ldh-str )

...where atext is as defined in RFC 5322, and ldh-str is as defined in RFC 1034.

What standard are you referring to? Wondering if perhaps you are talking about the WeC HTML 5.1 email state? For email user agents, it is acceptable to enter alias values that then look up the email address. If so, that's a different data element and would require a new field and edit.

But even in that standard, it states the value itself must conform too - and it links to the standard above.

It would help to have links to the standards you are referring to for review.

Thanks.

@AmyStephen
Contributor

@elinw

Tonight, I looked at a number of other PHP filters, across the board, I'm seeing FILTER_VALIDATE_EMAIL used.

I do believe it is appropriate to use. But, if there is reason to believe otherwise, my suggestion is to create an issue report for PHP so that the problem gets fixed. Then, if needed, we can add an edit following the PHP edit, linking to the issue report with PHP. That will allow us to rely on PHP core, as much as possible, document the problem so everyone understands why we are deviating from PHP, and then remove it, once our minimum PHP level is met for the fix. That's what we ask of our users and we should probably do the same for those projects whose software we use.

Do you agree with that approach?

Drupal 8

/**
 * Verifies the syntax of the given e-mail address.
 *
 * See @link http://tools.ietf.org/html/rfc5321 RFC 5321 @endlink for details.
 *
 * @param $mail
 *   A string containing an e-mail address.
 *
 * @return
 *   TRUE if the address is in a valid format.
 */
function valid_email_address($mail) {
  return (bool)filter_var($mail, FILTER_VALIDATE_EMAIL);
}

Wordpress
They have a preg filter for those sites with less than PHP 5.2

  /**
   * Check that a string looks roughly like an email address should
   * Static so it can be used without instantiation
   * Tries to use PHP built-in validator in the filter extension (from PHP 5.2), falls back to a reasonably competent regex validator
   * Conforms approximately to RFC2822
   * @link http://www.hexillion.com/samples/#Regex Original pattern found here
   * @param string $address The email address to check
   * @return boolean
   * @static
   * @access public
   */
  public static function ValidateAddress($address) {
    if (function_exists('filter_var')) { //Introduced in PHP 5.2
      if(filter_var($address, FILTER_VALIDATE_EMAIL) === FALSE) {
        return false;
      } else {
        return true;
      }
    } else {
      return preg_match('/^(?:[\w\!\#\$\%\&\'\*\+\-\/\=\?\^\`\{\|\}\~]+\.)*[\w\!\#\$\%\&\'\*\+\-\/\=\?\^\`\{\|\}\~]+@(?:(?:(?:[a-zA-Z0-9_](?:[a-zA-Z0-9_\-](?!\.)){0,61}[a-zA-Z0-9_-]?\.)+[a-zA-Z0-9_](?:[a-zA-Z0-9_\-](?!$)){0,61}[a-zA-Z0-9_]?)|(?:\[(?:(?:[01]?\d{1,2}|2[0-4]\d|25[0-5])\.){3}(?:[01]?\d{1,2}|2[0-4]\d|25[0-5])\]))$/', $address);
    }
  }

A few others, the first is Anthony, the second is Respect Validation, a very popular library, the third is from a respected PHP dev

https://github.com/ircmaxell/filterus/blob/master/lib/Filterus/Filters/Email.php

https://github.com/Respect/Validation/blob/develop/library/Respect/Validation/Rules/Email.php

https://github.com/vlucas/valitron/blob/master/src/Valitron/Validator.php#L256

@piotr-cz
Contributor

in case rules are more benevolent than current FILTER_VALIDATE_EMAIL, than situation could lead to false positives when email validates, but doesn't phpmailer (which uses it) returns exception.

I'm not an expert in emails but on the other side Joomla might be installed in intranet and admins are not restricted by strict standards.

@AmyStephen
Contributor

I did a search on "don't use FILTER_VALIDATE_EMAIL " and came across discussions on problem validations.

The name@localhost is one of those. I'm guessing (?) the example one is the same type of thing?

In a localhost situation, an IP address resolves for localhost, so the email server can handle it. I have not found a standard that identifies it as "valid" email address but it will function properly on a computer since it resolves to an IP address.

According to RFC2821 and RFC2822, 7 bit ASCII characters are allowed in Internet mail addresses. That explains why the following addresses validate as TRUE with PHP's edit. I believe Joomla's edits are incorrect

'~joe@bob.com'
'joe$@bob.com
'o\'reilly@there.com'

We need to see what @elinw has in terms of a standard on the @example email addresses. But otherwise, I believe this is better than what Joomla has now.

@piotr-cz How would we possibly provide a test for situations where the standards don't apply? What rules would we use? In those cases, people can override the function. I think that is all that can be done. But, maybe I didn't understand?

@AmyStephen
Contributor

@fitorec @piotr-cz @dongilbert @mbabker @elinw

Differences of opinion can sometimes result in wasted time and I want to avoid that with this path.

Our code of conduct states "The important goal is not to avoid disagreements or differing views but to resolve them constructively." I'd really like to see us pull together on this patch and get an agreement, based on standards, for this patch.

Towards that end, I provided links and excerpts of standards, I reviewed the source from five other applications, providing code examples or links to code examples that supported my position.

I recommend using the PHP edit patch.

Further, I recommend a practice of reporting problems found to PHP and linking to that issue report in the code. That way, we can anticipate removing the extra filter code when the the earliest version we support is current.

Seems like this has maybe stalled. I don't know what to do. The code of conduct says "When you are unsure, ask for help." So, I am asking for help. What is needed to get this in core? What do you want of me? Etc.

@dongilbert
Contributor

I think we should do the obvious thing and use filter_var($email, FILTER_VALIDATE_EMAIL); It makes the most sense, is the most widely accepted, and let's be honest - Joomla validation isn't the smartest thing around. :)

This is the approach we will be taking in Joomla Framework. It's time to stop being a "Joomla Dev" and start being a "PHP Dev".

@dongilbert
Contributor

I would say that if a sysadmin on an Intranet is needing to verify emails to a different standard than what is possible via filter_var, then it is their responsibility to write an override or custom validator. The platform needs to provide the base standard, not edge cases. But it's not a hill I'm willing to die on.

@mbabker
Member
mbabker commented Feb 22, 2013

+1 to the standard

On Friday, February 22, 2013, Don Gilbert wrote:

I would say that if a sysadmin on an Intranet is needing to verify emails
to a different standard than what is possible via filter_var, then it is
their responsibility to write an override or custom validator. The platform
needs to provide the base standard, not edge cases. But it's not a hill I'm
willing to die on.


Reply to this email directly or view it on GitHubhttps://github.com/joomla/joomla-platform/pull/1833#issuecomment-13978010.

@piotr-cz
Contributor

Agree with @dongilbert.
+1 to the standard.

@eddieajau
Contributor

@fitorec there appears to be a unit test failure.

http://developer.joomla.org/pulls/pulls/1833.html

Could you look into that please. Thanks.

@dongilbert
Contributor

IMO, this is entirely dependent on what the CMS wants to do with it. FWIW, the Framework is going to use filter_var($email, FILTER_VALIDATE_EMAIL)

@eddieajau eddieajau merged commit 2556578 into joomla:staging Apr 5, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment