fix(node): harden Arweave anchoring and add verification (#26)

[Gravirei]

Summary

Anchor ref updates and encrypted-blob manifests to Arweave as signed ANS-104 data items, and add /api/v1/arweave/verify/{tx_id} which fetches the anchor from the gateway and validates the embedded certificate chain (node Ed25519 signature, SHA-256 prev linkage, pusher RFC 9421 proof). Also harden the Arweave surface: spec-correct ANS-104 preimages, tamper rejection, fail-closed corroboration, credential redaction on every public output, structural URL handling that rejects fragments, an explicit verified bundler-funding configuration, and durable post-receive jobs persisted before the push is acknowledged.

Durability scope is deliberate and narrow: push accounting, per-ref certificates, and the Arweave anchor are the durable unit — a crash between the pack landing and that bookkeeping is recovered by the startup drain. The replication tail (Pinata/IPFS pinning, gossip, GraphQL push, peer notify) remains best-effort and is not part of the durable unit.

Closes #26.

Kind of change

• Feature
• Security fix
• Tests / CI

What changed

gitlawb-node

• ans104.rs — build and verify ANS-104 signed data items (ed25519, recursive deepHash). The tags preimage is the FLAT serialized tag stream (item.rawTags in the published arbundles getSignatureData), not a nested [[name, value], ...] list — the nested form is what Arweave layer-one transactions use, and a data item's signature would not reproduce with it. Zero tags is an empty blob, not deepHash([]). The empty-tags reference vector and a 3-tag interop fixture are produced by the independent arbundles package (createData + sign) and pinned as hex in tests; the node's own signer produces items this verifier accepts.
• arweave.rs — ref-update and encrypted-manifest anchors are POSTed to {bundler}/tx/{token} as raw signed data items with metadata embedded as tags, paying via the x-irys-paid-by header (Irys UploadHeaders.PAID_BY). The upload client classifies every outcome at the boundary (Accepted only for a well-formed 43-char base64url id; empty/missing/malformed ids and connection drops are Uncertain — never success, never a fabricated anchor), and exposes the content-derived ANS-104 data-item id plus a gateway presence probe (anchor_item_present) that the durable job uses to reconcile a possibly-landed upload without paying twice. verify_anchor() validates the full chain against the local DB; refusals are fail-closed — an uncorroborated outer repo/owner identity makes the result invalid, and the raw DB error never reaches the caller. Every send-error and non-success-body path on the upload side is masked through remote_send_error/remote_response_error so bundler account/token never leak; the same masking covers verify_anchor's connection and mid-stream errors. Tamper tests flip a signature byte on both the 13-field and 7-field verify paths and assert the specific signature error.
• repos.rs — post-receive bookkeeping (record_push, trust score, per-ref issue_ref_certificate) and the Arweave anchor now run inside a durable post-receive job. git_receive_pack persists the job row (id, pusher DID, repo, ref updates, RFC 9421 attestation) BEFORE acknowledging the push, then spawns process_post_receive_job; a crash between the pack landing and the bookkeeping is recovered by the startup drain (drain_post_receive_jobs in main), which resets stale rows to pending and replays them. Jobs are claimed atomically (UPDATE ... WHERE status IN ('pending','failed')): two concurrent drainers converge on a single executor per job. Every effect is idempotent: push_events is keyed on the job id (ON CONFLICT (id) DO NOTHING), certificate ids are deterministic per (job, ref) with insert_ref_certificate_tx idempotent, and a certificate issuance failure now fails the job (retryable) instead of warning and completing. The replication tail (Pinata/IPFS pinning, gossip, GraphQL push, peer notify) is spawned from the same job but is not awaited: the tail reports (announce, cid_map) back over a oneshot channel, and anchor_ref_updates runs in the job body only after that report arrives.
  • The anchor is a durable per-transition outbox state machine (fix(node): harden Arweave anchoring and add verification (#26) #224 review). Per ref transition: (1) claim — an atomic INSERT ... ON CONFLICT DO NOTHING against a new unique (repo, ref_name, old_sha, new_sha) index creates the durable claim in pending BEFORE any paid upload; exactly one worker wins, so exactly one worker can ever pay. (2) prepare — the signed item's deterministic ANS-104 id is persisted on the row before the request is sent. (3) upload — outcomes are classified: a definitive provider rejection marks the row failed; a connection drop or malformed success leaves it uploading (the item may have been accepted). (4) record — the accepted tx id is persisted as the terminal recorded state. A retry that finds a non-terminal claim probes the gateway for the persisted item id first: present → record as-is (no second paid artifact), absent → re-upload, no verdict → fail the job without uploading. The anchor's issuer is state.node_did, never the pusher.
• cert.rs (gitlawb-core) — satisfies_threshold counts distinct signer DIDs, not signature entries; a repeated signature from one maintainer counts once, so copy-pasted signatures cannot fabricate a threshold.
• config.rs — GITLAWB_BUNDLER_TOKEN added; validate() refuses to start with a bundler URL without both a funded account and a payment token (Irys bills at /tx/{token}). An anchoring node must now also set an explicit GITLAWB_ARWEAVE_GATEWAY: the implicit arweave.net default is gone, because it silently paired the gateway to the bundler URL and broke /verify for production deployments (devnet transactions are not resolvable via arweave.net). The legacy GITLAWB_IRYS_URL is adopted via legacy_bundler_url_fallback only when the funded account/token pair is also set; a bare legacy URL no longer silently enables anchoring (the node warns and starts with anchoring disabled).
• .env.example — the bundler block is split into commented Devnet (devnet.irys.xyz + matic + devnet gateway) and Production (node2.irys.xyz + ethereum + https://arweave.net) shapes, and the comment documents that anchoring needs the URL, a funded ACCOUNT, the TOKEN, and a gateway on the same network.
• README.md — corrected the bundler rows: GITLAWB_BUNDLER_ACCOUNT is the funded account that pays (sent as x-irys-paid-by), GITLAWB_BUNDLER_TOKEN is the payment-token slug billed at /tx/{token} (it is NOT an API key and is not sent in the paid-by header), and GITLAWB_ARWEAVE_GATEWAY has no default.
• db/mod.rs — RefCertificate gains seq/prev/pusher_sig/signature_input/content_digest/request_path; arweave_anchors gains cert_id, renames irys_tx_id→arweave_tx_id. The released v1 migration stays byte-identical (it has NO cert-chain columns); column work lives in append-only v18/v19, v20 drops the superseded (repo_id, ref_name) unique index (documented one-way, attributed to the v10 migration that created it), v21 adds the post_receive_jobs table, and v22 carries the anchor outbox: state/item_id/claim_token/claimed_at columns, arweave_tx_id made nullable (a claimed row has no tx yet), dedup of pre-v22 duplicate rows, and the unique per-transition index. The anchors listing reads only recorded (terminal) rows. An upgrade test replays the deployed v1 schema.
• SECURITY.md — corrected to the runtime: UCANs are signed JSON envelopes ({payload, s}, base64url Ed25519 over the payload), not JWTs; the middleware verifies the full delegation chain when X-Ucan is present; read enforcement and per-path visibility rules are wired; GITLAWB_ENFORCE_OWNER_PUSH defaults off.
• server.rs — mask_credential_url drops userinfo, query, and fragment; used by contracts info, the anchors listing, the gateway-inference log, and the verify error body. Bundler/gateway URLs are built with a structural join_url_path that preserves the query and rejects fragments; reqwest errors have the URL redacted and bodies truncated.

Reviewer checklist coverage

• ANS-104 interop (independent implementation): verify_data_item_matches_independent_interop_fixture; flat-tags reference deep_hash_matches_independent_reference_vector.
• Tamper rejection: test_verify_anchor_rejects_tampered_13_field_signature, test_verify_anchor_rejects_tampered_7_field_signature.
• Fail-closed corroboration: test_verify_anchor_fails_closed_when_outer_identity_cannot_be_corroborated; DB error masked at the fail-closed lookup.
• Credential redaction: list_anchors_drops_query_and_fragment_credentials, test_verify_anchor_error_does_not_leak_gateway_query_credentials, test_anchor_ref_update_redacts_credentials_in_error_body, test_manifest_anchor_redacts_credentials_in_error_body, test_verify_anchor_interrupted_stream_error_is_masked, redaction_helpers_scrub_urls_and_secrets, server::tests::drops_query_and_fragment_credentials.
• Structural URL join + fragment rejection: test_anchor_preserves_bundler_path_prefix, test_anchor_preserves_bundler_query, test_anchor_rejects_fragment_in_bundler_url, test_verify_anchor_preserves_gateway_query, test_verify_anchor_rejects_fragment_in_gateway_url.
• Funding model: bundler_url_requires_a_funded_account (config), bundler_url_requires_an_explicit_gateway (config), arweave_gateway_has_no_default_network (config), legacy_irys_url_is_adopted_only_with_funded_account_pair (config), env_example_bundler_block_is_startable, test_anchor_ref_update_rejects_missing_bundler_account (request).
• Durable post-receive jobs: post_receive_job_survives_handler_abort (crash-between-enqueue-and-spawn recovery plus idempotent replay), inv22_replication_tail_spawns_at_the_durability_boundary (ordering gate binds enqueue-before-spawn-before-release).
• Anchor-as-durable-unit: anchor_record_failure_is_reconciled_without_double_pay (accepted upload whose recorded UPDATE fails → row left uploading with its item id; the retry probes the gateway, finds the item, records it WITHOUT a second paid request), anchor_claim_db_failure_never_uploads (claim unanswerable → fail closed, no upload), anchor_probe_failure_never_uploads (gateway probe returns no verdict → fail closed, no upload, row stays non-terminal), post_receive_job_anchor_failure_retries_and_replay_never_reuploads (bundler 500 → row failed, the drain probes, re-uploads, records, done; replay never pays twice; the stored anchor's node_did is the node's, not the pusher's), two_concurrent_workers_claim_the_job_once (atomic job claim → exactly one upload).
• Tx-id boundary validation: test_upload_rejects_empty_missing_and_malformed_success_ids (empty/missing/malformed bundler ids are Uncertain; a 43-char base64url id is Accepted).
• Distinct-signer threshold: satisfies_threshold_rejects_duplicated_signature (a duplicated signature of one key fails a 2-of-3).
• Anchors listing: list_anchors_limit_zero_uses_default_limit (limit=0 falls back to the default page size instead of returning nothing), list_anchors_without_gateway_omits_arweave_url (no gateway → no relative /tx_id URL, the durable tx id still lists).
• Migration immutability: upgrade_path_tests::upgrading_released_v1_schema_lands_cert_and_anchor_columns.

How a reviewer can verify

DATABASE_URL=postgresql://gitlawb:changeme@localhost:5432/gitlawb cargo test --workspace

All 1417 tests across the workspace pass (877 in the gitlawb-node suite). cargo fmt --all -- --check and cargo clippy --workspace --all-targets -- -D warnings are clean.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This change replaces Irys uploads with bundler requests, embeds chained ref certificates and pusher signature metadata, adds anchor lifecycle persistence, and exposes gateway-based Arweave transaction verification.

Changes

Arweave integrity flow

Layer / File(s)	Summary
Certificate chain and pusher signature crates/gitlawb-node/src/auth/mod.rs, crates/gitlawb-node/src/cert.rs, crates/gitlawb-node/src/db/mod.rs, crates/gitlawb-node/src/api/repos.rs, crates/gitlawb-node/src/api/certs.rs	Verified pusher signatures are propagated into append-only certificates with sequence, predecessor hashes, RFC 9421 metadata, and expanded API responses.
Bundler anchoring and anchor lifecycle crates/gitlawb-node/src/arweave.rs, crates/gitlawb-node/src/api/repos.rs, crates/gitlawb-node/src/config.rs, crates/gitlawb-node/src/main.rs, crates/gitlawb-node/src/db/mod.rs, crates/gitlawb-node/src/server.rs	Anchors use bundler /v1/tx requests, configurable bundler and gateway settings, embedded certificates, and pending/confirmed/failed persistence.
Anchor verification API crates/gitlawb-node/src/arweave.rs, crates/gitlawb-node/src/api/arweave.rs, crates/gitlawb-node/src/server.rs	Gateway payloads are fetched and checked for certificate signatures and predecessor linkage at GET /api/v1/arweave/verify/:tx_id.
Validation and compatibility updates crates/gitlawb-node/src/arweave.rs, crates/gitlawb-node/src/db/mod.rs, crates/gitlawb-node/src/api/events.rs, crates/gitlawb-node/src/test_support.rs	Tests and fixtures cover bundler routes, gateway failures, append-only certificates, anchor lifecycle transitions, and expanded certificate fields.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant git_receive_pack
  participant issue_ref_certificate
  participant Bundler
  participant verify_anchor_endpoint
  participant verify_anchor
  participant ArweaveGateway
  participant Db
  Client->>git_receive_pack: Authenticated push request
  git_receive_pack->>issue_ref_certificate: Pass pusher signature and proof
  issue_ref_certificate->>Db: Store chained certificate
  git_receive_pack->>Bundler: POST /v1/tx with certificate anchor
  Bundler-->>git_receive_pack: Return transaction ID
  Client->>verify_anchor_endpoint: GET /api/v1/arweave/verify/{tx_id}
  verify_anchor_endpoint->>verify_anchor: Verify transaction ID
  verify_anchor->>ArweaveGateway: GET gateway/{tx_id}
  ArweaveGateway-->>verify_anchor: Return anchored payload
  verify_anchor->>Db: Load predecessor certificate
  verify_anchor-->>Client: Return validity, errors, and certificate

Loading

Possibly related PRs

• Gitlawb/node#72: Both PRs modify per-ref anchoring payloads and certificate issuance.
• Gitlawb/node#149: Both PRs modify certificate listing endpoints and certificate response fields.

Suggested labels: sev:high, kind:security, subsystem:attestation, subsystem:api

Suggested reviewers: jatmn, kevincodex1

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR implements provider-neutral anchoring, embedded signed certificates, pusher signature persistence, seq/prev chaining, and gateway-based verification.
Out of Scope Changes check	✅ Passed	The changes stay focused on Arweave anchoring, verification, auth, schema updates, and related tests, with no clear unrelated churn.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The title clearly summarizes the main changes: hardened Arweave anchoring and added anchor verification.
Description check	✅ Passed	The description is detailed and covers the changes, motivation, verification steps, tests, security impact, and known scope.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/gitlawb-node/src/arweave.rs`:
- Around line 351-373: Update the prev-linkage validation in verify_anchor to
fetch and hash the local certificate at sequence c.seq - 1, rather than the
newest certificate returned by get_most_recent_cert. Only perform the comparison
when that predecessor exists, while preserving the existing mismatch error
handling and payload hashing behavior.
- Around line 319-343: Update the signature decoding in the certificate
verification flow to use the URL-safe, no-padding base64 engine matching
node_keypair.sign_b64 output, while preserving the existing 64-byte validation
and error-result handling.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fee43d49-2e4b-4bbd-9921-8248f57fea48

📥 Commits

Reviewing files that changed from the base of the PR and between ad7c2b2 and 0801800.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (10)

• crates/gitlawb-node/src/api/arweave.rs
• crates/gitlawb-node/src/api/events.rs
• crates/gitlawb-node/src/api/repos.rs
• crates/gitlawb-node/src/arweave.rs
• crates/gitlawb-node/src/auth/mod.rs
• crates/gitlawb-node/src/cert.rs
• crates/gitlawb-node/src/config.rs
• crates/gitlawb-node/src/db/mod.rs
• crates/gitlawb-node/src/server.rs
• crates/gitlawb-node/src/test_support.rs

[jatmn]

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

Findings

• [P1] Use the configured gateway's data URL when verifying an anchor
  crates/gitlawb-node/src/arweave.rs:265
arweave_gateway defaults to https://arweave.net, but this requests /v1/tx/{id}, which is the bundler API used for uploads rather than an Arweave gateway data URL. Consequently every normally uploaded anchor is reported invalid with the documented default configuration. Fetch the item through the gateway's data path (or add a separately named bundler-read configuration), and cover the default configuration rather than a mock of the bundler path.

• [P1] Bind each anchor to the certificate for its own ref update
  crates/gitlawb-node/src/api/repos.rs:1306
  Certificates are issued once per update above this block, but every iteration subsequently reads the repository-wide latest certificate. In a multi-ref push, all permanent anchors therefore embed the last update's certificate; another completed push can also win the asynchronous race. Since verify_anchor never compares the certificate's repo/ref/old/new fields with the outer anchor, it returns valid for an anchor whose advertised transition was never signed. Preserve the returned certificate per update and reject a mismatch during verification.

• [P1] Preserve certificate history and fail closed on a missing predecessor
  crates/gitlawb-node/src/db/mod.rs:2013
  The existing (repo_id, ref_name) upsert overwrites the predecessor whenever that ref is pushed again, while the new seq/prev design requires that predecessor to remain available. verify_anchor then silently skips the check when get_cert_by_seq returns None or errors, so an ordinary repeated push produces a truncated chain reported as valid. Make chain entries append-only and treat an unavailable declared predecessor as invalid (or explicitly unverifiable).

• [P2] Allocate chain sequence numbers atomically
  crates/gitlawb-node/src/cert.rs:31
  Sequence allocation is a read-then-increment with no transaction, lock, or unique (repo_id, seq) constraint. Concurrent successful pushes can receive the same sequence and predecessor; get_cert_by_seq then selects an arbitrary row. This makes a signed chain nondeterministic under normal concurrent traffic. Allocate the sequence transactionally and enforce uniqueness, with retry on collision.

• [P1] Do not describe raw signature bytes as a verifiable pusher authorization proof
  crates/gitlawb-node/src/auth/mod.rs:252
  Only the 64-byte Ed25519 signature is persisted. The RFC 9421 Signature-Input, covered component values, method/path, and content digest are discarded, and verify_anchor never verifies pusher_sig. A third party therefore cannot reconstruct the signing string or bind these bytes to this push, yet the endpoint can report the anchor valid. Persist a complete verifiable authorization artifact and validate it, or remove the proof/verification claim.

• [P1] Bound the untrusted response read on the public verification route
  crates/gitlawb-node/src/arweave.rs:279
  The new unauthenticated, unthrottled route buffers the full gateway response with resp.bytes() before attempting JSON parsing. A caller can repeatedly select large data items and force corresponding memory and bandwidth consumption on the node. Apply a strict response-size limit (and a route-appropriate rate limit) before buffering or parsing the body.

• [P3] Implement the promised anchor failure lifecycle instead of dropping failed uploads
  crates/gitlawb-node/src/api/repos.rs:1324
  Upload failures only log a warning; no anchor row is created, retried, confirmed, or marked failed. The new pending/confirmed/failed methods are unused, and success-only rows are always inserted as pending. This does not meet the linked issue's stated retry/visible-gap acceptance criterion, so transient bundler failures silently leave history unanchored. Persist pending work before upload and drive it through bounded retry and terminal status handling.

• [P2] Keep the documented anchoring configuration working during the rename
  crates/gitlawb-node/src/config.rs:91
  This removes GITLAWB_IRYS_URL without a fallback, while both .env.example and README.md still instruct operators to set it. Upgrading an existing documented deployment leaves bundler_url empty and silently disables both anchoring paths. Support the legacy variable for a deprecation period or make the migration explicit and update all operator documentation in the same change.

• [P3] Expose the new signed fields through the certificate API
  crates/gitlawb-node/src/api/certs.rs:45
  The certificate signing payload now includes seq, prev, and pusher_sig, but both list and get responses omit all three fields. Consumers of the established certificate API (including gl cert) therefore cannot reconstruct the signed payload or inspect chain continuity after this change. Serialize the new fields and update the client display/verification path accordingly.

[kevincodex1]

@Gravirei please rebase to main and fix conflicts

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (1)

crates/gitlawb-node/src/test_support.rs (1)

4983-4988: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Make seed_cert produce chain-valid fixtures.

This helper creates the 10- and 55-certificate datasets, but every certificate has seq: 1 and a zero predecessor. The tests therefore cannot catch regressions that ignore monotonic ordering or prev links. Pass sequence/predecessor values through the helper or add a dedicated chained fixture.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/gitlawb-node/src/test_support.rs` around lines 4983 - 4988, Update the
seed_cert fixture helper so generated certificates form a valid chain: assign
increasing sequence values and set each certificate’s prev field to the
preceding certificate’s identifier or digest, with the first certificate using
the chain’s root predecessor. Ensure both the 10- and 55-certificate datasets
exercise monotonic ordering and linked predecessors.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/gitlawb-node/src/arweave.rs`:
- Around line 292-293: Update the payload parsing in verify_anchor_endpoint so
serde_json::from_slice failure is handled as an invalid verification result
rather than propagated as an internal error. Return VerifyResult with valid set
to false and an appropriate error string, while preserving the existing JSON
parsing path.
- Around line 281-293: Update the response handling around resp.bytes() in the
verification flow to enforce the 1 MiB limit before unbounded buffering: reject
any Content-Length above 1_048_576, and stream or otherwise cap reads so
responses without a trustworthy length header cannot exceed the limit. Preserve
the existing invalid VerifyResult fields and JSON parsing for accepted payloads.

In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 923-938: Migration version 13 must backfill distinct
per-repository sequence values before enforcing uniqueness. In the migration’s
`stmts` array, add an update that assigns deterministic, non-colliding `seq`
values to existing `ref_certificates` rows grouped by `repo_id`, then create
`idx_ref_certs_repo_seq`; preserve the existing column additions and append-only
index changes.
- Around line 5157-5162: Ensure each certificate created through make_cert
receives a unique seq value before insertion, either by incrementing it within
make_cert or overriding it at every test call site. Update the affected
certificate setup so list_ref_certificates_respects_limit and
insert_ref_certificate_append_only use distinct sequence numbers and avoid the
(repo_id, seq) uniqueness conflict.

---

Nitpick comments:
In `@crates/gitlawb-node/src/test_support.rs`:
- Around line 4983-4988: Update the seed_cert fixture helper so generated
certificates form a valid chain: assign increasing sequence values and set each
certificate’s prev field to the preceding certificate’s identifier or digest,
with the first certificate using the chain’s root predecessor. Ensure both the
10- and 55-certificate datasets exercise monotonic ordering and linked
predecessors.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4e15d3f3-7966-4b86-a8fa-640f7c92e10a

📥 Commits

Reviewing files that changed from the base of the PR and between c94e8ed and ae4f5fc.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (12)

• crates/gitlawb-node/src/api/arweave.rs
• crates/gitlawb-node/src/api/certs.rs
• crates/gitlawb-node/src/api/events.rs
• crates/gitlawb-node/src/api/repos.rs
• crates/gitlawb-node/src/arweave.rs
• crates/gitlawb-node/src/auth/mod.rs
• crates/gitlawb-node/src/cert.rs
• crates/gitlawb-node/src/config.rs
• crates/gitlawb-node/src/db/mod.rs
• crates/gitlawb-node/src/main.rs
• crates/gitlawb-node/src/server.rs
• crates/gitlawb-node/src/test_support.rs

🚧 Files skipped from review as they are similar to previous changes (5)

• crates/gitlawb-node/src/api/events.rs
• crates/gitlawb-node/src/server.rs
• crates/gitlawb-node/src/api/arweave.rs
• crates/gitlawb-node/src/config.rs
• crates/gitlawb-node/src/cert.rs

[jatmn]

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

Findings

• [P1] Backfill legacy certificate sequence numbers before adding the unique index
  crates/gitlawb-node/src/db/mod.rs:910
  Migration 12 gives every existing certificate seq = 1, but migration 13 then creates a unique (repo_id, seq) index. Version 10 only deduplicated (repo_id, ref_name), so any existing repository with certificates for two refs has duplicate (repo_id, 1) rows and cannot complete this migration or start. This is also immediately exposed by the changed seed_cert fixture, which inserts ten seq = 1 rows and makes list_certs_respects_limit_param fail. Assign deterministic per-repository sequence/chain values before creating the index and update the fixture.

• [P1] Enforce the verification response limit before buffering the gateway body
  crates/gitlawb-node/src/arweave.rs:282
  The new unauthenticated verification route calls resp.bytes() and only checks the 1 MiB limit after the whole Arweave object has been downloaded and allocated. A caller can select a huge or chunked gateway item and exhaust node memory/bandwidth before the rejection occurs. Stream into a capped reader (and reject a known excessive content length up front) instead of buffering first.

• [P1] Restore cryptographic verification to gl cert show
crates/gl/src/cert.rs:151
  This branch has drifted from its base and removes the base's --verify, --expect-node, Ed25519 verification routine, and tests, although the command is still documented as verifying a certificate. It now only prints a proposed payload and returns success for a modified or self-signed certificate. Please rebase without reverting that fail-closed CLI contract, then update its canonical payload for seq, prev, and pusher_sig.

• [P2] Hold the certificate-chain lock through allocation and insertion
  crates/gitlawb-node/src/db/mod.rs:2160
pg_advisory_xact_lock is transaction-scoped, but this standalone pooled query commits before issue_ref_certificate reads the previous certificate or inserts the new one. Concurrent pushes can therefore select the same sequence; with three or more contenders the single retry can collide again, leaving an accepted push without its certificate/anchor evidence. Use one acquired connection/transaction for the lock, predecessor read, and insert (or a robust serialization/retry strategy).

• [P2] Bind the embedded certificate to the enclosing Arweave anchor
  crates/gitlawb-node/src/arweave.rs:303
  Verification checks the copied certificate signature but never compares the untrusted outer repo, owner_did, ref, SHAs, or node DID to that certificate. An attacker can publish a payload with a valid public certificate while claiming a different ref update and receive valid: true. Reject field mismatches (and match a locally recorded transaction too if this endpoint is meant to validate local anchors).

• [P2] Keep gl status compatible with the remote created by gl init
crates/gl/src/status.rs:146
  This branch regresses the base's multi-remote lookup: the status command now accepts only a gitlawb:// fetch URL on origin, while gl init adds the same URL under the gitlawb remote. Immediately after the supported init flow, gl status reports that the repository is not a Gitlawb repo and skips the PR/issue queries. Rebase without reverting the base's lookup for the gitlawb remote and other Gitlawb fetch/push URLs.

• [P2] Do not hard-code main after a plain git init
crates/gl/src/init.rs:41
  This branch reverts the base's branch/commit-state handling. Plain git init honors the user's init.defaultBranch, but the command unconditionally instructs git push gitlawb main. On master, feature, detached, or unborn HEADs that instruction either targets a nonexistent/wrong ref or fails before the first commit. Rebase without dropping the previous branch/commit-state handling, or initialize main with the compatibility fallback.

• [P2] Preserve the legacy command-line spelling during the bundler rename
  crates/gitlawb-node/src/config.rs:75
  The environment fallback runs only after Clap parses arguments. Existing operators invoking gitlawb-node --irys-url … now receive an unknown-argument startup error even though the PR claims compatibility for GITLAWB_IRYS_URL. Add a deprecated long alias or normalize a retained legacy option as well as the environment variable.

• [P2] Do not remove unrelated CI safety gates from this anchoring change
  .github/workflows/pr-checks.yml:189
  This is stale-base drift rather than part of the Arweave feature: the branch deletes the shipped Windows CLI test lane and the only gitlawb-core dependency-purity gate, together with its allowlist and checker script. That removes platform regression visibility and a supply-chain control for the shared cryptographic core; rebase without deleting these protections, or justify and replace them in a separately scoped change.

[kevincodex1]

@coderabbitai please make an intensive review of this changes

[coderabbitai]

@kevincodex1 I’ll perform an intensive review of the complete change set, with particular attention to certificate-chain integrity, RFC 9421 signature verification, migration safety, anchoring failure handling, and API/security behavior.

✅ Action performed

Full review finished.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

crates/gitlawb-node/src/api/events.rs (1)

429-435: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy lift

Use valid per-repository certificate-chain fixtures.

• crates/gitlawb-node/src/api/events.rs#L429-L435: replace the process-global sequence generator with per-chain fixture state.
• crates/gitlawb-node/src/api/events.rs#L447-L452: derive prev from the preceding certificate instead of always using the genesis hash.
• crates/gitlawb-node/src/test_support.rs#L1481-L1486: avoid assigning a global sequence to an otherwise standalone certificate.
• crates/gitlawb-node/src/test_support.rs#L4967-L4973: make sequence generation scoped to a repository/chain.
• crates/gitlawb-node/src/test_support.rs#L4990-L4995: generate matching predecessor hashes for multi-certificate fixtures.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/gitlawb-node/src/api/events.rs` around lines 429 - 435, Replace the
process-global NEXT_FCERT_SEQ/ref_cert_seq state in
crates/gitlawb-node/src/api/events.rs:429-435 with sequence state scoped to each
certificate chain; update the certificate construction at
crates/gitlawb-node/src/api/events.rs:447-452 to derive prev from the preceding
certificate. In crates/gitlawb-node/src/test_support.rs:1481-1486, leave
standalone certificates without a global sequence; in lines 4967-4973, scope
sequence generation to the repository or chain; and in lines 4990-4995, generate
predecessor hashes that match the preceding certificates in multi-certificate
fixtures.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/gitlawb-node/src/api/certs.rs`:
- Around line 55-57: The certificate JSON responses in
crates/gitlawb-node/src/api/certs.rs must include complete pusher-signature
metadata. Update both the listed-certificates response at lines 55-57 and the
single-certificate response at lines 98-100 to include signature_input,
content_digest, and request_path alongside the existing pusher_sig fields.

---

Nitpick comments:
In `@crates/gitlawb-node/src/api/events.rs`:
- Around line 429-435: Replace the process-global NEXT_FCERT_SEQ/ref_cert_seq
state in crates/gitlawb-node/src/api/events.rs:429-435 with sequence state
scoped to each certificate chain; update the certificate construction at
crates/gitlawb-node/src/api/events.rs:447-452 to derive prev from the preceding
certificate. In crates/gitlawb-node/src/test_support.rs:1481-1486, leave
standalone certificates without a global sequence; in lines 4967-4973, scope
sequence generation to the repository or chain; and in lines 4990-4995, generate
predecessor hashes that match the preceding certificates in multi-certificate
fixtures.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5645b021-f503-4733-92c1-424a3ae2349e

📥 Commits

Reviewing files that changed from the base of the PR and between e9da517 and 8103450.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (12)

• crates/gitlawb-node/src/api/arweave.rs
• crates/gitlawb-node/src/api/certs.rs
• crates/gitlawb-node/src/api/events.rs
• crates/gitlawb-node/src/api/repos.rs
• crates/gitlawb-node/src/arweave.rs
• crates/gitlawb-node/src/auth/mod.rs
• crates/gitlawb-node/src/cert.rs
• crates/gitlawb-node/src/config.rs
• crates/gitlawb-node/src/db/mod.rs
• crates/gitlawb-node/src/main.rs
• crates/gitlawb-node/src/server.rs
• crates/gitlawb-node/src/test_support.rs

[kevincodex1]

@coderabbitai make an intensive review on this please

[coderabbitai]

@kevincodex1 I’ll perform an intensive end-to-end review of #224, focusing on certificate-chain correctness, RFC 9421 proof validation, database migrations, anchoring lifecycle/error handling, and API security.

✅ Action performed

Full review finished.

[coderabbitai]

♻️ Duplicate comments (1)

crates/gitlawb-node/src/arweave.rs (1)

303-304: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Non-JSON gateway payload still returns 500 instead of valid: false. serde_json::from_slice(&body_bytes)? propagates through verify_anchor_endpoint (mapped to AppError::Internal), inconsistent with the other graceful branches and the "could be JSON or raw bytes" comment. Convert a parse failure into VerifyResult { valid: false, .. } with an error string.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/gitlawb-node/src/arweave.rs` around lines 303 - 304, Update the anchor
parsing in verify_anchor_endpoint so serde_json::from_slice failures are handled
as an invalid verification result rather than propagated as AppError::Internal.
Return VerifyResult with valid set to false and an error string for non-JSON
payloads, while preserving the existing successful JSON path and other graceful
branches.

🧹 Nitpick comments (2)

crates/gitlawb-node/src/server.rs (1)

222-228: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Consider throttling the unauthenticated verify route. Each GET /api/v1/arweave/verify/{tx_id} triggers an outbound gateway fetch plus a DB lookup with no auth or per-IP brake, so it's an amplification/DoS surface (node → gateway) reachable by anonymous callers. Given the other cost-bearing routes here carry a per-IP IpRateLimiter, consider wrapping arweave_routes similarly. (tx_id is a fixed-host path segment, so this is a load concern, not SSRF.)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/gitlawb-node/src/server.rs` around lines 222 - 228, Wrap the
arweave_routes router, including GET /api/v1/arweave/verify/{tx_id}, with the
existing per-IP IpRateLimiter used by other cost-bearing routes. Preserve the
current list_anchors and verify_anchor_endpoint handlers while ensuring
anonymous requests are throttled before triggering gateway or database work.

crates/gitlawb-node/src/db/mod.rs (1)

2834-2870: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

cert_id is never persisted on anchor rows. RecordAnchorInputV2 has no cert_id field and record_arweave_anchor's INSERT omits it, so the cert_id column added in migration v12 stays NULL for every anchor even though list_arweave_anchors/list_pending_anchors project it. The push path in api/repos.rs already has the issued certificate in scope (ref_certs_clone), so the anchor→certificate DB linkage this column was added for is currently unreachable. Consider threading the cert id through so audits can join anchors to their certs. (gateway_url on the input is likewise accepted but ignored by this function.)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/gitlawb-node/src/db/mod.rs` around lines 2834 - 2870, The anchor
record flow does not persist the issued certificate ID. Add a cert_id field to
RecordAnchorInputV2, pass the corresponding ID from the push path using
ref_certs_clone, and include it in record_arweave_anchor’s INSERT and bindings
so cert_id is stored on each anchor row; also remove or persist gateway_url
consistently instead of silently ignoring it.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@crates/gitlawb-node/src/arweave.rs`:
- Around line 303-304: Update the anchor parsing in verify_anchor_endpoint so
serde_json::from_slice failures are handled as an invalid verification result
rather than propagated as AppError::Internal. Return VerifyResult with valid set
to false and an error string for non-JSON payloads, while preserving the
existing successful JSON path and other graceful branches.

---

Nitpick comments:
In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 2834-2870: The anchor record flow does not persist the issued
certificate ID. Add a cert_id field to RecordAnchorInputV2, pass the
corresponding ID from the push path using ref_certs_clone, and include it in
record_arweave_anchor’s INSERT and bindings so cert_id is stored on each anchor
row; also remove or persist gateway_url consistently instead of silently
ignoring it.

In `@crates/gitlawb-node/src/server.rs`:
- Around line 222-228: Wrap the arweave_routes router, including GET
/api/v1/arweave/verify/{tx_id}, with the existing per-IP IpRateLimiter used by
other cost-bearing routes. Preserve the current list_anchors and
verify_anchor_endpoint handlers while ensuring anonymous requests are throttled
before triggering gateway or database work.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d81eba6b-bb9a-4ee3-af1e-5e62923f6b5f

📥 Commits

Reviewing files that changed from the base of the PR and between e9da517 and a01160f.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (12)

• crates/gitlawb-node/src/api/arweave.rs
• crates/gitlawb-node/src/api/certs.rs
• crates/gitlawb-node/src/api/events.rs
• crates/gitlawb-node/src/api/repos.rs
• crates/gitlawb-node/src/arweave.rs
• crates/gitlawb-node/src/auth/mod.rs
• crates/gitlawb-node/src/cert.rs
• crates/gitlawb-node/src/config.rs
• crates/gitlawb-node/src/db/mod.rs
• crates/gitlawb-node/src/main.rs
• crates/gitlawb-node/src/server.rs
• crates/gitlawb-node/src/test_support.rs

[beardthelion]

Reviewed the current head against merged state. The certificate-chain design is genuinely strong and every serious finding from the earlier rounds is resolved on this head: per-ref cert binding, prev-hash linkage, fail-closed on a missing predecessor, atomic seq allocation (transaction-scoped advisory lock plus a (repo_id, seq) unique index and a 23505 retry), the gateway data-URL fix, the migration backfill-before-unique-index, and the full pusher-signature metadata on both cert endpoints. One functional bug blocks it: the verify endpoint never validates a real anchor. Findings highest first.

Findings

• [P1] Compare the outer anchor repo against the certificate using the same identifier domain
  crates/gitlawb-node/src/arweave.rs:334
  The verify cross-check does outer_repo != Some(&c.repo_id), but the outer anchor's repo is written as the slug {owner_key}/{name} (api/repos.rs:1222) while the embedded certificate's repo_id is the repo UUID (issue_ref_certificate(&record.id, ...), api/repos.rs:1001). Slug never equals UUID, so every honestly produced anchor pushes a repo-mismatch error and returns valid: false. The endpoint cannot go green on real data: push to a public repo with a bundler configured, take arweave_tx_id from /api/v1/arweave/anchors, and GET /api/v1/arweave/verify/{tx_id} reports invalid despite a good node signature. This is the cross-check added in response to the earlier "verify does not compare the transition" finding, so the fix landed but across mismatched identifier domains; the ref/old/new/node comparisons beside it are correct. Either carry the UUID in the outer anchor, or resolve the slug to the repo id on the verify side before comparing. The verify tests set the embedded cert's repo_id equal to the outer slug, which is why CI stays green while production never matches.

• [P2] Make the pusher authorization proof load-bearing, not silently skippable
  crates/gitlawb-node/src/arweave.rs:465
  The pusher-proof check is gated on all four of pusher_sig, signature_input, content_digest, request_path being present, but the node signing payload (cert.rs) covers only pusher_sig — not the other three. A holder of a valid node signature can null signature_input/content_digest/request_path; the node signature still verifies (those fields are unsigned), the whole if let (Some, Some, Some, Some) block is skipped, and verification returns valid with the independent RFC 9421 proof never checked. That defeats the stated goal of letting a third party verify the pusher authorization without trusting the node alone; it bites under node-key compromise. Bind the three context fields into the node payload and treat a present pusher_sig with missing context as invalid rather than passing.

• [P2] Bound the gateway body by bytes read, not Content-Length
  crates/gitlawb-node/src/arweave.rs:293
  The 1 MiB guard only short-circuits when the gateway sends an honest Content-Length; a chunked or header-omitting (or low-lying) response skips the pre-check, and resp.bytes().await then buffers the whole body before the post-check runs. The verify route is unauthenticated (IP-rate-limited only) and tx_id is caller-chosen, so a permissionless caller can drive multi-hundred-MB allocations on the async worker, bounded only by the 10s client timeout. Stream with a running cap (resp.chunk() loop, abort past 1 MiB) rather than buffering first.

• [P2] Add an executed upgrade-path test for the v13 seq backfill
  crates/gitlawb-node/src/db/mod.rs:923
  The v13 backfill and idx_ref_certs_repo_seq build are never exercised through run_migrations() against pre-existing multi-cert data: v10_upgrade_dedup_via_migration re-applies only v10 (it deletes just the v10 row from schema_migrations), and migration_v11_creates_owner_did_column seeds no certificates. The backfill logic itself is sound (ROW_NUMBER() OVER (PARTITION BY repo_id ORDER BY issued_at, id) over a NOT NULL column with a total tiebreaker), but a data migration this consequential needs a test that seeds schema_migrations at v12, inserts several same-repo/different-ref certs (all seq=1 after v12), runs the migrations, and asserts distinct seq plus the unique index present.

• [P3] Cluster of smaller items
  crates/gitlawb-node/src/arweave.rs:272
  Config docs still point at the dead knob: .env.example, README, and the arweave.rs module comment reference GITLAWB_IRYS_URL and never mention GITLAWB_BUNDLER_URL or GITLAWB_ARWEAVE_GATEWAY (not a break — main.rs falls back to the old env var with a deprecation warning — but the docs should match). A gateway-fetch failure or a malformed embedded node_did returns a 500 that echoes the internal error string (api/arweave.rs) rather than a clean valid:false with the right status. tx_id is unvalidated before being appended to the gateway URL (no host-swap SSRF given the fixed authority and redirect::none, but validate to the 43-char base64url shape as cheap defense). repo_lock_hash uses DefaultHasher, which the std docs do not guarantee stable across Rust versions, so two differently-built nodes on one Postgres could hash a repo to different lock keys (backstopped by the unique index and retry, so retry storms rather than corruption; prefer a stable hash). The outer old_sha/new_sha/node_did cross-checks are skipped when the field is absent (is_some() guards), unlike repo/ref; a forger who omits them still gets valid. Minor: the dead lock_repo_cert_issuance helper locks on the pool connection (immediate release, a no-op) and should be removed or made a session lock, and the endpoint doc comment says "most recent local cert" while the code chains against seq-1.

Core design is sound and the prior integrity findings are genuinely resolved; the P1 is the blocker and it is a last-mile identifier mismatch, not a redesign.

[jatmn]

Blocking findings

1. verify_anchor compares anchor repo slug against certificate repo_id

Severity: Blocking
Files: crates/gitlawb-node/src/arweave.rs:335, crates/gitlawb-node/src/api/repos.rs:1328, crates/gitlawb-node/src/cert.rs:97

The outer anchor payload stores the human-readable repo slug (owner_short/repo_name), while the embedded RefCertificate stores the database UUID (record.id). The verify cross-check does:

} else if outer_repo != Some(&c.repo_id) {

For every real anchor these values differ, so the endpoint returns valid: false even when the node signature, prev hash, and pusher proof are correct. This makes the verify endpoint unusable on production data.

Fix: Either store repo_id in the anchor payload (or add a dedicated repo_id field), or resolve the slug to the repo UUID on the verify side before comparing. Prefer storing the UUID in the anchor and keeping the slug for display only.

2. gl cert show --verify uses the old 7-field signed payload

Severity: Blocking
Files: crates/gl/src/cert.rs:259-267, crates/gitlawb-node/src/cert.rs:25-36

The CLI reconstructs the node-signed payload with only the original seven fields:

{ "repo_id", "ref", "old", "new", "pusher", "node", "ts" }

The node now signs ten fields:

{ "repo_id", "ref", "old", "new", "pusher", "node", "ts", "seq", "prev", "pusher_sig" }

Because serde_json serializes maps alphabetically, the added keys change the signed bytes. gl cert show --verify will report INVALID for every certificate issued after this PR, even though the node and the Arweave verifier accept them.

The CLI tests payload_serialization_matches_frozen_canonical_form and verify_signature_round_trip_and_tamper still pin the old format and therefore pass while real-world verification fails.

Fix: Update verify_signature in gl/src/cert.rs to include seq, prev, and pusher_sig, and update the frozen canonical tests accordingly.

High-priority findings

3. New anchor lifecycle methods are dead code

Severity: High
Files: crates/gitlawb-node/src/db/mod.rs:2920-2975, crates/gitlawb-node/src/api/repos.rs:1342-1354

confirm_arweave_anchor, fail_arweave_anchor, and list_pending_anchors are marked #[allow(dead_code)] and never called. Anchors are inserted with status = 'pending', deadline_height = NULL, and receipt_sig = NULL, and no background worker ever updates them. The new status fields are therefore unreliable for monitoring.

Fix: Either add a background confirmation worker in this PR, or remove the unused columns/methods and defer the lifecycle feature to a follow-up.

4. record_arweave_anchor failures are silently dropped

Severity: High
File: crates/gitlawb-node/src/api/repos.rs:1342-1354

let _ = db_clone
    .record_arweave_anchor(&crate::db::RecordAnchorInputV2 { ... })
    .await;

If the local DB insert fails, the anchor transaction exists on Arweave but is not recorded locally, with no log or metric.

Fix: Log a warning/error and optionally increment a metric when the insert fails.

5. contracts_info leaks URLs that may contain credentials

Severity: High
File: crates/gitlawb-node/src/server.rs:583-605

The unauthenticated /api/v1/contracts endpoint returns rpc_url, bundler_url, and arweave_gateway verbatim. If an operator configures a private RPC or paid bundler URL containing an API key, the key is exposed to anonymous callers.

Fix: Mask or omit URLs that may contain credentials, or require authentication for this endpoint.

6. Arweave verify route uses the per-DID creation limiter

Severity: High
Files: crates/gitlawb-node/src/server.rs:225-236, crates/gitlawb-node/src/main.rs:297-298

The verify route is throttled with state.rate_limiter, which is configured as a per-DID repo-creation limiter (10 requests per hour). Even though the middleware keys by IP, the threshold is far too restrictive for a public verification endpoint.

Fix: Add a dedicated Arweave IP rate limiter (e.g. GITLAWB_ARWEAVE_RATE_LIMIT) with a sensible default.

7. Historical certificates get broken prev values after migration

Severity: High
File: crates/gitlawb-node/src/db/mod.rs:923-950

Migration v13 renumbers seq but does not backfill prev. Existing rows except the first per repo keep the zero sentinel, so verify_anchor will reject all pre-upgrade anchors whose embedded certificate has seq > 1.

Fix: Backfill prev in v13 using the same canonical JSON + SHA-256 that cert::prev_hash uses, or explicitly document that historical anchors are intentionally unverifiable.

Medium-priority findings

8. Pusher proof is silently skipped when incomplete

Severity: Medium
File: crates/gitlawb-node/src/arweave.rs:465-551

verify_anchor only runs the RFC 9421 pusher verification when all four of pusher_sig, signature_input, content_digest, and request_path are present. If any is missing (e.g. pre-v13 certificates), the endpoint can still return valid: true without checking who authorized the push.

Fix: At minimum, emit an informational error/warning when a certificate is expected to carry a pusher proof but one is missing.

9. tx_id path parameter is not validated before gateway fetch

Severity: Medium
File: crates/gitlawb-node/src/arweave.rs:267

The user-supplied tx_id is appended directly to the gateway URL. Without length/format validation (Arweave IDs are 43-character base64url strings), the endpoint can be abused as a limited open proxy or SSRF vector against the configured gateway.

Fix: Validate tx_id against ^[A-Za-z0-9_-]{43}$ and return 400 Bad Request early.

10. Outer anchor checks are optional for old_sha, new_sha, node_did

Severity: Medium
File: crates/gitlawb-node/src/arweave.rs:351-371

The cross-check only errors if these fields are present and mismatch. If a malicious anchor omits them, verification still passes. They should be mandatory when a certificate is embedded.

Fix: Fail closed when old_sha, new_sha, or node_did are missing from the outer payload.

11. Advisory lock key uses unstable DefaultHasher

Severity: Medium
File: crates/gitlawb-node/src/db/mod.rs:2241-2246

repo_lock_hash derives the per-repo advisory lock key with std::collections::hash_map::DefaultHasher, which is not guaranteed stable across Rust versions or platforms. Different node builds could compute different lock keys and lose cross-instance serialization.

Fix: Use a stable hash such as the first 8 bytes of SHA-256(repo_id).

12. v1 schema was edited in-place

Severity: Medium
File: crates/gitlawb-node/src/db/mod.rs:462-473, :525-538, :648-663

The migration catalogue comment explicitly states that future changes must be added as new migrations and never appended to v1. However, v1 now already contains the columns that migrations v12/v13 add. While IF NOT EXISTS keeps upgrades safe, this breaks the migration narrative and can confuse future maintainers.

Fix: Either revert the v1 additions and rely solely on v12/v13, or document that v1 in this branch intentionally includes later columns.

Lower-priority findings

13. Test fixtures do not form valid certificate chains

Severity: Low-Medium
Files: crates/gitlawb-node/src/api/events.rs:429-454, crates/gitlawb-node/src/test_support.rs:4964-4997, crates/gitlawb-node/src/db/mod.rs:5215-5252

Test helpers use process-global sequence counters and hard-coded zero prev hashes. They do not exercise per-repo contiguous sequences or cryptographically correct prev linkage, so chain-related regressions could slip through.

Fix: Use per-repo sequence allocation and compute prev from the predecessor certificate, matching production issuance.

14. local_cert events omit the new certificate fields

Severity: Low-Medium
File: crates/gitlawb-node/src/api/events.rs:243-256

The event feed does not include seq, prev, pusher_sig, signature_input, content_digest, or request_path, so event consumers cannot validate chain continuity or reconstruct the pusher proof.

Fix: Include the new fields in the event payload.

[beardthelion]

Traced the signing/verification payload construction on both the node and gl sides line-by-line, and cross-checked the migration runner and the pre-existing rate-limiter/auth-middleware code this PR reuses. Most of the design (append-only per-repo hash chain, advisory-lock-serialized seq allocation, streamed-and-capped gateway fetch, fail-closed cross-checks) is solid. One finding should block merge.

Findings

• [P1] gl cert show always reports a legitimate push-issued certificate as invalid
  crates/gl/src/cert.rs:265
  The node signs the certificate over 13 fields (crates/gitlawb-node/src/cert.rs cert_payload), including signature_input, content_digest, and request_path. gl's client-side verify_signature rebuilds the payload to check the signature against but stops at pusher_sig, never reading or including those three fields. git_receive_pack always passes Some(..) for all three (the route sits behind require_signature, which unconditionally sets them), so every certificate issued for a real push carries them, and the server API (api/certs.rs) already returns them in the JSON — cmd_show just never parses or forwards them. The byte mismatch means the Ed25519 check gl cert show runs fails for every push-issued certificate, reporting a validly node-signed cert as tampered. The PR's own gl/src/cert.rs tests don't catch this because they only exercise the old 10-field shape (pusher_sig: null, no context fields).
  Fix: add signature_input, content_digest, request_path to gl's verify_signature payload and its call sites, matching the server's cert_payload exactly, then add a test that signs and verifies a cert with all three fields populated.

• [P3] .env.example and README still document the renamed config knob
  .env.example, README.md
  Both still reference GITLAWB_IRYS_URL only; neither mentions the new GITLAWB_BUNDLER_URL or GITLAWB_ARWEAVE_GATEWAY (config.rs). main.rs does fall back to the legacy env var with a deprecation warning, so this isn't a functional break, just stale operator-facing docs for a knob this PR renamed.

• [P3] verify_anchor 500s on a malformed node_did instead of returning valid:false
  crates/gitlawb-node/src/arweave.rs
  Every other malformed-input case in verify_anchor (gateway non-2xx, oversized/undecodable body, non-JSON payload) returns Ok(VerifyResult{valid:false, ..}). The node-DID parse (gitlawb_core::did::Did::from_str(&c.node_did).map_err(..)?) still uses ?, so a certificate whose embedded node_did fails to parse propagates as Err, which the handler turns into a 500 instead of the same controlled {valid:false} response every other bad-input path returns.

Superseded by re-review on 0b7a4f0

[beardthelion]

Re-reviewed at 0b7a4f08. Merge-base matches origin/main, CI is 12/12 green on this head, and the rebase ask from the last round is satisfied. I read all four comment surfaces: seven inline CodeRabbit threads, all resolved and outdated; jatmn's open round is still CHANGES_REQUESTED on 0f8c7f26, with no new human review on this push yet. The certificate-chain core from earlier rounds still holds (per-ref certs, issuer pinning, fail-closed corroboration, streaming 1 MiB cap on verify). I ran anchor_record_failure_is_reconciled_without_double_pay green on this head.

The rebased integration did not close the gateway-binding gaps or the Pinata coupling on the durable path. Five ranked open PRs overlap repos.rs, auth/mod.rs, and db/mod.rs on this branch; expect more conflict churn if those land first, but the findings below are about correctness on this head, not merge order.

Findings

• [P2] Bind verify_anchor gateway bodies to the requested tx_id
crates/gitlawb-node/src/arweave.rs:711

  verify_anchor fetches GET {gateway}/{tx_id} but only JSON-parses the body and runs cert/identity checks. It never compares tx_id to ans104::data_item_id(body_bytes) or any payload id field before returning valid: true. A gateway or proxy that returns the same valid anchor JSON for every id can certify the wrong transaction. anchor_item_present's raw-bytes path already binds ids; apply the same binding here (schema gate plus id match), and add a RED test where the mock returns identical JSON for wrong-tx and right-tx.

• [P2] Bind the JSON recovery probe to the probed item_id
crates/gitlawb-node/src/arweave.rs:524

  On a JSON 200, anchor_item_present returns Ok(true) when schema == gitlawb/ref-update/v1 without checking the response matches the requested item_id. The raw-bytes path derives data_item_id and compares (:533-545). A generic gitlawb-shaped JSON 200 can make crash recovery treat an uncertain upload as landed and skip a needed re-upload. Require id binding on the JSON path too, or fall through to the bytes check and fail closed on mismatch.

• [P2] Keep best-effort Pinata work out of the durable anchor completion path
  crates/gitlawb-node/src/api/repos.rs:2674

  run_post_receive_job awaits anchor_cid_rx before anchor_ref_updates, but the Ok sender lives inside the detached Pinata task and runs only after pin_sem_pinata.acquire_owned().await and pinata::pin_new_objects complete (:3371-3427). Comments mark Pinata as best-effort, yet the durable job blocks on it. When the global pin semaphore is saturated, the job stays processing and anchor rows never reach recorded until restart retries the same dependency. CID is already Option in anchor_ref_updates; report the announce decision (and any locally available CID) before dispatching Pinata, or treat CID enrichment as a separate best-effort update. Add an isolation test that holds the pin semaphore and proves the job reaches terminal anchor state without waiting for Pinata.

• [P2] Add a load-bearing test for the per-transition anchor claim
  crates/gitlawb-node/src/db/mod.rs:4473

  two_concurrent_workers_claim_the_job_once races claim_post_receive_job only. The ON CONFLICT (repo, ref_name, old_sha, new_sha) DO NOTHING guard at :4473 is real code but not pinned by a test that races two workers at the anchor-claim layer. Add a test where two tasks reach claim_anchor_claim concurrently and assert exactly one upload path wins.

Not an ask, recorded only: verify_anchor still accepts JSON gateway bodies only; raw ANS-104 bytes at GET /{tx_id} fail with "not valid JSON" even when authentic. Worth aligning with anchor_item_present's dual representation handling, but secondary to the id-binding gaps above.

One process note, not a finding: this branch overlaps #134 on server.rs / arweave.rs and several ranked security PRs on auth/repo paths. Rebase conflict resolution on those surfaces will need another pass when those PRs land.

[beardthelion]

Re-reviewed at aa9fc8b2. CI on that head is 10/12 green: fmt + clippy, MSRV, audit, Docker, and release jobs pass; test (stable) and test (beta) fail before any test logic runs because migration v26 aborts (gh run 33011515250). jatmn's last human review is on 6bf13d1e (2026-08-15); the commits since then address most of that round (explicit gateway validation, synchronous anchor outbox in the job body, README bundler-token contract, verify_anchor id binding). I am not re-litigating those as open on this head.

Findings

• [P1] Strip the cert-chain DDL out of migration v26
  crates/gitlawb-node/src/db/mod.rs:1168

  Migration v26 (pin_repair_sweep_discovery_cursor) currently runs an UPDATE ref_certificates SET seq = ..., creates idx_ref_certs_repo_seq, and adds signature_input / content_digest / request_path. Those statements belong in v32 (append_only_certs_and_pusher_proof), after v29 adds seq. On a fresh database v26 runs before seq exists, so every #[sqlx::test] panics at test_support.rs:43 with column "seq" of relation "ref_certificates" does not exist. I confirmed against origin/main, where v26 only adds the two pin_repair_sweep discovery columns. The duplicate block at db/mod.rs:1200-1231 is rebase debris from aa9fc8b2; remove it from v26 and keep the cert-chain work solely in v32.

• [P2] Add a migration-order regression test that would have caught v26
  crates/gitlawb-node/src/db/mod.rs:8099

  v26_discovery_continuation_applies_on_upgrade only checks the pin-repair discovery columns. It does not exercise a full fresh-chain apply through v32, which is the path CI runs. After fixing v26, add a test that applies migrations v1 through v32 on an empty database (or extend the existing replay upgrade test at db/mod.rs:10004) so cert-chain DDL cannot land before the seq column again.

jatmn's Aug 15 gateway-inference, detached-tail durability, README bundler-token, and upload-without-DB-record findings look addressed on this head: gateway inference is gone from main.rs, Config::validate() requires an explicit gateway when a bundler is set, run_post_receive_job awaits anchor_ref_updates with the outbox pattern, and README matches config.rs on the token slug. I am not restating those as open asks.

Not an ask, recorded only: gl cert show still prints VALID on internally consistent certs without --verify (issuer not anchored to the queried node). verify_anchor skips prev-chain checks for legacy zero-prev rows and warns when outer repo/owner_did are omitted and the repo is absent from the local DB. Both match documented tradeoffs; operators should use gl cert show --verify for third-party checks.

One process note, not a finding: expect a rebase conflict with #134 on arweave.rs if both land close together. This PR also overlaps repos.rs, db/mod.rs, and auth/mod.rs with several in-flight security PRs (#285, #306, #325, #314, #382); merge order may require a follow-up pass on merged state.

Superseded by re-review on aa9fc8b

[jatmn]

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

Findings

• [P1] Make the post-receive outbox durable before the ref update can become externally successful
  crates/gitlawb-node/src/api/repos.rs:2258
  crates/gitlawb-node/src/api/repos.rs:2321
  smart_http::receive_pack completes before this handler constructs and persists post_receive_jobs. At that point Git has already advanced the ref on disk, while the only recovery mechanism added by this PR still exists solely in the request future. A process crash, shutdown, or database failure between those two operations leaves an accepted transition with no job for drain_post_receive_jobs to enumerate. That loses the promised push accounting, trust update, per-ref certificate, and durable anchor for the original transition; sending the client an error cannot undo the landed ref, and retrying can be a no-op or represent a different transition.

  Please address the root atomicity/recovery boundary rather than adding another retry around enqueue_post_receive_job: ensure every ref transition that Git can expose has a durable, deterministic recovery record, or add startup reconciliation that derives and queues exactly the missing work for already-landed transitions. The solution must preserve the existing exactly-once properties across restart and concurrency: one accounting event, deterministic per-job/per-ref certificates, and no duplicate paid anchor. Add fault-injection coverage for a successful receive-pack followed by failed job persistence, then restart/drain and assert the original transition is recovered exactly once.

• [P1] Authenticate the ANS-104 envelope before treating the embedded certificate as an anchor
  crates/gitlawb-node/src/arweave.rs:694
  crates/gitlawb-node/src/ans104.rs:392
  verify_anchor binds the response bytes to the requested item ID and calls data_item_data, but that helper is intentionally structural-only: it does not validate the ANS-104 Ed25519 signature, its deep-hash, or the envelope owner. The subsequent certificate verification proves that a copied certificate was signed by this node; it does not prove the permanent data item containing that certificate was authored by this node. An attacker can create a syntactically valid item under their own key, embed a copied valid certificate and matching outer fields, and have the verify endpoint report it as valid.

  Please make envelope authenticity an explicit prerequisite of payload parsing/adjudication. Recompute the ANS-104 signature preimage from the served item, verify it with the configured node key rather than a key supplied by the artifact, and reject owner, signature, or deep-hash mismatches before accepting any JSON or certificate. Keep the certificate-version fallbacks for historical certificates separate from envelope authentication: old certificate payloads may remain supported, but every newly claimed signed anchor still needs a valid node-authored ANS-104 wrapper. Add a positive trusted-envelope test plus rejection tests for a tampered signature, a foreign owner, and a forged envelope containing an otherwise valid copied certificate.

Review guidance

This PR has accumulated follow-up findings because it joins several security-sensitive state machines: Git ref visibility, durable database recovery, certificate-chain provenance, paid immutable uploads, and public verification. Each layer is individually careful, but correctness depends on the boundaries between them. In both findings, the code validates or persists a later layer after an earlier irreversible effect has already occurred: Git makes a ref durable before the recovery record exists, and the verifier trusts an inner certificate before authenticating the enclosing immutable artifact.

For the remaining revision, please drive the implementation from the end-to-end invariants rather than from individual error branches:

1. For every accepted ref transition, define the durable identifier and recovery record before that transition can be acknowledged or otherwise become visible; test crash/restart at each boundary through accounting, certificate issuance, and anchoring.
2. For every verifier verdict, establish the trust anchor outside the fetched artifact first, authenticate each enclosing format from outside-in, and only then interpret nested claims.
3. Exercise fault-injection and adversarial tests that remove or invert each boundary. A happy-path upload, a valid inner certificate, or a passing retry test is not sufficient proof that a crash or substituted remote artifact cannot bypass the contract.

Keeping those invariants explicit should reduce further review churn while preserving the intended scope: durable per-transition bookkeeping and trustworthy signed Arweave anchors, without broadening replication behavior or redesigning the certificate protocol.

[beardthelion]

I re-read head 04271ad against merged state. CI is green on this push. The R4/R5 work closes the signed-tuple legacy corroboration hole, binds verify to the served ANS-104 item id, requires Content-Digest before certificate proof, masks gateway credentials on the anchors listing, and adds real accept-path verify tests. Two verify/recovery gaps are still open.

Findings

• [P2] Verify the ANS-104 data-item signature before reporting valid
  crates/gitlawb-node/src/arweave.rs:705
  Upload signs ref anchors with the node keypair (build_ref_anchor_item, line 139) and the mock bundler enforces ans104::verify_data_item. verify_anchor only checks that data_item_id(body) == tx_id, unwraps the payload, and validates the embedded certificate. It never checks that the item's Ed25519 signature or owner matches node_did. The accept tests wrap payloads with serve_signed_anchor(..., &node_kp, ...), so they pass without exercising a foreign signer. Anyone can republish a legitimately node-signed certificate inside their own ANS-104 item and get valid: true on their tx id without this node having uploaded it.

• [P2] Cap the gateway presence probe body the same way verify caps its fetch
  crates/gitlawb-node/src/arweave.rs:507
verify_anchor streams the gateway response with a 1 MiB running cap (lines 661-690). anchor_item_present calls resp.bytes().await with no size limit before it parses the item id. Post-receive recovery uses that probe to decide whether a crashed upload landed before re-paying. A hostile or misconfigured gateway can force an unbounded allocation on the worker path while verify itself is bounded.

• [P3] Return 400 for a malformed ?repo= filter on the anchors listing
  crates/gitlawb-node/src/db/mod.rs:4748
  R3 added rejection for malformed repo filters, but the error is a bare anyhow! that propagates through the handler as 500 Internal. Map that shape to BadRequest at the handler boundary so clients get a 400, not an internal error.

Not an ask, recorded only: legacy migration rows with the all-zero default prev still skip chain linkage with a warn only; signed-tuple corroboration blocks seq tamper when a matching stored row exists. Encrypted-manifest anchoring on the replication tail still lacks the UploadOutcome / probe reconciliation the durable ref path has; the PR scope treats that tail as best-effort.

One process note, not a finding: this branch overlaps open PRs on auth/mod.rs (#306) and repos.rs (#285, #325). Base is current main on this head, but landing those first may still need a conflict pass on the shared hunks.