fix: reject leading-zero octets in the ipv4 type#1904
Conversation
|
Thanks for opening a PR! Could you also fix ipv6 in this PR please? |
ErenAta16
left a comment
There was a problem hiding this comment.
Verified the fix independently rather than just checking the examples in the description: brute-forced all 256 canonical octet values (0-255) against both the old and new patterns, plus every leading-zero variant of each (single and double leading zero for 1-2 digit values). Old pattern accepts 10 leading-zero forms it shouldn't (00 through 09 as bare octets, on top of the two/three-digit cases already called out); new pattern rejects all of them while still matching every canonical value 0-255 and correctly rejecting anything above 255. Also re-checked the four full-address bad cases plus a handful of good ones (0.0.0.0, 255.255.255.255, 192.168.1.1, etc.) against the new pattern end to end.
On the ipv6 scope note: confirmed the same vulnerable octet form (1?[0-9]{1,2}) is indeed still present, twice, in the ipv4-mapped branches of the ipv6 regex a few lines down. Agreed this PR is fine to land without touching it, but it's a real gap, not a hypothetical one — worth a fast follow so ipv6's embedded IPv4 suffix doesn't silently diverge from the standalone ipv4 type's behavior.
One more data point for the "this is the established fix shape" case beyond semver and the date type: the email type's domain-literal branch ([a-z0-9-]*[a-z0-9]: alternative, for bracketed IP-literal addresses) already uses the exact 1[0-9][0-9]|[1-9]?[0-9] form this PR introduces. So this brings ipv4 in line with a pattern that's already present three times elsewhere in this same file, not just twice.
This looks correct and ready to merge from my side.
|
@RobinPicard @NishchayMahor filed the ipv6 side as a separate issue with the exact repro and fix shape, in case it's useful for scoping this PR or a follow-up: #1911. Same root cause (the old |
The ipv4 octet subpattern 1?[0-9]{1,2} accepted 2-digit leading-zero octets
(01.1.1.1, 00.0.0.0, 1.1.1.09) while rejecting the 3-digit form (010.1.1.1) --
an internal inconsistency, and it accepts strings Python's ipaddress and RFC
canonical form reject. Use a no-leading-zero octet, matching the semver type
(which already rejects 01.2.3) and the merged date-'00' fix. Adds test cases.
c7c8eba to
615f0ae
Compare
|
📚 Documentation preview: https://dottxt-ai.github.io/outlines/pr-preview/pr-1904/ Preview updates automatically with each commit. |
What
The
ipv4type's octet subpattern1?[0-9]{1,2}accepts 2-digit leading-zero octets but rejects the 3-digit form, so it over-accepts non-canonical addresses:types.ipv4(current)ipaddress01.1.1.100.0.0.01.1.1.09010.1.1.1That last row shows the inconsistency — a 3-digit leading-zero octet is already rejected, but the 2-digit one isn't. The constraint should reject leading zeros in all forms, matching RFC canonical dotted-decimal.
Fix
Use a no-leading-zero octet
(25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9]?[0-9])in theipv4definition (src/outlines/types/__init__.py). This is consistent with:semvertype, which already rejects leading zeros ("01.2.3"→False, pertest_custom_types.py), and"00".Scope: this PR intentionally touches only the standalone
ipv4type. Theipv6regex embeds the same octet pattern for IPv4-mapped addresses; I've left it untouched to keep this surgical and can follow up separately.Testing
Added leading-zero rejection rows to
test_type_regexintests/types/test_custom_types.py.pytest tests/types/test_custom_types.py -k test_type_regex→ 114 passed with the fix; the new rows fail onmain. Pure regex, offline. Exhaustively verified all canonical octets 0–255 still match and leading-zero forms are rejected.