fix(mqtt-contracts): refuse unpublishable write destinations and advise on them at deploy
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.
This commit is contained in:
Joseph Doherty
@@ -0,0 +1,63 @@
|
||||
namespace ZB.MOM.WW.OtOpcUa.Driver.Mqtt;
|
||||
|
||||
/// <summary>
|
||||
/// The two shape rules an authored MQTT address must satisfy to be <em>publishable</em>, shared by
|
||||
/// the deploy-time inspector (<see cref="MqttTagDefinitionFactory.Inspect"/>) and the write encoder
|
||||
/// (<see cref="MqttWriteEncoder"/>) so the warning an operator is shown and the refusal the driver
|
||||
/// makes can never disagree.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// <b>Neither rule gates the READ path, and that asymmetry is the point.</b> A tag whose
|
||||
/// subscription <c>topic</c> carries a wildcard still reads (ambiguously — hence
|
||||
/// <see cref="MqttTagDefinitionFactory.Inspect"/>'s long-standing advisory), and a Sparkplug tag
|
||||
/// with a malformed id still parses. What these rules decide is whether an <em>outbound</em>
|
||||
/// 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).
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>Why a wildcard write destination is a refusal rather than a best effort.</b> 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.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
internal static class MqttAddressRules
|
||||
{
|
||||
/// <summary>The MQTT wildcard characters — illegal in any concrete (published or subscribed) topic.</summary>
|
||||
private static readonly char[] TopicWildcards = ['+', '#'];
|
||||
|
||||
/// <summary>
|
||||
/// Characters that cannot appear in a <b>single MQTT topic segment</b>, which is what a Sparkplug
|
||||
/// group / edge-node / device id is. <c>/</c> 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.
|
||||
/// </summary>
|
||||
private static readonly char[] SegmentIllegalChars = ['/', '+', '#'];
|
||||
|
||||
/// <summary>
|
||||
/// Whether <paramref name="topic"/> carries an MQTT wildcard (<c>+</c> / <c>#</c>).
|
||||
/// </summary>
|
||||
/// <param name="topic">A topic string; a <see langword="null"/>/empty one carries none.</param>
|
||||
/// <returns><see langword="true"/> when a wildcard is present.</returns>
|
||||
internal static bool HasWildcard(string? topic) =>
|
||||
!string.IsNullOrEmpty(topic) && topic.IndexOfAny(TopicWildcards) >= 0;
|
||||
|
||||
/// <summary>
|
||||
/// Whether <paramref name="id"/> is unusable as a single MQTT topic segment — i.e. as a Sparkplug
|
||||
/// group, edge-node or device id.
|
||||
/// </summary>
|
||||
/// <param name="id">The authored id; a <see langword="null"/>/empty one is not judged here (absence
|
||||
/// is handled by the mappers' own presence checks, and <c>deviceId</c> is legitimately absent).</param>
|
||||
/// <returns><see langword="true"/> when the id contains <c>/</c>, <c>+</c> or <c>#</c>.</returns>
|
||||
/// <remarks>
|
||||
/// Deliberately NOT applied to <c>metricName</c>: a metric name rides in the Sparkplug
|
||||
/// <em>payload</em>, not in the topic, and Sparkplug's own canonical names use <c>/</c> (e.g.
|
||||
/// <c>Node Control/Rebirth</c>). Applying it there would refuse every conformant node-control tag.
|
||||
/// </remarks>
|
||||
internal static bool IsIllegalTopicSegment(string? id) =>
|
||||
!string.IsNullOrEmpty(id) && id.IndexOfAny(SegmentIllegalChars) >= 0;
|
||||
}
|
||||
@@ -47,6 +47,14 @@ public static class MqttTagDefinitionFactory
|
||||
/// <summary>The MQTT wildcard characters — illegal in a <em>tag's</em> (concrete) subscription topic.</summary>
|
||||
private static readonly char[] TopicWildcards = ['+', '#'];
|
||||
|
||||
/// <summary>
|
||||
/// The Sparkplug address keys that are single MQTT topic segments, and so cannot carry <c>/</c>,
|
||||
/// <c>+</c> or <c>#</c>. <see cref="MqttTagConfigKeys.MetricName"/> is deliberately absent — it
|
||||
/// rides in the payload, not the topic. See <see cref="MqttAddressRules.IsIllegalTopicSegment"/>.
|
||||
/// </summary>
|
||||
private static readonly string[] SparkplugSegmentKeys =
|
||||
[MqttTagConfigKeys.GroupId, MqttTagConfigKeys.EdgeNodeId, MqttTagConfigKeys.DeviceId];
|
||||
|
||||
/// <summary>
|
||||
/// Maps an authored <c>TagConfig</c> object to a typed definition keyed by <paramref name="rawPath"/>.
|
||||
/// 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
|
||||
/// </item>
|
||||
/// <item>
|
||||
/// <b>Advisories</b> — a <b>wildcard</b> tag topic (<c>+</c> / <c>#</c>), which the mapper
|
||||
/// deliberately ACCEPTS (ambiguous, not unparseable) so this runs either way; and a
|
||||
/// present-but-invalid <c>payloadFormat</c> 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
|
||||
/// <c>writeTopic</c>, which reads fine but can never be published to, so every write is
|
||||
/// refused <c>BadNotWritable</c>; a Sparkplug <c>groupId</c>/<c>edgeNodeId</c>/
|
||||
/// <c>deviceId</c> carrying a character no MQTT topic segment can hold, which makes the
|
||||
/// tag permanently unbindable; and a present-but-invalid <c>payloadFormat</c> on an
|
||||
/// ACCEPTED blob, which can only mean the Sparkplug reader took it and the Plain reader
|
||||
/// would not (Gitea #557 item 2).
|
||||
/// </item>
|
||||
/// </list>
|
||||
/// 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
|
||||
|
||||
@@ -19,9 +19,11 @@ public enum MqttWriteEncodeError
|
||||
None = 0,
|
||||
|
||||
/// <summary>
|
||||
/// The tag declares no write destination: a Plain tag with no <c>writeTopic</c>. Deliberately a
|
||||
/// hard refusal — see <see cref="MqttWriteEncoder"/>'s remarks on why a silent no-op would be
|
||||
/// worse than the honest read-only behaviour this replaced.
|
||||
/// The tag declares no <b>usable</b> write destination: a Plain tag with no <c>writeTopic</c> 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 <see cref="MqttAddressRules"/>).
|
||||
/// Deliberately a hard refusal — see <see cref="MqttWriteEncoder"/>'s remarks on why a silent
|
||||
/// no-op would be worse than the honest read-only behaviour this replaced.
|
||||
/// </summary>
|
||||
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;
|
||||
|
||||
@@ -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 ----
|
||||
|
||||
/// <summary>
|
||||
/// <c>MqttTagConfigModel.ValidatePlain</c> refuses a wildcard <c>writeTopic</c> 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.
|
||||
/// </summary>
|
||||
[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"));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A Sparkplug group / edge-node / device id is a single MQTT topic segment, so a decoded incoming
|
||||
/// id can never contain <c>/</c>, <c>+</c> or <c>#</c> — 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.
|
||||
/// </summary>
|
||||
/// <param name="key">The key the advisory must name.</param>
|
||||
/// <param name="blob">The tag config carrying an unbindable id.</param>
|
||||
[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));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The falsification control for the rule above: <c>metricName</c> is NOT a topic segment — it
|
||||
/// rides in the payload — and Sparkplug's own canonical names use <c>/</c>. Advising it would
|
||||
/// report every conformant <c>Node Control/Rebirth</c> tag as broken.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void A_metric_name_containing_a_slash_is_never_advised()
|
||||
=> MqttTagDefinitionFactory
|
||||
.Inspect("""{"groupId":"OtSim","edgeNodeId":"EdgeA","metricName":"Node Control/Rebirth"}""")
|
||||
.ShouldBeEmpty();
|
||||
|
||||
/// <summary>
|
||||
/// Both gates must keep agreeing on the VERDICT (Gitea #557 item 2). The #554 authoring consult in
|
||||
/// <c>MqttTagConfigModel</c> asks the identical union and only words a REFUSAL, so an advisory can
|
||||
|
||||
@@ -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);
|
||||
|
||||
Reference in New Issue
Block a user