Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 4 additions & 8 deletions src/jsc/bindings/DOMURL.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -117,14 +117,7 @@ static String applyIDNADeltaToURLAuthority(const String& urlString, StringView s
// The authority ends at the first path/query/fragment terminator;
// backslash terminates it for special schemes and never appears in a
// valid host, so treating it as a terminator is safe for both kinds.
size_t authorityEnd = view.length();
for (size_t i = authorityStart; i < view.length(); i++) {
char16_t ch = view[i];
if (ch == '/' || ch == '?' || ch == '#' || ch == '\\') {
authorityEnd = i;
break;
}
}
size_t authorityEnd = Bun::findURLHostTerminator(view, authorityStart);

// Userinfo is percent-encoded, not IDNA-mapped, in node too: only the
// host span after the last '@' gets the delta.
Expand All @@ -150,6 +143,9 @@ static String applyIDNADeltaToURLAuthority(const String& urlString, StringView s
return {};

auto mappedHost = Bun::applyUnicode16IDNADelta(hostView.toString());
// All-ignored host: splicing "" in would reparse http://\u180E/a as http:///a (host "a"); skip so the parser rejects it.
if (mappedHost.isEmpty())
return {};
StringBuilder builder;
builder.append(view.left(hostStart));
builder.append(mappedHost);
Expand Down
19 changes: 11 additions & 8 deletions src/jsc/bindings/NodeURL.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,16 @@ String applyUnicode16IDNADelta(const String& input)
return builder.toString();
}

size_t findURLHostTerminator(StringView view, size_t start)
{
for (size_t i = start; i < view.length(); i++) {
char16_t c = view[i];
if (c == '/' || c == '\\' || c == '?' || c == '#')
return i;
}
return view.length();
}

// Port of Node's icu-based ToASCII (removed in nodejs/node#55156):
// https://github.com/nodejs/node/blob/9f5000e0f2a2^/src/node_i18n.cc — filter
// the CheckHyphens/VerifyDnsLength error classes, fail otherwise unless lenient.
Expand Down Expand Up @@ -187,14 +197,7 @@ static String parseDomainAsHost(const String& rawDomain)
// The hostname setter's basic-URL parse stops at the first path, query,
// fragment, or backslash (special scheme) terminator.
StringView view { domain };
size_t end = view.length();
for (size_t i = 0; i < view.length(); i++) {
char16_t c = view[i];
if (c == '/' || c == '?' || c == '#' || c == '\\') {
end = i;
break;
}
}
size_t end = findURLHostTerminator(view);
String host = domain.left(end);
if (host.isEmpty())
return {};
Expand Down
4 changes: 4 additions & 0 deletions src/jsc/bindings/NodeURLHelpers.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,8 @@ bool containsUnicode16IDNADeltaSource(WTF::StringView view);
// the input unchanged when no delta source is present.
WTF::String applyUnicode16IDNADelta(const WTF::String& input);

// Index of the first WHATWG host-state terminator (/ \ ? #) at or after
// `start`, or view.length() when none.
Comment on lines +23 to +24

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

size_t findURLHostTerminator(WTF::StringView view, size_t start = 0);

} // namespace Bun
25 changes: 19 additions & 6 deletions src/jsc/bindings/URLDecomposition.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -121,11 +121,16 @@
// Non-special schemes and '['-prefixed (IPv6) hosts never run IDNA.
String mappedValue;
if (fullURL.hasSpecialScheme() && !value.startsWith('[')) {
size_t hostEnd = value.reverseFind(':');
auto hostSpan = hostEnd == notFound ? value : value.left(hostEnd);
size_t terminator = Bun::findURLHostTerminator(value);
size_t hostEnd = value.left(terminator).reverseFind(':');
size_t hostSpanEnd = hostEnd == notFound ? terminator : hostEnd;
auto hostSpan = value.left(hostSpanEnd);
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));

Check failure on line 133 in src/jsc/bindings/URLDecomposition.cpp

View check run for this annotation

Claude / Claude Code Review

