fix(attest): make verify_all lenient under RequireAll (#359)

[andreolf]

Summary

Registry::verify_all aborted the whole batch on any per-attestation error, violating its documented RequireAll leniency. Under Policy::RequireAll, a single malformed or unknown attestation now drops out of the result instead of failing the batch. Fixes #359.

Motivation & context

Closes #359

The module docstring promises that under RequireAll the registry "never short-circuits on unknown types — any attestation can be attached by any third party, so blocking the cert because an attacker added an unrelated attacker/spam/v1 would be a denial-of-service vector." But verify_all propagated every error via ?:

for a in attestations {
    verified.push(self.verify(a, expected_cert_hash)?);
}

So a bad signature, cert-hash mismatch, or malformed payload on any attached attestation aborted the entire batch — exactly the DoS the docstring warns about.

Kind of change

• Bug fix

What changed

Crate touched: gitlawb-attest (src/verifier.rs).

• Under Policy::RequireAll, verify_all now drops entries that fail to verify (filter_map(.ok())) instead of aborting. The required_types presence check remains the gate, and it only counts fully_verified entries — so a required type present only as a malformed attestation still fails with RequiredMissing. Leniency does not open a hole.
• AcceptKnown and RejectUnknown are unchanged: they stay strict and surface the first error.
• Expanded the verify_all docstring to state the per-policy contract.

How a reviewer can verify

cargo test -p gitlawb-attest --lib verifier
cargo clippy -p gitlawb-attest --all-targets -- -D warnings

Two new tests:

• require_all_drops_a_malformed_extra_and_still_accepts — a malformed extra (signed against the wrong cert hash) alongside a valid required attestation no longer aborts; the junk is dropped and the cert verifies. Fails on the pre-fix code.
• require_all_rejects_when_only_a_malformed_required_type_is_present — when the sole copy of a required type is malformed, verify_all still returns RequiredMissing.

Before you request review

• Scope is one logical change; no unrelated churn
• cargo test --workspace passes locally (ran gitlawb-attest: 52 tests pass)
• New behavior is covered by tests
• cargo fmt --all and cargo clippy --workspace --all-targets -- -D warnings are clean
• Commit titles use Conventional Commits (fix(attest): ...)
• Docs / .env.example updated if behavior or config changed (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 — attestation verification for ref-update certs. No wire-format or signature-scheme change; only the batch error-handling policy under RequireAll. Behavior for AcceptKnown/RejectUnknown is unchanged.
• Backward-compatible with existing nodes and previously signed history (strictly more permissive under RequireAll, and only for attestations that would otherwise have hard-failed the batch).

Notes for reviewers

• One appended junk signature makes verify_all reject a threshold-satisfying certificate #349 tracks the same short-circuit pattern in gitlawb-core's cert.rs; deliberately out of scope here to keep this PR to one crate/one change. Happy to follow up on that separately.

[coderabbitai]

Warning

Review limit reached

@andreolf, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 52 minutes

Limit details: You’ve used all 1 included review currently available under your plan.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: bff1ed9e-5992-41cf-9fd9-b4201f27bdd7

📥 Commits

Reviewing files that changed from the base of the PR and between e4c7458 and 9c31d15.

📒 Files selected for processing (1)

• crates/gitlawb-attest/src/verifier.rs

---

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.}

[beardthelion]

Verified the fix does what it claims and does not weaken the gate. Reverting the policy branch to the pre-fix strict loop turns require_all_drops_a_malformed_extra_and_still_accepts red, and disabling the required_types check turns require_all_rejects_when_only_a_malformed_required_type_is_present red, so both new tests bind real behavior rather than passing vacuously. The vector is reachable as described: cert_hash() strips attestations before hashing and attach() is public, so any relaying party can append junk without invalidating a signature. Direction is right, the gate holds, and the blast radius is one caller. Two asks, both in lines this PR adds.

Unrelated to this PR: cargo audit is red on main too, RUSTSEC-2026-0258 in h2 0.4.13. Not yours, and I am not holding this on it.

Findings

• [P2] Lock the non-RequireAll paths with an error-propagation test
  crates/gitlawb-attest/src/verifier.rs:166
  The new docstring promises AcceptKnown and RejectUnknown still surface the first error, but nothing asserts it. Replacing the strict branch's ? with a lenient if let Ok(v) leaves the whole crate green; gutting its return does go red, so the branch executes and it is specifically the error propagation that is unbound. A later edit applying the lenient loop to every policy would pass every test here while silently turning default AcceptKnown batch verification into drop-on-error. This closes it, and I confirmed it reddens under exactly that mutation:

  #[test]
  fn strict_policies_surface_the_first_batch_error() {
      let sk = fresh();
      let cert_hash = sample_hash();
      let bad = signed_demo(&sk, other_hash(), "ok");
  
      let mut accept = Registry::new().with_policy(Policy::AcceptKnown);
      accept.register(DemoVerifier);
      let err = accept.verify_all(&[bad.clone()], cert_hash).expect_err("must not drop");
      assert!(matches!(err, Error::CertHashMismatch { .. }), "got {err:?}");
  
      let mut reject = Registry::new().with_policy(Policy::RejectUnknown);
      reject.register(DemoVerifier);
      let err = reject.verify_all(&[bad], cert_hash).expect_err("must not drop");
      assert!(matches!(err, Error::CertHashMismatch { .. }), "got {err:?}");
  }

• [P3] Correct the docstring: unknown types are not dropped
  crates/gitlawb-attest/src/verifier.rs:149
  The added text lists "a type with no verifier" among entries dropped from the result. Under RequireAll, verify returns Ok with fully_verified = false for an unregistered type, so filter_map(.ok()) keeps it, and require_all_is_lenient_on_unknown_types_in_the_batch already asserts the entry survives with len == 2. Only the genuine Err paths are dropped. The PR summary carries the same slip, though the Motivation section states it correctly.