Conversation
…t name/origin getNodeVMContextOptions() always read the name and origin keys, which are createContext()'s spelling. Script#runInNewContext() takes the same options as contextName/contextOrigin (like contextCodeGeneration, which was already switched per caller), so it rejected name/origin and ignored contextName/contextOrigin. Replace the per-caller codeGeneration key with a per-caller key set covering all three. The contextName/contextOrigin check that compensated for this lived in the Script constructor's option parser, where Node has none; it made new Script(), vm.runInThisContext() and vm.runInContext() reject those keys. Remove it now that runInNewContext() validates them itself.
|
Warning Review limit reached
Next review available in: 43 minutes 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)
Comment |
|
Status: reproduced on main (debug build) and bun 1.4.0 with Fix is in this PR. Tests: |
There was a problem hiding this comment.
LGTM — mechanical extension of the existing codeGenerationKey parameterization to cover name/origin, plus removal of misplaced validation from the Script constructor.
What was reviewed:
- All three
getNodeVMContextOptions()call sites updated with the correct key set;ERR::INVALID_ARG_TYPEhas aWTF::Stringoverload somakeString("options."_s, keys.name)is fine. - Removed
contextName/contextOriginblock inScriptOptions::fromJS()only validated (never stored), so no downstream consumer loses a value. - Traced the JS wrapper
vm.runInNewContext→createContext+Script#runInNewContextto confirm the updated throwing-getter matrix still asserts the right key per entry point. inline constexprASCIILiteralaggregate matches existing usage in the file.
Extended reasoning...
Overview
This PR fixes which option keys getNodeVMContextOptions() reads depending on its caller. Node's vm.createContext() takes { name, origin, codeGeneration } while Script#runInNewContext() and vm.runInNewContext() take { contextName, contextOrigin, contextCodeGeneration }. Bun's shared native parser previously hard-coded name/origin for both callers (only codeGeneration was already parameterized). The fix generalizes the existing single-key parameter to a three-key struct NodeVMContextOptionKeys, and removes a compensating-but-misplaced contextName/contextOrigin check from the Script constructor's option parser (ScriptOptions::fromJS), where Node has no such check. Files touched: NodeVM.h (struct + two inline constexpr instances + forward decl + signature), NodeVM.cpp (function body + two call sites), NodeVMScript.cpp (removed block + one call site), and vm.test.ts (new context name/origin options describe block + updated throwing-getter matrix).
Security risks
None. The name/origin values are only validated, never stored or used — JSC has no context-name concept, so this is purely about which key is read and which error message is thrown. No auth, crypto, permissions, or memory-safety-relevant code paths are touched. The change is pure option-name plumbing over compile-time ASCIILiteral constants.
Level of scrutiny
Low-to-moderate. This is a Node.js compat fix that follows the exact pattern already in place for codeGenerationKey — the diff is mostly codeGenerationKey → keys.codeGeneration, "name"_s → keys.name, "origin"_s → keys.origin, plus building error-message strings with makeString (I confirmed ERR::INVALID_ARG_TYPE has a const WTF::String& overload for arg_name at ErrorCode.h:89). The removed ScriptOptions::fromJS block only did validation and set any = true; it never wrote to a field, so removing it can't strand a downstream consumer. I grepped for all getNodeVMContextOptions call sites and confirmed all three are updated with the right key set.
Other factors
Test coverage is thorough: the new describe block covers every entry point (Script#runInNewContext, vm.runInNewContext, createContext, new Script, runInThisContext, runInContext, Script#runInContext/#runInThisContext), both key spellings, three non-string value types, validation ordering vs Node, sandbox variants (undefined/existing context/DONT_CONTEXTIFY), and the negative contract via throwing getters (unreadable()). The existing throwing-getter subprocess matrix was updated to reflect the new key set each entry point reads; I traced vm.runInNewContext through src/js/node/vm.ts to confirm the listed keys (contextName/contextOrigin/contextCodeGeneration/importModuleDynamically/microtaskMode) each throw with the expected message on that path. The PR description states 9/17 new tests fail on the unfixed binary and all 97 test-vm-* Node parallel tests pass, and notes composition with #38326 for the remaining wrapper-level { name: 1 } divergence. No outstanding reviewer comments.
|
Updated 5:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit b7ccc51 has some failures in 🧪 To try this PR locally: bunx bun-pr 38409That installs a local version of the PR into your bun-38409 --bun |
|
Checked against #39588 on a build of its branch: not covered, so this still applies. #39588 validates |
Problem
new vm.Script("1").runInNewContext({}, { contextName: 1 })runs and returns 1; Node throwsERR_INVALID_ARG_TYPE: The "options.contextName" property must be of type string. Received type number (1). Same forcontextOrigin.new vm.Script("1").runInNewContext({}, { name: 1 })throwsThe "options.name" property must be of type string; Node runs it,name/originare notrunInNewContext()options. Same fororigin.getNodeVMContextOptions()(src/jsc/bindings/NodeVM.cpp:659, :669) always reads thename/originkeys, which arecreateContext()'s spelling, but it is also the option parser forScript#runInNewContext()(src/jsc/bindings/NodeVMScript.cpp:645), where Node readscontextName/contextOrigininstead. Only the codeGeneration key was switched per caller.contextName/contextOrigincheck that madevm.runInNewContext()appear to work was inScriptOptions::fromJS()(src/jsc/bindings/NodeVMScript.cpp:35), the Script constructor's option parser. Node's Script constructor has no such check, sonew vm.Script("1", { contextName: 1 }),vm.runInThisContext("1", { contextName: 1 })andvm.runInContext("1", ctx, { contextName: 1 })threw in Bun and run in Node.Fix
getNodeVMContextOptions()takes aNodeVMContextOptionKeys(name, origin, codeGeneration key names) instead of just the codeGeneration key.createContext()passes{ name, origin, codeGeneration }, the tworunInNewContext()entry points pass{ contextName, contextOrigin, contextCodeGeneration }; error messages are built from the key that was read.ScriptOptions::fromJS()no longer readscontextName/contextOrigin.Script#runInNewContext()validates them itself now, sovm.runInNewContext()(which calls it) still rejects them with the same message, and the constructor /runInThisContext()/runInContext()ignore them like Node.createContext()readsname/originandgetContextOptions()(used byrunInNewContext()) readscontextName/contextOrigin, and nothing else reads either pair. Every expectation in the new tests was run against node v26.3.0 as well.context name/origin options(9 of its 17 tests fail on the unfixed binary), plus the throwing-getter matrix in the same file now lists the keys each entry point reads.vm.runInNewContext('', {}, { contextName: null })).vm.runInNewContext()still passes its raw options tocreateContext(), so{ name: 1 }through that wrapper still throws; that wrapper-level remap is what node:vm: run runInNewContext() in the sandbox's existing context #38326 adds, and it composes with this change (its nativeScript#runInNewContext()path still goes throughgetNodeVMContextOptions()).Background
node:vmhas two ways to configure a context.vm.createContext(sandbox, options)takes{ name, origin, codeGeneration, microtaskMode }.vm.runInNewContext()andScript#runInNewContext()create a context and run in one call, so they take those same options on the combined options object under the namescontextName,contextOrigin,contextCodeGeneration(andmicrotaskMode), next to the script/run options likefilenameandtimeout. Node's lib/vm.jsgetContextOptions()does this remap and validates under theoptions.contextNamespelling.getNodeVMContextOptions(); the name/origin values are only validated (JSC has no use for a context name), so the bug is purely about which key is checked and which message is thrown.ScriptOptions::fromJS()parses the options given tonew vm.Script(). Bun's JS wrappers (vm.runInNewContext()etc.) hand the same options object tonew Script()and then to the run method, which is how a check in the constructor could stand in for the missing one inrunInNewContext().Behavior matrix, node v26.3.0 vs bun before / after