fix(audit): ScadaBridge C2 review — over-redact scrubs all sensitive free-text fields + outer-catch never-leak test + marker alignment
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] → <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.
This commit is contained in:
@@ -42,6 +42,15 @@ internal static class AuditRedactionPrimitives
|
||||
/// <summary>Over-redaction marker emitted when a redactor stage itself faults.</summary>
|
||||
public const string RedactorErrorMarker = "<redacted: redactor error>";
|
||||
|
||||
/// <summary>
|
||||
/// 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 <see cref="RedactorErrorMarker"/>: both represent a
|
||||
/// defensive scrub-everything fallback.
|
||||
/// </summary>
|
||||
public const string OverRedactedEventMarker = RedactorErrorMarker;
|
||||
|
||||
/// <summary>
|
||||
/// 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;
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -47,14 +47,14 @@ namespace ZB.MOM.WW.ScadaBridge.AuditLog.Redaction;
|
||||
/// </list>
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// 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
|
||||
/// <see cref="SafeDefaultAuditPayloadFilter"/>.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public sealed class ScadaBridgeAuditRedactor : IAuditRedactor
|
||||
{
|
||||
private const string OverRedactedMarker = "[redacted by ScadaBridgeAuditRedactor]";
|
||||
private const string OverRedactedMarker = AuditRedactionPrimitives.OverRedactedEventMarker;
|
||||
|
||||
private readonly IOptionsMonitor<AuditLogOptions> _options;
|
||||
private readonly ILogger<ScadaBridgeAuditRedactor> _logger;
|
||||
@@ -307,11 +307,13 @@ public sealed class ScadaBridgeAuditRedactor : IAuditRedactor
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Over-redaction copy returned from the never-throws catch: drop the
|
||||
/// request/response summaries inside <c>DetailsJson</c> to a safe marker and
|
||||
/// flag <see cref="AuditDetails.PayloadTruncated"/>. 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 <c>DetailsJson</c> to a safe
|
||||
/// marker and flag <see cref="AuditDetails.PayloadTruncated"/>. "All sensitive
|
||||
/// fields" = <c>RequestSummary</c>, <c>ResponseSummary</c>, <c>ErrorDetail</c>,
|
||||
/// <c>ErrorMessage</c>, and <c>Extra</c> — 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.
|
||||
/// </summary>
|
||||
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) };
|
||||
|
||||
+3
-3
@@ -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: <redacted>", 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: <redacted>", 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("<redacted>", Details(result).RequestSummary!);
|
||||
Assert.DoesNotContain("x-y-z", Details(result).RequestSummary!);
|
||||
}
|
||||
|
||||
|
||||
@@ -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<ScadaBridgeAuditRedactor>.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 = "<redacted: redactor error>";
|
||||
|
||||
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 ?? "");
|
||||
}
|
||||
|
||||
/// <summary>Counts <see cref="IAuditRedactionFailureCounter.Increment"/> calls.</summary>
|
||||
private sealed class CountingRedactionFailureCounter : IAuditRedactionFailureCounter
|
||||
{
|
||||
@@ -537,4 +589,17 @@ public class ScadaBridgeAuditRedactorTests
|
||||
public AuditLogOptions Get(string? name) => _value;
|
||||
public IDisposable? OnChange(Action<AuditLogOptions, string?> listener) => null;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// IOptionsMonitor test double that throws from <see cref="CurrentValue"/> to
|
||||
/// force the outer try/catch → OverRedact path in
|
||||
/// <see cref="ScadaBridgeAuditRedactor.Apply"/>.
|
||||
/// </summary>
|
||||
private sealed class ThrowingMonitor : IOptionsMonitor<AuditLogOptions>
|
||||
{
|
||||
public AuditLogOptions CurrentValue =>
|
||||
throw new InvalidOperationException("Simulated options fault for outer-catch test");
|
||||
public AuditLogOptions Get(string? name) => new AuditLogOptions();
|
||||
public IDisposable? OnChange(Action<AuditLogOptions, string?> listener) => null;
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user