fix(node): prefer canonical repo row over mirror row in get_repo (#124)#141
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates repo owner normalization and Changesget_repo canonical vs mirror row selection
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
beardthelion
added
crate:node
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The get_repo fix is correct. position('/' in id) ranks the canonical UUID row ahead of the slash-form mirror row, it matches the existing DEDUP_CTE ranking, and it makes the fetch_optional result deterministic instead of insertion-order-dependent. I built the change in isolation on top of main and ran it: the full node suite is green (317), clippy and MSRV 1.91 are clean, and the fix is load-bearing at the read gate. Without it, an anonymous read of a private repo resolves through the stale-public mirror row. Two things to sort before merge, plus a formatting nit.
Findings
-
[P1] Land #124 on its own; drop the #126 commits from this branch.
bur_fix_3carries all six commits from #133 (the #126 allowed-set flip) on top of the oneget_repocommit, so this PR's diff againstmainre-includes work that #133 is still reviewing, and that PR has an open changes-requested. Merging as-is would land #126 through the back door and skip its review. Rebase so this PR contains onlyfix(node): prefer canonical repo row over mirror row. It cherry-picks ontomaincleanly and has no dependency on #126. -
[P2] Make the regression test catch the bug it is guarding.
crates/gitlawb-node/src/db/mod.rs:3709The test seeds the canonical row first and the mirror second, so an unorderedfetch_optionalreturns the canonical row by insertion order. It passes with theORDER BYremoved, so it does not actually lock in the fix. Seed the mirror row before the canonical, which fails without the fix and passes with it, or add a caller-level case that drivesauthorize_repo_readand asserts an anonymous read of the private repo is denied. -
[P3] Fix the rustfmt violation.
crates/gitlawb-node/src/db/mod.rs:3752The trailingassert!(!got.is_public, ...)needs wrapping;cargo fmt --checkfails on it. Fork PRs don't run the fmt job, so CI here won't flag it.
…lawb#124) get_repo matched both mirror rows (bare short-DID, is_public=true, no visibility rules) and canonical rows (full did:key: DID, correct rules), returning one nondeterministically. When the mirror row won, the visibility gate saw is_public=true + empty rules, allowing unauthenticated read of private/withheld content. Add ORDER BY to prefer canonical rows (UUID id, no slash) over mirror rows (slash-form id), matching the ranking logic already used by DEDUP_CTE and dedupe_canonical_repos. Test seeds mirror FIRST so an unordered fetch_optional returns the mirror row — proving the test locks in the fix.
github-actions
Bot
added
the
needs-tests
|
Thanks for the contribution. A couple of things will help us review this faster:
See CONTRIBUTING.md. Update the PR and these notes will clear automatically. |
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The get_repo fix is correct and I verified it closes #124 through the read gate: the CASE WHEN position('/' in id) > 0 term dominates the sort, so the canonical row (hex UUID id) always outranks the mirror row ({short}/{name}, always slash-form), including the case where the mirror row was written before the canonical one and created_at ASC alone would pick the mirror. Rebased cleanly to one commit off main, the earlier fmt nit is resolved, and the db suite is green on the merged state. The listing path already ranks canonical-first with the identical CASE, so the two resolvers now agree.
Findings
- [P2] Make the regression test guard the
CASEterm, not just the presence of anORDER BY
crates/gitlawb-node/src/db/mod.rs:3729
The test locks in less than it looks. The canonical row is dated2026-01-01whileupsert_mirror_repostamps the mirror withUtc::now(), so thecreated_at ASCtiebreaker alone selects the canonical row: remove theCASEterm (keepingcreated_at ASC, id ASC) and the test still passes, even though theCASEis what actually fixes #124. A later "created_at already orders these" simplification would drop it and silently reopen the bypass. Date the canonical row after the mirror instead (e.g.ts("2126-01-01T00:00:00Z")on itscreated_at/updated_at), reproducing the mirror-written-first ordering; with that change, dropping theCASEfails the test and the full fix passes.
Optional, non-blocking: a two-canonical-row case would pin the created_at ASC, id ASC tiebreak, which is deterministic but currently untested.
@kevincodex1 — one P2 on the test; the fix itself is correct and closes the bypass.
beardthelion
dismissed stale reviews from themself
Stale: addressed on e7f1ee1; re-reviewing current head.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed on e7f1ee1. The regression test now guards the CASE term as it should. I ran it both ways on the merged state: green as-is, and dropping the CASE WHEN position('/' in id) term (leaving created_at ASC, id ASC) fails it with the mirror row winning over the canonical row, which is the exact #124 bypass. Dating the canonical row to 2126 is what makes created_at ASC alone select the mirror, so the CASE is load-bearing now. The fix itself is unchanged and correct, CI is green.
@kevincodex1 LGTM.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found an issue that needs to be addressed before this is ready.
Findings
- [P2] Keep
get_repomatching did:key aliases only
crates/gitlawb-node/src/db/mod.rs:894
The new ordering makesget_repo("z6...", name)deterministically prefer any matching non-slash row, but the predicate still matches every owner DID that contains:<bare>viaowner_did LIKE '%:' || $1 || '%'. That is broader than the did:key-only normalization used bydid_matches,DEDUP_CTE, and the existing distinct-DID-method tests:did:key:z6...anddid:gitlawb:z6.../did:web:z6...must stay separate owners even when they share the same trailing id and repo name. With this patch, a bare mirror lookup can rank a non-key DID row ahead of the exact slash-form mirror row and return the wrong repo/visibility rules. Please tightenget_repoto the same did:key-aware owner key used by the dedup paths, or otherwise restrict the canonical preference so it only applies to the matchingdid:key:<bare>row.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/gitlawb-node/src/db/mod.rs (1)
895-898: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOptional: extract the
did:key:normalization into a shared helper.This exact
match owner_did.strip_prefix("did:key:") { Some(rest) if !rest.contains(':') => rest, _ => owner_did }block is duplicated inlist_all_repos_deduped_with_stars(Line 1034-1037). Since both must stay in lock-step with the SQL CASE for the security invariant to hold, a singlefn normalize_owner_key(did: &str) -> &strreduces the drift risk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/db/mod.rs` around lines 895 - 898, Refactor the duplicated did:key normalization logic into a shared helper to keep both call sites in sync with the SQL CASE invariant. Add a small fn normalize_owner_key(did: &str) -> &str and use it from the owner_key handling in both list_all_repos_deduped_with_stars and the other matching block so the strip_prefix("did:key:") / contains(':') behavior lives in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 895-898: Refactor the duplicated did:key normalization logic into
a shared helper to keep both call sites in sync with the SQL CASE invariant. Add
a small fn normalize_owner_key(did: &str) -> &str and use it from the owner_key
handling in both list_all_repos_deduped_with_stars and the other matching block
so the strip_prefix("did:key:") / contains(':') behavior lives in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47bffae1-920f-4fda-b20e-76494f6faae5
📒 Files selected for processing (1)
crates/gitlawb-node/src/db/mod.rs
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I found one remaining issue that needs to be addressed before this is ready.
Findings
- [P2] Keep emitted clone URLs aligned with the tightened owner matching
crates/gitlawb-node/src/api/repos.rs:1635
get_reponow intentionally only treats a bare owner as equivalent todid:key:<owner>; non-key DIDs such asdid:gitlawb:z6...anddid:web:...remain distinct. Howeverto_responsestill buildsclone_urlby taking the last:segment for every owner DID, so a canonical non-key repo response advertises/<last-segment>/<repo>.git, and that URL now resolves throughget_repo("<last-segment>", repo)to no canonical row (or to a mirror if one exists) instead of the repo that was just returned. Please update the URL/slug construction to use the same did:key-only shortening rule, or otherwise keep the advertised clone URL resolvable under the new matcher.
There was a problem hiding this comment.
The get_repo tightening verifies correct on 7f92302a and your earlier did:key-only [P2] is resolved. Two to fold in before merge.
Findings
-
[P2] Add a read-gate test that proves the #124 leak is closed, not just row selection
crates/gitlawb-node/src/db/mod.rs(new tests ~3720)
The three new tests assertget_reporeturns the canonical row, but nothing drivesauthorize_repo_read, where the leak lived. Seed a private canonical plus a public mirror twin for the same owner+name (mirror inserted first), callauthorize_repo_readwithcaller=None, and assertErr(RepoNotFound). That locks the property at the gate, so a future regression is caught by the denial rather than by row order alone. While you're there, oneget_repocall with the fulldid:key:form (not just the bare id) covers the normalize branch the current tests skip. -
[P2] The clone_url slug from jatmn's note hits the fork check too, same fix
crates/gitlawb-node/src/api/repos.rs:1635and:1477
Confirmed jatmn's point:to_responsederives the slug fromowner_did.split(':').next_back(), which stops round-tripping through the tightenedget_repofor a non-key DID owner. The same last-segment slug feeds the fork-name-conflict check atrepos.rs:1477(forker_shortthenget_repo), so shorten both consumers with the same did:key-only rule (makenormalize_owner_keypub(crate)and reuse it). For calibration: no reachable path plants a non-key DID on a canonical row today (auth is did:key-only viato_verifying_key, and the two insert paths are create/fork withauth.0and the always-slash mirror upsert), so this is forward-consistency for when did:web/did:gitlawb ownership lands, not a live break. Cheap to align now.
@Gravirei once the gate test and the slug alignment are in, I'll re-review. Nice work tightening the matcher.
…ey matching - Make normalize_owner_key pub(crate) and reuse it in api/repos, api/events, api/peers, and visibility modules. - Add read-gate authorize_repo_read and to_response clone_url slug tests.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the previously discussed paths and do not see any remaining actionable issues from my side.
@kevincodex1 LGTM
beardthelion
dismissed
their stale review
Superseded: re-reviewed 084ca04 — both prior P2s (gate test, clone-URL slug) are resolved. Posting a fresh review with the remaining items.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed on 084ca046, and I verified the core by execution: the #124 gate test is load-bearing (on the head anon gets Err(RepoNotFound); reverting only the canonical-preference ORDER BY flips it to serving the is_public:true mirror for the private repo), normalize_owner_key matches the SQL CASE across the boundary set (did:key short/full, empty residual, did:key:z:extra, non-key, bare, empty, uppercase), and the is_owner change is fail-closed for reachable data. Both my earlier P2s and jatmn's clone-URL P2 are resolved. The fix is correct; a few things to fold in before merge, the first being the one that matters.
Findings
-
[P2] Guard the Rust/SQL owner-key normalization against drift
crates/gitlawb-node/src/db/mod.rs:829
normalize_owner_keyand the roughly nine verbatim SQLCASE WHEN owner_did LIKE 'did:key:%' ...copies (the get_repo WHERE at:907, the DEDUP_CTE, the count) are byte-identity-coupled with only a comment enforcing it, and the helper has no unit tests. They agree today (I checked the full boundary set), but a future edit to one side diverges silently: the clone-URL round-trip breaks, or the matcher mismatches and reopens a leak. Add unit tests over that boundary set, ideally asserting the Rust output equals a SQL round-trip, and consider hoisting the CASE into a sharedconstso a mismatch becomes a compile-time impossibility. -
[P3] Add an is_owner regression test for the non-key tightening
crates/gitlawb-node/src/visibility.rs:29
is_ownernow returns false when a non-key owner (did:gitlawb:z6Mkfoo) is probed with a bare last segment (z6Mkfoo), where the old.split(':').next_back()returned true. That behavior change has no test;root_rule_allows_owner_short_formonly covers the did:key path and passes against both old and new code. Add a non-key-owner case asserting the bare segment no longer matches. -
[P3] Add LIMIT 1 to the get_repo query
crates/gitlawb-node/src/db/mod.rs:913
The query can now match both a canonical and a mirror row, so theORDER BYbuilds a two-row set thatfetch_optionalthen trims.LIMIT 1makes the single-row intent explicit and skips the needless sort-and-discard. -
[P3] Fix the is_owner doc comment; keep normalize_owner_key
crates/gitlawb-node/src/visibility.rs:26
The comment saysis_ownermirrors the owner-match idiom inapi/protect.rs, butprotect.rsusesdid_matcheswhileis_ownernow uses the stricternormalize_owner_key. Update the comment to reflect the intentional did:key-only rule. Keepnormalize_owner_keyhere: switching todid_matcheswould reintroduce the cross-method looseness this PR removes. -
[P3] Document the non-key clone_url disk-path constraint
crates/gitlawb-node/src/api/repos.rs:1627
For a non-key DID owner,normalize_owner_keyreturns the full DID, soclone_urlbecomes/did:web:host:alice/repo.git; that resolves throughget_repo, but the colon-bearing path segment would break thesync.rsdisk-path join. Not reachable today (auth is did:key-only), so this is a forward constraint to document/handle before non-key ownership lands, not a live break.
Out of scope for this PR, tracking separately (no action needed here): api/agents.rs caller_matches_did still uses the old last-segment matcher; it's the one owner-match site the centralization didn't reach, and it isn't exploitable (deregister only revokes the caller's own DID).
@Gravirei nice work, the #124 fix is correct and verified. Once the drift guard is in (and the P3s while you're there), I'll re-review.
… review feedback - Hoist 7 verbatim SQL CASE copies into OWNER_KEY_CASE_SQL const (dedup_cte(), get_repo, count_repos_deduped), keeping one literal copy in the v7 migration with a cross-reference comment. - Add 9 unit tests for normalize_owner_key over the full boundary set. - Add LIMIT 1 to get_repo query for single-row intent. - Add non_key_owner_bare_short_does_not_match is_owner regression test. - Update is_owner doc to reflect did:key-only rule. - Document non-key clone_url disk-path constraint in to_response.
github-actions
Bot
removed
the
needs-tests
…KEY_CASE_SQL Verifies every boundary value produces identical results from the Rust function and the SQL CASE expression, preventing silent drift.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I do not see any actionable issues from my review.
@kevincodex1 LGTM
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed on cc0722e9. Every item from my last pass is resolved, and I verified the changed branches load-bearing by execution, both ways:
- #124 gate: reverting only the canonical-preference
ORDER BYterm flipsauthorize_repo_readto serve the public mirror row for the private canonical to an anonymous caller (Ok(is_public: true)instead ofErr(RepoNotFound)); with the term in place it denies. - did:key-only
is_owner:non_key_owner_bare_short_does_not_matchpasses as written and fails if I restore the oldsplit(':').next_back()(bare short wrongly matches the non-key owner), so it's load-bearing. - Drift guard:
normalize_owner_key_matches_sql_casepasses and fails if the Rust helper or the SQL const diverge; the hoist renders byte-identical for the deduped and count queries.
The P2 drift guard and the four P3s are all in.
- [P3] Cover the both-rows state end to end. The reachable way one node ends up with both a canonical and a mirror row for the same owner/name is
trigger_syncre-mirroring its own repo, but the tests only seed that state directly. Atrigger_sync -> upsert_mirrortest that then asserts the gate still denies would lock the path the fix exists for.
Non-blocking latent (safe while auth is did:key-only): the frozen idx_repos_owner_key_name migration keeps a verbatim CASE copy (index-usage only), and the owner-side-only is_owner normalization plus the agents.rs/profile last-segment matchers are worth revisiting when non-key DIDs land.
@kevincodex1 this is ready. LGTM.
The rebase onto main adopts #141's normalize_owner_key on the repo-scoped ref-update read path, which keeps a non-did:key DID intact instead of taking the last URL segment. Align the two did:web fixtures that still assumed the old {last-segment}/{name} wire slug: - repo_events_did_web_owner_reads_own_gossip stored "alice/widget", but the emit side (api/repos) publishes "did:web:example.com:alice/widget", so the read query no longer matched and the owner saw only the cert half. Store the emit-side slug so the gossip KEEP branch is exercised again. - repo_events_gossip_slug_collision_withheld_from_anon is removed. Its premise was that two did:web owners collide on the last-segment slug; #141 gives them distinct full-DID slugs, so the repo-scoped query fetches zero rows and the per-row drop is never reached (verified by probe: fetched=0). Global-feed collision coverage stays in feed_full_did_slug_dropped_for_anon and feed_truncated_key_slug_dropped_for_anon.
The rebase onto #141 changed the emit-side wire slug from the last-segment form to normalize_owner_key (bare short key for a single-segment did:key, full DID otherwise). Four comments still described the old behavior: - visibility.rs ref_update_row_names_repo: the emitter builds the slug via normalize_owner_key, not {last-segment}/{name}. The next_back reduction on the matcher is what tolerates the untrusted/crafted short form; it does not mirror an already-stripped emit form. - events.rs list_repo_events gossip half: the residual collision the per-row filter still guards is a did:key canonical owner vs its bare short-key mirror (both normalize to the same slug), not two did:web owners (#141 gives those distinct slugs). - the two did:web feed-visibility tests had their canonical/crafted framing backwards: post-#141 the full-DID slug is the emit form and the short slug is the crafted one. Comment-only; no behavior change.
A public bare-key mirror (z6MkX) and a private did:key canonical repo (did:key:z6MkX) normalize to the same owner key post-#141, so their gossip rows share the z6MkX/... slug space on the untrusted wire. Assert an anon global-feed read sees the public mirror's row but NOT the private canonical sibling's: the shared gate keys on the full slug (owner + name), so a readable public repo under an owner key does not unlock that owner's other private repos' rows. The removed repo-scoped did:web collision test never covered this pair. Verified load-bearing: disabling the per-row visibility drop serves z6MkX/secret to anon (RED); the gate drops it (GREEN).
…#143) * feat(node,visibility): add pure fail-closed feed-visibility filter (#112,#114) Add ref_update_row_visible: given the deduped local repo set + their visibility rules, decide whether a received_ref_updates row (keyed by its peer-supplied slug) is visible to the caller. Matches slug->repo by name plus prefix-tolerant owner key, and drops any row matching a local repo the caller cannot read at root; rows matching no local repo (remote/ gossip-only) and malformed slugs pass through. Fail-closed by construction (no default-keep). Pure/I-O-free so visibility.rs stays exhaustively unit-tested. Shared gate for the GraphQL (#112) and REST (#114) ref-updates feeds; call sites land in the following commits. Includes a DB-layer test pinning that a private canonical row beats a public mirror row in list_all_repos_deduped (else the feed gate would over-serve). * fix(node,graphql): gate refUpdates on read visibility (#112) The GraphQL ref_updates resolver mapped every received_ref_updates row straight out with no visibility gate, so an anonymous caller could read a private repo's ref metadata (refName, SHAs, pusherDid, nodeDid) by name. Thread the caller DID and retain rows through the shared ref_update_row_visible filter (deduped local set + rules, same '/' decision the repos resolver uses); applies to both the repo:Some and repo:None branches. Remote/gossip-only rows still pass. Removes the now-live filter's dead_code allow. Tested via the GraphQL schema: anon leak reproduced before the gate (RED) and closed after (GREEN); owner still sees their private row; full-DID and truncated-key slug aliases fail closed; remote rows kept. * fix(node,events): gate global ref-updates feed on read visibility (#114) GET /api/v1/events/ref-updates returned every received_ref_updates row across all repos, including private ones, to any anonymous caller (the REST analog of #112). Thread the caller DID via optional_signature and retain rows through the shared ref_update_row_visible filter, deriving count from the filtered set. Keeps the crate::error::Result return type; the deduped/rules loads fail closed (500) on a DB error rather than serving unfiltered. Remote/gossip-only rows still pass. Tested via the crate's HTTP-API harness: anon leak reproduced before the gate (RED) and closed after (GREEN); owner still sees their private row; mixed feed yields only public rows with a filtered count; full-DID and truncated-key slug aliases fail closed; remote rows kept. * fix(node,graphql): gate the ref-update subscription broadcast on announce (#112,#114) Code review surfaced a third reader of the same ref metadata the query and REST feeds gate: the GraphQL subscription. /graphql/ws is mounted after the optional_signature layer (no caller to gate against), and the push handler broadcast every ref update to it unconditionally — outside the if-announce block that already gates the sibling gossip and Arweave sends. So an anonymous websocket subscriber received live ref metadata (repo, refs, SHAs, pusher/node DID) for private repos as they were pushed — the subscription analog of the #112/#114 leak. Move the ref_update_tx broadcast inside the existing if-announce guard so only publicly-readable pushes reach the unauthenticated subscription, matching gossip and Arweave. Pin the gate decision with a replication_withheld_set test (announce false for private, true for public). Also harden the feed filter's owner-key normalization: normalize the slug owner to the last ':'-segment (same rule as record_key) instead of stripping only did:key:, so a multi-segment DID (did:web) and its full-DID slug match and fail closed rather than over-serving. Regression test added (RED under the old normalization). * docs(node,graphql): pin the subscription write-side gate invariant; add did:web keep test Code review flagged that the unauthenticated /graphql/ws ref_updates subscription now depends on a single-point invariant: safety rests entirely on every sender to ref_update_tx being announce-gated, since the resolver has no caller to gate against. Document that on the resolver so a future sender can't silently reopen the leak. Add the symmetric keep-side test for the did:web owner-key normalization (public multi-segment-DID repo row is kept for anon), so a regression that over-drops legitimate did:web rows is caught alongside the existing drop-side test. * docs(node,visibility): correct ref-update slug normalization rationale (#112,#114) The comment claimed parity with did_matches/DEDUP_CTE, but the last-segment normalization deliberately diverges: the emitter (publish_ref_update) broadcasts the short, method-stripped slug, so a repo's own rows arrive keyed by the trailing segment and the gate must match on it to stay fail-closed. Reusing the did:key-aware keep-whole rule would fail open on a did:web repo's short slug. Add feed_private_didweb_short_slug_dropped_for_anon to pin that load-bearing canonical case; relabel the full-DID test as the non-canonical hand-forged form. * fix(node,events): gate ref-update feeds before the limit via paging (#114) The REST global feed and the GraphQL ref_updates resolver loaded `limit` rows and dropped private ones in memory, so an anonymous request could get fewer than `limit` events (or zero) when the newest rows named private local repos, even though older public ref updates existed. Add a shared collect_visible_ref_updates helper that applies the fail-closed gate before the limit: it over-fetches in bounded pages (128 rows, capped at max(limit, 2048) scanned) until `limit` visible rows are collected or the table is exhausted. Both feeds route through it so the one gate cannot drift. Back it with list_ref_updates_page (stable timestamp,id ordering for offset paging) and drop the now-unused list_ref_updates / list_ref_updates_filtered. The page size is a parameter on an inner fn so a regression can exercise multi-page paging. Regressions: newest-private, older-public, small limit still returns the latest visible rows on both surfaces (RED before, GREEN after); and a page=2 test pins offset paging across page boundaries (no skip or duplicate). * docs(node,visibility): correct slug-builder attribution in ref-update gate comment The comment credited publish_ref_update with building the wire slug, but that p2p method only broadcasts a pre-built RefUpdateEvent; git_receive_pack assembles the {last-segment}/{name} slug and hands it over. Fix the attribution. * fix(node,events): bound feed limit and withhold quarantined mirrors (#114) Resolve two CodeRabbit findings on the ref-update feeds: - Clamp the effective limit inside the shared collector (max 200). REST already capped, but the GraphQL resolver passed its caller-provided limit uncapped, letting a large request return unbounded rows and scan unbounded DB rows. Both surfaces are now bounded identically. - Fold quarantined mirrors into the gate's match universe as unreadable. list_all_repos_deduped excludes quarantined rows, so a ref-update naming a quarantined local mirror matched nothing, was misclassified as remote, and was served to anon, bypassing the withheld-from-every-listing-surface contract. Add list_quarantined_repos and drop matching rows fail-closed. Regressions: oversized-limit clamp (201 rows -> 200) and quarantined-mirror withheld-from-anon, both RED before, GREEN after. * fix(node,events): gate the repo-scoped events feed on read visibility (#112, #114) GET /api/v1/repos/{owner}/{repo}/events (list_repo_events) served a repo's ref certificates and received gossip ref-updates with no caller identity and no visibility check, so an anonymous caller who guessed a private repo's owner/name read its ref metadata. It was the last repo-scoped reader in the #112/#114 ref-metadata leak class. Gate it in two layers, using the caller DID the optional_signature layer already supplies: - The cert half is gated by a repo-root authorize_repo_read (keyed by the unique repo record id): a denied, quarantined, or not-hosted repo returns an opaque 404 (RepoNotFound), never a 403. - The gossip half is filtered per row through the shared collect_visible_ref_updates. received_ref_updates rows are keyed by the lossy, non-unique wire slug {last-segment}/{name}, so two owners (did:web:a:alice, did:web:b:alice) collide on `alice/name` and a plain slug query would serve a colliding private repo's rows to anyone allowed to read this one; the per-row gate drops them fail-closed. Both halves now propagate DB errors as 500 instead of swallowing them into an empty 200. Removes the now-unused list_repo_ref_updates. Deliberate behavior change: a repo this node does not host returns 404 (it holds no visibility record for it and fails closed; remote gossip is read via the global /api/v1/events/ref-updates feed). Drops list_repo_events from the egress-guard KNOWN_UNGATED allowlist; the source-scrape staleness assert enforces the removal. Tests cover anon-private, owner-private (both datasets), anon/authed-public, quarantined, released-from-quarantine, non-owner, not-hosted, did:web (owner and anon), the slug collision, oversized-limit clamping, and DB-error-fails-closed on both the gate and the cert fetch. * fix(node,events): clamp negative ref-update feed limits to zero (#114) A caller-supplied `?limit=-1` cleared the `.min(MAX_VISIBLE_REF_UPDATES)` upper cap unchanged and reached `all_events.truncate(limit as usize)` in list_repo_events, where `-1 as usize` is usize::MAX, so the truncate was a no-op and the handler returned the full unbounded local ref-cert set. Floor both feed handlers' limit parse with `.clamp(0, MAX_VISIBLE_REF_UPDATES)`. Only the repo handler's clamp is load-bearing: it guards the local `truncate`, which does not pass through the shared collector. The global feed's clamp is a consistency measure (its limit is re-clamped in the collector, whose want==0 short-circuits before any scan), kept in lockstep so the two per-handler caps the module comment calls out don't drift. Tests, both load-bearing by execution: - repo_events_negative_limit_clamped: seeds local certs (the vulnerable half), RED (3) before the clamp and green (0) after. - feed_negative_limit_returns_empty: seeds 5 visible rows; green (0) with the collector clamp, 5 without it, proving the collector is the global feed's guard. * fix(node,events): drop quarantined repos before the feed visibility gate (#114) The ref-update feeds folded quarantined mirrors into the deduped repo set with is_public=false and relied on the row visibility gate to withhold them. That held for anonymous callers but not for one matching the mirror's owner_did: visibility_check short-circuits to Allow for the owner before is_public is read, so an owner-matched quarantined row was served on both the REST global feed and the GraphQL ref_updates resolver. Drop any row that names a quarantined repo in the shared collector, before the visibility gate runs, so the drop bypasses the owner short-circuit. Quarantine is a status decided at admission and checked separately from the mirror's visibility fields, so it must deny every caller including the owner. Extract the slug match into one ref_update_row_names_repo predicate reused by both the gate and the quarantine drop, so the two cannot disagree about which rows a repo owns. Tests cover the owner case at the collector, REST handler, and GraphQL resolver in both bare-key and full-did:key owner forms, plus a must-not case proving the drop withholds only quarantined rows while still serving unrelated visible ones. * test(node,events): align did:web feed tests with the #141 canonical slug The rebase onto main adopts #141's normalize_owner_key on the repo-scoped ref-update read path, which keeps a non-did:key DID intact instead of taking the last URL segment. Align the two did:web fixtures that still assumed the old {last-segment}/{name} wire slug: - repo_events_did_web_owner_reads_own_gossip stored "alice/widget", but the emit side (api/repos) publishes "did:web:example.com:alice/widget", so the read query no longer matched and the owner saw only the cert half. Store the emit-side slug so the gossip KEEP branch is exercised again. - repo_events_gossip_slug_collision_withheld_from_anon is removed. Its premise was that two did:web owners collide on the last-segment slug; #141 gives them distinct full-DID slugs, so the repo-scoped query fetches zero rows and the per-row drop is never reached (verified by probe: fetched=0). Global-feed collision coverage stays in feed_full_did_slug_dropped_for_anon and feed_truncated_key_slug_dropped_for_anon. * docs(node): correct ref-update slug comments for the #141 canonical form The rebase onto #141 changed the emit-side wire slug from the last-segment form to normalize_owner_key (bare short key for a single-segment did:key, full DID otherwise). Four comments still described the old behavior: - visibility.rs ref_update_row_names_repo: the emitter builds the slug via normalize_owner_key, not {last-segment}/{name}. The next_back reduction on the matcher is what tolerates the untrusted/crafted short form; it does not mirror an already-stripped emit form. - events.rs list_repo_events gossip half: the residual collision the per-row filter still guards is a did:key canonical owner vs its bare short-key mirror (both normalize to the same slug), not two did:web owners (#141 gives those distinct slugs). - the two did:web feed-visibility tests had their canonical/crafted framing backwards: post-#141 the full-DID slug is the emit form and the short slug is the crafted one. Comment-only; no behavior change. * test(node,events): add load-bearing two-repo owner-key collision deny A public bare-key mirror (z6MkX) and a private did:key canonical repo (did:key:z6MkX) normalize to the same owner key post-#141, so their gossip rows share the z6MkX/... slug space on the untrusted wire. Assert an anon global-feed read sees the public mirror's row but NOT the private canonical sibling's: the shared gate keys on the full slug (owner + name), so a readable public repo under an owner key does not unlock that owner's other private repos' rows. The removed repo-scoped did:web collision test never covered this pair. Verified load-bearing: disabling the per-row visibility drop serves z6MkX/secret to anon (RED); the gate drops it (GREEN). * fix(node,events): keyset-paginate the ref-update feed collector collect_visible_ref_updates paged received_ref_updates with LIMIT/OFFSET, so a concurrent gossip/notify insert (which sorts to the front under timestamp DESC) shifted every later offset and let one feed response duplicate or skip a visible row. Switch the collector to a keyset cursor on (timestamp, id): each page reads rows strictly older than the last row fetched, so a newer insert lands above the window and cannot shift it. One collector feeds the REST global, REST per-repo, and GraphQL paths, so this covers all three; there is no client-facing cursor, so the internal cursor tracks the last fetched row. Adds tests that OFFSET duplicates a row under a concurrent insert and that keyset stays stable. --------- Co-authored-by: t <t@t>
Resolve conflicts where main's already-merged work overlaps this PR's read-surface gating: - api/events.rs: take main's list_repo_events. #143 re-gated it via the canonical authorize_repo_read (and tightened the gossip-only path to a 404), superseding this PR's earlier #94 visibility_check gate. - visibility.rs: keep main's normalize_owner_key is_owner from #124/#141. - api/mod.rs known_ungated: drop list_webhooks/list_replicas/ list_protected_branches now that this PR gates them. Reconcile the two stale #94 remnants adopting main's list_repo_events exposed: point the gate-marker registry row at authorize_repo_read, and drop the obsolete gossip-only-serves-200 test (main's api::events tests cover the current 404 behavior). Full gitlawb-node suite: 431 passing.
4 participants
Replicated mirror rows are stored with
is_public=trueand no visibility rules, andget_repocould resolve a request to the mirror row instead of the canonical one. An unauthenticated caller could read a private/withheld repo's refs and objects through the bare short-DID owner, bypassing the canonical row's visibility rules.Fix: Added
ORDER BY CASE WHEN position('/' in id) > 0 THEN 1 ELSE 0 END, created_at ASC, id ASCtoget_reposo canonical rows (UUID id, no slash) are always preferred over mirror rows (slash-form id). This matches the ranking logic already used byDEDUP_CTEanddedupe_canonical_repos.Closes #124
Summary by CodeRabbit
did:key:owner inputs are normalized and matched only under the correct conditions, avoiding unintended DID cross-matches.