Skip to content

node:url: run domainToUnicode regardless of string backing store#33756

Open
robobun wants to merge 2 commits into
mainfrom
farm/fd2aa854/domaintounicode-16bit
Open

node:url: run domainToUnicode regardless of string backing store#33756
robobun wants to merge 2 commits into
mainfrom
farm/fd2aa854/domaintounicode-16bit

Conversation

@robobun

@robobun robobun commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

url.domainToUnicode returned its argument verbatim whenever the input WTF::String happened to be 16-bit backed, so the documented round trip never decoded and two === strings could produce different outputs.

Repro

import { domainToASCII, domainToUnicode } from "node:url";

domainToUnicode(domainToASCII("bücher.de"));
// bun: "xn--bcher-kva.de"   node: "bücher.de"

domainToUnicode("中.xn--fiqs8s");
// bun: "中.xn--fiqs8s"       node: "中.中国"

const a = "xn--bcher-kva.de";
const b = "xn--bcher-kva.de中".slice(0, -1);   // === a, 16-bit backed
console.log(a === b, domainToUnicode(a), domainToUnicode(b));
// bun: true "bücher.de" "xn--bcher-kva.de"
// node: true "bücher.de" "bücher.de"

Cause

jsDomainToUnicode in src/jsc/bindings/NodeURL.cpp short-circuited on !domain.is8Bit():

if (!domain.is8Bit())
    // this function is only for undoing punycode so its okay if utf-16 text makes it out unchanged.
    return JSC::JSValue::encode(arg0);

is8Bit() is a representation property of WTF::String, not a value property. An all-ASCII punycode host can be stored in a 16-bit buffer: domainToASCII always returns one (it builds the result from a char16_t[]), and so do slices of any 16-bit rope and literals from a source file containing non-Latin-1 text. A 16-bit string can also contain xn-- A-labels mixed with already-decoded labels ("中.xn--fiqs8s"), and UTS #46 ToUnicode is defined over all inputs, including mapping and case folding of code points above U+00FF.

Fix

Drop the fast path and always call uidna_nameToUnicode, matching the structure jsDomainToASCII already uses for the 8-bit/16-bit split.

Verification

test/js/node/url/url-domain-ascii-unicode.test.js: 134 pass. The four new tests fail on released Bun and pass with this change; the 130 pre-existing tests are unchanged.

#33206 is a broader rewrite of both domainToASCII and domainToUnicode through the WHATWG host parser that also removes this fast path; this PR is the minimal targeted fix for the representation-dependent result and conflicts only on the three lines it deletes.

jsDomainToUnicode returned its argument verbatim whenever the input
WTF::String was 16-bit backed. is8Bit() is a representation property,
not a value property: an all-ASCII punycode label can be stored in a
16-bit buffer (domainToASCII always returns one, as do slices of a
16-bit rope), and UTS #46 ToUnicode is defined over all inputs, not
only Latin-1 ones. The result was that domainToUnicode(domainToASCII(d))
never decoded, and two === strings could produce different outputs.

Drop the fast path and always call uidna_nameToUnicode, matching the
structure jsDomainToASCII already uses.
@robobun

robobun commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Reproduced with:

bun -e 'import {domainToASCII as a, domainToUnicode as u} from "node:url"; const x="xn--bcher-kva.de", y=(x+"中").slice(0,-1); console.log(x===y, u(x), u(y), u(a("bücher.de")))'

Fix in #33756.

CI: url-domain-ascii-unicode.test.js passes on every lane. The remaining red is unrelated infra flake (build 70489: postgres ECONNREFUSED on win-x64; build 70495: MySQL container health-check timeout on musl-aarch64 for 26030.test.ts). Ready for review.

@github-actions github-actions Bot added the claude label Jul 8, 2026
@robobun

robobun commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 7:05 AM PT - Jul 8th, 2026

@robobun, your commit 7ca8fa5 has 2 failures in Build #70495 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 33756

That installs a local version of the PR into your bun-33756 executable, so you can run:

bun-33756 --bun

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ef7a83c7-13bb-4ba1-81fe-031d8eb61f5c

📥 Commits

Reviewing files that changed from the base of the PR and between 332f744 and 7ca8fa5.

📒 Files selected for processing (2)
  • src/jsc/bindings/NodeURL.cpp
  • test/js/node/url/url-domain-ascii-unicode.test.js

Walkthrough

This PR changes jsDomainToUnicode so non-8-bit inputs proceed through IDNA conversion instead of being returned unchanged. It also adds tests for string-representation stability, round-tripping, mixed-label A-label decoding, and UTS #46 mapping.

Changes

domainToUnicode IDNA Conversion Fix

Layer / File(s) Summary
IDNA conversion path
src/jsc/bindings/NodeURL.cpp
Removes the non-8-bit early return in jsDomainToUnicode, converts only 8-bit inputs to UTF-16, and allows all inputs to reach uidna_nameToUnicode.
domainToUnicode test coverage
test/js/node/url/url-domain-ascii-unicode.test.js
Adds assertions for equal-string representation stability, Unicode-to-ASCII-to-Unicode round-tripping, A-label decoding with mixed labels, and UTS #46 mapping.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main fix: always decoding domainToUnicode independent of string backing store.
Description check ✅ Passed The description covers what changed, why, a repro, the fix, and verification, though it uses different headings than the template.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — removes a demonstrably incorrect fast path and mirrors the existing jsDomainToASCII structure, with tests covering the representation-dependence, round-trip, and mixed-label cases.

Extended reasoning...

Overview

This PR touches two files: a 3-line functional change in src/jsc/bindings/NodeURL.cpp (removing the !domain.is8Bit() early-return in jsDomainToUnicode and replacing it with the same if (is8Bit()) convertTo16Bit() pattern used by jsDomainToASCII directly above), and four new test cases appended to test/js/node/url/url-domain-ascii-unicode.test.js.

The root cause is well-argued and correct: WTF::String::is8Bit() reflects the backing store, not the value, so the old fast path made domainToUnicode return different results for ===-equal strings and broke the documented domainToUnicode(domainToASCII(x)) round trip (since domainToASCII builds its result from a char16_t[]). The fix unconditionally routes through uidna_nameToUnicode, which is what UTS #46 ToUnicode requires anyway.

Security risks

None. This is a Node-compat IDNA decoding helper; the change removes a short-circuit and hands the same input to the same ICU function that already handles the 8-bit case. No new allocation sizing, no new parsing of untrusted lengths, no auth/crypto/permissions surface.

Level of scrutiny

Low. The diff is tiny, mechanical, and mirrors the sibling function line-for-line. The new code path (span16()uidna_nameToUnicode) is the exact path already exercised by every 8-bit input after convertTo16Bit(), so there's no genuinely new behavior being introduced — just the removal of an incorrect bypass.

Other factors

The added tests are strong: they assert the specific representation-independence property (two === strings produce the same output), the round-trip over the existing 50+ pair fixture table, mixed A-label/U-label inputs, and UTS #46 mapping on non-Latin-1 input. The PR description verifies the new tests fail on released Bun and pass with the change, and notes the relationship to the broader #33206 rewrite. No outstanding reviewer comments, no bugs flagged by the bug-hunting pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant