From 3118213772ed93f0e1fc05e39cfc0aa16a0dc6cb Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 06:51:59 -0400 Subject: [PATCH] fix(mqtt-browser): reject a wildcard rebirth scope before any NCMD leaves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SparkplugRebirthScope (the AdminUI's typed input) rejects '+'/'#' and its own remarks say it is 'advisory, not the gate' because MqttBrowseSession .ResolveRebirthTargets 'refuses an unusable one BEFORE any message leaves'. That claim was false: the bare {group}/{edgeNode} branch — honoured on purpose, since a node that has not birthed is the prime rebirth target — gated on IsUsableLabel, which checks length and control characters only. A scope of 'Plant1/#' therefore published spBv1.0/Plant1/NCMD/# , a PUBLISH the MQTT spec forbids and a conformant broker answers by closing the browse connection; '+' would have fanned one operator click across every matching edge node. The check is a separate IsUsableRebirthSegment rather than a tightening of IsUsableLabel: that helper also judges Sparkplug METRIC names, which are arbitrary publisher-chosen UTF-8 and never part of a topic, so tightening it there would drop legitimate metrics from the tree. Cases 1 and 2 cannot reach this shape — their ids come off SparkplugTopic.TryParse, which already rejects both characters. Driver.Mqtt.Tests 724 (was 708). --- .../MqttBrowseSession.cs | 22 ++++++++++++++++++- .../MqttBrowseSessionTests.cs | 21 ++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs index 2e37d579..1c563e83 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs @@ -720,6 +720,19 @@ internal sealed class MqttBrowseSession : IRebirthCapableBrowseSession return true; } + /// + /// Whether an operator-supplied scope segment may be used to address an NCMD. Everything + /// requires, plus no MQTT wildcard: a group/edge-node id becomes a + /// topic segment of the published command, and MQTT-3.3.2-2 forbids a wildcard in a PUBLISH + /// topic. Kept separate from on purpose — that one also judges + /// Sparkplug metric names, which are arbitrary publisher-chosen UTF-8 and are never part + /// of a topic, so tightening it there would drop legitimate metrics from the tree. + /// + /// The candidate scope segment. + /// when an NCMD may be addressed with it. + private static bool IsUsableRebirthSegment(string segment) => + IsUsableLabel(segment) && segment.IndexOf('+') < 0 && segment.IndexOf('#') < 0; + /// Test seam over that takes the payload as text. /// The observed topic. /// The message body as text, or for an empty payload. @@ -895,7 +908,14 @@ internal sealed class MqttBrowseSession : IRebirthCapableBrowseSession } // 3. A bare {group}/{edgeNode} the window has never seen — the prime rebirth target. - if (segments.Length == 2 && IsUsableLabel(segments[0]) && IsUsableLabel(segments[1])) + // ⚠ This is the ONE path that takes an id the session never parsed off the wire, so it is the + // one that must check for a wildcard. IsUsableLabel deliberately permits '+'/'#' (a Sparkplug + // METRIC name is arbitrary UTF-8 and could legitimately contain either), so it is not enough + // here: `Plant1/#` would have assembled `spBv1.0/Plant1/NCMD/#`, a PUBLISH the MQTT spec + // forbids, which a conformant broker answers by closing the browse connection — while `+` + // would fan one operator click across every matching edge node. Cases 1 and 2 above cannot + // reach this shape: their ids come off SparkplugTopic.TryParse, which rejects both characters. + if (segments.Length == 2 && IsUsableRebirthSegment(segments[0]) && IsUsableRebirthSegment(segments[1])) { return [(segments[0], segments[1])]; } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttBrowseSessionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttBrowseSessionTests.cs index 5427e985..b84fdb7d 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttBrowseSessionTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttBrowseSessionTests.cs @@ -854,6 +854,27 @@ public sealed class MqttBrowseSessionTests s.PublishCountForTest.ShouldBe(0); } + [Theory] + [InlineData("Plant1/#")] + [InlineData("Plant1/+")] + [InlineData("+/EdgeA")] + [InlineData("#/EdgeA")] + public async Task RequestRebirthAsync_WildcardScope_PublishesNothing(string scope) + { + // SparkplugRebirthScope (the AdminUI's typed input) rejects a wildcard and its own remarks say + // it is "advisory, not the gate" because ResolveRebirthTargets "refuses an unusable one BEFORE + // any message leaves". That claim has to be true here: an unobserved `{group}/{edgeNode}` scope + // is honoured on purpose (a node that has not birthed is the prime rebirth target), and the + // label check behind it looked only at length and control characters. A wildcard segment would + // therefore have published `spBv1.0/Plant1/NCMD/#` — a PUBLISH the MQTT spec forbids, which a + // conformant broker answers by closing the browse connection. + await using var s = new MqttBrowseSession(MqttMode.SparkplugB); + + await Should.ThrowAsync( + async () => await s.RequestRebirthAsync(scope, TestContext.Current.CancellationToken)); + s.PublishCountForTest.ShouldBe(0); + } + [Fact] public async Task RequestRebirthAsync_InPlainMode_IsRefused() {