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

fix(ValidateFQDN): Modifying ValidateFQDN to only look at ICANN domains #534

Closed

Conversation

MattSilvaa
Copy link
Contributor

This PR addresses #532

To make ngrok.io not return false, we have to ignore all private domains in the PSL and only look at ICANN ones.

Copy link
Member

@ehsandeep ehsandeep left a comment

Choose a reason for hiding this comment

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

@MattSilvaa this results into a new issue with other domains.

validate-domains $./validate-domains 
validate-domains $bat invalid_domains.txt 
───────┬────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: invalid_domains.txt
───────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ clientondersteuning.co.nl
   2   │ mailfoogae.appspot.com
   3   │ d2evh2mef3r450.cloudfront.net
───────┴────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

@ehsandeep ehsandeep linked an issue Jun 25, 2023 that may be closed by this pull request
@MattSilvaa
Copy link
Contributor Author

MattSilvaa commented Jun 25, 2023

@MattSilvaa this results into a new issue with other domains.

validate-domains $./validate-domains 
validate-domains $bat invalid_domains.txt 
───────┬────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: invalid_domains.txt
───────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ clientondersteuning.co.nl
   2   │ mailfoogae.appspot.com
   3   │ d2evh2mef3r450.cloudfront.net
───────┴────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

I think the right move is to update our publicsuffix package to github.com/weppos/publicsuffix-go/publicsuffix so that it's pulling the latest PSL and then to also update chaos-bugbounty-list to remove all the incorrect domain names for ngrok... it should only be ngrok.com. This is the result of validate-domains invalid_domains file using the updated publicsuffix package while not ignoring private tlds.

ngrok.io
ngrok-free.app
ngrok-free.dev
ngrok.app
ngrok.dev

As you can see, it outlines the bug which is that we are using the wrong domain values for ngrok. I will push up the changes. Looking forward to hear your thoughts.

@ehsandeep
Copy link
Member

@MattSilvaa, thanks for the update, I've updated publicsuffix-go to the latest commit and still see the same issue, removing domains from the list is not a solution as a) we need to keep those domains 2) we can have more domains in future with same TLD's.

Copy link
Member

@tarunKoyalwar tarunKoyalwar left a comment

Choose a reason for hiding this comment

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

@MattSilvaa thanks for PR but we currently only rely on publicsuffix for domain validation which is not the case with domains like http://ngrok.io or http://ai . i have created a issue to furthur improve support for TLDs here

we can close this PR and if you are interested , you can pick that issue

@MattSilvaa MattSilvaa closed this Jun 27, 2023
@MattSilvaa MattSilvaa deleted the msilva/532-ValidateFQDN branch June 27, 2023 03:07
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.

Bug in ValidateFQDN function
3 participants