feat(loader): support constructed values and nested calls#3067
feat(loader): support constructed values and nested calls#3067luzhuang wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
WalkthroughReflection parsing now supports nested method targets, ordered property and call execution, recursive ChangesReflection parser updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SceneFormatV2Test
participant ReflectionParser
participant CallOrderComponent
SceneFormatV2Test->>ReflectionParser: parse mutation block
ReflectionParser->>CallOrderComponent: apply resolved props
ReflectionParser->>CallOrderComponent: invoke nested target method
CallOrderComponent-->>SceneFormatV2Test: expose updated state
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: private package registry requires authentication. Disable ESLint in CodeRabbit settings or use public packages. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3067 +/- ##
===========================================
+ Coverage 79.61% 79.79% +0.18%
===========================================
Files 904 904
Lines 101208 101313 +105
Branches 11404 11414 +10
===========================================
+ Hits 80579 80847 +268
+ Misses 20445 20284 -161
+ Partials 184 182 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts`:
- Around line 165-167: Move the standalone "$args" validation immediately after
obj is created in ReflectionParser, before branches handling "$ref", "$class",
"$entity", "$component", or "$signal", so any object containing "$args" without
"$type" is rejected. Add a test covering a mixed discriminator such as "$class"
with "$args" and assert it fails.
- Around line 43-57: Update the call traversal and invocation logic in
ReflectionParser to reject the property names "constructor", "prototype", and
"__proto__" in every call.target segment and in call.method before accessing or
invoking them. Return a rejected Promise using the existing validation-error
pattern, while preserving valid nested method resolution and invocation.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d5ce96ab-e890-4807-80ee-e87475a9bfd1
📒 Files selected for processing (3)
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.tspackages/loader/src/schema/CommonSchema.tstests/src/loader/SceneFormatV2.test.ts
| let target = instance; | ||
| if (call.target !== undefined) { | ||
| if (!Array.isArray(call.target) || call.target.some((key) => typeof key !== "string" || key.length === 0)) { | ||
| return Promise.reject(new Error(`Call "${call.method}" target must be an array of non-empty strings`)); | ||
| } | ||
| for (const key of call.target) target = target?.[key]; | ||
| } | ||
| const method = target?.[call.method]; | ||
| if (typeof method !== "function") { | ||
| return Promise.reject(new Error(`Call target does not have method "${call.method}"`)); | ||
| const path = call.target?.length ? `${call.target.join(".")}.` : ""; | ||
| return Promise.reject(new Error(`Call target does not have method "${path}${call.method}"`)); | ||
| } | ||
|
|
||
| return Promise.all((call.args ?? []).map((arg) => this._resolveValue(arg))) | ||
| .then((resolvedArgs) => Promise.resolve(method.apply(instance, resolvedArgs))) | ||
| .then((resolvedArgs) => Promise.resolve(method.apply(target, resolvedArgs))) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Block prototype and constructor traversal before invoking nested calls.
Loader-controlled paths can traverse function properties and reach Function, for example target: ["nested", "setValue"] with method: "constructor". Combined with call.result, this permits constructing and invoking arbitrary JavaScript.
Reject constructor, prototype, and __proto__ in both target segments and method names.
Proposed guard
+ const forbiddenKeys = new Set(["constructor", "prototype", "__proto__"]);
let target = instance;
if (call.target !== undefined) {
if (!Array.isArray(call.target) || call.target.some((key) => typeof key !== "string" || key.length === 0)) {
return Promise.reject(new Error(`Call "${call.method}" target must be an array of non-empty strings`));
}
+ if (call.target.some((key) => forbiddenKeys.has(key)) || forbiddenKeys.has(call.method)) {
+ return Promise.reject(new Error(`Call "${call.method}" contains a forbidden target key`));
+ }
for (const key of call.target) target = target?.[key];
}🤖 Prompt for 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.
In
`@packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts`
around lines 43 - 57, Update the call traversal and invocation logic in
ReflectionParser to reject the property names "constructor", "prototype", and
"__proto__" in every call.target segment and in call.method before accessing or
invoking them. Return a rejected Promise using the existing validation-error
pattern, while preserving valid nested method resolution and invocation.
| if ("$args" in obj) { | ||
| return Promise.reject(new Error("$args requires $type")); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate standalone $args before the other discriminator branches.
This check is unreachable for values containing $ref, $class, $entity, $component, or $signal, so malformed values such as { $class: "Foo", $args: [] } silently ignore $args. Move the check immediately after creating obj and add a mixed-discriminator test.
🤖 Prompt for 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.
In
`@packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts`
around lines 165 - 167, Move the standalone "$args" validation immediately after
obj is created in ReflectionParser, before branches handling "$ref", "$class",
"$entity", "$component", or "$signal", so any object containing "$args" without
"$type" is rejected. Add a test covering a mixed discriminator such as "$class"
with "$args" and assert it fails.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
审查 @ ceba5a347（首轮）— feat(loader): support constructed values and nested calls
总结
给 v2 场景 ReflectionParser 补三块运行时反序列化能力：① $type 支持递归解析的 $args 构造参数；② call.target 支持把方法调用打到嵌套属性路径的 owner 上；③ $number: "Infinity" 的 JSON-safe 正无穷哨兵；④ 把 parseMutationBlock 从「props/calls 并发无序」改成「props 先、calls 后」的确定性顺序。方向完全正确，是 #3061 那版错 base 的 d59c6392d 收敛到 dev/2.0 纯 Loader 改动后的干净版本。逐链核对无 P0/P1/P2，可合并。
逐链核对（非信 commit message / PR 描述）
- props-before-calls 顺序是收紧而非回退：原
Promise.all([props, calls])的 JSDoc 明确写「without imposing ordering」（来自 #2959284cce8e1），但那只是描述行为、未给出「必须无序」的理由。改成 props→calls 是确定性收紧：老行为并发交错，没有任何合法数据能依赖「calls 先于 props」，所以本改动严格更安全，且符合语义直觉（props 配置字段状态，calls 在其上动作，如addBurst需先配好emission）。走 Git 历史透镜确认无矛盾——不是还原任何fix:。 - 顺序测试反向可证伪：
should finish resolving props before executing calls构造精巧——$ref被 mock 成延迟 promise，await Promise.resolve()放过一个微任务后才 resolve。新序下 call 捕获"ready"；revert 回Promise.all则 call 在 ref 解析前就跑、捕获""，测试 fail。真守回归。 - particle 恢复测试是端到端真链路：
Burst($args:[0.5, curve, 2, 0.1])/ParticleCurve($args:[CurveKey,CurveKey])/CurveKey($args:[0,1])/ParticleCompositeCurve($args:[8])的构造签名逐一核对全部对齐真实构造函数，且经call.target:["generator","emission"] + addBurst/clearBurst走公开 API。这正是本特性的动机——editor 需要 round-trip 需构造参数 + 经方法（非纯 props）设置的粒子曲线/burst。reverse-falsifiable。 call.target绑定正确：method.apply(target, ...)打到遍历后的 owner；call.target === undefined时target = instance、target?.[call.method]退化为原instance?.[call.method]，行为等价。错误消息 path 拼接在 target 校验之后，安全。- schema 新增类型非死代码：
TypeValue/SpecialNumberValue在CommonSchema.ts无 parser 消费者，但这与既有ClassRef（同样零消费者）/Vec3Tuple/EntityRef完全一致——该文件的职责就是声明 v2 格式的规范词汇表，不是必须被 import 才算活。按项目惯例非冗余。 - 注释合规：更新后的
_resolveValue优先级 JSDoc 已同步补入$number(#4)/$type,$args?(#5) 重编号，与实际分支顺序一致；parseMutationBlockJSDoc 由「without imposing ordering」改为「Apply props before executing calls」——非 aspirational，与代码一致。CI 全绿（build×3 / codecov patch 88.37% + project 79.61% / prettier）。
已评估的 CodeRabbit 两条 finding（均非阻塞，据实校准严重度）
-
CR「Critical: prototype/constructor 遍历可达 RCE」→ 实为 P3 defensive、非新洞。核心事实：
call.target:["fn"], method:"constructor"能拿到Function进而call.result链式 RCE——但这条可达性早于本 PR：parseCalls从 #2959 起就有instance[call.method]+call.result → parseMutationBlock递归，{method:"constructor", result:{calls:[{method:"constructor", result:{calls:[{method:"call", args:[...]}]}}]}}早已可达Function并执行任意代码。本 PR 的target遍历只是便利路径，未引入新能力类。且 v2 场景数据是 editor 产出（与 glTF/scene 资产同信任级、等同 app 代码），非系统边界的不可信输入。CR 的 Critical 定级在此信任模型下过高。鉴于本 PR 恰好动到这段遍历代码，加一个constructor/prototype/__proto__黑名单守卫是合理的 defense-in-depth，但属可选加固，不阻塞。const forbidden = new Set(["constructor", "prototype", "__proto__"]); if (call.target?.some((k) => forbidden.has(k)) || forbidden.has(call.method)) { return Promise.reject(new Error(`Call "${call.method}" contains a forbidden key`)); }
问题
-
[P3] ReflectionParser.ts:165-167 — 独立
$args校验位置偏后（CR Minor，据实保留为 P3）。if ("$args" in obj) reject排在$ref/$class/$entity/$component/$signal分支之后，故{ $class:"Foo", $args:[] }会先命中$class分支、静默丢弃$args。editor 不会产出这种混合 discriminator，属畸形输入容错的小缺口。若要收紧，把该校验挪到obj创建后、所有 discriminator 分支之前，并补一条$class+$args的混合用例。不阻塞。 -
[P3] ReflectionParser.ts —
$number仅覆盖正无穷。当前只支持$number:"Infinity"（对应 Burst 无限循环 cycles），显式 rejectNaN。-Infinity暂无字段需要故未支持——YAGNI 合理。仅记录：若将来出现需持久化-Infinity的字段（某些下界），此哨兵需扩展。不阻塞。
简化建议
无。代码干净，抽象克制（$args 复用 _resolveValue 递归、call.target 与既有 parseCalls 无缝衔接），无过度设计。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
审查 @ 84b614111（第二轮，增量）— fix(loader): decode infinite burst cycles
总结
自上轮 ceba5a347 起线性追加一个 commit（parent 就是 ceba5a347，非 force-push）。改动只做一件事：删掉上轮刚引入的通用 $number: "Infinity" 哨兵，改成在 _resolveValue 的 $type 构造分支里对 $type === "Burst" 且 args[2] 做位置化、字符串化的特判——"Infinity" → Infinity，其余非 null 的非正整数 → reject。逻辑正确、反向证伪测试到位、CI 全绿（build×3 / codecov patch+project / e2e 4/4 / lint），可合并。但方向上把一个通用能力换成了单类硬编码特判，值得一议——记 [P2]，不阻塞。
逐链核对（增量）
Burst.$args[2]确是cycles：Burst构造签名(time, count, cycles?, repeatInterval?)（Burst.ts:51），args[2]位置对齐。cycles === Infinity是EmissionModule.ts:312的真运行时哨兵（maxCycles = cycles===Infinity ? ceil((duration-burstTime)/repeatInterval) : cycles），JSON 无法表达Infinity（JSON.stringify(Infinity)==="null"）故需 sentinel——需求真实。- 守卫逻辑穷举无洞：
"Infinity"→Infinity✓ /2→过 ✓ /"NaN"→非"Infinity"非null且非number→throw ✓（测试覆盖）/null|undefined→!=null短路跳过→构造函数Math.max(cycles??1,1)=1✓ /1.5/0/-Infinity→非正整数→throw ✓。 - 反向证伪成立：测试改用
"Infinity"串断言bursts[0].cycles === Infinity，并新增"NaN"用例断言rejects.toThrow(...)；revert 特判即红。 - Git 历史透镜无矛盾：
$number是本 PR 上一 commit（ceba5a347）自己引入的，全仓无第二处引用、从未发版/被消费——删它不是还原任何fix:，是作者重做自己刚加的机制。 - 注释合规：
_resolveValue优先级 JSDoc 从 10 条重编号回 9 条（去掉$number），与实际分支顺序（$ref3/$type4/$class5/$entity6/$component7/$signal8/plain 9）逐条一致，无残留。SpecialNumberValueinterface 同步删除。
问题
-
[P2] ReflectionParser.ts:130-137 — 通用
Infinity编码退化成Burst单类位置特判，是「类型枚举代替特征」+ 抽象泄漏。上轮的$number: "Infinity"虽然只覆盖正无穷（我上轮记为 P3-YAGNI），但它是格式层的通用 JSON-safe 数值编码——任何 prop / ctor arg 都能用。本轮把它换成if ($type === "Burst" && args.length > 2)对args[2]的字符串特判，埋进了本该类型无关的_resolveValue分发器里，带来两点结构代价:Infinity本质是通用序列化问题，不是 Burst 问题。同仓Joint.breakForce/breakTorque默认就是Infinity（Joint.ts:27-28，对应 Unity「不可断裂关节」惯例）且是 public 可序列化属性（Joint.ts:157/171）；ResourceManager.timeout、request.defaultTimeout同样默认Infinity。这些字段将来要经 v2 round-trip 时，通用$number本可覆盖，现在每个都得往 parser 里再加一条特判。新类型要 infinity → 必须改 parser——正是「加白名单，漏补即 bug」的信号。- 泛型分发器嵌入了单个引擎类的构造参数语义（硬编码「
Burst的第 3 个位置参是cycles」）。Burst构造顺序一旦调整，这里静默失效，且无编译期护栏。
另外，
"Infinity"这个魔法串契约在 schema 里零文档——TypeValue.$args?: unknown[]完全没提Burst的第三参接受字符串"Infinity"，producer（editor companion PR #3793）与 reader 全靠隐式约定。格式契约与 editor 共有、且处于 alpha 无兼容包袱——正是选对编码的最佳时机。建议保留通用 sentinel 方向（哪怕仍只实现正无穷），让「JSON-safe 特殊数值」成为格式层一等公民、与具体类解耦；Burst 侧只需在
$args里放该 sentinel 即可，parser 不必认识Burst。若坚持只服务 Burst，也至少把"Infinity"串契约写进CommonSchema.ts的TypeValue/ Burst 相关注释，别让它只活在 parser 实现里。不阻塞。
已在上轮闭环、本轮不重提
- CodeRabbit 在本 commit 重贴的两条（
constructor/prototype遍历可达 RCE = Critical；独立$args校验位置偏后 = Minor）我上轮均已评估：前者是早于本 PR 的既有可达性 + editor 信任模型下的可选加固（本轮 traversal 代码未变），后者我上轮已记为 P3。二者状态未变，不重复。
简化建议
若采纳上面 P2 的「恢复通用 sentinel」方向，_resolveValue 里那段 8 行 Burst 特判可整段删除，换成一条与类无关的 $number（或等价）分支，净复杂度更低且覆盖面更广——这本身就是更简的解。
2 participants
Summary
$typevaluesBoundary
This PR changes Loader only. It does not add particle collection setters or register the entire Math package. Editor source-v2 compilation is handled by companion PR galacean/editor#3793.
Verification
vitest run tests/src/loader/SceneFormatV2.test.ts -t ReflectionParser: 33 passedgit diff --check: passedThe full Loader type build is currently blocked by stale 1.4.5 workspace dependencies in this worktree; none of its diagnostics point to the touched Loader files.
Summary by CodeRabbit
New Features
$args) for polymorphic scene/resource loading.Bug Fixes