Repository navigation
Conversation
|
Warning Review limit reached
Next review available in: 1 minute 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 (2)
Comment |
|
Status: ready for review (280b005). Reviews so far have no open threads. Reproduced on bun 1.4.0 (node v26.3.0 prints const vm = require("node:vm");
vm.compileFunction("return leaked", [], { contextExtensions: [{ leaked: 1 }] });
vm.runInThisContext("typeof leaked"); // "number"
new Function("return typeof leaked")(); // "number"
const ctx = vm.createContext({});
vm.compileFunction("return x", [], { parsingContext: ctx, contextExtensions: [{ x: 1 }] });
vm.runInContext("typeof x", ctx); // "number"With this branch all of the above print CI: on the current sha, 177 of 179 jobs are green; the remaining two are the darwin 14 aarch64 test jobs, which have not been able to start (each attempt expires waiting for an agent and is re-queued). The only test failures in the build are retried flakes in unrelated files (napi, terminal, fake-timers, sqlite, resolve). The previous sha (284a961, identical source change apart from a comment) did run on darwin 14 aarch64 and Related PRs touching the same lines: #38297 (parsingContext realm) and #38305 (global let/const visibility); all three are independent and whichever lands later needs a trivial rebase. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — a one-line deletion of a call that leaked realm-wide state, with tests pinning both the fix and the behavior it must not regress.
What was reviewed:
- Confirmed
setGlobalScopeExtensionis not called anywhere else insrc/; the remainingglobalScopeExtension()reads only feedSourceCodeKey, which the description addresses. - Verified
functionScopestill reachesProgramCodeBlock::createandJSFunction::createinsideconstructAnonymousFunction, so the compiled function keeps itsJSWithScopechain — test 1 pins this. - Checked the
lexicallyScopedFeaturesread at NodeVM.cpp:136 uses the caller'sglobalObject(notparsingContext), so its value only changes in the no-parsingContext case, and only for the cachedData decode key.
Extended reasoning...
Overview
This PR removes a single call — options.parsingContext->setGlobalScopeExtension(functionScope) — from vmModuleCompileFunction in src/jsc/bindings/NodeVM.cpp, replaces the adjacent comment with one explaining why the extension chain must live only on the function's scope, and adds three targeted tests to test/js/node/vm/vm.test.ts under a new compileFunction() > contextExtensions describe block.
The bug: setGlobalScopeExtension installs a realm-wide fallback consulted by JSScope::resolve for every unresolved global lookup, and it was never cleared. So after one compileFunction call with contextExtensions, those extensions bled into every subsequent piece of code in that realm. The fix relies on the fact that functionScope (the JSWithScope chain) is already passed as the scope argument to constructAnonymousFunction, which threads it into both ProgramCodeBlock::create and JSFunction::create — so the compiled function's own scope chain already carries the extensions, and the realm-wide hook contributed nothing to the function itself.
Security risks
None introduced. This actually closes a scoping leak where extension objects unexpectedly became visible to unrelated code in the realm. No new user-controlled input paths, no allocation/lifetime changes.
Level of scrutiny
Moderate. The change is one deleted line, but it sits in JSC scope-resolution machinery and Node-compat code. I traced:
- No other
setGlobalScopeExtension/clearGlobalScopeExtensioncallers exist insrc/, so the realm's extension slot is now always null (its default). - The four remaining
globalScopeExtension()reads (NodeVM.cpp ×3, NodeVMScript.cpp, NodeVMSourceTextModule.cpp) only pickTaintedByWithScopeLexicallyScopedFeaturevsNoLexicallyScopedFeaturesfor aSourceCodeKey; the PR description explains why this doesn't affect cachedData compatibility. - The read at NodeVM.cpp:136 inside
constructAnonymousFunctionuses the caller'sglobalObject, so with aparsingContextit was already reading a different global than the one the removed line wrote — the behavior only changes in the no-parsingContext-with-extensions case, and only for the cachedData decode key.
Other factors
The tests are well-constructed: test 1 ("are visible to the compiled function") pins that the function itself still sees extensions (including via closure and direct eval), so the deletion cannot silently break the feature; tests 2 and 3 pin the leak fix across every path the description enumerates (module code, runInThisContext, new Function, indirect eval, previously-compiled functions, and the parsingContext equivalents). The PR states both test-vm-basic.js and isArray-proxy-crash.test.ts pass, and notes the trivial rebase interaction with #38297 and the separate global-lexical-environment issue that is intentionally out of scope.
|
Thanks. One refinement to the third point: the |
|
Updated 7:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit 280b005 has some failures in 🧪 To try this PR locally: bunx bun-pr 38302That installs a local version of the PR into your bun-38302 --bun |
|
Heads up: #38305 (compileFunction creating the function on the global lexical environment so it can see script-level |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…obal scope extension vm.compileFunction installed the JSWithScope chain built from contextExtensions (or, without extensions, the global object itself) as the parsing context's global scope extension and never cleared it. JSC consults that hook whenever identifier resolution reaches the global object, so after one such call the last extension object's properties resolved from any later code in the realm: module code, runInThisContext, new Function, indirect eval, and functions compiled earlier. The extensions already sit in the compiled function's own scope chain, which is what the function resolves through, so the hook is not needed for the function itself. Drop it.
a11a641 to
280b005
Compare
|
Pushed 280b005 (force-push, replaces 284a961 plus an empty retrigger commit): same fix and tests, with the three-line comment flagged by comment-cop dropped, so the source diff is now only the removal of the |
There was a problem hiding this comment.
LGTM — deletes the realm-wide setGlobalScopeExtension call that was never cleared; the JSWithScope chain is already threaded through constructAnonymousFunction as the function's own scope.
What was reviewed:
- Confirmed
functionScopestill reaches bothProgramCodeBlock::createandJSFunction::create, so the compiled function keeps seeing its extensions via its scope chain (test 1 pins this). - Grepped
src/— nothing else sets a global scope extension, so the remainingglobalScopeExtension()reads now consistently yieldNoLexicallyScopedFeatures; per the PR'sSourceCodeKey.hanalysis this does not change the cachedData key. - Tests cover both realms (main +
parsingContext), multiple lookup paths (runInThisContext,new Function, indirecteval, earlier/latercompileFunction), and the shadowing order — the negative assertions would fail on the unfixed build with"string"for the last extension.
Extended reasoning...
Overview
The PR deletes one line from vmModuleCompileFunction in src/jsc/bindings/NodeVM.cpp — the options.parsingContext->setGlobalScopeExtension(functionScope) call — and adds three tests under compileFunction() > contextExtensions in test/js/node/vm/vm.test.ts. The removed call was installing the per-call JSWithScope chain as realm-wide state and never clearing it, so properties of the last contextExtensions entry resolved as globals in every later evaluation in that realm. Node scopes these extensions to the compiled function only.
Security risks
None introduced. node:vm is sandbox-adjacent, but this change strictly removes a leak of per-call scope into realm-wide lookups — it tightens isolation rather than loosening it. The compiled function still sees its extensions because functionScope is passed as the scope argument to constructAnonymousFunction, which uses it for both ProgramCodeBlock::create(..., scope) and JSFunction::create(..., scope, ...). No new user-controlled data reaches native code.
Level of scrutiny
Medium. It is a one-line C++ deletion in JSC bindings, but the PR description traces the mechanism through JSScope::resolve / JSScope::abstractAccess and explains why the deleted hook was redundant for the compiled function (lookups against a chain containing a JSWithScope are marked Dynamic and walk the chain at runtime, hitting the extension before the global). I verified by grep that nothing else in src/ calls setGlobalScopeExtension, so after this change the realm's global scope extension stays null. The remaining globalScopeExtension() reads (NodeVM.cpp:136/443/458, NodeVMScript.cpp:177, NodeVMSourceTextModule.cpp:127) only pick a LexicallyScopedFeatures value for a SourceCodeKey, and the PR's follow-up comment confirms SourceCodeFlags masks that down to the strict-mode bit — so cachedData interop is unchanged.
Other factors
The tests are well-constructed per REVIEW.md guidance: test 1 pins the behavior the function itself relies on (visibility, later-extension-shadows-earlier, closures, direct eval), so the deletion cannot silently regress the intended semantics; tests 2–3 assert the leak is gone across every path the PR enumerates (module typeof, runInThisContext, new Function, indirect eval, an earlier-compiled function, and the parsingContext equivalents). The identifiers fromFirstExtension/fromSecondExtension are unique enough not to collide with the shared main realm the file's other tests run in. The comment-cop feedback about the long code comment was addressed in 280b005. test-vm-basic.js (Node's own contextExtensions/parsingContext/cachedData coverage for compileFunction) is reported passing. The interaction with #38297/#38305 is noted as a trivial rebase, not a semantic conflict.
|
Thanks for the second pass. Nothing outstanding from the reviews at this point: the one inline thread (comment-cop, about the code comment) is resolved in 280b005. CI on that sha is still running; the only change since the first green-minus-darwin run is the comment removal, so the fix and tests are as described above. |
This branch has not been deployed
1 participant
Problem
vm.compileFunction(code, params, { contextExtensions: [ext] })call, the properties of the last extension object resolve as globals from every later piece of code in that realm: ordinary module code,vm.runInThisContext,new Function, indirecteval, and functions that were compiled earlier. WithparsingContextthe same happens inside that context (vm.runInContext("typeof x", ctx)sees a previous compile's extensions). Node scopes the extensions to the compiled function only.vmModuleCompileFunctioninsrc/jsc/bindings/NodeVM.cppcalledoptions.parsingContext->setGlobalScopeExtension(functionScope)before compiling and never cleared it.JSScope::resolve(vendor/WebKit/Source/JavaScriptCore/runtime/JSScope.cpp) consults the realm's global scope extension whenever a lookup reaches the global object, so theJSWithScopechain stayed visible realm-wide. Without extensions the global object itself was installed, which is inert; a later extension-less compile therefore also happened to overwrite an earlier leak, so the symptom depends on call order.Fix
setGlobalScopeExtensioncall. Nothing else in Bun installs a global scope extension, so the realm stays in the same state it is in before anycompileFunctioncall.functionScope(theJSWithScopechain on top of the context's global scope) is both whatProgramCodeBlock::createlinks against and the scope theJSFunctionis created with. When JSC links a lookup against a chain containing aJSWithScopeit marks the lookupDynamic(JSScope::abstractAccess), and the runtime walk then finds the extension object before it ever reaches the global object, so the hook contributed nothing to the function itself. The hook also only ever exposed the head of the chain (JSScope::objectAtScope), which is why only the last extension leaked.cachedDatais unaffected: theglobalScopeExtension()reads that remain inNodeVM.cpp,NodeVMScript.cppandNodeVMSourceTextModule.cpponly chooseTaintedByWithScopeLexicallyScopedFeaturefor aSourceCodeKey, andSourceCodeFlagskeeps just the strict-mode bit of those features (parser/SourceCodeKey.h), so cached data produced before this change still decodes.test/js/node/vm/vm.test.ts(compileFunction() > contextExtensions): the two "not visible" tests fail on the current release ("string"where"undefined"is expected) and pass with this change; the third test pins the behavior the function itself relies on (visibility, later extensions shadowing earlier ones, closures and directevalinside the function).typeof,runInThisContext,new Function, indirecteval, an earlier compiled function, and theparsingContextequivalents) was also checked individually: all leak before, none after, and the output matches node v26.3.0.test/js/node/test/parallel/test-vm-basic.js(coverscontextExtensions,parsingContextandcachedDataforcompileFunction) passes.test/regression/issue/isArray-proxy-crash.test.tspasses.let/constbindings; not changed here). Whichever of these lands later needs a trivial rebase; the three changes are independent.Background
vm.compileFunctionbuilds oneJSWithScopepercontextExtensionsentry (same as awith (ext) {}block) and creates the function with that chain, mirroring how V8'sScriptCompiler::CompileFunctionimplements the option.JSGlobalObject(setGlobalScopeExtension/clearGlobalScopeExtension). When any lookup in the realm reaches the global object and misses, JSC also checks the object installed there. JSC's own users install it around a single evaluation and clear it afterwards, or install it once for the lifetime of an embedding context; it is realm state, not per-function state.[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file