From 5573a9eeb95ac8b13f474fd763c47f3a40f5f1d5 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 09:45:04 -0400 Subject: [PATCH 1/9] fix(template-engine): trigger-expression syntax check reports ALL violations/compile errors, not just the first (plan R2-05 T1) --- .../Validation/ValidationService.cs | 8 +++++--- .../Validation/ScriptCompilerTests.cs | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs index c29b3053..48ee0706 100644 --- a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs @@ -530,15 +530,17 @@ public class ValidationService /// A human-readable error message if the expression is invalid; null if well-formed. internal static string? CheckExpressionSyntax(string expression) { - // Authoritative forbidden-API verdict first. + // Authoritative forbidden-API verdict first. Report ALL violations, not just + // the first, so an operator fixing one forbidden API isn't surprised by a + // second on the next deploy attempt (mirrors ScriptCompiler.TryCompile). var violations = ScriptTrustValidator.FindViolations(expression); if (violations.Count > 0) - return $"uses forbidden API: {violations[0]}"; + return $"uses forbidden API: {string.Join("; ", violations)}"; // Real compile of the bare boolean expression against the trigger globals. var errors = RoslynScriptCompiler.Compile(expression, typeof(TriggerCompileSurface)); if (errors.Count > 0) - return $"is not a valid expression: {errors[0]}"; + return $"is not a valid expression: {string.Join("; ", errors)}"; return null; } 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 17df857d..1bf79106 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompilerTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompilerTests.cs @@ -149,6 +149,25 @@ public class ScriptCompilerTests Assert.Contains("forbidden", error, StringComparison.OrdinalIgnoreCase); } + [Fact] + public void CheckExpressionSyntax_MultipleForbiddenApis_ReportsAll() + { + var error = ValidationService.CheckExpressionSyntax( + "System.IO.File.Exists(\"x\") && System.Diagnostics.Process.GetProcesses().Length > 0"); + Assert.NotNull(error); + Assert.Contains("System.IO", error); // FAILS today: only violations[0] is surfaced + Assert.Contains("Process", error); + } + + [Fact] + public void CheckExpressionSyntax_MultipleCompileErrors_ReportsAll() + { + var error = ValidationService.CheckExpressionSyntax("NoSuchThingA > 1 && NoSuchThingB < 2"); + Assert.NotNull(error); + Assert.Contains("NoSuchThingA", error); // FAILS today: only errors[0] is surfaced + Assert.Contains("NoSuchThingB", error); + } + // --- Compile-verdict cache: unchanged code compiles once per process --- [Fact] From 06d79f6516e040a6e87588fa04ef8429858308a5 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 09:45:54 -0400 Subject: [PATCH 2/9] =?UTF-8?q?fix(template-engine):=20verdict-cache=20key?= =?UTF-8?q?=20gains=20the=20globals-surface=20discriminator=20=E2=80=94=20?= =?UTF-8?q?cross-surface=20verdict=20reuse=20structurally=20impossible=20(?= =?UTF-8?q?plan=20R2-05=20T2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Validation/ScriptCompileVerdictCache.cs | 36 +++++++++++++------ .../Validation/ScriptCompiler.cs | 2 +- .../Validation/ScriptCompilerTests.cs | 14 ++++++++ 3 files changed, 41 insertions(+), 11 deletions(-) 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() { From d233ecbe8fc44ac7bd5131be1cfce97cbf75c83e Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 09:46:36 -0400 Subject: [PATCH 3/9] =?UTF-8?q?perf(template-engine):=20cache=20Expression?= =?UTF-8?q?-trigger=20compile=20verdicts=20under=20the=20trigger-surface?= =?UTF-8?q?=20key=20=E2=80=94=20unchanged=20expressions=20compile=20once?= =?UTF-8?q?=20per=20process=20(plan=20R2-05=20T3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Validation/ValidationService.cs | 30 ++++++++++++------- .../Validation/ScriptCompilerTests.cs | 23 ++++++++++++++ 2 files changed, 42 insertions(+), 11 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs index 48ee0706..8b8db824 100644 --- a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs @@ -530,19 +530,27 @@ public class ValidationService /// A human-readable error message if the expression is invalid; null if well-formed. internal static string? CheckExpressionSyntax(string expression) { - // Authoritative forbidden-API verdict first. Report ALL violations, not just - // the first, so an operator fixing one forbidden API isn't surprised by a - // second on the next deploy attempt (mirrors ScriptCompiler.TryCompile). - var violations = ScriptTrustValidator.FindViolations(expression); - if (violations.Count > 0) - return $"uses forbidden API: {string.Join("; ", violations)}"; + // Memoise the verdict under the TRIGGER surface key (see ScriptCompileVerdictCache): + // the verdict is a pure function of expression + policy + TriggerCompileSurface, and + // it is the exact hot-path cost N1 flagged — every staleness sweep / import probe of + // an Expression-triggered config re-ran a full trust compilation + script compile. + var (_, error) = ScriptCompileVerdictCache.GetOrAdd(nameof(TriggerCompileSurface), expression, () => + { + // Authoritative forbidden-API verdict first. Report ALL violations, not + // just the first, so an operator fixing one forbidden API isn't surprised + // by a second on the next deploy attempt (mirrors ScriptCompiler.TryCompile). + var violations = ScriptTrustValidator.FindViolations(expression); + if (violations.Count > 0) + return (false, $"uses forbidden API: {string.Join("; ", violations)}"); - // Real compile of the bare boolean expression against the trigger globals. - var errors = RoslynScriptCompiler.Compile(expression, typeof(TriggerCompileSurface)); - if (errors.Count > 0) - return $"is not a valid expression: {string.Join("; ", errors)}"; + // Real compile of the bare boolean expression against the trigger globals. + var errors = RoslynScriptCompiler.Compile(expression, typeof(TriggerCompileSurface)); + if (errors.Count > 0) + return (false, $"is not a valid expression: {string.Join("; ", errors)}"); - return null; + return (true, (string?)null); + }); + return error; } /// 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 1f4977c4..1c0309ab 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompilerTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompilerTests.cs @@ -149,6 +149,29 @@ public class ScriptCompilerTests Assert.Contains("forbidden", error, StringComparison.OrdinalIgnoreCase); } + [Fact] + public void CheckExpressionSyntax_SameExpressionTwice_SecondCallIsCacheHit() + { + ScriptCompileVerdictCache.Clear(); + ValidationService.CheckExpressionSyntax("Attributes[\"Temp\"] != null"); + var hitsBefore = ScriptCompileVerdictCache.Hits; + var error = ValidationService.CheckExpressionSyntax("Attributes[\"Temp\"] != null"); + Assert.Null(error); + Assert.Equal(hitsBefore + 1, ScriptCompileVerdictCache.Hits); // FAILS today: no cache use + } + + [Fact] + public void CheckExpressionSyntax_ScriptSurfaceVerdict_NotReusedForTriggerSurface() + { + // "Notify != null" resolves on ScriptCompileSurface (Notify is a script global, + // ScriptCompileSurface.cs:53) but NOT on TriggerCompileSurface — a shared + // code-only cache entry would wrongly report the trigger expression clean. + ScriptCompileVerdictCache.Clear(); + Assert.True(new ScriptCompiler().TryCompile("Notify != null", "S").IsSuccess); // warms the SCRIPT-surface entry + var error = ValidationService.CheckExpressionSyntax("Notify != null"); + Assert.NotNull(error); // green today (no cache), and MUST STAY green after T3 — the T2 key makes it structural + } + [Fact] public void CheckExpressionSyntax_MultipleForbiddenApis_ReportsAll() { From 31fa5eafa488ee6a0af9491445afbcddabbab9d2 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 09:48:40 -0400 Subject: [PATCH 4/9] =?UTF-8?q?perf(template-engine):=20read-only=20stalen?= =?UTF-8?q?ess/comparison=20paths=20skip=20Expression-trigger=20compiles?= =?UTF-8?q?=20=E2=80=94=20deploy=20gate=20remains=20the=20authoritative=20?= =?UTF-8?q?check=20(plan=20R2-05=20T4)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Validation/ValidationService.cs | 52 +++++++++++++------ .../Validation/ValidationServiceTests.cs | 39 ++++++++++++++ 2 files changed, 76 insertions(+), 15 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs index 8b8db824..1f1d5502 100644 --- a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs @@ -97,11 +97,16 @@ public class ValidationService /// /// /// true (default) runs the authoritative Roslyn - /// stage — the single most expensive check (a non-collectible assembly load per script). - /// false skips ONLY that stage; every other structural/semantic check still runs. - /// Read-only staleness/comparison callers (DeploymentManager's FlatteningPipeline on its - /// comparison/probe paths) pass false because they need only the flattened config and - /// revision hash, not a compile. The deploy gate keeps the default (true). + /// stage — the single most expensive check (a non-collectible assembly load per script) — + /// AND the Expression-trigger syntax/compile check + /// (, a forbidden-API verdict plus a real compile of every + /// Expression-triggered script/alarm body against ). + /// false skips ONLY those two compile stages; every other structural/semantic check + /// still runs — including, for Expression triggers, the blank-expression check and the + /// cheap attribute-reference scan. Read-only staleness/comparison callers (DeploymentManager's + /// FlatteningPipeline on its comparison/probe paths) pass false because they need only + /// the flattened config and revision hash, not a compile. The deploy gate keeps the default + /// (true) — it is the authoritative check for both stages. /// /// A merged aggregating all pipeline stage outcomes. public ValidationResult Validate( @@ -128,7 +133,7 @@ public class ValidationService validateScriptCompilation ? ValidateScriptCompilation(configuration) : ValidationResult.Success(), ValidateAlarmTriggerReferences(configuration), ValidateScriptTriggerReferences(configuration), - ValidateExpressionTriggers(configuration), + ValidateExpressionTriggers(configuration, validateScriptCompilation), ValidateConnectionBindingCompleteness(configuration, enforceConnectionBindings, siteConnectionNames), ValidateSchemaReferences(configuration, sharedScripts, resolveSchemaRef), _semanticValidator.Validate(configuration, sharedScripts, alarmCapableConnectionNames) @@ -366,8 +371,16 @@ public class ValidationService /// /// /// The flattened configuration to validate. + /// + /// true (default) runs the authoritative + /// forbidden-API + compile stage for each Expression trigger. false skips ONLY + /// that stage — the blank-expression check and the attribute-reference scan still run — + /// so read-only staleness/comparison callers do not pay the per-expression compile. + /// /// A with errors and warnings from all expression trigger checks. - public static ValidationResult ValidateExpressionTriggers(FlattenedConfiguration configuration) + public static ValidationResult ValidateExpressionTriggers( + FlattenedConfiguration configuration, + bool validateExpressionSyntax = true) { var errors = new List(); var warnings = new List(); @@ -382,7 +395,7 @@ public class ValidationService CheckExpressionTrigger( ValidationCategory.ScriptTriggerReference, "script", script.CanonicalName, script.TriggerConfiguration, - attributeNames, errors, warnings); + attributeNames, errors, warnings, validateExpressionSyntax); } foreach (var alarm in configuration.Alarms) @@ -393,7 +406,7 @@ public class ValidationService CheckExpressionTrigger( ValidationCategory.AlarmTriggerReference, "alarm", alarm.CanonicalName, alarm.TriggerConfiguration, - attributeNames, errors, warnings); + attributeNames, errors, warnings, validateExpressionSyntax); } return new ValidationResult { Errors = errors, Warnings = warnings }; @@ -424,7 +437,8 @@ public class ValidationService string? triggerConfigJson, HashSet attributeNames, List errors, - List warnings) + List warnings, + bool validateExpressionSyntax = true) { var expression = ExtractExpressionFromTriggerConfig(triggerConfigJson); var strict = IsStrictAnalysis(triggerConfigJson); @@ -443,12 +457,20 @@ public class ValidationService return; } - var syntaxError = CheckExpressionSyntax(expression); - if (syntaxError != null) + // The syntax/compile stage is the expensive one (a forbidden-API verdict + a + // real Roslyn compile of the expression). Read-only staleness/comparison + // callers pass validateExpressionSyntax: false to skip ONLY this stage; the + // blank-expression check above and the attribute-reference scan below still run. + // The deploy gate keeps the default (true) — the authoritative check. + if (validateExpressionSyntax) { - errors.Add(ValidationEntry.Error(category, - $"The {entityLabel} '{entityName}' expression trigger failed validation: {syntaxError}", - entityName)); + var syntaxError = CheckExpressionSyntax(expression); + if (syntaxError != null) + { + errors.Add(ValidationEntry.Error(category, + $"The {entityLabel} '{entityName}' expression trigger failed validation: {syntaxError}", + entityName)); + } } foreach (var attrName in ExtractAttributeReferences(expression)) diff --git a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ValidationServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ValidationServiceTests.cs index 4ce13f9b..bdd4719c 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ValidationServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ValidationServiceTests.cs @@ -462,4 +462,43 @@ public class ValidationServiceTests Assert.DoesNotContain(strictResult.Errors, e => e.Category == ValidationCategory.AlarmTriggerReference); Assert.DoesNotContain(strictResult.Warnings, w => w.Category == ValidationCategory.AlarmTriggerReference); } + + [Fact] + public void Validate_SkipCompilation_SkipsExpressionTriggerSyntaxCheck() + { + var config = ConfigWithExpressionTriggerScript("this is not C# (("); + var gated = new ValidationService().Validate(config, validateScriptCompilation: false); + Assert.DoesNotContain(gated.Errors, e => e.Message.Contains("failed validation")); // FAILS today + var full = new ValidationService().Validate(config); + Assert.Contains(full.Errors, e => e.Message.Contains("failed validation")); // deploy gate unchanged + } + + [Fact] + public void Validate_SkipCompilation_StillChecksExpressionAttributeReferences() + { + // expression "Attributes[\"Ghost\"] != null" referencing a missing attribute: + // the reference scan is cheap string work and MUST survive the gate. + var config = ConfigWithExpressionTriggerScript("Attributes[\"Ghost\"] != null"); + var gated = new ValidationService().Validate(config, validateScriptCompilation: false); + Assert.Contains(gated.Errors, e => e.Message.Contains("Ghost")); + } + + private static FlattenedConfiguration ConfigWithExpressionTriggerScript(string expression) + { + var json = System.Text.Json.JsonSerializer.Serialize(new { expression }); + return new FlattenedConfiguration + { + InstanceUniqueName = "Instance1", + Attributes = [new ResolvedAttribute { CanonicalName = "Temp", Value = "25", DataType = "Double" }], + Scripts = + [ + new ResolvedScript + { + CanonicalName = "OnExpr", + TriggerType = "Expression", + TriggerConfiguration = json + } + ] + }; + } } From ab77094a260a8dc39a4545d7367c32959e8556b1 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 09:50:32 -0400 Subject: [PATCH 5/9] =?UTF-8?q?fix(transport):=20script-artifact=20change?= =?UTF-8?q?=20publisher=20covers=20Add=20resolutions=20=E2=80=94=20delete-?= =?UTF-8?q?then-reimport=20no=20longer=20starves=20non-self-healing=20subs?= =?UTF-8?q?cribers=20(plan=20R2-05=20T5)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Import/BundleImporter.cs | 17 +++--- .../Import/BundleImporterApplyTests.cs | 55 +++++++++++++++++++ 2 files changed, 65 insertions(+), 7 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs index 50100d3b..99c2868d 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs @@ -1767,12 +1767,15 @@ public sealed class BundleImporter : IBundleImporter /// /// /// Publishes one per script-bearing - /// artifact kind (ApiMethod / SharedScript / Template) whose resolution was an - /// Overwrite or Rename, using the post-resolution (renamed) names. Adds are - /// excluded — nothing is cached under a brand-new name yet. Over-approximation is - /// safe (an Overwrite that actually fell through to Add just triggers a harmless - /// consumer re-check), which matches the bus's advisory, at-least-once contract. - /// Fully guarded: never throws (it runs after the transaction has committed). + /// artifact kind (ApiMethod / SharedScript / Template) for every non-Skip + /// resolution — Overwrite, Rename, AND Add — using the post-resolution (renamed) + /// names. An Add is NOT "nothing cached yet": an artifact deleted on the target and + /// re-imported as Add under the same name can still have a stale + /// compiled-handler/_knownBadMethods entry on any node, so it must notify too. + /// Over-approximation stays safe (the bus is advisory, at-least-once; consumers + /// self-heal), which matches its stated contract. Only Skip resolutions are excluded + /// (nothing was written). Fully guarded: never throws (it runs after the transaction + /// has committed). /// private void PublishScriptArtifactChanges(IReadOnlyList resolutions) { @@ -1799,7 +1802,7 @@ public sealed class BundleImporter : IBundleImporter { var names = all .Where(r => string.Equals(r.EntityType, entityType, StringComparison.Ordinal) - && (r.Action is ResolutionAction.Overwrite or ResolutionAction.Rename)) + && r.Action is not ResolutionAction.Skip) .Select(r => r.Action == ResolutionAction.Rename ? r.RenameTo ?? r.Name : r.Name) .Distinct(StringComparer.Ordinal) .ToList(); diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs index 2be247be..f920ad0c 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs @@ -1397,6 +1397,61 @@ public sealed class BundleImporterApplyTests : IDisposable Assert.All(_artifactBus.Received, n => Assert.Equal("BundleImport", n.Source)); } + [Fact] + public async Task ApplyAsync_publishes_ScriptArtifactsChanged_for_Add_resolutions() + { + // ApiMethod imported as Add into a target where the name does not exist. + // Delete-then-reimport-as-Add is the N3 edge: a node can still hold a + // _knownBadMethods / compiled-handler entry under that very name, so Adds + // must notify too (over-approximation is safe per the bus contract). + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + ctx.ApiMethods.Add(new ApiMethod("DelmiaRecipeDownload", "return 1;") { TimeoutSeconds = 30 }); + await ctx.SaveChangesAsync(); + } + + Guid sessionId; + await using (var scope = _provider.CreateAsyncScope()) + { + var exporter = scope.ServiceProvider.GetRequiredService(); + var ctx = scope.ServiceProvider.GetRequiredService(); + var selection = new ExportSelection( + TemplateIds: Array.Empty(), + SharedScriptIds: Array.Empty(), + ExternalSystemIds: Array.Empty(), + DatabaseConnectionIds: Array.Empty(), + NotificationListIds: Array.Empty(), + SmtpConfigurationIds: Array.Empty(), + ApiMethodIds: await ctx.ApiMethods.Select(m => m.Id).ToListAsync(), + IncludeDependencies: false); + var stream = await exporter.ExportAsync(selection, user: "alice", sourceEnvironment: "dev", + passphrase: null, cancellationToken: CancellationToken.None); + using var ms = new MemoryStream(); + await stream.CopyToAsync(ms); + ms.Position = 0; + + // Delete the target method so the resolution is a genuine Add. + ctx.ApiMethods.RemoveRange(await ctx.ApiMethods.ToListAsync()); + await ctx.SaveChangesAsync(); + + var importer = scope.ServiceProvider.GetRequiredService(); + sessionId = (await importer.LoadAsync(ms, passphrase: null)).SessionId; + } + + await using (var scope = _provider.CreateAsyncScope()) + { + var importer = scope.ServiceProvider.GetRequiredService(); + await importer.ApplyAsync(sessionId, + new List { new("ApiMethod", "DelmiaRecipeDownload", ResolutionAction.Add, null) }, + user: "bob"); + } + + var notification = Assert.Single(_artifactBus.Received, + n => n.ArtifactKind == ScriptArtifactKinds.ApiMethod); + Assert.Contains("DelmiaRecipeDownload", notification.Names); // FAILS today: Adds are filtered out + } + [Fact] public async Task ApplyAsync_failed_apply_publishes_nothing() { From b02c15bbca2455420e4d11d515d4fbd5d9e73a2e Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 09:57:59 -0400 Subject: [PATCH 6/9] =?UTF-8?q?fix(security):=20import=20trust=20gate=20co?= =?UTF-8?q?vers=20instance=20alarm-override=20trigger=20expressions=20?= =?UTF-8?q?=E2=80=94=205th=20call=20site=20is=20surface-complete=20(plan?= =?UTF-8?q?=20R2-05=20T6)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Import/BundleImporter.cs | 46 ++++- .../SemanticValidatorImportTests.cs | 179 ++++++++++++++++++ 2 files changed, 221 insertions(+), 4 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs index 99c2868d..fc5664bf 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs @@ -946,8 +946,11 @@ public sealed class BundleImporter : IBundleImporter /// /// Enumerates every executable C# surface a bundle carries that the /// trust gate must vet: non-Skip template scripts + their Expression-trigger - /// bodies, template alarm Expression-trigger bodies, shared scripts, and - /// ApiMethod scripts. Expression triggers compile and execute at the site + /// bodies, template alarm Expression-trigger bodies, shared scripts, + /// ApiMethod scripts, AND instance alarm-override trigger expressions + /// (InstanceAlarmOverrideDto.TriggerConfigurationOverride). Expression + /// triggers — template alarms AND the instance overrides that replace them in + /// the flattened config — compile and execute at the site /// (`TriggerExpressionGlobals`), so they are a genuine trust surface, not just /// script bodies. A null (preview time, before /// resolutions are chosen) gates everything. Yields (kind, entityName, @@ -998,6 +1001,25 @@ public sealed class BundleImporter : IBundleImporter yield return ("ApiMethod", m.Name, m.Name, m.Script); } } + foreach (var i in content.Instances) + { + if (IsSkipResolution(resolutionMap, "Instance", i.UniqueName)) continue; + foreach (var o in i.AlarmOverrides) + { + // The DTO carries no trigger type (the type lives on the template + // alarm), so gate ANY override config carrying an {"expression":...} + // string body: if the overridden alarm is not Expression-triggered + // the body never executes, but vetting it anyway is fail-safe — the + // structured (HiLo/threshold) configs have no "expression" key, so + // there is no false-positive channel. + var expr = ExtractExpressionBody(o.TriggerConfigurationOverride); + if (!string.IsNullOrEmpty(expr)) + { + yield return ("Instance", i.UniqueName, + $"{o.AlarmCanonicalName} (alarm trigger override expression)", expr); + } + } + } } /// @@ -1009,8 +1031,24 @@ public sealed class BundleImporter : IBundleImporter private static string? ExtractTriggerExpression(string? triggerType, string? triggerConfigJson) { if (triggerType is null - || !triggerType.Equals("Expression", StringComparison.OrdinalIgnoreCase) - || string.IsNullOrWhiteSpace(triggerConfigJson)) + || !triggerType.Equals("Expression", StringComparison.OrdinalIgnoreCase)) + { + return null; + } + return ExtractExpressionBody(triggerConfigJson); + } + + /// + /// Extracts the C# boolean expression from an {"expression":"…"} trigger + /// configuration, WITHOUT a trigger-type guard — used where the caller has no + /// trigger type to check (an instance alarm override; the type lives on the + /// template alarm). A config carrying no "expression" string key (the + /// structured HiLo/threshold shapes) or malformed JSON yields null — nothing to + /// gate. + /// + private static string? ExtractExpressionBody(string? triggerConfigJson) + { + if (string.IsNullOrWhiteSpace(triggerConfigJson)) { return null; } diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/SemanticValidatorImportTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/SemanticValidatorImportTests.cs index 73b9de20..f944d1c2 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/SemanticValidatorImportTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/SemanticValidatorImportTests.cs @@ -2,6 +2,8 @@ using Microsoft.EntityFrameworkCore; using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Instances; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Sites; using ZB.MOM.WW.ScadaBridge.Commons.Entities.Templates; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; @@ -122,6 +124,78 @@ public sealed class SemanticValidatorImportTests : IDisposable return sessionId; } + /// + /// Seeds a template + site + instance whose single alarm override carries the + /// given as its + /// TriggerConfigurationOverride, so an export→load carries the instance + /// alarm-override trigger expression through the trust gate. + /// + private async Task SeedInstanceAlarmOverrideAsync(string triggerConfigJson) + { + await using var scope = _provider.CreateAsyncScope(); + var ctx = scope.ServiceProvider.GetRequiredService(); + + var template = new Template("Pump") { Description = "pump tpl" }; + ctx.Templates.Add(template); + + var site = new Site("Plant 1", "plant-1") + { + NodeAAddress = "akka://site@10.0.0.1:2552", + NodeBAddress = "akka://site@10.0.0.2:2552", + GrpcNodeAAddress = "10.0.0.1:8083", + GrpcNodeBAddress = "10.0.0.2:8083", + }; + ctx.Sites.Add(site); + await ctx.SaveChangesAsync(); + + var instance = new Instance("Pump-01") + { + TemplateId = template.Id, + SiteId = site.Id, + State = InstanceState.Enabled, + }; + instance.AlarmOverrides.Add(new InstanceAlarmOverride("HiAlarm") + { + TriggerConfigurationOverride = triggerConfigJson, + }); + ctx.Instances.Add(instance); + await ctx.SaveChangesAsync(); + } + + /// Exports every seeded site (+ template) closure into a bundle and loads it. + private async Task ExportSitesAndLoadAsync() + { + byte[] bundleBytes; + await using (var scope = _provider.CreateAsyncScope()) + { + var exporter = scope.ServiceProvider.GetRequiredService(); + var ctx = scope.ServiceProvider.GetRequiredService(); + var siteIds = await ctx.Sites.Select(s => s.Id).ToListAsync(); + var selection = new ExportSelection( + TemplateIds: await ctx.Templates.Select(t => t.Id).ToListAsync(), + SharedScriptIds: Array.Empty(), + ExternalSystemIds: Array.Empty(), + DatabaseConnectionIds: Array.Empty(), + NotificationListIds: Array.Empty(), + SmtpConfigurationIds: Array.Empty(), + ApiMethodIds: Array.Empty(), + IncludeDependencies: true, + SiteIds: siteIds); + var stream = await exporter.ExportAsync(selection, user: "alice", sourceEnvironment: "dev", + passphrase: null, cancellationToken: CancellationToken.None); + using var ms = new MemoryStream(); + await stream.CopyToAsync(ms); + bundleBytes = ms.ToArray(); + } + + await using (var scope = _provider.CreateAsyncScope()) + { + var importer = scope.ServiceProvider.GetRequiredService(); + using var input = new MemoryStream(bundleBytes, writable: false); + return (await importer.LoadAsync(input, passphrase: null)).SessionId; + } + } + [Fact] public async Task TemplateScript_with_unresolved_reference_warns_but_does_not_block_import() { @@ -406,6 +480,111 @@ public sealed class SemanticValidatorImportTests : IDisposable Assert.Contains(ex.Errors, err => err.Contains("Process", StringComparison.Ordinal)); } + [Fact] + public async Task Apply_InstanceAlarmOverrideExpressionWithForbiddenApi_HardBlocks() + { + // N5 — an instance alarm override's TriggerConfigurationOverride replaces the + // template alarm's trigger config in the flattened config and compiles/executes + // at the site, so a forbidden API in it must be caught at import review — the + // same hard-block severity as every other trust-gate surface. + await SeedInstanceAlarmOverrideAsync( + "{\"expression\":\"System.Diagnostics.Process.GetProcesses().Length > 0\"}"); + + var sessionId = await ExportSitesAndLoadAsync(); + + SemanticValidationException ex = default!; + await using (var scope = _provider.CreateAsyncScope()) + { + var importer = scope.ServiceProvider.GetRequiredService(); + ex = await Assert.ThrowsAsync(() => + importer.ApplyAsync(sessionId, + new List + { + new("Template", "Pump", ResolutionAction.Add, null), + new("Site", "plant-1", ResolutionAction.Add, null), + new("Instance", "Pump-01", ResolutionAction.Add, null), + }, + user: "bob")); + } + + Assert.NotEmpty(ex.Errors); + Assert.Contains(ex.Errors, e => + e.Contains("Process", StringComparison.Ordinal) + && e.Contains("trigger override", StringComparison.Ordinal)); + + // Rollback contract: nothing from THIS import was written (the seed rows + // pre-exist, but no new import ran — the throw is pre-write at Pass 0). + } + + [Fact] + public async Task Preview_InstanceAlarmOverrideExpressionWithForbiddenApi_SurfacesBlocker() + { + await SeedInstanceAlarmOverrideAsync( + "{\"expression\":\"System.Diagnostics.Process.GetProcesses().Length > 0\"}"); + + var sessionId = await ExportSitesAndLoadAsync(); + + ImportPreview preview; + await using (var scope = _provider.CreateAsyncScope()) + { + var importer = scope.ServiceProvider.GetRequiredService(); + preview = await importer.PreviewAsync(sessionId); + } + + Assert.Contains(preview.Items, b => + b.Kind == ConflictKind.Blocker && b.EntityType == "Instance" + && b.BlockerReason!.Contains("trust violation", StringComparison.Ordinal)); + } + + [Fact] + public async Task Apply_InstanceAlarmOverrideExpression_Clean_Imports() + { + // A clean expression against the trigger surface imports without hard-block; + // the override row is persisted verbatim (bundle fidelity). + await SeedInstanceAlarmOverrideAsync( + "{\"expression\":\"Attributes[\\\"Temp\\\"] != null\"}"); + + var sessionId = await ExportSitesAndLoadAsync(); + + // Wipe so the apply exercises a create; re-create the template the instance binds to. + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + ctx.InstanceAlarmOverrides.RemoveRange(ctx.InstanceAlarmOverrides); + ctx.Instances.RemoveRange(ctx.Instances); + ctx.Sites.RemoveRange(ctx.Sites); + ctx.Templates.RemoveRange(ctx.Templates); + await ctx.SaveChangesAsync(); + ctx.Templates.Add(new Template("Pump") { Description = "pump tpl" }); + await ctx.SaveChangesAsync(); + } + + var nameMap = new BundleNameMap( + Sites: new[] { new SiteMapping("plant-1", MappingAction.CreateNew, null) }, + Connections: Array.Empty()); + + await using (var scope = _provider.CreateAsyncScope()) + { + var importer = scope.ServiceProvider.GetRequiredService(); + await importer.ApplyAsync(sessionId, + new List + { + new("Template", "Pump", ResolutionAction.Skip, null), + new("Site", "plant-1", ResolutionAction.Add, null), + new("Instance", "Pump-01", ResolutionAction.Add, null), + }, + user: "bob", ct: CancellationToken.None, nameMap: nameMap); + } + + await using (var verify = _provider.CreateAsyncScope()) + { + var ctx = verify.ServiceProvider.GetRequiredService(); + var ovr = await ctx.InstanceAlarmOverrides.SingleAsync(); + Assert.Equal("HiAlarm", ovr.AlarmCanonicalName); + Assert.Contains("Attributes", ovr.TriggerConfigurationOverride!); + } + } + [Fact] public async Task Apply_ApiMethodReference_NotSuppressedByTemplateLocalFunction() { From db623c143571b2e9b90b64e8780cde84079ba428 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 10:04:43 -0400 Subject: [PATCH 7/9] =?UTF-8?q?fix(transport):=20warn=20on=20import=20of?= =?UTF-8?q?=20instance=20overrides=20targeting=20locked=20template=20membe?= =?UTF-8?q?rs=20=E2=80=94=20inert=20rows=20no=20longer=20silent=20(plan=20?= =?UTF-8?q?R2-05=20T7)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Import/BundleImporter.cs | 109 +++++++++++- .../Import/SiteInstanceImportTests.cs | 166 ++++++++++++++++++ 2 files changed, 273 insertions(+), 2 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs index fc5664bf..8b8a880d 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs @@ -684,8 +684,9 @@ public sealed class BundleImporter : IBundleImporter // its new name, but at preview time no resolution has been chosen yet, so // the in-bundle name is the original DTO name — which is what an instance // references. (Explicit rename remap is a D-wave apply-time concern.) + var targetTemplates = await _templateRepo.GetAllTemplatesAsync(ct).ConfigureAwait(false); var targetTemplateNames = new HashSet(StringComparer.Ordinal); - foreach (var t in await _templateRepo.GetAllTemplatesAsync(ct).ConfigureAwait(false)) + foreach (var t in targetTemplates) { targetTemplateNames.Add(t.Name); } @@ -883,6 +884,22 @@ public sealed class BundleImporter : IBundleImporter } } + // N4: advisory warning rows for instance overrides that target a LOCKED + // template member — the flattener drops them, so surface the inert row in the + // wizard (Kind: Warning, non-blocking). resolutionMap is null at preview time, + // so every instance is scanned. + foreach (var (instance, message) in CollectLockedOverrideWarnings(content, resolutionMap: null, targetTemplates)) + { + blockers.Add(new ImportPreviewItem( + EntityType: "Instance", + Name: instance, + ExistingVersion: null, + IncomingVersion: null, + Kind: ConflictKind.Warning, + FieldDiffJson: null, + BlockerReason: message)); + } + return blockers; } @@ -1075,6 +1092,85 @@ public sealed class BundleImporter : IBundleImporter => resolutionMap != null && ResolveOrDefault(resolutionMap, entityType, name).Action == ResolutionAction.Skip; + /// + /// Best-effort scan for instance overrides that target a LOCKED template member. + /// The flattener drops such overrides (an override on a locked attribute / alarm / + /// native-alarm-source never takes effect), so the bundle carries an inert row — + /// this surfaces an advisory warning so the operator learns the config won't apply, + /// WITHOUT blocking the import or changing what gets written (bundle fidelity keeps + /// the row; the flattener stays the behavioural authority). + /// + /// Matches locked members by DIRECT name only — own + inherited placeholder rows + /// both carry the IsLocked flag, so a lock on either matches. Overrides + /// addressing composed path-qualified members (y1.z.Val) or derived-shadow + /// locks (LockedInDerived on a base the placeholder row doesn't reflect) may + /// not match a direct row — an unmatched name emits NO warning (never a false + /// positive; the ManagementActor and flattener remain the enforcement points). The + /// instance's template is resolved from the bundle DTO (the version about to be + /// written) first, else the pre-existing target template. + /// + /// + private static IReadOnlyList<(string Instance, string Message)> CollectLockedOverrideWarnings( + BundleContentDto content, + Dictionary<(string, string), ImportResolution>? resolutionMap, + IReadOnlyList