Skip to content

Fix connection timeout issue when try combine MX + A records for specfic DNS when A record not exists #410

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: 4.x
Choose a base branch
from
Open

Fix connection timeout issue when try combine MX + A records for specfic DNS when A record not exists #410

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a7bb3d
Fix connection timeout issue when try combine MX + A records for spec…
alexander-schranz Mar 6, 2025
39fb12b
Move validate MxRecords in own method
alexander-schranz Mar 6, 2025
1b0892a
Add some phpdoc
alexander-schranz Jun 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 26 additions & 6 deletions src/Validation/DNSCheckValidation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
/**
* Reserved Top Level DNS Names (https://tools.ietf.org/html/rfc2606#section-2),
* mDNS and private DNS Namespaces (https://tools.ietf.org/html/rfc6762#appendix-G)
*
*
* @var string[]
*/
public const RESERVED_DNS_TOP_LEVEL_NAMES = [
Expand Down Expand Up @@ -145,14 +145,20 @@
*/
private function validateDnsRecords($host): bool
{
$dnsRecordsResult = $this->dnsGetRecord->getRecords($host, DNS_A + DNS_MX);
$dnsRecords = [];

if ($dnsRecordsResult->withError()) {
$this->error = new InvalidEmail(new UnableToGetDNSRecord(), '');
return false;
$mxRecordsResult = $this->dnsGetRecord->getRecords($host, DNS_MX);

if (! $mxRecordsResult->withError()) {
$dnsRecords = $mxRecordsResult->getRecords();
}

$dnsRecords = $dnsRecordsResult->getRecords();
// Combined check for A+MX can fail with connection timed out, even in the presence of valid MX record
$aRecordsResult = $this->dnsGetRecord->getRecords($host, DNS_A);

if (! $aRecordsResult->withError()) {
$dnsRecords = array_merge($dnsRecords, $aRecordsResult->getRecords());
}

// Combined check for A+MX+AAAA can fail with SERVFAIL, even in the presence of valid A/MX records
$aaaaRecordsResult = $this->dnsGetRecord->getRecords($host, DNS_AAAA);
Expand All @@ -163,13 +169,26 @@

// No MX, A or AAAA DNS records
if ($dnsRecords === []) {
if ($mxRecordsResult->withError()
&& $aRecordsResult->withError()
&& $aaaaRecordsResult->withError()
) {
$this->error = new InvalidEmail(new UnableToGetDNSRecord(), '');
return false;
}
Comment on lines +172 to +178
Copy link
Author

@alexander-schranz alexander-schranz Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unsure when we should return UnableToGetDNSRecord if one DNS lookup failed or when all failed?


$this->error = new InvalidEmail(new ReasonNoDNSRecord(), '');
return false;
}

return $this->validateMxRecords($dnsRecords);
}

private function validateMxRecords($dnsRecords): bool

Check failure on line 187 in src/Validation/DNSCheckValidation.php

View workflow job for this annotation

GitHub Actions / Psalm 

src/Validation/DNSCheckValidation.php:187:40: MissingParamType: Parameter $dnsRecords has no provided type (see https://psalm.dev/154)
{
// For each DNS record
foreach ($dnsRecords as $dnsRecord) {

Check failure on line 190 in src/Validation/DNSCheckValidation.php

View workflow job for this annotation

GitHub Actions / Psalm 

src/Validation/DNSCheckValidation.php:190:33: MixedAssignment: Unable to determine the type that $dnsRecord is being assigned to (see https://psalm.dev/032)
if (!$this->validateMXRecord($dnsRecord)) {

Check failure on line 191 in src/Validation/DNSCheckValidation.php

View workflow job for this annotation

GitHub Actions / Psalm 

src/Validation/DNSCheckValidation.php:191:42: MixedArgument: Argument 1 of Egulias\EmailValidator\Validation\DNSCheckValidation::validateMxRecord cannot be mixed, expecting array<array-key, mixed> (see https://psalm.dev/030)
// No MX records (fallback to A or AAAA records)
if (empty($this->mxRecords)) {
$this->warnings[NoDNSMXRecord::CODE] = new NoDNSMXRecord();
Expand All @@ -177,6 +196,7 @@
return false;
Copy link
Author

@alexander-schranz alexander-schranz Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this expected as soon as one record doesn't pass validateMXRecord it marks the domain as failed? Didn't found any issue with it but thought about if we should do instead of 3 lookup check after every lookup if we already found a valid dns record and early return true. But that would mean this line would not work as expected that it early return false.

Copy link
Author

@alexander-schranz alexander-schranz Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personally I would expect something like this:

        // For each DNS record
        foreach ($dnsRecords as $dnsRecord) {
            if ($this->validateMXRecord($dnsRecord)) {
                return true; // do not check all domains
            }
            
            //if (empty($this->mxRecords)) {
            //    $this->warnings[NoDNSMXRecord::CODE] = new NoDNSMXRecord();
            // }
        }

        return false;

and then do something like todo A and AAAA check only when MX failed:

        $mxRecordsResult = $this->dnsGetRecord->getRecords($host, DNS_MX);

        if (! $mxRecordsResult->withError()
+           && $this->validateMxRecords($mxRecordsResult->getRecords())) {
-            $dnsRecords = $mxRecordsResult->getRecords();
+            return true;
        }

        // Combined check for A+MX can fail with connection timed out, even in the presence of valid MX record
        $aRecordsResult = $this->dnsGetRecord->getRecords($host, DNS_A);

        if (! $aRecordsResult->withError()
+           && $this->validateMxRecords($aRecordsResult->getRecords())) {
-            $dnsRecords = array_merge($dnsRecords, $aRecordsResult->getRecords());
+            return true;
        }

        // Combined check for A+MX+AAAA can fail with SERVFAIL, even in the presence of valid A/MX records
        $aaaaRecordsResult = $this->dnsGetRecord->getRecords($host, DNS_AAAA);

        if (! $aaaaRecordsResult->withError()
+           && $this->validateMxRecords($aaaaRecordsResult->getRecords())) {
-            $dnsRecords = array_merge($dnsRecords, $aaaaRecordsResult->getRecords());
+            return true;
        }
        
+        if ($mxRecordsResult->withError()
+            && $aRecordsResult->withError()
+            && $aaaaRecordsResult->withError()
+        ) {
+            $this->error = new InvalidEmail(new UnableToGetDNSRecord(), '');
+            return false;
+        }
+        if ([] === $mxRecordsResult->getRecords()
+            && [] === $aRecordsResult->getRecords()
+            && [] === $aaaaRecordsResult->getRecords()
+        ) {
+            $this->error = new InvalidEmail(new ReasonNoDNSRecord(), '');
+            return false;
+        }

+        $this->warnings[NoDNSMXRecord::CODE] = new NoDNSMXRecord();
        
        return false;

but I'm not deep into this Lib or DNS records and maybe the early return false make sense here to avoid something unexpectly happening.

Copy link
Owner

@egulias egulias Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general strategy of the lib is fail fast . Now for this case, given that there are network roundtrips was to "confirm fase" if that make sense. Hence not checking all records, with one we do have a valid MX. Happy to change if you believe will improve accuracy.

With a more in depth validation like your proposal, I agree to the general idea you are suggesting on this comment: we go from more accurate to less accurate and we short-circuit when we find a match.

Implementation wise, I'd go for something like

        if (! $mxRecordsResult->withError()
            && $this->validateMxRecords($mxRecordsResult->getRecords())) {
             $dnsRecords = $mxRecordsResult->getRecords();
              return true;
        }
       // We know there are no MX records already
       $this->warnings[NoDNSMXRecord::CODE] = new NoDNSMXRecord();
      //If you feel like so, you can create new warnings for each missing DNS record
      //...

     //do the logic for error setting here
     $this->setError($mxRecordsResult, $aRecordsResult, $aaaaRecordsResult)

}
}

return true;
}

Expand Down
Loading