From a5a8710af5aec3a96ccf7d40c9dd0b60c3a7a429 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 24 Jul 2026 14:01:01 -0400 Subject: [PATCH] fix(mqtt): pin security-critical defaults with tests + redact password in ToString MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code review of f22db5d8 (approved-with-findings): the existing round-trip tests only exercised explicit JSON payloads, so nothing failed if UseTls / AllowUntrustedServerCertificate or the §5.1 numeric defaults were flipped. Adds a defaults-pinning test plus a partial-JSON test that omits the TLS knobs entirely (the production case — an operator config that just doesn't mention TLS must not silently land insecure). Also overrides the record's PrintMembers so ToString() no longer prints Password in plaintext (verified RED before the fix: the new test failed with the raw password in the rendered string). OpcUaClientDriverOptions has the same unredacted-ToString() shape but is out of scope for this task per the coordinator's note; flagging as a possible follow-up. Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW --- .../MqttDriverOptions.cs | 37 +++++++++++++++++++ .../MqttDriverOptionsTests.cs | 34 +++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttDriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttDriverOptions.cs index 4fe77477..47e93a45 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttDriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttDriverOptions.cs @@ -1,4 +1,5 @@ using System.ComponentModel.DataAnnotations; +using System.Text; namespace ZB.MOM.WW.OtOpcUa.Driver.Mqtt; @@ -95,6 +96,42 @@ public sealed record MqttDriverOptions /// ; null in Sparkplug B mode. /// public MqttPlainOptions? Plain { get; init; } + + /// + /// Record-generated ToString() member printer, overridden so + /// never renders in plaintext — this DTO routinely lands in logs / exception messages / + /// debugger watches, and the plan's cross-cutting rule is explicit: never commit or log + /// creds. A set password prints as ***; unset (null/empty) renders + /// distinguishably so the redaction can't be mistaken for a real secret. + /// + private bool PrintMembers(StringBuilder builder) + { + var passwordDisplay = Password switch + { + null => "", + "" => "", + _ => "***", + }; + + builder.Append("Host = ").Append(Host); + builder.Append(", Port = ").Append(Port); + builder.Append(", ClientId = ").Append(ClientId); + builder.Append(", UseTls = ").Append(UseTls); + builder.Append(", AllowUntrustedServerCertificate = ").Append(AllowUntrustedServerCertificate); + builder.Append(", CaCertificatePath = ").Append(CaCertificatePath); + builder.Append(", Username = ").Append(Username); + builder.Append(", Password = ").Append(passwordDisplay); + builder.Append(", ProtocolVersion = ").Append(ProtocolVersion); + builder.Append(", CleanSession = ").Append(CleanSession); + builder.Append(", KeepAliveSeconds = ").Append(KeepAliveSeconds); + builder.Append(", ConnectTimeoutSeconds = ").Append(ConnectTimeoutSeconds); + builder.Append(", ReconnectMinBackoffSeconds = ").Append(ReconnectMinBackoffSeconds); + builder.Append(", ReconnectMaxBackoffSeconds = ").Append(ReconnectMaxBackoffSeconds); + builder.Append(", Mode = ").Append(Mode); + builder.Append(", Sparkplug = ").Append(Sparkplug); + builder.Append(", Plain = ").Append(Plain); + return true; + } } /// diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttDriverOptionsTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttDriverOptionsTests.cs index 29b6317f..ce7e2c3c 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttDriverOptionsTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttDriverOptionsTests.cs @@ -36,4 +36,38 @@ public sealed class MqttDriverOptionsTests s.ShouldContain("\"Plain\""); s.ShouldNotContain("\"mode\":0"); } + + // Pins the plan's cross-cutting rule ("never ship anonymous/public-broker defaults") as an + // executable assertion — nothing else in this suite fails if UseTls / AllowUntrustedServerCertificate + // (or the §5.1 numeric defaults) get flipped by an unrelated edit. + [Fact] + public void Defaults_SecurityCriticalKnobs_MatchPlan() + { + var o = new MqttDriverOptions(); + o.UseTls.ShouldBeTrue(); + o.AllowUntrustedServerCertificate.ShouldBeFalse(); + o.ConnectTimeoutSeconds.ShouldBe(15); + o.ReconnectMinBackoffSeconds.ShouldBe(1); + o.ReconnectMaxBackoffSeconds.ShouldBe(30); + } + + // The production case: an operator-authored config blob that simply doesn't mention TLS must + // never silently deserialize into an insecure instance. + [Fact] + public void Deserialize_PartialJsonOmittingTlsKnobs_KeepsSecureDefaults() + { + const string json = """{ "host":"10.100.0.35","port":8883 }"""; + var o = JsonSerializer.Deserialize(json, J)!; + o.UseTls.ShouldBeTrue(); + o.AllowUntrustedServerCertificate.ShouldBeFalse(); + } + + [Fact] + public void ToString_DoesNotLeakPassword() + { + var o = new MqttDriverOptions { Password = "hunter2-supersecret" }; + var s = o.ToString(); + s.ShouldNotContain("hunter2-supersecret"); + s.ShouldContain("***"); + } }