diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs index ab70ad58..c4d4e055 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs @@ -791,57 +791,66 @@ public sealed class BundleImporter : IBundleImporter // it as a Blocker. This catches the documented use-case // (HelperFn() / ErpSystem.Call()) without combinatorial blowup. // #05-T19 severity split: track candidate references by ORIGIN. Template- - // script (and attribute-default-expression) references are advisory - // warnings — the design-time deploy gate re-validates them; ApiMethod - // references are hard blockers (no downstream gate). Local-function / - // method declarations in a body are excluded so a script's own helper - // isn't mistaken for a missing SharedScript. + // script references are advisory warnings — the design-time deploy gate + // re-validates them; ApiMethod references are hard blockers (no downstream + // gate). Local-function / method declarations are collected PER ORIGIN so a + // helper declared in a template script cannot suppress a genuinely-missing + // ApiMethod reference of the same name (and vice-versa) — a global set would + // silently defeat the ApiMethod hard-blocker. var referencedFromTemplates = new HashSet(StringComparer.Ordinal); var referencedFromApiMethods = new HashSet(StringComparer.Ordinal); - var locallyDeclared = new HashSet(StringComparer.Ordinal); + var locallyDeclaredInTemplates = new HashSet(StringComparer.Ordinal); + var locallyDeclaredInApiMethods = new HashSet(StringComparer.Ordinal); foreach (var t in content.Templates) { foreach (var s in t.Scripts) { CollectCallIdentifiers(s.Code, referencedFromTemplates); - CollectLocalDeclarations(s.Code, locallyDeclared); + CollectLocalDeclarations(s.Code, locallyDeclaredInTemplates); } foreach (var a in t.Attributes) { - // Attribute.Value carries the design-time default expression, which - // can be script-callable. DataSourceReference is an OPC UA node - // address path (e.g. "ns=3;s=Tank.Level") owned by the device -- - // it's never script source and must NOT be scanned, or the dot - // delimiter trips the heuristic into flagging the address segments - // as missing SharedScript/ExternalSystem references. + // Attribute.Value is a typed-literal default (int/double/string/list, + // decoded by AttributeValueCodec — never compiled), so it is not a + // trust surface; we still scan it for identifier-shaped tokens, but + // any finding is only ever an advisory template warning. + // DataSourceReference is an OPC UA node address path (e.g. + // "ns=3;s=Tank.Level") owned by the device and must NOT be scanned, + // or the dot delimiter trips the heuristic into flagging the address + // segments as missing SharedScript/ExternalSystem references. CollectCallIdentifiers(a.Value, referencedFromTemplates); - CollectLocalDeclarations(a.Value, locallyDeclared); + CollectLocalDeclarations(a.Value, locallyDeclaredInTemplates); } } foreach (var m in content.ApiMethods) { CollectCallIdentifiers(m.Script, referencedFromApiMethods); - CollectLocalDeclarations(m.Script, locallyDeclared); + CollectLocalDeclarations(m.Script, locallyDeclaredInApiMethods); } // For each candidate, only report it if it looks like a resource // reference (PascalCase, length > 1), isn't a well-known language / - // runtime / SQL token, isn't the body's own local declaration, AND - // isn't present anywhere we can satisfy it. A candidate that appears in - // an ApiMethod script is a hard Blocker; a template-only candidate is a - // non-blocking Warning. + // runtime / SQL token, AND isn't present anywhere we can satisfy it. A + // candidate genuinely unresolved in an ApiMethod (referenced there and NOT + // locally declared there) is a hard Blocker; a candidate genuinely + // unresolved only in a template is a non-blocking Warning. var allCandidates = new HashSet(referencedFromTemplates, StringComparer.Ordinal); allCandidates.UnionWith(referencedFromApiMethods); foreach (var candidate in allCandidates.OrderBy(n => n, StringComparer.Ordinal)) { if (!LooksLikeResourceName(candidate)) continue; if (KnownNonReferenceNames.Contains(candidate)) continue; - if (locallyDeclared.Contains(candidate)) continue; var isShared = sharedScriptNames.Contains(candidate); var isExternal = externalSystemNames.Contains(candidate); if (isShared || isExternal) continue; - var fromApiMethod = referencedFromApiMethods.Contains(candidate); + var apiUnresolved = referencedFromApiMethods.Contains(candidate) + && !locallyDeclaredInApiMethods.Contains(candidate); + var templateUnresolved = referencedFromTemplates.Contains(candidate) + && !locallyDeclaredInTemplates.Contains(candidate); + if (!apiUnresolved && !templateUnresolved) continue; // satisfied by a local decl in its own origin + + var fromApiMethod = apiUnresolved; blockers.Add(new ImportPreviewItem( EntityType: "Reference", Name: candidate, @@ -861,7 +870,29 @@ public sealed class BundleImporter : IBundleImporter // as distinct rows. foreach (var (kind, entityName, scriptLabel, code) in EnumerateTrustGatedScripts(content, resolutionMap: null)) { - foreach (var violation in ScriptTrustValidator.FindViolations(code)) + IReadOnlyList violations; + try + { + violations = ScriptTrustValidator.FindViolations(code); + } + catch (Exception ex) + { + // Fail closed (mirrors the apply-time gate): a validator throw on + // one body surfaces as a Blocker for that script, not a preview crash. + _logger?.LogWarning(ex, + "Script trust analysis threw for {Kind} '{EntityName}' script '{ScriptLabel}' during preview; surfacing as a Blocker.", + kind, entityName, scriptLabel); + blockers.Add(new ImportPreviewItem( + EntityType: kind, + Name: $"{entityName}.{scriptLabel}", + ExistingVersion: null, + IncomingVersion: null, + Kind: ConflictKind.Blocker, + FieldDiffJson: null, + BlockerReason: "Script trust analysis failed to complete — rejected.")); + continue; + } + foreach (var violation in violations) { blockers.Add(new ImportPreviewItem( EntityType: kind, @@ -935,11 +966,15 @@ public sealed class BundleImporter : IBundleImporter } /// - /// #05-T20 — enumerates every executable script body the trust gate must - /// vet: non-Skip template scripts, shared scripts, and ApiMethod scripts. - /// A null (preview time, before resolutions - /// are chosen) gates everything. Yields (kind, entityName, scriptLabel, - /// code) so callers can format either an error string or a preview row. + /// #05-T20 — 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 + /// (`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, + /// scriptLabel, code) so callers can format either an error string or a + /// preview row. /// private static IEnumerable<(string Kind, string EntityName, string ScriptLabel, string Code)> EnumerateTrustGatedScripts( BundleContentDto content, @@ -954,6 +989,19 @@ public sealed class BundleImporter : IBundleImporter { yield return ("Template", t.Name, s.Name, s.Code); } + var scriptExpr = ExtractTriggerExpression(s.TriggerType, s.TriggerConfiguration); + if (!string.IsNullOrEmpty(scriptExpr)) + { + yield return ("Template", t.Name, $"{s.Name} (trigger expression)", scriptExpr); + } + } + foreach (var a in t.Alarms) + { + var alarmExpr = ExtractTriggerExpression(a.TriggerType.ToString(), a.TriggerConfiguration); + if (!string.IsNullOrEmpty(alarmExpr)) + { + yield return ("Template", t.Name, $"{a.Name} (alarm trigger expression)", alarmExpr); + } } } foreach (var s in content.SharedScripts) @@ -974,6 +1022,38 @@ public sealed class BundleImporter : IBundleImporter } } + /// + /// Extracts the C# boolean expression from an {"expression":"…"} trigger + /// configuration when the trigger type is Expression — the same shape + /// the site's TriggerExpressionGlobals compiles. Returns null for any + /// non-Expression trigger, empty config, or malformed JSON (nothing to gate). + /// + private static string? ExtractTriggerExpression(string? triggerType, string? triggerConfigJson) + { + if (triggerType is null + || !triggerType.Equals("Expression", StringComparison.OrdinalIgnoreCase) + || string.IsNullOrWhiteSpace(triggerConfigJson)) + { + return null; + } + try + { + using var doc = JsonDocument.Parse(triggerConfigJson); + if (doc.RootElement.ValueKind == JsonValueKind.Object + && doc.RootElement.TryGetProperty("expression", out var expr) + && expr.ValueKind == JsonValueKind.String) + { + return expr.GetString(); + } + } + catch (JsonException) + { + // Malformed trigger config isn't executable as an expression — nothing + // to gate here (a separate validation surfaces the malformed config). + } + return null; + } + private static bool IsSkipResolution( Dictionary<(string, string), ImportResolution>? resolutionMap, string entityType, string name) => resolutionMap != null @@ -4250,19 +4330,39 @@ public sealed class BundleImporter : IBundleImporter var warnings = new List(); // ---- Pass 0: script trust gate ---- - // Bundle import is the fifth script-trust call site. Every executable - // script body (template, shared, ApiMethod) is run through the shared - // ScriptTrustValidator BEFORE name resolution — a forbidden-API verdict - // is authoritative (not the false-positive-prone name heuristic), so it - // is a HARD error for all kinds, and it must not be masked by a Pass-1 - // name-resolution error. Task 15's verdict cache keeps repeat cost nil. + // Bundle import is the fifth script-trust call site. Every executable C# + // surface a bundle carries (template / shared / ApiMethod script bodies + // AND template script + alarm Expression-trigger bodies) is run through + // the shared ScriptTrustValidator BEFORE name resolution — a forbidden-API + // verdict is authoritative (not the false-positive-prone name heuristic), + // so it is a HARD error for all kinds, and it must not be masked by a + // Pass-1 name-resolution error. Task 15's verdict cache keeps repeat cost nil. foreach (var (kind, entityName, scriptLabel, code) in EnumerateTrustGatedScripts(content, resolutionMap)) { - foreach (var violation in ScriptTrustValidator.FindViolations(code)) + IReadOnlyList violations; + try + { + violations = ScriptTrustValidator.FindViolations(code); + } + catch (Exception ex) + { + // Fail closed: a validator throw on one pathological body must not + // abort the whole import — treat it as a hard blocker FOR THAT + // script so the offender is named and the rest still surface. + _logger?.LogWarning(ex, + "Script trust analysis threw for {Kind} '{EntityName}' script '{ScriptLabel}'; treating as a hard blocker.", + kind, entityName, scriptLabel); + errors.Add($"{kind} '{entityName}' script '{scriptLabel}': trust analysis failed to complete — rejected."); + continue; + } + foreach (var violation in violations) { errors.Add($"{kind} '{entityName}' script '{scriptLabel}': {violation}"); } } + // Fail fast on trust violations: the operator sees trust errors OR name- + // resolution errors, not both at once. `warnings` is always empty here + // (Pass 0 emits only errors), so nothing surfacable is dropped. if (errors.Count > 0) return (errors, warnings); // ---- Pass 1: minimal name-resolution scan ---- @@ -4314,7 +4414,8 @@ public sealed class BundleImporter : IBundleImporter // are collected too so a script's own helper isn't flagged as missing. var referencedFromTemplates = new HashSet(StringComparer.Ordinal); var referencedFromApiMethods = new HashSet(StringComparer.Ordinal); - var locallyDeclared = new HashSet(StringComparer.Ordinal); + var locallyDeclaredInTemplates = new HashSet(StringComparer.Ordinal); + var locallyDeclaredInApiMethods = new HashSet(StringComparer.Ordinal); foreach (var t in content.Templates) { // Skip-resolved templates aren't being written, so their script @@ -4324,17 +4425,18 @@ public sealed class BundleImporter : IBundleImporter foreach (var s in t.Scripts) { CollectCallIdentifiers(s.Code, referencedFromTemplates); - CollectLocalDeclarations(s.Code, locallyDeclared); + CollectLocalDeclarations(s.Code, locallyDeclaredInTemplates); } foreach (var a in t.Attributes) { - // Value can hold script-callable design-time expressions; + // Attribute.Value is a typed-literal default (never compiled), so it + // is not a trust surface; scanned only for the advisory name heuristic. // DataSourceReference is an OPC UA address-space path (e.g. // "ns=3;s=Tank.Level") and must NOT be scanned, or the dot // delimiter will flag tag-path segments as missing references. // Symmetric with DetectBlockersAsync. CollectCallIdentifiers(a.Value, referencedFromTemplates); - CollectLocalDeclarations(a.Value, locallyDeclared); + CollectLocalDeclarations(a.Value, locallyDeclaredInTemplates); } } foreach (var m in content.ApiMethods) @@ -4342,26 +4444,32 @@ public sealed class BundleImporter : IBundleImporter var resolution = ResolveOrDefault(resolutionMap, "ApiMethod", m.Name); if (resolution.Action == ResolutionAction.Skip) continue; CollectCallIdentifiers(m.Script, referencedFromApiMethods); - CollectLocalDeclarations(m.Script, locallyDeclared); + CollectLocalDeclarations(m.Script, locallyDeclaredInApiMethods); } - // ApiMethod references are hard errors (no downstream deploy gate); - // template-only references are advisory warnings (the design-time deploy - // gate re-validates with a real compile). + // ApiMethod references genuinely unresolved (referenced there and NOT locally + // declared there) are hard errors (no downstream deploy gate); template-only + // unresolved references are advisory warnings (the deploy gate re-validates + // with a real compile). Local-declaration exclusion is per-origin so a + // template helper cannot mask a missing ApiMethod reference. var allCandidates = new HashSet(referencedFromTemplates, StringComparer.Ordinal); allCandidates.UnionWith(referencedFromApiMethods); foreach (var candidate in allCandidates.OrderBy(n => n, StringComparer.Ordinal)) { if (!LooksLikeResourceName(candidate)) continue; if (KnownNonReferenceNames.Contains(candidate)) continue; - if (locallyDeclared.Contains(candidate)) continue; if (sharedScriptNames.Contains(candidate) || externalSystemNames.Contains(candidate)) continue; - if (referencedFromApiMethods.Contains(candidate)) + + var apiUnresolved = referencedFromApiMethods.Contains(candidate) + && !locallyDeclaredInApiMethods.Contains(candidate); + var templateUnresolved = referencedFromTemplates.Contains(candidate) + && !locallyDeclaredInTemplates.Contains(candidate); + if (apiUnresolved) { errors.Add( $"Script references SharedScript or ExternalSystem '{candidate}' not present in bundle or target."); } - else + else if (templateUnresolved) { warnings.Add( $"Template script references SharedScript or ExternalSystem '{candidate}' not present in bundle or target — advisory; re-validated at deploy time."); diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/SemanticValidatorImportTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/SemanticValidatorImportTests.cs index b3502076..73b9de20 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/SemanticValidatorImportTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/SemanticValidatorImportTests.cs @@ -367,6 +367,86 @@ public sealed class SemanticValidatorImportTests : IDisposable Assert.Contains(ex.Errors, err => err.Contains("Process", StringComparison.Ordinal)); } + [Fact] + public async Task Apply_AlarmExpressionTriggerWithForbiddenApi_HardBlocks() + { + // #05-T20 (code-review fix): an alarm's Expression trigger compiles and + // executes at the site, so a forbidden API in the trigger expression must + // be caught at import — not just in template/shared/ApiMethod script bodies. + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + var t = new Template("AlarmedTank"); + t.Alarms.Add(new TemplateAlarm("Bad") + { + TriggerType = AlarmTriggerType.Expression, + TriggerConfiguration = "{\"expression\":\"System.Diagnostics.Process.GetCurrentProcess() != null\"}", + PriorityLevel = 1, + }); + ctx.Templates.Add(t); + await ctx.SaveChangesAsync(); + } + + var sessionId = await ExportWipeAndLoadAsync(); + + 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", "AlarmedTank", ResolutionAction.Add, null), + }, + user: "bob")); + } + + Assert.NotEmpty(ex.Errors); + Assert.Contains(ex.Errors, err => err.Contains("Process", StringComparison.Ordinal)); + } + + [Fact] + public async Task Apply_ApiMethodReference_NotSuppressedByTemplateLocalFunction() + { + // #05-T19 (code-review fix): a local function declared in a TEMPLATE script + // must not suppress a genuinely-missing reference of the same name in an + // ApiMethod — the ApiMethod finding stays a HARD error. A pre-fix global + // locallyDeclared set silently defeated this. + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + var t = new Template("Helper"); + t.Scripts.Add(new Commons.Entities.Templates.TemplateScript( + "init", + "decimal SharedHelper(decimal x) => x * 2; return SharedHelper(1m);")); + ctx.Templates.Add(t); + ctx.ApiMethods.Add(new Commons.Entities.InboundApi.ApiMethod( + "Ingest", + "var x = SharedHelper(); return x;")); + await ctx.SaveChangesAsync(); + } + + var sessionId = await ExportWipeAndLoadAsync(); + + 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", "Helper", ResolutionAction.Add, null), + new("ApiMethod", "Ingest", ResolutionAction.Add, null), + }, + user: "bob")); + } + + Assert.NotEmpty(ex.Errors); + Assert.Contains(ex.Errors, err => err.Contains("SharedHelper", StringComparison.Ordinal)); + } + [Fact] public async Task SemanticValidator_catches_alarm_trigger_type_mismatch_at_import() {