13 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	61edaa1ed0	Remove netstandard2.1 and net472 target frameworks (#4624) ... # Description Drops support for **netstandard2.1** and **.NET Framework 4.7.2**. Libraries now target `net48`, `net8.0`, `net9.0` and `net10.0`. `Opc.Ua.Types` and `Opc.Ua.SourceGeneration.Core` still also target `netstandard2.0`, but only because the Roslyn source generators load them. There is no general netstandard2.0 support. **Preprocessor directives** (112 files) - Every `#if`/`#elif` mentioning `NETSTANDARD2_1*`, `NET472*`, `NET_STANDARD_TESTS`, `SKIP_ECC_CERTIFICATE_REQUEST_SIGNING` or `NETSTANDARD*` was evaluated against the remaining TFM set (net48, net8/9/10, plus netstandard2.0 inside `Opc.Ua.Types`/`tools`) and simplified. Dead branches were removed. - Example: `NETSTANDARD2_1 || NET8_0_OR_GREATER` → `NET8_0_OR_GREATER`. - Example: `NET8_0_OR_GREATER || NET472_OR_GREATER` → removed (always true). - Netstandard-only code was removed: `UnixFileModeShim`, the CRDT reflection path in `ReplicatedGossipOptions.CreateReaderOptions`, and the netstandard2.1-only test variants. **Custom test logic** - `targets.props` drops the `net472`, `netstandard2.0` and `netstandard2.1` `CustomTestTarget` profiles, `NetStandardTests` and `SKIP_ECC_CERTIFICATE_REQUEST_SIGNING`. Supported values are now `net48`, `net8.0`, `net9.0`, `net10.0`. - The `NET_STANDARD_TESTS` define is removed from 29 test projects. - The "build netstandard shells as net10.0" overrides are removed from 18 projects. - `RestrictForLegacyTfm` now only applies to `net48`. - `tests/customtest.bat` is updated to the new values. **Build / packaging / CI** - MSBuild TFM conditions are simplified: Kestrel package choice, MQTTnet v4 (net48 only), BouncyCastle, Di/OpenUsd/Vision/Stress test excludes, and `System.IO.FileSystem.AccessControl` (dropped from Core). - The MigrationAnalyzer nuspec drops its net472/netstandard2.1 lib groups. The Azure signing lists are updated. - `get-ci-matrix.ps1` removes the net472/netstandard test profiles (including the PR-gating `linux-netstandard2.1`) and the matching build legs. **Breaking API change (2.0 preview):** removed `HttpsTransportListener.IsWssTransportSupported` and `ReplicatedGossipOptions.IsGossipStateDecodingSupported`. They only returned `false` on netstandard2.1 and are now always `true`. The test guards that used them were removed as well. **Docs** - NuGet READMEs, `README.md`, `DeveloperGuide.md`, `PubSub.md`, the certificate/ECC/identity docs and the 2.0 migration guide (plus its `.agents` skill mirror) are updated. - The migration guide notes that 4.7.2 consumers must move to 4.8 and netstandard2.1 consumers to net8.0+. ### Reviewer notes - The `System.IO.FileSystem.AccessControl` `PackageVersion` pin is left in `Directory.Packages.props`. It has no direct reference any more, but transitive pinning is on, so it was kept to avoid shifting resolved versions. - The `#if` rewrite was done mechanically by a truth-table evaluator. The diff in the `.cs` files is limited to directive lines, removed dead branches, and stale-comment updates. ### Verified locally - `dotnet build UA.slnx -p:CustomTestTarget=net48`: 0 errors. - `dotnet build UA.slnx -p:CustomTestTarget=net10.0`: 0 errors. - Default multi-target builds (no `CustomTestTarget`) of Opc.Ua.Types, Bindings.Https, PubSub.Mqtt and Redundancy.Server succeeded. - net10.0 tests: - Opc.Ua.Redundancy.Server.Tests: 840/840 passed. - Opc.Ua.Types.Tests: 11363 passed, 1 skipped. - ModelDependencyScanner tests: 20/20 passed. - The full test suite was **not** run locally. ## Related Issues - None ## 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. - [ ] 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. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>	2026-10-02 17:09:49 +02:00
romanett	20c2d77d57	Fix Core/Types audit findings and conform them to the OPC UA spec (stacked on #4569) (#4573) ... > **Stacked on #4569.** The base branch is `romanett/practical-murdock-fd9c56`, so this diff shows only the commits of this PR. Retarget to `master` after #4569 merges. ## Summary Bug audit of `Opc.Ua.Core`, `Opc.Ua.Core.Types`, `Opc.Ua.Types` and `Opc.Ua.Core.Schema`, with the findings fixed and the fixes grounded against the OPC UA specification (reference.opcfoundation.org). 133 commits in four groups: 1. **Audit fixes** (`Fix <ID>: ...`): 116 findings from a read-only audit, each with a regression test. IDs follow the audit scopes: T1-T6 `Opc.Ua.Types`, C1-C4 `Opc.Ua.Core`, S1 `Opc.Ua.Core.Types` + `Opc.Ua.Core.Schema`. 2. **Review fixes** (`Fix review <ID>: ...`): issues a code review of group 1 found, plus lower-severity leftovers. 3. **Spec fixes** (`Spec: #<n> ...`): the 18 places where an earlier fix only partly matched the spec text. 4. Test and analyzer follow-ups. Where `master` (#4516, #4522, #4540, #4564) or #4569 already fixed the same issue, their version was kept and this PR only adds what they didn't cover. ## Behaviour and user-facing changes ### Wire format and interoperability (breaking) - **PubSub AES-CTR (Aes128-CTR / Aes256-CTR)** now follows Part 14 Tables 156/157: - The counter block is `KeyNonce[4] | MessageNonce[8] | BlockCounter[4]`, and the block counter starts at 1. - The SecurityHeader nonce is 8 bytes: `Random[4] | UInt32 SequenceNumber`, with the sequence number starting at 1. - SKS key data is split with a 4-byte KeyNonce, giving 52 / 68 bytes. - **Not interoperable with earlier releases of this stack**, but matches the spec. There is a test against the independent RFC 3686 vector. - **User token signature** (Part 4 6.1.8): - The signed data is now built from the user token policy's SecurityPolicy on both client and server. - It also supports the SecurityMode None layout `ServerNonce | HASH(ServerCertificate) | ClientNonce`. - New overload `SecurityPolicyInfo.GetUserTokenSignatureData(..., MessageSecurityMode)`. - **Secure channel:** - The ECC AEAD `LastSequenceNumber` is correct across the 32-bit sequence wrap (Part 6 6.8.1). - The incoming chunk counts against MaxChunkCount/MaxMessageSize. - The sender no longer lets a message through that is 24 bytes over MaxMessageSize. - The OPN response signature is verified without overrunning the buffer (RSA_DH with differing key sizes). ### Sessions (server) - **CreateSession nonce:** - The client nonce is kept on None channels, because the None-mode user token signature covers it. - A nonce over 128 bytes returns `Bad_NonceInvalid` on every channel (Part 4 5.7.2.3). - The duplicate-nonce check runs only on secured channels. - A non-empty nonce under 32 bytes is still accepted on None, for compatibility. - **ActivateSession:** an embedded `EccEncryptedSecret` signing certificate is validated with the server's peer certificate validator (Part 6 6.8.3). Byte equality with the client certificate is only the fallback when no validator is available. New opt-in interface `ICertificateValidatorProvider`. - **FindServers / GetEndpoints:** `GatewayServerUri` and `DiscoveryProfileUri` are no longer swapped. ### Write, Read and Call (NodeState / node managers) - **UserWriteMask:** - Enforced for non-value attributes and for VariableType Value writes (`ValueForVariableType`), returning `Bad_UserAccessDenied`. - An unconfigured UserWriteMask (0, no handler) is not enforced, and reads back as the WriteMask so Read and Write agree (Part 3 8.60). - A throwing or Bad `OnReadUserWriteMask` handler fails only that item. - **Index-range writes:** - Slices are merged into the cached value under the attribute lock, also for `OnWriteValueAsync` handlers. - A data-change notification is still produced when the cache cannot absorb the slice. - The slice must match the unclipped range, otherwise `Bad_IndexRangeDataMismatch` (new status code in `Opc.Ua.Types`). - A range past the target returns `Bad_IndexRangeNoData`. - A handler without index-range support returns `Bad_WriteNotSupported` (Part 4 7.27, 5.11.4.4). - **Index-range reads on matrices** return partial results clipped to the source, instead of allocating the requested size (a Read with `"0:40000,0:40000"` no longer allocates about 1.6e9 elements). - **DataTypeDefinition writes:** - A value that isn't a decoded DataTypeDefinition, or whose kind doesn't match the DataType category, returns `Bad_TypeMismatch`. - Null is rejected for concrete Structure/Enumeration/OptionSet types. - WriteMask is checked first (Part 3 5.8.3). - **AnalogItem:** a null write skips the InstrumentRange check again. - **Method call:** the async fallback passes the ObjectId to the method handlers. - **Redundancy:** `DistributedValueParticipation` merges index-range writes and applies the index range on reads. - **Change masks:** `ClearChangeMasks` takes the bits atomically, so concurrent changes are no longer lost. - **`AccessLevelEx`:** preserved by Clone/Export. ### Alarms & Conditions (Part 9) - **Acknowledge / Confirm / AddComment:** - Accept any EventId the condition or its branches reported while that state is still pending (5.7.3). - An unknown id returns `Bad_EventIdUnknown`. - An already handled state returns `Bad_ConditionBranchAlreadyAcked` / `...AlreadyConfirmed`. - AddComment on a branch no longer changes the trunk, and the branch table is re-keyed when a branch's EventId changes. - **Shelving:** - OneShotShelve without MaxTimeShelved no longer overflows the timer. - A timed shelve is measured on the monotonic clock, so wall-clock steps don't delay the unshelve. - The state is named `OneShotShelved`. - **Audit events:** - They report failure for rejected method calls. - `SelectedResponse` is a UInt32. - A rejected negative Respond leaves SelectedResponse unset. - **`ContentFilterElement.Validate`:** an unknown FilterOperator is reported per element as `Bad_FilterOperatorInvalid`, instead of failing the whole service call. ### ContentFilter evaluation (Part 4 7.7.3, on top of #4564) - **Bitwise operators:** - The result takes the size of the larger operand. - Boolean, numeric String and one-element array operands are converted to integers implicitly. - Types without an implicit integer conversion give NULL. - **Strings:** ordering is ordinal, no longer culture dependent. - **Cast:** restricted to the Table 121 conversions, and QualifiedName to LocalizedText is added. The general `Variant.ConvertTo` keeps its wider set. - **Null and empty operands:** - `Like` with a NULL operand is NULL. - Empty arrays of the same DataType compare equal. - Null-string operands are NULL everywhere. ### Encoders and built-in types - **XML:** - Strings keep leading, trailing and whitespace-only content (Part 6 5.3.1.5). NodeSet import opts back into treating whitespace-only strings as empty with the new `XmlDecoder.TreatWhitespaceOnlyStringsAsEmpty`. - Layout whitespace around `<Identifier>` values is ignored, and trailing control characters are stripped from `s=` ids. - `XmlElement` ignores DTDs without expanding entities. - **JSON:** - A DateTime without an offset is read as UTC. - An unmappable `nsu=` NodeId and an unknown `svu=` ExpandedNodeId decode to a namespace-0 String NodeId (5.4.2.10 / 5.4.2.11). - `svr=` is rejected in a NodeId field. - Bad matrix dimensions return `BadDecodingError`. - The non-strict body stack stays balanced after an error. - The nesting limits match between encoder and decoder. - **Binary:** - `MaxByteStringLength` no longer applies to strings. - ExtensionObject bodies count toward the nesting limit on seekable streams (and in the XML encoder). - **Parsing:** - The NodeId parser requires `i=`/`s=`/`g=`/`b=`. - `ExpandedNodeId.Parse` rejects an empty `nsu=`/`svu=`. - NumericRange rejects a trailing `,` with `Bad_IndexRangeInvalid`. - RelativePath escapes `#`. - **Status codes:** `StatusCode.WithSubCode`/`WithCodeBits` keep the severity bits and look up the symbolic id for the new code. - **Variant / VariantHelper:** - `TryCastTo` returns false on a type mismatch. So `DataValue.GetValue<T>(default)` returns the default, and `GetValueOrDefault` / `CastTo` throw `BadTypeMismatch` as documented. - `System.DateTime` casts are supported. - Concrete encodeable arrays (e.g. `Argument[]`) no longer throw. - `ConvertTo` keeps one-element and empty arrays as arrays and checks the conversion is legal for empty arrays. - `CastFrom(object[])`, `CastFrom(Enum[])` and the obsolete `Variant(object)` constructors keep the array shape (previously a one-element array collapsed to a scalar and an empty one to Null). - `TypeInfo.Unknown` / `Variant.Null.TypeInfo` report `ValueRanks.Any`. - **Equality and hashing contracts fixed:** - Types: Matrix/MatrixOf, typed-null Variants, NodeId (the namespace index is now hashed), ExpandedNodeId, LocalizedText, DiagnosticInfo, StructureDefinition/EnumDefinition, CertificateIdentifier, UserIdentity and DateTimeUtc. - `MatrixOf.Dimensions` returns a copy. - Decimal equality and hashing now run in bounded time. - **Localized text:** a LocalizedText with locale `"mul"` and no usable translations, or null client locale ids, no longer throw. ### NodeSet, address space and utilities - **NodeSet export/import:** - Server index 0 is the local server on import and export, so local ids are no longer written as `svr=65535`. - `StructureDefinition.BaseDataType` is filled from the supertype on import. - **`NodeTable.Import`:** replaces duplicates, keeping the references neighbouring nodes hold. `TypeTable.Add` detaches a re-parented type from its old supertype, and `Node.Clone` gets its own ReferenceTable. - **`StringTable` / `NamespaceTable`:** capped at UInt16 indexes (`BadEncodingLimitsExceeded`) for Append and Update, instead of silently wrapping. - **Small fixes:** - `UnsecureRandom.Shared` is seeded randomly. - The peer's RetryAfterMs is clamped to one day. - The `CancelAsync` and `string.GetHashCode(StringComparison)` polyfills match BCL behaviour. - `CoreUtils.IsEqual` compares strings ordinally. ### Schema generation and ComplexTypeSystem - **JSON schema:** describes the 1.05 layout with the compact/verbose null rules, and now validates real `JsonEncoder` output. - **XSD:** - `EncodingMask` (`xs:unsignedLong`) only for StructureWithOptionalFields. - Guid and StatusCode are complex types. - Standard types are in the `Types.xsd` namespace. - **Structure-backed OptionSets** are described as `{Value, ValidBits}`. New `UaTypeDescription.IsStructureOptionSet` property and constructor overload. - **ComplexTypeSystem:** - A malformed type in a server dictionary is skipped per type instead of discarding the whole dictionary. - Cyclic supertype walks are bounded. - Declared definitions are no longer changed. - A dictionary validator is kept only if the dictionary parsed. ### Certificates and trust lists (Part 4 6.1.3, Part 12) - **Revocation:** - Checked for issuers supplied in the peer's chain and for inline-trusted issuers, against the CRLs in the trusted and issuer stores. - A store without CRL support no longer hides "revocation unknown". - `RejectUnknownRevocationStatus` is honoured for inline-trusted issuers unless the entry sets `SuppressRevocationStatusUnknown`. This changes the strict case of `ExplicitIssuerChainIsHonoredWithoutStorePathAsync`. - **Basic constraints / path length** violations fail with `Bad_CertificateInvalid`. - **`TreatAsInvalid`:** - Now yields `Bad_CertificateUntrusted`, which can't be suppressed or auto-accepted. - It is honoured on individual trust-list entries even when the certificate is also in a store. - **`EccEncryptedSecret`:** enforces the result of validating the sender certificate. - **`TrustListTransaction`:** - Routes each CRL to the store holding its issuer (TrustedCrls/IssuerCrls). - Validates staged CRLs, including the store's CRL support, before changing anything. - Applies certificates before CRLs. - **Other:** - One failing `CertificateChanges` observer no longer stops the others. - `InMemory:` stores share their contents per path, and `InMemoryStoreProvider` is `IDisposable`. ### WoT (`Opc.Ua.Types/Wot`) - **Projection security:** projection security schemes whose qualified names collide are refused. New diagnostic `WotDiagnosticCode.ProjectionSecurityConflict = 6063`. - **Input handling:** - Invalid Unicode (lone surrogates, bad UTF-8) is rejected as `JsonException`. - External-schema results are keyed per affordance. - A duplicate security-floor member is rejected. - `nsu=` ids are percent-encoded (Part 6 5.1.12). - The configured JSON depth is honoured on re-parse. - An oversized document is reported before it is parsed back. - **Performance and limits:** - Subtype-graph validation, `componentOf` linking and the namespace table are now linear. - Namespaces are capped at UInt16. ### New or changed public API - `ICertificateValidatorProvider` (server) - `SecurityPolicyInfo.GetUserTokenSignatureData(..., MessageSecurityMode)` overload - `XmlDecoder.TreatWhitespaceOnlyStringsAsEmpty` - `UaTypeDescription.IsStructureOptionSet` plus a constructor overload - `WotDiagnosticCode.ProjectionSecurityConflict` - `StatusCodes.BadIndexRangeDataMismatch` in the `Opc.Ua.Types` list - `InMemoryStoreProvider : IDisposable` ## Not changed / open - **Deferred:** - S1-14 (BSD OptionSet size needs the base type in `UaTypeDescription`, and BSD is deprecated). - C2-7 (restoring the own-certificate trust exemption would accept certificates the spec says to reject; removing the dead code is a separate decision). - A1-4 (config values keep surrounding whitespace, as the pre-April-2026 loader did). - H-2 (JsonDecoder has no real nesting counter). - A2-3 (the StringTable cap also applies to diagnostics tables; default `MaxArrayLength` already bounds them). - **Needs a product decision:** - The general `ConvertToString` writes a QualifiedName as `ns:Name`, while Table 121 drops the namespace. - Short (< 32 byte) client nonces are accepted on None channels. ## Verification - Every commit added a regression test that fails without its fix, where practical. - Before the rebases, with all audit and review fixes (before the `Spec: #…` commits): 0 build warnings and 16 test suites green on net10.0. The suites: Types, Core, Core.Encoders, Core.Security, Core.Schema, Client, Client.ComplexTypes, Server, Sessions, Gds, History, Features, Redundancy.Server, PubSub, WotCon and WotCon.Bindings. Core hit its known intermittent end-of-run hang once, and reruns were clean. - The `Spec: #…` fixes were verified per group on their own suites. Those include PubSub.Udp, Redundancy.PubSub, PubSub.Server, Configuration and SourceGeneration.Core. - **Not yet built or tested on this base.** It was rebased onto master (resolving overlaps with #4522, #4540 and #4564) and then onto #4569. This PR stays a draft until the full run on this base is green. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com> Co-authored-by: Marc Schier <marcschier@users.noreply.github.com>	2026-09-29 11:14:15 +00: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	fa8d45f2cf	Fix server/core validation, lifecycle and protocol regressions (#4477) ... # Description **Server/core audit fixes with regression tests. Latest review-fix validation, merge validation and earlier acceptance results are distinguished below.** This addresses a 95-finding server/core report. Every finding was reviewed with the OPC UA specification knowledge base or, where the standard does not prescribe a C# implementation, the corresponding ownership/configuration contract. Incorrect or withdrawn subclaims remain qualified rather than being presented as additional fixes. The original checkpoint covers session identity/certificate validation and revocation; certificate/key ownership and transactional updates; TCP reconnect, receive/admission and buffer lifetime; HTTP quotas; subscription admission, durability and pooled payloads; file-operation lifecycle; historian aggregates; fluent/state-machine behavior; and related utilities. It includes targeted regressions and directly related documentation. The approved acceptance cleanup is separate in the commit history: | Cleanup | Verified result | | --- | --- | | Invalid identity-policy and KeyCredential fixture inputs | Fixtures explicitly advertise Username and use its generated policy ID. Credential persistence uses valid clear mode inside the existing encrypted admin context. Original permission, lockout and negative encryption assertions remain. | | Native GDS lifetime and concurrent connection failures | Retain ApplicationInstance until client teardown. A deterministic native reproducer proved the test-only channel cap of 10 was exhausted; removing that override restores the existing bounded default of 1000. No global serialization or production quota change. | | Stress certificate ownership and close accounting | Balance two helper references and the fake transport's server-certificate ownership. Replace the invalid event-count bound with exactly-once physical disposal, opens = closes, zero active entries and one logical entry lifetime. | | Participant deadlines | Use the injected TimeProvider with the existing cross-framework WaitAsync overload. Tests pin one tick before, exact expiry, terminal notification, recovery and timely completion. | | Reproducibility and platform assertions | Preserve canonical WoT bytes/hashes, explicit display-culture contracts and exact numeric-vector assertions. Accept Apple's CryptographicException subtype without weakening disposal balance. | | Missing user-signature proof | Reject null/empty proofs before OS crypto. Return BadUserSignatureInvalid per Part 4, 5.7.3.3. None-policy and valid-signature controls remain. A CI-only test nullable-context error was corrected without changing production behavior. | | Durable sample restore and shared test storage | Dequeue waits for actual batch residency instead of treating failed restore completion as success, and restored files close before deletion. Five deterministic regressions failed before the fix. The two fixtures that clear the shared batch directory are isolated; all original 3,000-item assertions remain unchanged. | ## Latest review follow-up Commit `828856850219518970e8e2f580b8f94b3c1fc748` fixes the deferred robotics disposal reported in the latest review. `RobotIntentNodeManager.DisposeAsync` now joins a shared controller-host drain and then explicitly awaits base node-manager disposal. A controller's bounded shutdown wait may defer its cleanup, but no longer lets the node-manager override finish early. Concurrent callers, including an asynchronous call after synchronous `Dispose`, cannot bypass the drain after the host list has been detached. Synchronous disposal remains nonblocking for deferred work. Deferred initialization failures are logged and retained, and base cleanup failures propagate to asynchronous callers. The existing controller-level bounded disposal contract is unchanged; the node-manager contract is documented in `docs/Robotics.md`. | Regression requirement | Evidence | | --- | --- | | Wait for deferred hosts; preserve synchronous compatibility and concurrent joins | `NodeManagerAsyncDisposalWaitsForDeferredHostsAsync`: four cases covering both entry-point orders and zero/positive host shutdown waits. All four failed before the fix. | | Wait through base cleanup and observe failures | `NodeManagerDisposalWaitsForBaseCleanupAndReportsItsFailureAsync`: four cases hold the actual base-resource semaphore, verify pending callers after the host finishes, then check success or the same propagated exception and exactly-once cleanup. | The focused `IntentHostingCoverageTests` and `IntentControllerHostTests.Dispose` selection passed **30/30 on net10.0 and 30/30 on net48**, with no failures or skips. The final net10 solution build passed with zero errors and only the four previously documented CA2249 warnings in unchanged fuzz-test code. No new warnings, analyzer suppressions, dependencies, or weakened assertions were added. The review thread received a commit-specific reply and was explicitly resolved. This verification is scoped to the robotics feedback; the earlier full Subscriptions-run limitation remains recorded in the merge section below. ## Documentation review follow-up (2026-09-14) Commit `f9a9686fcc5d3cfd19cbbc8c2d826e20f5c7e72f` addresses the six follow-up threads on documentation and implementation rationale. Each has an in-thread reply citing the commit and an explicit resolution. Multiline XML documentation now covers the declarations introduced or modified throughout the entire PR, including internal contracts, samples, tests, and public members of related private helpers. A syntax-aware inventory checked **2,031 declarations across all 256 PR C# paths with zero documentation gaps**. A supplementary check also covers **75 previously unchanged public members** of private helpers involved in the changed code. Token comparison across the conditional-compilation branches proves this follow-up changes **no executable C# or test assertions**. Project-file changes are rationale comments only. The implementation questions are documented at their source: | Question | Resolution | | --- | --- | | Certificate assembly friendship | Server Push configuration reuses the internal explicit-subject CSR helper with the existing key; no public crypto API was added. | | Core.Types assembly friendship | Server builders use internal transition callbacks/revision guards; Server.Tests checks the revision deterministically without creating a public compatibility contract. | | Always-single-reader notifications | One dequeuer preserves event order; optional concurrency fans out within one event and joins before the next event. | | Asynchronous disposal completion | `DisposeAsync` already joins the completion published after admitted operations and all cleanup attempts. The compatibility `Dispose` remains nonblocking to avoid deadlocking an admitted callback against its own drain. | The requested detailed review disposition follows. It distinguishes actual fixes from confirmed policies and incorrect subclaims rather than counting every comment as a new runtime change. ### Detailed disposition of review 5191795284 The [SDK review](https://github.com/OPCFoundation/UA-.NETStandard/pull/4477#pullrequestreview-5191795284) was submitted against `1c4a9df6c`, before the subsequent merge and feedback fixes. Its **15 inline threads** have each received an action-specific reply and an explicit resolution. A fresh GraphQL query verified those replies and resolution states; the fixes in `b8a59990f` remain ancestors of the current branch. | Review concern | Action and disposition | Concrete regression evidence | | --- | --- | --- | | Injected certificate-store providers bypassed | **Fixed.** Registration, configuration snapshots, and validation use the injected providers. Explicit custom types remain authoritative; the built-in Directory catch-all is deferred so native X509 paths and later custom providers are not captured. Failed store opens dispose the created store. | `InjectedProviderResolvesRegisteredAndConfiguredTrustStores` covers default-provider ordering and both registration paths; provider validation/open-failure cases and the unchanged `TrustedCertificateInjectedViaIndependentX509StoreIsObservedAsync` cover real use and independent-store visibility. | | Unguarded timer-thread leadership callbacks | **Fixed.** Each subscriber's exception is logged independently and cannot suppress subsequent subscribers or fail lease operations. `IsLeader` reads the deadline without invoking application callbacks. | `ThrowingLeadershipSubscriberDoesNotFailAcquireOrDisposeAsync`, `ExpiryNotifiesOtherSubscribersWhenOneThrowsAsync`, and `IsLeaderDoesNotDispatchExpiryNotificationsAsync` were all demonstrated failing before the fix. | | Legacy pending-key provider accepts an unfinishable regenerated CSR | **Fixed by explicit capability validation.** A provider implementing only `IPendingCertificateKeyStore` receives `BadNotSupported` before key generation or persistence when regeneration is requested. Existing-key CSR/update remains supported. Regeneration requires matching-only consumption; no unsafe destructive probing was introduced. | `LegacyPendingProviderRejectsRegenerationBeforeCreatingOrSavingAKeyAsync` checks no key or store mutation; `LegacyPendingProviderCompletesExistingKeySigningAndUpdateAsync` proves the supported flow completes. | | Shutdown waits behind user deactivation | **Fixed.** Accepted deactivations drain before the master's lifecycle gate is taken. Configuration retains its binding until the shared drain completes, including concurrent shutdown callers and direct master disposal. | Four cases of `ShutdownDrainsDeactivationOutsideTheSessionClosingGateAsync` exercise real configuration/binding/ServerInternalData/master reentry, retained address space, exact session/subscription closure, and no production cancellation shortcut. | | Subclass cleanup runs under the operation-lifetime lock | **Fixed.** Cleanup starts outside that lock. A pre-published completion joins all disposers, and only the active asynchronous teardown context may use guarded helpers. Captured contexts lose that privilege when the callback exits. | `SynchronousOwnedCleanupLetsSamplingWorkerReachClosedAdmissionAsync`, `DisposeCoreCanUseGuardedMembersWithoutReopeningCallerAdmissionAsync`, captured-context rejection, and exact cleanup-failure/ownership assertions cover both synchronous and suspended cleanup. | | Discovery requestId-zero chunk read after pool return | **Fixed by the earlier merge.** Discovery validation precedes `SaveIntermediateChunk`; no read occurs after that path can return the buffer. The ordering is retained. | `DiscoveryChannelReadsTheFirstChunkBeforeItIsReturnedAsync` uses a poisoning pool and the requestId-zero path. | | T1.8 OPN cleanup misses post-decryption throws | **Fixed by the earlier merge and pinned with new regressions.** Both sequence rejection and certificate mismatch release the decrypted body and input ownership. | `OpenSecureChannelRejectionAfterDecryptionReturnsEveryRentalAsync` checks the exact audit status and pool conservation. Omitting either corresponding release in an isolated snapshot made its case fail; no mutation remains. | | T1.2 mount-root protection depends on the provider | **Fixed at the host boundary.** Separator-only aliases resolve to the protected root. Traversal, empty/whitespace segments, backslashes, and drive/stream qualifiers are rejected before arbitrary providers are invoked. Valid nested paths and root destinations remain supported. | A deliberately permissive provider is used by `NonCanonicalObjectPathsCannotDeleteMoveOrCopyAsync` and invalid-target cases; exact zero-call checks are paired with legitimate nested-path/root-destination controls. | | Inline trusted/issuer entries now face CRL checks | **Intentional strict behavior, documented and tested.** A configured backing store with strict unknown-status rejection requires current revocation information. Explicit-only lists without a store retain their no-store behavior. Actual revocation cannot be suppressed; a matching store entry's policy takes precedence. | `InlineIssuerWithStoreHonorsStrictUnknownRevocationPolicyAsync`, `InlineIssuerRevocationIsNeverSuppressedAsync`, store-precedence cases, and explicit-only controls preserve the missing-information versus actual-revocation distinction in Part 4 6.1.3. | | RSA minimum applied to unspecified ECC identifiers | **Fixed at the application-certificate check.** The RSA minimum is compared only with an actual RSA public key. An unspecified ECC identity is not replaced simply because its key size is below an RSA threshold. | `SilentCertificateCheckPreservesUnspecifiedEccIdentityAsync` checks DER, thumbprint, store contents, and private-key retention; explicit RSA and HTTPS minimum controls remain. | | Failed directory refresh wedges corrective mutations | **Fixed.** Recoverable automatic pre-refresh failures are logged and leave reconciliation pending, but do not block corrective provider mutations. Explicit refresh errors, capacity admission, caller cancellation, and actual provider denials remain enforced. | `CorrectiveDeleteRecoversAfterPersistentOverLimitRefreshFailureAsync` converges an externally over-limit directory back to its exact allowed contents. Capacity, cancellation/token, and six refresh-error controls check negative paths. | | Timed-transition rejection logs at 10 Hz | **Fixed.** Repeated `(state, revision, status)` warnings are deduplicated without changing the retry cadence. A different status or state entry permits another warning. | `RepeatedTimedRejectionLogsOnceAndRecoversAsync` checks one warning over eleven rejections and success on the next attempt. Status-change and same/different-state reentry controls pin reset behavior. | | Bad unsubscribe strands retired-generation bookkeeping | **Fixed.** Retired all-event bookkeeping completes even when unsubscribe reports Bad; the first Bad is still returned to the caller. | Good and BadCommunicationError cases of `RetiredAllEventsDeletionCompletesBookkeepingAndReleasesGenerationAsync` check event/snapshot removal, generation release, and exactly one unsubscribe without finalization replay. | | CSR subject parsing and fallback | **Fixed for existing and regenerated keys.** Omitted subjects fall back to the actual stored certificate. Regeneration resolves the public certificate's subject before creating a key. Malformed or unresolvable subjects return `BadInvalidArgument` before pending-key mutation. | Store-only fallback, real-generator null/empty cases, and missing-subject/malformed-DN controls verify the signed CSR, matching fresh pending key, unchanged active identity, and no destructive side effects. | | Committed RemoveUser change reported as failed | **Fixed for RemoveUser and ModifyUser.** Accepted cleanup is still awaited and close failures remain logged, but cannot replace the committed mutation result. Precommit failures retain their original result and side effects. | `CommittedUserChangeKeepsResultAndPublishedUsersWhenCloseFailsAsync` checks published Users, Good result, logging, and continued closes. `FailedUserChangeKeepsPublishedUsersAndDoesNotCloseSessionsAsync` checks BadNotFound with no publication or close. | | Alleged pooled decoded-request aliasing | **No runtime redesign needed; reviewer rebuttal confirmed.** The decoder copies the relevant input data. Misleading ownership comments were corrected rather than introducing a new dispatch model. | `DecodedRequestRetainsValuesAfterInputBuffersAreReturnedAsync` checks the exact decoded NodeId and ByteString after returned arrays are poisoned. | The current documentation-only follow-up rechecked **104 Core, 473 Server, and 4 Configuration cases** covering these concerns: **581 passed, zero failed or skipped**. The final net10 solution build also passed with **zero warnings and errors**. The complete earlier Core/Server/Configuration and net48 results remain listed separately rather than being presented as rerun current-head acceptance. ## Earlier review follow-up Commit `b8a59990fe2e8e967a12ffd5d9158beed7831143` addresses all 19 review threads. Each thread has an in-thread reply naming this commit and was explicitly resolved after verification; the final paginated query found 19 resolved and zero unresolved threads. The follow-up fixes injected certificate-store selection (including default provider ordering and native X509 paths), actual-RSA minimum enforcement, and CSR capability/subject validation before key mutation. Node-manager teardown uses simpler scoped ownership and drains accepted user revocation outside the master gate; committed user results and retired-event cleanup remain correct when cleanup reports failure. File mutations can recover after failed refresh without bypassing capacity or cancellation, provider paths protect mount roots, and repeated timed rejections no longer flood logs. Leadership subscriber failures are isolated without callbacks from `IsLeader`. Regressions also pin the already-merged OPN/discovery buffer fixes and prove decoded request values do not borrow returned input buffers. The strict inline issuer revocation policy is documented and tested, not weakened. Existing tests were changed only for the reviewed behavior: the incorrect committed-user result was corrected, and inline-issuer/root-alias controls were expanded. ### Review-fix validation | Suite | Scope | Framework | Passed | Existing TRX NotExecuted | | --- | --- | --- | ---: | ---: | | Core | Complete project | net10.0 | 4,970 | 88 | | Server | Complete project | net10.0 | 6,414 | 69 | | Configuration | Complete project | net10.0 | 231 | 0 | | Core | Certificate, lease and buffer compatibility slice | net48 | 153 | 0 | | Server | Reviewed lifecycle, user, file, timer and CSR compatibility slice | net48 | 692 | 0 | | Configuration | Complete project | net48 | 231 | 0 | | CSR provider regressions | Final portable-mock follow-up | net10.0 | 11 | 0 | All runs exited successfully with normal concurrency and leak detection. The Server console reports 5 skips; its TRX additionally records existing assumption/explicit cases, hence the 69-record count above. The Framework build caught two modern-only `ValueTask.CompletedTask` usages in new mock setup; only those expressions changed to the equivalent portable `default(ValueTask)`. The complete modern suites precede that test-only correction; all production files remained byte-identical, and the corrected fixture passed on both targets. The final net10 solution build succeeded with 0 errors and the same 4 CA2249 warnings in unchanged fuzz-test code. Diff checks and changed-line whitespace checks are clean; whole-file formatting still identifies 10 unchanged baseline lines, which were not reformatted outside the feedback scope. The committed 35-file tree was hash-verified against the validation inputs. This is the review-feedback verification scope, not a rerun or sign-off of the historical complete platform/native/soak acceptance campaign below. ## Latest upstream merge Merge `9d9bf679f317164c5b2b90924cd3cdc27a86d451` integrates `master` through `162fc5ebb` (#4486), including the AddNodes child-name index, shared certificate-error reporting, CTT sample-data corrections, and the accompanying regressions and documentation. The CreateSession conflict preserves this branch's certificate ownership, required certificate, application-URI, and channel-certificate checks. The validation result now retains nested errors through `ThrowIfInvalid`, allowing the upstream shared reporting helper to mask trust, policy, and revocation failures consistently without losing audit detail. The new indexed AddNodes regression initially attempted to reuse a deterministic NodeId after renaming its existing node. Renaming frees the BrowseName, not that NodeId. `IndexedBrowseNameReusePreservesExistingNodeIdentityAsync` now proves both contracts: the derived-ID collision leaves the original node intact, then a fresh explicitly requested ID can reuse the old name while the new name remains protected against duplicates. No production identity rule was relaxed. The incoming seeded random-index test uses the existing `UnsecureRandom` wrapper with the same seed and sequence; no new analyzer suppression was added. | Local merge validation | Framework | Passed | Existing exclusions | | --- | --- | ---: | ---: | | Core, complete project | net10.0 | 5,081 | 88 | | NodeState / BaseInstanceState | net10.0 | 579 | 0 | | Server node management, certificate and session-security selection | net10.0 | 112 | 0 | | InformationModel node-management services | net10.0 | 29 | 1 | | NodeState / BaseInstanceState | net48 | 578 | 0 | | Core certificate validation, error reporting and TCP buffers | net48 | 111 | 0 | | Server node management, certificate and session-security selection | net48 | 112 | 0 | All selected runs exited successfully at normal concurrency: **6,602 passing test executions**. Modified test assemblies were explicitly rebuilt, and the combined name/identity regression was verified by name in both net10 and net48 results. No skips or weakened assertions were introduced. The final `dotnet build UA.slnx -c Release --no-restore -m:1` with `CustomTestTarget=net10.0` succeeded with **zero warnings and zero errors**. The resolved source and adjusted tests pass whitespace verification, and their source hashes match the validation inputs. This is merge-specific verification. Earlier review results and the previously reported incomplete full Subscriptions runs below remain scoped to their own commits; they were not reopened or relabeled as passing by this operation. ## Previous upstream merge (2026-09-15 morning) Merge `c4694e20ab089c555af233642042133ff3a03ac8` integrates `master` through `484d6c887`, retaining #4481's shared Like parser/GDS query fixes, #4488's RequestHandle recovery in ServiceFaults, and #4487's expired-session and durable-transfer corrections. Three conflict resolutions preserve both sides: - Alias searches now use the shared `LikePattern` implementation, including upstream malformed-list validation. A timeout overload retains this branch's 100 ms per-alias deadline and `BadTimeout` reporting without keeping a second wildcard translator. The shared matcher's original unlimited overload remains unchanged for existing GDS/ContentFilter callers. New regressions cover bounded prefix/list scans, reuse after timeout, and the alias service result. - Expired activation closes outside the session-manager gate using upstream's once-only timeout claim, audit, and diagnostic accounting. This branch's original-channel and security-policy checks remain in place. - TCP buffer tests retain this branch's ownership, poisoning-pool and post-decryption checks alongside the upstream RequestHandle/fault overload tests; neither set was discarded. The upstream HTTPS binary behavior is intentionally retained: a decoder `ServiceResultException` produces HTTP 200 with an OPC UA ServiceFault carrying the decoder status, response timestamp, and recoverable RequestHandle, rather than the previous HTTP 500 text response. Other exceptions retain their existing HTTP error handling. | Local validation | Framework | Passed | TRX NotExecuted | | --- | --- | ---: | ---: | | Core, complete project | net10.0 | 5,054 | 88 | | Sessions, complete project | net10.0 | 780 | 610 | | Server aliases, expiry/security and subscriptions | net10.0 | 288 | 0 | | GDS database, paging and live directory calls | net10.0 | 263 | 0 | | Shared Like patterns and bounded evaluation | net10.0 | 109 | 0 | | Subscriptions transfer/durability/service selection | net10.0 | 23 | 1 | | HTTPS WebApi, complete project | net10.0 | 346 | 0 | | InformationModel aliases | net10.0 | 54 | 5 | | Shared Like patterns and bounded evaluation | net48 | 109 | 0 | | Core fault handles, TCP/HTTPS and filters | net48 | 268 | 0 | | Server aliases, expiry/security and subscriptions | net48 | 288 | 0 | These selected runs exited successfully with normal test concurrency: **7,582 passing test executions**. Sessions' console reports 379 skips; the TRX also contains conditional/assumption exclusions, so the table uses the actual 610 NotExecuted records. No skips or weakened assertions were added. The new `EvaluationTimeoutBoundsWorkAndPreservesPatternReuse` and `SharedPatternTimeoutRemainsAnAliasServiceError` tests passed on both targets. **Validation limitation:** two unfiltered Subscriptions runs did not produce a final result and were stopped, including a retry with a hang guard. They are not counted as passing. The isolated failover fixture passed 2/2, and the transfer/durability/service selection above passed. The stall's cause was not established; no unrelated code or test was changed to conceal it. The final `dotnet build UA.slnx -c Release --no-restore -m:1` with `CustomTestTarget=net10.0` succeeded with **0 warnings and 0 errors**. Whitespace verification passed for all hand-reconciled files, and staged source hashes match the validation inputs. These are merge-specific checks, not a new full platform/native/soak acceptance sign-off. Earlier review and acceptance evidence below remains dated to its own commit. ## Previous upstream merge (2026-09-14) Merge `c3e09553e4d1509502d4ba751960d58491bea2cf` integrates `master` through `6313e38a0` (#4478), including the CTT-driven Part 13 aggregate corrections and FindAlias argument validation. The upstream tests and CTT documentation are retained. The single conflict in `AliasNameWildcardMatcher` is reconciled through the branch's shared regex compiler: upstream malformed-pattern rejection and `IsValidPattern` coexist with OPC UA character escaping, whole-string/newline matching, the 100 ms timeout, and service-level error reporting. The upstream ReferenceTypeFilter checks and chronological aggregate behavior are preserved. Local merge verification: | Suite / selection | Framework | Passed | Existing skips | | --- | --- | ---: | ---: | | Server aliases, aggregates and historian | net10.0 | 1,011 | 0 | | Server aliases, aggregates and historian | net48 | 1,011 | 0 | | InformationModel aliases | net10.0 | 54 | 5 | | Features aliases | net10.0 | 11 | 0 | | Client aliases | net10.0 | 28 | 0 | | History aggregate/historical-access consumers | net10.0 | 265 | 0 | All selected tests passed at normal concurrency. The final net10 solution build passed with 0 errors and 0 warnings. The resolved file's whitespace check and the staged diff check passed. These are merge-specific checks, not a new full platform/native/soak acceptance claim. ## Previous upstream merges Integrates `master` through `8e874d073`, including the Client, Types, Core and source-generator audit fixes in #4471, #4473, #4474 and #4475, plus the event queue-size revision from #4480. The merge commits are `150ea0439` (26 conflict resolutions) and `a0e17c07c` (the base update that arrived during validation). The overlapping fixes are reconciled rather than replacing either branch: - **Rejected-certificate retention follows this branch, as explicitly requested:** zero means unlimited history; negative values disable new storage while preserving existing backend trimming behavior (Directory clears prior history; shared key/value leaves it intact). The processor and manager policy are not redefined, and documentation/tests describe the distinction. - Preserve owned certificate snapshots and borrowed validation-core lifetimes alongside upstream cache invalidation, store-first CRL checks and `BadCertificateRevocationUnknown` for unsupported revocation lookup. - Combine exactly-once chunk ownership, receive-loop cancellation, stale-epoch rejection and detach-before-adoption with upstream signature-before-padding verification and secret cleanup. - Preserve pooled `HttpClient` per-request configuration together with bounded response reads and deadlines that remain active through body consumption. Regression cases exercise already-used clients and both timeout precedence cases. - Retain the binary-compatible channel-key constructor and normalize omitted profiles; preserve exact profile filtering and upstream binary-first HTTPS/WSS endpoint ordering. - Preserve monotonic lease expiry and stale-confirmation fencing while adding ordered, non-reentrant leadership notifications outside the state lock. ## Validation Local validation of the conflict-resolution merge: | Suite | Framework | Passed | Existing skips | | --- | --- | ---: | ---: | | Core | net10.0 | 4,940 | 88 | | Client | net10.0 | 2,278 | 0 | | Server | net10.0 | 6,088 | 5 | | Types | net10.0 | 10,394 | 1 | | SourceGeneration.Core | net10.0 | 3,940 | 8 | | SourceGeneration | net10.0 | 185 | 0 | | Core | net48 | 4,855 | 104 | After the subsequent four-file #4480 update, the related Server slice passed 811 tests with 5 existing skips, the real durable queue-size cases passed 6/6, and EventManager's 21 cases passed again after analyzer-only expected-array cleanup. The final `dotnet build UA.slnx -c Release` with `CustomTestTarget=net10.0` succeeded: 0 errors and 4 pre-existing CA2249 warnings in unchanged fuzz-test code. Whitespace verification passed for the resolved files and the follow-up. Merge-specific proof includes `RequestTimeoutRemainsActiveWhileReadingTheOpenApiBodyAsync` on already-used shared clients with either the request or HttpClient timeout shorter, the existing ownership/expiry regressions from both branches, and both branches' retention tests adjusted only to the explicitly chosen zero/negative contract. Earlier acceptance at `1c4a9df6c` passed hosted Linux/macOS CI, CodeQL, image builds and the canonical coverage gate (87.15% line, 77.27% branch, 92.14% patch). Both Framework project matrices, all three Windows native suites (183 cases), and the unfiltered net10 stress project (230 cases, including full 60-minute chaos and 30-minute memory soaks) also passed. Those are **pre-merge results**, not a claim that the full framework/platform/native/soak matrix has been rerun on this merge. The previous acceptance workflow remains separate from this review-feedback operation. Security checks, meaningful assertions, leak detection, default native concurrency, full soak durations and coverage thresholds remain intact. The user-set ready-for-review state is unchanged. No architectural or complete merged-head acceptance sign-off is claimed. ## Related Issues - Fixes #4476 ## 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. - [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. - [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. - [x] I have addressed **all** PR feedback received.	2026-09-16 04:42:09 +00:00
Copilot	76efb34ab8	Fix FileSystemClient path qualification and canonical paths (#4365) ... ## Summary - Qualify unprefixed `CreateDirectoryAsync` path segments with the current parent namespace. - Preserve qualified segments after cached and uncached resolution so returned `FullPath` values are canonical. - Keep explicit namespace-prefix validation tied to caller input and use the server-reported `BrowseName` after creating a directory. - Add unit coverage for both `createIntermediate` modes, canonical file and directory paths, and cached resolution. - Add an integration regression using the reference server, `FileSystemNodeManager`, and `PhysicalFileSystemProvider`. ## Testing - `Opc.Ua.Client.Tests` filesystem category on `net10.0`: 118 passed. - `FileSystemClientIntegrationTests` on `net10.0`: 14 passed. - `Opc.Ua.Client.Tests` filesystem category on `net48`: 118 passed. - `FileSystemClientIntegrationTests` on `net48`: 14 passed. Fixes #4362 --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: romanett <7413710+romanett@users.noreply.github.com> Co-authored-by: Marc <marcschier@hotmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>	2026-09-01 20:30:15 +00:00
romanett	70161bc145	Fix Part 18 RoleSet: AddRole overwriting nodes, and Roles configured at startup never getting one (#4367) ... # Description Two defects in the Part 18 Role management implementation added by #3735. They are separate root causes but both concern Roles that are not one of the nine well known ones. ## 1. `AddRole` could allocate a NodeId that is already in use `RoleManager.AllocateDynamicNodeId` counted up from `i=1` in whatever namespace it was given, with no check that the identifier was free. The namespace came from the `NamespaceUri` argument of the standard `RoleSet.AddRole` Method, so a client holding `SecurityAdmin` could pass the namespace URI of a node manager's own model and receive the first identifier in it — which every model compiler and the source generator have already used. `MaterializeDynamicRoleAsync` then created the Role at that NodeId, replacing the model's node in the address space of a running server. The root cause is that `NamespaceUri` was treated as selecting the namespace the Role's NodeId is allocated in. Per Part 18 §4.2.2 it qualifies the **BrowseName**; the NodeId is server-assigned. The fix works on three levels: - `RoleManager.AddRole` resolves `NamespaceUri` into the BrowseName namespace only — recorded as both an index and a URI on the new `RoleEntry.NamespaceUri`, so the binding can re-resolve it against the server namespace table — and always allocates the NodeId in `defaultNamespaceIndex`, which `RoleStateBinding` picks from a namespace the RoleSet's NodeManager owns. A client can no longer steer allocation into another node manager's model at all. This also fixes a second-order problem: a Role node materialized into a foreign namespace was indexed in the diagnostics manager but routed elsewhere by the `MasterNodeManager`, so it could never be resolved. - `AllocateDynamicNodeId` skips identifiers that are already taken — by another Role, or by a node in the address space, via a probe `RoleStateBinding` installs. This is the fix suggested in the issue. - Materialization refuses rather than replaces. An occupied NodeId answers `Bad_NodeIdExists`, one outside the manager's namespaces answers `Bad_NodeIdInvalid`, both logged; `AddRole` rolls the manager back so it is all-or-nothing and never leaves a Role that grants access but has no node. ## 2. A Role configured at startup was enforced but got no node in the `RoleSet` `RoleStateBinding.Initialize` discovered Roles by walking the address space only, and `MaterializeDynamicRoleAsync` ran from `OnAddRoleAsync` alone. `RoleConfigurationChangeKind.RoleAdded` for an unbound Role did nothing. Every documented way of configuring a custom Role before the server is up — overriding `StandardServer.CreateRoleManager`, `IOpcUaServerBuilder.ConfigureRoles(...)`, `AddRoleManager<TManager>()` with a pre-populated manager — therefore produced a Role that `MasterNodeManager.ValidateRolePermissions` honours but that a client cannot browse, read `Identities` from, or call `AddIdentity` on. Part 18 §4.2 expects the `RoleSet` to describe the Roles the server supports, so this is a conformance gap as well as an ergonomic one. - `InitializeAsync` now materializes every `IRoleManager.RoleIds` entry the address-space walk did not return. - `OnRoleConfigurationChanged` handles `RoleAdded` for Roles that are not bound yet, so a Role added by a custom `IRoleManager` at runtime also appears. The manager raises the event synchronously from inside itself, so that work is queued rather than awaited in the handler. - Both go through one idempotent, serialized `EnsureRoleMaterializedAsync`, which is also where the NodeId checks from defect 1 live. ## Third-order fix: `ConfigureRoles` allocated NodeIds against a throwaway namespace table `ApplyRoleConfiguration` ran inside the DI factory for `IRoleManager`, where the server's namespace table does not exist yet. It built a local `NamespaceTable` and defaulted to index 1, so a configured custom Role was allocated in a namespace the RoleSet's NodeManager does not own — which the refusal added above would then correctly reject, leaving the Role invisible again and defect 2 unfixed for the `ConfigureRoles` path. The options are now staged on the manager (`RoleManager.PendingConfiguration`) and applied by the binding once the address space is up, against the real namespace table. Settings on Roles that already exist — the nine well-known ones — are still applied eagerly in the DI factory, so resolving `IRoleManager` before start-up behaves as it did. ## Notes for reviewers - **`RoleStateBinding.Bind` becomes `BindAsync`.** Materializing needs `AddPredefinedNodeAsync`. The only caller was `ServerInternalData`, in an already-async method. This is a public API change on a type introduced in `2.0.0-preview.2`. - **`AddRole`'s returned NodeId namespace changes** for a call that passes a non-empty `NamespaceUri`: it is now the server-chosen namespace rather than the caller's. This is the fix, and the existing XML doc on `IRoleManager.AddRole` already described this behaviour ("a fresh NodeId is allocated in `defaultNamespaceIndex`") — the implementation was what deviated. The BrowseName namespace is unchanged. - **`AddRole` is now all-or-nothing.** Materialization failure used to be logged and swallowed, returning `Good`. It now rolls the manager back and returns the reason, per the issue's "refusing would at least turn silent corruption into a diagnosable error". - **`RoleEntry` gains `NamespaceUri`** and its internal constructor gains a parameter. - The unit fixture in `RoleStateBindingTests` now logs "not in a namespace owned by the RoleSet's NodeManager" for eight well-known Roles. That is a fixture artifact — its test node manager does not own `ns=0`, unlike a real `DiagnosticsNodeManager` — not a real-server condition. - Unrelated but adjacent: `AddRoleHandler_TypedRoleStateUpgradedByBinding_CanCallAddIdentity` added a Role named `Engineer`, which `AddRole` rejects as a duplicate browse name. Its `Assume` never passed and its body never ran, on master too. Renamed to a non-well-known name so it exercises what it claims. ## Testing - `RoleManagerTests`: NodeId allocated in the default namespace while the BrowseName keeps the requested URI, unregistered URI rejected, identifiers reported in use by the address space skipped, an existing Role's NodeId never reused. - `RoleStateBindingTests`: `AddRole` with a foreign model namespace leaves the model's own node at `i=1` untouched; a stub `IRoleManager` that hands back an occupied NodeId gets `Bad_NodeIdExists` and a rollback; a Role configured before the address space existed is materialized and referenced by the `RoleSet`; `ConfigureRoles` options are applied at bind time; a Role added on the manager after bind is materialized. - `ServerFluentApiHostingTests`: end-to-end on a hosted server — a custom Role from `ConfigureRoles` has a node under the `RoleSet` carrying its configured identity mapping. - `RoleManagementIntegrationTests`: end-to-end against the reference server over an encrypted channel — `AddRole` with the model namespace URI qualifies the BrowseName only and the Role is browsable. Locally on .NET 10: `Opc.Ua.Server.Tests` 4801 passed, `Opc.Ua.Features.Tests` 136 passed, `Opc.Ua.Core.Security.Tests` 556 passed, `UA.slnx` builds with no new warnings. ## Related Issues - Fixes #4361 Found while building an OPC UA Part 18 sample for OPCFoundation/UA-.NETStandard-Samples#818. ## Checklist - [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. - [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. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-09-01 19:28:43 +00:00
romanett	651beaee1e	Fully support AliasNames in ConsoleReferenceServer (#4012) (#4321) ... # Description The CTT AliasName conformance units skip or fail against the ConsoleReferenceServer. `AliasName Base` skips with *"No instance of AliasNameType has been found in the address space"*; `Category Tags`, `Category Topics` and `Hierarchy` fail. Part 17 support landed in #3779, but aliases live only inside the `IAliasNameStore` — nothing is browsable, no `AliasNameCategoryType` is nested under another, and the Topics aliases point at the Server object rather than at a `PublishedDataSetType` instance. Part 17 §6.2 clients — the CTT among them — discover aliases by browsing, not only by calling `FindAlias`. This PR materializes the store contents as real nodes and fills the three gaps. ### SDK — `Opc.Ua.Server` * **`DiagnosticsNodeManager.MaterializeRegisteredAliasNameNodesAsync`** (new, opt-in) walks every registered `IAliasNameStore` and creates one `AliasNameType` instance per alias — BrowseName carrying the alias name, `AliasFor` references to its targets, and the inverse `HasAlias` reference on each local target (via `externalReferences`, since targets usually live in another node manager) — plus an `AliasNameCategoryType` instance for every store category the standard NodeSet does not already ship. Categories whose NodeId belongs to another node manager are skipped, so an `AliasNameNodeManager` that also registers its store with the server-wide registry does not get its NodeIds claimed twice. Opt-in: servers that only need the well-known `FindAlias` to answer from their store pay nothing. The nodes are a snapshot taken at address-space creation — later mutations change what `FindAlias` returns but do not add or remove nodes. * **Optional Part 17 methods on the well-known categories.** The same pass instantiates whatever a category declares through `AliasNameCapabilities`: `FindAliasVerbose` (§6.3.3), `AddAliasesToCategory` (§6.3.4), `DeleteAliasesFromCategory` (§6.3.5) and `LastChange` (§6.3.1). Mutations require a `SecurityAdmin` caller on a `SignAndEncrypt` channel; the gate previously private to `AliasNameNodeManager` moved to `AliasNameMethodDispatcher` so both paths share one implementation. * **`LastChange`** is now tracked per category rather than only for the standard `Aliases` node, and the duplicated seeding logic collapsed into one helper. ### Reference server * A nested **`Devices`** category under `TagVariables` holds `Pump1_Status` and `Heater_Power`. `FindAlias` on `TagVariables` still returns all six names through subtree recursion, while `FindAlias` on `Devices` returns exactly two — which is what the Hierarchy unit checks. * `ReferenceServerConfigurationNodeManager` creates three **`PublishedDataSet` instances** under `PublishedDataSets` (`i=17371`) and the Topics aliases now target them, satisfying the *"is a Dataset"* check. These are address-space instances only — no PubSub runtime is added, and the CTT only inspects the target's type definition. ## Note for reviewers — the reserved NodeIds The optional children are created at the NodeIds the OPC Foundation reserves for them (`Aliases.FindAliasVerbose` = `i=24054`, `TagVariables.LastChange` = `i=32854`, …). Those identifiers are allocated in `StandardTypes.csv` but are **not** declared in the ModelDesign and **not** present in the published NodeSet, because the standard address space does not instantiate optional children. The source generator therefore emits no parent-to-instance mapping and no `MethodIds` constants for them, so `DiagnosticsNodeManager.ReservedChildIds` supplies the nine method ids and the two missing `LastChange` ids explicitly, each traceable to its registry row. The 18 argument-property ids do have generated `VariableIds` constants and are referenced through those. I deliberately did **not** "fix" this by declaring the children in `tools/Opc.Ua.SourceGeneration.Core/Design/StandardTypes.xml`: that file is a verbatim copy of upstream `UA-ModelCompiler/Design.v105/StandardTypes.xml` (the only differences today are upstream being newer), so a local edit is lost on the next sync and the NodeIds would silently revert to factory-minted ones. `docs/AliasNames.md` records this. Worth raising upstream separately: `StandardTypes.csv` allocates `Aliases_FindAliasVerbose` and siblings while the ModelDesign never declares those children, so the ModelCompiler cannot emit ids it has itself reserved. If that is ever reconciled, `ReservedChildIds` becomes redundant and deletes cleanly. ## Related Issues - Fixes #4012 ## Checklist - [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. - [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. — **not done: verified on net8.0 only** (Server 4710, InformationModel 1099, Core 4443, plus the client and features alias suites; AOT build clean). Needs a run against net framework and net10 before this leaves draft. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. ### Tests `tests/Opc.Ua.InformationModel.Tests/Aliasname*Tests.cs` mirror the four CTT conformance units and no longer `Assert.Ignore` where the CTT reported failures — the alias suite is 37 tests with no skips. New `AliasnameOptionalMethodsTests` covers the optional methods, their reserved NodeIds, the anonymous-caller denial, an add/find/delete round trip and `LastChange` advancing on mutation. One pre-existing test changed premise: `EnumerateSubCategoriesAsyncOnLeafReturnsEmptyAsync` asserted that `TagVariables` has no sub-categories, which this PR deliberately makes false. It now targets `Topics` (still a leaf) and a new test guards the nesting. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: romanett <7413710+romanett@users.noreply.github.com> Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>	2026-08-29 18:05:42 +00:00
Marc Schier	f0ab7878ea	Core stack: reusable file directory binder, geolocation seam and state machines (#4123) ... **Stack 1/5** — base `master`. Part of the #4096 split; see #4096 for the overall picture. ## What this is The core stack changes the Robotics, OpenUSD and Positioning work depends on. No new projects; every change is to an existing one, so this is reviewable without any of the companion specs. ## Contents **`Opc.Ua.Server`** - `FileSystem` gains a reusable `IFileDirectoryBinder` and `IFileSystemHost`, so any node manager can serve a Part 5 `FileDirectoryType` over `IFileSystemProvider` instead of that binding living inside one companion model. - Entry names are validated before they reach the provider. The contract promises forward-slash provider-relative paths but never enforced it, so a backslash payload such as `..\..\..\Windows\System32\evil.txt` bypassed the obvious provider-side check on Windows. Defence in depth — the shipped `PhysicalFileSystemProvider` was already safe. - `Location` adds a single `IGeoLocationProvider` seam so one provider implementation serves both OPC 10000-211 GPOS and OPC 10030 ISA-95. The two specs disagree on representation, not on content, so the difference is a projection rather than a reason to write the provider twice. Ships a WKT text formatter and an in-memory provider. - `StateMachines` adds `FiniteStateMachineDispatcher`, the Part 16 transition guard used by DI software update and later by Robotics task control. **`Opc.Ua.Types`** - Node copies no longer consume NodeIds from the `NodeIdFactory`. `Initialize` materialises children through `CreateChild`, which assigns a per-instance NodeId, but the copy then overwrites every child NodeId from its source, so the assignment is discarded. A counting factory silently burned identifiers; a tracking one leaked them. - `JsonDecoder` resolves a relative encodeable type id against the namespace table before the factory lookup. **`Opc.Ua.Client`** — a file system path segment written without a namespace prefix is qualified with the namespace of the parent it is resolved against, matching the provider-owned BrowseNames. **`Directory.Build.targets`** — the `RestrictForLegacyTfm` no-op now also applies when a project pins a single `TargetFramework` and `CustomTestTarget` names a different one. That is the same situation it already handled for the legacy frameworks, and it is what keeps the per-TFM CI matrix green once net10.0-only projects appear later in the stack. ## Validation `dotnet build UA.slnx` — 0 errors. | Suite | Result | | --- | --- | | Opc.Ua.Types.Tests | 8088 / 0 | | Opc.Ua.Server.Tests | 3873 / 1 | | Opc.Ua.Client.Tests | 2105 / 0 | | Opc.Ua.Core.Tests | 4145 / 0 | | Opc.Ua.Features.Tests | 134 / 0 | The single `Opc.Ua.Server.Tests` failure is `ConfigureApplicationBuildsSharedClientAndServerConfigurationAsync`, which is pre-existing and environmental: the PKI path in my working directory exceeds MAX_PATH. It reproduces on clean `master` and passes from a shorter directory. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8cbb8cd0-f0cb-4ab0-bea2-6202fbf69485	2026-07-31 13:13:47 +02:00
MrAlaskan	2d55126cb8	fix: return BadInvalidState for unlatched alarm reset (#4020) ... ### Summary This PR fixes the reset status code returned for unlatched alarms and adds a regression test that verifies `Reset` and `Reset2` both report `BadInvalidState` when the alarm is not latched. ### Problem According to the alarm reset semantics described in OPC UA Part 9, `Reset` is only valid for a latched alarm that is no longer active and has already been acknowledged and confirmed when those states are present. For any other invalid state, the server should return `Bad_InvalidState`. Previously, `AlarmConditionState.HandleResetCore()` returned `BadConditionNotShelved` when `LatchedState.Id == false`. That status code belongs to the shelving state machine, not the latched alarm reset flow. As a result, clients calling `AlarmClient.ResetAsync()` could receive a shelving-related error for a reset precondition failure, which misrepresents the actual alarm state and breaks the expected Part 9 semantics. ### Changes - Change the unlatched branch in `AlarmConditionState.HandleResetCore()` to return `BadInvalidState` instead of `BadConditionNotShelved`. - Add a regression test in `AlarmConditionStateMethodTests` that covers both `Reset` and `Reset2` when `LatchedState` exists but the alarm is not latched.	2026-07-18 21:45:32 +02:00
Marc Schier	84fa8e8906	CTT follow-up: monitored items, alarms, codecs, history, and model validation (#4002) ... # Description Follow-up to #3978 based on the latest OPC UA CTT aggregate, matrix, Historical Access, Attribute, and Node Management runs. ## Aggregate run 16 - Ignore synthetic `BadBoundNotFound` raw-bound markers when feeding aggregate calculators. - Add direct and live historian regression coverage. ## Multi-dimensional Variant matrices - Fix `Variant.Copy()` so rank-2 Variant matrices preserve their payload and dimensions. - Enforce valid Variant matrix dimensions consistently across Binary, JSON, XML stream, and XML parser codecs. - Encoders return `BadEncodingError`; decoders return `BadDecodingError` for malformed matrix dimensions. - Add cross-codec valid/null/empty/malformed tests and a batched ReferenceServer Read regression. - Correct the previous CTT issue attribution: the invalid `Scalar_Static_Arrays2D_Variant` server encoding poisoned the batched CTT Read. ## Node Management AddNodes - Return `BadNodeIdRejected` for unsupported requested-NodeId namespaces. - Validate hierarchical ReferenceType source/target NodeClass constraints. - Document run-18 confirmation that all remaining 15 AddNodes errors are CTT project/script configuration issues. ## Historical Access and Attribute Services run 18 - Reject whitespace in `NumericRange` syntax with `BadIndexRangeInvalid`. - Add HistoryRead tests for operation-level invalid syntax and per-DataValue `BadIndexRangeNoData`. - Document CTT defects: - `012.js` expects `BadIndexRangeNoData` at the operation level instead of on returned DataValues; - `Err-012.js` uses a non-historizing node for an access-denied test; - generic Attribute array helpers omit NodeId[] and StatusCode[] conversion paths. ## Base Information model validation run 19 - Run the CTT main session as `sysadmin` over Basic256Sha256 SignAndEncrypt so SecurityAdmin RolePermissions and EncryptionRequired restrictions are both satisfied without weakening the server model. - Preserve predefined well-known NodeIds while making generated dynamic type and optional-child factories allocate compatible per-instance NodeIds, remap internal references, and update references from the owning subtree. - Materialize dynamic RoleType properties through generated helpers and keep role-child string NodeIds disjoint from numeric RoleManager root IDs. - Add net48/net10 regressions for generator output, NodeId allocation/reference remapping, DI mandatory children, dynamic roles, and low-privilege versus SecurityAdmin Browse visibility. - Document CTT defects for permission-unaware mandatory-child validation, the UA 1.04 reference model used against a UA 1.05 server, stale ServerDiagnostics cache, and the `ConformanceUnits` scalar/array mismatch. ## Related Issues - Follow-up to #3960 and #3978. ## Checklist - [x] I have signed the CLA and read the CONTRIBUTING document. - [x] Tests prove the fixes and improve coverage. - [x] Required CTT documentation is updated. - [x] Focused builds/tests introduce no warnings. - [x] Affected tests pass on .NET Framework and .NET 10. - [ ] CI and CodeQL are green. - [ ] All PR feedback is addressed. ## Latest CTT full run and A&C run 20 - Fix Monitor Value Change V2 `036.js`: an ambiguous `DataValue(StatusCode)` call encoded `BadIndexRangeNoData` as a Good value. - Prove all 19 configured matrix monitored items notify when the selected value actually changes; document the independent `042.js` script defects. - Rebase dynamic `CertificateExpired` / `TrustListOutOfDate` alarm roots and descendants, restore standard type declarations and ModellingRules, and expose the cross-node-manager references. - Deliver method-triggered events to every authorized Session, fixing the two-Session Confirm scenario. - Preserve locale-only `LocalizedText` values consistently across Binary, JSON, and XML and align Acknowledge/Confirm comment behavior and tests. - Document the remaining proven CTT defects for TransactionDiagnostics, SemanticChange, History `013`, PolicyId scope, A&C, durable subscriptions, and Subscription Minimum events. ## Base synchronization Merged `master` through `007cfaf90` (`#4010`, repository layout consolidation) and retained the matrix regression tests at their new `tests/` locations. ## Latest validation - Focused merged-tree tests pass on net10 and net48. - `Opc.Ua.Server` and `Quickstarts.Servers` Release builds complete with 0 warnings and 0 errors on net10 and net48. - The existing `DisableEnableViaTypeAndInstanceMethodNoEventsDuringDisabledAsync` timing test took its documented inconclusive timeout path after the layout merge; no test failure was reported. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: romanett <romanett98@gmail.com>	2026-07-18 05:01:03 +02:00
Marc Schier	8538f531e7	Update paths for the consolidated repository layout	2026-07-16 06:26:05 +02:00
Marc Schier	7cec2d614d	Move repository files to the consolidated layout	2026-07-16 06:25:54 +02:00