diff --git a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompileVerdictCache.cs b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompileVerdictCache.cs index 794fe3fc..f1b7a599 100644 --- a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompileVerdictCache.cs +++ b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompileVerdictCache.cs @@ -5,14 +5,25 @@ using System.Text; namespace ZB.MOM.WW.ScadaBridge.TemplateEngine.Validation; /// -/// Process-wide cache of script compile verdicts, keyed on a SHA-256 hash of the -/// script code. The verdict (clean/compiles, or the name-free error text) is a pure -/// function of the code plus the compile-time-static ScriptTrustPolicy and -/// globals surface, so it is sound to memoise it across the whole process: an +/// Process-wide cache of script compile verdicts, keyed on the pair +/// (globals surface, SHA-256 hash of the script code). The verdict (clean/compiles, +/// or the name-free error text) is a pure function of the code plus the +/// compile-time-static ScriptTrustPolicy AND the globals surface it was +/// compiled against, so it is sound to memoise it across the whole process: an /// unchanged script runs the forbidden-API analysis and the Roslyn compile exactly /// once, removing both the repeat CPU cost and the repeat compiled-assembly load. /// /// +/// The globals surface is part of the key because a verdict is NOT interchangeable +/// across surfaces: a trigger expression that compiles clean against +/// TriggerCompileSurface is not vetted for a ScriptCompileSurface +/// script body (different globals resolve different identifiers), so a code-only key +/// could return a stale "clean" for code never compiled against the caller's surface. +/// Keeping the surface in the key makes that cross-surface reuse structurally +/// impossible even as more compile surfaces gain cache entries. +/// +/// +/// /// The stored error is deliberately name-free — the caller's script name is /// formatted into the returned message at read time, so two callers submitting the /// same bad code each see their own name in the failure text while sharing one verdict. @@ -40,17 +51,22 @@ public static class ScriptCompileVerdictCache public static int Count => Cache.Count; /// - /// Returns the cached verdict for , or computes it via - /// and caches it on a miss. A hit increments - /// . The verdict's error text must be name-free — the caller - /// formats the script name in on return. + /// Returns the cached verdict for the pair (, + /// ), or computes it via and + /// caches it on a miss. A hit increments . The verdict's error + /// text must be name-free — the caller formats the script name in on return. /// + /// + /// The globals-surface discriminator (e.g. nameof(ScriptCompileSurface) or + /// nameof(TriggerCompileSurface)). Part of the key so a verdict computed + /// against one surface is never reused for another. + /// /// The script source code to look up (hashed to form the cache key). /// Computes the verdict on a cache miss. /// The cached or newly computed compile verdict. - public static (bool Ok, string? Error) GetOrAdd(string code, Func<(bool Ok, string? Error)> factory) + public static (bool Ok, string? Error) GetOrAdd(string surface, string code, Func<(bool Ok, string? Error)> factory) { - var key = Convert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(code))); + var key = surface + ":" + Convert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(code))); if (Cache.TryGetValue(key, out var verdict)) { diff --git a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompiler.cs b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompiler.cs index 80246ba4..5f23f0e0 100644 --- a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompiler.cs +++ b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompiler.cs @@ -39,7 +39,7 @@ public class ScriptCompiler // globals surface are compile-time static), so memoise it process-wide: an // unchanged script compiles exactly once per process. The cached error is // name-free — the script name is formatted in below at read time. - var (ok, error) = ScriptCompileVerdictCache.GetOrAdd(code, () => + var (ok, error) = ScriptCompileVerdictCache.GetOrAdd(nameof(ScriptCompileSurface), code, () => { // Authoritative forbidden-API verdict first — a security violation must // gate the script regardless of whether it otherwise compiles. Report diff --git a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompilerTests.cs b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompilerTests.cs index 1bf79106..1f4977c4 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompilerTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompilerTests.cs @@ -182,6 +182,20 @@ public class ScriptCompilerTests Assert.Equal(hitsBefore + 1, ScriptCompileVerdictCache.Hits); } + [Fact] + public void GetOrAdd_SameCodeDifferentSurface_ComputesSeparateVerdicts() + { + ScriptCompileVerdictCache.Clear(); + var a = ScriptCompileVerdictCache.GetOrAdd("SurfaceA", "return 1;", () => (true, null)); + var hitsAfterA = ScriptCompileVerdictCache.Hits; + var b = ScriptCompileVerdictCache.GetOrAdd("SurfaceB", "return 1;", () => (false, "err")); + + Assert.True(a.Ok); + Assert.False(b.Ok); // code-only key would return SurfaceA's verdict + Assert.Equal(hitsAfterA, ScriptCompileVerdictCache.Hits); // no false cross-surface hit + Assert.Equal(2, ScriptCompileVerdictCache.Count); + } + [Fact] public void TryCompile_CachedFailure_FormatsNameForEachCaller() {