Repository navigation
Conversation
|
Warning Review limit reached
This review includes 7 billable files and costs up to $1.75.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 15 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (7)
Comment |
|
Updated 3:45 AM PT - Oct 3rd, 2026
❌ @robobun, your commit 67bbd7e has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33798That installs a local version of the PR into your bun-33798 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-tested widening of the onLoad contents type gate to match the published .d.ts.
Extended reasoning...
Overview
This PR fixes a runtime/type-declaration mismatch: OnLoadResultSourceCode.contents is typed as string | ArrayBufferView | ArrayBuffer | SharedArrayBuffer, but both the runtime plugin path (ModuleLoader.cpp) and the bundler plugin path (BundlerPlugin.ts) only accepted string/ArrayBufferView. The fix adds a JSC::JSArrayBuffer downcast branch in C++ (mirroring the existing pattern at bindings.cpp:3159) and relaxes the JS-side gate via isAnyArrayBuffer() on the fallthrough path. Both error messages are updated to list the accepted types. Tests cover ArrayBuffer, SharedArrayBuffer, Uint8Array-over-SAB (control), and a negative case for both Bun.plugin and Bun.build.
Security risks
None. This widens an input-type acceptance gate to include buffer types that the downstream consumer (JSC__JSValue__asArrayBuffer / ZigString construction) already handles. No new parsing of untrusted data, no auth/crypto/permissions surface.
Level of scrutiny
Low-to-moderate. The C++ change is a 5-line branch that exactly mirrors an established pattern elsewhere in the bindings (null-check impl(), then read data()/byteLength()); a null impl() correctly falls through to the existing error path. The TS builtin change only adds a lazy require on the uncommon non-string/non-view path, consistent with other lazy requires in the same file. No CODEOWNERS apply to these files.
Other factors
Test coverage is thorough: positive cases for both new types plus a control and a negative assertion on the exact error text, exercised through both entry points (runtime import and Bun.build). The PR description confirms the new tests fail under USE_SYSTEM_BUN=1. The bug hunting system found no issues.
|
CI status on build 70707: 283 jobs passed. The three red lanes are unrelated infrastructure:
The new tests in |
|
A note for the rebase. #42405 changes the The |
…le a copy of buffer contents Bun.plugin onLoad and build.module accept a bare ArrayBuffer or SharedArrayBuffer as contents, as the published type says. Bun.build onLoad accepts them too. handleVirtualModuleResult, the one caller of Bun__transpileVirtualModule, copies the bytes of a typed array, DataView, ArrayBuffer or SharedArrayBuffer into a stack-local Vector before the transpile. Macros and onResolve plugins run JS during the transpile and another thread can write a SharedArrayBuffer, so the parser cannot read those bytes in place. A string still lends its characters. handleOnLoadResultNotPromise only checks the type of contents, so OnLoadResult holds no pointer. The Bun.build check of contents uses the $inheritsArrayBuffer intrinsic.
0ae37a9 to
67bbd7e
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Points `source` at the text of an onLoad `contents` for one transpile. False: an exception is pending. | ||
| // A string lends its characters. Buffer bytes go into `copy`: macros and onResolve plugins run JS during | ||
| // the transpile and another thread can write a SharedArrayBuffer, so the parser cannot read them in place. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
|
||
| auto bytes = bufferContentsBytes(contents); | ||
| ASSERT(bytes); | ||
| if (bytes->size() > Bun::maxVectorSize<uint8_t>() || !copy.tryAppend(*bytes)) [[unlikely]] { | ||
| throwOutOfMemoryError(globalObject, scope); | ||
| return false; | ||
| } | ||
| source = EncodedSlice { copy.span().data(), copy.size() }; |
There was a problem hiding this comment.
🔴 pre-existing, widened: plugin authors who hand Bun.plugin onLoad a UTF-8 encoded buffer with non-ASCII text (the await file.arrayBuffer() case this PR enables) get mojibake in string literals, while the same buffer in Bun.build is correct. sourceTextForTranspile builds EncodedSlice { copy.span().data(), copy.size() } with no encoding tag (ModuleLoader.cpp:238), and the Rust consumer treats an untagged 8-bit slice as Latin-1 and transcodes any byte >= 0x80 (src/bun_core/string/mod.rs:1488-1492). Fix: mark buffer-sourced slices as UTF-8 (the TAG_UTF8_BIT at src/bun_core/string/mod.rs:1173) or pass raw bytes so the runtime path matches Bun.build, which copies the bytes unchanged via from_js_to_owned_slice (src/runtime/node/types.rs:368-373).
Why this was flagged
A runtime plugin returns { contents: new TextEncoder().encode("export default 'héllo'"), loader: "js" } and the module is imported through Bun.plugin. handleVirtualModuleResult calls sourceTextForTranspile (src/jsc/bindings/ModuleLoader.cpp:426), which copies the bytes and wraps them as EncodedSlice { copy.span().data(), copy.size() } at ModuleLoader.cpp:238 with no UTF-8 tag bit set. Bun__transpileVirtualModule calls source_code.to_utf8() (src/runtime/jsc_hooks.rs:4445); EncodedSlice::to_utf8 at src/bun_core/string/mod.rs:1488 sees !self.is_utf8(), treats the bytes as Latin-1 and transcodes every byte >= 0x80 via to_utf8_from_latin1, so the two UTF-8 bytes of é become two characters (é). The module loads without error with wrong string values. The Bun.build path copies raw bytes (src/runtime/node/types.rs:368-373) and the bundler reads them as UTF-8, so the same plugin gives correct output there. The base already did this for TypedArray/DataView contents; this PR adds ArrayBuffer and SharedArrayBuffer, so the population hitting it grows.
Verification: ModuleLoader.cpp:238 wraps the copied bytes in an EncodedSlice with no tag bits. src/runtime/jsc_hooks.rs:4445 calls source_code.to_utf8(), and src/bun_core/string/mod.rs:1478-1492 transcodes untagged bytes >= 0x80 as Latin-1, so é becomes é. Bun.build copies the buffer verbatim (src/runtime/node/types.rs:368-373). The base already had this for views; this PR adds ArrayBuffer.
|
|
||
| if (!(typeof contents === "string") && !$isTypedArrayView(contents)) { | ||
| throw new TypeError('onLoad plugins must return an object with "contents" as a string or Uint8Array'); | ||
| if (!(typeof contents === "string") && !$isTypedArrayView(contents) && !$inheritsArrayBuffer(contents)) { |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Pre-existing, left by this partial fix: Bun.build plugin authors who return a DataView as onLoad contents still get a TypeError, while the runtime Bun.plugin path accepts it. At src/js/builtins/BundlerPlugin.ts:576 the gate is $isTypedArrayView || $inheritsArrayBuffer, and $isTypedArrayView excludes DataView (see the note at src/js/node/fs.ts:300). Fix: make the Bun.build gate accept every value the published contents type and the runtime path accept, including DataView, with a tamper-resistant check (an intrinsic or the native util.types predicate captured at module load, not looked up per call), and word the TypeError to match. The native side already handles DataViewType (bindings.cpp:3539), so only the JS gate is in the way.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A Bun.build plugin's onLoad returns { contents: new DataView(buffer), loader: "js" }, which type-checks against bun.d.ts:6297 (contents: string | ArrayBufferView | ArrayBuffer | SharedArrayBuffer). runOnLoadPlugins reaches src/js/builtins/BundlerPlugin.ts:576: typeof is not "string", $isTypedArrayView(DataView) is false because JSC's typed-view check excludes DataViewType (src/js/node/fs.ts:300-301 documents this), and $inheritsArrayBuffer is false for a view. The build fails with 'onLoad plugins must return an object with "contents" as a string, TypedArray, ArrayBuffer, or SharedArrayBuffer'. The runtime path accepts the same value: src/jsc/bindings/ModuleLoader.cpp:212 dynamicDowncastJSC::JSArrayBufferView matches JSDataView. The base branch rejected DataView the same way; the PR widened the gate only for ArrayBuffer/SharedArrayBuffer, leaving the two entry points disagreeing on DataView.
Verification: Pre-existing: the base gate already rejected DataView by the same route. Trigger: a Bun.build plugin's onLoad returns { contents: new DataView(buf), loader }. src/js/builtins/BundlerPlugin.ts:576 throws because $isTypedArrayView excludes DataViewType (src/js/node/fs.ts:300) and $inheritsArrayBuffer is false for a view, while the runtime path in src/jsc/bindings/ModuleLoader.cpp accepts DataView.
This branch has not been deployed
1 participant
Repro
Bun.buildwith the same plugin fails the same way (onLoad plugins must return an object with "contents" as a string or Uint8Array). AUint8Arrayview over the exact sameArrayBufferis accepted, so the data is fine; only the type check disagrees with the publishedOnLoadResultSourceCode.contentstype, which isstring | ArrayBufferView | ArrayBuffer | SharedArrayBuffer.This bites anyone passing the result of
await file.arrayBuffer()orfetch(...).arrayBuffer()straight intocontents: it type-checks againstbun.d.tsand then fails at runtime.Cause
src/jsc/bindings/ModuleLoader.cpp):handleOnLoadResultNotPromiseonly downcastscontentstoJSC::JSArrayBufferView.src/js/builtins/BundlerPlugin.ts): the validator only checkstypeof contents === "string" || $isTypedArrayView(contents)before callingonLoadAsync. The downstreamStringOrBuffer::from_js_to_owned_slice(viaJSC__JSValue__asArrayBuffer) already handlesArrayBufferType, so only the JS-side gate was in the way.Fix
ModuleLoader.cpp: add aJSC::JSArrayBufferbranch that readsimpl()->data()/byteLength(), matching the pattern inJSBuffer.cppandbindings.cpp.JSArrayBuffercovers bothArrayBufferandSharedArrayBuffer.BundlerPlugin.ts: whencontentsis neither a string nor a typed-array view, fall back torequire("node:util/types").isAnyArrayBuffer(contents)before rejecting. Therequire()is only reached on the uncommon non-string/non-view path.Both error messages now list the accepted types.
Verification
Each suite has an
ArrayBuffercase, aSharedArrayBuffercase, aUint8Array-over-SharedArrayBuffercontrol, and a negative case that asserts plain objects are still rejected. TheArrayBuffer/SharedArrayBuffercases fail underUSE_SYSTEM_BUN=1and pass with this change.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bun-build-api.test.ts