fix(security): bundle import runs the script trust gate — forbidden-API scripts rejected at import review, not at runtime

#05-T20. Bundle import is now the fifth ScriptTrustValidator delegating call site. RunSemanticValidationAsync gains a Pass 0 that runs ScriptTrustValidator .FindViolations over every non-Skip template / shared / ApiMethod script body before name resolution; any violation is a HARD error for all kinds (a trust verdict is authoritative, not the false-positive-prone name heuristic) and short-circuits so it is never masked by a Pass-1 name error. DetectBlockersAsync runs the same gate for preview parity, emitting entity-qualified Blocker rows. Transport gains a direct ScriptAnalysis project reference. Docs: Component- ScriptAnalysis (fifth call site) + Component-Transport updated. Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
2026-07-10 02:18:12 -04:00
parent c60347c5e7
commit 523e670579
7 changed files with 197 additions and 5 deletions
@@ -19,7 +19,7 @@
    { "id": 16, "subject": "Task 16: Skip script compilation on read-only staleness/comparison paths", "status": "completed", "blockedBy": [] },
    { "id": 17, "subject": "Task 17: Isolate the post-success deployment audit write", "status": "completed", "blockedBy": [16] },
    { "id": 18, "subject": "Task 18: Delete NotDeployed instances without a site round-trip", "status": "completed", "blockedBy": [17] },
-    { "id": 19, "subject": "Task 19: Import blocker heuristic — local-declaration exclusion, denylist additions, warning severity", "status": "pending", "blockedBy": [6] },
+    { "id": 19, "subject": "Task 19: Import blocker heuristic — local-declaration exclusion, denylist additions, warning severity", "status": "completed", "blockedBy": [6] },
    { "id": 20, "subject": "Task 20: Run the script trust gate on bundle import", "status": "pending", "blockedBy": [19] },
    { "id": 21, "subject": "Task 21: Harden the ScriptTrustPolicy deny-list", "status": "completed", "blockedBy": [] },
    { "id": 22, "subject": "Task 22: Bound LineDiffer input size", "status": "completed", "blockedBy": [] },
@@ -2,13 +2,13 @@

 ## Purpose

-The Script Analysis component is the single authoritative source of truth for the ScadaBridge script trust model. It provides a unified forbidden-API deny-list, a fused semantic and syntactic trust validator, a Roslyn compile wrapper, and compile-only globals stubs used by the design-time deploy gate. All four call sites that enforce the script trust boundary — Template Engine, Site Runtime, Inbound API, and Central UI — delegate to this component rather than maintaining their own divergent implementations.
+The Script Analysis component is the single authoritative source of truth for the ScadaBridge script trust model. It provides a unified forbidden-API deny-list, a fused semantic and syntactic trust validator, a Roslyn compile wrapper, and compile-only globals stubs used by the design-time deploy gate. All five call sites that enforce the script trust boundary — Template Engine, Site Runtime, Inbound API, Central UI, and Transport bundle import — delegate to this component rather than maintaining their own divergent implementations.

 ## Location

 `src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/`

-Referenced by: Template Engine, Site Runtime, Inbound API, Central UI.
+Referenced by: Template Engine, Site Runtime, Inbound API, Central UI, Transport.

 ## Responsibilities

@@ -161,7 +161,7 @@ There is a **third** hand-maintained mirror of the runtime globals: the Central

 ### REQ-SA-5: Consumer Delegation

-All four call sites that previously maintained their own script trust enforcement now delegate to this component. The key behavioral changes per consumer:
+All five call sites that previously maintained their own script trust enforcement (or, for Transport, had none) now delegate to this component. The key behavioral changes per consumer:

 | Consumer | Before | After |
 |----------|--------|-------|
@@ -170,6 +170,7 @@ All four call sites that previously maintained their own script trust enforcemen
 | **Site Runtime** `ScriptCompilationService.ValidateTrustModel` | Semantic resolver, no reflection-gateway hardening | Delegates to `FindViolations`; retains `CSharpScript.Compile` against real `ScriptGlobals` for execution |
 | **Inbound API** `ForbiddenApiChecker.FindViolations` | Syntactic walker, forbade all `System.Diagnostics` | Thin shim delegating to `ScriptTrustValidator.FindViolations`; `System.Diagnostics` loosened to `.Process`-only |
 | **Central UI** `ScriptAnalysisService` | Semantic + full compile, lenient threading | Delegates forbidden-API verdict and sources editor-marker deny-list from `ScriptTrustPolicy`; retains Test-Run execution host |
