fix(scriptanalysis): M3.1 review — Pass 2 self-sufficient descent, pin nested-forbidden + nameof cases, drop dead code

src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptTrustValidator.cs

@@ -38,6 +38,14 @@ namespace ZB.MOM.WW.ScadaBridge.ScriptAnalysis;
 /// is a true sandbox — this is best-effort defence-in-depth; genuine containment
 /// needs a runtime boundary.
 /// </para>
+///
+/// <para>
+/// A forbidden type reference inside <c>nameof(...)</c> (e.g.
+/// <c>nameof(System.IO.File)</c>) is deliberately flagged: this is
+/// conservative/fail-safe — <c>nameof</c> is rare in scripts and a script has
+/// no business naming a forbidden type even there, so we prefer fail-safe over
+/// adding special-case suppression logic to a security trust boundary.
+/// </para>
 /// </summary>
 public static class ScriptTrustValidator
 {
@@ -287,11 +295,18 @@ public static class ScriptTrustValidator
             var text = StripWhitespace(node.ToString());
 
             // An allowed-exception name (or a strict prefix of one, e.g.
-            // "System.Threading" under "System.Threading.Tasks") is OK — stop
+            // "System.Threading" under "System.Threading.Tasks") must NOT be
-            // descending so the bare forbidden-namespace qualifier of an allowed
+            // reported — but we STILL DESCEND into its children so a forbidden
-            // type is not re-examined and falsely flagged.
+            // type nested inside (e.g. a generic argument under an allowed
+            // System.Threading.Tasks.TaskCompletionSource<System.Threading.Mutex>)
+            // is independently visited and flagged by this pass. Suppress the
+            // outer report, do not stop the walk. (Pass 2 is self-sufficient and
+            // does not rely on the semantic pass to catch nested forbidden refs.)
             if (IsAllowedExceptionPrefix(text))
+            {
+                base.VisitQualifiedName(node);
                 return;
+            }
 
             // Check the longest qualified name; do not descend so a single
             // System.IO.File reference is reported once, not three times.
@@ -323,13 +338,21 @@ public static class ScriptTrustValidator
                 // Still descend: the receiver may contain a further violation.
             }
 
-            // An allowed-exception member-access (or a strict prefix of one) is
+            // An allowed-exception member-access (or a strict prefix of one)
-            // OK — stop descending so the bare forbidden-namespace qualifier of
+            // must NOT be reported — the bare forbidden-namespace qualifier of an
-            // an allowed type (e.g. the "System.Threading" inside
+            // allowed type (e.g. the "System.Threading" inside
-            // "System.Threading.Tasks.Task.Delay") is not re-examined and
+            // "System.Threading.Tasks.Task.Delay") is a false positive. But we
-            // falsely flagged.
+            // STILL DESCEND into its children so a forbidden reference nested
+            // inside (e.g. a System.IO.File access in an allowed
+            // System.Threading.Tasks.Task.Run(...) lambda body) is independently
+            // visited and flagged by this pass. Suppress the outer report, do not
+            // stop the walk. (Pass 2 is self-sufficient — it does not rely on the
+            // semantic pass to catch nested forbidden refs.)
             if (IsAllowedExceptionPrefix(text))
+            {
+                base.VisitMemberAccessExpression(node);
                 return;
+            }
 
             // Catches fully-qualified expressions such as System.IO.File.Delete(...).
             if (IsForbiddenDottedName(text))
@@ -356,8 +379,8 @@ public static class ScriptTrustValidator
             }
 
             // A bare reference to a reflection entry-point type. 'Activator' has
-            // no non-reflection use.
+            // no non-reflection use. ('dynamic' already returned above.)
-            if (ScriptTrustPolicy.ForbiddenIdentifiers.Contains(text) && text != "dynamic")
+            if (ScriptTrustPolicy.ForbiddenIdentifiers.Contains(text))
             {
                 _violations.Add($"forbidden reflection type reference '{text}'");
                 return;

tests/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests/ScriptTrustValidatorTests.cs

@@ -100,6 +100,53 @@ public class ScriptTrustValidatorTests
         Assert.NotEmpty(ScriptTrustValidator.FindViolations(code));
     }
 
+    [Fact]
+    public void Rejects_ForbiddenIo_NestedInAllowedTaskRunLambda()
+    {
+        // A forbidden System.IO reference buried inside an allowed Task.Run lambda.
+        // The allowed-exception prefix on the outer member access must NOT shadow
+        // the nested forbidden reference — Pass 2 must descend into the lambda.
+        var code = "await System.Threading.Tasks.Task.Run(() => System.IO.File.ReadAllText(\"x\"));";
+        Assert.NotEmpty(ScriptTrustValidator.FindViolations(code));
+    }
+
+    [Fact]
+    public void Rejects_ForbiddenMutex_AsGenericArg_UnderAllowedTasksPrefix()
+    {
+        // System.Threading.Mutex (forbidden) appears as a generic argument of an
+        // allowed System.Threading.Tasks.TaskCompletionSource<T>. The allowed
+        // outer name must not shadow the forbidden generic arg.
+        var code = "System.Threading.Tasks.TaskCompletionSource<System.Threading.Mutex> tcs = null;";
+        Assert.NotEmpty(ScriptTrustValidator.FindViolations(code));
+    }
+
+    [Fact]
+    public void Rejects_DirectThreadingMutex_NotThreadSleep()
+    {
+        // A direct forbidden System.Threading type (not Thread.Sleep) — pins that
+        // the broad System.Threading deny-list catches more than the one cased test.
+        var code = "var m = new System.Threading.Mutex();";
+        Assert.NotEmpty(ScriptTrustValidator.FindViolations(code));
+    }
+
+    [Fact]
+    public void Rejects_ForbiddenFileInfo_AsGenericArg()
+    {
+        // System.IO.FileInfo (forbidden) as a generic argument of an allowed
+        // System.Collections.Generic.List<T>.
+        var code = "System.Collections.Generic.List<System.IO.FileInfo> x = null;";
+        Assert.NotEmpty(ScriptTrustValidator.FindViolations(code));
+    }
+
+    [Fact]
+    public void Rejects_NameOf_ForbiddenType()
+    {
+        // Conservative fail-safe: naming a forbidden type inside nameof(...) is
+        // deliberately flagged (a script has no business naming it even there).
+        var code = "var s = nameof(System.IO.File);";
+        Assert.NotEmpty(ScriptTrustValidator.FindViolations(code));
+    }
+
     // ---- Clean (empty violations) -------------------------------------------
 
     [Fact]