Repository navigation
Fix JSON encoding fidelity issues found by fuzz crash replay and align with Part 6 - #4540
Conversation
The BinaryJson fuzz targets reported encoding fidelity findings for 339
external crash assets. None was a crash; they reduce to three encoder
defects and one oracle that was stricter than the specification:
- A LocalizedText with an empty Locale and no Text was written as
{"Text":null} (or {} in compact form), which reads back as the null
LocalizedText. An empty Locale is not encoded, so write it as null.
- Compact encoding omitted the Value of a Variant holding a null array,
which decodes as the default scalar of the UaType and loses the array.
Only default scalars are omitted now; a null array is written as [].
- ExpandedNodeId.Format(context) returned null whenever the identifier
was null, dropping a ServerIndex or NamespaceUri of a non-null value.
It now writes i=0, like the IFormatProvider overload.
- The fuzz oracle only treated null and empty arrays as equivalent when
IgnoreNullValues is set, although Part 6 5.1.11 makes them equivalent
for all DataEncodings.
Adds hand-crafted curated reproducers for each encoder defect.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
FuzzCrashAssets printed a full stack trace and an inline base64 reproducer for every tolerated encoding fidelity finding, and those findings consumed the per-target reproducer budget. With hundreds of tolerated findings from the external crash corpus, a genuine failure later in the enumeration would print no reproducer, and the private corpus inputs were published in the pipeline log on passing runs. Tolerated findings are now reported only as the one line summary entry, with the TargetInvocationException unwrapped so the entry names the actual cause. Full traces and reproducers are emitted for failures only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- LocalizedText: Text, like Locale, is not encoded if it is null or empty (Part 6 5.4.2.15). A value with neither is the null LocalizedText (Table 1: all fields default), so it is written as JSON null in Verbose and omitted in Compact. - Variant and DataValue: the Value field is omitted only when the value is a NULL of a nullable built-in type (Part 6 5.4.2.17, Table 1), in every mode. Default scalars of non-nullable types (numbers, Boolean, StatusCode) are always written, instead of being dropped in Compact. Variant and DiagnosticInfo contents still reach the writer so they are rejected as before. - The fuzz oracle treats null and empty LocalizedText Text and Locale as equivalent (Part 6 5.1.11). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Three moderate encoding issues remain unresolved.
Review effort: Lite
Findings: None
What changed in this PR
This PR fixes JSON encoding fidelity issues, aligns fuzz validation with OPC UA Part 6, and improves replay diagnostics.
Changes:
- Corrects
LocalizedText,Variant/DataValue, andExpandedNodeIdencoding. - Updates fuzz equivalence and tolerated-finding reporting.
- Adds regression tests and curated reproducers.
- Three moderate findings remain: null matrices may throw,
DateTime/Guiddefaults may be omitted, and defaultDataValuevalues may be omitted incorrectly.
| File | Summary |
|---|---|
tests/Opc.Ua.Types.Tests/Encoders/JsonEncoderTests.cs |
Adds encoder regression coverage. |
tests/Opc.Ua.Core.Encoders.Tests/JsonEncoderTests.cs |
Updates LocalizedText expectations. |
src/Opc.Ua.Types/Encoders/JsonEncoder.cs |
Implements JSON encoding changes; contains the remaining findings. |
src/Opc.Ua.Types/BuiltIn/ExpandedNodeId.cs |
Preserves null identifiers with absolute context. |
fuzzing/Opc.Ua.Encoders.Fuzz/FuzzableCode.JsonDecoder.cs |
Updates semantic equivalence handling. |
fuzzing/Common/Fuzz.Tests/FuzzTargetTestsBase.cs |
Improves tolerated-finding and reproducer reporting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A Variant holding a null matrix cannot carry Dimensions a peer accepts, because every entry must be greater than zero. Compact encoding used to omit its Value, which decodes as the default scalar, and Verbose failed with BadEncodingError. Now that Value is only omitted for NULLs of nullable types, Compact would fail too. Write the semantically equal empty array instead (Part 6 5.1.11). Also covers the review questions with tests: DateTime MinValue and the all-zero Guid are the NULL of nullable types (Part 6 Table 1), so their Value is omitted and round-trips, and a default DataValue is still written. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Code coverage✅ Coverage gate passed.
Coverage is above the recorded baseline - consider ratcheting Thresholds live in |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4540 +/- ##
==========================================
+ Coverage 81.59% 81.62% +0.03%
==========================================
Files 2069 2073 +4
Lines 309394 309934 +540
Branches 53699 53817 +118
==========================================
+ Hits 252449 252986 +537
+ Misses 38633 38617 -16
- Partials 18312 18331 +19
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…ield The Variant Value field ignored the encoder options, so Verbose dropped the Value of a null NodeId, DateTime or Guid although every other null field is written as JSON null unless nulls or defaults are ignored. The Value field now follows the same rules as every other field: a NULL of a nullable built-in type is omitted when IgnoreNullValues or IgnoreDefaultValues is set (like WriteNull), and the default of a non-nullable type only when IgnoreDefaultValues is set (like WriteValueProperty). An array Value is still never omitted, so a null array keeps its value rank. WriteVariantValue, which writes no UaType, now also omits nulls under IgnoreNullValues alone. Tests assert the exact JSON per option set. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LazyWaitForConnectionStartsManager cancelled WaitForConnectionAsync after 250 ms and then asserted immediately that the configured listener was open. The waiter's token only cancels the wait, not the shared lazy start, so on a slow runner the start was still in flight when the assertion ran (seen on a net48 CI shard). Poll the fake transport harness with a bounded 30 s timeout instead. Nothing else starts the manager in this test, so an opened listener still proves that the wait triggered the lazy start of exactly the configured endpoint. With the listener open delayed by 1 s the old assertion fails and the new one passes. The sibling tests await the start explicitly and are not affected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SharedKeyValueHistoryContinuationStoreTests waited at most 2 to 5 seconds for signals the store's background cleanup must raise (cleanup blocked, cleanup completed, resolution blocked, disposal completed). On a loaded net48 CI runner two tests timed out with TimeoutException although the signals are raised eventually. Every one of these waits expects the signal, so a longer bound only limits how long a real hang takes to fail and does not slow a passing run. Use a single 30 s bound for all of them, including the polling loop in WaitForCleanupAttemptsAsync. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…acked 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>
Description
The external crash corpus replayed by
Opc.Ua.Encoders.Fuzz.Testsstill produced several hundred tolerated encoding fidelity findings per run. They came from the 10 BinaryJson conversion targets (binary decode → JSON encode → JSON decode). None of them was a crash or an unexpected exception. Triage reduced them to three JSON encoder defects plus one oracle rule that was stricter than the specification. While checking the fixes against OPC 10000-6, two adjacent deviations turned up and are fixed as well.Encoder fixes (
Opc.Ua.Types){"Text":null}(Verbose) /{}(Compact). Reads back as null and re-encodes differentlynull(Verbose) / omitted (Compact)"Text":""writtenLocalizedText("")is the null LocalizedTextValueomitted → decoded as a scalar default (array rank lost)"Value":[]Valueomission ruleIgnoreDefaultValues;IgnoreNullValuesignoredIgnoreNullValuesorIgnoreDefaultValuesis set (likeWriteNull); a default of a non-nullable type only whenIgnoreDefaultValuesis set (likeWriteValueProperty). An arrayValueis never omittedExpandedNodeId.Format(context)with a ServerIndex/NamespaceUri and a null identifiernull, so JSON wrotenulland the server index was lostsvr=N;i=0/nsu=...;i=0, consistent with theIFormatProvideroverloadi=0)Fuzz oracle and harness (
fuzzing/)IgnoreNullValueswas set.FuzzCrashAssetsno longer prints a stack trace or an inline base64 reproducer for tolerated fidelity findings. Only the one-line summary entry is reported, and it now names the actual cause instead ofTargetInvocationException. Before, tolerated findings used up the 60-per-target reproducer budget, so a genuine failure later in the enumeration printed no reproducer. The externally supplied corpus inputs were also written into the pipeline log on passing runs.Assets/Repo/, which is always enforced strictly. Each one fails on the previous master and passes with this change.Reviewer notes: observable JSON output changes
LocalizedText("")/("en","")now round-trip withText == nullinstead of""(equivalent per 5.1.11).IgnoreNullValuesalone now also omits a NULLValue, as it does for every other null field."Value":[]in every mode instead of being dropped in Compact, and a null matrix is written as"Value":[]instead of failing withBadEncodingError.WriteBadVariantThrows).Testing (net10.0, locally)
"Text":"")Opc.Ua.Encoders.Fuzz.Testsbuilds with 0 warnings; net48 tests were not run locallyRelated Issues
Checklist
🤖 Generated with Claude Code