diff --git a/docs/requirements/Component-Transport.md b/docs/requirements/Component-Transport.md index b470aea2..2bf921ed 100644 --- a/docs/requirements/Component-Transport.md +++ b/docs/requirements/Component-Transport.md @@ -136,7 +136,7 @@ flowchart TD The component is central-hosted. It is registered in `ZB.MOM.WW.ScadaBridge.Host` for central roles only, never for site roles — even when a bundle carries site-scoped config (T18), Transport only writes the central configuration database, never the site clusters. All persistence flows through existing audited repository interfaces in `ZB.MOM.WW.ScadaBridge.ConfigurationDatabase` — the component does not call `DbContext.SaveChangesAsync` directly. `BundleSessionStore` is in-process on the active central node (matching Blazor Server circuit affinity): 30-minute TTL, eviction on expiry, 3-strike passphrase lockout per session, and a `MaxConcurrentImportSessions` cap (default 8; arch-review 05 Task 24 — startup-validated as `> 0` by `TransportOptionsValidator`, since a zero/negative cap would reject every import session forever) bounding the decrypted-content footprint since each open session pins a fully-decrypted bundle in memory. **The cap is observable (register row 53):** open (unexpired) sessions are published as the `scadabridge.central.transport.import_sessions.open` gauge on the `ZB.MOM.WW.ScadaBridge` meter — already in the `ObservedMeters` allowlist, so no telemetry config change is needed — and a rejection at the cap logs at Warning with the current count, the cap and the TTL. Reclaiming a store full of *expired* sessions is not a cap hit and logs nothing. **A log line is not alertable, so register row 66 adds the counter `scadabridge.central.transport.import_sessions.rejected`** on the same meter, incremented at the same single genuine-rejection point (the post-eviction recheck — never on the reclaim-then-succeed path, and never when re-opening an already-open session id). Alert on the counter; interpret it against the gauge — rejections rising while the gauge stays below the cap means genuine concurrency (raise the cap), whereas rejections rising while the gauge is pinned at the cap means a leaked session (raising the cap only defers it). -**Session release is explicit, not TTL-only (register row 43).** `ApplyAsync` releases the session it applied (zero-then-remove, T-007), and `IBundleImporter.Abandon(sessionId)` does the same for a session that will never be applied. Every abandonment path calls it: the Central UI wizard on new upload / Back-to-Upload / `ResetSessionState` / lockout reset / `Dispose` (navigation away or circuit teardown), the CLI `transport preview` handler (a preview never applies), and the CLI `transport import` handler for the aborts that precede apply (blocker rows, unresolved mappings). `Abandon` is idempotent — double release, an unknown id, and an already-expired id are all silent no-ops — and since session ids are random GUIDs handed only to their opener, abandoning the id you hold can never evict another operator's in-progress import. Relying on the TTL alone was an availability *and* secret-lifetime defect: eight abandoned wizards exhausted `MaxConcurrentImportSessions` and locked out every import on the active node for 30 minutes, each still holding its decrypted bundle content. +**Session release is explicit, not TTL-only (register row 43).** `ApplyAsync` releases the session it applied (zero-then-remove, T-007), and `IBundleImporter.Abandon(sessionId)` does the same for a session that will never be applied. Every abandonment path calls it: the Central UI wizard on new upload / Back-to-Upload / `ResetSessionState` / lockout reset / `Dispose` (navigation away or circuit teardown), the CLI `transport preview` handler (a preview never applies), and the CLI `transport import` handler for the aborts that precede apply (blocker rows, unresolved mappings). `Abandon` is idempotent — double release, an unknown id, and an already-expired id are all silent no-ops — and since session ids are random GUIDs handed only to their opener, abandoning the id you hold can never evict another operator's in-progress import. Relying on the TTL alone was an availability *and* secret-lifetime defect: eight abandoned wizards exhausted `MaxConcurrentImportSessions` and locked out every import on the active node for 30 minutes, each still holding its decrypted bundle content. **TTL expiry zeroes too (review 2026-10-21 04/C3):** a session dropped because it expired — lazily on `Get`, or by the `EvictExpired` sweep — has its content buffer zeroed by `BundleSessionStore` as it is removed, so every release path (apply, abandon, expiry) follows the same zero-then-remove discipline. **Preview→apply window (recorded decision).** The diff/preview an operator reviews is a point-in-time snapshot; there is **no optimistic concurrency** between preview and apply. If the target config changes in the window between `PreviewAsync` and `ApplyAsync` (another admin edits a template, say), the apply proceeds against the current state — the resolutions are keyed by `(EntityType, Name)`, so they still target the right rows, but the operator may not have seen the latest state in the diff. This is accepted for v1 (single-operator import is the expected workflow, and the terminal Confirm step already gates a typo-resistant environment-name entry). The bundle-format `version` fields are reserved for a future optimistic-concurrency check but are not yet enforced (Underdeveloped 5). diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleSessionStore.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleSessionStore.cs index fd713a69..8a1eed1c 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleSessionStore.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleSessionStore.cs @@ -153,7 +153,10 @@ public sealed class BundleSessionStore : IBundleSessionStore { if (!_sessions.TryGetValue(sessionId, out var session)) return null; if (session.ExpiresAt > _timeProvider.GetUtcNow()) return session; - _sessions.TryRemove(sessionId, out _); + if (_sessions.TryRemove(KeyValuePair.Create(sessionId, session))) + { + ZeroContent(session); + } return null; } @@ -169,9 +172,9 @@ public sealed class BundleSessionStore : IBundleSessionStore var now = _timeProvider.GetUtcNow(); foreach (var kv in _sessions) { - if (kv.Value.ExpiresAt <= now) + if (kv.Value.ExpiresAt <= now && _sessions.TryRemove(kv)) { - _sessions.TryRemove(kv.Key, out _); + ZeroContent(kv.Value); } } @@ -186,6 +189,24 @@ public sealed class BundleSessionStore : IBundleSessionStore } } + /// + /// Review 2026-10-21 04/C3: zeroes an expired session's content buffer as it is + /// dropped. ApplyAsync/Abandon already zero before removing (T-007); + /// without this, a session left to expire kept its (compressed) decrypted bundle + /// content — script bodies, configuration and the bundle's secret values — in + /// managed memory until the GC reclaimed the array. Only the instance this store + /// actually removed is zeroed, so a concurrent re- under the same + /// id is never touched. + /// + private static void ZeroContent(BundleSession session) + { + var buf = session.DecryptedContent; + if (buf is { Length: > 0 }) + { + Array.Clear(buf, 0, buf.Length); + } + } + /// public int GetUnlockFailureCount(string bundleContentHash) { 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 624263c5..67ee3745 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleSessionStoreTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleSessionStoreTests.cs @@ -130,6 +130,57 @@ public sealed class BundleSessionStoreTests Assert.Null(sut.Get(dropB.SessionId)); } + /// + /// Review 2026-10-21 04/C3: a session dropped by TTL expiry on the Get path + /// has its content buffer zeroed, matching the zero-then-remove discipline that + /// ApplyAsync/Abandon already follow. + /// + [Fact] + public void Get_after_TTL_zeroes_the_content_buffer() + { + var clock = new TestTimeProvider(DateTimeOffset.UtcNow); + var sut = new BundleSessionStore(Options(), clock); + var content = new byte[] { 0x11, 0x22, 0x33 }; + var session = SessionWithContent(clock.GetUtcNow().AddMinutes(30), content); + sut.Open(session); + + clock.Advance(TimeSpan.FromMinutes(31)); + + Assert.Null(sut.Get(session.SessionId)); + Assert.All(content, b => Assert.Equal(0, b)); + } + + /// + /// Review 2026-10-21 04/C3: EvictExpired zeroes the content buffer of each + /// session it drops, and leaves a live session's buffer untouched. + /// + [Fact] + public void EvictExpired_zeroes_the_content_of_expired_sessions_only() + { + var clock = new TestTimeProvider(DateTimeOffset.UtcNow); + var sut = new BundleSessionStore(Options(), clock); + var expiredContent = new byte[] { 0x11, 0x22 }; + var liveContent = new byte[] { 0x33, 0x44 }; + sut.Open(SessionWithContent(clock.GetUtcNow().AddMinutes(5), expiredContent)); + var live = SessionWithContent(clock.GetUtcNow().AddMinutes(60), liveContent); + sut.Open(live); + + clock.Advance(TimeSpan.FromMinutes(10)); + sut.EvictExpired(); + + Assert.All(expiredContent, b => Assert.Equal(0, b)); + Assert.Equal(new byte[] { 0x33, 0x44 }, liveContent); + Assert.NotNull(sut.Get(live.SessionId)); + } + + private static BundleSession SessionWithContent(DateTimeOffset expiresAt, byte[] content) => new() + { + SessionId = Guid.NewGuid(), + Manifest = StubManifest(), + DecryptedContent = content, + ExpiresAt = expiresAt, + }; + /// Verifies that the legacy failed-unlock-attempts field still round-trips through the shared reference. [Fact] public void Legacy_FailedUnlockAttempts_field_still_round_trips_via_shared_reference()