From 453c3dec184d20c344f29a6a11a26903cc3d3728 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 6 Sep 2026 14:22:01 -0400 Subject: [PATCH] fix(inbound-api): RouteCall audit row honours the method's SkipBodyCapture override (arch-review F11) RouteHelper/RouteTarget carry a new captureBodies flag (default true) threaded through every With* copy, same shape as the site-scope field Task 3 added. InboundScriptExecutor resolves IOptionsMonitor from the per-execution DI scope and looks up PerTargetOverrides[method.Name]?.SkipBodyCapture exactly as AuditWriteMiddleware.cs:183-187 does for the inbound row, then calls .WithBodyCapture(!skip) when building the script's Route helper. When set, the always-on ScriptTrace.RouteCall row omits its request/response summary, so an operator who suppressed a method's body from the audit log doesn't get it back via the routed call's arguments (derived from that same body). Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29 --- .../InboundScriptExecutor.cs | 15 +++- .../RouteHelper.cs | 48 ++++++++--- .../InboundScriptExecutorTests.cs | 74 +++++++++++++++++ .../RouteHelperTests.cs | 79 +++++++++++++++++++ 4 files changed, 203 insertions(+), 13 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs index 2a802c85..e1eaa417 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs @@ -4,6 +4,8 @@ using Microsoft.CodeAnalysis.CSharp.Scripting; using Microsoft.CodeAnalysis.Scripting; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using ZB.MOM.WW.ScadaBridge.AuditLog.Configuration; using ZB.MOM.WW.ScadaBridge.Commons.Entities.InboundApi; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; @@ -411,12 +413,23 @@ public class InboundScriptExecutor // Also bind the inbound request's ExecutionId (ParentExecutionId) so a // routed call carries it as ParentExecutionId — the // spawned site script execution points back at this inbound request. + // Arch-review F11: the always-on RouteCall row must honour the method's + // SkipBodyCapture override — the same per-target lookup AuditWriteMiddleware + // performs for the inbound row (AuditWriteMiddleware.cs:183-187) — so an + // operator who suppressed a method's body from the audit log doesn't get it + // back via the routed call's arguments (derived from that same body). + var auditOptions = scope?.ServiceProvider.GetService>()?.CurrentValue; + var skipBodyCapture = auditOptions != null + && auditOptions.PerTargetOverrides.TryGetValue(method.Name, out var perTargetOverride) + && perTargetOverride.SkipBodyCapture; + var context = new InboundScriptContext( parameters, route.WithDeadline(cts.Token) .WithRequestAborted(cancellationToken) .WithParentExecutionId(parentExecutionId) - .WithTrace(trace), + .WithTrace(trace) + .WithBodyCapture(!skipBodyCapture), dbHelper, cts.Token); diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs index b9a745d1..33269120 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs @@ -86,6 +86,7 @@ public class RouteHelper private readonly bool _trace; private readonly Action? _traceSink; private readonly IReadOnlySet? _permittedSiteIdentifiers; + private readonly bool _captureBodies; /// /// Initializes a new with no deadline token and no parent execution id. @@ -102,7 +103,7 @@ public class RouteHelper ICentralAuditWriter? auditWriter = null, INodeIdentityProvider? nodeIdentity = null) : this(instanceLocator, instanceRouter, CancellationToken.None, CancellationToken.None, parentExecutionId: null, - auditWriter, nodeIdentity, trace: false, traceSink: null, permittedSiteIdentifiers: null) + auditWriter, nodeIdentity, trace: false, traceSink: null, permittedSiteIdentifiers: null, captureBodies: true) { } @@ -116,7 +117,8 @@ public class RouteHelper INodeIdentityProvider? nodeIdentity, bool trace, Action? traceSink, - IReadOnlySet? permittedSiteIdentifiers) + IReadOnlySet? permittedSiteIdentifiers, + bool captureBodies) { _permittedSiteIdentifiers = permittedSiteIdentifiers; _instanceLocator = instanceLocator; @@ -128,6 +130,7 @@ public class RouteHelper _nodeIdentity = nodeIdentity; _trace = trace; _traceSink = traceSink; + _captureBodies = captureBodies; } /// @@ -141,7 +144,7 @@ public class RouteHelper /// A new inheriting the given deadline token. public RouteHelper WithDeadline(CancellationToken deadlineToken) => new(_instanceLocator, _instanceRouter, deadlineToken, _requestAbortedToken, _parentExecutionId, - _auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers); + _auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers, _captureBodies); /// /// Returns a carrying the raw request-abort @@ -155,7 +158,7 @@ public class RouteHelper /// A new carrying the given request-abort token. public RouteHelper WithRequestAborted(CancellationToken requestAbortedToken) => new(_instanceLocator, _instanceRouter, _deadlineToken, requestAbortedToken, _parentExecutionId, - _auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers); + _auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers, _captureBodies); /// /// Audit Log (ParentExecutionId): returns a whose @@ -170,7 +173,7 @@ public class RouteHelper /// A new carrying the given parent execution id. public RouteHelper WithParentExecutionId(Guid? parentExecutionId) => new(_instanceLocator, _instanceRouter, _deadlineToken, _requestAbortedToken, parentExecutionId, - _auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers); + _auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers, _captureBodies); /// /// Flow Debugger (design 2026-09-05 §5.2): returns a whose routed @@ -181,7 +184,7 @@ public class RouteHelper /// A new carrying the flag. public RouteHelper WithTrace(bool trace) => new(_instanceLocator, _instanceRouter, _deadlineToken, _requestAbortedToken, _parentExecutionId, - _auditWriter, _nodeIdentity, trace, _traceSink, _permittedSiteIdentifiers); + _auditWriter, _nodeIdentity, trace, _traceSink, _permittedSiteIdentifiers, _captureBodies); /// /// Flow Debugger (design 2026-09-05 §7.1): returns a that also @@ -193,7 +196,7 @@ public class RouteHelper /// A new carrying the sink. public RouteHelper WithTraceSink(Action? traceSink) => new(_instanceLocator, _instanceRouter, _deadlineToken, _requestAbortedToken, _parentExecutionId, - _auditWriter, _nodeIdentity, _trace, traceSink, _permittedSiteIdentifiers); + _auditWriter, _nodeIdentity, _trace, traceSink, _permittedSiteIdentifiers, _captureBodies); /// /// Flow Debugger outbound calls (plan 2026-09-06, D5/D7): the trace sink this helper carries, @@ -227,7 +230,22 @@ public class RouteHelper /// A new carrying the scope. public RouteHelper WithSiteScope(IReadOnlySet? permittedSiteIdentifiers) => new(_instanceLocator, _instanceRouter, _deadlineToken, _requestAbortedToken, _parentExecutionId, - _auditWriter, _nodeIdentity, _trace, _traceSink, permittedSiteIdentifiers); + _auditWriter, _nodeIdentity, _trace, _traceSink, permittedSiteIdentifiers, _captureBodies); + + /// + /// Arch-review F11: returns a whose always-on RouteCall row + /// carries no request/response summary when is false. + /// sets this from the executing method's + /// AuditLogOptions.PerTargetOverrides[method.Name].SkipBodyCapture — the same override + /// AuditWriteMiddleware honours for the inbound row — so an operator who suppressed a + /// method's body from the audit log doesn't get it back via the routed call's arguments + /// (which are derived from that same body). Default true (unchanged behaviour). + /// + /// False to omit the request/response summary on the RouteCall row. + /// A new carrying the flag. + public RouteHelper WithBodyCapture(bool captureBodies) => + new(_instanceLocator, _instanceRouter, _deadlineToken, _requestAbortedToken, _parentExecutionId, + _auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers, captureBodies); /// /// Creates a route target for the specified instance. @@ -238,7 +256,7 @@ public class RouteHelper { return new RouteTarget( instanceCode, _instanceLocator, _instanceRouter, _deadlineToken, _requestAbortedToken, _parentExecutionId, - _auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers); + _auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers, _captureBodies); } } @@ -258,6 +276,7 @@ public class RouteTarget private readonly bool _trace; private readonly Action? _traceSink; private readonly IReadOnlySet? _permittedSiteIdentifiers; + private readonly bool _captureBodies; /// /// Initializes a new for the specified instance. @@ -273,6 +292,7 @@ public class RouteTarget /// True to trace the routed site run. /// Receives the emitted audit events in order; null = none. /// Arch-review F2: site identifiers this call may reach; null = unscoped. + /// Arch-review F11: false omits the request/response summary on the RouteCall row. internal RouteTarget( string instanceCode, IInstanceLocator instanceLocator, @@ -284,7 +304,8 @@ public class RouteTarget INodeIdentityProvider? nodeIdentity = null, bool trace = false, Action? traceSink = null, - IReadOnlySet? permittedSiteIdentifiers = null) + IReadOnlySet? permittedSiteIdentifiers = null, + bool captureBodies = true) { _permittedSiteIdentifiers = permittedSiteIdentifiers; _instanceCode = instanceCode; @@ -297,6 +318,7 @@ public class RouteTarget _nodeIdentity = nodeIdentity; _trace = trace; _traceSink = traceSink; + _captureBodies = captureBodies; } /// @@ -385,8 +407,10 @@ public class RouteTarget parentExecutionId: null, durationMs: (int)Math.Min(elapsedMs, int.MaxValue), errorMessage: error, - requestSummary: TraceJson(args), - responseSummary: TraceJson(returnValue), + // Arch-review F11: a method with SkipBodyCapture set must not leak the routed + // call's arguments (derived from the same request body) or its return value. + requestSummary: _captureBodies ? TraceJson(args) : null, + responseSummary: _captureBodies ? TraceJson(returnValue) : null, extra: childExecutionId is null ? null : new ScriptTraceExtra { ChildExecutionId = childExecutionId }.ToJson()); } catch (Exception) diff --git a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs index f81573f1..068deb93 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs @@ -4,10 +4,13 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using NSubstitute; +using ZB.MOM.WW.Audit; +using ZB.MOM.WW.ScadaBridge.AuditLog.Configuration; using ZB.MOM.WW.ScadaBridge.Commons.Entities.InboundApi; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; using ZB.MOM.WW.ScadaBridge.Commons.Messages.InboundApi; using ZB.MOM.WW.ScadaBridge.Commons.Types; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Audit; namespace ZB.MOM.WW.ScadaBridge.InboundAPI.Tests; @@ -57,6 +60,77 @@ public class InboundScriptExecutorTests Assert.False(captured[1].Trace); } + [Fact] + public async Task Executor_MethodWithSkipBodyCapture_RoutesWithBodyCaptureOff() + { + // Arch-review F11: a method configured with PerTargetOverrides[method].SkipBodyCapture + // must build its Route helper WithBodyCapture(false), so the always-on RouteCall row + // carries no request/response summary — mirroring AuditWriteMiddleware's own + // SkipBodyCapture lookup for the inbound row (AuditWriteMiddleware.cs:183-187). + var locator = Substitute.For(); + var router = Substitute.For(); + locator.GetSiteIdForInstanceAsync("inst", Arg.Any()).Returns("SiteA"); + router.RouteToCallAsync("SiteA", Arg.Any(), Arg.Any()) + .Returns(ci => new RouteToCallResponse(((RouteToCallRequest)ci[1]).CorrelationId, true, 1, null, DateTimeOffset.UtcNow)); + + var writer = Substitute.For(); + AuditEvent? written = null; + writer.WriteAsync(Arg.Do(e => written = e), Arg.Any()) + .Returns(Task.CompletedTask); + var route = new RouteHelper(locator, router, writer, null); + + var services = new ServiceCollection(); + services.Configure(o => + o.PerTargetOverrides["routed"] = new PerTargetRedactionOverride { SkipBodyCapture = true }); + using var provider = services.BuildServiceProvider(); + + var executor = new InboundScriptExecutor(NullLogger.Instance, provider); + var method = new ApiMethod("routed", "x") { Id = 9, TimeoutSeconds = 10 }; + executor.RegisterHandler("routed", ctx => ctx.Route.To("inst").Call("work", new { x = 1 })); + + var result = await executor.ExecuteAsync(method, new Dictionary(), route, TimeSpan.FromSeconds(10)); + + Assert.True(result.Success, result.ErrorMessage); + Assert.NotNull(written); + var row = written!.AsRow(); + Assert.Null(row.RequestSummary); + Assert.Null(row.ResponseSummary); + } + + [Fact] + public async Task Executor_MethodWithoutSkipBodyCapture_RoutesWithBodyCaptureOn() + { + // Control: no PerTargetOverrides entry for this method → default behaviour unchanged. + var locator = Substitute.For(); + var router = Substitute.For(); + locator.GetSiteIdForInstanceAsync("inst", Arg.Any()).Returns("SiteA"); + router.RouteToCallAsync("SiteA", Arg.Any(), Arg.Any()) + .Returns(ci => new RouteToCallResponse(((RouteToCallRequest)ci[1]).CorrelationId, true, 1, null, DateTimeOffset.UtcNow)); + + var writer = Substitute.For(); + AuditEvent? written = null; + writer.WriteAsync(Arg.Do(e => written = e), Arg.Any()) + .Returns(Task.CompletedTask); + var route = new RouteHelper(locator, router, writer, null); + + var services = new ServiceCollection(); + services.Configure(o => + o.PerTargetOverrides["other-method"] = new PerTargetRedactionOverride { SkipBodyCapture = true }); + using var provider = services.BuildServiceProvider(); + + var executor = new InboundScriptExecutor(NullLogger.Instance, provider); + var method = new ApiMethod("routed", "x") { Id = 9, TimeoutSeconds = 10 }; + executor.RegisterHandler("routed", ctx => ctx.Route.To("inst").Call("work", new { x = 1 })); + + var result = await executor.ExecuteAsync(method, new Dictionary(), route, TimeSpan.FromSeconds(10)); + + Assert.True(result.Success, result.ErrorMessage); + Assert.NotNull(written); + var row = written!.AsRow(); + Assert.Contains("\"x\":1", row.RequestSummary); + Assert.Equal("1", row.ResponseSummary); + } + [Fact] public async Task RegisteredHandler_ExecutesSuccessfully() { diff --git a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/RouteHelperTests.cs b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/RouteHelperTests.cs index e07d4baf..168e3a47 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/RouteHelperTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/RouteHelperTests.cs @@ -1,7 +1,9 @@ using NSubstitute; +using ZB.MOM.WW.Audit; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; using ZB.MOM.WW.ScadaBridge.Commons.Messages.InboundApi; using ZB.MOM.WW.ScadaBridge.Commons.Types; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Audit; namespace ZB.MOM.WW.ScadaBridge.InboundAPI.Tests; @@ -703,4 +705,81 @@ public class RouteHelperTests await Assert.ThrowsAsync( () => helper.To("inst-1").Call("doWork")); } + + // --- Arch-review F11: WithBodyCapture --- + + [Fact] + public async Task RouteCallRow_WithBodyCaptureOff_HasNoRequestOrResponseSummary() + { + SiteResolves("inst-1", "SiteA"); + _router.RouteToCallAsync("SiteA", Arg.Any(), Arg.Any()) + .Returns(ci => new RouteToCallResponse( + ((RouteToCallRequest)ci[1]).CorrelationId, true, 99, null, DateTimeOffset.UtcNow)); + var writer = Substitute.For(); + AuditEvent? written = null; + writer.WriteAsync(Arg.Do(e => written = e), Arg.Any()) + .Returns(Task.CompletedTask); + var helper = new RouteHelper(_locator, _router, writer, null).WithBodyCapture(false); + + var result = await helper.To("inst-1").Call("doWork", new { x = 1 }); + + Assert.Equal(99, result); + Assert.NotNull(written); + var row = written!.AsRow(); + Assert.Null(row.RequestSummary); + Assert.Null(row.ResponseSummary); + } + + [Fact] + public async Task RouteCallRow_WithBodyCaptureOn_ByDefault_HasRequestAndResponseSummary() + { + SiteResolves("inst-1", "SiteA"); + _router.RouteToCallAsync("SiteA", Arg.Any(), Arg.Any()) + .Returns(ci => new RouteToCallResponse( + ((RouteToCallRequest)ci[1]).CorrelationId, true, 99, null, DateTimeOffset.UtcNow)); + var writer = Substitute.For(); + AuditEvent? written = null; + writer.WriteAsync(Arg.Do(e => written = e), Arg.Any()) + .Returns(Task.CompletedTask); + var helper = new RouteHelper(_locator, _router, writer, null); + + await helper.To("inst-1").Call("doWork", new { x = 1 }); + + Assert.NotNull(written); + var row = written!.AsRow(); + Assert.Contains("\"x\":1", row.RequestSummary); + Assert.Equal("99", row.ResponseSummary); + } + + [Fact] + public async Task WithBodyCapture_SurvivesTheOtherWithCopies() + { + // Mirrors WithSiteScope_SurvivesTheOtherWithCopies (Task 3): the flag must not be + // dropped by the copies InboundScriptExecutor makes (WithDeadline / + // WithRequestAborted / WithParentExecutionId / WithTrace / WithSiteScope). + SiteResolves("inst-1", "SiteA"); + _router.RouteToCallAsync("SiteA", Arg.Any(), Arg.Any()) + .Returns(ci => new RouteToCallResponse( + ((RouteToCallRequest)ci[1]).CorrelationId, true, 99, null, DateTimeOffset.UtcNow)); + var writer = Substitute.For(); + AuditEvent? written = null; + writer.WriteAsync(Arg.Do(e => written = e), Arg.Any()) + .Returns(Task.CompletedTask); + + var helper = new RouteHelper(_locator, _router, writer, null) + .WithBodyCapture(false) + .WithDeadline(CancellationToken.None) + .WithRequestAborted(CancellationToken.None) + .WithParentExecutionId(Guid.NewGuid()) + .WithTrace(true) + .WithTraceSink(null) + .WithSiteScope(Scope("SiteA")); + + await helper.To("inst-1").Call("doWork", new { x = 1 }); + + Assert.NotNull(written); + var row = written!.AsRow(); + Assert.Null(row.RequestSummary); + Assert.Null(row.ResponseSummary); + } }