dohertj2/ScadaBridge
1
0
Fork 0
Code Issues 2 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(audit): fail closed when a configured redactor is unavailable (#35)

Browse Source 
Component-AuditLog.md has always required "we over-redact, never under-redact,
on configuration faults", but the body / SQL-parameter redactors violated it.

AuditRegexCache rejects a pattern that is malformed OR whose compile exceeds a
100 ms budget, caching the rejection for the process lifetime.
ScadaBridgeAuditRedactor then simply dropped the rejected pattern from its
redactor set and emitted the payload anyway — publishing precisely the values
the operator configured it to suppress, onto a row that looks entirely normal
downstream. Recovery required a process restart and the only signal was one
Warning line. The SQL path was worse: TryGetSqlParamRedactor returned a bare
false for both "no redactor configured for this connection" and "the configured
one will not compile", and CLAUDE.md records SQL parameter capture as on by
default.

Two changes:

1. Fail closed. A pattern that is CONFIGURED but unavailable now over-redacts
   the whole payload and increments AuditRedactionFailure, reusing the existing
   safety-net path. "Not configured at all" stays permissive — conflating those
   two states is the actual defect, so both are pinned by tests.

2. Precompile off the hot path. The audit-log roadmap specifies patterns are
   "precompiled at startup; rejected if compile takes >100ms"; the implementation
   had drifted to compiling lazily on first event, which put a wall-clock budget
   on a hot path under production load. RegexOptions.Compiled emits IL during
   construction, so a busy node could blow the budget on a perfectly valid
   pattern. Warm-up now runs at construction and on every options reload. The
   residual window between a reload and its warm-up is safe because that path
   now fails closed.

Warm-up deliberately does not fail the boot — an unusable pattern degrades the
node to over-redaction (safe, loud) rather than refusing to start. Reading
CurrentValue happens inside the warm-up try so an options provider that throws
still surfaces via Apply's over-redact path, not the constructor
(OuterCatch_OptionsThrows_NeverLeaks_AllSensitiveFieldsOverRedacted).

Also de-flakes GrpcCentralTransportTests.DeadlineExceeded_IsNotRetriedOnThePeer,
which is how this was found. It black-holed node A behind a 300 ms deadline, but
on a saturated machine the call could fail to even START — a genuinely-unsent
failure that IsConnectFailure correctly fails over on, so node B's ack arrived
instead of the expected Status.Failure. The test read as a flake while actually
reporting that its own premise had not held. Split in two: the hard rule now
injects an explicit DeadlineExceeded via a trailers-only response (deterministic,
load-independent), and a new BlackHoledNode_DoesNotHang covers the
deadline-is-actually-applied half with both nodes black-holed so no ack can
arrive down any path.

Verified: both fixes were confirmed to fail before they pass — reverting the
fail-closed guard fails exactly the 5 fail-closed tests while the 4 controls
still pass, and adding DeadlineExceeded to IsConnectFailure fails the rewritten
transport test. AuditLog 367/367, Host.Tests GrpcCentralTransport 8/8, solution
build clean. The previously-intermittent
Filter_PicksUp_NewBodyRedactor_OnConfigReload is green in a full sweep for the
first time.

Not addressed here, and noted on #35: the 100 ms wall-clock budget remains a
weak proxy for catastrophic backtracking (RegexOptions.Compiled defers JIT to
first match, so construction time measures the wrong thing), and a rejection is
still cached permanently. Both are now safe rather than dangerous, so they are
hardening rather than a leak.
This commit is contained in:
Joseph Doherty
2026-08-12 03:04:50 -04:00
parent 7e594054e4
commit 006202f3c7
5 changed files with 654 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/ZB.MOM.WW.ScadaBridge.Host.Tests/GrpcCentralTransportTests.cs
+90 -7
View File
@@ -64,8 +64,16 @@ public class GrpcCentralTransportTests : IAsyncLifetime
backoffBase: TimeSpan.FromMilliseconds(50),
backoffCap: TimeSpan.FromMilliseconds(200));
/// <summary>
/// When set, node A's handler short-circuits every call with this gRPC status
/// instead of reaching its TestServer. See <see cref="GrpcStatusHandler"/>.
/// </summary>
private Grpc.Core.StatusCode? _nodeAForcedStatus;
private HttpMessageHandler HandlerFor(string endpoint) => endpoint == EndpointA
? new ToggleHandler(_nodeA.Server.CreateHandler(), () => _nodeA.IsUp)
? new GrpcStatusHandler(
new ToggleHandler(_nodeA.Server.CreateHandler(), () => _nodeA.IsUp),
() => _nodeAForcedStatus)
: new ToggleHandler(_nodeB.Server.CreateHandler(), () => _nodeB.IsUp);
private GrpcCentralTransport NewTransport(CentralChannelProvider provider, CommunicationOptions? options = null)
@@ -182,10 +190,43 @@ public class GrpcCentralTransportTests : IAsyncLifetime
[Fact]
public async Task DeadlineExceeded_IsNotRetriedOnThePeer()
{
// THE hard rule. Node A is UP but never replies, so the call deadlines. The transport must
// surface Status.Failure and must NOT try node B (the call may already have executed).
// THE hard rule: on DeadlineExceeded the call may ALREADY have executed, so the transport
// must surface Status.Failure and must NOT try node B.
//
// The status is injected rather than produced by black-holing node A behind a short
// deadline. That older setup was load-dependent and failed intermittently in full-solution
// sweeps: on a saturated machine the call could fail to even START, which IsConnectFailure
// correctly classifies as provably-unsent, so the transport failed over and node B's ack
// arrived instead of a Status.Failure. The test then read as a flake while actually
// reporting that its own premise had not held. Injecting the status makes the failure mode
// the test's subject rather than a race — see BlackHoledNode_DoesNotHang for the
// deadline-is-actually-applied half.
_nodeAForcedStatus = Grpc.Core.StatusCode.DeadlineExceeded;
using var provider = NewProvider();
var transport = NewTransport(provider);
var inbox = new Capture(_system);
transport.SubmitNotification(NewSubmit("n1"), inbox.Ref);
Assert.IsType<Status.Failure>(inbox.Receive(TimeSpan.FromSeconds(5)));
Assert.Equal(0, provider.CurrentIndex); // no failover on a deadline
Assert.Equal(0, _nodeB.SubmitCount); // peer never tried
}
[Fact]
public async Task BlackHoledNode_DoesNotHang_APerCallDeadlineIsApplied()
{
// The other half of the split: a node that accepts the call and never replies must not
// hang the caller forever — a per-call deadline bounds it. Both nodes black-hole, so this
// holds whichever node the transport ends up on and the assertion cannot be perturbed by
// whether the machine was loaded enough to turn the stall into a connect failure.
_nodeA.SetBlackHole();
var shortDeadline = new CommunicationOptions { NotificationForwardTimeout = TimeSpan.FromMilliseconds(300) };
_nodeB.SetBlackHole();
var shortDeadline = new CommunicationOptions
{
NotificationForwardTimeout = TimeSpan.FromMilliseconds(300),
};
using var provider = NewProvider();
var transport = NewTransport(provider, shortDeadline);
@@ -193,10 +234,9 @@ public class GrpcCentralTransportTests : IAsyncLifetime
transport.SubmitNotification(NewSubmit("n1"), inbox.Ref);
// A per-call deadline is applied (the call returns fast instead of hanging on the black hole).
// Returns rather than hanging: the 5 s inbox wait is far longer than the 300 ms deadline,
// so a missing deadline shows up as a TimeoutException from Receive.
Assert.IsType<Status.Failure>(inbox.Receive(TimeSpan.FromSeconds(5)));
Assert.Equal(0, provider.CurrentIndex); // no failover on a deadline
Assert.Equal(0, _nodeB.SubmitCount); // peer never tried
}
private static NotificationSubmit NewSubmit(string id) => new(
@@ -250,6 +290,49 @@ public class GrpcCentralTransportTests : IAsyncLifetime
}
}
/// <summary>
/// Short-circuits a call with a chosen gRPC status, so a test can pick the exact failure class
/// the transport must classify instead of trying to provoke it with timing.
/// </summary>
/// <remarks>
/// Emits a trailers-only response: HTTP 200 with <c>grpc-status</c> in the HEADERS and an empty
/// body, which is the shape gRPC defines for a call that fails before producing a message and
/// which <c>Grpc.Net.Client</c> surfaces as an <c>RpcException</c> carrying that status.
/// </remarks>
private sealed class GrpcStatusHandler : DelegatingHandler
{
private readonly Func<Grpc.Core.StatusCode?> _forced;
public GrpcStatusHandler(HttpMessageHandler inner, Func<Grpc.Core.StatusCode?> forced)
{
InnerHandler = inner;
_forced = forced;
}
protected override async Task<HttpResponseMessage> SendAsync(
HttpRequestMessage request, CancellationToken cancellationToken)
{
var forced = _forced();
if (forced == null)
{
return await base.SendAsync(request, cancellationToken).ConfigureAwait(false);
}
var response = new HttpResponseMessage(System.Net.HttpStatusCode.OK)
{
Version = new Version(2, 0),
Content = new ByteArrayContent(Array.Empty<byte>()),
RequestMessage = request,
};
response.Content.Headers.ContentType =
new System.Net.Http.Headers.MediaTypeHeaderValue("application/grpc");
response.Headers.TryAddWithoutValidation(
"grpc-status", ((int)forced.Value).ToString(System.Globalization.CultureInfo.InvariantCulture));
response.Headers.TryAddWithoutValidation("grpc-message", "injected by GrpcStatusHandler");
return response;
}
}
/// <summary>A gRPC channel handler that throws (a refused connection) while its node is "down".</summary>
private sealed class ToggleHandler : DelegatingHandler
{