Skip to content
Closed
Show file tree
Hide file tree
Changes from 5 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
3 changes: 3 additions & 0 deletions src/jsc/bindings/DOMURL.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,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
37 changes: 31 additions & 6 deletions src/jsc/bindings/URLDecomposition.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,18 @@ static unsigned countASCIIDigits(StringView string)
return length;
}

// The WHATWG host/hostname states stop consuming at the first of these; the
// IDNA delta must not touch anything at or past it (see DOMURL.cpp).
Comment thread
robobun marked this conversation as resolved.
Outdated
static size_t findHostTerminator(StringView value)
{
for (size_t i = 0; i < value.length(); i++) {
char16_t c = value[i];
if (c == '/' || c == '\\' || c == '?' || c == '#')
return i;
}
return value.length();
}
Comment thread
robobun marked this conversation as resolved.
Outdated

void URLDecomposition::setHost(StringView value)
{
auto fullURL = this->fullURL();
Expand All @@ -121,11 +133,16 @@ void URLDecomposition::setHost(StringView value)
// 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 = findHostTerminator(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;
Comment thread
robobun marked this conversation as resolved.
mappedValue = makeString(mappedHost, value.substring(hostSpanEnd));
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 +190,17 @@ void URLDecomposition::setHostname(StringView host)
// 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 = findHostTerminator(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
52 changes: 52 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,58 @@ 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(() => new URL("/x", "http://\u206A/base.example/")).toThrow(errInvalidURL);
// 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.
// 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
Loading