Repository navigation
Conversation
isContext(), createContext(), Script#runInNewContext() and
Script#runInContext() checked the context argument with
JSValue::isObject(), which is true for functions, so a function could be
contextified or silently reported as "not a context". Node validates the
argument in isContext() with validateObject(object, "object",
kValidateObjectAllowArray), which every other entry point goes through.
Add NodeVM::validateContextArg() implementing that check and use it at
the four native sites. This also gives createContext() and
Script#runInContext() Node's argument name ("object" instead of
"context") for primitives, and turns Script#runInNewContext()'s plain
TypeError ("Context must be an object") into the same ERR_INVALID_ARG_TYPE.
|
Warning Review limit reached
Next review available in: 40 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 bun 1.4.0 and main with Review so far has no findings to address; waiting on CI. |
There was a problem hiding this comment.
LGTM — small, well-scoped Node compat fix.
What was reviewed
validateContextArg():!isObject() || isCallable()correctly maps to Node'svalidateObject(..., kValidateObjectAllowArray)(arrays pass, functions/null/primitives rejected);isCallable()only reached whenisObject()is true.- All four call sites:
RETURN_IF_EXCEPTIONafter each call; downstreamasObject(contextArg)remains safe since validation guaranteesisObject(). getGlobalObjectFromContextnon-throwing path (Module classes with pre-validatedm_context) unchanged — still returns nullptr for non-objects without throwing.src/js/node/vm.tsvalidateContext()/runInNewContext()route through nativeisContext, so the JS-level entry points inherit the fix without changes.
Extended reasoning...
Overview
Adds NodeVM::validateContextArg() (rejects non-objects and callables with ERR_INVALID_ARG_TYPE for argument name "object") and applies it at four native sites: vmModule_isContext, vmModule_createContext, scriptRunInNewContext, and the throwing branch of getGlobalObjectFromContext. Changes the argument name from "context" to "object" and replaces a plain TypeError in scriptRunInNewContext with the coded error, matching Node's lib/vm.js which routes every context argument through isContext() → validateObject(object, 'object', kValidateObjectAllowArray). ~25 lines of production change plus a ~100-line test block.
Security risks
None. This tightens input validation on a Node-compat surface; no new code paths execute user input, and rejecting functions where they were previously accepted only reduces the input space.
Level of scrutiny
Low-to-medium. Mechanical extraction of a shared validator applied at four sites, each already had an isObject() check that is being replaced/augmented. The predicate isObject() && !isCallable() is exactly typeof x === 'object' && x !== null, which is what Node's kValidateObjectAllowArray accepts. Checked that:
getContextArg()still runs first at the two contextifying sites, soundefined/DONT_CONTEXTIFYbecome a fresh object before validation (covered by tests).- The
canThrow=falsecallers ofgetGlobalObjectFromContext(NodeVMModule/NodeVMSourceTextModule/NodeVMSyntheticModule, all passing an internalm_context) keep their prior silent-nullptr behavior. - Downstream
asObject()calls are guarded by the new validation. - Exception scopes: every call site has
RETURN_IF_EXCEPTIONimmediately after; PR states it was run underBUN_JSC_validateExceptionChecks=1. - The unused
ArgList argsinvmModule_isContextwas correctly dead (callFrame->argument(0)is used directly).
Other factors
Test coverage is thorough for a validation change: 6 entry points × 12 rejected shapes (including bound functions, callable Proxies, classes) with exact Node v26.3.0 messages, plus positive cases (arrays contextify, contextified array works everywhere, non-context objects still get the second-tier vm.Context error). PR description confirms 58 new cases fail on the released binary and pass with the fix, and that all 97 ported test-vm-* Node tests still pass. The one intentionally-skipped sibling (vmModuleRunInNewContext, unreachable and being deleted in #38326) is called out explicitly.
Problem
vm.isContext(function () {})returnsfalse,vm.createContext(fn)contextifies the function, andnew vm.Script("1").runInNewContext(fn)runs. Node throwsTypeError [ERR_INVALID_ARG_TYPE]: The "object" argument must be of type object. Received functionfrom all of them.vm.runInContext(),vm.runInNewContext()andScript#runInContext()do reject a function, but with the second-tier error (The "contextifiedObject" argument must be an vm.Context) instead of the type error above.JSValue::isObject(), which is true for functions:vmModule_isContext(src/jsc/bindings/NodeVM.cpp:1718),vmModule_createContext(NodeVM.cpp:1657),scriptRunInNewContext(src/jsc/bindings/NodeVMScript.cpp:636) andgetGlobalObjectFromContext(NodeVM.cpp:759, used byScript#runInContext).src/js/node/vm.tsdoes not need changes: itsvalidateContext()delegates to the nativeisContext.createContext(1)andscript.runInContext(1)name the argument"context"(Node:"object"), andscript.runInNewContext(1)throws a plainTypeError: Context must be an objectwith nocode.Fix
NodeVM::validateContextArg(): rejects anything that is not an object or that is callable, withERR_INVALID_ARG_TYPEfor"object", and uses it at the four sites.getGlobalObjectFromContext()applies it on its throwing path (Script#runInContext); its non-throwing callers (the Module classes, which pass an already validated context or the main global) keep returning null for non-objects as before.lib/vm.jsisContext()validates withvalidateObject(object, 'object', kValidateObjectAllowArray), andcreateContext(),runInContext(),runInNewContext()andScript#runInContext()/runInNewContext()all validate the context throughisContext().isObject() && !isCallable()istypeof x === "object" && x !== null, which is exactly what that validator accepts: arrays still pass, functions, null and primitives do not.vm.constants.DONT_CONTEXTIFYandundefinedare still turned into a fresh sandbox bygetContextArg()before the check runs, so those paths are unchanged (covered by the new tests and the existingDONT_CONTEXTIFYblock).vmModuleRunInNewContexthas the sameisObject()check but is not reachable (vm.tsimplementsrunInNewContextitself) and is removed by node:vm: run runInNewContext() in the sandbox's existing context #38326, so it is left alone.test/js/node/vm/vm.test.ts, "the context argument": for each of the six entry points, rejection of function declarations, anonymous/arrow/async functions, classes, bound functions and callable Proxies plus null and primitives with Node's exact message (checked against node v26.3.0), arrays/undefined/DONT_CONTEXTIFYstill contextifying, a contextified array being accepted everywhere, and non-context objects still getting thevm.Contexterror. 58 of the new cases fail on the released binary, all pass with the fix.test/js/node/vm/, all 97test/js/node/test/parallel/test-vm-*files (97 pass), and the new block plustest-vm-context.js/test-vm-is-context.js/test-vm-create-context-arg.js/test-vm-basic.jsunderBUN_JSC_validateExceptionChecks=1.Background
createContext()has associated with its ownNodeVMGlobalObjectrealm; Bun records the association in a WeakMap on the main global, andisContext()is a lookup in that map. Contextifying a function would therefore "work" mechanically, which is why nothing crashed; it is simply an input Node rejects.validateObjectflags: the default rejects null, arrays and functions;kValidateObjectAllowArray(used byisContext) additionally lets arrays through, socreateContext([])is valid and must stay valid.getContextArg()is the shared pre-step forcreateContext()/Script#runInNewContext(): it replacesundefinedand theDONT_CONTEXTIFYsymbol with a fresh object, which is why the new check runs after it.Received ...suffix (the new tests use a prefix match for those cases), and the missingReceived ...suffix on thevm.Contexterror.