fix(envelope): fail closed when an authenticated decrypt surfaces no skid (#52)

CHANGELOG.md

@@ -6,6 +6,19 @@ All notable changes to didcomm-dotnet are documented here. Format follows
 
 ## [Unreleased]
 
+### Added — fail-closed skid guard on authenticated decrypt (defense in depth, #52)
+
+Closes **#52**. The authcrypt unpack branch trusted the JOSE layer's definitional invariant
+`IsAuthenticated ⟺ non-empty skid` without asserting it; had that invariant ever regressed
+upstream, a null skid would have silently skipped the FR-CONSIST-01 `from`↔`skid` binding on an
+envelope still reported authenticated (and an empty skid would have escaped as a raw
+`ArgumentException`, off the FR-API-07 contract). `EnvelopeReader` now asserts a surfaced skid
+whenever a decrypt is authenticated, rejecting with `ConsistencyException` — parity with the JWS
+branch's empty-signer-kid guard. The check lives in `AddressingConsistency` as
+`CheckAuthcryptSkidSurfaced` so it is directly unit-tested; it is unreachable through today's
+parser, so no current input changes behavior. The JWS branch's `CryptoException` guard is
+deliberately untouched.
+
 ### Changed — lazy UTF-8 size on the inbound snapshot (perf, #53)
 
 Closes **#53**. Every successful `UnpackAsync` registers an `InboundMessageSnapshot`, but only the

src/DidComm.Core/Composition/EnvelopeReader.cs

@@ -318,6 +318,14 @@ public static async Task<UnpackResult> UnpackAsync(
 
                     if (jweResult.IsAuthenticated)
                     {
+                        // Fail closed: an authenticated decrypt MUST surface the sender skid,
+                        // else the FR-CONSIST-01 from↔skid check in the Plaintext branch would
+                        // silently no-op while the message is still reported authenticated —
+                        // letting 'from' assert an identity the envelope never bound. Guaranteed
+                        // by DataProofs' IsAuthenticated ⟺ non-empty-skid contract today; assert
+                        // it so the identity binding never depends on the delegated parser
+                        // upholding it (issue #52; parity with the JWS signer-kid guard above).
+                        AddressingConsistency.CheckAuthcryptSkidSurfaced(jweResult.SenderKid);
                         authenticated = true;
                         senderKid = jweResult.SenderKid;
                         shape.Add(LayerShape.AuthEncrypt);

src/DidComm.Core/Consistency/AddressingConsistency.cs

@@ -39,6 +39,23 @@ public static void CheckAuthcryptFromMatchesSkid(string? from, string skid)
                 $"Authcrypt 'skid' DID subject does not match plaintext 'from' (FR-CONSIST-01). from='{from}', skid='{skid}'.");
     }
 
+    /// <summary>
+    /// FR-CONSIST-01 precondition — an authenticated (authcrypt) decrypt MUST surface the sender
+    /// key identifier (<c>skid</c>). The JOSE layer defines <c>IsAuthenticated</c> as "skid
+    /// present", so a missing skid on an authenticated result means that upstream invariant
+    /// regressed; fail closed so <see cref="CheckAuthcryptFromMatchesSkid"/> cannot silently
+    /// no-op while the envelope is still reported authenticated. Parity with the JWS branch's
+    /// empty-signer-kid guard. (Issue #52.)
+    /// </summary>
+    /// <param name="skid">The sender key identifier surfaced by the authenticated decrypt.</param>
+    /// <exception cref="ConsistencyException">When <paramref name="skid"/> is null or empty.</exception>
+    public static void CheckAuthcryptSkidSurfaced(string? skid)
+    {
+        if (string.IsNullOrEmpty(skid))
+            throw new ConsistencyException(
+                "Authenticated (authcrypt) envelope did not surface a sender 'skid'; cannot bind the message 'from' to the sender (FR-CONSIST-01).");
+    }
+
     /// <summary>
     /// FR-CONSIST-02 — recipient <c>to</c> ↔ <c>kid</c>. When plaintext <c>to</c> is present,
     /// the DID subject of the recipient key actually used to decrypt MUST be a member of

tasks/todo20260720-173121.md

@@ -0,0 +1,56 @@
+# Issue #52 — Fail-closed skid guard on authenticated decrypt (authcrypt parity with JWS branch)
+
+Plan approval waived by user ("skip the approval for the plan for now. Implement").
+
+## Context
+
+`EnvelopeReader`'s authcrypt branch trusts DataProofs' definitional invariant
+`IsAuthenticated ⟺ non-empty skid` (`JweParser.cs:439/572`) without asserting it. If that
+invariant ever regressed (IsAuthenticated=true, null/empty SenderKid):
+
+- **null skid** → the FR-CONSIST-01 `from`↔`skid` check at `EnvelopeReader.cs:127` is silently
+  skipped; `message.From` is attacker-set on an envelope reported authenticated.
+- **empty skid** → `CheckAuthcryptFromMatchesSkid` throws a raw `ArgumentException`, escaping the
+  FR-API-07 unpack exception contract (fails closed, but wrong type).
+
+The JWS branch already has the equivalent guard (`EnvelopeReader.cs:232`, commit 6f393e9). The
+guard is unreachable through today's parser — pure defense in depth, zero behavior change for any
+current input.
+
+## Decisions (pre-made to avoid review churn)
+
+- **Exception type: `ConsistencyException`** — per the issue as written. The JWS branch's
+  `CryptoException` asymmetry is deliberate and left untouched (no scope creep).
+- **Testability**: the guard lives in `AddressingConsistency` as a named helper so it is directly
+  unit-testable (the EnvelopeReader call site is unreachable via the real parser — no seam, by
+  design). Same pattern as every other FR-CONSIST rule.
+- **Scope**: no changes to the JWS branch, line-127 semantics, DataProofs, or the PRD
+  (FR-CONSIST-01 already exists; this is an enforcement precondition, not a new rule).
+
+## Steps
+
+- [x] Branch `feat/authcrypt-skid-guard-52` off main
+- [x] Add `AddressingConsistency.CheckAuthcryptSkidSurfaced(string? skid)` — throws
+      `ConsistencyException` on null/empty
+- [x] Wire it in `EnvelopeReader` authcrypt branch (`if (jweResult.IsAuthenticated)`) before
+      setting `authenticated`/`senderKid`
+- [x] Unit tests in `AddressingConsistencyTests`: null → throws, empty → throws, valid → passes
+- [x] Full `dotnet test` — green, no existing test affected
+- [x] CHANGELOG entry under [Unreleased]
+- [x] Commit, push, PR referencing #52
+
+## Review
+
+- Guard implemented exactly as scoped: one helper (`AddressingConsistency.CheckAuthcryptSkidSurfaced`,
+  ~5 lines), one call site (`EnvelopeReader` authcrypt branch, before any trust state is set),
+  three unit tests (null / empty / valid).
+- Full suite green: 598 core + 159 interop = 757 passed, 0 failed, 0 skipped. No existing test
+  touched — consistent with the guard being unreachable through today's parser (DataProofs defines
+  `IsAuthenticated` as `!IsNullOrEmpty(senderKid)`, JweParser.cs:439/572).
+- Pre-made decisions held: `ConsistencyException` per the issue text; JWS branch untouched;
+  no PRD/DataProofs changes. `docs/codebase-architecture.md` referenced by CLAUDE.md does not
+  exist in the repo, so nothing to sync there (only `didcomm-dotnet_PRD.md` is present).
+- Verification note for reviewers: the EnvelopeReader call site cannot be exercised black-box by
+  construction (no parser seam — deliberate); the helper is unit-tested directly, same pattern as
+  every other FR-CONSIST rule. Precedent: the JWS signer-kid guard (6f393e9) shipped with no
+  message-level test at all.

tests/DidComm.Core.Tests/Consistency/AddressingConsistencyTests.cs

@@ -34,6 +34,23 @@ public void Authcrypt_with_null_from_short_circuits()
         AddressingConsistency.CheckAuthcryptFromMatchesSkid(from: null, "did:example:alice#k");
     }
 
+    [Theory]
+    [InlineData(null)]
+    [InlineData("")]
+    public void Authenticated_decrypt_without_surfaced_skid_fails_closed(string? skid)
+    {
+        // Issue #52 — if the JOSE layer's IsAuthenticated ⟺ non-empty-skid invariant ever
+        // regressed, the guard must reject rather than let the from↔skid binding no-op.
+        Action act = () => AddressingConsistency.CheckAuthcryptSkidSurfaced(skid);
+        act.Should().Throw<ConsistencyException>().WithMessage("*FR-CONSIST-01*");
+    }
+
+    [Fact]
+    public void Authenticated_decrypt_with_surfaced_skid_passes()
+    {
+        AddressingConsistency.CheckAuthcryptSkidSurfaced("did:example:alice#key-x25519-1");
+    }
+
     [Fact]
     public void Recipient_kid_membership_succeeds_when_subject_matches_any_to_entry()
     {