Skip to content
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.

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

return HasMXRecord as true when at least one valid mx records exist #94

Merged
merged 1 commit into from
Jun 26, 2023

Conversation

avivshafir
Copy link

Some domains may have several MX records and some of them may be invalid
for example nslookup for the domain test-unicode.definbox.com returns "some" valid MX records

I think we should return True in cases when at least one MX record exist and is valid.

Currently the library will return has_mx_records = False in this case although I think this is incorrect behavior
Inside the Go library, they made changes to support this case
https://github.com/golang/go/blob/dd96ded6c86b8a38f49fa087b758455243a0f08c/src/net/lookup.go#L515

some additional references:
https://go-review.googlesource.com/c/go/+/333331
golang/go#46979

I wanted to add a relevant test but if the tests are actually performing the MX check, if the DNS record is fixed in those domains the test will fail, making it unstable.
Any thoughts?

@lryong
Copy link
Contributor

lryong commented Jun 17, 2023

Hi @avivshafir , I'm glad to see your PR, I did go and see these references you have mentioned about Golang's own discussion of LookupMX.
But for this PR, I don't really recommend this change because I haven't seen enough test cases to illustrate the data for this case. I think if there is a problem with LookupMX, the better way is to modify it in Golang's source code, or in the future it will be modified to the way you mentioned.
If your project relies on the MX checking capability of email-verifier, and if you encounter an error while checking, then determine if the error is errMalformedDNSRecordsDetail, or see this issue, where we provide enable_mx capability so that users can extend MX checking on their own. 😃

@lryong lryong requested review from git-hulk and NeoCN June 17, 2023 14:01
@avivshafir avivshafir closed this Jun 18, 2023
@avivshafir avivshafir reopened this Jun 18, 2023
@avivshafir
Copy link
Author

@lryong Thanks for the feedback.
The response you are returning right now isn't correct, as a domain may have a valid MX record (HasMXRecord) even though it has invalid others.
Just checking the errMalformedDNSRecordsDetail won't be enough for most cases as you don't return the MX records
and is also a weird way to validate if MX records exist.

@git-hulk
Copy link
Member

@lryong I agree with what @avivshafir mentioned. It should return the correct Mx record even though we expected users to be aware of this error.

@lryong
Copy link
Contributor

lryong commented Jun 20, 2023

Okay, I can understand this point

@lryong lryong merged commit 5cce611 into AfterShip:main Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants