url: reject an empty string base in the URL constructor - #33210
Conversation
The constructor binding dispatched on base.isEmpty() to decide whether a base argument was supplied, but a WTF null String (base was undefined) and a real empty string are both isEmpty(). An empty string base was silently treated as no base, so new URL(absolute, "") succeeded while URL.parse and URL.canParse (which use isNull()) rejected the same input. Use isNull(), matching DOMURL::parse/canParse and the URL spec: a provided base is always parsed, and an empty string is not a valid URL.
|
Status: the diff is green and ready for a maintainer. The CI red on this PR is Reproduced on a stock 1.4.0 release build and on a debug build of current main: new URL("http://x/", "").href // "http://x/" (should be a TypeError)
URL.canParse("http://x/", "") // false
URL.parse("http://x/", "") // nullVerified with the debug build: About CI, with a correction to my earlier analysis here. In build 67745 the dominant failure is
I pushed one |
|
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 (1)
WalkthroughThis PR changes URL base handling so ChangesURL base handling fix
Compact metadata: URL constructor behavior and matching tests updated. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/web/url/url.test.ts (1)
22-39: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider covering explicit
nullbase, not just omitted/undefined.The underlying fix distinguishes
base.isNull()(only true when the JS argument isundefined/omitted) from a real base string. Passing a JSnullliteral explicitly goes throughIDLUSVStringconversion and coerces to the string"null"— a different code path than omission, and arguably the more interesting "null vs empty" case implied by the PR title. Consider adding a case fornew URL("http://example.com/", null)(and theURL.canParse/URL.parseequivalents) to confirm it's treated as a real (and here, invalid) base rather than "no base".🧪 Suggested additional test case
it("treats only an undefined base as no base", () => { expect(new URL("http://example.com/", undefined).href).toBe("http://example.com/"); expect(new URL("http://example.com/").href).toBe("http://example.com/"); expect(URL.canParse("http://example.com/", undefined)).toBe(true); expect(URL.parse("http://example.com/", undefined)?.href).toBe("http://example.com/"); }); + + it("treats an explicit null base as the string \"null\", not as no base", () => { + // `@ts-expect-error` - intentionally passing null to test WebIDL USVString coercion + expect(() => new URL("http://example.com/", null)).toThrow(expect.objectContaining({ code: "ERR_INVALID_URL" })); + // `@ts-expect-error` + expect(URL.canParse("http://example.com/", null)).toBe(false); + // `@ts-expect-error` + expect(URL.parse("http://example.com/", null)).toBeNull(); + });Based on the upstream constructor snippet in
src/jsc/bindings/webcore/JSDOMURL.cpp(Lines 155-172), which special-casesargument1.value().isUndefined()distinctly from other falsy/null-ish values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/js/web/url/url.test.ts` around lines 22 - 39, The URL tests only cover omitted/undefined base handling, but not an explicit null base, which follows a different code path. Add a case in the URL constructor tests using new URL("http://example.com/", null) and the matching URL.canParse/URL.parse assertions to verify null is treated as a real invalid base rather than “no base”. Keep the existing treats only an undefined base as no base coverage and update the expectations to reflect the distinction made by JSDOMURL constructor handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test/js/web/url/url.test.ts`:
- Around line 22-39: The URL tests only cover omitted/undefined base handling,
but not an explicit null base, which follows a different code path. Add a case
in the URL constructor tests using new URL("http://example.com/", null) and the
matching URL.canParse/URL.parse assertions to verify null is treated as a real
invalid base rather than “no base”. Keep the existing treats only an undefined
base as no base coverage and update the expectations to reflect the distinction
made by JSDOMURL constructor handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d8e274c8-2eed-4934-a67b-ad2a2d38ea10
📒 Files selected for processing (1)
test/js/web/url/url.test.ts
WebIDL coerces an explicit null base to the string "null", which is not a valid URL, so the constructor, URL.canParse, and URL.parse all reject it as an invalid base rather than treating it as no base.
|
Added in 1492d92. An explicit new URL("http://example.com/", null)
// TypeError: "http://example.com/" cannot be parsed as a URL against "null" (ERR_INVALID_URL)
URL.canParse("http://example.com/", null) // false
URL.parse("http://example.com/", null) // nullThis was already correct before the fix, since One deviation from the suggested diff: I used a bare |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-token spec fix (isEmpty() → isNull()) aligning the URL constructor with URL.parse/URL.canParse, with solid coverage.
Extended reasoning...
Overview
This PR changes a single predicate in JSDOMURLDOMConstructor::construct (src/jsc/bindings/webcore/JSDOMURL.cpp) from base.isEmpty() to base.isNull() when deciding whether to call the one- or two-argument DOMURL::create. It adds three test cases to test/js/web/url/url.test.ts covering "", undefined, and null as the base argument across new URL, URL.parse, and URL.canParse.
I verified against src/jsc/bindings/DOMURL.cpp: the two-argument DOMURL::create(url, base) and parseInternal (backing parse/canParse) both gate on !base.isNull() && !baseURL.isValid(), so an empty-but-non-null base string correctly hits the invalid-base error there. The old isEmpty() check in the constructor binding conflated a null WTF String() (produced when the JS argument is undefined) with a real "", silently dropping the empty base. Switching to isNull() restores parity with the statics and matches the WHATWG URL spec, Node, Chrome, and Safari.
Security risks
None. This tightens validation (an input that previously succeeded now throws ERR_INVALID_URL), which is strictly safer. No auth, crypto, or filesystem paths are touched.
Level of scrutiny
Low. This is effectively a one-word change in a WebCore binding whose correctness is directly verifiable against both the sibling code paths in DOMURL.cpp and the URL Standard. The behavioral change is narrow (only new URL(x, "") is affected) and moves from non-conformant to spec-conformant.
Other factors
- No CODEOWNERS coverage for the touched files.
- Tests assert exact error messages and
ERR_INVALID_URLcode, plus cross-check the constructor againstcanParse/parseon the same inputs, and pin down the preservedundefinedand already-correctnullbehavior — good regression coverage for the whole variant matrix. - The bug hunter found no issues.
- No prior human reviews or outstanding comments to address.
There was a problem hiding this comment.
LGTM — one-token predicate swap (isEmpty() → isNull()) that aligns the constructor with DOMURL::parse/canParse and the spec, with tests covering the fix and both adjacent guards.
Extended reasoning...
Overview
A single-line change in src/jsc/bindings/webcore/JSDOMURL.cpp: the base-argument dispatch in JSDOMURLDOMConstructor::construct switches from base.isEmpty() to base.isNull(). The rest is three new test cases in test/js/web/url/url.test.ts and a two-line explanatory comment.
I verified against src/jsc/bindings/DOMURL.cpp: the two-arg DOMURL::create(url, base) overload already guards on !base.isNull() && !baseURL.isValid(), and parseInternal (backing URL.parse/URL.canParse) uses the identical isNull() predicate. So the constructor was the only outlier, and this change makes all three entry points agree — exactly as the URL spec requires (a provided base is always parsed, and "" is not a valid URL).
Security risks
None. This tightens validation (previously-accepted new URL(abs, "") now throws), which is the spec-mandated and Node/Chrome/Safari behavior. No new parsing, no memory management, no allocation — just a predicate swap on a local WTF::String.
Level of scrutiny
Low. The functional change is one identifier on one line in an originally-generated binding file, bringing it into line with its siblings. The undefined → null-String() sentinel path two lines above is unchanged, so the no-base case is preserved (and now explicitly tested). Blast radius is limited to the exact edge case being fixed.
Other factors
Test coverage is thorough for the size of the change: the failing case (empty-string base), the must-still-work case (undefined/omitted base), and the adjacent null → "null" WebIDL coercion are all pinned across the constructor, URL.parse, and URL.canParse. The PR description confirms the new test fails on the unfixed build. The bug-hunting system found no issues, and there are no outstanding human review comments.
1 participant
Problem
new URL(absolute, "")succeeds when it should throw, and it disagrees with Bun's ownURL.parseandURL.canParseon the same input:Per the URL spec, a provided base is always run through the URL parser, and an empty string is not a valid URL, so the constructor must throw even when the input is absolute. This breaks the
canParse(x, base) && new URL(x, base)validation pattern: the guard says invalid and the constructor then accepts the construction anyway.Cause
JSDOMURLDOMConstructor::constructdispatched onbase.isEmpty()to decide whether a base was supplied:A WTF null
String(the base argument wasundefined) and a real""are bothisEmpty(), so an empty string base was silently treated as no base.DOMURL::parseandDOMURL::canParsealready use the correctisNull()predicate, which is why the statics disagreed with the constructor. TheisEmpty()dispatch was introduced in #10129; before that the binding always called the two argumentDOMURL::create(url, base), which checksbase.isNull()itself.Fix
Use
isNull(), matchingDOMURL::parse/canParseand the nullString()sentinel the other in-treeDOMURL::createcallers already pass for "no base".new URL(x)andnew URL(x, undefined)keep the single argument path and its shorter error message, so the existing error message assertions are unchanged.Verification
Tests added to
test/js/web/url/url.test.ts:rejects an empty string basecovers the constructor,URL.parse, andURL.canParseagreeing on the same input. It fails on the unfixed build (the constructor returns aURLinstead of throwing) and passes with the fix.treats only an undefined base as no baseguards the missing /undefinedbase paths that must keep working.treats an explicit null base as an invalid base, not as no basedocuments the WebIDL coercion of a JSnullto the string"null", a real (and invalid) base that all three entry points reject. This path was already correct and is unchanged by the fix; it pins down the remaining spelling of the base argument.All existing tests in
test/js/web/url/andtest/js/node/url/pass. The only failures in those directories are two pre-existing 5s timeouts on loop tests that are slow under the debug + ASAN build (pathToFileURL doesn't leak memory,URL.canParse repeatedly called produces same result); they time out identically with and without this change and never reach the constructor's base dispatch.