diff --git a/docs/requirements/Component-ScriptAnalysis.md b/docs/requirements/Component-ScriptAnalysis.md index 9746fa67..5b29e381 100644 --- a/docs/requirements/Component-ScriptAnalysis.md +++ b/docs/requirements/Component-ScriptAnalysis.md @@ -214,6 +214,8 @@ All five call sites that previously maintained their own script trust enforcemen | **Central UI** `ScriptAnalysisService` | Semantic + full compile, lenient threading | Delegates forbidden-API verdict and sources editor-marker deny-list from `ScriptTrustPolicy`; retains Test-Run execution host | | **Transport** `BundleImporter.RunSemanticValidationAsync` / `DetectBlockersAsync` | No trust gate — a bundle's forbidden-API scripts imported clean, deferred to runtime (fifth write path) | Runs `ScriptTrustValidator.FindViolations` over every non-Skip template / shared / ApiMethod body, template script + alarm Expression-trigger bodies, **and instance alarm-override trigger expressions** at import review — hard error (apply) / Blocker row (preview) for all kinds | +**Trust verdicts are memoised (review 2026-10-21 04/P3).** `ScriptTrustValidator.FindViolations(code)` — the default-reference-set entry point every consumer above uses — caches its verdict process-wide by SHA-256 of the code (`TrustVerdictCache`: two generations of 1 024, cold-hit promotion, a throwing analysis is never stored; verdicts are returned as read-only collections and a hit returns the same instance). The verdict is a pure function of the code because the policy and `AnalysisReferences` are process-static. This removes the repeat full-framework semantic analysis the Transport importer used to pay on preview **and** again on apply (and on every wizard re-entry to preview) with no change at its call sites. A call that passes non-empty `extraReferences` (Central UI's run gate) bypasses the cache, since extra references can change resolution; the explicit-`baseReferences` overload is never cached. + The static enforcement is **defence-in-depth**, not a true runtime sandbox. The denied API list blocks disallowed APIs at compile time but does not establish an operating-system security boundary. Site-runtime consumers execute in-process. Central UI Test Run uses a bounded, killable worker process; that adds resource/lifetime containment, while the worker retains the same operating-system identity and permitted gateway access. --- diff --git a/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptTrustValidator.cs b/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptTrustValidator.cs index 3445702f..a8e917ac 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptTrustValidator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptTrustValidator.cs @@ -68,11 +68,26 @@ public static class ScriptTrustValidator /// forwards its full compilation reference surface here precisely because /// doing so is safe. /// - /// A list of trust-model violation messages; empty if the script is clean. + /// + /// A read-only list of trust-model violation messages; empty if the script is clean. + /// When is null or empty the verdict is memoised + /// process-wide by code hash (a repeat call returns the same instance); a call with + /// extra references always analyses afresh, since they can change resolution. + /// public static IReadOnlyList FindViolations( string code, IEnumerable? extraReferences = null) - => FindViolations(code, ScriptTrustPolicy.AnalysisReferences, extraReferences); + { + var extras = extraReferences as IReadOnlyCollection ?? extraReferences?.ToList(); + if (extras is { Count: > 0 } || string.IsNullOrWhiteSpace(code)) + return FindViolations(code, ScriptTrustPolicy.AnalysisReferences, extras); + + // Review 2026-10-21 04/P3: the default-reference verdict is a pure function of + // the code, so memoise it — the importer's preview and apply passes otherwise + // re-run a full-framework semantic analysis for every script, every time. + return TrustVerdictCache.GetOrAdd(code, + () => FindViolations(code, ScriptTrustPolicy.AnalysisReferences, extraReferences: null)); + } /// /// Overload that runs the trust analysis against an explicit base reference diff --git a/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/TrustVerdictCache.cs b/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/TrustVerdictCache.cs new file mode 100644 index 00000000..cc1e5c11 --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/TrustVerdictCache.cs @@ -0,0 +1,85 @@ +using System.Collections.Concurrent; +using System.Security.Cryptography; +using System.Text; + +namespace ZB.MOM.WW.ScadaBridge.ScriptAnalysis; + +/// +/// Process-wide memo of verdicts for the +/// default policy reference set, keyed on the SHA-256 of the script code (review +/// 2026-10-21 04/P3). +/// +/// +/// A verdict is a pure function of (code, , +/// ), and the policy and +/// reference set are process-static, so the code hash alone identifies it. Each +/// miss is a full-framework semantic analysis (~170 references) — the bundle +/// importer runs the same script set through it on preview and again on apply, +/// and the wizard re-enters preview — so memoising here removes that repeat cost +/// without any change at the call sites. +/// +/// +/// +/// Bounded like the Template Engine's compile-verdict cache: two generations of +/// entries; a hit in the cold generation is promoted, +/// and overflow drops only the cold half nothing touched for a whole generation. +/// Only completed verdicts are stored — an analysis that throws leaves the slot +/// empty. Stored verdicts are read-only collections, so a caller cannot mutate a +/// shared entry. +/// +/// +internal static class TrustVerdictCache +{ + /// Upper bound on entries per generation (2 × this in total). + internal const int SegmentCapacity = 1024; + + private static readonly object RotateGate = new(); + private static volatile ConcurrentDictionary> _hot = new(); + private static volatile ConcurrentDictionary> _cold = new(); + + /// + /// Returns the cached verdict for , or computes it with + /// and caches it on a miss. + /// + /// The script source. + /// Computes the verdict on a miss; a throw is not cached. + /// The (read-only) violation list. + internal static IReadOnlyList GetOrAdd(string code, Func> analyse) + { + var key = Convert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(code))); + + var hot = _hot; + var cold = _cold; + if (hot.TryGetValue(key, out var verdict)) + return verdict; + + if (cold.TryGetValue(key, out verdict)) + { + Store(key, verdict); + return verdict; + } + + verdict = analyse().ToList().AsReadOnly(); + Store(key, verdict); + return verdict; + } + + private static void Store(string key, IReadOnlyList verdict) + { + if (_hot.Count >= SegmentCapacity) + { + lock (RotateGate) + { + if (_hot.Count >= SegmentCapacity) + { + _cold = _hot; + _hot = new ConcurrentDictionary>(); + } + } + } + + // TryAdd, not an indexer write: a racing thread that computed the same verdict + // keeps the instance already published, so every reader shares one object. + _hot.TryAdd(key, verdict); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests/ScriptTrustValidatorCacheTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests/ScriptTrustValidatorCacheTests.cs new file mode 100644 index 00000000..9d374e20 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests/ScriptTrustValidatorCacheTests.cs @@ -0,0 +1,66 @@ +using ZB.MOM.WW.ScadaBridge.ScriptAnalysis; + +namespace ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests; + +/// +/// Review 2026-10-21 04/P3: +/// is memoised by code hash for the default (policy) reference set, so the bundle +/// importer's preview and apply passes — which call it with no change at their call +/// sites — do not redo a full-framework semantic analysis per script. A cache hit +/// returns the very same verdict instance, which is what these tests observe. +/// +public class ScriptTrustValidatorCacheTests +{ + /// Verifies that the same violating code analysed twice is served from the cache. + [Fact] + public void FindViolations_SameCodeTwice_SecondCallIsCacheHit() + { + var code = $"var x = System.IO.File.Exists(\"{Guid.NewGuid()}\");"; + var first = ScriptTrustValidator.FindViolations(code); + var second = ScriptTrustValidator.FindViolations(code); + + Assert.NotEmpty(second); + Assert.Same(first, second); // pre-fix: a fresh list per call + } + + /// Verifies that a clean verdict is cached too. + [Fact] + public void FindViolations_CleanCodeTwice_SecondCallIsCacheHit() + { + var code = $"var x = \"{Guid.NewGuid()}\".Length;"; + var first = ScriptTrustValidator.FindViolations(code); + var second = ScriptTrustValidator.FindViolations(code); + + Assert.Empty(second); + Assert.Same(first, second); + } + + /// + /// Verifies that a call with extra references is never served from (or stored in) + /// the cache — extra references can change symbol resolution, so the verdict is a + /// pure function of the code only for the default reference set. + /// + [Fact] + public void FindViolations_WithExtraReferences_BypassesCache() + { + var code = $"var x = System.IO.File.Exists(\"{Guid.NewGuid()}\");"; + var extra = new[] { Microsoft.CodeAnalysis.MetadataReference.CreateFromFile(typeof(ScriptCompileSurface).Assembly.Location) }; + var a = ScriptTrustValidator.FindViolations(code, extra); + var b = ScriptTrustValidator.FindViolations(code, extra); + var cached = ScriptTrustValidator.FindViolations(code); + + Assert.NotSame(a, b); + Assert.NotSame(a, cached); + Assert.NotSame(b, cached); + } + + /// Verifies that the shared verdict cannot be mutated by a caller to poison the cache. + [Fact] + public void FindViolations_ReturnedList_IsReadOnly() + { + var code = $"var x = System.IO.File.Exists(\"{Guid.NewGuid()}\");"; + var verdict = ScriptTrustValidator.FindViolations(code); + Assert.False(verdict is List); + Assert.True(verdict is System.Collections.Generic.ICollection { IsReadOnly: true }); + } +}