Skip to content

Improved support for internationalised domains #102

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

Merged
merged 6 commits into from
Jun 20, 2018
Merged
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions lib/github-pages-health-check.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# frozen_string_literal: true

require "dnsruby"
require "addressable/idna"
require "addressable/uri"
require "ipaddr"
require "public_suffix"
5 changes: 4 additions & 1 deletion lib/github-pages-health-check/caa.rb
Original file line number Diff line number Diff line change
@@ -33,7 +33,10 @@ def records_present?
end

def records
@records ||= (get_caa_records(host) | get_caa_records(PublicSuffix.domain(host)))
unicode_host = Addressable::IDNA.to_unicode(host)
@records ||= begin
get_caa_records(host) | get_caa_records(PublicSuffix.domain(unicode_host))
end
end

private
12 changes: 7 additions & 5 deletions lib/github-pages-health-check/domain.rb
Original file line number Diff line number Diff line change
@@ -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(self.host,
:nameservers => nameservers)
end

@@ -147,7 +147,8 @@ def invalid_cname?
# Used as an escape hatch to prevent false positives on DNS checkes
def valid_domain?
return @valid if defined? @valid
@valid = PublicSuffix.valid?(host, :default_rule => nil)
unicode_host = Addressable::IDNA.to_unicode(host)
@valid = PublicSuffix.valid?(unicode_host, :default_rule => nil)
end

# Is this domain an apex domain, meaning a CNAME would be innapropriate
@@ -160,7 +161,8 @@ def apex_domain?
# It's aware of multi-step top-level domain names:
# E.g. PublicSuffix.domain("blog.digital.gov.uk") # => "digital.gov.uk"
# For apex-level domain names, DNS providers do not support CNAME records.
PublicSuffix.domain(host) == host
unicode_host = Addressable::IDNA.to_unicode(host)
PublicSuffix.domain(unicode_host) == unicode_host
end

# Should the domain use an A record?
@@ -421,8 +423,8 @@ def https_response
# Return the hostname.
def normalize_host(domain)
domain = domain.strip.chomp(".")
host = Addressable::URI.parse(domain).host
host ||= Addressable::URI.parse("http://#{domain}").host
host = Addressable::URI.parse(domain).normalized_host
host ||= Addressable::URI.parse("http://#{domain}").normalized_host
host unless host.to_s.empty?
rescue Addressable::URI::InvalidURIError
nil
2 changes: 1 addition & 1 deletion lib/github-pages-health-check/resolver.rb
Original file line number Diff line number Diff line change
@@ -32,7 +32,7 @@ def initialize(domain, nameservers: :default)
end

def query(type)
resolver.query(domain, type).answer
resolver.query(Addressable::IDNA.to_ascii(domain), type).answer
end

private
132 changes: 132 additions & 0 deletions spec/github_pages_health_check/domain_spec.rb
Original file line number Diff line number Diff line change
@@ -543,6 +543,30 @@
expect(subject).to be_valid
end
end

context "domains with unicode encoding" do
let(:domain) { "dómain.example.com" }
let(:cname) { "sómething.example.com" }
let(:headers) { { :server => "GitHub.com" } }
before(:each) { allow(subject).to receive(:dns) { [cname_packet] } }

it "doesn't error out on domains with unicode encoding" do
expect(subject).to be_served_by_pages
expect(subject).to be_valid
end
end

context "domains with punycode encoding" do
let(:domain) { "xn--dmain-0ta.example.com" }
let(:cname) { "xn--smething-v3a.example.com" }
let(:headers) { { :server => "GitHub.com" } }
before(:each) { allow(subject).to receive(:dns) { [cname_packet] } }

it "doesn't error out on domains with punycode encoding" do
expect(subject).to be_served_by_pages
expect(subject).to be_valid
end
end
end

context "not served by pages" do
@@ -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] } }
Copy link
Member Author

Choose a reason for hiding this comment

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

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.


it { is_expected.not_to be_https_eligible }

context "with unicode encoded domain" do
let(:domain) { "dómain.example.com" }

it { is_expected.not_to be_https_eligible }
end

context "with punycode encoded domain" do
let(:domain) { "xn--dmain-0ta.example.com" }

it { is_expected.not_to be_https_eligible }
end
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] } }
Copy link
Member Author

Choose a reason for hiding this comment

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

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.


it { is_expected.to be_https_eligible }

@@ -859,11 +897,52 @@

it { is_expected.to be_https_eligible }
end

context "with unicode encoded domain" do
let(:domain) { "dómain.example.com" }

it { is_expected.to be_https_eligible }

context "with bad CAA records" do
let(:caa_domain) { "digicert.com" }
before(:each) { allow(subject.send(:caa)).to receive(:query) { [caa_packet] } }

it { is_expected.not_to be_https_eligible }
end

context "with good CAA records" do
let(:caa_domain) { "letsencrypt.org" }
before(:each) { allow(subject.send(:caa)).to receive(:query) { [caa_packet] } }

it { is_expected.to be_https_eligible }
end
end

context "with punycode encoded domain" do
let(:domain) { "xn--dmain-0ta.example.com" }

it { is_expected.to be_https_eligible }

context "with bad CAA records" do
let(:caa_domain) { "digicert.com" }
before(:each) { allow(subject.send(:caa)).to receive(:query) { [caa_packet] } }

it { is_expected.not_to be_https_eligible }
end

context "with good CAA records" do
let(:caa_domain) { "letsencrypt.org" }
before(:each) { allow(subject.send(:caa)).to receive(:query) { [caa_packet] } }

it { is_expected.to be_https_eligible }
end
end
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] } }
Copy link
Member Author

Choose a reason for hiding this comment

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

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.


it { is_expected.to be_https_eligible }

@@ -880,11 +959,52 @@

it { is_expected.to be_https_eligible }
end

context "with unicode encoded domain" do
let(:domain) { "dómain.example.com" }

it { is_expected.to be_https_eligible }

context "with bad CAA records" do
let(:caa_domain) { "digicert.com" }
before(:each) { allow(subject.send(:caa)).to receive(:query) { [caa_packet] } }

it { is_expected.not_to be_https_eligible }
end

context "with good CAA records" do
let(:caa_domain) { "letsencrypt.org" }
before(:each) { allow(subject.send(:caa)).to receive(:query) { [caa_packet] } }

it { is_expected.to be_https_eligible }
end
end

context "with punycode encoded domain" do
let(:domain) { "xn--dmain-0ta.example.com" }

it { is_expected.to be_https_eligible }

context "with bad CAA records" do
let(:caa_domain) { "digicert.com" }
before(:each) { allow(subject.send(:caa)).to receive(:query) { [caa_packet] } }

it { is_expected.not_to be_https_eligible }
end

context "with good CAA records" do
let(:caa_domain) { "letsencrypt.org" }
before(:each) { allow(subject.send(:caa)).to receive(:query) { [caa_packet] } }

it { is_expected.to be_https_eligible }
end
end
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] } }
Copy link
Member Author

Choose a reason for hiding this comment

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

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.


it { is_expected.not_to be_https_eligible }

@@ -901,6 +1021,18 @@

it { is_expected.not_to be_https_eligible }
end

context "with unicode encoded domain" do
let(:domain) { "dómain.example.com" }

it { is_expected.not_to be_https_eligible }
end

context "with punycode encoded domain" do
let(:domain) { "xn--dmain-0ta.example.com" }

it { is_expected.not_to be_https_eligible }
end
end
end
end