console: guard throwing Event/AggregateError property reads in the native formatters - #35816
console: guard throwing Event/AggregateError property reads in the native formatters#35816robobun wants to merge 10 commits into
Conversation
The native Event formatter in ConsoleObject reads .type through the user-visible accessor before deciding whether to render a MessageEvent / ErrorEvent specially or fall through to the generic object printer. If an Event subclass overrides type with a getter that throws, the exception propagates out of console.log and Bun.inspect. The generic object printer (forEachProperty in bindings.cpp) already swallows getter exceptions for every other Event property, so only the four reads done ahead of that path were unguarded: type, and the message/data/error reads on the MessageEvent/ErrorEvent arm. Catch and clear those four, matching the existing print_to_json pattern.
|
Reproduced on current main: The guard is applied to the Event arm in both CI status: the diff itself is green on every lane that ran end to end (windows-aarch64, freebsd-aarch64, linux-aarch64-android, alpine-x64). Builds #81656 and #81769 both failed on infra: a |
|
Warning Review limit reached
Next review available in: 11 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Walkthrough
ChangesEvent inspection
Possibly related PRs
🚥 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.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/js/bun/util/inspect.test.js`:
- Around line 214-235: Strengthen the Bun.inspect assertions for ThrowType,
ThrowData, and ThrowError: require ThrowType output to contain `type: [Getter]`,
require ThrowData output to contain `data: undefined`, and assert ThrowError
output omits the throwing `message` and `error` fields while retaining `type:
"error"`. Use the existing inspect results in these test cases rather than only
checking that inspection does not throw.
🪄 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: 58fa2418-3016-4f65-9f43-2569b5949856
📒 Files selected for processing (2)
src/jsc/ConsoleObject.rstest/js/bun/util/inspect.test.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The snapshot/diff pretty-format in src/runtime/test_runner/pretty_format.rs has a near-identical Tag::Event arm with the same four unguarded reads. Apply the same clear-on-Err treatment so toMatchSnapshot on an Event subclass with a throwing type/data/error/message getter no longer fails with 'Failed to pretty format value'.
There was a problem hiding this comment.
LGTM — both prior concerns addressed (pretty_format.rs twin site guarded, test assertions strengthened to check the actual fallback rendering).
Extended reasoning...
Overview
Guards four Event property reads (.type, .message, .data, .error) in the native Bun.inspect/console.log formatter (src/jsc/ConsoleObject.rs::print_event) and its copy in JestPrettyFormat (src/runtime/test_runner/pretty_format.rs) so a subclass with a throwing getter no longer propagates out of inspection. Each read is converted from ? to a match that clears the pending exception on Err(_) and substitutes undefined/None. Two tests added to test/js/bun/util/inspect.test.js: one exercising Bun.inspect directly with strengthened rendering assertions, and one spawning a subprocess to exercise toMatchSnapshot on the same three cases.
Security risks
None. This is inspect/console output formatting; the change swallows a getter exception during best-effort stringification, matching the existing forEachProperty behavior in bindings.cpp and the print_to_json precedent at ConsoleObject.rs:4287.
Level of scrutiny
Low-to-medium. The transformation is mechanical — the same 6-line match { Ok(v) => ..., Err(_) => { clear_exception(); fallback } } applied eight times across two files, mirroring an existing pattern in the same file. The fallback values preserve prior Ok(None) semantics (UNDEFINED for .type/.data, None for .message/.error), so the only behavioral delta is on the throwing path. I verified the .type-throws case falls through to the generic print_as(Tag::Object, ...) arm via EventType::unknown, which is the pre-existing Ok(None) path.
Other factors
My previous review flagged (1) the unguarded twin in pretty_format.rs and (2) near-vacuous .toContain("type") assertions. Both were addressed in fb1009b and 1e88697: the four JestPrettyFormat reads are now guarded identically, tests assert type: [Getter], data: undefined, and absence of message:/error: fields, and a spawned-subprocess test covers toMatchSnapshot. The author explicitly scoped out the Tag::Error .message read and JSX .type reads as separate tag arms, which is a reasonable stated exclusion. The subprocess test uses --update-snapshots on the whole file (no -t filter), drains both pipes concurrently, and asserts 3 pass + exit 0. No bugs from the bug-hunting pass.
Extends the Event-getter guards to the remaining unguarded reads: - print_errorlike_object: getDirect on AggregateError.errors can return empty (deleted), a GetterSetter cell (defineProperty accessor), or a non-iterable object; for_each on any of those crashed or threw out of console.log/Bun.inspect. Gate on is_object() and clear the iteration exception. - test_runner/pretty_format.rs: apply the same type/message/data/error guards as ConsoleObject so toMatchSnapshot on a hostile Event subclass renders instead of failing with 'Failed to pretty format value'. Tests cover subclass getters, own-instance accessors, nested objects, and the three AggregateError.errors shapes.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
robobun
changed the title
|
Same fix was also prepared on |
There was a problem hiding this comment.
LGTM — all three review threads from earlier runs are addressed (pretty_format.rs guarded, test assertions strengthened to exact .toBe/[Getter], VirtualMachine.rs comment trimmed to one line).
What was reviewed:
- The four
Err(_) => clear_exception()guards in each of ConsoleObject.rs and pretty_format.rs match the existing pattern at ConsoleObject.rs:4287; fallback values (UNDEFINED/None) preserve the pre-PR rendering for the non-throwing case. is_object()on thegetDirectresult: verifiedJSType::GetterSetter= 5 is belowObjectTypein JSType.rs, so the guard rejects empty, GetterSetter, and primitives as claimed.- Tests cover subclass getters, own-instance
definePropertyaccessors, nested values, all three AggregateError shapes (accessor/non-iterable/deleted), and thetoMatchSnapshotpath via a spawned subprocess; evidence in the PR body shows they fail on main and pass with the fix.
Extended reasoning...
Overview
Guards eight Event property reads (.type/.message/.data/.error) across the two near-identical formatter arms in src/jsc/ConsoleObject.rs::print_event and src/runtime/test_runner/pretty_format.rs (Tag::Event), converting ? propagation into match { Err(_) => { clear_exception(); fallback } }. Also gates the AggregateError.errors iteration in VirtualMachine.rs::print_errorlike_object on is_object() and clears any exception for_each throws. Three new tests in test/js/bun/util/inspect.test.js.
Prior feedback resolution
I raised three items in earlier passes; all are addressed and the threads resolved:
- 🔴
pretty_format.rstwin site unguarded → fixed in 1e88697, with a spawned-subprocesstoMatchSnapshottest. - 🟡 vacuous
.toContain("type")→ replaced withtype: [Getter], exact.toBe(...)on the full MessageEvent/ErrorEvent output, and.not.toContain(<thrown message>). - 🟡 comment-cop / narration in the VirtualMachine.rs comment → trimmed to a single line stating only the non-obvious
getDirectreturn shapes.
Security risks
None. This is defensive exception handling inside inspection/formatting code; no input parsing, auth, or resource acquisition is touched. The change strictly narrows the set of inputs on which the formatter throws or crashes.
Level of scrutiny
Medium — native Rust touching JSC exception state. I verified the two non-obvious claims: (1) clear_exception() after a user-getter throw is the established idiom in this file (pre-existing at ConsoleObject.rs:4287, the toJSON call site), so REVIEW.md's "never clearException()" — which targets hiding real bugs behind a blanket clear — doesn't apply to a formatter deliberately swallowing hostile-getter throws; and (2) JSType::GetterSetter is 5 in src/jsc/JSType.rs while is_object() tests >= ObjectType, so the single is_object() guard correctly excludes empty, GetterSetter, and primitive returns from getDirect.
Other factors
The transformation is mechanical and localized: each x? becomes an explicit match whose Ok arms preserve the exact pre-PR value flow, so non-throwing inputs render identically. The PR body's evidence block shows the new tests failing on both debug-ASAN and release main and passing with the fix, and BUN_JSC_validateExceptionChecks=1 is reported clean. Test coverage is thorough — subclass vs. own-accessor, nested containers, all three errors shapes, and the snapshot-formatter path. Bug-hunting system found nothing.
There was a problem hiding this comment.
LGTM — both earlier rounds of feedback (the twin pretty_format.rs Event arm and the test-assertion / comment-length nits) have been addressed, and this run found nothing further.
What was reviewed:
- The four Event property reads now match on
Err(_)→clear_exception()+ fallback in bothConsoleObject.rsandpretty_format.rs; pattern matches the existingprint_to_jsonguard at ConsoleObject.rs:4287. VirtualMachine.rserrorsiteration is gated onis_object()(rejects empty/GetterSetter) and clears the exception iffor_eachthrows — verifiedget_errors_propertyis nothrowgetDirect.- Tests now assert exact rendered output (
toBeon the MessageEvent/ErrorEvent cases,type: [Getter]on the fallback), cover accessor/non-iterable/deletederrors, and exercisetoMatchSnapshotin a subprocess without a name filter.
Extended reasoning...
Overview
Guards five hostile-input paths in the native inspect/format code so a throwing user getter (or a tampered AggregateError.errors slot) no longer escapes console.log / Bun.inspect / toMatchSnapshot. Four value.get(...)? / fast_get(...)? reads in the Tag::Event arm of both src/jsc/ConsoleObject.rs and src/runtime/test_runner/pretty_format.rs become explicit match blocks that call global_this.clear_exception() on Err and fall back to undefined/None. The AggregateError.errors iteration in src/jsc/VirtualMachine.rs::print_errorlike_object is gated on is_object() and clears the exception if for_each throws. Three new tests in test/js/bun/util/inspect.test.js cover Bun.inspect, nested-object inspect, the AggregateError variants (accessor / non-iterable / deleted), and a spawned toMatchSnapshot run.
Security risks
None. This is display-layer formatting; the change swallows exceptions from user getters during best-effort inspection, which is the established contract for console.log (Node prints these values without throwing, and the existing generic-object printer already renders throwing getters as [Getter]). No auth, crypto, permission, or trust-boundary code is touched. The clear_exception() calls are scoped to the specific failed read and mirror the pre-existing pattern at ConsoleObject.rs:4287.
Level of scrutiny
Medium — native Rust/JSC code with exception-state handling, but the change is a narrow, mechanical defensive pattern applied identically at six sites. The PR was already reviewed twice by me: the first pass caught the missing sibling in pretty_format.rs (fixed in 1e88697) and the vacuous .toContain("type") assertion (fixed in fb1009b); the second pass flagged the over-long VirtualMachine.rs comment (trimmed to one line in e015b8a). All inline threads on the timeline are resolved. The PR evidence block shows the new tests fail on system Bun / release-without-fix and pass on the debug+ASAN build, and BUN_JSC_validateExceptionChecks=1 is clean.
Other factors
- The
Err(_) => clear_exception()shape is copied from an existing use in the same file (print_to_json), so it is not novel. - The subprocess snapshot test uses
--update-snapshotswithout a-tfilter, satisfying REVIEW.md's snapshot-update rule. - The author explicitly scoped out the
Tag::Error.messageread and JSX.typereads as separate tag arms (different bug class), which is a reasonable exclusion stated on the PR. - No CODEOWNERS on these paths; comment-cop is quiet after the trim.
|
Heads up: the VirtualMachine.rs hunk in this PR (the is_object() / clear_exception() guard around the AggregateError for_each) touches a block that #36602 removes entirely, so this will conflict once that lands. The Event/MessageEvent/ErrorEvent parts in ConsoleObject.rs and pretty_format.rs are independent; after #36602 this PR can drop the VirtualMachine.rs hunk and its AggregateError test cases, which are covered in test/js/bun/util/inspect-error.test.js there. |
2 participants
Repro
Cause
print_eventinsrc/jsc/ConsoleObject.rsreads.type/.message/.data/.errorviavalue.get(...)?/value.fast_get(...)?before deciding how to render. The?propagates a thrown getter straight out ofconsole.log/Bun.inspect. The near-identicalTag::Eventarm insrc/runtime/test_runner/pretty_format.rs(used bytoMatchSnapshot/DiffFormatter) has the same four unguarded reads.print_errorlike_objectinsrc/jsc/VirtualMachine.rsreadsAggregateError.errorsviagetDirectand unconditionally hands the result tofor_each.getDirecton a deleted prop returns empty (crash inforEachInIterable), on adefinePropertyaccessor returns theGetterSettercell (debug assert insynthesizePrototype, TypeError in release), and on a non-iterable objectfor_eachthrows; thelet _ =discarded theJsResultbut left the exception pending on the VM.Fix
Err(_)arm on the four Event reads in both formatters, callglobal_this.clear_exception(), and fall back toundefined/None. This mirrors the existingprint_to_jsonpattern inConsoleObject.rs. When.typethrows, the formatter falls through to the generic object printer which renderstype: [Getter].AggregateError.errorsiteration onis_object()(excludes empty /GetterSetter/ primitives) and clear the exception iffor_eachstill throws.Verification
USE_SYSTEM_BUN=1 bun test test/js/bun/util/inspect.test.js -t "throwing getter|hostile"fails (2 tests)bun bd test test/js/bun/util/inspect.test.jspasses (76 tests)BUN_JSC_validateExceptionChecks=1clean[review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file