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<AuditLogOptions> 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
2026-09-06 14:22:01 -04:00
parent 3f1981865a
commit 453c3dec18
4 changed files with 203 additions and 13 deletions
@@ -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<IOptionsMonitor<AuditLogOptions>>()?.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);

@@ -86,6 +86,7 @@ public class RouteHelper
    private readonly bool _trace;
    private readonly Action<AuditEvent>? _traceSink;
    private readonly IReadOnlySet<string>? _permittedSiteIdentifiers;
+    private readonly bool _captureBodies;

    /// <summary>
    /// Initializes a new <see cref="RouteHelper"/> 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<AuditEvent>? traceSink,
-        IReadOnlySet<string>? permittedSiteIdentifiers)
+        IReadOnlySet<string>? permittedSiteIdentifiers,
+        bool captureBodies)
    {
        _permittedSiteIdentifiers = permittedSiteIdentifiers;
        _instanceLocator = instanceLocator;
@@ -128,6 +130,7 @@ public class RouteHelper
        _nodeIdentity = nodeIdentity;
        _trace = trace;
        _traceSink = traceSink;
+        _captureBodies = captureBodies;
    }

    /// <summary>
@@ -141,7 +144,7 @@ public class RouteHelper
    /// <returns>A new <see cref="RouteHelper"/> inheriting the given deadline token.</returns>
    public RouteHelper WithDeadline(CancellationToken deadlineToken) =>
        new(_instanceLocator, _instanceRouter, deadlineToken, _requestAbortedToken, _parentExecutionId,
-            _auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers);
+            _auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers, _captureBodies);

    /// <summary>
    /// Returns a <see cref="RouteHelper"/> carrying the raw request-abort
@@ -155,7 +158,7 @@ public class RouteHelper
    /// <returns>A new <see cref="RouteHelper"/> carrying the given request-abort token.</returns>
    public RouteHelper WithRequestAborted(CancellationToken requestAbortedToken) =>
        new(_instanceLocator, _instanceRouter, _deadlineToken, requestAbortedToken, _parentExecutionId,
-            _auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers);
+            _auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers, _captureBodies);

    /// <summary>
    /// Audit Log (ParentExecutionId): returns a <see cref="RouteHelper"/> whose
@@ -170,7 +173,7 @@ public class RouteHelper
    /// <returns>A new <see cref="RouteHelper"/> carrying the given parent execution id.</returns>
    public RouteHelper WithParentExecutionId(Guid? parentExecutionId) =>
        new(_instanceLocator, _instanceRouter, _deadlineToken, _requestAbortedToken, parentExecutionId,
-            _auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers);
+            _auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers, _captureBodies);

    /// <summary>
    /// Flow Debugger (design 2026-09-05 §5.2): returns a <see cref="RouteHelper"/> whose routed
@@ -181,7 +184,7 @@ public class RouteHelper
    /// <returns>A new <see cref="RouteHelper"/> carrying the flag.</returns>
    public RouteHelper WithTrace(bool trace) =>
        new(_instanceLocator, _instanceRouter, _deadlineToken, _requestAbortedToken, _parentExecutionId,
-            _auditWriter, _nodeIdentity, trace, _traceSink, _permittedSiteIdentifiers);
+            _auditWriter, _nodeIdentity, trace, _traceSink, _permittedSiteIdentifiers, _captureBodies);

    /// <summary>
    /// Flow Debugger (design 2026-09-05 §7.1): returns a <see cref="RouteHelper"/> that also
@@ -193,7 +196,7 @@ public class RouteHelper
    /// <returns>A new <see cref="RouteHelper"/> carrying the sink.</returns>
    public RouteHelper WithTraceSink(Action<AuditEvent>? traceSink) =>
        new(_instanceLocator, _instanceRouter, _deadlineToken, _requestAbortedToken, _parentExecutionId,
-            _auditWriter, _nodeIdentity, _trace, traceSink, _permittedSiteIdentifiers);
+            _auditWriter, _nodeIdentity, _trace, traceSink, _permittedSiteIdentifiers, _captureBodies);

    /// <summary>
    /// Flow Debugger outbound calls (plan 2026-09-06, D5/D7): the trace sink this helper carries,
@@ -227,7 +230,22 @@ public class RouteHelper
    /// <returns>A new <see cref="RouteHelper"/> carrying the scope.</returns>
    public RouteHelper WithSiteScope(IReadOnlySet<string>? permittedSiteIdentifiers) =>
        new(_instanceLocator, _instanceRouter, _deadlineToken, _requestAbortedToken, _parentExecutionId,
-            _auditWriter, _nodeIdentity, _trace, _traceSink, permittedSiteIdentifiers);
+            _auditWriter, _nodeIdentity, _trace, _traceSink, permittedSiteIdentifiers, _captureBodies);
+
+    /// <summary>
+    /// Arch-review F11: returns a <see cref="RouteHelper"/> whose always-on <c>RouteCall</c> row
+    /// carries no request/response summary when <paramref name="captureBodies"/> is <c>false</c>.
+    /// <see cref="InboundScriptExecutor"/> sets this from the executing method's
+    /// <c>AuditLogOptions.PerTargetOverrides[method.Name].SkipBodyCapture</c> — the same override
+    /// <c>AuditWriteMiddleware</c> 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 <c>true</c> (unchanged behaviour).
+    /// </summary>
+    /// <param name="captureBodies">False to omit the request/response summary on the <c>RouteCall</c> row.</param>
+    /// <returns>A new <see cref="RouteHelper"/> carrying the flag.</returns>
+    public RouteHelper WithBodyCapture(bool captureBodies) =>
+        new(_instanceLocator, _instanceRouter, _deadlineToken, _requestAbortedToken, _parentExecutionId,
+            _auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers, captureBodies);

    /// <summary>
    /// 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<AuditEvent>? _traceSink;
    private readonly IReadOnlySet<string>? _permittedSiteIdentifiers;