+| **Transport** `BundleImporter.RunSemanticValidationAsync` / `DetectBlockersAsync` | No trust gate — a bundle's forbidden-API scripts imported clean, deferred to runtime (fifth write path) | Runs `ScriptTrustValidator.FindViolations` over every non-Skip template / shared / ApiMethod body at import review — hard error (apply) / Blocker row (preview) for all kinds |

 The static enforcement is **defence-in-depth**, not a true runtime sandbox. Scripts execute in-process; the denied API list prevents obvious escapes at compile time but does not provide the isolation guarantees of an out-of-process sandbox or a restricted `AssemblyLoadContext`. This caveat applies to all consumers.

@@ -189,3 +190,4 @@ No dependency on Akka.NET, ASP.NET Core, Entity Framework, or any other ScadaBri
 - **Site Runtime (#3)**: `ScriptCompilationService.ValidateTrustModel` delegates the trust verdict to `ScriptTrustValidator.FindViolations`; retains its own `CSharpScript.Compile` against the real `ScriptGlobals` for execution-time compilation.
 - **Inbound API (#14)**: `ForbiddenApiChecker.FindViolations` is a thin shim over `ScriptTrustValidator.FindViolations`.
 - **Central UI (#9)**: `ScriptAnalysisService` delegates the run-gate forbidden-API verdict and sources the editor-marker deny-list from `ScriptTrustPolicy`; retains the Test-Run execution host (`SandboxScriptHost`).
+- **Transport (#24)**: `BundleImporter` runs `ScriptTrustValidator.FindViolations` over every non-Skip template, shared, and ApiMethod script body during import validation (`RunSemanticValidationAsync` Pass 0) and preview (`DetectBlockersAsync`), rejecting forbidden-API scripts at import review rather than at runtime. Trust violations are hard errors for all script kinds (unlike the false-positive-prone name-resolution heuristic, whose template-script findings are advisory).
@@ -26,7 +26,7 @@ As of M8 (T18), Transport is no longer limited to central-only configuration: it
 - Validate `manifest.json` on upload: format version gating, SHA-256 content hash verification.
 - Manage in-memory `BundleSession` objects: 30-minute TTL, 3-strike passphrase lockout per session.
 - Compute a per-artifact diff between bundle contents and the target environment, classifying each artifact as Identical, Modified, New, a Blocker (import-stopping), or a Warning (advisory, non-blocking).
- Apply user-supplied conflict resolutions (Add, Overwrite, Skip, Rename) in a single EF transaction, running two-tier semantic validation before committing: a minimal name-resolution scan over the merged target (fails fast on unresolved SharedScript / ExternalSystem identifiers), then the full `SemanticValidator` from `ZB.MOM.WW.ScadaBridge.TemplateEngine` over each imported template's per-template `FlattenedConfiguration`.
+- Apply user-supplied conflict resolutions (Add, Overwrite, Skip, Rename) in a single EF transaction, running multi-pass semantic validation before committing: a **script trust gate** (`ScriptTrustValidator.FindViolations` over every non-Skip template / shared / ApiMethod body — bundle import is the fifth script-trust call site, so forbidden-API scripts are rejected at import review, not deferred to runtime), then a minimal name-resolution scan over the merged target (ApiMethod findings fail fast on unresolved SharedScript / ExternalSystem identifiers; template-script findings are advisory warnings), then the full `SemanticValidator` from `ZB.MOM.WW.ScadaBridge.TemplateEngine` over each imported template's per-template `FlattenedConfiguration`.
 - Emit `BundleExported`, `BundleImported`, `BundleImportFailed`, `UnencryptedBundleExport`, `BundleImportUnlockFailed`, `BundleImportAlarmScriptUnresolved`, `BundleImportCompositionUnresolved`, and `BundleImportBaseTemplateUnresolved` audit events via `IAuditService`.
 - Thread a `BundleImportId` correlation GUID through every per-entity `AuditLogEntry` written during `ApplyAsync` via a scoped `IAuditCorrelationContext`.
 - Enforce `RequireDesign` on export and `RequireAdmin` on import both at the Razor page layer and inside the service entrypoints (defense in depth).
@@ -23,6 +23,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Types.Transport;
 using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums;
 using ZB.MOM.WW.ScadaBridge.Commons.Types.Flattening;
 using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase;
+using ZB.MOM.WW.ScadaBridge.ScriptAnalysis;
 using ZB.MOM.WW.ScadaBridge.TemplateEngine;
 using ZB.MOM.WW.ScadaBridge.TemplateEngine.Validation;
 using ZB.MOM.WW.ScadaBridge.Transport.Encryption;
@@ -846,6 +847,26 @@ public sealed class BundleImporter : IBundleImporter
                    : $"Script references SharedScript or ExternalSystem '{candidate}' not present in bundle or target — advisory; the deploy-time gate re-validates."));
        }

+        // #05-T20 — script trust gate (preview parity with the apply-time gate).
+        // A forbidden-API verdict is authoritative → a hard Blocker for every
+        // script kind. No resolution map at preview time, so all scripts are
+        // vetted. Blocker Name is entity-qualified so multiple offenders surface
+        // as distinct rows.
+        foreach (var (kind, entityName, scriptLabel, code) in EnumerateTrustGatedScripts(content, resolutionMap: null))
+        {
+            foreach (var violation in ScriptTrustValidator.FindViolations(code))
+            {
+                blockers.Add(new ImportPreviewItem(
+                    EntityType: kind,
+                    Name: $"{entityName}.{scriptLabel}",
+                    ExistingVersion: null,
+                    IncomingVersion: null,
+                    Kind: ConflictKind.Blocker,
+                    FieldDiffJson: null,
+                    BlockerReason: $"Script trust violation — {violation}"));
+            }
+        }
+
        return blockers;
    }

