url: stop running url.parse() hostnames through the WHATWG host parser - #33460
url: stop running url.parse() hostnames through the WHATWG host parser#33460robobun wants to merge 6 commits into
Conversation
|
Warning Review limit reached
Next review available in: 20 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 (8)
WalkthroughAdds a native ChangesURL host/port validation tightening
Sequence Diagram(s)sequenceDiagram
participant Caller
participant UrlParse as Url.prototype.parse
participant ToASCII as toASCII binding
Caller->>UrlParse: parse(input)
UrlParse->>UrlParse: parseHost()
alt hostname is IPv6
UrlParse->>UrlParse: check forbiddenHostCharsIpv6
else hostname is not IPv6
UrlParse->>ToASCII: toASCII(hostname)
ToASCII-->>UrlParse: ASCII string or empty
UrlParse->>UrlParse: check empty or forbiddenHostChars
end
UrlParse-->>Caller: parsed Url or throw ERR_INVALID_URL
UrlParse->>UrlParse: getHostname() encounters ':'
UrlParse-->>Caller: throw ERR_INVALID_ARG_VALUE
Related Issues: None referenced. Related PRs: None referenced. Suggested labels: node.js, url, needs-review Suggested reviewers: None determinable from provided information. 🐰 A hostname twisted, colons astray, 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:06 PM PT - Jul 6th, 2026
❌ @robobun, your commit 34eef26 has some failures in 🧪 To try this PR locally: bunx bun-pr 33460That installs a local version of the PR into your bun-33460 --bun |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Good catch on #24812 — confirmed, and the stack trace in that issue points at the exact line this PR removes ( Verified against node v26.3.0 in the same container: Added Kept the test in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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-parse-invalid-input.test.js (1)
7-7: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe updated assertions are unreachable: they live inside a
test.todo(...)block that also references an undefinedcommonglobal.The whole body (lines 7-112) is wrapped in
test.todo("invalid input", () => {...}), and it usescommon.hasIntl/common.spawnPromisified(line 54, 99) even thoughcommonis never imported in this file (onlydescribe/testfrombun:test,assert, andurlare imported). That means the newassert.throws(() => url.parse(badURL), { code: "ERR_INVALID_ARG_VALUE" })assertions added here never actually execute, so this edit provides no real coverage of the new behavior. Equivalent, working coverage for these same cases already exists intest/js/node/url/url.test.ts(it.eachcases for"https://evil.com:.example.com"and"git+ssh://git@github.com:npm/npm"), so the dead code here should either be removed or the.todoshould be resolved now that this PR implements the exact behavior the// TODO: Support error code.comment calls out.Based on learnings/coding guidelines: "Un-skip
.todotests your fix makes pass."Also applies to: 90-93, 106-110
🤖 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-parse-invalid-input.test.js` at line 7, The new invalid-URL assertions are currently unreachable because the block is wrapped in test.todo and the file also references an undefined common global. Update the url.parse invalid-input test so the cases in the invalid input suite actually run, either by converting test.todo("invalid input", ...) to an active test or by moving the working assertions into an existing non-todo test, and remove/fix the common.hasIntl and common.spawnPromisified usages by importing or replacing them appropriately. Use the invalid input test name and the url.parse assertions as the anchor points when making the change.Source: Coding guidelines
🤖 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/NodeURL.cpp`:
- Around line 7-8: The IDNA error-mask is duplicated between the new file-scope
constant and the local constant in jsDomainToUnicode, so consolidate them into
one shared file-scope constant. Rename the shared mask to something like
allowedNameToIDNAErrors in NodeURL.cpp, then update both nameToASCII and
jsDomainToUnicode to use it instead of maintaining separate copies. Keep
hostnameBufferLength hoisted as-is and remove the redundant local
allowedNameToUnicodeErrors definition.
---
Outside diff comments:
In `@test/js/node/url/url-parse-invalid-input.test.js`:
- Line 7: The new invalid-URL assertions are currently unreachable because the
block is wrapped in test.todo and the file also references an undefined common
global. Update the url.parse invalid-input test so the cases in the invalid
input suite actually run, either by converting test.todo("invalid input", ...)
to an active test or by moving the working assertions into an existing non-todo
test, and remove/fix the common.hasIntl and common.spawnPromisified usages by
importing or replacing them appropriately. Use the invalid input test name and
the url.parse assertions as the anchor points when making the change.
🪄 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: 887c2896-4519-4872-8e6f-63d2c6285c1b
📒 Files selected for processing (8)
src/js/node/url.tssrc/jsc/bindings/NodeURL.cpptest/js/node/test/parallel/test-url-parse-format.jstest/js/node/test/parallel/test-url-parse-invalid-input.jstest/js/node/url/url-parse-format.test.jstest/js/node/url/url-parse-invalid-input.test.jstest/js/node/url/url-parse-ipv6.test.tstest/js/node/url/url.test.ts
💤 Files with no reviewable changes (2)
- test/js/node/url/url-parse-format.test.js
- test/js/node/test/parallel/test-url-parse-format.js
|
Review round addressed, all three findings were fair: Dead Duplicate IDNA error mask (240b928) — the two masks were byte-identical for the same reason, so there is now one
One deliberate narrowing there: the
|
CI status on fd0e934: the red lanes are queue starvation, not this diffBuild #68937 has been running for ~2h. Every failing status has the description
The C++ compile jobs that did get an agent all passed, and those are the ones that build the new So the binding compiles on 8 platforms including ASAN, and the lanes showing red are duplicates of those that never got scheduled. Deliberately not re-triggering. A new build would cancel the 24 green jobs and 244 in-flight ones and enqueue another ~280 onto the same saturated fleet, which is what caused the starvation in the first place. This branch pushed five commits in 75 minutes and each one cancelled its predecessor's build, so some of that pile-up is self-inflicted. Better to let #68937 drain. Locally on fd0e934: |
Legacy url.parse() mapped every hostname through `new URL("http://" + hostname)`
for IDNA support. That ran the WHATWG host parser, which canonicalizes IPv4 and
IPv6 hosts and rejects hosts the legacy grammar accepts.
Node applies a pure IDNA ToASCII to the hostname instead, then guards against
the mapping introducing a character that changes how the host is read. Add a
`toASCII` binding that does the UTS #46 mapping without host parsing, and port
node's forbidden-character guards. Also throw on an invalid port, which node
does as of v23 in place of the DEP0170 warning.
Both ICU call sites ignore the same error set, for the same reason: the WHATWG URL Standard turns off CheckHyphens and VerifyDnsLength. url-parse-invalid-input.test.js was entirely test.todo and referenced an undefined `common`, so it never ran. Its "TODO: Support error code" is what this branch implements, so rewrite it to run. The full badIDNA sweep stays in the parallel/ copy: it walks the whole code point range and takes minutes under a debug build.
node passes the reason where ERR_INVALID_ARG_VALUE expects the value, so the message it prints reads oddly. Assert it verbatim, and say so at the throw, to keep the argument order from being "corrected" into a message node never emits.
fd0e934 to
55a8401
Compare
Rebased onto main (55a8401); the one real failure was a stale base, not this diffFollowing up on my previous comment: most of #68937's red was status contexts expiring while their jobs sat unclaimed, but one job did eventually run and fail for real, so that comment was incomplete.
That is #32926 ( Nothing to do with It was already fixed on main by #33425 ( After the rebase: The two remaining |
There was a problem hiding this comment.
My earlier inline feedback is addressed and this pass found nothing new, but the scope — new C++ IDNA binding, changed url.parse() hostname semantics, DEP0170 warn→throw, and the spoofing-guard regexes — is security-adjacent enough that a maintainer should sign off.
Extended reasoning...
Overview
This PR replaces the WHATWG-host-parser detour in url.parse() with a pure UTS #46 toASCII binding, so hostnames are IDNA-mapped but not IP-canonicalized or WHATWG-validated — matching node's legacy grammar. Touches src/js/node/url.ts (hostname/port validation logic, new forbiddenHostChars guards, DEP0170 warning removed in favor of ERR_INVALID_ARG_VALUE throw), src/jsc/bindings/NodeURL.cpp (new nameToASCII helper + jsToASCII host function, consolidated allowedIDNAErrors mask), and six test files (new coverage plus re-syncing vendored node tests to v26.3.0 semantics). Fixes #24812.
Security risks
Hostname parsing is security-relevant. The PR description itself notes the pre-fix behavior could bypass hostname allow-lists via hex/octal/dword rewriting. The new forbiddenHostChars / forbiddenHostCharsIpv6 regexes are the load-bearing spoofing guards (: protocol spoof, @ auth spoof, [/] fake-IPv6). These mirror node's lib/url.js, and the PR verified byte-for-byte parity against node v26.3.0 across 36 hosts — but any error in these guards would be a security issue, so they deserve a second pair of eyes rather than bot-only approval.
Level of scrutiny
Medium-high. This is Node-compat code for a deprecated-but-still-widely-used API, with a new native binding and a lenient→strict behavioral change (invalid ports now throw instead of warn). The C++ side is small and follows the exact pattern of the existing jsDomainToASCII (same ICU transcoder, same error-mask, proper RETURN_IF_EXCEPTION after toWTFString). The JS side is a fairly direct port of node's own logic. Not a config tweak or mechanical change.
Other factors
- Both of my earlier inline comments were resolved: the dead
process.emitWarningmonkey-patch was removed, and the$ERR_INVALID_ARG_VALUEargument order was shown to match node v26.3.0 verbatim (now pinned by an exact-message test). - Test coverage is strong: new targeted tests in
url.test.ts, the previously-test.todourl-parse-invalid-input.test.jsrewritten to actually run, and vacuoustoMatchObject(expect.objectContaining(...))assertions inurl-parse-ipv6.test.tstightened to real assertions. - One known divergence (
https://[\\n::1]still throws) is documented in the PR body with its own root cause. - CI red on the latest push is queue-starvation
Expiredstatuses per the author's analysis; the C++ compile jobs that did run all passed.
Given the security-adjacent surface and the new native binding, deferring to a human maintainer rather than auto-approving.
Build 69089 went red on two infrastructure failures with no test or compile failure behind them: linux x64-baseline build-cpp expired without ever being assigned an agent (cascading 42 dependents), and darwin 26 aarch64 test-bun aborted on a 120s buildkite-agent artifact download timeout before running a single test. 238 jobs passed.
Build 69089: 238 passed, zero test or compile failures, two infrastructure abortsFinal tally on the rebased head:
Neither red mark is a test or compile failure.
It aborted fetching the prebuilt binary before the runner started. Meanwhile the jobs that compile the new 15 of 16. Note that Pushed a single |
There was a problem hiding this comment.
No bugs found and both earlier inline comments are addressed — deferring because this changes what hostnames url.parse() accepts/rejects (loosening validation to match node's legacy grammar), which is security-adjacent enough to warrant a human sign-off.
Extended reasoning...
Overview
This PR replaces the WHATWG-host-parser step in Url.prototype.parse with a pure UTS #46 toASCII binding, so url.parse() no longer canonicalizes/validates IPv4/IPv6 hosts and matches node's legacy grammar. It touches:
src/js/node/url.ts— swapsnew URL("http://" + hostname).hostnamefortoASCII(hostname)+forbiddenHostChars/forbiddenHostCharsIpv6guards;getHostnamenow throwsERR_INVALID_ARG_VALUEon invalid ports (was DEP0170 warning); drops theurlParsetry/catch since$ERR_INVALID_URLsets.inputitself (verified inErrorCode.cpp:2340-2347).src/jsc/bindings/NodeURL.cpp— extracts a sharednameToASCIIhelper, addsjsToASCII, refactorsjsDomainToASCIIto reuse it, consolidates the IDNA error mask.- Six test files re-synced/tightened, plus new coverage in
url.test.tsincluding the #24812 mongodb multi-host repro and a pinned error-message assertion.
Security risks
URL hostname parsing is security-adjacent: user code uses url.parse().hostname for allow-listing, logging, and re-emission. This change loosens what the legacy parser accepts (e.g. [:::1], [::banana], 192.168.1.256, 1.2.3.4.5, multi-host bracket groups now parse instead of throwing) and stops rewriting hex/octal/dword IPv4 forms. That is the correct node-compat behavior and the PR adds the same forbiddenHostChars spoofing guards node uses, but it is a deliberate accept/reject surface change on a legacy API that node itself flags as CVE-exempt. No new injection or memory-safety concerns spotted in the C++ — the new binding is a straightforward extraction of the existing ICU call with proper RETURN_IF_EXCEPTION after toWTFString.
Level of scrutiny
Medium-high. The diff is modest (~50 net LOC in src/) and closely mirrors node's lib/url.js, but it's a behavioral change to a parser whose output feeds security decisions, and it removes a validation layer (the WHATWG host parser) in favor of node's laxer legacy grammar. The author has been thorough — 36-host parity matrix vs node v26.3.0, per-platform CI analysis, and the known remaining divergence ([\n::1]) is called out — but a maintainer should confirm the accept/reject changes are the intended trade-off.
Other factors
Both of my earlier inline comments (dead process.emitWarning override; $ERR_INVALID_ARG_VALUE argument order) were addressed — the argument-order one turned out to be intentional node parity and is now pinned by a verbatim-message test with an explanatory comment. CodeRabbit's error-mask consolidation nit was also applied. CI on the latest push is pending (build #69179); prior builds were green modulo infrastructure expiries. Test coverage for the new behavior is solid, including the tightened toMatchObject assertions in url-parse-ipv6.test.ts that were previously vacuous.
On the accept/reject surface change, stated plainlyThe bot review is right that this loosens what Now accepted, previously threw
Now returned verbatim, previously rewritten
Now rejected, previously warned and continued Invalid ports: Still rejected, and the guard is now explicit rather than incidental Hostnames that IDNA-map into a character that changes how the host is read (U+FF20 to The honest trade-offIt depends entirely on which way the caller's check runs. For allow-lists, logging, and re-emission, the old behavior was the bug, and it is what prompted this report. For block-lists, i.e. SSRF guards, this is a real loosening and I do not want to paper over it. Before, Three things make that the correct call anyway:
So: deliberate, matches node byte for byte across the 36 host parity matrix, and the one behavior anybody should be relying on for a security decision is unchanged. Happy to gate any of it behind something else if a maintainer disagrees with the read. |
Re-roll (build 69179): 281 passed, same darwin artifact-download flake, re-roll spentThe
The failed shard never started a test: And This is the one sanctioned re-roll (34eef26) and it is now spent. I am not going to keep pushing Summary for a maintainer:
Ready to merge or to have the two darwin shards retried. |
|
landed in #34660, url.parse hostnames match node on main now |
2 participants
Fixes #24812
Repro
url.parse()also throws outright on a multi-host connection string, which node parses (#24812):Three user-visible breaks:
url.parse(x).hostname/.host/.hrefsees a different host string than node and than the wire input. Hostname allow-lists can be bypassed or broken by the hex/octal/dword rewrite.url.parse(hostHeader)with no try/catch has a reachable-input throw on any dotted name whose last label is out of range or extra (192.168.1.256,1.2.3.4.5).http://h:8a/xmis-splits into hostnameh+ pathname/:8a/xinstead of throwing.Cause
Url.prototype.parsepushed the hostname throughnew URL("http://" + hostname)for IDNA support. That runs the WHATWG host parser, which does far more than IDNA: it canonicalizes IPv4 and IPv6 hosts, and rejects hosts the legacy grammar accepts.Node's legacy parser applies a pure IDNA ToASCII and never validates or canonicalizes an IP. It is not
url.domainToASCIIeither: node'sdomainToASCIIis defined in terms of the host parser (domainToASCII("0x7f.1")is"127.0.0.1"), sourl.parseuses an internaltoASCIIinstead. #33206 is currently making Bun'sdomainToASCIImatch node on that point, which is correct for that API and is whyurl.parsemust not share it.Separately,
getHostnameemitted the DEP0170 warning and continued. Node removed that warning and throws instead, as of v23.Fix
toASCIIbinding toNodeURL.cpp: UTS Bun v0.0.41 #46 ToASCII via the same ICU transcoder, with no host parsing, so IPv4/IPv6 hosts pass through untouched.domainToASCIIand it now share onenameToASCIIhelper rather than two copies of the ICU call.Url.prototype.parseusestoASCII, then applies node's spoofing guards: reject a hostname that IDNA mapped to empty or to a character that changes how the host is read (:spoofs the protocol,@the auth,[/]fake IPv6). IPv6 hosts skip IDNA entirely and get the IPv6 variant of the guard.getHostnamethrowsERR_INVALID_ARG_VALUEon an invalid port.urlParse'scatchpatchedinputonto every error, only because thenew URL()throw carried the wrong one.$ERR_INVALID_URL(url)now sets it directly, so the catch is gone;ERR_INVALID_ARG_TYPEno longer gets a spuriousinput.Each guard is load-bearing:
toASCII("ab\uFF1Acd")returns"ab:cd", so onlyforbiddenHostCharsrejects it;toASCII("\u00AD")returns"", so only the empty check rejects it.Verification
Diffed against node v26.3.0 in the same container, comparing all 12
Urlproperties plus thrown error code and message, over 36 hosts (hex/octal/dword/short IPv4, IPv6 forms, out-of-range and extra-label IPv4, invalid ports, IDNA, punycode, spoofing code points). Identical, includingERR_INVALID_ARG_VALUE's exact message.test/js/node/url/: 201 pass. Two failures (pathToFileURL doesn't leak memory,URL.canParse > repeatedly called) reproduce unchanged on a build withsrc/reverted — they are 1e5-iteration/GC tests timing out under debug+ASAN.Test updates
test/js/node/url/url.test.ts: new coverage. 6 of its assertions fail without this diff.test-url-parse-invalid-input.js,test-url-parse-format.js: re-synced the affected blocks to node v26.3.0, which dropped the DEP0170 expectations and removed thegit+ssh://git@github.com:npm/npmparse case now that it throws.url-parse-ipv6.test.ts:[:::1]and[::banana]parse in node, so they move to the valid table. Its spot checks usedtoMatchObject(expect.objectContaining(x)), which is vacuous in Bun (it passes on a wrong value) and was hiding wrong expectations for[::F],[::1]:1and[0:0:0:1:2:3:4:5]; they now assert for real against node's values.One known divergence is left in place, with its own root cause: node's host scanner strips tab/LF/CR before splitting the host, Bun's copy predates that, so
url.parse("https://[\n::1]")still throws here.