From c232a3ce674a509c2e24ec31982136cf9343c544 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 24 Jul 2026 17:49:23 -0400 Subject: [PATCH] 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. --- .../MTConnectDriverProbe.cs | 36 +++++++- .../MTConnectDriverProbeTests.cs | 84 +++++++++++++++++++ 2 files changed, 118 insertions(+), 2 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverProbe.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverProbe.cs index 7768ec7b..85658b9a 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverProbe.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverProbe.cs @@ -23,6 +23,14 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.MTConnect; /// (MTConnectDriverFactoryExtensions), which is authored separately (Task 15). /// /// +/// Accepted gap: reachable ≠ deployable. Because only agentUri is read, a +/// config can show green on Test Connect and still fail to deploy — e.g. a non-positive +/// requestTimeoutMs 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". +/// +/// /// Deliberately DOES reuse and /// for the actual round trip. 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 /// Replaces a non-positive caller timeout — see the type remarks. private static readonly TimeSpan FallbackTimeout = TimeSpan.FromSeconds(5); + /// + /// Caps an excessive caller timeout. CancellationTokenSource.CancelAfter(TimeSpan)'s + /// legal range tops out near ~49.7 days ( ms minus one); an + /// uncapped pathological value (e.g. a caller passing TimeSpan.FromDays(1000)) throws + /// straight out of + /// CancelAfter, 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. + /// + 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); } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectDriverProbeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectDriverProbeTests.cs index 8e25ba29..03ce5781 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectDriverProbeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectDriverProbeTests.cs @@ -232,4 +232,88 @@ public sealed class MTConnectDriverProbeTests /// Local stand-in so the stub handler's "never returns" branch has a throw statement to satisfy flow analysis. private sealed class UnreachableException : Exception; + + // ── review follow-up: pathological (out-of-CancelAfter-range) timeout ──── + + /// + /// Reproduces the reviewer's finding: CancelAfter's legal range tops out near ~49.7 + /// days, and the caller-supplied timeout reached it uncapped (only floored on the low + /// end). A caller with no clamp of its own — unlike today's only caller, + /// AdminOperationsActor, which clamps to [1,60]s — must still get a + /// , never an escaping + /// : that is the whole point of the "Never throws" + /// contract. + /// + [Fact] + public async Task Pathologically_large_timeout_does_not_throw() + { + var probe = new MTConnectDriverProbe(); + + var r = await probe.ProbeAsync( + "{\"agentUri\":\"http://127.0.0.1:1/\"}", TimeSpan.FromDays(1000), CancellationToken.None); + + r.Ok.ShouldBeFalse(); + r.Message.ShouldNotBeNull(); + } + + // ── review follow-up: unenumerated exception type must not leak ex.Message ── + + /// Message text crafted to look like it embedded a credentialed URI, exactly what an unaudited ex.Message could leak. + private sealed class LeakyException() + : Exception("dial failed for http://leaky-user:leaky-pass@internal-host/probe (boom)"); + + /// + /// Reproduces the reviewer's finding: the final catch (Exception ex) forwarded + /// ex.Message verbatim for any type not enumerated above it, breaking the redaction + /// discipline every other catch was audited for. No currently-reachable exception type + /// exploits this, but nothing stopped a future one from doing so either — this test closes + /// that open door by asserting the final catch never echoes exception text. + /// + [Fact] + public async Task Unenumerated_exception_type_does_not_leak_ex_Message_via_final_catch() + { + var probe = ProbeWithHandler((_, _) => Task.FromException(new LeakyException())); + + var r = await probe.ProbeAsync( + "{\"agentUri\":\"http://stub-agent/\"}", QuickTimeout, CancellationToken.None); + + r.Ok.ShouldBeFalse(); + r.Message.ShouldNotBeNull(); + r.Message.ShouldNotContain("leaky-pass"); + r.Message.ShouldNotContain("leaky-user:leaky-pass"); + } + + // ── minor: genuine non-2xx status (not just connection-refused) ────────── + + [Fact] + public async Task NonSuccess_status_code_returns_not_ok() + { + var probe = ProbeWithHandler((_, _) => Task.FromResult(new HttpResponseMessage(HttpStatusCode.NotFound) + { + Content = new StringContent(string.Empty), + })); + + var r = await probe.ProbeAsync( + "{\"agentUri\":\"http://stub-agent/\"}", QuickTimeout, CancellationToken.None); + + r.Ok.ShouldBeFalse(); + r.Message.ShouldNotBeNull(); + r.Latency.ShouldBeNull(); + } + + // ── minor: credentialed agentUri + a SUCCESSFUL response must still redact ── + + [Fact] + public async Task Credentialed_agentUri_is_redacted_even_on_a_successful_probe() + { + var probe = ProbeWithHandler((_, _) => Task.FromResult(XmlResponse(ValidProbeXml))); + + var r = await probe.ProbeAsync( + "{\"agentUri\":\"http://stub-user:stub-pass@stub-agent/\"}", QuickTimeout, CancellationToken.None); + + r.Ok.ShouldBeTrue(); + r.Message.ShouldNotBeNull(); + r.Message.ShouldNotContain("stub-pass"); + r.Message.ShouldNotContain("stub-user:stub-pass"); + } }