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 5 commits into
Conversation
Route encrypted discovery/fetch/replicate through authorize_repo_read so
quarantined mirrors are opaque 404s. Exclude quarantined rows from
list_all_repos so GET /ipfs/{cid} cannot serve their objects.
|
Warning Review limit reached
Next review available in: 59 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. 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 ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRepository quarantine is now enforced consistently 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 logical 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 repo 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 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 | ✅ 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 |
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.
Gate withheld-paths and list_visibility through authorize_repo_read. Fold logical-repo quarantine into get_by_cid so a public mirror twin cannot serve while its canonical is quarantined. Expand owner, release, mirror-admission, and dual-row coverage.
|
@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
Only drop CID candidates when a quarantined canonical (UUID) twin exists, so a quarantined mirror cannot suppress a healthy canonical. Reuse authorize_repo_read rules in list_visibility (no double fetch).
|
@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.
Clone mirror objects to /tmp/{short}/dual-quar.git so get_by_cid acquire
matches production layout; extend encrypted mirror + owner-caller coverage
to replicate/get; assert list_visibility after quarantine release.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@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.
Unblocks cargo audit on the quarantine/CID PR head (advisory requires >=1.20.0; previous lock pin was 1.17.2). Co-authored-by: Cursor <cursoragent@cursor.com>
|
@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.
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