From 1b88ca1296597a3e0cdaf2bbc5828dd4b46b38c5 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 11:03:26 -0400 Subject: [PATCH] fix(security): amortize LoginThrottle.Prune off the failed-bind hot path (plan R2-07 T11) --- .../LoginThrottle.cs | 20 +++++++++++- .../LoginThrottleTests.cs | 31 +++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.Security/LoginThrottle.cs b/src/ZB.MOM.WW.ScadaBridge.Security/LoginThrottle.cs index 8d2a46bd..164d6b65 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Security/LoginThrottle.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Security/LoginThrottle.cs @@ -41,6 +41,18 @@ public sealed class LoginThrottle /// public const int MaxTrackedKeys = 10_000; + /// Amortization cadence: a full prune sweep runs every Nth failure write + /// (plus immediately whenever the map exceeds ), so a + /// spray does not pay an O(n) scan — and a possible full sort — on every failed + /// request (arch-review R2 N8). Lockout SEMANTICS are unaffected: IsLockedOut reads + /// window/lockout expiry directly and never depends on pruning. + internal const int PruneEveryNFailures = 64; + + /// Diagnostics/test accessor: number of currently tracked keys. + internal int TrackedKeyCount => _entries.Count; + + private int _failureWrites; + private readonly TimeProvider _clock; private readonly IOptions _options; @@ -118,7 +130,13 @@ public sealed class LoginThrottle return Arm(existing with { Count = existing.Count + 1 }, opts, now, lockout); }); - Prune(now); + // Amortized prune (R2 N8): every Nth failure, or immediately when over the hard + // cap — the cap is the memory-safety invariant and must never wait for the cadence. + if (Interlocked.Increment(ref _failureWrites) % PruneEveryNFailures == 0 + || _entries.Count > MaxTrackedKeys) + { + Prune(now); + } } /// diff --git a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/LoginThrottleTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/LoginThrottleTests.cs index 47f26d09..91ed4b1c 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/LoginThrottleTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/LoginThrottleTests.cs @@ -25,6 +25,12 @@ public class LoginThrottleTests private static LoginThrottle Create(TimeProvider clock, SecurityOptions? options = null) => new(clock, Options.Create(options ?? new SecurityOptions())); + private static (LoginThrottle throttle, TestTimeProvider clock) Create() + { + var clock = new TestTimeProvider(Start); + return (Create(clock), clock); + } + [Fact] public void FiveFailuresInWindow_LocksOut_ThenWindowExpiryUnlocks() { @@ -110,4 +116,29 @@ public class LoginThrottleTests Assert.False(throttle.IsLockedOut("alice", "10.0.0.1")); } + + // ── arch-review R2 N8: amortized prune ───────────────────────────────────── + + [Fact] + public void KeySpray_MapStaysBounded_AtTheHardCap() + { + var (throttle, _) = Create(); // existing helper: throttle over a fake clock + // Spray far past the cap: the over-cap eviction must run on EVERY write once + // the cap is exceeded, regardless of the amortized cadence (arch-review R2 N8). + for (var i = 0; i < LoginThrottle.MaxTrackedKeys + 500; i++) + throttle.RecordFailure($"user{i}", "10.0.0.1"); + Assert.True(throttle.TrackedKeyCount <= LoginThrottle.MaxTrackedKeys); + } + + [Fact] + public void ExpiredEntries_AreEventuallySwept_OnTheAmortizedCadence() + { + var (throttle, clock) = Create(); + throttle.RecordFailure("stale", "10.0.0.1"); + clock.Advance(TimeSpan.FromMinutes(10)); // window fully expired + for (var i = 0; i < LoginThrottle.PruneEveryNFailures; i++) + throttle.RecordFailure($"fresh{i}", "10.0.0.1"); // cadence tick fires within these + Assert.False(throttle.IsLockedOut("stale", "10.0.0.1")); + Assert.True(throttle.TrackedKeyCount <= LoginThrottle.PruneEveryNFailures); // "stale" swept + } }