From 9354d69bf92e6e6217f2e2db93474893e876974c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 6 Nov 2026 15:45:50 -0500 Subject: [PATCH] fix(diagnostics): redact WriteSecuredBulk/WriteSecured2Bulk payloads (#193) Adds both secured bulk kinds to GatewayLogRedactor's credential-bearing set, plus a contract cross-check test that walks every MxCommand payload descriptor and fails if a secured or password-bearing command kind is not redacted. --- docs/Diagnostics.md | 4 +- .../Diagnostics/GatewayLogRedactor.cs | 3 + .../Diagnostics/GatewayLogRedactorTests.cs | 85 +++++++++++++++++++ 3 files changed, 91 insertions(+), 1 deletion(-) diff --git a/docs/Diagnostics.md b/docs/Diagnostics.md index f061e11..b3252a7 100644 --- a/docs/Diagnostics.md +++ b/docs/Diagnostics.md @@ -71,6 +71,8 @@ private static readonly HashSet SensitiveCommandMethods = new(StringComp "AuthenticateUser", "WriteSecured", "WriteSecured2", + "WriteSecuredBulk", + "WriteSecured2Bulk", "AcknowledgeAlarm", "AcknowledgeAlarmByName" }; @@ -82,7 +84,7 @@ public static bool IsCredentialBearingCommand(string? commandMethod) } ``` -The names match the MXAccess command list in `gateway.md` exactly. `Write` and `Write2` are not in the set because their payloads are tag values, not credentials, and are governed by the `valueLoggingEnabled` flag described below. `AcknowledgeAlarm` and `AcknowledgeAlarmByName` are in the set because their commands carry the operator's `operator_password`. Independently of the method set, the name-based seam redacts any property whose name contains `password`, so a credential field added to a payload later is redacted without a code change. +The names match the MXAccess command list in `gateway.md` exactly. `Write` and `Write2` are not in the set because their payloads are tag values, not credentials, and are governed by the `valueLoggingEnabled` flag described below. `WriteSecured`, `WriteSecured2`, `WriteSecuredBulk` and `WriteSecured2Bulk` are in the set because every secured write — single-item or bulk — carries a credential-sensitive value. `AcknowledgeAlarm` and `AcknowledgeAlarmByName` are in the set because their commands carry the operator's `operator_password`. `GatewayLogRedactorTests` walks the `MxCommand` payload descriptors and fails if a secured or password-bearing command kind is missing from the set. Independently of the method set, the name-based seam redacts any property whose name contains `password`, so a credential field added to a payload later is redacted without a code change. ### API key redaction diff --git a/src/ZB.MOM.WW.MxGateway.Server/Diagnostics/GatewayLogRedactor.cs b/src/ZB.MOM.WW.MxGateway.Server/Diagnostics/GatewayLogRedactor.cs index 0d2020b..1099a91 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Diagnostics/GatewayLogRedactor.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Diagnostics/GatewayLogRedactor.cs @@ -11,8 +11,11 @@ public static class GatewayLogRedactor private static readonly HashSet SensitiveCommandMethods = new(StringComparer.OrdinalIgnoreCase) { "AuthenticateUser", + // Carry a credential-sensitive write value (single item and bulk). "WriteSecured", "WriteSecured2", + "WriteSecuredBulk", + "WriteSecured2Bulk", // Carry operator_password. "AcknowledgeAlarm", "AcknowledgeAlarmByName" diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Diagnostics/GatewayLogRedactorTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Diagnostics/GatewayLogRedactorTests.cs index 82ec1d3..c74ae79 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Diagnostics/GatewayLogRedactorTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Diagnostics/GatewayLogRedactorTests.cs @@ -1,3 +1,5 @@ +using Google.Protobuf.Reflection; +using ZB.MOM.WW.MxGateway.Contracts.Proto; using ZB.MOM.WW.MxGateway.Server.Diagnostics; namespace ZB.MOM.WW.MxGateway.Tests.Diagnostics; @@ -118,6 +120,8 @@ public sealed class GatewayLogRedactorTests [InlineData("AuthenticateUser")] [InlineData("WriteSecured")] [InlineData("WriteSecured2")] + [InlineData("WriteSecuredBulk")] + [InlineData("WriteSecured2Bulk")] [InlineData("AcknowledgeAlarm")] [InlineData("AcknowledgeAlarmByName")] public void IsCredentialBearingCommand_IdentifiesSensitiveMxAccessCommands(string commandMethod) @@ -146,6 +150,87 @@ public sealed class GatewayLogRedactorTests Assert.Equal("[redacted]", redacted); } + /// + /// Verifies that the secured bulk writes are redacted even with value logging enabled. These + /// carry the same credential-sensitive value as the single-item secured writes (#193). + /// + /// The secured bulk command kind. + [Theory] + [InlineData(MxCommandKind.WriteSecuredBulk)] + [InlineData(MxCommandKind.WriteSecured2Bulk)] + public void RedactCommandValue_RedactsSecuredBulkWriteEvenWhenValueLoggingIsEnabled(MxCommandKind kind) + { + // WorkerClient logs the method as MxCommandKind.ToString(); exercise that exact spelling. + object? redacted = GatewayLogRedactor.RedactCommandValue( + kind.ToString(), + "credential-bearing-bulk-value", + valueLoggingEnabled: true); + + Assert.Equal("[redacted]", redacted); + } + + /// + /// Cross-checks the redactor against the contract: every payload that + /// carries a password field, or is a secured write, must map to a credential-bearing command + /// kind. A new credential-bearing command added to the proto without a redactor entry fails here. + /// + [Fact] + public void IsCredentialBearingCommand_CoversEveryCredentialBearingPayloadInTheContract() + { + List sensitivePayloads = []; + foreach (FieldDescriptor field in MxCommand.Descriptor.Oneofs.Single().Fields) + { + if (field.FieldType != FieldType.Message) + { + continue; + } + + MessageDescriptor payload = field.MessageType; + if (payload.Name.Contains("Secured", StringComparison.Ordinal) + || CarriesPassword(payload, [])) + { + sensitivePayloads.Add(payload.Name); + } + } + + // Positive control: the walk must find the known credential carriers, or it proves nothing. + Assert.Contains("AuthenticateUserCommand", sensitivePayloads); + Assert.Contains("WriteSecuredBulkCommand", sensitivePayloads); + Assert.Contains("AcknowledgeAlarmByNameCommand", sensitivePayloads); + + foreach (string payloadName in sensitivePayloads) + { + string kindName = payloadName[..^"Command".Length]; + Assert.True(Enum.TryParse(kindName, out MxCommandKind _), $"No MxCommandKind named {kindName}."); + Assert.True( + GatewayLogRedactor.IsCredentialBearingCommand(kindName), + $"{kindName} carries credential material but is not redacted."); + } + } + + private static bool CarriesPassword(MessageDescriptor message, HashSet visited) + { + if (!visited.Add(message.FullName)) + { + return false; + } + + foreach (FieldDescriptor field in message.Fields.InDeclarationOrder()) + { + if (field.Name.Contains("password", StringComparison.OrdinalIgnoreCase)) + { + return true; + } + + if (field.FieldType == FieldType.Message && CarriesPassword(field.MessageType, visited)) + { + return true; + } + } + + return false; + } + /// Verifies that RedactCommandValue allows non-sensitive values only when value logging is enabled. [Fact] public void RedactCommandValue_AllowsNonSensitiveValueOnlyWhenValueLoggingIsEnabled()