fix(node): bound replay of authenticated gossip ref-update events - #334
Open
beardthelion wants to merge 124 commits into
Open
fix(node): bound replay of authenticated gossip ref-update events#334beardthelion wants to merge 124 commits into
beardthelion wants to merge 124 commits into
Conversation
…concurrency base Rebased onto #174 (fix/served-git-concurrency-cap). #173's incremental history cannot replay commit-by-commit because #174 rewrote the same handler files in parallel; this single commit carries the fully-integrated tree (identical to the verified merge result), with the follow-up fixes as separate commits on top. Brings in #173: GET /ipfs/{cid} CID->oid resolution via pinned_cids, per-caller path-scoped blob/tree/commit-tag gating (#135/#173 F1-F6), and the pin-source provenance table. Adapts #173's tree/commit-tag walks to #174's run_bounded_git so the held /ipfs walk-concurrency permit is duration-safe (F5).
P1-A: gate_and_serve held the /ipfs walk permit across a bare repo_store.acquire; wrap it in git_acquire_timeout_secs (mirroring #174's own handler) so a cold/hung Tigris acquire cannot pin the global walk slot. On expiry skip the repo and mark the search truncated (retryable 503, not a false 404). P1-B: the local-IPFS pin path recorded Kubo's provider Hash as pinned_cids.cid; for objects above the block size that is a dag-pb root that does not hash the raw content, so GET /ipfs/{cid} listed then 404'd them under the F2 integrity check. Record the locally-computed raw-content CID, mirroring the pinata twin.
…kfill pinata provenance P2-E: the object-type probe and the F6 size+read ran bare git cat-file inside spawn_blocking while the /ipfs walk permit was held, so a wedged cat-file could pin the global walk slot for the request's life. Bound both under git_service_timeout_secs; on timeout free the slot (truncated -> retryable 503) and skip the repo. P2-D: the pinata already-pinned branch now backfills NULL first-pinner provenance in lockstep with the ipfs_pin skip branch (consistency; the object was already resolvable via the pin_repo_sources union).
A provenanced object has a bounded source set (first-pinner + up to MAX_PIN_SOURCES additional). With the walk ceiling at 16 == MAX_PIN_SOURCES, an authorizing public source sorting after 16 path-scoped denials was never reached: the ceiling truncated the search and returned a false 503 for a readable object. Raise the ceiling to MAX_PIN_SOURCES + 1 so the whole bounded provenance set is always tried; the legacy scan stays bounded by MAX_LEGACY_PROBES_PER_REQUEST.
…chIncomplete - ipfs_pin::pin_new_objects returns the provider Hash (not the raw resolver key), matching the pinata twin's contract; the DB cid stays the raw content CID. The return is logging-only here, so this is drift-avoidance, not a functional fix. - AppError::SearchIncomplete now carries Retry-After: 1 like Overloaded, so both retryable 503s from the /ipfs handler advertise the retry hint consistently.
…n fallback
An attacker who pins a public object from MAX_PIN_SOURCES repos before the
legitimate public source registers fills pin_repo_sources (record_pin_source is
first-N-wins and drops later sources silently); the buried public source is then
never reached because a non-empty provenance set suppressed the legacy scan, so
anon GET /ipfs/{cid} 404s forever for a public object (re-breaks F1).
U1: db::pin_sources_at_cap reports whether pin_repo_sources is at MAX_PIN_SOURCES
for an oid (the only observable signal that a servable source may have been
dropped, since the write cap never overshoots).
U2: get_by_cid falls back to the bounded legacy scan on a provenance miss when the
set is empty OR at_cap. The scan gates every repo through the real per-caller gate,
so it finds the buried public copy. A complete (non-full) set still fast-404s, so
ordinary denials never fan out to O(repos) (INV-10/F3); the fallback honors the
is_throttled peek.
Regression ipfs_cid_buried_public_source_still_serves_via_scan_fallback: 404 before,
200 after; pin_sources_at_cap_flips_at_max covers the boundary.
The path-scoped upload-pack branch kept the read and per-caller permits as handler locals and called upload_pack_excluding with no AdmissionGuard, so a cancelled filtered clone released admission while KillGroupOnDrop was still reaping the git group, bypassing the read and per-caller concurrency caps. drive_git_child now returns the disarmed guard on success so one guard rides both build_filtered_pack stages (rev-list then pack-objects); the handler builds the guard from the permits as the plain path does. Adds a disconnect regression and an INV-22 gate row, both proven load-bearing.
get_by_cid held the global walk permit and the per-source permit as handler locals, so dropping the request future released both while the spawn_blocking probe, visibility walk, and object read kept running; a cancel-spam or timeout client could exceed MAX_CONCURRENT_IPFS_WALKS and the per-source cap, and the bare cat-file probe/read children had no teardown at all. Move the gated serve pipeline into a detached task that owns an AdmissionGuard, awaited by the handler, so admission releases only after the bounded work is gone; back the /ipfs probe and read with run_bounded_git twins for process-group teardown. Adds cancel-mid-walk, timeout-reap, and cancel-spam regressions plus two load-bearing INV-22 rows. Also reflows one U1 test line to satisfy cargo fmt.
Per-repo coalescing dropped every push that arrived while a repo's encryption task was in flight, and no reconciliation ever processed it, so a withheld blob added by the coalesced push had its recovery copy permanently absent. The drop also silently skipped the coalesced push's local-IPFS pin work. EncryptInflight becomes a dirty-flag map; the detached task loops, and the atomic check-and-clear sits at the task tail, unconditional on the has_path_scoped_rule gate and walk success (a public or rules-free repo would otherwise exit before it ran). Each requeue re-reads repo state fresh and re-enumerates the pin half through the fail-closed full scan, never bare list_all_objects, so a coalesced rule change is honored and no withheld or dangling object leaks. At-most-one-task-per-repo is preserved.
The knob was parsed and documented as the per-request /ipfs fan-out cap but never read in production: AppState seeded the legacy-probe budget from a fixed 256 constant, so setting the knob did nothing and an operator could still incur 256 acquire/probe operations per request. Seed ipfs_max_legacy_probes from the knob (default raised to 256 to preserve shipped behavior); leave the history-walk ceiling on its MAX_PIN_SOURCES+1 constant so a provenanced request is never truncated into a false 503. Updates the README and .env.example to the shipped behavior and adds cap-honored and plumbing tests.
The route middleware charged ipfs_rate_limiter once per request while the legacy scan and provenance walk charged the same bucket per probe/walk, so a one-probe request cost two tokens and GITLAWB_IPFS_RATE_LIMIT=1 admitted the request at the route then 429'd it at the pre-scan peek. Add a bounded ipfs_work_rate_limiter, move the in-scan, per-walk, and pre-scan charges onto it, and leave the route limiter as the pure once-per-request brake. The work-budget capacity derives from the route limit with a floor of one full legacy-probe budget per window (no new operator knob), so a default-config deep search never self-throttles. Adds it to the periodic sweep, updates all three AppState construction sites, and rewrites the contradictory limiter comments.
The three warn-only record sites in the detached post-push task dropped a transient DB failure silently, leaving a permanently incomplete pin-source set that makes the resolver 404 a valid public copy. Wrap them in a bounded retry (inserts are idempotent via ON CONFLICT DO NOTHING); on exhaustion the warn still fires so behavior degrades to today's. Documents the residual crash/outage window that only a reconciliation sweep retires.
Before this PR the pin path stored the provider CID (Kubo dag-pb / Pinata) in pinned_cids.cid, which the new resolver recomputes-and-404s, while /api/v1/ipfs/pins still advertises the stale value. Repair a legacy row in the already-pinned skip branch: gate on a cheap stored-CID codec check (only a non-raw-codec CID reads bytes, so raw rows keep the DB-only skip cost), recompute the raw CID, and rewrite it while stashing the old value in a new v14 legacy_provider_cid column (distinct from pinata_cid, which gates the Pinata pin-skip). Rows whose bytes are gone stay withheld. Documents that the deferred one-shot sweep, not this opportunistic path, fully retires the window.
Code review found each candidate's size and content reads were each granted a full git_service_timeout, so a single served candidate could hold the /ipfs walk permit for 2x the timeout. Share one deadline across both stages (mirrors build_filtered_pack) so the read holds admission for one timeout total, and correct the admission-bound comment: the worst case is O(candidates x timeout) bounded by the walk concurrency cap, not a single deadline.
… pin the coalescing key A cross-model adversarial pass found that run_post_push_replication holds the per-repo coalescing key until requeue_or_release, but pin_new_objects reached it only after two unbounded git reads: the U7 legacy-repair read and the pre-existing pin read, both plain Command::output with no teardown. A wedged cat-file on a stuck backend hung the task forever, so the key was held until process death and later pushes only marked it dirty without spawning a replacement (the same class this PR closed on the /ipfs serve path). Add read_object_bounded (the bounded twins under one shared deadline) and use it at both sites; a timeout skips that object and lets the task proceed to requeue_or_release. Adds a wedge-reap regression proven load-bearing two ways.
…cannot leak it `acquire_write` took `pg_try_advisory_lock` through the shared pool, then awaited the Tigris exists/download path before constructing `RepoWriteGuard`. Two bugs fell out of that. The `tokio::time::timeout` this branch added around the call can drop the future inside that window, after the lock is held and before the only unlock (`RepoWriteGuard::release`) exists, wedging every later push to the repo. And because a session advisory lock belongs to the connection that took it, running the lock and the unlock through the pool lets them land on different connections: the unlock returns false and leaks, while a competing acquire that draws the holding connection re-enters the lock and two writers run against the same repo concurrently. `RepoStore` now owns a dedicated lock pool built with an `after_release` hook that runs `pg_advisory_unlock_all()`. `acquire_write` checks out one connection, takes the lock on it, and moves it into the guard, which unlocks on that same connection. sqlx's `PoolConnection::drop` spawns `return_to_pool`, which runs the hook, so a connection dropped by cancellation still clears its locks; the unlock is asynchronous with respect to the drop, which the tests account for. The pool is separate from the main query pool because a push holds its connection for the whole receive-pack and `db_max_connections` (20) sits below `max_concurrent_git_pushes` (32). Seven tests cover the cancellation window, the same-session release, and the serialization the pool-mismatch bug was breaking. Both the cancellation regression and the concurrent-writer case were observed red before the fix, and removing the `after_release` hook or moving the unlock back to the pool turns them red again.
…tly dropped `requeue_or_release` clears the dirty flag atomically with the decision to loop, which is correct and stays as it is. The problem was what the pass did next: a failed `get_repo_by_id` fell into an Err arm that yielded no rules, and `list_visibility_rules(...).ok()` collapsed its own error the same way. With no rules, `replication_withheld_set` returns `(false, None)`, the pass skipped its work entirely, and the loop exited having consumed the dirty bit that stood for the coalesced push. There is no reconciliation sweep to re-derive it, so that push's objects were never pinned and its withheld blobs never sealed. `requeue_refresh_state` now separates the three cases the old code collapsed. `Ok(Some)` proceeds. `Ok(None)` means the repo really is gone and releases without touching the retry budget. `Err` from either read is transient and is retried with doubling backoff up to three attempts. A rules-read error is no longer indistinguishable from a repo that has no rules. Exhausting the retries still drops the pass, which is the pre-existing residual, but it now logs at error level with the repo id and attempt count instead of disappearing behind a warning. Six tests cover the retried-then-lands path, the bounded-exhaustion path, repo-gone, the rules-read arm, freshness of the applied rules, and the coalescing property that had to survive. Reverting the Err arm, restoring the `.ok()`, raising the bound, or downgrading the error log each turns a test red.
… fallback
The resolver treated a non-empty, below-cap pin-source set as proof that every
source had been recorded, and skipped the bounded scan on that basis. But
`record_pin_source` is best effort at every call site: the ipfs_pin sites were
warn-only once their retries exhausted, and both pinata sites had no retry at
all. An object first pinned from a private repo and later pushed from a public
one whose source write failed keeps a set naming only the private source, so
`GET /ipfs/{cid}` returned 404 for an object the public repo would have served.
Migration v15 adds `pinned_cids.pin_sources_incomplete`, NOT NULL DEFAULT FALSE
so every existing row reads as complete. A record that fails outright sets it; a
later successful record clears it, inside the same transaction as the insert so
the two paths cannot drift. `needs_scan` now ORs the marker in, short-circuiting
after the existing empty and at-cap checks so it costs nothing on the serve path.
Pinata's two bare calls now share `retry_db_record` with ipfs_pin. The ipfs_pin
first-pin path additionally records the CID and its source in one transaction;
Pinata's remains two retried calls.
INV-10 is preserved: a complete set still tail-404s with no preload, asserted on
the existing preload counter. The fallback is not an authorization bypass, and
the test for that asserts the scan actually ran before checking the denial, so
the denial cannot pass vacuously. Dropping the marker term from the gate, forcing
it true, or removing the clear each turns a test red.
The marker is per-object rather than per-(object, repo): a successful record from
one repo clears a marker set by another's failure. That is the conservative
direction, since the marker only ever adds the fallback back.
…-push
`repair_legacy_provider_cid` had exactly one trigger: the already-pinned skip
branch inside `pin_new_objects`, which a push only reaches when it re-carries the
object. Normal git negotiation omits objects the node already has, so on an
upgraded node that push generally never arrives. Meanwhile `list_pinned_cids`
kept advertising the stored provider CID that this branch's `/ipfs/{cid}`
deliberately withholds, so the node handed out CIDs it would then refuse to
serve, and previously pinned data stayed unresolvable.
Two halves, both needed. A detached background sweep walks `pinned_cids` in
bounded batches with an inter-batch delay, resolves each row's repo from its
recorded provenance, and reuses the existing repair (and its cost gate, so a row
already keyed on a raw CIDv1 reads no bytes). Migration v16 adds a single-row
cursor table so a restart mid-walk resumes rather than rewinding; the cursor
advances on every row read, before any skip, so an unrepairable row cannot wedge
the walk. A row whose bytes are gone, whose provenance is NULL, or whose repo is
not on local disk is left exactly as it is. And `list_pinned_cids` now omits rows
whose key is not a raw CIDv1, so the window before the sweep catches up
advertises nothing the resolver withholds.
The sweep resolves paths directly rather than through `repo_store.acquire`, which
would pull cold repos back from Tigris and turn a repair pass into a bulk
restore. Defaults are deliberately quiet: 64 rows per batch, 60s between batches.
Ten tests cover the repair, the bytes-gone and unrepairable arms, the cost gate,
the batch bound, cursor resumption, both states of one row through the advertise
filter, and the degenerate empty and no-legacy-rows tables. Removing the filter,
the batch bound, the cursor advance, the bytes-gone early return, or the
inter-batch sleep each turns a test red.
One pre-existing test used placeholder resolver keys that the new filter drops;
its values are now real raw CIDv1s with every assertion unchanged.
…ing the reaper Three regressions from the previous four commits, each demonstrated before being fixed. The advisory-lock retry loop checked its connection out before spinning, so a caller that lost the lock held a pool connection through all sixty one-second attempts. The old pool-per-query shape returned it between iterations, so a spinner occupied nothing. That matters more than the pool sizing suggested: `acquire_write` has three non-push callers (two in issues, one in pulls) that hold no concurrency permit, and the issue routes carry no rate limit, so any self-minted DID could fire enough concurrent closes at one repo to strand the pool and fail authenticated pushes on every repo. The checkout now happens per attempt and a losing attempt returns the connection before sleeping, so only the winner retains one. Lock-pool exhaustion also carries its own error now and sheds 503 with Retry-After instead of surfacing as a 500. Holding the lock on one connection then meant a client disconnect released it early: the handler future dies mid-receive-pack, the guard drops without `release`, and `after_release` frees the lock while the detached reaper is still giving the process group its SIGTERM grace. A second writer was admitted 90ms after the drop, well inside that window, which is the invariant smart_http already documents. The write guard now rides the AdmissionGuard into the reaper, the same seam the admission permits already use, so the lock outlives the group. The success path still reclaims the guard and releases it synchronously, and a dropped guard performs no Tigris upload, so an interrupted push cannot publish a half-applied repo. The requeue loop's exit on an exhausted re-read discarded a push that coalesced during the retry window: `requeue_or_release` had cleared the dirty bit without marking the guard released, so `Drop` removed the key and no pass was ever attempted. It now falls through with nothing to replicate and lets the tail's atomic check-and-clear decide. A lap only happens when a push actually coalesced and each pays a full bounded re-read, so a sustained outage still terminates, which is tested with ten thousand injected faults under a watchdog rather than argued. Also clamps the lock pool's derived size, which previously followed max_concurrent_git_pushes up to its million-connection ceiling, and documents the node's total connection budget on that knob.
…corded, and let the sweep retry Four follow-ups from an adversarial pass over the previous commits. The marker clear ran unconditionally inside `record_pin_source`'s transaction, but its INSERT is `ON CONFLICT DO NOTHING` guarded by the cap, so it records nothing when the repo is already a source. That is the common case, not a rare one: the pin path calls `record_pin_source` for every already-pinned object, and on a requeue pass that is the whole-repo enumeration. So a re-push from the repo that was already recorded cleared a marker set by a different repo's failure and the public copy went back to 404ing. The clear is now gated on the insert actually adding a row. A genuine record from a third repo still clears the marker, which needs a per-(oid, repo) table to close and is documented rather than papered over. The sweep advanced its cursor before every skip, which stops a wedge but made a transient skip permanent: once the walk reached the end of the table the cursor parked there and no later boot read another row. On a Tigris-backed node the common skip is "repo is not on local disk", so the sweep would repair whatever happened to be warm at boot and then never run again, and the new advertise filter turned those rows from "advertised but 404" into "never advertised, never repaired". Repairs now report whether a skip was retryable or terminal, and a run that skipped anything retryable rewinds the cursor once, after the walk, so the next run re-walks. A permanently unrepairable row is terminal and does not trigger a rewind, so this cannot become a hot loop. The sweep also reached its repo paths through the raw join helper, skipping the three-layer traversal barrier `RepoStore::local_path` applies. A test with a repo row named `../../escapee` showed it reading and repairing bytes from outside `repos_dir`. That barrier is now extracted and shared, so the two callers cannot drift. And the repair's synchronous `git cat-file` moved under `spawn_blocking`, matching the existing treatment of the withheld walk, so a wedged read no longer parks a worker thread for the full service timeout. Finally the resolver's two source-set queries ran ahead of the work-budget peek that exists to shed a spent caller before doing work. The peek now runs first. A throttled caller whose set turns out complete gets 429 where it used to get 404, which is the honest answer since its search never ran, and it removes an oracle that let a throttled caller tell a complete source set from an incomplete one.
…ool is exhausted The lock-pool work gave `acquire_write` a distinct exhaustion error and mapped it to 503 with Retry-After, but only on the push handler. The three other callers, both issue writes and the PR merge, still turned it into a generic git error and returned 500. Those are exactly the callers that hold no concurrency permit, so they are the likeliest to meet an exhausted pool in the first place. All three now go through the push handler's helper rather than repeating the mapping, so the log level and the message cannot drift between the four sites. Each has a test that occupies the single connection of a one-slot pool with a guard on a different repo, so the failure is pool capacity rather than advisory-lock contention, and asserts status 503 plus retry-after through IntoResponse. Reverting the mapping at one site turns only that site's test red. Worth knowing for review, and left alone deliberately: close_issue takes the write guard before its author check, so this shed is reachable by any authenticated caller whether or not they could close the issue. That ordering predates this change and reordering the handler is not this commit's business, but it does mean the 503 discloses that the repo's write pool is busy.
…sh never reaches the upload Two claims from earlier commits were argued rather than executed. Both are now observed. The legacy-CID sweep's spawn sat inline in main() where no test could reach it, so the batch size, the inter-batch delay, the detachment and the shutdown select were all reasoned. The block moves verbatim into `spawn_legacy_cid_sweep`, which a test calls directly. It seeds rows whose keys are already canonical, so the cost gate skips each and the persisted cursor reads out walk progress cleanly, then asserts the cursor stops at the configured batch boundary, the call returns without blocking, the task is still alive inside its delay, and shutdown ends it. Never spawning, reading the batch or delay from the wrong knob, or removing the shutdown branch each turns it red. What remains reasoned is the single call line inside main(), down from a twenty-six line block. The claim that a disconnect-interrupted push never uploads to Tigris was structural: the tests build the store with no Tigris client, so a counter inside the client arm could never move and would have proved nothing. The counter now sits at the upload decision, before the client is consulted, and the disconnect test asserts zero while the success test asserts exactly one. That positive control is what makes the zero meaningful, and it is itself load-bearing: releasing the success path with failure semantics drops it to zero, and adding a Drop impl that uploads raises the disconnect count to one. The counter observes reaching the upload site, not an S3 request. TigrisClient wraps a concrete SDK client with no injectable endpoint, and plumbing one through would be out of proportion to the risk here.
Reconciles the served-git concurrency cap with the /ipfs CID tree gate. The two branches independently reworked the same three seams, so most of this is picking one mechanism per seam rather than taking a side per file. /ipfs walk admission: keeps this branch's provenance-first resolver and its lazy legacy scan, but drops the detached serve task in favour of #174's shared Arc<WalkAdmission>, cloned into each spawn_blocking. Both close the permit-release bypass; the Arc does it without leaving an abandoned request's full legacy scan running against a held slot. Ported in with it: the request-budget gate at all four stages with each child deadline clamped to the remainder, the walk timeout computed inside the closure so queue delay is charged, taint sources in place of a single truncated flag, and the deterministic-fault 500 gated on nothing else having tainted. The two per-request ceilings are now composed rather than one shadowing the other. The visit ceiling was unwired by the merge and the walk cap answered to only one of the two knobs; both are honoured, the tighter one winning. Object probe: object_type_bounded takes a Duration and returns ProbeError, so this branch's call shape keeps #174's absence-vs-unreadable-store discriminator. A clean `missing` from cat-file is byte-identical whether the object is absent or the pack is unreadable, and the discriminator is store readability, not git's wording. Write lock: the advisory-lock pool's after_release hook does not cover an unlock that errors on a live session. Measured: a poisoned connection returned to the pool still held the lock 15s later, because the hook's own unlock_all fails the same way. So the guard now disposes of that connection, in release and in the Drop backstop. Post-push replication goes to #174's coalescer. This branch's requeue_faults integration suite has no equivalent seam on that design and is not carried over; it is recoverable from the pre-merge head. Also clamps the provenance path's three per-source lookups to the request budget. They run while the walk permits are held, which is the stall hazard the preload clamp already covered, and the structural guard found them because it now requires every occurrence to be wrapped instead of assuming exactly one. 1346 tests pass across the workspace; fmt and clippy are clean under --locked. The four re-anchored structural guards were mutation-tested and all four go red on the defect they name.
The merge deleted coverage that no longer compiled against the reconciled designs, rather than re-seaming it. Three of those tests have counterparts in the merged tree after all: unlock_error_connection_close_is_bounded and write_guard_dropped_off_runtime_disposes_the_connection came back with close_conn_bounded and the Drop backstop, which the lock-pool design needed after all (the after_release hook cannot free a lock when the unlock errors on a live session). Both now observe the store's derived lock pool rather than the pool handed to for_testing, which is where the guard's connection actually lives. get_by_cid_caps_repos_walked_knob_bounds_the_walks was orphaned when its body was spliced into a sibling test. It drives GITLAWB_IPFS_MAX_REPOS_WALKED, which is the binding cap only now that the gate takes the tighter of the two walk ceilings. The one test not restored is write_guard_release_when_not_locked: the guard has a single construction site and it sits below the lock-or-bail, so a guard that never took the lock cannot exist.
These covered post-push work being requeued rather than dropped, and were removed in the merge because they drove the replaced requeue loop. The behaviour they check is still there, so they re-seam onto the coalescer without any production change. The seam moved in three ways. The inflight key is the repo identity key, not the row id. A coalescing push now merges its (old, new) tip pairs into the pending slot, so a test that passes an empty vec merges nothing and the drain never laps: every coalesce carries real commit oids. And the full-scan arm is reached through the pending-slot overflow, so the leak test coalesces 1025 pairs and asserts the slot degraded to FullScan before the run. run_encrypt_pin_task_for_test builds the task context the way the production spawn site does and takes a real owner and name, because the drain re-fetches by owner and name; a blank name would resolve Gone on every lap. All four are mutation-proven: drain-does-nothing, stale-empty-rules, full-scan-skips-the-fail-closed-filter, and a leaked inflight key each turn their own test red.
A real Postgres pool will not fail on demand, so the drain's error arms have no way to be driven from a test. This ports the injection table the removed requeue loop used, keyed on the task's repo id, which is a fresh uuid per test so parallel cases cannot cross-inject. The two reads now go through drain_get_repo and drain_list_rules, whose bodies consult the seam under cfg(test) and then make the same call as before. Error handling is untouched: the repo read keeps its arms, the rules read keeps its ok() collapse, and no retry exists yet. The suite is unchanged at 856, which is the point. inject and counters carry a dead_code allow until the tests that call them land.
…push The drain read the repo row and its visibility rules once each. Either read failing dropped that coalesced push's pins and recovery copy for good: the repo read returned None on error, and the rules read collapsed through ok(), which makes the repo read as not announceable. Both fail closed, so nothing leaked, but nothing sweeps the lost work up either. A single database blip was enough. resolve_drain_object_list now refreshes through a bounded retry: three attempts, 50ms doubling, and either read failing is transient. A missing repo row is the one terminal answer, so it releases at once and spends no retry budget. Exhaustion logs at ERROR with the repo id and attempt count, so giving up is observable rather than silent. The coalescing loop is untouched. It already has the shape the retry needs: a refresh that gives up skips the pin call, and the next finish_or_take_pending picks up anything that coalesced while the retry was running. Eight restored tests cover it, written and observed red against the seam-only tree first. The five the plan predicted red came up red for the reasons it predicted, and the three controls stayed green. All eight are mutation-proven: no-retry, an off-by-one bound, gone burning budget, a rules error read as empty, an unfiltered candidate set, a duplicated refresh, break-on-giveup, and a pending slot that never drains each turn their own test red.
… marker Three gates scanned the "production half" of api/repos.rs by splitting at the first cfg(test) attribute. Adding test-only items above the code they check moved that split point above every line being scanned, so all three failed looking for production code that was still right there. They now split at the test module, which is what they meant. This is the same defect already fixed in the ipfs gate during the merge, and no split-on-the- attribute site remains. Both behavior-bearing gates were re-proven load-bearing: F2 goes red when the Pinata object set is re-derived before the pin permit is taken, and the tail gate goes red when release stops consuming the same success flag. F3 is a presence check whose marker cannot be removed by a single compiling edit, so it is not mutation-proven here.
…into fix/issue-135-ipfs-cid-tree-gate
… 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.
…rder 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.
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.
…t 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.
…temp path
The compose reinstall added last round is not safe under overlap, and `main`
never wrote compose from this document, so the race arrives with that commit.
Two executions — a double run, a retry while the first is still pulling, two
operators — shared the fixed `/opt/gitlawb/compose.yaml.new`. The second
truncates the file the first is about to install; and once the first renames,
the second's open descriptor keeps writing into the live `compose.yaml` inode.
Both outcomes stay valid YAML, so the `up -d --remove-orphans` on the next line
accepts a blended or truncated service set and deletes whatever fell off — on a
live node, the local postgres or the TLS terminator. Both reviewers reproduced
it independently; one measured a short file in 12 of 12 trials.
The whole step now runs under `flock -n` on /opt/gitlawb/.upgrade.lock and
exits non-zero if the lock is held, so a second run cannot restart against a
file the first is mid-install. The temp file comes from `mktemp`, so concurrent
writers cannot truncate each other, with a trap to clean it up on failure.
The compose body is base64-encoded rather than written through a nested
heredoc. The rendering carries `$${VAR}` passthroughs that must reach the file
unexpanded, which needs a quoted delimiter, and a shell heredoc nested inside an
indented Terraform heredoc depends on the dedent landing that delimiter at
column 0. Decoding one line has neither failure mode and no delimiter can
collide with the content.
Not validated locally: terraform is not installed on this machine, so this
change has had no `fmt`, `validate`, or `plan` run against it.
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
force-pushed
the
fix/p2p-gossip-ingest-auth
branch
from
August 17, 2026 12:34
491ac38 to
7ef43fc
Compare
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.
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.
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.
…e 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.
…ng 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.
…plit 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.
beardthelion
force-pushed
the
fix/p2p-gossip-ingest-auth
branch
from
August 17, 2026 13:14
7ef43fc to
c003f65
Compare
beardthelion
force-pushed
the
fix/gossip-replay-bound
branch
from
August 17, 2026 13:14
aee00b3 to
2349a89
Compare
jatmn
requested changes
Aug 21, 2026
jatmn
left a comment
jatmn
left a comment
Collaborator
There was a problem hiding this comment.
I found an issue that needs to be addressed before this is ready.
Findings
-
[P1] Sanitize every rejected timestamp before logging it
crates/gitlawb-node/src/p2p/mod.rs:1250
DateTime::parse_from_rfc3339accepts RFC 3339 fractional seconds with arbitrarily many digits (discarding precision after nanoseconds). A signed timestamp such as an old instant followed by megabytes of fractional digits therefore parses successfully, reaches theTooOldorTooFarFuturearm, and is copied intoStaleTimestamp; the swarm loop then logs that reason verbatim. Only the parser-error branch callssanitize_for_log.The root cause is treating successful parsing as proof that the original wire representation is safe to expose. The parsed
DateTimeis bounded, but the source string is not. A registered peer can consequently turn bounded gossip deliveries into unbounded log, disk, and log-pipeline traffic despite the per-source event limiter. Apply the same bounded/control-character-safe rendering to all three freshness failure branches, preferably by logging a bounded canonical rendering of the parsed timestamp forTooOldandTooFarFuturerather than the original wire string. Please add a regression test using an overlong but parseable fractional timestamp that exercises both directions and asserts that the emitted detail is bounded and contains no control characters.
…_msg to http.rs (#220)
fix(node): gate GET /ipfs/{cid} tree objects so a withheld subtree's structure can't leak (#135)
…-false-trust-lookup fix(gl): only report registered: false on 404 in whoami (#220)
fix(node)!: enforce owner-only push by default
These lock-path proofs use non-owner proof DIDs. With enforce_owner_push defaulting on, they fail before receive-pack runs.
fix(gl): read whoami error bodies through CappedBody.text
# Conflicts: # README.md # crates/gitlawb-node/src/db/mod.rs # crates/gitlawb-node/src/main.rs
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.
jatmn
requested changes
Aug 31, 2026
jatmn
left a comment
jatmn
left a comment
Collaborator
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Merge readiness
- [P1] Rebase onto the current target branch before merge
The PR is currentlyCONFLICTING/DIRTY. Its target (fix/p2p-gossip-ingest-auth) and head each mergedmainindependently and have diverged acrossp2p/mod.rs,metrics.rs, anddb/mod.rs, so this cannot merge as reviewed. Rebase or reconstruct the head on the current target, resolve the conflicts, and request review of the resulting diff.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bounds replay of authenticated gossip ref-update events. Stacked on #325, which is what makes these events authenticated in the first place.
Why this is separate from #325 rather than folded into it
With #325 merged and this not yet merged, an attacker's capability is strictly lower than it was before #325: forging arbitrary ref-updates for any peer required no key at all, and afterwards they are reduced to replaying events a legitimate node actually published, with the same repo, refs, and shas. The intermediate state is better than the pre-state, not worse, so the split does not leave a window where the system is more exposed than it was.
That is the test I applied, and it is the one the #173/#321 split failed: that resolver hardening withheld the provider CID for unrepaired rows, so deploying it alone actively broke existing pins, and the ordering argument only held as long as nothing shipped in the gap. Writing this out here rather than leaving it in a comment is the other half of that lesson.
What it does
Two layers, following the shape production gossip systems use, where a dedup-layer fix and an authentication-layer fix are paired rather than either being treated as sufficient.
A freshness window on the already-signature-covered
timestamp: 600 seconds into the past, 60 into the future. Two comparisons, never a distance. Anabs()window accepts a future-dated event as readily as a late one, and a future-dated event also pins a seen-set slot while sitting outside the past-window check until the clock catches up.A bounded seen-set keyed on SHA-256 of the canonical signing bytes, not the raw wire bytes. That distinction is the whole point: one signature verifies against a family of wire encodings, so a raw-bytes key deduplicates nothing. The frozen pre-version artifact and the same artifact with
"v":0injected are different lengths, both verify against one signature, and produce identical signing bytes.The guard runs immediately after signature verification and above the per-author debit. Below the debit it would still stop the duplicate row and the duplicate sync while letting every replay drain the victim author's budget, which is the harm this is about. It runs on the verified path only: unsigned bytes are predictable, so applying dedup there would let an attacker pre-send a victim's expected event and have the genuine one dropped as a replay.
A key is recorded only on acceptance, through a reservation whose drop releases it, so a transient write failure does not permanently burn an event's slot. At capacity the guard fails open and counts the degradation, because a saturated set that dropped all fresh gossip would convert a loud resource attack into quiet mesh-wide censorship.
Also included:
check_createdin gitlawb-core used a symmetricabs()window, so it tolerated 300 seconds of future-dating and a signer could roughly double a signature's effective validity by stamping forward. It is now two comparisons like the gossip path. Adding a freshness window obliges auditing the siblings, which is how that surfaced.What is proven rather than argued
Every guard here is backed by a mutation that turns a named test red, and they are present-but-wrong re-implementations rather than deletions, because each of these compiles and reads sensibly: keying on raw wire bytes, keying fresh per ingest, keying on
(repo, ref, sha), anabs()freshness window, recording before accept, failing closed at saturation, placing the guard below the author debit, a sweep that stops evicting, confirming on a write failure, and treating an expired entry as absent at capacity.Two are worth calling out because the obvious version of each test cannot catch what it names. The revert case needs three events, since two never collide under a
(repo, ref, sha)key. The layer-composition case has to run at the future-skew edge, since a present-stamped probe is rejected by freshness anyway and stays green under a shortened retention.An attacker-supplied
createdcould overflow the window subtraction and panic a debug build; that is fixed with saturating arithmetic and a test driving all four extremes of the type.Known open, not closed here
A compromised or malicious registered signer can still mint fresh signed events with fresh timestamps, each of which gets a distinct key and passes freshness. This bounds third-party replay of a captured event; it is not an aggregate write-volume defense and should not be described as one.
The seen-set is in-process, so a restart readmits any event still inside its freshness window, once per restart. Bounded by the window rather than unbounded, and documented on the guard.
POST /api/v1/sync/notifyreaches the same two sinks with no dedup. Bounded differently there, since that handler still accepts unsigned notifications naming any known peer DID, so forgery already beats replay and the per-IP limiter is the real bound. Worth closing separately.The freshness window is an availability bound with no resume: a peer returning from a partition longer than the window drains a backlog stamped at push time and every one of those events is dropped, with no republish and no repair path. That is a product decision about what federation should do with stale-but-genuine updates, not something to patch quietly.
Full suite, clippy and fmt clean locally; CI has the authoritative run for this head.