Conversation
NodeVMGlobalObject::put forwarded every store to the sandbox before consulting the global object's own attributes, so a sloppy-mode `globalThis.NaN = 123` inside a vm context created a fresh writable `NaN` on the sandbox that shadowed the non-writable global on the next read. The strict-mode path threw the expected TypeError but only after the sandbox had already been mutated. Mirror the guard that defineOwnProperty already has and that Node's contextify PropertySetterCallback applies: if the property exists on the global object as ReadOnly, route straight to Base::put so the ordinary sloppy no-op / strict TypeError semantics apply and the sandbox is left untouched.
|
Updated 12:38 PM PT - Jul 13th, 2026
✅ @robobun, your commit 20b8ba12bd0d8b76c450b6258c7ea8c9fa158feb passed in 🧪 To try this PR locally: bunx bun-pr 34071That installs a local version of the PR into your bun-34071 --bun |
|
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 (1)
WalkthroughRead-only global assignments in isolated VM contexts now bypass sandbox forwarding, with tests covering sloppy no-op writes and strict-mode errors. ChangesRead-only VM globals
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/vm/vm.test.ts`:
- Around line 423-447: Strengthen both read-only-global tests by asserting that
each tested name is not an own property of the sandbox context, rather than
relying only on Object.keys(context). Add the check for NaN, undefined, and
Infinity alongside the existing value and descriptor assertions, while
preserving the current mutation and strict-assignment expectations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6be428ea-29fc-44fa-b7d5-78702df1e86b
📒 Files selected for processing (2)
src/jsc/bindings/NodeVM.cpptest/js/node/vm/vm.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this touches NodeVMGlobalObject::put in the JSC bindings — vm sandboxing semantics are subtle enough that a human should confirm.
Checked: the new guard mirrors the existing pattern in defineOwnProperty (same qualified JSC::JSGlobalObject::getOwnPropertySlot call to bypass sandbox interception); exception scoping is correct (RETURN_IF_EXCEPTION after the lookup, RELEASE_AND_RETURN on the tail call).
Checked: only ReadOnly is tested here vs. ReadOnly && DontDelete in defineOwnProperty — this matches Node's PropertySetterCallback vs PropertyDefinerCallback split, so the asymmetry is intentional.
Checked: accessor properties won't trip the guard (no ReadOnly bit), so sandbox setters still fire; the notContextified path is unaffected since the guard runs before that branch.
Extended reasoning...
Overview
Adds a 13-line guard to NodeVMGlobalObject::put in src/jsc/bindings/NodeVM.cpp: before forwarding a [[Set]] to the sandbox, check whether the property already exists on the global object itself with the ReadOnly attribute; if so, delegate to Base::put (which applies the standard sloppy no-op / strict TypeError) instead of letting the sandbox acquire a shadowing own property. Two new tests in test/js/node/vm/vm.test.ts run across all four runInContext variants.
Security risks
node:vm is sandboxing-adjacent. This change tightens behavior (prevents mutation of non-writable globals) rather than loosening it, so the security direction is favorable. The lookup uses the base-class JSC::JSGlobalObject::getOwnPropertySlot explicitly, so it cannot be influenced by a hostile sandbox object's own traps. I don't see a way for this to weaken isolation.
Level of scrutiny
Medium-high. The diff is small and closely mirrors the existing defineOwnProperty guard in the same file, and the PR description cites the corresponding Node contextify callback. However, JSC method-table overrides interact in subtle ways (property slot receivers, InternalMethodType, ordering vs. the later isDeclaredOnSandbox lookup), and there's an edge case worth a maintainer's eye: when the sandbox already has an own property of the same name (e.g. createContext({NaN: 42})), writes now become no-ops because the guard consults the global rather than the sandbox — that appears to match Node, but it's the kind of semantic detail a human familiar with this file should confirm.
Other factors
- The author notes a textual overlap with #33486 that will need a rebase whichever lands second.
- CodeRabbit's one comment (test assertion strength) was addressed and resolved.
- Exception-check discipline (
RETURN_IF_EXCEPTION/RELEASE_AND_RETURN) looks correct; verified against neighboring code. - Node compat suite (
test-vm-*.js) reportedly still passes per the PR description.
Given this is C++ JSC bindings in the vm contextification layer, I'm deferring rather than approving.
|
Re the The guard consults the global object's own |
|
#39588 reworks how a contextified global resolves against its sandbox and covers this case. Its |
1 participant
What
Inside a
node:vmcontext, a plain sloppy-mode assignment could overwrite non-writable globals:The strict-mode path threw the expected
TypeError, but only after the sandbox had already been mutated, so a subsequent read still saw the overwritten value.Why
NodeVMGlobalObject::putforwarded every store to the sandbox viasandbox->methodTable()->put(...)without first checking whether the property already exists on the global object asReadOnly. The extensible sandbox happily acquired a fresh ownNaNthat then shadowed the non-writable global on the nextgetOwnPropertySlot.NodeVMGlobalObject::defineOwnPropertyalready carries exactly this guard, and Node's contextifyPropertySetterCallbackintercepts and returns early when the global's own attributes includeReadOnly.Fix
Before touching the sandbox, look up the property on the global object itself (bypassing the sandbox interception via
JSC::JSGlobalObject::getOwnPropertySlot). If it exists withReadOnly, delegate toBase::put, which applies the ordinary sloppy-mode no-op / strict-modeTypeErrorsemantics and leaves the sandbox untouched.Tests
Added to the shared
testRunInContextharness so they run againstrunInContext,runInNewContext,Script#runInContext, andScript#runInNewContext:globalThis.{NaN,undefined,Infinity} = 123is a no-op; the descriptor stays{writable:false, configurable:false}and the sandbox gains no keysglobalThis.NaN = 5throwsTypeErrorand leaves both the global value and the sandbox unmodifiedAll 97
test/js/node/test/parallel/test-vm-*.jsNode compat tests continue to pass.Note: this overlaps textually with #33486, which also adds a
JSGlobalObject::getOwnPropertySlotcall inputfor a different purpose (sandbox as store for guest-created globals). Whichever lands second will need a small rebase; the two guards are independent.[stamp-90s] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file