fix(inbound-api): Flow Debugger method-entry Live run enforces the operator's site scope at route time (arch-review F2)

A method-entry Live run had no site-scope check at all. RunValueChangeAsync
calls EnforceSiteScope on its named instance, but a method run has no single
target site up front — the operator picks a METHOD, and where its script routes
is only known once it runs — so a site-scoped Deployer could reach any site the
method happened to route to.

The scope is therefore pushed into RouteHelper and enforced at route time:

- RouteHelper.WithSiteScope(IReadOnlySet<string>?) carries a set of site
  IDENTIFIERS (null = unscoped, the state of every ordinary inbound HTTP
  request; an EMPTY set permits nothing, the fail-closed outcome for a scoped
  user whose sites no longer exist). Threaded through every With* copy and To().
- The gate lives in RouteTarget.ResolveSiteAsync — the single choke point that
  Call, GetAttributes, WaitForAttribute and SetAttributes all pass through — so
  it fires BEFORE any site traffic and throws the new RouteSiteScopeException.
- InboundScriptExecutor maps it to error code SITE_SCOPE_DENIED. The type is
  InboundAPI's own (that assembly cannot reference ManagementService), and it
  derives from InvalidOperationException so base-type callers are unaffected.
- FlowDebugRunner.RunMethodAsync resolves the user's PermittedSiteIds (numeric)
  to identifiers via ISiteRepository and converts SITE_SCOPE_DENIED back to
  SiteScopeViolationException (-> ManagementUnauthorized), AFTER writing the
  inbound audit row so a denied attempt is still recorded against the operator.

