Joseph Doherty
4ec01bdfc1
Three review findings, all "unbounded work/memory from untrusted broker input" rather than correctness bugs - but this points at a live plant broker. 1. InferPayload decoded the WHOLE payload on MQTTnet's dispatcher thread before trimming to 64 chars. The node cap never engages here: a multi-MB blob on an already-known topic creates no node, it just updates one, so the cost repeats per message forever. Now at most PayloadInspectMaxBytes (512) are examined. A blind slice can cut a multi-byte sequence, which the strict decoder would report as "binary" - TrimToUtf8Boundary backs the window off to the last whole sequence so good UTF-8 is never mislabelled. A clipped payload is String by construction rather than inferred from a partial view. 2. The node cap bounded COUNT, not bytes. MQTT topics run to ~65KB, so 50k nodes near that ceiling is a multi-GB tree. Topics now bounded at 1024 chars and segments at 256, rejected whole (a truncated topic is a DIFFERENT topic the picker would commit as a tag address) and surfaced via an __oversized__ marker kept distinct from __truncated__ - the operator's remedy differs. 3. Control characters in a snippet would break the picker's single-line row; Trim() only strips the ends. Now scrubbed to spaces. Each guard was falsified independently by neutering it and confirming exactly one test reddens. That found a real gap: the oversized-topic test was passing via the SEGMENT bound, leaving the whole-topic bound untested - the test now uses many short segments so only the topic bound can reject it. Also records the .Driver project reference as a known, deliberate exception to the .Browser -> .Contracts pattern, with its cost (AdminUI gains Core + Polly + Serilog) and the clean fix (a leaf transport project; NOT a move into .Contracts, which is deliberately transport-free). Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
57 lines
3.5 KiB
XML
57 lines
3.5 KiB
XML
<Project Sdk="Microsoft.NET.Sdk">
|
|
|
|
<PropertyGroup>
|
|
<TargetFramework>net10.0</TargetFramework>
|
|
<Nullable>enable</Nullable>
|
|
<ImplicitUsings>enable</ImplicitUsings>
|
|
<LangVersion>latest</LangVersion>
|
|
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
|
|
<RootNamespace>ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser</RootNamespace>
|
|
<AssemblyName>ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser</AssemblyName>
|
|
</PropertyGroup>
|
|
|
|
<ItemGroup>
|
|
<ProjectReference Include="..\..\Core\ZB.MOM.WW.OtOpcUa.Commons\ZB.MOM.WW.OtOpcUa.Commons.csproj"/>
|
|
<ProjectReference Include="..\ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts\ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts.csproj"/>
|
|
<!--
|
|
┌──────────────────────────────────────────────────────────────────────────────────┐
|
|
│ KNOWN, DELIBERATE EXCEPTION to the .Browser → .Contracts pattern. │
|
|
│ DO NOT COPY THIS LINE INTO A NEW DRIVER'S BROWSER WITHOUT READING THIS. │
|
|
└──────────────────────────────────────────────────────────────────────────────────┘
|
|
|
|
Every other bespoke browser in this repo (OpcUaClient, Galaxy) references only its own
|
|
.Contracts project plus its transport package. This one also references the runtime
|
|
.Driver project. That is a real widening, not a free one, and it was accepted knowingly:
|
|
|
|
WHY: the browse CONNECT reuses MqttConnection.BuildClientOptions, which owns the TLS
|
|
posture, the PEM CA-pin chain validator, the credential handling and the protocol
|
|
version mapping (~100 lines). A second copy here would be a silent security
|
|
divergence — browse quietly not honouring a pinned CA while the runtime driver does.
|
|
The method is pure (no I/O, no network), so the dependency costs nothing at run time.
|
|
|
|
WHAT IT COSTS: the AdminUI's project graph does NOT otherwise include Core.csproj
|
|
(only Host.csproj and the runtime Driver.* projects do). Referencing this browser
|
|
from the AdminUI therefore pulls in Core + Polly.Core + Serilog at build/deploy time.
|
|
|
|
THE CLEAN FIX (deferred, not blocked): a small leaf project — say
|
|
ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Transport (net10, refs .Contracts + MQTTnet) — holding
|
|
BuildClientOptions/ConfigureTls/the CA-pin helpers, referenced by BOTH .Driver and
|
|
.Browser. Note the obvious-looking alternative does NOT work: those members cannot
|
|
move into .Contracts, because .Contracts is deliberately transport-free (Task 1
|
|
removed its MQTTnet reference to keep it so) and BuildClientOptions returns
|
|
MqttClientOptions, an MQTTnet type.
|
|
-->
|
|
<ProjectReference Include="..\ZB.MOM.WW.OtOpcUa.Driver.Mqtt\ZB.MOM.WW.OtOpcUa.Driver.Mqtt.csproj"/>
|
|
</ItemGroup>
|
|
|
|
<ItemGroup>
|
|
<PackageReference Include="MQTTnet"/>
|
|
<PackageReference Include="Microsoft.Extensions.Logging.Abstractions"/>
|
|
</ItemGroup>
|
|
|
|
<ItemGroup>
|
|
<InternalsVisibleTo Include="ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests"/>
|
|
</ItemGroup>
|
|
|
|
</Project>
|