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

Why 127.0.0.1 valid and how to validate only cidr notation #290

Closed
mertcangokgoz opened this issue Aug 22, 2023 · 2 comments · Fixed by #295
Closed

Why 127.0.0.1 valid and how to validate only cidr notation #290

mertcangokgoz opened this issue Aug 22, 2023 · 2 comments · Fixed by #295
Assignees
Labels
bug Issue: Works not as designed

Comments

@mertcangokgoz
Copy link

mertcangokgoz commented Aug 22, 2023

I wrote a cidr validation code like below

def ip_address_cidr_validation(value: str) -> bool:
    """Validate IP address in CIDR notation."""
    return bool(
        validators.ipv4(value, cidr=True, strict=True)
        or validators.ipv6(value, cidr=True, strict=True)
    )

while there was no problem in older versions, I saw that the following 3 values are "True" in the latest version of the library.

shouldn't the following three ip's be false? it's not "cidr"

2001:0db8:85a3:0000:0000:8a2e:0370:7334 -> False
127.0.0.1 -> False
1.1.1.1/24 -> False
192.0.2.0/255.255.255.0 -> False

My test case

@pytest.mark.parametrize(
    ("cidr", "status"),
    [
        ("1.1.1.1/32", True),
        ("178.251.45.0/24", True),
        ("2001:0db8:85a3:0000:0000:8a2e:0370:7334/128", True),
        ("192.0.2.0/255.255.255.0", False),
        ("192.168.0.0/32", True),
        ("1.1.1.1/24", True),
        ("127.0.0.1", False),
        ("127.0.0.2.4", False),
        ("2001:0db8:85a3:0000:0000:8a2e:0370:7334", False),
        ("192.168.0. 1", False),
        ("192.168.0.1 /32", False),
    ],
)
def test_ipaddress_cidr_validation_valid(cidr: str, status: bool) -> None:
    """Ensure the function works correctly when CIDR is entered correctly."""
    cidr_validator = ip_address_cidr_validation(cidr)
    assert cidr_validator is status

How can this happen? Am I applying this validator incorrectly?

@yozachar
Copy link
Collaborator

@mertcangokgoz, thanks for raising this issue, I see now that the strict parameter is confusing. Reference source:

if not value:
return False
try:
if cidr and value.count("/") == 1:
return IPv4Network(value, strict=strict)
return IPv4Address(value)
except (AddressValueError, NetmaskValueError):
return False

