chore: scene runtime V8 engine configuration#9432
Conversation
Set AllowReflection to false on the scene V8 engine and add regression coverage in V8Tests. Host member access continues to resolve through UseReflectionBindFallback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
dalkia
changed the title
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review — PR #9432: chore: tighten scene runtime V8 engine configuration
STEP 2 — Root-cause check: PASS
The PR correctly addresses the root cause. AllowReflection = true was the direct configuration that allowed scene scripts to call System.Reflection members (e.g. GetType(), GetMethod()) on host objects, enabling a sandbox escape vector. Setting it to false is the correct and minimal fix. This is not a symptom workaround — it directly closes the reflection surface.
STEP 3 — Design & integration: PASS
- No new units introduced — no new systems, plugins, managers, or helpers.
V8EngineFactory.Create()is the single factory method that creates all scene V8 engines (confirmed via codebase search: consumed bySceneRuntimeFactory,SceneFacadetests,SceneFactorytests, andSceneRuntimetests). This is the correct and only place for this configuration change.- Tests are added to the existing
V8Testsclass — no new test fixtures or files. AllowReflection = falsesits alongsideDisableDynamicBinding = trueandUseReflectionBindFallback = true— the factory already centralizes all engine security/binding settings.- No lifecycle or teardown concerns — this is a property assignment on engine creation.
Owner search: V8EngineFactory is the owner of engine creation. No other code path creates V8ScriptEngine instances. Configuration belongs here.
STEP 4 — Member audit: PASS
No new public properties, accessors, or methods introduced. The only production change is a property assignment value (true → false).
STEP 5 — Line-level findings
Two P2 findings — see inline comments with suggestions.
| # | Sev | File | Line | Issue |
|---|---|---|---|---|
| 1 | P2 | V8Tests.cs | 87 | ReflectionProbe shadows UnityEngine.ReflectionProbe |
| 2 | P2 | V8Tests.cs | 74 | Comment describes external mechanism instead of what the test verifies |
Security review
No security issues found. This change is itself a security hardening:
- Setting
AllowReflection = falsecorrectly prevents scene scripts from callingSystem.Reflectionmembers on host objects (GetType(),GetMethod(), etc.). UseReflectionBindFallback = trueremains set — this is an internal ClearScript mechanism for IL2CPP compatibility (resolves host member bindings via reflection internally), and does not expose reflection APIs to scripts.- No
AddHostTypecalls exist in production scene runtime code — onlyAddHostObjectis used (viaJsApiBunchandSceneRuntimeImpl), exposing specificJsApiWrapperinstances rather than entire types. - No
HostItemFlagsusage found in the codebase. - The sandbox surface is appropriately narrowed.
STEP 6 — Complexity: SIMPLE
2 files changed, ~33 lines total. A single config flag change plus regression tests. No architectural changes.
STEP 7 — QA: YES
Modifies runtime code (V8EngineFactory) that affects every scene engine instance. Could break scenes if AllowReflection/UseReflectionBindFallback interaction is unexpected. The PR description correctly requests IL2CPP standalone testing.
STEP 8 — Non-blocking warnings
None.
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Config flag change in V8 engine factory with regression tests — no architectural or subsystem changes.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Warnings not reduced: 14668 => 26622 — remove at least one warning to merge. |
There was a problem hiding this comment.
✅Smoke test performed on windows and Mac:
- ✔️ Log In/Log Out (New OTP aacount, Metamask and Google)
- ✔️ Backpack and wearables in world
- ✔️ Emotes in world and in backpack
- ✔️ Teleport with map/coordinates/Jump In
lautarox.dcl.eth- Bloomgarden.dcl.eth
- kickoff.dcl.eth
- cleantheclub.dcl.eth
- ✔️ Chat and multiplayer
- ✔️ Profile card
- ✔️ Camera
- ✔️ Skybox
- ✔️ Add friend
- accept friend request
- Remove friend
- ✔️ Join a community
- ✔️ Daily quest completetion + reward
- ✔️ Claim rewards
- ✔️ Settings
|
💰 Review cost: unknown · |
5 participants
Description
Adjusts the V8 script engine configuration used to run scenes and adds regression coverage for the engine's configuration and host-member binding behavior.
Technical
AllowReflection = falseon the scene V8 engine (V8EngineFactory).UseReflectionBindFallback, unchanged.V8Testscases asserting the engine configuration and that normal host member access still works.