Repository navigation
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings (both pre-existing, not introduced by this diff), I also checked the one behavior change on the refusal path: a special-scheme host that IDNA rejects now leaves an invalid URL with the raw spliced string instead of an untouched one. The Bun-side callers handle it — URLDecomposition::setHostname/setHost only commit when fullURL.isValid(), and URLPatternCanonical checks setHost's return value — so no caller ends up holding the raw text.
Extended reasoning...
The diff is a pure deletion in Source/WTF/wtf/URL.cpp (appendEncodedHostname and its uidna pre-pass removed from setHost and setHostAndPort, raw host now spliced and handed to parse()) plus new table-driven gtest rows in Tools/TestWebKitAPI/Tests/WTF/URL.cpp that CI does not run. It touches URL host parsing, a security-relevant surface (host/authority confusion), and the two confirmed inline findings are pre-existing gaps in the same functions rather than regressions from this change. The refusal-path behavior change was traced to its WebCore callers and is guarded by isValid()/return-value checks there.
|
Preview build of 151f5ec: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughHost setters now pass host text directly to URL parsing instead of applying local IDNA conversion. Tests cover Unicode and percent-encoded input, whitespace, delimiters, IPv6, opaque hosts, and port handling. ChangesURL host setting
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue is established for the host-setter change; merge after normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
oven-sh/WebKit#724 deletes the IDNA pass that WTF::URL::setHost and setHostAndPort ran on the raw value. The preview pin is for CI only. It becomes the merged oven-sh/WebKit commit before this PR leaves draft.
Both setters ran uidna_nameToASCII on the raw value (appendEncodedHostname),
spliced the result into the URL string and parsed that string. IDNA maps some
code points to text with an ASCII delimiter: the fullwidth forms of
/ \ ? # @ : %, U+2100 (a/c), U+2048 (?!) and others, 17 in the BMP. Mapped
before the splice, that text was parsed as URL structure:
https://base.test/p setHost("bad.c℀.good.com") -> https://bad.ca/c.good.com/p
https://base.test/p setHost("x＠evil.example") -> https://x@evil.example/p
https://base.test/p setHost("a：81") -> https://a:81/p
https://base.test/p setHost("[::１]") -> https://[::1]/p
The '@' and ':' checks of 270425, 289609 and 289789 run on the raw value, before
the mapping, so these values passed them. The first value is the one Node.js
refuses in url.parse() since nodejs/node#38631. A URL string with the same host
text does not parse.
The pass also ran before the parser removed tab and newline and before it
percent-decoded: setHost("ß%41.de") gave xn--a-4ka.de where the URL string gives
xn--a-pfa.de, and setHost("\tß.de") failed where the URL string parses.
Delete appendEncodedHostname. The setters splice the raw host text, after the
existing delimiter cut and the '@' and ':' checks, and
URLParser::parseHostAndPort does what it does for a URL string: one IDNA pass
after percent-decoding, then the forbidden host code point check. A host that
is not special already took this path.
The URL string also stays 8-bit. The helper always produced a 16-bit buffer, so
the string was 16-bit after any host set on a special URL.
Not changed: the cut and the checks still look at the raw value. A value that
is empty after the cut, or only tab and newline, on a special URL still makes
the parser read the host from the path, as before.
cd049ba to
151f5ec
Compare
…the constructor Rows for oven-sh/WebKit#724, which deletes the IDNA pass that WTF::URL::setHost and setHostAndPort ran on the raw value before the URL parser saw it. - url.test.ts: 33 values through hostname=, host=, host= with a port and the constructor: non-ASCII with %XX, with tab or newline, the code points that IDNA maps to a delimiter (U+2100, the fullwidth forms), IPv6 literals with a fullwidth digit. Five special schemes. href stays an 8-bit string. - urlpattern.test.ts: 19 hostnames through the pattern constructor and exec() input, and a dictionary input that maps to text with '@'. - pathToFileURL.test.ts: a UNC host with a tab or a %XX escape. Every expected value is what Node.js v26.10.0 gives. 45 of these tests fail on WebKit 299c5323. Some rows come from #33757.
oven-sh/WebKit#724 deletes the IDNA pass that WTF::URL::setHost and setHostAndPort ran on the raw value. The preview pin is for CI only. It becomes the merged oven-sh/WebKit commit before this PR leaves draft.
Problem
https://base.test/p,setHost("bad.c℀.good.com")(U+2100) giveshttps://bad.ca/c.good.com/p. In Bun 1.4.3 this isurl.hostname=,url.host=and URLPattern hostnames. Node.js refuses the value.appendEncodedHostname(Source/WTF/wtf/URL.cpp:496). It runsuidna_nameToASCIIon the raw value, and the result is spliced into the URL string and parsed as URL structure. IDNA maps 17 BMP code points to text with a delimiter."ß%41.de"givesxn--a-4ka.de, a URL string givesxn--a-pfa.de."\tß.de"fails.Fix
appendEncodedHostname. Both setters splice the raw host text.URLParser::parseHostAndPortthen runs IDNA once, after percent-decoding, and rejects a forbidden host code point, as for a URL string.WTF_URL.URLSetHostandURLSetHostAndPort(25 values, 8-bit and 16-bit) fail with the stockf20ce774libWTF.aand pass with thisURL.cpp.WTF_URLParserpasses.Background
URLsetter splices the new component into the URL string and parses the whole string again.url.parse()(url: forbid certain confusable changes from being introduced by toASCII nodejs/node#38631).URLDecomposition(url: preprocess host/hostname setter input so IDNA matches the constructor bun#33757): it keeps the pre-pass, and URLPattern callsURL::setHostdirectly.Downsides
URL::setHost. ASCII values cost 10% to 17% less. Table in Notes."[::１]"(U+FF11) set[::1].Notes
Lineage. This completes oven-sh/bun#38013 and WebKit#39273, which removed the other IDNA pre-pass, in front of the constructor. The setters were outside that differential. Upstream added the
@and:checks insetHost/setHostAndPortin 270425 ("URL::setHostAndPort() needs to be aware of host delimiters"), 289609 and 289789. They look at the raw value, so the fullwidth forms pass them and become@and:in the pre-pass.bad.c℀.good.comis the value from HackerOne 678487 that nodejs/node#38631 (70157b9cb7d7, v17.0.0) refuses inurl.parse(). Bun's legacyurl.parse()has that guard (src/js/node/url.ts:392). CPython had the same class as CVE-2019-9636. Upstream WebKit main still hasappendEncodedHostname, so this deletion applies there too. It is worth a bugs.webkit.org report by someone with an account.Reach. Plain IDN hosts and ASCII hosts give the same result before and after. Only three kinds of value change: non-ASCII with a
%XXescape, non-ASCII with a tab or newline, and the code points that IDNA maps to a delimiter. The value must reach a setter as decoded text. A percent-encoded query parameter does (searchParams.get("h")with?h=bad.c%E2%84%80.good.comgiveshttps://bad.ca/c.good.com/pon 1.4.3). A raw header value does not: header bytes decode as Latin-1, and the 17 code points are all above U+00FF.The 17 code points. A sweep of every non-ASCII BMP code point
cassetHost("a" + c + "b.example")finds 17 where the setter and the parser disagree: U+2047, U+2048, U+2049, U+2100, U+2101, U+2105, U+2106, U+FE16, U+FE56, U+FE5F, U+FE68, U+FE6B, U+FF03, U+FF0F, U+FF1F, U+FF20, U+FF3C. With this change they agree on every row.Step 2. The delimiter cut and the
@/:/[checks still look at the raw value, before the parser removes tab and newline. SosetHost("\t[::1]")is refused, and a value that is empty after the cut, or only tab and newline, leaveshttps:///p, which parsespas the host:setHost("/"),"#","?","\\"or"\t"onhttps://h/evil.example/pgiveshttps://evil.example/p. This is the same before and after this PR. These host-state rules belong inURL::setHostandsetHostAndPorttoo, for the reason given above against oven-sh/bun#33757: URLPattern does not go through bun's wrapper. Today oven-sh/bun#39458 (tab and newline) and oven-sh/bun#32826 ([::1]:80throughhostname=, default port, oven-sh/bun#28661) handle parts of it inURLDecomposition. This PR stays a deletion so that it is quick to review. It is the first step of any such change: a host-state parse also wants exactly one IDNA pass, inside the parser.Unblocks. The bun draft oven-sh/bun#43872 drops bun's
xn--reject layer (hasAcceptableHost). Without that layeru.hostname = "é.%78n--a"setsxn--9ca.xn--atoday, because the pre-pass convertséand the parser then sees only ASCII. With this change the parser percent-decodes first, seesé.xn--a, ToASCII fails, and the set is refused.#643 changes the same lines (
tryMakeString, no copy of an ASCII host into theVector). After this PR only itsmakeStringtotryMakeStringedit remains for these two functions. The second one to land needs a small rebase.bun side. oven-sh/bun#43883 (draft) pins the preview build of this PR and adds the tests: 45 fail on
f20ce774, all 499 in the three files pass on the preview build (bun bd test, debug ASAN). bun's CI passed on every platform with the preview builds ofcd049ba3and151f5eca(Buildkite builds 120269 and 121226). Re-checked on0c06faad, after the upstream merge todbdca7545d: this diff applies unchanged, the two new tests fail with the stocklibWTF.a, and all 32 pass with thisURL.cpp. The whole generated table of setter, URLPattern and constructor results is identical to Node.js v26.10.0 on that build.Measurements. bun main
6d504dd98release build (ThinLTO), relinked twice from the same link line with one object in front oflibWTF.a:URL.cppat299c5323andURL.cppwith this change.URL.cppis the same file atf20ce774. Instructions are single-step counts on the main thread from the entry of the function to its return, JIT off, steady state (the calls give the same count). Allocator calls are hits onmi_malloc.WTF::URL::setHostother.examplebücher.exampleאבג.exampleother.examplebücher.exampleאבג.exampleother.examplebücher.exampleאבג.exampleThe extra allocations are the parser's: it sees a non-ASCII host, builds the result in its ASCII buffer (256 bytes inline) and copies it out. Before, the pre-pass handed it canonical text and it kept its input string. The URL string was 16-bit after any host set on a special URL and is 8-bit now, so later setters parse 8-bit text: bun's
pathname=after onehostname=goes from 5,231 to 4,747 instructions. Binary: text 80,684,744 to 80,681,159 bytes (-3,585).WPT.
url/resources/setters_tests.json(c48d58747e) has no row for this class: bun passes 274 of 278 before and after, and the same 4 rows fail.