# 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>
# Description
Fixes every `src/Opc.Ua.Core` item found by a twelve-reviewer audit of
`src/Opc.Ua.Server` + `src/Opc.Ua.Core` taken at 0e600c0f0, across all
three tiers of that report (security/data-loss/crash, confirmed
functional, and reviewer-traced). **The `src/Opc.Ua.Server` half of the
audit is deliberately out of scope here** and remains open.
## Security and resource safety
- **Explicitly configured trusted certificates were ignored.**
`CertificateTrustList.TrustedCertificates` (the `<TrustedCertificates>`
config element, `SecurityConfiguration.AddTrustedPeer`) never reached
the validator, so a peer trusted only that way was rejected with
`BadCertificateUntrusted`. The list now travels from configuration
through `TrustListEntry` into `TrustListState`, and is consulted both
when deciding trust and when completing a chain — OPC 10000-4 6.1.3:
"The list of trusted Certificates may contain a Certificate issued to
another Application or it may be a Certificate belonging to a CA". The
store is still consulted first, so a CA configured both ways keeps its
CRL check. Cached validation cores are evicted when a trust list is
re-registered, so a reconfiguration actually takes effect.
- **Unauthenticated receive-buffer leak.** `SaveIntermediateChunk` now
always takes ownership of the chunk: it either stores it or returns it
to the `BufferManager`. A server without a `None` endpoint gives every
TCP client a discovery-only channel, and each non-discovery request
leaked one `ReceiveBufferSize` buffer. Added `TakeSavedChunks` for the
"body already saved" case; passing the chunk again would have queued the
same buffer twice. A channel torn down while a multi-chunk message is
still arriving now releases what it had buffered, which nothing did
before.
- **Decrypted OpenSecureChannel buffers** are returned when verification
fails, including the sibling paths where the sequence-number or
certificate check rejects the message *after* the body was handed back.
- **RSA Diffie-Hellman peer values** are validated against `2 <= y <=
p-2` (RFC 7919 5.1), so the shared secret can no longer be forced to a
known constant by sending 0, 1 or p-1.
- **`RemovePadding` verified no padding at all** for channel messages:
the loop started at `data.Offset` and stopped at `paddingSize`, so on
any message with a header it ran zero times. The filler loop is
corrected — OPC 10000-6 6.7.2.5.1: "The value of each byte of the
padding is equal to PaddingSize" — and the signature is now checked
*before* the padding is inspected so the two failures cannot be told
apart.
- **Key material zeroing** on the ECC secret path: the plain text, the
derived key and IV, and the ECDH shared secret, on both sides. The
decrypt wipe covers the failure path too, which is the one that matters
— the body is decrypted in place before the signature is verified.
- **HTTPS responses** are read with `ResponseHeadersRead` and bounded by
`MaxMessageSize`, instead of the whole body being allocated before the
limit was applied.
- **`X509CertificateStore.IsRevokedAsync`** reports an unanswerable
revocation check as `BadCertificateRevocationUnknown` instead of
throwing. The throw surfaced as an unsuppressible
`BadCertificateInvalid` and failed every CA-issued certificate on Linux
and macOS. See reviewer note 2.
- **Certificate handle ownership**: `ClientChannelManager` AddRefs what
it hands to a transport that disposes it (and releases it again when the
channel never opens), `CertificateTrustList` releases the handle `Add`
took its own reference to, and `X509CertificateStore` disposes the
platform handles nothing else owns.
## Transport
- The receive loop is now **one atomically swapped epoch of (transport,
token)**, so a reconnect that binds a new socket to an existing channel
supersedes the old loop rather than racing it. Previously the supersede
path was unguarded and could start two readers on one socket, dispose an
in-use `CancellationTokenSource`, and leak one per reconnect.
- A **reconnect hand-over detaches the transport before closing
anything**, so the socket the adopted channel now answers on is not
closed underneath it. Previously the queued `OpenSecureChannelResponse`
failed with `BadConnectionClosed` and reconnect to a dropped channel
could never succeed.
- A blocked or unservable **accepted socket is closed** rather than left
open until process exit.
- An **invalid response sequence number reconnects the channel** instead
of throwing into a handler that did nothing, which left the pending
request hanging until timeout. OPC 10000-6 6.7.7 requires the receiver
to close the channel, and the same section requires the error to be
logged — which `HandleMessageProcessingError` now does instead of
discarding it silently.
- The **abort chunk** is encoded against the room the chunk has rather
than what it currently holds, so a short offending chunk can still carry
the error body (OPC 10000-6 6.7.3).
## Discovery and endpoints
- `GetEndpoints` no longer drops the HTTPS and WSS JSON/OpenAPI
endpoints, and no longer collapses them onto the binary one (the dedup
ignored `TransportProfileUri`). `profileUris` filtering matches them too
— OPC 10000-4 5.5.4.2 defines it as the "List of Transport Profile that
the returned Endpoints shall support", and a client asking for the JSON
profile previously got an empty list back. Results are ordered
deterministically with **binary first**, so a client that takes the
first endpoint that fits keeps getting the one it got before these twins
were published.
## Smaller confirmed defects
`FilterEvaluator.InList` returned after the first string operand;
`SelectedUserTokenPolicy` setter always ended at `-1`;
`Utils.UpdateExtension` could not delete; a lease holder never stepped
down while the shared store was unreachable *or hanging*;
`IsUriHttpRelatedScheme` never matched `http`;
`SessionLessServiceMessage` dropped a single `LocaleId`;
`GetRandomDouble` used `ToSingle` and typed variant arrays ignored the
requested `builtInType`; source-generated log calls never reached the
trace file.
## Related Issues
- No tracking issue was opened for this work; it derives from an audit
report rather than a filed issue. Happy to open one and link it if
maintainers prefer.
## Notes for reviewers
1. **`MaxRejectedCertificates` semantics.**
`ICertificateStore.AddRejectedAsync` documented "0 is unlimited", but
`DirectoryCertificateStore` wrote every certificate and then deleted all
of them at 0 (net effect: store nothing), and `CertificateManager`
clamped a negative value onto 0 and *depended* on that to implement
"clear the store". `SharedKeyValueCertificateStore` meanwhile treated 0
as genuinely unlimited, so the two store types did opposite things at
the same setting. This PR keeps the existing observable behaviour (0 or
less keeps no history and clears what is stored), removes the
write-then-delete-all churn, aligns the KV store, and corrects the
interface doc and `docs/DependencyInjection.md`. **The KV store is the
one place whose behaviour changes** relative to master.
2. **Revocation status where X509Store has no CRLs.** `IsRevokedAsync`
used to throw there, which surfaced as an unsuppressible
`BadCertificateInvalid` and failed every CA-issued certificate on Linux
and macOS - the outage the audit flagged. It now returns
`BadCertificateRevocationUnknown`, which is what the Windows branch of
the same method and a directory store without a CRL already return, and
the status `RejectUnknownRevocationStatus` decides on (OPC 10000-4
6.1.3, Find Revocation List). With the default policy the certificate is
accepted; an operator who opted into strict mode gets a rejection, as
before on master. An earlier revision returned `BadNotSupported`, which
the validator discards outright and which made revocation fail open even
in strict mode; review caught it.
3. **`ManagedChannelKey` gained a `TransportProfileUri`** property and
an optional trailing constructor parameter. Source-compatible, but it is
public API.
## 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.
- [ ] 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.
### Regression test coverage
Each of these fails against `master`'s source and passes with the fix.
| Fix | Test |
|---|---|
| Padding filler bytes were never verified |
`CryptoUtilsSymmetricPaddingTests` — forged filler rejected, plus a
round trip over every padding length |
| Padding removed before the signature was checked |
`CryptoUtilsSymmetricPaddingTests.SymmetricDecryptReportsSignatureFailureAheadOfPadding`
|
| RSA-DH peer value not range checked | `RsaDiffieHellmanPeerValueTests`
— 0, 1, p-1, p, above p, both range boundaries, group mismatch |
| A degenerate DH nonce threw instead of being rejected |
`UaSCBinaryChannelNonceTests` |
| `<TrustedCertificates>` never reached the validator |
`CertificateValidationCoreTests` (peer, issuer, and the negative
control),
`CertificateManagerTests.ExplicitlyTrustedPeerFromConfigurationIsAcceptedAsync`
|
| Cached validation cores survived a trust-list change |
`CertificateManagerTests` — registration after a failed validation, and
`UpdateAsync` onto a different store |
| `MaxRejectedCertificates` normalisation |
`CertificateManagerTests.MaxRejectedCertificatesNormalisesNegativeValues`
|
| `AddRejectedAsync` retention contract |
`RejectedCertificateRetentionTests` — both store types, at zero,
negative and a positive cap |
| PEM rewrite left the old tail behind |
`DirectoryCertificateStoreTests.DeleteFromAMultiCertificatePemFileTruncatesTheFileAsync`
|
| `IsRevokedAsync` threw where CRLs are unsupported |
`CertificateStoreTest.IsRevokedAsyncReportsNotSupportedWithoutCrlSupportAsync`
(runs on Linux/macOS, ignored on Windows) |
| ECC secret left in the caller's buffer |
`EncryptedSecretTests.EccDecryptWipesThePlainTextFromTheCallersBufferOnFailure`
|
| Trust-list read leaked a handle per entry |
`CertificateTrustListHandleTests` |
| A channel that never opened leaked its certificate |
`ClientChannelManagerHandleTests` |
| Chunk ownership, CLO double free, chunk-limit teardown |
`TcpServerChannelBufferTests` (4 new) |
| Receive loop did not follow a replaced transport |
`TcpServerChannelDeterministicTests` (3 new) |
| HTTPS body unbounded, failures unmapped |
`HttpsTransportChannelResponseTests` (5) |
| JSON/OpenAPI endpoints dropped, collapsed, or unordered |
`ServerBaseTransportProfileTests` (12) |
| Channel key ignored the transport profile |
`ClientChannelManagerManagedTests` (2 new) |
| `SelectedUserTokenPolicy` setter always ended at -1 |
`ConfiguredEndpointTests` (4 new) |
| `UpdateExtension` could not delete | `UtilsAdditionalTests` (3 new) |
| A single `LocaleId` was dropped | `SessionLessServiceMessageTests` (2
new) |
| `GetRandomDouble` / typed variant arrays | `DataGeneratorTests` (4
new) |
| `TraceLoggerProvider.IsEnabled` short-circuited every log call |
`TraceLoggerProviderTests` (3 new) |
| A leader never stepped down on an expired lease |
`SharedStoreLeaseElectionTests` (3 new) |
| `FilterEvaluator.InList`, `IsUriHttpRelatedScheme` | existing tests
corrected in the first commit |
**Not covered.** The reconnect hand-over ordering, the OpenSecureChannel
decrypted-buffer returns, the client-side bad-sequence reconnect, the
abort-chunk sizing and the listener closing a blocked socket all need a
two-socket reconnect or a full asymmetric OpenSecureChannel exchange,
which the deterministic channel harness does not model. They are
exercised by the integration suites but not asserted on directly.
## Spec conformance
Where a fix has a normative basis I checked it against the published
specification rather than only against the audit report:
| Change | Reference |
|---|---|
| Every padding filler byte is verified | OPC 10000-6 6.7.2.5.1 — "The
value of each byte of the padding is equal to PaddingSize". The loop
started at `data.Offset` and stopped at `paddingSize`, so on any message
with a header it ran zero times and no filler byte was ever checked. |
| Signature verified before the padding is inspected | OPC 10000-6
6.7.2.5.1 — the signature "includes the headers, all Message data, the
PaddingSize and the Padding"; 6.7.7 — the chunk "shall not be
interpreted until the Message is decrypted and the signature and
sequence number verified". |
| Message-processing errors are logged | OPC 10000-6 6.7.7 — "the
receiver shall log a Bad_SequenceNumberInvalid error", and stacks "shall
have a mechanism to log errors when invalid Messages are discarded". The
handler was an empty method. |
| A bad response sequence number reconnects the channel | OPC 10000-6
6.7.7 — "the receiver shall close the communication channel". It
previously threw into that empty handler and the pending request hung
until timeout. |
| The trust list's own certificates are honoured as peers **and** as
issuers | OPC 10000-4 6.1.3 — "The list of trusted Certificates may
contain a Certificate issued to another Application or it may be a
Certificate belonging to a CA". |
| The abort chunk is sized by the chunk's capacity | OPC 10000-6 6.7.3 —
the abort body is `Error` plus a `Reason` string, so a chunk shorter
than the error body could not carry it. |
| JSON/OpenAPI endpoints are published and match `profileUris` | OPC
10000-4 5.5.4.2 — `profileUris` is the "List of Transport Profile that
the returned Endpoints shall support"; a client asking for the JSON
profile previously got an empty list. |
| RSA-DH peer value restricted to `2 <= y <= p-2` | RFC 7919 5.1, which
the finite-field policies are built on. I found no OPC-specific wording
for this. |
One point where the spec argues against the current shape is called out
in reviewer note 2 below.
### Test runs
On **net10.0**, with `Opc.Ua.Core.Tests` and
`Opc.Ua.Security.Certificates.Tests` also run on **net48**. The
remaining suites were not run on the .NET Framework targets locally, so
that checklist box stays unchecked.
| Suite | Result |
|---|---|
| Opc.Ua.Core.Tests (net10.0) | 4630 passed, 0 failed, 88 skipped |
| Opc.Ua.Core.Tests (net48) | 4545 passed, 0 failed, 104 skipped |
| Opc.Ua.Core.Security.Tests | 556 passed, 0 failed |
| Opc.Ua.Security.Certificates.Tests (net10.0) | 355 passed, 0 failed |
| Opc.Ua.Security.Certificates.Tests (net48) | 353 passed, 0 failed |
| Opc.Ua.Configuration.Tests | 227 passed, 0 failed |
| Opc.Ua.Core.Encoders.Tests | 2929 passed, 0 failed |
| Opc.Ua.Bindings.Https.WebApi.Tests | 346 passed, 0 failed |
| Opc.Ua.Redundancy.Server.Tests | 783 passed, 0 failed |
| Opc.Ua.Client.Tests | 2217 passed, 0 failed |
| Opc.Ua.Server.Tests | 5552 passed, 0 failed |
| Opc.Ua.Sessions.Tests | 778 passed, 0 failed, 379 skipped |
| Opc.Ua.Gds.Tests | 1117 passed, 0 failed |
`Opc.Ua.Sessions.Tests` reports 379 skipped; that is identical to a
`master` baseline run in a separate worktree (security policies the
platform does not advertise), not a regression from this change.
`dotnet format whitespace --verify-no-changes` reports no violations in
any file this PR touches.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
romanett