@@ -906,6 +927,51 @@ public sealed class BundleImporter : IBundleImporter
        }
    }

+    /// <summary>
+    /// #05-T20 — enumerates every executable script body the trust gate must
+    /// vet: non-Skip template scripts, shared scripts, and ApiMethod scripts.
+    /// A null <paramref name="resolutionMap"/> (preview time, before resolutions
+    /// are chosen) gates everything. Yields <c>(kind, entityName, scriptLabel,
+    /// code)</c> so callers can format either an error string or a preview row.
+    /// </summary>
+    private static IEnumerable<(string Kind, string EntityName, string ScriptLabel, string Code)> EnumerateTrustGatedScripts(
+        BundleContentDto content,
+        Dictionary<(string, string), ImportResolution>? resolutionMap)
+    {
+        foreach (var t in content.Templates)
+        {
+            if (IsSkipResolution(resolutionMap, "Template", t.Name)) continue;
+            foreach (var s in t.Scripts)
+            {
+                if (!string.IsNullOrEmpty(s.Code))
+                {
+                    yield return ("Template", t.Name, s.Name, s.Code);
+                }
+            }
+        }
+        foreach (var s in content.SharedScripts)
+        {
+            if (IsSkipResolution(resolutionMap, "SharedScript", s.Name)) continue;
+            if (!string.IsNullOrEmpty(s.Code))
+            {
+                yield return ("SharedScript", s.Name, s.Name, s.Code);
+            }
+        }
+        foreach (var m in content.ApiMethods)
+        {
+            if (IsSkipResolution(resolutionMap, "ApiMethod", m.Name)) continue;
+            if (!string.IsNullOrEmpty(m.Script))
+            {
+                yield return ("ApiMethod", m.Name, m.Name, m.Script);
+            }
+        }
+    }
+
+    private static bool IsSkipResolution(
+        Dictionary<(string, string), ImportResolution>? resolutionMap, string entityType, string name)
+        => resolutionMap != null
+            && ResolveOrDefault(resolutionMap, entityType, name).Action == ResolutionAction.Skip;
+
    /// <summary>
    /// Names that look like PascalCase references but are never user-defined
    /// SharedScripts or ExternalSystems. Filters the false-positive noise the
@@ -4176,6 +4242,22 @@ public sealed class BundleImporter : IBundleImporter
        var errors = new List<string>();
        var warnings = new List<string>();

+        // ---- 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.
+        foreach (var (kind, entityName, scriptLabel, code) in EnumerateTrustGatedScripts(content, resolutionMap))
+        {
+            foreach (var violation in ScriptTrustValidator.FindViolations(code))
+            {
+                errors.Add($"{kind} '{entityName}' script '{scriptLabel}': {violation}");
+            }
+        }
+        if (errors.Count > 0) return (errors, warnings);
+
        // ---- Pass 1: minimal name-resolution scan ----

        // Build the known-resolvable set. For in-bundle entries, EXCLUDE the
@@ -16,6 +16,7 @@
  <ItemGroup>
    <ProjectReference Include="../ZB.MOM.WW.ScadaBridge.Commons/ZB.MOM.WW.ScadaBridge.Commons.csproj" />
    <ProjectReference Include="../ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.csproj" />
