refactor(centralui): M3.5 ScriptAnalysisService uses shared deny-list + delegates trust verdict

2026-06-16 19:40:03 -04:00
parent 64d6ac7288
commit cf935d5744
3 changed files with 167 additions and 40 deletions
@@ -12,6 +12,7 @@ using Microsoft.CodeAnalysis.Text;
 using Microsoft.Extensions.Caching.Memory;
 using Microsoft.Extensions.DependencyInjection;
 using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services;
+using SharedTrust = ZB.MOM.WW.ScadaBridge.ScriptAnalysis;

 namespace ZB.MOM.WW.ScadaBridge.CentralUI.ScriptAnalysis;

@@ -56,18 +57,6 @@ public class ScriptAnalysisService
            "System.Text",
            "System.Threading.Tasks");

-    // Namespaces and types banned by the script trust model.
-    // Tasks live under System.Threading.Tasks and remain allowed.
-    private static readonly string[] ForbiddenNamespacePrefixes =
-    {
-        "System.IO",
-        "System.Diagnostics",
-        "System.Reflection",
-        "System.Net",
-        "System.Threading.Thread",
-        "System.Threading.Tasks.Sources",
-    };
-
    private readonly ISharedScriptCatalog _sharedScripts;
    private readonly IMemoryCache _cache;
    private readonly IServiceProvider _services;
@@ -1152,14 +1141,22 @@ public class ScriptAnalysisService
    }

    /// <summary>
