47 Commits

Include renames

Author	SHA1	Message	Date
beardthelion	6f990197ea	feat(node): per-IP write-surface rate brake + purge-spam admin tool (#196) ... * feat(node): add per-IP write-surface rate brake and apply to write_routes Introduces a dedicated write_rate_limiter bucket (GITLAWB_WRITE_RATE_LIMIT, default 600/hr, 0 disables), separate from the creation brake so a write flood cannot drain the creation budget (KTD-1). Attaches rate_limit_by_ip to write_routes, mirroring creation_routes. Per-DID deliberately not paired (a DID farm never trips it). Tests (route-level, execution-verified against Postgres): a write_routes sink (PUT /star) is 429'd before auth; an exhausted write bucket does not throttle repo creation (separate-bucket property). * feat(node): extend the per-IP write brake to the remaining write sinks Attaches rate_limit_by_ip (write bucket) to task_write_routes, issue_write_routes, bounty_write_routes, profile_write_routes, and the /graphql POST/GET surface (KTD-5 — an HTTP-layer brake counts every /graphql request; /graphql/ws subscriptions stay unbraked). write_ip_limiter is now built once and shared. Every non-git, non-creation authenticated write group is braked. Tests (execution-verified against Postgres): /graphql POST is 429'd, and a representative REST write sink (issue comment) is 429'd, by the write brake. * test(node): adversarial TrustedProxy verification through the write brake Middleware-level tests complementing the client_key unit coverage: a spoofer varying the client-controlled leftmost X-Forwarded-For hop cannot rotate its bucket key (keyed on the trusted rightmost hop); distinct trusted client hops get distinct buckets (no cross-starvation); and a request with no trusted header in a proxy mode falls back to the socket-peer key — the documented collapse, asserted by name so any change is deliberate. Operator-warning on missing-header deferred deliberately: a per-request warning is a log-noise risk and the security-load-bearing fallback (peer key, never skip) is what these tests lock down. * feat(node): purge-spam admin subcommand for the empty-burst cleanup Adds a purge-spam subcommand (clap #[command(subcommand)] split; no-subcommand path still runs the node unchanged). Candidate selection is signature-based, not per-DID: a repo qualifies only if owned by the named burst DID AND verified empty (zero git refs) per repo. A hard exclusion gate runs BEFORE the empty check, so an empty repo owned by the content-bearing user or the mirror-bot DID is never a candidate. Dry-run is the default; deletion needs an explicit --execute flag and operates per-repo, never 'all repos of DID X'. Tests (RED->GREEN, exclusion gate confirmed load-bearing by targeting an excluded DID directly): empty target listed; target-with-refs spared; empty excluded/ intern repos spared (must-assert); dry-run deletes nothing; execute removes only the empty target on disk. * fix(review): make purge-spam empty-check reject git-discovery reads P1 from code review: on_disk_ref_count shelled 'git for-each-ref' with only the repo path as cwd and no --git-dir, so a path that exists but is not a bare repo let git discover a parent .git (repos_dir may sit inside the operator's checkout) and read a DIFFERENT repo's refs — possibly 0, deleting a real repo. Now require the bare-repo markers (HEAD file + objects/ dir) before trusting any count; anything else fails closed (skipped). Tests: a non-git dir under a git ancestor (zero-ref) now fails closed — verified RED without the guard (read the ancestor's 0 refs); target-DID-never-excluded invariant pinned; write_flood test anchored to a genuinely-drained bucket so its assert_ne can't pass vacuously. * fix(review): normalization-consistent purge, brake helper, CLI + coverage Addresses the remaining code-review findings: - purge-spam DID matching now uses did:key normalization on both sides (list_repos_by_owner_did via OWNER_KEY_CASE_SQL, is_excluded and the target scope via normalize_owner_key), so short/full form is consistent and an excluded identity is protected in either form. Adds a short-form exclusion test. - Extract the per-IP write brake into a chainable WriteBraked helper so a future write group can't silently ship unbraked (kept separate from add_auth_layers by design; documented why). - Mark --repos-dir / --database-url global so admin subcommands accept them after the subcommand (was an 'unexpected argument' parse error); verified. - Document GITLAWB_WRITE_RATE_LIMIT in .env.example incl. the per-IP NAT-aggregate caveat so operators can size it. - Coverage: adoption-floor (under-limit write passes), write-limit=0 disables, task/bounty/profile route-level 429s, /graphql/ws stays unbraked. Full suite 927 pass / 0 fail; fmt + clippy -D warnings clean. * test(review): close the reasoned-not-run gaps by execution - purge-spam execute now re-verifies emptiness immediately before delete (TOCTOU: a push between selection and delete no longer risks deleting a now-non-empty repo) and removes the on-disk bare repo dir so DB and disk stay consistent; a per-repo delete failure warns and continues rather than aborting the batch. - Tests (RED-verified where they cover new behavior): partition_for_delete skips a repo no longer empty; execute removes the on-disk dir (RED without the removal); list_repos_by_owner_did finds a short-form burst row (RED without the SQL normalization); every braked write group passes under-limit (per-group grant path, not just the star representative). * fix(node): reject path-traversal repo names in purge-spam before remove_dir_all A peer-mirror row skips API name validation, so a burst-owned repo can carry a '../' name; repo_disk_path joined it verbatim and the emptiness marker check passed on the traversed target, so purge-spam selected and remove_dir_all'd a repo OUTSIDE repos_dir. Validate the name via repo_store::validate_repo_name at selection (fail closed -> non-candidate) and assert canonical containment inside repos_dir immediately before the destructive remove. Adversarial RED->GREEN: a victim bare repo reachable via '../../victim' is deleted before, survives after; symlink-escape covered by path_within. * fix(node): delete child rows transactionally in delete_repo_by_id delete_repo_by_id was a bare DELETE FROM repos with no cascade and the schema has no FKs, so every child row (ref_certificates, pull_requests + pr_reviews/ pr_comments, webhooks, push_events, protected_branches, repo_stars, repo_replicas, repo_labels, visibility_rules, encrypted_blobs, repo_icaptcha_proofs, agent_tasks, and the slug-keyed branch_cids/sync_queue/received_ref_updates/arweave_anchors) was orphaned and could re-join a later repo reusing the id. Wrap parent+child deletes in one transaction, resolving the owner_short/name slug for the repo-TEXT children. bounties (financial) and issue_comments (unmappable) are left intact by design. RED->GREEN test seeds a repo_id child, a slug child, and a PR grandchild. * fix(node): reap write_rate_limiter in the periodic cleanup loop The PR's new write_rate_limiter was the one limiter omitted from main()'s hand-maintained cleanup list, so its key map never shed expired entries until the 200k cap. Move the sweep into AppState::cleanup_rate_limiters (co-located with the limiter fields, single source of truth) and call it from the loop, so a newly-added limiter is reaped rather than silently dropped. Guard test seeds a reclaimable entry into write_rate_limiter and asserts the sweep reaps it — RED if write_rate_limiter is dropped from cleanup_rate_limiters (verified). * fix(node): hold the per-repo advisory lock across purge recheck+delete purge-spam took no write lock, so a concurrent receive-pack could land a ref between the emptiness recheck and the delete, silently losing a live push. Add RepoStore::try_lock_repo — a lock-only, non-blocking counterpart to acquire_write (no Tigris I/O) whose guard holds a dedicated pooled connection so the advisory lock and its unlock run on the same session. purge now locks each candidate, rechecks emptiness UNDER the lock, deletes, and releases; a repo held by a live writer is skipped, not force-deleted. RED->GREEN: a locked empty repo survives purge, then deletes once the writer releases (neuter-check confirms the lock is load-bearing). * fix(node): count purge-spam disk-removal failures separately from deletes The Ok(n) arm incremented the deleted total before remove_dir_all, whose failure only warn!'d, so the summary reported N deleted even when the on-disk dir survived (DB/disk drift shown as clean success). run_purge_spam now returns a PurgeSummary and counts a failed (or containment-refused) on-disk removal in disk_failed, never folding it into deleted; the summary line surfaces 'M on-disk removal(s) failed'. RED->GREEN: a read-only parent dir forces the removal to fail; deleted=1 AND disk_failed=1 with the dir surviving (neuter-check confirms the counter is load-bearing). * fix(node): warn on unparseable rate-limit env vars instead of silently defaulting GITLAWB_CREATE/WRITE/PUSH_RATE_LIMIT parsed with .ok().and_then(parse).unwrap_or, so a typo (e.g. 5O with a letter O) silently reverted to the default and left the operator believing a stricter cap was in force. Route all three through rate_limit_from_env, which distinguishes absent/empty (default, silent) from present-but-unparseable (default, WARN). Pure resolve_rate_limit is unit-tested across absent/empty/valid/zero/unparseable — 0 stays valid (its own ==0 warn handles the disable case). * docs(node): document GITLAWB_IPFS_API/TIGRIS_BUCKET/METRICS_ADDR/SHUTDOWN_GRACE_SECS in .env.example Four config knobs read in config.rs were absent from .env.example, so operators had no template for them. Add each in its matching section with the config default (INV-24: operator docs match the config the code reads, in the same PR that touches the file). * test(node): exercise the dry-run guard with a real on-disk candidate present dry_run_deletes_nothing ran against an empty repos_dir, so select_spam_candidates found nothing and run_purge_spam hit the no-candidate early return — the if !execute { return } guard was never exercised with candidates present (a vacuous guard, INV-21). Materialize a real empty bare repo so a genuine candidate exists, then assert dry-run leaves both the row and the on-disk dir. Verified load-bearing: removing the !execute guard turns the test RED (dry-run deletes). * style(node): cargo fmt for the #196 fix set * fix(node): pin the writer's advisory-lock connection in RepoWriteGuard acquire_write took the session-scoped advisory lock through the shared pool and returned the lock-owning connection to the pool, so release unlocked on a different connection (silent no-op) and a concurrent try_lock_repo could be handed the idle lock-owning connection and reentrantly re-grab the lock, defeating the purge tool's mutual exclusion. Pin a dedicated PoolConnection for the guard's lifetime and unlock on it, mirroring RepoLockGuard. Load-bearing contention test (single-connection pool to force the reentrant grab deterministic): RED (Ok(Some)) before, GREEN after. * refactor(node): introduce ObjectStore trait seam over TigrisClient Extract an async ObjectStore trait (exists/upload/download/delete) that TigrisClient implements, and hold Option<Arc<dyn ObjectStore>> in RepoStore instead of Option<TigrisClient>. A dyn async trait needs async-trait (added as a direct dep; already transitively present), which E0038 would otherwise reject. Pure refactor — no behavior change; full suite stays green. Enables an in-test fake for the purge Tigris work (U3). Cargo.lock also picks up the pre-existing 0.5.0->0.5.1 version sync (#185: the committed lock was stale vs the manifests) that any cargo run regenerates. * fix(node): make purge-spam Tigris-authoritative and archive-deleting purge-spam built its RepoStore with no object store, so on a Tigris deployment it rechecked emptiness against possibly-stale local disk (deleting a repo with live remote refs) and left the archive behind (to be re-downloaded into a later same-owner/name repo). Give the admin command the configured object store, refresh the local copy from the authoritative archive under the per-repo lock before the recheck (fail-closed on a store error via a new skipped_store_error), and delete the archive on a successful purge (failures counted in a new archive_failed, never folded into success). When no bucket is configured the None path is byte-for-byte unchanged. Three load-bearing guards verified RED->GREEN on the Tigris-blind purge: remote-refs-not-deleted, archive-deleted, fail-closed-on-unreachable. * fix(review): don't hold/leak the pool connection in acquire_write Two regressions the connection-pinning fix (010c625) introduced, caught in pre-push review: - Pool pressure: acquire_write held its pinned connection through the whole 60s lock-retry backoff; pre-change the retry probed via the pool and returned the connection between attempts. Under a same-repo write burst every spinner now held a pool connection through its sleeps and could starve the shared pool. Restructure the loop to return the connection between FAILED attempts and keep it only on the winning one. - Lock leak: on the object-store download-error early-return the pinned connection dropped back to the pool with the advisory lock still held (no guard Drop runs pre-construction, and sqlx does not release session locks on connection return), blocking every future write to that repo until the pool reaped the connection. Unlock on the pinned connection before bailing. Add acquire_write_guard_excludes_try_lock_across_connections: a multi-connection test proving the exclusion holds across sessions (the production topology), which also catches a pin-but-don't-lock regression the single-connection test cannot. * fix(node): make advisory-lock guards drop-safe and reorder acquire_write RepoWriteGuard and RepoLockGuard now hold their pooled connection as an Option: release() takes it (returns it to the pool) while a Drop impl closes the connection when release() was never reached — an early ?, a panic, or the handler future cancelled on client disconnect. Closing the connection ends the Postgres session so the server frees the session-scoped advisory lock, instead of returning a lock-holding connection to the pool where it would block every later write to that repo until the connection is reaped (up to ~30min at the pool's default max_lifetime). acquire_write now wraps the winning connection in the guard BEFORE the freshness download, so a cancellation during that await is covered by the guard's Drop rather than orphaning the lock on a bare connection. Tests observe the leak by disabling the pool's ambient reaping (which otherwise masks it ~1.8s after drop), so the lock stays held until the fix's own unlock. RED before / GREEN after for both guards and the acquire-side cancel; each protection line proven load-bearing by revert (INV-21). * fix(node): complete a fully-received push after client disconnect git_receive_pack now runs the acquire -> receive-pack -> release core in a spawned task the handler awaits but whose cancellation it does not propagate. The pack body is fully buffered before the handler runs, so the task is self-contained: a client that disconnects mid-apply drops the handler future without cancelling the task, and the push still applies, uploads, and releases its lock in order. Drop-safety (the guard's close-on-drop) remains the backstop; this closes the dominant window where a disconnect both bricked the lock and abandoned a received push. The post-push bookkeeping tail stays in the cancellable handler (a disconnect still forgoes it, as today). Settled OQ1 by execution: a guard abandoned inside a detached task when the tokio runtime tears down does not panic in Drop (5/5 runs), so detached-task shutdown tracking stays deferred. Kept the check as a regression guard. Disconnect regression drives a real push with a sleeping pre-receive hook and drops the handler mid-hook: RED on the inline handler (the git group is killed, ref absent), GREEN once detached. * fix(node): serialize archive uploads under the per-repo advisory lock The lazy-migration upload in acquire(), init()'s background upload, and release_after_write() (fork's upload) previously PUT to the object store with no lock, so an in-flight upload could resurrect an archive purge-spam had just deleted under the same lock, and init's background upload could clobber a freshly-pushed archive. All three now go through upload_locked(): take the per-repo lock, re-check the local dir exists under it (a purge removes the dir under its lock, so a post-purge uploader finds it gone and skips), upload, then release. The background migration/init uploads skip on contention and self-heal; fork's foreground upload waits (bounded) instead, since a skipped fork upload has no later retry. Tests (recording object-store double): init upload skips under a held lock, fork upload waits then PUTs once after release, a late upload after a purge-style delete does not resurrect the archive, and an uncontended upload PUTs once. Load-bearing per INV-21 — neutralizing the lock+dir-check turns the three contention/resurrect tests RED. * fix(node): let purge-spam reach repos that exist only as archives On a multi-machine (object-store) deployment a spam repo can exist only as a Tigris archive with no local copy. Selection filtered on a ref count that returns non-empty for a missing local path, so such a repo was never a candidate and the tool it was hardened for could not reach it. Selection now distinguishes missing-local (Option::None) from a one-ref repo and admits a missing-local row as a *remote-unverified* candidate only when an object store is configured (via RepoStore::has_object_store); partition_for_delete passes those through rather than re-dropping them on the local ref count. The execute loop is unchanged — it already refreshes from the archive and rechecks emptiness under the per-repo lock — so a remote-only empty archive is deleted, one with refs is refreshed and skipped, and missing-both fails closed (refresh no-ops, the local recheck reports non-empty). Storeless deployments keep the old fail-closed behavior. Dry-run marks remote-only candidates distinctly and counts them in a new PurgeSummary field. Tests (recording object-store double): remote-only empty archive deleted, remote-only archive-with-refs skipped, missing-both fail-closed, storeless missing-local not a candidate. Load-bearing per INV-21 — reverting the store-configured admission turns the three remote-reach tests RED. * refactor(node): dedup limiter setup + write-brake test driver; fix .env wording - .env.example: the write brake wraps the whole /graphql route, so its docs now say it covers all GraphQL HTTP requests (queries and the playground GET), not only mutations, with WebSocket subscriptions called out as excluded. - main.rs: collapse the identical create/write/push per-IP limiter setup into build_ip_limiter (resolve-env, build bounded, warn-on-zero), preserving the exact limits/defaults and the present-but-unparseable warning in rate_limit_from_env; each limiter now also info-logs its configured value. - api/repos.rs: the nine per-IP write-brake tests share a send_from helper mirroring rate_limit.rs's post_from/post_with, replacing the repeated request-build + ConnectInfo + oneshot + status boilerplate. Behavior identical. * docs(node): drop the .env.example shutdown-grace entry #196 added (#196) The GITLAWB_SHUTDOWN_GRACE_SECS field, its doc-comment, and main.rs's grace handling pre-date this PR (#22); #196's diff only added the .env.example entry, whose comment promised a 503-on-expiry the node does not implement. Remove just that entry. PR #194 owns the real bounded drain and the config prose in disjoint files. * fix(node): give advisory-lock guards a dedicated pool and bound the release upload (#196) Repo-write guards pinned a connection from the shared app pool for the whole receive-pack plus an untimed archive upload, so ~20 concurrent distinct-repo pushes starved unrelated DB handlers. Guards now draw from a dedicated lock_pool (GITLAWB_DB_LOCK_POOL_MAX_CONNECTIONS, default 32, sized to peak writers) separate from the app pool that serves handlers and the purge delete, and release()'s Tigris upload is wrapped in a timeout so the connection hold is bounded on every path. The guard still locks and unlocks on its single held connection (no off-session unlock). Db::pool() is now test-only since nothing in the bin reads the raw app pool directly. * fix(node): advertise a window-derived Retry-After on rate-limit 429s (#196) Both 429 sites hardcoded Retry-After: 60 while every limiter uses a one-hour window, so a compliant client retrying at 60s kept hitting 429 for nearly another hour. The limiter now returns the delay from its oldest live timestamp (window - elapsed, clamped >= 1); a cap-full rejection with no bucket advertises the full window. The check() -> bool API is preserved as a wrapper so unrelated callers are untouched. * fix(node): skip slug-scoped purge cascade when a mirror row shares the slug (#196) delete_repo_by_id cascaded branch_cids/sync_queue/received_ref_updates/ arweave_anchors by the normalized owner/name slug, which is non-unique across repos rows (INV-9): a did:key:zX row and a bare zX mirror of the same name share the slug. Purging an empty did:key row therefore wiped a surviving non-empty mirror's branch->CID index and Arweave anchors. The cascade now runs only when no other repos row resolves to the same slug (checked with the same OWNER_KEY_CASE_SQL the slug is built from, on the delete's own transaction); the physical row and id-keyed children are always removed. The concurrent colliding-sibling-create window stays an accepted operator-scoped residual. * fix(node): fail closed classifying the purge target directory (#196) local_refs_on_disk returned None for a missing path, an existing non-bare directory, AND an unreadable path alike; with a store configured, None promotes the candidate to remote_unverified and the archive refresh's remove_dir_all replaces whatever sits at the target. It now returns None only for a verified NotFound (symlink_metadata + ErrorKind::NotFound, so a dangling symlink counts as present); every other state fails closed as present so a non-repo directory at the target is never overwritten. * fix(node): run the push completion tail in the disconnect-surviving task (#196) The receive-pack refactor detached only acquire/apply/release; the whole success tail (push+trust records, ref certificates, webhooks, replication/pinning, ref-update broadcast, peer-notify) ran inline in the cancellable request future. A client disconnect mid-apply committed and uploaded the git state but dropped that tail, leaving accepted git history with none of the metadata or fan-out every other push produces. The tail now runs inside the detached task, gated on receive-pack Ok; report-status still returns to a connected client early over a oneshot (never awaited on the JoinHandle), and a dropped receiver no longer aborts the tail. guard.release runs on every path. * fix(node): tombstone bounties when their repo is purged (#196) Bounties are keyed only on (repo_owner, repo_name) with no repo id, and purge left them intact, so recreating the same name re-attached the old open bounty and claim_bounty succeeded against it. Purge now transitions the repo's non-terminal bounties (open/claimed/submitted) to a dead 'purged' status on the delete's own transaction, preserving the financial record. 'purged' is inert against every mutating bounty endpoint, including dispute (which would otherwise reset claimed/ submitted back to open). * fix(node): serialize create_repo against the purge advisory lock (#196) Purge holds a per-owner/name advisory lock across delete-row + remove-dir, but create_repo took no synchronous lock, so a concurrent same-key create slipped into the row-deleted-but-dir-not-removed window and left a repos row pointing at storage the purge then removed. create_repo now acquires the same lock (lock_repo_blocking -> the guard the purge holds, keyed identically) around its existence-check, init, and insert, releasing on every path. Serializes a same-key create against a purge; the INV-9 colliding-sibling case stays the accepted KTD4 residual. * test(node): prove purge deletes at MAX_CONNECTIONS=1 with the split pools (#196) The split pool from the earlier fix already resolves the one-connection self-deadlock (the guard holds a lock_pool connection while the delete runs on the app pool), so this locks that wiring in: a purge at app-pool MAX_CONNECTIONS=1 deletes the target row. The test is load-bearing for the wiring - a shared-pool store (guard and delete on one pool) deadlocks the delete's begin() on the held connection and deletes nothing. No production change; U2 did the wiring. * fix(node): fail the push when the durable upload times out, don't 200 (#196) The bounded release() upload (from the lock-pool fix) abandoned a slow PUT, freed the lock, and returned no failure signal, so receive-pack reported 200 while the archive never reached object storage — and the next acquire_write downloaded the stale pre-push archive over local disk, reverting the pushed refs. A timed-out or errored durable upload now returns upload_ok=false; the handler sends a 5xx and skips the success tail so the client re-pushes idempotently instead of trusting a lost commit. The lock still frees on every path and the connection-hold bound is unchanged. * fix(node): tombstone bounties under both owner forms on purge (#196) The tombstone matched only the repo's full did:key owner, but create_bounty stores repo_owner verbatim from the URL path — commonly the bare short form. A short-owner-form bounty therefore stayed open and re-attached to a recreated repo, the exact hole the tombstone closes for the full form. The UPDATE now matches repo_owner IN (full, normalized short) so both forms are tombstoned. Adds short-form + submitted-status fixtures and asserts dispute cannot reopen a purged bounty. * fix(node): fail closed on a symlink at the purge target (#196) local_refs_on_disk used symlink_metadata only to detect NotFound, then the HEAD/objects marker checks followed a symlink — so a symlink to a real empty bare repo outside repos_dir was admitted as an empty purgeable candidate, and the DB row was deleted before path_within refused the dir removal. The symlink is now classified present/skipped before the marker checks. Adds outside-empty-bare and dangling-symlink tests. * fix(node): retry-after on the push-advert 429, and create_repo locks on the app pool (#196) Two review follow-ups: - The unauthenticated info/refs?service=git-receive-pack limiter still returned a bare 429 with no Retry-After while the other limiters advertise the window-derived delay; it now uses check_retry and the shared too_many_requests helper. Removed the orphaned AppError::TooManyRequests variant and gated the now-test-only check(). - create_repo's transient serialization lock drew from the write lock_pool, so a burst that pins lock_pool starved repo creation. Postgres advisory locks are database-global, so create now takes its lock from the app pool (via lock_repo_blocking_on) and still serializes against a purge holding the same key from lock_pool. * fix(node): correct the create-pool and bounty-tombstone remediations (#196) A re-run adversarial panel caught three issues in the prior round's own fixes: - create_repo took its serialization lock from the app pool and then did its DB work on the same pool, so at MAX_CONNECTIONS=1 it self-deadlocked (a config finding E requires to work). Reverted to locking on the write lock_pool: lock and create-work use separate pools, and the DB-global advisory lock still serializes against a purge. The extreme-burst lock_pool wait is an accepted retryable-503 residual. Db::pool() is test-only again. - The bounty tombstone matched only one owner form and ran unconditionally. It now normalizes BOTH sides (covering did:key-row + short-bounty AND bare-row + full-bounty) and is gated on the same no-surviving-sibling check as the slug cascade, so it never tombstones a bounty a surviving mirror row still serves. Tests: create-at-1-app-connection (no self-deadlock), bare-row full-form tombstone, and sibling-survives-keeps-bounty-open, each RED before / GREEN after. * fix(node): close the durable-upload data-loss class on every write path (#196) The earlier fix that fails a push when its Tigris upload times out was wired only into git_receive_pack; the merge, issue-create, and issue-close write paths still ignored release()'s result and returned 2xx after a failed upload, so the next acquire_write reverted their change from the stale archive. release() is now #[must_use], which surfaces every caller: the production upload paths fail with a 5xx on a false return (so the client retries), and the internal/test callers are explicitly justified. create_repo's lock-acquire failure now returns a retryable 503 (new AppError::Unavailable) instead of a 500, matching its own contract. * refactor(node): route the per-DID 429 through the shared too_many_requests helper * fix(node): init's background upload waits out the creator's advisory lock (#196) create_repo holds the per-repo lock across init, so the wait=false spawn try-locked against its creator and died permanently, leaving a new repo with no object-store archive until its first later access. RED observed: init_upload_waits_out_creators_lock_then_uploads failed (PUT count 0) against the old code; the bounded-skip negative is retained as init_upload_gives_up_after_bounded_lock_wait. * fix(node): roll back the local issue ref before unlock when the durable upload fails (#196) The failed-upload 500 previously left refs/gitlawb/issues/{id} on disk, so a client retry minted a second issue once the store recovered. The delete now runs via RepoWriteGuard::release_with_failure_cleanup while the advisory lock is still held, closing the unlock-to-delete window a concurrent write could have archived the orphan ref through. RED observed on both new tests (orphan ref survived; retry yielded 2 issues); the retry test pins the store double's exists()=false contract so it cannot pass via a stale-archive revert. * fix(node): fork publishes only after a durable upload, serialized on the target-namespace lock (#196) fork_repo now takes the per-repo advisory lock before its conflict check and holds it through clone, upload, and row insert, 503ing on contention exactly as create_repo does. The upload runs under the held guard via upload_under_guard, PUT bounded by release_upload_timeout, returning false only for an attempted upload that failed or timed out (no-store nodes fork unchanged); on failure the mirror is removed and the fork 500s before any row is built. release_after_write is retired; its tests migrated onto the new method. RED observed on the serialization, durability, and contention tests; defect injection (row insert reordered above the upload) turned the durability test RED and was reverted. * fix(node): read-path downloads publish under the purge lock, without pinning it across the network (#196) acquire's cache-miss and every acquire_fresh refresh now coalesce on a per-repo download mutex, download to a temp sibling with no advisory lock held, then try-lock, re-check the archive exists under the lock, and swap - so an in-flight read can no longer resurrect a repository purge just deleted (RED observed on both the cache-miss and refresh-over-existing repros), a post-purge reader degrades instead of publishing, and concurrent cold reads download once. The hit path stays lock-free (deterministic held-lock fence) and no lock-pool connection is held while a download is parked. * fix(node): bound every under-lock object-store call and dedup the upload path (#196) Round-two review found the durability fixes reintroduced the unbounded-work-under-lock class they were meant to close: upload_locked's PUT (now reachable under the lock via init's wait=true), the read-path download, and the under-lock exists() re-check all ran with no timeout while pinning the advisory lock and its lock-pool connection. All three now wrap the call in timeout(release_upload_timeout) and fail closed on elapse, matching RepoWriteGuard::release's existing INV-22 pattern. upload_under_guard and upload_locked's duplicated resolve/recheck/upload body is extracted into upload_dir_locked, threading the load-bearing dir-gone signal (fork needs success, lazy migration needs skip-and-retry) through a parameter rather than aligning it. RED observed: upload_locked_put_stall_is_bounded failed (lock never freed) against the un-timeout'd PUT. * fix(node): harden the read-path download against cancellation, stale swaps, and lock contention (#196) Four read-path gaps the review surfaced. A TempDownloadDir RAII guard now cleans the temp download dir if the handler future is cancelled on client disconnect (the read handlers have no spawn shield, and a post-download disconnect leaked the full populated dir), plus an entry-time sweep of stale siblings. A cold or refresh reader that downloaded while a concurrent writer published fresher on-disk state now discards its stale temp and serves the writer's copy instead of swapping over it (detected by presence for the cold path, mtime for the refresh path). The takeover path re-registers its download-mutex entry so late readers coalesce instead of starting a duplicate download. And the try_lock Ok(None) degrade arm - the KTD-3 invariant that a reader degrades rather than resurrecting under a live writer/purge - is now tested by holding the advisory lock via an out-of-pool observer. RED observed on both stale-swap tests. * fix(node): keep the fork's unbounded work out of the target-lock span (#196) The fork tail held the target-namespace advisory lock across two unbounded operations. The source acquire, which on a cold source triggers a full object-store download plus a nested lock-pool acquire on the same writer-sized pool, now runs before the target lock is taken (it is read-only on a different namespace and nothing under the lock depends on it) so a cold-source fork can no longer demand two connections from a one-per-writer pool at once. The git clone --mirror now runs via tokio::process with kill_on_drop under a timeout(git_service_timeout_secs) ceiling, cleaning the partial mirror and releasing the lock on elapse. RED observed by executing the pool-size-1 cold-source fork with the source acquire moved back under the lock: PoolTimedOut, source never publishes, clone fails. * refactor(node): move read-path download coordination into a repo_store submodule (#196) repo_store.rs had grown past 2800 lines. The download-coordination code (DownloadOutcome, TempDownloadDir, download_published, download_and_publish, remove_download_entry) moves to a private repo_store/download.rs child module, which reaches the parent's private fields and methods without widening visibility beyond pub(super) on the two items the parent names. Pure move: no behavior change, the same 44 git::repo_store tests pass unchanged. * fix(node): bound acquire_write's download and free the download-map entry on cancel (#196) Two gaps a second-model adversarial pass caught that the in-family review missed. acquire_write's under-lock exists()/download() ran with no timeout, so a stalled Tigris GET pinned the write lock and its lock-pool connection forever (the local-copy fallback only fires on an error, never a hang) - both are now wrapped in timeout(release_upload_timeout), a timeout mapping to the existing download-error fallback, which also makes the prior 'bound every under-lock call' claim true. The download_locks map entry was removed only on normal return, not on a cancelled cold read, so a disconnecting caller could grow the map across public repos; a DownloadEntryGuard now prunes it in Drop (outer map moved to std::sync::Mutex, held only for map ops, never across an await). RED observed on both new tests, including a real mid-download cancellation. * fix(node): roll back the fork mirror and archive on late clone/insert failures (#196) The bounded-clone rework cleaned up the mirror on the timeout and spawn arms but not on a clone that creates the destination then exits nonzero, so a retry hit 'destination already exists' and could never complete that fork name. And because upload_under_guard is now required and runs before the row insert, a create_repo failure reliably orphaned the durable archive too, not just the local dir. Both failure arms now remove the mirror, delete the fork's archive (no-op on a store-less node), release the lock, and return. RED observed on both new tests (a non-empty-dest clone-128 repro and a decoy-row insert-failure repro). * fix(node): serialize repo purge cascade with mirror ingest via advisory lock delete_repo_by_id ran a point-in-time sibling check then cascaded slug-scoped deletes; upsert_mirror_repo (peer sync) did not serialize against that window, so a colliding mirror could land mid-purge and get half-wiped. Both now take a transaction-scoped pg_advisory_xact_lock keyed on the normalized slug. (F1, #196) * fix(node): roll back local ref on failed durable upload (close/merge/receive-pack) close_issue, merge_pr, and the receive-pack task applied a local ref write then released with an empty failure cleanup, so a failed Tigris upload left stale state that the local-fast-path read served despite the client getting a 5xx. Each now captures the prior ref and restores it via release_with_failure_cleanup when the upload fails, matching create_issue. (F2, #196) * fix(node): tie cancelled cold-download cleanup to blocking extraction On a download timeout the TempDownloadDir guard dropped while the detached spawn_blocking extraction kept running and could rename its output back into the just-removed temp path; that dir was only reclaimed on a later same-repo request, so cold reads across distinct repos accumulated extracted repos on disk. The timeout arm now hands the owned download future to a detached janitor that awaits extraction completion before removing the temp dir, off the critical path so waiters are still freed immediately. (F3, #196) * chore(node): fmt + clippy cleanup for the #196 fixes Auto-deref (&mut tx) on the lock_repo_slug calls and rustfmt of the new test lines. No behavior change. * fix(node): never let a failed ref snapshot become a delete-all rollback; cover merge and push rollbacks * fix(node): slug-lock every peer-ingest writer and drive the real ingest in the purge-race test * fix(node): own cold-download cleanup in a spawned task so handler cancellation cannot resurrect temp dirs * refactor(node): delegate issue ref helpers to the store ref primitives * fix(node): slug-lock arweave anchor writes; fail closed when the pre-push ref snapshot fails * fix(node): close review residuals: coalesce cold downloads to one in-flight task, compensate a timed-out upload with the rolled-back tree, aggregate partial ref restores, race-test the anchor lock, fail-closed snapshot test * fix(node): re-apply create/fork purge serialization and restore brake coverage The merge-forward kept main's durable-write machinery but dropped two feature pieces the tests now prove are load-bearing: - create_repo/fork_repo take the per-owner/name advisory lock (lock_repo_blocking) across their existence-check -> write -> row-insert span, so neither can land inside a same-key purge's delete-row + remove-dir window. Contention maps to RepoBusy (503). fork_repo also materializes the source before the target lock and bounds the mirror clone, clearing the partial dest on non-zero exit, spawn failure, or timeout. - The per-IP write-brake coverage is restored against the merged router: per-group 429s, write/creation bucket separation, /graphql POST braked while /graphql/ws keeps its DID layer outside the brake, under-limit floors, and the zero-disable knob. The purge serialization tests drive try_lock_repo directly, and the fork durable-write tests run on the shared S3 mock: an ambiguous upload keeps the clone for reconciliation, and a row-insert failure compensates mirror and attempt-stamped archive. * fix(node): close review gaps in purge, locks, and fork cleanup - try_lock_repo_on routes the acquire through LockProbe so cancellation cannot return a lock-owning connection to the pool - run_pending_migrations runs on the connection holding the advisory lock, so a max_connections=1 purge cannot deadlock at startup - purge-spam canonicalizes repos_dir once; the ./repos default's CurDir component made validated_repo_disk_path reject every candidate - delete_archive fences on the generation verified empty; a PUT landing after the check makes the delete refuse instead of erasing it - refresh and archive delete run under transfer_bound while the repo lock is held, so a stalled transfer cannot pin the lock - init_handle returns the archive upload JoinHandle; create_repo awaits it before releasing the creation lock so a purge or retry cannot find the row with the PUT still in flight - ForkCloneGuard is created before the clone and claim_fork_disk_path creates the owner dir for the stamp; every clone failure, timeout, and cancellation path now cleans only what this attempt owns * fix(node): subscriptions-only ws schema and unsettled-work purge guard - /graphql/ws serves a schema with no mutation root, so the socket can no longer execute task mutations outside the write-brake bucket; queries and subscriptions still run, and the schema carries the same Db, broadcast, and cursor-key data the resolvers consume - purge-spam skips a repo that still has unsettled receive-pack requests or pending ref transitions (checked under the repo lock, fail-closed) instead of stranding that work mid-effect * fix(node): wire close_issue rollback, close purge symlink gap, correct ws doc - close_issue snapshots the pre-close JSON and uses release_compensating, so a failed durable upload restores the ref instead of leaving the local repo closed while the client saw a 5xx (mirroring create_issue) - ref_count_on_disk rejects a symlink at the repo path, matching the selection-side fail-closed rule so the execute recheck cannot follow a link to an empty bare repo outside repos_dir - build_ws_schema rustdoc now states mutations are disabled via EmptyMutation while queries still run on QueryRoot * fix(node): close purge/upload lifecycle gaps from review - purge-spam execute: the object-store archive delete now runs on every successful row delete; a refused or failed local dir removal no longer strands the archive (P1) - purge-spam summary: skipped_unsettled is now printed, and a dedicated skipped_unsettled_probe counter records fail-closed skips when the unsettled-work probe itself errors (P2) - close_issue: rollback compensation uses create_issue_bounded, the same deadline-bounded git strategy as create_issue's compensator, so a hung git cannot stall the write guard past git_service_timeout (P2) - create_repo: the initial archive upload's outcome now gates the 201; a failed or fenced upload rolls back the row and disk dir and surfaces an error instead of reporting success (P2) - purge-spam: tigris_bucket is clap-global so --tigris-bucket parses after the subcommand (P3) * fix(node): merge rollback, honest create rollback, purge row recheck - merge_pr snapshots the pre-merge target tip and uses release_compensating to rewind it via bounded update-ref on a refused publish, matching the close_issue pattern; regression proves the ref returns to its prior OID after a refused upload (P1) - create_repo rollback names residual state instead of claiming a full rollback when the row delete fails or dir removal fails (P2) - purge execute re-checks row absence via get_repo_by_id immediately before remove_dir_all and refuses the disk delete when a row reappeared after commit, closing the post-commit mirror-ingest window (P2) - sweep_evicts_expired_keys_from_every_limiter now seeds and overrides the write_rate_limiter TTL (P3) * fix(node): probe purge slug liveness, gate issue write brake behind the read gate (#196) Three items from review round 6: - merge_pr rollback test seeded commits with git commit-tree under no configured identity, which fails on CI runners. The helper now passes -c user.name/-c user.email per invocation. - run_purge_spam's post-delete guard probed the deleted primary key, but a concurrent upsert_mirror_repo lands a NEW row for the same slug with a mirror id, and the archive delete ran unconditionally. The probe now reads slug liveness via get_repo(owner, name) and gates BOTH the disk and archive deletes, counted as skipped_slug_live. - issue_write_routes carried the outer per-IP write brake, so an exhausted bucket answered 429 before authorize_repo_read could return the opaque 404 a hidden repo owes the caller. The brake is now debited inside close_issue and create_issue_comment, immediately after the read gate; the route group carries no outer brake. * fix(node): close-issue rollback uses resolved ref, drop router brake test, tag fixtures get -m (#196) - close_issue's compensating rollback now rewrites the resolved full issue id; a close-by-prefix refused at publish left the full ref closed while the client saw 5xx (P2, regression test included) - issue_comment_is_rate_limited_by_ip asserted an outer 429 the issue write routes deliberately lack (P1); issues.rs owns the ordering - git tag fixtures pass -m so dumb CI terminals do not hit "EDITOR unset" * fix(node): close_issue spends neither bucket when either cap refuses, complete write-brake operator copy (#196) close_issue chained two consuming limiter checks, so a close refused by its own cap still debited the shared write bucket. Probe both with is_throttled first and consume only when both have room. Rewrite the write-brake consumer list in .env.example, lib.rs and state.rs to cover every surface on the bucket. * fix(node): refund the write slot when the close cap refuses, send Retry-After on write-bucket 429s (#196) close_issue debited the write bucket and then the close bucket, so a concurrent close taking the last close slot between the probe and the check left the write slot spent with no close done. Drop the probes: debit write, then close, and refund the write slot when close refuses. The in-handler write-bucket refusals in close_issue and create_issue_comment now return AppError::RateLimited, which maps to 429 with a window-derived Retry-After like the router middleware's 429s. The close-cap-only refusal keeps its plain 429.	2026-11-13 01:52:04 +00:00
beardthelionandVasanthdev2004	3a0d91a801	feat(node)!: anchor the UCAN proof chain and honour delegated git/push (#567) ... * feat(core)!: verify_chain returns the proof chain's root issuer verify_chain returned Result<()>, so a caller learned that a chain was internally consistent and nothing else. `did:key` is self-certifying: anyone can mint a keypair, self-issue `*` on `*`, and produce a token that verifies. That is correct UCAN behaviour rather than a defect — a token with no proofs is its own root — and it still verifies after this change. But it does mean "the chain verifies" is not an authorization answer on its own, and without the root identity no caller could ask the question that actually decides a push: does this chain rest on someone this repository trusts? Returning the root means a caller cannot accept a chain without being handed the identity it rests on. Callers that legitimately do not care — the node middleware checking that a bootstrap token is well-formed, for one — discard it explicitly. The doc comment now states outright that this establishes internal consistency and not trust, and names what a caller has to compare the root against. Multi-proof chains are refused: more than one proof means more than one root, and nothing says which root authorized a given capability, so returning any single one would be unsound. The old loop followed every proof but returned nothing, so a two-proof chain verified silently; the new test pinned that by observing the pre-fix code accept one. Ucan::delegate only ever writes one proof, so no token this codebase produces is affected. BREAKING CHANGE: Ucan::verify_chain returns Result<Did> instead of Result<()>. * fix(node): gate two Unix-only tests so the crate compiles on Windows Two tests build a fake git as a `/bin/sh` script, mark it executable through `std::os::unix::fs::PermissionsExt`, and reap the hung `rev-list` with `libc::kill(SIGKILL)`. None of that exists on Windows, and neither test was cfg-gated, so `cargo test -p gitlawb-node` failed to compile there with six errors before running anything. A Windows checkout could not run a single test in the crate, including the ones that are platform-independent. Gate both with `#[cfg(unix)]`. The attribute is a no-op on Linux, so CI keeps running them exactly as before; only the Windows build changes, from "does not compile" to "runs the platform-independent tests". Refs #228 * feat(node): carry the verified UCAN and its root into the request require_ucan_chain validated a presented token and discarded it, so no handler could read the result and Ucan::can had no call site anywhere in the node. A UCAN could only ever fail a request, never authorize one. The middleware's own behaviour is unchanged — an absent header passes through, an invalid chain is still 401. It now also parks the verified token and the root issuer its chain rests on in request extensions, so an authorization decision downstream can use them. The root is stored rather than recomputed so the chain is walked once per request. Holding a VerifiedUcan is deliberately not an authorization decision. did:key is self-certifying, so a chain that verifies proves only internal consistency; the caller has to compare the root against an identity it trusts for the resource being touched. The next commit adds that comparison for git/push. * feat(node): add ucan_grants_push, anchored at the repo owner The predicate that decides whether a delegated capability authorizes a push. The chain must root at the repo owner — data the node holds independently of the token, which is what a self-minted chain cannot forge — and the leaf must carry git/push for this repo. Only the leaf is examined for the capability. verify_chain has already established that each leaf capability is attenuated by its proof, transitively to the root, so a surviving leaf capability is no broader than what the root granted; re-walking the chain would duplicate that guarantee. Resource matching is structural rather than a string compare because owner_did is stored full on canonical rows and bare on mirror rows; a literal match would deny valid delegations for every mirror, which is a defect that would have looked like a permissions bug rather than a parsing one. A capability carrying nb authorizes nothing while constraints stay uninterpreted, so an owner who writes a ref restriction never accidentally grants repo-wide push. Not yet wired into the push path — the next commit does that — so a non-test build still reports VerifiedUcan::ucan as unread. * feat(node): honour a delegated git/push capability on the push path caller_authorized_to_push becomes owner || delegated, exactly the Phase 2 its own doc comment described. The owner check is unconditional and runs first, so the UCAN path can only ever turn a 403 into a 200, never the reverse. A push carrying no X-Ucan reaches the same owner-only decision it did before. git_receive_pack takes the verified token as Option<Extension<VerifiedUcan>>; axum extracts an absent extension as None rather than rejecting, so the optional header stays optional. The denial message is deliberately identical whether a delegation was absent, expired, or named a different repository. A behavioural test asserts the two bodies are byte-identical, because a difference would turn the refusal into an oracle for which capabilities exist. The behavioural test drives both auth layers with a real RFC 9421 signature and a real invocation, and discriminates on status: 500 means the request passed require_signature, passed require_ucan_chain, cleared the owner gate, and reached git on a repo with no disk backing. A bare `!= 403` would let a 401 regression through. It needs no fake-git shim, so unlike the rest of the push path it is not cfg(unix) and runs on every platform. Both new tests were verified to fail: with the delegation branch removed the behavioural test reports 403 where it wants 500, and the unit test's assertion fires. A test written after its implementation proves nothing until it has been watched failing. * feat(gl): gl ucan import stores a delegation for the push helper The git remote helper needs a delegation on disk in a location it can derive from a gitlawb:// URL alone. Files key on the bare base58 owner key because did:key contains a colon, which Windows will not accept in a filename, and the same identity appears in both full and bare form across this codebase — storing under one and looking up by the other would silently miss. Import decodes the token so a malformed delegation fails here, where the error is actionable, rather than surfacing as an unexplained 403 in the middle of a git push. A delegation naming no repository is rejected with the resources it did carry, since that is almost always a wrong --cap argument. * feat(git-remote): send a delegated push capability as X-Ucan Wraps a stored delegation into an invocation (iss=agent, aud=node, prf=[delegation]) and attaches it to the receive-pack POST, which is what lets a CI or delegated key push once owner-only push is enforced. Only on the push: a fetch is gated by read visibility, not by git/push. Capabilities are copied from the delegation unchanged, so an invocation can never be broader than what was delegated. No expiry is set on the invocation itself — the delegation`s own exp still bounds the chain because verify_chain checks each proof`s expiry as it recurses. A test proves that rather than asserting it: an already-expired delegation still fails the chain after wrapping, so an expired grant cannot be laundered into an open-ended one. The whole path is best-effort. A missing delegation, an unreachable node, or an unreadable stored token all send the request without the header. The node decides whether one was required, and a node denial has to reach the user instead of being pre-empted by a local guess. delegation_path duplicates gl`s six-line version because gl is not a dependency of this crate; both carry a pointer to the other. * test(git-remote): cover pack-POST URL splitting split_pack_post_url decides which repo a delegation is looked up for, so a parsing slip silently means no delegation was found, surfacing as a confusing 403 rather than a visible error. Covers the optional .git suffix, a repo genuinely named x.git, and the malformed shapes that must not parse. * build: sync Cargo.lock for the git-remote-gitlawb dependencies Adding serde_json and chrono to crates/git-remote-gitlawb/Cargo.toml updated Cargo.lock locally, but the lockfile lives at the repo root and was never staged. Every CI job builds with --locked, so all seven compiling jobs failed immediately with "cannot update the lock file ... because --locked was passed". * fix(core,gl,git-remote): close two review findings on delegated push Both are mine, both were found in review, and both are load-bearing. 1. Stripping `nb` mid-chain escalated a constrained delegation. `Capability::is_attenuated_by` compared only `with` and `can`, so the holder of a capability constrained by `nb` could re-delegate the same resource and action with the constraints removed and `verify_chain` still accepted the chain. The node's push gate refuses constrained capabilities at the LEAF, so it then saw an unconstrained one and granted repo-wide push — guarding the leaf guarded the wrong end of the chain. Constraints now participate in attenuation: an unconstrained parent permits anything, an identical child is attenuated, and both a differing child and a child that drops the constraints are refused. `nb` has no interpreted semantics yet, so "different" cannot be shown to be narrower and is refused with the stripping case. Tests cover the forged strip, the legal unchanged case, and adding constraints under an unconstrained parent. 2. `gl ucan import` wrote to a path taken from an untrusted token. `repo_from_resource` split the resource with `rsplit_once('/')`, so the owner half could carry separators, `..`, or an absolute prefix. `Path::join` with an absolute component discards the base entirely, so a resource of `gitlawb://repos/C:/Windows/System32/x` did not merely climb out of the delegations directory — it replaced it. The value then reached `std::fs::write`. The resource must now be exactly `gitlawb://repos/<owner>/<repo>`, with each half an allow-listed component: alphanumerics plus `.`, `-`, `_` and `:` (a DID carries colons), never `.` or `..` or anything containing `..`. An allow-list rather than a deny-list of separators, which would miss whichever ones the next platform introduces. The same shape existed in the helper's `split_pack_post_url`. There the value comes from the remote URL and the path is only read, but a traversing owner would read an arbitrary file and send its contents to the node as `X-Ucan`, so it carries the same guard. Also: the node-DID probe now uses a 5s timeout instead of inheriting the shared client's 300s. It is best-effort metadata, and a stalled node should not delay every delegated push by five minutes before falling back to sending no header. * fix(core,node,gl,git-remote): close the second review round on delegated push Each finding was reproduced against the code before being changed; two were confirmed with the mutation the reviewer described. A delegation could be perpetual -------------------------------- `exp` is optional, `is_expired` reports false when it is absent, `gl ucan delegate` defaulted to no expiry, and there is no revocation path. So the default flow minted a permanent push grant: once the token leaked, the owner's only remedy was rotating the DID the repo is keyed on. The PR body claimed the damage window was the token's `exp`, which was simply untrue when `exp` was `None`. `Ucan::chain_lifetime_is_bounded` walks every link, and `ucan_grants_push` requires it. It is deliberately NOT enforced inside `verify_chain`: an unbounded token is well-formed and may suit a read-only capability; whether an unbounded grant is acceptable is the consumer's policy, not the format's. `gl ucan delegate` now defaults to 720 hours with an explicit `--no-expiry` opt-out, and the helper carries the delegation's expiry onto the invocation so the leaf is bounded too. The recursion to the root was untested --------------------------------------- Every chain in the suite was depth two, where the immediate proof IS the root, so nothing distinguished walking to the true root from returning the proof's issuer. Confirmed by mutation: keeping full recursive validation but returning `proof.payload.iss` left gitlawb-core at 92 passed, the node's UCAN tests at 17, and the e2e green. A three-link owner -> lead -> agent test now pins it, with `assert_ne!` against the middle issuer as well as `assert_eq!` against the root — without the former, returning the middle would still pass. A path-prefixed node base broke delegated push entirely -------------------------------------------------------- `GITLAWB_NODE` may carry a path (`https://host/gitlawb` behind a proxy), and `repo_base` passes it through verbatim. `split_pack_post_url` read the FIRST two segments as owner/repo, so the prefix became the owner: the delegation lookup missed, the DID probe hit the wrong URL, no `X-Ucan` was sent, and a valid delegate got a 403 — silently, because every failure on that path is best-effort. It now strips the known trailing `<owner>/<repo>/<service>` instead, which is correct at any prefix depth, and prefix segments carry the same allow-list so a `..` cannot redirect the probe. A wildcard delegation grew without limit ----------------------------------------- `with: "*"` covered every repo the owner created AFTER signing, a scope nobody chose. `build_invocation` now narrows to the concrete repo, which `is_attenuated_by` accepts under a `*` parent, so a captured invocation is worth one repo rather than all of them. Constraints are copied from the covering capability rather than dropped, since dropping them is a widening. Branch protection and the owner gate now disagree, on purpose -------------------------------------------------------------- A delegate clears the owner gate and is still refused on a protected branch. That is the intended policy: a protected branch is the owner's explicit marker that even routine writes should stop, and if a delegation overrode it, issuing any capability would weaken every protection already set. The comment claiming non-owners never reach that loop is corrected, and `delegated_push_is_still_refused_on_a_protected_branch` pins it — asserting the body names the branch, so the refusal is provably branch protection rather than the owner gate. Smaller items ------------- The denial body no longer says "only the repo owner may push", which stopped being true once a delegation could authorize one. It stays a single unconditional message: varying it by whether a delegation was presented, expired, or named another repo would turn the refusal into an oracle for which capabilities exist. `gl ucan import` writes the delegation 0600, matching the sibling identity key. The token alone cannot push — the node requires `iss` to equal the request signer — but it discloses the delegation graph. `docs/RUN-A-NODE.md` told operators not to enable owner-push until every pusher was the owner, which is the workflow this change adds. It now documents the delegation flow, the four requirements the node enforces and why, that a delegation does not override branch protection, and that withdrawal is by expiry only. `README.md` no longer describes UCAN as a future workflow. * fix(gl): restrict the delegations directory, and state the contract The imported delegation was written 0600 on Unix and left to inherit the directory ACL elsewhere. Tightening only that one file would have been theatre: identity.pem sits in the same directory under exactly the same assumption, and its disclosure is strictly worse. The directory itself is now 0700 on Unix, and the comment states the contract that already governed the private key — gitlawb_dir accepts any path, std::fs has no portable ACL API, so on other targets the caller must supply a user-private directory, which a per-user profile provides by default. * fix(gl,git-remote): make a delegation issued the documented way actually work Two silent failures, both of which end as a 403 telling the delegate to obtain the delegation they are already holding. The narrowing fix broke the documented issuing form ---------------------------------------------------- `build_invocation` built `gitlawb://repos/{owner}/{repo}` from the push URL and compared it to the delegation's `with` with `==`. The URL always carries the BARE owner, since `parse_gitlawb_url` takes the last colon-delimited segment, while `docs/RUN-A-NODE.md` tells the owner to issue `--cap gitlawb://repos/<owner-did>/<repo>` — the full DID. Those strings never match, `find` returns None, and because every failure in `delegation_header` is best-effort the push goes out with no `X-Ucan`. This was introduced by the previous round: copying `att` through unchanged worked because the node normalizes both forms in `did_matches`. Narrowing to a URL-derived string did not. The owner segment is now compared on the bare key, and the parent's `with` is kept VERBATIM whenever it already names this repo — `is_attenuated_by` compares `with` by exact equality, so re-emitting the bare form under a full-DID parent would fail attenuation at the node and trade one silent refusal for another. Only a `*` parent uses the URL-derived resource, which is the case narrowing exists for. Both combinations are now tested; neither side exercised them before. The two halves disagreed about where delegations live ------------------------------------------------------ `git-remote-gitlawb` resolves its store from `resolve_key_path().parent()`, which honors `GITLAWB_KEY`. `gl ucan import` wrote under `gitlawb_dir(None)`, which was always `~/.gitlawb`. With `GITLAWB_KEY=/data/keys/identity.pem` — the shape `.env.example` documents — the import stored the token in one directory and the helper read another, empty one. `gitlawb_dir` now falls back to the parent of `GITLAWB_KEY` before `~/.gitlawb`, so both halves derive the store from the same setting. Also: `.env.example` still said a push from a non-owner DID is rejected, which is the behaviour this branch removes. * fix(gl): refuse a relative or non-UTF-8 GITLAWB_KEY when deriving the store Two defects in the previous round's fix, both of which put the delegation somewhere the helper does not look and end as an unexplained 403. A relative GITLAWB_KEY resolved differently in each half. `gl ucan import` and `git-remote-gitlawb` do not share a working directory, so `keys/identity.pem` sends the import to one `delegations` directory and the lookup to another. A one-component value is worse: `parent()` yields "", so the store becomes `./delegations` relative to whatever directory happened to be current. It is now refused with a message that says why, rather than silently resolved. `std::env::var` returns Err for a non-UTF-8 value, which the code treated as unset — indistinguishable from having no GITLAWB_KEY at all, and silently selecting ~/.gitlawb instead of the operator's real key directory. `var_os` keeps the OsString, so a valid non-UTF-8 path now works and an empty one is still treated as unset. * fix(identity): one resolver for GITLAWB_KEY across gl and the helper `gl` and `git-remote-gitlawb` each carried their own answer to "where is my identity?", and so did five call sites inside `gl` itself. The round-three fix hardened one of them — `gitlawb_dir` — and left the rest, which is why the same misconfiguration kept surfacing in a new place each round. The rules now live in `gitlawb-core::identity_path`, the crate both binaries already depend on: identity_key_path() $GITLAWB_KEY, else ~/.gitlawb/identity.pem identity_dir() its parent — the delegation store read through `var_os` (so a non-UTF-8 path is refused rather than folded into "unset" by `var`), with `~/` expanded on the first path component, a relative value refused, and a bare `~`, `~/`, or `/` refused rather than resolved to a directory whose parent is not where anything lives. Both take the home directory as an argument rather than looking it up: core is held to an explicit dependency allowlist and both callers already carry `dirs`, so the rules can be shared without widening core's tree. It also lets every case be tested against a fixed home instead of the machine's. Call sites moved onto it: gl identity::gitlawb_dir delegates to identity_dir gl identity::load_keypair_from_dir was ~/.gitlawb even after `gl identity new` wrote elsewhere, so `gl ucan delegate` signed with a stale DID — or found nothing — for exactly the operators who moved their key. Every `load_keypair_from_dir(None)` caller (register, repo, pr, clone, mcp) is fixed with it. gl doctor::run the one command whose job is to explain a broken setup was reporting on a directory the setup does not use gl init (ucan.json, generate_identity) gl mcp ucan_show gl ucan_cmd::cmd_show helper resolve_key_path / the delegation store behind delegation_header The helper's version was the loosest of the set: `env::var`, a literal `"~/"` prefix, `HOME` falling back to `"."` (never set on Windows), and no absolute check at all. Also in this commit: - `gl ucan import` creates the store and the token at their final mode. `create_dir_all` then chmod leaves 0755 under the usual umask, and `fs::write` then chmod leaves 0644, both readable by any local user until the second call lands. `DirBuilder::mode` and `OpenOptions::mode` close the window; the trailing `set_permissions` now only matters for a store an older `gl` left behind. - `gl ucan import` refuses a delegation the push path cannot use. It filtered on resource shape only, so a `pr/open` token printed "Stored delegation for owner/repo" and was then dropped by `build_invocation` behind a `tracing::warn`, surfacing as a 403 with nothing connecting it to the earlier success. Import now applies the same push-class filter the helper does. - README's write-authorization limitation is narrowed to what is actually missing (revocation, `nb` interpretation, non-push capabilities) rather than claiming delegated push is not implemented. Every new guard was verified by disabling it and watching the matching test go red: the absolute-path check, the bare-`~` refusal, the `load_keypair_from_dir` routing, and the import action filter each own a failing test. * test(git-remote): cover delegation_header end to end `delegation_header` was the one piece of the delegated push path with no test. Its parts each had one — `split_pack_post_url`, `build_invocation`, `delegation_path` — but nothing checked they compose, and this is the function where a regression is silent by construction: every failure inside it returns `None` and the push goes out without `X-Ucan`, so a break surfaces as a 403 at the node with nothing locally to connect it to. Five cases, driven against a mockito node with the store seeded where `gl ucan import` would have left it: - the delegated push: a stored token becomes an invocation issued by the agent, addressed to the node's DID, rooted at the repo owner, with every link bounded - the owner's own push: no store read, no node round-trip. The comparison has to survive the form mismatch — the keypair holds `did:key:z…` while the URL carries the bare key — so the mock asserts zero hits - no usable delegation: an empty store, a token that does not decode, and a `git/fetch` capability that carries nothing to wrap. All three yield no header rather than an error - an unreadable node DID: a 500, a JSON body with no `did`, a proxy's HTML error page, and a `did` that does not parse. The push loses its header, never aborts - a path-prefixed node base: the DID probe must go to `/gitlawb`, not `/`. `split_pack_post_url` is unit-tested for the prefix, but nothing checked the probe followed it; the mock on `/` asserts zero hits Each case was watched failing before it was kept, against five separate mutations: the owner short-circuit removed, the prefix dropped from the node base, `build_invocation`'s push-class filter widened, the node-DID lookup given a fallback, and the invocation addressed to the agent instead of the node. * fix(gl): finish the identity-path sweep on the write side The previous commit unified where `gl` READS the identity from and stopped there. Four resolvers were left holding their own `~/.gitlawb`, and one of them is a write: register.rs ucan_path the bootstrap UCAN from POST /api/register quickstart.rs the wizard dir identity generation AND the UCAN it stores name.rs identity_dir reads identity.pem node_stake.rs load_did reads identity.pem `register.rs` is the one that actually breaks a working setup. With `GITLAWB_KEY=/data/keys/identity.pem` — the shape `.env.example` documents — `gl register` loaded the key from `/data/keys/identity.pem` and then wrote `ucan.json` to `~/.gitlawb/`. Registration reported success; `gl doctor`, `gl ucan show`, `gl init`, and `gl mcp ucan_show` all read `ucan.json` from the key's directory, found nothing, and reported an unregistered identity. Split storage, no error anywhere. `quickstart` had the same shape end to end: it generates the identity and stores the bootstrap token, so an operator who ran the wizard under `GITLAWB_KEY` got both in a directory nothing else consults. `name.rs` and `node_stake.rs` only read, but both fell back to `PathBuf::from(".")` when the home directory could not be determined, which makes the identity path depend on the working directory — the same defect the helper's `resolve_key_path` carried before it moved to the shared resolver. All four now go through `crate::identity::gitlawb_dir`. Afterwards the only `dirs::home_dir()` calls left in `gl` are the one inside `gitlawb_dir` itself, one test expectation, and `doctor`'s shell-rc scan, which is unrelated to the identity directory. `gl register` also no longer prints a path it did not use: the success line was hardcoded to `~/.gitlawb/ucan.json`, so an operator with `GITLAWB_KEY` set was told to look somewhere the file was not. It now prints the real destination, and says so plainly when the node returned no token at all. The `GITLAWB_KEY` test guard moves to `identity::test_env` so the resolver's own cases and the new register case share one lock. Two suites each holding their own mutex over the same process-global variable would not serialise against each other. Verified by reverting `ucan_path` to the hardcoded form and watching `register_saves_the_bootstrap_ucan_beside_the_key` fail on exactly that assertion. * fix(gl,core): close round five — lazy home, envelope reads, honest register Five findings from the round-five review, four of them introduced by round four's own centralisation. resolve_key_path and resolve_identity_dir opened with `home_dir()?`, but identity_key_path only needs a home when the key is unset, empty, or `~/`-prefixed. On a host where `dirs::home_dir()` returns None — no HOME and no passwd entry, an ordinary container shape — a perfectly valid absolute GITLAWB_KEY was discarded and the push went out unsigned, blaming the home directory for a setting the operator had got right. The pre-round code resolved an absolute key without consulting home at all, so this was a regression, not an inherited gap. The core resolvers now take `Option<&Path>` and demand a home only where the value being resolved needs one. `gl ucan show` called Ucan::decode on the whole of ucan.json, but register, init, and quickstart all write an envelope — {"ucan", "node", "did", "saved_at"} — and doctor and quickstart already read it as one. Ucan is {payload, s}, so show failed with "missing field `payload`" immediately after a successful `gl register`. It now reads the envelope like every other reader, and still accepts a bare token so files written by an older gl stay readable. The predating mismatch belongs to this round because this round rerouted cmd_show and claimed to unify the identity flow. `gl register` printed "You are now a verified agent" even when the node returned no bootstrap UCAN — the exact shape the previous commit message said it existed to remove, one line below the fix. The closing line is now inside the branch that actually stored a token. The MCP `ucan_show` tool called gitlawb_dir(None) while every sibling tool in call_tool honours the server's --dir, so one session read two identity directories. The parameter was already in scope. Six `--dir` help texts still promised "default: ~/.gitlawb" after the resolver stopped having that default. Both behavioural fixes were verified by mutation: demanding a home unconditionally turns an_absolute_key_resolves_without_a_home_directory red, and dropping the envelope branch turns the_register_envelope_decodes red. * fix(node,gl,git-remote): enforce repo narrowing at the node, honour the key basename Round seven. Two P1s, and the first changes an authorization outcome. The node accepted a leaf resource of `*` for delegated push. Narrowing a wildcard delegation to the concrete repository happened only in `git-remote-gitlawb`, which is one client's behaviour and not a protocol boundary: a delegate can sign an otherwise valid `agent -> node` invocation that keeps `with: "*"`, it passes attenuation because its proof is `"*"` too, and the node then authorised a push to any repository owned by the root DID — including repositories created after the delegation was issued. `docs/RUN-A-NODE.md` promised exactly the opposite, and named the growth it was preventing, while resting that promise on the helper. `repo_capability_matches` now requires a concrete `gitlawb://repos/<owner>/<repo>`. Wildcard delegations still work through the normal path, because the helper narrows before signing; only a hand-built wildcard leaf is refused. `honours_the_resource_wildcard_and_repo_admin` asserted the old behaviour, so it is now `refuses_a_resource_wildcard_and_honours_repo_admin`, and it separates the two axes: a `*` ACTION on a concrete repository is still fine, since attenuation bounds it. `a_wildcard_delegation_cannot_reach_a_second_repository` covers the attack directly. `gl` discarded the basename of `GITLAWB_KEY`. The shared resolver returns the configured PEM path, but `gl` kept only its parent and re-appended `identity.pem`, while the helper opens the configured path. With `GITLAWB_KEY=/data/keys/ci-agent.pem` the CLI wrote and read `/data/keys/identity.pem` while every push loaded `/data/keys/ci-agent.pem`, so the two either disagreed on identity or the helper found no key — and both owner enforcement and the delegation proof key off that identity. This was mine, from last round's centralisation: a directory-level contract substituted where callers needed a key-file-level one. `key_path_for` now serves both, and the seven production sites that still hardcoded the basename go through it: `identity` (load/new/backup/restore), `doctor`, `init`, `name` (x2), `node_stake`, `quickstart` (x2). An explicit `--dir` keeps `<dir>/identity.pem`. Also: - `gl ucan import` binds the delegation's audience to the local identity and verifies its chain, expiry, and bounded lifetime before storing. It previously accepted a token addressed to a different agent, reported success, and left the push to fail on proof linkage with nothing pointing back. - `build_invocation` selects only a capability the node can authorise. It took the first push-class capability, including a constrained one, while the node refuses constrained leaves outright — so a delegation holding both a constrained and an unconstrained grant succeeded or failed purely on their order in `att`. - Delegation files are replaced atomically: written to a sibling temp at 0600, fsynced, then renamed. `truncate(true)` on the live path emptied a working delegation before the replacement was written, so an interruption left the helper with an unreadable token and pushes that silently drop `X-Ucan`. - `gl quickstart` no longer reports "UCAN saved to ..." when the node returned no token. Both reviewers flagged this independently; it is the same false success fixed in `register` last round, one file over. Every fix was verified by mutation: restoring the node's wildcard acceptance, restoring `<parent>/identity.pem`, disabling the audience check, and dropping the helper's constraint filter each turn the matching test red. * fix(gl): align the MCP ucan_show tool with gl ucan show The two surfaces answer the same question and answered it in different shapes: `gl ucan show` decodes the envelope and prints fields, while the MCP tool returned the file verbatim. That was the round-seven non-blocking note. Aligning them removes a second problem the note did not name. The file contains the bootstrap token, so the MCP tool handed the caller the credential itself — and an MCP response travels further than a terminal does, into a model's context, transcripts, and logs. Nothing needs the token to answer "am I registered, with what, until when", so the tool now returns issuer, audience, version, capabilities, expiry, expired, signature validity, and the path it read. The tool description said "Show the saved bootstrap UCAN token"; it now says what is returned and that the token is not. Verified by mutation: restoring the raw-file return turns `ucan_show_returns_decoded_fields_and_withholds_the_token` red on the assertion that the token is absent. * fix(gl): drop a needless borrow in quickstart's unix key write `generate_identity` now takes `path: &Path` rather than deriving it from an owned directory, so `set_permissions(&path, ..)` became a double borrow. Clippy refuses it under `-D warnings`. It sits inside `#[cfg(unix)]`, which is why the local gate passed: this machine is Windows, so that block is never compiled or linted here. Only CI sees it. The sibling `fs::write` on the line above was already corrected; this one was missed because the same pass could not fail locally. The other `&path` / `&dest` / `&tmp` borrows in the unix blocks this PR touches are on owned `PathBuf`s, where the borrow is required — `quickstart` is the sole place a parameter changed from owned to reference, and so the sole place the lint applies. * fix(node,gl,git-remote): fix a delegation's scope at issuance Round eight. The round-seven wildcard fix closed the wrong half, and I confirmed that by execution before changing anything: PROBE ucan_grants_push = true An owner issues `with: "*"`, the delegate narrows it to a concrete leaf for a repository created afterwards, `verify_chain` passes, and the node authorised the push. Refusing a wildcard LEAF did nothing, because `is_attenuated_by` accepts a concrete child under a `*` parent — and that narrowing is exactly what `build_invocation` performed, so the first-party helper was the working mint path. The round-seven tests never saw it: they injected a self-issued `*` leaf with no `prf`, so they never touched a proof chain at all. The contract is now that a delegation's scope is fixed when it is issued, which is what both operator documents already promised: - The node requires EVERY link to name the repository, not just the leaf (`proofs_name_repo` walks `prf` after `verify_chain` has established the chain). - `git-remote-gitlawb` no longer selects or narrows a `*` capability, so a stored wildcard fails closed locally with a clear message instead of minting an invocation the node would refuse. - `gl ucan delegate` refuses to issue a push-class wildcard at all. Also, all from round eight: - `gl quickstart` loads the exact key it selected. It resolved `pem_path` through `key_path_for` but then loaded through `Some(&dir)`, which re-derived the basename as `identity.pem` — so with `GITLAWB_KEY=/data/keys/ci-agent.pem` the existence check and the load disagreed, and the Err arm regenerated ONTO `ci-agent.pem`, destroying a working key and changing the DID that repository ownership, registrations, and delegations all key off. - `gl ucan import` binds every capability's owner segment to the VERIFIED ROOT. `verify_chain` proves a chain is internally valid; it says nothing about which repository it applies to. Without this, any key holder could issue a valid bounded token naming `gitlawb://repos/<victim>/repo` and displace the working delegation for a repository they have no authority over. The whole set is validated before anything is written, so a later bad capability cannot leave a multi-repository import half applied. - Import applies the same push-class rule the helper and node use, including `constraints.is_none()`. A constrained-only grant used to import cleanly and then authorise nothing. - Delegation replacement stages to a per-call path (`create_new`, pid + counter) rather than a shared `.<name>.tmp`, so two concurrent refreshes cannot write the same inode and publish each other's bytes. The non-Unix path stages and renames too; it previously truncated the live file, so a failed refresh destroyed a working token on that platform. Three tests that were weaker than they read, all mine: - `a_refused_reimport_leaves_the_stored_delegation_intact` never reached `write_private_file` — it stopped at the audience check. Renamed to say what it covers, and `a_failed_write_leaves_the_stored_delegation_intact` now forces a real write failure against a read-only store. - The MCP token-withholding assertion used `!out.contains(&token)`, which passes whether or not the token leaked: the token is itself JSON, so embedding it escapes every quote. It now asserts on the signature, a bare base64 string that survives escaping. - `build_invocation_narrows_a_wildcard_to_the_pushed_repo` asserted the behaviour this commit removes; it is now `build_invocation_refuses_a_wildcard_delegation`. * fix(gl): drop an unused import in the unix-only refresh tests `refresh_atomicity_tests` moved from `token_for_agent` to `owned_token` when import began requiring the resource owner to equal the verified root, and the import was left behind. The module is `#[cfg(all(test, unix))]`, so it does not compile on this Windows host and no local clippy run could see it. Cross-linting is not available either: `cargo clippy --target x86_64-unknown-linux-gnu` fails in ring's build script for want of a C cross-compiler. CI is the only linter for cfg(unix) code from here, which is worth recording rather than rediscovering. The sibling `import_binding_tests` still uses both helpers; only the unix module's import changed. * fix(node): pass the UCAN extension at the last two receive-pack call sites Two `git_receive_pack` calls in `#[cfg(unix)]` test blocks still supplied six arguments. Like the rest of that class they do not compile on this Windows host, so neither the local build nor clippy could see them; CI found them one at a time. Rather than wait for a third round, every call site was audited by grep instead of by compiler: 25 calls, 2 missing, both fixed, none others. The other signatures this branch changed — `caller_authorized_to_push`, `identity_key_path`, `identity_dir`, `key_path_for` — were audited the same way and are consistent. Both sites push as a synthetic non-owner DID under an owner-push-disabled config, so `None` is correct: no delegation is involved in what they test (write-lock release on disconnect, and on push success). * fix(core,node,gl,git-remote): one definition of a usable push capability Round nine. Both reviewers named the same pattern independently: the rule for "can this token push to this repository?" was reconstructed at every boundary — issuance, import, invocation construction, node authorization — and each round found a token one boundary accepted and the next refused. This round's own instance: the node walked the whole proof chain, import checked only the leaf, so a concrete leaf on a wildcard proof imported cleanly and 403'd on every push. The rule now lives once, in `gitlawb_core::ucan::push`, the crate all four already depend on: did_key_eq the node's exact did:key collapse rule — bare and full forms are one identity; a bare id never matches across methods (`did:gitlawb:X` is not `X`) is_push_action git/push, "*", repo/admin parse_repo_resource gitlawb://repos/<owner>/<repo>, exactly two segments is_push_wildcard a push-class action on a "*" resource Capability::grants_push_to(owner, repo) push-class, unconstrained, names exactly this repo Ucan::chain_grants_push_to(owner, repo) the leaf AND every proof behind it, depth-bounded, failing closed on undecodable or multi proofs and every boundary calls it: node ucan_grants_push = root anchored && bounded && chain_grants_push_to. `repo_capability_matches` is a thin wrapper; `proofs_name_repo` and `push_class_names_repo` are gone. `did_matches` delegates to did_key_eq so there is one collapse rule, not two. import the same walk, before any write. A leaf naming the repo on a "*" proof is refused with a message that names the proof. Everything is validated before anything is written. helper `build_invocation` refuses a stored token whose chain does not grant push to this repo, with a message that says why, before selecting a leaf — instead of minting an invocation the node is certain to refuse. issuance `gl ucan delegate` and the MCP `ucan_delegate` tool share is_push_wildcard. The MCP arm had no guard at all. Two tests that failed CI, both invisible on this Windows host because they are cfg(unix): - `import_creates_the_store_and_token_owner_only` still issued against a synthetic `z6MkAbc` owner, so import's owner-root binding refused it before the mode assertions. Now built from `owned_token`, like its siblings. - `a_failed_write_leaves_the_stored_delegation_intact` chmod'd the store to 0500 and expected the staging create to fail. It did not: `create_private_dir` repairs the store to 0700 before every write, so the second import succeeded and the test failed on `is_err()` — and a privileged runner would have made the arrangement pass anyway. Failure is now injected through a test-only seam (`fault::FailStagingWrites`) that fires after the bytes are written and before anything is published, on every platform, and the test also asserts no staging file is left behind. The module is no longer unix-only. Also: the node's rustdocs for `repo_capability_matches` and `ucan_grants_push` described the pre-round-8 behaviour (helper narrowing; leaf-only coverage) and now match the code. An orphaned docblock above `staging_path` — the old `write_private_file` doc, left behind in round 8 and claiming "not create_new" when the writer uses exactly that — is removed and the writer has a correct one. Verified by mutation, each restored byte-for-byte afterwards: core walks no proofs → core 3-link, node 2-link, node 3-link, import, and helper wildcard tests all red core walks ONE proof only → ONLY the two 3-link tests red; every 2-link test stays green — the recursion is load-bearing MCP guard removed → MCP deny test red, MCP named-resource test green staging seam disconnected → atomicity test red import chain check removed → import deny test red, delegate deny test green fmt, clippy -D warnings, check --locked --workspace --all-targets clean. gitlawb-core 122, gl 389, git-remote-gitlawb 65, node auth:: 23, and the DB-backed delegated-push tests pass alongside main's owner-push test. * fix(node): drop repo_capability_matches, orphaned by the shared push rule Round nine routed the node's authorization through Ucan::chain_grants_push_to, which parses the resource and compares owners with did_key_eq itself. The thin wrapper left behind had no callers, and CI's -D warnings build rejects dead code. Nothing else in the crate referenced it. * fix(core,gl): bound every proof-chain walk and make verify report the chain Round ten of #331. `verify_chain` and `chain_lifetime_is_bounded` recursed with no depth guard while `chain_grants_push_to` carried one. The node verifies the chain on the `X-Ucan` header of any signed request before the scope walk runs, so the bound protected nothing: a deep chain met the unbounded walk first. Both now stop at `MAX_CHAIN_DEPTH`, which moves to the module root since all three walks share it, and a chain at the bound still validates end to end. The MCP `ucan_delegate` tool issued an unbounded token when `expiry_hours` was omitted, which import and the node both refuse for push. It now defaults to the CLI's 720 hours and refuses a non-positive value. `gl ucan verify` and the MCP `ucan_verify` tool answered from the leaf alone, so a good signature on a broken proof chain reported valid and then failed at import and at the node. Both report through one `VerifyReport`, which walks the chain; `valid` requires it. The non-Unix `write_private_file` removed the live delegation before `rename`, on the belief that Windows refuses to replace an existing destination. std maps `rename` to `MoveFileExW` with `MOVEFILE_REPLACE_EXISTING`, so the remove only opened a window with no delegation at all. Dropped; a refresh over an existing token is now tested on every platform. Also: `cmd_import` and `cmd_verify` share one token-argument reader that reports an unreadable path as a file problem and caps the file size, instead of falling through to a decode error on the path string; and the MCP `ucan_delegate` description says import is CLI-only. * docs: state the two gates a delegated push crosses, and finish the sweep Round eleven of #331, documentation half. A `git push` crosses two independent gates: read visibility on the `info/refs` advertisement, and push authorization on the receive-pack POST. This PR implements the second and left the first as it was, and a UCAN grants no read access — so on a private repository a delegate must also be a reader. Nothing said so. RUN-A-NODE now states the contract once, in the owner-push section, and the delegation section carries a "Private repositories" note with the `gl visibility set` invocation and a "One repository per token" note matching what import enforces. The rest is the drift the earlier doc edits left behind: the README env-var table still said a `git/push` capability was "verified but not yet honored", the known-limitations bullet still said enforcement defaults to `false`, and the RUN-A-NODE section opened with a blanket 403 for every non-owner two paragraphs above the text that says a delegate can push. All three now describe the same node. * fix(gl): store one repository per imported delegation; pin the two-gate composition Round eleven of #331, code half. `gl ucan import` validated every capability up front and then wrote one file per repository, and `write_private_file` is atomic per file, not across files: a token naming two repositories could publish the first delegation, fail on the second, and report failure for both. `gl ucan delegate` and the MCP tool issue one capability per token, so such a token is hand-built; import now refuses it with the supported shape in the message. Several capabilities on one repository are still one file. `delegated_push_on_a_private_repo_needs_read_visibility_too` pins the composition the docs now describe, through the middleware production gives each route: on a private repository a valid delegation is refused at the `info/refs` advertisement (404) until the owner names the agent as a reader, clears the receive-pack POST on its own, and a reader with no delegation is still refused on the push. Making the visibility check admit everyone reddens the advertisement assertion. * fix(gl,core): sanitize token-derived output, prove the publish step, add MCP import Round eleven of #331, second half. Every string a token carries is chosen by whoever built it: `Did` deserializes any string (only `FromStr` validates), and a capability field is free text. `gl ucan verify`, `gl ucan show`, and the import refusal messages printed them raw, so an ANSI or OSC sequence in `with`, `can`, `iss` or `aud` — or in the error text `verify_chain` quotes them into — reached the operator's terminal. The strip is now one definition in core, `sanitize::strip_terminal_controls` (both halves of INV-6), and `gl`'s existing `sanitize_node_msg` is that plus its length cap. In `ucan_cmd`, `shown()` wraps every token-derived value on its way out. The refresh-replacement test could not tell the current non-Unix writer from the one that removed the live file before renaming, because a rename that succeeds ends in the same state either way. A second seam, `fault::FailPublish`, fails the rename after the staged bytes are complete; the new test requires the old token to survive that, and reinstating the remove reddens it on the platform that had it. The MCP server could issue and verify a delegation but not store it where `git-remote-gitlawb` looks. `cmd_import` is now a printer over `import_delegation`, and the `ucan_import` tool is the same function with a JSON answer: one set of checks, one store. * test(gl): count the MCP tool list at 41 with ucan_import `e3314c6` added the tool and left the count test at 40, so `test (windows)` went red first. The test also carried "42" in its name while asserting 40; it is now named for what it checks, and the names test covers `ucan_import` alongside the other two UCAN tools. * fix(node,gl): reader gate on receive-pack POST, GITLAWB_KEY in optional load, sanitize + bound UCAN surfaces (#331) * test(gl): pin GITLAWB_KEY honouring in load_optional_keypair (#331) * fix(gl): drop needless borrow caught by clippy * fix(gl): doctor reports refused resolvers, sanitize MCP UCAN JSON, atomic key write (#567) * fix(ucan): match push capabilities against the canonical repo name A capability written gitlawb://repos/<owner>/<repo>.git passed import and filed under the suffixed name, but the helper looks the store up by the canonical name and the node compared the resource verbatim against record.name, so the delegation missed at every hop and the push died at the owner gate with a bare 403. Introduce ucan::push::canonical_repo_name - the same at-most-one-.git strip smart_http_repo_name applies - and run it everywhere a push capability is issued (gl ucan delegate, MCP ucan_delegate), imported (repo_from_resource now returns the canonical segment, so the file lands where the helper looks), loaded (split_pack_post_url), and matched (grants_push_to, hence chain_grants_push_to and the node gate). At-most-once preserves the literal foo.git repo, reachable as foo.git.git. Document the canonical form in RUN-A-NODE.md. * fix(ucan): refuse key clobber on unreadable identity, fix doc gate sites and message escapes - quickstart: bail on an existing-but-unreadable identity instead of regenerating over it, and write the fresh key with create_new so a file that appears mid-setup is not clobbered (same rule gl init uses) - RUN-A-NODE: gate 1 read visibility applies on both GET info/refs and the git-receive-pack POST (same 404 for a client that skips the advertisement); visibility set example carries an explicit --mode - ucan delegate: replace literal \n escapes in the wildcard refusal messages (CLI and MCP) with line continuations so the error prints as one sentence; MCP expiry bound uses the shared MAX_EXPIRY_HOURS - MCP ucan_show: run the token version field through shown() - helper tests: restore GITLAWB_KEY via a drop guard so a panicking case cannot leak a tempdir path, and run the advertisement signature test under the shared lock with a deterministic empty store * docs(node): gate 1 allow-list semantics, fix comment-strip order in fn_body - RUN-A-NODE: a `/` rule is an allow-list that replaces the public flag, so "public repo = open to everyone / reader step unnecessary" only holds when no `/` rule exists; gate-1 bullet, the both-gates summary, and the private-repositories section now tell that one story - authz_guard fn_body strips // line comments before /* */ blocks so a line comment containing /* cannot open a range that swallows a gate * fix(node,gl): fail closed on protection-lookup errors, surface push readiness in verify - git-receive-pack: a non-owner whose is_branch_protected lookup errors is now refused instead of treated as unprotected; the owner path is unchanged (skip the check outright) - VerifyReport gains bounded_lifetime and push_ready so `gl ucan verify` and MCP verify show what import and the node would do with an unbounded push chain instead of a bare valid:true - RUN-A-NODE.md: the owner-push bullet names the delegation exception, and the delegation walkthrough now states the GITLAWB_KEY absolute-path prerequisite (import --dir must land in the same store the helper reads) - pins: delegate push refused when the protection lookup errors; an unbounded push-shaped delegation verifies but is not push_ready * fix(gl): judge push readiness with the shared push predicates in ucan verify - push_class now uses the same is_push_action rule import and the node apply, so repo/admin and * actions classify as push-shaped instead of omitting the Push-ready line - push_ready is a stored verdict, not just bounded+valid: every concrete gitlawb://repos/... push capability on the leaf must pass chain_grants_push_to, so a wildcard proof behind a concrete leaf verifies cryptographically yet reports not push-ready, matching what import and git-receive-pack would do - pins: repo/admin and * actions are push-ready; a wildcard-proof chain verifies but is not push-ready * fix(git-remote): gate delegation_header on import's push predicates and the file cap - a stored delegation that is expired or has an unbounded link is now ignored instead of attached: the node would refuse it at middleware with a distinguishable invalid_ucan response, and omitting the header keeps the denial on the unified owner-push path - the store file is size-checked against the same 1 MiB cap MAX_TOKEN_FILE_BYTES applies at import; an oversize entry is ignored like an unreadable one rather than read into memory on every push - pins: an expired on-disk delegation produces no X-Ucan (and no node probe); an oversize store entry produces none either --------- Co-authored-by: Vasanthdev2004 <[email protected]>	2026-11-11 02:49:14 +00:00
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]>	2026-11-09 23:02:05 +00:00
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]>	2026-11-09 22:48:47 +00:00
5 people	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 a0ea4c78e4. Refs #396 * docs(db): remove stray dedup comment escapes Restore readable root-path notation in the deduplicated listing documentation. Fixup for a0ea4c78e4. Refs #396 * fix(node): address #464 review findings - Hold migration 27/28 numbering: reservation comment now names the competing Gitlawb/node#384 range (27-35) and states the numbering is on hold pending #384. No renumber. - Restore crates/gl/src/identity.rs to LF endings so the diff shows only the added load_optional_keypair function. - Drop the dead single-column idx_agent_tasks_assignee_key build from v27 (v28 builds the keyset-ordered variants directly); adjust the v27 test to assert the raw index is dropped and the expression index is never built. - Align task_claimable with claim_task's conditional write: drop the delegator short-circuit and correct the docstring. - Pin error_for_status on the gl task list path with a 403 mock that serves a well-formed empty page, so the test goes red if the call is removed. * test(node): pin the delegator deny on a pre-assigned claim - Add claim_task_delegator_cannot_claim_preassigned_task: pins opaque 404 for delegator claiming a pre-assigned task (regression guard for P2) - Fix test_list_tasks_server_error_surfaces regex: /api/v1/tasks\? -> /api/v1/tasks\? (escaped ? for literal question mark) - Assert actual reqwest StatusCode::FORBIDDEN instead of matching "403" in error string (avoids spurious pass from random mock port) * test(node): exercise streamed byte-budget guard with chunked body Use with_chunked_body instead of with_body so mockito sends no Content-Length, pinning the no-length streamed accumulation guard. Assert the "exceeded" message to distinguish from the declared-length check. * fix(node): pin declared-length byte-budget arm in oversized content-length test Addresses beardthelion review finding (2026-09-26): test_read_task_page_json_oversized_content_length now asserts on the "(declared" arm the way the chunked twin asserts "(exceeded", so the Content-Length pre-check at crates/gl/src/task.rs:211 is genuinely pinned. Verified the pin by neutering the pre-check (test goes red), then restored. fmt clean. * fix(node): fail closed on claim for unresolvable repo task_claimable returned true for an unassigned task whose repo_id is slash-form (mirror row) or names a repo this node does not host, while task_visible fails closed on the same rows. A signed stranger could claim such a task even though reads 404. Return false for a Some(repo_id) the node cannot resolve to a locally-hosted repo, matching task_visible's fail-closed check; keep the None arm open (repo-less tasks claimable by design). Adds a regression test asserting a stranger's claim 404s and leaves the assignee unchanged for both row classes. * fix(node): pad sealed task cursor to fixed width The scan-ceiling token seals the last examined row's (created_at, id) as variable-length JSON under a length-preserving keystream, so token length leaked the withheld row's field widths: a 32-char to_rfc3339() rendering mints a 148-char token where a 35-char one mints 161 chars. Pad the serialized payload with trailing JSON whitespace to a fixed 128-byte bucket before the SIV tag is computed; decode tolerates the padding (serde_json ignores trailing whitespace) and tokens minted before this change still verify. REST and GraphQL share task_cursor::encode, so one fix covers both. Adds token_length_does_not_vary_with_position_width: asserts identical token length across narrow/wide timestamp positions and that the padded token still decodes to the exact stored position. * fix(node): address beardthelion round-six P3 findings on task cursor - Pin the encode docstring compat claim: add unpadded_legacy_token_still_verifies test minting a token via the pre-padding recipe (siv + apply_keystream with no padding) and asserting decode returns the position. - Pin the PAYLOAD_BUCKET round-up claim: add oversized_payload_rounds_up_to_next_bucket test with a 140-char id crossing the 128-byte bucket, asserting round-trip at two buckets. - Correct timestamp-width figures: to_rfc3339() renders 25 chars with no fractional seconds (not 32); 32 is the microsecond width. * fix(node): address beardthelion round-seven P3 findings on task cursor - Correct wire-form docstring to v1.<iv>.<body> - Fix payload width figures: 101 bytes total, to_rfc3339() precision ladder - Tighten oversized-payload test to assert exactly 2*PAYLOAD_BUCKET - Pin legacy back-compat test with a frozen v1 token fixture * test(node): address beardthelion round-eight P3 findings on task cursor - Pin the one-bucket width: round_trips_position_verbatim now asserts the decoded body of a normal token is exactly PAYLOAD_BUCKET, catching a pad that skips next-multiple or a drifted bucket constant. Every other width assertion here is bucket-relative, so only an absolute check goes red. - Assert the frozen fixture is the unpadded shape: unpadded_legacy_token_still_verifies decodes the body segment and asserts raw.len() < PAYLOAD_BUCKET, so a literal re-minted under the padded recipe would retire the back-compat pin loudly instead of passing silently. - Narrow the recipe-pin claim: the docstring now names what the frozen literal actually binds (domains, MAC input, keystream, segment layout, field names) instead of claiming padding and serialization changes break the test. The padding-width claim stays with oversized_payload_rounds_up_to_next_bucket. * fix(node): trim stored assignee on every task gate; align SECURITY.md task-read wording * test(node): pin mirror-row list-vs-get asymmetry; fix plan-test bind (#553) - EXPLAIN helper binds normalize_owner_key(assignee) to match the production list_tasks_keyset bind. - Pin the deliberate asymmetry jatmn asked about: a delegator sees a slash-form (mirror-hosted) task in a list, while get fails closed for every caller because the row has no local repo backing. Notes in get_visible_task and a regression test keep it from drifting. - Repair test-mod drift from the main merge: build_schema gains its TaskCursorKey arg at three sites and list_tasks calls move to list_tasks_keyset. * test(node): reconcile tasks complexity contract with TaskPageType (#553) The four GraphQL task/brake tests failed on this branch: contract tests still queried the flat tasks { id } shape that TaskPageType replaced, and the alias-brake tests ran default-size lists whose six aliases exceed GRAPHQL_MAX_COMPLEXITY before the brake can debit. - Contract tests now select through items { ... } with refreshed cost arithmetic. - Brake tests use tasks(limit: 1), keeping six aliases under the budget so the 6th field reaches TaskReadBrake and fails RATE_LIMIT_MESSAGE. Complexity stays the outer gate (#552); the brake stays the per-request field-count gate. * docs: note mirror-hosted task list-vs-get asymmetry (#553) Option 3 for slash-form mirror repo_id rows was pinned in code and test at 6c0926de; this adds the operator-facing half jatmn asked for. README and SECURITY task-read bullets now state that mirror rows list for party readers while get-by-id and GraphQL task fail closed for all callers. * ci: retrigger codeql scan after alert 86 dismissal --------- Co-authored-by: euxarist <[email protected]> Co-authored-by: cairn-code <[email protected]> Co-authored-by: cairn-intern <[email protected]> Co-authored-by: Marlowe <[email protected]>	2026-11-09 19:02:54 +00:00
3 people	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]>	2026-11-09 15:35:18 +00:00
3 people	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 a6113fd8 merge onto #370 left the workspace red: a missing closing `}` in `web_url_rejects_non_absolute_or_non_browser_urls` so the next test nested inside it (config.rs), a duplicate `url = "2"` workspace dep (Cargo.toml, one from the redirect-rule PR, one from #370), and `fetch_node_web_url` derefing `CappedBody` as `&str` instead of `.text` (repo.rs, regression in the same merge). All three are straightforward to repair; the brace was the only one blocking CI. The new `cmd_create_view_url_tracks_node_advertisement` integration test drives the real `gl repo create` binary against a mockito node that answers both `POST /api/v1/repos` and `GET /`, and asserts the printed `View:` line tracks the advertised web_url — present with the advertised origin when set, absent when the node omits it. The helper-level `fetch_node_web_url_*` tests would still pass if `cmd_create` reverted to a hardcoded URL; this one would not, so it is the load-bearing check for #370. * fix(gl,node): normalize stored web_url and reject userinfo (#370 review) Two P2s from the #370 review addressed at the configuration boundary. `Config::validate()` now rewrites `self.web_url` to the canonical `url::Url::to_string()` after `validate_web_url` succeeds, so the regular and degraded `GET /` handlers advertise the same string that was actually validated instead of the operator-typed one. The CLI trims a second time in `fetch_node_web_url`, which hid the divergence on the View: line, but any other consumer parsing `web_url` directly would see a non-canonical base. `validate` is now `&mut self` to carry the write; the one non-test caller already binds `mut` and the two test bindings are updated to match. Both `validate_web_url` (node) and `fetch_node_web_url` (CLI) now reject URLs with a non-empty username or a password: the configured value is published on public `GET /` and reprinted on the View: line, so an accidentally configured `https://user:[email protected]` would land in public metadata, terminal scrollback, and CI logs. New regression tests cover user-only and user+password forms on both sides, plus a normalization test that the stored value matches the canonical form (with and without operator padding). * fix(gl,node): classify capped node-info bodies and reject encoded bidi in web_url An oversized or half-read GET / body previously fell through JSON parse failure into the same silent-None arm as a node with no web front-end. The classify step now consults the capped body's truncated and read_failed flags and warns before omitting the View link. web_url validation on both sides also scanned only literal bytes for control and bidi characters, so a percent-encoded reordering character (%E2%80%AE) passed boot validation and CLI ingest even though a browser decodes it into a bidi spoof. Both validators now check the decoded host and path (INV-6). Also fixes the control-byte test, which served invalid JSON and passed without reaching the check it pinned. --------- Co-authored-by: Gravirei <[email protected]> Co-authored-by: Gravirei <[email protected]>	2026-11-08 18:54:05 +00:00
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	2026-11-05 16:46:47 +00:00
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 e04efe4 regenerated the app's compose file on every access, overwriting edits operators make to pass settings the generated environment block omits (e.g. GITLAWB_OPERATOR_STRICT_MODE). Generate the file only when it is missing; when it exists, keep it and rewrite only references to the retired ghcr.io/gitlawb/node image, keeping tags and digests. Co-Authored-By: Claude Opus 5.5 <[email protected]> * fix(macos): only migrate complete ghcr.io/gitlawb/node references Without a left boundary the migration also rewrote the retired path when it appeared inside a longer reference, such as a registry mirror (registry.example.com/ghcr.io/gitlawb/node), pointing it at a path the mirror may not have. Co-Authored-By: Claude Opus 5.5 <[email protected]> --------- Co-authored-by: Claude Opus 5.5 <[email protected]>	2026-11-04 21:58:29 +00:00
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]>	2026-11-04 21:38:41 +00:00
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]>	2026-11-04 17:02:15 +00:00
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]>	2026-11-04 16:34:10 +00:00
Kevin Codex	73fd7473af	Merge pull request #330 from Gitlawb/fix/default-enforce-owner-push ... fix(node)!: enforce owner-only push by default	2026-08-25 22:06:12 +08:00
Vasanthdev2004	2f0174cc04	fix(node): close review findings on the owner-push default ... Every item below was verified against the code before changing it; two of them were confirmed by running the probe the reviewer described. A test the fixture sweep missed, and that could not fail -------------------------------------------------------- `f1_write_cap_is_inert_without_a_resolvable_source_key` seeded `z6f1none` and pushed as `z6MkF1NoKeyPusher…`, so the owner gate rejected it before it reached the per-caller cap the test exists to measure. Its assertion was `!matches!(r, Err(Overloaded(_)))`, which a `Forbidden` satisfies, so it reported ok while measuring nothing — and it is not `#[cfg(unix)]`, so it did that inside every green run. Confirmed by inverting the assertion to expect `Forbidden`: it passed, proving the request stopped at the gate. The pusher is now the owner, and the assertion is positive: reaching git on the missing on-disk repo (`Err(Git(_))`) is proof the request cleared both the gate and the cap. The sibling `f1_write_cap_key_is_per_source_not_per_repo` carried the same weak negative in its second assertion and is tightened the same way. The default was barely tested, and what tested it read the environment --------------------------------------------------------------------- `Config::parse_from` consults the process environment, so on a host exporting `GITLAWB_ENFORCE_OWNER_PUSH=false` the old `enforce_owner_push_defaults_to_true` reported the operator's setting rather than what this crate declares — it failed outright, which would also break any contributor who sets the variable. It is replaced by two tests that are env-independent and behavioural: * `enforce_owner_push_is_declared_true_independent_of_the_environment` reads the declared default off `Config::command()`. * `default_config_refuses_a_non_owner_push` drives the handler on an untouched `test_state` config and asserts 403 — authenticated by a real RFC 9421 signature, so 403 rather than 401 is what makes it a statement about authorization. Both were checked by reverting `default_value_t` to `false`: the handler test reports 500 where it wants 403, and the declared test reports `["false"]`. The cross-tenant test lost its second tenant -------------------------------------------- The sweep collapsed `u1_parked_push_does_not_shed_another_pusher_behind_the_same _ip` onto one identity, leaving the test named for cross-tenant behaviour with a single tenant and two docstrings describing a scenario it no longer ran. `n2` is now owned by `z6u1nat2` and its push signs as that owner, so two distinct tenants share one edge IP again — which is the property under test, since the per-source key is the resolved peer address and never the DID. Four comments outlived their premise ------------------------------------- `repos.rs` (3 sites) and `.env.example` still said owner enforcement defaults off. The IP-keying rationale they support is unchanged and still correct — a DID farm defeats a DID key — so only the dead premise is rewritten: the cap is load-bearing because the rate limiter bounds arrival rate rather than occupancy, because the gate can be turned off for a rolling upgrade, and because one owner can open many concurrent pushes to their own repo. Operators on the container path got none of this ------------------------------------------------- `GITLAWB_ENFORCE_OWNER_PUSH` is now forwarded in `docker-compose.yml`, the macOS app's bundled compose, and `infra/aws/compose.yaml.tftpl`, defaulting to `true` to match the node. Without it the rolling-upgrade opt-out this PR documents in three places was unreachable for anyone running compose. A cutover this project never promised -------------------------------------- The README paragraph announcing a 15 September 2026 flip of `GITLAWB_REQUIRE_SIGNED_PEER_WRITES` is removed. That flag is not changed here, the date appears nowhere on main, and a PR about a different flag is not where that commitment would be made. The table row for `GITLAWB_ENFORCE_OWNER_PUSH` carries the part that belongs. A docstring that credited the wrong thing ------------------------------------------ The field and test docs claimed the value-taking action is what makes `GITLAWB_ENFORCE_OWNER_PUSH=false` resolve to `false`. It is not: the env form resolved under the presence-only declaration too. What the change buys is `--enforce-owner-push false` parsing at all, rather than erroring as an unexpected argument. Both docstrings now say so.	2026-08-14 20:43:43 +05:30
Vasanthdev2004	cdbda19c29	fix(node)!: enforce owner-only push by default ... A `did:key` signature is authentication, not authorization: the method is self-certifying, so any party can generate a keypair, derive its DID and sign. With `GITLAWB_ENFORCE_OWNER_PUSH` defaulting to `false`, `owner_push_rejection` short-circuited and every `git-receive-pack` carrying any valid signature was accepted — including pushes to a repository the signer does not own, and including private ones, on every branch not explicitly protected. The gate itself already worked when switched on. What was missing is that no reachable default configuration switched it on, so a node started with nothing configured accepted a push from anyone. That default is what this changes. The argument now takes a value so there is a way back: `--enforce-owner-push false` and `GITLAWB_ENFORCE_OWNER_PUSH=false` both disable it, while the bare `--enforce-owner-push` form still means `true`. Without this the flag would be presence-only (`ArgAction::SetTrue`), which with a `true` default would leave operators no way to opt out during a rolling upgrade. Three concurrency tests pushed as non-owners in order to reach the shedding logic they actually assert on. They now push as the repo owner, so they keep exercising the shipped default rather than opting out of the gate. Docs updated in README.md, .env.example and docs/RUN-A-NODE.md, including the delegated-key caveat below. README also carries a dated cutover for `GITLAWB_REQUIRE_SIGNED_PEER_WRITES`, which stays `false` for now so live peers can finish upgrading. BREAKING CHANGE: `git-receive-pack` now rejects a push whose authenticated DID is not the repo owner, returning 403 before any ref update is applied. Delegated and CI keys count as non-owners: a UCAN `git/push` capability is verified but not yet honored for authorization, so an agent pushing under its own DID cannot push while this is on. Set `GITLAWB_ENFORCE_OWNER_PUSH=false` during a rolling upgrade, or have automation push as the repo owner, until scoped collaborator / UCAN-delegated push rights land.	2026-08-14 17:22:23 +05:30
beardthelion	0d7711c089	fix(node): bound the pre-walk CID resolve on its own short budget ... get_by_cid takes the global walk permit and the per-source permit before resolving the CID, and that lookup was clamped only by the 600s request budget. A well-formed CID with no pinned row does no probe and no walk, but against a stalled pool it held a scarce walk slot for the full window, so requests from enough distinct sources could reject every real retrieval at admission while nothing was walking. The lookup now runs under its own budget, defaulting to ten seconds and clamped by the request budget, so a larger value degrades rather than extends. Admission still comes first. Resolving before admission was the other way to fix this and it trades one amplification for another: taking the read out from behind the gate lets unadmitted anonymous callers stack concurrent queries, and shedding a flood cheaply before it reaches the database is what admission-first is for. Only that one await moves. Everything after it stays on the request budget because it runs interleaved with work the caller has already been admitted for: the per-oid source lookup runs after real probing from the second candidate on, the marker pair only on a provenance miss, and the pager fetches between pages that cost probes. A short clock anchored at admission would be long spent by then, so widening it would shed requests that are making progress. The region comment now carries that assignment per await. The shed names the knob an operator can actually turn, separately from the request-budget shed, so the two are distinguishable in the body and the log. Verified by mutation: reverting the clamp overruns the short budget, and anchoring one short clock across the whole permit-held region sheds a slow but progressing walk. The second leaves the first test green, which is what makes it a boundary case rather than a second removal.	2026-08-12 16:59:02 -05:00
beardthelion	22f4db94b2	fix(node): bound the legacy CID scan and let the caller carry its position ... A denial-only inventory paged through the whole repo table. gate_and_serve returns Skip on quarantine and on the root visibility deny before either walk.probes or walk.visits increments, and those two counters were the pager's only stop conditions, so an anonymous caller holding a CID from the public pins index could read every row and its rules while the global IPFS walk admission stayed held for the request budget. Three parts, and the shape of each is set by a way an earlier attempt was wrong. A row ceiling, with a rules companion. Rows scanned bounds the DB reads and the pager's row retention; it does not bound rules, since the rules query has no LIMIT and an owner picks how many rules their repos carry, so that gets its own ceiling. Both taint into the existing retryable 503 and never a 404. A ceiling alone would stop there and strand content. The scan starts from a NULL cursor over (created_at, id) ASC, so a bare ceiling cuts the same oldest prefix on every request and a holder past it is unservable for good, which is the defect this PR already carries a finding about. So the 503 hands back the position. Server-side alternatives were tried and dropped: a node-global cursor lets concurrent callers skip each other's windows, and a per-caller map lets a key farm evict an honest caller mid-ladder. State the caller carries has neither problem. The token is sealed, not encoded. It names a row the caller may not be allowed to see, and id derives from the owner DID, so plaintext would leak a withheld repo's existence, creation time and owner. XChaCha20-Poly1305 with a fresh nonce per seal, the canonical CID as associated data, an expiry inside the sealed bytes, and a fixed-width plaintext so token length cannot vary with the row it names. Undecryptable, expired, malformed and foreign-CID tokens are one indistinguishable case: absent, start at the front. The wrap taint keys on the pager reaching exhaustion while resumed, not on any break site, so a token pointing at the last row still sheds a retryable 503 instead of falling through to the 404 tail. Pages are charged to the per-IP work bucket. Without that the ceiling bounds one request and nothing across requests, and the denial path pays nothing, which is the per-request-free-allowance shape already rejected once on this branch. The derived work-budget floor grows by the same page term so a deep scan cannot self-throttle mid-ladder. A toll shed renders 429 rather than 503, which reverses the tail's documented precedence for this one case. The tail comment names the exception: the caller's own bucket stopped the scan, not the node's search, and a 503 would invite an immediate retry into an empty bucket. Four existing tests hand-size the work limiter to their exact probe count and now also pay for a page; each gained one token and a comment showing the arithmetic. Production is unaffected because the floor grew by the same term. Verified by mutation: removing the ceiling unbounds the scan; truncating without tainting turns it into a 404; emitting a token but never resuming never reaches the holder; dropping the toll makes pages free; encoding the token as base64 plaintext leaks the withheld fields; dropping the CID from the associated data honors a cross-CID resume; a variable-width encoding makes token length vary with the row; and a constant nonce makes two seals identical.	2026-08-12 15:49:51 -05:00
beardthelion	8f3a52175a	docs(node): correct the walk-cap and db-pool operator docs ... Two doc drifts jatmn flagged on #173, both docs-only. The /ipfs walk cap is min(internal ceiling, GITLAWB_IPFS_MAX_REPOS_WALKED), where the internal ceiling is MAX_PIN_SOURCES + 1 = 17. config.rs, README, and .env.example all described a max() floor instead, so an operator reading them would think the default of 64 was the effective ceiling and that raising the knob bought headroom. Neither is true. The runtime is not changed: two tests set the knob to 1 and depend on the cap actually being 1, so wiring the documented floor would make the knob inert and break them. db_max_connections has defaulted to 48 for a while and four comments still said 20. Two of them did arithmetic on it. main.rs and repo_store.rs justified the separate advisory-lock pool by "20 is below 32", which is now false and made the separation look like a consequence of the defaults; it is structural, since a push holds its connection for the whole receive-pack at any pool size. Also adds a RUN-A-NODE troubleshooting entry for the boot failure an in-place upgrade hits when it is still configured with the old pool size of 20.	2026-08-10 01:24:46 -05:00
beardthelion	9f74811269	Merge origin/main into fix/issue-135-ipfs-cid-tree-gate ... #174 landed on main at 5981257, so the integration merge this branch carried is now redundant with main's own history. Five files conflicted, and two more auto-merged into states that did not compile. store.rs: main migrated the bounded-read family from a per-call timeout: Duration to a shared deadline: Instant. The auto-merge took main's batch_check_probe alongside this branch's object_type_bounded, which compose, and produced two read_object_bounded definitions with incompatible signatures and no conflict marker. Took main's convention throughout and re-applied this branch's additions on top: object_size_bounded, the wedged-child reaper test, and the doc enrichments. ipfs_pin.rs: the pin read is bounded by the earlier of the batch deadline (#174) and the object's own git_timeout (#173). Neither implies the other and both are load-bearing: with only the batch deadline, one wedged cat-file holds the pin permit for the whole 120s budget; with only git_timeout, a batch of slow reads runs past the budget. Each direction was verified by injecting it and observing the RED. api/repos.rs: kept main's choice of the literal "git" over state.git_bin for the pin path's cat-file, since that knob is the walk binary and tests point it at a fake. The parameter stays so the seam is still drivable from a test, and the reason now sits at the call site. main.rs: dropped main's free sweep_rate_limiters, superseded by AppState::sweep_rate_limiters, which sweeps the same limiters plus ipfs_work_rate_limiter. Both call sites already used the method. db/mod.rs: dispositioned main's two new peers-table test writers in the completeness ledger. Migration numbering is unchanged. v17 is byte-identical on both sides, and main's own reservation comment already allots 18-23 to this work.	2026-08-10 01:08:46 -05:00
beardthelion	dac29e550c	docs(env): stop claiming the pin pool bounds retained memory ... GITLAWB_MAX_CONCURRENT_PIN_TASKS caps how many pin loops run at once, not how much object-id list memory is retained. On the local IPFS path the caller materializes the list before pin_new_objects_gated acquires, so a parked loop still holds its full list and the parked count is capped only per repo by EncryptInflight. config.rs and the pin_new_objects_gated docstring already say this; the env template still said the knob bounds the memory.	2026-08-06 21:54:31 -05:00
beardthelion	e5c1da2bc6	Merge #174 into the CID tree-gate branch ... 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.	2026-08-02 16:09:57 -05:00
beardthelion	bc826df2ec	fix(review): correct the budget-knob claims and mirror the range in operator docs ... Review round on the three preceding commits. The knob's doc said a value past the ceiling "aborts every /ipfs/{cid} request in a release build". That is wrong, and three reviewers plus an outside model landed on it independently: the ceiling is 100 years, the Instant addition does not actually overflow until roughly 584 years on a u64-nanosecond Instant, so values in between are representable and are rejected as a deliberate safety margin, not because they panic. Reworded to say that, since a confidently wrong comment is worse than the vague one it replaced. Also on the knob: - GIT_SERVICE_TIMEOUT_SECS_MAX has two consumers now, so its own doc says so, and notes this knob derives only the Instant addition and not the lease-steal multiply. - The "clears every disable setting" phrasing overclaimed: u32::MAX sits above the ceiling. Now names the documented sentinels instead. Operator-facing docs: the sibling knob's ceiling commit updated both README.md and .env.example in the same change, and this one had not. Both now carry the accepted range, matching that precedent. House style: five comment lines added by the preceding commits used em dashes, which this project does not allow. Four were carried over from the text being edited and one was copied from the sibling test. All replaced. Checked mechanically over the added lines rather than by eye. Two comments restated an argument in full immediately after pointing at the canonical site for it. Trimmed to the pointer plus the local delta. Scoping correction worth recording: the two preceding commits claimed "no behavior change". That holds for repos.rs, state.rs, main.rs and test_support.rs, but a config.rs field doc IS clap --help output, so the pin-knob doc edit changes operator-visible help text. No runtime behavior changes either way. Verification: full suite 733 passed, fmt clean, clippy clean under the CI form. The range guard was separately proven load-bearing in both directions by mutation: reverting to range(1..) goes RED on the rejection assertions, and tightening the ceiling to 500_000_000 goes RED on the disable-value assertions. Known residual, not fixed here: the bound is parse-side only, so a Config built by direct construction rather than through clap can still hold an unrepresentable value.	2026-07-31 01:24:46 -05:00
beardthelion	33063baf5a	fix(node): set the git_service_timeout ceiling by representability, not tidiness (#174) ... A second-model pass on the previous two commits caught an upgrade footgun in the first and stale prose in the second. The 365-day ceiling was tighter than the defect required. The help text has always told operators to set this very large to disable the bound, and clap accepted every positive u64, so values like 999999999 (~31 years) are working production "off" settings that neither overflow `* 2 + 60` nor exceed an Instant deadline. A node running one of those would have failed to start after this upgrade, over a value that was never the defect. The ceiling moves to 100 years, which clears every such setting and stays an order of magnitude under the ~584-year limit of a u64-nanosecond Instant, the tightest representation on any platform we build for. Every value that worked before still parses; only the ones that would have panicked are rejected. The test now pins the pre-existing disable values alongside the rejections so the distinction cannot quietly regress. Clamp-and-warn was the suggested remedy and is not what landed: it would keep accepting a value the node cannot honor and silently substitute a different one, where the range says what is representable and fails fast, matching how every other bound in this file behaves. The lease-bound comment and its test still described the pre-ceiling world. Both now say what is true, and the test deliberately keeps u64::MAX rather than moving to the ceiling as suggested: Config is reachable by direct construction, and at the ceiling the arithmetic does not overflow, so a test pinned there would pass with the saturation removed and prove nothing. Re-confirmed load-bearing after the rewording.	2026-07-30 18:07:16 -05:00
beardthelion	b231d32703	fix(node): bound git_service_timeout_secs to a range its deadlines can represent (#174) ... Tracing the lease-bound overflow to its other consumers turned up a worse instance of the same defect. This PR routed git_service_timeout_secs into build_filtered_pack and blob_paths, and both build a deadline as `Instant::now() + Duration::from_secs(..)`. That addition panics on overflow in RELEASE as well as debug, so a large-but-accepted value crashes the serve path rather than merely disabling a timeout. Hardening each arithmetic site as it is found would leave the next one open, so the bound goes where the value enters: clap now rejects anything above 365 days at parse time, which keeps every derived duration in range at once and fails fast at boot instead of at the first path-scoped clone. A year is many orders of magnitude above any real clone or push, so it remains the practical way to disable the bound. The test pins both derivations at the maximum and rejects the value one past it and u64::MAX. RED when the upper bound is dropped from the value_parser.	2026-07-30 17:28:36 -05:00
beardthelion	9bfe7d5d30	docs(node): correct stale admission-bound comments and document derived caps (#62) ... Four prose corrections on this PR's surfaces, no behavior change. The receive-pack advertisement no longer draws from the write pool, it draws from a dedicated advert pool, but five comments still described the old shape: the info/refs per-source block, the cap construction in main.rs, the State field doc, and a test doc plus its assertion message. The state.rs and main.rs ones mattered most because they contradicted the operator text being written in this same commit. The /ipfs probe and content-read subprocesses each run under their own deadline now (the lesser of git_service_timeout_secs and the remaining budget, reaped by process-group teardown), but four surfaces still claimed they had no duration bound past the pre-start budget check. The correction stays scoped rather than declaring the route safe: the probe's object_store_readable check is a synchronous filesystem sweep with nothing to reap, so a wedged filesystem can still hold the walk slot past the deadline, and that residual is now stated where the old warning used to be. A grep for the full phrase misses two of these surfaces, since one wraps across doc-comment lines and one says "duration clamp" instead, so the sweep uses the fragment. Both push-side per-source concurrency caps are derived as max_concurrent_git_pushes / 8 with a floor of 1 and have no environment variable, so neither was documented anywhere an operator reads. The write-side one is load-bearing for push availability: it is acquired before the global write permit, and owner enforcement defaults off, so without it one host minting disposable did:key identities could monopolize the write pool. They are described in .env.example beside the read cap rather than in README's table, which is keyed by variable name. The hung-walk assertion in visibility_pack printed {err}, showing only the top anyhow context and dropping the underlying io error. A beta-lane failure surfaced as "failed to spawn git for-each-ref" with no errno, which made it undiagnosable. {err:#} prints the chain. The underlying flake is still undiagnosed: it did not reproduce in 123 local runs and the stable lane passed on the same commit, so this only improves the next occurrence.	2026-07-29 21:44:53 -05:00
beardthelion	1219c21469	fix(node): share one deadline across the withheld walk and the pack serve (#62) ... The path-scoped git-upload-pack branch spent git_service_timeout_secs twice: once on the withheld-blob classification walk, then a second full budget on the serve. One clone could hold a read-pool permit for ~2x the configured budget, which is the occupancy hole this PR otherwise exists to close. Thread one Instant deadline through both phases, mirroring fail_closed_full_scan_objects and build_filtered_pack: the walk runs against the remainder and the serve gets what the walk left, computed after the blocking task's await so it cannot pick up a fresh budget. A walk that consumes the whole budget saturates the serve to zero, which surfaces as GitServiceTimeout -> 504 rather than running unbounded. Under-serving one slow clone is the safe direction versus over-holding the pool. Both serve arms are covered because a fix that reached only the plain arm would leave the ~2x hold reachable by any clone of a repo that actually withholds a blob. Each test was observed RED before the change (status 200, the serve completing on a fresh budget) and GREEN after, and reverting the filtered arm alone flips only its own test, so neither guard is a proxy for the other. The knob's meaning changed for this path, so .env.example now says the value bounds the two phases combined rather than each separately.	2026-07-29 21:44:28 -05:00
beardthelion	799759f1fc	fix(node): sweep the /ipfs rate limiter and document the lease waiter cap ... The 300s cleanup task swept the DID, create-IP, push, sync-trigger and peer-write limiters but not ipfs_rate_limiter, so its expired keys sat until the map hit max_keys and the inline capacity sweep reclaimed them. Drive the sweep off AppState in a named sweep_rate_limiters so a limiter added to the state and forgotten here fails a test instead of leaking keys, and cover all six with a regression that goes red when any one is dropped from the sweep. GITLAWB_REPO_LEASE_MAX_WAITERS was also missing from .env.example and the README settings table, leaving operators without a way to discover or tune the cap behind the "repo is busy with another push" 503.	2026-07-28 16:01:30 -05:00
beardthelion	86fc2763a3	fix(node): wire GITLAWB_IPFS_MAX_REPOS_WALKED to the legacy-probe budget ... 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.	2026-07-20 19:17:12 -05:00
beardthelion	cc9615961f	fix(node): bound concurrent pin-loop memory; document the per-repo task cap (#174 F6) ... EncryptInflight bounds the outstanding post-push encryption-task set to one per repo, but its state.rs doc implied a global cap it does not deliver, and each pin loop (ipfs_pin + pinata pin_new_objects) holds a full per-push object-id list while walking it — so N distinct authenticated first-pushes could hold N MB-scale lists at once, unbounded. Add a global pin_semaphore (GITLAWB_MAX_CONCURRENT_PIN_TASKS, default 8) and gate BOTH pin loops through it: they DEFER when the pool is full, never drop a pin. Tighten the state.rs comment to state the bound is PER-REPO, with the cross-repo residual (throttled by auth + rate limits, memory bounded by the pin permit) called out honestly. Also correct .env.example GITLAWB_DB_MAX_CONNECTIONS (20 -> 48): with the #174 F1 boot validation, 20 is below max_concurrent_git_pushes(32)+headroom(8) and would fail startup — closing the INV-24 doc gap from the F1 change. Tests: pin_new_objects_gated_defers_when_pin_pool_exhausted (RED without the permit acquire); max_concurrent_pin_tasks range/default.	2026-07-18 11:13:32 -05:00
beardthelion	2eaf8b1086	fix(review): state the probe/read stages' unbounded-hang residual honestly; net the acquire-taint found path ... The budget-knob docs claimed the overshoot was bounded by one unclamped probe; an unclamped subprocess is not a bound, so the README, .env.example, config help, and module comments now say plainly that a hung git probe holds the walk slot for the hang's duration. New regression: an acquire-tainting ghost row ahead of a healthy public copy must not stop the scan; the later found short-circuit still serves 200.	2026-07-17 20:42:48 -05:00
beardthelion	00618603ad	fix(node): bound an admitted /ipfs request's total lifetime with a shared budget (#174) ... GITLAWB_IPFS_REQUEST_BUDGET_SECS (default 600) is captured as one deadline at handler entry: every acquire and expensive walk is clamped to the remaining budget, every stage checks remaining before starting, and exhaustion taints the scan into the retryable 503. A walk in flight is never aborted at the tokio layer (that would free the walk permit while the blocking thread still runs); its clamped git deadline plus group teardown ends it. Docs state the residual overshoot: the in-flight stage's kill/reap slack plus the unclamped probe subprocesses.	2026-07-17 18:03:49 -05:00
beardthelion	d3ead53aee	fix(node): make /ipfs scan verdicts honest: walk-scoped cap, visit ceiling, 503 on truncation (#174) ... A request-scoped taint list replaces the silent folds: any repo skipped without a verdict (acquire timeout, probe or read failure, walk failure, cap or ceiling exhaustion) turns the terminal into a retryable 503 with Retry-After, and 404 is reserved for a scan in which every candidate reached a verdict. The walk cap now counts only expensive allowed-set walks and skip-continues on exhaustion so a plain public copy past the cap still serves; a new GITLAWB_IPFS_MAX_REPO_VISITS knob (default 1024) bounds the acquire+probe cost class and stops the scan, documented with its worst-case object-store fetch count.	2026-07-17 17:39:03 -05:00
beardthelion	cdc62a703a	fix(node): reject unsupported git service before the read slot; document the new admission knobs (#174) ... P2-1: git_info_refs treated every service other than git-receive-pack as a read op and did the DB/visibility/Tigris work under a read permit, validating the service string only downstream in smart_http. So an unauthenticated `?service=anything` to a public repo consumed a read slot + storage work before rejection. Validate the service is exactly git-upload-pack or git-receive-pack immediately after parsing, returning 400 before the pre-DB shed and the permit. Handler-layer regression: with the read pool exhausted, `?service=git-explode` returns 400 (validated first), not 503 — removing the validation makes it 503 (RED). U5/INV-24 docs: document the new knobs from U2/U3 in .env.example and the README env-var table — GITLAWB_GIT_ACQUIRE_TIMEOUT_SECS, GITLAWB_MAX_CONCURRENT_IPFS_WALKS, GITLAWB_IPFS_WALK_PER_SOURCE, GITLAWB_IPFS_MAX_REPOS_WALKED, GITLAWB_IPFS_RATE_LIMIT. Each is already enforced on the path it names (U2/U3 tests), so the docs match the implemented routing.	2026-07-15 13:16:40 -05:00
beardthelion	1e1058ca29	docs(node): correct concurrency + timeout docs to match the final git routing (#174) ... jatmn P2 (INV-24): the config help, .env.example, and README described routing the handler no longer does. - The read pool serves upload-pack and the UPLOAD-PACK info/refs advertisement only; the anon receive-pack info/refs advertisement draws from its own dedicated pool (sized like the write pool but disjoint), not the read pool and not the write pool. - The write pool is the authenticated push POST only. - The per-caller read cap is keyed on the resolved source IP, never the DID: a signature does not move a caller off it (the docs claimed a per-DID budget, which would tell a NATed authenticated client it escapes the shared cap when it does not). - git_service_timeout_secs now also bounds the push-side pin-candidate discovery (rev-list / cat-file), reaped via process-group teardown, alongside the info/refs advertisements and the withheld-blob walk.	2026-07-13 10:46:24 -05:00
beardthelion	6176fbb697	docs(config): add GITLAWB_MAX_CONCURRENT_GIT_OPS to .env.example (#174) ... The read-pool knob was referenced by the push and per-caller entries' comments but had no example line of its own, so operators couldn't discover it from the template. Add it with the config default (128).	2026-07-10 16:05:29 -05:00
beardthelion	b519251e00	docs(node,config): reconcile the concurrency-cap comments with the fix (#62) ... The max_concurrent_git_ops and git_service_timeout_secs doc-comments (and .env.example) described the info/refs and withheld-blob paths as unbounded follow-up gaps. Both are now closed (#174): the comments reflect the read/write pool split, the per-caller sub-cap, and that every capped path is duration-bounded with process-group teardown. Verified the pattern-doc pre-ship checklist: every git_permit / write-permit / per-caller site holds only a timeout+teardown git path, and all three size knobs are range(1..=1_048_576). Closes the #174 work. No behavior change.	2026-07-10 13:20:18 -05:00
beardthelion	2c6cef6ad0	feat(node,config): add write-pool and per-caller read-cap knobs (#62) ... Add max_concurrent_git_pushes (default 32) and max_concurrent_reads_per_caller (default 16), both clap range(1..=1_048_576) so an oversized value is a clean CLI error, not a Semaphore::new boot panic. The per-caller knob documents that per-source-IP keying is only as granular as GITLAWB_TRUSTED_PROXY. Wiring lands in the following commits; these are the config surface for the #174 concurrency-fairness fix. Resolves jatmn P1a/P1b groundwork on #174.	2026-07-10 12:18:07 -05:00
3 people	dfcaa22b23	fix(node): rate-limit repo/agent creation per client IP to stop DID-farm spam floods (#180) ... * fix(node): add per-IP flood brake to repo/agent creation endpoints The recurring spam-repo floods are not bypassing the iCaptcha gate — they pass it. The enforced level (3) draws only machine-solvable deterministic challenges (arithmetic/algebra/sequence), and the /v1/challenge API lets the caller pin the easy type, so a bot mints a fresh throwaway did:key, solves a real proof bound to that DID, and creates a repo. The per-DID creation limiter never trips because each DID makes a single create call. Add a per-client-IP rate limiter to the creation route group (repos, register, fork, issues, pulls), reusing the same IpRateLimiter the push path already uses against the identical DID-farm bypass. It wraps the auth layer (outermost) so flood traffic is rejected before signature verification. Default 120/hour/IP, GITLAWB_CREATE_RATE_LIMIT overrides, 0 disables. * docs(env): document GITLAWB_CREATE_RATE_LIMIT in .env.example * fix: apply CodeRabbit auto-fixes Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <[email protected]> * test(node): unit-test the per-IP creation brake runs before auth The route-level coverage (repo_creation_is_rate_limited_by_ip) is #[sqlx::test] and needs Postgres, so it is skipped in the plain unit-test pass. Add a DB-free middleware test that layers the per-IP creation limiter over an always-401 auth stand-in and asserts an exhausted IP is rejected with 429 before auth runs — the ordering the DID-farm flood defense relies on. --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit <[email protected]>	2026-07-10 19:44:04 +08:00
3 people	2109d08e2c	Fix/db startup resilience (#170) ... * Keep node alive during database outages * Fix startup-resilience gaps: reachable 503s, bounded retries, seamless handoff - Route sqlx errors buried in anyhow chains back to AppError::Db and widen the unavailable class (Io/Tls/pool), so DB outages return 503 instead of 500 — the previous 503 arm was unreachable dead code - Bound each connect+migrate attempt (GITLAWB_DB_CONNECT_TIMEOUT_SECS) so a held migration advisory lock cannot wedge the startup retry loop forever - Share one bound socket between the degraded and full servers and stop awaiting the degraded drain: no closed-port window during the handoff and no slow client can block the full server from starting - Probe the database from /ready (2s bound) and gate Fly routing/deploys on it, so mid-life outages drain traffic and a bad DATABASE_URL fails the deploy visibly instead of going green; classify auth/catalog errors as likely-permanent (error-level log, straight to max backoff) - Start the metrics listener before the DB connects so the degraded window is observable; degraded /health now returns 503 so peer pings and uptime monitors stop counting a DB-less node as alive - Move the five GITLAWB_DB_* knobs onto clap Config (validated, in --help), drop the redundant outer connect timeout, default the pool to 20 conns, fix the backoff exponent cap that defeated GITLAWB_DB_RETRY_MAX_SECS, and strip at most one .git suffix in smart-HTTP repo names * fix: apply CodeRabbit auto-fixes Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <[email protected]> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit <[email protected]>	2026-07-10 09:44:02 +08:00
beardthelionandt	cd67718f49	fix(node,git): bound a hung served git with a total-duration timeout (#62) (#165) ... * feat(node,git): bound a hung served git with a total-duration timeout (#62) Part 1 of #62. A served git upload-pack/receive-pack that neither finishes nor disconnects parks forever, pinning a git PID (and, for receive-pack, holding the write lock); on a public repo an anonymous caller can trigger it. No tower TimeoutLayer exists and axum imposes no default. Wrap the child interaction in tokio::time::timeout, bounded by a new GITLAWB_GIT_SERVICE_TIMEOUT_SECS knob (default 600, must be positive). Drain stdout/stderr concurrently with the stdin write so a large body can't deadlock. On expiry run_git_service returns a typed GitServiceTimeout and reaps the WHOLE process group before returning (SIGTERM so git clears its .git/*.lock, wait for the leader and its grandchildren like index-pack to exit, escalate to SIGKILL past a grace, hard-capped), so a caller releasing the receive-pack write lock can't race a still-live git on the same repo. The handlers map GitServiceTimeout to a distinct 504 (not the generic 500 git error, and clear of the read-gate 404 / auth 401 the client keys retries on) via a pure, unit-testable classifier rather than matching the anyhow string. Surface git's own non-zero exit (its stderr) before a stdin-write EPIPE so a malformed body is classified 400, not 500. Out of scope, deferred in #62: the info/refs advertisement and the withheld-blob path (spawn_blocking, which tokio timeout cannot cancel); both remain unbounded (noted in the config knob's docs). Tests: the timeout fires and tears the group down; it reaps the whole group before returning (RED if the reap is leader-only); the wrap is load-bearing (RED via an outer bound, not a hang); the error must be the typed timeout; the 504 mapping and classifier are unit-tested; config rejects 0. Full suite green. * fix(review): harden #62 timeout tests and drop a dead reap flag - test(node): cover the `protocol error` arm of git_service_app_error independently (the input has no "bad line length" substring, so it isolates the second classifier arm and goes RED if that arm is removed) - test(node): make the reap-before-return test's grandchild IGNORE SIGTERM (`trap "" TERM`) and outlive the ~4s reap cap, so the SIGKILL escalation is load-bearing. Previously the grandchild self-exited under the cap, so neutering the SIGKILL still passed; the escalation was exercised but not required. Verified: RED with SIGKILL disabled, GREEN with it. - test(node): add a receive-pack timeout test. Every prior timeout test used an upload-pack fake; this proves the push path (which also holds the repo write lock) is bounded too. RED-verified with the internal timeout removed. - test(node): poll for the pidfile in the reap-before-return timeout test so a loaded runner can't false-panic before the fake git writes it - remove the redundant `sigkilled` guard in reap_group_on_timeout; step == 200 fires exactly once in the 0..400 loop, so no re-entry guard is needed and the SIGKILL still fires exactly once * fix(node): log malformed receive-pack as warn, not error (#165) git_receive_pack's error classifier routed every non-Timeout error, including client-caused BadRequest (400), through tracing::error!. Mirror the git_upload_pack arm so a malformed push logs at warn level instead of as a server error. * test(node,git): make smart_http timeout tests pass under the parallel runner (#165) Addresses jatmn's review of 41eca0b. [P2] The #62 timeout tests failed under `cargo test --workspace`: - run_git_service_tears_down_group_when_future_dropped re-polled a completed future ("async fn resumed after completion"): the advance-until-pids loop ignored the timeout's Ok(_) (future-finished) case and polled the resolved future again. It now breaks on early completion. - Under fork-storm load a freshly-written fake `git` transiently fails to exec (ETXTBSY) or is timed out before recording its pids. Added a fake_git_run_with_pids retry helper (used by the four pid-reading tests) plus a matching inline retry on the drop test, with growing backoff for the correlated bursts and a 12-attempt cap that still fails loudly on a genuine never-spawns regression. Reverted two tests from a speculative 2000ms bound back to 300ms/1000ms now that the retry covers the timeout-vs-pidfile race. [P3] Documented GITLAWB_GIT_SERVICE_TIMEOUT_SECS in .env.example and the README config table (default 600; bounds upload-pack/receive-pack, not info/refs or the withheld-blob path). The tests stay load-bearing: breaking process_group(0), the post-reap disarm, the timeout-arm reap, or the internal timeout each still turns the corresponding test red (mutation-verified). --------- Co-authored-by: t <t@t>	2026-07-10 08:54:59 +08:00
beardthelionandt	d00d89ae5b	fix(node): gate POST /api/v1/sync/trigger and rate-limit the peer-sync routes (#82) (#161) ... * feat(node): add per-client-IP limiters + knobs for the peer-sync routes (#82) Plumb two dedicated per-IP RateLimiters into config/state so the peer-sync enqueue routes can be braked independently: sync_trigger_rate_limiter (tight, GITLAWB_SYNC_TRIGGER_RATE_LIMIT, default 60/hr) and peer_write_rate_limiter (generous, GITLAWB_PEER_WRITE_RATE_LIMIT, default 600/hr). Both are bounded and swept by the periodic cleanup loop, keyed via the node-wide push_limiter_trust. No route is wired to them yet; that lands next. * fix(node): require a signature on /sync/trigger and rate-limit the peer-sync routes (#82) /api/v1/sync/trigger was reachable unauthenticated in the default config and drove an O(peers) outbound fan-out + per-repo sync_queue enqueue — an anonymous amplification/DoS. The same enqueue_sink is reachable via /sync/notify, which also accepts unsigned known-peer requests and had no rate limit. - Move /sync/trigger into its own always-signed route group (add_auth_layers), independent of require_signed_peer_writes, and give it a tight per-IP brake. - Add a separate, generous per-IP brake to the peer-write group (/peers/announce, /sync/notify); notify's signature behavior is unchanged (rolling-upgrade compat), it only gains the brake. Separate buckets so an unsigned notify flood can't drain the signed trigger caller's quota. - Per-IP (not per-DID): a did:key farm self-registers, so a signature/per-DID gate does not cap cost (INV-10); the brake keys on the client IP before auth. Adds an adversarial test matrix (unsigned->401 both modes, IP flood->429, DID-agnostic brake, forged X-Forwarded-For can't bypass, notify flood->429, distinct-IP not throttled, cross-route non-exhaustion, 0-disables-but-sig-holds, announce still accepts unsigned). * fix(gl): surface node denial on sync trigger and sanitize the error body (#82) Now that the node gates /api/v1/sync/trigger behind a signature and rate limit, 'gl sync trigger' must not render a 401/429 as a fabricated '✓ sync triggered / 0 peers' success. Check resp.status() before parsing; on a non-2xx, extract the node's message and bail. Strip C0/C1 control bytes from the node-supplied string before printing (INV-6) — a hostile node could embed ANSI/OSC escapes in the body. Adds mockito tests: 401/429 surface as errors (RED without the status check), control bytes are stripped, and the 200 path is unchanged. * fix(review): bound gl error-body read, strip bidi controls, genericize 429 message (#82) Code-review follow-ups on the #82 change: - gl sync trigger: read the node error body with a capped reader (read_body_capped) instead of resp.text(), so a hostile/broken node can't force an unbounded allocation to surface a denial (INV-6, read half). - sanitize_node_msg: also strip Unicode bidi/format controls (U+202A-202E, U+2066-2069, LRM/RLM/ALM) — is_control() only covers the Cc category, but a right-to-left override can reorder the displayed error line. - too_many_requests(): the shared per-IP 429 middleware now serves the peer-sync routes too, so drop the push-specific wording ('rate limit exceeded'); the push-specific 429 in the receive-pack handler is unchanged. Adds tests for the bidi/length sanitize and the oversized-body path. * test(#82): close reasoned-not-run gaps with load-bearing tests Executes the branches that were previously only reasoned: - read_body_capped byte-cap test (RED against resp.text() — 2MB body -> ≤cap): vets the bounded-read fix, not just the display cap. - sync-route 429 body asserts 'rate limit exceeded' and NOT 'push' (RED before the message was genericized); also finishes genericizing the warn log, which the response-body change had missed. - real RFC 9421 signature through the full router reaches the handler (200) in both config modes — the positive path through the actual gate, not a direct-mount with an injected DID. - /peers/announce flood -> 429 (the peer_write brake covers announce, not just notify). - trigger_counts extraction unit test (happy + malformed -> 0,0), closing the count-formatting gap in the 200 path. No production behavior change beyond the warn-log wording. * fix(#82): require an identity for sync trigger and fix the rollout docs Address review feedback on the always-signed /sync/trigger route. - gl sync trigger now requires a local identity via load_keypair_from_dir and fails locally with "identity not found" before issuing a request, instead of .ok() silently sending an unsigned request that can only 401 remotely. Add a test proving it fails before dialing the node. - Split /sync/trigger out of the staged unsigned peer-write rollout in the README and the OSS-readiness audit; only /peers/announce and /sync/notify keep the rolling-upgrade unsigned behavior. - Reorder the two peer-sync rate-limit vars in .env.example for dotenv-linter. - Bump crossbeam-epoch to 0.9.20 (RUSTSEC-2026-0204), matching main, so cargo audit passes. --------- Co-authored-by: t <t@t>	2026-07-08 08:44:55 +08:00
3 people	2df6ff9d30	fix(node): close two spam-vector root causes (trust upsert + ungated push) (#152) ... * fix(node): close two spam-vector root causes (trust upsert + ungated push) The June 2026 spam wave kept re-materializing after cleanup because two paths let an unauthenticated-by-captcha actor keep operating: 1. `update_trust_score` was an upsert. Any authenticated push/issue/PR from a DID with no agent row silently INSERTed one with a fresh `registered_at`, bypassing the iCaptcha gate on /api/register — so deregistered spam DIDs came back the moment they pushed. Make it an UPDATE-only no-op for unregistered DIDs; registration is the sole way into the agents table. Regression test added. 2. `git-receive-pack` had no rate limit. The per-DID limiter on the creation routes can't brake a push flood from a DID farm (one throwaway identity per repo), so the node absorbed several pushes/sec per IP, each a Tigris round-trip + peer-notify fan-out. Add a per- client-IP limiter (Fly-Client-IP / X-Forwarded-For) on the push path, default 600/h, `GITLAWB_PUSH_RATE_LIMIT` override, 0 disables. Fails open without a proxy header so self-hosted nodes aren't broken. Co-Authored-By: Claude Opus 4.8 <[email protected]> * fix(node): address PR review — advertisement throttle, trusted-proxy IP key, bounded limiter Resolves the review findings on the push rate limiter: P1 — advertisement path bypassed the limiter. A push hits GET info/refs?service=git-receive-pack first, which calls acquire_fresh() (a Tigris download); it sat in git_read_routes under only optional_signature, so the flood was reachable via the cheap GET without ever sending the throttled POST. Now git_info_refs applies the same per-IP brake before the fresh acquire. P1 — client-supplied forwarded headers were trusted. The key came from Fly-Client-IP / the first X-Forwarded-For hop, both client-settable, so a flooder could rotate the header and never fill a bucket. Replaced with a TrustedProxy policy (GITLAWB_TRUSTED_PROXY = fly | x-forwarded-for | unset): trust only the operator's edge header, fall back to the real socket peer (ConnectInfo) otherwise. Fly config trusts Fly-Client-IP; the AWS/Caddy compose trusts the rightmost X-Forwarded-For hop. P2 — unbounded limiter key set / insert-before-reject. Added a max_keys cap (reject-before-insert, inline eviction when full) so a varied key cannot grow the map, and a rejected request never allocates. Applied to both the per-IP and per-DID limiters. P2 — malformed forwarded header disabled the brake. Empty leading XFF hop returned None and skipped the limiter; empty Fly-Client-IP returned Some(""). client_key now takes the rightmost XFF hop and always falls back to the peer, so a malformed header can never turn the limiter off. P2 — missing tests. Added: middleware 429 path, per-peer isolation, key-cap eviction/rejection, trusted-proxy header resolution, and an integration test asserting the receive-pack advertisement is throttled before the acquire. Also serve with into_make_service_with_connect_info so the peer address is available. New env: GITLAWB_PUSH_RATE_LIMIT, GITLAWB_TRUSTED_PROXY (documented in .env.example). Co-Authored-By: Claude Opus 4.8 <[email protected]> * Update infra/aws/compose.yaml.tftpl Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(node): amortize the rate limiter's capacity eviction scan `RateLimiter::check` ran a full O(max_keys) `retain` sweep on every new-key miss once the map was at capacity — which is exactly the distinct-key-flood state the cap defends against, so the limiter serialized all traffic behind a 200k-entry scan per request under the mutex. Gate the inline sweep to run at most once per `sweep_interval` (min of the window and 1s), tracked by `last_sweep`. Between sweeps a new key is rejected without scanning; the background `cleanup()` loop still reclaims independently. The fast path for existing keys is unchanged, the key cap invariant holds (map never exceeds max_keys, rejected keys never allocate), and post-flood self-healing stays bounded to one interval. Added a test asserting a burst of capacity misses does not trigger repeated eviction scans. Co-Authored-By: Claude Opus 4.8 <[email protected]> --------- Co-authored-by: Claude Opus 4.8 <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>	2026-07-05 17:31:21 +08:00
Kevin CodexandClaude Opus 4.8	adc20f9eff	feat(node): iCaptcha proof-of-intelligence gate on create_repo + register (#108) ... * feat(node): iCaptcha proof-of-intelligence gate on create_repo + register Add an optional gate that requires callers to present an iCaptcha proof (an Ed25519-signed token from icaptcha.gitlawb.com, earned by solving an escalating challenge) on the two spam-prone endpoints. Proofs are verified OFFLINE with ed25519-dalek against the published public key, and bound to the authenticated agent DID so they cannot be shared. Backward-compatible by default: ICAPTCHA_MODE is `off` unless set, so deploying changes nothing. `shadow` verifies and logs would-be rejections but always allows; `enforce` rejects requests lacking a valid, strong-enough proof. If the public key cannot be loaded at startup the gate stays inert (fail safe) so an iCaptcha outage can never break repo creation or registration. - New src/icaptcha.rs: env-driven verifier (OnceLock), /v1/pubkey fetch at startup or ICAPTCHA_PUBKEY override, proof parse + signature + expiry + level + DID-binding checks; mode-aware decide() split out for testing. - Wired into create_repo and register (X-ICaptcha-Proof header). git push (receive-pack) is intentionally NOT gated. - 11 unit tests incl. verification of a real captured proof (JS-signed, Rust-verified), tamper/expiry/wrong-DID/level/missing-header rejections, and the off/shadow/enforce/fail-safe mode matrix. Author: Kevin Codex <[email protected]> * fix(node): address review on icaptcha gate (validate config, bound key fetch) Addresses CodeRabbit and @jatmn review feedback on #108: - parse_mode now returns Option and rejects unrecognized ICAPTCHA_MODE values; init() warns and falls back to off instead of silently disabling the gate on a typo like "enforced". [P2] - ICAPTCHA_REQUIRED_LEVEL: warn on a non-empty unparseable value instead of silently lowering the threshold to the default. [P2] - fetch_key uses a reqwest client with a 5s timeout so a hung /v1/pubkey can never block node startup; still fails inert on timeout/error. [P2] - .env.example: reorder ICAPTCHA_* keys (MODE, PUBKEY, REQUIRED_LEVEL, URL) to satisfy dotenv-linter. [P3] - add parse_mode unit test (rejects junk, accepts documented values). cargo test -p gitlawb-node icaptcha: 12 passed. clippy + fmt clean. Author: Kevin Codex <[email protected]> * fix(node): consume iCaptcha proof jti so a proof can't be replayed Addresses @jatmn P2 on #108: the verifier checked exp/level/sub but never consumed the proof's jti, so one solved challenge was a reusable bearer proof for that DID until expiry, covering every gated create_repo/register in the window. Now, in enforce mode, each verified proof's jti is recorded once and replays are rejected. - db: migration v7 adds icaptcha_consumed_proofs(jti PK, expires_at); new consume_proof_jti (INSERT ON CONFLICT DO NOTHING -> bool) and sweep_expired_proofs. DB-backed so it holds across restarts and instances. - icaptcha: ProofClaims carries jti; verify returns the claims; decide() stays pure and returns Allow | Reject | Consume{jti,exp}; check() is now async and spends the jti via the DB, rejecting replays (enforce only; shadow observes without consuming). - handlers: create_repo/register await check(&state.db, ...). - main: periodic sweep of expired proof rows alongside rate-limit cleanup. - tests: 12 pass incl. decide() yields Consume with the proof's jti. Author: Kevin Codex <[email protected]> * fix(node): consume iCaptcha proof only after request passes validation Addresses CodeRabbit (Major) on #108: check() spent the jti as soon as the proof verified, but create_repo/register ran it before name/DID/existence checks, so a rejected request (bad name, already-exists, wrong DID) permanently burned a valid proof without the action succeeding. Split verification from consumption: - icaptcha: replace check() with verify_request() -> ProofGuard (verifies and rejects invalid/missing proofs early; off/shadow/inert yield a no-op guard) plus ProofGuard::consume(db) which spends the jti right before the write and rejects replays. Guard is #[must_use]. - create_repo/register: verify_request() up front, then consume() immediately before the first write, after cheap validation passes. Replay protection unchanged (consume-before-write). 12 tests pass; clippy clean. Author: Kevin Codex <[email protected]> * fix(node): give iCaptcha rejections a dedicated error code Addresses CodeRabbit on #108: iCaptcha failure paths returned AppError::Unauthorized, which maps to the "not_an_agent" API code and conflates a valid-but-unproven agent with a failed signature. Add a dedicated AppError::IcaptchaProofRequired variant mapped to the "icaptcha_proof_required" code (HTTP 401), and use it for both the missing/invalid-proof and replay paths so clients can distinguish "solve a challenge" from "your signature is invalid". 12 tests pass; clippy + fmt clean. Author: Kevin Codex <[email protected]> * fix(node): gate fork_repo with iCaptcha and cover the proof ledger Address remaining PR #108 review comments: - fork_repo was the third repo-creation entrypoint and bypassed the iCaptcha gate. Verify the proof up front (fail fast on missing/invalid) and consume it only just before the git clone, so cheap rejections (bad name, conflict, withheld subtree) never burn a valid proof — matching create_repo's verify/consume split. - Add #[sqlx::test] coverage for the single-use ledger that the gate's anti-replay value depends on: consume_proof_jti (first-use vs replay vs distinct jti) and sweep_expired_proofs (only expired rows removed, unexpired spent proofs still block replays). - Return 403 Forbidden (was 401) for IcaptchaProofRequired: the caller is authenticated but forbidden without a valid proof. The distinct icaptcha_proof_required code is unchanged. - Recommend pinning ICAPTCHA_PUBKEY in production in .env.example to avoid the startup fetch dependency. The two shadow-mode review comments were already resolved by the earlier verify/consume refactor (decide/verify are IO-free; check_proof_jti gone). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>	2026-06-30 10:58:08 +08:00
3 people	0a15e763d2	feat(node): owner-only push enforcement behind GITLAWB_ENFORCE_OWNER_PUSH (#31) (#68) ... * feat(node): owner-only push enforcement behind GITLAWB_ENFORCE_OWNER_PUSH (#31) A valid did:key signature on git-receive-pack is authentication, not authorization: any party can generate a key, derive its DID, sign, and push to an unprotected branch. Owner checks only ran on protected branches, so a non-owner could write to unprotected branches of an arbitrary repo. Add an opt-in flag GITLAWB_ENFORCE_OWNER_PUSH (default false, mirroring GITLAWB_REQUIRE_SIGNED_PEER_WRITES) that requires the authenticated pusher to be the repo owner on every branch. Default-off keeps live nodes unaffected. Design follows the maintainer's review on #31: - Authorize on the canonical AuthenticatedDid injected by require_signature, via Extension<AuthenticatedDid> on the handler — not a re-parse of the headers. The legacy branch-protection check is left as-is. - Fail closed: an absent identity or a non-owner caller is rejected. The policy lives in a pure fn (owner_push_rejection) so it is unit-tested without a DB. - The gate runs before branch protection, so one rejection yields one error body. - Factor the duplicated owner-match into auth::is_repo_owner, reused by receive-pack, protect.rs, and visibility.rs (each keeps its own message). events.rs / peers.rs are left out — they aren't owner checks. - Name the push gate by intent (caller_authorized_to_push) so the planned Phase 2 UCAN git/push capability is a pure addition, not a rewrite. Tests: owner (full + short DID) allowed, non-owner rejected, missing-DID rejected, flag-off legacy. Docs: .env.example + docs/RUN-A-NODE.md. Co-Authored-By: Claude Opus 4.8 <[email protected]> * fix(node): address review on #68 — 403 status, finish owner-match consolidation Per @beardthelion's review: - Return 403 Forbidden (not 400) for authorization denials: the owner-push gate, the branch-protection denial, and the owner checks in protect.rs / visibility.rs. 400 reads as a malformed request and some git/CI clients retry it. - Finish the is_repo_owner consolidation: route the branch-protection check and the repos list filter through the helper. The branch-protection check now uses the verified AuthenticatedDid (auth.0) instead of re-parsing the headers. - Drop the second identity source on the push path entirely: the post-push event recording also uses auth.0, so extract_did_from_auth (and the handler's HeaderMap param + import) are removed. - Tests assert the Forbidden variant (so a future status regression fails loudly) and add direct coverage for is_repo_owner / caller_authorized_to_push. - Docs: warn that enabling the flag blocks delegated and CI agents until the Phase 2 UCAN git/push capability is honored. Co-Authored-By: Claude Opus 4.8 <[email protected]> --------- Co-authored-by: FrozenRaspberry <[email protected]> Co-authored-by: Claude Opus 4.8 <[email protected]>	2026-06-24 11:03:28 +08:00
Vasanth T	0b056f3ad0	[codex] fix Dependabot Rust alerts (#15) ... * fix(deps): resolve dependabot rust alerts * docs: clarify dependabot validation	2026-05-28 16:44:05 +08:00
Vasanth T	d366fc6b97	[codex] chore: add live-safe OSS readiness pass (#14) ... * chore: add live-safe OSS readiness pass * fix(node): verify optional peer signatures during rollout * docs: rewrite README for node roadmap	2026-05-28 15:34:47 +08:00
Kevin CodexandOpenClaude	6b9c5278d4	chore: initial public release ... Open-source release of the gitlawb node, CLI, and git remote helper. - gitlawb-node: Axum HTTP server with git smart-HTTP and DID auth - gl: command-line client (identity, repos, PRs, MCP server) - git-remote-gitlawb: git remote helper for gitlawb:// URLs - gitlawb-core: shared crypto primitives (DID, CID, HTTP sigs, UCAN) Dual licensed under MIT or Apache-2.0. Co-Authored-By: OpenClaude <[email protected]>	2026-04-29 16:34:27 +08:00