From 6eace4bbba7bbf1dbdaf0d43a0ad79f7d6654a6a Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 06:51:39 -0400 Subject: [PATCH] fix(mqtt-contracts): refuse unpublishable write destinations and advise on them at deploy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A wildcard writeTopic was published verbatim. MQTT-3.3.2-2 forbids a wildcard in a PUBLISH topic and a conformant broker answers a protocol violation by closing the connection — which for this driver is the one connection every subscription lives on, so a single write to a mis-authored tag took the whole driver dark. The Sparkplug side had the same hole plus a worse one: the binding tuple IS the NCMD/DCMD topic, so a '/' in groupId/edgeNodeId/deviceId silently addressed a different edge node than the tag names. MqttWriteEncoder now refuses both as NoWriteDestination (-> BadNotWritable); the read path is untouched. metricName is exempt on purpose — it rides in the payload and Sparkplug's own canonical names use '/'. MqttTagDefinitionFactory.Inspect gained the matching ADVISORIES (never rejections — the mappers accept these blobs, and the gate must never be stricter than the runtime). Both rules were already hard errors in the AdminUI's typed editor, but that is one authoring path of several: the raw-JSON textarea, CSV import and the API all reach the deploy gate directly, so the authoring surface was stricter than the gate — the drift class the #549-#559 arc exists to close. Shared shape rules single-sourced in the new internal MqttAddressRules. Driver.Mqtt.Tests 724 (was 708); AdminUI.Tests 1466; ControlPlane.Tests 261. --- .../MqttAddressRules.cs | 63 +++++++++++++++++++ .../MqttTagDefinitionFactory.cs | 48 +++++++++++++- .../MqttWriteEncoder.cs | 31 ++++++++- .../MqttInspectAcceptGateTests.cs | 48 ++++++++++++++ .../MqttWriteEncoderTests.cs | 52 +++++++++++++++ 5 files changed, 236 insertions(+), 6 deletions(-) create mode 100644 src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttAddressRules.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttAddressRules.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttAddressRules.cs new file mode 100644 index 00000000..205913f8 --- /dev/null +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttAddressRules.cs @@ -0,0 +1,63 @@ +namespace ZB.MOM.WW.OtOpcUa.Driver.Mqtt; + +/// +/// The two shape rules an authored MQTT address must satisfy to be publishable, shared by +/// the deploy-time inspector () and the write encoder +/// () so the warning an operator is shown and the refusal the driver +/// makes can never disagree. +/// +/// +/// +/// Neither rule gates the READ path, and that asymmetry is the point. A tag whose +/// subscription topic carries a wildcard still reads (ambiguously — hence +/// 's long-standing advisory), and a Sparkplug tag +/// with a malformed id still parses. What these rules decide is whether an outbound +/// message could legally be addressed at all, so they belong to the write encoder and to the +/// inspector, never to the mappers' accept gate — which must stay exactly as strict as the +/// runtime reader (Gitea #551). +/// +/// +/// Why a wildcard write destination is a refusal rather than a best effort. MQTT-3.3.2-2 +/// forbids a wildcard in a PUBLISH topic, and a conformant broker answers a protocol violation by +/// closing the network connection — which for this driver is the one connection every +/// subscription lives on. Publishing anyway would turn one mis-authored tag into a whole-driver +/// outage; refusing costs that tag its write and nothing else. +/// +/// +internal static class MqttAddressRules +{ + /// The MQTT wildcard characters — illegal in any concrete (published or subscribed) topic. + private static readonly char[] TopicWildcards = ['+', '#']; + + /// + /// Characters that cannot appear in a single MQTT topic segment, which is what a Sparkplug + /// group / edge-node / device id is. / joins the wildcards here because it would silently + /// re-shape the command topic into a different address rather than making it unpublishable — and + /// because a decoded incoming id can never contain one, so an authored id that does is a binding + /// that could never have resolved. + /// + private static readonly char[] SegmentIllegalChars = ['/', '+', '#']; + + /// + /// Whether carries an MQTT wildcard (+ / #). + /// + /// A topic string; a /empty one carries none. + /// when a wildcard is present. + internal static bool HasWildcard(string? topic) => + !string.IsNullOrEmpty(topic) && topic.IndexOfAny(TopicWildcards) >= 0; + + /// + /// Whether is unusable as a single MQTT topic segment — i.e. as a Sparkplug + /// group, edge-node or device id. + /// + /// The authored id; a /empty one is not judged here (absence + /// is handled by the mappers' own presence checks, and deviceId is legitimately absent). + /// when the id contains /, + or #. + /// + /// Deliberately NOT applied to metricName: a metric name rides in the Sparkplug + /// payload, not in the topic, and Sparkplug's own canonical names use / (e.g. + /// Node Control/Rebirth). Applying it there would refuse every conformant node-control tag. + /// + internal static bool IsIllegalTopicSegment(string? id) => + !string.IsNullOrEmpty(id) && id.IndexOfAny(SegmentIllegalChars) >= 0; +} diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttTagDefinitionFactory.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttTagDefinitionFactory.cs index f3c725a9..241dc8e6 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttTagDefinitionFactory.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttTagDefinitionFactory.cs @@ -47,6 +47,14 @@ public static class MqttTagDefinitionFactory /// The MQTT wildcard characters — illegal in a tag's (concrete) subscription topic. private static readonly char[] TopicWildcards = ['+', '#']; + /// + /// The Sparkplug address keys that are single MQTT topic segments, and so cannot carry /, + /// + or #. is deliberately absent — it + /// rides in the payload, not the topic. See . + /// + private static readonly string[] SparkplugSegmentKeys = + [MqttTagConfigKeys.GroupId, MqttTagConfigKeys.EdgeNodeId, MqttTagConfigKeys.DeviceId]; + /// /// Maps an authored TagConfig object to a typed definition keyed by . /// The input is always an authored TagConfig JSON object (there is no name-vs-blob heuristic in v3), @@ -265,9 +273,13 @@ public static class MqttTagDefinitionFactory /// /// /// Advisories — a wildcard tag topic (+ / #), which the mapper - /// deliberately ACCEPTS (ambiguous, not unparseable) so this runs either way; and a - /// present-but-invalid payloadFormat on an ACCEPTED blob, which can only mean the - /// Sparkplug reader took it and the Plain reader would not (Gitea #557 item 2). + /// deliberately ACCEPTS (ambiguous, not unparseable) so this runs either way; a wildcard + /// writeTopic, which reads fine but can never be published to, so every write is + /// refused BadNotWritable; a Sparkplug groupId/edgeNodeId/ + /// deviceId carrying a character no MQTT topic segment can hold, which makes the + /// tag permanently unbindable; and a present-but-invalid payloadFormat on an + /// ACCEPTED blob, which can only mean the Sparkplug reader took it and the Plain reader + /// would not (Gitea #557 item 2). /// /// /// Empty for a clean, readable blob, for a blank one, or for one that is not an equipment-tag @@ -370,6 +382,36 @@ public static class MqttTagDefinitionFactory "concrete, or the tag will be fed by every matching topic."); } + // The write destination gets the same treatment as the read topic, and for a sharper reason: a + // wildcard cannot be PUBLISHED to at all (MQTT-3.3.2-2), so the tag reads perfectly while every + // write is refused with BadNotWritable — a split nothing else surfaces. The AdminUI's typed + // editor already refuses this, but it is only ONE authoring path (the raw-JSON textarea, CSV + // import and the API all reach the deploy gate directly), so leaving it out here made the + // authoring surface stricter than the deploy gate — the drift this inspector exists to close. + var writeTopic = ReadString(root, MqttTagConfigKeys.WriteTopic); + if (MqttAddressRules.HasWildcard(writeTopic)) + { + advisories.Add( + $"value '{writeTopic}' for 'writeTopic' contains an MQTT wildcard (+ or #); a wildcard " + + "cannot be published to, so the tag reads normally but every write is refused " + + "(BadNotWritable)."); + } + + // Same shape, Sparkplug side: a group/edge-node/device id is a single MQTT topic segment, so a + // decoded incoming id can never contain '/', '+' or '#'. An authored one that does is a binding + // that can never resolve — the mapper accepts it (it checks presence, not shape), so this is the + // only place it can be said. `metricName` is deliberately exempt: it rides in the payload, and + // Sparkplug's own canonical names use '/'. + foreach (var key in SparkplugSegmentKeys) + { + var value = ReadString(root, key); + if (!MqttAddressRules.IsIllegalTopicSegment(value)) continue; + advisories.Add( + $"value '{value}' for '{key}' contains a character ('/', '+' or '#') that cannot appear " + + "in an MQTT topic segment; a decoded Sparkplug id never contains one, so this tag can " + + "never bind and will read BadNodeIdUnknown in Sparkplug B mode."); + } + // Gitea #557 item 2 — the ONE key whose verdict depends on the DRIVER'S mode, reported as an // advisory because no tag-level gate can see that mode. A present-but-invalid payloadFormat is // fatal to the Plain parse and invisible to the Sparkplug one, so a union accept here can only diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttWriteEncoder.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttWriteEncoder.cs index 45ee2981..649d49f0 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttWriteEncoder.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttWriteEncoder.cs @@ -19,9 +19,11 @@ public enum MqttWriteEncodeError None = 0, /// - /// The tag declares no write destination: a Plain tag with no writeTopic. Deliberately a - /// hard refusal — see 's remarks on why a silent no-op would be - /// worse than the honest read-only behaviour this replaced. + /// The tag declares no usable write destination: a Plain tag with no writeTopic or + /// one carrying an MQTT wildcard, or a Sparkplug tag whose binding tuple is incomplete or whose + /// group/edge-node/device id is not a legal topic segment (see ). + /// Deliberately a hard refusal — see 's remarks on why a silent + /// no-op would be worse than the honest read-only behaviour this replaced. /// NoWriteDestination, @@ -121,6 +123,17 @@ public static class MqttWriteEncoder return MqttWriteEncodeError.NoWriteDestination; } + // A wildcard topic is not a destination — MQTT-3.3.2-2 forbids one in a PUBLISH, and a + // conformant broker answers a protocol violation by CLOSING the connection, which is the same + // connection every one of this driver's subscriptions lives on. So publishing anyway would turn + // one mis-authored tag into a whole-driver outage. `NoWriteDestination` (→ BadNotWritable) is the + // right verdict rather than a new error: the tag genuinely has nowhere a write could be sent, and + // the read path is untouched. See MqttAddressRules. + if (MqttAddressRules.HasWildcard(def.WriteTopic)) + { + return MqttWriteEncodeError.NoWriteDestination; + } + if (value is null) { // No MQTT payload means "the value is null" — a zero-length payload is a legitimate @@ -175,6 +188,18 @@ public static class MqttWriteEncoder return MqttWriteEncodeError.NoWriteDestination; } + // …and an id that is not a legal topic SEGMENT is the same failure by a different route: the + // tuple is assembled into the NCMD/DCMD topic, so a '/' would silently address a different node + // than the tag names while '+'/'#' would make the PUBLISH a protocol violation (see the Plain + // branch). `metricName` is exempt on purpose — it rides in the payload, and Sparkplug's own + // canonical names use '/'. + if (MqttAddressRules.IsIllegalTopicSegment(groupId) + || MqttAddressRules.IsIllegalTopicSegment(edgeNodeId) + || MqttAddressRules.IsIllegalTopicSegment(def.DeviceId)) + { + return MqttWriteEncodeError.NoWriteDestination; + } + if (value is null) { return MqttWriteEncodeError.ValueNotRepresentable; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttInspectAcceptGateTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttInspectAcceptGateTests.cs index 498648f0..36a87ca9 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttInspectAcceptGateTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttInspectAcceptGateTests.cs @@ -252,6 +252,54 @@ public sealed class MqttInspectAcceptGateTests warnings.ShouldNotContain(w => w.Contains("Plain mode"), "an unserved tag is not a mode-dependent case"); } + // ---- The two authoring-only rules the deploy gate used to pass over in silence ---- + + /// + /// MqttTagConfigModel.ValidatePlain refuses a wildcard writeTopic outright, but the + /// typed editor is only ONE authoring path — the raw-JSON textarea, CSV import and the API all + /// reach the deploy gate directly. A wildcard cannot be published to at all (MQTT-3.3.2-2), so the + /// tag reads perfectly while every write is refused; that has to be said somewhere the operator + /// sees it, and an ADVISORY is the right class because the blob IS accepted. + /// + [Fact] + public void A_wildcard_write_topic_is_advised_on_an_accepted_blob() + { + const string Blob = """{"topic":"plant/line1/temp","writeTopic":"plant/+/set"}"""; + + EitherModeAccepts(Blob).ShouldBeTrue("the tag still READS fine — only the write is unaddressable"); + MqttTagDefinitionFactory.Inspect(Blob) + .ShouldContain(w => w.Contains("writeTopic") && w.Contains("wildcard")); + } + + /// + /// A Sparkplug group / edge-node / device id is a single MQTT topic segment, so a decoded incoming + /// id can never contain /, + or # — an authored one that does is a binding + /// that can never resolve, and the mapper accepts it (it validates presence, not shape). The + /// AdminUI model calls this out in words and refuses the save; the deploy gate said nothing. + /// + /// The key the advisory must name. + /// The tag config carrying an unbindable id. + [Theory] + [InlineData("groupId", """{"groupId":"Ot/Sim","edgeNodeId":"EdgeA","metricName":"T"}""")] + [InlineData("edgeNodeId", """{"groupId":"OtSim","edgeNodeId":"Edge+A","metricName":"T"}""")] + [InlineData("deviceId", """{"groupId":"OtSim","edgeNodeId":"EdgeA","deviceId":"F#1","metricName":"T"}""")] + public void A_sparkplug_id_that_is_not_a_topic_segment_is_advised(string key, string blob) + { + EitherModeAccepts(blob).ShouldBeTrue("the reader accepts it — which is exactly why it must be advised"); + MqttTagDefinitionFactory.Inspect(blob).ShouldContain(w => w.Contains(key)); + } + + /// + /// The falsification control for the rule above: metricName is NOT a topic segment — it + /// rides in the payload — and Sparkplug's own canonical names use /. Advising it would + /// report every conformant Node Control/Rebirth tag as broken. + /// + [Fact] + public void A_metric_name_containing_a_slash_is_never_advised() + => MqttTagDefinitionFactory + .Inspect("""{"groupId":"OtSim","edgeNodeId":"EdgeA","metricName":"Node Control/Rebirth"}""") + .ShouldBeEmpty(); + /// /// Both gates must keep agreeing on the VERDICT (Gitea #557 item 2). The #554 authoring consult in /// MqttTagConfigModel asks the identical union and only words a REFUSAL, so an advisory can diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttWriteEncoderTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttWriteEncoderTests.cs index 1629f299..d6ce97e9 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttWriteEncoderTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttWriteEncoderTests.cs @@ -237,6 +237,58 @@ public sealed class MqttWriteEncoderTests EncodeTopic(MqttMode.SparkplugB, both).ShouldBe("spBv1.0/OtOpcUaSim/DCMD/EdgeA/Filler1"); } + // ---- A destination that cannot be published to is not a destination ------------------------- + + [Theory] + [InlineData("factory/+/set")] + [InlineData("factory/oven/#")] + public void A_wildcard_write_topic_is_refused_rather_than_published(string writeTopic) + { + // MQTT-3.3.2-2: a PUBLISH topic MUST NOT contain a wildcard, and a conformant broker answers a + // protocol violation by CLOSING the connection — which, for this driver, is the same connection + // every subscription lives on. So one write to a mis-authored tag takes the WHOLE driver dark, + // not just that tag. The typed AdminUI editor already refuses this, but a raw-JSON / CSV / + // API-authored blob reaches the encoder untouched, so the refusal has to live here too. + MqttWriteEncoder.TryEncode(MqttMode.Plain, Plain(writeTopic), 1, out var encoded) + .ShouldBe(MqttWriteEncodeError.NoWriteDestination); + encoded.Topic.ShouldBeNull(); + } + + [Theory] + [InlineData("Ot/Sim", "EdgeA", null)] + [InlineData("OtSim", "Edge+A", null)] + [InlineData("OtSim", "EdgeA", "Fill#er")] + [InlineData("OtSim", "EdgeA", "Fill/er")] + public void A_binding_tuple_that_is_not_a_legal_topic_segment_has_no_command_address( + string groupId, string edgeNodeId, string? deviceId) + { + // The same protocol hazard on the Sparkplug side, plus one the Plain side does not have: the + // tuple IS the command topic, so a '/' silently re-shapes it into a DIFFERENT address (an + // NCMD for group "Ot" node "Sim" rather than the tag's own node) while '+'/'#' make it + // unpublishable. Refusing is the only honest verdict for all three. + var def = Sparkplug(deviceId) with { GroupId = groupId, EdgeNodeId = edgeNodeId }; + + MqttWriteEncoder.TryEncode(MqttMode.SparkplugB, def, 1.0, out _) + .ShouldBe(MqttWriteEncodeError.NoWriteDestination); + } + + [Fact] + public void A_metric_name_containing_a_slash_still_encodes() + { + // The falsification control for the rule above. Sparkplug's own canonical metric names use '/' + // (`Node Control/Rebirth`), and metricName rides in the PAYLOAD, never in the topic — so the + // segment rule must not reach it or every conformant node-control write would be refused. + MqttWriteEncoder.TryEncode( + MqttMode.SparkplugB, + Sparkplug(deviceId: null, DriverDataType.Boolean, metricName: "Node Control/Rebirth"), + true, + out var encoded) + .ShouldBe(MqttWriteEncodeError.None); + + encoded.Topic.ShouldBe("spBv1.0/OtOpcUaSim/NCMD/EdgeA"); + Payload.Parser.ParseFrom(encoded.Payload).Metrics[0].Name.ShouldBe("Node Control/Rebirth"); + } + private static string EncodeTopic(MqttMode mode, MqttTagDefinition def) { MqttWriteEncoder.TryEncode(mode, def, 1.0, out var encoded).ShouldBe(MqttWriteEncodeError.None);