Improved support for internationalised domains #102

snh · 2018-06-09T10:09:19Z

I came across a couple of issues with internationalised domain names today that I hope to solve in this PR.

PublicSuffix doesn't support Punycode encoded internationalised public suffixes, and instead expects them to be Unicode encoded (weppos/publicsuffix-ruby#46). If you feed in a Punycode encoded public suffix, then no result, or a more broader result, may be returned.

Impacts:

Domain.valid_domain? can return false, leading to almost everything in Domain failing, including Domain.https_eligible? always returning false.
CAA.records isn't able to retrieve the CAA records for the apex domain.

Unfortunately Dnsruby is the reverse way around, and expects the internationalised domain to be Punycode encoded, and exceptions if fed a Unicode encoded internationalised domain (alexdalitz/dnsruby#94).

Impacts:

Feeding a Unicode encoded internationalised domain in to most of the GitHubPages::HealthCheck::Domain or GitHubPages::HealthCheck::CAA methods will lead to a Dnsruby exception.

To work around these issues, I am feeding the domain in to Addressable's normalized_host method, which converts the internationalised domain to Punycode encoding, which appears to be the generally more accepted way to work with domains in code. I am then re-encoding in Unicode where required (which is just when using PublicSufix for now).

Addressable's normalized_host method will also take care of removing a trailing dot, making our existing way to doing this redundant.

These encoding transformations cope fine with a non-internationalised domain.

I have added tests, though these actually pass even without these fixes, as to surface this bug you need to use a domain with internationalised characters in the public suffix. I didn't want to pick a random name, since it may be in use, or registered in the future. It might be worth us registering a domain in one of the internationalised country code top-level domain's which appears in the public suffix list.

Glossary:

Public Suffix: As defined at https://publicsuffix.org/, an example is the com portion of pages.github.com.
Apex Domain: As defined at https://help.github.com/articles/about-supported-custom-domains/#apex-domains, an example is the github.com portion of pages.github.com`.

PublicSuffix doesn't support Punycode encoded internationalised domain names, and instead expects them to be unicode encoded. Ref: weppos/publicsuffix-ruby#46.

Dnsruby expects the string containing the host to be ascii encoded. Ref: alexdalitz/dnsruby#94. Addressable's normalized_host method will also take care of removing a trailing dot.

snh · 2018-06-09T10:11:03Z

spec/github_pages_health_check/domain_spec.rb

    end

    context "CNAME record pointed to username" do
      let(:cname) { "foobar.github.io" }
      before(:each) { allow(subject).to receive(:dns) { [cname_packet] } }
+      before(:each) { allow(subject.send(:caa)).to receive(:query) { [cname_packet] } }


This is required so that we return the CNAME when no CAA exists, imitating the behaviour seen in the real world. This hasn't been a issue until now, as the existing checks use a domain of pages.github.com, so we get back entries from the real world here. It is only now that I have started using fake names that this has become a problem.

snh · 2018-06-09T10:11:34Z

spec/github_pages_health_check/domain_spec.rb

    end

    context "A records pointed to new IPs" do
      let(:ip) { "185.199.108.153" }
      before(:each) { allow(subject).to receive(:dns) { [a_packet] } }
+      before(:each) { allow(subject.send(:caa)).to receive(:query) { [a_packet] } }


This is required so that we return the A when no CAA exists, imitating the behaviour seen in the real world. This hasn't been a issue until now, as the existing checks use a domain of pages.github.com, so we get back entries from the real world here. It is only now that I have started using fake names that this has become a problem.

snh · 2018-06-09T10:11:44Z

spec/github_pages_health_check/domain_spec.rb

    end

    context "CNAME record pointed elsewhere" do
      let(:cname) { "jinglebells.com" }
      before(:each) { allow(subject).to receive(:dns) { [cname_packet] } }
+      before(:each) { allow(subject.send(:caa)).to receive(:query) { [cname_packet] } }


This is required so that we return the CNAME when no CAA exists, imitating the behaviour seen in the real world. This hasn't been a issue until now, as the existing checks use a domain of pages.github.com, so we get back entries from the real world here. It is only now that I have started using fake names that this has become a problem.

snh · 2018-06-09T10:12:18Z

spec/github_pages_health_check/domain_spec.rb

@@ -836,13 +860,27 @@
    context "A records pointed to old IPs" do
      let(:ip) { "192.30.252.153" }
      before(:each) { allow(subject).to receive(:dns) { [a_packet] } }
+      before(:each) { allow(subject.send(:caa)).to receive(:query) { [a_packet] } }


This is required so that we return the A when no CAA exists, imitating the behaviour seen in the real world. This hasn't been a issue until now, as the existing checks use a domain of pages.github.com, so we get back entries from the real world here. It is only now that I have started using fake names that this has become a problem.

benbalter

Didn't do a code review, but 👍 to the approach (and GREAT write up).

snh · 2018-06-10T08:04:14Z

lib/github-pages-health-check/domain.rb

@@ -100,7 +100,7 @@ def initialize(host, nameservers: :default)

        @host = normalize_host(host)
        @nameservers = nameservers
-        @resolver = GitHubPages::HealthCheck::Resolver.new(host,
+        @resolver = GitHubPages::HealthCheck::Resolver.new(@host,


Found this while undertaking some more testing just then when I managed to trigger some further Dnsruby exceptions when using a Unicode encoded IDN. Seems we weren't passing the normalised host through to the Resolver class.

The host method should have been normalized?

As host exists as a locally scoped variable in the initialize method, my understanding is that we need to explicitly refer to the instance level scoped host where we want to use it, otherwise we will get back the locally scoped one, which is pre normalize_host.

$ cat local-vs-instance.rb def test(host) @host = host.upcase puts "host: #{host}, @host: #{@host}" end test("github.com") $ ruby local-vs-instance.rb host: github.com, @host: GITHUB.COM

Totally right! The host parameter is first in the order of precedence, then the interpreter will attempt to find a method called host. I'd love to use self.host here so that we're calling the method. I'm worried about reordering these calls and causing @host to be nil by accident. What do you think?

Thanks @parkr, I'm a little lost here, as from what I understand we don't have a host method?, would you mind expanding on what you mean here?

On line 6, there's an attr_reader for :host which creates a reader for @host! That's where I'm understanding that there is a host method. 😄

Thanks @parkr, it finally clicked 😄 I've updated the PR to use self.host.

While not necessary at the moment, doing this in case we ever use this class directly elsewhere.

snh added 3 commits June 9, 2018 18:49

Test with internationalized domain names

c8a1e16

Feed a unicode encoded host to publicsuffix

67aa75b

PublicSuffix doesn't support Punycode encoded internationalised domain names, and instead expects them to be unicode encoded. Ref: weppos/publicsuffix-ruby#46.

Convert host to ascii

591019d

Dnsruby expects the string containing the host to be ascii encoded. Ref: alexdalitz/dnsruby#94. Addressable's normalized_host method will also take care of removing a trailing dot.

snh added the bug label Jun 9, 2018

snh commented Jun 9, 2018

View reviewed changes

benbalter approved these changes Jun 9, 2018

View reviewed changes

Pass the normalised host in to the resolver

05cd822

snh commented Jun 10, 2018

View reviewed changes

snh added 2 commits June 10, 2018 18:38

Always pass a punycode encoded domain to dnsruby

228d097

While not necessary at the moment, doing this in case we ever use this class directly elsewhere.

Use the host method

cd45272

parkr approved these changes Jun 20, 2018

View reviewed changes

parkr merged commit 97ce501 into github:master Jun 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improved support for internationalised domains #102

Improved support for internationalised domains #102

Uh oh!

snh commented Jun 9, 2018

Uh oh!

snh Jun 9, 2018

Uh oh!

snh Jun 9, 2018

Uh oh!

snh Jun 9, 2018

Uh oh!

snh Jun 9, 2018

Uh oh!

benbalter left a comment

Uh oh!

snh Jun 10, 2018

Uh oh!

parkr Jun 11, 2018

Uh oh!

snh Jun 12, 2018

Uh oh!

parkr Jun 12, 2018

Uh oh!

snh Jun 15, 2018

Uh oh!

parkr Jun 15, 2018

Uh oh!

snh Jun 20, 2018

Uh oh!

Uh oh!

Improved support for internationalised domains #102

Improved support for internationalised domains #102

Uh oh!

Conversation

snh commented Jun 9, 2018

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!

benbalter 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!

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!

Uh oh!