fix(scriptanalysis): memoise ScriptTrustValidator verdicts by code hash for the default reference set (review 2026-10-21 04/P3)
This commit is contained in:
@@ -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.
|
||||
|
||||
---
|
||||
|
||||
@@ -68,11 +68,26 @@ public static class ScriptTrustValidator
|
||||
/// forwards its full compilation reference surface here precisely because
|
||||
/// doing so is safe.
|
||||
/// </param>
|
||||
/// <returns>A list of trust-model violation messages; empty if the script is clean.</returns>
|
||||
/// <returns>
|
||||
/// A read-only list of trust-model violation messages; empty if the script is clean.
|
||||
/// When <paramref name="extraReferences"/> 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.
|
||||
/// </returns>
|
||||
public static IReadOnlyList<string> FindViolations(
|
||||
string code,
|
||||
IEnumerable<MetadataReference>? extraReferences = null)
|
||||
=> FindViolations(code, ScriptTrustPolicy.AnalysisReferences, extraReferences);
|
||||
{
|
||||
var extras = extraReferences as IReadOnlyCollection<MetadataReference> ?? 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));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Overload that runs the trust analysis against an explicit base reference
|
||||
|
||||
@@ -0,0 +1,85 @@
|
||||
using System.Collections.Concurrent;
|
||||
using System.Security.Cryptography;
|
||||
using System.Text;
|
||||
|
||||
namespace ZB.MOM.WW.ScadaBridge.ScriptAnalysis;
|
||||
|
||||
/// <summary>
|
||||
/// Process-wide memo of <see cref="ScriptTrustValidator"/> verdicts for the
|
||||
/// default policy reference set, keyed on the SHA-256 of the script code (review
|
||||
/// 2026-10-21 04/P3).
|
||||
///
|
||||
/// <para>
|
||||
/// A verdict is a pure function of (code, <see cref="ScriptTrustPolicy"/>,
|
||||
/// <see cref="ScriptTrustPolicy.AnalysisReferences"/>), 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.
|
||||
/// </para>
|
||||
///
|
||||
/// <para>
|
||||
/// Bounded like the Template Engine's compile-verdict cache: two generations of
|
||||
/// <see cref="SegmentCapacity"/> 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.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
internal static class TrustVerdictCache
|
||||
{
|
||||
/// <summary>Upper bound on entries per generation (2 × this in total).</summary>
|
||||
internal const int SegmentCapacity = 1024;
|
||||
|
||||
private static readonly object RotateGate = new();
|
||||
private static volatile ConcurrentDictionary<string, IReadOnlyList<string>> _hot = new();
|
||||
private static volatile ConcurrentDictionary<string, IReadOnlyList<string>> _cold = new();
|
||||
|
||||
/// <summary>
|
||||
/// Returns the cached verdict for <paramref name="code"/>, or computes it with
|
||||
/// <paramref name="analyse"/> and caches it on a miss.
|
||||
/// </summary>
|
||||
/// <param name="code">The script source.</param>
|
||||
/// <param name="analyse">Computes the verdict on a miss; a throw is not cached.</param>
|
||||
/// <returns>The (read-only) violation list.</returns>
|
||||
internal static IReadOnlyList<string> GetOrAdd(string code, Func<IReadOnlyList<string>> 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<string> verdict)
|
||||
{
|
||||
if (_hot.Count >= SegmentCapacity)
|
||||
{
|
||||
lock (RotateGate)
|
||||
{
|
||||
if (_hot.Count >= SegmentCapacity)
|
||||
{
|
||||
_cold = _hot;
|
||||
_hot = new ConcurrentDictionary<string, IReadOnlyList<string>>();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// 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);
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,66 @@
|
||||
using ZB.MOM.WW.ScadaBridge.ScriptAnalysis;
|
||||
|
||||
namespace ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Review 2026-10-21 04/P3: <see cref="ScriptTrustValidator.FindViolations(string, IEnumerable{Microsoft.CodeAnalysis.MetadataReference})"/>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
public class ScriptTrustValidatorCacheTests
|
||||
{
|
||||
/// <summary>Verifies that the same violating code analysed twice is served from the cache.</summary>
|
||||
[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
|
||||
}
|
||||
|
||||
/// <summary>Verifies that a clean verdict is cached too.</summary>
|
||||
[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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
[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);
|
||||
}
|
||||
|
||||
/// <summary>Verifies that the shared verdict cannot be mutated by a caller to poison the cache.</summary>
|
||||
[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<string>);
|
||||
Assert.True(verdict is System.Collections.Generic.ICollection<string> { IsReadOnly: true });
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user