Add basic checking of mx records for mailto links #57

tomtom5152 · 2017-08-20T13:08:40Z

Helps ensure that the domain is registered and at least partially configured to recieve email.

codecov-io · 2017-08-20T13:11:43Z

Codecov Report

Merging #57 into master will decrease coverage by 2.21%.
The diff coverage is 60.97%.

@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
- Coverage   95.67%   93.45%   -2.22%     
==========================================
  Files          17       17              
  Lines         902      917      +15     
==========================================
- Hits          863      857       -6     
- Misses         34       49      +15     
- Partials        5       11       +6

Impacted Files	Coverage Δ
htmltest/check-link.go	`91.6% <60.97%> (-3.5%)`	⬇️
htmltest/htmltest.go	`89.18% <0%> (-8.63%)`	⬇️
htmltest/options.go	`89.06% <0%> (-0.17%)`	⬇️
htmldoc/document.go	`88.37% <0%> (+0.27%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d42cbee...75fff69. Read the comment docs.

tomtom5152 · 2017-08-20T13:14:22Z

Coverage has decreased as I can't think of a way to trigger the errors to execute those lines of code. In theory they should never execute, but it is a bit of catch all error handling in a default and else statement.

davidtaylorhq · 2017-08-20T13:53:15Z

Does this pass for the case that a domain has no MX records, but has a valid A/CNAME record? (I believe it should)

tomtom5152 · 2017-08-20T14:53:07Z

I believe you are correct according to this RFC 5321 Section 5 but that doesn't current exist.

wjdp · 2017-08-23T19:23:57Z

Looking at the changes coverage is fine.

Re. the RFC my understanding is as follows:

Checking items off as I read them

DNS lookup must be performed
First check for MX
If CNAME present restart from beginning
If neither, fallback to A or AAAA
If NXDOMAIN then error
If haven't found a valid record then error

wjdp

Guessing you're not finished yet, but have some comments 😄

wjdp · 2017-08-26T15:48:20Z

htmltest/check-link.go

+			continue
+
+		} else if dnserr, ok = err.(*net.DNSError); !ok || dnserr.Err != "no such host" {
+			// this isn't an error we are expecting to see here


The exact error on Linux for the loop CNAME test is server misbehaving (value of dnserr.Err) which seems in-line-ish with nslookup:

~ ❯ nslookup loop1.dtaylor.uk Server: 127.0.1.1 Address: 127.0.1.1#53 ** server can't find loop1.dtaylor.uk: SERVFAIL

Likely an error discrepancy between Linux and OSX. You may need to restructure a bit to handle this cleanly.

I'm not sure if this is infact an issue, no such host is an isolated case where we want to proceed to checking A records. Should a CNAME loop not produce an warning of some variety?
I accept that this may be inconsistent between platforms, but I feel the Linux way is actually better in this case and the OS X way is an acceptable fallback?

wjdp · 2017-08-26T15:48:37Z

htmltest/check-link.go

+
+	domain := strings.Split(ref.URL.Opaque, "@")[1]
+
+	for domain != "" {


Comment here would be nice: we're going to loop until we find a valid record or error

wjdp · 2017-08-26T15:51:39Z

htmltest/check-link_test.go

+func TestMailtoInvalidCname(t *testing.T) {
+	// fails for invalid mailto links
+	hT := tTestFile("fixtures/links/invalid_mailto_cname.html")
+	tExpectIssueCount(t, hT, 0)


Guessing this isn't done yet. In general write the tests with expected result and add t.Skip("Not yet dealt with") at the top.

The test was complete, it was checking that just a CNAME pointing to another domain that was valid would work as expected, comments should hopefully clear that up.

wjdp · 2017-08-26T15:52:43Z

htmltest/check-link_test.go

+func TestMailtoInvalidAFallback(t *testing.T) {
+	// fails for invalid mailto links
+	hT := tTestFile("fixtures/links/invalid_mailto_fallback.html")
+	tExpectIssueCount(t, hT, 0)


wjdp · 2017-08-26T15:54:14Z

htmltest/check-link_test.go

 	// fails for invalid mailto links
 	hT := tTestFile("fixtures/links/invalid_mailto_link.html")
 	tExpectIssueCount(t, hT, 1)
 	tExpectIssue(t, hT, "contains an invalid email address", 1)
 }

+func TestMailtoInvalidCname(t *testing.T) {
+	// fails for invalid mailto links


Write these comments so we can get an idea of what's actually being tested

Helps ensure that the domain is registered and at least partially configured to recieve email.

As per https://tools.ietf.org/html/rfc5321#section-5 this should fallback to the hostname for that domain.

LookupMX will perform this if the CNAME result has an MX record as well, but the spec states we should "start again" so we will do just that. This allows fallback to A/AAAA records in a CNAME -> A configuration. Loops are handled by the LookupCNAME method.

tomtom5152 · 2017-09-11T11:20:12Z

Rebased and made the changes requested.

wjdp requested changes Aug 26, 2017

View reviewed changes

tomtom5152 added 3 commits September 11, 2017 12:12

Add basic checking of mx records for mailto links

c545c7c

Helps ensure that the domain is registered and at least partially configured to recieve email.

Add fallback to A/AAAA records if no MX records are found.

8349811

As per https://tools.ietf.org/html/rfc5321#section-5 this should fallback to the hostname for that domain.

tomtom5152 force-pushed the mailto-mx branch from c10c559 to 7d0f854 Compare September 11, 2017 11:19

Add comments to tests as requested by @wjdp

75fff69

tomtom5152 force-pushed the mailto-mx branch from 7d0f854 to 75fff69 Compare September 11, 2017 11:27

wjdp self-assigned this Jan 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic checking of mx records for mailto links #57

Add basic checking of mx records for mailto links #57

tomtom5152 commented Aug 20, 2017

codecov-io commented Aug 20, 2017 •

edited

Loading

tomtom5152 commented Aug 20, 2017

davidtaylorhq commented Aug 20, 2017 •

edited

Loading

tomtom5152 commented Aug 20, 2017 •

edited

Loading

wjdp commented Aug 23, 2017 •

edited

Loading

wjdp left a comment

wjdp Aug 26, 2017

tomtom5152 Sep 11, 2017

wjdp Aug 26, 2017

wjdp Aug 26, 2017

tomtom5152 Sep 11, 2017

wjdp Aug 26, 2017

wjdp Aug 26, 2017

tomtom5152 commented Sep 11, 2017


		domain := strings.Split(ref.URL.Opaque, "@")[1]

		for domain != "" {

Add basic checking of mx records for mailto links #57

Are you sure you want to change the base?

Add basic checking of mx records for mailto links #57

Conversation

tomtom5152 commented Aug 20, 2017

codecov-io commented Aug 20, 2017 • edited Loading

Codecov Report

tomtom5152 commented Aug 20, 2017

davidtaylorhq commented Aug 20, 2017 • edited Loading

tomtom5152 commented Aug 20, 2017 • edited Loading

wjdp commented Aug 23, 2017 • edited Loading

wjdp left a comment

Choose a reason for hiding this comment

wjdp Aug 26, 2017

Choose a reason for hiding this comment

tomtom5152 Sep 11, 2017

Choose a reason for hiding this comment

wjdp Aug 26, 2017

Choose a reason for hiding this comment

wjdp Aug 26, 2017

Choose a reason for hiding this comment

tomtom5152 Sep 11, 2017

Choose a reason for hiding this comment

wjdp Aug 26, 2017

Choose a reason for hiding this comment

wjdp Aug 26, 2017

Choose a reason for hiding this comment

tomtom5152 commented Sep 11, 2017

codecov-io commented Aug 20, 2017 •

edited

Loading

davidtaylorhq commented Aug 20, 2017 •

edited

Loading

tomtom5152 commented Aug 20, 2017 •

edited

Loading

wjdp commented Aug 23, 2017 •

edited

Loading