search

whitequark / ipaddr.js

IP address manipulation library in JavaScript
MIT License
570 stars 92 forks source link

Compact CIDR parsing produces wrong results. #136

Closed haight6716 closed 3 years ago

haight6716 commented 4 years ago

const ipaddr = require('ipaddr.js'); const range = ipaddr.parseCIDR('8/8'); range.toString() === '8.0.0.0/8' // false range.toString() '0.0.0.8/8'

kennethtran93 commented 3 years ago

I don't think this is an actual issue as I haven't heard of a 'Compact CIDR' before. CIDR is already a compact form for the subnet 192.168.1.0/255.255.255.0 => 192.168.1.0/24.

The only compact portion may be in IPv6 addressing, with the :: on one side.

If you were looking for compact IPv6, it would most likely be 8::/8 or ::8/8

whitequark commented 3 years ago

@kennethtran93 Thanks for the investigation!

haight6716 commented 3 years ago

Things may exist that you haven't heard of.

Here is a stack overflow question dealing with the issue, so clearly I didn't just make this up on my own: https://networkengineering.stackexchange.com/questions/30694/is-it-ok-to-leave-the-trailing-0s-off-in-ipv4-cidr-notation

The answers there are the same as yours (clutching pearls) but I assure you, people use this shorthand and it saves time.

Story time: I'm working on commercial software designed for network operations terms. A request for this feature came in from our customers. I wanted to use this library to implement it, but obviously I couldn't. Wound up rolling my own. Filed the issue to try to help.

Even if you don't support it, you shouldn't be converting it incorrectly.

kennethtran93 commented 3 years ago

It's not converting it incorrectly per se, but in reference to #44 and here: https://linux.die.net/man/3/inet_aton, if only one octet is given, it is perseved as a 32-bit value as is. That is the intended format.

This script separates the ip and cidr and processes it separately. It does first attempt to parse as ipv6 first before ipv4.

In your own script you can wrap around this function by checking the ip given, and if cidr is given then manually append the '.0' as many time as you need until a full ip address is given. Or probably just one '.0' may even be enough.

haight6716 commented 3 years ago

https://github.com/whitequark/ipaddr.js/pull/143

haight6716 commented 3 years ago

I think in the context of a cidr string, it's more likely to be the abbreviated format I'm talking about. I have seen numeric ips in urls and such - where parsing them as numbers makes sense, but this is different. The CIDR slash implies that the first part is a dotted-quad string. See my PR - I think it improves this case and doesn't hurt anything else - the other tests all still pass.