[JSC] Let an embedder allow a Proxy in the prototype chain of its global object - #626
[JSC] Let an embedder allow a Proxy in the prototype chain of its global object#626robobun wants to merge 1 commit into
Conversation
…bal object ProgramExecutable::initializeGlobalProperties throws "Proxy is not allowed in the global prototype chain" when any link of the global object's prototype chain is a Proxy. A global object is normally an immutable prototype exotic object, so this only matters for an embedder that made the prototype mutable. Bun's node:vm global does that, and jsdom 28+ puts a Proxy (WindowProperties) in the chain of its window. V8 runs programs against such a chain, so Node accepts it. Add JSGlobalObject::setAllowsProxyInPrototypeChain() under USE(BUN_JSC_ADDITIONS). When a global object opts in, the check is skipped. Global declaration instantiation only reads own properties of the global object, so no Proxy trap runs while the program links. Name resolution at run time already walks the chain through the ordinary [[HasProperty]], [[Get]] and [[Set]] paths, which handle a Proxy.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it relaxes a security-originated invariant in ProgramExecutable::initializeGlobalProperties and Source/JavaScriptCore is CODEOWNER-covered, a human look would still be worthwhile.
What was reviewed:
- Verified the new
m_allowsProxyInPrototypeChainflag defaults tofalseand both the header member/accessors and the.cppguard compile to the pre-existing behavior whenUSE(BUN_JSC_ADDITIONS)is off (the#elsebranch hardcodesrejectProxyInPrototypeChain = true). - Checked that the moved prototype-walk loop is byte-for-byte identical to the original inside the new
if— no semantic drift when the flag is unset. - Confirmed the new flag follows the neighboring
m_needsSiteSpecificQuirkspattern (privatem_bool + inline getter/setter) per the JSC CLAUDE.md convention of copying existing patterns.
Extended reasoning...
Overview
This PR adds an opt-in boolean flag m_allowsProxyInPrototypeChain to JSGlobalObject (Source/JavaScriptCore/runtime/JSGlobalObject.h) and consults it in ProgramExecutable::initializeGlobalProperties (Source/JavaScriptCore/runtime/ProgramExecutable.cpp) to skip the loop that throws TypeError: Proxy is not allowed in the global prototype chain. Both additions are guarded by #if USE(BUN_JSC_ADDITIONS); the non-Bun path sets rejectProxyInPrototypeChain = true and preserves the original behavior verbatim. The motivation is jsdom 28+ under node:vm, which installs a Proxy (WindowProperties) in the window's prototype chain.
Security risks
The original check (WebKit bug 165205) was added as a security/correctness invariant to prevent Proxy traps from interfering with global scope resolution. This change makes that check bypassable, but only when an embedder explicitly opts a specific JSGlobalObject in via setAllowsProxyInPrototypeChain(true) — the default remains false, and non-Bun builds cannot reach the opt-out at all. The PR argues global declaration instantiation only touches own properties of the global object, so no Proxy trap fires during program linking; runtime name resolution already goes through ordinary [[Get]]/[[Set]]/[[HasProperty]] which handle Proxies. That reasoning is plausible, but whether any JIT/IC path (e.g., global property resolution watchpoints, GlobalVar/GlobalLexicalVar resolve types, or scope-chain assumptions in DFG/FTL) independently assumes "no Proxy in the global's prototype chain" is a JSC-domain question I can't fully verify from this diff alone.
Level of scrutiny
Medium-high. The diff itself is tiny, mechanical, and follows the established USE(BUN_JSC_ADDITIONS) + neighboring-bool-flag pattern from CLAUDE.md. However, it touches execution semantics at the global-object boundary, an area historically sensitive enough that upstream WebKit added an explicit rejection. .github/CODEOWNERS assigns /Source/JavaScriptCore to WebKit/jsc-reviewers, which per the approval guidelines means a human owner should review rather than auto-approve.
Other factors
The change is default-off and additive, so blast radius is limited to embedders that call the new setter (the matching Bun-side PR). No tests are added here; the PR description reports manual verification against a Bun debug build. The flag placement between m_canDoASCIIUCADUCETLocaleCompare and m_globalLexicalBindingEpoch keeps the bool cluster together and doesn't perturb non-Bun object layout (it's #if-guarded). Given CODEOWNERS coverage and the security-adjacent subject matter, deferring is the appropriate call even though I found no defects in the diff.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughThe change adds Bun-specific state and accessors for allowing proxies in a global object's prototype chain. Global prototype validation uses this setting in Bun builds and retains unconditional rejection in other builds. ChangesProxy Prototype Chain Handling
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The opt-in remains disabled by default and the existing rejection behavior is retained outside Bun builds. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
Preview Builds
|
1 participant
Problem
vm.runInContext("1 + 1", ctx)throwsTypeError: Proxy is not allowed in the global prototype chain.when the global of anode:vmcontext has a Proxy anywhere in its prototype chain. jsdom 28+ puts one there (WindowProperties), so jsdom undernode:vmand VitestvmThreadsfail. Node prints2. node:vm rejects a Proxy in a DONT_CONTEXTIFY global prototype chain, breaking jsdom with Vitest vmThreads bun#42331.Object.setPrototypeOf(globalThis, Object.create(new Proxy({}, {}))),vm.runInThisContext()and CommonJSrequire()throw the same error. Node runs both.ProgramExecutable::initializeGlobalProperties(runtime/ProgramExecutable.cpp:120). It runs before every program links.Fix
JSGlobalObject::setAllowsProxyInPrototypeChain()underUSE(BUN_JSC_ADDITIONS). Default false. When a global object opts in, the check is skipped. Nothing else changes. Bun opts in fromBun::GlobalScope::finishCreation, the base of every Bun global (main, workers, shadow realms,node:vmcontexts).hasRestrictedGlobalProperty,canDeclareGlobalVar,canDeclareGlobalFunction,createGlobalVarBinding, andabstractAccessinJSScope.cpp), so no Proxy trap runs while a program links. Run time name resolution walks the chain through the ordinary[[HasProperty]],[[Get]]and[[Set]]paths, which handle a Proxy. That is also what V8 does.varand function globals, reads and writes through the Proxy, and throwsReferenceErrorfor a missing name, inDONT_CONTEXTIFYcontexts, contextified contexts, and the main realm (runInThisContext,require). The wholetest/js/node/vmsuite and the 95 vendoredtest-vm-*.jspass. The Bun side is the matching PR in oven-sh/bun.Background
Window.prototypeis mutable andvardeclaration usedhasPropertyat the time.IsImmutablePrototypeExoticObjectfrom its structure (ZigGlobalObject.cpp:443,NodeVM.cpp:941), in part so that jsdom can set the prototype of the window. That is how a Proxy reaches a global's chain in Bun, and V8 permits it for Node's globals.