url: reject invalid Punycode (xn--) labels in special-scheme hosts - #33201
url: reject invalid Punycode (xn--) labels in special-scheme hosts#33201robobun wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 10 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 (5)
WalkthroughAdds Bun IDNA helpers for ACE-label detection, domain-to-ASCII conversion, and URL host validation. DOMURL now uses the new host check for URL validity, NodeURL routes domain-to-ASCII through the helper, and tests cover invalid and valid punycode labels. ChangesIDNA validation feature
Related PRs: None specified. Suggested labels: Suggested reviewers: None specified. 🐰
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:59 PM PT - Jul 7th, 2026
❌ @robobun, your commit 696eb97 has some failures in 🧪 To try this PR locally: bunx bun-pr 33201That installs a local version of the PR into your bun-33201 --bun |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/jsc/bindings/DOMURL.cpp (1)
82-88: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate the
URLoverload's base before resolving absolute inputs.Line 86 only checks
completeURL. Whenurlis absolute,URL completeURL { base, url }ignoresbase, so this overload can still accept a base likehttp://xn--a-ecp.example/even though Line 94 rejects the same base in the string overload. Please reject!isValidCompleteURL(base)first, otherwise this remains a bypass for the new IDNA validation.Suggested fix
ExceptionOr<Ref<DOMURL>> DOMURL::create(const String& url, const URL& base) { ASSERT(base.isValid() || base.isNull()); + if (!base.isNull() && !isValidCompleteURL(base)) + return Exception { InvalidURLError, makeString(redact(url), " cannot be parsed as a URL against "_s, redact(base.string())) }; URL completeURL { base, url }; if (!isValidCompleteURL(completeURL)) return Exception { InvalidURLError, makeString(redact(url), " cannot be parsed as a URL."_s) }; return adoptRef(*new DOMURL(WTF::move(completeURL))); }🤖 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 `@src/jsc/bindings/DOMURL.cpp` around lines 82 - 88, In DOMURL::create(const String& url, const URL& base), validate the base URL with isValidCompleteURL(base) before constructing completeURL, since absolute inputs can bypass the base check by ignoring it during resolution. Add the rejection path near the existing ASSERT and keep the current completeURL validation afterward so both the base and resolved URL are enforced consistently with the string overload.
🤖 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 `@src/jsc/bindings/DOMURL.cpp`:
- Around line 82-88: In DOMURL::create(const String& url, const URL& base),
validate the base URL with isValidCompleteURL(base) before constructing
completeURL, since absolute inputs can bypass the base check by ignoring it
during resolution. Add the rejection path near the existing ASSERT and keep the
current completeURL validation afterward so both the base and resolved URL are
enforced consistently with the string overload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2410dd0f-ed16-4211-8de7-8d5107b75383
📒 Files selected for processing (6)
src/jsc/bindings/BunIDNA.cppsrc/jsc/bindings/BunIDNA.hsrc/jsc/bindings/DOMURL.cppsrc/jsc/bindings/NodeURL.cpptest/js/node/url/url-domain-ascii-unicode.test.jstest/js/web/url/url.test.ts
b920f2f to
3c2b2fc
Compare
|
Pushed 3c2b2fc: rebased onto main, with autofix.ci's formatting change included. On the finding about The exact scenario described, an absolute input against a bad base, is covered by a test in this PR and throws: expect(() => new URL("http://ok.example/", "http://xn--a-ecp.example/")).toThrow("cannot be parsed as a URL");Rather than add an unreachable runtime check, I strengthened the existing precondition assert from |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Both earlier nits are addressed and I found nothing further, but this is a security-relevant behavioral change to new URL() host parsing that intentionally leaves fetch/Request on the old behavior — worth a human sign-off on that trade-off before merging.
Extended reasoning...
Overview
Adds src/jsc/bindings/BunIDNA.{h,cpp} with three helpers (domainHasACELabel, domainToASCII, urlHostIsValidIDNA) and wires them into DOMURL::create/parse/canParse/setHref, URLDecomposition::origin()'s blob inner-URL branch, and jsDomainToASCII in NodeURL.cpp. Net effect: special-scheme URLs whose host contains an invalid Punycode (xn--) label are now rejected, matching Node and the URL Standard. ~90 lines of new C++ plus ~120 lines of tests across two files.
Security risks
The change is about security — it closes a host-parser differential usable to bypass URL allowlists / SSRF filters that validate with Bun before a conformant parser fetches. The direction is strictly tightening (reject more), so the new code itself introduces no obvious new attack surface. The one subtlety is that the PR knowingly leaves WTF::URL (and therefore fetch/Request) on the permissive path, so within Bun URL.canParse(x) === false while fetch(x) still resolves the host — an intra-runtime inconsistency the description defers to an oven-sh/WebKit change.
Level of scrutiny
High. This is production URL host parsing, explicitly security-relevant, and a user-visible behavioral change to a core Web API (new URL() now throws on inputs it previously accepted). IDNA/UTS-46 semantics are subtle enough that a maintainer should confirm the ICU option set, the allowedNameToASCIIErrors mask reuse, and the decision to ship a partial fix ahead of the WebKit-side change.
Other factors
My two prior inline comments (ICU U_BUFFER_OVERFLOW_ERROR retry; blob: origin sibling site) were both fixed in 4b55c3e with tests, and the current bug-hunt pass found nothing. I verified the host/hostname setter no-op claim: DOMURL::setFullURL at DOMURL.h:78 routes through setHref, whose ExceptionOr is discarded, yielding the spec-required silent no-op. Test coverage is thorough (constructor/canParse/parse/setters/base-URL/blob-origin/non-special-scheme/overflow-retry, plus valid-ACE and non-ACE negative controls). The code looks correct to me; deferring purely because security-sensitive URL-parsing behavioral changes warrant a human reviewer per policy.
|
Thanks for the review. Both earlier findings are fixed in 4b55c3e, and I agree the
CI status (build 70038, final)284 jobs passed. The only failures are both shards of A retrigger cannot fix that agent, so I have not pushed one. This is ready to merge with that lane overridden or retried. |
|
Heads up on an overlap: #33206 rewrites The |
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.
4b55c3e to
696eb97
Compare
|
Closing: this behaviour landed on main through #34660 (merged Aug 7), which rejects invalid Checked by applying this PR's test additions to current main (5c050bc, ICU 78.3): the The cases from here that main's tests did not already have (the decodable but disallowed |
…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
Bun accepts hosts with invalid Punycode labels that Node and the URL Standard's host parser reject:
xn--a-ecpdecodes toU+0061 U+2488(afollowed by DIGIT ONE FULL STOP), which UTS-46 disallows in a domain label, so the spec's domain to ASCII step must fail. The same divergence exists inURL.canParse,URL.parse, thehref/host/hostnamesetters,.originon ablob:URL (Node gives"null"), andurl.domainToASCII(which returns the input verbatim instead of the documented"").This is a parser differential on the host, the security-relevant component of a URL: anything that validates a URL with Bun's
URLclass before a Node service or a browser fetches it (allowlists, SSRF filters, link sanitizers, webhook validators) disagrees with every conformant parser about whether the host is even valid.Cause
WTF::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.jsDomainToASCIIinNodeURL.cpphas the same early return copied from it.Fix
New
src/jsc/bindings/BunIDNA.{h,cpp}:Bun::domainHasACELabel(StringView): does any dot-separated label start withxn--Bun::domainToASCII(StringView): UTS-46 domain to ASCII with the same ICU transcoder and allowed-error mask thatURLParser::domainToASCIIuses; nullStringon failureBun::urlHostIsValidIDNA(const URL&): for a parsed special-scheme URL whose host contains an ACE label, re-rundomainToASCIIand fail if it rejectsDOMURL::create,parseInternal, andsetHrefnow requireurlHostIsValidIDNAin addition toisValid(). Thehref,host, andhostnamesetters all funnel throughDOMURL::setHrefviaURLDecomposition::setFullURL, so they are covered by the same check:hrefthrows, andhost/hostnamebecome the silent no-op the spec requires.URLDecomposition::origin()applies the same check to the inner URL it re-parses out of ablob:URL's path, so an invalid inner host yields the opaque ("null") origin.jsDomainToASCIIgates its fast path on!domainHasACELabeland routes ACE labels through the sharedBun::domainToASCII.Only hosts that actually contain an
xn--label reach ICU. For every other all-ASCII host, UTS-46 with the URL spec's options is equivalent to the lowercasing the fast path already does, so nothing else changes. Opaque hosts of non-special schemes are untouched, matching Node.Bun::domainToASCIIretries once with a heap buffer when ICU reports a buffer overflow, so a spec-valid host longer than ICU's 2048-code-unit stack buffer is not rejected.Not covered
The fast path itself lives in WebKit's
URLParser.cpp(vendor/WebKit, shipped prebuilt), soRequestandfetch, which build aWTF::URLdirectly, still accept these hosts. Fixing that belongs in oven-sh/WebKit. This PR covers theURLclass andnode:url.Verification
A 66-case matrix over the constructor,
canParse,parse, the setters, anddomainToASCII(invalid ACE labels, valid ACE labels likexn--bcher-kva.deandxn--e1afmkfd.xn--p1ai, case and percent-encoding variants, non-ACE hosts with the same hyphen patterns, IPs, opaque hosts) produces byte-identical output from this build and from Node 26. The two test files fail 21 of their assertions under the released Bun.