improve port matching

Author: steven10a

src/guardrails/checks/text/urls.py

@@ -320,13 +320,13 @@ def _is_url_allowed(
     url_host = url_host.lower()
     url_domain = url_host.replace("www.", "")
     scheme_lower = parsed_url.scheme.lower() if parsed_url.scheme else ""
-    # Check if port was explicitly specified (safely)
+    # Safely get port (rejects malformed ports)
+    url_port = _safe_get_port(parsed_url, scheme_lower)
+    # Early rejection of malformed ports
     try:
-        url_port_explicit = parsed_url.port
+        _ = parsed_url.port  # This will raise ValueError for malformed ports
     except ValueError:
-        # Malformed port (out of range or invalid) - reject the URL
         return False
-    url_port = _safe_get_port(parsed_url, scheme_lower)
     url_path = parsed_url.path or "/"
     url_query = parsed_url.query
     url_fragment = parsed_url.fragment
@@ -368,8 +368,8 @@ def _is_url_allowed(
             # Scheme matching for IPs: if both allow list and URL have explicit schemes, they must match
             if has_explicit_scheme and url_had_explicit_scheme and allowed_scheme and allowed_scheme != scheme_lower:
                 continue
-            # Port matching: only enforce if either side explicitly specified a port
-            if (allowed_port_explicit is not None or url_port_explicit is not None) and allowed_port != url_port:
+            # Port matching: enforce if allow list has explicit port
+            if allowed_port_explicit is not None and allowed_port != url_port:
                 continue
             if allowed_ip == url_ip:
                 return True
@@ -390,8 +390,8 @@ def _is_url_allowed(
 
         allowed_domain = allowed_host.replace("www.", "")
 
-        # Port matching: only enforce if either side explicitly specified a port
-        if (allowed_port_explicit is not None or url_port_explicit is not None) and allowed_port != url_port:
+        # Port matching: enforce if allow list has explicit port
+        if allowed_port_explicit is not None and allowed_port != url_port:
             continue
 
         host_matches = url_domain == allowed_domain or (

tests/unit/checks/test_urls.py

@@ -230,30 +230,34 @@ def test_is_url_allowed_handles_cidr_blocks() -> None:
 
 
 def test_is_url_allowed_handles_port_matching() -> None:
-    """Port matching: only enforced when explicitly specified."""
+    """Port matching: enforced if allow list has explicit port, otherwise any port allowed."""
     config = URLConfig(
         url_allow_list=["https://example.com:8443", "api.internal.com"],
         allow_subdomains=False,
         allowed_schemes={"https"},
     )
-    # Explicit port 8443 matches allow list
+    # Explicit port 8443 matches allow list's explicit port → ALLOWED
     correct_port, _, had_scheme1 = _validate_url_security("https://example.com:8443", config)
-    # Implicit 443 doesn't match explicit 8443 in allow list
+    # Implicit 443 doesn't match allow list's explicit 8443 → BLOCKED
     wrong_port, _, had_scheme2 = _validate_url_security("https://example.com", config)
-    # Explicit 9000 doesn't match implicit default in allow list (no port = not checking)
-    explicit_port_vs_implicit, _, had_scheme3 = _validate_url_security("https://api.internal.com:9000", config)
-    # Implicit default matches implicit default
+    # Explicit 9000 with no port restriction in allow list → ALLOWED
+    explicit_port_no_restriction, _, had_scheme3 = _validate_url_security("https://api.internal.com:9000", config)
+    # Implicit 443 with no port restriction in allow list → ALLOWED
     implicit_match, _, had_scheme4 = _validate_url_security("https://api.internal.com", config)
+    # Explicit default 443 with no port restriction in allow list → ALLOWED (regression fix)
+    explicit_default_port, _, had_scheme5 = _validate_url_security("https://api.internal.com:443", config)
 
     assert correct_port is not None  # noqa: S101
     assert wrong_port is not None  # noqa: S101
-    assert explicit_port_vs_implicit is not None  # noqa: S101
+    assert explicit_port_no_restriction is not None  # noqa: S101
     assert implicit_match is not None  # noqa: S101
+    assert explicit_default_port is not None  # noqa: S101
 
     assert _is_url_allowed(correct_port, config.url_allow_list, config.allow_subdomains, had_scheme1) is True  # noqa: S101
     assert _is_url_allowed(wrong_port, config.url_allow_list, config.allow_subdomains, had_scheme2) is False  # noqa: S101
-    assert _is_url_allowed(explicit_port_vs_implicit, config.url_allow_list, config.allow_subdomains, had_scheme3) is False  # noqa: S101
+    assert _is_url_allowed(explicit_port_no_restriction, config.url_allow_list, config.allow_subdomains, had_scheme3) is True  # noqa: S101
     assert _is_url_allowed(implicit_match, config.url_allow_list, config.allow_subdomains, had_scheme4) is True  # noqa: S101
+    assert _is_url_allowed(explicit_default_port, config.url_allow_list, config.allow_subdomains, had_scheme5) is True  # noqa: S101
 
 
 def test_is_url_allowed_handles_query_and_fragment() -> None:
@@ -415,6 +419,31 @@ def test_is_url_allowed_handles_ipv6_addresses() -> None:
     assert _is_url_allowed(ipv6_with_ftp, config.url_allow_list, config.allow_subdomains, had_scheme2) is True  # noqa: S101
 
 
+def test_is_url_allowed_handles_ipv6_cidr_notation() -> None:
+    """IPv6 CIDR blocks should be handled correctly (brackets stripped, path concatenated)."""
+    config = URLConfig(
+        url_allow_list=["[2001:db8::]/64", "[fe80::]/10"],
+        allow_subdomains=False,
+        allowed_schemes={"https"},
+    )
+    # IP within first CIDR range
+    ip_in_range1, _, had_scheme1 = _validate_url_security("https://[2001:db8::1234]", config)
+    # IP within second CIDR range
+    ip_in_range2, _, had_scheme2 = _validate_url_security("https://[fe80::5678]", config)
+    # IP outside CIDR ranges
+    ip_outside, _, had_scheme3 = _validate_url_security("https://[2001:db9::1]", config)
+
+    assert ip_in_range1 is not None  # noqa: S101
+    assert ip_in_range2 is not None  # noqa: S101
+    assert ip_outside is not None  # noqa: S101
+
+    # IPs within CIDR ranges should be allowed
+    assert _is_url_allowed(ip_in_range1, config.url_allow_list, config.allow_subdomains, had_scheme1) is True  # noqa: S101
+    assert _is_url_allowed(ip_in_range2, config.url_allow_list, config.allow_subdomains, had_scheme2) is True  # noqa: S101
+    # IP outside should be blocked
+    assert _is_url_allowed(ip_outside, config.url_allow_list, config.allow_subdomains, had_scheme3) is False  # noqa: S101
+
+
 @pytest.mark.asyncio
 async def test_urls_guardrail_blocks_subdomains_and_paths_correctly() -> None:
     """Verify subdomains and paths are still blocked according to allow list rules."""