diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index 14c61341..35db4d74 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -3298,9 +3298,12 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 /// identifier spaces, so an identifier alone does not name a node. /// /// - /// 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 + /// excludes it from its own decision cache: it + /// addresses no ACL scope, so it is not an identity to key on. /// /// /// 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); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs index e712777e..a0b3c57d 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs @@ -519,6 +519,62 @@ public sealed class NodeManagerStage6EnforcementTests : IDisposable finally { await host.DisposeAsync(); } } + /// + /// An EMPTY s= identifier names no node, so it must not be memoized: every item on such a + /// node asks the authorizer for itself. + /// + /// + /// + /// An empty identifier addresses no ACL scope at all — + /// 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. + /// + /// + /// 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. + /// is the control that + /// the memo still engages for a non-empty identifier, so this cannot pass by the memo having + /// been removed. + /// + /// + [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().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 ───────────────────────────── /// A cluster-wide Subscribe grant — what CreateMonitoredItems resolves to. @@ -531,9 +587,16 @@ public sealed class NodeManagerStage6EnforcementTests : IDisposable } /// Registers a stub data-change item against as if the SDK had created it. - 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); + + /// + /// Registers a stub data-change item against a hand-built — the only way + /// to reach a node id the address space cannot mint, such as an empty s= identifier. + /// + 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;