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() {