+    private readonly bool _captureBodies;

    /// <summary>
    /// Initializes a new <see cref="RouteTarget"/> for the specified instance.
@@ -273,6 +292,7 @@ public class RouteTarget
    /// <param name="trace">True to trace the routed site run.</param>
    /// <param name="traceSink">Receives the emitted audit events in order; null = none.</param>
    /// <param name="permittedSiteIdentifiers">Arch-review F2: site identifiers this call may reach; null = unscoped.</param>
+    /// <param name="captureBodies">Arch-review F11: false omits the request/response summary on the <c>RouteCall</c> row.</param>
    internal RouteTarget(
        string instanceCode,
        IInstanceLocator instanceLocator,
@@ -284,7 +304,8 @@ public class RouteTarget
        INodeIdentityProvider? nodeIdentity = null,
        bool trace = false,
        Action<AuditEvent>? traceSink = null,
-        IReadOnlySet<string>? permittedSiteIdentifiers = null)
+        IReadOnlySet<string>? permittedSiteIdentifiers = null,
+        bool captureBodies = true)
    {
        _permittedSiteIdentifiers = permittedSiteIdentifiers;
        _instanceCode = instanceCode;
@@ -297,6 +318,7 @@ public class RouteTarget
        _nodeIdentity = nodeIdentity;
        _trace = trace;
        _traceSink = traceSink;
+        _captureBodies = captureBodies;
    }

    /// <summary>
@@ -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)
@@ -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<IInstanceLocator>();
+        var router = Substitute.For<IInstanceRouter>();
+        locator.GetSiteIdForInstanceAsync("inst", Arg.Any<CancellationToken>()).Returns("SiteA");
+        router.RouteToCallAsync("SiteA", Arg.Any<RouteToCallRequest>(), Arg.Any<CancellationToken>())
+            .Returns(ci => new RouteToCallResponse(((RouteToCallRequest)ci[1]).CorrelationId, true, 1, null, DateTimeOffset.UtcNow));
+
+        var writer = Substitute.For<ICentralAuditWriter>();
+        AuditEvent? written = null;
+        writer.WriteAsync(Arg.Do<AuditEvent>(e => written = e), Arg.Any<CancellationToken>())
+            .Returns(Task.CompletedTask);
+        var route = new RouteHelper(locator, router, writer, null);
+
+        var services = new ServiceCollection();
+        services.Configure<AuditLogOptions>(o =>
+            o.PerTargetOverrides["routed"] = new PerTargetRedactionOverride { SkipBodyCapture = true });
+        using var provider = services.BuildServiceProvider();
+
+        var executor = new InboundScriptExecutor(NullLogger<InboundScriptExecutor>.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<string, object?>(), 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<IInstanceLocator>();
+        var router = Substitute.For<IInstanceRouter>();
+        locator.GetSiteIdForInstanceAsync("inst", Arg.Any<CancellationToken>()).Returns("SiteA");
+        router.RouteToCallAsync("SiteA", Arg.Any<RouteToCallRequest>(), Arg.Any<CancellationToken>())
+            .Returns(ci => new RouteToCallResponse(((RouteToCallRequest)ci[1]).CorrelationId, true, 1, null, DateTimeOffset.UtcNow));
+
+        var writer = Substitute.For<ICentralAuditWriter>();
+        AuditEvent? written = null;
+        writer.WriteAsync(Arg.Do<AuditEvent>(e => written = e), Arg.Any<CancellationToken>())
+            .Returns(Task.CompletedTask);
+        var route = new RouteHelper(locator, router, writer, null);
+
+        var services = new ServiceCollection();
+        services.Configure<AuditLogOptions>(o =>
+            o.PerTargetOverrides["other-method"] = new PerTargetRedactionOverride { SkipBodyCapture = true });
+        using var provider = services.BuildServiceProvider();
+
+        var executor = new InboundScriptExecutor(NullLogger<InboundScriptExecutor>.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<string, object?>(), 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()
    {
@@ -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<RouteSiteScopeException>(
            () => 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<RouteToCallRequest>(), Arg.Any<CancellationToken>())
+            .Returns(ci => new RouteToCallResponse(
+                ((RouteToCallRequest)ci[1]).CorrelationId, true, 99, null, DateTimeOffset.UtcNow));
+        var writer = Substitute.For<ICentralAuditWriter>();
+        AuditEvent? written = null;
+        writer.WriteAsync(Arg.Do<AuditEvent>(e => written = e), Arg.Any<CancellationToken>())
+            .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<RouteToCallRequest>(), Arg.Any<CancellationToken>())
+            .Returns(ci => new RouteToCallResponse(
+                ((RouteToCallRequest)ci[1]).CorrelationId, true, 99, null, DateTimeOffset.UtcNow));
+        var writer = Substitute.For<ICentralAuditWriter>();
+        AuditEvent? written = null;
+        writer.WriteAsync(Arg.Do<AuditEvent>(e => written = e), Arg.Any<CancellationToken>())
+            .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<RouteToCallRequest>(), Arg.Any<CancellationToken>())
+            .Returns(ci => new RouteToCallResponse(
+                ((RouteToCallRequest)ci[1]).CorrelationId, true, 99, null, DateTimeOffset.UtcNow));
+        var writer = Substitute.For<ICentralAuditWriter>();
+        AuditEvent? written = null;
+        writer.WriteAsync(Arg.Do<AuditEvent>(e => written = e), Arg.Any<CancellationToken>())
+            .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);
+    }
 }