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

Impossible to validate host (domain name or IP) #704

Closed
kachkaev opened this issue Aug 26, 2017 · 6 comments
Closed

Impossible to validate host (domain name or IP) #704

kachkaev opened this issue Aug 26, 2017 · 6 comments

Comments

@kachkaev
Copy link

kachkaev commented Aug 26, 2017

Hi folks,

Just came across your library this morning and was quite surprised with the richness of it. Super nice!

Here's one one observation from a newbie: I've been trying to validate a host and have noticed that the combination of isFQDN and isIP does match my expectations in a couple of edge cases.

Check this out:

const validateHost = (input) => {
  if (!isFQDN(input, { require_tld: false }) && !isIP(input)) {
    throw new EnvError(`Invalid host (domain or ip): "${input}"`)
  }
}
validateHost('example.com')     // no exception, as expected
validateHost('localhost')       // no exception, as expected
validateHost('localhost.')      // exception, as expected

validateHost('192.168.0.1')     // no exception, as expected
validateHost('192.168.0')       // no exception, despite expected (isFQDN returned true!)
validateHost('192.168.0.9999')  // no exception, despite expected (isFQDN returned true!)

The function I've written is supposed to check environment variables in a microservice, so it's important to accept both domain names and IP addresses. It is also necessary to make sure there are no typos in the IP address (ideally both for ipv4 and ipv6).

How about making isFQDN a bit more strict? Or what about isHost() that would work slightly better than a combination of two validators?

@chriso
Copy link
Collaborator

chriso commented Aug 26, 2017

I'd be ok with making isFQDN more strict and would be happy to accept a PR. 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.

An alternative might be to disallow numeric TLDs? I'm not aware of any numeric gTLDs, and in fact this may not be allowed by ICANN. It'd be safest to control the behavior with a new option: allow_numeric_tld

Something like this @ isFQDN.js:31 would work:

if (!options.allow_numeric_tld && i == parts.length - 1 && /^\d+$/.test(part)) {
   return false; // reject numeric TLDs
}

@mika-s
Copy link
Contributor

mika-s commented Aug 26, 2017

I guess you could do a check for IP address in isFQDN and return false if it is one. Not as an option but more as a guard clause.

The proposed isHost is pretty much the same as isURL I think, when you set require_valid_protocol = false and require_tld = false.

@maikelmclauflin
Copy link

this also fails. is this expected?

validator.isURL('http://docker-continer-web')

@chriso
Copy link
Collaborator

chriso commented May 16, 2018

@maikelmclauflin try require_tld:

> validator.isURL('http://docker-continer-web')
false
> validator.isURL('http://docker-continer-web', { require_tld: false })
true

@Jassi10000-zz
Copy link

Can someone close this issue as the PR is merged already

@kachkaev
Copy link
Author

kachkaev commented Oct 25, 2021

Closing as resolved in #1474

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants