From f33f25a10b3577e66d9561c8c3ad68900eb280f5 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 17 Aug 2026 06:05:29 -0400 Subject: [PATCH] fix(auth+cli): fail-closed site-scope gate (row 98); case-insensitive audit filter values with a strict server-side check (row 99) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Register row 98 — the site-scope authorization gate was FAIL-OPEN on an unresolvable target. The resolve-then-check shape was written as `if (target != null) EnforceSiteScope(...)`, so a null resolve skipped the check entirely and a site-scoped caller passed the gate for any id in the system. Authorization now fails CLOSED: a target that cannot be PROVEN to sit inside the caller's scope is treated as outside it. The defect turned out to have THREE more instances than the register recorded. Besides `EnforceSiteScopeForInstance` and `EnforceSiteScopeForIdentifier`, the same inline shape sat in `HandleGetInstance`, `HandleGetSite` and `HandleGetDataConnection` — read handlers that resolve their own target and so never call the helpers. Those were the sharper half of the bug: they answered success-with-null for a nonexistent id and Unauthorized for an out-of-scope one, making the pair an id-enumeration oracle over the whole fleet. All five now route through one shared `DenyUnresolvedTargetIfScoped` arm. Two properties are load-bearing and pinned by tests: - System-wide callers (Administrator, or a Deployer with no site grants) are untouched — they return at the guards above the lookup and keep the handler's ordinary "not found" / null answer. - The denial is indistinguishable from an out-of-scope denial: same ManagementUnauthorized shape, and a message that never states whether the target exists. Register row 99 — CLI `audit query --status delivered` failed client-side while the server would have accepted it: `AcceptOnlyFromAmong` matches ordinally against the capitalised literals. Same treatment row 88 gave `deploy status` — the client-side gate is removed, the valid set moves into the option help text, and the server becomes the single authoritative check. That required ADDING the server check, not just relying on one: the shared audit filter parser was LAX, silently dropping an unrecognised value and leaving the dimension unconstrained — so removing the CLI gate alone would have turned a loud client-side failure into a plausible, complete-looking page that did not answer the question asked. New `ParseEnumListStrict` (case-insensitive, rejects ordinals — `Enum.TryParse` accepts "3", and a filter is a name-based contract) backs the ManagementService /api/audit/query + /api/audit/export endpoints, which now return 400 BAD_REQUEST naming the valid set. Other AcceptOnlyFromAmong sites surveyed in the audit group: `--channel`, `--kind`, `--status` on BOTH `query` and `export` (6 uses) plus `export --format` (rejected `--format CSV` though the server lowercases it) — all seven fixed here. Deliberately left alone: `template folder reorder --direction` (pure client-side literal, no server-side parse to defer to) and the global `--format json|table` (CLI-local output selection, never sent to a server). The CentralUI AuditLogPage drill-in parser and the CentralUI export endpoint it feeds deliberately KEEP the lax parse — they read a query string a human may have bookmarked, where a stale fragment must still render the page. Both contracts now live side by side in AuditQueryParamParsers and are documented as non-reconcilable. Docs: Component-ManagementService.md gains a "Site-scope gate: fail-closed on an unresolvable target" section and a strict-filter note on the audit endpoints; Component-Security.md records the fail-closed shape under Deployer site scoping; the CLI README marks the audit filter values case-insensitive and states the no-silent-drop / no-ordinals contract. Tests: 5 new ManagementActorTests fail before the row-98 fix (verified by reverting ManagementActor.cs) and cover scoped+nonexistent -> denied, system-wide+nonexistent -> unchanged (both Administrator and no-grant Deployer), scoped+in-scope -> allowed, and the indistinguishability property. 21 CLI tests fail before the row-99 fix. Four pre-existing tests that pinned the removed client-side gate were rewritten to pin the new contract. --- .../Component-ManagementService.md | 19 ++ docs/requirements/Component-Security.md | 1 + .../Commands/AuditCommands.cs | 48 ++--- src/ZB.MOM.WW.ScadaBridge.CLI/README.md | 23 ++- .../Types/Audit/AuditFilterParseException.cs | 21 ++ .../Types/Audit/AuditQueryParamParsers.cs | 86 +++++++- .../AuditEndpoints.cs | 68 ++++++- .../ManagementActor.cs | 111 +++++++++-- .../CommandTreeTests.cs | 83 +++++++- .../Commands/AuditExportCommandTests.cs | 39 +++- .../Commands/AuditQueryCommandTests.cs | 62 +++--- .../Types/AuditQueryParamParsersTests.cs | 102 +++++++++- .../AuditEndpointsTests.cs | 69 ++++++- .../ManagementActorTests.cs | 187 ++++++++++++++++++ 14 files changed, 818 insertions(+), 101 deletions(-) create mode 100644 src/ZB.MOM.WW.ScadaBridge.Commons/Types/Audit/AuditFilterParseException.cs diff --git a/docs/requirements/Component-ManagementService.md b/docs/requirements/Component-ManagementService.md index aefc8ba2..4e077dce 100644 --- a/docs/requirements/Component-ManagementService.md +++ b/docs/requirements/Component-ManagementService.md @@ -83,6 +83,12 @@ In addition to `/management`, the Management Service exposes a dedicated REST su Both endpoints honour any site-scope rules attached to the caller's audit role by intersecting the caller-supplied `sourceSiteId` filter with the user's `PermittedSiteIds` (out-of-scope requests yield HTTP 403). Permission denial returns HTTP 403 with the same envelope shape used by `/management`. +**Filter parsing is strict on the enum dimensions** (`channel`, `kind`, `status`; register row 99). Values are matched **case-insensitively** against the enum member names, and an unrecognised value returns **HTTP 400** `{ error, code: "BAD_REQUEST" }` naming the full valid set — it is not silently dropped. Silently dropping a filter would leave that dimension unconstrained and hand the caller a plausible, complete-looking page that does not answer the question they asked (the same failure row 88 fixed for `deploy status`). **Ordinals are rejected**: a filter is a name-based contract, so `status=3` is an error even though `Enum.TryParse` would accept it and `3` is a real member. The `correlationId`/`executionId`/`parentExecutionId`/date dimensions stay **lax** (unparseable values are dropped). + +This is the *authoritative* check for those three dimensions — the CLI deliberately carries no client-side vocabulary gate. It used to (`AcceptOnlyFromAmong`), but that check matches **case-sensitively**, so `audit query --status delivered` failed at the CLI before ever reaching a server that would have accepted it. The valid set now lives in the CLI's option help text only. + +> The CentralUI `AuditLogPage` drill-in parser and the CentralUI export endpoint it feeds deliberately keep the **lax** parse. They read a query string a human may have bookmarked or hand-edited, where a stale fragment must still render the page rather than fault it. Both contracts live side by side in `AuditQueryParamParsers` (`ParseEnumList` vs `ParseEnumListStrict`); do not reconcile them to one. + ## Message Groups ### Templates @@ -244,6 +250,19 @@ Every incoming message carries the authenticated user's identity and roles. The Unauthorized commands receive an `Unauthorized` response message. Failed authorization attempts are not audit logged (consistent with existing behavior). +#### Site-scope gate: fail-closed on an unresolvable target + +The site-scope helpers resolve the command's target (instance id, site id, site identifier, data-connection id) and check the resolved site against the caller's `PermittedSiteIds`. **A target that cannot be resolved is DENIED for a site-scoped caller** — `SiteScopeViolationException` → `ManagementUnauthorized`, exactly as if it were out of scope. + +The gate used to be **fail-open** here (deferred-work register row 98). The resolve-then-check shape was written as `if (target != null) EnforceSiteScope(...)`, so a null resolve skipped the check entirely and a site-scoped caller fell through into the handler body — reaching `HandleGetInstance`/`HandleGetSite`/`HandleGetDataConnection`'s null answer, or a handler fault, for any id in the system. The rule is now: *a target that cannot be PROVEN to sit inside the caller's scope is treated as outside it.* + +Two properties are load-bearing: + +- **System-wide callers are unaffected.** `Administrator`, and a `Deployer` carrying no site grants, return at the guards above the lookup — they were never scoped, so they still reach the handler and still receive its ordinary "not found" / null answer for a missing id. Only a scoped caller is denied. +- **The denial does not leak existence.** A scoped caller gets the identical `ManagementUnauthorized` shape for "does not exist" and for "exists at a site you may not see", and the message never states which. Otherwise the gate would be an id-enumeration oracle over the whole fleet — which is what the fail-open behaviour was, since `GetInstance`/`GetSite`/`GetDataConnection` answered success-with-null for a nonexistent id and `Unauthorized` for an out-of-scope one. + +The shared denial arm is `DenyUnresolvedTargetIfScoped`; `EnforceSiteScopeForInstance`, `EnforceSiteScopeForIdentifier` and the three read handlers that resolve their own target all route through it. + ## Service Dependencies (DI) The ManagementActor receives the following services and repositories via DI (injected through the actor's constructor or via a service provider): diff --git a/docs/requirements/Component-Security.md b/docs/requirements/Component-Security.md index d5f5f473..c8a1f28f 100644 --- a/docs/requirements/Component-Security.md +++ b/docs/requirements/Component-Security.md @@ -117,6 +117,7 @@ Set in a local or docker-dev environment via the environment variable `ScadaBrid - View site event logs. - **Site scoping**: A user with site-scoped Deployer role can only perform these actions for instances at their permitted sites. - **Administrator overrides site scoping.** A user who holds `Administrator` is system-wide **regardless of the `SiteId` claims they carry** — a scoped Deployer grant does not narrow an Administrator. This is enforced identically on both surfaces: `ManagementActor.EnforceSiteScope` (CLI / HTTP management API) and the Central UI's `SiteScopeService`. The two disagreed until deferred-work register row 86 — the UI was claims-only, so the same user saw the whole fleet through the CLI and only their sites in the UI. **No other role carries this bypass**; in particular `Designer` does not (Designer has no site-scoped permissions to bypass in the first place). +- **The scope check fails CLOSED on an unresolvable target.** Site scoping works by resolving the command's target (instance id, site id/identifier, data-connection id) and comparing its site to the permitted set. When the target does not resolve, a **site-scoped caller is DENIED** — the same `Unauthorized` result an out-of-scope target produces. The rule is *a target that cannot be PROVEN to sit inside the caller's scope is treated as outside it*; it fails open only for callers who were never scoped (Administrator, or a Deployer with no site grants), who keep their ordinary "not found" answer. The check was fail-open until deferred-work register row 98, which also made the denial **indistinguishable from an out-of-scope denial** — same response shape, and a message that never says whether the target exists — so a scoped caller cannot use the gate to enumerate ids across the fleet. See `Component-ManagementService.md` → *Site-scope gate: fail-closed on an unresolvable target*. > **Area management (arch-review C6)**: Managing area definitions per site is gated **any-of [Designer, Deployer]**, not Administrator. Areas are authored both in the Designer tooling and inside the Central UI Deployment Topology workflow (`Topology.razor` is `RequireDeployment`), so either role qualifies; Administrator is not implicitly included in the actor gate. diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/AuditCommands.cs b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/AuditCommands.cs index 7510b371..fdfc6968 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/AuditCommands.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/AuditCommands.cs @@ -41,28 +41,30 @@ public static class AuditCommands // --channel/--kind/--status/--site are multi-valued: System.CommandLine accepts // both repeated tokens (--channel A --channel B) and, with // AllowMultipleArgumentsPerToken, a single token carrying several values - // (--channel A B). AcceptOnlyFromAmong validates EACH supplied value. + // (--channel A B). + // + // The valid sets are named in the help text rather than enforced with + // AcceptOnlyFromAmong: that check is case-SENSITIVE, so `--status delivered` + // failed client-side even though the server has always parsed these + // case-insensitively (register row 99 — same treatment row 88 gave + // `deploy status`). The server rejects an unrecognised value with a 400 naming + // the same list and is the authoritative check; HTTP API callers never pass + // through these options at all. var channelOption = new Option("--channel") { - Description = "Filter by channel (ApiOutbound, DbOutbound, Notification, ApiInbound); repeatable", + Description = "Filter by channel (ApiOutbound, DbOutbound, Notification, ApiInbound; case-insensitive); repeatable", AllowMultipleArgumentsPerToken = true, }; - channelOption.AcceptOnlyFromAmong("ApiOutbound", "DbOutbound", "Notification", "ApiInbound"); var kindOption = new Option("--kind") { - Description = "Filter by event kind (ApiCall, ApiCallCached, DbWrite, DbWriteCached, NotifySend, NotifyDeliver, InboundRequest, InboundAuthFailure, CachedSubmit, CachedResolve); repeatable", + Description = "Filter by event kind (ApiCall, ApiCallCached, DbWrite, DbWriteCached, NotifySend, NotifyDeliver, InboundRequest, InboundAuthFailure, CachedSubmit, CachedResolve; case-insensitive); repeatable", AllowMultipleArgumentsPerToken = true, }; - kindOption.AcceptOnlyFromAmong( - "ApiCall", "ApiCallCached", "DbWrite", "DbWriteCached", "NotifySend", - "NotifyDeliver", "InboundRequest", "InboundAuthFailure", "CachedSubmit", "CachedResolve"); var statusOption = new Option("--status") { - Description = "Filter by status (Submitted, Forwarded, Attempted, Delivered, Failed, Parked, Discarded, Skipped); repeatable", + Description = "Filter by status (Submitted, Forwarded, Attempted, Delivered, Failed, Parked, Discarded, Skipped; case-insensitive); repeatable", AllowMultipleArgumentsPerToken = true, }; - statusOption.AcceptOnlyFromAmong( - "Submitted", "Forwarded", "Attempted", "Delivered", "Failed", "Parked", "Discarded", "Skipped"); var siteOption = new Option("--site") { Description = "Filter by source site ID; repeatable", @@ -143,34 +145,36 @@ public static class AuditCommands { var sinceOption = new Option("--since") { Description = "Start time: relative (1h, 24h, 7d) or ISO-8601", Required = true }; var untilOption = new Option("--until") { Description = "End time: relative (1h, 24h, 7d) or ISO-8601", Required = true }; - var formatExportOption = new Option("--format") { Description = "Export format", Required = true }; - formatExportOption.AcceptOnlyFromAmong("csv", "jsonl", "parquet"); + // Same rationale as --channel/--kind/--status below and on `query` (register + // row 99): AcceptOnlyFromAmong is case-SENSITIVE, so it rejected `--format CSV` + // even though the server lowercases the value before matching. The server + // returns a 400 naming csv/jsonl/parquet and is the authoritative check. + var formatExportOption = new Option("--format") + { + Description = "Export format (csv, jsonl, parquet; case-insensitive)", + Required = true, + }; var outputOption = new Option("--output") { Description = "Destination file path", Required = true }; // --channel/--kind/--status/--site are multi-valued — same shape as the // `query` subcommand: repeated tokens (--channel A --channel B) and, with // AllowMultipleArgumentsPerToken, a single token carrying several values - // (--channel A B). AcceptOnlyFromAmong validates EACH supplied value. + // (--channel A B). Valid sets are named in the help text and enforced + // server-side, case-insensitively — see the `query` builder for the full note. var channelOption = new Option("--channel") { - Description = "Filter by channel (ApiOutbound, DbOutbound, Notification, ApiInbound); repeatable", + Description = "Filter by channel (ApiOutbound, DbOutbound, Notification, ApiInbound; case-insensitive); repeatable", AllowMultipleArgumentsPerToken = true, }; - channelOption.AcceptOnlyFromAmong("ApiOutbound", "DbOutbound", "Notification", "ApiInbound"); var kindOption = new Option("--kind") { - Description = "Filter by event kind (ApiCall, ApiCallCached, DbWrite, DbWriteCached, NotifySend, NotifyDeliver, InboundRequest, InboundAuthFailure, CachedSubmit, CachedResolve); repeatable", + Description = "Filter by event kind (ApiCall, ApiCallCached, DbWrite, DbWriteCached, NotifySend, NotifyDeliver, InboundRequest, InboundAuthFailure, CachedSubmit, CachedResolve; case-insensitive); repeatable", AllowMultipleArgumentsPerToken = true, }; - kindOption.AcceptOnlyFromAmong( - "ApiCall", "ApiCallCached", "DbWrite", "DbWriteCached", "NotifySend", - "NotifyDeliver", "InboundRequest", "InboundAuthFailure", "CachedSubmit", "CachedResolve"); var statusOption = new Option("--status") { - Description = "Filter by status (Submitted, Forwarded, Attempted, Delivered, Failed, Parked, Discarded, Skipped); repeatable", + Description = "Filter by status (Submitted, Forwarded, Attempted, Delivered, Failed, Parked, Discarded, Skipped; case-insensitive); repeatable", AllowMultipleArgumentsPerToken = true, }; - statusOption.AcceptOnlyFromAmong( - "Submitted", "Forwarded", "Attempted", "Delivered", "Failed", "Parked", "Discarded", "Skipped"); var siteOption = new Option("--site") { Description = "Filter by source site ID; repeatable", diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md index dd9a0772..81e97487 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/README.md +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/README.md @@ -1688,9 +1688,9 @@ scadabridge --url audit query [options] |--------|----------|---------|-------------| | `--since` | no | — | Start time: relative (`1h`, `24h`, `7d`) or ISO-8601 | | `--until` | no | — | End time: relative (`1h`, `24h`, `7d`) or ISO-8601 | -| `--channel` | no | — | Filter by channel (`ApiOutbound`, `DbOutbound`, `Notification`, `ApiInbound`); repeatable — multiple values are OR-combined | -| `--kind` | no | — | Filter by event kind (`ApiCall`, `ApiCallCached`, `DbWrite`, `DbWriteCached`, `NotifySend`, `NotifyDeliver`, `InboundRequest`, `InboundAuthFailure`, `CachedSubmit`, `CachedResolve`); repeatable — multiple values are OR-combined | -| `--status` | no | — | Filter by status (`Submitted`, `Forwarded`, `Attempted`, `Delivered`, `Failed`, `Parked`, `Discarded`, `Skipped`); repeatable — multiple values are OR-combined | +| `--channel` | no | — | Filter by channel (`ApiOutbound`, `DbOutbound`, `Notification`, `ApiInbound`; case-insensitive); repeatable — multiple values are OR-combined | +| `--kind` | no | — | Filter by event kind (`ApiCall`, `ApiCallCached`, `DbWrite`, `DbWriteCached`, `NotifySend`, `NotifyDeliver`, `InboundRequest`, `InboundAuthFailure`, `CachedSubmit`, `CachedResolve`; case-insensitive); repeatable — multiple values are OR-combined | +| `--status` | no | — | Filter by status (`Submitted`, `Forwarded`, `Attempted`, `Delivered`, `Failed`, `Parked`, `Discarded`, `Skipped`; case-insensitive); repeatable — multiple values are OR-combined | | `--site` | no | — | Filter by source site ID; repeatable — multiple values are OR-combined | | `--target` | no | — | Filter by target (external system, DB connection, notification list) | | `--actor` | no | — | Filter by actor | @@ -1707,6 +1707,15 @@ either as repeated flags (`--channel ApiOutbound --channel DbOutbound`) or space-separated after one flag (`--channel ApiOutbound DbOutbound`). Values within one filter are OR-combined; the different filters are AND-combined. +**Filter values are case-insensitive** — `--status delivered`, `--status Delivered` +and `--status DELIVERED` are the same query. The CLI does not validate the +vocabulary itself; the server does, and an unrecognised value is an **error naming +the valid set**, not a silently-dropped filter. That matters: dropping the filter +would return a plausible, complete-looking page that does not answer the question +asked (the same failure mode as `deploy status`). Ordinals are rejected — a filter +is a name-based contract, so `--status 3` is an error even though `3` is a real +enum value. + With `--format table`, events render as an aligned text table with columns `OccurredAtUtc`, `Channel`, `Kind`, `Status`, `Target`, `Actor`, `DurationMs`, `HttpStatus`; long `Target`/`Actor` values are truncated with an ellipsis. With @@ -1726,11 +1735,11 @@ scadabridge --url audit export --since