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"); + } }