From 5aaf9e2923729ff6f4c859a47e07091d139fcc2a Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 2 Jun 2026 11:12:18 -0400 Subject: [PATCH] =?UTF-8?q?fix(audit):=20ScadaBridge=20C2=20review=20?= =?UTF-8?q?=E2=80=94=20over-redact=20scrubs=20all=20sensitive=20free-text?= =?UTF-8?q?=20fields=20+=20outer-catch=20never-leak=20test=20+=20marker=20?= =?UTF-8?q?alignment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I1 (security): OverRedact() in ScadaBridgeAuditRedactor now suppresses ErrorDetail, ErrorMessage, and Extra (in addition to RequestSummary/ResponseSummary) to the over-redacted marker in BOTH code paths (Deserialize+with path and the fallback new-AuditDetails path). SafeDefaultAuditRedactor catch block aligned to match. M3 (test): OuterCatch_OptionsThrows_NeverLeaks_AllSensitiveFieldsOverRedacted forces the outer try/catch → OverRedact path via a ThrowingMonitor that throws from CurrentValue (the first statement in the try block). Asserts (a) Apply does not throw, and (b) all five sensitive free-text fields are suppressed to the over-redacted marker with PayloadTruncated=true. M1 (consistency): SafeDefaultAuditRedactor now uses AuditRedactionPrimitives constants (RedactedMarker for line-format header values, OverRedactedEventMarker for the catch block), eliminating the divergent [REDACTED]/[redacted by ...] strings. AuditRedactionPrimitives gains OverRedactedEventMarker = RedactorErrorMarker. SafeDefaultAuditRedactorTests updated from [REDACTED] → . M2 (comment): Added one-line note in TruncateField explaining why the char-count (result.Length != value.Length) truncation check is sufficient given TruncateUtf8 only ever shortens. --- .../Payload/AuditRedactionPrimitives.cs | 11 ++++ .../Redaction/SafeDefaultAuditRedactor.cs | 19 ++++-- .../Redaction/ScadaBridgeAuditRedactor.cs | 24 ++++--- .../SafeDefaultAuditRedactorTests.cs | 6 +- .../ScadaBridgeAuditRedactorTests.cs | 65 +++++++++++++++++++ 5 files changed, 109 insertions(+), 16 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Payload/AuditRedactionPrimitives.cs b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Payload/AuditRedactionPrimitives.cs index 663b21e5..81c01220 100644 --- a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Payload/AuditRedactionPrimitives.cs +++ b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Payload/AuditRedactionPrimitives.cs @@ -42,6 +42,15 @@ internal static class AuditRedactionPrimitives /// Over-redaction marker emitted when a redactor stage itself faults. public const string RedactorErrorMarker = ""; + /// + /// Marker used by the outer never-throws safety net when the entire redaction + /// pipeline fails catastrophically — all potentially-sensitive string fields are + /// set to this value so no raw payload leaks on an unexpected fault. + /// Deliberately equal to : both represent a + /// defensive scrub-everything fallback. + /// + public const string OverRedactedEventMarker = RedactorErrorMarker; + /// /// JSON serializer options used to re-emit redacted summaries. The /// UnsafeRelaxedJsonEscaping encoder is required so the redaction marker @@ -276,6 +285,8 @@ internal static class AuditRedactionPrimitives return null; } var result = TruncateUtf8(value, cap); + // Char-count comparison is sufficient: TruncateUtf8 only ever shortens the + // string, so result.Length < value.Length iff bytes were removed. if (result.Length != value.Length) { truncated = true; diff --git a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Redaction/SafeDefaultAuditRedactor.cs b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Redaction/SafeDefaultAuditRedactor.cs index 29940916..9fc7d3b9 100644 --- a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Redaction/SafeDefaultAuditRedactor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Redaction/SafeDefaultAuditRedactor.cs @@ -2,6 +2,7 @@ using System.Text.RegularExpressions; using ZB.MOM.WW.Audit; using ZB.MOM.WW.ScadaBridge.AuditLog.Payload; using ZB.MOM.WW.ScadaBridge.Commons.Types.Audit; +using static ZB.MOM.WW.ScadaBridge.AuditLog.Payload.AuditRedactionPrimitives; namespace ZB.MOM.WW.ScadaBridge.AuditLog.Redaction; @@ -60,12 +61,18 @@ public sealed class SafeDefaultAuditRedactor : IAuditRedactor } catch { - // Over-redact: drop both summaries entirely so a malformed parse - // path never leaks the original. The contract is "never throw." + // Over-redact: suppress ALL sensitive free-text fields so a failure + // on any internal path never leaks the original. The contract is + // "never throw." Uses the shared OverRedactedEventMarker so all + // redactor safety-nets emit the same sentinel string. var safe = new AuditDetails { - RequestSummary = "[redacted by SafeDefaultAuditRedactor]", - ResponseSummary = "[redacted by SafeDefaultAuditRedactor]", + RequestSummary = OverRedactedEventMarker, + ResponseSummary = OverRedactedEventMarker, + ErrorDetail = OverRedactedEventMarker, + ErrorMessage = OverRedactedEventMarker, + Extra = OverRedactedEventMarker, + PayloadTruncated = true, }; return rawEvent with { DetailsJson = AuditDetailsCodec.Serialize(safe) }; } @@ -82,7 +89,9 @@ public sealed class SafeDefaultAuditRedactor : IAuditRedactor { if (string.Equals(name, sensitive, StringComparison.OrdinalIgnoreCase)) { - return $"{name}: [REDACTED]"; + // Use the shared RedactedMarker so line-format and JSON-format + // header redaction emit the same sentinel string. + return $"{name}: {RedactedMarker}"; } } return m.Value; diff --git a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Redaction/ScadaBridgeAuditRedactor.cs b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Redaction/ScadaBridgeAuditRedactor.cs index 2db12e7b..e343d4d9 100644 --- a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Redaction/ScadaBridgeAuditRedactor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Redaction/ScadaBridgeAuditRedactor.cs @@ -47,14 +47,14 @@ namespace ZB.MOM.WW.ScadaBridge.AuditLog.Redaction; /// /// /// -/// MUST NOT throw — wrapped in try/catch; over-redacts (drops the summaries to a -/// safe marker) on any internal failure, mirroring +/// MUST NOT throw — wrapped in try/catch; over-redacts (drops ALL sensitive free-text +/// fields to a safe marker) on any internal failure, mirroring /// . /// /// public sealed class ScadaBridgeAuditRedactor : IAuditRedactor { - private const string OverRedactedMarker = "[redacted by ScadaBridgeAuditRedactor]"; + private const string OverRedactedMarker = AuditRedactionPrimitives.OverRedactedEventMarker; private readonly IOptionsMonitor _options; private readonly ILogger _logger; @@ -307,11 +307,13 @@ public sealed class ScadaBridgeAuditRedactor : IAuditRedactor } /// - /// Over-redaction copy returned from the never-throws catch: drop the - /// request/response summaries inside DetailsJson to a safe marker and - /// flag . Best-effort re-serialise; - /// if even that fails, return the input with no summaries via an empty - /// details bag. + /// Over-redaction copy returned from the never-throws catch: suppress ALL + /// potentially-sensitive string fields inside DetailsJson to a safe + /// marker and flag . "All sensitive + /// fields" = RequestSummary, ResponseSummary, ErrorDetail, + /// ErrorMessage, and Extra — all body-regex redaction targets + /// that can carry sensitive values. Best-effort re-serialise; if even that + /// fails, return the input with no sensitive fields via a minimal details bag. /// private static AuditEvent OverRedact(AuditEvent rawEvent) { @@ -321,6 +323,9 @@ public sealed class ScadaBridgeAuditRedactor : IAuditRedactor { RequestSummary = OverRedactedMarker, ResponseSummary = OverRedactedMarker, + ErrorDetail = OverRedactedMarker, + ErrorMessage = OverRedactedMarker, + Extra = OverRedactedMarker, PayloadTruncated = true, }; return rawEvent with { DetailsJson = AuditDetailsCodec.Serialize(d) }; @@ -331,6 +336,9 @@ public sealed class ScadaBridgeAuditRedactor : IAuditRedactor { RequestSummary = OverRedactedMarker, ResponseSummary = OverRedactedMarker, + ErrorDetail = OverRedactedMarker, + ErrorMessage = OverRedactedMarker, + Extra = OverRedactedMarker, PayloadTruncated = true, }; return rawEvent with { DetailsJson = AuditDetailsCodec.Serialize(safe) }; diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Redaction/SafeDefaultAuditRedactorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Redaction/SafeDefaultAuditRedactorTests.cs index cb74fc8b..bbba3ddc 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Redaction/SafeDefaultAuditRedactorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Redaction/SafeDefaultAuditRedactorTests.cs @@ -43,7 +43,7 @@ public class SafeDefaultAuditRedactorTests var result = SafeDefaultAuditRedactor.Instance.Apply(evt); var d = Details(result); - Assert.Contains("Authorization: [REDACTED]", d.RequestSummary!); + Assert.Contains("Authorization: ", d.RequestSummary!); Assert.DoesNotContain("secret-token", d.RequestSummary!); Assert.Contains("Content-Type: application/json", d.RequestSummary!); } @@ -56,7 +56,7 @@ public class SafeDefaultAuditRedactorTests var result = SafeDefaultAuditRedactor.Instance.Apply(evt); var d = Details(result); - Assert.Contains("Set-Cookie: [REDACTED]", d.ResponseSummary!); + Assert.Contains("Set-Cookie: ", d.ResponseSummary!); Assert.DoesNotContain("abc123", d.ResponseSummary!); Assert.Contains("X-Other: ok", d.ResponseSummary!); } @@ -68,7 +68,7 @@ public class SafeDefaultAuditRedactorTests var result = SafeDefaultAuditRedactor.Instance.Apply(evt); - Assert.Contains("[REDACTED]", Details(result).RequestSummary!); + Assert.Contains("", Details(result).RequestSummary!); Assert.DoesNotContain("x-y-z", Details(result).RequestSummary!); } diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Redaction/ScadaBridgeAuditRedactorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Redaction/ScadaBridgeAuditRedactorTests.cs index ce271f7f..0c2e4eb7 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Redaction/ScadaBridgeAuditRedactorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Redaction/ScadaBridgeAuditRedactorTests.cs @@ -520,6 +520,58 @@ public class ScadaBridgeAuditRedactorTests Assert.Null(ex); } + [Fact] + public void OuterCatch_OptionsThrows_NeverLeaks_AllSensitiveFieldsOverRedacted() + { + // Force the outer try/catch → OverRedact path by injecting an + // IOptionsMonitor whose CurrentValue getter throws. This is the FIRST + // statement in the try block, so the exception escapes before any + // redaction work has run — the input event could still carry live + // sensitive values in all five free-text fields. + // + // The never-leak contract requires ALL of RequestSummary, ResponseSummary, + // ErrorDetail, ErrorMessage, and Extra to be suppressed to the + // over-redacted marker, and PayloadTruncated to be true. + var sensitiveEvent = NewEvent( + request: "SENSITIVE-REQUEST-DATA", + response: "SENSITIVE-RESPONSE-DATA", + errorDetail: "SENSITIVE-ERROR-DETAIL", + extra: "SENSITIVE-EXTRA-DATA"); + // Manually inject ErrorMessage into DetailsJson (NewEvent does not expose it). + var withMsg = sensitiveEvent with + { + DetailsJson = AuditDetailsCodec.Serialize( + AuditDetailsCodec.Deserialize(sensitiveEvent.DetailsJson) with + { + ErrorMessage = "SENSITIVE-ERROR-MESSAGE", + }), + }; + + var redactor = new ScadaBridgeAuditRedactor( + new ThrowingMonitor(), + Microsoft.Extensions.Logging.Abstractions.NullLogger.Instance); + + // (a) Apply must not throw even when options access itself throws. + var ex = Record.Exception(() => redactor.Apply(withMsg)); + Assert.Null(ex); + + // (b) Returned event must have ALL sensitive free-text fields suppressed + // (never-leak guarantee), and PayloadTruncated must be true. + var result = redactor.Apply(withMsg); + var d = Details(result); + const string marker = ""; + + Assert.Equal(marker, d.RequestSummary); + Assert.Equal(marker, d.ResponseSummary); + Assert.Equal(marker, d.ErrorDetail); + Assert.Equal(marker, d.ErrorMessage); + Assert.Equal(marker, d.Extra); + Assert.True(d.PayloadTruncated); + + // Confirm no raw sensitive values survive anywhere in DetailsJson. + Assert.DoesNotContain("SENSITIVE", result.DetailsJson ?? ""); + } + /// Counts calls. private sealed class CountingRedactionFailureCounter : IAuditRedactionFailureCounter { @@ -537,4 +589,17 @@ public class ScadaBridgeAuditRedactorTests public AuditLogOptions Get(string? name) => _value; public IDisposable? OnChange(Action listener) => null; } + + /// + /// IOptionsMonitor test double that throws from to + /// force the outer try/catch → OverRedact path in + /// . + /// + private sealed class ThrowingMonitor : IOptionsMonitor + { + public AuditLogOptions CurrentValue => + throw new InvalidOperationException("Simulated options fault for outer-catch test"); + public AuditLogOptions Get(string? name) => new AuditLogOptions(); + public IDisposable? OnChange(Action listener) => null; + } }