diff --git a/code-reviews/TemplateEngine/findings.md b/code-reviews/TemplateEngine/findings.md index 9241d58..a0dec5a 100644 --- a/code-reviews/TemplateEngine/findings.md +++ b/code-reviews/TemplateEngine/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 9 | +| Open findings | 4 | ## Summary @@ -263,7 +263,7 @@ Regression test: `CreateTemplate_WithParent_DoesNotRunDeadCollisionQuery`. |--|--| | Severity | Medium | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.TemplateEngine/Validation/ScriptCompiler.cs:21`, `src/ScadaLink.TemplateEngine/Validation/ValidationService.cs:318` | **Description** @@ -289,7 +289,20 @@ limitation prominently and treat the substring scan as advisory, not authoritati **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit `pending`): fixed the false-positive half and +documented the (deferred) bypass half. Added `CSharpDelimiterScanner.ContainsInCode`, +a code-region-aware substring search that blanks out string/char-literal/comment +spans before matching, so the inert text `System.IO.` inside a string or comment is +no longer flagged. `ScriptCompiler.TryCompile` and `ValidationService.CheckExpressionSyntax` +now use it. The bypass half (namespace aliases, `using static`, `global::`) genuinely +requires Roslyn semantic symbol analysis, which the TemplateEngine project deliberately +does not reference — that authoritative check is deferred to the real script compiler / +Site Runtime sandbox. The limitation is now documented prominently as a `SECURITY +LIMITATION` note in the `ScriptCompiler` class summary and the `ForbiddenPatterns` +doc, and the scan is explicitly labelled advisory. Regression tests: +`TryCompile_ForbiddenApiTextInsideStringLiteral_NotFlagged`, +`TryCompile_ForbiddenApiTextInsideComment_NotFlagged`, +`TryCompile_ForbiddenApiInRealCode_StillFlagged`. ### TemplateEngine-007 — Brace-balance "compilation" misjudges verbatim / interpolated / raw strings @@ -297,7 +310,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.TemplateEngine/Validation/ScriptCompiler.cs:54`, `src/ScadaLink.TemplateEngine/SharedScriptService.cs:124` | **Description** @@ -321,7 +334,18 @@ to something that cannot false-positive. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit `pending`): replaced both hand-rolled string trackers +with `CSharpDelimiterScanner`, a single string-/comment-aware scanner that correctly +skips regular strings (with `\` escapes), verbatim strings (`@"..."`, `""` escape), +interpolated strings (`$"..."` / `$@"..."`, interpolation holes `{...}` treated as +code, `{{`/`}}` as escaped braces), C# 11 raw string literals (`"""..."""`), char +literals, and line/block comments while tracking `{}`/`[]`/`()` depth. `ScriptCompiler +.TryCompile` and `SharedScriptService.ValidateSyntax` now delegate to it, so a valid +script containing a delimiter inside a literal/comment is no longer falsely rejected; +genuine mismatches are still caught. Regression tests in `ScriptCompilerTests` +(`TryCompile_VerbatimStringWithBrace_*`, `_VerbatimStringWithEscapedQuote_*`, +`_InterpolatedStringWithBraces_*`, `_RawStringLiteralWithBraces_*`, `_CharLiteralWithBrace_*`, +`_GenuineMismatchedBraces_StillDetected`) and `SharedScriptServiceTests.ValidateSyntax_DelimiterInsideStringOrComment_ReturnsNull`. ### TemplateEngine-008 — `SetAlarmOverrideAsync` accepts overrides for unknown / composed alarms with no validation @@ -329,7 +353,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.TemplateEngine/Services/InstanceService.cs:178` | **Description** @@ -351,7 +375,16 @@ the lock check to composed alarms too. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit `pending`): `SetAlarmOverrideAsync` now resolves the +instance template's full effective alarm set via `TemplateResolver.ResolveAllMembers` +(loaded from `GetAllTemplatesAsync`) instead of looking up only the template's direct +alarms. An override whose canonical name is absent from that set is rejected with a +"does not exist" failure (mirroring `SetAttributeOverrideAsync`); the `IsLocked` check +now also applies to composed (path-qualified) and inherited alarms, closing the +lock-bypass. Regression tests: `SetAlarmOverride_NonExistentAlarm_ReturnsFailure`, +`SetAlarmOverride_ComposedLockedAlarm_ReturnsFailure`, +`SetAlarmOverride_ComposedUnlockedAlarm_ReturnsSuccess`, +`SetAlarmOverride_DirectLockedAlarm_ReturnsFailure`. ### TemplateEngine-009 — N+1 query in `TemplateDeletionService.CanDeleteTemplateAsync` @@ -359,7 +392,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Performance & resource management | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.TemplateEngine/Services/TemplateDeletionService.cs:75` | **Description** @@ -380,15 +413,24 @@ template. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit `pending`): `CanDeleteTemplateAsync` Check 3 now reads +the `Compositions` navigation already loaded by `GetAllTemplatesAsync` (a single +`SelectMany`) instead of issuing one `GetCompositionsByTemplateIdAsync` round-trip +per template — the same source `TemplateService.DeleteTemplateAsync` uses for the +equivalent check. The per-delete cost no longer scales with template count. +Regression test: `CanDeleteTemplate_DoesNotIssuePerTemplateCompositionQuery` +(verifies `GetCompositionsByTemplateIdAsync` is never called); the existing +`CanDeleteTemplate_ComposedByOthers_ReturnsFailure` and +`CanDeleteTemplate_MultipleConstraints_AllErrorsReported` tests were updated to seed +the `Compositions` navigation, matching how EF's `GetAllTemplatesAsync` loads it. ### TemplateEngine-010 — `InstanceService` documents optimistic concurrency that is not implemented | | | |--|--| -| Severity | Medium | +| Severity | Low — re-triaged from Medium: this is a stale XML comment, not a behavioural defect. The code matches the design (last-write-wins); only the doc string was wrong. | | Category | Documentation & comments | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.TemplateEngine/Services/InstanceService.cs:9` | **Description** @@ -407,9 +449,24 @@ If last-write-wins is acceptable for instance state, correct the XML doc. If opt concurrency is required, add a concurrency token to `Instance` and surface a conflict result. +**Re-triage** + +Verified against the design: `docs/requirements/Component-TemplateEngine.md` states +"Concurrent editing uses **last-write-wins** — no pessimistic locking or conflict +detection." The system's optimistic-concurrency decision (per CLAUDE.md) applies to +*deployment status records*, not instance state. The code is therefore correct — a +plain read-modify-write is the intended behaviour — and the only defect is the stale +"with optimistic concurrency" phrase in the class XML summary. Re-triaged from +Medium (Error handling) to Low (Documentation): doc-only fix, no behaviour change. + **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit `pending`): corrected the `InstanceService` class XML +summary — replaced "Enabled/disabled state with optimistic concurrency" with an +explicit statement that instance-state edits are last-write-wins (no version token / +conflict detection), citing the design decision and noting that optimistic concurrency +in the system applies to deployment status records, not instance state. No code or +behaviour change; no regression test (documentation-only). ### TemplateEngine-011 — `SortedPropertiesConverterFactory` is dead code with a misleading comment diff --git a/src/ScadaLink.TemplateEngine/Services/InstanceService.cs b/src/ScadaLink.TemplateEngine/Services/InstanceService.cs index 0a0338e..42d0767 100644 --- a/src/ScadaLink.TemplateEngine/Services/InstanceService.cs +++ b/src/ScadaLink.TemplateEngine/Services/InstanceService.cs @@ -3,6 +3,7 @@ using ScadaLink.Commons.Interfaces.Repositories; using ScadaLink.Commons.Interfaces.Services; using ScadaLink.Commons.Types; using ScadaLink.Commons.Types.Enums; +using ScadaLink.TemplateEngine; namespace ScadaLink.TemplateEngine.Services; @@ -13,7 +14,11 @@ namespace ScadaLink.TemplateEngine.Services; /// - Override non-locked attribute values /// - Cannot add or remove attributes (only override existing ones) /// - Per-attribute connection binding (bulk assignment support) -/// - Enabled/disabled state with optimistic concurrency +/// - Enabled/disabled state. Concurrent edits are last-write-wins — there is no +/// version token or conflict detection on instance state, matching the design +/// decision (Component-TemplateEngine.md: "Concurrent editing uses +/// last-write-wins — no pessimistic locking or conflict detection"). Optimistic +/// concurrency in the system applies to deployment status records, not here. /// - Audit logging /// public class InstanceService @@ -170,10 +175,11 @@ public class InstanceService } /// - /// Sets a per-instance alarm override. The alarm must exist on the - /// template and must not be locked. For HiLo alarms, the override JSON - /// merges into the inherited TriggerConfiguration setpoint-by-setpoint; - /// for binary trigger types, it replaces the whole config. + /// Sets a per-instance alarm override. The alarm must exist in the + /// instance's effective alarm set (direct, inherited, or composed) and + /// must not be locked. For HiLo alarms, the override JSON merges into the + /// inherited TriggerConfiguration setpoint-by-setpoint; for binary trigger + /// types, it replaces the whole config. /// public async Task> SetAlarmOverrideAsync( int instanceId, @@ -187,17 +193,25 @@ public class InstanceService if (instance == null) return Result.Failure($"Instance with ID {instanceId} not found."); - // Verify alarm exists in the template and is not locked. Only direct - // template alarms are checked here — composed-member overrides go - // through but are silently ignored at runtime if the name doesn't - // match (same behavior as attribute overrides). - var templateAlarms = await _repository.GetAlarmsByTemplateIdAsync(instance.TemplateId, cancellationToken); - var templateAlarm = templateAlarms.FirstOrDefault(a => a.Name == alarmCanonicalName); - if (templateAlarm != null && templateAlarm.IsLocked) - { + // Verify the alarm exists in the instance's effective alarm set and is + // not locked. The effective set is resolved via TemplateResolver so that + // composed (path-qualified) and inherited alarms are found — a lookup + // against the template's direct alarms alone would miss them, silently + // accepting an override for a non-existent name or bypassing the lock + // rule for a composed alarm. Mirrors SetAttributeOverrideAsync. + var allTemplates = await _repository.GetAllTemplatesAsync(cancellationToken); + var resolvedAlarm = TemplateResolver + .ResolveAllMembers(instance.TemplateId, allTemplates) + .FirstOrDefault(m => m.MemberType == "Alarm" && m.CanonicalName == alarmCanonicalName); + + if (resolvedAlarm == null) + return Result.Failure( + $"Alarm '{alarmCanonicalName}' does not exist in template {instance.TemplateId}. " + + "Cannot override an unknown alarm."); + + if (resolvedAlarm.IsLocked) return Result.Failure( $"Alarm '{alarmCanonicalName}' is locked and cannot be overridden."); - } var existingOverride = await _repository.GetAlarmOverrideAsync( instanceId, alarmCanonicalName, cancellationToken); diff --git a/src/ScadaLink.TemplateEngine/Services/TemplateDeletionService.cs b/src/ScadaLink.TemplateEngine/Services/TemplateDeletionService.cs index e18d552..88d4976 100644 --- a/src/ScadaLink.TemplateEngine/Services/TemplateDeletionService.cs +++ b/src/ScadaLink.TemplateEngine/Services/TemplateDeletionService.cs @@ -72,16 +72,15 @@ public class TemplateDeletionService } // Check 3: Other templates compose it directly (e.g., pre-Phase-3 data). - var composingTemplates = new List<(string TemplateName, string InstanceName)>(); - foreach (var t in allTemplates) - { - var compositions = await _repository.GetCompositionsByTemplateIdAsync(t.Id, cancellationToken); - foreach (var comp in compositions) - { - if (comp.ComposedTemplateId == templateId) - composingTemplates.Add((t.Name, comp.InstanceName)); - } - } + // Read the Compositions navigation already loaded by GetAllTemplatesAsync + // rather than issuing one GetCompositionsByTemplateIdAsync round-trip per + // template (TemplateEngine-009) — this is the same source TemplateService + // .DeleteTemplateAsync uses for the equivalent check. + var composingTemplates = allTemplates + .SelectMany(t => t.Compositions + .Where(comp => comp.ComposedTemplateId == templateId) + .Select(comp => (TemplateName: t.Name, comp.InstanceName))) + .ToList(); if (composingTemplates.Count > 0) { diff --git a/src/ScadaLink.TemplateEngine/SharedScriptService.cs b/src/ScadaLink.TemplateEngine/SharedScriptService.cs index 1c029ea..25db51d 100644 --- a/src/ScadaLink.TemplateEngine/SharedScriptService.cs +++ b/src/ScadaLink.TemplateEngine/SharedScriptService.cs @@ -118,7 +118,10 @@ public class SharedScriptService /// /// Basic structural validation of C# script code. - /// Checks for balanced braces and basic syntax structure. + /// Checks for balanced braces/brackets/parentheses. The scan is string- and + /// comment-aware (see ) so a + /// delimiter inside a regular/verbatim/interpolated/raw string literal, a + /// char literal, or a comment does not produce a false syntax error. /// Full Roslyn compilation would be added in a later phase when the scripting sandbox is available. /// internal static string? ValidateSyntax(string code) @@ -126,38 +129,28 @@ public class SharedScriptService if (string.IsNullOrWhiteSpace(code)) return "Script code cannot be empty."; - // Check for balanced braces - int braceCount = 0; - int bracketCount = 0; - int parenCount = 0; - - foreach (var ch in code) + return Validation.CSharpDelimiterScanner.Scan(code) switch { - switch (ch) - { - case '{': braceCount++; break; - case '}': braceCount--; break; - case '[': bracketCount++; break; - case ']': bracketCount--; break; - case '(': parenCount++; break; - case ')': parenCount--; break; - } - - if (braceCount < 0) - return "Syntax error: unmatched closing brace '}'."; - if (bracketCount < 0) - return "Syntax error: unmatched closing bracket ']'."; - if (parenCount < 0) - return "Syntax error: unmatched closing parenthesis ')'."; - } - - if (braceCount != 0) - return "Syntax error: unmatched opening brace '{'."; - if (bracketCount != 0) - return "Syntax error: unmatched opening bracket '['."; - if (parenCount != 0) - return "Syntax error: unmatched opening parenthesis '('."; - - return null; + Validation.CSharpDelimiterScanner.Mismatch.None => null, + Validation.CSharpDelimiterScanner.Mismatch.UnexpectedCloseBrace => + "Syntax error: unmatched closing brace '}'.", + Validation.CSharpDelimiterScanner.Mismatch.UnclosedBrace => + "Syntax error: unmatched opening brace '{'.", + Validation.CSharpDelimiterScanner.Mismatch.UnexpectedCloseBracket => + "Syntax error: unmatched closing bracket ']'.", + Validation.CSharpDelimiterScanner.Mismatch.UnclosedBracket => + "Syntax error: unmatched opening bracket '['.", + Validation.CSharpDelimiterScanner.Mismatch.UnexpectedCloseParen => + "Syntax error: unmatched closing parenthesis ')'.", + Validation.CSharpDelimiterScanner.Mismatch.UnclosedParen => + "Syntax error: unmatched opening parenthesis '('.", + Validation.CSharpDelimiterScanner.Mismatch.UnclosedBlockComment => + "Syntax error: unclosed block comment.", + Validation.CSharpDelimiterScanner.Mismatch.UnterminatedString => + "Syntax error: unterminated string literal.", + Validation.CSharpDelimiterScanner.Mismatch.UnterminatedChar => + "Syntax error: unterminated character literal.", + _ => null, + }; } } diff --git a/src/ScadaLink.TemplateEngine/Validation/CSharpDelimiterScanner.cs b/src/ScadaLink.TemplateEngine/Validation/CSharpDelimiterScanner.cs new file mode 100644 index 0000000..c224ecd --- /dev/null +++ b/src/ScadaLink.TemplateEngine/Validation/CSharpDelimiterScanner.cs @@ -0,0 +1,413 @@ +namespace ScadaLink.TemplateEngine.Validation; + +/// +/// String/comment-aware scanner for the balanced-delimiter ("does it look like +/// valid C#") checks used by and +/// SharedScriptService.ValidateSyntax. +/// +/// +/// This is not a compiler. It is an interim structural check that walks +/// the source once and tracks {}, [] and () depth while +/// correctly skipping over the C# lexical constructs in which a delimiter is +/// inert: line/block comments, regular string literals (with \ escapes), +/// verbatim strings (@"...", where "" escapes a quote and \ +/// is literal), interpolated strings ($"..." / $@"..." — the holes +/// {...} are code and {{/}} are escaped braces), raw string +/// literals ("""..."""), and char literals ('}'). +/// +/// +/// +/// It is intentionally conservative: when the real Roslyn-based compiler is +/// wired in (see ) this hand-rolled scan should be +/// replaced by CSharpSyntaxTree.ParseText diagnostics. Until then this +/// scanner removes the false positives that a naive character count produced +/// for valid scripts containing a delimiter inside a string or comment. +/// +/// +internal static class CSharpDelimiterScanner +{ + /// The kind of delimiter mismatch found, if any. + internal enum Mismatch + { + None, + UnexpectedCloseBrace, + UnexpectedCloseBracket, + UnexpectedCloseParen, + UnclosedBrace, + UnclosedBracket, + UnclosedParen, + UnclosedBlockComment, + UnterminatedString, + UnterminatedChar, + } + + /// + /// Returns true when occurs in a code + /// region of — i.e. not wholly inside a string + /// literal, char literal, or comment. Used by the interim forbidden-API + /// scan so that the inert text System.IO. in a comment or string + /// literal is not flagged as a forbidden API call (TemplateEngine-006). + /// + /// + /// This removes the false-positive half of the substring scan. It does + /// not close the bypass half: namespace aliases, using static, + /// and global::-qualified references still evade a pure text match. + /// Authoritative forbidden-API enforcement requires Roslyn semantic symbol + /// analysis and is deferred to the real script compiler / Site Runtime + /// sandbox; this check is advisory only. + /// + /// + internal static bool ContainsInCode(string code, string pattern) + { + if (string.IsNullOrEmpty(pattern)) + return false; + + // Blank out every string/char-literal/comment span, then do an ordinary + // substring search over what remains (the code regions). + var codeOnly = BlankNonCodeSpans(code); + return codeOnly.Contains(pattern, StringComparison.Ordinal); + } + + /// + /// Replaces the content of every comment, string literal, and char literal + /// with spaces (newlines preserved), leaving only code regions intact. + /// Delimiter characters themselves are also blanked so a pattern cannot + /// straddle a literal boundary. + /// + private static string BlankNonCodeSpans(string code) + { + var buffer = code.ToCharArray(); + int n = code.Length; + int i = 0; + + void Blank(int from, int to) + { + for (int k = from; k < to && k < n; k++) + if (buffer[k] != '\n' && buffer[k] != '\r') + buffer[k] = ' '; + } + + while (i < n) + { + char c = code[i]; + char next = i + 1 < n ? code[i + 1] : '\0'; + int start = i; + + if (c == '/' && next == '/') + { + i += 2; + while (i < n && code[i] != '\n') i++; + Blank(start, i); + continue; + } + if (c == '/' && next == '*') + { + i += 2; + while (i < n && !(code[i] == '*' && i + 1 < n && code[i + 1] == '/')) i++; + if (i < n) i += 2; + Blank(start, i); + continue; + } + if (c == '"' && next == '"' && i + 2 < n && code[i + 2] == '"') + { + SkipRawString(code, ref i); + Blank(start, i); + continue; + } + if (c == '$') + { + int j = i + 1; + bool verbatim = false; + if (j < n && code[j] == '@') { verbatim = true; j++; } + if (j < n && code[j] == '"') + { + i = j; + SkipInterpolatedString(code, ref i, verbatim); + Blank(start, i); + continue; + } + } + if (c == '@' && next == '"') + { + i++; + SkipVerbatimString(code, ref i); + Blank(start, i); + continue; + } + if (c == '"') + { + SkipRegularString(code, ref i); + Blank(start, i); + continue; + } + if (c == '\'') + { + SkipCharLiteral(code, ref i); + Blank(start, i); + continue; + } + + i++; + } + + return new string(buffer); + } + + /// + /// Walks once and reports the first structural + /// delimiter problem, or when the source is + /// balanced. Delimiters inside comments, strings, and char literals are + /// ignored. + /// + internal static Mismatch Scan(string code) + { + int brace = 0, bracket = 0, paren = 0; + int i = 0; + int n = code.Length; + + while (i < n) + { + char c = code[i]; + char next = i + 1 < n ? code[i + 1] : '\0'; + + // Line comment. + if (c == '/' && next == '/') + { + i += 2; + while (i < n && code[i] != '\n') i++; + continue; + } + + // Block comment. + if (c == '/' && next == '*') + { + i += 2; + bool closed = false; + while (i < n) + { + if (code[i] == '*' && i + 1 < n && code[i + 1] == '/') + { + i += 2; + closed = true; + break; + } + i++; + } + if (!closed) return Mismatch.UnclosedBlockComment; + continue; + } + + // Raw string literal: three or more consecutive quotes open it; the + // same number of quotes closes it. Detected before $/@-prefixed and + // plain strings. + if (c == '"' && next == '"' && i + 2 < n && code[i + 2] == '"') + { + if (!SkipRawString(code, ref i)) return Mismatch.UnterminatedString; + continue; + } + + // Interpolated string ($"..." or $@"..." / @$"..."). + if (c == '$') + { + int j = i + 1; + bool verbatim = false; + if (j < n && code[j] == '@') { verbatim = true; j++; } + if (j < n && code[j] == '"') + { + i = j; + if (!SkipInterpolatedString(code, ref i, verbatim)) return Mismatch.UnterminatedString; + continue; + } + } + + // Verbatim string (@"..."). + if (c == '@' && next == '"') + { + i++; // now on the opening quote + if (!SkipVerbatimString(code, ref i)) return Mismatch.UnterminatedString; + continue; + } + + // Regular string literal. + if (c == '"') + { + if (!SkipRegularString(code, ref i)) return Mismatch.UnterminatedString; + continue; + } + + // Char literal. + if (c == '\'') + { + if (!SkipCharLiteral(code, ref i)) return Mismatch.UnterminatedChar; + continue; + } + + switch (c) + { + case '{': brace++; break; + case '}': + brace--; + if (brace < 0) return Mismatch.UnexpectedCloseBrace; + break; + case '[': bracket++; break; + case ']': + bracket--; + if (bracket < 0) return Mismatch.UnexpectedCloseBracket; + break; + case '(': paren++; break; + case ')': + paren--; + if (paren < 0) return Mismatch.UnexpectedCloseParen; + break; + } + + i++; + } + + if (brace != 0) return Mismatch.UnclosedBrace; + if (bracket != 0) return Mismatch.UnclosedBracket; + if (paren != 0) return Mismatch.UnclosedParen; + return Mismatch.None; + } + + /// + /// Advances past a regular "..." string literal. + /// On entry code[i] == '"'. Returns false if the string is unterminated. + /// + private static bool SkipRegularString(string code, ref int i) + { + int n = code.Length; + i++; // past opening quote + while (i < n) + { + char c = code[i]; + if (c == '\\') { i += 2; continue; } // escape — skip next char + if (c == '\n') return false; // unterminated (no multi-line) + if (c == '"') { i++; return true; } + i++; + } + return false; + } + + /// + /// Advances past a verbatim @"..." string. On entry code[i] == '"'. + /// Inside, \ is literal and "" is an escaped quote. + /// + private static bool SkipVerbatimString(string code, ref int i) + { + int n = code.Length; + i++; // past opening quote + while (i < n) + { + if (code[i] == '"') + { + if (i + 1 < n && code[i + 1] == '"') { i += 2; continue; } // escaped quote + i++; + return true; + } + i++; + } + return false; + } + + /// + /// Advances past an interpolated string. selects + /// the $@"..." escaping rules. Interpolation holes {...} are + /// skipped over (their braces are code, not literal text); {{/}} + /// are escaped braces. On entry code[i] == '"'. + /// + private static bool SkipInterpolatedString(string code, ref int i, bool verbatim) + { + int n = code.Length; + i++; // past opening quote + while (i < n) + { + char c = code[i]; + + if (!verbatim && c == '\\') { i += 2; continue; } + + if (c == '"') + { + if (verbatim && i + 1 < n && code[i + 1] == '"') { i += 2; continue; } + i++; + return true; + } + + if (c == '{') + { + if (i + 1 < n && code[i + 1] == '{') { i += 2; continue; } // escaped brace + // Interpolation hole — skip to the matching '}', tracking nested + // braces so a hole containing an object initializer is handled. + i++; + int depth = 1; + while (i < n && depth > 0) + { + char h = code[i]; + if (h == '{') depth++; + else if (h == '}') depth--; + else if (h == '"') + { + // A nested string inside the hole. + if (!SkipRegularString(code, ref i)) return false; + continue; + } + i++; + } + continue; + } + + if (c == '}' && i + 1 < n && code[i + 1] == '}') { i += 2; continue; } // escaped brace + + i++; + } + return false; + } + + /// + /// Advances past a raw string literal """...""" (C# 11). On entry + /// code[i] is the first of three or more opening quotes. + /// + private static bool SkipRawString(string code, ref int i) + { + int n = code.Length; + int openCount = 0; + while (i < n && code[i] == '"') { openCount++; i++; } + + // Look for a run of the same number of quotes. + while (i < n) + { + if (code[i] == '"') + { + int closeCount = 0; + int start = i; + while (i < n && code[i] == '"') { closeCount++; i++; } + if (closeCount >= openCount) return true; + // Fewer quotes than the opener — they are literal content; keep scanning. + if (closeCount == 0) i = start + 1; + } + else + { + i++; + } + } + return false; + } + + /// + /// Advances past a 'x' char literal. On entry code[i] == '\''. + /// + private static bool SkipCharLiteral(string code, ref int i) + { + int n = code.Length; + i++; // past opening quote + while (i < n) + { + char c = code[i]; + if (c == '\\') { i += 2; continue; } + if (c == '\n') return false; + if (c == '\'') { i++; return true; } + i++; + } + return false; + } +} diff --git a/src/ScadaLink.TemplateEngine/Validation/ScriptCompiler.cs b/src/ScadaLink.TemplateEngine/Validation/ScriptCompiler.cs index c5acc7c..155f47a 100644 --- a/src/ScadaLink.TemplateEngine/Validation/ScriptCompiler.cs +++ b/src/ScadaLink.TemplateEngine/Validation/ScriptCompiler.cs @@ -9,6 +9,18 @@ namespace ScadaLink.TemplateEngine.Validation; /// and enforces the forbidden API list (System.IO, Process, Threading, Reflection, raw network). /// /// For now, this implementation performs basic syntax validation. +/// +/// +/// 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. +/// /// public class ScriptCompiler { @@ -17,6 +29,13 @@ public class ScriptCompiler /// ) 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 = [ @@ -39,10 +58,12 @@ public class ScriptCompiler if (string.IsNullOrWhiteSpace(code)) return Result.Failure($"Script '{scriptName}' has empty code."); - // Check for forbidden APIs + // 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 (code.Contains(pattern, StringComparison.Ordinal)) + if (CSharpDelimiterScanner.ContainsInCode(code, pattern)) { return Result.Failure( $"Script '{scriptName}' uses forbidden API: '{pattern.TrimEnd('.')}'. " + @@ -50,76 +71,35 @@ public class ScriptCompiler } } - // Basic brace matching validation - var braceDepth = 0; - var inString = false; - var inLineComment = false; - var inBlockComment = false; - - for (int i = 0; i < code.Length; i++) + // 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 { - var c = code[i]; - var next = i + 1 < code.Length ? code[i + 1] : '\0'; - - if (inLineComment) - { - if (c == '\n') inLineComment = false; - continue; - } - - if (inBlockComment) - { - if (c == '*' && next == '/') - { - inBlockComment = false; - i++; - } - continue; - } - - if (c == '/' && next == '/') - { - inLineComment = true; - i++; - continue; - } - - if (c == '/' && next == '*') - { - inBlockComment = true; - i++; - continue; - } - - if (c == '"' && !inString) - { - inString = true; - continue; - } - - if (c == '"' && inString) - { - // Check for escaped quote - if (i > 0 && code[i - 1] != '\\') - inString = false; - continue; - } - - if (inString) continue; - - if (c == '{') braceDepth++; - else if (c == '}') braceDepth--; - - if (braceDepth < 0) - return Result.Failure($"Script '{scriptName}' has mismatched braces (unexpected closing brace)."); - } - - if (braceDepth != 0) - return Result.Failure($"Script '{scriptName}' has mismatched braces ({braceDepth} unclosed)."); - - if (inBlockComment) - return Result.Failure($"Script '{scriptName}' has an unclosed block comment."); - - return Result.Success(true); + 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), + }; } } diff --git a/src/ScadaLink.TemplateEngine/Validation/ValidationService.cs b/src/ScadaLink.TemplateEngine/Validation/ValidationService.cs index 4c81246..bc66b7c 100644 --- a/src/ScadaLink.TemplateEngine/Validation/ValidationService.cs +++ b/src/ScadaLink.TemplateEngine/Validation/ValidationService.cs @@ -317,9 +317,12 @@ public class ValidationService /// 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 (expression.Contains(pattern, StringComparison.Ordinal)) + if (CSharpDelimiterScanner.ContainsInCode(expression, pattern)) { return $"uses forbidden API '{pattern.TrimEnd('.')}'. " + "Trigger expressions cannot use System.IO, Process, Threading, Reflection, or raw network APIs."; diff --git a/tests/ScadaLink.TemplateEngine.Tests/Services/InstanceServiceTests.cs b/tests/ScadaLink.TemplateEngine.Tests/Services/InstanceServiceTests.cs index 734f0e1..c3ae1b8 100644 --- a/tests/ScadaLink.TemplateEngine.Tests/Services/InstanceServiceTests.cs +++ b/tests/ScadaLink.TemplateEngine.Tests/Services/InstanceServiceTests.cs @@ -119,6 +119,106 @@ public class InstanceServiceTests Assert.Equal("99", result.Value.OverrideValue); } + // --- TemplateEngine-008 regression: SetAlarmOverrideAsync validation --- + + private static Template TemplateWithAlarms(int id, params TemplateAlarm[] alarms) + { + var t = new Template($"T{id}") { Id = id }; + foreach (var a in alarms) + { + a.TemplateId = id; + t.Alarms.Add(a); + } + return t; + } + + [Fact] + public async Task SetAlarmOverride_NonExistentAlarm_ReturnsFailure() + { + var instance = new Instance("Inst1") { Id = 1, TemplateId = 1 }; + _repoMock.Setup(r => r.GetInstanceByIdAsync(1, It.IsAny())) + .ReturnsAsync(instance); + _repoMock.Setup(r => r.GetAllTemplatesAsync(It.IsAny())) + .ReturnsAsync(new List