From d52a3b1c64f75363ad27a729cbfa455c2e734f4b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 10 Nov 2026 22:46:51 -0500 Subject: [PATCH] fix(client): Client.Shared EndpointSelector uses the shared OpcUaEndpointUrlRewriter (#812) Client.Shared now project-references Driver.OpcUaClient.Contracts (Core.Abstractions only, no NuGet/SDK; pinned by ContractsIsDependencyLightTests) and delegates the host/port rewrite to OpcUaEndpointUrlRewriter, deleting its own copy. The two copies differed; reconciled toward the client so its pinned behaviour is unchanged (EndpointSelectorRewriteParityTests, written green against the old copy): - no-op rewrite (same host case-insensitively + same port) now returns the advertised string verbatim in the shared rewriter too (it re-serialised: added a trailing slash and case-folded a case-only host difference); - a real rewrite serialises via AbsoluteUri, not Uri.ToString(), which un-escaped the advertised path (/x%20y became /x y) in the driver + browser; - unusable URLs: the rewriter still falls back to the configured URL (driver/browser); the client keeps its loud failure as call-site validation (null -> InvalidOperation, unparseable -> UriFormatException). --- .../Adapters/DefaultEndpointDiscovery.cs | 35 ++++++++++--------- .../ZB.MOM.WW.OtOpcUa.Client.Shared.csproj | 8 +++++ .../OpcUaEndpointUrlRewriter.cs | 28 +++++++++------ .../OpcUaClientEndpointRewriteTests.cs | 22 ++++++++++-- 4 files changed, 64 insertions(+), 29 deletions(-) diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultEndpointDiscovery.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultEndpointDiscovery.cs index d2ade3d82..3b23b5575 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultEndpointDiscovery.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Adapters/DefaultEndpointDiscovery.cs @@ -1,6 +1,7 @@ using Opc.Ua; using Opc.Ua.Client; using Serilog; +using ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient; namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Adapters; @@ -36,8 +37,7 @@ internal static class EndpointSelector /// /// Picks the best endpoint from the discovery response that matches the requested /// security mode, preferring Basic256Sha256, and rewrites the endpoint URL - /// host to match the user-supplied URL when the discovery response advertises a - /// different hostname. + /// host and port to the user-supplied ones via the shared . /// /// Endpoints returned by the discovery query, in any order. /// The endpoint URL the operator supplied; supplies the hostname rewrite target. @@ -85,22 +85,23 @@ internal static class EndpointSelector $"No endpoint found with security mode '{requestedMode}'. Available endpoints: {available}"); } - // Rewrite the endpoint URL host AND port to the user-supplied ones (as the SDK's - // CoreClientUtils.SelectEndpoint does). A server behind a port mapping or internal DNS - // advertises its own host:port (docker-dev containers all say :4840); following that would - // open the session on a different server and fail the secure-channel certificate check. - // Only when the scheme matches: a different scheme (https vs opc.tcp) is a different transport. - var serverUri = new Uri(best.EndpointUrl ?? throw new InvalidOperationException("Endpoint has no URL.")); - var requestedUri = new Uri(endpointUrl); - // A URL typed without a port (opc.tcp has no registered default, so Port is -1) keeps the advertised port. - var port = requestedUri.Port > 0 ? requestedUri.Port : serverUri.Port; - if (string.Equals(serverUri.Scheme, requestedUri.Scheme, StringComparison.OrdinalIgnoreCase) - && (serverUri.Host != requestedUri.Host || serverUri.Port != port)) + // Rewrite the endpoint URL host AND port to the user-supplied ones, keeping the advertised path. A server + // behind a port mapping or internal DNS advertises its own host:port (docker-dev containers all say :4840); + // following that would open the session on a different server and fail the secure-channel certificate check. + // The rule is the ONE shared with the OpcUaClient driver and the AdminUI browse picker (#812, #806, #676): + // same scheme only, configured port only when it names one, the advertised string verbatim on a no-op. + // + // Validation stays here, not in the rewriter: the shared rewriter falls back to the configured URL on an + // unusable advertised one (right for a deployed driver), while this interactive tool reports it — a null URL + // throws InvalidOperationException and an unparseable one (either side) UriFormatException, as before #812. + var advertisedUrl = best.EndpointUrl ?? throw new InvalidOperationException("Endpoint has no URL."); + _ = new Uri(advertisedUrl); + _ = new Uri(endpointUrl); + var rewritten = OpcUaEndpointUrlRewriter.Rewrite(advertisedUrl, endpointUrl); + if (!string.Equals(rewritten, advertisedUrl, StringComparison.Ordinal)) { - var builder = new UriBuilder(best.EndpointUrl) { Host = requestedUri.Host, Port = port }; - best.EndpointUrl = builder.ToString(); - Logger.Debug("Rewrote endpoint from {ServerHost}:{ServerPort} to {RequestedHost}:{RequestedPort}", - serverUri.Host, serverUri.Port, requestedUri.Host, port); + best.EndpointUrl = rewritten; + Logger.Debug("Rewrote endpoint from {AdvertisedUrl} to {RewrittenUrl}", advertisedUrl, rewritten); } return best; diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/ZB.MOM.WW.OtOpcUa.Client.Shared.csproj b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/ZB.MOM.WW.OtOpcUa.Client.Shared.csproj index 4ad02026d..9cbb49092 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/ZB.MOM.WW.OtOpcUa.Client.Shared.csproj +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/ZB.MOM.WW.OtOpcUa.Client.Shared.csproj @@ -12,6 +12,14 @@ + + + + + diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/OpcUaEndpointUrlRewriter.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/OpcUaEndpointUrlRewriter.cs index 003474469..c15217a60 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/OpcUaEndpointUrlRewriter.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/OpcUaEndpointUrlRewriter.cs @@ -5,7 +5,9 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient; /// OpcUaClient driver family. A server reports its GetEndpoints results with ITS view of its own address; behind /// NAT, a port map, a container port publish or a proxy that address is unreachable from here (or bypasses the /// proxy). Shared by the runtime driver (Gitea #676) and the AdminUI live-browse picker (Gitea #806, which used -/// the advertised URL unchanged and failed DNS on a published-port fixture the driver handled). +/// the advertised URL unchanged and failed DNS on a published-port fixture the driver handled), and — since +/// Gitea #812 — by the Client CLI/UI's EndpointSelector (Client.Shared), whose own copy of this rule it +/// replaced. /// /// /// Pure string/ logic, deliberately in .Contracts: it is not session-typed, so it must not @@ -18,8 +20,10 @@ public static class OpcUaEndpointUrlRewriter /// /// The address to open the session on. When the advertised URL's scheme matches the configured one, the /// configured host AND port (when it names one) replace the advertised ones and the advertised path is kept — - /// the SDK's own CoreClientUtils.SelectEndpoint rule. A different scheme is a different transport and is - /// left alone; an unparseable advertised URL falls back to the configured one. + /// the SDK's own CoreClientUtils.SelectEndpoint rule. When host (case-insensitively) and port already + /// match, the advertised string is returned verbatim. A different scheme is a different transport and is left + /// alone; an unparseable advertised URL falls back to the configured one (callers that must fail loudly + /// instead, like the Client CLI/UI, validate before calling). /// /// The EndpointUrl the server returned from GetEndpoints. /// The URL the operator configured (the one GetEndpoints was sent to). @@ -32,12 +36,16 @@ public static class OpcUaEndpointUrlRewriter return configuredUrl; if (!string.Equals(advertised.Scheme, configured.Scheme, StringComparison.OrdinalIgnoreCase)) return advertisedUrl; // A port-less configured URL (Uri.Port == -1 for opc.tcp) keeps the advertised port — copying -1 would drop it - // and the SDK would fall back to 4840 (review of 3b88056f; same fix as the client's EndpointSelector). - var builder = new UriBuilder(advertised) - { - Host = configured.DnsSafeHost, - Port = configured.Port > 0 ? configured.Port : advertised.Port, - }; - return builder.Uri.ToString(); + // and the SDK would fall back to 4840 (review of 3b88056f). + var port = configured.Port > 0 ? configured.Port : advertised.Port; + // Nothing to rewrite: return the advertised string verbatim rather than a re-serialised copy (no added + // trailing slash, no case-folding of a host that only differs in case — DNS is case-insensitive). This is the + // Client CLI/UI's long-standing rule, kept when its own copy was folded in here (#812). + if (string.Equals(advertised.DnsSafeHost, configured.DnsSafeHost, StringComparison.OrdinalIgnoreCase) + && advertised.Port == port) + return advertisedUrl; + var builder = new UriBuilder(advertised) { Host = configured.DnsSafeHost, Port = port }; + // AbsoluteUri, not ToString(): ToString() un-escapes, turning an advertised "/x%20y" into "/x y" (#812). + return builder.Uri.AbsoluteUri; } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientEndpointRewriteTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientEndpointRewriteTests.cs index d6ceebb53..53e9b7a9c 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientEndpointRewriteTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientEndpointRewriteTests.cs @@ -54,10 +54,28 @@ public sealed class OpcUaClientEndpointRewriteTests } [Fact] - public void Host_matching_is_case_insensitive_and_keeps_the_configured_spelling() + public void Host_matching_is_case_insensitive_so_a_case_only_difference_is_not_a_rewrite() { + // #812: unified with the Client CLI/UI rule — nothing to rewrite, so the advertised string comes back verbatim + // (it used to be re-serialised with the configured spelling; DNS is case-insensitive, so the target is the same). OpcUaEndpointUrlRewriter.Rewrite("opc.tcp://PLC-A:4840/Server", "OPC.TCP://plc-a:4840/") - .ShouldBe("opc.tcp://plc-a:4840/Server"); + .ShouldBe("opc.tcp://PLC-A:4840/Server"); + } + + [Fact] + public void A_no_op_rewrite_returns_the_advertised_string_verbatim() + { + // #812: no trailing slash appended when nothing changes (the Client CLI/UI's long-standing behaviour). + OpcUaEndpointUrlRewriter.Rewrite("opc.tcp://server:4840", "opc.tcp://server:4840") + .ShouldBe("opc.tcp://server:4840"); + } + + [Fact] + public void A_rewrite_keeps_the_advertised_path_escaping() + { + // #812: Uri.ToString() un-escapes ("/x%20y" became "/x y"); AbsoluteUri keeps the server's own escaping. + OpcUaEndpointUrlRewriter.Rewrite("opc.tcp://a:4840/x%20y?q=1", "opc.tcp://b:1/") + .ShouldBe("opc.tcp://b:1/x%20y?q=1"); } [Fact]