Skip to content

Fix missing exception check on Bun.plugin target coercion #31956

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 1 commit into from
Closed

Fix missing exception check on Bun.plugin target coercion #31956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/jsc/bindings/BunPlugin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,8 +306,11 @@
auto targetValue = obj->getIfPropertyExists(globalObject, Identifier::fromString(vm, "target"_s));
RETURN_IF_EXCEPTION(throwScope, {});
if (targetValue) {
if (auto* targetJSString = targetValue.toStringOrNull(globalObject)) {
auto* targetJSString = targetValue.toStringOrNull(globalObject);
RETURN_IF_EXCEPTION(throwScope, {});
if (targetJSString) {

Check warning on line 311 in src/jsc/bindings/BunPlugin.cpp

View check run for this annotation

Claude / Claude Code Review

Redundant null check after RETURN_IF_EXCEPTION 

nit: now that `RETURN_IF_EXCEPTION` follows `toStringOrNull()`, `targetJSString` can never be null on line 311 — `toStringOrNull` returns null iff an exception is pending (see the `[[ZIG_EXPORT(null_is_throw)]]` annotation at bindings.cpp:4697). Per CLAUDE.md ("guards a new validator makes redundant — a leftover null-check misleads readers into thinking the value can be absent"), drop the `if (targetJSString)` wrapper and unindent the body.
Comment on lines +309 to +311

@claude claude Bot Jun 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit: now that RETURN_IF_EXCEPTION follows toStringOrNull(), targetJSString can never be null on line 311 — toStringOrNull returns null iff an exception is pending (see the [[ZIG_EXPORT(null_is_throw)]] annotation at bindings.cpp:4697). Per CLAUDE.md ("guards a new validator makes redundant — a leftover null-check misleads readers into thinking the value can be absent"), drop the if (targetJSString) wrapper and unindent the body.

Extended reasoning...

What & why

JSC's JSValue::toStringOrNull(globalObject) is implemented as toString() followed by an internal RETURN_IF_EXCEPTION(scope, nullptr). toString() itself never returns null (it returns an empty JSString on failure), so the only way toStringOrNull yields nullptr is when an exception is pending. Bun's own FFI export documents this contract explicitly:

// src/jsc/bindings/bindings.cpp:4697
[[ZIG_EXPORT(null_is_throw)]] JSC::JSString* JSC__JSValue__toStringOrNull(...)

This PR adds RETURN_IF_EXCEPTION(throwScope, {}) immediately after the toStringOrNull call (line 310). Once that macro returns on a pending exception, control reaching line 311 implies no exception is pending, which in turn implies targetJSString != nullptr. The if (targetJSString) guard is therefore unreachable-false — dead code introduced by this very change.

Step-by-step proof

Consider the two possible outcomes of line 309:

  1. toString/@@toPrimitive throws (the fuzzer case this PR fixes). toStringOrNull returns nullptr and sets a pending exception. Line 310's RETURN_IF_EXCEPTION observes the exception and returns {}. Line 311 is never reached.
  2. Coercion succeeds. toString() returns a non-null JSString*; the internal scope sees no exception, so toStringOrNull forwards that non-null pointer. Line 310 sees no exception and falls through. Line 311 evaluates if (non-null) — always true.

There is no third path where line 311 is reached with targetJSString == nullptr.

Why existing code doesn't justify keeping it

One reviewer might note that NodeModuleModule.cpp:220-224 has the same belt-and-suspenders shape (toStringOrNull → RETURN_IF_EXCEPTION → if (!code)). However, the codebase is not uniform here — JSBuffer.cpp:279-281 and ErrorCode.cpp:376-378 both dereference the result directly after RETURN_IF_EXCEPTION with no null guard:

auto arg_ = arg.toStringOrNull(lexicalGlobalObject);
RETURN_IF_EXCEPTION(scope, {});
const auto& view = arg_->view(lexicalGlobalObject);  // no null check

So "prevailing convention" cuts both ways. The tiebreaker is the project's own contributor guidance — CLAUDE.md:323 is unambiguous and calls out this exact scenario:

Delete dead code in the same PR that makes it dead (these deletions are required scope, not drive-bys) … guards a new validator makes redundant (a leftover null-check misleads readers into thinking the value can be absent)

This PR added the validator (RETURN_IF_EXCEPTION); per the documented rule, removing the now-redundant guard is in-scope for the same PR rather than a drive-by style preference.

Impact

None at runtime — the branch is always taken. The cost is purely readability: a future reader seeing if (targetJSString) after the exception check will reasonably (but wrongly) infer there's a non-exception path where toStringOrNull returns null, and may cargo-cult that defensive shape elsewhere.

Fix

auto* targetJSString = targetValue.toStringOrNull(globalObject);
RETURN_IF_EXCEPTION(throwScope, {});
String targetString = targetJSString->value(globalObject);
RETURN_IF_EXCEPTION(throwScope, {});
if (!(targetString == "node"_s || targetString == "bun"_s || targetString == "browser"_s)) {
    JSC::throwTypeError(globalObject, throwScope, "plugin target must be one of 'node', 'bun' or 'browser'"_s);
    return {};
}

(Equivalently, switch to targetValue.toString(globalObject) — which never returns null — and keep the RETURN_IF_EXCEPTION; either form removes the dead branch.)

String targetString = targetJSString->value(globalObject);
RETURN_IF_EXCEPTION(throwScope, {});
if (!(targetString == "node"_s || targetString == "bun"_s || targetString == "browser"_s)) {
JSC::throwTypeError(globalObject, throwScope, "plugin target must be one of 'node', 'bun' or 'browser'"_s);
return {};
Expand Down
19 changes: 18 additions & 1 deletion test/js/bun/plugin/plugins.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/// <reference types="./plugins" />
import { plugin } from "bun";
import { describe, expect, it } from "bun:test";
import { describe, expect, it, jest } from "bun:test";
import { resolve } from "path";

declare global {
Expand Down Expand Up @@ -366,6 +366,23 @@ describe("errors", () => {
}).toThrow("plugin target must be one of 'node', 'bun' or 'browser'");
});

it("handles a 'target' whose toString throws", () => {
const setup = jest.fn();
const opts = {
setup,
target: {
toString() {
throw new Error("boom from target toString");
},
},
};

expect(() => {
plugin(opts as any);
}).toThrow("boom from target toString");
expect(setup).not.toHaveBeenCalled();
});

it("invalid loaders throw", () => {
const invalidLoaders = ["blah", "blah2", "blah3", "blah4"];
const inputs = ["body { background: red; }", "<h1>hi</h1>", '{"hi": "there"}', "hi"];
Expand Down
Loading