fix(node): harden Arweave anchoring and add verification (#26) - #224
fix(node): harden Arweave anchoring and add verification (#26)#224Gravirei wants to merge 25 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.
ae4f5fc to
e9a1a5e
Compare
|
@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
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.
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.
…tems Bundlers (Irys, Turbo) require the upload to be a signed Arweave data item; the previous unsigned JSON POST with an x-bundler-tags header was neither a supported upload protocol nor authenticated. Build and sign ANS-104 items with the node keypair (the signature IS the upload credential) and embed the indexing metadata as item tags inside the signed item. - ans104: deepHash matching @irys/arbundles (length-tagged SHA-384 chain), Avro-style tag serialization, build_signed_data_item/verify_data_item, pinned to an independent reference vector plus tamper/forge rejection tests - anchor_ref_update/anchor_encrypted_manifest now POST signed data items; x-bundler-tags header helpers removed - repos.rs passes the node keypair at both anchor call sites - bundler POST tests now run against a real in-process server that parses the item, verifies the signature, and checks tags/payload (denies on any failure); adds an end-to-end wrong-key rejection test
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] Provide a funded/authenticated upload path for production bundlers
crates/gitlawb-node/src/arweave.rs:109
Bothanchor_ref_updateandanchor_encrypted_manifestPOST only an ANS-104 item to{bundler}/v1/tx; neither call supplies a payment credential, chooses a payment token, nor has access to a configured funded account. An ANS-104 signature proves who created the data item, but it does not pay the bundler's storage charge. Turbo's/v1/txservice charges Turbo Credits, and Irys' documented upload path requires a funded wallet/account (including devnet faucet funds). A normal configured production bundler therefore rejects this request for insufficient funds/credit. The failure reachespost_receive_replication_tail, which only warns atapi/repos.rs:2584; the Git push remains successful but has no permanent ref or manifest anchor, contrary to the advertised anchoring behavior. The in-process “enforcing bundler” test deliberately accepts any syntactically valid item and cannot detect this integration failure.Make the payment model part of the node's supported configuration and request construction: for example, configure a funded supported wallet/account and use the bundler's documented upload protocol, or require an account/credit identity that corresponds to the data-item signer. Cover the real boundary with an integration fixture that rejects unfunded uploads and accepts only an item associated with the configured funding identity. If anchoring is intentionally best-effort and externally pre-funded, validate that prerequisite at startup and document the exact identity, funding, and devnet-faucet setup rather than stating that the node signature is sufficient authentication.
-
[P1] Redact query credentials before deriving or logging public gateway URLs
crates/gitlawb-node/src/api/arweave.rs:88
The public handler passesGITLAWB_ARWEAVE_GATEWAYthroughmask_credential_urland appends the transaction ID. That helper removes onlyuser:pass@; it preserves query and fragment material. Withhttps://gateway.example/data?token=SECRET, an unauthenticatedGET /api/v1/arweave/anchorsserializesSECRETin everyarweave_url(and constructs the malformed...?token=SECRET/<tx_id>URL). The same raw secret leaks to process logs whenmain.rs:95-99infers the gateway from a credential-bearingGITLAWB_BUNDLER_URL. This is a direct disclosure to every API caller and to log readers, despite the PR already recognizing that these configuration URLs may carry credentials.Replace the string-based redactor with parsed-URL handling. Construct public anchor links from only the safe scheme, host, port, and intended path prefix; drop userinfo, query, and fragment before appending the transaction ID. Use that same safe display value for every log and status/metadata response, while retaining the original URL only for the outbound client. Add regression cases for userinfo, query-token, fragment-token, and a configured path prefix so the redaction fix does not turn valid gateway routing into another leak.
-
[P2] Keep the v1 migration immutable
crates/gitlawb-node/src/db/mod.rs:533
The PR rewrites the already-released v1 definitions ofref_certificates(addingseq,prev, andpusher_sig) andarweave_anchors(replacingirys_tx_id/arweave_urlwitharweave_tx_id). An existing deployment hasschema_migrations.version = 1, so it never executes these changedCREATE TABLE IF NOT EXISTSstatements; a fresh deployment does. The later v18/v19 ALTERs happen to close parts of that gap today, but the two installations now have different claimed v1 histories, and a later migration cannot safely infer which schema came from which history. This directly violates the repository's migration invariant that a merged migration is never edited; CI creates a fresh schema, so it cannot expose the upgrade-path divergence.Restore the v1 SQL byte-for-byte to the base definition. Put every certificate-column addition and Arweave column rename/drop in new append-only migrations that are safe both for existing databases and for clean installs running the whole sequence. Add an upgrade test that creates the exact base-v1 schema, records v1 as applied, runs the current migrations, and asserts the final schema and a representative cert/anchor write work; retain the corresponding clean-install test so the two paths stay equivalent.
Superseded by re-review of the current head.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed at 99acf93d. CI is 12/12 green and the merge base is current main. The new commit does post a signed ANS-104 item: replacing that body with unsigned JSON turns test_anchor_success red (Bundler returned 400 ... unsupported signature type 8827). Missing Content-Digest is now 400 (content_digest_missing); deleting that reject turns the new test into 401. Legacy chain corroboration keys on the signed tuple; putting get_ref_certificate(&c.id) back makes the borrowed-row forgery valid:true with an empty errors list.
Three holes from earlier rounds are still live. The public URL redactor still lets a query token through.
Findings
-
[P1] Fail closed when outer repo/owner cannot be corroborated
crates/gitlawb-node/src/arweave.rs:463
Ok(None)andErrfromget_repo_by_idlog a warning and push no error, so the outer identity check does not run. I addedrepo: "victim-owner/victim-repo"andowner_did: "did:key:zVictim"to the authentic 13-field accept test (lazy pool, so the lookup errors) and it stayedvalid:truewitherrors=[]. That accept test is green because corroboration cannot run, not because the outer identity was checked. On miss or lookup error, push a distinct unverifiable error sovalidcannot stay true. The current accept test will go red; seed a repo row, or keep the lazy pool only if you also fail closed when the outer fields are present and the lookup does not complete. -
[P1] Drop query and fragment before emitting any public gateway URL
crates/gitlawb-node/src/server.rs:601
mask_credential_urlstrips onlyuser:pass@. Asserting that?token=SECRETmust not survive turnsmasks_userinfo_preserving_scheme_and_pathred.GET /api/v1/arweave/anchorsis unauthenticated and builds everyarweave_urlfrom that helper (api/arweave.rs:88);GET /api/v1/contractsuses it forbundler_urlandgateway. The same token also lands in the verify error body:verify_anchoragainsthttp://127.0.0.1:1/?token=SECRETreturnedArweave gateway connection failed: error sending request for url (http://127.0.0.1:1/?token=SECRET/<tx_id>). Drop query and fragment, then strip userinfo; keep the raw URL only on the outbound client. I compiled that split in the mutation tree: the SECRET asserts went green, including the existing userinfo cases. Cover query-token, fragment-token, userinfo, and a path prefix. -
[P1] Stop treating the node signature as bundler payment
crates/gitlawb-node/src/arweave.rs:109
Both upload paths POST an ANS-104 item to{bundler}/v1/txwith no payment credential. The in-process enforcing bundler checks the signature, tags, and JSON, then returns 200; it cannot see an unfunded production node. Irys documentsNot enough balance for transactionwhen you upload without funding the bundler. Defaultbundler_urlis empty, so this is opt-in, butarweave.rs:8-12andans104.rs:4-6still say the node keypair is the upload credential and that Irys allows free uploads under 100 KiB. A configured production bundler therefore fails inpost_receive_replication_tail, which only warns (api/repos.rs:2584); the git push succeeds with no permanent anchor. Make the funding model part of config and the request, or refuse to start when a bundler URL is set without a documented funded identity. Do not claim the signature is sufficient authentication. -
[P2] Keep the v1 migration immutable
crates/gitlawb-node/src/db/mod.rs:533
Compared toorigin/main, v1ref_certificatesnow inlinesseq/prev/pusher_sig, and v1arweave_anchorsusesarweave_tx_idinstead ofirys_tx_id/arweave_url. An existing deployment hasschema_migrations.version = 1, so it never re-runs thoseCREATE TABLEstatements; a fresh install does. v18'sADD COLUMN IF NOT EXISTSand conditional rename close parts of that gap today, but the two histories are no longer the same v1. Restore theorigin/mainv1 SQL byte-for-byte. Keep the column adds and the rename in v18+. Add an upgrade test that creates that base v1, records version 1, runs current migrations, and writes a cert and an anchor.
Not an ask, recorded only: the all-zeros prev skip at arweave.rs:641 is still there for migrated rows; 13-field certs sign prev, and the legacy path now corroborates against the signed tuple. #134 still owns gating /arweave/anchors.
One process note, not a finding: this branch shares auth/mod.rs, api/repos.rs, and db/mod.rs with other open work, so expect a rebase after those land. v18/v19/v20 here collide with #173's claimed versions; whichever merges second has to renumber.
…-arweave-anchoring-verification # Conflicts: # crates/gitlawb-node/src/db/mod.rs
…immutable Review follow-ups on the signed ANS-104 anchoring work: - Funded upload model: the node's ANS-104 signature is authorship, not payment. Add GITLAWB_BUNDLER_ACCOUNT, send it as x-bundler-address on every upload, refuse to start when a bundler URL is set without a funded account, and correct the docs that claimed signature-as-authentication. - Redact gateway/bundler URLs everywhere they surface publicly: drop userinfo, query, and fragment in mask_credential_url, the anchors listing, the gateway-inference log, and the verify error body (which reqwest seeded with the raw URL). - Fail closed in verify_anchor: when outer repo/owner identity is present but the repo row cannot be corroborated, the result is invalid instead of silently skipping the check. - Keep the released v1 migration byte-identical to origin/main; the cert chain and anchor column work lives in v18+, with an upgrade test that replays the deployed v1 schema and proves certs and anchors round-trip.
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] Fix the required stable test before asking for another review
crates/gitlawb-node/src/arweave.rs:1586
The new test's “tampering” isformat!("A{}", &signature[1..]), which leaves the signature unchanged whenever its first base64url character already isA. That is not theoretical: the current head's requiredtest (stable)job took exactly that branch. The certificate then verifies as a valid legacy signature, reaches the new DB-corroboration path, and the assertion sees only the lazy-pool lookup error instead of the expected signature failure.The root cause is mutating an encoding character without proving it differs from the original, which makes the security regression test probabilistic. Decode the signature, flip a byte (or otherwise construct a value guaranteed to differ), re-encode it, and retain an assertion that the altered value differs before calling
verify_anchor. This makes the negative test load-bearing and restores the required check. -
[P1] Hash ANS-104 tags as the required nested deep-hash structure
crates/gitlawb-node/src/ans104.rs:78
build_signed_data_itemputs the Avro-serialized tag stream into the signature preimage as one blob. ANS-104's data-item preimage instead deep-hashes tags as a nested list of[name, value]chunks; the binary serialization is the wire representation, not the value to substitute for that nested element. Bothanchor_ref_updateandanchor_encrypted_manifestalways attach tags, so a real bundler recomputes a different signing message and rejects every enabled upload. The local “enforcing bundler” cannot expose this because it calls the sameverify_data_itemimplementation and therefore accepts the same nonstandard preimage.The root cause is conflating the tag wire encoding with the signed logical data-item structure. Make the deep-hash routine recursive (bytes versus list), construct the tags element from nested name/value byte chunks, and use that same standard preimage in verification. Add an interoperability vector produced by an independent ANS-104 implementation with nonempty tags; a round trip through this module alone is insufficient.
-
[P1] Keep the post-receive tail detached at the durable-success boundary
crates/gitlawb-node/src/api/repos.rs:2030
Before this change, a successfulreceive-packimmediately spawnedpost_receive_replication_tailand only then awaitedguard.release(). This branch insertsrecord_push, trust-score reads/writes, and one or moreissue_ref_certificatedatabase transactions before the spawn at line 2075. Those awaits are still part of the client request. If the client disconnects while any one is waiting—for example, certificate issuance is waiting on its per-repo advisory lock behind another push—the future is dropped after Git has accepted the pack but before any tail exists. Pinning, gossip, GraphQL notification, and anchoring are then silently lost for that successful push.The root cause is making the tail depend on data collected by cancellable request-owned work. Preserve the durable-success boundary: either issue certificates inside an owned continuation that subsequently runs the tail, or spawn an owned coordinator at the original boundary and have it issue certificates before anchoring. Do not leave a successful pack without a durable owner for its follow-up work. Add a cancellation test that blocks certificate/DB issuance (not just
guard.release) and proves the tail still executes. -
[P1] Join configured gateway and bundler URLs structurally
crates/gitlawb-node/src/arweave.rs:115
arweave.rs:221
arweave.rs:306
Appending/v1/txor/{tx_id}to the complete configured URL breaks the query- and fragment-credential forms the PR explicitly supports and redacts. For example,https://gateway.example/data?token=SECRETbecomes.../data?token=SECRET/<tx_id>, placing the transaction ID in the query rather than the request path; with#fragment, the suffix is never sent to the server at all. The same string-concatenation bug affects both uploads and verification, so authenticated or path-prefixed endpoints cannot anchor or retrieve an anchor.The root cause is treating a structured URL as an opaque path prefix. Parse the configured URL once, extend its path segments with
v1/txor the transaction ID, and preserve its intended query separately. Reject fragments in outbound endpoint configuration (they are not HTTP request data), rather than silently constructing a misleading URL. Add request-target assertions for bare origins, path prefixes, query-token endpoints, and rejected fragments on both upload functions and verification. -
[P1] Apply credential redaction to every bundler and gateway error path
crates/gitlawb-node/src/arweave.rs:124
crates/gitlawb-node/src/arweave.rs:343
The new masking only replaces the initial gateway connection error. A failed response stream is returned verbatim by the unauthenticated verify endpoint, and both upload functions wrap raw reqwest errors verbatim beforeapi/repos.rsemits them witherr = %e. Reqwest errors include the request URL, so a credentialed gateway or bundler that fails after response headers, during a response body read, or during upload can discloseuser:passor?token=values through public API output or node logs.The root cause is applying redaction at one call site instead of treating raw configured URLs and URL-bearing client errors as sensitive values throughout the boundary. Centralize outbound URL construction and error sanitization: retain the raw URL only for the request, map every client error to a representation with userinfo/query/fragment removed before returning or logging it, and avoid interpolating unbounded response error bodies into logs as well. Cover initial-connect, failed-stream, non-success response, JSON-decode, and both upload paths with credential-bearing fixtures.
-
[P2] Make the shipped environment template startable
.env.example:54
The template enablesGITLAWB_BUNDLER_URL=https://devnet.irys.xyz, but neither it nor the README defines the now-mandatoryGITLAWB_BUNDLER_ACCOUNT.Config::validate()rejects every nonempty bundler URL with an empty account, so a normal deployment that copies the advertised environment file fails during startup before serving traffic. The new README settings table also presents the URL, gateway, and rate limit without explaining the required companion setting.The root cause is changing a cross-field startup invariant without updating the canonical operator configuration and documentation as one contract. Either leave the template's bundler URL empty so the default sample boots with anchoring disabled, or include an explicitly required account setting and complete setup instructions. Add a configuration test that parses the checked-in example (or its extracted Arweave section) and calls
validate()so future documentation changes cannot reintroduce a non-startable sample.
There was a problem hiding this comment.
Re-reviewed at 33a6f240. Most of the round landed: v1 is byte-identical to origin/main again (I diffed the migration region, no differences), the DDL now sits in v17-v20 with a replay upgrade test, verify_anchor fails closed when outer identity cannot be corroborated, and the credential redaction reaches the anchors listing, the inference log, and the verify error body.
jatmn's review landed on this same head, so I have trimmed the overlap rather than repeat it. Take their six findings as part of this round; I agree with their .env.example P2 on the same line I had it, and their tamper-test P1 matches what I measured. Below is only what is mine to add.
Findings
-
[P1] Send the payer header the target bundler actually reads
crates/gitlawb-node/src/arweave.rs:120
x-bundler-addressis not a header Irys or Turbo honors. Irys readsx-irys-paid-by(UploadHeaders.PAID_BYinIrys-xyz/js-sdk,packages/upload-core/src/types.ts) and posts to/tx/{token}; ArDrive Turbo readsx-paid-byonPOST /v1/tx(ardriveapp/turbo-upload-service,src/routes/dataItemPost.ts). A code search forx-bundler-addressreturns nothing anywhere. So the upload still bills whoever the data item's signer resolves to,GITLAWB_BUNDLER_ACCOUNTnever leaves the process, and becauseapi/repos.rs:2589degrades an upload error to a warning, an operator who follows the new instructions loses every anchor silently. Note this compounds with jatmn's deep-hash finding: the header is ignored and the preimage is nonstandard, so the upload path fails against a real bundler at two independent points. It also has the same root cause as theirans104.rs:78finding and as the enforcing-bundler test here, which is that our fixture verifies with the same code that signs, so it can only ever confirm our own convention. Whatever the fix, prove it against something we did not write. -
[P2] The tamper tests fail 1 run in 64, and their
!validassertion is not load-bearing
crates/gitlawb-node/src/arweave.rs:1501and:1586
Same defect jatmn flagged, with the measurement behind it: I ran the compiled test 300 times and it failed 5, matching the 1-in-64 you would expect from a leading base64 character survivingformat!("A{}", &signature[1..]). Both call sites use the idiom, not just the one that went red. The part worth adding is that when the tamper is a no-op the first assertion still passes, because the placeholder-pool lookup errors and drivesvalidfalse on its own. So the test currently proves nothing in either branch: on a no-op run it passes for an unrelated reason. Fixing the tamper is necessary but not sufficient; assert on the specific signature error, not just on!valid. -
[P3] Correct the v1 migration note
crates/gitlawb-node/src/db/mod.rs:466
The note still says v1 includesseq,prev, andpusher_sig"for development convenience". This round moved all three to v18, and the v1 region no longer mentions them. The rest of the note (cert_id in v18, the proof columns in v19) is accurate. -
[P3] Mask the DB error the same way the adjacent branch already does
crates/gitlawb-node/src/arweave.rs:505
Supporting detail for jatmn's redaction finding, on a path their list does not name. The new fail-closed branch interpolates the raw error into the response withrepo lookup failed for {} ... {e}, while the cert-lookup branch twenty lines down deliberately omits it (error looking up certificate {} in node database). That text reaches an unauthenticated caller:verify_anchor_endpointis registered atserver.rs:242-250behind a rate-limit layer only, neveradd_auth_layers, andapi/arweave.rs:51returnserrorsverbatim. I have not confirmed what a failed sqlx lookup actually prints, so treat this as the masking inconsistency it plainly is rather than a demonstrated credential leak.
Two things that are not asks. The v20 drop of idx_ref_certs_repo_ref is settled: append-only requires multiple rows per (repo_id, ref_name) and a unique index forbids them, so keeping it through a rollout window is not something the feature can accommodate. The head's insert carries no ON CONFLICT clause, so nothing here depends on it. Say plainly in the migration comment that v20 is one-way and that rolling back to an older binary is unsupported. Separately, #173 claims v18-v23 and this branch uses v17-v20, so whichever lands second will need to renumber.
…tion, detached post-receive continuation
- ans104: tags preimage is the spec nested [[name,value]] list, not Avro tag
bytes; deep_hash is recursive; regenerate 0-tags vector and add an interop
fixture signed by arbundles' deepHash + Node crypto.
- tamper: b64url-decode the sig, flip a byte, re-encode, assert the specific
signature error for both the 13-field and 7-field verify paths.
- payer: require GITLAWB_BUNDLER_TOKEN alongside the account; send
x-irys-paid-by and upload to /tx/{token}; make .env.example startable and
gate it with a config test reading the shipped file.
- urls: structural join_path helper preserving query and rejecting fragments on
both bundler and gateway URLs; redact creds and truncate error bodies; mask
the raw DB error at the fail-closed repo lookup.
- continuation: move record_push, trust score, and per-ref certificate
issuance into an owned post_receive_continuation spawned at the durability
boundary, so a disconnect after the pack lands can no longer drop certs or
the replication tail; add post_receive_continuation_survives_handler_abort
and update the U5 ordering gate.
- migrations: note v20's one-way drop and fix the stale v1 comment.
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] Keep the legacy Irys configuration startable
crates/gitlawb-node/src/main.rs:76
A deployed node using the previously documentedGITLAWB_IRYS_URL=https://devnet.irys.xyzenters the fallback at lines 76–82, which assigns that value tobundler_url.Config::validate()then rejects it at lines 660–680 because the old deployment has neitherGITLAWB_BUNDLER_ACCOUNTnorGITLAWB_BUNDLER_TOKEN. The process exits before it can serve traffic, despite the fallback calling this a compatibility path. The root cause is treating a renamed setting as backward-compatible while imposing a new cross-field startup invariant on its existing values. Preserve an actually compatible legacy configuration path, or make the migration explicit: document the required new variables and avoid activating the new invariant for a legacy value until the operator has opted into the new configuration. -
[P1] Do not turn credential-bearing bundler failures back into log disclosures
crates/gitlawb-node/src/arweave.rs:127
Thesend()transport-error branch masksurl, but a 4xx/5xx response is converted intoBundler returned {status}: {body}at lines 127–130 (and again at lines 234–237). That error reaches theerr = %elogging path inapi/repos.rs; a credential-bearing bundler URL is explicitly supported by this PR, and a proxy/bundler can reflect its request URL—including userinfo or a query token—in its response body. The public verification route has the same omission for a mid-streambytes_stream()error at line 401, where the raw reqwest error is returned in the response body. The root cause is sanitizing only the initialsend()error instead of establishing a single safe error boundary for all URL-bearing transport and remote-response failures. Centralize error construction so every raw URL and reqwest error is masked before it can be returned or logged; do not interpolate remote response bodies unless they are redacted as well. Add credential-bearing fixtures for non-success bodies and interrupted response streams. -
[P1] Give successful pushes durable ownership of their certificates and tail
crates/gitlawb-node/src/api/repos.rs:2043
receive-packcan succeed, spawnpost_receive_continuation, release the guard, and return success before the continuation reachesrecord_pushat line 2174 or certificate issuance at line 2189. Tokio cancels spawned tasks during a process restart or shutdown, so a crash in that interval leaves Git's on-disk ref update durable but omits its certificate, trust/accounting rows, anchor, and follow-up replication work with no recovery record. The replication tail was already detached, but the certificate/accounting work was awaited before the successful response on the base branch; moving it here creates this new loss window. The root cause is using an in-memory detached task as the durable owner of work that must survive an acknowledged write. Persist a recoverable post-receive job before acknowledging the push and run it idempotently on startup, or keep the certificate-critical transaction in the acknowledged path and only detach effects that can safely be retried. -
[P2] Document the required funding settings alongside the bundler URL
README.md:361
The settings table presentsGITLAWB_BUNDLER_URLas the switch that enables anchoring, butConfig::validate()rejects every nonempty URL unless bothGITLAWB_BUNDLER_ACCOUNTandGITLAWB_BUNDLER_TOKENare supplied. An operator who follows the README's canonical configuration reference therefore gets a non-starting node and is not told how to satisfy the error. The root cause is updating the sample environment file without updating the corresponding operator-facing configuration contract. Document both companion variables, state that all three are required as a set, and explain their wire use (/tx/{token}andx-irys-paid-by) next to the URL setting rather than leaving the requirements discoverable only from startup failure or.env.examplecomments. -
[P3] Correct the migration-history note
crates/gitlawb-node/src/db/mod.rs:470
The note says v1 containsseq,prev, andpusher_sig, but the immutable v1 schema has none of those columns: v18 adds all three, and v19 adds the remaining proof columns. This directly contradicts the PR's migration-immutability claim and gives operators the wrong history when they inspectschema_migrationsor debug an upgrade. The root cause is leaving the explanatory note from the earlier, invalid attempt to modify v1 after restoring the actual v1 SQL. Update the note to identify v18/v19 as the append-only schema changes, and keep comments tied to the migration definitions they describe.
Overall guidance
This PR is operating across several contracts that have to agree at once: the operator configuration and upgrade path, signed upload and verification boundaries, logging/redaction policy, database migration history, and the durability semantics of a successful Git push. The recurring findings come from repairing one local symptom without first making the complete contract—and its failure behavior—explicit. For example, the legacy environment variable is accepted at parsing time but rejected later by startup validation; URL masking is applied to one reqwest branch but not response bodies or streamed-body failures; and work is detached to survive a client disconnect but is not made durable against the process interruption that a successful push must also tolerate.
Please take one pass over the feature as a set of explicit invariants before making another narrow fix:
-
Define the operator contract in one place. Specify the supported legacy configurations, the new required configuration tuple, the migration/deprecation behavior, and exactly which values are secrets. Make
Config::validate(),.env.example, README, startup logs, and tests all implement that same contract. A checked-in example and a legacy-upgrade fixture should both parse and validate, rather than relying on prose or a fallback warning. -
Treat every configured external URL and every value derived from it as sensitive until it crosses one centralized safe-display/error boundary. Apply that boundary to connection errors, body-stream errors, response bodies, JSON-decode errors, API responses, and logs. Redaction tests should use a userinfo-plus-query credential and exercise each failure phase, not only the initial connection failure.
-
Define the durable-success boundary for
receive-packseparately from the client-cancellation boundary. Once Git has accepted a pack and the node returns success, any certificate/audit/anchor work required to describe that push needs a durable, idempotent recovery mechanism. A baretokio::spawnis useful for latency and cancellation isolation, but it is not a queue and provides no restart guarantee. Persist a job or other recovery record before acknowledgement, then test cancellation, restart/recovery, duplicate delivery, and partial completion. -
Keep migration history and compatibility as executable facts. Never rely on comments to describe a schema version that the actual v1 SQL does not create; add upgrade fixtures from each supported released schema and assert the resulting tables, indexes, and writes. When a migration is intentionally one-way, document the operational downgrade boundary next to the concrete migration rather than relying on a broad catalogue note.
-
Prefer boundary-level tests over implementation round trips. The strongest tests here should cross the same boundaries production does: an old environment into startup, a credentialed mock server into every error surface, a successful receive-pack through restart recovery, and a real persisted migration history into current writes. Tests that only use helpers from the same module are valuable unit coverage, but they do not establish that the surrounding operational contract is intact.
Addressing these as a cohesive pass should make the remaining work much more reviewable and prevent another cycle where each narrowly fixed review comment exposes the next unmodeled boundary.
Superseded: re-reviewed on the current head 4b2aeb4.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Verification of the anchoring work holds up: the verify endpoint checks the certificate against this node's own DID rather than one the artifact carries, a mismatch fails closed, and the verdict is computed from the error list in one place. The 1 MiB cap now runs over the stream before anything is buffered, and a non-JSON payload comes back as an invalid result instead of a 500. Those were the right fixes.
One finding from me. jatmn's round from earlier today is against this same head and I am not restating it.
Findings
-
[P1] Sign the tags element as the serialized tag blob, not a nested list
crates/gitlawb-node/src/ans104.rs:89
This round changed the tags element of the deep-hash preimage from the serialized tag bytes to a nested list of[name, value]blobs. Both published bundler libraries do the opposite:getSignatureDatahashesitem.rawTags, the flat serialized blob, as the seventh element. Running the library's owndeepHashover the realrawTagsfrom the fixture in this file gives71a8ec4739f096c1...for the flat form and06342d2d966d7e0c...for the nested one, and the fixture's embedded Ed25519 signature verifies against the nested value and fails against the flat one. So data items this node signs will not verify under the bundler toolchain that receives them. The nested form is what Arweave layer-one transactions use for tags; data items are specified differently.Nothing in the suite can catch this, which explains the green run and is the part worth fixing alongside the preimage. The comment at
crates/gitlawb-node/src/ans104.rs:399describes the fixture as produced by an independent implementation,@irys/arbundles, but that package is not on the registry (npm viewreturns 404), and the fixture verifies only under the preimage this module itself produces. The enforcement bundler in the tests closes the same loop:crates/gitlawb-node/src/arweave.rs:968decides whether to reject an upload by callingcrate::ans104::verify_data_item, so the mock accepts whatever this module signs no matter what the real bundler would do. A fixture and a mock both derived from the implementation under test will agree with it in every case. Please regenerate the fixture witharbundlesor@irys/bundlesso the expected hash comes from the library rather than from this module, and do the same for the hand-written vector that moved in this change.One consequence for the rollout:
verify_data_itemchanged with the signer, so any anchor already written to Arweave under the previous serialized-blob preimage will now fail verification. Worth deciding whether the verifier needs to accept both forms.
Separately, on migration numbering, nothing for you to do. This branch's v18/v19/v20 collide with three of the nine versions #173 adds, and the runner keys the applied check on the version number alone, so whichever landed second would be skipped without an error. #224 keeps 18/19/20 and I will renumber #173 before it merges.
…tags, funded-account bundler docs Post-receive work becomes durable: git_receive_pack persists a post_receive_jobs row (id, pusher DID, repo, ref updates, RFC 9421 attestation) BEFORE acking the push, so a crash between the pack landing and the bookkeeping (record_push, trust score, certs, replication tail) is recoverable instead of dropping a durable push with no record. Startup drains rows a previous process left processing/failed and replays them; every effect is idempotent so a replay is safe: - push_events is keyed on the job id (ON CONFLICT (id) DO NOTHING) so a replay never double-counts the push - certificate ids are deterministic per (job, ref) and insert_ref_certificate_tx is idempotent, so a replay cannot mint a second certificate - the Arweave anchor upload is gated on an existence check for the exact transition, so a replay cannot write a second permanent artifact Also lands the reviewed ANS-104 flat-tags preimage (arbundles getSignatureData semantics, empty-tag reference vector), legacy GITLAWB_IRYS_URL adoption gated on the funded account/token pair, the centralized redaction boundary for bundler credentials in errors, the immutable-v1 migration note, and the README GITLAWB_BUNDLER_ACCOUNT/GITLAWB_BUNDLER_TOKEN rows. Refs Gitlawb#224
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] Stop inferring the verify gateway from the bundler upload URL on production configs
crates/gitlawb-node/src/main.rs:116What happens: An operator sets
GITLAWB_BUNDLER_URL=https://node2.irys.xyz(or enables the legacyGITLAWB_IRYS_URLalias with the funded-account pair) but does not setGITLAWB_ARWEAVE_GATEWAY. Clap still has the defaulthttps://arweave.net, butarweave_gateway_explicitly_settreats that default as “not explicit” because it only checksValueSource::DefaultValue. Startup then overwritesconfig.arweave_gatewaywith the bundler URL (main.rs:119). Uploads go to{bundler}/tx/{token};/api/v1/arweave/verify/:tx_idfetches{bundler}/{tx_id}instead of{arweave.net}/{tx_id}. Anchors that uploaded successfully return gateway errors or non-JSON bodies from the verify endpoint even though the on-chain artifact is fine.Root cause: Upload and verify are two different services (bundler vs data gateway), but startup conflates them whenever the operator relies on the documented default gateway. The inference was added to help devnet (where bundler and gateway share a host), but it fires equally on mainnet configs where README still says the gateway defaults to
arweave.net(README.md:364).How to fix: Pick one contract and enforce it everywhere:
- Preferred: Remove automatic overwrite in
main.rs:116-123. When anchoring is enabled, require an explicitGITLAWB_ARWEAVE_GATEWAYinConfig::validate()(same way you already requireGITLAWB_BUNDLER_ACCOUNT+GITLAWB_BUNDLER_TOKEN). Fail fast with a message that names both URLs and which network each must use. - If devnet convenience must stay: Gate inference on a detectable devnet bundler host only, never on mainnet hosts like
node2.irys.xyz. Log loudly when inference runs. - Add a config test: bundler URL set + gateway left at clap default → startup error (or explicit inference only for devnet), and a verify integration test that uploads via bundler mock and fetches via a separate gateway mock.
- Preferred: Remove automatic overwrite in
-
[P2] Align the PR description with what the durable job actually waits for
crates/gitlawb-node/src/api/repos.rs:2325What happens: The PR says a crash cannot drop “its replication tail.” In code,
run_post_receive_jobawaitspost_receive_replication_tail, but that functiontokio::spawns the Pinata/gossip/Arweave worker atrepos.rs:2631and returns immediately.process_post_receive_jobthen marks thepost_receive_jobsrowdonewhile the inner task may not have started or finished. A process restart afterdonewill not replay Arweave uploads,arweave_anchorsrows, Pinata pins, gossip, or peer notifications — even though the push was already acknowledged and the job looks complete.Root cause: This is a layering mismatch, not a brand-new detach pattern. The detached tail spawn predates this PR (
#174 F2; onmainthe handler also spawnedpost_receive_replication_tailfire-and-forget). What changed is that a durable job row now exists and reaches terminaldonewhile the inner tail is still in flight, so operators and the PR description infer durability the state machine does not actually provide. The job correctly recoversrecord_push_joband certificate issuance; it does not recover the inner tail.How to fix: Choose one honest contract:
- If the tail must be crash-durable (matches current PR text): Refactor so
run_post_receive_jobdoes not returnOk(())until every must-not-lose tail effect completes. Concretely: eitherawaitthe innertokio::spawnhandle frompost_receive_replication_tail, or move Arweave anchoring (and any other permanent effects) into the synchronous job body behind the existing idempotent guards (arweave_anchor_exists, deterministic cert ids). Only then mark the jobdone. - If only certs/accounting need durability (acceptable for this PR): Narrow the PR description, migration notes, and
db/mod.rscomments to say the job recovers push accounting and certificate issuance, not Pinata/gossip/Arweave. Do not mark the jobdonein a way that implies the tail finished. - Add a test that enqueues a job, lets
post_receive_replication_tailspawn, kills the process before the inner task completes, and asserts either (a) the job stays non-terminal and replays the tail, or (b) the documented contract explicitly excludes tail replay.
- If the tail must be crash-durable (matches current PR text): Refactor so
-
[P2] Correct the README wire contract for
GITLAWB_BUNDLER_TOKEN
README.md:363What happens: README says
GITLAWB_BUNDLER_TOKENis an “API key” sent asx-irys-paid-bywith the account. Operators configure a secret in the wrong place. Uploads fail with balance/token errors that look like funding problems when the real issue is wiring: the account goes inx-irys-paid-by, and the payment-token slug (e.g.matic) goes in the URL path/tx/{token}(arweave.rs:113-118,config.rs:142-146).Root cause: README was not updated when the upload path moved to the Irys
/tx/{token}+x-irys-paid-bymodel..env.exampleandconfig.rsalready describe the correct contract; README contradicts them.How to fix: Update
README.mdtable row forGITLAWB_BUNDLER_TOKENto matchconfig.rs:142-146and.env.example: it is the payment-token slug the funded account holds, used in the upload URL path, not thex-irys-paid-byheader. Add a one-line note besideGITLAWB_BUNDLER_ACCOUNTandGITLAWB_BUNDLER_URLthat all three must be set together. Optionally extendconfig::tests::env_example_bundler_block_is_startableor add a README-focused test that greps for the forbidden “API key” / “x-irys-paid-by” pairing for the token row.
-
[P2] Treat a successful bundler upload with a failed
record_arweave_anchoras retriable work
crates/gitlawb-node/src/api/repos.rs:2808What happens:
anchor_ref_updatereturns a non-emptytx_id, butrecord_arweave_anchorfails. The code logstracing::warn!and continues (repos.rs:2823). The permanent on-chain artifact exists, but the node has noarweave_anchorsrow,arweave_anchor_existsreturns false, and a later replay can upload a second artifact for the same ref transition. Because this runs inside the detached inner tail and the job may already bedone, there is no automatic recovery.Root cause: Upload and DB persist are treated as independent best-effort steps with no shared failure domain. The idempotency gate (
arweave_anchor_exists) keys off the DB row, so a successful upload without a row breaks the replay story the PR describes.How to fix:
- Treat
(upload succeeded, DB write failed)as a failed unit of work: bubble an error up from the tail (or inner task) so the job staysfailed/pendingand startup drain retries. - Alternatively, record the
tx_idin a durable intermediate column or outbox row before returning from the upload path, so replay can complete the DB insert without re-uploading. - Add a test: mock bundler returns
tx_id, DB insert fails → job is not terminal and a replay does not call the bundler again once the row exists.
- Treat
-
[P2] Do not treat DB errors from
arweave_anchor_existsas “not anchored”
crates/gitlawb-node/src/api/repos.rs:2772What happens: Before uploading, the tail calls
arweave_anchor_exists(...).await.unwrap_or(false)(repos.rs:2775). A transient DB outage or pool error is indistinguishable from “no anchor yet,” so the code proceeds toanchor_ref_updateand spends bundler balance on a duplicate upload for a transition that may already be anchored.Root cause:
unwrap_or(false)optimizes for the happy path at the expense of correct failure semantics. Unknown is treated as false.How to fix: Propagate the
Resultfromarweave_anchor_exists. OnErr, log and skip the upload (or fail the job/tail task) rather than uploading. Match the fail-closed posture used elsewhere in this PR’s verify path. Add a test with a closed pool or injected DB error: assert no bundler upload is attempted.
-
[P3] Align shipped
.env.examplegateway guidance with code defaults
.env.example:67What happens: The startable example ships an active
GITLAWB_ARWEAVE_GATEWAY=https://devnet.irys.xyz. That makesarweave_gateway_explicitly_settrue, so bundler-pairing inference inmain.rs:116never runs. An operator who later uncomments a production bundler URL but leaves the copied devnet gateway will upload to mainnet and verify against devnet — every verify call returnsvalid: falsefor real anchors. The comment on line 66 also calls devnet the “default” while clap/README default tohttps://arweave.net.Root cause:
.env.exampleoptimizes for a devnet quick-start but ships a value that becomes a silent footgun when the operator moves to production piecemeal.How to fix: Leave
GITLAWB_ARWEAVE_GATEWAYcommented out in the startable block (so inference or explicit operator choice applies), or split the example into clearly labeled devnet vs production blocks with a comment that bundler and gateway must be switched together. Extendenv_example_bundler_block_is_startableor add a companion test that the startable example does not pin a gateway when bundler is disabled.
-
[P3] Honor the documented default limit for
GET /api/v1/arweave/anchors
crates/gitlawb-node/src/api/arweave.rs:74What happens: The comment says “treat anything below 1 as the default,” but
q.limit.clamp(1, 200)turnslimit=0into1, not the serde default of50(default_limit()at line 65).Root cause: Comment and implementation diverged when clamping was added to avoid Postgres
LIMIT -1.How to fix: Replace
clamp(1, 200)with logic such aslet limit = if q.limit < 1 { default_limit() } else { q.limit.min(200) };, or change the comment to say sub-1 values clamp to1. Add a one-line handler test for?limit=0.
Not treated as blocking defects
These came up during review but are intentional tradeoffs, unproven without live gateway tests, or alpha-scope limitations rather than regressions introduced here:
- Certificate issuance fail-open (
repos.rs:2010): the code explicitly documents that issuance errors are logged and skipped so a cert outage degrades to a cert-less announce rather than a dropped push. That is policy, not an oversight; changing it is a product decision, not a bug fix. - Content-Digest now required (
auth/mod.rs:204): deliberate security hardening —mainpreviously substituted""for a missing header, which let proofs bind to no bytes. Worth a release note for third-party push clients, not a merge blocker. - ANS-104 decode on verify: upload wraps JSON in a signed data item, but verify only JSON-parses the gateway body. Tests mock plain JSON; whether production Irys gateways return raw item bytes vs decoded JSON was not demonstrated here. File a follow-up only after a live gateway fixture proves the mismatch.
- Multi-instance job drain without
SKIP LOCKED: same pattern assync_queue; relevant only if multiple nodes share one Postgres, which alpha may not support yet.
What landed well on this head
Head 6bf13d1e genuinely improves several earlier blocking areas: ANS-104 uses the flat serialized tag preimage with independent interop vectors (ans104.rs); legacy GITLAWB_IRYS_URL no longer prevents startup without the funded-account pair (main.rs:82-102); public gateway/bundler credential redaction drops userinfo/query/fragment (server.rs:606-628); and certificates plus record_push_job are now inside a persisted post-receive job enqueued before the push ack (repos.rs:2052-2090). That is real progress over main, where a disconnect during cert issuance could drop proof artifacts entirely.
Why these findings keep surfacing (and how to stop the drip)
This is not a “you missed seven small bugs” situation. The PR has been through six labeled review rounds plus several protocol pivots (unsigned JSON → signed ANS-104, signature-as-payment → funded account → /tx/{token} + x-irys-paid-by, fire-and-forget tail → post_receive_continuation → durable post_receive_jobs, nested deepHash → flat tags). Each round did close real defects — gateway data URL vs bundler API, per-ref cert binding, append-only chain, pusher proof persistence, body caps, migration backfill, credential redaction, and more are genuinely fixed on head 6bf13d1e. CI is green and the security-sensitive crypto paths are materially better than main.
What keeps generating new review feedback is a structural pattern: the branch keeps adding capabilities faster than it settles the cross-cutting contracts those capabilities depend on. Reviewers (human and bot) then file the next symptom at the newest layer. Fixing symptoms one at a time without locking the contracts first is why this feels endless.
The five patterns driving repeat rounds
1. Scope grew faster than the mental model
This PR is no longer “anchor ref updates to Arweave.” It is simultaneously: ANS-104 cryptography, a funded third-party upload protocol, a certificate chain with RFC 9421 corroboration, a public verify API, four migrations, operator config with legacy aliases, and a new durable job subsystem. Each pivot was necessary, but each one introduced new knobs (bundler URL, gateway URL, account, token slug, explicit-vs-inferred gateway) that must agree across config.rs, main.rs, README, .env.example, and the PR description. When those sources disagree — README still describes the token as an API key in x-irys-paid-by while code puts the slug in /tx/{token} — the next review round finds “another bug” that is really documentation debt from the last pivot.
2. Point fixes instead of boundary contracts
The commit history shows a healthy response to specific findings (prev linkage, seq backfill, streaming body cap, fail-closed corroboration). That work is real. What has not happened yet is writing down and enforcing the runtime contracts those fixes assume:
| Contract | What the code/docs currently imply | What actually happens |
|---|---|---|
| Upload vs verify | Two URLs on two services | Startup may collapse them via inference (main.rs:116) |
| Crash durability | “A crash cannot drop … replication tail” (PR summary) | Job reaches done while inner tail is still tokio::spawned (repos.rs:2631) |
| Anchor idempotency | “Gated on existence check; replay cannot double-anchor” (PR + db/mod.rs comments) |
unwrap_or(false) treats DB errors as “not anchored”; upload-without-row is warn-only |
| Operator setup | README default gateway arweave.net; .env.example pins devnet |
Copy-paste from either doc produces a different live config |
Until those four rows are one chosen truth enforced in Config::validate() and tested, every review pass will find another edge of the same mismatch.
3. Convenience logic that trades one footgun for another
Gateway inference (main.rs:116-123) is a good example of the pattern: it solves devnet (“bundler host can also serve data”) but silently breaks the documented mainnet default (“gateway is arweave.net”). The prior round added Irys gateway pairing for the same class of problem. Each convenience shortcut fixes the reporter’s scenario and creates a new misconfiguration path for the next operator. The durable fix is not another inference rule — it is fail fast when anchoring is enabled and the operator has not named both endpoints explicitly.
4. Tests prove components, not the operator story
The PR checklist is long and valuable, but most tests mock bundler and gateway as the same in-process server or exercise crypto in isolation. They do not currently prove:
- Production-shaped config: bundler at
node2.irys.xyz, gateway atarweave.net, no inference. - Durability boundary: job row terminal state vs inner tail completion (kill process mid-tail).
- Paid-work idempotency: DB down during
arweave_anchor_exists→ no upload attempted; upload OK + DB fail → job retries without second bundler call.
Without those three integration tests, each review relies on code reading to discover contract gaps — which is exactly the drip pattern you are trying to escape.
5. Layering without re-homing side effects
The durable job is the right direction, but it was wrapped around existing #174 tail architecture instead of re-homing permanent effects into the job boundary. Certificates and record_push_job moved inside; Arweave upload stayed in a nested spawn. That is why “job done before tail finishes” keeps reappearing even though it is not a new detach — the new durability primitive inherited the old fire-and-forget shape. The next review will not ask “did you spawn?” again; it will ask “does done mean what you say it means?” until you either await the inner work or rewrite the claims.
What a “last round” should look like
Do not treat this as seven unrelated nits. Treat it as one coordinated contract pass with four deliverables:
-
Write the operator contract in one place (
config.rsmodule docs +validate()), then make README,.env.example, and PR summary mechanically consistent (grep tests or snapshot tests are fine). Four knobs, one table, no inference on mainnet. -
Pick a durability scope and make the state machine honest. Either:
- Full tail durability:
run_post_receive_jobdoes not return until Arweave row + upload (and anything else you claim) complete; job status reflects that; startup drain retries incomplete work. - Certs-only durability: PR summary, migration comments, and
drain_post_receive_jobsdoc comment say so explicitly; jobdonemeans certs + push accounting only.
Mixing “full durability” marketing with “certs-only” implementation is the single biggest source of repeat findings.
- Full tail durability:
-
Unify anchoring failure semantics.
arweave_anchor_exists,anchor_ref_update, andrecord_arweave_anchorshare oneResultchain: DB unknown → no paid upload; upload OK + DB fail → job/tail fails and retries; only terminal success when both succeed. Deleteunwrap_or(false)and warn-only persist on this path. -
Add three integration tests that lock the contracts (listed above). Once those pass, point-fix rounds should stop because the contracts are executable, not aspirational.
What is not being asked again
To be explicit so this does not feel like moving goalposts: the following prior-round themes are resolved on this head and should not be re-litigated in the next push:
- ANS-104 flat-tags preimage and independent interop vectors
- Per-ref certificate binding and append-only
(repo_id, seq)chain - Pusher RFC 9421 context persisted and verified
- Gateway data URL (not bundler API path) for verify fetch
- Credential redaction on public outputs
- Legacy
GITLAWB_IRYS_URLgated on funded account/token pair - Cert + job enqueue before push ack (real improvement over
main)
The remaining seven findings are not “more of the same crypto bugs.” They are the integration and honesty debt left after those fixes landed. Closing them in one pass — contracts, state machine, failure domain, three tests — is how you stop the drip.
Overall guidance
The certificate-chain and verify-path hardening are in good shape, and CI is green. The remaining work clusters around three root themes (same as the contract table above):
-
Operator contract clarity — bundler URL, gateway URL, account, and payment-token slug are four distinct knobs; README,
.env.example, and startup inference must tell the same story. Fix the gateway inference footgun first; it silently breaks verify on the most common mainnet setup. -
Durability honesty — the post-receive job is a real improvement for certs and push accounting, but terminal
doneplus a nestedtokio::spawnin the tail overpromises crash recovery. Either extend the job boundary to cover permanent effects (Arweave row + upload) or narrow every operator-facing claim to what the job actually waits for. -
Fail-closed idempotency for anchoring —
arweave_anchor_existsandrecord_arweave_anchorshould share a single failure domain: unknown DB state must not trigger a paid re-upload, and upload-without-row must be retryable. Tightening these while you are already touchingrepos.rsanddb/mod.rsprevents silent double-spend on the bundler.
Addressing root causes in those three areas is preferable to adding more warnings: the code already warns in several of these paths, but operators and replay logic cannot act on warnings when the job row already says done.
If you want a merge path with minimal scope change: take option (2) for durability (narrow claims, keep current architecture), fix gateway inference and README (operator contract), and unify anchoring Result handling. That is three focused commits and three tests — not another month of round-by-round symptom fixes.
Superseded by my re-review at 6bf13d1.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-review at 6bf13d1e. CI is green with nothing pending, and the merge base is two commits behind main whose only change is gitlawb-core/src/cert.rs, which this branch does not touch.
The ANS-104 finding from my last round is closed, and I checked it against the real library rather than against the branch's own tests. With arbundles@0.11.2 installed, DataItem.verify() returns true on the new interop fixture, and its getSignatureData() is 7a37ca35d582..., the flat serialized-tag preimage. I recomputed the same deepHash independently in Python and got the same value; the nested form gives 0c9eb2f33d5e... and the fixture's signature does not verify against it. The empty-tags constant in deep_hash_matches_independent_reference_vector is byte-identical to what the library computes over the same eight elements. The fixture no longer agrees with the module merely because the module produced it, which was the substance of the ask.
I also ran the direction that decides whether uploads work: items this node signs, fed to the library. build_signed_data_item output for a three-tag ref-update item and for a zero-tag item both pass DataItem.verify(), and flipping one data byte makes it false, so the pass is not vacuous. The zero-tag case is the one I was least sure about, since an empty tag stream and an empty list deep-hash differently, and the library accepts the empty-blob form this module emits.
jatmn's round from earlier today is against this same head and I am not restating it. Two things on top, one of which is a design call rather than a defect.
Findings
-
[P1] Remove the gateway inference and require an explicit gateway when anchoring is enabled
crates/gitlawb-node/src/main.rs:117
Settling this rather than leaving it open: upload and verify are two different services, so startup should never derive one from the other. Drop the overwrite at lines 116-123 and add a rule toConfig::validate()alongside the existing account/token pair, so a non-emptyGITLAWB_BUNDLER_URLwith no explicitGITLAWB_ARWEAVE_GATEWAYfails at startup with a message naming both URLs and the network each has to point at. One extra variable is a smaller cost than a node that uploads to one host and verifies against another.
This also settles jatmn's.env.examplepoint in the other direction, so you are not getting two contradictory asks: keepGITLAWB_ARWEAVE_GATEWAY=https://devnet.irys.xyzset in the startable block, and add a matching#GITLAWB_ARWEAVE_GATEWAY=https://arweave.netto the commented production block so bundler and gateway are switched together. -
[P3] Attribute the dropped unique index to v10, not v1
crates/gitlawb-node/src/db/mod.rs:1031
idx_ref_certs_repo_refis created by migration v10 (ref_cert_unique_per_ref,db/mod.rs:952); v1 creates only the non-uniqueidx_ref_certs_repoonrepo_id. The drop is correct and so is the one-way rollback warning, but the wording sends anyone auditing the downgrade path to the wrong migration. Same fix needed in the schema note atdb/mod.rs:529.
Migration numbering is still mine and there is nothing for you to do. #173 has grown to v18-v26, so it now overlaps all four of this branch's v18-v21 rather than three; I renumber #173 before it merges and this branch keeps its block.
…explicit gateway Round-4 reviewer findings on Gitlawb#224: - Move Arweave anchoring out of the spawned replication tail and into the awaited post-receive job body: the tail now reports (announce, cid_map) over a oneshot channel, and anchor_ref_updates runs after the replication tail returns. A failed upload, an unpersistable row, or an unanswerable existence check now fails the job instead of leaking into the tail, so the startup drain retries the whole unit. The per-ref existence check keeps retries and job replays from paying for a second on-chain artifact. - Drop the implicit gateway default (arweave.net) and refuse to start with a bundler configured but no explicit GITLAWB_ARWEAVE_GATEWAY: the old behavior silently paired the gateway to the bundler URL, which broke /verify for production deployments (devnet transactions are not resolvable via arweave.net). Enforced in Config::validate() next to the existing ACCOUNT/TOKEN checks. - .env.example: split the commented devnet/production bundler blocks and document that anchoring needs URL + funded ACCOUNT + TOKEN + a gateway on the same network. - README: GITLAWB_BUNDLER_ACCOUNT is the funded payer (x-irys-paid-by), GITLAWB_BUNDLER_TOKEN is the payment-token slug billed at /tx/{token} (not an API key), GITLAWB_ARWEAVE_GATEWAY has no default. - anchors list: limit 0 now falls back to the default page size instead of returning nothing. - Tests: P4 unit (upload OK but row insert blocked -> job body errors, retry re-uploads exactly once, replay never re-calls the bundler), P5 unit (unanswerable existence check -> fail closed, no upload), and an end-to-end job test (bundler 500 -> job stays failed, drain retries, replay after the row exists never re-uploads).
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-review at edd40361. CI is green and the round's central moves are right: gateway inference is gone with a fail-fast validate() when a bundler is configured without one, the anchor is now a real awaited unit of the post-receive job with a fail-closed existence check that won't double-pay on replay, and ?limit=0 behaves as absent. One regression came in with the refactor.
Findings
-
[P1] Pass the node DID, not the pusher DID, into the anchor
crates/gitlawb-node/src/api/repos.rs:2352
run_post_receive_jobpassesdid(which isjob.pusher_did) asanchor_ref_updates'snode_did, so every anchor this head writes carries the pusher's DID innode_did. The certificate is issued from the node DID, andverify_anchorrejects when the outernode_diddiffers from the certificate's, so these anchors fail/verifypermanently and there is no fixing them after upload. The previous head usedstate.node_did.to_string()here. The new tests can't see it: they callanchor_ref_updatesdirectly with a literal node DID, so nothing exercises the call site. Pass&state.node_did.to_string()and assert the recorded row'snode_didmatches the node's. -
[P2] Omit
arweave_urlwhen no gateway is configured
crates/gitlawb-node/src/api/arweave.rs:99
The listing always formats{gateway}/{tx_id}, and the gateway has no default. A node with anchors but noGITLAWB_ARWEAVE_GATEWAY(allowed, sincevalidate()only requires it alongside a bundler) publishes"/<tx_id>"as the anchor URL. This migration also drops the storedarweave_urlcolumn, so the derived value is the only one a client gets. Skip the field when the gateway is empty; the new limit-zero test always sets one. -
[P3] Attribute the dropped index to v10, not v1
crates/gitlawb-node/src/db/mod.rs:1035
idx_ref_certs_repo_refis created by migration v10 (ref_cert_unique_per_ref), not by v1. Same wording at:533. This carried over from the last round.
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.
Overall guidance
This PR is trying to harden several security- and durability-critical contracts at once: certificate authorization, HTTP-signature provenance, database migrations, an asynchronous post-receive job, paid third-party uploads, recovery after process failure, public API responses, CLI verification, and the operator security model. The recurring problem is not a missing guard in one function; it is that the new cross-boundary invariants are described in comments and tests, but are not represented as enforceable state transitions at the owning boundaries.
Please take a consolidation pass before another review round rather than addressing these as isolated line edits:
-
Establish the authoritative contracts first.
- Write down the exact identity roles for every artifact: repository owner, authenticated pusher, node issuer, anchor author, funded bundler account, and gateway. Each serialized field should have one owner and be passed by a role-specific name, not through an ambiguous
didvariable. - State which post-receive outputs are best-effort and which are durable obligations. If certificates and ref anchors are required outputs of a landed push, failure to create either must be a durable, observable retry state—not a warning followed by
done. - Treat certificate threshold authorization, configured visibility, and the documented security boundary as public contracts. Do not weaken a mature guard or delete its adversarial regression test while changing unrelated anchoring code.
- Write down the exact identity roles for every artifact: repository owner, authenticated pusher, node issuer, anchor author, funded bundler account, and gateway. Each serialized field should have one owner and be passed by a role-specific name, not through an ambiguous
-
Design the paid-upload workflow as a durable state machine, not a retry wrapper around an HTTP call.
- A database read before an external side effect cannot provide exactly-once behavior. The database needs a unique per-transition record and an atomic claim/reservation before any upload is attempted.
- Separate states such as
pending,claimed,uploading,uploaded(tx_id),recorded, andfailed, with ownership/lease information suitable for crash recovery. The recovery path must know whether it may submit an upload, must resume a known upload, or must reconcile a previously submitted request. - Make concurrent workers converge through conditional updates/row locks and uniqueness constraints. Do not rely on in-process spawning or a non-locking
SELECTfor ownership when startup recovery can run from more than one process. - Use a provider-supported idempotency key where available. If the provider cannot provide one, retain enough durable request identity and response state to reconcile uncertain outcomes without charging for a second permanent artifact.
- Define and test interruption points explicitly: before reservation, after reservation, during upload, after provider acceptance but before local persistence, after local persistence but before job completion, and while another worker owns the same job.
-
Make tests exercise the real production continuation, including failure paths.
- The direct helper tests supplied with a literal node DID did not cover the
run_post_receive_jobcall site that supplied the pusher DID. Prefer a test that queues a job, processes it, and verifies the stored and serialized anchor against the node identity. - Every advertised idempotency or recovery property needs an adversarial test. A test that observes two paid uploads after a failed insert is evidence that the property is not achieved, not proof that retry is safe.
- Add fault injection for certificate issuance, each database write, malformed provider responses, dropped task channels, crash/restart replay, and two concurrent drainers. Assert both externally visible output and durable database state.
- Retain security regression tests when refactoring. In particular, threshold tests must include repeated entries for the same valid signer; happy-path signatures alone cannot prove a multi-party authorization rule.
- The direct helper tests supplied with a literal node DID did not cover the
-
Put validation at the boundary that receives untrusted or optional data.
- Validate a bundler response completely before calling it success: transaction IDs must have the expected non-empty format, and malformed success responses must leave retryable work.
- Keep optional configuration separate from derived output. A stored transaction ID remains meaningful with no gateway; a presentation URL must only be emitted when it is absolute and constructible.
- Validate serialized claims against the trusted source at each boundary. For example, an anchor's issuer is the node identity, not the pusher identity merely because the pusher triggered the job.
-
Reduce and verify documentation drift as part of the implementation.
- The README,
.env.example, module rustdoc, configuration validation, SECURITY.md, and runtime behavior must describe one coherent deployment contract. Changes to defaults, token formats, authorization behavior, or visibility guarantees should be checked against all of them in the same patch. - Security documentation should distinguish implemented guarantees, known limitations, and planned work. It must never substitute an intended architecture for current runtime behavior.
- Migration commentary is operational documentation: cite the exact migration that created a schema object and preserve upgrade/downgrade accuracy.
- The README,
-
Split follow-up work where that is the safer way to establish confidence.
- The durable job/outbox/idempotency redesign, certificate-format and verification changes, and broad security/documentation rewrite each have enough independent risk to deserve focused review and dedicated tests.
- If this PR keeps all of them, keep the implementation organized around small, named invariants with tests that prove each one. Avoid mixing behavior changes, broad comment rewrites, and unrelated security-policy edits without a direct source/test link.
The goal of this pass should be to make the system explain and enforce its own invariants: a reviewer should be able to follow an artifact or job from creation through every failure and restart path, and find one durable answer to who owns it, whether it was completed, and whether it is safe to retry.
Findings
-
[P1] Count distinct maintainers for certificate thresholds
crates/gitlawb-core/src/cert.rs:138
verify_all()returns one entry for every element in the untrustedsignaturesarray. This change replaces the base branch'sHashSetof authorized signer DIDs with a raw entry count, while deleting the regression test for duplicated signatures. A caller can therefore copy one valid maintainerCertSignatureN times and satisfy an N-of-M policy with one key.Restore distinct-DID counting at the authorization boundary (rather than trying to reject only byte-identical duplicates), and restore a test that serializes repeated valid signatures from one signer against a threshold greater than one. That keeps the invariant correct for equivalent-but-differently-encoded duplicate entries as well.
-
[P1] Pass the node DID to the anchor writer
crates/gitlawb-node/src/api/repos.rs:2352
didis assigned fromjob.pusher_did, but is passed asnode_didtoanchor_ref_updates. The resulting anchor and database row name the pusher as the node, while the embedded certificate is signed withstate.node_did.verify_anchorcorrectly compares the outer value with the certificate issuer, so every anchor created through the durable-job path is permanently invalid after it has been paid for and published.Keep actor identities explicit through this continuation: pass
state.node_didfor the anchor issuer and retainjob.pusher_didonly for the pusher/provenance fields. Add an end-to-end durable-job test that inspects the emitted anchor, not merely a directanchor_ref_updatesunit test supplied with a literal node DID. -
[P1] Do not complete the durable job when issuing a certificate fails
crates/gitlawb-node/src/api/repos.rs:2300
Certificate issuance errors are only logged. The missing map entry then makesanchor_ref_updatesskip that ref, returnOk(()), and letsprocess_post_receive_jobmark the rowdone. A transient certificate insert/sequence/DB failure after the Git push lands therefore loses both the per-ref certificate and its durable anchor permanently: startup recovery only revisits pending or failed rows.Model certificate creation and anchoring as required stages of the same durable job. Propagate certificate-stage failure so the job remains retryable, or persist per-ref stage state and only mark the parent job complete once every required ref reaches its terminal durable state. Add a failure-injection test that proves a certificate error leaves work for the startup drain.
-
[P1] Make paid anchor retries atomic with their durable state
crates/gitlawb-node/src/api/repos.rs:2438
The current "idempotency" check is a non-atomicSELECT, followed by a paid remote upload, followed by an insert. If the insert fails or the process crashes after the upload, the retry finds no row and uploads—and pays—for the same transition again; the new test explicitly observes its counter move from one to two. It also races between workers: startup draining selects jobs without an atomic claim, and there is no unique transition reservation, so two instances can both observe no anchor before either records one.Address the root distributed-systems boundary rather than adding another post-upload existence check. Create/claim a unique, durable per-transition outbox state before uploading; make job claiming conditional/locked; and use a stable remote idempotency key or recovery lookup tied to that state. The DB uniqueness constraint and state transitions must make competing workers converge, while restart recovery must be able to decide whether an upload happened before it attempts another paid request. Add tests for an insert failure after a successful upload, process interruption in that interval, and two simultaneous workers.
-
[P2] Reject an empty bundler transaction ID
crates/gitlawb-node/src/arweave.rs:140
Any stringid, including"", is treated as a successful upload. The caller then silentlycontinues on the empty value and marks the durable job done without an anchor row or retry record. A proxy, incompatible bundler, or otherwise malformed successful response can therefore turn a required anchor into a silent no-op.Validate the provider response at the client boundary: require the expected non-empty Arweave ID format before returning
Ok, and treat invalid responses as retryable job failures. Cover empty, missing, and malformed IDs in the upload-client tests. -
[P2] Omit the derived anchor URL when no gateway is configured
crates/gitlawb-node/src/api/arweave.rs:99
The new configuration deliberately permits an empty gateway when anchoring is disabled, while existing anchor rows remain listable. This unconditionally publishes"/<tx_id>"asarweave_url, which is a broken relative URL that clients can mistakenly resolve against the node's own origin.Keep the durable transaction identifier separate from an optional presentation URL. Emit
arweave_urlonly when a configured gateway can form an absolute URL, and add coverage for a node with existing rows but no gateway. -
[P2] Correct the security policy to describe the implemented boundary
SECURITY.md:31
This revision calls UCANs JWTs even though the runtime emits a signed JSON{ payload, signature }envelope, says the middleware does not validate proof chains although it callsverify_chain, and claims writes are authorized even though owner-push enforcement defaults off. It also replaces the implemented repo/path visibility boundary with an assertion that no private-read enforcement exists. These claims can cause clients to send an unsupported token format and operators to make unsafe deployment decisions.Derive this policy from the current runtime contract, not an intended v0.2 design: document the actual UCAN wire format and chain checks, state the remaining issuer/capability and owner-push limitations precisely, and preserve the visibility guarantees and their documented exceptions. Add a documentation check or an owner-reviewed source-of-truth process for security claims that are easy to invalidate through refactors.
-
[P2] Update the Arweave module's gateway documentation
crates/gitlawb-node/src/arweave.rs:28
The rustdoc still says the gateway defaults tohttps://arweave.net, but this PR removes that default andConfig::validate()rejects a configured bundler without an explicit gateway. An operator following the module documentation can deploy a configuration that now fails startup.Make the docs describe the same configuration invariant enforced by
Config::validate(): a bundler requires an explicit gateway for the matching network, while a non-anchoring node may leave it unset. Keep this text aligned with.env.exampleand README configuration tables. -
[P3] Attribute the dropped index to the migration that created it
crates/gitlawb-node/src/db/mod.rs:1035
idx_ref_certs_repo_refis created by migration v10, not v1. The v20 downgrade warning currently points operators at the wrong migration history, which makes an already risky one-way migration harder to audit during incident response or rollback planning.Correct this and the adjacent wording to refer to v10, and keep migration commentary tied to the catalogue entry that actually created or removed each schema object.
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-256prevlinkage, 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
What changed
gitlawb-node
item.rawTagsin the publishedarbundlesgetSignatureData), 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, notdeepHash([]). The empty-tags reference vector and a 3-tag interop fixture are produced by the independentarbundlespackage (createData+sign) and pinned as hex in tests; the node's own signer produces items this verifier accepts.{bundler}/tx/{token}as raw signed data items with metadata embedded as tags, paying via thex-irys-paid-byheader (IrysUploadHeaders.PAID_BY).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 throughremote_send_error/remote_response_errorso 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.record_push, trust score, per-refissue_ref_certificate) and the Arweave anchor now run inside a durable post-receive job.git_receive_packpersists the job row (id, pusher DID, repo, ref updates, RFC 9421 attestation) BEFORE acknowledging the push, then spawnsprocess_post_receive_job; a crash between the pack landing and the bookkeeping is recovered by the startup drain (drain_post_receive_jobsin main), which resets stale rows topendingand replays them. 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, andanchor_ref_updatesruns in the job body only after that report arrives. Every effect is idempotent:push_eventsis keyed on the job id (ON CONFLICT (id) DO NOTHING), certificate ids are deterministic per (job, ref) withinsert_ref_certificate_txidempotent, and the Arweave anchor upload is gated on an exact-transition existence check — so a replay never double-counts, double-issues, or double-anchors. A failed anchor (upload error, unpersistable row, or unanswerable existence check) fails the job rather than leaking into the tail, so the drain retries the whole unit and the existence gate keeps the retry from paying twice.GITLAWB_BUNDLER_TOKENadded;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 explicitGITLAWB_ARWEAVE_GATEWAY: the implicitarweave.netdefault is gone, because it silently paired the gateway to the bundler URL and broke/verifyfor production deployments (devnet transactions are not resolvable viaarweave.net). The legacyGITLAWB_IRYS_URLis adopted vialegacy_bundler_url_fallbackonly 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).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.GITLAWB_BUNDLER_ACCOUNTis the funded account that pays (sent asx-irys-paid-by),GITLAWB_BUNDLER_TOKENis the payment-token slug billed at/tx/{token}(it is NOT an API key and is not sent in the paid-by header), andGITLAWB_ARWEAVE_GATEWAYhas no default.RefCertificategainsseq/prev/pusher_sig/signature_input/content_digest/request_path;arweave_anchorsgainscert_id, renamesirys_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), and v21 adds thepost_receive_jobstable. An upgrade test replays the deployed v1 schema.mask_credential_urldrops 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 structuraljoin_url_paththat preserves the query and rejects fragments; reqwest errors have the URL redacted and bodies truncated.Reviewer checklist coverage
verify_data_item_matches_independent_interop_fixture; flat-tags referencedeep_hash_matches_independent_reference_vector.test_verify_anchor_rejects_tampered_13_field_signature,test_verify_anchor_rejects_tampered_7_field_signature.test_verify_anchor_fails_closed_when_outer_identity_cannot_be_corroborated; DB error masked at the fail-closed lookup.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.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.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).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_upload_ok_but_db_row_fails_is_retried_without_double_pay(upload lands but the row cannot be written → job body errors; the retry re-uploads exactly once; a replay after the row exists never re-calls the bundler),anchor_existence_check_failure_never_uploads(unanswerable existence check → fail closed, no upload),post_receive_job_anchor_failure_retries_and_replay_never_reuploads(bundler 500 → job staysfailed, the drain retries todone, and replay never pays twice).list_anchors_limit_zero_uses_default_limit(limit=0falls back to the default page size instead of returning nothing).upgrade_path_tests::upgrading_released_v1_schema_lands_cert_and_anchor_columns.How a reviewer can verify
DATABASE_URL=postgresql://gitlawb:changeme@localhost:5433/gitlawb cargo test --workspaceAll 1412 tests across the workspace pass (873 in the
gitlawb-nodesuite).cargo fmt --all -- --checkandcargo clippy --workspace --all-targets -- -D warningsare clean.