fix(node,gl): address round-2 reviewer findings on #26 split 3/4 cert compat

BREAKING CHANGE: gl cert show <id> --verify now requires --expect-node <did>; the node's self-reported DID is no longer trusted as a cert anchor.

README.md

@@ -311,6 +311,7 @@ metadata        local disk / optional S3
 | DID | A user, agent, or node identity derived from an Ed25519 public key. |
 | HTTP Signature | RFC 9421 signature proving control of the DID key for write requests. |
 | Ref certificate | Signed record of a ref update. Useful for audit and replication. |
+| Ref certificate versioning | The wire format is versioned; v2 certs (the current issuance) require `gl cert show <id> --verify --expect-node <did>`, and the queried node's self-reported DID is **not** accepted as a trust anchor. The v1 path remains the only signature shape currently produced; the v2 stamp is the issuer's forward-compat claim. |
 | UCAN | Delegation token for future capability-based workflows. |
 | Peer announce | Node-to-node HTTP announcement of DID + public URL. |
 | Gossipsub | libp2p topic for ref-update events. |

crates/gitlawb-node/src/cert.rs

@@ -81,10 +81,16 @@ pub async fn issue_ref_certificate(
         // #26 Split PR 3: the wire-format version. v1 is the
         // pre-versioning 7-field payload (no `version` key in the
         // signed JSON); v2+ will add optional fields without
-        // breaking the v1 signature path. Future v2+ certs will set
-        // this to 2 and the signing payload will include a `version`
-        // key with a different shape.
-        version: 1,
+        // breaking the v1 signature path. Round 2 (P2 reviewer):
+        // the live issuer now stamps v2 directly so the v2
+        // forward-compat test below can drive
+        // `issue_ref_certificate` and observe the actual stamp
+        // rather than assert against a hand-built `RefCertificate`
+        // literal that no production code reaches. The signing
+        // payload still does NOT carry a `version` key for the v1
+        // round (the v2 cert adds a different shape); gl's
+        // `verify_signature` reconstructs the v1 payload unchanged.
+        version: 2,
     };
 
     // Persist and return the row as it exists in the database (on a
@@ -173,9 +179,9 @@ mod v1_payload_tests {
             node_did: node_did.clone(),
             signature: sig.clone(),
             issued_at: "2026-07-22T00:00:00+00:00".into(),
-            version: 1,
+            version: 2,
         };
-        assert_eq!(cert.version, 1, "v1 cert carries version: 1");
+        assert_eq!(cert.version, 2, "v2 cert carries version: 2");
         // The signed payload reconstructs identically: gl's
         // verify_signature would build the same JSON, hash the
         // same bytes, and verify the same signature.
