dohertj2/lmxopcua
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(scripting): resolve High code-review finding (Core.Scripting-002)

Browse Source 
The ForbiddenTypeAnalyzer syntax walker only inspected four node kinds
(ObjectCreation, Invocation-with-member-access, MemberAccess, bare
Identifier), so a forbidden type named through typeof, a generic type
argument, a cast, an is/as type pattern, default(T), an array-creation
element type, or an explicitly-typed local declaration produced no
examined node and bypassed the sandbox check.

Analyze now runs a second pass that resolves GetTypeInfo on every
TypeSyntax node and recursively unwraps array element types and generic
type arguments, so forbidden types nested at any depth are rejected at
compile. The original member/call node-kind switch is kept deliberately
narrow (rather than resolving GetSymbolInfo on every node) to avoid
flagging harmless inherited members such as typeof(int).Name, whose Name
property is declared by System.Reflection.MemberInfo. A span+type dedupe
keeps the two passes from emitting duplicate rejections.

Regression tests added in ScriptSandboxTests cover typeof, generic type
arguments, casts, default(T), is/as patterns, array element types, and
typed local declarations with forbidden types, plus over-block guards
asserting allowed generics and typeof still compile.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-22 06:08:08 -04:00
parent 8c7c605478
commit 7bb21c2aa2
3 changed files with 230 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
code-reviews/Core.Scripting/findings.md
View File

@@ -7,7 +7,7 @@
| Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` |
| Status | Reviewed |
| Open findings | 10 |
| Open findings | 9 |
## Checklist coverage
@@ -73,7 +73,7 @@ types (Math, String, …) stay usable. Regression tests added in `ScriptSandboxT
| Severity | High |
| Category | Security |
| Location | `ForbiddenTypeAnalyzer.cs:70` |
| Status | Open |
| Status | Resolved |
**Description:** The syntax walker only inspects four node kinds:
`ObjectCreationExpressionSyntax`, `InvocationExpressionSyntax` with a member-access target,
@@ -95,7 +95,15 @@ that "must fail at compile" — it currently does not.
then check the resolved type plus every type argument. Add tests covering `typeof`,
generic arguments, casts, and `default(T)` with forbidden types.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-22 — `ForbiddenTypeAnalyzer.Analyze` now runs a second
pass that resolves `GetTypeInfo` on every `TypeSyntax` node and recursively unwraps array
element types and generic type arguments, so forbidden types named via `typeof`, generic
arguments (`List<FileInfo>`), casts, `is`/`as` patterns, `default(T)`, array-creation
element types, and explicitly-typed local declarations are all rejected at compile. The
original member/call node-kind switch is kept (deliberately narrow to avoid flagging
inherited members such as `typeof(int).Name`), and a span+type dedupe prevents duplicate
rejections from the two passes. Regression tests added in `ScriptSandboxTests` for each
node form plus over-block guards for allowed generics/`typeof`.
### Core.Scripting-003