url: preprocess host/hostname setter input so IDNA matches the constructor#33757
url: preprocess host/hostname setter input so IDNA matches the constructor#33757robobun wants to merge 3 commits into
Conversation
The URL host/hostname setter must run the basic URL parser with state override, which strips ASCII tab/newline and percent-decodes before domain-to-ASCII. WTF::URL::setHost instead runs uidna_nameToASCII on the raw value whenever it contains non-ASCII, so a tab or a percent-escape is baked into the Punycode A-label and the setter disagrees with the constructor on the same input. Strip ASCII tab/newline and UTF-8 percent-encode non-ASCII in URLDecomposition before calling WTF::URL::setHost so it takes its ASCII passthrough and the full URLParser runs on reparse.
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 8:05 AM PT - Jul 8th, 2026
❌ @robobun, your commit 4e87493 has some failures in 🧪 To try this PR locally: bunx bun-pr 33757That installs a local version of the PR into your bun-33757 --bun |
There was a problem hiding this comment.
I didn't find bugs, but this is a non-trivial workaround for WebKit's appendEncodedHostname that relies on forcing its ASCII-passthrough path, touches security-adjacent hostname parsing, and textually conflicts with three other open PRs on the same file — worth a human deciding on approach and merge order.
Extended reasoning...
Overview
Adds preprocessHostSetterValue() in src/jsc/bindings/URLDecomposition.cpp and calls it from both setHost and setHostname. The helper strips ASCII tab/LF/CR and UTF-8 percent-encodes any non-ASCII code points, so the value handed to WTF::URL::setHost is always pure ASCII. That forces WebKit's appendEncodedHostname onto its ASCII passthrough branch, after which the full URLParser reparse performs the spec's percent-decode + domain-to-ASCII correctly. 11 new test cases in test/js/web/url/url.test.ts assert setter output matches the constructor and Node.
Security risks
URL hostname parsing is security-adjacent — the PR description itself frames the bug as a filter-bypass primitive (validate new URL(x) then assign u.hostname = x and get a different host). The fix narrows that gap, which is good, but changes to host canonicalization deserve human eyes on edge cases (IPv6 literals, opaque hosts, non-special schemes, interaction with the port-splitting logic in setHost which now runs on the preprocessed string).
Level of scrutiny
Medium-high. The mechanism is a deliberate workaround coupled to internal WebKit behavior ("all-ASCII input takes the passthrough and reparses") rather than a direct fix, because the buggy code lives in the prebuilt WebKit. That's a reasonable pragmatic choice, but it's exactly the kind of design decision ("patch here vs. patch the fork") a maintainer should sign off on. The String preprocessed lifetime keeping the reassigned StringView value valid is correct but subtle.
Other factors
- The PR body explicitly calls out textual conflicts with #32826, #33195, and #33206 on the same file; a human should decide merge order.
U8_APPEND_UNSAFEis justified by the IDLUSVString comment, which is correct for the WHATWGURLinterface, but I did not exhaustively verify every caller ofURLDecomposition::setHostguarantees USVString conversion.- Test coverage is good (tab/LF/CR × non-ASCII, percent-escapes, astral plane, port, empty-after-strip) and asserts against both the constructor and hard-coded Node-verified values.
|
On the |
|
CI status: the diff itself is green. 284 of 287 jobs passed on build 70498 with no url-related failures; the only red is |
The URL
host/hostnamesetter must run the basic URL parser with state override (https://url.spec.whatwg.org/#dom-url-host), which strips ASCII tab/newline from the input and then has the host parser UTF-8 percent-decode before domain-to-ASCII.WTF::URL::setHostinstead hands any non-ASCII value straight touidna_nameToASCII(viaappendEncodedHostnameinWTF/wtf/URL.cpp), so the tab/newline or the literal%xxis baked into the Punycode A-label and the setter disagrees with the constructor on the same input.The first two inputs mint hostnames whose A-label decodes to an embedded control character, which no spec-conforming host parser can produce (the forbidden-host-code-point check runs on the already-encoded label and cannot see it), so a filter that validates
new URL(x)and then assignsu.hostname = xvalidates a different host than it uses.Fix
appendEncodedHostnameonly diverges from the spec on non-ASCII input: for all-ASCII it appends the value as-is and the fullURLParserthen does the right thing on reparse.URLDecomposition::setHost/setHostnamenow strip ASCII tab/newline and UTF-8 percent-encode any non-ASCII code points before callingWTF::URL::setHost, which forces that ASCII passthrough and letsURLParser::parseHostAndPortdo the spec's percent-decode + domain-to-ASCII exactly as the constructor does. For all-ASCII values with no tab/newline (the common case) the preprocessor is a no-op.The upstream
appendEncodedHostnameitself lives in the prebuilt WebKit, so it is not touched here; the WHATWG setter semantics are owned byURLDecompositionin this repo.Verification
11 new cases in
test/js/web/url/url.test.tsassert setter == constructor for tab/LF/CR + non-ASCII,%xx+ non-ASCII, mixed, surrogate-pair code points, thehostsetter with a port, and a pure tab/newline value. Every expected value matches Node 26. 10 of the 11 fail on the released Bun (the one pass is the pure-ASCII control case); all 25 tests in the file pass with this change.test/js/node/url/url.test.tsandtest/js/web/urlpattern/urlpattern.test.ts(414 tests) pass with the fix.Relationship to open
URLDecompositionPRs#32826 (host/port split and origin), #33195 (empty-host guard), and #33206 (
domainToASCII) touch the same file for adjacent bugs but none of them fixes this one: with each of those applied alone,u.hostname = "\tß.de"still yields"xn---qfa.de". #33206's PR body notes this exact mechanism ("setHostapplies IDNA to its raw argument before the parse removes them") but only works around it insidedomainToASCII. These changes conflict textually onURLDecomposition.cppandurl.test.ts; whichever lands second gets a small rebase.