fixed wrongly approved zero padded ip 4

[yonjah]

No description provided.

[chriso]

Thanks.

[addaleax]

Btw, this still leaves validator.isIP('002.2.2.2') === true. If one really wants to exclude IPv4s with leading zeroes, one should rather turn \d into [1-9] here.

[yonjah]

I'm not sure how complex you want this regex to be (since in general regex might have their own security issues)
I guess if your only checking if the first value is not zero it might not get too complex but
you still have to think about the case of 0.0.0.0 which is commonly used.

Also I'm not sure if everyone considers leading zeros to be completely invalid so this might better go with a feature flag or in a major release.

[addaleax]

I agree that leading zeroes are not necessarily a problem (and yes, sorry, didn’t think of things like 0.0.0.0 or 10.0.0.1), but I feel like making a difference between 002.2.2.2 and 0200.2.2.2.2 is kind of arbitrary?

[yonjah]

how about 00000000000001.01.01.01 ?
or 1000's padded zeroes ?
what happens if this data later passes to a db or a log file without checking for its length (since user assumes a valid IP address )

IPv4 is made out of maximum three digits subset it makes sense not to allow any padding to anything more than that

[addaleax]

Yeah, I do get why one may want to forbid leading zeroes, and I’m not disagreeing in any way with that being a good idea! :) I’m just saying the way it’s implemented now is inconsistent, and inconsistency is usually something you want to avoid (for obvious reasons).

[yonjah]

Ah, I guess I didn't understand what you meant.
I guess I don't see it as inconsistency.
Might be more consistent if you wont look at it a s a padding limit but as a size limit
Each subset of the ip is allowed to have up to 3 chars.
so 0200.2.2.2.2 breaks this rule but also 1200.2.2.2.2 but 012.2.2.2 or 12.2.2.2 do not

[addaleax]

Exactly, I’m a little afraid someone will trip over the difference between handling 0200.2.2.2 and 012.2.2.2, that’s all.
And I’d be in favor of going stricter and forbidding leading zeroes at all, because a) I don’t see any way one would intentionally write it like that and b) the leading-zero format itself seems to be confusing to others, too; e.g. Node.js resolves '012.2.2.2' to '12.2.2.2', but the ping(1) on my machine reads 012 as octal and translates the address to 10.2.2.2.

[yonjah]

Agree.
I think preventing leading zeroes in all might be a good idea, but I would still treat it as a breaking change

[yonjah]

Here is an implementation suggestion.
I decided not to use regex since I don't see the benefit of using it for such a complex test.
(you'll either get a really long and complex regex that will be hard to understand or a very basic regex that will any way require more code to validate )

if (str.length < 7 || str.length > 15) {
    return false;
}
var parts = str.split('.');
var i;
var num;
if (parts.length !== 4)  {
    return false;
}
for(i = 0; i < 4; i += 1) {
    num = parseInt(parts[i], 10);
    if (!(num >= 0 && num <= 255 && num.toString() === parts[i])) {
        return false;
    }
}
return true;

[yonjah]

BTW, I checked many js libraries that validate ips (joi, ip, ip-address)
and I tried to look at some other validation suggestions online.
I couldn't find any that doesn't allow leading zeros in ips ( well I actually found one but it also declared '127.0.0.1' as invalid ip ) and while I still agree that ips shouldn't have leading zeros this will probably be inconsistent with any other library currently out there so I would be quite hesitant about actually doing it.

[addaleax]

+1 on your implementation! I’d suggest you add a comment for explaining where the “magic” constants 7 and 15 in the first line come from (I mean, it’s kind of clear, but it might save others the time of doing the calculations each time when they first look at the code). Feel free to go ahead and create a new PR :)