Skip to content

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

Closed
Gravirei wants to merge 32 commits into
Twigpine:mainfrom
Gravirei:fix/issue-26-harden-arweave-anchoring-verification
Closed

Gravirei wants to merge 32 commits into
Twigpine:mainfrom
Gravirei:fix/issue-26-harden-arweave-anchoring-verification

Conversation

@Gravirei

@Gravirei Gravirei commented Jul 20, 2026 •

Copy link
Copy Markdown
Contributor

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 AI review requested due to automatic review settings July 20, 2026 09:44

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@coderabbitai

coderabbitai Bot commented Jul 20, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

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.

@beardthelion beardthelion added crate:node gitlawb-node — the serving node and REST API kind:bug Defect fix — wrong or unsafe behavior subsystem:identity DID/UCAN, http-sig auth, push authorization subsystem:storage Blob/object store, Arweave, IPFS, archives labels Jul 20, 2026
@Gravirei
Gravirei force-pushed the fix/issue-26-harden-arweave-anchoring-verification branch from 0801800 to bd09c35 Compare July 20, 2026 09:50

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread crates/gitlawb-node/src/arweave.rs Resolved
Comment thread crates/gitlawb-node/src/arweave.rs Resolved

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@Gravirei please rebase to main and fix conflicts

@Gravirei
Gravirei force-pushed the fix/issue-26-harden-arweave-anchoring-verification branch from c94e8ed to ae4f5fc Compare July 22, 2026 16:28

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread crates/gitlawb-node/src/arweave.rs Resolved
Comment thread crates/gitlawb-node/src/arweave.rs Resolved
Comment thread crates/gitlawb-node/src/db/mod.rs Resolved
Comment thread crates/gitlawb-node/src/db/mod.rs Resolved

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Gravirei
Gravirei force-pushed the fix/issue-26-harden-arweave-anchoring-verification branch from ae4f5fc to e9a1a5e Compare July 23, 2026 04:39
@Gravirei
Gravirei requested a review from jatmn July 23, 2026 05:29
@kevincodex1

Copy link
Copy Markdown
Member

@coderabbitai please make an intensive review of this changes

@coderabbitai

coderabbitai Bot commented Jul 23, 2026 •

Copy link
Copy Markdown
Contributor

@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 coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread crates/gitlawb-node/src/api/certs.rs Resolved
@kevincodex1

Copy link
Copy Markdown
Member

@coderabbitai make an intensive review on this please

@coderabbitai

coderabbitai Bot commented Jul 23, 2026 •

Copy link
Copy Markdown
Contributor

@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 coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ 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

@Gravirei Gravirei closed this Jul 23, 2026
@Gravirei Gravirei reopened this Jul 23, 2026

@beardthelion beardthelion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Gravirei
Gravirei requested review from beardthelion and jatmn July 24, 2026 07:22

@beardthelion beardthelion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Gravirei
Gravirei requested a review from beardthelion July 24, 2026 13:47

@beardthelion beardthelion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Remove stale upstream code blocks concatenated by rebase conflict
resolution (inline record_push/issue_ref_certificate, duplicate
guard.release/result handling, dead standalone sweep_rate_limiters).
Fix EncryptTaskCtx missing fields, repo_store local_path visibility,
git_receive_pack test arg order, and f2a_tail missing ref_certs arg.
@Gravirei
Gravirei force-pushed the fix/issue-26-harden-arweave-anchoring-verification branch from 0b7a4f0 to aa9fc8b Compare August 26, 2026 20:39
@Gravirei
Gravirei requested review from beardthelion and jatmn August 26, 2026 20:41

@beardthelion beardthelion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@beardthelion
beardthelion dismissed their stale review August 26, 2026 21:02

Superseded by re-review on aa9fc8b

- Restore v19 (pinned_cids_repo_provenance) dropped by rebase concat
- Remove stale cert-chain stmts from v26 (belongs in v32 only)
- Add ESCAPE '!' to list_ref_certificates_by_prefix LIKE query
- Add arweave_rate_limiter to AppState sweep
- Move post-receive job spawn above guard.release for disconnect safety
- Add PusherSignature/PusherProof to signed_request_as test helper

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 beardthelion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Gravirei and others added 2 commits August 28, 2026 15:18
…velope auth, gateway probe cap, ?repo= 400

Twigpine#26 review found four gaps in the durable Arweave anchoring path.
Two reviewers in two rounds; this commit closes every one.

**P1 durability: pending_ref_transitions outbox + startup recovery**

A push that crashed between the bare-repo ref advancing and the
post_receive_jobs row landing left an accepted ref with no recovery
record. The handler now writes one row to pending_ref_transitions
per ref-update on the same success path that lands the ref, BEFORE
calling enqueue_post_receive_job. On startup,
recover_orphan_ref_transitions (called from drain_post_receive_jobs
BEFORE the existing reset/drain pass) derives a deterministic-id
post-receive job per orphan. The deterministic id makes a second
recovery a no-op via the existing ON CONFLICT (id) DO NOTHING
enqueue; the anchor claim layer is unique on (repo, ref_name,
old_sha, new_sha), so no duplicate paid anchor.

The recovery uses PostReceiveAttestation::default() (all None) so
the resulting cert is 7-field legacy form. The original RFC 9421
proof fields cannot be regenerated from on-disk state. Such certs
are accepted by the verifier's 7-field fallback and chained to
their predecessor via the per-repo_id seq stored at issuance. The
residual is documented in the docstrings of enqueue_post_receive_job,
recover_orphan_ref_transitions, and verify_anchor (durable doc per
team memory).

Migration v33 (pending_ref_transitions) is additive. v32 remains
the last migration on this branch.

**P1/P2 envelope authentication: ANS-104 signature gate in verify_anchor**

verify_anchor now takes node_vk: &ed25519_dalek::VerifyingKey and
authenticates the served body against it before parsing the
embedded cert. A cert copied into a foreign envelope (signed by a
third party whose data_item_id still matches the requested tx_id
because the id derives from the signature region) used to return
valid: true; it now returns valid: false with the same
"refuses to adjudicate" shape the existing id-mismatch and
bare-JSON paths use. Legacy 7/13/14-field certs already on the
network continue to verify under their respective payload forms —
the envelope check is the gate for newly claimed signed anchors
only.

verify_data_item and the deserialize_tags / read_string / read_long
helpers it calls are no longer cfg(test); they were production-grade
helpers with a historical test-only gate. deserialize_tags now
short-circuits on an empty buffer (the production bug the test
gate was hiding).

**P2 gateway probe cap: read_capped_body helper**

anchor_item_present used to call resp.bytes().await with no size
cap while verify_anchor already streamed with a 1 MiB cap. A
hostile gateway could drive unbounded allocation on the recovery
path. read_capped_body is a private helper that returns the body
or a credential-safe error text within a per-call limit.
verify_anchor keeps the existing 1 MiB cap and on overflow
returns the existing populated VerifyResult shape.
anchor_item_present uses 65_536 and on overflow returns Ok(false) —
a 2xx with a body too large to be a real ANS-104 item cannot be a
real data item, and the durable job must re-upload rather than
fail-closed.

**P3 handler error mapping: 400 not 500 for ?repo=**

list_anchors now validates the same owner/name shape the DB layer
rejects and returns AppError::BadRequest (400). The DB-layer
rejection stays as defense-in-depth so any other internal caller
cannot accidentally turn a malformed filter into an unfiltered
listing.

**Tests (8 new)**

- test_verify_anchor_envelope_matching_node_signer_adjudicates
- test_verify_anchor_envelope_with_tampered_signature_is_invalid
- test_verify_anchor_envelope_signed_by_foreign_key_is_invalid
- test_verify_anchor_envelope_with_copied_cert_claiming_foreign_node_did_is_invalid
- probe_caps_oversized_response_body
- list_anchors_malformed_repo_returns_400
- post_receive_job_survives_persist_failure
- post_receive_recovery_is_idempotent_under_double_drain

All 1160 unit tests + 7 integration tests pass. cargo clippy
--all-targets -D warnings clean.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Gravirei
Gravirei requested review from beardthelion and jatmn August 28, 2026 09:29

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR has grown beyond a reviewable and safely mergeable scope. Please stop revising it as one branch. Split the work into the independent PRs described below and treat this PR as superseded; another monolithic follow-up will recreate the same review and integration churn.

Required next step: split the PR

Create four narrow, self-contained PRs:

  1. Durable post-receive lifecycle

    Own only the Git transition intent/outbox, authentic pusher and RFC 9421 proof persistence, restart drain, push accounting, certificate issuance, and anchor handoff. This PR must close the pre-outbox crash boundary and prove exactly-once recovery. Do not include ANS-104 format work, public gateway/API changes, policy documentation, or unrelated migrations.

  2. Arweave transport and verification

    Own only ANS-104 serialization and verification, bundler/gateway request-response binding, recovery outcome classification, URL and credential handling, and the public verification endpoint. Model recovery as three outcomes—present, definitively absent, and indeterminate—and permit another paid upload only for a trustworthy, protocol-defined absence.

  3. Certificate, API, and CLI compatibility

    Own only certificate payload versions, API representation, CLI display and verification, and legacy compatibility fixtures. New fields must be optional for readers, and both old-server/new-client and new-server/old-client combinations must remain safe. None of the three current findings is assigned to this PR; it is still a required split because this protocol/client work is independently reviewable and should not remain coupled to post-receive recovery.

  4. Configuration and operator policy

    Own only configuration validation/default tests and the corresponding .env.example, README, and SECURITY.md changes. This PR should make every operator-facing surface describe the tested runtime behavior consistently; it should not carry unrelated runtime changes.

Independence and overlap requirements

Every split PR must be safe to land on current main without any sibling PR:

  • It must compile, migrate, run, and pass its focused tests by itself. Do not require an unmerged branch, a coordinated same-day merge, hidden deployment steps, or manual database repair.
  • If it reads a new table or column, include the required append-only migration and upgrade test in that same PR. Do not edit released migrations. Reserve distinct migration versions so the final schema is the same regardless of which PR lands first.
  • If it changes a serialized payload or API response, version the boundary or use optional fields and compatibility fallbacks. Do not introduce a shared signature or wire-format change that requires another PR to land simultaneously.
  • State one owned invariant in the PR description and list the producer, persistence, restart/restore, consumer/effect, and cleanup/failure boundaries covered by its tests.

There is also active work on the same contracts. Before opening each split PR, explicitly state whether it supersedes, excludes, or composes with the following open PRs:

  • #134: authentication for the anchors route
  • #285: receive-pack/advisory-lock behavior
  • #306: signed requests and Content-Digest
  • #314: DID/Ed25519 identity validation
  • #324: node/libp2p identity handling
  • #325: authenticated ref-update ingestion
  • #382: replication and withheld-object pinning

These overlaps are semantic, not merely textual: they affect authorization, request provenance, receive-pack execution, identity, and replication. Choose one owner for each contract. Do not carry competing implementations into another broad conflict-resolution branch.

Findings the split PRs must address

  • [P1][Split PR 1] Persist authentic recovery state before a successful Git update can be lost
    crates/gitlawb-node/src/api/repos.rs:2258

    receive_pack can apply the ref before the handler reaches the first record_pending_ref_transition call at line 2341. If the process exits, the request is cancelled, or that DB write fails in this interval, neither pending_ref_transitions nor post_receive_jobs records the landed transition. Startup drain enumerates only those sources, so it cannot reconstruct the missing work. Returning an error cannot roll back the Git ref: the original push permanently lacks accounting, its per-ref certificate, and its required anchor.

    The partial fallback after a transition row exists does not preserve provenance either. Because that row stores no authenticated pusher or RFC 9421 proof, recovery substitutes did:key:recovered and an empty attestation, then emits a normal node-signed legacy certificate. That is not equivalent to recovering the original authenticated push.

    Address the irreversible-effect boundary itself. Before Git can leave a visible ref with no durable record, persist enough authentic intent to identify and recover the transition, or implement reliable startup reconciliation that can derive every landed-but-unaccounted transition without inventing its identity. Preserve exactly-once accounting, certificate issuance, and anchor payment across retries. A retry added after receive_pack does not close this gap.

    Required proof: inject failure after Git applies the ref but before the first transition/job write, restart the node, and show that the original transition produces exactly one push event, one certificate carrying the original pusher/proof, and at most one anchor upload. Also prove that a failed or cancelled receive-pack does not turn a prepared intent into completed accounting or anchoring.

  • [P1][Split PR 2] Do not treat gateway 400/410 responses as proof that an uncertain upload is absent
    crates/gitlawb-node/src/arweave.rs:546

    anchor_item_present maps BAD_REQUEST and GONE to Ok(false). anchor_ref_updates interprets false as permission to reclaim the row and upload again. Neither response proves that the previous paid item was never accepted: 410 can describe an artifact that existed but is no longer served, and 400 can be produced by gateway, proxy, routing, or request-processing failure.

    The concrete failure sequence is: the bundler accepts the item, the response or terminal DB write is lost, recovery probes the persisted item_id, the gateway returns 400 or 410, and the node pays for a second immutable item for the same transition. This contradicts the PR's fail-closed, no-double-payment recovery contract.

    Fix the classification at its source. Only a trustworthy, protocol-defined absence may authorize another upload. Transport failures, malformed responses, item-ID mismatches, 400, 410, and other ambiguous outcomes must keep the outbox non-terminal and retry the probe without paying.

    Required proof: for a persisted item_id, cover confirmed presence, confirmed absence, 400, 410, transport failure, malformed or oversized response, and a 2xx response bound to a different item. Assert that only confirmed absence sends another upload request.

  • [P2][Split PR 4] Make SECURITY.md state the actual owner-push default
    SECURITY.md:67

    SECURITY.md states twice that GITLAWB_ENFORCE_OWNER_PUSH defaults to false, while Config defaults it to true and the README and environment example agree with the runtime. The document therefore tells operators that any authenticated DID may push unless they enable the flag—the inverse of the live authorization policy. This PR also adds the second incorrect limitation while claiming the security policy was aligned with runtime.

    Use the tested configuration default as the source of truth. State that owner enforcement is enabled by default and describe GITLAWB_ENFORCE_OWNER_PUSH=false only as the explicit rolling-upgrade compatibility override. Consolidate the duplicate limitation sections.

    Required proof: add a small regression check that keeps CLI parsing, environment behavior, .env.example, README, and SECURITY.md aligned for security-sensitive defaults.

Why the current approach keeps producing follow-up findings

The remaining defects are not isolated local mistakes. This branch combines multiple irreversible and externally visible state machines—Git ref application, database recovery, signed certificate provenance, paid immutable uploads, gateway reads, public APIs, and operator security policy. The failures occur at the handoffs between them:

  • idempotence is expected to recover a transition that was never durably recorded;
  • a remote response is reduced to a boolean even when its semantics are uncertain;
  • tests start from already-seeded recovery state instead of failing at the irreversible boundary;
  • runtime defaults and operator documentation have separate sources of truth;
  • parallel security PRs modify the same contracts while this branch continues to absorb conflict resolutions.

The split above is intended to make each invariant finite and reviewable. For each new PR, include a compact state-transition table or failure matrix and make the tests load-bearing: removing the durable intent, changing an indeterminate probe result to “absent,” or restoring the incorrect security default must make the corresponding test fail. That evidence will show the lifecycle is closed instead of demonstrating only that the latest reported branch was patched.

@beardthelion
beardthelion dismissed stale reviews from themself August 28, 2026 15:34

Superseded by re-review at 193cfa5

@beardthelion beardthelion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed head 193cfa5f. CI is 12/12 green. I ran anchor_record_failure_is_reconciled_without_double_pay (pass) and gutted ans104::verify_data_item in verify_anchor (foreign-envelope test goes RED). Jatmn's new round on this head asks for a four-PR split; I agree. This branch still couples Git ref application, recovery outbox, paid immutable uploads, and public verify in one diff, and the defects below sit at those handoffs.

Findings

  • [P1] Persist authentic transition intent before receive_pack can leave a durable ref

crates/gitlawb-node/src/api/repos.rs:2258

smart_http::receive_pack runs at line 2258; the first record_pending_ref_transition call is at line 2341, after Git may have advanced the ref on disk. A crash, cancellation, or DB failure in that window leaves no outbox row and no post_receive_jobs row, while startup drain only replays those sources. The recovery fallback that invents did:key:recovered does not restore the original pusher or RFC 9421 proof. Persist enough authentic intent before the irreversible Git effect, or reconcile every landed-but-unaccounted transition without fabricating identity.

  • [P1] Do not treat gateway 400/410 as proof the probed item is absent

crates/gitlawb-node/src/arweave.rs:546

anchor_item_present maps BAD_REQUEST and GONE to Ok(false). Recovery then reclaims the outbox row and pays for another upload. Neither status proves the prior paid item never landed: 410 can mean "no longer served" and 400 can be routing or proxy failure. Only a trustworthy, protocol-defined absence should authorize another upload; ambiguous outcomes must keep the row non-terminal and retry the probe without paying.

  • [P2] Fix SECURITY.md to match the runtime owner-push default

SECURITY.md:67

SECURITY.md states twice that GITLAWB_ENFORCE_OWNER_PUSH defaults to false. Config sets default_value_t = true at config.rs:98, and README and .env.example match the runtime. Operators reading SECURITY.md will think any authenticated DID may push until they opt in, which is the inverse of production behavior.

  • [P2] Stop logging bundler_token on successful uploads

crates/gitlawb-node/src/arweave.rs:286

Success paths log bundler_token = %bundler_token at lines 286 and 412. That value is the Irys payment-token slug billed at /tx/{token}, not a display label. Error paths redact credentials; info-level success logs would still park a live payment credential in centralized tracing.

One process note, not a finding: split PRs need explicit composition with open #134, #285, #306, #314, #325, and #382 on the same contracts (auth, receive-pack, identity, replication). This branch overlaps those files; do not resolve conflicts by absorbing competing implementations into another monolithic branch.

Not an ask, recorded only: encrypted-manifest anchoring on the replication tail still lacks the ref-anchor outbox and probe reconciliation; the PR treats that path as best-effort.

@Gravirei

Copy link
Copy Markdown
Contributor Author

Closing in favor of a four-way split. Review feedback was that the branch had grown beyond a reviewable scope: durable post-receive lifecycle, Arweave transport/verification, certificate/CLI compatibility, and operator policy were all coupled together, and that's why each round kept producing new findings at the handoffs.

Replacing this PR with four narrow, independently mergeable splits, each from main and each safe to land on its own:

  1. Durable post-receive lifecycle — Git transition intent/outbox, authentic pusher + RFC 9421 proof persistence, restart drain, push accounting, certificate issuance, anchor handoff. Closes the pre-outbox crash window and proves exactly-once recovery.
  2. Arweave transport and verification — ANS-104 serialization/verification, bundler/gateway req-resp binding, recovery outcome classification (present / definitively absent / indeterminate), public verification endpoint. Only confirmed absence re-uploads.
  3. Certificate, API, and CLI compatibility — certificate payload versions, API representation, CLI display + verification, legacy compatibility fixtures. Optional fields for old readers; both old/new combinations remain safe.
  4. Configuration and operator policy — config validation/defaults tests, .env.example / README / SECURITY.md alignment. The owner-push default mismatch is fixed here.