Here's what strict currently does.

  • It is only used when cidr=True and the input string has a /.
  • But instead of invalidating IP addresses without CIDR notation (as you've assumed),
  • It distinguishes between a network and a host IP address.

So when cidr=True and strict=True, if host bits are set in the supplied IP address, it raises a validation error.

$ python -c "from validators import ipv4; print(ipv4('192.168.1.5/24', strict=True))"
ValidationError(func=ipv4, args={'reason': '192.168.1.5/24 has host bits set', 'value': '192.168.1.5/24', 'strict': True})
$ # Expected a network address like 192.168.1.0 (where network part is `192.168.1` and host part is `0`)

I'm pondering on a solution, I'll update you when I'm ready. Meanwhile feel free to comment below.

@yozachar
Copy link
Collaborator

Here's the patch
diff --git a/src/validators/ip_address.py b/src/validators/ip_address.py
index 3ae638d..e6d01f6 100644
--- a/src/validators/ip_address.py
+++ b/src/validators/ip_address.py
@@ -15,7 +15,7 @@ from .utils import validator
 
 
 @validator
-def ipv4(value: str, /, *, cidr: bool = True, strict: bool = False):
+def ipv4(value: str, /, *, cidr: bool = True, strict: bool = False, host_bit: bool = True):
     """Returns whether a given value is a valid IPv4 address.
 
     From Python version 3.9.5 leading zeros are no longer tolerated
@@ -36,11 +36,12 @@ def ipv4(value: str, /, *, cidr: bool = True, strict: bool = False):
         value:
             IP address string to validate.
         cidr:
-            IP address string may contain CIDR annotation
+            IP address string may contain CIDR notation
         strict:
-            If strict is True and host bits are set in the supplied address.
-            Otherwise, the host bits are masked out to determine the
-            appropriate network address. ref [IPv4Network][2].
+            IP address string is strictly in CIDR notation
+        host_bit:
+            If `False` and host bits (along with network bits) _are_ set in the supplied
+            address, this function raises a validation error. ref [IPv4Network][2].
             [2]: https://docs.python.org/3/library/ipaddress.html#ipaddress.IPv4Network
 
     Returns:
@@ -58,15 +59,17 @@ def ipv4(value: str, /, *, cidr: bool = True, strict: bool = False):
     if not value:
         return False
     try:
-        if cidr and value.count("/") == 1:
-            return IPv4Network(value, strict=strict)
+        if cidr:
+            if strict and value.count("/") != 1:
+                raise ValueError("IPv4 address was expected in CIDR notation")
+            return IPv4Network(value, strict=not host_bit)
         return IPv4Address(value)
-    except (AddressValueError, NetmaskValueError):
+    except (ValueError, AddressValueError, NetmaskValueError):
         return False
 
 
 @validator
-def ipv6(value: str, /, *, cidr: bool = True, strict: bool = False):
+def ipv6(value: str, /, *, cidr: bool = True, strict: bool = False, host_bit: bool = True):
     """Returns if a given value is a valid IPv6 address.
 
     Including IPv4-mapped IPv6 addresses. The initial version of ipv6 validator
@@ -88,9 +91,10 @@ def ipv6(value: str, /, *, cidr: bool = True, strict: bool = False):
         cidr:
             IP address string may contain CIDR annotation
         strict:
-            If strict is True and host bits are set in the supplied address.
-            Otherwise, the host bits are masked out to determine the
-            appropriate network address. ref [IPv6Network][2].
+            IP address string is strictly in CIDR notation
+        host_bit:
+            If `False` and host bits (along with network bits) _are_ set in the supplied
+            address, this function raises a validation error. ref [IPv6Network][2].
             [2]: https://docs.python.org/3/library/ipaddress.html#ipaddress.IPv6Network
 
     Returns:
@@ -108,8 +112,10 @@ def ipv6(value: str, /, *, cidr: bool = True, strict: bool = False):
     if not value:
         return False
     try:
-        if cidr and value.count("/") == 1:
-            return IPv6Network(value, strict=strict)
+        if cidr:
+            if strict and value.count("/") != 1:
+                raise ValueError("IPv6 address was expected in CIDR notation")
+            return IPv6Network(value, strict=not host_bit)
         return IPv6Address(value)
-    except (AddressValueError, NetmaskValueError):
+    except (ValueError, AddressValueError, NetmaskValueError):
         return False
diff --git a/tests/test_ip_address.py b/tests/test_ip_address.py
index 312e98e..f28cdec 100644
--- a/tests/test_ip_address.py
+++ b/tests/test_ip_address.py
@@ -13,10 +13,6 @@ from validators import ValidationError, ipv4, ipv6
         ("127.0.0.1",),
         ("123.5.77.88",),
         ("12.12.12.12",),
-        # w/ cidr
-        ("127.0.0.1/0",),
-        ("123.5.77.88/8",),
-        ("12.12.12.12/32",),
     ],
 )
 def test_returns_true_on_valid_ipv4_address(address: str):
@@ -25,6 +21,22 @@ def test_returns_true_on_valid_ipv4_address(address: str):
     assert not ipv6(address)
 
 
+@pytest.mark.parametrize(
+    ("address", "cidr", "strict", "host_bit"),
+    [
+        ("127.0.0.1/0", True, True, True),
+        ("123.5.77.88", True, False, True),
+        ("12.12.12.0/24", True, True, False),
+    ],
+)
+def test_returns_true_on_valid_ipv4_cidr_address(
+    address: str, cidr: bool, strict: bool, host_bit: bool
+):
+    """Test returns true on valid ipv4 CIDR address."""
+    assert ipv4(address, cidr=cidr, strict=strict, host_bit=host_bit)
+    assert not ipv6(address, cidr=cidr, strict=strict, host_bit=host_bit)
+
+
 @pytest.mark.parametrize(
     ("address",),
     [
@@ -33,10 +45,6 @@ def test_returns_true_on_valid_ipv4_address(address: str):
         ("900.200.100.75",),
         ("0127.0.0.1",),
         ("abc.0.0.1",),
-        # w/ cidr
-        ("1.1.1.1/-1",),
-        ("1.1.1.1/33",),
-        ("1.1.1.1/foo",),
     ],
 )
 def test_returns_failed_validation_on_invalid_ipv4_address(address: str):
