[beardthelion]

Summary

Closes the three gitlawb-core verification gaps from the Oct 2 2026 audit (A20): a zero maintainer threshold no longer satisfies a certificate, did:key resolution accepts only the canonical base58btc spelling, and the replay contract is documented on the certificate type that carries seq/nonce.

Continues #512 (the commits are carried over unchanged; main was merged in as a separate commit, resolving an overlap where main added small-order key rejection to the same to_verifying_key function and test module).

Motivation & context

Closes #509

• RefUpdateCert::satisfies_threshold compared a count against threshold directly, so 0 always passed and turned the maintainer check into a no-op for any caller that derived the threshold from configuration.
• seq and nonce are signed but unused: core is stateless, and no verifier exists to reject a reused certificate. The field docs claimed "Prevents replay", which was false.
• Did::to_verifying_key used multibase::decode's result without checking the alphabet, so one Ed25519 key had many did:key spellings while gates, allowlists, and database rows compare DIDs as strings. gitlawb-attest already enforces base58btc for attestation signers; core now matches.

Kind of change

• Bug fix
• Security fix

What changed

gitlawb-core only.

• cert.rs: satisfies_threshold rejects threshold == 0 with Error::RefCert("threshold must be at least 1"), before any signature work, so zero can never be read as "no signatures required" and an invalid certificate cannot mask the invalid threshold.
• cert.rs: module docs gained a "Replay" section. It records that core is stateless, that seq and nonce are inside the signed body, and that the verifier which persists certificates must reject a reused nonce and a non-advancing seq per (repo, ref_name). The seq/nonce field docs no longer claim enforcement that does not exist.
• did.rs: to_verifying_key requires the base58btc (z) prefix and Base::Base58Btc from multibase::decode. This is the single decode point behind HTTP-signature auth, UCAN issuers, ref-cert signers, peer announces, gl, and git-remote-gitlawb, so one key now has exactly one DID spelling at every verification site.
• Merge resolution: main's small-order Ed25519 rejection and this PR's canonical-multibase check both live in to_verifying_key; the merged function and doc comments carry both properties, and both test blocks are kept.

How a reviewer can verify

cargo test -p gitlawb-core --lib
cargo clippy -p gitlawb-core -p gitlawb-attest --all-targets -- -D warnings
cargo fmt --all -- --check

New tests:

• threshold_zero_is_rejected and threshold_zero_is_rejected_before_signature_verification: fail on the pre-fix code (Ok(true) and SignatureInvalid respectively).
• replay_fields_are_covered_by_the_signature: mutating seq or nonce invalidates the certificate.
• to_verifying_key_rejects_non_base58btc_multibase: base64url/base32 spellings of a real key were accepted before and are refused now; the canonical spelling still resolves. Base58flickr, an empty method id, and a wrong multicodec are covered separately.
• verify_all_rejects_a_non_canonical_did_spelling: a valid signature over a base64url-spelled signer is refused by the prefix rule (that spelling fits under the length guard, so the test fails if the DID fix is reverted), while the canonical spelling still satisfies the threshold.

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 (N/A, code docs updated)
• 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

Notes for reviewers

I ran the gitlawb-core, gitlawb-attest, and gitlawb-node suites, not the whole workspace.

Summary by CodeRabbit

• Bug Fixes
  • Certificate validation now rejects zero signature thresholds before checking signatures.
  • did:key resolution accepts only canonical base58btc encodings and rejects alternate spellings.
  • Changing a certificate’s nonce or sequence number invalidates its signature.
• Documentation
  • Clarified that sequence numbers and nonces are signed replay inputs. Replay protection requires a stateful verifier to reject reused nonces and sequence numbers that do not advance for the same repository and reference.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

The core crate rejects zero certificate thresholds and non-canonical did:key encodings. Its documentation states that a stateful verifier must enforce replay protection for sequence numbers and nonces.

Changes: Core validation

Layer / File(s)	Summary
Canonical did:key resolution crates/gitlawb-core/src/did.rs	DID resolution accepts only base58btc encodings with the z prefix. Tests cover alternate multibase encodings, empty method IDs, and an unsupported multicodec.
Certificate threshold and replay inputs crates/gitlawb-core/src/cert.rs	A zero threshold returns an error before signature verification. The documentation assigns replay enforcement to the stateful verifier. Tests check threshold validation order, signature coverage of seq and nonce, and rejection of a non-canonical signer DID.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Possibly related PRs

