Fix unchecked exception in Bun.plugin target conversion#31086
Closed
robobun wants to merge 1 commit into
Closed
Conversation
When the target option passed to Bun.plugin() was an object whose toString/valueOf returned a non-primitive, toStringOrNull() would throw and return null, but the pending exception was not checked before continuing to construct the builder object and invoke setup(), tripping an ExceptionScope assertion.
Collaborator
Author
|
Updated 11:32 AM PT - May 19th, 2026
❌ @robobun, your commit 989e6aa has 9 failures in
🧪 To try this PR locally: bunx bun-pr 31086That installs a local version of the PR into your bun-31086 --bun |
Contributor
|
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 (2)
WalkthroughThis PR hardens plugin target validation by refactoring the C++ string extraction code to check for exceptions immediately after converting the target value to a string, and adds a test case verifying the plugin handles malicious toString/valueOf implementations without crashing. ChangesPlugin target robustness
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
Collaborator
Author
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
LGTM — straightforward RETURN_IF_EXCEPTION additions following the established JSC pattern, with a regression test.
Extended reasoning...
Overview
This PR adds two RETURN_IF_EXCEPTION(throwScope, {}) checks in setupBunPlugin() (src/jsc/bindings/BunPlugin.cpp) after targetValue.toStringOrNull(globalObject) and after targetJSString->value(globalObject). Previously, if toStringOrNull threw (e.g. an object whose toString/valueOf both return non-primitives), it returned nullptr, the if body was skipped, and execution continued with a pending exception — tripping ExceptionScope::assertNoException() in debug builds. The fix restructures the if to check the exception immediately and bail out. A regression test in test/js/bun/plugin/plugins.test.ts verifies the call now throws cleanly instead of asserting.
Security risks
None. This is defensive exception-propagation hardening on a user-supplied options object. The change strictly narrows behavior: where the old code would proceed with a pending exception (assertion in debug, undefined-ish behavior in release), the new code returns early and surfaces the JS TypeError to the caller. No new code paths, no new inputs accepted, no auth/crypto/permissions involved.
Level of scrutiny
Low. The diff is 4 effective lines of C++ that apply the same RETURN_IF_EXCEPTION macro already used a dozen+ times in this very function and file (e.g. after getIfPropertyExists, toWTFString, etc.). It is a mechanical, idiomatic JSC fix for a fuzzer-found edge case, not a logic or design change.
Other factors
- The added test directly exercises the repro from the PR description and asserts
.toThrow(), so the fix is covered. - No CODEOWNERS entry matches
src/jsc/bindings/. - No prior reviewer comments to address; the bug-hunting system found no issues.
- The second
RETURN_IF_EXCEPTIONafter->value(globalObject)is belt-and-suspenders (rope resolution can in principle throw OOM) and is harmless/correct.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes an
ExceptionScope::assertNoException()assertion failure in debug builds whenBun.plugin()is called with atargetoption that throws during string conversion (e.g. an object whosetoString/valueOfreturn non-primitives).toStringOrNull()returnsnullptrand leaves the exception pending. The previous code only checked for a non-null return and fell through to construct the builder object and callsetup()with the exception still pending.Repro
Before:
After: the
TypeError: No default valueexception is propagated to JS and can be caught.Found by Fuzzilli. Fingerprint:
2fddca8bb30d77a3