The cursor was the table-global bigserial, handed to clients verbatim and
accepted back as any non-negative integer, then passed straight into a
per-repo seq > $1. Two things followed from that, and both were silent.
A value issued by a DIFFERENT repository — or by this one before a
restore — is an ordinary larger number here, so the query matched
nothing, the surface answered 200 with an empty page and echoed the value
back, and the subscriber sat permanently past history it had never
received with nothing anywhere reading as an error. And because
repo_push_events is written by every repository on the node, the gaps
between one repository's cursors measured how much the others pushed in
between, private ones included.
The cursor is now an opaque token carrying a version, a binding to
(node DID, repository), and the id of the last event served. The binding
is a hash and not a MAC, because there is nothing to forge: the position
inside is a row id resolved against this repository's own rows anyway.
What it buys is that a foreign cursor is refused by shape, with a message
naming which mistake was made, instead of being applied to a repository
it was never issued for. An id that resolves to nothing — a token kept
across a restore that lost the row — is a 400 that names the recovery,
not an empty page the poller reads as being up to date.
The global sequence still orders the walk internally, which is what keeps
the page cheap, but it no longer leaves the node. The position is the row
id, which this surface already publishes in every event, so the token
discloses nothing new.
The two properties an earlier round established are kept, in their new
shape: next_cursor is never null and never moves backwards, and the start
of history is an expressible position rather than an absent one, so a
first poll of a repository with no events still hands back a token the
endpoint accepts back rather than a value a persisting poller would
misread.
receive-pack exits zero when the process ran, not when the push applied.
A non-fast-forward, a hook rejection or a failed update comes back as an
ng pkt-line inside a response this handler forwarded without reading, so
every side effect keyed on the refs parsed from the REQUEST fired for
refs that were refused: the pull request on that branch had its stored
head moved to a commit this node does not have, and the catch-up poll
surface handed subscribers a SHA that resolves to nothing.
The report is now parsed and only the refs it names with ok are used.
unpack fail accepts nothing, since the pack never landed. A report that
does not parse at all is inconclusive and keeps every declared ref: a
client that never requested report-status is told nothing about
individual refs, and reading that silence as rejection would permanently
drop the events and the head update for a push git really did accept,
which is the failure the poll surface exists to prevent.
The parser is ported from PR #384, which introduced it for this same
problem, so the two branches converge on one definition of "accepted"
rather than growing two. Whichever lands second should drop its copy. It
diverges by three lines, and they are load-bearing: git double wraps the
report when the client negotiates side-band-64k, which git push over
smart HTTP does. The observed framing from git 2.50.1 is an outer
pkt-line carrying a band byte whose payload is itself a pkt-line stream,
so one pass of strip_sideband leaves "000eunpack ok" as the first line,
the parse fails, and every ref falls back to inconclusive — the parser
answering "cannot tell" for exactly the pushes it exists to classify. A
second pass peels the inner layer and is a no-op on the single-wrapped
shape. The tests carry the real captured byte layout, both wrappings, so
this is pinned rather than assumed.
Both writes also moved inside the repository write lock. That lock orders
the ref updates of two concurrent pushes; released before these ran, the
pair was an unordered race, and B's unconditional UPDATE landing before
A's leaves the stored head — which the rollup prefers over its fallback —
pinned to a commit the branch has moved past. record_push_events has the
same inversion by a different route, since latest_push_sha_for_ref reads
the highest seq and seq order is insert order. There is no per-row
version to make the UPDATE conditional on, so the ordering has to come
from the lock. Holding it across two batched statements on an already
open pool, on a path that just ran a subprocess over the whole pack, is
the cheaper half of that trade. A source guard pins the position, because
this is a property of where the calls sit.
Separately, the 10,000-ref bound was checked only after the parser had
scanned the entire request and kept three heap strings per valid
pkt-line. The git routes raise the body limit to GITLAWB_MAX_PACK_BYTES —
2 GB by default — so a signed caller could force the whole scan and the
whole allocation and only then be told 400: the work the cap exists to
refuse, done in full before the refusal. The parser stops one past the
cap, which is still enough for bound_declared_refs to refuse on, and the
message no longer reports a count that would now be a floor rather than
the request's real one.
A claim is provenance, so the signature stored beside it has to reach the
bytes stored beside it. It did not. The signing string covers the body
only through a content-digest line, and require_signature rebuilds the
covered values from the request: with no Content-Digest header the
covered digest is the empty string on the signing side and the verifying
side alike, the Ed25519 check passes over method, path and nothing else,
and the digest-versus-body comparison was skipped for want of a header.
An owner could sign that once and post any body under it. The row that
produced carried a valid signature, a signing string that verifies under
the owner's key, and a request_body the signing string says nothing
about — history nobody can re-verify, which is the one thing the row is
for.
The write path now recomputes the digest of the bytes it is about to
persist and requires the signing string to carry exactly that line, which
is the write-time form of the check re_verify already ran against a
stored row. Line-exact rather than a substring search: build_signing_string
emits one component per line, and a contains would also be satisfied by
the digest appearing inside another component's value.
This is deliberately not the only line of defence, and deliberately not
the middleware's. PR #306 requires the header in require_signature and
closes the same hole one layer earlier for every signed route; that
belongs there, because no other route's correctness should depend on this
handler. Neither substitutes for the other. Without #306 the empty digest
is still accepted everywhere else; without this, a later relaxation of the
middleware resumes writing unverifiable history in silence. Nothing here
touches require_signature, so the two do not collide.
The route's advertised 8 KiB body bound was also a bound on what gets
stored rather than on what gets buffered: require_signature collects and
hashes the whole body before any handler runs, so by the time the handler
measured it the allocation had happened, and concurrent requests
multiplied exactly the cost the number was written to prevent. A
RequestBodyLimitLayer now carries the same figure at the transport,
outside the auth layers and inside the rate limiters, matching the shape
the git routes already use. The handler keeps its own check: it is the one
that holds for a handler reached by any other route table.
The hand-injected test material had to become coherent with the request
it rides on, because a stand-in built over other bytes now takes every
bare-router test down the refusal branch. It is built from the body it
accompanies and carries a real content-digest line.
The signature middleware handed every signed request a second handle to
the buffered body, and only the status write reads it. The field is
optional now and populated behind a marker the persisting route group
applies.
Be accurate about what that saves, because the first reading of this
finding overstated it and the plan was corrected before implementation.
The push handler takes its body as an extractor, so axum consumes the
request and drops every extension at that point; the extra handle lived
across the remaining middleware chain, not the handler's lifetime. The
whole pack really is pinned for the whole receive-pack, but by the
handler's own parameter, which is pre-existing and untouched here.
Layer order is the failure mode worth guarding. A marker applied inside
the auth layers is never seen by the middleware that reads it, so the
body silently stops being captured while every test stays green. The
mutation that proves this leaves the layer present on the same group and
only moves it inside, and the production-router test still reddens. The
handler also refuses an absent body rather than storing an empty column,
so a lost marker fails loudly at the one place it matters.
Four comments claimed a timestamp or a uuid tiebreak decided ordering
when the code orders on the sequence. Three were named in review; the
fourth turned up next to them, calling the timestamp format load-bearing
for a cursor that no longer reads it. The mirror-dedup comments that
correctly document timestamp ordering were checked and left alone.
The shared scrape helper advanced by one byte to stop an end anchor
matching at position zero, which reports a missing anchor for any region
starting on a multibyte character. It advances by a character now. Not
reachable today since every anchor is ASCII, but five guards share this
helper and one that silently finds nothing is worse than one that fails.
The sequence is a bigserial, so Postgres allocates it at INSERT and makes
it visible at COMMIT, and nextval does not roll back. Two overlapping
writes to one repo could therefore commit out of sequence order: one
allocates, another allocates and commits, a poller reads the later row
and advances, and the earlier row then becomes visible behind a cursor
that has already passed it. Never delivered, no error, no gap signal,
which is the one thing the catch-up surface exists to prevent.
This is the second time this branch has traded one ordering hazard for
another. The cursor moved off a wall clock because stamping and
committing disagree; it moved onto a sequence where allocating and
committing disagree.
The insert now runs in an explicit transaction that takes a per-repo
advisory lock, so the lock releases at commit and no writer can allocate
until the previous row is visible. The wait is bounded, because the lock
sits on a pooled connection on the inline push path and an unbounded
wait would turn one busy repo into pool exhaustion for unrelated
requests. Three attempts at a one second timeout leaves headroom under
the pool's own acquire timeout.
That bound needs the caller to stop swallowing failures. It warned and
continued, so any error the lock introduced would silently drop those
events forever while the push had already returned success, reaching the
same outcome by another route. It now retries once and then logs at
error level naming the repo, the refs, and the SHAs that were lost.
The defect is pinned by a test that owns both transaction boundaries
rather than a barrier. A barrier cannot reproduce it: before this change
the write was a single autocommit statement, so allocate and commit were
one round trip from the client and the interleaving window was never
open to a caller of the public API.
Both defects came from an independent cross-model review, and neither
was visible to the reviewers that share this session's model.
A signed write was protected only by clock skew: no nonce, no
idempotency key, no record of signatures already seen. So a captured
request replayed inside the skew window inserted a new row with a new
sequence number, and because the projection takes the latest claim per
producer and context, a stale success outranked the failure that
superseded it. The append-only design is what made this reversible
rather than merely duplicative: a replay that only duplicated a row
would be harmless, a replay that earns a fresh sequence flips the
answer. An earlier reviewer saw the replay window and concluded
append-only bounded it, which had the mechanism exactly backwards.
An exact replay is now idempotent instead of an error. The row carries a
digest over the signature, its input, and the body, a unique index
enforces it in the database rather than a check-then-insert race, and a
repeat returns the original claim. Erroring would have punished the
legitimate case this design otherwise handles worst: a client whose
response was lost retrying the same signed request and concluding its
report failed when it had succeeded.
The fan-out cap was mine, added while fixing unbounded work, and it
truncated at 256 refs after receive-pack had already accepted them. A
push whose pull request source branch sat past that point left the head
permanently stale with no push event, announced only by a log line git
had already contradicted. The writers chunk now, so nothing accepted is
dropped, and the bound that remains refuses before the accept rather
than after: chunking bounds a statement, not a request, and the work
that scales with ref count sits upstream anyway, one protection query
per ref before the service runs and one certificate and webhook per ref
after, with body limits disabled on the git routes.
Removing the cap also exposed a quadratic branch dedupe it had been
keeping cheap; that is a map now.
The push-event cursor paged on an application-stamped wall clock, so a
row stamped later could commit earlier and a poller past that point
would never see the earlier row. A clock step backwards widens it. It
now pages on a database-assigned sequence, the same decision
status_claims already made, so the two surfaces do not disagree about
what ordering means. The rollup's branch resolve had the same flaw and
is fixed with it.
The receive-pack path issued one round trip per ref for each of two
writers, sequentially, on the user's push, with nothing bounding the ref
count. Both are single statements now and the per-push fan-out is
capped, with a warning when it truncates rather than silent loss. The
head update batches through a VALUES join, with last-write-wins dedup on
duplicate branches: git will not produce them, but a join over
duplicates picks arbitrarily where the loop it replaced was
deterministic.
The cursor was unvalidated. A malformed value produced a wrong page
instead of a refusal, limit zero returned a page with a null cursor, and
a poller persisting that cursor silently restarted from the beginning of
history. It is validated, the limit is clamped, and the returned cursor
never moves backwards. This changes the query parameter names, which is
a breaking wire change on an endpoint that has never shipped; every
consumer was checked and only the route registration reads them.
Three copies of the source-scraping parser became one shared helper.
That refactor exposed a real gap: the empty-region mutation reddened
four of the five guards, but the gossip-containment guard passed on an
empty region because it only asserts a must-not. It now asserts its scan
covered what it claims to cover, so a helper bug cannot make it
vacuously green.
status.rs is split into status/mod.rs and status/tests.rs, production
logic unchanged. Four things read the old path and all were retargeted,
including 22 mutation targets across four specs, each re-confirmed to
still match exactly once.
Five reviewers independently flagged the capped insert. It counted then
inserted inside one transaction with a docstring promising a concurrent
writer could not slip past, which READ COMMITTED does not deliver, and
every cap test was sequential so deleting the transaction would have
kept them green. A per-repo advisory lock now serializes writers on the
bound they are both testing. The new test runs eight writers through a
barrier at cap minus one: without the lock it left 4, 5, and 5 rows
against a cap of 3 on consecutive runs, so the window was real.
The projection compared producer_did as a raw column while the handler
stored whatever spelling the signer used, so one owner writing as
did:key:X and as X produced two entries for one context and the
superseded claim kept voting. Identity is now canonicalized at write
time, and the read filter is a single equality rather than a set.
That fix had a trap. Applying normalize_owner_key directly, as the
finding suggested, collapses to the bare key, which is not a parseable
DID, and would have quietly undone the re-verifiability the previous
commit established. canonical_did canonicalizes to the full did:key
form instead and is written as a function of normalize_owner_key so the
equivalence with did_matches still holds exactly, including the case
where a naive prefix-add would merge two distinct identities.
With that in place the ported copy of the DID collapse in the status
module is gone rather than relocated, and did_matches itself now
delegates, so the Rust gate, the stored column, and the SQL all trace to
one definition.
The projection also selected the signature columns it never renders,
which the previous commit made worse by adding the request body. It has
its own narrow type now, enforced by a source-read test.
The per-repo cap counted for all time with nothing pruning the table, so
a repo that reached it was permanently closed while 429 told clients to
keep retrying. It is a rolling window now, which still bounds the
fan-out a caller-chosen SHA allows, and makes the refusal true.
Also folded two count queries that scanned the same rows into one.
The row stored the RFC 9421 signing string under a column called
signed_payload. That string covers the body only through the content
digest, so it proved someone signed a request with some digest but not
that this claim row was what they signed. The whole append-only argument
rests on a claim staying verifiable after the request is gone, so the
row now carries the request body as well, in a column that says so, and
a test re-verifies a claim from the row alone: body to digest, digest
into the signing string, signature over that string under the producer's
key. Its negative mutates the stored state and shows the same procedure
refuses.
Migration v24 is amended in place rather than stacked, since it has
never shipped.
The persisted material had no size bound and is caller-influenced, so
each field is now bounded. The write response returned the whole claim,
so the wire carried the stored bytes as a JSON integer array; it now
returns a client-facing type and StatusClaim no longer derives
Serialize, so the leak cannot reappear by accident.
Carrying the body as Bytes rather than a Vec matters: the middleware
runs on every signed request including receive-pack POSTs bounded at
2 GB, so copying would have doubled peak memory on every push to serve
a field only the status write reads.
Two review findings.
The fallback read branch_cids, whose only production writer sits inside
a Pinata pin-success branch. With no Pinata configured that table is
never written, so on a default node the fallback could never resolve and
every open pull request without a stored head answered head_resolved
false forever. It now reads repo_push_events, which the receive-pack
path writes unconditionally for every ref update, with both predicates
and the ordering in SQL against the existing keyset index. Still one
database read per request, still no repository acquire.
This is better, not complete: repo_push_events only carries pushes that
land after this ships, so a pull request whose branch was last pushed
before deployment still will not resolve until it is pushed again. There
is no backfill.
The best-effort persist propagated its error, so a transient database
failure turned a rollup that had already resolved its answer into a 500.
It now catches and logs, matching the sibling writes on the push path.
The test asserts the persist genuinely fails before driving the read, so
it cannot pass vacuously.
Also corrected a doc comment that claimed the resolve branch is never
taken twice for one pull request. Only the persist is once-per-pull-
request; the resolve attempt repeats on every read until it succeeds.
The accompanying test pins the corrected wording rather than a behavior
change, since the old code counted the same way.
The resolve counter is process-global while sqlx test databases are
per-test, so unguarded tests were inflating the counts guarded tests
assert on. The guard now covers every test that triggers a resolve.
Webhook delivery is spawned, fire-and-forget, and never retried, so a
checker whose endpoint was unreachable during a push simply never learns
about the commit. Rather than add retry machinery and an unbounded queue
pointed at an owner-supplied URL, delivery reliability becomes a
read-side property: a push records an event, and a subscriber walks
forward from a cursor to find every commit a webhook would have told it
about.
Containment is the load-bearing constraint. The row goes into
repo_push_events, written only by the receive-pack path and read only by
this repo-scoped, gate-checked handler. It must never go into
received_ref_updates, which the unauthenticated global feed also reads;
writing local pushes there would publish private-repo pushes on an
anonymous surface. A test pushes to a private repo and asserts nothing
appears on that feed, and retargeting the write reddens it.
The cursor emits a canonical Z-form timestamp with fixed sub-second
width rather than the default rfc3339. The default emits a +00:00
offset, and + decodes to a space in a query string, so a poller echoing
the cursor back re-read the same page forever. The fixed width also
makes the TEXT column's lexicographic order match time order for the
keyset predicate. Found by seeding a timestamp collision, not by
reading.
The scenario tests drive the recorder directly, so a source-scrape test
pins its call site in the push handler, the same way the stored-head
update is pinned. Removing that call was verified to turn it red.
GET /api/v1/repos/{owner}/{repo}/pulls/{number}/status answers for the
pull request's head. Both read surfaces now call one extracted
projection, so "the rollup uses the same projection as the commit read"
holds by construction rather than by two implementations agreeing today.
A pull request row stores branch names, so the head has to come from
somewhere. Preference order is the stored head, then a database-backed
branch lookup for an open pull request that has none. No path in this
module acquires the repository or lists refs from disk: that would
download the whole repository from object storage on a cold node, and
this read group carries no rate limiter. A source-read test enforces
that, and asserts its own scan still covers the handler so it cannot
shrink to vacuously passing.
The read-side persist is self-limiting by SQL, not by handler ordering:
the update is conditioned on the head being absent and the pull request
open, so a push landing mid-request cannot be rolled back to a staler
tip and a closed pull request is never back-filled. Reading the same
pull request twice resolves the branch once.
That property is invisible in the response. A variant that re-resolved
on every call and then still preferred the stored head would return
byte-identical bodies across the whole suite, so the test counts the
work done rather than the results emitted, and injecting exactly that
variant reddens only the counter assertion.
An unresolvable head is not a fifth wire state. The state field stays
inside the four values and the condition rides in head_resolved beside
the pull request's own state, so the two cases differ on that boolean
alone and both are tested.
GET /api/v1/repos/{owner}/{repo}/commits/{sha}/status returns the
projection over the claim history, computed per read rather than
materialized. A materialized index gated only at write time is how this
codebase previously kept serving a repo's slug, owner, branches, and
SHAs after it was made private, so the regression test writes a claim
while public, tightens visibility, and asserts an anonymous read finds
no trace of it.
Three outcomes stay distinct, which is the point of the unit. A caller
who cannot read the repo gets the repo's own not-found, compared byte
for byte against a missing repo so the endpoint is not an existence
oracle. A commit nobody has reported on gets 200 with the explicit
pending zero-count body, asserted exactly so no client can render it as
green. A failed lookup propagates as a 500 carrying the db_error code,
never as an empty success; nothing on this path uses ok-or-default.
Latest-per-context is decided by the database-assigned sequence, not by
the timestamp and not by the uuid. The ordering test seeds the winning
row with the lexically smaller id and a far older timestamp, so it fails
under either wrong key rather than passing by luck.
The projection filters to claims authorized by the current owner, so an
ownership transfer drops the prior owner's claims from the answer while
the history keeps them. That filter is a set-membership test inside the
query, which is the constraint the delegated-capability follow-on
inherits. It expands the owner DID into its equivalent forms, and a test
runs that expansion against did_matches over a ten-case matrix so the
two cannot drift apart silently.
POST /api/v1/repos/{owner}/{repo}/statuses/{sha} appends a status claim.
Authorization runs read-visibility first, then owner, and the order is
the security property: authorize_repo_read denies a quarantined repo
before the visibility gate and answers with the repo's own not-found, so
a caller who cannot read the repo cannot learn it exists. Loading the
repo and comparing the owner would answer 403 there and turn the
endpoint into an existence oracle. That case is tested directly, since
it is what separates a correct implementation from a plausible one.
Three caps, all evaluated in the insert transaction and all refusing
with 429: per producer and context, distinct contexts per commit, and
total rows per repo. The per-tuple cap alone bounds nothing, because the
context string and the SHA are both caller-chosen and the SHA is never
existence-checked. The route group also carries the per-DID and per-IP
limiters that the creation routes use; the plain write group carries
neither and this endpoint appends on every call.
Wider than the endpoint: require_signature now attaches the verified
RFC 9421 material as a request extension, so a handler can persist what
the signature actually covered. Rebuilding the canonical string in the
handler would store a string nobody verified, and none of it survives
the request. The write fails closed with a 500 when that material is
absent, rather than storing an empty payload, because a claim nobody can
re-verify is not history the later attestation substrate can adopt.
Nine injected-defect mutations confirm the gates are load-bearing rather
than decorative, including that removing the route group's merge leaves
a group that compiles, lints, and does not exist at runtime.
The rollup needs an honest target commit, and a pull request row stores
only branch names. Resolving the branch at read time is racy (the SHA
moves between the resolve and the rollup) and gives a merged or
deleted-branch pull request no answer at all, since the merge commit is
computed and never persisted.
The receive-pack path now sets head_commit for open pull requests whose
source branch matches an updated ref, one statement keyed on repo,
branch, and open status. Excluding closed and merged rows from the WHERE
clause is what freezes the value. The merge path stamps the source head
it actually consumed, which covers a push landing between the handler
reading the row and committing the merge.
A ref arriving on the push path is a full refs/heads/ name while
source_branch stores the bare branch, so the normalisation is tested
directly; a silently non-matching WHERE clause would make this a no-op
that still reads green. Branch deletions are skipped rather than stored,
since 40 zeros is a target no commit resolves to.
The scenario tests drive the update helper directly, because a real
receive-pack POST needs a pack file. That proves the mechanism and says
nothing about the sink, so a source-scrape test pins the call site in
the push handler. Removing that call was verified to turn it red.
Schema for the commit-status surface, all three objects in one versioned
entry rather than three: an append-only status_claims table, a nullable
head_commit column on pull_requests, and repo_push_events for the
catch-up poll surface.
Version 24 rather than 18 because origin/pr-173 and
origin/fix/issue-135-ipfs-cid-tree-gate both claim through 23, and the
migration runner keys only on version without comparing the recorded
name, so a collision is a silent full skip rather than an error.
status_claims orders on a database-assigned bigserial seq, not the uuid
id. A uuid v4 is random, so using it as the timestamp tiebreak would let
a retried pending claim beat a success roughly half the time when both
land in the same rfc3339 tick.
Each claim stores the producer's RFC 9421 signature headers and the
canonical signed bytes. Append-only preserves rows, but only stored
signature material preserves provenance, and a claim nobody can verify
after the request is gone is not history a signed-attestation substrate
can adopt later.
The table is repo_push_events, not push_events: the latter already
exists in v1 for agent trust scoring, so a second CREATE TABLE under
IF NOT EXISTS is a silent no-op whose follow-on index then fails.
The upgrade-path test is the load-bearing one. A sqlx::test provisions
an empty database and runs the whole array, so the fresh-chain tests
stay green even when DDL is appended to an already-applied entry.
Add a lone non-key DID fixture check to get_profile_does_not_match_non_key_did to verify that a bare short query returns None when only a non-key profile exists. Add profile_did_case_sql_matches_normalize_owner_key to pin PROFILE_DID_CASE_SQL against Rust normalize_owner_key across the full owner-key boundary matrix. Add cfg(unix) target gates to Unix-specific symlink and permission test helpers.
Refs #298
get_profile equates bare short ids with did:key forms. upsert_profile
must UPDATE by existing.did so a did🔑 alias against a bare-stored
profile actually persists. set_profile_cid uses the same CASE match.
Add a regression test covering both write paths.
Replace the broad LIKE suffix match in profile resolution with the same
normalize_owner_key / CASE pattern already used by get_repo, so non-key
DID methods that share a short-id suffix cannot resolve the wrong profile.
Add a sqlx test locking in bare, did:key, and non-key DID behavior.
Review of the previous two commits found the SQL correct and the comment
wrong in two places, plus two implementations of the intent that all four
tests accepted.
Comment fixes:
The claim that four http_url consumers "never read the flag at all" named
the public resolve route, which does read it: api/resolve.rs republishes it
as `reachable`, as does the peer listing. The accurate split is that only
the federated repo fan-out in api/repos.rs GATES on the flag, four consumers
act on a repointed URL regardless of it, and two surfaces republish it.
Three reviewers caught this independently.
The atomicity sentence implied the conflict row lock protects the flag. It
orders announces against each other and nothing else: mark_peer_ping writes
by DID with no http_url predicate, so a probe of the previous URL that lands
after a reset can still re-grant the flag until the next round. Said so, and
qualified the self-healing claim on cosmetic differences, which holds only
if no further announce lands first.
Two tests added, each proven load-bearing against a shape the original four
accept:
cosmetic_url_difference_counts_as_a_change
Pins the exact-comparison decision. Normalizing the comparison
(rtrim(peers.http_url, '/') vs rtrim($2, '/')) passes the other five and
fails only this one. The original four only ever compare identical or
wholly different hosts, so none of them constrain the comparison.
same_url_reannounce_still_advances_last_seen
The reset must ride the existing UPDATE, not gate it. Hoisting the
condition to a statement-level WHERE passes the other five while
skipping the update entirely on a same-URL re-announce, so liveness
stops advancing and the peer ages out on last_seen. Observed with both
timestamps identical.
Gates: cargo test -p gitlawb-node 516 passed 0 failed; clippy --all-targets
-D warnings clean workspace-wide; fmt --check clean.
Refs #270
The ON CONFLICT branch rewrote http_url and left last_ping_ok alone, so an
unsigned announce could repoint an existing peer's URL and inherit the
reachable=true that the previous host earned. api/repos.rs filters the
federated fan-out on that flag and then stamps each entry from the peer's
response body with the peer's DID, so the inheritance is what made the
injection instant rather than probe-delayed.
Clear the flag when the URL actually changes, and leave it alone when it does
not. In the DO UPDATE branch peers.http_url is the existing pre-update row and
the comparison runs under the conflict row lock, so this needs no second round
trip and does not race a read-then-write. Both writers funnel through
upsert_peer, so the announce handler and the bootstrap announce-back are both
covered.
Observed transition on the tests from the previous commit:
before url_change_clears_reachability ... FAILED (last_ping_ok assertion)
after url_change_clears_reachability ... ok
the other three ... ok throughout
Both guards were confirmed load-bearing by reverting, not by reading a green
suite. Removing the CASE clause entirely turns url_change_clears_reachability
RED again. Flattening it to last_ping_ok = (peers.http_url IS NOT DISTINCT FROM
$2) still passes three of the four cases and is caught only by
same_url_reannounce_does_not_grant_reachability, which is why that case exists.
This bounds the automatic inheritance; it does not close the rewrite. Four
other http_url consumers never read this flag, and until #248 lands the
unauthenticated ping route can write it back from the attacker's own host.
#273 is the closure.
Closes#270
upsert_peer's ON CONFLICT branch rewrites http_url and says nothing about
last_ping_ok, so a repointed peer keeps the reachability the previous host
earned. These four cases state the behavior the fix has to produce, and they
land before it so the failure is observed rather than assumed.
Pre-fix results:
url_change_clears_reachability ... FAILED
same_url_reannounce_keeps_reachability ... ok
fresh_peer_inserts_unreachable ... ok
same_url_reannounce_does_not_grant_reachability ... ok
panicked at crates/gitlawb-node/src/db/mod.rs:5737:
a repointed peer must re-earn reachability, not inherit it
The failure is on the last_ping_ok assertion; the http_url assertion above it
passes, so the row is being rewritten and only the gate is wrong.
The three that already pass are regression pins, not filler.
same_url_reannounce_keeps_reachability guards against clearing the flag on
every conflict rather than only on a change.
same_url_reannounce_does_not_grant_reachability guards the other direction and
is the only case that fails if the conditional is flattened to
last_ping_ok = (peers.http_url IS NOT DISTINCT FROM $2), which would let any
unsigned same-URL re-announce set the flag TRUE with no probe.
fresh_peer_inserts_unreachable guards the INSERT branch.
Refs #270
v12 was main's current_max + 1, which INV-7a says is necessary and not
sufficient: the runner keys the applied set on the integer alone, so a
version another in-flight branch also claims is skipped in full on
whichever side merges second. No error, no warning, schema_migrations
still reading healthy, and the column simply absent, after which
dequeue_pending_syncs fails on every poll and sync ingestion stalls.
Two open branches already claim into that range. #135/#173 holds
through 14 (15 once it rebases past main's v11) and #253 took 16. 17
clears both, and the reservation is recorded in a comment so the next
author can see it. Gaps are harmless; the runner iterates the array and
never requires contiguity.
Not doing the other half here. INV-7a's load-bearing rule is the name
check that makes the silent skip impossible rather than merely
unlikely, and that lands with #253 on test/replay-guards-253; adding it
on this branch would collide with that work.
Three review fixes on the two commits above.
The docstring claimed more than the code delivers. UPDATE ... RETURNING
does not order its output, so the batch is the right set in no
particular order (nothing in process_batch depends on the order); and
nothing is claimed, since the rows stay pending with no lock held past
the statement, so two workers against one database can still be handed
the same batch. Single-worker is the existing assumption and
FOR UPDATE SKIP LOCKED is what would change it. It also implied the
caller handles a failed stamp robustly; the caller logs and skips the
poll, which is worth stating plainly now that this writes and can fail
for reasons a plain SELECT could not.
The outer UPDATE now repeats status = 'pending'. Between the subquery
and the update a concurrent worker can settle a row, and without the
predicate the statement would stamp and return a row that had already
left the pending set.
Both starvation tests now assert the healthy row is still pending
before the poll that frees it. Without that they would keep passing if
the batch size ever grew past the stuck set, having silently stopped
exercising head-of-line yield, and the larger test now also asserts the
stuck rows rotated rather than settled.
A row the worker cannot make progress on stays pending, and while its
ordering key never moved it stayed among the oldest rows forever,
holding the fixed-size window against every healthy repo behind it.
Classifying more error kinds as terminal does not close this: an owner
directory that exists but denies traversal lets create_dir_all succeed
and defers at path_within_root instead, per-repo rather than as a
whole-repos_dir outage.
dequeue_pending_syncs now selects and stamps in one statement, ordering
by COALESCE(attempted_at, enqueued_at), so the key means least recently
handed out. Doing it in the dequeue rather than at each deferral branch
makes it hold by construction: no call site can forget it, a batch that
dies mid-loop still leaves its rows stamped, and a failed stamp is a
failed dequeue the caller already handles. enqueued_at keeps meaning
enqueue time so backlog age stays measurable.
The column lands as migration v12, not a v1 append: v1 never re-runs on
an existing install, so appending there would leave every deployed node
dequeueing against a column that does not exist. The upgrade-path test
simulates a v11 node and is the only test that catches that placement,
since every #[sqlx::test] otherwise starts from a fresh database.
N stuck rows still cost N slots per rotation, delaying a healthy row by
up to ceil(N/10) polls, so the attempts column and a retry cap remain
worth doing; this removes the starvation, not the residual cost.
create_dir_all reports AlreadyExists when a regular file, dangling
symlink, or link loop occupies repos_dir/<owner>, and none of those
clear on a retry. Classifying them as transient left the row pending,
and since dequeue_pending_syncs is oldest-first over a fixed batch of
10, a handful of them held the window and starved every healthy repo
behind them. This restores the pre-#272 behavior, where the same state
failed the clone and hit the terminal Err arm.
A concurrent mkdir resolves to Ok rather than AlreadyExists, because
create_dir_all falls back to is_dir() on EEXIST, so the race is not
caught by this classification.
Two follow-ups from code review.
The pending-on-IoError arm had no operator-visible signal: the only
record_sync_processed calls were the done and failed arms, so a queue
stalled on an unmounted repos_dir looked identical to an idle one. Both
deferring arms now record "deferred" and the guard rejections record
"rejected", which also distinguishes a retryable stall from a terminal
refusal. There is still no attempt cap; bounding retries needs an attempts
column on sync_queue, which is a migration and belongs in its own change.
The two escape tests were asserting that repos_dir stays empty, but the
owner directory is created before containment has a parent to canonicalize,
so that assertion was detecting the create_dir_all side effect rather than
an escape. They now assert no mirror exists anywhere under repos_dir.
Their comments were also wrong about which layer holds. Mutation testing
says the slug rule and containment are each independently sufficient for
that input: removing either alone leaves the tests green, and only removing
both turns them red, with the mirror observed written outside repos_dir.
The comments now say that, and point at the per-layer witnesses instead:
./hello isolates the slug rule because it resolves back inside repos_dir
and containment approves it, and the two symlink tests isolate containment
because no character rule can see a symlink.
Code review caught an availability regression introduced by the guard added
earlier on this branch. validate_owner_did bails only above 256 chars, but
the owner half becomes a single path component and Linux NAME_MAX is 255, so
a 256-char owner passed validation and then failed create_dir_all with
ENAMETOOLONG on every attempt. The worker left such a row pending, and
dequeue_pending_syncs is oldest-first with a fixed batch of ten, so ten
unsigned notify calls held the entire window and starved every healthy repo
behind them. Before this branch the same row reached git clone, failed, and
was retired.
Two changes. Reject the length in the shared validator, so an undeliverable
slug never enters the queue from either boundary. Then split the
create_dir_all arm by whether the error can ever clear: a transient one
still leaves the row pending to be retried, a permanently uncreatable path
is marked failed rather than re-picked forever.
The earlier test missed this by asserting 257, one past the bound. It now
pins both sides: 256 rejected, 255 still accepted.
Also make the two permission-bit tests fail loudly instead of returning
early. One probed with symlink_metadata on a path that was never created,
which is Err for every user, so its root guard could never fire; the other
returned green after an eprintln that cargo swallows on a passing test, so
a root runner would have reported ok while exercising nothing.
The two throwaway probes from the investigation, in attack form, asserting
the closed outcome. The router-level one drives the full router with no auth
extension, because the point it pins is reachability: the attack needs no
signature and no operator config change, only an unsigned announce followed
by an unsigned notify.
The worker-level one seeds the peer row directly, since process_batch
resolves the peer URL before it looks at the slug and an unseeded row dies
at the no-peer arm, which would leave the test green with the guard gone.
It asserts on the parent directory too, because git clone removes the
destination on failure but leaves the parent.
Note for reviewers: reverting the worker slug check alone does not recreate
the absolute-path escape, since containment catches it and leaves the row
pending. That check is independently load-bearing for ./hello, which
canonicalizes back inside repos_dir and so passes containment.
The character rules on the slug cannot see a symlink. Two cases get through
them: a symlinked owner directory, and a symlinked <name>.git leaf. The leaf
is the one a parent-only check misses, since the parent canonicalizes clean,
exists() follows the link, and the fetch branch then runs git -C through it
with the mirror refspec, force-overwriting refs outside repos_dir.
So the predicate canonicalizes the candidate when symlink_metadata says it
exists, and its parent only when it does not. Canonicalizing the candidate
unconditionally would reject every first clone, which is total loss of
mirroring, and the must-not-break test covers that direction.
Three-valued rather than a bool because the worker has to tell a hostile
path from a filesystem that could not answer. Outside is terminal; an I/O
error leaves the row pending, since dequeue only selects pending and a
transient EACCES would otherwise retire a legitimate repo permanently.
The predicate is pure. create_dir_all lives at the call site so the admin
purge path can ask a containment question without mutating anything.
/api/v1/sync/notify accepts unsigned callers by default, and its only gate
was that node_did appear in the peer table, which one unsigned announce of
the caller's own DID satisfies. req.repo then reached enqueue_sync verbatim.
Validate after the peer check so an unknown peer still gets the unknown-peer
message, and before enqueue_sync so nothing is written. The 400 names the
field and carries no path: the validator's text never interpolates the slug,
so a hostile string is not echoed back either.
NOTIFY_BODY used "demo", a slug with no separator, so the three brake tests
would have started asserting against the new 400 instead of the brake.
process_batch split item.repo on the first '/' and joined both halves onto
repos_dir. Since PathBuf::join does not normalize and an absolute second
component replaces the path, a queued `a//tmp/x` cloned an attacker's
origin to /tmp/x.git with repos_dir left empty.
The worker is the real choke point rather than defence in depth: three
writers reach sync_queue (the notify handler, trigger_sync, and the
gossipsub ref-update handler at p2p/mod.rs), and rows queued before this
change are already in the table.
The tests serve the composed remote URL for every hostile slug, not just
the escaping one. With an unreachable remote those rows end failed anyway
because the clone fails, so the scenarios would have passed before the
guard existed and proven nothing.
The sync queue carries a peer-supplied `repo` string that the worker turns
into a filesystem path, and `PathBuf::join` does not normalize, so an
absolute second component replaces the accumulated path. `a//tmp/x`
resolved to `/tmp/x.git`, outside repos_dir entirely.
Validate the slug once, in the module that already owns the owner and name
rules, and return the two halves so callers cannot re-split and drift. The
only rule added here is the leading `.`/`-` check on the owner half, which
`validate_owner_did` lacks because it also serves full DIDs.
Each guard is covered by a case that isolates it: the earlier rules shadow
the obvious candidates, so `../hello` does not actually exercise the
owner delegation and `a//tmp/x` does not exercise the separator count.
Settles the review's design question: a valid signature alone proves
internal consistency — the payload was signed by whatever key the
certificate itself names — so a hostile source could self-sign and
pass. --verify now also requires the issuing node_did to match a
trusted anchor: --expect-node when given, else the DID of the node
being queried; unanchorable (node info unreachable, no --expect-node)
fails closed. The displayed verdict wording says what it proves.
Also from review:
- A frozen-literal test pins gl's payload serialization to the node's
canonical byte form, so serialization drift (new field, a
preserve_order feature unifying across the workspace) fails a test
instead of silently rendering every real certificate INVALID.
- The printed unalias remediation uses the idempotent-safe
'unalias gl 2>/dev/null' form everywhere (doctor, install.sh, brew
caveats), matching what doctor already suggested.
- Acknowledged, deliberately left: bash-side alias detection and
127/8-range loopback matching (noted as out of scope for a
warning-level heuristic).
- cert show: the node-DID comparison is contextual; a node-info fetch
error now prints a note instead of failing the command after the
certificate and verdict were already shown.
- doctor: loopback detection parses the URL host exactly — substring
matching misread localhost.example and localhost-in-path URLs.
- init: a detached HEAD gets its own hint (switch to a branch first)
instead of a guessed 'git push gitlawb main'.
- status: every fetch AND push URL of each remote is inspected — a
gitlawb:// push URL behind an https fetch URL now counts.
- doctor/install.sh: unalias detection is command-aware — comments,
'unalias global', and gl-only-in-a-trailing-comment no longer
suppress the warning (BSD-grep-compatible pattern; ordering vs
oh-my-zsh load documented as accepted slack). Eight new test cases.
oh-my-zsh's default git plugin ships gl='git pull', silently shadowing
gl in every interactive zsh — the symptom is the baffling 'fatal: not a
git repository'. Aliases beat PATH, so no installer can fix it; doctor
now detects the setup (explicit alias gl= in ~/.zshrc, or the omz git
plugin without an unalias) and prescribes the one-line fix. Best-effort
heuristic, warning-level, covered by four unit tests.
A localhost GITLAWB_NODE red-flagged every self-hosted and dev-harness
setup even when the node was healthy; the connectivity check already
fails loudly when the configured node is unreachable. The env check now
passes with an explanatory note.
show printed the signing payload and told the user to verify offline
themselves. It now rebuilds the node's canonical payload (field order
mirrors gitlawb-node cert issuance), derives the Ed25519 key from the
certificate's node DID, verifies, and prints a VALID/INVALID verdict;
--verify makes the exit code reflect it for scripting. Round-trip +
tamper + malformed-signature unit tests included.
- git init -b main (plain-init fallback for git <2.28) so a fresh repo
can't land on master while every hint says main.
- The closing hint checks HEAD: with no commits it prints the
add/commit step first — a bare 'git push gitlawb main' at that point
fails with 'src refspec main does not match any', the exact trap the
acceptance test walked into. The current branch name is used in the
hint either way.
gl init names its remote 'gitlawb', but detect_gitlawb_remote() only
ever read origin — so gl status reported 'not in a gitlawb repo' in the
very repo gl init had just set up. Scan all remotes, preferring origin
(clones) then gitlawb, then any other gitlawb:// URL. (#231 acceptance
test finding)
PRs #113 and #145 raced: #145 added owner_did to ReceivedRefUpdate and
updated its own call sites, but the seed initializer in
list_repo_events_fails_closed_when_repo_lookup_errors (added by #113)
was never updated, so main fails to compile test targets (E0063).
This breaks clippy --all-targets, both test jobs, and MSRV on CI,
including the release PR #182.
owner_did stays None here on purpose: the test exercises the legacy
None-branch slug fallback.
Co-authored-by: Kevin Codex <[email protected]>
* fix(gl): point doctor's version check at Gitlawb/node, not the frozen Gitlawb/releases repo
check_version queried https://api.github.com/repos/gitlawb/releases/releases/latest.
Gitlawb/releases is a separate repo that stopped receiving tags after v0.3.8
(2026-03-30). Releases have shipped directly on Gitlawb/node since v0.4.0, via
release-please, and install.sh already treats Gitlawb/node as the canonical
release source. The old query target meant gl doctor's version check could
never again detect a real update.
Points the check at Gitlawb/node instead, matching install.sh's own default,
and makes the GitHub API base injectable so the fix is covered by a test
against a mocked server rather than the real GitHub API.
Closes#197
* fix(gl): check HTTP status before parsing the release response; strengthen up-to-date test
Addresses CodeRabbit review on #198:
- check_version now checks resp.status().is_success() before parsing the body,
so a GitHub rate-limit or other API error surfaces as its actual HTTP status
instead of a generic "could not parse latest tag".
- test_check_version_up_to_date now also asserts the detail message, so the
test can't silently pass on an unmatched mock request (mockito's fallback for
an unhandled request is a 501, which check_version also maps to Ok).
* test(gl): add unit test for non-2xx check_version response
* fix(gl): remove trailing blank line at EOF in doctor.rs
cargo fmt --all -- --check fails on the extra terminal blank line, and the
fmt + clippy workflow runs that check as a required gate.
* fix(core): honor wildcard capabilities in Ucan::can, matching is_attenuated_by
Ucan::can did strict equality on both resource and action, so a validly
delegated wildcard capability (with: "*", or can: "*" / "repo/admin") could
never match a concrete runtime request even though is_attenuated_by treats
those same values as wildcards during delegation-chain verification.
Not a security issue: the mismatch only produces false negatives (denying a
capability the delegation chain actually grants), never false positives.
Dormant today since no production code path calls can() yet — auth's
caller_authorized_to_push is pure did_matches(), with the UCAN-capability
check noted as future (Phase 2) work.
Makes can() mirror is_attenuated_by's semantics: the stored capability plays
the "parent" role, so with == "*" matches any resource and can == "*" or
"repo/admin" matches any action.
Closes#199
* test: add can_honors_action_wildcard unit test for Ucan::can
Kevin Codex