Mika56 / PHP-SPF-Check

Simple library to check an IP address against a domain's SPF record
MIT License
44 stars 25 forks source link

Check fails for empty MX record #45

Closed sLIDe5 closed 1 year ago

sLIDe5 commented 1 year ago

We're getting PHP Warning: dns_get_record(): An unexpected server failure occurred. in /path/to/vendor/mika56/spfcheck/src/DNS/DNSRecordGetter.php on line 18 when trying to validate SPF records for some domains. In our case the culprit seems to be in \Mika56\SPFCheck\DNS\DNSRecordGetter::resolveMx method where an empty domain is returned and it then fails down the line.

dns_get_record('example.com', DNS_MX) returns the following data

[
    [
         "host" => "example.com",
         "class" => "IN",
         "ttl" => 0,
         "type" => "MX",
         "pri" => 0,
         "target" => "",
     ],
     [
         "host" => "example.com",
         "class" => "IN",
         "ttl" => 0,
         "type" => "MX",
         "pri" => 10,
         "target" => "record.example.com",
     ],
]

And this means that resolveMx method returns the following array ["", "record.example.com"] which then fails in resolveA method.

Probably need to add check in resolveMx to skip entries with empty target or automatically return false in resolveA if an empty domain is passed.

Mika56 commented 1 year ago

Do you think this could be a domain implementing the proposed standard RFC7505? It seems to me it is unclear as what should be done. Should they count towards void lookups which are limited to 2 per evaluation? Could you try and tell me how a tool such as MXToolbox reacts with your example domain?

sLIDe5 commented 1 year ago

Looks like you're right about RFC7507 although there are still other MX records.

As for what should be done I don't have any ideas.

The domain we tried this on is mavms.be

Mika56 commented 1 year ago

I could not find any information on what should be done in this case in the RFCs, but it looks like the good practice is to ignore the faulty MX records. I'm merging your PR, thanks!

Mika56 commented 1 year ago

Interestingly, I just discovered it was already covered by the test "MX mechanism syntax: mx-empty" from the SPF suite test, but the DNSRecordGetterOpenSPF class was not behaving as it should have and the test was passing wrongly. I'm moving your check to Session, which makes it working with any DNSRecordGetterInterface.