10 Commits

Author	SHA1	Message	Date
romanett	9b034f8291	Enable nullable reference types centrally; migrate all unset and annotations-only projects (#4700) ... # Description Nullable reference types are now enabled once, in `common.props`, and every project in the repository builds warning-clean with them. **Configuration** - `<Nullable>enable</Nullable>` lives in `common.props`. All per-project `<Nullable>` settings are removed: 178 project files (enable, annotations, and the `disable` in the three Aot test projects). - 419 per-file `#nullable enable`/`restore` directives and the 4 `#nullable disable` directives are removed. - Kept on purpose: - The 5 `<auto-generated>` schema files (`UANodeSet.cs`, `UA Type Dictionary.cs`, `OPCBinarySchema.cs`, `SecuredApplication.cs`, `PInvokeHelper.cs`) keep `#nullable enable`. The compiler ignores the project setting in generated code, so the directive is required there. - `tests/Opc.Ua.Interop.LegacyPeer` keeps `disable`. It builds against the 1.5.378 packages and does not import the repository props. - `docs/DeveloperGuide.md` and `.github/copilot-instructions.md` now describe the central setting. **Migration** (the 43 unset and 13 annotations-only projects, roughly 10k diagnostics) - **Production code: annotated at the declarations.** Covers `tools/Opc.Ua.SourceGeneration.Core`, `src/Opc.Ua.Lds.Server` and the LDS sample. - Members, parameters and returns are nullable where null actually flows. - Null-check helpers carry `[NotNullWhen]` (`IsNull`, `IsValidXmlName`, `IsMethodTypeDesign`). - `CoreUtils.Format`, `Utils.Format`, `ITemplateWriter.WriteLine` take `params object?[]`. `Template.AddReplacement` takes `string?` (rendering already skips null replacements). - `IRegisteredServerStore.RegisterAsync` returns `Task<RegistrationEntry?>`: it returns null when a registration is removed. - Dereferences backed by an invariant (for example, the validated model) use `!`, so runtime behaviour is unchanged. - **Behavioural changes in production code** (small, deliberate): - `NodeSetToModelDesign` throws `InvalidOperationException` when the file is not a NodeSet. Before, it threw a NullReferenceException on the next line. - `ModelDesignValidator` starts its lookup tables empty instead of null. A redundant `if (m_nodes == null) return;` guard is gone. - **Test code: minimal annotations.** - `?` on values that can be null, `!` where the test asserts non-null. - `= null!` for fixture members assigned in SetUp. - `TryGetStructure<T>(out T? x)` where the inferred `T?` broke the `IEncodeable` constraint. - A few redundant `== null` checks on structs (`NodeId`, `ArrayOf<T>`) folded into the adjacent `IsNull`/`Count` checks. **Notes for reviewers** - The diff is large (~1,500 files) but almost entirely mechanical: `?`, `!` and directive removal. The non-annotation changes are listed above. - Known follow-up, not done here: `IModelDesign.Namespaces/TargetNamespace/NamespaceUris` stay non-nullable even though the validator returns null before it loads a dictionary. Making them nullable would ripple through every generator call site. ## Related Issues - Fixes #4439 ## Checklist - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [ ] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. The "no new warnings" box covers net10.0 and net48: full non-incremental builds of `UA.slnx`, `tools/SourceGeneration.slnx`, `tools/MigrationAnalyzer.slnx` and the Aot test projects finished with 0 warnings and 0 errors under the repository's warnings-as-errors settings. net9.0 and net8.0 also had no nullable diagnostics, but they were not rebuilt after the final fixes. Tests have not been run locally yet. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>	2026-10-08 08:48:13 +02:00
romanett	35c08874ca	Server view parameter status codes; client transfer fallback when GetMonitoredItems is unavailable (#4654) ... Two defects found by interop testing the 2.0 stack against foreign OPC UA stacks (node-opcua, Milo, asyncua, open62541, async-opcua, gopcua). ## 1. Server: view timestamp/version without a ViewId returned Bad_ViewIdUnknown A Browse with a `ViewDescription` whose `ViewId` is null but whose `Timestamp` (or `ViewVersion`) is set got `Bad_ViewIdUnknown` from the node managers. `ViewDescription.IsDefault` is false, so `ValidateViewDescription` looked up the null `ViewId` and failed with "unknown view". Spec (OPC 10000-4): - §7.44: a null `viewId` means the entire AddressSpace. Timestamp and viewVersion each select a version of the View, and "either the Timestamp or the viewVersion parameter may be set by a Client, but not both". - Table 178 / Browse service results (§5.9.2.3): `Bad_ViewTimestampInvalid` = "the view timestamp is not available or not supported", `Bad_ViewVersionInvalid` likewise, and `Bad_ViewParameterMismatch` = the view parameters are not consistent with each other. Fix: a new internal `ViewDescriptionValidator.ValidateParameters` handles the checks that don't depend on the View node: - timestamp **and** version set → `Bad_ViewParameterMismatch` (before this, a known view got `Bad_ViewTimestampInvalid`) - null ViewId + timestamp → `Bad_ViewTimestampInvalid` - null ViewId + version → `Bad_ViewVersionInvalid` It is called at service level in `NodeManagerServiceDispatcher.BrowseAsync`, because these are Browse *service* results. It is also called in `CustomNodeManager2.ValidateViewDescription`, `AsyncCustomNodeManager.ValidateViewDescription` and the `DiagnosticsNodeManager` override, so a node manager called directly answers the same way. TranslateBrowsePaths and RegisterNodes take no ViewDescription, and BrowseNext reuses the validated view from the continuation point. ## 2. Client: transfer failed when the optional Server.GetMonitoredItems is unavailable After `TransferSubscriptions` returns Good, the client calls `Server.GetMonitoredItems` (i=11492) to map server handles to client handles. That call is only made for a subscription that is not live in this client: one restored from saved state, or a clone. Its own live subscriptions skip it. `GetMonitoredItems` is an **Optional** method of ServerType (OPC 10000-5 §6.3.1, §9.1), and several stacks don't provide it: asyncua 2.0.1 answers `BadNothingToDo`, open62541 built without `UA_ENABLE_XML_ENCODING` answers `BadInternalError`, and others answer `BadMethodInvalid` or `BadNotSupported`. When the call failed, the whole transfer was reported as failed ("SubscriptionId N could not be moved") even though the server had moved the subscription. For non-"definitive" codes such as BadNothingToDo or BadInternalError, the transferred server subscription was also left behind as an orphan. ### Chosen fallback The status of a failed call is now classified (`GetMonitoredItemsFallback.IsTransientFailure`): - **Transient** (timeout, busy server, too many operations, lost connection, channel or session): behaviour is unchanged. The transfer fails and nothing is deleted, so it can be retried. - **Anything else** (method missing, not supported, BadNothingToDo, BadInternalError, unusable output): the server does not provide the method. Then: 1. **The client knows server ids for its items** (the usual case: restored state, or a reconnect of its own subscription). Use that mapping and the transfer succeeds. A transfer keeps item ids and client handles, which is why OPC 10000-4 §6.8 tells clients to store the SubscriptionId together with the MonitoredItem client and server handles. Items saved without an id were never created on the server, and the normal apply path creates them, as before. 2. **No server ids are known** (a clone of a live subscription, `CloneSubscriptions`). Notifications would still arrive, because clones copy the client handles. But the items could never be modified or deleted, and recreating them would duplicate them on the server, since stale items can't be deleted without their ids. So: - classic engine, when the caller just moved the subscription (`Session.TransferSubscriptionsAsync`): delete the transferred server subscription and **recreate it in place** with the same items and client handles. The transfer reports success, but notifications the server still had queued for the old subscription are lost. The subscription gets a new id. - classic `Subscription.TransferAsync` (also used to adopt the session's own subscriptions, so nothing was moved): fails as before, and nothing is deleted or created. - new engine (`MonitoredItemManager.TrySynchronizeHandlesAsync`): fails as before. Its callers (`SubscriptionManager`) already recreate the subscription in that case. Both engines now treat an unusable GetMonitoredItems answer (wrong output arguments) the same way as an unavailable method. The two existing `TryCompleteTransferAsyncShouldReturnFalseWhenResponseWrong{1,2}Async` tests were updated and renamed to match. The existing classic test that expected a "definitive" failure to delete the subscription now checks the fallback to the known ids instead. Warnings are logged when the fallback is used (`Subscription + 71..73`, `MonitoredItemManager + 6`). ## Tests - Server: `AsyncCustomNodeManagerTests.BrowseAsyncNullViewIdWithVersionParametersReturnsViewParameterErrorAsync` (sync and async node manager, timestamp, version, both) and `MasterNodeManagerDeterministicTests.BrowseAsyncNullViewIdWithVersionParametersThrowsViewParameterError` (service level). - Classic client (`ClassicSubscriptionCoverageTests`): fallback to known ids for every non-transient code, while transient codes still fail; a Bad method result (asyncua style); clone recreated in place (old subscription deleted, items created with the original client handles); clone adopted through `TransferAsync` is neither deleted nor recreated. - New engine (`SubscriptionTests`): known ids used for Bad / BadMethodInvalid / BadNotSupported / BadNothingToDo / BadInternalError; transient failure and unknown ids still fail. Run locally (net10.0, sequential): - Opc.Ua.Client.Tests: 2745 passed / 0 failed - Opc.Ua.Server.Tests: 8089 passed / 0 failed / 5 skipped - Opc.Ua.Subscriptions.Durable.Tests: 36 passed / 11 skipped - Opc.Ua.InformationModel.Tests (View/Browse filter): 289 passed / 4 skipped - Opc.Ua.Sessions.Tests (Transfer/Reconnect filter): 155 passed / 103 skipped - Opc.Ua.Subscriptions.Tests (Transfer/Durable filter): 15 passed / 1 skipped - net48 build of both test projects: clean The interop test that found (2), `ServerFeatureInteropTests.TransferSubscriptionToNewSessionAsync`, is in the uncommitted foreign-interop work and isn't on master yet. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Marc Schier <marcschier@users.noreply.github.com>	2026-10-04 19:01:38 +00:00
romanett	a0d569d756	Fix Server and GDS audit findings, review findings and spec-conformance gaps (#4571) ... ## Summary Fixes the findings of a bug audit of `Opc.Ua.Server`, `Opc.Ua.Gds.Server` and `Opc.Ua.Gds.Client`. Rebased on master after #4516. What went in: - **Audit:** 93 findings, each checked against the OPC UA specification. - **Code review of those fixes:** 35 further findings. - **Spec grounding of every fix commit:** 18 conformance gaps, now closed. Commits are one per finding, named `Fix <ID>`, `Fix review <ID>` or `Fix spec gap <name>`. The IDs refer to the audit report (S = Server areas, G1 = GDS server, G2 = GDS client). Validation: - Each finding was traced in code and checked against OPC 10000 (parts 3, 4, 5, 9, 11, 12, 13, 18, 20). - Every fix commit was checked against the spec text again afterwards: 0 non-conformant, remaining gaps fixed. - Regression tests were added for most fixes. - Suites run on net10.0 before the rebase: Server, GDS, History, Configuration, Features, Core.Security, DI, Redundancy, LDS, InformationModel, PubSub.Server, Client, Sessions and the three Subscriptions suites, all green. The same suites on the rebased branch are still running; results will be posted as a comment. ## Behaviour and user-facing changes ### Secure defaults - **`Historize()` / `HistorianBuilder`** grant HistoryRead only. History writes need an explicit `historyAccessLevel: HistoryRead | HistoryWrite`. - **File-system mount:** - Anonymous sessions can no longer create, delete, move or open files for writing (`Bad_UserAccessDenied`). - UserWritable and UserExecutable now reflect the user. - `AllowAnonymousWrite` restores the old behaviour; the ReferenceServer sample turns it on. - **Default PKI root** for the hosted server, hosted GDS, hosted LDS and `OpcUaApplicationConfigurationProvider`: - Moved from the shared temp directory to `{LocalApplicationData}/OPC Foundation/{ApplicationName}/pki`, and startup fails if no per-user directory exists. - When certificate stores of an earlier version still exist under temp, a warning names both paths. Existing deployments that relied on the default get a new application certificate (and the hosted GDS a new CA) unless `PkiRoot` is set to the old path. - **GDS:** - `GdsServerOptions.AutoApprove` defaults to `false`. - The default KeyCredential store grants no roles unless `AuthorizeRoles` allows them. - Certificate requests (StartSigningRequest, StartNewKeyPairRequest, FinishRequest) need an encrypted channel. - **Hosted server anonymous logins:** - Anonymous is rejected (`Bad_IdentityTokenRejected`) when the default authenticator options disable it. - A custom `AddIdentityAuthenticator<T>()` on its own no longer disables anonymous. - With anonymous disabled and no configured token policies, the endpoint advertises the authenticators' token types instead of Anonymous. - **Runtime-added roles:** their properties and methods need SecurityAdmin over an encrypted channel, like the well-known roles. - **Disabled users:** users disabled through Part 18 UserManagement can no longer log in through `RegisterDefaultAuthenticators(IUserDatabase)`. ### GDS server (Part 12) - **Certificate requests:** - FinishRequest / ReadRequest with another application's RequestId returns `Bad_InvalidArgument`. - Every Start*Request returns a new RequestId. - A CSR without the ApplicationUri is rejected (`Bad_CertificateUriInvalid`). - An abstract or unsupported certificate type is rejected at Start (`Bad_InvalidArgument`). - **Private-key password:** no longer persisted in the JSON applications database. A password saved by an earlier version is read into memory once and dropped on the next save. - **CheckRevocationStatus:** validates offline against the GDS's own trust lists and CRLs. It returns Good for good GDS-issued certificates and `Bad_CertificateInvalid` for input that is not a certificate. - **Certificate-group trust lists:** - Writes need CertificateAuthorityAdmin; SecurityAdmin and application admins are read-only. - Writes are validated with the Part 4 certificate validation. - **AuthorizationService:** - The legacy RequestAccessToken applies `AccessControl` and issues the token for the session identity. - StartRequestToken passes the session identity to every identity-aware provider. - Unknown ResourceIds return `Bad_NotFound`. - Expired token requests and refresh tokens are pruned, with at most 10 000 pending requests (`Bad_TooManyOperations`). - **KeyCredential:** requests with PublicKey + SecurityPolicyUri are rejected with `Bad_SecurityPolicyRejected` (secret encryption is not implemented); a PublicKey without SecurityPolicyUri is rejected with `Bad_InvalidArgument`. - **Audit events:** access-token audit events no longer contain the password or token data. - **RegisterTickets / UnregisterTickets:** need the RegistrarAdmin role when called from a session. - **Managed application nodes:** moved from namespace 0 to their own namespace (`ManagedApplicationsNamespaceUri`); the NodeId is the ApplicationUri, and the mandatory child properties exist. - **Hosted GDS:** - New `GdsServerOptions.CertificateGroups` (`GdsCertificateGroupOptions`), bindable from configuration. - The mandatory DefaultApplicationGroup is always present. - The default CA subject uses the resolved application name. - **JSON applications database:** - A corrupt file throws instead of being silently replaced by an empty database. - Saves are atomic and write in place on bind mounts; on net8+ builds they also follow symlinks and keep the Unix file mode. - Issued certificates and trust-list links survive a reload. ### GDS client - **Trust-list download:** - Reads until an empty chunk (Part 20 short reads). - A size-limit rejection of the final end-of-file read, as sent by released servers, counts as end of file. - **Default certificate group:** a null group means DefaultApplicationGroup, and vendor groups in other namespaces are no longer mistaken for well-known groups. - **Status codes and errors:** - Missing directory proxies report `Bad_ServerNotConnected`. - Short or wrongly typed method outputs throw `BadUnexpectedError` / `BadTypeMismatch`. - **GdsAccessTokenProvider:** no longer caches a failed client task, and one caller's cancellation no longer fails other callers. ### Trust lists, push and ConfigurationFile (Part 12 / Part 20) - **AddCertificate:** - `IsTrustedCertificate=false` returns `Bad_CertificateInvalid`. - Added certificates are validated (Part 4), and a certificate revoked by a current CRL returns `Bad_CertificateRevoked`. - **CloseAndUpdate:** validates the uploaded list against its own certificates and CRLs, not the stores it replaces. Validation failures are no longer recorded in the server's Rejected store. - **RemoveCertificate:** removing a trusted or issuer CA that other certificates still need returns `Bad_CertificateChainIncomplete`. - **Handles:** - Several read handles may be open at once. - Any open while a write handle exists returns `Bad_NotReadable` / `Bad_NotWritable`. - A write open while any handle is open returns `Bad_NotWritable`. - Handles of closed sessions are released. - Idle handles close after ActivityTimeout (default 60 s). - **Status codes:** - Read clamps to the remaining bytes and no longer enforces the size limit. - Oversized Write / AddCertificate returns `Bad_RequestTooLarge`. - Unsupported open modes return `Bad_NotSupported`. - **ConfigurationFile.CloseAndUpdate:** - Empty Targets return `Bad_InvalidArgument`. - The ConfigurationUpdatedAuditEvent is raised with the configuration owner as source. - With the new `IApplicationConfigurationFileTargetProvider`, providers receive the Targets, a bad target returns Uncertain with per-target results, and session-interrupting updates are applied after RestartDelayTime. - **Default namespace permissions:** namespaces registered after the first permission lookup now get their NamespaceMetadata default RolePermissions and AccessRestrictions (no stale negative cache). ### Sessions, identity and roles (Part 18) - **Session timeouts:** a failure closing one timed-out session no longer stops the session-timeout monitor. - **ChangePassword:** an empty old password returns `Bad_IdentityTokenInvalid`. - **Removing or disabling a user:** closes only that user's UserName-token sessions. - **Role configuration changes:** roles granted by an `ImpersonateUser` mapping are kept. - **X509Subject rules:** a certificate subject value containing a quote never matches an X509Subject rule. - **`ImpersonateUser` callback:** no longer runs under the global session lock. - **Session diagnostics:** LocaleIds are kept as the client sent them, while null LocaleIds are ignored for translation. ### Subscriptions and monitored items (Part 4) - **Acknowledgements:** a failing acknowledgement returns a per-entry Bad status instead of failing the whole Publish. - **`DiscardedMessageCount`:** counts messages dropped from the retransmission queue and unsent overflow messages. - **Durable subscriptions:** transferring a restored anonymous subscription works (no NullReferenceException). - **Transferred subscriptions:** sampling and permission checks use the item's current owner session. - **Access denied:** a monitored item whose user may no longer read the attribute gets a `Bad_UserAccessDenied` notification instead of going silent. Non-Value attributes need Browse permission (RolePermissions needs ReadRolePermissions), and User* attributes are evaluated per subscriber. - **ModifyMonitoredItems:** a SamplingInterval of -1 uses the subscription's publishing interval. - **AggregateFilter priming:** needs HistoryRead / ReadHistory permission and is bounded (100 000 values, bounded page size). - **Resource leak:** event monitored items no longer leak a consumer task per create/delete cycle. ### Node management services (Part 3 / Part 4) - **Reference types:** AddNodes, AddReferences, DeleteReferences and Browse reject ids that are not ReferenceTypes with `Bad_ReferenceTypeIdInvalid`. AddNodes, AddReferences and DeleteReferences also reject abstract ReferenceTypes; Browse still accepts them (e.g. `HierarchicalReferences` with includeSubtypes). - **Concurrent AddNodes:** duplicate BrowseNames under one parent get `Bad_BrowseNameDuplicated`. - **TranslateBrowsePathsToNodeIds:** - Requires Browse permission on intermediate nodes and final targets (RolePermissions and AccessRestrictions). - A long RelativePath no longer rejects the whole request. - **SemanticChangeEvent:** - Raised once per actual change of a semantic property, whether or not it is monitored. - Also raised for properties with the SemanticChange access-level bit. - Reports the owning node's TypeDefinition as AffectedType. - **Localized diagnostics:** operation-level text is translated when OperationLocalizedText is requested. ### History and aggregates (Part 11 / Part 13) - **DeleteRawModified:** an unspecified or reversed range returns `Bad_InvalidTimestampArgument`, and StartTime == EndTime deletes the value at StartTime for every provider. - **ReadAtTime fallback:** reads only the values around each requested time. A time whose bound is not found within the search limit returns `Bad_BoundNotSupported` instead of failing the request. - **ReadProcessed:** a ProcessingInterval shorter than one tick returns `Bad_AggregateInvalidInputs`. - **Delta / DeltaBounds:** widen to the next larger signed type when the result does not fit the source type (e.g. a decreasing unsigned counter, or an Int16 overflow). - **Range / Range2:** return the source data type. ### Alarms and fluent API (Part 9) - **Suppression:** an alarm stays suppressed while any suppression group is active, and re-evaluation without a state change does not move TransitionTime. - **`MonitorVariable(variable)`:** sets SourceNode, SourceName and InputNode to the variable and makes it a ConditionSource (HasCondition to the alarm, HasEventSource from the object). - **Fluent input/output arguments:** InputArguments / OutputArguments added through the fluent API to a registered method are readable. - **Root notifiers:** a failed root-notifier registration no longer leaves an orphaned event worker. - **State-machine handlers:** exceptions are logged at Error. ### File system (Part 20) - **Delete / MoveOrCopy:** - Only act on direct children of the directory. - A NodeId whose kind does not match the entry returns `Bad_NotFound` (a file NodeId can no longer delete a directory). - Copying a directory into its own subtree returns `Bad_InvalidArgument`. - **Locking:** - An open file, or a directory containing one, cannot be deleted or moved (`Bad_InvalidState`). - The check is atomic with opens. - While a Delete or Move runs, read opens get `Bad_NotReadable` and write opens `Bad_InvalidState`. - **Open:** - Reserved mode bits or EraseExisting without Write return `Bad_InvalidArgument`. - Read+Write returns `Bad_NotSupported`. - Read while a writer is open returns `Bad_NotReadable`. - **Client `FileSystemErrors`:** maps `Bad_NotReadable` to `IOException`. - **RuntimeNodeSet:** well-formed NodeSets with a stray `Model` element, or two models in one file where one requires the other, now load. ### Server core, hosting and lifecycle - **`SessionEstablishmentQueueLimit` > 0:** CreateSession/ActivateSession now queue for a permit. The permit is acquired before the request is registered, and a waiting handshake releases its worker through the request-parking policy. - **Node manager lifecycle:** - Add, reload and remove no longer wait for parked Publish requests. - A Migrate reload racing with CreateMonitoredItems no longer blocks later lifecycle operations. - Late items on retired or removed managers get `Bad_NodeIdUnknown`. - **HistoryUpdate:** a batch mixing data and event details is bounded by both limits. - **Server stop/start:** stopping the server disposes the configuration watcher and certificate subscription; starting recreates them. - **Monitored-item queue factory:** a factory supplied by the caller or the DI container is no longer disposed by the server. - **Audit events:** each certificate validation error raises its own AuditCertificate event. A failed OpenSecureChannel without a service result is audited as failed. ## New public API (additive) - `IQueuedSessionEstablishmentLimiter` - `StandardServer.OwnsMonitoredItemQueueFactory` (protected virtual) - `DefaultPkiRoot.Get` (Opc.Ua.Configuration) - `TrustList.SetCertificateValidation` - `NamespaceMetadataRegistry.Invalidate` - `IApplicationConfigurationFileTargetProvider` and `ApplicationConfigurationUpdatePlan` - `GdsServerOptions.CertificateGroups` / `GdsCertificateGroupOptions` - `InMemoryKeyCredentialRequestStore.AuthorizeRoles` - `DefaultManagedApplicationsNodeManager.ManagedApplicationsNamespaceUri` - `AuthorizationServiceManager.RequestAccessTokenAsync(..., IUserIdentity?, ...)` - `AllowAnonymousWrite` on the file-system node manager and its factory - `IFileSystemHost.TryBeginMutation` / `EndMutation`, replacing `HasOpenHandles` ## Not fixed / known limitations - **KeyCredential:** secret encryption with the client's PublicKey (requests needing it are rejected). - **CSR SAN restriction:** restricting subject / SAN values copied from a CSR (CA policy). - **Ticket signatures:** ticket signature verification (needs a trusted-signer configuration). - **FileType Read+Write open:** needs a read/write stream contract in `IFileSystemProvider`. - **Custom `IUserDatabase` implementations:** the disabled-user check still scans all users per login. The built-in stores do a single lookup. - **64-bit Delta precision:** limited by the calculator's double arithmetic. ## Tests with changed expectations Existing tests were updated where they asserted the old behaviour: - File-system status codes - Trust-list handle eviction - `DiscardedMessageCount` - Localized diagnostics bits - Delta on unsigned sources - SourceNode of fluent alarms - ReferenceServer transfer permission test (now expects `Bad_UserAccessDenied` notifications) - `UserDeactivationShutdownReviewTests` (the revoked session is a UserName login) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>	2026-09-29 07:26:05 +02:00
Marc Schier	138086863b	Fix audit findings and review regressions across the OPC UA stack (#4503) ... ## Scope Audit fixes and review follow-ups across the OPC UA Core, Client, Server, transport, and redundancy components. This includes implemented Tier 1, Tier 2, and Tier 3 work; it does not claim that every item in the 244-finding audit backlog is complete. The review batch fixes mandatory full-key record protection and owned plaintext buffers; transactional pending-certificate claims; shared-channel nonce and certificate ownership; discovery and token-policy validation; reconnect/callback ordering without changing unbounded buffering defaults; server permissions, filters, monitored-item lifetime, and alarm topology; historian paging, retention, and annotations; atomic user/password metadata; and canonical file identities with bounded alias and role refresh workers. ## Review-batch verification at `ab686ccae` The following results come from fresh test builds using `dotnet test`, `--no-restore`, `-m:1`, and an explicit `CustomTestTarget`. They replace the earlier description's library-build/`--no-build` totals, which did not establish the state of the merged test sources. | Scope | Target | Passed | Skipped | |---|---|---:|---:| | Core review regressions | net10.0 | 827 | 0 | | Client and lifecycle regressions | net10.0 | 630 | 0 | | Server review regressions | net10.0 | 1970 | 0 | | Redundancy Server / Client / PubSub | net10.0 | 276 / 10 / 11 | 0 | | Sessions / file-and-alias / streaming / HTTPS-WSS integration | net10.0 | 8 / 21 / 30 / 20 | 0 | | Core review regressions | net48 | 786 | 39 | | Client recovery and policy regressions | net48 | 54 | 0 | | Server ownership, aliases, queues, and user-state regressions | net48 | 137 | 0 | Every listed run exited successfully. The 39 net48 skips are existing platform/capability cases. New key-agreement tests assert unavailable-policy behavior on unsupported targets rather than adding skips or pretending those targets performed an unsupported handshake. Additional exact node/permission/filter/alarm checks passed 13 Core and 46 Server cases. The PR-scoped formatter verification covers 26 projects and 389 non-generated C# files. A Roslyn syntax audit covers 53 changed interfaces and 263 members, with no missing parameter/type-parameter/return contracts and a 120-column limit for their declarations and documentation. No full-solution, all-target-framework, or NativeAOT matrix is claimed from these local runs; the new CI run must establish its own result. ## CI follow-up at `e1bc3ac3b` The follow-up fixes the failures reported by Azure build 18400. TLS certificates retain configured DNS names, including mixed-case URI schemes, without relaxing hostname validation. Reverse-WSS fixtures use the DNS identity in the client certificate. Reconnect completion is published only after cycle cleanup, so a subsequent reconnect cannot join an old completed cycle and retain old certificates. Certificate-alarm roots now support event subscriptions. The pump sample has one continuous notifier path, with no duplicate shortcut. Historian and aggregate fixtures share the provider's retention clock. Alarm collection lets the server expire idle Publish requests rather than canceling only the client wait. The corrected server, WoT, and managed-session fixtures exercise valid requests and initialized lifecycle contracts. Downlevel crypto tests preserve real credential and key-ownership assertions without enabling unsupported raw-ECDH policies or adding skips. Fresh local evidence after integration includes: | Scope | Target | Passed | Skipped | |---|---|---:|---:| | Client lifecycle / channel ownership / builder fixtures | net10.0 and net48, each | 96 / 103 / 123 | 0 | | Combined server lifecycle, events, aggregates, crypto, and contracts | net48 | 358 | 0 | | Full lifecycle plus certificate-alarm and aggregate fixtures | net10.0 | 116 | 0 | | Final server-contract and history-routing delta | net10.0 and net48, each | 84 | 0 | | Subscription / XRegistry / WoT event regressions | net10.0 | 7 / 2 / 3 | 0 | | Full Sessions project | net10.0 | 780 | 379 | | Full ComplexTypes project / downlevel resolver fixtures | net10.0 / net48 | 2854 / 15 | 0 | | Full WoT project / downlevel structured-write fixtures | net10.0 / net48 | 1496 / 27 | 0 | | TLS domain-name regressions | net10.0 and net48, each | 21 | 0 | All listed runs exited 0. The Sessions skips come from existing endpoint and policy capability guards; they are not counted as passes. No skip was added. Worker validation also covered full DI (426 passes), full History (540 passes, 21 existing skips), 38 exact downlevel event/history failures, and 60 repeated certificate-rotation race cases. The final crypto fixtures passed 73 cases on each target with the unchanged leak detector; a temporary premature-disposal mutation made all three borrower tests fail and was then removed. All 29 C# files in this follow-up pass included-file formatting at info severity across 12 projects. The complete PR interface audit now covers 54 interfaces and 271 members with zero missing contracts or line-width findings. No full-solution or NativeAOT matrix is claimed. New-head CI has started and remains authoritative for the remote result. ## Test-result gate follow-up at `4b6e1203a` Azure build 18401 reported the net48 Client job as successful despite a test-host crash after only 264 passing tests. Its old evaluator counted per-test failures but ignored the failed run verdict. This was a partial run, not a benign exit after the suite had completed. The shared result evaluator now rejects aborted, errored, incomplete, malformed, or inconsistent TRX reports. Each file must describe a completed run, so a passing file cannot mask another file's failure. It retains support for completed runs followed by a benign nonzero host exit, existing skipped-result conventions, and no-TRX MTP jobs. All 78 regression cases pass after integration using Windows PowerShell 5.1. Real completed Sessions and Client reports also pass. The process-test fixture follows the existing NET10_0-only tools-test pattern; its net48 build is not counted as a test run. Formatting passes for the added test file. The Client host crash itself has not reproduced locally. The isolated fixture passed 59 tests, and two fresh full fast-CI selections each completed with 2,392 passes and three existing skips, including a run with crash collection enabled. The CI merge tree is identical to the tested source tree. No speculative runtime change was made, and the original partial CI run is not claimed as a full-suite pass. Azure build 18402 has now completed the full net48 Client run: 2,392 passed, three existing console skips, and a successful test-host exit. The corrected evaluator independently validated outcome=Completed. Across the matrix, 119 test-project jobs succeeded; Sessions net10.0 was canceled when the remote provider deprovisioned its runner, so the overall Azure pipeline was canceled. That cancellation is not claimed as a code failure or a successful run. CodeQL and three Docker image jobs were still running at the latest check. The migration discovery failure remains separate. ## Documentation follow-up at `57ed02bea` The environment blocker has been removed by the maintainer. The three bundled migration references now exactly match the canonical documents after deterministic link rewriting; all 15 bundled documents pass `validate-migration-plugin.ps1`. The canonical migration prose was not rewritten. `FileSystemClient.md` now documents that extended Windows roots preserve literal trailing-dot names, completing the filesystem review follow-up. The filesystem thread has been replied to and resolved after publication. The review pass now has 108 of 110 threads resolved. The two explanation-only threads remain open as requested; no unfinished change request is being represented as resolved. ## Cross-platform follow-up at `0d9d936a8` With migration discovery restored, the Linux and macOS test matrix exposed two stale durable-subscription expectations. The expected value 83,442 encoded the old 32-bit overflow when converting 3,600 hours to milliseconds. The existing overflow-safe server correctly returns 14,400,000 lifetime intervals at a 900 ms publishing interval. Only those two expected constants changed; production behavior, negotiated-hour checks, and all other assertions remain unchanged. The exact local lifetime selection reproduced both failures before the correction. The full durable suite now passes 36 cases with 11 existing skips on each of net10.0 and net48. All four lifetime cases executed and passed on both targets, and included-file info-severity formatting passes. ## Session shutdown follow-up at `8daf7f091` A late net48 Sessions failure exposed a shutdown race. Canceling an in-flight keepalive read can produce a transport `ServiceResultException`. The retiring worker treated its own cancellation as proof that the peer was unavailable, so `CloseAsync` or `DisposeAsync` could skip the server-side `CloseSession` request. The worker now ignores that service error only when its own token is canceled; genuine peer errors still update liveness. The full integration fixture reproduced the original error-code mismatch. Four deterministic mock-transport cases then produced three failures and one passing genuine-error control before the fix. They verify successful close, returned server error, asynchronous disposal, and unchanged behavior for a real keepalive failure. Fresh related runs pass 75 client and 101 integration cases on each of net48 and net10.0, with no skips. Ten repetitions of the formerly flaky full net48 fixture pass all 1,010 cases. All three changed files pass info-severity formatting, and temporary diagnostic instrumentation was removed. ## Atomic listener snapshot follow-up at `82dc22b5d` Ubuntu Core CI exposed an unsafe formatter rewrite in TCP listener disposal: `ConcurrentDictionary.ToArray()` had become a collection spread. A concurrent channel removal between the spread's count and copy operations could leave a default entry, causing a null dereference during disposal. The listener now uses the dictionary's atomic `Values` snapshot and disposes those channels outside the listener lock. Deterministic regressions close zero, one, or all channels at the copy boundary. Before the fix, the one/all-removal cases reproduced the exact null dereference; the unchanged-count control passed. All 23 admission/listener cases now pass on net10.0 and net48, and the full fast Core net10.0 suite passes 5,267 cases with 88 existing platform skips. Both changed files pass info-severity formatting. A bounded audit of the formatter's other snapshot rewrites found no further verified unsafe concurrent-registry copies. ## Shutdown-test observation follow-up at `58403ee8b` The net48 shutdown-order test raced its own observation: it read `m_publishWorkerTask` after starting shutdown, which can legitimately drain the worker and clear that field immediately. The test now captures the worker before shutdown starts and confirms it is initially running. It still requires the worker to finish while shutdown remains blocked by the held manager semaphore. Production behavior is unchanged. A temporary scheduling perturbation reproduced the exact null-field failure. The corrected capture passed 100 perturbed iterations; both the temporary repeat and yield were then removed. The final 126 subscription/lifecycle cases pass on each of net48 and net10.0, and formatting passes. A complete local Sessions net10.0 run also passed 780 cases with 379 existing capability skips in 7m20s after the earlier keepalive correction. ## Retirement and alias-startup follow-up at `1a048ff39` The shadow-generation test now observes its documented asynchronous cleanup through the existing bounded helper. Removing the replacement does not join a background drain that already claimed the retired generation. The test still checks live-registration removal immediately and requires the old node to disappear; no timeout or production lifecycle behavior changed. All 81 lifecycle cases pass on each of net10.0 and net48. Initial alias-node materialization now retries one timed-out, read-only snapshot query before creating nodes for that root. The retry is logged through source-generated logging. Each query retains the original shared 100 ms deadline; repeated timeouts, other errors, and cancellation still abort startup. Client searches and live refresh do not gain this immediate retry. Fault-injection tests cover success after one timeout, a persistent timeout, a different store failure, and cancellation. All 187 related alias/hosting cases and the original certificate-rotation integration case pass on each target; the positive test records exactly one retry warning. The alias documentation now accurately describes the shared request budget. All changed C# files pass formatter checks, and the complete 54-interface/271-member documentation audit passes. Fresh CI is running on `1a048ff39`; the previous completed matrix exposed only the two failures corrected in this batch and their dependent summaries. ## Sample endpoint allocation follow-up at `d08a94925` The strong-cluster test harness allocated OPC UA and Raft endpoints in separate released-port batches. Uniqueness was enforced only inside each batch, so the same OS-assigned port could appear in both protocol groups. The harness now requests one distinct batch for both groups and splits it before launching any replicas. Deterministic one- and three-replica allocation cases reproduced overlapping groups with the old plan and pass with the corrected plan. The complete 17-case short-haul sample suite passes without skips; all failover, historian-continuation, and service assertions remain unchanged. Both changed files pass info-severity formatting. This prevents intra-cluster self-collisions but does not claim to reserve released ports against unrelated external processes. ## Code-review follow-up at `085fd9636` A review of the complete branch diff found five defects this PR introduced. All are now fixed, each in its own commit. `RelatedTo`'s recursive-to-iterative rewrite defaulted the optional `includeTypeDefinitionSubtypes` and `includeReferenceTypeSubtypes` operands to `false`. Part 4 §7.7.4 specifies both default to `true`, so a filter that omitted them silently stopped matching subtypes. Dynamically created key-credential nodes derived their namespace from the containing folder, which is the standard `i=18155` node in namespace 0 — the namespace reserved for the OPC UA standard address space; they now resolve the server-owned namespace and fail with `BadInternalError` rather than guessing index 1. The retained audience-less `CreateTokenData` overload produced tokens that verification always rejects, so it is now `[Obsolete]` and throws instead. A failed reference to a remote parent left the new node registered with a one-sided inverse reference, and is now rolled back. An existing `.pfx` is restricted before it is rewritten, rather than after its new key bytes are on disk. One further reported finding was rejected after investigation. `JwksIssuerKeyResolver` deliberately does not dispose retired key snapshots: `RetiredKeysAreCollectibleWhileResolverRemainsAliveAsync` proves the resolver retains no retired snapshot, and `ReturnedKeysSurviveRepeatedRefreshAndResolverDisposalAsync` requires already-returned keys to keep verifying across refreshes and resolver disposal. Adding disposal would have broken that tested contract, so the exploratory change was reverted. Validated on net10.0 and net48: 256 Core and 633 Server cases pass, with one pre-existing net48 skip. The build reports zero compiler and analyzer diagnostics, all eight included-file info-format checks pass, the contract audit covers 58 interfaces and 310 members with no findings, and the 15 bundled migration documents validate. All 122 review threads are resolved; the key-credential namespace thread was reopened to retract an earlier incorrect answer and then resolved against the real fix. ## Review-body follow-up at `da0d59cfb` The re-review at `8daf7f091 + 82dc22b5d` listed four items in its summary that had no inline thread of their own, so they had not been picked up. All four are now fixed. Both event-dispatch permission gates had been tightened from not-Bad to `IsGood`, which silently dropped events whose permission verdict is Uncertain; the data-change path in the same file still treated Uncertain as deliverable, so the two had diverged. `EventSourceRegistry` only cleared `ConsecutiveFailures` when a source was deactivated, so a source that had backed off to the 30-second cap kept that delay for the rest of the process even after starting successfully. `UserNameIdentityTokenHandler` built a temporary issuer chain whose `Add` calls take independent certificate references, and never released them. The fourth was the receiver-side sequence rollover, which the reviewer had correctly re-flagged as still open despite its thread being resolved. `m_sequenceRollover` was set permanently, so the second legal wrap on a long-lived channel was rejected and faulted the channel along with the transport-reconnect OPN. OPC 10000-6 does not cap a SecureChannel at one wrap. The flag is now cleared once an accepted number has traversed the normal range, which keeps rejecting a replayed low number that never left the rollover window. Validated on net10.0 and net48: 636 / 593 Core cases and 135 / 134 Server cases pass, including the new `UaSCBinaryChannelSequenceRolloverTests` and the Uncertain-permission delivery cases. The build reports zero compiler and analyzer diagnostics and the changed files pass included-file info-format checks on both targets. The previous head `085fd9636` was fully green: 279 checks passed and Azure build 18414 succeeded with 137 of 137 jobs, after the platform automatically retried a job whose NuGet tool download had hit a transient TLS error. ## Reported issues #4508 and #4509 at `6ad1ecf16` **#4508 (V2 subscription recreate deadlocks against a faulted transport) was not yet fixed and is now closed by `bac260a06`.** Both drain sites the report identifies were still unbounded. A publish worker that started its request before the quiesce parks inside the service call on the channel ready gate while holding an active publish request; the recreate that would make the channel ready is the very operation awaiting the drain, so the wait never ends. The drain now cancels in-flight attempts through a per-attempt token linked to the worker token. An aborted worker rolls its acknowledgements back, releases the request and returns to the paused park instead of terminating, so it resumes when the caller leaves the quiesce. Publish ingress stays quiesced for the caller's operation, so `DropPendingForSubscription` keeps the guarantee the report asks to preserve and no blind timeout is added. The new regression parks a worker on a cancellable gate that never completes on its own: it reproduces the hang for the full 30-second budget before the change and passes after it. **#4509 (identity provider fails the initial connect) was already fixed for its primary symptom.** `HandleConnectAsync` materializes the provider's identity through `CreateIdentitySelectionContext` before the first `CreateAsync`, so Anonymous is no longer substituted against a UserName-only endpoint. `6ad1ecf16` adds the two remaining details from the report: the enabled-policy set now includes the negotiated channel policy, so a client cannot reject the policy it already accepted as `NotEnabledByClient`; and when discovery has not populated the endpoint's token policies the connect keeps the configured identity so the attempt fails at the transport layer for the reconnect policy to retry, instead of failing at the identity layer. Validated on net10.0 and net48: 477 / 473 client session and subscription cases pass, the build is free of compiler and analyzer diagnostics, and the changed files pass included-file info-format checks on both targets. The full `Opc.Ua.Sessions.Tests` suite also passes locally on net10.0 with 782 passed and 379 pre-existing capability skips. GitHub checks are green on this head: Build and Test .NET 10.0, Docker Sample Images CI, and NuGet Publish all succeeded. Azure build 18417 completed 134 of its 135 jobs successfully; its net10.0 Sessions job was cancelled by the hosted-agent provider ("Received request to deprovision"), the same infrastructure flake that hit build 18409 earlier on a head predating these changes. That job is not claimed as passing, and the local full-suite run above covers the same project. The previous head `da0d59cfb` completed Azure build 18415 with every job succeeded. ## Review follow-up at `a08f0c12b` Eleven new review threads arrived, nearly all documentation. Before acting on each "this is new in 2.0" remark I checked the API against `master378` rather than assuming, because a genuine 1.5 API still needs its migration entry. `ManagedSession`'s server-redundancy refresh had two hard-coded two-second bounds. The new `ServerRedundancyOptions` exposes `RefreshTimeout` for the metadata read and `PeerDiscoveryTimeout` for each peer lookup, defaults both to the previous two seconds, validates that a bound is positive or `Timeout.InfiniteTimeSpan`, and flows through `ManagedSessionOptions`, `ManagedSession.CreateAsync`, the fluent builder, and `DefaultServerRedundancyHandler`. `AsyncCustomNodeManager` and `KeyCredentialBridgeAuthenticator` are both absent from `master378`, so their obsolete members had no compatibility constraint and are removed rather than carried: `AsyncCustomNodeManager.ReadInitialValue`, the audience-less `CreateTokenData` that only threw, and the audience-less `CreateProof` that produced a proof no server accepts. `CustomNodeManager2.ReadInitialValue` and the `MonitoredNode2` migration section are genuine 1.5 APIs and are untouched. The documentation now states master's behaviour instead of the delta from 1.5.378, with each moved item already covered under `docs/migrate/2.0.x/`. `IHistorianAnnotationProvider` is described by its annotation-time key rather than as "legacy" — it is new in 2.0, so the label implied a compatibility constraint that does not exist. The KeyCredential bridge payload's `"version"` field is explained as a vendor-extension format version with no OPC UA counterpart, and its version-1 upgrade narrative is gone. `CertificateManager.md`'s migration walkthrough is replaced by the resolver API it targets. `NodeManagers.md`, `Sessions.md`, `AsyncServerSupport.md`, `Certificates.md`, and `Profiles.md` no longer announce features relative to 1.5. `Benchmarks.md`, `WhatsNewIn2.0.md`, `DeveloperGuide.md`, and `docs/migrate/` keep their 1.5.378 references, which are their subject. CI then exposed a defect this batch introduced: five fixtures resolve `ManagedSession`'s private constructor by its exact positional parameter types, so the added parameter compiled cleanly but made `GetConstructor` return `null` at run time. `7df1d5c40` updates every site; `Redundancy.Client` passes 126 of 126 and the affected client fixtures 112 of 112. The BouncyCastle question is answered and tracked in #4510: `Opc.Ua.Core` already project-references `Opc.Ua.Security.Certificates`, which already carries the conditional BouncyCastle reference, so the raw-ECDH polyfill belongs there rather than as a new Core dependency. The `IUserDatabase` metadata question is addressed to another maintainer and is deliberately left open. ## Reported issue #4509 follow-up at `5d4d13d63` The reporter verified `6ad1ecf16` and found one case still failing, correctly. The empty-token-policies branch kept the configured identity so a server that is down surfaces a transport failure rather than an identity error. A provider-only configuration has no configured identity, so it kept `null`: `OpenCoreAsync` turned that into Anonymous and `OpenValidateIdentity` rejected it with `BadIdentityTokenInvalid`, silently ignoring the configured provider. The branch now requires a configured identity to defer to. Without one the provider is consulted, so its own `BadIdentityTokenRejected` names the offered and rejected policy set instead. Two regression tests cover both configurations; the first fails against the previous behaviour by reaching the session factory with a `null` identity, which is the exact step that produced the reported error. That branch had shipped without a test, which is why the case was missed. ## Review and CI status at `5d4d13d63` **All actionable review threads are resolved: 132 of 133.** The single open thread is a question the reviewer addressed to another maintainer about moving user metadata into `IUserDatabase`; it is left open for them rather than resolved silently. Every resolved thread carries a reply naming its fix commit. **Latest-head CI is green on `5d4d13d63`: 279 checks passed, zero failed, zero pending**, with one conditional signed-package publishing job skipped. Local validation for this batch: the full `Opc.Ua.Client.Tests` suite passes 2,430 of 2,430 on net10.0, `Opc.Ua.Redundancy.Client.Tests` 126 of 126, and `Opc.Ua.Server.Tests` 590 of 590 for the affected selections. Both net10.0 and net48 build clean for `Opc.Ua.Client`, `Opc.Ua.Server`, and the changed test projects, and all changed files pass included-file info-severity formatting on both targets. The PR remains open; no merge was performed. As stated at the top, this does not claim that every item in the 244-finding audit backlog is complete. ## Latest CI follow-up at `b6f2dfa43` Azure build 18442 exposed a net48 compile regression in the Kafka adapter: `RuntimeHelpers.GetUninitializedObject` is unavailable on .NET Framework. Commit `2cf1e0eed` now uses normal `ProducerMessage` construction on `NETFRAMEWORK` while retaining reflection-based construction for modern targets that can load a different Dekaf runtime asset. The exact net48 CI selection passes 92 of 92 tests, and the netstandard2.1 adapter guard selection passes 11 of 11. After merging the latest `master`, GitHub Actions run 35643592139 is green. The initial Ubuntu Core.Schema job built successfully and passed 160 of 160 tests, then hit an external 403 while finalizing its test-result artifact; its failed-job rerun completed successfully. Azure build 18451 was canceled when its remote provider deprovisioned the net10.0 Sessions runner (`Received request to deprovision: The request was cancelled by the remote provider`). Azure attached no test task log or source error, and the canceled parent left that child check stale as in-progress. No source workaround, test skip, or check relaxation was made for either infrastructure failure. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68f26810-22b8-476f-abcd-0a8c82d6e208	2026-09-22 09:10:58 +02:00
Marc Schier	5bc8a9d705	Reduce ISession: type the continuation points, delete the obsolete validate overload (#4199) ... ## Stacked on #4197 Base is `marcschier/server-bind-phase`. The full stack is **#4188 → #4193 → #4197 → this**. GitHub retargets each base automatically as the stack merges. **Review the earlier three first.** **#4217 has been merged into this branch** (squash `53dd4b618`), so the last remaining item from the issue — `ISession.SessionDiagnostics` — is now part of this PR. Closes #4187. Refs #4203 — **does not close it.** #4217 swapped `OnUpdateDiagnostics` onto `m_diagnosticsLock`, but it still calls `Variant.FromStructure(ServerDiagnostics)` with the default `copy: false`, which wraps the *live* structure in the `ExtensionObject`. The caller reads its fields after the lock is released, so the lock currently excludes nothing. #4222 adds `copy: true` and is what actually closes #4203. ## The finding that changed the approach `ISession` has **exactly one production adapter**: `src/Opc.Ua.Server/Session/Session.cs`. The other `ISession` implementations in the tree are the unrelated **client** interface (`src/Opc.Ua.Client/Session/Session.cs`, `RedundantClientSession`) and a migration-analyzer test stub. One adapter means the seam is hypothetical; two means it is real. The issue proposed splitting `ISession` into five concern-interfaces — that would turn one hypothetical seam into five. The interface earns its keep for testability and because node managers receive it through `OperationContext`, but that argues for making it **smaller**, not for multiplying it. **So: reduce, don't split.** Reasoning posted to the issue as [comment 5226712936](https://github.com/OPCFoundation/UA-.NETStandard/issues/4187#issuecomment-5226712936) so the five-way framing does not get re-proposed later. ## Measurement 38 members, 368 references. External production consumers are **4 files using 5 members**: | Consumer | Members used | |---|---| | `Opc.Ua.Redundancy.Server/Sessions/DistributedSessionManager.cs` | `Id`, `InitializeAsync`, `Dispose` | | `Opc.Ua.Di.Server/Locking/DefaultLockService.cs` | `Id` | | `samples/ConsoleReferenceServer/UAServer.cs` | `Id`, `Identity`, `ReadDiagnostics` | | `samples/Quickstarts.Servers/.../DataChangeMonitoredItem.cs` | type reference only | Every production caller of the five continuation-point members is inside `Opc.Ua.Server` (`MasterNodeManager`, `NodeManagerLifecycle`, `HistorianDispatcher`). **`ISession` is 38 → 32 members.** ## Commits ### `882b2937` Type the history continuation point `ISession` leaked `object` twice, against the repository rule: ```csharp void SaveHistoryContinuationPoint(Guid id, object continuationPoint); object? RestoreHistoryContinuationPoint(ByteString continuationPoint); ``` The erasure was **not** hiding a genuinely opaque plugin token. `HistorianDispatcher` is the only production caller, it always passes a `HistorianContinuationState`, and it casts straight back to that type on restore. The `object` was a round trip through the type system that bought nothing. `HistorianContinuationState` is `internal`, so it cannot appear on a public interface. `IHistoryContinuationPoint` names the concept instead: it carries an `Id` and extends `IDisposable`. Two latent bugs fell out of typing it: - The session disposed only those points that **happened** to implement `IDisposable` and **silently leaked the rest**. Every point is now disposable by construction, and three `as IDisposable` probes are gone. - Carrying the `Id` on the point removes the separate `Guid` parameter, which could disagree with the payload it was filed under. The mock stores in the historian tests had the same latent inconsistency. ### `0d020b77` Delete the obsolete synchronous `ValidateBeforeActivate` **Zero production callers** — the nine references were all tests written for it. `[Obsolete]` since 1.5.378, which the repository rules allow removing in master. It is also the divergent-signature problem the runtime analysis flagged: the async form returns the validated handler and policy as a tuple, while the sync one took the same two values as trailing `out` parameters, so a caller had to learn two shapes for one operation. The overload could not verify a user token requiring decryption, so on a secure endpoint it failed closed and told the caller to use the async path anyway. The one test asserting that fail-closed behaviour goes with the method it described; the path it pointed at is already covered by `ActivationValidationDecodesBinaryEncodedUserIdentityToken`. The other eight tests are **converted, not deleted** — the channel, signature, security-policy and token-policy validation they cover is still asserted, now against the path the server actually uses. ### `821e8302` Route the continuation points behind one member Five `ISession` members were one-line pass-throughs to `SessionContinuationPoints`, a 419-line module that already owns the behaviour and already has its own `IContinuationPointStore` seam. The interface restated the module's surface without adding anything. ```csharp // was session.SaveContinuationPoint(cp); session.RestoreHistoryContinuationPoint(bytes); // now session.ContinuationPoints.SaveBrowse(cp); session.ContinuationPoints.RestoreHistory(bytes); ``` The holder is `internal sealed`, so `ISessionContinuationPoints` is what makes it addressable. **This is composition, not the service locator `IServerContext` refused to be.** A subsystem with an independent lifetime should be injected; continuation points belong to the session and cannot exist without it. `InvalidateContinuationPoints` called `RemoveBrowseForManager` and `RemoveHistoryForManager` in sequence, so the pair becomes `RemoveForManager` — the caller's intent is that a node manager is going away, not that two collections need visiting. ## Tests Three harnesses had hand-rolled a `Dictionary<Guid, object>` to stand in for the holder. They now use the **real** one, so the dispatcher and `BrowseNext` meet the eviction and disposal rules a live session actually applies. **One hazard worth recording for future interface work.** A Moq member that is never stubbed returns `null`. Where the old code called a method straight on `ISession`, that `null` was harmless. Reaching *through* a property means an unstubbed `ContinuationPoints` null-derefs instead. Three tests failed exactly that way; they are wired to a real holder, not restubbed. | Suite | net10.0 | net48 | |---|---|---| | `Opc.Ua.Server.Tests` | 4032 passed, 1 failed | builds clean | | Touched areas (continuation points, historian, master node manager, session) | 898 passed | — | The single failure is `ConfigureApplicationBuildsSharedClientAndServerConfigurationAsync`, a local certificate-store problem verified to reproduce unchanged at HEAD with these changes stashed. Not caused by this PR. `Opc.Ua.Redundancy.Server`, `Opc.Ua.Di.Server`, `Quickstarts.Servers` and `ConsoleReferenceServer` all build clean. ## Docs `docs/MigrationGuide.md` gains a "Removed members on ISession" section covering the `ValidateBeforeActivate` removal with the async replacement, and the `IHistoryContinuationPoint` change with before/after. ## The SessionDiagnostics removal (folded in from #4217) `ISession.SessionDiagnostics` handed out the whole mutable `SessionDiagnosticsDataType` — the structure the session's diagnostics lock protects — so a caller could read a field while the owner was writing it. Every server-side reader wanted exactly one value out of it, so the interface now exposes those two values (`SessionName`, `ClientApplicationUri`) directly and the structure stays behind the lock. That work landed here as #4217 rather than as a separate PR, and it also fixes #4203: `OnUpdateDiagnostics` now takes `m_diagnosticsLock` instead of locking the diagnostics payload it was handed. ## Reviewer questions - `ISessionContinuationPoints` exposes `MaxBrowse` as read-only while the concrete holder keeps a setter (the test framework adjusts it to exercise limits). Should the setter be on the interface, or is test-only mutation better left to the concrete type? - `RemoveForManager` collapses two removal methods. The concrete `RemoveBrowseForManager`/`RemoveHistoryForManager` remain public on the holder — worth making them private now that nothing outside calls them individually? --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e9c13dd0-a6e3-4ad2-8ad6-1c3fe6323041	2026-08-11 06:57:40 +02:00
Marc Schier	e73e71184d	[WotCon] Add OPC UA WoT Connectivity 1.1 with the Types, source generation, Server and xRegistry work it builds on (#4128) ... ## Summary Adds OPC UA WoT Connectivity 1.1 to the stack, together with the Types, source generation, Server and xRegistry work it is built on. This branch is the merge point for the fourteen-PR stack that replaced integration PR #4093; every constituent PR was reviewed and approved separately and is listed below. 414 files changed, +108,936 / -3,199. Roughly 46k added lines of product code, 57k of tests, plus samples, tools and documentation. ## Types - Complete `ExtensionObject` raw-body decoding for binary, XML and JSON bodies by resolving the concrete type through the message context's encodeable factory, and simplify the `Variant` structure helpers by delegating to it (#4128). - Add `IAtomicFileReplace` as an optional `IFileSystem` capability so existing external implementations keep working, with atomic publish for `LocalFileSystem` via `File.Replace`/`File.Move` and for `VirtualFileSystem` by re-keying the in-memory entry (#4128). - Add lossless conversion between WoT documents and NodeSet2 (`Opc.Ua.Wot.WotNodeSetConverter`). A byte-exact `uav:nodeSet` envelope is preserved when requested, the structured `uav:nodes` projection is used when the readable vocabulary is incomplete, and NodeSet2 is otherwise synthesized from readable WoT terms. Unmapped JSON members survive a round trip as pointer-addressed residue (#4131). ## Source generation - Improve the NodeSet to ModelDesign conversion and the generated node state (#4132). - Generate OPC UA models directly from WoT Thing Description files (#4134). ## Server - Add live NodeManager shadow and immediate reload. A reload materializes a new generation beside the active one and switches atomically; the superseded generation is retired gracefully so existing MonitoredItems keep being served until they drain, or immediately when the caller asks for it. Includes the request admission and drain machinery that lets an orderly shutdown wait for admitted requests instead of tearing down underneath them (#4147). - Make subscription transfer between sessions transactional, so a failed transfer leaves neither session holding a partially moved subscription (#4150). ## xRegistry - Allow clients to use explicit registry roots rather than assuming a well-known location (#4130). ## WoT Connectivity - Add the WoT Connectivity 1.1 information model: a registry-first revision layered on the abstract xRegistry base model, incorporating the complete published OPC 10100-1 v1.02 surface as deprecated nodes in the same namespace so existing 1.02 clients keep working (#4139). - Add the protocol binding abstractions and planners that compile WoT forms into executable plans (#4140). - Add the HTTP, Modbus, OPC UA and MQTT binding executors (#4144). - Add the registry and its stores, including document versioning, validation, dependency resolution and refresh (#4146). - Add the registry client (#4142). - Materialize WoT Thing Descriptions into the server address space, deriving types from Thing Models and instances from Thing Descriptions (#4154). - Restore the WoT asset registry bridge so the deprecated 1.02 asset surface is backed by the registry (#4171). - Add the WoT Connectivity samples and complete the documentation (#4156), and restore the remaining coverage and docs (#4172). ## New projects `src/Opc.Ua.WotCon.Bindings`, `src/Opc.Ua.WotCon.Bindings.Mqtt`, `tests/Opc.Ua.WotCon.Bindings.Tests`, `tests/Opc.Ua.WotCon.Samples.Tests`, and the `samples/WotCon` AggregationServer, AggregationClient and FlatTagServer samples. ## Constituent pull requests Merged into this branch top-down, each independently approved: | PR | Title | | --- | --- | | #4128 | [Types] Complete ExtensionObject decoding and add an atomic file replace capability | | #4131 | [Types] Add lossless conversion between WoT documents and NodeSet2 | | #4132 | [SourceGeneration] Improve NodeSet to ModelDesign conversion and generated node state | | #4134 | [SourceGeneration] Generate OPC UA models from WoT Thing Description files | | #4147 | [Server] Add live NodeManager shadow and immediate reload | | #4150 | [Server] Make subscription transfer between sessions transactional | | #4130 | [XRegistry] Allow clients to use explicit registry roots | | #4139 | [WotCon] Add the WoT Connectivity 1.1 information model | | #4140 | [WotCon] Add the WoT protocol binding abstractions and planners | | #4144 | [WotCon] Add the HTTP, Modbus, OPC UA and MQTT binding executors | | #4146 | [WotCon] Add the WoT Connectivity registry and its stores | | #4142 | [WotCon] Add the WoT Connectivity registry client | | #4154 | [WotCon] Materialize WoT Thing Descriptions into the server address space | | #4156 | [WotCon] Add the WoT Connectivity samples and complete the documentation | | #4171 | Restore WoT asset registry bridge | | #4172 | Restore WoT coverage and docs | Supersedes #4093. ## Follow-up The WoT Connectivity and WoT Binding drafts have moved to 1.1-draft2 since this work was authored. Aligning with that revision - the removed group vocabulary, the new projection/View construct, the remaining model vocabulary terms and the two new portable-identity validation rules - is tracked separately and will follow in its own pull request. ## Validation Built for every target framework with zero warnings and zero errors, and validated at the tip of the stack with `Opc.Ua.WotCon.Tests` at 999 passed / 0 failed and `Opc.Ua.Server.Tests` at 4024 passed / 0 failed. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8	2026-08-05 10:14:15 +02:00
Marc Schier	275c3800b5	[Server] Add live NodeManager lifecycle support (#4094) ... # Description Adds supported live add, reload, and remove lifecycle operations for NodeManager instances on a running server. The change introduces a DI-backed `INodeManagerLifecycle` service with direct `StandardServer` access, generation-aware registration handles, copy-on-write routing, transactional prepare/commit/rollback, runtime NodeSet convenience APIs, append-only namespace updates, model and semantic change notifications, additive-compatible complex type priming, and complete teardown behavior. Lifecycle publication is coordinated with requests, sessions, subscriptions, monitored items, continuation points, and shutdown so retired managers cannot remain reachable or be disposed while still in use. Compatible monitored items transfer to replacement generations without a transient bad status. Removed or incompatible Nodes publish `BadNodeIdUnknown` as required by OPC UA Part 4 section 5.8.4.1, retain their MonitoredItem identity, and automatically recover when a compatible Node with the same NodeId returns. Registration is documented end to end in the new [docs/NodeManagerRegistration.md](docs/NodeManagerRegistration.md), which covers compile-time, startup, and runtime registration together with the guarantees for MonitoredItems, Browse continuation points, namespace indexes, DataTypes, and change notifications. ## Review feedback addressed Interface surface reduced, as requested: - `INodeManagerMonitoredItemTracker` folded into `ISubscriptionMonitoredItemLifecycle`. - `INodeManagerMutationCoordinator` and the asynchronous MonitoredItem recovery entry point folded into `IDynamicNodeManagerHost`. - `ISessionClosingRegistry` folded into `IServerInternal`, then replaced by `ISession.IsClosing`. - `ISubscriptionDeletionRegistry` folded into `ISubscriptionManager`, then removed entirely. - `INodeManagerContinuationPointTracker` folded into `ISession`. - The synchronous recovery entry point kept on its own `ISyncNodeManagerMonitoredItemRecovery`, because `AddPredefinedNode` cannot await; it still routes through the asynchronous `INodeManagerMonitoredItemLifecycle` that `AsyncNodeManagerAdapter` exposes. - `IMonitoredItemLifecycle` renamed to `IDetachableMonitoredItem`. Reach into the Subscription area reduced to the minimum the feature needs: - The lifecycle feature itself leaves `SubscriptionManager` almost untouched: two guards that reject a request from a Session that is closing, on `CreateSubscription` and on `TransferSubscriptions`, because it would create state that is about to be torn down. The deletion claim registry, the session closing gates, the closed-session bookkeeping and the deletion restore paths introduced by earlier revisions are gone. Transferring a subscription away from a session that is closing stays allowed, since that is the recovery path the service exists for; the race that exposed is fixed by serialising source-queue closure with the transfer rather than by rejecting it. - Queueing is disabled again for a MonitoredItem with `QueueSize <= 1`, as before this PR. Without a queue the Client is served the last sampled value, so the missing-Node notification simply becomes that value; losing values is expected for a MonitoredItem without queuing. - What remains of the notification is one `DataValue` plus one bool in `DataChangeQueueHandler`, mirroring the existing overflow pair, which stops it being the value discarded when a configured queue is full. - `MonitoredItem` keeps its original publish path, its original queue creation and restore paths, its original triggering behaviour, and no per-epoch bookkeeping. - `IStoredMonitoredItem` carries `IsDeleted` and `IsDetached`, so a restored item reports the state it was stored with rather than one inferred from the last value's StatusCode. Other feedback: - Documented `HostedNodeManagerLifecycle`, `IDynamicNodeManagerHost`, `NodeManagerRoutingTable`, and the `RequestManager` validation and request scopes. - Renamed `ValidateMonitoredItemAsync` to `CanAttachMonitoredItemAsync` and `RestoreMonitoredItemAsync` to `RecoverMonitoredItemAsync`, so the lifecycle API no longer collides with the durable Subscription `RestoreMonitoredItem` path used at server startup. - Removed the `CustomNodeManager2` dependency from the synchronous MonitoredItem recovery path. - `DataChangeQueueHandler` is back to the two constructors 1.5.378 shipped, each taking an optional `TimeProvider`. - Session closing is a one-way latch, and the Session is removed even when teardown throws. - Documented what a custom NodeManager has to implement to take part in live lifecycle operations, and which NodeManager implements `INodeManagerReloadParticipant` today. ## Fixes found during review - The guard that stops a NodeManager lifecycle operation being started from inside a service handler never fired. It was backed by an `AsyncLocal` written inside `async ValidateRequestAsync`, which is invisible to the caller that awaited it, so a lifecycle call from a NodeManager callback waited for its own request and held the lifecycle semaphore forever. The mark is now applied by the service dispatcher, and the drain is bounded so no path can wedge the lifecycle permanently. - `ValidateRequestAsync` is no longer virtual. An override that rejected a request after calling `base` left it registered forever; subclasses now use `OnRequestValidatedAsync`, whose failures complete the request. - A MonitoredItem deleted and disposed while a reload was handing it to the replacement NodeManager could be attached anyway. The item now reserves itself for the hand-over and the transition takes it back if it was disposed meanwhile. - The Overflow bit is recorded and applied when the value is published, as the surrounding design does, rather than written into the value at enqueue time. - MonitoredItem cleanup for a Subscription being deleted no longer passes null entries to `DeleteMonitoredItemsAsync`. - Browse continuation points of a retired NodeManager are persisted and disposed outside the Session lock. ## Open review discussions Three threads are open, replied to and awaiting reviewer decisions: separating service-call dispatch from NodeManager handling in `MasterNodeManager` (tracked as #4098), the request-scoping shape in `StandardServer`, and how much of the async MonitoredItem service implementation should stay in `Subscription`. Issue #4102 records a specification ambiguity found while implementing the missing-Node notification: Part 4 §5.8.4.1 makes it mandatory, but the protected over-capacity queue entry §5.13.1.5 defines applies to `EventQueueOverflowEventType` only, so the specification does not say how it survives a full queue. ## Validation - Full `Opc.Ua.Server.Tests` on net10.0: 3,768 passed, 5 skipped. - Lifecycle, MonitoredItem, Subscription and continuation point suites: 185 passed on net10.0 and 75 on net48. - Server builds complete with zero warnings or errors. - Two failures are pre-existing and unrelated: `DurableDataValueQueueVerifyReferenceBatchingAsync` (flaky `BatchPersistor` file contention, passes in isolation on net10.0 and net48) and `ConfigureApplicationBuildsSharedClientAndServerConfigurationAsync` (fails on a clean tree with these changes stashed). ## Related Issues - Fixes #3993 - Follow-up tracked in #4098	2026-07-28 16:06:28 +02:00
Alexander Köpke	a568a7e54c	Set ContinuationPoint (when browse is finished) to null instead of empty array, align with existing 1.5.xxx behavior (#4057) ... This pull request standardizes the handling of the `ContinuationPoint` property in `BrowseResult` objects, ensuring it is always explicitly set to `null` (or its equivalent) when no continuation point is present. This change improves clarity and consistency in both the implementation and the tests, replacing checks for an "empty" continuation point with checks for a "null" continuation point. **Codebase consistency and correctness:** * All `BrowseResult` objects now explicitly set `ContinuationPoint = default` when no continuation point is to be returned, ensuring consistent initialization. (`src/Opc.Ua.Server/NodeManager/MasterNodeManager.cs`) [[1]](diffhunk://#diff-851e5925c677925c42bd70bce11ea1a75a5f5cbdb810ac851351385e16f54a71L1900-R1904) [[2]](diffhunk://#diff-851e5925c677925c42bd70bce11ea1a75a5f5cbdb810ac851351385e16f54a71L2068-R2076) [[3]](diffhunk://#diff-851e5925c677925c42bd70bce11ea1a75a5f5cbdb810ac851351385e16f54a71L2078-R2090) **Test updates for new semantics:** * All test assertions now use `ContinuationPoint.IsNull` instead of `IsEmpty` to check for the absence of a continuation point, reflecting the new contract. (`tests/Opc.Ua.InformationModel.Tests/BrowseContinuationPointTests.cs`, `tests/Opc.Ua.Server.Tests/NodeManager/MasterNodeManagerDeterministicTests.cs`) [[1]](diffhunk://#diff-904275d6b4bee35e6884f38e0db5cbc955216eaf2aa7f468ba285133470e1596L121-R122) [[2]](diffhunk://#diff-904275d6b4bee35e6884f38e0db5cbc955216eaf2aa7f468ba285133470e1596L394-R394) [[3]](diffhunk://#diff-904275d6b4bee35e6884f38e0db5cbc955216eaf2aa7f468ba285133470e1596L435-R435) [[4]](diffhunk://#diff-904275d6b4bee35e6884f38e0db5cbc955216eaf2aa7f468ba285133470e1596L634-R634) [[5]](diffhunk://#diff-bda1da5dc01533ffe61d1c65b6e968b6462e11a63758ac95ded000ffc885345eL420-R420) * Additional test cases have been added to verify that completed browse operations and error scenarios always return a null continuation point. (`tests/Opc.Ua.Server.Tests/NodeManager/MasterNodeManagerDeterministicTests.cs`) [[1]](diffhunk://#diff-3d9efc81935ec60d7513334833e9eab033925194f5a0d11105668204cc60f54aR203-R230) [[2]](diffhunk://#diff-3d9efc81935ec60d7513334833e9eab033925194f5a0d11105668204cc60f54aR327) [[3]](diffhunk://#diff-3d9efc81935ec60d7513334833e9eab033925194f5a0d11105668204cc60f54aR349)# Description _Describe the changes here to communicate to the maintainers why they should accept this pull request. By default - this will become the Commit message after merging and thus define history._ ## Related Issues _Reference all GitHub issues this PR addresses. If there is no issue yet, open one and link it here._ _If this is a relatively large or complex change, a design must have been discussed in the related tracking issue and signed off (which becomes the Architectural Decision Record (ADR))._ - Fixes #github-issue-number, ... ## Checklist _Put an `x` in the boxes that apply. You can complete these step by step after opening the PR._ - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [ ] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. --------- Co-authored-by: Marc Schier <marcschier@users.noreply.github.com> Co-authored-by: Marc Schier <marcschier@hotmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>	2026-07-21 20:54:19 +02:00
MrAlaskan	399747f6a0	fix: don't return continuation points on BadNoContinuationPoints (#4022) ... ### Summary This PR fixes `Browse` and `BrowseNext` so they do not return a usable continuation point when the server reports `BadNoContinuationPoints`, and adds regression tests that cover both code paths. ### Problem According to OPC UA Part 4, if the server cannot allocate another continuation point for a `Browse` or `BrowseNext` operation, the result should be `Bad_NoContinuationPoints`. In that case, the server must not also return a continuation point that the client can continue to use. Previously, `MasterNodeManager.FetchReferencesAsync()` could return `BadNoContinuationPoints` while still passing a live continuation point object back to its callers. `BrowseAsync()` then copied that continuation point into the result, producing a contradictory response with `StatusCode = BadNoContinuationPoints` and a non-empty `ContinuationPoint`. `BrowseNextAsync()` went further and overwrote the error with `Good` whenever a continuation point object was still present. That behavior hides continuation-point quota exhaustion and breaks the expected paging semantics. ### Changes - Dispose and clear the continuation point when `FetchReferencesAsync()` hits the no-continuation-points path. - Only copy a continuation point into `Browse` and `BrowseNext` results when the operation completed successfully. - Add deterministic regression tests that verify both `Browse` and `BrowseNext` return `BadNoContinuationPoints` without a continuation point when the continuation-point quota is exhausted.	2026-07-20 08:02:45 +02:00
Marc Schier	7cec2d614d	Move repository files to the consolidated layout	2026-07-16 06:25:54 +02:00