From 219611c82194af11dad45ad9352f3c0203683220 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 20 Oct 2026 07:09:02 -0400 Subject: [PATCH] fix(worker): narrow AlarmAckWriter ArgumentException interop filter Resolve the operator user, password and comment before the interop try blocks, and match ArgumentException as an MXAccess failure only when it carries E_INVALIDARG and is not ArgumentNullException or ArgumentOutOfRangeException. A programming error inside the try blocks now propagates instead of being reported as an authentication or write failure. Fixes #164 Claude-Session: https://claude.ai/code/session_016J6isfcgqy4SB2RK3ZvFvT --- docs/Grpc.md | 2 +- .../MxAccess/AlarmAckWriterTests.cs | 74 +++++++++++++++++++ .../MxAccess/AlarmAckWriter.cs | 36 +++++++-- 3 files changed, 105 insertions(+), 7 deletions(-) diff --git a/docs/Grpc.md b/docs/Grpc.md index cdccebc..8873ed9 100644 --- a/docs/Grpc.md +++ b/docs/Grpc.md @@ -115,7 +115,7 @@ The worker calls `AuthenticateUser` as the operator, then writes `operator_comme | `AuthenticateUser` failed with a server-gone HRESULT (`RPC_E_DISCONNECTED`, `RPC_S_SERVER_UNAVAILABLE`, `RPC_E_SERVERFAULT`); the writer tears down its MXAccess registration and re-registers on the next ack | `MXACCESS_FAILURE` | that HRESULT | | `AddItem`, `Advise` or `Write` on the `AckMsg` item raised an MXAccess error (`COMException`, or `ArgumentException` for `E_INVALIDARG`); the item is dropped and re-added on the next ack, and a server-gone HRESULT also tears down the registration | `MXACCESS_FAILURE` | the exception's HRESULT | | Write rejected (`OnWriteComplete` non-OK) | `MXACCESS_FAILURE` | the `detail` of the `OnWriteComplete` `MXSTATUS_PROXY`, or `-1` when `detail` is `0` | -| Any other exception escaping the ack (e.g. the ack writer's MXAccess `Register` failing, or the monitor's worker session having no active alarm subscription — `InvalidOperationException`) | `MXACCESS_FAILURE` | the exception's `HResult`; the diagnostic names only the exception type and HRESULT | +| Any other exception escaping the ack (e.g. the ack writer's MXAccess `Register` failing, the monitor's worker session having no active alarm subscription — `InvalidOperationException` — or an argument error that is not the interop's `E_INVALIDARG`: `ArgumentNullException`, `ArgumentOutOfRangeException`, or an `ArgumentException` with another HRESULT, which signal a bug rather than an MXAccess failure) | `MXACCESS_FAILURE` | the exception's `HResult`; the diagnostic names only the exception type and HRESULT | `reply.Statuses` carries the `OnWriteComplete` `MxStatusProxy` rows when they were observed. Because an unconfirmed write also returns `OK`, a client that must be certain should watch for the resulting transition on `StreamAlarms`. `AcknowledgeAlarm` stays under the `invoke:write` scope; see [Authorization](./Authorization.md). diff --git a/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/AlarmAckWriterTests.cs b/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/AlarmAckWriterTests.cs index 2332d8d..0714173 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/AlarmAckWriterTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/AlarmAckWriterTests.cs @@ -187,6 +187,70 @@ public sealed class AlarmAckWriterTests Assert.DoesNotContain("Unregister", server.Operations); } + [Fact] + public void Acknowledge_WhenAuthenticateUserThrowsArgumentExceptionWithEInvalidArg_ReturnsAuthenticationFailed() + { + // The HRESULT is what the narrowed filter keys on; set it explicitly so the + // test does not depend on ArgumentException's default HResult. + RecordingAckServer server = new RecordingAckServer + { + AuthenticateResult = 5, + AuthenticateException = new HResultArgumentException("bad credentials", unchecked((int)0x80070057)), + }; + using AlarmAckWriter writer = new AlarmAckWriter(server, new MxAccessWriteCompletionCache(), () => { }, TimeSpan.FromMilliseconds(10), "MxGateway"); + + AlarmAckResult result = writer.Acknowledge("T.A.AckMsg", "one", Alice()); + + Assert.Equal(AlarmAckOutcome.AuthenticationFailed, result.Outcome); + Assert.Equal(unchecked((int)0x80070057), result.NativeStatus); + } + + [Fact] + public void Acknowledge_WhenAuthenticateUserThrowsArgumentNullException_Propagates() + { + // A programming error is a bug, not an operator authentication failure. + RecordingAckServer server = new RecordingAckServer + { + AuthenticateResult = 5, + AuthenticateException = new ArgumentNullException("verifyUser"), + }; + using AlarmAckWriter writer = new AlarmAckWriter(server, new MxAccessWriteCompletionCache(), () => { }, TimeSpan.FromMilliseconds(10), "MxGateway"); + + Assert.Throws(() => writer.Acknowledge("T.A.AckMsg", "one", Alice())); + } + + [Fact] + public void Acknowledge_WhenAuthenticateUserThrowsArgumentExceptionWithOtherHResult_Propagates() + { + RecordingAckServer server = new RecordingAckServer + { + AuthenticateResult = 5, + AuthenticateException = new HResultArgumentException("not interop", unchecked((int)0x80131500)), + }; + using AlarmAckWriter writer = new AlarmAckWriter(server, new MxAccessWriteCompletionCache(), () => { }, TimeSpan.FromMilliseconds(10), "MxGateway"); + + Assert.Throws(() => writer.Acknowledge("T.A.AckMsg", "one", Alice())); + } + + [Fact] + public void Acknowledge_WhenWriteThrowsArgumentOutOfRangeException_Propagates() + { + RecordingAckServer server = new RecordingAckServer { AuthenticateResult = 5, WriteException = new ArgumentOutOfRangeException("value") }; + using AlarmAckWriter writer = new AlarmAckWriter(server, new MxAccessWriteCompletionCache(), () => { }, TimeSpan.FromMilliseconds(10), "MxGateway"); + + Assert.Throws(() => writer.Acknowledge("T.A.AckMsg", "one", Alice())); + } + + [Fact] + public void Acknowledge_WhenOperatorUserNameIsNull_ThrowsBeforeAuthenticating() + { + RecordingAckServer server = new RecordingAckServer { AuthenticateResult = 5 }; + using AlarmAckWriter writer = new AlarmAckWriter(server, new MxAccessWriteCompletionCache(), () => { }, TimeSpan.FromMilliseconds(10), "MxGateway"); + + Assert.Throws(() => writer.Acknowledge("T.A.AckMsg", "one", new AlarmAckOperator { UserName = null! })); + Assert.DoesNotContain(server.Operations, op => op.StartsWith("AuthenticateUser", StringComparison.Ordinal)); + } + [Fact] public void Acknowledge_WhenWriteThrowsArgumentException_RejectsAndDropsItem() { @@ -290,6 +354,16 @@ public sealed class AlarmAckWriterTests } } +/// ArgumentException with an explicit HRESULT (the setter is protected on net48). +internal sealed class HResultArgumentException : ArgumentException +{ + public HResultArgumentException(string message, int hresult) + : base(message) + { + HResult = hresult; + } +} + internal sealed class RecordingAckServer : IMxAccessServer { private int nextItemHandle = 100; diff --git a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/AlarmAckWriter.cs b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/AlarmAckWriter.cs index fafe755..eed231d 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/AlarmAckWriter.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/AlarmAckWriter.cs @@ -34,6 +34,9 @@ public sealed class AlarmAckWriter : IAlarmAckWriter /// public const int DefaultMaxCachedItems = 256; + /// E_INVALIDARG, which the interop raises as ArgumentException. + private const int EInvalidArg = unchecked((int)0x80070057); + private readonly IMxAccessComObjectFactory? factory; private readonly MxAccessWriteCompletionCache completionCache; private readonly Action pumpStep; @@ -104,12 +107,20 @@ public sealed class AlarmAckWriter : IAlarmAckWriter if (string.IsNullOrWhiteSpace(itemAddress)) throw new ArgumentException("Ack item address is required.", nameof(itemAddress)); if (ackOperator is null) throw new ArgumentNullException(nameof(ackOperator)); + // Resolve every argument before the interop try blocks: their catch + // filters accept E_INVALIDARG ArgumentExceptions as MXAccess failures, so + // an argument error raised by this code must fail before reaching them. + if (ackOperator.UserName is null) throw new ArgumentException("Operator user name is required.", nameof(ackOperator)); + string verifyUser = ackOperator.VerifyUser; + string password = ackOperator.Password ?? string.Empty; + string commentValue = comment ?? string.Empty; + IMxAccessServer mx = EnsureRegistered(); int userId; try { - userId = mx.AuthenticateUser(serverHandle, ackOperator.VerifyUser, ackOperator.Password ?? string.Empty); + userId = mx.AuthenticateUser(serverHandle, verifyUser, password); } catch (Exception ex) when (IsInteropFailure(ex)) { @@ -130,7 +141,7 @@ public sealed class AlarmAckWriter : IAlarmAckWriter return AlarmAckResult.Create( AlarmAckOutcome.AuthenticationFailed, ex.HResult, - $"Operator authentication failed for '{ackOperator.VerifyUser}' (HRESULT 0x{ex.HResult:X8})."); + $"Operator authentication failed for '{verifyUser}' (HRESULT 0x{ex.HResult:X8})."); } if (userId == 0) @@ -138,7 +149,7 @@ public sealed class AlarmAckWriter : IAlarmAckWriter return AlarmAckResult.Create( AlarmAckOutcome.AuthenticationFailed, -1, - $"Operator authentication failed for '{ackOperator.VerifyUser}' (no user id returned)."); + $"Operator authentication failed for '{verifyUser}' (no user id returned)."); } ulong baselineVersion = 0; @@ -177,7 +188,7 @@ public sealed class AlarmAckWriter : IAlarmAckWriter pumpStep(); step = "Write"; baselineVersion = completionCache.CurrentVersion(serverHandle, itemHandle); - mx.Write(serverHandle, itemHandle, comment ?? string.Empty, userId); + mx.Write(serverHandle, itemHandle, commentValue, userId); } catch (Exception ex) when (IsInteropFailure(ex)) { @@ -249,10 +260,23 @@ public sealed class AlarmAckWriter : IAlarmAckWriter /// ArgumentException, E_ACCESSDENIED to UnauthorizedAccessException) and /// the rest to COMException. Each carries the HRESULT in Exception.HResult. /// + /// + /// The ArgumentException arm exists because a wrong password or unknown + /// user makes AuthenticateUser throw ArgumentException with + /// E_INVALIDARG (0x80070057), observed live on windev on 2026-10-19. It + /// is narrowed to that HRESULT, and excludes ArgumentNullException and + /// ArgumentOutOfRangeException, so a programming error inside the try + /// blocks propagates as a bug instead of being reported as an MXAccess + /// authentication or write failure. The interop never raises those two + /// subtypes for a COM HRESULT. + /// private static bool IsInteropFailure(Exception ex) => ex is ExternalException - || ex is ArgumentException - || ex is UnauthorizedAccessException; + || ex is UnauthorizedAccessException + || (ex is ArgumentException + && ex is not ArgumentNullException + && ex is not ArgumentOutOfRangeException + && ex.HResult == EInvalidArg); private static bool IsServerGone(int hresult) => hresult == unchecked((int)0x80010108) // RPC_E_DISCONNECTED