Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded WebIDL-style validation for the optional Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 6:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit 3019519 has some failures in 🧪 To try this PR locally: bunx bun-pr 29198That installs a local version of the PR into your bun-29198 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean, spec-compliant fix with good test coverage. One inline nit about the error message omitting null, but the runtime behavior is correct.
Extended reasoning...
Overview
This PR fixes a spec compliance bug in fetchImpl (src/bun.js/webcore/fetch.zig). The change adds ~9 lines to explicitly handle the init argument as a Web IDL dictionary: undefined/null → treated as omitted, object/DOMWrapper → used as options, any other primitive → TypeError. A new regression test file covers all the relevant cases (number, bigint, string, boolean, symbol must reject; undefined, null, plain object must not reject).
Security risks
None. This is a validation tightening with no auth, crypto, or permission implications.
Level of scrutiny
Low. The change is small, isolated to a single brk: block in fetchImpl, follows the existing pattern of returning JSPromise.dangerouslyCreateRejectedPromiseValueWithoutNotifyingVM, and aligns the implementation with the WHATWG Fetch + Web IDL spec.
Other factors
The one bug found (error message says "must be of type object or undefined" without mentioning null) is a pure developer-experience nit — the runtime behavior of accepting null is already correct and is verified by the test. This doesn't warrant blocking the PR. Test coverage is thorough and well-structured.
There was a problem hiding this comment.
LGTM — my prior feedback on the error message was addressed in c9de504.
Extended reasoning...
Overview
This PR modifies () to throw a when the argument is a non-nullish primitive (number, bigint, string, boolean, symbol), matching WHATWG Fetch spec / Web IDL dictionary conversion semantics. A regression test file () is added covering all relevant cases. The diff is 9 lines of logic and 44 lines of tests.
Security risks
None. This is a pure input-validation tightening — it rejects more inputs than before (previously silently ignored, now rejected with TypeError). No auth, crypto, or permission code is touched.
Level of scrutiny
Low. The change is a targeted three-way branch (undefined/null → pass through, object/DOMWrapper → use as options, anything else → TypeError), directly following the spec. It follows the existing error-handling pattern used elsewhere in the same function and is fully covered by the new regression tests.
Other factors
My earlier inline comment about the error message omitting was resolved in the follow-up commit (c9de504). The bug hunter flagged a pre-existing gap in () that is out of scope for this PR and should be tracked as a separate issue. No outstanding reviewer comments remain unaddressed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29195.test.ts`:
- Around line 28-34: The tests use Jest-style .rejects assertions but Bun
expects the async throw form; update both cases to use the Bun pattern by
wrapping the async call in a function passed to expect and calling
toThrow(TypeError) — e.g. replace await expect(fetch(url, value as
any)).rejects.toBeInstanceOf(TypeError) with expect(async () => { await
fetch(url, value as any); }).toThrow(TypeError), and similarly replace await
expect(fetch(url, Symbol("test") as any)).rejects.toBeInstanceOf(TypeError) with
expect(async () => { await fetch(url, Symbol("test") as any);
}).toThrow(TypeError); keep the same test names and use the same variables
(bad_init, url, fetch) so only the assertion form changes.
- Around line 36-47: The test using test.each(good_init) relies on a network/DNS
failure to avoid TypeError coupling; change the fetch call to use a
deterministic always-success URL (e.g., a data: URL) instead of url so the
outcome is not tied to network error shapes, then assert that calling fetch with
each value does not reject with a TypeError (or simply does not throw); update
the block around the fetch(...) in the test callback (the async (_label, value)
=> { ... } function) to fetch a guaranteed-success resource and adjust the
expectation to assert no TypeError is thrown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 14acac2c-f49b-4d3f-8fd3-f5ebd458b8f6
📒 Files selected for processing (2)
src/bun.js/webcore/Request.zigtest/regression/issue/29195.test.ts
There was a problem hiding this comment.
LGTM — both prior issues fixed, remaining nits are no-ops with no behavioral impact.
Extended reasoning...
Overview
The PR adds spec-compliant TypeError rejection in both fetchImpl (fetch.zig) and constructInto (Request.zig) when the init argument is a non-nullish primitive. A regression test file covers numbers, bigint, strings, booleans, symbols, and valid values (undefined, null, object).
Security risks
None. This is a validation change that makes Bun throw sooner on invalid input, matching browser and Node.js behavior.
Level of scrutiny
Low. The changes are small, self-contained, and follow the established pattern in the codebase. Both code paths are early-exit guards before any resource allocation occurs.
Other factors
Both issues flagged in prior reviews have been resolved: the error message now correctly lists 'undefined, or null', and Request.zig received the same guard. The two remaining bug reports are harmless nits: a vestigial dead-code is_error = true assignment (pre-existing pattern, zero behavioral impact) and a slightly inaccurate test comment. Test coverage is thorough.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/regression/issue/29195.test.ts (1)
30-35: 🧹 Nitpick | 🔵 TrivialPrefer Bun’s async rejection assertion form (
toThrow) over.rejects.Line 31 and Line 35 still use Jest-style
.rejects. In Bun tests, the preferred pattern is wrapping the async call and asserting with.toThrow(TypeError).Suggested diff
test.each(bad_init)("rejects with TypeError when init is %s", async (_label, value) => { - await expect(fetch(url, value as any)).rejects.toBeInstanceOf(TypeError); + await expect(async () => { + await fetch(url, value as any); + }).toThrow(TypeError); }); test("rejects with TypeError when init is a symbol", async () => { - await expect(fetch(url, Symbol("test") as any)).rejects.toBeInstanceOf(TypeError); + await expect(async () => { + await fetch(url, Symbol("test") as any); + }).toThrow(TypeError); });#!/bin/bash set -euo pipefail # Verify current assertion form in this file rg -nP --type=ts '\.rejects\.' test/regression/issue/29195.test.ts # Sample nearby regression-test conventions using async wrapper + toThrow rg -nP --type=ts 'expect\s*\(\s*async\s*\(\)\s*=>\s*\{' test/regression/issue -g '*.test.ts' -m 20 rg -nP --type=ts '\)\.toThrow\(' test/regression/issue -g '*.test.ts' -m 20Based on learnings, in Bun tests async rejection assertions should use
expect(async () => { await ... }).toThrow().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/regression/issue/29195.test.ts` around lines 30 - 35, Replace the Jest-style `.rejects.toBeInstanceOf(TypeError)` assertions with Bun's async wrapper + `.toThrow(TypeError)` form: wrap the async call to `fetch(url, value as any)` used in the `test.each(bad_init)` case and the standalone symbol case in an `async () => { await fetch(...) }` arrow function and assert `expect(async () => { await fetch(...) }).toThrow(TypeError)` so both the `test.each` callback and the `"rejects with TypeError when init is a symbol"` test use Bun's preferred `toThrow` pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/regression/issue/29195.test.ts`:
- Around line 30-35: Replace the Jest-style `.rejects.toBeInstanceOf(TypeError)`
assertions with Bun's async wrapper + `.toThrow(TypeError)` form: wrap the async
call to `fetch(url, value as any)` used in the `test.each(bad_init)` case and
the standalone symbol case in an `async () => { await fetch(...) }` arrow
function and assert `expect(async () => { await fetch(...)
}).toThrow(TypeError)` so both the `test.each` callback and the `"rejects with
TypeError when init is a symbol"` test use Bun's preferred `toThrow` pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f776276c-8a81-4f53-8ddf-b2e8f9fc3912
📒 Files selected for processing (1)
test/regression/issue/29195.test.ts
There was a problem hiding this comment.
LGTM — all substantive issues from prior reviews have been addressed.
Extended reasoning...
Overview
This PR adds a three-way check on the fetch() and new Request() init argument: undefined/null are treated as absent, objects (including DOMWrapper) are used as options, and any other primitive immediately throws a TypeError. Changes touch fetch.zig (~10 lines), Request.zig (~7 lines), and a new regression test file.
Security risks
None. This is a validation tightening that throws earlier on invalid input — no auth, crypto, or permission paths are touched.
Level of scrutiny
Low. The fix is mechanically straightforward and closely follows the pattern already used throughout fetchImpl. The spec reference (Web IDL dictionary conversion) is accurate, and the test coverage is comprehensive and hermetic (uses data: URLs).
Other factors
Both issues I raised in previous review rounds were fixed. The residual is_error = true assignment is a pre-existing dead-code pattern spanning 40+ call sites across the function — out of scope here. The CodeRabbit test-style suggestion was a labeled nitpick. CI failures in fetch.upgrade.test.ts on aarch64 appear pre-existing and unrelated to these changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/webcore/Request.zig`:
- Around line 646-651: The init-type guard in Request.zig is currently
validating arguments[1] too early and can mask exceptions thrown during
first-argument stringification; remove or relocate the existing if-block that
checks arguments[1].isUndefinedOrNull() / isObject() and instead re-insert that
exact validation immediately after the first-argument URL/stringifier conversion
(the constructInto path that reads .url/toString()) completes, but before any
subsequent reads from arguments[1]; keep the same error message and behavior so
the TypeError still fires for invalid init values but only after left-to-right
conversion of the first argument.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a1d9338c-9e16-43ac-b9a8-77ffddbd4e0d
📒 Files selected for processing (2)
src/bun.js/webcore/Request.zigsrc/bun.js/webcore/fetch.zig
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/bun.js/webcore/Request.zig (1)
646-650:⚠️ Potential issue | 🟠 Major
initvalidation is still ordered too early and can mask first-argument conversion errors.On Line 648, this check runs before the object-input URL/toString conversion path (Lines 759-773).
new Request({ toString() { throw new Error("boom"); } }, 0 as any)will throw the"init"TypeErrorinstead of"boom".Suggested fix (move validation after first-arg URL/string conversion path)
- // https://fetch.spec.whatwg.org/#dom-request — init is a Web IDL dictionary. - // Checked after the url-string branch so an invalid-URL error surfaces first. - if (arguments.len > 1 and !arguments[1].isUndefinedOrNull() and !arguments[1].isObject()) { - return globalThis.ERR(.INVALID_ARG_TYPE, "Failed to construct 'Request': The \"init\" argument must be of type object, undefined, or null.", .{}).throw(); - } const values_to_try_ = [_]JSValue{ if (arguments.len > 1 and arguments[1].isObject()) arguments[1] @@ if (!fields.contains(.url)) { if (try value.fastGet(globalThis, .url)) |url| { req.url = try bun.String.fromJS(url, globalThis); @@ if (globalThis.hasException()) return error.JSError; } + + // Validate `init` only after first-argument URL/toString conversion has run. + if (`@intFromEnum`(value) == `@intFromEnum`(values_to_try[values_to_try.len - 1]) and + arguments.len > 1 and + !arguments[1].isUndefinedOrNull() and + !arguments[1].isObject()) + { + return globalThis.ERR(.INVALID_ARG_TYPE, "Failed to construct 'Request': The \"init\" argument must be of type object, undefined, or null.", .{}).throw(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/webcore/Request.zig` around lines 646 - 650, The init-argument type check runs too early in the Request constructor and can mask errors from converting the first argument (e.g., when the first arg is an object with toString that throws); move the block that validates arguments[1] (the "init" object-type check that returns ERR(.INVALID_ARG_TYPE, ...)) to after the first-argument URL/string conversion path (the code handling object-input URL/toString conversion in Request.zig), so the first-arg conversion executes and can propagate its errors before validating the second argument; update any comments accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/bun.js/webcore/Request.zig`:
- Around line 646-650: The init-argument type check runs too early in the
Request constructor and can mask errors from converting the first argument
(e.g., when the first arg is an object with toString that throws); move the
block that validates arguments[1] (the "init" object-type check that returns
ERR(.INVALID_ARG_TYPE, ...)) to after the first-argument URL/string conversion
path (the code handling object-input URL/toString conversion in Request.zig), so
the first-arg conversion executes and can propagate its errors before validating
the second argument; update any comments accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4368bcfc-e7c8-4e33-83f9-ae8067b3fb23
📒 Files selected for processing (3)
src/bun.js/webcore/Request.zigsrc/bun.js/webcore/fetch.zigtest/js/web/fetch/fetch.test.ts
3df2c51 to
0445170
Compare
8830f06 to
ee0efe7
Compare
|
The diff is green where it matters: cargo clippy + all build-rust lanes pass, and every Linux test lane including |
|
CI status: cargo clippy + all build-rust lanes green, all Linux test lanes green including |
1a84215 to
ebcba4f
Compare
There was a problem hiding this comment.
The parse error from db39877's predecessor is fixed and I found no new issues, but this is a user-visible behavioral change to fetch()/Request with manual ref-counting in the live Rust path — worth a maintainer's eyes before merge.
Extended reasoning...
Overview
This PR adds WebIDL-compliant validation so that fetch(url, init) and new Request(url, init) reject/throw TypeError when init is a non-nullish primitive (number, string, boolean, bigint, symbol). It touches the live Rust implementations in src/runtime/webcore/fetch.rs (fetch_impl) and src/runtime/webcore/Request.rs (construct_into), keeps the non-compiled .zig siblings in sync as porting references, and adds ~50 lines of tests to test/js/web/fetch/fetch.test.ts.
Since my last review, commit db398775 restored the }); that closed the preceding it() before the new describe block — fetch.test.ts now parses cleanly (verified with esbuild). The bug-hunting pass on the current head found nothing.
Security risks
None identified. The change tightens input validation on a public API; it does not touch auth, crypto, networking transport, or filesystem access. The only resource concern was the url_str_optional WTFStringImpl ref, and the explicit .deref() before the early return in fetch.rs handles it.
Level of scrutiny
Moderate-to-high. fetch() and the Request constructor are among the most heavily exercised code paths in Bun, and this is a user-visible behavioral change — code that previously passed e.g. fetch(url, 0) and silently succeeded will now reject. The Rust change also threads through manual ref-counting (bun_core::String is Copy with no Drop) and the Promise-vs-sync-throw distinction, both of which were gotten wrong earlier in this PR's history and fixed via review.
Other factors
The PR has been through roughly a dozen review iterations (error-message wording, sibling Request path, argument-ordering, test placement, the .zig→.rs port, a string leak, sync-throw vs. reject, and the test-file parse error), each of which was addressed. Test coverage for the new behavior is solid (8 bad-init × 2 APIs + 3 good-init × 2 APIs + an ordering test). The robobun CI status still shows unrelated -no-pie linker warnings on android/freebsd build-rust lanes; the test lanes for db398775 aren't yet reflected in the status comment. Given the behavioral surface area and the history, a maintainer sign-off is the right call rather than a bot approval.
|
CI on db39877: 71 lanes pass, cargo clippy passes, and every test lane that got a runner is green. The 2 red lanes are |
db39877 to
d768e8f
Compare
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
There was a problem hiding this comment.
All prior feedback is addressed and I found nothing new this round; the change looks correct and well-tested, but since it adds an early-return with manual BunString deref inside fetch_impl (and a guard in Request::construct_into), a maintainer should give it a final look before merge.
Extended reasoning...
Overview
This PR adds WebIDL-compliant init argument validation to fetch() and new Request(): non-nullish primitives now reject/throw TypeError instead of being silently ignored. The final diff touches src/runtime/webcore/fetch.rs (~35 lines: defer init_arg capture, then a new validation block after StringOrURL::from_js that derefs url_str_optional and returns a rejected promise), src/runtime/webcore/Request.rs (~17 lines: a bail! guard after the url-string branch), and test/js/web/fetch/fetch.test.ts (a new describe block with 8 bad-init + 3 good-init cases for both fetch and Request, plus an argument-ordering test). The two docs/*.mdx changes are pure autofix.ci whitespace/table-padding reflow with no content change.
Security risks
None. The change strictly adds an input-validation error path; the happy path (object/undefined/null init) is byte-for-byte unchanged. No auth, crypto, or permission surfaces are involved.
Level of scrutiny
Moderate-to-high. fetch_impl is one of Bun's hottest production code paths and uses manual bun_core::String refcounting (Copy + no Drop). The new early return correctly calls s.deref() on url_str_optional before bailing and returns a rejected promise (not a sync throw) — both of which were earlier review findings that have been fixed and verified — but this is exactly the kind of lifetime-sensitive edit a Bun maintainer should sign off on rather than a bot.
Other factors
This PR has been through ~15 review iterations; every prior inline finding (error-message wording, sibling Request path, WebIDL ordering, url_str leak, sync-throw vs reject, test placement, parse error from rebase, stale .zig comment) is resolved in the current diff. The bug-hunting system found nothing new this run. The one CI failure (test-net-connect-memleak.js on two Linux x64 lanes) is unrelated to fetch/Request. No CODEOWNERS entry covers src/runtime/webcore/. Given the critical path + manual refcount handling, I'm deferring rather than approving.
|
The fix is complete and verified, and all review threads are resolved. The diff is green on the lanes that exercise this change: on build 66849 every Linux test lane passed, including The remaining red lanes are unrelated to this fetch/Request change:
Since the only blocker on the relevant platforms is a recurring infra timeout a re-run will not reliably clear, this needs a maintainer to merge. Happy to rebase if it goes stale. |
5558731 to
2767d18
Compare
|
Rebased onto latest main and force-pushed. Squashed the iteration history into a single commit and dropped two unrelated autofix.ci docs reflow files ( CI on the rebased commit (build 70438): green on all Linux lanes including |
There was a problem hiding this comment.
LGTM — the last nit (stale .zig reference in the comment) was addressed in the rebase, and the squashed diff is small, focused, and well-tested.
Extended reasoning...
Overview
This PR makes fetch(url, init) and new Request(url, init) reject/throw a TypeError when init is a non-nullish primitive, per the Web IDL dictionary conversion rules. It touches three files: src/runtime/webcore/fetch.rs (~40 net lines in fetch_impl — defer reading init, validate after first-arg conversion, reject with a promise, explicitly deref() the borrowed URL string on the early return), src/runtime/webcore/Request.rs (~17 lines in construct_into — same three-way check after the url-string branch, routed through the existing bail! cleanup), and test/js/web/fetch/fetch.test.ts (~50 lines — a hermetic describe block with 8 bad-init and 3 good-init cases for each of fetch() and new Request(), plus a left-to-right ordering assertion).
Security risks
None. This tightens input validation on a public API by rejecting inputs that were previously silently ignored. No new parsing, no auth/crypto/permissions surface, no untrusted data flowing into new sinks.
Level of scrutiny
Moderate — fetch_impl and Request::construct_into are core runtime paths with manual refcount management, so the leak/ordering concerns raised earlier were real. But the final diff is small and follows the exact patterns already used by adjacent validation errors in the same function (ctx.to_type_error + dangerously_create_rejected_promise_value_without_notifying_vm, and bail!(Err(global_this.err(...).throw())) respectively). The explicit url_str_optional.deref() before the early return mirrors the OwnedString wrap comment immediately below it.
Other factors
This PR has been through ~15 review rounds; every prior finding (missing null in the message, sibling Request path, url_str leak, sync-throw vs. rejected-promise, test placement, brace mismatch, stale .zig comment reference) is resolved in the current squashed diff. The bug-hunting pass on the current HEAD found nothing. Tests are hermetic (data: URL, no network), use it.each over the primitive matrix, and live in the correct module test file. Prior CI runs on the same code (pre-squash) were green on all Linux lanes including ASAN; the only red was unrelated Windows/macOS infra.
There was a problem hiding this comment.
LGTM — small spec-compliance fix, all prior review feedback addressed and no issues found in the current revision.
Extended reasoning...
Overview
This PR adds WebIDL-compliant validation for the init argument to fetch() and new Request(): when init is a non-nullish primitive (number, string, boolean, bigint, symbol), Bun now rejects/throws TypeError instead of silently ignoring it, matching the WHATWG Fetch spec, browsers, and Node.js. The change touches three files: src/runtime/webcore/fetch.rs (~40 lines net: defers init capture until after first-arg conversion, adds a three-way type check that returns a rejected promise with explicit url_str_optional.deref() on the error path), src/runtime/webcore/Request.rs (~17 lines: adds the same check after first-arg ToString via the existing bail! cleanup macro), and test/js/web/fetch/fetch.test.ts (~50 lines: 23 hermetic test cases using a data: URL).
Security risks
None. This tightens input validation on a user-facing API — previously-accepted garbage input now errors early. No new parsing, no untrusted-data handling, no auth/crypto/permission surface. The only resource-management concern (the +1 WTFStringImpl ref from StringOrURL::from_js) is explicitly released on the new early-return path, and the Request.rs path routes through the existing bail! → finalize_without_deinit cleanup which I verified derefs req.url via OwnedStringCell::set.
Level of scrutiny
Medium. fetch() and Request() are critical user-facing hot paths, but the actual change is a small, mechanical three-way type check that follows the exact patterns already used throughout both functions (dangerously_create_rejected_promise_value_without_notifying_vm for fetch validation errors; bail! for Request constructor errors). The behavioral change — rejecting on primitive init — is spec-mandated and matches every other runtime, so breakage risk for real code is minimal.
Other factors
This PR has been through an unusually thorough review cycle: 17 inline review threads over ~3 months, all resolved, covering error-message wording, sibling-path coverage (Request constructor), WebIDL argument-conversion ordering, a memory leak on the early-return path, sync-throw vs. rejected-promise semantics, test-file placement, a syntax error from a bad splice, and stale comment references — every one of which is fixed in the current squashed diff. I verified the final state against each: the .zig reference is gone, the comment in Request.rs carries the corrected wording, test/regression/issue/29195.test.ts is deleted, tests live in fetch.test.ts and use a hermetic data: URL, and the ordering test is delivery-mode agnostic. The bug-hunting system found nothing in the current revision. Test coverage is comprehensive (8 bad-init × 2 APIs, 3 good-init × 2 APIs, plus an ordering test). The author reports all 23 tests pass locally after the rebase; CI on prior revisions was green on all lanes that exercise this code (including ASAN).
2767d18 to
849cc76
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
849cc76 to
0dc26a1
Compare
fetch() and new Request() silently ignored a non-nullish primitive init argument (number, bigint, string, boolean, symbol) instead of failing. Per the WHATWG Fetch spec (Web IDL dictionary conversion), a non-object init must produce a TypeError: fetch() rejects and new Request() throws, matching Node. undefined, null, and objects are unchanged. Closes #29195
0dc26a1 to
3019519
Compare
There was a problem hiding this comment.
LGTM — both nits from the last round are addressed (RAII OwnedString wrap at acquisition + .into_inner() at consumption in fetch.rs, and the stale test-comment parenthetical dropped).
What was reviewed
fetch_impl: init validated after first-argStringOrURL::from_js(WebIDL left-to-right);url_str_optionalnowOption<OwnedString>so the +1 releases viaDropon the new early return — no manualderef(), no leak.Request::construct_into: same dictionary check via the existingbail!cleanup path;is_object()covers DOMWrapper so no divergence from the fetch.rs guard.- Tests: hermetic
data:URL, full primitive matrix × both entry points, plus the toString-ordering case;fetch.test.tsparses at HEAD.
Extended reasoning...
Overview
Tightens WebIDL dictionary conversion for the init argument on both fetch() and new Request(): a non-nullish primitive now rejects/throws TypeError instead of being silently ignored. Three files: src/runtime/webcore/fetch.rs (~35 net lines — capture init_arg, validate after first-arg conversion, reject via the same rejected-promise shape every other validation error in fetch_impl uses; plus the OwnedString RAII wrap of url_str_optional), src/runtime/webcore/Request.rs (14 lines — same check via the established bail! cleanup macro), and test/js/web/fetch/fetch.test.ts (47 lines — it.each matrix over 8 primitives × {fetch, Request} + 3 accepted values + a WebIDL argument-ordering test).
Security risks
None. This adds an argument-type guard that rejects more inputs; no new parsing, no new I/O, no auth/crypto/permissions. The good-path tests use a data: URL so no network is touched.
Level of scrutiny
Medium — fetch_impl is a hot, ownership-sensitive path — but the change is narrowly a validation guard plus a small RAII refactor that this review specifically requested. The one memory-safety-relevant edit (wrapping url_str_optional in Option<bun_core::OwnedString> at acquisition and disarming with .into_inner() at consumption) matches the idiom already used in bun_fetch_preconnect and the proxy-href sites in the same file, and OwnedString::into_inner (src/bun_core/string/mod.rs:1182) hands the +1 back without deref'ing, so the outer OwnedString::new(...) wrap doesn't double-count. The rejected-promise construction on the new early return matches every sibling validation error in the function.
Other factors
This PR has been through ~17 review threads over four months, all resolved. My two remaining nits from the 08:24 round are now addressed exactly as suggested; the comment-cop flags on the long rationale comments are cleared (comments trimmed to 1–2 lines each). The bug-hunting system found nothing this run. I re-verified fetch.test.ts parses at HEAD (a prior iteration had a brace-splice regression). The Request.rs guard uses !is_object(), which covers DOMWrapper (JSType ≥ ObjectType), so it accepts the same set as the fetch.rs is_object() || js_type() == DOMWrapper guard.
1 participant
What / why
fetch(url, 0),fetch(url, ""),fetch(url, false),fetch(url, 0n)andfetch(url, Symbol())silently resolved in Bun instead of rejecting with aTypeError.Per the WHATWG Fetch spec, the
initargument is a Web IDL dictionary, and dictionary conversion throwsTypeErrorfor primitives other thanundefined/null. Node.js honours this — Bun did not.Root cause
src/bun.js/webcore/fetch.zigfetchImplpulled the second argument withargs.nextEat()and kept it only if it was an object / DOMWrapper. For every other value type it fell through tonulland treated the call as ifinithad been omitted.Fix
Explicit three-way check on the second argument:
undefined/null→ treated as missing (spec-compliant).TypeError.Repro
Verification
test/regression/issue/29195.test.tscovers number, bigint, string, boolean, and symbol init (all must reject withTypeError) plusundefined,null, and object init (must not reject withTypeError).Fixes #29195