Repository navigation
Conversation
|
Preview build of c061b10: |
… parse
parseIPv6Host records a syntax violation when the address ends right after a
piece and a ':'. With a "::" compression earlier in the address it then
returns the address, and the serializer drops the colon:
http://[::1:]/ -> http://[::1]/
http://[1::2:]:81/ -> http://[1::2]:81/
Without a compression the piece count check rejects the address, so only the
compressed form parses.
This reverses an upstream decision. r247267 (57936a0,
https://bugs.webkit.org/show_bug.cgi?id=199627, "URLParser should reserialize
IPv6 addresses that end with a colon") added the syntaxViolation() call and
the WTF_URLParser.Basic row that expects "a://[::2:]" to parse as "a://[::2]".
Its reason: "When an IPv6 address ends in a colon, the colon should be removed
[...] This matches the URL specification, and I added a unit test that used to
assert." Earlier, r207162 (128a4c7, bug 162660) changed the line of
fast/url/ipv6-expected.txt for that address from PASS to FAIL, with the note:
'"http://[0:0::0:0:8:]/" should indeed be compressed to "http://[::8]/"'.
The URL Standard does not say that. Its IPv6 parser, after a piece and a ':',
increases the pointer, and "If c is the EOF code point, [validation error],
return failure". The text had that step when both commits were made
(whatwg/url 85359fc of 2016-09-02 and 8809598 of 2019-04-04), and it has it
now (845bafc). Chromium, rust-url and Node.js fail on such an address.
Return std::nullopt at that step. The address fails before anything is
serialized, so the assertion that r247267 fixed has nothing to check, and the
rows below pass in a build with assertions. URL::setHost and
URL::setHostAndPort parse the new URL string with the same function, so they
refuse the value. A "::" at the end of the address is a compression and still
parses.
Tests: the "a://[::2:]" row of WTF_URLParser.Basic expects failure, and
ParserFailures has three more rows. fast/url/ipv6-expected.txt has PASS again
for "http://[0:0::0:0:8:]/", which fast/url/ipv6.html lists under "Don't allow
leading or trailing colons". JSTests/BUN-TEST-DIFFERENCES.md lists the
URLParser.cpp rows of this commit and of the previous one.
b01ec55 to
c061b10
Compare
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
IPv6 parsing rules and regression coverage Tools/TestWebKitAPI/Tests/WTF/URLParser.cpp, Source/WTF/wtf/URLParser.cpp, LayoutTests/fast/url/ipv6-expected.txt, JSTests/BUN-TEST-DIFFERENCES.md |
The parser rejects IPv6 addresses that end in a single colon and processes compressed addresses with IPv4 tails. Tests cover the updated parsing cases, and the layout-test expectation and test-difference documentation are updated. |
Merge Risk: 
c061b
-
The IPv6 parsing updates and corresponding expectations are consistent and ready to merge.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: updating IPv6 host parsing to follow the URL Standard. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The description is detailed and directly explains the problem, fix, affected behavior, testing, performance impact, and related references. It does not follow the exact commit-message template because… Full details: Description check
-
Explanation
The description is detailed and directly explains the problem, fix, affected behavior, testing, performance impact, and related references. It does not follow the exact commit-message template because it lacks a dedicated Bugzilla link for this change, a Reviewed by line, and an explicit changed-file/function list. These omissions do not prevent review because the required technical context is present.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it intentionally changes which bracketed IPv6 hosts parse (reaching new URL, fetch, Request, WebSocket and URLPattern) and the paths are under WTF code ownership, a human look would still be worthwhile.
What was reviewed:
- Traced
parseIPv6Hostwith the piece-5 guard removed:[2001:db8:1234::5efe:127.0.0.1]writesaddress[5]/address[6](no out-of-bounds), and the compression swap yields2001:db8:1234::5efe:7f00:1as the newParserDifferencesrows expect;[1:2:3:4:5::5efe:127.0.0.1]still fails via the hex path. - Checked the trailing-colon change against
[1::],[::]and[::1:]: a final::still parses, only a piece followed by a lone:at end returnsstd::nullopt, matching the URL Standard's "c is EOF, return failure" step. - The
BUN-TEST-DIFFERENCES.mdrow, theipv6-expected.txtflip to PASS, and the modifiedBasic/ParserFailuresrows are consistent with the new behavior.
Extended reasoning...
The change touches Source/WTF/wtf/URLParser.cpp (three lines in parseIPv6Host: removes the compressed-address IPv4-tail-at-piece-5 rejection and makes a trailing single ':' fail instead of being normalized away), the matching TestWebKitAPI rows, a LayoutTests expectation, and a documentation row in JSTests/BUN-TEST-DIFFERENCES.md. It alters host parsing for every URL entry point, which is a security-adjacent surface (host-based allowlists, fetch destinations), though both changes move toward the URL Standard and Node.js and neither introduces memory-unsafety. No findings were reported and no third-party objections appear in the timeline, but the paths are covered by the repository's CODEOWNERS (/Source/WTF, /Tools/TestWebKitAPI/Tests/WTF, /JSTests) and the change is a deliberate behavioral divergence from upstream WebKit, so a human should weigh the intent rather than this being auto-approved.
Problem
new URL("http://[::1:]/")giveshttp://[::1]/, andfetch("http://[::1:]:3000/")connects to[::1]. Node.js, Chromium and rust-url refuse the URL.new URL("http://[2001:db8:1234::5efe:127.0.0.1]/")throws. Node.js parses it (WTF::URLParser should accept compressed IPv4-in-IPv6 addresses #648).URLParser::parseIPv6Host(Source/WTF/wtf/URLParser.cpp:3293,:3332). Upstream added the first on purpose (r247267, bug 199627), misreading the URL Standard.Fix
:fails. A final::still parses.WTF_URLParserandWTF_URL(30 tests),linux-amd64-debug-asanlane flags. Each changed test fails without its commit. 334,619 bracketed hosts: 488 differ from Node.js v26.3.0 before, 0 after..failingBun tests now.Background
::(a compression) is a run of zero pieces.syntaxViolationmarks valid input that needs normalizing.std::nulloptmakes the URL invalid.z.ipv6()uses this parser.URL::setHostand URLPattern all end here. These are the first intended result changes since Make WTF::URLParser faster than Ada #452.Downsides
:no longer parses.new URLthrows,fetchrejects and a setter keeps the old host, as in Node.js.URLParser.cpp.oshrinks by 99 bytes. Valid IPv6 URLs run 26 fewer to 5 more instructions (Notes).Notes
The Bun side. oven-sh/bun#47236 (draft) pins the preview build of this branch and adds the tests.
bun bd teston its three test files: 5 failures on Bun main's pinfcc1ea09, 574 passes with the preview build (debug ASAN).test/js/web/url,test/js/web/urlpatternandtest/js/node/urlgive 765 passes and no failure, and the 42test-url*andtest-whatwg-url*files of the Node.js suite keep their exit codes. This branch is onfcc1ea09, which Bun main pinned when that pull request was opened, so the preview differs from that pin by these two commits only. Bun main's pin has moved since. Both branches are rebased when this pull request gets a review or merges.Repro.
http://[::1:]/http://[::1]/http://[1::2:]:81/http://[1::2]:81/http://[1:2:3:4:5:6:7:]/http://[1::]/http://[1::]/http://[1::]/http://[2001:db8:1234::5efe:127.0.0.1]/http://[2001:db8:1234::5efe:7f00:1]/A trailing colon was accepted only with a
::in the address. Without one, the laterpiecePointer != 8check rejects the address.Reach. Every URL entry point of Bun ends in this function:
new URL,URL.parse,URL.canParse, thehref,hostandhostnamesetters,Request,WebSocket,fetch, URLPattern inputs,url.domainToASCIIandurl.domainToUnicode. Bun 1.4.3:fetch("http://[::1:]:PORT/x")gets a 200 from a server on::1, withHost: [::1]:PORT, whilenet.isIPv6("::1:")is false. zod v4 checksz.ipv6()andz.cidrv6()withURL.canParse("http://[" + value + "]")(packages/zod/src/v4/core/schemas.ts,isValidIPv6), so on Bun it accepts::1:and on Node.js it does not. No issue in oven-sh/bun or here reports either bug.The upstream history of commit 2. r247267 (
57936a0555eb, 2019, bug 199627, "URLParser should reserialize IPv6 addresses that end with a colon") added thesyntaxViolationcall and theWTF_URLParser.Basicrow that expectsa://[::2:]to parse asa://[::2]. Its reason: "the colon should be removed [...] This matches the URL specification, and I added a unit test that used to assert." Earlier, r207162 (128a4c7e0e, 2016, bug 162660) changed the line ofLayoutTests/fast/url/ipv6-expected.txtforhttp://[0:0::0:0:8:]/fromPASStoFAIL, with the note that the address "should indeed be compressed".The URL Standard has said otherwise at each of those dates. After a piece and a
:its IPv6 parser increases the pointer, then: "If c is the EOF code point, syntax violation, return failure" (whatwg/url85359fc, 2016-09-02), "validation error, return failure" (8809598, 2019-04-04), "IPv6-invalid-code-point validation error, return failure" (845bafc, today). Its table of validation errors giveshttps://[1:2:3:]as an example. Chromiumurl/url_canon_ip.h(DoParseIPv6) allows an empty component at the end only directly after the contraction. rust-urlurl/src/host.rs(parse_ipv6addr) returnsInvalidIpv6Addresswhen the input ends after the:.The address now fails before anything is serialized, so the assertion that r247267 fixed has nothing to check. The new rows pass in a build with assertions.
Commit 1 is #648. The cherry-pick is clean and keeps the author. #648 is a pull request from a fork, so this repository's CI never built it. If you prefer to land #648 by itself, drop commit 1: commit 2 does not depend on it. Only one comment and the row in
JSTests/BUN-TEST-DIFFERENCES.mdname its test rows. A merge commit keeps the two commits apart. A squash keeps the author of commit 1 as a co-author.Tests.
WTF_URLParser.Basicnow expectsa://[::2:]to fail (changed in place, with a// Bun:comment).ParserFailureshas three more rows andParserDifferenceshas the four rows of #648.LayoutTests/fast/url/ipv6-expected.txthasPASSagain forhttp://[0:0::0:0:8:]/, whichipv6.htmllists under "Don't allow leading or trailing colons". That layout test is not part of this repository's CI and was not run.The prebuilt lane ships its
compile_commands.json.URLParser.cpp,Tests/WTF/URLParser.cpp,Tests/WTF/URL.cppand gtest were compiled with the lane's own commands and linked to thelibWTF.aofautobuild-680ef47c:libWTF.aBasic,ParserDifferencesandParserFailuresfailBasicandParserFailuresfailParserDifferencesfailsHow much differs from Node.js. 334,619 strings between
http://[and]/: every string of length 0 to 8 over0 1 : ., 300,000 random token strings, and a structured set. Each one goes through Bun and Node.js v26.3.0. Stock: 488 differences (478 trailing colons, 10 IPv4 tails). Commit 2 only: 10. This branch: 0, also with the preview build of this pull request.Cost.
URLParser.cppcompiled with the flags of thelinux-amd64release lane (-O3). The text of the object goes from 206,518 to 206,419 bytes. Instructions are single-step counts in gdb from the entry ofURL::URL(String&&)to its return, equal on each of three calls.http://[::1]/http://[2001:db8::1]:8080/http://[1:2:3:4:5:6:7:8]/http://[fe80::1ff:fe23:4567:890a]/pathhttp://[::ffff:192.168.0.1]/http://[1::]/http://example.com/Since #452. #452 rewrote this function with output identical to the old parser. These two commits change results on purpose. A differential against the old parser now differs for a compressed address that ends with a piece and a
:(parsed before, fails now) and for a compressed address with an IPv4 tail at piece 5 (failed before, parses now).Upstream. WebKit main (
762eb8cd71d2) has both lines, thea://[::2:]row and theFAILline. WPT has no row for either case: its only literal that ends with a piece and a:ishttps://[0:0:], which has too few pieces anyway. A bugs.webkit.org bug that follows up 199627, and WPT rows such ashttps://[::1:]andhttps://[1::2:](failure) andhttps://[::1:2:3:4:1.2.3.4]([::1:2:3:4:102:304]), need accounts that I do not have. Commit 2 applies upstream with only the iterator spelling changed.Self-review, not taken. Three concerns ask for an upstream WebKit bug, an upstream pull request and a WPT pull request before or beside this one. They need accounts that I do not have. The Upstream note has what they would contain. One concern asks to land the Bun tests now, marked
.failing. The pin bump that brings the fix then goes red for whoever makes it. The tests land with the pin instead.Related. #724 (with oven-sh/bun#43883) changes
URL::setHostand does not touch this function. The Bun tests of oven-sh/bun#47236 sit at other places in the same test files.