• Twigpine/node#236: Its certificate verification path calls Did::to_verifying_key, whose accepted DID encodings change in this PR.

Suggested labels: crate:core, kind:bug

Suggested reviewers: the-fool-00, kevincodex1

Merge Risk: Minimal · up to 52b6d

The change rejects zero certificate thresholds and non-canonical did:key spellings, and documents that replay enforcement belongs to the stateful verifier. No actionable merge-blocking risk is evident from the supplied context.

Pre-merge checks: 5

Check name	Status	Explanation
Title check		The title names the three changes in the PR: rejecting a zero threshold, canonicalizing did:key, and documenting replay. The raw summary confirms all three in gitlawb-core (cert.rs and did.rs). The ti…
Description check		The description follows the template's main sections: Summary, Motivation & context (with Closes #509), Kind of change, What changed, How a reviewer can verify, Before you request review, Protocol & s…
Linked Issues check		Issue #509 is the only direct linked issue. It is open and has three coding requirements in gitlawb-core. No parent or contextual issues were supplied. All three are met at the current head 52b6dc9. …
Out of Scope Changes check		The whole-PR diff from merge base 0e5a160 to head 52b6dc9 changes only crates/gitlawb-core/src/cert.rs and crates/gitlawb-core/src/did.rs. Each change maps to #509. - cert.rs: threshold-zero re…
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…

Full details: Title check

Explanation

The title names the three changes in the PR: rejecting a zero threshold, canonicalizing did:key, and documenting replay. The raw summary confirms all three in gitlawb-core (cert.rs and did.rs). The title is specific and in Conventional Commits form. Its only weak point is the length, 78 characters, which is at the upper end but still readable.

Full details: Description check

Explanation

The description follows the template's main sections: Summary, Motivation & context (with Closes #509), Kind of change, What changed, How a reviewer can verify, Before you request review, Protocol & signing impact, and Notes for reviewers. It explains the three fixes, lists the new tests with commands, and marks the protocol checkboxes. Two gaps remain. The template's own fields are not all filled in. The Before-review checklist says the full workspace tests pass, but the Notes section says only three crate suites ran. That conflicts with the checkbox and should be corrected or explained. Also, the description says the change is backward-compatible, but it rejects previously accepted non-canonical DID spellings, so that claim needs support.

Resolution

Resolve the conflict between the checked box 'cargo test --workspace passes locally' and the note that only the gitlawb-core, gitlawb-attest, and gitlawb-node suites ran. Either run the full workspace suite and keep the box checked, or uncheck the box. Also explain why rejecting non-canonical did:key spellings is backward-compatible with existing nodes and previously signed history.

Full details: Linked Issues check

Explanation

Issue #509 is the only direct linked issue. It is open and has three coding requirements in gitlawb-core. No parent or contextual issues were supplied. All three are met at the current head 52b6dc9.

