-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Make isFQDN
more strict
#1180
Conversation
{ allow_numeric_tld: true, require_tld: false }, | ||
], | ||
valid: [ | ||
'example.0', |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
cc. @profnandaa.
@CristhianMotoche -- just fix the merge conflicts and we should get this in, thanks! |
Sorry for the late reply. Sure thing @profnandaa I'll do it later today. |
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. |
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 🙂