diff --git a/docs/requirements/Component-InboundAPI.md b/docs/requirements/Component-InboundAPI.md index 26e1ab04..08976a49 100644 --- a/docs/requirements/Component-InboundAPI.md +++ b/docs/requirements/Component-InboundAPI.md @@ -189,7 +189,7 @@ Inbound API scripts **cannot** call shared scripts directly — shared scripts a - `Route.To("instanceUniqueCode").GetAttributes("attr1", "attr2", ...)` — Read multiple attribute values in a **single call**, returned as a dictionary of name-value pairs. - `Route.To("instanceUniqueCode").SetAttribute("attributeName", value)` — Write a single attribute value on a specific instance at any site. - `Route.To("instanceUniqueCode").SetAttributes(dictionary)` — Write multiple attribute values in a **single call**, accepting a dictionary of name-value pairs. -- `Route.To("instanceUniqueCode").WaitForAttribute("attributeName", targetValue, timeout)` — Wait, event-driven, until an attribute on a specific instance at any site reaches `targetValue` (value-equality only across the wire), bounded by `timeout`. Returns `true` if matched within the timeout, `false` if it timed out. The cluster call is bounded by the wait timeout rather than the generic integration timeout. +- `Route.To("instanceUniqueCode").WaitForAttribute("attributeName", targetValue, timeout)` — Wait, event-driven, until an attribute on a specific instance at any site reaches `targetValue` (value-equality only across the wire), bounded by `timeout`. Returns `true` if matched within the timeout, `false` if it timed out. **The wait is bounded by its own `timeout`, not the generic method-level timeout** — this is the one routed call that may legitimately outlive the method timeout (the site enforces `timeout` and returns `false` when it elapses). A client disconnect still cancels the wait. This is the deliberate exception to the rule below that routed calls inherit the method-level timeout (see "Routing Behavior"): a long event-driven wait is the explicit reason `timeout` governs here. #### Input/Output - **Input parameters** are available as defined in the method definition. @@ -199,20 +199,38 @@ Inbound API scripts **cannot** call shared scripts directly — shared scripts a - `Parameters["key"]` — Raw dictionary access. - `Parameters.Get("key")` — Typed access (same API as site runtime scripts). See Site Runtime component for full type support. -> **No direct database access.** Inbound API scripts are not given a raw database -> client. Handing a script a raw `SqlConnection` is in direct tension with the -> ScadaBridge script trust model (scripts are forbidden `System.IO`, `Process`, -> `Threading`, `Reflection`, and raw network access). The `ForbiddenApiChecker` -> statically enforces this by delegating to the shared `ScriptAnalysis` -> `ScriptTrustValidator` (component #25), which is the single authoritative -> source of truth for the forbidden-API policy. The unified policy permits -> `System.Diagnostics.Stopwatch`/`Debug` while retaining the `Process`-only ban, -> and adds reflection-gateway member and `dynamic`/`Activator` hardening. -> This is defence-in-depth static enforcement, not a true runtime sandbox. Scripts -> interact with the system only through the curated `Route` and `Parameters` -> surfaces above. If a method needs data from the configuration or machine-data -> databases, that access belongs behind a dedicated, scoped helper — not a -> general-purpose connection — and would be added here as an explicit design change. +#### Database Access + +Inbound API scripts may read from and write to the configuration / machine-data +databases through the **curated, scoped `Database` helper** — `InboundDatabaseHelper`, +exposed as `InboundScriptContext.Database`. This is the "dedicated, scoped helper added +as an explicit design change" that the script trust model requires: scripts are **never** +handed a raw `SqlConnection`, and never reference `System.Data` (the `ForbiddenApiChecker`, +delegating to the shared `ScriptAnalysis` `ScriptTrustValidator` (#25), still statically +bans `System.IO`, `Process`, `Threading`, `Reflection`, and raw network access — that is +defence-in-depth static enforcement, not a true runtime sandbox). + +The helper API (all asynchronous — scripts `await` them; bounded by the method timeout): + +- `await Database.QuerySingleAsync("connectionName", sql, parameters)` — first column of the first row as `T` (default if no rows). +- `await Database.QueryAsync("connectionName", sql, parameters)` — all rows as case-insensitive column→value dictionaries. +- `await Database.ExecuteAsync("connectionName", sql, parameters)` — run a write (INSERT/UPDATE/DELETE/DDL); returns rows affected. **Writes are permitted** — the move-in integration records results, not just reads them. + +Containment rules (enforced, not advisory): + +- **Named connections only.** `connectionName` selects one of the connections configured + on the central database gateway (`IDatabaseGateway`); a script cannot supply an arbitrary + connection string or reach a database the gateway is not configured for. +- **SQL-injection protection.** Statement text is authored by the (design-time) method + script, but every request-derived **value** is passed via `parameters` and bound as a + named `@`-prefixed SQL parameter — never string-concatenated into the command text. + Request input therefore reaches the database only through parameter binding. +- **Deadline-bound.** Calls use the async ADO.NET path end-to-end (no pool-thread blocking) + and honour the method deadline token, with a `CommandTimeout` backstop derived from the + method timeout, so a slow query is bounded by the method timeout. + +For everything else, scripts interact with the system through the curated `Route` and +`Parameters` surfaces above. ### Routing Behavior - The `Route.To()` helper resolves the instance's site assignment from the configuration database and routes the request to the correct site cluster via the Communication Layer. diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/InboundScriptHost.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/InboundScriptHost.cs index 607f42bb..689b5041 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/InboundScriptHost.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/InboundScriptHost.cs @@ -20,11 +20,52 @@ public class InboundScriptHost /// public RouteHelper Route { get; } = new(); + /// + /// Scoped, parameterized database access. Editor mirror of + /// ZB.MOM.WW.ScadaBridge.InboundAPI.InboundDatabaseHelper (InboundAPI-026/030) — its + /// signatures must match the runtime helper so a script using Database.* + /// type-checks during analysis. + /// + public DatabaseAccessor Database { get; } = new(); + /// /// The cancellation token for the operation. /// public System.Threading.CancellationToken CancellationToken { get; } + /// Editor mirror of ZB.MOM.WW.ScadaBridge.InboundAPI.InboundDatabaseHelper. + public class DatabaseAccessor + { + /// First column of the first row as (default if no rows). + /// The scalar result type. + /// A configured database connection name. + /// The SQL statement; values supplied via . + /// Optional anonymous object of bound parameters. + /// A task that resolves to the scalar, or default. + public System.Threading.Tasks.Task QuerySingleAsync( + string connectionName, string sql, object? parameters = null) => + System.Threading.Tasks.Task.FromResult(default); + + /// All rows as case-insensitive column→value dictionaries. + /// A configured database connection name. + /// The SQL statement; values supplied via . + /// Optional anonymous object of bound parameters. + /// A task that resolves to the result rows. + public System.Threading.Tasks.Task>> QueryAsync( + string connectionName, string sql, object? parameters = null) => + System.Threading.Tasks.Task.FromResult>>( + new List>()); + + /// Runs a write statement; returns rows affected. + /// A configured database connection name. + /// The SQL statement; values supplied via . + /// Optional anonymous object of bound parameters. + /// A task that resolves to the number of rows affected. + public System.Threading.Tasks.Task ExecuteAsync( + string connectionName, string sql, object? parameters = null) => + System.Threading.Tasks.Task.FromResult(0); + } + /// Editor mirror of ZB.MOM.WW.ScadaBridge.InboundAPI.RouteHelper. public class RouteHelper { diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/SandboxInboundScriptHost.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/SandboxInboundScriptHost.cs index 2c155945..238d54eb 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/SandboxInboundScriptHost.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/SandboxInboundScriptHost.cs @@ -23,6 +23,48 @@ public class SandboxInboundScriptHost /// Gets the route accessor; every call throws in a test run. public RouteAccessor Route { get; } = new(); + /// Gets the database accessor; every call throws in a test run. + public DatabaseAccessor Database { get; } = new(); + + /// + /// Mirror of ZB.MOM.WW.ScadaBridge.InboundAPI.InboundDatabaseHelper — signatures match + /// the runtime helper so the same user code compiles, but every call throws because a + /// central Test Run has no configured database connection to reach (mirrors how + /// throws on cross-site routing). + /// + public class DatabaseAccessor + { + /// Always throws ; database access is unavailable in a Test Run. + /// The scalar result type. + /// Connection name (included in the exception message). + /// Unused SQL. + /// Unused parameters. + /// Never returns; always throws . + public Task QuerySingleAsync(string connectionName, string sql, object? parameters = null) => + throw Unavailable($"Database.QuerySingleAsync(\"{connectionName}\")"); + + /// Always throws ; database access is unavailable in a Test Run. + /// Connection name (included in the exception message). + /// Unused SQL. + /// Unused parameters. + /// Never returns; always throws . + public Task>> QueryAsync( + string connectionName, string sql, object? parameters = null) => + throw Unavailable($"Database.QueryAsync(\"{connectionName}\")"); + + /// Always throws ; database access is unavailable in a Test Run. + /// Connection name (included in the exception message). + /// Unused SQL. + /// Unused parameters. + /// Never returns; always throws . + public Task ExecuteAsync(string connectionName, string sql, object? parameters = null) => + throw Unavailable($"Database.ExecuteAsync(\"{connectionName}\")"); + + private static ScriptSandboxException Unavailable(string operation) => + new($"{operation} is not available in Test Run — database access needs the " + + "central configuration / machine-data databases."); + } + /// Mirror of ZB.MOM.WW.ScadaBridge.InboundAPI.RouteHelper. public class RouteAccessor { diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundDatabaseHelper.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundDatabaseHelper.cs index 2cb6b53b..eef170f8 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundDatabaseHelper.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundDatabaseHelper.cs @@ -5,45 +5,90 @@ using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; namespace ZB.MOM.WW.ScadaBridge.InboundAPI; /// -/// Read-only database access exposed to inbound API scripts. All ADO.NET stays -/// internal here — scripts call QuerySingle/Query by name and never reference -/// System.Data. Named connections only; parameters are bound (anonymous-object -/// properties become @-prefixed SQL parameters), never string-concatenated. +/// Scoped, parameterized database access exposed to inbound API scripts as +/// InboundScriptContext.Database. This is the dedicated, curated data-access +/// helper authorized by the design doc (Component-InboundAPI.md, "Database access") — +/// not a raw connection handed to the script. All ADO.NET stays internal here: scripts +/// call / / +/// by name and never reference System.Data. +/// +/// +/// SQL-injection protection (InboundAPI-026). Statement text is authored by the +/// (design-time) method script, but every value is bound as a named SQL +/// parameter (anonymous-object properties become @-prefixed parameters via +/// ) and is NEVER string-concatenated into the command text. +/// Request-derived values therefore reach the database only through parameter binding, +/// closing the injection vector. Connection access is restricted to the named +/// connections configured on the central — a script +/// cannot supply an arbitrary connection string. +/// +/// +/// +/// Reads and writes are both permitted (InboundAPI-026 design decision): the move-in +/// integration needs to record results, not just read them. Use +/// / for reads and for writes. +/// +/// +/// +/// Async + deadline-bound (InboundAPI-027). Every call uses the async ADO.NET path +/// end-to-end (no .GetAwaiter().GetResult() blocking a pool thread) and honours the +/// executing method's deadline token on the command itself, with a +/// backstop derived from the method timeout — so a slow query is bounded by the method +/// timeout instead of running unbounded. +/// /// public sealed class InboundDatabaseHelper { private readonly IDatabaseGateway? _gateway; private readonly CancellationToken _ct; + private readonly int _commandTimeoutSeconds; - public InboundDatabaseHelper(IDatabaseGateway? gateway, CancellationToken ct) - { _gateway = gateway; _ct = ct; } + /// + /// Initializes the helper. + /// + /// The central database gateway, or null when no gateway is registered (the helper then throws on first use). + /// The executing method's deadline token; forwarded to every async DB call so a slow query is bounded by the method timeout. + /// The method timeout; used to derive a backstop. (the default) leaves the provider default. + public InboundDatabaseHelper(IDatabaseGateway? gateway, CancellationToken ct, TimeSpan commandTimeout = default) + { + _gateway = gateway; + _ct = ct; + _commandTimeoutSeconds = commandTimeout > TimeSpan.Zero + ? (int)Math.Ceiling(commandTimeout.TotalSeconds) + : 0; + } - /// First column of the first row converted to T (default if no rows). - public T? QuerySingle(string connectionName, string sql, object? parameters = null) + /// First column of the first row converted to (default if no rows). + /// The type to convert the scalar result to. + /// Name of a connection configured on the central database gateway. + /// The SQL statement. Values MUST be supplied via , not concatenated. + /// Optional anonymous object whose properties become bound @-prefixed parameters. + /// The converted scalar, or default when there are no rows / a NULL value. + public async Task QuerySingleAsync(string connectionName, string sql, object? parameters = null) { if (_gateway is null) throw new InvalidOperationException("Database is not available for this inbound method"); - using var conn = _gateway.GetConnectionAsync(connectionName, _ct).GetAwaiter().GetResult(); - using var cmd = conn.CreateCommand(); - cmd.CommandText = sql; - AddParameters(cmd, parameters); - var result = cmd.ExecuteScalar(); + await using var conn = await _gateway.GetConnectionAsync(connectionName, _ct); + await using var cmd = CreateCommand(conn, sql, parameters); + var result = await cmd.ExecuteScalarAsync(_ct); if (result is null or DBNull) return default; if (result is T t) return t; return (T)Convert.ChangeType(result, typeof(T), CultureInfo.InvariantCulture); } /// All rows as column→value dictionaries (case-insensitive keys). - public IReadOnlyList> Query( + /// Name of a connection configured on the central database gateway. + /// The SQL statement. Values MUST be supplied via , not concatenated. + /// Optional anonymous object whose properties become bound @-prefixed parameters. + /// The result rows; empty when nothing matches. + public async Task>> QueryAsync( string connectionName, string sql, object? parameters = null) { if (_gateway is null) throw new InvalidOperationException("Database is not available for this inbound method"); - using var conn = _gateway.GetConnectionAsync(connectionName, _ct).GetAwaiter().GetResult(); - using var cmd = conn.CreateCommand(); - cmd.CommandText = sql; - AddParameters(cmd, parameters); - using var reader = cmd.ExecuteReader(); + await using var conn = await _gateway.GetConnectionAsync(connectionName, _ct); + await using var cmd = CreateCommand(conn, sql, parameters); + await using var reader = await cmd.ExecuteReaderAsync(_ct); var rows = new List>(); - while (reader.Read()) + while (await reader.ReadAsync(_ct)) { var row = new Dictionary(StringComparer.OrdinalIgnoreCase); for (var i = 0; i < reader.FieldCount; i++) @@ -56,6 +101,35 @@ public sealed class InboundDatabaseHelper return rows; } + /// + /// Executes a write statement (INSERT/UPDATE/DELETE/DDL) and returns the number of + /// rows affected. Writes are authorized for inbound API scripts (InboundAPI-026); + /// values are still bound as parameters, never concatenated. + /// + /// Name of a connection configured on the central database gateway. + /// The SQL statement. Values MUST be supplied via , not concatenated. + /// Optional anonymous object whose properties become bound @-prefixed parameters. + /// The number of rows affected. + public async Task ExecuteAsync(string connectionName, string sql, object? parameters = null) + { + if (_gateway is null) throw new InvalidOperationException("Database is not available for this inbound method"); + await using var conn = await _gateway.GetConnectionAsync(connectionName, _ct); + await using var cmd = CreateCommand(conn, sql, parameters); + return await cmd.ExecuteNonQueryAsync(_ct); + } + + private DbCommand CreateCommand(DbConnection conn, string sql, object? parameters) + { + var cmd = conn.CreateCommand(); + cmd.CommandText = sql; + // InboundAPI-027: a CommandTimeout backstop derived from the method timeout so a + // slow query cannot outrun the method deadline even if the provider does not + // honour token cancellation mid-statement. + if (_commandTimeoutSeconds > 0) cmd.CommandTimeout = _commandTimeoutSeconds; + AddParameters(cmd, parameters); + return cmd; + } + private static void AddParameters(DbCommand cmd, object? parameters) { if (parameters is null) return; diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs index b863ac16..9ef6078a 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs @@ -237,12 +237,13 @@ public class InboundScriptExecutor using var cts = CancellationTokenSource.CreateLinkedTokenSource( cancellationToken, timeoutCts.Token); - // IpsenMES MoveIn: expose a read-only Database helper to the script. - // Resolve the gateway from a fresh DI scope (declared outside the try so it - // lives until after the script handler runs and is disposed here) so a scoped - // IDatabaseGateway is honoured. GetService (not GetRequiredService) so a method - // that never touches Database still runs even when no IDatabaseGateway is - // registered; the helper throws on first use if built without a gateway. + // IpsenMES MoveIn: expose the scoped, parameterized Database helper to the script + // (reads + writes — InboundAPI-026). Resolve the gateway from a fresh DI scope + // (declared outside the try so it lives until after the script handler runs and is + // disposed here) so a scoped IDatabaseGateway is honoured. GetService (not + // GetRequiredService) so a method that never touches Database still runs even when + // no IDatabaseGateway is registered; the helper throws on first use if built + // without a gateway. // // A service provider that cannot produce a scope (e.g. a bare test double with // no IServiceScopeFactory) is tolerated: the gateway is simply unavailable, so @@ -265,18 +266,28 @@ public class InboundScriptExecutor try { var gateway = scope?.ServiceProvider.GetService(); - var dbHelper = new InboundDatabaseHelper(gateway, cts.Token); + // InboundAPI-027: pass the method timeout so the helper derives a + // CommandTimeout backstop and forwards cts.Token to every async DB call — + // a slow query is then bounded by the method deadline. + var dbHelper = new InboundDatabaseHelper(gateway, cts.Token, timeout); // InboundAPI-016: bind the route helper to the method deadline so a // routed Route.To(...).Call(...) inherits the method-level timeout // without the script having to thread the context token by hand. // + // InboundAPI-029: also pass the raw request-abort token separately so + // Route.To(...).WaitForAttribute(...) can be bounded by its WAIT timeout + // (not the generic method deadline) while still being cancelled by a + // client disconnect — see RouteTarget.WaitForAttribute. + // // Audit Log #23 (ParentExecutionId): also bind the inbound request's // ExecutionId so a routed call carries it as ParentExecutionId — the // spawned site script execution points back at this inbound request. var context = new InboundScriptContext( parameters, - route.WithDeadline(cts.Token).WithParentExecutionId(parentExecutionId), + route.WithDeadline(cts.Token) + .WithRequestAborted(cancellationToken) + .WithParentExecutionId(parentExecutionId), dbHelper, cts.Token); @@ -382,7 +393,8 @@ public class InboundScriptContext public RouteHelper Route { get; } /// - /// Read-only database access for the script (named connections; bound parameters). + /// Scoped, parameterized database access for the script (named connections; bound + /// parameters; reads and writes — see ). /// public InboundDatabaseHelper Database { get; } @@ -396,7 +408,7 @@ public class InboundScriptContext /// /// The input parameters for the script. /// The route helper for cross-site routing. - /// The read-only database helper for the script. + /// The scoped, parameterized database helper for the script. /// The cancellation token. public InboundScriptContext( IReadOnlyDictionary parameters, diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs index b335f232..0354e3ae 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs @@ -19,6 +19,7 @@ public class RouteHelper private readonly IInstanceLocator _instanceLocator; private readonly IInstanceRouter _instanceRouter; private readonly CancellationToken _deadlineToken; + private readonly CancellationToken _requestAbortedToken; private readonly Guid? _parentExecutionId; /// @@ -29,7 +30,7 @@ public class RouteHelper public RouteHelper( IInstanceLocator instanceLocator, IInstanceRouter instanceRouter) - : this(instanceLocator, instanceRouter, CancellationToken.None, parentExecutionId: null) + : this(instanceLocator, instanceRouter, CancellationToken.None, CancellationToken.None, parentExecutionId: null) { } @@ -37,11 +38,13 @@ public class RouteHelper IInstanceLocator instanceLocator, IInstanceRouter instanceRouter, CancellationToken deadlineToken, + CancellationToken requestAbortedToken, Guid? parentExecutionId) { _instanceLocator = instanceLocator; _instanceRouter = instanceRouter; _deadlineToken = deadlineToken; + _requestAbortedToken = requestAbortedToken; _parentExecutionId = parentExecutionId; } @@ -55,7 +58,20 @@ public class RouteHelper /// The executing method's timeout cancellation token to inherit for routed calls. /// A new inheriting the given deadline token. public RouteHelper WithDeadline(CancellationToken deadlineToken) => - new(_instanceLocator, _instanceRouter, deadlineToken, _parentExecutionId); + new(_instanceLocator, _instanceRouter, deadlineToken, _requestAbortedToken, _parentExecutionId); + + /// + /// InboundAPI-029: returns a carrying the raw request-abort + /// token (a client disconnect) separately from the method deadline. Most + /// routed calls remain bounded by the method deadline (which already incorporates the + /// abort), but uses this token so its wait + /// is bounded by the WAIT timeout — not the generic method deadline (spec §6) — while + /// still being cancelled by a client disconnect. + /// + /// The raw client-disconnect token, independent of the method timeout. + /// A new carrying the given request-abort token. + public RouteHelper WithRequestAborted(CancellationToken requestAbortedToken) => + new(_instanceLocator, _instanceRouter, _deadlineToken, requestAbortedToken, _parentExecutionId); /// /// Audit Log #23 (ParentExecutionId): returns a whose @@ -69,7 +85,7 @@ public class RouteHelper /// The inbound request's execution id to stamp as the spawning parent on routed calls, or null for non-routed runs. /// A new carrying the given parent execution id. public RouteHelper WithParentExecutionId(Guid? parentExecutionId) => - new(_instanceLocator, _instanceRouter, _deadlineToken, parentExecutionId); + new(_instanceLocator, _instanceRouter, _deadlineToken, _requestAbortedToken, parentExecutionId); /// /// Creates a route target for the specified instance. @@ -79,7 +95,7 @@ public class RouteHelper public RouteTarget To(string instanceCode) { return new RouteTarget( - instanceCode, _instanceLocator, _instanceRouter, _deadlineToken, _parentExecutionId); + instanceCode, _instanceLocator, _instanceRouter, _deadlineToken, _requestAbortedToken, _parentExecutionId); } } @@ -88,10 +104,18 @@ public class RouteHelper /// public class RouteTarget { + // InboundAPI-029: a small grace past the wait timeout. The SITE enforces the wait + // timeout and returns Matched=false when it elapses; the local backstop fires only + // if the site fails to respond, so it must sit slightly LATER than the wait timeout + // (it must not pre-empt the site's own timed-out response and turn a clean `false` + // into a cancellation). + private static readonly TimeSpan WaitResponseGrace = TimeSpan.FromSeconds(5); + private readonly string _instanceCode; private readonly IInstanceLocator _instanceLocator; private readonly IInstanceRouter _instanceRouter; private readonly CancellationToken _deadlineToken; + private readonly CancellationToken _requestAbortedToken; private readonly Guid? _parentExecutionId; /// @@ -101,18 +125,21 @@ public class RouteTarget /// Service to resolve the site id for the instance. /// Service to route cross-site calls. /// Cancellation token representing the method-level deadline. + /// Raw client-disconnect token, independent of the method timeout (InboundAPI-029). /// Optional parent execution id for audit correlation on routed calls. internal RouteTarget( string instanceCode, IInstanceLocator instanceLocator, IInstanceRouter instanceRouter, CancellationToken deadlineToken, + CancellationToken requestAbortedToken, Guid? parentExecutionId) { _instanceCode = instanceCode; _instanceLocator = instanceLocator; _instanceRouter = instanceRouter; _deadlineToken = deadlineToken; + _requestAbortedToken = requestAbortedToken; _parentExecutionId = parentExecutionId; } @@ -211,11 +238,22 @@ public class RouteTarget /// wire: the target is canonically encoded via and /// the site evaluates equality — there is no predicate and no quality flag in the /// comparison. + /// + /// + /// InboundAPI-029: unlike the other routed calls (which inherit the method deadline + /// per InboundAPI-016), the wait is bounded by — the WAIT + /// timeout — NOT the generic method deadline. The SITE enforces + /// and returns Matched=false when it elapses; the local token is built from a + /// per-wait CTS (the wait timeout plus a small grace), an explicit caller token, and + /// the client-disconnect token, but deliberately EXCLUDES the method deadline. So a + /// wait longer than the method timeout runs to its full wait timeout, as spec §6 + /// requires, while a client disconnect still cancels it. + /// /// /// Name of the attribute to wait on. /// Target value the attribute must equal for the wait to match. - /// Maximum time to wait for the attribute to reach the target value. - /// Optional cancellation token; defaults to the method deadline. + /// Maximum time to wait for the attribute to reach the target value; this — not the method deadline — bounds the wait. + /// Optional explicit cancellation token (a tighter caller-supplied bound); the wait is otherwise bounded by . /// A task that resolves to true if the attribute reached the target value, false if the wait timed out. public async Task WaitForAttribute( string attributeName, @@ -223,7 +261,12 @@ public class RouteTarget TimeSpan timeout, CancellationToken cancellationToken = default) { - var token = Effective(cancellationToken); + // InboundAPI-029: bound the wait by the WAIT timeout (+ grace backstop), the + // client-disconnect token, and an explicit caller token — NOT the method deadline. + using var waitCts = new CancellationTokenSource(timeout + WaitResponseGrace); + using var linked = CancellationTokenSource.CreateLinkedTokenSource( + waitCts.Token, _requestAbortedToken, cancellationToken); + var token = linked.Token; var siteId = await ResolveSiteAsync(token); // Audit Log #23 (ParentExecutionId): mirrors the Call path — stamp the diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs index a044e39a..f8a93e04 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs @@ -660,4 +660,21 @@ public class ScriptAnalysisServiceTests Assert.DoesNotContain(resp.Markers, m => m.Code.StartsWith("CS")); Assert.DoesNotContain(resp.Markers, m => m.Code.StartsWith("SCADA")); } + + [Fact] + public void InboundScript_Database_DiagnosesClean() + { + // InboundAPI-026/030: the scoped Database helper is a shipped inbound-script + // capability. The editor must not flag its async read/write API as an error — + // the InboundScriptHost mirror must expose QuerySingleAsync/QueryAsync/ExecuteAsync + // with signatures matching the runtime helper so Roslyn resolves them. + var resp = _svc.Diagnose(new DiagnoseRequest( + Code: "var code = await Database.QuerySingleAsync(\"BTDB\", \"SELECT Code FROM Machine WHERE SAPID=@s\", new { s = (string)Parameters[\"sap\"] });\n" + + "var rows = await Database.QueryAsync(\"BTDB\", \"SELECT Code FROM Machine\");\n" + + "var n = await Database.ExecuteAsync(\"BTDB\", \"UPDATE Machine SET Code=@c WHERE SAPID=@s\", new { c = code, s = \"131453\" });", + Kind: ScriptKind.InboundApi)); + + Assert.DoesNotContain(resp.Markers, m => m.Code.StartsWith("CS")); + Assert.DoesNotContain(resp.Markers, m => m.Code.StartsWith("SCADA")); + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointExtensionsTests.cs b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointExtensionsTests.cs index 4a9f792c..a34fe3cd 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointExtensionsTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointExtensionsTests.cs @@ -16,8 +16,12 @@ using ZB.MOM.WW.Auth.ApiKeys.Sqlite; using ZB.MOM.WW.ScadaBridge.Commons.Entities.InboundApi; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.InboundApi; using ZB.MOM.WW.ScadaBridge.Commons.Observability; +using ZB.MOM.WW.ScadaBridge.Commons.Types; using ZB.MOM.WW.ScadaBridge.InboundAPI.Middleware; +using Microsoft.Data.Sqlite; +using System.Data.Common; using System.Diagnostics.Metrics; using System.Net; using System.Net.Http.Headers; @@ -548,6 +552,102 @@ public sealed class EndpointExtensionsTests : IDisposable public void Dispose() => _listener.Dispose(); } + // --- InboundAPI-028: drive the new script-facing capabilities (Database + + // WaitForAttribute) end-to-end through the real POST /api/{methodName} flow, + // so a wiring regression in executor→context→helper / RouteHelper is caught. --- + + [Fact] + public async Task Script_UsingDatabase_RunsEndToEndThroughEndpoint() + { + // A method whose script reads through Database.QueryAsync — proving the executor + // builds the Database helper, resolves IDatabaseGateway from the per-execution + // DI scope, and the parameterized read reaches the body. (InboundAPI-028) + var method = SeedMethod(1, "movein", + "return await Database.QuerySingleAsync(\"BTDB\", \"SELECT Code FROM Machine WHERE SAPID=@s\", new { s = (string)Parameters[\"sap\"] });", + """[{"name":"sap","type":"String","required":true}]"""); + + using var host = await BuildHostAsync(method, additionalServices: services => + { + services.AddSingleton(SeededSqliteGateway()); + }); + var token = await SeedKeyAsync(host, keyId: "key1", displayName: "movein-caller", + scopes: new[] { "movein" }); + var client = host.GetTestClient(); + + var response = await client.SendAsync(BuildPost("movein", """{"sap":"131453"}""", token)); + var body = await response.Content.ReadAsStringAsync(); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Contains("Z28061A", body); + } + + [Fact] + public async Task Script_UsingWaitForAttribute_RunsEndToEndThroughEndpoint() + { + // A method whose script awaits Route.To(...).WaitForAttribute(...) — proving the + // executor binds the RouteHelper and the wait routes through IInstanceRouter and + // returns its result through the endpoint. (InboundAPI-028) + var method = SeedMethod(2, "waitmethod", + "return await Route.To(\"inst-1\").WaitForAttribute(\"Flag\", true, System.TimeSpan.FromSeconds(1));"); + + var locator = Substitute.For(); + locator.GetSiteIdForInstanceAsync("inst-1", Arg.Any()).Returns("SiteA"); + var router = Substitute.For(); + router.RouteToWaitForAttributeAsync("SiteA", Arg.Any(), Arg.Any()) + .Returns(ci => new RouteToWaitForAttributeResponse( + ((RouteToWaitForAttributeRequest)ci[1]).CorrelationId, + Matched: true, Value: true, Quality: "Good", TimedOut: false, + Success: true, ErrorMessage: null, DateTimeOffset.UtcNow)); + + using var host = await BuildHostAsync(method, additionalServices: services => + { + services.RemoveAll(); + services.AddSingleton(locator); + services.RemoveAll(); + services.AddSingleton(router); + }); + var token = await SeedKeyAsync(host, keyId: "key1", displayName: "wait-caller", + scopes: new[] { "waitmethod" }); + var client = host.GetTestClient(); + + var response = await client.SendAsync(BuildPost("waitmethod", "{}", token)); + var body = await response.Content.ReadAsStringAsync(); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Contains("true", body); + } + + // SQLite-backed IDatabaseGateway fake. Owns its keep-alive connection so the shared + // in-memory db lives for the gateway's (= host's) lifetime — NOT a GC-eligible local, + // which could be collected mid-suite (under GC pressure + SqliteConnection.ClearAllPools + // in Dispose) and drop the db before the request opens its own connection. A unique db + // name per instance also keeps tests from contaminating each other. Seeded with + // Machine(Code,SAPID)=('Z28061A','131453'). + private sealed class SqliteGateway : IDatabaseGateway, IDisposable + { + private readonly string _cs; + private readonly SqliteConnection _keepAlive; + public SqliteGateway(string cs) + { + _cs = cs; + _keepAlive = new SqliteConnection(cs); + _keepAlive.Open(); + using var cmd = _keepAlive.CreateCommand(); + cmd.CommandText = "CREATE TABLE IF NOT EXISTS Machine(Code TEXT, SAPID TEXT); DELETE FROM Machine; INSERT INTO Machine VALUES('Z28061A','131453');"; + cmd.ExecuteNonQuery(); + } + public async Task GetConnectionAsync(string name, CancellationToken ct = default) + { var c = new SqliteConnection(_cs); await c.OpenAsync(ct); return c; } + public Task CachedWriteAsync(string c, string s, + IReadOnlyDictionary? p = null, string? o = null, CancellationToken ct = default, + TrackedOperationId? t = null, Guid? e = null, string? src = null, Guid? pe = null) + => throw new NotImplementedException(); + public void Dispose() => _keepAlive.Dispose(); + } + + private static SqliteGateway SeededSqliteGateway() => + new($"DataSource=file:movein-endpoint-{Guid.NewGuid():N}?mode=memory&cache=shared"); + private static HttpRequestMessage BuildPost(string methodName, string body, string bearerToken) { var request = new HttpRequestMessage(HttpMethod.Post, "/api/" + methodName) diff --git a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundDatabaseHelperTests.cs b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundDatabaseHelperTests.cs index 4a54a9bd..47508bbf 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundDatabaseHelperTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundDatabaseHelperTests.cs @@ -12,46 +12,51 @@ public class InboundDatabaseHelperTests private sealed class SqliteGateway : IDatabaseGateway { private readonly string _cs; - public SqliteGateway(string cs) => _cs = cs; + // InboundAPI-027 test seam: when false the open ignores the passed token, so a + // pre-cancelled token surfaces at Execute/Read (not at open) — letting a test + // prove the EXECUTE path honours the deadline token, not just the open. + private readonly bool _honorTokenOnOpen; + public SqliteGateway(string cs, bool honorTokenOnOpen = true) + { _cs = cs; _honorTokenOnOpen = honorTokenOnOpen; } public async Task GetConnectionAsync(string name, CancellationToken ct = default) - { var c = new SqliteConnection(_cs); await c.OpenAsync(ct); return c; } + { var c = new SqliteConnection(_cs); await c.OpenAsync(_honorTokenOnOpen ? ct : CancellationToken.None); return c; } public Task CachedWriteAsync(string c, string s, IReadOnlyDictionary? p = null, string? o = null, CancellationToken ct = default, TrackedOperationId? t = null, Guid? e = null, string? src = null, Guid? pe = null) => throw new NotImplementedException(); } - private static SqliteGateway SeededGateway() + private static SqliteGateway SeededGateway(bool honorTokenOnOpen = true) { var keep = new SqliteConnection("DataSource=file:movein?mode=memory&cache=shared"); keep.Open(); // keep-alive: shared in-memory db lives until process exit using var cmd = keep.CreateCommand(); cmd.CommandText = "CREATE TABLE IF NOT EXISTS Machine(Code TEXT, SAPID TEXT); DELETE FROM Machine; INSERT INTO Machine VALUES('Z28061A','131453');"; cmd.ExecuteNonQuery(); - return new SqliteGateway("DataSource=file:movein?mode=memory&cache=shared"); + return new SqliteGateway("DataSource=file:movein?mode=memory&cache=shared", honorTokenOnOpen); } [Fact] - public void QuerySingle_returns_first_column_with_bound_parameter() + public async Task QuerySingleAsync_returns_first_column_with_bound_parameter() { var helper = new InboundDatabaseHelper(SeededGateway(), CancellationToken.None); - var code = helper.QuerySingle("BTDB", "SELECT Code FROM Machine WHERE SAPID=@s", new { s = "131453" }); + var code = await helper.QuerySingleAsync("BTDB", "SELECT Code FROM Machine WHERE SAPID=@s", new { s = "131453" }); Assert.Equal("Z28061A", code); } [Fact] - public void QuerySingle_returns_default_when_no_rows() + public async Task QuerySingleAsync_returns_default_when_no_rows() { var helper = new InboundDatabaseHelper(SeededGateway(), CancellationToken.None); - var code = helper.QuerySingle("BTDB", "SELECT Code FROM Machine WHERE SAPID=@s", new { s = "999999" }); + var code = await helper.QuerySingleAsync("BTDB", "SELECT Code FROM Machine WHERE SAPID=@s", new { s = "999999" }); Assert.Null(code); } [Fact] - public void Query_returns_row_with_case_insensitive_keys() + public async Task QueryAsync_returns_row_with_case_insensitive_keys() { var helper = new InboundDatabaseHelper(SeededGateway(), CancellationToken.None); - var rows = helper.Query("BTDB", "SELECT Code, SAPID FROM Machine WHERE SAPID=@s", new { s = "131453" }); + var rows = await helper.QueryAsync("BTDB", "SELECT Code, SAPID FROM Machine WHERE SAPID=@s", new { s = "131453" }); Assert.Single(rows); var row = rows[0]; Assert.Equal("Z28061A", row["code"]); @@ -59,10 +64,82 @@ public class InboundDatabaseHelperTests } [Fact] - public void Query_returns_empty_list_when_no_rows_match() + public async Task QueryAsync_returns_empty_list_when_no_rows_match() { var helper = new InboundDatabaseHelper(SeededGateway(), CancellationToken.None); - var rows = helper.Query("BTDB", "SELECT Code, SAPID FROM Machine WHERE SAPID=@s", new { s = "999999" }); + var rows = await helper.QueryAsync("BTDB", "SELECT Code, SAPID FROM Machine WHERE SAPID=@s", new { s = "999999" }); Assert.Empty(rows); } + + // --- InboundAPI-026: writes are permitted (design decision) --- + + [Fact] + public async Task ExecuteAsync_performs_write_and_returns_rows_affected() + { + var helper = new InboundDatabaseHelper(SeededGateway(), CancellationToken.None); + var affected = await helper.ExecuteAsync( + "BTDB", "UPDATE Machine SET Code=@c WHERE SAPID=@s", new { c = "UPDATED", s = "131453" }); + Assert.Equal(1, affected); + + // The write actually landed (read it back through the same helper). + var code = await helper.QuerySingleAsync("BTDB", "SELECT Code FROM Machine WHERE SAPID=@s", new { s = "131453" }); + Assert.Equal("UPDATED", code); + } + + // --- InboundAPI-026: SQL-injection protection — values are bound, not concatenated --- + + [Fact] + public async Task ParameterValues_are_bound_not_concatenated() + { + // A classic injection payload as the parameter VALUE. If the helper concatenated + // it, "WHERE SAPID='131453' OR '1'='1'" would match the seeded row and return + // "Z28061A". Because the value is bound as a literal parameter, it matches no + // SAPID and the query returns null — proving the value never altered the SQL. + var helper = new InboundDatabaseHelper(SeededGateway(), CancellationToken.None); + var code = await helper.QuerySingleAsync( + "BTDB", "SELECT Code FROM Machine WHERE SAPID=@s", new { s = "131453' OR '1'='1" }); + Assert.Null(code); + } + + // --- InboundAPI-028: negative paths --- + + [Fact] + public async Task QuerySingleAsync_null_gateway_throws() + { + var helper = new InboundDatabaseHelper(gateway: null, CancellationToken.None); + await Assert.ThrowsAsync( + () => helper.QuerySingleAsync("BTDB", "SELECT 1")); + } + + [Fact] + public async Task QueryAsync_null_gateway_throws() + { + var helper = new InboundDatabaseHelper(gateway: null, CancellationToken.None); + await Assert.ThrowsAsync( + () => helper.QueryAsync("BTDB", "SELECT 1")); + } + + [Fact] + public async Task ExecuteAsync_null_gateway_throws() + { + var helper = new InboundDatabaseHelper(gateway: null, CancellationToken.None); + await Assert.ThrowsAsync( + () => helper.ExecuteAsync("BTDB", "DELETE FROM Machine")); + } + + // --- InboundAPI-027: the method-deadline token bounds the query on the EXECUTE path --- + + [Fact] + public async Task QuerySingleAsync_honours_cancellation_token_on_execute() + { + // The gateway opens with None (so the open succeeds), but the helper forwards the + // pre-cancelled deadline token to ExecuteScalarAsync — which must observe it. This + // proves the fix threads the token past the connection open, where the old + // synchronous ExecuteScalar() ignored cancellation entirely. + using var cts = new CancellationTokenSource(); + await cts.CancelAsync(); + var helper = new InboundDatabaseHelper(SeededGateway(honorTokenOnOpen: false), cts.Token); + await Assert.ThrowsAnyAsync( + () => helper.QuerySingleAsync("BTDB", "SELECT Code FROM Machine")); + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs index bdfdde0b..dc7de810 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs @@ -549,7 +549,7 @@ public class InboundScriptExecutorTests [Fact] public async Task Script_UsingDatabase_QueriesViaGateway() { - // A script that calls Database.QuerySingle runs against an executor whose + // A script that calls Database.QuerySingleAsync runs against an executor whose // ServiceProvider registers an IDatabaseGateway backed by in-memory SQLite. var services = new ServiceCollection(); services.AddSingleton(SeededSqliteGateway()); @@ -560,7 +560,7 @@ public class InboundScriptExecutorTests var method = new ApiMethod( "movein", - "return new { v = Database.QuerySingle(\"BTDB\", \"SELECT Code FROM Machine WHERE SAPID=@s\", new { s = (string)Parameters[\"sap\"] }) };") + "return new { v = await Database.QuerySingleAsync(\"BTDB\", \"SELECT Code FROM Machine WHERE SAPID=@s\", new { s = (string)Parameters[\"sap\"] }) };") { Id = 1, TimeoutSeconds = 10, diff --git a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/RouteHelperTests.cs b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/RouteHelperTests.cs index a812f205..6fa7827b 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/RouteHelperTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/RouteHelperTests.cs @@ -214,10 +214,15 @@ public class RouteHelperTests } [Fact] - public async Task WaitForAttribute_WithNoExplicitToken_InheritsMethodDeadlineToken() + public async Task WaitForAttribute_IsNotBoundByMethodDeadline_WaitTimeoutGoverns() { + // InboundAPI-029 (spec §6): unlike Call/GetAttributes/SetAttributes, the wait is + // bounded by its OWN timeout, not the method deadline. Even with the method + // deadline ALREADY cancelled, the wait still proceeds and returns the site's + // wait-timeout result — the deadline does not cut it short. SiteResolves("inst-1", "SiteA"); using var deadline = new CancellationTokenSource(); + await deadline.CancelAsync(); // method deadline already elapsed CancellationToken seen = default; _router.RouteToWaitForAttributeAsync("SiteA", Arg.Any(), Arg.Do(t => seen = t)) .Returns(ci => new RouteToWaitForAttributeResponse( @@ -226,9 +231,52 @@ public class RouteHelperTests Success: true, ErrorMessage: null, DateTimeOffset.UtcNow)); var bound = CreateHelper().WithDeadline(deadline.Token); + var matched = await bound.To("inst-1").WaitForAttribute("Flag", true, TimeSpan.FromSeconds(30)); + + Assert.False(matched); // the site's wait-timeout result, not a cancellation + Assert.False(seen.IsCancellationRequested); // the wait token ignores the cancelled method deadline + Assert.NotEqual(deadline.Token, seen); // it is a per-wait token, not the method deadline + } + + [Fact] + public async Task WaitForAttribute_ExplicitToken_IsHonoured() + { + // An explicit caller token still applies (a tighter bound the script chose). + SiteResolves("inst-1", "SiteA"); + using var explicitCts = new CancellationTokenSource(); + await explicitCts.CancelAsync(); + CancellationToken seen = default; + _router.RouteToWaitForAttributeAsync("SiteA", Arg.Any(), Arg.Do(t => seen = t)) + .Returns(ci => new RouteToWaitForAttributeResponse( + ((RouteToWaitForAttributeRequest)ci[1]).CorrelationId, + Matched: false, Value: null, Quality: null, TimedOut: true, + Success: true, ErrorMessage: null, DateTimeOffset.UtcNow)); + + await CreateHelper().To("inst-1") + .WaitForAttribute("Flag", true, TimeSpan.FromSeconds(30), explicitCts.Token); + + Assert.True(seen.IsCancellationRequested); // the explicit caller token cancels the wait + } + + [Fact] + public async Task WaitForAttribute_ClientDisconnect_CancelsTheWait() + { + // A client disconnect (the raw request-abort token, threaded via WithRequestAborted) + // still cancels the wait even though the method deadline does not bound it. + SiteResolves("inst-1", "SiteA"); + using var clientAbort = new CancellationTokenSource(); + await clientAbort.CancelAsync(); + CancellationToken seen = default; + _router.RouteToWaitForAttributeAsync("SiteA", Arg.Any(), Arg.Do(t => seen = t)) + .Returns(ci => new RouteToWaitForAttributeResponse( + ((RouteToWaitForAttributeRequest)ci[1]).CorrelationId, + Matched: false, Value: null, Quality: null, TimedOut: true, + Success: true, ErrorMessage: null, DateTimeOffset.UtcNow)); + + var bound = CreateHelper().WithRequestAborted(clientAbort.Token); await bound.To("inst-1").WaitForAttribute("Flag", true, TimeSpan.FromSeconds(30)); - Assert.Equal(deadline.Token, seen); + Assert.True(seen.IsCancellationRequested); // the client-abort token cancels the wait } [Fact]