From 5065384305a5dd0edcd99ade39aa62c94e81df36 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 16 May 2026 05:57:39 -0400 Subject: [PATCH] fix(triggers): use explicit ValidationCategory + tighten expression syntax validation --- .../Validation/ScriptCompiler.cs | 9 +- .../Validation/ValidationService.cs | 94 ++++++++++++------- 2 files changed, 69 insertions(+), 34 deletions(-) diff --git a/src/ScadaLink.TemplateEngine/Validation/ScriptCompiler.cs b/src/ScadaLink.TemplateEngine/Validation/ScriptCompiler.cs index 92316d3..c5acc7c 100644 --- a/src/ScadaLink.TemplateEngine/Validation/ScriptCompiler.cs +++ b/src/ScadaLink.TemplateEngine/Validation/ScriptCompiler.cs @@ -12,8 +12,13 @@ namespace ScadaLink.TemplateEngine.Validation; /// public class ScriptCompiler { - // Forbidden namespace patterns - scripts must not use these - private static readonly string[] ForbiddenPatterns = + /// + /// Forbidden namespace patterns — scripts (and trigger expressions, via + /// ) must not use these. Trigger expressions run + /// under the same trust model as scripts, so the list is shared from here rather + /// than duplicated. + /// + internal static readonly string[] ForbiddenPatterns = [ "System.IO.", "System.Diagnostics.Process", diff --git a/src/ScadaLink.TemplateEngine/Validation/ValidationService.cs b/src/ScadaLink.TemplateEngine/Validation/ValidationService.cs index 9b85423..514c109 100644 --- a/src/ScadaLink.TemplateEngine/Validation/ValidationService.cs +++ b/src/ScadaLink.TemplateEngine/Validation/ValidationService.cs @@ -22,21 +22,6 @@ public class ValidationService private readonly SemanticValidator _semanticValidator; private readonly ScriptCompiler _scriptCompiler; - /// - /// Forbidden namespace patterns for trigger expressions. Mirrors the list in - /// — trigger expressions run under the same trust - /// model as scripts. - /// - private static readonly string[] ForbiddenExpressionPatterns = - [ - "System.IO.", - "System.Diagnostics.Process", - "System.Threading.", - "System.Reflection.", - "System.Net.Sockets.", - "System.Net.Http.", - ]; - public ValidationService(SemanticValidator semanticValidator, ScriptCompiler scriptCompiler) { _semanticValidator = semanticValidator; @@ -224,7 +209,8 @@ public class ValidationService continue; CheckExpressionTrigger( - "Script", script.CanonicalName, script.TriggerConfiguration, + ValidationCategory.ScriptTriggerReference, "script", + script.CanonicalName, script.TriggerConfiguration, attributeNames, errors, warnings); } @@ -234,7 +220,8 @@ public class ValidationService continue; CheckExpressionTrigger( - "Alarm", alarm.CanonicalName, alarm.TriggerConfiguration, + ValidationCategory.AlarmTriggerReference, "alarm", + alarm.CanonicalName, alarm.TriggerConfiguration, attributeNames, errors, warnings); } @@ -248,8 +235,20 @@ public class ValidationService /// Runs the blank / syntax / attribute-reference checks for a single /// Expression-trigger entity and appends any findings to the shared lists. /// + /// + /// The to file every finding under + /// ( for scripts, + /// for alarms). The same + /// category is used for blank, syntax, and attribute-reference findings so an + /// alarm's syntax error is not miscategorised as script compilation. + /// + /// + /// Human-readable entity-type label ("script"/"alarm") used in + /// message text only. + /// private static void CheckExpressionTrigger( - string entityType, + ValidationCategory category, + string entityLabel, string entityName, string? triggerConfigJson, HashSet attributeNames, @@ -260,11 +259,8 @@ public class ValidationService if (string.IsNullOrWhiteSpace(expression)) { - warnings.Add(ValidationEntry.Warning( - entityType == "Alarm" - ? ValidationCategory.AlarmTriggerReference - : ValidationCategory.ScriptTriggerReference, - $"{entityType} '{entityName}' has an expression trigger with no expression; it will never fire.", + warnings.Add(ValidationEntry.Warning(category, + $"The {entityLabel} '{entityName}' has an expression trigger with no expression; it will never fire.", entityName)); return; } @@ -272,8 +268,8 @@ public class ValidationService var syntaxError = CheckExpressionSyntax(expression); if (syntaxError != null) { - errors.Add(ValidationEntry.Error(ValidationCategory.ScriptCompilation, - $"{entityType} '{entityName}' expression trigger failed validation: {syntaxError}", + errors.Add(ValidationEntry.Error(category, + $"The {entityLabel} '{entityName}' expression trigger failed validation: {syntaxError}", entityName)); } @@ -281,11 +277,8 @@ public class ValidationService { if (!attributeNames.Contains(attrName)) { - errors.Add(ValidationEntry.Error( - entityType == "Alarm" - ? ValidationCategory.AlarmTriggerReference - : ValidationCategory.ScriptTriggerReference, - $"{entityType} '{entityName}' expression trigger references attribute '{attrName}' which does not exist in the flattened configuration.", + errors.Add(ValidationEntry.Error(category, + $"The {entityLabel} '{entityName}' expression trigger references attribute '{attrName}' which does not exist in the flattened configuration.", entityName)); } } @@ -302,8 +295,11 @@ public class ValidationService try { using var doc = JsonDocument.Parse(triggerConfigJson); - if (doc.RootElement.TryGetProperty("expression", out var prop)) + if (doc.RootElement.TryGetProperty("expression", out var prop) + && prop.ValueKind == JsonValueKind.String) + { return prop.GetString(); + } } catch (JsonException) { @@ -321,7 +317,7 @@ public class ValidationService /// internal static string? CheckExpressionSyntax(string expression) { - foreach (var pattern in ForbiddenExpressionPatterns) + foreach (var pattern in ScriptCompiler.ForbiddenPatterns) { if (expression.Contains(pattern, StringComparison.Ordinal)) { @@ -335,10 +331,29 @@ public class ValidationService var braceDepth = 0; var inString = false; var inChar = false; + var inLineComment = false; + var inBlockComment = false; for (int i = 0; i < expression.Length; i++) { var c = expression[i]; + var next = i + 1 < expression.Length ? expression[i + 1] : '\0'; + + if (inLineComment) + { + if (c == '\n') inLineComment = false; + continue; + } + + if (inBlockComment) + { + if (c == '*' && next == '/') + { + inBlockComment = false; + i++; + } + continue; + } if (inString) { @@ -354,6 +369,20 @@ public class ValidationService continue; } + if (c == '/' && next == '/') + { + inLineComment = true; + i++; + continue; + } + + if (c == '/' && next == '*') + { + inBlockComment = true; + i++; + continue; + } + switch (c) { case '"': inString = true; break; @@ -376,6 +405,7 @@ public class ValidationService } } + if (inBlockComment) return "unterminated block comment."; if (inString) return "unterminated string literal."; if (inChar) return "unterminated character literal."; if (parenDepth != 0) return $"mismatched parentheses ({parenDepth} unclosed).";