Administrators and unscoped users get a null scope and are unaffected, exactly
as EnforceSiteScope treats them. Every contract change is a trailing optional
parameter. Mutation-checked: disabling the gate fails 3 of the 5 new RouteHelper
tests and 1 of the runner tests, while the in-scope/unscoped positives still pass.

Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29
This commit is contained in:
Joseph Doherty
2026-09-06 14:13:32 -04:00
parent cccef7b2f4
commit 20a05941ee
5 changed files with 317 additions and 10 deletions
@@ -506,6 +506,23 @@ public class InboundScriptExecutor
_logger.LogDebug("Inbound API request for method {Method} cancelled by client", method.Name);
return new InboundScriptResult(false, null, "Request cancelled by client");
}
catch (RouteSiteScopeException ex)
{
// Arch-review F2: the script routed to a site outside the caller's site
// scope. RouteTarget.ResolveSiteAsync refused BEFORE any site traffic, so
// the script never saw a routed reply — there is nothing to roll back.
// The message is the operator-facing denial, safe to surface verbatim
// (it names only the instance and its site), unlike the generic
// SCRIPT_ERROR path which deliberately hides internals.
//
// Ordered before the SiteUnreachableException catch purely for
// readability: the two types are siblings under InvalidOperationException,
// so neither shadows the other.
_logger.LogWarning(
"API method {Method} refused a routed call outside the caller's site scope: {Message}",
method.Name, ex.Message);
return new InboundScriptResult(false, null, ex.Message, "SITE_SCOPE_DENIED");
}
catch (SiteUnreachableException)
{
// C3: a routed Route.To(...).Call(...) reached a site that could not be
@@ -36,6 +36,34 @@ public sealed class SiteUnreachableException : InvalidOperationException
public SiteUnreachableException(string message, Exception innerException) : base(message, innerException) { }
}
/// <summary>
/// Arch-review F2: a routed call whose target site is outside the caller's site
/// scope. Raised by <c>RouteTarget.ResolveSiteAsync</c> — the single choke point every routed
/// operation passes through — so it is thrown <b>before any site traffic</b>: nothing is sent,
/// nothing is read, nothing is written.
///
/// <para>
/// The scope exists for the Flow Debugger's method-entry Live run, where an operator picks a
/// method rather than a site: a site-scoped Deployer must not reach outside their sites just
/// because the method's script routes there. The ordinary HTTP inbound path attaches no scope
/// (an API key is not a site-scoped principal) and is unaffected.
/// </para>
///
/// <para>
/// Derives from <see cref="InvalidOperationException"/> so existing callers catching the base
/// type are unaffected, while <see cref="InboundScriptExecutor"/> catches this more specific
/// type to surface the <c>SITE_SCOPE_DENIED</c> error code instead of a generic
/// <c>SCRIPT_ERROR</c>. Deliberately NOT the ManagementService's <c>SiteScopeViolationException</c>
/// — InboundAPI cannot reference ManagementService; the runner converts the code back.
/// </para>
/// </summary>
public sealed class RouteSiteScopeException : InvalidOperationException
{
/// <summary>Initializes a new <see cref="RouteSiteScopeException"/> with the given message.</summary>
/// <param name="message">The scope-denial message naming the instance and its site.</param>
public RouteSiteScopeException(string message) : base(message) { }
}
/// <summary>
/// Route.To() helper for cross-site calls from inbound API scripts.
/// Resolves instance to site, routes via <see cref="IInstanceRouter"/>, blocks until
@@ -57,6 +85,7 @@ public class RouteHelper
private readonly INodeIdentityProvider? _nodeIdentity;
private readonly bool _trace;
private readonly Action<AuditEvent>? _traceSink;
private readonly IReadOnlySet<string>? _permittedSiteIdentifiers;
/// <summary>
/// Initializes a new <see cref="RouteHelper"/> with no deadline token and no parent execution id.
@@ -73,7 +102,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)
auditWriter, nodeIdentity, trace: false, traceSink: null, permittedSiteIdentifiers: null)
{
}
@@ -86,8 +115,10 @@ public class RouteHelper
ICentralAuditWriter? auditWriter,
INodeIdentityProvider? nodeIdentity,
bool trace,
Action<AuditEvent>? traceSink)
Action<AuditEvent>? traceSink,
IReadOnlySet<string>? permittedSiteIdentifiers)
{
_permittedSiteIdentifiers = permittedSiteIdentifiers;
_instanceLocator = instanceLocator;
_instanceRouter = instanceRouter;
_deadlineToken = deadlineToken;
@@ -110,7 +141,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);
_auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers);
/// <summary>
/// Returns a <see cref="RouteHelper"/> carrying the raw request-abort
@@ -124,7 +155,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);
_auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers);
/// <summary>
/// Audit Log (ParentExecutionId): returns a <see cref="RouteHelper"/> whose
@@ -139,7 +170,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);
_auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers);
/// <summary>
/// Flow Debugger (design 2026-09-05 §5.2): returns a <see cref="RouteHelper"/> whose routed
@@ -150,7 +181,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);
_auditWriter, _nodeIdentity, trace, _traceSink, _permittedSiteIdentifiers);
/// <summary>
/// Flow Debugger (design 2026-09-05 §7.1): returns a <see cref="RouteHelper"/> that also
@@ -162,7 +193,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);
_auditWriter, _nodeIdentity, _trace, traceSink, _permittedSiteIdentifiers);
/// <summary>
/// Flow Debugger outbound calls (plan 2026-09-06, D5/D7): the trace sink this helper carries,
@@ -172,6 +203,32 @@ public class RouteHelper
/// </summary>
public Action<AuditEvent>? TraceSink => _traceSink;
/// <summary>
/// Arch-review F2: returns a <see cref="RouteHelper"/> that refuses a routed call
/// whose target site is not in <paramref name="permittedSiteIdentifiers"/> — site
/// <em>identifiers</em> (e.g. <c>site-a</c>), the values <see cref="IInstanceLocator"/> returns,
/// NOT the numeric ids an <c>AuthenticatedUser.PermittedSiteIds</c> carries. Matching is
/// whatever comparer the supplied set uses, so pass an ordinal-ignore-case set.
///
/// <para>
/// <c>null</c> = unscoped, the default and the state of every ordinary inbound HTTP request.
/// An EMPTY set is not the same thing: it permits nothing, which is the correct outcome for
/// a site-scoped user whose sites no longer exist.
/// </para>
///
/// <para>
/// The Flow Debugger's method-entry Live run sets this from the operator's site-scoped
/// Deployer role, because the operator picks a <em>method</em> — the site is only known once
/// the script routes, so the check cannot happen up front the way it does for a value-change
/// run's named instance.
/// </para>
/// </summary>
/// <param name="permittedSiteIdentifiers">The site identifiers routed calls may reach; null = unscoped.</param>
/// <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);
/// <summary>
/// Creates a route target for the specified instance.
/// </summary>
@@ -181,7 +238,7 @@ public class RouteHelper
{
return new RouteTarget(
instanceCode, _instanceLocator, _instanceRouter, _deadlineToken, _requestAbortedToken, _parentExecutionId,
_auditWriter, _nodeIdentity, _trace, _traceSink);
_auditWriter, _nodeIdentity, _trace, _traceSink, _permittedSiteIdentifiers);
}
}
@@ -200,6 +257,7 @@ public class RouteTarget
private readonly INodeIdentityProvider? _nodeIdentity;
private readonly bool _trace;
private readonly Action<AuditEvent>? _traceSink;
private readonly IReadOnlySet<string>? _permittedSiteIdentifiers;
/// <summary>
/// Initializes a new <see cref="RouteTarget"/> for the specified instance.
@@ -210,6 +268,11 @@ public class RouteTarget
/// <param name="deadlineToken">Cancellation token representing the method-level deadline.</param>
/// <param name="requestAbortedToken">Raw client-disconnect token, independent of the method timeout.</param>
/// <param name="parentExecutionId">Optional parent execution id for audit correlation on routed calls.</param>
/// <param name="auditWriter">Central-direct-write audit writer for the always-on <c>RouteCall</c> row; null = no row.</param>
/// <param name="nodeIdentity">Stamps <c>SourceNode</c> on the <c>RouteCall</c> row; null = NULL.</param>
/// <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>
internal RouteTarget(
string instanceCode,
IInstanceLocator instanceLocator,
@@ -220,8 +283,10 @@ public class RouteTarget
ICentralAuditWriter? auditWriter = null,
INodeIdentityProvider? nodeIdentity = null,
bool trace = false,
Action<AuditEvent>? traceSink = null)
Action<AuditEvent>? traceSink = null,
IReadOnlySet<string>? permittedSiteIdentifiers = null)
{
_permittedSiteIdentifiers = permittedSiteIdentifiers;
_instanceCode = instanceCode;
_instanceLocator = instanceLocator;
_instanceRouter = instanceRouter;
@@ -549,6 +614,16 @@ public class RouteTarget
$"Instance '{_instanceCode}' not found or has no assigned site");
}
// Arch-review F2: the site scope gate. This method is the single choke point —
// Call, GetAttributes, WaitForAttribute and SetAttributes all resolve the site
// here first — so refusing at this point guarantees no site traffic of any kind
// is emitted for an out-of-scope instance.
if (_permittedSiteIdentifiers is not null && !_permittedSiteIdentifiers.Contains(siteId))
{
throw new RouteSiteScopeException(
$"Access denied: instance '{_instanceCode}' is at site '{siteId}', outside your site scope.");
}
return siteId;
}
@@ -38,6 +38,14 @@ public sealed class FlowDebugRunner
/// <summary>Error code when the method / instance / site does not exist.</summary>
public const string NotFoundCode = "NOT_FOUND";
/// <summary>
/// Arch-review F2: <see cref="InboundScriptExecutor"/>'s code for a routed call refused
/// because the target site is outside the operator's site scope. InboundAPI cannot
/// reference this assembly, so it raises its own <c>RouteSiteScopeException</c> and the
/// runner converts the code back to <see cref="SiteScopeViolationException"/>.
/// </summary>
public const string SiteScopeDeniedCode = "SITE_SCOPE_DENIED";
/// <summary>The <c>Extra.source</c> value stamped on rows this runner synthesizes.</summary>
public const string SourceTag = "flow-debugger";
@@ -103,7 +111,12 @@ public sealed class FlowDebugRunner
var sw = Stopwatch.StartNew();
var timeout = TimeSpan.FromSeconds(Math.Max(1, method.TimeoutSeconds));
var route = sp.GetRequiredService<RouteHelper>().WithTraceSink(events.Enqueue);
// Arch-review F2: a method Live run has no single target site to check up front the
// way RunValueChangeAsync does — the operator picks a METHOD, and where it routes is
// only known once the script runs. So the operator's scope is pushed into the
// RouteHelper and enforced at route time, before any site traffic.
var scope = await ResolvePermittedSiteIdentifiersAsync(sp, user, ct).ConfigureAwait(false);
var route = sp.GetRequiredService<RouteHelper>().WithTraceSink(events.Enqueue).WithSiteScope(scope);
var executor = sp.GetRequiredService<InboundScriptExecutor>();
InboundScriptResult result;
@@ -140,6 +153,16 @@ public sealed class FlowDebugRunner
extra: new ScriptTraceExtra { Source = SourceTag }.ToJson());
await WriteCentralAsync(sp, inboundRow, ct).ConfigureAwait(false);
// Arch-review F2: convert the executor's code into the same authorization failure
// RunValueChangeAsync raises, so both Live entry points surface one exception type
// (→ ManagementUnauthorized). Deliberately AFTER the inbound row is written: a
// denied attempt must still be audited against the operator's name.
if (string.Equals(result.ErrorCode, SiteScopeDeniedCode, StringComparison.Ordinal))
{
throw new SiteScopeViolationException(
result.ErrorMessage ?? "Access denied: routed call outside your site scope.");
}
var ordered = new List<AuditEvent>(events.Count + 1) { inboundRow };
ordered.AddRange(events.OrderBy(e => e.OccurredAtUtc).ThenBy(e => e.EventId));
@@ -301,6 +324,35 @@ public sealed class FlowDebugRunner
}
}
/// <summary>
/// Arch-review F2: the operator's <c>PermittedSiteIds</c> — NUMERIC site ids as strings —
/// resolved to the site IDENTIFIERS <see cref="IInstanceLocator"/> speaks, for
/// <c>RouteHelper.WithSiteScope</c>. Returns null (unscoped) for an unscoped user or an
/// Administrator, exactly as <see cref="EnforceSiteScope"/> treats them.
///
/// <para>
/// A returned set may be EMPTY — a scoped user whose sites no longer exist routes nowhere.
/// That is the intended fail-closed outcome, and is why null and empty are not the same.
/// </para>
/// </summary>
private static async Task<IReadOnlySet<string>?> ResolvePermittedSiteIdentifiersAsync(
IServiceProvider sp, AuthenticatedUser user, CancellationToken ct)
{
if (user.PermittedSiteIds.Length == 0) return null;
if (user.Roles.Contains(Roles.Administrator, StringComparer.OrdinalIgnoreCase)) return null;
var sites = sp.GetRequiredService<ISiteRepository>();
var set = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
foreach (var raw in user.PermittedSiteIds)
{
if (!int.TryParse(raw, out var id)) continue;
var site = await sites.GetSiteByIdAsync(id, ct).ConfigureAwait(false);
if (site is not null) set.Add(site.SiteIdentifier);
}
return set;
}
private async Task WriteCentralAsync(IServiceProvider sp, AuditEvent evt, CancellationToken ct)
{
var writer = sp.GetService<ICentralAuditWriter>();