From 0642d3b9889f7b04e6845e690c77f5194346d8b0 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 10 Jul 2026 02:32:37 -0400 Subject: [PATCH] =?UTF-8?q?chore:=20low-severity=20sweep=20=E2=80=94=20imp?= =?UTF-8?q?ort=20session=20cap,=20full=20compile-error=20lists,=20leaf-nam?= =?UTF-8?q?e-fallback=20validation=20warning?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #05-T24. Three low-severity cleanups, each TDD'd: - TransportOptions.MaxConcurrentImportSessions (8): BundleSessionStore.Open rejects a new session past the cap (evicting expired entries first), bounding the N×~200 MB decrypted-content footprint. Soft cap (count/add not atomic). - ScriptCompiler.TryCompile now joins ALL forbidden-API violations / compile errors (string.Join instead of [0]) so an operator sees every problem at once; the T15 verdict cache stores the joined string. - SemanticValidator emits an advisory warning when a CallScript target resolves ONLY via the composed leaf-name fallback (dynamic child path, not statically verified) instead of silently accepting it. Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj --- .../Validation/ScriptCompiler.cs | 8 ++-- .../Validation/SemanticValidator.cs | 12 +++++ .../Import/BundleSessionStore.cs | 20 +++++++++ .../TransportOptions.cs | 7 +++ .../Validation/ScriptCompilerTests.cs | 13 ++++++ .../Validation/SemanticValidatorTests.cs | 44 +++++++++++++++++++ .../Import/BundleSessionStoreTests.cs | 30 +++++++++++++ 7 files changed, 131 insertions(+), 3 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompiler.cs b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompiler.cs index 4e23e64c..ee35a833 100644 --- a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompiler.cs +++ b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompiler.cs @@ -42,15 +42,17 @@ public class ScriptCompiler var (ok, error) = ScriptCompileVerdictCache.GetOrAdd(code, () => { // Authoritative forbidden-API verdict first — a security violation must - // gate the script regardless of whether it otherwise compiles. + // gate the script regardless of whether it otherwise compiles. Report + // ALL violations (#05-T24), not just the first, so an operator fixing + // one forbidden API isn't surprised by a second on the next attempt. var violations = ScriptTrustValidator.FindViolations(code); if (violations.Count > 0) - return (false, $"uses forbidden API: {violations[0]}"); + return (false, $"uses forbidden API: {string.Join("; ", violations)}"); // Real CSharpScript compile against the runtime-mirroring globals surface. var errors = RoslynScriptCompiler.Compile(code, typeof(ScriptCompileSurface)); if (errors.Count > 0) - return (false, $"failed to compile: {errors[0]}"); + return (false, $"failed to compile: {string.Join("; ", errors)}"); return (true, (string?)null); }); diff --git a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/SemanticValidator.cs b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/SemanticValidator.cs index 883e6c4b..69324088 100644 --- a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/SemanticValidator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/SemanticValidator.cs @@ -119,6 +119,18 @@ public class SemanticValidator } else { + // #05-T24: if the target resolved ONLY via the composed + // leaf-name fallback (not a same-scope sibling), the child + // path is dynamic and cannot be statically verified — surface + // an advisory warning instead of accepting it silently. + if (!scriptNames.Contains(call.TargetName) + && composedLeafNames.Contains(call.TargetName)) + { + warnings.Add(ValidationEntry.Warning(ValidationCategory.CallTargetNotFound, + $"Script '{script.CanonicalName}' call target '{call.TargetName}' matched by composed leaf name only — child path not verified.", + script.CanonicalName)); + } + ValidateCallParameters(script.CanonicalName, call, scriptParamMap, errors); ValidateCallReturnType(script.CanonicalName, call, scriptReturnMap, errors); diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleSessionStore.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleSessionStore.cs index a669c767..54f78b05 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleSessionStore.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleSessionStore.cs @@ -73,6 +73,26 @@ public sealed class BundleSessionStore : IBundleSessionStore public BundleSession Open(BundleSession session) { ArgumentNullException.ThrowIfNull(session); + // #05-T24: bound the number of concurrently-open sessions — each pins a + // decrypted bundle (up to MaxBundleSizeMb of plaintext) in memory until + // it expires or is applied/cancelled. Re-opening an existing id (overwrite) + // never grows the count, so only a genuinely-new session is gated. A soft + // cap: the count/add pair is not atomic, so a race may briefly exceed it + // by one — acceptable for a memory-pressure guard, not a security invariant. + if (!_sessions.ContainsKey(session.SessionId)) + { + var cap = _options.Value.MaxConcurrentImportSessions; + if (_sessions.Count >= cap) + { + // A full store may be full only of stale entries — reclaim first. + EvictExpired(); + if (_sessions.Count >= cap) + { + throw new InvalidOperationException( + $"Too many concurrent import sessions ({cap}). Complete or cancel an in-progress import before starting another."); + } + } + } // Overwrite on collision is defensive: GUIDs are random so practical // collisions don't happen, but if a caller reuses an id we always // honor the latest Open call. diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/TransportOptions.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/TransportOptions.cs index 265580b5..b8165cb2 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/TransportOptions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/TransportOptions.cs @@ -28,6 +28,13 @@ public sealed class TransportOptions /// generous headroom for unusually compressible bundles. /// public int MaxBundleEntryCompressionRatio { get; set; } = 50; + /// + /// #05-T24: maximum number of concurrently-open import sessions. Each open + /// session pins a fully-decrypted bundle (up to + /// of plaintext) in memory until it expires or is applied/cancelled, so this + /// bounds the N×~200 MB decrypted-content footprint the central node can hold. + /// + public int MaxConcurrentImportSessions { get; set; } = 8; /// Gets or sets the maximum number of failed passphrase unlock attempts before a session is locked. public int MaxUnlockAttemptsPerSession { get; set; } = 3; /// Gets or sets the maximum number of unlock attempts allowed per IP address per hour. 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 1405fbd3..17df857d 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompilerTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompilerTests.cs @@ -40,6 +40,19 @@ public class ScriptCompilerTests Assert.Contains("forbidden", result.Error, StringComparison.OrdinalIgnoreCase); } + [Fact] + public void TryCompile_MultipleForbiddenApis_ReportsAll() + { + // #05-T24: the failure message must enumerate EVERY forbidden API, not + // just the first, so an operator fixing one isn't surprised by the next. + var result = _sut.TryCompile( + "System.IO.File.ReadAllText(\"x\"); System.Diagnostics.Process.Start(\"y\");", + "Test"); + Assert.True(result.IsFailure); + Assert.Contains("System.IO", result.Error, StringComparison.Ordinal); + Assert.Contains("Process", result.Error, StringComparison.Ordinal); + } + [Theory] // Namespace alias — the old substring scan never saw "System.IO." spelled out. [InlineData("using IO = System.IO; IO.File.ReadAllText(\"x\");")] diff --git a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/SemanticValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/SemanticValidatorTests.cs index 2eaa4111..c10c57e5 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/SemanticValidatorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/SemanticValidatorTests.cs @@ -105,6 +105,50 @@ public class SemanticValidatorTests Assert.DoesNotContain(result.Errors, e => e.Category == ValidationCategory.CallTargetNotFound); } + [Fact] + public void Validate_CallScriptCompositionDelegatedByLeafName_EmitsAdvisoryWarning() + { + // #05-T24: a call target accepted ONLY by the composed leaf-name fallback + // (dynamic child path, not statically verifiable) must surface an advisory + // warning rather than being silently accepted. + var config = new FlattenedConfiguration + { + InstanceUniqueName = "Instance1", + Scripts = + [ + new ResolvedScript { CanonicalName = "LeftMESReceiver.MoveIn", Code = "var x = 1;" }, + new ResolvedScript { CanonicalName = "MesMoveIn", Code = "await Children[side + \"MESReceiver\"].CallScript(\"MoveIn\", Parameters);" } + ] + }; + + var result = _sut.Validate(config); + + Assert.Contains(result.Warnings, w => + w.Category == ValidationCategory.CallTargetNotFound + && w.Message.Contains("leaf name only", StringComparison.Ordinal)); + } + + [Fact] + public void Validate_CallScriptSameScopeSibling_EmitsNoLeafWarning() + { + // A same-scope sibling resolves directly (not the leaf fallback), so no + // advisory warning is emitted. + var config = new FlattenedConfiguration + { + InstanceUniqueName = "Instance1", + Scripts = + [ + new ResolvedScript { CanonicalName = "Target", Code = "var x = 1;" }, + new ResolvedScript { CanonicalName = "Caller", Code = "CallScript(\"Target\");" } + ] + }; + + var result = _sut.Validate(config); + + Assert.DoesNotContain(result.Warnings, w => + w.Message.Contains("leaf name only", StringComparison.Ordinal)); + } + [Fact] public void Validate_CallScriptUnknownLeafName_StillReturnsError() { diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleSessionStoreTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleSessionStoreTests.cs index 91d77e6b..de4e64e4 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleSessionStoreTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleSessionStoreTests.cs @@ -29,6 +29,36 @@ public sealed class BundleSessionStoreTests Summary: new BundleSummary(0, 0, 0, 0, 0, 0, 0, 0), Contents: Array.Empty()); + [Fact] + public void Open_beyond_MaxConcurrentImportSessions_throws_and_removing_one_admits_a_new_session() + { + // #05-T24: the store bounds concurrently-open sessions (each pins a + // decrypted bundle in memory). The (cap+1)th Open is rejected; freeing a + // slot re-admits a new session. + var clock = new TestTimeProvider(DateTimeOffset.UtcNow); + var opts = Options(); + var cap = opts.Value.MaxConcurrentImportSessions; + var sut = new BundleSessionStore(opts, clock); + var future = clock.GetUtcNow().AddMinutes(30); + + var sessions = new List(); + for (var i = 0; i < cap; i++) + { + var s = SessionExpiringAt(future); + sut.Open(s); + sessions.Add(s); + } + + var overflow = SessionExpiringAt(future); + var ex = Assert.Throws(() => sut.Open(overflow)); + Assert.Contains("Too many concurrent import sessions", ex.Message, StringComparison.Ordinal); + + // Free a slot → the previously-rejected session is admitted. + sut.Remove(sessions[0].SessionId); + sut.Open(overflow); + Assert.NotNull(sut.Get(overflow.SessionId)); + } + [Fact] public void Open_then_Get_returns_session() {