From 60366b72c6db43649e60b800ee8261e2555f14c8 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 09:23:03 -0400 Subject: [PATCH] fix(scripting): resolve Medium code-review finding (Core.Scripting-003) Add System.Threading.Tasks to ForbiddenNamespacePrefixes so scripts cannot use Task.Run / Parallel to spawn background work that outlives the per-evaluation timeout. Document the unbounded-memory accepted trade-off and the Task denial rationale in docs/VirtualTags.md (new "Known resource limits" subsection) and cross-reference from docs/ScriptedAlarms.md. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Core.Scripting/findings.md | 18 +++++++++--------- docs/ScriptedAlarms.md | 2 +- docs/VirtualTags.md | 8 +++++++- .../ForbiddenTypeAnalyzer.cs | 17 ++++++++++++----- 4 files changed, 29 insertions(+), 16 deletions(-) diff --git a/code-reviews/Core.Scripting/findings.md b/code-reviews/Core.Scripting/findings.md index 68772e7..2ef06e3 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 | 9 | +| Open findings | 5 | ## Checklist coverage @@ -112,7 +112,7 @@ node form plus over-block guards for allowed generics/`typeof`. | Severity | Medium | | Category | Security | | Location | `TimedScriptEvaluator.cs:9`, `ScriptSandbox.cs:30` | -| Status | Open | +| Status | Resolved | **Description:** There is no bound on memory a script may allocate or on the number of threads/tasks a script may spawn. The class docs acknowledge unbounded memory as "a budget @@ -132,7 +132,7 @@ script authoring behind an Admin permission and treat the test-harness preview a control point, or track an explicit v3 issue for out-of-process execution. Record the decision so it is not silently lost. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — added `System.Threading.Tasks` to `ForbiddenNamespacePrefixes` (blocking `Task.Run` / `Parallel` fan-out); documented the unbounded-memory accepted risk and the `Task` denial rationale in `docs/VirtualTags.md` (new "Known resource limits" subsection) and cross-referenced from `docs/ScriptedAlarms.md`. ### Core.Scripting-004 @@ -141,7 +141,7 @@ decision so it is not silently lost. | Severity | Medium | | Category | Correctness & logic bugs | | Location | `DependencyExtractor.cs:73` | -| Status | Open | +| Status | Resolved | **Description:** The walker matches tag-access calls purely by spelling — any `InvocationExpressionSyntax` whose member name is `GetTag` or `SetVirtualTag` is treated as @@ -160,7 +160,7 @@ member-access call to a non-ctx `GetTag` is untested and would be misattributed. (matching the `ScriptGlobals.ctx` field name). Add a test for `someOtherObject.GetTag("X")` asserting it is ignored. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `VisitInvocationExpression` now additionally checks that `member.Expression` is an `IdentifierNameSyntax` with `ValueText == "ctx"` before treating the call as a dependency; test `Ignores_member_access_GetTag_on_non_ctx_receiver` added to `DependencyExtractorTests`. ### Core.Scripting-005 @@ -215,7 +215,7 @@ evicted. | Severity | Medium | | Category | Error handling & resilience | | Location | `TimedScriptEvaluator.cs:60` | -| Status | Open | +| Status | Resolved | **Description:** `RunAsync` wraps the inner run in `Task.Run(...)` and then awaits `WaitAsync(Timeout, ct)`. If the caller-supplied `ct` cancels at roughly the same time the @@ -231,7 +231,7 @@ and throw `OperationCanceledException(ct)` instead of `ScriptTimeoutException` w caller's token is cancelled, so caller cancellation deterministically wins regardless of race ordering. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — in the `catch (TimeoutException)` handler, `ct.IsCancellationRequested` is now checked and `OperationCanceledException(ct)` thrown before `ScriptTimeoutException`, so caller cancellation deterministically wins regardless of race ordering; regression test `Caller_cancellation_wins_even_when_timeout_fires_first` added to `TimedScriptEvaluatorTests`. ### Core.Scripting-008 @@ -292,7 +292,7 @@ code. | Severity | Medium | | Category | Testing coverage | | Location | `tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptSandboxTests.cs:54` | -| Status | Open | +| Status | Resolved | **Description:** The sandbox-escape test suite covers only the four obvious vectors (File / Http / Process / Reflection) as direct member-access calls. It does not test: @@ -309,7 +309,7 @@ surface. Core.Scripting-002 and every forbidden namespace/member in Core.Scripting-001. Each must assert a `ScriptSandboxViolationException` (or `CompilationErrorException`) at compile. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — added `ScriptSandboxTests` cases for `System.Threading.Thread`, `System.Threading.Tasks.Task.Run`, `System.Runtime.InteropServices.Marshal`, and `Microsoft.Win32.Registry` (the four namespace-deny-list vectors that had no test); the 001/002 vectors (Environment.Exit/FailFast/AppDomain/GC/Activator, typeof, generics, cast, default(T), is/as, array element, declared variable) were already covered by the -001/-002 resolution commits. All 79 tests pass. ### Core.Scripting-011 diff --git a/docs/ScriptedAlarms.md b/docs/ScriptedAlarms.md index 744fcf7..63fe2d1 100644 --- a/docs/ScriptedAlarms.md +++ b/docs/ScriptedAlarms.md @@ -35,7 +35,7 @@ new ScriptedAlarmDefinition( ## Predicate evaluation -Alarm predicates reuse the same Roslyn sandbox as virtual tags — `ScriptEvaluator` compiles the source, `TimedScriptEvaluator` wraps it with the configured timeout (default from `TimedScriptEvaluator.DefaultTimeout`), and `DependencyExtractor` statically harvests the tag paths the script reads. The sandbox rules (forbidden types, cancellation, logging sinks) are documented in [VirtualTags.md](VirtualTags.md); ScriptedAlarms does not redefine them. +Alarm predicates reuse the same Roslyn sandbox as virtual tags — `ScriptEvaluator` compiles the source, `TimedScriptEvaluator` wraps it with the configured timeout (default from `TimedScriptEvaluator.DefaultTimeout`), and `DependencyExtractor` statically harvests the tag paths the script reads. The sandbox rules (forbidden types, cancellation, logging sinks) are documented in [VirtualTags.md](VirtualTags.md); ScriptedAlarms does not redefine them. The known memory / CPU resource limits are documented there as well. `AlarmPredicateContext` (`AlarmPredicateContext.cs`) is the script's `ScriptContext` subclass: diff --git a/docs/VirtualTags.md b/docs/VirtualTags.md index 0758e04..435f11b 100644 --- a/docs/VirtualTags.md +++ b/docs/VirtualTags.md @@ -18,7 +18,13 @@ User scripts are compiled via `Microsoft.CodeAnalysis.CSharp.Scripting` against `ScriptSandbox.Build` allow-lists exactly: `System.Private.CoreLib` (primitives + `Math` + `Convert`), `System.Linq`, `Core.Abstractions` (for `DataValueSnapshot` / `DriverDataType`), `Core.Scripting` (for `ScriptContext` + `Deadband`), `Serilog` (for `ILogger`), and the concrete context type's assembly. Pre-imported namespaces: `System`, `System.Linq`, `ZB.MOM.WW.OtOpcUa.Core.Abstractions`, `ZB.MOM.WW.OtOpcUa.Core.Scripting`. -`ForbiddenTypeAnalyzer.ForbiddenNamespacePrefixes` currently denies `System.IO`, `System.Net`, `System.Diagnostics`, `System.Reflection`, `System.Threading.Thread`, `System.Runtime.InteropServices`, `Microsoft.Win32`. Matching is by prefix against the resolved symbol's containing namespace, so `System.Net` catches `System.Net.Http.HttpClient` and every subnamespace. `System.Environment` is explicitly allowed. +`ForbiddenTypeAnalyzer.ForbiddenNamespacePrefixes` currently denies `System.IO`, `System.Net`, `System.Diagnostics`, `System.Reflection`, `System.Threading.Thread`, `System.Threading.Tasks`, `System.Runtime.InteropServices`, `Microsoft.Win32`. Matching is by prefix against the resolved symbol's containing namespace, so `System.Net` catches `System.Net.Http.HttpClient` and every subnamespace. `System.Threading.Tasks` is denied because scripts are synchronous predicates with no legitimate need to start background tasks — a `Task.Run` fan-out would outlive the per-evaluation timeout entirely (Core.Scripting-003). `System.Environment`, `System.AppDomain`, `System.GC`, and `System.Activator` are denied type-granularly via `ForbiddenFullTypeNames` because they live directly in the `System` namespace (which is otherwise allowed for primitives) — `Environment.Exit` / `FailFast` terminate the host process outright (Core.Scripting-001). + +#### Known resource limits (accepted trade-offs) + +The sandbox cannot prevent a script from **allocating unbounded memory**. A script calling `new byte[int.MaxValue]` repeatedly, or accumulating a large LINQ enumeration, can drive the server process to `OutOfMemoryException` before the 250 ms timeout fires. Script authoring is gated behind the Admin permission as the primary control; the test-harness preview (Stream F.4) allows operators to exercise a script before publishing. Out-of-process script execution is a v3 concern. + +Similarly, **`System.Threading.Tasks` is now denied** (Core.Scripting-003), which prevents `Task.Run` / `Parallel` fan-out that would spawn background work outliving the timeout. However, a tight CPU-bound loop still runs on its thread-pool thread after `WaitAsync` returns — see the `TimedScriptEvaluator` remarks for detail. The orphaned thread is reclaimed when the Roslyn runtime eventually returns; in practice the operator fixes the script once the structured timeout warning appears in `scripts-*.log`. ### Compile cache (`CompiledScriptCache`) 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 77f8a1f..67c617d 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/ForbiddenTypeAnalyzer.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/ForbiddenTypeAnalyzer.cs @@ -18,9 +18,12 @@ 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.IO, System.Net, System.Diagnostics, /// System.Reflection, System.Threading.Thread, -/// System.Runtime.InteropServices. +/// System.Threading.Tasks (scripts are synchronous predicates — no +/// legitimate need to start background tasks; a Task.Run fan-out outlives +/// the evaluation timeout entirely), System.Runtime.InteropServices, +/// Microsoft.Win32. (Core.Scripting-003.) /// /// /// Deny-list prefix match. System.Net catches System.Net.Http, @@ -58,11 +61,15 @@ public static class ForbiddenTypeAnalyzer [ "System.IO", "System.Net", - "System.Diagnostics", // catches Process, ProcessStartInfo, EventLog, Trace/Debug file sinks + "System.Diagnostics", // catches Process, ProcessStartInfo, EventLog, Trace/Debug file sinks "System.Reflection", - "System.Threading.Thread", // raw Thread — Tasks stay allowed (different namespace) + "System.Threading.Thread", // raw Thread — blocks the thread-pool + "System.Threading.Tasks", // Task.Run / Parallel — scripts are synchronous predicates + // and have no legitimate need to start background work; + // a Task fan-out outlives the evaluation timeout entirely + // (Core.Scripting-003). "System.Runtime.InteropServices", - "Microsoft.Win32", // registry + "Microsoft.Win32", // registry ]; ///