c232a3ce67
Two review findings on Task 14's MTConnectDriverProbe, both red-first:
- CancelAfter(effectiveTimeout) sat before the try block with only a low-end
floor on a non-positive timeout, never a high-end cap. A pathological
timeout (e.g. TimeSpan.FromDays(1000)) threw an uncaught
ArgumentOutOfRangeException straight out of CancelAfter's ~49.7-day legal
range, violating the probe's own "Never throws" contract. Not reachable
through today's only caller (AdminOperationsActor clamps to [1,60]s and
wraps the call), but a landmine for any future caller without that clamp.
Fixed by clamping effectiveTimeout to a new MaxTimeout (10 minutes).
- The final `catch (Exception ex) { return new(false, ex.Message, null); }`
forwarded ex.Message verbatim for any exception type not enumerated above
it, breaking the redaction discipline every other catch was audited for
(AgentUri may carry userinfo credentials). Not exploitable by any
currently-reachable exception type, but an open door for a future one.
Fixed to build a message from the already-redacted safeUri + the
exception's type name instead of its message.
Also: a genuine non-2xx stub-handler test (prior HttpRequestException
coverage was connection-refused only), a credentialed-agentUri +
successful-probe redaction test, and a one-line doc remark on the accepted
reachable-vs-deployable gap.
238 lines
12 KiB
C#
238 lines
12 KiB
C#
using System.Diagnostics;
|
|
using System.Net.Http;
|
|
using System.Text.Json;
|
|
using System.Text.Json.Serialization;
|
|
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
|
|
|
|
namespace ZB.MOM.WW.OtOpcUa.Driver.MTConnect;
|
|
|
|
/// <summary>
|
|
/// One-shot MTConnect Agent reachability probe for the AdminUI's Test Connect button. Issues a
|
|
/// single <c>GET {AgentUri}/probe</c> under the supplied deadline and reports success/failure
|
|
/// with latency — it does not construct or initialize an <see cref="MTConnectDriver"/> instance.
|
|
/// </summary>
|
|
/// <remarks>
|
|
/// <para>
|
|
/// <b>Deliberately does NOT delegate to <see cref="MTConnectDriver.ParseOptions"/>.</b> That
|
|
/// method builds the full driver options document, including every authored
|
|
/// <c>MTConnectTagDefinition</c> (<c>BuildTag</c> can throw on a malformed tag entry that has
|
|
/// nothing to do with reachability). A probe only needs <c>agentUri</c>, so this type parses
|
|
/// its own minimal DTO — a config that is invalid for the driver (bad tag, bad data type) can
|
|
/// still be reachability-tested, matching the AdminUI's "does the endpoint answer" intent for
|
|
/// Test Connect. It also avoids a compile-time dependency on the factory DTO shape
|
|
/// (<c>MTConnectDriverFactoryExtensions</c>), which is authored separately (Task 15).
|
|
/// </para>
|
|
/// <para>
|
|
/// <b>Accepted gap: reachable ≠ deployable.</b> Because only <c>agentUri</c> is read, a
|
|
/// config can show green on Test Connect and still fail to deploy — e.g. a non-positive
|
|
/// <c>requestTimeoutMs</c> or a malformed tag entry the factory's full parse would reject.
|
|
/// This is the direct consequence of the independent-parsing decision above, judged
|
|
/// acceptable because Test Connect's job is "is the endpoint there", not "will this exact
|
|
/// config deploy".
|
|
/// </para>
|
|
/// <para>
|
|
/// <b>Deliberately DOES reuse <see cref="MTConnectAgentClient"/> and
|
|
/// <see cref="MTConnectProbeParser"/> for the actual round trip.</b> Hand-rolling the HTTP
|
|
/// call would have to re-derive the exact behaviour the client already provides: per-call
|
|
/// <see cref="HttpClient.Timeout"/> bounding, a linked-CTS deadline, and — most importantly —
|
|
/// correct handling of an Agent's <c>MTConnectError</c> document served under HTTP 200 (so
|
|
/// <c>EnsureSuccessStatusCode</c> never fires). <see cref="MTConnectProbeParser"/> already
|
|
/// lifts the Agent's own error text into the thrown exception's message; re-parsing by hand
|
|
/// here would either duplicate that logic or under-validate (accepting any 200 response as
|
|
/// healthy, which is precisely the "empty-but-successful" defect class this codebase has hit
|
|
/// before, #485). The one-off cost of a full probe-document parse is negligible next to that
|
|
/// risk.
|
|
/// </para>
|
|
/// <para>
|
|
/// <b>Never throws</b> (the <see cref="IDriverProbe"/> contract): every failure — unparseable
|
|
/// JSON, missing/malformed <c>agentUri</c>, DNS/TCP failure, a non-2xx response, a 200 body
|
|
/// that is not a valid <c>MTConnectDevices</c> document, and timeout — becomes
|
|
/// <c>Ok = false</c> with a message. Genuine caller cancellation is likewise converted rather
|
|
/// than left to propagate, matching <c>ModbusDriverProbe</c>'s posture (its linked-CTS catch
|
|
/// does not distinguish the caller's token from its own deadline).
|
|
/// </para>
|
|
/// <para>
|
|
/// <b>Non-positive <c>timeout</c> (arch-review 01/S-6).</b> A <c>0</c> or negative
|
|
/// timeout does NOT mean "wait forever" — it is replaced with <see cref="FallbackTimeout"/>
|
|
/// (5s, matching <see cref="MTConnectDriverOptions.RequestTimeoutMs"/>'s own default) so an
|
|
/// operator-authorable field can never brick the probe UI.
|
|
/// </para>
|
|
/// <para>
|
|
/// <b>Secrets discipline.</b> <c>AgentUri</c> may carry userinfo credentials
|
|
/// (<c>http://user:pass@host/</c>). Every message this type constructs from the URI uses a
|
|
/// redacted display form (<see cref="RedactedDisplay"/>) built ONLY from
|
|
/// <see cref="Uri.Scheme"/>/<see cref="Uri.Host"/>/<see cref="Uri.Port"/>/<see cref="Uri.AbsolutePath"/>
|
|
/// — never <see cref="Uri.UserInfo"/> and never the raw config string — so a password can
|
|
/// never reach the AdminUI via the returned <see cref="DriverProbeResult.Message"/>.
|
|
/// </para>
|
|
/// </remarks>
|
|
public sealed class MTConnectDriverProbe : IDriverProbe
|
|
{
|
|
/// <summary>Replaces a non-positive caller <c>timeout</c> — see the type remarks.</summary>
|
|
private static readonly TimeSpan FallbackTimeout = TimeSpan.FromSeconds(5);
|
|
|
|
/// <summary>
|
|
/// Caps an excessive caller <c>timeout</c>. <c>CancellationTokenSource.CancelAfter(TimeSpan)</c>'s
|
|
/// legal range tops out near ~49.7 days (<see cref="uint.MaxValue"/> ms minus one); an
|
|
/// uncapped pathological value (e.g. a caller passing <c>TimeSpan.FromDays(1000)</c>) throws
|
|
/// <see cref="ArgumentOutOfRangeException"/> straight out of
|
|
/// <c>CancelAfter</c>, which would violate the "Never throws" contract just as surely as a
|
|
/// non-positive timeout meaning "wait forever" would (arch-review 01/S-6 — this is the same
|
|
/// defect class, the high end rather than the low end). 10 minutes is generous for a
|
|
/// reachability probe and leaves an enormous margin under the legal ceiling.
|
|
/// </summary>
|
|
private static readonly TimeSpan MaxTimeout = TimeSpan.FromMinutes(10);
|
|
|
|
private static readonly JsonSerializerOptions JsonOptions = new()
|
|
{
|
|
PropertyNameCaseInsensitive = true,
|
|
UnmappedMemberHandling = JsonUnmappedMemberHandling.Skip,
|
|
Converters = { new JsonStringEnumConverter() },
|
|
};
|
|
|
|
/// <summary>Test seam: routes the reused <see cref="MTConnectAgentClient"/> through a stub handler so response-shape cases need no socket.</summary>
|
|
private readonly HttpMessageHandler? _handler;
|
|
|
|
/// <summary>Creates the probe used in production (real sockets).</summary>
|
|
public MTConnectDriverProbe()
|
|
: this(handler: null)
|
|
{
|
|
}
|
|
|
|
/// <summary>Test seam constructor — see <see cref="_handler"/>.</summary>
|
|
internal MTConnectDriverProbe(HttpMessageHandler? handler)
|
|
{
|
|
_handler = handler;
|
|
}
|
|
|
|
/// <inheritdoc />
|
|
/// <remarks>
|
|
/// Sourced from the <see cref="DriverTypeNames"/> constant, not a literal — the probe is
|
|
/// looked up by <c>AdminOperationsActor</c> under this key, so a drift from the constant
|
|
/// silently disables the Test Connect button rather than failing anything at build time.
|
|
/// </remarks>
|
|
public string DriverType => DriverTypeNames.MTConnect;
|
|
|
|
/// <summary>Minimal probe-only config shape. Deliberately independent of the factory DTO — see the type remarks.</summary>
|
|
private sealed class MTConnectProbeConfigDto
|
|
{
|
|
public string? AgentUri { get; set; }
|
|
}
|
|
|
|
/// <inheritdoc />
|
|
public async Task<DriverProbeResult> ProbeAsync(string configJson, TimeSpan timeout, CancellationToken ct)
|
|
{
|
|
MTConnectProbeConfigDto? dto;
|
|
try
|
|
{
|
|
dto = JsonSerializer.Deserialize<MTConnectProbeConfigDto>(configJson, JsonOptions);
|
|
}
|
|
catch (Exception ex)
|
|
{
|
|
return new(false, $"Config JSON is invalid: {ex.Message}", null);
|
|
}
|
|
|
|
if (dto is null)
|
|
{
|
|
return new(false, "Config JSON deserialized to null.", null);
|
|
}
|
|
|
|
var agentUri = dto.AgentUri?.Trim();
|
|
if (string.IsNullOrWhiteSpace(agentUri))
|
|
{
|
|
return new(false, "Config has no agentUri to probe.", null);
|
|
}
|
|
|
|
if (!Uri.TryCreate(agentUri, UriKind.Absolute, out var parsed) ||
|
|
(parsed.Scheme != Uri.UriSchemeHttp && parsed.Scheme != Uri.UriSchemeHttps))
|
|
{
|
|
return new(false, "Config's agentUri is not an absolute http(s) URI.", null);
|
|
}
|
|
|
|
var safeUri = RedactedDisplay(parsed);
|
|
|
|
// arch-review 01/S-6: a non-positive timeout must never mean "wait forever" (low end), and a
|
|
// pathologically large one must never overrun CancelAfter's legal range (high end) — see
|
|
// MaxTimeout.
|
|
var effectiveTimeout = timeout > TimeSpan.Zero ? timeout : FallbackTimeout;
|
|
if (effectiveTimeout > MaxTimeout)
|
|
{
|
|
effectiveTimeout = MaxTimeout;
|
|
}
|
|
|
|
var requestTimeoutMs = Math.Max(1, (int)Math.Ceiling(effectiveTimeout.TotalMilliseconds));
|
|
|
|
var sw = Stopwatch.StartNew();
|
|
using var cts = CancellationTokenSource.CreateLinkedTokenSource(ct);
|
|
cts.CancelAfter(effectiveTimeout);
|
|
|
|
var options = new MTConnectDriverOptions
|
|
{
|
|
AgentUri = agentUri,
|
|
RequestTimeoutMs = requestTimeoutMs,
|
|
};
|
|
|
|
try
|
|
{
|
|
using var client = new MTConnectAgentClient(options, _handler);
|
|
_ = await client.ProbeAsync(cts.Token).ConfigureAwait(false);
|
|
sw.Stop();
|
|
|
|
return new(true, $"MTConnect /probe OK ({safeUri})", sw.Elapsed);
|
|
}
|
|
catch (OperationCanceledException)
|
|
{
|
|
// Covers both the caller's token and our own CancelAfter deadline firing — see the type
|
|
// remarks on why cancellation is never allowed to propagate.
|
|
return new(false, $"Probe timed out after {effectiveTimeout.TotalSeconds:F0}s.", null);
|
|
}
|
|
catch (TimeoutException)
|
|
{
|
|
// MTConnectAgentClient's own request-deadline signal.
|
|
return new(false, $"Probe timed out after {effectiveTimeout.TotalSeconds:F0}s.", null);
|
|
}
|
|
catch (HttpRequestException ex)
|
|
{
|
|
// Connection failure or non-2xx status. HttpRequestException messages describe the
|
|
// socket endpoint or status code, never the request URI/userinfo, so ex.Message is safe.
|
|
return new(false, $"Request to {safeUri} failed: {ex.Message}", null);
|
|
}
|
|
catch (InvalidDataException ex)
|
|
{
|
|
// MTConnectProbeParser: not well-formed XML, not an MTConnectDevices document (including
|
|
// an MTConnectError-under-200, whose own errorCode/text ex.Message already carries), or
|
|
// a document with no devices. Never includes the request URI.
|
|
return new(false, $"Agent at {safeUri} did not return a valid MTConnect device model: {ex.Message}", null);
|
|
}
|
|
catch (ArgumentException)
|
|
{
|
|
// Defensive: MTConnectAgentClient's own AgentUri validation, reached only if this
|
|
// method's own Uri.TryCreate above passed but the client's stricter check disagreed.
|
|
// Never forward the exception's message verbatim — it can echo the raw (possibly
|
|
// credentialed) config string.
|
|
return new(false, "Config's agentUri was rejected by the MTConnect client as invalid.", null);
|
|
}
|
|
catch (Exception ex)
|
|
{
|
|
// Last-resort catch for an exception type none of the above enumerate. Deliberately does
|
|
// NOT forward ex.Message: unlike the typed catches above (each individually audited for
|
|
// whether its message can carry the raw AgentUri/userinfo), an unenumerated type has no
|
|
// such audit, so ex.Message is an open door for a credential leak. safeUri + the
|
|
// exception's TYPE name is enough for an operator to act on without that risk.
|
|
return new(false, $"Probe against {safeUri} failed unexpectedly ({ex.GetType().Name}).", null);
|
|
}
|
|
}
|
|
|
|
/// <summary>
|
|
/// Builds a display form of <paramref name="uri"/> for use in returned messages, composed
|
|
/// ONLY from scheme/host/port/path — never <see cref="Uri.UserInfo"/> — so a credentialed
|
|
/// <c>AgentUri</c> can never leak a password into the AdminUI.
|
|
/// </summary>
|
|
private static string RedactedDisplay(Uri uri)
|
|
{
|
|
var portSegment = uri.IsDefaultPort ? string.Empty : $":{uri.Port}";
|
|
|
|
return $"{uri.Scheme}://{uri.Host}{portSegment}{uri.AbsolutePath}";
|
|
}
|
|
}
|