Repository navigation
test runner: don't panic formatting a FormData whose toJSON is not callable - #36915
Conversation
WalkthroughFormData formatting now checks that ChangesFormData formatting
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: reproduced on 1.4.0-canary ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted crash fix replacing an .unwrap() on user-reachable input with the same Tag::Object / JSType::Event fallback already used elsewhere in this file (lines ~1688, ~1873). One nit inline about draining stdout in the test.
Extended reasoning...
Overview
This PR fixes a panic in the jest pretty-formatter's FormData branch (src/runtime/test_runner/pretty_format.rs). The old code did value.get(global, "toJSON")?.unwrap(), which panicked when toJSON was shadowed with undefined or the prototype was nulled out — taking down the entire bun test run on a single failing assertion. The fix wraps the lookup in if let Some(...).filter(|f| f.is_callable()), calling toJSON only when it exists and is callable, and otherwise falling through to the generic object printer via print_as::<Tag::Object>(..., value, JSType::Event). A new spawned-fixture test covers both the shadowed-toJSON and null-prototype variants and asserts the child reports 2 fail / exit 1 rather than aborting.
Correctness
- The fallback shape (
print_as::<W, { Tag::Object }, ...>(writer.ctx, value, JSType::Event)) is byte-identical to the existing genericTag::Privatefallback at ~1688-1690 and theTag::Eventfallback at ~1873-1875 in the same file, so there's no novel code path here. - Inside the
Tag::Objectprinter (~2302-2380), thejs_typeargument isn't consulted — it iterates own properties and, when none are found, prints<ClassName> {}viaget_class_name. For the shadowed-toJSONcase that yieldsFormData {}, satisfying the test's"Received: FormData"assertion. is_callable()is a non-throwing FFI check (JSC__JSValue__isCallable), so the.filterclosure can't introduce an unchecked exception.- The
.filter(is_callable)also coverstoJSON: 42(previously threw from.call()inside the formatter); that now prints the object instead. This mirrors the intent of theConsoleObject.rsFormData branch, which already guards withif let Some(...).
Security risks
None. This is diff-formatting code for test-runner output; no untrusted-input parsing, auth, or I/O boundaries are touched.
Level of scrutiny
Low-to-medium. It's a ~15-line change in a display/formatting path, replacing a panicking .unwrap() with an established fallback pattern from the same function. REVIEW.md explicitly calls out "user-reachable failures are recoverable errors, never panics" — this is a textbook instance.
Other factors
- The one inline finding is a nit: the test spawn leaves
stdoutat its default"pipe"without draining it.bun testwrites results to stderr and the fixture emits nothing on stdout, so there's no realistic deadlock — it's a harness-convention consistency point, not a blocker. - The test is hermetic (tempDir + spawned child, no network), asserts stderr content before exit code, and would fail on an unfixed build (child aborts with exit 134 instead of 1, no
"2 fail"line).
…llable The FormData branch of the jest pretty-formatter unwrapped the result of looking up `toJSON`, so a FormData with `toJSON` shadowed by undefined (or with a null prototype) panicked with 'called Option::unwrap() on a None value' as soon as a failing matcher tried to print it, aborting the whole `bun test` run. Only take the toJSON path when the property exists and is callable, otherwise fall through to the generic object printer like the console formatter already does.
d9b66ac to
fd458fc
Compare
|
Updated 2:05 PM PT - Aug 4th, 2026
✅ @robobun, your commit fa3a236a39684192b23d6a1abc09249bc4f3a813 passed in 🧪 To try this PR locally: bunx bun-pr 36915That installs a local version of the PR into your bun-36915 --bun |
There was a problem hiding this comment.
LGTM — the .unwrap() → guarded if let with the Tag::Object/JSType::Event fallback matches the existing generic-printer pattern at pretty_format.rs:1688, and the earlier stdout-drain nit was addressed in fd458fc. The remaining inline note is a test-coverage suggestion, not a correctness issue.
Extended reasoning...
Overview
This PR fixes a panic in the jest pretty-formatter's FormData branch (src/runtime/test_runner/pretty_format.rs). The old code did value.get(global, "toJSON")?.unwrap(), which panics when get() returns None (property missing or undefined) — reachable from user code by shadowing toJSON or nulling the prototype, and triggered whenever any failing matcher tries to print such a value. The fix guards with if let Some(f) = ... .filter(|f| f.is_callable()) and, when absent/non-callable, falls through to print_as::<_, { Tag::Object }, _>(.., value, JSType::Event). A new spawned-subprocess test asserts the child bun test reports 2 fail / exit 1 instead of aborting.
Security risks
None. This is diagnostic-output formatting inside the test runner; no auth, crypto, filesystem, or network surface is touched. The change strictly removes a user-reachable panic.
Level of scrutiny
Low-to-medium. The fallback is byte-for-byte the same call already used at pretty_format.rs:1688-1690 and :1873-1875 for other Tag::Private values with no special handler, so there is no new behavior to reason about — only which branch is taken. The sibling ConsoleObject.rs FormData path already handles get() == None without unwrapping (it swallows call errors via unwrap_or_else(take_exception)), so this brings the test-runner formatter to parity rather than diverging. The ? on get() still propagates a throwing getter correctly.
Other factors
- My previous nit (stdout piped but not drained) was addressed in fd458fc; the test now drains stdout and stderr concurrently with
proc.exited, usestempDir/bunEnv/bunExe, and asserts stderr content before exit code per the harness template. - robobun confirmed fails-before/passes-after with
bun bd test. - The one open inline finding is a nit: the
is_callable()filter isn't independently pinned by a fixture case (both existing cases short-circuit atget() → None). That's a coverage refinement worth adding but doesn't affect correctness of the fix, so it doesn't block approval.
There was a problem hiding this comment.
LGTM — both earlier nits (drain stdout; pin the is_callable guard with a toJSON: 42 case) are addressed in fd458fc and fa3a236.
What was reviewed:
- The
.unwrap()→if let Some(...).filter(is_callable)change and its fallback toprint_as::<Tag::Object>(…, JSType::Event), which matches the existing DOMWrapper fallback atpretty_format.rs:1688. - Confirmed
forEachPropertyOrderedusesDontEnumPropertiesMode::Include, so the non-enumerabletoJSON: 42own prop is iterated and the"toJSON": 42assertion actually pins the callable filter. - Null-prototype case has zero own JS properties, so it hits the
iter_i == 0→get_class_namepath (noget_prototype()call on a null proto).
Extended reasoning...
Overview
Two files: a ~15-line change in src/runtime/test_runner/pretty_format.rs replacing an .unwrap() on value.get(global, "toJSON") with a guarded if let Some(...).filter(|f| f.is_callable()), plus a graceful fallback to the generic object printer; and a new spawned-fixture regression test at test/js/bun/test/expect-formdata-tojson-crash.test.ts covering three variants (undefined-shadowed, null prototype, non-callable value).
Security risks
None. This is display-only formatter code inside bun test's failing-matcher printer. No auth/crypto/parsing of untrusted external input; the worst pre-fix outcome was already a panic, and the fix strictly narrows behavior to "print the object instead of crashing".
Level of scrutiny
Low-medium. It's a crash fix (.unwrap() on user-manipulable state → a REVIEW.md "user-reachable failures are recoverable errors, never panics" violation), so the mechanism is unambiguous. I checked that the fallback shape matches the existing Tag::Private DOMWrapper fallback three lines below (print_as::<Tag::Object>(…, JSType::Event) at line 1688), that the Tag::Object arm doesn't consume the js_type argument so the JSType::Event choice is purely conventional, and that ConsoleObject.rs already handles the get() == None case (which is why console.log never crashed). The ? on .get() still propagates a throwing getter as before.
Other factors
Both of my earlier review comments have been addressed in follow-up commits: stdout is now drained in the Promise.all, and the third fixture case exercises .filter(|f| f.is_callable()) (I verified via bindings.cpp:5437 that forEachPropertyOrdered includes non-enumerable own props, so the "toJSON": 42 assertion is reachable and load-bearing). The test uses tempDir/bunEnv/bunExe, drains both pipes, asserts stderr content before exit code, and asserts exitCode === 1 (so a panic/exit-134 fails it). No outstanding reviewer comments.
…llable (oven-sh#36915) ### Repro ```js // bun test ./x.test.mjs import { test, expect } from "bun:test"; test("x", () => { const fd = new FormData(); Object.defineProperty(fd, "toJSON", { value: undefined }); // or Object.setPrototypeOf(fd, null) expect(fd).toEqual(1); }); ``` ``` panic: called `Option::unwrap()` on a `None` value src/runtime/test_runner/pretty_format.rs:1585:87 oh no: Bun has crashed ... (exit 134) ``` One failing assertion takes down the entire `bun test` run. ### Cause The FormData branch of the jest pretty-formatter (`Tag::Private` in `pretty_format.rs`) did `value.get(global, "toJSON")?.unwrap()`. `JSValue::get` returns `None` when the property is missing or `undefined`, so a FormData with `toJSON` shadowed by `undefined`, or with its prototype swapped out, panicked as soon as any failing matcher tried to print the received value. ### Fix Only take the `toJSON` path when the property exists and is callable; otherwise fall through to the generic object printer (same shape `ConsoleObject.rs` already uses for `console.log` / `Bun.inspect`, which is why those never crashed). The callable check also covers `toJSON: 42`, which previously threw from inside the formatter instead of printing the value. ### Verification `test/js/bun/test/expect-formdata-tojson-crash.test.ts` spawns `bun test` on a fixture with the three variants (`toJSON: undefined`, null prototype, `toJSON: 42`) and asserts the run reports `3 fail` / exit 1 with the FormData printed in the diff. Before the fix the child aborts with the panic above. <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 1 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 1 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/test/expect-formdata-tojson-crash.test.ts bun test v1.4.0 (fa3a236) test/js/bun/test/expect-formdata-tojson-crash.test.ts: killed 1 dangling process (fail) failing matcher on FormData without a callable toJSON does not abort the test runner [5005.66ms] ^ this test timed out after 5000ms. 0 pass 1 fail Ran 1 test across 1 file. [6.98s] error: script "bd" exited with code 1 __F:1:S:0 release without fix: all passed bun test v1.4.0-canary.1 (fd458fc) test/js/bun/test/expect-formdata-tojson-crash.test.ts: (pass) failing matcher on FormData without a callable toJSON does not abort the test runner [8.69ms] 1 pass 0 fail 5 expect() calls Ran 1 test across 1 file. [157.00ms] __F:0:S:0 ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/test/expect-formdata-tojson-crash.test.ts bun test v1.4.0 (fa3a236) test/js/bun/test/expect-formdata-tojson-crash.test.ts: (pass) failing matcher on FormData without a callable toJSON does not abort the test runner [355.62ms] 1 pass 0 fail 5 expect() calls Ran 1 test across 1 file. [2.34s] __F:0:S:0 release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 708ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/6] gen generated_host_exports.rs generated_host_exports.rs: 94 exports (host=3, lazy=10, generic=81, rust=0); 240 extern-C blocks audited [1/6] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19) �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[92m Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys) �[1m�[92m Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety) �[1m�[92m Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys) �[1m�[92m Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys) �[1m�[92m Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd) �[1m�[92m Compiling�[0m bun_picohttp v0.0.0 (/workspace/bun/src/picohttp) �[1m�[92m Compiling�[0m bun_brotli v ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/runtime/test_runner/pretty_format.rs | 29 ++++++++----- .../bun/test/expect-formdata-tojson-crash.test.ts | 47 ++++++++++++++++++++++ 2 files changed, 65 insertions(+), 11 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 1 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/runtime/test_runner/pretty_format.rs 4 4 0 test/js/bun/test/expect-formdata-tojson-crash.test.ts 3 6 0 ``` </details> <!-- robobun:evidence:end -->
3 participants
Repro
One failing assertion takes down the entire
bun testrun.Cause
The FormData branch of the jest pretty-formatter (
Tag::Privateinpretty_format.rs) didvalue.get(global, "toJSON")?.unwrap().JSValue::getreturnsNonewhen the property is missing orundefined, so a FormData withtoJSONshadowed byundefined, or with its prototype swapped out, panicked as soon as any failing matcher tried to print the received value.Fix
Only take the
toJSONpath when the property exists and is callable; otherwise fall through to the generic object printer (same shapeConsoleObject.rsalready uses forconsole.log/Bun.inspect, which is why those never crashed). The callable check also coverstoJSON: 42, which previously threw from inside the formatter instead of printing the value.Verification
test/js/bun/test/expect-formdata-tojson-crash.test.tsspawnsbun teston a fixture with the three variants (toJSON: undefined, null prototype,toJSON: 42) and asserts the run reports3 fail/ exit 1 with the FormData printed in the diff. Before the fix the child aborts with the panic above.[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file