diff --git a/code-reviews/Core.Scripting/findings.md b/code-reviews/Core.Scripting/findings.md
index 3d88496..adad00d 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 | 11 |
+| Open findings | 10 |
## Checklist coverage
@@ -36,7 +36,7 @@ a category produced nothing rather than leaving it blank.
| Severity | Critical |
| Category | Security |
| Location | `ForbiddenTypeAnalyzer.cs:45`, `ScriptSandbox.cs:54` |
-| Status | Open |
+| Status | Resolved |
**Description:** `System.Environment` lives in the allowed `System` namespace (it is in
`System.Private.CoreLib`, which is allow-listed for primitives) and is not on the
@@ -57,7 +57,14 @@ for types in allowed namespaces. Add an explicit forbidden-member deny-list to
`GC.AddMemoryPressure`), and `System.Activator.CreateInstance` (a reflection-equivalent
escape). Reject these in `CheckSymbol` by resolved method symbol, with a test for each.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-22 — added a type-granular `ForbiddenFullTypeNames`
+deny-list (`System.Environment`, `System.AppDomain`, `System.GC`, `System.Activator`) to
+`ForbiddenTypeAnalyzer`; `CheckSymbol` now rejects any resolved type symbol whose
+fully-qualified name matches, alongside the existing namespace-prefix check, so dangerous
+`System`-namespace process-control types are blocked at compile while legitimate `System`
+types (Math, String, …) stay usable. Regression tests added in `ScriptSandboxTests` for
+`Environment.Exit` / `Environment.FailFast` / `Environment.GetEnvironmentVariable` /
+`AppDomain` / `GC.Collect` / `Activator.CreateInstance`.
### Core.Scripting-002
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 be7b1fb..edf08fd 100644
--- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/ForbiddenTypeAnalyzer.cs
+++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/ForbiddenTypeAnalyzer.cs
@@ -20,10 +20,7 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Scripting;
/// Deny-list is the authoritative Phase 7 plan decision #6 set:
/// System.IO, System.Net, System.Diagnostics.Process,
/// System.Reflection, System.Threading.Thread,
-/// System.Runtime.InteropServices. System.Environment (for process
-/// env-var read) is explicitly left allowed — it's read-only process state, doesn't
-/// persist outside, and the test file pins this compromise so tightening later is
-/// a deliberate plan decision.
+/// System.Runtime.InteropServices.
///
///
/// Deny-list prefix match. System.Net catches System.Net.Http,
@@ -32,6 +29,21 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Scripting;
/// operator audience authors it through a helper the plan team adds as part of
/// the surface, not by unlocking the namespace.
///
+///
+/// A namespace-prefix deny-list is necessary but not sufficient: dangerous types
+/// such as System.Environment, System.AppDomain, System.GC,
+/// and System.Activator live directly in the System
+/// namespace inside System.Private.CoreLib — the same allow-listed assembly
+/// that supplies primitives (int, string, Math). They cannot
+/// be blocked by namespace because System itself must stay allowed. They
+/// are therefore denied type-granularly via
+/// . Environment.Exit /
+/// Environment.FailFast kill the in-process OPC UA server outright;
+/// Activator.CreateInstance is a reflection-equivalent escape; GC
+/// and AppDomain expose process-wide control. Legitimate System
+/// types (Math, String, Convert, DateTime, …) are not
+/// on the list and stay usable. (Core.Scripting-001.)
+///
///
public static class ForbiddenTypeAnalyzer
{
@@ -53,6 +65,36 @@ public static class ForbiddenTypeAnalyzer
"Microsoft.Win32", // registry
];
+ ///
+ /// Fully-qualified type names scripts are NOT allowed to reference, regardless of
+ /// namespace. These types live directly in the allow-listed System
+ /// namespace (in System.Private.CoreLib), so a namespace-prefix rule cannot
+ /// reach them without also blocking primitives. Matched by exact fully-qualified
+ /// name against the resolved type symbol — every member of the type
+ /// (including read-only ones) is therefore rejected.
+ ///
+ ///
+ ///
+ /// - System.Environment — Exit / FailFast terminate
+ /// the host process; the whole type is denied (the read members have no
+ /// legitimate SCADA-predicate use either).
+ /// - System.AppDomain — process-wide assembly-load /
+ /// unhandled-exception control.
+ /// - System.GC — Collect / AddMemoryPressure perturb
+ /// the process memory subsystem.
+ /// - System.Activator — CreateInstance is a
+ /// reflection-equivalent escape that constructs a forbidden type by name
+ /// without ever naming it syntactically.
+ ///
+ ///
+ public static readonly IReadOnlyList ForbiddenFullTypeNames =
+ [
+ "System.Environment",
+ "System.AppDomain",
+ "System.GC",
+ "System.Activator",
+ ];
+
///
/// Scan the for references to forbidden types.
/// Returns empty list when the script is clean; non-empty list means the script
@@ -121,6 +163,29 @@ public static class ForbiddenTypeAnalyzer
return;
}
}
+
+ // Type-granular deny-list — dangerous types that live in the allow-listed
+ // System namespace and so cannot be caught by ForbiddenNamespacePrefixes
+ // (Core.Scripting-001). Matched on the full type name; OriginalDefinition
+ // unwraps any generic construction before naming.
+ var fullTypeName = typeSymbol.OriginalDefinition.ToDisplayString(
+ SymbolDisplayFormat.FullyQualifiedFormat.WithGlobalNamespaceStyle(
+ SymbolDisplayGlobalNamespaceStyle.Omitted));
+ foreach (var forbiddenType in ForbiddenFullTypeNames)
+ {
+ if (fullTypeName == forbiddenType)
+ {
+ rejections.Add(new ForbiddenTypeRejection(
+ Span: span,
+ TypeName: typeSymbol.ToDisplayString(),
+ Namespace: ns,
+ Message: $"Type '{forbiddenType}' is on the Phase 7 sandbox forbidden-type " +
+ $"deny-list. Scripts cannot reach process-control types " +
+ $"(Environment / AppDomain / GC / Activator) even though they " +
+ $"live in the allowed 'System' namespace."));
+ 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 3ac74f5..aa8d69f 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
@@ -92,23 +92,83 @@ public sealed class ScriptSandboxTests
"""));
}
+ [Fact]
+ public void Rejects_Environment_Exit_at_compile()
+ {
+ // System.Environment lives in System.Private.CoreLib (allow-listed for
+ // primitives) so a namespace-prefix deny-list cannot block it. Environment.Exit
+ // terminates the whole in-process OPC UA server — every connected client and
+ // every driver — so it MUST be rejected member-granularly. (Core.Scripting-001.)
+ Should.Throw(() =>
+ ScriptEvaluator.Compile(
+ """
+ System.Environment.Exit(0);
+ return 0;
+ """));
+ }
+
+ [Fact]
+ public void Rejects_Environment_FailFast_at_compile()
+ {
+ // Environment.FailFast crashes the host process immediately — same outage as
+ // Exit. (Core.Scripting-001.)
+ Should.Throw(() =>
+ ScriptEvaluator.Compile(
+ """
+ System.Environment.FailFast("boom");
+ return 0;
+ """));
+ }
+
+ [Fact]
+ public void Rejects_AppDomain_at_compile()
+ {
+ // AppDomain.CurrentDomain exposes process-wide control (assembly load events,
+ // unhandled-exception hooks). Not script surface. (Core.Scripting-001.)
+ Should.Throw(() =>
+ ScriptEvaluator.Compile(
+ """
+ var n = System.AppDomain.CurrentDomain.FriendlyName;
+ return 0;
+ """));
+ }
+
+ [Fact]
+ public void Rejects_GC_Collect_at_compile()
+ {
+ // GC.Collect / GC.AddMemoryPressure let a script perturb the whole process's
+ // memory subsystem. Not script surface. (Core.Scripting-001.)
+ Should.Throw(() =>
+ ScriptEvaluator.Compile(
+ """
+ System.GC.Collect();
+ return 0;
+ """));
+ }
+
+ [Fact]
+ public void Rejects_Activator_CreateInstance_at_compile()
+ {
+ // Activator.CreateInstance is a reflection-equivalent escape — it can construct
+ // a forbidden type by name without ever naming it syntactically. (Core.Scripting-001.)
+ Should.Throw(() =>
+ ScriptEvaluator.Compile(
+ """
+ var o = System.Activator.CreateInstance(typeof(object));
+ return 0;
+ """));
+ }
+
[Fact]
public void Rejects_Environment_GetEnvironmentVariable_at_compile()
{
- // Environment lives in System.Private.CoreLib (allow-listed for primitives) —
- // BUT calling .GetEnvironmentVariable exposes process state we don't want in
- // scripts. In an allow-list sandbox this passes because mscorlib is allowed;
- // relying on ScriptSandbox alone isn't enough for the Environment class. We
- // document here that the CURRENT sandbox allows Environment — acceptable because
- // Environment doesn't leak outside the process boundary, doesn't side-effect
- // persistent state, and Phase 7 plan decision #6 targets File/Net/Process/
- // reflection specifically.
- //
- // This test LOCKS that compromise: operators should not be surprised if a
- // script reads an env var. If we later decide to tighten, this test flips.
- var evaluator = ScriptEvaluator.Compile(
- """return System.Environment.GetEnvironmentVariable("PATH");""");
- evaluator.ShouldNotBeNull();
+ // The whole System.Environment type is forbidden (Core.Scripting-001) — even the
+ // read-only GetEnvironmentVariable member. Once Exit / FailFast made the type
+ // dangerous, the cleanest member-granular rule is to deny the type outright; the
+ // read path has no legitimate use in a SCADA predicate either.
+ Should.Throw(() =>
+ ScriptEvaluator.Compile(
+ """return System.Environment.GetEnvironmentVariable("PATH");"""));
}
[Fact]
diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/TimedScriptEvaluatorTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/TimedScriptEvaluatorTests.cs
index 765f0f1..91a2ee6 100644
--- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/TimedScriptEvaluatorTests.cs
+++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/TimedScriptEvaluatorTests.cs
@@ -29,12 +29,13 @@ public sealed class TimedScriptEvaluatorTests
[Fact]
public async Task Script_longer_than_timeout_throws_ScriptTimeoutException()
{
- // Scripts can't easily do Thread.Sleep in the sandbox (System.Threading.Thread
- // is denied). But a tight CPU loop exceeds any short timeout.
+ // Scripts can't reach the sandbox-denied process surface (System.Threading.Thread
+ // and System.Environment are both denied — Core.Scripting-001). A tight CPU loop
+ // that never returns exceeds any short timeout without touching a forbidden type.
var inner = ScriptEvaluator.Compile(
"""
- var end = Environment.TickCount64 + 5000;
- while (Environment.TickCount64 < end) { }
+ long acc = 0;
+ while (true) { acc++; if (acc < 0) break; }
return 1;
""");
var timed = new TimedScriptEvaluator(
@@ -54,8 +55,8 @@ public sealed class TimedScriptEvaluatorTests
// paths aren't misclassified as timeouts.
var inner = ScriptEvaluator.Compile(
"""
- var end = Environment.TickCount64 + 10000;
- while (Environment.TickCount64 < end) { }
+ long acc = 0;
+ while (true) { acc++; if (acc < 0) break; }
return 1;
""");
var timed = new TimedScriptEvaluator(
@@ -119,8 +120,8 @@ public sealed class TimedScriptEvaluatorTests
{
var inner = ScriptEvaluator.Compile(
"""
- var end = Environment.TickCount64 + 5000;
- while (Environment.TickCount64 < end) { }
+ long acc = 0;
+ while (true) { acc++; if (acc < 0) break; }
return 1;
""");
var timed = new TimedScriptEvaluator(