Task #39714

Validate emailaddresses by filter_var()

Added by Markus Günther about 9 years ago. Updated about 9 years ago.

Status:
Resolved
Priority:
Should have
Category:
Validation
Start date:
2012-08-12
Due date:
% Done:

100%

Estimated time:
Sprint:
PHP Version:
Has patch:
No
Complexity:

Description

We have had an issue in the extbase issue tracker (#6970) for simplify the email validator in extbase.

Actually the exbase and FLOW3 email validator is codewise the same. Because we wanna stay in sync with FLOW3, i suggest to use filter_var also in FLOW3 for validating the email addresses.

In extbase we could use an TYPO3 core function for that, so we have to port this codewise to FLOW3 or we renounce the idna converter and only use the filter_var function and the php workaround for version lower than 5.3.4.


Related issues

Is duplicate of TYPO3.Flow - Bug #27375: simplify email validatorRejectedStefan Neufeind2011-06-11

Actions
#1

Updated by Bastian Waidelich about 9 years ago

  • Tracker changed from Suggestion to Task
  • Project changed from TYPO3 Flow Base Distribution to TYPO3.Flow
#2

Updated by Bastian Waidelich about 9 years ago

  • Has patch set to No

In my opinion, the validator should allow an email address if in doubt (e.g. containig "+" or "@localhost") so I agree to Karstens comment on https://review.typo3.org/#/c/2665/
But if you think this is required, we could add a validator option to use filter_var() instead.

#3

Updated by Gerrit Code Review about 9 years ago

  • Status changed from New to Under Review

Patch set 1 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/13608

#4

Updated by Markus Günther about 9 years ago

Bastian Waidelich wrote:

In my opinion, the validator should allow an email address if in doubt (e.g. containig "+" or "@localhost") so I agree to Karstens comment on https://review.typo3.org/#/c/2665/
But if you think this is required, we could add a validator option to use filter_var() instead.

These option is really a good idea.. so we can have both :)

#5

Updated by Alexander Schnitzler about 9 years ago

So, if Karsten is right, I don't know why this should be discussed again. You cannot provide a validator for every case out there. So why not decide the behaviour and point that out clearly? If anyone has a different usecase he has to write his own validator then.

I just agree in using filter_var as long as it really supports the RFC. If it has bugs, it's not better then a preg_match. But IMHO introducing options for changing/choosing a behaviour is just sick.

#6

Updated by Helmut Hummel about 9 years ago

Hi Bastian,

Bastian Waidelich wrote:

In my opinion, the validator should allow an email address if in doubt (e.g. containig "+" or "@localhost") so I agree to Karstens comment on https://review.typo3.org/#/c/2665/

Could you explain why you and Karsten think so?

For testing email sending and receiving this might be fine to use such an email address, but for an application used in production, I would expect that an address like "example@localhost" fails to validate.
I'm not alone with that opinion. Symfony explicitly expects "user@localhost" to be invalid: https://github.com/symfony/Validator/blob/master/Tests/Constraints/EmailValidatorTest.php

They even fixed their validator to have this behaviour, for PHP versions below 5.3.3 where such an email passed the filter_var() test. https://github.com/symfony/symfony/commit/d235570653fe5c79961e0ab61a3317a954e0463b

According to RFC 5321 only email addresses with a resolvable FQDNs are considered to be valid. "foo@localhost" is never resolvable thus always invalid.

However it seems that "fred@com" is valid, as com is a TLD and resolvable. (see last comment here: https://bugs.php.net/bug.php?id=49576#1282080887)

My suggestion for released FLOW3 versions:
  • Add a string length check (like in t3lib_div::validEmail) as a security measurement for PHP < 5.3.4
  • Use filter_var() as the validation is correct as of PHP 5.3.3
  • Add an additional check for PHP 5.3.3 like Symfony did
  • Change the tests accordingly
My suggestion for master branch:
  • All of the above
  • Transform
  • Add an option to additionally do an MX check (see Symfony EmailValidator)

This would still fail to validate mail addresses like "fred@com", I have no solution for that but I still find that being too restrictive in an validator is better than too loose at least for the default case.

Alexander Schnitzler wrote:

I just agree in using filter_var as long as it really supports the RFC. If it has bugs, it's not better then a preg_match.

The only known bug is the one with not validating "foo@com".

But IMHO introducing options for changing/choosing a behaviour is just sick.

Having an option to somehow validate this to true (in combination with an MX check) would make sense to me.

#7

Updated by Helmut Hummel about 9 years ago

After all, what seemed to be easy at first sight (just use filter_var), is not that easy if you have a closer look.

In the end, neither t3lib_div::validEmail is absolutely correct, nor the EmailValidator in FLOW3 or Extbase.
But if you follow my arguments above: 3lib_div::validEmail is more accurate, more consistent and more secure than the
current FLOW3 Validator and this is why I will approve the change in Extbase #6970 to use this method in favor of the regex.

I leave it to the FLOW3 team to decide whether they agree to this or if the code base diverges in this case.

#8

Updated by Alexander Schnitzler about 9 years ago

[...] but I still find that being too restrictive in an validator is better than too loose at least for the default case.

You're absolutely right with it!

Alexander Schnitzler wrote:

I just agree in using filter_var as long as it really supports the RFC. If it has bugs, it's not better then a preg_match.

The only known bug is the one with not validating "foo@com".

Alright, didn't know this is the only bug but then we could discuss to fix this (next quote) or just clearly point out the new behaviour.

But IMHO introducing options for changing/choosing a behaviour is just sick.

Having an option to somehow validate this to true (in combination with an MX check) would make sense to me.

Full ack here too if it's the way you described it. I just wouldn't add an option to choose between pure filter_var and the current preg_match.

#9

Updated by Gerrit Code Review about 9 years ago

Patch set 2 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/13608

#10

Updated by Karsten Dambekalns about 9 years ago

  • Category set to Validation
  • Target version set to 2.0 beta 1
#11

Updated by Adrian Föder about 9 years ago

at https://github.com/php/php-src/blob/master/ext/filter/logical_filters.c#L499 there are some interesting comments about the intention of failing particular occurrences (regarding that @com for example). Maybe that gives some insight.

#12

Updated by Bastian Waidelich about 9 years ago

Helmut Hummel wrote:

Hi Helmut,

Bastian Waidelich wrote:

In my opinion, the validator should allow an email address if in doubt (e.g. containig "+" or "@localhost") so I agree to Karstens comment on https://review.typo3.org/#/c/2665/

Could you explain why you and Karsten think so?

I can't speak for Karsten of course, but I'm in favour of less strict validators usually because they're mostly not about security but about preventing the user from mistyping - if a hacker can't use foo@localhost he will use *g
I currently work on a registration where username = email address. I use local email addresses to test this, otherwise I had to create real emails or disable the validator while testing.
Apart from that it would be quite annoying if someone could not register with my service because his email address is considered invalid.

But apparently "+" is allowed by filter_var() in fact and to work around the other issue I can use a "proper" local domain. So, I'm happy with the change :)

#13

Updated by Bastian Waidelich about 9 years ago

Bastian Waidelich wrote:

if a hacker can't use foo@localhost he will use

Ha, it seems redmine doesn't support @localhost emails either :-D

#14

Updated by Gerrit Code Review about 9 years ago

Patch set 3 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/13608

#15

Updated by Markus Günther about 9 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100

Also available in: Atom PDF