url: reject special-scheme hosts made only of IDNA-ignored code points - #37167
url: reject special-scheme hosts made only of IDNA-ignored code points#37167robobun wants to merge 8 commits into
Conversation
A host consisting solely of IDNA-ignored code points (U+180E,
U+206A..U+206F) maps to the empty string under the Unicode 16 IDNA
delta. Splicing that empty host back into the input before parsing let
the special-authority-ignore-slashes state take the first path segment
as the host: new URL("http://\u180E/evil.example/x") parsed with host
"evil.example" where node and previous bun throw ERR_INVALID_URL.
An empty domain-to-ASCII result is parse failure, so leave the input
untouched and let the parser reject the original code points.
The host/hostname setters had the same conflation for file: URLs:
assigning a host that maps to empty cleared the host instead of
no-opping like a failed host parse (a literal empty string still
clears it, matching node).
WalkthroughThe URL implementation limits IDNA mapping to the host portion, preserves URL suffixes, and rejects hosts that map to an empty string. Tests cover constructors, parsing APIs, setters, special URLs, mixed hosts, and file hosts. ChangesURL host validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, targeted guard that closes a host-confusion regression from the IDNA delta pre-scanner. The one inline nit (bare .toThrow()) is non-blocking; the adjacent canParse/parse assertions already pin the behavior.
What was reviewed:
- Verified the
mappedHost.isEmpty()guard is only reachable whencontainsUnicode16IDNADeltaSource(hostView)was true, so the host span is provably non-empty — the check correctly means "non-empty input mapped to empty". - Confirmed returning
{}fromapplyIDNADeltaToURLAuthorityfalls back to the original input (mapped.isNull() ? url : mapped), so the parser sees the raw disallowed code points and rejects. - Checked the setter early-returns match WHATWG failed-host-parse semantics (no-op) and don't regress the literal
""file-host clear (that path never enters the delta branch).
Extended reasoning...
Overview
Three identical guards added across the IDNA-delta pre-scanner paths: applyIDNADeltaToURLAuthority in DOMURL.cpp (used by the URL constructor, canParse, parse, and href setter) and the setHost/setHostname setters in URLDecomposition.cpp. Each detects the case where a non-empty host span maps to the empty string under the Unicode 16 IDNA delta (i.e., the host was made entirely of ignored-class code points like U+180E or U+206A..U+206F) and skips the rewrite so the underlying parser rejects the original input instead of promoting the first path segment to the host. A comprehensive test block is added to test/js/web/url/url.test.ts.
Security risks
This fixes a security-relevant regression rather than introducing one. The unfixed behavior — new URL("http:///evil.example/x") parsing with host evil.example — is the validator/consumer-disagreement shape that enables SSRF and origin-check bypass when Bun's URL parser disagrees with node/browsers. The fix restores agreement with node (ada) and the WHATWG spec (empty domain-to-ASCII result is parse failure). The guards themselves cannot introduce new host confusion: they only decline to rewrite, falling back to the pre-#34660 behavior of letting ICU reject the raw code points.
Level of scrutiny
Medium-high because URL host parsing is security-adjacent, but the actual change is mechanically simple: three if (mappedHost.isEmpty()) return ...; lines, each placed immediately after the mapping call and guarded by a prior containsUnicode16IDNADeltaSource check that guarantees the input span was non-empty. I traced each return: in DOMURL.cpp a null return means "use original input" (all callers do mapped.isNull() ? url : mapped); in the setters an early return is the standard WHATWG no-op for a failed host parse.
Other factors
- The test covers the full variant matrix named in REVIEW.md: constructor/
canParse/parse/hrefsetter, http/https/ws/file, userinfo, port, base-URL forms, bothhostandhostnamesetters, plus the negative case (mixedabstill maps toab) and the file: literal-empty-string clear still working. - The guard placement is consistent across all three sites — the "fix the whole class" rule is satisfied.
- The one inline finding is a test-style nit (bare
.toThrow()vs assertingERR_INVALID_URL); the neighboring pre-existing test in the same file uses the same bare pattern, and thecanParse === false/parse === nullassertions on identical inputs already discriminate the fixed vs unfixed behavior via non-throwing entry points. Not blocking.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
The host/hostname setters applied the Unicode 16 IDNA delta to the whole
input, so an all-ignored host followed by a terminator ("\u180E/x")
mapped to a non-empty string, bypassed the empty-host guard, and reached
the parser: on file: URLs it cleared the host and on http it promoted
the first path segment, where node treats the pre-terminator span as a
failed host parse and no-ops. Bound the delta (and the empty-host check)
to the span before the first / \ ? #, matching the WHATWG host state
and the constructor path in DOMURL.cpp.
![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
🤖 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/web/url/url.test.ts`:
- Around line 184-201: Extend the URL tests around the existing scheme-relative
and invalid-base cases to cover URL.parse for the invalid host, plus
URL.canParse and URL.parse when the base contains the invalid code point. Add a
separate file: hostname = "" assertion using URLDecomposition::setHostname
behavior, confirming the host clears to an empty value while preserving the
existing host setter cases.
🪄 Autofix
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: 2cd45237-01f4-4228-82ae-50f7ce0af573
📒 Files selected for processing (3)
src/jsc/bindings/DOMURL.cppsrc/jsc/bindings/URLDecomposition.cpptest/js/web/url/url.test.ts
|
Updated 7:42 PM PT - Aug 7th, 2026
✅ @robobun, your commit 3c1dd0fcb6e4d8ee3bc3acdd51252e994e101a6d passed in 🧪 To try this PR locally: bunx bun-pr 37167That installs a local version of the PR into your bun-37167 --bun |
c1b8511 to
0b20ff5
Compare
| // Index of the first WHATWG host-state terminator (/ \ ? #) at or after | ||
| // `start`, or view.length() when none. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
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/URLDecomposition.cpp (1)
124-133: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBound port detection to the host span.
After this block preserves the suffix, the later
value.reverseFind(':')at Line 140 also scans the path, query, and fragment. Forhost\u180E:80/path:x, it selects the suffix colon and can lose the valid80port.Recompute the separator from the value before the first host terminator.
Proposed fix
- size_t separator = value.reverseFind(':'); + size_t separator = value.left(Bun::findURLHostTerminator(value)).reverseFind(':');🤖 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/URLDecomposition.cpp` around lines 124 - 133, Update the later port-separator lookup in the URL decomposition flow to search only within the substring ending at the first host terminator, rather than calling value.reverseFind(':') across the path, query, and fragment. Recompute the separator from that bounded host value so inputs like host\u180E:80/path:x preserve the valid port.
🤖 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/URLDecomposition.cpp`:
- Around line 124-133: Update the later port-separator lookup in the URL
decomposition flow to search only within the substring ending at the first host
terminator, rather than calling value.reverseFind(':') across the path, query,
and fragment. Recompute the separator from that bounded host value so inputs
like host\u180E:80/path:x preserve the valid port.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54d1a8a9-225b-45da-a752-7828a1bf2f1c
📒 Files selected for processing (4)
src/jsc/bindings/DOMURL.cppsrc/jsc/bindings/NodeURL.cppsrc/jsc/bindings/NodeURLHelpers.hsrc/jsc/bindings/URLDecomposition.cpp
| if (Bun::containsUnicode16IDNADeltaSource(hostSpan)) { | ||
| auto mappedHost = Bun::applyUnicode16IDNADelta(hostSpan.toString()); | ||
| mappedValue = hostEnd == notFound ? mappedHost : makeString(mappedHost, value.substring(hostEnd)); | ||
| // A host mapping to empty is a failed host parse, not an assignable literal "". | ||
| if (mappedHost.isEmpty()) | ||
| return; | ||
| mappedValue = makeString(mappedHost, value.substring(hostSpanEnd)); |
There was a problem hiding this comment.
🔴 The mappedHost.isEmpty() guard is checked before the WHATWG parser's tab/newline strip, so "\t" maps to "\t" (non-empty), bypasses the guard, and fullURL.setHost("\t") on file://server/share then strips it to "" and commits file:///share where Node no-ops. Same #34660 regression class as the already-fixed / \ ? # terminator smuggle; affects both setHost here and setHostname at :185–188. Fix: strip \t/\n/\r from the host span before the delta (mirroring applyIDNADeltaToURLAuthority in DOMURL.cpp, which does this up front) or test emptiness post-strip, and add "\t"/"\n"/"\r" variants to the terminator loop in the new test.
Extended reasoning...
What
URLDecomposition::setHost and setHostname bound the IDNA-delta host span at the first / \ ? # (via findURLHostTerminator) and then early-return when the delta-mapped span is empty. But the WHATWG basic URL parser also strips ASCII tab/newline from its input unconditionally, before the state machine runs (step 3), and applyUnicode16IDNADelta passes U+0009/U+000A/U+000D through verbatim via the default: arm (NodeURL.cpp:118-120). So a host span consisting only of IDNA-ignored code points plus tab/CR/LF maps to a non-empty string of just tab/CR/LF, walks past the isEmpty() guard, and reaches fullURL.setHost(...) — where the parser then strips it to the empty string.
Step-by-step proof
With const f = new URL("file://server/share"); f.host = "\t"; (also "\n", "\t", "\r/x", …):
findURLHostTerminator("\t")→ 2 (no/ \ ? #), sohostSpan = "\t".containsUnicode16IDNADeltaSource(hostSpan)→ true (U+180E; the string is 16-bit).applyUnicode16IDNADelta("\t")→"\t"— U+180E dropped bycase 0x180E: break;,\tappended by thedefault:arm.mappedHost.isEmpty()→ false. Guard skipped.mappedValue = "\t",value = "\t"; not empty;separator = notFound;fullURL.setHost("\t")is called.- WHATWG basic URL parser step 3 strips tab/newline → input
""; state override = host state; url's scheme =file→ redirect to file host state; EOF with empty buffer → "set url's host to the empty string, and if state override is given, return." fullURL.isValid()true (file:permits an empty host);hasAcceptableHosttrue (noxn--).setFullURLcommitsfile:///share.
Node v26 on the same input: basic URL parser strips \t first → input ""; file host state buffers U+180E; EOF; host-parse of "" → domain-to-ASCII (ada) maps to "" → failure → return. Setter no-ops; f.href stays "file://server/share".
Pre-#34660 Bun also no-opped: there was no delta pre-scan, setHost("\t") reached the parser directly, and bundled ICU rejects U+180E as disallowed. So this is the same regression class the PR is titled after.
setHostname (:181–191) is affected identically — same terminator scan, same delta, same isEmpty() guard, same fullURL.setHost(...) sink.
Why the existing guards don't catch it
findURLHostTerminatorscans only for/ \ ? #; tab/CR/LF stay inside the host span.mappedHost.isEmpty()sees"\t", not"".- The trailing
fullURL.isValid() && hasAcceptableHost(fullURL)gate saves the non-file special schemes (an empty http/https/ws/wss/ftp host is invalid), so — as with the earlier/xsibling — the observable divergence isfile:-only. applyIDNADeltaToURLAuthorityin DOMURL.cpp is not affected because it strips tab/CR/LF up front (theisTabOrNewlineblock right after the 8-bit early-return) before locating the host span, so itsmappedHost.isEmpty()guard sees the post-strip span. The setters have no equivalent pre-strip.
Impact
file://server/share → file:///share is the exact path-integrity divergence the PR's own setter test guards against for the four / \ ? # terminators; tab/CR/LF is the remaining sibling. This is the direct twin of the finding fixed in ec82ee6 during this review, and REVIEW.md is explicit: "Fix the whole class in the same PR (same-class sites are ONE concern, not scope creep)."
Fix
Either strip tab/CR/LF from hostSpan before applying the delta (mirroring DOMURL.cpp — the two setters and DOMURL.cpp already share NodeURLHelpers.h, so the isTabOrNewline predicate could live there too), or test mappedHost.removeCharacters(isASCIITabOrNewline).isEmpty() in place of mappedHost.isEmpty(). In the new test's terminator loop, add bare "\t" / "\n" / "\r" and one combined variant (e.g. "\t/x") alongside the existing four tails.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
we're gonna delete all this IDNA handling and simply upgrade libicu
|
Sounds good, closing this in favor of the ICU upgrade. If useful, the test block added here ( |
2 participants
Repro
URL.canParsereturned true andURL.parsenon-null for each; thehrefsetter behaved the same. A string every WHATWG parser (node, browsers, previous bun) rejects instead parsed with a host taken from the path, which is the validator/consumer-disagreement shape. Regression from the Unicode 16 IDNA delta pre-scanner in #34660.Cause
applyIDNADeltaToURLAuthority(DOMURL.cpp) maps the host span through the Unicode 16 IDNA delta, under which U+180E and U+206A..U+206F are ignored (deleted). A host made only of those code points maps to the empty string, and splicing that empty host back into the input rebuilthttp://\u180E/evil.example/xashttp:///evil.example/x. The special-authority-ignore-slashes state then consumes the slash run and takesevil.exampleas the host. Per the URL spec an empty domain-to-ASCII result is parse failure, which is what node (ada) does.Fix
When the mapped host is empty while the original host span was not, skip the rewrite and leave the input untouched. The parser then rejects the original code points (bundled ICU treats them as disallowed), producing
ERR_INVALID_URLexactly as before the pre-scanner.The host/hostname setters (URLDecomposition.cpp) had the same conflation: assigning
"\u180E"mapped to""and cleared afile:host, where node no-ops like any failed host parse. The setters also applied the delta to the whole input, so a terminator after the ignored code points (u.host = "\u180E/x") smuggled a non-empty string past the empty check and cleared afile:host or promoted the first path segment on http. Both setters now bound the delta span (and the empty-host check) at the first/ \ ? #, matching the WHATWG host state and the constructor path. Assigning a literal empty string still clears a file host, matching node.Verification
test/js/web/url/url.test.tsfails on main, passes with the fix: constructor,canParse,parse, andhrefsetter for http/https/ws/file plus userinfo/port and base-URL variants, the setter no-ops (bare and with each of the four terminators, on file: and http), and mixed hosts (a\u180Eb->ab) still mapping.test/js/web/url/,test/js/node/url/, and the WPT runners (test-whatwg-url-custom-parsing,custom-setters,toascii,canparse,custom-domainto,custom-href-side-effect) pass. The one exception isurl-canParse-whatwg.test.js"repeatedly called produces same result", which loopsURL.canParse100k times and exceeds its 5s timeout under a debug+ASAN build regardless of this change (the all-ASCII input returns from the delta scanner on its first line).[review] gate passed · iteration 0 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 0
evidence per changed file