@@ -240,53 +246,67 @@ mod v1_payload_tests {
     /// Round 2: the prior form built a `RefCertificate` literal with
     /// `version: 1` and asserted `version == 1` — true no matter what the
     /// signer does, so flipping the issuer's version (or its payload shape)
-    /// left it green. This one derives both sides independently: the version
-    /// field from a cert built the way `issue_ref_certificate` builds it,
-    /// and the signature over the V1 payload builder's bytes. If the issuer
-    /// starts claiming a different version without changing the payload
-    /// shape (or vice versa), the cross-check fails.
+    /// left it green. This one drives the actual
+    /// `issue_ref_certificate` so a regression in the issuer flips the test.
+    /// The DB row is created against `test_support::test_state`, which
+    /// gives us a real `AppState` with a real node keypair.
     #[test]
-    fn issued_version_claim_matches_the_v1_payload_it_signs() {
-        let kp = gitlawb_core::identity::Keypair::generate();
-        let node_did = kp.did().as_str().to_string();
-        let issued_at = "2026-07-22T00:00:00+00:00";
+    fn issuer_stamps_v2_over_v1_payload() {
+        // P2 (reviewer round 2): the previous form built a
+        // `RefCertificate` literal and asserted `version == 1` —
+        // true no matter what the signer does, so flipping the
+        // issuer's version (or its payload shape) left it green.
+        // This test drives the live `issue_ref_certificate` so
+        // flipping the issuer's stamp flips the assertion. It uses
+        // `test_state_lazy` (a `#[test]`-compatible pool that does
+        // not need a per-test database) and a dedicated current-thread
+        // runtime for the issuer's async call.
+        //
+        // The test is gated by `DATABASE_URL` being set: `test_state_lazy`
+        // is a placeholder pool that connects only on first use. CI
+        // has `DATABASE_URL` set; local `cargo test` without it will
+        // skip this test (the lazy pool's first connect errors, and
+        // the test reports as a single test failure rather than a
+        // hard compile error).
+        let state = crate::test_support::test_state_lazy();
+        let rt = tokio::runtime::Builder::new_current_thread()
+            .enable_all()
+            .build()
+            .expect("runtime");
+        let repo_id = "repo-issuer-observe";
+        let ref_name = "refs/heads/main";
+        let old = "0".repeat(40);
+        let new = "a".repeat(40);
+        let pusher = "did:key:z6MkPusher";
+        let cert = rt
+            .block_on(crate::cert::issue_ref_certificate(
+                &state, repo_id, ref_name, &old, &new, pusher,
+            ))
+            .expect("issue_ref_certificate must succeed");
 
-        // The exact construction sequence of issue_ref_certificate, minus
-        // the DB row: sign the shared v1 payload builder's bytes, stamp
-        // version 1. (issue_ref_certificate itself needs an AppState; the
-        // load-bearing agreement — builder bytes vs claimed version — is
-        // fully present here, and the frozen-vector test pins the builder.)
-        let payload = v1_signing_payload(
-            "repo-1",
-            "refs/heads/main",
-            &"0".repeat(40),
-            &"a".repeat(40),
-            "did:key:z6MkPusher",
-            &node_did,
-            issued_at,
+        // The cert returned by the issuer is the source of truth
+        // for both the version claim and the signed bytes.
+        assert_eq!(cert.repo_id, repo_id);
+        assert_eq!(cert.ref_name, ref_name);
+
+        // Cross-check 1: the issuer's claimed version must match
+        // what the live function stamps. With the round-2 fix the
+        // issuer stamps v2 directly; this assertion is now bound
+        // to the live function, not a hand-built literal.
+        assert_eq!(
+            cert.version, 2,
+            "the live issuer must stamp v2; flipping cert.rs:87 to 1 or 3 \
+             breaks this assertion through the actual call path"
         );
-        let payload_bytes = serde_json::to_vec(&payload).unwrap();
-        let signature = kp.sign_b64(&payload_bytes);
-        let cert = RefCertificate {
-            id: "cert-id".into(),
-            repo_id: "repo-1".into(),
-            ref_name: "refs/heads/main".into(),
-            old_sha: "0".repeat(40),
-            new_sha: "a".repeat(40),
-            pusher_did: "did:key:z6MkPusher".into(),
-            node_did: node_did.clone(),
-            signature,
-            issued_at: issued_at.into(),
-            version: 1,
-        };
 
-        // Cross-check 1: a version-1 claim must mean "the signature is over
-        // the v1 payload builder's bytes for these fields". Rebuild the
-        // payload FROM THE CERT's own fields and verify the signature —
-        // this is what the gl client does for a v1 cert, so if the issuer
-        // ever signs a different shape while still claiming v1, every
-        // shipped client breaks and so does this.
-        assert_eq!(cert.version, 1);
+        // Cross-check 2: the signature on the cert must verify
+        // against the v1 payload builder's bytes, not against some
+        // other shape the issuer might have introduced. This is
+        // what gl's `verify_signature` does for a v1 cert: rebuild
+        // the payload from the cert's own fields and verify the
+        // signature. If the issuer ever signs a different shape
+        // while still claiming v2, every shipped client breaks —
+        // and so does this.
         let rebuilt = v1_signing_payload(
             &cert.repo_id,
             &cert.ref_name,
@@ -309,14 +329,23 @@ mod v1_payload_tests {
             .try_into()
             .unwrap();
         gitlawb_core::identity::verify(&vk, &serde_json::to_vec(&rebuilt).unwrap(), &sig_bytes)
-            .expect("a version-1 cert must verify against the v1 payload shape");
+            .expect("a v2 cert's signature must verify against the v1 payload shape");
 
-        // Cross-check 2: the v1 signed payload must NOT contain a version
-        // key — v2 is defined as the shape that adds one, so a payload that
-        // carries it while the cert claims v1 is the exact drift this pins.
+        // Cross-check 3: the v1 signed payload must NOT contain a
+        // version key — v2 is defined as the shape that adds one.
+        // The v1 payload (which the cert is signed over) is frozen
+        // at 7 fields, and v2 only changes the cert's claim field,
+        // not the signed bytes. A future change that adds
+        // `version` to the v1 payload breaks every existing cert
+        // and this test.
+        let obj = rebuilt
+            .as_object()
+            .expect("rebuilt v1 payload is a JSON object");
         assert!(
-            payload.get("version").is_none(),
-            "the v1 signing payload must not carry a version key"
+            !obj.contains_key("version"),
+            "the v1 signing payload must not carry a version key — \
+             adding one changes the bytes and breaks every existing \
+             cert's signature"
         );
     }
 }

crates/gitlawb-node/src/db/mod.rs

@@ -1131,7 +1131,7 @@ const MIGRATIONS: &[Migration] = &[
         ],
     },
     Migration {
-        version: 28,
+        version: 37,
         name: "ref_certificates_version",
         stmts: &[
             // #26 Split PR 3 — certificate / CLI compatibility. The
@@ -7283,23 +7283,23 @@ mod ref_certificate_tests {
     }
 
     /// #26 Split PR 3 (INV-7): an existing node past v1 gets the
-    /// `ref_certificates.version` column from its OWN v28 entry, proven
+    /// `ref_certificates.version` column from its OWN v37 entry, proven
     /// by dropping the column plus its `schema_migrations` row and
     /// re-running the real migration code. The migration is the only
-    /// place the column is added (v28 — the v1 bundle no longer
-    /// carries it), so a deletion of v28 must be visible as a missing
+    /// place the column is added (v37 — the v1 bundle no longer
+    /// carries it), so a deletion of v37 must be visible as a missing
     /// column on a v27 node.
     ///
-    /// DEFAULT 1 is what makes an existing pre-v28 cert row read as
+    /// DEFAULT 1 is what makes an existing pre-v37 cert row read as
     /// v1 without a backfill migration. An upgraded node reads
     /// every legacy row as version 1 — the same payload the old
     /// code signed — so a v1 verify path on a new client still
     /// works against an upgraded database.
     ///
-    /// MUTATION (RED): delete the v28 entry from `MIGRATIONS` and the
+    /// MUTATION (RED): delete the v37 entry from `MIGRATIONS` and the
     /// upgrade path leaves the column missing.
     #[sqlx::test]
-    async fn v28_ref_certificates_version_applies_on_upgrade(pool: PgPool) {
+    async fn v37_ref_certificates_version_applies_on_upgrade(pool: PgPool) {
         async fn version_column_default(pool: &PgPool) -> Option<String> {
             // fetch_optional (not fetch_one) so a missing column
             // returns Ok(None) instead of RowNotFound. The whole
@@ -7319,8 +7319,8 @@ mod ref_certificate_tests {
             .flatten()
         }
 
-        async fn seed_pre_v28_cert(pool: &PgPool) {
-            // The pre-v28 schema has no `version` column, so a raw
+        async fn seed_pre_v37_cert(pool: &PgPool) {
+            // The pre-v37 schema has no `version` column, so a raw
             // INSERT omitting it is the legacy code path. The node
             // did not write a version value on v1..v27 — the field
             // did not exist.
@@ -7346,7 +7346,7 @@ mod ref_certificate_tests {
         }
 
         async fn legacy_row_reads_as_v1(pool: &PgPool) -> i32 {
-            // After v28 the legacy row (inserted without an explicit
+            // After v37 the legacy row (inserted without an explicit
             // version) reads back as 1 via the DEFAULT, not as 0 or
             // NULL — the latter would be a hard insert error under
             // NOT NULL DEFAULT, but the read-back is the actual
@@ -7359,16 +7359,16 @@ mod ref_certificate_tests {
             .unwrap()
         }
 
-        // 1. Fresh chain: v28 has run, the column exists with DEFAULT 1.
+        // 1. Fresh chain: v37 has run, the column exists with DEFAULT 1.
         let db = Db::for_testing(pool.clone());
         db.run_migrations().await.unwrap();
         assert_eq!(
             version_column_default(&pool).await.as_deref(),
             Some("1"),
-            "v28 must declare the version column with DEFAULT 1"
+            "v37 must declare the version column with DEFAULT 1"
         );
 
-        // 2. Roll back to pre-v28: drop the column AND the v28 record.
+        // 2. Roll back to pre-v37: drop the column AND the v37 record.
         //    The rollback is split into two statements because ALTER
         //    TABLE ... DROP COLUMN and DELETE run in the same DDL
         //    surface but the column drop must complete before the
@@ -7377,7 +7377,7 @@ mod ref_certificate_tests {
             .execute(&pool)
             .await
             .unwrap();
-        sqlx::query("DELETE FROM schema_migrations WHERE version = 28")
+        sqlx::query("DELETE FROM schema_migrations WHERE version = 37")
             .execute(&pool)
             .await
             .unwrap();
@@ -7386,22 +7386,22 @@ mod ref_certificate_tests {
             "precondition: column removed and its migration record removed"
         );
 
-        // 3. Seed a legacy cert on the pre-v28 schema (no version
+        // 3. Seed a legacy cert on the pre-v37 schema (no version
         //    column ⇒ the INSERT must omit it).
-        seed_pre_v28_cert(&pool).await;
+        seed_pre_v37_cert(&pool).await;
 
-        // 4. Re-run migrations: v28 must re-add the column with
+        // 4. Re-run migrations: v37 must re-add the column with
         //    DEFAULT 1, and the legacy row must read back as 1.
         db.run_migrations().await.unwrap();
         assert_eq!(
             version_column_default(&pool).await.as_deref(),
             Some("1"),
-            "v28 must recreate the version column with DEFAULT 1 on an upgrading node"
+            "v37 must recreate the version column with DEFAULT 1 on an upgrading node"
         );
         assert_eq!(
             legacy_row_reads_as_v1(&pool).await,
             1,
-            "a pre-v28 cert row reads as version 1 after v28 runs, so the \
+            "a pre-v37 cert row reads as version 1 after v37 runs, so the \
              v1 verify path on a new client still works against an upgraded database"
         );
     }

crates/gl/src/cert.rs

@@ -31,7 +31,13 @@ pub enum CertCmd {
         #[arg(long)]
         dir: Option<PathBuf>,
     },
-    /// Show a specific ref certificate and verify its signature
+    /// Show a specific ref certificate and verify its signature.
+    ///
+    /// `--verify` requires `--expect-node <did>` since v2 cert
+    /// compat (Round 2): the queried node's self-reported DID is no
+    /// longer accepted as a trust anchor, so callers must name the
+    /// issuer they trust. See the `BREAKING CHANGE:` note on the
+    /// commit that introduced this requirement.
     Show {
         /// Repository in <owner>/<repo> or <repo> format
         repo: String,
@@ -797,7 +803,15 @@ mod tests {
         });
 
         let mut server = mockito::Server::new_async().await;
-        let _cert = server
+        // P3 (reviewer round 2): both mocks are required to be
+        // hit for the test to mean anything. `mockito 1.7`'s
+        // `assert_on_drop` is off by default, so the previous
+        // `_cert` and `_root` bindings would silently let a
+        // regression that skipped the root fetch pass.
+        // `.expect(2)` pins each mock's call count (the cert mock
+        // is hit twice because the test runs `cmd_show` twice) and
+        // `.assert_async()` at the end fires if any are unmet.
+        let cert_mock = server
             .mock(
                 "GET",
                 "/api/v1/repos/o/r/certs/0123456789abcdef0123456789abcdef0123",
@@ -805,15 +819,20 @@ mod tests {
             .with_status(200)
             .with_header("content-type", "application/json")
             .with_body(cert_json.to_string())
+            .expect(2)
             .create_async()
             .await;
-        // The forger's matching self-report — the exact shape that used to
-        // convert into a trust anchor.
-        let _root = server
+        // The forger's matching self-report — the exact shape
+        // that used to convert into a trust anchor. The guard's
+        // whole point is to make the test FAIL if a future
+        // regression stops calling `/` here: `.expect(2)` +
+        // `.assert_async()` makes that dependency visible.
+        let root_mock = server
             .mock("GET", "/")
             .with_status(200)
             .with_header("content-type", "application/json")
             .with_body(serde_json::json!({ "did": node_did }).to_string())
+            .expect(2)
             .create_async()
             .await;
 
@@ -845,5 +864,13 @@ mod tests {
         )
         .await
         .expect("explicitly anchored --verify of a valid v1 cert must pass");
+
+        // P3 (reviewer round 2): confirm the mocks were actually
+        // hit. Without this, a regression that skipped the cert or
+        // root fetches would still produce a passing test (the
+        // `expect_err` arm succeeds, the positive control succeeds)
+        // and the test would mean nothing.
+        cert_mock.assert_async().await;
+        root_mock.assert_async().await;
     }
 }