fix(node): harden Arweave anchoring and add verification (#26) - #224
fix(node): harden Arweave anchoring and add verification (#26)#224Gravirei wants to merge 15 commits into
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis 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. ChangesArweave integrity flow
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
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
beardthelion
added
crate:node
0801800 to
bd09c35
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
crates/gitlawb-node/src/api/arweave.rscrates/gitlawb-node/src/api/events.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/arweave.rscrates/gitlawb-node/src/auth/mod.rscrates/gitlawb-node/src/cert.rscrates/gitlawb-node/src/config.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/server.rscrates/gitlawb-node/src/test_support.rs
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
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_gatewaydefaults tohttps://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. Sinceverify_anchornever 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 newseq/prevdesign requires that predecessor to remain available.verify_anchorthen silently skips the check whenget_cert_by_seqreturnsNoneor 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_seqthen 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 9421Signature-Input, covered component values, method/path, and content digest are discarded, andverify_anchornever verifiespusher_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 withresp.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 aspending. 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 removesGITLAWB_IRYS_URLwithout a fallback, while both.env.exampleandREADME.mdstill instruct operators to set it. Upgrading an existing documented deployment leavesbundler_urlempty 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 includesseq,prev, andpusher_sig, but both list and get responses omit all three fields. Consumers of the established certificate API (includinggl 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.
|
@Gravirei please rebase to main and fix conflicts |
c94e8ed to
ae4f5fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
crates/gitlawb-node/src/test_support.rs (1)
4983-4988: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake
seed_certproduce chain-valid fixtures.This helper creates the 10- and 55-certificate datasets, but every certificate has
seq: 1and a zero predecessor. The tests therefore cannot catch regressions that ignore monotonic ordering orprevlinks. 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
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
crates/gitlawb-node/src/api/arweave.rscrates/gitlawb-node/src/api/certs.rscrates/gitlawb-node/src/api/events.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/arweave.rscrates/gitlawb-node/src/auth/mod.rscrates/gitlawb-node/src/cert.rscrates/gitlawb-node/src/config.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/server.rscrates/gitlawb-node/src/test_support.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/gitlawb-node/src/api/events.rs
- crates/gitlawb-node/src/server.rs
- crates/gitlawb-node/src/api/arweave.rs
- crates/gitlawb-node/src/config.rs
- crates/gitlawb-node/src/cert.rs
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
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 certificateseq = 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 changedseed_certfixture, which inserts tenseq = 1rows and makeslist_certs_respects_limit_paramfail. 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 callsresp.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 forseq,prev, andpusher_sig. -
[P2] Hold the certificate-chain lock through allocation and insertion
crates/gitlawb-node/src/db/mod.rs:2160
pg_advisory_xact_lockis transaction-scoped, but this standalone pooled query commits beforeissue_ref_certificatereads 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 outerrepo,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 receivevalid: true. Reject field mismatches (and match a locally recorded transaction too if this endpoint is meant to validate local anchors). -
[P2] Keep
gl statuscompatible with the remote created bygl init
crates/gl/src/status.rs:146
This branch regresses the base's multi-remote lookup: the status command now accepts only agitlawb://fetch URL onorigin, whilegl initadds the same URL under thegitlawbremote. Immediately after the supported init flow,gl statusreports that the repository is not a Gitlawb repo and skips the PR/issue queries. Rebase without reverting the base's lookup for thegitlawbremote and other Gitlawb fetch/push URLs. -
[P2] Do not hard-code
mainafter a plaingit init
crates/gl/src/init.rs:41
This branch reverts the base's branch/commit-state handling. Plaingit inithonors the user'sinit.defaultBranch, but the command unconditionally instructsgit push gitlawb main. Onmaster, 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 initializemainwith 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 invokinggitlawb-node --irys-url …now receive an unknown-argument startup error even though the PR claims compatibility forGITLAWB_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 onlygitlawb-coredependency-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.
…or prev hash check, add get_cert_by_seq
Gitlawb#26) - Use configured gateway's data URL (/tx_id) instead of bundler API for verify - Bound untrusted response to 1 MiB on public verify route - Bind each anchor to its own ref update certificate (not repo-wide latest) - Make certificate storage append-only with unique (repo_id, seq) constraint - Add migration v13 to drop old unique index and add sequence uniqueness - Allocate chain sequence numbers atomically using per-repo advisory lock - Fail closed on missing predecessor cert during verification - Persist full RFC 9421 HTTP Signature context (signature-input, content-digest, path) - Verify pusher authorization proof in verify_anchor - Add legacy GITLAWB_IRYS_URL fallback for GITLAWB_BUNDLER_URL - Expose seq, prev, pusher_sig through certificate API list/get responses
ae4f5fc to
e9a1a5e
Compare
…ias, cross-check cert, transaction lock - v13 migration: backfill seq values per repo before creating unique (repo_id, seq) index to prevent failures on existing data - verify_anchor: check Content-Length before buffering response body - config: add deprecated --irys-url alias for bundler_url - verify_anchor: cross-check outer anchor fields (repo, ref_name, old_sha, new_sha, node_did) against embedded certificate - cert issuance: wrap lock/lookup/insert in a single Postgres transaction so pg_advisory_xact_lock is held for the full sequence - Add _tx variants of db methods accepting &mut PgConnection - Fix test helpers to use atomic counters for unique seq values - Fix v10 dedup tests to drop v13 index during pre-migration setup
|
@coderabbitai please make an intensive review of this changes |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/gitlawb-node/src/api/events.rs (1)
429-435: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftUse 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: deriveprevfrom 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
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
crates/gitlawb-node/src/api/arweave.rscrates/gitlawb-node/src/api/certs.rscrates/gitlawb-node/src/api/events.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/arweave.rscrates/gitlawb-node/src/auth/mod.rscrates/gitlawb-node/src/cert.rscrates/gitlawb-node/src/config.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/server.rscrates/gitlawb-node/src/test_support.rs
|
@coderabbitai make an intensive review on this please |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/gitlawb-node/src/arweave.rs (1)
303-304: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winNon-JSON gateway payload still returns 500 instead of
valid: false.serde_json::from_slice(&body_bytes)?propagates throughverify_anchor_endpoint(mapped toAppError::Internal), inconsistent with the other graceful branches and the "could be JSON or raw bytes" comment. Convert a parse failure intoVerifyResult { 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 winConsider 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-IPIpRateLimiter, consider wrappingarweave_routessimilarly. (tx_idis 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_idis never persisted on anchor rows.RecordAnchorInputV2has nocert_idfield andrecord_arweave_anchor's INSERT omits it, so thecert_idcolumn added in migration v12 stays NULL for every anchor even thoughlist_arweave_anchors/list_pending_anchorsproject it. The push path inapi/repos.rsalready 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_urlon 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
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
crates/gitlawb-node/src/api/arweave.rscrates/gitlawb-node/src/api/certs.rscrates/gitlawb-node/src/api/events.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/arweave.rscrates/gitlawb-node/src/auth/mod.rscrates/gitlawb-node/src/cert.rscrates/gitlawb-node/src/config.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/server.rscrates/gitlawb-node/src/test_support.rs
…ate limit - serde_json parse failure in arweave verify returns VerifyResult instead of propagating as AppError::Internal - RecordAnchorInputV2 replaces unused gateway_url with cert_id, persisted in arweave_anchors INSERT, sourced from push-time certificate - arweave routes wrapped with per-IP IpRateLimiter
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
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 doesouter_repo != Some(&c.repo_id), but the outer anchor'srepois written as the slug{owner_key}/{name}(api/repos.rs:1222) while the embedded certificate'srepo_idis 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 returnsvalid: false. The endpoint cannot go green on real data: push to a public repo with a bundler configured, takearweave_tx_idfrom/api/v1/arweave/anchors, andGET /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'srepo_idequal 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 ofpusher_sig,signature_input,content_digest,request_pathbeing present, but the node signing payload (cert.rs) covers onlypusher_sig— not the other three. A holder of a valid node signature can nullsignature_input/content_digest/request_path; the node signature still verifies (those fields are unsigned), the wholeif 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 presentpusher_sigwith 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 honestContent-Length; a chunked or header-omitting (or low-lying) response skips the pre-check, andresp.bytes().awaitthen buffers the whole body before the post-check runs. The verify route is unauthenticated (IP-rate-limited only) andtx_idis 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 andidx_ref_certs_repo_seqbuild are never exercised throughrun_migrations()against pre-existing multi-cert data:v10_upgrade_dedup_via_migrationre-applies only v10 (it deletes just the v10 row fromschema_migrations), andmigration_v11_creates_owner_did_columnseeds 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 seedsschema_migrationsat v12, inserts several same-repo/different-ref certs (allseq=1after v12), runs the migrations, and asserts distinctseqplus 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 thearweave.rsmodule comment referenceGITLAWB_IRYS_URLand never mentionGITLAWB_BUNDLER_URLorGITLAWB_ARWEAVE_GATEWAY(not a break —main.rsfalls back to the old env var with a deprecation warning — but the docs should match). A gateway-fetch failure or a malformed embeddednode_didreturns a 500 that echoes the internal error string (api/arweave.rs) rather than a cleanvalid:falsewith the right status.tx_idis unvalidated before being appended to the gateway URL (no host-swap SSRF given the fixed authority andredirect::none, but validate to the 43-char base64url shape as cheap defense).repo_lock_hashusesDefaultHasher, 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 outerold_sha/new_sha/node_didcross-checks are skipped when the field is absent (is_some()guards), unlike repo/ref; a forger who omits them still gets valid. Minor: the deadlock_repo_cert_issuancehelper 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 againstseq-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
left a comment
jatmn
left a comment
There was a problem hiding this comment.
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.
… proof, streaming body, v13 test, dead code cleanup, dedicated arweave limiter, P3 fixes P1: - arweave.rs: add repo_id to RefAnchor, cross-check compares UUID vs UUID - gl/src/cert.rs: update verify_signature to sign 10 fields (seq, prev, pusher_sig) P2: - cert.rs: bind signature_input/content_digest/request_path into node payload - arweave.rs: fail closed when pusher_sig present but context fields missing - arweave.rs: replace Content-Length body guard with streaming 1 MiB chunk cap - db/mod.rs: add v13_seq_backfill_via_migration upgrade-path test - db/mod.rs: remove dead confirm/fail/list_pending_anchors methods and tests - api/repos.rs: log record_arweave_anchor failures via tracing::warn - server.rs: mask credential URLs in contracts_info endpoint - state/main/server: add dedicated arweave_rate_limiter (GITLAWB_ARWEAVE_RATE_LIMIT) P3: - api/arweave.rs: validate tx_id (43-char base64url) before gateway fetch - arweave.rs: make old_sha/new_sha/node_did mandatory when cert is present - db/mod.rs: replace DefaultHasher with stable SHA-256 prefix for lock keys - db/mod.rs: document v1 schema inclusion of later columns - api/events.rs: include seq, prev, pusher_sig, context fields in local_cert events
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
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.rscert_payload), includingsignature_input,content_digest, andrequest_path.gl's client-sideverify_signaturerebuilds the payload to check the signature against but stops atpusher_sig, never reading or including those three fields.git_receive_packalways passesSome(..)for all three (the route sits behindrequire_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_showjust never parses or forwards them. The byte mismatch means the Ed25519 checkgl cert showruns fails for every push-issued certificate, reporting a validly node-signed cert as tampered. The PR's owngl/src/cert.rstests don't catch this because they only exercise the old 10-field shape (pusher_sig: null, no context fields).
Fix: addsignature_input,content_digest,request_pathtogl'sverify_signaturepayload and its call sites, matching the server'scert_payloadexactly, 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 referenceGITLAWB_IRYS_URLonly; neither mentions the newGITLAWB_BUNDLER_URLorGITLAWB_ARWEAVE_GATEWAY(config.rs).main.rsdoes 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 inverify_anchor(gateway non-2xx, oversized/undecodable body, non-JSON payload) returnsOk(VerifyResult{valid:false, ..}). The node-DID parse (gitlawb_core::did::Did::from_str(&c.node_did).map_err(..)?) still uses?, so a certificate whose embeddednode_didfails to parse propagates asErr, which the handler turns into a 500 instead of the same controlled{valid:false}response every other bad-input path returns.
… docs, and fix node_did 500 on verify_anchor
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed at ed80c90c. All three findings from the last round are genuinely fixed, and I confirmed the signing-payload one by execution rather than inspection: the node's cert_payload and gl's verify_signature now emit byte-identical bytes across all-Some, partial-Some, and unicode/escaping cases. The streamed gateway cap, the fail-closed outer cross-checks, and the incomplete-pusher-context rejection all landed cleanly too.
Three things block merge, all on the verification path this PR exists to add, all reproduced end to end against the real handler.
Findings
-
[P1] Anchor the certificate issuer instead of trusting its self-asserted
node_did
crates/gitlawb-node/src/arweave.rs:414
verify_anchorderives the verifying key fromc.node_did, a field inside the certificate it is checking, so the signature only proves the certificate is internally consistent. I minted a fresh keypair, self-signed a certificate naming its own DID withseq: 1(skipping the prev-chain check) andpusher_sig: null(skipping the pusher proof), served it from a mock gateway, and the endpoint returnedvalid: truewith an emptyerrorsarray for a push that never happened against a repo the node has never seen. Arweave uploads are permissionless and free under 100 KiB, so anyone can place the bytes and hand the node atx_id.glalready implements exactly this guard (crates/gl/src/cert.rs:241-252), and its comment names the attack verbatim: "A hostile source can mint a keypair, put its DID in node_did, and self-sign." Carry that check to the server: comparec.node_didagainststate.node_didviacrate::api::did_matchesand push an error on mismatch. If verifying another node's anchors is intended, that needs an explicit trusted-issuer input (a peer lookup or an?expect_node=) that fails closed when absent, not an implicit trust of whatever the payload claims. -
[P1] Verify a certificate against the payload it was actually signed with
crates/gitlawb-node/src/arweave.rs:396,crates/gl/src/cert.rs:285
The signing payload grew from 7 fields to 13, and both verifiers rebuild the 13-field form unconditionally with no version discriminator and no fallback. Every certificate issued before this PR was signed over{repo_id, ref, old, new, pusher, node, ts}(origin/main'scert.rs), and after the v12/v13 migration those rows read back asseq(renumbered by the v13 backfill),prev= 64 zeros from the column default, and three NULL context fields. Executed: signing that exact legacy payload and feeding the migrated row shape to the new verifier returnsErr("Ed25519 signature does not match the signed payload"), while the same signature verifies against the 7-field payload withtrue. Sogl cert showand the verify endpoint both report every pre-#224 certificate as tampered, and the v13prevmismatch compounds it independently for any repo with more than one certificate. The PR body's "Backward-compatible with existing nodes and previously signed history" is the claim this contradicts. The cheapest fix that keeps old history verifiable is to fall back to the 7-field payload when the 13-field check fails and all four proof fields are NULL, reporting the result as a legacy certificate whoseseq/prevare not covered by the signature; a"v": 2key incert_payloadwould make the next format change detectable instead of silent. -
[P2] Return a generic message when the predecessor lookup fails
crates/gitlawb-node/src/arweave.rs:499
Thesqlxerror is formatted intoerrors, andapi/arweave.rs:44serializes that array straight to an anonymous caller on an unauthenticated route. A failing lookup surfaces the raw database error text, including the connecting user, to anyone who can reach the endpoint. Log the error and push a fixed "could not verify prev-hash linkage" string instead. -
[P1] Bind the pusher proof to the ref transition, and require it
crates/gitlawb-node/src/arweave.rs:513
The pusher signature is meant to be the independent leg: evidence the named pusher authorized this specific ref update, not just the node's word for it. It cannot carry that weight as built. The signing string is rebuilt from three values only,@methodfixed toPOST,@pathfromrequest_path, andcontent-digest(lines 521-525), none of which mentionref_name,old_shaornew_sha, and the verifier holds no pack body to check the digest against. Executed: I generated a pusher keypair, built and signed the real RFC 9421 signing string forPOST /alice/repo.git/git-receive-packwith a content-digest, then embedded that genuine signature in a certificate claimingrefs/heads/releasemoving111...toddd..., a transition the pusher never signed. The endpoint returnedvalid: truewith an emptyerrorsarray. One captured signature therefore vouches for any ref transition on that repo path. Separately the whole block hangs offif let Some(pusher_sig)with noelse, so a certificate withpusher_sig: nullskips the check silently and still reports valid; the present-but-incomplete arm you added this round is correct, but absence is the easier bypass. Have the pusher sign a component committing to<ref> <old> <new>and check that value against the certificate's own fields, then treat a missingpusher_sigas invalid, since every certificate this branch issues populates all four fields. -
[P2] Move the new columns out of the applied v1 migration block
crates/gitlawb-node/src/db/mod.rs:543
The v1CREATE TABLEstatements forref_certificatesandarweave_anchorswere edited to include the columns v12/v13 add, and the NOTE above the array acknowledges it was done "for development convenience". The end states do converge here because v12 usesADD COLUMN IF NOT EXISTSand guards the rename, so this is not a live break, but it contradicts the array's own standing rule two hundred lines up ("Future schema changes MUST be added as v2, v3, … — never appended to v1") and it has a concrete cost: the migration test now starts from a v1 that already carries the columns, so v12's realADD COLUMN/ rename /DROP COLUMNpath is never exercised by anything. Restore v1 to its pre-PR shape and let v12/v13 do the work they already do correctly. -
[P2] Make the gateway-URL test load-bearing
crates/gitlawb-node/src/arweave.rs:815
test_verify_anchor_uses_correct_gateway_urlmocks a 404 and asserts only!r.valid, with anErrarm that accepts any message containing "error". I changed the URL construction to{gateway}/v1/tx/{tx_id}, the exact regression the test's own comment describes, and it still passed. Assert the mock was consumed (_mock.assert_async().await) and drive a 200 with a real body so the path shape is actually pinned. -
[P3] Smaller items
crates/gitlawb-node/src/arweave.rs:278
A gateway connect or DNS failure still uses?and becomes a 500, unlike every other bad-input path beside it that returns a controlledvalid: false.prev_hashcovers the predecessor's 7 canonical fields but not its ownseqorprev, so the link is not recursive over history; the node signature does cover those fields, which is why this is a naming question rather than a hole, but "hash chain" in the docs promises more than the link delivers.arweave_anchors.statusis inserted as'pending'and never updated by any code path, anddeadline_height/receipt_sigare never written at all, solist_anchorsreports a lifecycle that does not exist; the PR body also still listsconfirm_arweave_anchor(),fail_arweave_anchor()andlist_pending_anchors(), which are not on this head.repo_lock_hashusesi64::from_ne_bytestwo lines below a comment promising portability.mask_credential_urlsplits on the first@, sohttps://user:p@ss@hostleaks the password tail.
The certificate-chain design continues to hold up well and the trajectory across rounds has been good. The three P1s share one root: the verifier accepts what the certificate says about itself. Pinning the issuer, versioning the payload, and binding the pusher proof to the transition are each contained changes, but the endpoint should not ship reporting valid: true until all three are in.
Three rules a patch author cannot recover from the code alone, all three found by review rounds on #224 and #236: Anchor the verifying key outside the artifact. Deriving it from a field of the object under verification proves self-consistency, not authenticity, and identities here are permissionless. Found on #236 in the gl client and again on #224 in the server-side twin, where a self-signed certificate verified against an unauthenticated route. Drive the accepting verdict in a test. #224's verify endpoint shipped with sixteen tests, every one asserting the invalid path, which left it unable to observe a forged artifact being accepted. Treat a signed payload's field set as a versioned format. #224 grew it from seven fields to thirteen with no discriminator and no fallback, so every certificate issued before it now reports as tampered.
… binding; P2/P3 items
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Preserve legacy certificate verification in the CLI
crates/gl/src/cert.rs:285
This always reconstructs the new 13-field payload. Certificates issued before this PR were signed over the original seven fields, and migrations only add/default/backfillseq,prev, and proof columns; they do not re-sign those rows. As a result,gl cert show --verifyreports every existing valid certificate as invalid (and exits nonzero). The node verifier already has the necessary guarded 7-field fallback; apply the same compatibility handling ingland cover a legacy fixture. -
[P2] Actually bind the pusher proof to the claimed ref transition
crates/gitlawb-node/src/arweave.rs:581
Adding@ref_name,@old_sha, and@new_shatorequest_valuesdoes not put them in the signature: verification uses only components present in the savedSignature-Input. The shipped signer hard-codes only@method,@path, andcontent-digest(crates/gitlawb-core/src/http_sig.rs:24,173-186). A node can therefore reuse a valid receive-pack proof while issuing a node-signed certificate for different ref/SHA values, and this endpoint will label the pusher proof valid. Require and sign a canonical transition component end-to-end (or retain enough signed request data to derive it) before claiming this proof authorizes the transition. -
[P1] Make the documented default anchor configuration verifiable
.env.example:49
The default uploader is Irys devnet while the verification endpoint always uses the configuredhttps://arweave.netgateway. Irys documents transaction retrieval through its own gateway and says devnet data is deleted after roughly 60 days; it is not an Arweave-mainnet permanent object. Thus an anchor created with the supplied defaults cannot be fetched by/api/v1/arweave/verify/{tx_id}, contrary to the permanent-anchor/verification flow. Configure a matching Irys gateway for that mode, or use a genuinely Arweave-backed default and document the production credential flow. Irys transaction IDs, Irys networks -
[P2] Keep the v1 anchor-list response backward compatible
crates/gitlawb-node/src/db/mod.rs:918
This migration drops the persistedarweave_urland renamesirys_tx_id, whileGET /api/v1/arweave/anchorsserializes the changed struct directly. Existing v1 clients therefore lose both the URL they use to retrieve historical anchors and theirys_tx_idfield, with no response alias or versioned endpoint. Retain the historical URL/data and expose compatibility aliases until clients can migrate.
…ment, env.example defaults, anchor compat fields
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Preserve legacy certificate verification after assigning sequence numbers
crates/gitlawb-node/src/db/mod.rs:928
v12 gives every existing certificate the all-zeroprev, then this migration assigns sequence numbers without backfilling that field. The verifier subsequently performs the 7-field legacy-signature fallback but, for everyseq > 1, unconditionally compares the retained zero value to the predecessor's SHA-256 hash (arweave.rs:521). Thus every pre-upgrade certificate after the first in a repository is reported invalid even though its original signature verifies, contradicting the claimed compatibility with previously signed history. Mark legacy rows as non-chain records and skip this check for them, or migrate their linkage with an explicit versioned compatibility rule. -
[P2] Pair the legacy Irys fallback with its gateway
crates/gitlawb-node/src/main.rs:73
Existing deployments that only set the documentedGITLAWB_IRYS_URL=https://devnet.irys.xyzare migrated only for uploads: this copies the value intobundler_urlbut leavesarweave_gatewayathttps://arweave.net. Their new anchors are therefore written to Irys devnet while both the verify endpoint and the compatibility URL returned by/api/v1/arweave/anchorsattempt to read mainnet, so the advertised verification/retrieval flow cannot work after an otherwise backward-compatible upgrade. Infer or require the matching gateway when using the legacy setting (without overriding an explicitly supplied new gateway). -
[P2] Do not publish anchors that the new verifier must reject
crates/gitlawb-node/src/api/repos.rs:1017
Certificate issuance errors are logged and ignored, but the later anchoring loop still uploads the ref update withcertificate: None(:1325) and persists a nullcert_id.verify_anchornecessarily rejects that permanent artifact as having no embedded certificate. A transient DB/sequence failure after a successful push therefore creates an anchor that can never satisfy this PR's verification contract. Skip or retry anchoring until its certificate is persisted, or record and expose an explicit unverifiable/failed state instead of publishing it as a normal anchor. -
[P3] Limit only the expensive verification endpoint
crates/gitlawb-node/src/server.rs:229
The new 120/hour IP bucket wraps both the gateway-fetching/verify/{tx_id}route and the pre-existing/anchorslisting route. Requests to either consume the same quota, so verification traffic can make ordinary anchor-list clients receive 429 for an hour (and cheap list polling can block verification). Split the routers or apply this limiter to/verify/{tx_id}only; the stated reason for the quota is the new outbound gateway work.
…nverifiable anchors, separate rate limiter
There was a problem hiding this comment.
Edited 2026-07-27: the pusher-proof finding below had the mechanism wrong. The finding and the ask are unchanged; the reasoning and the suggested fix are corrected inline, and the follow-up is now #252.
Re-reviewed at 18946fa. The issuer anchor and the legacy-payload fallback both landed and I confirmed them by execution rather than reading: a self-signed certificate naming its own DID is now rejected, and a genuine pre-#224 certificate signed over the old seven fields verifies again through the fallback. The gateway-URL test is load-bearing now too (I broke the path construction and watched it go red), gl's new legacy test fails when I remove the fallback it covers, and this last commit's separate verify limiter, gateway pairing and skip-the-anchor-without-a-certificate all check out.
Two things block merge, both on the accept side of the verifier, and the newest commit widened the first one.
Findings
-
[P1] Cover
seqandprevbefore reporting a legacy certificate as valid
crates/gitlawb-node/src/arweave.rs:493,crates/gitlawb-node/src/arweave.rs:528
The seven-field fallback payload omitsseqandprev, so on that path both values are whatever the anchor says. Executed against the real handler at the previous head: I took a certificate the node genuinely signed pre-#224, changed nothing the signature covers, rewroteseqto 1 andprevto 64fs, served it from a mock gateway and gotvalid: truewith an emptyerrorsarray. The new all-zeros escape at:528widens this rather than closing it. It keys onc.prev, which is exactly the field the fallback leaves unsigned, so the attacker no longer has to claimseq: 1to skip the chain check. Executed on this head: same genuine legacy certificate,seq: 999999,prevleft at the migration default, and the chain check is skipped with awarn!and the verdict isvalid: true, errors: []. Reaching that state needs no privilege.GET /api/v1/repos/{owner}/{repo}/certsserves the certificate to an anonymous caller for any public repo (api/certs.rs:29gates throughauthorize_repo_read), and sub-100 KiB Arweave uploads are permissionless, so anyone can place the bytes and hand the node atx_id. The transition itself stays authentic. The append-only chain position is the property this PR adds, and on the legacy path nothing signs it. The invariant a fix has to preserve is the one both of these changes exist for: a pre-#224 certificate whose original signature verifies must still come back valid, so deleting the fallback is not the answer. [DIRECTION] Corroborate the unsigned fields against the node's own record rather than trusting the anchor for them (db.get_ref_certificate(&c.id)atdb/mod.rs:2163already returns the row), and error when they disagree, or stop asserting the chain in the verdict and return an explicit legacy marker sovalidnever covers a field nothing signed. If the all-zeros skip stays in some form it needs to be conditioned on the node's own record saying that row is a migrated one, not on a value the caller supplies. -
[P1] Make the issuer check load-bearing before shipping it
crates/gitlawb-node/src/arweave.rs:346
I deleted thec.node_did != node_didblock on this head and all 11 arweave tests stayed green.test_verify_anchor_malformed_node_didis the only test named for it and its assertion is an OR (does not match this nodeorinvalid node DID); the fixture's malformed DID independently fails the parse a few lines down, so the arm that would catch a missing issuer check never has to fire. With the block gone, the exact self-signed forgery from the last round comes back atvalid: truewith an emptyerrorsarray. The reason it can hide is that no test anywhere drives this endpoint to an accepting verdict: bothverify_anchortests assert!valid, so nothing in CI distinguishes this verifier from one that rejects everything, and the prev-linkage and pusher-proof blocks have no test in either direction. Add a sign-then-verify test that builds the real 13-field payload, signs it with a generated keypair, serves it through mockito and assertsvalidwith empty errors. Once that oracle exists, split the issuer test so it asserts only the issuer error, and the negatives around it start meaning something. -
[P2] Move the
idx_ref_certs_repo_refdrop to a later migration
crates/gitlawb-node/src/db/mod.rs:948
v13 drops that index, but pre-#224 code inserts certificates withON CONFLICT (repo_id, ref_name)(origin/maindb/mod.rs:1995), which Postgres rejects once no matching unique index exists. During any rolling or multi-machine rollout the new instance runs the migration while old instances are still serving pushes, so their certificate issuance starts failing for the length of the window. The failure is swallowed atapi/repos.rs:1017, so those pushes succeed with no certificate and, on new code, no anchor either, with only a warn line to show for it. Ship v13 with the new(repo_id, seq)index only and drop the old one in v14 next release. -
[P2] Say what the pusher proof actually proves
crates/gitlawb-node/src/arweave.rs:572Correction to the original version of this finding. I wrote that the pusher signs
@method,@pathandcontent-digest, "none of which mention ref_name, old_sha or new_sha," and that one captured proof therefore vouches for every transition on the repo path. That reasoning is wrong and I am retracting it. The component names are fixed, but thecontent-digestvalue is SHA-256 over the entire receive-pack body (gitlawb-core/src/http_sig.rs:200-204), the git-remote helper signs those real bytes (git-remote-gitlawb/src/main.rs:386), and the node parsesold_sha new_sha ref_nameout of the same bytes (api/repos.rs:875, parser at:1658). The pusher does cryptographically commit to the exact transition on every push, and a genuine proof commits to its own transition and no other.The finding and the ask are unchanged, because the defect is on the verification side. The node retains none of that commitment:
require_signatureforwards onlyAuthenticatedDidand drops the signature headers (auth/mod.rs:241), the body goes to git and is not kept, andref_certificateshas no column for pusher signature material. So the verifier has nothing to check the digest against and cannot tell a matched pairing from a mismatched one. Executed: I signed a real receive-pack proof, embedded it verbatim in a node-signed certificate claimingrefs/heads/releasemoving111...toddd..., and the endpoint returnedvalid: truewith no errors. The verdict still reduces to trusting the issuing node.My original suggested fix, adding a transition component to
COVERED_COMPONENTS, is also retracted. It is a bidirectional flag day: that constant is both the signer's component set and the verifier's floor, an old client against a new node fails themissing_componentsgate, and a new client against an old node fails too because the verifier builds the signing string from the client's component list against a three-key map (auth/mod.rs:172-184). Eight production call sites are affected including three node-to-node paths, so a node upgrade would sever its own peers.What stands is the smaller ask: this PR should not describe the pusher proof as evidence the pusher authorized the transition. Drop the reuse argument from the comment, and correct
api/arweave.rs:28, which still calls the pusher check "optional, informational" whenvalid: errors.is_empty()makes it blocking. The underlying gap and the real remedies are now #252, which also records the three traps I hit, so nobody spends the time I did. -
[P3] Smaller items
crates/gitlawb-node/src/db/mod.rs:935
The v13 comment says existingprevvalues already match what was computed at issuance. That is not true of any pre-upgrade row, sinceorigin/main'sref_certificateshas noprevcolumn at all and every migrated row carries v12's zero default. The new comment atarweave.rs:522now says the opposite in the same PR, so one of the two is going to mislead whoever reads it next.GITLAWB_ARWEAVE_RATE_LIMIT(main.rs:317) is read through a rawstd::env::varinstead of aConfigfield, so it is missing from--help, README and.env.example, and a malformed value silently becomes 120. The new gateway inference keys onstd::env::var(...).is_err(), so an operator who exportsGITLAWB_ARWEAVE_GATEWAY=empty gets neither the explicit value nor the inferred one. The module doc atarweave.rs:18still says transaction IDs are 43-character base58 while the validator you added checks base64url. Theref_certificatesblock in the v1 migration still carries v12's columns, which is why no test ever exercises v12's realADD COLUMN, rename orDROP COLUMNpath.
The certificate design continues to hold up and the fixes have been real ones every round. The pattern worth naming after five rounds is that every defect found so far has been on the accept side, and the suite still has no accepting-verdict test to see them with. Adding that test is worth more than any single fix in this list.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
The certificate-chain work from earlier rounds looks solid on this head: per-ref cert binding, append-only issuance, issuer pinning, streaming gateway cap, legacy CLI payload fallback, separate verify rate limiting, and skip-anchoring-when-no-cert are all in place. The remaining problems are on the verify accept path this PR adds.
Findings
-
[P1] Do not report
valid: truefor legacy certificates whose unsignedseq/prevwere tampered
crates/gitlawb-node/src/arweave.rs:493
The guarded 7-field signature fallback verifies onlyrepo_id, ref, and SHAs.seqandprevare not covered on that path. Whenprevis the migration default all-zero value andseq > 1, the verifier skips the chain check entirely (:528). A genuinely pre-#224 certificate whose 7-field signature still verifies can therefore be served with rewrittenseq/prevand come backvalid: truewith emptyerrors. Public repos expose certificates over/api/v1/repos/{owner}/{repo}/certs, and sub-100 KiB Arweave uploads are permissionless, so this needs no push privilege. The all-zero skip was added to avoid false negatives on migrated rows, but it leaves chain position unsigned on the legacy accept path. Please corroborateseq/prevagainst the node's own stored row (db.get_ref_certificate) before accepting the legacy path, or return an explicit legacy/unverifiable-chain marker instead of a blanketvalid: true. -
[P2] Add a sign-then-verify acceptance test for the new endpoint
crates/gitlawb-node/src/arweave.rs:703
This is test debt, not a current runtime bug — the issuer check is present at:346. Every existingverify_anchortest still asserts only!validor transport failures. There is no test that signs the real 13-field payload, serves it through a mock gateway, and expectsvalid: truewith emptyerrors. That gap already hid a missing issuer check in an earlier round: deleting thec.node_did != node_didblock leaves all arweave tests green, andtest_verify_anchor_malformed_node_diddoes not exercise the issuer arm because the malformed DID fails parsing first. Please add the positive oracle first, then split issuer, prev-linkage, and pusher-context negatives so those guards cannot regress silently. -
[P2] Pair the gateway when
GITLAWB_BUNDLER_URLis set directly
crates/gitlawb-node/src/main.rs:93
Gateway inference currently runs only when the legacyGITLAWB_IRYS_URLfallback is used. An operator who sets onlyGITLAWB_BUNDLER_URL=https://devnet.irys.xyzstill getsarweave_gateway = https://arweave.netfrom config defaults. Anchors upload to devnet, but/api/v1/arweave/verify/{tx_id}and/api/v1/arweave/anchorsresolve through mainnet..env.examplenow sets both knobs correctly, so the copy-paste path is fine; README and clap defaults still allow the mismatch for partial env setup. Mirror the legacy pairing for the new env name (without overriding an explicitly set gateway). -
[P2] Defer dropping
idx_ref_certs_repo_refuntil after the rolling-upgrade window
crates/gitlawb-node/src/db/mod.rs:948
Migration v13 drops the(repo_id, ref_name)unique index in the same release that switches issuance to append-onlyINSERTs.origin/mainstill upserts withON CONFLICT (repo_id, ref_name), which requires that index. During a mixed-version rollout, old binaries continue to issue certificates against the dropped index and fail;git_receive_packswallows those failures (api/repos.rs:1017) and the new code then skips anchoring for the affected ref. This only bites if you roll nodes gradually; an all-at-once deploy avoids it. Ship v13 with only the new(repo_id, seq)index and drop the old one in v14 once old writers are gone. -
[P3] Clamp negative
limiton the anchors list route
crates/gitlawb-node/src/api/arweave.rs:68
list_anchorspassesq.limit.min(200)straight into SQL.list_ref_certificatesalready clamps with.max(1)at the DB boundary, butlist_arweave_anchorsdoes not, so?limit=-1becomesLIMIT -1and returns 500. Apply the same lower bound used elsewhere before querying. -
[P3] Update stale pusher-proof wording on the verify route
crates/gitlawb-node/src/api/arweave.rs:28
The route comment still says the pusher check is "optional, informational", butverify_anchortreats missing/incomplete proof context as blocking. The deeper gap — the verifier cannot re-check the signed receive-pack body the digest names — is tracked in #252. Until that lands, please correct the endpoint/docs wording so callers do not treatvalid: trueas proof the pusher independently authorized the transition. -
[P3] Publish the issued
cert_idin P2P ref-update gossip
crates/gitlawb-node/src/api/repos.rs:1285
This push path now issues per-ref certificates and persistscert_idwith anchors, butp2p.publish_ref_updatestill hardcodescert_id: None. Peers cannot correlate gossiped ref updates with the certificate/anchor evidence this PR adds. Pass the issued certificate id through the event when present. -
[P3] Drop unrelated Fly config deletions from this branch
infra/fly/gitlawb-node-2.fly.toml,infra/fly/gitlawb-node-3.fly.toml
Both production Fly app configs exist onmainand are removed here with no replacement ininfra/fly/. That is unrelated anchoring churn; please revert or justify the deletion separately. -
[P3] Document
GITLAWB_ARWEAVE_RATE_LIMITwith the other operator knobs
crates/gitlawb-node/src/main.rs:317
The dedicated verify limiter is read from a rawstd::env::varand is absent fromConfig,--help, README, and.env.example. A malformed value silently falls back to 120/hour. Please expose it through the normal configuration surface.
Summary
Replace the Irys upload path with a generic Bundler endpoint, capture the pusher RFC 9421 HTTP Signature in ref certificates, add monotonic sequence numbers with SHA-256 chaining to certificate chains, and expose a
/api/v1/arweave/verify/{tx_id}endpoint that fetches the anchor from Arweave and validates the embedded certificate chain.Motivation & context
Closes #26
Kind of change
What changed
gitlawb-node
GITLAWB_IRYS_URLrenamed toGITLAWB_BUNDLER_URL;GITLAWB_ARWEAVE_GATEWAYadded.{url}/uploadto{url}/v1/tx; content-typeapplication/octet-stream; headerx-bundler-tags; Bundler response parsing.RefAnchornow carries an optional embeddedRefCertificate. Newverify_anchor()function fetches from gateway, extracts the cert, verifies the node Ed25519 signature and checksprevhash linkage against the local DB.RefCertificategainsseq(i64),prev(String),pusher_sig(Option).ArweaveAnchorgainsstatus,deadline_height,receipt_sig,cert_id;irys_tx_id→arweave_tx_id. Migration v11 upgrades existing databases.RecordAnchorInputV2replaces the old input struct. Addedget_most_recent_cert(),confirm_arweave_anchor(),fail_arweave_anchor(),list_pending_anchors().issue_ref_certificateacceptspusher_sig, chains from the previous cert via SHA-256 prev hash, builds a v2 signing payload.require_signaturemiddleware injectsPusherSignatureextension (base64-encoded Ed25519 sig bytes).git_receive_packextractsPusherSignature, passes it to cert issuance.RefAnchorconstruction fetches latest cert from DB. UsesRecordAnchorInputV2.GET /api/v1/arweave/verify/{tx_id}endpoint.RefCertificateinitializers for new fields.How a reviewer can verify
DATABASE_URL=postgresql://gitlawb:changeme@172.19.0.2:5432/gitlawb cargo test --package gitlawb-nodeAll 495 tests pass.
Before you request review
cargo test --workspacepasses locally (495 passed)cargo fmt --allandcargo clippy --workspace --all-targets -- -D warningsare cleanfix(...)).env.exampleupdated if behavior or config changed (or N/A)Protocol & signing impact
did:key, Ed25519 / RFC 9421 signatures, UCAN, ref certs, or P2P wire formatsNotes for reviewers
The
gateway_urlfield inRecordAnchorInputV2is passed through by callers but not persisted in the DB — it is used by the caller to construct the Arweave URL at query time. Thecert_idcolumn onarweave_anchorsis reserved for a follow-up linking pass.Summary by CodeRabbit
New Features
GET /api/v1/arweave/verify/:tx_id, returning validity, error details, and optional embedded certificate info.Improvements
seq,prev, and related optional HTTP signature fields.Configuration