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

Next included review available in 55 minutes.

View limit details

Limit details: You’ve used the included review currently available.

This review ran on the open-source allowance, not this organization's plan, because the pull request author doesn't have an assigned seat. Waiting won't change this — ask an organization admin to assign them a seat, or add seats in Billing if every seat is already assigned, then retry.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d9530198-9a75-4b61-8947-5e212fe82581

📥 Commits

Reviewing files that changed from the base of the PR and between bfc44f9 and 2b11419.

📒 Files selected for processing (1)

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

---

_{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.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P2] Lock the strict batch policies with an error-propagation test
  crates/gitlawb-attest/src/verifier.rs:166
  The root cause is that the refactor splits verify_all by policy but the new test coverage only exercises the lenient RequireAll arm. Neither AcceptKnown nor RejectUnknown is invoked through verify_all with an attestation that makes self.verify fail. Consequently, an apparently harmless future cleanup that applies the filter_map(Result::ok) pattern to all policies would make malformed, bad-signature, or wrong-cert-hash entries disappear instead of returning an error; the current added tests would still pass, silently changing the default and fail-closed contracts despite the promise at lines 159–160.

  Preserve the explicit strict loop, and add a batch-level regression test for both strict policies using a genuinely malformed entry (for example, the existing wrong-cert-hash helper). Assert that verify_all returns the underlying verification error rather than a partial result. That binds the policy boundary itself, rather than only the single-attestation verify behavior.

• [P3] Do not say unknown attestations are dropped
  crates/gitlawb-attest/src/verifier.rs:149
  The root cause is that the documentation treats “no verifier” as a verification failure, but it is an intentional successful result under RequireAll: verify returns Ok(VerifiedAttestation { fully_verified: false }), so filter_map(Result::ok) retains the entry. The existing require_all_is_lenient_on_unknown_types_in_the_batch test already proves this with a two-entry result containing the unverified other/v1 attachment. The new wording therefore tells callers that the returned vector excludes unknown valid attestations when it does not.

  Correct the contract at its source by limiting the dropped-entry list to actual Err paths—bad signatures, cert-hash mismatches, and malformed payloads—while stating that valid unknown types remain in the result as unverified. Do not change the implementation to drop unknown entries: retaining them is the established RequireAll behavior and preserves the distinction between untrusted and invalid attestations.

[jatmn]

lgtm

Superseded by the round at 2b11419, which adds the strict-arm error-propagation test this review asked for. Re-reviewing the current head.

[beardthelion]

Approving. The strict arms now have a test that fails when they are made lenient, which is what the last round asked for. I checked it by giving AcceptKnown/RejectUnknown the filter_map(Result::ok) shape and watching strict_policies_propagate_a_batch_members_error go red, and by reverting the RequireAll arm to ? propagation, which reds both require_all_* tests. Blast radius is inside the crate: nothing in the workspace depends on gitlawb-attest, and gitlawb-core::cert::verify_all is a separate method.

Two follow-ups, neither blocking.

Findings

• [P3] Assert CertHashMismatch exactly instead of the two-variant union
  crates/gitlawb-attest/src/verifier.rs:373
verify_signature compares cert_hash before it touches the signature, so a member signed against a different hash always fails as CertHashMismatch and the Error::Signature(_) arm is dead. I narrowed the matches! to CertHashMismatch alone and the suite stayed at 44 passed; narrowing it to Signature(_) alone went red. Pinning the exact variant makes the test say which contract it is holding.

• [P3] Drive the other strict error classes through verify_all too
  crates/gitlawb-attest/src/verifier.rs:171-177
  Only the cert-hash class crosses the batch path today. I mutated the strict arm to swallow just Err(Error::UnknownType(_)), which makes a RejectUnknown registry silently accept an attacker-attached unregistered attestation, and the whole suite stayed green at 44 passed; swallowing just Err(Error::Payload(_)) was also green. A RejectUnknown batch with an unregistered member asserting Err(UnknownType), plus a payload-failing member under a strict policy, would close both.