fix(mtconnect): cap CancelAfter overflow + stop leaking ex.Message in probe fallback (review follow-up)
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.
This commit is contained in:
@@ -23,6 +23,14 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.MTConnect;
|
||||
/// (<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
|
||||
@@ -63,6 +71,18 @@ 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,
|
||||
@@ -131,8 +151,15 @@ public sealed class MTConnectDriverProbe : IDriverProbe
|
||||
|
||||
var safeUri = RedactedDisplay(parsed);
|
||||
|
||||
// arch-review 01/S-6: a non-positive timeout must never mean "wait forever".
|
||||
// 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();
|
||||
@@ -187,7 +214,12 @@ public sealed class MTConnectDriverProbe : IDriverProbe
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
return new(false, ex.Message, null);
|
||||
// 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);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user