fix(security): pin the cached-key expiry guard, meter cache_hit after it, and close two wiring gaps (review fixes)

archreview/plans/R3-05-acl-hot-path-expiry-plan.md

@@ -781,3 +781,39 @@ Two notes for whoever runs it:
    cannot resurrect a clock-expired key.
 4. New public members added purely so wiring could be pinned by reference: `DataPlaneAuthorizer.Clock`,
    `BoundedApiKeyStore.Inner` / `.Clock`.
+
+---
+
+## Review outcome — FIX-FIRST, applied 2026-08-20
+
+Coordinator verdict on the branch was **FIX-FIRST** with four items. All four applied; Host.Tests and
+OpcUaServer.Tests rerun green.
+
+1. **[Important] `IsExpired` had no test — the review was right, and the gap was mine.** T3's execution
+   note called the guard a "fail-closed refinement" and shipped it with *no case that fails without it*:
+   deleting `&& !IsExpired(cached, utcNow)` left all 14 `BoundedApiKeyStoreTests` green. That is the
+   unchanged-observable trap — the guard's whole effect is invisible unless a cached entry outlives its
+   key's `ExpiresUtc` **inside** the TTL, which no existing case constructed.
+   Added **`A_cached_key_that_expires_by_the_clock_is_not_served`**: key expiring at `start + 2 s`, TTL
+   60 s, first `FindAsync` non-null (and now cached); flip the inner store to `Revoked`; advance the fake
+   clock 5 s — past `ExpiresUtc`, far inside the TTL; assert `null` and `inner.Calls == 2`. The
+   `Revoked` flip is deliberate: it makes "served from cache" and "re-read" differ in the **answer**, not
+   only in a call count, so a build that re-read and ignored the result cannot pass either.
+   **RED confirmed by falsification:** with the guard removed, exactly **1 failed / 14 passed** — only the
+   new case — then restored, 15/15 green.
+2. **[Minor] `cache_hit` was metered before the expiry guard could reject.** `TryServe` counted a hit for
+   an entry that then fell through to a store read, so the counter over-reported. The `Add` moved to the
+   two call sites, **after** the guard, behind a `MeterCacheHit()` helper; `TryServe` is now purely a
+   lookup. This matters operationally — `cache_hit` is the ratio §"Bounding the key store" tells an
+   operator to watch.
+3. **[Minor] Added `A_node_with_neither_role_registers_the_deny_all_authenticator`** — asserts the
+   `IOpcUaUserAuthenticator` singleton **instance** is `NullOpcUaUserAuthenticator.Instance` and that no
+   `IApiKeyStore` is registered. It closes the third branch of the role conditional, which the guard suite
+   had left unpinned even though that branch is the one whose absence stops a node booting.
+4. **[Minor] `StartRevalidationTimer` now disposes before overwriting.** `StartAsync` can run again when a
+   host restarts the service around the same instance, and the overwritten field would have left the
+   previous timer armed with nothing left to dispose it — the dispose-before-overwrite rule the driver
+   reconnect paths already carry.
+
+**Verification after the fixes:** solution build 0 errors · Host.Tests **134 passed / 0 failed** (was 132)
+· OpcUaServer.Tests **493 passed / 1 skipped**. T11 remains `pending` for the orchestrator.

src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs

@@ -108,6 +108,10 @@ public sealed class BoundedApiKeyStore : IApiKeyStore
             && TryServe(_keys, apiKeyId, ttl, out var cached)
             && !IsExpired(cached, utcNow))
         {
+            // Metered HERE rather than inside TryServe: an entry that is inside the TTL but whose key has
+            // since expired is not a hit — it falls through to a store read — so counting it in TryServe
+            // would report cache hits that never happened.
+            MeterCacheHit();
             return cached;
         }
 
