url: accept Unicode 16.0 IDNA-mapped hostnames and reject invalid Punycode labels - #34731
url: accept Unicode 16.0 IDNA-mapped hostnames and reject invalid Punycode labels#34731robobun wants to merge 9 commits into
Conversation
…Unicode 16.0 The bundled ICU prebuilts (75.1 on Linux/musl, 73.2 on Windows) derive their uts46.nrm from Unicode 15.x's IdnaMappingTable.txt, which marks a family of late-casefolded capitals as 'disallowed': U+04C0 CYRILLIC LETTER PALOCHKA, U+10A0..10C5 Georgian capitals, U+2132 TURNED CAPITAL F, U+2183 ROMAN NUMERAL REVERSED ONE HUNDRED, plus several format controls. Unicode 16.0's IdnaMappingTable.txt changed these to 'mapped'/'ignored', so WHATWG URL now expects them to parse. Ship a uts46.nrm regenerated from Unicode 16.0's data in Nrm2 format version 4 (readable by ICU 73/75) and swap it in via the existing ICU udata hook (bun_icu_maybe_decompress), matched by its exact 48-byte header prefix. The override is a no-op on macOS (system ICU) and self-disables once the prebuilts bundle ICU 76 or later.
|
Warning Review limit reached
Next review available in: 3 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 (1)
WalkthroughChangesThe PR adds a reproducible ICU UTS UTS
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:05 AM PT - Jul 28th, 2026
❌ @robobun, your commit 1d27a31 has some failures in 🧪 To try this PR locally: bunx bun-pr 34731That installs a local version of the PR into your bun-34731 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find bugs, but this warrants a human look: it ships a checked-in binary blob (icu_uts46_override.nrm) and hooks the ICU udata load path to hot-swap normalization data affecting hostname parsing. The runtime-override-vs-bump-the-prebuilt tradeoff and accepting an opaque binary in-tree are maintainer calls.
What was reviewed
maybeOverrideUTS46: length-guarded 48-bytememcmpbefore deref, falls through on mismatch,*lengthonly mutated on the override branch — non-matching items unaffected.- Hook ordering: override runs only on the non-zstd branch, so decompressed items and the existing cache path are untouched.
- Tests: platform-gated to Linux/Windows (matching the
#if OS(...)guard), cover both newly-mapped and newly-ignored codepoints plus a regression case for pre-existing mappings.
Extended reasoning...
Overview
This PR fixes IDNA hostname parsing for codepoints that Unicode 16.0 reclassified from disallowed to mapped/ignored in UTS #46. The bundled ICU prebuilts (75.1 Linux, 73.2 Windows) carry pre-16.0 uts46.nrm data, so uidna_nameToASCII rejects hostnames that Node.js (Ada) and WPT accept. The fix regenerates uts46.nrm from Unicode 16.0's IdnaMappingTable using ICU 75's gennorm2 (so the output stays at Nrm2 format v4), checks the ~60KB blob into the repo, embeds it via #embed, and swaps it in at runtime through the existing bun_icu_maybe_decompress udata hook by matching the stock item's exact 48-byte header prefix.
Security risks
URL/hostname parsing is security-relevant — the set of codepoints that are mapped vs. ignored vs. disallowed in IDNA directly shapes what hostnames a URL parser produces, which flows into DNS resolution, origin checks, and TLS SNI. The change widens acceptance (previously-rejected inputs now parse), aligning with Node/WPT rather than diverging, so the compat direction is right. But the actual mapping data is an opaque binary that reviewers can't inspect line-by-line; correctness rests on trusting the regeneration script and the upstream ICU 76 uts46.txt. No injection or memory-safety concern in the C++ itself — the memcmp is bounds-checked and the embedded array is static constexpr in rodata.
Level of scrutiny
High. This is not a mechanical fix: it introduces a new mechanism (runtime data-item substitution keyed on a byte-prefix fingerprint), adds a binary artifact to source control, and encodes an architectural choice — patch the data at runtime rather than bump the oven-sh/WebKit ICU prebuilt to 76+. That tradeoff (faster ship, self-disabling once the prebuilt catches up, but an extra moving part and an unreviewable blob) is a maintainer decision. The 48-byte-prefix uniqueness claim ("unique among the *.nrm in the data package") is asserted in the PR but not something I can verify from the diff.
Other factors
The implementation is careful: static_assert on the embedded blob's format bytes, the regeneration script verifies format-version 4, the override is confined to the two known prefixes and falls through otherwise, and tests include a "still maps pre-existing cases" guard against regressing unrelated IDNA behavior. macOS is intentionally excluded (system ICU, no udata hook). The PR description notes local verification passed and adjacent Intl/URL suites are unchanged, but the robobun evidence block reads "no test proof" (platform-specific), so CI is the real gate. Given the binary artifact, the security-adjacent surface, and the architectural call embedded here, deferring to a human reviewer.
|
On the points the bot review left for a human: 48-byte prefix uniqueness. The first 32 bytes (ICU
Binary reproducibility. Runtime override vs. prebuilt bump. Agreed that bumping the oven-sh/WebKit prebuilt to ICU 76 is the durable fix; that's a separate two-repo change (Dockerfile edits, rebuild all platform tarballs, bump |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/regenerate-uts46-override.sh`:
- Around line 19-20: Update the path setup in regenerate-uts46-override.sh to
derive an absolute repository root once, then construct OUT from that root
instead of relying on the preceding cd and a relative path.
- Around line 4-12: Condense the history comment above the script to no more
than three lines, retaining only the durable contract that it generates a
Unicode 16.0 UTS46 override in ICU-compatible Nrm2 format for runtime use.
Remove version-specific background, affected code-point examples, and the
temporary deletion note.
- Around line 15-17: Update the download flow in
scripts/regenerate-uts46-override.sh to fetch the ICU toolchain archive and ICU
data input into temporary files, verify each against pinned SHA-256 digests or
signatures, and only then extract or compile them. Keep the existing
ICU_TOOLCHAIN_TAG, ICU_TOOLCHAIN_SRC, and ICU_DATA_TAG selections while ensuring
no unverified content is streamed directly into tar.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 62e811a2-60d2-4117-a349-adf08d7487f4
📒 Files selected for processing (4)
scripts/regenerate-uts46-override.shsrc/jsc/bindings/bun_icu_decompress.cppsrc/jsc/bindings/icu_uts46_override.nrmtest/js/web/url/url.test.ts
|
CI on 7499cbc (build 75881): 283/286 passed. Remaining reds are unrelated to this diff:
All review threads resolved. Ready for a maintainer to take a look. |
WTF::URLParser takes an all-ASCII fast path for special-scheme hosts that never runs UTS-46 "domain to ASCII", so an "xn--" label that does not decode to a valid IDNA label (for example "xn--a-ecp") parses successfully. Node and the URL Standard's host parser reject it. Add Bun::urlHostIsValidIDNA, which re-runs ICU's uidna_nameToASCII on a parsed special-scheme host, but only when one of its labels starts with "xn--"; for every other all-ASCII host the fast path is already equivalent to the spec. Wire it into DOMURL's create, parse, canParse, and setHref, which also covers the href, host, and hostname setters. url.domainToASCII had the same all-ASCII early return; gate it on the same predicate and route ACE labels through the shared helper.
… host Bun::domainToASCII used a fixed 2048-code-unit buffer and treated U_BUFFER_OVERFLOW_ERROR as "invalid IDNA", so an all-ASCII host longer than that which happens to contain an "xn--" label was newly rejected. Retry once with a heap buffer of the length ICU reports, following ICU's preflight convention. The URL Standard puts no length limit on a domain, and Node accepts these. URLDecomposition::origin() re-parses a blob: URL's path with a raw WTF::URL and only checked isValid(), so an invalid "xn--" label in the inner host still produced a tuple origin. Apply the same host check so the inner parse fails and the origin serializes to "null", as Node does.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
robobun
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/bindings/BunIDNA.cpp`:
- Line 34: In the domain conversion flow containing uidna_nameToASCII, validate
domain.length() before converting it to the ICU int32_t parameter. Reject and
return early when the length exceeds INT32_MAX, then pass the checked int32_t
value to uidna_nameToASCII instead of an unchecked cast.
In `@test/js/web/url/url.test.ts`:
- Around line 302-310: Update URLDecomposition::setHost() and setHostname() so
they commit mutations only when fullURL.isValid() and
Bun::urlHostIsValidIDNA(fullURL) both pass. Preserve the existing no-op behavior
for invalid IDNA hostnames while leaving valid host and hostname updates
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1398ab71-4289-44f9-8366-851ec79af66c
📒 Files selected for processing (9)
scripts/regenerate-uts46-override.shsrc/jsc/bindings/BunIDNA.cppsrc/jsc/bindings/BunIDNA.hsrc/jsc/bindings/DOMURL.cppsrc/jsc/bindings/NodeURL.cppsrc/jsc/bindings/URLDecomposition.cppsrc/jsc/bindings/bun_icu_decompress.cpptest/js/node/url/url-domain-ascii-unicode.test.jstest/js/web/url/url.test.ts
There was a problem hiding this comment.
No bugs found, but this needs a maintainer's eyes: it ships a binary ICU data blob swapped in at runtime via byte-prefix matching, and layers IDNA re-validation on top of WTF::URLParser — both are architectural workarounds the PR itself flags as temporary pending oven-sh/WebKit changes.
Checked: domainHasACELabel label-boundary logic (dot-delimited, case-insensitive, doesn't false-positive on mid-label xn--); the domainToASCII overflow retry loop terminates (single heap retry, then bails); maybeOverrideUTS46 prefix match is length-guarded and falls through cleanly on non-match; jsDomainToASCII fast-path gate preserves the prior return-arg0 behavior for non-ACE ASCII; all DOMURL entry points (create×3, parseInternal, setHref) plus URLDecomposition::setHost/setHostname/origin are consistently gated.
Extended reasoning...
Overview
Two-part IDNA/URL-Standard conformance fix. Part 1 ships a regenerated uts46.nrm (Unicode 16.0 data in Nrm2 v4 format) as a checked-in binary, embedded via #embed and substituted for the bundled ICU 73/75 item at runtime inside bun_icu_maybe_decompress by matching a 48-byte header prefix. Part 2 adds src/jsc/bindings/BunIDNA.{h,cpp} with domainHasACELabel / domainToASCII / urlHostIsValidIDNA, and threads urlHostIsValidIDNA through every DOMURL construction/mutation path plus URLDecomposition and jsDomainToASCII, so all-ASCII xn-- labels that WTF::URLParser's fast path lets through are re-validated via ICU. Tests cover both halves across constructor, canParse/parse, href/host/hostname setters, base URLs, blob: origin, non-special schemes, and the ICU buffer-overflow retry.
Security risks
URL parsing is security-relevant (origin computation, host normalization, redirect/SSRF filtering). This PR tightens validation (previously-accepted invalid Punycode now rejected) and loosens it for a small set of Unicode 16.0 reclassifications — both are moves toward the URL Standard / WPT, so the direction is right. The binary .nrm blob is the more unusual surface: it's regenerated from SHA-256-pinned upstream ICU sources by scripts/regenerate-uts46-override.sh, and the author states it reproduces byte-for-byte, but a maintainer should independently confirm the checked-in bytes match the script's output before merging a binary that ICU will consume as normalization data.
Level of scrutiny
High. This is not a mechanical change: it introduces a runtime ICU data-substitution mechanism keyed on exact byte prefixes (with the author's justification for uniqueness in a PR comment), adds a validation layer above WTF::URLParser rather than fixing the parser itself (acknowledged as a stopgap until the WebKit prebuilt is bumped), and changes user-visible new URL() acceptance behavior. The approach is well-reasoned and thoroughly tested, but the "ship an override blob + re-check at the DOMURL layer" strategy is exactly the kind of design call a Bun maintainer should sign off on rather than a bot.
Other factors
CI on the pre-latest commit was green on the target suite across all lanes; all CodeRabbit and comment-cop threads are resolved. Test coverage is strong (variant matrix across schemes, entry points, valid/invalid ACE labels, percent-encoded, non-special schemes, buffer-overflow retry). The Unicode 16.0 half is platform-gated (skipIf(!(isLinux || isWindows))), which the PR explains. The PR also notes it does not extend to WTF::URL callers outside DOMURL (fetch/Request) — a known scope boundary a maintainer may want to weigh.
|
CI on 1d27a31 (build 83753) never ran: all 32 build jobs show The last build that actually executed (75881 on 7499cbc, before the #33201 cherry-pick) was 283/286 with All review threads are resolved. Ready for a maintainer when CI capacity recovers. |
|
Closing: both parts of this are on main by a different route.
The cases main's tests did not already have are ported in #39415. |
…II and URL Ports the test tables from the superseded #33201, #34731 and #33206 that current tests did not cover: domainToASCII/domainToUnicode host parsing (percent-decoding, IPv4/IPv6, authority terminators, tab and newline stripping, invalid punycode), punycode labels that decode to disallowed code points across every special scheme, the setters, hosts longer than the IDNA conversion's initial buffer, and more of the Unicode 16 UTS #46 reclassifications. Also makes the domainToUnicode invalid-input loop call domainToUnicode instead of domainToASCII.
1 participant
Two IDNA divergences from the URL Standard, both surfaced by WPT
url/IdnaTestV2.Part 1: accept hostnames reclassified from disallowed to mapped in Unicode 16.0
These codepoints were
disallowedin Unicode 15.1's IdnaMappingTable.txt and changed tomappedin Unicode 16.0's (UTS #46 revision 33). They are the "late casefold additions": uppercase letters that existed in early Unicode but only gained lowercase counterparts in a later version. Unicode 16.0 also reclassified several format controls (U+180E, U+2061..2063, U+3164, ...) fromdisallowedtoignored.WTF::URLParser::domainToASCIIcalls ICU'suidna_nameToASCII, which consultsuts46.nrm. The bundled ICU prebuilts are ICU 75.1 (Linux/musl) and ICU 73.2 (Windows), both derived from pre-16.0 data.Fix: ship a replacement
uts46.nrmregenerated from Unicode 16.0's IdnaMappingTable, emitted in Nrm2 format version 4 (what ICU 73/75's Normalizer2 reads), and swap it in via the existingbun_icu_maybe_decompressudata hook. The stock item is matched by its exact 48-byte prefix (DataHeader + first four Nrm2indexes[], unique among the*.nrmin the package).scripts/regenerate-uts46-override.shrebuilds the blob deterministically from SHA-256-pinned upstream ICU artifacts. Applies on Linux and Windows (the platforms whose prebuilts carry the udata hook); self-disables once the WebKit prebuilt moves to ICU 76+.Part 2: reject invalid Punycode (
xn--) labels in special-scheme hostsWTF::URLParser::parseHostAndPorthas an all-ASCII fast path (and a second one inside its owndomainToASCII) that lowercases and appends the host without ever calling ICU, soxn--labels are never decoded or validated. The URL Standard's "domain to ASCII" runs UTS-46 processing on every host, which validates A-labels.Fix:
URLParser.cppitself ships in the prebuiltlibWTF.a, so the check is applied one layer up in Bun's own code.src/jsc/bindings/BunIDNA.{h,cpp}providesdomainHasACELabel/domainToASCII/urlHostIsValidIDNA;DOMURL::create,parseInternal,setHref, andURLDecomposition::originrequireurlHostIsValidIDNAin addition toisValid().jsDomainToASCIIgates its ASCII fast path on!domainHasACELabel. Only hosts that actually contain anxn--label reach ICU; every other all-ASCII host keeps the fast path.Supersedes #33201 (same commits, cherry-picked).
Verification
Adjacent suites unchanged:
test/regression/issue/24191.test.ts,test/js/node/url/url-domain-ascii-unicode.test.js,test/js/deno/url/url.test.ts,test/js/web/intl/intl.test.ts.Not covered here
Both upstream causes live in the oven-sh/WebKit prebuilt:
!containsACELabelremoves the need for the DOMURL-layer re-check (and extends it toWTF::URLcallers outside DOMURL such as fetch/Request)Those belong in separate oven-sh/WebKit changes followed by a
WEBKIT_VERSIONbump.no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/url/url.test.ts