+    <ProjectReference Include="../ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.csproj" />
    <ProjectReference Include="../ZB.MOM.WW.ScadaBridge.TemplateEngine/ZB.MOM.WW.ScadaBridge.TemplateEngine.csproj" />
  </ItemGroup>

@@ -426,6 +426,38 @@ public sealed class BundleImporterPreviewTests : IDisposable
            && i.BlockerReason.Contains("MissingHelper", StringComparison.Ordinal));
    }

+    [Fact]
+    public async Task PreviewAsync_emits_Blocker_when_template_script_has_forbidden_api()
+    {
+        // #05-T20 — the script trust gate runs in preview too, so a forbidden-API
+        // script surfaces as a Blocker row (parity with the apply-time
+        // SemanticValidationException). Trust violations block regardless of kind.
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            var t = new Template("Malicious") { Description = "forbidden" };
+            t.Scripts.Add(new TemplateScript("init", "System.Diagnostics.Process.Start(\"cmd\");"));
+            ctx.Templates.Add(t);
+            await ctx.SaveChangesAsync();
+        }
+
+        var bundleStream = await ExportTemplatesAsync();
+        var bytes = await StreamToBytes(bundleStream);
+
+        ImportPreview preview;
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var importer = scope.ServiceProvider.GetRequiredService<IBundleImporter>();
+            var session = await importer.LoadAsync(new MemoryStream(bytes), passphrase: null);
+            preview = await importer.PreviewAsync(session.SessionId);
+        }
+
+        Assert.Contains(preview.Items, i =>
+            i.Kind == ConflictKind.Blocker
+            && i.BlockerReason is not null
+            && i.BlockerReason.Contains("Process", StringComparison.Ordinal));
+    }
+
    [Fact]
    public async Task PreviewAsync_does_not_flag_opcua_tag_paths_in_DataSourceReference_as_blockers()
    {
@@ -292,6 +292,81 @@ public sealed class SemanticValidatorImportTests : IDisposable
        }
    }

+    [Fact]
+    public async Task Apply_TemplateScriptWithForbiddenApi_HardBlocks()
+    {
+        // #05-T20 — bundle import is the fifth script-trust call site. A template
+        // script that reaches a forbidden API (Process.Start) must be rejected at
+        // import review, NOT left to fail at runtime. Unlike the name-resolution
+        // heuristic, a trust verdict is authoritative → hard error for ALL kinds.
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            var t = new Template("Malicious");
+            t.Scripts.Add(new Commons.Entities.Templates.TemplateScript(
+                "init",
+                "System.Diagnostics.Process.Start(\"cmd\");"));
+            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<IBundleImporter>();
+            ex = await Assert.ThrowsAsync<SemanticValidationException>(() =>
+                importer.ApplyAsync(sessionId,
+                    new List<ImportResolution>
+                    {
+                        new("Template", "Malicious", ResolutionAction.Add, null),
+                    },
+                    user: "bob"));
+        }
+
+        Assert.NotEmpty(ex.Errors);
+        Assert.Contains(ex.Errors, err => err.Contains("Process", StringComparison.Ordinal));
+
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            Assert.False(await ctx.Templates.AnyAsync(t => t.Name == "Malicious"));
+        }
+    }
+
+    [Fact]
+    public async Task Apply_ApiMethodWithForbiddenApi_HardBlocks()
+    {
+        // #05-T20 — same trust gate over ApiMethod script bodies.
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            ctx.ApiMethods.Add(new Commons.Entities.InboundApi.ApiMethod(
+                "Ingest",
+                "System.Diagnostics.Process.Start(\"cmd\"); return 1;"));
+            await ctx.SaveChangesAsync();
+        }
+
+        var sessionId = await ExportWipeAndLoadAsync();
+
+        SemanticValidationException ex = default!;
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var importer = scope.ServiceProvider.GetRequiredService<IBundleImporter>();
+            ex = await Assert.ThrowsAsync<SemanticValidationException>(() =>
+                importer.ApplyAsync(sessionId,
+                    new List<ImportResolution>
+                    {
+                        new("ApiMethod", "Ingest", ResolutionAction.Add, null),
+                    },
+                    user: "bob"));
+        }
+
+        Assert.NotEmpty(ex.Errors);
+        Assert.Contains(ex.Errors, err => err.Contains("Process", StringComparison.Ordinal));
+    }
+
    [Fact]
    public async Task SemanticValidator_catches_alarm_trigger_type_mismatch_at_import()
    {