fix(security): receive-path correctness (#22, #23, #24)

[moisesja]

Summary

Resolves three Low-severity audit findings on the unpack / JOSE receive surface (#22, #23, #24). Each fix ships with tests, and the mandatory adversarial red-team pass (AGENTS.md §2) found one further gap that is fixed here.

Full suite: 615 tests green; dotnet build clean under warnings-as-errors.

⚠️ Scoping note (the issue text predates the JOSE delegation now on main)

• Wrong-length JWE iv/tag throws an undocumented ArgumentException that escapes the unpack exception contract #22 cited Jose/Encryption/JweParser.cs + Crypto/Aead/*, which were deleted when the JOSE layer was delegated to DataProofsDotnet.Jose. Investigation (and an empirical test) shows the reported raw-ArgumentException escape is already resolved upstream — the dependency's parser now wraps the AEAD's wrong-length-iv/tag ArgumentException (and base64 FormatException) as MalformedJoseException. This PR adds a defensive boundary catch in EnvelopeReader (ArgumentException → MalformedMessageException at both JWS/JWE delegation points) plus a regression test, so the unpack contract is robust regardless of the delegate. Cross-repo: the root defect lives in the dependency and is already fixed there.

Fixes

#	Change
#22	Defensive ArgumentException → MalformedMessageException boundary catch at the JWS+JWE delegation points in EnvelopeReader; regression test (wrong-length iv → MalformedMessageException).
#24	Base64Url.Decode rejects = padding, whitespace, and +// (anything outside [A-Za-z0-9-_]) with FormatException, per RFC 7515 §2 / RFC 4648 §3.2-3.3.
#23	UnpackResult.AnonymousSender derived from the outermost encrypt layer (matching its doc) instead of OR-accumulating across layers. The contradiction the finding described was already blocked by the merged #17 gate.

Adversarial red-team pass (AGENTS.md §2)

Subagents hit the session limit, so I ran the pass inline against the real code:

• Wrong-length JWE iv/tag throws an undocumented ArgumentException that escapes the unpack exception contract #22 holds — the external parser comprehensively wraps base64/AEAD/AES-KW exceptions; the boundary catch is sound defense-in-depth.
• UnpackResult AnonymousSender / Authenticated flags are accumulated across layers and contradict their documented "outermost layer" contract #23 holds — outermost-first derivation is correct for every legal shape; the gate runs before the UnpackResult return; the independent-flags semantics carry no dangerous false-positive.
• Base64url decoder accepts '=' padding and silently strips embedded ASCII whitespace, diverging from strict JOSE base64url-no-pad #24 — REAL FINDING, fixed in this PR. ForwardProcessor.ExtractAttachmentBytes had no try/catch, so a malformed forward attachment threw a raw FormatException out of the mediator's ProcessAsync — the issue's premise ("call sites map FormatException") was false there. Now mapped to MalformedMessageException with a regression test.

Test plan

• Wrong-length JWE iv/tag throws an undocumented ArgumentException that escapes the unpack exception contract #22: tamper a valid envelope with a 5-byte iv → MalformedMessageException (not a raw exception).
• Base64url decoder accepts '=' padding and silently strips embedded ASCII whitespace, diverging from strict JOSE base64url-no-pad #24: padding / whitespace / +// → FormatException; clean no-pad input still round-trips; an end-to-end padded from_prior segment → ProtocolException; a malformed forward attachment → MalformedMessageException.
• UnpackResult AnonymousSender / Authenticated flags are accumulated across layers and contradict their documented "outermost layer" contract #23: anoncrypt(authcrypt) yields AnonymousSender=true (outermost anon) and Authenticated=true/SenderKid (inner authcrypt).

Closes #22, closes #23, closes #24

🤖 Generated with Claude Code

[moisesja]

Review — solid, honest, well-tested. No blockers; one deliberate-decision item I'd like your read on before it lands.

This is careful work. The scoping note is refreshingly honest: #22 and #23 were already largely closed (the upstream MalformedJoseException wrap and the merged #17 gate), and you say so rather than overclaiming. The red-team pass earning its keep on the unguarded ForwardProcessor.ExtractAttachmentBytes is exactly what AGENTS.md §2 is for. Every change fails in the safe (reject) direction — no new attack surface. I verified the load-bearing claims myself; specifics below in priority order.

1. #24 strictness is global, and it tightens the mediator's tolerance of peer forward attachments

I empirically ran System.Buffers.Text.Base64Url.DecodeFromChars on net10.0 to confirm the "before" behavior:

input	before (BCL)	after (#24)
Mw==, SGVsbG8= (padding)	ACCEPT	reject
space / tab / CRLF	ACCEPT (silently stripped)	reject
a+b/ (+//)	reject	reject

So padded/whitespaced base64url was previously accepted at every Base64Url.Decode call site. On the from_prior JWT path (FromPriorValidator.cs:69-71) this tightening is a clear win — non-canonical encoding over signed bytes is a real parser-differential concern. But the same codec backs the mediator's forward relay (ForwardProcessor.cs:119): a forward whose data.base64 carries = padding is now rejected (MalformedMessageException) and no longer relayed, where before it relayed fine. On the relay path the mediator just passes bytes onward and the real recipient re-parses, so the security upside of strictness there is low while the interop cost (refusing a peer's forward) is concrete.

Question: are you certain padded data.base64 is out-of-spec for DIDComm v2.1 attachments? The JOSE no-pad rule is unambiguous for JWS/JWE segments, but attachment data.base64 is looser historically (Aries RFC 0017). If you're not confident every peer emits no-pad, consider keeping the forward-attachment decode lenient while keeping the JWT/JOSE paths strict.

Mitigating fact I checked: the strict change touches only four sites — local signer d, OOB URL, from_prior, and forward attachment. Externally-resolved DID-doc key material decodes inside DataProofsDotnet.Jose, not this codec, so resolved peers' keys are unaffected. Blast radius is contained.

2. The #22 and #23 tests guard the contract but don't exercise the new code

• Wrong_length_iv_throws_MalformedMessageException passes via the upstream MalformedJoseException catch (as your own test comment notes) — delete the new catch (ArgumentException) in EnvelopeReader and it stays green. The boundary catch is genuinely untested. A unit test with a stub JWS/JWE parser that throws ArgumentException would actually cover it.
• AnonymousSender_reflects_the_outermost_encrypt_layer passes on main too: for the only legal multi-encrypt shape (anoncrypt(authcrypt)) the old OR-accumulation already yielded anonymous=true. The behavioral difference only appears on illegal shapes the #17 gate rejects, so this can't regression-guard the change. Fine for defense-in-depth — just don't mistake it for a regression test.

3. Minor: the ArgumentException catch is broad and its message is over-specific

The try wraps the whole JweParser.Parse / JwsParser.Parse call, which invokes consumer-supplied lookups — so a buggy IInternalSecretsLookup that throws ArgumentException (or an internal ArgumentNullException) gets relabeled "Malformed JWE field length (iv/tag)", masking a non-input error behind a misleading cause. Fail-closed + preserved InnerException makes it acceptable, but a neutral message ("Malformed JWE.") would age better.

---

None of this blocks merge — #24's core is correct and the from_prior coverage is the right call. I mainly want your decision on (1), since it changes what a mediator will relay. (I did not re-run the full 615-test suite locally; trusting CI for that, but I did verify the BCL behavior and the call-site blast radius directly.)

[moisesja]

Thanks — all three addressed in c4ef29d. Full suite 617 green.

1. Forward-attachment strictness → relaxed (your decision item)

You're right, and I checked the spec to be sure: no-pad is mandated only for the JOSE/JWT/OOB paths — FR-ENC-13/14 (apv/apu), FR-SIG-04 (signed payload), FR-OOB-02 ("base64url(whitespace-stripped …)"). Attachment data.base64 is governed only by FR-ATT-02 ("at least one of jws/hash/links/base64/json"), which imposes no no-pad rule — consistent with Aries RFC 0017's looseness. And as you note, the mediator just relays bytes the recipient re-parses strictly.

So: kept strict Base64Url.Decode for from_prior / OOB / JWK, and added Base64Url.DecodeRelaxed (tolerates = padding + whitespace) used only by ForwardProcessor. A peer's padded forward is relayed again; a genuinely-invalid attachment (+///non-alphabet) still maps to MalformedMessageException (no raw FormatException escape). New test ProcessAsync_relays_a_padded_base64_attachment_FR_ATT_02 locks the interop behavior; the malformed-attachment test now uses a truly-invalid value. Blast radius is exactly the four sites you identified; only the relay one changed.

2. Tests now exercise the new code

• Wrong-length JWE iv/tag throws an undocumented ArgumentException that escapes the unpack exception contract #22 boundary catch — added ArgumentException_from_the_delegated_parse_maps_to_MalformedMessageException: a signer lookup that throws ArgumentException (invoked at JwsParser.cs:183, outside the parser's own try) propagates raw to the new catch → MalformedMessageException with InnerException preserved. Delete the catch and it goes red — so it genuinely covers the new code, unlike the wrong-length-iv test (which I've kept as a contract regression guard).
• UnpackResult AnonymousSender / Authenticated flags are accumulated across layers and contradict their documented "outermost layer" contract #23 — agreed, it can't regression-guard (the EnvelopeReader accepts illegal envelope compositions (arbitrary layer ordering up to depth 4) #17 gate rejects the only differing shapes). Relabeled it as a characterization / defense-in-depth test in the comment so it isn't mistaken for one.

3. Broad catch / message

Messages are now neutral — "Malformed JWE." / "Malformed JWS." — and the comment notes the cause may be a field length or a throwing consumer lookup, with the original preserved as InnerException. The catch stays broad-but-fail-closed by design (the Parse call is a single opaque delegate boundary).

Captured the strictness-scoping takeaway as lesson L-027. Ready for re-review.