fix(mqtt-browser): reject a wildcard rebirth scope before any NCMD leaves

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).
This commit is contained in:
Joseph Doherty
2026-08-10 06:51:59 -04:00
parent 6eace4bbba
commit 3118213772
2 changed files with 42 additions and 1 deletions
@@ -720,6 +720,19 @@ internal sealed class MqttBrowseSession : IRebirthCapableBrowseSession
return true;
}
/// <summary>
/// Whether an operator-supplied scope segment may be used to <b>address</b> an NCMD. Everything
/// <see cref="IsUsableLabel"/> 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 <see cref="IsUsableLabel"/> on purpose — that one also judges
/// Sparkplug <i>metric</i> 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.
/// </summary>
/// <param name="segment">The candidate scope segment.</param>
/// <returns><see langword="true"/> when an NCMD may be addressed with it.</returns>
private static bool IsUsableRebirthSegment(string segment) =>
IsUsableLabel(segment) && segment.IndexOf('+') < 0 && segment.IndexOf('#') < 0;
/// <summary>Test seam over <see cref="Observe"/> that takes the payload as text.</summary>
/// <param name="topic">The observed topic.</param>
/// <param name="payload">The message body as text, or <see langword="null"/> for an empty payload.</param>
@@ -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])];
}
@@ -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<ArgumentException>(
async () => await s.RequestRebirthAsync(scope, TestContext.Current.CancellationToken));
s.PublishCountForTest.ShouldBe(0);
}
[Fact]
public async Task RequestRebirthAsync_InPlainMode_IsRefused()
{