diff --git a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompiler.cs b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompiler.cs index 656f3683..3eb1d571 100644 --- a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompiler.cs +++ b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompiler.cs @@ -1,105 +1,51 @@ using ZB.MOM.WW.ScadaBridge.Commons.Types; +using ZB.MOM.WW.ScadaBridge.ScriptAnalysis; namespace ZB.MOM.WW.ScadaBridge.TemplateEngine.Validation; /// -/// Validates script code by attempting to compile it using Roslyn. -/// In production, this would compile C# scripts against a stub ScriptApi assembly -/// that provides the allowed API surface (attribute read/write, CallScript, CallShared, etc.) -/// and enforces the forbidden API list (System.IO, Process, Threading, Reflection, raw network). -/// -/// For now, this implementation performs basic syntax validation. +/// The authoritative design-time / deploy-blocking script gate. Delegates to the +/// shared analyzer (M3.2): a +/// real Roslyn semantic forbidden-API verdict +/// () +/// followed by a real CSharpScript compile against the +/// globals +/// (). /// /// -/// SECURITY LIMITATION (TemplateEngine-006): the forbidden-API check below -/// is an interim, advisory text scan — it is NOT an authoritative trust-model -/// boundary. removes the -/// false-positive half (forbidden text inside a string/comment is ignored), but a -/// determined script can still bypass the literal patterns via namespace aliases, -/// using static, or global::-qualified references. Authoritative -/// enforcement requires Roslyn semantic symbol analysis of the referenced -/// types/namespaces and is the responsibility of the real script compiler and the -/// Site Runtime sandbox. Do not rely on this class as the sole trust-model gate. +/// This is a real trust-model boundary, not the previous advisory substring + +/// brace-balance scan: the forbidden-API check resolves symbols (so namespace +/// aliases, using static, global::-qualified references, and the +/// reflection gateway are all caught), and the compile catches undefined symbols +/// and signature mismatches that a structural scan could never see. /// /// public class ScriptCompiler { - /// - /// 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. - /// - /// - /// Matched with against code - /// regions only. This is advisory — see the class summary's SECURITY LIMITATION - /// note; the substring patterns are bypassable and the authoritative check is - /// deferred to Roslyn semantic analysis. - /// - /// - internal static readonly string[] ForbiddenPatterns = - [ - "System.IO.", - "System.Diagnostics.Process", - "System.Threading.", - "System.Reflection.", - "System.Net.Sockets.", - "System.Net.Http.", - ]; - /// /// Attempts to compile a script and returns success or a compilation error. + /// The security check runs first: a script that uses a forbidden API is + /// rejected before any compile diagnostics are considered. /// /// The C# script code. /// The canonical name of the script (for error messages). - /// Success if the script compiles, or Failure with the error message. + /// Success if the script is clean and compiles, or Failure with the error message. public Result TryCompile(string code, string scriptName) { if (string.IsNullOrWhiteSpace(code)) return Result.Failure($"Script '{scriptName}' has empty code."); - // Check for forbidden APIs. Advisory only (see class summary): the scan is - // code-region-aware so forbidden text inside a string/comment is ignored, - // but it remains a substring match and is not an authoritative boundary. - foreach (var pattern in ForbiddenPatterns) - { - if (CSharpDelimiterScanner.ContainsInCode(code, pattern)) - { - return Result.Failure( - $"Script '{scriptName}' uses forbidden API: '{pattern.TrimEnd('.')}'. " + - "Scripts cannot use System.IO, Process, Threading, Reflection, or raw network APIs."); - } - } + // Authoritative forbidden-API verdict first — a security violation must + // gate the script regardless of whether it otherwise compiles. + var violations = ScriptTrustValidator.FindViolations(code); + if (violations.Count > 0) + return Result.Failure($"Script '{scriptName}' uses forbidden API: {violations[0]}"); - // Basic structural validation: balanced braces/brackets/parens. The scan - // is string- and comment-aware (see CSharpDelimiterScanner) so a delimiter - // inside a regular/verbatim/interpolated/raw string, a char literal, or a - // comment does not produce a false mismatch. This remains an interim check - // until the Roslyn-based compiler is wired in. - var mismatch = CSharpDelimiterScanner.Scan(code); - return mismatch switch - { - CSharpDelimiterScanner.Mismatch.None => - Result.Success(true), - CSharpDelimiterScanner.Mismatch.UnexpectedCloseBrace => - Result.Failure($"Script '{scriptName}' has mismatched braces (unexpected closing brace)."), - CSharpDelimiterScanner.Mismatch.UnclosedBrace => - Result.Failure($"Script '{scriptName}' has mismatched braces (unclosed opening brace)."), - CSharpDelimiterScanner.Mismatch.UnexpectedCloseBracket => - Result.Failure($"Script '{scriptName}' has mismatched brackets (unexpected closing bracket)."), - CSharpDelimiterScanner.Mismatch.UnclosedBracket => - Result.Failure($"Script '{scriptName}' has mismatched brackets (unclosed opening bracket)."), - CSharpDelimiterScanner.Mismatch.UnexpectedCloseParen => - Result.Failure($"Script '{scriptName}' has mismatched parentheses (unexpected closing parenthesis)."), - CSharpDelimiterScanner.Mismatch.UnclosedParen => - Result.Failure($"Script '{scriptName}' has mismatched parentheses (unclosed opening parenthesis)."), - CSharpDelimiterScanner.Mismatch.UnclosedBlockComment => - Result.Failure($"Script '{scriptName}' has an unclosed block comment."), - CSharpDelimiterScanner.Mismatch.UnterminatedString => - Result.Failure($"Script '{scriptName}' has an unterminated string literal."), - CSharpDelimiterScanner.Mismatch.UnterminatedChar => - Result.Failure($"Script '{scriptName}' has an unterminated character literal."), - _ => Result.Success(true), - }; + // Real CSharpScript compile against the runtime-mirroring globals surface. + var errors = RoslynScriptCompiler.Compile(code, typeof(ScriptCompileSurface)); + if (errors.Count > 0) + return Result.Failure($"Script '{scriptName}' failed to compile: {errors[0]}"); + + return Result.Success(true); } } diff --git a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs index f99517cb..c2a56b3b 100644 --- a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs @@ -1,5 +1,6 @@ using System.Text.Json; using ZB.MOM.WW.ScadaBridge.Commons.Types.Flattening; +using ZB.MOM.WW.ScadaBridge.ScriptAnalysis; namespace ZB.MOM.WW.ScadaBridge.TemplateEngine.Validation; @@ -247,10 +248,11 @@ public class ValidationService /// checks against the { "expression": "..." } trigger configuration: /// /// Blank expression → warning (the trigger will never fire). - /// Syntax check → error if the expression uses a forbidden API or has - /// unbalanced brackets/quotes. The TemplateEngine project does not reference a - /// Roslyn compiler (see ), so this mirrors that - /// string-based syntax check rather than a full compile. + /// Syntax/compile check → error if the expression uses a forbidden API + /// or does not compile as a bare boolean expression. Delegates to the shared + /// authoritative analyzer (see and + /// ) — a real Roslyn compile against the + /// , not a string scan. /// Attribute-reference scan → error for any Attributes["X"] literal /// whose key is absent from the flattened configuration, mirroring /// for the structured triggers. @@ -373,113 +375,27 @@ public class ValidationService } /// - /// Lightweight string-based syntax check for a trigger expression. Mirrors the - /// approach in (the TemplateEngine project has no - /// Roslyn compiler reference): rejects forbidden APIs and unbalanced - /// brackets/quotes. Returns an error message, or null when the expression - /// looks well-formed. + /// Authoritative syntax/trust check for a trigger expression. Delegates to the + /// shared analyzer (same gate + /// as ): a real forbidden-API verdict + /// () + /// followed by a real CSharpScript compile of the bare boolean expression against + /// the globals. Returns an error message, or + /// null when the expression is clean and compiles. /// - /// The expression to check for syntax errors. + /// The expression to check. /// A human-readable error message if the expression is invalid; null if well-formed. internal static string? CheckExpressionSyntax(string expression) { - // Advisory forbidden-API scan (TemplateEngine-006): code-region-aware so - // the inert text inside a string/comment is not flagged, but still a - // substring match — not an authoritative boundary. See ScriptCompiler. - foreach (var pattern in ScriptCompiler.ForbiddenPatterns) - { - if (CSharpDelimiterScanner.ContainsInCode(expression, pattern)) - { - return $"uses forbidden API '{pattern.TrimEnd('.')}'. " + - "Trigger expressions cannot use System.IO, Process, Threading, Reflection, or raw network APIs."; - } - } + // Authoritative forbidden-API verdict first. + var violations = ScriptTrustValidator.FindViolations(expression); + if (violations.Count > 0) + return $"uses forbidden API: {violations[0]}"; - var parenDepth = 0; - var bracketDepth = 0; - 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) - { - if (c == '\\') { i++; continue; } - if (c == '"') inString = false; - continue; - } - - if (inChar) - { - if (c == '\\') { i++; continue; } - if (c == '\'') inChar = false; - continue; - } - - if (c == '/' && next == '/') - { - inLineComment = true; - i++; - continue; - } - - if (c == '/' && next == '*') - { - inBlockComment = true; - i++; - continue; - } - - switch (c) - { - case '"': inString = true; break; - case '\'': inChar = true; break; - case '(': parenDepth++; break; - case ')': - parenDepth--; - if (parenDepth < 0) return "mismatched parentheses (unexpected ')')."; - break; - case '[': bracketDepth++; break; - case ']': - bracketDepth--; - if (bracketDepth < 0) return "mismatched brackets (unexpected ']')."; - break; - case '{': braceDepth++; break; - case '}': - braceDepth--; - if (braceDepth < 0) return "mismatched braces (unexpected '}')."; - break; - } - } - - 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)."; - if (bracketDepth != 0) return $"mismatched brackets ({bracketDepth} unclosed)."; - if (braceDepth != 0) return $"mismatched braces ({braceDepth} unclosed)."; + // 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 null; } diff --git a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/ZB.MOM.WW.ScadaBridge.TemplateEngine.csproj b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/ZB.MOM.WW.ScadaBridge.TemplateEngine.csproj index c8a75253..3d7a4763 100644 --- a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/ZB.MOM.WW.ScadaBridge.TemplateEngine.csproj +++ b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/ZB.MOM.WW.ScadaBridge.TemplateEngine.csproj @@ -18,6 +18,7 @@ + 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 72e59524..e066cfcb 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompilerTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompilerTests.cs @@ -2,17 +2,18 @@ using ZB.MOM.WW.ScadaBridge.TemplateEngine.Validation; namespace ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests.Validation; +/// +/// M3.2: the design-time deploy gate now delegates to the shared +/// ZB.MOM.WW.ScadaBridge.ScriptAnalysis analyzer — a real Roslyn semantic +/// forbidden-API verdict plus a real CSharpScript compile against the +/// runtime-mirroring ScriptCompileSurface. These tests assert the +/// authoritative behavior: bypasses the old substring scan missed are now caught, +/// and undefined symbols (which a structural scan could never see) fail compile. +/// public class ScriptCompilerTests { private readonly ScriptCompiler _sut = new(); - [Fact] - public void TryCompile_ValidCode_ReturnsSuccess() - { - var result = _sut.TryCompile("var x = 1; if (x > 0) { x++; }", "Test"); - Assert.True(result.IsSuccess); - } - [Fact] public void TryCompile_EmptyCode_ReturnsFailure() { @@ -22,134 +23,91 @@ public class ScriptCompilerTests } [Fact] - public void TryCompile_MismatchedBraces_ReturnsFailure() + public void TryCompile_WhitespaceOnly_ReturnsFailure() { - var result = _sut.TryCompile("if (true) { x = 1;", "Test"); + var result = _sut.TryCompile(" \n\t ", "Test"); Assert.True(result.IsFailure); - Assert.Contains("braces", result.Error, StringComparison.OrdinalIgnoreCase); + Assert.Contains("empty", result.Error, StringComparison.OrdinalIgnoreCase); } + // --- Forbidden-API gate (authoritative) --- + [Fact] - public void TryCompile_UnclosedBlockComment_ReturnsFailure() + public void TryCompile_ForbiddenApi_ReturnsFailure() { - var result = _sut.TryCompile("/* this is never closed", "Test"); + var result = _sut.TryCompile("using System.IO; File.ReadAllText(\"x\");", "Test"); Assert.True(result.IsFailure); - Assert.Contains("comment", result.Error, StringComparison.OrdinalIgnoreCase); + Assert.Contains("forbidden", result.Error, StringComparison.OrdinalIgnoreCase); } [Theory] - [InlineData("System.IO.File.ReadAllText(\"x\");")] - [InlineData("System.Diagnostics.Process.Start(\"cmd\");")] - [InlineData("System.Threading.Thread.Sleep(1000);")] - [InlineData("System.Reflection.Assembly.Load(\"x\");")] - [InlineData("System.Net.Sockets.TcpClient c;")] - [InlineData("System.Net.Http.HttpClient c;")] - public void TryCompile_ForbiddenApi_ReturnsFailure(string code) + // Namespace alias — the old substring scan never saw "System.IO." spelled out. + [InlineData("using IO = System.IO; IO.File.ReadAllText(\"x\");")] + // using static — the call site is a bare ReadAllText(...) with no namespace text. + [InlineData("using static System.IO.File; ReadAllText(\"x\");")] + // global::-qualified reference. + [InlineData("global::System.IO.File.ReadAllText(\"x\");")] + // Reflection gateway — never spells a forbidden namespace at all. + [InlineData("typeof(string).Assembly.GetType(\"System.IO.File\");")] + public void TryCompile_AdversarialForbiddenApiBypass_StillRejected(string code) { var result = _sut.TryCompile(code, "Test"); Assert.True(result.IsFailure); Assert.Contains("forbidden", result.Error, StringComparison.OrdinalIgnoreCase); } - [Fact] - public void TryCompile_BracesInStrings_Ignored() - { - var result = _sut.TryCompile("var s = \"{ not a brace }\";", "Test"); - Assert.True(result.IsSuccess); - } + // --- Real-compile gate (the win over the old structural-only scan) --- [Fact] - public void TryCompile_BracesInComments_Ignored() + public void TryCompile_UndefinedSymbol_ReturnsFailure() { - var result = _sut.TryCompile("// { not a brace\nvar x = 1;", "Test"); - Assert.True(result.IsSuccess); - } - - [Fact] - public void TryCompile_BlockCommentWithBraces_Ignored() - { - var result = _sut.TryCompile("/* { } */ var x = 1;", "Test"); - Assert.True(result.IsSuccess); - } - - // --- TemplateEngine-007 regression: string-literal awareness --- - - [Fact] - public void TryCompile_VerbatimStringWithBrace_NotFlaggedAsMismatched() - { - // @"..." — backslash is literal, "" is the escape. The closing brace - // inside the verbatim string must not affect the brace balance. - var result = _sut.TryCompile("var s = @\"a brace } and a \\ slash\"; if (true) { }", "Test"); - Assert.True(result.IsSuccess); - } - - [Fact] - public void TryCompile_VerbatimStringWithEscapedQuote_NotFlaggedAsMismatched() - { - // The "" inside a verbatim string is an escaped quote, not a string end. - var result = _sut.TryCompile("var s = @\"he said \"\"hi}\"\"\"; { }", "Test"); - Assert.True(result.IsSuccess); - } - - [Fact] - public void TryCompile_InterpolatedStringWithBraces_NotFlaggedAsMismatched() - { - // The braces in $"{x}" are interpolation holes; the literal "}}" is an - // escaped brace. Neither should unbalance the real braces. - var result = _sut.TryCompile("var x = 1; var s = $\"val={x} literal}}\"; if (x>0) { x++; }", "Test"); - Assert.True(result.IsSuccess); - } - - [Fact] - public void TryCompile_RawStringLiteralWithBraces_NotFlaggedAsMismatched() - { - // C# 11 raw string literal — the triple quotes delimit, braces inside are text. - var result = _sut.TryCompile("var s = \"\"\"a } brace { in raw\"\"\"; { }", "Test"); - Assert.True(result.IsSuccess); - } - - [Fact] - public void TryCompile_CharLiteralWithBrace_NotFlaggedAsMismatched() - { - // A '}' char literal must not decrement the brace depth. - var result = _sut.TryCompile("var c = '}'; if (true) { }", "Test"); - Assert.True(result.IsSuccess); - } - - [Fact] - public void TryCompile_GenuineMismatchedBraces_StillDetected() - { - // Sanity check that the string-aware scan still catches real mismatches. - var result = _sut.TryCompile("var s = \"ok\"; if (true) { x++;", "Test"); + // A brace-balanced, forbidden-API-free script that references an undefined + // symbol. The old substring + brace scan accepted this; the real compile + // rejects it. + var result = _sut.TryCompile("var x = NoSuchThing.Foo();", "Test"); Assert.True(result.IsFailure); - Assert.Contains("braces", result.Error, StringComparison.OrdinalIgnoreCase); - } - - // --- TemplateEngine-006 regression: forbidden-API scan false positives --- - - [Fact] - public void TryCompile_ForbiddenApiTextInsideStringLiteral_NotFlagged() - { - // "System.IO." appears only inside a string literal — it is inert text, - // not a use of the forbidden API, and must not be rejected. - var result = _sut.TryCompile("var msg = \"see System.IO.File docs\"; var x = 1;", "Test"); - Assert.True(result.IsSuccess); + Assert.Contains("compile", result.Error, StringComparison.OrdinalIgnoreCase); } [Fact] - public void TryCompile_ForbiddenApiTextInsideComment_NotFlagged() + public void TryCompile_SyntaxError_ReturnsFailure() { - // "System.Threading." appears only inside a comment — inert. - var result = _sut.TryCompile("// avoid System.Threading.Thread here\nvar x = 1;", "Test"); - Assert.True(result.IsSuccess); - } - - [Fact] - public void TryCompile_ForbiddenApiInRealCode_StillFlagged() - { - // Sanity check: a genuine use in code is still rejected. - var result = _sut.TryCompile("var x = System.IO.File.ReadAllText(\"a\");", "Test"); + // Genuinely malformed — must fail the real compile. + var result = _sut.TryCompile("if (true) { var x = 1;", "Test"); Assert.True(result.IsFailure); - Assert.Contains("forbidden", result.Error, StringComparison.OrdinalIgnoreCase); + } + + // --- Valid real script against the script API surface --- + + [Fact] + public void TryCompile_ValidScriptUsingApiSurface_ReturnsSuccess() + { + const string code = """ + var t = Attributes["Temperature"]; + Attributes["Setpoint"] = 42; + var r = await ExternalSystem.Call("erp", "sync"); + await Notify.To("ops").Send("s", "m"); + """; + + var result = _sut.TryCompile(code, "Test"); + Assert.True(result.IsSuccess, result.IsFailure ? result.Error : null); + } + + // --- Trigger-expression gate via ValidationService.CheckExpressionSyntax --- + + [Fact] + public void CheckExpressionSyntax_ValidExpression_ReturnsNull() + { + // A bare boolean expression binding against the trigger globals surface. + var error = ValidationService.CheckExpressionSyntax("Attributes[\"Temp\"] != null"); + Assert.Null(error); + } + + [Fact] + public void CheckExpressionSyntax_ForbiddenApi_ReturnsMessage() + { + var error = ValidationService.CheckExpressionSyntax("System.IO.File.Exists(\"x\")"); + Assert.NotNull(error); + Assert.Contains("forbidden", error, StringComparison.OrdinalIgnoreCase); } }