merge(r2): r2-plan05

This commit is contained in:
Joseph Doherty
2026-07-13 11:08:27 -04:00
15 changed files with 772 additions and 55 deletions
@@ -5,14 +5,25 @@ using System.Text;
namespace ZB.MOM.WW.ScadaBridge.TemplateEngine.Validation;
/// <summary>
/// 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 <c>ScriptTrustPolicy</c> 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 <c>ScriptTrustPolicy</c> 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.
///
/// <para>
/// The globals surface is part of the key because a verdict is NOT interchangeable
/// across surfaces: a trigger expression that compiles clean against
/// <c>TriggerCompileSurface</c> is not vetted for a <c>ScriptCompileSurface</c>
/// 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.
/// </para>
///
/// <para>
/// The stored error is deliberately <em>name-free</em> — 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;
/// <summary>
/// Returns the cached verdict for <paramref name="code"/>, or computes it via
/// <paramref name="factory"/> and caches it on a miss. A hit increments
/// <see cref="Hits"/>. 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 (<paramref name="surface"/>,
/// <paramref name="code"/>), or computes it via <paramref name="factory"/> and
/// caches it on a miss. A hit increments <see cref="Hits"/>. The verdict's error
/// text must be name-free — the caller formats the script name in on return.
/// </summary>
/// <param name="surface">
/// The globals-surface discriminator (e.g. <c>nameof(ScriptCompileSurface)</c> or
/// <c>nameof(TriggerCompileSurface)</c>). Part of the key so a verdict computed
/// against one surface is never reused for another.
/// </param>
/// <param name="code">The script source code to look up (hashed to form the cache key).</param>
/// <param name="factory">Computes the verdict on a cache miss.</param>
/// <returns>The cached or newly computed compile verdict.</returns>
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))
{
@@ -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
@@ -97,11 +97,16 @@ public class ValidationService
/// </param>
/// <param name="validateScriptCompilation">
/// <c>true</c> (default) runs the authoritative Roslyn <see cref="ValidateScriptCompilation"/>
/// stage — the single most expensive check (a non-collectible assembly load per script).
/// <c>false</c> 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 <c>false</c> because they need only the flattened config and
/// revision hash, not a compile. The deploy gate keeps the default (<c>true</c>).
/// stage — the single most expensive check (a non-collectible assembly load per script)
/// AND the Expression-trigger syntax/compile check
/// (<see cref="CheckExpressionSyntax"/>, a forbidden-API verdict plus a real compile of every
/// Expression-triggered script/alarm body against <see cref="TriggerCompileSurface"/>).
/// <c>false</c> 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 <c>false</c> because they need only
/// the flattened config and revision hash, not a compile. The deploy gate keeps the default
/// (<c>true</c>) — it is the authoritative check for both stages.
/// </param>
/// <returns>A merged <see cref="ValidationResult"/> aggregating all pipeline stage outcomes.</returns>
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
/// </list>
/// </summary>
/// <param name="configuration">The flattened configuration to validate.</param>
/// <param name="validateExpressionSyntax">
/// <c>true</c> (default) runs the authoritative <see cref="CheckExpressionSyntax"/>
/// forbidden-API + compile stage for each Expression trigger. <c>false</c> 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.
/// </param>
/// <returns>A <see cref="ValidationResult"/> with errors and warnings from all expression trigger checks.</returns>
public static ValidationResult ValidateExpressionTriggers(FlattenedConfiguration configuration)
public static ValidationResult ValidateExpressionTriggers(
FlattenedConfiguration configuration,
bool validateExpressionSyntax = true)
{
var errors = new List<ValidationEntry>();
var warnings = new List<ValidationEntry>();
@@ -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<string> attributeNames,
List<ValidationEntry> errors,
List<ValidationEntry> warnings)
List<ValidationEntry> 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))
@@ -530,17 +552,27 @@ public class ValidationService
/// <returns>A human-readable error message if the expression is invalid; <c>null</c> if well-formed.</returns>
internal static string? CheckExpressionSyntax(string expression)
{
// Authoritative forbidden-API verdict first.
var violations = ScriptTrustValidator.FindViolations(expression);
if (violations.Count > 0)
return $"uses forbidden API: {violations[0]}";
// 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: {errors[0]}";
// 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;
}
/// <summary>
@@ -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<string>(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;
}
@@ -946,8 +963,11 @@ public sealed class BundleImporter : IBundleImporter
/// <summary>
/// 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
/// (<c>InstanceAlarmOverrideDto.TriggerConfigurationOverride</c>). 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 <paramref name="resolutionMap"/> (preview time, before
/// resolutions are chosen) gates everything. Yields <c>(kind, entityName,
@@ -998,6 +1018,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);
}
}
}
}
/// <summary>
@@ -1009,8 +1048,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);
}
/// <summary>
/// Extracts the C# boolean expression from an <c>{"expression":"…"}</c> 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 <c>"expression"</c> string key (the
/// structured HiLo/threshold shapes) or malformed JSON yields null — nothing to
/// gate.
/// </summary>
private static string? ExtractExpressionBody(string? triggerConfigJson)
{
if (string.IsNullOrWhiteSpace(triggerConfigJson))
{
return null;
}
@@ -1037,6 +1092,85 @@ public sealed class BundleImporter : IBundleImporter
=> resolutionMap != null
&& ResolveOrDefault(resolutionMap, entityType, name).Action == ResolutionAction.Skip;
/// <summary>
/// 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).
/// <para>
/// Matches locked members by DIRECT name only — own + inherited placeholder rows
/// both carry the <c>IsLocked</c> flag, so a lock on either matches. Overrides
/// addressing composed path-qualified members (<c>y1.z.Val</c>) or derived-shadow
/// locks (<c>LockedInDerived</c> 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.
/// </para>
/// </summary>
private static IReadOnlyList<(string Instance, string Message)> CollectLockedOverrideWarnings(
BundleContentDto content,
Dictionary<(string, string), ImportResolution>? resolutionMap,
IReadOnlyList<Template> targetTemplates)
{
var warnings = new List<(string, string)>();
var bundleTemplates = new Dictionary<string, TemplateDto>(StringComparer.Ordinal);
foreach (var t in content.Templates) bundleTemplates[t.Name] = t;
var targetByName = new Dictionary<string, Template>(StringComparer.Ordinal);
foreach (var t in targetTemplates) targetByName[t.Name] = t;
foreach (var inst in content.Instances)
{
if (IsSkipResolution(resolutionMap, "Instance", inst.UniqueName)) continue;
if (string.IsNullOrEmpty(inst.TemplateName)) continue;
HashSet<string> lockedAttrs;
HashSet<string> lockedAlarms;
HashSet<string> lockedSources;
if (bundleTemplates.TryGetValue(inst.TemplateName, out var dto))
{
lockedAttrs = new(dto.Attributes.Where(a => a.IsLocked).Select(a => a.Name), StringComparer.Ordinal);
lockedAlarms = new(dto.Alarms.Where(a => a.IsLocked).Select(a => a.Name), StringComparer.Ordinal);
lockedSources = new(dto.NativeAlarmSources.Where(s => s.IsLocked).Select(s => s.Name), StringComparer.Ordinal);
}
else if (targetByName.TryGetValue(inst.TemplateName, out var tgt))
{
lockedAttrs = new(tgt.Attributes.Where(a => a.IsLocked).Select(a => a.Name), StringComparer.Ordinal);
lockedAlarms = new(tgt.Alarms.Where(a => a.IsLocked).Select(a => a.Name), StringComparer.Ordinal);
lockedSources = new(tgt.NativeAlarmSources.Where(s => s.IsLocked).Select(s => s.Name), StringComparer.Ordinal);
}
else
{
// Template unresolvable — a separate blocker/validation covers that.
continue;
}
foreach (var o in inst.AttributeOverrides)
{
if (lockedAttrs.Contains(o.AttributeName))
warnings.Add((inst.UniqueName, LockedOverrideMessage(inst.UniqueName, "attribute", o.AttributeName)));
}
foreach (var o in inst.AlarmOverrides)
{
if (lockedAlarms.Contains(o.AlarmCanonicalName))
warnings.Add((inst.UniqueName, LockedOverrideMessage(inst.UniqueName, "alarm", o.AlarmCanonicalName)));
}
foreach (var o in inst.NativeAlarmSourceOverrides)
{
if (lockedSources.Contains(o.SourceCanonicalName))
warnings.Add((inst.UniqueName, LockedOverrideMessage(inst.UniqueName, "native-alarm-source", o.SourceCanonicalName)));
}
}
return warnings;
}
private static string LockedOverrideMessage(string unique, string kind, string name) =>
$"Instance '{unique}' {kind} override '{name}' targets a LOCKED template member — " +
"the flattener ignores it, so this part of the bundle's instance config will never take effect.";
/// <summary>
/// Names that look like PascalCase references but are never user-defined
/// SharedScripts or ExternalSystems. Filters the false-positive noise the
@@ -1204,7 +1338,7 @@ public sealed class BundleImporter : IBundleImporter
if (validationWarnings.Count > 0)
{
_logger?.LogWarning(
"Bundle import {BundleImportId}: {Count} advisory template-script reference warning(s): {Warnings}",
"Bundle import {BundleImportId}: {Count} advisory import warning(s): {Warnings}",
bundleImportId, validationWarnings.Count, string.Join("; ", validationWarnings));
}
@@ -1767,12 +1901,15 @@ public sealed class BundleImporter : IBundleImporter
/// </summary>
/// <summary>
/// Publishes one <see cref="ScriptArtifactsChanged"/> 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/<c>_knownBadMethods</c> 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).
/// </summary>
private void PublishScriptArtifactChanges(IReadOnlyList<ImportResolution> resolutions)
{
@@ -1799,7 +1936,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();
@@ -4334,6 +4471,15 @@ public sealed class BundleImporter : IBundleImporter
// (Pass 0 emits only errors), so nothing surfacable is dropped.
if (errors.Count > 0) return (errors, warnings);
// N4: advisory warnings for instance overrides that target a LOCKED template
// member. Best-effort, non-blocking (rides ImportResult.Warnings); the row is
// still written (bundle fidelity) — the flattener is the behavioural authority.
var lockScanTemplates = await _templateRepo.GetAllTemplatesAsync(ct).ConfigureAwait(false);
foreach (var (_, message) in CollectLockedOverrideWarnings(content, resolutionMap, lockScanTemplates))
{
warnings.Add(message);
}
// ---- Pass 1: minimal name-resolution scan ----
// Build the known-resolvable set. For in-bundle entries, EXCLUDE the
@@ -48,6 +48,10 @@ public sealed class TransportOptionsValidator : OptionsValidatorBase<TransportOp
$"ScadaBridge:Transport:MaxUnlockAttemptsPerSession must be positive " +
$"(was {options.MaxUnlockAttemptsPerSession}); it caps failed passphrase attempts before a session locks.");
builder.RequireThat(options.MaxConcurrentImportSessions > 0,
$"ScadaBridge:Transport:MaxConcurrentImportSessions must be positive " +
$"(was {options.MaxConcurrentImportSessions}); a zero/negative cap rejects every import session forever.");
builder.RequireThat(options.MaxUnlockAttemptsPerIpPerHour > 0,
$"ScadaBridge:Transport:MaxUnlockAttemptsPerIpPerHour must be positive " +
$"(was {options.MaxUnlockAttemptsPerIpPerHour}); it caps unlock attempts per IP address per hour.");