fix(acl): revalidation memo rejects empty node identifiers, mirroring the authorizer's guard

2026-08-17 03:47:19 -04:00
parent e22ecea008
commit c4d8e4a200
2 changed files with 72 additions and 6 deletions
@@ -3298,9 +3298,12 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
    ///         identifier spaces, so an identifier alone does not name a node.
    ///     </para>
    ///     <para>
-    ///         Anything the key cannot be built from — no role-carrying identity, no principal, a non-string
-    ///         node identifier — falls through to the un-memoized call and keeps its existing verdict. Those
-    ///         are the cases the authorizer denies anyway; memoizing them would only make a deny cheaper.
+    ///         Anything the key cannot be built from — no role-carrying identity, no principal, a node
+    ///         identifier that is not a non-empty string — falls through to the un-memoized call and keeps
+    ///         its existing verdict. Those are the cases the authorizer denies anyway; memoizing them would
+    ///         only make a deny cheaper. The empty identifier is excluded for the same reason
+    ///         <see cref="Security.DataPlaneAuthorizer"/> excludes it from its own decision cache: it
+    ///         addresses no ACL scope, so it is not an identity to key on.
    ///     </para>
    ///     <para>
    ///         This memo does cover the key expiry check that the authorizer's own cache excludes, but only
@@ -3318,7 +3321,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2

        if (identity is null
            || (identity.ApiKeyId ?? identity.Principal) is not { Length: > 0 } sessionKey
-            || nodeId.Identifier is not string identifier)
+            || nodeId.Identifier is not string { Length: > 0 } identifier)
        {
            return _authorizer!.IsAllowedCached(
                identity, OpcUaOperation.CreateMonitoredItems, nodeId, realm);
@@ -519,6 +519,62 @@ public sealed class NodeManagerStage6EnforcementTests : IDisposable
        finally { await host.DisposeAsync(); }
    }

+    /// <summary>
+    ///     An EMPTY <c>s=</c> identifier names no node, so it must not be memoized: every item on such a
+    ///     node asks the authorizer for itself.
+    /// </summary>
+    /// <remarks>
+    ///     <para>
+    ///         An empty identifier addresses no ACL scope at all —
+    ///         <see cref="Security.DataPlaneAuthorizer"/> rejects it before its own decision cache for
+    ///         exactly that reason — so collapsing every item on one onto a single pass-local verdict is
+    ///         keying the memo on something that is not an identity. The guard mirrors the authorizer's:
+    ///         an empty identifier falls through to the un-memoized call, which is fail-safe (each item
+    ///         gets its own verdict) rather than merely cheaper.
+    ///     </para>
+    ///     <para>
+    ///         Two items on DISTINCT empty-identifier nodes cannot be constructed — an empty identifier in
+    ///         one realm is one node id — so what is pinned here is the bypass itself: the authorizer is
+    ///         asked once per item, where the memoized path asks once per pass.
+    ///         <see cref="Revalidation_evaluates_one_verdict_per_session_and_node"/> is the control that
+    ///         the memo still engages for a non-empty identifier, so this cannot pass by the memo having
+    ///         been removed.
+    ///     </para>
+    /// </remarks>
+    [Fact]
+    public async Task Revalidation_does_not_memoize_an_empty_node_identifier()
+    {
+        var (host, server) = await BootAsync();
+        try
+        {
+            var nm = server.NodeManager!;
+            var clock = new CountingTimeProvider(new DateTime(2026, 8, 15, 12, 0, 0, DateTimeKind.Utc));
+            nm.Authorizer = SubscribeAuthorizer(clock);
+
+            // The manager's raw namespace index, borrowed from a real node, with an empty identifier.
+            var rawNamespace = Variable(nm, "Plant/Modbus/dev1/Speed").NamespaceIndex;
+            var emptyId = new NodeId(string.Empty, rawNamespace);
+
+            // Non-vacuity: this really is a STRING identifier of length zero, the one case the guard used
+            // to let through — not a null identifier the guard already rejected.
+            emptyId.Identifier.ShouldBeOfType<string>().ShouldBeEmpty();
+
+            var identity = Identity();
+            var items = Enumerable.Range(0, 2)
+                .Select(_ => TrackItem(nm, new NodeHandle { NodeId = emptyId }, identity))
+                .ToList();
+
+            clock.Reset();
+            nm.RevalidateMonitoredItems();
+
+            clock.Calls.ShouldBe(2, "an empty identifier is not a memo key, so each item is asked for itself");
+
+            // And the un-memoized verdict is the authorizer's own deny, reaching every item.
+            items.ShouldAllBe(i => i.Denials == 1);
+        }
+        finally { await host.DisposeAsync(); }
+    }
+
    // ───────────────────────────── helpers ─────────────────────────────

    /// <summary>A cluster-wide Subscribe grant — what <c>CreateMonitoredItems</c> resolves to.</summary>
@@ -531,9 +587,16 @@ public sealed class NodeManagerStage6EnforcementTests : IDisposable
    }

    /// <summary>Registers a stub data-change item against <paramref name="nodeId"/> as if the SDK had created it.</summary>
-    private static StubSampledItem TrackItem(OtOpcUaNodeManager nm, NodeId nodeId, IUserIdentity identity)
+    private static StubSampledItem TrackItem(OtOpcUaNodeManager nm, NodeId nodeId, IUserIdentity identity) =>
+        TrackItem(nm, (NodeHandle)nm.GetManagerHandle(nodeId), identity);
+
+    /// <summary>
+    ///     Registers a stub data-change item against a hand-built <paramref name="handle"/> — the only way
+    ///     to reach a node id the address space cannot mint, such as an empty <c>s=</c> identifier.
+    /// </summary>
+    private static StubSampledItem TrackItem(
+        OtOpcUaNodeManager nm, NodeHandle handle, IUserIdentity identity)
    {
-        var handle = (NodeHandle)nm.GetManagerHandle(nodeId);
        var item = new StubSampledItem(handle, identity);
        nm.TrackLiveMonitoredItem(handle, item);
        return item;