node:url: strip tab and newline before the bracket and port checks in domainToASCII/domainToUnicode - #39415
node:url: strip tab and newline before the bracket and port checks in domainToASCII/domainToUnicode#39415robobun wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 0 remain after this review. WalkthroughThe change strips selected ASCII whitespace before host parsing and adds parity coverage for domain conversion and URL hostname handling, including UTS ChangesURL host parsing parity
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: ready for review.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only additions salvaging coverage from three superseded PRs, plus a fix for the domainToUnicode invalid-input loop that was accidentally calling domainToASCII.
What was reviewed:
- Verified the
invalidstable values ("@","a@b",null,undefined, bare IPv6) hold fordomainToUnicodetoo, so the one-line fix doesn't break existing assertions. - Checked test conventions:
Buffer.alloc(n, fill).toString()for long strings,JSON.stringify(input)for unique test names, existingparseInt(process.versions.icu) < 76skip pattern reused, tests appended to existing files. - Checked for flakiness sources — no sleeps, no network, no timing assertions; all data-driven
toEqual/toBeon fixed inputs.
Extended reasoning...
Overview
Test-only PR touching two files: test/js/node/url/url-domain-ascii-unicode.test.js and test/js/web/url/url.test.ts. It adds a ~75-row hostParserParity table exercising domainToASCII/domainToUnicode as a full WHATWG host parse (percent-decoding, forbidden code points, IPv4/IPv6 canonicalization, tab/newline stripping, authority terminators, invalid punycode), a >256-code-unit host test to cover the runUIDNA U_BUFFER_OVERFLOW_ERROR retry, a Unicode 16 UTS #46 test under an ICU-version skip, and four new URL tests covering invalid xn-- labels across all six special schemes, the href/host/hostname setters, and valid/non-punycode xn-- substrings. It also fixes a copy-paste bug where the domainToUnicode invalid-input loop was calling domainToASCII.
Security risks
None. No runtime code is touched; only test assertions are added.
Level of scrutiny
Low-to-medium. The change is purely additive test coverage plus a one-line test fix. The main risk is a wrong expected value causing CI to go red on some platform, and the PR description addresses this directly: every expected value was cross-checked against Node 26.3, both files pass on main (bun bd test, 210 + 28 pass), and the tests were mutation-tested by stubbing hasValidPunycodeHost to confirm they fail when the code they cover is broken. The Unicode 16 assertions are correctly gated behind parseInt(process.versions.icu) < 76 (matching the pre-existing pattern in the same file) so macOS with older system ICU won't fail.
Other factors
- Repo test conventions are followed:
Buffer.alloc(1000, "a").toString()instead of"a".repeat(1000); tests added to existing files rather than new ones; exact-value assertions viatoEqualon{ascii, unicode}objects (single failure shows both values);JSON.stringify(input)produces unique test names for control-character inputs. - The one-line
domainToASCII→domainToUnicodefix in the existing invalid-input loop is safe: the fiveinvalidsentries ("@","a@b",null→"null",undefined→"undefined", bare IPv6→"") produce identical results under both functions, so the assertion values remain correct. - No hermeticity concerns: no subprocesses, no network, no filesystem, no timing.
- No prior human review comments to address; only a CodeRabbit rate-limit notice on the timeline.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Note The previously reviewed commits are no longer reachable (likely due to a force-push or rebase), so CodeRabbit is performing a full review instead of an incremental one. This review may take a little longer. |
robobun
changed the title
There was a problem hiding this comment.
LGTM — the description now matches the diff (my earlier nit is addressed), and the 3-line parseDomainAsHost change plus test additions look correct.
What was reviewed:
removeCharactersstrips tab/LF/CR before the bracket/:/@checks, matching the basic URL parser's first step; inputs without those chars hit the no-copy path.- The
domainToUnicodeinvalids loop was callingdomainToASCII— now fixed; both functions share the sameinvalidsexpected values so the assertion still holds. - New
hostParserParityrows and theurl.test.tsadditions are additive, ICU-76-gated where needed, and avoid pinning the all-ASCIIxn--rejection that Node 26.7 changed.
Extended reasoning...
Overview
Three files: a 3-line change to parseDomainAsHost in src/jsc/bindings/NodeURL.cpp (rename the parameter to input, strip \t/\n/\r via WTF::String::removeCharacters into domain before the existing terminator scan and bracket/port/userinfo checks), ~115 lines of new test rows in test/js/node/url/url-domain-ascii-unicode.test.js (a hostParserParity table, a Unicode 16 block behind process.versions.icu < 76, and a fix for the existing domainToUnicode invalids loop that was calling domainToASCII), and ~32 lines in test/js/web/url/url.test.ts (more Unicode 16 code points in the existing ICU-gated test, plus one new it() for valid xn-- labels and opaque-host schemes).
Security risks
None identified. The strip runs before the pre-checks, which only makes domainToASCII/domainToUnicode accept inputs like "\t[::1]" that WTF::URL (and Node, and the WHATWG basic URL parser) already accept — Bun's own new URL("ws://\t[::1]/") already parsed these. The only other callers reach parseDomainAsHost via jsDomainToASCII/jsDomainToUnicode with user-supplied strings; url.format and fileURLToPath pass hostnames taken from already-parsed URLs, which never contain tab/newline.
Level of scrutiny
Low-to-moderate. The native change is a single spec-step reorder in a Node-compat helper; removeCharacters returns the receiver unchanged when nothing matches, so the common path is unaffected. The bulk of the diff is test coverage with expected values checked against Node 26.3 and 26.7, and the evidence block confirms 3 rows fail on main and pass with the fix under both ASAN-debug and release.
Other factors
Since my previous review the PR description was rewritten to drop the long-host / "four tests" claims I flagged, so that nit is resolved without code changes. The comment-cop feedback on the long inline comment was addressed in 5be43fb (now a one-line spec citation). The scope note deliberately avoids adding rows that pin all-ASCII invalid-xn-- rejection, which diverged between Node 26.3 and 26.7 — that keeps the new tests stable across the upstream change. No outstanding reviewer comments remain.
…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.
… domainToASCII/domainToUnicode parseDomainAsHost looked for the IPv6 brackets and for ':' / '@' in the raw input, so a tab or newline next to the brackets failed the host even though the URL parser removes those characters first; Node returns the address. The test tables are reduced to the rows whose expected values hold in both Node 26.3 and Node 26.7: ada 4.0 (Node 26.7), the URL Standard and WPT no longer fail all-ASCII hosts with invalid xn-- labels, so the rows pinning that rejection, and the long-host tests built on it, are dropped.
5be43fb to
15eb75f
Compare
There was a problem hiding this comment.
LGTM — the description now matches the diff (my earlier nit is addressed), and the two-line removeCharacters strip in parseDomainAsHost is the spec-correct ordering.
What was reviewed:
removeCharacterspredicate matches exactly the WHATWG "ASCII tab or newline" set (\t,\n,\r); no-match inputs take the no-copy path, and the lambda works for both 8-bit and 16-bit WTF strings.parseDomainAsHostis file-static with onlyjsDomainToASCII/jsDomainToUnicodeas callers; the siblingurl.hostnamesetter gap is called out as a separate WTF::URL issue.- The
domainToUnicodeinvalid-input loop fix (was callingdomainToASCII) — theinvalidstable's expected values hold for both functions, so the corrected loop still passes. - The
hostParserParitytable deliberately omits all-ASCII invalid-xn--rejection rows per the scope note (Node 26.3 vs 26.7 divergence) — no test locks in behaviour that's about to change upstream.
Extended reasoning...
Overview
Two-line native change in src/jsc/bindings/NodeURL.cpp: parseDomainAsHost now strips ASCII tab, LF and CR from its input before running the terminator scan and the [/]/:/@ pre-checks, so domainToASCII("\t[::1]") and domainToUnicode("[::1]\n") return "[::1]" instead of "", matching Node and Bun's own new URL(). The rest is test coverage: a ~60-row hostParserParity table in url-domain-ascii-unicode.test.js (three rows fail on main), a fix to the existing domainToUnicode invalid-input loop that was accidentally calling domainToASCII, an ICU-76-gated Unicode 16 block, and one new + one extended test in url.test.ts.
Security risks
None. This is a Node-compat string-normalization step in url.domainToASCII/domainToUnicode, which return strings and are not on any auth, TLS, or network-connect path. The change makes Bun accept exactly the inputs the WHATWG basic URL parser and Node already accept (tab/newline are stripped, not passed through), and the actual host validation still goes through WTF::URL and hasValidPunycodeHost unchanged.
Level of scrutiny
Low-to-medium. The native change is one removeCharacters call whose predicate is the literal WHATWG "ASCII tab or newline" set, placed at the top of a static helper with two in-file callers. WTF::String::removeCharacters returns *this when nothing matches, so the common path is unaffected. The test additions are large but mechanical — table rows whose expected values were checked against Node 26.3 and 26.7 — and the [review] gate evidence shows 3 fail without the fix / all pass with it on both ASAN-debug and release.
Other factors
My previous review's only finding was a description/diff mismatch (promised long-host and extra url.test.ts tests that weren't in the diff). The description has since been rewritten to describe exactly what's present, and that thread is resolved. The comment-cop bot's paragraph-comment complaint was also addressed (comment shortened to one line). The scope note explaining why no all-ASCII invalid-xn-- rejection rows are added is sound — Node 26.7 changed that behaviour, so pinning it now would just create churn. Nothing outstanding remains.
1 participant
Problem
url.domainToASCII("\t[::1]")andurl.domainToUnicode("[::1]\n")return""in Bun; Node returns"[::1]"for both (26.3 and 26.7)."\t[::1]"as a hostname also parses fine in Bun's ownnew URL("ws://\t[::1]/"), so the two entry points disagree with each other as well.parseDomainAsHost(src/jsc/bindings/NodeURL.cpp) looks for the IPv6 brackets and for:/@in the raw input before handing it toWTF::URL. The basic URL parser removes ASCII tab and newline from its input before anything else, andWTF::URLdoes, but these checks ran first, so"\t[::1]"failed the:check and"[::1]\n"failed the closing bracket check. Tab or newline inside the brackets, or in a domain name, already worked because onlyWTF::URLsaw them.Fix
parseDomainAsHoststrips tab, LF and CR from the input before the terminator scan and the bracket /:/@checks. That is the order the host parse Node runs (ada'sset_hostname) uses; for the 24 tab/newline inputs in the details block Bun now prints the same as Node 26.7 byte for byte. Inputs without those characters takeremoveCharacters' no-copy path and are unaffected; the other callers of the two functions (url.format,fileURLToPath) pass hostnames taken from parsed URLs, which never contain them.test/js/node/url/url-domain-ascii-unicode.test.js: the new"\t[::1]","[::1]\n"and"\t[::1]\r\n"rows fail on main ("") and pass with the change. The rest of thehostParserParitytable pins the host-parse semantics ofdomainToASCII/domainToUnicodethat nothing on main covered: percent-decoding before IDNA, forbidden code points, IPv4 and IPv6 canonicalization, the/ ? # \terminators, port and userinfo failing, directdomainToUnicodeoutputs. The existingdomainToUnicodeinvalid-input loop calleddomainToASCII; it now callsdomainToUnicode. A Unicode 16 test is added under the sameprocess.versions.icu < 76skip the other Unicode 16 tests use.test/js/web/url/url.test.ts: more Unicode 16 code points in the existing test, plus a table of hosts that must keep parsing: validxn--labels, a label that merely containsxn--,xn--in userinfo / path / query / fragment, and opaque hosts of non-special schemes.xn--label is rejected. The URL Standard (whatwg/url a8d5ca3716), WPT (b63305b743) and ada 4.0, shipped in Node 26.7, changed in June to August to accept such hosts (domainToASCII("xn--a")is"xn--a"in Node 26.7,""in 26.3). Bun'shasValidPunycodeHostfrom node compat batch: callback-throw dispatch, Assert class + native deep-equality parity, Intl gate + URL/buffer fallout, compile cache, watch kill-signal, profilers (+98 tests) #34660 still rejects them, as do the WPT fixtures vendored from Node 26.3, so adding more tests for the rejection would only make following upstream harder; whether to follow it is a separate decision (it also affects url: give blob: URLs a null origin when the inner host has invalid punycode #39404). Every expected value in this PR holds in Node 26.3 and Node 26.7 (checked with both binaries).bun bd test test/js/node/url/url-domain-ascii-unicode.test.js(200 pass; 3 fail without theNodeURL.cppchange),bun bd test test/js/web/url/url.test.ts(25 pass), the vendoredtest-url-domain-ascii-unicode.js,test-whatwg-url-custom-domainto.js,test-whatwg-url-toascii.js,test-url-fileurltopath.jsandtest-url-format-whatwg.jspass.bun bd test test/js/node/url test/js/web/urlhas one failure, the pre-existingurl-canParse-whatwg.test.js5s timeout under the debug build, identical without this change.url.hostname = "\t[::1]"(andurl.host =) is still a no-op in Bun while Node sets[::1]; that one is insideWTF::URL::setHost, a different code path, and everything else in that area ("\texample.com","[::1]\n","\t[::1]:81") already matches.Background
url.domainToASCII/domainToUnicodeare defined by Node as: take aws://xURL, set its hostname to the input, return the resulting hostname, or""if the setter failed. That is why percent-decoding, IP address canonicalization, the authority terminators and tab/newline removal all show up in their results. Bun implements the same thing inparseDomainAsHostwith a few pre-checks in front ofWTF::URL(the hostname setter rejects ports, anda@bwould otherwise parse as userinfo).new URL()and the setters. Its first step removes every ASCII tab, LF and CR from the input; everything after that, including the host parser, never sees them.xn--label: the ASCII form of an internationalized label. UTS Bun v0.0.41 #46 defines which ones are valid; a non-ASCII host still goes through full UTS Bun v0.0.41 #46 processing everywhere (Bun, Node 26.3, Node 26.7), which is what the table'sxn--te\u0161larow pins. What changed upstream is only the treatment of hosts that are entirely ASCII.process.versions.icuis older.domainToASCII / domainToUnicode with tab and newline, before and after (Node 26.3 and 26.7 agree with the "after" column on every line)
[review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file