Repository navigation
Conversation
|
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 (4)
WalkthroughChangesThe pull request centralizes object validation, applies it to nested Node VM options, and adds tests for indexed error paths and accepted or rejected values. VM validation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: ready for review. Code is at d8a3399 (9bd6aca is an empty CI re-run). CI (build 95904): 177 of 179 jobs passed, including both macOS builds and the node:vm tests on every Linux and Windows lane. The two Reproduced on bun 1.4.0 and on main with Review feedback is addressed: the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — swaps the hand-written isObject() check for the existing V::validateObject() helper so arrays/functions are rejected like Node.
What was reviewed:
getNodeVMContextOptions():asObject(codeGenerationValue)on the next line is still safe (validateObject guaranteesisObject());RETURN_IF_EXCEPTIONcovers the revoked-Proxy throw fromJSC::isArray.V::validateObjectsignature wideningASCIILiteral→const WTF::String&: all ~18 existing"..."_scallers (crypto/*, BunProcess, JSWorker) convert implicitly;ERR::INVALID_ARG_TYPEalready has theWTF::String&name overload.- Tests: 3 entry points × {array, function/class/arrow, Proxy, null/primitives, valid objects/undefined} with exact name/code/message assertions.
Extended reasoning...
Overview
Two-line fix in src/jsc/bindings/NodeVM.cpp replacing a hand-written !codeGenerationValue.isObject() + ERR::INVALID_ARG_TYPE with a call to the existing V::validateObject() helper (Node's validateObject() port), so codeGeneration/contextCodeGeneration reject arrays and functions the way Node does. The V::validateObject signature in NodeValidator.{h,cpp} widens its name parameter from ASCIILiteral to const WTF::String& so the caller can pass makeString("options."_s, codeGenerationKey). ~70 lines of new tests in test/js/node/vm/vm.test.ts.
Security risks
None. This tightens option validation on a Node compat surface; no new code paths that read/allocate/execute anything.
Level of scrutiny
Low. This is a targeted Node.js compatibility fix that reuses an existing, already-tested in-tree helper at the shared layer (getNodeVMContextOptions() serves all three entry points). The only non-mechanical piece is the parameter-type widening, which I checked: ASCIILiteral implicitly converts to WTF::String, and grepping shows ~18 existing call sites all pass "..."_s literals. ERR::INVALID_ARG_TYPE (called inside validateObject) already has a const WTF::String& name overload (ErrorCode.h:89), so the body needs no change.
Other factors
- Exception handling is correct:
V::validateObjectcallsJSC::isArray(throws on revoked Proxy) and checks the scope; the new call site follows withRETURN_IF_EXCEPTION(scope, ). The subsequentasObject(codeGenerationValue)is safe becausevalidateObjectonly returns cleanly whenvalue.isObject()and it's not an array/callable — still an object. - The undefined case is handled by the pre-existing early return above the changed lines, so
validateObjectnever sees it (matching Node, where the option is optional). - Test coverage is thorough per REVIEW.md: covers all three sibling entry points via
describe.each, asserts exact error class/code/message (not baretoThrow()), includes Proxy-of-array/Proxy-of-function edge cases, and re-asserts the previously-working null/primitive rejections and valid-object acceptances haven't regressed. - PR description states the new tests were verified under
BUN_JSC_validateExceptionChecks=1and that the otherV::validateObjectcallers' tests still pass.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #38326, #34623 or #33077. Each of those adds a JS-side On current main, with any of those three applied, |
…#38381) ### Problem - `vm.createContext({}, [])`, `vm.createContext({}, function () {})`, `new vm.Script("1", [])`, `vm.compileFunction("1", [], [])`, `vm.runInThisContext("1", [])`, `script.runInThisContext([])`, `script.runInContext(ctx, [])` and `script.runInNewContext({}, [])` are all accepted. Node throws from every one of them: `TypeError [ERR_INVALID_ARG_TYPE]: The "options" argument must be of type object. Received an instance of Array` (`Received function foo` for a function). - Cause: both native checks of the `options` argument use `JSValue::isObject()`, which is true for arrays and functions: `vmModule_createContext()` and `BaseVMOptions::fromJS()` in `src/jsc/bindings/NodeVM.cpp`. Node's lib/vm.js checks the same argument with `validateObject(options, 'options')`. - `null` and primitives were already rejected by those checks, so arrays and functions were the only values slipping through. ### Fix - Both checks now call `Bun::V::validateObject()` (`src/jsc/bindings/NodeValidator.cpp`), the native port of Node's `validateObject()` that `BunProcess.cpp` already uses. The error has Node's code and message; a Proxy around an array is reported as `an instance of Array` too, like `Array.isArray`. - `vm.runInContext()` and `vm.runInNewContext()` spread `options` into a fresh object, as lib/vm.js does, so they still accept any non-string options value. Node accepts `[]`, a function, `null` and `1` there; before this change the two wrappers handed the value straight to `Script`, so they rejected `null`/`1` and would have started rejecting arrays and functions as well. - `Script#runInNewContext()` is left in Node's order: context options are read and the context is created, then `runInContext()` rejects the value. `getNodeVMContextOptions()` already tolerates non-objects the way Node's `getContextOptions()` does, so it needed no change. - Why this is right: every entry point whose `options` reaches `validateObject()` in lib/vm.js (`createContext`, the `Script` constructor, `getRunInContextArgs()` behind the three run methods, `compileFunction`) now rejects exactly what it rejects, and the two wrappers whose `options` never reaches it still accept everything. The whole matrix below was checked against node v26.3.0; the only remaining differences are the two pre-existing ones noted there, which #38373 covers and which are about the context argument, not `options`. - Verified with `test/js/node/vm/vm.test.ts` (`the options argument`): 36 cases, 22 fail on the current build (arrays, proxied arrays and functions across the 7 entry points, plus the wrapper case), all pass with the fix. - All 97 `test/js/node/test/parallel/test-vm-*` files and the 3 sequential ones pass with the fix; the rest of `test/js/node/vm` passes as well. ### Background - Node's `validateObject(value, name)` (lib/internal/validators.js) throws `ERR_INVALID_ARG_TYPE` for `null`, anything `Array.isArray()` accepts, functions, and non-objects. `Bun::V::validateObject` implements the same four checks natively (`isNull`, `JSC::isArray`, `isCallable`, `!isObject`). - `BaseVMOptions::fromJS()` parses the options shared by all scripts (`filename`, `lineOffset`, `columnOffset`). `ScriptOptions::fromJS` (the `Script` constructor), `RunningScriptOptions::fromJS` (`runInThisContext`, `runInContext`, `runInNewContext`) and `CompileFunctionOptions::fromJS` (`compileFunction`) all call it first, so it is the one place the `options` argument is type-checked for those entry points. - lib/vm.js's `runInContext()` and `runInNewContext()` never pass the caller's value on: they build a new object with `{ ...options }` (and `runInNewContext()` derives the context options from it separately). That is why `vm.runInNewContext("1", {}, [])` works in Node while `new vm.Script("1").runInNewContext({}, [])` throws. Related open PRs, all independent of this one: #38371 (same change for `options.codeGeneration`), #38373 (`createContext()` returns an existing context before looking at `options`), #38326 (`Script#runInNewContext()` reusing an existing context). #38317 also copies `options` in the two wrappers, there in order to attach the parsing context to the copy; the native check in this PR is what makes the copy matter for validation, and whichever of the two lands second only has to drop its duplicate copy line. <details> <summary>node v26.3.0 vs bun, options argument matrix</summary> `ok` means the call succeeded; otherwise the error code is shown. `existing` is a sandbox that was already passed to `createContext()`. | call | node | bun before | bun after | | --- | --- | --- | --- | | `createContext({}, [])` | ERR_INVALID_ARG_TYPE | ok | ERR_INVALID_ARG_TYPE | | `createContext({}, function () {})` | ERR_INVALID_ARG_TYPE | ok | ERR_INVALID_ARG_TYPE | | `createContext({}, null)` / `createContext({}, 1)` | ERR_INVALID_ARG_TYPE | ERR_INVALID_ARG_TYPE | ERR_INVALID_ARG_TYPE | | `createContext(undefined, [])` / `createContext(DONT_CONTEXTIFY, [])` | ERR_INVALID_ARG_TYPE | ok | ERR_INVALID_ARG_TYPE | | `new Script("1", [])` / `new Script("1", function () {})` | ERR_INVALID_ARG_TYPE | ok | ERR_INVALID_ARG_TYPE | | `compileFunction("1", [], [])` / `(..., function () {})` | ERR_INVALID_ARG_TYPE | ok | ERR_INVALID_ARG_TYPE | | `vm.runInThisContext("1", [])` / `(..., function () {})` | ERR_INVALID_ARG_TYPE | ok | ERR_INVALID_ARG_TYPE | | `script.runInThisContext([])` / `(function () {})` | ERR_INVALID_ARG_TYPE | ok | ERR_INVALID_ARG_TYPE | | `script.runInContext(existing, [])` / `(..., function () {})` | ERR_INVALID_ARG_TYPE | ok | ERR_INVALID_ARG_TYPE | | `script.runInNewContext({}, [])` / `(..., function () {})` | ERR_INVALID_ARG_TYPE | ok | ERR_INVALID_ARG_TYPE | | `vm.runInContext("1", existing, [])` / function | ok | ok | ok | | `vm.runInNewContext("1", {}, [])` / function | ok | ok | ok | | `vm.runInContext("1", existing, 1)` / `null` / `true` / symbol | ok | ERR_INVALID_ARG_TYPE | ok | | `vm.runInNewContext("1", {}, 1)` / `null` / `true` / symbol | ok | ERR_INVALID_ARG_TYPE | ok | Unchanged pre-existing differences, not about `options`: | call | node | bun | | --- | --- | --- | | `createContext(existing, [])` | ok (returns before validating options) | ERR_INVALID_ARG_TYPE (#38373) | | `createContext(function () {}, {})` | ERR_INVALID_ARG_TYPE (`"object"` argument) | ok | </details>
|
Updated 7:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit 9bd6aca has some failures in 🧪 To try this PR locally: bunx bun-pr 38371That installs a local version of the PR into your bun-38371 --bun |
…eration getNodeVMContextOptions() only checked isObject() on the codeGeneration container, so an array or a function was accepted where Node's validateObject() throws ERR_INVALID_ARG_TYPE. Use V::validateObject(), whose name parameter now takes a WTF::String so the option name can be built from the codeGeneration/contextCodeGeneration key.
…way; keep the ASCIILiteral validateObject overload validateObject() keeps its ASCIILiteral overload and gains a WTF::String one; both (and the JS-facing host function) share one implementation, so callers passing literals do not build a String on the success path. The contextExtensions[i] check in CompileFunctionOptions::fromJS() was the other isObject() check standing in for Node's validateObject(); it now uses the helper too, and reports the offending index instead of always [0].
f5cac30 to
d6b8806
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
robobun
changed the title
There was a problem hiding this comment.
LGTM — tightens two node:vm option checks to match Node's validateObject() semantics.
What was reviewed:
- The two
NodeVM.cppcall sites:RETURN_IF_EXCEPTIONfollows each newV::validateObjectcall, andasObject(codeGenerationValue)remains safe sincevalidateObjectonly returns success for real objects. validateObjectImpl<Name>refactor: verifiedERR::INVALID_ARG_TYPEhas overloads forJSValue,WTF::String(andASCIILiteralvia implicit conversion, error-path only), so all three instantiations resolve; behavior of the pre-existingASCIILiteralandjsFunction_validateObjectpaths is unchanged.- Tests cover all three entry points × arrays/functions/proxies/primitives/null and the per-index
contextExtensionsmessage; still-accepted cases ({},Object.create(null),undefined) are asserted too.
Extended reasoning...
Overview
Two option checks in src/jsc/bindings/NodeVM.cpp that used JSValue::isObject() (which accepts arrays and functions) are replaced with V::validateObject(), matching Node's lib/vm.js. The contextExtensions[i] message now carries the real index instead of a hard-coded [0]. To support runtime-built names, NodeValidator.{h,cpp} gains a const WTF::String& overload of V::validateObject; the two overloads and jsFunction_validateObject are collapsed into one static validateObjectImpl<Name> template so the logic isn't duplicated three ways. Tests in test/js/node/vm/vm.test.ts cover the full value/entry-point matrix.
Security risks
None. This strictly tightens input validation to match Node; nothing is loosened. The one intentional divergence (still rejecting null in contextExtensions where Node aborts on a native assertion) is the safer choice and unchanged from before.
Level of scrutiny
Low–medium. It's a small Node-compat validation fix following the exact pattern already landed in #38381 for the options argument itself in the same file. The NodeValidator refactor is a pure de-duplication — I confirmed ERR::INVALID_ARG_TYPE has JSValue and const WTF::String& name overloads (ErrorCode.h), so all three template instantiations compile and the ASCIILiteral success path stays allocation-free. Exception handling is correct: each new validateObject is followed by RETURN_IF_EXCEPTION with the right sentinel (void return in getNodeVMContextOptions, {} = false in CompileFunctionOptions::fromJS).
Other factors
No CODEOWNERS for these files. The comment-cop bot's inline note was addressed in d8a3399 (the explanatory comment on validateObjectImpl was dropped). The PR description documents that vendored Node tests (test-vm-basic.js, test-vm-codegen.js, test-vm-options-validation.js) and the other native validateObject callers' tests still pass, and that the new tests pass under BUN_JSC_validateExceptionChecks=1. The overlap with #38326/#34623/#33077 is JS-side-only for vm.runInNewContext and composes cleanly with this native fix.
There was a problem hiding this comment.
LGTM — focused Node-compat validation fix following the same pattern as #38381.
What was reviewed:
- The two
isObject()→V::validateObject()conversions in NodeVM.cpp: both followed byRETURN_IF_EXCEPTION, andasObject(codeGenerationValue)after the check is still safe since validateObject rejects everything non-object. - The
validateObjectImpl<Name>template: byte-identical logic to the two functions it replaces; verifiedERR::INVALID_ARG_TYPEhas overloads for all three name types (JSValue,WTF::String, andASCIILiteral→String), so each instantiation compiles to the same error path as before. makeStringnow runs on the success path at the two new call sites — unavoidable for a runtime-built name, and this is cold option-parsing code; the ~20 existingASCIILiteralcallers are unaffected.- Tests cover the full matrix (3 entry points × arrays/functions/classes/arrows/proxies/null/primitives + positive cases) with exact code/name/message assertions, and the
contextExtensions[i]index fix.
Extended reasoning...
Overview
Converts the last two JSValue::isObject() option checks in src/jsc/bindings/NodeVM.cpp to V::validateObject(), so codeGeneration/contextCodeGeneration (in getNodeVMContextOptions(), shared by createContext, vm.runInNewContext, Script#runInNewContext) and each contextExtensions[i] entry (in CompileFunctionOptions::fromJS()) now reject arrays and functions with ERR_INVALID_ARG_TYPE, matching Node's validateObject(). Also fixes the hardcoded contextExtensions[0] index to the real i. To support runtime-built names, adds a const WTF::String& overload of V::validateObject and folds all three overloads (JSValue / ASCIILiteral / String) into one validateObjectImpl<Name> template. 81 lines of tests in vm.test.ts.
Security risks
None. This tightens input validation on a Node-compat surface. No new codepaths accept previously-rejected input; the only behavioral change is that arrays/functions passed as these options now throw instead of being silently treated as {}. The contextExtensions: [null] case (which Node lets through and then aborts on a native assertion) continues to be rejected cleanly, unchanged from before.
Level of scrutiny
Low-to-medium. This is a small (~24 native lines) Node-compat validation change that follows the exact pattern of #38381, which already landed and converted the sibling options-argument checks. The validateObjectImpl refactor touches a helper used by ~20 other callers (crypto, process, Worker), but the body is line-for-line identical to the two functions it replaces — only the name-parameter type is templated, and I confirmed ERR::INVALID_ARG_TYPE in ErrorCode.h has matching overloads for each instantiation. Exception discipline is preserved: isArray (which can throw on a revoked Proxy) is still followed by RETURN_IF_EXCEPTION, and both new NodeVM.cpp call sites check the scope after validateObject before calling asObject(). The description reports the change was run under BUN_JSC_validateExceptionChecks=1 and against the vendored test-vm-* and other validateObject-consumer test files.
Other factors
- Tests are strong: they cover the full variant matrix per REVIEW.md guidance (all three entry points, arrays, named/arrow/class functions, Proxy-of-array, Proxy-of-function, null, primitives, and the still-accepted plain-object/
Object.create(null)/undefinedcases), assert exactname/code/message, and verify the real-index fix forcontextExtensions. The Proxy-of-function case sensibly asserts only the code, with a comment explaining the knowndetermineSpecificTypeformatting divergence. makeStringfor the option name now runs on the success path too, but only at these two new call sites (rarely-used options); the ASCIILiteral overload is kept precisely so the existing hot callers don't allocate.- The comment-cop bot's feedback (paragraph-long comment on the template) was addressed in d8a3399 and the thread is resolved.
- The find-duplicate-prs bot flagged three overlapping PRs; the author's response is correct — those only cover the JS-side
vm.runInNewContextpath, while this PR fixes the shared native check thatcreateContextandScript#runInNewContextalso reach. They compose cleanly.
This branch has not been deployed
1 participant
Problem
vm.createContext({}, { codeGeneration: [] })and{ codeGeneration: function () {} }are accepted; Node throwsERR_INVALID_ARG_TYPE: The "options.codeGeneration" property must be of type object. Received an instance of Array(orReceived function ...). Same forcontextCodeGenerationonvm.runInNewContext()andScript#runInNewContext(), which return the script's result instead of throwing.vm.compileFunction("", [], { contextExtensions: [[]] })and[function () {}]are accepted too (Node:The "options.contextExtensions[0]" property must be of type object. Received an instance of Array), and a bad entry at any index is reported ascontextExtensions[0]; Node names the real index.src/jsc/bindings/NodeVM.cppuseJSValue::isObject(), which is true for arrays and functions: thecodeGeneration/contextCodeGenerationcontainer ingetNodeVMContextOptions()(line 715; all three entry points read the option through it) and thecontextExtensions[i]loop inCompileFunctionOptions::fromJS()(line 2131, with a hard-coded[0]in the message). Node's lib/vm.js checks both withvalidateObject().optionsargument itself toV::validateObject()and left these two, which need a name built at runtime, to this PR. After it, these are the lastisObject()option checks in the file.1/nullcases that were already rejected (test-vm-codegen.js, test-vm-basic.js).Fix
V::validateObject()(src/jsc/bindings/NodeValidator.cpp), Bun's native port of Node'svalidateObject(), with the name built withmakeString("options." + key,"options.contextExtensions[" + i + "]").nulland primitives produce the same error as before; thecontextExtensionsmessage now carries the real index.V::validateObject()gets aconst WTF::String&overload next to the existingASCIILiteralone. The two overloads and the JS-facingjsFunction_validateObject(require("internal/validators").validateObject, whose name is aJSValue) now share one implementation,validateObjectImpl<Name>; the name is only converted when an error is thrown, so the 20 existing"..."_scallers do not build aStringon the success path.validateObject()rejectsnull,Array.isArray(value)and functions;V::validateObject()uses JSC'sisArray()(the IsArray operation, so a Proxy around an array is rejected too) andisCallable(), and checks for an exception afterisArray()because a revoked Proxy throws there (Node throws a TypeError fromArray.isArrayin that case as well). Error name, code and message are identical to Node's for arrays, named functions, classes,null, primitives and thecontextExtensionsindex. One intentional difference: Node passesnullthrough itscontextExtensionscheck (kValidateObjectAllowNullable) and then dies on a native assertion (Assertion failed: val->IsObject(), node v26.3.0); Bun keeps rejecting it withERR_INVALID_ARG_TYPE, as it did before.test/js/node/vm/vm.test.ts: "the codeGeneration option itself is checked like Node's validateObject()" (3 entry points x arrays, function/class/arrow, Proxy-of-array, Proxy-of-function, and the still-rejectednull/primitives and still-accepted objects/undefined) and "each contextExtensions entry is checked like Node's validateObject(), under its own index". 10 of the 16 tests fail on bun 1.4.0, all pass with this change; the whole file passes (266).test-vm-basic.js(asserts thecontextExtensions[0]message),test-vm-codegen.js,test-vm-options-validation.jsand the othertest-vm-*context*/*create*node tests; for the othervalidateObjectusers,process-execve.test.ts,test-process-threadCpuUsage-*.js,test-crypto-prime.js,test-crypto-keygen*.js,test-crypto-key-objects.js,test-crypto-dh-stateless.js,test-vm-module-*.jsandtest-vm-measure-memory*.js(JS-sidevalidateObject); the new tests also pass underBUN_JSC_validateExceptionChecks=1.functionwithout a name (Node prints the target's name), which is adetermineSpecificTypedetail tracked separately.strings/wasmreads a few lines below the first hunk (its tests sit next to these, so whichever lands second gets a trivial test conflict); node:vm: run runInNewContext() in the sandbox's existing context #38326, node:vm: run DONT_CONTEXTIFY contexts directly against the real global #34623 and node:vm: reject Object.freeze/seal/preventExtensions on a contextified global #33077 each add a JS-sidegetContextOptions()forvm.runInNewContext()only, whilecreateContext()andScript#runInNewContext()are native and still reach the check fixed here; node:vm: return an existing context from createContext() before validating options #38373 coverscreateContext()validating options for an already-contextified sandbox.Background
createContext(sandbox, { codeGeneration: { strings, wasm } })controls whether code in the new context may useeval/new Functionand compile WebAssembly;vm.runInNewContext()andScript#runInNewContext()take the same object ascontextCodeGeneration.compileFunction(code, params, { contextExtensions: [obj, ...] })wraps the function in onewith-style scope per object. Bun reads the first group ingetNodeVMContextOptions()and the second inCompileFunctionOptions::fromJS(), both in NodeVM.cpp.validateObject(value, name)(lib/internal/validators.js) is the check Node applies to option objects:ERR_INVALID_ARG_TYPEfornull, non-objects, arrays and functions.Bun::V::validateObjectis the native equivalent, used by thenode:crypto,processandWorkerbindings;jsFunction_validateObjectexposes the same check to Bun's built-in JS modules.JSC::isArray(globalObject, value)implements the spec's IsArray: true for arrays and for Proxies whose target is an array, and it throws for a revoked Proxy, hence the throw-scope check right after it.JSValue::isCallable()is the native equivalent oftypeof value === "function".ASCIILiteralis WTF's type for a"..."_sstring literal; converting one to aWTF::Stringallocates a smallStringImplheader, which is why the literal overload is kept rather than widening the parameter.Probe: node v26.3.0 vs bun 1.4.0
[review] gate passed · iteration 1 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
The vm option parsing in getNodeVMContextOptions() and in the compileFunction contextExtensions loop only checked JSValue::isObject(), which is true for arrays and functions, so codeGeneration, contextCodeGeneration and each contextExtensions entry accepted values that Node's validateObject() rejects with ERR_INVALID_ARG_TYPE. The fix replaces those checks with the shared V::validateObject() helper, which rejects null, arrays (via isArray, so proxies are covered) and callables using the same option name and "object" type as Node; a WTF::String overload was added so the contextExtensions nam…