Tracking the new branches in the team's issue-26 thread. Thanks for the detailed review — splitting is the right call.

@Gravirei Gravirei closed this Aug 28, 2026
euxarist pushed a commit to euxarist/node that referenced this pull request Nov 6, 2026
… 2/4) (Twigpine#524)

* fix(node): ANS-104 transport, three-outcome probe, verify endpoint (Twigpine#26 split 2/4)

Reviewer 2 closed PR Twigpine#224 on 2026-08-28 with a directive: split
into four narrow PRs. This is Split PR 2 (Arweave transport and
verification).

The P1 finding the reviewer assigned to this split: the legacy
`anchor_item_present` mapped `BAD_REQUEST` and `GONE` to `Ok(false)`,
and the recovery code interpreted false as permission to reclaim the
row and pay for another upload. Neither 400 nor 410 proves a
previously paid item was never accepted: 410 can describe an
artifact that existed but is no longer served, and 400 can be
produced by gateway, proxy, or routing failure. The concrete
failure sequence the reviewer named: bundler accepts the item,
the response or terminal DB write is lost, recovery probes the
persisted item_id, the gateway returns 400 or 410, and the node
pays for a second immutable item for the same transition.

Fix: model the probe result as three outcomes, and authorize a
re-upload only on a trustworthy, protocol-defined absence.

NEW MODULE crates/gitlawb-node/src/ans104.rs
  - DataItem: the on-wire ANS-104 shape (signature, owner, target,
    anchor, tags, data — all base64url-encoded except target/anchor).
  - deep_hash: the canonical signing input per the spec. The
    signature is over a SHA-256 mix of the dataitem/list/map
    discriminators, the signature type, owner, target, anchor, the
    recursively-hashed tag list, and the data payload. A
    hand-rolled sha256 of the JSON body would produce a hash no
    Arweave gateway would recognize; the deep-hash is the only
    correct form.
  - sign_data_item: Ed25519 over deep_hash.
  - verify_data_item: parses the owner, checks the signature against
    an expected pubkey, and reports a specific failure reason on
    each branch.
  - 7 unit tests covering round-trip, flipped signature, wrong
    expected key, mutated data, wire-shape round-trip, deep-hash
    stability, and the empty-tags edge case.

NEW MODULE crates/gitlawb-node/src/arweave_v2.rs
  - ProbeOutcome: Present (2xx, body parses as ANS-104, sig
    verifies), DefinitivelyAbsent (404 with a known JSON body
    shape), Indeterminate (everything else). The reviewer's
    three-outcome model.
  - ProbeOutcome::permits_reupload: the recovery policy. ONLY
    DefinitivelyAbsent authorizes a paid re-upload. Indeterminate
    keeps the outbox non-terminal; Present skips re-payment.
  - probe_anchor_item: the gateway probe. 2xx bound to a different
    item id is Indeterminate, not Present. 2xx with a body that
    does not parse as ANS-104 is Indeterminate. Oversized bodies
    are Indeterminate (defense against body-stuffing).
  - read_capped_body: enforces PROBE_MAX_BODY_BYTES (1 MiB) on the
    full body, with a Content-Length fast-path that rejects
    before reading.
  - verify_anchor: the full path the public verify endpoint takes.
    Fetches the data item, parses it, verifies the Ed25519
    signature against the persisted node_did, decodes the data
    payload as JSON, and reports the result.
  - 15 unit tests pinning each classification boundary:
    400/410/5xx/transport/2xx-non-json/2xx-bad-sig/2xx-different-
    owner are all Indeterminate; 404 with empty or known-JSON
    body is DefinitivelyAbsent; 2xx with a valid signed item is
    Present; the re-upload policy is exhaustive.

NEW DB METHOD crates/gitlawb-node/src/db/mod.rs
  - get_arweave_anchor_by_item_id: looks up an existing
    arweave_anchors row by its id (which is the Irys tx id, the
    same value the Arweave gateway uses to serve the data item).
    Used by the public verify endpoint to fetch the persisted
    node_did so the envelope signature can be verified against it.

NEW CONFIG crates/gitlawb-node/src/config.rs
  - GITLAWB_ARWEAVE_GATEWAY_URL: the Arweave gateway the probe
    reads from. Defaults to https://arweave.net; operators can
    point it at a private mirror.

NEW HTTP ENDPOINT crates/gitlawb-node/src/api/arweave.rs
  - GET /api/v1/arweave/anchors/verify/{item_id}: the public
    verify endpoint. Returns {item_id, status, verified,
    owner_did, data_payload, error} where status is one of
    "verified" / "definitively_absent" / "indeterminate".
  - 3 endpoint-level tests covering the verified, 404, and 400
    shapes via the live router, asserting the public surface
    surfaces the three outcomes.

WIRE-UP crates/gitlawb-node/src/server.rs
  - The verify route is registered alongside the existing
    /api/v1/arweave/anchors list route.

NOT IN THIS SLICE (the bundler upload, the next commit):
  - The handler that builds a DataItem from a ref-cert and POSTs
    it to the bundler is not in this PR. The ANS-104 module is
    ready to be called by it; the cert/push-event flow in PR 1
    persists the row that the bundler upload would consume.
  - PR 1's recovery drain produces an `anchor_jobs` row keyed on
    the (repo, ref, old, new) tuple; the next slice reads that row,
    builds the ANS-104 data item, POSTs to the bundler, and writes
    the resulting tx id to arweave_anchors.

Compiles clean, 1113 tests pass with 0 regressions, clippy clean
under -D warnings, fmt clean.

Cross-PR overlap (declared in the PR description):

  - Twigpine#134 (anchors auth): composes. The verify endpoint reads from
    arweave_anchors; Twigpine#134's auth layer applies unchanged. The
    probe model is server-side and unauthenticated by design.
  - Twigpine#285 (advisory-lock session affinity): independent. The probe
    is read-only.
  - Twigpine#306 (Content-Digest on signed requests): independent. The
    probe does not sign requests.
  - Twigpine#314 (small-order Ed25519): composes. The verify path calls
    Did::to_verifying_key which already enforces the small-order
    check from Twigpine#314.
  - Twigpine#324 (libp2p keypair persistence): independent.
  - Twigpine#325 (gossip ref-update auth): independent. The verify path
    is for the public Arweave gateway, not gossip.
  - Twigpine#382 (replication withheld-subtree trees): independent.

* fix(node): address reviewer findings on Twigpine#26 split 2/4 (transport)

- P1 (DB lookup): get_arweave_anchor_by_item_id now filters on
  irys_tx_id (the externally-routable item id) rather than the
  internal UUID primary key. The production writer stores a fresh
  UUID in `id` and the Irys response / ANS-104 derived id in
  `irys_tx_id`; the old filter would always 404. New
  integration tests exercise the production writer path with
  distinct values for the two columns.
- P1 (verify-gate): verify_anchor now gates on repo read via
  authorize_repo_read. The route is split in server.rs: the
  public list_anchors stays ungated (Twigpine#134 tracks that surface),
  the verify route gets optional_signature per the team memory
  `axum-layer-vs-merge-pitfall.md`. Denials on row-lookup, gate,
  and missing repo all collapse to the same opaque 404 so the
  public endpoint does not leak anchor-row existence.
- P1 (ANS-104 deep-hash): rewritten to the verified SHA-384
  recursive list/blob algorithm (matches `arbundles` JS
  reference). deep_hash returns [u8; 48]; signature is over the
  raw 48-byte digest. Three reference vectors from
  `Irys-xyz/arbundles/src/__tests__/deepHash.spec.ts` are
  bit-exact-asserted as the interop canary per the team memory
  `self-roundtrip-tests-do-not-prove-interop.md`.
- P1 (artifact-identity): v2 verify now derives the protocol id
  via DataItem::id() (base64url(SHA256(signature))) and requires
  equality with the requested URL item_id. A node key signs
  many data items, so a valid signature only proves who signed
  the response — not that the served item is the one the caller
  asked to verify. The team memory
  `verify-against-artifact-id-not-signer.md` is the policy.
- P1 (verifier can't read this head's writes): verify_anchor
  now accepts BOTH the v2 ANS-104 format and the v1 raw-JSON
  shape the live path on this branch actually writes. The v1
  path does a field-equality check on repo, ref_name, old_sha,
  new_sha, node_did against the persisted row (no signature in
  v1; Irys storage plus the JSON parse are the integrity
  guarantee).
- P2 (ProbeOutcome in result): AnchorVerifyResult now carries
  outcome: ProbeOutcome. The HTTP handler maps the structured
  field to the status string instead of parsing the human-
  readable error message — any future wording change in
  arweave_v2.rs can no longer silently reclassify a
  DefinitivelyAbsent as Indeterminate.
- P2 (empty 404 body): classify_404 returns Indeterminate for
  bodyless 404s. The team memory
  `distinguish-unknown-from-empty.md` is the policy: a
  bodyless 404 from a proxy is not a proof of absence, and the
  recovery policy (permits_reupload -> true on
  DefinitivelyAbsent) authorizes a paid, irreversible re-upload.
- P2 (streaming cap): read_capped_body no longer uses
  resp.bytes() (which buffers the full response before checking
  size). It now reads chunk-by-chunk and aborts the moment the
  cumulative byte count crosses the cap, so a chunked response
  without Content-Length cannot force unbounded allocation.

* fix(node): address three reviewer findings on Twigpine#26 split 2/4 (round 2)

* fix(node): address five reviewer findings on Twigpine#26 split 2/4 (round 3)

* fix(node): address reviewers round-4 on Twigpine#26 split 2/4 (interop)

* fix(node): address coderabbit on Twigpine#26 split 2/4 (raw tags, vint overflow, golden key)

* fix(node): enforce artifact-id in probe_item on Twigpine#26 split 2/4

* fix(node): require expected owner for probe Present on Twigpine#26 split 2/4

* fix(node): narrow Twigpine#26 split 2/4 to codec and policy types

* fix(node): silence dead-code for deferred slice API on Twigpine#26 split 2/4

* fix(node): doc and bounds-check cleanup in ans104 on Twigpine#26 split 2/4

* docs(node): point tests doc at Ed25519 golden script on Twigpine#26 split 2/4

* fix(node): strict owner lengths, sign-before-hash on Twigpine#26 split 2/4

* fix(node): keep ans104 tags and field widths consistent on hash and encode

DataItem keeps both the decoded `tags` and the original Avro `raw_tags`, both
public. After a parse, changing `tags` left deep_hash and verify_data_item
using the stale raw bytes, and to_binary wrote a header tag count that no
longer matched the payload, so the frame failed to parse back.

Take the tag block for hashing and encoding from one helper that decodes
`raw_tags` and requires it to equal `tags`, and fail closed otherwise. The
header count comes from the same validated pairs. Parsed multi-block and
size-prefixed items still re-encode byte-exact and keep their signatures.

deep_hash now also rejects a non-empty target or anchor that is not 32 bytes,
as to_binary already does, so sign_data_item fails instead of signing an item
that cannot be encoded. The legacy non-base64url anchor fallback is kept.

The module and signature-type docs now say binary parsing accepts sigtypes 1
to 4 while signing and verifying are Ed25519 only on this slice.

* build(scripts): declare the golden script's direct npm dependencies

ans104_golden_ed25519.mjs imports bs58, @noble/ed25519 and base64url, and
ans104_golden.mjs imports base64url, but package.json listed only arbundles, so
they resolved only through hoisting. Declare them at the versions already
locked, update the lockfile root entry (no resolved version or hash changes),
and note the regeneration command in the golden output file.

---------

Co-authored-by: Gravirei <gravirei@users.noreply.github.com>
cairn-intern pushed a commit to cairn-intern/node that referenced this pull request Nov 7, 2026
…e#537)

* docs: state that GITLAWB_ENFORCE_OWNER_PUSH defaults to true

PR Twigpine#330 flipped the code default to true and updated .env.example, the
README env-var table, the AWS guide, the macOS app, and docs/RUN-A-NODE.md,
but two prose mentions in operator-facing docs still describe the
opposite. SECURITY.md line 57 ("Repository write authorization defaults"
section) and README.md line 69 (Known limitations) both say the flag
"defaults to `false`", which contradicts the live runtime and the rest
of the same files. Reviewer 2 closed Twigpine#224 with this as the P2 finding
to fix in the config-and-policy split.

The correct text already exists in the README env-var table; the prose
restatements were missed. Bring the prose into line with the code.

- SECURITY.md: rewrite the section to lead with the default being true
  and describe `false` as the explicit rolling-upgrade override.
- README.md: rewrite the known-limitations bullet to state the default
  is true and the `false` opt-out is a rolling-upgrade only.
- Add `crates/gitlawb-node/tests/owner_push_default_docs.rs`: a
  regression guard that reads the four surfaces (clap default, .env.example,
  README known-limitations prose, README env-var table, SECURITY.md) and
  asserts each one says the default is `true`. Reverting either doc edit
  turns one named test red; flipping `default_value_t` back to `false`
  turns the env example test red. Each surface is checked independently
  so a stale doc lies alone.

No code change. No new migration. No dependency change. Compiles clean,
clippy clean under `-D warnings`.

Refs: closes the P2 finding from the review of Twigpine#224.

* fix(docs): clarify default value consistency for GITLAWB_ENFORCE_OWNER_PUSH

* docs(readme): drop the default-open roadmap line and the em-dash punctuation

---------

Co-authored-by: Gravirei <gravirei@users.noreply.github.com>
Ayush7614 pushed a commit to Ayush7614/node that referenced this pull request Nov 10, 2026
…plit 3/4) (Twigpine#548)

* fix(node): version ref_certificates for forward compat (Twigpine#26 split 3/4)

Reviewer 2 closed PR Twigpine#224 on 2026-08-28 with a directive: split
into four narrow PRs. This is Split PR 3 (certificate / CLI
compatibility). No findings are assigned to it; it is required
because the protocol/client work is independently reviewable and
should not remain coupled to post-receive recovery.

The wire-format problem: the ref-cert payload is a fixed 7-field
JSON blob the node signs. Future changes (additional pusher
attestations, content-binding fields, secondary signatures) would
either break every existing cert or be impossible to ship without
a flag day. Versioning the cert makes the format forward-compat
and explicit.

NEW MIGRATION v28
  ref_certificates gains `version INTEGER NOT NULL DEFAULT 1`.
  DEFAULT 1 means an existing row reads as v1 with no backfill
  needed; NOT NULL means a missing value is a hard error rather
  than a silent v0.

NEW FIELD ON RefCertificate
  `version: u32`. v1 is the pre-versioning 7-field payload
  (no `version` key in the signed JSON, so a v1 cert is
  byte-for-byte identical to today's cert). v2+ certs will
  include a `version` key and any new fields, both
  backwards-compatible: an old client reading a v2 cert sees the
  unknown `version` field and either ignores it (graceful) or
  refuses to verify (explicit).

API CHANGES
  - list_certs and get_cert now include `version` in the JSON
    response.
  - issue_ref_certificate sets `version: 1` on every new cert.

GL CHANGES
  - gl cert show reads the `version` field; missing field
    defaults to 1 (forward-compat with old servers). v1 certs
    go through the unchanged verify path. v2+ certs are
    explicitly rejected with an "upgrade the client" message
    rather than silently guessing the payload shape — a future
    client that supports v2 cannot accidentally treat a v1 cert
    as v2.

GOLDEN-FORMAT FIXTURE
  v1_payload_matches_frozen_canonical_form pins the v1 signing
  payload byte-for-byte. Reverting this to a v2 shape (adding a
  `version` key) is what the versioned format forbids: an old
  client reading a v1 cert must not see a `version` key in the
  signed JSON. The same byte-form is what gl's verify_signature
  reconstructs in crates/gl/src/cert.rs, so any drift between
  node and CLI breaks the test instead of silently rendering
  every real cert INVALID.

TESTS
  - 3 v1_payload_tests in crates/gitlawb-node/src/cert.rs:
    frozen-canonical-form, round-trip structural, default
    version is 1.
  - 2 new tests in crates/gl/src/cert.rs:
    missing_version_defaults_to_1_and_verifies,
    explicit_version_1_takes_v1_path.
  - 12 existing ref_certificate_tests still pass with the
    version column added.

Compiles clean, 1091 tests pass with 0 regressions, gl 4 cert
tests pass, clippy clean under -D warnings, fmt clean.

Cross-PR overlap (declared in the PR description):

  - Twigpine#134 (anchors auth): independent. The cert API is unchanged
    in shape; auth gates apply as before.
  - Twigpine#285 (advisory-lock session affinity): independent.
  - Twigpine#306 (Content-Digest on signed requests): independent.
  - Twigpine#314 (small-order Ed25519): independent. v1 certs verify
    through the same path, which already enforces the small-
    order check from Twigpine#314 on the embedded node DID.
  - Twigpine#324 (libp2p keypair persistence): independent.
  - Twigpine#325 (gossip ref-update auth): independent. PR 1's signed
    HTTP push is the durable-intent producer; this PR owns the
    cert shape. The two PRs read and write the same cert table
    but do not step on each other (PR 1 calls
    issue_ref_certificate_idempotent; this PR keeps the
    deterministic cert id but does not change who calls what).
  - Twigpine#382 (replication withheld-subtree trees): independent.

* fix(node,gl): address review feedback on cert version compat

- Drop the duplicate `version` ALTER from the v1 migration bundle; v28
  is now the sole owner of the column.
- Add a v28 upgrade-path test (v25/v26 pattern) that seeds a legacy
  pre-v28 ref_certificates row, drops the column + migration record,
  re-runs migrations, and asserts DEFAULT 1 + legacy row reads as v1.
- Extract a shared `v1_signing_payload` builder so the live signer
  and the frozen-vector test share one literal; a regression in the
  signer now fails the test.
- Tighten gl's `version` parsing: missing key → v1, present value
  must be a JSON integer that fits in u32 and equals 1. Null, strings,
  floats, overflow, and unknown versions all return Err instead of
  collapsing to v1 (Reviewer 2). Add a truth-table test pinning every
  cell of the parser.
- Extend the v1 forward-compat tests to actually round-trip through
  `verify_signature`, and add a regression test that pins the verdict
  branch rejects v2 even when the v1 signature would otherwise verify.

* fix(cert): address CI failures on PR Twigpine#386

- cargo fmt --all (CI fmt check was red on the prior commit)
- v28 upgrade-path test: switch version_column_default from
  fetch_one to fetch_optional, so a pre-v28 database with the
  column dropped returns Ok(None) rather than RowNotFound. The
  precondition assertion is what detects a missing column;
  RowNotFound would have masked a regression.
- clippy `needless_borrow` on the v2 verdict-branch test:
  drop the leading `&` on `cert_json["signature"].as_str()`.

* fix(gl,node): make the v2 rejection and issuer anchor real, and test them as such

Round-2 findings, all three demonstrated by execution:

- cmd_show's Ok(v != 1) verdict arm was dead code (parse_cert_version
  admits only 1), and the test aimed at it copied the match instead of
  calling the command. The match now mirrors the parser's contract and
  two end-to-end mockito tests drive cmd_show itself: a version-2 cert
  with a valid v1 signature fails --verify with the version named.

- --verify no longer accepts the queried node's self-reported DID as a
  trust anchor: the node that served a forged cert can serve a matching
  self-report, and that fallback passed a wholly forged certificate with
  exit 0. --verify now requires --expect-node before claiming a trusted
  issuer, with a test proving the refusal (and the anchored positive
  control passing).

- The node-side version test asserted version == 1 on its own literal;
  it now cross-checks the issuing construction: a version-1 claim must
  verify against the v1 payload builder's bytes rebuilt from the cert's
  own fields, and the v1 payload must carry no version key.

* fix(node,gl): address round-2 reviewer findings on Twigpine#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.

* fix(node): bind cert v2 test to live issuer via sqlx::test

* fix(node,gl): address round-3 reviewer findings on Twigpine#26 split 3 cert compat

- Stamp new certs v1 until v2 verifier ships; v2 issuance refused by shipped gl
- Add v2_signing_payload with version in signed bytes, downgrade test
- Verify signature in well-formed test, add DB read-back to issuer test
- Fix README/db comments and version-refusal message

* fix(gl): route cert list through error_for_status; correct cert show example

Round 4 reviewer findings on Twigpine#26 split 3 cert compat:

- P2 (cert.rs:113): cmd_list called .json() straight through on the
  cert list endpoint and let a gated 404 (whose body lacks a
  `certificates` key) fall into the empty-list branch, printing "No
  ref certificates" with exit 0. Wire cmd_list through the same
  error_for_status() gate that cmd_show and resolve_cert_id already
  use, and pin the new behavior with a load-bearing test
  (`cmd_list_surfaces_gated_404_as_err`) that drives cmd_list end
  to end against a 404 mock. cmd_show is unchanged.

- P3 (README.md:314): the documented `gl cert show <id> --verify`
  example bound the certificate ID as the REPO positional, because
  the parser is `gl cert show [OPTIONS] <REPO> <ID>`. Insert the
  missing `<repo>` positional so the example matches the CLI
  parser.

The original BREAKING CHANGE footer in bb553b6 carried the same
shortened `gl cert show <id> --verify` example; this commit's
footer supersedes it with the canonical command, which is what
release-note scrapers read. The round-2 commit message itself is
left in history unchanged; the new footer is the one callers
should rely on.

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

* style(gl): fix fmt in cert list response parsing

* fix(gl): label version refusals distinctly and clean cert show help

A cert declaring an unsupported version printed INVALID, which reads as
a failed Ed25519 check even though verify_signature never ran. The
display now prints UNSUPPORTED VERSION for version refusals and keeps
INVALID for real signature failures. The Show help no longer cites
review rounds or commit history.

* docs(node): align RefCertificate.version rustdoc with the signed-bytes v2 contract

---------

Co-authored-by: Gravirei <gravirei@users.noreply.github.com>
Co-authored-by: Kevin Codex <kevin@gitlawb.com>
Ayush7614 pushed a commit to Ayush7614/node that referenced this pull request Nov 10, 2026
…split 1/4) (Twigpine#558)

* fix(node): durable post-receive outbox at the DB layer (Twigpine#26 split 1/4)

Reviewer 2 closed PR Twigpine#224 on 2026-08-28 with a directive: split the
work into four narrow PRs. This is Split PR 1 (durable post-receive
lifecycle) at the DB layer; the handler refactor in
crates/gitlawb-node/src/api/repos.rs:2007 (git_receive_pack) lands in
the next slice so the test can drive the failure injection end-to-end.

The pre-outbox crash window the reviewer flagged: receive_pack can
apply a ref to disk and return Ok, and a process exit, a dropped
future, or a DB failure before the bookkeeping at
crates/gitlawb-node/src/api/repos.rs:2361 (push event + cert + webhook)
loses the recovery record. Startup drain enumerates only sources
written from that bookkeeping, so it cannot reconstruct the missing
work. The partial fallback that re-derives from a row present in the
bookkeeping substitutes did:key:recovered and an empty attestation,
which is not equivalent to the original authenticated push.

This commit adds the durable boundary the handler will lean on.

NEW TABLE pending_ref_transitions (migration v27):
  - Written by the handler BEFORE smart_http::receive_pack, in state
    'prepared', carrying the verified pusher DID, the raw RFC 9421
    signature header, signature-input, and content-digest that
    authorized the push, the request id, and the parsed ref update.
  - The handler transitions the row to 'applied' on receive_pack Ok
    or 'cancelled' on Err. The drain reads only 'applied'.
  - A failed or cancelled receive-pack therefore leaves the row in
    'prepared' or 'cancelled', which the drain never promotes. This
    is what closes the reviewer's second proof ("a failed or cancelled
    receive-pack does not turn a prepared intent into completed
    accounting or anchoring").

NEW TABLE anchor_jobs (migration v27, owned by PR 1, consumed by PR 2):
  - One row per (repo_id, ref_name, old_sha, new_sha) transition.
    PR 1 inserts it on 'applied'; PR 2 reads it and updates claimed_at.
  - ON CONFLICT (id) DO NOTHING makes the insert idempotent on the
    deterministic id, so a recovery re-pass cannot create a second
    upload request. This is the handoff boundary; the bundler call
    itself is PR 2.

NEW DB METHODS on Db:
  - insert_pending_ref_transitions: writes one 'prepared' row per
    ref update, returns the persisted rows.
  - mark_pending_ref_transitions_applied / _cancelled: state flip,
    gated on the FROM state, idempotent.
  - list_pending_ref_transitions_applied: drain query, oldest first.
  - delete_pending_ref_transition: called by recovery after the
    artifacts land; a third pass is a no-op.
  - record_push_with_id: ON CONFLICT (id) DO NOTHING on the
    deterministic id.
  - insert_ref_certificate_idempotent: ON CONFLICT (repo_id, ref_name)
    DO NOTHING, returns None if a live-path cert already exists.
  - insert_anchor_job_idempotent: ON CONFLICT (id) DO NOTHING on the
    deterministic per-transition id.

NEW HELPERS in db/mod.rs:
  - deterministic_id: SHA-256 hex with an ASCII Unit Separator
    between fields so two distinct tuples never collide on prefix
    overlap.
  - push_event_id_for, ref_cert_id_for, anchor_job_id_for: the
    derived ids above, one helper per artifact so a caller cannot
    derive a wrong id by mistake.

NEW STRUCTS:
  - PendingRefTransition: the row shape.
  - AnchorJob: the handoff row shape.
  - pending_state: const strings ('prepared' / 'applied' /
    'cancelled') shared by tests, the producer, and the drain so a
    typo on one side cannot silently mismatch the other.

NEW TESTS in db::pending_ref_transition_tests (8 tests, all green):
  - insert_then_mark_applied_flips_state_for_every_ref: producer
    contract.
  - mark_applied_is_idempotent_on_repeat: re-fire is a no-op.
  - cancelled_rows_are_not_returned_by_the_drain: reviewer's second
    proof at the DB layer.
  - prepared_rows_are_not_returned_by_the_drain: same proof for the
    pre-flip state (handler crashed before reaching post-Ok).
  - mark_cancelled_is_idempotent_on_repeat: counterpart.
  - drain_then_re_derive_is_idempotent: reviewer's first proof at
    the DB layer. Inserts a row in 'applied' state directly via
    insert_pending_ref_transition_for_test, drains it, derives the
    artifact ids twice, exercises record_push_with_id and
    insert_anchor_job_idempotent directly, asserts exactly one push
    event row and exactly one anchor job row regardless of how many
    times the drain runs.
  - deterministic_id_avoids_prefix_overlap_collisions: the
    separator regression test.
  - push_event_id_for_is_stable: derived ids match across calls and
    differ on each varied input.

OTHER:
  - Make RefUpdate and its fields pub(crate) so the DB methods can
    iterate the parsed ref updates. No public API change.

NOT IN THIS SLICE (the handler refactor, next commit):
  - The receive-pack handler does not yet call insert_pending_ref_
    transitions before the receive_pack call, nor mark_applied /
    mark_cancelled after. The DB layer is in place for it; the
    handler will call these methods and the startup drain will be
    wired in main.rs.
  - The startup drain in main.rs is not yet called; it will iterate
    list_pending_ref_transitions_applied, re-derive the artifacts,
    and delete the row.
  - The cert/push event issuance in cert.rs and the bookkeeping in
    api/repos.rs:2361 are not yet changed to use the deterministic
    ids. The helper functions exist and are tested; the callers
    follow.

Compiles clean, clippy clean under -D warnings, fmt clean.

* fix(node): wire durable outbox into the receive-pack handler (Twigpine#26 split 1/4)

This is the handler-level half of Split PR 1. The previous commit
added the migration and the DB methods; this one threads them
through crates/gitlawb-node/src/api/repos.rs:2007 (git_receive_pack),
the cert issuer, and the startup drain.

CHANGES IN THE HANDLER
======================

In git_receive_pack, AT THE LAST POSSIBLE MOMENT before the
smart_http::receive_pack call, the handler now:

  1. Generates a per-handler request_id (UUID).
  2. Captures the raw Signature, Signature-Input, and Content-Digest
     headers from the request.
  3. Calls db.insert_pending_ref_transitions(request_id, ...) which
     writes one row per ref update in state 'prepared'.

The receive_pack call runs as before. After it returns:

  4. On Ok: db.mark_pending_ref_transitions_applied(request_id) —
     the row is the ONLY thing that promotes a 'prepared' row to
     'applied', and the drain reads only 'applied' rows. A process
     crash before this call leaves the row in 'prepared', which the
     drain never promotes.
  5. On Err: db.mark_pending_ref_transitions_cancelled(request_id) —
     a failed receive_pack leaves the row in 'cancelled', which the
     drain never promotes.

This is what closes the reviewer's two proofs:

  Proof 1 (crash window): if the process dies after
  mark_pending_ref_transitions_applied but before the bookkeeping
  writes, the row is in 'applied' and the next startup drain
  re-derives the push event, the per-ref certificate (carrying the
  ORIGINAL pusher DID, not a placeholder), and the anchor handoff.
  The drain uses the persisted authentic pusher DID and signature
  header, not a recovered placeholder.

  Proof 2 (failed receive-pack): the row is only ever flipped to
  'applied' in the explicit Ok branch above. A 'prepared' or
  'cancelled' row is invisible to the drain, so a failed or dropped
  receive_pack cannot turn a prepared intent into completed
  accounting or anchoring.

BOOKKEEPING IS NOW DETERMINISTIC-ID
===================================

The post-Ok bookkeeping at api/repos.rs:2448 now uses:

  - record_push_with_id with push_event_id_for(request_id, first_ref)
    — ON CONFLICT (id) DO NOTHING, so a recovery re-pass is a no-op.
  - issue_ref_certificate_idempotent with
    ref_cert_id_for(request_id, ref_name) — ON CONFLICT (repo_id,
    ref_name) DO NOTHING, returns None if a live-path cert already
    exists.
  - insert_anchor_job_idempotent with
    anchor_job_id_for(repo_id, ref_name, old_sha, new_sha) — the
    per-transition tuple key, so two pushes to the same ref produce
    one anchor upload per landed state.

The legacy entry points (record_push, issue_ref_certificate,
insert_ref_certificate) remain for callers that prefer a fresh UUID
per cert; they are #[allow(dead_code)] for the PR 3 cert/CLI compat
pass to decide whether to keep or remove.

STARTUP DRAIN
=============

crates/gitlawb-node/src/main.rs calls
durable_outbox::drain_pending_ref_transitions(state, 1000) ONCE
before serving, after migrations and after the existing peer /
quarantine prunes. Non-fatal: a transient drain failure logs and
leaves the rows for the next startup.

durable_outbox::drain_pending_ref_transitions reads every 'applied'
row, calls derive_one (which re-derives the three artifacts using the
persisted authentic pusher DID and signature header), then deletes
the row. A second drain pass is a no-op for both the artifacts
(idempotent inserts) and the row (gone after the first pass).

NEW END-TO-END TESTS
====================

crates/gitlawb-node/src/durable_outbox.rs adds three end-to-end
tests in drain_tests, complementing the eight DB-layer tests in
db::pending_ref_transition_tests:

  - drain_re_derives_all_three_artifacts_for_an_applied_row: the
    reviewer's first proof. Inserts a row in 'applied' state (the
    crash window), drains, asserts exactly one push event row,
    exactly one cert row carrying the original pusher DID (not a
    placeholder), and exactly one anchor job row. Asserts the
    deterministic cert id matches. Asserts a second drain pass is a
    no-op.
  - cancelled_row_produces_no_artifacts: the reviewer's second proof
    for the cancelled state. A row in 'cancelled' (receive_pack
    returned Err) is invisible to the drain.
  - prepared_row_produces_no_artifacts: the reviewer's second proof
    for the prepared state. A row in 'prepared' (handler crashed
    between insert_prepared and the post-Ok branch) is invisible to
    the drain.

Each test names the invariant it pins and the production line it
covers. Reverting that line turns the named assertion red.

Compiles clean, 1099 tests pass with 0 regressions, clippy clean
under -D warnings, fmt clean.

Cross-PR overlap (declared in the PR description):

  - Twigpine#134 (anchors auth): composes. The /arweave/anchors route
    already requires auth; this PR does not change the route.
  - Twigpine#285 (advisory-lock session affinity): composes. The durable
    intent is written inside the same handler that holds the lock
    from Twigpine#285; no changes to the lock layer.
  - Twigpine#306 (Content-Digest on signed requests): composes. PR 1
    persists the Content-Digest header that Twigpine#306 makes mandatory.
  - Twigpine#314 (small-order Ed25519): independent. PR 1's tests use strong
    keys.
  - Twigpine#324 (libp2p keypair persistence): independent. PR 1 does not
    touch p2p identity.
  - Twigpine#325 (gossip ref-update auth): independent. PR 1's signed
    envelope is the HTTP-side equivalent, not the gossip-side.
  - Twigpine#382 (replication withheld-subtree trees): independent. PR 1
    does not touch replication or pin selection.

* fix(node): address four reviewer findings on Twigpine#26 split 1/4

- P1-A: add startup reconcile step that promotes `prepared` rows to
  `applied` when the on-disk ref matches the row's `new_sha`. The
  recovery drain (which only reads `applied` rows) can now pick up
  a ref that landed when the live handler's
  `mark_pending_ref_transitions_applied` call errored or was
  interrupted. Strict SHA equality is the load-bearing check — a
  `prepared` row whose target did NOT actually land stays
  `prepared`.
- P1-B: route the live handler's cert issuance through
  `cert::issue_ref_certificate` (the upsert) instead of
  `issue_ref_certificate_idempotent` (DO NOTHING). A re-push to the
  same ref now updates the cert's `old_sha` / `new_sha` /
  `pusher_did` / `issued_at` / `signature` to the new transition
  while preserving the deterministic `cert_id`. The recovery drain
  keeps the idempotent variant; both paths collapse to one row.
- P2-A: refactor the drain into a `drain_pending_ref_transitions_with`
  testable seam that does per-row log-and-continue, and add
  `drain_pending_ref_transitions_all` that loops
  `DRAIN_PER_PASS_LIMIT=1000` rows for `DRAIN_MAX_PASSES=10` passes.
  A failing row no longer stalls the batch; a backlog above 1000
  rows is fully processed across passes.
- P2-B: add a `first_ref_name` column to `pending_ref_transitions`
  via migration v28. The live handler hoists a `first_ref_name`
  local and persists it on every row of the same `request_id`. The
  drain's `derive_one` keys the push event id on
  `row.first_ref_name` instead of `row.ref_name`, so live and
  recovery produce the same id and `ON CONFLICT (id) DO NOTHING`
  collapses a multi-ref push to one push event row (and one trust-
  score bump). Cert and anchor ids stay per-ref / per-transition.

* fix(node): address four reviewer findings on Twigpine#26 split 1/4 (round 2)

* fix(node): address reviewer round-3 findings on Twigpine#26 split 1/4

- P1: Delete outbox rows after live durable effects complete so they
  don't replay on every restart
- P1: Parse git report-status for per-ref ok/ng results; mark only
  proven rejections as cancelled, uncertain outcomes as recoverable
- P1: Introduce 'uncertain' state for receive-pack errors where some
  refs may have landed; reconcile checks these against disk at startup
- P2: Promote deletion transitions during reconcile (new_sha == ZERO_SHA
  with absent ref = successful deletion)
- P2: Loop reconcile across multiple passes so backlogs beyond the first
  page are processed in the same startup

Closes review round 3 findings from reviewer-1 and reviewer-2.

* fix(node): fix CI failures from round-3 changes

- Add COMMENT ON TABLE to v29 migration so migration_bodies_are_non_empty passes
- Return error on non-zero receive-pack exit (preserving backward compat
  with tests that expect Err(AppError::Git(_))) while still parsing
  report-status for outbox row handling

* fix(node): update inv22 gate tests for receive_pack_raw refactor

- F3 gate: scan for smart_http::receive_pack_raw( instead of receive_pack(
- U5 gate: scan for all_refs_ok instead of receive_result.is_ok()

* fix(node): require reflog proof before the reconcile promotes a row (Twigpine#26 split 1/4)

Closes the second half of the reviewer's round-3 [P1]: the startup
reconcile still promoted on `disk_refs.get(ref) == row.new_sha`, which
also matches a coincidental current tip (old=B, new=A while the ref is
already A). That is not an exotic case — it is the ordinary shape of a
REJECTED push, since git refuses an update whose expected old value does
not match the ref. Both the SHA check and the MAX_RECONCILE_AGE window
pass, the push never happened, and the drain then writes a push event, a
signed certificate and an anchor for it.

The live path proves landings from git's report-status body; that body is
gone by the time the reconcile runs, so the proof is re-derived from the
repository. `git::store::ref_reflog_entries` reads `logs/<ref>` and a row
is promoted only when an entry carries its exact `old -> new` pair,
stamped at or after the row was written (REFLOG_CLOCK_SKEW absorbs git's
whole-second truncation against the row's sub-second `created_at`). The
timestamp floor is what separates a landing from a later push that
re-introduced the same pair: proof must postdate the intent it proves.

Deletions stay exempt from the reflog half and keep the absence-plus-age
rule already in place — git removes a ref's reflog along with the ref, so
no other evidence can exist for one.

`init_bare` now sets `core.logAllRefUpdates`, which bare repos default
off; a repo with no reflog yields no proof and its rows stay put for
human-attended recovery rather than becoming accounting the node cannot
substantiate.

Tests: reconcile_refuses_a_coincidental_tip_with_no_reflog_proof,
reconcile_promotes_a_row_the_reflog_proves,
reconcile_leaves_a_row_prepared_when_the_repo_keeps_no_reflog,
reconcile_refuses_a_reflog_entry_older_than_the_row,
reconcile_still_promotes_a_landed_deletion_which_can_have_no_reflog,
init_bare_keeps_reflogs_so_a_landing_can_be_proven,
ref_reflog_entries_is_none_when_the_repo_kept_no_log.

No migrations added or renumbered.

* fix(node): walk the reconcile backlog on a keyset cursor (Twigpine#26 split 1/4)

The multi-pass reconcile looped but never advanced. Each pass re-issued
the same cursor-less `LIMIT n` query, and unlike the drain — which
deletes every row it finishes, so its next page is always new work — the
reconcile leaves every row it cannot promote exactly where it was. One
unprovable row at the head of the ordering therefore pinned page one on
every pass, and the backlog behind it was never examined at all, which
is the condition the multi-pass loop was added to remove. Rows keep
ageing toward MAX_RECONCILE_AGE while they wait, so "picked up next
restart" can mean "never recovered".

Pages now resume strictly after the `(created_at, id)` of the last row
EXAMINED, via `list_pending_ref_transitions_prepared_or_uncertain_after`.
A keyset cursor rather than an OFFSET because rows leave the set
underneath the walk when they are promoted: an absent row simply does
not appear on a later page, and it can never shift an unvisited row into
a page that was already read. Pass budget, per-pass limit, short-page
exit and the residual-backlog warning are unchanged.

This matters more with the reflog gate in place: a landing in a repo
that keeps no reflog is PERMANENTLY unprovable, so before this it would
jam page one on every startup rather than once.

Tests: reconcile_all_walks_the_backlog_past_the_first_page,
reconcile_all_advances_past_an_unprovable_row (whose blocker is a
no-reflog row, the class the gate introduces).

* fix(node): bound the reflog read to a recent tail (Twigpine#26 split 1/4)

`ref_reflog_entries` slurped the whole reflog. A reflog grows one line
(~150 bytes) per ref update and how often a ref is updated is
PUSHER-controlled, so that was an attacker-sized allocation, taken once
per stranded row, at startup — the moment the node can least absorb it.
Every other attacker-influenced read in this crate is bounded
(read_body_capped, MAX_SIGNATURE_ENTRIES, the pin/walk budgets); this one
should be too.

The bound costs nothing in proving power, which is why it is a ceiling
rather than a tradeoff: the landing gate only ever accepts an entry
stamped at or after `created_at - REFLOG_CLOCK_SKEW`, and reflogs are
append-ordered oldest-first, so every entry that could possibly qualify
is at the tail. The read now seeks to the last REFLOG_TAIL_BYTES (256KB,
~2000 recent updates to one ref) and `take`s that many bytes, so the
allocation stays bounded even if the file grows between the stat and the
read.

A window into the middle of a file starts mid-record, so the bytes before
its first newline are discarded: without that, a boundary landing inside
a record's old SHA leaves a suffix that still tokenizes and yields a
truncated SHA as a bogus `old -> new` pair — and landing proof is an
exact pair match, so a bogus pair is a fabricated proof. An entry older
than the window reads as absent, which is the safe direction: the caller
treats "no matching entry" as unproven and leaves the row where it is.
`Ok(None)` for a missing file, and the absence-of-evidence contract, are
unchanged.

Tests: ref_reflog_entries_reads_whole_records_from_the_recent_tail (lays
the file out so the boundary lands 20 bytes into a record's old SHA — the
alignment that would otherwise parse), an_entry_older_than_the_reflog_tail_reads_as_unproven.

* fix(node): read the report through both side-band frames

The report-status is framed twice whenever the client negotiates
side-band-64k, which `git push` over smart HTTP does: the outer
side-band envelope carries a stream that is itself pkt-line encoded.
One strip pass left the first line as `000eunpack ok`, so the unpack
check failed and parse_report_status returned None — read by the caller
as "no report", which keeps every declared ref. The per-ref landing
gate was therefore inert for exactly the pushes it exists to filter.

The regression test is a byte-for-byte capture from git 2.50.1
rejecting one ref of a two-ref push, because a hand-written
single-framed fixture parses under the old code and hides the case.

* fix(node): address round-4 reviewer findings on Twigpine#26 split 1/4

* fix(node): rustfmt round-4 reviewer fixes

* fix(node): remove empty line after doc comment

* fix(node): drop redundant reflog gate; implicit-ok on exit-zero no-report

* fix(node): address round-5 reviewer findings on Twigpine#26 split 1/4

* fix(db): v30 migration — add receive_pack_requests table and ordinal column

The reviewer's round-5 finding: the push event identity was encoded
into a mutable per-ref column (first_ref_name) whose correct value is
knowable only after git, and the correction was not committed
atomically with the per-ref outcomes. A crash between git updating
a later ref and the rewrite left the durable rows naming the
rejected ref, and derive_one's row.ref_name == row.first_ref_name
guard then meant no push event ever landed for the accepted child.

Per the state-transition model at
.gravirei/plans/state-model-durable-post-receive.md, this migration
introduces the request-level record that owns the push event and the
trust score, and extends the per-ref child with an ordinal column
the drain and the effect executor walk together. first_ref_name is
dropped; the push event id is keyed on
(request_id, accepted_ordinal) and lives on the request row.

The next commit (handler rewrite) wires the live path and the
recovery drain against the new tables.

BREAKING CHANGE: pending_ref_transitions.first_ref_name is dropped.
Callers that read or write that column must move to
receive_pack_requests.accepted_ordinal.

* fix(node): rewrite receive-pack handler against the request-level model (Twigpine#26 split 1/4 step 2)

The v30 migration added receive_pack_requests and dropped first_ref_name,
but no Rust code used either. The handler still ran a four-branch per-ref
state flip plus a first_ref_name rewrite after git returned, which is the
mixed-outcome bug a push with the first ref rejected exposes: the live
path and the drain computed different push-event ids.

- Insert a receive_pack_requests row in state `received` BEFORE git runs
  (insert_receive_pack_request), carrying the request bytes and the
  SHA-256 of the body. Crash between intent and git return is now
  recoverable via the reconcile step.
- Drop the first_ref_name rewrite at api/repos.rs:2454. The push event
  identity is now (request_id, accepted_ordinal); accepted_ordinal is
  computed once at the per-ref state flip.
- Re-key push_event_id_for and ref_cert_id_for on (request_id, ordinal).
  Anchor job id stays per-transition.
- Transition the request row to outcomes_committed (with parsed_report
  and accepted_ordinal) or rejected_at_git (no report + non-zero exit)
  in the same handler tail as the per-ref state flip. The drain (step 3)
  picks up outcomes_committed rows; today the live path also runs the
  per-ref effects inline.
- Stub the step-3 surface (mark_request_effects_pending, complete,
  list_receive_pack_requests_due, update_request_attempt) with
  #[allow(dead_code)] + contract-pin comments so the rewrite is
  bisectable: reverting this commit restores the pre-rewrite live path.

The U5 gate (replication tail spawn inside push_succeeded, before
guard.release) is preserved; inv22_replication_tail_spawns_at_the_
durability_boundary stays green.

* fix(node): use raw bytes for request_bytes_hash (Twigpine#26 split 1/4 step 2)

The v30 migration defined request_bytes_hash as BYTEA but the handler
was binding a hex-encoded String. Postgres rejected the insert with
"column request_bytes_hash is of type bytea but expression is of type
text" and the handler shed the push with a 503.

Switch the handler to bind the raw 32-byte digest and update the
ReceivePackRequest struct field type to Vec<u8> to match. The
drain-side test fixture already used Vec<u8> and needs no change.

Pinned by the bin test suite going from 24 failures (all the
receive-pack-cap tests that send a 4-byte body) to 0 regressions
on the new model.

* fix(node): factor out apply_request_effects; drain walks receive_pack_requests (Twigpine#26 split 1/4 step 3)

The step-2 commit moved the request row to be the unit of work but
left the per-ref effects fan-out inline in the handler and the
drain walking pending_ref_transitions per-ref. Step 3 factors the
effects fan-out into apply_request_effects(state, request_id) and
rewrites the drain to walk receive_pack_requests.

- apply_request_effects lives in durable_outbox.rs; the live handler
  in api/repos.rs and the drain both call it. Idempotent on
  (request_id, accepted_ordinal). Returns EffectsOutcome::{Done,
  Nothing, Retry}.
- Drain switches from list_pending_ref_transitions_applied +
  derive_one to list_receive_pack_requests_due +
  apply_request_effects. derive_one and the per-ref drain entry
  points are deleted.
- The step-3 stubs in db/mod.rs (mark_request_effects_pending,
  mark_request_complete, list_receive_pack_requests_due) lose
  their #[allow(dead_code)] annotations; every stub has a caller
  in this PR.
- Reconcile (reconcile_prepared_from_disk_all) is unchanged.
  The crash window between intent durable and outcomes commit
  still routes through per-ref reflog proof.
- Drain tests rewritten to stage receive_pack_requests rows; the
  per-ref fixtures (make_row, lookup_accepted_ordinal) are gone.
- New inv26_step3_live_and_drain_share_apply_request_effects
  assertion pins the live/drain sharing and the per-request
  drain walk.

* fix(node): bounded retirement purge for terminal request rows (Twigpine#26 split 1/4 step 4)

The v30 partial index idx_receive_pack_requests_completed_at
exists but no code reads it. Step 4 wires a periodic purge task
that deletes terminal `complete` and `rejected_at_git` rows
older than the retention window, along with their per-ref
children. `quarantined` (a step-5 state) is never purged by the
timer.

- `purge_completed_receive_pack_requests(older_than, limit)` —
  deletes parent requests using the v30 partial index.
- `purge_completed_pending_ref_transitions(older_than, limit)` —
  deletes children of purged parents, gated on the child's own
  applied_at / cancelled_at.
- `purge_request_queue(db, retention_days, limit)` — orchestrator
  in durable_outbox.rs that calls both and returns the totals.
  The deletion order (parents first, then children) is the
  contract; a crash mid-purge leaves orphaned children that the
  next pass will pick up.
- `spawn_queue_lifecycle_sweep` in main.rs — periodic task on
  the same detached pattern as `spawn_legacy_cid_sweep`, 24-hour
  interval, shutdown-aware. The interval matches the spec's "one
  per cluster per day" target.
- Config knobs `queue_retention_days` (default 7, range 1..=365)
  and `queue_purge_batch` (default 1000, matches
  DRAIN_PER_PASS_LIMIT).
- 3 new `durable_outbox::drain_tests::purge_*` tests pin the
  contract: only old terminal rows are deleted, the second pass
  is a no-op, and a row inside the retention window survives.
- New `inv26_step4_queue_lifecycle_purge_is_wired` gate asserts
  the wiring (main.rs calls the purge, the DB helpers exist, the
  Config knobs exist).

* fix(node): reference-transaction marker + quarantined state + failure matrix (Twigpine#26 split 1/4 step 5)

Steps 2-4 gave the request row the unit of work, the shared
executor, and the bounded retirement policy. Step 5 closes the
evidence gap: the reconcile now requires a per-request marker
ref (refs/gitlawb/requests/<id>) whose value matches
request_bytes_hash. A missing or mismatched marker quarantines
the request; an operator reclassifies it.

- v31 migration: adds `quarantined` to the state vocabulary and a
  partial index for operator queries.
- Handler writes the marker ref before git-receive-pack; the
  marker is causally bound by being in the same async task as
  the receive-pack call. The marker's value is content-addressed
  (git hash-object of the request bytes), so the gate compares
  consistent SHAs on both sides.
- git::store::read_ref reads a single ref's value, returning
  Ok(None) for absent refs. Used by the marker gate.
- git::store::marker_value_for computes the content-addressed
  marker value; both the live handler and the reconcile use it
  so the write and the read agree.
- Reconcile gains a marker gate between the age check and the
  reflog proof. Mismatch or absent ⇒ mark_request_quarantined +
  mark_children_rejected_for_quarantined_parent.
- effects_max_attempts bound (config knob, default 8) flips
  retry-stuck requests to `quarantined` after N attempts,
  closing the infinite-retry DoS window.
- New failure_matrix_tests submodule covers the spec's outcome ×
  ref-kind × exit-point × recovery-scenario matrix (6 cells).
- New inv26_step5_marker_quarantine_and_bound_are_wired gate
  asserts the marker gate, the bound check, the handler's
  pre-receive-pack ordering, and every load-bearing helper.
- Existing 7 reconcile tests updated to stage a marker ref via
  the new `stage_marker` test helper.

* fix(node): converge durable post-receive to singular request-level lifecycle

Unify producer, evidence, aggregate outcome, executor, retry, and
retirement on the request row: atomic intent and outcome commits,
synthetic normalized reports for implicit-ok/reconciled paths,
reconcile parent promotion with fail-closed deletions and
competing-claimant guard, retry progression from both executable
states with backoff/quarantine, terminal-parent-gated purge with
marker cleanup and hidden refs, minimal digest-only intent plus
request-level RFC9421 proof (v32).

* fix(node): harden request occurrence lifecycle, proof, retry, and retirement

Reject incomplete report framing and omitted refs as indeterminate,
preserve uncertain children on completion, terminalize all-rejected,
bind reconcile with landing history, durable proof with ack gate,
occurrence-keyed anchors, background due loop, deletion quarantine
with operator resolve, children-before-parent purge with marker
tombstones, recovery prereq upgrade check, and bounded marker runner.

* fix(node): keep recovery prereq upgrade best-effort on push path

Refusing pushes when git config upgrade fails broke fake-git and
non-repo disk-path tests with 503. Reconcile already fails closed
on missing reflogs, so downgrade to warn-and-proceed.

* fix(node): close reviewer round on outcome authority and lifecycle ownership

Gate effects on unpack_ok with cleared ordinal and terminal no-effects,
intersect executor with applied children, treat absent report-status as
indeterminate regardless of exit, require landing history before child
delete, claim due requests with recoverable leases, dedupe webhooks by
occurrence ledger, ack-gate proof retention with terminal disposition,
fair marker tombstones with dead-letter, kill_on_drop marker bounds,
and loud migration version collision guard.

* fix(node): load-bearing coverage for intent, guards, and delivery ledger

Handler intent test plus inv22 gate so disabling the durable insert
turns red; unpack-false, mixed ok/ng, divergent cancelled, partial
sibling, and proof-ack tests pin each executor guard; webhook
deliveries record sent_at with stale-pending reclaim; stale
parsed_report comment corrected.

* fix(node): centralize live retry, bound outcome commit, close retention orphans

Route live Retry/Err through schedule_request_retry_or_quarantine so
attempt progression matches the drain; retry post-git outcome commits
3x before restart repair; block terminal purge while non-terminal
children exist; add v27 upgrade-path test.

* fix(node): document attended-restart commit policy and pin executor retry arms

Track outcome-commit success, skip metrics/touch on failure, correct
the lease-expiry comment to startup reconcile; add restart-repair test
for stuck received parents plus RED tests for landing-history and
proof-ack failure paths.

* fix(node): correct marker prereq comment to warn-and-proceed

* fix(node): single partial-completion model with disposition gate

Split apply_request_effects into phase-1 bundle (push, cert, anchor,
history, webhook, proof ack before child deletion) and phase-2 gate
that retries while non-cancelled siblings remain; Nothing only when
nothing owed. Gate tail, Tigris, and effects on PostGitDisposition
with documented replication carve-out; add partial-sibling webhook
and disposition tests; fix stale comments.

* fix(node): close review round on outbox ordering, marker visibility, and repair reachability

Address the P1/P2 review findings: await the webhook delivery claim
before accepted-child deletion; exempt the marker namespace from the
fail-closed ref peeling guard so visibility walks tolerate blob-pointing
request markers; keep request marker deletion bounded in the purge path;
promote cross-page siblings via idempotent report superset; run
stuck-parent repair when the prepared queue is empty; bound the durable
intent write while admission permits are held; remove dead proof
verification scaffolding.

* fix(node): close review round on pre-git refusal lifecycle, namespace gate, and promotion merge

* fix(node): close review round on namespace gate, intent lifecycle, and promotion atomicity

Namespace gate hardening:
- Refuse non-UTF-8 ref-update lines (git accepts bytes ≥0x80; gate never sees them)
- Catch bare 'refs/gitlawb' name (starts_with missed it)
- Add deny test for both holes plus refusal wiring verification

Intent lifecycle guards:
- Add IntentDropGuard: terminalize stranded intents on dropped futures
- Bound refuse_receive_pack_before_git to 10s (no statement_timeout on pool)
- Guard mirror fetch: hideRefs blocks non-exempt refs/gitlawb/* from +refs/*:refs/*

Promotion merge atomicity:
- Take row under SELECT FOR UPDATE so merge is atomic with state read
- Prevents racing due worker from stranding newly applied children outside report

* fix(test): correct inv22 namespace gate assertions to match actual implementation

The test was looking for the wrong patterns:
- UTF-8 check: error message string, not pre-loop condition
- Namespace check: `*r == "refs/gitlawb"`, not `ref_name ==`
- Visibility exemption: `&&` logic (both false to exclude), not `||`

* chore: cargo fmt

* fix(node): phase-aware intent guard, mirror import prune, terminal-state merge

* fix(node): guard cleanup retry with fates, fail-closed mirror prune, refusal coverage

* fix(node): bound guard attempts, sibling-wait outcome, fail-closed prune enumeration

* fix(node): sibling-wait outcome, no-report path, bounded prune and guard retry

* fix(node): no-report durability, cert landing order, bounded reconcile git, flush checks

* fix(node): gate trust-score bump on a new push event row

* chore: update rustls 0.23.37 → 0.23.45 to resolve RUSTSEC-2026-0285

* fix(node): surface git's report-status when a stdin EPIPE follows child exit

* fix(node): cap receive-pack ref-update count before per-ref work and intent

* fix(node): cap ref-update parsing in-loop and dedupe identical commands

* docs(node): correct round-4 comment claims on client outcome and gate cost

* fix(node): single replication tail, proven marker taxonomy, claimed webhook delivery (Twigpine#558)

- One post_receive_replication_tail per push: the carve-out spawn and the
  durability-bound spawn are mutually exclusive positions on the same
  proven-landed ref set; inv22 now fails on any unfiltered or ungated spawn.
- Marker reconcile: bounded-read errors (git Err/deadline) stay prepared
  and retry instead of quarantining as 'missing marker ref'; only a
  successfully read marker proves missing or mismatch. Err-injection test
  pins no-quarantine on unproven reads.
- Claimed webhook delivery: the synchronous claim phase returns the
  claimed Webhook rows so the send task never re-lists; a list failure
  after claim can no longer drop claimed deliveries. Source gate pins it.

* docs(node): name claimed-hooks variant in fire_event_occurrence doc

* fix(node): order pending_ref_transitions create before outbox alters, fill v29 (Twigpine#558)

* fix(node): proven-missing marker taxonomy, v38 upgrade test, report-ok tail fixtures (Twigpine#558)

* fix(node): read marker refs via rev-parse so present refs return their oid (Twigpine#558)

---------

Co-authored-by: Gravirei <gravirei@users.noreply.github.com>
Co-authored-by: Kevin Codex <kevin@gitlawb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

crate:node gitlawb-node — the serving node and REST API kind:bug Defect fix — wrong or unsafe behavior subsystem:identity DID/UCAN, http-sig auth, push authorization subsystem:storage Blob/object store, Arweave, IPFS, archives

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Harden Arweave anchoring and add verification

5 participants