mirror of
https://github.com/Twigpine/node.git
synced 2026-11-10 05:01:48 -10:00
main
354
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
f68d0dd3ff |
fix(node): durable post-receive outbox for receive-pack (#26 split 1/4) (#558)
* fix(node): durable post-receive outbox at the DB layer (#26 split 1/4) Reviewer 2 closed PR #224 on 2026-08-28 with a directive: split the work into four narrow PRs. This is Split PR 1 (durable post-receive lifecycle) at the DB layer; the handler refactor in crates/gitlawb-node/src/api/repos.rs:2007 (git_receive_pack) lands in the next slice so the test can drive the failure injection end-to-end. The pre-outbox crash window the reviewer flagged: receive_pack can apply a ref to disk and return Ok, and a process exit, a dropped future, or a DB failure before the bookkeeping at crates/gitlawb-node/src/api/repos.rs:2361 (push event + cert + webhook) loses the recovery record. Startup drain enumerates only sources written from that bookkeeping, so it cannot reconstruct the missing work. The partial fallback that re-derives from a row present in the bookkeeping substitutes did:key:recovered and an empty attestation, which is not equivalent to the original authenticated push. This commit adds the durable boundary the handler will lean on. NEW TABLE pending_ref_transitions (migration v27): - Written by the handler BEFORE smart_http::receive_pack, in state 'prepared', carrying the verified pusher DID, the raw RFC 9421 signature header, signature-input, and content-digest that authorized the push, the request id, and the parsed ref update. - The handler transitions the row to 'applied' on receive_pack Ok or 'cancelled' on Err. The drain reads only 'applied'. - A failed or cancelled receive-pack therefore leaves the row in 'prepared' or 'cancelled', which the drain never promotes. This is what closes the reviewer's second proof ("a failed or cancelled receive-pack does not turn a prepared intent into completed accounting or anchoring"). NEW TABLE anchor_jobs (migration v27, owned by PR 1, consumed by PR 2): - One row per (repo_id, ref_name, old_sha, new_sha) transition. PR 1 inserts it on 'applied'; PR 2 reads it and updates claimed_at. - ON CONFLICT (id) DO NOTHING makes the insert idempotent on the deterministic id, so a recovery re-pass cannot create a second upload request. This is the handoff boundary; the bundler call itself is PR 2. NEW DB METHODS on Db: - insert_pending_ref_transitions: writes one 'prepared' row per ref update, returns the persisted rows. - mark_pending_ref_transitions_applied / _cancelled: state flip, gated on the FROM state, idempotent. - list_pending_ref_transitions_applied: drain query, oldest first. - delete_pending_ref_transition: called by recovery after the artifacts land; a third pass is a no-op. - record_push_with_id: ON CONFLICT (id) DO NOTHING on the deterministic id. - insert_ref_certificate_idempotent: ON CONFLICT (repo_id, ref_name) DO NOTHING, returns None if a live-path cert already exists. - insert_anchor_job_idempotent: ON CONFLICT (id) DO NOTHING on the deterministic per-transition id. NEW HELPERS in db/mod.rs: - deterministic_id: SHA-256 hex with an ASCII Unit Separator between fields so two distinct tuples never collide on prefix overlap. - push_event_id_for, ref_cert_id_for, anchor_job_id_for: the derived ids above, one helper per artifact so a caller cannot derive a wrong id by mistake. NEW STRUCTS: - PendingRefTransition: the row shape. - AnchorJob: the handoff row shape. - pending_state: const strings ('prepared' / 'applied' / 'cancelled') shared by tests, the producer, and the drain so a typo on one side cannot silently mismatch the other. NEW TESTS in db::pending_ref_transition_tests (8 tests, all green): - insert_then_mark_applied_flips_state_for_every_ref: producer contract. - mark_applied_is_idempotent_on_repeat: re-fire is a no-op. - cancelled_rows_are_not_returned_by_the_drain: reviewer's second proof at the DB layer. - prepared_rows_are_not_returned_by_the_drain: same proof for the pre-flip state (handler crashed before reaching post-Ok). - mark_cancelled_is_idempotent_on_repeat: counterpart. - drain_then_re_derive_is_idempotent: reviewer's first proof at the DB layer. Inserts a row in 'applied' state directly via insert_pending_ref_transition_for_test, drains it, derives the artifact ids twice, exercises record_push_with_id and insert_anchor_job_idempotent directly, asserts exactly one push event row and exactly one anchor job row regardless of how many times the drain runs. - deterministic_id_avoids_prefix_overlap_collisions: the separator regression test. - push_event_id_for_is_stable: derived ids match across calls and differ on each varied input. OTHER: - Make RefUpdate and its fields pub(crate) so the DB methods can iterate the parsed ref updates. No public API change. NOT IN THIS SLICE (the handler refactor, next commit): - The receive-pack handler does not yet call insert_pending_ref_ transitions before the receive_pack call, nor mark_applied / mark_cancelled after. The DB layer is in place for it; the handler will call these methods and the startup drain will be wired in main.rs. - The startup drain in main.rs is not yet called; it will iterate list_pending_ref_transitions_applied, re-derive the artifacts, and delete the row. - The cert/push event issuance in cert.rs and the bookkeeping in api/repos.rs:2361 are not yet changed to use the deterministic ids. The helper functions exist and are tested; the callers follow. Compiles clean, clippy clean under -D warnings, fmt clean. * fix(node): wire durable outbox into the receive-pack handler (#26 split 1/4) This is the handler-level half of Split PR 1. The previous commit added the migration and the DB methods; this one threads them through crates/gitlawb-node/src/api/repos.rs:2007 (git_receive_pack), the cert issuer, and the startup drain. CHANGES IN THE HANDLER ====================== In git_receive_pack, AT THE LAST POSSIBLE MOMENT before the smart_http::receive_pack call, the handler now: 1. Generates a per-handler request_id (UUID). 2. Captures the raw Signature, Signature-Input, and Content-Digest headers from the request. 3. Calls db.insert_pending_ref_transitions(request_id, ...) which writes one row per ref update in state 'prepared'. The receive_pack call runs as before. After it returns: 4. On Ok: db.mark_pending_ref_transitions_applied(request_id) — the row is the ONLY thing that promotes a 'prepared' row to 'applied', and the drain reads only 'applied' rows. A process crash before this call leaves the row in 'prepared', which the drain never promotes. 5. On Err: db.mark_pending_ref_transitions_cancelled(request_id) — a failed receive_pack leaves the row in 'cancelled', which the drain never promotes. This is what closes the reviewer's two proofs: Proof 1 (crash window): if the process dies after mark_pending_ref_transitions_applied but before the bookkeeping writes, the row is in 'applied' and the next startup drain re-derives the push event, the per-ref certificate (carrying the ORIGINAL pusher DID, not a placeholder), and the anchor handoff. The drain uses the persisted authentic pusher DID and signature header, not a recovered placeholder. Proof 2 (failed receive-pack): the row is only ever flipped to 'applied' in the explicit Ok branch above. A 'prepared' or 'cancelled' row is invisible to the drain, so a failed or dropped receive_pack cannot turn a prepared intent into completed accounting or anchoring. BOOKKEEPING IS NOW DETERMINISTIC-ID =================================== The post-Ok bookkeeping at api/repos.rs:2448 now uses: - record_push_with_id with push_event_id_for(request_id, first_ref) — ON CONFLICT (id) DO NOTHING, so a recovery re-pass is a no-op. - issue_ref_certificate_idempotent with ref_cert_id_for(request_id, ref_name) — ON CONFLICT (repo_id, ref_name) DO NOTHING, returns None if a live-path cert already exists. - insert_anchor_job_idempotent with anchor_job_id_for(repo_id, ref_name, old_sha, new_sha) — the per-transition tuple key, so two pushes to the same ref produce one anchor upload per landed state. The legacy entry points (record_push, issue_ref_certificate, insert_ref_certificate) remain for callers that prefer a fresh UUID per cert; they are #[allow(dead_code)] for the PR 3 cert/CLI compat pass to decide whether to keep or remove. STARTUP DRAIN ============= crates/gitlawb-node/src/main.rs calls durable_outbox::drain_pending_ref_transitions(state, 1000) ONCE before serving, after migrations and after the existing peer / quarantine prunes. Non-fatal: a transient drain failure logs and leaves the rows for the next startup. durable_outbox::drain_pending_ref_transitions reads every 'applied' row, calls derive_one (which re-derives the three artifacts using the persisted authentic pusher DID and signature header), then deletes the row. A second drain pass is a no-op for both the artifacts (idempotent inserts) and the row (gone after the first pass). NEW END-TO-END TESTS ==================== crates/gitlawb-node/src/durable_outbox.rs adds three end-to-end tests in drain_tests, complementing the eight DB-layer tests in db::pending_ref_transition_tests: - drain_re_derives_all_three_artifacts_for_an_applied_row: the reviewer's first proof. Inserts a row in 'applied' state (the crash window), drains, asserts exactly one push event row, exactly one cert row carrying the original pusher DID (not a placeholder), and exactly one anchor job row. Asserts the deterministic cert id matches. Asserts a second drain pass is a no-op. - cancelled_row_produces_no_artifacts: the reviewer's second proof for the cancelled state. A row in 'cancelled' (receive_pack returned Err) is invisible to the drain. - prepared_row_produces_no_artifacts: the reviewer's second proof for the prepared state. A row in 'prepared' (handler crashed between insert_prepared and the post-Ok branch) is invisible to the drain. Each test names the invariant it pins and the production line it covers. Reverting that line turns the named assertion red. Compiles clean, 1099 tests pass with 0 regressions, clippy clean under -D warnings, fmt clean. Cross-PR overlap (declared in the PR description): - #134 (anchors auth): composes. The /arweave/anchors route already requires auth; this PR does not change the route. - #285 (advisory-lock session affinity): composes. The durable intent is written inside the same handler that holds the lock from #285; no changes to the lock layer. - #306 (Content-Digest on signed requests): composes. PR 1 persists the Content-Digest header that #306 makes mandatory. - #314 (small-order Ed25519): independent. PR 1's tests use strong keys. - #324 (libp2p keypair persistence): independent. PR 1 does not touch p2p identity. - #325 (gossip ref-update auth): independent. PR 1's signed envelope is the HTTP-side equivalent, not the gossip-side. - #382 (replication withheld-subtree trees): independent. PR 1 does not touch replication or pin selection. * fix(node): address four reviewer findings on #26 split 1/4 - P1-A: add startup reconcile step that promotes `prepared` rows to `applied` when the on-disk ref matches the row's `new_sha`. The recovery drain (which only reads `applied` rows) can now pick up a ref that landed when the live handler's `mark_pending_ref_transitions_applied` call errored or was interrupted. Strict SHA equality is the load-bearing check — a `prepared` row whose target did NOT actually land stays `prepared`. - P1-B: route the live handler's cert issuance through `cert::issue_ref_certificate` (the upsert) instead of `issue_ref_certificate_idempotent` (DO NOTHING). A re-push to the same ref now updates the cert's `old_sha` / `new_sha` / `pusher_did` / `issued_at` / `signature` to the new transition while preserving the deterministic `cert_id`. The recovery drain keeps the idempotent variant; both paths collapse to one row. - P2-A: refactor the drain into a `drain_pending_ref_transitions_with` testable seam that does per-row log-and-continue, and add `drain_pending_ref_transitions_all` that loops `DRAIN_PER_PASS_LIMIT=1000` rows for `DRAIN_MAX_PASSES=10` passes. A failing row no longer stalls the batch; a backlog above 1000 rows is fully processed across passes. - P2-B: add a `first_ref_name` column to `pending_ref_transitions` via migration v28. The live handler hoists a `first_ref_name` local and persists it on every row of the same `request_id`. The drain's `derive_one` keys the push event id on `row.first_ref_name` instead of `row.ref_name`, so live and recovery produce the same id and `ON CONFLICT (id) DO NOTHING` collapses a multi-ref push to one push event row (and one trust- score bump). Cert and anchor ids stay per-ref / per-transition. * fix(node): address four reviewer findings on #26 split 1/4 (round 2) * fix(node): address reviewer round-3 findings on #26 split 1/4 - P1: Delete outbox rows after live durable effects complete so they don't replay on every restart - P1: Parse git report-status for per-ref ok/ng results; mark only proven rejections as cancelled, uncertain outcomes as recoverable - P1: Introduce 'uncertain' state for receive-pack errors where some refs may have landed; reconcile checks these against disk at startup - P2: Promote deletion transitions during reconcile (new_sha == ZERO_SHA with absent ref = successful deletion) - P2: Loop reconcile across multiple passes so backlogs beyond the first page are processed in the same startup Closes review round 3 findings from reviewer-1 and reviewer-2. * fix(node): fix CI failures from round-3 changes - Add COMMENT ON TABLE to v29 migration so migration_bodies_are_non_empty passes - Return error on non-zero receive-pack exit (preserving backward compat with tests that expect Err(AppError::Git(_))) while still parsing report-status for outbox row handling * fix(node): update inv22 gate tests for receive_pack_raw refactor - F3 gate: scan for smart_http::receive_pack_raw( instead of receive_pack( - U5 gate: scan for all_refs_ok instead of receive_result.is_ok() * fix(node): require reflog proof before the reconcile promotes a row (#26 split 1/4) Closes the second half of the reviewer's round-3 [P1]: the startup reconcile still promoted on `disk_refs.get(ref) == row.new_sha`, which also matches a coincidental current tip (old=B, new=A while the ref is already A). That is not an exotic case — it is the ordinary shape of a REJECTED push, since git refuses an update whose expected old value does not match the ref. Both the SHA check and the MAX_RECONCILE_AGE window pass, the push never happened, and the drain then writes a push event, a signed certificate and an anchor for it. The live path proves landings from git's report-status body; that body is gone by the time the reconcile runs, so the proof is re-derived from the repository. `git::store::ref_reflog_entries` reads `logs/<ref>` and a row is promoted only when an entry carries its exact `old -> new` pair, stamped at or after the row was written (REFLOG_CLOCK_SKEW absorbs git's whole-second truncation against the row's sub-second `created_at`). The timestamp floor is what separates a landing from a later push that re-introduced the same pair: proof must postdate the intent it proves. Deletions stay exempt from the reflog half and keep the absence-plus-age rule already in place — git removes a ref's reflog along with the ref, so no other evidence can exist for one. `init_bare` now sets `core.logAllRefUpdates`, which bare repos default off; a repo with no reflog yields no proof and its rows stay put for human-attended recovery rather than becoming accounting the node cannot substantiate. Tests: reconcile_refuses_a_coincidental_tip_with_no_reflog_proof, reconcile_promotes_a_row_the_reflog_proves, reconcile_leaves_a_row_prepared_when_the_repo_keeps_no_reflog, reconcile_refuses_a_reflog_entry_older_than_the_row, reconcile_still_promotes_a_landed_deletion_which_can_have_no_reflog, init_bare_keeps_reflogs_so_a_landing_can_be_proven, ref_reflog_entries_is_none_when_the_repo_kept_no_log. No migrations added or renumbered. * fix(node): walk the reconcile backlog on a keyset cursor (#26 split 1/4) The multi-pass reconcile looped but never advanced. Each pass re-issued the same cursor-less `LIMIT n` query, and unlike the drain — which deletes every row it finishes, so its next page is always new work — the reconcile leaves every row it cannot promote exactly where it was. One unprovable row at the head of the ordering therefore pinned page one on every pass, and the backlog behind it was never examined at all, which is the condition the multi-pass loop was added to remove. Rows keep ageing toward MAX_RECONCILE_AGE while they wait, so "picked up next restart" can mean "never recovered". Pages now resume strictly after the `(created_at, id)` of the last row EXAMINED, via `list_pending_ref_transitions_prepared_or_uncertain_after`. A keyset cursor rather than an OFFSET because rows leave the set underneath the walk when they are promoted: an absent row simply does not appear on a later page, and it can never shift an unvisited row into a page that was already read. Pass budget, per-pass limit, short-page exit and the residual-backlog warning are unchanged. This matters more with the reflog gate in place: a landing in a repo that keeps no reflog is PERMANENTLY unprovable, so before this it would jam page one on every startup rather than once. Tests: reconcile_all_walks_the_backlog_past_the_first_page, reconcile_all_advances_past_an_unprovable_row (whose blocker is a no-reflog row, the class the gate introduces). * fix(node): bound the reflog read to a recent tail (#26 split 1/4) `ref_reflog_entries` slurped the whole reflog. A reflog grows one line (~150 bytes) per ref update and how often a ref is updated is PUSHER-controlled, so that was an attacker-sized allocation, taken once per stranded row, at startup — the moment the node can least absorb it. Every other attacker-influenced read in this crate is bounded (read_body_capped, MAX_SIGNATURE_ENTRIES, the pin/walk budgets); this one should be too. The bound costs nothing in proving power, which is why it is a ceiling rather than a tradeoff: the landing gate only ever accepts an entry stamped at or after `created_at - REFLOG_CLOCK_SKEW`, and reflogs are append-ordered oldest-first, so every entry that could possibly qualify is at the tail. The read now seeks to the last REFLOG_TAIL_BYTES (256KB, ~2000 recent updates to one ref) and `take`s that many bytes, so the allocation stays bounded even if the file grows between the stat and the read. A window into the middle of a file starts mid-record, so the bytes before its first newline are discarded: without that, a boundary landing inside a record's old SHA leaves a suffix that still tokenizes and yields a truncated SHA as a bogus `old -> new` pair — and landing proof is an exact pair match, so a bogus pair is a fabricated proof. An entry older than the window reads as absent, which is the safe direction: the caller treats "no matching entry" as unproven and leaves the row where it is. `Ok(None)` for a missing file, and the absence-of-evidence contract, are unchanged. Tests: ref_reflog_entries_reads_whole_records_from_the_recent_tail (lays the file out so the boundary lands 20 bytes into a record's old SHA — the alignment that would otherwise parse), an_entry_older_than_the_reflog_tail_reads_as_unproven. * fix(node): read the report through both side-band frames The report-status is framed twice whenever the client negotiates side-band-64k, which `git push` over smart HTTP does: the outer side-band envelope carries a stream that is itself pkt-line encoded. One strip pass left the first line as `000eunpack ok`, so the unpack check failed and parse_report_status returned None — read by the caller as "no report", which keeps every declared ref. The per-ref landing gate was therefore inert for exactly the pushes it exists to filter. The regression test is a byte-for-byte capture from git 2.50.1 rejecting one ref of a two-ref push, because a hand-written single-framed fixture parses under the old code and hides the case. * fix(node): address round-4 reviewer findings on #26 split 1/4 * fix(node): rustfmt round-4 reviewer fixes * fix(node): remove empty line after doc comment * fix(node): drop redundant reflog gate; implicit-ok on exit-zero no-report * fix(node): address round-5 reviewer findings on #26 split 1/4 * fix(db): v30 migration — add receive_pack_requests table and ordinal column The reviewer's round-5 finding: the push event identity was encoded into a mutable per-ref column (first_ref_name) whose correct value is knowable only after git, and the correction was not committed atomically with the per-ref outcomes. A crash between git updating a later ref and the rewrite left the durable rows naming the rejected ref, and derive_one's row.ref_name == row.first_ref_name guard then meant no push event ever landed for the accepted child. Per the state-transition model at .gravirei/plans/state-model-durable-post-receive.md, this migration introduces the request-level record that owns the push event and the trust score, and extends the per-ref child with an ordinal column the drain and the effect executor walk together. first_ref_name is dropped; the push event id is keyed on (request_id, accepted_ordinal) and lives on the request row. The next commit (handler rewrite) wires the live path and the recovery drain against the new tables. BREAKING CHANGE: pending_ref_transitions.first_ref_name is dropped. Callers that read or write that column must move to receive_pack_requests.accepted_ordinal. * fix(node): rewrite receive-pack handler against the request-level model (#26 split 1/4 step 2) The v30 migration added receive_pack_requests and dropped first_ref_name, but no Rust code used either. The handler still ran a four-branch per-ref state flip plus a first_ref_name rewrite after git returned, which is the mixed-outcome bug a push with the first ref rejected exposes: the live path and the drain computed different push-event ids. - Insert a receive_pack_requests row in state `received` BEFORE git runs (insert_receive_pack_request), carrying the request bytes and the SHA-256 of the body. Crash between intent and git return is now recoverable via the reconcile step. - Drop the first_ref_name rewrite at api/repos.rs:2454. The push event identity is now (request_id, accepted_ordinal); accepted_ordinal is computed once at the per-ref state flip. - Re-key push_event_id_for and ref_cert_id_for on (request_id, ordinal). Anchor job id stays per-transition. - Transition the request row to outcomes_committed (with parsed_report and accepted_ordinal) or rejected_at_git (no report + non-zero exit) in the same handler tail as the per-ref state flip. The drain (step 3) picks up outcomes_committed rows; today the live path also runs the per-ref effects inline. - Stub the step-3 surface (mark_request_effects_pending, complete, list_receive_pack_requests_due, update_request_attempt) with #[allow(dead_code)] + contract-pin comments so the rewrite is bisectable: reverting this commit restores the pre-rewrite live path. The U5 gate (replication tail spawn inside push_succeeded, before guard.release) is preserved; inv22_replication_tail_spawns_at_the_ durability_boundary stays green. * fix(node): use raw bytes for request_bytes_hash (#26 split 1/4 step 2) The v30 migration defined request_bytes_hash as BYTEA but the handler was binding a hex-encoded String. Postgres rejected the insert with "column request_bytes_hash is of type bytea but expression is of type text" and the handler shed the push with a 503. Switch the handler to bind the raw 32-byte digest and update the ReceivePackRequest struct field type to Vec<u8> to match. The drain-side test fixture already used Vec<u8> and needs no change. Pinned by the bin test suite going from 24 failures (all the receive-pack-cap tests that send a 4-byte body) to 0 regressions on the new model. * fix(node): factor out apply_request_effects; drain walks receive_pack_requests (#26 split 1/4 step 3) The step-2 commit moved the request row to be the unit of work but left the per-ref effects fan-out inline in the handler and the drain walking pending_ref_transitions per-ref. Step 3 factors the effects fan-out into apply_request_effects(state, request_id) and rewrites the drain to walk receive_pack_requests. - apply_request_effects lives in durable_outbox.rs; the live handler in api/repos.rs and the drain both call it. Idempotent on (request_id, accepted_ordinal). Returns EffectsOutcome::{Done, Nothing, Retry}. - Drain switches from list_pending_ref_transitions_applied + derive_one to list_receive_pack_requests_due + apply_request_effects. derive_one and the per-ref drain entry points are deleted. - The step-3 stubs in db/mod.rs (mark_request_effects_pending, mark_request_complete, list_receive_pack_requests_due) lose their #[allow(dead_code)] annotations; every stub has a caller in this PR. - Reconcile (reconcile_prepared_from_disk_all) is unchanged. The crash window between intent durable and outcomes commit still routes through per-ref reflog proof. - Drain tests rewritten to stage receive_pack_requests rows; the per-ref fixtures (make_row, lookup_accepted_ordinal) are gone. - New inv26_step3_live_and_drain_share_apply_request_effects assertion pins the live/drain sharing and the per-request drain walk. * fix(node): bounded retirement purge for terminal request rows (#26 split 1/4 step 4) The v30 partial index idx_receive_pack_requests_completed_at exists but no code reads it. Step 4 wires a periodic purge task that deletes terminal `complete` and `rejected_at_git` rows older than the retention window, along with their per-ref children. `quarantined` (a step-5 state) is never purged by the timer. - `purge_completed_receive_pack_requests(older_than, limit)` — deletes parent requests using the v30 partial index. - `purge_completed_pending_ref_transitions(older_than, limit)` — deletes children of purged parents, gated on the child's own applied_at / cancelled_at. - `purge_request_queue(db, retention_days, limit)` — orchestrator in durable_outbox.rs that calls both and returns the totals. The deletion order (parents first, then children) is the contract; a crash mid-purge leaves orphaned children that the next pass will pick up. - `spawn_queue_lifecycle_sweep` in main.rs — periodic task on the same detached pattern as `spawn_legacy_cid_sweep`, 24-hour interval, shutdown-aware. The interval matches the spec's "one per cluster per day" target. - Config knobs `queue_retention_days` (default 7, range 1..=365) and `queue_purge_batch` (default 1000, matches DRAIN_PER_PASS_LIMIT). - 3 new `durable_outbox::drain_tests::purge_*` tests pin the contract: only old terminal rows are deleted, the second pass is a no-op, and a row inside the retention window survives. - New `inv26_step4_queue_lifecycle_purge_is_wired` gate asserts the wiring (main.rs calls the purge, the DB helpers exist, the Config knobs exist). * fix(node): reference-transaction marker + quarantined state + failure matrix (#26 split 1/4 step 5) Steps 2-4 gave the request row the unit of work, the shared executor, and the bounded retirement policy. Step 5 closes the evidence gap: the reconcile now requires a per-request marker ref (refs/gitlawb/requests/<id>) whose value matches request_bytes_hash. A missing or mismatched marker quarantines the request; an operator reclassifies it. - v31 migration: adds `quarantined` to the state vocabulary and a partial index for operator queries. - Handler writes the marker ref before git-receive-pack; the marker is causally bound by being in the same async task as the receive-pack call. The marker's value is content-addressed (git hash-object of the request bytes), so the gate compares consistent SHAs on both sides. - git::store::read_ref reads a single ref's value, returning Ok(None) for absent refs. Used by the marker gate. - git::store::marker_value_for computes the content-addressed marker value; both the live handler and the reconcile use it so the write and the read agree. - Reconcile gains a marker gate between the age check and the reflog proof. Mismatch or absent ⇒ mark_request_quarantined + mark_children_rejected_for_quarantined_parent. - effects_max_attempts bound (config knob, default 8) flips retry-stuck requests to `quarantined` after N attempts, closing the infinite-retry DoS window. - New failure_matrix_tests submodule covers the spec's outcome × ref-kind × exit-point × recovery-scenario matrix (6 cells). - New inv26_step5_marker_quarantine_and_bound_are_wired gate asserts the marker gate, the bound check, the handler's pre-receive-pack ordering, and every load-bearing helper. - Existing 7 reconcile tests updated to stage a marker ref via the new `stage_marker` test helper. * fix(node): converge durable post-receive to singular request-level lifecycle Unify producer, evidence, aggregate outcome, executor, retry, and retirement on the request row: atomic intent and outcome commits, synthetic normalized reports for implicit-ok/reconciled paths, reconcile parent promotion with fail-closed deletions and competing-claimant guard, retry progression from both executable states with backoff/quarantine, terminal-parent-gated purge with marker cleanup and hidden refs, minimal digest-only intent plus request-level RFC9421 proof (v32). * fix(node): harden request occurrence lifecycle, proof, retry, and retirement Reject incomplete report framing and omitted refs as indeterminate, preserve uncertain children on completion, terminalize all-rejected, bind reconcile with landing history, durable proof with ack gate, occurrence-keyed anchors, background due loop, deletion quarantine with operator resolve, children-before-parent purge with marker tombstones, recovery prereq upgrade check, and bounded marker runner. * fix(node): keep recovery prereq upgrade best-effort on push path Refusing pushes when git config upgrade fails broke fake-git and non-repo disk-path tests with 503. Reconcile already fails closed on missing reflogs, so downgrade to warn-and-proceed. * fix(node): close reviewer round on outcome authority and lifecycle ownership Gate effects on unpack_ok with cleared ordinal and terminal no-effects, intersect executor with applied children, treat absent report-status as indeterminate regardless of exit, require landing history before child delete, claim due requests with recoverable leases, dedupe webhooks by occurrence ledger, ack-gate proof retention with terminal disposition, fair marker tombstones with dead-letter, kill_on_drop marker bounds, and loud migration version collision guard. * fix(node): load-bearing coverage for intent, guards, and delivery ledger Handler intent test plus inv22 gate so disabling the durable insert turns red; unpack-false, mixed ok/ng, divergent cancelled, partial sibling, and proof-ack tests pin each executor guard; webhook deliveries record sent_at with stale-pending reclaim; stale parsed_report comment corrected. * fix(node): centralize live retry, bound outcome commit, close retention orphans Route live Retry/Err through schedule_request_retry_or_quarantine so attempt progression matches the drain; retry post-git outcome commits 3x before restart repair; block terminal purge while non-terminal children exist; add v27 upgrade-path test. * fix(node): document attended-restart commit policy and pin executor retry arms Track outcome-commit success, skip metrics/touch on failure, correct the lease-expiry comment to startup reconcile; add restart-repair test for stuck received parents plus RED tests for landing-history and proof-ack failure paths. * fix(node): correct marker prereq comment to warn-and-proceed * fix(node): single partial-completion model with disposition gate Split apply_request_effects into phase-1 bundle (push, cert, anchor, history, webhook, proof ack before child deletion) and phase-2 gate that retries while non-cancelled siblings remain; Nothing only when nothing owed. Gate tail, Tigris, and effects on PostGitDisposition with documented replication carve-out; add partial-sibling webhook and disposition tests; fix stale comments. * fix(node): close review round on outbox ordering, marker visibility, and repair reachability Address the P1/P2 review findings: await the webhook delivery claim before accepted-child deletion; exempt the marker namespace from the fail-closed ref peeling guard so visibility walks tolerate blob-pointing request markers; keep request marker deletion bounded in the purge path; promote cross-page siblings via idempotent report superset; run stuck-parent repair when the prepared queue is empty; bound the durable intent write while admission permits are held; remove dead proof verification scaffolding. * fix(node): close review round on pre-git refusal lifecycle, namespace gate, and promotion merge * fix(node): close review round on namespace gate, intent lifecycle, and promotion atomicity Namespace gate hardening: - Refuse non-UTF-8 ref-update lines (git accepts bytes ≥0x80; gate never sees them) - Catch bare 'refs/gitlawb' name (starts_with missed it) - Add deny test for both holes plus refusal wiring verification Intent lifecycle guards: - Add IntentDropGuard: terminalize stranded intents on dropped futures - Bound refuse_receive_pack_before_git to 10s (no statement_timeout on pool) - Guard mirror fetch: hideRefs blocks non-exempt refs/gitlawb/* from +refs/*:refs/* Promotion merge atomicity: - Take row under SELECT FOR UPDATE so merge is atomic with state read - Prevents racing due worker from stranding newly applied children outside report * fix(test): correct inv22 namespace gate assertions to match actual implementation The test was looking for the wrong patterns: - UTF-8 check: error message string, not pre-loop condition - Namespace check: `*r == "refs/gitlawb"`, not `ref_name ==` - Visibility exemption: `&&` logic (both false to exclude), not `||` * chore: cargo fmt * fix(node): phase-aware intent guard, mirror import prune, terminal-state merge * fix(node): guard cleanup retry with fates, fail-closed mirror prune, refusal coverage * fix(node): bound guard attempts, sibling-wait outcome, fail-closed prune enumeration * fix(node): sibling-wait outcome, no-report path, bounded prune and guard retry * fix(node): no-report durability, cert landing order, bounded reconcile git, flush checks * fix(node): gate trust-score bump on a new push event row * chore: update rustls 0.23.37 → 0.23.45 to resolve RUSTSEC-2026-0285 * fix(node): surface git's report-status when a stdin EPIPE follows child exit * fix(node): cap receive-pack ref-update count before per-ref work and intent * fix(node): cap ref-update parsing in-loop and dedupe identical commands * docs(node): correct round-4 comment claims on client outcome and gate cost * fix(node): single replication tail, proven marker taxonomy, claimed webhook delivery (#558) - One post_receive_replication_tail per push: the carve-out spawn and the durability-bound spawn are mutually exclusive positions on the same proven-landed ref set; inv22 now fails on any unfiltered or ungated spawn. - Marker reconcile: bounded-read errors (git Err/deadline) stay prepared and retry instead of quarantining as 'missing marker ref'; only a successfully read marker proves missing or mismatch. Err-injection test pins no-quarantine on unproven reads. - Claimed webhook delivery: the synchronous claim phase returns the claimed Webhook rows so the send task never re-lists; a list failure after claim can no longer drop claimed deliveries. Source gate pins it. * docs(node): name claimed-hooks variant in fire_event_occurrence doc * fix(node): order pending_ref_transitions create before outbox alters, fill v29 (#558) * fix(node): proven-missing marker taxonomy, v38 upgrade test, report-ok tail fixtures (#558) * fix(node): read marker refs via rev-parse so present refs return their oid (#558) --------- Co-authored-by: Gravirei <[email protected]> Co-authored-by: Kevin Codex <[email protected]> |
||
|
beardthelionandcarlitos973
|
3c1ee27b60 |
fix(node): make creation-route rate limit configurable; fix(gl): surface real errors on pr create failure (#560)
- gitlawb-node: rate limiter was hardcoded to 10 req/hour per DID across all creation routes (repo/issue/PR create, register, fork). Adds GITLAWB_RATE_LIMIT_MAX and GITLAWB_RATE_LIMIT_WINDOW_SECS config fields (clap env-var pattern matching the rest of Config), defaults unchanged. Lets node operators tune this for real agent/bot usage without recompiling. - gl pr create: previously called resp.json() before checking response status, so any non-2xx response with an empty or non-JSON body crashed with a generic "invalid JSON" error instead of surfacing the real cause. Now reads the raw response body first and includes it in the error message on failure. Co-authored-by: carlitos973 <[email protected]> |
||
|
beardthelionandKRATOS
|
209618a2fe |
feat(node): add scheduled inbound mirror worker (#555)
* feat(node): persist upstream mirror transition state Origin-Session: local-d6a143 | Codex | 12 prompts * fix(node): reject null mirror transition state Origin-Session: local-d6a143 | Codex | 14 prompts * fix(node): address mirror state review findings Origin-Session: local-d6a143 | Codex | 16 prompts * fix(node): close mirror review blockers Origin-Session: local-d6a143 | Codex | 20 prompts * fix(node): align peer URL validation message Origin-Session: local-d6a143 | Codex | 20 prompts * fix(node): close remaining mirror review findings Origin-Session: local-d6a143 | Codex | 21 prompts * feat(node): add scheduled inbound mirror worker Origin-Session: local-d6a143 | Codex | 18 prompts * fix(node): harden scheduled mirror fetches Origin-Session: local-d6a143 | Codex | 24 prompts * fix(node): close inbound mirror review blockers Origin-Session: local-d6a143 | Codex | 24 prompts * fix(node): reject special-use upstream literals at configure time (#555) validate_mirror_upstream_url used only the peer public-host classifier, so an IP literal the fetch worker's stricter permit denies (192.0.2/24, 198.18/15, 2001:db8::/32, ...) could be stored and then hard-fail on every scheduled scan. The IANA special-use permit moves to peers:: mirror_upstream_ip_permitted, shared by configure validation and the resolve-path check; the negative test set now covers the denied literals. * test(node): scrub inherited git env in mirror fixture spawns (#555) The repository_local_* fixture tests ran plain Command::new("git") and inherited any GIT_DIR/GIT_CONFIG_* state from an outer checkout, so a fixture spawn could act on the wrong repository. A shared fixture_git helper now sets the cwd and removes git environment vars; all five spawn sites use it. * fix(node): bound mirror preflight probes separately, align operator docs (#555) * chore(node): retrigger checks after code-scanning alert triage (#555) * chore(node): retrigger checks after code-scanning alert triage (#555) --------- Co-authored-by: KRATOS <[email protected]> |
||
|
|
4cd0a6a306 |
fix(node)!: gate agent-task reads behind visibility rules (#553)
* fix(node): gate agent-task reads behind the same visibility rules as repo data list_tasks and get_task had no authorization at all: any anonymous caller could enumerate every task on the node, including another party's repo-less task, its ucan_token, and its payload (#268). Add task_visible, mirroring the repo read-visibility gate already used by the ref-updates feed: the delegator and assignee can always read their own task, a repo-scoped task follows that repo's normal visibility rules, and a task naming no repo (or a repo this node doesn't host) is visible only to its delegator/assignee. Both REST and GraphQL now route through the same collect_visible_tasks/get_visible_task collectors so the two surfaces cannot drift, and neither read path echoes ucan_token back, since the holder already received it via the create/claim response. Fixes #268 * fix(node): query the task limit-clamp test as the delegator tasks_limit_ceiling_clamped_to_200 seeded 201 repo-less tasks and read them back anonymously, expecting all 200. That read is exactly the enumeration #268 closes, so the new visibility gate correctly returns none of them and the test went red. The clamp ceiling is what this test pins, not the gate, so query as the tasks' delegator, who can legitimately see all 201 rows. Refs #268 * fix(node): scope task visibility lookups to the page's repos collect_visible_tasks loaded every repo on the node and every visibility rule in order to gate at most 200 tasks, so an anonymous request paid for the whole node's repo and rule set. Narrow both lookups to the repo ids the fetched page actually names, and skip them when no task names a repo. The deduped repo snapshot stays the source of truth for resolving a repo_id: it collapses mirror and canonical pairs and omits quarantined repos, and an id missing from it has to keep failing closed. Resolving ids straight from the repos table would surface exactly those withheld rows. Add GraphQL denial tests as well. Nothing pinned that the task resolvers delegate to the shared collectors, so a resolver that queried the database directly would not have gone red. Refs #268 * Preserve authorized task reads across shipped clients. Refs #268 * Bound denied task history scans. Refs #268 * Signal incomplete task scans with recoverable cursors and map read errors to AppError. Refs #268 * Stop disclosing denied task rows in the scan-wall cursor and add GraphQL pagination state. Refs #268 * Normalize task cursors and gate mutation endpoints behind visibility. Canonicalize RFC 3339 timestamps in parse_after_cursor to handle URL-decoded spaces, reject mixed cursor alias families, gate complete_task and fail_task behind get_visible_task so unreadable tasks 404 instead of leaking existence with 403, and only flag incomplete when hitting candidate ceilings on full SQL batches. Refs #268 * Update complete_task test in test_support to exercise both 404 on unreadable and 403 on non-assignee tasks. Refs #268 * Bind task to repo id in complete_task test in test_support. Refs #268 * Preserve timestamp fractional width and anchor continuation tests on legitimate cursors. Refs #268 * Match on Value::Object to extract task ID in GraphQL continuation test. Refs #268 * Remove unused mut binding on visible_newer in GraphQL query test. Refs #268 * Hide unauthorized claim and keep assigned tasks from being stolen Gate REST and GraphQL claim behind the same visibility check as complete and fail, refuse claim when another assignee already holds the task, and only broadcast publicly visible task events. Treat a full list page as incomplete when more candidates remain. Surface HTTP errors from CLI and MCP claim and complete helpers. Refs #327 Co-Authored-By: cairn-code <[email protected]> * Keep task-list incomplete for the scan ceiling only. A full visible page was flagged incomplete whenever the SQL batch was full, so the first page of any list with more than 200 candidates looked stalled. Align the GraphQL claim test with the visibility gate's not-found message. Refs #268 * fix(node): pin claim guards and map mutation errors through AppError Review required tests that go red if the pre-assigned claim predicate or the anonymous announce gate is deleted, and incomplete must not stay true when the candidate stream is exhausted at the scan ceiling. Route claim, complete, and fail through AppError so closed-pool outages stay 503 and 404s match the read envelope. Refs #268 * fix(node): match bare and did:key assignee forms in claim and list create_task stores the supplied assignee unchanged, so a raw SQL equality check drops a designated assignee who presents the other did:key form. Compare the normalized key so claim and filtered list agree with did_matches. Refs #268 * fix(node): check task creation status and add assignee expression index - Add error_for_status() to cmd_create and task_create MCP tool - Update test_create_task_server_error to assert failure on 500 - Add migration v18 creating expression index idx_agent_tasks_assignee_key matching ASSIGNEE_DID_CASE_SQL - Add did:web:z6Mkfoo single-residual shape to parity boundary matrix Refs #327 # Conflicts: # crates/gitlawb-node/src/db/mod.rs * fix(node)!: Replace task cursors with opaque continuation tokens The task read path treated visibility, pagination, and error vocabulary as separate edits, so each one broke where they met. Rework them as one contract. A raw (created_at, id) cursor forced a choice between two broken options: it could name the last visible row, and then a denied window longer than the 1,000-candidate scan budget was unpageable forever; or it could name the last examined row, and then a denied read leaked the id and timestamp of a task GET /tasks/{id} otherwise 404s. Continuation tokens remove the choice. They carry the last examined candidate, so paging always advances a full scan budget per request, and they are encrypted and authenticated under a node-derived key, so the caller learns nothing from one and cannot forge one naming a row of their choosing. Encryption is a synthetic-IV construction over the hmac/sha2 pair already used for webhook signatures, so it adds no dependency and needs no randomness source. Making the token the only accepted cursor also gives the ordering key one domain. agent_tasks.created_at is TEXT and compared as TEXT, so a caller-typed '...Z' and '...+00:00' denote one instant but sort differently, and a client could silently skip or repeat same-time rows. The token carries the stored string verbatim, so the value compared is always one the server wrote. The raw after_*/cursor_* pairs are removed rather than kept alongside it, since a second domain is the bug. Separate the two facts the old single incomplete flag conflated: has_more says candidates remain, incomplete says this page is short only because the authorization scan hit its ceiling. Both REST and GraphQL now return has_more, incomplete, and next_cursor from the shared collector, and REST echoes the limit it actually applied so a clamped request is visible as clamped. Have gl task list and MCP task_list follow next_cursor instead of issuing one request: --limit 500 returned a successful but silently truncated 200 rows. Following is bounded by a page cap and a no-progress guard, and a run stopped by either reports an explicit incomplete result with a resume cursor. Route claimTask, completeTask, and failTask through the same task_write_conflict classifier the REST handlers use, via curated helpers in the graphql module so the map_err source guard still holds. A claim race or stale finish reached GraphQL clients as a generic database error while REST clients got an actionable conflict; genuine sqlx faults stay opaque on both. Refs #327 * fix(node): End the candidate stream only after the batch is examined A short SQL batch means no rows exist past it, not that every row in it was examined. When the page filled mid-batch the collector treated the two as the same, marked the stream ended, and suppressed the continuation, so every row after the one that filled the page was unreachable. The equal-timestamp paging tests caught it: three rows with a limit of one returned only the first. Track how much of each batch was consumed and end the stream only when the whole of a short batch has been examined. Otherwise leave `has_more` to the probe row, which resumes from the last examined candidate. Refs #327 * fix(gl): Reject a non-positive task-list limit and fault the write under test A `--limit 0` reached the node, which clamped it to zero and answered with an empty page marked complete, so an invalid request read as proof that no tasks exist. Reject a non-positive limit in `fetch_tasks()`, the helper the CLI and MCP share, so the guard cannot drift between the two surfaces. `task_write_sql_faults_stay_opaque` did not exercise what it named. Dropping `updated_at` also broke the SELECT in `get_task()`, so the fault surfaced from the `get_visible_task()` pre-check through `graphql_app_err` and never reached `graphql_claim_conflict`. A `BEFORE UPDATE` trigger keeps every read valid and faults only inside `Db::claim_task`, and the test now also asserts that a write-time fault is not reclassified as a claim race. Refs #268 * fix(node): Bind task cursors to the caller and brake the task read routes A continuation token names the last candidate a scan examined, not the last row it returned, so it encodes how far that scan got under one caller's visibility. The MAC bound the page filter but not the presenting identity, so resuming a token as a different caller started the scan past rows that caller was entitled to read and dropped them from the answer with nothing to signal the loss. Bind the caller's normalized DID into the MAC, with anonymous flagged absent rather than encoded as empty. Normalization goes through normalize_owner_key so the two spellings of one did:key identity bind identically, matching did_matches on the read path: a caller who presents the other form of their own DID keeps their own page. A mismatched token renders the existing single rejection message, so this adds no oracle. GET /api/v1/tasks and GET /api/v1/tasks/{id} are anonymously reachable, and the visibility gate costs a task lookup plus deduped-repo and visibility-rule queries before it can return the opaque 404. An unauthenticated prober therefore pays nothing while the node pays per request, whether or not the id exists. Attach the per-IP limiter already used on /ipfs/{cid}, configurable through GITLAWB_TASK_READ_RATE_LIMIT and swept by the periodic task like every other per-key limiter. Refs #268 * fix(node): Brake anonymous GraphQL task reads on the same per-IP bucket The per-IP brake added for the task read routes covered only /api/v1/tasks*, so an anonymous caller reached the same collect_visible_tasks and get_visible_task gate over /graphql with no bucket at all. The fence had an open lane beside it. Carry the brake as GraphQL request data and debit it in the tasks and task resolvers rather than layering rate_limit_by_ip onto the GraphQL router: /graphql is one endpoint for every operation, so a router layer would charge unrelated queries and every mutation against the task-read bucket. Debiting per resolved field also prices an aliased query honestly, since ten aliased tasks fields run the gate ten times. Extract RATE_LIMIT_MESSAGE so the GraphQL surface, which cannot return a 429 status inside a 200 envelope, refuses with the same text the REST routes use. /graphql/ws serves the query root as well and stays unbraked; closing it needs a WebSocketUpgrade handler and is left for a follow-up. Refs #268 Refs #327 * fix(gl,node): parse typed task pages, track cursor cycles, and normalize assignee filter MAC * fix(node): Cap aliased GraphQL task reads per request and derive has_more from visible rows - Cap aliased GraphQL task read fields per request using an atomic counter on TaskReadBrake (MAX_GRAPHQL_TASK_READS_PER_REQUEST = 5). - Derive has_more in collect_visible_tasks by scanning for bounded_limit + 1 visible rows, eliminating the un-gated keyset probe that could leak the presence of trailing denied tasks. - Add regression tests covering aliased GraphQL capping and trailing denied task has_more privacy. Refs #327 * fix(node): Check database continuation when scan budget ends on exact batch boundary When candidate scanning reaches MAX_TASK_SCAN_CANDIDATES without finding a target_visible row and the final batch was full, probe the database for rows beyond the scan position so an exhausted candidate stream is not erroneously marked incomplete. Refs #327 * fix(node): Document and pin the bound on the task scan-ceiling probe The scan-ceiling branch of collect_visible_tasks settles has_more with an un-gated LIMIT 1 probe, so a caller can learn whether any row - readable or not - trails the position the scan stopped at. Withholding the probe does not remove that bit: enumeration past a denied window longer than one scan budget requires handing back a continuation, and following that continuation returns the same terminal page one round trip later. State what the probe discloses (one bit, only at server-chosen positions a full scan budget apart, reachable only through a MAC'd cursor, never a denied row's id, payload or ucan_token) and pin it end to end. Also correct the comment above the branch, which claimed has_more never comes from an un-gated probe while the code below it did exactly that. Refs #327 * Index task keyset pages so the scan ceiling bounds database work. Optional IS NULL predicates kept the planner from using a created_at/id order, so every list_tasks_keyset batch could sort a growing match set before LIMIT. Dedicated per-domain SQL plus v28 indexes make the candidate ceiling a database bound. Refs #327 * Reject oversized task pages so list never returns more than the requested limit. fetch_tasks asked for the remaining total, then appended every row on a valid-shaped page. A remote that sent more tasks than want could make gl and MCP expose more than --limit. Treat that page as protocol-invalid before any extra row is kept. Refs #327 * Enforce task page limits on legacy responses and drop redundant single-column indexes. Refs #327 * Address review feedback on task-read authorization gating and client safety. - Wire TaskReadBrake into /graphql/ws subscriptions and verify per-request field caps and per-IP rate limiting over WebSocket connections. - Define open-claim eligibility separately from read visibility so unassigned tasks on readable/unscoped domains can be claimed without making task bodies enumerable. - Propagate identity errors on explicit key directories in CLI and MCP instead of silently falling back to anonymous mode. - Enforce response byte limits before deserializing task pages, validate row schema and page-local uniqueness before row commit, and sanitize continuation cursors in terminal diagnostics. - Add GraphQL denial assertions for repo-less tasks and token isolation. Refs #268 Refs #327 * Address review feedback on task-read authorization gating, WebSocket budgets, and test framing. Refs #395 * Address review feedback on quarantined task claims, WebSocket auth, and MCP limit parsing. Refs #395 * Reject forged signatures during WebSocket authentication upgrade. Refs #395 * Accumulate forged WebSocket handshake response until headers terminate. Refs #395 * Withhold quarantined-repo tasks on all read surfaces before party checks. Refs #395 * docs: Align task-read documentation with enforced visibility and rate limits. Refs #395 * docs: Distinguish task list and get behavior in rate-limit guidance. Refs #395 * fix(node): Batch task quarantine checks to bound read costs. Refs #395 * docs(node): clarify the shared task-read budget Document that GraphQL task queries over HTTP and WebSocket debit the same per-IP hourly budget as REST task reads. Refs #395 * fix(node,gl): bound scoped repo lookup and validate MCP cursor Bound repository access in list_repos_deduped_by_ids to requested groups via idx_repos_owner_key_name, leaving dedup_cte unscoped. Strictly validate MCP task_list cursor argument before network dispatch. Refs #396 * test(db): verify scoped lookup work in prepared plans Measure discarded rows and verify actual custom and generic prepared-plan counters, including the previous full-index-scan regression. Fixup for |
||
|
|
a2262d92b2 |
fix(node): implement reconciliation sweep as durability backstop (#218) (#551)
* feat(node): implement reconciliation sweep as durability backstop (#218) Reconciliation sweep: periodic background worker that re-derives pin/seal sets and fills gaps so a dropped replication job never means data loss. - visibility_pack.rs: all_object_paths (two-phase ls-tree + cat-file --batch-all-objects), allowed_blob_tree_sets_bounded, ObjectPath/BlobTreeSets type aliases - db/mod.rs: atomic policy transactions, get/set_node_state, has_ipfs_cid, filter_ipfs_pinned_oids, filter_pinata_pinned_oids, repo_policy_epoch, migration v27 (node_state table + policy_epoch column) - ipfs_pin.rs: PolicyFence struct, dispatch fence re-check before POST - pinata.rs: dispatch fence re-check before POST - encrypted_pin.rs: dispatch fence re-check - reconciliation.rs: full sweep worker with keyset cursor, PolicyFence - api/ipfs.rs: effective_cid fallback in list_pins - main.rs: spawn reconciliation sweep worker - .cargo/audit.toml: RUSTSEC-2026-0258 h2 ignore (pending #368) * fix: add missing parameters to pinning functions and adjust timeout settings * fix(tests): update CID assertions to handle Option type and adjust parameters in pinning functions * fix(node): close review findings on reconciliation sweep (#218) Reviewer-1 P1 / Reviewer-2 P1: v27/v28/v29 were referenced by tests but never added to the MIGRATIONS array, so every existing deployed node would fail to boot (node_state, repos.policy_epoch, pinned_cids.cid NOT NULL → NULL all unreadable). Forward-only migrations: v27 pinata_only_clear_legacy_equal_cid: ALTER cid DROP NOT NULL, UPDATE cid = NULL WHERE cid = pinata_cid v28 node_state_key_value: new key/value table for the sweep cursor v29 repos_policy_epoch: BIGINT NOT NULL DEFAULT 0 Reviewer-2 P1: visibility_pack.rs:542 phase 1 used ls-tree -rz, which under -r recurses into blobs and never emits tree entries. Trees then arrived only from the phase-2 catch-all with empty path and the fail-closed filter denied every one — the sweep could repair a non-flat repo's git graph. Switched phase 1 to ls-tree -r -t -z and add a per-commit rev-parse <commit>^{tree} that registers the root tree at "/". 48 visibility_pack unit tests pass. Reviewer-1 P2 / Reviewer-2 P2: list_pinned_cids called is_raw_cidv1 on the cid column before decoding it as Option<String>, so SQL NULL failed the decode and the new Pinata-only rows never reached the handler. Decode cid as Option first, drop only the both-NULL case. The is_raw_cidv1 filter was removed entirely: the new contract lists every row that has something to advertise, and the handler at api/ipfs.rs:list_pins is the seam that decides what to do with a legacy-shape row (the resolver 404s on a mismatched key, per #173 U4). Updated list_pinned_cids_omits_unrepaired_legacy_row to match. Reviewer-1 P2: two new sqlx::test cases pin the gate order: sweep_skips_quarantined_repos_before_scan (SQL filter, plus a defense-in-depth per-row re-check) and sweep_skips_private_repos_before_scan (per-repo listable_at_root). Both use expect(0) on the mock IPFS so any future reorder that moves the scan ahead of the gate fails the build. Also: record_pinned_cid ON CONFLICT now rewrites cid (R1-P2: a stale wrong CID from a previous push is repaired by a subsequent one). record_pinata_cid ON CONFLICT clears cid to NULL when the existing row has the legacy cid = pinata_cid shape; on INSERT, stores cid = NULL when raw_cid == pinata_cid (the Pinata-only signal). * feat: implement per-backend continuation offset for missing OIDs - Enhance `missing_oids` function to accept a `start_after` parameter, allowing for rotation of the missing set based on the last attempted OID. - Update `run_pass` to load and save the continuation offset for both IPFS and Pinata backends, ensuring that previously attempted OIDs are retried last in subsequent passes. - Introduce tests to validate the behavior of the new offset mechanism, ensuring deterministic ordering and proper handling of gaps in the missing set. - Ensure that the offset is cleared when no missing OIDs are found, allowing for fresh starts in subsequent passes. * fix(node): trace provenance/structural invariants through every consumer The v30 local_ipfs_provenance column was set by the writers but only consulted by has_ipfs_cid and filter_ipfs_pinned_oids. The pin loop's "is this already done" check still used row existence (is_pinned), so a Pinata-only row hit the early-return branch without ever reaching the local writer, and the flag stayed FALSE forever. Three other seams carried the same shape-vs-provenance confusion. P1 (ipfs_pin.rs:1843): pin loop's skip keys on has_ipfs_cid (writer-owned local_ipfs_provenance = TRUE), not on row existence. A Pinata-only row now falls through to the local writer path, which calls record_pinned_cid_with_source and flips the flag on the conflict branch. The Pinata pin loop's skip remains keyed on has_pinata_cid (a separate seam). P1 (db/mod.rs v30 backfill): tighten the WHERE clause to cid IS NOT NULL AND pinata_cid IS NULL. The previous permissive (cid <> pinata_cid) clause marked pre-v30 Pinata-first uploads (where record_pinata_cid deliberately stored cid = raw_cid without a local Kubo push) as local IPFS pins, so the sweep filtered them as already-durable and an operator enabling IPFS later never got a local recovery copy. Ambiguous dual-shape rows stay at FALSE under the strict rule and the sweep re-derives on re-pin (cheap, idempotent). P1 (db/mod.rs record_pinned_cid_with_source ON CONFLICT): restore cid = COALESCE(pinned_cids.cid, EXCLUDED.cid). A Pinata-only row with cid = NULL (the raw_cid == pinata_cid shape) would otherwise be marked complete with local_ipfs_provenance = TRUE and a NULL resolver key. P1b (visibility_pack.rs): remove the synthetic "/" path on root trees and add a structural entry-level check (structurally_safe_root_tree / _for_caller) in both the sweep gate (allowed_blob_tree_sets_bounded) and the per-request tree gate (allowed_tree_set_for_caller_bounded). A tree is admitted only if every direct entry in its serialized bytes is safe at /{filename} and (for tree entries) the child tree is itself in the allow set. The previous path-based "/" check let the root tree of a public repo slip through even when its bytes name a denied subtree entry. P2 (api/ipfs.rs list_pins): add local_ipfs_provenance to PinnedCidRecord, include in list_pinned_cids SELECT, and surface as local_pinned / pinata_pinned in the JSON response. A Pinata-only row previously returned the raw resolver key as local_cid, making it indistinguishable from a real dual-backed row. The new fields let a gl consumer distinguish local-only, remote-only, and dual rows without re-inferring semantics from nullability. local_cid is preserved for backward compatibility. Tests: - migration_v30_backfills_local_ipfs_provenance_heuristically: flipped both-distinct expectation to FALSE under strict rule. - migration_v12_makes_cid_nullable_and_preserves_classification and migration_v27_clears_legacy_equal_cid: both-distinct / distinct row expectations flipped to FALSE. - sweep_never_pins_root_tree_naming_withheld_subtree (new): assert root tree + secret subtree tree absent from pinned_cids. - allowed_tree_set_gates_withheld_subtree_tree: root tree assertion inverted to EXCLUDED for anon. - list_pins_reports_writer_owned_provenance_for_all_shapes (new): assert 4 row shapes produce correct local_pinned / pinata_pinned JSON. Scope discipline preserved: PolicyFence capture order, _pin_permit reuse for the seal phase, fresh per-arm rederive_budget, keyset repo cursor with lookahead, mirror-row skip, Kubo 2xx-without-Hash refusal, v27 provenance-clearing migration all untouched. v31 schema unchanged. * fix(node): structural tree gate at every depth + non-commit ref tolerance The prior round installed a `local_ipfs_provenance` column and applied a structural tree gate only to root trees. The fresh review pass found two structural gaps the writer contract didn't propagate to: P1 (visibility_pack.rs:1519): the path-based tree admission admitted a tree whenever its path was allowed, regardless of whether its serialized bytes named a denied subtree. Concrete failure: a public repo with `/public/secret/file` and a `/public/secret/**` deny admitted the `/public` tree (its path is `/public`, allowed) but `/public` still contained a `secret/` entry pointing at the denied subtree — anyone who obtained `/public`'s CID could `cat-file -p` it and read the subtree's name and child OID. The prior round's post-pass structural check ran on root trees only. The fix: replace the prior round's `structurally_safe_root_tree` / `_for_caller` / `root_tree_oids` helpers with one recursive `tree_structurally_safe` that walks every tree's entries at every depth, recursing on tree entries. Bundled the rule/path args into a `TreeCheckCtx` struct to fix the clippy `too_many_arguments` warnings the prior round introduced. A tree is admitted iff every direct entry is safe at its path AND (for tree entries) the child tree is itself structurally safe there. A denied-subtree tree fails at its level; the parent fails because the denied-path child entry fails; the propagation continues up to the root. The `admitted` set memoizes trees proven safe at some path so the recursion short-circuits on cycles and on trees reachable at multiple allowed paths. P1 (visibility_pack.rs:533): `all_object_paths` called `assert_all_refs_are_commits`, which bailed on any ref that didn't peel to a commit (tag-of-tree, tag-of-blob). The sweep (`run_pass`) converted that error into a full-scan failure that skipped the whole repo. A repo with a pushable tag-of-tree never got its missing public pins repaired by the new hourly worker, even though its commit-reachable graph was otherwise safe to classify (`ipfs_cid_tree_served_despite_non_commit_ref` is the in-repo example of that supported shape). The fix: remove the `assert_all_refs_are_commits` call from `all_object_paths` (and the analogous one in `blob_paths`, used by the encrypted recovery path — the same reasoning applies). `git rev-list --all` already silently skips non-commit refs, so the commit-reachable object set is what the sweep needs. The unused `assert_all_refs_are_commits` helper is removed. Unclassifiable ref targets still fail closed at a different layer: the cat-file catch-all enumerates them with no path, and the path-based allow filter drops empty-path entries. Test inversions and new regressions: - `migration_v12_makes_cid_nullable_and_preserves_classification` and `migration_v27_clears_legacy_equal_cid` — both-distinct / distinct row expectations flipped to FALSE under the v30 strict backfill (prior round's tightening is re-affirmed here; the v30 backfill is `cid IS NOT NULL AND pinata_cid IS NULL`, the only unambiguous local-IPFS shape). - `ipfs_cid_gate_withholds_blob_from_unauthorized` — root tree assertion inverted to NOT_FOUND (the recursive structural gate denies root trees whose entries name a denied subtree). - `ipfs_cid_legacy_provider_cid_repaired_on_repush` — legacy-shape row insert now sets `local_ipfs_provenance = TRUE` to reflect that the bytes ARE on local IPFS (the legacy CID is just the wrong key). - `ipfs_cid_tree_served_despite_non_commit_ref` — root tree assertion inverted (same reason as the gate test). - `fails_closed_on_annotated_tag_of_a_blob` and `fails_closed_when_a_ref_cannot_be_traversed` — renamed to `skips_*`, assertions inverted from `Err` to `Ok` (a ref pointing at a non-commit object is no longer a walk error; the blob falls out of the gap set because `git rev-list --all` skips it). - `ipfs_cid_walk_error_fails_closed` — removed (relied on the now-removed `assert_all_refs_are_commits` to force an error; the fail-closed arm is still covered by `ipfs_cid_commit_tag_walk_error_fails_closed`, which uses a different trigger — a nonexistent ref object makes `rev-list --all` fail). - `sweep_does_not_publish_public_ancestor_tree_naming_withheld_subtree` (new) — recursive depth test: a `/public/secret/**` repo asserts root tree, `/public` tree, and `/public/secret` subtree are all absent from `pinned_cids` while `/public/visible.txt` is IPFS-pinned. - `sweep_repairs_commit_reachable_object_in_repo_with_tag_of_tree` (new) — tag-of-tree repo with a separate `mktree`'d tree (not commit-reachable); asserts the commit-reachable blob is IPFS-pinned and the unclassifiable tag-of-tree is not. Scope discipline preserved: PolicyFence capture order, `_pin_permit` reuse for the seal phase, fresh per-arm `rederive_budget`, keyset repo cursor with lookahead, mirror-row skip, Kubo 2xx-without-Hash refusal, v27 provenance-clearing migration, v30 strict backfill, v31 schema, per-(repo, backend) reconciliation offset, and the `local_ipfs_provenance` writer/reader contract are all untouched. * test(node): ignore pre-existing RED replication-tail test on CI `api::repos::tests::receive_pack_tail_survives_a_disconnect_during_release` documents an open bug in `api/repos.rs` (the replication tail must be spawned above `release` and gated on `receive_result.is_ok()`; today it's spawned below, so a disconnect during `release` drops the pins, recovery copy, and announce). The test's `assert!` message starts with "RED:" — this is a TDD-style "keep the design intent visible in the code" test, not a working-behavior assertion. The test was added 2026-07-29 by `beardthelion` (PR #215 lineage), well before the #218 sweep work, and is unrelated to this PR. CI is failing on it under load (passes in isolation) because the open bug hasn't been fixed yet. `#[ignore]` until the open bug is fixed. The test's design intent and the fix description stay in the doc comment and the assert message; only the CI failure is suppressed. Run the test explicitly with `cargo test -- --ignored` to verify the bug is fixed when the tail-spawn moves above `release`. * fix(node): align encrypted manifest anchor log with one-shot contract The "will retry next pass" log at reconciliation.rs:1034 was false: plan_seal returns SkipUnchanged on every subsequent pass once the recipients tag matches, so sealed stays empty and the anchor block never runs again. A transient Irys outage at the moment of a fresh seal therefore LOSES that anchor permanently — the next pass has no delta to anchor and no retry fires. This is intentionally best-effort: re-anchoring an unchanged manifest would burn Irys writes for no recovery benefit. Factored the warn message into ENCRYPTED_MANIFEST_ANCHOR_FAILED_MSG and added a unit test that pins the constant; the test goes RED if "will retry" reappears in the log. * fix(node): address eight reviewer findings on #218 reconciliation sweep (round 3) * fix(node): close audit.toml array and drop inline comment * fix(node): repair the stale for-each-ref fake-git fixture (round 8 P1) `upload_pack_shares_one_deadline_across_walk_and_filtered_serve` — the #174 shared-deadline guard for the FILTERED serve arm — was red on both `test (stable)` and `test (beta)`. The fake git answered `for-each-ref` with the single token `refs/heads/main`, a ref NAME, where `blob_paths` phase 2 asks for `--format=%(objectname) %(objecttype)`. The phase-2 parse fails closed on a line it cannot read, so the walk returned an error and the request surfaced as a generic 500 instead of being reaped into a 504. That silently repurposed the test from "the filtered serve shares one deadline" into "the walk errors" — the guard proved nothing while it was red. The fixture is what was stale, not the production path: the arm now emits the column shape the walk parses. A commit tip has no peeled referent, so two columns is the whole line. The 1.2s walk cost stays on `ls-tree` and the serve's on `pack-objects`, so the two phases remain independently attributable. * fix(node): peel annotated tags in the blob_paths ref walk (round 8 P1) `%(objecttype)` reports the type of the ref's OWN object, which for an annotated tag is `tag`. The phase-2 `if kind == "blob" || kind == "tree"` arms therefore never saw the referent, and a blob reachable ONLY through an annotated tag escaped the withheld set — while `rev_list_keep`'s `git rev-list --objects --all` does peel tags and served it. Phase 2 closed the leak for a direct ref-to-blob and for that shape only. The walk now asks for the peeled atoms `%(*objectname) %(*objecttype)` and inserts the referent with an empty path, alongside the direct blob/tree tips it already handled. Line parsing moves to a field-count match: two columns for a non-tag tip (the peeled atoms expand to empty), four for a tag tip, anything else still fails closed. Measured against git 2.50, the `*` atoms peel the WHOLE chain — a tag-of-a-tag-of-a-tag-of-a-blob reports the blob — so a `tag` peeled type does not occur on stock git. That arm is kept anyway, because `push_delta.rs`'s ref-type guard documents `%(*objecttype)` as a one-level peel: under such a git it finishes the chain with `rev-parse <tip>^{}` plus a `cat-file -t` probe rather than bailing and 500ing every clone of a repo that merely carries a nested tag. Both children are bounded by the walk's shared deadline and are unreachable on the common one-line-per-ref path. * fix(node): one empty-path decision for both blob_paths consumers (round 8 P1) `blob_paths` has two consumers and only one of them carried the empty-path policy. `withheld_from_pairs` (the deny side, what the smart-http serve filter excludes) withheld a phase-2 empty-path entry from everyone but the owner; `allowed_blob_set_for_caller_bounded` (the allow side, what `GET /ipfs/{cid}` hands over) called `visibility_check(..., "")` directly. On a public repo no glob matches the empty path, so that returns `Allow`: the serve filter withheld the OID while the IPFS gate served the bytes — the leak phase 2 exists to close, reopened one layer over. The policy moves into `pair_decision`, which both consumers now call, so the two gates cannot disagree and a future change to the empty-path rule moves both at once. `withheld_blob_recipients_bounded` routes through it too: an unclassifiable blob grants a recovery copy to the owner only, never to a rule's named reader whose grant was written against a path this object does not have. No behavior change for non-empty paths — `pair_decision` delegates to `visibility_check` unchanged. * test(node): bind the phase-2 ref tests to a commit-unreachable blob (round 8 P2) `skips_a_ref_pointing_at_a_blob` and `skips_an_annotated_tag_of_a_blob` were vacuous. Both hung their ref on `fixture()`'s `secret` blob, which is COMMITTED at `secret/b.txt`, so phase 1's per-commit `ls-tree` already produced `(secret, "/secret/b.txt")`, the `/secret/**` rule already denied it, and `withheld.contains(&secret)` passed with phase 2 deleted outright. Both now use `orphan_blob`, a blob written straight to the object store that no commit's tree names, so the ONLY route into the withheld set is the `for-each-ref` phase. That is also the real leak shape: `rev_list_keep`'s `git rev-list --objects --all` does follow a ref to such a blob, so an under-withheld one ships in the clone pack. Verified by mutation: with `if false` injected at the phase-2 filter, every test below goes RED (they were green under that mutation before). New coverage: - `ref_only_blob_is_denied_on_the_allow_side_too` — the allow-side denial for the shared `pair_decision`; it also asserts deny and allow agree on one OID, and that the owner IS still admitted, so it cannot pass by denying everything. Reverting the allow side to `visibility_check` turns it red on exactly that assertion. - `skips_a_nested_annotated_tag_of_a_blob` — real-git tag-of-a-tag. - `peels_a_tag_whose_peeled_target_is_still_a_tag` — drives the one-level-peel fallback through the fake-git seam, so that arm is tested rather than dead. * fix(node): advance the reconciliation continuation only for dispatched work (round 8 P2) `ipfs_last` / `pinata_last` were captured from the missing set above the pin permit, both `PolicyFence` captures and both pin loops, then written under nothing but `if ipfs_enabled` / `if pinata_enabled`. Every stage in between can legitimately produce nothing — a fence capture that fails, a quarantine/visibility recheck that says skip, a pin-boundary re-derivation that errors — and each is transient. Advancing past OIDs that were never attempted is not a harmless retry delay: `missing_oids` rotates strictly past the stored offset, so the whole unattempted prefix lands BEHIND the entire backlog on the next pass. For an at-cap repo — the only kind the continuation exists for — the backlog never drains inside one cap window, so those objects are starved indefinitely. That is a durability hole in the durability backstop. The offset now moves only for work actually handed to a backend, tracked at the dispatch boundary as `to_pin.last()` (not the missing set's last: an OID the pin-boundary re-derivation dropped was never offered). It is recorded before the call, so a pin phase that times out mid-batch still counts as dispatched. The write becomes three outcomes rather than two, via one `next_offset_write` helper so the backends cannot drift apart: * work dispatched -> advance to the last dispatched OID * nothing missing, query OK -> None, marking the row done * nothing dispatched, or the missing-set query failed -> write NOTHING, leaving the resume point a capped pass already paid for `*_scan_ok` is what separates the last two: an empty missing set means "everything is pinned" or "the filter query failed and we know nothing", and only the first should mark the row done. Test: `sweep_leaves_offset_untouched_when_nothing_was_dispatched`, using a `#[cfg(test)]` seam on the pin-boundary re-derivation — that stage cannot be starved from the outside, because the mid-scan re-filter runs first on the same rules and budget and makes the sweep `continue` well before the offset write. It asserts the stored offset comes back byte-identical after a declining pass, then that the same repo does advance it once dispatch happens, so the assertion cannot pass on a dead write site. Restoring the old semantics turns it red. * fix(gl): render the Pinata provider CID in `gl ipfs list` (round 8 P2) The server-side wire change landed without its client half. `cmd_list` read `pin["cid"].as_str().unwrap_or("?")` as the entry heading and nothing in the CLI read `pinata_cid` at all, so a Pinata-only row printed as a bare `?` — the listing showed nothing usable for an object that is in fact durably stored. `cid` is legitimately null for such a row: it is the key `GET /ipfs/{cid}` resolves against, and the node stopped aliasing a Pinata provider CID into it (round 3) precisely because that CID 404s there. So the fix is on this side. Rendering moves into `render_pin`, which leads with the node-resolvable CID when there is one and otherwise the provider CID, labelled so nobody feeds it back to this node's resolver. A dual row keeps both visible. A `backends:` line names where the bytes are, read from the writer-owned `local_pinned` / `pinata_pinned` booleans rather than re-inferred from CID shape — which is what the node's own comment warns against. Older nodes send neither flag; those `cid`-only rows fall back to CID presence and render as they always did (`render_pin_handles_a_legacy_cid_only_row`). * fix(node): implement third fence check for IPFS pinning and enhance visibility rules handling * fix(node): enhance documentation and add tests for ProgressState and helper functions * fix(node): address round-9 reviewer findings on #218 reconciliation sweep * fix(node): correct consumer-4 recipient test for call-invariant set * fix(git): fail closed on non-UTF-8 tree listing in walk_tree_oids_inner (#218 round 10 P1) walk_tree_oids_inner previously returned Ok(()) on a non-UTF-8 ls-tree -z listing, inserting only the tree OID and skipping every child blob/tree OID. A direct tree ref (or an annotated tag peeling to a tree) is valid Git input; git rev-list --objects --all still enumerates the tree and every descendant, so the keep-side removed the tree from the served set but passed the child blob OIDs to pack-objects, exposing their bytes to an anonymous clone. Phase 1 already bails on the same input at the blob_paths walk; this fix puts the tree-walk on the same fail-closed outcome. The new fails_closed_on_non_utf8_tree_tip test pins the invariant for both a direct refs/tags/direct-tree ref and an annotated tag-of-tree (the two non-commit shapes round 9 added tolerance for), confirming the walker bails rather than returning Ok with a partial withheld set. * fix(recon): run encrypted recovery even when public list is empty (#218 round 10 P1) A path-scoped repo whose only reachable object is a direct blob or direct tree ref yields an empty public list (the anonymous public classifier removes the only object from the served set) while withheld_blob_recipients_bounded still assigns that object to the owner recovery set. The early `continue` on object_list.is_empty() (and the second one after the mid-pass refilter) suppressed encrypted recovery too, and a lost or failed encrypted copy was never repaired. Track empty-public-work as a flag and gate the public phase on it. Encrypted phase 2 runs regardless. The backend enable flags and `_pin_permit` move out of the gated block so phase 2 can read them when the public phase did not run. * fix(db): rename migration v12 test to v32 (#218 round 10 P3) The test name and its docstring/comments still say "v12", but the cid-nullable migration is at v32 (line 1163). The test itself is correct — it drives the real path — so this is naming drift rather than dead coverage. A future reader looking for the migration would otherwise chase a v12 that no longer exists. * fix(git): index OID membership before quadratic classifier scan (#218 round 10 P2) all_object_paths was doing O(O×P) work in two places: the catch-all cat-file branch checked `blob_set.iter().any(|o| o == oid)` and `tree_set.iter().any(...)` for every reachable object, and the phase 1 loop also did a per-commit unused `git rev-parse <commit>^{tree}` probe whose result was discarded (the root-tree OID is recomputed by `root_tree_oids`). On the 50k-object repos the sweep exists for, the quadratic scan and the redundant per-commit child process can consume each authorization deadline before any object reaches a backend, producing a permanent hourly skip. Maintain a separate `blob_oids` / `tree_oids` HashSet alongside the path-pair sets so the membership check is O(1). Drop the unused per-commit rev-parse probe. * fix(git): bound tree walk by ls-tree invocation count, not just depth (#218 round 10 P2) walk_tree_oids_inner stopped at MAX_TREE_WALK_DEPTH and the shared deadline. A wide shallow tree reachable from N ref tips or from multiple parents still spawned one ls-tree child process per subtree well inside the depth cap, and the wall-clock bound could not stop that — expiry was the only stop. The ceil was wall-clock only. Add a structural invocation cap (MAX_TREE_WALK_INVOCATIONS) the walker fails closed at, and a memo of walked tree OIDs so the same tree reachable from multiple ref tips is walked once. Round 10 P2. * fix(pinata): suppress push when DB record is not durable (#218 round 10 P2) pin_new_objects pushed (sha, cid) on provider POST success regardless of whether record_pinata_cid persisted the row. A closed/failed DB write or an explicit-transaction source record timeout meant the next sweep queried the DB, found no row, and re-offered the same gap — with reconciliation having counted a fill that never happened. Track a `db_record_durable` flag. On hard failure of either record_pinata_cid (non-timeout) or record_pin_source (any failure, since it's a multi-statement transaction), set the flag false and skip the `pinned.push`. The `BoundedDbError::Elapsed` autocommit case stays as the previous comment's "may have committed" reasoning: the push fires so the reconcile still sees the fill. Round 10 P2. * fix(node): serialize policy narrow with irreversible upload via in-process mutex (#218 round 10 P1) The PolicyFence's epoch read was a one-way read: a visibility narrow (rule insert, quarantine set) could commit between the fence's epoch read and the upload's HTTP POST, and the upload's DB-record INSERT's epoch check would see the new epoch and reject — but the provider already accepted the object. Encrypted envelopes had no fence at all. Add a per-repo in-process mutex that PolicyFence acquires at capture and releases at drop. The narrow paths (set_repo_quarantine, set_visibility_rule, remove_visibility_rule) acquire the same lock for the duration of their DB transaction. A narrow therefore blocks until the in-flight batch finishes, and an in-flight batch waits for the narrow to commit before it even reads the epoch. Multi-process / multi-node is a known gap: a Postgres advisory lock per repo would extend the same guarantee across processes. Deferred to a follow-up. * fix(pinata): update post-upload stalled record test for round 10 P2 contract The previous test asserted that a successful upload with a timed-out post-upload source record still returned the (sha, cid) pair. That assertion pinned the buggy behavior the reviewer called out: the reconcile would count a fill that was not durable, then re-offer the same gap. The test name and the `record_pin_source` arm of `pin_new_objects` are unchanged; the assertion now reflects the new contract: when the post-upload source record times out (its `mark_pin_sources_incomplete` already sets the source set to incomplete, so the next pass re-offers), the pair is suppressed so the reconcile sees the gap as not-filled. Round 10 P2 follow-up. * fix: clippy allow + rustfmt on round 10 commits clippy::too_many_arguments on walk_tree_oids_inner (8 args after the round 10 memo + invocation counter threading). rustfmt re-ran on the if/else blocks added in the has_public_work refactor (round 10 P1 #42). No behavior change. * fix(node): address round-11 review findings on #218 reconciliation sweep P1: remove the whole-batch policy mutex. PolicyFence no longer holds a per-repo lock from capture through drop and the narrow paths (set_visibility_rule, remove_visibility_rule, set_repo_quarantine) no longer acquire it, so a visibility narrow commits immediately and the batch aborts on its next is_current check instead of the narrow blocking behind the sweep. Accepted residual is a single in-flight object; the fenced DB record still refuses to land a raced row as durable. encrypt_and_pin_stops_sealing_when_reader_removed_mid_batch is green again. PolicyMutexes is deleted, resolving the key-leak P2 by removal. P2: cover the round-10 has_public_work branch with sweep_seals_withheld_blob_when_public_list_is_empty: a direct-blob-ref repo yields an empty public list with a non-empty owner recovery set; asserts no public gaps/fills, no cleartext pin, exactly one POST (the seal envelope), and a recorded encrypted copy. P3: peel-case isolation in the non-UTF-8 tree-tip test (delete the direct-tree ref before the second call so the peel arm must run); v12->v32 rename drift in the migration test (boundary, comments, and the stale (2)->true doc line); accurate comments for the pre-refilter flag and the post-refilter fall-through. * fix(node): address merge-readiness findings on #218 reconciliation sweep Reworks the sweep around explicit state-machine contracts instead of adding conditionals: backend results carry last_attempted plus confirmed durability separately (PinBatchOutcome), provider permits scope to provider effects only, discovery pages by commit window with a persisted cursor, legacy provenance migrates as unknown, and the pin list advertises only validated raw local keys. - Empty-path owner recovery without path rules: phase 2 drops the has_path_scoped_rule gate and runs on the actual recipient result, with a dedicated recovery recheck (no anonymous-listability requirement); matrix test over blob/tree/tag refs x zero/root rules, including recreate-after-delete. - Pinata Elapsed is non-durable unless verify_pinata_record proves the exact row and fence epoch; loop test with row-locked repos. - list_pins gates cid/local_cid on is_raw_cidv1 plus confirmed provenance; response-shape matrix uses real CIDs with a legacy row and GET /ipfs follow-through; docs match the nullable wire contract. - Permits acquired per provider effect and dropped after; no nesting at pool size 1 (test renamed to the new shape). - Scan discovery pages SCAN_COMMIT_WINDOW commits oldest-first with a node_state cursor (advance on drained/empty/attempted, delete on cover); refilters and phase-2 recipients run on the same window; walk-layer paging/invocation/union tests plus a 1050-commit cursor-to-completion sweep test. - v35 migrates legacy rows as unknown (no TRUE backfill); v35 test asserts unknown + preserved Pinata history; sweep re-derivation test re-pins instead of filtering. - Drained completions DELETE continuation rows (no tombstones); FK cascade on repo delete; row-count regression test. - Audit: lru waiver gets an explicit retirement owner plus a weekly drift guard; revocation model recorded as a product decision on PolicyFence (prompt revocation, single-in-flight residual). * fix(tests): configure git identity in bare clone for tag fixture The windowed-union test creates its annotated tag in the bare clone, which carries no config. It passed locally only via the developer's global git identity; CI has none, so 'git tag -a' failed there. Verified with an empty GIT_CONFIG_GLOBAL/SYSTEM. * fix(tests): ignore pre-existing hung-git pid race on CI read_object_bounded_returns_by_deadline_with_a_hung_git reads the fake's pid file with no readiness handshake; a slow scheduler yields 'No such file or directory' (stable-only red, identical beta green). Unrelated to #218 (store.rs untouched); re-enable with a readiness signal from the fake. * fix(node): address round-12 review findings on #218 reconciliation sweep - P1 roots: admit structurally safe root trees at "/" into the allow set and candidates, so safe snapshots fully replicate; roots naming a withheld subtree stay excluded via the recursive structural check. - P1 per-pass WalkBudget on sweep enumeration (entries / output bytes / git invocations) with fail-closed budget-exhaustion; serve paths keep the uncapped budget. - P2 recovery lane: independent recovery cursor pages unlistable history across restarts instead of sharing the scan window. - P2 gap-hold: failed gap queries hold discovery advance but not the healthy backend, via test-only failure seams. - P2 nested tags: collect full tag chains so nested tag objects pin without an inner ref. - P2 RepoStore: thread the Tigris-backed store into the sweep with an acquire timeout and shutdown select; restore cache-miss and skip missing. - P2 Pinata source: keep confirmed pairs when the primary already covers the repo; suppress push for stalled post-upload records without coverage. - Update large-history sweep expectation to blob+root+commit per round. * fix(node): address consolidated review findings on #218 reconciliation sweep P1: wire WalkBudgetExceeded through the sweep cursor logic. Both the public scan and the recovery walk now dispatch the typed budget-exhaustion signal at the sweep boundary: a budget-hit window advances past content the sweep refuses to classify partially (scan cursor save/delete via the existing persist site; recovery cursor likewise), while git/policy/panic/timeout failures keep the cursor for retry. visibility_pack documents the obligation; the match arms are the handler, with a one-line pattern note for future typed walk errors. P2: run encrypted recovery when the public scan fails. Scan walk/panic/timeout arms now fall through with an empty public list instead of continuing the whole repo iteration, so phase 2 runs on the recovery lane's own cursor like the unlistable-repo path. The scan cursor stays untouched for retry. Refilter/recheck-public failures remain fail-closed whole-iteration continues by design. Includes a repo-scoped test seam (process-global set, safe across spawn_blocking and parallel tests via fresh UUIDs). P2: bump policy_epoch only when remove_visibility_rule deletes a row, mirroring set_repo_quarantine's rows-affected guard, so a typo'd or double-deleted glob cannot abort fenced batches. Also adds the repo-loop outcome matrix (every early exit × scan cursor / recovery cursor / phase 1 / phase 2) and deny tests: budget-hit advances both lane cursors with zero pins/seals (real 250k-entry trip, early-trip fixture runs in ~30s), scan failure still seals the recovery copy, delete-missing-rule leaves the epoch unchanged. Each deny was verified by temporarily restoring the old branch. * fix(node): address jatmn review findings on reconciliation sweep re-home * fix(node): reload persisted sweep cursor between passes * fix(node,gl): pinata offset warn wording and boundary-safe pin timestamp --------- Co-authored-by: Gravirei <[email protected]> Co-authored-by: Kevin Codex <[email protected]> |
||
|
|
c805e35f40 |
fix(node): bound GraphQL query cost (#552)
* fix(node): bound GraphQL query cost * fix(node)!: bound repo names so emitted cursors always parse The reposPage cursor embeds base64(JSON(owner_did, name)) while parse_repo_cursor rejects cursors over 4096 bytes, but repo names were unbounded on every write path. A single long-named visible repo made the server emit an endCursor it then rejected as "invalid repository cursor", dead-ending pagination for every caller past that row. Bound names to MAX_REPO_NAME_LEN (100 bytes) at the create_repo and fork_repo handlers via a shared validate_repo_name helper (bad charset is still a 400), and reject overlong peer-supplied names in Db::upsert_mirror_repo (the sync caller already drops the error and skips the mirror). Also in this round: - docs/graphql-pagination.md states the 200 page-size maximum and corrects the over-budget remedy: a smaller limit does not reduce the reposPage or repos price, which is charged at the full MAX_VISIBLE_REPO_PAGE_SIZE scan regardless of limit. - Tests pin refUpdates(limit: -1) clamping to zero rows, the parse_repo_cursor wrong-shape-JSON arm with its exact "invalid repository cursor" message, the longest-admissible-name cursor round-trip, and the mirror write-path rejection. BREAKING CHANGE: the GraphQL `repos` field now returns an error instead of the full list when more than MAX_VISIBLE_REPO_PAGE_SIZE (200) repositories are visible; larger lists must use `reposPage` with `limit`/`after`. Repository names are additionally limited to 100 bytes at creation, fork, and mirror-registration time. * fix(node): address beardthelion P3 findings on name bound and input messages - Assert each rejection's own message in the reposPage invalid-inputs loop: the loop no longer accepts either message for every query, so a misattributed message (limit arm returning the cursor message) goes red. - Pin the create/fork name-bound wiring through the handler: new test drives create_repo with an overlong name via test_state_lazy and asserts the 400; deleting the validate_repo_name call site would proceed to the lazy DB pool and fail with a connection error instead. - Document the API-side vs disk-side validate_repo_name divergence on the new function, and tie the store's literal 100 to MAX_REPO_NAME_LEN so the two validators cannot drift apart on length. --------- Co-authored-by: Marlowe <[email protected]> Co-authored-by: cairn-intern <[email protected]> |
||
|
|
a4026e84ff |
fix(node): honor pre-assigned task assignee on claim (#550)
* fix(node): honor pre-assigned task assignee on claim Stop strangers from overwriting a reserved assignee_did (and receiving the UCAN). Bind finish_task to the stored assignee; return 403 on reserved steals; keep exact stored DID forms; normalize blank assignees; pin production claim/finish SQL race guards with FOR UPDATE tests; trim broadcast by_did; and tighten blank/SQL re-check regressions. * test(git-remote-gitlawb): wait for shim before real_git_fetch on Windows Windows CI can race: git fetch connects before the shim accept loop is polling, yielding connection aborted (os error 10053) with 0 POSTs. Probe the listener after spawn so multi-round and withheld-path tests start only once the shim is ready. * test(git-remote-gitlawb): retry transient Windows fetch connection errors Windows CI can still abort localhost connections (os error 10053) after the shim is ready. Retry fetch_with_helper up to three times on transient connection failures before asserting. * test(git-remote-gitlawb): revert Windows fetch workarounds The retry-on-10053 and wait-for-shim probes papered over the accepted-socket inheriting the listener's non-blocking state. #312 fixed that at the source (normalize_accepted_stream) and is merged, so take the workarounds back out and let the real fix carry the Windows lane. * test(git-remote-gitlawb): pin the fetch retry's absence with an exact GET count The POST assertions are floors, so a reintroduced fetch-level retry could satisfy them with a second full fetch while masking the withheld shape. A retry cannot avoid re-advertising, so count GET /info/refs in the shim and assert exactly one per fetch in both the multi-round and withheld tests. The merge from main also brings #312's normalize_accepted_stream into this head, closing the round's rebase ask. * fix(node): mirror assignee normalization across GraphQL task mutations GraphQL createTask returned the pre-insert input, so a whitespace-only assignee came back as a reservation the store never kept; complete and fail compared the caller against the raw stored assignee where REST and Db::finish_task trim; and task-event broadcasts reported the caller string rather than the stored assignee form REST publishes. All three paths now mirror the REST and DB contract: the create response is the stored row, finish gates compare the trimmed stored assignee, and by_did on claim/complete/fail broadcasts derives from the returned row. --------- Co-authored-by: Ayush7614 <[email protected]> Co-authored-by: Kevin Codex <[email protected]> |
||
|
|
38bab4191b |
fix(node): version ref_certificates for forward compat (#26 split 3/4) (#548)
* fix(node): version ref_certificates for forward compat (#26 split 3/4)
Reviewer 2 closed PR #224 on 2026-08-28 with a directive: split
into four narrow PRs. This is Split PR 3 (certificate / CLI
compatibility). No findings are assigned to it; it is required
because the protocol/client work is independently reviewable and
should not remain coupled to post-receive recovery.
The wire-format problem: the ref-cert payload is a fixed 7-field
JSON blob the node signs. Future changes (additional pusher
attestations, content-binding fields, secondary signatures) would
either break every existing cert or be impossible to ship without
a flag day. Versioning the cert makes the format forward-compat
and explicit.
NEW MIGRATION v28
ref_certificates gains `version INTEGER NOT NULL DEFAULT 1`.
DEFAULT 1 means an existing row reads as v1 with no backfill
needed; NOT NULL means a missing value is a hard error rather
than a silent v0.
NEW FIELD ON RefCertificate
`version: u32`. v1 is the pre-versioning 7-field payload
(no `version` key in the signed JSON, so a v1 cert is
byte-for-byte identical to today's cert). v2+ certs will
include a `version` key and any new fields, both
backwards-compatible: an old client reading a v2 cert sees the
unknown `version` field and either ignores it (graceful) or
refuses to verify (explicit).
API CHANGES
- list_certs and get_cert now include `version` in the JSON
response.
- issue_ref_certificate sets `version: 1` on every new cert.
GL CHANGES
- gl cert show reads the `version` field; missing field
defaults to 1 (forward-compat with old servers). v1 certs
go through the unchanged verify path. v2+ certs are
explicitly rejected with an "upgrade the client" message
rather than silently guessing the payload shape — a future
client that supports v2 cannot accidentally treat a v1 cert
as v2.
GOLDEN-FORMAT FIXTURE
v1_payload_matches_frozen_canonical_form pins the v1 signing
payload byte-for-byte. Reverting this to a v2 shape (adding a
`version` key) is what the versioned format forbids: an old
client reading a v1 cert must not see a `version` key in the
signed JSON. The same byte-form is what gl's verify_signature
reconstructs in crates/gl/src/cert.rs, so any drift between
node and CLI breaks the test instead of silently rendering
every real cert INVALID.
TESTS
- 3 v1_payload_tests in crates/gitlawb-node/src/cert.rs:
frozen-canonical-form, round-trip structural, default
version is 1.
- 2 new tests in crates/gl/src/cert.rs:
missing_version_defaults_to_1_and_verifies,
explicit_version_1_takes_v1_path.
- 12 existing ref_certificate_tests still pass with the
version column added.
Compiles clean, 1091 tests pass with 0 regressions, gl 4 cert
tests pass, clippy clean under -D warnings, fmt clean.
Cross-PR overlap (declared in the PR description):
- #134 (anchors auth): independent. The cert API is unchanged
in shape; auth gates apply as before.
- #285 (advisory-lock session affinity): independent.
- #306 (Content-Digest on signed requests): independent.
- #314 (small-order Ed25519): independent. v1 certs verify
through the same path, which already enforces the small-
order check from #314 on the embedded node DID.
- #324 (libp2p keypair persistence): independent.
- #325 (gossip ref-update auth): independent. PR 1's signed
HTTP push is the durable-intent producer; this PR owns the
cert shape. The two PRs read and write the same cert table
but do not step on each other (PR 1 calls
issue_ref_certificate_idempotent; this PR keeps the
deterministic cert id but does not change who calls what).
- #382 (replication withheld-subtree trees): independent.
* fix(node,gl): address review feedback on cert version compat
- Drop the duplicate `version` ALTER from the v1 migration bundle; v28
is now the sole owner of the column.
- Add a v28 upgrade-path test (v25/v26 pattern) that seeds a legacy
pre-v28 ref_certificates row, drops the column + migration record,
re-runs migrations, and asserts DEFAULT 1 + legacy row reads as v1.
- Extract a shared `v1_signing_payload` builder so the live signer
and the frozen-vector test share one literal; a regression in the
signer now fails the test.
- Tighten gl's `version` parsing: missing key → v1, present value
must be a JSON integer that fits in u32 and equals 1. Null, strings,
floats, overflow, and unknown versions all return Err instead of
collapsing to v1 (Reviewer 2). Add a truth-table test pinning every
cell of the parser.
- Extend the v1 forward-compat tests to actually round-trip through
`verify_signature`, and add a regression test that pins the verdict
branch rejects v2 even when the v1 signature would otherwise verify.
* fix(cert): address CI failures on PR #386
- cargo fmt --all (CI fmt check was red on the prior commit)
- v28 upgrade-path test: switch version_column_default from
fetch_one to fetch_optional, so a pre-v28 database with the
column dropped returns Ok(None) rather than RowNotFound. The
precondition assertion is what detects a missing column;
RowNotFound would have masked a regression.
- clippy `needless_borrow` on the v2 verdict-branch test:
drop the leading `&` on `cert_json["signature"].as_str()`.
* fix(gl,node): make the v2 rejection and issuer anchor real, and test them as such
Round-2 findings, all three demonstrated by execution:
- cmd_show's Ok(v != 1) verdict arm was dead code (parse_cert_version
admits only 1), and the test aimed at it copied the match instead of
calling the command. The match now mirrors the parser's contract and
two end-to-end mockito tests drive cmd_show itself: a version-2 cert
with a valid v1 signature fails --verify with the version named.
- --verify no longer accepts the queried node's self-reported DID as a
trust anchor: the node that served a forged cert can serve a matching
self-report, and that fallback passed a wholly forged certificate with
exit 0. --verify now requires --expect-node before claiming a trusted
issuer, with a test proving the refusal (and the anchored positive
control passing).
- The node-side version test asserted version == 1 on its own literal;
it now cross-checks the issuing construction: a version-1 claim must
verify against the v1 payload builder's bytes rebuilt from the cert's
own fields, and the v1 payload must carry no version key.
* fix(node,gl): address round-2 reviewer findings on #26 split 3/4 cert compat
BREAKING CHANGE: gl cert show <id> --verify now requires --expect-node <did>; the node's self-reported DID is no longer trusted as a cert anchor.
* fix(node): bind cert v2 test to live issuer via sqlx::test
* fix(node,gl): address round-3 reviewer findings on #26 split 3 cert compat
- Stamp new certs v1 until v2 verifier ships; v2 issuance refused by shipped gl
- Add v2_signing_payload with version in signed bytes, downgrade test
- Verify signature in well-formed test, add DB read-back to issuer test
- Fix README/db comments and version-refusal message
* fix(gl): route cert list through error_for_status; correct cert show example
Round 4 reviewer findings on #26 split 3 cert compat:
- P2 (cert.rs:113): cmd_list called .json() straight through on the
cert list endpoint and let a gated 404 (whose body lacks a
`certificates` key) fall into the empty-list branch, printing "No
ref certificates" with exit 0. Wire cmd_list through the same
error_for_status() gate that cmd_show and resolve_cert_id already
use, and pin the new behavior with a load-bearing test
(`cmd_list_surfaces_gated_404_as_err`) that drives cmd_list end
to end against a 404 mock. cmd_show is unchanged.
- P3 (README.md:314): the documented `gl cert show <id> --verify`
example bound the certificate ID as the REPO positional, because
the parser is `gl cert show [OPTIONS] <REPO> <ID>`. Insert the
missing `<repo>` positional so the example matches the CLI
parser.
The original BREAKING CHANGE footer in
|
||
|
|
d84c445f94 |
fix(gl): derive View: URL from node instead of hardcoding gitlawb.com (#370) (#549)
* fix(gl): derive View: URL from node instead of hardcoding gitlawb.com (#370)
gl repo create and gl mirror print a View: link that was unconditionally
pointed at gitlawb.com, which 404s for repos on self-hosted nodes.
Make the node advertise an optional web_url on GET / (sourced from the
new GITLAWB_WEB_URL env var). The CLI fetches GET / after repo creation
and only prints View: when the node supplies web_url. Nodes without the
config skip the line entirely, which is better than printing a broken link.
* fix(gl): validate web_url response and trim trailing slashes (#370)
Address review feedback:
- Check GET / response status before parsing; only omit View: on
success responses that lack web_url
- Trim trailing '/' from web_url to avoid double-slash in the URL
- Skip the line when web_url is empty after trimming
* fix: address review feedback on View-URL PR (#370)
- Extract fetch_node_web_url() helper for testability; add 6 unit tests
covering: web_url present, trailing-slash trim, absent, empty string,
whitespace-only, and server error
- Reject empty/whitespace-only GITLAWB_WEB_URL at boot in Config::validate()
- Trim whitespace from web_url in the CLI helper (not just trailing slashes)
- Document GITLAWB_WEB_URL in .env.example alongside GITLAWB_PUBLIC_URL
- Mirror uses shared fetch_node_web_url() helper
* fix: validate web_url format and surface node info failures (#370)
Node side: GITLAWB_WEB_URL must now parse as an absolute http(s) URL —
scheme-less hosts and other garbage fail at boot instead of serving a
View link no browser can follow.
CLI side: a non-success GET / status or a malformed advertised web_url
now warns on stderr (with the status/value) instead of silently dropping
the View link; successful responses lacking the field still omit it
quietly, as self-hosted nodes without a web front-end are expected to.
* fix: reject query/fragment web_urls and warn on node info errors (#370)
Both web_url validators now reject URLs carrying a query string or
fragment: the CLI builds View links by string-appending /{owner}/{repo},
so "?a=1" would swallow the repo path into the query. The CLI also no
longer swallows transport-level failures on GET / — a refused or failed
connection warns on stderr like the other degraded-link paths.
* fix(gl): cap node info body and reject control-byte web_urls (#370)
Treat GET / like every other caller-chosen node reply: bound the body
read at 8 KiB and parse from the cap (peer.rs precedent). A web_url
containing control or bidi characters — which passes URL parsing but
would reach the terminal verbatim through the View: line — is now
rejected as malformed, with a defanged preview in the warning.
* chore: bump h2 to 0.4.19 for RUSTSEC-2026-0258
h2 0.4.13 is vulnerable to unbounded empty DATA frames (remote DoS);
0.4.16+ carries the fix. Lockfile-only bump, no manifest change.
* fix(gl): close merge regressions on #370 branch and pin View: with integration test
The
|
||
|
|
0189990ac9 |
fix(node): bound federated repository aggregation (#546)
* fix(node): bound federated repository aggregation * fix(node): propagate federated peer truncation * fix(node): Bound federation latency and preserve truncated peer pages. * fix(node): complete federation review follow-ups Register the bounded peer-query fixture in the writer ledger, name federation bounds, and share the state-owned limiter across routers and the cleanup sweep. Cover exhausted-bucket rejection before database access. * fix(node): pin federated response contract end to end (#466 review) Address beardthelion's P2: the federated envelope was only exercised at unit level, so hardcoding "truncated": false, deleting the failed-peer truncated flag, or dropping node_did from enrich_federated_repo each left all nine federated tests green. Add four route tests driving build_router and asserting on the real HTTP 200 body: - envelope fields (count, nodes_queried, truncated) and local annotations (node_url, node_did, local) for a seeded public repo - truncated == true when the peer table overflows MAX_FEDERATED_PEERS (unreachable peer URLs, no live peer needed) - truncated == true on the failed-fetch path with the table under the bound - peer annotations (node_url, node_did, local: false) via a mockito peer * fix: strengthen federated response bound tests per review - db/mod.rs: add writer-ledger entries and disposition docs for the three federated route tests touching the peers table - api/repos.rs: seed the envelope test repo under a non-node owner DID so node_did must equal the actual node DID; pin the outbound peer query to exact limit=200&offset=0; make the peer-table-overflow test use a reachable stub for every peer so the count bound is the only possible reason for truncated=true, and assert nodes_queried --------- Co-authored-by: euxarist <[email protected]> Co-authored-by: Marlowe <[email protected]> |
||
|
|
b33562f1a9 |
fix(node): bound Identify-derived Kademlia addresses (#544)
* fix(node): bound Identify-derived Kademlia addresses. * fix(node): preserve refreshed Identify addresses. * fix(node): Log rejected known-peer addresses for diagnosis. * fix(node): bound Identify report processing and refresh eviction Avoid retaining peers with empty or rejected address reports, bound canonicalization to a fixed input prefix, and move refreshed addresses behind stale addresses in global eviction order. Add regressions for each bound and below-cap growth. * fix(node): address CodeRabbit majors on Identify address bounds - Reject Identify address admissions once the global address budget is full instead of evicting another peer's entry. Cross-peer eviction let a flood of new identities push honest peers' addresses out of the book and, when the evicted address was their last one, out of the Kademlia routing table. Per-peer eviction of the reporting peer's own addresses is unchanged. - Re-add refreshed Identify addresses to Kademlia, not just newly added ones. Kademlia may have dropped a retained address after a failed dial or never kept it for a full k-bucket, and a TTL refresh alone would never re-admit it. * fix(node): address #462 review findings from beardthelion - remove the write-only insertion_order bookkeeping (field, remove_insertion_token, and its call sites) and collapse the repeated peers.get/get_mut lookups in IdentifyAddressBook::update into a single entry binding - drop a peer row at the end of update() when every candidate was rejected, so zero-address rows no longer linger until the expiry tick - add a regression test proving per-peer eviction still applies at full global budget (fails if the global check is hoisted above eviction) - extract the explicit-address retention predicate into the tested is_explicit_address helper used by both Kademlia removal sites * fix(node): pin row-removal and expiry guards in Identify address book Address beardthelion's round-two P3 findings on #462: - Add test that a fully-rejected fresh peer at full global budget leaves no peers row (pins the empty-row drop in update()). - Add test that update() past IDENTIFY_ADDRESS_TTL reports the stale address in changes.removed (pins expiry reporting). - Add test that expire() drops rows emptied by expiry (pins the sibling drop in expire_peer). - Distinguish per-peer from cross-peer eviction in the saturation test: insert a sentinel peer before the victim and assert its address survives, so a global-oldest eviction policy would fail. * fix(node): pin refresh predicate negative direction per beardthelion round 3 --------- Co-authored-by: euxarist <[email protected]> Co-authored-by: Marlowe <[email protected]> Co-authored-by: cairn-intern <[email protected]> |
||
|
beardthelionandGravirei
|
d3845ced69 |
fix(node): gate /ipfs/pins and /arweave/anchors behind authentication (#121) (#543)
* fix(node): gate /ipfs/pins and /arweave/anchors behind authentication (#134) Add auth rejection to list_pins and list_anchors handlers. The pin index spans the entire node and would expose metadata for every object ever pushed; anonymous callers must not see it. - Require AuthenticatedDid extension in both handlers, return 401 when absent - Add server.rs regression test for anonymous rejection through build_router - Fix closed-pool tests to pass authenticated requests (test 503 path) * fix(node): enforce authentication on /api/v1/ipfs/pins and /api/v1/arweave/anchors routes * fix(node): format arweave routes for consistency in build_router function * fix(node): gate scoped /api/v1/arweave/anchors reads on repository visibility Closes the review follow-up on #134: a signed non-reader could pass the auth-layer check on `?repo=<owner>/<name>` and obtain private-repo anchor metadata (ref names, old/new SHAs, CIDs, irys tx ids). The auth check proved only that the signature was valid; it did not bind the request to the canonical repo-read policy. * Parse the slug through `validate_repo_slug` (same helper the sync path uses) and call `authorize_repo_read` before the SQL query. Missing repos, quarantined mirrors, and signed non-readers all collapse to the standard 404 (`repo_not_found`). * The unscoped global listing stays auth-only (#121 narrowing; the wider permissionless-identity enumeration is the #136 class, out of scope). * Clamp the limit to `[0, 200]` so `?limit=-1` no longer reaches Postgres as `LIMIT -1`. Five new `build_router` regressions cover the full production contract through the real `optional_signature` middleware: scoped unsigned 401, scoped owner 200, scoped non-reader 404 with no anchor metadata leaked in the body, scoped missing-repo 404 indistinguishable from non-reader, and `?limit=-1` clamped to an empty 200. * fix(node): keep /api/v1/ipfs/pins out of the CID resolver's per-IP flood brake Follow-up to #134: the previous reordering put `/api/v1/ipfs/pins` and `/ipfs/{cid}` under the same `rate_limit_by_ip` + `IpRateLimiter` layer stack, so both routes shared the resolver's flood brake bucket. That bucket is documented as the once-per-request brake for `GET /ipfs/{cid}` because the resolver can drive bounded-but-expensive repo walks. The pin listing is a single `list_pinned_cids()` call and has no reason to share the bucket. Sharing meant /ipfs/{cid} traffic could exhaust the bucket and 429 the pins endpoint, and signed pin polling could exhaust the bucket and 429 legitimate CID reads. Compose `ipfs_routes` from two sub-routers with independent policies, then merge: * `/ipfs/{cid}` keeps `optional_signature` + `rate_limit_by_ip` + the `IpRateLimiter` extension. Same behavior as before #134. * `/api/v1/ipfs/pins` gets only `optional_signature`. Auth-required (handler denies with 401), but no rate limit — it has nothing to flood. Two new `build_router` regressions exercise the bucket independence through `TrustedProxy::XForwardedFor`: * 5 signed pin requests against a size-2 CID bucket all 200, then a `/ipfs/{cid}` from the same IP also 200 (pins did not debit the bucket). * A `/ipfs/{cid}` exhausts a size-1 CID bucket (200, then 429), then a signed pin from the same IP is 200 (the bucket does not gate pins). All 65 tests in `server::tests::` + `test_support::tests::ipfs_*` still pass, including the existing 429 tests for the CID resolver. * fix(node): normalize scoped anchor SQL filter to stored repo slug After authorize_repo_read succeeds, the SQL filter used the raw ?repo= string (format!("{owner}/{name}")). Anchor rows are stored with normalize_owner_key(owner_did)/name (short key), so a caller using ?repo=did🔑.../name passed authz but got anchors: [] — a false empty page that broke callers that legitimately pass the full DID form. Build the filter from record.owner_did normalized and record.name instead, matching the shape the seeder uses. * All three findings addressed on bug_fix_2: P2 — signed_scoped_anchors_full_did_form_matches_stored_slug_through_build_router now seeds a second repo under a different owner and asserts irys-other-repo-tx is absent. Negative control: flipping the production filter to None made the test go RED with count: 2 showing the cross-repo leak. P3 — let short = owner_did.split(':').next_back().unwrap() replaced with let short = crate::db::normalize_owner_key(&owner_did).to_string(). Reader and writer now share one convention. P3 — PR #134 description updated: pins bullet now describes the actual pre-merge layering (and cites the axum-layer-vs-merge-pitfall memory entry), and round 2's stored-slug normalization is added as its own bullet with the regression's coverage. 5/5 anchor-related tests pass, cargo fmt --check clean. * fix(node): pass SignatureBodyLimit state to optional_signature layers * test(node): complete listing-denial regressions for the new gates --------- Co-authored-by: Gravirei <[email protected]> |
||
|
beardthelionandGravirei
|
bac49f150a |
fix(node): owner-gate create_task repo_id against hosted repos (#541)
* fix(node): owner-gate create_task repo_id against hosted repos (#496) * fix(node): address review on #496 gate — opaque lookup errors, no-persist asserts, GraphQL arm pins, honest oracle comment * fix(node): address P3 review on #496 gate — shared denial text, honest oracle wording, db-fault pins - Give the REST and GraphQL copies of the create_task owner gate one denial sentence: add REPO_OWNER_ONLY_MESSAGE, used by require_repo_owner and the REST forbidden() envelope, and pin it from both surface tests. - Carry the corrected one-bit-oracle wording into the GraphQL gate comment and the REST test docstring (drop the "oracle-free" claim). - Fault-inject the gate's repo lookups: drop machine_id (get_repo_by_id) for REST and GraphQL, and quarantined (is_repo_quarantined) for REST, asserting the fixed opaque db_error envelope and no schema text. * docs(node): stop claiming caller-scoped task reads in #496 gate comments The create_task gate is a write gate. Unknown/quarantined repo ids are stored verbatim, and the REST/GraphQL task-list paths are caller-independent on this base, so the comments must not imply those ids are read-scoped to the delegator/assignee. Describe only the enforced ownership check; note the one-bit oracle split without asserting a read contract. * fix(node): mirror AppError's 503 split and opaque INSERT in create_task - db_error now sends pool-class failures to 503 db_unavailable like AppError::Db (db_unavailable made pub(crate)); query faults stay the opaque 500. Unit tests cover the split and context-wrapped sqlx errors. - Route the create_task INSERT through db_error instead of serializing e.to_string(); a REST fault-injection test pins the opaque envelope. - The deny/allow matrix reads back the unknown and quarantined tasks to prove the id is stored verbatim, and drops no-leak asserts on the fixed forbidden envelope that could never carry the repo id. * test(node): pin stored-verbatim repo_id on GraphQL gate arms The REST deny/allow matrix reads each accepted task back and asserts the stored repo_id is the supplied id; the GraphQL unknown/quarantined arms only asserted no errors, so a GraphQL regression that dropped, normalized, or rebound the id stayed green. Read those tasks back by the mutation's returned id and assert the stored label matches. * test(node): pin createTask repo-ownership marker in the GraphQL authz fence --------- Co-authored-by: Gravirei <[email protected]> |
||
|
beardthelionandeuxarist
|
0cdb4bcc7c |
fix(api,sync): Delimit positional arguments in git clone mirror invocations (#539)
* fix(api,sync): Delimit positional arguments in git clone mirror invocations Pass "--" before positional path/URL arguments in Command invocations for "git clone --mirror" across repos.rs, sync.rs, and gl mirror.rs. Fixes #374 * style(gl): Format args array in mirror.rs Fix cargo fmt line-length break for git clone --mirror arguments. Refs #376 * fix(node): Pin the git clone delimiter against remote-argument injection The delimiter change had no test, so nothing stopped it being reverted as cosmetic. What it guards is real: git parses a remote beginning with a dash as an option, and --upload-pack=<cmd> is one git hands to a shell. No path reaches that today. Peer URLs are gated by is_public_http_url, so the origin clone_repo composes can only begin with http:// or https://; fork paths are joins under repos_dir; and clap rejects a leading-dash positional in gl mirror. The delimiter is what removes git's dependence on those three unrelated gates staying correct. Assert git reports the whole argument as a repository it cannot find, which is only true once the delimiter forces it to be a path. Undelimited, git consumes it as an option and fails against the destination instead, never naming the injected string, so the assertion is red before the fix and green after. Refs #374 * fix(sync,gl): Delimit positional arguments in fetch_repo and gl clone Carry the positional argument delimiter into fetch_repo when setting the remote origin URL and into setup_partial_clone across all clone branches. Correct the error string quoted in the clone regression test docstring. Refs #374 * test(gl,node): Pin the argv delimiter on every git sink this PR changed The delimiter contract is duplicated across five direct `git` argv constructions, but only `sync::clone_repo` had a test that went red when `--` was deleted. Removing or misplacing the delimiter in any other sink left the suite green, so the hardening rested on review rather than on the tests. Each changed sink now has a subprocess-facing regression driven through real git: the three `setup_partial_clone` shapes (plain, `--branch`, and the sparse/promisor arm), `fetch_repo`'s `remote set-url`, `gl mirror`, and the fork clone. Two of these could not carry the obvious assertion: - `git_global` and `git_run` both format failures as `git {args:?} failed: {stderr}`, so the injected value is already in the message via the debug-printed argv. Asserting that the error merely contains it passes with `--` deleted. The clone tests assert on git's own single-quoted `repository '<value>'` instead, which only appears when git read the value as a repository. - `remote set-url` does not fail on a delimited option-shaped value, it stores it. The `fetch_repo` test asserts the stored URL rather than an error: undelimited, git exits 129 with `unknown option` and leaves the previous URL in place. `gl mirror` and `fork_repo` needed their argv extracted to be testable. `mirror::run` loads a keypair and contacts a node before reaching the clone, and bails with a message that interpolates the source whether or not the delimiter is present. The extraction keeps the production `.status()` call, so a large mirror clone still streams git's progress to the terminal instead of being captured. Every one of the five was confirmed red with its delimiter removed and green with it restored. Refs #374 * fix(gl): Route the last three clone warnings through emit_warning `emit_warning` sanitizes a line before writing it, which strips ANSI escape and bidi characters from untrusted text. Three warning sites in the blob-recovery paths still used a bare `eprintln!` while interpolating `oid`, which comes from node JSON or an Arweave gateway manifest, so those escapes reached the user's terminal raw. Refs #374 * style(gl): elide the lifetime in mirror_clone_args * test(gl,node): pin LC_ALL=C on git-subprocess stderr assertions --------- Co-authored-by: euxarist <[email protected]> |
||
|
|
0e5a1608e2 |
feat(node): persist upstream mirror configuration and transition state (#538)
* feat(node): persist upstream mirror transition state Origin-Session: local-d6a143 | Codex | 12 prompts * fix(node): reject null mirror transition state Origin-Session: local-d6a143 | Codex | 14 prompts * fix(node): address mirror state review findings Origin-Session: local-d6a143 | Codex | 16 prompts * fix(node): close mirror review blockers Origin-Session: local-d6a143 | Codex | 20 prompts * fix(node): align peer URL validation message Origin-Session: local-d6a143 | Codex | 20 prompts * fix(node): close remaining mirror review findings Origin-Session: local-d6a143 | Codex | 21 prompts * test(node): track the Db pool field change in the mirror-state migration tests * fix(node): revalidate stored webhook URLs at delivery and cover the subdomain repeated-dot case * docs(node): document the invalid_url webhook delivery outcome --------- Co-authored-by: KRATOS <[email protected]> Co-authored-by: Kevin Codex <[email protected]> |
||
|
beardthelionandGravirei
|
37947208c5 |
docs: state that GITLAWB_ENFORCE_OWNER_PUSH defaults to true (#537)
* docs: state that GITLAWB_ENFORCE_OWNER_PUSH defaults to true PR #330 flipped the code default to true and updated .env.example, the README env-var table, the AWS guide, the macOS app, and docs/RUN-A-NODE.md, but two prose mentions in operator-facing docs still describe the opposite. SECURITY.md line 57 ("Repository write authorization defaults" section) and README.md line 69 (Known limitations) both say the flag "defaults to `false`", which contradicts the live runtime and the rest of the same files. Reviewer 2 closed #224 with this as the P2 finding to fix in the config-and-policy split. The correct text already exists in the README env-var table; the prose restatements were missed. Bring the prose into line with the code. - SECURITY.md: rewrite the section to lead with the default being true and describe `false` as the explicit rolling-upgrade override. - README.md: rewrite the known-limitations bullet to state the default is true and the `false` opt-out is a rolling-upgrade only. - Add `crates/gitlawb-node/tests/owner_push_default_docs.rs`: a regression guard that reads the four surfaces (clap default, .env.example, README known-limitations prose, README env-var table, SECURITY.md) and asserts each one says the default is `true`. Reverting either doc edit turns one named test red; flipping `default_value_t` back to `false` turns the env example test red. Each surface is checked independently so a stale doc lies alone. No code change. No new migration. No dependency change. Compiles clean, clippy clean under `-D warnings`. Refs: closes the P2 finding from the review of #224. * fix(docs): clarify default value consistency for GITLAWB_ENFORCE_OWNER_PUSH * docs(readme): drop the default-open roadmap line and the em-dash punctuation --------- Co-authored-by: Gravirei <[email protected]> |
||
|
beardthelionandAyush7614
|
8c9e4adc2d |
fix(node): canonicalize and 404-on-no-match in remove_label (#344) (#530)
* fix(node): canonicalize and 404-on-no-match in remove_label (#344) add_label stored labels trimmed and lowercased, but remove_label passed the path segment through as-is, so a label stored as 'bug' did not match DELETE /repos/{owner}/{repo}/labels/Bug. Worse, remove_label's DB call discarded the row count and the handler always returned 200 with {removed: true}, including when the delete matched nothing — the no-op-rendered-as-success shape the rest of the API rejects. Move trim+lowercase and charset/length validation into a shared canonicalize_label helper used by both handlers, and have db::remove_label return the affected row count so the handler can return 404 when no label was deleted. add_label's behavior is unchanged (idempotent semantics preserved); the only externally observable change is that mixed-case and surrounding-whitespace path segments now match the stored label, and a delete that matches no row returns 404 instead of {removed: true}. Tests: * remove_label_canonicalizes_and_404s_on_no_match exercises the mixed-case path, the trimmed path, the empty-after-trim rejection, the disallowed-charset rejection, and the no-op-returns-404 case. * remove_label_denies_non_owner re-pins the 403 owner-gated shape (matches AGENTS.md: owner-gated mutations return 403 after the repo lookup, distinct from the not-found shape). * test(node): pin len>50 arm in remove_label canonicalize The helper's length branch had no coverage — 51 chars must 400 and 50 chars must pass validation (404 when absent). This pins the arm lifted into canonicalize_label and decouples the no-match 404 from the double-delete above, as requested in review of #390. --------- Co-authored-by: Ayush7614 <[email protected]> |
||
|
beardthelion
|
8eedb935c3 |
fix(node): gate bounty mutations on repo read before status checks (#529)
* fix(node): gate id-keyed bounty mutations on repo read before status checks
submit, approve, cancel, and dispute on /api/v1/bounties/{id}/* looked the
bounty up and ran status and participant checks without gating on the
bounty's repo read visibility. A signed stranger holding an id got a
distinguishable 400/403 on a private-repo bounty where an absent id gets
404, so the routes were an existence and lifecycle oracle.
Insert the same authorize_repo_read block get_bounty and claim_bounty
already carry, right after the lookup, so a denied caller sees the
not-found shape either way. Participant checks are unchanged; on a public
repo a stranger still reaches them.
Closes #341
* Address PR review feedback (#448)
- assert the exact not_found body shape for both held and absent bounty
ids in the mutation-gate helper, so a denial carrying lifecycle or
participant detail fails the test
|
||
|
beardthelion
|
47c6e6ce22 |
fix(node): fail pin_git_object when the add response carries no Hash (#528)
* fix(node): fail pin_git_object when the add response carries no Hash A 200 from /api/v0/add without a parseable Hash fell back to the caller-computed raw CID, recording an address the backend never confirmed stored. For payloads above the backend's chunk threshold (262144 bytes on Kubo) that raw CID is not even a stored block, so every later /api/v0/cat on it fails permanently: get_encrypted_blob errors on every fetch and mirrors retry the envelope on every sync. A Hash-less 200 is now an add error carrying a truncated body excerpt. The delaying_endpoint test fixture, which relied on the fallback by returning an empty 200 body, now answers with a conformant NDJSON line. * fix(node): reject a non-CID Hash in the add response Address PR review feedback (#453): a string-valued Hash that does not parse as a CID would be recorded into encrypted_blobs.cid and fail on every later cat, the same failure class as a missing Hash. Validate the returned Hash before accepting it and cover the malformed case with a regression test. * test(node): return real CIDs from the add-response test mocks The pin_git_object CID validation rejects non-CID Hash strings, and a dozen test_support fixtures answered 200 with placeholder values like "bafyprovider" or "x". Replace them with a valid CID so the fixtures model a healthy backend; no test asserts on the literal value. * fix(node): bound the invalid Hash echoed in the add error |
||
|
beardthelionandMystic
|
317a1550c2 |
fix(node): scope bounty aggregates to anonymously-listable repos (#477) (#527)
* fix(#477): scope bounty aggregates to anonymously-listable repos bounty_stats and agent_bounty_stats ran unfiltered aggregates over all bounties, leaking private-repo bounty activity to anonymous callers. Mirror the stats() pattern from server.rs (#104): 1. Batch-load all deduped repos + visibility rules (2 SQL round-trips) 2. Filter to listable_at_root(rules, is_public, owner_did, None) 3. Pass visible (owner, name) pairs into SQL aggregates via EXISTS/unnest with owner key normalization on both sides (BOUNTY_OWNER_CASE_SQL). 4. Normalize claimant DID in agent_bounty_stats_visible and group by normalized claimant in bounty_leaderboard_visible (BOUNTY_CLAIMANT_CASE_SQL). 5. Remove orphaned unfiltered aggregate methods to pass dead_code lints. 6. In tests, seed completed bounty claimant state via the real claim/approve path, and verify root-rule-decided visibility overrides. Aggregates are intentionally anonymous-scoped for all callers (matching the #104 stats pattern). Fail-closed: DB errors collapse the visible set to empty, so all counts return 0 — an under-count never leaks existence. Fixes #477 * style(node): apply rustfmt to the bounty visibility change * test(node): pin bounty aggregate visibility for mirror rows * docs(node): record why mirror rows are handled in the bounty visibility filter * perf(node): keep the claimant index usable in per-agent bounty stats * fix(node): return DB errors from bounty stats instead of zero counts --------- Co-authored-by: Mystic <[email protected]> |
||
|
JATMNandClaude Opus 5.5
|
30637193a6 |
fix(node): resolve issue ids from git refnames, not request input (#526)
* fix(node): resolve issue ids from git refnames, not request input resolve_issue_id answered an exact match by echoing the request-path id back to callers, so get_issue and close_issue spliced caller input into `git cat-file blob refs/gitlawb/issues/<id>`. #521 restricted the input to [A-Za-z0-9-], but the value handed to git was still the request string (CodeQL rust/command-line-injection #83). Resolve exact and prefix lookups through one for-each-ref listing and return the refname git reports. An exact refname still wins over longer ids that share it as a prefix; unique-prefix, ambiguous, not-found, and revision-syntax behavior is unchanged. Co-Authored-By: Claude Opus 5.5 <[email protected]> * fix(node): keep a dangling exact issue ref from winning over longer ids for-each-ref lists a ref even when its blob is missing, so the exact-id precedence added in the previous commit picked a dangling exact ref over a readable longer sibling: resolve_issue_id returned the dangling id where main reported an ambiguous prefix, get_issue then returned None, and close_issue hit its expect(). Let the exact id win only when `git cat-file -e` finds its object, run on git's own refname so no request input returns to the argv. Otherwise fall through to the unchanged prefix rules, matching main for every layout. Co-Authored-By: Claude Opus 5.5 <[email protected]> --------- Co-authored-by: Claude Opus 5.5 <[email protected]> |
||
|
beardthelionandGravirei
|
2c341f71ac |
fix(node): ANS-104 codec and recovery policy types (#26 split 2/4) (#524)
* fix(node): ANS-104 transport, three-outcome probe, verify endpoint (#26 split 2/4) Reviewer 2 closed PR #224 on 2026-08-28 with a directive: split into four narrow PRs. This is Split PR 2 (Arweave transport and verification). The P1 finding the reviewer assigned to this split: the legacy `anchor_item_present` mapped `BAD_REQUEST` and `GONE` to `Ok(false)`, and the recovery code interpreted false as permission to reclaim the row and pay for another upload. Neither 400 nor 410 proves a previously paid item was never accepted: 410 can describe an artifact that existed but is no longer served, and 400 can be produced by gateway, proxy, or routing failure. The concrete failure sequence the reviewer named: bundler accepts the item, the response or terminal DB write is lost, recovery probes the persisted item_id, the gateway returns 400 or 410, and the node pays for a second immutable item for the same transition. Fix: model the probe result as three outcomes, and authorize a re-upload only on a trustworthy, protocol-defined absence. NEW MODULE crates/gitlawb-node/src/ans104.rs - DataItem: the on-wire ANS-104 shape (signature, owner, target, anchor, tags, data — all base64url-encoded except target/anchor). - deep_hash: the canonical signing input per the spec. The signature is over a SHA-256 mix of the dataitem/list/map discriminators, the signature type, owner, target, anchor, the recursively-hashed tag list, and the data payload. A hand-rolled sha256 of the JSON body would produce a hash no Arweave gateway would recognize; the deep-hash is the only correct form. - sign_data_item: Ed25519 over deep_hash. - verify_data_item: parses the owner, checks the signature against an expected pubkey, and reports a specific failure reason on each branch. - 7 unit tests covering round-trip, flipped signature, wrong expected key, mutated data, wire-shape round-trip, deep-hash stability, and the empty-tags edge case. NEW MODULE crates/gitlawb-node/src/arweave_v2.rs - ProbeOutcome: Present (2xx, body parses as ANS-104, sig verifies), DefinitivelyAbsent (404 with a known JSON body shape), Indeterminate (everything else). The reviewer's three-outcome model. - ProbeOutcome::permits_reupload: the recovery policy. ONLY DefinitivelyAbsent authorizes a paid re-upload. Indeterminate keeps the outbox non-terminal; Present skips re-payment. - probe_anchor_item: the gateway probe. 2xx bound to a different item id is Indeterminate, not Present. 2xx with a body that does not parse as ANS-104 is Indeterminate. Oversized bodies are Indeterminate (defense against body-stuffing). - read_capped_body: enforces PROBE_MAX_BODY_BYTES (1 MiB) on the full body, with a Content-Length fast-path that rejects before reading. - verify_anchor: the full path the public verify endpoint takes. Fetches the data item, parses it, verifies the Ed25519 signature against the persisted node_did, decodes the data payload as JSON, and reports the result. - 15 unit tests pinning each classification boundary: 400/410/5xx/transport/2xx-non-json/2xx-bad-sig/2xx-different- owner are all Indeterminate; 404 with empty or known-JSON body is DefinitivelyAbsent; 2xx with a valid signed item is Present; the re-upload policy is exhaustive. NEW DB METHOD crates/gitlawb-node/src/db/mod.rs - get_arweave_anchor_by_item_id: looks up an existing arweave_anchors row by its id (which is the Irys tx id, the same value the Arweave gateway uses to serve the data item). Used by the public verify endpoint to fetch the persisted node_did so the envelope signature can be verified against it. NEW CONFIG crates/gitlawb-node/src/config.rs - GITLAWB_ARWEAVE_GATEWAY_URL: the Arweave gateway the probe reads from. Defaults to https://arweave.net; operators can point it at a private mirror. NEW HTTP ENDPOINT crates/gitlawb-node/src/api/arweave.rs - GET /api/v1/arweave/anchors/verify/{item_id}: the public verify endpoint. Returns {item_id, status, verified, owner_did, data_payload, error} where status is one of "verified" / "definitively_absent" / "indeterminate". - 3 endpoint-level tests covering the verified, 404, and 400 shapes via the live router, asserting the public surface surfaces the three outcomes. WIRE-UP crates/gitlawb-node/src/server.rs - The verify route is registered alongside the existing /api/v1/arweave/anchors list route. NOT IN THIS SLICE (the bundler upload, the next commit): - The handler that builds a DataItem from a ref-cert and POSTs it to the bundler is not in this PR. The ANS-104 module is ready to be called by it; the cert/push-event flow in PR 1 persists the row that the bundler upload would consume. - PR 1's recovery drain produces an `anchor_jobs` row keyed on the (repo, ref, old, new) tuple; the next slice reads that row, builds the ANS-104 data item, POSTs to the bundler, and writes the resulting tx id to arweave_anchors. Compiles clean, 1113 tests pass with 0 regressions, clippy clean under -D warnings, fmt clean. Cross-PR overlap (declared in the PR description): - #134 (anchors auth): composes. The verify endpoint reads from arweave_anchors; #134's auth layer applies unchanged. The probe model is server-side and unauthenticated by design. - #285 (advisory-lock session affinity): independent. The probe is read-only. - #306 (Content-Digest on signed requests): independent. The probe does not sign requests. - #314 (small-order Ed25519): composes. The verify path calls Did::to_verifying_key which already enforces the small-order check from #314. - #324 (libp2p keypair persistence): independent. - #325 (gossip ref-update auth): independent. The verify path is for the public Arweave gateway, not gossip. - #382 (replication withheld-subtree trees): independent. * fix(node): address reviewer findings on #26 split 2/4 (transport) - P1 (DB lookup): get_arweave_anchor_by_item_id now filters on irys_tx_id (the externally-routable item id) rather than the internal UUID primary key. The production writer stores a fresh UUID in `id` and the Irys response / ANS-104 derived id in `irys_tx_id`; the old filter would always 404. New integration tests exercise the production writer path with distinct values for the two columns. - P1 (verify-gate): verify_anchor now gates on repo read via authorize_repo_read. The route is split in server.rs: the public list_anchors stays ungated (#134 tracks that surface), the verify route gets optional_signature per the team memory `axum-layer-vs-merge-pitfall.md`. Denials on row-lookup, gate, and missing repo all collapse to the same opaque 404 so the public endpoint does not leak anchor-row existence. - P1 (ANS-104 deep-hash): rewritten to the verified SHA-384 recursive list/blob algorithm (matches `arbundles` JS reference). deep_hash returns [u8; 48]; signature is over the raw 48-byte digest. Three reference vectors from `Irys-xyz/arbundles/src/__tests__/deepHash.spec.ts` are bit-exact-asserted as the interop canary per the team memory `self-roundtrip-tests-do-not-prove-interop.md`. - P1 (artifact-identity): v2 verify now derives the protocol id via DataItem::id() (base64url(SHA256(signature))) and requires equality with the requested URL item_id. A node key signs many data items, so a valid signature only proves who signed the response — not that the served item is the one the caller asked to verify. The team memory `verify-against-artifact-id-not-signer.md` is the policy. - P1 (verifier can't read this head's writes): verify_anchor now accepts BOTH the v2 ANS-104 format and the v1 raw-JSON shape the live path on this branch actually writes. The v1 path does a field-equality check on repo, ref_name, old_sha, new_sha, node_did against the persisted row (no signature in v1; Irys storage plus the JSON parse are the integrity guarantee). - P2 (ProbeOutcome in result): AnchorVerifyResult now carries outcome: ProbeOutcome. The HTTP handler maps the structured field to the status string instead of parsing the human- readable error message — any future wording change in arweave_v2.rs can no longer silently reclassify a DefinitivelyAbsent as Indeterminate. - P2 (empty 404 body): classify_404 returns Indeterminate for bodyless 404s. The team memory `distinguish-unknown-from-empty.md` is the policy: a bodyless 404 from a proxy is not a proof of absence, and the recovery policy (permits_reupload -> true on DefinitivelyAbsent) authorizes a paid, irreversible re-upload. - P2 (streaming cap): read_capped_body no longer uses resp.bytes() (which buffers the full response before checking size). It now reads chunk-by-chunk and aborts the moment the cumulative byte count crosses the cap, so a chunked response without Content-Length cannot force unbounded allocation. * fix(node): address three reviewer findings on #26 split 2/4 (round 2) * fix(node): address five reviewer findings on #26 split 2/4 (round 3) * fix(node): address reviewers round-4 on #26 split 2/4 (interop) * fix(node): address coderabbit on #26 split 2/4 (raw tags, vint overflow, golden key) * fix(node): enforce artifact-id in probe_item on #26 split 2/4 * fix(node): require expected owner for probe Present on #26 split 2/4 * fix(node): narrow #26 split 2/4 to codec and policy types * fix(node): silence dead-code for deferred slice API on #26 split 2/4 * fix(node): doc and bounds-check cleanup in ans104 on #26 split 2/4 * docs(node): point tests doc at Ed25519 golden script on #26 split 2/4 * fix(node): strict owner lengths, sign-before-hash on #26 split 2/4 * fix(node): keep ans104 tags and field widths consistent on hash and encode DataItem keeps both the decoded `tags` and the original Avro `raw_tags`, both public. After a parse, changing `tags` left deep_hash and verify_data_item using the stale raw bytes, and to_binary wrote a header tag count that no longer matched the payload, so the frame failed to parse back. Take the tag block for hashing and encoding from one helper that decodes `raw_tags` and requires it to equal `tags`, and fail closed otherwise. The header count comes from the same validated pairs. Parsed multi-block and size-prefixed items still re-encode byte-exact and keep their signatures. deep_hash now also rejects a non-empty target or anchor that is not 32 bytes, as to_binary already does, so sign_data_item fails instead of signing an item that cannot be encoded. The legacy non-base64url anchor fallback is kept. The module and signature-type docs now say binary parsing accepts sigtypes 1 to 4 while signing and verifying are Ed25519 only on this slice. * build(scripts): declare the golden script's direct npm dependencies ans104_golden_ed25519.mjs imports bs58, @noble/ed25519 and base64url, and ans104_golden.mjs imports base64url, but package.json listed only arbundles, so they resolved only through hoisting. Declare them at the versions already locked, update the lockfile root entry (no resolved version or hash changes), and note the regeneration command in the golden output file. --------- Co-authored-by: Gravirei <[email protected]> |
||
|
beardthelion
|
911be4d9fe |
fix(node): validate ref-update SHAs are hex, not just length 40 (#433)
* fix(node): validate ref-update SHAs are hex, not just length 40 parse_ref_updates accepted any 40-byte string as an object id, so a peer-supplied pkt-line with non-hex content was forwarded into RefUpdateEvents on gossip, branch_cid rows, and sync-notify JSON as a canonical-looking identifier. Require [0-9a-fA-F] on both fields. Closes #398 * chore: refresh PR head |
||
|
beardthelion
|
0366f18458 |
fix(node): reconcile public-read messaging with per-repo enforcement (#439)
* fix(node): reconcile public-read messaging with per-repo enforcement GITLAWB_PUBLIC_READ is read nowhere except a startup warning, while read enforcement actually happens per repository through is_public and path-scoped visibility rules. The flag's help text, the startup warning, .env.example, and the readiness audit all still claimed private-read enforcement was unwired. The help and warning now state the flag is reserved and inert, and point at the real per-repo control. A config test asserts the rendered help neither claims enforcement is missing nor omits that the flag does nothing. Closes #338. * docs: drop the stale private-read priority entry * chore: refresh PR head |
||
|
beardthelion
|
131bb95c68 |
fix(node): scope the refs listing by the canonical repo slug (#388)
* fix(node): scope the refs listing by the canonical repo slug `list_refs` built its `branch_cids` filter from the raw URL path segments while the only production writer stores the canonical `normalize_owner_key(owner_did)/name` slug. `get_repo` resolves both `did:key:zX` and bare `zX` to the same record, so a request in the full DID form passed the read gate and then matched zero rows, returning 200 with an empty ref list. A denial shaped like a success is worse than an error: a caller cannot tell an empty repository from one it queried under a form the table does not carry. Bind the record `authorize_repo_read` already returns and build the filter from it, so the queried resource is the one that was authorized. This is the existing pattern in `api/events.rs` and in this file's own push path. Two regressions, each proven to fail without the line it guards. The first drives both owner-DID forms through `build_router` and asserts they return the same refs, with a second repo whose refs must not appear; widening the query to match any owner turns that half red. The second covers the deny direction, which the endpoint had never had: a private repo answers an anonymous caller with the same 404 a missing repo gets and puts no CID in the body. Without it, removing the read gate outright left the suite green. * chore: refresh PR head |
||
|
beardthelion
|
0303222eae |
fix(node): gate replica register/unregister on repo read visibility (#445)
* fix(node): gate replica register/unregister on repo read visibility list_replicas already applied authorize_repo_read, but the two mutations looked the repo up directly: a non-reader of a private repo could register or remove their own replica and receive the replica count in the response. Route both handlers through authorize_repo_read so a denied caller gets the same 404 as a missing repo and nothing is written. Self-registration and self-removal are unchanged for authorized readers. Fixes #435 * test(node): assert denial bodies leak no replica or repo data CodeRabbit nitpick: the new replica-mutation denial tests only checked the 404 status. The path instructions for this file require non-leaking body assertions on denied responses, matching the sibling list_replicas_is_read_visibility_gated test. Assert the denied PUT and DELETE bodies contain neither the submitted replica URL nor the repo id, in both the handler-level test and the real-signature e2e. * chore: refresh PR head |
||
|
beardthelion
|
c97695a259 |
Add a completeness fence over the GraphQL mutation surface (#219)
* test(node): add a completeness fence over the GraphQL mutation surface Gate the GraphQL mutation surface the way REST is gated, as a SEPARATE guard with its own bucket table (a guard over a heterogeneous set must classify per member, INV-1). every_graphql_mutation_has_its_gate asserts the mutation set is complete (every mutation classified) and that each carries its gate marker. Completeness is derived from async-graphql's own introspection: the field names come from the engine as structured data, so no source-text or SDL-format shape (a brace or async fn in a comment or string, a whitespace/generic/skip variant, a brace in a rustdoc description) can hide a live mutation. A new unregistered mutation, including a repo-write, fails the build. The marker check (does each resolver body call its gate) is a best-effort static backstop, bounded to each resolver's own comment-masked, string-aware body. It is explicitly not the completeness guarantee and not load-bearingness: a marker inside a string literal is a documented vacuity residual, and the adversarial sqlx tests in graphql/mutation.rs are the real proof the gates reject. Verified by execution: an unregistered mutation reddens (unclassified), a removed did_matches gate reddens (missing marker), the clean tree is green; no DB, no feature flag. * docs(node): reconcile GraphQL mutation-auth claims with the shipped state README's known limitations, the OSS-readiness live-network blockers, and the maintainer roadmap still listed GraphQL mutation auth as an open item, which contradicts #87: GraphQL mutations already require a verified signer bound to the acting DID. Reconcile each to the shipped reality while keeping the genuinely-open nuance: the GraphQL mutation surface is the agent-task queue, repo writes stay REST-only, and no owner-gated repo-write mutation is exposed over GraphQL. The every_graphql_mutation_has_its_gate fence forces any new mutation to be classified in its table; owner-gate and REST parity stay the INV-1 review check. * fix(node): source the GraphQL mutation fence from SDL, not introspection Resolves jatmn's #219 P1. The every_graphql_mutation_has_its_gate completeness fence enumerated mutation fields via async-graphql introspection, which filters out `#[graphql(visible = false)]` fields. Those fields stay executable, so a future hidden mutation would need no registry row and no marker while both set-equality checks stayed green -- defeating the fence's guarantee that no live mutation goes unclassified. (Confirmed by execution: a synthetic visible=false mutation is absent from introspection, executable, and present in schema.sdl().) Source completeness from the engine's own SDL instead. A new pure helper mutation_field_names_from_sdl parses schema.sdl() with async-graphql's parser AST (async_graphql::parser::parse_schema) and reads the mutation type's fields; SDL includes visible=false fields and the AST parse is immune to a brace in a field description. Introspection is kept as a cross-check: any field present in SDL but hidden from introspection now fails named as a forbidden hidden mutation. The stale doc comments (which claimed introspection could not hide a field) are corrected. A self-checking version-safety test builds a synthetic schema with a visible=false mutation and asserts the SDL-sourced set includes it while introspection omits it -- so if a future async-graphql ever filters SDL by visibility, that test goes RED rather than the gap reopening silently. Load-bearing verified by execution: stubbing the SDL enumeration to empty turns four tests RED, including the fence itself; restoring returns green. Only api/mod.rs changed; the mutation resolvers are untouched. `cargo test -p gitlawb-node --bins` green (495); clippy and fmt clean. * fix(node): make the SDL mutation-fence wiring load-bearing; admit deprecated An adversarial review of the SDL switch found three gaps: - The fence's completeness source (SDL) and its hidden-mutation cross-check were not load-bearing: because the real MutationRoot has no visible=false field, reverting `fields` back to introspection and dropping the cross-check left the whole suite green (INV-21). Extract the sourcing + cross-check into a generic `assert_mutation_fence<Q,M,S>` that the real fence AND a synthetic hidden schema both drive. A new #[should_panic] test (fence_rejects_a_hidden_mutation) feeds a visible=false mutation that IS registered, so only the cross-check can fire; reverting the SDL source or deleting the cross-check now changes/removes that panic and turns the test RED (verified). - The introspection cross-check used bare `fields { name }` (GraphQL default includeDeprecated:false), so a `#[graphql(deprecation = "...")]` mutation -- executable and present in SDL -- would false-red as a hidden visible=false mutation. Use `fields(includeDeprecated: true)`; a new test (fence_admits_a_deprecated_mutation) pins it, going RED without the flag. - schema_lists_exactly_the_registered_mutations still sourced from introspection and its comment called introspection "the source of truth", contradicting the fix. Retargeted at the SDL set with a corrected comment. Also folds the two introspection navigations into one generic helper (introspection_mutation_fields) and points the version-safety test at it, so there is a single introspection query. cargo test -p gitlawb-node --bins green (497); clippy and fmt clean. Only api/mod.rs changed. * fix(node): rebase #219 and require signer gate in the GraphQL fence Rebase onto current main and skip the obsolete 0.5.1 lockfile commit. The mutation fence now requires both require_signer(ctx)? and did_matches( in every registered resolver, and each_mutation_requires_and_binds_signer covers all four task mutations for unsigned and DID-mismatch denial. * docs(node): reconcile GraphQL fence docs with SDL discovery Restore the historical OSS audit blocker text and add a dated status update for #219. README and the maintainer roadmap now describe the SDL-sourced fence and signer-plus-binding markers. * ci: trigger the PR checks on a new head |
||
|
beardthelion
|
fc87586411 |
fix(node): hold fork to the same repo-name rules as every other creation route (#412)
* fix(node): hold fork to the same repo-name rules as every other creation route
fork_repo built its clone destination with store::repo_disk_path, a raw
join with no validation, while create_repo and the sync mirror path both
go through validated_repo_disk_path. Its own name check is
`.chars().all(|c| c.is_alphanumeric() || c == '-' || c == '_')`, which is
Unicode-aware and has no length or leading-character rule, so it admits
four shapes validate_repo_name refuses: the empty string (all() is
vacuously true on it), a non-ASCII alphanumeric such as "cafe" with an
acute e, a leading '-', and a name over the 100-char bound.
The empty name was the reachable defect. Driven through the production
router with a real Ed25519 signature, a `{"name":""}` fork returned 201
Created with a repo row whose name is "" and a clone_url ending in
`/.git`, materialized at `<repos_dir>/<owner_slug>/.git`. It now returns
400 and creates neither the row nor the directory.
This is a deliberate narrowing, not only an empty-name fix: the other
three shapes above are now 400 on fork where they previously reached the
raw join. None of them could ever have been created through create_repo,
which fails at repo_store.init on the same validator, and no existing row
can carry such a name (create_repo validates before its DB insert, and
mirror rows go through validate_repo_slug), so no repo becomes
unforkable. A fork that defaults to source.name is unaffected.
The traversal shape of the same join is NOT reachable and is not what
this changes: forker_did comes from AuthenticatedDid, which auth/mod.rs
inserts only after Did::to_verifying_key resolves a multibase-decodable
did:key, so it can carry neither ".." nor "/".
repo_disk_path is now cfg(test). It has no production caller left, and
the attribute turns the next one into a compile error rather than a
silent bypass. Tests keep it because a fixture that must escape repos_dir
cannot be built with the validated form.
Two tests. A unit test on the mounted handler, and an end-to-end test
through server::build_router with real signatures that covers all four
refused shapes plus a positive control asserting an ordinary name still
forks to disk and to a row. Both are mutation-verified in both
directions: reverting to the raw join reddens the refusals, and forcing
fork to always refuse reddens the control.
Closed #272 fixed this class on the sync route and pointed at
repo_disk_path as the sanitizing convention; it is in fact the
unvalidated join, and the fork route was never scoped.
* fix(node): validate the fork name before spending the proof
The barrier landed after `proof.consume` and after `repo_store.acquire`, so a
name the character allowlist admits but `validate_repo_name` refuses burned a
valid iCaptcha proof and paid a source-repo download before its 400. The
handler's own header comment promises the opposite: a fork rejected for a bad
name never burns a proof. Reported by CodeRabbit on the review of
|
||
|
beardthelionandKevin Codex
|
288ad92fe8 |
fix(node): bound the request body the signature middleware buffers before it verifies (#262)
* fix(node): bound the request body the signature middleware buffers before it verifies
`require_signature` buffered the whole request body with an unlimited
`body.collect()` before parsing a single header, and nothing upstream capped it.
On the git write routes the 2 GB `RequestBodyLimitLayer` is applied to the inner
router while `add_auth_layers` wraps it, and a later `.layer()` is the outer
service, so the middleware ran first and saw the raw body. The other signed
groups carry no limit layer at all and lean on axum's 2 MB `DefaultBodyLimit`,
which the extractors enforce through a request extension this middleware never
consults, so that default was bypassed too. `optional_signature` forwards to the
same code whenever a caller merely presents signature headers, so the reach
extended to `/graphql`, `/ipfs/{cid}` and the other optional-signature groups.
Measured against the real router: an unsigned push with `GITLAWB_MAX_PACK_BYTES`
set to 64 KiB drained the full 4 MiB body and only then returned 401. No DID, no
signature, and no repo were required, and in front of it sits only a per-IP
arrivals-per-hour counter, no timeout layer and no connection cap.
The collect now runs through `http_body_util::Limited`, so reading stops at the
limit instead of after the fact, and an over-limit request gets 413 rather than
the 400 reserved for a genuinely unreadable body. The limit is per route group:
`add_auth_layers` takes it and passes it to the middleware as state, the git
groups keep `max_pack_bytes` so large pushes are unaffected, and the rest get a
constant equal to axum's default because that is already what their extractors
enforce. Verification, content-digest, clock-skew and the request reconstruction
are untouched.
The regression test asserts bytes actually polled out of a streaming body, not
the status code, because a status assertion passes whether or not the body was
drained. Neutralizing the limit turns it red; handing every group the git limit
turns the non-git test red; returning 400 instead of 413 turns the status test
red.
This bounds per-request buffering only. Total resident memory is still
O(connections x limit); a read timeout and a connection cap are what close that
and are deliberately not in this change.
* test(node): prove a signed push under the limit lands on disk, and an over-limit one writes nothing
The body-limit guard was covered only by tests that reject. Nothing proved it
still admits what it should, so the fix could have passed every test by refusing
everything, and nothing would have caught a middleware that truncated or
corrupted the body it hands downstream.
These drive a real push: a source repo with a 300 KiB incompressible blob, an
empty bare repo at the path the store resolves, and the byte-for-byte
receive-pack request body captured off the wire from an actual `git push`. The
body is captured rather than generated, because `git send-pack --stateless-rpc`
emits remote-curl's extra RPC framing (a wrapping length plus a stray flush
before PACK) and `git receive-pack --stateless-rpc` rejects it with a pack
signature mismatch.
The positive case sets the cap just above the body and asserts the ref moved and
the tree resolves, not merely that the status was 200. That distinction is
load-bearing: under a truncation mutation the status stays 200, because git
reports the failure inside report-status, and only the on-disk assertion goes
red. Running the whole suite under that mutation leaves exactly one test failing,
so this is the only thing standing between a corrupted body and a green build.
The negative case sets the cap just below the body and asserts 413 with neither
the ref nor the commit present afterwards, so a rejected push cannot half-write.
Its existence check peels to ^{commit}, since rev-parse echoes any well-formed
40-hex string back and would have passed vacuously.
* test(node): give the unresolvable-DID middleware test the signature body limit state
---------
Co-authored-by: Kevin Codex <[email protected]>
|
||
|
beardthelion
|
86c544a577 |
fix(core): reject small-order Ed25519 keys at DID resolution and before X25519 (#314)
* fix(core): reject small-order Ed25519 keys at DID resolution and before X25519 A did:key can encode a small-order (weak) Ed25519 point. VerifyingKey::from_bytes only decompresses, so such a key resolved cleanly, and x25519_public converted it to the all-zero Montgomery u. X25519 against u = 0 is the all-zero shared secret for every scalar, so the per-recipient wrap in seal_blob could be rebuilt with no secret at all. Since one content key is wrapped per recipient, a single weak recipient exposed the blob for every recipient. Reject in Did::to_verifying_key, which is the choke point every consumer that resolves a DID shares, so a weak key becomes unresolvable and the existing fail-closed recipient handling catches it without a new code path. Guard x25519_public as well so the primitive is safe for a caller that obtained the key some other way. The attacker test performs the real attack rather than asserting an absence: with either guard removed it recovers the plaintext and fails printing it. * fix(attest): mirror the small-order key rejection in its own did:key parser gitlawb-attest parses did:key itself in verifying_key_from_did_key and never routes through gitlawb-core's Did. It cannot: gitlawb-core is a dev-dependency here, so the choke-point rejection is structurally unreachable from this crate and has to be mirrored rather than inherited. Signature verification is already strict, so this is defense in depth against a future consumer of this parser that does not verify strictly, not the closing of an exploitable path. One consequence worth naming: verify_rejects_weak_key_signature now observes Error::Did rather than Error::Signature, because the weak signer is refused at resolution before verify_strict is reached. That test is retargeted accordingly. It no longer exercises verify_strict, which remains the second layer for malleability that does not involve a weak public key (a small-order R under an honest key); this fixture cannot construct that case. * test(node): pin that a weak recipient DID falls into the existing fail-closed arm Because a small-order did:key no longer resolves, it lands in the unresolved set and plan_seal returns the #47 SkipUnresolvable arm that already refuses to seal to a partial recipient set. Nothing is sealed for the blob and the existing bounded operator warning fires. No new branch, no new SealPlan variant, no new log site. The must-not-over-reject control asserts an all-legitimate set still seals. * fix(gl): stop telling a did:key that did:key is unsupported did_resolve reported every resolution failure as "only did:key is supported without a resolver". That is right for a method with no local resolver and misleading for a did:key that parsed fine and failed for its own reason, which now includes a small-order key. Split the message on whether the DID is a did:key and surface the underlying error when it is. Extracted as a pure helper so both branches are unit-testable without an MCP client harness. * fix(review): restore verify_strict coverage the parser guard displaced Refusing a small-order key at DID resolution means a weak signer can no longer reach attestation.rs's verify_strict through verify_signature, which left that line with no test at all: downgrading it to the non-strict verify kept the whole attest suite green. Split the signature check into verify_sig_with_key so a test can drive it directly with a weak key. Check order in verify_signature is unchanged; only the seam is new. Also from review: - auth/mod.rs sent a did:key that failed on its own key material away with "only did:key is supported in alpha", the same misleading hint the gl MCP tool was just fixed for. Made it conditional on is_did_key so the two parallel surfaces say the same thing. - The guard comments claimed a small-order point converts to the all-zero Montgomery u. That holds for order 1 and 2 only; order 4 and 8 are annihilated by the scalar clamping instead. is_weak covers the whole torsion set, and mixed-order points are correctly still accepted. - did.rs doc comment now states the new rejection. - encrypt.rs: extracted the repeated weak-key fixture into a test helper and switched to the file's existing Err(anyhow!(..)) rejection idiom. - gl tests derive the did:key discriminator from a real parsed Did instead of hardcoding the bool, so inverting it at the call site now fails. * fix(attest): cap the did:key method-id before decoding, mirroring core The parser here mirrors gitlawb-core's small-order rejection because it cannot call into it (gitlawb-core is a dev-dependency of this crate). It was mirroring only one of the two guards that sit three lines apart in the function being copied: core also caps the method-id at 64 bytes BEFORE the multibase decode, because base58 decoding is quadratic in its input. signer comes off an attacker-supplied attestation and verify_signature reaches this parser after computing the JCS bytes, so an oversized method-id bought a large decode for a short request. An ed25519 did:key method-id is a fixed 48 characters, so the bound is slack rather than a behavior change. The test asserts the length error specifically rather than any error: a multibase error there would mean the cap ran after the decode and bought nothing. * fix(node): surface stored envelopes sealed to a now-unresolvable recipient plan_seal short-circuited on a matching stored tag before resolving the recipient set, so a blob sealed to a small-order did:key before the choke-point guard landed kept its old CID and was silently treated as unchanged. Resolve the set on the match and surface a now-unresolvable reader as SkipUnresolvableStored so the operator sees the exposure. * fix(attest): anchor VerifiedAttestation.signer to the key that verified Registry::verify discarded the VerifyingKey returned by verify_signature and copied the artifact's signer string into the result. The parser is canonical so the strings agree for honest artifacts and a mismatched field already fails verification, but the consumer-facing value now provably comes from the key that verified rather than a raw artifact field. * fix(node): bound the echoed DID in the unresolvable-did error and pin the hint branches The keyid is unauthenticated request input and was reflected into the response message at full length. Bound the echo to 96 chars and extract the two-branch hint selection into a testable helper with both branches pinned. * test(gl): build the weak did:key fixture from its identity-point bytes The hardcoded WEAK_DID_KEY constant trips secret scanners, which cannot tell a public DID from an API key. Deriving the DID from the compressed identity point states why the fixture is weak and matches the house pattern used in the core, attest, and node crates. ed25519-dalek is added as a dev-dependency (it was already a workspace dependency). * fix(core): skip a low-order ephemeral on the envelope open side open_blob built the X25519 box from the header-supplied eph with no small-order check, so a crafted entry forced the all-zero shared secret and unwrapped for any reader. Skip entries whose eph decompresses to a small-order point; honest entries in the same envelope still open. * fix(core): test the exchange result, not the ephemeral's encoding The open-side guard decompressed the ephemeral to Edwards and asked is_small_order, treating a failure to decompress as safe. That reports safe for any low-order input that is not a valid Edwards encoding. Measured across the seven standard low-order encodings: six caught, u = p-1 missed. Its exchange with any reader is still the all-zero shared secret, so an entry built on it unwraps for everyone and the attacker controls the plaintext. Check the result instead: an ephemeral is rejected when the exchange with this reader's own scalar is all-zero. That is the property the guard exists to enforce, and it holds for every low-order input regardless of encoding. The new must-not test needed two attempts. The first passed with the guard, without it, and with the original pre-fix guard alike, because a blanket `use chacha20poly1305::aead::Aead` in the test resolved the wrap's encrypt through a different aead version than open_blob decrypts with, building a ciphertext that could never open for any reason. Every call in it now names its trait. * docs(core): anchor open-side guard comments to exchange-result invariant The seal-side comment still named is_low_order_montgomery and the low-order vector test doc claimed a decode-path filter that does not exist. Both now describe yields_all_zero_shared_secret. * test(core): use generated keys in poisoned-envelope fixtures Replace hard-coded test scalars that CodeQL flags as cryptographic material. The all-zero shared-secret attack does not depend on which scalar builds the attacker box. * test(core): use Default::default() for zero-point fixtures flagged by CodeQL CodeQL flags [0u8; 32] literals that flow to a cryptographic key argument, even in test fixtures where the value is the all-zero Montgomery point (a public low-order test vector, not a secret). Default::default() produces the same bytes without a literal, clearing the alert without changing behavior. * test(core): generate body nonces with OsRng in poisoned-envelope fixtures CodeQL flags hard-coded byte arrays used as nonces, even in test fixtures where the value is a fixed test vector. Generate with OsRng instead, matching the production seal path. * chore: sync Cargo.lock after merging main (ed25519-dalek edge for gl) |
||
|
beardthelionandKevin Codex
|
206e0827ad |
fix(node): bound replay of authenticated gossip ref-update events (#334)
* feat(node): add the freshness check and replay guard, not yet wired Builds the two layers that bound replay of an authenticated ref-update, with nothing calling them yet; the ingest path is wired in the next commit. The freshness check is deliberately two-directional rather than an absolute delta: an abs() window admits an event stamped up to the window ahead and pins its seen-set slot until the clock catches up. Producers were enumerated before settling the unparseable arm; the sole production publish site emits RFC-3339, so an unparseable timestamp is refused rather than admitted. The replay guard keys on SHA-256 of the canonical signing bytes, not the raw wire bytes, because one signature verifies against many encodings and only the canonical form collapses them to a single key. A golden digest is frozen for the pre-version artifact, and the same constant is asserted for its v-injected twin, so the collapse is pinned rather than described. Reservations settle through a drop guard so only a confirmed entry outlives the ingest call, which keeps a transient write failure from permanently burning an event's slot. Replayed and StaleTimestamp are separate outcomes because they diagnose different conditions, a mesh replay against a broken clock or a healing partition, and folding them would be the same observability lie the unsigned shed variant already exists to avoid. * fix(node): bound replay of authenticated gossip ref-update events Wires the freshness check and the replay guard into ingest, immediately after signature verification and above the author debit. The lower placement, just above the writes, stops the duplicate row and the duplicate sync but still lets every replay drain the victim author's budget, which is the harm the defect names: a captured signature replayed 500 times empties the victim's window and their next genuine push is refused. The guard runs on the verified path only. Unsigned event bytes are predictable, so applying it there would let an attacker pre-send a victim's expected event and have the genuine one dropped as a replay, which is a censorship primitive rather than a defense. A replay flood still costs a parse and one Ed25519 verify, because the guard has to sit below verification for the reason above. What it removes is the peer_exists round trip, the victim's author debit, the ref-update row and the sync enqueue. The existing author-budget test signed one event and ingested the same bytes five hundred times, so it had to re-sign per iteration to keep exercising the budget under a shared guard. Giving it a fresh guard per call would have kept it green while gutting the property it exists to prove. Its over-budget probe needed the same treatment, since the replay gate sits above the author gate and would have refused the burst's last bytes before the budget assertion ran. Both saturation tests now take a shared lock so each keeps an exact assertion on a process-wide counter; a lower bound would stay green if the fail-open branch ever double-counted. * fix(review): bound the saturated guard's rescan and the log it writes At capacity the inline sweep ran on every event, so a full O(capacity) retain happened under the lock once per message and reclaimed nothing when nothing had expired. The guard that bounds replay became a CPU amplifier in exactly the state an attacker drives toward. The sweep is now rate limited to once a second; the periodic sweep still reclaims on its own cadence, so only the redundant rescans go away. An unparseable timestamp was echoed into the refusal detail verbatim. That value is attacker-controlled and arbitrary length, so it reached a warn! as both a log-injection and an unbounded-size sink. Only that arm needs sanitizing; the other two ran through the parser first. The capacity rationale cited a count of registered DIDs, which this same file says elsewhere an attacker mints freely through the announce path, so it was not a bound at all. It now cites the bound that is real: reaching saturation costs a hundred thousand durable rows and as many sync enqueues inside one retention horizon, which the database makes loud. ingest_now was read twice per ingest while its own doc comment claimed the two layers share one reading, which is the invariant the retention derivation rests on. Now read once and passed to both. Also: the Unparseable outcome is driven through ingest rather than only as a pure function, the periodic sweep is observable without a live swarm, the restart exposure is written down where the rest of the tradeoffs already are, and the saturation-counter lock covers every test that can reach Saturated rather than the two I first found. * test(node): close the replay guard's remaining proof gaps, and fix the sibling window Seven tests for the six gaps a review found. The reservation's release path was reasoned rather than executed: the drop guard was only ever proven through an early refusal, while the case its own doc comment names, a transient write failure burning the event's slot, was never driven. Both write directions now are. An expired entry at capacity must be replaced in place rather than answer Saturated, confirm on an already-swept entry is pinned as a deliberate no-op, and the single-critical-section shape is now driven concurrently rather than asserted sequentially. Restart behavior was documented but untested; a fresh guard readmits a seen event and the freshness window still bounds it, which is the composition that makes the restart exposure finite. check_created in gitlawb-core used a symmetric abs() window, so a request stamped 299 seconds ahead was accepted and a signer could roughly double a signature's effective validity by stamping forward. It is now two comparisons like the gossip path, 300s late and 60s early, with the error naming the direction so a fast peer and a slow one need different operator action. There was no future-direction test at all; there are now five covering both. Every caller was checked and none depended on the symmetry. Adding a freshness window obliges auditing the siblings, which is how this one surfaced: the repo argued both ways in two files for a week. * fix(core): stop an attacker-supplied created timestamp from overflowing the window check created is parsed as an unrestricted i64 straight from the Signature-Input header, so the sender picks it. At i64::MIN the past-side subtraction overflows, which panics a debug build and wraps a release one into a value that can read as inside the window. Confirmed by execution before the fix: 'attempt to subtract with overflow'. Saturating subtraction answers correctly at both ends, since a timestamp that far out is refused by whichever side it saturates toward. A test drives all four extremes of the type and asserts each is refused by a named direction; reverting to plain subtraction reddens it on the overflow. The symmetric abs() form this replaced had the same hazard, so splitting the window into two comparisons did not introduce it, but it did not remove it either. Found by a cross-family review pass after six same-family reviewers and I had all read the line. * fix(review): reconcile the replay guard with the unsigned-admission split Integration only, from rebasing this branch onto #325's current head. #325 now returns IngestOutcome::UnsignedAdmitted where it used to return Accepted for an unsigned rolling-upgrade admission, and this branch predates that split. Three sites. The seen-set bypass test asserted Accepted on both unsigned deliveries and on the stale-timestamp case; both now expect UnsignedAdmitted, and the properties under test are unchanged, that unsigned bytes are admitted twice rather than deduplicated and that the freshness window does not reach them. The warn-only-on-admission test's budget-spent case gained the ReplayGuard argument the signature now takes, with a fresh guard because that case drives an unsigned event the replay block skips. The ingest match also grows an arm rather than changing one: `None if unsigned` returns UnsignedAdmitted without settling a reservation, since the replay block is gated on `verified` and an unsigned admission never holds one. * fix(node): render rejected gossip timestamps bounded before logging DateTime::parse_from_rfc3339 accepts arbitrarily many fractional-second digits, so a signed timestamp can parse successfully and still carry megabytes of attacker-chosen wire bytes into the TooOld / TooFarFuture refusal details, which the swarm loop logs verbatim. Only the parser-error branch sanitized. The root cause was treating successful parsing as proof the wire representation is safe to expose. The TooOld and TooFarFuture variants now carry the PARSED instant and the detail renders its canonical to_rfc3339() form - bounded by this build's formatter and control-free by construction - while the unparseable arm keeps sanitize_for_log on the wire string. All three arms go through one freshness_refusal_detail helper so the invariant has a single home, plus a regression test driving an overlong but parseable fractional stamp through both directions. * test(node): pass the replay guard to the post-fork shed tests The two ingest tests added on the target branch after this PR forked (an_over_budget_author_is_shed_without_a_peer_lookup and the_early_author_check_does_not_track_an_unseen_did) still call ingest_ref_update with the pre-guard signature. Thread a fresh ReplayGuard through each call, matching every other test site. * test(node): drive the live-mesh publish and refusal path end to end Two real swarms on QUIC loopback with real signatures and real Postgres: a signed ref-update from one node is admitted and stored by the other, a re-delivery of the same signed content under a malleated wire encoding is refused by the replay guard, and an unsigned event is refused under require_signed. Both refusals are asserted in the warn stream, and a control event landing after them proves the mesh still delivers. This closes the seams ref_update_publish_args names as needing a live swarm: the PublishRefUpdate select! arm dispatching to sign+publish, and require_signed reaching the receive path. It needs a test-only command variant, PublishRawRefUpdate, since the production publish path signs and serializes itself and can never put a malleated or unsigned event on the wire. Still uncovered and named in the test's doc comment: the run_ingest_sweep select! arm on its 300-second interval, and the main.rs half of the flag wiring above p2p::start. * fix(node): harden the gossip rejection path against unauthenticated bytes Two findings on the pre-auth reject path: - An unsigned event under require_signed was resolved and slug-validated before the flag refused it, so a hostile flood bought DID resolution and a rejection reason built from attacker-chosen fields under only the loose pre-parse brake. The unsigned refusal now runs immediately after the version gate, before any field is read. - The swarm loop logged IngestOutcome::Rejected and WriteFailed reasons verbatim, and several producers embed wire fields (the claimed node_did in resolve failures, for one). A signed event carrying a control-bearing node_did reached the operator log with escape sequences intact and no length bound. Both warn sites now render through sanitize_for_log, so every present and future Rejected producer is bounded and control-free at the one sink. The live-mesh test now also publishes a signed event with a 4096-char control-bearing node_did and asserts the warn stream shows the refusal with no escape byte and no run past the sanitizer ceiling. A unit test pins the new ordering by asserting the exact fixed refusal for an unsigned event with a hostile node_did. * fix(node): dedupe the ref-update row on the replay key, not on a random id The reservation releases on a failed write so a republish can repair the half that never landed. But each ingest minted a fresh row id, so a retry after a partial failure (row stored, queue write failed) stored the same signed content a second time. A replay in a degraded state could repeat a landed side effect until a budget refused it. The row id for a verified event is now derived from the replay key, so the retry's insert dedupes on the existing ON CONFLICT instead of duplicating. The queue entry still retries, which is the repair the release exists for. Unsigned events keep a random id: they carry no replay key by design, and deduplicating unauthenticated content would let an attacker pre-send a victim's expected event and have the genuine one dropped. Two existing tests pinned the duplicate write and now assert the deduped shape: the partial-failure republish repairs the queue entry while the row stays at one, and the restart readmission's residual effect is a duplicate queue entry (a refetch), not a second record. The new test asserts both the retry reaches the writes (WriteFailed, not Replayed) and the row count stays at one. * test(node): pass a libp2p identity to p2p::start in the gossip delivery tests * ci: trigger the PR checks on the retargeted base --------- Co-authored-by: Kevin Codex <[email protected]> |
||
|
|
421f03ac46 |
fix(node): reject revision syntax in issue ids and drop service passthrough (#521)
* fix(node): reject revision syntax in issue ids and drop service passthrough
resolve_issue_id spliced the request-path issue id straight into a git
revision (`cat-file -e refs/gitlawb/issues/<id>`) and a for-each-ref
pattern, and on an exact match echoed the raw input back as the
canonical id. Revision and glob syntax such as `<uuid>^{blob}`, `~0`, or
`*` therefore resolved to an existing issue and was returned to callers
(and stored by create_issue_comment) as if it were the id. Accept only
ASCII alphanumerics and '-' and report anything else as not found.
service_to_command fell through to returning the caller's service string
for an unknown service. Every caller validates first, but the mapping
now yields only the two fixed subcommands and errors otherwise, so an
unrecognized value can never reach git's argv.
Addresses CodeQL rust/command-line-injection alerts #83 and #80.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
* test(node): cover service_to_command allowlist and passthrough rejection
Direct unit test ensures unknown services and option-shaped inputs cannot
map to git argv even if HTTP-layer validate_service is bypassed in future.
Co-authored-by: Cursor <[email protected]>
---------
Co-authored-by: Claude Opus 5.5 <[email protected]>
Co-authored-by: Cursor <[email protected]>
|
||
|
JATMNandClaude Opus 5.5
|
e2ae21795a |
chore: rebrand user-facing gitlawb references to Twigpine (#520)
* chore: rebrand user-facing gitlawb references to Twigpine
Rename the product name in docs, README, CLI help/output, MCP tool
descriptions, macOS app strings, installers, npm metadata and license
headers from gitlawb/Gitlawb to Twigpine. Point GitHub links at the
renamed Twigpine org (and fix the release workflow's repository guard,
which still matched Gitlawb/node), and switch the public node hosts and
website/install URLs to twigpine.com.
Protocol and operator identifiers are intentionally unchanged
(did:gitlawb, gitlawb://, signing domains, GITLAWB_* env vars, crate
and binary names, X-Gitlawb-* headers, icaptcha and peer-network URLs);
they need a follow-up migration.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
* docs: point image pulls at ghcr.io/twigpine/node and fix macOS DMG name
The release workflow publishes the container image to
ghcr.io/${GITHUB_REPOSITORY,,}, which is ghcr.io/twigpine/node since the
org rename; the old ghcr.io/gitlawb/node path no longer resolves for
anonymous pulls. Point the RUN-A-NODE docker run example, the OSS
readiness audit's claim about it, and the macOS app's generated compose
file at the published path.
The README listed the macOS DMG as "Twigpine Node.dmg", but
build-macos-app.sh writes GitlawbNode-macOS.dmg (kept on purpose), so
document the real file name.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
* fix(infra): keep the AWS security group description to avoid replacement
`description` on aws_security_group is ForceNew, so renaming it forced a
replace of the in-use group on the next apply of an existing deployment.
Restore the original text and note why it must stay until a separate
infra migration.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
* fix(macos): regenerate the app's compose file instead of treating it as a repo's
Config.repoPath fell back to the data directory when no repo clone was
found, and DockerCompose then picked up the compose file it had generated
there as if it belonged to a repo. The generator therefore only ran on
first launch, so upgraded installs kept their original compose, including
the old ghcr.io/gitlawb/node image, which no longer serves anonymous pulls.
repoPath now returns nil when there is no clone, and the app regenerates
its compose file whenever no repo compose is in use. Project directory and
named volumes are unchanged.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
* fix(infra): default the AWS node image to ghcr.io/twigpine/node
release.yml publishes to ghcr.io/${GITHUB_REPOSITORY,,}, which is now
ghcr.io/twigpine/node; ghcr.io/gitlawb/node denies anonymous pulls, so the
default made new deployments fail to start and the upgrade command fail
on existing ones. On an existing deployment `terraform apply` only updates
the upgrade SSM document in place (user_data drift is ignored); the new
image is pulled on the next upgrade_command.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
* fix(macos): keep operator edits to the generated compose, migrate only the image
|
||
|
beardthelionandKevin Codex
|
ed76adfe29 |
fix(node): fence a repo publish on the write attempt that owns it (#285)
* fix(node): close the advisory-lock probe's session if it is dropped mid-acquire A cancelled .await does not cancel an already-sent SQL statement, so a pg_try_advisory_lock whose future is dropped still takes the lock server-side while the caller abandons the result, leaving nothing to release it. The connection then returns to the pool holding the lock and wedges that repo until sqlx recycles the session. Introduce LockProbe, which owns the connection across the in-flight try-lock and closes it in its own Drop if it is still held. close_on_drop is a one-way setter, so the arming lives in Drop rather than being set up front and cleared on success; disarming is Option::take, which is what into_conn does once an acquire is actually observed. This is now the only place that issues pg_try_advisory_lock. The committed gate drops a probe without taking its connection, which is the state a cancellation leaves behind, and polls a standalone observer until the lock frees. Deterministic on purpose: the timing sweep that found this window leaks roughly 1 in 600, which is not something a CI gate can rest on. Observed RED before this change with the lock still held for the full 10s window. Refs #279 * fix(node): add a dedicated, lazily-connected advisory-lock pool Pinning a connection for the lock's lifetime is only safe if those connections come from somewhere other than the pool serving ordinary request handlers, otherwise a push burst starves every other query. Add GITLAWB_DB_LOCK_POOL_MAX_CONNECTIONS (default 32) and a Db::lock_pool builder, with the sizing tradeoff documented on the field and in .env.example: every in-flight write pins one connection here, so the value is a hard ceiling on simultaneous writes node-wide. The pool connects lazily on purpose. The main pool must connect eagerly because it runs migrations, which is why it needs connect_db_with_retry's backoff and degraded-server handoff; that function is not a generic retry helper and the lock pool is built well after the db-ready handoff has already resolved. A lazy pool has no startup work, so it adds no new way for the process to fail to boot and needs no second copy of that machinery. If Postgres is unreachable when the first write arrives, that write fails on the pool's own acquire timeout, like any other database-backed request. Pure configuration, so no proof-first cycle: the knob is covered by a parse/default/reject-zero test. Db::lock_pool has no caller until the guard wiring lands, hence the temporary dead_code attribute. Refs #279 * fix(node): hold the lock-owning connection in RepoWriteGuard (#279) Postgres advisory locks are session-scoped: only the backend that took one can release it. acquire_write took the lock through fetch_one(&pool) and release unlocked through execute(&pool), two independent checkouts, so the unlock usually landed on a session that held nothing and returned false. Measured on main: two writers on one node and the same repo BOTH acquired, 50 of 50 sequential cycles leaked, and 100 writes left 100 orphaned advisory locks on the server. The guard now owns the PoolConnection that took the lock, drawn from the dedicated lock pool, and releases on that same session. The retry loop probes through LockProbe so a cancellation mid-acquire cannot strand the lock, and hands the connection back before each backoff so a spinner on a contended repo does not pin a slot while idle. Pool exhaustion is deliberately not retried. It is a different condition from lock contention, and retrying it would spend all 60 attempts on a capacity problem unrelated to this repo while reporting it as someone else holding the lock. Both #279 acceptance tests were observed RED first: the exclusion test admitted the second writer, and the leak test reported 1 lock held where 0 was required. Both GREEN after. Full crate suite 516 passed. Db::pool() is removed because this change was its only caller. Refs #279 * fix(node): free the advisory lock when a write guard dies without releasing A guard can exit without reaching release(): an early ? on the pre-write download, a panic, or an axum handler future cancelled when the client disconnects. Its session still holds the lock, so returning that connection to the pool would block every future write to the repo until sqlx recycles it, which on the 0.8.6 defaults is ten minutes idle or thirty minutes lifetime. Close the session instead and let Postgres free the lock at session end. One hazard found by writing the teardown test rather than by reasoning: PoolConnection::drop spawns onto the runtime for both closing and returning, and panics outright when no runtime handle exists. That panic would fire inside a Drop and abort the process during unwind. It is not introduced by this commit, it comes with owning a PoolConnection at all, but this is where it becomes reachable. So Drop checks for a runtime first and, with none, leaks the handle deliberately rather than panicking: the process is already exiting and socket teardown ends the session, which is what frees the lock at exit anyway. Observed RED before the fix, with the lock still held for the full 10s poll window against a standalone observer on a no-reap pool, and the teardown case panicking in sqlx-core connection.rs:208. Proven load-bearing after: neutering the close_on_drop call turns the drop test RED again. The must-not case (a released guard reuses its backend pid across four writes on a pool sized 1) passes in both states, so the signal is specific to the abandoned-guard path. Full crate suite 519 passed. Refs #279 * fix(node): read the advisory unlock's result instead of discarding it pg_advisory_unlock reports "you did not hold this lock" as a false RETURN VALUE plus a server WARNING, never an error, so let _ = execute(...) could not distinguish a real release from a no-op. Three blindnesses stacked in those four lines: execute discards the row, the boolean lives in the row, and let _ discarded the Result too. Read it through fetch_one into (bool,). A false means this session's lock state is not what we believe it is, so the connection stays in the guard for Drop to close rather than being handed back to the pool as clean. Only a confirmed unlock returns it. A query error gets the same treatment, since the lock must not outlive a session we can no longer reason about. Note this is the only unlock site: the pre-write download's error path returns through the guard, so Drop covers it and there is no second place to keep in sync. RED before: the connection came back on the same backend pid after an unlock that returned false. GREEN after, and proven load-bearing by treating false as success, which turns it RED again. The must-not case (a normal release still reuses its backend) passes in both states, so this does not over-close the happy path. Full crate suite 520 passed. Refs #279 * fix(node): bound the object-storage transfers that run under the write lock Two transfers happen while the per-repo advisory lock is held: the archive download inside acquire_write, which runs after the lock is taken and before the guard exists, and the upload inside release. Both were free before the guard pinned a lock-pool connection, because the lock's connection went back to the pool immediately. Now an unbounded stall holds a lock-pool slot for its whole duration, so enough concurrent stalls deny every write on the node with no reaping path. Add GITLAWB_LOCK_HELD_TRANSFER_TIMEOUT_SECS (default 300) and route both through it. A timed-out upload is UNKNOWABLE rather than failed, so the timeout arm takes no compensating action: the PUT may well have landed. The lock is released either way, which trades a narrow last-writer-wins window for not wedging the repo behind a stalled transfer. Deliberate, and stated here rather than discovered later. Note what is NOT bounded by this knob: acquire_fresh's download. It runs before any lock is taken, so it holds nothing. An earlier revision of this change put the bound there by mistake, because both functions contain an identical download call and the first match won. Two disclosures. Coverage: the committed tests cover the bound mechanism, not the wiring. Driving a genuinely stalled transfer through acquire_write needs either the object-store abstraction (out of scope) or a process-global AWS_ENDPOINT_URL_S3 mutation, which would make the suite order-dependent under the concurrent runner. That a stalled transfer is bounded is therefore verified by reading, not by execution. Behavior: on timeout with a local copy present, the download falls into the pre-existing self-healing fallback and the write proceeds against that local copy. This widens the conditions reaching that path from corrupt-or-unreachable to include merely-slow, so a stale tree could now be written and re-uploaded on a slow link. Kept consistent with the existing failed-download behavior rather than inventing new semantics here; changing it is its own decision. Full crate suite 522 passed. Refs #279 * fix(node): authorize before taking the write lock in close_issue close_issue acquired the per-repo advisory lock, then ran the owner-or-author check, then returned 403 with the lock still held. That was harmless only because the lock excluded nothing. Making it work, as the preceding commits do, turns this ordering into a denial primitive: any caller with repo read access could take the write lock on demand and be refused the write, while a legitimate writer burned its 60-attempt retry budget against a lock held by someone with no write authorization. On a public repo that is every permissionless identity. This series creates the exposure, so it closes it in the same series. The owner check is cheap and moves above the lock outright. The author fallback needs the issue's git-JSON blob, since there is no author column, so it now reads through acquire() rather than acquire_write(): an issue's author is set at creation and never changes, so reading it outside the lock races nothing, and only the mutation needs exclusion. Two deliberate behavior choices. A non-owner whose authorship cannot be established gets 403 rather than 404, so this route does not tell an unauthorized caller whether an issue exists. And the owner's existing 404-for-a-missing-issue path is preserved by re-reading under the guard, which the mutation wants anyway. Swept the other three acquire_write sites: merge_pr owner-gates at :200 before acquiring at :214, the repo write path checks did_matches at :916 before :933, and create_issue's read gate is legitimate because that caller IS authorized for the action it performs. close_issue was the only one with the wrong order. RED before: with an independent session holding the repo's lock, a stranger's request sat in the retry loop until the 3s deadline fired (Elapsed), which is the wedge itself. GREEN after, refused immediately. Proven load-bearing: disabling the pre-lock refusal restores the Elapsed. Full crate suite 523 passed. Refs #279 * fix(node): address the review findings on the advisory-lock series Seven fixes from a seven-reviewer pass. Eight of the nine substantive findings were defects introduced by this branch, and two contradicted claims made in its own earlier commit messages. P0, the worst of them. The under-lock download's timeout was folded into the same Err as a corrupt archive, so it fell through to the use-the-local-copy fallback and the write proceeded. Two ways that destroys data: we do not know whether we hold the latest tree, so re-uploading can overwrite another node's newer archive; and the abandoned download's extraction runs in an uncancellable spawn_blocking that ends in remove_dir_all + rename over the same path, so git would have been running against a directory a background task was about to delete. Now the Option is branched rather than collapsed: a genuine fetch error keeps the local-copy fallback, a timeout refuses the acquire and lets the guard's Drop free the lock. LockProbe closed its connection on ordinary contention, so a 60-attempt spinner tore down 60 backends -- the exact behavior an earlier commit claimed it avoided. The test written to guarantee that asserted the HOLDER's lock count, which cannot observe the probe's own connection, so it passed throughout. The correct predicate turned out to be narrower than the first attempt at this fix: not "we saw an answer" but "we positively know nothing was acquired," because a true answer means the lock IS held and dropping without handoff leaks it exactly as a cancellation would. The first attempt reopened U1's leak and U1's gate test caught it. tigris.exists() ran unbounded inside the lock-held span, so the knob's promise was not kept and the docs were wrong about the shape. The HEAD and the download now share one budget, so worst-case occupancy is one budget per span rather than two. close_issue's pre-lock author read used acquire(), whose fast path returns on a cached directory and never contacts object storage, so on a multi-node deploy an issue author would be refused their own issue. Now acquire_fresh, which refreshes without taking the lock -- the property the comment already claimed. That comment also asserted authorship is immutable, which a reviewer disproved by force-pushing a forged blob at refs/gitlawb/**; reworded to the real justification (a pre-check whose mutation re-reads under the guard). The underlying pushability is pre-existing and larger than this branch. The retry loop had no wall-clock cap, reaching ~360s against a 120s proxy idle timeout that was itself lowered from 600s after held connection slots caused a production outage. Capped at 90s, with the bail message naming the real bound. All four acquire_write call sites stringified their error, destroying the anyhow downcast that error.rs documents explicitly ("without this, every database outage surfaces as a 500 instead of a 503"). PoolTimedOut is in that 503 set, so a saturated lock pool told clients not to retry something transient. Now propagated. Tests: six that the plan required and the first pass never wrote. The R4 pool-isolation test (named in the plan as proving the pool split, entirely absent), close_issue's owner and non-owner-author twins (INV-21c, a two-principal gate with only its deny arm covered), a waiter-does-not-block- an-unrelated-repo case, and a config test for the transfer knob. The runtime-teardown test no longer returns green when DATABASE_URL is unset. Drop's no-runtime branch now detaches via leak() instead of mem::forget: PgConnection has no Drop impl, so the socket closes synchronously and the lock frees immediately rather than at process exit. Evidence. The probe predicate is proven load-bearing in BOTH directions: forcing always-close reddens the churn regression, forcing never-close reddens the cancellation gate, and only the correct predicate satisfies both. The author twin reddens when the fallback is removed. One honest limit: the author twin does NOT redden when acquire_fresh is reverted to acquire. With tigris disabled in tests the two calls are identical, so that fix is correct by reading, not by execution -- the same seam that leaves the transfer bound's wiring untested. Full crate suite 529 passed, clippy -D warnings clean. Refs #279 * fix(node): log lock-pool saturation in the request path, not via readiness Completes the shed-in-path half of the availability story. F7 already made a pool timeout a retryable 503 by letting the sqlx downcast through; this adds the operator-facing half, with the pool's own size/idle counters so an incident can distinguish "the pool is full" from "the database is gone" without reproducing it. Deliberately NOT a readiness probe. /ready gates Fly routing with no fail-open, so failing it on a saturated pool would pull this node's READS out of service too and push its write load onto peers carrying the same load. That is the downward spiral AWS's health-check guidance names and the SRE Book documents independently. A lock-pool readiness probe would also add nothing on the reachability axis, because both pools are built from the same database_url, so the existing app-pool ping already answers it. The error is still returned via .context() rather than replaced, because anyhow preserves downcastability through context layers and the 503 mapping depends on it. Cost is bounded by construction: one line per failed acquire, and a failed acquire has already paid a multi-second pool timeout. Refs #279 * fix(node): refuse a write when object storage is unknowable, and shed contention as 503 Four defects a seven-reviewer pass found in the previous two commits, three of them cases where one arm of a match contradicted its neighbour. The under-lock refresh read a failed Tigris HEAD as "no archive" (`exists().await.unwrap_or(false)`), skipped the refresh, and wrote against a possibly-stale tree, then re-uploaded over it. That is the outcome the timeout arm twenty lines below explicitly refuses. A failed HEAD and a failed download leave us knowing different things, so they no longer share a branch: only a download failure after a successful HEAD establishes that the local copy is a sound thing to fall back to and re-upload. A HEAD failure now refuses the write. Lock contention that ran out the acquire deadline surfaced as a 500 carrying the owner slug and repo name in the client-visible body, while pool exhaustion fifteen lines up returned a deliberate 503. Contention is transient and ordinary, so it now maps through a typed `RepoBusy` to a retryable 503 with a fixed body; the detail stays in the log at the raise site. `lock_not_taken` was assigned after the try-lock answered, so an error left a previous `true`-derived value standing and could return a lock-holding session to the pool. It is now cleared before the statement is sent. `leak()`'s no-panic property in the guard's no-runtime Drop branch depended on `min_connections == 0` without saying so; a future tuning change would have silently re-armed a panic inside Drop. Made explicit with the reason. Two tests that did not bind: - `waiter_on_one_repo_does_not_block_another` proved only that two lock keys do not collide. It now samples the pool counters across more than two backoff cycles. RED at 0/50 samples with `drop(probe)` moved after the sleep, the exact defect it names, which the previous version survived. - the exhaustion test's `Ok(Err(_)) | Err(_)` was satisfied by its own outer timeout. It now requires `PoolTimedOut` and asserts the slots are accounted for. RED with the pool sized N+1. `contended_acquire_sheds_as_repo_busy_not_internal_error` is new and drives the deadline path for real; the deadline became a field so it does not wait 90s. RED at 500-vs-503 with the downcast removed. The acquire deadline's docstring claimed a total under the 120s proxy idle timeout, which the 300s under-lock transfer bound in the same function contradicts. It bounds the wait only, and now says so. The backoff is clamped to the remaining budget. * fix(node): re-authorize close_issue under the guard instead of only re-checking existence The guarded re-read matched `Ok(Some(_))` and discarded the blob, so the authorization decision rested entirely on the pre-lock read. A comment above claimed the opposite. That is not a narrow window: `acquire_write` re-downloads the archive after locking, so the tree that gets mutated is routinely not the one the author was read from. With owner-push enforcement defaulting to false and branch protection covering only `refs/heads/*`, `refs/gitlawb/issues/*` is pushable, so a forged author blob landing between the two reads was honored. The blob is already in hand under the guard, so re-asserting owner-or-author costs a deserialize. A non-owner whose issue has vanished gets 403 rather than 404, matching the pre-check's existing refusal to reveal existence. `owner_can_still_close_after_the_reorder` seeded the owner as their own issue's author, which made it unable to fail: with the owner check disabled the author fallback granted the close and the test stayed green. It now seeds a third party, so only the owner arm can grant. RED with `is_owner = false`. Also drops the claim that the author twin covers the acquire-vs-acquire_fresh distinction. `RepoStore::for_testing` hardcodes `tigris: None`, so the two calls are identical in every test here and reverting that line leaves the twin green. Separating them needs an object-storage seam. Stating the gap beats asserting coverage that does not exist. * style(node): rustfmt the new contention test Whitespace only; cargo fmt --check gates the push. * fix(node): shed an unrefreshable write as a retryable 503 with a fixed body The two refusal arms added for the under-lock transfer bound returned bare anyhow errors, so AppError's From impl (which downcasts only sqlx::Error and RepoBusy) landed them in Internal: a 500 internal_error whose body was the error string. Two defects in one. A transient object-storage failure told the client not to retry, and the timeout arm's message interpolated the owner slug and repo name straight into the response body, contradicting the fixed-body policy the RepoBusy arm sets six lines above it. RepoUnavailable follows RepoBusy exactly: a fieldless type raised with the operator detail in a context string, downcast to its own rung, mapped to a 503 whose body interpolates nothing. The detail stays in the log at the raise site. The timeout arm logs at error! where its sibling logs at warn!, deliberately: a 300s stall pinned a lock-pool slot for five minutes and is the condition the transfer bound exists to surface, so it must keep paging when the handler classifier demotes the ordinary blip case. * fix(node): refuse a fresh-copy write when the archive HEAD cannot be read acquire_fresh collapsed a failed HEAD into "no archive exists" via unwrap_or(false) and served the local copy, while the sibling path under the lock already refused on the same condition. A push that hit a storage blip got an advertisement built from a possibly-stale tree, uploaded a whole pack, and was then refused by acquire_write for the reason the advertisement had already swallowed. The authorship pre-check on close_issue read the same way, which is an infrastructure failure resolving toward a denial. A failed HEAD now raises RepoUnavailable, so both callers surface it as the retryable 503 rather than a stale success. The download-failure fallback is unchanged: a present-but-unreadable archive is still self-healed from local. info_refs keeps its map_err for every other failure so the read path's error vocabulary does not move; only the typed error takes the From chain. A test-only TigrisClient constructor pointed at a closed port makes both refusals executable, so this is no longer verified by reading. It also proves the under-lock arm end to end, which the earlier draft had recorded as an untestable gap. * fix(node): log expected transient acquire failures at warn, not error Both acquire call sites logged every failure at error, including RepoBusy, which the raise site already logs at warn. Ordinary write contention paged. The previous commit widened the problem: info_refs now raises RepoUnavailable on a storage blip, so that site would have started paging on the condition this series just classified as transient and retryable. A classifier over the two typed refusals picks the level at both sites, mirroring the startup path's permanent-vs-transient split. Anything it cannot classify still logs at error, so an unknown failure keeps paging. * fix(node): log the read failure the close_issue pre-check folds into a denial The authorship pre-check treated a get_issue I/O error and a genuinely absent issue as the same None, with no log line. The client answer is deliberately identical, since a caller who cannot write must not learn whether the issue exists, but the two are not the same event and an operator had no way to tell a real authorization denial from a filesystem or parse failure behind it. Splitting the arm leaves the 403 exactly where it was and makes the read failure visible in the log. * test(node): poll for the closed session instead of sleeping a fixed 300ms The release-invariant test slept 300ms for a Drop-spawned close before comparing backend pids, which is a coin flip on a loaded CI runner. It now polls pg_stat_activity for the captured pid on a standalone connection, the same discipline poll_until_free documents: a pooled observer would be handed the session under measurement and hide the effect. The conversion was checked against the failure it exists to catch rather than assumed. Pooling the session on an unlock that returned false makes the test fail after the poll deadline, not hang, and no_reap_pool disables idle timeout and max lifetime so nothing but the close under test can retire that backend. A generous deadline would have been the same defect as the sleep. * fix(node): raise a failed fresh download with no local copy as RepoUnavailable acquire_fresh already refused a failed Tigris HEAD as RepoUnavailable, so the handler layer mapped it to a retryable 503. A failed GET on the same path still returned a bare anyhow error, which the info/refs map_err closure stringified to AppError::Git and answered 500. One endpoint therefore told the client a transient object-storage blip was permanent or retryable depending on which call failed, and close_issue's pre-check inherited the same split through its bare ?. Raise it at the source instead of at each consumer: From<anyhow::Error> for AppError already downcasts RepoUnavailable out of the context chain, so both callers pick up the retryable mapping without touching either. The new test drives HEAD 200 with GET 500, which is the exact state the refusal is for: archive present per HEAD, GET failed, no local copy to fall back on. * test(node): assert the second writer sheds as RepoBusy instead of timing out two_writers_on_the_same_repo_are_not_both_admitted wrapped the second acquire_write in an 8-second outer timeout and only checked the future had not finished. That passes for any stall, including lock-pool saturation or a slow CI box, so it could not tell a working shed from an unrelated hang, and it cost 8 seconds on every suite run. Use with_lock_acquire_deadline and assert the typed RepoBusy downcast while the first guard is still held, matching what contended_acquire_sheds_as_repo_busy_ not_internal_error already does. It now also fails loudly if a second writer is admitted, which the timeout version could not distinguish. * fix(node): read the close_issue author pre-check from a non-mutating snapshot The non-owner author fallback refreshed through acquire_fresh, which publishes into the live repo directory: its extract step removes the existing directory and renames the new one into place. That runs with no write lock held, so any signed non-owner could trigger a directory swap underneath an in-flight guarded write on the same path. read_snapshot downloads to a throwaway temp dir and hands back a RepoSnapshot that removes it on drop, so the pre-check still sees fresh data and the live path is never touched. download_to grows a publish flag to serve both shapes from one path. The wedge invariant still holds: a stranger is refused without waiting on the write lock. * fix(node): bound every await in the lock-acquire loop by the deadline The remaining budget was checked before the pool checkout but bounded neither the checkout nor the pg_try_advisory_lock query that follows it. A checkout starting just under the deadline could wait out the pool's own acquire timeout, and a slow query could be accepted after the budget was spent, so the advertised wall-clock cap held only on the fast path. Both awaits now run under the remaining budget and shed as RepoBusy when it runs out. The probe's Drop closes its session, which cannot hold a lock it never confirmed taking, so the query-timeout arm is a plain shed. * test(node): add an S3 mock that really enforces conditional writes The fence work landing next is only as good as what the tests can observe, and a mock that answers 200 to every PUT would make the whole suite vacuous. This one holds the object and its ETag, refuses a mismatched If-Match and an If-None-Match "*" over an existing object with 412, and mints a fresh ETag per successful PUT so two byte-identical archives never share a token. Capture-then-replay is deliberate rather than parking a handler and hoping it resumes: when tokio drops an SDK future the client can tear the connection down and cancel the server task with it. Replaying what arrived models the arm that matters (body fully transmitted, commit decided later) with no timing in it. Six tests pin the semantics in both directions so a hollowed mock cannot hide. * fix(node): stop pretending a held lock fences a late upload The timeout arm claimed that keeping the advisory lock protected a successor from an abandoned PUT. It does not. release takes mut self, so the guard drops the moment it returns and Drop closes the session; measured on this branch, a successor took the same repo's lock 5ms after release returned while the PUT was still in flight. The comment and warn now say what is actually true: the outcome is unknowable, the PUT may still land, the lock releases normally, and a conditional upload is what keeps a late publish from overwriting a successor's archive. Two tests replace the claim. Session disposition is the observable that separates the two shapes, so the unlock is pinned to run and be confirmed on the guard's own session with the connection returned to the pool, checked by backend pid. Successor admission is pinned too, but noted as not what proves the point, since the lock frees within milliseconds either way. * feat(node): let the storage client express and detect a conditional write upload takes an UploadPrecondition (IfMatch, IfAbsent, Unconditional) rather than an Option<String>, so the absent case and the deliberate no-fence case are distinguishable at the type level and a caller cannot lose the fence by passing None. head_etag reads the current ETag alongside exists, which keeps exists and its other callers untouched. A failed precondition has to be classified off the raw HTTP status: PutObjectError models no PreconditionFailed variant, so a 412 arrives as Unhandled with nothing useful on it. 412 is always a lost precondition and 409 is one under IfAbsent. 404 deliberately is not: no archive delete exists on this line, so a 404 on a conditional PUT means a wrong bucket or endpoint, and reporting that as retryable would send clients into a loop against a permanent fault. The three background uploads outside the write guard now publish with IfAbsent. They fire only where the archive is expected absent, and leaving them unconditional would defeat the fence from the side: init uploads an empty bare repo, so a push landing just before it could have its archive replaced by that empty one. A refusal there means someone else already published the key, which is logged as the correct outcome rather than a failure. * fix(node): fence the release publish so an abandoned upload cannot win acquire_write now reads the archive's ETag under the advisory lock and the guard carries it, so release publishes conditionally on the generation it actually refreshed from. An upload abandoned by an earlier writer no longer overwrites a successor's archive: the store rejects it, because the ETag it was written against is gone. A refused precondition gets exactly one supersede-retry, never a loop. The distinction that makes this sound is that a 412 is a definite answer, unlike the timeout arm where nothing is knowable, and the retrying writer still holds the lock, so whatever landed underneath was written without one and its tree is not the authority. Two losses in a row refuse instead of escalating. That retry is what keeps ordinary pushes working now that init publishes create-only: a first push racing init's upload of the empty repo loses once and then wins, rather than surfacing a 503 on the most common operation there is. release returns a must-use outcome and the four publishing handlers propagate it before any trust bump, webhook, or success body, so a publish the store refused reads as a retryable 503 instead of a 201. The three release(false) sites deliberately do not map it: they publish nothing, and a 503 there would shadow the 403 or 404 the route means to return. The download-failure fallback also publishes fenced now. That arm knows the stored generation (its HEAD succeeded, only the GET failed), so publishing unconditionally from it would reintroduce the same overwrite. * test(node): drive the abandoned-upload race end to end, and probe the real backend The headline test is the one that had to exist. Writer A's release is parked past its transfer bound and returns with the outcome unknowable, B acquires and publishes, and A's captured PUT is then replayed: the store answers 412 and B's archive survives. The create-only variant covers the arm whose real-world failure is silent rather than loud. The control is what makes those attributable. With no interleaved B, an abandoned-then-replayed PUT whose generation still matches lands. Without it the headline would only show that replays get rejected, not that staleness is what rejects them. Header assertions come last in all three, so a lost fence reds on the outcome it is about rather than on a wire-format check. A mock cannot prove Tigris honors any of this, and the vendor requires a Single-region or Multi-region bucket for conditional operations, so against a Global or Dual-region bucket the fence is a silent no-op. The credentials-gated probe checks both arms against the real endpoint and cleans up unconditionally, including when an assertion fails, which is the case it exists to catch. It accepts 412 or 409 on the create-only arm because both mean the precondition was enforced and both are already classified as a loss. * fix(node): reconcile F-series tests and test seams with the rebased guard shape The rebase onto main carried main's #174 F-series guard tests forward. The branch's guard replaces main's locked/released/test_pre_unlock_gate Drop mechanics with close-on-drop (runtime) and leak (off-runtime), so two tests that asserted the replaced mechanics are reconciled: the off-runtime disposal test now asserts the leak observable (the slot never returns to idle) and the detached-unlock-returns-connection test is dropped, its invariant covered by the U-series drop-frees-the-lock gates. The pre-unlock gate seam is restored (test-only) so the mid-unlock cancellation tests keep their deterministic park. ipfs.rs tests are updated to the branch's sync test-client constructor and 4-arg RepoStore::new. Refs #279 * fix(node): address the 2026-08-10 review round on the advisory-lock series - P1: serve the receive-pack advertisement from a non-mutating snapshot instead of acquire_fresh, so the unlocked advertisement cannot delete or swap the live repo directory under a concurrent guarded write (the stale-ETag success ordering). acquire_fresh loses its only production caller and is removed. - P1: read-gate, rate-limit, bound, and cancellation-clean the close_issue pre-lock snapshot, so a signed non-owner cannot drive unbounded Tigris downloads and blocking extractions, and an abandoned extraction no longer leaks its temp dir. - P1: classify raw 409/412 responses as a lost conditional write via SdkError::raw_response() in both upload and the publish supersede-retry, so an unparsable error body cannot acknowledge a write that was never published. - P2: wrap the cold-cache under-lock download failure in RepoUnavailable so it sheds as a retryable 503 like the HEAD arm, not a permanent 500. - P2: surface a refused create-only fork upload as PreconditionLost and refuse the fork, so an orphan archive cannot shadow a fork's DB record. Refs #279 * fix(node): record why the close-issue read gate cannot carry a synced row The gate added for the pre-lock snapshot reads a repo row's own visibility rules and public flag. A row synced from a peer is stored public and carries none of the owner's rules, so for that class the gate can only return allow. Refusing instead would deny the repo's real owner and the issue's real author on any node whose only copy is a synced one, and it would not buy the protection it appears to, because a synced row's recorded owner comes from the peer that sent it. Every other read gate in the API reaches the same verdict for such a row, so this is left as is and stated at the call site rather than special-cased here. The test that covered the gate seeds a locally created repo and cannot observe this. Adds one that pins the property directly: the gate allows an arbitrary caller on a synced row, and the handler refuses that caller anyway, because the owner-or-author check is what decides this route. * fix(node): address jatmn review on merged advisory-lock head Align .env.example db pool with validate floor, move snapshot cleanup into the blocking extract task, remove fork clones on PreconditionLost, restore upload_site_reached test plumbing, and use owner DIDs in receive-pack tests after #330 owner-push default. * fix(node): cap close_issue snapshot work on the read pool Take a git_read_semaphore permit before the non-owner author snapshot so parallel close attempts cannot each drive a full archive extraction while still under the hourly rate bucket. Format owner-DID test helpers. * fix(node): mark close_issue mirror visibility gate for push hook Annotate list_visibility_rules with mirror-rows-handled so the pre-push surface detector accepts the synced-row read gate. * fix: unblock pre-push after main merge Read CappedBody.text in gl whoami (#381 shape), restore legacy CID sweep wiring dropped in the merge, route rate-limiter cleanup through AppState, and allow build_lock_pool for test-only callers. * fix(node): carry transient acquire failures through every write handler Extend acquire_write_app_error so RepoBusy and RepoUnavailable map to fixed-body 503s on issue and pull paths, matching receive-pack. Fix the flaky u2 drain exhaustion test via a drain_faults seam and add handler regressions for contention and storage unavailability on create_issue. * fix(node): drop unused logcap helper after drain_faults seam * fix(node): route fork disk paths through validated_repo_disk_path CodeQL flagged remove_dir_all on a user-derived path in fork_repo. Use the same three-layer barrier as RepoStore::local_path and reject names the validator refuses before clone. * fix(node): harden release-upload outcomes and replication tail gating Treat timed-out or failed archive uploads as non-durable in into_result, gate pinata/gossip on confirmed publish durability while keeping inv22 spawn order, split lock-pool vs main DB pool validation, map LockPoolBusy through From<anyhow::Error>, and attach Retry-After on repo write contention errors. * fix(node): close jatmn round on publish authority and durability gating Revoke under-lock refresh swaps when the advisory lock ends, fail closed on unrecorded release outcomes before Pinata/gossip, compensate fork archives when create_repo fails, recompute acquire backoff from the live deadline, and route publish swaps through validated_repo_disk_path for CodeQL. * fix(node): harden issue close/create and fork publish paths on #285 Move close_issue rate limiting after the read gate so denied readers see 404 not 429, roll back local issue refs when publish refuses, re-check swap authority under publish_lock, propagate fork upload failures before DB insert, and update lock-pool config help text. * fix(node): close write-attempt ownership gaps on advisory-lock release Make publish swaps claim-exclusive via CAS, invalidate the local read cache on definite publish refusals, harden fork create compensation and clone cleanup, and keep snapshot temp dirs owned through the async handoff. * fix(node): gate replication tail on publish durability and split close_issue limits Wait for a confirmed release outcome before post_receive_replication_tail runs any walks or coalescing work. Give close_issue its own per-IP rate bucket so it cannot drain receive-pack quota. Add regression tests for both paths. * fix(node): add ValidatedRepoDiskPath for CodeQL and reconcile tail durability Introduce a validated-path newtype with the same inline join and component walk main uses, so path-injection sinks only accept sanitised repo paths. Reconcile the publish-durability gate with the disconnect-safe tail spawn via PublishDurabilitySlot and explicit UploadUnknowable handling. * fix(node): close three jatmn findings on advisory-lock PR Retry fork create confirmation before compensating or returning, schedule background recovery when the lookup stays unavailable, install publish durability outcomes before the recorded flag, and size the lock pool for non-push mutations (default 40). * fix(node): verify adversarial findings on advisory-lock PR Fail closed when publish durability never records, bound pg_advisory_unlock with the same transfer budget as upload, and cap close_issue pre-lock snapshots at git_acquire_timeout_secs instead of the write-lock transfer bound. * fix(node): compensate under the write lock and harden tail durability handoff Run create_issue rollback through release_compensating so issue refs are deleted only while the advisory lock is still held and only on definite publish refusals, not UploadUnknowable. Replace the tail durability slot's async try_lock drop with a std mutex so cancellation during polling cannot skip installing UploadUnknowable. * fix(node): refuse stale under-lock refresh and gate tail durability on release Under-lock GET failures now always shed as RepoUnavailable, even when a local copy exists, so a transient download error cannot publish over a newer stored generation. PublishDurabilitySlot only synthesizes UploadUnknowable after release starts, and receive-pack marks that boundary before spawning the tail. Adds regression tests for both paths; inv22 F4 gate accepts the release wrapper. * style(node): rustfmt advisory-lock gap-closure commits * fix(node): satisfy clippy on stale-local download test path join * feat(node): add a publish attempt/stage boundary The four lifecycle findings on this branch share one root cause: a resource was tracked by a coarse state ("release started", "the path exists", "a row with this name exists") when the safety question was about the IDENTITY and the PUBLICATION STAGE of one specific write attempt. This is the vocabulary that makes those questions answerable: - `PublishAttemptId` — minted before the request is built, carried with the bytes as object user metadata, read back off the store to decide whether what is published is THIS attempt's work. That turns "did my request succeed", which a lost response makes undecidable, into "are the published bytes mine", which the store can answer. - `PublishStage` / `PublishStageCell` — how far one attempt got, observable from OUTSIDE the future doing the work. A cancelled handler never returns an outcome, so the stage it had reached is the only thing that can classify it. - `UploadError` split by what a failure ENTITLES A CALLER TO DO rather than by which HTTP status came back. Destructive compensation is licensed only by `proves_not_published()`. Deliberately free of any object-storage type. #79 deletes `git/tigris.rs` and replaces it with a BlobStore layer; this module is what survives that swap, with each backend supplying only its own error classifier. * feat(node): stamp uploads with an attempt id and preserve dispatch ambiguity `upload` now reports its progress into a `PublishStageCell` and stamps every PUT with an attempt id in object user metadata, so both of the questions a cancelled or unanswered publish raises become answerable: - `PreparingArchive` is marked before the blocking compression and `PutDispatched` immediately before `send()`. The conditional PUT is not constructed until compression returns, so a caller abandoned in that window definitely never attempted publication — a fact nothing could previously observe, because the only report was the return value of a future that no longer existed. - `attempt_landed()` HEADs the key and compares the stored attempt id, which is what lets a client whose response was lost recover its own committed write instead of guessing. `UploadError::Other` is replaced by `NotPublished` (proven never committed) and `Ambiguous` (may have been dispatched and may have committed). The single backend-aware classifier is `classify_put_failure`: a 4xx is an answer the server gave before storing anything and proves non-publication; a 5xx does not, and neither does a timeout, a dispatch failure, or a response the SDK could not read. `SdkError` is `#[non_exhaustive]`, so the fallback arm is the cautious one. `delete` is replaced by `delete_if_attempt_matches`, which reads the attempt off the object and fences the DELETE with `If-Match` on the generation it came from. A second name lookup before an unconditional delete would only narrow the window in which a successor's object can be erased; the conditional delete closes it. * fix(node): key durability, cache validity and fork cleanup on the attempt Closes the four lifecycle findings by consuming the attempt/stage boundary at the sites that were deciding on coarse state. P1 — cancellation before PUT dispatch is not publish durability. `PublishDurabilitySlot` is armed on the guard's publish STAGE rather than on a "release started" flag, and its `Drop` classifies an abandoned handler by how far the attempt actually got: a cancellation during compression records a definite refusal, a cancellation after dispatch records unknowable, and a cancellation after the store acknowledged records `Released` (which the flag could never say). `publish_durability_confirmed` now requires `Released`, so the detached tail no longer does IPFS/Pinata/P2P/GraphQL/Arweave/peer work for refs that exist only in a rejected local write. A guard with no storage backend seeds `NoBackend`, keeping Tigris-less deployments' tails running. P1 — an unknowable generation is quarantined, not served as ordinary cache. `release` writes a sidecar marker naming the unresolved attempt beside the live tree (never inside it: it would be tarred into the next archive), and `acquire`'s existing-path fast path reconciles it before serving. Only the store confirming it holds that attempt lifts the quarantine; anything else is a retryable `RepoUnavailable`. The tree is deliberately NOT deleted — the PUT may have landed and this can be the only local copy. A confirmed publish, an under-lock refresh, and cache invalidation each clear the marker, so the quarantine is a bounded refusal rather than a standing outage. The release-side timeout arm also stops treating every stall alike: a bound that expires at `PreparingArchive` is a definite non-publication, and a bound that expires after dispatch spends one short, separately bounded HEAD trying to reconcile the attempt before falling back to unknowable. P1 — fork confirmation and cleanup bind to the creating attempt. The DB row id is minted up front and used as the attempt id, so the object's metadata, the clone's sidecar stamp and the row all name one attempt. `confirm_fork_repo_row` looks the row up BY ID and reports Ours / Foreign / Absent, so a concurrent create, mirror registration or retry can no longer be returned as this request's own commit. Every destructive step — `compensate_fork_archive`, its background retries, and `ForkCloneGuard::drop` — is conditional on the resource still belonging to this attempt, so recovery that observed `None` before a successor committed can no longer erase that successor's archive or directory afterwards. P2 — response-loss ambiguity is preserved rather than compensated. Guarded writes map an ambiguous publish to `UploadUnknowable` + quarantine instead of `UploadFailed` + cache invalidation + the caller's undo, so `create_issue` no longer deletes an issue ref whose archive is durable. Fork creation reconciles by attempt id: a create-only PUT that committed and lost its response is RECOVERED and the row inserted, and an unresolved one keeps its only local clone and refuses retryably. The fork name is no longer fenced behind the attempt's own orphan. Tests, each RED-checked by reverting its guard: - slot_drop_during_compression_records_a_definite_non_publication - slot_drop_after_dispatch_records_unknowable - slot_drop_after_the_store_acknowledged_records_released - slot_drop_with_no_storage_backend_records_released - publish_durability_confirmed_accepts_only_released - publish_durability_confirmed_refuses_quickly_after_unrecorded_slot_drop - receive_pack_cancelled_during_compression_publishes_nothing_and_runs_no_tail (+ receive_pack_that_completes_its_publish_still_runs_the_tail as control) - a_bound_that_expires_before_dispatch_is_a_definite_failure - an_unresolved_publish_quarantines_the_tree_and_a_later_read_refuses - a_quarantined_tree_is_served_once_the_store_confirms_the_attempt - the_next_write_clears_an_inherited_quarantine - a_confirmed_publish_leaves_the_tree_readable (control) - a_guarded_write_whose_response_is_lost_is_not_compensated - a_put_that_commits_and_loses_its_response_is_ambiguous_and_reconcilable - a_closed_response_with_no_http_status_is_ambiguous - upload_classifies_404_as_a_definite_non_publication - upload_classifies_500_as_ambiguous_not_definite_failure - a_generation_that_moves_between_the_head_and_the_delete_is_not_deleted - an_attempts_own_object_is_deleted_and_a_foreign_one_is_not - fork_confirmation_never_claims_a_concurrent_attempts_row - fork_confirmation_recognizes_this_attempts_own_committed_row - fork_confirmation_reports_absent_when_nothing_owns_the_name - fork_clone_guard_leaves_a_successors_mirror_alone - fork_recovery_resuming_after_a_successor_deletes_neither_object_nor_path - fork_compensation_removes_what_this_attempt_still_owns - fork_publish_that_loses_its_response_stays_recoverable - a_fork_whose_publish_lost_its_response_is_recovered_not_fenced - a_fork_whose_publish_is_unresolved_keeps_its_clone_and_refuses_retryably - an_ordinary_fork_publishes_and_commits (control) - mock_round_trips_the_attempt_metadata_a_put_stamped The S3 mock gains attempt metadata, conditional DELETE, a commit-then-lose- the-response mode, a delivered-but-not-committed mode, and a per-key object store (fork creation touches the source and fork keys in one request, and a single slot would have let an assertion about one silently read the other). The compression seam is a condvar gate so no test holds a guard across an await. * fix(node): settle the cache contract on the abandoned path and the attempt The publication boundary this branch introduced was applied at call sites that each had to be remembered, and five were not. Close the eight gaps that survived, grouped by what actually failed rather than by symptom. The marker's lifecycle was answered by the wrong event in three places. The swap that replaces a live tree now owns the clear, so a cache-miss download no longer leaves a stale marker that fences the archive it just installed, and the under-lock refresh no longer lifts a quarantine on a HEAD that downloaded nothing. init clears a predecessor's marker. A cancelled handler never reached release, so it applied the tail contract and not the cache contract. RepoWriteGuard::drop now settles the tree it abandoned, keyed on how far the publish actually got, and a guard that handed out its path for writing marks the tree whatever stage it reached. A marker naming no attempt is a definite non-publication rather than a permanent refusal: it invalidates and the next read serves the stored generation, so an abandoned first push cannot wedge a repo at 503. Only an unreadable sidecar still fails shut. Both of the store's live-path hand-outs now go through one confirming helper, so read_snapshot can no longer return an unconfirmed tree as an ordinary snapshot. The gate binds ahead of the lazy-migration spawn, because a quarantined tree backfilled to object storage by an IfAbsent PUT would publish exactly the refs the marker exists to withhold. The marker no longer rests on a single fs::write; an unwritable sidecar falls back to an in-memory record that reads consult. Fork cleanup claims ownership by renaming the stamp before it touches the tree, so a successor that claims the same path cannot lose its directory to a predecessor's check-then-unlink, and the directory-absent path no longer unlinks a stamp it does not own. A create-only precondition loss whose reconciliation HEAD fails is refused retryably instead of reported as a permanent name conflict, and its clone is released so the retry can re-clone rather than finding the destination occupied. The pin sweep skips a tree whose publish is unresolved, so provider CIDs are not rewritten from refs that may never become durable. That is the sidecar marker, distinct from the operator-level quarantined row flag. 26 tests, each written and observed failing before its fix. Suite 1211 passed, 0 failed, 1 ignored; fmt, clippy -D warnings and --locked clean. * fix(node): settle the abandoned tree at the disconnect, not at reaper exit The settlement that marks an abandoned write ran from RepoWriteGuard's Drop, and on a client disconnect that guard is held by the Arc clone riding the admission guard into the detached reaper, so it ran only once the process group had been reaped. Reads take no advisory lock and consult only the sidecar, so for the width of that window a concurrent fetch was served the abandoned refs off the live tree with no marker beside it. Confirmed by execution before the fix: no marker on disk, acquire returning the live path, and the abandoned ref still on the tree it served. Only the lock release has to wait for the reaper; the settlement does not. TreeSettlement is a token carrying the path, the publish stage and the hand-out flag, holding the exhaustive stage match that used to live in Drop. The handler takes it out of the guard before the guard is shared with the reaper, so on a disconnect the handler future drops first and the marker lands at the disconnect instant while the reaper still holds the lock. A successful release disarms it. Drop still settles a guard nobody took the token from, so the arms have one home. The regression test reads inside the window and asserts the abandoned refs are not served. It samples pg_locks from an independent session first, so a run where the reaper had already finished reports itself inconclusive rather than passing for the wrong reason. Suite 1212 passed, 0 failed, 1 ignored. The five settlement mutations are load-bearing, including one that reverts this split and reddens the window test. * fix(node): let exactly one classifier settle an abandoned tree Moving settlement into a token the handler holds left two classifiers able to run for one write. release() classifies the tree and only then awaits the unlock, while the handler disarms the token after that await returns, so a cancel in the unlock gap dropped a still-armed token and settled a second time. After a definite refusal that is a quarantine marker written beside the tree release had just deleted, which sends every later read down the refuse-and-reconcile path for a repo whose state was already resolved. Observed before the fix: the tree gone, the marker there. The arm is now an AtomicBool shared between the guard and its token. release stores false at the classification point, above the unlock await, and settle takes the arm with compare_exchange, so whichever classifier gets there first is the only one that acts. tree_settled stays, and now has a test that says why: after the handler takes the settlement the arm is still true, so tree_settled is the only thing stopping the guard's own Drop, which on a disconnect runs in the detached reaper, from settling again at reaper exit. The reap-window test was passing for weaker reasons than it claimed. Its fake git took SIGTERM and let the group die, so the reaper could free the advisory lock before the probe read; it now traps TERM and re-parks so only SIGKILL ends it. It asserts the marker exists at the disconnect instant rather than only mentioning it, and the served-refs half no longer passes when acquire fails for an unrelated reason. Suite 1214 passed, 0 failed, 1 ignored. Nineteen mutations load-bearing, including one that reverts the release-side disarm and one that reverts the settlement split. * test(node): use the test's own pool now that Db::pool is gone --------- Co-authored-by: Kevin Codex <[email protected]> |
||
|
beardthelion
|
19a7ce2bbd |
fix(node): drop withheld-subtree trees from the replication pin set (#382)
* fix(node): drop withheld-subtree trees from the replication pin set A tree reachable only under a denied path survived both pin filters because trees were never in the blob universe, so public IPFS still learned child filenames and blob oids. Filter trees the same way as blobs on the delta withheld set and the full-scan allow-list. Closes #172. * chore: re-trigger code quality check |
||
|
beardthelion
|
189aad96e5 |
fix(node): require Content-Digest on a signed request (#306)
* fix(node): require Content-Digest on a signed request
require_signature rebuilds the RFC 9421 signing string from the request
itself, and it read the Content-Digest header with a .unwrap_or("")
fallback. When the header was absent, the covered content-digest component
came out as the empty string on both the signing side and the verifying
side, so the Ed25519 check passed over a string that committed to nothing
about the body. The comparison that would have caught it was guarded on the
presence of that same absent header, so it was skipped. The two defects
compose: a signed request could carry any body at all, and the handler
behind the middleware saw it as authenticated.
The header is now required. An absent Content-Digest is a 400 with
missing_content_digest, and the digest-versus-body comparison is
unconditional, since presence is established before it runs. There is no
longer a path through the middleware on which the body goes uncompared.
Refusing is safe for every client that already works. sign_request computes
the digest with no branch and always returns it in SignedHeaders, and all
eight production call sites attach it as a Content-Digest header: the three
in gl's HTTP client, the fetch and push requests in git-remote-gitlawb, the
node's peer announce, the sync push in sync.rs, and the sync notify in
api/repos.rs. So no conforming client regresses. What the refusal removes is
the case where a caller signed an empty digest and sent a body no signature
covered.
HttpSignature::parse now also refuses a covered component that appears more
than once, per RFC 9421 section 2.1. A repeated identifier says nothing the
single one did not, and it lengthens the signing string a verifier has to
build from a list the caller chooses.
* fix(core): linear duplicate check and reject component parameters
Use a HashSet for covered-component duplicate detection so hostile
Signature-Input headers cannot force quadratic work before rejection.
Reject RFC 9421 component parameters during parse since only bare names
are supported end to end.
* chore: re-trigger code quality check
|
||
|
beardthelion
|
6233bd9d85 |
fix(node): validate git branch refs to close option injection (#379)
* fix(node): validate git branch refs to close option injection
PR branch refs and a repo's default_branch were stored from request bodies
with no ref validation, then interpolated into single git argv elements:
git diff {target}...{source} (branch_diff / branch_diff_names), and
git worktree add ... {target} / git merge {source} (merge_branch). A value
beginning with '-', e.g. --output=/tmp/x, is parsed by git as an option
rather than a revision, so it becomes an arbitrary file write. get_pr_diff
takes an optional identity, so on a public repo the trigger is unauthenticated;
planting the PR needs only read access, and the write happens at the withhold
check before the visibility gate.
Defense is applied at two layers:
- Storage boundaries: create_pr validates source_branch and the resolved
target_branch; create_repo validates default_branch (which becomes a PR's
target when the PR omits one). These fail fast with 400 and keep junk out
of the DB.
- The sink: branch_diff, branch_diff_names, and merge_branch reject an
option-shaped ref before building the git argv, so the property holds for
every caller and every row, including legacy rows and any future writer
that skips the boundary check.
The shared validator is crate::git::store::validate_git_ref (git
check-ref-format rules, leading-dash rejection as the core), re-exported as
crate::api::validate_git_ref for the boundary handlers. No -- delimiter is
used: the arguments are revisions, and -- there reinterprets them as
pathspecs.
Both boundary guards and the sink guard are mutation-proven load-bearing.
resolve_head is unaffected (it prefixes refs/heads/); fork_repo takes no
branch from the request.
* fix(node): delegate branch ref validation to git check-ref-format
Rebase onto current main and tighten validate_git_ref to call
git check-ref-format --branch so symbolic names like HEAD and
trailing-dot components are rejected at storage boundaries and
git sinks. Add boundary and legacy-row tests for HEAD, feature.,
and feature/x.
* fix(node): reject fully qualified refs and close review test gaps
Reject refs/ prefixes after check-ref-format so tags and qualified
head names cannot be stored as PR branch fields. Add sink tests for
option-shaped merge source, HEAD target, and poisoned default_branch
resolution; assert get_pr_diff fails on poisoned diff rows.
* fix(node): allow-unbounded-git for check-ref-format validation
Stateless ref-name validation holds no repo concurrency permit.
* style: cargo fmt
* fix(node): reject revision shorthands and split ref-validation errors
git check-ref-format accepts @ and pseudoref names that git later treats as
symbolic revisions, so merge could succeed without merging the source branch.
Reject those explicitly at the shared validator and add sink/boundary tests.
Spawn failures from check-ref-format now surface as git_error (500) instead of
bad_request (400) at create_pr/create_repo boundaries.
* test(node): close ce-review gaps on git ref validation
Add sink tests for @{-1} and pseudoref poisoned rows at diff/merge,
HTTP tests that spawn failure returns git_error (500) not bad_request,
and expand API boundary cases for remaining pseudorefs and qualified refs.
Introduce GITLAWB_TEST_VALIDATE_GIT_BIN test seam with a mutex so parallel
sqlx tests do not race on the override.
* fix(node): prefix sink git argv with refs/heads and close shorthand gap
Pass stored branch short names to diff/merge/worktree as refs/heads/{name}
so revision shorthands cannot retarget another ref namespace at the sink.
Reject heads/, tags/, and remotes/ prefixes at storage boundaries, extend the
pseudoref denylist, and route boundary validation through AppState::git_bin
instead of a process-global test env override that raced parallel tests.
* fix(node): attach merge worktree to the target branch
git worktree add with refs/heads/{name} checks out detached HEAD, so a
successful merge did not advance refs/heads/{target}. Use the branch name
for the worktree and keep refs/heads/ qualification on merge revisions.
Add an end-to-end regression for diverged branches.
* chore: cargo fmt on merge ref-advance test
* fix(node): bind merge worktree to the exact local target branch
validate_git_ref proves branch-name grammar only; git's revision DWIM can
resolve the same short name in another namespace (refs/tags/{name}), so a
PR targeting a tag-shadowed name checked out a detached HEAD, merged onto
it, discarded the commit at cleanup, and the unchecked plain rev-parse
echoed refs/heads/{target} back as a bogus merge SHA — marking the PR
merged and firing pull_request.merged with no branch advanced.
Close the resolution class instead of denylisting names: (1) require the
exact refs/heads/{target} via show-ref --verify before creating the
worktree, (2) verify the worktree's symbolic HEAD is attached to exactly
that ref before merging, (3) accept the merge only if rev-parse --verify
{target}^{commit} exits zero. Add end-to-end regressions for a same-named
tag with no local branch (merge fails, PR stays open, no merged event) and
for a tag/branch coexistence (the local branch advances, the tag stays).
* fix(node): recover a leftover merge worktree instead of wedging the endpoint
merge_branch cleaned up a leftover _merge_worktree only when the directory
still existed. The state git worktree add actually refuses is the opposite: a
registration under $GIT_DIR/worktrees whose directory is gone. It fails with
"missing but already registered" (exit 128), and once a repo is in that state
every later merge on it fails, permanently.
The state is self-inflicted. remove_worktree runs git worktree remove and then
removes the directory unconditionally, so one failed removal leaves the
registration behind.
Two changes. Run the cleanup unconditionally at entry, which also makes the
bail-without-cleanup paths self-healing on the next merge. And pass --force
twice: a single --force refuses a LOCKED worktree ("cannot remove a locked
working tree"), and git worktree prune skips locked registrations too, so with
one flag a locked leftover survives the whole cleanup and git worktree add
then fails with "missing but locked worktree".
Enumerated the leftover states that can reach this code (registered or not, x
directory present / gone / locked / corrupt admin dir, x gc.worktreePruneExpire
default and never). remove -f -f plus the directory removal clears all of them,
and adding a separate git worktree prune changes no outcome in any of them, so
there is no prune call here.
Four tests, one per leftover shape that reaches this code. Against
merge_branch: the plain stale registration, the locked variant, and an
unregistered leftover DIRECTORY (git worktree remove refuses that one with "is
not a working tree", so the directory removal is what clears it). Plus an
end-to-end pass through the merge endpoint on the locked state, asserting the
PR row reaches "merged", the reported merge_sha IS the new refs/heads/{target},
and the merge leaves no worktree behind for the next one. The e2e fixture
asserts git really refuses the state before asserting recovery, so a future git
cannot make it vacuous.
* chore: re-trigger code quality check
|
||
|
beardthelionandKevin Codex
|
76ca04f355 |
fix(node): close three host-class gaps in the public-URL gate (#346)
* fix(node): close three host-class gaps in the public-URL gate (#339) is_public_http_url is the shared gate for every operator-supplied outbound URL: peer announce, webhook creation, and the boot-time peer prune. Three host forms got past it that the checks around them plainly meant to exclude. A name with no dot never matched the `.local` / `.internal` suffix rules, so `http://internal/` and `http://wpad/` were accepted and left to the resolver to complete from its search domain. The dotless rule sits on the branch where the host did not parse as an IP literal, which keeps bracketed IPv6 (also dotless) on the accepting path; the existing 6to4 and NAT64 accept cases pin that. fec0::/10 fell through the IPv6 arm because the link-local mask does not cover it: 0xfec0 & 0xffc0 is 0xfec0, not 0xfe80. RFC 3879 deprecated the range, which argues for rejecting it rather than ignoring it. Trailing root dots were stripped once, so `localhost..` reduced to `localhost.` and matched neither the equality nor the suffix check. Stripping to fixation closes that. Each of the three is covered by a test that reddens when its own production line is reverted, and the dotted forms stay as negative controls so the suffix checks remain proven. * fix(gl): consume CappedBody text in whoami error path after main merge * chore: re-trigger code quality check --------- Co-authored-by: Kevin Codex <[email protected]> |
||
|
beardthelionandKevin Codex
|
c301a68161 |
fix(node): persist the libp2p identity instead of deriving it from the node DID (#324)
* feat(node): add a persistent libp2p identity key file
Add p2p_key_path (--p2p-key-path / GITLAWB_P2P_KEY, default
~/.gitlawb/p2p.key) with a resolver mirroring resolved_key_path, and
load_or_create_p2p_keypair, which generates an Ed25519 keypair on first
start, persists it 0600, and loads it thereafter. Mirrors the existing
load_or_create_keypair idiom for the node identity PEM.
A corrupt or unreadable key file is a hard error naming the path rather
than a silent regeneration, so a disk problem cannot quietly rotate the
node's network identity.
Not yet wired into p2p::start; that follows.
* fix(node): load the libp2p identity from the persistent key file
p2p::start now takes the Ed25519 keypair loaded by
load_or_create_p2p_keypair instead of computing one from the node DID,
so a node's network identity is generated once from the OS RNG and kept
on disk rather than recomputed from a public value on every start.
The node DID parameter is gone from start; the call site loads the key
first and continues without p2p if the key file cannot be read, matching
how a swarm-start failure is already handled.
The gossipsub message_id_fn is untouched and keeps its own hasher.
* fix(node): create the p2p key with 0600 at creation and reject a loose one
Open the key file with create_new and the mode set at creation, then
fsync, instead of writing it and narrowing the mode afterwards. The
secret is never on disk under a wider mode, an interrupted start cannot
leave it readable, and the exclusive open also refuses a pre-existing
entry at the path and makes a concurrent start take the key that landed
rather than clobber it.
Refuse to load a key file whose mode grants group or other access, and
name the observed mode so the operator can fix it. Report an empty key
file as empty rather than surfacing a protobuf decode error that blames
a missing rsa feature.
Pin GITLAWB_P2P_KEY onto the mounted volume in the Docker and fly
configs and document it, so the key does not depend on home-directory
resolution to land on persistent storage.
* fix(node): publish the p2p key atomically and pin its directory
Write the key to a scratch file in the same directory and hard-link it
onto the final path. The bytes are durable before any name points at
them, so a crash cannot leave a partial key that fails to load on the
next start and takes the node off the network until someone reads the
logs. A concurrent reader can no longer observe a half-written file
either, since the final name appears complete or not at all.
hard_link rather than rename: rename replaces its destination silently,
so refusing to clobber an existing key would depend on a check followed
by a separate rename, and a concurrent start can land in that gap.
hard_link is atomic and refuses an occupied path, including a symlink,
which it does not follow.
Create the key directory 0700 and tighten it when an existing one grants
group or other access. A 0600 key under a writable directory can still be
replaced or unlinked. Tightening rather than refusing to start, because
existing installs already have 0755 there and refusing would take p2p
down on all of them through a path that only warns.
Formatting on the branch is swept up here; it was already failing
cargo fmt --check before this change.
* style(node): comma instead of a dash in the key-load warning
House style avoids em dashes in text we write. The swarm-failure warning
beside it predates this branch and is left alone.
* fix(node): require the p2p key path to name a directory
A bare filename in GITLAWB_P2P_KEY put the key in whatever directory the
process started from, and the directory guard was skipped entirely on that
path: Path::parent returns Some("") for a bare filename, which the caller
filtered out before ever reaching ensure_key_dir. The key file was created
0600 inside a directory that kept whatever mode it already had.
Config::validate now rejects a p2p key path that names no directory, so the
node says so at boot instead of starting with a key it cannot protect. That
placement is the point: an error raised in the p2p start path is logged and
stepped over, leaving the node running without p2p and reporting healthy.
The check is lexical on the tilde-resolved path. canonicalize would fail on a
parent that does not exist yet, which is the shipped ~/.gitlawb default and
every container's first boot, and comparing against the working directory
would reject /data/p2p.key under the image's WORKDIR, an absolute directory
the operator did name.
Three sites answered the parent question differently, which is how the gap
arose: one filtered the empty case out, one already normalized it, and one
opened "" and silently skipped its fsync. They now share key_parent, and
Config::validate calls it rather than adding a fourth answer.
load_or_create_p2p_keypair also refuses a path naming no directory. That is a
backstop behind the config gate, not the gate, so a later caller that skips
validation cannot quietly restore the old behaviour.
* test(node): run the key-permission probe in its own process
The probe zeroes the umask so the assertion means something: under a
restrictive ambient umask the bits are masked to 0600 regardless of whether
the code pins the mode, and the check passes either way.
Zeroing it in the shared test process is the problem. umask is process-global
and cargo runs these tests on threads, so any test creating a file in that
window inherits 000. Measured before this change: an unrelated concurrent
test's file was created 0666.
The probe now runs in a child process, where the zeroed umask cannot reach a
sibling and dies with the child. The parent is an ordinary test that runs
concurrently with everything else. Double-gated with #[ignore] plus an env
check so a bare --ignored sweep does not zero the umask in the shared process
after all.
The parent asserts the child ran exactly one test and that it passed, not
just that it exited 0. A libtest filter matching nothing runs zero tests and
still exits 0, so without that assertion a renamed fixture would read as a
green permission check while asserting nothing. Verified by pointing the
filter at a name that does not exist and watching the parent fail.
* docs(node): scope the p2p key permission claim and note the rotation
The old wording said the key file is "created with owner-only permissions"
without qualification, which is only true on Unix: every permission path in
p2p/mod.rs is cfg(unix), so on other platforms the file inherits whatever the
directory gives it and nothing is enforced. Say what is actually enforced and
where.
Also document what operators now have to do rather than leaving them to
discover it:
- GITLAWB_P2P_KEY must name a directory, since a bare filename is refused at
startup.
- The PeerId rotates once on the first start after upgrading, so a
GITLAWB_P2P_BOOTSTRAP multiaddr pinning a peer's old id with a /p2p/<PeerId>
suffix needs updating or dropping. Suffix-less addresses and the HTTP seed
list are unaffected.
- If the node reports tightening a loose key directory, the key that was in it
should be treated as possibly exposed and deleted so a fresh one is
generated.
* fix(review): close a `..` escape from the p2p key-path check
Two reviewers found the same hole independently: the check rejected a path
naming no directory, but a relative parent that walks back out through `..`
named one and still landed in the working directory. `a/../p2p.key` and
`./keys/../p2p.key` resolve to the cwd itself and `../p2p.key` resolves above
it, so all three put the key exactly where the check exists to keep it out of,
and had ensure_key_dir chmod that directory to 0700 on the way. Verified by
running the paths through the predicate and printing where each parent lands.
The rule is now that a relative key path must name a directory and must not
walk back out: at least one Normal component, no ParentDir. `..` inside an
absolute path stays accepted, since it cannot depend on where the process
started. The predicate moves into names_no_usable_directory next to key_parent,
and the config gate and the load_or_create_p2p_keypair backstop both call it, so
they cannot drift apart.
Also fixes two smaller gaps found in the same pass:
- The permission fixture could report "1 passed" while asserting nothing. Its
env gate returns early, and an early return is a passing test, so a renamed
variable would look green. It now prints a sentinel after its assertions and
the parent requires it. Confirmed by pointing the child at a different
variable and watching the parent fail.
- A GITLAWB_P2P_KEY starting with `~/` is refused when no home directory
resolves, instead of creating a literal `~` directory relative to wherever
the node happened to start.
The backstop had no test, so it has one now, along with a both-directions test
for the predicate. That test cleans up after itself: with the guard removed it
really does write a key next to the source, which broke a later run once.
* fix(review): reject `..` in an absolute p2p key path too
The previous commit closed this for relative paths and exempted absolute ones,
reasoning that an absolute path cannot depend on the working directory. That is
true and it is not the hazard. `key_parent` hands `ensure_key_dir` the lexical
parent, so `/data/keys/../p2p.key` chmods `/data` rather than the `keys`
directory the path appears to name, and `/data/../p2p.key` run as root would
try to tighten `/` to 0700. The exemption also had a test asserting the first of
those was fine, so the gap was written down as intended behaviour.
`..` is now rejected wherever it appears. An absolute path's root counts as
naming a directory, so `/p2p.key` still validates and `/data/keys/p2p.key` is
unaffected.
Found by a second-model review pass after the in-process reviewers had cleared
the relative half.
* fix(node): scrub the serialized p2p private key from memory
Two buffers held the private key in its protobuf form and dropped without
scrubbing: the encoding produced when a new identity is generated, and the file
contents read back on every subsequent start. Both are now Zeroizing, matching
what gitlawb-core already does for its own key material.
Scope worth being honest about: this scrubs our copies of the serialized form,
not the libp2p Keypair itself, which owns the secret for the process lifetime
and exposes no way to zeroize it. The gain is that the encoded bytes do not
outlive the write and the read.
zeroize was already in the tree through gitlawb-core, so this promotes it to a
direct dependency of gitlawb-node and adds no packages; the lockfile change is
the one line recording that.
* fix(node): validate the p2p key path before securing its parent
Reject filesystem-root parents, directory-valued paths, and symlinks before
any directory is created or chmodded. Create missing ancestors at the ambient
mode and pin only the nominated key directory to 0700. Refuse symlink reads
with O_NOFOLLOW, propagate directory durability sync failures, and document
the loose-key rejection path for operators.
* fix(node): satisfy clippy single-element-loop on p2p path test
* fix(node): tighten the p2p key storage contract before mutation
Anchor parent-directory checks on symlink_metadata, reject ~/ escapes and
non-regular existing keys with a bounded read, treat directory AlreadyExists
as a first-boot race, and skip key-path validation when P2P is disabled.
* fix(node): refuse foreign-owned p2p key paths and cap reads on all platforms
Check directory and key ownership before chmod or load, walk ancestors for
foreign control, bound non-Unix reads to MAX_P2P_KEY_BYTES, and gate unix-only
fixture tests without panicking on other targets.
* fix(node): hold the p2p key directory open across its trust checks
Resolve the key path once, then do every check and mutation through an
O_DIRECTORY|O_NOFOLLOW handle on the parent (openat/fstat/fchmod/linkat),
so nothing can be swapped between the ownership check, the tighten, and
the key open. Key reads go through one bounded, non-blocking loader on
every platform; new keys are generated in an unlinked scratch file and
published atomically, with concurrent first boots converging on the
winner's identity. The storage contract is indexed by two matrix tests:
filesystem shapes in p2p (each refusal proven side-effect free against a
recursive snapshot) and path spellings plus the p2p-disabled guarantee
in config.
* fix(node): walk the p2p key ancestor chain descriptor-anchored
Replace the path-following metadata walk (foreign_ancestor_error) and the
post-walk create_dir_all gap with a single descriptor-anchored walk that
verifies and creates the chain together. Missing components are created at
0700 relative to the verified parent and re-verified; AlreadyExists is a race
the winner must pass. Non-unix builds keep create_dir_all.
* fix(node): refuse group-writable and non-directory p2p key ancestors
The ancestor walk now refuses any component with group or world write bits
unless sticky, and any intermediate component that is a symlink or another
object type. Tests drive the walk through real boots: 0770/0775 ancestors
refused before any key IO, 0755 and 1777-sticky ancestors accepted with stable
reload, and an intermediate symlink refused with its target untouched. Both
guards proven load-bearing by neuter revert-checks.
* fix(node): verify the cwd anchor for relative p2p key paths
A bare-relative key path is now walked from a verified working directory:
the cwd is opened as '.' with O_NOFOLLOW|O_DIRECTORY, fstat-verified for
ownership and write bits before any relative component is touched, and a
writable or foreign-owned cwd is refused before anything is created. The
refusal names the working directory path. Child-process fixtures prove both
directions without mutating the shared process cwd.
* test(node): stop rendering uids in p2p assert-message strings
CodeQL flags the uid-interpolating assert messages as cleartext logging to a
console sink. Every assert message in the p2p test module now avoids
interpolating uid values or the uid-bearing error text, while the substantive
assertions (the refusal names both uids, the ownership check fires before the
chmod) stay intact and are verified by the same assertions.
* test(node): add walk rows to the p2p key-storage contract matrix
The matrix now proves the ancestor-walk refusals table-driven: a
group-writable (0775) ancestor, a world-writable non-sticky (0777) ancestor,
and an intermediate symlink component are each refused with the no-mutation
snapshot discipline, while the existing boot rows still reload to a stable
PeerId. The fchmod-on-create path pins newly created ancestors to 0700
regardless of the ambient umask.
* docs(node): note the verified working directory for relative p2p key paths
README, .env.example, and the clap help for GITLAWB_P2P_KEY now state that a
relative key path is anchored at a verified working directory, and that an
unsafe cwd or ancestor is refused with p2p off while HTTP stays up. Also
normalizes two pre-existing em dashes in the README settings table.
* fix(node): pin and verify the mode of every key object this process creates
POSIX applies the process umask to every requested creation mode, so the
0700 passed to mkdirat and the 0600 passed to openat were requests rather
than results. Under a mask that strips owner bits the created directory
landed 0000 and the no-follow reopen failed EACCES before the repairing
fchmod was reached, so that half failed loudly. The scratch key was worse:
it was published unreadable, the boot that created it succeeded on its
already-open descriptor, and only the next boot lost p2p, behind a health
check that still reported healthy.
Every object this process creates now goes through one rule: create, pin
the mode on the object just created, reopen, and verify the achieved mode
by fstat. The directory pin is issued by name off the verified parent
before the reopen, because a directory that landed 0000 cannot be opened
at all, so a pin that waits for the reopen is unreachable in exactly the
case it exists for. A race winner is verified as-is and never chmodded,
which is the rule the ancestor walk already applied.
Pinned's field is private to the pin module and no constructor is
exported, so a descriptor that skipped the pin cannot be turned into a
key-directory handle or reach publication. That is what keeps the rule
from depending on review.
Two adjacent corrections the same defect exposed. The leaf mode predicate
was supersets-only: mode & 0o077 != 0 asks whether anything is granted
beyond the owner, so 0000, 0100 and 0400 all passed it while being
unusable, and an inherited setgid bit was judged on the wrong bit width. A
loose directory is still tightened; an over-closed one is now refused with
its remedy rather than widened, matching how an over-closed key file is
already handled. And an unreadable key or key directory now names its own
cause: every open failure previously reported a refused symlink, which is
what an operator saw for a key masked at creation.
The matrix runs first boot and fresh-process reload across umask 0000,
0022 and 0777, crossed with missing and existing ancestors and leaf, with
concurrent creators and an injected write failure. It runs in child
processes because umask is process global, and it requires at least one
successful concurrent creator so a row where every creator fails cannot
pass. All 21 rows were RED under umask 0777 before this change.
* fix(node): create and publish the node identity key through the pinned path
The node identity PEM had its own storage flow: create_dir_all with no
mode, then write, then set_permissions. That is the exists-then-write-then-
chmod sequence INV-23 prohibits, and it sits in the same ~/.gitlawb the p2p
key uses, so the umask defect the previous commit fixed for one key was
still live for the other.
Measured on the current code, as a non-root uid, all three RED:
umask 0000 directory landed 0777, world-writable, holding the key
umask 0022 directory landed 0755, world-traversable
umask 0777 directory landed 0000 and the write failed with EACCES
The third is the one that mattered most: load_or_create_keypair runs before
the listener binds, so the node exited there and no p2p code was reached at
all. The umask-independence guarantee could not be demonstrated on the
shipped default while this stood.
The directory is now created through the same pin helper, at a verified
0700, and the PEM is published through the same scratch-then-link path at a
verified 0600. An existing directory that grants access beyond the owner is
tightened, which closes the other half of the same gap: a 0600 key inside a
0755 directory is still replaceable by anyone who can write that directory.
An existing key is loaded untouched and never chmodded.
Deliberately not the full ensure_key_dir. That carries the ancestor trust
walk, and importing its refusals onto a path that never had them would turn
an unsafe but currently booting deployment into a boot failure on upgrade.
Only the immediate parent goes through the pin helper, which verifies the
grandparent it is about to chmod a child of; ancestors above that keep the
existing create_dir_all behavior. The one new refusal is a group or world
writable non-sticky grandparent, which is the case where another local user
can replace the node's identity outright.
* fix(node): split p2p key config validation from live storage validation
The key-path validator answered two different questions and its callers
disagreed about what to do with the answer. Bare names, `..`, a trailing
separator and the filesystem root are properties of the configured value,
and Config::validate refuses them before the listener binds. The same
function also stat'd the key path and its parent, so a symlinked parent, a
non-directory parent or an unreadable parent exited the node as invalid
configuration, while the identical class of fault found one layer later in
load_or_create_p2p_keypair only logged and left HTTP serving.
That split was invisible from the outside and the docs described only one
half of it. Measured against the binary before this change: a symlinked
parent, a regular-file parent and an unreadable parent all exited 1 before
bind, which is the opposite of what README and .env.example promise.
Validation is now lexical only, behind a P2pKeyConfigError so the two
domains cannot be confused at a call site. Every live storage fact is left
to the load path, which already re-establishes each one on a descriptor it
opened rather than a pathname it stat'd: O_NOFOLLOW on the key open, a
regular-file check by fstat, and ELOOP or ENOTDIR at the leaf. The verdicts
are unchanged; what changes is that one policy now decides all of them.
Two supporting changes. The whole p2p port gate moves ahead of the database
connect, both arms together, because connect_db_with_retry retries forever
and left the disabled arm unreachable without a database; only p2p::start
still needs the pool. And a failed key load is now logged at error with a
stable event name and mirrored into a gauge, because the policy this commit
settles on is to keep serving HTTP with a green health check while the node
is off the p2p network, and that is only defensible if the outage is
visible to something other than a human reading startup logs.
A new integration test drives the real binary with no database and proves
both domains at the process boundary: nine lexical spellings exit before
binding, ten storage faults serve HTTP with the failure logged, and
GITLAWB_P2P_PORT=0 reaches its disabled log without touching the key tree.
Every row asserts a before/after snapshot, because a refusal that mutates
storage on its way out is its own defect and a returned error cannot show
it. Three of those rows were RED before this change.
A symlinked key directory also reports as a symlink again. Linux returns
ENOTDIR rather than ELOOP when O_NOFOLLOW meets O_DIRECTORY, so the errno
alone cannot separate a symlink from a regular file and the message had
regressed to naming the wrong cause.
* docs(node): state the two-domain key-path policy and how to see a degrade
README and .env.example promised that p2p stays off while HTTP keeps
serving when the key cannot be loaded, including for an unsafe ancestor or
working directory. Measured against the binary, that was false for a
symlinked parent, a non-directory parent and an unreadable parent, each of
which exited before binding. The preceding commit made the code match the
promise; this makes the promise precise.
All three operator surfaces now name the split the same way. A value that
cannot name a securable key file is refused before the node binds. Anything
wrong with the storage itself degrades, and the sentence says how to notice:
the node logs p2p_identity_key_load_failed at error and sets the matching
metric, /health stays green in that state, and fixing the storage and
restarting restores the same PeerId. A node quietly off the network is the
cost this policy accepts, so the docs point at the signal rather than
leaving an operator to find it in startup logs.
Two corrections while here. The key directory is tightened only when it
grants access beyond its owner; one closed too far is refused with a
chmod 700 remedy rather than widened, so the sentence no longer implies the
node will open up a directory an operator locked down. And the existing
advice to delete a key after a tightening warning is now scoped to the
loosening case it was written for: an over-closed directory is refused, not
tightened, and nothing in it was ever exposed, so following that advice
would have meant deleting a safe key and taking a second PeerId rotation
that breaks every pinned bootstrap multiaddr.
* test(node): pin the degrade signal and wait for both startup lines
Two gaps in the boundary test, both found by extending the mutation spec to
cover this commit's own guards.
The degrade rows asserted only the prose message, so renaming the event
field or dropping the level would have passed while every alert built on
the operator docs broke and the node still looked healthy. The rows now
pin the error level and the p2p_identity_key_load_failed name the docs tell
operators to watch.
The rows also waited on a single log line, which is a race: the degraded
server logs "ready" from a spawned task while the p2p gate logs its verdict
from the main task, so whichever the reader stopped at first left the other
uncaptured. It passed when the test binary ran directly and when the row ran
alone, and failed only once other work shifted the scheduling. Reads now
wait for every needle before returning.
The fix stays on the test side on purpose. Logging the p2p verdict before
the degraded server is spawned would also make the order deterministic, but
that is production sequencing changed for a test's convenience.
* fix(node): keep key publication transactional and path-policy split
Bare GITLAWB_KEY filenames publish into the working directory without
chmodding it. A directory this process created is removed if pin or
verify fails. Ancestor walks use search descriptors so a safe 0111
component is traversable. Scratch unlink is part of publish success and
is fsynced. Rotation advice is limited to writable or readable exposure,
not ordinary 0755 tightening.
* fix(node): refuse a writable cwd for a bare identity key
Publishing identity.pem into cwd must not chmod cwd, but a 0600 key is still replaceable if that directory is group or world writable. Apply the same trusted-parent check used for nominated key directories before the handle is returned.
* fix(node): load identity without following pathnames
Create already published through a directory handle. Load still used exists-then-read, which followed a symlink at the key and opened the grandparent by pathname. Open both no-follow, treat /identity.pem like a cwd publish so it reaches the filesystem, and open a search-only grandparent with the walk flags.
* fix(node): parse key paths, leave identity parents, repair special modes
A terminal `.` in GITLAWB_P2P_KEY was accepted then retargeted by Path onto
the parent directory. Identity creation imported p2p's 0700 pin onto any
existing named parent. A setgid-only 2700 key directory skipped repair and
disabled P2P. Darwin ancestor walks still required directory-list permission.
* fix(node): type-check the non-unix p2p scratch writer
The shared scratch-then-link path names pin::Pinned on every target, but
the pin module was unix-only, so gitlawb-node did not compile off Unix.
* fix(node): refuse identity path spellings that Path would retarget
GITLAWB_KEY had the same terminal-dot hole as GITLAWB_P2P_KEY, and its
tilde expansion did not refuse a suffix that escaped home. Bare filenames
stay legal.
* fix(node): create every missing identity parent on one pinned walk
The identity key path split one creation transition across two policies.
`create_dir_all` built every ancestor above the immediate parent at the
ambient umask, then `create_dir_pinned_at` ran `verify_trusted_parent` on
the last of those and refused it. Under umask 0002 that ancestor lands
0775, so the phase that made it handed it to the phase that rejects it:
the node exits before binding and the failed boot leaves the directory
behind at 0775, so the next attempt starts from state the first one made.
Replace the split with one descriptor-anchored walk. Pass 1 walks up from
the key's parent until a component opens; that descriptor is the anchor, and
it is the same fd the next step creates into. Pass 2 walks back down calling
`create_dir_pinned_at` per component, each one pinned 0700 and verified at
its achieved mode before it becomes the parent of the next, so from the
anchor down no pathname is resolved twice. Pre-existing ancestors are
adopted as-is and never chmodded, since GITLAWB_KEY is a file path rather
than a dedicated-directory setting; write-authority is still required on the
directory each component is created in.
A boot that fails past the walk now removes exactly what it created,
deepest first, through unlinkat with AT_REMOVEDIR against the descriptor
each entry was made in. A race-adopted directory is never in that list, and
AT_REMOVEDIR refuses a non-empty directory, so a leaf another boot has
published into survives. The accumulator is an out-parameter so a mid-walk
failure and a post-walk failure reach the same single rollback arm.
Two consequences worth naming. A symlink at the deepest existing ancestor is
refused when the missing suffix reaches it, where `create_dir_all` followed
it. Ancestors ABOVE that anchor are still resolved by pathname and their
symlinks still followed, unchanged from before this walk, because O_NOFOLLOW
binds only the final component of a path-based open; closing that would mean
an openat chain from the root and the full ancestor policy that GITLAWB_KEY
paths deliberately do not get. The doc comment now says so rather than
claiming more than the code does. And a losing concurrent first boot can
remove an intermediate another boot adopted but has not yet written into,
turning that boot's start into an ENOENT failure; both still fail closed.
Also replace the root-adjacent test. It proved a pathname property by
running the full mutating path against the real filesystem root and
assuming the runner lacks permission. Under euid 0, common in build
containers, `/` is accepted (root-owned, 0755, no group or other write
bit), the production path returns Ok, the test's panic arm fires before its
cleanup assertion, and a real private key is left at /identity.pem, after
which the leading exists() check makes every later run a silent skip. The
routing is now asserted lexically through a named predicate, with no
filesystem access and no dependence on euid.
The matrix covers one, two and three missing levels under umasks 0000,
0002, 0022 and 0777, a pre-existing 0755 ancestor that must stay 0755, a
relative multi-level path, a symlinked deepest ancestor, refusal without
partial mutation, and a populated leaf whose removal must be refused and
reported. Created-directory modes are asserted by exact equality, and each
success row reloads in a fresh process and requires the same DID.
* test(node): race real concurrent first boots of the identity key
The rollback added with the pinned walk can remove an intermediate that a
concurrent boot has already adopted, and until now that was reasoned about
rather than executed. Nothing in the suite started two real first boots
against the same key path.
Add a `concurrent-first-boot` layout to the self-exec fixture and a driver
that races several children through `load_or_create_keypair_at` on one
missing multi-level path. A two-phase file rendezvous puts every child
inside a spin loop before any of them is released, so the spread across the
racing call is one loop iteration rather than process-startup jitter. Three
arms: unaided boots, one injected write failure against one unaided boot,
and four injected failures against one, which is what actually opens the
window.
The assertions are properties of every legal outcome, never one
interleaving, so the test is deterministic even though the race is not. A
boot that claims success has a parseable 0600 key on disk, all successes
agree on the DID, surviving directories are exactly 0700 with no scratch
residue, and if no key was published then nothing claimed success and a
retry must still work. Observation counters are printed, not asserted, so a
run that never hits the window says so rather than passing as proof.
Two things the race showed that reading could not. The documented window is
real: a boot was refused with "No such file or directory" on its publish
after a loser rolled back the leaf it had adopted. But the more common shape
is one hop earlier, the loser removing an adopted intermediate so the ENOENT
lands on the next component's creation instead. And under enough concurrent
rollbacks every boot can fail, not merely the losers, so several nodes
starting against a shared first-boot path can all need a restart. Two-child
racing never opened the window at all in 500 iterations.
* fix(node): resolve identity key path components one at a time
`O_NOFOLLOW` binds only the final component of a path-based `open(2)`, which
is all the kernel offers for a multi-component pathname. Three sites resolved
a whole configured path in one open and so guarded only the last position:
the load path, the existing-named-parent fast path, and the walk that creates
missing parents. Every interior symlink was followed, and `verify_trusted_parent`
then attested the directory that was reached rather than the path taken to it.
That is enough to substitute the node's whole identity, not merely misplace
its key. Load and create resolve the configured pathname identically, so
repointing one interior link redirects both. Point it at a directory holding
another PEM and the node boots as that DID while its real key sits untouched
on disk and nothing is logged as wrong. Point it at an empty directory and
the node mints a fresh identity and reports a first boot. Neither needs the
attacker to read anything: ownership and mode checks still pass, because they
run on the object at the end of the path.
Resolve each component with `openat` and `O_NOFOLLOW` against the descriptor
of the one before it, from `/` for an absolute path or the working directory
for a relative one, so a symlink is refused at every position. Interior
components open search-only and the final component keeps the caller's flags,
so a legitimate 0111 ancestor still resolves and the leaf descriptor is still
the one that gets fsynced and chmodded.
Components are adopted with no ownership or mode judgment. The only new
refusal is a symlink on the key path: a shared 0775 volume, a foreign-owned
ancestor and `/etc` all keep working, because `GITLAWB_KEY` is a file path
rather than a dedicated-directory setting. `verify_trusted_parent` stays
exactly where it was, on the descriptor a child is created in. The p2p key
path is untouched; it already walked component by component.
This is pre-existing rather than a regression: the same probe against the
branch point produces the same result. It is fixed here because closing it
replaces the walk this branch just added, so splitting it would mean
reviewing that code twice.
Covered by rows for an interior symlink with the next level present, an
absolute target escaping the base, a two-link chain, the fast path, a
relative configured path, and both load-path variants asserting on the DID
the node ends up presenting rather than on a mode or an error string. The
cases that were already refused are pinned too, including a symlink at the
immediate parent and a dangling link, neither of which had a test before.
---------
Co-authored-by: Kevin Codex <[email protected]>
|
||
|
beardthelion
|
8a98cab243 |
test(node): cover the component walk in validated_repo_disk_path (#411) (#441)
* test(node): cover the component walk in validated_repo_disk_path * chore: re-trigger code quality check |
||
|
JATMNandClaude Opus 5.5
|
623b600835 |
fix(deps): bump libp2p to pick up hickory 0.26 and serde_with 3.22 (#518)
hickory-proto/hickory-resolver 0.25.2 were pulled in only by libp2p-dns 0.44. libp2p-dns 0.45 moves to hickory 0.26 but needs libp2p-core 0.44 and libp2p-identity 0.3, so the node's direct libp2p crates move to the matching release line (core 0.44, gossipsub 0.50.1, identify 0.48, identity 0.3, kad 0.49, quic 0.14, swarm 0.48, dns 0.45). multiaddr goes to 0.19 to match libp2p-core and avoid compiling two copies. No source changes were needed. Lockfile: hickory-proto/resolver 0.25.2 -> 0.26.3 (GHSA-q2qq-hmj6-3wpp / RUSTSEC-2026-0119, GHSA-6f2x-v7q7-m7m5), serde_with 3.18.0 -> 3.22.0 (GHSA-7gcf-g7xr-8hxj). With the fix in, drop the RUSTSEC-2026-0118/0119 ignores from .cargo/audit.toml as #76 requires. Closes #76 Co-authored-by: Claude Opus 5.5 <[email protected]> |
||
|
beardthelionandKevin Codex
|
9bda2e103c |
fix(node): authenticate gossip ref-update events before writing them (#325)
* feat(node): sign and verify gossip ref-update events Add an optional sig field to RefUpdateEvent carrying a base64 Ed25519 signature over the event's own fields, plus signing_bytes, the single producer of signing input on both sides, and sign/verify helpers. The field is optional and skipped when absent, following the owner_did precedent from #144, so an event from a signing node still deserializes on a peer that predates this change. A nested envelope would have broken that and is deliberately not used. Verification resolves the key from the claimed node_did, so a signature that does not bind the DID it claims is refused. Non-did:key and unresolvable DIDs are refused with the peers-table denial sentences, built from PeerWriteDenied rather than retyped so the two surfaces cannot drift apart. A golden test pins the signing input: a field reorder changes what every peer signs, which no round-trip test can see. Helpers are not wired into the ingest or emit paths yet. * fix(node): authenticate gossip ref-update events before writing them The gossipsub ingest path accepted a ref-update from any peer and wrote it, while the HTTP route carrying the same payload checks the signature against the claimed DID, requires a known peer, and validates the repo slug. Extract the path into ingest_ref_update and apply the same three gates in that order, so both transports answer the same input the same way. A present signature must verify in either mode. An absent one is refused when GITLAWB_REQUIRE_SIGNED_PEER_WRITES is set and otherwise accepted with a warning naming the flag, so peers that predate signing keep federating until the fleet is ready. The DID-method refusal runs right after parse in both modes and reuses the peers-table denial sentences. Rejection writes nothing: neither a ref-update row nor a sync-queue entry. Those are separate sinks and the tests assert them separately. from_peer's comment now says it records the forwarder, not the author. * fix(node): bound gossip ingest per source and sign outbound events Brake the ingest path at 60 events per minute per forwarding peer, checked ahead of the parse and the signature work. Verifying a signature is the expensive step, so a brake behind it would sell an unauthenticated flood the CPU the brake exists to protect. Rejection writes nothing to either sink. Sign events on the way out, so a fleet running with enforcement on can verify what its peers publish. A signing failure warns and skips the publish rather than emitting something a verifying peer would drop. The emit-to-ingest round trip is asserted end to end, which is what proves both sides agree on the signing input by execution rather than by inspection. * test(node): cover the did-method gate and correct overclaiming comments The did-method gate had no load-bearing test: deleting it left the suite green, because the only non-did:key case was signed, so verification resolved the DID internally and returned the same refusal. Add the case only the gate decides, an unsigned non-did:key event from a registered peer with enforcement off, and correct the comment that claimed the other test covered it. Assert the limiter's source bound, which no test pinned, and make the backward-compat test able to see an added field by parsing under a deny_unknown_fields reader as well as a permissive one. Say plainly at the peers check that authentication is not authorization: a self-registered did:key passes it, so what the gate buys is blocking an unregistered DID and, with the signature, blocking impersonation of an existing peer. Drop the claim that the unsigned-acceptance warning is covered by a test, because it is not. An outcome of Accepted now means the row was stored; a failed write reports itself instead of reading as success. * fix(node): charge the ingest budget to the author, not the mesh edge A single 60-per-forwarder budget was both too tight and pointed at the wrong principal. Pushes emit one gossip event per ref, so a push touching more than 60 refs lost its tail with no signal to the publisher, and because gossipsub re-shares mesh-wide, a flood routed through an honest neighbour spent that neighbour's budget on every receiver. Split it. A loose pre-parse bound on the forwarding peer still caps the CPU spent before anything is parsed or verified, which is all a free-to-mint identity can honestly buy. A tighter bound on the authenticated node_did sits after verification and the peer gate, so the two durable writes are charged to whoever actually authored them, and it is sized to admit a large push whole. Both rejections now name the budget and its window, so a dropped ref-update says which bound refused it rather than vanishing. Replace the per-event full peers scan with a keyed lookup on the same column; upsert_peer shares it. A push above the author bound still loses its tail, now loudly. The two transports continue to disagree about events per push: gossip publishes per ref while sync/notify batches. * docs(node): the did-method gate runs after the parse, not before it Two test comments called it the pre-parse did-method gate. The parse is above it and the gate is below; what is true is that it runs before the signature branch, in both enforcement modes. The pre-parse label belongs to the forwarder brake, which is genuinely the first statement, and the nearby correct usage is where the wrong one came from. * fix(node): charge the gossip write budget only to a proven author An unsigned ref-update event's node_did is asserted, not proven. The author budget was debited on that key in both signature arms, so a flood of unsigned events naming a registered peer drained that peer's budget and its own genuine signed updates were shed. The attacker chose the victim. The author budget is now charged only when the signature verified. Unsigned compatibility traffic charges a separate forwarder-keyed bucket sized at GOSSIP_UNSIGNED_SOURCE_MAX_EVENTS (1500), above the per-author cap because a forwarder aggregates many authors, and below the pre-parse brake; the compile-time assert now pins all three in one chain. The shed carries its own IngestOutcome variant so a forwarder is never named as an author. validate_repo_slug moves above both the signature match and peer_exists, so a malformed event costs no database round trip rather than merely being charged to nobody. A test-only peer_exists tally proves that placement, which an outcome assertion cannot distinguish. * fix(node): version the signed ref-update format and refuse unknown versions The signed payload carried no version and verify_ref_update knew one representation, so the next signed-field addition would partition the mesh mid-rollout: an upgraded publisher signs bytes an older receiver cannot reproduce, and the older receiver reports a genuine event as a bad signature. RefUpdateEvent now carries v, skipped when zero, so a v0 event's wire and signing bytes stay byte-identical to the versionless form. GOLDEN_SIGNING_BYTES is unchanged and every signature already issued still verifies. A frozen artifact signed before the field existed is checked into the suite and verified without being re-signed, since a signature a test just produced is self-consistent by construction and proves nothing about compatibility. An event whose version this build cannot interpret is now refused in its own words, ahead of every other gate: no field below an unknown version has known meaning. Without that, a v1 event was admitted outright rather than merely mislabelled. * docs(node): describe the real signing handoff at the publish site The comment claimed signing was future work and the event went out unsigned. It does not: the swarm loop signs via p2p::signed_publish_bytes immediately before publishing, and skips the publish entirely when signing fails. Leaving sig None here is correct, and the comment now says why rather than describing a state the code left behind. * docs: GITLAWB_REQUIRE_SIGNED_PEER_WRITES spans both transports The flag gates gossip ref-update ingest as well as the HTTP announce and sync routes, but README and .env.example still scoped it to those two routes. An operator who upgraded the HTTP peers and then enabled it would silently drop gossip from every publisher still running an old build, with no error visible on the publisher's side because gossip carries no response. Documents the cross-transport behavior and the one ordering requirement that follows: upgrade every gossip publisher before enabling. * fix(review): count gossip ingest outcomes and sweep the ingest limiters Every IngestOutcome reached tracing and nothing else, so gossip-ingest health was invisible on /metrics, the only surface this daemon exposes to a dashboard or an alert. Adds gitlawb_gossip_ingest_events_total{outcome} alongside the existing sync-queue and webhook counters, with the label derived from an exhaustive match so a seventh variant fails to compile rather than shipping uncounted. The reason strings stay in the log line; they are sender-shaped and would make the label set unbounded. The three ingest limiters were built as locals in the swarm task, so sweep_rate_limiters could not reach them and their keys stayed resident until the 200k ceiling. The swarm loop now sweeps them on its own interval, and cleanup iterates a destructured array so a fourth limiter breaks the build instead of being silently missed, which is the failure sweep_rate_limiters' own comment records against the /ipfs limiter. * test(node): close four gaps the review found in this PR's own guards The frozen signing constants pinned only the all-Some optional shape, but the sole publish site always emits cert_id None, so no constant covered the encoding every real event uses. Proven during review: adding skip_serializing_if to cert_id left the suite green while the signing input changed for 100% of production events. A second constant now pins the null-carrying form. The legacy artifact's provenance assertion claimed re-capturing it from current code was what the assertion refused. It was not: skip-when-zero means a regenerated artifact also carries no version key, so the check passed and the claim was unfounded. A SHA-256 over the frozen bytes does the job the message promised, and the message now states only what it checks. WriteFailed was the one outcome variant no test constructed, so the independent-writes property it exists to express was unexercised. Both directions are now driven by failing a real write. The publish arm's body is extracted so the exact bytes handed to gossipsub are asserted signed and accepted by ingest under enforcement. The select! dispatch and the require_signed threading from main.rs still need a live swarm and are named as uncovered rather than implied covered. * fix(review): close the remaining findings from the round on this PR The unsigned-acceptance warning fired before two gates that could still drop the event, so a message the operator reads as an admission could be followed by a forwarder-budget shed or an unknown-peer refusal. It now fires only once the event is genuinely admitted, and a test pins that it fires exactly once, which nothing checked before. The publish site hardcoded the format version while the ingest gate compared against the constant, so emitter and gate could drift apart silently. The publish site now reads the constant. The known-peer gate changed a federation precondition without documenting it: a gossip publisher must also be a known peer on the receiving node, via bootstrap or a prior announce, or its events are dropped in both flag modes. That is now in the README beside the rollout ordering, derived from what the ingest path does rather than from another docstring. Six tests each hand-rolled the same both-modes rejection loop; they now share one helper. The log-capture helper was left under a tmp_ prefix from the round that added it and is renamed, since scaffolding vocabulary should not ship. * fix(review): report unsigned gossip admissions as their own outcome and metric (#325) An unsigned event that survives the rolling-upgrade window was returned as IngestOutcome::Accepted, so `gitlawb_gossip_ingest_events_total{outcome="accepted"}` mixed signature-verified admissions with unauthenticated legacy traffic. An operator could not tell whether the fleet still relied on the compatibility allowance or authenticated gossip was healthy. Add IngestOutcome::UnsignedAdmitted with its own `unsigned_admitted` metric label, returned when an unsigned event passes every gate and the writes land. `Accepted` is now reserved for signature-verified events. Propagate through the swarm-loop logging and the tests that asserted `Accepted` for unsigned admissions. Both guards proven load-bearing by execution: reverting the return-site change goes RED on flag_off_unsigned_known_peer_event_is_accepted ("got Accepted"), and giving UnsignedAdmitted the "accepted" label goes RED on every_ingest_outcome_carries_a_distinct_metric_label. * fix(node): shed an over-budget gossip author before the peer lookup (#325) The author brake sat below `db.peer_exists`, so a signed author already past its budget still paid a Postgres round trip before being refused. That is a brake behind the work it bounds: the lookup is done for a request that was never going to be admitted, and a signed author can drive it at the pre-parse source rate, further with peer-id rotation. The brake now answers first, and the answer is a READ rather than the charge. `check` inserts a window for a key it has never seen, so probing with it here would let a flood of self-minted signed DIDs occupy the bounded author map before `peer_exists` could refuse them, and legitimate new authors would be shed once it filled. That would have made the remedy worse than the finding. `is_over_budget` allocates nothing, reports an unseen key as within budget, and leaves the charge itself below the lookup, unmoved. The only behaviour that changes is WHEN an already-over-budget author is refused. Two guards, both proven load-bearing by execution. Removing the early shed makes `an_over_budget_author_is_shed_without_a_peer_lookup` red on the call counter, which observes the lookup not happening rather than restating the ordering. Swapping the read for `check` makes `the_early_author_check_does_not_track_an_unseen_did` red on the map size, which is the must-not that keeps the brake from becoming its own memory-fill surface. Found by a cross-family refute pass on the pushed head. * chore: retrigger checks after a CodeQL infrastructure failure The swift analysis job on run 32041989078 failed in "Set up job", before checkout or any analysis step. GitHub refuses a rerun on default-setup (dynamic) runs, so a new head is the only way to get a fresh scan. * fix(deps): bump h2 to 0.4.19 for RUSTSEC-2026-0258 The branch lockfile still pinned h2 0.4.13, so cargo audit failed on every push while main had already moved on. This is lockfile-only; no source change in the gossip auth work. * chore: re-trigger code quality check --------- Co-authored-by: Kevin Codex <[email protected]> |
||
|
beardthelion
|
65f5989a1d |
test(node): pin owner-push off in receive-pack lock tests
These lock-path proofs use non-owner proof DIDs. With enforce_owner_push defaulting on, they fail before receive-pack runs. |
||
|
Kevin Codex
|
73fd7473af |
Merge pull request #330 from Gitlawb/fix/default-enforce-owner-push
fix(node)!: enforce owner-only push by default |
||
|
beardthelion
|
76f630008f |
fix(node): never hand back a continuation that made no progress
On a resumed request whose visit budget was already spent by the provenance phase, the scan's top-of-loop visit arm sealed pager.cursor, which at that moment is the position the caller just sent. The node returned the caller's own token, verbatim, rung after rung. Three rungs were observed returning an identical position. A token looks like progress, so the client keeps going: gl retries to its resume cap, and every one of those requests re-runs the full provenance phase, up to seventeen repo acquires and cat-file subprocesses, advancing nothing before it errors. That is roughly nine anonymous requests worth of work for none, and it is worse than shedding nothing, because a caller who is told the ladder is over stops immediately. A seal now has to be strictly ahead of where the request itself started: a different candidate is ahead by construction, since only the gated advance can name one, and the same candidate needs a row past the start row. A request that started at the front is before everything, so its seals pass untouched. When the proposer settled at least one row this rung, the existing ceiling arm already seals that row, and it is strictly ahead because a resumed scan only walks rows past its cursor. Only a rung that settled nothing sheds without a token, and that is honest: the spender is the provenance phase, which runs the same way every rung, so no retry can do better. The filter sits at the single mint site, where a future call site cannot bypass it, and it logs the drop as a boolean. record_scan_truncation has already logged that a position was sealed by then, and a 503 carrying no token next to that line is the confusion that log exists to prevent. |
||
|
beardthelion
|
d0635d95b7 |
fix(node): resume the /ipfs scan per candidate so a starved one is not skipped
A CID that maps to several oids shared one resume slot across every candidate, and the slot kept the maximum position. An earlier candidate could spend the probe budget walking past a repo that holds the object for a later one, seal a position beyond it, and the next rung would resume past a row that candidate never examined, wrap, and shed tokenless. The client reads an absent token as the ladder being over, so the object became permanently unretrievable, at stock config, deterministically on every retry. The token now names which candidate it is resuming, and the rules that keep that sound are narrower than they first look: Only one candidate per request may seal, and which one depends on where the REQUEST started. On a resumed request it is the resumed candidate alone, since the shared pager holds only the table suffix from the caller's cursor, so a later candidate walked a suffix and never saw the front. On a front-started request it is the first unfinished candidate, since there every candidate walks from the front and a later candidate's stop is honest coverage. Silencing later candidates unconditionally would remove the only thing that mints rung 1 when the first candidate wraps untruncated. A candidate is finished when its row loop walked every fetched row, or when it owed no scan at all. Both matter: a properly provenanced candidate never wraps, so without the second arm the ladder dies every rung. The wrap is witnessed per candidate at the row loop's own two exits, never by reading the shared pager flag at the tail, which any short page sets and which would let a candidate that truncated mid-page look finished and strand the rows it refused. Finishing a non-final candidate advances the seal to the next one at a front sentinel and taints, because the tail emits a continuation only when something tainted; sealing without tainting would suppress the taint and return a definitive 404 while discarding the token it had just minted. The keep-the-maximum comparison is gone. With one proposer per request the slot is written at most once, so an assertion states that directly instead. The pager stays shared per request. A per-candidate pager would restore the fan-out the paging exists to remove. |
||
|
beardthelion
|
2656718916 |
feat(core): carry the resumed candidate's identity in the scan token
A CID can map to several git oids, and the ladder needs to name which one it is resuming. The sealed position gains the candidate's oid hex so a rung resumes that candidate rather than a position in a list: oids_for_cid is a sorted set, so an ordinal silently repoints at a different candidate when a pin that sorts earlier arrives between rungs, while an identity degrades safely to "not found, restart at the front". The field is length-prefixed and padded to 64, matching the framing the row fields already use, because production oids are 40 hex, not 64: repos are created with --object-format=sha1 and only the test fixtures are sha256. A fixed 64-byte field would fail every seal on a real deployment and shed a tokenless 503, which the client reads as the ladder being over. Both widths are exercised, and a zero-length candidate is rejected at decode so it cannot be confused with the front-of-table sentinel, which is empty row fields with a real candidate. VERSION goes to 3 and the plaintext to 527 bytes, so a token minted under the old layout opens to None and the caller restarts at the front. Nothing has minted one outside tests. The slot carrying the position is a struct rather than a widened tuple on purpose: a 3-tuple would have pulled the hex into the existing keep-the-maximum comparison, changing behavior this commit is meant to leave alone. Token length stays invariant across both oid widths, since length would otherwise be a side channel for the withheld row it names. That is asserted on a real seal in gitlawb-core, not on the gl fixtures: nothing in gl seals or opens a token, so its width constant cannot detect a wrong layout. |
||
|
beardthelion
|
366b599331 |
fix(node): order the CID candidate list so it cannot depend on heap order
oids_for_cid ran a bare SELECT with no ORDER BY, so Postgres was free to return the candidates in physical heap order. get_by_cid walks those candidates under one shared probe budget, visit budget and pager, so whichever comes back first is the one that spends the request's budget: two nodes holding identical data, or one node before and after an unrelated write, could resolve the same CID differently and one could shed a 503 where the other serves. The instability is not hypothetical. An unpin and re-pin of a single object, which is an ordinary production sequence, moves that row to the end of the heap and rotates the list. The sibling pin_sources_for_oid already orders its union for exactly this reason, and the handler comment next to it leans on that determinism. |
||
|
beardthelion
|
1bdefb18c2 |
chore(node): log every /ipfs scan truncation and correct the helper's docs
Code review of the previous commit turned up three things worth fixing in place. record_scan_truncation centralized the taint and the seal but logged nothing, so centralizing actually made a truncation less visible than the scattered inline taints it replaced: only the visit ceiling logged, and it logs from inside the gate, before the caller decides whether a position gets sealed. Two identical log lines could therefore mean "the ladder continues" or "the caller is stranded". One debug line now carries the reason and whether a position was sealed. It logs only whether one exists, never its value, since the position names a withheld row's created_at and id. The doc comment claimed to be "the one site that records a scan truncation" while eight other taint sites bypass it. The distinction is real but it is not the one the comment drew: a ceiling stops the scan and owes the caller a position, while a transient skip refuses one row and the rows behind it are still walked. Says that now. The forward-only rule's stated justification was wrong. A later candidate's position is still ahead of the token the caller echoed, so letting it win would not move the ladder backwards; it would shrink each rung toward a single row. Also records that the comparison is Rust byte order while the pager's keyset runs under the database collation, which can disagree on a non-C collation, and why that costs a replay rather than a skipped row. |
||
|
beardthelion
|
69cfbd4b3c |
fix(node): mint a continuation when a ceiling stops the /ipfs scan mid-page
The probe and visit ceilings taint inside gate_and_serve and returned Skip, so the row they refused and every row behind it were walked past without a verdict while the resume position was sealed from pager.cursor, the end of the fetched page. Two ways that stranded content: - On the final page, `pager.exhausted` breaks ahead of every mint arm, so the shed carried no continuation at all. A tokenless search_incomplete is the wrapped-scan answer, which tells `gl ipfs get` its ladder is over, so a holder on that page was unreachable on every retry. - Mid-page, the sealed cursor sat past the refused rows, so the resume skipped them. The shipped ladder tests all set page_rows == ceiling, which puts the break exactly on a page boundary and hides both. The ceiling now returns GateOutcome::CeilingStop rather than tainting on its way out, and the scan loop stops there and seals the row in front of the one that was refused. record_scan_truncation is the single site that records a truncation: it taints and seals together, and only ever moves the position forward, so a later oid candidate re-walking the same rows on a spent budget cannot hand back a token the caller already echoed. The wrapped-scan tail no longer clears a seal, since a ceiling can stop a resumed scan part way through the last page. |