2 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	d98eb6b28c	Fix server session defects found by a spec-checked review of CreateSession, ActivateSession, session lifecycle and continuation points (#4632) ... # Description Fixes defects found by a spec-checked review of the server session implementation: CreateSession, ActivateSession, the session lifecycle (per-request validation, CloseSession, Cancel, timeout, shutdown) and session continuation points. Each finding was checked against OPC 10000-4 (§5.7, §7.9, §7.32), OPC 10000-5 (session diagnostics and audit events) and OPC 10000-6 §6.8.2. There is one commit per finding, and each commit message cites the spec section. A max-effort code review of the fixes then found 15 more issues, mostly regressions the first round introduced; they are fixed in the `revfix` merges. ## What changed **CreateSession** - At MaxSessionCount the oldest session that was never activated is closed to make room (§5.7.2.1). Sessions that are activated, already closing, or have an activation in flight are never evicted. The victim is claimed atomically, so a late activation commit on it is rejected. - `requestedSessionTimeout` NaN is revised. A malformed `endpointUrl` no longer fails the call with Bad_UnexpectedError. `serverUri` is ignored, as §5.7.2.2 requires. - Unnamed sessions get distinct names. - Session diagnostics now report `MaxResponseMessageSize` and `ServerUri`, and `SessionSecurityDiagnostics.Encoding` uses the specified names ("UA Binary", "XML", "JSON"). - AuditCreateSessionEvent carries SecureChannelId and the request certificate, also on failure. - Every rejected CreateSession is counted, and only once. A rejection during server startup no longer turns into Bad_UnexpectedError. - If an `OnApplicationCertificateError` override accepts an error, the certificate is now kept and the remaining checks still run. A certificate accepted this way does not grant TrustedApplication or application-based roles. **ActivateSession** - EphemeralKey handling follows OPC 10000-6 §6.8.2: - A key used to decrypt a UserName or IssuedToken secret is retired and replaced in the same commit. - A key that was not used is kept, and nothing is returned. - ECDHPolicyUri is honoured; an unsupported policy gets Bad_SecurityPolicyRejected. - Request parameters are no longer echoed back. - After the activation commits, a failure or cancellation no longer faults the response, which would hide the new server nonce. Post-commit callbacks run uncancellably. - An activation that overlaps a session close or timeout fails with Bad_SessionClosed instead of returning Good. - The PolicyId of an AnonymousIdentityToken is validated. - Status codes now follow §5.7.3.3: - Bad_ApplicationSignatureInvalid for a mismatched client-signature algorithm - Bad_UserSignatureInvalid for a mismatched X509 user-token signature algorithm - Bad_IdentityTokenInvalid for an undecodable token or an unknown policy - `ClientUserIdOfSession` keeps the user of the first activation. **Lifecycle and requests** - Requests on an expired or closing session are rejected with Bad_SessionClosed (§5.7.2.1). A CloseSession on an expired session that is not yet closing is still processed as the client's close and honours `deleteSubscriptions`. - Closing a session aborts its outstanding requests with Bad_SessionClosed. A request that registers after that abort pass is rejected too. - Cancel completes the matching requests with Bad_RequestCancelledByClient (§5.7.5.1). It also reaches requests still waiting in the request queue, but only when both timestamps prove the request was sent earlier. - There is one AuditCancelEvent per Cancel call, carrying the client's audit entry id and user. - Every close path goes through a single close claim, so SessionAbortCount, SessionTimeoutCount and the "Session/Terminated" and "Session/Timeout" audits are each counted once. - Teardown that started cannot be cancelled. Requests rejected by session validation are counted. **Continuation points** - HistoryRead returns Bad_NoContinuationPoints instead of evicting points of the same request (§7.9). Only the request that saved a point pins it, so points from prior requests can still be freed. - Points saved by a HistoryRead that faults are released. - Session-less Browse and BrowseNext no longer dereference a null session. ## Behaviour changes reviewers should know about - At the session cap, CreateSession evicts a non-activated session instead of returning Bad_TooManySessions. The eviction is counted in SessionAbortCount. - Clients see Bad_SessionClosed for requests on expired or closing sessions, and Bad_RequestCancelledByClient for cancelled requests. - `ActivateSessionProcessAdditionalParameters` is now called on every successful activation, with or without an AdditionalHeader. - If an `OnApplicationCertificateError` override returns instead of throwing, CreateSession now accepts the certificate. - Timeout and termination audits are now raised after the session teardown. - The only public API changes are additive overloads in `AuditEvents` and `RequestManager`. **Known partial fixes:** - The session's `maxResponseMessageSize` is reported but not enforced; only the channel limit applies. - With overlapping ActivateSession calls on one session, an earlier response can still carry a key that a later activation has already replaced. Closing that fully needs a new `ISession` member. ## Related Issues - The Redundancy.Server session-mirroring findings from the same review are tracked separately in #4627. ## Tests - Regression tests cover each finding. The new files are: - `CreateSessionSpecRegressionTests` - `ActivateSessionValidationRegressionTests` - `StandardServerActivateSessionRegressionTests` - `SessionLifecycleRegressionTests` - `SessionRequestAdmissionRegressionTests` - `HistoryReadContinuationDispatchTests` Existing test classes were extended too. - Local results on net10.0: - `Opc.Ua.Server.Tests`: 7883 passed / 0 failed / 5 skipped - `Opc.Ua.Sessions.Tests`: 790 passed / 0 failed / 379 skipped (same as master) - net48 Release build of `Opc.Ua.Server.Tests`: 0 errors / 0 warnings. ## 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. - [ ] 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.5 <noreply@anthropic.com>	2026-10-03 05:42:01 +02:00