[robobun]

Fixes #28661

WPT-sourced audit of Bun's URL and URLPattern against the WHATWG URL and URLPattern standards. With this change, Bun's results on the full WPT url/resources/urltestdata.json (891 cases) and setters_tests.json (278 cases) suites are identical to Node's: the only remaining divergences from the WPT fixtures are the 14 xn-- punycode assertions that Node (Ada) also fails and that main now intentionally fails too since #34660 (those come from the constructor's and setters' UTS #46 host validation, which this PR preserves; see the rebase note below). Against main's own source the same runs fail 15 and 11 assertions, and the delta is exactly the cases fixed here. Bun also passes the WPT urlpatterntestdata.json suite (refreshed here to 369 entries).

URL host / hostname / port setters (URLDecomposition.cpp)

Per the spec, the setters run the basic URL parser with a state override (https://url.spec.whatwg.org/#api). URLDecomposition::setHost instead split the value at the last : and handed the pieces to URL::setHostAndPort, which diverges in four ways:

// 1. A port equal to the scheme's default must be cleared (#28661)
u = new URL("http://x:8080"); u.host = "example.com:80";
u.href // got "http://example.com:8080/", want "http://example.com/"

// 2. An out-of-range port fails the port state, but the host was already committed
u = new URL("http://x:8080"); u.host = "newhost:99999";
u.href // got "http://newhost/", want "http://newhost:8080/"

// 3. The file host state never splits off a port, and ":" is a forbidden host code point
u = new URL("file:///a/b"); u.host = "h2:x";
u.href // got "file://h2/a/b", want "file:///a/b"

// 4. Only the empty string clears the port. The parser removes ASCII tab/newline,
//    leaving the port state with an empty buffer, which fails without touching the port
u = new URL("http://h:123/"); u.port = "\t\n";
u.href // got "http://h/", want "http://h:123/"

// 5. The hostname state fails on any ":" outside an IPv6 literal
u = new URL("http://example.net/"); u.hostname = "[::1]:80";
u.href // got "http://[::1]/", want "http://example.net/"

// 6. The parser strips ASCII tab/newline before the host state runs, so a host part that
//    is only tab/newline is the same empty host as a bare ":" (found in review)
u = new URL("foo://x/"); u.host = "\t:";
u.href // got "foo:///", want "foo://x/"

1 and 4 are verbatim failures in WPT's setters_tests.json; the rest are gaps in that suite. Every expected value above, plus the 27-entry table added to test/js/web/url/url.test.ts, was verified against Node 26.3 (Ada), which agrees on all of them.

setHost now scans for the first : outside brackets and before a terminator (the host state's transition to the port state), commits the host via URL::setHost, and then runs the existing parsePort on the port part. parsePort reports an empty digit buffer as a failure, which is what the port state does once a state override is given.

One intentional side effect: parsePort's other caller is URLPatternCanonical::canonicalizePort, so new URLPattern({ port: "\t\n" }) now throws a TypeError like { port: "abc" } already did (both hit the same port-state rule). Node returns "" for the former and throws for the latter, which is an Ada inconsistency; WPT doesn't cover it.

URL.prototype.origin (URLDecomposition.cpp)

7 verbatim failures in WPT's urltestdata.json. origin() treated ftps as a special scheme and gave blob: URLs with an inner ftp/ws/wss/file URL a tuple origin. Per https://url.spec.whatwg.org/#concept-url-origin only ftp/http/https/ws/wss have a tuple origin directly, and for blob: only an inner http(s) URL does. Everything else serializes as "null".

new URL("ftps://host/path").origin  // got "ftps://host", want "null"
new URL("blob:ws://h/").origin      // got "ws://h",      want "null"
new URL("blob:file:///x").origin    // got "file://",     want "null"

The blob: expectations in the existing it("blob urls") test were asserting the previous non-spec values (they were written alongside the implementation in #5825) and are updated to the values WPT, Node, and browsers agree on.

URLPattern.prototype.exec group leakage (URLPatternComponent.cpp)

URLPatternComponent::componentMatch iterated every capture group of the compiled RegExp and fell back to an empty-string group name once it ran past the component's group name list. A part's user-supplied regexp can contain its own named groups:

new URLPattern({ pathname: "/:foo((?<x>a))" }).exec({ pathname: "/a" }).pathname.groups
// got { foo: "a", "": "a" }, want { foo: "a" }

Clamp the loop to the group name list, matching upstream WebKit's createComponentMatchResult and https://urlpattern.spec.whatwg.org/#create-a-component-match-result. test/js/web/urlpattern/urlpatterntestdata.json is refreshed from WPT master (349 to 369 entries); the two new nested-group entries cover exactly this, and the other 18 new entries already pass.

Verification

# bun bd against main's URLDecomposition.cpp / URLDecomposition.h / URLPatternComponent.cpp
$ bun bd test test/js/web/url/url.test.ts test/js/web/urlpattern/urlpattern.test.ts
458 pass, 12 fail   # every failure is one of the cases above

# with this change (counts as of the latest rebase; upstream has since added tests to these files)
$ bun bd test test/js/web/url/url.test.ts test/js/web/urlpattern/urlpattern.test.ts
481 pass, 0 fail

test/js/node/test/parallel/test-whatwg-url-{toascii,custom-setters,override-hostname,custom-href-side-effect,custom-properties,canparse,custom-parsing}.js, test/js/web/url/, test/js/web/urlpattern/, test/js/node/url/, and test/js/web/html/URLSearchParams.test.ts all pass with the fix (1574 pass in the directory sweep). The one remaining failure in test/js/node/url/ under bun bd (URL.canParse > repeatedly called produces same result, a debug+ASAN timing test) reproduces identically with main's source.

Rebase note: punycode host validation

While this PR was open, #34660 and #38013 added a UTS #46 check to the old setHost/setHostname (hasAcceptableHost, so that url.host = "xn--<invalid>" is a silent no-op on special schemes, matching the constructor and Node). The rewrite here deletes the code those commits patched, so the check is re-applied by hand: every path that commits a new host in setHost/setHostname now goes through a small setHostChecked() helper that runs URL::setHost followed by hasAcceptableHost. That is five commit sites (the old code had two), so the check also covers the file: and host-with-port paths. Verified by test-whatwg-url-toascii.js, which assigns every WPT toascii input through both setters, plus a direct probe of all five paths with an invalid and a valid xn-- label.

Not fixed here: two WTF::URLParser bugs

The same audit turned up two parser (not setter) bugs, but they live in the prebuilt WebKit, so they need an oven-sh/WebKit change plus a WEBKIT_VERSION bump rather than anything in this repo. Recording them with exact locations so they are not lost:

1. new URL("sc://u@h?x").pathname is "/" instead of "". URLParser.cpp's State::Host branch (around line 1502 at cd821fec) appends '/' on ?/# with no m_urlIsSpecial guard; the sibling State::AuthorityOrHost branch, reached when there is no @, has the guard. So new URL("sc://h?x").pathname is already correct.
2. new URL("sc:/a/..\\x").href is "sc:/x" instead of "sc:/a/..\\x". isSingleDotPathSegment/isDoubleDotPathSegment call isSlashQuestionOrHash, whose characterClassTable entry for '\\' is unconditional, so ..\ is misdetected as a double-dot segment for non-special schemes. The main State::Path slash check is correctly guarded, which is why the plain non-special://host/a\b cases in WPT's urltestdata.json pass and the suite never catches this.

Not a bug, recorded so it is not re-filed: new URL("?a#b", "data:,x") throws in Bun, and that is correct. The URL Standard's no-scheme state only allows # against an opaque-path base (https://url.spec.whatwg.org/#no-scheme-state step 1). Node accepts "?a#b" but rejects "?a" against the same base, which is an Ada inconsistency, not something to copy.

Supersedes #28662

#28662 fixes only case 1 by delegating the whole value to URL::setHostAndPort, but that function has cases 2 and 3 baked in (it rebuilds the string from pathStart(), dropping the existing port when the new port is invalid, and splits file hosts at :), so it cannot cover the class. Closing it in favor of this.

---

[review] gate passed · iteration 11 · 6 files touched

fails on main (without fix)

ASAN without fix: 13 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/web/url/url.test.ts test/js/web/urlpattern/urlpattern.test.ts
bun test v1.4.0 (8326d1bd3)

test/js/web/url/url.test.ts:
(pass) url > URL throws [12.36ms]
(pass) url > ERR_INVALID_URL carries input and, when given, base [9.19ms]
67 |     expect(url.protocol).toBe("ftp:");
68 |     expect(url.origin).toBe("ftp://example.com");
69 |     // "ftps" is not a special scheme, so its origin is opaque.
70 |     url = new URL("ftps://example.com");
71 |     expect(url.protocol).toBe("ftps:");
72 |     expect(url.origin).toBe("null");
                            ^
error: expect(received).toBe(expected)

Expected: "null"
Received: "ftps://example.com"

      at <anonymous> (/workspace/bun/test/js/web/url/url.test.ts:72:24)
(fail) url > should have correct origin and protocol [10.41ms]
107 |     url = new URL("blob:http://example.com/x");
108 |     expect(url.protocol).toBe("blob:");
109 |     expect(url.origin).toBe("http://example.com");
110 |     url = new URL("blob:file://text.txt");
111 |     expect(url.protocol).toBe("blob:");
112 |   
... (truncated)

release without fix: 1 FAILED
bun test v1.4.0-canary.1 (42203c3ce)

test/js/web/url/url.test.ts:
(pass) url > URL throws [0.13ms]
(pass) url > ERR_INVALID_URL carries input and, when given, base [0.12ms]
(pass) url > should have correct origin and protocol [0.13ms]
(pass) url > blob urls [0.07ms]
(pass) url > leaves opaque (non-special-scheme) hosts unchanged [0.08ms]
(pass) url > special-scheme hosts use the Unicode 16 IDNA table [0.10ms]
(pass) url > rejects invalid punycode labels however they are spelled in the input (like Node) [0.23ms]
(pass) url > prints [1.90ms]
(pass) url > URLContext offsets account for the /. pathname guard [0.68ms]
(pass) url > works [0.16ms]
(pass) url > URL.canParse > URL.canParse(undefined, undefined) [0.02ms]
(pass) url > URL.canParse > URL.canParse(a:b, undefined)
(pass) url > URL.canParse > URL.canParse(undefined, a:b)
(pass) url > URL.canParse > URL.canParse(a:/b, undefined)
(pass) url > URL.canParse > URL.canParse(undefined, a:/b)
(pass) url > URL.canParse > URL.canParse(https://test:test, undefined)
(pass) url > URL.canParse > URL.canParse(a, https://b/)
(pass) url > URL.canParse > URL.canParse.length should be 1 [0.01ms]
(pass) url > URLSearchParams constru
... (truncated)

passes on PR (with fix)

ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/web/url/url.test.ts test/js/web/urlpattern/urlpattern.test.ts
bun test v1.4.0 (8326d1bd3)

test/js/web/url/url.test.ts:
(pass) url > URL throws [11.78ms]
(pass) url > ERR_INVALID_URL carries input and, when given, base [9.45ms]
(pass) url > should have correct origin and protocol [13.13ms]
(pass) url > blob urls [9.26ms]
(pass) url > leaves opaque (non-special-scheme) hosts unchanged [7.45ms]
(pass) url > special-scheme hosts use the Unicode 16 IDNA table [4.72ms]
(pass) url > rejects invalid punycode labels however they are spelled in the input (like Node) [21.75ms]
(pass) url > prints [97.80ms]
(pass) url > URLContext offsets account for the /. pathname guard [45.21ms]
(pass) url > works [12.43ms]
(pass) url > URL.canParse > URL.canParse(undefined, undefined) [1.41ms]
(pass) url > URL.canParse > URL.canParse(a:b, undefined) [0.38ms]
(pass) url > URL.canParse > URL.canParse(undefined, a:b) [0.27ms]
(pass) url > URL.canParse > URL.canParse(a:/b, undefined) [0.59ms]
(pass) url > URL.canParse > URL.canParse(undefined, a:/b) [0.31
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 632ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/17] cxx obj/unified/UnifiedSource-src_jsc_bindings-5.cpp.o
[2/17] cxx obj/src/jsc/bindings/BunObject.cpp.o
[3/17] cxx obj/unified/UnifiedSource-src_jsc_modules-0.cpp.o
[4/17] cxx obj/unified/UnifiedSource-src_jsc_bindings_webcore-3.cpp.o
[5/17] cxx obj/src/jsc/bindings/bindings.cpp.o
[6/17] cxx obj/unified/UnifiedSource-src_jsc_bindings_webcore-1.cpp.o
[7/17] cxx obj/src/jsc/bindings/ZigGlobalObject.cpp.o
[8/17] cxx obj/unified/UnifiedSource-src_jsc_bindings_webcore-2.cpp.o
[9/17] cxx obj/unified/UnifiedSource-src_jsc_bindings-0.cpp.o
[10/17] cxx obj/unified/UnifiedSource-src_jsc_bindings-1.cpp.o
[11/17] gen cpp.rs (cppbind)
[11/17] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)

  nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19)

�[1m�[92m   Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core)
�[1m�[92m   Compiling�[0m bun_highway v0.0.0 (/workspace/bun/src/highway)
�[1m�[92m   Compiling�[0m bun_hash v0.0.0 (/workspace/bun/s
... (truncated)

diff hotspot

src/jsc/bindings/URLDecomposition.cpp            | 131 ++-
 src/jsc/bindings/URLDecomposition.h              |   3 +-
 src/jsc/bindings/webcore/URLPatternComponent.cpp |  11 +-
 test/js/web/url/url.test.ts                      | 231 +++++-
 test/js/web/urlpattern/urlpattern.test.ts        |  14 +
 test/js/web/urlpattern/urlpatterntestdata.json   | 979 ++++++++++++++---------
 6 files changed, 916 insertions(+), 453 deletions(-)

gate history · 3 passed · 0 rejected · iteration 11

evidence per changed file

file                                              reads  edits  tests
src/jsc/bindings/URLDecomposition.cpp                 8     20     17
src/jsc/bindings/URLDecomposition.h                   2      3     17
src/jsc/bindings/webcore/URLPatternComponent.cpp      2      2     17
test/js/web/url/url.test.ts                           4      7     11
test/js/web/urlpattern/urlpattern.test.ts             3      3      9
test/js/web/urlpattern/urlpatterntestdata.json        0      0     17

[coderabbitai]

Walkthrough

This PR reworks URLDecomposition's origin, host, hostname, and port handling in Bun's JSC bindings to follow WHATWG URL state semantics, and fixes URLPatternComponent's capture group emission to only expose named groups. Test suites for URL setters and URLPattern nested captures are added, and URLPattern test fixtures are reformatted and extended.

Changes

URL origin/host/port state semantics

Layer / File(s)	Summary
Origin computation for tuple and blob URLs src/jsc/bindings/URLDecomposition.cpp, test/js/web/url/url.test.ts	origin() narrows tuple-origin scheme checks and restricts blob URL origin derivation to inner http/https URLs; tests updated for ftps: opaque origin and revised blob URL origin expectations.
Host and hostname setter state machine src/jsc/bindings/URLDecomposition.cpp	setHost() and setHostname() are rewritten around a new findHostPortSeparator() helper implementing WHATWG host/hostname-state rules, IPv6 bracket handling, file-URL routing, and special-scheme empty-host behavior.
Port parsing and setPort behavior src/jsc/bindings/URLDecomposition.cpp, src/jsc/bindings/URLDecomposition.h	parsePort() fails explicitly with no digits and represents default-port as an inner nullopt; setPort() clears on empty string and commits via setFullURL(); header comment updated to document nested optional semantics.
URL setter test suite test/js/web/url/url.test.ts	A new data-driven describe("setters") block covers host/hostname/port assignment edge cases including whitespace, invalid/out-of-range ports, IPv6 literals, and terminator characters.

URLPattern nested capture group fix

Layer / File(s)	Summary
Capture group emission fix and regression test src/jsc/bindings/webcore/URLPatternComponent.cpp, test/js/web/urlpattern/urlpattern.test.ts	componentMatch caps emitted capture groups to min(numSubpatterns, groupNameList size) so only named groups are appended; a new test verifies nested regexp capture groups don't leak into results.
URLPattern test fixture updates test/js/web/urlpattern/urlpatterntestdata.json	Fixtures are reformatted and extended across path resolution, unicode/percent-encoding, port/hostname edge cases, authority/credential parsing, and advanced/recursive pattern capture scenarios.

Suggested reviewers: alii, cirospaciari

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the main URL and URLPattern specification fixes.
Description check	✅ Passed	The description explains the changes, verification steps, test results, scope limits, and related issue in sufficient detail.

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 3:17 PM PT - Aug 17th, 2026}

✅ @robobun, your commit 947a725a7452a72c0bc48fb3fa7e3b3bf6491f5e passed in Build #100194! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 32826

That installs a local version of the PR into your bun-32826 executable, so you can run:

bun-32826 --bun

[claude]

I didn't find any bugs, but this rewrites the host/hostname/port setter state-machine logic and changes parsePort semantics (with an acknowledged knock-on effect on URLPatternCanonical::canonicalizePort), so it's worth a human pass — in particular the hand-written nested-group test asserting { a: "12", b: "1" } where :b(3) matched 3 looks counterintuitive enough to double-check against Chrome.

Extended reasoning...

Overview

This PR audits Bun's URL and URLPattern against the WHATWG specs and fixes several conformance gaps. It touches three C++ binding files: URLDecomposition.cpp (a full rewrite of setHost/setHostname, a new findHostPortSeparator helper, a semantic change to parsePort so an empty digit buffer now fails, an explicit empty-string branch in setPort, and tightened origin() rules for ftps/blob:), URLDecomposition.h (comment only), and URLPatternComponent.cpp (clamping the match-result loop to m_groupNameList.size() so nested user-regexp captures don't leak as empty-named groups). Tests are extensive: 26 new setter cases cross-checked against Node 26.3/WPT, updated blob: origin assertions, a refreshed WPT urlpatterntestdata.json (349→369 entries), and a hand-written nested-group test.

Security risks

None apparent. The changes tighten parsing toward spec behavior (more rejections, e.g. hostname = "[::1]:80" now no-ops; origin returns "null" more often). No new allocation patterns, no untrusted size arithmetic; the ovector indexing in componentMatch is now bounded by min(numSubpatterns, groupNameList.size()) which is strictly safer than before.

Level of scrutiny

Moderate-to-high. URL setters are a core, widely-exercised web API and the new setHost is a from-scratch reimplementation of the host/port state-override transitions rather than a small patch. The PR description explicitly calls out an intentional cross-cutting behavior change: parsePort's other caller, URLPatternCanonical::canonicalizePort, now throws for { port: "\t\n" } where it previously returned "". That's defensible per the port-state spec text but is the kind of ripple a human should sign off on.

Other factors

The work is exceptionally well-documented (spec citations, Node/WPT cross-verification, before/after test counts) and the bug-hunting system found nothing. One thing I'd flag for the reviewer: the new urlpattern.test.ts case new URLPattern({ pathname: "/:a((?<x>1)(?<y>2)):b(3)" }).exec({ pathname: "/123" }) asserts groups is { a: "12", b: "1" }. With the simple min(numSubpatterns, groupNameList.size()) clamp, capture index 2 (the nested (?<x>1)) is what gets paired with name b, not capture index 4 (the (3) part). That may well be what upstream WebKit and the current spec text produce, but it reads surprising enough that it's worth a quick sanity check in Chrome before landing. The two WPT-sourced nested-group entries in urlpatterntestdata.json each have only one top-level named group, so they don't exercise this index-shift case.

[robobun]

Good flag on b: "1", that is the spot worth staring at, so here is the provenance.

The index shift is pre-existing and unchanged by this PR. Before the fix Bun returned { a: "12", b: "1", "": "3" } for that input; the only delta from the clamp is that the "" key is gone. The value comes from the compiled regex: :a((?<x>1)(?<y>2)) contributes capture 1 and the nested named groups occupy captures 2 and 3, so :b(3)'s own capture is index 4 while the group name list is just ["a", "b"] and names are paired with exec indices positionally.

Three sources agree on { a: "12", b: "1" }:

• Node 26.3 (Ada):

  new URLPattern({pathname:"/:a((?<x>1)(?<y>2)):b(3)"}).exec({pathname:"/123"}).pathname.groups
  // { b: '1', a: '12' }

• Upstream WebKit's URLPatternComponent::createComponentMatchResult uses the same clamp, std::min(execResult.length, m_groupNameList.size() + 1). That is the function this file was forked from; the fork replaced the JS exec array with the raw ovector and dropped the clamp, which is the bug.
• The spec's algorithm (https://urlpattern.spec.whatwg.org/#create-a-component-match-result step 5) names exec index N with group name list[N - 1] and never accounts for nested user-regexp captures shifting indices, so a literal reading produces the same pairing. For the extra captures it indexes the name list out of bounds, which is the underspecification every implementation resolves by clamping.

I could not run Chrome in this environment. If liburlpattern disagrees it would be splitting from Node, Safari, and the spec text together, which reads like a spec issue to file upstream rather than something for Bun to diverge on. The two WPT urlpatterntestdata.json entries added upstream for this (/:foo((?<x>a)) and /foo/(bar(?<x>baz))) each have a single named group precisely so they are unambiguous; the hand-written case exists to also pin that the "" key does not appear even when a later named group follows the nested one. Pushed a comment onto the test spelling out the index shift so the next reader does not have the same double take.

On the parsePort ripple: new URLPattern({ port: "abc" }) already threw in both Bun and Node before this change, via the same port-state rule (an empty digit buffer with a state override is return failure). { port: "\t\n" } now joins it, which makes Bun self-consistent and matches the spec text; Node returns "" there, which is Ada being inconsistent with its own "abc" handling. WPT does not exercise it. Happy to special-case it back if Node parity is preferred over the spec here.

[claude]

I didn't find any bugs, but this rewrites the URL host/hostname/port setter state machine and changes observable origin/URLPattern behavior (including the flagged URLPattern({ port: "\t\n" }) Node-parity question), so it's worth a human look before merging.

Extended reasoning...

Overview

This PR touches three C++ binding files and three test files to bring Bun's URL and URLPattern into WHATWG spec conformance:

• URLDecomposition.cpp: substantial rewrite of setHost, setHostname, setPort, parsePort, and origin(). Adds a new findHostPortSeparator scanner that mimics the spec's host-state ':' handling (bracket-aware, terminator-aware). Reorders the opaque-path / file-scheme / empty-host guards and changes how a failed port parse interacts with an already-committed host.
• URLDecomposition.h: comment-only.
• URLPatternComponent.cpp: clamps the componentMatch loop to m_groupNameList.size() so nested user-regexp captures don't leak as ""-keyed groups — a small, well-contained fix matching upstream WebKit.
• Tests: 26 new setter cases in url.test.ts (cross-checked against Node 26.3), updated blob:/ftps: origin expectations, one hand-written URLPattern nested-group test, and a WPT refresh of urlpatterntestdata.json (349→369 entries, plus reformatting).

Security risks

None identified. URL setters parse attacker-controllable strings, but the new code only narrows what the setter accepts (more cases now no-op instead of partially mutating). findHostPortSeparator is a bounded linear scan over a StringView with no allocation. The origin() change makes more schemes serialize as "null" rather than fewer, which is the safer direction for any origin-based check downstream.

Level of scrutiny

Medium-high. URL is a foundational web API; these setters are reachable from any user script and the edge-case matrix (file vs special vs non-special scheme, opaque path, IPv6 brackets, terminators, default ports, tab/newline stripping) is large. The setHost rewrite is not mechanical — it replaces a reverseFind(':')/setHostAndPort approach with a forward scan + two-phase commit, and the guard ordering changed. The author has done unusually thorough verification (full WPT urltestdata.json and setters_tests.json pass, Node cross-check, spec citations inline), which raises confidence considerably, but the logic still merits a maintainer's eye.

Other factors

• The PR explicitly leaves one decision open for a maintainer: parsePort's new empty-buffer failure means new URLPattern({ port: "\t\n" }) now throws where Node returns "". The author argues this is spec-correct and Node is internally inconsistent, but offers to special-case it back if Node parity is preferred. That's a judgment call this review can't make.
• The origin() change flips previously-passing test assertions (e.g. blob:ws://… → now "null"). The PR description says the old assertions were written alongside the buggy implementation and that WPT/Node/browsers agree on the new values, but it's a user-visible behavior change worth a maintainer ack.
• The bug-hunting pass found nothing, and the URLPattern clamp is straightforward. The bulk of the review weight is on the ~60 lines of new setHost/setHostname logic in URLDecomposition.cpp.

[robobun]

Rebased onto 0002bf8006. This one had a real conflict: #34660 and #38013 added a UTS #46 punycode check (hasAcceptableHost) to the old setHost/setHostname, which this PR rewrites, so the check is re-applied by hand via a setHostChecked() helper at every host-commit path (five sites; the old code had two). Details in the "Rebase note" section of the description.

Verification after the rebase:

• bun bd test on test/js/web/url/url.test.ts + test/js/web/urlpattern/urlpattern.test.ts: 482 pass / 0 fail as of 947a725, which also adds a sixth setter case found in review (a host part that is only tab/newline before a : is the same empty host as a bare :; Node already agreed, Bun did not).
• test-whatwg-url-toascii.js (assigns every WPT toascii input through both setters, so it exercises the re-applied check; ICU 78.3, not skipped) plus the other six test-whatwg-url-* node tests: all pass.
• Direct probe: an invalid xn-- label is a no-op and a valid one commits on all five paths (host=, host= with port, file: host=, hostname=, file: hostname=).
• Full WPT urltestdata.json + setters_tests.json: 8 + 6 failing assertions with this branch vs 15 + 11 against main's own source. The 14 shared ones are the xn-- cases that main and Node both intentionally fail since node compat batch: callback-throw dispatch, Assert class + native deep-equality parity, Intl gate + URL/buffer fallout, compile cache, watch kill-signal, profilers (+98 tests) #34660; the delta is exactly the cases this PR fixes.
• Directory sweep of test/js/web/url/, test/js/web/urlpattern/, test/js/node/url/, URLSearchParams.test.ts: 1574 pass, 1 pre-existing debug+ASAN timing failure (URL.canParse > repeatedly called) that reproduces with main's source.

Note on the red Source lints check at 42203c3: the failing test is test/internal/source-lints/jsresult-swallow.test.ts, a Rust ratchet. This PR's diff is three C++ files and three test files, and the same test fails identically on the bare base commit 0002bf8006 with none of this PR applied, so it is a break on main (reported separately), not something this change introduces.

Ready for review.

[claude]

I didn't find any bugs, but this rewrites the host/hostname/port setter state machine and changes origin semantics for ftps:/blob: URLs (plus an intentional Node divergence on URLPattern({ port: "\t\n" })), so it's worth a human pass on the spec reasoning before merging.

Extended reasoning...

Overview

This PR rewrites URLDecomposition::setHost/setHostname/setPort/parsePort/origin in src/jsc/bindings/URLDecomposition.{cpp,h} to follow the WHATWG URL spec's state-override parser, and clamps the capture-group loop in URLPatternComponent::componentMatch. Tests in test/js/web/url/url.test.ts add 26 setter cases (cross-checked against Node/WPT), urlpattern.test.ts adds a nested-group case, and urlpatterntestdata.json is refreshed from WPT (349→369 entries, plus reformatting).

Security risks

URL.prototype.origin is security-relevant — it feeds same-origin checks. The change here is strictly more restrictive (ftps:// and blob: with non-http(s) inner URLs now serialize as "null" instead of a tuple origin), which aligns with the spec/Node/browsers and is the safe direction. The host/hostname setter rewrite changes which host gets committed under malformed input (e.g., out-of-range ports now keep the old port but commit the new host), which is spec-correct but a behavior shift. I don't see an exploitable issue, but origin/host parsing is exactly the kind of code where a second pair of eyes on the state-machine edge cases is warranted.

Level of scrutiny

Medium-high. This is not a mechanical change: setHost went from a reverseFind(':') heuristic to a hand-rolled scan (findHostPortSeparator) that mirrors the spec's host-state terminator rules, with new commit-ordering semantics. The parsePort change also has a deliberate ripple into URLPatternCanonical::canonicalizePort, making new URLPattern({ port: "\t\n" }) throw where Node returns "" — the PR argues this is spec-correct and Node is inconsistent, which seems right, but it's an intentional divergence a maintainer should sign off on.

Other factors

Test coverage is excellent (full WPT urltestdata.json + setters_tests.json reportedly pass, CI is green modulo an infra hiccup), the PR description is thorough with spec citations for every case, and the bug-hunting system found nothing. The existing blob urls test had its expectations changed (previously asserting non-spec values), which is justified but means the old behavior was load-bearing in at least one test. No CODEOWNERS apply to these paths. Given the scope of the logic rewrite and the origin/host security adjacency, I'm deferring rather than approving.

[claude]

I didn't find any bugs, but this rewrites the URL host/hostname/port setter semantics and changes observable origin/URLPattern behavior, so it's worth a human sanity-check on the spec interpretation and the intentional parsePort ripple into URLPatternCanonical::canonicalizePort.

Extended reasoning...

Overview

This PR touches three C++ files in src/jsc/bindings/ (URL setter logic in URLDecomposition.cpp/.h and the URLPattern match-result builder in URLPatternComponent.cpp) plus three test files. The core change is a rewrite of setHost/setHostname/setPort to follow the WHATWG URL spec's state-override algorithm rather than the previous ad-hoc reverseFind(':') + setHostAndPort approach, a tightening of origin() to only return tuple origins for the five spec-listed schemes (and only inner http(s) for blob:), and a clamp on the URLPattern group-name loop to stop nested user-regexp captures leaking into exec() results. The WPT urlpatterntestdata.json is refreshed from 349 to 369 entries with a large formatting-only diff mixed in.

Security risks

None identified. URL parsing is security-adjacent (origin computation feeds same-origin decisions in browsers), but Bun's URL.prototype.origin is a getter on a userland object, not a security boundary here. The changes tighten behavior toward the spec (more "null" origins, more setter no-ops on invalid input) rather than loosening it.

Level of scrutiny

This warrants human review. It is a non-mechanical rewrite of user-visible Web API semantics in C++: the old setHost logic is fully replaced, parsePort's contract changes (empty digit buffer now fails instead of returning the default-port sentinel), and existing test expectations are edited to match new behavior (the blob:ws:///blob:file:// origin assertions flip from tuple origins to "null"). The PR description is thorough and cites WPT/Node/spec agreement for each case, and the bug-hunting pass found nothing, but the combination of "rewrites a state machine by hand", "changes existing assertions", and "has a documented intentional side effect on a second API (new URLPattern({ port: "\\t\\n" }) now throws)" is exactly the kind of change a maintainer should sign off on.

Other factors

Test coverage is strong (26-entry setter table cross-checked against Node, refreshed WPT data, hand-written nested-group test). CI ran green on the URL/URLPattern lanes per the author's rebase comment. There are no prior claude[bot] reviews on this PR, and no outstanding human reviewer comments. The one open judgment call the author explicitly flags — spec-over-Node for URLPattern({ port: "\\t\\n" }) — is a reasonable place for a maintainer to weigh in.

[robobun]

The port setter case in this PR (case 4, u.port = "\t\n" clearing the port) was re-reported separately from the list of remaining setter differences in #39458. Checked it against current main (30799d7a1e): it still reproduces, new URL("http://h:90/p").port = "\t" gives http://h/p (Node and the spec leave it at http://h:90/p), and the setPort / parsePort change plus the port row in the setter table here cover it, so no separate PR is being opened for it.

One thing to note: the branch no longer merges cleanly. #38013 added hasAcceptableHost to setHost and setHostname in src/jsc/bindings/URLDecomposition.cpp, which conflicts with the rewrite of those two functions here, so this needs a rebase before it can land.

[robobun]

Three more host setter cases from the #39458 list were handed over separately. Checked against current main (30799d7a1e), Node 26.3 and whatwg-url 14.1.0; two of them are covered by the setHost rewrite here, so no separate PR is being opened. Notes for the rebase:

1. Default port with an existing port, including the credentials variant. new URL("http://u:p@h:90/p").host = "x.com:80" gives http://u:p@x.com:90/p on main; Node and whatwg-url give http://u:p@x.com/p. The setHost + setPort(std::nullopt) sequence here keeps the credentials (URL::setHost rebuilds from hostStart()) and removes the port, so this is the same fix as the Assigning a hostname with port 80 to URL.host will not override the existing port #28661 rows in the table. Worth adding the credentials row, since it is the one spelling URL::setHostAndPort could not have produced.

2. file: host with a colon but no digits. new URL("file:///p").host = "x.com:" gives file://x.com/p on main (the portLength == 0 branch drops the : and commits the host); "[::1]:" gives file://[::1]/p the same way. Node and whatwg-url leave the URL unchanged for both (file host state, : is a forbidden host code point, and [::1]: fails the IPv6 parse). The file branch here hands the whole value to URL::setHost, which rejects x.com: on its colon check and fails the reparse of file://[::1]:/p (new URL("file://[::1]:/p") already throws on main), so both are covered; the table only has the h2:x / x:123 spellings, so the empty-port spelling would be a useful extra row or two.

3. Not a bug, please keep the current behavior when rebasing: new URL("foo:/p").host = "" (and hostname = "") gives foo:///p on main, and Node gives foo:/p. The host state for a non-special URL with no credentials and no port host-parses the empty buffer and sets the host to the empty string, so the serialization gains //; whatwg-url 14.1.0 returns foo:///p as well. Node only materializes the empty host when the path starts with /. (the WPT non-spec:/.//p row, where it returns non-spec:////p) and otherwise leaves the URL alone, which is an Ada shortcut rather than the spec. The view-source+http://example.net/foo row in the table is the same rule with a host already present; foo:/p is the null-host variant of it.

[claude]

I re-reviewed after 3e52404 addressed the tab/newline empty-host nit and didn't find further issues. Given that this is a full rewrite of the setHost/setHostname state machine plus a change to origin() semantics, a human look is still worthwhile.

What was reviewed:

• findHostPortSeparator bracket/terminator scan and the five setHostChecked commit sites — the re-applied hasAcceptableHost check covers every path that writes a host, including the new file: and host-with-port branches.
• parsePort's second caller URLPatternCanonical::canonicalizePort — the !foundDigit → nullopt change makes new URLPattern({port: "\t\n"}) throw, which is the intentional Node divergence called out in the description.
• origin() narrowing — only removes tuple origins (ftps, blob:ws/wss/ftp/file), never adds one, so it's the safe direction.
• componentMatch clamp — m_groupNameList[i - 1] is bounded by the std::min, matching upstream WebKit's createComponentMatchResult.

Extended reasoning...

Overview

Three C++ source changes and three test files. URLDecomposition.cpp rewrites setHost() and setHostname() around a new findHostPortSeparator() helper that models the WHATWG host-state → port-state transition, changes setPort() so only a literally-empty string clears the port, splits parsePort()'s no-digit case from the default-port case, and narrows origin() to the spec's tuple-origin scheme list. URLPatternComponent.cpp clamps the group-emission loop to min(numSubpatterns, groupNameList.size()). Tests add a 27-row setter table verified against Node/whatwg-url, refresh the WPT URLPattern fixtures to 369 entries, and update the pre-existing blob: origin assertions to spec values.

Security risks

origin() is same-origin-policy relevant. The change strictly narrows what receives a tuple origin (ftps:// and blob: with inner ws/wss/ftp/file now serialize as "null"), which is the conservative direction — nothing that was previously opaque becomes a tuple. The host setters route every commit through setHostChecked() → hasAcceptableHost(), so the UTS #46 punycode gate from #34660/#38013 is preserved on all five new commit paths (previously two). No new untrusted-input parsing beyond what WTF::URL::setHost already does.

Level of scrutiny

High. This is a hand-implemented state machine replacing ~30 lines of ad-hoc reverseFind(':') logic with ~60 lines that must correctly interleave IPv6 bracket tracking, terminator handling (/ ? # \\), tab/newline stripping, file-scheme special-casing, special-vs-opaque host emptiness rules, and the punycode check — in a Web API that every URL-bearing feature depends on. The parsePort change ripples to URLPatternCanonical::canonicalizePort, producing an intentional divergence from Node that the description flags for maintainer sign-off. The rebase manually re-applied a security-relevant check across a rewrite. None of this is mechanical.

Other factors

Test coverage is thorough: gate evidence shows 12 fail → 0 fail across both test files under debug+ASAN and release, WPT parity with Node on the full urltestdata/setters_tests suites, and a 1574-pass directory sweep. The one inline nit I raised on the previous push ("\t:" bypassing the empty-host guard) was fixed in 3e52404 via containsOnly<isASCIITabOrNewline>() with a matching test row. The comment-cop bot flags are all resolved. The two remaining WTF::URLParser bugs the description records are correctly deferred to the WebKit fork. Deferring because the scope (URL setter rewrite + origin semantics + intentional Node divergence) is beyond what I'd approve without a human maintainer's eye, not because of any specific concern with the implementation.