From 708389829bad0dac1a667e6ad7d16f5194326aaa Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 05:42:08 -0400 Subject: [PATCH 1/2] fix(transport): release abandoned BundleSessions instead of leaking a slot (row 43) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only BundleImporter.ApplyAsync released an import session, so every path that opened one and did NOT apply held one of MaxConcurrentImportSessions (8) — and the DECRYPTED bundle content that T-007 deliberately zeroes on the apply paths — for the full BundleSessionTtlMinutes (30). Eight abandonments locked out all imports on the active node; observed live (R6: 3 back-to-back suite runs held 6 of 8 slots until central-b was restarted). New IBundleImporter.Abandon(sessionId): the same zero-then-remove release ApplyAsync performs, idempotent (double release / unknown id / expired id are no-ops) and only ever given the random-GUID id its caller opened, so it can never evict another circuit's session. Wired into every abandonment path: - Central UI wizard — ReleaseSession() from new upload, TryLoadAsync (before it overwrites _session), Back-to-Upload / ResetSessionState, the 3-strike lockout reset, and Dispose (navigation away / circuit teardown). - CLI transport preview — a preview never applies, so nothing released it at all; now released in a finally. - CLI transport import — covers the pre-apply aborts (blocker rows, unresolved site/connection mappings). Class doc's "an abandoned wizard does not leak" claim is true again (it only ever covered the staged plaintext temp file). Component-Transport.md + docs/components/Transport.md now record the explicit-release contract and that BundleSessionEvictionService is a TTL backstop, not an abandonment path. Tests: 4 bUnit tests over a REAL BundleSessionStore (dispose / back-to-upload / apply-then-dispose idempotency / concurrent-circuit isolation) — 3 of the 4 fail against the pre-fix page; 4 importer tests (release + zeroing, idempotency, cross-session isolation, and the slot-cap regression). --- docs/components/Transport.md | 10 +- docs/requirements/Component-Transport.md | 4 +- .../Pages/Design/TransportImport.razor.cs | 60 +++++- .../Interfaces/Transport/IBundleImporter.cs | 21 ++ .../ManagementActor.cs | 123 +++++++----- .../Import/BundleImporter.cs | 26 +++ .../Pages/Design/TransportImportPageTests.cs | 186 ++++++++++++++++++ .../Import/BundleImporterLoadTests.cs | 86 ++++++++ 8 files changed, 459 insertions(+), 57 deletions(-) diff --git a/docs/components/Transport.md b/docs/components/Transport.md index ff002939..cddd079c 100644 --- a/docs/components/Transport.md +++ b/docs/components/Transport.md @@ -39,7 +39,15 @@ When a passphrase is supplied, `BundleSecretEncryptor` derives a 256-bit key via ### BundleSession and session lifecycle -After `LoadAsync` validates and decrypts a bundle, the plaintext content is stored in a `BundleSession` held by the singleton `BundleSessionStore` (a `ConcurrentDictionary`). Sessions have a 30-minute TTL (`BundleSessionTtlMinutes`). `BundleSessionEvictionService` sweeps the store every minute so abandoned sessions — and the secrets they carry — are released without waiting for the next `Get` call. `ApplyAsync` explicitly zeros and removes the session on both success and failure (T-007). +After `LoadAsync` validates and decrypts a bundle, the plaintext content is stored in a `BundleSession` held by the singleton `BundleSessionStore` (a `ConcurrentDictionary`). Sessions have a 30-minute TTL (`BundleSessionTtlMinutes`). `BundleSessionEvictionService` sweeps the store every minute, but that only reclaims sessions whose TTL has already **elapsed** — it is a backstop, not an abandonment path. `ApplyAsync` explicitly zeros and removes the session on both success and failure (T-007). + +`IBundleImporter.Abandon(sessionId)` is the explicit release for a session that will never be applied, applying the same zero-then-remove ordering as `ApplyAsync`. It is idempotent (double release, unknown id, and already-expired id are all silent no-ops) and, because session ids are random GUIDs handed only to their opener, can never release another caller's session. Every abandonment path calls it: + +- Central UI wizard — new upload, Back-to-Upload / `ResetSessionState`, the 3-strike lockout reset, and `Dispose` (navigation away or circuit teardown). +- CLI `transport preview` — a preview never applies, so the handler releases in a `finally`. +- CLI `transport import` — covers the aborts that happen before `ApplyAsync` (blocker rows, unresolved site/connection mappings). + +Without this, an abandoned wizard or a plain CLI preview held one of the eight `MaxConcurrentImportSessions` slots — and the decrypted bundle content — for the full TTL; eight of them locked out every import on the active node (deferred register row 43, observed live). ### Conflict resolution and `BundleImportId` correlation diff --git a/docs/requirements/Component-Transport.md b/docs/requirements/Component-Transport.md index 5a67c215..2b150c16 100644 --- a/docs/requirements/Component-Transport.md +++ b/docs/requirements/Component-Transport.md @@ -112,7 +112,7 @@ The manifest is plaintext so the import wizard can preview bundle contents and s flowchart TD subgraph T["ZB.MOM.WW.ScadaBridge.Transport"] EXPORTER["IBundleExporter
ExportAsync(ExportSelection, Passphrase?, ct) → Stream"] - IMPORTER["IBundleImporter
LoadAsync(stream, Passphrase?, ct) → BundleSession
PreviewAsync(sessionId, ct) → ImportPreview
ApplyAsync(sessionId, resolutions, ct) → ImportResult"] + IMPORTER["IBundleImporter
LoadAsync(stream, Passphrase?, ct) → BundleSession
PreviewAsync(sessionId, ct) → ImportPreview
ApplyAsync(sessionId, resolutions, ct) → ImportResult
Abandon(sessionId)"] RESOLVER["DependencyResolver"] SERIALIZER["BundleSerializer
(manifest + content JSON; ZIP packer)"] ENCRYPTOR["BundleSecretEncryptor
(AES-256-GCM + PBKDF2)"] @@ -136,6 +136,8 @@ 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. +**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. + **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). ## Export Flow diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/TransportImport.razor.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/TransportImport.razor.cs index 9461e76a..d7a5a2f6 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/TransportImport.razor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/TransportImport.razor.cs @@ -51,6 +51,21 @@ namespace ZB.MOM.WW.ScadaBridge.CentralUI.Components.Pages.Design; /// retained on the component. The file is deleted on Back-to-Upload / Reset / /// successful Apply / component Dispose, so an abandoned wizard does not leak /// staged bundle plaintext beyond circuit teardown. +/// +/// +/// Server-side session release (register row 43): the staged temp file is only +/// half of an abandoned wizard's footprint — the server-side +/// opened by +/// holds the DECRYPTED bundle content and occupies one of +/// TransportOptions.MaxConcurrentImportSessions slots for the full +/// BundleSessionTtlMinutes. Only +/// used to release it, so eight abandoned wizards locked out every import on the +/// active node. Every path that drops _session — new upload, Back-to-Upload, +/// , lockout reset, circuit teardown via +/// — now goes through , which +/// calls (zero + remove, idempotent and +/// safe after a successful Apply has already released the same id). +/// /// public partial class TransportImport : ComponentBase, IDisposable { @@ -149,7 +164,9 @@ public partial class TransportImport : ComponentBase, IDisposable { _errorMessage = null; _uploadInProgress = true; - _session = null; + // Release the previous upload's server-side session before we lose the + // only reference to its id — otherwise re-selecting a file leaks a slot. + ReleaseSession(); DeleteBundleTempFile(); try { @@ -207,6 +224,10 @@ public partial class TransportImport : ComponentBase, IDisposable _errorMessage = "No bundle staged — please re-select the file."; return; } + // A successful LoadAsync overwrites _session; release whatever it is + // replacing first so the id of the outgoing session is never dropped on + // the floor (that would strand its slot for the full TTL). + ReleaseSession(); try { // Read the staged bundle straight off disk; the @@ -808,7 +829,7 @@ public partial class TransportImport : ComponentBase, IDisposable private void ResetSessionState() { - _session = null; + ReleaseSession(); DeleteBundleTempFile(); _preview = null; _resolutions = null; @@ -854,12 +875,41 @@ public partial class TransportImport : ComponentBase, IDisposable } /// - /// Ensures the staged temp file does not survive circuit - /// teardown. Blazor invokes Dispose when the user navigates away or the - /// circuit ends, so an abandoned wizard cleans up automatically. + /// Releases the server-side this wizard opened + /// (zeroing its decrypted content) and clears the local reference. Idempotent: + /// a null _session is a no-op, and + /// tolerates an id that already + /// released. Only ever passes the id THIS circuit opened, so a concurrent + /// operator's session is never touched. + /// + /// Abandon failure must not block the wizard (or, from , + /// tear down the circuit), so it is swallowed on the same best-effort footing + /// as the staged-file delete below. + /// + /// + private void ReleaseSession() + { + var session = _session; + _session = null; + if (session is null) return; + try + { + BundleImporter.Abandon(session.SessionId); + } + catch + { + // Best-effort: the session's own TTL is the backstop. + } + } + + /// + /// Ensures neither the staged temp file nor the server-side bundle session + /// survives circuit teardown. Blazor invokes Dispose when the user navigates + /// away or the circuit ends, so an abandoned wizard cleans up automatically. /// public void Dispose() { + ReleaseSession(); DeleteBundleTempFile(); } } diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Transport/IBundleImporter.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Transport/IBundleImporter.cs index 9ad6a977..3990348d 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Transport/IBundleImporter.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Transport/IBundleImporter.cs @@ -46,4 +46,25 @@ public interface IBundleImporter string user, CancellationToken ct = default, BundleNameMap? nameMap = null); + + /// + /// Releases a session opened by that will never be + /// applied — the operator navigated away, restarted the wizard, or a CLI + /// preview/import gave up before ran. Zeroes the + /// session's decrypted plaintext and drops it from the session store, which + /// frees one of the MaxConcurrentImportSessions slots immediately + /// instead of at the end of BundleSessionTtlMinutes. + /// + /// Idempotent and safe to call after (which performs + /// the same release on both its success and failure paths) or with an unknown + /// / already-expired id — the call is then a no-op and never throws. + /// + /// + /// Ownership: session ids are random GUIDs handed only to the caller that + /// opened them, so abandoning the id you hold can never release a session + /// another circuit / CLI caller owns. + /// + /// + /// Session id returned by . + void Abandon(Guid sessionId); } diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index ab92e42e..5dfbaba0 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -3571,16 +3571,27 @@ public class ManagementActor : ReceiveActor throw new ManagementCommandException("Wrong passphrase or bundle was tampered."); } - var preview = await importer.PreviewAsync(session.SessionId); - var adds = preview.Items.Count(i => i.Kind == ConflictKind.New); - var mods = preview.Items.Count(i => i.Kind == ConflictKind.Modified); - var ids = preview.Items.Count(i => i.Kind == ConflictKind.Identical); - var blocks = preview.Items.Count(i => i.Kind == ConflictKind.Blocker); - // Surface the required site/connection mappings so an operator - // (CLI or UI) sees which references need resolving before applying. - return new PreviewBundleResult( - preview.Items, adds, mods, ids, blocks, - preview.RequiredSiteMappings, preview.RequiredConnectionMappings); + // Register row 43: a preview NEVER applies, so nothing else would ever + // release this session — every `transport preview` used to burn one of + // MaxConcurrentImportSessions (and hold the decrypted bundle) for the + // full BundleSessionTtlMinutes. Release it on the way out, always. + try + { + var preview = await importer.PreviewAsync(session.SessionId); + var adds = preview.Items.Count(i => i.Kind == ConflictKind.New); + var mods = preview.Items.Count(i => i.Kind == ConflictKind.Modified); + var ids = preview.Items.Count(i => i.Kind == ConflictKind.Identical); + var blocks = preview.Items.Count(i => i.Kind == ConflictKind.Blocker); + // Surface the required site/connection mappings so an operator + // (CLI or UI) sees which references need resolving before applying. + return new PreviewBundleResult( + preview.Items, adds, mods, ids, blocks, + preview.RequiredSiteMappings, preview.RequiredConnectionMappings); + } + finally + { + importer.Abandon(session.SessionId); + } } finally { @@ -3618,50 +3629,62 @@ public class ManagementActor : ReceiveActor throw new ManagementCommandException("Wrong passphrase or bundle was tampered."); } - var preview = await importer.PreviewAsync(session.SessionId); - - var blockers = preview.Items.Where(i => i.Kind == ConflictKind.Blocker).ToList(); - if (blockers.Count > 0) + // Register row 43: ApplyAsync releases the session on both its success + // and failure paths, but every abort BEFORE it (blockers, unresolved + // mappings) used to strand the session — and its decrypted content — + // for the full TTL. Abandon is idempotent, so covering the whole block + // is safe even when ApplyAsync already released the same id. + try { - var details = string.Join("; ", - blockers.Select(b => $"{b.Name}: {b.BlockerReason}")); - throw new ManagementCommandException( - $"Bundle has {blockers.Count} blocker(s); import aborted. {details}"); - } + var preview = await importer.PreviewAsync(session.SessionId); - // Resolve every required site/connection reference into a - // BundleNameMap. Precedence: an explicit operator spec wins; otherwise - // fall back to the preview's auto-match; otherwise (no match) create-new - // ONLY if the create-missing flag is set, else fail with a clear message. - var nameMap = BuildNameMap(cmd, preview); - - // Dedupe by (EntityType, Name) -- the preview can emit multiple rows per - // entity (e.g. one row per modified member of a template), but ApplyAsync - // requires a unique resolution per key. Last-write-wins matches the - // Central UI's TransportImport.BuildDefaultResolutions behavior. For - // DataConnection rows the preview item Name is already site-qualified - // ({site}/{name}), so keying generically by (EntityType, Name) is - // automatically correct — no bare-connection-name special case needed. - var renameStamp = DateTime.UtcNow.ToString("yyyyMMdd-HHmmss"); - var resolutionsMap = new Dictionary<(string, string), ImportResolution>(); - foreach (var item in preview.Items) - { - var action = item.Kind switch + var blockers = preview.Items.Where(i => i.Kind == ConflictKind.Blocker).ToList(); + if (blockers.Count > 0) { - ConflictKind.New => ResolutionAction.Add, - ConflictKind.Identical => ResolutionAction.Skip, - ConflictKind.Modified => policy, - _ => ResolutionAction.Skip, - }; - var renameTo = (item.Kind == ConflictKind.Modified && policy == ResolutionAction.Rename) - ? $"{item.Name}-imported-{renameStamp}" - : null; - resolutionsMap[(item.EntityType, item.Name)] = new ImportResolution( - item.EntityType, item.Name, action, renameTo); - } + var details = string.Join("; ", + blockers.Select(b => $"{b.Name}: {b.BlockerReason}")); + throw new ManagementCommandException( + $"Bundle has {blockers.Count} blocker(s); import aborted. {details}"); + } - return await importer.ApplyAsync( - session.SessionId, resolutionsMap.Values.ToList(), username, nameMap: nameMap); + // Resolve every required site/connection reference into a + // BundleNameMap. Precedence: an explicit operator spec wins; otherwise + // fall back to the preview's auto-match; otherwise (no match) create-new + // ONLY if the create-missing flag is set, else fail with a clear message. + var nameMap = BuildNameMap(cmd, preview); + + // Dedupe by (EntityType, Name) -- the preview can emit multiple rows per + // entity (e.g. one row per modified member of a template), but ApplyAsync + // requires a unique resolution per key. Last-write-wins matches the + // Central UI's TransportImport.BuildDefaultResolutions behavior. For + // DataConnection rows the preview item Name is already site-qualified + // ({site}/{name}), so keying generically by (EntityType, Name) is + // automatically correct — no bare-connection-name special case needed. + var renameStamp = DateTime.UtcNow.ToString("yyyyMMdd-HHmmss"); + var resolutionsMap = new Dictionary<(string, string), ImportResolution>(); + foreach (var item in preview.Items) + { + var action = item.Kind switch + { + ConflictKind.New => ResolutionAction.Add, + ConflictKind.Identical => ResolutionAction.Skip, + ConflictKind.Modified => policy, + _ => ResolutionAction.Skip, + }; + var renameTo = (item.Kind == ConflictKind.Modified && policy == ResolutionAction.Rename) + ? $"{item.Name}-imported-{renameStamp}" + : null; + resolutionsMap[(item.EntityType, item.Name)] = new ImportResolution( + item.EntityType, item.Name, action, renameTo); + } + + return await importer.ApplyAsync( + session.SessionId, resolutionsMap.Values.ToList(), username, nameMap: nameMap); + } + finally + { + importer.Abandon(session.SessionId); + } } finally { diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs index ef5a9932..ffbc646c 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs @@ -1715,6 +1715,32 @@ public sealed class BundleImporter : IBundleImporter } } + /// + /// + /// Register row 43: every abandonment path (Central UI wizard reset / + /// back-to-upload / circuit teardown, CLI preview, CLI import that aborts + /// before Apply) funnels here so a session that will never be applied stops + /// holding one of the MaxConcurrentImportSessions slots — and stops + /// holding its decrypted plaintext — for the rest of its TTL. Applies the same + /// zero-then-remove ordering as the T-007 release inside + /// : zeroing first means a caller still holding the + /// session reference (the Razor page keeps one for the Result step) observes + /// the cleared buffer too. + /// + public void Abandon(Guid sessionId) + { + // Get() returns null for an unknown or already-expired id, which makes the + // double-release after ApplyAsync (which already removed it) a no-op rather + // than a throw. Remove is called unconditionally so an expired-but-still- + // resident entry is dropped as well. + var session = _sessionStore.Get(sessionId); + if (session is not null) + { + ZeroDecryptedContent(session); + } + _sessionStore.Remove(sessionId); + } + /// /// Enumerates the deployed instances stale-ed by this import. /// Overwriting a template changes the flattened-config hash of every instance diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/Design/TransportImportPageTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/Design/TransportImportPageTests.cs index c1b13770..0848a963 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/Design/TransportImportPageTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/Design/TransportImportPageTests.cs @@ -17,6 +17,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Types.Transport; using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase; using ZB.MOM.WW.ScadaBridge.Security; using ZB.MOM.WW.ScadaBridge.Transport; +using ZB.MOM.WW.ScadaBridge.Transport.Import; using TransportImportPage = ZB.MOM.WW.ScadaBridge.CentralUI.Components.Pages.Design.TransportImport; namespace ZB.MOM.WW.ScadaBridge.CentralUI.Tests.Pages.Design; @@ -688,6 +689,191 @@ public class TransportImportPageTests : BunitContext Assert.Empty(connChoices); } + // ───────────────────────────────────────────────────────────────────── + // Register row 43: an abandoned wizard must not strand its server-side + // BundleSession. These tests swap the NSubstitute importer for one backed + // by a REAL BundleSessionStore so the assertion is "the slot is actually + // released", not "a mock was called". + // ───────────────────────────────────────────────────────────────────── + + /// + /// Minimal over a real . + /// and mirror the production + /// importer's zero-then-remove release; every other member is unused by these + /// tests. Sessions are seeded directly with because + /// driving InputFile from bUnit is impractical (see the class remarks). + /// + private sealed class StoreBackedImporter : IBundleImporter + { + public BundleSessionStore Store { get; } = new( + Microsoft.Extensions.Options.Options.Create(new TransportOptions()), + TimeProvider.System); + + public int AbandonCalls { get; private set; } + + public BundleSession OpenSession(string sourceEnv = "prod-cluster") + { + var seed = BuildEncryptedSession(sourceEnv); + var session = new BundleSession + { + SessionId = seed.SessionId, + Manifest = seed.Manifest, + // Non-zero bytes so the zeroing assertion is meaningful. + DecryptedContent = new byte[] { 0x11, 0x22, 0x33, 0x44 }, + ExpiresAt = DateTimeOffset.UtcNow.AddMinutes(30), + }; + return Store.Open(session); + } + + public Task LoadAsync(Stream bundleStream, string? passphrase, CancellationToken ct = default) + => throw new NotSupportedException("These tests seed sessions via OpenSession()."); + + public Task PreviewAsync(Guid sessionId, CancellationToken ct = default) + => Task.FromResult(new ImportPreview(sessionId, new List())); + + public Task ApplyAsync( + Guid sessionId, + IReadOnlyList resolutions, + string user, + CancellationToken ct = default, + BundleNameMap? nameMap = null) + { + Release(sessionId); + return Task.FromResult(new ImportResult( + BundleImportId: Guid.NewGuid(), + Added: 1, + Overwritten: 0, + Skipped: 0, + Renamed: 0, + StaleInstanceIds: Array.Empty(), + AuditEventCorrelation: Guid.NewGuid().ToString())); + } + + public void Abandon(Guid sessionId) + { + AbandonCalls++; + Release(sessionId); + } + + private void Release(Guid sessionId) + { + var session = Store.Get(sessionId); + if (session is { DecryptedContent.Length: > 0 }) + { + Array.Clear(session.DecryptedContent, 0, session.DecryptedContent.Length); + } + Store.Remove(sessionId); + } + } + + [Fact] + public async Task Disposing_the_wizard_releases_its_bundle_session() + { + // Circuit teardown / navigation away. Before the fix, Dispose deleted only + // the staged temp file, so the session (and its decrypted content) held one + // of MaxConcurrentImportSessions for the full 30-minute TTL. + var importer = new StoreBackedImporter(); + Services.AddSingleton(importer); + + var cut = Render(); + var session = importer.OpenSession(); + var content = session.DecryptedContent; + await cut.InvokeAsync(() => SetField(cut.Instance, "_session", session)); + + cut.Instance.Dispose(); + + Assert.Null(importer.Store.Get(session.SessionId)); + Assert.All(content, b => Assert.Equal(0, b)); + Assert.Null(GetField(cut.Instance, "_session")); + } + + [Fact] + public async Task BackToUpload_releases_the_bundle_session() + { + // The wizard's only cancel affordance ("Back" on steps 2/3/5) routes through + // BackToUpload → ResetSessionState, which used to null the local field only. + var importer = new StoreBackedImporter(); + Services.AddSingleton(importer); + + var cut = Render(); + var session = importer.OpenSession(); + var content = session.DecryptedContent; + await cut.InvokeAsync(() => + { + SetField(cut.Instance, "_session", session); + SetField(cut.Instance, "_step", TransportImportPage.ImportWizardStep.Diff); + }); + + await cut.InvokeAsync(() => + { + var method = cut.Instance.GetType().GetMethod( + "BackToUpload", + System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic)!; + method.Invoke(cut.Instance, Array.Empty()); + }); + + Assert.Null(importer.Store.Get(session.SessionId)); + Assert.All(content, b => Assert.Equal(0, b)); + } + + [Fact] + public async Task Apply_then_dispose_releases_the_session_once_without_throwing() + { + // ApplyAsync already removes the session; the wizard's own release must be + // idempotent so circuit teardown after a completed import is a silent no-op. + var importer = new StoreBackedImporter(); + Services.AddSingleton(importer); + + var cut = Render(); + var session = importer.OpenSession(sourceEnv: "prod-cluster"); + await cut.InvokeAsync(() => + { + SetField(cut.Instance, "_session", session); + SetField(cut.Instance, "_resolutions", + new Dictionary<(string EntityType, string Name), ImportResolution> + { + [("Template", "Pump")] = new("Template", "Pump", ResolutionAction.Add, null), + }); + SetField(cut.Instance, "_step", TransportImportPage.ImportWizardStep.Confirm); + SetField(cut.Instance, "_confirmEnvironmentText", "prod-cluster"); + }); + + await cut.InvokeAsync(() => InvokeAsyncMethod(cut.Instance, "ApplyAsync")); + Assert.Equal( + TransportImportPage.ImportWizardStep.Result, + GetField(cut.Instance, "_step")); + Assert.Null(importer.Store.Get(session.SessionId)); + + // Must not throw, and must not resurrect or double-release anything. + cut.Instance.Dispose(); + cut.Instance.Dispose(); + + Assert.Null(importer.Store.Get(session.SessionId)); + } + + [Fact] + public async Task Disposing_one_wizard_leaves_a_concurrent_circuits_session_open() + { + // Sessions are keyed by a GUID held only by the circuit that opened them, + // so one operator abandoning their import must not evict another's. + var importer = new StoreBackedImporter(); + Services.AddSingleton(importer); + + var first = Render(); + var second = Render(); + var mine = importer.OpenSession(); + var theirs = importer.OpenSession(sourceEnv: "other-cluster"); + await first.InvokeAsync(() => SetField(first.Instance, "_session", mine)); + await second.InvokeAsync(() => SetField(second.Instance, "_session", theirs)); + + first.Instance.Dispose(); + + Assert.Null(importer.Store.Get(mine.SessionId)); + var survivor = importer.Store.Get(theirs.SessionId); + Assert.NotNull(survivor); + Assert.Contains(survivor!.DecryptedContent, b => b != 0); + } + // ───────────────────────────────────────────────────────────────────── // Reflection helpers — the wizard's per-instance state is private (the // razor partial pattern). We poke at it via reflection rather than diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleImporterLoadTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleImporterLoadTests.cs index 0059fb79..2148f7f0 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleImporterLoadTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleImporterLoadTests.cs @@ -502,6 +502,92 @@ public sealed class BundleImporterLoadTests () => rig.Importer.LoadAsync(stream, passphrase: null)); } + // ───────────────────────────────────────────────────────────────────── + // Register row 43 — abandoned-session release. + // ───────────────────────────────────────────────────────────────────── + + [Fact] + public async Task Abandon_releases_the_session_slot_and_zeroes_its_decrypted_content() + { + // Row 43: only ApplyAsync used to release a session, so any caller that + // loaded a bundle and walked away held one of MaxConcurrentImportSessions + // (plus the decrypted plaintext) for the full TTL. + var rig = BuildRig(); + using var packed = PackEncryptedBundle( + rig.Serializer, rig.ManifestBuilder, rig.Encryptor, SmallContent(), "correct"); + var session = await rig.Importer.LoadAsync( + new MemoryStream(((MemoryStream)packed).ToArray()), passphrase: "correct"); + + var decrypted = session.DecryptedContent; + Assert.NotEmpty(decrypted); + Assert.Contains(decrypted, b => b != 0); + Assert.NotNull(rig.SessionStore.Get(session.SessionId)); + + rig.Importer.Abandon(session.SessionId); + + Assert.Null(rig.SessionStore.Get(session.SessionId)); + // Zeroed in place, so even a caller still holding the reference (the Razor + // page keeps one for the Result step) can no longer read the plaintext. + Assert.All(decrypted, b => Assert.Equal(0, b)); + } + + [Fact] + public async Task Abandon_is_idempotent_and_tolerates_unknown_ids() + { + // The Razor page calls Abandon from Dispose even after a successful Apply + // already released the same id; a double release must be a silent no-op. + var rig = BuildRig(); + using var packed = PackPlainBundle(rig.Serializer, rig.ManifestBuilder, SmallContent()); + var session = await rig.Importer.LoadAsync(packed, passphrase: null); + + rig.Importer.Abandon(session.SessionId); + rig.Importer.Abandon(session.SessionId); + rig.Importer.Abandon(Guid.NewGuid()); + + Assert.Null(rig.SessionStore.Get(session.SessionId)); + } + + [Fact] + public async Task Abandon_does_not_release_another_callers_session() + { + // Sessions are keyed by a random GUID handed only to their opener, so one + // operator abandoning their wizard must not evict a concurrent import. + var rig = BuildRig(); + using var packedA = PackPlainBundle(rig.Serializer, rig.ManifestBuilder, SmallContent()); + using var packedB = PackPlainBundle(rig.Serializer, rig.ManifestBuilder, EmptyContent()); + var a = await rig.Importer.LoadAsync(packedA, passphrase: null); + var b = await rig.Importer.LoadAsync(packedB, passphrase: null); + + rig.Importer.Abandon(a.SessionId); + + Assert.Null(rig.SessionStore.Get(a.SessionId)); + var survivor = rig.SessionStore.Get(b.SessionId); + Assert.NotNull(survivor); + Assert.NotEmpty(survivor!.DecryptedContent); + } + + [Fact] + public async Task Abandon_frees_a_concurrent_import_session_slot() + { + // The availability half of row 43: abandoned sessions used to exhaust the + // cap and lock out every import on the node until the TTL expired. + var rig = BuildRig(opts => opts.MaxConcurrentImportSessions = 2); + using var packed = PackPlainBundle(rig.Serializer, rig.ManifestBuilder, SmallContent()); + var bundleBytes = ((MemoryStream)packed).ToArray(); + + var first = await rig.Importer.LoadAsync(new MemoryStream(bundleBytes), passphrase: null); + await rig.Importer.LoadAsync(new MemoryStream(bundleBytes), passphrase: null); + + var full = await Assert.ThrowsAsync( + () => rig.Importer.LoadAsync(new MemoryStream(bundleBytes), passphrase: null)); + Assert.Contains("Too many concurrent import sessions", full.Message); + + rig.Importer.Abandon(first.SessionId); + + var third = await rig.Importer.LoadAsync(new MemoryStream(bundleBytes), passphrase: null); + Assert.NotNull(rig.SessionStore.Get(third.SessionId)); + } + /// /// Builds a zip directly so the test can write a manifest whose /// ContentHash or BundleFormatVersion intentionally From 5ab26a42338a44c127070a76273b690f470637e0 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 06:01:14 -0400 Subject: [PATCH 2/2] fix(transport): enforce the documented per-IP bundle-unlock throttle (row 44) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BundleUnlockRateLimiter was registered (ServiceCollectionExtensions), injected into BundleImporter, unit-tested, documented on TransportOptions.MaxUnlockAttemptsPerIpPerHour = 10 and pinned in appsettings.Central.json — but _unlockRateLimiter was never invoked. A brute-force passphrase attacker against a stolen bundle was bounded only by the per-bundle 3-strike lockout, which a re-export side-steps (new ContentHash). LoadAsync now registers every unlock attempt against the limiter. Ordering is load-bearing: AFTER the per-bundle lockout check (hammering a locked bundle spends no hourly budget) and BEFORE the decrypt (so a denial burns no per-bundle strike — otherwise the throttle would be a way to lock a bundle someone is legitimately importing). Over budget throws the new BundleUnlockThrottledException, fail-closed with a clear message on both surfaces; it lives in Commons because the management surface (Commons-only references) must catch it to return a curated error rather than MapFault's generic internal-error text. Client key plumbing: - Central UI: new scoped CircuitClientAddressProvider snapshots Connection.RemoteIpAddress at circuit establishment — the same constraint and the same pattern as CookieAuthenticationStateProvider, since HttpContext is null by the time a circuit submits a passphrase. - CLI/management: additive ManagementEnvelope.ClientIp, stamped by ManagementEndpoints and threaded through DispatchCommand to the preview/import bundle handlers. - No attributable address => a single SHARED "" bucket, not a bypass: unknown clients get a stricter budget, never none. Per-client isolation is only real behind a ForwardedHeaders-trusted proxy, the same caveat the login throttle carries (Component-Security.md). Docs: Component-Transport.md + docs/components/Transport.md record the two-level ordering, the client-key sourcing, the shared-bucket fallback and a troubleshooting entry for the new error. Tests: 6 importer tests (cap enforced; denial != passphrase failure, no strike burned; per-client isolation; shared unattributed bucket; unencrypted bundles consume no budget; a locked bundle spends no hourly budget) — 4 of the 6 fail with the limiter call disabled, i.e. against the dead-code state. Plus ManagementActor tests pinning the ClientIp forwarding and the curated throttle error. --- docs/components/Transport.md | 14 +- docs/requirements/Component-Transport.md | 4 +- .../Auth/CircuitClientAddressProvider.cs | 53 ++++++ .../Pages/Design/TransportImport.razor.cs | 34 +++- .../ServiceCollectionExtensions.cs | 7 + .../Interfaces/Transport/IBundleImporter.cs | 14 +- .../Messages/Management/ManagementEnvelope.cs | 17 +- .../BundleUnlockThrottledException.cs | 43 +++++ .../ManagementActor.cs | 31 ++- .../ManagementEndpoints.cs | 9 +- .../Import/BundleImporter.cs | 43 ++++- .../Pages/Design/TransportImportPageTests.cs | 16 +- .../Pages/QueryStringDrillInTests.cs | 4 + .../ManagementActorTests.cs | 101 +++++++++- .../TransportGateTests.cs | 2 +- .../Import/BundleImporterLoadTests.cs | 177 ++++++++++++++++++ 16 files changed, 545 insertions(+), 24 deletions(-) create mode 100644 src/ZB.MOM.WW.ScadaBridge.CentralUI/Auth/CircuitClientAddressProvider.cs create mode 100644 src/ZB.MOM.WW.ScadaBridge.Commons/Types/Transport/BundleUnlockThrottledException.cs diff --git a/docs/components/Transport.md b/docs/components/Transport.md index cddd079c..c005d609 100644 --- a/docs/components/Transport.md +++ b/docs/components/Transport.md @@ -118,7 +118,15 @@ Inbound API keys are explicitly excluded — per re-architecture C4, keys are en **Phase 1 — `LoadAsync`**: copies the upload stream to a seekable `MemoryStream`, enforces the bundle size cap (`MaxBundleSizeMb`), validates the ZIP envelope (entry count, per-entry decompressed size, compression ratio — all before decompression), reads and validates `manifest.json` (format version, SHA-256 content hash), and decrypts `content.enc` when `Encryption` is present. The decrypted bytes are stored in a new `BundleSession`. -Passphrase lockout operates at two levels: per-bundle (3-strike counter in `BundleSessionStore`, keyed by `ContentHash` so a second browser tab sharing the same bundle bytes cannot reset the counter) and per-IP-per-hour (`BundleUnlockRateLimiter`, default 10 attempts). A successful decrypt clears the per-bundle counter. +Passphrase lockout operates at two levels: per-bundle (3-strike counter in `BundleSessionStore`, keyed by `ContentHash` so a second browser tab sharing the same bundle bytes cannot reset the counter) and per-IP-per-hour (`BundleUnlockRateLimiter`, `MaxUnlockAttemptsPerIpPerHour`, default 10 attempts). A successful decrypt clears the per-bundle counter. + +The two levels interact in a specific order, and it is load-bearing (register row 44 — the limiter had been registered, injected and unit-tested but never called, so only the per-bundle lockout was real): + +1. **Per-bundle lockout first.** An already-locked bundle throws `BundleLockedException` without registering a rate-limiter attempt, so hammering a locked bundle cannot spend the client's hourly budget. +2. **Per-IP throttle next, before the decrypt.** Over budget throws `BundleUnlockThrottledException` (Commons, so the management surface can catch it and return a curated message). Because it precedes the decrypt, a denial burns **no** per-bundle strike — otherwise the throttle would be a way to lock out a bundle someone is legitimately importing. Every unlock attempt is registered, successful ones included: the documented budget counts attempts, not failures. +3. Only then the AES-GCM decrypt, whose failure increments the per-bundle counter as before. + +The client key is the caller's remote IP: the Central UI wizard reads it from `CircuitClientAddressProvider` (a scoped service that snapshots `Connection.RemoteIpAddress` at circuit establishment, since `HttpContext` is gone by the time a circuit submits a passphrase — the same pattern as `CookieAuthenticationStateProvider`), and the management endpoint stamps it on `ManagementEnvelope.ClientIp` for the CLI `transport preview` / `transport import` paths. A caller with no attributable address is counted against a single **shared** bucket rather than skipping the throttle — "unknown client" degrades to a stricter budget, never to none. As with the login throttle (`Component-Security.md`), per-client isolation is only real when ForwardedHeaders is configured for the fronting proxy; behind an untrusted proxy every caller collapses into the proxy's bucket. ```csharp // From BundleImporter.LoadAsync — decrypt path (simplified) @@ -249,6 +257,10 @@ A template's scripts reference a `SharedScript` or `ExternalSystem` that exists After 3 wrong passphrase attempts against the same bundle (keyed by `ContentHash`), `BundleImporter.LoadAsync` throws `BundleLockedException`. The session is unusable. Re-upload the bundle file to get a new session with a fresh counter. A `BundleImportUnlockFailed` audit row is written on each failed attempt. +### Unlock throttled ("Too many bundle unlock attempts") + +`BundleUnlockThrottledException` — this **client** (not this bundle) has used its `MaxUnlockAttemptsPerIpPerHour` budget within the trailing hour. Re-uploading does not help; the budget is keyed by IP and rolls forward on a sliding window, and the counters are process-local (a central node restart clears them, deliberately, so an outage doesn't strand legitimate operators). The denial burns no per-bundle strike, so the bundle itself is still importable once the window frees up. Raise `ScadaBridge:Transport:MaxUnlockAttemptsPerIpPerHour` if a legitimate workflow (many encrypted bundles in one sitting) trips it routinely. + ### BundleImportAlarmScriptUnresolved / BundleImportCompositionUnresolved warnings These audit rows appear when the second-pass rewire (`ResolveAlarmScriptLinksAsync` / `ResolveCompositionEdgesAsync`) cannot match a name to a persisted row. The import commits — the FK is left null / the composition row is skipped — but the warning signals an incomplete import. Re-examine the bundle's dependency graph and re-export with the missing artifacts included. diff --git a/docs/requirements/Component-Transport.md b/docs/requirements/Component-Transport.md index 2b150c16..a1b7287e 100644 --- a/docs/requirements/Component-Transport.md +++ b/docs/requirements/Component-Transport.md @@ -112,7 +112,7 @@ The manifest is plaintext so the import wizard can preview bundle contents and s flowchart TD subgraph T["ZB.MOM.WW.ScadaBridge.Transport"] EXPORTER["IBundleExporter
ExportAsync(ExportSelection, Passphrase?, ct) → Stream"] - IMPORTER["IBundleImporter
LoadAsync(stream, Passphrase?, ct) → BundleSession
PreviewAsync(sessionId, ct) → ImportPreview
ApplyAsync(sessionId, resolutions, ct) → ImportResult
Abandon(sessionId)"] + IMPORTER["IBundleImporter
LoadAsync(stream, Passphrase?, ct, ClientIp?) → BundleSession
PreviewAsync(sessionId, ct) → ImportPreview
ApplyAsync(sessionId, resolutions, ct) → ImportResult
Abandon(sessionId)"] RESOLVER["DependencyResolver"] SERIALIZER["BundleSerializer
(manifest + content JSON; ZIP packer)"] ENCRYPTOR["BundleSecretEncryptor
(AES-256-GCM + PBKDF2)"] @@ -336,7 +336,7 @@ Imports are all-or-nothing per bundle. A bundle either applies fully or not at a - **AES-256-GCM** for content encryption with **PBKDF2-SHA256 / 600 000 iterations** (OWASP 2023+ guidance), per-bundle random salt, random IV per encryption. The GCM auth tag is verified before decryption — a wrong passphrase fails cleanly. - **Passphrase never persisted.** It lives only inside the export/import service call path and is discarded after use. No environment variable, no log line. -- **Failed-unlock rate limit:** per-session 3-strike lockout; per-IP-per-hour cap (default 10, configurable) to deter brute force against a stolen bundle. Each failed attempt produces a `BundleImportUnlockFailed` audit row. +- **Failed-unlock rate limit:** per-bundle 3-strike lockout (keyed by `ContentHash`, server-side); per-IP-per-hour cap (`MaxUnlockAttemptsPerIpPerHour`, default 10, configurable) to deter brute force against a stolen bundle. Each failed attempt produces a `BundleImportUnlockFailed` audit row. The two are evaluated in order — bundle lockout, then per-IP throttle, then decrypt — so hammering an already-locked bundle spends no hourly budget, and a throttle denial (`BundleUnlockThrottledException`, fail-closed with a curated message on both the UI and CLI surfaces) burns **no** per-bundle strike, since it never reaches the decryptor. Successful unlocks count against the hourly budget too: it caps attempts, not failures. The client key is the caller's remote IP — from `CircuitClientAddressProvider` (snapshotted at Blazor circuit establishment, since `HttpContext` is unavailable mid-circuit) for the Central UI, and from `ManagementEnvelope.ClientIp` for the CLI; a caller with no attributable address shares a single stricter bucket rather than bypassing the throttle. Per-client isolation is only real behind a ForwardedHeaders-trusted proxy, the same caveat the login throttle carries in `Component-Security.md`. (Register row 44: the limiter existed, was configured and unit-tested, but was never invoked until this was wired.) - **Bundle size cap** on upload (default 100 MB, configurable) to bound memory. - **In-transit:** existing HTTPS to the Central UI; no new channel. - **Audit trail is the chain of custody.** Every export, every import (including aborted ones at validation), and every unlock failure is audit-logged with source env, content hash, encrypted yes/no, and artifact summary. diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Auth/CircuitClientAddressProvider.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Auth/CircuitClientAddressProvider.cs new file mode 100644 index 00000000..a591fb49 --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Auth/CircuitClientAddressProvider.cs @@ -0,0 +1,53 @@ +using Microsoft.AspNetCore.Http; + +namespace ZB.MOM.WW.ScadaBridge.CentralUI.Auth; + +/// +/// Supplies the remote IP of the client that established this Blazor circuit. +/// +/// Same constraint — and the same solution — as +/// : +/// is valid only during the initial HTTP request that establishes the circuit and +/// is null (or stale/foreign) for the life of the SignalR connection afterwards. +/// Registered Scoped, this type is constructed inside that request's DI +/// scope, so it snapshots Connection.RemoteIpAddress once in the +/// constructor and serves that snapshot for the circuit's lifetime. +/// +/// +/// The value is only as trustworthy as the proxy configuration: it is whatever +/// Kestrel reports as the connection's remote address, which ForwardedHeaders +/// (see ForwardedHeadersSetup, enabled for the shipped Traefik topologies) +/// rewrites from X-Forwarded-For for CONFIGURED proxies only. Behind an +/// untrusted proxy every circuit collapses onto the proxy's address — consumers +/// must treat that as a shared bucket, not as per-client isolation. +/// +/// +public sealed class CircuitClientAddressProvider +{ + /// + /// Snapshots the client address from the current HTTP context for use throughout + /// the circuit lifetime. + /// + /// Accessor used to read the circuit-establishing request. + public CircuitClientAddressProvider(IHttpContextAccessor httpContextAccessor) + { + ArgumentNullException.ThrowIfNull(httpContextAccessor); + ClientAddress = httpContextAccessor.HttpContext?.Connection.RemoteIpAddress?.ToString(); + } + + /// + /// Test seam: constructs the provider with an explicit address instead of reading + /// an . + /// + /// The address to serve, or null for "unknown". + public CircuitClientAddressProvider(string? clientAddress) + { + ClientAddress = clientAddress; + } + + /// + /// The remote IP of the circuit-establishing request, or null when it could not be + /// determined (no HTTP context, or a transport that reports no remote address). + /// + public string? ClientAddress { get; } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/TransportImport.razor.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/TransportImport.razor.cs index d7a5a2f6..dd4b93dd 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/TransportImport.razor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/TransportImport.razor.cs @@ -86,6 +86,11 @@ public partial class TransportImport : ComponentBase, IDisposable [Inject] private IAuditService AuditService { get; set; } = default!; [Inject] private ScadaBridgeDbContext DbContext { get; set; } = default!; + // Register row 44: the per-IP-per-hour unlock throttle needs the caller's + // address, and HttpContext is long gone by the time a circuit submits a + // passphrase — this provider snapshots it when the circuit was established. + [Inject] private CircuitClientAddressProvider ClientAddress { get; set; } = default!; + // The Map step needs the destination environment's sites + each // site's connections to populate the "map to existing target" dropdowns. [Inject] private ISiteRepository SiteRepo { get; set; } = default!; @@ -236,7 +241,8 @@ public partial class TransportImport : ComponentBase, IDisposable using var stream = new FileStream( _bundleTempPath, FileMode.Open, FileAccess.Read, FileShare.Read, bufferSize: 81920, useAsync: true); - _session = await BundleImporter.LoadAsync(stream, passphrase, CancellationToken.None); + _session = await BundleImporter.LoadAsync( + stream, passphrase, CancellationToken.None, ClientAddress.ClientAddress); _errorMessage = null; } catch (ArgumentException) @@ -253,6 +259,14 @@ public partial class TransportImport : ComponentBase, IDisposable _session = null; throw; } + catch (BundleUnlockThrottledException) + { + // Register row 44: per-IP-per-hour budget spent. Bubble so the caller + // can surface it WITHOUT counting a passphrase failure — the attempt + // never reached the decryptor. + _session = null; + throw; + } catch (InvalidDataException ex) { _session = null; @@ -330,6 +344,11 @@ public partial class TransportImport : ComponentBase, IDisposable /// shared across tabs / CLI / circuits). The Razor counter is kept ONLY for /// display ("3 of N attempts used") — it is no longer the source of truth. /// + /// + /// Register row 44: a is a third, + /// distinct outcome — this CLIENT has spent its per-IP-per-hour unlock budget. + /// It burns no per-bundle strike and leaves the wizard on Step 2. + /// /// private async Task SubmitPassphraseAsync() { @@ -360,6 +379,19 @@ public partial class TransportImport : ComponentBase, IDisposable ResetSessionState(); _step = ImportWizardStep.Upload; } + catch (BundleUnlockThrottledException ex) + { + // Register row 44: fail closed on the per-IP-per-hour cap. Deliberately + // does NOT touch _failedUnlockAttempts — a throttled attempt never + // reached the decryptor, so it must not consume one of the bundle's + // three strikes (otherwise the throttle could be used to lock a bundle + // an operator is legitimately importing). The wizard stays on Step 2 so + // the operator can retry once the hour window rolls forward. + _passphrase = string.Empty; + var throttledEntity = _session?.Manifest.ContentHash ?? ""; + await EmitUnlockFailedAuditRowAsync(throttledEntity, _failedUnlockAttempts, ex.Message); + _errorMessage = ex.Message; + } catch (CryptographicException ex) { _failedUnlockAttempts++; diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/ServiceCollectionExtensions.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/ServiceCollectionExtensions.cs index 2509313a..4cbada93 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/ServiceCollectionExtensions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/ServiceCollectionExtensions.cs @@ -27,6 +27,13 @@ public static class ServiceCollectionExtensions services.AddScoped(); services.AddCascadingAuthenticationState(); + // Snapshots the circuit-establishing request's remote IP (HttpContext is only + // valid then), so circuit-lifetime code can still attribute a client address — + // today the Transport import wizard's per-IP unlock throttle. Explicit factory + // because the type carries a second (test-seam) string ctor. + services.AddScoped(sp => new CircuitClientAddressProvider( + sp.GetRequiredService())); + // Resolves the current user's permitted site set from their SiteId claims // so Deployment/Monitoring pages can enforce site scoping. services.AddScoped(); diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Transport/IBundleImporter.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Transport/IBundleImporter.cs index 3990348d..4e6d6176 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Transport/IBundleImporter.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Transport/IBundleImporter.cs @@ -10,8 +10,20 @@ public interface IBundleImporter /// Stream containing the bundle zip archive. /// Optional passphrase for decrypting an encrypted bundle. /// Cancellation token. + /// + /// Identity of the calling client — the remote IP — used to enforce the + /// per-IP-per-hour unlock throttle (TransportOptions.MaxUnlockAttemptsPerIpPerHour) + /// on encrypted bundles. Trailing optional so unencrypted / in-process callers + /// need not supply one; null or blank counts the attempt against a single shared + /// "unattributed caller" bucket rather than skipping the throttle, so a caller + /// that cannot be attributed can never spend an unbounded budget. + /// /// A task that resolves to session metadata for the loaded bundle. - Task LoadAsync(Stream bundleStream, string? passphrase, CancellationToken ct = default); + Task LoadAsync( + Stream bundleStream, + string? passphrase, + CancellationToken ct = default, + string? clientKey = null); /// /// Diffs the loaded bundle against the target database and returns a per-artifact preview. diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/ManagementEnvelope.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/ManagementEnvelope.cs index 9c5915f7..e674518e 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/ManagementEnvelope.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/ManagementEnvelope.cs @@ -4,7 +4,22 @@ public record AuthenticatedUser( string Username, string DisplayName, string[] Roles, string[] PermittedSiteIds); -public record ManagementEnvelope(AuthenticatedUser User, object Command, string CorrelationId); +/// The authenticated caller. +/// The management command to run. +/// Per-request correlation id echoed on the reply. +/// +/// Remote IP of the HTTP caller, when the envelope came in over the management +/// endpoint. Additive trailing-optional (message contracts evolve additively), so +/// in-cluster senders that construct an envelope directly stay source-compatible and +/// simply carry null. Consumed by the Transport bundle handlers to enforce the +/// per-IP-per-hour unlock throttle; null degrades to a shared throttle bucket rather +/// than to no throttling (register row 44). +/// +public record ManagementEnvelope( + AuthenticatedUser User, + object Command, + string CorrelationId, + string? ClientIp = null); public record ManagementSuccess(string CorrelationId, string JsonData); public record ManagementError(string CorrelationId, string Error, string ErrorCode); diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Transport/BundleUnlockThrottledException.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Transport/BundleUnlockThrottledException.cs new file mode 100644 index 00000000..6a63e52d --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Transport/BundleUnlockThrottledException.cs @@ -0,0 +1,43 @@ +namespace ZB.MOM.WW.ScadaBridge.Commons.Types.Transport; + +/// +/// T-004 / register row 44: thrown by IBundleImporter.LoadAsync when a client has +/// spent its trailing-hour bundle-unlock budget (TransportOptions.MaxUnlockAttemptsPerIpPerHour, +/// default 10) as measured by BundleUnlockRateLimiter. +/// +/// Distinct from BundleLockedException on purpose: that one means "this BUNDLE has +/// burned its 3 strikes" and is keyed by the bundle's content hash; this one means "this +/// CLIENT has attempted too many unlocks, across any bundles" and is keyed by the caller's +/// IP. A throttled attempt never reaches the decryptor, so it deliberately burns no +/// per-bundle strike — otherwise the throttle could be used to lock out a bundle an +/// operator is legitimately importing. +/// +/// +/// Lives in Commons rather than beside the importer because it is part of the +/// IBundleImporter.LoadAsync contract, which is declared here: the management +/// surface (which references Commons but not Transport) must be able to catch it to +/// return a clear, curated error instead of a generic internal-failure message. +/// +/// +public sealed class BundleUnlockThrottledException : Exception +{ + /// The rate-limiter key the attempt was counted against — the client IP, or the shared unattributed-caller bucket. + public string ClientKey { get; } + + /// The configured trailing-window cap that was reached. + public int MaxAttemptsPerWindow { get; } + + /// + /// Initializes a new . + /// + /// The rate-limiter key (client IP, or the shared unattributed bucket). + /// The configured per-window cap that was reached. + public BundleUnlockThrottledException(string clientKey, int maxAttemptsPerWindow) + : base( + $"Too many bundle unlock attempts ({maxAttemptsPerWindow} per hour) from this client. " + + "Wait for the hour window to roll forward before trying again.") + { + ClientKey = clientKey ?? throw new ArgumentNullException(nameof(clientKey)); + MaxAttemptsPerWindow = maxAttemptsPerWindow; + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index 5dfbaba0..5a6c8f66 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -130,7 +130,7 @@ public class ManagementActor : ReceiveActor private async Task ProcessCommand(ManagementEnvelope envelope, AuthenticatedUser user) { using var scope = _serviceProvider.CreateScope(); - var result = await DispatchCommand(scope.ServiceProvider, envelope.Command, user); + var result = await DispatchCommand(scope.ServiceProvider, envelope.Command, user, envelope.ClientIp); return new ManagementSuccess(envelope.CorrelationId, SerializeResult(result)); } @@ -294,7 +294,8 @@ public class ManagementActor : ReceiveActor _ => null }; - private async Task DispatchCommand(IServiceProvider sp, object command, AuthenticatedUser user) + private async Task DispatchCommand( + IServiceProvider sp, object command, AuthenticatedUser user, string? clientIp = null) { return command switch { @@ -474,8 +475,8 @@ public class ManagementActor : ReceiveActor // Transport bundle operations ExportBundleCommand cmd => await HandleExportBundle(sp, cmd, user.Username), - PreviewBundleCommand cmd => await HandlePreviewBundle(sp, cmd), - ImportBundleCommand cmd => await HandleImportBundle(sp, cmd, user.Username), + PreviewBundleCommand cmd => await HandlePreviewBundle(sp, cmd, clientIp), + ImportBundleCommand cmd => await HandleImportBundle(sp, cmd, user.Username, clientIp), // NOTE: ResolveRolesCommand is intentionally NOT dispatched. The two-step // "ResolveRoles + command" flow is retired — the HTTP endpoint performs LDAP @@ -3549,7 +3550,7 @@ public class ManagementActor : ReceiveActor /// not apply anything; callers can use this to gate Apply on the diff shape. /// private static async Task HandlePreviewBundle( - IServiceProvider sp, PreviewBundleCommand cmd) + IServiceProvider sp, PreviewBundleCommand cmd, string? clientIp) { await TransportGate.WaitAsync(); try @@ -3560,7 +3561,7 @@ public class ManagementActor : ReceiveActor try { await using var stream = new MemoryStream(bytes); - session = await importer.LoadAsync(stream, cmd.Passphrase); + session = await importer.LoadAsync(stream, cmd.Passphrase, clientKey: clientIp); } catch (ArgumentException ex) { @@ -3570,6 +3571,13 @@ public class ManagementActor : ReceiveActor { throw new ManagementCommandException("Wrong passphrase or bundle was tampered."); } + catch (BundleUnlockThrottledException ex) + { + // Register row 44: per-IP-per-hour unlock cap. Curated so the CLI + // prints the reason instead of MapFault's generic internal-error + // message; no per-bundle strike was burned. + throw new ManagementCommandException(ex.Message); + } // Register row 43: a preview NEVER applies, so nothing else would ever // release this session — every `transport preview` used to burn one of @@ -3605,7 +3613,7 @@ public class ManagementActor : ReceiveActor /// rows fail the call before Apply. /// private static async Task HandleImportBundle( - IServiceProvider sp, ImportBundleCommand cmd, string username) + IServiceProvider sp, ImportBundleCommand cmd, string username, string? clientIp) { await TransportGate.WaitAsync(); try @@ -3618,7 +3626,7 @@ public class ManagementActor : ReceiveActor try { await using var stream = new MemoryStream(bytes); - session = await importer.LoadAsync(stream, cmd.Passphrase); + session = await importer.LoadAsync(stream, cmd.Passphrase, clientKey: clientIp); } catch (ArgumentException ex) { @@ -3628,6 +3636,13 @@ public class ManagementActor : ReceiveActor { throw new ManagementCommandException("Wrong passphrase or bundle was tampered."); } + catch (BundleUnlockThrottledException ex) + { + // Register row 44: per-IP-per-hour unlock cap. Curated so the CLI + // prints the reason instead of MapFault's generic internal-error + // message; no per-bundle strike was burned. + throw new ManagementCommandException(ex.Message); + } // Register row 43: ApplyAsync releases the session on both its success // and failure paths, but every abort BEFORE it (blockers, unresolved diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementEndpoints.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementEndpoints.cs index fab85847..0d70ea5a 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementEndpoints.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementEndpoints.cs @@ -134,7 +134,14 @@ public static class ManagementEndpoints } var correlationId = Guid.NewGuid().ToString("N"); - var envelope = new ManagementEnvelope(authenticatedUser, command, correlationId); + // Carry the caller's remote IP so the Transport bundle handlers can enforce + // the per-IP-per-hour unlock throttle (register row 44). ForwardedHeaders has + // already rewritten this from X-Forwarded-For for CONFIGURED proxies only + // (Program.cs), so it is a real client address in the shipped topologies and + // the proxy's address behind an untrusted one. + var envelope = new ManagementEnvelope( + authenticatedUser, command, correlationId, + context.Connection.RemoteIpAddress?.ToString()); var askTimeout = ResolveAskTimeout( context.RequestServices.GetService>()?.Value, diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs index ffbc646c..c4263069 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs @@ -165,8 +165,22 @@ public sealed class BundleImporter : IBundleImporter _logger = logger; } + /// + /// T-004 / register row 44: the rate-limiter bucket used when a caller supplies no + /// client key. Deliberately a single SHARED key rather than a per-call bypass — an + /// unattributable caller is throttled together with every other unattributable + /// caller, so "no IP available" degrades to a stricter budget, never to none. A + /// production unlock always carries an IP (Central UI circuit, management endpoint), + /// so in practice this bucket only serves in-process callers and tests. + /// + internal const string UnattributedClientKey = ""; + /// - public async Task LoadAsync(Stream bundleStream, string? passphrase, CancellationToken ct = default) + public async Task LoadAsync( + Stream bundleStream, + string? passphrase, + CancellationToken ct = default, + string? clientKey = null) { ArgumentNullException.ThrowIfNull(bundleStream); @@ -264,6 +278,33 @@ public sealed class BundleImporter : IBundleImporter throw new BundleLockedException(manifest.ContentHash, priorFailures); } + // T-004 / register row 44: the documented per-IP-per-hour cap. The limiter + // was registered, injected and unit-tested but never invoked, so a + // brute-force attacker against a stolen bundle was bounded only by the + // per-bundle 3-strike lockout (side-steppable by re-exporting the bundle + // for a fresh ContentHash). + // + // Ordering is load-bearing: + // * AFTER the per-bundle lockout check, so hammering an already-locked + // bundle does not spend the client's hourly budget on rejected calls. + // * BEFORE the decrypt, so a THROTTLED attempt never reaches + // IncrementUnlockFailureCount — a denial must not burn a per-bundle + // strike, or the throttle would become a way to lock a bundle that an + // operator is legitimately importing. + // Registered on every unlock attempt (successful ones included) because the + // documented budget counts attempts, not failures. + var throttleKey = string.IsNullOrWhiteSpace(clientKey) + ? UnattributedClientKey + : clientKey; + var perIpCap = _options.Value.MaxUnlockAttemptsPerIpPerHour; + if (!_unlockRateLimiter.TryRegisterAttempt(throttleKey, perIpCap)) + { + _logger?.LogWarning( + "Bundle unlock throttled: client {ClientKey} exceeded {Cap} attempts in the trailing hour.", + throttleKey, perIpCap); + throw new BundleUnlockThrottledException(throttleKey, perIpCap); + } + var aad = Encryption.BundleManifestAad.Compute(manifest); try { diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/Design/TransportImportPageTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/Design/TransportImportPageTests.cs index 0848a963..74e4f21f 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/Design/TransportImportPageTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/Design/TransportImportPageTests.cs @@ -9,6 +9,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; using NSubstitute; using NSubstitute.ExceptionExtensions; +using ZB.MOM.WW.ScadaBridge.CentralUI.Auth; using ZB.MOM.WW.ScadaBridge.Commons.Entities.Sites; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; @@ -45,6 +46,9 @@ public class TransportImportPageTests : BunitContext // the Map tests) can render the wizard without a missing-service failure. private readonly ISiteRepository _siteRepo = Substitute.For(); + /// Client address the wizard reports for the per-IP unlock throttle (register row 44). + private const string TestClientIp = "203.0.113.7"; + public TransportImportPageTests() { JSInterop.Mode = JSRuntimeMode.Loose; @@ -52,6 +56,11 @@ public class TransportImportPageTests : BunitContext Services.AddSingleton(_importer); Services.AddSingleton(_auditService); + // Register row 44: the wizard passes the circuit's client IP to LoadAsync so + // the per-IP unlock throttle can attribute the attempt. bUnit has no HTTP + // context, so use the provider's explicit-address test seam. + Services.AddSingleton(new CircuitClientAddressProvider(TestClientIp)); + _siteRepo.GetAllSitesAsync(Arg.Any()) .Returns(Array.Empty()); _siteRepo.GetDataConnectionsBySiteIdAsync(Arg.Any(), Arg.Any()) @@ -132,7 +141,7 @@ public class TransportImportPageTests : BunitContext public async Task Decryption_failure_increments_attempt_counter() { // Set up the importer to throw CryptographicException for wrong passphrases. - _importer.LoadAsync(Arg.Any(), Arg.Any(), Arg.Any()) + _importer.LoadAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) .Throws(new CryptographicException("authentication tag mismatch")); var cut = Render(); @@ -162,7 +171,7 @@ public class TransportImportPageTests : BunitContext [Fact] public async Task Three_failed_unlocks_force_reupload() { - _importer.LoadAsync(Arg.Any(), Arg.Any(), Arg.Any()) + _importer.LoadAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) .Throws(new CryptographicException("authentication tag mismatch")); var cut = Render(); @@ -725,7 +734,8 @@ public class TransportImportPageTests : BunitContext return Store.Open(session); } - public Task LoadAsync(Stream bundleStream, string? passphrase, CancellationToken ct = default) + public Task LoadAsync( + Stream bundleStream, string? passphrase, CancellationToken ct = default, string? clientKey = null) => throw new NotSupportedException("These tests seed sessions via OpenSession()."); public Task PreviewAsync(Guid sessionId, CancellationToken ct = default) diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/QueryStringDrillInTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/QueryStringDrillInTests.cs index 7d21bdd1..5026caf6 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/QueryStringDrillInTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Pages/QueryStringDrillInTests.cs @@ -234,6 +234,10 @@ public sealed class QueryStringDrillInTests Services.AddSingleton(Substitute.For()); // M8 E2: TransportImport's Map step injects ISiteRepository. Services.AddSingleton(Substitute.For()); + // Register row 44: the wizard reads the circuit's client IP for the + // per-IP unlock throttle. No HTTP context under bUnit — use the seam. + Services.AddSingleton(new ZB.MOM.WW.ScadaBridge.CentralUI.Auth.CircuitClientAddressProvider( + clientAddress: null)); Services.AddSingleton>( Microsoft.Extensions.Options.Options.Create(new TransportOptions { diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs index 66366757..84ff8003 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs @@ -2734,7 +2734,7 @@ public class ManagementActorTests : TestKit, IDisposable var (_, importer) = AddBundleSubstitutes(); var sessionId = Guid.NewGuid(); - importer.LoadAsync(Arg.Any(), Arg.Any(), Arg.Any()) + importer.LoadAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) .Returns(new Commons.Types.Transport.BundleSession { SessionId = sessionId, @@ -2787,7 +2787,7 @@ public class ManagementActorTests : TestKit, IDisposable var (_, importer) = AddBundleSubstitutes(); var sessionId = Guid.NewGuid(); - importer.LoadAsync(Arg.Any(), Arg.Any(), Arg.Any()) + importer.LoadAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) .Returns(new Commons.Types.Transport.BundleSession { SessionId = sessionId, @@ -2865,7 +2865,7 @@ public class ManagementActorTests : TestKit, IDisposable IReadOnlyList connectionMappings) { var sessionId = Guid.NewGuid(); - importer.LoadAsync(Arg.Any(), Arg.Any(), Arg.Any()) + importer.LoadAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) .Returns(new Commons.Types.Transport.BundleSession { SessionId = sessionId, @@ -3089,7 +3089,7 @@ public class ManagementActorTests : TestKit, IDisposable var (_, importer) = AddBundleSubstitutes(); var sessionId = Guid.NewGuid(); - importer.LoadAsync(Arg.Any(), Arg.Any(), Arg.Any()) + importer.LoadAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) .Returns(new Commons.Types.Transport.BundleSession { SessionId = sessionId, @@ -3126,6 +3126,99 @@ public class ManagementActorTests : TestKit, IDisposable Assert.Contains("OpcA", success.JsonData); } + [Fact] + public void PreviewBundleCommand_ReleasesTheSession_AndForwardsTheCallersIp() + { + // Register row 43: a preview never applies, so nothing else releases the + // session — it used to hold one of MaxConcurrentImportSessions for the full + // TTL. Register row 44: the envelope's client IP must reach LoadAsync so the + // per-IP-per-hour unlock throttle can attribute the attempt. + var (_, importer) = AddBundleSubstitutes(); + + var sessionId = Guid.NewGuid(); + importer.LoadAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(new Commons.Types.Transport.BundleSession + { + SessionId = sessionId, + Manifest = null!, + ExpiresAt = DateTimeOffset.UtcNow.AddMinutes(5), + }); + importer.PreviewAsync(sessionId, Arg.Any()) + .Returns(new Commons.Types.Transport.ImportPreview( + sessionId, + Array.Empty())); + + var payload = Convert.ToBase64String(new byte[] { 0x01, 0x02, 0x03 }); + var actor = CreateActor(); + var envelope = Envelope(new PreviewBundleCommand(payload, null), "Administrator") + with { ClientIp = "198.51.100.9" }; + + actor.Tell(envelope); + + ExpectMsg(TimeSpan.FromSeconds(5)); + importer.Received(1).LoadAsync( + Arg.Any(), Arg.Any(), Arg.Any(), "198.51.100.9"); + importer.Received(1).Abandon(sessionId); + } + + [Fact] + public void ImportBundleCommand_WithBlockerRow_StillReleasesTheSession() + { + // Register row 43: ApplyAsync releases the session, but this import aborts + // BEFORE apply — the handler must release it anyway. + var (_, importer) = AddBundleSubstitutes(); + + var sessionId = Guid.NewGuid(); + importer.LoadAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(new Commons.Types.Transport.BundleSession + { + SessionId = sessionId, + Manifest = null!, + ExpiresAt = DateTimeOffset.UtcNow.AddMinutes(5), + }); + importer.PreviewAsync(sessionId, Arg.Any()) + .Returns(new Commons.Types.Transport.ImportPreview( + sessionId, + new[] + { + new Commons.Types.Transport.ImportPreviewItem( + EntityType: "Template", + Name: "BlockedTemplate", + ExistingVersion: null, + IncomingVersion: 1, + Kind: Commons.Types.Transport.ConflictKind.Blocker, + FieldDiffJson: null, + BlockerReason: "FK to missing site"), + })); + + var payload = Convert.ToBase64String(new byte[] { 0x01, 0x02, 0x03 }); + var actor = CreateActor(); + actor.Tell(Envelope(new ImportBundleCommand(payload, null, "skip"), "Administrator")); + + ExpectMsg(TimeSpan.FromSeconds(5)); + importer.Received(1).Abandon(sessionId); + } + + [Fact] + public void ImportBundleCommand_ThrottledUnlock_SurfacesTheCuratedReason() + { + // Register row 44: a per-IP throttle denial must fail closed with a clear + // message, not MapFault's generic "an internal error occurred". + var (_, importer) = AddBundleSubstitutes(); + + importer.LoadAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(_ => + throw new Commons.Types.Transport.BundleUnlockThrottledException("198.51.100.9", 10)); + + var payload = Convert.ToBase64String(new byte[] { 0x01, 0x02, 0x03 }); + var actor = CreateActor(); + actor.Tell(Envelope(new ImportBundleCommand(payload, "pass", "skip"), "Administrator")); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Equal("COMMAND_FAILED", response.ErrorCode); + Assert.Contains("Too many bundle unlock attempts", response.Error); + } + // ======================================================================== // Native alarm source CRUD (Task 21) // ======================================================================== diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/TransportGateTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/TransportGateTests.cs index 7b84db27..c687570e 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/TransportGateTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/TransportGateTests.cs @@ -53,7 +53,7 @@ public class TransportGateTests : TestKit, IDisposable var loadCalls = 0; var session = new BundleSession { SessionId = Guid.NewGuid() }; - _importer.LoadAsync(Arg.Any(), Arg.Any(), Arg.Any()) + _importer.LoadAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) .Returns(_ => { Interlocked.Increment(ref loadCalls); diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleImporterLoadTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleImporterLoadTests.cs index 2148f7f0..d901fa92 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleImporterLoadTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/BundleImporterLoadTests.cs @@ -502,6 +502,183 @@ public sealed class BundleImporterLoadTests () => rig.Importer.LoadAsync(stream, passphrase: null)); } + // ───────────────────────────────────────────────────────────────────── + // Register row 44 — the per-IP-per-hour unlock throttle is actually enforced. + // BundleUnlockRateLimiter was registered, injected and unit-tested but never + // invoked, so these all passed-by-doing-nothing before the wiring. + // ───────────────────────────────────────────────────────────────────── + + [Fact] + public async Task LoadAsync_rejects_unlock_attempts_past_the_per_ip_hourly_cap() + { + // Per-bundle strikes are effectively disabled here (100) so the throttle is + // unambiguously what stops the run. + var rig = BuildRig(opts => + { + opts.MaxUnlockAttemptsPerIpPerHour = 3; + opts.MaxUnlockAttemptsPerSession = 100; + }); + using var packed = PackEncryptedBundle( + rig.Serializer, rig.ManifestBuilder, rig.Encryptor, EmptyContent(), "correct"); + var bundleBytes = ((MemoryStream)packed).ToArray(); + + for (var attempt = 1; attempt <= 3; attempt++) + { + await Assert.ThrowsAnyAsync( + () => rig.Importer.LoadAsync( + new MemoryStream(bundleBytes), passphrase: "wrong", clientKey: "10.1.1.5")); + } + + var throttled = await Assert.ThrowsAsync( + () => rig.Importer.LoadAsync( + new MemoryStream(bundleBytes), passphrase: "wrong", clientKey: "10.1.1.5")); + Assert.Equal(3, throttled.MaxAttemptsPerWindow); + Assert.Equal("10.1.1.5", throttled.ClientKey); + + // Fail closed: even the CORRECT passphrase is refused while the client is + // over budget. + await Assert.ThrowsAsync( + () => rig.Importer.LoadAsync( + new MemoryStream(bundleBytes), passphrase: "correct", clientKey: "10.1.1.5")); + } + + [Fact] + public async Task Throttled_unlock_is_not_a_passphrase_failure_and_burns_no_strike() + { + // A throttled attempt never reaches the decryptor, so it must not consume one + // of the bundle's three strikes — otherwise the per-IP throttle would become a + // way to lock a bundle that an operator is legitimately importing. + var rig = BuildRig(opts => + { + opts.MaxUnlockAttemptsPerIpPerHour = 1; + opts.MaxUnlockAttemptsPerSession = 3; + }); + using var packed = PackEncryptedBundle( + rig.Serializer, rig.ManifestBuilder, rig.Encryptor, EmptyContent(), "correct"); + var bundleBytes = ((MemoryStream)packed).ToArray(); + var contentHash = rig.Serializer.ReadManifest(new MemoryStream(bundleBytes)).ContentHash; + + await Assert.ThrowsAnyAsync( + () => rig.Importer.LoadAsync( + new MemoryStream(bundleBytes), passphrase: "wrong", clientKey: "10.1.1.6")); + Assert.Equal(1, rig.SessionStore.GetUnlockFailureCount(contentHash)); + + await Assert.ThrowsAsync( + () => rig.Importer.LoadAsync( + new MemoryStream(bundleBytes), passphrase: "wrong", clientKey: "10.1.1.6")); + + // Still 1 — the denial burned no strike, so the bundle is not locked. + Assert.Equal(1, rig.SessionStore.GetUnlockFailureCount(contentHash)); + } + + [Fact] + public async Task Unlock_throttle_is_isolated_per_client() + { + // One noisy client must not spend another operator's budget. + var rig = BuildRig(opts => + { + opts.MaxUnlockAttemptsPerIpPerHour = 1; + opts.MaxUnlockAttemptsPerSession = 100; + }); + using var packed = PackEncryptedBundle( + rig.Serializer, rig.ManifestBuilder, rig.Encryptor, EmptyContent(), "correct"); + var bundleBytes = ((MemoryStream)packed).ToArray(); + + await Assert.ThrowsAnyAsync( + () => rig.Importer.LoadAsync( + new MemoryStream(bundleBytes), passphrase: "wrong", clientKey: "10.1.1.7")); + await Assert.ThrowsAsync( + () => rig.Importer.LoadAsync( + new MemoryStream(bundleBytes), passphrase: "wrong", clientKey: "10.1.1.7")); + + // A different client still has its full budget — reaches the decryptor and + // fails on the passphrase, not on the throttle. + await Assert.ThrowsAnyAsync( + () => rig.Importer.LoadAsync( + new MemoryStream(bundleBytes), passphrase: "wrong", clientKey: "10.1.1.8")); + } + + [Fact] + public async Task Unattributed_callers_share_one_throttle_bucket_rather_than_bypassing_it() + { + // Conservative fallback: a caller with no IP is counted against a single + // shared bucket, so "unknown client" is a stricter budget, never none. + var rig = BuildRig(opts => + { + opts.MaxUnlockAttemptsPerIpPerHour = 1; + opts.MaxUnlockAttemptsPerSession = 100; + }); + using var packed = PackEncryptedBundle( + rig.Serializer, rig.ManifestBuilder, rig.Encryptor, EmptyContent(), "correct"); + var bundleBytes = ((MemoryStream)packed).ToArray(); + + await Assert.ThrowsAnyAsync( + () => rig.Importer.LoadAsync(new MemoryStream(bundleBytes), passphrase: "wrong")); + + // Blank is treated the same as null — both land in the shared bucket, which + // the first attempt already emptied. + var throttled = await Assert.ThrowsAsync( + () => rig.Importer.LoadAsync( + new MemoryStream(bundleBytes), passphrase: "wrong", clientKey: " ")); + Assert.Equal(BundleImporter.UnattributedClientKey, throttled.ClientKey); + } + + [Fact] + public async Task Unencrypted_bundles_do_not_consume_the_unlock_budget() + { + // The throttle guards passphrase brute-forcing; a plain bundle has no unlock + // step, so repeated loads must not be able to exhaust an operator's budget. + var rig = BuildRig(opts => opts.MaxUnlockAttemptsPerIpPerHour = 1); + using var packed = PackPlainBundle(rig.Serializer, rig.ManifestBuilder, SmallContent()); + var bundleBytes = ((MemoryStream)packed).ToArray(); + + for (var i = 0; i < 3; i++) + { + var session = await rig.Importer.LoadAsync( + new MemoryStream(bundleBytes), passphrase: null, clientKey: "10.1.1.9"); + Assert.NotNull(session); + rig.Importer.Abandon(session.SessionId); + } + } + + [Fact] + public async Task An_already_locked_bundle_does_not_spend_the_clients_hourly_budget() + { + // Ordering pin: the per-bundle lockout is checked BEFORE the throttle, so + // hammering a locked bundle cannot burn through the client's hourly budget + // and spill over into denying its next legitimate import. + var rig = BuildRig(opts => + { + opts.MaxUnlockAttemptsPerSession = 1; + opts.MaxUnlockAttemptsPerIpPerHour = 2; + }); + using var packed = PackEncryptedBundle( + rig.Serializer, rig.ManifestBuilder, rig.Encryptor, EmptyContent(), "correct"); + var bundleBytes = ((MemoryStream)packed).ToArray(); + + // One wrong attempt locks the bundle (cap 1) and spends 1 of 2 IP budget. + await Assert.ThrowsAsync( + () => rig.Importer.LoadAsync( + new MemoryStream(bundleBytes), passphrase: "wrong", clientKey: "10.1.1.10")); + + // Three more attempts on the locked bundle stay BundleLocked — none of them + // register against the rate limiter. + for (var i = 0; i < 3; i++) + { + await Assert.ThrowsAsync( + () => rig.Importer.LoadAsync( + new MemoryStream(bundleBytes), passphrase: "wrong", clientKey: "10.1.1.10")); + } + + // A DIFFERENT bundle from the same client still has budget left. + using var other = PackEncryptedBundle( + rig.Serializer, rig.ManifestBuilder, rig.Encryptor, SmallContent(), "correct2"); + var otherSession = await rig.Importer.LoadAsync( + new MemoryStream(((MemoryStream)other).ToArray()), + passphrase: "correct2", clientKey: "10.1.1.10"); + Assert.NotNull(otherSession); + } + // ───────────────────────────────────────────────────────────────────── // Register row 43 — abandoned-session release. // ─────────────────────────────────────────────────────────────────────