diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs index d78fdd46..a111a93d 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs @@ -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 } /// - /// Finds every reference to a forbidden API — the documented script trust model, - /// see . Identifiers are resolved against - /// the semantic model, so a forbidden type or member is caught however it is - /// written: bare (File), fully qualified - /// (System.IO.File.WriteAllText), or via an alias — while a user identifier - /// that merely shares a name with a forbidden type (var File = …) does not - /// false-positive. Used both for editor diagnostics and as the pre-execution - /// trust-model gate (see ). + /// Finds every reference to a forbidden API for the editor squiggle — the + /// documented script trust model, sourced from the shared + /// / + /// deny-list via + /// . Identifiers are resolved against the semantic + /// model, so a forbidden type or member is caught however it is written: bare + /// (File), fully qualified (System.IO.File.WriteAllText), or via an + /// alias — while a user identifier that merely shares a name with a forbidden type + /// (var File = …) 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 + /// , which delegates to the shared + /// (and additionally covers + /// reflection-gateway members + dynamic/Activator hardening that the editor + /// advisory does not mark). /// private static IEnumerable FindForbiddenApiUsages(SyntaxTree tree, SemanticModel model) { @@ -1208,25 +1205,45 @@ public class ScriptAnalysisService /// /// 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 System.Threading.Thread bans that exact type - /// while System.Threading (e.g. CancellationToken) stays allowed. + /// name. M3.5: the allowed-exception carve-out is applied at the WHOLE-SYMBOL + /// level (mirroring the shared + /// semantic pass), so a type whose full name is an allowed exception + /// (e.g. System.Threading.CancellationTokenSource) is NOT flagged even + /// though its bare containing namespace System.Threading is a forbidden + /// root. Without the symbol-level check the namespace scope alone would + /// false-positive the allowed cancellation types. /// 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); } - /// Fully-qualified names a symbol reference implicates for trust-model checking. + /// + /// Fully-qualified names a symbol reference implicates for trust-model checking. + /// M3.5: a bare namespace symbol is intentionally ignored — mirroring the shared + /// semantic pass. A namespace name + /// on its own performs no action; harm requires referencing a type or member, so + /// flagging the bare System.Threading qualifier of an otherwise-allowed + /// type (System.Threading.CancellationTokenSource) would be a false + /// positive. The forbidden-using-directive path is checked separately on + /// the directive text, so namespace imports are still caught. + /// private static IEnumerable 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)); + /// + /// M3.5: the forbidden-namespace verdict for the editor walker is sourced + /// from the shared — the single + /// deny-list the run gate () also uses, so + /// the editor squiggle and the run gate agree on which namespaces are + /// forbidden. A name under a + /// root is forbidden UNLESS it is also under a + /// entry + /// (async/await + cancellation tokens survive the System.Threading block). + /// + 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); /// - /// 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 — + /// 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 Process + /// reached through using System.Diagnostics; resolves to + /// System.Diagnostics.Process 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. /// private static IReadOnlyList EnforceTrustModel(Compilation compilation) { var tree = compilation.SyntaxTrees.FirstOrDefault(); if (tree == null) return Array.Empty(); - 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(); + + // 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) diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/ZB.MOM.WW.ScadaBridge.CentralUI.csproj b/src/ZB.MOM.WW.ScadaBridge.CentralUI/ZB.MOM.WW.ScadaBridge.CentralUI.csproj index 0632b677..49d53880 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/ZB.MOM.WW.ScadaBridge.CentralUI.csproj +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/ZB.MOM.WW.ScadaBridge.CentralUI.csproj @@ -23,6 +23,7 @@ + diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs index 798b4a90..4b7500d3 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs @@ -54,11 +54,14 @@ public class ScriptAnalysisServiceTests } [Theory] - [InlineData("System.Diagnostics")] [InlineData("System.Reflection")] [InlineData("System.Net")] + [InlineData("System.Threading")] public void ForbiddenUsing_AllBannedNamespaces(string ns) { + // M3.5: the editor deny-list is sourced from the shared ScriptTrustPolicy. + // System.Threading is now a forbidden root (only Tasks + + // CancellationToken(Source) are excepted). var resp = _svc.Diagnose(new DiagnoseRequest($"using {ns};")); Assert.Contains(resp.Markers, m => m.Code == "SCADA001" && m.Message.Contains(ns)); } @@ -71,6 +74,61 @@ public class ScriptAnalysisServiceTests Assert.Contains(resp.Markers, m => m.Code == "SCADA002" && m.Message.Contains("File")); } + [Fact] + public void SystemDiagnosticsNamespace_NoLongerWholesaleForbidden() + { + // M3.5: the unified policy forbids only System.Diagnostics.Process, not the + // whole System.Diagnostics namespace — so a bare `using System.Diagnostics;` + // is no longer flagged (Stopwatch & friends are allowed). + var resp = _svc.Diagnose(new DiagnoseRequest("using System.Diagnostics;")); + Assert.DoesNotContain(resp.Markers, m => m.Code == "SCADA001"); + } + + [Fact] + public void StopwatchUsage_IsAllowedUnderUnifiedPolicy() + { + // System.Diagnostics.Stopwatch is NOT under the forbidden + // System.Diagnostics.Process scope — it must not be flagged. + var resp = _svc.Diagnose(new DiagnoseRequest( + "using System.Diagnostics; var sw = Stopwatch.StartNew(); return sw.ElapsedMilliseconds;")); + Assert.DoesNotContain(resp.Markers, m => m.Code is "SCADA001" or "SCADA002"); + } + + [Fact] + public void ProcessUsage_IsForbiddenUnderUnifiedPolicy() + { + // System.Diagnostics.Process IS the forbidden scope — flagged via the + // semantic marker even though the parent namespace is allowed. + var resp = _svc.Diagnose(new DiagnoseRequest( + "using System.Diagnostics; var p = Process.GetCurrentProcess(); return p.Id;")); + Assert.Contains(resp.Markers, m => m.Code == "SCADA002" && m.Message.Contains("Process")); + } + + [Theory] + [InlineData("Monitor")] + [InlineData("Interlocked")] + [InlineData("Mutex")] + public void ThreadingPrimitives_NowFlagged_UnderUnifiedPolicy(string typeName) + { + // CentralUI's old deny-list allowed most of System.Threading (only Thread + + // Tasks.Sources were blocked). The unified policy forbids ALL of + // System.Threading except Tasks + CancellationToken(Source), so these + // previously-clean primitives are now flagged by the editor. + var resp = _svc.Diagnose(new DiagnoseRequest( + $"using System.Threading; var x = typeof({typeName}); return 1;")); + Assert.Contains(resp.Markers, m => m.Code is "SCADA001" or "SCADA002"); + } + + [Fact] + public void CancellationToken_StaysAllowed_UnderUnifiedPolicy() + { + // The AllowedExceptions carve-out keeps CancellationToken(Source) clean even + // though the rest of System.Threading is forbidden. + var resp = _svc.Diagnose(new DiagnoseRequest( + "var cts = new System.Threading.CancellationTokenSource(); return cts.Token.IsCancellationRequested;")); + Assert.DoesNotContain(resp.Markers, m => m.Code is "SCADA001" or "SCADA002"); + } + [Fact] public void UserIdentifierNamedFile_DoesNotFalsePositive() {