From 7bb21c2aa266f4867e43949c02dc9c241880c8d9 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 06:08:08 -0400 Subject: [PATCH] fix(scripting): resolve High code-review finding (Core.Scripting-002) 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) --- code-reviews/Core.Scripting/findings.md | 14 +- .../ForbiddenTypeAnalyzer.cs | 75 ++++++++- .../ScriptSandboxTests.cs | 146 ++++++++++++++++++ 3 files changed, 230 insertions(+), 5 deletions(-) diff --git a/code-reviews/Core.Scripting/findings.md b/code-reviews/Core.Scripting/findings.md index adad00d..68772e7 100644 --- a/code-reviews/Core.Scripting/findings.md +++ b/code-reviews/Core.Scripting/findings.md @@ -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`), 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 diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/ForbiddenTypeAnalyzer.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/ForbiddenTypeAnalyzer.cs index edf08fd..77f8a1f 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/ForbiddenTypeAnalyzer.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/ForbiddenTypeAnalyzer.cs @@ -100,6 +100,33 @@ public static class ForbiddenTypeAnalyzer /// Returns empty list when the script is clean; non-empty list means the script /// must be rejected at publish with the rejections surfaced to the operator. /// + /// + /// + /// The walker has two passes per node. Pass (1) is the member / call surface: + /// ObjectCreationExpressionSyntax, InvocationExpressionSyntax with + /// a member-access target, MemberAccessExpressionSyntax, and bare + /// IdentifierNameSyntax are resolved via + /// .GetSymbolInfo. This catches static calls + /// (System.IO.File.ReadAllText) and constructors, and is deliberately + /// narrow: resolving GetSymbolInfo on every node would flag + /// harmless inherited members (e.g. typeof(int).Name resolves + /// Name to System.Reflection.MemberInfo, the base type that + /// declares it, even though the receiver type System.Type is allowed). + /// + /// + /// Pass (2) — the Core.Scripting-002 fix — resolves the type of every + /// TypeSyntax node via GetTypeInfo. The old walker only inspected + /// the four node kinds above, so a forbidden type named through + /// typeof(System.IO.File), a generic argument + /// (List<System.IO.FileInfo>), a cast + /// ((System.IO.Stream)null), an is / as type pattern, + /// default(System.Reflection.Assembly), an array-creation element type, + /// or an explicitly-typed local declaration produced no examined node and so + /// slipped through. Every TypeSyntax resolves to a concrete + /// ; generic type arguments and array element types + /// are unwrapped recursively so a forbidden type nested at any depth is caught. + /// + /// public static IReadOnlyList Analyze(Compilation compilation) { if (compilation is null) throw new ArgumentNullException(nameof(compilation)); @@ -111,6 +138,9 @@ public static class ForbiddenTypeAnalyzer var root = tree.GetRoot(); foreach (var node in root.DescendantNodes()) { + // Pass (1) — member / call surface. Narrowly targeted at the node kinds + // that name a callable member or constructor, so inherited-member + // resolution does not produce false positives. switch (node) { case ObjectCreationExpressionSyntax obj: @@ -130,11 +160,43 @@ public static class ForbiddenTypeAnalyzer CheckSymbol(semantic.GetSymbolInfo(id).Symbol, id.Span, rejections); break; } + + // Pass (2) — type-reference surface (Core.Scripting-002). Every TypeSyntax + // resolves to the type it names, regardless of the syntactic form that + // introduced it (typeof operand, cast type, generic argument, default(T) + // operand, array element type, is/as pattern type, declared local type). + // Type arguments and array element types are walked recursively. + if (node is TypeSyntax) + CheckTypeSymbol(semantic.GetTypeInfo(node).Type, node.Span, rejections); } } return rejections; } + /// + /// Reject if it (or, recursively, any of its generic type + /// arguments / array element types) is forbidden. Walks the full type tree so a + /// forbidden type nested inside an allowed generic — e.g. + /// List<System.IO.FileInfo> — is still caught. + /// + private static void CheckTypeSymbol(ITypeSymbol? type, TextSpan span, List rejections) + { + if (type is null) return; + + CheckSymbol(type, span, rejections); + + switch (type) + { + case IArrayTypeSymbol array: + CheckTypeSymbol(array.ElementType, span, rejections); + break; + case INamedTypeSymbol named: + foreach (var arg in named.TypeArguments) + CheckTypeSymbol(arg, span, rejections); + break; + } + } + private static void CheckSymbol(ISymbol? symbol, TextSpan span, List rejections) { if (symbol is null) return; @@ -149,6 +211,15 @@ public static class ForbiddenTypeAnalyzer }; if (typeSymbol is null) return; + var typeName = typeSymbol.ToDisplayString(); + + // The broadened walk (Core.Scripting-002) resolves both GetSymbolInfo and + // GetTypeInfo on every node, so the same forbidden reference can be hit several + // times. Dedupe on span + type so the operator sees one rejection per offending + // reference, not a noisy pile of identical messages. + if (rejections.Any(r => r.Span == span && r.TypeName == typeName)) + return; + var ns = typeSymbol.ContainingNamespace?.ToDisplayString() ?? string.Empty; foreach (var forbidden in ForbiddenNamespacePrefixes) { @@ -156,9 +227,9 @@ public static class ForbiddenTypeAnalyzer { rejections.Add(new ForbiddenTypeRejection( Span: span, - TypeName: typeSymbol.ToDisplayString(), + TypeName: typeName, Namespace: ns, - Message: $"Type '{typeSymbol.ToDisplayString()}' is in the forbidden namespace '{ns}'. " + + Message: $"Type '{typeName}' is in the forbidden namespace '{ns}'. " + $"Scripts cannot reach {forbidden}* per Phase 7 sandbox rules.")); return; } diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptSandboxTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptSandboxTests.cs index aa8d69f..0f0a2c0 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptSandboxTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptSandboxTests.cs @@ -171,6 +171,152 @@ public sealed class ScriptSandboxTests """return System.Environment.GetEnvironmentVariable("PATH");""")); } + // --- Core.Scripting-002: type-reference node forms that bypassed the old walker --- + // The old walker only inspected ObjectCreation / Invocation-with-member-access / + // MemberAccess / bare-Identifier nodes. typeof, generic type arguments, casts, + // is/as patterns, default(T), array element types, and declared variable types all + // name a forbidden type without producing any of those nodes. The broadened walker + // resolves GetTypeInfo on every TypeSyntax / ExpressionSyntax so they are all caught. + + [Fact] + public void Rejects_typeof_forbidden_type_at_compile() + { + // Phase 7 plan A.6 explicitly calls out typeof as a sandbox-escape that must + // fail at compile. (Core.Scripting-002.) + Should.Throw(() => + ScriptEvaluator.Compile( + """return typeof(System.IO.File).Name;""")); + } + + [Fact] + public void Rejects_generic_type_argument_forbidden_type_at_compile() + { + // new List() — the forbidden type is nested inside an + // allowed generic. The walker unwraps type arguments recursively. (Core.Scripting-002.) + Should.Throw(() => + ScriptEvaluator.Compile( + """ + var l = new System.Collections.Generic.List(); + return l.Count; + """)); + } + + [Fact] + public void Rejects_cast_to_forbidden_type_at_compile() + { + // (System.IO.Stream)null — a cast expression names the type without invoking + // or constructing it. (Core.Scripting-002.) + Should.Throw(() => + ScriptEvaluator.Compile( + """ + var s = (System.IO.Stream)null; + return 0; + """)); + } + + [Fact] + public void Rejects_default_of_forbidden_type_at_compile() + { + // default(System.Reflection.Assembly) names a forbidden type via the default + // operator. (Core.Scripting-002.) + Should.Throw(() => + ScriptEvaluator.Compile( + """ + var a = default(System.Reflection.Assembly); + return 0; + """)); + } + + [Fact] + public void Rejects_is_pattern_forbidden_type_at_compile() + { + // 'o is System.IO.FileStream' names a forbidden type as the pattern's type + // operand. (Core.Scripting-002.) + Should.Throw(() => + ScriptEvaluator.Compile( + """ + object o = ctx.GetTag("X").Value; + return o is System.IO.FileStream; + """)); + } + + [Fact] + public void Rejects_as_expression_forbidden_type_at_compile() + { + // 'o as System.IO.Stream' names a forbidden type via the as operator. (Core.Scripting-002.) + Should.Throw(() => + ScriptEvaluator.Compile( + """ + object o = ctx.GetTag("X").Value; + var s = o as System.IO.Stream; + return 0; + """)); + } + + [Fact] + public void Rejects_array_creation_forbidden_element_type_at_compile() + { + // new System.IO.FileInfo[0] — the forbidden type is the array element type. + // (Core.Scripting-002.) + Should.Throw(() => + ScriptEvaluator.Compile( + """ + var a = new System.IO.FileInfo[0]; + return a.Length; + """)); + } + + [Fact] + public void Rejects_local_declared_variable_forbidden_type_at_compile() + { + // An explicitly-typed local declaration 'System.Net.Http.HttpClient c;' names + // a forbidden type with no construction or call. (Core.Scripting-002.) + Should.Throw(() => + ScriptEvaluator.Compile( + """ + System.Net.Http.HttpClient c = null; + return 0; + """)); + } + + [Fact] + public void Rejects_typeof_forbidden_type_inside_Activator_at_compile() + { + // Activator is already denied by ForbiddenFullTypeNames, but the typeof operand + // is independently a forbidden-type reference — confirm the typeof path catches + // it even when the Activator type itself were allowed. (Core.Scripting-002.) + Should.Throw(() => + ScriptEvaluator.Compile( + """ + var t = typeof(System.Diagnostics.Process); + return 0; + """)); + } + + [Fact] + public async Task Allowed_generic_type_argument_still_compiles() + { + // Guard against over-blocking: a generic with only allow-listed type arguments + // (List) must still compile and run. (Core.Scripting-002.) + var evaluator = ScriptEvaluator.Compile( + """ + var l = new System.Collections.Generic.List { 1, 2, 3 }; + return l.Count; + """); + var result = await evaluator.RunAsync(new FakeScriptContext(), TestContext.Current.CancellationToken); + result.ShouldBe(3); + } + + [Fact] + public async Task Allowed_typeof_still_compiles() + { + // typeof against an allow-listed type (int) must not be rejected. (Core.Scripting-002.) + var evaluator = ScriptEvaluator.Compile( + """return typeof(int).Name;"""); + var result = await evaluator.RunAsync(new FakeScriptContext(), TestContext.Current.CancellationToken); + result.ShouldBe("Int32"); + } + [Fact] public async Task Script_exception_propagates_unwrapped() {