fix(node): close quarantine bypass on encrypted blobs and CID serve - #276
fix(node): close quarantine bypass on encrypted blobs and CID serve#276Ayush7614 wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 12 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughRepository quarantine is enforced across encrypted-blob, visibility, repository enumeration, and IPFS CID endpoints. Shared authorization returns opaque 404 responses, repository listing excludes quarantined rows, and IPFS serving filters mirror twins of quarantined canonical repositories. ChangesQuarantine-aware repository access
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Caller
participant RepositoryRoute
participant authorize_repo_read
participant RepositoryDatabase
Caller->>RepositoryRoute: request repository resource
RepositoryRoute->>authorize_repo_read: authorize read
authorize_repo_read->>RepositoryDatabase: load repository and quarantine state
RepositoryDatabase-->>authorize_repo_read: record and rules
authorize_repo_read-->>RepositoryRoute: authorized record or opaque 404
RepositoryRoute-->>Caller: resource response
sequenceDiagram
participant Caller
participant get_by_cid
participant RepositoryDatabase
participant ObjectStore
Caller->>get_by_cid: request CID
get_by_cid->>RepositoryDatabase: fetch quarantined canonical repositories
RepositoryDatabase-->>get_by_cid: quarantine rows
get_by_cid->>RepositoryDatabase: inspect candidate repositories
get_by_cid->>ObjectStore: check object type
ObjectStore-->>get_by_cid: object result
get_by_cid-->>Caller: content or opaque 404
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
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
added
crate:node
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The two production changes are correct and both are load-bearing. I reverted each one and watched the matching test go red: dropping the WHERE quarantined = FALSE makes get_by_cid_skips_quarantined_public_repo serve the blob (200, public bytes), and restoring the old encrypted.rs gate makes the discovery route hand back the blob index. The ordering the fix depends on is right too, since authorize_repo_read checks quarantine at api/mod.rs:54 before visibility_check at :58, and I confirmed by driving the handler that a quarantined repo now 404s for its own owner in both the full did:key: and bare-key forms.
What holds the round is the route next door.
Findings
-
[P1] Gate
withheld-pathson quarantine as well
crates/gitlawb-node/src/api/visibility.rs:213
This handler is mounted in the same anonymously-reachable group as the three routes you fixed (server.rs:430-449) and still does the bareget_repoplusvisibility_checkthat the PR removed fromencrypted.rs. I drove it against a public-but-quarantined repo: it returns 200 with{"withheld":["/secret/**"],"reinclude":[]}whileencrypted-blobson that same repo correctly returns 404. So an anonymous caller still learns the repo is admitted here and learns its private-subtree layout, and because the owner short-circuit sits insidevisibility_check, the row's claimed owner gets the same answer. It also has a cross-node consequence: a peer'swithheld-pathsanswer picks the mirror mode insync.rs. Swapping the resolve forlet (record, rules) = crate::api::authorize_repo_read(&state, &owner, &repo, caller, "/").await?;is the same shape as yourencrypted.rschange; I applied exactly that and ran it, and the quarantined request became 404 while a released repo still returns its globs. -
[P2] Add an owner-caller regression in both DID forms
crates/gitlawb-node/src/test_support.rs:4233,:4290
Both new tests drive onlyanon_get/cid_anon. The code is right, and I verified that separately, but an anonymous pass cannot distinguish "dropped before the gate" from "denied by the gate", and those come apart the moment someone moves the quarantine check belowvisibility_checkor turns the SQL predicate into an in-loop skip after the visibility branch. The invariant this PR implements is that a quarantined repo is withheld from every reader including its owner, and the tree already has that shape for the feed collector infeed_quarantined_mirror_withheld_from_ownerandfeed_quarantined_full_did_repo_withheld_from_owner. Mirror those here: one owner-caller case per surface, in the fulldid:key:and bare-key forms. -
[P2] Gate
list_visibilityon quarantine
crates/gitlawb-node/src/api/visibility.rs:174
Same bareget_repofollowed byrequire_owner, no quarantine drop, so the caller matching the row'sowner_didgets 200 with every rule'spath_glob,mode,reader_dids, andcreated_by. On a quarantined mirror thatowner_didcame off the wire with the row, so "the owner" here is whoever asserted the DID. Narrower reach than the one above since it needs that DID, which is why it is not P1, but it is the same invariant and the same one-line fix. -
[P3] Extend the release control to all three encrypted routes
crates/gitlawb-node/src/test_support.rs:4272
The deny loop coversencrypted-blobs,encrypted-blobs/replicate, andencrypted-blob/{oid}, but the release control only re-requests the first. The other two have no must-not-over-drop side, so a change that 404s them even after release would still pass. Worth noting for the third leg specifically: I reordered the loop and confirmed that on revert it fails at 500 rather than 200, because the handler gets past the gate and then dies inipfs_pin::cat. It detects the regression, it just is not demonstrating the leak its name claims.
One process note, not a finding: the body ticks "CI test (stable) with Postgres", but the suite has not run on this head. The workflow is sitting at action_required awaiting approval, which is the normal shape for a fork PR. I approved it so it runs. I also ran the quarantine suite locally against your head and it is 16 green.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
What checks out
The two production changes in this diff are correct and load-bearing for the single physical row cases they target:
- Routing the three encrypted handlers through
authorize_repo_readcloses the quarantine bypass that used bareget_repo+visibility_check. I reverted this hunk locally and the new deny test goes red. - Adding
WHERE quarantined = FALSEtolist_all_reposcloses the CID serve bypass for a quarantined public row with no unquarantined sibling. I reverted this hunk locally andget_by_cid_skips_quarantined_public_reposerves the blob again.
CI on head bcc7fb4 is green (fmt + clippy, test (stable), MSRV, cargo audit, Docker smoke). None of the findings below are regressions this PR introduced; they are sibling gaps, an incomplete multi-row closure on the CID path, or test holes.
Findings
-
[P1] Gate
withheld-pathson quarantine (pre-existing sibling gap)
crates/gitlawb-node/src/api/visibility.rs:208
This handler is mounted beside the three routes you fixed (server.rs:430-449) and still uses the oldencrypted.rspattern: bareget_repoplusvisibility_check, with nois_repo_quarantined/authorize_repo_readshort-circuit. On a public-but-quarantined repo,visibility_checkallows"/"for anonymous callers when there is no matching deny rule, so the handler returns 200 withwithheld/reincludeinstead of the opaque 404 thatauthorize_repo_readwould return. That discloses both admission and private-subtree layout. The owner short-circuit insidevisibility_check(visibility.rs:79-83) means the row's claimed owner gets the same answer. Downstream,sync.rs:245-256andglclone'sfetch_withhelduse this response to choose promisor mirror mode and sparse-clone withhold sets, so the leak is not local to discovery. Please resolve throughauthorize_repo_readthe same wayencrypted.rsnow does (or an equivalent quarantine-first gate). -
[P1]
get_by_cidstill serves via an unquarantined mirror twin while slug routes 404 (incomplete CID fix / comment overclaim)
crates/gitlawb-node/src/api/ipfs.rs:81-116,crates/gitlawb-node/src/db/mod.rs:1234-1238
The newlist_all_reposfilter drops only rows withquarantined = TRUEon that physical row. When a node holds both a quarantined canonical row (UUIDid) and a non-quarantined mirror row ({owner}/{name}) for the same logical repo — a shape the tree explicitly supports (get_repo_prefers_canonical_over_mirror,authorize_repo_read_denies_private_canonical_even_with_public_mirror; mirror upsert setsquarantined = falsewheneverget_repoalready finds a canonical row atsync.rs:290-309) — slug endpoints 404 throughauthorize_repo_readbecauseget_repoprefers the quarantined canonical, butget_by_cidstill iterates the surviving mirror row, passesvisibility_check(mirror is hardcoded public with no rules), and serves the object. That contradicts the comment claiming the same fail-closed posture asauthorize_repo_read(ipfs.rs:43-44). The feed collector already guards this class by consultinglist_quarantined_reposand dropping any row naming a quarantined logical repo (events.rs:66-103);get_by_cidneeds the same logical-repo quarantine fold before serving. This is not the separately tracked stale-public-mirror bypass (#124, explicitly deferred atipfs.rs:55-57). -
[P2] Gate
list_visibilityon quarantine (pre-existing sibling gap)
crates/gitlawb-node/src/api/visibility.rs:174
This owner-authenticated route still doesget_repo+require_ownerwith no quarantine drop. A caller matching the row'sowner_didgets 200 with every rule'spath_glob,mode,reader_dids, andcreated_byeven while the mirror is quarantined. Narrower reach thanwithheld-pathsbecause it needs that DID, but it violates the same "withheld from every reader" invariant stated inevents.rs:58-65. Same one-line shape as theencrypted.rsfix. -
[P2] Add owner-caller regression tests for the new surfaces (test gap)
crates/gitlawb-node/src/test_support.rs:4233,:4290
Both new HTTP tests drive only anonymous callers (anon_get/cid_anon). That cannot distinguish "quarantine checked before visibility" from "denied later by visibility", and those diverge if someone moves the quarantine check belowvisibility_checkor relies only on the SQL filter inlist_all_repos. The tree already has owner-withheld coverage for feeds (feed_quarantined_mirror_withheld_from_owner,feed_quarantined_full_did_repo_withheld_from_owner). Please mirror that here: one owner-caller case per surface (encrypted discovery and CID serve), in both bare-key and fulldid:key:owner forms. -
[P3] Extend the encrypted release control to all three routes (test gap)
crates/gitlawb-node/src/test_support.rs:4272
The deny loop coversencrypted-blobs,encrypted-blobs/replicate, andencrypted-blob/{oid}, but the post-release control only re-requestsencrypted-blobs. A regression that kept replicate/get at 404 after release would still pass. Please assert 200 (or the appropriate success shape) on all three afterset_repo_quarantine(..., false). -
[P3] Exercise mirror-admission row shape and dual-row CID deny in HTTP tests (test gap)
crates/gitlawb-node/src/test_support.rs:4233,:4290;crates/gitlawb-node/src/db/mod.rs:4623
The new integration tests quarantine viacreate_repo+set_repo_quarantineon a UUIDid. Production mirror admission usesupsert_mirror_repowith a slash-formid(owner/name) and setsquarantinedonly on first insert. The DB unit testlist_all_repos_excludes_quarantinedcovers the SQL filter for mirror rows only. No HTTP test drives the admission path or the canonical-quarantined / mirror-unquarantined coexistence case that the P1 CID finding above depends on.
Reviewer follow-up
beardthelion's CHANGES_REQUESTED review on head bcc7fb4 covers the withheld-paths gate, list_visibility gate, owner-caller tests, and partial release control. None of those are addressed on the current head. Reconciliation also surfaced the dual-row get_by_cid bypass above, which is in scope for this PR's CID serve change and comment but was not in that review.
Out of scope for this round
The following sibling surfaces were inspected and are real, but tracked or deferred separately and not counted as blockers on this PR: stale-public mirror CID serve (#124, ipfs.rs:55-57), ungated global list_pins / arweave anchors (#121), GraphQL/REST listing when a mirror row survives dedup, and owner mutations (merge/close/webhooks) on quarantined repos.
|
@beardthelion @jatmn addressed the review findings on this head: P1
P2 P3 Ready for another look. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/gitlawb-node/src/api/visibility.rs (1)
169-198: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDuplicate
list_visibility_rulesquery inlist_visibility.
authorize_repo_readalready fetches and returns the repo's visibility rules, but they're discarded via_rulesand then re-fetched identically on Line 180 (state.db.list_visibility_rules(&record.id)). Reuse the tuple's rules instead of querying twice.♻️ Proposed fix
- let (record, _rules) = + let (record, rules) = crate::api::authorize_repo_read(&state, &owner, &repo, Some(&auth.0), "/").await?; require_owner(&record, &auth.0)?; - let rules = state.db.list_visibility_rules(&record.id).await?; let rules_json: Vec<_> = rules🤖 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/api/visibility.rs` around lines 169 - 198, Reuse the visibility rules returned by authorize_repo_read in list_visibility instead of discarding them and calling state.db.list_visibility_rules again. Rename the tuple binding from _rules to rules and keep the existing serialization and response behavior unchanged.
🤖 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.
Inline comments:
In `@crates/gitlawb-node/src/api/ipfs.rs`:
- Around line 89-98: Restrict the CID quarantine filter around quarantined and
repo candidate matching to quarantined mirror rows, so a quarantined mirror
cannot suppress a healthy canonical repository with the same owner/name. Update
the `quarantined.iter().any(...)` logic and preserve consistency with
`authorize_repo_read`; alternatively, if retaining whole-logical-repo
suppression, align the related gated routes and comments/tests accordingly.
---
Nitpick comments:
In `@crates/gitlawb-node/src/api/visibility.rs`:
- Around line 169-198: Reuse the visibility rules returned by
authorize_repo_read in list_visibility instead of discarding them and calling
state.db.list_visibility_rules again. Rename the tuple binding from _rules to
rules and keep the existing serialization and response behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a209c5b1-c4ab-42c6-afea-05fd43666349
📒 Files selected for processing (5)
crates/gitlawb-node/src/api/encrypted.rscrates/gitlawb-node/src/api/ipfs.rscrates/gitlawb-node/src/api/visibility.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/test_support.rs
|
@coderabbitai addressed your review on
@beardthelion @jatmn FYI — this tightens the dual-row fold from the prior round without changing the quarantined-canonical + healthy-mirror deny case. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline crates/gitlawb-node/src/api/ipfs.rs --items all --type function --match 'get_by_cid'
ast-grep outline crates/gitlawb-node/src/api/visibility.rs --items all --type function --match 'list_visibility'
rg -n -C 5 --type rust \
'get_by_cid_serves_canonical_when_only_mirror_quarantined|list_quarantined_repos|list_visibility|list_visibility_rules|authorize_repo_read' \
crates/gitlawb-node/src/api/ipfs.rs \
crates/gitlawb-node/src/api/visibility.rs \
crates/gitlawb-node/src/test_support.rsLength of output: 15232 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '62,165p' crates/gitlawb-node/src/api/ipfs.rs
sed -n '4580,4655p' crates/gitlawb-node/src/test_support.rsLength of output: 7676
The original findings are addressed. 🐇 ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
GitHub state
- Mergeability:
MERGEABLE,mergeStateStatus: BLOCKED(my priorCHANGES_REQUESTEDonbcc7fb4, plus beardthelion's on the same head). - Checks on head
fb14bd2: onlyQuality-signal triageandCodeRabbitsucceeded.PR Checks(fmt + clippy,test (stable), MSRV, cargo audit, Docker smoke) isaction_requiredon this head and onb555276— the suite has not run since your reviewer-response commits. The PR body still ticks CItest (stable)with Postgres as done.
What checks out
Your production changes on this head address the core bypasses from my first round. I rechecked after pushing back on my own draft — the fold is not broken in production for the standard did:key dual-row case:
- Encrypted discovery/fetch/replicate,
withheld-paths, andlist_visibilityroute throughauthorize_repo_read, so a public-but-quarantined canonical row opaque-404s for anon, owner, and peers. list_all_reposexcludesquarantined = TRUErows, closing the single-row CID serve bypass.- The CID logical-repo fold (quarantined canonical only, via
!id.contains('/')+did_matches) matchesget_repopreference: a quarantined mirror no longer suppresses a healthy canonical (get_by_cid_serves_canonical_when_only_mirror_quarantined). - Fold verification: with mirror git seeded at the production acquire path (
/tmp/{bare_short}/dual-quar.git), the handler 404s with the fold present and servespublic byteswith the fold removed. The stock test passes in both cases because it never populates that path — a test-layout issue, not a production leak on this head. - CodeRabbit nit from
b555276is fixed:list_visibilityreuses rules fromauthorize_repo_read(no double fetch). - Locally, I ran
cargo test -p gitlawb-node quarantine— 20 tests, all green on headfb14bd2.
Findings
-
[P1]
get_by_cid_skips_mirror_twin_of_quarantined_canonicaldoes not exercise the logical-repo fold
crates/gitlawb-node/src/test_support.rs:4513-4578
This is the regression test for the dual-row CID bypass beardthelion and I blocked on. It seeds git at/tmp/{slug}/{name}.gitwhereslug = owner.replace([':', '/'], "_")(seed_cid_repos, line 2953), but the mirror twin row uses bareowner_did = short, sorepo_store::acquireresolves to/tmp/{short}/{name}.git(repo_store.rs:262-266) — a path the fixture never populates.upsert_mirror_repo'sdisk_pathis not consulted byget_by_cid. With the fold removed,object_typemisses on the mirror row and the handler 404s anyway, so the test passes vacuously. In production, sync stores mirror bare repos at{repos_dir}/{owner_short}/{name}.git(sync.rs:313-319), where objects exist; I confirmed the fold blocks serve when that path is populated and leaks when the fold is removed. Please align the fixture with production disk layout (e.g. clone to/tmp/{short}/dual-quar.git) so removing the fold makes this test go red. -
[P2] Required CI has not run on the reviewer-response heads
PR metadata / GitHub Actions
Your comment onfb14bd2and the PR test plan claim CItest (stable)with Postgres is done, butPR Checksisaction_requiredonfb14bd2andb555276(onlybcc7fb4has a completed greenPR Checksrun). Please getPR Checksgreen on the current head before merge; do not rely on the first-commit run for the two follow-up fix commits. -
[P3] Mirror-admission encrypted test still covers only list discovery
crates/gitlawb-node/src/test_support.rs:4318-4359
encrypted_blobs_quarantined_mirror_admission_opaque_404exercises onlyencrypted-blobs(anon + bare-key owner). beardthelion's P3 request also named replicate and get. Worth extending toencrypted-blobs/replicateandencrypted-blob/{oid}if you want full mirror-admission coverage; not a production gap on this head. -
[P3] Owner-caller regression still missing for encrypted replicate/get
crates/gitlawb-node/src/test_support.rs:4271-4288
The owner loop inencrypted_blobs_quarantined_repo_opaque_404only hitsencrypted-blobs. Replicate and get are anon-only in the deny loop. Low-risk test hole — production handlers share the sameauthorize_repo_readgate. -
[P3]
list_visibilityrelease path untested after quarantine clear
crates/gitlawb-node/src/test_support.rs:4436-4447
withheld_paths_and_list_visibility_quarantine_opaquere-checkswithheld-pathsafter release but notlist_visibility. Minor test completeness gap.
Follow-up on my prior review
My CHANGES_REQUESTED items on bcc7fb4 are addressed in production code on fb14bd2 for the did:key single-row and dual-row cases: withheld-paths, list_visibility, CID canonical fold, owner-caller coverage on encrypted list and CID serve, expanded release controls on list/replicate, and mirror-admission / dual-row HTTP tests. beardthelion's overlapping items land the same way.
What's still holding this head: the dual-row CID test is not load-bearing (P1 above — test quality, not a production bypass on fb14bd2), and PR Checks has not run on your fix commits (P2). The P3 items are incomplete test coverage from the prior round, not new production defects.
CodeRabbit findings on b555276 (canonical-only fold scope, list_visibility rules reuse) are fixed on fb14bd2.
I rechecked and am dropping these from this round (drift / pre-existing / out of scope):
- Truncated-owner /
did:gitlawbdual-row geometry viadid_matchesvsref_update_row_names_repo— pre-existingget_repoowner-key behavior; same semantics asauthorize_repo_read; not introduced or worsened by this diff. - Visibility
PUT/DELETEduring quarantine — pre-existing; I counted owner mutations on quarantined repos out of scope in my first review. - TOCTOU between
list_all_reposandlist_quarantined_repos, per-requestlist_quarantined_reposcost — same class as other non-transactional gates in the tree; not PR-specific actionable defects. list_visibility403 → 404 for authenticated non-owners — intentional opaque-404 posture from routing throughauthorize_repo_readbeforerequire_owner.
Out of scope for this round
Stale-public mirror CID serve (#124), ungated global list_pins / arweave anchors (#121), GraphQL/REST listing when a mirror row survives dedup, and owner mutations on quarantined repos outside visibility (merge/close/webhooks) remain separately tracked. I'm not counting them as blockers here unless this PR regresses them.
|
@jatmn @beardthelion addressed the latest review on head
CI (P2): fork PR Checks still need maintainer approval on this head — please approve when you can. |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
@jatmn @beardthelion on head Prior code/test findings from the dual-row fixture round remain addressed on Please re-approve / re-run |
There was a problem hiding this comment.
Prior-round production asks hold on 0cc91df (lock-only tip on c167be8). Encrypted list/get/replicate, withheld-paths, and list_visibility go through authorize_repo_read; list_all_repos drops quarantined rows; the canonical-only CID fold is load-bearing (removing it serves public bytes via the mirror twin; widening it to mirrors 404s a healthy canonical). Owner full-DID and bare-key denials still land. The dual-row fixture path fix from the last round is real.
What this round catches is a pin the fold made vacuous, and a sibling gap on the same /visibility path this PR partially gated.
Findings
-
[P2] Pin list_all_repos WHERE with a quarantined mirror-only CID HTTP test
crates/gitlawb-node/src/test_support.rs:4503
get_by_cid_skips_quarantined_public_repo seeds a quarantined canonical. After the dual-row fold landed, gutting WHERE quarantined = FALSE leaves that test green because the fold skips the row when it reappears in list_all_repos. The SQL filter is still the only drop for a quarantined public mirror with no canonical sibling: I seeded that shape, removed WHERE, and got 200 with public bytes; restoring WHERE 404s. list_all_repos_excludes_quarantined still reds on gut, but it never drives the HTTP serve path. Add a CID test for quarantined mirror-only (objects at /tmp/{short}/{name}.git) that fails when WHERE is removed. Keep the existing canonical and dual-row cases. -
[P2] Gate set_visibility and remove_visibility through quarantine before require_owner
crates/gitlawb-node/src/api/visibility.rs:82
This PR routes GET list_visibility through authorize_repo_read so a quarantined repo is opaque even to owner_did. PUT and DELETE on the same path still do bare get_repo + require_owner. I drove owner PUT during quarantine: 201 Created, while GET list_visibility on that row 404s. That discloses admission and lets the owner rewrite path_glob/reader_dids while reads are opaque. Same shape as list_visibility: authorize_repo_read first, then require_owner. Cover both verbs with an owner-caller quarantine deny (full DID and bare key). Also pin list_visibility's authorize_repo_read marker in the completeness fence (api/mod.rs still lists only require_owner for that handler).
One process note, not a finding: your 0cc91df lock bump matches open #292 (RUSTSEC-2026-0220 / ruint). After #292 merges, rebase and drop the parallel lock commit if it is still duplicated.
Not an ask, recorded only: get_by_cid's docstring still cites #124 as open; that issue closed via #141. The deny-body !contains("public bytes") asserts on CID 404s are framing debt (opaque envelopes never carry object bytes); the release 200 witnesses are the load-bearing half.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Run the required PR checks on the current head
GitHub Actions / current head0cc91dff2c83fc809948c83eb30b4c3d02a7256e
The current head has onlyQuality-signal triageand CodeRabbit statuses; the requiredPR Checksworkflow isaction_requiredand has no run for the reviewer-response commits or the lockfile update. The changed quarantine paths and their new regression fixtures therefore have not received the repository's required fmt/clippy/test/audit validation. Please obtain a greenPR Checksrun for this exact head before merging. -
[P2] Complete the quarantine posture across the
/visibilityroute
crates/gitlawb-node/src/api/visibility.rs:82
PUTandDELETEretain their pre-existingget_repoplusrequire_ownerflow, while this PR changesGET /visibilityto make a quarantined repository opaque even to its owner. A signed owner can therefore create or remove visibility rules and receive a successful response whileGETreturns 404. Complete the route's quarantine posture by checking before the owner gate on both mutations, with full-DID and bare-key denial tests that prove no write persists. -
[P2] Make the new CID regression test exercise the SQL quarantine filter end-to-end
crates/gitlawb-node/src/test_support.rs:4503
The new DB test coversWHERE quarantined = FALSE, butget_by_cid_skips_quarantined_public_repoquarantines a canonical row. If that predicate is removed, the new canonical-quarantine fold inget_by_cidstill skips the row, so the HTTP regression remains green and does not witness the SQL filter's serve-path effect. Add a quarantined mirror-only fixture with its object at the bare-owner acquisition path (/tmp/{short}/{repo}.git); that shape has no canonical row for the fold to discard and must 404 only because the SQL filter excludes it.
github-actions
Bot
removed
the
needs-tests
fe1fa00 to
7b60f4f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/gitlawb-node/src/db/mod.rs (1)
3664-3678: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftEnforce uniqueness for the normalized profile identity.
get_profiletreats a bare key anddid:key:<key>as the same profile, but the table is unique only on the rawdidvalue. Two concurrentupsert_profilecalls can both observe no row and insert both spellings.fetch_optionalthen has no deterministic winner, andset_profile_cidupdates every matching duplicate.Canonicalize profile inserts and add a normalized unique index after cleaning existing duplicates. Add a concurrent alias regression test.
Also applies to: 3724-3737, 3752-3763, 4897-4992
🤖 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 3664 - 3678, Update upsert_profile to canonicalize bare identities to the same normalized form used by get_profile before inserting, while preserving the stored identity used by the update path. Add a migration that cleans existing duplicate bare/did:key profiles deterministically and creates a unique index on the normalized profile identity. Ensure set_profile_cid targets the normalized identity without affecting unrelated profiles, and add a concurrent alias regression test covering both spellings.
🧹 Nitpick comments (1)
crates/gitlawb-node/src/db/mod.rs (1)
1680-1702: 🩺 Stability & Availability | 🔵 TrivialConfirm or remove the single-worker assumption.
Two workers can select the same pending row before either worker settles it. If multiple sync workers share this database, use row locking with
FOR UPDATE SKIP LOCKED, or enforce one worker at deployment.🤖 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 1680 - 1702, Resolve the concurrency assumption in dequeue_pending_syncs: either enforce and document single-worker deployment for a shared database, or change the batch-selection query to lock rows with FOR UPDATE SKIP LOCKED so concurrent workers cannot receive the same pending row. Preserve the existing ordering, limit, update, and returned SyncQueueItem behavior.
🤖 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.
Inline comments:
In `@crates/gitlawb-node/src/api/mod.rs`:
- Around line 241-243: Update the visibility authorization drift-check rows for
set_visibility, remove_visibility, and list_visibility to assert both
require_owner( and authorize_repo_read(. Retain the existing
authorize_repo_read( assertions and add a require_owner( entry for each handler
so every visibility operation remains covered by the owner gate.
In `@crates/gitlawb-node/src/test_support.rs`:
- Around line 4853-4886: Update the mirror setup in the test around
seed_cid_repos so it reuses the production repository path created by that
helper instead of deleting and cloning it. Remove the source/destination clone
setup and retain the guard only for any separately created temporary artifacts,
ensuring the mirror repository remains available for the subsequent CID
assertions.
---
Outside diff comments:
In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 3664-3678: Update upsert_profile to canonicalize bare identities
to the same normalized form used by get_profile before inserting, while
preserving the stored identity used by the update path. Add a migration that
cleans existing duplicate bare/did:key profiles deterministically and creates a
unique index on the normalized profile identity. Ensure set_profile_cid targets
the normalized identity without affecting unrelated profiles, and add a
concurrent alias regression test covering both spellings.
---
Nitpick comments:
In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 1680-1702: Resolve the concurrency assumption in
dequeue_pending_syncs: either enforce and document single-worker deployment for
a shared database, or change the batch-selection query to lock rows with FOR
UPDATE SKIP LOCKED so concurrent workers cannot receive the same pending row.
Preserve the existing ordering, limit, update, and returned SyncQueueItem
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d353a1e6-5b7f-4557-9e1b-6b922b162ec1
📒 Files selected for processing (5)
crates/gitlawb-node/src/api/ipfs.rscrates/gitlawb-node/src/api/mod.rscrates/gitlawb-node/src/api/visibility.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/test_support.rs
💤 Files with no reviewable changes (1)
- crates/gitlawb-node/src/api/ipfs.rs
4bba09d to
3303fb3
Compare
|
@beardthelion @jatmn — rebased onto current Your latest round (
Prior rounds (still on this head)
Note: the parallel Ready for another look. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Rebase this conflicting branch before merge
crates/gitlawb-node/src/db/mod.rs:6977
GitHub reports this head asCONFLICTING, and it is not descended from the PR base: its merge base is225644a, while the declared base is241b366. Resolving the merge currently conflicts in this file. The base-to-head diff also shows older versions of the signature and opaque-error code, but those are stale-base drift rather than changes authored by this branch. Rebase onto currentmain, resolve the conflict, and rerun the full review and validation on the resulting diff. -
[P2] Preserve the owner-gate check in the visibility completeness fence
crates/gitlawb-node/src/api/mod.rs:241
These rows now only assertauthorize_repo_read, while the separate assertion only proves that the helper itself usesdid_matches; it does not prove that each visibility handler callsrequire_owner. If that call is removed from GET, PUT, or DELETE, the guard stays green and any admitted reader can inspect or mutate visibility rules. Assert both the quarantine/read gate andrequire_owner(for each visibility handler. -
[P2] Bound the canonical-quarantine matching before the CID visit cap
crates/gitlawb-node/src/api/ipfs.rs:354
Every row returned by the unboundedlist_all_repos()scans every quarantined canonical row beforeipfs_max_repo_visitsor any deadline check can run. Thus an unauthenticated CID request performs O(servable repos × quarantined canonical repos) synchronous comparisons while holding the scarce walk permit, so a large mirror/quarantine set can starve the service despite the request budget. Precompute normalized(owner, name)canonical keys in aHashSet(or filter in SQL) and make the per-repo test O(1).
Stop strangers from overwriting a reserved assignee_did (and receiving the UCAN). Bind finish_task to the stored assignee; return 403 on reserved steals; keep exact stored DID forms; normalize blank assignees; pin production claim/finish SQL race guards with FOR UPDATE tests; trim broadcast by_did; and tighten blank/SQL re-check regressions.
Route encrypted and visibility handlers through authorize_repo_read; exclude quarantined rows from list_all_repos; fold canonical quarantine into get_by_cid (with main walk-budget/admission); gate visibility mutations; and add load-bearing HTTP regressions for mirror-only and dual-row CID paths.
3303fb3 to
896aff0
Compare
|
@beardthelion @jatmn — rebased onto current Conflict: took main's peer-writer LEDGER wording (duplicate add). Your latest round:
Prior quarantine gates, dual-row fold, visibility PUT/DELETE, and HTTP regressions remain on this head. Ready for re-review when CI is green. |
Windows CI can race: git fetch connects before the shim accept loop is polling, yielding connection aborted (os error 10053) with 0 POSTs. Probe the listener after spawn so multi-round and withheld-path tests start only once the shim is ready.
|
Pushed All other required checks were already green on |
…rors Windows CI can still abort localhost connections (os error 10053) after the shim is ready. Retry fetch_with_helper up to three times on transient connection failures before asserting.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The quarantine change itself is in good shape, and both asks from the last round have landed. What blocks this round is what else the branch now carries: three of its four commits are not this PR's subject.
Findings
-
[P1] Drop the assignee and Windows commits from this branch
crates/gitlawb-node/src/api/tasks.rs:1
e5a84688'scrates/tree is byte-identical to #275's head1676eb0e, and #275 is open with changes requested.95445594and013bd856are a second attempt at the Windows shim race that #312 already fixes at the root, in the same file. Merging #276 as it stands lands all three without their own reviews finishing, and it would make #312 conflict for no reason. Rebase down to896aff0balone. -
[P1] Remove the committed Cursor rule
.cursor/rules/rtk-token-savings.mdc:1
The file's entire content is/Users/ayushkumar/.cursor/rules/rtk-token-savings.mdc, an absolute path from your machine. It rode in one5a84688, so it needs removing from #275 too. It also breaks checkout for anyone whose working tree keeps its own ignored.cursor/directory, since git writes straight through it. -
[P2] The Windows retry lets
posts >= 2pass without multi-round negotiation
crates/git-remote-gitlawb/tests/real_git_fetch.rs:813
shim.postsis created once instart_shimand counts cumulatively, andfetch_with_helpernow retries up to three times against that same shim. Two aborted single-POST attempts therefore satisfy theposts >= 2lower bound, which is the one assertion proving the bridging path was exercised at all. The deadlock signature is still caught, since a timeout returnscompleted == falseand the transient predicate does not fire on it, so this is narrower than it looks. Whatever shape the fix takes, the count the assertion reads has to come from the attempt that actually succeeded. This goes away if the commits move to #312, which is why it sits under the ask above rather than beside it. -
[P3] Delete the unreachable arm in
quarantine_owner_keyand pin its equivalence
crates/gitlawb-node/src/api/ipfs.rs:97
The second and third match arms return the same value, so the middle guard never changes the result. The more useful half: the fold's correctness now rests on this key agreeing withdid_matchesfor every owner spelling, and nothing tests that. I ran it over 289 pairs includingdid:web/did:gitlawbcollisions and nesteddid:key:did:key:with no disagreement, but that belongs in a unit test beside the helper.
What checks out on 896aff0b: I gutted each of the four production lines one at a time and watched the matching test fail, then restored. The WHERE quarantined = FALSE filter, the CID canonical fold, the encrypted-blob gate and the set_visibility gate are all load-bearing. The completeness fence now pins both markers per visibility handler, and removing the owner gate from set_visibility turns it red. The fold is a hash lookup instead of a scan per candidate. list_all_repos has exactly one production caller, so the new WHERE cannot over-withhold on a listing surface, and authorize_repo_read is a strict superset of the check it replaced in encrypted.rs and visibility.rs, so the tightening can only withhold quarantined repos. CodeRabbit's test_support.rs:4877 note is a false positive: slug and short resolve to different directories and the test passes.
Superseded by the review on 013bd85.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Drop the unrelated assignee and Windows commits from this branch
crates/gitlawb-node/src/api/tasks.rs:1
The quarantine/CID implementation is896aff0b, but the current base-to-head range also includese5a8468's task-claim behavior and9544559/013bd856's Windows fetch-test changes. Those are separate subjects with open reviews (#275 and #312), so this is current branch-scope drift rather than a change required by the quarantine fix. Rebase this branch down to the quarantine work and keep the other changes in their own reviews. -
[P1] Remove the committed author-local Cursor rule
.cursor/rules/rtk-token-savings.mdc:1
The path is a tracked symlink (120000) whose complete target is/Users/ayushkumar/.cursor/rules/rtk-token-savings.mdc. It is dangling in a fresh checkout and can conflict with a contributor's own tooling directory, so it cannot provide a usable repository rule. Remove it from this branch (and from the task branch that introduced it) or replace it with actual repository-owned content. -
[P2] Keep the successful fetch's POST count separate from retried attempts
crates/git-remote-gitlawb/tests/real_git_fetch.rs:262
fetch_with_helpernow retries against the sameShim, butshim.postsis cumulative. Two transient attempts that each reach one POST, followed by a successful one-POST fetch, satisfy theposts >= 2assertion at the multi-round test even though the successful attempt never exercised multi-round negotiation. Reset or snapshot the counter for each attempt and assert the delta for the successful attempt; alternatively, move this unrelated retry change out with the other Windows work.
3 participants
Summary
list/get/replicate) usedvisibility_checkalone after a bareget_repo, skipping the quarantine short-circuit inauthorize_repo_read. A public-but-quarantined mirror still exposed its encrypted blob index (and would serve envelopes).GET /ipfs/{cid}iterateslist_all_repos(), which previously had noquarantined = FALSEfilter (unlike listing/dedup paths). Visibility alone admitted quarantined public rows, so objects remained reachable by CID.authorize_repo_read;list_all_reposexcludes quarantined rows (its only caller is the CID serve path).Why (direct PR)
Real quarantine invariant break on two surfaces. Not covered by existing issues/PRs (#110/#126 are visibility/CID gating; #135 is withheld-subtree trees — different).
Test plan
encrypted_blobs_quarantined_repo_opaque_404— discovery + replicate + get 404 while quarantined; no CID/oid leak; released repo lists againget_by_cid_skips_quarantined_public_repo— quarantined public repo 404s CID; released serves contentlist_all_repos_excludes_quarantined— DB enumeration skips sick mirrors-D warningscleantest (stable)with PostgresSummary by CodeRabbit
Bug Fixes
Tests