Bump WebKit (oven-sh/WebKit#460 preview): URL.pathname keeps a dotted first segment in host-less URLs, fixing custom-scheme URLPatterns - #39457
Conversation
|
Updated 10:43 PM PT - Aug 17th, 2026
❌ @robobun, your commit 124d932 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 39457That installs a local version of the PR into your bun-39457 --bun |
|
Status: ready for review; blocked on oven-sh/WebKit#460 merging first.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 0 remain after this review. WalkthroughThe build now selects a WebKit preview release. URL and URLPattern tests add regression coverage for dotted first pathname segments across host-less and non-special URLs. ChangesURL path regressions
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it bumps WEBKIT_VERSION (an engine change) and is currently pinned to a preview build that the description says must be re-pinned to the merged oven-sh/WebKit commit before this lands, a human look is still warranted.
What was reviewed:
- The new URL tests: dotted-first-segment pathnames, the
/.guard control cases, relative resolution, and the pathname/host/search/protocol setters — expected values line up with the URL Standard's serializer step and Node's behavior. - The new URLPattern tests: constructor-string,
URLPatternInit,baseURL, and matched-input forms for non-special schemes, plus the unaffectedhttps/protocol-less controls. prebuiltUrl/prebuiltDestDirinscripts/build/deps/webkit.tsalready handleautobuild--prefixed version strings, so the preview pin resolves correctly.
Extended reasoning...
Overview
This PR changes WEBKIT_VERSION in scripts/build/deps/webkit.ts from commit c6cfe90c60... to the preview tag autobuild-preview-pr-460-e932620f, and adds ~190 lines of regression tests across test/js/web/url/url.test.ts and test/js/web/urlpattern/urlpattern.test.ts. The actual code fix lives in oven-sh/WebKit#460 (a change to WTF::URL::pathStart()); no Bun source is modified.
Security risks
None identified. The tests are pure in-process assertions with no network, filesystem, or subprocess use. The version bump changes which prebuilt WebKit tarball CI downloads from oven-sh/WebKit releases — same trusted origin as before.
Level of scrutiny
High. WEBKIT_VERSION selects the JavaScriptCore engine build for the entire runtime; even a one-line bump here is a dependency upgrade in the sense of the repo's "Dependencies & vendoring" guidance. The description states the preview branch is directly on top of the previous pin, so the delta is claimed to be exactly the pathStart() fix, but I cannot verify the WebKit-side diff from this repo. The PR also explicitly says the pin must be moved to the merged WebKit commit before this lands — approving now would risk merging a preview pin.
Other factors
The test additions themselves are well-constructed: they follow the file's existing conventions, use exact-value toEqual on labelled objects (so a failure names the offending row), include negative/control cases confirming the /. guard is still stripped where it should be, and cover the setter and URLContext offset paths that share the same pathStart() computation. The description reports 9 of the new tests fail on the old pin and all pass on the new one, which satisfies the fails-for-the-right-reason bar. Given the engine bump and the pending re-pin, deferring to a human is the right call.
… first segment in host-less URLs
WTF::URL::pathStart() skipped the "/." that a host-less URL serializes in
front of a path starting with "//" without checking that a "/" follows, so
new URL("foo:/.a/b").pathname was "a/b" and every URLPattern with a
non-special scheme and a pathname starting with a dot compiled to the
truncated path. oven-sh/WebKit#460 fixes the offset; this pins its preview
build and adds URL and URLPattern tests for the getter, the setters,
relative resolution, the inspected URLContext offsets and pattern matching.
fd636e3 to
124d932
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs in the test additions or the version-pin change. Since it bumps the WebKit engine pin (and to a preview build that the description says must be re-pinned to the merged oven-sh/WebKit commit before this lands), a human should sign off.
What was reviewed:
- New URL tests: dotted-first-segment pathnames, control rows for the
/.guard, relative resolution, and the pathname/host/search/protocol setters — expectations line up with the URL Standard's serializer step and Node's behavior. - New URLPattern tests: constructor-string / init / baseURL forms with non-special schemes, plus unaffected
https/protocol-less controls. prebuiltUrl/prebuiltDestDirinscripts/build/deps/webkit.tsalready handleautobuild--prefixed version strings, so the preview tag resolves correctly.
Extended reasoning...
Overview
The PR changes WEBKIT_VERSION in scripts/build/deps/webkit.ts from a commit hash to the preview-build tag autobuild-preview-pr-460-8da5349b, and adds ~190 lines of regression tests to test/js/web/url/url.test.ts and test/js/web/urlpattern/urlpattern.test.ts covering the WTF::URL::pathStart() fix in oven-sh/WebKit#460 (host-less URLs whose first path segment starts with a dot).
Security risks
None identified. The change is a version pin plus test additions; no auth, crypto, or input-handling code in Bun itself is touched. The engine-side change (in the WebKit fork, not in this diff) narrows a condition in pathStart() — it makes the /. guard skip more restrictive, not less.
Level of scrutiny
High. WEBKIT_VERSION pins the entire JavaScriptCore engine; even a one-line change in WTF affects every URL operation in the runtime. The repo's review guidance calls out dependency/vendor bumps as needing situational attention, and WebKit bumps in particular are the kind of change a maintainer familiar with the oven-sh/WebKit release process should approve. The PR description also explicitly states the preview pin must be replaced with the merged commit before this lands, so it is not in a mergeable state by the author's own account.
Other factors
The test coverage is thorough and follows the repo's conventions: added to the existing files for URL/URLPattern, exact-value assertions, negative controls confirming the /. guard is still stripped for foo:/.//b, variant coverage across setters and URLPattern constructor forms, and expected values cross-checked against Node 26. I confirmed prebuiltUrl() and prebuiltDestDir() already special-case values starting with autobuild-, so the preview tag will download and cache correctly. Nothing in the tests looks flaky (no sleeps, no network). The remaining reason to defer is purely that engine pin changes — especially to an unmerged preview build — warrant a maintainer's explicit go-ahead.
Bumps
WEBKIT_VERSIONto pick up oven-sh/WebKit#460 and adds thebun:testcoverage for it.Pinned to the preview build of oven-sh/WebKit#460 so CI exercises the change; to be re-pinned to the merged oven-sh/WebKit commit before this merges.
Problem
URL.prototype.pathnamedrops the leading/.of a URL that has no host and whose first path segment starts with a dot.hrefis right, only the getter is wrong, and the same happens through thepathnamesetter:new URL("foo:/.//b").pathnameis//beverywhere, and URLs with a host (foo://h/.a,https://h/.a,file:/.a) are unaffected.Every
URLPatternwith a custom scheme and a pathname starting with a dot is compiled wrong as a result, because for non-special schemes the pathname text is canonicalized by parsing it as the path of a host-less dummy URL and reading the pathname back (src/jsc/bindings/webcore/URLPatternCanonical.cpp,canonicalizeOpaquePathname). The pattern's host makes no difference since the dummy URL never has one:httpspatterns and{ pathname: "/.well-known/*" }without a protocol are unaffected.u.host = "h:1"on such a URL was also silently ignored, andutil.inspect(url, { showHidden: true })reported nosearch_startforfoo:/.foo?x, since both are computed from the same offset.Cause:
WTF::URL::pathStart()(Source/WTF/wtf/URL.cppin oven-sh/WebKit, the offset every path accessor and setter uses) skips a/.directly after the scheme without checking that a/follows it. That/.is meant to be the guard the serializer puts in front of a path starting with//in a host-less URL, but the check also matches a real first segment such as.aor.git. No Bun source is involved:URLDecomposition::pathname()returnsURL::path(), and the URLPattern canonicalizers read the same accessor. Same in upstream WebKit and in every Bun release; the WPTurltestdata.jsonand URLPattern corpora that Bun runs have no host-less URL with a dotted first segment, which is why this was only found by a differential run against Node and Deno.Fix
pathStart()also require the character after/.to be/(the only shape the parser ever inserts, and the same test its relative-URL path already used). The branch is directly on top ofeeab04040f, the commit main is pinned to, so the new pin is exactly the current engine plus that change. Since the fix is in the offset itself,pathname, thepathnameandhostsetters, relative resolution, URLPattern and the inspect offsets are all fixed at once.test/js/web/url/url.test.ts: the dotted first segments above (including?q#f,%2F,/.a//b,/./.a), control rows showing the/.guard is still omitted forfoo:/.//b,foo:/..//b,foo:/.//.aand that hosts and special schemes are unchanged, relative resolution against and onto such URLs, thepathname,search,protocolandhostsetters (bothhandh:1, on a dotted path and on a/.//path), andsearch_startin the inspectedURLContext.test/js/web/urlpattern/urlpattern.test.ts: the constructor-string,URLPatternInitandbaseURLforms with a non-special scheme,test()andexec()results including the matchedpathname.input, and unaffectedhttps/ protocol-less / later-segment patterns. Every expected value was checked against Node 26.USE_SYSTEM_BUN=1 bun teston the two files: 436 pass, 9 fail, all in the new blocks plus the extendedURLContexttest) and the two control tests pass. Against this pin the two files pass in full underbun bd test, including the WPT URL constructor and URLPattern corpora they already contain. The WebKit change itself was also checked by compiling WebKit's ownWTF_URL/WTF_URLParsergtest files (with the rows added in WTF: URL::path() keeps a first path segment that starts with a dot in host-less URLs (URL("foo:/.a/b").path() was "a/b") WebKit#460) against theeeab0404debug ASAN prebuilt: 3 of 29 tests fail with the stocklibWTF.a, 29 of 29 pass with the patchedURL.cpplinked in front of it.Background
scheme:/.followed by the path (foo:/.//bfor the path//b), becausefoo://bwould re-parsebas a host. Thepathnamegetter returns the path without that guard, and only the guard; a first segment that merely starts with.is ordinary path text.URLkeeps the serialized string plus component offsets and derives the start of the path from the end of the host and port, which is where the guard special case lives. Bun'sURL(src/jsc/bindings/URLDecomposition.cpp) and URLPattern (src/jsc/bindings/webcore/URLPattern*.cpp) are thin layers over it.WEBKIT_VERSIONinscripts/build/deps/webkit.tsis the only place the engine version lives; CI andbun bddownload the prebuiltautobuild-<version>release for it from oven-sh/WebKit. Preview builds of a WebKit PR are published asautobuild-preview-pr-<n>-<sha>and can be pinned the same way.[decide:webkit] gate passed · iteration 0 · 3 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file