fix(webvh): did-witness.json spec wire format — proof member + versionId signed document (#135)

[moisesja]

Fixes #135

What was broken

The DIF didwebvh-test-suite replay (#134) showed no witnessed did:webvh DID interoperated in either direction. Assessment confirmed the issue's finding and uncovered a second divergence the issue did not name — both are required for interop:

1. Member name (the issue's finding): did-witness.json was written and parsed with proofs; did:webvh v1.0 and all six reference implementations use proof (WitnessValidator.cs:196/:272 pre-fix).
2. Signed data model (new finding): NetDid verified witness proofs against the log entry serialized without its proof; the spec defines them as eddsa-jcs-2022 proofs over the {"versionId": "..."} input document. Verified three independent ways: a from-scratch Python verifier against the ts/rust/java/dart suite vectors (versionId-doc ⇒ all verify; entry-without-proof ⇒ all fail), a second independent verifier during adversarial review (all 6 implementations), and the ts source (witness.ts signs { versionId }). Fixing only the key name left 10/10 witnessed replays rejected.

Soundness: the versionId embeds the entry hash that chain validation independently recomputes over full entry content before witness validation runs, and the signed document is always built from the chain-validated entry's versionId — so a witness approval still binds the whole entry, and cross-DID/cross-version replay fails (pinned by suite negative vectors).

Evidence

• DIF replay: witnessed happy-path acceptance 0/10 → 10/10 (witness-threshold + witness-update × ts/rust/java/java-eecc/dart). The two witness negatives are now rejected by real threshold logic instead of vacuously at parse time. Regenerated report committed (tasks/webvh-vector-divergence.md); remaining 15 rejections are the pre-existing python controller divergence (out of scope).
• 8 suite artifacts committed as known-answer fixtures + 22 Issue135_* tests. Fail-first proven twice: 6/9 round-1 tests red against main; 10/22 round-2 tests red against the round-1 commit.
• Full gate: 0-warning Release build, 1,731 tests green (W3C conformance 233/233), all samples exit 0.

Adversarial review (3 independent worktree-isolated agents)

Confirmed findings, all fixed in this PR:

• HIGH: the ts implementation authors one witness-file entry per proof, so a version's approvals legitimately arrive split across duplicate-versionId entries; FirstOrDefault consulted only the first (falsely rejecting threshold≥2 ts deployments) and MergeWitnessProofs kept only the last duplicate. Both now aggregate; pinned by a suite-genesis known-answer test.
• Proof-less entries (spec: "SHOULD be removed", i.e. a legal transient) no longer reject the whole file; created is optional end-to-end (VC-DI); proofPurpose must be assertionMethod to count (matching reference resolvers); null Created is omitted on serialize.
• Parse diagnostics are bounded by construction (type name + numeric position, or fixed library text) — ex.Message is never echoed, since System.Text.Json embeds hostile member names (incl. U+2028 line separators) in it.
• Update/Deactivate throw ArgumentException on unparseable CurrentWitnessContent instead of silently publishing a witness artifact stripped of the caller's existing proofs; the legacy proofs member is rejected as an explicit migration tripwire. Non-string versionId no longer reaches a dictionary crash through public Update/Deactivate.

Refuted (design held): replay/substitution, vote-accounting inflation, parse-path exception escapes (28-case hostile corpus, zero escapes), TOCTOU, duplicate-member strictness harming conformant emitters (0 duplicates in all suite artifacts).

Breaking (wire compatibility)

A did-witness.json published by an earlier NetDid release is invalid under the corrected format — its proofs were signed over the wrong document, so no key rename can rescue them; affected DIDs must re-collect witness proofs. If an external witness toolchain minted spec-correct proofs that NetDid merely stored under proofs, renaming that member to proof in the published file restores validity. Tolerant reading of proofs was deliberately not kept: it could never validate NetDid-minted legacy proofs and would only re-enable silent proof-shedding during migration. Details in CHANGELOG.

🤖 Generated with Claude Code

[moisesja]

Do not merge this as written. The title, description, and commit messages are clear, the implementation scope is coherent, and the 22 issue-specific tests are useful. That does not compensate for the following correctness and security failures in the witness trust boundary.

1. HIGH — witness proof options are dropped before verification and on republish

   WitnessValidator.cs:404-413 parses only six fields, WebVhProofVerifier.cs:51-65 reconstructs a reduced proof, and WitnessValidator.cs:237-247 serializes only that reduced shape. W3C Data Integrity proof configuration includes additional proof options such as id, expires, domain, challenge, nonce, previousProof, and extensions. Those fields are signature-bound. A conforming proof signed with expires is therefore rejected after NetDid drops expires; Update/Deactivate then republishes the proof without that field but with the old proofValue, permanently corrupting it. Worse, an attacker can append an unsigned extra option to an otherwise valid proof and NetDid verifies the truncated original configuration. Preserve the complete proof configuration and verify/re-emit it through the Data Integrity implementation. Add tests for a genuinely signed additional option and for unsigned option injection. The current comment claiming dropped fields fail closed is false.

2. HIGH — malformed CurrentWitnessContent is still silently ignored

   The new parse-and-throw behavior is nested under WitnessProofs is { Count: > 0 } at DidWebVhMethod.cs:633-654 and :750-771. Pass garbage or legacy proofs content with WitnessProofs = null or []: Update/Deactivate succeeds, never parses the supplied file, and returns no witness artifact. That is the same silent ignored-input/proof-shedding class that the PR body, XML docs, and CHANGELOG claim is eliminated. Consume and validate CurrentWitnessContent whenever it is supplied, independent of whether a new proof batch is present, and add null/empty-batch tests for both public operations.

3. HIGH — the revised merge is TOCTOU-vulnerable despite the PR claiming TOCTOU was refuted

   MergeWitnessProofs accepts a caller-controlled IReadOnlyList and enumerates it once at WitnessValidator.cs:291 to delete versions, then again at :297 to add entries. A switching implementation can yield version A on the first pass and version B on the second; existing A is deleted with no replacement. Create/Update/Deactivate also wait before consuming WitnessProofs, so ordinary concurrent mutation has the same class of problem. Snapshot the outer list and every nested Proofs list exactly once at the public operation boundary, before any await, and use only that copy. Add a hostile changing-enumerator regression test across all three public surfaces.

4. HIGH — same-version incremental collection still deletes prior approvals

   WitnessValidator.cs:290-298 removes the entire existing aggregate for every version named by the new batch. Existing v2 proof A plus newly received v2 proof B produces only B. This makes threshold collection unable to accumulate incrementally and can turn a threshold-satisfying file into an unresolvable one. The new split-entry test only adds a different version, so it misses the collision; the old test explicitly pins replacement. did:webvh v1.0 describes publishing as proofs are added, while the public docs say merge/include. Append and deduplicate same-version proofs, or document an explicit complete-replacement contract everywhere and justify why it is compatible with the spec workflow.

5. HIGH availability — duplicate aggregation has no cryptographic-work bound

   ValidateAllWitnesses at WitnessValidator.cs:74-81 nests governed entries over later versions; :127-149 verifies every proof; and :207-211 rescans every witness entry for each version. There is no proof budget, memoization, threshold early-exit, or cancellation check. A valid 5 MiB log plus a legal <=1 MiB witness response containing thousands of duplicate invalid proofs can force repeated Ed25519 verification for every earlier governed entry after the HTTP timeout is already over. The byte cap is not a CPU cap. Index once, memoize verification, stop once the threshold is met, and enforce a configurable verification-attempt budget with adversarial tests.

6. MEDIUM — bare witness verification methods are incorrectly counted

   WebVhProofVerifier.cs:83-123 explicitly accepts did:key:<multibase> without a fragment. did:webvh v1.0 requires witness proof verification methods in the exact did:key:<multibase>#<multibase> form with equal body and fragment. An authorized witness can sign with the bare form; NetDid counts it while conforming resolvers discard it. Add a witness-specific strict parser and a signed negative test; do not globally tighten the shared helper because configured witness IDs are bare DIDs.

CI is also currently red. The failure is the unrelated timing-sensitive did:ethr test Issue116_TerminalHop_SlowEnumerationPastDeadline_InternalError_NotSuccess; I reran that exact test five times locally and all five passed, so it looks flaky rather than caused by this diff. It still needs a green CI rerun before merge.

Requested changes: fix and test the six items above, rerun the full gate, and rerun the adversarial review against the revised surface.

[moisesja]

Thanks — all six findings are fixed on this branch (commits 6ee920a fixes, 3c71599 docs, 99015f3 promoted re-attack tests). Each fix is proven fail-first (a throwaway RedProbeOldApi adapted to the pre-rework API is 5/5 RED against d5047d7; F1b confirms the unsigned-injection bypass was real), and I re-ran the full gate plus an adversarial re-attack of the revised surface.

1 (HIGH) — dropped proof options. Witness proofs are now verified by the same DataIntegrityProofPipeline the controller path uses, over the proof's complete wire configuration: RawJson is captured at parse and re-emitted verbatim on republish. Signature-bound id/expires/nonce/extensions are honored during verification and preserved on republish, and an unsigned injected member now fails verification instead of being truncated away. WebVhProofVerifier.VerifyAndExtractSigner (the reduced reconstruction) is deleted. The false "dropped fields fail closed" comment is gone. Tests: Issue135_R2_ProofWithSignedExpires_VerifiesAndCounts, _UnsignedInjectedProofMember_FailsVerification, _ProofWithIdAndExtensionMembers_Verifies, _Republish_PreservesSignedExtraMembers.

2 (HIGH) — silently-ignored CurrentWitnessContent. Parsing is hoisted out of the WitnessProofs.Count > 0 block into ParseRequiredWitnessContent, called whenever content is supplied. Garbage or legacy proofs content with a null/empty batch now throws ArgumentException; valid content republishes. Tests: _Update_GarbageWitnessContent_NoNewProofs_Throws, _Deactivate_..._Throws, _Update_ValidWitnessContent_NoNewProofs_Republishes.

3 (HIGH) — TOCTOU. SnapshotWitnessProofs copies the outer list and every nested Proofs list exactly once at each public boundary before any await; downstream reads only the private copy. Tests use a hostile IReadOnlyList that switches contents per enumeration and assert Enumerations == 1 across Create and Update.

4 (HIGH) — same-version deletion. MergeWitnessProofs now appends and dedupes byte-identical proofs instead of replacing the aggregate. The old replacement-pinning test is updated to assert append. Tests: _Merge_SameVersionProof_AppendsToExisting, _Merge_ByteIdenticalProof_DedupesAcrossSides, _Update_SameVersionProof_AppendsInArtifact.

5 (HIGH availability) — unbounded work. One VerificationSession per run: the file is indexed once, per-proof verdicts are memoized across governed entries, unconfigured/duplicate/wrong-purpose signers are skipped before any cryptography, per-entry scanning stops at the threshold, cancellation is honored, and a configurable per-resolution verification budget fails closed when exhausted (DefaultMaxWitnessProofVerifications = 1024, new DidWebVhMethod ctor overload). Tests: _VerificationBudgetExhausted_FailsClosed, _ThresholdMet_StopsVerifying, _CumulativeCoverage_MemoizesAcrossGovernedEntries, _UnconfiguredSigners_ConsumeNoBudget, _Validation_HonorsCancellation.

6 (MED) — bare verificationMethod. New WebVhWitnessKeyResolver + ExtractWitnessDidKeyMultibase require the strict did:key:<mb>#<mb> form for witness proofs only; the shared helper stays permissive because configured witness ids are bare DIDs. Test: _BareDidKeyVerificationMethod_DoesNotCount.

Gate: 0-warning Release build; 1,777 tests green (W3C conformance 233/233); DIF vector replay byte-identical — witnessed 10/10 accepted, 21/21 negatives rejected. The flaky Issue116_TerminalHop_SlowEnumerationPastDeadline ethr test passes locally here too — agreed it's unrelated to this diff and needs a green CI rerun.

Re-review: an independent adversarial re-attack of the revised surface found no High or Medium defects; all six fixes hold. It confirmed the subtle one — the pipeline strips the proof member before hashing, so the secured document reduces to {"versionId":X} and isolating one proof per verification is correct — and that each proof instance is filed under exactly one chain-validated versionId, so the reference-keyed memo can never return a foreign-document verdict (now pinned by _ProofSignedForForeignVersion_DoesNotCount). Three residuals are LOW/by-design and fail-closed: configured-signer garbage consumes bounded budget, write-side witness-file growth, and byte[0] treated as unparseable (null is the absent sentinel).

[moisesja]

Second review: the original six findings appear fixed, but this is still not ready to merge. The new implementation introduced or exposed four concrete defects, two of which I reproduced with focused tests.

1. BLOCKER — valid Data Integrity proof chains are rejected (WitnessValidator.cs:216-220, 309-320).

   VerifyWitnessProofAsync serializes only the single proof being counted. That destroys the proof set needed to resolve previousProof. I generated a genuine two-proof chain with the pinned DataProofs pipeline: verifying {"versionId": ..., "proof": [A, B]} returned Verified=true with both proof results true; the PR's validator rejected the exact artifact because it isolated B and left its previousProof dangling. This is not malformed input—the VC Data Integrity model explicitly supports proof chains.

   Verify the complete per-version proof set (or at least the dependency closure) and count successful configured signers from the per-proof results. Add a regression where the configured witness proof depends on another valid proof.

2. BLOCKER — the advertised work bound only limits signature calls; attacker-controlled preprocessing remains quadratic (WitnessValidator.cs:148-170, 276-295, 327-333).

   For N governed entries with a usable proof only at the latest version, the nested i / j scans perform N(N+1)/2 version lookups. Separately, P syntactically valid but unconfigured proofs are each compared against W configured witnesses via FirstOrDefault, giving O(P×W) work that does not consume the verification budget. Session construction and long proof buckets also do not observe cancellation until much later. Calling this “bounded” is false; only cryptographic invocations are bounded.

   Build indexed witness membership once, avoid rescanning the suffix for every governed entry (compute cumulative coverage once), and check cancellation while indexing and walking proof buckets. Add adversarial scale tests that assert operation counts, not elapsed time.

3. BUG — same-proof deduplication fails in the normal public API path (WitnessValidator.cs:402, 415-416, 547; DataIntegrityProofValue.RawJson has an internal setter).

   A proof parsed from CurrentWitnessContent has non-null RawJson. The identical caller-supplied WitnessProofs object cannot set RawJson, so including RawJson in the dedupe key makes them unequal. A focused Update merge produced two identical emitted proofs. The current regression test masks this by parsing both sides, which public callers cannot do.

   Deduplicate by normalized full proof content, not by a provenance/cache field, and test parsed existing content against a programmatically supplied proof.

4. BUG — the “per-resolution” witness budget resets inside one resolution (DidWebVhMethod.cs:374, 425; WitnessValidator.cs:82).

   Historical resolution can validate the requested prefix and then validate the deactivated tail. Each call creates a fresh VerificationSession, so one resolution can perform approximately 2× the configured maximum and reverify the same proofs. Share one session/budget for the entire resolution, or stop documenting this as a per-resolution cap.

Also, the new witness budget is only configurable through direct construction. NetDidBuilder.AddDidWebVh still exposes only maxControllerProofsPerEntry and instantiates the three-argument constructor (NetDidBuilder.cs:54-88), so normal DI consumers cannot set the policy this PR claims to expose.

Validation performed on 17eefa5:

• Existing focused review/interoperability tests: 41/41 passed.
• Full WebVh project suite: 486/486 passed.
• PR CI is green.
• Two throwaway regression probes above: 0/2 passed; both failed on the exact asserted behavior. The probe file was removed and the worktree is clean.

Green CI does not compensate for tests that encode the wrong boundary. Please fix these and add regressions before merging.

[moisesja]

One additional consequence of finding 1 needs to be fixed at the public API boundary, not only in validation.

DidWebVhCreateOptions.WitnessProofs / Update / Deactivate accept DataIntegrityProofValue, but that public type exposes only the six minimum members. Its only carrier for signed options such as id, expires, domain, nonce, and previousProof is RawJson, whose setter is internal (DataIntegrityProofValue.cs:48-59). WriteProofObject consequently emits only the reduced six-member shape for every programmatically supplied proof (WitnessValidator.cs:369-385).

So a caller holding a conforming externally produced proof cannot pass it to Create at all without stripping signed configuration and invalidating the signature. Update/Deactivate work only when the proof happened to arrive via parsed CurrentWitnessContent; Create has no equivalent raw witness-file input.

Expose a safe way to submit the complete proof configuration (prefer a parsed/validated JSON representation over a bag of lossy modeled fields), preserve it through the snapshot/merge path, and exercise a public Create call with a genuine chained or extended proof. Otherwise the PR still does not provide the spec wire-format interop its title claims.