[WTF] URLParser: file: drive letter quirk with a host, before ?/#, after dot segments, and as a relative base - #462
Conversation
|
Warning Review limit reached
On-demand reviews are free for the next 26 days. After that, they cost $0.25 per reviewed file. Or wait 32 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between eeab040 and ebf620e974349e5b7b75410623195db240b8d835. 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 0 remain after this review. WalkthroughThe URL parser now preserves and normalizes Windows drive-letter paths in ChangesFile URL drive-letter handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed and directly explains the problem, implementation, affected tests, changed file, and verification results. It does not include the required Bugzilla URL, reviewer line, or a template-formatted changed-function list. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it modifies state-machine transitions and offset bookkeeping in URLParser::parse — a security-relevant, spec-conformance-critical path — a human look would still be worthwhile.
What was reviewed:
- Traced the four
FileHost/FilePathStart/Path/Filetransitions against the URL Standard's file-host and path states; the reprocessing of?/#inState::Pathand the newFilePathStartentry after a parsed host both preserve the existing slash/segment handling. - Checked that the new
FilePathStartassertion (m_pathAfterLastSlash == m_hostEnd + m_portLength + 1) holds on both entry paths, and thatcopyBaseWindowsDriveLetteris guarded by the single-segment check so multi-segment bases are unaffected. - Verified the EOF
FileHostoffset change keepsm_pathAfterLastSlash <= m_pathEndand now matches the offsetsfile:///C:produces.
Extended reasoning...
Overview
This PR fixes four spec deviations in WTF::URLParser::parse's handling of the Windows drive letter quirk for file: URLs, found via differential fuzzing against Ada. It touches five spots in the ~3000-line parser state machine (File default case, FileHost windows-quirk and post-host branches, FilePathStart, the Path dot-segment branch, and the EOF FileHost finalizer) and adds an 82-row test plus updates two existing ParserDifferences expectations.
Security risks
URL parsing is security-relevant — origin computation, path normalization, and host handling all flow through this code, and non-idempotent serialization (which two of the fixed cases exhibited) is a classic source of parser-differential issues. The changes here are narrowly scoped to file: URLs with drive-letter-shaped first segments, and each branch is gated on m_urlIsFile / protocolIsFile() or is already inside a file-only state. I did not find a way for the new logic to affect non-file schemes or to widen what is accepted as a host. The fixes move behavior toward the reference implementation (whatwg-url) and eliminate two non-idempotent outputs, which is a net reduction in differential-attack surface. Still, any edit to this state machine deserves a second pair of eyes.
Level of scrutiny
High. URLParser.cpp is core WTF infrastructure shared by every URL constructed in Bun, the state machine is dense with implicit invariants (buffer position vs. recorded offsets, m_didSeeSyntaxViolation gating, iterator advancement), and one of the edits replaces a runtime position check with a debug assertion. The PR description is exceptionally thorough — each fix is justified against a specific spec step, WPT results are byte-identical, and 7932 generated inputs were diffed against whatwg-url with zero remaining differences — but the fork's CI does not build TestWebKitAPI, so the new tests were run out-of-band against a prebuilt.
Other factors
No prior human review on the timeline (CodeRabbit was rate-limited). The change is not mechanical: it restructures control flow (early break to State::Path in the windows-quirk branch, conditional FilePathStart vs PathStart after a host) and adjusts which offsets are recorded where. The test coverage added is excellent and the negative cases (C|x, C%7C, second-segment drive letters, http scheme) look right, but given the criticality of the file and the lack of in-CI test execution, I'm deferring rather than approving.
Preview Builds
|
c05387a to
ebf620e
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it modifies four state transitions in the URLParser::parse state machine — a security-sensitive core path — a human look would still be worthwhile.
What was reviewed:
- Traced every entry into
FilePathStartafter the refactor (empty-host slash, post-parseHostAndPortslash, post-localhostshrink) and confirmed the newm_pathAfterLastSlash == m_hostEnd + m_portLength + 1assertion holds on each. - Checked the
windowsQuirk→State::Pathreprocessing:cis left on the slash/?/#, soPathhandles the separator/query/fragment without adding a segment;file://C|/x,file://C|\x,file://C|/..behavior is preserved by the test rows. - Verified the new
File-statecopyBaseWindowsDriveLetterguard only fires when the copied base path is exactly/(single-segment base), matching the spec's shorten-is-no-op rule; and that the EOFFileHostoffset fix now records the samem_pathAfterLastSlash/m_pathEndas parsingfile:///C:. - Checked the
Path-state dot-segment addition is gated onm_urlIsFileand an empty path, so http URLs and non-first segments are unaffected (covered by the negative test rows).
Extended reasoning...
Overview
This PR aligns WTF's URLParser with the WHATWG URL Standard's Windows drive-letter quirk in four places inside URLParser::parse: the FileHost state when a drive letter is followed by ?/#/slash, the FileHost → path transition after a real host (including the localhost-dropped case), the Path state after dot segments empty the path, and the File state when resolving a relative reference against a base whose path is a lone drive letter. It also corrects the EOF FileHost offsets so file://C| and file:///C: produce identical m_pathAfterLastSlash. A runtime position check in FilePathStart is converted to an ASSERT now that all entries into that state come directly after the host. 82 new test rows plus two updated ParserDifferences expectations cover the fixed shapes and the shapes that must not change.
Security risks
URL parsing is security-relevant: differences in how file:// URLs are canonicalized can affect origin comparison, path-based access checks, and idempotence assumptions in downstream consumers. The change moves toward stricter spec conformance (matching Node/Ada and whatwg-url), which is generally the safer direction, and two of the four fixes eliminate non-idempotent serializations. I did not find any case where the new behavior widens what a file: URL can reach — the drive letter is retained where the spec says it must be, and .. still cannot pop it (popPath() already refuses). Nonetheless, changes to a hand-rolled state-machine parser with manual offset bookkeeping in a foundational library warrant a human reviewer's eyes.
Level of scrutiny
High. URLParser::parse backs new URL(...) in Bun and every URL-consuming API. The diff is small in line count but each hunk changes control flow in a state machine where a misplaced offset shows up only in relative-resolution behavior, not in the serialized string. The author's verification is unusually thorough (WPT byte-identical, 7932-input differential against whatwg-url with 0 remaining diffs, ASAN + ASSERT_ENABLED run of the full WTF_URLParser suite, tab-injection at every position), and I traced each new state transition and offset assignment against the surrounding code without finding a problem. But converting a runtime guard to an ASSERT and rerouting FileHost through Path/FilePathStart are the kind of invariant changes a maintainer familiar with this parser should confirm.
Other factors
The fork's CI does not build TestWebKitAPI, so the 82 new test rows are not exercised automatically — verification was done manually against a prebuilt. The PR description is exceptionally detailed and the differential testing against the reference implementation gives strong confidence in correctness. No prior human review on the thread; CodeRabbit was rate-limited. Given the critical path and the assertion change, I'm deferring rather than auto-approving.
…quirk with a host, before ?/#, after dot segments, and as a relative base Pins WEBKIT_VERSION to the preview build of oven-sh/WebKit#462, which makes the URL parser apply the file: drive letter quirk whenever the path is empty and keep a lone drive letter when resolving a relative path against it: new URL("file://C|?q").href file:///C:/?q -> file:///C:?q new URL("file://host/C|/x").href file://host/C|/x -> file://host/C:/x new URL("file://localhost/C|/x").href file:///C|/x -> file:///C:/x (was not idempotent) new URL("file:///./C|").href file:///C| -> file:///C: (was not idempotent) new URL("x", "file:///C:").href file:///x -> file:///C:/x Adds the matching coverage to test/js/web/url/url.test.ts.
ebf620e to
f1436cb
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks several interacting state transitions in URLParser::parse (a security-relevant parser) and swaps a runtime path-position check for an assertion, a human look would still be worthwhile.
What was reviewed:
- The four state-machine edits (
File,FileHost,FilePathStart,Path, and the EOFFileHostoffset fix) against the WHATWG file-host / path-state / shorten-path steps cited in the comments. - That the new
FilePathStartassertion holds on every path now reaching it (the twoFileHosttransitions only enter it immediately after writing the host, with nothing yet in the path). - That the
copyBaseWindowsDriveLetterguard inFileonly fires when the base's path is exactly/<drive>and reuses the same helperFileSlashalready relies on. - The 82 new test rows and the two updated
ParserDifferencesrows against Node/whatwg-url expectations described in the PR.
Extended reasoning...
Overview
This PR fixes four spec deviations in WTF's URLParser around the Windows drive-letter quirk for file: URLs, all inside the ~800-line URLParser::parse state machine. It touches the File default case (relative resolution against a base whose path is a lone drive letter), the FileHost state (drive letter followed by ?/#/slash, and the transition after a real host), FilePathStart (a position check becomes an ASSERT), the Path state's dot-segment branch, and the EOF FileHost offset bookkeeping. It adds an 82-row test and updates two existing ParserDifferences rows.
Security risks
URL parsing is security-relevant: offsets and serialization drive origin comparison and relative resolution. The changes here are confined to file: URLs and to the drive-letter quirk specifically, and they move behaviour toward the WHATWG spec / Node / whatwg-url (verified by the author against WPT urltestdata.json byte-for-byte and a 7932-input differential set). I did not find any new failure modes, but the parser's state machine is intricate enough that subtle cross-state interactions are hard to rule out by reading alone.
One specific point worth a second pair of eyes: FilePathStart previously guarded appendWindowsDriveLetter with currentPosition(c) == m_url.m_hostEnd + 1; that is now an ASSERT on m_hostEnd + m_portLength + 1. I traced the two callers and both enter FilePathStart directly after setting m_hostEnd/m_portLength with an empty path, so the invariant holds — but in a release build the assertion is compiled out, so if a future edit adds another entry path the guard is gone.
Level of scrutiny
High. This is not a mechanical or config change — it restructures control flow in a hand-written state machine that every new URL(...) in Bun goes through. The author's verification is unusually thorough (ASAN debug build, WPT parity, differential fuzzing against whatwg-url with zero remaining divergences), which raises confidence considerably, but the change still falls outside what I'd auto-approve.
Other factors
The fork's CI does not build TestWebKitAPI, so the new tests were run manually against a prebuilt libWTF.a. The two modified ParserDifferences expectations now match the corresponding WPT rows. No prior human review comments are on the thread.
…quirk with a host, before ?/#, after dot segments, and as a relative base Pins WEBKIT_VERSION to the preview build of oven-sh/WebKit#462, which makes the URL parser apply the file: drive letter quirk whenever the path is empty and keep a lone drive letter when resolving a relative path against it: new URL("file://C|?q").href file:///C:/?q -> file:///C:?q new URL("file://host/C|/x").href file://host/C|/x -> file://host/C:/x new URL("file://localhost/C|/x").href file:///C|/x -> file:///C:/x (was not idempotent) new URL("file:///./C|").href file:///C| -> file:///C: (was not idempotent) new URL("x", "file:///C:").href file:///x -> file:///C:/x Adds the matching coverage to test/js/web/url/url.test.ts.
f1436cb to
e0c8382
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes state transitions and offset bookkeeping in URLParser::parse — a core, security-adjacent path — a human look would still be worthwhile.
What was reviewed:
- The four state-machine edits (FileHost quirk → Path, FileHost after host → FilePathStart, Path dot-segment drive letter, File-state
copyBaseWindowsDriveLetter) against the WHATWG spec steps cited. - The FilePathStart position check → ASSERT: confirmed the only remaining entry points (empty-host FileHost, post-
parseHostAndPorton/or\\) all arrive withm_pathAfterLastSlash == m_hostEnd + m_portLength + 1. - The
D|-against-file:///C:case (bare drive-letter input against a lone-drive-letter base) —shouldCopyFileURLreturns false for a leading drive letter, so the newcopyBaseWindowsDriveLetterbranch is not reached and the base'sC:is correctly replaced.
Extended reasoning...
Overview
This PR fixes four spec-conformance deviations in WTF's URLParser::parse() around the WHATWG URL Standard's Windows drive-letter quirk for file: URLs. The changes are in Source/WTF/wtf/URLParser.cpp (the FileHost, FilePathStart, Path, and File states, plus the EOF-in-FileHost final-state block) and add an 82-row test in Tools/TestWebKitAPI/Tests/WTF/URLParser.cpp plus updates to two existing ParserDifferences rows.
Security risks
URL parsing is generally security-sensitive because it feeds origin determination and path resolution. These changes are confined to file: URLs with Windows drive-letter first segments — a narrow surface that doesn't affect http(s) origin computation — and they move behavior toward the spec and toward idempotence (the pre-change behavior produced non-idempotent serializations like file:///C|/x). I don't see a new injection, host-confusion, or path-traversal vector introduced here; the popPath() guard against popping a first-segment drive letter was already in place and is unchanged.
Level of scrutiny
High. URLParser::parse() is a hand-rolled state machine with tightly coupled offset fields (m_hostEnd, m_portLength, m_pathAfterLastSlash, m_pathEnd), and this PR edits state transitions, converts a runtime position check into an ASSERT, and adjusts offset assignments in the EOF FileHost block. These are exactly the kinds of edits where an off-by-one or a missed entry path shows up far from the change site. The author's verification is unusually thorough (WPT byte-identical, 7932-input differential against whatwg-url with 0 remaining diffs, ASAN debug run with the new assertion active), which raises confidence substantially — but the parser's criticality still puts this above the bar for auto-approval.
Other factors
- No prior human review on the thread yet.
- The fork's CI does not build TestWebKitAPI, so the 82 new test rows are not exercised in CI here (the author ran them manually against a prebuilt).
- One candidate issue was raised and refuted during the multi-agent pass: a bare drive-letter relative input (
D|) against a lone-drive-letter base (file:///C:). I re-checked:shouldCopyFileURL(c)returns false whencstarts with a Windows drive letter, so the newcopyBaseWindowsDriveLettercall in the File state is skipped and the base'sC:is not kept — matching the test expectationfile:///D:. - The FilePathStart runtime check → ASSERT is sound for the two remaining entry points I traced, but if any other path can reach FilePathStart with a non-empty path buffer, debug builds would crash. I did not find such a path.
…quirk with a host, before ?/#, after dot segments, and as a relative base Pins WEBKIT_VERSION to the preview build of oven-sh/WebKit#462, which makes the URL parser apply the file: drive letter quirk whenever the path is empty and keep a lone drive letter when resolving a relative path against it: new URL("file://C|?q").href file:///C:/?q -> file:///C:?q new URL("file://host/C|/x").href file://host/C|/x -> file://host/C:/x new URL("file://localhost/C|/x").href file:///C|/x -> file:///C:/x (was not idempotent) new URL("file:///./C|").href file:///C| -> file:///C: (was not idempotent) new URL("x", "file:///C:").href file:///x -> file:///C:/x Adds the matching coverage to test/js/web/url/url.test.ts.
e0c8382 to
68406ff
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks several state transitions in the URLParser::parse state machine (URL parsing is security-adjacent) and converts a runtime position check in FilePathStart into an assertion, a human look would still be worthwhile.
What was reviewed:
- The four
FileHost/FilePathStart/Path/Filestate changes against the WHATWG URL Standard sections cited in the diff. - Checked that routing
FileHost→FilePathStart(instead ofPathStart) for/and\\produces the same buffer/offset writes for non-drive-letter paths. - Confirmed
copyBaseWindowsDriveLetteris guarded bym_pathAfterLastSlash == hostEnd+portLength+1, so multi-segment bases are unaffected. - Verified the EOF
FileHostoffset change makesm_pathAfterLastSlashpoint at the slash (matchingfile:///C:), withm_pathEndstill at the drive-letter end.
Extended reasoning...
Overview
This PR fixes four spec-conformance divergences in Source/WTF/wtf/URLParser.cpp around the WHATWG URL Standard's Windows drive-letter quirk for file: URLs, and adds an 82-row test (FileWindowsDriveLetterWithAuthority) plus updates two existing ParserDifferences rows in Tools/TestWebKitAPI/Tests/WTF/URLParser.cpp. The parser changes touch five locations in URLParser::parse:
State::Filedefault branch: aftercopyURLPartsUntil(PathAfterLastSlash), if the base's path was a lone segment, re-append the drive letter viacopyBaseWindowsDriveLetterand a trailing/so relative resolution againstfile:///C:keeps the drive.State::FileHostwindows-quirk branch: instead of falling through into the shared empty-host block (which appended/?or/#), setm_pathAfterLastSlashand jump straight toState::Pathso the current?/#/slash is reprocessed there.State::FileHostafterparseHostAndPort: transition toFilePathStart(notPathStart) when the next code point is/or\\, so a drive letter after a real host (including the droppedlocalhost) gets normalized.State::FilePathStart: thecurrentPosition(c) == m_url.m_hostEnd + 1runtime guard becomes anASSERT(m_url.m_pathAfterLastSlash == m_url.m_hostEnd + m_url.m_portLength + 1), and the drive-letter check is now unconditional within the slash branch.State::Pathdot-segment branch: after consuming./..that leave the buffer at exactlyhostEnd+portLength+1, applyappendWindowsDriveLetterif the next segment is a drive letter.- EOF
State::FileHost:m_pathAfterLastSlashnow points just after the leading/(not afterC:), withm_pathEnd/m_queryEndset to the actual end — matching the offsetsfile:///C:would produce.
Security risks
URL parsing is security-relevant: parser differentials between engines can enable host/path confusion. This change reduces divergence from the URL Standard and from Node (Ada) / whatwg-url, and the affected inputs are file: URLs with drive-letter-shaped first segments — a narrow surface. I did not identify a way for the new transitions to misparse a host as a path or vice versa; the FileHost→FilePathStart reroute only fires when *c is a slash, and FilePathStart writes the same single / that PathStart→Path would have. Still, any change to the URL parser state machine merits a second pair of eyes.
Level of scrutiny
High. URLParser::parse is a ~1500-line hand-written state machine with tightly coupled offset bookkeeping (m_hostEnd, m_portLength, m_pathAfterLastSlash, m_pathEnd), where an off-by-one in one branch surfaces as a relative-resolution bug elsewhere. The PR replaces a runtime check with an assertion on the claim that FilePathStart is now only reached immediately after a host; I traced the entries to FilePathStart (the empty-host authorityOrHostBegin == c branch and the new post-parseHostAndPort branch) and both satisfy the invariant, but this is exactly the kind of global reasoning a maintainer familiar with the parser should confirm.
Other factors
The PR description documents unusually thorough verification: all 10 WTF_URLParser gtests pass under debug ASAN with ASSERT_ENABLED, the vendored WPT urltestdata.json (869 rows) is byte-identical before/after, and a 7932-input generated corpus goes from 736 differences vs whatwg-url to 0. Every new test row was cross-checked against Node 26 and also run with a tab inserted at every position. The bug-hunting system found nothing. The change is well-motivated and appears correct, but its scope — multiple interdependent state-machine edits in core WTF code that upstream WebKit also carries — puts it outside what I'd auto-approve without a human sign-off.
…quirk with a host, before ?/#, after dot segments, and as a relative base Pins WEBKIT_VERSION to the preview build of oven-sh/WebKit#462, which makes the URL parser apply the file: drive letter quirk whenever the path is empty and keep a lone drive letter when resolving a relative path against it: new URL("file://C|?q").href file:///C:/?q -> file:///C:?q new URL("file://host/C|/x").href file://host/C|/x -> file://host/C:/x new URL("file://localhost/C|/x").href file:///C|/x -> file:///C:/x (was not idempotent) new URL("file:///./C|").href file:///C| -> file:///C: (was not idempotent) new URL("x", "file:///C:").href file:///x -> file:///C:/x Adds the matching coverage to test/js/web/url/url.test.ts.
68406ff to
f17207c
Compare
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunting pass found no issues. Because it rewires control flow across four states of the URLParser state machine — code that determines origin/path for every file: URL and diverges from upstream WebKit — a human look would still be worthwhile.
What was reviewed:
FileHostwindows-quirk branch now falls through toPathwithout consumingc; verifiedPathhandles the reprocessed/,\\,?,#correctly andm_pathAfterLastSlashpoints at the leading/.FilePathStart's position check →ASSERT: confirmed all remaining entry points (empty-host and post-parseHostAndPortslash) satisfy the invariant.Filerelative-resolution: the newcopyBaseWindowsDriveLettercall is gated on the base path being a single segment, so non-drive-letter and multi-segment bases are unaffected.- Dot-segment quirk in
Pathis guarded bym_urlIsFileand empty-path, so non-file schemes and non-empty paths keep prior behavior.
Extended reasoning...
Overview
This PR fixes four spec-conformance gaps in WTF's URLParser::parse around the Windows drive-letter quirk for file: URLs, found by differential fuzzing against Ada/Node. It touches the File, FileHost, FilePathStart, and Path states of the parser state machine (~40 lines of logic changes in Source/WTF/wtf/URLParser.cpp) and adds an 82-row test in Tools/TestWebKitAPI/Tests/WTF/URLParser.cpp plus updates two existing ParserDifferences expectations.
Security risks
URL parsing is security-critical: it determines origin, host, and path boundaries used for file access, same-origin checks, and path normalization. The changes here are narrowly scoped to file: URLs with drive-letter-shaped first segments, and every branch is additive normalization (rewriting | → :, preserving a lone drive letter under ../relative resolution) that brings behavior toward the WHATWG spec and toward idempotence — which generally reduces parser-differential attack surface. I did not find any way for the new branches to relax host/path boundaries for non-file: schemes (all are gated on m_urlIsFile or reachable only from FileHost/File). Still, any change to a URL parser's state transitions carries nonzero risk of an unforeseen input shape.
Level of scrutiny
High. This is hand-written state-machine code with subtle offset bookkeeping (m_pathAfterLastSlash, m_hostEnd, m_portLength) where an off-by-one shows up only in downstream relative resolution, not in the serialized string. The PR also intentionally diverges from upstream WebKit's current behavior, so it will be carried as a fork patch until/unless upstreamed. The fork's CI does not build TestWebKitAPI, so the 82 new test rows and the ASSERT in FilePathStart are not exercised by CI — verification relies on the author's local ASAN debug run described in the PR body.
Other factors
The PR is exceptionally well-documented (spec citations at each change site, differential testing against whatwg-url 14 with 7932 generated inputs going from 736 → 0 mismatches, WPT urltestdata.json byte-identical). The test coverage is thorough, including negative cases (C|x, C||, 1|, non-first segments, http scheme). One state transition worth a second pair of eyes: FilePathStart's former runtime check currentPosition(c) == m_url.m_hostEnd + 1 is now an ASSERT — I traced every entry point and believe the invariant holds, but a reviewer familiar with the parser should confirm no other path can reach FilePathStart with a non-empty path prefix.
…quirk with a host, before ?/#, after dot segments, and as a relative base Pins WEBKIT_VERSION to the preview build of oven-sh/WebKit#462, which makes the URL parser apply the file: drive letter quirk whenever the path is empty and keep a lone drive letter when resolving a relative path against it: new URL("file://C|?q").href file:///C:/?q -> file:///C:?q new URL("file://host/C|/x").href file://host/C|/x -> file://host/C:/x new URL("file://localhost/C|/x").href file:///C|/x -> file:///C:/x (was not idempotent) new URL("file:///./C|").href file:///C| -> file:///C: (was not idempotent) new URL("x", "file:///C:").href file:///x -> file:///C:/x Adds the matching coverage to test/js/web/url/url.test.ts.
f17207c to
2a49cb8
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The four state-machine changes look correct against the WHATWG spec sections cited, and the 82-row test plus the whatwg-url differential give strong coverage. That said, this is non-trivial control-flow surgery inside URLParser::parse — a security-relevant parser — including a runtime check swapped for an assertion, so a human look would still be worthwhile.
Checked: the FileHost windows-quirk branch now falls through to State::Path without consuming c, and Path correctly handles the trailing /, \\, ?, # there. The new FilePathStart assertion holds on both entry paths (empty host and post-parseHostAndPort, where m_hostEnd/m_portLength are set before the transition). The copyBaseWindowsDriveLetter call in the File state is guarded by m_pathAfterLastSlash == hostEnd + portLength + 1, so multi-segment bases are unaffected. The Path-state dot-segment addition is gated on m_urlIsFile and an empty path buffer, so http URLs and non-empty paths are untouched.
Extended reasoning...
Overview
This PR fixes four spec deviations in WTF's URLParser::parse around the WHATWG URL Standard's Windows drive-letter quirk for file: URLs. It touches four states in the parser state machine:
File(relative resolution): aftercopyURLPartsUntil(PathAfterLastSlash), if the base path is a lone drive letter, re-append it viacopyBaseWindowsDriveLetter()so"x"againstfile:///C:yieldsfile:///C:/xinstead offile:///x.FileHost: when the "host" is a drive letter followed by/,?or#, append the drive letter, setm_pathAfterLastSlash, and reprocesscinState::Path(rather than falling through to the empty-host branch that appended an extra/). After a real host, transition toFilePathStart(notPathStart) when a slash follows so the drive-letter check runs.FilePathStart: thecurrentPosition == hostEnd + 1runtime guard becomes anASSERT(now always entered directly after the host).Path: after dot-segment removal leaves the path empty in afile:URL, applyappendWindowsDriveLetter()sofile:///./C|normalizes tofile:///C:.- EOF
FileHost:m_pathAfterLastSlashnow points just after the leading/(matchingfile:///C:), withm_pathEnd/m_queryEndat the buffer end.
Tests: two ParserDifferences expectations updated, and a new 82-row FileWindowsDriveLetterWithAuthority test covering hosts (named, uppercase, IPv4, IPv6, localhost), delimiters, backslashes, dot segments, relative resolution, and negative cases (C|x, C||, C%7C, 1|, second-segment drive letters, http URLs).
Security risks
URL parsing is security-sensitive: parser divergences can enable origin confusion, SSRF filter bypasses, and path-traversal-style issues. This change reduces divergence from the spec and from Node/Ada/whatwg-url, and the author verified idempotence and offset consistency across ~8k generated inputs. I did not spot any new hazard — the changes are narrowly scoped to file: + drive-letter shapes, all gated by m_urlIsFile / isWindowsDriveLetter / empty-path checks. But the blast radius of a mistake here (every new URL() in Bun) is high enough that it should not be auto-approved.
Level of scrutiny
High. URLParser::parse is a ~1000-line hand-written state machine with tightly coupled offset bookkeeping (m_hostEnd, m_portLength, m_pathAfterLastSlash, m_pathEnd), where an off-by-one shows up only in downstream relative resolution rather than in the serialized string. One change replaces a runtime guard with a debug assertion — I traced both entry paths to FilePathStart and the invariant holds, but this is the kind of thing a maintainer should confirm. This also intentionally diverges from upstream WebKit behavior.
Other factors
- The PR description is unusually rigorous: differential testing against whatwg-url 14 (7932 inputs, 736→0 diffs), WPT
urltestdata.jsonbyte-identical, all 12WTF_URLParsertests pass under ASAN with assertions enabled, and every test row cross-checked against Node 26. - The fork's CI doesn't build TestWebKitAPI, so the new tests won't run in CI — verification was done manually against prebuilts.
- No prior human review comments to address.
Given the sensitivity of the code path and the assertion change, deferring to a human reviewer despite finding no issues.
…quirk with a host, before ?/#, after dot segments, and as a relative base Pins WEBKIT_VERSION to the preview build of oven-sh/WebKit#462, which makes the URL parser apply the file: drive letter quirk whenever the path is empty and keep a lone drive letter when resolving a relative path against it: new URL("file://C|?q").href file:///C:/?q -> file:///C:?q new URL("file://host/C|/x").href file://host/C|/x -> file://host/C:/x new URL("file://localhost/C|/x").href file:///C|/x -> file:///C:/x (was not idempotent) new URL("file:///./C|").href file:///C| -> file:///C: (was not idempotent) new URL("x", "file:///C:").href file:///x -> file:///C:/x Adds the matching coverage to test/js/web/url/url.test.ts.
2a49cb8 to
f17207c
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes state-machine logic and offset bookkeeping in URLParser::parse — a core, security-adjacent component — a human look would still be worthwhile.
What was reviewed:
- The four state-machine edits (File, FileHost, FilePathStart, Path, and the EOF FileHost handler) against the WHATWG spec steps cited in the description.
- That
FilePathStartis now only reachable with*c == '/' || '\\'and directly after a host, so the newASSERTand dropped position guard are safe. - That the new
copyBaseWindowsDriveLettercall inState::Fileis gated on the base path being a single segment, so multi-segment bases are unaffected. - That the Path-state drive-letter check is confined to the dot-segment branch and gated on
m_urlIsFile, leaving non-file URLs and the fast path unchanged.
Extended reasoning...
Overview
This PR fixes four spec deviations in WTF's URLParser::parse around the WHATWG URL Standard's Windows drive-letter quirk for file: URLs. It touches Source/WTF/wtf/URLParser.cpp (the File, FileHost, FilePathStart, and Path states, plus the EOF FileHost handler) and adds an 82-row test in Tools/TestWebKitAPI/Tests/WTF/URLParser.cpp along with updating two existing ParserDifferences expectations. The changes align serialization and m_pathAfterLastSlash/m_pathEnd offsets with the spec, Node/Ada, and whatwg-url.
Security risks
URL parsing is security-adjacent: it feeds origin computation, relative resolution, and file-path derivation. The changes here are narrowly scoped to file: drive-letter handling and move behavior toward the spec (fixing non-idempotent serializations), which generally reduces risk. I did not find any way these edits could affect non-file: schemes, host parsing, or origin boundaries. Still, any change to this state machine warrants a human eye given its blast radius.
Level of scrutiny
High. URLParser.cpp is core infrastructure shared with upstream WebKit, and the change is non-mechanical: it restructures control flow (splitting the windowsQuirk branch out of the empty-host branch, redirecting FileHost → FilePathStart after a real host), converts a runtime position check into an ASSERT, and adjusts offset bookkeeping in the EOF handler. These are exactly the kinds of edits where an off-by-one or missed entry path could regress relative resolution in ways the string output alone doesn't reveal.
Other factors
The PR is unusually well-verified: all 12 WTF_URLParser tests pass under debug/ASAN, the 869-entry WPT urltestdata.json is byte-identical before/after, and a 7932-input differential run against whatwg-url 14 goes from 736 mismatches to 0. The description traces each fix to a specific spec step. The bug-hunting system found nothing. That said, the fork's CI does not build TestWebKitAPI, this intentionally diverges from upstream WebKit behavior, and the change is intricate enough that it does not meet the bar for auto-approval without a human reviewer signing off.
… is empty, and keep a bare drive letter when resolving against it
The Windows drive letter quirk of the URL Standard rewrites a first path segment
"C|" to "C:" in a file: URL, and a path consisting of such a segment alone is
never shortened. URLParser deviated from that in four places:
- file host state: "file://C|?q" and "file://C|#f" gained an extra empty
segment ("file:///C:/?q" instead of "file:///C:?q"). The drive letter is
now appended and the current code point is reprocessed in the path state,
as the specification says.
- After a host the parser went to PathStart, which does not know about drive
letters, so "file://host/C|/x" kept the "|" and "file://localhost/C|/x"
serialized as "file:///C|/x", which re-parses as "file:///C:/x". A slash
after a file host now goes through FilePathStart like an empty host does.
- Dot segments that leave the path empty disabled the quirk: "file:///./C|"
was "file:///C|" (also not idempotent). The path state now checks for a
drive letter after removing dot segments when nothing else is in the path.
- Resolving a relative path against a URL whose path is a drive letter alone
dropped the drive letter: "x" against "file:///C:" was "file:///x", and
against "file://C|" it was "file:///C:x" because the EOF case of the file
host state recorded pathAfterLastSlash past the drive letter. Both now give
"file:///C:/x".
The two ParserDifferences rows for "//C|?foo/bar" and "//C|#foo/bar" encoded
the first deviation and are updated. The new test covers each case and the
shapes that must stay unchanged (segments that are not drive letters, a drive
letter that is not the first segment, http URLs, other lone segments as bases).
f17207c to
9904593
Compare
…quirk with a host, before ?/#, after dot segments, and as a relative base Pins WEBKIT_VERSION to the preview build of oven-sh/WebKit#462, which makes the URL parser apply the file: drive letter quirk whenever the path is empty and keep a lone drive letter when resolving a relative path against it: new URL("file://C|?q").href file:///C:/?q -> file:///C:?q new URL("file://host/C|/x").href file://host/C|/x -> file://host/C:/x new URL("file://localhost/C|/x").href file:///C|/x -> file:///C:/x (was not idempotent) new URL("file:///./C|").href file:///C| -> file:///C: (was not idempotent) new URL("x", "file:///C:").href file:///x -> file:///C:/x Adds the matching coverage to test/js/web/url/url.test.ts.
2 participants
Problem
The file: drive letter quirk (
Source/WTF/wtf/URLParser.cpp) deviates from the URL Standard in four places. Node (Ada) and the whatwg-url reference implementation agree with the spec on all of them; upstream WebKit has the same behavior as this fork. Found by a differential run of URL against Ada (Bun's fuzz ledger). In Bun each of these isnew URL(...):Causes, all in
URLParser::parse:FileHost: when the "host" is a drive letter and?or#follows, the state appends/?or/#after the drive letter, adding an empty path segment. The spec (file host state step 1.1) reprocesses the code point in the path state, which adds no segment.FileHostafter a real host (includinglocalhost, which is dropped) goes toPathStart, and only the empty-host path (FilePathStart) looks for a drive letter. The spec's quirk (path state, "url's path is empty") has no host condition. The WPT rows with a host only useC:, which needs no rewriting, so this was not caught.Path: after.or..segments are removed the path can still be empty, but the segment that follows them was copied as is.File(relative path against a file: base): the base is copied up topathAfterLastSlash, which for a path of/C:is the slash, so the drive letter is removed. The spec's "shorten a URL's path" never removes a lone drive letter. The EOF case ofFileHostadditionally recordedpathAfterLastSlashafter the drive letter instead of after the slash, which is whyfile://C|as a base gavefile:///C:xrather than thefile:///xthatfile:///C:gave (the two parse to the same string and now have the same offsets).Fix
FileHost, drive letter followed by a slash,?or#: append the drive letter, setpathAfterLastSlashto just after the slash, and continue inPathwithout consuming the code point.Pathalready turns/and\into a separator and?/#into the query / fragment, sofile://C|/x,file://C|\xandfile://C|/..serialize exactly as before. The empty-host branch is unchanged apart from no longer sharing its condition with the quirk.FileHostafter a host: go toFilePathStartwhen the next code point is a slash (?and#still go throughPathStart, which inserts the/).FilePathStartis now only entered directly after a host, so its position check becomes an assertion.Path, after consuming dot segments: if the URL is a file: URL, the path so far is just the leading slash, and a drive letter follows, append it throughappendWindowsDriveLetter(). This is inside the existing dot-segment branch, so the common path is not affected.File, aftercopyURLPartsUntil(PathAfterLastSlash): if the base's path is a single segment and that segment is a drive letter, keep it and append a/(copyBaseWindowsDriveLetter(), the helperFileSlashalready uses for/xagainst such a base). SincepopPath()already refuses to pop a first-segment drive letter,..and.againstfile:///C:givefile:///C:/, as in the spec. The EOF case ofFileHostnow records the same offsets as parsingfile:///C:does.Tools/TestWebKitAPI/Tests/WTF/URLParser.cpp. The twoParserDifferencesrows for//C|?foo/barand//C|#foo/barencoded the extra segment and now expectfile:///C:?foo/bar/file:///C:#foo/bar(they match the existing WPT rowsC|?andC|#against a file base, which givefile://host/C:?/file://host/C:#). The newFileWindowsDriveLetterWithAuthoritytest has 82 rows for the four cases, with hosts of every kind (host, uppercase, IPv4, IPv6,localhost), and the shapes that must not change:C|x,C||,C%7C,1|, a drive letter as the second segment or after//, http URLs,?/#directly after a host, and non-drive-letter lone segments as bases. Every row's expectation was checked against Node 26 before running it here; all rows also run with a tab inserted at every position, which covers thetakesTwoAdvancesUntilEnd/advancehandling in the quirk.URLParser.cppwere compiled with the flags from theautobuild-cb61607fdebug ASAN prebuilt'scompile_commands.json(the branch is now rebased onto1cb96a7b, Bun's current pin, whose WTF URL sources are identical tocb61607f; that upstream merge added twoWTF_URLParsertests, which pass before and after; the same run againstautobuild-eeab0404andautobuild-c6cfe90c, the earlier bases, gave identical numbers) and linked against that prebuilt'slibWTF.a, once as is and once with thisURLParser.cppin front of it. Before:ParserDifferencesand the new test fail (2672 failed expectations, including the tab variants). After: all 12WTF_URLParsertests pass, withASSERT_ENABLEDand the new assertion active.urltestdata.json(869 entries) gives byte-identical results before and after. A generated set of 7932 file: inputs around the quirk (13 authority prefixes x drive-letter-like segments x suffixes, plus 26 relative inputs against 30 drive-letter bases) compared against whatwg-url 14: 736 differ before this change, 0 after, and no result is non-idempotent or has inconsistent offsets. Against Node the same set leaves 52 differences, all of them Ada treatingfile:///C:xas a drive letter (file:///C:x/..givesfile:///C:x/in Node andfile:///in the spec, whatwg-url, and WTF before and after this change).Background
:or|(and then a slash,?,#or the end) is a drive letter, the|is rewritten to:, and a path that consists of that segment alone is never shortened by..or by relative resolution.file://C|/xis the same quirk seen from the file host state: the drive letter is taken as the path of a host-less URL, not as a host.URLParserwrites the serialization into a buffer as it goes and records offsets in theURL(m_hostEnd,m_pathAfterLastSlash,m_pathEnd, ...).m_pathAfterLastSlashis where a relative path is spliced onto a base, so an offset that is off by a segment shows up as a relative-resolution bug rather than in the string itself;URLParser::internalValuesConsistent()(checked by every test row) only verifies the offsets are ordered, not that they point at the right segment boundary.syntaxViolation()); every branch touched here is after such a point, so the fast path for already-canonical URLs is unchanged. Bun pins this fork by commit inscripts/build/deps/webkit.ts; the matching Bun tests will come with the bump to this change.Reproducing the verification
Prebuilt:
autobuild-eeab04040fa61fd595695980f9d054b7fc0ed855,bun-webkit-linux-amd64-debug-asan(earlier run:autobuild-c6cfe90c6064bd80a1916b844c2f092735cfc720, same results).Tests/WTF/URLParser.cppwas compiled with theTestWTFflags from itscompile_commands.json(minus the header maps and PCH) plus the bundled gtest fromSource/ThirdParty/gtest;Source/WTF/wtf/URLParser.cppwith theWTFflags; both linked with the prebuilt'slibWTF.a,libbmalloc.aand ICU. The WPT and generated-input drivers callURL(String)/URL(base, String)and compareurl.string()(orisValid()for failure rows), and re-parse every result to check idempotence.