fix(net/unstable): parse IP addresses per the URL standard - #7316
Open
tomas-zijdemans wants to merge 3 commits into
Open
fix(net/unstable): parse IP addresses per the URL standard#7316tomas-zijdemans wants to merge 3 commits into
tomas-zijdemans wants to merge 3 commits into
Conversation
`isIPv4()` was a `split(".")` plus `Number()` per part, which accepts
whitespace, leading zeros, hex, exponent and sign forms, and empty
octets. `isIPv6()` expanded `::` by hand and accepted a leading lone
`":"` and repeated `"::"`.
Adds `parseIPv4()` and `parseIPv6()`, returning the address bytes or
`undefined`, and reduces `isIPv4()`/`isIPv6()` to `parseX() !== undefined`.
`parseIPv4()` implements the URL standard's valid IPv4-address string
grammar (strict dotted quad, shortest decimal spelling). `parseIPv6()`
implements the URL standard's IPv6 parser, which omits zone IDs.
`matchSubnets()`, `matchIPv4Subnet()` and `matchIPv6Subnet()` now go
through the parsers, so `expandIPv6()` and `ipv6ToBytes()` are gone.
CIDR prefix lengths are now validated as decimal digits within range
rather than handed to `parseInt()`, which read `/0x18` as 0 and so
matched every address. Zero-padded lengths such as `/024` keep working:
unlike an address octet they have only one reading.
Inputs accepted before and rejected now:
| Input | was | now |
|---|---|---|
| `" 1.2.3.4"`, `"1.2.3.4 "` | true | false |
| `"01.002.3.4"` | true | false |
| `"0x7f.0.0.1"` | true | false |
| `"1e2.1.1.1"` | true | false |
| `"+1.2.3.4"` | true | false |
| `"1.2.3."`, `"1..2.3"` | true | false |
| `":1"` | true | false |
| `"2001:db8:::1"` | true | false |
| `"::1 "` | true | false |
| `"::ffff:0x1.2.3.4"` | true | false |
| `"fe80::1%eth0"` | true | false |
Plus, in subnet prefix lengths, the `parseInt()` leniencies: `/0x18`,
`/+24`, `/ 24`, `/24abc`, `/1e1`, `/24/8`.
Every behavior change is an invalid input that is no longer accepted, so
this is `fix` rather than `BREAKING`.
Towards denoland#7315.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7316 +/- ##
==========================================
- Coverage 95.03% 95.03% -0.01%
==========================================
Files 617 617
Lines 51637 51636 -1
Branches 9359 9365 +6
==========================================
- Hits 49075 49073 -2
Misses 2021 2021
- Partials 541 542 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The `/ 24`, `/+24` and `/1e1` rows checked `1.2.3.4` against
`192.168.1.0`, which is outside the subnet under any prefix length those
spellings could produce. They passed on the base implementation too, so
they proved nothing. They now use `192.168.1.1`, which the base
implementation does match: `parseInt()` read `/1e1` as 1 and the rest as
24. Adds the same row for `/0x18`, which meant 0.
Also adds `1:2:3:4:5:6:7::` and `::1:2:3:4:5:6:7` as positive cases. The
old `isIPv6()` rejected both: a leading or trailing `::` makes
`split(":")` return 9 elements, so the `while (hextets.length < 8)`
expansion never ran and the length check failed. Enumerating every
`::` position over 0 to 9 groups, with and without an IPv4 tail, these
two are the only valid addresses the old implementation rejected.
Two of the three guards in the IPv4-tail branch of parseIPv6() had no test on their reject side. Both are load-bearing: - pieceIndex > 6: the pieceIndex !== 8 backstop only runs when there is no compress, so with a "::" present this guard is the only thing rejecting an over-long tail. Without it "1:2:3:4:5:6::1.2.3.4" parses as valid, because the ninth write falls off the end of the Uint16Array and is dropped. - !isDigit: without it "::1.2.3." and "::1..2.3" are accepted. Each row was checked against a build with its guard removed, so none of them pass for the wrong reason. The third partial, the length === 0 check, is left uncovered on purpose. No input can discriminate it: when length is 0 the rewind is a no-op and numbersSeen is 0, so the dot is not consumed and the !isDigit check rejects on the same character. It is kept for fidelity to the URL standard's algorithm, where it is equally redundant.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First step of #7315. Adds
parseIPv4()andparseIPv6(), which return theaddress bytes or
undefined, and rebuildsisIPv4(),isIPv6()and the threesubnet matchers on top of them. This follows through on the parser design
0f-0b asked for
in #6765.
Problem
isIPv4()split on.and passed each part toNumber(), which happily readshex, exponents, signs, leading zeros, whitespace and the empty string.
isIPv6()expanded::by hand and never checked that colons sat where thegrammar wants them.
isIPv4" 1.2.3.4","1.2.3.4 "truefalse"01.002.3.4"truefalse"0x7f.0.0.1"truefalse"1e2.1.1.1"truefalse"+1.2.3.4"truefalse"1.2.3.","1..2.3"truefalseisIPv6":1"truefalse"2001:db8:::1"truefalse"::1 "truefalse"::ffff:0x1.2.3.4"truefalse"fe80::1%eth0"truefalse, zone IDs are out of scopematchSubnets()gated on a privateisValidIP()built from the same twofunctions, so every one of those reached the matchers.
Change
parseIPv4()implements the URL standard'svalid IPv4-address string:
four decimal octets, shortest spelling, no leading zeros. That last rule is what
stops
010.0.0.1from being read as octal, a parser differential with a longCVE history. Go, Rust and Python all reject it too.
parseIPv6()is a transcription of the URL standard'sIPv6 parser, including the
trailing
x:x:x:x:x:x:d.d.d.dform. Zone IDs are now rejected, which thestandard calls out as deliberate. Worth knowing that Deno's own
node:netpolyfillaccepts them, so
isIPv6()andnode:net'sisIP()now disagree onfe80::1%eth0.isIPv4()andisIPv6()becomeparseX() !== undefined. The matchers parseboth sides into bytes and compare the leading prefix bits, so the private
expandIPv6()andipv6ToBytes()are gone.This is the strict grammar from the open question at the bottom of #7315. If the
answer comes back "permissive",
parseIPv4()is the only thing that changes.One more tightening, in subnet prefixes
Prefix lengths used to go through
parseInt(s, 10), so/0x18parsed as0and matched every address. An allowlist built on that let everything through.
They are now validated as decimal digits in range, which also rejects
/+24,/ 24,/24abcand/24/8, all of which used to quietly mean 24, and/1e1,which meant 1.
Zero-padded lengths still work.
/024has only one reading, unlike an octet, sothere was no reason to break configs that spell it that way.
Semver
fix, notBREAKING. The module is unstable, and the behavior changes go inboth directions, neither of them breaking.
Almost everything that moves is an input that was invalid all along and is now
rejected. Calling that out since it is a wide change: anything relying on
isIPv4(" 1.2.3.4")beingtruewill notice.Two changes go the other way, and both are false negatives fixed:
1:2:3:4:5:6:7::and::1:2:3:4:5:6:7are valid, and the oldisIPv6()rejected them. A leading or trailing
::makessplit(":")return 9 elements,so the expansion loop never ran and the length check failed. I enumerated every
::position over 0 to 9 groups, with and without an IPv4 tail: those two arethe only valid addresses the old implementation turned away.
Validation
env -u NO_COLOR deno task okon Deno 2.9.6: 7,450 tests pass. One unrelatedfailure,
writeTextFile() handles an AbortSignalinfs/, which is a temp-dircleanup race under
--paralleland passes three times out of three on its own.The
netsuite is 25 tests including doc examples, green every run.Both parsers were differential-tested against the runtime's own WHATWG URL
parser, which implements the same two algorithms:
new URL("http://[" + s + "]/"): nodisagreement on validity. For the 2,716 that parse, I serialized the bytes
back through a WHATWG IPv6 serializer and compared against
URL.hostname.Byte-exact on all of them.
URL.hostnameround-trip:no disagreement. The corpus has to be restricted that way, because
0.0.0.around-trips as an opaque domain rather than an address, and an unrestricted
oracle reports that as a parser bug.
136 (IPv6) with up to three leading zeros, 3,560 comparisons against the base
commit: no behavior change.
Tests cover the tables above, the two fixed false negatives, plus
1:2:3:4:5:6:7::8,1:2:3:4:5:6:7:8:9,::ffff:1.2.3, uppercase hex, bothmapped forms, and bit boundaries either side of
/25,/33and/121. Everyrow was checked against the base commit, so none of them pass for the wrong
reason.
I used Claude Code to help investigate and write this change.