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

Custom domain simplifaction #163

Merged
merged 21 commits into from
Apr 16, 2024
Merged

Custom domain simplifaction #163

merged 21 commits into from
Apr 16, 2024

Conversation

tsusdere
Copy link
Contributor

@tsusdere tsusdere commented Apr 9, 2024

closes: https://github.com/github/pages-engineering/issues/4205

Relaxes the validation process for CNAME by checking if the domain it points to points to GitHub Pages. This is done by creating a new method which checks if there is a record of type A in the DNS to which the CNAME points to. If this is the case then we just check if that points to Pages.

@tsusdere tsusdere marked this pull request as ready for review April 10, 2024 16:13
@tsusdere tsusdere requested a review from a team April 10, 2024 16:14
@@ -243,6 +243,16 @@ def cname_to_github_user_domain?
cname? && !cname_to_pages_dot_github_dot_com? && cname.pages_domain?
end

# Check if the CNAME points to a Domain that points to pages
# e.g. CNAME -> Domain -> Pages
def cname_to_domain_to_pages?
Copy link
Contributor

Choose a reason for hiding this comment

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

this function seems not check the cname point to A record which points to pages.

Does this allow the user input arbitrary cname?

Copy link
Contributor Author

@tsusdere tsusdere Apr 10, 2024

Choose a reason for hiding this comment

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

It should the main thing is that we are checking for is that the CNAME doesn't already point to us then we run this check

return true if cname_to_github_user_domain? || cname_to_domain_to_pages?

I have a test that checks if it points to it but let me add a test in the event that it doesn't

Copy link
Contributor

@YiMysty YiMysty Apr 10, 2024

Choose a reason for hiding this comment

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

I mainly mean if we want to only support www.fontawesome.it => fontawesome.it => pagesxxx

or loosely support blog.fontawesome.it => fontawesome.it => pagesxxxx

hmmm loosely is fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohh I see let me double check on that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it looks like we just want to support www.fontawesome.it => fontawesome.it => pages where we match the host after the www

@tsusdere tsusdere requested a review from YiMysty April 10, 2024 17:53
@YiMysty
Copy link
Contributor

YiMysty commented Apr 10, 2024

version file need to be changed if this need consume somewhere.

@@ -410,6 +410,8 @@ def cname
cnames = dns.take_while { |answer| answer.type == Dnsruby::Types::CNAME }
return if cnames.empty?

# check to see if the CNAME starts with www
@wwwcname ||= cnames.last.name.to_s.start_with?("www")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might want this to be www. so we don't match on wwwblog.cname.com?

Suggested change
@wwwcname ||= cnames.last.name.to_s.start_with?("www")
@wwwcname ||= cnames.last.name.to_s.start_with?("www.")

@tsusdere tsusdere merged commit 7fd2e16 into master Apr 16, 2024
3 checks passed
@tsusdere tsusdere deleted the custom-domain-simplifaction branch April 16, 2024 15:58
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