From a5c0c82661314e74b6f23721a337627a643ef0e4 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 15 Jun 2026 02:45:51 -0400 Subject: [PATCH] fix(opcua): address code review on write-outcome surfacing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - A.1 (false-rejection safety): restrict the structural fail-fast's confident-mismatch check to the CLOSED set of built-in types ResolveBuiltInDataType emits (numeric families + Boolean/ String/DateTime/ByteString). Any other expected type (Enumeration, Guid, …) now defers to the SDK, so a coercible write (Int32→Enumeration) is never false-rejected. + A7/A8 regression tests. - C.1: guard BuildWriteFailureAuditEvent (under Lock) in try/catch like ReportAuditEvent, so a SetChildValue surprise is swallowed+logged, never thrown out of the fire-and-forget continuation. --- .../OtOpcUaNodeManager.cs | 28 ++++++++++++++++++- .../EquipmentWriteGateTests.cs | 20 +++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index 7cc28982..747b3d43 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -828,6 +828,12 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 var expected = TypeInfo.GetBuiltInType(variable.DataType); // NodeId ⇒ built-in; abstract/unknown ⇒ Null if (expected is BuiltInType.Null or BuiltInType.Variant or BuiltInType.DataValue) return null; // unresolved / wildcard + // SAFETY BOUNDARY: only reject against the CLOSED set of built-in types a writable equipment node can + // actually carry (per ResolveBuiltInDataType). Any other expected type (Enumeration, Guid, NodeId, + // StatusCode, …) is DEFERRED to the SDK's authoritative coercion so this fail-fast can NEVER false-reject + // a write the SDK would have accepted (e.g. an Int32 payload to an Enumeration node coerces fine). + if (!IsCheckableExpectedType(expected)) return null; + // TypeInfo.Construct(object) classifies the runtime payload; an unclassifiable value ⇒ Unknown (Null). var actual = TypeInfo.Construct(value).BuiltInType; if (actual == BuiltInType.Null) return null; // couldn't classify the payload ⇒ defer to the SDK @@ -869,6 +875,13 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 BuiltInType.Int32 or BuiltInType.UInt32 or BuiltInType.Int64 or BuiltInType.UInt64 or BuiltInType.Float or BuiltInType.Double; + /// The CLOSED set of expected built-in DataTypes the structural fail-fast is allowed to reject + /// against — exactly the types can emit for a writable equipment node + /// (the numeric families + Boolean / String / DateTime / ByteString). Any expected type OUTSIDE this set is + /// deferred to the SDK so a coercible write (Int32→Enumeration, etc.) is never false-rejected. + private static bool IsCheckableExpectedType(BuiltInType t) => + IsNumeric(t) || t is BuiltInType.Boolean or BuiltInType.String or BuiltInType.DateTime or BuiltInType.ByteString; + /// /// Pure decision for the write-outcome self-correction: revert the node to its pre-write value ONLY on /// a FAILED outcome AND only while the node still holds the optimistic value. The @@ -951,7 +964,20 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 variable.ClearChangeMasks(SystemContext, includeChildren: false); // notify — restore prior // Item C: build the audit event-state while we hold the node reference + Lock, but DON'T report yet. - auditEvent = BuildWriteFailureAuditEvent(variable, outcome, optimisticValue, priorValue, clientUserId); + // Guarded like ReportAuditEvent: the revert above has ALREADY happened, so a surprise from the SDK + // child-population path (e.g. a SetChildValue on a null OldValue/NewValue) must be swallowed + logged, + // never thrown out of this fire-and-forget continuation. + try + { + auditEvent = BuildWriteFailureAuditEvent(variable, outcome, optimisticValue, priorValue, clientUserId); + } + catch (Exception ex) + { +#pragma warning disable CS0618 // Utils.LogError is [Obsolete] in favour of an ITelemetryContext this manager doesn't carry. + Utils.LogError(ex, "OtOpcUaNodeManager: failed to build AuditWriteUpdateEvent for {0}", nodeId); +#pragma warning restore CS0618 + auditEvent = null; + } } // Report OUTSIDE Lock — keeps the hold short and sidesteps any re-entrancy through the server event path. diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/EquipmentWriteGateTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/EquipmentWriteGateTests.cs index b29d97a6..c6afe7ed 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/EquipmentWriteGateTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/EquipmentWriteGateTests.cs @@ -169,6 +169,26 @@ public sealed class EquipmentWriteGateTests OtOpcUaNodeManager.EvaluateEquipmentWriteStructure(value: 1, folder).ShouldBeNull(); } + /// (A7) Closed-set safety boundary: an Enumeration-typed node with an Int32 payload DEFERS + /// (returns null) rather than false-rejecting — an enum write normally carries an Int32 the SDK coerces. + /// Before the closed-set gate this returned BadTypeMismatch — a false rejection of a valid write. + [Fact] + public void Structure_enumeration_datatype_defers_to_sdk() + { + var node = ValueNode(DataTypeIds.Enumeration); + OtOpcUaNodeManager.EvaluateEquipmentWriteStructure(value: 2, node).ShouldBeNull(); + } + + /// (A8) Closed-set safety boundary: any expected built-in type outside the materialiser's emitted + /// set (here Guid) DEFERS — only the numeric families + Boolean/String/DateTime/ByteString are ever rejected, + /// so the fail-fast can never reject a write the SDK would coerce. + [Fact] + public void Structure_noncheckable_expected_type_defers() + { + var node = ValueNode(DataTypeIds.Guid); + OtOpcUaNodeManager.EvaluateEquipmentWriteStructure(value: "any", node).ShouldBeNull(); + } + private static BaseDataVariableState ValueNode(NodeId dataType) => new(null) { NodeId = new NodeId("eq/x", 2),