fix(node): fail closed when a recipient DID can't be resolved (#47)

[beardthelion]

Summary

encrypted_pin::encrypt_and_pin sealed a withheld blob to whatever subset of its recipient DIDs resolved locally and recorded the full intended set as covered, permanently locking out any reader whose DID could not be resolved at seal time. This makes sealing fail closed: resolve all recipients or skip the blob.

Motivation & context

Closes #47

did_to_key only resolves did:key; did:web/did:gitlawb (the canonical identity actors migrate to once DHT anchoring is live) and any malformed did:key return None. The old filter_map dropped those silently and sealed to the rest as long as one key resolved, then recorded the full DID set. Because the dedup compares the recorded set against the desired set, it matched on the next push and never re-sealed, so the dropped reader stayed locked out forever with no signal.

Kind of change

• Bug fix
• Feature
• Security fix
• Docs
• Tests / CI
• Refactor (no behavior change)
• Breaking or protocol change (issue required first)

What changed

gitlawb-node (crates/gitlawb-node/src/encrypted_pin.rs):

• Add resolve_all_recipients: resolves every recipient DID all-or-nothing, returning Err(unresolved) listing the DIDs that failed. encrypt_and_pin now skips the blob on any failure instead of sealing to a partial set. Nothing partial is recorded, so the dedup stays honest and the blob re-seals on a later push once resolution is available (no automatic backfill).
• Bounded per-blob warn (the unresolved DIDs are user-controlled rule data; log a count plus a small sample, not an unbounded dump) with wording that stays neutral about the cause (a malformed did:key is not DHT-pending).
• One aggregate coverage warn per call, so a fully-migrated did:gitlawb org dropping to zero encrypted-pin coverage is a single greppable line rather than a per-oid scrape.
• Give the previously silent read_object and pin_git_object failure arms warn logs.

Fail-closed is deliberate: a blob with an unresolvable reader gets no encrypted recovery envelope until resolution exists, but it is never leaked in plaintext (it was already pinned as withheld upstream). This trades recovery coverage during a transition for a clean audit invariant: a recorded encrypted_blobs row always means every authorized reader can recover the blob.

How a reviewer can verify

cargo test -p gitlawb-node encrypted_pin
cargo test --workspace
cargo clippy --workspace --all-targets -- -D warnings

The core regression is mixed_set_with_did_gitlawb_fails_closed: a resolvable did:key subset plus one did:gitlawb must return Err naming only the unresolvable DID, never a sealable key set. malformed_did_key_fails_closed covers the corrupt-did:key case.

Before you request review

• Scope is one logical change; no unrelated churn
• cargo test --workspace passes locally
• New behavior is covered by tests (required for fixes)
• cargo fmt --all and cargo clippy --workspace --all-targets -- -D warnings are clean
• Commit titles use Conventional Commits (feat(...), fix(...), docs(...))
• Docs / .env.example updated if behavior or config changed (or N/A)
• Checked existing PRs so this isn't a duplicate

Protocol & signing impact

• Touches DID / did:key, Ed25519 / RFC 9421 signatures, UCAN, ref certs, or P2P wire formats
• Discussed in an issue before implementation
• Backward-compatible with existing nodes and previously signed history

This reads DID strings to resolve recipient keys but changes no signature, wire-format, or identity logic; existing envelopes and recorded recipient sets are untouched.

Notes for reviewers

The peer-sync path (sync.rs records an empty recipient set for replicated envelopes via an overwriting upsert) interacts with the dedup but self-heals on the next push; it is pre-existing and out of scope here. A typed resolution-failure reason (distinguishing "method not resolvable yet" from "malformed did:key") is a possible follow-up; fail-closed behavior is identical for both today.

Summary by CodeRabbit

• Improvements

  • Strengthened recipient validation to ensure all recipients are properly resolved before processing, improving reliability and consistency.
  • Enhanced error reporting with clearer distinction between validation failures and empty recipient scenarios.

• Tests

  • Added comprehensive test coverage for recipient validation edge cases and error conditions.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: cb88660c-b178-4564-bdc9-9fba88f36ebc

📥 Commits

Reviewing files that changed from the base of the PR and between e37ea7f and eafdd87.

📒 Files selected for processing (1)

• crates/gitlawb-node/src/encrypted_pin.rs

---

📝 Walkthrough

Walkthrough

encrypt_and_pin is changed from fail-open (partial sealing via filter_map) to fail-closed recipient resolution. A new resolve_all_recipients helper returns all keys or the set of unresolved DIDs. The main loop now skips and counts blobs when any DID fails resolution. Pin result handling gains distinct warn branches, and unit tests cover all resolution paths.

Changes

Fail-closed recipient resolution in encrypt_and_pin

Layer / File(s)	Summary
resolve_all_recipients helper and fail-closed sealing logic crates/gitlawb-node/src/encrypted_pin.rs	Adds resolve_all_recipients returning Err(unresolved_set) if any DID fails. Introduces skipped_unresolvable counter. Replaces filter_map partial-seal with fail-closed logic that skips and logs on any unresolved DID. Adds distinct warn! branches for empty-CID and pin-error outcomes from pin_git_object.
Aggregate warning and unit tests crates/gitlawb-node/src/encrypted_pin.rs	Emits an aggregate warning after the loop when skipped_unresolvable > 0. Extends #[cfg(test)] with cases for all-resolvable, mixed, did:web, malformed did:key, empty DID set, and single-unresolvable-DID inputs to resolve_all_recipients.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 Hoppin' through the DID fields one by one,
No partial seals shall sneak past me and run!
If any key stays dark, the whole blob waits,
Fail-closed, we guard the cryptographic gates.
With tests for every path and warns galore,
This rabbit locks it tight forevermore. 🔐

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: fail-closed behavior for unresolvable recipient DIDs in encrypted blob sealing.
Description check	✅ Passed	The description comprehensively covers all template sections including summary, motivation, kind of change, what changed, verification steps, pre-review checklist, and protocol impact.
Linked Issues check	✅ Passed	All coding requirements from issue #47 are met: new resolve_all_recipients function implements all-or-nothing resolution, fail-closed behavior skips partial seals, bounded per-blob warnings log unresolved DIDs, and warn logs added to silent failure paths.
Out of Scope Changes check	✅ Passed	All changes in encrypted_pin.rs are directly scoped to the issue requirements: DID resolution refactoring, warning log improvements, and comprehensive test coverage for the new behavior.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/encrypted-pin-partial-recipients

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}