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

Make isFQDN more strict #1180

Closed
wants to merge 1 commit into from
Closed

Make isFQDN more strict #1180

wants to merge 1 commit into from

Conversation

CristhianMotoche
Copy link
Contributor

Hello!

I reviewed the issue described in #704 and I applied the solution suggested there. I think making isFQDN more strict is good solution. Nevertheless, there are some discussions about it. I'd like to know your thoughts.

Please, let me know if this PR solves #704 🙂

{ allow_numeric_tld: true, require_tld: false },
],
valid: [
'example.0',
Copy link
Member

Choose a reason for hiding this comment

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

Why would example.0 be a valid FQDN? 🤔 And even .9999, is there anything I'm not understanding...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @profnandaa ! Thanks for the quick response.

Why would example.0 be a valid FQDN? And even .9999, is there anything I'm not understanding...

In this test case, allow_numeric_tld is true and require_tld is false, this mimics the current behavior of isFQDN when require_tld is false. In that way, isFQDN will allow numeric TLDs.

I think this solution make sense and doesn't complicate the implementation of isFQDN.

Other options?

The RFC 3696 mentions two ways to validate the TLD:

One is by polling the list published by IANA DomainList and validate against it. However, based on the comment in #704:

Ideally the validator would check TLDs against a list of known values, however the addition of gTLDs mean that the list would be long and ever-growing. I'm not interested in bloating the library or keeping the list up to date.

Since we don't want to keep that list up today, the second option is to validate that the TLD name is not all-numeric.

Does it make sense?

Note:

I think we could avoid the parameter allow_numeric_tld and always require that the TLD is not all-numeric. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. Really sorry for the long delay on this one, can't believe it has almost been a year!

/cc. @tux-tn @ezkemboi -- could you please have a look at this and advise.

Copy link
Member

Choose a reason for hiding this comment

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

It makes sense to me @profnandaa.
After checking on comments and some research online, I agree with the changes.

Copy link
Member

@ezkemboi ezkemboi left a comment

Choose a reason for hiding this comment

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

LGTM.
cc. @profnandaa.

@profnandaa
Copy link
Member

@CristhianMotoche -- just fix the merge conflicts and we should get this in, thanks!

@profnandaa profnandaa added the mc-to-land Just merge-conflict standing between the PR and landing. label Oct 5, 2020
@CristhianMotoche
Copy link
Contributor Author

Sorry for the late reply. Sure thing @profnandaa I'll do it later today.

@CristhianMotoche
Copy link
Contributor Author

Hey @profnandaa I'm really sorry but I think at some point I removed my fork from my repository. 😞 I'll open a new issue. I'm sorry for the inconvenience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mc-to-land Just merge-conflict standing between the PR and landing. needs-more-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants