fix(template-engine): verdict-cache key gains the globals-surface discriminator — cross-surface verdict reuse structurally impossible (plan R2-05 T2)

This commit is contained in:
Joseph Doherty
2026-07-13 09:45:54 -04:00
parent 5573a9eeb9
commit 06d79f6516
3 changed files with 41 additions and 11 deletions
@@ -5,14 +5,25 @@ using System.Text;
namespace ZB.MOM.WW.ScadaBridge.TemplateEngine.Validation;
/// <summary>
/// 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 <c>ScriptTrustPolicy</c> 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 <c>ScriptTrustPolicy</c> 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.
///
/// <para>
/// The globals surface is part of the key because a verdict is NOT interchangeable
/// across surfaces: a trigger expression that compiles clean against
/// <c>TriggerCompileSurface</c> is not vetted for a <c>ScriptCompileSurface</c>
/// 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.
/// </para>
///
/// <para>
/// The stored error is deliberately <em>name-free</em> — 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;
/// <summary>
/// Returns the cached verdict for <paramref name="code"/>, or computes it via
/// <paramref name="factory"/> and caches it on a miss. A hit increments
/// <see cref="Hits"/>. 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 (<paramref name="surface"/>,
/// <paramref name="code"/>), or computes it via <paramref name="factory"/> and
/// caches it on a miss. A hit increments <see cref="Hits"/>. The verdict's error
/// text must be name-free — the caller formats the script name in on return.
/// </summary>
/// <param name="surface">
/// The globals-surface discriminator (e.g. <c>nameof(ScriptCompileSurface)</c> or
/// <c>nameof(TriggerCompileSurface)</c>). Part of the key so a verdict computed
/// against one surface is never reused for another.
/// </param>
/// <param name="code">The script source code to look up (hashed to form the cache key).</param>
/// <param name="factory">Computes the verdict on a cache miss.</param>
/// <returns>The cached or newly computed compile verdict.</returns>
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))
{
@@ -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
@@ -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()
{