-    /// Finds every reference to a forbidden API — the documented script trust model,
-    /// see <see cref="ForbiddenNamespacePrefixes"/>. Identifiers are resolved against
-    /// the semantic model, so a forbidden type or member is caught however it is
-    /// written: bare (<c>File</c>), fully qualified
-    /// (<c>System.IO.File.WriteAllText</c>), or via an alias — while a user identifier
-    /// that merely shares a name with a forbidden type (<c>var File = …</c>) does not
-    /// false-positive. Used both for editor diagnostics and as the pre-execution
-    /// trust-model gate (see <see cref="EnforceTrustModel"/>).
+    /// Finds every reference to a forbidden API for the editor squiggle — the
+    /// documented script trust model, sourced from the shared
+    /// <see cref="SharedTrust.ScriptTrustPolicy.ForbiddenScopes"/> /
+    /// <see cref="SharedTrust.ScriptTrustPolicy.AllowedExceptions"/> deny-list via
+    /// <see cref="IsForbiddenName"/>. Identifiers are resolved against the semantic
+    /// model, so a forbidden type or member is caught however it is written: bare
+    /// (<c>File</c>), fully qualified (<c>System.IO.File.WriteAllText</c>), or via an
+    /// alias — while a user identifier that merely shares a name with a forbidden type
+    /// (<c>var File = …</c>) does not false-positive.
+    ///
+    /// This is the position-aware, namespace-level editor advisory: it keeps the
+    /// line/column markers Monaco needs. The authoritative run verdict lives in
+    /// <see cref="EnforceTrustModel"/>, which delegates to the shared
+    /// <see cref="SharedTrust.ScriptTrustValidator"/> (and additionally covers
+    /// reflection-gateway members + dynamic/Activator hardening that the editor
+    /// advisory does not mark).
    /// </summary>
    private static IEnumerable<DiagnosticMarker> FindForbiddenApiUsages(SyntaxTree tree, SemanticModel model)
    {
@@ -1208,25 +1205,45 @@ public class ScriptAnalysisService
    /// <summary>
    /// The forbidden namespace/type a symbol implicates, or null if it is allowed.
    /// Checks the symbol's namespace and — for a type or member — the type's full
-    /// name, so an entry like <c>System.Threading.Thread</c> bans that exact type
-    /// while <c>System.Threading</c> (e.g. <c>CancellationToken</c>) stays allowed.
+    /// name. M3.5: the allowed-exception carve-out is applied at the WHOLE-SYMBOL
+    /// level (mirroring the shared <see cref="SharedTrust.ScriptTrustValidator"/>
+    /// semantic pass), so a type whose full name is an allowed exception
+    /// (e.g. <c>System.Threading.CancellationTokenSource</c>) is NOT flagged even
+    /// though its bare containing namespace <c>System.Threading</c> is a forbidden
+    /// root. Without the symbol-level check the namespace scope alone would
+    /// false-positive the allowed cancellation types.
    /// </summary>
    private static string? ForbiddenNameFor(ISymbol? symbol)
    {
        if (symbol == null) return null;
-        foreach (var name in QualifiedNamesOf(symbol))
-            if (IsForbiddenName(name))
-                return name;
-        return null;
+        var names = QualifiedNamesOf(symbol).ToList();
+        if (names.Count == 0) return null;
+
+        // Allowed exception on ANY of the symbol's qualified names wins outright —
+        // the rest of System.Threading stays forbidden, but Tasks +
+        // CancellationToken(Source) survive.
+        if (names.Any(n =>
+                SharedTrust.ScriptTrustPolicy.AllowedExceptions.Any(a => IsUnderScope(n, a))))
+            return null;
+
+        return names.FirstOrDefault(IsForbiddenName);
    }

-    /// <summary>Fully-qualified names a symbol reference implicates for trust-model checking.</summary>
+    /// <summary>
+    /// Fully-qualified names a symbol reference implicates for trust-model checking.
+    /// M3.5: a bare namespace symbol is intentionally ignored — mirroring the shared
+    /// <see cref="SharedTrust.ScriptTrustValidator"/> semantic pass. A namespace name
+    /// on its own performs no action; harm requires referencing a type or member, so
+    /// flagging the bare <c>System.Threading</c> qualifier of an otherwise-allowed
+    /// type (<c>System.Threading.CancellationTokenSource</c>) would be a false
+    /// positive. The forbidden-<c>using</c>-directive path is checked separately on
+    /// the directive text, so namespace imports are still caught.
+    /// </summary>
    private static IEnumerable<string> QualifiedNamesOf(ISymbol symbol)
    {
        switch (symbol)
        {
-            case INamespaceSymbol { IsGlobalNamespace: false } ns:
-                yield return ns.ToDisplayString();
+            case INamespaceSymbol:
                break;
            case ITypeSymbol type:
                if (type.ContainingNamespace is { IsGlobalNamespace: false } tn)
@@ -1249,23 +1266,74 @@ public class ScriptAnalysisService
            ? ns.ToDisplayString() + "." + type.Name
            : type.Name;

-    private static bool IsForbiddenName(string qualifiedName) =>
-        ForbiddenNamespacePrefixes.Any(p =>
-            qualifiedName == p || qualifiedName.StartsWith(p + ".", StringComparison.Ordinal));
+    /// <summary>
+    /// M3.5: the forbidden-namespace verdict for the editor walker is sourced
+    /// from the shared <see cref="SharedTrust.ScriptTrustPolicy"/> — the single
+    /// deny-list the run gate (<see cref="ScriptTrustValidator"/>) also uses, so
+    /// the editor squiggle and the run gate agree on which namespaces are
+    /// forbidden. A name under a <see cref="SharedTrust.ScriptTrustPolicy.ForbiddenScopes"/>
+    /// root is forbidden UNLESS it is also under a
+    /// <see cref="SharedTrust.ScriptTrustPolicy.AllowedExceptions"/> entry
+    /// (async/await + cancellation tokens survive the System.Threading block).
+    /// </summary>
+    private static bool IsForbiddenName(string qualifiedName)
+    {
+        if (SharedTrust.ScriptTrustPolicy.AllowedExceptions.Any(a => IsUnderScope(qualifiedName, a)))
+            return false;
+        return SharedTrust.ScriptTrustPolicy.ForbiddenScopes.Any(f => IsUnderScope(qualifiedName, f));
+    }
+
+    private static bool IsUnderScope(string actual, string root) =>
+        actual.Equals(root, StringComparison.Ordinal)
+        || actual.StartsWith(root + ".", StringComparison.Ordinal);

    /// <summary>
-    /// Pre-execution trust-model gate (CentralUI-001). Returns the forbidden-API
-    /// markers (SCADA001/SCADA002) for a compiled script; an empty list means the
-    /// script is clear to run. This is a static semantic gate, not a process
-    /// sandbox — reflection-based indirection is still out of its reach; full
-    /// isolation would require running scripts in a separate constrained process.
+    /// Pre-execution trust-model gate (CentralUI-001). M3.5: the VERDICT is
+    /// delegated to the shared <see cref="SharedTrust.ScriptTrustValidator"/> —
+    /// the single source of truth the editor squiggle deny-list also derives from,
+    /// so the run gate and the editor agree. The shared validator additionally
+    /// enforces reflection-gateway members + dynamic/Activator hardening beyond the
+    /// editor's namespace-level advisory. An empty result means clear to run.
+    ///
+    /// The compilation's own metadata references are forwarded to the shared
+    /// validator as extra references so its semantic pass resolves symbols against
+    /// the SAME metadata the script was compiled against (the full BCL surface,
+    /// including the SandboxScriptHost globals assembly). This keeps the gate's
+    /// resolution identical to the editor walker's — e.g. a bare <c>Process</c>
+    /// reached through <c>using System.Diagnostics;</c> resolves to
+    /// <c>System.Diagnostics.Process</c> and is caught. Forwarding references never
+    /// whitelists anything: the deny-list still applies regardless of what is
+    /// referenced.
+    ///
+    /// The shared validator returns positionless messages; the run gate doesn't
+    /// need precise positions (only the verdict), so each violation is surfaced as a
+    /// single error marker anchored at the script start. This is a static semantic
+    /// gate, not a process sandbox — full isolation would require a separate
+    /// constrained process.
    /// </summary>
    private static IReadOnlyList<DiagnosticMarker> EnforceTrustModel(Compilation compilation)
    {
        var tree = compilation.SyntaxTrees.FirstOrDefault();
        if (tree == null) return Array.Empty<DiagnosticMarker>();
-        var model = compilation.GetSemanticModel(tree);
-        return FindForbiddenApiUsages(tree, model).ToList();
+
+        var violations = SharedTrust.ScriptTrustValidator.FindViolations(
+            tree.ToString(), compilation.References);
+        if (violations.Count == 0) return Array.Empty<DiagnosticMarker>();
+
+        // Positionless verdict → one error marker per violation at the script
+        // start. Code SCADA002 (forbidden API reference) matches the editor's
+        // semantic-marker code, so existing gate assertions ("SCADA001"/"SCADA002")
+        // continue to hold.
+        return violations
+            .Select(v => new DiagnosticMarker(
+                Severity: 8,
+                StartLineNumber: 1,
+                StartColumn: 1,
+                EndLineNumber: 1,
+                EndColumn: 2,
+                Message: $"{v} — not allowed in scripts (script trust model).",
+                Code: "SCADA002"))
+            .ToList();
    }

    private static CompletionItem ToCompletionItem(ISymbol symbol)
@@ -23,6 +23,7 @@

  <ItemGroup>
    <ProjectReference Include="../ZB.MOM.WW.ScadaBridge.Commons/ZB.MOM.WW.ScadaBridge.Commons.csproj" />
+    <ProjectReference Include="../ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.csproj" />
    <ProjectReference Include="../ZB.MOM.WW.ScadaBridge.Security/ZB.MOM.WW.ScadaBridge.Security.csproj" />
    <ProjectReference Include="../ZB.MOM.WW.ScadaBridge.TemplateEngine/ZB.MOM.WW.ScadaBridge.TemplateEngine.csproj" />
    <ProjectReference Include="../ZB.MOM.WW.ScadaBridge.DeploymentManager/ZB.MOM.WW.ScadaBridge.DeploymentManager.csproj" />