[Validator] check for empty host when calling checkdnsrr() #22109

apetitpa · Mar 22, 2017

Q	A
Branch?	master
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#22106
License	MIT
Doc PR	n/a

This should fix the email validator issue

fabpot · Mar 22, 2017

The regex should not be changed. But the condition should indeed be added in other methods using checkdnsrr.

apetitpa · Mar 23, 2017

thank you @fabpot and @jordscream I'll make some modifications and add unit tests.

xabbuh · Mar 23, 2017

src/Symfony/Component/Validator/Constraints/EmailValidator.php

@@ -118,6 +118,10 @@ public function validate($value, Constraint $constraint)
     */
    private function checkMX($host)
    {
+        if (null === $host || '' === $host) {


$host can never be null, can it?

I think you're right, I'll fix this. Thank you

xabbuh

Can you please also add a test to prevent future regressions?

jsamouh

What about unit test for the main issue ? ^^

jsamouh · Mar 23, 2017

src/Symfony/Component/Validator/Constraints/EmailValidator.php

+        if ('' === $host) {
+            return false;
+        }
+


preferred empty host condition in return statement

jsamouh · Mar 23, 2017

src/Symfony/Component/Validator/Constraints/EmailValidator.php

+        if ('' === $host) {
+            return false;
+        }
+
        return $this->checkMX($host) || (checkdnsrr($host, 'A') || checkdnsrr($host, 'AAAA'));


preferred empty host condition in return statement

apetitpa · Mar 23, 2017

About the unit test for the main issue, since checkMX() and checkHost() are both private how can I test it ? There is already a test case that gives an empty host array('example@'),

jsamouh · Mar 23, 2017

@apetitpa just test a use case as "test@test@" or "test@" in the EmailValidator. It should return the appropriate violation

apetitpa · Mar 23, 2017

Thank you @jordscream I forgot that the original issue was with emails containing @ twice

Nek- · Mar 23, 2017

src/Symfony/Component/Validator/Constraints/EmailValidator.php

@@ -118,7 +118,7 @@ public function validate($value, Constraint $constraint)
     */
    private function checkMX($host)
    {
-        return checkdnsrr($host, 'MX');
+        return ('' !== $host) && checkdnsrr($host, 'MX');


why not !empty() check?

Well there is a similar check line 45 if (null === $value || '' === $value) that does not use the native empty function so I assumed I should do the same but maybe I was wrong ?

We try to avoid using empty in Symfony.

@fabpot interesting, why? (is the reason documented somewhere?)

empty is just too broad. For instance, it includes the 0 string as being false. Sometimes, that not desirable. So, we prefer to be very explicit about our expectations.

fabpot · Mar 23, 2017

src/Symfony/Component/Validator/Constraints/EmailValidator.php

@@ -118,7 +118,7 @@ public function validate($value, Constraint $constraint)
     */
    private function checkMX($host)
    {
-        return checkdnsrr($host, 'MX');
+        return ('' !== $host) && checkdnsrr($host, 'MX');


parentheses can be removed

I'll fix this in a minute thank you.

jsamouh · Mar 23, 2017

@apetitpa , it seems your unit test fails cause you do not check the appropriate violation

apetitpa · Mar 23, 2017

@jordscream I think you're right I should check for MX_CHECK_FAILED_ERROR or HOST_CHECK_FAILED_ERROR

ro0NL · Mar 23, 2017

src/Symfony/Component/Validator/Constraints/UrlValidator.php

@@ -72,7 +72,7 @@ public function validate($value, Constraint $constraint)
        if ($constraint->checkDNS) {
            $host = parse_url($value, PHP_URL_HOST);

-            if (!checkdnsrr($host, 'ANY')) {
+            if ('' === $host || !checkdnsrr($host, 'ANY')) {


Perhaps switch to !is_string($host) due possible false or null return values.

I think this is a good idea. I'll do it thank you.

jsamouh · Mar 24, 2017

@apetitpa . When you think to do it ? ^^. Need this fix for a customer :-P

apetitpa · Mar 24, 2017

@jordscream I hope this week-end but to be honest I'm facing difficulties creating an effective and working test for this issue. Some help would be appreciated.

ro0NL · Mar 24, 2017

@apetitpa have a look at https://symfony.com/doc/current/components/phpunit_bridge.html#dns-sensitive-tests

I would add a new ~~, dns-senstive,~~ test which covers exactly this case, so you can assert the expected violation code; or tweak a current test so you can provide the expected code.

edit: Dont think it needs to be dns-senstive though, as there will be no network traffic for foo@bar@.

xabbuh · Mar 24, 2017

Dont think it needs to be dns-senstive though, as there will be no network traffic for foo@bar@.

Agreed, the issue to fix is that the DNS functions should in fact not be called anymore.

ro0NL · Mar 24, 2017

src/Symfony/Component/Validator/Constraints/EmailValidator.php

@@ -118,7 +118,7 @@ public function validate($value, Constraint $constraint)
     */
    private function checkMX($host)
    {
-        return checkdnsrr($host, 'MX');
+        return !empty($host) && checkdnsrr($host, 'MX');


Please avoid empty :) We expect a string, and only a empty string ($host === '') should not be passed to checkdnsrr.

I know but substr($value, strrpos($value, '@') + 1) with $value = 'foo@bar.fr@' returns "" with PHP 7.* and false with PHP 5.6. So only checking for string emptiness is not enough.

Let's cast the substring result above. $host = (string) ...;

It's done, thank you

ro0NL · Mar 24, 2017

src/Symfony/Component/Validator/Tests/Constraints/EmailValidatorTest.php

@@ -229,4 +229,32 @@ public function testHostnameIsProperlyParsed()

        $this->assertNoViolation();
    }
+
+    public function getCheckTypes()


Should be provideCheckTypes i guess, but it's a tiny semantic ;)

I'll make the modification thank you

apetitpa · Mar 27, 2017

@xabbuh is it ok now ?

fabpot · Mar 27, 2017

src/Symfony/Component/Validator/Tests/Constraints/EmailValidatorTest.php

@@ -229,4 +229,32 @@ public function testHostnameIsProperlyParsed()

        $this->assertNoViolation();
    }
+
+    public function provideCheckTypes()


Can you move the provider after the test (that's the convention we are using in Symfony).

Of course, will do in a minute.

| Q | A | | --- | --- | | Branch? | master | | Bug fix? | yes | | New feature? | no | | BC breaks? | no | | Deprecations? | no | | Tests pass? | yes | | Fixed tickets | #22106 | | License | MIT | | Doc PR | n/a |

Add empty on host in other methods where checkdnsrr is called and add unit tests to prevent future regressions

Remove malformed EmailValidatorTest + Update UrlValidator test

Switch to `empty` because depending on the PHP version, substr returns '' or false

xabbuh

👍

fabpot · Apr 5, 2017

Thank you @apetitpa.

… (apetitpa) This PR was submitted for the master branch but it was merged into the 2.7 branch instead (closes #22109). Discussion ---------- [Validator] check for empty host when calling checkdnsrr() | Q | A | | --- | --- | | Branch? | master| | Bug fix? | yes | | New feature? | no | | BC breaks? | no | | Deprecations? | no | | Tests pass? | yes | | Fixed tickets | #22106 | | License | MIT | | Doc PR | n/a | This should fix the email validator issue Commits ------- 7104c4e move provider after test c07fb41 update dataProvider function name 1825d0f cast substr result to string and remove empty function use 894127b rename dataset provider d973eb1 Add a test to prevent future regressions 6f24b05 Switch to `empty` native function to check emptiness f390f29 remove non relevant test case 91b665c Switch to `is_string` native method 6071ff6 Remove unnecessary parentheses 9753a27 Add a test case to prevent future regressions 4fcb24b Move empty condition in return statement a090ef8 Use LF line separator 60392fd fix coding standard to comply with fabbot 197d19f Remove malformed EmailValidatorTest + Update UrlValidator test 6b0702e Add empty check on host in other methods + add unit tests 6dd023f [Validator] Allow checkMX() to return false when $host is empty

carsonbot added Status: Needs Review Validator Bug labels Mar 22, 2017

xabbuh reviewed Mar 23, 2017

View reviewed changes

xabbuh requested changes Mar 23, 2017

View reviewed changes

jsamouh reviewed Mar 23, 2017

View reviewed changes

apetitpa changed the title ~~[Validator] Allow checkMX() to return false when $host is empty~~ [Validator] check for empty host when calling checkdnsrr() Mar 23, 2017

Nek- reviewed Mar 23, 2017

View reviewed changes

fabpot reviewed Mar 23, 2017

View reviewed changes

ro0NL reviewed Mar 23, 2017

View reviewed changes

ro0NL reviewed Mar 24, 2017

View reviewed changes

fabpot reviewed Mar 27, 2017

View reviewed changes

apetitpa added 6 commits March 27, 2017 16:54

[Validator] Allow checkMX() to return false when $host is empty

74629a4

| Q | A | | --- | --- | | Branch? | master | | Bug fix? | yes | | New feature? | no | | BC breaks? | no | | Deprecations? | no | | Tests pass? | yes | | Fixed tickets | #22106 | | License | MIT | | Doc PR | n/a |

Add empty check on host in other methods + add unit tests

b7f6db6

Add empty on host in other methods where checkdnsrr is called and add unit tests to prevent future regressions

Remove malformed EmailValidatorTest + Update UrlValidator test

af3b0d9

Remove malformed EmailValidatorTest + Update UrlValidator test

fix coding standard to comply with fabbot

fd9843a

Use LF line separator

61e2439

Move empty condition in return statement

1683fee

apetitpa and others added 10 commits March 27, 2017 16:54

Add a test case to prevent future regressions

00f8e7b

Remove unnecessary parentheses

6a919dd

Switch to is_string native method

628c2d8

remove non relevant test case

794f63d

Switch to empty native function to check emptiness

756f9ce

Switch to `empty` because depending on the PHP version, substr returns '' or false

Add a test to prevent future regressions

b6f471a

rename dataset provider

7defce7

cast substr result to string and remove empty function use

c9b4613

update dataProvider function name

67e1dcc

move provider after test

bb05115

nicolas-grekas added this to the 3.3 milestone Mar 28, 2017

xabbuh approved these changes Mar 29, 2017

View reviewed changes

fabpot closed this Apr 5, 2017

apetitpa deleted the fix-email-validator branch April 14, 2017 18:50

This was referenced May 1, 2017

Release v2.7.27 #22598

Merged

Release v2.8.20 #22599

Merged

Release v3.2.8 #22601

Merged

Search code, repositories, users, issues, pull requests...

Uh oh!

[Validator] check for empty host when calling checkdnsrr() #22109

[Validator] check for empty host when calling checkdnsrr() #22109

Uh oh!

Conversation

apetitpa commented Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabpot commented Mar 22, 2017

Uh oh!

apetitpa commented Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh left a comment

Choose a reason for hiding this comment

Uh oh!

jsamouh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apetitpa commented Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsamouh commented Mar 23, 2017

Uh oh!

apetitpa commented Mar 23, 2017

Uh oh!

Nek- Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apetitpa Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsamouh commented Mar 23, 2017

Uh oh!

apetitpa commented Mar 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsamouh commented Mar 24, 2017

Uh oh!

apetitpa commented Mar 24, 2017

Uh oh!

ro0NL commented Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xabbuh commented Mar 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apetitpa Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ro0NL Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apetitpa commented Mar 22, 2017 •

edited

Loading

apetitpa commented Mar 23, 2017 •

edited

Loading

apetitpa commented Mar 23, 2017 •

edited

Loading

Nek- Mar 23, 2017 •

edited

Loading

apetitpa Mar 23, 2017 •

edited

Loading

ro0NL commented Mar 24, 2017 •

edited

Loading

apetitpa Mar 24, 2017 •

edited

Loading

ro0NL Mar 24, 2017 •

edited

Loading