mirror of
https://github.com/Twigpine/node.git
synced 2026-11-10 05:01:48 -10:00
feat/commit-status-surface
110
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Kevin Codex
|
6b0763ac2a |
fix(api): make the push-event cursor opaque and bound to its repository
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. |
||
|
Kevin Codex
|
8652b86d33 |
fix(api): apply a push's side effects only to the refs git accepted
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. |
||
|
Kevin Codex
|
a4e2b9f3f3 |
fix(status): bind a stored claim to the body its signature covered
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. |
||
|
beardthelion
|
61033af261 |
fix(auth): carry the request body only where a route persists it
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. |
||
|
beardthelion
|
5445dda3f5 |
fix(db): stop the push cursor skipping a concurrently committed row
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. |
||
|
beardthelion
|
d3074a6c86 |
fix(status): make a replayed write idempotent and stop dropping refs
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. |
||
|
beardthelion
|
612d9d2dbe |
fix(api): order the poll cursor on a sequence and batch the push writes
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. |
||
|
beardthelion
|
806f9027c0 |
fix(status): make the caps hold and the projection mean one identity
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. |
||
|
beardthelion
|
ad71f79e6d |
fix(status): make a stored claim actually re-verifiable
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. |
||
|
beardthelion
|
90b8294c2b |
fix(api): resolve the rollup head from push events, not pinned CIDs
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. |
||
|
beardthelion
|
db3681e48d |
feat(api): add the catch-up poll surface for missed push webhooks
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. |
||
|
beardthelion
|
7be9f08b3d |
feat(api): add the pull request head rollup
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.
|
||
|
beardthelion
|
6663fed064 |
feat(api): serve the combined commit status behind the visibility gate
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.
|
||
|
beardthelion
|
ac31868205 |
feat(api): add the owner-gated commit status write path
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.
|
||
|
beardthelion
|
af17831429 |
feat(pulls): track the source branch head on open pull requests
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. |
||
|
beardthelion
|
04263dbd49 |
feat(db): add migration v24 for status claims, PR head, and push events
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. |
||
|
github-actions[bot]
|
cd810b3375 | chore(main): release 0.7.1 | ||
|
euxaristia
|
b4016dce9d |
fix(db): pin profile DID matching with lone non-key fixture and boundary matrix
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 |
||
|
euxaristia
|
9836c3a21b |
fix: target stored did on profile update aliases
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.
|
||
|
euxaristia
|
13b565bcac |
fix: strict DID matching in get_profile
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. |
||
|
Kevin Codex
|
1964f88378 |
Merge pull request #280 from Gitlawb/fix/270-reset-peer-reachability-on-url-change
fix(node): reset a peer's reachability gate when an announce changes its http_url (#270) |
||
|
t
|
811ad504bc |
fix(review): correct two comment claims and guard two more wrong shapes (#270)
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
|
||
|
t
|
e2560ee46c |
fix(node): reset a peer's reachability gate when an announce changes its http_url (#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
|
||
|
t
|
75e3e03c09 |
test(node): pin peer reachability across a URL change, a repeat, and a fresh insert (#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 |
||
|
beardthelion
|
fbc5868bd1 |
fix(node): move the attempted_at migration to v17, clear of two open branches
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. |
||
|
beardthelion
|
457be11d72 |
fix(review): stop the dequeue overclaiming, and pin the starvation premise
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. |
||
|
beardthelion
|
6e64d0d0a7 |
fix(node): stamp every dequeued sync row so a stuck one yields the batch
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. |
||
|
beardthelion
|
2e664b7885 |
fix(node): fail a sync row terminally when a non-directory holds the owner path
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. |
||
|
beardthelion
|
9aa4c17aef |
fix(node): make a stalled sync queue visible, and fix two test claims
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. |
||
|
beardthelion
|
27b0495a6f |
fix(node): bound the slug owner by NAME_MAX, not the DID limit (#272)
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. |
||
|
beardthelion
|
6456bfb0ba |
test(node): commit the #272 probes as regression tests
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. |
||
|
beardthelion
|
3ca94739ed |
fix(node): prove the mirror path is inside repos_dir before running git (#272)
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. |
||
|
beardthelion
|
c587a40a50 |
fix(node): reject a malformed slug at the notify boundary (#272)
/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. |
||
|
beardthelion
|
9f00e24aec |
fix(node): validate the queued slug before the worker derives a path (#272)
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. |
||
|
beardthelion
|
51d82ded57 |
fix(node): add a shared owner/name slug validator (#272)
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. |
||
|
github-actions[bot]
|
32006b26a2 | chore(main): release 0.7.0 | ||
|
github-actions[bot]
|
f9506b466e | chore(main): release 0.6.0 | ||
|
Kevin Codex
|
d5ec4695f4 |
test(node): add missing owner_did field to fails-closed seed update (#229)
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]> |
||
|
GravireiandGravirei
|
69a62e51c2 |
fix(node): carry full owner DID on ref-update wire event (#144) (#145)
* fix(node): carry full owner DID on ref-update wire event (#144) * test(node): add ref-update owner_did round-trip, DB, and API tests (#144) * fix(node): move owner_did migration to v10, add upgrade-path test * fix(node): add owner_did to GraphQL, broadcast, and REST feed surfaces (#144) * fix(node): address review feedback — drop ipfs regression, fix doc, improve migration test, use local owner_did * fix(node): fix migration v10 test to actually start from v9 schema; complete owner_did API assertion * fix(node): show local owner on global feed, assert nullable in migration test, add owner_did echo assertions * fix(node): prefer stored wire owner_did over lossy slug-local resolution; mirror projection in GraphQL * fix: address review findings — restore iCaptcha PoW/origin matching, fix feed owner_did projections * fix: match icaptcha-client Cargo.toml ordering with upstream/main sha2 was at line 16 in our branch but at line 18 in upstream/main. The 3-way merge in CI's pull/145/merge checkout kept both lines, causing a 'duplicate key' error. Moving sha2 after tracing matches upstream/main exactly, so the merge commit will have a single entry. * fix(node): bind wire owner_did to local repo before surfacing as trusted The peer-supplied owner_did (HTTP notify + gossipsub receive) and the repo slug arrive over untrusted paths, yet REST and GraphQL projected the wire value as trusted feed ownership. - P1: add Db::resolve_ref_update_owner_did so a wire owner_did is echoed only when it matches the canonical owner of the actual local repo at that slug; otherwise it is dropped (returns null). The canonical DID is returned, never the raw wire bytes. - P3: legacy owner_did = None rows are now attributed via an exact, normalized, unique local match (get_repo), not the prefix-tolerant drop gate, so a cross-method slug collision cannot synthesize the wrong owner. - Unify REST (global + per-repo gossip) and GraphQL on the shared resolver so the two surfaces cannot drift. Add tests for forged-wire drop and exact-legacy attribution. * style(node): apply cargo fmt to owner_did resolver * fix: batch owner_did resolution, propagate errors, preserve full DID for mirrors P2 batch: replace per-row join_all with resolve_ref_update_owner_dids that issues at most one get_repo query per distinct local repo in the page rather than one per event row. Applies to both REST and GraphQL surfaces. P2 error: REST handler now propagates DB errors (rather than silently coercing them to null) by calling the batch method with ?. P2 mirror: when the validated wire DID carries a full method prefix (e.g. did:key:z...) and the matching repo is a mirror storing only the bare key, return the full wire DID instead of the bare key to preserve the DID-method contract. P1/P3 already addressed in fb553e6. * fix: rebase onto current main and use single batch SQL for owner DID resolution Rebased from stale |
||
|
beardthelionandt
|
ee8d7c03b2 |
fix(node): gate /hooks and sibling read surfaces leaking private repo metadata (#94) (#113)
* fix(node): owner-gate list_webhooks to stop leaking webhook URLs (#94) list_webhooks sat on the optional_signature read group with no auth or ownership check, so any unauthenticated caller who knew a repo's owner/name could enumerate its webhooks (target URL, created_by_did, events; only the secret was redacted). Gate it read-visibility-then-owner, mirroring create_webhook/delete_webhook: headerless -> 401, non-reader of a private repo -> 404 (existence hidden, uniform with the sibling read surfaces), non-owner of a public repo -> 403. Add a list_webhooks row to the authz_guard drift table so the gate can't silently regress. * fix(node): read-visibility-gate list_replicas for private repos (#94) list_replicas sat on optional_signature with no visibility check, so a private repo's replica URLs were enumerable by anyone who knew owner/name. Replica lists are a documented public mirror-discovery surface, so gate on read visibility (authorize_repo_read) rather than owner-only: a public repo stays anonymously listable, a private repo returns 404 to anyone who cannot read it. register_replica registers non-owner DIDs, and a replica operator is not a visibility reader, so a non-owner replica operator of a private repo gets 404 — the intended contract, pinned by the new test. Add the authz_guard row. * fix(node): read-visibility-gate list_protected_branches (#94) list_protected_branches sat on optional_signature with no visibility check, leaking a private repo's branch names to any unauthenticated caller. Gate on read visibility (authorize_repo_read at the root, INV-2 root-listing exception): public repos stay anonymously listable, private repos 404 to a non-reader. Add the authz_guard row. * fix(node): gate list_repo_events for local private repos, keep gossip-only (#94) list_repo_events served a private repo's ref certificates and gossip ref metadata (ref names, SHAs) to any unauthenticated caller. The handler also tolerates a repo it knows only via gossip (no local row) and legitimately serves its events, so a blanket get_repo->authorize_repo_read swap would have 404'd that federation path. Gate only the locally-hosted branch: when get_repo returns a record, run visibility_check on it at the root and 404 on deny before fetching either the cert or gossip events. A gossip-only repo (no local row) keeps its existing public federation-feed behavior; the global /api/v1/events/ref-updates feed remains a separately tracked deferral. Add the authz_guard row (visibility_check marker, since the gate is direct to avoid a second get_repo). * fix(gl): sign the webhook list request now that GET /hooks is owner-gated (#94) `gl webhook list` built its NodeClient with no keypair, so the GET went out anonymously. With the node now owner-gating GET /hooks, that request returns 401. Thread a --dir through the List subcommand and load the keypair, matching `gl webhook create`/`delete`, so the listing is signed as the owner. (Replica and protected-branch listings stay anonymous for public repos under read-visibility gating, so their gl clients need no change.) * fix(review): sign the webhook list GET (get_signed), not plain get (#94) Code review caught that NodeClient::get() never attaches the RFC 9421 signature even when a keypair is loaded — only get_signed/post/put/delete sign. The earlier gl webhook-list fix loaded the keypair but still called get(), so `gl webhook list` (and the MCP webhook_list tool) 401'd against the now-owner-gated endpoint. Switch both to get_signed. Harden the two cmd_list tests to require a Signature/Signature-Input header on the mock (verified: they fail against plain get(), pass with get_signed) — the missing assertion that let the bug through. Add the untested owner-of-private- repo webhooks path (200 + redaction). Fix the stale module doc and the authz_guard bucket comment for list_webhooks. * fix(gl): sign read-visibility GETs so private-repo owners can read their own repos (#94) The #94 server change read-visibility-gates GET /replicas and /branches/protected: public repos stay anonymous, private repos require the owner/reader to authenticate. Four gl read commands sent unsigned GETs and so broke (404) or silently degraded (empty output) for a private repo's owner: gl protect list, gl repo owner (silent empty protected list), gl repo info (silent missing replica count), gl repo replicas (and it had no --dir flag). Add NodeClient::get_maybe_signed — signs when an identity is present, stays anonymous otherwise (mirrors the conditional signing of post/put/delete) — and route the four call sites through it. Add --dir to `gl repo replicas`. Harden the protect-list tests to assert the request is signed (verified: they fail on plain get()). Public-repo anonymous reads are unchanged. * fix(review): sign the primary repo fetch in gl repo info/owner too (#94) Round-2 review caught that cmd_info and cmd_owner loaded a keypair and signed their replica/protected-branch sub-fetch but left the PRIMARY GET /api/v1/repos/{owner}/{name} on plain get(). get_repo is read-visibility-gated, so for a private repo that primary fetch 404'd first and the command bailed before reaching the signed sub-fetch — the half-fix never actually let an owner inspect their own private repo. Route both primary fetches through get_maybe_signed. Add the missing signing guards: harden test_cmd_owner_is_owner to require a signature on both mocks, and add tests for cmd_replicas and cmd_info (both were untested) asserting the request is signed when an identity is present. Out of scope (pre-existing, gated before #94, untouched here): gl repo commits, gl pr list/view/diff, and the MCP repo_get/repo_commits/pr_* tools call other already-gated read endpoints unsigned and are broken for private repos independently of this change. * test(review): execute the previously-reasoned-only paths (#94) Close the vetting gaps from the review: - node: a listed reader (non-owner) passes the read gate but is still refused the webhook list (403), and headerless on an existing-private vs an absent repo both 401 — proving the 401 fires before any lookup, so it is not an existence oracle. - node: the read-visibility surfaces (replicas, protected branches) admit a listed reader who is not the owner (the allow-list branch of visibility_check), while a non-reader stranger still 404s. - gl: get_maybe_signed's anonymous fallback (no identity -> unsigned GET) is now exercised, RED-proven by flipping it to get_signed (which signs and fails the no-signature-header assertion). * test(review): execute the MCP dispatch and gl->node signature seams (#94) Close the last two reasoned-only paths by execution: - MCP webhook_list: a call_tool("webhook_list", ...) dispatch test against a mock node asserts the /hooks request carries a signature (get_signed). RED-proven: flipping the arm back to get() fails the header assertion. - gl->node end-to-end: a real RFC-9421 signature produced exactly as the gl client's get_signed does (gitlawb_core::http_sig::sign_request over GET + empty body) is run through the node's actual optional_signature middleware, which verifies it and authorizes the owner (200 + webhook body). RED-proven: signing over POST while sending GET makes the node reject it. This stitches the gl signing side and the node verifying side in one test rather than mocking one end. * fix(review): fail closed when the repo lookup errors in list_repo_events (#94) `get_repo(...).await.ok().flatten()` collapsed a lookup Err into None, so a transient DB failure skipped the read-visibility gate and the handler served a private repo's gossip ref-update events. Propagate with `?` instead: an error now maps to AppError::Internal (500) and fails closed, while a genuine Ok(None) (repo not hosted locally) stays the intentional ungated pass-through. Add a regression test that forces a get_repo error (drop the column its SELECT reads) and asserts 500 with no secret in the body. The injection is self-verifying: it asserts the lookup succeeds pre-drop and errors post-drop, so the test can't pass vacuously if get_repo's projection changes later. * fix(node,gl): close residual gaps in repo-read gating (#113) Address jatmn's review feedback on the read-surface gate: - gl webhook: resolve owner from the signing identity, not the node DID. cmd_create/list/delete all routed through the local resolve_owner helper, which only ever returned the node root DID, so for a repo owned by the local identity the owner-gated /hooks route returned 404/403. Drop that helper and reuse repo::resolve_owner_repo_pair (accepts owner/repo, else derives from --dir/keypair). Fixed across all three subcommands so create-writes and list/delete-reads stay on the same owner. - gl webhook list: check the HTTP status before reading `webhooks`. A 401/403/404 body has no webhooks field and previously printed "No webhooks" and exited 0, hiding the rejection; now bails with the server message like cmd_create. - visibility::is_owner: delegate to api::did_matches so the read gate and require_repo_owner share one normalization. Fixes the under-match (bare owner stored, full did:key caller, as mirror rows) that denied the owner with the opaque 404, and the cross-method over-match. - authz_guard: pin both halves of list_webhooks (authorize_repo_read and require_repo_owner) so removing the existence-hiding gate fails the drift test. cargo fmt/clippy clean; 644 workspace tests pass. * test(node)+fix(gl): close events reader-coverage gap, align label-list signing Self-review follow-ups on the read-gate work: - list_repo_events: add list_repo_events_admits_listed_reader, pinning the allow-list branch of the events gate (a listed non-owner reader gets 200 with the cert; a non-reader stranger 404s with no leak). Mirrors the read_visibility_admits_listed_reader coverage the sibling replica and protected-branch surfaces already had; the events handler was the only gated read surface missing it. - gl repo: cmd_label_list now loads an identity and uses get_maybe_signed instead of a bare anonymous get(), matching cmd_replicas/cmd_info. Public repos stay anonymously listable; a private-repo owner can read their own labels once /labels is read-visibility-gated, with no silent client drift. fmt/clippy clean; 645 workspace tests pass. * fix(review): pin the label-list signing and the events 404 sha-no-leak Final-review hardening of the prior two follow-ups: - gl repo label-list tests: require the signature/signature-input headers on the signed-branch mocks, and add an anonymous (no-identity) test asserting the header is absent. Without the header assertion the tests passed even with a bare get(), so the signing change was not actually pinned (RED-verified: reverting to get() now fails both signed-branch tests). - list_repo_events_admits_listed_reader: also assert the cert sha (not just the ref name) is absent from the stranger's 404 body, matching the list_repo_events_is_read_visibility_gated leak-check convention. fmt/clippy clean; 646 workspace tests pass. * fix(gl): error on 2xx webhook-list body missing the webhooks array (#113) A 2xx /hooks response without a `webhooks` array could hide a node/API contract regression. cmd_list now errors with context instead of mapping it to "No webhooks". An empty-but-present array stays a valid no-webhooks result. Adds a test for the missing-array case. * fix(gl,node): scope MCP webhook_list owner to keypair, gate list_labels on read visibility (#113) - MCP webhook_list: default owner to the signing keypair's short DID (explicit owner arg still overrides) instead of the node root DID, so the signed, owner-gated /hooks request resolves to the caller's own repo. Mirrors the gl webhook list fix. resolve_owner and webhook_create/delete left untouched. - list_labels: gate on authorize_repo_read so a private repo's label names 404 for non-readers (public repos stay anonymously listable), matching the sibling read surfaces and making the existing client comment true. Added the authz_guard marker and a read-visibility integration test. - gl label list: check HTTP status before reading the body so the now-gated 404 surfaces as an error instead of printing "No labels" and exiting 0, matching the sibling list clients (replicas, protected branches). * fix(gl): surface denied repo info reads instead of fabricating a repo (#113) cmd_info parsed the GET /api/v1/repos/{owner}/{name} body without checking the HTTP status, so a private-repo JSON 404 was rendered as a fake repo summary (owner / ? / public / main) and exited 0. Capture the status and bail on non-2xx, matching the sibling list commands. Also assert the anonymous 404 label-list body does not leak the label name, closing the last unread-body branch in the read-visibility gate test. * test(node): drop #113-gated handlers from KNOWN_UNGATED; sync lockfile to 0.4.0 The repo-scoped egress guard's allowlist (added in #122, merged after this branch was opened) still listed the read surfaces this branch gates. Now that list_labels, list_repo_events, list_webhooks, list_replicas, and list_protected_branches are read-visibility gated, the staleness assert requires removing them from the allowlist. The #120 surfaces (certs, issues, bounties, stars) stay listed; git_info_refs stays until #119 lands. Lockfile versions follow the 0.4.0 release the rebase landed on. * fix(gl): fail the MCP webhook_list tool on non-2xx node responses The MCP webhook_list path deserialized the response body and returned it as a successful tool result without checking the HTTP status, so a 401/403/404 JSON error (missing identity, wrong owner, private or deleted repo) was reported as success with the error body as the result. Check status().is_success() before deserializing and surface the node's message as a tool error, matching the `gl webhook list` path. Covered by a non-2xx test (RED without the check). * fix(gl): read the protected_branches key in repo owner gl repo owner parsed the protected-branch response as a bare array or a "branches" key, but the node returns {"protected_branches":[...],"count":N}, so it always printed "Protected: (none)" even when branches were protected. Extract the parse into parse_protected_branches matching the gl protect list shape, and update the owner tests to mock the real schema instead of bare arrays (which is why the bug went uncaught). * test(gl): cover the deny/negative arms reconciled by the merge The merge changed three gl branches that no existing test executed (they were only reasoned): resolve_owner_did with no local identity, cmd_info on an authenticated-but-denied 403, and get_maybe_signed with no keypair. Add a test per branch asserting the must-not case (error/unsigned, never a fabricated repo card). Verified load-bearing by mutation: disabling the cmd_info status gate turns the 403 test red. --------- Co-authored-by: t <t@t> |
||
|
github-actions[bot]
|
3ec8cbf1e1 |
chore(main): release 0.5.1 (#158)
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> |
||
|
GravireiandGravirei
|
6b5e5bc7ae |
fix(node): bound list_ref_certificates with LIMIT and add upsert to prevent unbounded growth (#147) (#149)
* fix(node): bound list_ref_certificates with LIMIT and add upsert to prevent unbounded growth (#147) * test: add INV-7 v10 upgrade-path dedup test * fix INV-7 test: correct id-DESC tiebreaker ids and fix fmt * fix: insert_ref_certificate returns persisted id via RETURNING; issue_ref_certificate uses it * fix: guard cert upsert on issued_at so older certs cannot regress newer ones * fix: .max(0) in events limit parsing, re-add lost cert tests after rebase * address reviewer findings: return full persisted row, add monotonic clock comment, note test drift risk * fix: add ?prefix= server-side cert lookup for CLI short-ID resolution --------- Co-authored-by: Gravirei <[email protected]> |
||
|
|
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]> |
||
|
|
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]> |
||
|
GravireiandGravirei
|
c5e5099def |
refactor(visibility): route is_owner through the shared did_matches matcher (#153) (#156)
* fix(node): gate GET /ipfs/{cid} on reachable allowed-set, not deny-set (#126)
The IPFS visibility gate used withheld_blob_oids (a deny-set enumerating
only reachable blobs), so a dangling/unreachable blob was absent from the
set and served in cleartext to anonymous callers. Flip to an allowed-set
(allowed_blob_set_for_caller) that enumerates reachable blobs the caller
may read: a dangling blob has no path, is never in the set, and 404s.
* perf(ipfs): check object existence before allowed-blob walk
Move store::read_object before the allowed_blob_set_for_caller
spawn_blocking call so random-CID spray against repos with
path-scoped rules cannot trigger full-history git walks on
repos that don't carry the object.
* refactor(ipfs): improve formatting and readability in get_by_cid function
✓ P3 blocker fixed: cargo fmt applied — the format gate will pass.
✓ P3 cleanup resolved: withheld_blob_oids is still used by replication code in repos.rs, so it stays.
• P2 follow-up: Tree/commit disclosure tracked in #135 — out of scope here.
* refactor(ipfs): streamline object retrieval by separating type and content reading
* docs(ipfs): update get_by_cid comment to reflect split object retrieval
* Run cargo fmt on store.rs
* refactor(visibility): route is_owner through the shared did_matches matcher (#153)
* style(profiles): remove redundant borrow in format arg
---------
Co-authored-by: Gravirei <[email protected]>
|
||
|
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
|
||
|
GravireiandGravirei
|
6bafaa6dc5 |
fix(bounties): add tests for claim_bounty repo-read gate (#160) (#169)
* fix(node): gate GET /ipfs/{cid} on reachable allowed-set, not deny-set (#126)
The IPFS visibility gate used withheld_blob_oids (a deny-set enumerating
only reachable blobs), so a dangling/unreachable blob was absent from the
set and served in cleartext to anonymous callers. Flip to an allowed-set
(allowed_blob_set_for_caller) that enumerates reachable blobs the caller
may read: a dangling blob has no path, is never in the set, and 404s.
* perf(ipfs): check object existence before allowed-blob walk
Move store::read_object before the allowed_blob_set_for_caller
spawn_blocking call so random-CID spray against repos with
path-scoped rules cannot trigger full-history git walks on
repos that don't carry the object.
* refactor(ipfs): improve formatting and readability in get_by_cid function
✓ P3 blocker fixed: cargo fmt applied — the format gate will pass.
✓ P3 cleanup resolved: withheld_blob_oids is still used by replication code in repos.rs, so it stays.
• P2 follow-up: Tree/commit disclosure tracked in #135 — out of scope here.
* refactor(ipfs): streamline object retrieval by separating type and content reading
* docs(ipfs): update get_by_cid comment to reflect split object retrieval
* Run cargo fmt on store.rs
* fix(bounties): add tests for claim_bounty repo-read gate (#160)
* fix(profiles): remove redundant borrow in format arg
---------
Co-authored-by: Gravirei <[email protected]>
|
||
|
GravireiandGravirei
|
26bc3f6987 |
fix(visibility): gate repo-scoped read surfaces on visibility (#120) (#157)
* fix(node): gate GET /ipfs/{cid} on reachable allowed-set, not deny-set (#126)
The IPFS visibility gate used withheld_blob_oids (a deny-set enumerating
only reachable blobs), so a dangling/unreachable blob was absent from the
set and served in cleartext to anonymous callers. Flip to an allowed-set
(allowed_blob_set_for_caller) that enumerates reachable blobs the caller
may read: a dangling blob has no path, is never in the set, and 404s.
* perf(ipfs): check object existence before allowed-blob walk
Move store::read_object before the allowed_blob_set_for_caller
spawn_blocking call so random-CID spray against repos with
path-scoped rules cannot trigger full-history git walks on
repos that don't carry the object.
* refactor(ipfs): improve formatting and readability in get_by_cid function
✓ P3 blocker fixed: cargo fmt applied — the format gate will pass.
✓ P3 cleanup resolved: withheld_blob_oids is still used by replication code in repos.rs, so it stays.
• P2 follow-up: Tree/commit disclosure tracked in #135 — out of scope here.
* refactor(ipfs): streamline object retrieval by separating type and content reading
* docs(ipfs): update get_by_cid comment to reflect split object retrieval
* Run cargo fmt on store.rs
* fix(visibility): gate repo-scoped read surfaces on visibility (#120)
* fix(visibility): enhance repo read authorization checks and improve test coverage
* Fix review feedback: clamp limit, memoize authz, identical 404 body, resolved issue id in comments
* cargo update -p crossbeam-epoch: fix RUSTSEC-2026-0204 (invalid pointer dereference)
* Fix review findings: correct authz memoization, page past hidden bounties, gate star/unstar
* Fix findings: sign CLI/MCP reads, keyset pagination, gate claim_bounty
---------
Co-authored-by: Gravirei <[email protected]>
|
||
|
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> |
||
|
github-actions[bot]
|
5f63f28215 |
chore(main): release 0.5.0 (#142)
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> |