1. Threshold 0 [#509]: RefUpdateCert::satisfies_threshold in cert.rs returns Error::RefCert("threshold must be at least 1") when threshold == 0. The check runs before verify_all(). Tests threshold_zero_is_rejected and threshold_zero_is_rejected_before_signature_verification cover it. The issue also asks for rejection at construction. RefUpdateCert::new takes no threshold, and the diff does not add one, so no construction-time check applies.

2. Replay [#509]: the issue allows either enforcement or documented ownership. The PR documents ownership. The cert.rs module docs state that core is stateless, that seq and nonce are in the signed bytes, and that the persisting verifier must reject a reused nonce and a non-advancing seq per (repo, ref_name). The seq and nonce field docs match this text. Test replay_fields_are_covered_by_the_signature checks that changing either field invalidates the signature. The signed payload is unchanged, so the versioning requirement in the issue does not apply.

3. Canonical did:key multibase [#509]: Did::to_verifying_key in did.rs requires a leading z and Base::Base58Btc from multibase::decode. Other spellings return Error::InvalidDid. Tests to_verifying_key_rejects_non_base58btc_multibase, to_verifying_key_rejects_a_base58flickr_spelling, to_verifying_key_rejects_an_empty_method_id, to_verifying_key_rejects_base58btc_with_a_wrong_multicodec, and verify_all_rejects_a_non_canonical_did_spelling cover it. The canonical spelling still resolves.

The incremental diff from the previous reviewed head d46a25a to head was unavailable, because changed blobs are missing from the checkout. The whole-PR diff from merge base 0e5a160 to head was readable. It covers all commits, including 52b6dc9, so the conclusions above rest on that diff. The effect of the CodeQL-related commit is test assertion messages only. The head source shows no error echo in the new cert.rs assertion messages.

Full details: Out of Scope Changes check

Explanation

The whole-PR diff from merge base 0e5a160 to head 52b6dc9 changes only crates/gitlawb-core/src/cert.rs and crates/gitlawb-core/src/did.rs. Each change maps to #509.

• cert.rs: threshold-zero rejection in satisfies_threshold, replay documentation in the module docs and on seq and nonce, and tests for threshold order, replay-field signature coverage, and non-canonical signer rejection.
• did.rs: base58btc prefix and decode check in to_verifying_key, its doc comments, and tests for alternate multibase spellings, wrong multicodec, and empty method id.

The existing length guard and the small-order check are unchanged. No public signatures, payload layout, or unrelated files change. The CodeQL-related commit changes test assertion messages. Those messages support the tests for #509 and do not change production behavior. Some did.rs test assertion messages still include {err}. This is test-only and does not affect scope or the linked-issue requirements. The effect of the DID change on stored non-canonical DID strings in other crates was not read. It is a deployment concern, not an out-of-scope change.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 2 files.

Finishing Touches

Generate docstrings

• Commit to this branch
• Create a new PR

Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 81518509-4239-49f2-950a-cca922206a06

Commits

Reviewing files that changed from the base of the PR and between d46a25a and 52b6dc9.

Files selected for processing: 1

• crates/gitlawb-core/src/cert.rs

Files skipped from review as they are similar to previous changes: 1

• crates/gitlawb-core/src/cert.rs

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

[beardthelion]

@coderabbitai full review

[coderabbitai]

Action performed

Full review finished.

---

Your included review limit is currently reached under our Fair Usage Limits Policy. Usage-based billing is enabled for your organization, so this review is expected to continue as a usage-based review. Final billing checks, including your spending cap and usage spend policies, run when the review starts. Your next included review will be available in 17 minutes.

[beardthelion]

Addressed the two CodeQL cleartext-logging alerts at 52b6dc9: the assert failure messages in the new cert tests no longer echo the error (the contains checks that pin the contract are unchanged). The third site with the same shape is fixed too. cert::tests is 14/14 green.

[jatmn]

🟢 Thanks for the contribution. I do not see any actionable code issues from my review.

Merge readiness

• Rebase onto current main before merge. Live origin/main is 0cdb4bcc7c4f7b0eb997fc5ea26981b1e0e26e3c (includes #539). This branch’s merge-base is 0e5a1608e21f4859b2919307287747263f9fa8d0, one commit behind. The diff does not touch those files, but rebasing keeps CI and release metadata aligned with project guidance.

• Human review still required. GitHub reports REVIEW_REQUIRED (requested reviewer: jatmn).

• PR template accuracy. The body marks cargo test --workspace as done while Notes say only gitlawb-core, gitlawb-attest, and gitlawb-node suites ran. Align the checkbox with what was actually executed, or run the full workspace suite.

• Backward-compatibility claim. The Protocol checklist asserts backward compatibility with existing nodes and prior signed history, while this change rejects previously accepted non-canonical did:key spellings at every to_verifying_key consumer. That is intentional per #509; consider tightening the checkbox language (e.g. canonical spellings only; signatures over canonical repo/signer strings unchanged) so operators know alternate wire spellings will not verify after upgrade.

Findings

No verified in-scope code defects on this head.

---

Review metadata

Item	Value
Head	52b6dc9809b27bc8d7f91951fc963548fb4a862d
Merge-base	0e5a1608e21f4859b2919307287747263f9fa8d0
Live main	0cdb4bcc7c4f7b0eb997fc5ea26981b1e0e26e3c
Mergeable	yes (BLOCKED = reviews, not conflicts)
Required checks	all green on head