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
@@ -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