nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.62k stars 29.6k forks source link

How about support other representations for `net.isIPv4()`? #38860

Open XadillaX opened 3 years ago

XadillaX commented 3 years ago

Is your feature request related to a problem? Please describe.

> net.isIPv4('123')
false // expected to be true
> net.isIPv4('1234')
false // expected to be true
> net.isIPv4('123.1')
false // expected to be true
> net.isIPv4('123.1.1.1')
true

Refer: https://en.wikipedia.org/wiki/IPv4#Address_representations

Describe the solution you'd like

If we can modify that method, I can make a pull request.

Describe alternatives you've considered

Some more representations for IPv6 too (I'm not sure).

mscdex commented 3 years ago

I'd say modifying the existing method is probably not worth the potential breakage.

XadillaX commented 3 years ago

I'd say modifying the existing method is probably not worth the potential breakage.

Maybe by adding a secondary optional parameter?

Ayase-252 commented 3 years ago

How about a new method? Something like net.isIPv4Representation (probably not a good name)

bl-ue commented 3 years ago

For reference:

$ ping 1
PING 1 (0.0.0.1): 56 data bytes
$ ping 123
PING 123 (0.0.0.123): 56 data bytes
$ ping 123456789
PING 123456789 (7.91.205.21): 56 data bytes
$ ping 1234567890
PING 1234567890 (73.150.2.210): 56 data bytes
XadillaX commented 3 years ago

For reference:

$ ping 1
PING 1 (0.0.0.1): 56 data bytes
$ ping 123
PING 123 (0.0.0.123): 56 data bytes
$ ping 123456789
PING 123456789 (7.91.205.21): 56 data bytes
$ ping 1234567890
PING 1234567890 (73.150.2.210): 56 data bytes

That's it.

Trott commented 3 years ago

Other than a sense of correctness, what real world problem would this change solve? It seems to have the potential to introduce many, many real world problems. I'm sure many users will be surprised to find that their filtering on variations of 127.0.0.1 can now be bypassed with 127.1 because we've started allowing that. (Users should have allow lists for stuff like that rather than block lists, but that's not the point.)

jasnell commented 3 years ago

I'm generally -1 on this. There is no defined standard for IPv4 text representation and support for non-typical alternatives is inconsistent at best. @Trott's point about filters based on the idiomatic de facto standard representation is quite valid here.

Trott commented 3 years ago

We could introduce text (if it's not already there) in the docs indicating that the function only supports quad-dotted notation (or dot-decimal notation, or whatever the standard accepted term is).

And if we do want to add something more flexible, we would want to preserve default behavior and have the more lenient functionality involve passing an explicit option. The option could even have many values, such as one that allows (or even requires) CIDR notation, and one that allows any 32-bit representation. But I think the functionality would be prone to bugs and not used by many people. I don't think it's a good use of our resources. But if there are significant and convincing use cases out there for it that I'm not aware of, I could be persuaded.