setHost/setHostname empty-host guard bypassed by tab/CR/LF alongside IDNA-ignored code points

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 `applyIDNADeltaT
Comment on lines 128 to +133

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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", …):

  1. findURLHostTerminator("\t") → 2 (no / \ ? #), so hostSpan = "\t".
  2. containsUnicode16IDNADeltaSource(hostSpan) → true (U+180E; the string is 16-bit).
  3. applyUnicode16IDNADelta("\t")"\t" — U+180E dropped by case 0x180E: break;, \t appended by the default: arm.
  4. mappedHost.isEmpty()false. Guard skipped.
  5. mappedValue = "\t", value = "\t"; not empty; separator = notFound; fullURL.setHost("\t") is called.
  6. 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."
  7. fullURL.isValid() true (file: permits an empty host); hasAcceptableHost true (no xn--). setFullURL commits file:///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

  • findURLHostTerminator scans 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 /x sibling — the observable divergence is file:-only.
  • applyIDNADeltaToURLAuthority in DOMURL.cpp is not affected because it strips tab/CR/LF up front (the isTabOrNewline block right after the 8-bit early-return) before locating the host span, so its mappedHost.isEmpty() guard sees the post-strip span. The setters have no equivalent pre-strip.

Impact

file://server/sharefile:///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.

value = mappedValue;
}
}
Expand Down Expand Up @@ -173,9 +178,17 @@
// See setHost: the input is a hostname by definition, and only special
// schemes run IDNA on it.
String mappedHost;
if (fullURL.hasSpecialScheme() && !host.startsWith('[') && Bun::containsUnicode16IDNADeltaSource(host)) {
mappedHost = Bun::applyUnicode16IDNADelta(host.toString());
host = mappedHost;
if (fullURL.hasSpecialScheme() && !host.startsWith('[')) {
size_t terminator = Bun::findURLHostTerminator(host);
auto hostSpan = host.left(terminator);
if (Bun::containsUnicode16IDNADeltaSource(hostSpan)) {
auto mappedSpan = Bun::applyUnicode16IDNADelta(hostSpan.toString());
// See setHost: mapping a non-empty hostname to empty is failure, not "".
if (mappedSpan.isEmpty())
return;
mappedHost = makeString(mappedSpan, host.substring(terminator));
host = mappedHost;
}
}
if (host.isEmpty() && !fullURL.protocolIsFile() && fullURL.hasSpecialScheme())
return;
Expand Down
58 changes: 58 additions & 0 deletions test/js/web/url/url.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,64 @@ describe("url", () => {
expect(h2.host).toBe("xn--foo-7ka:81");
});

it("rejects special-scheme hosts made only of IDNA-ignored code points", () => {
// U+180E and U+206A..U+206F map to nothing under UTS #46, so these hosts
// map to the empty string: domain-to-ASCII failure (node throws
// ERR_INVALID_URL), never "promote the first path segment to the host".
const inputs = [
"http://\u180E/evil.example/x",
"https://\u206A\u206F/other.example/p",
"ws://\u180E/h/p",
"file://\u180E/some/dir/f",
"http://\u180E\u206B:8080/x",
"http://user@\u180E/x",
];
const errInvalidURL = expect.objectContaining({ code: "ERR_INVALID_URL" });
for (const input of inputs) {
expect(() => new URL(input)).toThrow(errInvalidURL);
expect(URL.canParse(input)).toBe(false);
expect(URL.parse(input)).toBeNull();
const u = new URL("http://ok.example/");
expect(() => (u.href = input)).toThrow(errInvalidURL);
}
// Scheme-relative input and an all-ignored base reach the same host span.
expect(() => new URL("//\u180E/evil.example/", "http://good.example/")).toThrow(errInvalidURL);
expect(URL.canParse("//\u180E/evil.example/", "http://good.example/")).toBe(false);
expect(URL.parse("//\u180E/evil.example/", "http://good.example/")).toBeNull();
expect(() => new URL("/x", "http://\u206A/base.example/")).toThrow(errInvalidURL);
expect(URL.canParse("/x", "http://\u206A/base.example/")).toBe(false);
expect(URL.parse("/x", "http://\u206A/base.example/")).toBeNull();
// Mixed hosts still strip the ignored code point rather than failing.
expect(new URL("http://a\u180Eb/").href).toBe("http://ab/");
expect(new URL("file://a\u180Eb/x").host).toBe("ab");
// Setters: a non-empty host that maps to empty is a failed host parse and
// no-ops, even for file: where assigning a literal "" clears the host.
const f1 = new URL("file://server/share");
f1.host = "\u180E";
expect(f1.href).toBe("file://server/share");
const f2 = new URL("file://server/share");
f2.hostname = "\u180E";
expect(f2.href).toBe("file://server/share");
const f3 = new URL("file://server/share");
f3.host = "";
expect(f3.href).toBe("file:///share");
Comment thread
coderabbitai[bot] marked this conversation as resolved.
const f4 = new URL("file://server/share");
f4.hostname = "";
expect(f4.href).toBe("file:///share");
// A terminator after the ignored code points must not smuggle the tail
// past the empty-host guard: the host span ends at the first / \ ? #.
for (const tail of ["/x", "\\x", "?x", "#x"]) {
for (const base of ["file://server/share", "http://ok.example/p"]) {
const withHost = new URL(base);
withHost.host = "\u180E" + tail;
expect(withHost.href).toBe(base);
const withHostname = new URL(base);
withHostname.hostname = "\u180E" + tail;
expect(withHostname.href).toBe(base);
}
}
});

it("prints", () => {
// URL.prototype carries [Symbol.for("nodejs.util.inspect.custom")], so
// Bun.inspect matches node's util.inspect output.
Expand Down