fix(auth+cli): fail-closed site-scope gate (row 98); case-insensitive audit filter values with a strict server-side check (row 99)
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.
This commit is contained in:
@@ -0,0 +1,21 @@
|
||||
namespace ZB.MOM.WW.ScadaBridge.Commons.Types.Audit;
|
||||
|
||||
/// <summary>
|
||||
/// Thrown by <see cref="AuditQueryParamParsers.ParseEnumListStrict{TEnum}"/> when an
|
||||
/// Audit Log filter value is not a member name of its enum.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// The message is composed to be shown VERBATIM to the caller — it names the offending
|
||||
/// value and the full valid set, and carries no server-internal detail — so the
|
||||
/// ManagementService audit endpoints surface it directly as the body of a
|
||||
/// <c>400 BAD_REQUEST</c> rather than replacing it with a generic fault string.
|
||||
/// </remarks>
|
||||
public class AuditFilterParseException : Exception
|
||||
{
|
||||
/// <summary>
|
||||
/// Initializes a new instance with a caller-safe message naming the bad value
|
||||
/// and the valid set.
|
||||
/// </summary>
|
||||
/// <param name="message">The caller-safe error message.</param>
|
||||
public AuditFilterParseException(string message) : base(message) { }
|
||||
}
|
||||
@@ -21,11 +21,24 @@ namespace ZB.MOM.WW.ScadaBridge.Commons.Types.Audit;
|
||||
/// </para>
|
||||
///
|
||||
/// <para>
|
||||
/// <b>Lax-parse contract.</b> Every value of a repeated parameter is parsed
|
||||
/// independently; an unparseable or blank element is silently dropped (NO 400)
|
||||
/// rather than failing the whole set. An empty result collapses to <c>null</c> so
|
||||
/// the corresponding filter dimension stays unconstrained.
|
||||
/// <b>Two parse contracts, chosen per surface.</b>
|
||||
/// </para>
|
||||
/// <list type="bullet">
|
||||
/// <item><b>Lax</b> (<see cref="ParseEnumList{TEnum}"/>): every value of a repeated
|
||||
/// parameter is parsed independently; an unparseable or blank element is silently
|
||||
/// dropped (NO 400) rather than failing the whole set. An empty result collapses to
|
||||
/// <c>null</c> so the corresponding filter dimension stays unconstrained. This is the
|
||||
/// right contract for the CentralUI <c>AuditLogPage</c> drill-in parser and the
|
||||
/// CentralUI export endpoint it feeds: those read a query string a human may have
|
||||
/// bookmarked or hand-edited, and a stale fragment must still render the page rather
|
||||
/// than fault it.</item>
|
||||
/// <item><b>Strict</b> (<see cref="ParseEnumListStrict{TEnum}"/>): an unrecognised
|
||||
/// value is an ERROR naming the valid set. This is the right contract for the
|
||||
/// ManagementService <c>/api/audit/query</c> + <c>/api/audit/export</c> endpoints,
|
||||
/// which answer a CLI/API caller who asked a specific question: silently dropping
|
||||
/// their filter returns a plausible, complete-looking result that does NOT answer it
|
||||
/// (register rows 46/88/99 — the same loud-failure principle).</item>
|
||||
/// </list>
|
||||
/// </summary>
|
||||
public static class AuditQueryParamParsers
|
||||
{
|
||||
@@ -57,6 +70,71 @@ public static class AuditQueryParamParsers
|
||||
return parsed.Count > 0 ? parsed : null;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Parses each raw value as <typeparamref name="TEnum"/> (case-insensitive),
|
||||
/// REJECTING an unrecognised value with an
|
||||
/// <see cref="AuditFilterParseException"/> that names the valid set. Blank
|
||||
/// elements are still dropped silently (a repeated query param can legitimately
|
||||
/// carry an empty slot); a set that yields no value collapses to <c>null</c> so
|
||||
/// the filter dimension stays unconstrained.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Register row 99. Two traps this closes that a bare
|
||||
/// <see cref="Enum.TryParse{TEnum}(string, bool, out TEnum)"/> does not:
|
||||
/// <list type="number">
|
||||
/// <item><b>Numeric strings.</b> <c>TryParse</c> happily converts <c>"7"</c> —
|
||||
/// and <c>"3"</c>, which lands on a REAL member — into an enum value. A filter
|
||||
/// is a caller-facing name-based contract, never an ordinal one, so any value
|
||||
/// starting with a digit or sign is rejected outright. Enum member names can
|
||||
/// never start with those characters, so the check costs no legitimate input.</item>
|
||||
/// <item><b>Undefined composites.</b> <see cref="Enum.IsDefined{TEnum}(TEnum)"/>
|
||||
/// catches anything that parsed to a value with no declared member.</item>
|
||||
/// </list>
|
||||
/// </remarks>
|
||||
/// <typeparam name="TEnum">The enum type to parse each raw value into.</typeparam>
|
||||
/// <param name="rawValues">Raw query-parameter string values to parse; may be null.</param>
|
||||
/// <param name="parameterName">Query-parameter name, used in the error message (e.g. <c>"status"</c>).</param>
|
||||
/// <returns>A non-empty list of parsed values, or <c>null</c> if no values were supplied.</returns>
|
||||
/// <exception cref="AuditFilterParseException">A supplied value is not a member name of <typeparamref name="TEnum"/>.</exception>
|
||||
public static IReadOnlyList<TEnum>? ParseEnumListStrict<TEnum>(
|
||||
IEnumerable<string?>? rawValues, string parameterName)
|
||||
where TEnum : struct, Enum
|
||||
{
|
||||
if (rawValues is null)
|
||||
{
|
||||
return null;
|
||||
}
|
||||
|
||||
var parsed = new List<TEnum>();
|
||||
foreach (var raw in rawValues)
|
||||
{
|
||||
if (string.IsNullOrWhiteSpace(raw))
|
||||
{
|
||||
continue;
|
||||
}
|
||||
|
||||
var trimmed = raw.Trim();
|
||||
if (LooksNumeric(trimmed)
|
||||
|| !Enum.TryParse<TEnum>(trimmed, ignoreCase: true, out var value)
|
||||
|| !Enum.IsDefined(value))
|
||||
{
|
||||
throw new AuditFilterParseException(
|
||||
$"Unknown {parameterName} '{trimmed}'. Valid values: {string.Join(", ", Enum.GetNames<TEnum>())}.");
|
||||
}
|
||||
|
||||
parsed.Add(value);
|
||||
}
|
||||
|
||||
return parsed.Count > 0 ? parsed : null;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// True when the value opens with a digit or a sign — i.e. an ordinal, not an
|
||||
/// enum member name. No enum member name can start with these characters.
|
||||
/// </summary>
|
||||
private static bool LooksNumeric(string value)
|
||||
=> value.Length > 0 && (char.IsAsciiDigit(value[0]) || value[0] == '+' || value[0] == '-');
|
||||
|
||||
/// <summary>
|
||||
/// Trims each raw value and drops blank entries. Returns <c>null</c> when
|
||||
/// <paramref name="rawValues"/> is <c>null</c>, empty, or every value was
|
||||
|
||||
Reference in New Issue
Block a user