@@ -122,7 +126,11 @@ public sealed class BoundedApiKeyStore : IApiKeyStore
     public async Task<IReadOnlyList<NodeAcl>> FindGrantsAsync(string principal, CancellationToken ct)
     {
         var ttl = Ttl;
-        if (ttl > TimeSpan.Zero && TryServe(_grants, principal, ttl, out var cached)) return cached;
+        if (ttl > TimeSpan.Zero && TryServe(_grants, principal, ttl, out var cached))
+        {
+            MeterCacheHit();
+            return cached;
+        }
 
         var fresh = await BoundAsync(token => _inner.FindGrantsAsync(principal, token), ct)
             .ConfigureAwait(false);
@@ -140,6 +148,10 @@ public sealed class BoundedApiKeyStore : IApiKeyStore
     private static bool IsExpired(ApiKeyLookup? cached, DateTime utcNow) =>
         cached?.Key.ExpiresUtc is { } expires && expires <= utcNow;
 
+    /// <summary>Counts one lookup the cache actually answered.</summary>
+    private static void MeterCacheHit() =>
+        OtOpcUaTelemetry.ApiKeyStoreBounded.Add(1, new KeyValuePair<string, object?>("outcome", "cache_hit"));
+
     /// <summary>Serves an entry if one is present and still inside the TTL.</summary>
     /// <typeparam name="T">The cached value type.</typeparam>
     /// <param name="cache">The cache to read.</param>
@@ -152,8 +164,6 @@ public sealed class BoundedApiKeyStore : IApiKeyStore
     {
         if (cache.TryGetValue(key, out var entry) && _time.GetUtcNow() - entry.StampedUtc < ttl)
         {
-            OtOpcUaTelemetry.ApiKeyStoreBounded.Add(
-                1, new KeyValuePair<string, object?>("outcome", "cache_hit"));
             value = entry.Value;
             return true;
         }

src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs

@@ -307,12 +307,19 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl
     ///     </para>
     ///     <para>Separate from <see cref="StartAsync"/> so the timer's lifecycle is testable without booting the SDK.</para>
     /// </remarks>
-    internal void StartRevalidationTimer() =>
+    internal void StartRevalidationTimer()
+    {
+        // Dispose before overwriting. StartAsync runs again when a host restarts the service around the
+        // same instance, and an overwritten field would leave the previous timer armed with nothing left
+        // to dispose it — the same dispose-before-overwrite rule the driver reconnect paths carry.
+        _revalidationTimer?.Dispose();
+
         _revalidationTimer = new AclRevalidationTimer(
             () => _server?.NodeManager?.RevalidateMonitoredItems(),
             TimeSpan.FromSeconds(_aclOptions.RevalidationIntervalSeconds),
             _time,
             _loggerFactory.CreateLogger<AclRevalidationTimer>());
+    }
 
     /// <summary>
     /// Stops the OPC UA server asynchronously.

tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/ApiKeyStoreRegistrationTests.cs

@@ -76,6 +76,26 @@ public sealed class ApiKeyStoreRegistrationTests
         sp.GetRequiredService<IApiKeyStore>().ShouldBeOfType<ArtifactApiKeyStore>();
     }
 
+    /// <summary>
+    ///     A node carrying neither role registers the deny-all authenticator <b>explicitly</b>, and no key
+    ///     store at all.
+    /// </summary>
+    /// <remarks>
+    ///     Explicitly, not by absence: <c>OtOpcUaServerHostedService</c> takes
+    ///     <see cref="IOpcUaUserAuthenticator"/> as a required constructor parameter, and the built-in
+    ///     container does not fall back to a default for an unregistered service — it throws, and the node
+    ///     does not start. The assertion is on the singleton INSTANCE, so a future registration that
+    ///     happened to resolve some other deny-all type still surfaces here.
+    /// </remarks>
+    [Fact]
+    public void A_node_with_neither_role_registers_the_deny_all_authenticator()
+    {
+        using var sp = BuildNode(hasAdmin: false, hasDriver: false);
+
+        sp.GetRequiredService<IOpcUaUserAuthenticator>().ShouldBeSameAs(NullOpcUaUserAuthenticator.Instance);
+        sp.GetService<IApiKeyStore>().ShouldBeNull("a node that serves no data plane has no keys to read");
+    }
+
     /// <summary>Builds the data-plane authentication wiring for one node role combination.</summary>
     /// <param name="hasAdmin">Whether the node carries the <c>admin</c> role.</param>
     /// <param name="hasDriver">Whether the node carries the <c>driver</c> role.</param>

tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreTests.cs

@@ -272,6 +272,43 @@ public sealed class BoundedApiKeyStoreTests
         inner.Calls.ShouldBe(callsAfterFlood + 1);
     }
 
+    /// <summary>
+    ///     A cached key whose <c>ExpiresUtc</c> passes <b>inside</b> the TTL is not served: the store is
+    ///     re-read instead.
+    /// </summary>
+    /// <remarks>
+    ///     <para>
+    ///         Without this the TTL would resurrect an expired key for up to <c>StoreCacheSeconds</c> —
+    ///         the exact axis 03/S19 exists to close, re-opened by 03/S18's own cache. The cache key is
+    ///         <c>apiKeyId</c> alone (<c>utcNow</c> changes every call and would defeat the cache), so the
+    ///         clock check has to happen on the way OUT of the cache.
+    ///     </para>
+    ///     <para>
+    ///         The inner store is flipped to <c>Revoked</c> first, so "served from cache" and "re-read"
+    ///         give different ANSWERS rather than only different call counts — a build that re-read and
+    ///         then ignored the result could not pass.
+    ///     </para>
+    /// </remarks>
+    [Fact]
+    public async Task A_cached_key_that_expires_by_the_clock_is_not_served()
+    {
+        var start = new DateTimeOffset(2026, 8, 20, 12, 0, 0, TimeSpan.Zero);
+        var (_, inner) = SetUp(expiresUtc: start.UtcDateTime.AddSeconds(2));
+        var clock = new FakeClock(start);
+        var sut = Bounded(inner, timeoutMs: 0, cacheSeconds: 60, clock);
+
+        (await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default))
+            .ShouldNotBeNull("non-vacuity: the key was usable and IS now cached");
+
+        inner.Revoked = true;
+
+        // Past ExpiresUtc (+2 s) but far inside the 60 s TTL — the entry is still "fresh" to the cache.
+        clock.Advance(TimeSpan.FromSeconds(5));
+
+        (await sut.FindAsync(inner.KeyId, clock.GetUtcNow().UtcDateTime, default)).ShouldBeNull();
+        inner.Calls.ShouldBe(2, "the expired entry must be re-read, not served");
+    }
+
     // ---------- metering ----------
 
     /// <summary>A deadline that fires meters <c>outcome=timeout</c> — the alarm signal for a sick ConfigDb.</summary>
@@ -347,7 +384,7 @@ public sealed class BoundedApiKeyStoreTests
         Options.Create(new ApiKeyOptions { Username = "apikey", Pepper = Pepper }),
         NullLogger<ApiKeyOpcUaUserAuthenticator>.Instance);
 
-    private static (string Presented, GatedApiKeyStore Store) SetUp()
+    private static (string Presented, GatedApiKeyStore Store) SetUp(DateTime? expiresUtc = null)
     {
         var (presented, apiKeyId, hash) = ApiKeySecret.Generate(Pepper);
         var key = new ApiKey
@@ -358,6 +395,7 @@ public sealed class BoundedApiKeyStoreTests
             Principal = Principal,
             Enabled = true,
             CreatedUtc = DateTime.UtcNow,
+            ExpiresUtc = expiresUtc,
         };
         return (presented, new GatedApiKeyStore(key));
     }