@@ -44,6 +52,22 @@ def test_returns_failed_validation_on_invalid_ipv4_address(address: str):
     assert isinstance(ipv4(address), ValidationError)
 
 
+@pytest.mark.parametrize(
+    ("address", "cidr", "strict", "host_bit"),
+    [
+        ("1.1.1.1/1", False, True, True),
+        ("1.1.1.1/33", True, False, True),
+        ("1.1.1.1/24", True, True, False),
+        ("1.1.1.1/-1", True, True, True),
+    ],
+)
+def test_returns_failed_validation_on_invalid_ipv4_cidr_address(
+    address: str, cidr: bool, strict: bool, host_bit: bool
+):
+    """Test returns failed validation on invalid ipv4 CIDR address."""
+    assert isinstance(ipv4(address, cidr=cidr, strict=strict, host_bit=host_bit), ValidationError)
+
+
 @pytest.mark.parametrize(
     ("address",),
     [
@@ -56,14 +80,6 @@ def test_returns_failed_validation_on_invalid_ipv4_address(address: str):
         ("::192.168.30.2",),
         ("0000:0000:0000:0000:0000::",),
         ("0:a:b:c:d:e:f::",),
-        # w/ cidr
-        ("::1/128",),
-        ("::1/0",),
-        ("dead:beef:0:0:0:0:42:1/8",),
-        ("abcd:ef::42:1/32",),
-        ("0:0:0:0:0:ffff:1.2.3.4/16",),
-        ("2001:0db8:85a3:0000:0000:8a2e:0370:7334/64",),
-        ("::192.168.30.2/128",),
     ],
 )
 def test_returns_true_on_valid_ipv6_address(address: str):
@@ -72,6 +88,26 @@ def test_returns_true_on_valid_ipv6_address(address: str):
     assert not ipv4(address)
 
 
+@pytest.mark.parametrize(
+    ("address", "cidr", "strict", "host_bit"),
+    [
+        ("::1/128", True, True, True),
+        ("::1/0", True, True, True),
+        ("dead:beef:0:0:0:0:42:1/8", True, True, True),
+        ("abcd:ef::42:1/32", True, True, True),
+        ("0:0:0:0:0:ffff:1.2.3.4/16", True, True, True),
+        ("2001:0db8:85a3:0000:0000:8a2e:0370:7334/64", True, True, True),
+        ("::192.168.30.2/128", True, True, True),
+    ],
+)
+def test_returns_true_on_valid_ipv6_cidr_address(
+    address: str, cidr: bool, strict: bool, host_bit: bool
+):
+    """Test returns true on valid ipv6 CIDR address."""
+    assert ipv6(address, cidr=cidr, strict=strict, host_bit=host_bit)
+    assert not ipv4(address, cidr=cidr, strict=strict, host_bit=host_bit)
+
+
 @pytest.mark.parametrize(
     ("address",),
     [
@@ -91,12 +127,24 @@ def test_returns_true_on_valid_ipv6_address(address: str):
         ("::1:2::",),
         ("8::1:2::9",),
         ("02001:0000:1234:0000:0000:C1C0:ABCD:0876",),
-        # w/ cidr
-        ("::1/129",),
-        ("::1/-1",),
-        ("::1/foo",),
     ],
 )
 def test_returns_failed_validation_on_invalid_ipv6_address(address: str):
     """Test returns failed validation on invalid ipv6 address."""
     assert isinstance(ipv6(address), ValidationError)
+
+
+@pytest.mark.parametrize(
+    ("address", "cidr", "strict", "host_bit"),
+    [
+        ("::1/128", False, True, True),
+        ("::1/129", True, False, True),
+        ("dead:beef:0:0:0:0:42:1/8", True, True, False),
+        ("::1/-130", True, True, True),
+    ],
+)
+def test_returns_failed_validation_on_invalid_ipv6_cidr_address(
+    address: str, cidr: bool, strict: bool, host_bit: bool
+):
+    """Test returns failed validation on invalid ipv6 CIDR address."""
+    assert isinstance(ipv6(address, cidr=cidr, strict=strict, host_bit=host_bit), ValidationError)

PRs are welcome.

@yozachar yozachar added the bug Issue: Works not as designed label Aug 23, 2023
yozachar added a commit to yozachar/pyvalidators that referenced this issue Sep 2, 2023
yozachar added a commit to yozachar/pyvalidators that referenced this issue Sep 2, 2023
@yozachar yozachar self-assigned this Sep 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue: Works not as designed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants