refactor(inboundapi): M3.4 ForbiddenApiChecker delegates to shared ScriptAnalysis validator

src/ZB.MOM.WW.ScadaBridge.InboundAPI/ForbiddenApiChecker.cs

@@ -1,6 +1,4 @@
-using Microsoft.CodeAnalysis;
+using ZB.MOM.WW.ScadaBridge.ScriptAnalysis;
-using Microsoft.CodeAnalysis.CSharp;
-using Microsoft.CodeAnalysis.CSharp.Syntax;
 
 namespace ZB.MOM.WW.ScadaBridge.InboundAPI;
 
@@ -8,89 +6,30 @@ namespace ZB.MOM.WW.ScadaBridge.InboundAPI;
 /// InboundAPI-005: Enforces the ScadaBridge script trust model on inbound API method
 /// scripts before they are compiled into executable handlers.
 ///
-/// The trust model (CLAUDE.md, Akka.NET conventions) forbids scripts from reaching
+/// This class is now a thin shim that delegates to the shared, authoritative
-/// <c>System.IO</c>, <c>System.Diagnostics.Process</c>, <c>System.Threading</c>,
+/// <see cref="ScriptTrustValidator.FindViolations"/> implemented in
-/// <c>System.Reflection</c>, and raw network APIs. Roslyn scripting performs no
+/// <c>ZB.MOM.WW.ScadaBridge.ScriptAnalysis</c> (M3.4). The unified validator runs
-/// API allow/deny-listing — restricting default imports is a convenience, not a
+/// both a semantic symbol pass (catching alias / <c>global::</c> / <c>using static</c>
-/// sandbox — so a script can fully-qualify any referenced type. This static check
+/// escapes) and the reflection-gateway + <c>dynamic</c> / <c>Activator</c> syntactic
-/// walks the script syntax tree and rejects any reference to a forbidden namespace,
+/// hardening that previously lived exclusively in this file.
-/// whether reached through a <c>using</c> directive or a fully-qualified name.
 ///
 /// <para>
 /// InboundAPI-015: a purely namespace-textual deny-list is bypassable because
 /// reflection is reachable through members of <em>permitted</em> types that never
 /// spell a forbidden namespace, e.g.
-/// <c>typeof(string).Assembly.GetType("System.IO.File")</c>. The walker therefore
+/// <c>typeof(string).Assembly.GetType("System.IO.File")</c>. The shared validator
-/// also rejects a curated set of reflection-gateway member names (<c>GetType</c>,
+/// handles this with both semantic resolution and reflection-gateway member
-/// <c>Assembly</c>, <c>GetMethod</c>, <c>InvokeMember</c>, <c>CreateInstance</c>, …)
+/// hardening — <c>GetType</c>, <c>Assembly</c>, <c>GetMethod</c>, <c>InvokeMember</c>,
-/// and the <c>dynamic</c> keyword. This is hardening of a best-effort static check,
+/// <c>CreateInstance</c>, and the <c>dynamic</c> keyword are all rejected. This
-/// <strong>not</strong> a true sandbox — a determined script author may still find
+/// remains hardening of a best-effort static check, <strong>not</strong> a true sandbox
-/// a vector the syntax walker cannot see (see the security notes in
+/// (see the security notes in <c>code-reviews/InboundAPI/findings.md</c>,
-/// <c>code-reviews/InboundAPI/findings.md</c>, InboundAPI-015). The check is
+/// InboundAPI-015). The check is defence-in-depth; genuine containment needs a
-/// defence-in-depth; genuine containment needs a runtime boundary (restricted
+/// runtime boundary (restricted <c>AssemblyLoadContext</c> / curated reference set /
-/// <c>AssemblyLoadContext</c> / curated reference set / out-of-process sandbox).
+/// out-of-process sandbox).
 /// </para>
 /// </summary>
 public static class ForbiddenApiChecker
 {
-    /// <summary>
-    /// Namespace prefixes the trust model forbids. A script segment matches if it is
-    /// equal to one of these or is a child namespace of it.
-    /// </summary>
-    private static readonly string[] ForbiddenNamespaces =
-    {
-        "System.IO",
-        "System.Diagnostics",   // covers Process
-        "System.Threading",     // Task/Tasks is explicitly re-allowed below
-        "System.Reflection",
-        "System.Net",           // raw network (Sockets, HttpClient, etc.)
-        "System.Runtime.InteropServices",
-        "Microsoft.Win32",
-    };
-
-    /// <summary>
-    /// Namespaces that would otherwise be caught by a forbidden prefix but are
-    /// required for normal async script authoring and carry no host-access risk.
-    /// </summary>
-    private static readonly string[] AllowedExceptions =
-    {
-        "System.Threading.Tasks",
-    };
-
-    /// <summary>
-    /// InboundAPI-015: member names that are reflection gateways. Reaching any of
-    /// these — even off a permitted type such as <c>typeof(string)</c> or a plain
-    /// <c>string</c> — lets a script escape the namespace deny-list (obtain an
-    /// arbitrary <c>Type</c>, load an assembly, late-bind a method). They are
-    /// rejected regardless of the receiver expression. <c>Invoke</c> is deliberately
-    /// excluded because <c>Action</c>/<c>Func</c> delegate invocation is legitimate;
-    /// the reflection <c>MethodInfo.Invoke</c> path is already cut off by rejecting
-    /// the <c>GetMethod</c>/<c>GetConstructor</c> that produces the <c>MethodInfo</c>.
-    /// </summary>
-    private static readonly HashSet<string> ForbiddenMemberNames = new(StringComparer.Ordinal)
-    {
-        "GetType",          // object.GetType() / Type.GetType(string) — yields a System.Type
-        "GetTypeInfo",      // -> TypeInfo (reflection)
-        "Assembly",         // Type.Assembly — yields a System.Reflection.Assembly
-        "Module",           // Type.Module / MethodBase.Module
-        "CreateInstance",   // Activator.CreateInstance / Assembly.CreateInstance
-        "InvokeMember",     // Type.InvokeMember — late-bound dispatch
-        "GetMethod",
-        "GetMethods",
-        "GetConstructor",
-        "GetConstructors",
-        "GetField",
-        "GetFields",
-        "GetProperty",
-        "GetProperties",
-        "GetMember",
-        "GetMembers",
-        "GetRuntimeMethod",
-        "GetRuntimeMethods",
-        "MethodHandle",     // RuntimeMethodHandle escape
-        "TypeHandle",
-    };
-
     /// <summary>
     /// Analyses the script source and returns the list of trust-model violations.
     /// An empty list means the script is acceptable.
@@ -102,111 +41,6 @@ public static class ForbiddenApiChecker
         if (string.IsNullOrWhiteSpace(scriptCode))
             return Array.Empty<string>();
 
-        var tree = CSharpSyntaxTree.ParseText(
+        return ScriptTrustValidator.FindViolations(scriptCode);
-            scriptCode,
-            new CSharpParseOptions(kind: SourceCodeKind.Script));
-
-        var walker = new ForbiddenApiWalker();
-        walker.Visit(tree.GetRoot());
-        return walker.Violations;
-    }
-
-    private static bool IsForbidden(string dottedName)
-    {
-        foreach (var allowed in AllowedExceptions)
-        {
-            if (dottedName == allowed || dottedName.StartsWith(allowed + ".", StringComparison.Ordinal))
-                return false;
-        }
-
-        foreach (var forbidden in ForbiddenNamespaces)
-        {
-            if (dottedName == forbidden || dottedName.StartsWith(forbidden + ".", StringComparison.Ordinal))
-                return true;
-        }
-
-        return false;
-    }
-
-    private sealed class ForbiddenApiWalker : CSharpSyntaxWalker
-    {
-        private readonly List<string> _violations = new();
-
-        /// <summary>Gets the accumulated list of trust-model violation descriptions.</summary>
-        public IReadOnlyList<string> Violations => _violations;
-
-        /// <inheritdoc />
-        public override void VisitUsingDirective(UsingDirectiveSyntax node)
-        {
-            if (node.Name is not null && IsForbidden(node.Name.ToString()))
-                _violations.Add($"forbidden namespace import '{node.Name}'");
-
-            base.VisitUsingDirective(node);
-        }
-
-        /// <inheritdoc />
-        public override void VisitQualifiedName(QualifiedNameSyntax node)
-        {
-            // Check the longest qualified name; do not descend so a single
-            // System.IO.File reference is reported once, not three times.
-            var text = node.ToString();
-            if (IsForbidden(text))
-            {
-                _violations.Add($"forbidden type reference '{text}'");
-                return;
-            }
-
-            base.VisitQualifiedName(node);
-        }
-
-        /// <inheritdoc />
-        public override void VisitMemberAccessExpression(MemberAccessExpressionSyntax node)
-        {
-            // Catches fully-qualified expressions such as System.IO.File.Delete(...).
-            var text = node.ToString();
-            if (IsForbidden(text))
-            {
-                _violations.Add($"forbidden API access '{text}'");
-                return;
-            }
-
-            // InboundAPI-015: reject reflection-gateway members regardless of the
-            // receiver. typeof(string).Assembly.GetType("System.IO.File") never
-            // spells a forbidden namespace, but '.Assembly' and '.GetType' do
-            // appear here as the accessed member name.
-            var memberName = node.Name.Identifier.ValueText;
-            if (ForbiddenMemberNames.Contains(memberName))
-            {
-                _violations.Add($"forbidden reflection member access '.{memberName}'");
-                // Still descend: the receiver may contain a further violation.
-            }
-
-            base.VisitMemberAccessExpression(node);
-        }
-
-        /// <inheritdoc />
-        public override void VisitIdentifierName(IdentifierNameSyntax node)
-        {
-            // InboundAPI-015: 'dynamic' widens late-bound member access that the
-            // static walker cannot see through — reject its use outright. The
-            // 'dynamic' contextual keyword surfaces as an identifier name.
-            if (node.Identifier.ValueText == "dynamic")
-            {
-                _violations.Add("forbidden use of the 'dynamic' keyword");
-                return;
-            }
-
-            // InboundAPI-015: a bare reference to the reflection entry-point types
-            // (e.g. 'Activator', 'Type') as an identifier. 'Activator' has no
-            // non-reflection use; flag it. ('Type' as an identifier is too broad
-            // to flag here — the gateway members above already cut off its use.)
-            if (node.Identifier.ValueText == "Activator")
-            {
-                _violations.Add("forbidden reflection type reference 'Activator'");
-                return;
-            }
-
-            base.VisitIdentifierName(node);
-        }
     }
 }

src/ZB.MOM.WW.ScadaBridge.InboundAPI/ZB.MOM.WW.ScadaBridge.InboundAPI.csproj

@@ -17,6 +17,7 @@
     <!-- AuditWriteMiddleware reads AuditLogOptions.InboundMaxBytes to bound
          per-request request/response audit capture at the source. -->
     <ProjectReference Include="../ZB.MOM.WW.ScadaBridge.AuditLog/ZB.MOM.WW.ScadaBridge.AuditLog.csproj" />
+    <ProjectReference Include="../ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.csproj" />
   </ItemGroup>
 
   <ItemGroup>

tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/ForbiddenApiCheckerTests.cs

@@ -3,9 +3,18 @@ namespace ZB.MOM.WW.ScadaBridge.InboundAPI.Tests;
 /// <summary>
 /// InboundAPI-005 / InboundAPI-015: tests for the script-trust-model checker.
 ///
-/// InboundAPI-015 hardens the textual walker against reflection reached through
+/// Since M3.4 <see cref="ForbiddenApiChecker"/> delegates to the shared
-/// permitted-type members that never spell a forbidden namespace, e.g.
+/// <c>ScriptTrustValidator</c> in <c>ZB.MOM.WW.ScadaBridge.ScriptAnalysis</c>.
-/// <c>typeof(string).Assembly.GetType("System.IO.File")</c>.
+/// The unified policy differs from the old local deny-list in one notable way:
+/// <c>System.Diagnostics</c> is loosened to <c>System.Diagnostics.Process</c>
+/// only — <c>Stopwatch</c>, <c>Debug</c>, and other non-Process Diagnostics
+/// types are now ALLOWED.
+///
+/// InboundAPI-015 hardening (reflection-gateway / dynamic / Activator) is
+/// preserved unchanged via the shared validator.
+///
+/// Violation message wording is intentionally not asserted — callers should
+/// use <c>.Count</c> / <c>.Any()</c> rather than exact strings.
 /// </summary>
 public class ForbiddenApiCheckerTests
 {
@@ -26,11 +35,37 @@ public class ForbiddenApiCheckerTests
         Assert.False(IsRejected(script), script);
     }
 
+    // --- System.Diagnostics loosening: Process is still forbidden; non-Process types
+    //     (Stopwatch, Debug) are now ALLOWED by the unified policy (M3.4). The old
+    //     InboundAPI checker blocked the entire System.Diagnostics namespace. ---
+
+    [Fact]
+    public void Diagnostics_Process_StillForbidden()
+    {
+        // System.Diagnostics.Process is an explicit forbidden scope in ScriptTrustPolicy.
+        Assert.True(IsRejected("System.Diagnostics.Process.Start(\"/bin/sh\"); return null;"));
+    }
+
+    [Fact]
+    public void Diagnostics_Stopwatch_NowAllowed()
+    {
+        // System.Diagnostics.Stopwatch is NOT under the forbidden scope
+        // "System.Diagnostics.Process" — the unified policy only forbids
+        // System.Diagnostics.Process, not the whole System.Diagnostics namespace.
+        Assert.False(IsRejected("var sw = new System.Diagnostics.Stopwatch(); sw.Start(); return null;"));
+    }
+
+    [Fact]
+    public void Diagnostics_Debug_NowAllowed()
+    {
+        // System.Diagnostics.Debug is also not covered by the Process-only scope.
+        Assert.False(IsRejected("System.Diagnostics.Debug.WriteLine(\"hi\"); return null;"));
+    }
+
     // --- Baseline: forbidden namespaces (textual) must still be rejected ---
 
     [Theory]
     [InlineData("System.IO.File.Delete(\"/tmp/x\"); return null;")]
-    [InlineData("System.Diagnostics.Process.Start(\"/bin/sh\"); return null;")]
     [InlineData("using System.Reflection; return null;")]
     [InlineData("var s = new System.Net.Sockets.Socket(default, default, default); return null;")]
     public void ForbiddenNamespace_Rejected(string script)
@@ -101,4 +136,17 @@ public class ForbiddenApiCheckerTests
         // dynamic widens late-bound member access the static walker cannot see through.
         Assert.True(IsRejected("dynamic d = Parameters; return null;"));
     }
+
+    // --- FindViolations returns a non-null, non-throwing result for edge inputs ---
+
+    [Theory]
+    [InlineData("")]
+    [InlineData("   ")]
+    [InlineData(null!)]
+    public void EmptyOrWhitespace_ReturnsEmptyList(string? script)
+    {
+        var result = ForbiddenApiChecker.FindViolations(script!);
+        Assert.NotNull(result);
+        Assert.Empty(result);
+    }
 }