node:url: handle long internationalised hosts in domainToASCII/domainToUnicode#33758
node:url: handle long internationalised hosts in domainToASCII/domainToUnicode#33758robobun wants to merge 5 commits into
Conversation
…ToUnicode domainToASCII returned '' for any non-ASCII host whose ToASCII output exceeded the fixed 2048-unit buffer or that contained a label longer than ICU's ENCODE_MAX_CODE_UNITS (1000 UTF-16 units). The URL Standard runs UTS #46 with VerifyDnsLength=false so neither limit should cause rejection; Node accepts all of these. On U_BUFFER_OVERFLOW_ERROR the call is now retried with a heap buffer sized from the preflight length. On U_INPUT_TOO_LONG_ERROR (which ICU's uts46.cpp propagates when u_strToPunycode hits its per-label code-unit cap) we fall back to running uidna_nameToUnicode for the UTS #46 mapping/NFC/validation and then encode each non-ASCII label with an uncapped RFC 3492 encoder. domainToUnicode gets the same buffer-overflow retry. domainToASCII now returns an 8-bit string so domainToUnicode(domainToASCII(x)) round-trips. The analogous limits in WTF::URLParser::domainToASCII (new URL) live in WebKit and need a change there plus a WEBKIT_VERSION bump.
|
Warning Review limit reached
Next review available in: 5 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)
WalkthroughAdds ICU fallback handling for long IDN labels in ChangesIDN Punycode Fallback
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@test/js/node/url/url-domain-ascii-unicode.test.js`:
- Around line 103-106: The explanatory comment in the URL domain ASCII/unicode
test exceeds the repo’s 3-line limit. Compact the comment in
url-domain-ascii-unicode.test.js so it still explains the ICU
ENCODE_MAX_CODE_UNITS and 2048-unit buffer behavior, but fits within three
lines; keep the wording near the existing comment block and preserve the
reference to the URL Standard/UTS `#46` behavior.
- Line 108: The test suite is using a require for node:punycode inside the test
body, but this file is not testing require behavior. Move the punycode
dependency to a module-scope import at the top level of
url-domain-ascii-unicode.test.js, keeping the test logic unchanged and
referencing the existing punycode usage points so the suite no longer depends on
require.
🪄 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: 389e9ac2-cb26-4fb2-bb42-6e4afd27c80e
📒 Files selected for processing (2)
src/jsc/bindings/NodeURL.cpptest/js/node/url/url-domain-ascii-unicode.test.js
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/node/url/url-domain-ascii-unicode.test.js (1)
123-126: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNormalization inconsistency between expected-value and round-trip assertions.
Line 123 computes the expected ASCII form via
input.normalize("NFC").toLowerCase(), but the round-trip check on line 126 compares againstinput.toLowerCase()without the NFC step. Current test inputs don't contain characters affected by NFC, so this passes, but it's a latent inconsistency if a future case includes a decomposed character.♻️ Proposed fix
- expect(url.domainToUnicode(got)).toBe(input.toLowerCase()); + expect(url.domainToUnicode(got)).toBe(input.normalize("NFC").toLowerCase());🤖 Prompt for 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. In `@test/js/node/url/url-domain-ascii-unicode.test.js` around lines 123 - 126, The test in url-domain-ascii-unicode.test.js has a normalization mismatch between the expected ASCII value and the round-trip Unicode assertion. Update the round-trip check in the affected test case to use the same NFC normalization as the expected-value computation, matching the logic around punycode.toASCII and url.domainToUnicode so both assertions compare against the same normalized input.
🤖 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.
Outside diff comments:
In `@test/js/node/url/url-domain-ascii-unicode.test.js`:
- Around line 123-126: The test in url-domain-ascii-unicode.test.js has a
normalization mismatch between the expected ASCII value and the round-trip
Unicode assertion. Update the round-trip check in the affected test case to use
the same NFC normalization as the expected-value computation, matching the logic
around punycode.toASCII and url.domainToUnicode so both assertions compare
against the same normalized input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 834bd83c-bdb8-49d7-b807-3fe0e1919424
📒 Files selected for processing (1)
test/js/node/url/url-domain-ascii-unicode.test.js
|
CI status: the two remaining failures on build 70509 are unrelated to this change.
|
1 participant
url.domainToASCII/url.domainToUnicodesilently reject long internationalised hosts at two internal limits the URL Standard does not have, so valid domains come back as""in Bun but are accepted by Node.The URL Standard runs UTS #46 ToASCII with
VerifyDnsLength=false, so host length is not the URL parser's business. The same-length all-ASCII forms are accepted with no limit.Cause
jsDomainToASCII/jsDomainToUnicodeinsrc/jsc/bindings/NodeURL.cppcalluidna_nameToASCII/uidna_nameToUnicodeonce into a fixedchar16_t[2048]and treat any non-successUErrorCodeas "invalid domain". Two distinct ICU conditions are being conflated with invalidity:U_BUFFER_OVERFLOW_ERROR: the UTS Bun v0.0.41 #46 result does not fit in 2048 units. ICU returns the required length; this is the standard preflight signal, not an invalid domain.U_INPUT_TOO_LONG_ERROR: a single non-ASCII label exceedsENCODE_MAX_CODE_UNITS(1000) in ICU'su_strToPunycode.uts46.cpppropagates this error code without retry. The domain is valid; ICU's punycode encoder just has a hard cap.Fix
In
NodeURL.cpp:U_BUFFER_OVERFLOW_ERROR, retry with a heap buffer sized from the preflight return value (both ToASCII and ToUnicode).U_INPUT_TOO_LONG_ERRORfrom ToASCII, fall back to runninguidna_nameToUnicode(which performs the UTS Bun v0.0.41 #46 mapping/NFC/validation without the punycode-encode step for non-ACE labels) and then encode each non-ASCII label with an uncapped RFC 3492 encoder. Overflow in the RFC 3492 delta arithmetic is handled viaCheckedUint32. The sameallowedNameToASCIIErrorsmask is applied so disallowed/invalid input is still rejected.domainToASCIInow returns an 8-bitWTF::String(the result is always ASCII), sodomainToUnicode(domainToASCII(x))round-trips instead of hitting the!is8Bit()fast path.Verification
New tests compare
domainToASCIIoutput byte-for-byte against the pure-JSnode:punycodereference (no length cap) and round-trip throughdomainToUnicode; all expectations were cross-checked against Node v26.3.0. Coverage: single label just past the 1000-unit cap, 1500-unit label, long label mixed with ASCII labels, supplementary-plane code point, all-non-ASCII label, several long labels (>2048 output), ToUnicode with >2048 output, and that a disallowed code point in a long label is still rejected.Not covered here
new URL(\http://${label1001}/`)still throws:WTF::URLParser::domainToASCIIhas the same fixed buffer and no overflow/input-too-long handling, but that code lives in the prebuilt WebKit (vendor/WebKitis not compiled by the default profile). Fixing it needs a change in oven-sh/WebKit and aWEBKIT_VERSION` bump.